[PC-416] 유저 설정 기능 구현 #50
Conversation
| @Column(name = "user_id") | ||
| private Long userId; | ||
|
|
||
| @Column(name = "match_notification") |
There was a problem hiding this comment.
matcNotification과 notification으로 구분하신 이유가 있을까요 ?
네이밍상으로 봤을 때, 매치알림과 알림으로 구분한 것인데 알림은 매치알림을 포괄하는 개념이기 때문에
매치알림과 푸시알림으로 구분하는 것이 적절할 것 같습니다.
notification 대신 pushNotification은 어떠신가요
There was a problem hiding this comment.
기능정의서와 알림 기능과 매치 알림 기능의 설정이 따로 있기 때문에 구분했습니다.
알림은 매치알림을 포괄하는 개념이 맞고, 그 점 고려해서 설계한 겁니다.
알림은 푸시알림밖에 없기 때문에 notification으로 충분히 전달이 된다고 생각했는데 헷갈리시면 바꾸겠습니다.
하지만 충분히 전달이 되는데 굳이 바꿔야하는지는 의문이긴 합니다.
지금까지 만들어온 모든 푸시알람 기능들이 push notification이 아니라 notification으로 되어있기도 하구요.
There was a problem hiding this comment.
답변을 보고, 알림을 보낼 떄 상위 개념인 notification 에 대한 설정이 우선 적용되고, 그 다음 하위 개념인 matchNotification 설정이 되는 것으로 이해했으며 아래와 같이 케이스를 분류할 수 있을 것 같습니다.
case 1) notification=true, matchNotification=true
- 서비스의 모든 알림을 보냄
case 2) notification=true, matchNotification=false
- 매치 알림을 제외한 알림을 보냄
case 3) notification=false, matchNotification=true
- 서비스의 모든 알림을 보내지 않음
case 4) notification=false, matchNotification=false
- 서비스의 모든 알림을 보내지 않음
이렇게 동작하는 하는 것이 맞다면 pushNotification으로 변경하지 않아도 될 것 같습니다.
다만, case3과 case4에서 보시는바와 같이 notification=false 인 경우 matchNotification=true는 의미가 없으므로
notification이 false로 업데이트 될 떄, matchNotification도 false로 변경되는 로직이 updateNotification 메서드에 있으면 좋을 것 같습니다 !
.
There was a problem hiding this comment.
기능상 알람기능이 꺼졌을 때 매칭 알람도 안가는 것은 맞지만, 알람기능을 끌 때 매칭알람을 끌 필요는 없다고 생각합니다.
어차피 알람 설정이 꺼져있으니 필요없는 일람을 보낼일이 없습니다.
오히려 알람을 껐을 때 이전 매칭알림 관련 설정을 잃어버리는 결과가 됩니다.
바꿨을 때 사용자 입장에서 도움될 일이 없고 오히려 불편하게 느낄여지가 있으며, 서버에서도 불필요한 로직이 추가되는 격이니 좋지 않다고 생각합니다.
혹시 해당 로직이 필요하다고 생각하신 이유를 알 수 있을까요?
There was a problem hiding this comment.
말쑴허신대로, 이전에 설정된 알림 설정값을 모두 잃게 되면 사용자가 불편을 느낄 수 있을 것 같숩니다.
하지만, 피그마 UI상으로 두 개의 토글 버튼이 같은 계층에 속하므로 푸시 알림이 매치 알림의 상위 개념으로 받아들이기 어려울 수 있습니다.
이때, 푸시알림을 false로 업데이트 하는 시점에 하위 모든 알림이 false로 설정된다면 유저는 푸쉬 알림이 모든 알림의 상위 개념이라는 것을 즉각적으로 인지할 수 있기 때문에 제안드렸습니다.
제가 생각한 문제는 UI적으로도 해결할 수 있는 문제이고, 찬호님이 지적하신 문제는 서버에서만 해결할 수 있는 문제인 것 같습니다. 그래서 찬호님 방식이 좋을 것 같습니다. 다만, 제가 지적한 문제는 PM 님과 논의될만한 사항같습니다. 알림 관련 기능은 찬호님이 개발중이시니 자체 판단하에 PM 님과 논의 여부를 결정하시면 될 것 같습니다 !
| public class SettingInfoResponse { | ||
|
|
||
| private Boolean isNotificationEnabled; | ||
| private Boolean isMatchingNotificationEnabled; |
There was a problem hiding this comment.
Settomg엔티티에서는 matchNotification 으로 되어있는데 Res[pmse dto에서는 Matching으로 되어있습니다 .
통일되면 좋을 것 같습니다 !
| } | ||
|
|
||
| @PutMapping("/notification/{switch}") | ||
| public ResponseEntity<CommonResponse<Void>> updateMatchNotificationStatus( |
There was a problem hiding this comment.
Restful API PathVariable은 자원을 식별하기 위한 id 값을 사용하는 것이 권장됩니다.
하지만 개발하신 API에서는 자원의 식별값이 아닌 자원의 상태값을 변경하기 위한 데이터로 사용되고 있는데, 자원의 상태값을 변경하기 위한 데이터는 Body를 통해 전달받는 것이 좋을 것 같습니다 .
아래의 글을 읽어보시면 도움 될 것 같습니다 !
https://ryan-han.com/post/translated/pathvariable_queryparam
There was a problem hiding this comment.
추가로 Setting 엔티티의 필드를 변경하는 메서드의 파라미터에는 status라는 boolean 변수를 사용하지만, 컨트롤러단에서는 switch 또는 switchValue를 사용하고 있습니다. 엔티티 메서드의 파라미터명과 컨트롤러에서의 변수명이 일치되었으면 좋겠습니다. 또한 switch라는 것은 자바의 예약어이기 때문에 권장되지 않는 변수명인 것 같습니다.
There was a problem hiding this comment.
notification/{switch}에서 "on"과 "off"가 설정값을 나타내는 것이므로, 이것이 곧 리소스의 상태를 나타낸다고 볼 수 있다고 생각해서 API를 짰는데, 클라이언트 측에서 혼란스러울 수도 있겠네요. body로 받도록 수정하겠습니다.
| @Column(name = "user_id") | ||
| private Long userId; | ||
|
|
||
| @Column(name = "match_notification") |
There was a problem hiding this comment.
답변을 보고, 알림을 보낼 떄 상위 개념인 notification 에 대한 설정이 우선 적용되고, 그 다음 하위 개념인 matchNotification 설정이 되는 것으로 이해했으며 아래와 같이 케이스를 분류할 수 있을 것 같습니다.
case 1) notification=true, matchNotification=true
- 서비스의 모든 알림을 보냄
case 2) notification=true, matchNotification=false
- 매치 알림을 제외한 알림을 보냄
case 3) notification=false, matchNotification=true
- 서비스의 모든 알림을 보내지 않음
case 4) notification=false, matchNotification=false
- 서비스의 모든 알림을 보내지 않음
이렇게 동작하는 하는 것이 맞다면 pushNotification으로 변경하지 않아도 될 것 같습니다.
다만, case3과 case4에서 보시는바와 같이 notification=false 인 경우 matchNotification=true는 의미가 없으므로
notification이 false로 업데이트 될 떄, matchNotification도 false로 변경되는 로직이 updateNotification 메서드에 있으면 좋을 것 같습니다 !
.
|
수정 완료했습니다! |
🔗 관련 이슈
PC-416
✨ 작업 내용
✅ 체크리스트
🎃 새롭게 알게된 사항
📋 참고 사항