Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
const getDiscountWindow = require('../utils/get-discount-window');

/**
* @typedef {import('../../../../services/offers/application/offer-mapper').OfferDTO} OfferDTO
*/
Expand Down Expand Up @@ -101,42 +103,15 @@ class NextPaymentCalculator {
* @returns {ActiveDiscount|null}
*/
_getActiveDiscount(subscription, offer) {
// Offers are based on a Stripe coupon, with a discount_start / discount_end
if (subscription.discount_start) {
return {
start: subscription.discount_start,
end: subscription.discount_end
};
}

// Backportability for old signup offers without discount_start / discount_end
if (offer.redemption_type !== 'signup') {
// Skip if there's no Stripe discount data and the offer isn't eligible for legacy backport:
// - signup offers are backported for legacy data without discount_start
// - retention offers are excluded because they were introduced after discount_start
// was available, so missing discount_start means there's no active discount
if (!subscription.discount_start && offer.redemption_type !== 'signup') {
return null;
}

// Signup offers with once have already been applied to first payment
if (offer.duration === 'once') {
return null;
}

// Signup offer with forever don't expire
if (offer.duration === 'forever') {
return {start: subscription.start_date, end: null};
}

// Signup repeating offer expire after start_date + duration_in_months
if (offer.duration === 'repeating' && offer.duration_in_months > 0) {
const end = new Date(subscription.start_date);
end.setUTCMonth(end.getUTCMonth() + offer.duration_in_months);

if (new Date() >= end) {
return null;
}

return {start: subscription.start_date, end};
}

return null;
return getDiscountWindow(subscription, offer);
}
Comment on lines +106 to 115
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Expired/future Stripe windows can be treated as active discounts.

Line 112 returns getDiscountWindow(...) directly. Since Stripe windows may include past end values, _getActiveDiscount can return non-active discounts and calculate() will still reduce the next payment amount.

Suggested fix
     _getActiveDiscount(subscription, offer) {
         // Skip if there's no Stripe discount data and the offer isn't eligible for legacy backport:
         // - signup offers are backported for legacy data without discount_start
         // - retention offers are excluded because they were introduced after discount_start
         //   was available, so missing discount_start means there's no active discount
         if (!subscription.discount_start && offer.redemption_type !== 'signup') {
             return null;
         }

-        return getDiscountWindow(subscription, offer);
+        const discountWindow = getDiscountWindow(subscription, offer);
+        if (!discountWindow) {
+            return null;
+        }
+
+        const now = new Date();
+        const start = discountWindow.start ? new Date(discountWindow.start) : null;
+        const end = discountWindow.end ? new Date(discountWindow.end) : null;
+
+        if (start && now < start) {
+            return null;
+        }
+
+        if (end && now >= end) {
+            return null;
+        }
+
+        return discountWindow;
     }
📝 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.

Suggested change
// Skip if there's no Stripe discount data and the offer isn't eligible for legacy backport:
// - signup offers are backported for legacy data without discount_start
// - retention offers are excluded because they were introduced after discount_start
// was available, so missing discount_start means there's no active discount
if (!subscription.discount_start && offer.redemption_type !== 'signup') {
return null;
}
// Signup offers with once have already been applied to first payment
if (offer.duration === 'once') {
return null;
}
// Signup offer with forever don't expire
if (offer.duration === 'forever') {
return {start: subscription.start_date, end: null};
}
// Signup repeating offer expire after start_date + duration_in_months
if (offer.duration === 'repeating' && offer.duration_in_months > 0) {
const end = new Date(subscription.start_date);
end.setUTCMonth(end.getUTCMonth() + offer.duration_in_months);
if (new Date() >= end) {
return null;
}
return {start: subscription.start_date, end};
}
return null;
return getDiscountWindow(subscription, offer);
}
// Skip if there's no Stripe discount data and the offer isn't eligible for legacy backport:
// - signup offers are backported for legacy data without discount_start
// - retention offers are excluded because they were introduced after discount_start
// was available, so missing discount_start means there's no active discount
if (!subscription.discount_start && offer.redemption_type !== 'signup') {
return null;
}
const discountWindow = getDiscountWindow(subscription, offer);
if (!discountWindow) {
return null;
}
const now = new Date();
const start = discountWindow.start ? new Date(discountWindow.start) : null;
const end = discountWindow.end ? new Date(discountWindow.end) : null;
if (start && now < start) {
return null;
}
if (end && now >= end) {
return null;
}
return discountWindow;
}
🤖 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/next-payment-calculator.js`
around lines 104 - 113, _getActiveDiscount currently returns the raw result of
getDiscountWindow(subscription, offer), which can include expired or future
windows; update _getActiveDiscount to call getDiscountWindow(subscription,
offer), then verify the returned window is actually active (e.g., check a
returned window.active flag or ensure window.start <= Date.now() < window.end)
and return null if it is not active, so calculate() only sees active discounts.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was existing behaviour before this refactor right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mike182uk Yes, you're absolutely right! Looking at the old code, when subscription.discount_start was present, it directly returned the discount window without any validation:

if (subscription.discount_start) {
    return {
        start: subscription.discount_start,
        end: subscription.discount_end
    };
}

There was no check to verify whether the discount was actually active (e.g., whether end had passed or start was in the future). The refactor preserves this existing behavior. I apologize for flagging this as if it were a new issue introduced by the refactor.


🧠 Learnings used
Learnt from: rob-ghost
Repo: TryGhost/Ghost PR: 25791
File: ghost/core/core/server/api/endpoints/member-comment-ban.js:64-68
Timestamp: 2026-01-08T10:26:38.700Z
Learning: In the Ghost API, endpoints rely on the serialization layer to prepare frame.data[docName] as a non-empty array before query() executes. Endpoints access frame.data[docName][0] directly (e.g., frame.data.comment_bans[0], frame.data.members[0], frame.data.posts[0]) without per-endpoint validation. This pattern is common across API endpoints. When maintaining or creating endpoints, avoid duplicating validation for frame.data[docName] and ensure the serializer guarantees the shape and non-emptiness. If you add a new endpoint that uses this frame.data[docName], follow the same assumption and avoid redundant checks unless there's a documented exception.

Learnt from: rob-ghost
Repo: TryGhost/Ghost PR: 26219
File: ghost/core/test/e2e-api/members-comments/comments.test.js:939-983
Timestamp: 2026-02-04T15:58:09.124Z
Learning: In Ghost core tests and code that interact with the Ghost comments API, count.replies is a backward-compatible alias for count.total_replies (all descendants via parent_id) and does not represent direct replies. The new field count.direct_replies returns tree-native direct reply counts. Reviewers should verify any code paths, tests, or API surface areas that rely on count.replies are preserved for compatibility, and consider updating or adding tests to cover count.direct_replies for direct counts. When updating or adding tests, ensure behavior is documented and that any assertions reflect the distinction between total (including descendants) and direct reply counts to avoid regressions in API consumer expectations.


/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/**
* Computes the discount window for a subscription based on available data.
* Returns {start, end} if a discount window can be determined, null otherwise.
*
* Handles two data paths:
* 1. Stripe coupon discounts (post-6.16) - uses discount_start / discount_end
* 2. Legacy fallback - computes from offer duration and start_date
*
* @param {object} subscription - plain object with: discount_start, discount_end, start_date
* @param {object|null} offer - offer data with: duration, duration_in_months.
* Pass null to skip offer-dependent checks.
* @returns {{start: *, end: *}|null}
*/
module.exports = function getDiscountWindow(subscription, offer) {
// Stripe coupon discount (post-6.16 data)
if (subscription.discount_start) {
return {
start: subscription.discount_start,
end: subscription.discount_end || null
};
}

// Legacy fallback: compute window from offer duration
if (!offer) {
return null;
}

if (offer.duration === 'once') {
return null;
}

if (offer.duration === 'forever') {
return {start: subscription.start_date, end: null};
}

if (offer.duration === 'repeating' && offer.duration_in_months > 0) {
const end = new Date(subscription.start_date);
end.setUTCMonth(end.getUTCMonth() + offer.duration_in_months);

if (new Date() >= end) {
return null;
}

return {start: subscription.start_date, end};
}

return null;
};
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
const getDiscountWindow = require('./get-discount-window');

/**
* Determines if a subscription currently has an active offer.
* Uses discount_start/discount_end (synced from Stripe) when available,
Expand All @@ -8,16 +10,21 @@
* @returns {Promise<boolean>}
*/
module.exports = async function hasActiveOffer(subscriptionModel, offersAPI) {
const discountStart = subscriptionModel.get('discount_start');
const discountEnd = subscriptionModel.get('discount_end');
const trialEndAt = subscriptionModel.get('trial_end_at');
const subscriptionData = {
discount_start: subscriptionModel.get('discount_start'),
discount_end: subscriptionModel.get('discount_end'),
start_date: subscriptionModel.get('start_date')
};

// Check for active Stripe discount (post-6.16 data)
if (discountStart) {
return !discountEnd || new Date(discountEnd) > new Date();
// discount_start takes precedence over trial and legacy fallback
const discountWindow = getDiscountWindow(subscriptionData, null);
if (discountWindow) {
return !discountWindow.end || new Date(discountWindow.end) > new Date();
}

// Check for active trial (trial offers)
const trialEndAt = subscriptionModel.get('trial_end_at');
if (trialEndAt && new Date(trialEndAt) > new Date()) {
return true;
}
Expand All @@ -35,26 +42,14 @@ module.exports = async function hasActiveOffer(subscriptionModel, offersAPI) {
return false;
}

if (offer.duration === 'forever') {
return true;
}

if (offer.duration === 'once') {
return false; // once = already applied and expired
const legacyWindow = getDiscountWindow(subscriptionData, offer);
if (legacyWindow) {
return !legacyWindow.end || new Date(legacyWindow.end) > new Date();
}

if (offer.duration === 'repeating' && offer.duration_in_months > 0) {
const startDate = new Date(subscriptionModel.get('start_date'));
const end = new Date(startDate);

end.setUTCMonth(end.getUTCMonth() + offer.duration_in_months);

return new Date() < end;
}
return false;
} catch (e) {
// If we can't look up the offer, err on the side of blocking
return true;
}

return false;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
const assert = require('node:assert/strict');
const getDiscountWindow = require('../../../../../../../core/server/services/members/members-api/utils/get-discount-window');

describe('getDiscountWindow', function () {
function createSubscription(overrides = {}) {
return {
discount_start: null,
discount_end: null,
start_date: new Date('2025-01-01T00:00:00.000Z'),
...overrides
};
}

// Stripe coupon discount (post-6.16 data)

describe('stripe coupon discount', function () {
it('returns window when discount_start is present with discount_end', function () {
const discountStart = new Date('2025-05-01T00:00:00.000Z');
const discountEnd = new Date('2025-11-01T00:00:00.000Z');
const subscription = createSubscription({
discount_start: discountStart,
discount_end: discountEnd
});

const result = getDiscountWindow(subscription, null);

assert.deepEqual(result, {start: discountStart, end: discountEnd});
});

it('returns window with null end for forever discounts', function () {
const discountStart = new Date('2025-05-01T00:00:00.000Z');
const subscription = createSubscription({
discount_start: discountStart,
discount_end: null
});

const result = getDiscountWindow(subscription, null);

assert.deepEqual(result, {start: discountStart, end: null});
});

it('returns window even when discount_end is in the past (trusts Stripe data)', function () {
const discountStart = new Date('2025-01-01T00:00:00.000Z');
const discountEnd = new Date('2025-06-01T00:00:00.000Z');
const subscription = createSubscription({
discount_start: discountStart,
discount_end: discountEnd
});

const result = getDiscountWindow(subscription, null);

assert.deepEqual(result, {start: discountStart, end: discountEnd});
});

it('discount_start takes precedence over legacy offer data', function () {
const discountStart = new Date('2025-05-01T00:00:00.000Z');
const subscription = createSubscription({
discount_start: discountStart,
discount_end: null
});

const result = getDiscountWindow(subscription, {duration: 'once'});

assert.deepEqual(result, {start: discountStart, end: null});
});
});

// Legacy fallback (no discount_start, uses offer duration)

describe('legacy fallback', function () {
it('returns null when no offer is provided', function () {
const subscription = createSubscription();

const result = getDiscountWindow(subscription, null);

assert.equal(result, null);
});

it('returns null for once duration', function () {
const subscription = createSubscription();

const result = getDiscountWindow(subscription, {duration: 'once'});

assert.equal(result, null);
});

it('returns window with null end for forever duration', function () {
const startDate = new Date('2025-01-01T00:00:00.000Z');
const subscription = createSubscription({start_date: startDate});

const result = getDiscountWindow(subscription, {duration: 'forever'});

assert.deepEqual(result, {start: startDate, end: null});
});

it('returns window for repeating offer still within duration', function () {
const startDate = new Date(Date.now() - 90 * 24 * 60 * 60 * 1000); // 3 months ago
const subscription = createSubscription({start_date: startDate});

const result = getDiscountWindow(subscription, {duration: 'repeating', duration_in_months: 6});

assert.notEqual(result, null);
assert.deepEqual(result.start, startDate);
assert.ok(result.end instanceof Date);
assert.ok(result.end > new Date());
});

it('returns null for repeating offer past its duration', function () {
const startDate = new Date(Date.now() - 365 * 24 * 60 * 60 * 1000); // 1 year ago
const subscription = createSubscription({start_date: startDate});

const result = getDiscountWindow(subscription, {duration: 'repeating', duration_in_months: 6});

assert.equal(result, null);
});

it('returns null for repeating offer with zero duration_in_months', function () {
const subscription = createSubscription();

const result = getDiscountWindow(subscription, {duration: 'repeating', duration_in_months: 0});

assert.equal(result, null);
});

it('returns null for unknown duration', function () {
const subscription = createSubscription();

const result = getDiscountWindow(subscription, {duration: 'unknown'});

assert.equal(result, null);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ describe('hasActiveOffer', function () {
});

it('returns true for a forever offer (legacy data)', async function () {
const model = createSubscriptionModel({offerId: 'offer_123'});
const model = createSubscriptionModel({offerId: 'offer_123', startDate: new Date('2025-01-01')});
const offersAPI = createOffersAPI({duration: 'forever'});
const result = await hasActiveOffer(model, offersAPI);

Expand Down