Feature/505 #506
Feature/505 #506
Conversation
๐ WalkthroughWalkthroughThis PR relocates phone number collection from the user signup flow to the rental process. The phoneNumber field becomes optional during registration, required during umbrella rental, and the system automatically captures it if the user lacks one. Related entity, DTO, service, and test changes support this new workflow. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Controller as RentController
participant Service as RentService
participant DB as Database
User->>Controller: GET /rent-form/{umbrellaId}
Controller->>Controller: Extract user from session
Controller->>DB: Fetch user details
DB-->>Controller: User (hasPhoneNumber: boolean)
Controller->>Service: findRentForm(umbrellaId, user)
Service->>DB: Fetch umbrella
DB-->>Service: Umbrella
Service-->>Controller: RentFormResponse {hasPhoneNumber}
Controller-->>User: Form with phone requirement flag
User->>Controller: POST /rent {phoneNumber, ...}
Controller->>Service: addRental(umbrellaId, request)
alt User has no phone number
Service->>DB: UPDATE user.phoneNumber
DB-->>Service: Success
else User already has phone number
Service->>Service: Skip phone update
end
Service->>DB: Create rental record
DB-->>Service: Rental created
Service-->>Controller: RentalHistoryResponse
Controller-->>User: Rental confirmed
Estimated code review effort๐ฏ 3 (Moderate) | โฑ๏ธ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
๐ฅ Pre-merge checks | โ 1 | โ 2โ Failed checks (1 warning, 1 inconclusive)
โ Passed checks (1 passed)
โ๏ธ Tip: You can configure your own custom pre-merge checks in the settings. โจ Finishing touches
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and canโt be posted inline due to platform limitations.
โ ๏ธ Outside diff range comments (1)
src/test/kotlin/upbrella/be/user/controller/UserControllerTest.kt (1)
108-109: Mark phoneNumber as optional in the API documentation.Since phoneNumber is now nullable (as reflected in UserInfoResponse and the broader PR changes), it should be marked as
.optional()in the response field documentation, consistent with the join request documentation on Line 372.๐ Proposed fix
fieldWithPath("phoneNumber").type(JsonFieldType.STRING) - .description("์ฌ์ฉ์ ์ ํ๋ฒํธ"), + .description("์ฌ์ฉ์ ์ ํ๋ฒํธ") + .optional(),
๐ค Fix all issues with AI agents
In @src/main/kotlin/upbrella/be/rent/dto/request/RentUmbrellaByUserRequest.kt:
- Around line 15-18: The phoneNumber property in RentUmbrellaByUserRequest
currently has a default empty string which conflicts with the @NotBlank
validation; remove the "= \"\"" initializer on the val phoneNumber so the field
is required at construction (or alternatively provide a valid non-blank default
if you intend it optional), ensuring the @field:NotBlank/@field:Pattern
validations can be satisfied.
๐งน Nitpick comments (2)
src/main/kotlin/upbrella/be/user/entity/User.kt (1)
74-74: Consider using null for deleted phone numbers.The code uses the sentinel value
"deleted"to mark phone numbers of deleted/withdrawn users. While this works, consider whethernullwould be more idiomatic now that phoneNumber is nullable, or whether this distinction (never provided vs. deleted) is semantically important for your domain.If the distinction matters, consider documenting this convention or using a more explicit approach (e.g., an enum or separate flag).
Also applies to: 85-85
docs/feature/505.md (1)
1-202: Excellent documentation with minor suggestion.The feature documentation is comprehensive, well-structured, and accurately reflects the code changes. It covers all aspects including API changes, affected files, and migration considerations.
Consider updating the test case checkboxes (lines 182-193) to reflect that the tests have been implemented:
- โ Test cases in
RentServiceTest.ktcover phone number handling during rental- โ Test cases in
UserTest.ktcoverupdatePhoneNumberandhasPhoneNumbermethods- โ Test cases in
RentControllerTest.ktcover the REST API changes
๐ Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
๐ Files selected for processing (15)
docs/feature/505.mdsrc/main/kotlin/upbrella/be/rent/controller/RentController.ktsrc/main/kotlin/upbrella/be/rent/dto/request/RentUmbrellaByUserRequest.ktsrc/main/kotlin/upbrella/be/rent/dto/response/HistoryInfoDto.ktsrc/main/kotlin/upbrella/be/rent/dto/response/RentFormResponse.ktsrc/main/kotlin/upbrella/be/rent/dto/response/RentalHistoryResponse.ktsrc/main/kotlin/upbrella/be/rent/service/RentService.ktsrc/main/kotlin/upbrella/be/user/dto/request/JoinRequest.ktsrc/main/kotlin/upbrella/be/user/dto/response/SingleUserInfoResponse.ktsrc/main/kotlin/upbrella/be/user/dto/response/UserInfoResponse.ktsrc/main/kotlin/upbrella/be/user/entity/User.ktsrc/test/kotlin/upbrella/be/rent/controller/RentControllerTest.ktsrc/test/kotlin/upbrella/be/rent/service/RentServiceTest.ktsrc/test/kotlin/upbrella/be/user/controller/UserControllerTest.ktsrc/test/kotlin/upbrella/be/user/entity/UserTest.kt
โฐ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
๐ Additional comments (23)
src/test/kotlin/upbrella/be/user/controller/UserControllerTest.kt (1)
372-372: LGTM!The phoneNumber field is correctly marked as optional in the join request documentation, aligning with the PR objective to make phone numbers optional during signup.
src/main/kotlin/upbrella/be/rent/controller/RentController.kt (1)
34-40: LGTM!The method correctly retrieves the authenticated user from the session and passes it to the rent service. This enables the rent form to determine whether the user has a phone number, consistent with the PR's objective to capture phone numbers during rental.
src/main/kotlin/upbrella/be/rent/dto/response/HistoryInfoDto.kt (1)
9-9: LGTM!The phoneNumber field is correctly updated to nullable (
String?), consistent with the PR's objective to make phone numbers optional during signup. QueryDSL's@QueryProjectionwill handle the nullable field appropriately.src/main/kotlin/upbrella/be/user/dto/response/SingleUserInfoResponse.kt (1)
9-9: LGTM!The phoneNumber field is correctly updated to nullable (
String?), aligning with the PR's goal of making phone numbers optional during signup. ThefromUserfactory method properly handles the nullable mapping.src/main/kotlin/upbrella/be/user/dto/response/UserInfoResponse.kt (1)
8-8: LGTM! Clean nullable type update.The phoneNumber field correctly reflects the nullable type from the User entity. The mapping logic remains sound.
src/main/kotlin/upbrella/be/user/entity/User.kt (3)
17-17: LGTM! Appropriate nullable field declaration.Making phoneNumber nullable with a default of null correctly supports the optional phone number flow during signup.
106-108: LGTM! Clean phone number update method.The method correctly accepts a non-null phone number and updates the entity. Validation at the service layer (via RentUmbrellaByUserRequest) ensures valid input.
110-112: LGTM! Robust phone number presence check.Using
isNullOrBlank()correctly handles null, empty strings, and whitespace-only values. This provides a reliable predicate for the rental flow's phone number collection logic.src/main/kotlin/upbrella/be/rent/dto/response/RentalHistoryResponse.kt (1)
9-9: LGTM! Consistent nullable type propagation.The phoneNumber field correctly reflects nullable types across the rental history response chain. Both factory methods properly handle the nullable assignment.
src/main/kotlin/upbrella/be/rent/dto/response/RentFormResponse.kt (1)
10-10: LGTM! Well-designed progressive data collection.The
hasPhoneNumberfield enables the rental flow to conditionally collect phone numbers only when needed. This improves the user experience by deferring optional data collection to the point where it becomes required.The factory method cleanly integrates with
User.hasPhoneNumber()to determine the flag value.Also applies to: 13-13, 19-19
src/main/kotlin/upbrella/be/user/dto/request/JoinRequest.kt (1)
12-14: No changes needed.The nullable
phoneNumberfield with@Patternand@Sizevalidators follows correct Bean Validation semantics. WhenphoneNumberisnull, both validators are skipped by default per JSR-303 specification, allowing optional submission. When provided,@Patternenforces the format^\d{3}-?\d{4}-?\d{4}$, and@Sizelimits to 16 characters. The controller applies@Validon theJoinRequestparameter, ensuring validation is triggered. The implementation is sound.src/test/kotlin/upbrella/be/rent/service/RentServiceTest.kt (3)
98-99: LGTM!The setUp correctly initializes the new
conditionReportandphoneNumberfields inRentUmbrellaByUserRequestto support the phone number collection flow during rental.
262-289: LGTM!The test correctly verifies that existing phone numbers are preserved during rental, ensuring the service doesn't overwrite a user's existing phone number.
233-260: Verify that the User's phone number update is actually persisted to the database.The service correctly relies on JPA dirty checking via
@Transactionalto persist thephoneNumberchange. However, the test uses a detached User entity created manually, which won't be managed by the persistence context. The assertion only verifies the in-memory state. Consider adding a verification that the User is explicitly saved or loaded from the database to confirm the change persists beyond the transaction scope.src/test/kotlin/upbrella/be/rent/controller/RentControllerTest.kt (3)
82-118: LGTM!The test correctly implements session-based authentication and properly wires the user retrieval flow. The SessionUser id matches the userReader mock, and the User entity is correctly passed to
findRentForm.
103-143: LGTM!The
hasPhoneNumberfield is correctly added toRentFormResponseand properly documented in REST Docs, enabling the frontend to conditionally display phone number input fields.
240-291: LGTM!The
phoneNumberfield is correctly added toRentUmbrellaByUserRequestand properly documented as a required field in REST Docs, aligning with the new rental flow requirements.src/test/kotlin/upbrella/be/user/entity/UserTest.kt (4)
170-190: LGTM!The test correctly verifies that
updatePhoneNumbersuccessfully updates a null phone number to the provided value.
192-209: LGTM!The test correctly verifies that
hasPhoneNumber()returns true when a user has a non-null, non-empty phone number.
211-228: LGTM!The test correctly verifies that
hasPhoneNumber()returns false when a user has a null phone number.
230-247: LGTM!The test correctly verifies that
hasPhoneNumber()returns false for empty strings, ensuring robust validation of phone number presence.src/main/kotlin/upbrella/be/rent/service/RentService.kt (2)
68-71: LGTM!The phone number update logic correctly populates the user's phone number only when absent, preserving existing phone numbers. This design ensures first-time renters capture their phone number while preventing accidental overwrites for existing users.
44-49: All callers offindRentFormhave been properly updated to pass the User parameter. The breaking change has been correctly applied across the codebase:
- Controller invocation:
rentService.findRentForm(umbrellaId, userToRent)โ- Test mock:
rentService.findRentForm(2L, userToRent)โ
| @field:NotBlank(message = "์ ํ๋ฒํธ๋ ํ์์ ๋๋ค.") | ||
| @field:Size(max = 16) | ||
| @field:Pattern(regexp = "^\\d{3}-?\\d{4}-?\\d{4}$", message = "์ ํจํ ์ ํ๋ฒํธ ํ์์ด ์๋๋๋ค.") | ||
| val phoneNumber: String = "" |
There was a problem hiding this comment.
Remove the default value or reconsider the validation.
The default value of "" (empty string) conflicts with the @NotBlank validation, which requires a non-empty string. If a caller relies on the default, validation will always fail. Consider removing the default value entirely, as phoneNumber is required during rental per the PR objectives.
๐ง Proposed fix
@field:NotBlank(message = "์ ํ๋ฒํธ๋ ํ์์
๋๋ค.")
@field:Size(max = 16)
@field:Pattern(regexp = "^\\d{3}-?\\d{4}-?\\d{4}$", message = "์ ํจํ ์ ํ๋ฒํธ ํ์์ด ์๋๋๋ค.")
- val phoneNumber: String = ""
+ val phoneNumber: String๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @field:NotBlank(message = "์ ํ๋ฒํธ๋ ํ์์ ๋๋ค.") | |
| @field:Size(max = 16) | |
| @field:Pattern(regexp = "^\\d{3}-?\\d{4}-?\\d{4}$", message = "์ ํจํ ์ ํ๋ฒํธ ํ์์ด ์๋๋๋ค.") | |
| val phoneNumber: String = "" | |
| @field:NotBlank(message = "์ ํ๋ฒํธ๋ ํ์์ ๋๋ค.") | |
| @field:Size(max = 16) | |
| @field:Pattern(regexp = "^\\d{3}-?\\d{4}-?\\d{4}$", message = "์ ํจํ ์ ํ๋ฒํธ ํ์์ด ์๋๋๋ค.") | |
| val phoneNumber: String |
๐ค Prompt for AI Agents
In @src/main/kotlin/upbrella/be/rent/dto/request/RentUmbrellaByUserRequest.kt
around lines 15 - 18, The phoneNumber property in RentUmbrellaByUserRequest
currently has a default empty string which conflicts with the @NotBlank
validation; remove the "= \"\"" initializer on the val phoneNumber so the field
is required at construction (or alternatively provide a valid non-blank default
if you intend it optional), ensuring the @field:NotBlank/@field:Pattern
validations can be satisfied.
๐ขย ๊ตฌํ๋ด์ฉ
๐งฉย ๊ณ ๋ฏผ๊ณผ ํด๊ฒฐ๊ณผ์
Summary by CodeRabbit
Release Notes
โ๏ธ Tip: You can customize this high-level summary in your review settings.