Added a job to remind members before their gift subscription ends#27436
Open
Added a job to remind members before their gift subscription ends#27436
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
✅ Files skipped from review due to trivial changes (9)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds gift subscription reminder functionality: introduces two timing constants, an HTML and plaintext reminder email template and renderer, and a reminder send flow in the email service. Extends the Gift domain with a nullable Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
mike182uk
force-pushed
the
BER-3525-add-job-to-send-reminder
branch
from
a44c3c1 to
bf545de
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ghost/core/test/integration/services/members/send-gift-reminders.test.js (1)
171-188: Add the missing-email variant next to this skip-path test.
email_disabledandemail === nullare separate reminder guards, but only one is exercised end-to-end here. A small sibling test would protect against trying to send to an empty address or failing after the reminder stamp is written.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/integration/services/members/send-gift-reminders.test.js` around lines 171 - 188, Add a sibling test mirroring the existing "marks the gift as reminded but does not email when the redeemer has email_disabled" case but instead set the redeemer's email to null (e.g., await models.Member.edit({email: null}, {id: redeemerMember.id});), create the same inWindow gift with createRedeemedGift({consumesAt: inWindow}), call giftService.service.processReminders(), and assert the same results: remindedCount 0, skippedCount 1, failedCount 0, emailMockReceiver.assertSentEmailCount(0), and that models.Gift.findOne(...).get('consumes_soon_reminder_sent_at') is set to ensure the reminder stamp is written even when email is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/core/core/server/services/members/jobs/send-gift-reminders.js`:
- Around line 9-25: The cancel path currently only posts a 'cancelled' message
to parentPort and doesn't stop the running job, so late cancels still let the
async worker finish and post 'done'; fix by wiring a cancellation/abort signal
into the worker: create an AbortController in the worker scope, pass its signal
into processReminders() (and any downstream async functions) so they can
early-return when signal.aborted, and in cancel() call controller.abort() and
ensure the worker stops (avoid posting 'done' after abort and exit/cleanup as
before); update the parentPort.once('message') handler to call cancel() which
now aborts the controller and prevents further processing.
---
Nitpick comments:
In `@ghost/core/test/integration/services/members/send-gift-reminders.test.js`:
- Around line 171-188: Add a sibling test mirroring the existing "marks the gift
as reminded but does not email when the redeemer has email_disabled" case but
instead set the redeemer's email to null (e.g., await models.Member.edit({email:
null}, {id: redeemerMember.id});), create the same inWindow gift with
createRedeemedGift({consumesAt: inWindow}), call
giftService.service.processReminders(), and assert the same results:
remindedCount 0, skippedCount 1, failedCount 0,
emailMockReceiver.assertSentEmailCount(0), and that
models.Gift.findOne(...).get('consumes_soon_reminder_sent_at') is set to ensure
the reminder stamp is written even when email is missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e18775f2-d332-47eb-a87c-0d2e61d143ce
📒 Files selected for processing (19)
ghost/core/core/server/services/gifts/constants.tsghost/core/core/server/services/gifts/email-templates/gift-reminder.hbsghost/core/core/server/services/gifts/email-templates/gift-reminder.tsghost/core/core/server/services/gifts/gift-bookshelf-repository.tsghost/core/core/server/services/gifts/gift-email-renderer.tsghost/core/core/server/services/gifts/gift-email-service.tsghost/core/core/server/services/gifts/gift-repository.tsghost/core/core/server/services/gifts/gift-service.tsghost/core/core/server/services/gifts/gift.tsghost/core/core/server/services/members/jobs/index.jsghost/core/core/server/services/members/jobs/send-gift-reminders.jsghost/core/core/server/services/members/service.jsghost/core/test/integration/services/members/send-gift-reminders.test.jsghost/core/test/unit/server/services/gifts/gift-bookshelf-repository.test.tsghost/core/test/unit/server/services/gifts/gift-controller.test.tsghost/core/test/unit/server/services/gifts/gift-email-service.test.jsghost/core/test/unit/server/services/gifts/gift-service.test.tsghost/core/test/unit/server/services/gifts/gift.test.tsghost/core/test/unit/server/services/gifts/utils.ts
Comment on lines
+9
to
+25
| function cancel() { | ||
| if (parentPort) { | ||
| parentPort.postMessage('Gift reminder job cancelled before completion'); | ||
| parentPort.postMessage('cancelled'); | ||
| } else { | ||
| setTimeout(() => { | ||
| process.exit(0); | ||
| }, 1000); | ||
| } | ||
| } | ||
|
|
||
| if (parentPort) { | ||
| parentPort.once('message', (message) => { | ||
| if (message === 'cancel') { | ||
| return cancel(); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Cancellation only acknowledges the message; it does not stop the worker.
With parentPort, cancel() just posts 'cancelled'. The async IIFE keeps running, so a late cancel can still initialize services, call processReminders(), send emails, and then post 'done'. That defeats the shutdown path and can report conflicting terminal states.
Suggested direction
+let cancelled = false;
+
function cancel() {
+ cancelled = true;
if (parentPort) {
parentPort.postMessage('Gift reminder job cancelled before completion');
parentPort.postMessage('cancelled');
- } else {
- setTimeout(() => {
- process.exit(0);
- }, 1000);
+ return;
}
+
+ setTimeout(() => {
+ process.exit(0);
+ }, 1000);
}
if (parentPort) {
parentPort.once('message', (message) => {
if (message === 'cancel') {
@@
(async () => {
try {
+ if (cancelled) {
+ return;
+ }
+
const startDate = new Date();
debug('Starting gift reminder send');
const giftService = require('../../gifts');
await giftService.init();
+
+ if (cancelled) {
+ return;
+ }
+
const {remindedCount, skippedCount, failedCount} = await giftService.service.processReminders();If shutdown is expected to interrupt an in-flight batch too, this needs to be plumbed through processReminders() with an abort signal rather than only acknowledging the parent message.
Also applies to: 28-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ghost/core/core/server/services/members/jobs/send-gift-reminders.js` around
lines 9 - 25, The cancel path currently only posts a 'cancelled' message to
parentPort and doesn't stop the running job, so late cancels still let the async
worker finish and post 'done'; fix by wiring a cancellation/abort signal into
the worker: create an AbortController in the worker scope, pass its signal into
processReminders() (and any downstream async functions) so they can early-return
when signal.aborted, and in cancel() call controller.abort() and ensure the
worker stops (avoid posting 'done' after abort and exit/cleanup as before);
update the parentPort.once('message') handler to call cancel() which now aborts
the controller and prevents further processing.
ref https://linear.app/ghost/issue/BER-3525 - sends a one-time reminder email to the gift redeemer 7 days before their paid access ends (`consumes_at`) - introduces `processReminders` on `GiftService` + daily `send-gift-reminders` worker, gated on the `giftSubscriptions` labs flag - skips gifts that consume within the next 3 days (`GIFT_REMINDER_FLOOR_DAYS`) - a reminder with too little time to act on is worse than no reminder - records `consumes_soon_reminder_sent_at` inside the locked transaction before sending the email: any duplicate reminder path (e.g. the scheduler integration shipping in a follow-up PR) serialises on the row lock and no-ops, guaranteeing at-most-once delivery at the cost of a possible missed reminder on SMTP failure - tier lookup happens before the transaction so a missing tier is a recoverable failure - the next run will pick the gift up again after the admin restores the tier - per-gift errors are caught and counted as `failedCount` so a single transient failure doesn't abort the whole batch - email-disabled and missing-email redeemers are skipped but still marked as reminded to prevent retry loops
mike182uk
force-pushed
the
BER-3525-add-job-to-send-reminder
branch
from
bf545de to
adffd9a
Compare
|
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
1 participant
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.
ref https://linear.app/ghost/issue/BER-3525
consumes_at)processRemindersonGiftService+ dailysend-gift-remindersworker, gated on thegiftSubscriptionslabs flagGIFT_REMINDER_FLOOR_DAYS) - a reminder with too little time to act on is worse than no reminderconsumes_soon_reminder_sent_atinside the locked transaction before sending the email: any duplicate reminder path (e.g. the scheduler integration shipping in a follow-up PR) serialises on the row lock and no-ops, guaranteeing at-most-once delivery at the cost of a possible missed reminder on SMTP failurefailedCountso a single transient failure doesn't abort the whole batch