Issue: (#83) MY_03 알림 기능 수정 및 보완 #84
Conversation
Walkthrough이번 변경에서는 알림(Notification) 관련 도메인 로직과 API가 확장 및 리팩토링되었습니다. Notification 엔티티의 Boolean 필드 업데이트 메서드가 명시적으로 값을 받도록 변경되었고, 회원의 알림 정책을 조회하는 새로운 GET API가 추가되었습니다. 이를 위해 DTO와 컨트롤러, 파사드, 서비스 계층이 수정 및 보완되었습니다. 또한 테스트 코드가 새로운 로직에 맞게 보강되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Controller
participant Facade
participant Service
participant Repository
participant DTO
User->>Controller: GET /v1/member/my-notification
Controller->>Facade: getMyNotification(customUserDetails)
Facade->>Repository: findNotificationByUserId(userId)
Repository-->>Facade: Notification
Facade->>DTO: MyNotificationResponse.of(Notification)
DTO-->>Facade: MyNotificationResponse
Facade-->>Controller: MyNotificationResponse
Controller-->>User: ApiResponse<MyNotificationResponse>
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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
Documentation and Community
|
![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)
.github/workflows/deploy.yml (1)
6-7: pull_request 이벤트 트리거 추가
main브랜치를 대상으로 PR 이벤트에서 CI/CD가 실행되도록 설정된 것은 적절합니다. 다만, 외부 포크 PR에서는 secrets 접근이 차단되어 워크플로가 실패할 수 있으므로, 경우에 따라pull_request_target이벤트 사용이나 secrets 스텝에 대한 조건부 실행 설정을 고려해 보세요.src/main/kotlin/gomushin/backend/member/facade/MemberInfoFacade.kt (1)
65-68: 알림 설정 조회 기능 추가사용자의 알림 설정을 조회하는 새로운 메서드가 올바르게 구현되었습니다. 트랜잭션 어노테이션이 누락되었지만, 읽기 전용 작업이므로 필수는 아닙니다. 다만, 읽기 전용 트랜잭션으로 명시하는 것을 고려해볼 수 있습니다.
+@Transactional(readOnly = true) fun getMyNotification(customUserDetails: CustomUserDetails): MyNotificationResponse { val notification = notificationService.getByMemberId(customUserDetails.getId()) return MyNotificationResponse.of(notification) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/deploy.yml(1 hunks)src/main/kotlin/gomushin/backend/member/domain/entity/Notification.kt(1 hunks)src/main/kotlin/gomushin/backend/member/dto/response/MyNotificationResponse.kt(1 hunks)src/main/kotlin/gomushin/backend/member/facade/MemberInfoFacade.kt(2 hunks)src/main/kotlin/gomushin/backend/member/presentation/ApiPath.kt(1 hunks)src/main/kotlin/gomushin/backend/member/presentation/MemberInfoController.kt(2 hunks)src/test/kotlin/gomushin/backend/member/facade/MemberInfoFacadeTest.kt(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/kotlin/gomushin/backend/member/facade/MemberInfoFacade.kt (1)
src/main/kotlin/gomushin/backend/member/domain/entity/Notification.kt (2)
changeDday(41-43)changePartnerStatus(45-47)
🔇 Additional comments (8)
src/main/kotlin/gomushin/backend/member/presentation/ApiPath.kt (1)
13-13: 중복된 API 경로 상수가 있습니다.
UPDATE_MY_NOTIFICATION_POLICY(11줄)와MY_NOTIFICATION(13줄) 상수가 동일한 값"/v1/member/my-notification"을 가지고 있습니다. 두 상수는 HTTP 메서드(POST/GET)로만 구분되므로 혼란을 야기할 수 있습니다.가능한 개선 방안:
- 경로 값에 HTTP 메서드를 포함시키거나 (예:
/v1/member/my-notification/get,/v1/member/my-notification/update)- 상수 이름에 HTTP 메서드를 포함시키는 것 (예:
GET_MY_NOTIFICATION,UPDATE_MY_NOTIFICATION)이것은 코드의 명확성을 위한 제안이며, 현재 구현도 기술적으로는 문제가 없습니다.
src/test/kotlin/gomushin/backend/member/facade/MemberInfoFacadeTest.kt (2)
175-182: 코드 변경이 요구 사항을 충족하며 적절하게 구현되었습니다.기존 토글 방식에서 boolean 값을 직접 설정하는 방식으로 변경되었으며, 테스트 코드도 이에 맞게 잘 수정되었습니다.
184-194: 알림 설정 조회 테스트가 잘 추가되었습니다.새로운 알림 설정 조회 기능에 대한 테스트 케이스가 잘 작성되었습니다. given-when-then 패턴을 따라 모든 필요한 검증을 포함하고 있습니다.
src/main/kotlin/gomushin/backend/member/presentation/MemberInfoController.kt (1)
117-125: 새로운 알림 설정 조회 API 엔드포인트가 잘 구현되었습니다.알림 설정을 조회하는 새로운 GET 엔드포인트가 컨트롤러에 적절하게 추가되었습니다. Swagger 어노테이션을 포함한 코드 구조가 기존 패턴을 잘 따르고 있습니다.
src/main/kotlin/gomushin/backend/member/domain/entity/Notification.kt (1)
41-47: 직접 값을 설정하는 메서드 추가가 적절하게 구현되었습니다.기존의 토글 메서드(
updateDday,updatePartnerStatus)와 함께 값을 직접 설정할 수 있는 새로운 메서드(changeDday,changePartnerStatus)가 잘 추가되었습니다. 이는 PR의 목표인 알림 설정 방식 변경을 적절히 지원합니다.src/main/kotlin/gomushin/backend/member/dto/response/MyNotificationResponse.kt (1)
1-18: 적절한 응답 DTO 구현알림 설정을 조회하는 API의 응답 데이터를 위한 DTO가 잘 구현되었습니다. Swagger 문서화를 위한 Schema 어노테이션도 적절히 적용되어 있고, companion object를 통한 도메인 객체 변환 메서드도 깔끔하게 구현되었습니다.
src/main/kotlin/gomushin/backend/member/facade/MemberInfoFacade.kt (2)
14-14: 적절한 임포트 추가새로 추가된 MyNotificationResponse 클래스를 위한 임포트가 올바르게 추가되었습니다.
59-62: 토글 로직에서 직접 값 설정 방식으로 변경알림 설정 업데이트 로직이 토글 방식에서 직접 값을 설정하는 방식으로 변경되었습니다. 이는 PR의 목적에 부합하는 변경이며, 도메인 엔티티에 새로 추가된
changeDday와changePartnerStatus메서드를 적절히 활용하고 있습니다.
| this.partnerStatus = !this.partnerStatus | ||
| } | ||
|
|
||
| fun changeDday(dday: Boolean) { |
There was a problem hiding this comment.
아예 이렇게 새로 만들지 말고, updateDday랑 updatePartnerStatus 를 수정하는게 낫지 않을까?
물론 그러면 온보딩 쪽에 엮여있는 코드도 수정해야하긴 하는데, 어렵진 않을꺼야
| notificationService.getByMemberId(customUserDetails.getId()).apply { | ||
| if (updateMyNotificationRequest.dday) updateDday() | ||
| if (updateMyNotificationRequest.partnerStatus) updatePartnerStatus() | ||
| changeDday(updateMyNotificationRequest.dday) |
There was a problem hiding this comment.
updateDday랑 updatePartnerStatus 를 수정하면 이 코드는 없어도 될 듯?
…따로 changeDday/PartenerStatus와 같이 추가적인 메서드 안 만들게) #83
| if (isNotification) { | ||
| notification.updateDday() | ||
| notification.updatePartnerStatus() | ||
| notification.updateDday(true) |
There was a problem hiding this comment.
isNotification 이 Boolean 값이고, 알림 받겠다고 했으면 true, 아니면 false 값이니까
true 보다
notification.updateDday(isNotification);
이렇게 가는게 더 이어지는 거 같아
✅ PR 유형
어떤 변경 사항이 있었나요?
📝 작업 내용
#83
Summary by CodeRabbit