feat: notify users when monthly periods close - #498
Conversation
|
Approach I will add one scheduled period close producer that runs through the existing in-process and Hangfire registration paths. It will resolve each subscribed user's local date, select the just-closed month only on the first local day, require activity in that month, persist a localized Notification with an atomic dedupe key, and send it through IPushNotificationService after persistence. The implementation will land in a new PeriodCloseNotificationService under Infrastructure, the background job registration extension and BackgroundServices configuration under Api, the Notification entity and notification query contract under Domain and Application, OrbitDbContext plus an EF migration under Infrastructure, and focused tests in the existing Domain, Application, and Infrastructure test projects. I will carry the notification kind, year, and month as additive nullable fields and retain the app-relative URL for the existing push transport. I rejected a second scheduler, a separate delivery table, and URL-only identity because those shapes would bypass the repository mechanisms or leave the in-app notification contract unable to expose the exact period. |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Reviewed the PR scaffold at 8361855.
- Initialization commit — The sole commit contains no file changes, so this revision introduces no runtime behavior, persistence changes, tests, or API contracts to assess.
GPT Sol | 𝕏
|
Implementation is paused before code changes because the clarified identity shape cannot satisfy the acceptance criteria against the live model.
No safe implementation can preserve both the exact key and URL shape while delivering one notification per user per week, month, and year. |
|
Revised approach after the ticket correction I will implement monthly notifications only. The producer will use the existing scheduled job registration, evaluate each eligible user at the first day of their local month, identify the just closed month, require activity during that month, create the existing Notification entity with the app relative URL /progress?wrapped=month&year={year}&month={month}, and use the globally unique key wrapped-{userId}-{year}-{month}. It will persist before using the existing push service, with duplicate insertion handled atomically so retries and concurrent runs remain safe. The change will land in the existing job and notification service areas under Application and Infrastructure, the BackgroundServices configuration and registration under Api, the existing URL helper, and focused unit tests in the matching test projects. No Notification columns, notification type, second scheduler, or cross repository route changes will be added. The feature flag will default to false until #63 is live. |
There was a problem hiding this comment.
Important
The monthly dedupe record can permanently suppress the only push after a transient or cancellation-time delivery failure.
Reviewed changes Reviewed the implementation added in 0f966d3 since the prior Pullfrog review at 8361855.
- Scheduled producer Added default-off registration through both the in-process and durable recurring-job paths, with a 30-minute period-close scan.
- Local period selection Resolved each subscribed user's local date, grouped first-of-month users by closed period, and required positive historical habit activity.
- Localized notification Added English and Brazilian Portuguese copy, an addressable closed-month URL, and a user-period dedupe key before push dispatch.
- Focused coverage Added URL, registration, eligibility, local-boundary, localization, and sequential deduplication tests.
GPT Sol | 𝕏
|
Review fix approach The persisted Notification remains the concurrency and rerun claim. If IPushNotificationService throws after that claim is saved, I will remove the just-created row with an uncancelled cleanup save before propagating the failure. This prevents a transient or cancellation-time exception from permanently suppressing the period while preserving the existing entity, unique DedupeKey, scheduler, and push transport. Focused tests will prove both an ordinary push exception and cancellation leave no row and allow the next run to retry. |
There was a problem hiding this comment.
Important
The exception-only rollback still misses exhausted transient failures reported by the production push service, so a monthly push can remain permanently suppressed.
Reviewed changes Reviewed the retry recovery added in 8957952 after the previously analyzed 522939e revision.
- Exception recovery Deleted the persisted notification claim with an uncancelled database save when the push call throws, then propagated the original failure.
- Failure-path coverage Added retry tests for a throwing push substitute and a canceled task.
GPT Sol | 𝕏
|
Pullfrog identified that the existing push abstraction hides exhausted transport failures. I will add an append-only outcome-returning method to IPushNotificationService while preserving SendToUserAsync for current callers. PushNotificationService will report success only when at least one subscription accepts delivery, and the period-close producer will release its notification claim when no delivery succeeds. I will cover exhausted Web Push, stale or unavailable transports, partial success, and the producer retry path. This shape avoids changing existing notification semantics while giving the new dedupe workflow the confirmed outcome it requires. |
|
There was a problem hiding this comment.
Important
No new issues were introduced by 98b6455, but approval remains withheld because the earlier unconfirmed-delivery thread is still open.
Reviewed changes Reviewed the delivery-outcome fix added in 98b6455 since the prior Pullfrog review at 8957952.
- Delivery outcomes Added a boolean acceptance result across FCM and Web Push while preserving existing callers through
SendToUserAsync. - Producer recovery Released the monthly notification claim when no subscription accepts delivery, allowing the next eligible scan to retry.
- Focused coverage Asserted concrete Web Push outcomes and producer retries after an explicit failed-delivery result.
GPT Sol | 𝕏
There was a problem hiding this comment.
Important
The delivery aggregation is coherent, but the new Firebase success signal lacks the required external interface evidence in the PR body.
Reviewed changes Re-reviewed the delivery outcome work added in 98b6455 since the prior Pullfrog review at 8957952.
-
Added delivery outcomes across FCM and Web Push while preserving existing callers through
SendToUserAsync. -
Released failed monthly claims when no subscription accepts delivery so later eligible scans can retry.
-
Expanded focused coverage for successful, stale, transient, mixed, and producer retry outcomes.
GPT Sol | 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Reviewed the delivery-outcome changes added in 98b6455 since the prior Pullfrog review at 8957952.
-
Delivery outcomes Added an explicit acceptance result across FCM and Web Push while preserving existing callers through
SendToUserAsync. -
Claim recovery Released the monthly notification claim when no subscription accepts delivery, allowing a later eligible scan to retry.
-
Focused coverage Added exact successful, stale, transient, mixed, and producer retry assertions; the focused infrastructure tests pass.
GPT Sol | 𝕏




Closes thomasluizon/orbit-tickets#366 ## Summary - Adds a recurring monthly Wrapped notification producer through the existing scheduled job paths. - Resolves each subscribed user's local date and emits only on the first local day of a month. - Requires a positive, non-deleted habit log in the closed month, including historical logs from deleted habits. - Persists the existing Notification entity before sending through IPushNotificationService. - Uses a user-scoped unique dedupe key so reruns and concurrent instances cannot create duplicates. - Adds an append-only push outcome method and releases the dedupe row when no subscription accepts delivery or the push call throws, allowing a later run to retry. - Localizes the title, body, and month name for English and Brazilian Portuguese. - Defaults the rollout flag to false until the client route is live. ## Contract The notification URL is /progress?wrapped=month&year={year}&month={month}. Ticket thomasluizon/orbit-tickets#63 consumes this exact app-relative shape. Weekly and yearly closed periods remain unsupported because the recap endpoint cannot address them. That gap is tracked by thomasluizon/orbit-tickets#369. ## External interface evidence
A real Firebase invocation was not used because it would send to live device tokens. The installed FirebaseAdmin 3.6.0 package contract was inspected at C:\Users\thoma.nuget\packages\firebaseadmin\3.6.0\lib\net6.0\FirebaseAdmin.xml. Re-derive it with
rg -n "BatchResponse|SendResponse.IsSuccess|FirebaseMessaging.SendAsync" C:\Users\thoma\.nuget\packages\firebaseadmin\3.6.0\lib\net6.0\FirebaseAdmin.xml.The complete relevant SDK response shape is
BatchResponse { Responses: IReadOnlyList<SendResponse>, SuccessCount: int, FailureCount: int }andSendResponse { MessageId: string?, Exception: Exception?, IsSuccess: bool }. The package contract states thatIsSuccess == trueguarantees a non-null MessageId, while false guarantees a non-null Exception. It also states that a successfulSendAsyncreturn is acceptance by FCM and its message ID represents successful handoff. This supports treating either a true batchIsSuccessor a completed individualSendAsynccall as an accepted delivery for the retry decision.Verification - dotnet build Orbit.slnx: passed with 0 errors - Focused tests: 48 passed - dotnet test --no-build: 5,980 passed ## Assumptions - Placed the producer in Orbit.Infrastructure/Services beside the repository's concrete IScheduledJob implementations; rejected a new Orbit.Application Jobs folder because no existing recurring producer is implemented there. - Treated the absence of PushSubscription rows as notifications disabled; rejected creating an in-app Notification row for an unsubscribed user because UnsubscribePushCommand removes those rows and the acceptance criterion requires no notification. ## Manual steps - After ticket thomasluizon/orbit-tickets#63 is live in the Play fleet, set BackgroundServices__PeriodCloseNotificationEnabled to true in the Render Dashboard for the Orbit API web service under Environment. Confirm the service restart registers period-close-notification and the next eligible local month boundary creates a wrapped-{userId}-{year}-{month} Notification row.