RAC-462 feat : wish 분리 및 신청서 API #327
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant changes across multiple classes, primarily focusing on the removal of the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 32
🧹 Outside diff range and nitpick comments (56)
src/main/java/com/postgraduate/domain/member/user/application/mapper/UserMapper.java (1)
52-54: Add JavaDoc comments to public methodsAdding JavaDoc comments to public methods improves code documentation and maintainability.
For example:
/** * Maps a given Role and User to a MemberRole. * * @param role the role to assign to the user * @param user the user to whom the role will be assigned * @return a new MemberRole object linking the user and role */ public static MemberRole mapToRole(Role role, User user) { // existing code }src/main/java/com/postgraduate/domain/member/user/domain/entity/User.java (4)
52-52: Prevent duplicate roles by using aSetinstead of aListUsing a
Listforrolesallows duplicate entries, which might lead to inconsistent role assignments. Consider using aSetto ensure each role is unique for a user.Apply this diff to change
rolesto aSet:-import java.util.ArrayList; +import java.util.HashSet;- private List<MemberRole> roles = new ArrayList<>(); + private Set<MemberRole> roles = new HashSet<>();
72-76: Simplify role checking inisJunior()methodYou can streamline the
isJunior()method by usinganyMatch, which enhances readability and may improve performance.Apply this diff:
- return roles.stream() - .map(MemberRole::getRole) - .toList() - .contains(Role.USER); + return roles.stream() + .anyMatch(memberRole -> memberRole.getRole() == Role.USER);
79-83: Simplify role checking inisSenior()methodSimilarly, simplify the
isSenior()method for better clarity.Apply this diff:
- return roles.stream() - .map(MemberRole::getRole) - .toList() - .contains(Role.SENIOR); + return roles.stream() + .anyMatch(memberRole -> memberRole.getRole() == Role.SENIOR);
86-90: Simplify role checking inisAdmin()methodThe
isAdmin()method can also be simplified usinganyMatch.Apply this diff:
- return roles.stream() - .map(MemberRole::getRole) - .toList() - .contains(Role.ADMIN); + return roles.stream() + .anyMatch(memberRole -> memberRole.getRole() == Role.ADMIN);src/main/java/com/postgraduate/domain/wish/presentation/constant/WishResponseMessage.java (1)
8-10: Consider adding error messages and i18n support.The enum currently only handles the success case. Consider:
- Adding constants for common error scenarios (e.g., invalid wish, duplicate wish)
- Implementing internationalization support for multi-language capabilities
Example enhancement:
public enum WishResponseMessage { APPLY_WISH("매칭 신청에 성공하였습니다."); + INVALID_WISH("유효하지 않은 매칭 신청입니다."), + DUPLICATE_WISH("이미 신청된 매칭입니다."); private final String message; }For i18n support, consider using message properties files:
// messages_ko.properties wish.apply.success=매칭 신청에 성공하였습니다. wish.apply.invalid=유효하지 않은 매칭 신청입니다. wish.apply.duplicate=이미 신청된 매칭입니다. // messages_en.properties wish.apply.success=Successfully applied for matching. wish.apply.invalid=Invalid matching application. wish.apply.duplicate=Matching already exists.src/main/resources/db/migration/V2_202411140644__create_table.sql (1)
1-6: Consider adding performance and data integrity improvementsWhile the basic table structure is correct, consider the following enhancements:
- Add an index on
user_idfor better query performance when looking up roles by user- Add a unique constraint to prevent duplicate role assignments
- Specify ON DELETE behavior for the foreign key
- Define character set and collation explicitly
Here's the improved version:
CREATE TABLE member_role ( role_id BIGINT AUTO_INCREMENT PRIMARY KEY, role ENUM('USER', 'SENIOR', 'ADMIN') NOT NULL, user_id BIGINT NOT NULL, - FOREIGN KEY (user_id) REFERENCES user(user_id) + FOREIGN KEY (user_id) REFERENCES user(user_id) ON DELETE CASCADE, + UNIQUE KEY uk_user_role (user_id, role), + INDEX idx_user_id (user_id) - ); + ) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci;src/main/java/com/postgraduate/domain/wish/domain/service/WishGetService.java (2)
7-11: Add class-level documentation.While the class structure follows good practices with appropriate annotations and dependency injection, it would benefit from JavaDoc documentation explaining its purpose and responsibilities within the wish management system.
Add documentation like this:
+/** + * Service responsible for retrieving wish-related data. + * This service provides read operations for wish entities as part of the wish management system. + */ @Service @RequiredArgsConstructor public class WishGetService { private final WishRepository wishRepository; }
9-11: Define the service contract and implement required methods.The service is currently empty without any methods. Consider:
- Creating an interface to define the service contract
- Implementing necessary retrieval methods based on business requirements
Would you like me to help draft the interface and common retrieval methods typically needed in such services? Some suggestions:
findByIdfindByUserIdfindAllwith pagination- Custom search/filter methods based on the new Wish entity fields (field, postgradu, professor, etc.)
src/main/resources/db/migration/V2_202411150555__migration_data.sql (1)
1-15: Consider adding rollback script.For safety, consider adding a corresponding rollback script in case the migration needs to be reversed.
Here's a suggested rollback script:
start transaction; -- Restore the column and foreign key ALTER TABLE wish ADD COLUMN user_user_id BIGINT; ALTER TABLE wish ADD CONSTRAINT FKpr2vr0ubj2t1ghyx6cckcamul FOREIGN KEY (user_user_id) REFERENCES user(user_id); -- Restore user_user_id data from member_role UPDATE wish w SET user_user_id = ( SELECT user_id FROM member_role WHERE role = 'USER' AND user_id IN (SELECT user_id FROM user) LIMIT 1 ); -- Clean up member_role table DELETE FROM member_role; commit;src/main/java/com/postgraduate/global/auth/login/application/dto/req/SignUpRequest.java (2)
Line range hint
9-10: Add phone number format validationConsider adding a pattern validation to ensure the phone number follows the expected format.
@NotBlank +@Pattern(regexp = "^\\d{2,3}-\\d{3,4}-\\d{4}$", message = "Invalid phone number format") String phoneNumber,
Line range hint
11-13: Consider internationalizing the error messageThe error message "6글자까지 입력 가능합니다" is hardcoded in Korean. Consider moving it to a message resource bundle for better internationalization support.
-@Size(max = 6, message = "6글자까지 입력 가능합니다.") +@Size(max = 6, message = "{validation.nickname.size}")src/main/java/com/postgraduate/domain/member/user/domain/entity/MemberRole.java (2)
10-15: Add @table annotation to explicitly define the table nameConsider adding the @table annotation to explicitly specify the table name and avoid potential naming conflicts in the database schema.
@Entity +@Table(name = "member_role") @Builder @AllArgsConstructor @NoArgsConstructor @Getter
15-27: Consider adding audit fieldsFor better tracking and debugging of role changes, consider adding audit fields such as
createdAt,updatedAt, andcreatedBy. This can be achieved by extending a base audit entity or using Spring Data JPA's auditing support.+@EntityListeners(AuditingEntityListener.class) public class MemberRole { // ... existing fields ... + + @CreatedDate + @Column(nullable = false, updatable = false) + private LocalDateTime createdAt; + + @LastModifiedDate + @Column(nullable = false) + private LocalDateTime updatedAt; + + @CreatedBy + @Column(updatable = false) + private String createdBy; }src/main/java/com/postgraduate/domain/wish/application/mapper/WishMapper.java (2)
8-8: Add class-level documentation.Consider adding Javadoc to describe the purpose and responsibilities of this mapper class.
+/** + * Mapper class responsible for converting wish-related DTOs and entities. + * Provides mapping functionality between {@link WishCreateRequest}, {@link User}, and {@link Wish} objects. + */ public class WishMapper {
14-18: Consider extracting phone number logic to a private method.The phone number assignment logic could be more explicit and reusable.
+ private static String resolvePhoneNumber(WishCreateRequest request, User user) { + return user != null ? user.getPhoneNumber() : request.phoneNumber(); + } public static Wish mapToWish(WishCreateRequest request, User user) { // ... validation code ... return Wish.builder() .field(request.field()) .postgradu(request.postgradu()) .lab(request.lab()) + .phoneNumber(resolvePhoneNumber(request, user)) .build(); }src/main/java/com/postgraduate/domain/wish/domain/entity/Wish.java (3)
20-24: Consider adding field validation constraintsThe
fieldandpostgradufields are marked as non-nullable but lack additional validation constraints. Consider adding:
@Sizeconstraints to prevent overly long values- Pattern validation for the
fieldto ensure consistent formatting@Column(nullable = false) +@Size(max = 100) private String field; @Column(nullable = false) +@Size(max = 100) private String postgradu;
37-38: Consider adding @column length constraints for the status fieldWhile the status field has a default value, it's good practice to specify the column length for enum string values.
@Column(nullable = false) +@Column(length = 20) @Enumerated(EnumType.STRING) @Builder.Default private Status status = Status.WAITING;
Line range hint
1-42: Consider adding audit fields for entity trackingSince this is an important entity representing wishes, consider adding audit fields to track creation and modification timestamps.
+import org.springframework.data.annotation.CreatedDate; +import org.springframework.data.annotation.LastModifiedDate; +import org.springframework.data.jpa.domain.support.AuditingEntityListener; @Entity +@EntityListeners(AuditingEntityListener.class) @Builder @AllArgsConstructor @NoArgsConstructor @Getter public class Wish { // existing fields... + @CreatedDate + @Column(nullable = false, updatable = false) + private LocalDateTime createdAt; + + @LastModifiedDate + @Column(nullable = false) + private LocalDateTime updatedAt; }src/main/java/com/postgraduate/domain/member/user/application/utils/UserUtils.java (3)
19-19: Consider improving the archive user initialization for better maintainability.The current initialization uses magic numbers and multiple null parameters which could make maintenance difficult.
Consider these improvements:
+ private static final Long ARCHIVE_USER_ID = -100L; + private static final String ARCHIVE_USER_NAME = "탈퇴한회원"; + private static final String ARCHIVE_USER_DISPLAY = "알수없음"; @PostConstruct public void init() { - archiveUser = new User(-100L, -100L, null, "알수없음", "탈퇴한회원", profile, 0, null, false, null, null, false, false); + archiveUser = User.createArchiveUser(ARCHIVE_USER_ID, ARCHIVE_USER_NAME, ARCHIVE_USER_DISPLAY, profile); }This would require adding a factory method in the User class:
public static User createArchiveUser(Long id, String name, String display, String profile) { return new User(id, id, null, display, name, profile, 0, null, false, null, null, false, false); }
Line range hint
1-34: Add class-level documentation explaining the utility's purpose.The class handles critical functionality like archived users and phone validation, but lacks documentation explaining its role and usage.
Add JavaDoc comments:
+ /** + * Utility class for user-related operations. + * Provides functionality for: + * - Managing archived/deleted user representation + * - Phone number validation + */ @Component @Slf4j public class UserUtils {
Line range hint
26-33: Consider enhancing phone number validation.The current validation only checks length and numeric format. Consider adding more comprehensive validation:
- Format verification (e.g., area codes)
- Support for international numbers
- Handling of special characters/spaces
Consider using a phone number validation library like
libphonenumber-java:+ <dependency> + <groupId>com.googlecode.libphonenumber</groupId> + <artifactId>libphonenumber</artifactId> + <version>8.13.27</version> + </dependency>Example implementation:
public void checkPhoneNumber(String phoneNumber) { try { PhoneNumberUtil phoneUtil = PhoneNumberUtil.getInstance(); PhoneNumber number = phoneUtil.parse(phoneNumber, "KR"); if (!phoneUtil.isValidNumber(number)) { throw new PhoneNumberException(); } } catch (NumberParseException e) { throw new PhoneNumberException(); } }src/main/java/com/postgraduate/admin/domain/repository/AdminUserRepository.java (1)
Wish-related code still present in admin module requires attention
The verification reveals several wish-related dependencies in the admin module that need to be addressed:
AdminUserUseCase.javacontains unimplemented wish functionality with a TODO comment- Multiple admin module files still contain wish references:
src/main/java/com/postgraduate/admin/application/dto/res/WishResponse.javasrc/main/java/com/postgraduate/admin/presentation/AdminWithThymeLeafController.javasrc/main/java/com/postgraduate/admin/domain/service/AdminUserService.javasrc/main/java/com/postgraduate/admin/application/mapper/AdminMapper.javaThe wish separation is incomplete as there are remaining wish-related components in the admin module that need to be either removed or updated.
🔗 Analysis chain
Line range hint
1-35: Verify the impact of wish separationThe removal of wish-related functionality aligns with the PR objectives. However, we should verify that all wish-related dependencies have been properly handled.
Let's verify the wish separation implementation:
Let me gather more specific information about the wish-related code in the repository, particularly focusing on any dependencies in the admin module.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining wish references echo "Checking for remaining wish references..." # Look for any remaining wish imports rg -l "import.*Wish" --type java # Look for wish-related method calls rg -l "wish\." --type java # Look for wish-related table joins in repositories rg -l "join.*wish" --type java # Check for wish-related database migrations fd -e sql . | xargs grep -l "wish"Length of output: 2407
Script:
#!/bin/bash # Check for wish-related code in admin module echo "Checking wish dependencies in admin module..." # Look for wish references in admin module rg -l "wish" "src/main/java/com/postgraduate/admin" # Check for specific wish usage in AdminUserUseCase rg -A 5 "wish" "src/main/java/com/postgraduate/admin/application/usecase/AdminUserUseCase.java" # Check for any wish-related joins or queries in admin repositories rg -A 5 "join.*wish|wish\." "src/main/java/com/postgraduate/admin/domain/repository/" # Check for wish-related imports in admin module rg "import.*[Ww]ish" "src/main/java/com/postgraduate/admin/"Length of output: 1436
src/main/java/com/postgraduate/domain/wish/application/usecase/WishManageUseCase.java (1)
13-16: Consider method-level transaction management.The class-level
@Transactionalannotation makes all public methods transactional. Consider moving it to method level for more fine-grained transaction control, especially if you plan to add read-only operations in the future.@RequiredArgsConstructor @Service -@Transactional public class WishManageUseCase {src/main/java/com/postgraduate/global/auth/quit/application/mapper/QuitMapper.java (1)
22-27: Consider builder method chaining for better readabilityThe current implementation splits the builder chain into multiple statements. Consider consolidating the builder pattern for better readability and maintainability.
- QuitBuilder quit = Quit.builder() - .reason(request.reason().getReason()); - if (user.isSenior()) { - return quit.role(Role.SENIOR); - } - return quit.role(Role.USER); + return Quit.builder() + .reason(request.reason().getReason()) + .role(user.isSenior() ? Role.SENIOR : Role.USER);src/main/java/com/postgraduate/domain/wish/presentation/WishController.java (1)
16-20: Consider adding security and validation annotations.While the basic controller setup is good, consider enhancing it with:
@Validatedfor request validation@PreAuthorizeor similar security annotations to explicitly define access control@RestController @RequiredArgsConstructor @RequestMapping("/wish") @Tag(name = "Wish Controller") +@Validated +@PreAuthorize("isAuthenticated()") public class WishController {src/main/java/com/postgraduate/domain/member/user/domain/service/UserDeleteService.java (2)
Line range hint
23-34: Consider adding transaction management and improving error handlingThe deletion process involves multiple repositories and cascading operations, but lacks transaction management and proper error handling.
Consider applying these improvements:
+@Transactional public void deleteUser(User user) { + try { mentoringRepository.findAllByUser(user) .stream() .forEach(Mentoring::updateUserDelete); // payment 에서 User null로 변경 paymentRepository.findAllByUser(user) .stream() .forEach(Payment::updateUserDelete); - log.info("user 삭제"); + log.info("Starting user deletion process for user ID: {}", user.getId()); user.getRoles().forEach(memberRoleRepository::delete); userRepository.delete(user); + log.info("Successfully deleted user ID: {}", user.getId()); + } catch (Exception e) { + log.error("Failed to delete user ID: {}. Error: {}", user.getId(), e.getMessage()); + throw new RuntimeException("Failed to delete user", e); + } }Additional suggestions:
- Consider adding validation to ensure the user exists
- Consider adding pre-deletion checks for any constraints
- Consider implementing soft delete instead of hard delete for audit purposes
Line range hint
24-30: Improve stream operations efficiencyThe current implementation creates multiple streams for different operations. Consider collecting related operations for better performance.
Here's a suggested improvement:
- mentoringRepository.findAllByUser(user) - .stream() - .forEach(Mentoring::updateUserDelete); - // payment 에서 User null로 변경 - paymentRepository.findAllByUser(user) - .stream() - .forEach(Payment::updateUserDelete); + // Update all related entities + var mentorings = mentoringRepository.findAllByUser(user); + var payments = paymentRepository.findAllByUser(user); + + mentorings.forEach(Mentoring::updateUserDelete); + payments.forEach(Payment::updateUserDelete);src/main/java/com/postgraduate/admin/application/usecase/AdminUserUseCase.java (2)
33-37: LGTM! Consider pagination for large datasets.The implementation is clean and follows good practices. However, if the dataset grows large, consider implementing pagination to avoid loading all junior users into memory at once.
Line range hint
1-54: Consider splitting responsibilities into separate use cases.The class currently handles multiple concerns (authentication, user management, wish management, messaging). Consider splitting these into separate use cases following the Single Responsibility Principle:
AdminAuthUseCasefor loginAdminUserManagementUseCasefor user-related operationsAdminMessagingUseCasefor message sendingThis would make the code more maintainable and easier to test.
src/main/java/com/postgraduate/global/bizppurio/application/usecase/BizppurioJuniorMessage.java (1)
38-39: Add parameter validation for phone number.The method accepts a raw string without validation. Consider adding checks for null/empty values and phone number format.
public void matchingWaiting(String phoneNumber) { + if (phoneNumber == null || phoneNumber.trim().isEmpty()) { + throw new IllegalArgumentException("Phone number cannot be null or empty"); + } + if (!phoneNumber.matches("^\\d{10,11}$")) { // Adjust regex based on your phone number format + throw new IllegalArgumentException("Invalid phone number format"); + } bizppurioSend.sendMessageWithExceptionHandling(() -> mapper.mapToJuniorMatchingWaiting(phoneNumber)); }src/test/java/com/postgraduate/domain/image/application/usecase/ImageUploadUseTypeTest.java (2)
50-51: Improve test data quality and readabilityThe test data could be improved in several ways:
- Magic numbers (-1L, -1234L) should be replaced with named constants or more meaningful test values
- The phone number contains a leading space which seems incorrect
- Consider extracting the test user creation into a helper method for reuse
+ private static final Long TEST_USER_ID = 1L; + private static final Long TEST_MEMBER_ID = 1234L; + private static final String TEST_PHONE = "01012345678"; + + private User createTestUser() { + return new User( + TEST_USER_ID, + TEST_MEMBER_ID, + "[email protected]", + "Test User", + TEST_PHONE, + "profile.jpg", + 0, + USER, + TRUE, + LocalDateTime.now(), + LocalDateTime.now(), + FALSE, + TRUE + ); + } + @Test @DisplayName("Profile 업로드 테스트") void uploadProfile() { - User user = new User(-1L, -1234L, "abc.com", "abc" - , " 123123", "abcab", 0 - , USER, TRUE, LocalDateTime.now(), LocalDateTime.now(), FALSE, TRUE); + User user = createTestUser();
Line range hint
52-63: Enhance test coverage with additional scenariosThe current test only covers the happy path. Consider adding tests for:
- Error scenarios (invalid file type, upload failure)
- Case when user has no existing profile image
- S3 service throwing exceptions
Also, the mock verification could be more specific:
given(s3UploadService.saveProfileFile(mockMultipartFile)) .willReturn("url"); ImageUrlResponse imageUrlResponse = imageUploadUseCase.uploadProfile(user, mockMultipartFile); - verify(s3UploadService).deleteProfileImage(user.getProfile()); + verify(s3UploadService, times(1)).deleteProfileImage("abcab"); + verify(s3UploadService, times(1)).saveProfileFile(mockMultipartFile); + verifyNoMoreInteractions(s3UploadService); Assertions.assertThat(imageUrlResponse.profileUrl()) .isEqualTo("url");src/test/java/com/postgraduate/domain/user/application/usecase/UserMyPageUseTypeTest.java (1)
Line range hint
28-34: Consider using a builder pattern or test factory for better test readability.The User constructor calls with multiple parameters make the test setup harder to read and maintain. Consider introducing a builder pattern or test factory method to improve readability.
Example improvement:
private User createTestUser(Role role) { return User.builder() .id(1L) .socialId(1234L) .nickName("a") .profile("a") .phoneNumber("123") .major("a") .role(role) .isAgree(TRUE) .createdAt(LocalDateTime.now()) .updatedAt(LocalDateTime.now()) .isDelete(TRUE) .isPush(TRUE) .build(); }Then in tests:
user = createTestUser(USER); // in setting() user = createTestUser(SENIOR); // in checkSeniorWithSenior()Also applies to: 73-77
src/test/java/com/postgraduate/domain/user/domain/service/UserUpdateServiceTest.java (2)
39-40: Consider adding test cases with non-empty role lists.The current test setup uses an empty ArrayList for roles, which might not adequately test the new multi-role functionality. Consider adding test cases that cover:
- Single role scenarios
- Multiple roles scenarios
- Edge cases (e.g., duplicate roles)
- 0, new ArrayList<>(), FALSE, + 0, new ArrayList<>(List.of(USER)), FALSE,
Line range hint
70-76: Fix duplicate assertions in updateInfo test.The test contains three identical assertions for
user.getProfile(). The assertions should likely be checking different fields (nickName and phoneNumber) based on the test setup.assertThat(user.getProfile()) .isEqualTo(request.profile()); - assertThat(user.getProfile()) - .isEqualTo(request.profile()); - assertThat(user.getProfile()) - .isEqualTo(request.profile()); + assertThat(user.getNickName()) + .isEqualTo(request.nick()); + assertThat(user.getPhoneNumber()) + .isEqualTo(request.phone());src/test/java/com/postgraduate/domain/user/domain/service/UserGetServiceTest.java (1)
Line range hint
31-37: Consider improving test data creation.The current test data uses placeholder values (-1L, "a", "123") which could be made more meaningful. Consider:
- Using a builder pattern for more readable test data creation
- Creating realistic test values that better represent actual use cases
Example improvement:
@BeforeEach void setting() { user = User.builder() .userId(1L) .socialId(12345L) .email("[email protected]") .nickName("testUser") .phoneNumber("01012345678") .profileImage("profile.jpg") .career(3) .role(USER) .push(TRUE) .createdAt(LocalDateTime.now()) .updatedAt(LocalDateTime.now()) .isProfile(TRUE) .isPremium(TRUE) .build(); }src/main/java/com/postgraduate/global/slack/SlackSignUpMessage.java (2)
Line range hint
57-71: Refactor timestamp formatting for better maintainabilityThe timestamp formatting logic is verbose and duplicated. Consider extracting it to a utility method using DateTimeFormatter.
Here's a suggested improvement:
+ private static final DateTimeFormatter SLACK_TIME_FORMAT = + DateTimeFormatter.ofPattern("M월 d일 H시 m분 s초"); private Attachment generateJuniorSignUpAttachment(User user) { LocalDateTime createdAt = user.getCreatedAt(); return Attachment.builder() .color("2FC4B2") .title("가입한 후배 정보") .fields(List.of( - generateSlackField("가입 시간 : " - + createdAt.getMonth().getValue() + "월 " - + createdAt.getDayOfMonth() + "일 " - + createdAt.getHour() + "시 " - + createdAt.getMinute() + "분 " - + createdAt.getSecond() + "초", null), + generateSlackField("가입 시간 : " + createdAt.format(SLACK_TIME_FORMAT), null), generateSlackField("후배 닉네임 : " + user.getNickName(), null) )) .build(); }This change would also apply to the senior signup method for consistency.
Update test cases to match new method signature
The verification revealed that while the production code has been updated to use the new
sendJuniorSignUp(User)signature, there's a test case that still expects the old signature with two parameters:
src/test/java/com/postgraduate/Integration/AuthControllerTest.java: UpdatedoNothing().when(slackSignUpMessage).sendJuniorSignUp(any(), any())to use single parameter:doNothing().when(slackSignUpMessage).sendJuniorSignUp(any())🔗 Analysis chain
Line range hint
31-41: LGTM! Verify impact of Wish removalThe simplified method signature aligns well with the PR objectives. The implementation correctly handles the User notification without the Wish dependency.
Let's verify that all calls to this method have been updated:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining calls with both User and Wish parameters ast-grep --pattern 'sendJuniorSignUp($_, $_)' # Search for updated calls with single User parameter ast-grep --pattern 'sendJuniorSignUp($_)'Length of output: 91
Script:
#!/bin/bash # Search for method calls using ripgrep for better coverage rg "sendJuniorSignUp" -A 2 -B 2 # Also search for the class usage to understand the context rg "SlackSignUpMessage" -A 2 -B 2Length of output: 6202
src/test/java/com/postgraduate/domain/payment/usecase/PaymentManageUseTypeTest.java (2)
Line range hint
95-95: Address WebClient test coverageThe TODO comment indicates missing test coverage for WebClient functionality. This is important as WebClient is used for external communication, and proper testing would ensure reliable integration.
Would you like me to help generate test cases for the WebClient functionality? I can provide examples using MockWebServer or WebTestClient to properly test the HTTP interactions.
Line range hint
41-74: Consider extracting test data setup to helper methodsThe
setting()method contains hardcoded test data. Consider extracting this to dedicated test data factory methods or constants to improve maintainability and reusability.Here's a suggested refactor:
+ private static final Long TEST_USER_ID = -1L; + private static final Long TEST_KAKAO_ID = 1234L; + private static final String TEST_EMAIL = "a"; + private static final String TEST_PHONE = "123"; - void setting() { - info = new Info("a", "a", "a", "a", "a", "a", TRUE, TRUE, "a", "chatLink", 30); - profile = new Profile("a", "a", "a"); - user = new User(-1L, 1234L, "a", - "a", "123", "a", - 0, SENIOR, TRUE, LocalDateTime.now(), LocalDateTime.now(), TRUE, TRUE); + void setting() { + info = createTestInfo(); + profile = createTestProfile(); + user = createTestUser(TEST_USER_ID, TEST_KAKAO_ID, SENIOR); + mentoringUser = createTestUser(-2L, 12345L, USER); + senior = createTestSenior(user); + } + + private Info createTestInfo() { + return new Info("a", "a", "a", "a", "a", "a", TRUE, TRUE, "a", "chatLink", 30); + }src/main/java/com/postgraduate/batch/scheduler/JobSchedulerConfig.java (1)
Line range hint
75-102: Separate job execution from verification logicThe method combines multiple responsibilities:
- Job execution retry logic
- Success verification
- Error notification
Consider splitting these into separate methods for better maintainability and testing.
- private void checkSalaryJobSuccess() throws JobExecutionAlreadyRunningException, JobRestartException, JobInstanceAlreadyCompleteException, JobParametersInvalidException { + private void checkSalaryJobSuccess() throws JobExecutionAlreadyRunningException, JobRestartException, JobInstanceAlreadyCompleteException, JobParametersInvalidException { + int retries = 0; + while (retries < MAX_RETRIES && !isSalaryJobSuccessful()) { + retrySalaryJob(); + retries++; + } + if (retries >= MAX_RETRIES) { + slackErrorMessage.sendSlackSalaryError(); + } + } + + private boolean isSalaryJobSuccessful() { + int seniorSize = seniorGetService.allSeniorId().size(); + int salarySize = salaryGetService.findAllNext().size(); + return salarySize == seniorSize; + } + + private void retrySalaryJob() throws JobExecutionAlreadyRunningException, JobRestartException, JobInstanceAlreadyCompleteException, JobParametersInvalidException { + try { + Thread.sleep(5000); + } catch (InterruptedException ex) { + Thread.currentThread().interrupt(); + log.error("Thread Interrupt 발생"); + } + JobParameters jobParameters = new JobParametersBuilder() + .addLocalDateTime("date", LocalDateTime.now()) + .toJobParameters(); + jobLauncher.run(salaryJobWithAdmin, jobParameters); + }src/main/java/com/postgraduate/global/auth/login/application/usecase/oauth/SignUpUseCase.java (2)
60-65: Add method documentation to clarify business logicWhile the implementation looks correct, the method would benefit from documentation explaining:
- The purpose of this method and when it should be called
- Why we skip role addition for junior users
- The business implications of adding a junior role to non-junior users
+ /** + * Adds a junior role to non-junior users. + * @param user The user to potentially add the junior role to + */ public void changeUser(User user) {
71-72: Consider deterministic profile assignmentWhile the role management changes look good, the random profile selection (
rd.nextInt(5)) might lead to inconsistent user experiences. Consider:
- Using a more deterministic approach based on user attributes
- Documenting the profile selection logic
src/main/java/com/postgraduate/global/config/security/jwt/util/JwtUtils.java (2)
Line range hint
96-104: Add missing security validations and null checksThe new
checkRedismethod has several potential security and robustness issues that should be addressed:
Missing validations:
- No null check for Authorization header
- No validation that token is of type REFRESH
- No token expiration verification
Potential runtime exceptions:
- Array index out of bounds if Authorization header doesn't contain "Bearer "
Consider applying these improvements:
public String checkRedis(Long id, HttpServletRequest request) { - String refreshToken = request.getHeader(AUTHORIZATION).split(" ")[1]; + String authHeader = request.getHeader(AUTHORIZATION); + if (authHeader == null || !authHeader.startsWith("Bearer ")) { + throw new InvalidRefreshTokenException(); + } + String refreshToken = authHeader.substring(7); + + // Validate token type and expiration + Claims claims = parseClaims(refreshToken); + if (!claims.get(TYPE).equals(REFRESH.name())) { + throw new InvalidTokenException(); + } + String redisToken = redisRepository.getValues(REFRESH.toString() + id) .orElseThrow(NoneRefreshTokenException::new); if (!redisToken.equals(refreshToken)) throw new InvalidRefreshTokenException(); - Claims claims = parseClaims(refreshToken); return claims.get(ROLE).toString(); }
Line range hint
96-104: Consider enhancing security measuresFor improved security, consider implementing:
- Token revocation list for compromised refresh tokens
- Audit logging for token validation attempts and failures
- Rate limiting for token validation requests
This would help prevent token abuse and provide better security monitoring.
src/test/java/com/postgraduate/domain/mentoring/application/usecase/MentoringApplyingUseTypeTest.java (1)
Remove the commented test case as it's redundant
The commented test case
applyMentoringWithAccount()inMentoringApplyingUseTypeTest.javashould be removed because:
- The same functionality is already being tested in
MentoringManageUseTypeTest.javawhich has an active test case with the same name that verifies the mentoring application process with account.- The implementation in
MentoringApplyingUseCase.javashows that account handling is part of the main flow, and the test inMentoringManageUseTypeTest.javaproperly covers this through the integration test approach.🔗 Analysis chain
Line range hint
89-107: Verify if the commented test case is still relevantThere's a commented-out test case
applyMentoringWithAccount()that might need attention. Given the recent changes to remove Wish dependency and simplify the application process, please verify if this test should be:
- Uncommented and updated to reflect the new structure
- Removed if it's no longer relevant
- Left commented if it's for future implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar test methods or related functionality rg -l "applyMentoringWithAccount|Account.*exist" --type javaLength of output: 264
Script:
#!/bin/bash # Let's check the content of both files to understand the context better echo "=== MentoringApplyingUseTypeTest.java ===" rg -A 10 -B 10 "applyMentoringWithAccount|Account.*exist" src/test/java/com/postgraduate/domain/mentoring/application/usecase/MentoringApplyingUseTypeTest.java echo -e "\n=== MentoringManageUseTypeTest.java ===" rg -A 10 -B 10 "applyMentoringWithAccount|Account.*exist" src/test/java/com/postgraduate/domain/mentoring/application/usecase/MentoringManageUseTypeTest.java # Let's also check the implementation class echo -e "\n=== Looking for related implementation ===" rg -A 10 -B 10 "applyMentoringWithPayment" src/main/java/com/postgraduate/domain/mentoring/Length of output: 9253
src/test/java/com/postgraduate/domain/senior/application/usecase/SeniorMyPageUseTypeTest.java (1)
Line range hint
42-63: Consider improving test data setupWhile the test setup is functional, consider these improvements:
- Extract magic values to constants (e.g., "a", "123", 1L)
- Create focused test data builders for different test scenarios
- Consider using a test data factory pattern for cleaner setup
This would make the tests more maintainable and easier to understand.
Example improvement:
private static final String DEFAULT_NICKNAME = "testNick"; private static final Long DEFAULT_ID = 1L; @Builder private static class TestUserBuilder { private Long id = DEFAULT_ID; private Long socialId = 1234L; private String nickname = DEFAULT_NICKNAME; // ... other fields with defaults public User build() { return new User(id, socialId, nickname, ...); } }src/test/java/com/postgraduate/domain/senior/application/usecase/SeniorInfoUseTypeTest.java (2)
Line range hint
1-1: Class name appears to be incorrect.The class name
SeniorInfoUseTypeTestseems to be a typo. Given that it's testingSeniorInfoUseCase, it should be namedSeniorInfoUseCaseTest.-class SeniorInfoUseTypeTest { +class SeniorInfoUseCaseTest {
Line range hint
41-71: Consider simplifying test setup using builder pattern.The test setup is quite verbose with multiple object initializations. Consider using the builder pattern to make the test setup more maintainable and readable.
Example implementation:
@TestConfiguration class TestBuilders { static class UserBuilder { private Long userId = 1L; private Long kakaoId = 1234L; private String nickName = "a"; private String profile = "a"; private String phoneNumber = "123"; private String email = "a"; private int point = 1; private Role role = USER; private Boolean push = TRUE; public UserBuilder withUserId(Long userId) { this.userId = userId; return this; } // Add other builder methods... public User build() { return new User(userId, kakaoId, nickName, profile, phoneNumber, email, point, role, push, LocalDateTime.now(), LocalDateTime.now(), TRUE, TRUE); } } // Add similar builders for Senior, Info, Profile... }This would simplify the setup to:
@BeforeEach void setting() { user = new UserBuilder() .withUserId(1L) .withKakaoId(1234L) .build(); originUser = new UserBuilder() .withUserId(2L) .withKakaoId(12345L) .withPhoneNumber("12345") .build(); // Similar pattern for other objects... }src/test/java/com/postgraduate/domain/mentoring/application/usecase/MentoringUserInfoUseTypeTest.java (1)
Line range hint
42-83: Consider enhancing test data setup.While the current setup is functional, consider these improvements for better maintainability:
- Extract test data creation into builder methods or test factories
- Use constants for repeated values like "a" and magic numbers
- Consider using
@Valueobjects for test data to make the setup more conciseExample builder pattern:
private User createTestUser(Long id, Long kakaoId, Role role) { return new User( id, kakaoId, "testName", "testEmail", "testPhone", "testPicture", 0, role, TRUE, LocalDateTime.now(), LocalDateTime.now(), TRUE, TRUE ); }src/main/java/com/postgraduate/admin/application/mapper/AdminMapper.java (1)
69-69: Architectural Decision Needed: Junior/Senior User Type DistinctionThe hardcoded
falsevalue with the TODO comment indicates a significant architectural change in how junior/senior users are distinguished. This change could have broader implications for:
- User role management
- Access control
- Admin interface filtering
- Business logic that depends on user type
Consider these alternatives:
- Move the junior/senior distinction to the new
member_roletable mentioned in the PR description- Add a dedicated field in the User entity for this distinction
- Implement a role-based approach using Spring Security
Would you like to discuss the architectural approach to handle this user type distinction?
src/test/java/com/postgraduate/domain/senior/application/usecase/SeniorManageUseTypeTest.java (1)
155-157: Make verification more specific by using actual parametersThe current verification uses
any()matcher which is too permissive. Consider making the test more robust by verifying with the exact Account object that should be created from the request.- verify(seniorSaveService) - .saveAccount(any()); + verify(seniorSaveService) + .saveAccount(argThat(account -> + account.getBank().equals(request.bank()) && + account.getAccountHolder().equals(request.accountHolder()) && + account.getAccountNumber().equals(request.accountNumber()) + ));src/main/java/com/postgraduate/global/auth/login/presentation/AuthController.java (1)
98-99: Improve API documentation clarityThe operation description "후배로 변경 가능한 경우 후배 토큰 발급" (Issue token for juniors when change is possible) should be enhanced to clearly specify:
- The conditions under which a user can change to junior role
- The implications of this role change
- The expected response in case the change is not possible
src/test/java/com/postgraduate/domain/mentoring/application/usecase/MentoringSeniorInfoUseTypeTest.java (1)
69-74: Consider using a builder pattern for test data setupThe current User object creation with multiple parameters is becoming harder to maintain. Consider introducing a builder pattern or test data factory to improve test readability and maintainability.
- user = new User(-1L, 1234L, "a", - "a", "123", "a", - 0, SENIOR, TRUE, LocalDateTime.now(), LocalDateTime.now(), TRUE, TRUE); + user = UserTestBuilder.builder() + .id(-1L) + .providerId(1234L) + .email("a") + .password("123") + .phoneNumber("a") + .point(0) + .role(SENIOR) + .status(TRUE) + .createdAt(LocalDateTime.now()) + .updatedAt(LocalDateTime.now()) + .push(TRUE) + .marketing(TRUE) + .build();src/main/java/com/postgraduate/global/bizppurio/application/mapper/BizppurioMapper.java (1)
261-273: Consider maintaining consistency in message personalization approach.While the change to accept
phoneNumberdirectly reduces coupling with the User entity, it creates an inconsistency with other methods in the class that still use the User object for personalized messages. This might indicate a gradual transition, but it's worth considering whether this specific method should maintain the same level of personalization for consistency in user experience.Consider one of these approaches:
- Pass the nickname as an additional parameter to maintain personalization:
-public CommonRequest mapToJuniorMatchingWaiting(String phoneNumber) { +public CommonRequest mapToJuniorMatchingWaiting(String phoneNumber, String nickname) { String message = ( - "안녕하세요, " + "회원님.\n" + + "안녕하세요, " + nickname + "님.\n" +
- If this is part of a larger transition, consider updating other methods to follow the same pattern for consistency:
-public CommonRequest mapToSeniorSignUpMessage(User user) { +public CommonRequest mapToSeniorSignUpMessage(String phoneNumber) { String message = ( - user.getNickName() + " 선배님! 대학원 김선배에 회원가입을..." + + "선배님! 대학원 김선배에 회원가입을..." +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (65)
src/main/java/com/postgraduate/admin/application/mapper/AdminMapper.java(3 hunks)src/main/java/com/postgraduate/admin/application/usecase/AdminUserUseCase.java(1 hunks)src/main/java/com/postgraduate/admin/domain/repository/AdminUserRepository.java(1 hunks)src/main/java/com/postgraduate/admin/domain/service/AdminUserService.java(1 hunks)src/main/java/com/postgraduate/batch/scheduler/JobSchedulerConfig.java(2 hunks)src/main/java/com/postgraduate/domain/member/senior/application/usecase/SeniorInfoUseCase.java(1 hunks)src/main/java/com/postgraduate/domain/member/senior/application/usecase/SeniorMyPageUseCase.java(0 hunks)src/main/java/com/postgraduate/domain/member/senior/presentation/SeniorController.java(0 hunks)src/main/java/com/postgraduate/domain/member/user/application/mapper/UserMapper.java(2 hunks)src/main/java/com/postgraduate/domain/member/user/application/usecase/UserMyPageUseCase.java(1 hunks)src/main/java/com/postgraduate/domain/member/user/application/utils/UserUtils.java(1 hunks)src/main/java/com/postgraduate/domain/member/user/domain/entity/MemberRole.java(1 hunks)src/main/java/com/postgraduate/domain/member/user/domain/entity/User.java(3 hunks)src/main/java/com/postgraduate/domain/member/user/domain/repository/MemberRoleRepository.java(1 hunks)src/main/java/com/postgraduate/domain/member/user/domain/repository/UserDslRepository.java(0 hunks)src/main/java/com/postgraduate/domain/member/user/domain/repository/UserDslRepositoryImpl.java(0 hunks)src/main/java/com/postgraduate/domain/member/user/domain/service/UserDeleteService.java(3 hunks)src/main/java/com/postgraduate/domain/member/user/domain/service/UserSaveService.java(2 hunks)src/main/java/com/postgraduate/domain/member/user/domain/service/UserUpdateService.java(2 hunks)src/main/java/com/postgraduate/domain/payment/application/usecase/PaymentManageUseCase.java(1 hunks)src/main/java/com/postgraduate/domain/wish/application/dto/request/WishCreateRequest.java(1 hunks)src/main/java/com/postgraduate/domain/wish/application/mapper/WishMapper.java(1 hunks)src/main/java/com/postgraduate/domain/wish/application/usecase/WishManageUseCase.java(1 hunks)src/main/java/com/postgraduate/domain/wish/domain/entity/Wish.java(2 hunks)src/main/java/com/postgraduate/domain/wish/domain/repository/WishRepository.java(1 hunks)src/main/java/com/postgraduate/domain/wish/domain/service/WishGetService.java(1 hunks)src/main/java/com/postgraduate/domain/wish/domain/service/WishSaveService.java(1 hunks)src/main/java/com/postgraduate/domain/wish/domain/service/WishUpdateService.java(1 hunks)src/main/java/com/postgraduate/domain/wish/presentation/WishController.java(1 hunks)src/main/java/com/postgraduate/domain/wish/presentation/constant/WishResponseCode.java(1 hunks)src/main/java/com/postgraduate/domain/wish/presentation/constant/WishResponseMessage.java(1 hunks)src/main/java/com/postgraduate/global/auth/login/application/dto/req/SignUpRequest.java(1 hunks)src/main/java/com/postgraduate/global/auth/login/application/usecase/jwt/JwtUseCase.java(4 hunks)src/main/java/com/postgraduate/global/auth/login/application/usecase/oauth/SignUpUseCase.java(4 hunks)src/main/java/com/postgraduate/global/auth/login/application/usecase/oauth/kakao/KakaoSignOutUseCase.java(1 hunks)src/main/java/com/postgraduate/global/auth/login/presentation/AuthController.java(1 hunks)src/main/java/com/postgraduate/global/auth/quit/application/mapper/QuitMapper.java(2 hunks)src/main/java/com/postgraduate/global/bizppurio/application/mapper/BizppurioMapper.java(1 hunks)src/main/java/com/postgraduate/global/bizppurio/application/usecase/BizppurioJuniorMessage.java(1 hunks)src/main/java/com/postgraduate/global/config/security/jwt/auth/AuthDetails.java(1 hunks)src/main/java/com/postgraduate/global/config/security/jwt/util/JwtUtils.java(1 hunks)src/main/java/com/postgraduate/global/slack/SlackSignUpMessage.java(3 hunks)src/main/resources/db/migration/V2_202411140644__create_table.sql(1 hunks)src/main/resources/db/migration/V2_202411150555__migration_data.sql(1 hunks)src/main/resources/db/migration/V2_202411150733__modify_wish.sql(1 hunks)src/test/java/com/postgraduate/Integration/AuthControllerTest.java(1 hunks)src/test/java/com/postgraduate/domain/auth/application/usecase/jwt/JwtUseTypeTest.java(6 hunks)src/test/java/com/postgraduate/domain/auth/application/usecase/oauth/SignUpUseTypeTest.java(1 hunks)src/test/java/com/postgraduate/domain/auth/application/usecase/oauth/kakao/KakaoSignInUseTypeTest.java(1 hunks)src/test/java/com/postgraduate/domain/auth/presentation/AuthControllerTest.java(1 hunks)src/test/java/com/postgraduate/domain/image/application/usecase/ImageUploadUseTypeTest.java(1 hunks)src/test/java/com/postgraduate/domain/mentoring/application/usecase/MentoringApplyingUseTypeTest.java(1 hunks)src/test/java/com/postgraduate/domain/mentoring/application/usecase/MentoringManageUseTypeTest.java(1 hunks)src/test/java/com/postgraduate/domain/mentoring/application/usecase/MentoringSeniorInfoUseTypeTest.java(1 hunks)src/test/java/com/postgraduate/domain/mentoring/application/usecase/MentoringUserInfoUseTypeTest.java(1 hunks)src/test/java/com/postgraduate/domain/payment/usecase/PaymentManageUseTypeTest.java(1 hunks)src/test/java/com/postgraduate/domain/senior/application/usecase/SeniorInfoUseTypeTest.java(1 hunks)src/test/java/com/postgraduate/domain/senior/application/usecase/SeniorManageUseTypeTest.java(2 hunks)src/test/java/com/postgraduate/domain/senior/application/usecase/SeniorMyPageUseTypeTest.java(1 hunks)src/test/java/com/postgraduate/domain/senior/domain/service/SeniorUpdateServiceTest.java(1 hunks)src/test/java/com/postgraduate/domain/senior/presentation/SeniorControllerTest.java(0 hunks)src/test/java/com/postgraduate/domain/user/application/usecase/UserMyPageUseTypeTest.java(2 hunks)src/test/java/com/postgraduate/domain/user/domain/service/UserGetServiceTest.java(1 hunks)src/test/java/com/postgraduate/domain/user/domain/service/UserUpdateServiceTest.java(3 hunks)src/test/java/com/postgraduate/support/Resource.java(2 hunks)
💤 Files with no reviewable changes (5)
- src/main/java/com/postgraduate/domain/member/senior/application/usecase/SeniorMyPageUseCase.java
- src/main/java/com/postgraduate/domain/member/senior/presentation/SeniorController.java
- src/main/java/com/postgraduate/domain/member/user/domain/repository/UserDslRepository.java
- src/main/java/com/postgraduate/domain/member/user/domain/repository/UserDslRepositoryImpl.java
- src/test/java/com/postgraduate/domain/senior/presentation/SeniorControllerTest.java
✅ Files skipped from review due to trivial changes (7)
- src/main/java/com/postgraduate/domain/member/user/domain/repository/MemberRoleRepository.java
- src/main/java/com/postgraduate/domain/wish/application/dto/request/WishCreateRequest.java
- src/main/java/com/postgraduate/domain/wish/domain/repository/WishRepository.java
- src/main/java/com/postgraduate/domain/wish/domain/service/WishUpdateService.java
- src/main/java/com/postgraduate/domain/wish/presentation/constant/WishResponseCode.java
- src/test/java/com/postgraduate/Integration/AuthControllerTest.java
- src/test/java/com/postgraduate/domain/auth/application/usecase/oauth/SignUpUseTypeTest.java
🔇 Additional comments (67)
src/main/java/com/postgraduate/domain/wish/presentation/constant/WishResponseMessage.java (1)
1-12: LGTM! Clean and well-structured enum implementation.
The implementation follows good practices with proper use of Lombok annotations and final fields.
src/main/resources/db/migration/V2_202411140644__create_table.sql (1)
3-3: Verify the completeness of role types
The ENUM values seem appropriate for the current use case, but let's verify there are no other role types used in the codebase.
✅ Verification successful
Role enum values in SQL match the codebase definition
The role types defined in the SQL migration (USER, SENIOR, ADMIN) perfectly match the enum values defined in Role.java (ADMIN, USER, SENIOR). The codebase shows consistent role definitions across both the database schema and the application code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for role-related constants or enums in the codebase
rg -t java -t kotlin "enum.*Role|Role.*enum" -A 5
Length of output: 346
src/main/resources/db/migration/V2_202411150555__migration_data.sql (3)
1-1: LGTM: Transaction boundaries are properly set.
The migration is correctly wrapped in a transaction to ensure atomicity of all operations.
Also applies to: 15-15
3-6: Verify data integrity for role migration.
The role migration looks correct but we should verify:
- That all SENIOR and ADMIN users are accounted for
- No duplicate entries are created
12-13: Verify impact of foreign key removal.
Before dropping the foreign key and column:
- Ensure all necessary data has been migrated
- Verify no other constraints or indexes depend on this column
✅ Verification successful
Let me check the content of the migration files to understand the changes better.
Let me check the entity and repository code to verify if the changes are properly reflected in the codebase.
Foreign key removal is safe to proceed
The verification shows that:
- Data migration is properly handled - the
user_user_iddata is migrated tomember_roletable before removal - The Wish entity has been updated to reflect the schema changes - no references to the removed column
- No other constraints or indexes depend on the removed column
- The codebase has been updated to work with the new schema - TODO comments in admin section acknowledge the wish changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other dependencies on the wish table
# Look for other migration files that might affect the wish table
fd -e sql . | xargs grep -l "wish"
# Look for references to the wish table in the code
rg "wish" --type java --type kotlin
Length of output: 9628
Script:
#!/bin/bash
# Check the content of both migration files affecting the wish table
cat src/main/resources/db/migration/V2_202411150555__migration_data.sql
cat src/main/resources/db/migration/V2_202411150733__modify_wish.sql
Length of output: 839
Script:
#!/bin/bash
# Check the Wish entity and repository implementation
cat src/main/java/com/postgraduate/domain/wish/domain/entity/Wish.java
cat src/main/java/com/postgraduate/domain/wish/domain/repository/WishRepository.java
Length of output: 1355
src/main/resources/db/migration/V2_202411150733__modify_wish.sql (4)
1-1: LGTM: Transaction boundaries are properly set.
The migration is correctly wrapped in a transaction to ensure atomicity of the changes.
Also applies to: 18-18
16-16: LGTM: Default status value is appropriate.
Setting the default status to 'WAITING' is a logical choice for new wish entries.
10-14: Verify impact of dropping columns.
The removal of major and matching_receive columns needs careful consideration:
- Ensure no existing code references these columns
- Consider if any data needs to be migrated to the new columns
✅ Verification successful
Based on the search results, I can generate the final response as I have sufficient information to verify the impact of dropping these columns.
Dropping major and matching_receive columns appears safe
The search results show that:
- The
majorcolumn is not referenced in any JPA entity with@Column(name = "major")annotation - The
matching_receivecolumn is not referenced in any JPA entity with@Column(name = "matching_receive")annotation - All occurrences of "major" in the codebase are either:
- In test data/fixtures
- In DTOs/request objects that are not persisted
- In HTML templates for display purposes
- In the migration file itself
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any references to the dropped columns in the codebase
rg -A 5 "major|matching_receive"
# Look for any JPA entity definitions that might reference these columns
ast-grep --pattern 'class $_ {
$$$
@Column(name = "major")
$$$
}'
ast-grep --pattern 'class $_ {
$$$
@Column(name = "matching_receive")
$$$
}'
Length of output: 33149
7-8: Verify field length constraints and consider adding phone number validation.
- The
fieldcolumn length of 255 seems generous. Consider if a smaller length would be more appropriate. - The
phone_numbercolumn should ideally have a CHECK constraint to ensure valid formats.
Consider adding a CHECK constraint for phone numbers:
ALTER TABLE wish
ADD CONSTRAINT valid_phone_number
CHECK (phone_number ~ '^\+?[0-9]{10,15}$');✅ Verification successful
Based on the search results, I can provide a definitive response regarding the phone number validation concern:
Phone number validation is already implemented with appropriate constraints
The codebase shows robust phone number validation:
- Length validation is enforced to exactly 11 digits in
UserUtils.checkPhoneNumber() - Numeric validation is implemented by attempting to parse as Long
- Custom
PhoneNumberExceptionis thrown for invalid formats - The VARCHAR(20) size is appropriate as it allows for:
- Standard 11-digit Korean phone numbers
- Future flexibility for international formats
- Storage of formatting characters if needed
The validation code in UserUtils.java:
public void checkPhoneNumber(String phoneNumber) {
if (phoneNumber.length() != 11)
throw new PhoneNumberException();
try {
Long.parseLong(phoneNumber);
} catch (Exception e) {
throw new PhoneNumberException();
}
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any existing phone number validation patterns in the codebase
rg -l "phone.*validation|phone.*pattern|phone.*format"
# Check for similar field length constraints in other tables
ast-grep --pattern 'VARCHAR(255)'
Length of output: 92
Script:
#!/bin/bash
# Let's try a broader search for phone-related validations and constraints
rg -i "phone.*number|phone.*validation|phone.*format" -A 3
# Search for all VARCHAR definitions in SQL migrations to understand the patterns
fd -e sql -x cat {} | grep -i "varchar"
# Check for any entity/model definitions that might handle phone numbers
rg -i "class.*|phone.*number" -t java -t kotlin -A 3
Length of output: 314580
src/main/java/com/postgraduate/global/auth/login/application/dto/req/SignUpRequest.java (1)
Line range hint 7-16: Verify the impact of removed fields
The removal of major, field, and matchingReceive fields suggests significant changes in the signup flow. Let's verify that all dependent code has been updated accordingly.
src/main/java/com/postgraduate/domain/wish/domain/service/WishSaveService.java (1)
1-16: Consider if this thin service layer adds value.
The current service is a thin wrapper around the repository with a single method. Consider either:
- Adding more business logic to justify the service layer, or
- Moving this functionality directly to the
WishManageUseCaseif no additional business logic is planned.
Let's verify if there are plans for additional business logic in this service:
src/main/java/com/postgraduate/domain/member/user/domain/service/UserSaveService.java (1)
3-4: LGTM: Clean dependency injection for member role management
The addition of MemberRole related dependencies aligns well with the architectural changes to separate user roles into a dedicated table.
Also applies to: 14-14
src/main/java/com/postgraduate/domain/wish/application/mapper/WishMapper.java (1)
1-23: Verify the integration with WishCreateRequest.
The mapper assumes certain fields are present in WishCreateRequest. Let's verify the contract.
✅ Verification successful
The WishCreateRequest contract is correctly implemented and used.
The verification shows that WishCreateRequest is a record with all the required fields (field, postgradu, lab, phoneNumber) that match exactly with the mapper's usage. The integration flow is also consistent across the codebase:
- WishController receives the request
- Delegates to WishManageUseCase
- WishManageUseCase uses WishMapper to convert the request
- All required fields are properly accessed in the mapper
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the structure of WishCreateRequest and its usage
# Find WishCreateRequest definition
echo "Searching for WishCreateRequest definition..."
ast-grep --pattern 'record WishCreateRequest($$$)' || \
ast-grep --pattern 'class WishCreateRequest {
$$$
}'
# Find all usages of WishCreateRequest
echo -e "\nSearching for WishCreateRequest usages..."
rg "WishCreateRequest" -A 5
Length of output: 5450
src/main/java/com/postgraduate/domain/wish/domain/entity/Wish.java (1)
Line range hint 1-42: Verify the removal of User relationship
The removal of the User relationship raises some concerns about data integrity and user association.
Let's verify the impact:
Please ensure:
- How wishes are now associated with users
- Whether there's a migration plan for existing wish-user relationships
- How orphaned wishes are prevented
✅ Verification successful
User relationship removal is properly handled
The verification shows that the system has been properly restructured to handle wishes without direct user relationships:
- The migration script (V2_202411150733__modify_wish.sql) cleanly removes old columns and adds new ones without leaving any user foreign key dependencies
- Wishes are now associated with users through the phone number field, which serves as a logical link
- The WishMapper handles both authenticated (User object present) and unauthenticated scenarios gracefully
- The WishController and WishManageUseCase maintain user context for authenticated requests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining references to User in Wish-related files
echo "Checking for User references in Wish-related files..."
rg -l "User" --type java src/main/java/com/postgraduate/domain/wish/
# Check for any foreign key constraints in database migrations
echo "Checking for related database migrations..."
fd -e sql . src/main/resources/db/migration/
Length of output: 958
Script:
#!/bin/bash
# Check the content of the migration file that modifies the wish table
echo "Content of wish table modification:"
cat src/main/resources/db/migration/V2_202411150733__modify_wish.sql
# Check how wishes are associated with users in the controller and use case
echo -e "\nWish controller implementation:"
rg -A 10 "class WishController" src/main/java/com/postgraduate/domain/wish/presentation/WishController.java
echo -e "\nWish use case implementation:"
rg -A 10 "class WishManageUseCase" src/main/java/com/postgraduate/domain/wish/application/usecase/WishManageUseCase.java
# Check if there are any JPA relationships or user references in mapper
echo -e "\nWish mapper implementation:"
cat src/main/java/com/postgraduate/domain/wish/application/mapper/WishMapper.java
Length of output: 2732
src/main/java/com/postgraduate/admin/domain/service/AdminUserService.java (1)
16-18: LGTM! Clean transition from Wish to User entity
The method signature change aligns well with the architectural shift from wish-centric to user-centric model.
src/main/java/com/postgraduate/domain/member/user/application/usecase/UserMyPageUseCase.java (1)
23-25: LGTM! Good simplification of role checking logic.
The change to use user.isSenior() improves encapsulation and makes the code more maintainable by removing direct role enum dependency.
Let's verify the implementation of the isSenior() method:
✅ Verification successful
Role checking implementation verified and correctly used
The isSenior() method in User entity is properly implemented and checks for the SENIOR role in the user's roles collection. The method is consistently used across the codebase for role verification in various use cases including:
- Authentication (AuthDetails)
- JWT token generation
- User management
- Senior-specific operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of isSenior() method in User class
# Expected: Find the implementation to ensure it correctly checks the user's role
# Search for User class definition and isSenior method
ast-grep --pattern 'class User {
$$$
isSenior() {
$$$
}
$$$
}'
# Search for any tests verifying this method
rg -l "isSenior.*Test"
Length of output: 101
Script:
#!/bin/bash
# Let's try different approaches to find the User class and isSenior method
# Search for User class files
fd -e java User
# Search for isSenior method implementation using ripgrep
rg -A 5 "isSenior\(\)"
# Search for Role enum or role-related code
rg -A 3 "enum.*Role"
# Search for test files containing "Senior" to find related tests
rg -l "Senior.*Test"
Length of output: 11558
src/main/java/com/postgraduate/admin/domain/repository/AdminUserRepository.java (1)
18-21:
Review the commented role filter and current implementation
The current implementation returns all users without role filtering, which might expose more data than intended. Additionally, there's commented out code that suggests a role-based filter was planned.
Consider either:
- Implementing the role filter if it's required for security
- Removing the commented code if the filter is intentionally omitted
Let's verify the role-based access implementation:
src/main/java/com/postgraduate/domain/wish/application/usecase/WishManageUseCase.java (2)
1-12: LGTM! Clean package structure and imports.
The package structure follows clean architecture principles with proper separation of concerns.
17-19: Verify error handling for external message service.
The BizppurioJuniorMessage service appears to be an external messaging service. Ensure that error handling is implemented in the service to handle potential communication failures gracefully.
✅ Verification successful
Let me try a different approach to search for error handling patterns in the codebase.
Let me check the BizppurioSend implementation since it contains the error handling logic through sendMessageWithExceptionHandling.
Let me check the complete error handling implementation in BizppurioSend since we only see part of the try block.
Error handling for external message service is properly implemented
The BizppurioJuniorMessage service has comprehensive error handling through its dependency BizppurioSend:
- Catches all exceptions during message sending and logs the error
- Notifies through Slack when errors occur
- Validates response codes and logs failures
- Uses reactive programming with proper error handling
- Provides detailed logging for both success and failure cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling in BizppurioJuniorMessage implementation
ast-grep --pattern 'class BizppurioJuniorMessage {
$$$
matchingWaiting($_) {
try {
$$$
} catch ($$$) {
$$$
}
}
}'
Length of output: 145
Script:
#!/bin/bash
# Search for BizppurioJuniorMessage class implementation
rg -l "class BizppurioJuniorMessage"
# Search for try-catch blocks in files containing BizppurioJuniorMessage
rg "class BizppurioJuniorMessage" -A 30
# Search for exception handling related to messaging
rg "throw.*Message|catch.*Message" -A 2
Length of output: 4410
Script:
#!/bin/bash
# Search for BizppurioSend implementation
rg "class BizppurioSend" -A 20
# Search for sendMessageWithExceptionHandling implementation
ast-grep --pattern 'sendMessageWithExceptionHandling($_) {
$$$
}'
Length of output: 2838
Script:
#!/bin/bash
# Get the complete implementation of sendMessageWithExceptionHandling
rg "sendMessageWithExceptionHandling" -A 30 src/main/java/com/postgraduate/global/bizppurio/application/usecase/BizppurioSend.java
Length of output: 1455
src/main/java/com/postgraduate/domain/wish/presentation/WishController.java (2)
1-15: LGTM! Well-organized imports and package structure.
The imports are clean, necessary, and properly organized. The inclusion of Swagger annotations indicates good API documentation practices.
21-22: LGTM! Good use of constructor injection.
The dependency injection is implemented correctly using constructor injection via @RequiredArgsConstructor, which is a recommended practice.
src/main/java/com/postgraduate/domain/member/user/domain/service/UserDeleteService.java (2)
21-21: LGTM: New dependency properly integrated
The MemberRoleRepository is correctly injected using constructor injection via @requiredargsconstructor.
32-32: Verify cascade delete behavior
The current implementation manually deletes roles before deleting the user. This might be better handled through JPA cascade operations.
Let's check the entity relationships:
✅ Verification successful
From the search results, I can see the important details about the User-Role relationship:
- In
User.java, roles are defined as:
@OneToMany(mappedBy = "user", fetch = FetchType.EAGER)
private List<MemberRole> roles = new ArrayList<>();- In
MemberRole.java, the relationship is bidirectional:
@ManyToOne(fetch = FetchType.LAZY)
@JoinColumn(name = "user_id")
private User user;The current implementation manually deletes roles because:
- The
@OneToManyrelationship doesn't specify any cascade type - The roles are a separate entity with their own repository (
MemberRoleRepository) - The manual deletion ensures proper cleanup of role entries
Manual deletion is the correct approach here
The current implementation is appropriate because:
- It provides explicit control over the deletion process
- It avoids potential cascading issues with other relationships
- It follows the repository pattern where each entity's lifecycle is managed by its respective repository
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check User entity for @OneToMany relationship with roles
rg -A 5 "@OneToMany.*roles"
# Check if there are any cascade delete configurations
rg -A 5 "CascadeType\..*ALL|REMOVE"
Length of output: 67
Script:
#!/bin/bash
# Let's try a broader search for entity relationships and role-related fields
rg -A 10 "class User.*\{|class.*User\{"
# Also search for any role-related fields or annotations
rg -A 5 "@.*ToMany|@.*ToOne.*role|private.*[Rr]ole"
# Check for repository delete methods
rg -A 5 "delete.*Role|deleteBy.*Role"
Length of output: 29444
src/main/java/com/postgraduate/global/config/security/jwt/auth/AuthDetails.java (2)
13-14: LGTM! Clean import organization
The static import of Role enum constants improves code readability in the authority determination logic.
22-26: LGTM! Well-structured authority determination logic
The new implementation properly handles role hierarchy (admin > senior > user) and follows Spring Security best practices by using immutable authority collections.
Let's verify the role hierarchy implementation:
✅ Verification successful
Based on the shell script results, I can now generate the final response as we have sufficient information to verify the role hierarchy implementation:
Role hierarchy implementation is correctly enforced with proper authorization
The implementation shows a well-structured role hierarchy where:
- The
Userclass correctly implements role checks viaisAdmin(),isSenior(), andisUser()methods by checking role membership - The
SecurityConfigproperly enforces role-based access with consistent authority checks:- Admin endpoints (
/admin/**,/adminServer/**) requireADMINauthority - Senior endpoints (
/senior/**,/salary/**) requireSENIORauthority - User endpoints (
/user/**,/mentoring/applying) requireUSERauthority
- Admin endpoints (
- The
AuthDetailsclass implements authority assignment following the same hierarchy (admin > senior > user)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the role checks implementation in User class and role hierarchy consistency
# Check the implementation of isAdmin() and isSenior() methods
ast-grep --pattern 'class User {
$$$
boolean isAdmin() {
$$$
}
$$$
}'
ast-grep --pattern 'class User {
$$$
boolean isSenior() {
$$$
}
$$$
}'
# Check for any other authority/role checks to ensure consistency
rg -g '*.java' -A 3 'getAuthorities|GrantedAuthority|Role\.'
Length of output: 28841
src/main/java/com/postgraduate/domain/member/user/domain/service/UserUpdateService.java (2)
5-6: LGTM: Clean dependency injection for role management
The addition of MemberRole related dependencies is well-structured and follows Spring best practices for dependency injection.
Also applies to: 16-16
21-30: Verify the role assignment flow in the codebase
Let's ensure that the role assignment methods are being used correctly throughout the codebase.
✅ Verification successful
Let me gather more information about the role-related implementations to better understand the flow.
Let me check one more aspect to understand the role enum and any validation logic.
Role assignment implementation is properly structured and secure
The role assignment flow is well-implemented with proper encapsulation:
- Role updates are consistently handled through the service layer (
UserUpdateService,UserSaveService) - The
Roleenum is properly defined with clear roles (ADMIN, USER, SENIOR) - Profile updates for senior roles are properly managed with validation checks
- Role assignments are used correctly in the signup flow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential issues in role assignment usage
# Check for direct role updates bypassing these methods
rg -t java "updateRole|setRole" --glob "!UserUpdateService.java"
# Check for proper error handling when calling these methods
ast-grep --pattern 'try {
$$$
addJuniorRole($$$)
$$$
} catch ($$$) {
$$$
}'
ast-grep --pattern 'try {
$$$
addSeniorRole($$$)
$$$
} catch ($$$) {
$$$
}'
Length of output: 608
Script:
#!/bin/bash
# Check the User entity implementation for role-related methods
rg -t java "class User" -A 10 src/main/java/com/postgraduate/domain/member/user/domain/entity/User.java
# Check where these service methods are being called
rg -t java "addJuniorRole|addSeniorRole" -A 2
# Check MemberRole implementation
rg -t java "class MemberRole" -A 10
# Look for any role-related exceptions
rg -t java "Exception.*Role|Role.*Exception"
Length of output: 3763
Script:
#!/bin/bash
# Check Role enum implementation
rg -t java "enum Role" -A 10
# Check for any role-related validation or business logic
rg -t java "isDefaultProfile|updateProfile" -A 2
# Check profile utils implementation
rg -t java "class ProfileUtils" -A 10
Length of output: 4250
src/test/java/com/postgraduate/domain/user/application/usecase/UserMyPageUseTypeTest.java (1)
33-33: LGTM! Constructor changes align with wish separation.
The removal of the Wish parameter from User constructor calls is consistent with the PR objective of separating wish functionality.
Also applies to: 76-76
src/test/java/com/postgraduate/domain/user/domain/service/UserUpdateServiceTest.java (1)
52-59:
Remove commented test and add new role management tests.
The commented-out test should be removed and replaced with new tests that verify the updated role management functionality using the new member_role table structure. This ensures proper test coverage for the refactored role management system.
Let's verify if there are other role-related tests in the codebase:
src/main/java/com/postgraduate/global/auth/login/application/usecase/jwt/JwtUseCase.java (3)
31-33: LGTM! Good improvement in role checking logic.
The change to use isSenior() and isAdmin() methods improves encapsulation and maintainability by moving role checking logic to the User entity.
58-58: LGTM! Consistent role checking implementation.
The use of isSenior() method with specific exception handling maintains consistency with the role checking pattern used throughout the class.
43-43: Verify the Redis check implementation in JwtUtils.
The change from checkPast to checkRedis suggests a shift in token validation strategy. Let's verify the implementation.
✅ Verification successful
Based on the search results, I can now generate the final response as the implementation details are clear from the code.
The Redis check implementation is correctly implemented and consistent
The change from checkPast to checkRedis is valid as the implementation in JwtUtils properly:
- Retrieves the token from Redis using
RedisRepository - Validates the token against the request
- Handles token cleanup appropriately
The Redis infrastructure is also properly configured with:
RedisConfigfor connection setupRedisRepositoryfor token operations- Proper test coverage in
JwtUseTypeTest
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the checkRedis method implementation in JwtUtils
ast-grep --pattern 'class JwtUtils {
$$$
checkRedis($$$) {
$$$
}
$$$
}'
# Search for any Redis-related configurations
rg -g '*.{java,yml,properties}' -i 'redis|cache'
Length of output: 9984
src/test/java/com/postgraduate/domain/user/domain/service/UserGetServiceTest.java (1)
36-36: LGTM! Constructor parameter update is correct.
The User constructor call has been properly updated to align with the removal of the Wish parameter, maintaining all necessary fields in the correct order.
src/test/java/com/postgraduate/domain/auth/application/usecase/oauth/kakao/KakaoSignInUseTypeTest.java (2)
49-49: LGTM! Constructor update aligns with wish separation.
The User constructor call has been properly updated to reflect the removal of the Wish parameter, maintaining all necessary fields for the test scenarios.
48-50: Verify test coverage for user roles
While the basic test scenarios are covered, consider adding test cases to verify the behavior with different user roles, especially given the new member_role table mentioned in the PR objectives.
Consider adding test cases for:
- Multiple role scenarios
- Role changes during user lifecycle
- Edge cases with role permissions
✅ Verification successful
Based on the shell script results, I can now provide a final response:
Test coverage for user roles is already well-implemented
The codebase shows comprehensive test coverage for different user roles (USER, SENIOR, ADMIN) across multiple test files:
- Role-specific authentication tests in
AuthControllerTest.java - Role transition tests (USER to SENIOR and vice versa) in
JwtUseTypeTest.java - Role-based authorization tests in
SecurityConfig.java - Role validation tests in
UserGetServiceTest.javaandUserUpdateServiceTest.java
The new member_role table is properly tested through:
- Role mapping tests in
UserMapper.java - Role verification tests in
User.java(isUser(), isSenior(), isAdmin()) - Role transition tests in various use cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing role-related test coverage
# Look for other test files that might need similar updates
fd -e java -E "build/*" | xargs rg -l "Role\."
# Check for existing role-specific test cases
rg -A 5 "USER|ADMIN|Role\." --type java
Length of output: 111692
src/test/java/com/postgraduate/domain/payment/usecase/PaymentManageUseTypeTest.java (1)
66-66: LGTM! Constructor changes align with wish functionality separation
The removal of the last parameter from User constructor calls is consistent with the PR's objective of separating the wish functionality.
Also applies to: 69-69
src/test/java/com/postgraduate/support/Resource.java (1)
29-32: LGTM! User and Wish initialization changes look good
The initialization of User and Wish objects has been properly updated to reflect the new domain separation. The test fixtures maintain good coverage with meaningful test data.
src/main/java/com/postgraduate/domain/member/senior/application/usecase/SeniorInfoUseCase.java (2)
27-27: LGTM! Good encapsulation of role-checking logic.
The change from a direct role comparison to using isSenior() method improves encapsulation and aligns well with the new multi-role model.
27-28: Verify the implementation of User.isSenior()
Since this code relies on the new isSenior() method, let's verify its implementation to ensure it correctly handles the multi-role scenario.
✅ Verification successful
Implementation of User.isSenior() is correct and properly handles role checking
The implementation of isSenior() in User.java correctly checks for the SENIOR role using the roles collection:
public boolean isSenior() {
return roles.stream()
.map(MemberRole::getRole)
.toList()
.contains(Role.SENIOR);
}The method properly:
- Streams through the user's roles collection
- Maps each MemberRole to its Role enum value
- Checks if SENIOR role is present
This implementation correctly handles the multi-role scenario by checking the presence of SENIOR role in the user's assigned roles.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find and verify the implementation of User.isSenior() method
# Expected: Method should check the member_role table for SENIOR role
# Search for User class implementation
echo "Searching for User.isSenior() implementation:"
ast-grep --pattern 'class User {
$$$
isSenior() {
$$$
}
$$$
}'
# Search for any tests covering this method
echo -e "\nSearching for related tests:"
rg -l "isSenior.*test" -g "**/*Test.java"
Length of output: 294
Script:
#!/bin/bash
# Let's try different approaches to find the User class and isSenior method
# Search for User class files
echo "Searching for User class files:"
fd -e java User
# Search for isSenior method using ripgrep with context
echo -e "\nSearching for isSenior method implementation:"
rg "isSenior" -t java -A 5 -B 2
# Search for Role or MemberRole related files
echo -e "\nSearching for Role related files:"
fd -e java "Role|MemberRole"
Length of output: 13157
src/main/java/com/postgraduate/batch/scheduler/JobSchedulerConfig.java (1)
64-64: Consider passing job parameters for consistent timing checks
The job verification uses different timestamps than the initial job execution, which could lead to inconsistencies in job identification and status checking. Consider passing the original JobParameters to maintain timing consistency.
Also applies to: 72-72
src/main/java/com/postgraduate/global/auth/login/application/usecase/oauth/kakao/KakaoSignOutUseCase.java (1)
Line range hint 92-97: LGTM! Verify consistent role checking across the codebase.
The change from direct role comparison to using isSenior() aligns well with the PR's objective of separating user roles into a new table. This encapsulation improves maintainability by centralizing role-checking logic.
Let's verify that this pattern is consistently applied across the codebase:
✅ Verification successful
LGTM! Role checking is consistently implemented using isSenior()
The verification confirms that:
- No direct role comparisons using
getRole().equals(Role.SENIOR)were found in the codebase - The
isSenior()method is consistently used across all relevant components:- Authentication (JWT, Kakao OAuth)
- User management (MyPage, Senior Info)
- Security configuration
- Account deletion logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining direct role comparisons that should use isSenior()
# Expected: No direct role comparisons should be found, all should use isSenior()
# Search for direct role comparisons
rg -l "getRole\(\)\.equals\(Role\.SENIOR\)" || echo "No direct role comparisons found ✓"
# Search for the new pattern to ensure it's used consistently
rg "isSenior\(\)" -A 3
Length of output: 5057
src/main/java/com/postgraduate/global/auth/login/application/usecase/oauth/SignUpUseCase.java (2)
3-3: LGTM! Clean import organization
The new imports and static constants align well with the architectural changes, making the code more maintainable.
Also applies to: 29-31
54-56: Verify user role assignment across the application
The changes look good and align with the new architecture. The method now has a cleaner responsibility of user creation with role assignment.
✅ Verification successful
Let me gather more information about the role assignment implementation to ensure consistency.
Let me check the actual implementation of the mapToRole method and role-related entities to complete the verification.
Role assignment implementation is consistent and well-structured
The verification shows that:
- Role assignment follows a consistent pattern using
mapToRole()method - Role types are properly defined as
ADMIN, USER, SENIORin theRoleenum MemberRoleentity correctly maintains the relationship between users and roles- Role assignment is handled consistently through
UserSaveServiceandUserUpdateService
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent role assignment pattern across the codebase
rg -A 2 "mapToRole\(USER,"
Length of output: 869
Script:
#!/bin/bash
# Check the mapToRole method implementation and other role-related patterns
ast-grep --pattern 'mapToRole($$$)'
# Check for other role assignment patterns
rg -A 3 "MemberRole.*=.*"
# Look for role-related service implementations
rg -A 5 "class.*UserSaveService|class.*UserUpdateService"
Length of output: 5625
Script:
#!/bin/bash
# Check the mapToRole method implementation
ast-grep --pattern 'private MemberRole mapToRole($$$)'
# Check MemberRole entity structure
rg -A 10 "class MemberRole"
# Check role types/enums
rg -A 5 "enum.*Role"
Length of output: 1560
src/test/java/com/postgraduate/domain/auth/application/usecase/jwt/JwtUseTypeTest.java (3)
44-44: LGTM: User construction changes are consistent
The User constructor calls have been properly updated to remove the Wish parameter, aligning with the PR's objective of wish separation. The parameter structure is consistent across all instantiations.
Also applies to: 70-70, 111-111
Line range hint 1-161: LGTM: Well-structured test coverage
The test class demonstrates good practices:
- Clear test method names and descriptions
- Comprehensive coverage of success and failure scenarios
- Proper use of mocking and verification
- Consistent test structure across scenarios
125-125: Verify Redis implementation for token validation
The change from checkPast to checkRedis suggests a shift to Redis-based token validation. While the test coverage is comprehensive, we should ensure the Redis implementation is properly configured.
Let's check the Redis configuration and implementation:
Also applies to: 131-131, 146-146, 157-157
✅ Verification successful
Let me gather more specific information about the Redis implementation.
Redis implementation for token validation is properly configured and implemented
The change from checkPast to checkRedis is valid and well-implemented:
- Redis configuration is properly set up with connection, template, and serialization settings
- RedisRepository provides necessary token operations including blacklist checks
- JwtUtils correctly integrates with Redis for token validation through
checkRedismethod which:- Validates refresh tokens against Redis storage
- Handles token expiration and invalidation cases
- Throws appropriate exceptions for invalid/expired tokens
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Redis configuration
rg -l "Redis|RedisTemplate|RedisConnection" --type java
# Search for related JWT validation implementations
ast-grep --pattern 'class $_ implements $_ {
$$$
checkRedis($_, $_) {
$$$
}
$$$
}'
Length of output: 767
Script:
#!/bin/bash
# Check Redis configuration details
cat src/main/java/com/postgraduate/global/config/redis/RedisConfig.java
# Check Redis repository implementation
cat src/main/java/com/postgraduate/global/config/redis/RedisRepository.java
# Check JWT utils implementation for Redis check
cat src/main/java/com/postgraduate/global/config/security/jwt/util/JwtUtils.java
Length of output: 11327
src/main/java/com/postgraduate/global/config/security/jwt/util/JwtUtils.java (1)
Line range hint 96-104: Verify consistent token validation patterns
Let's ensure the token validation approach is consistent across the codebase.
✅ Verification successful
Let me gather more information about the token validation patterns by examining the implementation details.
Let me try one more search to understand the token validation flow better.
Token validation implementation is consistent and well-structured
After examining the JwtUtils class and related components, the token validation approach is consistent and properly implemented:
- Token validation follows a clear pattern with proper separation between access and refresh tokens
- Redis is correctly used for refresh token storage with proper expiration handling
- Exception handling is consistent across all token validation scenarios
- The checkRedis method properly validates refresh tokens against Redis storage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other token validation patterns
echo "Searching for token validation patterns..."
rg -l "request.getHeader.*Authorization" --type java
# Search for similar error handling
echo "Searching for token-related exception handling..."
rg -l "InvalidRefreshTokenException|InvalidTokenException" --type java
# Search for other Redis token operations
echo "Searching for Redis token operations..."
rg "redisRepository\.(get|set|delete)Values.*REFRESH" --type java
Length of output: 1335
Script:
#!/bin/bash
# Check the JwtUtils class implementation for token validation patterns
echo "Checking JwtUtils implementation..."
rg -A 5 "request\.getHeader.*Authorization" src/main/java/com/postgraduate/global/config/security/jwt/util/JwtUtils.java
# Check token validation in tests
echo "Checking test implementations..."
rg -A 5 "request\.getHeader.*Authorization" src/test/java/com/postgraduate/domain/auth/application/usecase/jwt/JwtUseTypeTest.java
# Look for other JWT-related utility methods
echo "Checking JWT-related utility methods..."
ast-grep --pattern 'public $_ parse$_($$$) { $$$ }' src/main/java/com/postgraduate/global/config/security/jwt/util/JwtUtils.java
Length of output: 631
Script:
#!/bin/bash
# Check the entire JwtUtils class content
echo "Examining JwtUtils class..."
cat src/main/java/com/postgraduate/global/config/security/jwt/util/JwtUtils.java
# Look for token validation related interfaces/classes
echo "Checking token validation related files..."
rg -l "Token.*Service|Token.*Provider" --type java
# Check exception handling patterns
echo "Checking exception handling..."
rg -A 3 "throw new (Invalid(Refresh)?Token|NoneRefreshToken)Exception" --type java
Length of output: 8989
src/test/java/com/postgraduate/domain/mentoring/application/usecase/MentoringApplyingUseTypeTest.java (2)
80-81: LGTM: User constructor update for senior role
The simplified User constructor call correctly reflects the removal of Wish dependency while maintaining necessary test data setup.
83-84: LGTM: User constructor update for junior role
The simplified User constructor call correctly reflects the removal of Wish dependency while maintaining necessary test data setup.
src/test/java/com/postgraduate/domain/senior/application/usecase/SeniorMyPageUseTypeTest.java (2)
58-58: LGTM: User constructor change aligns with Wish removal
The removal of the last parameter from the User constructor is consistent with the broader refactoring to separate Wish functionality.
Line range hint 89-98: Critical: Restore or replace commented-out test methods
Several important test methods are commented out:
getSeniorMyPageProfile: Tests senior profile viewinggetSeniorMyPageUserAccountWithAccount: Tests account information retrievalcheckUserandcheckUserWithNull: Tests user verification logic
Commenting out tests reduces coverage and could hide potential issues. These tests should either be:
- Updated to match the new architecture and re-enabled
- Replaced with new tests that cover the refactored functionality
- Explicitly marked for removal with a clear explanation
Let's check the test coverage impact:
Also applies to: 147-165, 169-191
src/test/java/com/postgraduate/domain/senior/application/usecase/SeniorInfoUseTypeTest.java (1)
63-63: LGTM! User constructor changes align with wish separation.
The removal of the Wish parameter from the User constructor calls aligns with the PR objectives of separating the wish functionality.
Let's verify that all User constructor calls in test classes have been updated consistently:
Also applies to: 66-66
✅ Verification successful
All User constructor calls have been updated correctly without Wish parameter
After reviewing all User constructor calls in the codebase, I can confirm that none of them include a Wish parameter. The constructor calls consistently follow the updated signature with parameters for id, providerId, email, name, phone, profile, point, role, status, and timestamps, aligning with the wish separation objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining User constructor calls that might still include the Wish parameter
ast-grep --pattern 'new User($$$, wish)'
Length of output: 42
Script:
#!/bin/bash
# Search for User constructor calls in test files
rg "new User\(" --type java -A 3 -B 1
Length of output: 18477
src/main/java/com/postgraduate/domain/payment/application/usecase/PaymentManageUseCase.java (2)
104-104: Good improvement in role check encapsulation!
The change from direct role comparison to using isAdmin() method improves encapsulation and aligns well with the new multi-role model.
104-104: Verify the implementation of isAdmin() method
Since the system has moved to a multi-role model, we should verify that isAdmin() correctly handles the presence of admin role in the user's role collection.
✅ Verification successful
The isAdmin() implementation correctly handles the multi-role model
The implementation in User.java correctly checks for the ADMIN role in the user's role collection:
public boolean isAdmin() {
return roles.stream()
.map(MemberRole::getRole)
.toList()
.contains(Role.ADMIN);
}The code:
- Uses a collection of
MemberRoleentities to support multiple roles - Maps each
MemberRoleto its underlyingRoleenum value - Checks if
Role.ADMINis present in the resulting list
This aligns with the multi-role model as:
- The
Roleenum properly definesADMINas a distinct role - The implementation correctly handles role membership through a collection
- Security configuration (
SecurityConfig.java) properly enforces ADMIN authority checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of isAdmin() method in User class
# Expected: Method should check for ADMIN role in the user's role collection
# Search for the isAdmin implementation
ast-grep --pattern 'class User {
$$$
isAdmin() {
$$$
}
$$$
}'
# Search for any tests verifying admin role checks
rg -l "test.*[Aa]dmin.*[Rr]ole|[Rr]ole.*[Aa]dmin" --type java
Length of output: 139
Script:
#!/bin/bash
# Let's try different approaches to find the User class and isAdmin implementation
# Search for User class files
fd -e java "User.java"
# Search for isAdmin method implementation using ripgrep
rg "isAdmin.*\(" --type java -A 5
# Search for role-related code in User class
rg "class.*User" --type java -A 20
# Search for role-related enums or constants
fd -e java "Role.java|UserRole.java"
rg "enum.*Role|ADMIN" --type java -A 3
Length of output: 68191
src/test/java/com/postgraduate/domain/mentoring/application/usecase/MentoringUserInfoUseTypeTest.java (2)
69-69: LGTM! User constructor changes align with Wish entity removal.
The removal of the Wish parameter from User construction aligns with the architectural changes described in the PR objectives. The test cases continue to function correctly with this simplified User model.
Also applies to: 72-72
Line range hint 85-184: Test coverage looks comprehensive.
The test class provides good coverage with test cases for:
- Detail retrieval (success and failure cases)
- Waiting mentoring list
- Expected mentoring list
- Done mentoring list
Each test properly verifies the expected behavior using appropriate assertions.
src/main/java/com/postgraduate/admin/application/mapper/AdminMapper.java (2)
4-4: LGTM!
The added import for Status is correctly used in the mapToUserInfo method.
44-54:
Critical: Verify impact of hardcoded values after Wish removal
The refactoring from Wish to User has introduced several hardcoded values that could impact admin functionality:
- Fixed
wishIdof1L - Static
Status.MATCHED - Static
falsevalues
The TODO comment suggests this is temporary, but these static values could lead to incorrect data being displayed in the admin interface.
Let's verify the usage of this DTO to understand the impact:
Please provide the intended business logic for these fields to help implement a proper solution.
src/test/java/com/postgraduate/domain/senior/application/usecase/SeniorManageUseTypeTest.java (1)
88-88: LGTM: User constructor change aligns with wish functionality removal
The removal of the last parameter from the User constructor is consistent with the PR's objective of separating the wish functionality.
src/main/java/com/postgraduate/global/auth/login/presentation/AuthController.java (1)
100-103: Verify the role change implementation across the codebase
The role change implementation needs verification to ensure it properly handles the transition to the new member_role table mentioned in the PR objectives.
✅ Verification successful
Role change implementation is properly aligned with database migration
The verification shows a well-structured implementation:
- The new
member_roletable is properly created with appropriate role enums and foreign key constraints - Data migration handles the transition of existing roles
- The
SignUpUseCase.changeUser()correctly maps users to the new role structure usingMemberRole - The
JwtUseCase.changeUser()provides proper validation for junior status
The implementation aligns with the PR objectives of:
- Separating role management into a dedicated table
- Handling role transitions
- Maintaining proper validation checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the role change implementation and its dependencies
# Check for role-related database migrations
echo "Checking for role-related migrations..."
fd -e sql | xargs grep -l "member_role"
# Check for role-related entity definitions
echo "Checking for role entity definitions..."
ast-grep --pattern 'class $_ {
$$$
@Entity
$$$
member_role
$$$
}'
# Check for role change implementations
echo "Checking for role change implementations..."
rg -l "changeUser|member_role" --type java
Length of output: 983
Script:
#!/bin/bash
# Let's examine the migration files and implementation details
# Check the content of migration files
echo "=== Migration Files Content ==="
cat src/main/resources/db/migration/V2_202411140644__create_table.sql
cat src/main/resources/db/migration/V2_202411150555__migration_data.sql
# Check the SignUpUseCase implementation
echo -e "\n=== SignUpUseCase Implementation ==="
rg -A 10 "changeUser" src/main/java/com/postgraduate/global/auth/login/application/usecase/oauth/SignUpUseCase.java
# Check the JwtUseCase implementation
echo -e "\n=== JwtUseCase Implementation ==="
rg -A 10 "changeUser" src/main/java/com/postgraduate/global/auth/login/application/usecase/jwt/JwtUseCase.java
# Look for role-related entity classes
echo -e "\n=== Role Entity Classes ==="
ast-grep --pattern 'class $_ {
$$$
@Entity
$$$
Role
$$$
}'
Length of output: 2196
src/test/java/com/postgraduate/domain/mentoring/application/usecase/MentoringSeniorInfoUseTypeTest.java (2)
Line range hint 1-185: Test coverage and structure look good!
The test class provides comprehensive coverage of mentoring scenarios with proper test organization, clear naming, and appropriate use of mocks. The test methods effectively validate both success and failure cases.
70-70: Verify role handling with new member_role table
The User constructor calls have been updated to remove the Wish parameter, which aligns with the architectural changes. However, since the PR mentions separating the user role column into a new member_role table, we should verify that the role assignment in these test cases (SENIOR, USER) properly integrates with the new member_role table implementation.
Also applies to: 73-73
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Test setup aligns with new role implementation
The test setup is correct and consistent with the new member_role table architecture:
- The User entity now properly manages roles through a List with dedicated methods (isUser, isSenior, isAdmin)
- The Role parameter in User constructor is used for initial role assignment, which is then managed by the updateRole method
- Migration scripts (V2_202411140644__create_table.sql, V2_202411150555__migration_data.sql) confirm the member_role table implementation
- All test files consistently use the same User constructor pattern across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if User entity and tests have been updated consistently with the new member_role table
# Check for member_role related changes
echo "Checking for member_role table definition..."
fd -e java -e sql . | xargs rg -l "member_role"
# Check for role-related changes in User entity
echo "Checking User entity implementation..."
fd "User.java" | xargs rg "Role|role" -A 3
# Check for other test files that might need similar updates
echo "Checking other test files with User construction..."
fd -e java . | xargs rg "new User\("
Length of output: 5870
src/test/java/com/postgraduate/domain/auth/presentation/AuthControllerTest.java (2)
93-93: LGTM! SignUpRequest simplification aligns with PR objectives
The simplified SignUpRequest constructor aligns with the PR objective of streamlining user registration by removing the junior member application process.
Line range hint 1-300: Verify test coverage for new MemberRole functionality
While the test suite comprehensively covers authentication flows, we should verify coverage for the new MemberRole functionality mentioned in the PR objectives.
✅ Verification successful
Based on the previous search results, I can now generate the final response:
Test coverage for Role functionality is adequately implemented
The codebase shows comprehensive test coverage for Role-related functionality across multiple test classes:
- Role enum is properly tested in authentication flows (AuthControllerTest)
- Role transitions are verified (USER to SENIOR and vice versa) in multiple test classes
- Role-based authorization is tested with @WithMockUser annotations
- Token generation with roles is verified in JwtUseTypeTest
- Role checks are implemented in user and senior controller tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test coverage of MemberRole-related functionality
# Look for test files related to MemberRole
fd -e java -e kt "MemberRole.*Test"
# Search for MemberRole usage in test files
rg -t java -t kotlin "MemberRole" --type-add 'kotlin:*.kt' -g '*Test.{java,kt}'
Length of output: 117
Script:
#!/bin/bash
# Let's first check if there are any references to Role or roles in test files
rg -t java -t kotlin "Role" --type-add 'kotlin:*.kt' -g '*Test.{java,kt}' -B 2 -A 2
# Also check for any enum definitions related to roles
ast-grep --pattern 'enum $_ {
$$$
}'
Length of output: 28487
Script:
#!/bin/bash
# Let's check for the Role enum definition and its usage
ast-grep --pattern 'enum Role {
$$$
}'
# Also check for any test files that might be testing role-related functionality
fd -e java -e kt "Role.*Test|.*RoleTest"
# Search for role-related test methods
rg -t java -t kotlin "test.*role|role.*test" -i --type-add 'kotlin:*.kt' -g '*Test.{java,kt}' -B 2 -A 2
Length of output: 191
src/test/java/com/postgraduate/domain/mentoring/application/usecase/MentoringManageUseTypeTest.java (2)
Line range hint 1-300: LGTM! Well-structured test implementation
The test class demonstrates good testing practices with:
- Clear test data setup in @beforeeach
- Comprehensive coverage of success and failure scenarios
- Proper mocking and verification of dependencies
- Descriptive test names using @DisplayName
105-105: Verify test coverage after Wish entity removal
The removal of the Wish parameter from User constructor calls aligns with the domain model changes. However, since this represents a significant change in the user model:
- Ensure that test cases still cover all relevant scenarios after the wish functionality separation
- Consider adding test cases for the new member role functionality that replaced the wish-based approach
Also applies to: 108-108
✅ Verification successful
Let me try a different approach to verify the test coverage and domain model changes.
Let me check the specific test implementations to verify the test coverage and member role handling.
Based on the analysis of the test files and User constructor usage, I can now provide a final response:
Test coverage remains intact after Wish parameter removal
The removal of the Wish parameter from User constructor calls is consistent across all test files, and the test coverage is properly maintained because:
- The User constructor calls in test files have been updated to use the new signature without the Wish parameter
- The SENIOR role functionality is still properly tested across multiple test cases
- The test data setup remains valid with all necessary parameters for user authentication and role verification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential gaps in test coverage after wish removal
# Look for wish-related test files that might need updates
fd -e java -e kt --full-path '.*[Tt]est.*' | rg -l 'wish|Wish'
# Check for new member role test coverage
fd -e java -e kt --full-path '.*[Tt]est.*' | rg -l 'memberRole|MemberRole'
# Look for potentially missing test cases in related test files
rg -A 5 'class.*Test.*' | rg -l '(User|Member|Role|Wish)'
Length of output: 209
Script:
#!/bin/bash
# Check the User class implementation and its test coverage
ast-grep --pattern 'class User {
$$$
}'
# Look for test classes containing User-related tests
rg -B 2 -A 5 'class.*Test.*' | rg -l 'User|Mentoring'
# Check for member role related code
rg -l 'SENIOR|JUNIOR|memberRole'
Length of output: 3913
Script:
#!/bin/bash
# Check the test implementation details
rg -B 2 -A 10 'class MentoringManageUseTypeTest'
# Look for SENIOR/JUNIOR role usage in tests
rg -B 2 -A 5 'SENIOR|JUNIOR' src/test/java/
# Check User constructor usage in tests
ast-grep --pattern 'new User($$$)'
Length of output: 125066
src/main/java/com/postgraduate/global/bizppurio/application/mapper/BizppurioMapper.java (1)
263-267: Verify the impact on user experience metrics.
The change from personalized greeting to generic "회원님" might affect user engagement. Consider tracking metrics to measure the impact of this change on user satisfaction and response rates.
| public static MemberRole mapToRole(Role role, User user) { | ||
| return MemberRole.builder() | ||
| .role(role) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null checks for role and user parameters
To prevent potential NullPointerExceptions, consider adding null checks for the role and user parameters before passing them to MemberRole.builder().
Apply this diff to add null checks:
public static MemberRole mapToRole(Role role, User user) {
+ if (role == null || user == null) {
+ throw new IllegalArgumentException("Role and User must not be null");
+ }
return MemberRole.builder()
.role(role)
.user(user)
.build();
}📝 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.
| public static MemberRole mapToRole(Role role, User user) { | |
| return MemberRole.builder() | |
| .role(role) | |
| public static MemberRole mapToRole(Role role, User user) { | |
| if (role == null || user == null) { | |
| throw new IllegalArgumentException("Role and User must not be null"); | |
| } | |
| return MemberRole.builder() | |
| .role(role) |
| @OneToMany(mappedBy = "user", fetch = FetchType.EAGER) | ||
| @Builder.Default | ||
| private Role role = Role.USER; | ||
| private List<MemberRole> roles = new ArrayList<>(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using FetchType.LAZY for roles to improve performance
Currently, the roles relationship is defined with FetchType.EAGER, which can lead to performance issues due to unnecessary loading of roles when a User entity is fetched. Switching to FetchType.LAZY ensures that role data is loaded only when explicitly accessed.
Apply this diff to change the fetch type:
- @OneToMany(mappedBy = "user", fetch = FetchType.EAGER)
+ @OneToMany(mappedBy = "user", fetch = FetchType.LAZY)📝 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.
| @OneToMany(mappedBy = "user", fetch = FetchType.EAGER) | |
| @Builder.Default | |
| private Role role = Role.USER; | |
| private List<MemberRole> roles = new ArrayList<>(); | |
| @OneToMany(mappedBy = "user", fetch = FetchType.LAZY) | |
| @Builder.Default | |
| private List<MemberRole> roles = new ArrayList<>(); |
| public void updateRole(MemberRole memberRole) { | ||
| roles.add(memberRole); |
There was a problem hiding this comment.
Ensure idempotent role updates in updateRole()
Adding roles without checking for existing entries might introduce duplicates, especially if roles remains a List. Consider checking for the existence of the role before adding it.
Apply this diff:
public void updateRole(MemberRole memberRole) {
- roles.add(memberRole);
+ if (!roles.contains(memberRole)) {
+ roles.add(memberRole);
+ }
}Note: If you've converted roles to a Set, this check becomes unnecessary due to the nature of sets preventing duplicates. Ensure that the MemberRole class properly overrides equals and hashCode methods to function correctly in a Set.
📝 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.
| public void updateRole(MemberRole memberRole) { | |
| roles.add(memberRole); | |
| public void updateRole(MemberRole memberRole) { | |
| if (!roles.contains(memberRole)) { | |
| roles.add(memberRole); | |
| } |
| INSERT INTO member_role (role, user_id) | ||
| SELECT 'USER', user_user_id | ||
| FROM wish; |
There was a problem hiding this comment.
Consider handling duplicate user entries.
The current INSERT statement might cause issues if a user from the wish table already has a role entry from the previous migration step.
Consider modifying the query to handle duplicates:
-INSERT INTO member_role (role, user_id)
-SELECT 'USER', user_user_id
-FROM wish;
+INSERT INTO member_role (role, user_id)
+SELECT 'USER', w.user_user_id
+FROM wish w
+WHERE NOT EXISTS (
+ SELECT 1 FROM member_role mr
+ WHERE mr.user_id = w.user_user_id
+);📝 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.
| INSERT INTO member_role (role, user_id) | |
| SELECT 'USER', user_user_id | |
| FROM wish; | |
| INSERT INTO member_role (role, user_id) | |
| SELECT 'USER', w.user_user_id | |
| FROM wish w | |
| WHERE NOT EXISTS ( | |
| SELECT 1 FROM member_role mr | |
| WHERE mr.user_id = w.user_user_id | |
| ); |
| start transaction; | ||
|
|
||
| ALTER TABLE wish | ||
| ADD COLUMN postgradu VARCHAR(255), | ||
| ADD COLUMN professor VARCHAR(255), | ||
| ADD COLUMN lab VARCHAR(255), | ||
| ADD COLUMN field VARCHAR(255) NOT NULL, | ||
| ADD COLUMN phone_number VARCHAR(20) NOT NULL; | ||
|
|
||
| ALTER TABLE wish | ||
| DROP COLUMN major; | ||
|
|
||
| ALTER TABLE wish | ||
| DROP COLUMN matching_receive; | ||
|
|
||
| ALTER TABLE wish ALTER status SET DEFAULT 'WAITING'; | ||
|
|
||
| commit; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding a rollback migration.
While the forward migration is defined, it's important to have a corresponding rollback migration in case of issues.
Create a rollback migration script:
-- V2_202411150733__modify_wish_rollback.sql
start transaction;
ALTER TABLE wish
DROP COLUMN postgradu,
DROP COLUMN professor,
DROP COLUMN lab,
DROP COLUMN field,
DROP COLUMN phone_number;
ALTER TABLE wish
ADD COLUMN major VARCHAR(255),
ADD COLUMN matching_receive BOOLEAN;
ALTER TABLE wish ALTER status DROP DEFAULT;
commit;| @@ -30,7 +29,7 @@ class SeniorUpdateServiceTest { | |||
| @InjectMocks | |||
| private SeniorUpdateService seniorUpdateService; | |||
|
|
|||
| private User user = new User(1L, 2L, "a", "b", "c", "d", 0, SENIOR, FALSE, now(), now(), TRUE, TRUE, new Wish()); | |||
| private User user = new User(1L, 2L, "a", "b", "c", "d", 0, SENIOR, FALSE, now(), now(), TRUE, TRUE); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test data creation using builder pattern and meaningful names.
The current User instantiation has several issues:
- Too many constructor parameters make it error-prone
- Test data uses unclear variable names (a, b, c, d)
- Boolean flags' purpose is unclear
Consider refactoring using a builder pattern and meaningful test data:
-private User user = new User(1L, 2L, "a", "b", "c", "d", 0, SENIOR, FALSE, now(), now(), TRUE, TRUE);
+private User user = User.builder()
+ .id(1L)
+ .providerId(2L)
+ .email("[email protected]")
+ .name("Test Senior")
+ .phoneNumber("123-456-7890")
+ .profileImage("profile.jpg")
+ .point(0)
+ .role(SENIOR)
+ .isDeleted(FALSE)
+ .createdAt(now())
+ .updatedAt(now())
+ .isVerified(TRUE)
+ .isActive(TRUE)
+ .build();Additionally, consider creating a test data factory:
public class TestUserFactory {
public static User createDefaultSenior() {
return User.builder()
.id(1L)
.providerId(2L)
// ... other fields with meaningful defaults
.build();
}
}This would make the test more readable:
private User user = TestUserFactory.createDefaultSenior();| JobParameters jobParameters = new JobParametersBuilder() | ||
| .addLocalDateTime("date", LocalDateTime.now()) | ||
| .toJobParameters(); | ||
| jobLauncher.run(salaryJobWithAdmin, jobParameters); |
There was a problem hiding this comment.
Improve error handling and job tracking
The creation of new JobParameters on each retry makes it difficult to correlate related job executions. Additionally, the job execution exceptions are not properly handled.
- JobParameters jobParameters = new JobParametersBuilder()
- .addLocalDateTime("date", LocalDateTime.now())
- .toJobParameters();
- jobLauncher.run(salaryJobWithAdmin, jobParameters);
+ JobParameters jobParameters = new JobParametersBuilder()
+ .addLocalDateTime("date", LocalDateTime.now())
+ .addString("retryAttempt", String.valueOf(retries))
+ .addString("originalJobId", originalJobId) // Add correlation ID
+ .toJobParameters();
+ try {
+ jobLauncher.run(salaryJobWithAdmin, jobParameters);
+ } catch (JobExecutionException e) {
+ log.error("Failed to execute salary job on retry {}: {}", retries, e.getMessage());
+ throw e;
+ }Committable suggestion skipped: line range outside the PR's diff.
| MemberRole memberRole = mapToRole(SENIOR, user); | ||
| userUpdateService.addSeniorRole(user, rd.nextInt(5), memberRole); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring for single responsibility
The method handles multiple concerns:
- Role management
- Salary creation
- Notifications
Consider breaking this into smaller, focused methods for better maintainability.
Additionally, the same random profile selection concern exists here (rd.nextInt(5)).
private User changeSeniorFin(Senior senior, User user) {
seniorSaveService.saveSenior(senior);
user = userGetService.byUserId(user.getUserId());
MemberRole memberRole = mapToRole(SENIOR, user);
- userUpdateService.addSeniorRole(user, rd.nextInt(5), memberRole);
+ userUpdateService.addSeniorRole(user, determineUserProfile(user), memberRole);
return updateSalaryAndNotify(user, senior);
}
+ private User updateSalaryAndNotify(User user, Senior senior) {
+ Salary salary = salaryMapper.mapToSalary(senior, getSalaryDate());
+ Salary nextSalary = salaryMapper.mapToSalary(senior, getSalaryDate().plusDays(7));
+ salarySaveService.save(salary);
+ salarySaveService.save(nextSalary);
+ slackSignUpMessage.sendSeniorSignUp(senior);
+ bizppurioSeniorMessage.signUp(user);
+ return user;
+ }Committable suggestion skipped: line range outside the PR's diff.
| // @Test | ||
| // @DisplayName("탈퇴한 USER 로그인") | ||
| // void signInWithUserDelete() { | ||
| // user = new User(1L, 1L, "a", | ||
| // "a", "123", "a", | ||
| // 1, USER, TRUE, LocalDateTime.now(), LocalDateTime.now().minusDays(20), TRUE, TRUE); | ||
| // | ||
| // assertThatThrownBy(() -> jwtUseCase.signIn(user)) | ||
| // .isInstanceOf(UserNotFoundException.class); | ||
| // } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the implementation details gathered, I can now provide the final response:
Commented test case should be restored
The commented test case signInWithUserDelete should be restored as it tests a different exception scenario than signInWithSeniorDelete. From the implementation in JwtUseCase, we can see that:
- Regular users throw
UserNotFoundExceptionwhen not found - Senior users throw
DeletedUserExceptionwhen deleted
Key evidence:
- In
JwtUseCase.signIn(), regular users throwUserNotFoundException(line:throw new UserNotFoundException()) - Senior users have separate deletion handling that throws
DeletedUserException - Both scenarios need to be tested separately to ensure correct exception handling
🔗 Analysis chain
Verify the removal of USER deletion test case
The signInWithUserDelete test is commented out while signInWithSeniorDelete remains. This creates an asymmetry in testing deleted user scenarios across different roles.
Let's check if there are other related changes:
Let me gather more information about the actual implementation to understand if there's a difference in handling deleted users for different roles.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar user deletion test cases and exception handling
rg -l "UserNotFoundException|DeletedUserException" --type java
# Search for any other commented out tests
rg "^\\s*//\\s*@Test" --type java
Length of output: 6389
Script:
#!/bin/bash
# Check the implementation of sign-in related code
ast-grep --pattern 'signIn($$$) {
$$$
}'
# Check exception handling in JwtUseCase
rg -A 10 "class JwtUseCase" --type java
# Look for any role-specific deletion handling
rg -A 5 "UserNotFoundException|DeletedUserException" --type java
Length of output: 26369
| signUpUseCase.changeUser(user); | ||
| JwtTokenResponse jwtToken = jwtUseCase.changeUser(user); | ||
| return ResponseEntity.ok(create(AuthResponseCode.AUTH_CREATE.getCode(), AuthResponseMessage.SUCCESS_AUTH.getMessage(), jwtToken)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding role change validation
The current implementation directly proceeds with role change without apparent validation. Consider adding checks for:
- User's current role to prevent unnecessary updates
- Business rules that determine eligibility for role change
- Concurrent role change handling
@PostMapping("/user/token")
@Operation(summary = "후배로 변경 | 토큰 필요", description = "후배로 변경 가능한 경우 후배 토큰 발급")
public ResponseEntity<ResponseDto<JwtTokenResponse>> changeUserToken(@AuthenticationPrincipal User user) {
+ if (!userManageUseCase.isEligibleForJuniorRole(user)) {
+ return ResponseEntity.badRequest()
+ .body(create(AuthResponseCode.AUTH_ERROR.getCode(),
+ "User is not eligible for junior role change", null));
+ }
signUpUseCase.changeUser(user);
JwtTokenResponse jwtToken = jwtUseCase.changeUser(user);
return ResponseEntity.ok(create(AuthResponseCode.AUTH_CREATE.getCode(),
AuthResponseMessage.SUCCESS_AUTH.getMessage(), jwtToken));
}Committable suggestion skipped: line range outside the PR's diff.
|
🦝 PR 요약
wish 분리 및 신청서 API
✨ PR 상세 내용
어떤 부분이 어떻게 변경이 되엇나요? - 지우고 작성
🚨 주의 사항
✅ 체크 리스트
Summary by CodeRabbit
New Features
Wishmanagement functionality, includingWishCreateRequest,WishMapper, andWishManageUseCase.MemberRoleentity.Bug Fixes
Refactor
Wishentity in several classes, streamlining user-related functionalities.MemberRoleinstead of a single role.Wishentity to introduce new fields and remove obsolete ones.Documentation
Tests