Pricing models - #781
Open
mitchelljkotler wants to merge 19 commits into
Open
Conversation
mitchelljkotler
temporarily deployed
to
squarelet-pi-pricing-mo-axc6qu
August 26, 2026 15:39
Inactive
mitchelljkotler
force-pushed
the
pricing-models
branch
from
August 27, 2026 13:03
df41037 to
8a26f70
Compare
mitchelljkotler
temporarily deployed
to
squarelet-pi-pricing-mo-axc6qu
August 27, 2026 13:03
Inactive
mitchelljkotler
temporarily deployed
to
squarelet-pi-pricing-mo-axc6qu
August 27, 2026 20:08
Inactive
Member
Author
|
Claude notes: Additive schema plus Stripe plumbing; changes nobody's bill. PlanPrice, Plan.product/stripe_product_id, the ensure_stripe_* methods, the admin, and consolidate_stripe_products. Highest-value checks: add a price through the admin and confirm it appears in Stripe with squarelet_variant metadata; try editing that price's amount and confirm it's refused; run consolidate_stripe_products twice and confirm the second run creates nothing. I ran consolidate_stripe_products twice and it worked as expected. I also ensured that existing price's have read only fields for fields which shouldn't change. |
mitchelljkotler
temporarily deployed
to
squarelet-pi-pricing-mo-fyxwgw
September 1, 2026 13:57
Inactive
mitchelljkotler
force-pushed
the
pricing-models
branch
from
September 1, 2026 15:50
36f3d20 to
cb0277d
Compare
mitchelljkotler
temporarily deployed
to
squarelet-pi-pricing-mo-fyxwgw
September 1, 2026 15:50
Inactive
mitchelljkotler
force-pushed
the
pricing-models
branch
from
September 1, 2026 15:56
cb0277d to
95c30e2
Compare
mitchelljkotler
temporarily deployed
to
squarelet-pi-pricing-mo-fyxwgw
September 1, 2026 15:56
Inactive
mitchelljkotler
marked this pull request as ready for review
September 1, 2026 17:24
6 tasks
A Stripe Price with no `recurring` block cannot be a subscription item, and access here is granted exclusively through subscription lines -- so a one_time PlanPrice would bill correctly and grant nothing. It was also reachable from the admin dropdown, where saving one raised ValueError out of create_price and returned a 500. Nothing used it. A plan that should charge once and stop already has a mechanism: Plan.auto_renew=False creates an ordinary subscription with cancel_at_period_end set, so the customer keeps access for the period they paid for and the existing Resubscribe action renews it if they want. A real one-off purchase -- a credit top-up, a setup fee -- needs its own model, and the questions that come with it (expiry, refunds, whether it grants entitlements at all) should shape that design rather than be guessed at now. Re-adding the choice then is one line. Folded into 0081 rather than added as a new migration: nothing here is deployed, and a new 0083 would collide with the rename migration on the branch stacked above this one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ATOMIC_REQUESTS wraps every admin request in a transaction, so the warning this code already carries -- that Stripe objects cannot be rolled back and so must not be created inside one -- applies to the admin path it did not cover. Creating a Price and then hitting any later error in the request rolled the database back and left the Price behind, and the obvious next move, clicking save again, made a second one. ensure_stripe_price now looks for an existing Price before creating one, keyed on what the price *is* -- plan, interval, label, code, amount, currency -- rather than on the local row's primary key, which a rollback discards. A retry therefore adopts the orphan instead of duplicating it. That covers the admin formset and supersede() as well, without moving Stripe calls into on_commit, which would have hidden failures from whoever clicked save. supersede() no longer wraps its Stripe call in @transaction.atomic; only the two database writes are atomic. It had no callers, so this was a trap set for whoever wired it up rather than a live fault. consolidate_stripe_products treated any existing active row as finished, so a row left blank by a failed Stripe call was skipped by every later run while the command reported success -- the opposite of the "re-running after a partial failure completes the remainder" its docstring promises. It now completes such rows and reports them separately. Adds the first tests for PlanPrice, including the rollback-and-retry case that all of this turns on. There were none, which is why this went unnoticed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ensure_stripe_product had exactly the orphan problem ensure_stripe_price was just fixed for, and its own docstring names it: "a Product that exists in Stripe but whose ID was never persisted is an orphan nothing can find again". Saving the ID immediately does not prevent that, because ATOMIC_REQUESTS makes every admin request a transaction that can roll the save back afterwards. It now looks the Product up by slug first and adopts it. Nothing stopped an admin editing the amount, currency or interval of a PlanPrice that already has a Stripe Price. Stripe will not change a Price, so the row would quote one figure while subscribers kept being billed another -- the same silent divergence between what is displayed and what Stripe charges that this project already has one live bug from. clean() now rejects those three fields once a Price exists and points at supersede. label and code stay editable: they are local classification and change nothing Stripe charges. Reversing 0082 raised a bare ProtectedError once consolidate_stripe_products had run, because PlanPrice.plan is PROTECT. It now says so, and says why dropping those rows automatically would be wrong: they point at live Stripe Prices that reversing cannot delete. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The pack grants AI credits; only the slug still said pages, left over from when DocumentCloud billed by page. Slugs are effectively permanent once they reach production, and this one has not, so it is free to fix now: 0082 has never been applied outside development. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
"Run the tier and pack migrations first" is misleading when they have already run. 0082 resolves each pack's OIDC client by finding which client's entitlements carry its resource key, and skips the pack when no client carries it or when more than one does -- so the migration can report success having created only some of the packs, and the first sign of it is this command failing with advice that does not apply. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A pack's resources are per-unit rather than flat -- base_requests 0, minimum_users 0, requests_per_user 10 -- so what the client actually grants is 10 x quantity. Squarelet's half of that is emitting `resources` untouched and `quantity` beside it, and nothing asserted on either. A refactor that dropped quantity would leave every pack granting its base once, which is zero, and every existing test would still pass. Built from factories rather than the migration's packs on purpose: a transactional test flushes migration seed data, so anything relying on it is empty by the time it runs. Verified the tests fail when quantity is hardcoded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
It still said partner pricing was deliberately absent and that such deals ride on a standard or nonprofit price with a coupon. That was true before the `code` field existed; it is not now. Stripe has no negative coupon, so an above-list negotiated rate cannot be expressed as a discount on one -- which is why `code` exists, and why below-list negotiated rates use it too rather than being handled a second way. Coupons are kept for time-limited promotions. The `code` field's own help_text already said this, so the class docstring was contradicting the field directly beneath it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both pointed at a document a reviewer does not have, and both used its step numbering - "Step 2d", "Step 3c", "Step 3e", "Step 3h" - which means nothing outside it. One of those references was in a CommandError, so it reached operators too. Replaced with what the reader actually needs: why the price matrix is hardcoded, why negotiated rates are prices rather than coupons, and what the pack resources shape is for. The pack comment now spells out the trap in full rather than gesturing at the step that will hit it - flattening a pack the way a tier is flattened leaves it granting nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`.get()` does not exist on a StripeObject. It is dict-*like* but not a dict, so `product.metadata.get(...)` raises AttributeError -- which is what running consolidate_stripe_products against staging hit, on the very first lookup. A product carrying no metadata at all raises too, on the attribute access itself. The codebase already knew this: get_current_period_end carries a comment saying `.get` is not available on newer Stripe typed objects and uses bracket notation instead. I wrote both of these lookups anyway. Metadata is now converted with `.to_dict()` before reading, tolerating both an absent `metadata` and objects created outside squarelet. The reason no test caught it is that every existing test mocks the plan service wholesale, so the provider methods never ran. Added tests that exercise them against real StripeObject values built with `construct_from`, which reproduce the staging traceback exactly when the fix is reverted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The model already refused edits to amount, currency and interval once a Stripe Price exists, but the admin still presented them as editable -- so the way you found out was by typing a new value and having the form reject it. A field that cannot be changed should not invite the attempt. They are now `disabled` on a row that already has a Stripe Price. That is `disabled` rather than the admin's `readonly_fields` because the decision has to be made per row: an inline's get_readonly_fields hook receives the *parent* object, never the individual instance, so readonly there could only mean "lock the amount on every price, including newly added ones". Both are equally safe against a tampered POST -- a readonly field is excluded from the form entirely, and a disabled one keeps its initial value -- so that is not what decides between them. `label` and `code` stay editable; they are local classification and change nothing Stripe charges. The model's clean() stays as the backstop for anything not going through the admin. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two corrections to the previous commit. The rule was "locked once a Stripe Price exists", which left comped rows editable because they never get one. A comped row has no *Stripe* counterpart but is still what an organization was granted, and changing what it says it costs makes it describe something else. Only a brand-new row is editable now; anything saved is locked. And a disabled input still reads as editable at a glance, which defeats the point of not offering the edit. The locked fields now render with the admin's own readonly markup and class, so they look like what they are. Still a form-level lock rather than `readonly_fields`, because it has to vary per row: an inline's hook sees only the parent object, so readonly there would lock the empty row being added as well. label, code and active stay editable - they are what supersede is driven by. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mitchelljkotler
force-pushed
the
pricing-models
branch
from
September 5, 2026 20:02
95c30e2 to
b066ff5
Compare
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.
This creates a new PlanPrice model to re-organize our plans using Stripe's price feature. Instead of offering multiple plans with the same functionality but different payment options, there is only one plan per functional plan with different PlanPrice's. This covers monthly vs annual charging, comped accounts, non-profit pricing, and special negotiated rates.
This is still a draft and not ready to merge, just wanted to get this up here.