Refactor/181 university and my api#4
Conversation
|
Here's the code health analysis summary for commits Analysis Summary
|
WalkthroughThis pull request introduces several modifications across the codebase. In the authentication module, the exception declaration in the password validation method has been removed while preserving internal logic. New error codes are added to the error-handling enum. The user profile update functions are consolidated into a single endpoint with updated request mappings and dependency injections. University endpoints are pluralized and refined with a new cancellation operation for wish-listed universities, while related exception handling is revised. Several test files have been updated or removed, and a new SQL record for a user is inserted. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as SiteUserController
participant S as SiteUserService
participant S3 as S3Service
U->>C: PATCH /my with image & nickname
C->>S: updateMyPageInfo(user, imageFile, nickname)
S->>S3: (Optional) Validate/Upload image file
S-->>C: Update performed (no response body)
C-->>U: HTTP 200 OK
sequenceDiagram
participant U as User
participant UC as UniversityController
participant ULS as UniversityLikeService
U->>UC: DELETE /universities/{id}/like
UC->>ULS: cancelLikeUniversity(user, id)
alt University not liked
ULS-->>UC: Throw CustomException (NOT_LIKED_UNIVERSITY)
UC-->>U: HTTP error response
else Like exists
ULS-->>UC: LikeResultResponse (cancellation confirmed)
UC-->>U: HTTP 200 OK with response
end
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here. PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here. PR Code Suggestions ✨Explore these optional code suggestions:
|
![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
🔭 Outside diff range comments (1)
src/test/java/com/example/solidconnection/e2e/UniversityLikeTest.java (1)
31-123:⚠️ Potential issueAdd test coverage for already liked universities.
A test case for handling the scenario where a user attempts to like an already liked university has been removed. Based on the AI summary, the service now throws a
CustomExceptionwithALREADY_LIKED_UNIVERSITYerror code in this case. This scenario should be tested to ensure proper error handling.Add the following test case:
@Test void 이미_좋아요한_대학교에_좋아요를_누른다() { // setUp - Save initial like likedUniversityRepository.save(createLikedUniversity(siteUser, 괌대학_A_지원_정보)); // request - Attempt to like again RestAssured.given() .header("Authorization", "Bearer " + accessToken) .log().all() .post("/universities/" + 괌대학_A_지원_정보.getId() + "/like") .then().log().all() .statusCode(HttpStatus.BAD_REQUEST.value()) .body("code", equalTo("ALREADY_LIKED_UNIVERSITY")); }
🧹 Nitpick comments (12)
src/main/java/com/example/solidconnection/auth/service/EmailSignInService.java (1)
38-42: Consider keeping thethrows CustomExceptiondeclaration.While the method still throws
CustomException, removing the declaration from the method signature makes the error handling less explicit. This could make it harder for other developers to understand and handle potential exceptions.- private void validatePassword(String rawPassword, String encodedPassword) { + private void validatePassword(String rawPassword, String encodedPassword) throws CustomException {src/main/java/com/example/solidconnection/university/service/UniversityLikeService.java (1)
54-68: Consider adding more descriptive error messages.While the error handling is good, consider adding more descriptive error messages to help users understand what went wrong.
- throw new CustomException(NOT_LIKED_UNIVERSITY); + throw new CustomException(NOT_LIKED_UNIVERSITY, + "University with ID " + universityInfoForApplyId + " is not in your wish list");src/main/java/com/example/solidconnection/university/controller/UniversityController.java (1)
91-91: Address the TODO comment regarding return type.The comment indicates a needed change in the return type to
UniversityInfoForApplyPreviewResponses.Would you like me to help implement this change or create an issue to track this task?
src/test/java/com/example/solidconnection/university/service/UniversityLikeServiceTest.java (1)
156-167: Consider using a test data builder pattern.While the current test data creation is functional, consider implementing a builder pattern for
SiteUserto make test data creation more flexible and maintainable.Example implementation:
- private SiteUser createSiteUser() { - SiteUser siteUser = new SiteUser( - "test@example.com", - "nickname", - "profileImageUrl", - "1999-01-01", - PreparationStatus.CONSIDERING, - Role.MENTEE, - Gender.MALE - ); - return siteUserRepository.save(siteUser); - } + @Builder + private static class TestSiteUserBuilder { + private String email = "test@example.com"; + private String nickname = "nickname"; + private String profileImageUrl = "profileImageUrl"; + private String birthDate = "1999-01-01"; + private PreparationStatus status = PreparationStatus.CONSIDERING; + private Role role = Role.MENTEE; + private Gender gender = Gender.MALE; + + public SiteUser build() { + return new SiteUser(email, nickname, profileImageUrl, + birthDate, status, role, gender); + } + } + + private SiteUser createSiteUser() { + return siteUserRepository.save( + TestSiteUserBuilder.builder().build() + ); + }src/main/java/com/example/solidconnection/siteuser/controller/SiteUserController.java (1)
33-40: Consider returning updated info in the response.
Consolidating the nickname and profile image updates in one method is clean, but returning an empty response makes it harder for clients to confirm new values. Consider returning a response with updated fields for convenience.src/main/java/com/example/solidconnection/siteuser/service/SiteUserService.java (5)
50-53: Ensure partial updates are supported if desired.
All validations (nickname uniqueness, recency, and non-empty image) occur simultaneously. If the user intends to only update one item (e.g., just the nickname), it will still require a new image file. Clarify if every update must include a new image or if partial updates should be allowed.
61-62: User entity updates are straightforward.
Assigning the new URL and nickname here is fine. Optionally, you could unify these updates in a single setter or a domain method if you want stricter consistency checks or event triggers.
67-67: Consider normalization for nickname uniqueness checks.
By default,existsByNicknameis likely case-sensitive. Ensure you want "Nick" and "nick" as distinct entries. If not, convert the nickname to a canonical form before checking.
84-87: Allowing empty image might be desired?
Currently, the code enforces strictly that files not be empty. If the user only wants to change the nickname, they still must upload an image. Confirm this is intentional.
90-93: Fragile default-image check.
Relying on the'profile/'prefix to distinguish default images may break if future default URLs differ. A more explicit field or default-flag might be more robust.src/main/resources/data.sql (2)
44-47: Avoid dependency on external profile images in test data.The profile image URL points to a specific GitHub user's profile picture. This creates an external dependency that could break tests if the image becomes unavailable.
Consider using a local test image or a reliable placeholder image service:
-VALUES ('1999-01-01', 'test@test.email', 'yonso','https://github.com/nayonsoso.png', +VALUES ('1999-01-01', 'test@test.email', 'yonso','https://via.placeholder.com/150', 'FEMALE', 'CONSIDERING', 'MENTEE', '$2a$10$psmwlxPfqWnIlq9JrlQJkuXr1XtjRNsyVOgcTWYZub5jFfn0TML76', 'EMAIL'); -- 12341234
47-47: Use a stronger password for test data.The comment indicates the password is "12341234", which is a weak password. Even for test data, it's good practice to use strong passwords to prevent accidental use in production.
Consider using a stronger password that meets common security requirements (e.g., minimum length, mixed case, numbers, special characters).
- '$2a$10$psmwlxPfqWnIlq9JrlQJkuXr1XtjRNsyVOgcTWYZub5jFfn0TML76', 'EMAIL'); -- 12341234 + '$2a$10$psmwlxPfqWnIlq9JrlQJkuXr1XtjRNsyVOgcTWYZub5jFfn0TML76', 'EMAIL'); -- TestUser123!@#
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/main/java/com/example/solidconnection/auth/service/EmailSignInService.java(1 hunks)src/main/java/com/example/solidconnection/custom/exception/ErrorCode.java(1 hunks)src/main/java/com/example/solidconnection/siteuser/controller/SiteUserController.java(2 hunks)src/main/java/com/example/solidconnection/siteuser/service/SiteUserService.java(2 hunks)src/main/java/com/example/solidconnection/university/controller/UniversityController.java(4 hunks)src/main/java/com/example/solidconnection/university/service/UniversityLikeService.java(4 hunks)src/main/resources/data.sql(1 hunks)src/test/java/com/example/solidconnection/e2e/MyPageTest.java(1 hunks)src/test/java/com/example/solidconnection/e2e/MyPageUpdateTest.java(0 hunks)src/test/java/com/example/solidconnection/e2e/UniversityDetailTest.java(1 hunks)src/test/java/com/example/solidconnection/e2e/UniversityLikeTest.java(3 hunks)src/test/java/com/example/solidconnection/e2e/UniversityRecommendTest.java(5 hunks)src/test/java/com/example/solidconnection/e2e/UniversitySearchTest.java(7 hunks)src/test/java/com/example/solidconnection/siteuser/service/SiteUserServiceTest.java(8 hunks)src/test/java/com/example/solidconnection/university/service/UniversityLikeServiceTest.java(2 hunks)
💤 Files with no reviewable changes (1)
- src/test/java/com/example/solidconnection/e2e/MyPageUpdateTest.java
🔇 Additional comments (32)
src/test/java/com/example/solidconnection/e2e/UniversityRecommendTest.java (2)
69-69: LGTM! Endpoint changes follow REST API best practices.The endpoint updates from
/university/recommendsto/universities/recommendacross all test methods are consistent and align with REST API conventions by using plural nouns for resource collections.Also applies to: 97-97, 124-124, 151-151, 174-174
60-190: LGTM! Test implementation is robust and comprehensive.The test class demonstrates excellent test practices:
- Clear test scenarios covering all important use cases
- Well-structured test methods with setup, execution, and verification phases
- Comprehensive assertions using
assertAll- Descriptive test names
src/test/java/com/example/solidconnection/e2e/UniversityLikeTest.java (1)
75-75: LGTM! Endpoint paths updated to follow REST conventions.The endpoint paths have been updated from
/university/*to/universities/*to better align with REST conventions for resource collections.Also applies to: 94-94, 116-116
src/main/java/com/example/solidconnection/university/service/UniversityLikeService.java (1)
41-44: LGTM! Improved error handling for duplicate likes.The change properly handles the case when a user tries to like an already liked university by throwing a specific exception.
src/main/java/com/example/solidconnection/university/controller/UniversityController.java (1)
28-28: LGTM! Improved RESTful API design.The change from
/universityto/universitiesbetter follows REST conventions by using plural nouns for resource collections.src/test/java/com/example/solidconnection/e2e/UniversityDetailTest.java (1)
48-48: LGTM! Test updated to match new endpoint structure.The test has been properly updated to use the new pluralized endpoint path
/universities/, maintaining consistency with the controller changes.src/main/java/com/example/solidconnection/custom/exception/ErrorCode.java (1)
86-87: LGTM! Well-structured error codes.The new error codes follow the established pattern and maintain consistency with existing error codes in terms of HTTP status codes and message formatting.
src/test/java/com/example/solidconnection/university/service/UniversityLikeServiceTest.java (2)
41-72: LGTM! Well-organized test structure using nested classes.The use of
@Nestedclasses improves test organization and readability by grouping related test cases together.
44-71: LGTM! Comprehensive test coverage with proper assertions.The test methods effectively cover both success and failure scenarios:
- Uses
assertAllfor multiple assertions- Properly tests exception cases with
assertThatCodesrc/test/java/com/example/solidconnection/e2e/MyPageTest.java (2)
47-47: LGTM! Endpoint URL update aligns with controller changes.The endpoint URL change from
/my-pageto/mycorrectly reflects the updated@RequestMappinginSiteUserController.
41-57: Verify test coverage after MyPageUpdateTest.java deletion.Since
MyPageUpdateTest.javawas deleted, please ensure that all update operations previously tested there are now covered by other test files.❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for tests covering the new consolidated update endpoint rg -t java "test.*my.*update" "src/test" # Search for any remaining references to the old endpoint rg -t java "my-page" "src/test"Length of output: 70
Test Coverage for Update Operations Not Evident
The searches did not return any test cases specifically referencing update operations (e.g., tests using “update”, “put”, or “patch” with the “/my” endpoint). This could mean that either update operations were integrated into other tests with different naming, or that these cases are currently not being tested. Please manually verify that update scenarios previously covered in MyPageUpdateTest.java are now appropriately tested.
src/test/java/com/example/solidconnection/e2e/UniversitySearchTest.java (1)
46-46: LGTM! Consistent endpoint updates across all test methods.The changes to pluralize the university endpoints from
/university/searchto/universities/searchare consistent across all test methods and align with REST API best practices of using plural nouns for resources.Let's verify that these endpoint changes are coordinated with the controller updates:
Also applies to: 70-70, 88-88, 105-105, 123-123, 142-142, 158-158
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify that the UniversityController's base request mapping has been updated to match the test endpoints # Search for the UniversityController class and its request mapping ast-grep --pattern $'@RequestMapping($path) class UniversityController { $$$ }'Length of output: 82
I'll generate an additional shell script to further verify that the base request mapping in the UniversityController has been updated to use the
/universitiesendpoint.Please run this script to check for the appropriate updated mapping in UniversityController. If these commands show that the controller now uses
/universities, then the changes are coordinated. Otherwise, manual verification may be required.
🏁 Script executed:
#!/bin/bash echo "Searching for UniversityController file(s):" # List Java files mentioning 'UniversityController' fd -e java -x grep -H "class UniversityController" {} echo "------------------------------------------" echo "Searching for updated @RequestMapping endpoints (should include '/universities'):" # Search for @RequestMapping annotations that include '/universities' rg -n '@RequestMapping\(".*universities' .Length of output: 727
Endpoint updates are coordinated and consistent.
The UniversityController in
src/main/java/com/example/solidconnection/university/controller/UniversityController.javanow maps to"/universities"(line 28), which perfectly aligns with the test endpoints inUniversitySearchTest.java.src/main/java/com/example/solidconnection/siteuser/controller/SiteUserController.java (3)
4-4: No concerns with new import usage.
ImportingS3Servicehere appears correct and consistent with the changes below.
18-18: Heads-up on potential breaking change.
Changing the endpoint path from"/my-page"to"/my"could break existing client integrations if they are not updated accordingly.
23-23: Validate necessity of direct usage in controller.
S3Serviceis declared here but is not directly referenced in this controller code. Confirm if it’s needed or should remain only in the service layer to keep the controller lean.src/main/java/com/example/solidconnection/siteuser/service/SiteUserService.java (3)
47-47: Documentation alignment is fine.
This line properly explains the method’s purpose in Korean. No issues spotted.
58-59: Watch for upload failures.
While the integration withS3Servicelooks correct, consider handling potential exceptions from the upload call to provide more robust error responses.
95-104: Wish-list retrieval logic looks good.
Method implementation is straightforward, returns the correct list of universities. No issues found here.src/test/java/com/example/solidconnection/siteuser/service/SiteUserServiceTest.java (14)
19-19: Import of @beforeeach is correct.
Introducing@BeforeEachusage below is typical for shared setup logic.
39-41: Mockito imports look fine.
Using BDDMockito for readability in test setup is consistent with your existing pattern.
112-112: Method call coverage is appropriate.
Ensures the newly introducedupdateMyPageInfologic is under test.
115-115: Verification of updated profile image is accurate.
Asserts that the service call updatesprofileImageUrlcorrectly.
127-127: Repeated service call for coverage.
Testing another scenario or repeated usage is good practice. No concerns here.
155-155: Exception handling test is valid.
Confirms that empty image files trigger the expected exception, covering a crucial edge case.
164-168: Setup method removal of duplication.
Centralizing repeated stubbing logic for S3 in@BeforeEachis good practice.
174-174: Test-scope valid image creation is standard.
Creation of a validMockMultipartFilefor testing is clear and reusable.
178-178: Ensures nickname path is covered.
Updating both nickname and image in the same test verifies combined logic.
183-183: Checks final nickname state properly.
Asserting the final entity state ensures correctness of nickname updates.
191-191: Reusing valid file creation approach.
Shows consistency in test code. No concerns here.
194-196: Duplicate nickname exception is handled.
Verifies that the custom exception triggers as expected for collisions.
203-203: No concerns with file creation usage.
Continues consistent test scaffolding.
211-213: Testing insufficient time gap edge case.
Confirms the custom exception is thrown for nickname changes too soon.
1 participant
User description
관련 이슈
작업 내용
특이 사항
리뷰 요구사항 (선택)
PR Type
Enhancement, Bug fix, Tests
Description
Refactored and streamlined API endpoints for user and university operations.
my-pageendpoint tomyand consolidated update operations.universityendpoint touniversitiesand separated like/unlike operations.Enhanced error handling for user and university-related actions.
Improved test coverage and refactored test cases.
Added development environment test data for easier testing.
Changes walkthrough 📝
1 files
Removed unnecessary `throws` declaration in password validation5 files
Added error codes for university like/unlike operationsRefactored user API endpoints and consolidated update operationsConsolidated user profile and nickname update logicRefactored university API endpoints and separated like/unlikeoperationsAdded separate methods for liking and unliking universities8 files
Updated tests for refactored user API endpointsRemoved redundant tests for user profile updatesUpdated tests for refactored university detail endpointUpdated tests for university like/unlike operationsUpdated tests for university recommendation endpointUpdated tests for university search endpointRefactored tests for user profile and nickname updatesAdded tests for university like/unlike error handling1 files
Added test data for development environmentSummary by CodeRabbit
New Features
API Changes
Improvements