fix: 재촉알림을 보낸 사람 기준으로 nudgeNotification이 저장되도록 수정 #671

coli-geonwoo · 2024-10-06T09:01:24Z

🚩 연관 이슈

close #670

📝 작업 내용

fix: nudgeMessage 구성시 재촉당한 사람이 보여지는 문제 해결 #670

🏞️ 스크린샷 (선택)

ex)
재촉한 사람 : 방어진
재촉당한 사람: 김건우

방어진이 재촉해요! 라는 메시지가 떠야 하는데 재촉당한 사람을 기준으로 로그 메시지가 구성됨

🗣️ 리뷰 요구사항 (선택)

github-actions · 2024-10-06T09:02:43Z

Test Results

157 tests 157 ✅ 5s ⏱️
45 suites 0 💤
45 files 0 ❌

Results for commit 58691ce.

♻️ This comment has been updated with latest results.

github-actions · 2024-10-06T09:02:44Z

📝 Test Coverage Report

Overall Project	80.03%	🍏
Files changed	100%	🍏

File	Coverage
NotificationService.java	98.28%	🍏
Notification.java	89.29%	🍏
DirectMessage.java	52.05%	🍏

hyeon0208

역시 안드로이드 대표 주자 콜리
실사용을 바탕으로 버그를 찾아내셨네요 👍
간단한 질문만 남겨봐요 😄

hyeon0208 · 2024-10-06T10:52:15Z

backend/src/main/java/com/ody/notification/service/NotificationService.java

+        Notification nudgeNotification = notificationRepository.save(Notification.createNudge(requestMate));
        fcmPushSender.sendNudgeMessage(
                nudgeNotification,
-                DirectMessage.createMessageToOther(requestMate, nudgeNotification)
+                DirectMessage.createMessageToOther(requestMate, nudgedMate, nudgeNotification)


[질문]
nudgeNotification안에 nudgedMate가 포함되어 있는데 추가로 인자에 담은 이유가 있을까요 ??

nudgeNotification 안에 nudgedMate가 담겨 있어서 찌른 사람이 아니라 찔린 사람이 notification 정보에 담겨 나온 것이 버그의 원인 이었어요.

즉, 리팩터링 이후에는 nudgeNotification에는 nudgedMate가 아닌 requestMate가 담기게 됩니다.

제가 인자를 착각했네요 😅
이미 담겨있는 정보인 requestMate를 넘기지 않고 Notification에서 꺼내서 사용하면 어떨까요 ??

public static DirectMessage createMessageToOther(Mate receiver, Notification senderNotification) {

✅ 현재로서는 nudge 에만 쓰이니 nudge 맥락에 집중해서 메서드를 구성해주어도 좋겠네요. 반영해주었습니다!

hyeon0208 · 2024-10-06T10:53:51Z

backend/src/test/java/com/ody/notification/service/NotificationServiceTest.java

@@ -161,7 +163,7 @@ void unSubscribeTopic() {

    @DisplayName("재촉하기 메시지가 발송된다")
    @Test
-    void sendSendNudgeMessageMessage() {
+    void sendSendNudgeMessage() {


[제안]
원래 메서드 이름이 sendSendNudgeMessageMessage이여서
sendSendNudgeMessage로 바꾸셨군요 👍
한 번더 바꿔서 sendNudgeMessage로 바꿔야할 꺼 같아요 😂

좋은 캐치네요! 꼼꼼하지 못했는데 잘 검토해주어서 감사해요 카키!

ㅋㅋㅋㅋㅋㅋㅋㅋㅋ 이 메서드 왜 이러나요?

hyeon0208 · 2024-10-06T10:57:41Z

backend/src/test/java/com/ody/notification/service/NotificationServiceTest.java

+        Meeting meeting = fixtureGenerator.generateMeeting();
+        Mate sender = fixtureGenerator.generateMate(meeting);
+        Mate receiver = fixtureGenerator.generateMate(meeting);
+        ArgumentCaptor<Notification> captor = ArgumentCaptor.forClass(Notification.class);


오 ArgumentCaptor의 적절한 활용 👍

eun-byeol

실사용으로 QA 하는 콜리 �멋진데?
간단한 리뷰 남겨서 approve 할게요

부대복귀라.. 부대복귀 문제 추천드릴게요

eun-byeol · 2024-10-06T14:04:34Z

backend/src/main/java/com/ody/notification/domain/message/DirectMessage.java

@@ -6,12 +6,12 @@

 public record DirectMessage(Message message) {

-    public static DirectMessage createMessageToOther(Mate sender, Notification recipientNotification) {
+    public static DirectMessage createMessageToOther(Mate sender, Mate receiver, Notification recipientNotification) {


[제안]
인자로 받은 Notification을 사용하는 쪽이 type밖에 없는데, noti 대신 type을 인자로 받으면 어떤가요?

우선 현재 DirectMessage는 그렇지만 이후 로직이 수정될 때, Notification의 다른 정보가 필요할 수도 있다고 생각했어요! 저는 개인적으로 type만 받게 된다면 nudge message 로직에 DirectMessage 생성 로직이 너무 종속되는 것 같은데, 다른 분들 생각은 어떠신가요?

eun-byeol · 2024-10-06T14:22:19Z

backend/src/main/java/com/ody/notification/domain/Notification.java

@@ -79,9 +79,9 @@ public static Notification createDepartureReminder(Mate mate, LocalDateTime send
        );
    }

-    public static Notification createNudge(Mate nudgeMate) {
+    public static Notification createNudge(Mate nudgeRequestMate) {


[제안]
requestMate로 통일하면 어떤가요?

✅ 반영해주었습니다

mzeong · 2024-10-08T00:37:50Z

backend/src/main/java/com/ody/notification/service/NotificationService.java

    public void sendNudgeMessage(Mate requestMate, Mate nudgedMate) {
-        Notification nudgeNotification = notificationRepository.save(Notification.createNudge(nudgedMate));
+        Notification nudgeNotification = notificationRepository.save(Notification.createNudge(requestMate));
        fcmPushSender.sendNudgeMessage(
                nudgeNotification,
-                DirectMessage.createMessageToOther(requestMate, nudgeNotification)
+                DirectMessage.createMessageToOther(nudgedMate, nudgeNotification)
        );
    }


[질문] requestMate = 재촉한 사람, nudgeMate = 재촉당한 사람인가요?
Notification에 저장하는 mate 정보는 알림을 받을 mate여야 하는데 그러면 nudgedMate를 넘겨줘야 하지 않나요?

현재 nudge notification의 로그는 안드로이드에서 00이가 재촉해요로 구성됩니다

여기서 00이는 requestmate여야 합니다.

기존에 nudgedmate를 넣어서 위의 이슈가 발생한 거였어요

데이터베이스 상에서 Notification의 mate가 의미가 달라져서 리뷰를 남겼습니다!

그런데 "재촉한 사람"이 재촉해요 👀라는 로그 메세지를 구성하기 위해 Notification에 재촉한 사람의 mate 정보를 넣었다는 콜리의 설명이 납득이 됐습니다 현재 테이블 구조에서 에러를 픽스하기 위한 최선인 것 같아요~ 😄

현재 PR은 머지를 해서 에러를 해결하고 추후 개선하는 것으로 해요! 이슈 생성해두었습니다 #689

coli-geonwoo added 2 commits October 6, 2024 17:41

refactor: 재촉 요청을 한 사람이 log에 기록되도록 수정

01491e9

test: sender 기준으로 log가 저장되는지 검증

bf69900

coli-geonwoo added BE BUG labels Oct 6, 2024

coli-geonwoo requested review from eun-byeol, mzeong and hyeon0208 October 6, 2024 09:01

coli-geonwoo self-assigned this Oct 6, 2024

coli-geonwoo linked an issue Oct 6, 2024 that may be closed by this pull request

fix: nudgeMessage 구성시 재촉당한 사람이 보여지는 문제 해결 #670

Open

1 task

hyeon0208 requested changes Oct 6, 2024

View reviewed changes

eun-byeol approved these changes Oct 6, 2024

View reviewed changes

coli-geonwoo added 3 commits October 7, 2024 03:08

refactor: 파라미터명 변경

39ed62f

chore: test명 변경

1435ea6

refactor: 불필요한 sender 파라미터 삭제

58691ce

mzeong requested changes Oct 8, 2024

View reviewed changes

coli-geonwoo requested review from mzeong and hyeon0208 October 8, 2024 00:45

mzeong mentioned this pull request Oct 8, 2024

refactor: Notification에 sender mate 정보 저장 #689

Open

mzeong approved these changes Oct 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: 재촉알림을 보낸 사람 기준으로 nudgeNotification이 저장되도록 수정 #671

fix: 재촉알림을 보낸 사람 기준으로 nudgeNotification이 저장되도록 수정 #671

coli-geonwoo commented Oct 6, 2024 •

edited

Loading

github-actions bot commented Oct 6, 2024 •

edited

Loading

github-actions bot commented Oct 6, 2024 •

edited

Loading

hyeon0208 left a comment

hyeon0208 Oct 6, 2024

coli-geonwoo Oct 6, 2024

hyeon0208 Oct 7, 2024

coli-geonwoo Oct 7, 2024

hyeon0208 Oct 6, 2024

coli-geonwoo Oct 6, 2024

mzeong Oct 8, 2024

hyeon0208 Oct 6, 2024

eun-byeol left a comment

eun-byeol Oct 6, 2024

coli-geonwoo Oct 6, 2024

eun-byeol Oct 6, 2024

coli-geonwoo Oct 6, 2024

mzeong Oct 8, 2024

coli-geonwoo Oct 8, 2024 •

edited

Loading

mzeong Oct 8, 2024

fix: 재촉알림을 보낸 사람 기준으로 nudgeNotification이 저장되도록 수정 #671

Are you sure you want to change the base?

fix: 재촉알림을 보낸 사람 기준으로 nudgeNotification이 저장되도록 수정 #671

Conversation

coli-geonwoo commented Oct 6, 2024 • edited Loading

🚩 연관 이슈

📝 작업 내용

🏞️ 스크린샷 (선택)

🗣️ 리뷰 요구사항 (선택)

github-actions bot commented Oct 6, 2024 • edited Loading

Test Results

github-actions bot commented Oct 6, 2024 • edited Loading

📝 Test Coverage Report

hyeon0208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eun-byeol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coli-geonwoo Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coli-geonwoo commented Oct 6, 2024 •

edited

Loading

github-actions bot commented Oct 6, 2024 •

edited

Loading

github-actions bot commented Oct 6, 2024 •

edited

Loading

coli-geonwoo Oct 8, 2024 •

edited

Loading