RAC-466 fix : 신청일, 매칭일 컬럼 추가 및 신청시 교수님 추가 #328
Conversation
WalkthroughThe changes introduce enhancements to the Changes
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: 1
🧹 Outside diff range and nitpick comments (6)
src/main/resources/db/migration/V2_202411210500__addBaseTime.sql (1)
1-3: Consider adding indexes if these columns will be used in queries.If you plan to query records by creation date or last update (e.g., for filtering recent applications or finding stale matches), consider adding appropriate indexes.
Example index addition:
ALTER TABLE wish ADD COLUMN created_at datetime(6) not null DEFAULT CURRENT_TIMESTAMP(6), - ADD COLUMN updated_at datetime(6); + ADD COLUMN updated_at datetime(6), + ADD INDEX idx_wish_created_at (created_at);src/main/java/com/postgraduate/domain/wish/application/dto/request/WishCreateRequest.java (3)
5-5: Consider renaming 'postgradu' field for clarityThe field name 'postgradu' appears to be an abbreviation. Consider using a more descriptive name like 'postgraduateProgram' for better readability and maintainability.
- String postgradu, + String postgraduateProgram,
3-9: Add JavaDoc documentation to explain the record's purpose and fieldsConsider adding documentation to explain the purpose of this DTO and its fields, especially for the newly added professor field.
+/** + * Data Transfer Object for creating a new wish/application request. + * + * @param field The academic field or major + * @param postgradu The postgraduate program details + * @param professor The professor's name associated with the application + * @param lab The laboratory or research group + * @param phoneNumber Contact phone number of the applicant + */ public record WishCreateRequest(
8-8: Consider adding phone number format validationThe phoneNumber field might benefit from a pattern validation to ensure it follows the expected format.
+ @Pattern(regexp = "^\\d{2,3}-\\d{3,4}-\\d{4}$", message = "Invalid phone number format") String phoneNumbersrc/main/java/com/postgraduate/domain/wish/domain/entity/Wish.java (2)
44-50: Consider field naming and constraints alignmentA few suggestions to improve the implementation:
The field names (
createdAt/updatedAt) don't directly reflect the business terms from the PR (신청일/매칭일). Consider either:
- Renaming to
applicationDateandmatchingDate- Adding documentation explaining the mapping between technical and business terms
Consider adding
nullable = falsetoupdatedAtfor consistency, as it will always have a value (either matching the creation time initially or the last update time).@Column(nullable = false) @CreationTimestamp private LocalDateTime createdAt; - @UpdateTimestamp + @Column(nullable = false) + @UpdateTimestamp private LocalDateTime updatedAt;
51-53: Consider capturing matching timestamp explicitlyThe
updateDone()method changes the status toMATCHEDbut doesn't explicitly capture the matching timestamp. Consider adding a dedicatedmatchedAtfield that's set when the status changes toMATCHED.@UpdateTimestamp private LocalDateTime updatedAt; + private LocalDateTime matchedAt; + protected void updateDone() { this.status = Status.MATCHED; + this.matchedAt = LocalDateTime.now(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/main/java/com/postgraduate/domain/wish/application/dto/request/WishCreateRequest.java(1 hunks)src/main/java/com/postgraduate/domain/wish/domain/entity/Wish.java(2 hunks)src/main/resources/db/migration/V2_202411210500__addBaseTime.sql(1 hunks)
🔇 Additional comments (2)
src/main/resources/db/migration/V2_202411210500__addBaseTime.sql (1)
1-3: LGTM! The column additions look good.
The migration safely adds timestamp columns with appropriate nullability and defaults:
created_at: Not null with CURRENT_TIMESTAMP(6) defaultupdated_at: Nullable for @UpdateTimestamp handling
src/main/java/com/postgraduate/domain/wish/domain/entity/Wish.java (1)
9-12: LGTM! Appropriate imports added for timestamp functionality
The necessary imports for Hibernate timestamp annotations and LocalDateTime are correctly added.
| public record WishCreateRequest( | ||
| String field, | ||
| String postgradu, | ||
| String professor, | ||
| String lab, | ||
| String phoneNumber | ||
| ) {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation annotations to ensure data integrity
Consider adding validation annotations to ensure the required fields are properly validated before processing the request.
+import jakarta.validation.constraints.NotBlank;
+
public record WishCreateRequest(
+ @NotBlank(message = "Field is required")
String field,
+ @NotBlank(message = "Postgraduate program is required")
String postgradu,
+ @NotBlank(message = "Professor name is required")
String professor,
+ @NotBlank(message = "Lab is required")
String lab,
+ @NotBlank(message = "Phone number is required")
String phoneNumber
) {}📝 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 record WishCreateRequest( | |
| String field, | |
| String postgradu, | |
| String professor, | |
| String lab, | |
| String phoneNumber | |
| ) {} | |
| import jakarta.validation.constraints.NotBlank; | |
| public record WishCreateRequest( | |
| @NotBlank(message = "Field is required") | |
| String field, | |
| @NotBlank(message = "Postgraduate program is required") | |
| String postgradu, | |
| @NotBlank(message = "Professor name is required") | |
| String professor, | |
| @NotBlank(message = "Lab is required") | |
| String lab, | |
| @NotBlank(message = "Phone number is required") | |
| String phoneNumber | |
| ) {} |
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/test/java/com/postgraduate/support/Resource.java (1)
31-31: Consider using fixed timestamps for deterministic testing.While using
now()works, it may lead to flaky tests since the timestamp will be different each time tests run. Consider using fixed timestamps for more deterministic test behavior.- private Wish wish = new Wish(-1L, "major", "field", "professor", "lab", "123", com.postgraduate.domain.member.user.domain.entity.constant.Status.WAITING, now(), now()); + private Wish wish = new Wish(-1L, "major", "field", "professor", "lab", "123", com.postgraduate.domain.member.user.domain.entity.constant.Status.WAITING, + LocalDateTime.of(2024, 1, 1, 0, 0), // Fixed creation time + LocalDateTime.of(2024, 1, 1, 0, 0) // Fixed update time + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/test/java/com/postgraduate/support/Resource.java(1 hunks)
🔇 Additional comments (2)
src/test/java/com/postgraduate/support/Resource.java (2)
31-31: Verify alignment with requirements for application and matching dates.
The PR objectives mention adding 신청일 (application date) and 매칭일 (matching date), but the implementation shows createdAt and updatedAt. Please verify if these timestamps serve the same purpose or if additional fields are needed.
✅ Verification successful
The implementation correctly handles application and matching dates
The current implementation appropriately addresses the PR objectives:
createdAt(with@CreationTimestamp) serves as the application date (신청일), automatically recording when a wish is createdupdatedAt(with@UpdateTimestamp) serves as the matching date (매칭일), automatically updating when the status changes to MATCHED via theupdateDone()method- The migration file
V2_202411210500__addBaseTime.sqlproperly adds these columns to the wish table
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the correct date fields are implemented
# Look for application date and matching date fields in entity
echo "Checking Wish entity implementation:"
rg -A 10 "class.*Wish" "src/main/java"
# Check database migration scripts
echo "\nChecking database migrations:"
rg "ALTER.*wish" "src/main/resources/db/migration"
Length of output: 8186
Script:
#!/bin/bash
# Let's check the content of the migration files and entity class to understand the date fields
# Check the base time migration
echo "Checking base time migration:"
cat src/main/resources/db/migration/V2_202411210500__addBaseTime.sql
# Check the Wish entity class completely
echo -e "\nChecking complete Wish entity:"
rg -l "class.*Wish.*\{" --type java | xargs cat
# Check the WishCreateRequest to see what fields are accepted
echo -e "\nChecking WishCreateRequest:"
fd -e java WishCreateRequest | xargs cat
Length of output: 7280
31-31: Verify test coverage for the updated Wish entity.
Since this is a shared test resource, we should verify that all test cases using this Wish instance have been updated to account for the new timestamp fields.
✅ Verification successful
Let me gather more information about the actual usage of the Wish entity in the test files.
Based on the findings, I can now generate the final response:
The Wish entity changes appear to be properly handled in the test suite
The timestamp fields (now()) are already included in the Resource class's Wish instance, and the only other usage of the Wish constructor in SignUpUseTypeTest appears to be using a different constructor overload. The test files using this Resource class don't perform any timestamp-specific assertions, suggesting the changes are safe and won't impact existing test behavior.
Resource.javaalready includes the timestamp fields in its Wish instantiationAuthControllerTest.javaonly retrieves the Wish instance without any timestamp-specific assertionsSignUpUseTypeTest.javauses a different constructor overload, unaffected by these changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all test files using the Resource class and verify test coverage
# Find test files that use the Resource class
echo "Test files using Resource class:"
rg -l "Resource" "src/test"
# Check test coverage for Wish entity tests
echo "\nWish entity test coverage:"
rg -A 5 "class.*WishTest" "src/test"
# Look for potential test cases that might need updates
echo "\nPotential affected test cases:"
rg "getWish\(\)" "src/test"
Length of output: 960
Script:
#!/bin/bash
# Check how the Wish entity is used in test files
echo "Usage of Wish in AuthControllerTest:"
rg -B 2 -A 2 "getWish\(\)" "src/test/java/com/postgraduate/Integration/AuthControllerTest.java"
echo "\nChecking Wish constructor usage in test files:"
rg -B 2 -A 2 "new Wish\(" "src/test"
echo "\nChecking timestamp-related assertions in tests:"
rg "assert.*[Tt]ime" "src/test/java/com/postgraduate/Integration/AuthControllerTest.java"
Length of output: 2360
🦝 PR 요약
신청일, 매칭일 컬럼 추가 및 신청시 교수님 추가
✨ PR 상세 내용
신청일, 매칭일 DB 컬럼 추가 및 SQL작성
dto에 교수님 추가
🚨 주의 사항
주의할 부분이 무엇인가요? - 지우고 작성
✅ 체크 리스트
Summary by CodeRabbit
New Features
Database Changes