Added staff notification on gift redemption#27414
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors GiftService.redeem() to return both the redeemed gift and the locked member from the repository transaction, moves tier lookup and staff notification out of the DB transaction, and catches/logs errors from the notification flow so they do not affect redemption. Adds StaffServiceEmails.notifyGiftSubscriptionStarted(...), new HTML and plain-text staff email templates for "new gift subscription", and updates unit and e2e tests to assert notification calls and resilience to notification failures. The public redeem signature remains unchanged and still returns the redeemed Gift. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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
force-pushed
the
gift-subscriptions/add-staff-notification
branch
from
fea3d0b to
7d569b9
Compare
![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: fea3d0b256
ℹ️ 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".
sagzy
force-pushed
the
gift-subscriptions/add-staff-notification
branch
2 times, most recently
from
a5a5b14 to
1767438
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: 2
🧹 Nitpick comments (1)
ghost/core/core/server/services/staff/staff-service-emails.js (1)
340-381: PrefersendToStaff()here to avoid duplicate email-delivery plumbing.This method reimplements shared template data + send loop that already exists in
sendToStaff, making behavior drift more likely over time.♻️ Refactor sketch
async notifyGiftSubscriptionStarted({memberId, memberName, memberEmail, tierName, buyerEmail}, options = {}) { const users = await this.models.User.getEmailAlertUsers('paid-started', options); - - for (const user of users) { - const to = user.email; - const memberData = this.getMemberData({ - id: memberId, - name: memberName ?? null, - email: memberEmail - }); - - const subject = `🎁 New paid subscriber: ${memberData.name}`; - const tierData = { - name: tierName - }; - - let staffUrl = this.urlUtils.urlJoin(this.urlUtils.urlFor('admin', true), '#', `/settings/staff/${user.slug}/email-notifications`); - - const templateData = { - memberData, - tierData, - giftedByEmail: buyerEmail, - siteTitle: this.settingsCache.get('title'), - siteIconUrl: this.blogIcon.getIconUrl({absolute: true, fallbackToDefault: false}), - siteUrl: this.urlUtils.getSiteUrl(), - siteDomain: this.siteDomain, - accentColor: this.settingsCache.get('accent_color'), - fromEmail: this.fromEmailAddress, - toEmail: to, - staffUrl: staffUrl - }; - - const {html, text} = await this.renderEmailTemplate('new-gift-subscription', templateData); - - await this.sendMail({ - to, - subject, - html, - text - }); - } + const memberData = this.getMemberData({ + id: memberId, + name: memberName ?? null, + email: memberEmail + }); + const subject = `🎁 New paid subscriber: ${memberData.name}`; + + await this.sendToStaff({ + users, + subject, + template: 'new-gift-subscription', + memberData, + templateData: { + tierData: {name: tierName}, + giftedByEmail: buyerEmail + } + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/staff/staff-service-emails.js` around lines 340 - 381, The notifyGiftSubscriptionStarted function duplicates email-template-data construction and the per-user send loop; replace its manual loop with a call to the shared helper sendToStaff so we reuse the centralized plumbing. Modify notifyGiftSubscriptionStarted to build only the templateData (memberData, tierData, giftedByEmail, siteTitle, siteIconUrl, siteUrl, siteDomain, accentColor, fromEmail) and then call this.sendToStaff('new-gift-subscription', templateData, { alert: 'paid-started', usersOptions: options }) or equivalent signature used by sendToStaff (ensuring to pass buyerEmail as giftedByEmail and the staff URL/toEmail values via sendToStaff options if required) and remove the manual renderEmailTemplate/sendMail loop; keep the existing memberData construction and references to this.getMemberData and this.urlUtils where needed.
🤖 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/test/e2e-api/members/signin.test.js`:
- Around line 226-228: The subject assertion in mockManager.assert.sentEmail is
too strict—update the regex used in the subject check to match the notifier's
current output by removing the mandatory " (via gift)" or making it optional;
for example, change the subject matcher from /New paid subscriber: Gift Receiver
\(via gift\)/ to either /New paid subscriber: Gift Receiver/ or /New paid
subscriber: Gift Receiver(?: \(via gift\))?/ so the test passes whether the
notifier includes the parenthetical or not.
In `@ghost/core/test/unit/server/services/gifts/gift-service.test.ts`:
- Around line 588-594: The test fails because the assertion expects keys
name/email but the implementation calls
staffServiceEmails.notifyGiftSubscriptionStarted with memberName/memberEmail;
update the sinon.assert.calledOnceWithExactly expected payload to use
memberName: 'Member Name' and memberEmail: 'member@example.com' (keeping
memberId, tierName, buyerEmail unchanged) so the object keys match the
notifyGiftSubscriptionStarted invocation.
---
Nitpick comments:
In `@ghost/core/core/server/services/staff/staff-service-emails.js`:
- Around line 340-381: The notifyGiftSubscriptionStarted function duplicates
email-template-data construction and the per-user send loop; replace its manual
loop with a call to the shared helper sendToStaff so we reuse the centralized
plumbing. Modify notifyGiftSubscriptionStarted to build only the templateData
(memberData, tierData, giftedByEmail, siteTitle, siteIconUrl, siteUrl,
siteDomain, accentColor, fromEmail) and then call
this.sendToStaff('new-gift-subscription', templateData, { alert: 'paid-started',
usersOptions: options }) or equivalent signature used by sendToStaff (ensuring
to pass buyerEmail as giftedByEmail and the staff URL/toEmail values via
sendToStaff options if required) and remove the manual
renderEmailTemplate/sendMail loop; keep the existing memberData construction and
references to this.getMemberData and this.urlUtils where needed.
🪄 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: 9540c873-288c-47fe-b9a6-d8929df8cd83
📒 Files selected for processing (7)
ghost/core/core/server/services/gifts/gift-service.tsghost/core/core/server/services/staff/email-templates/new-gift-subscription.hbsghost/core/core/server/services/staff/email-templates/new-gift-subscription.txt.jsghost/core/core/server/services/staff/staff-service-emails.jsghost/core/test/e2e-api/members/signin.test.jsghost/core/test/unit/server/services/gifts/gift-service.test.tsghost/core/test/unit/server/services/staff/staff-service.test.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/staff/staff-service-emails.js`:
- Around line 343-380: The loop over users currently aborts on the first
sendMail rejection; to fix it, isolate per-recipient work in the for loop by
wrapping the await this.sendMail(...) call in a try/catch and on error log the
failure (e.g. this.logger.error or existing logging helper) with context
(user.slug / to / memberId) and continue to the next user; also reduce
duplication by computing shared data (memberData, tierData, site-level fields
from this.settingsCache / this.blogIcon / this.urlUtils) once outside the loop
and then building a per-user templateData (adding to and staffUrl) inside the
loop before calling this.renderEmailTemplate and sendMail so failures are
handled per-recipient without aborting the whole notification.
🪄 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: a986a74f-6c67-4c43-95fc-fb15e8819fd6
📒 Files selected for processing (7)
ghost/core/core/server/services/gifts/gift-service.tsghost/core/core/server/services/staff/email-templates/new-gift-subscription.hbsghost/core/core/server/services/staff/email-templates/new-gift-subscription.txt.jsghost/core/core/server/services/staff/staff-service-emails.jsghost/core/test/e2e-api/members/signin.test.jsghost/core/test/unit/server/services/gifts/gift-service.test.tsghost/core/test/unit/server/services/staff/staff-service.test.js
✅ Files skipped from review due to trivial changes (3)
- ghost/core/core/server/services/staff/email-templates/new-gift-subscription.txt.js
- ghost/core/test/unit/server/services/staff/staff-service.test.js
- ghost/core/core/server/services/staff/email-templates/new-gift-subscription.hbs
🚧 Files skipped from review as they are similar to previous changes (3)
- ghost/core/test/e2e-api/members/signin.test.js
- ghost/core/core/server/services/gifts/gift-service.ts
- ghost/core/test/unit/server/services/gifts/gift-service.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/test/e2e-api/members/signin.test.js (1)
224-241: The suggested refactor is a reasonable style improvement, though not necessary.The
sentEmailfunction uses a sequential counter (emailCount) to retrieve emails in order, not first-match or uniqueness-sensitive behavior. Each call tosentEmail({to: 'jbloggs@example.com'})correctly retrieves the next email in the sequence and validates it—the current code is deterministic and works as intended.However, the suggested refactor is still worthwhile for clarity: embedding the
subjectmatcher directly in eachsentEmailcall makes the assertion intent more explicit and aligns with the predominant pattern used elsewhere in the test suite.Suggested refactor
mockManager.assert.sentEmailCount(2); - - const sentEmails = [ - mockManager.assert.sentEmail({ - to: 'jbloggs@example.com' - }), - mockManager.assert.sentEmail({ - to: 'jbloggs@example.com' - }) - ]; - const sentSubjects = sentEmails.map(mail => mail.subject); - - assert( - sentSubjects.some(subject => /Free member signup: Gift Receiver/.test(subject)), - `Expected one email subject to match /Free member signup: Gift Receiver/, got ${sentSubjects.join(', ')}` - ); - assert( - sentSubjects.some(subject => /New paid subscriber: Gift Receiver/.test(subject)), - `Expected one email subject to match /New paid subscriber: Gift Receiver/, got ${sentSubjects.join(', ')}` - ); + mockManager.assert.sentEmail({ + to: 'jbloggs@example.com', + subject: /Free member signup: Gift Receiver/ + }); + mockManager.assert.sentEmail({ + to: 'jbloggs@example.com', + subject: /New paid subscriber: Gift Receiver/ + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/e2e-api/members/signin.test.js` around lines 224 - 241, The test currently collects two emails via mockManager.assert.sentEmail and then inspects their subjects using sentSubjects and assert; to make intent explicit and follow suite patterns, call mockManager.assert.sentEmail with a subject matcher for each expected email (e.g., mockManager.assert.sentEmail({to: 'jbloggs@example.com', subject: /Free member signup: Gift Receiver/}) and the corresponding one for /New paid subscriber: Gift Receiver/) instead of mapping subjects and using sentSubjects with assert, so replace the two sentEmail calls and their subsequent sentSubjects/assert checks with two sentEmail calls that include the subject matchers directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/test/e2e-api/members/signin.test.js`:
- Around line 224-241: The test currently collects two emails via
mockManager.assert.sentEmail and then inspects their subjects using sentSubjects
and assert; to make intent explicit and follow suite patterns, call
mockManager.assert.sentEmail with a subject matcher for each expected email
(e.g., mockManager.assert.sentEmail({to: 'jbloggs@example.com', subject: /Free
member signup: Gift Receiver/}) and the corresponding one for /New paid
subscriber: Gift Receiver/) instead of mapping subjects and using sentSubjects
with assert, so replace the two sentEmail calls and their subsequent
sentSubjects/assert checks with two sentEmail calls that include the subject
matchers directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fd1dbd71-146f-4372-83d4-f3d8e055b6d8
📒 Files selected for processing (1)
ghost/core/test/e2e-api/members/signin.test.js
sagzy
force-pushed
the
gift-subscriptions/add-staff-notification
branch
from
240cdcb to
2c15f3e
Compare
sagzy
force-pushed
the
gift-subscriptions/add-staff-notification
branch
from
2c15f3e to
f0fccf8
Compare
|
mike182uk
reviewed
Apr 16, 2026
| async redeem({token, memberId}: {token: string; memberId: string}): Promise<Gift> { | ||
| return await this.deps.giftRepository.transaction(async (transacting) => { | ||
| const member = await this.deps.memberRepository.get({id: memberId}, {transacting, forUpdate: true}); | ||
| const {redeemed, member} = await this.deps.giftRepository.transaction(async (transacting) => { |
Member
There was a problem hiding this comment.
what about renaming these to redeemedGift and redeemMember so we don't have to do the ugly _?
Contributor
Author
There was a problem hiding this comment.
haha I find it cognitively lighter that way (less variables to hold in mind) but certainly don't want you to be disgusted looking at that code! I'll rename those in my follow-up PR in which I merge test files 🤝
mike182uk
approved these changes
Apr 16, 2026
franky19
pushed a commit
to franky19/Ghost
that referenced
this pull request
Apr 18, 2026
closes https://linear.app/ghost/issue/BER-3532 - when a member redeems a gift subscription, we now send a new paid member email notification, with the email body noting "Gift subscription" as source - we're not introducing a new staff notification setting for this, it's part of the existing "New paid members" setting - email copy is not finalised
minimaluminium
added a commit
that referenced
this pull request
Apr 22, 2026
#27500) no issues Follow-up design/copy pass on the gift redemption staff notification added in #27414, to bring it in line with the existing "new paid subscriber" email. - Subject is now `🎁 Paid subscription started: {name}` — mirrors the existing `💸 Paid subscription started: {name}` pattern used for regular paid signups - Tier row now includes the gift cadence (e.g. `Premium • 1 year`) instead of just the tier name - Dropped the `Source: Gift subscription` row — `Gifted by` already conveys that, and the regular paid email omits source when there's no attribution
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
2 participants
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.
closes https://linear.app/ghost/issue/BER-3532
Email copy (to be finalised)
Staff notification setting