-
Notifications
You must be signed in to change notification settings - Fork 0
feat: 마이페이지 리팩토링 #4
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
Conversation
Ho-Tea
left a comment
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.
번거로운 작업이였을텐데 고생하셨습니다!!
사소한 코멘트 몇개 달아드렸으니 확인 부탁드립니다 🙇♂️
급한건 아니지만 저희끼리 한번 얘기해볼만한 것들 아래에 달아놓을게요!!
Adapter에서Optional로service에 올리는 경우와 객체로 변환해서 올리는 경우를 구분해야 할 것 같은데 (서로 다른adapter에서 올라온 것들에 대해 통합적으로 검증해야하는 경우 ->Optional로service까지 올림, 각adapter에서 처리할 수 있는 검증은adpater에서 처리) 는 어떨까요!- 테스트 작성해주신것 너무 좋아요 👍👍👍 나중에 시간될때 저희가 앱잼하면서 놓쳤던 테스트 보강해도 좋을것같아용
- Controller <-> Service <-> Repostiroy 에서 어떤경우에는 Long을 사용하고 어떤 경우에는 long을 사용하기에 일관성을 맞추는 방향으로 얘기해도 좋을것같아요!
| return ResponseEntity.ok(MemberResponse.from(memberService.findById(memberId))); | ||
| } | ||
|
|
||
| @PatchMapping("/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.
사소하기는하지만.. 내부적으로 모든 필드에 재대입이 들어가기때문에 Put은 어떨까용
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.
저도 이게 참 애매했는데,
아래와 같은 생각으로 patch로 정했습니당.
해당 api는 이름, 전화번호, 닉네임, 이미지url을 요청으로 받습니다.
그런데 저희 테이블에는 이름, 전화번호, 닉네임, 이미지url 이외에 role, oauth관련 정보들도 포함하고 있습니다.
그래서 업데이트할 때 리소스(멤버)의 모든 컬럼을 변경하지 않기에, 리소스 전체를 업데이트한다는 put보다는 patch가 어울린다고 생각했습니다!
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.
저희 테이블에는 이름, 전화번호, 닉네임, 이미지url 이외에 role, oauth관련 정보들도 포함하고 있습니다.
제가 이 부분을 놓쳤네요. 충분히 납득되었습니다 👍👍👍
| lesson.getRepresentativeImageUrl(), | ||
| lesson.getGenre(), | ||
| lesson.getLevel(), | ||
| -1 * (int) LocalDateTime.now().until(lesson.getStartTime(), java.time.temporal.ChronoUnit.DAYS)); |
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.
매직넘버는 상수화해도 좋을 것 같아요!
| void updateRole(Long id, Role role); | ||
|
|
||
| @Query("select m.nickname from MemberJpaEntity m where m.id = :memberId") | ||
| Optional<String> findNickNameById(long memberId); |
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.
단순 쿼리메서드를 통해서도 가능할 것 같아요!
Optional<String> findNicknameById(Long id);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.
바로~ 반영하겠슴다
| Assertions.assertThat(myTeacherProfileResult.profileImage()).isEqualTo("www.example.com/teacher1.png"); | ||
| Assertions.assertThat(myTeacherProfileResult.nickname()).isEqualTo("nickname"); | ||
| Assertions.assertThat(myTeacherProfileResult.instagram()).isEqualTo("@hong_dancer1"); | ||
| Assertions.assertThat(myTeacherProfileResult.youtube()).isEqualTo("youtube.com/hong_dancer1"); |
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.
assertAll로 묶을 수 있겠네요!
assertAll(
() -> assertThat(myTeacherProfileResult.profileImage()).isEqualTo("www.example.com/teacher1.png"),
() -> ...
)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.
역시 테스트 코드왕
| .detail("경력 10년의 힙합 댄서") | ||
| .educations(List.of("한국예술대학교 댄스학과")) | ||
| .experiences(List.of("다양한 공연 및 강의 경험")) | ||
| .prizes(List.of("앱잼 1등")) |
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.
ㅋㅋㅋㅋㅋㅋㅋ큐ㅠㅠㅠㅠㅠㅠㅠㅠㅠㅠㅠㅠㅠㅠㅠ
src/test/java/be/dash/dashserver/core/domain/reservation/service/ReservationServiceTest.java
Outdated
Show resolved
Hide resolved
Test Results27 files 27 suites 4s ⏱️ Results for commit 283d062. |
Ho-Tea
left a comment
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.
고생하셨습니다!!!!
Related Issue 📌
close #3
Description ✔️
변경된 ui에 따라 멤버 정보 반환 api를 변경했습니다.
변경된 ui에 따라 선생님 정보 반환 api를 생성했습니다.
멤버 정보 수정 api를 생성했습니다.
수업 수강 수 api를 생성했습니다.
teacher에 prize column을 추강했습니다.
teacherImageJpaEntity에서 식별자만 가지도록 변경했슴다.
To Reviewers
pr이 생각보다 커져서..
선생 정보 업데이트는 다른 pr로 올리겠습니당