-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
모임 상태 및 찜, 참여 기능 추가 #171
모임 상태 및 찜, 참여 기능 추가 #171
Conversation
- 특정 모임에 대한 참여자를 명시하기 위해 ByMoim 추가
# Conflicts: # backend/src/main/java/mouda/backend/moim/service/MoimService.java
# Conflicts: # backend/src/main/java/mouda/backend/moim/service/MoimService.java
# Conflicts: # backend/src/main/java/mouda/backend/moim/service/MoimService.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
간단한 리뷰 남겼어요 😀
러프하게 확인하고 반영할 필요가 있는 것만 반영 부탁드려요!
수고했습니다 👍
private final ChamyoService chamyoService; | ||
|
||
@Override | ||
@GetMapping("/me") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 로그인한 사용자의 권한을 불러오는 거군요 .. ! /chamyo/me
보다는 chamyo/role
는 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 로그인한 사용자의 권한을 불러오는 거군요 .. !
/chamyo/me
보다는chamyo/role
는 어떨까요?
프론트 처리를 위해 role을 반환하도록 했지만 사실상 참여 여부 조회
에 사용되고 있습니다 ㅎㅎ 어찌됐든 Role에 미참여자도 포함된 만큼 상의 후 처리해볼게용!
|
||
@Override | ||
@GetMapping("/all") | ||
public ResponseEntity<RestResponse<ChamyoFindAllResponses>> findAllChamyoByMoim(@RequestParam Long moimId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moimId
가 없으면 안되니 NULL 체크가 필요하겠네요
@RequestParam
은 NULL이 입력되었을 때 어떻게 처리되나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moimId
가 없으면 안되니 NULL 체크가 필요하겠네요@RequestParam
은 NULL이 입력되었을 때 어떻게 처리되나요?
기본값이 required=true라서 없으면 MissingServletRequestParameterException
에러가 터지긴 합니다 ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오호 😀 그럼 핸들링해서 400 에러를 보내는 과정도 필요하겠네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저희 dto 패키지를 request/response 로 한번 더 나누기로 했었던 걸로 기억해요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저희 dto 패키지를 request/response 로 한번 더 나누기로 했었던 걸로 기억해요!
굿! 전체적으로 확인하고 수정해둘게요
MOIM_NOT_FOUND("참여하려는 모임이 존재하지 않습니다."), | ||
MOIM_ALREADY_JOINED("이미 참여한 모임입니다."), | ||
MOIM_FULL("최대 인원 수를 초과했습니다."), | ||
MOIMING_CANCLED("취소된 모임입니다."), | ||
MOIMING_COMPLETE("모집이 완료된 모임입니다."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MOIM이라는 접두사랑 MOIMING이라는 접두사는 무슨 차이가 있는 지 궁금합니다 😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MOIM이라는 접두사랑 MOIMING이라는 접두사는 무슨 차이가 있는 지 궁금합니다 😀
MOIM은 모임 그 자체를, MOIMING은 모집 중이라는 행위를 표현하려는 의도였어요 ㅎㅎ
MOIM_COMPLETE라고 하면 이게 모임 자체가 끝난건지, 사람을 더 안받는건지 애매하니깐요!
return new ChamyoFindAllResponses(responses); | ||
} | ||
|
||
public void chamyoMoim(MoimChamyoRequest request, Member member) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
주석은 혹시 일부러 하신 건가요 ? 아니면 실수로 남겨놓은 건가요? 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
주석은 혹시 일부러 하신 건가요 ? 아니면 실수로 남겨놓은 건가요? 😃
ㅋㅋㅋㅋ 수정할게요..ㅋㅋㅋㅋㅋㅋ 어제 마지막 부분만 못하고 갔거든요
// 현재 참여로 인해 모임 인원이 꽉 찬 경우 모임 상태를 변경 | ||
updateMoimStatus(moim); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
모임 상태를 어떻게 변경했는 지 여기서 드러날 필요가 있지 않을까요? 😃 한번 더 타고 들어가야 하는 불편함이 생길 수 있겠네요 ㅠㅠ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
모임 상태를 어떻게 변경했는 지 여기서 드러날 필요가 있지 않을까요? 😃 한번 더 타고 들어가야 하는 불편함이 생길 수 있겠네요 ㅠㅠ
저는 합쳐도 상관없다는 생각이지만 메서드가 길다는 생각이 들 수도 있어서 분리한건데 다시 합쳐넣을게용~~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
엇 저는 메서드 분리는 좋지만 메서드 명에서 Complete
로 변경한다는 것을 표현하면 좋겠다는 의미였어요 👍👍
zzimRepository.findByMoimIdAndMemberId(request.moimId(), member.getId()) | ||
.ifPresentOrElse( | ||
zzimRepository::delete, | ||
() -> zzimRepository.save(Zzim.builder().moim(moim).member(member).build()) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ifPresentOrElse
의 의미가
- 이미 찜이 되어있으면 삭제한다.
- 찜이 되어있지 않으면 저장한다.
요거 맞나요?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ifPresentOrElse
의 의미가
- 이미 찜이 되어있으면 삭제한다.
- 찜이 되어있지 않으면 저장한다.
요거 맞나요?!
옙! 찜 버튼은 하나라서 하나의 API로 처리하려고 했어용
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드량이 엄청나네요!!! 고생하셨습니다 👍
@ManyToOne | ||
@JoinColumn(name = "moim_id") | ||
private Moim moim; | ||
|
||
@ManyToOne | ||
@JoinColumn(name = "member_id") | ||
private Member member; | ||
|
||
@Enumerated(EnumType.STRING) | ||
private MoimRole moimRole; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nullable = false 추가가 필요할 것 같아용
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nullable = false 추가가 필요할 것 같아용
추가완료 👍
|
||
@DisplayName("현재 로그인 된 회원은 아직 모임에 참여하지 않은 상태이다.") | ||
@Test | ||
void fail() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
실패를 기대하지는 않는 것 같은데 fail 보단 위처럼 findMoimRole_WhemMemberIsNonMoimee
이 어떨까요'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
실패를 기대하지는 않는 것 같은데 fail 보단 위처럼
findMoimRole_WhemMemberIsNonMoimee
이 어떨까요'?
예리하군.. 바로 수정했어요👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고했어요 😀
조금 리뷰 달아놓기는 했는데 빠른 작업을 위해 어푸르브 바로 드려요!
확인 후 가능하면 수정해주세요 ~
PR의 목적이 무엇인가요?
새로 추가된 모임 상태, 찜, 참여 기능의 추가
이슈 ID는 무엇인가요?
설명
질문 혹은 공유 사항 (Optional)
주의
빠른 기능 구현을 위해 놓친 부분이 있을 수 있습니다. 구현한 모든 기능에 대한 테스트는 완료하였으나 일부 컨벤션 및 디테일에 차이가 있을 수 있습니다.
기존 기능 수정
테스트는 삭제했지만, 기존 서비스 로직은 그대로 놔뒀어요!
모임 참여
찜