Merged
Conversation
Walkthrough권한 제거 기능을 구현하기 위해 새로운 DELETE 엔드포인트를 추가하고 RemovePermissionUseCase와 RemovePermissionService를 도입합니다. AddPermissionCommand를 PermissionCommand로 일반화하여 기존 권한 추가 기능을 리팩터링하고, 필요한 DTO 및 Repository 메서드를 추가합니다. Changes
Sequence DiagramsequenceDiagram
participant Client
participant PermissionController
participant RemovePermissionService
participant GroupRoleRepositoryPort
participant GroupRoleRepositoryAdapter
Client->>PermissionController: DELETE /v1/groups/{groupId}/permissions
PermissionController->>RemovePermissionService: removePermission(PermissionCommand)
RemovePermissionService->>GroupRoleRepositoryPort: findRole(userId, groupId)
GroupRoleRepositoryPort->>GroupRoleRepositoryAdapter: findRole(userId, groupId)
GroupRoleRepositoryAdapter-->>GroupRoleRepositoryPort: GroupMemberRole
GroupRoleRepositoryPort-->>RemovePermissionService: GroupMemberRole
RemovePermissionService->>RemovePermissionService: 요청자 권한 > 변경 대상 권한 검증
RemovePermissionService->>RemovePermissionService: 요청자 권한 보유 여부 확인
RemovePermissionService->>RemovePermissionService: 변경 대상 권한 존재 여부 확인
RemovePermissionService->>GroupRoleRepositoryPort: removePermission(groupId, role, permission)
GroupRoleRepositoryPort->>GroupRoleRepositoryAdapter: removePermission(groupId, role, permission)
GroupRoleRepositoryAdapter->>GroupRoleRepositoryAdapter: 권한 삭제 및 남은 권한 조회
GroupRoleRepositoryAdapter-->>GroupRoleRepositoryPort: List<GroupPermission>
GroupRoleRepositoryPort-->>RemovePermissionService: List<GroupPermission>
RemovePermissionService-->>PermissionController: RemovePermissionResult
PermissionController-->>Client: RemovePermissionResponseDto
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![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
🧹 Nitpick comments (3)
src/main/resources/application.yml (1)
55-55: 기본 프로필 DEBUG 설정은 운영 리스크가 있습니다.Line 55의
flipnote.group: DEBUG는 기본 설정에서 로그량 증가와 민감 정보 노출 가능성을 키웁니다. 기본은INFO로 두고, 디버그는 로컬/개발 프로필로 분리하는 편이 안전합니다.제안 변경안
logging: level: - flipnote.group: DEBUG + flipnote.group: INFO🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/application.yml` at line 55, 현재 기본 설정에 flipnote.group: DEBUG로 되어 있어 운영 리스크가 있으니 기본 로그 레벨을 DEBUG에서 INFO로 변경하고, DEBUG 레벨은 개발 전용 프로파일로 분리하세요; 구체적으로 application.yml의 flipnote.group 값을 INFO로 바꾸고 개발용 설정(예: application-dev.yml 또는 application.yml 내 spring.profiles: dev 블록)에 flipnote.group: DEBUG를 옮겨 로컬/개발 환경에서만 활성화되도록 하세요.src/main/java/flipnote/group/adapter/in/web/PermissionController.java (1)
58-70: 삭제 엔드포인트의 응답 타입을 구체 DTO로 고정하세요.
ResponseEntity<?>대신ResponseEntity<RemovePermissionResponseDto>를 사용하면 API 계약이 명확해지고 타입 안정성이 좋아집니다.🔧 제안 변경
- public ResponseEntity<?> changeDownPermission( + public ResponseEntity<RemovePermissionResponseDto> changeDownPermission( `@RequestHeader`("X-USER-ID") Long userId, `@PathVariable`("groupId") Long groupId, `@Valid` `@RequestBody` RemovePermissionRequestDto req) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/flipnote/group/adapter/in/web/PermissionController.java` around lines 58 - 70, Update the controller method signature and return type to be concrete: change changeDownPermission's return type from ResponseEntity<?> to ResponseEntity<RemovePermissionResponseDto>, and ensure the method returns ResponseEntity.ok(res) where res is the RemovePermissionResponseDto already built; locate the changeDownPermission method and the RemovePermissionResponseDto usage (and keep PermissionCommand and removePermissionUseCase.removePermission(cmd) calls unchanged) so the method signature, any annotations, and imports reflect ResponseEntity<RemovePermissionResponseDto> for type safety.src/main/java/flipnote/group/application/service/RemovePermissionService.java (1)
27-50: 권한 검증 규칙을 공통 컴포넌트로 추출하는 것을 권장합니다.현재 검증 흐름이
AddPermissionService와 거의 동일하게 반복되어, 이후 정책 변경 시 한쪽만 수정되는 드리프트 위험이 있습니다.PermissionValidationPolicy(또는 Validator)로 추출해 add/remove에서 재사용하는 편이 안전합니다.♻️ 리팩터링 예시
public class RemovePermissionService implements RemovePermissionUseCase { + private final PermissionValidationPolicy permissionValidationPolicy; private final GroupRoleRepositoryPort groupRoleRepository; `@Override` `@Transactional` public RemovePermissionResult removePermission(PermissionCommand cmd) { GroupMemberRole role = groupRoleRepository.findRole(cmd.userId(), cmd.groupId()); - - if(!role.isHigherThan(cmd.changeRole())) { - throw new IllegalArgumentException("host lower than changeRole"); - } - - boolean existHostPermission = groupRoleRepository.checkPermission(cmd.userId(), cmd.groupId(), cmd.permission()); - if(!existHostPermission) { - throw new IllegalArgumentException("host not exist permission"); - } - - boolean existPermission = groupRoleRepository.existPermission(cmd.changeRole(), cmd.groupId(), cmd.permission()); - if(!existPermission) { - throw new IllegalArgumentException("role not exist permission"); - } + permissionValidationPolicy.validateManagePermission(cmd, role); List<GroupPermission> groupPermissions = groupRoleRepository.removePermission( cmd.groupId(), cmd.changeRole(), cmd.permission() );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/flipnote/group/application/service/RemovePermissionService.java` around lines 27 - 50, The permission validation logic duplicated between RemovePermissionService.removePermission and AddPermissionService should be extracted into a shared component (e.g., PermissionValidationPolicy or PermissionValidator) to avoid drift; create a new class (PermissionValidationPolicy) that encapsulates checks currently performed via groupRoleRepository.findRole(...), role.isHigherThan(...), groupRoleRepository.checkPermission(...), and groupRoleRepository.existPermission(...), then replace the inline checks in RemovePermissionService.removePermission (and mirror in AddPermissionService) with calls to the new policy methods so both services reuse the same validation flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/flipnote/group/adapter/out/persistence/GroupRoleRepositoryAdapter.java`:
- Around line 179-181: Update the thrown IllegalArgumentException in
GroupRoleRepositoryAdapter where roleEntity is fetched (the
findByGroupIdAndRole(...) orElseThrow) to use a message that reflects a missing
role (e.g., "role not found for group" or include groupId and role) instead of
"not exists member" so the error matches the failure in role lookup.
---
Nitpick comments:
In `@src/main/java/flipnote/group/adapter/in/web/PermissionController.java`:
- Around line 58-70: Update the controller method signature and return type to
be concrete: change changeDownPermission's return type from ResponseEntity<?> to
ResponseEntity<RemovePermissionResponseDto>, and ensure the method returns
ResponseEntity.ok(res) where res is the RemovePermissionResponseDto already
built; locate the changeDownPermission method and the
RemovePermissionResponseDto usage (and keep PermissionCommand and
removePermissionUseCase.removePermission(cmd) calls unchanged) so the method
signature, any annotations, and imports reflect
ResponseEntity<RemovePermissionResponseDto> for type safety.
In
`@src/main/java/flipnote/group/application/service/RemovePermissionService.java`:
- Around line 27-50: The permission validation logic duplicated between
RemovePermissionService.removePermission and AddPermissionService should be
extracted into a shared component (e.g., PermissionValidationPolicy or
PermissionValidator) to avoid drift; create a new class
(PermissionValidationPolicy) that encapsulates checks currently performed via
groupRoleRepository.findRole(...), role.isHigherThan(...),
groupRoleRepository.checkPermission(...), and
groupRoleRepository.existPermission(...), then replace the inline checks in
RemovePermissionService.removePermission (and mirror in AddPermissionService)
with calls to the new policy methods so both services reuse the same validation
flow.
In `@src/main/resources/application.yml`:
- Line 55: 현재 기본 설정에 flipnote.group: DEBUG로 되어 있어 운영 리스크가 있으니 기본 로그 레벨을 DEBUG에서
INFO로 변경하고, DEBUG 레벨은 개발 전용 프로파일로 분리하세요; 구체적으로 application.yml의 flipnote.group
값을 INFO로 바꾸고 개발용 설정(예: application-dev.yml 또는 application.yml 내 spring.profiles:
dev 블록)에 flipnote.group: DEBUG를 옮겨 로컬/개발 환경에서만 활성화되도록 하세요.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/main/java/flipnote/group/adapter/in/web/PermissionController.javasrc/main/java/flipnote/group/adapter/out/persistence/GroupRoleRepositoryAdapter.javasrc/main/java/flipnote/group/api/dto/request/RemovePermissionRequestDto.javasrc/main/java/flipnote/group/api/dto/response/RemovePermissionResponseDto.javasrc/main/java/flipnote/group/application/port/in/AddPermissionUseCase.javasrc/main/java/flipnote/group/application/port/in/RemovePermissionUseCase.javasrc/main/java/flipnote/group/application/port/in/command/PermissionCommand.javasrc/main/java/flipnote/group/application/port/in/result/RemovePermissionResult.javasrc/main/java/flipnote/group/application/port/out/GroupRoleRepositoryPort.javasrc/main/java/flipnote/group/application/service/AddPermissionService.javasrc/main/java/flipnote/group/application/service/RemovePermissionService.javasrc/main/java/flipnote/group/domain/model/member/GroupMemberRole.javasrc/main/java/flipnote/group/infrastructure/persistence/jpa/GroupRolePermissionRepository.javasrc/main/resources/application.yml
💤 Files with no reviewable changes (1)
- src/main/java/flipnote/group/domain/model/member/GroupMemberRole.java
src/main/java/flipnote/group/adapter/out/persistence/GroupRoleRepositoryAdapter.java
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
릴리스 노트
New Features
Refactor