-
Notifications
You must be signed in to change notification settings - Fork 1
[FEATURE] 유저 회원가입 추가 #303
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
[FEATURE] 유저 회원가입 추가 #303
Conversation
- 기존 쿠키 방식에서 수정됨으로 해당 로직의 변경이 필요함, 협의 필요.
- 명명 일관성 유지
- 당장 삭제하면 문제 발생할 것 같아서 우선 구현 후 삭제 예정.
- 유저로 책임이 넘어갔음.
- new suffix 는 삭제 예정 - 봉사자와 기관에 겹치는 속성인 연락처는 공통 속성에 추가.
- 저장 기능 추가
- center repository 구현체 수정
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.
빌드에 실패했습니다.
import 문제 |
1 similar comment
import 문제 |
| private UUID userId; | ||
|
|
||
| @Column(name = "name", nullable = false) | ||
| private String name; |
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.
name은 기관과 봉사자가 각자 관리하는건가요?
기존에는 공통속성에 들어있던 부분이라 여쭤봤습니다!
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.
그렇네요.. erd 상에서는 현재 구분되어있습니다. name을 공통 속성으로 옮기는 방법이 더 일관성 있다고 생각해서 코드와 erd 모두 수정하겠습니다!
|
고생하셨습니다 작업 해놓으신 부분 따라서 리팩토링 진행해보겠습니다 감사합니다! 개행 추가나 어노테이션 제거가 필요한 부분이 있긴하지만 각자의 리팩토링 pr때 자세히 보고 다시 한번 고생하셨습니다 |
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 class NEWCenter extends BaseEntity { | ||
|
|
||
| public static final String DEFAULT_NAME = "기관"; | ||
| @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.
public으로 열여둔 이유가 있으신가요? 다른데에서 사용되지 않으면 private가 좋아보입니당
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 NEWCenter register(UUID userId) { | ||
| NEWCenter center = NEWCenter.createDefault(userId); | ||
| return NEWCenterRepository.save(center); | ||
| } |
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.
NEWCenter 엔티티 클래스를 반환하는 이유가 혹시 있으신가요? 없다면 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.
리뷰보고 고민하다가 좋은 인사이트를 얻었네요..
리포지토리 계층에서는 center를 반환한다고 해도 서비스에서 이를 굳이 반환하지 않고, 끊어내면 된다는 것을 인지하지 못했던 것 같습니다. 감사합니다~
| UUID userId) { | ||
|
|
||
| tokenManager.removeRefreshToken(userId.toString()); | ||
| } |
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.
HttpServletResponse response
signOut 메서드에서 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.
기존에는 쿠키를 사용해야해서 있었는데, 이후 로직에서 쿠키를 전혀 사용하지 않는다면 취소할 것 같습니다!
현재 어떻게 액세스 토큰을 리프레시할지 상의도 못해봐서 고민이 되네용..
| public class NEWVolunteer extends BaseEntity { | ||
|
|
||
| public static final String DEFAULT_NAME = "봉사자"; | ||
| @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.
요기도 public인 이유가 있나요?
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.
없습니다! 위에서 리뷰받은 후에 코드를 보니, 엔티티의 필드와 같은 레벨에 상수가 존재하는 부분이 지저분해서 확실히 수정했습니다!
|
|
||
| @Override | ||
| public NEWVolunteer register(UUID userId) { | ||
| NEWVolunteer volunteer = NEWVolunteer.createDefault(userId); |
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.
NEWVolunteer 여기도 필요한 경우가 아니라면 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.
넵 ㅎㅎ
| } | ||
|
|
||
| NEWRegisterCenterUseCase.register(userId); | ||
| } |
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.
맞습니다. 리포지토리에서 반환된 값을 사용해야한다고 생각했던 것 같아요!
ayoung-dev
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.
너무.. 고생하셨습니다..
자잘한 것들도 다 일관되도록 수정하셨군요
리팩토링 잘해보겠습니당
| private UserCommonAttribute(UUID userId, String nickname, String imgUrl, String introduce, boolean isCustomized) { | ||
| private UserCommonAttribute(UUID userId, String nickname, String contactNumber, String imgUrl, String introduce, boolean isCustomized) { | ||
| this.userId = userId; | ||
| this.nickname = nickname; |
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.
이게 봉사자의 닉네임, 센터의 이름이 되는 게 맞는 거죠?
그렇다면 volunteer와 center에 있는 name은 빠져야하는 거 아닐까욤
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.
현재 닉네임과 네임이 따로 있습니다!
그래서
- 공통 속성의 nickname을 봉사자에게만 제공할까?
- 공통 속성에 name은 포함시켜야겠다.
(봉사자와 기관의 name은 공통 속성으로 옮겨야겠다.)
이렇게 생각 중입니다!
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.
제가 erd만 보다가 봉사자도 이름이 필요하다는 걸 놓쳤네요!
확인했습니당
- 유저 공통 속성에 name 추가 (봉사자와 기관에서 삭제). - 봉사자에 nickname 추가.
|




resolved :
📌 과제 설명
회원가입을 만들기 위해서 기존 로직들을 적절히 리팩토링한 후, 작업을 시작했습니다.
기존 봉사자와 기존 기관의 로직에서 문제가 생기지 않도록 새로운 봉사자와 기관을 구현했습니다.
이를 기준으로 각자 맡으신 도메인의 수정을 부탁드리겠습니다!
👩💻 요구 사항과 구현 내용
✅ PR 포인트 & 궁금한 점
rollback 커밋들은 기존 봉사자와 기관의 로직을 삭제하던 중, 이를 취소했던 커밋입니다. 너무 일이 커지는 것 같아서 되돌리고 새로운 봉사자와 기관을 구현하는 것으로 방향을 변경했습니다.