billing: add --dry-run mode and always send manual invoices#3099
Draft
jshearer wants to merge 6 commits into
Draft
billing: add --dry-run mode and always send manual invoices#3099jshearer wants to merge 6 commits into
--dry-run mode and always send manual invoices#3099jshearer wants to merge 6 commits into
Conversation
* Add `--dry-run`: classify every invoice against the livemode Stripe API and report what would happen without creating or modifying anything. The previous testmode trial run misreported manual bills that already had invoices as phantom creates, because the sandbox lacked livemode state. Manual bills already invoiced in an earlier run now report as `AlreadyProcessed` instead of erroring. * Manual invoices are always sent as invoices rather than charged to a stored payment method, even when one is on file. Stored payment methods cover monthly usage overages; manual bills such as contracts and one-off charges should let the customer choose how to pay. Both changes were part of PR #2849, which was closed while the rest of the branch merged via #2883 and #2902, so they never landed. This reapplies them onto current master.
… from --recreate-finalized The classify/execute split returns no Stripe customer for skips resolved in the read-only Phase 1, which silently disabled --clean_up for those tenants. Separately, --recreate-finalized still applied to manual invoices, conflicting with the new `AlreadyProcessed` handling that makes manual reruns idempotent. * --clean_up falls back to a `find_customer` lookup when the `Skip` action carries no customer, so it again removes stale draft invoices for `FreeTier`, `FutureTrialStart`, and `LessThanMinimum` tenants. Phase 1 stays free of Stripe calls, so the lookup only runs under --clean_up. * Manual invoices are excluded from --recreate-finalized so an already-sent open manual invoice is not voided and reissued. An open manual invoice reports `AlreadyProcessed`, and a manual draft is refreshed through the normal Update path.
jshearer
force-pushed
the
jshearer/billing_dry_run
branch
from
473d8e8 to
5810d41
Compare
The manual-invoice -> send_invoice override in execute() was only applied on the create path. When an existing draft was updated, execute() reused whatever collection method the invoice was originally created with, so a manual invoice previously stored as charge_automatically was left to auto-charge the customer's card instead of being sent for manual payment. Hoist the collection method / due date derivation from `mode` and, on the update path, POST the reconciled collection method (and due date) to Stripe when it differs from what the invoice currently carries.
execute()'s delete/recreate path re-retrieves the invoice and re-checks its status to guard against it changing between classification and execution, but the update path did not: it retrieved the invoice and immediately cleared and re-added line items. If a concurrent run or a human finalized/paid/voided the draft in that window, the line-item mutations would fail with an opaque Stripe error instead of a clear diagnostic. Add the symmetric status re-check: bail with a 'changed state since classification' message unless the invoice is still a draft.
The dry-run arm reported "Would publish"/"Would update" straight from classify(), which never exercises the customer/email resolution that lives in execute(). A tenant with no resolvable billing email (no customer email, no tenants.billing_email, and no admin user) was previewed as a success but would bail on a real run, so dry-run gave false confidence. Add a read-only email-resolvability check mirroring ensure_customer_for_invoicing and report such tenants under the Error bucket in the preview. Extract the admin lookup into earliest_admin_email() and reuse it in both paths (the SQL text is kept byte-identical so it hits the existing offline query cache). Document on the flag that the invoice total cannot be reconciled in dry-run, since that requires creating the invoice in Stripe.
Address the remaining publish.rs review feedback in one pass: - Thread the customer found during classify() into the Create/Update actions so execute() reuses it via ensure_customer_for_invoicing instead of issuing a second identical Stripe customer search per non-skipped invoice (#8). This also collapses the ad-hoc Option<Option<Customer>> memo into a single lookup (#13). - Replace the .expect() on a Final invoice's `extra` payload with a bail!, so a null payload fails that one tenant instead of panicking and aborting the run (#5). - Preserve the manual-invoice annotation on the error path by recomputing it via a shared manual_annotation() helper (#7). - Derive is_update from existing_invoice_id.is_some() rather than carrying it in the destructure tuple (#14). - De-duplicate the dry-run/normal invoice-count log and the per-tenant result debug line (a shared log_result closure) (#11, #12). - Drop comments that merely restate the following call and qualify the bare InvoiceStatus reference, per CLAUDE.md (#15). Not changed: the skip-reason ordering shift is intentional (and avoids a Stripe lookup for skipped tenants); the execute()-side invoice re-retrieve is now required by the draft-status re-check; and the fresh customer balance re-fetch is deliberate.
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.
--dry-runmodeThe invoice generator's trial-run workflow ran against a testmode Stripe account, which lacked livemode invoice state, so manual bills that already had open or paid invoices showed up as phantom creates.
--dry-runruns against the livemode API in read-only mode and reports what each invoice would do without creating or modifying anything in Stripe.To make that safe, the per-invoice work is separated into two phases:
classifyperforms only reads (validation and Stripe lookups) and returns the action that would be taken.executeperforms all writes (customer and invoice creation, line items, amount verification).--dry-runstops afterclassifyand logs the would-be action; a normal run classifies and then executes, so both paths share the same decision logic.A behavior change rides along with this: manual bills that already have an open, paid, void, or uncollectible invoice in Stripe are now classified as
AlreadyProcessedand skipped instead of erroring. This is expected for date-range-overlapping manual bills invoiced in a previous run, and it applies to real runs too, not only dry runs.Always send manual invoices
Stored payment methods cover monthly usage overages. Manual bills such as contracts and one-off charges should be sent as invoices so the customer decides how to pay, rather than being charged automatically. Manual invoices now use
send_invoiceregardless of--charge-type, both at creation and during the send phase, even when a payment method is on file.