Folded gift signups into Monthly / Annual on the paid subscription breakdown chart#27624
Merged
Folded gift signups into Monthly / Annual on the paid subscription breakdown chart#27624
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughFrontend: removed gift-specific logic and the 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 2/5 reviews remaining, refill in 24 minutes and 1 second. Comment |
mike182uk
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/stats/subscription-stats-service.js`:
- Around line 15-17: The code concatenates paidDeltas and giftDeltas into
subscriptionDeltaEntries which breaks chronological order before
getSubscriptionHistory consumes the stream; before calling
getSubscriptionHistory (and similarly where else paid/gift arrays are merged
around lines 158-185) merge or sort the combined array by the delta timestamp
(ascending) so deltas are applied in time order — e.g., replace simple concat
with a merge/sort of fetchAllSubscriptionDeltas() and fetchAllGiftDeltas()
results (use the delta date field used by getSubscriptionHistory, and add a
secondary sort key for same-day tie-breaking if needed).
🪄 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: e6d6ddb4-c86a-4b12-b96c-ee361167ee05
📒 Files selected for processing (4)
apps/stats/src/views/Stats/Growth/components/new-subscribers-cadence.tsxapps/stats/test/unit/components/growth/new-subscribers-cadence.test.tsxghost/core/core/server/services/stats/subscription-stats-service.jsghost/core/test/unit/server/services/stats/subscriptions.test.js
sagzy
requested changes
Apr 29, 2026
| .select(knex.raw('COUNT(id) as count')) | ||
| .groupByRaw('DATE(redeemed_at), tier_id, cadence'), | ||
| knex('gifts') | ||
| .whereIn('status', ['consumed', 'expired', 'refunded']) |
Contributor
There was a problem hiding this comment.
A gift can be expired or refunded without ever been redeemed - in this case, they shouldn't be counted as cancellations?
Adding .whereNotNull('redeemed_at') to the cancellations query should do the trick
mike182uk
force-pushed
the
growth-pie-gift-members
branch
from
24966d1 to
93f94eb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/stats/subscription-stats-service.js`:
- Around line 20-22: After merging paidDeltas and giftDeltas into
subscriptionDeltaEntries, aggregate entries with the same date, tier and cadence
into a single entry (summing numeric delta fields like
countDelta/revenueDelta/etc.) before sorting and before the rollback loop;
update the code that creates subscriptionDeltaEntries (and the analogous block
at lines 187-190) to group by the composite key
(moment(date).format('YYYY-MM-DD') or equivalent normalized date, tier, cadence)
and replace duplicates with a single aggregated entry so the subsequent rollback
history loop emits one snapshot per date/tier/cadence.
🪄 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: 915e1762-2a0b-48bc-85b5-8dc19c0677a4
📒 Files selected for processing (4)
apps/stats/src/views/Stats/Growth/components/new-subscribers-cadence.tsxapps/stats/test/unit/components/growth/new-subscribers-cadence.test.tsxghost/core/core/server/services/stats/subscription-stats-service.jsghost/core/test/unit/server/services/stats/subscriptions.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/stats/test/unit/components/growth/new-subscribers-cadence.test.tsx
- apps/stats/src/views/Stats/Growth/components/new-subscribers-cadence.tsx
- ghost/core/test/unit/server/services/stats/subscriptions.test.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/stats/subscription-stats-service.js`:
- Around line 15-22: The current baseline totals used to seed the rollback
(values from fetchSubscriptionCounts() -> counts / meta.totals) don't include
active redeemed gifts, causing undercounted histories; update the logic so the
starting totals include active gifts too—either by extending
fetchSubscriptionCounts() to union in active gift rows (e.g., gifts WHERE
status='redeemed') mapped to the same schema, or by calling a new helper (e.g.,
fetchActiveGiftTotals()) and merging its results into counts/meta.totals before
using subscriptionDeltaEntries (from aggregateDeltas and fetchAllGiftDeltas) to
roll back; ensure the merged totals use the same tier/cadence keys as the
subscription counts so the rollback loop produces correct snapshots.
🪄 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: 87d6e37e-28dc-4f5d-b35e-8c3bd48567cd
📒 Files selected for processing (2)
ghost/core/core/server/services/stats/subscription-stats-service.jsghost/core/test/unit/server/services/stats/subscriptions.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/test/unit/server/services/stats/subscriptions.test.js
…eakdown chart ref https://linear.app/ghost/issue/BER-3491 - the "Paid subscription breakdown" pie chart was showing "Gift" as a separate slice alongside Monthly/Annual/Complimentary, which mixed billing-period semantics with member-status semantics - gifts have an inherent cadence (month/year) stored on the gifts table, so they belong in the corresponding billing-period bucket rather than as their own slice - surfaced gifts through the existing subscription-stats endpoint by adding `fetchAllGiftDeltas` (gift redemptions as signups, consumed/expired/refunded gifts as cancellations) so the chart's existing per-cadence aggregation picks them up with no special-casing in the frontend - preferred extending subscription-stats over a new endpoint to keep the change small; the side effect is that gifts now also count toward the "Paid subscriptions" daily new/cancelled chart, which is consistent with treating gifts as paid acquisitions
mike182uk
force-pushed
the
growth-pie-gift-members
branch
from
3bcce5e to
d0535c3
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/test/unit/server/services/stats/subscriptions.test.js (1)
396-427: ⚡ Quick winCover the remaining redeemed gift cancellation branches.
The new suite exercises
consumedand pre-redemptionrefunded, but it still misses redeemed→expiredand redeemed→refunded. A regression in either timestamp/filter path would still pass here.Possible extension
+ await db('gifts').insert([ + { + id: 'g-expired', + tier_id: 'basic', + cadence: 'year', + status: 'expired', + redeemed_at: '1970-01-01T00:00:00.000Z', + expired_at: '1970-01-03T00:00:00.000Z' + }, + { + id: 'g-refunded-after-redemption', + tier_id: 'basic', + cadence: 'year', + status: 'refunded', + redeemed_at: '1970-01-01T00:00:00.000Z', + refunded_at: '1970-01-04T00:00:00.000Z' + } + ]); + + assert.equal(sumWhere('basic', 'year', '1970-01-03', 'cancellations'), 1); + assert.equal(sumWhere('basic', 'year', '1970-01-04', 'cancellations'), 1);Also applies to: 477-497
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/stats/subscriptions.test.js` around lines 396 - 427, Add tests covering redeemed→expired and redeemed→refunded gift branches: when inserting test gifts via db('gifts').insert (the same area that currently creates 'g-month' and 'g-year'), add entries for a redeemed gift that later becomes expired (set redeemed_at and expired_at timestamps) and a redeemed gift that later becomes refunded (set redeemed_at and refunded_at timestamps). Then extend the assertions that use sumWhere (helper alongside createTiers, createEvent, insertEvents and the call to SubscriptionStatsService.getSubscriptionHistory) to check that signups include those redeemed rows on their redeemed_at dates and that cancellations (or the appropriate cancellation-type field) appear on the expired_at and refunded_at dates respectively; repeat the same coverage for the similar test block around lines 477-497.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/test/unit/server/services/stats/subscriptions.test.js`:
- Around line 396-427: Add tests covering redeemed→expired and redeemed→refunded
gift branches: when inserting test gifts via db('gifts').insert (the same area
that currently creates 'g-month' and 'g-year'), add entries for a redeemed gift
that later becomes expired (set redeemed_at and expired_at timestamps) and a
redeemed gift that later becomes refunded (set redeemed_at and refunded_at
timestamps). Then extend the assertions that use sumWhere (helper alongside
createTiers, createEvent, insertEvents and the call to
SubscriptionStatsService.getSubscriptionHistory) to check that signups include
those redeemed rows on their redeemed_at dates and that cancellations (or the
appropriate cancellation-type field) appear on the expired_at and refunded_at
dates respectively; repeat the same coverage for the similar test block around
lines 477-497.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2530157-32c0-47f7-907a-25b63ec044ee
📒 Files selected for processing (4)
apps/stats/src/views/Stats/Growth/components/new-subscribers-cadence.tsxapps/stats/test/unit/components/growth/new-subscribers-cadence.test.tsxghost/core/core/server/services/stats/subscription-stats-service.jsghost/core/test/unit/server/services/stats/subscriptions.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/stats/src/views/Stats/Growth/components/new-subscribers-cadence.tsx
- ghost/core/core/server/services/stats/subscription-stats-service.js
sagzy
approved these changes
Apr 30, 2026
mike182uk
added a commit
that referenced
this pull request
May 7, 2026
ref https://linear.app/ghost/issue/BER-3615 ref #27624 ref #27703 - the "Paid subscriptions" bar chart and "Paid subscription breakdown" pie chart were counting gift redemptions as signups and gift end-of-life events as cancellations, which mixed gift activity into charts that are meant to track paid Stripe activity only - gifts now flow through their own member status (`gift`) and surface in the Paid members KPI tooltip instead, so duplicating them in the per- cadence/per-tier breakdowns double-counted activity and made the bar and pie charts disagree - reverted the gift-aware additions to subscription-stats-service so `/stats/subscriptions` returns paid Stripe deltas and counts only; `gift`→`paid` upgrades still appear via the regular paid subscription event flow once the trial converts and MRR begins
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref https://linear.app/ghost/issue/BER-3491
fetchAllGiftDeltas(gift redemptions as signups, consumed/expired/refunded gifts as cancellations) so the chart's existing per-cadence aggregation picks them up with no special-casing in the frontend