Fixed next payment details for free month offers#26608
Conversation
WalkthroughA new helper 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
ref https://linear.app/ghost/issue/BER-3376 - we've recently switched to using a Stripe coupon for free-month offers (instead of a trial period) - in this transition, the next billing date and amount were left inconsistent in Admin / Portal
sagzy
force-pushed
the
fix/next-month-display
branch
from
6bc85cd to
12d9bf2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/portal/test/unit/components/pages/AccountHomePage/account-home-page.test.js (1)
60-61: Prefer runtime-consistent subscription keys in this fixture.Using
current_period_end/next_paymenthere would better mirror production payload shape and make this test less dependent on fixture normalization behavior.Suggested change
- currentPeriodEnd: currentPeriodEnd.toISOString(), - nextPayment: getNextPaymentData({ + current_period_end: currentPeriodEnd.toISOString(), + next_payment: getNextPaymentData({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/portal/test/unit/components/pages/AccountHomePage/account-home-page.test.js` around lines 60 - 61, Test fixture uses camelCase keys currentPeriodEnd and nextPayment; change them to runtime-consistent snake_case keys so the fixture matches production payloads. Replace currentPeriodEnd: currentPeriodEnd.toISOString() with current_period_end: currentPeriodEnd.toISOString() and replace nextPayment: getNextPaymentData({...}) with next_payment: getNextPaymentData({...}) in the account home page test fixture so the keys align with production naming; keep the same values and calls (referencing currentPeriodEnd and getNextPaymentData) but use current_period_end and next_payment as the object property names.
🤖 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/AccountHomePage/components/account-welcome.js`:
- Around line 49-54: The renewal date assignment may become an empty string when
getFreeMonthsOfferRenewalDate returns '' for incomplete metadata; update the
renewalDate logic (the ternary using subscriptionHasFreeMonthsOffer,
getFreeMonthsOfferRenewalDate, getDateString and currentPeriodEnd) to fall back
to getDateString(currentPeriodEnd) whenever getFreeMonthsOfferRenewalDate
returns a falsy/empty value (e.g., call the helper, check its result, and use
getDateString if the result is empty) so the UI never receives an empty renewal
date.
In
`@apps/portal/src/components/pages/AccountHomePage/components/paid-account-actions.js`:
- Around line 206-213: Normalize subscription?.offer?.duration_in_months to a
number before formatting the label: coerce the value used to compute months (the
variable currently named months) so it is a Number (not a string) prior to
building monthsText and label; keep using getFreeMonthsOfferRenewalDate as-is
(it already coerces durationInMonths) but ensure the same numeric value is used
for monthsText and label to avoid "1 months free" when duration_in_months is a
string. Reference variables/functions: subscription?.offer?.duration_in_months,
months, getFreeMonthsOfferRenewalDate, monthsText, label.
In `@ghost/admin/app/utils/subscription-data.js`:
- Around line 93-104: getFreeMonthsRenewalDate currently trusts
freeMonthsOffer.duration_in_months and sub.current_period_end and can produce an
invalid date; update the function to defensively validate both values: after
extracting durationInMonths from freeMonthsOffer, check
Number.isFinite(durationInMonths) and durationInMonths > 0 (or >= 0 if zero
months allowed), and construct currentPeriodEnd using
moment.utc(sub.current_period_end) then verify currentPeriodEnd.isValid(); if
either check fails return undefined, otherwise proceed with
Math.trunc(durationInMonths), add months to currentPeriodEnd and format the
result as before; reference getFreeMonthsRenewalDate, freeMonthsOffer,
durationInMonths, and currentPeriodEnd when locating the code to change.
In `@ghost/admin/tests/unit/utils/subscription-data-test.js`:
- Around line 308-313: The fixtures for the free-month offers are missing
redemption_type and therefore won't satisfy the isFreeMonthsOffer predicate;
update the offer objects (e.g., the fixtures with id 'offer_1' used in these
tests) to include redemption_type: 'retention' so the predicate
(isFreeMonthsOffer) evaluates true and the free-month code paths are exercised.
---
Nitpick comments:
In
`@apps/portal/test/unit/components/pages/AccountHomePage/account-home-page.test.js`:
- Around line 60-61: Test fixture uses camelCase keys currentPeriodEnd and
nextPayment; change them to runtime-consistent snake_case keys so the fixture
matches production payloads. Replace currentPeriodEnd:
currentPeriodEnd.toISOString() with current_period_end:
currentPeriodEnd.toISOString() and replace nextPayment:
getNextPaymentData({...}) with next_payment: getNextPaymentData({...}) in the
account home page test fixture so the keys align with production naming; keep
the same values and calls (referencing currentPeriodEnd and getNextPaymentData)
but use current_period_end and next_payment as the object property names.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/portal/package.jsonapps/portal/src/components/pages/AccountHomePage/components/account-welcome.jsapps/portal/src/components/pages/AccountHomePage/components/paid-account-actions.jsapps/portal/src/components/pages/account-plan-page.jsapps/portal/src/utils/helpers.jsapps/portal/test/unit/components/pages/AccountHomePage/account-home-page.test.jsapps/portal/test/unit/components/pages/AccountHomePage/paid-account-actions.test.jsapps/portal/test/utils/helpers.test.jsghost/admin/app/utils/subscription-data.jsghost/admin/tests/unit/utils/subscription-data-test.js
| const renewalDate = subscriptionHasFreeMonthsOffer({sub: subscription}) | ||
| ? subscription?.next_payment?.discount?.end | ||
| : currentPeriodEnd; | ||
| ? getFreeMonthsOfferRenewalDate({ | ||
| currentPeriodEnd, | ||
| durationInMonths: subscription?.offer?.duration_in_months | ||
| }) | ||
| : getDateString(currentPeriodEnd); |
There was a problem hiding this comment.
Ensure renewal date falls back when helper returns an empty string.
When free-month metadata is incomplete, getFreeMonthsOfferRenewalDate returns '', which can render an empty renewal date in the UI.
Proposed fix
- const renewalDate = subscriptionHasFreeMonthsOffer({sub: subscription})
- ? getFreeMonthsOfferRenewalDate({
- currentPeriodEnd,
- durationInMonths: subscription?.offer?.duration_in_months
- })
- : getDateString(currentPeriodEnd);
+ const freeMonthsRenewalDate = subscriptionHasFreeMonthsOffer({sub: subscription})
+ ? getFreeMonthsOfferRenewalDate({
+ currentPeriodEnd,
+ durationInMonths: subscription?.offer?.duration_in_months
+ })
+ : '';
+ const renewalDate = freeMonthsRenewalDate || getDateString(currentPeriodEnd);Also applies to: 58-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/portal/src/components/pages/AccountHomePage/components/account-welcome.js`
around lines 49 - 54, The renewal date assignment may become an empty string
when getFreeMonthsOfferRenewalDate returns '' for incomplete metadata; update
the renewalDate logic (the ternary using subscriptionHasFreeMonthsOffer,
getFreeMonthsOfferRenewalDate, getDateString and currentPeriodEnd) to fall back
to getDateString(currentPeriodEnd) whenever getFreeMonthsOfferRenewalDate
returns a falsy/empty value (e.g., call the helper, check its result, and use
getDateString if the result is empty) so the UI never receives an empty renewal
date.
| const months = subscription?.offer?.duration_in_months ?? 0; | ||
| const discountEnd = nextPayment?.discount?.end; | ||
| const renewalDate = discountEnd ? getDateString(discountEnd) : null; | ||
| const renewalDate = getFreeMonthsOfferRenewalDate({ | ||
| currentPeriodEnd: subscription?.current_period_end, | ||
| durationInMonths: months | ||
| }); | ||
| const monthsText = months === 1 ? '1 month free' : `${months} months free`; | ||
| const label = renewalDate ? `${monthsText} - Renews ${renewalDate}` : monthsText; | ||
|
|
There was a problem hiding this comment.
Normalize duration_in_months before label formatting.
getFreeMonthsOfferRenewalDate already coerces durationInMonths to a number, but monthsText uses raw months. If duration_in_months is "1", UI becomes 1 months free.
Proposed fix
function FreeMonthsLabel({subscription}) {
- const months = subscription?.offer?.duration_in_months ?? 0;
+ const months = Math.trunc(Number(subscription?.offer?.duration_in_months ?? 0));
const renewalDate = getFreeMonthsOfferRenewalDate({
currentPeriodEnd: subscription?.current_period_end,
durationInMonths: months
});
const monthsText = months === 1 ? '1 month free' : `${months} months free`;
const label = renewalDate ? `${monthsText} - Renews ${renewalDate}` : monthsText;📝 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.
| const months = subscription?.offer?.duration_in_months ?? 0; | |
| const discountEnd = nextPayment?.discount?.end; | |
| const renewalDate = discountEnd ? getDateString(discountEnd) : null; | |
| const renewalDate = getFreeMonthsOfferRenewalDate({ | |
| currentPeriodEnd: subscription?.current_period_end, | |
| durationInMonths: months | |
| }); | |
| const monthsText = months === 1 ? '1 month free' : `${months} months free`; | |
| const label = renewalDate ? `${monthsText} - Renews ${renewalDate}` : monthsText; | |
| const months = Math.trunc(Number(subscription?.offer?.duration_in_months ?? 0)); | |
| const renewalDate = getFreeMonthsOfferRenewalDate({ | |
| currentPeriodEnd: subscription?.current_period_end, | |
| durationInMonths: months | |
| }); | |
| const monthsText = months === 1 ? '1 month free' : `${months} months free`; | |
| const label = renewalDate ? `${monthsText} - Renews ${renewalDate}` : monthsText; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/portal/src/components/pages/AccountHomePage/components/paid-account-actions.js`
around lines 206 - 213, Normalize subscription?.offer?.duration_in_months to a
number before formatting the label: coerce the value used to compute months (the
variable currently named months) so it is a Number (not a string) prior to
building monthsText and label; keep using getFreeMonthsOfferRenewalDate as-is
(it already coerces durationInMonths) but ensure the same numeric value is used
for monthsText and label to avoid "1 months free" when duration_in_months is a
string. Reference variables/functions: subscription?.offer?.duration_in_months,
months, getFreeMonthsOfferRenewalDate, monthsText, label.
| function getFreeMonthsRenewalDate(sub = {}) { | ||
| const freeMonthsOffer = sub.offer; | ||
|
|
||
| if (!isFreeMonthsOffer(freeMonthsOffer)) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const durationInMonths = Number(freeMonthsOffer.duration_in_months); | ||
| const currentPeriodEnd = moment.utc(sub.current_period_end); | ||
|
|
||
| return currentPeriodEnd.add(Math.trunc(durationInMonths), 'months').format('D MMM YYYY'); | ||
| } |
There was a problem hiding this comment.
Add defensive validation before returning free-month renewal date.
getFreeMonthsRenewalDate should return undefined for invalid duration_in_months / current_period_end; otherwise it can override validUntil with an invalid value.
Proposed fix
function getFreeMonthsRenewalDate(sub = {}) {
const freeMonthsOffer = sub.offer;
if (!isFreeMonthsOffer(freeMonthsOffer)) {
return undefined;
}
- const durationInMonths = Number(freeMonthsOffer.duration_in_months);
- const currentPeriodEnd = moment.utc(sub.current_period_end);
-
- return currentPeriodEnd.add(Math.trunc(durationInMonths), 'months').format('D MMM YYYY');
+ const durationInMonths = Number(freeMonthsOffer.duration_in_months);
+ const months = Math.trunc(durationInMonths);
+ const currentPeriodEnd = moment.utc(sub.current_period_end);
+
+ if (!Number.isFinite(durationInMonths) || months < 1 || !currentPeriodEnd.isValid()) {
+ return undefined;
+ }
+
+ return currentPeriodEnd.add(months, 'months').format('D MMM YYYY');
}📝 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.
| function getFreeMonthsRenewalDate(sub = {}) { | |
| const freeMonthsOffer = sub.offer; | |
| if (!isFreeMonthsOffer(freeMonthsOffer)) { | |
| return undefined; | |
| } | |
| const durationInMonths = Number(freeMonthsOffer.duration_in_months); | |
| const currentPeriodEnd = moment.utc(sub.current_period_end); | |
| return currentPeriodEnd.add(Math.trunc(durationInMonths), 'months').format('D MMM YYYY'); | |
| } | |
| function getFreeMonthsRenewalDate(sub = {}) { | |
| const freeMonthsOffer = sub.offer; | |
| if (!isFreeMonthsOffer(freeMonthsOffer)) { | |
| return undefined; | |
| } | |
| const durationInMonths = Number(freeMonthsOffer.duration_in_months); | |
| const months = Math.trunc(durationInMonths); | |
| const currentPeriodEnd = moment.utc(sub.current_period_end); | |
| if (!Number.isFinite(durationInMonths) || months < 1 || !currentPeriodEnd.isValid()) { | |
| return undefined; | |
| } | |
| return currentPeriodEnd.add(months, 'months').format('D MMM YYYY'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ghost/admin/app/utils/subscription-data.js` around lines 93 - 104,
getFreeMonthsRenewalDate currently trusts freeMonthsOffer.duration_in_months and
sub.current_period_end and can produce an invalid date; update the function to
defensively validate both values: after extracting durationInMonths from
freeMonthsOffer, check Number.isFinite(durationInMonths) and durationInMonths >
0 (or >= 0 if zero months allowed), and construct currentPeriodEnd using
moment.utc(sub.current_period_end) then verify currentPeriodEnd.isValid(); if
either check fails return undefined, otherwise proceed with
Math.trunc(durationInMonths), add months to currentPeriodEnd and format the
result as before; reference getFreeMonthsRenewalDate, freeMonthsOffer,
durationInMonths, and currentPeriodEnd when locating the code to change.
| id: 'offer_1', | ||
| type: 'percent', | ||
| amount: 100, | ||
| duration: 'repeating' | ||
| duration: 'repeating', | ||
| duration_in_months: 1 | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the structure of the test file and examine the fixtures at the specified lines
wc -l ghost/admin/tests/unit/utils/subscription-data-test.jsRepository: TryGhost/Ghost
Length of output: 116
🏁 Script executed:
# Check the fixtures at the specified lines
sed -n '300,320p' ghost/admin/tests/unit/utils/subscription-data-test.jsRepository: TryGhost/Ghost
Length of output: 835
🏁 Script executed:
# Check lines 497-503
sed -n '490,510p' ghost/admin/tests/unit/utils/subscription-data-test.jsRepository: TryGhost/Ghost
Length of output: 844
🏁 Script executed:
# Check lines 659-665
sed -n '652,672p' ghost/admin/tests/unit/utils/subscription-data-test.jsRepository: TryGhost/Ghost
Length of output: 832
🏁 Script executed:
# Search for isFreeMonthsOffer definition in the codebase
fd -t f "subscription-data.js$" | xargs -I {} sh -c 'echo "File: {}"; rg -n "isFreeMonthsOffer" "{}" -A 10'Repository: TryGhost/Ghost
Length of output: 1695
Add redemption_type: 'retention' to test fixtures for free-month offers.
The isFreeMonthsOffer predicate checks four conditions:
return offer?.type === 'percent'
&& offer?.amount === 100
&& offer?.duration === 'repeating'
&& offer?.redemption_type === 'retention';These fixtures omit redemption_type: 'retention', so they will not match the predicate. The tests at lines 308-313, 497-503, and 659-665 won't exercise the free-month code paths as intended.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ghost/admin/tests/unit/utils/subscription-data-test.js` around lines 308 -
313, The fixtures for the free-month offers are missing redemption_type and
therefore won't satisfy the isFreeMonthsOffer predicate; update the offer
objects (e.g., the fixtures with id 'offer_1' used in these tests) to include
redemption_type: 'retention' so the predicate (isFreeMonthsOffer) evaluates true
and the free-month code paths are exercised.
![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: 6bc85cd0ce
ℹ️ 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".
| @@ -1,7 +1,9 @@ | |||
| import moment from 'moment-timezone'; | |||
| import {getNonDecimal, getSymbol} from 'ghost-admin/utils/currency'; | |||
| import {isFreeMonthsOffer} from 'ghost-admin/utils/helpers'; | |||
There was a problem hiding this comment.
This import references ghost-admin/utils/helpers, but there is no corresponding utils/helpers module in the admin app tree, so Ember module resolution will fail when subscription-data is built or loaded. That makes this a release-blocking issue for any surface that imports this util; either add the missing module or keep the free-month predicate local in this file.
Useful? React with 👍 / 👎.
| }, | ||
| startDate: sub.start_date ? moment(sub.start_date).format('D MMM YYYY') : '-', | ||
| validUntil: validUntil(sub), | ||
| validUntil: freeMonthsRenewalDate || validUntil(sub), |
There was a problem hiding this comment.
Setting validUntil to freeMonthsRenewalDate || validUntil(sub) bypasses the existing canceled-immediately rule in validUntil(sub) (status === 'canceled' && !cancel_at_period_end), so canceled free-month subscriptions can now show a fabricated future date instead of the intentionally blank value. This regresses the current behavior in exactly the case where we do not store the exact cancellation timestamp.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/portal/src/components/pages/AccountHomePage/components/paid-account-actions.js (1)
205-212:⚠️ Potential issue | 🟡 MinorNormalize
monthsbefore building the label.The strict equality comparison
months === 1assumes a number type. While the backend enforcesduration_in_monthsas an integer, adding explicit normalization would be consistent with defensive patterns used ingetFreeMonthsOfferRenewalDateand other similar code throughout the codebase. This prevents unintended "1 months free" output if the type assumption is ever violated.Proposed fix
function FreeMonthsLabel({subscription}) { - const months = subscription?.offer?.duration_in_months ?? 0; + const months = Number(subscription?.offer?.duration_in_months ?? 0); const renewalDate = getFreeMonthsOfferRenewalDate({ currentPeriodEnd: subscription?.current_period_end, durationInMonths: months });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/portal/src/components/pages/AccountHomePage/components/paid-account-actions.js` around lines 205 - 212, The label logic in FreeMonthsLabel assumes subscription?.offer?.duration_in_months is a number; normalize it first (e.g., convert to a Number or parseInt with a safe fallback to 0) and use that normalized value (e.g., normalizedMonths) everywhere: for the strict check (normalizedMonths === 1), for building monthsText, and when calling getFreeMonthsOfferRenewalDate (pass durationInMonths: normalizedMonths) so the label never shows "1 months free" if the input is a string or other type.
🧹 Nitpick comments (1)
ghost/admin/tests/unit/utils/subscription-data-test.js (1)
657-675: Consider addingpriceproperty for consistency.Unlike other tests in this
getDiscountPricedescribe block (lines 602-654), this test fixture omits thepriceproperty. While the function may return early for free-months offers without needing it, includingpricewould maintain consistency with other tests and ensure the test remains valid if the implementation changes.it('returns null for free months offers', function () { const result = getDiscountPrice({ offer: { id: 'offer_1', type: 'percent', amount: 100, duration: 'repeating', - duration_in_months: 1 + duration_in_months: 1, + redemption_type: 'retention' }, + price: {currency: 'usd', amount: 5000}, next_payment: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/admin/tests/unit/utils/subscription-data-test.js` around lines 657 - 675, The test fixture for getDiscountPrice omits the price property; update the test case in subscription-data-test.js (the 'returns null for free months offers' spec) to include a price object matching the payment values (e.g., price: {amount: 5000, currency: 'usd'}) so the fixture is consistent with other tests and resilient if getDiscountPrice later relies on price; ensure the price.amount and price.currency align with next_payment.original_amount and next_payment.currency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@apps/portal/src/components/pages/AccountHomePage/components/paid-account-actions.js`:
- Around line 205-212: The label logic in FreeMonthsLabel assumes
subscription?.offer?.duration_in_months is a number; normalize it first (e.g.,
convert to a Number or parseInt with a safe fallback to 0) and use that
normalized value (e.g., normalizedMonths) everywhere: for the strict check
(normalizedMonths === 1), for building monthsText, and when calling
getFreeMonthsOfferRenewalDate (pass durationInMonths: normalizedMonths) so the
label never shows "1 months free" if the input is a string or other type.
---
Nitpick comments:
In `@ghost/admin/tests/unit/utils/subscription-data-test.js`:
- Around line 657-675: The test fixture for getDiscountPrice omits the price
property; update the test case in subscription-data-test.js (the 'returns null
for free months offers' spec) to include a price object matching the payment
values (e.g., price: {amount: 5000, currency: 'usd'}) so the fixture is
consistent with other tests and resilient if getDiscountPrice later relies on
price; ensure the price.amount and price.currency align with
next_payment.original_amount and next_payment.currency.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/portal/package.jsonapps/portal/src/components/pages/AccountHomePage/components/account-welcome.jsapps/portal/src/components/pages/AccountHomePage/components/paid-account-actions.jsapps/portal/src/components/pages/account-plan-page.jsapps/portal/src/utils/helpers.jsapps/portal/test/unit/components/pages/AccountHomePage/account-home-page.test.jsapps/portal/test/unit/components/pages/AccountHomePage/paid-account-actions.test.jsapps/portal/test/utils/helpers.test.jsghost/admin/app/utils/subscription-data.jsghost/admin/tests/unit/utils/subscription-data-test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/portal/test/utils/helpers.test.js
- apps/portal/src/components/pages/AccountHomePage/components/account-welcome.js
- ghost/admin/app/utils/subscription-data.js
1 participant
ref https://linear.app/ghost/issue/BER-3376
Before
After