Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis change refactors the password change feature to identify users by phone number rather than login ID. It introduces a custom Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as MyUserController
participant Resolver as PhoneNumberArgumentResolver
participant Service as MyUserService
participant Repo as UserJpaRepository
participant Util as PhoneNumberUtil
Client->>Controller: POST /change-password (with request body)
Controller->>Resolver: Resolve @PhoneNumber parameter
Resolver->>Controller: Inject phone number from security context
Controller->>Util: formatPhoneNumberWithHyphen(phoneNumber)
Util-->>Controller: Formatted phone number
Controller->>Service: changePassword(request, formattedPhoneNumber)
Service->>Repo: findByPhoneNumber(formattedPhoneNumber)
Repo-->>Service: UserJpaEntity (if found)
Service-->>Controller: ChangePasswordResponse
Controller-->>Client: ResponseEntity<ApiResponseWrapper<ChangePasswordResponse>>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @jbh010204, 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!
This pull request significantly refactors the password change mechanism by shifting the user authentication from login ID to phone number. It introduces a custom annotation and an argument resolver to streamline the retrieval of the authenticated user's phone number in controller methods, along with a dedicated utility for phone number formatting. These changes aim to simplify the password change flow and improve code clarity.
Highlights
- Password Change Logic Refactor: The password change functionality has been refactored to authenticate users directly via their phone number instead of their login ID. This simplifies the process by removing the need for a separate login ID lookup and validation.
- Custom
@PhoneNumberAnnotation: A new custom@PhoneNumberannotation has been introduced. This annotation, combined with aPhoneNumberArgumentResolver, allows the phone number of the authenticated user to be automatically injected into controller method parameters, leading to cleaner and more readable API signatures. - Phone Number Utility: A new utility class,
PhoneNumberUtil, has been added to centralize the formatting of 11-digit phone numbers into a hyphenatedXXX-XXXX-XXXXformat, ensuring consistency across the application. - Request DTO Simplification: The
ChangePasswordRequestDTO has been simplified by removing theloginIdfield, as user identification for password changes now relies solely on the authenticated phone number.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request refactors the password change functionality to use a new @PhoneNumber custom annotation and argument resolver. This cleans up the controller logic by abstracting away the details of retrieving the phone number from the security context.
Suggestions include using consistent language for logging, improving variable naming for clarity, and removing unused code and annotations. These changes will make the code easier to understand and maintain.
| public ChangePasswordResponse changePassword(ChangePasswordRequest request, String phoneNumber) { | ||
|
|
||
| log.info("loginId: {}, phoneNumber: {}", request.loginId(), phoneNumber); | ||
| log.info("비밀번호 요청 하는 phoneNumber: {}", phoneNumber); |
There was a problem hiding this comment.
The log message is in Korean. For consistency, it's better to use English. This improves maintainability, especially in a diverse team.
| log.info("비밀번호 요청 하는 phoneNumber: {}", phoneNumber); | |
| log.info("Request to change password for phoneNumber: {}", phoneNumber); |
| String loginId = request.loginId(); | ||
| UserJpaEntity user = userJpaRepository.findByLoginId(loginId) | ||
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.USER_NOT_FOUND)); | ||
| String rgxPhone = PhoneNumberUtil.formatPhoneNumberWithHyphen(phoneNumber); |
There was a problem hiding this comment.
The variable name rgxPhone can be misleading. A more descriptive name like formattedPhoneNumber would improve code readability.
| String rgxPhone = PhoneNumberUtil.formatPhoneNumberWithHyphen(phoneNumber); | |
| String formattedPhoneNumber = PhoneNumberUtil.formatPhoneNumberWithHyphen(phoneNumber); |
| import life.mosu.mosuserver.global.annotation.PhoneNumber; | ||
| import life.mosu.mosuserver.global.filter.KmcAuthenticationToken; | ||
| import org.springframework.core.MethodParameter; | ||
| import org.springframework.core.io.support.SpringFactoriesLoader.ArgumentResolver; |
There was a problem hiding this comment.
This import org.springframework.core.io.support.SpringFactoriesLoader.ArgumentResolver is unused and should be removed.
|
|
||
| import org.springframework.stereotype.Component; | ||
|
|
||
| @Component |
There was a problem hiding this comment.
The @Component annotation is unnecessary for a utility class that only contains static methods. Consider making the class final and providing a private constructor to prevent instantiation.
| Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); | ||
|
|
There was a problem hiding this comment.
The authentication variable is initialized but is no longer used after the refactoring. This unused code should be removed.
|
|
||
| public record ChangePasswordRequest( | ||
| String loginId, | ||
| @Schema( |
There was a problem hiding this comment.
The loginId field is no longer needed in this DTO, as the phone number is now used for identification. Remove this field to simplify the request object.
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Improvements
API Changes