Skip to content
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

스탬프 추가 기능 구현 #65

Merged
merged 31 commits into from
Feb 18, 2024
Merged

스탬프 추가 기능 구현 #65

merged 31 commits into from
Feb 18, 2024

Conversation

jhsseonn
Copy link
Member

2/17
실수로 기존 feature/44 브랜치를 삭제해버렸습니다... 그래서 pr 새로 올리게 되었습니다. 죄송합니다..
작업한 수정내용입니다.

[작업 내용]

  • goalTeam fetch join 해 골 검색하는 쿼리문 추가
  • 골 도메인에서 스탬프 리스트 삭제
    해당 부분 이전 pr에서 코멘트 남겼으나 확인하시기 복잡할 것 같아 여기에 같은 내용으로 남깁니다.
    처음에 단일 골 조회에서 스탬프까지 모두 조회해오려고 추가를 했는데 이후 골 정보 조회와 스탬프 조회를 서로 다른 api로 통신하는 것으로 변경하게 되면서 필요가 없어진 것 같습니다. 그리고 1:n의 경우 한 개까지만 fetch join이 가능한지는 몰랐네요...! 앞으로 참고하도록 하겠습니다. 해당 부분은 삭제하는 것으로 수정하겠습니다!

2/11
스탬프 추가 기능 구현 완료했습니다.
조회 기능까지 한꺼번에 올리려다 이슈를 스탬프 추가로만 파두어서 추가 먼저 올리고 조회는 따로 올리도록 하겠습니다!
확인하시고 리뷰 부탁드립니다!

[작업내용]

  • 새로운 스탬프 생성
  • 과거 스탬프도 생성 가능하도록 미래일 경우에만 예외처리
  • closed [Feat] 스탬프 추가 #44

스탬프 테이블이 제대로 생성되지 않는 문제 때문에 쓸데없이 시간을 많이 잡아먹었습니다...
문제 원인은 스탬프 필드명 중 날짜 필드명을 day로만 설정했는데 sql에서는 예약어가 존재했는지 sql에서 SyntaxError가 발생해 day 필드명을 sql에서는 stamp_day로 name 따로 설정해주니 테이블 정상적으로 잘 생성되었습니다. 이렇게 또 삽질로 배워가네요..

@jhsseonn jhsseonn added the feature 기능 추가 label Feb 16, 2024
@jhsseonn jhsseonn self-assigned this Feb 16, 2024
@jhsseonn jhsseonn changed the title Feature/44 스탬프 추가 기능 구현 Feb 16, 2024
Copy link
Member

@JJ503 JJ503 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생 많으셨습니다!
그런데 스탬프 추가에 대한 resonse에 대해 의문이 드는 부분이 있어 rc 해두었습니다.
확인해 주시면 감사하겠습니다!

또한, 두 개 이상의 컬렉션을 fetch join 했을 때의 문제점에 대한 추가 설명을 지난 pr 코멘트에 대해 적어보았습니다. 해당 부분도 참고해주시면 좋을 것 같습니다!

Comment on lines +40 to +41
JOIN FETCH g.teams.goalTeams gt
JOIN FETCH gt.user gtu
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment on lines 5 to 9
public record ReadStampDto(
Long goalId,
int day,
String message
) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

스탬프 아이디는 굳이 넘겨줄 필요가 없을까요?
오히려 골 아이디를 넘겨줘야 하는가에 대한 의문도 있습니다.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가로 사용자 정보는 넘길 필요가 없는지 궁금합니다!
저희가 아직 이미지를 신경 쓰지 않기로 하긴 했지만, 이미지가 없는 경우에는 사용자의 테마 색을 통해 기본 이미지로 만들기로 했던 걸로 기억해서요! (지난 기획 때)
또한, 스탬프 아래 메시지와 함께 사용자 아림이 떴던 걸로 기억하는 데 아닐까요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사용자 이름은 제가 빼먹었네요...! 추가하도록 하겠습니다.
골 아이디는 스탬프가 업로드 될 골 아이디가 필요하다고 생각해 넣었습니다만 응답을 받아 화면에 보여줄 때 골 아이디는 굳이 필요하지 않을수도 있겠네요! 오히려 스탬프 식별에는 스탬프 아이디가 더 맞을 것 같네요. 스탬프 아이디로 수정하도록 하겠습니다.
스탬프 이미지를 추가하지 않을 경우 저는 이미지 없이 사용자 이름과 메시지만 올라간다고 생각했습니다..! 안드로이드로 작업할 때 스탬프 담당이 아니었다보니 기억이 잘 나지 않았네요... 사용자 색상으로 기본 이미지를 생성해야할 경우 사용자 프로필 색상도 함께 넘겨주는 것이 좋겠네요!
그러면 골 아이디를 빼고 스탬프 아이디와 사용자 이름과 프로필 색상을 함께 넘겨주는 것으로 하겠습니다!

Comment on lines 66 to 67
this.day = validateDay(goal, day);
this.message = validateMessage(message);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

daymessage를 vo로 만드는 것에 대해 어떻게 생각하시나요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋네요! day와 message 모두 vo 반영해보겠습니다!

Copy link
Member

@JJ503 JJ503 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확인 완료했습니다. 리뷰 반영하시느라 고생 많으셨습니다!
아주 간단한 수정 사항이 있어, 해당 부분은 코멘트로 작성해 두었으니 확인해 주시면 감사하겠습니다.
간단한 사항이라 이만 approve 합니다~

Comment on lines 30 to 44
private int validateDay(final Goal goal, final int day) {
final long nowStampDay = ChronoUnit.DAYS.between(goal.getGoalTerm().getStartDate(), LocalDate.now()) + 1;

if (day > nowStampDay) {
throw new InvalidStampException.InvalidStampDayFuture();
}
if (day > goal.getGoalTerm().getDays()) {
throw new InvalidStampException.InvalidStampDay();
}
if (goal.getGoalTerm().getStartDate().isAfter(LocalDate.now())) {
throw new InvalidStampException.InvalidStampDay();
}

return day;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

별건 아니고, GoalTerm만 사용할 거면, goal이 아닌 goalTerm을 넘겨줬어도 좋을 것 같아요!
중요한 부분은 아니라 효선님 편하신대로 하면 될 것 같습니다!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그렇네요! 수정해서 올리도록 하겠습니다!

Comment on lines 30 to 31
private static final int STAMP_DAY_MINIMUM = 1;
private static final int STAMP_MESSAGE_MAXIMUM = 30;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 클래스에서는 더 이상 사용하지 않으니 없어져도 괜찮겠네요!

Copy link

sonarcloud bot commented Feb 18, 2024

@jhsseonn jhsseonn merged commit c77c5ae into develop Feb 18, 2024
2 checks passed
@jhsseonn jhsseonn deleted the feature/44 branch February 18, 2024 04:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 기능 추가
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Feat] 스탬프 추가
2 participants