Added support for continuing a gift sub as paid sub#27432
Conversation
WalkthroughAdds UI banner 🚥 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9df5e573cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (gift.isConsumed()) { | ||
| return null; |
There was a problem hiding this comment.
getActiveByRedeemer() currently treats any non-consumed gift as active, so a refunded redeemed gift (which still has redeemer_member_id and often a future consumesAt) is returned and converted into trial_period_days during checkout. In the refund flow this allows members to receive a new free trial after their gift has already been refunded, which is a revenue-impacting eligibility bug.
Useful? React with 👍 / 👎.
| const model = await this.model.findOne({ | ||
| redeemer_member_id: memberId | ||
| }, {require: false}); |
There was a problem hiding this comment.
getByRedeemerMemberId() uses an unconstrained findOne by redeemer_member_id, so members with multiple historical gifts can get an arbitrary row (often an older consumed/refunded one) instead of their currently active redeemed gift. That causes incorrect trial behavior in checkout (for example, no trial when a newer gift still has remaining days), because getActiveByRedeemer() only sees this single returned record.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@apps/portal/src/components/pages/account-plan-page.js`:
- Around line 97-123: GiftTrialBanner currently uses getSubscriptionExpiry()
which doesn't read gift expiry; update the component to read the expiry from the
gift product on the member (member.products[].expiry_at) when member?.status ===
'gift' instead of relying on getSubscriptionExpiry(). In the GiftTrialBanner
component, detect isGift and look up the product entry with an expiry_at (e.g.,
first product or the one with a gift marker) and use that expiry value as
expiryDate passed into the Interpolate string; keep existing behavior for
non-gift members (use getSubscriptionExpiry) and ensure null checks for
member.products to avoid runtime errors.
In `@ghost/core/core/server/services/gifts/gift-bookshelf-repository.ts`:
- Around line 78-83: The current getByRedeemerMemberId method does a
nondeterministic findOne({redeemer_member_id}) which can return an old
consumed/refunded row; change the query in getByRedeemerMemberId (and/or the
caller getActiveByRedeemer) to only return the active redeemed gift by adding a
status constraint (e.g. status = 'redeemed' and not consumed/refunded) and order
by the latest redeem timestamp (redeemed_at or created_at) descending so you
return the most recent active gift; update the model.findOne call to include the
status filter and an order clause (latest first) so the method deterministically
returns the active redeemed gift.
In `@ghost/core/core/server/services/gifts/gift-service.ts`:
- Around line 285-297: The getActiveByRedeemer method currently only checks
gift.isConsumed() after retrieving a gift via
deps.giftRepository.getByRedeemerMemberId(memberId); update this method
(getActiveByRedeemer) to also verify that gift.status === 'redeemed' (in
addition to !gift.isConsumed()) before returning the gift, and return null if
the status is anything else so refunded/non-redeemed gifts are not returned.
🪄 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: d823ca8d-45db-4409-8adf-98374da013eb
📒 Files selected for processing (7)
apps/portal/src/components/pages/account-plan-page.jsghost/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/members/members-api/members-api.jsghost/core/core/server/services/members/members-api/services/payments-service.jsghost/core/test/e2e-api/members/gift-subscriptions.test.js
| const GiftTrialBanner = () => { | ||
| const {member} = useContext(AppContext); | ||
| const isGift = member?.status === 'gift'; | ||
| const subscriptionExpiry = getSubscriptionExpiry({member}); | ||
|
|
||
| if (!isGift || !subscriptionExpiry) { | ||
| return null; | ||
| } | ||
|
|
||
| return ( | ||
| <div style={{ | ||
| display: 'flex', | ||
| justifyContent: 'center', | ||
| margin: '8px 0 32px' | ||
| }}> | ||
| <div style={{ | ||
| background: '#FEF3C7', | ||
| borderRadius: '5px', | ||
| padding: '16px 24px', | ||
| maxWidth: '420px', | ||
| fontSize: '1.35rem', | ||
| lineHeight: '1.5em', | ||
| color: '#1d1d1d', | ||
| textAlign: 'center' | ||
| }}> | ||
| <Interpolate | ||
| string={t('Your gift subscription is valid until <strong>{expiryDate}</strong>. If you continue with a paid subscription, the remaining days will be added as a free trial.', {expiryDate: subscriptionExpiry})} |
There was a problem hiding this comment.
Read the expiry from gift membership data, not getSubscriptionExpiry().
apps/portal/src/utils/helpers.js:96-102 only reads subscription.tier.expiry_at, but gift redemption stores the end date on member.products[].expiry_at (ghost/core/core/server/services/gifts/gift-service.ts:248-254, also reflected in ghost/core/test/e2e-api/members/gift-subscriptions.test.js:446-452). In this component that leaves subscriptionExpiry empty for gift members, so the new banner never renders for the users it is meant for.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/portal/src/components/pages/account-plan-page.js` around lines 97 - 123,
GiftTrialBanner currently uses getSubscriptionExpiry() which doesn't read gift
expiry; update the component to read the expiry from the gift product on the
member (member.products[].expiry_at) when member?.status === 'gift' instead of
relying on getSubscriptionExpiry(). In the GiftTrialBanner component, detect
isGift and look up the product entry with an expiry_at (e.g., first product or
the one with a gift marker) and use that expiry value as expiryDate passed into
the Interpolate string; keep existing behavior for non-gift members (use
getSubscriptionExpiry) and ensure null checks for member.products to avoid
runtime errors.
| async getByRedeemerMemberId(memberId: string): Promise<Gift | null> { | ||
| const model = await this.model.findOne({ | ||
| redeemer_member_id: memberId | ||
| }, {require: false}); | ||
|
|
||
| return model ? this.toGift(model) : null; |
There was a problem hiding this comment.
Don't fetch an arbitrary gift row for a redeemer.
This lookup is nondeterministic once a member has historical gifts. findOne({redeemer_member_id}) can return an older consumed/refunded record instead of the current redeemed one, which means getActiveByRedeemer() may miss the active gift and fail to grant the intended trial days. The query needs to constrain itself to the active redeemed gift, or at least order by the latest gift before returning one.
🤖 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 78 - 83, The current getByRedeemerMemberId method does a nondeterministic
findOne({redeemer_member_id}) which can return an old consumed/refunded row;
change the query in getByRedeemerMemberId (and/or the caller
getActiveByRedeemer) to only return the active redeemed gift by adding a status
constraint (e.g. status = 'redeemed' and not consumed/refunded) and order by the
latest redeem timestamp (redeemed_at or created_at) descending so you return the
most recent active gift; update the model.findOne call to include the status
filter and an order clause (latest first) so the method deterministically
returns the active redeemed gift.
| async getActiveByRedeemer(memberId: string): Promise<Gift | null> { | ||
| const gift = await this.deps.giftRepository.getByRedeemerMemberId(memberId); | ||
|
|
||
| if (!gift) { | ||
| return null; | ||
| } | ||
|
|
||
| if (gift.isConsumed()) { | ||
| return null; | ||
| } | ||
|
|
||
| return gift; | ||
| } |
There was a problem hiding this comment.
Only return gifts that are still in the redeemed state.
isConsumed() is not enough here. A refunded gift still has a redeemerMemberId, so this method can hand payments-service a non-consumed but refunded gift and grant trial days that should no longer exist. Gate this on gift.status === 'redeemed' as well.
Possible fix
async getActiveByRedeemer(memberId: string): Promise<Gift | null> {
const gift = await this.deps.giftRepository.getByRedeemerMemberId(memberId);
if (!gift) {
return null;
}
- if (gift.isConsumed()) {
+ if (gift.status !== 'redeemed' || gift.isConsumed()) {
return null;
}
return gift;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async getActiveByRedeemer(memberId: string): Promise<Gift | null> { | |
| const gift = await this.deps.giftRepository.getByRedeemerMemberId(memberId); | |
| if (!gift) { | |
| return null; | |
| } | |
| if (gift.isConsumed()) { | |
| return null; | |
| } | |
| return gift; | |
| } | |
| async getActiveByRedeemer(memberId: string): Promise<Gift | null> { | |
| const gift = await this.deps.giftRepository.getByRedeemerMemberId(memberId); | |
| if (!gift) { | |
| return null; | |
| } | |
| if (gift.status !== 'redeemed' || gift.isConsumed()) { | |
| return null; | |
| } | |
| return gift; | |
| } |
🤖 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 285 -
297, The getActiveByRedeemer method currently only checks gift.isConsumed()
after retrieving a gift via deps.giftRepository.getByRedeemerMemberId(memberId);
update this method (getActiveByRedeemer) to also verify that gift.status ===
'redeemed' (in addition to !gift.isConsumed()) before returning the gift, and
return null if the status is anything else so refunded/non-redeemed gifts are
not returned.
sagzy
force-pushed
the
gift-subscriptions/upgrade-gift-to-paid
branch
from
9df5e57 to
cc9e18c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/portal/src/components/pages/account-plan-page.js (1)
99-103:⚠️ Potential issue | 🟠 MajorUse gift product expiry, not
getSubscriptionExpiry(), for gift members.At Line 100,
getSubscriptionExpiry({member})resolves from active paid subscription tier expiry, so gift members can end up with an empty value and the banner won’t render. Read expiry frommember.products[].expiry_atformember.status === 'gift'.💡 Suggested fix
const GiftTrialBanner = () => { const {member} = useContext(AppContext); const isGift = member?.status === 'gift'; - const subscriptionExpiry = getSubscriptionExpiry({member}); + const giftExpiryAt = member?.products?.find(product => product?.expiry_at)?.expiry_at; + const subscriptionExpiry = isGift ? getDateString(giftExpiryAt) : getSubscriptionExpiry({member}); if (!isGift || !subscriptionExpiry) { return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/portal/src/components/pages/account-plan-page.js` around lines 99 - 103, The banner currently uses getSubscriptionExpiry({member}) which returns paid-subscription expiry and can be empty for gift members; change the logic in the block that computes subscriptionExpiry (and the early return that uses isGift) to use member.products[].expiry_at when member.status === 'gift' (i.e., if isGift true, derive subscriptionExpiry from the gift product's expiry_at), otherwise keep using getSubscriptionExpiry({member}); ensure the variable names isGift and subscriptionExpiry are preserved so the subsequent return logic still works.
🧹 Nitpick comments (1)
ghost/core/core/server/services/members/members-api/services/payments-service.js (1)
95-107: Skip the gift lookup when a coupon already exists.Coupon offers still win later at Line 125, so this branch does an unnecessary gift read for every coupon-backed checkout. Guarding on
!couponkeeps the priority order explicit and avoids extra work on the hot path.♻️ Suggested tweak
- if (trialDays === null && member) { + if (trialDays === null && !coupon && member) { const gift = await this.giftService.service.getActiveByRedeemer(member.id); if (gift && gift.consumesAt) { const now = new Date(); const diffMs = gift.consumesAt.getTime() - now.getTime();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/members/members-api/services/payments-service.js` around lines 95 - 107, The gift lookup block currently runs even when a coupon is present; update the conditional to only attempt the gift lookup when coupon is not set so we avoid unnecessary reads on the coupon-backed checkout hot path. Specifically, change the if that checks "trialDays === null && member" to also require "!coupon" before calling this.giftService.service.getActiveByRedeemer and computing diffDays; keep the existing logic that caps trialDays to Math.min(diffDays, 730) and retain the later coupon-priority logic (the code that handles coupon offers).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/portal/src/components/pages/account-plan-page.js`:
- Around line 122-126: Replace the hardcoded paragraph that uses
subscriptionExpiry with a single translatable string using t() and Interpolate
from `@doist/react-interpolate`: create one t() call that includes a placeholder
like "{expiry}" and pass subscriptionExpiry as the interpolation value,
embedding the <strong> element via Interpolate so the bolded expiry remains a
React element; update the JSX in the AccountPlanPage component (where the <p>
currently renders subscriptionExpiry) to use t(...) + <Interpolate ... /> and
remove the literal string to satisfy i18n linting.
---
Duplicate comments:
In `@apps/portal/src/components/pages/account-plan-page.js`:
- Around line 99-103: The banner currently uses getSubscriptionExpiry({member})
which returns paid-subscription expiry and can be empty for gift members; change
the logic in the block that computes subscriptionExpiry (and the early return
that uses isGift) to use member.products[].expiry_at when member.status ===
'gift' (i.e., if isGift true, derive subscriptionExpiry from the gift product's
expiry_at), otherwise keep using getSubscriptionExpiry({member}); ensure the
variable names isGift and subscriptionExpiry are preserved so the subsequent
return logic still works.
---
Nitpick comments:
In
`@ghost/core/core/server/services/members/members-api/services/payments-service.js`:
- Around line 95-107: The gift lookup block currently runs even when a coupon is
present; update the conditional to only attempt the gift lookup when coupon is
not set so we avoid unnecessary reads on the coupon-backed checkout hot path.
Specifically, change the if that checks "trialDays === null && member" to also
require "!coupon" before calling this.giftService.service.getActiveByRedeemer
and computing diffDays; keep the existing logic that caps trialDays to
Math.min(diffDays, 730) and retain the later coupon-priority logic (the code
that handles coupon offers).
🪄 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: ffe69000-5c76-44be-b0a2-c52ce028df17
📒 Files selected for processing (8)
apps/portal/package.jsonapps/portal/src/components/pages/account-plan-page.jsghost/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/members/members-api/members-api.jsghost/core/core/server/services/members/members-api/services/payments-service.jsghost/core/test/e2e-api/members/gift-subscriptions.test.js
✅ Files skipped from review due to trivial changes (5)
- ghost/core/core/server/services/gifts/gift-repository.ts
- ghost/core/core/server/services/members/members-api/members-api.js
- apps/portal/package.json
- ghost/core/core/server/services/gifts/gift-bookshelf-repository.ts
- ghost/core/test/e2e-api/members/gift-subscriptions.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/server/services/gifts/gift-service.ts
| {/* TODO: Add translation strings once copy has been finalised */} | ||
| {/* eslint-disable i18next/no-literal-string */} | ||
| <p> | ||
| Your gift subscription is valid until <strong>{subscriptionExpiry}</strong>. If you continue with a paid subscription, the remaining days will be added as a free trial. | ||
| </p> |
There was a problem hiding this comment.
Localize banner copy with a single translatable string + interpolation.
Line 125 is hardcoded and bypasses i18n linting. This should use t() + Interpolate with {variable} placeholders.
💡 Suggested fix
- {/* TODO: Add translation strings once copy has been finalised */}
- {/* eslint-disable i18next/no-literal-string */}
<p>
- Your gift subscription is valid until <strong>{subscriptionExpiry}</strong>. If you continue with a paid subscription, the remaining days will be added as a free trial.
+ <Interpolate
+ string={t('Your gift subscription is valid until {expiryDate}. If you continue with a paid subscription, the remaining days will be added as a free trial.', {expiryDate: subscriptionExpiry})}
+ mapping={{
+ expiryDate: <strong>{subscriptionExpiry}</strong>
+ }}
+ />
</p>As per coding guidelines: “Never split sentences across multiple t() calls. Use @doist/react-interpolate to embed React elements within a single translatable string” and “Use {variable} syntax for dynamic values in translation strings.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {/* TODO: Add translation strings once copy has been finalised */} | |
| {/* eslint-disable i18next/no-literal-string */} | |
| <p> | |
| Your gift subscription is valid until <strong>{subscriptionExpiry}</strong>. If you continue with a paid subscription, the remaining days will be added as a free trial. | |
| </p> | |
| <p> | |
| <Interpolate | |
| string={t('Your gift subscription is valid until {expiryDate}. If you continue with a paid subscription, the remaining days will be added as a free trial.', {expiryDate: subscriptionExpiry})} | |
| mapping={{ | |
| expiryDate: <strong>{subscriptionExpiry}</strong> | |
| }} | |
| /> | |
| </p> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/portal/src/components/pages/account-plan-page.js` around lines 122 -
126, Replace the hardcoded paragraph that uses subscriptionExpiry with a single
translatable string using t() and Interpolate from `@doist/react-interpolate`:
create one t() call that includes a placeholder like "{expiry}" and pass
subscriptionExpiry as the interpolation value, embedding the <strong> element
via Interpolate so the bolded expiry remains a React element; update the JSX in
the AccountPlanPage component (where the <p> currently renders
subscriptionExpiry) to use t(...) + <Interpolate ... /> and remove the literal
string to satisfy i18n linting.
sagzy
force-pushed
the
gift-subscriptions/upgrade-gift-to-paid
branch
from
cc9e18c to
53a0fe7
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ghost/core/test/unit/server/services/gifts/gift-service.test.ts (1)
956-963: Add a repository-call assertion in the null-path test.This test currently validates output only. Also assert the repository is called with the expected member id to prevent false positives.
Suggested patch
it('returns null when the repository has no redeemed gift for the member', async function () { giftRepository.getRedeemedByMember.resolves(null); const service = createService(); const result = await service.getRedeemedByMember('member_without_gift'); assert.equal(result, null); + sinon.assert.calledOnceWithExactly(giftRepository.getRedeemedByMember, 'member_without_gift'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/gifts/gift-service.test.ts` around lines 956 - 963, Add an assertion in the "returns null when the repository has no redeemed gift for the member" test to verify the repository method was invoked with the expected member id: after calling service.getRedeemedByMember('member_without_gift') assert that giftRepository.getRedeemedByMember was called with 'member_without_gift' (e.g., using sinon/Chai call assertion such as sinon.assert.calledWith or assert.calledOnceWithExactly) so the test checks both the return value and that the repository was queried with the correct parameter.ghost/core/core/server/services/members/members-api/services/payments-service.js (1)
33-34: Consider adding JSDoc type annotation for consistency.Other dependencies in the constructor have JSDoc type annotations (e.g., lines 13-16). Adding one for
giftServicewould improve discoverability and IDE support.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/members/members-api/services/payments-service.js` around lines 33 - 34, Add a JSDoc type annotation for the giftService dependency to match the other constructor annotations: locate the constructor where this.giftService = deps.giftService is assigned and add a JSDoc line (similar to the existing annotations above it) documenting the expected type/interface for giftService (e.g., `@param` {GiftService} deps.giftService or `@type` {import('path').GiftService}) so IDEs and consumers can discover the type from the PaymentsService constructor/giftService field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ghost/core/core/server/services/members/members-api/services/payments-service.js`:
- Around line 95-107: The code assumes this.giftService exists when computing
gift-based trialDays; add a defensive guard to only call
this.giftService.service.getRedeemedByMember(member.id) if this.giftService and
this.giftService.service are defined (or otherwise skip gift logic), so that
PaymentsService can be instantiated without giftService; update the block around
the getRedeemedByMember call in payments-service.js (where giftService,
this.giftService.service and getRedeemedByMember are referenced) to check
existence before awaiting and proceed without error if absent.
---
Nitpick comments:
In
`@ghost/core/core/server/services/members/members-api/services/payments-service.js`:
- Around line 33-34: Add a JSDoc type annotation for the giftService dependency
to match the other constructor annotations: locate the constructor where
this.giftService = deps.giftService is assigned and add a JSDoc line (similar to
the existing annotations above it) documenting the expected type/interface for
giftService (e.g., `@param` {GiftService} deps.giftService or `@type`
{import('path').GiftService}) so IDEs and consumers can discover the type from
the PaymentsService constructor/giftService field.
In `@ghost/core/test/unit/server/services/gifts/gift-service.test.ts`:
- Around line 956-963: Add an assertion in the "returns null when the repository
has no redeemed gift for the member" test to verify the repository method was
invoked with the expected member id: after calling
service.getRedeemedByMember('member_without_gift') assert that
giftRepository.getRedeemedByMember was called with 'member_without_gift' (e.g.,
using sinon/Chai call assertion such as sinon.assert.calledWith or
assert.calledOnceWithExactly) so the test checks both the return value and that
the repository was queried with the correct parameter.
🪄 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: 5d865da2-340f-4682-a415-babcfc49e588
📒 Files selected for processing (10)
apps/portal/package.jsonapps/portal/src/components/pages/account-plan-page.jsghost/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/members/members-api/members-api.jsghost/core/core/server/services/members/members-api/services/payments-service.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.ts
✅ Files skipped from review due to trivial changes (5)
- apps/portal/package.json
- ghost/core/core/server/services/gifts/gift-repository.ts
- ghost/core/core/server/services/members/members-api/members-api.js
- ghost/core/test/unit/server/services/gifts/gift-bookshelf-repository.test.ts
- apps/portal/src/components/pages/account-plan-page.js
🚧 Files skipped from review as they are similar to previous changes (3)
- ghost/core/core/server/services/gifts/gift-service.ts
- ghost/core/core/server/services/gifts/gift-bookshelf-repository.ts
- ghost/core/test/e2e-api/members/gift-subscriptions.test.js
| // If active gift subscription, add remaining days as trial | ||
| if (trialDays === null && member) { | ||
| const gift = await this.giftService.service.getRedeemedByMember(member.id); | ||
|
|
||
| if (gift && gift.consumesAt) { | ||
| const now = new Date(); | ||
| const diffMs = gift.consumesAt.getTime() - now.getTime(); | ||
| const diffDays = Math.ceil(diffMs / (1000 * 60 * 60 * 24)); | ||
| if (diffDays > 0) { | ||
| trialDays = Math.min(diffDays, 730); // Stripe max trial period | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how giftService is constructed and passed to PaymentsService
# Find where PaymentsService is instantiated and giftService is passed
rg -n -A 5 "new PaymentsService" --type js
# Check the giftService structure to understand the .service accessor
rg -n -A 10 "giftService" ghost/core/core/server/services/members/members-api/members-api.jsRepository: TryGhost/Ghost
Length of output: 4313
🏁 Script executed:
# Check the PaymentsService constructor definition
rg -n -B 5 -A 30 "constructor\(" ghost/core/core/server/services/members/members-api/services/payments-service.js | head -60Repository: TryGhost/Ghost
Length of output: 1612
🏁 Script executed:
# Check all usages of giftService within PaymentsService
rg -n "giftService" ghost/core/core/server/services/members/members-api/services/payments-service.jsRepository: TryGhost/Ghost
Length of output: 196
🏁 Script executed:
# Check if giftService is optional in PaymentsService tests
rg -n -B 5 -A 5 "giftService" ghost/core/test/unit/server/services/members/members-api/services/payments-service.test.js | head -80Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Search for tests that might test the gift trial logic
rg -n -B 3 -A 10 "getRedeemedByMember\|gift.*trial" ghost/core/test/unit/server/services/members/members-api/services/payments-service.test.jsRepository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Check the PaymentsService instantiation in members-api.js more carefully, particularly the giftService parameter
cat -n ghost/core/core/server/services/members/members-api/members-api.js | sed -n '180,195p'Repository: TryGhost/Ghost
Length of output: 506
🏁 Script executed:
# Check what giftService actually is at the module level in members-api.js
rg -n "giftService\s*=" ghost/core/core/server/services/members/members-api/members-api.js | head -5Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Check broader context - what is giftService at the top of members-api.js
head -100 ghost/core/core/server/services/members/members-api/members-api.js | grep -n "giftService"Repository: TryGhost/Ghost
Length of output: 76
🏁 Script executed:
# Search for where giftService is passed/defined in members-api initialization
rg -n "giftService" ghost/core/core/server/services/members/members-api/ --type js | head -20Repository: TryGhost/Ghost
Length of output: 775
🏁 Script executed:
# Check if there are any integration tests that might cover this code path
rg -n "getRedeemedByMember" ghost/core/test --type jsRepository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Check how members-api is initialized and where giftService comes from
rg -n "members-api.init\|require.*members-api" ghost/core/core/server/services/members --type js | head -20Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Look at the function signature of members-api to understand dependencies
sed -n '60,100p' ghost/core/core/server/services/members/members-api/members-api.js | cat -nRepository: TryGhost/Ghost
Length of output: 1183
🏁 Script executed:
# Check if giftService is conditionally passed or always provided
rg -n -B 10 "members-api\.init" ghost/core/core/server/services/members/index.js | head -50Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Check members/index.js to see how members-api is initialized with giftService
cat ghost/core/core/server/services/members/index.js | head -150Repository: TryGhost/Ghost
Length of output: 96
🏁 Script executed:
# Look for where giftService service is defined/created
rg -n "giftService\s*=" ghost/core/core/server/services/ --type js | grep -v "this.giftService\|deps.giftService"Repository: TryGhost/Ghost
Length of output: 357
🏁 Script executed:
# Check the gifts service to understand the structure
cat ghost/core/core/server/services/gifts/index.js | head -80Repository: TryGhost/Ghost
Length of output: 163
🏁 Script executed:
# Check members/api.js where giftService is required and how it's used
cat -n ghost/core/core/server/services/members/api.js | head -50Repository: TryGhost/Ghost
Length of output: 2446
🏁 Script executed:
# Check the GiftServiceWrapper to understand the .service property
cat ghost/core/core/server/services/gifts/gift-service-wrapper.js | head -100Repository: TryGhost/Ghost
Length of output: 2064
🏁 Script executed:
# Check if there's any code path where giftService might not be provided
rg -n "giftService" ghost/core/core/server/services/members/api.jsRepository: TryGhost/Ghost
Length of output: 125
🏁 Script executed:
# Check if giftService.init() is called before PaymentsService is used
rg -n "giftService\.init\|await giftService" ghost/core/core/server/services/members/ --type jsRepository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Check the MembersApi initialization to see if giftService is properly set up
rg -n "async function createApiInstance\|giftService" ghost/core/core/server/services/members/api.js | head -20Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Check when giftService gets initialized - look for boot/initialization sequence
rg -n "giftService" ghost/core/core/server/services/members/index.jsRepository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Search for where giftService.init() is called in the initialization sequence
rg -n "\.init\(\)" ghost/core/core/server/services/ --type js | grep -i giftRepository: TryGhost/Ghost
Length of output: 154
🏁 Script executed:
# Check the main service initialization in core/server/services
rg -n "giftService\|gifts" ghost/core/core/server/services/index.jsRepository: TryGhost/Ghost
Length of output: 138
🏁 Script executed:
# Look for when PaymentsService methods are actually called and if giftService would be initialized
rg -n "paymentsService\|getTrialDaysFromTierOrOffer" ghost/core/core/server/services/members --type js | head -20Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Check the main initialization sequence in Ghost - find where services are initialized
find ghost/core/core/server -name "index.js" -o -name "init.js" | head -10Repository: TryGhost/Ghost
Length of output: 655
🏁 Script executed:
# Check Ghost's main entry points to understand initialization order
rg -n "giftService\|membersService\|init()" ghost/core/core/index.js --type js | head -30Repository: TryGhost/Ghost
Length of output: 122
🏁 Script executed:
# Search for where giftService.init() is called globally
rg -n "await.*giftService\.init\|giftService\.init" ghost/core --type jsRepository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Check the server startup sequence
find ghost/core/core/server -name "*.js" -type f | xargs grep -l "giftService\.init\|membersService" | head -10Repository: TryGhost/Ghost
Length of output: 698
🏁 Script executed:
# Look for where memberService/paymentsService methods are called with giftService context
rg -n "getTrialDaysFromTierOrOffer" ghost/core --type jsRepository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Check if giftService.service is ever checked for null/undefined in the codebase
rg -n "giftService\?.service\|giftService && giftService\.service\|giftService\.service\?" ghost/core --type jsRepository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Check if there's any initialization or setup code that validates giftService before PaymentsService is used
rg -n "PaymentsService\|paymentsService.*getTrialDaysFromTierOrOffer" ghost/core/core/server/services/members --type js -B 5 -A 5 | head -50Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Look at the actual test file to see if giftService is tested with PaymentsService
cat ghost/core/test/unit/server/services/members/members-api/services/payments-service.test.js | grep -n -A 10 -B 10 "gift\|trial" | head -80Repository: TryGhost/Ghost
Length of output: 2909
🏁 Script executed:
# Check if giftService is ever made optional or conditional in any initialization
rg -n "giftService.*\?" ghost/core/core/server/services/ --type jsRepository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Check the test for PaymentsService to see if giftService is ever tested with getRedeemedByMember
rg -n -B 5 -A 15 "getTrialDaysFromTierOrOffer\|gift.*trial" ghost/core/test/unit/server/services/members/members-api/services/payments-service.test.jsRepository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Check if the code at lines 95-107 is ever actually tested
rg -n "trialDays.*null\|giftService.*getRedeemedByMember" ghost/core/test --type jsRepository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Verify: can PaymentsService be instantiated without giftService?
cat -n ghost/core/test/unit/server/services/members/members-api/services/payments-service.test.js | sed -n '308,320p'Repository: TryGhost/Ghost
Length of output: 636
🏁 Script executed:
# Confirm: Check how many instances of PaymentsService are created in tests WITHOUT giftService
rg -n -B 2 -A 8 "new PaymentsService" ghost/core/test/unit/server/services/members/members-api/services/payments-service.test.js | grep -A 8 "new PaymentsService"Repository: TryGhost/Ghost
Length of output: 964
🏁 Script executed:
# Double-check: Are there ANY null/undefined guards before calling giftService in payments-service.js?
grep -n "giftService" ghost/core/core/server/services/members/members-api/services/payments-service.jsRepository: TryGhost/Ghost
Length of output: 196
Add defensive check for optional giftService parameter.
The code at line 97 unconditionally accesses this.giftService.service without checking if giftService was provided. Test cases show PaymentsService can be instantiated without giftService, making it optional, yet the code will throw TypeError: Cannot read property 'service' of undefined if absent.
Add the guard:
Suggested fix
// If active gift subscription, add remaining days as trial
- if (trialDays === null && member) {
+ if (trialDays === null && member && this.giftService) {
const gift = await this.giftService.service.getRedeemedByMember(member.id);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // If active gift subscription, add remaining days as trial | |
| if (trialDays === null && member) { | |
| const gift = await this.giftService.service.getRedeemedByMember(member.id); | |
| if (gift && gift.consumesAt) { | |
| const now = new Date(); | |
| const diffMs = gift.consumesAt.getTime() - now.getTime(); | |
| const diffDays = Math.ceil(diffMs / (1000 * 60 * 60 * 24)); | |
| if (diffDays > 0) { | |
| trialDays = Math.min(diffDays, 730); // Stripe max trial period | |
| } | |
| } | |
| } | |
| // If active gift subscription, add remaining days as trial | |
| if (trialDays === null && member && this.giftService) { | |
| const gift = await this.giftService.service.getRedeemedByMember(member.id); | |
| if (gift && gift.consumesAt) { | |
| const now = new Date(); | |
| const diffMs = gift.consumesAt.getTime() - now.getTime(); | |
| const diffDays = Math.ceil(diffMs / (1000 * 60 * 60 * 24)); | |
| if (diffDays > 0) { | |
| trialDays = Math.min(diffDays, 730); // Stripe max trial period | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ghost/core/core/server/services/members/members-api/services/payments-service.js`
around lines 95 - 107, The code assumes this.giftService exists when computing
gift-based trialDays; add a defensive guard to only call
this.giftService.service.getRedeemedByMember(member.id) if this.giftService and
this.giftService.service are defined (or otherwise skip gift logic), so that
PaymentsService can be instantiated without giftService; update the block around
the getRedeemedByMember call in payments-service.js (where giftService,
this.giftService.service and getRedeemedByMember are referenced) to check
existence before awaiting and proceed without error if absent.
1 participant
ref https://linear.app/ghost/issue/BER-3477
Summary
When a member with an active gift subscription continues with a paid plan, they currently lose the remaining days of their gift. This PR preserves that value by converting the remaining gift days into a Stripe free trial period.
Changes
Backend
getByRedeemerMemberId(memberId)to look up a gift by the member who redeemed it.getActiveByRedeemer(memberId)which returns the member's gift if it exists and hasn't been consumed.giftService. IngetPaymentLink(), if there's no offer-based trial set, we check for an active gift and convert the remaining days (capped at Stripe's 730-day max) intotrial_period_dayson the Stripe checkout session. The existing coupon logic still wins — if an offer adds a coupon, trial days are cleared.Trial day priority:
Frontend (Portal)
GiftTrialBannerto the "Change plan" page in Portal. For gift members, it shows above the plan cards with the message: