[알림] 알림 연관 기능 구현(#66) #74
Merged
Conversation
This file contains 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
알림 종류는 다음과 같다. - 화살 수신 - 채팅 요청 수신 - 요청한 채팅 수락됨
페이지 형태로 알림 목록 제공, 페이지 사이즈는 고정 9
query string 통해 페이지 번호를 받아 받은 알림 목록 제공
- 알림 타입의 경우 외부에서 요청하여 생성하는 것이 아닌 코드에서만 설정 - 열거형 타입으로 정의하여 코드에서 안정적으로 다룰 수 있도록 구성 - permissionStatus의 경우 @Getter 사용시 is... 네이밍으로 형성되어 의미를 명확히 표현하지 못한다고 생각해 별도의 getter 구성
사용자의 알림 동의 내역중 주어진 알림 종류에 해당하는 내역 조회
- 요청한 채팅 수락 시 알림 동의 여부 필드 추가 - not null 제약 조건 추가
더티 체킹 활용, 사용자의 각 종류별 알림 동의 내역의 동의 상태 업데이트
알림 제목, 내용외 부가적인 데이터로 다음과 같은 것들이 포함된다. - 알림 생성 사용자 ID(화살 보낸 사용자, 채팅 요청한 사용자 등) - 알림 생성 일시
- 모든 필드가 불변이기에 레코드 활용 - 알림 제목, 내용을 제공하는 편의 메서드 구성
알림 동의 내역 리스트를 바탕으로 알림 타입별 동의 내역 Map을 형성하여 응답하도록 코드를 간결하게 수정
- 기존 로직은 주어진 ID보다 작거나 같은 ID를 가진 알림을 전부 읽음 처리 - 모든 사용자가 받은 알림이 읽음 처리되는 잘못된 로직 - 특정 사용자가 받은 알림만 ID를 기준으로 알림 처리할 수 있도록 로직 수정 - 메서드명 변경
- 페이지 번호가 optional해짐에 따라 요청 DTO 검증 로직 추가 - 읽음 처리 메서드명 변경에 따른 사용 위치 수정
Comment on lines
103
to
104
| public ResponseEntity<CustomResponse<NotificationPermissionsResponse>> | ||
| getNotificationPermissions(@RequestAttribute("userId") long userId) { |
There was a problem hiding this comment.
Suggested change
| public ResponseEntity<CustomResponse<NotificationPermissionsResponse>> | |
| getNotificationPermissions(@RequestAttribute("userId") long userId) { | |
| public ResponseEntity<CustomResponse<NotificationPermissionsResponse>>getNotificationPermissions( | |
| @RequestAttribute("userId") long userId) { |
Comment on lines
106
to
107
| NotificationPermissionsResponse response = | ||
| myPageService.getNotificationPermissions(userId); |
There was a problem hiding this comment.
Suggested change
| NotificationPermissionsResponse response = | |
| myPageService.getNotificationPermissions(userId); | |
| NotificationPermissionsResponse response = myPageService.getNotificationPermissions(userId); |
Comment on lines
41
to
42
| map.put(NotificationType.CHATTING_REQUEST_ACCEPTED, | ||
| this.allowChattingRequestAcceptedNotification); |
There was a problem hiding this comment.
Suggested change
| map.put(NotificationType.CHATTING_REQUEST_ACCEPTED, | |
| this.allowChattingRequestAcceptedNotification); | |
| map.put(NotificationType.CHATTING_REQUEST_ACCEPTED, this.allowChattingRequestAcceptedNotification); |
Comment on lines
36
to
38
| public Map<NotificationType, Boolean> toNotificationTypePermissionStatusMap() { | ||
|
|
||
| Map<NotificationType, Boolean> map = new HashMap<>(); |
There was a problem hiding this comment.
- Map을 사용하신 이유는 무엇인가요?
- 변수명에 특정 자료구조를 사용하는 것은 지양해주세요.
There was a problem hiding this comment.
- 각 알림 타입에 해당하는 알림 동의 여부를
Map으로 다루는 것이 로직을 간결하게 구성할 수 있다고
판단하였습니다. - 메서드명, 변수명 수정하겠습니다.
Map타입의 변수 네이밍을 어떻게 해야할 지 감이 안 잡히는데 팁을 좀 주실 수
있을까요?
There was a problem hiding this comment.
솔직히 이 기능을 위해서 사용된 로직이 간결해 보이지가 않습니다.(처음 만든 Map의 key, value가 각각 무엇을 의미하는지, DB에 저장할 엔티티는 언제 만들어지는 등 코드 읽기가 쉽지 않습니다.)
Map을 로직에 사용하게 되면 간결하지 않은 경우가 많아지는 것같습니다.(제 경험상으로는 그렇습니다.)
그래서 저는 해당 API request를 바꾸는 것이 더 좋아보입니다.
예를 들면 [{NotificationType, Status}] 형태로 받으면 단순 List로 훨씬 간결하게 코드를 작성할 수 있을 것같은데, 이렇게 하기에는 무리가 있을까요?
- 저는 Map이든 List든 크게 신경안쓰고 그 변수에 담겨지는 데이터 자체가 의미하는 게 무엇인지를 생각하고 최대한 그대로 표현하는 것이 아직까지 제 기준에서는 나쁘지는 않은 것같습니다.
There was a problem hiding this comment.
알림 유형,동의 여부 필드를 가지는 DTO를 별도로 정의하여 로직을 재구성하였습니다. DTO 이름, 요청 값과 열거형 매핑, 알림 동의 내역 업데이트와 관련하여 더 좋은 방향이 있으면 의견 주시면 감사하겠습니다.
참고로, 요청 DTO의 List 타입 필드에 null값이 포함되어 있는 지 확인하는 검증은
List<@NotNull NotificationPermissionDetail> permissions;
위와 같은 형태로도 수행이 가능하지만, 예외 메시지를 추가하면 다소 가독성이 떨어진다고 생각하여 서비스 계층에서
검증하고 커스텀 예외를 발생시키도록 로직을 구성했습니다.
- 알림 내역 상세 DTO를 이용도록 변경 - 알림 내역에 초점을 두도록 클래스 이름 변경
- 알림 내역 상세 DTO를 이용하도록 변경 - 알림에 초점을 두도록 클래스 이름 변경
새로 구성한 알림 동의 내역 상세 DTO를 이용하여 코드를 보다 간결하게 변경
- 요청 동의 내역 리스트, null 값 포함 여부 검증 추가 - 요청 DTO, 알림 유형 필드 타입 변경에 따른 업데이트 로직 수정
- 기존 정규식 검증으로도 공백 값 검증 가능 - 공백 값 요청될 시 좀 더 상세한 예외 상황 표현을 위해 @notblank 어노테이션으로 변경
acceptor-gyu
approved these changes
May 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
작업 대상
다음 기능들 구현
📄 작업 내용
🙋🏻 주의 사항
최근 받은 알림 목록 조회 API가
@PatchMapping인 이유알림 읽음 처리 로직 설명
badge = 최근 읽지 않은 알림 개수
알림 핸들링을 SpringEvent를 활용하여 구현한 이유?
알림 이벤트 로직의 구성
알림 생성 일시를 포함하도록 구성
record활용현재 상황에선 불필요하게 복잡도를 높인다고 판단
FCM 서버에 푸시 알림을 요청하는 로직을 비동기 처리한 이유?
📎 관련 이슈
JSON 파일을 secrets으로 관리하는 방법
제대로 인식되지 않음
형식으로 문제 해결
ETC
레퍼런스