feat: 애플리케이션 재시작 시 PENDING 상태 알림 스케줄링 및 새벽 4시마다 지난 모임 삭제 기능 추가 #410

hyeon0208 · 2024-08-17T10:27:07Z

🚩 연관 이슈

close #395

📝 작업 내용

시스템 타임 존 설정 추가

시스템의 시간이 로컬과 다른 타임 존일 경우 잘못된 시간에 스케줄링이 우려가 되어 애플리케이션의 타임 존을 설정했습니다.

애플리케이션 재시작 시 PENDING 상태의 출발 알림 스케줄링 예약

출발 알림을 스케줄링하기 위해서는 약속방의 topic 이름이 필요했어요
하지만 구독 시에 사용되는 topic 이름은 로직이 끝나면 휘발되어
topic 이름이 필요할 때마다 Meeting을 가져와 createdAt 필드를 사용해 FcmTopic을 초기화 해줘야하는 부분이 비효율적이라고 생각했어요
그리고 topic 구독 취소를 위해서도 topic 이름을 저장해서 알고 있어야하기에
fcm_topic 컬럼을 추가하게 되었고 해당 컬럼은 제리가 만든 FcmTopic 클래스를 임베디드로 적용해 사용하도록 했습니다.

새벽 4시마다 스케줄링을 통해 Meeting 테이블 논리 삭제

아직은 논리삭제를 적용해 얻을 수 있는 장점은 없지만 추후에
회원별 지각 횟수, 지각 랭킹, 지난 약속 모아보기 같은 기능들이 추가 됐을 때도
약속 날짜가 지난 meeting 데이터를 유용하게 활용할 수 있다고 생각해 논리적 삭제를 진행하게 되었어요
그래서 meeting 테이블에 기간이 지났는지를 판별하는 overdue 필드를 추가하게 되었습니다.

Meeting 이 논리 삭제 될 때 해당 Meeting의 Fcm 토픽 구독 취소 적용

overdue가 true로 update 된 시점이 "새벽 4시마다 Meeting 테이블 논리 삭제" 작업에서 일어난 시점 이후인
meeting 방들의 topic 이름으로 구독 취소를 적용해야했기에
해당 작업이 일어난 시점을 파악하려 BaseEntity에 updatedAt 컬럼을 추가하게 되었습니다.

🏞️ 스크린샷 (선택)

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

작업 내용에서 설명한 이유로 ERD 수정이 있었는데 좋은 의견있으면 알려주세요 ! 🙇🏼‍♂️

github-actions · 2024-08-17T10:28:21Z

Test Results

128 tests +7 120 ✅ +7 4s ⏱️ -1s
40 suites ±0 8 💤 ±0
40 files ±0 0 ❌ ±0

Results for commit 1725cbf. ± Comparison against base commit 22d4168.

This pull request removes 6 and adds 13 tests. Note that renamed tests count towards both.

com.ody.auth.JwtTokenProviderTest$validateAccessToken ‑ [1] accessToken=com.ody.auth.token.AccessToken@bdb5d84
com.ody.auth.JwtTokenProviderTest$validateAccessToken ‑ [2] accessToken=com.ody.auth.token.AccessToken@6576cdd
com.ody.common.validator.FutureOrPresentDateTimeValidatorTest ‑ [1] date=2024-09-06, time=11:03:00.941351166, expected=false
com.ody.common.validator.FutureOrPresentDateTimeValidatorTest ‑ [2] date=2024-09-06, time=12:03:00.941382666, expected=true
com.ody.common.validator.FutureOrPresentDateTimeValidatorTest ‑ [3] date=2024-09-06, time=10:03:00.941370012, expected=false
com.ody.common.validator.FutureOrPresentDateTimeValidatorTest ‑ [4] date=2024-09-07, time=11:03:00.941351166, expected=true

com.ody.auth.JwtTokenProviderTest$validateAccessToken ‑ [1] accessToken=com.ody.auth.token.AccessToken@4ac4644e
com.ody.auth.JwtTokenProviderTest$validateAccessToken ‑ [2] accessToken=com.ody.auth.token.AccessToken@744e29d8
com.ody.common.validator.FutureOrPresentDateTimeValidatorTest ‑ [1] date=2024-09-07, time=17:56:08.949395004, expected=false
com.ody.common.validator.FutureOrPresentDateTimeValidatorTest ‑ [2] date=2024-09-07, time=18:56:08.949426423, expected=true
com.ody.common.validator.FutureOrPresentDateTimeValidatorTest ‑ [3] date=2024-09-07, time=16:56:08.949413859, expected=false
com.ody.common.validator.FutureOrPresentDateTimeValidatorTest ‑ [4] date=2024-09-08, time=17:56:08.949395004, expected=true
com.ody.meeting.repository.MeetingRepositoryTest ‑ 약속 기한이 지나지 않은 모임방을 조회한다.
com.ody.meeting.repository.MeetingRepositoryTest ‑ 약속 날짜가 오늘 이전인 약속방들의 overdue 상태를 true로 변경한다
com.ody.meeting.repository.MeetingRepositoryTest ‑ 오늘 약속의 기한이 지난 약속 리스트들을 조회한다
com.ody.notification.repository.NotificationRepositoryTest ‑ 알림 타입과 알림 상태 조건을 가진 알림 리스트를 조회한다
…

♻️ This comment has been updated with latest results.

github-actions · 2024-08-17T10:28:23Z

📝 Test Coverage Report

Overall Project	77.91% `-1.71%`	❌
Files changed	74.37%	❌

File	Coverage
FcmSendRequest.java	100%	🍏
ClockConfig.java	100%	🍏
Meeting.java	100%	🍏
NotificationService.java	95.52%	🍏
FcmTopic.java	88.89%	🍏
Notification.java	87.78% `-4.44%`	🍏
MeetingService.java	85.14% `-11.43%`	❌
MateService.java	83.07% `-5.29%`	❌
BaseEntity.java	66.67% `-33.33%`	❌
FcmPushSender.java	13.73% `-9.8%`	❌
FcmSubscriber.java	10.77% `-53.85%`	❌
PushMessage.java	0% `-17.39%`	❌

eun-byeol

카카의 코드 공장은 열일중이네요👍
코드 변경사항이 많아서 코드 리뷰하기가 어려웠지만..🥲, pr 메시지 잘 써주셔서 많은 도움이 됐어요.
그래도 다음부터는 이슈를 기능 단위로 나눠서 올려주시면 더 리뷰하기 좋을 것 같습니다!

approve 남기지만, 제가 아직 코드를 완벽히 이해하지 못해서
월요일에 간단히 오디톡 해주시면 좋을 것 같아요!! 기다리고있겠습니다~

애플리케이션 재시작 시 PENDING 상태의 출발 알림 스케줄링 예약

필드 추가는 합리적이라고 생각해요👍

새벽 4시마다 스케줄링을 통해 Meeting 테이블 논리 삭제

논리 삭제를 한 것도 동의합니다!
지금은 지난 meeting 참여를 허용하고 있는데, 추후 mate 생성 시에 overdue가 false인 meeting에 대한 예외처리도 추가되면 좋을 것 같아요🤔

eun-byeol · 2024-08-18T08:19:28Z

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

+    @Test
+    void unSubscribeTopic() {


[제안]

DisplayName도 추가해주세요!

놓친 부부들이 많네요 😅
반영완료 !

eun-byeol · 2024-08-18T08:29:09Z

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

+                    .forEach(notification -> fcmSubscriber.unSubscribeTopic(
+                            notification.getFcmTopic(),
+                            notification.getMate().getMember().getDeviceToken())
+                    );


[제안]

토큰을 바로 가져오는 getter 한 개 더 만드는건 어떤가요?
개행도 맞춰주면 어떤가요?

Suggested change

.forEach(notification -> fcmSubscriber.unSubscribeTopic(

notification.getFcmTopic(),

notification.getMate().getMember().getDeviceToken())

);

.forEach(notification -> fcmSubscriber.unSubscribeTopic(

notification.getFcmTopic(),

notification.getMateMemberDeviceToken()

)

);

디미터 법칙 준수하면서
Mate에서 getMemberDeviceToken() -> Notification에선 getMateDeviceToken()으로 토큰을 가져올 수 있도록 수정했습니다 !

개행도 반영완료 !

eun-byeol · 2024-08-18T08:31:27Z

backend/src/test/java/com/ody/common/BaseControllerTest.java

@@ -19,16 +19,16 @@
 public abstract class BaseControllerTest {

    @MockBean
-    private FcmConfig fcmConfig;
+    protected FcmConfig fcmConfig;


[질문]

FcmConfig, DatabaseCleaner를 상속받아서 사용하는 부분이 없던데, protected로 둔 이유가 있나요?

getter 제거후 접근 제어자를 변경하는 과정에서 일괄적으로 변경했는데
해당 두 클래스는 자식 클래스에서 사용될 여지가 없어보이니 private로 막아버리는게 좋겠네요 👍🏻
반영완료 !

eun-byeol · 2024-08-18T08:34:12Z

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

+    private void saveAndSendDepartureReminderNotification(Meeting meeting, Mate mate, RouteTime routeTime,
+                                                          FcmTopic fcmTopic) {


[제안]

개행이 됐네요! 컨벤션 맞춰주세요~

Suggested change

private void saveAndSendDepartureReminderNotification(Meeting meeting, Mate mate, RouteTime routeTime,

FcmTopic fcmTopic) {

private void saveAndSendDepartureReminderNotification(

Meeting meeting,

Mate mate,

RouteTime routeTime,

FcmTopic fcmTopic

) {

인텔리제이가 자동으로 감지하고 정렬해줬으면 ㅠㅠ 😭
반영완료 !

eun-byeol · 2024-08-18T08:41:38Z

backend/src/main/java/com/ody/common/domain/BaseEntity.java

+    @NotNull
+    @LastModifiedDate
+    private LocalDateTime updatedAt;


[질문]

Notification에도 updatedAt 필드가 생겼네요.
의도한 동작이었을까요?

BaseEntity를 상속받는 상황에서 어쩔수 없는 상황이라 생각했고
불필요한 정보도 아니라고 생각했습니다 😄

[질문]

BaseEntity에서 updateAt이나 createAt은 Eta 필드로도 있는데요. 필드명이 같을 경우에 updateAt 반영이 하위 클래스의 같은 필드명인 updateAt에는 영향을 주지 않는지 궁금해요! 만약 영향을 주게 된다면 isMissing true 처리 같은 로직에 영향을 받을 수 있다고 생각이 되어서요!

Eta는 BaseEntity를 상속받고 있지 않아 영향이 없을 것 같습니다 !
다만 필드명이 동일해서 헷갈리긴 할 것 같네요
혼동을 방지하기 위해서 데이터 생성, 변경 시간 측정을 위해 사용한 BaseEntity의 필드명을 변경하는 것 보다는
로직으로 활용하고 있는 Eta의 필드 이름을 바꿔주는게 좋을 것 같은데 어떻게 생각하시나요 ?

eun-byeol · 2024-08-18T08:54:13Z

backend/src/main/java/com/ody/meeting/repository/MeetingRepository.java

+            and meeting.overdue = false
            """
    )
    List<Meeting> findAllByMemberId(Long memberId);


[제안]

헷갈릴 수 있을 것 같은데, 메서드명을 바꿔주면 어떤가요? NotOverdue 키워드가 들어가면 좋을 것 같아요

Suggested change

and meeting.overdue = false

"""

)

List<Meeting> findAllByMemberId(Long memberId);

and meeting.overdue = false

"""

)

List<Meeting> findAllNotOverdueByMemberId(Long memberId);

헷갈릴 수 있을 것 같다했는데
메서드를 만든 저부터 헷갈려서 Overdue가 아님에도 부정어를 붙이지 않았었네요 😅 반영완료 !

coli-geonwoo

카키! 지나칠 수 있는 부분인데 자발적으로 클렌징 작업 완료해주셨네요 👍
로직 여기저기 제가 잘 모르는 애너테이션이 많은데 다시 오디톡 해주길 기다릴게요!

아마 카카오 로그인이 들어오면 크게 충돌이 발생할 것 같아요.
그 이전에 merge가 되는 편이 좋을 것 같습니다.

고생 많았어요 💪

coli-geonwoo · 2024-08-18T15:57:45Z

backend/src/main/java/com/ody/common/domain/BaseEntity.java

+    @NotNull
+    @LastModifiedDate
+    private LocalDateTime updatedAt;


[질문]

BaseEntity에서 updateAt이나 createAt은 Eta 필드로도 있는데요. 필드명이 같을 경우에 updateAt 반영이 하위 클래스의 같은 필드명인 updateAt에는 영향을 주지 않는지 궁금해요! 만약 영향을 주게 된다면 isMissing true 처리 같은 로직에 영향을 받을 수 있다고 생각이 되어서요!

coli-geonwoo · 2024-08-18T16:03:02Z

backend/src/main/java/com/ody/meeting/repository/MeetingRepository.java

+
+    @Modifying(clearAutomatically = true)
+    @Query("update Meeting m set m.overdue = true where m.overdue = false and m.date < CURRENT_DATE")
+    void updateAllByNotOverdueMeetings();


[의견]

현재 새벽 4시마다 하고 있는 작업은

논리 삭제

논리 삭제 된 meeting의 구독 삭제

이렇게 2가지로 이해했어요!

update 쿼리도 좋지만 더티 체킹을 최대한 활용해보는 것도 좋을 것 같아요. 서비스 로직이 레포지토리에 들어간 느낌을 받을 수 있다고도 생각해서요!

레포지토리 단에서는 삭제대상인 meeting들 만을 가져오고 삭제 + 구독 해지 작업을 애플리 케이션 단에서 해주는 건 어떨까요?

카키의 생각이 궁금합니다!!

오디톡 설명 완 !

coli-geonwoo · 2024-08-18T16:03:54Z

backend/src/main/java/com/ody/meeting/service/MeetingService.java

@@ -86,4 +91,13 @@ public MateSaveResponse saveMateAndSendNotifications(MateSaveRequest mateSaveReq
        Meeting meeting = findByInviteCode(mateSaveRequest.inviteCode());
        return mateService.saveAndSendNotifications(mateSaveRequest, member, meeting);
    }
+
+    @Scheduled(cron = "0 0 4 * * *", zone = "Asia/Seoul")


[질문]

스케쥴드 애너테이션은 처음 활용해보아서 그런데 cron 안에 들어있는 건 4시를 나타낸다고 보아도 좋을까요?

넵 순서대로 다음과 같아요
<초> <분> <시> <일> <월> <요일> <년>

*은 모든 값을 나타내므로, 해석하면

매 년, 매 요일, 매 월, 매 일 , 04:00:00에 스케줄링을 하는 것 입니다 !

coli-geonwoo · 2024-08-18T16:05:09Z

backend/src/main/java/com/ody/notification/dto/request/FcmSendRequest.java

 import com.ody.notification.domain.Notification;

-public record FcmSendRequest(FcmTopic fcmTopic, Notification notification) {
+public record FcmSendRequest(Notification notification) {


notification이 가져도 되는 정보라 생각해요! 좋은 리팩터링이네요 👍

coli-geonwoo · 2024-08-18T16:08:08Z

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

+        try {
+            FirebaseMessaging.getInstance()
+                    .unsubscribeFromTopic(List.of(deviceToken.getDeviceToken()), fcmTopic.getValue());
+            log.info("주제 구독 취소에 성공 했습니다. -- TOKEN = {}, TOPIC = {}", deviceToken.getDeviceToken(), fcmTopic.getValue());


꼼꼼한 로깅 👍

coli-geonwoo · 2024-08-18T16:12:09Z

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

+        log.info("{} 상태 알림 {}에 스케줄링 예약", fcmSendRequest.notification().getStatus(), startTime);
+    }
+
+    @EventListener(ApplicationReadyEvent.class)


요부분 이해가 잘 되지 않아서 어떤 로직인지 오디톡 해주면 좋을 것 같아요!

@EventListener(ApplicationReadyEvent.class)

오디톡 완 !

mzeong · 2024-09-05T07:04:18Z

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

+    @TransactionalEventListener
+    public void unSubscribeTopic(List<Meeting> meetings) {
+        for (Meeting meeting : meetings) {
+            notificationRepository.findAllMeetingIdAndType(meeting.getId(), NotificationType.DEPARTURE_REMINDER)
+                    .forEach(notification -> fcmSubscriber.unSubscribeTopic(
+                                    notification.getFcmTopic(),
+                                    notification.getMateDeviceToken()
+                            )
+                    );
+        }
+    }


[제안] 모든 약속의 구독 취소를 하나의 트랜잭션으로 묶기보다는 각 약속의 구독 취소마다 트랜잭션을 가져가는 게 좋을 것 같아요

mzeong 합니다 😄

@Scheduled(cron = "0 0 4 * * *", zone = "Asia/Seoul") public void scheduleOverdueMeetings() { meetingRepository.updateAllByNotOverdueMeetings(); List<Meeting> meetings = meetingRepository.findAllByUpdatedTodayAndOverdue(); log.info("약속 시간이 지난 약속들 overdue = true로 update 쿼리 실행"); notificationService.unSubscribeTopic(meetings); }

외부 서비스인 FCM 구독 취소를 한 트랜잭션으로 묶으면서 우리 서비스 내의 벌크 쿼리 작업에 영향을 주지 않아도 된다 생각해
하나의 트랜잭션으로 묶이지 않게 수정했습니다 😄
묶이지 않게 했다기 보다는 스케줄링 메서드에 트랜잭션을 없애고 벌크 쿼리 실행 후 commit 되고 다음 조회, 구독 취소 메서드들의 결과에 영향을 안받도록 했어요
그리고 unSubscribeTopic() 로직은 쓰기 작업이 없어 트랜잭션이 필요 없었어요 그래서 읽기 트랜잭션을 사용하도록 변경했습니다.
그리고 각 모임마다 구독 취소시에 에러가 발생하면 어떤 topic이 어떠한 이유로 에러가 발생했는지 알 수 있게 로그가 남도록 했습니다

hyeon0208 · 2024-09-07T17:57:49Z

그 때 이야기 나눴던 부분들 반영해 다시 리뷰 요청 드립니다 ~
얘기 나왔던 부분 중에 까먹어서 반영안한 부분도 있을 수 있을 것 같은데 있다면 알려주세요 !

@coli-geonwoo 🥦
@eun-byeol 🐥
@mzeong 🐀

약속 참여 시간이 지난 약속방 참여 검증

새벽 4시 스케줄링 코드 이벤트 리스너 제거 및 트랜잭션 제거

그리고 추가로 머지 충돌 해결하면서 반영안된 부분들 추가로 수정했습니다.
해당 부분 수정 범위

mzeong · 2024-09-09T05:53:35Z

backend/src/main/java/com/ody/mate/repository/MateRepository.java

+            and mate.meeting.overdue = false
            """)
    Optional<Mate> findFetchedMateById(Long id);


[질문] 지난 약속이 조회되지 않도록 쿼리를 수정한 이유가 있나요? MateService의 findFetchedMate에서 약속 참여자이지만 약속이 지났기 때문에 "존재하지 않는 약속 참여자입니다."라고 에러가 나가게 되는데 적절하지 않은 것 같아요. 애플리케이션 단에서 관리하는 게 낫지 않을까요?

[의견]

저도 제리와 같은 의견이에요. 아마 findFetchedMate가 재촉하기 도메인에서 쓰일 텐데, [진짜 mate가 없는 상황 / 약속이 지나 mate가 존재하지 않는 상황]에서 Optional.empty()가 반환되면 mate가 조회되지 않는 원인을 특정하기 힘들 수 있을 것 같아요!

논리적 삭제도 삭제라 생각해 예외 메세지를 그대로 사용해도 된다고 생각했는데 말씀하신 부분들이 충분히 합리적인 것 같아요 👍
조회된 mate의 meeting의 약속 기한이 지났을 때 "기한이 지난 약속입니다."라는 다른 메세지로 원인을 파악할 수 있도록 반영했습니다 !

mzeong · 2024-09-09T05:57:51Z

backend/src/main/java/com/ody/meeting/repository/MeetingRepository.java

@@ -24,4 +25,6 @@ public interface MeetingRepository extends JpaRepository<Meeting, Long> {

    @Query("select m from Meeting m where m.overdue = true and CAST(m.updatedAt AS LOCALDATE) = CURRENT_DATE")
    List<Meeting> findAllByUpdatedTodayAndOverdue();
+
+    Optional<Meeting> findByIdAndOverdueFalse(Long id);


[제안] 네이밍 findNotOverdueById로 하는 거 어떤가요?

쿼리메소드라서 안 될것같아요🤔

쿼리 네이밍 메소드를 사용하기 위해서 위 처럼 작성했습니다 !
근데 확실히 네이밍 쿼리가 가독성이 그리 좋지 않은 것 같긴해요 😂

mzeong · 2024-09-09T06:06:45Z

backend/src/test/java/com/ody/meeting/repository/MeetingRepositoryTest.java

+    @DisplayName("약속 기한이 지나지 않은 모임방을 조회한다.")
+    @Test
+    void findByIdAndOverdueFalse() {
+        LocalDateTime dateTime = TimeUtil.nowWithTrim();
+        Meeting wotecoMeeting = new Meeting(
+                null,
+                "우테코 등교",
+                dateTime.toLocalDate(),
+                dateTime.toLocalTime(),
+                TARGET_LOCATION,
+                "초대코드",
+                true
+        );
+        Meeting savedWotecoMeeting = meetingRepository.save(wotecoMeeting);
+
+        Meeting odyMeeting = new Meeting(
+                null,
+                "오디 회식",
+                dateTime.toLocalDate(),
+                dateTime.toLocalTime(),
+                TARGET_LOCATION,
+                "초대코드",
+                false
+        );
+        Meeting savedOdyMeeting = meetingRepository.save(odyMeeting);
+
+        assertThat(meetingRepository.findByIdAndOverdueFalse(savedWotecoMeeting.getId())).isEmpty();
+        assertThat(meetingRepository.findByIdAndOverdueFalse(savedOdyMeeting.getId())).isPresent();
+    }


[제안] 어떤 약속인지 드러나면(overdueMeeting, notOverdueMeeting) 코드가 더 눈에 잘 들어올 것 같아요!!

Suggested change

@DisplayName("약속 기한이 지나지 않은 모임방을 조회한다.")

@Test

void findByIdAndOverdueFalse() {

LocalDateTime dateTime = TimeUtil.nowWithTrim();

Meeting wotecoMeeting = new Meeting(

null,

"우테코 등교",

dateTime.toLocalDate(),

dateTime.toLocalTime(),

TARGET_LOCATION,

"초대코드",

true

);

Meeting savedWotecoMeeting = meetingRepository.save(wotecoMeeting);

Meeting odyMeeting = new Meeting(

null,

"오디 회식",

dateTime.toLocalDate(),

dateTime.toLocalTime(),

TARGET_LOCATION,

"초대코드",

false

);

Meeting savedOdyMeeting = meetingRepository.save(odyMeeting);

assertThat(meetingRepository.findByIdAndOverdueFalse(savedWotecoMeeting.getId())).isEmpty();

assertThat(meetingRepository.findByIdAndOverdueFalse(savedOdyMeeting.getId())).isPresent();

}

@DisplayName("약속 기한이 지난/지나지 않은 모임방을 조회한다.")

@Test

void findByIdAndOverdueFalse() {

boolean isOverdue = true;

Meeting overdueMeeting = meetingRepository.save(generateMeeting(isOverdue));

Meeting notOverdueMeeting = meetingRepository.save(generateMeeting(!isOverdue));

assertThat(meetingRepository.findByIdAndOverdueFalse(savedOverdueMeeting.getId())).isEmpty();

assertThat(meetingRepository.findByIdAndOverdueFalse(savedNotOverdueMeeting.getId())).isPresent();

}

private Meeting generateMeeting(boolean overdue) {

LocalDateTime dateTime = TimeUtil.nowWithTrim();

return new Meeting(

null,

"우테코 등교",

dateTime.toLocalDate(),

dateTime.toLocalTime(),

TARGET_LOCATION,

"초대코드",

overdue

);

}

mzeong 합니다 👍
어떤 약속인지 드러나는게 더 명확할 것 같아요
메서드로 빼는 것은 추후에 전체적으로 추상 클래스에서 공통화할 수 있을 것 같아 변수명만 바꿨습니다 !

mzeong · 2024-09-09T06:09:59Z

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

@@ -176,6 +175,6 @@ void sendSendNudgeMessageMessage() {

        notificationService.sendNudgeMessage(requestMate, nudgedMate);

-        Mockito.verify(getFcmPushSender(), times(1)).sendNudgeMessage(any(), any());
+        Mockito.verify(fcmPushSender, Mockito.times(1)).sendNudgeMessage(any(), any());


[제안] static 임포트 추가해주고 Mockito는 제거해도 괜찮을 것 같네요 😃

Suggested change

Mockito.verify(fcmPushSender, Mockito.times(1)).sendNudgeMessage(any(), any());

Mockito.verify(fcmPushSender, times(1)).sendNudgeMessage(any(), any());

Static import를 가급적 사용하지 않기로 얘기했던 것 같아서 일부로 추가하긴 했어요
전체적으로 Static 메서드를 사용할 때 클래스를 명시되어 있어서 맞추고자 일부로 추가했습니다 !

eun-byeol

간단한 리뷰만 남겨서, approve할게요!
고생했어 카키~~~~

eun-byeol · 2024-09-09T09:02:12Z

backend/src/main/java/com/ody/mate/service/MateService.java

+            throw new OdyBadRequestException("약속방에 이미 참여한 회원입니다.");
+        }
+        if (meeting.isOverdue()) {
+            throw new OdyBadRequestException("참여 가능한 시간이 지난 약속방에 참여할 수 없습니다.");


[제안]
약속으로 표현 통일해주면 더 좋을 것 같아요!

Suggested change

throw new OdyBadRequestException("약속방에 이미 참여한 회원입니다.");

}

if (meeting.isOverdue()) {

throw new OdyBadRequestException("참여 가능한 시간이 지난 약속방에 참여할 수 없습니다.");

throw new OdyBadRequestException("약속에 이미 참여한 회원입니다.");

}

if (meeting.isOverdue()) {

throw new OdyBadRequestException("참여 가능한 시간이 지난 약속에 참여할 수 없습니다.");

좋은 것 같아요 👍
"약속방"이라는 단어를 사용하는 몇가지 파일들이 존재해 일괄로 "약속"이란 단어를 사용하도록 변경했습니다 !

eun-byeol · 2024-09-09T09:03:40Z

backend/src/main/java/com/ody/meeting/repository/MeetingRepository.java

@@ -24,4 +25,6 @@ public interface MeetingRepository extends JpaRepository<Meeting, Long> {

    @Query("select m from Meeting m where m.overdue = true and CAST(m.updatedAt AS LOCALDATE) = CURRENT_DATE")
    List<Meeting> findAllByUpdatedTodayAndOverdue();
+
+    Optional<Meeting> findByIdAndOverdueFalse(Long id);


쿼리메소드라서 안 될것같아요🤔

coli-geonwoo

오우.. 카키 충돌이 많이 났을 텐데 잘 해결하신 것 같네요!

몇가지 리뷰는 취사 선택해주세요! 고생많으셨습니다 🙇

coli-geonwoo · 2024-09-09T10:56:45Z

backend/src/main/java/com/ody/mate/repository/MateRepository.java

+            and mate.meeting.overdue = false
            """)
    Optional<Mate> findFetchedMateById(Long id);


[의견]

저도 제리와 같은 의견이에요. 아마 findFetchedMate가 재촉하기 도메인에서 쓰일 텐데, [진짜 mate가 없는 상황 / 약속이 지나 mate가 존재하지 않는 상황]에서 Optional.empty()가 반환되면 mate가 조회되지 않는 원인을 특정하기 힘들 수 있을 것 같아요!

coli-geonwoo · 2024-09-09T11:00:17Z

backend/src/main/java/com/ody/mate/repository/MateRepository.java

            """)
    Optional<Mate> findFetchedMateById(Long id);

    @Query("""
            select mate
            from Mate mate
            join fetch mate.member
-            where mate.meeting.id = :meetingId
+            where mate.meeting.id = :meetingId 
+            and mate.meeting.overdue = false
            """)
    List<Mate> findAllByMeetingId(Long meetingId);


[제안]

유효한 meetingId로부터 mate들을 가져온다는 걸 메서드 네이밍에 담는 건 어떨까요?

만약 어떤 repository 메서드는 overdue=false가 조건으로 걸려있고 또 다른 메서드에는 걸려 있지 않다면 직관적으로 로직을 파악하기 힘들 것 같다는 생각이 들었어요! 팀원 간에 컨벤션으로기본 쿼리 메서드 명에는 overdue =false를 일관적으로 거는 것을 디폴트로 생각하자! 라고 합의한다면 현재 메서드 명도 괜찮을 것 같아요 😄

Suggested change

List<Mate> findAllByMeetingId(Long meetingId);

List<Mate> findAllByNotOverdueMeetingId(Long meetingId);

동의합니다 !
아무래도 컨벤션으로 정하더라도 논리삭제를 적용한 엔티티 안한 엔티티들로 인해 헷갈릴 여지가 많을 수 있을 것 같아
메서드 명으로 명시하는게 좋을 것 같아요 ! 👍

다른 메서드의 경우에는 쿼리 네이밍 메서드를 사용해 OverdueFalse라고 되어 있어 이를 맞추고자
findAllByOverdueFalseMeetingId로 네이밍 적용했습니다 !

coli-geonwoo · 2024-09-09T11:08:21Z

backend/src/test/java/com/ody/meeting/repository/MeetingRepositoryTest.java

+        );
+        Meeting savedOdyMeeting = meetingRepository.save(odyMeeting);
+
+        assertThat(meetingRepository.findByIdAndOverdueFalse(savedWotecoMeeting.getId())).isEmpty();


[제안]

검증문이 2줄이라 assertAll로 묶어주어도 좋을 것 같습니다!

반영완료 !

…서 처리

hyeon0208 added 7 commits August 17, 2024 17:36

feat: 시스템 타임 존 설정 추가

358528e

feat: 알림 테이블에 fcmTopic 컬럼 추가

2ae437b

feat: 애플리케이션 시작 시 PENDING 상태 알림 스케줄링 적용

2b7a5a5

feat: 지난 약속방 논리 삭제 스케줄링 추가

0ecba42

test: 접근제어자 protected로 변경 및 getter 제거

7329707

feat: 오늘 약속의 기한이 지난 약속 리스트 조회 기능 추가 및 이벤트 발행 기능 추가

d1f6e0d

feat: fcm topic 구독 해제 기능 추가

e2bae23

hyeon0208 added BE FEATURE labels Aug 17, 2024

hyeon0208 requested review from eun-byeol, mzeong and coli-geonwoo August 17, 2024 10:27

hyeon0208 self-assigned this Aug 17, 2024

eun-byeol approved these changes Aug 18, 2024

View reviewed changes

hyeon0208 added 5 commits August 19, 2024 00:39

test: 테스트 설명 추가

604ec35

refactor: Device Token getter 디미터 법칙 적용

bc45cff

test: Base 추상 클래스 접근 제어자 수정

3f4498b

refactor: 개행 적용

c76171b

style: 벌크 쿼리 메서드명 수정

e779dc0

hyeon0208 mentioned this pull request Aug 20, 2024

refactor: 재촉하기 API에 재촉한 사람 정보가 포함되도록 수정 #437

Merged

1 task

coli-geonwoo approved these changes Aug 20, 2024

View reviewed changes

mzeong reviewed Sep 5, 2024

View reviewed changes

hyeon0208 mentioned this pull request Sep 6, 2024

refactor: 지난 약속에 참여하지 못하도록 예외처리 #474

Merged

1 task

hyeon0208 added 2 commits September 8, 2024 00:57

feat: 약속 참여 시간이 지난 약속방 참여 검증 추가

69d3195

refactor: 새벽 4시 스케줄링 코드 이벤트 리스너 제거 및 트랜잭션 제거

185fae5

hyeon0208 requested review from mzeong, coli-geonwoo and eun-byeol September 7, 2024 16:54

hyeon0208 and others added 4 commits September 8, 2024 02:18

Merge branch 'develop' into feature/395

8ffcad9

refactor: 머지 충돌 작업 해결

70b61b0

refactor: 조회 메서드에 약속 기간이 지나지 않은 조건 추가

51ddc35

feat: 기간이 지나지 않은 약속 단건 조회 메서드 추가

1725cbf

mzeong approved these changes Sep 9, 2024

View reviewed changes

eun-byeol approved these changes Sep 9, 2024

View reviewed changes

coli-geonwoo approved these changes Sep 9, 2024

View reviewed changes

hyeon0208 added 5 commits September 10, 2024 00:05

refactor: findFetchedMateById() 메서드 사용 시 약속 기한이 지난 약속 처리 로직을 service에…

15d7d0d

…서 처리

test: 기한이 지난 약속 조회 테스트 어떤 약속인지 명확하게 변수명 네이밍

72f86c5

style: 약속방 -> 약속으로 텍스트 변경

9ae47eb

style: 기간이 지나지 않은 약속방 전체 Mate 조회 메서드 네이밍 수정

eed3d75

test: 2가지 검증 구문을 assertAll로 래핑

69fb48a

hyeon0208 merged commit 0b3c379 into develop Sep 9, 2024
1 check passed

hyeon0208 deleted the feature/395 branch September 9, 2024 15:26

		private void saveAndSendDepartureReminderNotification(Meeting meeting, Mate mate, RouteTime routeTime,
		FcmTopic fcmTopic) {

	Mockito.verify(fcmPushSender, Mockito.times(1)).sendNudgeMessage(any(), any());
	Mockito.verify(fcmPushSender, times(1)).sendNudgeMessage(any(), any());

	List<Mate> findAllByMeetingId(Long meetingId);
	List<Mate> findAllByNotOverdueMeetingId(Long meetingId);

feat: 애플리케이션 재시작 시 PENDING 상태 알림 스케줄링 및 새벽 4시마다 지난 모임 삭제 기능 추가 #410

feat: 애플리케이션 재시작 시 PENDING 상태 알림 스케줄링 및 새벽 4시마다 지난 모임 삭제 기능 추가 #410

Conversation

hyeon0208 commented Aug 17, 2024 • edited Loading

🚩 연관 이슈

📝 작업 내용

🏞️ 스크린샷 (선택)

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

github-actions bot commented Aug 17, 2024 • edited Loading

Test Results

github-actions bot commented Aug 17, 2024 • edited Loading

📝 Test Coverage Report

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

hyeon0208 Aug 18, 2024 • edited Loading

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

coli-geonwoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coli-geonwoo Aug 18, 2024 • edited Loading

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

Choose a reason for hiding this comment

hyeon0208 commented Sep 7, 2024

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

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

coli-geonwoo 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

hyeon0208 commented Aug 17, 2024 •

edited

Loading

github-actions bot commented Aug 17, 2024 •

edited

Loading

github-actions bot commented Aug 17, 2024 •

edited

Loading

hyeon0208 Aug 18, 2024 •

edited

Loading

coli-geonwoo Aug 18, 2024 •

edited

Loading