🐛 Fixed missing member discount data after migrations #25720
Conversation
WalkthroughThis PR adds Stripe coupon support and offer creation flows: introduces a StripeCoupon value object with validation; extends Offer to include stripe_coupon_id and a factory createFromStripeCoupon; adds OffersAPI.ensureOfferForStripeCoupon with transactional and race-condition handling; updates OfferBookshelfRepository to persist stripe_coupon_id; refactors MemberRepository to accept offersAPI and to call ensureOfferForStripeCoupon when creating offers from subscription metadata; and adds/updates tests (unit and e2e) to cover the new behavior. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2025-03-13T09:02:50.102ZApplied to files:
📚 Learning: 2025-03-13T09:02:50.102ZApplied to files:
📚 Learning: 2025-08-11T19:39:00.428ZApplied to files:
⏰ 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)
🔇 Additional comments (2)
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 |
2cdd204 to
74e6fff
Compare
| offerId = ensuredOfferId; | ||
| } | ||
| } else if (offerId) { | ||
| offer = await this._offerRepository.getById(offerId, {transacting: options.transacting}); |
There was a problem hiding this comment.
Note: we don't actually use offer in the rest of the linkSubscription method, so this code wasn't actually being used
74e6fff to
865b74b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ghost/core/core/server/services/offers/service.js (1)
38-38: Property name should match the assignment at line 29.This property is declared as
offerImportServicebut line 29 assigns tothis.importService, creating an inconsistency. See comment on lines 29-32 for the fix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
ghost/core/core/server/services/members/api.js(1 hunks)ghost/core/core/server/services/members/members-api/members-api.js(3 hunks)ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js(7 hunks)ghost/core/core/server/services/offers/OfferBookshelfRepository.js(2 hunks)ghost/core/core/server/services/offers/OfferImportService.js(1 hunks)ghost/core/core/server/services/offers/application/OffersAPI.js(2 hunks)ghost/core/core/server/services/offers/domain/models/Offer.js(5 hunks)ghost/core/core/server/services/offers/service.js(2 hunks)ghost/core/test/e2e-api/members/webhooks.test.js(6 hunks)ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js(2 hunks)ghost/core/test/unit/server/services/members/members-api/services/OfferImportService.test.js(1 hunks)ghost/core/test/unit/server/services/offers/application/OffersAPI.test.js(1 hunks)ghost/core/test/unit/server/services/offers/domain/models/Offer.test.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
ghost/core/core/server/services/offers/OfferImportService.js (2)
ghost/core/core/server/services/offers/service.js (1)
OfferImportService(7-7)ghost/core/test/unit/server/services/members/members-api/services/OfferImportService.test.js (11)
OfferImportService(3-3)offersAPI(14-16)offersAPI(33-35)offersAPI(56-58)offersAPI(75-77)offersAPI(95-97)offerRepository(11-13)offerRepository(30-32)offerRepository(53-55)offerRepository(72-74)offerRepository(92-94)
ghost/core/core/server/services/offers/application/OffersAPI.js (1)
ghost/core/core/server/services/offers/OfferBookshelfRepository.js (1)
Offer(4-4)
ghost/core/core/server/services/offers/domain/models/Offer.js (2)
ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js (1)
stripeCouponId(1041-1041)ghost/core/test/e2e-api/members/webhooks.test.js (1)
stripeCouponId(1692-1692)
ghost/core/core/server/services/members/members-api/members-api.js (2)
ghost/core/test/unit/server/services/members/members-api/services/OfferImportService.test.js (5)
offersAPI(14-16)offersAPI(33-35)offersAPI(56-58)offersAPI(75-77)offersAPI(95-97)ghost/core/test/unit/server/services/members/members-api/controllers/RouterController.test.js (1)
offersAPI(10-10)
ghost/core/test/unit/server/services/offers/domain/models/Offer.test.js (3)
ghost/core/core/server/services/offers/OfferBookshelfRepository.js (1)
Offer(4-4)ghost/core/core/server/services/offers/domain/models/Offer.js (1)
ObjectID(2-2)ghost/core/test/unit/server/services/offers/application/OffersAPI.test.js (1)
ObjectID(3-3)
ghost/core/core/server/services/offers/OfferBookshelfRepository.js (1)
ghost/core/test/unit/server/services/offers/domain/models/Offer.test.js (11)
offer(24-37)offer(45-58)offer(64-77)offer(163-163)offer(209-209)offer(230-230)offer(298-298)offer(322-322)offer(352-352)offer(381-381)offer(415-415)
ghost/core/core/server/services/offers/service.js (1)
ghost/core/test/unit/server/services/members/members-api/services/OfferImportService.test.js (1)
OfferImportService(3-3)
ghost/core/test/e2e-api/members/webhooks.test.js (2)
ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js (7)
stripeCouponId(1041-1041)member(347-347)member(679-682)member(764-764)member(1450-1452)member(1486-1486)member(1530-1530)ghost/core/core/server/services/members/api.js (1)
models(7-7)
ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js (1)
ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js (1)
subscriptionData(1063-1093)
🔇 Additional comments (24)
ghost/core/test/unit/server/services/offers/domain/models/Offer.test.js (1)
44-61: LGTM: Clean test for stripe_coupon_id storage.The test properly validates that the Offer model stores and exposes the
stripe_coupon_idfield via thestripeCouponIdgetter. The test structure follows the existing pattern and provides adequate coverage for this new functionality.ghost/core/core/server/services/offers/OfferBookshelfRepository.js (2)
124-124: LGTM: Proper mapping of stripe_coupon_id.The mapping correctly passes
stripe_coupon_idfrom the JSON model to the Offer domain model, consistent with other field mappings in this method.
220-222: LGTM: Safe conditional persistence of stripe_coupon_id.The conditional check prevents setting
undefinedvalues in the database, which is a good defensive practice. The pattern is consistent with how optional fields should be handled.ghost/core/core/server/services/offers/application/OffersAPI.js (2)
12-12: LGTM: Documentation clarification.The JSDoc correctly specifies the concrete repository implementation type being used, improving code documentation accuracy.
45-59: LGTM: Clean transaction handling refactor.The refactoring properly supports both transactional and non-transactional execution paths. This enables callers to provide an existing transaction for atomicity in multi-step operations (e.g., creating offers as part of member import flows). The pattern is idiomatic and maintains backward compatibility.
ghost/core/test/unit/server/services/members/members-api/services/OfferImportService.test.js (5)
10-27: LGTM: Test correctly validates offer reuse.The test properly verifies that when an offer already exists for a Stripe coupon, the service returns the existing offer ID without creating a duplicate. The assertion confirms
createOfferis not called.
29-50: LGTM: Transaction propagation correctly tested.The test validates that when creating a new offer, the service properly forwards the
transactingcontext to thecreateOffercall. This ensures atomicity in multi-step operations.
52-69: LGTM: Proper handling of invalid coupon data.The test confirms that when the mapper rejects a coupon (missing required discount fields), the service returns
nullwithout attempting to create an offer. This defensive behavior prevents invalid data from being persisted.
71-89: LGTM: Currency propagation validated.The test ensures that
amount_offcoupons correctly propagate thecurrencyfield to the created offer, which is essential for fixed-amount discount calculations.
91-109: LGTM: Unsupported coupon type handled correctly.The test validates that yearly repeating coupons are rejected (as mentioned in the PR description), returning
nullwithout creating an offer. This aligns with the stated limitation that repeating discounts on yearly subscriptions are not supported.ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js (2)
464-531: LGTM: Comprehensive integration test for coupon-based offer creation.The test thoroughly validates the integration between MemberRepository and OfferImportService:
- Properly mocks all dependencies including the coupon data structure
- Verifies that
ensureOfferForCouponis called with the correcttransactingcontext- Asserts that the created offer ID is associated with the subscription
- Covers the full flow of creating an offer from a Stripe coupon during subscription linking
626-626: Minor formatting improvement.The added blank line improves readability by separating the test setup from assertions.
ghost/core/test/unit/server/services/offers/application/OffersAPI.test.js (2)
28-45: LGTM: Transaction reuse correctly tested.The test validates that when a transaction is provided in options,
createOfferuses it directly without opening a new transaction. The stub setup that rejects oncreateTransactionensures this behavior is enforced.
47-61: LGTM: New transaction creation validated.The test confirms that when no transaction is provided,
createOfferproperly creates a new transaction and propagates it through the save operation. The callback pattern correctly simulates transaction creation.ghost/core/core/server/services/members/api.js (1)
247-247: No action required. The referenceoffersService.importServicein members/api.js is correct—it references the property that is actually assigned in offers/service.js (line 28). The unused declarationofferImportService: nullin offers/service.js is a separate matter and does not impact this code.Likely an incorrect or invalid review comment.
ghost/core/core/server/services/offers/domain/models/Offer.js (1)
31-31: LGTM! Clean implementation of stripeCouponId field.The additions integrate well with the existing Offer model:
- Type definitions properly declare the field as nullable
- Getter follows existing patterns
- Nullish coalescing ensures proper null default
- Field is correctly passed through the constructor chain
Also applies to: 52-52, 197-199, 299-299, 364-364
ghost/core/test/e2e-api/members/webhooks.test.js (2)
35-38: LGTM! Helper follows existing patterns.The new
getOfferByStripeCouponhelper is consistent with other test helpers in the file and provides a clean way to verify offer creation by coupon ID.
1687-1831: Solid test coverage for coupon-to-offer flow.The test effectively validates the end-to-end functionality:
- Verifies no offer exists before webhook
- Confirms offer is created from coupon data
- Validates offer is linked to subscription
- Checks correct MRR calculation with discount
One minor consideration: the hardcoded
stripeCouponId = 'stripe-coupon-id'at line 1692 should be sufficiently unique for test isolation, but if tests are extended in the future, consider using a dynamic ID (e.g., with timestamp or UUID) to prevent potential collisions.ghost/core/core/server/services/members/members-api/members-api.js (1)
72-72: LGTM! Clean dependency injection.The
offerImportServiceis properly wired through the members API module and passed toMemberRepositoryalongside the existingofferRepository, following established dependency injection patterns.Also applies to: 115-116
ghost/core/core/server/services/offers/OfferImportService.js (3)
24-40: LGTM! Name generation provides clear user-facing descriptions.The method generates intuitive offer names from coupon data:
- Handles both percentage and fixed amount discounts
- Properly converts cents to currency units for display (line 35)
- Includes duration information (forever, once, or N months)
- Has a sensible fallback for edge cases
112-141: LGTM! Robust implementation with proper error handling.The
ensureOfferForCouponmethod demonstrates good practices:
- Validates inputs before processing
- Checks for existing offers to avoid duplicates (line 122)
- Properly propagates
transactingcontext for database consistency- Gracefully handles errors by logging and returning null rather than throwing, appropriate for a feature that shouldn't break member creation
The defensive checks at lines 117-120 ensure the service degrades gracefully if dependencies are unavailable.
47-100: The code correctly handles the mapping logic, but could benefit from explicit currency validation.At line 62, the code takes
coupon.currencywithout validating it matches the subscription's currency. While Stripe's API likely prevents applying a coupon with a mismatched currency to a subscription (a payment processing requirement), this code relies on that implicit validation rather than explicitly checking it. SincesubscriptionPriceData.currencyis available in the calling context (MemberRepository line 1027), you could pass it here for explicit validation if desired—but this is optional given Stripe's own constraints.Other aspects are correct:
- Line 61: Amount kept in cents (not divided by 100) is correct for storage
- Lines 71-73: Correctly rejects unsupported yearly + repeating combination
- Line 88: Status hardcoded to 'archived' is intentional per design
ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js (2)
55-55: LGTM! Dependency properly injected.The
offerImportServicedependency follows the established pattern for dependency injection in the repository, including JSDoc documentation, constructor parameter, and instance storage.Also applies to: 77-77, 97-97
1041-1061: Clean integration of coupon-to-offer mapping.The implementation correctly handles the new coupon-based offer creation:
- Line 1041: Safe extraction of coupon ID with optional chaining
- Lines 1046-1061: Well-structured conditional that only creates offers when appropriate (coupon exists, no explicit offer provided, and product context available)
- Line 1055: Proper propagation of
transactingcontext ensures database consistency- Lines 1058-1060: Safe handling of null return from
ensureOfferForCouponThe requirement for
ghostProductat line 1046 makes sense—without product context, we can't create a properly scoped offer. This ensures offers are always associated with a valid tier.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ghost/core/test/e2e-api/members/webhooks.test.js (1)
1687-1831: End‑to‑end coupon→offer test is well‑constructedThe new
Supports creating an offer from a Stripe coupontest correctly:
- Ensures no pre‑existing offer for the coupon id
- Drives the
checkout.session.completedwebhook path- Asserts that an offer is created and linked via
offer_idand exposed onmember.subscriptions[0].offer.If you want even stronger coverage, you could also assert the subscription’s
mrrin theassertSubscriptioncall, but the current checks are sufficient given the existing discount tests.ghost/core/test/e2e-api/admin/members.test.js (1)
1742-1867: Coupon‑driven offer creation e2e test covers the right behaviorsThe new test:
- Ensures no existing offer for
stripeCouponId- Seeds Stripe mocks with a subscription carrying a
amount_offcoupon- Verifies the created offer’s key fields (name, discount type/amount, duration, archived status)
- Confirms the subscription row has
offer_idset and the API member payload exposes the same offer.This gives solid end‑to‑end coverage of the new import flow. One optional improvement would be to also assert the subscription’s
mrrinassertSubscriptionfor completeness, but it’s not strictly necessary given other tests.ghost/core/core/server/services/offers/OffersImportService.js (1)
3-95: Coupon→offer mapping is consistent with offer semantics
_generateNameFromCouponand_mapCouponToOfferDatasensibly translate Stripe coupon fields into offer data:
- Supports
percent_offandamount_off(with currency required for the latter)- Maps Stripe
duration/duration_in_monthsto offer duration fields- Derives a readable name (e.g.
USD 5 off once) and uses it for both name and display_title- Marks offers as
archivedby default and attachesstripe_coupon_id.One small optional hardening would be to guard against missing
tier/tier.idand log+returnnullinstead of throwing if a caller passes an invalid tier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/admin/__snapshots__/members.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
ghost/core/core/server/services/members/api.js(1 hunks)ghost/core/core/server/services/members/members-api/members-api.js(3 hunks)ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js(7 hunks)ghost/core/core/server/services/offers/OfferBookshelfRepository.js(2 hunks)ghost/core/core/server/services/offers/OffersImportService.js(1 hunks)ghost/core/core/server/services/offers/application/OffersAPI.js(2 hunks)ghost/core/core/server/services/offers/domain/models/Offer.js(5 hunks)ghost/core/core/server/services/offers/service.js(2 hunks)ghost/core/test/e2e-api/admin/members.test.js(1 hunks)ghost/core/test/e2e-api/members/webhooks.test.js(6 hunks)ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js(2 hunks)ghost/core/test/unit/server/services/members/members-api/services/OffersImportService.test.js(1 hunks)ghost/core/test/unit/server/services/offers/application/OffersAPI.test.js(1 hunks)ghost/core/test/unit/server/services/offers/domain/models/Offer.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- ghost/core/test/unit/server/services/offers/domain/models/Offer.test.js
- ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js
- ghost/core/core/server/services/offers/service.js
- ghost/core/test/unit/server/services/offers/application/OffersAPI.test.js
- ghost/core/core/server/services/offers/OfferBookshelfRepository.js
🧰 Additional context used
🧬 Code graph analysis (4)
ghost/core/test/e2e-api/admin/members.test.js (3)
ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js (4)
stripeCouponId(1041-1041)initialMember(539-541)newsletters(420-420)newsletters(480-480)ghost/core/test/e2e-api/members/webhooks.test.js (6)
stripeCouponId(1692-1692)existingOffer(1694-1694)initialMember(229-234)initialMember(292-292)initialMember(438-438)initialMember(677-677)ghost/core/core/server/services/members/members-api/members-api.js (1)
newMember(285-285)
ghost/core/core/server/services/offers/OffersImportService.js (1)
ghost/core/test/unit/server/services/members/members-api/services/OffersImportService.test.js (13)
OffersImportService(3-3)offersAPI(15-17)offersAPI(34-36)offersAPI(57-59)offersAPI(102-104)offersAPI(120-120)offersAPI(197-197)offerRepository(12-14)offerRepository(31-33)offerRepository(54-56)offerRepository(99-101)offerRepository(123-125)offerRepository(200-202)
ghost/core/core/server/services/members/members-api/members-api.js (2)
ghost/core/test/unit/server/services/members/members-api/services/OffersImportService.test.js (6)
offersAPI(15-17)offersAPI(34-36)offersAPI(57-59)offersAPI(102-104)offersAPI(120-120)offersAPI(197-197)ghost/core/test/unit/server/services/members/members-api/controllers/RouterController.test.js (1)
offersAPI(10-10)
ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js (5)
ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js (4)
offerImportService(465-467)data(551-556)data(597-597)data(722-722)ghost/core/test/e2e-api/admin/members.test.js (1)
stripeCouponId(1743-1743)ghost/core/test/e2e-api/members/webhooks.test.js (1)
stripeCouponId(1692-1692)ghost/core/core/server/services/members/members-api/controllers/RouterController.js (2)
offerId(274-274)data(584-584)ghost/core/core/server/services/stripe/services/webhook/CheckoutSessionEventService.js (4)
offerId(206-206)offerId(237-237)data(66-84)_(1-1)
⏰ 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). (4)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (15)
ghost/core/test/e2e-api/members/webhooks.test.js (1)
35-38: Helper for fetching offers by Stripe coupon looks correctWrapping
models['Offer'].findOne({stripe_coupon_id})behindgetOfferByStripeCouponkeeps the test code clean and matches the patterns used for other model helpers in this file.ghost/core/core/server/services/members/api.js (1)
244-248: offerImportService wiring is appropriateExposing
offerImportService: offersService.importServiceon the Members API config cleanly surfaces the new import functionality to the members layer, and matches how other services are wired here.ghost/core/core/server/services/offers/application/OffersAPI.js (1)
12-16: createOffer transaction refactor preserves behavior and adds flexibilityRefactoring
createOfferinto arun(transaction)helper and short‑circuiting whenoptions.transactingis provided keeps the logic identical for the non‑transactional case while allowing callers to reuse an existing transaction.UniqueCheckerandsaveOptionscontinue to use the same transaction instance, so consistency is maintained.Also applies to: 44-60
ghost/core/core/server/services/offers/OffersImportService.js (1)
97-136: ensureOfferForCoupon handles reuse, creation, and failures gracefully
ensureOfferForCoupon:
- Quickly bails out on missing
coupon.idor missing dependencies- Reuses existing offers via
getByStripeCouponId- Creates offers via
offersAPI.createOffer, propagatingtransactingso it composes with caller transactions- Logs and returns
nullfor unsupported coupons or creation errors instead of throwing.This is a good balance between robustness and non‑disruptive behavior when offer import isn’t possible.
ghost/core/test/unit/server/services/members/members-api/services/OffersImportService.test.js (4)
1-8: LGTM!Clean test setup with proper
sinon.restore()cleanup inafterEachto prevent stub leakage between tests.
10-116: Good test coverage forensureOfferForCouponbehavior.The tests comprehensively cover:
- Reusing existing offers (avoiding duplicates)
- Creating new offers with proper transaction context propagation
- Validating invalid coupon scenarios (missing percent_off/amount_off, null coupon, missing id, amount_off without currency)
- Graceful error handling when
createOfferthrows
118-193: Thorough validation of coupon-to-offer mapping.The tests correctly verify:
- Percent-off coupons map to
type: 'percent'withcurrency: null- Amount-off coupons map to
type: 'fixed'with proper currencystatus: 'archived'prevents imported offers from being used for new signups (aligns with PR objective)stripe_coupon_idis properly set for linking back to Stripe- Duration handling for
repeatingvs non-repeating coupons
195-231: Good parameterized testing for name generation.The test cases cover the key formatting scenarios including fractional amounts (e.g., 199 cents → "USD 1.99 off once"). This ensures human-readable offer names are generated correctly from Stripe coupon data.
ghost/core/core/server/services/members/members-api/members-api.js (2)
72-72: Clean dependency injection ofofferImportService.The new parameter is properly added to the factory function signature.
115-117: Proper wiring of dependencies to MemberRepository.Both
offerRepository(fromoffersAPI.repository) andofferImportServiceare correctly passed to enable the new coupon-to-offer creation flow.ghost/core/core/server/services/offers/domain/models/Offer.js (3)
31-31: Properly typedstripeCouponIdproperty.The optional
{string|null}type is consistent with other optional properties in the model.
197-199: Getter follows established patterns.The
stripeCouponIdgetter is consistent with other property accessors in the class.
299-299: Correct data flow forstripeCouponId.The nullish coalescing operator ensures
undefinedbecomesnull, and the property is properly propagated to the Offer instance.Also applies to: 364-364
ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js (2)
55-55: Clean dependency injection pattern.The
offerImportServiceis properly documented, accepted in the constructor, and assigned to the instance.Also applies to: 77-77, 97-97
1041-1061: Well-guarded offer creation from Stripe coupons.The implementation correctly:
- Extracts the coupon ID using safe optional chaining
- Guards against overwriting existing offers (
!offerId)- Requires a valid
ghostProductbefore attempting offer creation- Passes the transaction context for consistency
- Only sets
offerIdwhenensureOfferForCouponsucceeds (returns a truthy value)This aligns with the PR objective of creating offers during imports when a Stripe coupon exists but no Ghost offer is linked.
865b74b to
8faceeb
Compare
8faceeb to
2f69f02
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
ghost/core/core/server/services/members/api.js(1 hunks)ghost/core/core/server/services/members/members-api/members-api.js(3 hunks)ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js(7 hunks)ghost/core/core/server/services/offers/OfferBookshelfRepository.js(2 hunks)ghost/core/core/server/services/offers/OffersImportService.js(1 hunks)ghost/core/core/server/services/offers/OffersModule.js(2 hunks)ghost/core/core/server/services/offers/application/OffersAPI.js(2 hunks)ghost/core/core/server/services/offers/domain/models/Offer.js(5 hunks)ghost/core/core/server/services/offers/service.js(1 hunks)ghost/core/test/e2e-api/members/webhooks.test.js(6 hunks)ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js(2 hunks)ghost/core/test/unit/server/services/members/members-api/services/OffersImportService.test.js(1 hunks)ghost/core/test/unit/server/services/offers/application/OffersAPI.test.js(1 hunks)ghost/core/test/unit/server/services/offers/domain/models/Offer.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- ghost/core/core/server/services/offers/application/OffersAPI.js
- ghost/core/test/unit/server/services/members/members-api/services/OffersImportService.test.js
- ghost/core/core/server/services/offers/domain/models/Offer.js
- ghost/core/test/unit/server/services/offers/domain/models/Offer.test.js
- ghost/core/core/server/services/members/members-api/members-api.js
- ghost/core/core/server/services/offers/service.js
- ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js
- ghost/core/core/server/services/offers/OfferBookshelfRepository.js
🧰 Additional context used
🧬 Code graph analysis (5)
ghost/core/core/server/services/offers/OffersModule.js (1)
ghost/core/core/server/services/offers/service.js (1)
OffersModule(6-6)
ghost/core/core/server/services/offers/OffersImportService.js (3)
ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js (1)
require(6-6)ghost/core/core/server/services/offers/OffersModule.js (1)
OffersImportService(9-9)ghost/core/test/unit/server/services/members/members-api/services/OffersImportService.test.js (7)
OffersImportService(3-3)offersAPI(15-17)offersAPI(34-36)offersAPI(57-59)offersAPI(102-104)offersAPI(120-120)offersAPI(197-197)
ghost/core/test/unit/server/services/offers/application/OffersAPI.test.js (3)
ghost/core/core/server/services/offers/domain/models/Offer.js (1)
ObjectID(2-2)ghost/core/core/server/services/offers/OffersModule.js (1)
OffersAPI(8-8)ghost/core/core/server/services/offers/service.js (1)
repository(18-21)
ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js (1)
ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js (1)
offersImportService(465-467)
ghost/core/test/e2e-api/members/webhooks.test.js (2)
ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js (7)
stripeCouponId(1041-1041)member(347-347)member(679-682)member(764-764)member(1450-1452)member(1486-1486)member(1530-1530)ghost/core/core/server/services/offers/service.js (1)
models(4-4)
⏰ 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). (8)
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Lint
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (11)
ghost/core/core/server/services/offers/OffersModule.js (1)
9-9: LGTM!The
OffersImportServiceintegration is cleanly wired intoOffersModule. The constructor accepts the new dependency with proper JSDoc documentation, and the staticcreate()factory correctly instantiates the service with the required dependencies (offersAPIandofferRepository).Also applies to: 18-20, 68-72
ghost/core/core/server/services/members/api.js (1)
247-247: LGTM!The
offersImportServiceis correctly wired into the MembersApi configuration, following the same pattern as the existingoffersAPIwiring.ghost/core/test/unit/server/services/offers/application/OffersAPI.test.js (1)
1-62: LGTM!Well-structured unit tests covering the transaction behavior of
OffersAPI.createOffer. The tests verify both paths: reusing a provided transaction and creating a new one when none is provided. Good use of stubs and clear assertions.ghost/core/test/e2e-api/members/webhooks.test.js (2)
35-38: LGTM!The
getOfferByStripeCouponhelper follows the established pattern of other helper functions likegetMemberandgetSubscription.
1687-1831: Comprehensive e2e test for the new coupon-to-offer creation flow.The test properly validates the core functionality:
- Asserts no offer exists initially for the coupon
- Creates a subscription with the Stripe coupon discount
- Verifies an offer was automatically created
- Confirms the subscription is correctly linked to the created offer
This provides good coverage for the main use case described in the PR objectives.
ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js (2)
55-55: LGTM!The
offersImportServicedependency is properly added to the constructor with JSDoc documentation and stored as an instance property, following the existing pattern for other dependencies.Also applies to: 77-77, 97-97
1041-1061: Clean implementation of the coupon-to-offer mapping.The logic correctly:
- Extracts the Stripe coupon ID from the subscription discount
- Only attempts offer creation when: a coupon exists, no explicit
offerIdwas provided, and aghostProductis available for tier information- Passes the transaction context to maintain data consistency
- Preserves the existing behavior for trial offers (which pass
offerIdvia metadata)The
ensureOfferForCouponcall receivingcadenceandtierinfo enables proper offer creation matching the subscription's billing interval and product.ghost/core/core/server/services/offers/OffersImportService.js (4)
1-18: LGTM!Clean dependency injection pattern with sensible defaults for the logger.
105-134: Well-structured with proper error handling.The method demonstrates good practices:
- Input validation and early returns
- Deduplication check to prevent duplicate offers
- Graceful error handling with logging
- Transaction support for atomicity
- Returns
nullon failures rather than throwing, which is appropriate for migration scenarios where you want to continue processing other members
54-66: [rewritten comment]
[classification tag]
75-92: Offer code uniqueness is protected by database constraint and application validation, but deduplication is incomplete.The concern about unique constraint conflicts is valid. The offers table has a
unique: trueconstraint on thecodefield (schema.js line 484). Line 77 usescoupon.idas the code, and line 115 deduplicates bystripe_coupon_idonly—not by code.However, the
createOffermethod does validate code uniqueness viaUniqueCheckerbefore saving (Offer.js lines 285-290). If a code conflict occurs, it throwsInvalidOfferCode, which is caught in the try-catch at lines 126-133 and logged as a warning. This prevents crashes but means failed imports are silently logged without creating the offer. The deduplication logic should check for existing offers by code as well, not just bystripe_coupon_id, to provide clearer feedback if imports are skipped due to code conflicts.
allouis
left a comment
There was a problem hiding this comment.
Overall I think the solution of creating offers from coupons make sense, I have one concern about a race condition, and I also think we can simplify this implementation a bit!
null checks and flow
There is a lot of defensive programming here which makes the flow more complicated, we do a bunch of checks against null where null should not be possible under the Stripe API contract
IMO we should use types to make it clear the data structures we're working with (and expect) - and we can do a single top level validation and throw (fail fast) if things don't match. That way we're immediately alerted to the fact that something is broken between us and Stripe.
If we really don't want to throw we can log an error, but at least the downstream logic doesn't need null checks everywhere
new OffersImportService
I'm not 100% sure we need this 🤔 The existing pattern for creating Offers is a static create factory method on the Offer domain object - I think we could add a createFromStripeCoupon factory too, which would encapsulate the logic of generating the name and offer data.
And then we have the ensureOfferForCoupon - which could just live on the OffersAPI? This second part is less of a deal to me tbh but curious what you think
ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js
Outdated
Show resolved
Hide resolved
| return null; | ||
| } | ||
|
|
||
| const existing = await this.offerRepository.getByStripeCouponId(coupon.id, {transacting}); |
There was a problem hiding this comment.
This gets called as part of linkSubscription which gets called on incoming webhooks - is there chance of a race condition here? Let's say we get two incoming webhooks, both of them pass this step with existing === null, and then they both go on to attempt to create the offer?
There was a problem hiding this comment.
Great catch! 🎣
Added protection against race condition in 76db34b, we now:
- Check that the offer exists
- Try to create a new one
- If it fails, check again if the offer exists
- Else throw
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
ghost/core/test/unit/server/services/offers/domain/models/StripeCoupon.test.js (1)
4-171: Consider adding a test for missing discount type.The tests comprehensively cover validation scenarios, but there's no test for when neither
percent_offnoramount_offis provided. If theStripeCoupon.createfactory should reject such input, a test would ensure that behavior is verified.it('Throws if neither percent_off nor amount_off is set', function () { should.throws(() => { StripeCoupon.create({ id: 'coupon_123', duration: 'forever' }); }, /must have either `percent_off` or `amount_off`/); });ghost/core/core/server/services/offers/domain/models/StripeCoupon.js (2)
64-68: Truthiness check may skip validation for zero values.The condition
input.percent_off && input.amount_offuses truthiness, which means if either value is0, the mutual exclusivity check would pass incorrectly. While Stripe likely doesn't issue coupons with zero discounts, defensive validation using explicitundefinedchecks would be more robust.- if (input.percent_off && input.amount_off) { + if (input.percent_off !== undefined && input.amount_off !== undefined) {
70-80: Type checks skipped when values are falsy (e.g., 0).Similar to the mutual exclusivity check, the type validations at lines 70 and 76 use truthiness. If
percent_offoramount_offis0, the type check is bypassed. Consider using explicit checks:- if (input.percent_off && typeof input.percent_off !== 'number') { + if (input.percent_off !== undefined && typeof input.percent_off !== 'number') { throw new InvalidStripeCoupon({ message: 'Stripe coupon `percent_off` must be a number.' }); } - if (input.amount_off && typeof input.amount_off !== 'number') { + if (input.amount_off !== undefined && typeof input.amount_off !== 'number') { throw new InvalidStripeCoupon({ message: 'Stripe coupon `amount_off` must be a number.' }); }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
ghost/core/core/server/services/members/members-api/members-api.js(2 hunks)ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js(7 hunks)ghost/core/core/server/services/offers/OfferBookshelfRepository.js(3 hunks)ghost/core/core/server/services/offers/application/OffersAPI.js(3 hunks)ghost/core/core/server/services/offers/domain/errors/index.js(2 hunks)ghost/core/core/server/services/offers/domain/models/Offer.js(7 hunks)ghost/core/core/server/services/offers/domain/models/StripeCoupon.js(1 hunks)ghost/core/test/e2e-api/members/webhooks.test.js(6 hunks)ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js(2 hunks)ghost/core/test/unit/server/services/offers/application/OffersAPI.test.js(1 hunks)ghost/core/test/unit/server/services/offers/domain/models/Offer.test.js(2 hunks)ghost/core/test/unit/server/services/offers/domain/models/StripeCoupon.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js
- ghost/core/test/unit/server/services/offers/application/OffersAPI.test.js
- ghost/core/core/server/services/members/members-api/members-api.js
🧰 Additional context used
🧬 Code graph analysis (7)
ghost/core/test/e2e-api/members/webhooks.test.js (3)
ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js (7)
stripeCouponId(1038-1038)member(344-344)member(676-679)member(761-761)member(1444-1446)member(1480-1480)member(1524-1524)ghost/core/core/server/services/offers/service.js (1)
models(4-4)ghost/core/core/server/services/members/api.js (1)
models(7-7)
ghost/core/core/server/services/offers/application/OffersAPI.js (1)
ghost/core/test/unit/server/services/offers/application/OffersAPI.test.js (14)
coupon(66-66)coupon(84-84)coupon(113-113)coupon(141-141)coupon(160-160)coupon(183-183)coupon(203-203)tier(67-67)tier(85-85)tier(114-114)tier(142-142)tier(161-161)tier(184-184)tier(204-204)
ghost/core/core/server/services/offers/domain/errors/index.js (1)
ghost/core/core/server/services/offers/domain/models/StripeCoupon.js (1)
InvalidStripeCoupon(3-3)
ghost/core/core/server/services/offers/domain/models/StripeCoupon.js (2)
ghost/core/core/server/services/offers/domain/models/Offer.js (1)
StripeCoupon(17-17)ghost/core/test/unit/server/services/offers/domain/models/StripeCoupon.test.js (4)
StripeCoupon(2-2)coupon(7-11)coupon(21-26)coupon(37-42)
ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js (3)
ghost/core/core/server/services/members/members-api/controllers/RouterController.js (3)
offerId(274-274)data(584-584)options(566-573)ghost/core/core/server/services/stripe/services/webhook/CheckoutSessionEventService.js (4)
offerId(206-206)offerId(237-237)data(66-84)_(1-1)ghost/core/test/unit/server/services/offers/application/OffersAPI.test.js (14)
coupon(66-66)coupon(84-84)coupon(113-113)coupon(141-141)coupon(160-160)coupon(183-183)coupon(203-203)tier(67-67)tier(85-85)tier(114-114)tier(142-142)tier(161-161)tier(184-184)tier(204-204)
ghost/core/core/server/services/offers/domain/models/Offer.js (1)
ghost/core/test/unit/server/services/offers/domain/models/StripeCoupon.test.js (4)
StripeCoupon(2-2)coupon(7-11)coupon(21-26)coupon(37-42)
ghost/core/test/unit/server/services/offers/domain/models/Offer.test.js (1)
ghost/core/core/server/services/offers/domain/models/Offer.js (1)
ObjectID(2-2)
⏰ 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). (8)
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Lint
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (13)
ghost/core/core/server/services/offers/domain/errors/index.js (1)
26-41: LGTM!The new
InvalidStripeCouponerror class follows the established pattern of other domain validation errors and is properly exported.ghost/core/core/server/services/offers/OfferBookshelfRepository.js (3)
159-175: LGTM!The
getByStripeCouponIdmethod follows the same pattern asgetById, correctly querying with the related product and mapping to an Offer domain model.
220-222: LGTM!Conditionally including
stripe_coupon_idonly when defined prevents accidental overwrites during updates and maintains backward compatibility.
124-124: LGTM!The
stripe_coupon_idis correctly mapped from the database model to the domain model.ghost/core/test/unit/server/services/offers/domain/models/Offer.test.js (2)
44-61: LGTM!Good test coverage for verifying that
stripe_coupon_idis correctly stored on the Offer instance when provided during creation.
434-618: LGTM!Excellent test coverage for the
createFromStripeCouponfactory. Tests validate the full range of coupon types, durations, name generation, tier association, and the archived status for migration-created offers.ghost/core/core/server/services/offers/domain/models/StripeCoupon.js (1)
1-45: LGTM!The value object implementation is clean, with well-typed getters and proper JSDoc annotations. Exposing
InvalidStripeCouponas a static property is a good pattern for error handling by consumers.ghost/core/core/server/services/offers/application/OffersAPI.js (1)
140-174: Well-structured race condition handling for concurrent offer creation.The
ensureOfferForStripeCouponmethod correctly handles the check-then-create race condition by catching duplicate entry errors and falling back to fetching the existing offer. The transaction management with support for external transactions is clean.One minor observation: if the duplicate error occurs but
getByStripeCouponIdreturnsnullin the catch block (perhaps due to an unexpected constraint violation), the original error is rethrown, which is the correct behavior.ghost/core/test/e2e-api/members/webhooks.test.js (2)
35-38: Good addition of test helper for Stripe coupon offer lookup.The helper function follows the same pattern as
getSubscriptionandgetMemberhelpers in the file.
1687-1834: Comprehensive e2e test for Stripe coupon offer creation flow.The test properly validates:
- No offer exists initially for the coupon ID
- Offer is automatically created during subscription processing
- Offer properties match expected values (code, archived status, auto-generated name)
- Subscription is correctly linked to the created offer
This provides good coverage for the new migration fix.
ghost/core/core/server/services/offers/domain/models/Offer.js (2)
198-200: LGTM - Clean getter for stripeCouponId.The getter follows the established pattern in the class.
372-432: Well-designed factory method for creating offers from Stripe coupons.The
createFromStripeCouponmethod:
- Properly validates input via
StripeCoupon.create()- Generates descriptive names including the coupon ID for uniqueness
- Archives created offers to prevent use for new signups (as per PR objectives)
- Delegates to the existing
create()method for consistent validationThe design aligns well with the PR objective to reconcile Stripe coupon data with Ghost offers during migrations.
ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js (1)
54-55: Consistent dependency injection update from offerRepository to offersAPI.The JSDoc, constructor parameter, and instance property are all correctly updated to use
offersAPI.Also applies to: 75-76, 94-95
ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js
Show resolved
Hide resolved
3891b11 to
3f345e7
Compare
| */ | ||
|
|
||
| /** @extends ValueObject<StripeCouponInput> **/ | ||
| class StripeCoupon extends ValueObject { |
There was a problem hiding this comment.
@allouis I was on the fence about these validations:
- on one hand, we shouldn't need to validate the format of Stripe Coupons
- on the other hand, if the API contract changes after a Stripe upgrade and our code needs adapting, it'd be great to see a readable error in the logs and an unit test failure
There was a problem hiding this comment.
I think these are great - we've moved all of those null checks into explicit assertions, and they're all in one place which is easy to find. Maybe it doesn't belong in the "domain" and could be validation at the API layer - but I think that's nitpicking 🐜
3f345e7 to
91856a2
Compare
… / offer entity, fail fast over null fallbacks
91856a2 to
76db34b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
ghost/core/core/server/services/members/members-api/members-api.js(2 hunks)ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js(7 hunks)ghost/core/core/server/services/offers/OfferBookshelfRepository.js(3 hunks)ghost/core/core/server/services/offers/application/OffersAPI.js(2 hunks)ghost/core/core/server/services/offers/domain/errors/index.js(2 hunks)ghost/core/core/server/services/offers/domain/models/Offer.js(7 hunks)ghost/core/core/server/services/offers/domain/models/StripeCoupon.js(1 hunks)ghost/core/test/e2e-api/members/webhooks.test.js(6 hunks)ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js(2 hunks)ghost/core/test/unit/server/services/offers/application/OffersAPI.test.js(1 hunks)ghost/core/test/unit/server/services/offers/domain/models/Offer.test.js(2 hunks)ghost/core/test/unit/server/services/offers/domain/models/StripeCoupon.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- ghost/core/core/server/services/offers/domain/errors/index.js
- ghost/core/core/server/services/offers/OfferBookshelfRepository.js
- ghost/core/core/server/services/members/members-api/members-api.js
- ghost/core/test/unit/server/services/members/members-api/repositories/MemberRepository.test.js
- ghost/core/test/unit/server/services/offers/application/OffersAPI.test.js
- ghost/core/core/server/services/members/members-api/repositories/MemberRepository.js
- ghost/core/test/e2e-api/members/webhooks.test.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Use factory pattern for all test data creation instead of hard-coded data or direct database manipulation
Applied to files:
ghost/core/test/unit/server/services/offers/domain/models/Offer.test.js
🧬 Code graph analysis (4)
ghost/core/core/server/services/offers/domain/models/StripeCoupon.js (3)
ghost/core/core/server/services/offers/domain/errors/index.js (1)
require(1-1)ghost/core/core/server/services/offers/domain/models/Offer.js (1)
StripeCoupon(17-17)ghost/core/test/unit/server/services/offers/domain/models/StripeCoupon.test.js (4)
StripeCoupon(2-2)coupon(7-11)coupon(21-26)coupon(37-42)
ghost/core/core/server/services/offers/domain/models/Offer.js (1)
ghost/core/test/unit/server/services/offers/domain/models/StripeCoupon.test.js (4)
StripeCoupon(2-2)coupon(7-11)coupon(21-26)coupon(37-42)
ghost/core/test/unit/server/services/offers/domain/models/Offer.test.js (2)
ghost/core/core/server/services/offers/domain/models/Offer.js (1)
ObjectID(2-2)ghost/core/test/unit/server/services/offers/domain/models/StripeCoupon.test.js (1)
should(1-1)
ghost/core/test/unit/server/services/offers/domain/models/StripeCoupon.test.js (2)
ghost/core/core/server/services/offers/domain/errors/index.js (1)
require(1-1)ghost/core/core/server/services/offers/domain/models/Offer.js (1)
StripeCoupon(17-17)
🔇 Additional comments (11)
ghost/core/test/unit/server/services/offers/domain/models/StripeCoupon.test.js (1)
1-181: LGTM! Comprehensive test coverage for StripeCoupon validation.The test suite thoroughly covers all creation scenarios and validation paths, including:
- Valid creation with both percent_off and amount_off coupons
- Repeating duration handling
- All error cases with appropriate error message assertions
- Type checking for all fields
- Mutual exclusivity and requirement validations
The tests align well with the StripeCoupon domain model implementation and will catch any regressions in validation logic.
ghost/core/test/unit/server/services/offers/domain/models/Offer.test.js (2)
44-61: LGTM! Properly tests stripe_coupon_id storage.The test verifies that the new
stripe_coupon_idproperty is correctly stored and accessible via thestripeCouponIdgetter, following the existing test patterns in the file.
434-618: LGTM! Comprehensive test coverage for createFromStripeCoupon factory.The test suite thoroughly exercises the new factory method:
- Both percent_off and amount_off coupon types
- All duration types (forever, once, repeating)
- Name generation formatting with currency and duration
- Cadence and tier association
- Archived status and new-offer flag
The tests provide excellent coverage and will catch regressions in the factory method logic.
ghost/core/core/server/services/offers/domain/models/StripeCoupon.js (2)
48-107: LGTM! Comprehensive validation logic.The validation logic correctly handles all requirements:
- Proper type checking for all fields
- Mutual exclusivity of percent_off and amount_off
- Required field validation for at least one discount type (addressed from past review)
- Currency requirement when amount_off is used
- Clear, specific error messages for each validation failure
The validation order is logical and will catch issues early. Based on past review comments, the missing validation for at least one discount type was correctly added.
15-110: LGTM! Well-structured value object.The StripeCoupon class properly extends ValueObject and provides:
- Typed getters for all properties with correct return types
- Optional properties correctly typed as
| undefined- Static InvalidStripeCoupon exposure for error handling
- Clean separation between data structure and validation logic
ghost/core/core/server/services/offers/application/OffersAPI.js (2)
140-174: LGTM! Proper transaction management.The method correctly handles both scenarios:
- Using an existing transaction when provided via options
- Creating a new transaction when not provided
The pattern follows the existing transaction management approach used in other methods (e.g.,
createOffer,updateOffer).
125-139: LGTM! Well-documented method signature.The JSDoc comprehensively documents:
- All coupon properties with correct types and optionality
- Cadence and tier parameters
- Transaction options for integration with existing flows
- Return type for consumers
ghost/core/core/server/services/offers/domain/models/Offer.js (4)
17-17: LGTM! Proper integration of stripe_coupon_id property.The property additions are well-integrated:
- Correct type annotations in JSDoc
- Proper getter implementation
- Nullish coalescing operator correctly handles both undefined and null
- Property is correctly passed through the creation flow
Also applies to: 32-32, 53-53, 198-200, 300-300, 365-365
386-407: LGTM! Proper derivation of offer properties from coupon.The method correctly:
- Validates the coupon via
StripeCoupon.create(which ensures at least one discount type exists, per past review feedback)- Derives type, amount, currency, and name based on coupon type
- Formats names appropriately with duration text and coupon ID
- Handles currency formatting for fixed amounts (uppercase, divided by 100)
No else clause is needed after the if-else if structure because
StripeCoupon.createguarantees exactly one discount type is set.
394-407: LGTM! Clear and consistent name formatting.The name generation logic produces human-readable offer names:
- Duration text correctly handles "for X months" vs direct duration values
- Percent-off names include percentage and duration
- Fixed-amount names include uppercase currency, converted amount (cents to major units), and duration
- Coupon ID suffix ensures uniqueness and traceability
409-431: LGTM! Properly constructed offer data.The data object correctly:
- Uses coupon.id for both code and stripe_coupon_id (code will be normalized by OfferCode.create)
- Sets status to 'archived' to prevent use for new signups (per PR objectives)
- Includes all required fields for Offer.create
- Passes through tier information correctly
- Delegates to the existing Offer.create method for validation and construction
| */ | ||
|
|
||
| /** @extends ValueObject<StripeCouponInput> **/ | ||
| class StripeCoupon extends ValueObject { |
There was a problem hiding this comment.
I think these are great - we've moved all of those null checks into explicit assertions, and they're all in one place which is easy to find. Maybe it doesn't belong in the "domain" and could be validation at the API layer - but I think that's nitpicking 🐜
| }); | ||
|
|
||
| it('Silently ignores an invalid offer id in metadata', async function () { | ||
| it('Supports creating an offer from a Stripe coupon', async function () { |
There was a problem hiding this comment.
We had an explicit test for the opposite of this behaviour! At least it was half expected I guess 😁
There was a problem hiding this comment.
Haha yeah! Outside of migration flows, I think it's fair to assume that the offer will pre-exist in 99% of the cases and we don't need to "generate" it? Maybe a limitation we were aware of, but didn't want to complicate things for an edge case (outside of migrations) - don't know :)
ref https://linear.app/ghost/issue/ONC-1284 - before: during migrations, paid members with a Stripe coupon would be created without the corresponding offer in Ghost. As a result, while they were charged the right discounted amount by Stripe, they would appear as if they were paying the full subscription price in Ghost Admin or Portal - now: we create an offer based on Stripe coupons during migrations. That way, the subscription price in Ghost matches the subscription price in Stripe, taking into account the discount - the offer is marked as archived, so that it cannot be used for signups. The name is computed based on the coupon type, duration and code for uniqueness, e.g. `"15% off forever (dai8HKn)"`
ref https://linear.app/ghost/issue/ONC-1284
"15% off forever (dai8HKn)"