Pricing models item split - #784
Draft
mitchelljkotler wants to merge 14 commits into
Draft
Conversation
mitchelljkotler
force-pushed
the
pricing-models-item-split
branch
from
August 27, 2026 20:10
895d18a to
f50f2fb
Compare
mitchelljkotler
temporarily deployed
to
squarelet-pi-pricing-mo-aoqhqk
August 27, 2026 20:10
Inactive
Member
Author
|
Claude notes: The riskiest, and the checklist is longest. The core thing to prove is the one-invoice behaviour, in Stripe's dashboard rather than in squarelet:
Then the cancellation matrix: cancelling one line of two leaves the other billing and leaves both items on Stripe until period end; cancelling the only line sets cancel_at_period_end; resubscribe reverses each. And one interface check I'd not skip: diff an organization's OIDC entitlements payload before and after the migration. That's the contract MuckRock and DocumentCloud consume. It should be byte-identical — same entries, same resources, same quantity — but it's the thing that breaks other teams if I'm wrong. Two caveats on the checklists: they assume Stripe test keys in the preview environment, and #782/#784 both assume consolidate_stripe_products has been run there first, since the backfill needs the prices to exist. |
mitchelljkotler
force-pushed
the
pricing-models-item-split
branch
from
August 27, 2026 21:02
f50f2fb to
a3df14a
Compare
mitchelljkotler
temporarily deployed
to
squarelet-pi-pricing-mo-aoqhqk
August 27, 2026 21:03
Inactive
mitchelljkotler
temporarily deployed
to
squarelet-pi-pricing-mo-hudumy
September 1, 2026 13:57
Inactive
Follows the model split by moving every call site onto the right half of the pair, and fixes the behaviour that the split had silently changed. Adding a plan now joins the organization's existing subscription when the billing shape matches, so an org receives one invoice instead of several. SubscriptionItemQuerySet.start() picks the subscription by interval and collection method, creates it only when nothing matches, and otherwise pushes the new line onto the live Stripe subscription. Fixes carried in with it: - Subscription.free read item.free, which is a Plan attribute - Subscription.cancel() called send_slack_notification, which is on the line; it now announces every line it cancelled - SubscriptionItem had no cancel(), so removing one plan cancelled the whole subscription. The last line cancels the subscription at period end; any other is dropped from Stripe with proration suppressed - subscription_cancelled() and the invoice metadata assumed one plan per subscription - the admin's plan filter used to_attr on a nested prefetch path, so plan_subscriptions was always None - the Mailchimp journey and "started" Slack notification were lost in the split; both are back on the line, which is what names a plan - webhook handlers looked up Stripe subscription ids on the line - a bulk rename had mangled user-facing strings into "SubscriptionItem Payment", "SubscriptionItem cancelled." and similar SubscriptionItem.organization is a read-only property reaching through the parent, so the column stays on Subscription and a line can never disagree with the subscription it bills on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Removing one plan from a multi-plan subscription dropped it from Stripe immediately, while removing an org's only plan let them keep it until the period they had paid for ran out. Same button, different outcome depending on how many other plans the customer happened to have. SubscriptionItem now carries `cancelled` and `cancel_at`, mirroring Subscription. Cancelling a line flags it and leaves it billing; the line still grants access, and `uncancel()` reverses it. Stripe has no per-item cancel_at_period_end, so `restore_organization` is what enforces it: once `cancel_at` arrives the line is removed from the Stripe subscription with proration suppressed, since it has already been paid for through the end of the period. That sweep runs at 00:05, comfortably ahead of the renewal invoice for any subscription whose period ends later in the day. A subscription whose period ends within the first hour after midnight could still be invoiced for a line that was due to go; see the note in the plan. test_subscription.py is split into TestSubscription and TestSubscriptionItem, which also keeps either class under the public method limit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two things the test suite could not see, because neither path had coverage. `get_subscription_next_date()` read `stripe_subscription` off the object it was handed, but both callers hand it a SubscriptionItem and that property moved to the parent. Manage Subscriptions and Cancel Subscription raised AttributeError on render. There are two copies of the function; the unused one in organizations/views is fixed too rather than left as a landmine. Cancelling a whole subscription left its lines unflagged, so a customer who cancelled their only subscription still saw it listed as active -- the templates iterate lines, not subscriptions. Subscription.cancel() now flags every line it carries and uncancel() clears them, and the renewal sweep skips lines whose whole subscription is already going, so it never tries to remove them from Stripe individually. Not addressed here, both pre-existing: get_subscription_next_date makes one live Stripe call per line on every page render, and payments/views sets `cost` from `plan.base_price`, ignoring price_per_user. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Cancel and resubscribe are already per-line -- both views key on a SubscriptionItem pk -- but the model behind them got two cases wrong. `cancel()` asked whether the subscription had one item, not one *active* item. Cancelling two lines one at a time therefore flagged both and never cancelled the subscription, leaving Stripe to renew it; the sweep would then have tried to delete the subscription's only remaining line, which Stripe rejects. It now counts lines that are not already cancelled, so the second cancel is recognised as the last one. `uncancel()` cleared the line's own flag even when the whole subscription was cancelled, so a customer could resubscribe, see the line listed as active, and still have Stripe stop it at period end. Reviving a line on a cancelled subscription now revives the subscription and every line on it, which is symmetric with cancelling the last line taking them all. Also removes a stray `print(subscription)` from the resubscribe view. Packs still cancel independently of the base plan they extend, which is wrong but needs a relationship the schema does not have yet -- written up under "Add-on Packs — Follow-up Work" in the plan, together with the missing pack-quantity UI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Manage Subscriptions renders one row per subscription line and asked Stripe for the period end on every one, so a customer with three lines made three live API calls just to draw the page. `current_period_end` is already cached on Subscription, kept current by the webhook and verified by `audit_subscriptions`, so a new `next_date` property reads it directly and the page costs one query. The list is select_related on subscription and plan, so walking to the parent adds nothing. The conversion is the part worth care: the cached value is UTC, and a period ending just after midnight UTC is still the previous evening locally. `localtime()` keeps the date the customer sees the same as before; there is a test pinning exactly that case. Both copies of `get_subscription_next_date` are gone -- the one in organizations/views was dead already. Also restores a comment that a bulk rename had turned into "SubscriptionItem views". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both commands walked SubscriptionItem and did select_related on "organization", which is a property now -- so both raised FieldError on their first query. Worse, both filtered `subscription_id=None`, which still runs but now refers to the FK rather than the Stripe id, so the "no Stripe ID" report would have come back empty while real records went unreported. Neither command had any test coverage. Both are subscription-level operations: subscription_id, stripe_status and current_period_end all describe the Stripe subscription rather than any one line on it. They now walk Subscription and prefetch its items, and report every plan on a subscription instead of a single slug. The audit's comparison is the part that actually needed rethinking. It read items.data[0] throughout, which silently compares the wrong line as soon as a subscription bills more than one. _compare_items now matches each local line to its Stripe item by stripe_item_id and reports lines missing from Stripe, quantity and price drift per line, and -- the case that costs money -- Stripe items we have no local record of at all. Three bugs found while verifying against the dev database: - sync_subscriptions caught InvalidRequestError, but retrieve() swallows it and returns None, so a subscription deleted on Stripe crashed with AttributeError instead of being reported - it cached stripe_status but saved only current_period_end, dropping every status correction it made - iterator() after prefetch_related() needs a chunk_size; both call sites materialise the queryset anyway Also removes imports orphaned by deleting get_subscription_next_date. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The container sets HOME to /app, which is the repo bind-mount, so anything writing to $HOME lands in the working tree. .cache and .ipython/ are already ignored for this reason; the Stripe library adds .config/stripe/telemetry_id the first time a command talks to Stripe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Folds 0084's related-name AlterFields into the rename that caused them -- they were a second generated pass, and nothing here is deployed -- then renumbers so the chain is 0083 rename, 0084 parent, 0085 cancellation. Rehearsing the down path against a scratch database showed it did not work, in three separate ways: - Reversing re-added SubscriptionItem.organization as NOT NULL before split_back_out could populate it. The column is now relaxed in 0083, one migration earlier, so reversing re-adds it nullable, fills it, and restores NOT NULL in a transaction of its own. Postgres will not ALTER a table in the same transaction that just wrote to it, which is why the relaxation cannot simply sit next to the data move. - split_back_out deleted every Subscription row, queueing deferred foreign-key triggers that blocked the DDL following it. Reversing CreateModel drops that table anyway, so the delete was redundant. - The remaining writes left their own pending triggers, so the reverse now ends with SET CONSTRAINTS ALL IMMEDIATE to flush them. Verified by seeding a subscription at head, migrating back to 0082 and forward again: organization, subscription_id, cancelled, interval, collection_method and quantity all survive the round trip. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An organization holds one subscription per billing shape, so two annual plans share a subscription. Subscription.auto_renew was `all(item.plan.auto_renew ...)`, which meant adding a single auto_renew=False plan would set cancel_at_period_end on the whole subscription and cancel the renewing lines along with it. The subscription now renews while any line still wants to. A non-renewing plan instead flags its own line at start, with cancel_at taken from the period end, so the existing sweep drops just that line -- the same path a line the customer cancelled takes, which also means Resubscribe already reverses it. A subscription ends only once every line has stopped. The sweep will not strip a subscription's last line: Stripe rejects removing the only item, and in that case it is already cancelling the subscription itself at period end. Latent until now, since no plan sets auto_renew=False, but it would have fired the first time one did. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The rename commit turned Subscription into SubscriptionItem here but not the semantics that went with it, and the result was worse than a crash. is_billing read `subscription_id` off the line, which after the split is the foreign key column and therefore always set. Every line would have looked like it was billing, and every comped organization -- years of admin-granted free access -- would have been pointed at a paid price. The Stripe id lives on the parent, so it reads that. _pending had the same confusion in its exclusion, and select_related on "organization" -- a property now, not a relation -- so the command in fact died on a FieldError before reaching any of it. That is the only reason the mis-classification was never going to reach production. Uniqueness also moved: SubscriptionItem is unique on (subscription, plan), not (organization, plan), so the collision check keys on the parent subscription. Lines on different subscriptions of one organization no longer collide, and the test says so. Also fixes an order-dependent fixture I introduced with these tests. It built a plan at a canonical slug without checking whether the migration had already seeded one; when it had, AutoSlugField quietly made `professional-2` and the command could not find its targets. That passed or failed depending on whether a transactional test had flushed the database first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both values it showed were reading attributes that do not exist. `organization.plan` was removed from the model years ago and `subscription.update_on` was never on a subscription at all; Django resolves a missing attribute to the empty string rather than raising, so the page told every paying customer their plan was "Free" and rendered "Subscription ends on" with no date. Both predate this branch, but the split is what makes them fixable: `get_plans()` lists what an organization actually holds, and `cancel_at` now exists on the line. Listing plans rather than one also matches what the split allows -- an organization can hold a tier and a pack at once. The block moves to its own include so it can be tested. It sat above a crispy form that talks to Stripe, which is why nothing covered it and why both attributes could rot unnoticed. Also stops the view picking an arbitrary "first" line for the ends-on banner: it prefers a line that is actually ending, so the banner no longer depends on plan ordering. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mitchelljkotler
force-pushed
the
pricing-models-item-split
branch
from
September 1, 2026 18:11
a3df14a to
135cb72
Compare
mitchelljkotler
temporarily deployed
to
squarelet-pi-pricing-mo-hudumy
September 1, 2026 18:12
Inactive
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.
Split subscriptions into subscriptions and subscription items to allow multiple subscriptions to be on the same invoice