Added gift subscription refund handling via Stripe charge.refunded webhook#27321
Merged
Added gift subscription refund handling via Stripe charge.refunded webhook#27321
charge.refunded webhook#27321Conversation
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:
WalkthroughThis PR implements gift refund functionality by introducing a refund flow across the domain, service, and webhook layers. It adds a 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 unit tests (beta)
Comment |
mike182uk
force-pushed
the
BER-3474-handle-gift-subscription-refund
branch
from
124b8e7 to
0b68cc7
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 (2)
ghost/core/test/unit/server/services/gifts/gift.test.ts (1)
137-160: DeduplicatebuildGifthelper to reduce test maintenance cost.This helper is functionally identical to the one already defined in the same file; extracting one shared helper will keep fixture changes in one place.
♻️ Proposed refactor
describe('Gift', function () { + function buildGift(overrides: Partial<ConstructorParameters<typeof Gift>[0]> = {}) { + return new Gift({ + token: 'gift-token', + buyerEmail: 'buyer@example.com', + buyerMemberId: 'buyer_member_1', + redeemerMemberId: null, + tierId: 'tier_1', + cadence: 'year', + duration: 1, + currency: 'usd', + amount: 5000, + stripeCheckoutSessionId: 'cs_123', + stripePaymentIntentId: 'pi_456', + consumesAt: null, + expiresAt: new Date('2030-01-01T00:00:00.000Z'), + status: 'purchased', + purchasedAt: new Date('2026-01-01T00:00:00.000Z'), + redeemedAt: null, + consumedAt: null, + expiredAt: null, + refundedAt: null, + ...overrides + }); + } + describe('redeemability', function () { - function buildGift(...) { ... } ... }); describe('markRefunded', function () { - function buildGift(...) { ... } ... }); });🤖 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.test.ts` around lines 137 - 160, There are two identical buildGift helpers; remove this duplicate and reuse the existing buildGift to avoid divergence—keep one canonical buildGift (signature: function buildGift(overrides: Partial<ConstructorParameters<typeof Gift>[0]> = {}) { ... }) and in the other location delete the duplicate and reference the single helper instead (or move it to a common/top-level spot in the test file), ensuring the overrides typing and default fields for Gift (token, buyerEmail, buyerMemberId, tierId, cadence, duration, currency, amount, stripeCheckoutSessionId, stripePaymentIntentId, consumesAt, expiresAt, status, purchasedAt, etc.) remain intact.ghost/core/core/server/services/gifts/gift-bookshelf-repository.ts (1)
88-108: Extract a shared Gift→row mapper to avoid drift betweencreateandupdate.The field mapping is duplicated and easy to desynchronize during future schema changes.
♻️ Suggested refactor
+ private toRow(gift: Gift): Partial<GiftRow> { + return { + token: gift.token, + buyer_email: gift.buyerEmail, + buyer_member_id: gift.buyerMemberId, + redeemer_member_id: gift.redeemerMemberId, + tier_id: gift.tierId, + cadence: gift.cadence, + duration: gift.duration, + currency: gift.currency, + amount: gift.amount, + stripe_checkout_session_id: gift.stripeCheckoutSessionId, + stripe_payment_intent_id: gift.stripePaymentIntentId, + consumes_at: gift.consumesAt, + expires_at: gift.expiresAt, + status: gift.status, + purchased_at: gift.purchasedAt, + redeemed_at: gift.redeemedAt, + consumed_at: gift.consumedAt, + expired_at: gift.expiredAt, + refunded_at: gift.refundedAt + }; + } + async create(gift: Gift) { - await this.model.add({ - ... - }); + await this.model.add(this.toRow(gift)); } async update(gift: Gift): Promise<void> { const existing = await this.model.findOne({token: gift.token}, {require: false}); if (!existing) { throw new errors.NotFoundError({message: `Gift not found: ${gift.token}`}); } const id = existing.toJSON().id; - await this.model.edit({ - ... - }, {id}); + await this.model.edit(this.toRow(gift), {id}); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/gifts/gift-bookshelf-repository.ts` around lines 88 - 108, Extract the duplicated field mapping into a single shared mapper function (e.g. mapGiftToRow) used by both create and update paths so create and update cannot drift; move the object with token, buyer_email, buyer_member_id, redeemer_member_id, tier_id, cadence, duration, currency, amount, stripe_checkout_session_id, stripe_payment_intent_id, consumes_at, expires_at, status, purchased_at, redeemed_at, consumed_at, expired_at, refunded_at (and the buyerMemberId/redeemerMemberId/stripe* keys) into that helper and call it from the code that invokes this.model.edit(...) and the create method in gift-bookshelf-repository.ts so both use the same mapping logic.
🤖 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/gifts/gift-service.ts`:
- Around line 185-186: When a gift refund is processed in gift-service.ts, add a
reconciliation path that detects if the gift has been redeemed/consumed and, if
so, revokes the granted entitlement from the recipient member and downgrades
them back to free; locate the refund handler (e.g., the method that marks a gift
refunded or processesRefund/refundGift) and after marking the gift refunded call
the members/subscription API (e.g., MemberService.revokePaidAccess or
SubscriptionService.downgradeMemberToFree) to remove the paid access, persist
the change, and emit the appropriate events/audit log; add unit tests and an e2e
test that simulates a redeemed gift being refunded and asserts the member loses
paid access and billing/subscription state is consistent.
---
Nitpick comments:
In `@ghost/core/core/server/services/gifts/gift-bookshelf-repository.ts`:
- Around line 88-108: Extract the duplicated field mapping into a single shared
mapper function (e.g. mapGiftToRow) used by both create and update paths so
create and update cannot drift; move the object with token, buyer_email,
buyer_member_id, redeemer_member_id, tier_id, cadence, duration, currency,
amount, stripe_checkout_session_id, stripe_payment_intent_id, consumes_at,
expires_at, status, purchased_at, redeemed_at, consumed_at, expired_at,
refunded_at (and the buyerMemberId/redeemerMemberId/stripe* keys) into that
helper and call it from the code that invokes this.model.edit(...) and the
create method in gift-bookshelf-repository.ts so both use the same mapping
logic.
In `@ghost/core/test/unit/server/services/gifts/gift.test.ts`:
- Around line 137-160: There are two identical buildGift helpers; remove this
duplicate and reuse the existing buildGift to avoid divergence—keep one
canonical buildGift (signature: function buildGift(overrides:
Partial<ConstructorParameters<typeof Gift>[0]> = {}) { ... }) and in the other
location delete the duplicate and reference the single helper instead (or move
it to a common/top-level spot in the test file), ensuring the overrides typing
and default fields for Gift (token, buyerEmail, buyerMemberId, tierId, cadence,
duration, currency, amount, stripeCheckoutSessionId, stripePaymentIntentId,
consumesAt, expiresAt, status, purchasedAt, etc.) remain intact.
🪄 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: d79bba21-5aea-45b8-8f40-feb293aa8281
📒 Files selected for processing (14)
ghost/core/core/server/services/gifts/gift-bookshelf-repository.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/stripe/services/webhook/charge-refunded-event-service.jsghost/core/core/server/services/stripe/stripe-service.jsghost/core/core/server/services/stripe/webhook-controller.jsghost/core/core/server/services/stripe/webhook-manager.jsghost/core/test/e2e-api/members/gifts.test.jsghost/core/test/unit/server/services/gifts/gift-bookshelf-repository.test.tsghost/core/test/unit/server/services/gifts/gift-service.test.tsghost/core/test/unit/server/services/gifts/gift.test.tsghost/core/test/unit/server/services/stripe/services/webhooks/charge-refunded-event-service.test.jsghost/core/test/unit/server/services/stripe/webhook-controller.test.js
Comment on lines
+185
to
+186
| // TODO: if the gift was already redeemed/consumed, we should also | ||
| // downgrade the recipient member back to free. |
Contributor
There was a problem hiding this comment.
Refunded redeemed gifts can keep paid access (missing downgrade path).
Line 185 leaves a functional gap: if a gift was already redeemed/consumed, refunding only the gift record can leave member entitlement inconsistent with billing state. Please implement the downgrade/reconciliation path (with tests) before release.
I can help draft the downgrade flow and the corresponding unit/e2e test cases if you want me to open a follow-up issue outline.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 185-185: Complete the task associated to this "TODO" comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ghost/core/core/server/services/gifts/gift-service.ts` around lines 185 -
186, When a gift refund is processed in gift-service.ts, add a reconciliation
path that detects if the gift has been redeemed/consumed and, if so, revokes the
granted entitlement from the recipient member and downgrades them back to free;
locate the refund handler (e.g., the method that marks a gift refunded or
processesRefund/refundGift) and after marking the gift refunded call the
members/subscription API (e.g., MemberService.revokePaidAccess or
SubscriptionService.downgradeMemberToFree) to remove the paid access, persist
the change, and emit the appropriate events/audit log; add unit tests and an e2e
test that simulates a redeemed gift being refunded and asserts the member loses
paid access and billing/subscription state is consistent.
0b68cc7 to
603df28
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ghost/core/test/e2e-api/members/gift-subscriptions.test.js (1)
346-380: Make the ignore-path tests assert the no-op.These two tests currently only prove that
sendWebhook()resolves. They won't catch a regression where the handler still returns200but incorrectly creates or mutates a gift. Please add an explicit DB assertion that no matching gift exists, or that a control gift remains unchanged after the webhook.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/e2e-api/members/gift-subscriptions.test.js` around lines 346 - 380, The tests currently only call stripeMocker.sendWebhook and DomainEvents.allSettled but don't assert the webhook was a true no-op; update each test to query the gifts table (e.g., via the same Gift model/repository used elsewhere in tests) after DomainEvents.allSettled and assert that no gift exists with payment_intent 'pi_non_gift_charge' and 'pi_sub_charge' respectively, or alternatively create a control gift before the webhook and assert its fields remain unchanged after the webhook; use the existing stripeMocker.sendWebhook and DomainEvents.allSettled calls and the project's Gift model/repository to perform the DB assertions.
🤖 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/gifts/gift.ts`:
- Around line 144-152: markRefunded() currently only sets status and refundedAt
which can leave redeemedAt/consumedAt/expiredAt set and cause checkRedeemable()
to still return 'redeemed'/'consumed'/'expired'; update markRefunded() to make
the refund transition exclusive by clearing other terminal timestamps (e.g.,
redeemedAt, consumedAt, expiredAt) and any related flags when setting status =
'refunded' and refundedAt, and/or modify checkRedeemable() to give isRefunded()
precedence (evaluate refunded before redeemed/consumed/expired) so refunded
state always wins.
---
Nitpick comments:
In `@ghost/core/test/e2e-api/members/gift-subscriptions.test.js`:
- Around line 346-380: The tests currently only call stripeMocker.sendWebhook
and DomainEvents.allSettled but don't assert the webhook was a true no-op;
update each test to query the gifts table (e.g., via the same Gift
model/repository used elsewhere in tests) after DomainEvents.allSettled and
assert that no gift exists with payment_intent 'pi_non_gift_charge' and
'pi_sub_charge' respectively, or alternatively create a control gift before the
webhook and assert its fields remain unchanged after the webhook; use the
existing stripeMocker.sendWebhook and DomainEvents.allSettled calls and the
project's Gift model/repository to perform the DB assertions.
🪄 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: c2ab977b-71f9-4710-b8e2-8136b9cbae87
📒 Files selected for processing (15)
ghost/core/core/server/services/gifts/gift-bookshelf-repository.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/stripe/services/webhook/charge-refunded-event-service.jsghost/core/core/server/services/stripe/stripe-service.jsghost/core/core/server/services/stripe/webhook-controller.jsghost/core/core/server/services/stripe/webhook-manager.jsghost/core/test/e2e-api/members/gift-subscriptions.test.jsghost/core/test/unit/server/services/gifts/gift-bookshelf-repository.test.tsghost/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.tsghost/core/test/unit/server/services/stripe/services/webhooks/charge-refunded-event-service.test.jsghost/core/test/unit/server/services/stripe/webhook-controller.test.js
✅ Files skipped from review due to trivial changes (2)
- ghost/core/test/unit/server/services/gifts/gift-bookshelf-repository.test.ts
- ghost/core/core/server/services/gifts/gift-repository.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- ghost/core/core/server/services/stripe/webhook-manager.js
- ghost/core/test/unit/server/services/gifts/gift.test.ts
- ghost/core/test/unit/server/services/gifts/gift-service.test.ts
- ghost/core/test/unit/server/services/stripe/services/webhooks/charge-refunded-event-service.test.js
- ghost/core/core/server/services/stripe/services/webhook/charge-refunded-event-service.js
- ghost/core/core/server/services/gifts/gift-service.ts
- ghost/core/core/server/services/gifts/gift-bookshelf-repository.ts
mike182uk
force-pushed
the
BER-3474-handle-gift-subscription-refund
branch
from
603df28 to
b086e55
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/gift-subscriptions.test.js`:
- Around line 346-380: Update the two tests "Ignores Stripe charge.refunded
webhook for non-gift charges" and "Ignores Stripe charge.refunded webhook for
subscription charges" to seed a purchased Gift (use the same model/setup
utilities used elsewhere in this test file to create a gift with status
"purchased" and a non-null purchased_at), call stripeMocker.sendWebhook(...) as
before, await DomainEvents.allSettled(), then reload the Gift from the DB and
assert its status and refunded_at remain unchanged (status still "purchased" and
refunded_at still null or unchanged timestamp). Reference the existing test
helpers and the stripeMocker.sendWebhook and DomainEvents.allSettled calls so
you only add the gift creation and post-webhook assertions around those calls.
In `@ghost/core/test/unit/server/services/gifts/gift.test.ts`:
- Around line 128-138: The test for the "already refunded" case only sets
refundedAt, so tighten it by seeding a fully refunded gift with both refundedAt
and status: 'refunded' when calling buildGift, then call gift.markRefunded() and
assert it returns false and that both gift.refundedAt remains the original date
and gift.status remains 'refunded' (i.e., verify markRefunded does not change
the status or refundedAt); reference the buildGift helper and the markRefunded
method and the gift.refundedAt / gift.status properties when making these
assertions.
🪄 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: 41b03316-6645-49c6-8526-bef9d42b7cff
📒 Files selected for processing (15)
ghost/core/core/server/services/gifts/gift-bookshelf-repository.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/stripe/services/webhook/charge-refunded-event-service.jsghost/core/core/server/services/stripe/stripe-service.jsghost/core/core/server/services/stripe/webhook-controller.jsghost/core/core/server/services/stripe/webhook-manager.jsghost/core/test/e2e-api/members/gift-subscriptions.test.jsghost/core/test/unit/server/services/gifts/gift-bookshelf-repository.test.tsghost/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.tsghost/core/test/unit/server/services/stripe/services/webhooks/charge-refunded-event-service.test.jsghost/core/test/unit/server/services/stripe/webhook-controller.test.js
✅ Files skipped from review due to trivial changes (4)
- ghost/core/test/unit/server/services/gifts/gift-bookshelf-repository.test.ts
- ghost/core/test/unit/server/services/gifts/utils.ts
- ghost/core/core/server/services/gifts/gift-repository.ts
- ghost/core/core/server/services/stripe/webhook-manager.js
🚧 Files skipped from review as they are similar to previous changes (7)
- ghost/core/core/server/services/stripe/stripe-service.js
- ghost/core/test/unit/server/services/stripe/services/webhooks/charge-refunded-event-service.test.js
- ghost/core/core/server/services/gifts/gift-service.ts
- ghost/core/test/unit/server/services/gifts/gift-service.test.ts
- ghost/core/core/server/services/stripe/services/webhook/charge-refunded-event-service.js
- ghost/core/core/server/services/gifts/gift.ts
- ghost/core/test/unit/server/services/stripe/webhook-controller.test.js
…webhook ref https://linear.app/ghost/issue/BER-3474 Added gift subscription refund handling via Stripe `charge.refunded` webhook that marks the gift as refunded
mike182uk
force-pushed
the
BER-3474-handle-gift-subscription-refund
branch
from
b086e55 to
43ba770
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/gifts/gift-bookshelf-repository.ts`:
- Around line 59-63: The update(gift: Gift) implementation currently writes the
entire row via this.toRow(gift) which can overwrite concurrent changes (e.g.,
redeemer_member_id, redeemed_at, consumed_at); change it to only persist
refund-related columns (e.g., refunded_at, refund_reason, refund_transaction_id,
refund_amount — use the actual refund field names used in the Gift model) by
building a partial patch object and calling this.model.edit(patch, {id}) instead
of this.toRow(gift), or alternatively implement optimistic locking around the
edit (read the current row, compare a version/timestamp field and fail/retry if
mismatched) so concurrent redeems cannot be clobbered; ensure you still locate
the row using the existing findOne(...) + id logic and keep require:true
behavior.
In `@ghost/core/core/server/services/stripe/stripe-service.js`:
- Around line 138-149: GiftService.refundGift currently only updates the gift DB
row and leaves a TODO for redeemed/consumed gifts, risking donor refunds that
mark gifts as refunded while recipients keep paid access; before wiring
ChargeRefundedEventService/WebhookController live, implement the remaining logic
inside GiftService.refundGift to detect redeemed or consumed gifts and either
revoke the recipient’s access (undo membership/subscription, remove access
flags, and emit any necessary events/notifications) or block marking the gift
refunded until cleanup completes, then ensure ChargeRefundedEventService
(constructed with get giftService() { return giftService.service; }) invokes
GiftService.refundGift so refunds processed by the webhook perform the
revoke/cleanup path.
In `@ghost/core/core/server/services/stripe/webhook-controller.js`:
- Around line 157-159: The chargeRefundedEvent handler currently forwards every
Stripe charge.refunded event to downstream processing which may call
giftService.refundGift(); modify async chargeRefundedEvent (and/or
chargeRefundedEventService.handleEvent) to first verify the refund is full by
checking charge.refunded === true (or by comparing charge.amount_refunded ===
charge.amount) and only call the refund/gift-voiding path when the refund is
full; leave partial refunds to be ignored or handled separately so gifts are not
voided incorrectly.
🪄 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: a03d410d-ca31-4a72-8e39-9ed1d53dd48e
📒 Files selected for processing (15)
ghost/core/core/server/services/gifts/gift-bookshelf-repository.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/stripe/services/webhook/charge-refunded-event-service.jsghost/core/core/server/services/stripe/stripe-service.jsghost/core/core/server/services/stripe/webhook-controller.jsghost/core/core/server/services/stripe/webhook-manager.jsghost/core/test/e2e-api/members/gift-subscriptions.test.jsghost/core/test/unit/server/services/gifts/gift-bookshelf-repository.test.tsghost/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.tsghost/core/test/unit/server/services/stripe/services/webhooks/charge-refunded-event-service.test.jsghost/core/test/unit/server/services/stripe/webhook-controller.test.js
✅ Files skipped from review due to trivial changes (3)
- ghost/core/test/unit/server/services/gifts/gift-bookshelf-repository.test.ts
- ghost/core/core/server/services/gifts/gift-repository.ts
- ghost/core/core/server/services/stripe/webhook-manager.js
🚧 Files skipped from review as they are similar to previous changes (7)
- ghost/core/core/server/services/gifts/gift.ts
- ghost/core/test/unit/server/services/stripe/webhook-controller.test.js
- ghost/core/core/server/services/gifts/gift-service.ts
- ghost/core/test/unit/server/services/gifts/utils.ts
- ghost/core/core/server/services/stripe/services/webhook/charge-refunded-event-service.js
- ghost/core/test/unit/server/services/stripe/services/webhooks/charge-refunded-event-service.test.js
- ghost/core/test/e2e-api/members/gift-subscriptions.test.js
Comment on lines
+138
to
+149
| const chargeRefundedEventService = new ChargeRefundedEventService({ | ||
| get giftService() { | ||
| return giftService.service; | ||
| } | ||
| }); | ||
|
|
||
| const webhookController = new WebhookController({ | ||
| webhookManager, | ||
| subscriptionEventService, | ||
| invoiceEventService, | ||
| checkoutSessionEventService | ||
| checkoutSessionEventService, | ||
| chargeRefundedEventService |
Contributor
There was a problem hiding this comment.
Close the redeemed/consumed downgrade gap before wiring this live.
GiftService.refundGift() still stops after updating the gift row and leaves a TODO for already redeemed/consumed gifts. With this handler enabled, a donor refund can mark the gift as refunded while leaving the recipient's paid access intact.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ghost/core/core/server/services/stripe/stripe-service.js` around lines 138 -
149, GiftService.refundGift currently only updates the gift DB row and leaves a
TODO for redeemed/consumed gifts, risking donor refunds that mark gifts as
refunded while recipients keep paid access; before wiring
ChargeRefundedEventService/WebhookController live, implement the remaining logic
inside GiftService.refundGift to detect redeemed or consumed gifts and either
revoke the recipient’s access (undo membership/subscription, remove access
flags, and emit any necessary events/notifications) or block marking the gift
refunded until cleanup completes, then ensure ChargeRefundedEventService
(constructed with get giftService() { return giftService.service; }) invokes
GiftService.refundGift so refunds processed by the webhook perform the
revoke/cleanup path.
Comment on lines
+157
to
+159
| async chargeRefundedEvent(charge) { | ||
| await this.chargeRefundedEventService.handleEvent(charge); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Stripe charge.refunded webhook: does this event fire for partial refunds, and which Charge fields (refunded, amount_refunded, amount) distinguish a full refund from a partial refund?
💡 Result:
Yes, the Stripe charge.refunded webhook event fires for partial refunds. Official Stripe documentation states: "Sent when a charge is refunded, including partial refunds. Listen to refund.created for information about the refund." To distinguish full vs. partial refunds on the Charge object in the event data: - refunded (boolean): true only if the charge has been fully refunded (total refunds equal the original charge amount). Remains false for partial refunds. - amount_refunded (integer): Total amount refunded so far (in cents). Compare to amount (original charge amount in cents): - Full refund: amount_refunded == amount (and refunded == true) - Partial refund: amount_refunded < amount (and refunded == false) The Charge also has a refunds list with details of individual refunds.
Citations:
- 1: https://docs.stripe.com/refunds
- 2: https://docs.stripe.com/api/events/types
- 3: https://docs.stripe.com/api/charges/object?api-version=2025-10-29.preview
Ensure partial refunds don't void gifts.
The Stripe charge.refunded webhook event fires for both partial and full refunds. To distinguish them, check the refunded boolean field (true only for full refunds) or compare amount_refunded to amount. The current implementation forwards all charge.refunded events to giftService.refundGift() without verifying it's a full refund, which would incorrectly void gifts for partial manual refunds. Add a check to gate this path on full refunds only, e.g., if (charge.refunded) { ... }.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ghost/core/core/server/services/stripe/webhook-controller.js` around lines
157 - 159, The chargeRefundedEvent handler currently forwards every Stripe
charge.refunded event to downstream processing which may call
giftService.refundGift(); modify async chargeRefundedEvent (and/or
chargeRefundedEventService.handleEvent) to first verify the refund is full by
checking charge.refunded === true (or by comparing charge.amount_refunded ===
charge.amount) and only call the refund/gift-voiding path when the refund is
full; leave partial refunds to be ignored or handled separately so gifts are not
voided incorrectly.
sagzy
reviewed
Apr 13, 2026
| findOne(data: Record<string, unknown>, unfilteredOptions?: unknown): Promise<BookshelfDocument<T> | null>; | ||
| edit(data: Partial<T>, unfilteredOptions?: unknown): Promise<BookshelfDocument<T>>; | ||
| findOne(data: Record<string, unknown>, options: {require: true}): Promise<BookshelfDocument<T>>; | ||
| findOne(data: Record<string, unknown>, options?: {require?: false}): Promise<BookshelfDocument<T> | null>; |
Contributor
There was a problem hiding this comment.
I read two findOne - is that a leftover?
Member
Author
There was a problem hiding this comment.
heh this is a ts overload fn signature - notice the diff in accepted args and what is returned (for this specific use if options: {require: true} is provided, we should always expect Promise<BookshelfDocument<T>>)
| return false; | ||
| } | ||
|
|
||
| const refunded = gift.markRefunded(); |
Contributor
There was a problem hiding this comment.
nitpick(naming): what do you think of gift.refund()?
Member
Author
There was a problem hiding this comment.
Yeh this is more domain centric 👍
| } | ||
|
|
||
| this.status = 'refunded'; | ||
| this.refundedAt = new Date(); |
Contributor
There was a problem hiding this comment.
Should we return a new instance instead of modifying the current one?
return new Gift({
...this,
status: 'refunded',
refundedAt: new Date()
});
Member
Author
There was a problem hiding this comment.
Hm, not sure, do we follow this pattern elsewhere? (in Ghost)
Member
Author
There was a problem hiding this comment.
Ah i see we do this in the newly added redeem method. I'll follow the same convention here 👍
| const id = existing.toJSON().id; | ||
|
|
||
| await this.model.edit(this.toRow(gift), {id}); | ||
| } |
Contributor
There was a problem hiding this comment.
FYI: I've added a .save() method in #27367
Ghost/ghost/core/core/server/services/gifts/gift-bookshelf-repository.ts
Lines 93 to 109 in d17f879
But equally happy to go for an update() method - we'll just have some cleanup to do after merging both of our PRs (no need for both save() and update())
…subscription-refund # Conflicts: # ghost/core/core/server/services/gifts/gift-bookshelf-repository.ts # ghost/core/core/server/services/gifts/gift-repository.ts # ghost/core/core/server/services/gifts/gift-service.ts # ghost/core/core/server/services/gifts/gift.ts # ghost/core/test/unit/server/services/gifts/gift-bookshelf-repository.test.ts # ghost/core/test/unit/server/services/gifts/gift-service.test.ts # ghost/core/test/unit/server/services/gifts/gift.test.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ghost/core/core/server/services/gifts/gift-service.ts (1)
161-175:⚠️ Potential issue | 🔴 CriticalProtect refund writes from redeem/refund races (lost-update risk).
refundGift()on Line 174 saves outside a transaction/lock, and the repository save path writes a full gift snapshot. A concurrent redeem can be clobbered (e.g.,redeemer_member_id,redeemed_at,consumed_at).💡 Suggested direction
async refundGift(paymentIntentId: string): Promise<boolean> { - const gift = await this.deps.giftRepository.getByPaymentIntentId(paymentIntentId); + return await this.deps.giftRepository.transaction(async (transacting) => { + const gift = await this.deps.giftRepository.getByPaymentIntentId(paymentIntentId, {transacting, forUpdate: true}); - if (!gift) { - return false; - } + if (!gift) { + return false; + } - const refunded = gift.refund(); + const refunded = gift.refund(); - if (!refunded) { - return true; - } + if (!refunded) { + return true; + } - await this.deps.giftRepository.save(refunded); + await this.deps.giftRepository.save(refunded, {transacting}); + return true; + }); }Also ensure repository persistence for this path patches only refund-related columns (or uses optimistic locking).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/gifts/gift-service.ts` around lines 161 - 175, The refundGift flow currently reads the Gift via this.deps.giftRepository.getByPaymentIntentId, calls gift.refund(), and then calls this.deps.giftRepository.save(refunded) outside of any concurrency protection, which risks lost-update when a concurrent redeem modifies redeem-related columns; fix by making the refund persistence atomic: use a DB transaction or row-level lock when loading the Gift (or employ optimistic locking/version check) and persist only refund-related fields (or call a repository method like patchRefundColumns/updateRefundFields) rather than writing a full snapshot; update refundGift to use the repository's transactional/patch API or add version checking so concurrent redeem writes cannot be clobbered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ghost/core/core/server/services/gifts/gift-service.ts`:
- Around line 161-175: The refundGift flow currently reads the Gift via
this.deps.giftRepository.getByPaymentIntentId, calls gift.refund(), and then
calls this.deps.giftRepository.save(refunded) outside of any concurrency
protection, which risks lost-update when a concurrent redeem modifies
redeem-related columns; fix by making the refund persistence atomic: use a DB
transaction or row-level lock when loading the Gift (or employ optimistic
locking/version check) and persist only refund-related fields (or call a
repository method like patchRefundColumns/updateRefundFields) rather than
writing a full snapshot; update refundGift to use the repository's
transactional/patch API or add version checking so concurrent redeem writes
cannot be clobbered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9b7f53b1-94a9-44bb-ae6a-87789d19621f
📒 Files selected for processing (6)
ghost/core/core/server/services/gifts/gift-bookshelf-repository.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/test/unit/server/services/gifts/gift-service.test.tsghost/core/test/unit/server/services/gifts/gift.test.ts
✅ Files skipped from review due to trivial changes (2)
- ghost/core/core/server/services/gifts/gift.ts
- ghost/core/core/server/services/gifts/gift-repository.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/test/unit/server/services/gifts/gift.test.ts
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 24364355012 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
2 similar comments
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 24364355012 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 24364355012 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
# Conflicts: # ghost/core/test/e2e-api/members/gift-subscriptions.test.js
… refs deleted by main
|
sagzy
approved these changes
Apr 14, 2026
franky19
pushed a commit
to franky19/Ghost
that referenced
this pull request
Apr 18, 2026
…webhook (TryGhost#27321) ref https://linear.app/ghost/issue/BER-3474 Added gift subscription refund handling via Stripe `charge.refunded` webhook that marks the gift as refunded
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.
ref https://linear.app/ghost/issue/BER-3474
Added gift subscription refund handling via Stripe
charge.refundedwebhook that marks the gift as refunded