fix(subscriptions): prepare for the Stripe webhook version cutover - #21122
Open
david1alvarez wants to merge 1 commit into
Open
fix(subscriptions): prepare for the Stripe webhook version cutover#21122david1alvarez wants to merge 1 commit into
david1alvarez wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds backward-compatible Stripe webhook handling during the API-version transition.
Changes:
- Normalizes previous plan/price webhook shapes.
- Prevents renewal events from triggering upgrade emails.
- Adds Stripe API-version telemetry and subscription tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
stripe-webhook.ts |
Tags legacy webhook telemetry with API version. |
stripe.ts |
Derives previous pricing across Stripe payload shapes. |
stripe.spec.ts |
Tests payload normalization and renewal filtering. |
stripe-webhooks.service.ts |
Tags NestJS webhook telemetry with API version. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+2448
to
+2449
| interval: planOldDiff.interval ?? planNew.interval, | ||
| interval_count: planOldDiff.interval_count ?? planNew.interval_count, |
| request.payload, | ||
| request.headers['stripe-signature'] | ||
| ); | ||
| Sentry.setTag('stripe_api_version', event.api_version); |
Comment on lines
+33
to
+36
| Sentry.setTag( | ||
| 'stripe_api_version', | ||
| webhookEventResponse.event.api_version | ||
| ); |
Because: * The Stripe webhook endpoint's own api_version sets inbound payload shape, so the version flip has to be revertible without breaking parsing. * Basil removed Subscription.plan, which several consumers still read straight off the Firestore mirror. * The mirror is written only from SDK reads, so it already holds both shapes, and the sync script that has to migrate the rest could not tell them apart. This commit: * Reads the previous price from previous_attributes.plan, items.data[].plan or items.data[].price, and only when the price id moved. * Tags both Stripe webhook receivers with stripe_api_version. * Reads the subscription plan through singlePlan() in subscriptionsToResponse and the admin-server subscription list. * Flags mirror docs still carrying the top-level fields basil dropped, across subscriptions and their invoices. * Resyncs through StripeFirestore rather than a customer metadata write, behind a --dry-run defaulting to true. Addresses: PAY-3900
david1alvarez
force-pushed
the
PAY-3900
branch
from
September 1, 2026 22:03
8846b27 to
73ccaf8
Compare
david1alvarez
changed the title
Member
|
Is the check firestore stripe sync script the correct home for this migration? I think this deserves a separate script and shouldn't bear the load of this logic. |
julianpoy
reviewed
Sep 1, 2026
Comment on lines
+2432
to
+2436
| // Acacia sent the previous price as the top-level `plan`; basil removed | ||
| // that field and reports item changes under `items.data[]` instead — | ||
| // including the billing-period bump on every renewal, so only a moved | ||
| // price id is an actual upgrade. | ||
| // |
Member
There was a problem hiding this comment.
[nit] Mind removing these past-present looking comments? I think these types of comments should live in tickets rather than in code
Contributor
Author
There was a problem hiding this comment.
Generally agreed, though in this particular case it may be worth keeping despite its verbosity. We're going to be revisiting this to remove the dual-shape-handling code, and having this context at that point may be useful.
Want me to keep it with a // TODO: remove comment alongside code after Firestore-Stripe Sync script run, or to just remove it?
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
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.
Because
payloads don't match the shapes our code reads. We need to flip it and be able
to roll back.
Subscription.plan, and three consumers still read it — theupgrade/downgrade email off the webhook payload, and
/v1/accountplus theadmin-panel subscription list off the Firestore mirror.
stripe.*.retrieve, so it has been emittingbasil docs since the SDK pin landed and already holds both shapes. The sync
script that has to migrate the stragglers couldn't tell them apart.
This pull request
previous_attributes.plan(acacia) or thesubscription item's
plan/price(basil), treating an item diff as an upgradeonly when the price id actually moved — basil reports the billing-period bump
on every renewal, so without that guard every subscriber gets an upgrade notice
each cycle.
singlePlan()insubscriptionsToResponseand the admin-server subscription list, retiring an
as anyand a@ts-ignoreover the removed top-level
plan.stripe_api_version, covered on eachside including that it fires only after signature verification.
top-level fields basil dropped, across subscriptions and their invoices. It
previously compared only scalars present on the freshly fetched Stripe object,
so a stale doc differed from a current one exactly where it wasn't looking and
every one of them reported "in sync".
StripeFirestorerather than poking customermetadata to provoke a webhook, behind a
--dry-runthat defaults to true.Issue that this pull request solves
Addresses: PAY-3900
Checklist
Put an
xin the boxes that applyHow to review (Optional)
planOldinextractSubscriptionUpdateEventDetailsForEmail, and the shape detection plusresync path in
check-firestore-stripe-sync/.stripe.tsblock and its spec cases, then the twosinglePlan()call sites, then the sync script end to end.planOldis the only thing that sends anupgrade/downgrade email, and the id comparison is what keeps renewals out of
that path. The sync script is the migration vehicle for production mirror data,
so its failure modes matter more than its happy path — errors are logged and
swallowed per record rather than failing the run.
Screenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
version, running the migration, and removing the dual-format code are still
open — hence
Addresses:rather thanCloses:. The endpoint version has norepo artifact; it's a Stripe Dashboard setting.
grep -rn "PAY-3900: remove"returns the complete step-4 deletion list.the mirror's shape follows the SDK pin, so the flip can't pollute it and a
rollback can't leave mixed-shape docs behind. What it does mean is that the
mirror is mixed today, and only records untouched since the SDK pin are stale.
run()usesautoPagingEach,whose page fetches bypass the script's Stripe rate limiter, so
--rate-limitisn't a hard ceiling.
payment_methodsmirror docs get no shape check, sincebasil barely moved that object.