-
Notifications
You must be signed in to change notification settings - Fork 2
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] User API 수정 #39
Conversation
…to KAN-26-fix/user
Test Results1 tests 1 ✅ 0s ⏱️ Results for commit b27245b. ♻️ This comment has been updated with latest results. |
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 명세서에도 반영하신걸로 알고 있는데 맞나요?
코멘트 몇개 남겼는데 확인해주세요 ~~ 🚗
@PostMapping("/temporary-password") | ||
private ApiResponse<TemporaryPasswordResponse> sendTemporaryPassword(@RequestBody EmailRequest request){ | ||
TemporaryPasswordResponse response = emailService.sendTemporaryPassword(request); | ||
return ApiUtil.success(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.
기존의 임시 비밀번호 발급 대신에 비밀번호를 변경할 수 있도록 로직을 변경하기로 했던거 같은데 맞을까요?
비밀번호 변경 엔드포인트가 따로 있는것으로 알고 있는데, 그러면 이 엔드포인트를 삭제해야하는거 아닌가..
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와 비밀번호 변경 로직이 존재하지만, 사용하는 상황에 따른 분리가 필요할 것 같은데 이를 처리하는데 어려움이 있음
-
임시 비밀번호 발급의 경우 해당 이메일에 비밀번호가 전송되는 것으로, 하면 좋겠지만 굳이 이메일 인증이 필요하지 않음(타인이 다른 사람의 이메일을 통해 임시 비밀번호를 변경한다 하더라도 확인할 필요가 없음). 또한 Figma 상에서 이메일 인증 절차는 따로 없는 것으로 확인됨
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.
의도는 이해했어요.
제가 사용했던 서비스를 생각해보면 임시 비밀번호 발급만을 지원하는 서비스보다는 임시 비밀번호 + 비밀번호 변경을 같이 사용하고 있었어서 만약 임시 비밀번호를 사용하게 된다면 임시 비밀번호 + 비밀번호 변경 두가지를 모두 사용해야한다고 생각합니다.(딴 여기서 비밀번호 변경은 JWT단에서 인증인가를 모두 처리)
기획적인 내용이여서 전체 회의때 이야기 해봐요
private ApiResponse<String> signUp(@Valid @RequestBody SignUpRequest signUpRequest){ | ||
String response = userService.signUp(signUpRequest); | ||
return ApiUtil.success(response); | ||
private ApiResponse<Void> signUp(@Valid @RequestBody SignUpRequest signUpRequest){ |
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도 래핑 클래스가 있더라고요
|
||
private final JavaMailSender emailSender; | ||
|
||
@Async("mailExecutor") |
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.
mailExecutor을 쓰는게 어떤 의미일까요?
} | ||
|
||
public TemporaryPasswordResponse sendTemporaryPassword(EmailRequest request) { | ||
String email = request.getEmail(); | ||
log.warn("{}", email); |
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.
로깅을 warn으로 정한 이유가 있을까요?
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.
디버깅하면서 잠시 사용했던 로그인데 못지웠네요 수정하겠습니다!
public class EmailRequest { | ||
|
||
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.
해당 어노테이션을 사용한 경우의 Request가 올바르지 않은 경우의 Validation이 주는 예외를 어떤 형식으로 처리하면 될 지 고민 되어 삭제하였는데, BindingResult를 사용하면 될 것 같아서 유효성 검증은 이후 형식들이 정해지면 한번에 정하겠습니다
@Getter | ||
@NoArgsConstructor |
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 String email; | ||
|
||
@NotBlank |
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 AsyncConfig implements AsyncConfigurer { | ||
|
||
@Override | ||
@Bean(name = "mailExecutor") |
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.
여기서 등록해서 @async에서 이름으로 호출하는 형태라고 이해해도 될까요?
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.
넵 맞습니다. 또한 mailExcutor 라는 이름의 Bean으로 등록하여 사용하기 위해 Service 어노테이션을 사용하는 UserService에서 분리하여 AsyncEmailSenderService로 분리하였습니다.
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.
넵 구현한 API들은 노션에 정리해두었습니다!
임시 비밀번호API 및 Reissue 관련하여 약간 회의하면 될 것 같습니당
@PostMapping("/temporary-password") | ||
private ApiResponse<TemporaryPasswordResponse> sendTemporaryPassword(@RequestBody EmailRequest request){ | ||
TemporaryPasswordResponse response = emailService.sendTemporaryPassword(request); | ||
return ApiUtil.success(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.
현재 비밀번호 변경이 아닌 임시 비밀번호 발급으로 구현을 한 상태입니다. 그 이유는 아래와 같습니다,
-
비밀번호 변경을 허용할 경우 이메일 인증 절차를 필수로 거쳐야 한다고 생각함. 현재 이메일 인증 API와 비밀번호 변경 로직이 존재하지만, 사용하는 상황에 따른 분리가 필요할 것 같은데 이를 처리하는데 어려움이 있음
-
임시 비밀번호 발급의 경우 해당 이메일에 비밀번호가 전송되는 것으로, 하면 좋겠지만 굳이 이메일 인증이 필요하지 않음(타인이 다른 사람의 이메일을 통해 임시 비밀번호를 변경한다 하더라도 확인할 필요가 없음). 또한 Figma 상에서 이메일 인증 절차는 따로 없는 것으로 확인됨
} | ||
|
||
public TemporaryPasswordResponse sendTemporaryPassword(EmailRequest request) { | ||
String email = request.getEmail(); | ||
log.warn("{}", email); |
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 EmailRequest { | ||
|
||
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.
해당 어노테이션을 사용한 경우의 Request가 올바르지 않은 경우의 Validation이 주는 예외를 어떤 형식으로 처리하면 될 지 고민 되어 삭제하였는데, BindingResult를 사용하면 될 것 같아서 유효성 검증은 이후 형식들이 정해지면 한번에 정하겠습니다
public class AsyncConfig implements AsyncConfigurer { | ||
|
||
@Override | ||
@Bean(name = "mailExecutor") |
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.
넵 맞습니다. 또한 mailExcutor 라는 이름의 Bean으로 등록하여 사용하기 위해 Service 어노테이션을 사용하는 UserService에서 분리하여 AsyncEmailSenderService로 분리하였습니다.
✏️ 작업 개요
⛳ 작업 분류
🔨 작업 상세 내용
💡 생각해볼 문제