[BE-Refactor] 개인 일정 생성, 업데이트, 수정 구조 변경 #212
Conversation
WalkthroughThe changes refactor the PersonalEvent module by renaming key service methods for event creation and updating, and updating corresponding controller and test calls. The repository interface now removes a filtering method based on Changes
Sequence Diagram(s)sequenceDiagram
participant C as PersonalEventController
participant S as PersonalEventService
participant R as PersonalEventRepository
participant D as DiscussionProcessor
C->>S: createWithRequest(PersonalEventRequest)
S->>R: Save new PersonalEvent
S->>D: Process discussions for created event
D-->>S: Acknowledge discussion updates
S-->>C: Return PersonalEventResponse
sequenceDiagram
participant C as PersonalEventController
participant S as PersonalEventService
participant R as PersonalEventRepository
participant D as DiscussionProcessor
C->>S: updateWithRequest(PersonalEventRequest, eventId)
S->>R: Update existing PersonalEvent
S->>D: Process discussions for updated event
D-->>S: Acknowledge discussion changes
S-->>C: Return PersonalEventResponse
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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: 0
🧹 Nitpick comments (7)
backend/src/test/java/endolphin/backend/domain/personal_event/PersonalEventControllerTest.java (1)
39-41: Consider improving test method name and coverage.The current test method name
createPersonalEventand display namebuildResourceURI 생성 테스트don't clearly reflect that this test is specifically validating the Location header in the response. Additionally, the test could be more comprehensive.Consider these improvements:
- Rename the test method to better reflect its purpose:
-@DisplayName("buildResourceURI 생성 테스트") -void createPersonalEvent() throws Exception { +@DisplayName("Personal event creation should return correct location URI") +void shouldReturnLocationHeaderOnPersonalEventCreation() throws Exception {
- Add additional assertions to validate:
- Response status code
- Response body (if any)
- Format of the returned URI
backend/src/test/java/endolphin/backend/domain/personal_event/PersonalEventServiceTest.java (1)
256-263: Consider consolidating helper methods.There are two helper methods for creating personal events:
createWithRequest(String title, int minusHour, User user)createWithRequest(String title)The second method could be enhanced to match the functionality of the first one for consistency.
Consider updating the mock-based helper method to match the builder-based one:
-PersonalEvent createWithRequest(String title) { - PersonalEvent personalEvent = Mockito.mock(PersonalEvent.class); - return personalEvent; -} +PersonalEvent createWithRequest(String title) { + return createWithRequest(title, 0, testUser); +}Also applies to: 278-281
backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventRepository.java (1)
17-26: Optimize date range query performance.Based on previous learnings, using CAST operations in JPQL can impact performance and index utilization. Consider using direct datetime comparisons instead.
@Query("SELECT p FROM PersonalEvent p " + "WHERE p.user = :user " + "AND (" + - " CAST(p.startTime as localdate) <= :endDate " + - " AND CAST(p.endTime as localdate) >= :startDate" + + " p.startTime <= CONCAT(:endDate, ' 23:59:59.999999') " + + " AND p.endTime >= CONCAT(:startDate, ' 00:00:00.000000')" + ")")backend/src/test/java/endolphin/backend/domain/personal_event/PersonalEventRepositoryTest.java (1)
60-75: Enhance test documentation for clarity.The test method name and DisplayName could be more descriptive to reflect that it's testing date-based filtering rather than time-based filtering.
@Test - @DisplayName("개인 일정 검색 테스트") + @DisplayName("개인 일정을 날짜 기준으로 필터링하여 검색") void searchTest() {Also, consider adding comments to clarify that the test data is set up with specific times but filtered by date only.
backend/src/main/java/endolphin/backend/domain/personal_event/dto/PersonalEventRequest.java (2)
13-14: Consider adding @NotNull to Boolean fields.The
isAdjustableandsyncWithGoogleCalendarfields are nullable Booleans. This could lead to NPEs in business logic. Consider adding@NotNullconstraints or using primitivebooleaninstead.- Boolean isAdjustable, - Boolean syncWithGoogleCalendar + @NotNull Boolean isAdjustable, + @NotNull Boolean syncWithGoogleCalendar
17-25: Verify default values and add null check.Two suggestions for the factory method:
- Verify that
falseis the correct default for bothisAdjustableandsyncWithGoogleCalendarwhen converting from Google events.- Add null check for the input parameter.
public static PersonalEventRequest of(GoogleEvent googleEvent) { + Objects.requireNonNull(googleEvent, "GoogleEvent must not be null"); return new PersonalEventRequest( googleEvent.summary(), googleEvent.startDateTime(), googleEvent.endDateTime(), false, false ); }backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventService.java (1)
76-76: Track Google Calendar sync implementationConsider creating a separate issue to track the Google Calendar sync implementation.
Would you like me to help create an issue for tracking the Google Calendar sync implementation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 211e47f and 13e3a89c37285ea0525d25739b3eca4deaaaf0c1.
📒 Files selected for processing (7)
backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventController.java(2 hunks)backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventRepository.java(1 hunks)backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventService.java(8 hunks)backend/src/main/java/endolphin/backend/domain/personal_event/dto/PersonalEventRequest.java(1 hunks)backend/src/test/java/endolphin/backend/domain/personal_event/PersonalEventControllerTest.java(1 hunks)backend/src/test/java/endolphin/backend/domain/personal_event/PersonalEventRepositoryTest.java(1 hunks)backend/src/test/java/endolphin/backend/domain/personal_event/PersonalEventServiceTest.java(11 hunks)
🧰 Additional context used
🧠 Learnings (3)
backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventRepository.java (2)
Learnt from: efdao
PR: softeer5th/Team4-enDolphin#157
File: backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventRepository.java:29-37
Timestamp: 2025-02-13T04:53:38.618Z
Learning: When querying overlapping events with datetime ranges, use direct datetime comparisons (startTime <= endDateTime AND endTime >= startDateTime) instead of BETWEEN operator to correctly catch all overlapping events, including those that span across the entire range.
Learnt from: efdao
PR: softeer5th/Team4-enDolphin#142
File: backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventRepository.java:19-24
Timestamp: 2025-02-11T03:23:12.271Z
Learning: When querying date range overlaps in JPQL, prefer using direct datetime comparisons (e.g., `startTime <= :endDate AND endTime >= :startDate`) over CAST operations for better performance and index utilization.
backend/src/test/java/endolphin/backend/domain/personal_event/PersonalEventRepositoryTest.java (2)
Learnt from: efdao
PR: softeer5th/Team4-enDolphin#157
File: backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventRepository.java:29-37
Timestamp: 2025-02-13T04:53:38.618Z
Learning: When querying overlapping events with datetime ranges, use direct datetime comparisons (startTime <= endDateTime AND endTime >= startDateTime) instead of BETWEEN operator to correctly catch all overlapping events, including those that span across the entire range.
Learnt from: efdao
PR: softeer5th/Team4-enDolphin#142
File: backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventRepository.java:19-24
Timestamp: 2025-02-11T03:23:12.271Z
Learning: When querying date range overlaps in JPQL, prefer using direct datetime comparisons (e.g., `startTime <= :endDate AND endTime >= :startDate`) over CAST operations for better performance and index utilization.
backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventController.java (1)
Learnt from: kwon204
PR: softeer5th/Team4-enDolphin#45
File: backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventController.java:28-33
Timestamp: 2025-02-04T01:27:54.848Z
Learning: For search/filter operations in REST APIs, prefer using GET with query parameters over POST with request body, as it follows REST principles and enables caching, bookmarking, and better API discoverability.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Send PR Notification to Reviewers
🔇 Additional comments (14)
backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventController.java (2)
73-78: LGTM! Method call renamed for clarity.The renaming of the service method to
createWithRequestaligns with the PR's refactoring objectives while maintaining correct REST principles and response handling.
94-100: LGTM! Method call renamed for clarity.The renaming of the service method to
updateWithRequestaligns with the PR's refactoring objectives while maintaining correct REST principles and response handling.backend/src/test/java/endolphin/backend/domain/personal_event/PersonalEventControllerTest.java (1)
54-54: LGTM! Method rename aligns with service layer changes.The update to use
createWithRequestinstead ofcreatePersonalEventcorrectly reflects the service layer refactoring.backend/src/test/java/endolphin/backend/domain/personal_event/PersonalEventServiceTest.java (4)
75-77: LGTM! Method renaming is consistent.The renaming from
createPersonalEventtocreateWithRequestis consistently applied across test methods, aligning with the service layer changes.Also applies to: 98-99
82-82: LGTM! Repository method update is consistent.The change from
findByUserAndStartTimeBetweentofindFilteredPersonalEventsaligns with the repository interface changes, now usingLocalDateinstead ofLocalDateTimefor filtering.
207-254: Verify Google sync test coverage.The Google sync test verifies preprocessing for both updated and deleted events, but there are a few scenarios that could be added:
- New Google events (not previously existing in the system)
- Edge cases like null dates or invalid status
Consider adding these test cases to ensure comprehensive coverage of the sync functionality.
289-450: LGTM! Comprehensive user info tests.The tests for
findUserInfoWithPersonalEventsByUsersare well-structured and cover important scenarios:
- Users with events requiring adjustment
- Users with events not requiring adjustment
- Users without any events
- Selected vs non-selected users
backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventRepository.java (1)
28-29: LGTM!The formatting change improves readability while maintaining the same functionality.
backend/src/main/java/endolphin/backend/domain/personal_event/dto/PersonalEventRequest.java (1)
3-3: LGTM!The import for GoogleEvent is appropriately added to support the new factory method.
backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventService.java (5)
48-49: LGTM! Improved date filtering approachThe change to use
findFilteredPersonalEventswithLocalDateparameters is more appropriate for date-based filtering.
68-74: LGTM! Added discussion preprocessingThe addition of discussion preprocessing after event creation aligns with the PR objectives and ensures discussions are properly updated.
99-116: LGTM! Improved update method structureThe refactoring to separate the update logic into a private method improves code organization and readability.
118-136: LGTM! Well-structured event update preprocessingThe implementation correctly:
- Preserves the old event state
- Updates the event
- Triggers preprocessing only when necessary
142-147: LGTM! Added pre-deletion discussion preprocessingThe addition of discussion preprocessing before event deletion ensures that discussions are properly updated.
efdao
force-pushed
the
refactor/be/preprocess-event
branch
from
13e3a89 to
86b1239
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventService.java (2)
76-77: Consider creating a tracking issue for Google Calendar sync.The TODO comment indicates a pending feature for Google Calendar synchronization.
Would you like me to help create a tracking issue for implementing the Google Calendar sync feature?
218-221: Consider including title and isAdjustable in change detection.The current implementation only checks for time changes. Consider including other relevant fields like title and isAdjustable in the change detection.
private boolean isChanged(PersonalEvent personalEvent, PersonalEventRequest newEvent) { return !personalEvent.getStartTime().equals(newEvent.startDateTime()) - || !personalEvent.getEndTime().equals(newEvent.endDateTime()); + || !personalEvent.getEndTime().equals(newEvent.endDateTime()) + || !personalEvent.getTitle().equals(newEvent.title()) + || personalEvent.isAdjustable() != newEvent.isAdjustable(); }backend/src/test/java/endolphin/backend/domain/personal_event/PersonalEventServiceTest.java (1)
256-263: Consider consolidating duplicate helper methods.There are two helper methods for creating personal events:
createWithRequest(String title, int minusHour, User user)createWithRequest(String title)The second method could be enhanced to leverage the first method's functionality for better code reuse.
- PersonalEvent createWithRequest(String title) { - PersonalEvent personalEvent = Mockito.mock(PersonalEvent.class); - return personalEvent; - } + PersonalEvent createWithRequest(String title) { + return createWithRequest(title, 0, Mockito.mock(User.class)); + }Also applies to: 278-281
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 13e3a89c37285ea0525d25739b3eca4deaaaf0c1 and 86b1239.
📒 Files selected for processing (8)
backend/.gitignore(1 hunks)backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventController.java(2 hunks)backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventRepository.java(1 hunks)backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventService.java(8 hunks)backend/src/main/java/endolphin/backend/domain/personal_event/dto/PersonalEventRequest.java(2 hunks)backend/src/test/java/endolphin/backend/domain/personal_event/PersonalEventControllerTest.java(1 hunks)backend/src/test/java/endolphin/backend/domain/personal_event/PersonalEventRepositoryTest.java(1 hunks)backend/src/test/java/endolphin/backend/domain/personal_event/PersonalEventServiceTest.java(11 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/.gitignore
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/src/main/java/endolphin/backend/domain/personal_event/dto/PersonalEventRequest.java
- backend/src/test/java/endolphin/backend/domain/personal_event/PersonalEventControllerTest.java
- backend/src/test/java/endolphin/backend/domain/personal_event/PersonalEventRepositoryTest.java
- backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventRepository.java
- backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventController.java
🔇 Additional comments (10)
backend/src/main/java/endolphin/backend/domain/personal_event/PersonalEventService.java (7)
48-49: LGTM! Method name improvement.The change to
findFilteredPersonalEventsbetter describes the method's filtering functionality.
54-54: LGTM! Improved event creation with discussion preprocessing.The addition of discussion preprocessing ensures that related discussions are properly updated when a new personal event is created.
Also applies to: 67-75
99-116: LGTM! Improved update logic with discussion preprocessing.The method has been well-structured with clear separation of concerns and proper discussion preprocessing.
142-149: LGTM! Added discussion preprocessing before deletion.The addition of discussion preprocessing ensures that related discussions are properly updated before the event is deleted.
252-253: LGTM! Improved code formatting.The formatting changes improve code readability while maintaining the same functionality.
190-191: LGTM! Consistent use of request factory method.The change maintains consistency by using the PersonalEventRequest factory method for Google event updates.
118-136:Details
❓ Verification inconclusive
Consider adding validation for the copied event.
While the event copying logic is good for handling preprocessing, consider adding validation to ensure the copy operation preserves all necessary data.
🏁 Script executed:
#!/bin/bash # Search for the copy method implementation to verify its completeness ast-grep --pattern 'class PersonalEvent { $$$ copy() { $$$ } $$$ }'Length of output: 81
Action: Verify the integrity of the copied PersonalEvent data
Based on our automated check (which returned no explicit output), we couldn't conclusively determine if the current implementation of the
copy()method inPersonalEventadequately replicates all required fields. Please manually verify that:
- The
copy()method inPersonalEvent(likely inbackend/src/main/java/endolphin/backend/domain/personal_event/PersonalEvent.java) correctly preserves all necessary data.- The copied event is valid for the subsequent preprocessing steps.
If any discrepancies are found, consider adding explicit validation logic after the copy operation in
PersonalEventService#updatePersonalEvent.backend/src/test/java/endolphin/backend/domain/personal_event/PersonalEventServiceTest.java (3)
94-94: Method renaming follows a consistent pattern.The test method names have been updated to align with the service method changes, maintaining a clear and consistent naming convention that reflects the underlying service operations.
Also applies to: 116-116, 140-140, 154-154, 209-209
289-450: Comprehensive test coverage for edge cases.The test class includes thorough coverage for:
- Users with no events
- Time range filtering
- Requirement adjustment sorting
- User selection states
75-82:Details
❓ Verification inconclusive
Verify repository method change impact.
The repository method has changed from
findByUserAndStartTimeBetweentofindFilteredPersonalEvents, which now usesLocalDateinstead ofLocalDateTime. Ensure this change doesn't affect the test's time precision requirements.
🏁 Script executed:
#!/bin/bash # Search for the repository interface definition to verify the method signature ast-grep --pattern 'interface PersonalEventRepository { $$$ findFilteredPersonalEvents($$$) { $$$ } $$$ }'Length of output: 120
Action: Verify Repository Method Signature Usage and Its Impact
The repository method change (from using
LocalDateTimeinfindByUserAndStartTimeBetweento usingLocalDateinfindFilteredPersonalEvents) remains unverified because our initial script did not yield any results. This raises concerns about whether the method’s signature now usesLocalDateas intended and whether the test’s time precision requirements are still being met.To proceed, please run the following shell script to locate the method signature in the repository and confirm that the change in parameter types has been correctly implemented:
Once you verify that the method indeed uses
LocalDateinstead ofLocalDateTime, please ensure that the tests inPersonalEventServiceTest.java(lines 75-82) adjust for any changes in time precision accordingly. If any discrepancies are found, update the test data or assertions to accommodate the new date type precision.
🏁 Script executed:
#!/bin/bash # Search for occurrences of findFilteredPersonalEvents and its method signature across Java files rg "public .*findFilteredPersonalEvents" -t javaLength of output: 50
Action Required: Manual Verification of Repository Method Signature
The automated scripts did not yield any output regarding the declaration of
findFilteredPersonalEvents. This lack of evidence prevents us from confirming that the method’s parameters have indeed been changed fromLocalDateTimetoLocalDate.Please manually verify that:
- The method
findFilteredPersonalEventsis declared inPersonalEventRepository(or its super-interface) with the expectedLocalDateparameters.- The change does not introduce issues with the test's time precision, especially within the code in
PersonalEventServiceTest.java(lines 75-82).Once confirmed, you can ensure that test data and assertions are correctly adjusted for the new date precision.
#️⃣ 연관된 이슈>
📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)
🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요
Summary by CodeRabbit
New Features
Refactor
Tests