Conversation
|
Caution Review failedThe pull request is closed. Walkthrough이 PR은 모임 V2 도메인 모델 전체를 재설계합니다. 14개의 새로운 엔티티/열거형을 추가하여 모임 상태 전이, 이미지 변형 관리, 사용자 멤버십 생명주기, 위치 검증을 지원합니다. 동시에 모임/이미지/위치/상태 검증을 위한 8개의 새로운 오류 코드를 추가합니다. Changes
예상 코드 리뷰 노력🎯 4 (복잡함) | ⏱️ ~45분 다음 부분들에 특별한 주의가 필요합니다:
관련된 PR들
시
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (15)
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. Comment |
github-project-automation
bot
moved this from In progress
to Done
in WeGo-Together Backend
There was a problem hiding this comment.
Pull request overview
This PR introduces a redesigned V2 version of the Group entity domain model, adding new entities and supporting classes for an improved group management system with enhanced image handling, user role management, and status transitions.
Key Changes:
- New entity model with GroupV2 as the aggregate root managing relationships with users, images, and tags
- State machine implementation for group status transitions (RECRUITING → FULL/CLOSED/CANCELLED/FINISHED)
- Image variant support with multiple sizes and formats (CARD_440_240, THUMBNAIL_100_100) for optimized display
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 24 comments.
Show a summary per file
| File | Description |
|---|---|
| GroupV2Repository.java | Updated repository to reference new GroupV2 entity instead of legacy Group entity |
| ImageV2Format.java | New enum defining supported image formats (PNG, JPEG, WEBP) |
| GroupV2Status.java | New enum with state machine logic for group lifecycle status transitions |
| GroupV2Address.java | New embeddable value object for location information with validation |
| GroupV2.java | Core aggregate root entity with bidirectional relationships and status management |
| GroupUserV2Status.java | New enum for user participation states (ATTEND, LEFT, KICKED, BANNED) |
| GroupUserV2Role.java | New enum defining user roles (HOST, MANAGER, MEMBER) |
| GroupUserV2.java | Join entity managing group-user relationships with role and status |
| GroupTagV2.java | Join entity for group-tag associations |
| GroupImageV2VariantType.java | Enum defining image variant types with dimensions and default formats |
| GroupImageV2Variant.java | Entity storing different size/format variants of group images |
| GroupImageV2.java | Entity managing group images with sort order and multiple variants |
| GetGroupV2Response.java | Minor formatting fix (trailing newline added) |
| GetGroupListV2Response.java | Minor formatting fix (trailing newline added) |
| GroupErrorCode.java | Added 8 new error codes for V2 validation scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private GroupImageV2(GroupV2 group, int sortOrder) { | ||
| this.sortOrder = sortOrder; | ||
| assignTo(group); | ||
| group.addImage(this); | ||
| } |
There was a problem hiding this comment.
The constructor creates a circular bidirectional call pattern. It calls assignTo(group) which sets this.group, and then immediately calls group.addImage(this), which in turn calls assignTo again on the same image. This results in the image being added to the group's images list during construction. While this works, it's unconventional and can cause issues if someone later calls GroupImageV2.create() followed by group.addImage() manually, resulting in duplicate entries. Consider refactoring to either handle the bidirectional relationship in one place or document this behavior clearly.
| this.images.remove(image); | ||
| image.unassign(); |
There was a problem hiding this comment.
The removeImage method could fail silently if the image is not in the list. The List.remove() method will return false if the item doesn't exist, but this is not checked. Additionally, calling unassign() on an image that wasn't in the list will still set its group to null. Consider checking if the image exists in the list before calling unassign(), or returning a boolean to indicate success.
| this.images.remove(image); | |
| image.unassign(); | |
| if (this.images.remove(image)) { | |
| image.unassign(); | |
| } |
| public boolean canTransitionTo(GroupV2Status status) { | ||
| return allowedNext(this).contains(status); | ||
| } | ||
|
|
||
| private static Set<GroupV2Status> allowedNext(GroupV2Status current) { | ||
| return switch (current) { | ||
| case RECRUITING -> EnumSet.of(FULL, CLOSED, CANCELLED, FINISHED); | ||
| case FULL -> EnumSet.of(RECRUITING, CLOSED, CANCELLED, FINISHED); | ||
| case CLOSED -> EnumSet.of(CANCELLED, FINISHED); | ||
| case CANCELLED, FINISHED -> EnumSet.noneOf(GroupV2Status.class); | ||
| }; | ||
| } |
There was a problem hiding this comment.
The canTransitionTo method and the state transition logic are not documented. The allowed state transitions are business-critical and non-obvious (for example, why CLOSED can transition to CANCELLED but CANCELLED cannot transition to anything). Add Javadoc comments explaining the state machine, the purpose of each transition, and why certain transitions are not allowed.
| public void addTag(GroupTagV2 groupTag) { | ||
| this.groupTags.add(groupTag); | ||
| groupTag.assignTo(this); | ||
| } |
There was a problem hiding this comment.
The addTag method has the same circular dependency issue as addImage and addUser. The method calls groupTag.assignTo(this), but in GroupTagV2.create(), it already calls group.addTag(groupTag) after creating the instance. This can lead to duplicate additions in the groupTags list. Consider refactoring to avoid the bidirectional method calls or add a check to prevent duplicate additions.
| public static GroupImageV2Variant create( | ||
| GroupImageV2VariantType type, | ||
| String imageUrl) { | ||
| return new GroupImageV2Variant(type, imageUrl, type.defaultFormat()); | ||
| } | ||
|
|
||
| public static GroupImageV2Variant create( | ||
| GroupImageV2VariantType type, | ||
| String imageUrl, | ||
| ImageV2Format format) { | ||
| return new GroupImageV2Variant(type, imageUrl, format); | ||
| } |
There was a problem hiding this comment.
The create methods don't validate the imageUrl parameter. Null or blank URLs are not checked, which could violate the @column(nullable = false) constraint and cause database errors. Add validation to ensure imageUrl is not null or blank before creating the variant.
| @Table(name = "v2_group_tags") | ||
| @Entity |
There was a problem hiding this comment.
The GroupTagV2 entity is missing a unique constraint on the combination of group_id and tag_id. Without this constraint, the same tag could be added to a group multiple times, creating duplicate entries. Add a unique constraint similar to the one in GroupUserV2 and GroupImageV2 to prevent duplicate group-tag associations.
| import lombok.AccessLevel; | ||
| import lombok.Getter; | ||
|
|
||
| @Getter(AccessLevel.PUBLIC) |
There was a problem hiding this comment.
The annotation @Getter(AccessLevel.PUBLIC) on an enum is unnecessary. Enum constants are inherently public and accessible. Since enum values don't have fields requiring getters, this annotation serves no purpose and should be removed.
| import lombok.AccessLevel; | |
| import lombok.Getter; | |
| @Getter(AccessLevel.PUBLIC) |
|
|
||
| public int width() { | ||
| return width; | ||
| } | ||
|
|
||
| public int height() { | ||
| return height; | ||
| } | ||
|
|
||
| public ImageV2Format defaultFormat() { | ||
| return defaultFormat; | ||
| } |
There was a problem hiding this comment.
The methods width(), height(), and defaultFormat() are redundant as Lombok's @Getter annotation already generates getWidth(), getHeight(), and getDefaultFormat() methods. These duplicate methods add unnecessary code without providing additional value. Remove these methods and rely on the Lombok-generated getters.
| public int width() { | |
| return width; | |
| } | |
| public int height() { | |
| return height; | |
| } | |
| public ImageV2Format defaultFormat() { | |
| return defaultFormat; | |
| } |
| changeStatus(GroupV2Status.FINISHED); | ||
| } | ||
|
|
||
| public void addImage(GroupImageV2 image) { |
There was a problem hiding this comment.
The addImage method can lead to duplicate additions and potential infinite recursion. The method calls image.assignTo(this), but in GroupImageV2.create(), it already calls group.addImage(this) after assignTo(group). This creates a circular dependency where addImage→assignTo is called, and then create→assignTo→addImage is called. Consider refactoring to avoid the bidirectional method calls or add a check to prevent duplicate additions.
| public void addImage(GroupImageV2 image) { | |
| public void addImage(GroupImageV2 image) { | |
| if (this.images.contains(image)) { | |
| return; | |
| } |
| public void addUser(GroupUserV2 groupUser) { | ||
| this.users.add(groupUser); | ||
| groupUser.assignTo(this); | ||
| } |
There was a problem hiding this comment.
The addUser method has the same circular dependency issue as addImage. The method calls groupUser.assignTo(this), but in GroupUserV2.create(), it already calls group.addUser(groupUser) after creating the instance. This can lead to duplicate additions in the users list and potential inconsistencies. Consider refactoring to avoid the bidirectional method calls or add a check to prevent duplicate additions.
1 participant
📝 Pull Request
📌 PR 종류
해당하는 항목에 체크해주세요.
✨ 변경 내용
모임 v2 엔티티 재설계와 예외 코드가 추가 되었습니다. 자세한 모임 v2에 대한 설계 구조는 별도로 정리하겠습니다.
🔍 관련 이슈
🧪 테스트
변경된 기능에 대한 테스트 범위 또는 테스트 결과를 작성해주세요.
🚨 확인해야 할 사항 (Checklist)
PR을 제출하기 전에 아래 항목들을 확인해주세요.
🙋 기타 참고 사항
리뷰어가 참고하면 좋을 만한 추가 설명이 있다면 적어주세요.
Summary by CodeRabbit
릴리스 노트
✏️ Tip: You can customize this high-level summary in your review settings.