Fixed unwanted new free member notification when redeeming a gift#27460
Conversation
WalkthroughGift redemption API changed from an object to a positional signature: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ghost/core/core/server/services/members/members-api/repositories/member-repository.js (1)
316-350:⚠️ Potential issue | 🟠 MajorValidate explicit member statuses before persisting.
Line 316 now accepts caller-provided
data.statuswithout validation, and Lines 344-350 skip the status inference logic whenever it is present. An invalid or misspelled status value (e.g.,'ompec'instead of'comped') will be persisted without error. This bypasses repository invariants and affects automation gating at line 373, which checksmemberData.status === 'free'to trigger welcome emails.While member-bread-service.js validates
compedandstripe_customer_id, it does not validate thestatusfield before callingmemberRepository.create(). Add explicit validation to ensure only allowed values ('free','paid','comped','gift') are accepted.🛡️ Proposed guard
+const VALID_MEMBER_STATUSES = new Set(['free', 'paid', 'comped', 'gift']); + ... const memberData = _.pick(data, ['email', 'name', 'note', 'subscribed', 'geolocation', 'created_at', 'products', 'newsletters', 'email_disabled', 'status']); + + if (memberData.status && !VALID_MEMBER_STATUSES.has(memberData.status)) { + throw new errors.ValidationError({ + message: 'Invalid member status', + property: 'status' + }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/members/members-api/repositories/member-repository.js` around lines 316 - 350, The member creation code currently accepts caller-provided memberData.status without validation which allows invalid values to be persisted; update the create flow in member-repository (the block that sets memberData.transient_id and evaluates memberData.status) to validate any explicit memberData.status against the allowed set ['free','paid','comped','gift'] and throw a ValidationError (similar to the existing errors.ValidationError usage) if the value is not one of those; retain the existing inference logic (setting status when memberData.status is not present) so callers who omit status still get the correct default, and ensure downstream checks (e.g., where memberData.status === 'free' triggers welcome emails) can rely on this strict set.
🤖 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/unit/server/services/members/members-api/repositories/member-repository.test.js`:
- Around line 2097-2105: The test "respects the status passed in data instead of
overriding it" currently omits products so it won't catch regressions that infer
"comped" from products; update the call to buildRepo().create in that test to
include a non-empty products array (e.g., products: ['monthly'] or similar)
alongside email/name/status:'gift', then assert (using memberAdd and
memberAdd.firstCall.args[0].status) that the status remains 'gift' to ensure
explicit status is not overridden by product-based inference.
---
Outside diff comments:
In
`@ghost/core/core/server/services/members/members-api/repositories/member-repository.js`:
- Around line 316-350: The member creation code currently accepts
caller-provided memberData.status without validation which allows invalid values
to be persisted; update the create flow in member-repository (the block that
sets memberData.transient_id and evaluates memberData.status) to validate any
explicit memberData.status against the allowed set
['free','paid','comped','gift'] and throw a ValidationError (similar to the
existing errors.ValidationError usage) if the value is not one of those; retain
the existing inference logic (setting status when memberData.status is not
present) so callers who omit status still get the correct default, and ensure
downstream checks (e.g., where memberData.status === 'free' triggers welcome
emails) can rely on this strict set.
🪄 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: 8047ba10-0139-43a8-b615-f889a3873913
📒 Files selected for processing (9)
ghost/core/core/server/services/gifts/gift-controller.tsghost/core/core/server/services/gifts/gift-service.tsghost/core/core/server/services/members/members-api/members-api.jsghost/core/core/server/services/members/members-api/repositories/member-repository.jsghost/core/test/e2e-api/members/gift-subscriptions.test.jsghost/core/test/unit/server/services/gifts/gift-controller.test.tsghost/core/test/unit/server/services/gifts/gift-service.test.tsghost/core/test/unit/server/services/members/members-api/members-api.test.jsghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js
sagzy
force-pushed
the
gift-subscriptions/do-not-send-new-free-member-notification
branch
from
d09caf3 to
b1942c2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/core/server/services/members/members-api/members-api.js (1)
280-282: Consider applying the same transactional pattern to the existing-member redeem path.The new-member path now wraps redemption in
Member.transaction, but the existing-member branch at line 281 still callsgiftService.service.redeem(giftToken, member.id)without atransactingoption. For an existing member this doesn't cause the "free signup" notification, so it isn't a blocker for this PR's objective — but for consistency and atomicity of the redeem operation itself (subscription creation + member status update insideredeem), you may want to wrap this call in a transaction as well in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/members/members-api/members-api.js` around lines 280 - 282, The existing-member branch currently calls giftService.service.redeem(giftToken, member.id) outside a transaction; wrap this call in Member.transaction similarly to the new-member path and pass the transaction object into the redeem call (e.g., via a transacting option) so the subscription creation and member updates inside giftService.service.redeem run atomically; locate the call sites referencing giftService.service.redeem, Member.transaction, giftToken, giftSubscriptionsEnabled and ensure redeem accepts/receives the transacting option and uses it for DB operations.
🤖 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/core/server/services/members/members-api/members-api.js`:
- Around line 280-282: The existing-member branch currently calls
giftService.service.redeem(giftToken, member.id) outside a transaction; wrap
this call in Member.transaction similarly to the new-member path and pass the
transaction object into the redeem call (e.g., via a transacting option) so the
subscription creation and member updates inside giftService.service.redeem run
atomically; locate the call sites referencing giftService.service.redeem,
Member.transaction, giftToken, giftSubscriptionsEnabled and ensure redeem
accepts/receives the transacting option and uses it for DB operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e8571bb2-5431-485f-b49d-dac4266096ed
📒 Files selected for processing (9)
ghost/core/core/server/services/gifts/gift-controller.tsghost/core/core/server/services/gifts/gift-service.tsghost/core/core/server/services/members/members-api/members-api.jsghost/core/core/server/services/members/members-api/repositories/member-repository.jsghost/core/test/e2e-api/members/gift-subscriptions.test.jsghost/core/test/unit/server/services/gifts/gift-controller.test.tsghost/core/test/unit/server/services/gifts/gift-service.test.tsghost/core/test/unit/server/services/members/members-api/members-api.test.jsghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js
✅ Files skipped from review due to trivial changes (2)
- ghost/core/test/unit/server/services/members/members-api/members-api.test.js
- ghost/core/test/e2e-api/members/gift-subscriptions.test.js
🚧 Files skipped from review as they are similar to previous changes (6)
- ghost/core/core/server/services/gifts/gift-controller.ts
- ghost/core/core/server/services/gifts/gift-service.ts
- ghost/core/test/unit/server/services/gifts/gift-service.test.ts
- ghost/core/test/unit/server/services/gifts/gift-controller.test.ts
- ghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js
- ghost/core/core/server/services/members/members-api/repositories/member-repository.js
sagzy
force-pushed
the
gift-subscriptions/do-not-send-new-free-member-notification
branch
from
b1942c2 to
f13db08
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/test/unit/server/services/gifts/gift-service.test.ts (1)
1047-1074: Consider strengthening thenewMember: trueassertion to explicitly verifyassertRedeemable(gift, null)is used.The test currently stubs
memberRepository.getto return a member with status'gift', which also happens to satisfycheckRedeemablewithoutnewMemberbeing set (only'paid'is explicitly rejected). That means this test wouldn't fail ifnewMemberwere quietly ignored and the member's status were used instead.To make the test actually prove the
newMemberbranch ingift-service.ts(Lines 262-266), consider either:
- Spying on
service.assertRedeemableand asserting it's called withnullas the second argument, or- Using a member status that would be rejected without the
newMemberoverride (though none of the current statuses behave that way, so a spy is probably cleaner).Not blocking — the positive-path assertions still catch regressions in the overall flow.
♻️ Example: spy on assertRedeemable to pin the branch
it('allows a newly created gift member to redeem when newMember is true', async function () { const gift = buildGift(); giftRepository.getByToken.resolves(gift); memberRepository.get.resolves({ id: 'member_1', get: sinon.stub().withArgs('status').returns('gift') }); const service = createService(); + const assertRedeemableSpy = sinon.spy(service, 'assertRedeemable'); const redeemed = await service.redeem('gift-token', 'member_1', {newMember: true}); + sinon.assert.calledOnceWithExactly(assertRedeemableSpy, gift, null); sinon.assert.calledOnce(giftRepository.transaction);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/gifts/gift-service.test.ts` around lines 1047 - 1074, The test should assert that the newMember branch is exercised by verifying assertRedeemable is called with null as the member argument; spy on the service.assertRedeemable method (or sinon.spy on the service instance) prior to calling service.redeem('gift-token', 'member_1', {newMember: true}) and assert that assertRedeemable(gift, null) was invoked, so the test ensures the code path in gift-service.ts (assertRedeemable used for newMember) is actually taken rather than relying on the stubbed member status.
🤖 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/unit/server/services/gifts/gift-service.test.ts`:
- Around line 1047-1074: The test should assert that the newMember branch is
exercised by verifying assertRedeemable is called with null as the member
argument; spy on the service.assertRedeemable method (or sinon.spy on the
service instance) prior to calling service.redeem('gift-token', 'member_1',
{newMember: true}) and assert that assertRedeemable(gift, null) was invoked, so
the test ensures the code path in gift-service.ts (assertRedeemable used for
newMember) is actually taken rather than relying on the stubbed member status.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 13991906-d317-49bd-864b-be0331a99173
📒 Files selected for processing (9)
ghost/core/core/server/services/gifts/gift-controller.tsghost/core/core/server/services/gifts/gift-service.tsghost/core/core/server/services/members/members-api/members-api.jsghost/core/core/server/services/members/members-api/repositories/member-repository.jsghost/core/test/e2e-api/members/gift-subscriptions.test.jsghost/core/test/unit/server/services/gifts/gift-controller.test.tsghost/core/test/unit/server/services/gifts/gift-service.test.tsghost/core/test/unit/server/services/members/members-api/members-api.test.jsghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js
🚧 Files skipped from review as they are similar to previous changes (5)
- ghost/core/test/unit/server/services/gifts/gift-controller.test.ts
- ghost/core/core/server/services/gifts/gift-controller.ts
- ghost/core/test/e2e-api/members/gift-subscriptions.test.js
- ghost/core/test/unit/server/services/members/members-api/members-api.test.js
- ghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js
| await buildRepo().create({ | ||
| email: 'test@example.com', | ||
| name: 'Test Member', | ||
| status: 'gift' |
There was a problem hiding this comment.
Should we also add products here to validate the line:
if (memberData.products && memberData.products.length === 1) {
There was a problem hiding this comment.
Added new unit test to cover that in 3e1a9c2
| logging.error('Failed to notify staff of gift redemption', err); | ||
| if (options.transacting) { | ||
| // Only notify once the transaction has finished | ||
| options.transacting.executionPromise.then(notify, () => {}); |
There was a problem hiding this comment.
Did we want to catch and log or something here?
There was a problem hiding this comment.
I had a warning log that said "Skipped staff notification on gift redemption (transaction rollback)" initially, but I'm not sure this is valuable, as this is sort of expected behaviour (transaction failed, don't continue)?
The error log from the caller are more valuable: transaction failed because of x
What do you think?
There was a problem hiding this comment.
Hm, yeh i guess there probably isn't much value to this. We also try/catch in the notify fn anyway so nothing should be thrown?
| } | ||
|
|
||
| const memberData = _.pick(data, ['email', 'name', 'note', 'subscribed', 'geolocation', 'created_at', 'products', 'newsletters', 'email_disabled']); | ||
| const memberData = _.pick(data, ['email', 'name', 'note', 'subscribed', 'geolocation', 'created_at', 'products', 'newsletters', 'email_disabled', 'status']); |
There was a problem hiding this comment.
status will get validated at the model / db level right? So no issue if we pass an invalid value for status?
There was a problem hiding this comment.
An invalid status will be rejected at the model level yes. But I'm thinking we should add an extra validation in the create() / update() method and reject invalid status values with a ValidationError, so that we can test this properly and return a more readable error
closes https://linear.app/ghost/issue/BER-3476 - when a new member redeemed a gift subscription, they were briefly created with status 'free' and only flipped to 'gift' by the subsequent redeem call, which triggered the "new free signup" staff notification - now fixed - they would also receive the free member welcome email - now fixed - wrapped member creation and gift redemption in a transaction to ensure atomicity
sagzy
force-pushed
the
gift-subscriptions/do-not-send-new-free-member-notification
branch
from
daf3c02 to
3e1a9c2
Compare
|
❌ The last analysis has failed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ghost/core/test/unit/server/services/gifts/gift-service.test.ts (1)
1017-1045: Make the external-transaction test prove notification deferral.Using
Promise.resolve()lets the notification run immediately, so this only proves the repo calls use the external transaction. A deferredexecutionPromisewould catch regressions that notify before commit/rollback is known.🧪 Proposed test tightening
- memberRepository.get.resolves({ - id: 'member_1', - get: sinon.stub().withArgs('status').returns('free') - }); + const memberGet = sinon.stub(); + memberGet.withArgs('status').returns('free'); + memberGet.withArgs('email').returns('member@example.com'); + memberGet.withArgs('name').returns('Member Name'); + memberRepository.get.resolves({ + id: 'member_1', + get: memberGet + }); const service = createService(); - const externalTrx = {executionPromise: Promise.resolve()}; + let resolveExecutionPromise!: (value?: unknown) => void; + const executionPromise = new Promise<unknown>((resolve) => { + resolveExecutionPromise = resolve; + }); + const externalTrx = {executionPromise}; const redeemed = await service.redeem('gift-token', 'member_1', {transacting: externalTrx}); sinon.assert.notCalled(giftRepository.transaction); @@ }); sinon.assert.calledOnceWithExactly(giftRepository.update, redeemed, {transacting: externalTrx}); + sinon.assert.notCalled(staffServiceEmails.notifyGiftSubscriptionStarted); + + resolveExecutionPromise(); + await executionPromise; + await new Promise(resolve => setImmediate(resolve)); + + sinon.assert.calledOnce(staffServiceEmails.notifyGiftSubscriptionStarted); assert.equal(redeemed.status, 'redeemed');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/gifts/gift-service.test.ts` around lines 1017 - 1045, Replace the immediate Promise.resolve() used for externalTrx.executionPromise with a deferred promise so the test can assert notifications are not delivered until the transaction's executionPromise settles: create a deferred object (e.g., let resolveExecution; const deferred = new Promise(res => { resolveExecution = res; }); set externalTrx = {executionPromise: deferred}; call service.redeem('gift-token', ...) and assert repo methods were called but any notification stubs/spies were not called yet, then call resolveExecution() and await the redeem result (or await externalTrx.executionPromise) and finally assert the notification was called and redeemed.status is 'redeemed'; keep references to externalTrx, executionPromise and service.redeem when updating the test.
🤖 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/members-api/repositories/member-repository.js`:
- Around line 346-359: Change validation and defaulting to check for the
presence of the status key instead of truthiness: in member-repository.js
replace the truthy checks on memberData.status with presence checks (e.g.
Object.prototype.hasOwnProperty.call(memberData, 'status') or 'status' in
memberData) so that update() will validate and persist explicit values like ''
or 'gift' (if present, throw ValidationError when memberData.status is not in
MEMBER_STATUSES); and only apply the comped/free default when status is not
provided at all (i.e. when the status property is absent). Apply the same
presence-based logic to the analogous block referenced around 536-600.
---
Nitpick comments:
In `@ghost/core/test/unit/server/services/gifts/gift-service.test.ts`:
- Around line 1017-1045: Replace the immediate Promise.resolve() used for
externalTrx.executionPromise with a deferred promise so the test can assert
notifications are not delivered until the transaction's executionPromise
settles: create a deferred object (e.g., let resolveExecution; const deferred =
new Promise(res => { resolveExecution = res; }); set externalTrx =
{executionPromise: deferred}; call service.redeem('gift-token', ...) and assert
repo methods were called but any notification stubs/spies were not called yet,
then call resolveExecution() and await the redeem result (or await
externalTrx.executionPromise) and finally assert the notification was called and
redeemed.status is 'redeemed'; keep references to externalTrx, executionPromise
and service.redeem when updating the test.
🪄 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: 4c2157e1-0469-4675-bfc6-c047f5549e5e
📒 Files selected for processing (9)
ghost/core/core/server/services/gifts/gift-controller.tsghost/core/core/server/services/gifts/gift-service.tsghost/core/core/server/services/members/members-api/members-api.jsghost/core/core/server/services/members/members-api/repositories/member-repository.jsghost/core/test/e2e-api/members/gift-subscriptions.test.jsghost/core/test/unit/server/services/gifts/gift-controller.test.tsghost/core/test/unit/server/services/gifts/gift-service.test.tsghost/core/test/unit/server/services/members/members-api/members-api.test.jsghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js
🚧 Files skipped from review as they are similar to previous changes (4)
- ghost/core/core/server/services/gifts/gift-controller.ts
- ghost/core/test/e2e-api/members/gift-subscriptions.test.js
- ghost/core/test/unit/server/services/members/members-api/members-api.test.js
- ghost/core/core/server/services/members/members-api/members-api.js
| if (memberData.status && !MEMBER_STATUSES.includes(memberData.status)) { | ||
| throw new errors.ValidationError({ | ||
| message: tpl(messages.invalidMemberStatus, {statuses: MEMBER_STATUSES.join(', ')}), | ||
| property: 'status' | ||
| }); | ||
| } | ||
|
|
||
| if (memberData.products && memberData.products.length === 1) { | ||
| memberStatusData.status = 'comped'; | ||
| if (!memberData.status) { | ||
| if (memberData.products && memberData.products.length === 1) { | ||
| memberData.status = 'comped'; | ||
| } else { | ||
| memberData.status = 'free'; | ||
| } | ||
| } |
There was a problem hiding this comment.
Persist explicit status updates and validate by presence, not truthiness.
update() now validates data.status, but memberData still omits status, so repo.update({status: 'gift'}, ...) is silently ignored unless a product-change branch also sets memberStatusData.status. Also, status: '' bypasses validation in both create/update and is treated as omitted. Please include status in the update payload and validate when the field is present.
🐛 Proposed fix
+ const hasMemberStatus = Object.prototype.hasOwnProperty.call(memberData, 'status');
- if (memberData.status && !MEMBER_STATUSES.includes(memberData.status)) {
+ if (hasMemberStatus && !MEMBER_STATUSES.includes(memberData.status)) {
throw new errors.ValidationError({
message: tpl(messages.invalidMemberStatus, {statuses: MEMBER_STATUSES.join(', ')}),
property: 'status'
});
}
- if (!memberData.status) {
+ if (!hasMemberStatus) {
if (memberData.products && memberData.products.length === 1) {
memberData.status = 'comped';
} else {
memberData.status = 'free';
}
@@
'expertise',
'email_disabled',
- 'transient_id'
+ 'transient_id',
+ 'status'
]);
@@
- if (data.status && !MEMBER_STATUSES.includes(data.status)) {
+ if (Object.prototype.hasOwnProperty.call(data, 'status') && !MEMBER_STATUSES.includes(data.status)) {
throw new errors.ValidationError({
message: tpl(messages.invalidMemberStatus, {statuses: MEMBER_STATUSES.join(', ')}),
property: 'status'
});
}Also applies to: 536-600
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ghost/core/core/server/services/members/members-api/repositories/member-repository.js`
around lines 346 - 359, Change validation and defaulting to check for the
presence of the status key instead of truthiness: in member-repository.js
replace the truthy checks on memberData.status with presence checks (e.g.
Object.prototype.hasOwnProperty.call(memberData, 'status') or 'status' in
memberData) so that update() will validate and persist explicit values like ''
or 'gift' (if present, throw ValidationError when memberData.status is not in
MEMBER_STATUSES); and only apply the comped/free default when status is not
provided at all (i.e. when the status property is absent). Apply the same
presence-based logic to the analogous block referenced around 536-600.
2 participants
closes https://linear.app/ghost/issue/BER-3476