[BE-Feat] 논의 삭제 기능 구현 #386
Conversation
WalkthroughThe pull request introduces a deletion workflow for discussions and their participants. A new DELETE endpoint is added to the discussion controller, and corresponding service methods in the DiscussionService and DiscussionParticipantService handle validations such as host verification and discussion status checks. Additionally, repository methods for retrieving and bulk deleting discussion participants by discussionId are added. The changes are complemented by comprehensive tests that cover successful deletion scenarios and error cases where deletion is not permitted. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant DC as DiscussionController
participant DS as DiscussionService
participant DPS as DiscussionParticipantService
participant DMS as DiscussionBitmapService
participant DR as DiscussionRepository
C->>DC: DELETE /discussions/{discussionId}
DC->>DS: deleteDiscussion(discussionId)
DS->>DS: getDiscussionByIdWithLock(discussionId)
DS->>DS: verify current user is host
alt Not Host
DS-->>C: ApiException (NOT_ALLOWED_USER)
else
DS->>DS: check discussion status (ONGOING)
alt Not Ongoing
DS-->>C: ApiException (DISCUSSION_NOT_ONGOING)
else
DS->>DMS: deleteDiscussionBitmaps(discussionId)
DS->>DPS: deleteDiscussionParticipants(discussionId)
DPS->>DPS: deleteAllByDiscussionId(discussionId)
DS->>DR: delete discussion entity
DS-->>C: 204 No Content
end
end
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📥 CommitsReviewing files that changed from the base of the PR and between 8150c71dfefbb4f204334c2a5825b3faf23a15d2 and f121860. 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (2)
backend/src/main/java/endolphin/backend/domain/discussion/DiscussionController.java (1)
313-332: Well-designed DELETE endpoint with comprehensive API documentationThe endpoint is correctly implemented using proper HTTP method (DELETE), path variable validation, and response status (204 No Content). The OpenAPI documentation is thorough, covering all potential response scenarios.
Consider adding logging for this operation to aid in troubleshooting and auditing:
@DeleteMapping("/{discussionId}") public ResponseEntity<Void> deleteDiscussion( @PathVariable("discussionId") @Min(1) Long discussionId) { + log.info("Deleting discussion with ID: {}", discussionId); discussionService.deleteDiscussion(discussionId); + log.info("Successfully deleted discussion with ID: {}", discussionId); return ResponseEntity.noContent().build(); }backend/src/main/java/endolphin/backend/domain/discussion/DiscussionParticipantService.java (1)
83-88: Consider graceful handling of the "no participants" case in deletion methodThe current implementation will throw an exception if no participants are found, which may not be the desired behavior for a deletion operation.
Consider modifying the implementation to handle the case where no participants exist more gracefully:
public void deleteDiscussionParticipants(Long discussionId) { - List<DiscussionParticipant> participants = getDiscussionParticipantsByDiscussionId( - discussionId); - - discussionParticipantRepository.deleteAll(participants); + List<DiscussionParticipant> participants = discussionParticipantRepository.findByDiscussionId( + discussionId); + + if (!participants.isEmpty()) { + discussionParticipantRepository.deleteAll(participants); + } }This approach treats "no participants found" as a valid state for deletion rather than an error condition.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 5aa4921 and 8150c71dfefbb4f204334c2a5825b3faf23a15d2.
📒 Files selected for processing (6)
backend/src/main/java/endolphin/backend/domain/discussion/DiscussionController.java(1 hunks)backend/src/main/java/endolphin/backend/domain/discussion/DiscussionParticipantRepository.java(1 hunks)backend/src/main/java/endolphin/backend/domain/discussion/DiscussionParticipantService.java(1 hunks)backend/src/main/java/endolphin/backend/domain/discussion/DiscussionService.java(1 hunks)backend/src/test/java/endolphin/backend/domain/discussion/DiscussionParticipantServiceTest.java(2 hunks)backend/src/test/java/endolphin/backend/domain/discussion/DiscussionServiceTest.java(1 hunks)
🔇 Additional comments (8)
backend/src/main/java/endolphin/backend/domain/discussion/DiscussionParticipantRepository.java (1)
119-120: Clean implementation of the repository methodThe newly added method follows Spring Data JPA's naming convention for automatic query generation, which is appropriate for this simple query. This method will be useful for retrieving all participants associated with a discussion, which is necessary for the deletion workflow.
backend/src/main/java/endolphin/backend/domain/discussion/DiscussionService.java (1)
398-412: Well-structured discussion deletion logic with appropriate validationsThe method properly implements the requirements by:
- Verifying the user is the host
- Ensuring the discussion status is ONGOING
- Deleting associated data in the correct order (bitmaps, participants, then the discussion)
The implementation is thorough and handles the business rules specified in the PR objectives.
backend/src/main/java/endolphin/backend/domain/discussion/DiscussionParticipantService.java (1)
71-81: Good implementation with appropriate transaction and error handlingThe method is correctly annotated with
@Transactional(readOnly = true)for a read operation and properly validates the existence of participants.backend/src/test/java/endolphin/backend/domain/discussion/DiscussionParticipantServiceTest.java (2)
613-629: Test correctly verifies discussion participant deletion.The test properly verifies that the
deleteDiscussionParticipantsmethod works by:
- Setting up mock discussion participants
- Verifying the repository's
deleteAllmethod is called with the correct participantsThis test aligns with requirement #4 from the PR objectives (deleting participants associated with a discussion).
631-644: Test properly handles the error case for participant deletion.This test correctly verifies that an exception is thrown when attempting to delete participants for a discussion that has none. The test ensures:
- The error code matches
DISCUSSION_PARTICIPANT_NOT_FOUND- The service method properly validates input before proceeding
Good practice to test both success and error scenarios.
backend/src/test/java/endolphin/backend/domain/discussion/DiscussionServiceTest.java (3)
906-927: Test correctly verifies the complete discussion deletion workflow.This test properly validates the discussion deletion process by:
- Setting up a discussion with ONGOING status
- Verifying the user is the host before proceeding
- Checking that all required deletion steps are called in order:
- Deleting discussion bitmaps
- Deleting discussion participants
- Deleting the discussion itself
This aligns with all the requirements specified in the PR objectives.
929-951: Test correctly verifies authorization for discussion deletion.The test properly checks that only the host can delete a discussion by:
- Setting up a scenario where the current user is not the host
- Verifying that an
ApiExceptionwith error codeNOT_ALLOWED_USERis thrown- Ensuring no deletion methods are called in this case
Good security practice to verify authorization before performing destructive operations.
953-975: Test correctly verifies discussion status validation.This test properly validates that only discussions with ONGOING status can be deleted by:
- Setting up a discussion with FINISHED status
- Verifying that an
ApiExceptionwith error codeDISCUSSION_NOT_ONGOINGis thrown- Ensuring no deletion methods are called in this case
This aligns with requirement #2 from the PR objectives (error when discussion status is not ONGOING).
backend/src/main/java/endolphin/backend/domain/discussion/DiscussionParticipantService.java
Outdated
Show resolved
Hide resolved
# Conflicts: # backend/src/main/java/endolphin/backend/domain/discussion/DiscussionParticipantRepository.java
efdao
force-pushed
the
feature/be/delete-discussion
branch
from
8150c71 to
f121860
Compare
#️⃣ 연관된 이슈>
📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)
🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요
Summary by CodeRabbit