Refactor notification services for improved scheduling and logic#43
Refactor notification services for improved scheduling and logic#43
Conversation
…ize WorkScheduleTime for scheduling notifications
…e to use NotificationEarningsService and NotificationEligibilityService for improved notification logic
… for improved type safety and clarity
Test Results46 tests 46 ✅ 0s ⏱️ Results for commit 95ac961. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
Refactors the notification subsystem by extracting shared eligibility checks and earnings calculations into dedicated services, standardizing work schedule time handling (including midnight-crossing shifts), and improving FCM dispatch type-safety.
Changes:
- Centralized eligibility data loading/checks into
NotificationEligibilityService+NotificationEligibilityContext, and updated batch services to use them. - Introduced
WorkScheduleTimeto normalize clock-in/out times and correctly compute clock-out dates for overnight shifts; applied it in batch + sync flows. - Added
FcmRequestDTO and updated dispatch/Fcm service interfaces accordingly; extracted earnings calculation intoNotificationEarningsService.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/kotlin/com/moa/service/notification/PaydayNotificationBatchService.kt | Switches payday notification generation to centralized eligibility context/checks. |
| src/main/kotlin/com/moa/service/notification/NotificationSyncService.kt | Uses WorkScheduleTime for consistent minute-truncation and midnight-crossing clock-out scheduling. |
| src/main/kotlin/com/moa/service/notification/NotificationMessageBuilder.kt | Delegates daily earnings computation to NotificationEarningsService. |
| src/main/kotlin/com/moa/service/notification/NotificationEligibilityService.kt | New service to load eligibility-related data (terms/settings/tokens/overrides). |
| src/main/kotlin/com/moa/service/notification/NotificationEligibilityContext.kt | New context object encapsulating eligibility checks and override access. |
| src/main/kotlin/com/moa/service/notification/NotificationEarningsService.kt | New service extracting policy/payroll resolution + daily earnings calculation. |
| src/main/kotlin/com/moa/service/notification/NotificationDispatchService.kt | Updates dispatch pipeline to use FcmRequest and scopes DispatchItem privately. |
| src/main/kotlin/com/moa/service/dto/FcmRequest.kt | Adds request DTO for FCM dispatch. |
| src/main/kotlin/com/moa/service/FcmService.kt | Updates sendEach signature to accept FcmRequest and adjusts batching logic. |
| src/main/kotlin/com/moa/entity/notification/WorkScheduleTime.kt | Adds normalized work schedule time structure with midnight-crossing clock-out date logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private fun buildClockOutBody(notification: NotificationLog): String { | ||
| val earnings = calculateTodayEarnings(notification.memberId, notification.scheduledDate) | ||
| val earnings = notificationEarningsService.calculateTodayEarnings( | ||
| notification.memberId, | ||
| notification.scheduledDate, | ||
| ) | ||
| if (earnings == BigDecimal.ZERO) { | ||
| return CLOCK_OUT_FALLBACK_BODY | ||
| } | ||
| val formatted = NumberFormat.getNumberInstance(Locale.KOREA).format(earnings) | ||
| return notification.notificationType.getBody(formatted) | ||
| } |
There was a problem hiding this comment.
calculateTodayEarnings() can throw NotFoundException when a member is missing a representative work policy or payroll. Since NotificationDispatchService doesn't catch exceptions from NotificationMessageBuilder, a single missing policy/payroll can abort the entire dispatch loop and leave pending logs unsent/unupdated. Consider catching NotFoundException (and possibly other runtime failures) here and falling back to CLOCK_OUT_FALLBACK_BODY (or treating earnings as 0) so dispatch remains robust.
| data class WorkScheduleTime( | ||
| val clockInTime: LocalTime, | ||
| val clockOutTime: LocalTime, | ||
| ) { | ||
| val isMidnightCrossing: Boolean = clockOutTime < clockInTime | ||
|
|
||
| fun clockOutDate(baseDate: LocalDate): LocalDate = | ||
| if (isMidnightCrossing) baseDate.plusDays(1) else baseDate | ||
|
|
||
| companion object { | ||
| fun of(clockIn: LocalTime, clockOut: LocalTime) = WorkScheduleTime( | ||
| clockInTime = LocalTime.of(clockIn.hour, clockIn.minute), | ||
| clockOutTime = LocalTime.of(clockOut.hour, clockOut.minute), | ||
| ) | ||
| } |
There was a problem hiding this comment.
WorkScheduleTime is a public data class, so callers can instantiate it directly with LocalTime values that include seconds/nanos, while of() truncates to minute precision. This can lead to inconsistent scheduling and incorrect isMidnightCrossing/clockOutDate calculations if the primary constructor is used. Consider enforcing normalization in the class itself (e.g., truncate in init) or making the primary constructor private and requiring of() for construction.
…tions and update WorkScheduleTime to a private constructor for better encapsulation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/main/kotlin/com/moa/service/notification/NotificationEligibilityService.kt
Outdated
Show resolved
Hide resolved
…bilityService.kt Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This pull request introduces a significant refactoring and modularization of the notification scheduling, eligibility, and messaging logic. It extracts and centralizes eligibility checks into dedicated services and context classes, introduces new data structures for work schedule time handling, and improves the clarity and maintainability of FCM notification dispatching. Additionally, it moves the calculation of daily earnings for notifications into its own service.
Notification Eligibility and Scheduling Refactor:
NotificationEligibilityServiceandNotificationEligibilityContextto centralize and encapsulate all logic related to notification eligibility checks, such as required term agreements, notification settings, FCM token presence, and work schedule overrides. This replaces scattered logic and makes it reusable across batch services. [1] [2]NotificationBatchServiceto use the new eligibility service/context, removing duplicated eligibility logic and simplifying notification creation. Eligibility checks now use more descriptive and extensible methods. [1] [2] [3]Work Schedule Handling:
WorkScheduleTimedata class to encapsulate clock-in/out times and logic for handling midnight-crossing shifts, improving clarity and correctness when scheduling notifications.WorkScheduleTime, ensuring correct handling of clock-out dates for overnight shifts. [1] [2]Notification Dispatch and FCM Improvements:
FcmRequestDTO and updatedFcmService.sendEachto use this type, improving type safety and clarity in FCM notification dispatching. [1] [2] [3] [4] [5]NotificationDispatchServiceto use the newFcmRequestand moved theDispatchItemdata class to a private scope, enhancing encapsulation. [1] [2] [3]Earnings Calculation Modularization:
NotificationEarningsService, removing this responsibility fromNotificationMessageBuilderand related classes. This improves separation of concerns and testability. [1] [2] [3]These changes collectively make the notification system more modular, maintainable, and extensible, laying groundwork for future enhancements and reducing duplication.