Remove holiday notification logic from batch processing#55
Merged
Conversation
Test Results68 tests 68 ✅ 1s ⏱️ Results for commit 070e334. ♻️ This comment has been updated with latest results. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the notification system’s public-holiday behavior by removing public-holiday-specific notification generation and making the automatic batch skip generating any work notifications on holidays, while clarifying that user-initiated scheduling can still create notifications.
Changes:
NotificationBatchServicenow returns early on public holidays and removes allPUBLIC_HOLIDAYnotification generation logic.NotificationBatchScheduleradds a “batch completed” log for improved traceability.- Holiday-related tests are rewritten to reflect the new policy, and
NotificationSyncServiceadds a clarifying comment about holiday guards applying only to the batch.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/test/kotlin/com/moa/service/notification/NotificationBatchServiceHolidayTest.kt | Rewrites tests to assert no batch notifications are generated on public holidays. |
| src/main/kotlin/com/moa/service/notification/NotificationSyncService.kt | Adds documentation clarifying holiday guard scope (batch-only) and adjusts formatting. |
| src/main/kotlin/com/moa/service/notification/NotificationBatchService.kt | Removes PUBLIC_HOLIDAY notification path and adds holiday early-return in batch generation. |
| src/main/kotlin/com/moa/service/notification/NotificationBatchScheduler.kt | Adds completion logging around the daily notification batch run. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
26
to
30
| @Transactional | ||
| fun generateNotificationsForDate(date: LocalDate) { | ||
| if (publicHolidayService.isHoliday(date)) { | ||
| generateHolidayNotifications(date) | ||
| } else { | ||
| generateWorkNotifications(date) | ||
| } | ||
| } | ||
|
|
||
| private fun generateHolidayNotifications(date: LocalDate) { | ||
| val allPolicies = workPolicyVersionRepository.findLatestEffectivePoliciesPerMember(date) | ||
| if (allPolicies.isEmpty()) return | ||
|
|
||
| val allMemberIds = allPolicies.map { it.memberId } | ||
| val alreadyGeneratedMemberIds = notificationLogRepository | ||
| .findMemberIdsByScheduledDateAndNotificationTypeAndMemberIdIn(date, NotificationType.PUBLIC_HOLIDAY, allMemberIds) | ||
| .toSet() | ||
|
|
||
| val targetPolicies = allPolicies.filter { it.memberId !in alreadyGeneratedMemberIds } | ||
| if (targetPolicies.isEmpty()) return | ||
|
|
||
| val memberIds = targetPolicies.map { it.memberId } | ||
| val requiredTermCodes = notificationEligibilityService.findRequiredTermCodes() | ||
| val context = notificationEligibilityService.loadContext(memberIds, date) | ||
|
|
||
| log.info("Generating public holiday notifications for {} members on {}", memberIds.size, date) | ||
|
|
||
| val notifications = targetPolicies.mapNotNull { policy -> | ||
| createHolidayNotificationIfEligible(policy.memberId, date, requiredTermCodes, context) | ||
| } | ||
|
|
||
| notificationLogRepository.saveAll(notifications) | ||
| log.info("Created {} public holiday notification logs for {}", notifications.size, date) | ||
| } | ||
|
|
||
| private fun createHolidayNotificationIfEligible( | ||
| memberId: Long, | ||
| date: LocalDate, | ||
| requiredCodes: Set<String>, | ||
| context: NotificationEligibilityContext, | ||
| ): NotificationLog? { | ||
| if (!context.hasAgreedToAll(memberId, requiredCodes)) return null | ||
| if (!context.isSettingEnabled(memberId, NotificationSettingType.WORK)) return null | ||
| if (!context.hasFcmToken(memberId)) return null | ||
|
|
||
| return NotificationLog(memberId, NotificationType.PUBLIC_HOLIDAY, date, PUBLIC_HOLIDAY_NOTIFICATION_TIME) | ||
| if (publicHolidayService.isHoliday(date)) return | ||
| generateWorkNotifications(date) | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
아니야 생성하는건 공휴일 전날 근무일 기준으로 발송 예약하는거라 상관없을듯
Comment on lines
+23
to
+27
| @@ -24,5 +24,6 @@ class NotificationBatchScheduler( | |||
| log.info("{} : 출퇴근 알림 전송 배치 실행", LocalDateTime.now()) | |||
| val today = LocalDate.now(ZoneId.of("Asia/Seoul")) | |||
| notificationBatchService.generateNotificationsForDate(today) | |||
| log.info("{} : 출퇴근 알림 전송 배치 완료", LocalDateTime.now()) | |||
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/test/kotlin/com/moa/service/notification/NotificationBatchServiceHolidayTest.kt:163
- 이 테스트는 CLOCK_OUT의 scheduledDate가 공휴일(신정)이어도 로그가 생성된다고 검증합니다. PR 설명에는 "공휴일에는 CLOCK_IN/CLOCK_OUT 포함 어떤 알림도 보내지 않는다"고 되어 있어 정책과 테스트가 충돌합니다. 공휴일에 실제 발송/예약을 허용할 의도라면 PR 설명 및 상단 정책 문구를 수정하고, 반대로 금지 정책이라면 NotificationBatchService에서 clockOutDate가 공휴일인 경우 CLOCK_OUT 생성 제외(또는 scheduledDate 기준 공휴일 가드)를 추가하는 쪽으로 정리해 주세요.
@Test
fun `야간 근무자의 CLOCK_OUT 은 다음날이 공휴일이어도 정상 생성된다`() {
공휴일로_지정(신정)
회원_등록(
id = 1L,
정책기준일 = 공휴일전날,
출근시각 = LocalTime.of(22, 0),
퇴근시각 = LocalTime.of(6, 0),
)
sut.generateNotificationsForDate(공휴일전날)
val clockOut = notificationLogs.first { it.notificationType == NotificationType.CLOCK_OUT }
assertThat(clockOut.scheduledDate).isEqualTo(신정)
}
Comment on lines
+29
to
33
| * 공휴일 정책 박제 테스트. | ||
| * | ||
| * - 모든 **out-of-process** 의존성(JPA Repository)만 Fake로 대체한다. | ||
| * - `PublicHolidayService`, `NotificationEligibilityService` 는 **실물**을 사용한다. | ||
| * - 검증은 오직 최종 상태(`notificationLogs` 리스트의 내용)로만 한다 — `verify` 금지. | ||
| * | ||
| * Fake 저장소(`holidayCalendar`, `workPolicies`, …)가 프로덕션의 MySQL 테이블 역할을 한다. | ||
| * - 정책: 공휴일에는 어떤 푸시 알림도 생성하지 않는다 (이전에는 PUBLIC_HOLIDAY 알림을 보냈음) | ||
| * - `NotificationBatchService.generateNotificationsForDate` 의 holiday 가드 동작을 검증 | ||
| */ |
Comment on lines
26
to
30
| @Transactional | ||
| fun generateNotificationsForDate(date: LocalDate) { | ||
| if (publicHolidayService.isHoliday(date)) { | ||
| generateHolidayNotifications(date) | ||
| } else { | ||
| generateWorkNotifications(date) | ||
| } | ||
| } | ||
|
|
||
| private fun generateHolidayNotifications(date: LocalDate) { | ||
| val allPolicies = workPolicyVersionRepository.findLatestEffectivePoliciesPerMember(date) | ||
| if (allPolicies.isEmpty()) return | ||
|
|
||
| val allMemberIds = allPolicies.map { it.memberId } | ||
| val alreadyGeneratedMemberIds = notificationLogRepository | ||
| .findMemberIdsByScheduledDateAndNotificationTypeAndMemberIdIn(date, NotificationType.PUBLIC_HOLIDAY, allMemberIds) | ||
| .toSet() | ||
|
|
||
| val targetPolicies = allPolicies.filter { it.memberId !in alreadyGeneratedMemberIds } | ||
| if (targetPolicies.isEmpty()) return | ||
|
|
||
| val memberIds = targetPolicies.map { it.memberId } | ||
| val requiredTermCodes = notificationEligibilityService.findRequiredTermCodes() | ||
| val context = notificationEligibilityService.loadContext(memberIds, date) | ||
|
|
||
| log.info("Generating public holiday notifications for {} members on {}", memberIds.size, date) | ||
|
|
||
| val notifications = targetPolicies.mapNotNull { policy -> | ||
| createHolidayNotificationIfEligible(policy.memberId, date, requiredTermCodes, context) | ||
| } | ||
|
|
||
| notificationLogRepository.saveAll(notifications) | ||
| log.info("Created {} public holiday notification logs for {}", notifications.size, date) | ||
| } | ||
|
|
||
| private fun createHolidayNotificationIfEligible( | ||
| memberId: Long, | ||
| date: LocalDate, | ||
| requiredCodes: Set<String>, | ||
| context: NotificationEligibilityContext, | ||
| ): NotificationLog? { | ||
| if (!context.hasAgreedToAll(memberId, requiredCodes)) return null | ||
| if (!context.isSettingEnabled(memberId, NotificationSettingType.WORK)) return null | ||
| if (!context.hasFcmToken(memberId)) return null | ||
|
|
||
| return NotificationLog(memberId, NotificationType.PUBLIC_HOLIDAY, date, PUBLIC_HOLIDAY_NOTIFICATION_TIME) | ||
| if (publicHolidayService.isHoliday(date)) return | ||
| generateWorkNotifications(date) | ||
| } |
Comment on lines
+29
to
+33
| * 공휴일 정책 박제 테스트. | ||
| * | ||
| * - 모든 **out-of-process** 의존성(JPA Repository)만 Fake로 대체한다. | ||
| * - `PublicHolidayService`, `NotificationEligibilityService` 는 **실물**을 사용한다. | ||
| * - 검증은 오직 최종 상태(`notificationLogs` 리스트의 내용)로만 한다 — `verify` 금지. | ||
| * | ||
| * Fake 저장소(`holidayCalendar`, `workPolicies`, …)가 프로덕션의 MySQL 테이블 역할을 한다. | ||
| * - 정책: 자동 배치는 생성 기준일(date)이 공휴일이면 푸시 알림 로그를 생성하지 않는다. | ||
| * - 비공휴일 근무에서 발생한 야간 근무자의 익일 CLOCK_OUT 알림은 scheduledDate가 공휴일이어도 생성한다. | ||
| * - `NotificationBatchService.generateNotificationsForDate` 의 holiday 가드 동작을 검증 |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or 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
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.
This pull request removes the generation of public holiday notifications from the notification batch process. Now, no notifications (including CLOCK_IN or CLOCK_OUT) are sent on public holidays, and the related code and tests for public holiday notifications have been deleted or updated accordingly. The change also clarifies that the public holiday guard only affects the automatic batch, not explicit user actions.
Key changes:
Notification Batch Logic:
NotificationBatchServiceno longer generates any notifications on public holidays; it simply returns early if the date is a holiday. All code related to generatingPUBLIC_HOLIDAYnotifications has been removed.Testing and Documentation:
NotificationBatchServicehas been rewritten to reflect the new policy: no notifications are generated on public holidays, regardless of member eligibility. All tests and helper methods related toPUBLIC_HOLIDAYnotifications have been removed or updated. [1] [2]Code Cleanup: