MOSU-358 refactor: 아이디 찾기 휴대폰 번호 비교 시 U Prefix 추가#359
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughFindLoginId now formats the phone number before repository lookup by calling PhoneNumberUtil.formatPhoneNumber(...) in findByNameAndPhoneNumber. After loading a user, it checks the user's provider and throws CustomRuntimeException(ErrorCode.KAKAO_ACCOUNT_DUPLICATED) if the provider is KAKAO. If no user is found it still throws NOT_FOUND_LOGIN_ID; otherwise it returns the login ID. No public signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant S as MyUserService
participant P as PhoneNumberUtil
participant R as UserRepository
U->>S: findLoginId(name, phoneNumber)
S->>P: formatPhoneNumber(phoneNumber)
P-->>S: formattedPhone
S->>R: findByNameAndPhoneNumber(name, formattedPhone)
alt user found
R-->>S: user
alt provider == KAKAO
S-->>U: throw CustomRuntimeException(KAKAO_ACCOUNT_DUPLICATED)
else other provider
S-->>U: loginId
end
else not found
R-->>S: null/empty
S-->>U: throw CustomRuntimeException(NOT_FOUND_LOGIN_ID)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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/Issue comments)Type 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 @chominju02, 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은 아이디 찾기 기능의 견고성을 향상시키기 위한 리팩토링입니다. 사용자로부터 입력받은 휴대폰 번호가 시스템에 저장된 형식과 일치하도록 PhoneNumberUtil을 통해 전처리하는 로직을 추가하여, 정확한 사용자 조회를 보장합니다. 이는 데이터 일관성을 유지하고 잠재적인 조회 오류를 방지하는 데 기여합니다.
Highlights
- 휴대폰 번호 형식 통일: 아이디 찾기 기능에서 휴대폰 번호를 비교할 때,
PhoneNumberUtil.formatPhoneNumber유틸리티를 사용하여 입력된 휴대폰 번호에 'U' 접두사를 추가하여 데이터베이스에 저장된 형식과 일치하도록 변경했습니다.
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. 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
-
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. ↩
| UserJpaEntity user = userJpaRepository.findByNameAndPhoneNumber(request.name(), | ||
| request.phoneNumber()) | ||
| PhoneNumberUtil.formatPhoneNumber(request.phoneNumber())) | ||
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.NOT_FOUND_LOGIN_ID)); |
There was a problem hiding this comment.
휴대폰 번호 형식(하이픈 포함/미포함)에 관계없이 일관된 조회를 위해, 조회 전 휴대폰 번호를 정규화하는 것이 좋습니다. 예를 들어, request.phoneNumber()에서 하이픈을 제거한 후 formatPhoneNumber를 호출하면 사용자가 어떤 형식으로 번호를 입력하더라도 동일한 결과로 조회할 수 있습니다.
UserJpaEntity에 getPhoneNumberWithoutHyphen() 메서드가 있는 것으로 보아, 데이터베이스에 하이픈이 포함된 휴대폰 번호가 저장될 수 있는 것 같습니다. 이 제안은 데이터베이스에 저장된 번호가 하이픈이 없는 숫자 형식으로 통일되어 있다는 가정 하에 유효합니다. 만약 하이픈이 포함된 형식으로 저장된다면, 정규화 방식에 대한 추가적인 논의가 필요해 보입니다.
| UserJpaEntity user = userJpaRepository.findByNameAndPhoneNumber(request.name(), | |
| request.phoneNumber()) | |
| PhoneNumberUtil.formatPhoneNumber(request.phoneNumber())) | |
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.NOT_FOUND_LOGIN_ID)); | |
| UserJpaEntity user = userJpaRepository.findByNameAndPhoneNumber(request.name(), | |
| PhoneNumberUtil.formatPhoneNumber(request.phoneNumber().replaceAll("-", ""))) | |
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.NOT_FOUND_LOGIN_ID)); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/java/life/mosu/mosuserver/application/user/MyUserService.java (3)
44-46: Minor readability: lift normalized value to a localPulling the normalized phone into a local improves debuggability and keeps parity with how other methods often stage intermediate values.
Apply this diff:
- UserJpaEntity user = userJpaRepository.findByNameAndPhoneNumber(request.name(), - PhoneNumberUtil.formatPhoneNumber(request.phoneNumber())) + String normalizedPhone = PhoneNumberUtil.formatPhoneNumber(request.phoneNumber()); + UserJpaEntity user = userJpaRepository.findByNameAndPhoneNumber(request.name(), + normalizedPhone)
32-35: Avoid double formatting before repository lookup
changePassword(...)first formats with hyphens and then re-formats; ifformatPhoneNumber(...)already normalizes to the canonical stored form (including theUprefix), the hyphenated step is redundant.Apply this simplification:
- String rgxPhone = PhoneNumberUtil.formatPhoneNumberWithHyphen(phoneNumber); - UserJpaEntity user = userJpaRepository.findByPhoneNumber( - PhoneNumberUtil.formatPhoneNumber(rgxPhone)) + String normalizedPhone = PhoneNumberUtil.formatPhoneNumber(phoneNumber); + UserJpaEntity user = userJpaRepository.findByPhoneNumber(normalizedPhone)If
formatPhoneNumberWithHyphen(...)encodes additional validation, keep it—but consider moving that validation intoformatPhoneNumber(...)for a single source of truth.
42-49: Add focused tests for normalized lookup pathsPlease add tests that assert successful lookup when
FindLoginIdRequest.phoneNumberis provided as:
- Raw digits (no
U, no separators)- Hyphenated digits
- Already
U-prefixed (upper and lower case inputs if allowed)- Negative path: non-existent phone returns
NOT_FOUND_LOGIN_IDI can draft test cases if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/main/java/life/mosu/mosuserver/application/user/MyUserService.java(1 hunks)
🔇 Additional comments (2)
src/main/java/life/mosu/mosuserver/application/user/MyUserService.java (2)
44-46: U-prefix normalization at lookup — LGTMSwitching to
PhoneNumberUtil.formatPhoneNumber(request.phoneNumber())beforefindByNameAndPhoneNumberaligns with the PR objective and keeps the lookup consistent with existing normalization patterns elsewhere in the service. No functional or behavioral regressions apparent.
45-46: PhoneNumberUtil semantics and data consistency verified, no further action needed
formatPhoneNumber(String)always prefixes an uppercase “U” to its input and does not strip hyphens; since callers only pass raw phone numbers (validated to match\d{3}-\d{4}-\d{4}), it effectively adds exactly one “U” and need not be idempotent.- All writes to the
user.phone_numbercolumn use exactly the value returned byformatPhoneNumber(...)(andformatGuestPhoneNumber(...)for guest users), so stored DB values always match the util’s output.FindLoginIdRequest.phoneNumberis annotated with@NotBlankPhoneNumberPatternand the controller method is marked@Valid, preventing null or malformed inputs before the util is invoked.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java (3)
171-171: Add trailing period to keep message style consistentMost messages in this enum end with a period. Align this entry for consistency and UX polish.
- KAKAO_ACCOUNT_DUPLICATED(HttpStatus.CONFLICT, "카카오로 가입한 사용자입니다", CriticalLevel.LOW), + KAKAO_ACCOUNT_DUPLICATED(HttpStatus.CONFLICT, "카카오로 가입한 사용자입니다.", CriticalLevel.LOW),
171-171: Clarify semantics vs existing KAKAO_DUPLICATEDThere’s already
KAKAO_DUPLICATED(Line 37) under Auth errors. The newKAKAO_ACCOUNT_DUPLICATEDunder “ID 찾기” has a different intent (social sign-up detected during Find ID), but the similar naming can confuse clients and devs. Consider adding a brief Javadoc to document intended usage scope (Find ID only) and expected client behavior (prompt social login), so consumers don’t misuse these two.Example Javadoc to add above the constant:
/** * ID 찾기 시 요청한 사용자가 카카오 소셜 가입자일 때 반환합니다. * 클라이언트는 '카카오 로그인으로 이용해주세요' 안내를 노출하세요. */Would you like me to open a follow-up PR to add these docs across similar provider-specific codes?
171-171: Confirm 409 (CONFLICT) aligns with API contract and client handlingUsing 409 here implies a state conflict (requested flow incompatible with account type). That can be fine, but please verify:
- API spec/docs list this new error for Find ID.
- Mobile/Web clients map 409+
KAKAO_ACCOUNT_DUPLICATEDto the correct UX (redirect/prompt to Kakao login), not a generic “error” toast.- Logging/metrics dashboards (e.g., Sentry/Grafana) treat this as expected, not an incident.
I can help update the API docs and client error mapping if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/main/java/life/mosu/mosuserver/application/user/MyUserService.java(2 hunks)src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/life/mosu/mosuserver/application/user/MyUserService.java
|
|
||
| // ID 찾기 관련 | ||
| NOT_FOUND_LOGIN_ID(HttpStatus.NOT_FOUND, "해당 아이디를 찾을 수 없습니다.", CriticalLevel.LOW), | ||
| KAKAO_ACCOUNT_DUPLICATED(HttpStatus.CONFLICT, "카카오로 가입한 사용자입니다", CriticalLevel.LOW), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add test coverage for FindLoginId Kakao-joined path
Since this code is used in the Find ID flow, add a service-layer test that:
- Given a user with provider=KAKAO returned by the repository (after phone formatting),
- When FindLoginId runs,
- Then it throws
CustomRuntimeExceptionwithErrorCode.KAKAO_ACCOUNT_DUPLICATED.
Also add a negative test asserting non-Kakao users return loginId, and a “not found” path still throws NOT_FOUND_LOGIN_ID.
You can locate usages and missing tests with:
🏁 Script executed:
#!/bin/bash
# Find new error usage sites
rg -n "KAKAO_ACCOUNT_DUPLICATED"
# Sanity-check FindLoginId flow and phone formatting
rg -n -C3 "find(Login)?Id|findByNameAndPhoneNumber|formatPhoneNumber"
# Locate exception mapping to HTTP responses (ControllerAdvice)
rg -n -C2 "CustomRuntimeException|@ExceptionHandler|ErrorCode\."Length of output: 154354
🏁 Script executed:
#!/bin/bash
# Search for service-layer tests covering the FindLoginId flow in MyUserService
rg -n 'findLoginId' -g 'src/test/java/**'
rg -n 'MyUserService' -g 'src/test/java/**'
rg -n 'ErrorCode.KAKAO_ACCOUNT_DUPLICATED' -g 'src/test/java/**'Length of output: 150
Add missing unit tests for MyUserService.findLoginId
The service-layer behavior for findLoginId currently lacks explicit test coverage. Please add a new test class (e.g. MyUserServiceTest) under src/test/java/life/mosu/mosuserver/application/user with the following mandatory test cases:
- Given a user returned by
userJpaRepository.findByNameAndPhoneNumberwhoseprovider == AuthProvider.KAKAO, invokingfindLoginIdshould throwCustomRuntimeExceptionwithErrorCode.KAKAO_ACCOUNT_DUPLICATED. - Given a non-Kakao user (e.g.
AuthProvider.MOSU), invokingfindLoginIdshould return a successfulFindLoginIdResponsecontaining the user’sloginId. - Given no matching user (repository returns empty), invoking
findLoginIdshould throwCustomRuntimeExceptionwithErrorCode.NOT_FOUND_LOGIN_ID.
Use Mockito (or your preferred mocking framework) to stub UserJpaRepository, and assert exception types and error codes, or response payloads, accordingly.
🤖 Prompt for AI Agents
In src/test/java/life/mosu/mosuserver/application/user/MyUserServiceTest
(covering behavior invoked from
src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java line 171),
add a test class that mocks UserJpaRepository and exercises
MyUserService.findLoginId with three cases: 1) stub findByNameAndPhoneNumber to
return a User whose provider == AuthProvider.KAKAO and assert it throws
CustomRuntimeException with ErrorCode.KAKAO_ACCOUNT_DUPLICATED; 2) stub to
return a non-KAKAO user (e.g. AuthProvider.MOSU) and assert the returned
FindLoginIdResponse contains the expected loginId; 3) stub to return
Optional.empty() and assert it throws CustomRuntimeException with
ErrorCode.NOT_FOUND_LOGIN_ID; use Mockito for stubbing and assertions on
exception type and embedded ErrorCode, and construct the service with the mocked
repository.
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
Bug Fixes
Chores