-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: 애플리케이션 재시작 시 PENDING 상태 알림 스케줄링 및 새벽 4시마다 지난 모임 삭제 기능 추가 #410
Changes from 18 commits
358528e
2ae437b
2b7a5a5
0ecba42
7329707
d1f6e0d
e2bae23
604ec35
bc45cff
3f4498b
c76171b
e779dc0
69d3195
185fae5
8ffcad9
70b61b0
51ddc35
1725cbf
15d7d0d
72f86c5
9ae47eb
eed3d75
69fb48a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
package com.ody.common.config; | ||
|
||
import java.time.Clock; | ||
import java.time.ZoneId; | ||
import lombok.extern.slf4j.Slf4j; | ||
import org.springframework.context.annotation.Bean; | ||
import org.springframework.context.annotation.Configuration; | ||
|
||
@Slf4j | ||
@Configuration | ||
public class ClockConfig { | ||
|
||
@Bean | ||
public Clock clock() { | ||
String zoneId = "Asia/Seoul"; | ||
log.info("시스템 타임 존 설정 : {}", zoneId); | ||
return Clock.system(ZoneId.of(zoneId)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -13,15 +13,17 @@ public interface MateRepository extends JpaRepository<Mate, Long> { | |||||
from Mate mate | ||||||
join fetch mate.meeting | ||||||
join fetch mate.member | ||||||
where mate.id = :id | ||||||
where mate.id = :id | ||||||
and mate.meeting.overdue = false | ||||||
""") | ||||||
Optional<Mate> findFetchedMateById(Long id); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [질문] 지난 약속이 조회되지 않도록 쿼리를 수정한 이유가 있나요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [의견] 저도 제리와 같은 의견이에요. 아마 findFetchedMate가 재촉하기 도메인에서 쓰일 텐데, [진짜 mate가 없는 상황 / 약속이 지나 mate가 존재하지 않는 상황]에서 Optional.empty()가 반환되면 mate가 조회되지 않는 원인을 특정하기 힘들 수 있을 것 같아요! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 논리적 삭제도 삭제라 생각해 예외 메세지를 그대로 사용해도 된다고 생각했는데 말씀하신 부분들이 충분히 합리적인 것 같아요 👍 |
||||||
|
||||||
@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); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [제안] 유효한 meetingId로부터 mate들을 가져온다는 걸 메서드 네이밍에 담는 건 어떨까요? 만약 어떤 repository 메서드는 overdue=false가 조건으로 걸려있고 또 다른 메서드에는 걸려 있지 않다면 직관적으로 로직을 파악하기 힘들 것 같다는 생각이 들었어요! 팀원 간에 컨벤션으로기본 쿼리 메서드 명에는 overdue =false를 일관적으로 거는 것을 디폴트로 생각하자! 라고 합의한다면 현재 메서드 명도 괜찮을 것 같아요 😄
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 동의합니다 ! 다른 메서드의 경우에는 쿼리 네이밍 메서드를 사용해 OverdueFalse라고 되어 있어 이를 맞추고자 |
||||||
|
||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -34,15 +34,16 @@ public class MateService { | |||||||||||||||||
private final NotificationService notificationService; | ||||||||||||||||||
private final RouteService routeService; | ||||||||||||||||||
|
||||||||||||||||||
|
||||||||||||||||||
@Transactional | ||||||||||||||||||
public MateSaveResponseV2 saveAndSendNotifications( | ||||||||||||||||||
MateSaveRequestV2 mateSaveRequest, | ||||||||||||||||||
Member member, | ||||||||||||||||||
Meeting meeting | ||||||||||||||||||
) { | ||||||||||||||||||
public MateSaveResponseV2 saveAndSendNotifications(MateSaveRequestV2 mateSaveRequest, Member member, Meeting meeting) { | ||||||||||||||||||
if (mateRepository.existsByMeetingIdAndMemberId(meeting.getId(), member.getId())) { | ||||||||||||||||||
throw new OdyBadRequestException("약속에 이미 참여한 회원입니다."); | ||||||||||||||||||
throw new OdyBadRequestException("약속방에 이미 참여한 회원입니다."); | ||||||||||||||||||
} | ||||||||||||||||||
if (meeting.isOverdue()) { | ||||||||||||||||||
throw new OdyBadRequestException("참여 가능한 시간이 지난 약속방에 참여할 수 없습니다."); | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [제안]
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 좋은 것 같아요 👍 |
||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
Mate mate = saveMateAndEta(mateSaveRequest, member, meeting); | ||||||||||||||||||
notificationService.saveAndSendNotifications(meeting, mate, member.getDeviceToken()); | ||||||||||||||||||
return MateSaveResponseV2.from(meeting); | ||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -2,7 +2,9 @@ | |||||||||||||||||
|
||||||||||||||||||
import com.ody.meeting.domain.Meeting; | ||||||||||||||||||
import java.util.List; | ||||||||||||||||||
import java.util.Optional; | ||||||||||||||||||
import org.springframework.data.jpa.repository.JpaRepository; | ||||||||||||||||||
import org.springframework.data.jpa.repository.Modifying; | ||||||||||||||||||
import org.springframework.data.jpa.repository.Query; | ||||||||||||||||||
|
||||||||||||||||||
public interface MeetingRepository extends JpaRepository<Meeting, Long> { | ||||||||||||||||||
|
@@ -12,7 +14,17 @@ public interface MeetingRepository extends JpaRepository<Meeting, Long> { | |||||||||||||||||
from Meeting meeting | ||||||||||||||||||
right join Mate mate on meeting.id = mate.meeting.id | ||||||||||||||||||
where mate.member.id = :memberId | ||||||||||||||||||
and meeting.overdue = false | ||||||||||||||||||
""" | ||||||||||||||||||
) | ||||||||||||||||||
List<Meeting> findAllByMemberId(Long memberId); | ||||||||||||||||||
Comment on lines
+17
to
20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [제안] 헷갈릴 수 있을 것 같은데, 메서드명을 바꿔주면 어떤가요? NotOverdue 키워드가 들어가면 좋을 것 같아요
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 헷갈릴 수 있을 것 같다했는데 |
||||||||||||||||||
|
||||||||||||||||||
@Modifying(clearAutomatically = true) | ||||||||||||||||||
@Query("update Meeting m set m.overdue = true where m.overdue = false and m.date < CURRENT_DATE") | ||||||||||||||||||
void updateAllByNotOverdueMeetings(); | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [의견] 현재 새벽 4시마다 하고 있는 작업은
이렇게 2가지로 이해했어요! update 쿼리도 좋지만 더티 체킹을 최대한 활용해보는 것도 좋을 것 같아요. 서비스 로직이 레포지토리에 들어간 느낌을 받을 수 있다고도 생각해서요! 레포지토리 단에서는 삭제대상인 meeting들 만을 가져오고 삭제 + 구독 해지 작업을 애플리 케이션 단에서 해주는 건 어떨까요? 카키의 생각이 궁금합니다!! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 오디톡 설명 완 ! |
||||||||||||||||||
|
||||||||||||||||||
@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); | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [제안] 네이밍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 쿼리메소드라서 안 될것같아요🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 쿼리 네이밍 메소드를 사용하기 위해서 위 처럼 작성했습니다 ! |
||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,16 +15,20 @@ | |
import com.ody.meeting.dto.response.MeetingWithMatesResponse; | ||
import com.ody.meeting.repository.MeetingRepository; | ||
import com.ody.member.domain.Member; | ||
import com.ody.notification.service.NotificationService; | ||
import com.ody.util.InviteCodeGenerator; | ||
import com.ody.util.TimeUtil; | ||
import java.time.LocalDateTime; | ||
import java.util.Comparator; | ||
import java.util.List; | ||
import java.util.stream.Collectors; | ||
import lombok.RequiredArgsConstructor; | ||
import lombok.extern.slf4j.Slf4j; | ||
import org.springframework.scheduling.annotation.Scheduled; | ||
import org.springframework.stereotype.Service; | ||
import org.springframework.transaction.annotation.Transactional; | ||
|
||
@Slf4j | ||
@Service | ||
@RequiredArgsConstructor | ||
@Transactional(readOnly = true) | ||
|
@@ -35,6 +39,7 @@ public class MeetingService { | |
private final MateService mateService; | ||
private final MeetingRepository meetingRepository; | ||
private final MateRepository mateRepository; | ||
private final NotificationService notificationService; | ||
|
||
@Transactional | ||
public MeetingSaveResponseV1 saveV1(MeetingSaveRequestV1 meetingSaveRequestV1) { | ||
|
@@ -58,7 +63,7 @@ public Meeting findByInviteCode(String inviteCode) { | |
} | ||
|
||
public Meeting findById(Long meetingId) { | ||
return meetingRepository.findById(meetingId) | ||
return meetingRepository.findByIdAndOverdueFalse(meetingId) | ||
.orElseThrow(() -> new OdyNotFoundException("존재하지 않는 모임입니다.")); | ||
} | ||
|
||
|
@@ -93,4 +98,12 @@ public MateSaveResponseV2 saveMateAndSendNotifications(MateSaveRequestV2 mateSav | |
} | ||
return mateService.saveAndSendNotifications(mateSaveRequest, member, meeting); | ||
} | ||
|
||
@Scheduled(cron = "0 0 4 * * *", zone = "Asia/Seoul") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [질문] 스케쥴드 애너테이션은 처음 활용해보아서 그런데 cron 안에 들어있는 건 4시를 나타낸다고 보아도 좋을까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 넵 순서대로 다음과 같아요 *은 모든 값을 나타내므로, 해석하면 매 년, 매 요일, 매 월, 매 일 , 04:00:00에 스케줄링을 하는 것 입니다 ! |
||
public void scheduleOverdueMeetings() { | ||
meetingRepository.updateAllByNotOverdueMeetings(); | ||
List<Meeting> meetings = meetingRepository.findAllByUpdatedTodayAndOverdue(); | ||
log.info("약속 시간이 지난 약속들 overdue = true로 update 쿼리 실행"); | ||
notificationService.unSubscribeTopic(meetings); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,7 @@ | ||
package com.ody.notification.dto.request; | ||
|
||
import com.ody.notification.domain.FcmTopic; | ||
import com.ody.notification.domain.Notification; | ||
|
||
public record FcmSendRequest(FcmTopic fcmTopic, Notification notification) { | ||
public record FcmSendRequest(Notification notification) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. notification이 가져도 되는 정보라 생각해요! 좋은 리팩터링이네요 👍 |
||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[질문]
Notification에도 updatedAt 필드가 생겼네요.
의도한 동작이었을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BaseEntity를 상속받는 상황에서 어쩔수 없는 상황이라 생각했고
불필요한 정보도 아니라고 생각했습니다 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[질문]
BaseEntity에서 updateAt이나 createAt은 Eta 필드로도 있는데요. 필드명이 같을 경우에 updateAt 반영이 하위 클래스의 같은 필드명인 updateAt에는 영향을 주지 않는지 궁금해요! 만약 영향을 주게 된다면 isMissing true 처리 같은 로직에 영향을 받을 수 있다고 생각이 되어서요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eta
는BaseEntity
를 상속받고 있지 않아 영향이 없을 것 같습니다 !다만 필드명이 동일해서 헷갈리긴 할 것 같네요
혼동을 방지하기 위해서 데이터 생성, 변경 시간 측정을 위해 사용한
BaseEntity
의 필드명을 변경하는 것 보다는로직으로 활용하고 있는
Eta
의 필드 이름을 바꿔주는게 좋을 것 같은데 어떻게 생각하시나요 ?