Skipped creating offers for invalid Stripe coupons during migrations #25816
+178
−8
Conversation
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
Contributor
WalkthroughThe change wraps the Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@ghost/core/core/server/services/members/members-api/repositories/member-repository.js:
- Around line 1048-1067: The catch around
this._offersAPI.ensureOfferForStripeCoupon in member-repository.js is too broad;
narrow it to only swallow known offer-validation errors by checking the error
type/name/property (e.g., if e.name === 'ValidationError' or e.type ===
'offer_validation') and only then log and continue (set offerId absent),
otherwise rethrow or let the error propagate; if swallowing all errors is
intentional, update the existing comment to state that network/db/unexpected
errors are intentionally ignored and why.
🧹 Nitpick comments (1)
ghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js (1)
533-603: Test coverage looks good.The test verifies the core behavior: when
ensureOfferForStripeCouponfails, the subscription is still created withoffer_idset to null. The test setup and assertions are comprehensive.💡 Optional: Verify no OfferRedemptionEvent is dispatched
You could enhance the test by verifying that no
OfferRedemptionEventis dispatched when offer creation fails:+ const offerRedemptionNotifySpy = sinon.spy(); + DomainEvents.subscribe(OfferRedemptionEvent, offerRedemptionNotifySpy); + await repo.linkSubscription({ id: 'member_id_123', subscription: {...subscriptionData, discount: {coupon: {id: 'coupon_invalid'}}} }, { transacting, context: {} }); // Verify ensureOfferForStripeCoupon was called offersAPI.ensureOfferForStripeCoupon.calledOnce.should.be.true(); // Verify subscription was still created, but without an offer_id StripeCustomerSubscription.add.calledOnce.should.be.true(); assert.equal(StripeCustomerSubscription.add.firstCall.args[0].offer_id, null); + + // Verify no offer redemption event was dispatched + offerRedemptionNotifySpy.called.should.be.false();
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/core/core/server/services/members/members-api/repositories/member-repository.jsghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-08T10:26:38.700Z
Learnt from: rob-ghost
Repo: TryGhost/Ghost PR: 25791
File: ghost/core/core/server/api/endpoints/member-comment-ban.js:64-68
Timestamp: 2026-01-08T10:26:38.700Z
Learning: In the Ghost API, endpoints rely on the serialization layer to prepare frame.data[docName] as a non-empty array before query() executes. Endpoints access frame.data[docName][0] directly (e.g., frame.data.comment_bans[0], frame.data.members[0], frame.data.posts[0]) without per-endpoint validation. This pattern is common across API endpoints. When maintaining or creating endpoints, avoid duplicating validation for frame.data[docName] and ensure the serializer guarantees the shape and non-emptiness. If you add a new endpoint that uses this frame.data[docName], follow the same assumption and avoid redundant checks unless there's a documented exception.
Applied to files:
ghost/core/core/server/services/members/members-api/repositories/member-repository.jsghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js
🧬 Code graph analysis (1)
ghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js (1)
ghost/core/core/server/services/members/members-api/repositories/member-repository.js (1)
subscriptionData(1069-1099)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Lint
- GitHub Check: i18n
- GitHub Check: Cursor Bugbot
- GitHub Check: Build & Push Docker Image
ghost/core/core/server/services/members/members-api/repositories/member-repository.js
Show resolved
Hide resolved
Contributor
React E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 20823604778 -n playwright-report-react -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
ref https://linear.app/ghost/issue/BER-3162 During migrations, some Stripe coupons from other platforms may not compatible with Ghost offers. For example, a `yearly` coupon set to `repeating` (Ghost only accepts `once` or `forever` for `yearly` coupons). When we attempt to create a Ghost offer based on an invalid Stripe coupon, we can either: 1. Surface the offer validation errors during migrations, so that they can be resolved case-by-case, until all invalid Stripe coupons have been removed from paid members 2. Skip creating the offer but still create the paid member The migration team has opted for solution 2, in particular in the context of self-migrations where resolving those errors may be difficult
sagzy
force-pushed
the
skip-invalid-offers-during-migrations
branch
from
c2527d1 to
c70d803
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js (1)
534-607: LGTM! Test correctly verifies graceful handling of invalid coupon.The test properly verifies that when
ensureOfferForStripeCouponfails with a validation error, the system continues to create the subscription withoffer_idset to null. The test structure follows established patterns and includes appropriate assertions.Optional: Consider verifying domain events
The test could additionally verify that domain events are dispatched correctly when offer creation fails. Specifically:
SubscriptionCreatedEventshould still be dispatched (since the subscription is created)OfferRedemptionEventshould not be dispatched (since no offer was created)This would provide more comprehensive coverage of the failure path, similar to the existing tests on lines 343-371 and 373-419.
Example addition after line 606:
// Verify domain events are handled correctly subscriptionCreatedNotifySpy.calledOnce.should.be.true(); offerRedemptionNotifySpy.called.should.be.false();Note: This would require adding the domain event spies setup similar to lines 253-254.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/core/core/server/services/members/members-api/repositories/member-repository.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 (1)
- ghost/core/core/server/services/members/members-api/repositories/member-repository.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-08T10:26:38.700Z
Learnt from: rob-ghost
Repo: TryGhost/Ghost PR: 25791
File: ghost/core/core/server/api/endpoints/member-comment-ban.js:64-68
Timestamp: 2026-01-08T10:26:38.700Z
Learning: In the Ghost API, endpoints rely on the serialization layer to prepare frame.data[docName] as a non-empty array before query() executes. Endpoints access frame.data[docName][0] directly (e.g., frame.data.comment_bans[0], frame.data.members[0], frame.data.posts[0]) without per-endpoint validation. This pattern is common across API endpoints. When maintaining or creating endpoints, avoid duplicating validation for frame.data[docName] and ensure the serializer guarantees the shape and non-emptiness. If you add a new endpoint that uses this frame.data[docName], follow the same assumption and avoid redundant checks unless there's a documented exception.
Applied to files:
ghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js
🧬 Code graph analysis (1)
ghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js (1)
ghost/core/core/server/services/members/members-api/repositories/member-repository.js (5)
errors(2-2)require(6-6)require(8-8)require(13-13)subscriptionData(1073-1103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Cursor Bugbot
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Lint
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (1)
ghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js (1)
4-4: LGTM! Import is necessary for the new test.The import of
@tryghost/errorsis correctly added to support the new test case that creates aValidationError.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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-3162
During migrations, some Stripe coupons from other platforms may not compatible with Ghost offers. For example, a
yearlycoupon set torepeating(Ghost only acceptsonceorforeverforyearlycoupons).When we attempt to create a Ghost offer based on an incompatible Stripe coupon, we can either:
1. Surface the offer validation errors during migrations, so that they can be resolved case-by-case, until all invalid Stripe coupons have been removed from paid members
2. Skip creating the offer but still create the paid member
The migration team has opted for solution 2, in particular in the context of self-migrations where resolving those errors may be difficult.
As we discover more incompatibilities with Stripe coupons from other platforms, we can expand the list of error codes here:
Ghost/ghost/core/core/server/services/members/members-api/repositories/member-repository.js
Line 1057 in 43c9847