Fixed duplicate staff email when gift members upgrade to paid#27679
Fixed duplicate staff email when gift members upgrade to paid#27679
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe changes introduce an 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sagzy
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f64842d183
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| offerId: offerId, | ||
| batchId: options.batch_id | ||
| batchId: options.batch_id, | ||
| isFromGift: memberModel.get('status') === 'gift' |
There was a problem hiding this comment.
Using memberModel.get('status') here misses the incomplete → active checkout path for gift redeemers. In that flow, the first linkSubscription call (while Stripe status is incomplete) can already move the member off gift before the later activation webhook runs, so this flag becomes false and StaffService still sends the duplicate paid-subscription-started email the change is meant to suppress. Please compute this from the persisted pre-update status transition data (e.g., the previous status used for welcome-email gating) rather than the current model status at dispatch time.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/core/server/services/staff/staff-service.js (1)
113-117: ⚡ Quick winMove the gift short-circuit before data loading.
The guard works, but
getDataFromIdsstill runs first (Line 90), so suppressed events still do unnecessary DB reads.Suggested diff
async handleEvent(type, event) { if (type === MilestoneCreatedEvent && event.data.milestone) { await this.emails.notifyMilestoneReceived(event.data); } if (!['api', 'member'].includes(event.data.source)) { return; } + + // Suppress for gift→paid upgrades before fetching related models. + if (type === SubscriptionActivatedEvent && event.data.isFromGift) { + return; + } const {member, tier, subscription, offer} = await this.getDataFromIds({ memberId: event.data.memberId, tierId: event.data.tierId, subscriptionId: event.data.subscriptionId, offerId: event.data.offerId }); if (type === MemberCreatedEvent && member.status === 'free') { ... } else if (type === SubscriptionActivatedEvent) { - // Suppress for gift→paid upgrades — staff was already notified - // when the member redeemed the gift (notifyGiftSubscriptionStarted). - if (event.data.isFromGift) { - return; - } let attribution; ... }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/core/server/services/staff/staff-service.js` around lines 113 - 117, The early-return for gift-originated events is placed after getDataFromIds, causing unnecessary DB reads; move the isFromGift guard to run before calling getDataFromIds so that when event.data.isFromGift is true the function returns immediately and getDataFromIds is never invoked. Locate the current block referencing getDataFromIds(...) and the subsequent if (event.data.isFromGift) { return; } and reorder them so the isFromGift check occurs first, preserving the existing comment about suppressing gift→paid upgrades.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ghost/core/core/server/services/staff/staff-service.js`:
- Around line 113-117: The early-return for gift-originated events is placed
after getDataFromIds, causing unnecessary DB reads; move the isFromGift guard to
run before calling getDataFromIds so that when event.data.isFromGift is true the
function returns immediately and getDataFromIds is never invoked. Locate the
current block referencing getDataFromIds(...) and the subsequent if
(event.data.isFromGift) { return; } and reorder them so the isFromGift check
occurs first, preserving the existing comment about suppressing gift→paid
upgrades.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d11ea977-0145-48e0-9dcf-e28d3491daa0
📒 Files selected for processing (5)
ghost/core/core/server/services/members/members-api/repositories/member-repository.jsghost/core/core/server/services/staff/staff-service.jsghost/core/core/shared/events/subscription-activated-event.jsghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.jsghost/core/test/unit/server/services/staff/staff-service.test.js
- staff already receive a "🎁 Paid subscription started" notification when a member redeems a gift, so the follow-up "💸 Paid subscription started" email when that member later starts a real paid subscription is redundant - carries an `isFromGift` flag on `SubscriptionActivatedEvent`, set at the dispatch site in `member-repository.linkSubscription` based on the member's pre-update status — same precedent the paid welcome email already uses for this case - staff service short-circuits the email when the flag is set; the gift-service-wrapper subscriber on the same event still runs and consumes the gift as before Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
sagzy
force-pushed
the
gift-subscriptions/do-not-resend-paid-staff-notification
branch
from
1598936 to
172e393
Compare
1 participant
closes https://linear.app/ghost/issue/BER-3608
Summary
When a gift member upgrades to a real paid subscription, Ghost was sending staff two near-identical "Paid subscription started" emails:
🎁 Paid subscription started: <name>— at gift redemption (correct)💸 Paid subscription started: <name>— when the member later starts a paid subscription (redundant)This PR suppresses (2) for the gift→paid path.
Staff already know the member is paying — they were notified at gift redemption. The second email is redundant noise and slightly confusing because it looks like a fresh signup when it isn't.
Changes
Followed the precedent already established for the paid welcome email at
member-repository.js:1540, which uses_previousAttributes.status !== 'gift'to skip a duplicate welcome email for previously-gifted members.The staff notification fires off
SubscriptionActivatedEvent. We can't suppress the event itself becausegift-service-wrapperalso subscribes to it to consume the redeemed gift. So instead:isFromGiftflag on the event. Set at the dispatch site inmember-repository.linkSubscriptionbased onmemberModel.get('status') === 'gift'— at that point the member's status field still reflects the pre-update value, the same invariant the welcome-email check relies on one level up the call stack. Both dispatch sites (theincomplete → active3D-Secure path and thenew + immediately activepath — the common one for gift→paid) are updated.staff-service.handleEventbefore fetching attribution and callingnotifyPaidSubscriptionStarted.gift-service-wrapperstill consumes the gift; only the staff email is suppressed.This keeps the suppression decision colocated with the data (no extra DB query in the staff service), avoids the race where the gift may already be consumed by the parallel subscriber by the time the staff handler runs, and keeps the staff service decoupled from the gift service.
Tests
staff-service.test.js: verifiesnotifyPaidSubscriptionStartedis not called whenevent.data.isFromGift === true.member-repository.test.js: verify the dispatchedSubscriptionActivatedEventcarriesisFromGift: truefor astatus: 'gift'member, andisFromGift: falseotherwise.All existing unit tests still pass (46 staff-service, 54 member-repository), lint clean.
Manual verification
paid_subscription_started_notificationfor the staff user (default on).🎁email arrives in Mailpit.status = 'gift', complete a paid subscription checkout — confirm no💸email arrives.consumed, member ispaid, subscription is active.💸email.