Conversation
Summary of ChangesHello @Dockerel, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 사용자 계정 관리를 위한 중요한 기능들을 도입합니다. 사용자는 이제 마이페이지를 통해 자신의 정보를 확인하고, 안전하게 로그아웃하며, 필요에 따라 계정을 탈퇴하거나 비밀번호를 변경할 수 있습니다. 이는 전반적인 사용자 경험을 향상시키고 계정 보안을 강화하는 데 기여합니다. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
이번 PR은 마이페이지, 로그아웃, 회원 탈퇴, 비밀번호 변경 등 멤버 관련 주요 기능들을 추가하는 변경 사항을 담고 있습니다. 전반적으로 기능 구현이 잘 이루어졌고, 특히 다양한 시나리오에 대한 테스트 코드를 충실하게 작성해주신 점이 인상적입니다. 몇 가지 개선점을 제안드립니다. 로그아웃 시 응답 메시지 오류, 회원 탈퇴 시 연관 데이터 처리 문제, 테스트 코드의 검증 로직 강화 등에 대한 피드백을 포함했습니다. 이를 통해 코드의 안정성과 유지보수성을 더욱 높일 수 있을 것입니다.
|
|
||
| public LogoutResponse logout(String refreshToken) { | ||
| memberRepository.deleteRefreshToken(refreshToken); | ||
| return LogoutResponse.of("회원가입이 완료되었습니다."); |
There was a problem hiding this comment.
로그아웃 성공 메시지가 "회원가입이 완료되었습니다."로 잘못 설정되어 있습니다. "로그아웃이 완료되었습니다."와 같이 올바른 메시지로 수정해야 합니다.
| return LogoutResponse.of("회원가입이 완료되었습니다."); | |
| return LogoutResponse.of("로그아웃이 완료되었습니다."); |
| public WithdrawResponse withdraw(String email) { | ||
| memberRepository.deleteMemberByEmail(email); | ||
|
|
||
| // TODO : 히스토리 및 채팅 삭제 | ||
|
|
||
| return WithdrawResponse.of("회원탈퇴가 완료되었습니다."); | ||
| } |
There was a problem hiding this comment.
회원 탈퇴 시 member만 삭제하고 관련 데이터(히스토리, 채팅)는 삭제하지 않고 있습니다. TODO로 명시되어 있지만, 이로 인해 고아 데이터(orphaned data)가 발생하여 데이터 무결성을 해칠 수 있습니다. 회원과 연관된 모든 데이터를 함께 삭제하거나, soft delete를 적용하는 것을 고려해야 합니다. 예를 들어, Member 엔티티에 deleted 플래그를 추가하고 회원 탈퇴 시 이 플래그를 업데이트하는 방식이 있습니다.
| @DisplayName("비밀번호를 변경한다.") | ||
| @Test | ||
| void updatePasswordByEmail() { | ||
| // given | ||
| String email = "email"; | ||
| String oldPassword = "password"; | ||
| String newPassword = "newPassword"; | ||
|
|
||
| Member member = Member.builder() | ||
| .email(email) | ||
| .password(oldPassword) | ||
| .build(); | ||
|
|
||
| memberJpaRepository.save(member); | ||
|
|
||
| // when | ||
| memberRepositoryImpl.updatePasswordByEmail(email, newPassword); | ||
|
|
||
| // then | ||
| MemberDto memberDto = memberRepositoryImpl.findByEmail(email); | ||
| assertThat(memberDto.getPassword()).isEqualTo(newPassword); | ||
| } |
There was a problem hiding this comment.
updatePasswordByEmail 테스트가 잘못 작성되었습니다.
updatePasswordByEmail메서드는 파라미터 이름(newEncryptedPassword)에서 알 수 있듯이 암호화된 비밀번호를 받도록 의도되었습니다. 하지만 테스트에서는 암호화되지 않은newPassword를 전달하고 있습니다.then절에서memberDto.getPassword()가 암호화되지 않은newPassword와 같다고 단언하고 있습니다. 이는 암호화되지 않은 비밀번호가 DB에 저장되는 것을 테스트하는 셈이며, 이는 보안상 매우 위험합니다.
테스트를 수정하여 PasswordEncryptor를 사용해 비밀번호를 암호화하여 전달하고, verifyPassword를 통해 올바르게 변경되었는지 검증해야 합니다.
// 예시
@Autowired
private PasswordEncryptor passwordEncryptor;
// ...
// given
// ...
memberJpaRepository.save(member);
String encryptedNewPassword = passwordEncryptor.encryptPassword(newPassword);
// when
memberRepositoryImpl.updatePasswordByEmail(email, encryptedNewPassword);
// then
MemberDto memberDto = memberRepositoryImpl.findByEmail(email);
assertThat(passwordEncryptor.verifyPassword(newPassword, memberDto.getPassword())).isTrue();| @@ -0,0 +1,18 @@ | |||
| package knu_chatbot.application.dto.response; | |||
There was a problem hiding this comment.
이 클래스는 서비스 계층에 대한 요청 데이터를 담는 DTO이므로, knu_chatbot.application.dto.request 패키지로 이동하는 것이 패키지 구조의 일관성 측면에서 더 적절해 보입니다. 현재는 response 패키지에 위치해 있어 혼란을 줄 수 있습니다.
|
|
||
| // 비밀번호 검증 | ||
| if (!passwordEncryptor.verifyPassword(request.getOldPassword(), memberDto.getPassword())) { | ||
| throw new KnuChatbotException(ErrorType.USER_LOGIN_ERROR); |
There was a problem hiding this comment.
비밀번호 변경 시 현재 비밀번호가 일치하지 않을 때 USER_LOGIN_ERROR를 사용하고 있습니다. 이 오류 유형은 로그인 실패를 위해 만들어진 것으로, "아이디 또는 비밀번호가 맞지 않습니다."라는 메시지를 반환하여 사용자에게 혼란을 줄 수 있습니다. "현재 비밀번호가 일치하지 않습니다."와 같은 명확한 메시지를 제공하는 새로운 오류 유형(예: CURRENT_PASSWORD_MISMATCH_ERROR)을 정의하여 사용하는 것이 좋습니다.
|
|
||
| member.changePassword(newEncryptedPassword); | ||
|
|
||
| memberJpaRepository.save(member); |
There was a problem hiding this comment.
updatePasswordByEmail 메서드는 서비스 계층의 @Transactional 컨텍스트 내에서 호출됩니다. 따라서 getOrThrowMemberByEmail을 통해 조회된 Member 엔티티는 영속성 컨텍스트에 의해 관리되는 상태(managed state)입니다. member.changePassword()를 통해 엔티티의 상태를 변경하면, 트랜잭션이 커밋될 때 변경 감지(dirty checking)에 의해 자동으로 UPDATE 쿼리가 실행됩니다. 따라서 이 save 호출은 불필요하며, 제거하여 코드를 더 간결하게 만들 수 있습니다.
| ChangePasswordResponse response = memberService.changePassword(authUser, request); | ||
|
|
||
| // then | ||
| assertThat(response).isNotNull(); |
There was a problem hiding this comment.
테스트의 then 절에서 response가 null이 아닌지만 검증하고 있습니다. 이는 비밀번호가 실제로 변경되었는지 보장하지 못합니다. 테스트의 신뢰도를 높이기 위해, 변경된 비밀번호로 로그인이 성공하는지, 그리고 이전 비밀번호로는 로그인이 실패하는지 등을 검증하는 로직을 추가하는 것이 좋습니다.
// then
assertThat(response).isNotNull();
// 새 비밀번호로 로그인 시도
LoginServiceRequest loginRequest = LoginServiceRequest.builder()
.email(email)
.password(newPassword)
.build();
// 예외가 발생하지 않아야 함
assertThatCode(() -> memberService.login(loginRequest)).doesNotThrowAnyException();
// 이전 비밀번호로 로그인 시도
LoginServiceRequest oldLoginRequest = LoginServiceRequest.builder()
.email(email)
.password(oldPassword)
.build();
// 예외가 발생해야 함
assertThatThrownBy(() -> memberService.login(oldLoginRequest))
.isInstanceOf(KnuChatbotException.class);Add step to generate application-dev.properties with secrets.
1 participant
#️⃣ 연관된 이슈
x
📚 배경
x
📝 작업 내용
💬 리뷰 요구사항
x
✏ Git Close
x