Skip to content

Conversation

@efdao
Copy link
Collaborator

@efdao efdao commented Mar 26, 2025

#️⃣ 연관된 이슈>

📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)

  • 논의 기본 정보 수정하는 updateDiscussion() 추가했습니다.
    • 논의 정보를 입력받아 update하고 논의 범위가 변경되었다면 기존 전처리 데이터를 삭제합니다.
    • 삭제 후 변경된 범위에 맞게 전처리를 수행합니다.
  • bitmap 데이터 삭제를 동기로 처리하는 메서드를 추가했습니다.

🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요

Summary by CodeRabbit

  • New Features

    • Introduced a discussion update capability that lets users modify key details such as title, schedule, location, and deadlines through a dedicated update flow.
  • Enhancements

    • Improved the update process with robust validations and seamless handling of time changes, ensuring that scheduling adjustments and related notifications work more reliably.

@efdao efdao added the 🛠️ BE Backend label Mar 26, 2025
@efdao efdao added this to the 💪8차 스프린트 milestone Mar 26, 2025
@efdao efdao self-assigned this Mar 26, 2025
@efdao efdao requested a review from kwon204 as a code owner March 26, 2025 14:30
@coderabbitai
Copy link

coderabbitai bot commented Mar 26, 2025

Walkthrough

This pull request introduces an update discussion endpoint in the discussion domain. A new DTO (UpdateDiscussionRequest) and an update method in the Discussion entity enable handling of discussion updates. The service layer now validates the host and checks for time changes, triggering asynchronous deletion of discussion bitmaps via a new method in DiscussionBitmapService with a retry mechanism. Corresponding test cases have been added and updated to reflect these changes.

Changes

Files Change Summary
backend/src/.../discussion/DiscussionController.java, backend/src/.../discussion/DiscussionService.java, backend/src/.../discussion/dto/UpdateDiscussionRequest.java, backend/src/.../discussion/entity/Discussion.java Added update discussion endpoint and supporting domain logic, including request DTO, entity update method, host validation, and time-change checks.
backend/src/.../redis/DiscussionBitmapService.java, backend/src/.../scheduler/DiscussionStatusScheduler.java Replaced synchronous bitmap deletion with an asynchronous approach using a new retry-enabled method and helper for deletion logic.
backend/src/test/.../discussion/DiscussionServiceTest.java, backend/src/test/.../redis/DiscussionBitmapServiceTest.java, backend/src/test/.../scheduler/DiscussionStatusSchedulerTest.java Added tests for update discussion scenarios and updated existing tests to use the new asynchronous bitmap deletion method.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Controller
    participant Service
    participant Entity
    participant BitmapSvc

    Client->>Controller: PUT /{discussionId} with UpdateDiscussionRequest
    Controller->>Service: updateDiscussion(discussionId, request)
    Service->>Entity: discussion.update(request)
    alt Time fields changed
        Service->>BitmapSvc: deleteDiscussionBitmapsAsync(discussionId)
        BitmapSvc-->>Service: Async deletion result
    end
    Service->>Controller: Return updated DiscussionResponse
    Controller->>Client: 200 OK, DiscussionResponse
Loading

Possibly related PRs

Suggested reviewers

  • kwon204

Poem

I hopped through code with gentle delight,
Updating discussions from morning till night.
Bitmaps dance away in asynchronous streams,
While tests cheer on in our dev dreams.
With each new line, my heart does sing,
A rabbit’s joy in every bug it brings!
🐇✨

✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@efdao efdao linked an issue Mar 26, 2025 that may be closed by this pull request
3 tasks
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (6)
backend/src/main/java/endolphin/backend/domain/discussion/entity/Discussion.java (1)

97-107: Update method properly implements all fields from request.

The update method provides a clean way to modify all necessary discussion fields from the request object. This follows good design practices by centralizing the update logic in the entity.

Consider adding validation to ensure dateRangeStart is before or equal to dateRangeEnd, and timeRangeStart is before timeRangeEnd within this method for additional data integrity.

public void update(UpdateDiscussionRequest request) {
    this.title = request.title();
    this.dateRangeStart = request.dateRangeStart();
    this.dateRangeEnd = request.dateRangeEnd();
    this.timeRangeStart = request.timeRangeStart();
    this.timeRangeEnd = request.timeRangeEnd();
    this.duration = request.duration();
    this.meetingMethod = request.meetingMethod();
    this.location = request.location();
    this.deadline = request.deadline();
+   
+   // Optional: Add additional validation
+   if (this.dateRangeStart.isAfter(this.dateRangeEnd)) {
+       throw new IllegalArgumentException("Start date cannot be after end date");
+   }
+   if (this.timeRangeStart.isAfter(this.timeRangeEnd)) {
+       throw new IllegalArgumentException("Start time cannot be after end time");
+   }
}
backend/src/main/java/endolphin/backend/domain/discussion/dto/UpdateDiscussionRequest.java (1)

1-26: Well-structured DTO with appropriate validations.

The record class is well designed with comprehensive validation annotations for most fields. This ensures data integrity when updating discussion information.

There are a few validations that might enhance the robustness of this DTO:

  1. No cross-field validations to ensure dateRangeStart is before or equal to dateRangeEnd
  2. No validation to ensure timeRangeStart is before timeRangeEnd
  3. No validation to ensure the duration makes sense with the time range

Consider adding custom validation annotations or implementing a validator class to handle these cross-field validations.

You could add a validator class like this:

@Component
public class UpdateDiscussionRequestValidator implements Validator {
    
    @Override
    public boolean supports(Class<?> clazz) {
        return UpdateDiscussionRequest.class.isAssignableFrom(clazz);
    }
    
    @Override
    public void validate(Object target, Errors errors) {
        UpdateDiscussionRequest request = (UpdateDiscussionRequest) target;
        
        // Validate date range
        if (request.dateRangeStart() != null && request.dateRangeEnd() != null 
                && request.dateRangeStart().isAfter(request.dateRangeEnd())) {
            errors.rejectValue("dateRangeStart", "date.invalid", 
                    "Start date cannot be after end date");
        }
        
        // Validate time range
        if (request.timeRangeStart() != null && request.timeRangeEnd() != null 
                && request.timeRangeStart().isAfter(request.timeRangeEnd())) {
            errors.rejectValue("timeRangeStart", "time.invalid", 
                    "Start time cannot be after end time");
        }
        
        // Validate duration with time range
        if (request.timeRangeStart() != null && request.timeRangeEnd() != null && request.duration() != null) {
            int minutesBetween = (int) ChronoUnit.MINUTES.between(
                    request.timeRangeStart(), request.timeRangeEnd());
            if (request.duration() > minutesBetween) {
                errors.rejectValue("duration", "duration.invalid", 
                        "Duration cannot exceed the time range");
            }
        }
    }
}
backend/src/main/java/endolphin/backend/global/redis/DiscussionBitmapService.java (1)

125-140: Consider making deleteDiscussionBitmaps private if not intended for direct use.

The extracted deleteDiscussionBitmaps method contains the core logic for bitmap deletion, but it's currently public. If it's not intended to be called directly from outside this class, consider making it private to encapsulate the implementation details.

-public void deleteDiscussionBitmaps(Long discussionId) {
+private void deleteDiscussionBitmaps(Long discussionId) {
backend/src/test/java/endolphin/backend/domain/discussion/DiscussionServiceTest.java (3)

824-868: Good negative test case with proper verification.

This test properly verifies that bitmap deletion and personal event restoration are not called when time-related fields don't change.

For consistency in naming between test cases:

// then
-verify(discussionBitmapService, never()).deleteDiscussionBitmaps(anyLong());
+verify(discussionBitmapService, never()).deleteDiscussionBitmapsAsync(anyLong());
verify(personalEventService, never()).restorePersonalEvents(any(Discussion.class));

870-904: Well-structured authorization test.

This test properly verifies that non-host users cannot update discussions. The exception validation and verification that no methods are called are done correctly.

For consistency with other tests:

verify(discussionRepository, never()).save(any(Discussion.class));
-verify(discussionBitmapService, never()).deleteDiscussionBitmaps(anyLong());
+verify(discussionBitmapService, never()).deleteDiscussionBitmapsAsync(anyLong());
verify(personalEventService, never()).restorePersonalEvents(any());

777-904: Consider reducing test duplication.

The three new test methods contain similar setup code for creating discussions and update requests.

Consider extracting common setup code to helper methods or using JUnit's @BeforeEach to reduce duplication. For example:

private Discussion createTestDiscussion(Long id) {
    Discussion discussion = Discussion.builder()
        .title("Test Discussion")
        .dateStart(LocalDate.of(2025, 3, 1))
        .dateEnd(LocalDate.of(2025, 3, 1))
        .timeStart(LocalTime.of(10, 0))
        .timeEnd(LocalTime.of(12, 0))
        .duration(120)
        .deadline(LocalDate.of(2025, 3, 15))
        .meetingMethod(MeetingMethod.ONLINE)
        .location("Test Location")
        .build();
    discussion.setDiscussionStatus(DiscussionStatus.ONGOING);
    ReflectionTestUtils.setField(discussion, "id", id);
    return discussion;
}

private UpdateDiscussionRequest createUpdateRequest(LocalDate endDate) {
    return new UpdateDiscussionRequest(
        "팀 회의",
        LocalDate.of(2025, 3, 1),
        endDate,
        LocalTime.of(10, 0),
        LocalTime.of(12, 0),
        120,
        MeetingMethod.ONLINE,
        "회의실 1",
        LocalDate.of(2025, 3, 15)
    );
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1a67c33 and b959609.

📒 Files selected for processing (9)
  • backend/src/main/java/endolphin/backend/domain/discussion/DiscussionController.java (2 hunks)
  • backend/src/main/java/endolphin/backend/domain/discussion/DiscussionService.java (3 hunks)
  • backend/src/main/java/endolphin/backend/domain/discussion/dto/UpdateDiscussionRequest.java (1 hunks)
  • backend/src/main/java/endolphin/backend/domain/discussion/entity/Discussion.java (2 hunks)
  • backend/src/main/java/endolphin/backend/global/redis/DiscussionBitmapService.java (2 hunks)
  • backend/src/main/java/endolphin/backend/global/scheduler/DiscussionStatusScheduler.java (1 hunks)
  • backend/src/test/java/endolphin/backend/domain/discussion/DiscussionServiceTest.java (6 hunks)
  • backend/src/test/java/endolphin/backend/global/redis/DiscussionBitmapServiceTest.java (2 hunks)
  • backend/src/test/java/endolphin/backend/global/scheduler/DiscussionStatusSchedulerTest.java (2 hunks)
🔇 Additional comments (15)
backend/src/main/java/endolphin/backend/domain/discussion/entity/Discussion.java (1)

3-3: Appropriate import addition.

The import for UpdateDiscussionRequest is correctly added to support the new update functionality.

backend/src/test/java/endolphin/backend/global/scheduler/DiscussionStatusSchedulerTest.java (2)

62-63: Method call correctly updated to use async version.

The test has been properly updated to use the new asynchronous method deleteDiscussionBitmapsAsync instead of the previous synchronous method. This ensures the test aligns with the implementation changes.


154-155: Method call correctly updated to use async version.

The test has been properly updated to use the new asynchronous method deleteDiscussionBitmapsAsync instead of the previous synchronous method. This ensures the test aligns with the implementation changes.

backend/src/main/java/endolphin/backend/global/scheduler/DiscussionStatusScheduler.java (1)

62-70:

Details

❓ Verification inconclusive

Successfully migrated to asynchronous bitmap deletion.

The implementation has been correctly updated to use the asynchronous method deleteDiscussionBitmapsAsync instead of the synchronous version. The callbacks for success and error handling are well-implemented.

However, since this is happening within a transactional method, be aware that the asynchronous operation will execute outside the transaction boundary. Ensure that failure in the async operation won't leave the system in an inconsistent state.

Consider verifying the transaction behavior with the following test:


🏁 Script executed:

#!/bin/bash
# Check if there's any transaction management in the DiscussionBitmapService implementation

rg -A 5 -B 5 "deleteDiscussionBitmapsAsync" backend/src/main/java/endolphin/backend/global/redis/DiscussionBitmapService.java
rg "@Transactional" backend/src/main/java/endolphin/backend/global/redis/DiscussionBitmapService.java

Length of output: 533


Attention: Verify Asynchronous Transaction Context

The migration to asynchronous bitmap deletion is correctly implemented using deleteDiscussionBitmapsAsync(). The asynchronous callbacks for logging success and handling errors are well set up.

  • It appears that the deleteDiscussionBitmapsAsync() method is annotated with @Async but does not have any transaction management (no @Transactional found). This means that the deletion runs outside the transaction boundary.
  • Please verify that executing this asynchronous call outside an active transaction is intentional and that any failure in this operation won’t compromise system consistency.
backend/src/main/java/endolphin/backend/domain/discussion/DiscussionController.java (2)

17-17: Good addition of the necessary import.

The import for UpdateDiscussionRequest is properly added to support the new update discussion functionality.


290-311: Well-implemented discussion update endpoint.

The new endpoint follows RESTful practices with appropriate:

  • HTTP method (PUT) for updates
  • Path variable validation
  • Request body validation
  • Comprehensive API documentation
  • Clear error response definitions

The implementation correctly delegates to the service layer while maintaining the controller's responsibility of handling HTTP concerns.

backend/src/test/java/endolphin/backend/global/redis/DiscussionBitmapServiceTest.java (3)

53-53: Method name updated to match implementation.

Test method name has been appropriately updated to reflect the renamed service method.


66-67: Service method call updated to match implementation.

The test now correctly calls the renamed async method.


73-74: Assertion message properly updated.

The assertion message has been updated to reflect the new method name, maintaining test clarity.

backend/src/main/java/endolphin/backend/global/redis/DiscussionBitmapService.java (1)

99-123: Well-implemented asynchronous deletion with retry mechanism.

The new deleteDiscussionBitmapsAsync method:

  • Maintains asynchronous behavior with CompletableFuture
  • Implements a robust retry mechanism (3 attempts)
  • Properly handles interruptions and failures
  • Returns appropriate responses for success and failure cases

This is a good improvement that enhances reliability for bitmap deletion operations.

backend/src/main/java/endolphin/backend/domain/discussion/DiscussionService.java (2)

118-124: Asynchronous bitmap deletion with proper error handling.

The updated method call to deleteDiscussionBitmapsAsync maintains the asynchronous operation pattern and includes proper logging for both success and failure cases.


398-403: Good helper method for time change detection.

The isTimeChanged method clearly expresses the logic for determining if any time-related fields have changed. This improves readability and encapsulates the condition in a well-named method.

backend/src/test/java/endolphin/backend/domain/discussion/DiscussionServiceTest.java (3)

8-8: Imports added correctly for the new functionality.

The imports for anyLong and UpdateDiscussionRequest are appropriately added to support the new updateDiscussion functionality and its test cases.

Also applies to: 26-26


239-240: Good use of CompletableFuture for failure testing.

This test correctly mocks a failed future for testing async failure handling.


258-258: Method name update to match implementation.

The verification correctly uses deleteDiscussionBitmapsAsync here.

Comment on lines +371 to +396
public DiscussionResponse updateDiscussion(Long discussionId, UpdateDiscussionRequest request) {
Discussion discussion = getDiscussionById(discussionId);

if (!discussionParticipantService.amIHost(discussionId)) {
throw new ApiException(ErrorCode.NOT_ALLOWED_USER);
}

if (isTimeChanged(discussion, request)) {
discussionBitmapService.deleteDiscussionBitmaps(discussionId);
personalEventService.restorePersonalEvents(discussion);
}

discussion.update(request);
discussion = discussionRepository.save(discussion);

return new DiscussionResponse(
discussion.getId(),
discussion.getTitle(),
discussion.getDateRangeStart(),
discussion.getDateRangeEnd(),
discussion.getMeetingMethod(),
discussion.getLocation(),
discussion.getDuration(),
TimeUtil.calculateTimeLeft(discussion.getDeadline())
);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

New update functionality handles permission validation and time-change detection.

The implementation:

  • Validates the user is the host
  • Handles special case when time-related fields change
  • Updates the discussion entity
  • Returns a consistent response structure

However, there's an inconsistency in how bitmap deletion is handled.

In this method, deleteDiscussionBitmaps is called directly, which executes synchronously without the retry mechanism. In contrast, confirmSchedule uses the asynchronous version with retries. For consistency and resilience, consider using the async version here as well:

-discussionBitmapService.deleteDiscussionBitmaps(discussionId);
+discussionBitmapService.deleteDiscussionBitmapsAsync(discussionId)
+    .thenRun(() -> log.info("Redis keys deleted successfully for discussionId : {}", discussionId))
+    .exceptionally(ex -> {
+        log.error("Failed to delete Redis keys for update operation", ex);
+        return null;
+    });
📝 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.

Suggested change
public DiscussionResponse updateDiscussion(Long discussionId, UpdateDiscussionRequest request) {
Discussion discussion = getDiscussionById(discussionId);
if (!discussionParticipantService.amIHost(discussionId)) {
throw new ApiException(ErrorCode.NOT_ALLOWED_USER);
}
if (isTimeChanged(discussion, request)) {
discussionBitmapService.deleteDiscussionBitmaps(discussionId);
personalEventService.restorePersonalEvents(discussion);
}
discussion.update(request);
discussion = discussionRepository.save(discussion);
return new DiscussionResponse(
discussion.getId(),
discussion.getTitle(),
discussion.getDateRangeStart(),
discussion.getDateRangeEnd(),
discussion.getMeetingMethod(),
discussion.getLocation(),
discussion.getDuration(),
TimeUtil.calculateTimeLeft(discussion.getDeadline())
);
}
public DiscussionResponse updateDiscussion(Long discussionId, UpdateDiscussionRequest request) {
Discussion discussion = getDiscussionById(discussionId);
if (!discussionParticipantService.amIHost(discussionId)) {
throw new ApiException(ErrorCode.NOT_ALLOWED_USER);
}
if (isTimeChanged(discussion, request)) {
- discussionBitmapService.deleteDiscussionBitmaps(discussionId);
+ discussionBitmapService.deleteDiscussionBitmapsAsync(discussionId)
+ .thenRun(() -> log.info("Redis keys deleted successfully for discussionId : {}", discussionId))
+ .exceptionally(ex -> {
+ log.error("Failed to delete Redis keys for update operation", ex);
+ return null;
+ });
personalEventService.restorePersonalEvents(discussion);
}
discussion.update(request);
discussion = discussionRepository.save(discussion);
return new DiscussionResponse(
discussion.getId(),
discussion.getTitle(),
discussion.getDateRangeStart(),
discussion.getDateRangeEnd(),
discussion.getMeetingMethod(),
discussion.getLocation(),
discussion.getDuration(),
TimeUtil.calculateTimeLeft(discussion.getDeadline())
);
}

Comment on lines +177 to 179
when(discussionBitmapService.deleteDiscussionBitmapsAsync(
any(Long.class)
)).thenReturn(CompletableFuture.completedFuture(null));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Method name inconsistency between tests.

The method name has been changed from deleteDiscussionBitmapsUsingScan to deleteDiscussionBitmapsAsync here, which indicates a change to asynchronous processing. However, this test uses deleteDiscussionBitmapsAsync while the new tests below use deleteDiscussionBitmaps (non-async version).

Make sure to consistently use the same method name throughout all tests:

when(discussionBitmapService.deleteDiscussionBitmapsAsync(
    any(Long.class)
)).thenReturn(CompletableFuture.completedFuture(null));

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +777 to +822
@DisplayName("updateDiscussion: 시간이 변경된 경우 (host) - bitmap 삭제 및 개인 일정 복원 호출")
@Test
public void updateDiscussion_whenTimeChanged_callsBitmapDeletionAndRestore() {
// Given
Long discussionId = 1L;
Discussion discussion = Discussion.builder()
.title("Test Discussion")
.dateStart(LocalDate.of(2025, 3, 1))
.dateEnd(LocalDate.of(2025, 3, 1))
.timeStart(LocalTime.of(10, 0))
.timeEnd(LocalTime.of(12, 0))
.duration(120)
.deadline(LocalDate.now().plusDays(10))
.meetingMethod(MeetingMethod.ONLINE)
.location("Test Location")
.build();
discussion.setDiscussionStatus(DiscussionStatus.ONGOING);
ReflectionTestUtils.setField(discussion, "id", discussionId);

UpdateDiscussionRequest request = new UpdateDiscussionRequest(
"team meeting",
LocalDate.of(2025, 3, 1),
LocalDate.of(2025, 3, 2),
LocalTime.of(10, 0),
LocalTime.of(12, 0),
120,
MeetingMethod.ONLINE,
"회의실 1",
LocalDate.now().plusDays(10)
);

when(discussionRepository.findById(discussionId)).thenReturn(Optional.of(discussion));
when(discussionParticipantService.amIHost(discussionId)).thenReturn(true);
when(discussionRepository.save(discussion)).thenReturn(discussion);

// when
DiscussionResponse response = discussionService.updateDiscussion(discussionId, request);

// then
verify(discussionBitmapService).deleteDiscussionBitmaps(discussionId);
verify(personalEventService).restorePersonalEvents(discussion);

assertThat(response).isNotNull();
assertThat(response.id()).isEqualTo(discussionId);
assertThat(response.title()).isEqualTo("team meeting");
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Thorough test for time change scenario, but method name inconsistency.

This test comprehensively checks the behavior when discussion time changes. It verifies that bitmap deletion and personal event restoration are triggered. However, there's an inconsistency in the method name used for verification.

Update the verification to use the async method name for consistency:

// then
-verify(discussionBitmapService).deleteDiscussionBitmaps(discussionId);
+verify(discussionBitmapService).deleteDiscussionBitmapsAsync(discussionId);
verify(personalEventService).restorePersonalEvents(discussion);

Additionally, consider using fixed dates for deadline rather than relative dates like LocalDate.now().plusDays(10) to make tests more deterministic and less time-sensitive.

📝 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.

Suggested change
@DisplayName("updateDiscussion: 시간이 변경된 경우 (host) - bitmap 삭제 및 개인 일정 복원 호출")
@Test
public void updateDiscussion_whenTimeChanged_callsBitmapDeletionAndRestore() {
// Given
Long discussionId = 1L;
Discussion discussion = Discussion.builder()
.title("Test Discussion")
.dateStart(LocalDate.of(2025, 3, 1))
.dateEnd(LocalDate.of(2025, 3, 1))
.timeStart(LocalTime.of(10, 0))
.timeEnd(LocalTime.of(12, 0))
.duration(120)
.deadline(LocalDate.now().plusDays(10))
.meetingMethod(MeetingMethod.ONLINE)
.location("Test Location")
.build();
discussion.setDiscussionStatus(DiscussionStatus.ONGOING);
ReflectionTestUtils.setField(discussion, "id", discussionId);
UpdateDiscussionRequest request = new UpdateDiscussionRequest(
"team meeting",
LocalDate.of(2025, 3, 1),
LocalDate.of(2025, 3, 2),
LocalTime.of(10, 0),
LocalTime.of(12, 0),
120,
MeetingMethod.ONLINE,
"회의실 1",
LocalDate.now().plusDays(10)
);
when(discussionRepository.findById(discussionId)).thenReturn(Optional.of(discussion));
when(discussionParticipantService.amIHost(discussionId)).thenReturn(true);
when(discussionRepository.save(discussion)).thenReturn(discussion);
// when
DiscussionResponse response = discussionService.updateDiscussion(discussionId, request);
// then
verify(discussionBitmapService).deleteDiscussionBitmaps(discussionId);
verify(personalEventService).restorePersonalEvents(discussion);
assertThat(response).isNotNull();
assertThat(response.id()).isEqualTo(discussionId);
assertThat(response.title()).isEqualTo("team meeting");
}
@DisplayName("updateDiscussion: 시간이 변경된 경우 (host) - bitmap 삭제 및 개인 일정 복원 호출")
@Test
public void updateDiscussion_whenTimeChanged_callsBitmapDeletionAndRestore() {
// Given
Long discussionId = 1L;
Discussion discussion = Discussion.builder()
.title("Test Discussion")
.dateStart(LocalDate.of(2025, 3, 1))
.dateEnd(LocalDate.of(2025, 3, 1))
.timeStart(LocalTime.of(10, 0))
.timeEnd(LocalTime.of(12, 0))
.duration(120)
.deadline(LocalDate.now().plusDays(10))
.meetingMethod(MeetingMethod.ONLINE)
.location("Test Location")
.build();
discussion.setDiscussionStatus(DiscussionStatus.ONGOING);
ReflectionTestUtils.setField(discussion, "id", discussionId);
UpdateDiscussionRequest request = new UpdateDiscussionRequest(
"team meeting",
LocalDate.of(2025, 3, 1),
LocalDate.of(2025, 3, 2),
LocalTime.of(10, 0),
LocalTime.of(12, 0),
120,
MeetingMethod.ONLINE,
"회의실 1",
LocalDate.now().plusDays(10)
);
when(discussionRepository.findById(discussionId)).thenReturn(Optional.of(discussion));
when(discussionParticipantService.amIHost(discussionId)).thenReturn(true);
when(discussionRepository.save(discussion)).thenReturn(discussion);
// when
DiscussionResponse response = discussionService.updateDiscussion(discussionId, request);
// then
- verify(discussionBitmapService).deleteDiscussionBitmaps(discussionId);
+ verify(discussionBitmapService).deleteDiscussionBitmapsAsync(discussionId);
verify(personalEventService).restorePersonalEvents(discussion);
assertThat(response).isNotNull();
assertThat(response.id()).isEqualTo(discussionId);
assertThat(response.title()).isEqualTo("team meeting");
}

Copy link
Contributor

@kwon204 kwon204 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다

@efdao efdao merged commit d33db15 into dev Mar 27, 2025
7 checks passed
@efdao efdao deleted the feature/be/update-discussion-info branch March 27, 2025 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🛠️ BE Backend

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] 논의 CRUD 완성

3 participants