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

Feat #27 최초 회원가입 구현 #27

Merged
merged 23 commits into from
Jan 14, 2025

Conversation

koreaioi
Copy link
Member

📌 관련 이슈

관련 이슈 번호 #26
Close #26

🚀 작업 내용

PR에서 작업한 내용을 설명

  • 약관 동의 정보 업데이트 API 구현
  • 사용자 추가 정보 업데이트 API 구현 + UserStatus ACTIVE로 업데이트
  • 여러곳에 중복 정의된 토큰 상수 ex ACCESS_TOKEN_SUBJECT, REFRESH_TOKEN_SUBJECT 중복 제거 후 재사용
  • 토큰, 쿠키 응답을 Controller 단에 책임을 두도록 변경 (주영님 리뷰 반영)
  • @CookieValue를 적용해 컨트롤러 단에서 RefreshToken 추출 (강혁님 리뷰 반영)
  • GachtaxiApplication상단 @EnableJpaAuditing 적용

약관 동의 정보 업데이트 API 구현

이용약관, 개인정보, 마케팅 수신 동의를 받아 임시 유저의 상태를 업데이트 합니다.
단순히 약관 동의 정보만 업데이트 합니다.
따라서 임시 토큰에서 추출한 id로 멤버 검증만 합니다.

사용자 추가 정보 업데이트 API 구현 + UserStatus ACTVIE로 업데이트

프로필사진, 닉네임, 실명, 학번, 성별과 UserStatus를 ACTIVE로 업데이트합니다.

사용자가 존재하는 지 검증을 거칩니다.
학번은 고유값이므로 검증 로직을 거칩니다.

토큰, 쿠키 응답을 Controller 단에 책임을 두도록 변경

기존 토큰과 쿠키를 심을 때, 비지니스 로직에 response를 넘겨주어 서비스단에서 함께 처리하도록 했습니다.
토큰과 쿠키를 응답하는건, 요청과 응답 책임을 가진 Controller에 명시적으로 작성하는 게 좋을 거 같다는 주영님의 리뷰를 반영해 수정했습니다.

@CookieValue가 던지는 예외

다음과 같이 @CookieValue를 적용했을 때 값이 없으면 MissingRequestCookieException을 던지게 되고 이는 GlobalExceptionHandler의 @ExceptionHandler(Exception.class)에서 처리됩니다.

@CookieValue(value = REFRESH_TOKEN_SUBJECT) String refreshToken

image

Request에 쿠키가 없는 사용자 요청의 문제이지만, HttpStatus가 500으로 반환(이는 서버 내부 에러 코드)됩니다.
잘못된 의미의 HttpStatus 코드가 반환되고 있어 @ExceptionHandler(MissingRequestCookieException.class)로 따로 잡아 처리했습니다.

물론 @CookieValue(value = REFRESH_TOKEN_SUBJECT, required = false) 설정으로 refreshToken에 null을 허용한 다음
if로 커스텀 에러를 던질 수 있습니다.
refreshToken을 null로 받아서 if문으로 null 체크 후 커스텀 예외를 던지는 방식보다는
컨트롤러단에서 빠르게 @CookieValue로 예외를 처리하는 게 더 효과적이라고 생각했습니다.
이에 대한 리뷰어 들의 생각이 궁금합니다.

📸 스크린샷

사용자 약관 동의 업데이트 API

image

사용자 추가 정보 업데이트 API

image

토큰 재발급 실패

image

📢 리뷰 요구사항

리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성

@CookieValue 관련 예외 처리 방식

  1. @CookieValue가 던지는 MissingRequestCookieException를 GlobalExceptionHander에서 따로 정의
  2. 쿠키가 없을 경우 null로 채운 뒤 커스텀 예외 처리

@koreaioi koreaioi linked an issue Jan 11, 2025 that may be closed by this pull request
2 tasks
Copy link
Member

@huncozyboy huncozyboy left a 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 {
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.

앗 친구 추가 기능이 닉네임 검색인 건 생각하지 못했네요!
매칭 채팅방에 같은 닉네임이 있다면 계좌를 이체할 때 혼동될 수 있을거 같아, 저도 닉네임 중복은 허용하지 않는 게 더 좋을거같아요!
다른 분들 의견도 궁금하네요!

만약 닉네임 중복 허용하지 않는다면, 다음과 같이 닉네임 중복 확인 버튼을 만드는 것도 프론트분들과 이야기 나눠봐야겠네요!
image

Copy link
Member

Choose a reason for hiding this comment

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

닉네임 중복 처리는 좋은 것 같아욤 근데 버튼을 따로 두기 보다는 중복 닉인 경우엔 toast 메시지를 주는게 사용자 경험이 더 좋을 것 같아요. 버튼이 따로 들어가면 디자인이나 UI가 조금 안 어울릴 것 같아서!

Copy link
Collaborator

Choose a reason for hiding this comment

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

넵 저도 닉네임 부분은 중복확인 넣는게 좋다고 생각해용
중복 검사 부분은 디자이너분과도 얘기 나눠봐야할 것 같네영

@koreaioi koreaioi self-assigned this Jan 11, 2025
@koreaioi koreaioi added the ✨ Feature 기능 개발 및 요구사항 변경 반영 label Jan 11, 2025
@koreaioi koreaioi requested review from hyxklee and soyesenna January 11, 2025 21:03
Copy link
Member

@hyxklee hyxklee left a 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)
Copy link
Member

Choose a reason for hiding this comment

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

해당 메서드는 주영님 피알에 구현이 되어있는 걸로 알긴 하는데, 개별 메서드로 구현해서 public으로 풀어놓고 공용으로 사용하는게 좋아 보여용!
주영님 피알이 머지되면 수정 하시거나 아니면 먼저 수정해주셔도 조을 것 같아욤

Copy link
Member Author

Choose a reason for hiding this comment

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

주영님 코드의 다음 부분 말씀해주시는 거 같네요!!
image

주영님 께서 NoSuchMemberExeption을 제가 작성한 MemberNotFoundException으로 수정한다고 하셔서
제 쪽에서는 findById() 수정 후 반영하도록 하겠습니다!

@@ -39,13 +28,30 @@ public InactiveMemberDto saveTmpMember(Long kakaoId){
}

@Transactional
public void updateInactiveMemberOfEmail(String email, Long userId) {
public void updateMemberEmail(String email, Long userId) {
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.

앗 그러네요!
이전 이슈에서 이메일 중복체크를 수행한 줄 알았는데 빠뜨렸네요..
큰일 날 뻔했네요!
인증코드를 이메일로 보내는 API에서 이메일 중복체크를 포함하도록 하겠습니다!

? LOGIN_SUCCESS
: UN_REGISTER;
return ApiResponse.response(HttpStatus.OK, OAUTH_STATUS.getMessage(), res);
public ApiResponse<Void> kakaoLogin(@RequestBody @Valid KakaoAuthCode kakaoAuthCode, HttpServletResponse response) {
Copy link
Member

Choose a reason for hiding this comment

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

이전엔 body로 로그인 상태와 유저 Id를 반환해줬는데 그게 빠진 이유가 있을까요?

클라이언트에서 구분할 수 없을 것 같아서요!

Copy link
Member Author

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가 없이 사용해도 괜찮을 거 같다고 하셨습니다!

Copy link
Member

Choose a reason for hiding this comment

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

로그인 상태도 구분이 가능하다고 하셨나요??

가입된 상태: 액세스 토큰, 리프레시 토큰 반환 -> 홈으로 라우팅
미가입 상태: 임시 토큰 반환 -> 회원가입으로 라우팅

이전에 body로 enum 값을 넣어서 구분 컬럼을 주고 있었는데, 그 부분이 빠져서 질문 드립니다!
아니면 토큰 반환 여부로 체크를 하신다고 하셨을까요??

Copy link
Member Author

Choose a reason for hiding this comment

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

앗 제가 API 응답에서 userId 삭제한다는 걸 로그인 상태까지 삭제해버렸네요...
다시 추가해서 반영했습니다!
감사합니다!

Copy link
Collaborator

@soyesenna soyesenna left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!!

@koreaioi koreaioi merged commit 540e161 into dev Jan 14, 2025
2 checks passed
@koreaioi koreaioi deleted the feat/#26/최초-회원가입-구현 branch January 26, 2025 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Feature 기능 개발 및 요구사항 변경 반영
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feat] #26 최초 회원가입 구현
4 participants