-
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
Feat #27 최초 회원가입 구현 #27
The head ref may contain hidden characters: "feat/#26/\uCD5C\uCD08-\uD68C\uC6D0\uAC00\uC785-\uAD6C\uD604"
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.
고생하셨습니다 !
전체적으로 깔끔한거같습니다 Reviewers 랑 Assignees, Labels 설정해주시면 더 좋을거같아요
@@ -18,18 +17,8 @@ | |||
@RequiredArgsConstructor | |||
public class MemberService { |
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.
닉네임 중복 처리는 좋은 것 같아욤 근데 버튼을 따로 두기 보다는 중복 닉인 경우엔 toast 메시지를 주는게 사용자 경험이 더 좋을 것 같아요. 버튼이 따로 들어가면 디자인이나 UI가 조금 안 어울릴 것 같아서!
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/main/java/com/gachtaxi/domain/members/controller/AuthController.java
Outdated
Show resolved
Hide resolved
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.
고생하셨습니다!
제 생각 몇 가지 적어 봤어용
Members members = memberRepository.findById(userId) | ||
.orElseThrow(MemberNotFoundException::new); | ||
|
||
members.updateEmail(email); | ||
} | ||
|
||
@Transactional | ||
public void updateMemberAgreement(MemberAgreementRequestDto dto, Long userId) { | ||
Members members = memberRepository.findById(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.
해당 메서드는 주영님 피알에 구현이 되어있는 걸로 알긴 하는데, 개별 메서드로 구현해서 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.
@@ -39,13 +28,30 @@ public InactiveMemberDto saveTmpMember(Long kakaoId){ | |||
} | |||
|
|||
@Transactional | |||
public void updateInactiveMemberOfEmail(String email, Long userId) { | |||
public void updateMemberEmail(String email, Long 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.
앗 그러네요!
이전 이슈에서 이메일 중복체크를 수행한 줄 알았는데 빠뜨렸네요..
큰일 날 뻔했네요!
인증코드를 이메일로 보내는 API에서 이메일 중복체크를 포함하도록 하겠습니다!
? LOGIN_SUCCESS | ||
: UN_REGISTER; | ||
return ApiResponse.response(HttpStatus.OK, OAUTH_STATUS.getMessage(), res); | ||
public ApiResponse<Void> kakaoLogin(@RequestBody @Valid KakaoAuthCode kakaoAuthCode, HttpServletResponse 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.
이전엔 body로 로그인 상태와 유저 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.
유저Id라는 엔티티 식별값을 ResponseBody로 계속 주고받는 방식보다는 백엔드에서 토큰으로 유저Id를 추출하는 게 더 좋다고 생각했습니다.
해당 내용(userId반환 안해도 괜찮은지)을 프론트 측과 이야기를 나눠봤는데, 프론트 측에서는 유저 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.
로그인 상태도 구분이 가능하다고 하셨나요??
가입된 상태: 액세스 토큰, 리프레시 토큰 반환 -> 홈으로 라우팅
미가입 상태: 임시 토큰 반환 -> 회원가입으로 라우팅
이전에 body로 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.
앗 제가 API 응답에서 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.
고생하셨습니다!!
…nto feat/#26/최초-회원가입-구현
📌 관련 이슈
관련 이슈 번호 #26
Close #26
🚀 작업 내용
약관 동의 정보 업데이트 API 구현
이용약관, 개인정보, 마케팅 수신 동의를 받아 임시 유저의 상태를 업데이트 합니다.
단순히 약관 동의 정보만 업데이트 합니다.
따라서 임시 토큰에서 추출한 id로 멤버 검증만 합니다.
사용자 추가 정보 업데이트 API 구현 + UserStatus ACTVIE로 업데이트
프로필사진, 닉네임, 실명, 학번, 성별과 UserStatus를 ACTIVE로 업데이트합니다.
사용자가 존재하는 지 검증을 거칩니다.
학번은 고유값이므로 검증 로직을 거칩니다.
토큰, 쿠키 응답을 Controller 단에 책임을 두도록 변경
기존 토큰과 쿠키를 심을 때, 비지니스 로직에 response를 넘겨주어 서비스단에서 함께 처리하도록 했습니다.
토큰과 쿠키를 응답하는건, 요청과 응답 책임을 가진 Controller에 명시적으로 작성하는 게 좋을 거 같다는 주영님의 리뷰를 반영해 수정했습니다.
@CookieValue가 던지는 예외
다음과 같이 @CookieValue를 적용했을 때 값이 없으면 MissingRequestCookieException을 던지게 되고 이는 GlobalExceptionHandler의 @ExceptionHandler(Exception.class)에서 처리됩니다.
Request에 쿠키가 없는 사용자 요청의 문제이지만, HttpStatus가 500으로 반환(이는 서버 내부 에러 코드)됩니다.
잘못된 의미의 HttpStatus 코드가 반환되고 있어 @ExceptionHandler(MissingRequestCookieException.class)로 따로 잡아 처리했습니다.
물론 @CookieValue(value = REFRESH_TOKEN_SUBJECT, required = false) 설정으로 refreshToken에 null을 허용한 다음
if로 커스텀 에러를 던질 수 있습니다.
refreshToken을 null로 받아서 if문으로 null 체크 후 커스텀 예외를 던지는 방식보다는
컨트롤러단에서 빠르게 @CookieValue로 예외를 처리하는 게 더 효과적이라고 생각했습니다.
이에 대한 리뷰어 들의 생각이 궁금합니다.
📸 스크린샷
사용자 약관 동의 업데이트 API
사용자 추가 정보 업데이트 API
토큰 재발급 실패
📢 리뷰 요구사항
@CookieValue 관련 예외 처리 방식