-
Notifications
You must be signed in to change notification settings - Fork 0
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
refactor: 승인된 신청자 수 필드 추가 #464
Conversation
|
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.
고생하셨습니다 민규님!
main/src/main/java/org/sopt/makers/crew/main/global/dto/MeetingResponseDto.java
Show resolved
Hide resolved
&& meeting.getCanJoinOnlyActiveGeneration(); | ||
|
||
return new MeetingResponseDto(meeting.getId(), meeting.getTitle(), | ||
meeting.getTargetActiveGeneration(), meeting.getJoinableParts(), meeting.getCategory(), | ||
meeting.getTargetActiveGeneration(), meeting.getJoinableParts(), meeting.getCategory().getValue(), |
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.
왜 Category Getter에는 getValue() 옵션이 추가되었을까요?
기존에는 없었어서 여쭈어봅니다.
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.
저게 enum이 아니라 "스터디" 이런식으로 줘야해서 그랬던 거 같습니다!
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.
아하 확인했습니다!
public LocalDateTime getmStartDate() {
return mStartDate;
}
public LocalDateTime getmEndDate() {
return mEndDate;
}
해당 getter도 사용하지 않는 것 같은데 삭제하는게 어떨까요?
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.
이렇게 하지 않으면 스웨거 상에서 앞에 "m" 글자가 사라지더라고요..
👩💻 Contents
예전에 말씀드린 작업진행했습니다!
설명을 드리면, 모임
신청 수
가 아니라승인된 신청 수
를 반환해야 했습니다! 하지만 제가 착각을 해서 옛날에신청 수
로 구현했었습니다! 그래서 최근에신청 수
가 아닌승인된 신청 수
를 반환하도록 로직을 변경했습니다. 하지만 변수명은 아직appliedCount (신청 수)
로 되어있습니다. 이게 자꾸 헷갈리게 만든다고 생각해서 개선하고자 했습니다!원래는 meeting/v2/patch-1 이런식으로 새로운 API를 만들려고 했는데요!
그런 방식은 필드 하나 바꾸는 거에 비해 너무 수정 범위가 크다고 생각했습니다!
간단하게 해결할 방법 고민하다가 그냥 필드를 추가한 후에, 나중에 마이그레이션 끝나면 해당 필드 삭제 하는게 가장 효율적일 것 같아 그렇게 구현했습니다!
📝 Review Note
📣 Related Issue
✅ 점검사항