Excluded gift-to-paid upgrades from the cancelled subscriptions count

[mike182uk]

ref https://linear.app/tryghost/issue/BER-3611/

• when a gift member upgraded to a paid subscription, the gift was being marked as consumed which counted as a cancellation in the Paid subscriptions chart, even though the member had actually converted to a paying customer (not churned)
• distinguish upgrades from natural gift endings by comparing consumed_at to consumes_at: the cron-driven natural-end path only fires once consumes_at has passed, so consumed_at >= consumes_at, whereas the upgrade path consumes the gift mid-period
• this prevents the chart from showing misleading cancellations for members who upgraded, which understated retention

[coderabbitai]

Walkthrough

The changes add a new consumes_at column to the gifts table schema to track when gifts end. The subscription statistics query is updated to include a filter that excludes consumed gifts where the member upgraded to a paid plan (identified by consumed_at < consumes_at), preventing these upgrades from being counted as churn. Test cases are modified to accommodate the new column and validate the updated filtering behavior.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly and clearly describes the main change: excluding gift-to-paid upgrades from cancellation counts, which is the core objective of this pull request.
Description check	✅ Passed	The description clearly explains the problem (gifts marked as consumed were incorrectly counted as cancellations), the solution (comparing consumed_at to consumes_at), and the benefit (preventing misleading cancellation data).
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch BER-3611-analytics-exclude-gift-upgrades

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@ghost/core/core/server/services/stats/subscription-stats-service.js`:
- Around line 189-195: The WHERE clause in subscription-stats-service.js uses
whereRaw with the expression NOT (status = 'consumed' AND consumed_at <
consumes_at), which becomes NULL when consumes_at is NULL and thus incorrectly
filters out rows; update the condition in the whereRaw call (the line that
builds the SQL filter in subscription-stats-service.js) to explicitly handle
NULLs by requiring consumes_at IS NOT NULL before comparing consumed_at <
consumes_at (i.e., only treat a gift as an upgrade if status = 'consumed' AND
consumes_at IS NOT NULL AND consumed_at < consumes_at), ensuring rows with NULL
consumes_at are not dropped.

🪄 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: bec7380a-6119-4a9f-bc99-f546d054acde

📥 Commits

Reviewing files that changed from the base of the PR and between c8352ba and 047f4bd.

📒 Files selected for processing (2)

• ghost/core/core/server/services/stats/subscription-stats-service.js
• ghost/core/test/unit/server/services/stats/subscriptions.test.js

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify SQL NULL behavior for the current predicate shape.
python - <<'PY'
import sqlite3
con = sqlite3.connect(":memory:")
cur = con.cursor()
cur.execute("create table gifts(status text, consumed_at text, consumes_at text)")
cur.executemany(
    "insert into gifts values (?, ?, ?)",
    [
        ("consumed", "1970-01-05", "1970-02-01"),  # upgrade (should exclude)
        ("consumed", "1970-01-05", "1970-01-05"),  # natural end (should include)
        ("consumed", "1970-01-05", None),          # nullable consumes_at case
    ],
)
print("Current predicate:")
for row in cur.execute("""
    select status, consumed_at, consumes_at,
           NOT (status='consumed' AND consumed_at < consumes_at) as keep
    from gifts
"""):
    print(row)
print("\nRows kept by WHERE with current predicate:")
for row in cur.execute("""
    select status, consumed_at, consumes_at
    from gifts
    where NOT (status='consumed' AND consumed_at < consumes_at)
"""):
    print(row)
PY

Repository: TryGhost/Ghost

Length of output: 289

---

🏁 Script executed:

# Check if the file exists and examine lines 189-195
head -195 ghost/core/core/server/services/stats/subscription-stats-service.js | tail -20

Repository: TryGhost/Ghost

Length of output: 1292

---

🏁 Script executed:

# Search for the schema definition of gifts table to confirm consumes_at is nullable
find . -type f -name "*.js" -o -name "*.json" | xargs grep -l "consumes_at" | head -10

Repository: TryGhost/Ghost

Length of output: 711

---

🏁 Script executed:

# Check migration or schema files for gifts table definition
find . -type f \( -name "*migration*" -o -name "*schema*" \) | xargs grep -l "consumes_at" 2>/dev/null | head -10

Repository: TryGhost/Ghost

Length of output: 105

---

🏁 Script executed:

# Check the schema definition for the gifts table
grep -A 50 "gifts" ghost/core/core/server/data/schema/schema.js | head -60

Repository: TryGhost/Ghost

Length of output: 2086

---

🏁 Script executed:

# Also check the migration file to confirm consumes_at nullability
cat ghost/core/core/server/data/migrations/versions/6.27/2026-04-06-07-48-06-add-gifts-table.js

Repository: TryGhost/Ghost

Length of output: 1619

---

Make the upgrade exclusion NULL-safe (Line 195).

NOT (status = 'consumed' AND consumed_at < consumes_at) evaluates to NULL when consumes_at is NULL, so those rows are dropped by WHERE. Since consumes_at is nullable in the schema, this undercounts consumed-gift cancellations.

Suggested fix

-                .whereRaw(`NOT (status = 'consumed' AND consumed_at < consumes_at)`)
+                .whereRaw(`NOT (status = 'consumed' AND consumes_at IS NOT NULL AND consumed_at < consumes_at)`)

📝 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

	// Exclude gifts that were consumed because the member upgraded
	// to a paid subscription. In that case `consumed_at` is set
	// before the gift's planned end (`consumes_at`). When the gift
	// ends naturally via the cron job, `consumed_at` is always
	// >= `consumes_at`, so this condition cleanly distinguishes
	// upgrades (not churn) from natural endings (real churn).
	.whereRaw(`NOT (status = 'consumed' AND consumed_at < consumes_at)`)
	// Exclude gifts that were consumed because the member upgraded
	// to a paid subscription. In that case `consumed_at` is set
	// before the gift's planned end (`consumes_at`). When the gift
	// ends naturally via the cron job, `consumed_at` is always
	// >= `consumes_at`, so this condition cleanly distinguishes
	// upgrades (not churn) from natural endings (real churn).
	.whereRaw(`NOT (status = 'consumed' AND consumes_at IS NOT NULL AND consumed_at < consumes_at)`)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ghost/core/core/server/services/stats/subscription-stats-service.js` around
lines 189 - 195, The WHERE clause in subscription-stats-service.js uses whereRaw
with the expression NOT (status = 'consumed' AND consumed_at < consumes_at),
which becomes NULL when consumes_at is NULL and thus incorrectly filters out
rows; update the condition in the whereRaw call (the line that builds the SQL
filter in subscription-stats-service.js) to explicitly handle NULLs by requiring
consumes_at IS NOT NULL before comparing consumed_at < consumes_at (i.e., only
treat a gift as an upgrade if status = 'consumed' AND consumes_at IS NOT NULL
AND consumed_at < consumes_at), ensuring rows with NULL consumes_at are not
dropped.

[mike182uk]

consumes_at cannot be null if a gift has redeemed though?

[coderabbitai]

🧩 Analysis chain

Good question — let me verify the schema constraints on consumes_at to check whether it can actually be NULL on a redeemed gift.

---

🏁 Script executed:

#!/bin/bash
# Check the schema definition for consumes_at in the gifts table
grep -A 5 "consumes_at" ghost/core/core/server/data/schema/schema.js

# Also check any migration that adds consumes_at
find . -type f -name "*.js" | xargs grep -l "consumes_at" 2>/dev/null | head -10

# Check each of those files for nullability/default constraints
for f in $(find . -type f -name "*.js" | xargs grep -l "consumes_at" 2>/dev/null | head -10); do
    echo "=== $f ==="; grep -B2 -A5 "consumes_at" "$f"
done

Length of output: 12618

---

@mike182uk, you're right. Looking at the evidence from the schema and tests:

• While the schema defines consumes_at as nullable: true, that nullable state only applies to purchased (unredeemed) gifts before the redemption flow sets it.
• The gift-subscriptions.test.js redemption tests all assert assert.ok(gift.get('consumes_at')) after redemption, confirming the application layer always populates it at redemption time.
• Since the query already filters with .whereNotNull('redeemed_at'), all rows in scope are redeemed (or further along), and consumes_at will always be set for those rows.

My original concern was overly cautious — the NULL case cannot arise within the filtered result set due to the application-level invariant. The current predicate NOT (status = 'consumed' AND consumed_at < consumes_at) is safe as-is.

( ˘▽˘)っ🐇

---

🧠 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.

---

Learnt from: vershwal
Repo: TryGhost/Ghost PR: 27290
File: ghost/core/package.json:76-77
Timestamp: 2026-04-09T09:44:26.783Z
Learning: In the TryGhost/Ghost monorepo, treat `tryghost/admin-api-schema` as the single abstraction layer over AJV version differences. Do not raise code review findings for AJV-internal error field changes (e.g., `dataPath` → `instancePath` between AJV v6 and v8) when evaluating Ghost consumer code. The consumer-facing error contract for this package (`ValidationError` with `message`, `property`, `errorDetails`) is expected to remain stable, and Ghost wrapper code should not inspect raw AJV error objects—so review should focus on the stable `ValidationError` shape rather than AJV internals.