From 786318b6d476ec09a0b3bacc2d68b5b7774cdeb5 Mon Sep 17 00:00:00 2001 From: nexxo Date: Thu, 30 Jul 2026 17:30:56 +0200 Subject: [PATCH] Never tell anyone to delete a catalogue their contracts bill on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The refusal added in the previous commit had a state it read wrongly, and the wrong reading was destructive. record() sat at the END of handle() and in no try/finally, while createProduct() and ensure() throw uncaught. A run that died after the first object left hasStoredObjects() true and recorded() null. The same state arises with no failure at all: CheckoutController → PlanPrices:: ensure() and BookAddon → SyncStripeAddonItems → AddonPrices::ensure() mint missing prices and never call record(). In that state the next run — in the SAME mode — refused with the foreign account message and its "Clear plan_families.stripe_product_id, plan_prices.stripe_price_id and the … registers first", and billing.catalogue_synced blocked with the same text. The objects were from the account in force. The right move was to resume, which is what the idempotency keys exist for; instead an operator was handed a delete instruction for a catalogue live contracts are billed on. Two changes: 1. record() moves ahead of the creation loop, right behind the refusal. There it is already proved that either nothing is stored or what is stored belongs to the active account, so the moment carries the claim just as well — and a run that dies part-way can no longer leave a state that contradicts itself. 2. "Origin never recorded" gets its own sentence and its own cure, separate from "established: other account". StripeCatalogueMode::matchesActiveMode() becomes belongsToAnotherMode(), which is only true where the other account is fact. The check still blocks — the origin cannot be proved — but it says "run the sync again", and it names no register to empty. Red first: ⨯ it takes up a catalogue whose origin was never written down Failed asserting that 1 matches expected 0. ⨯ it leaves no half-built catalogue that contradicts itself when a run dies Failed asserting that null is identical to an object of class "App\Support\OperatingMode". ⨯ it tells an unrecorded origin apart from a foreign account The two states are held apart by assertion, not by wording: only the foreign-account sentence may name stripe_plan_prices, and the unrecorded one must name stripe:sync-catalogue instead. The existing foreign-account test keeps its teeth. Full suite: 1817 passed, 6366 assertions. Co-Authored-By: Claude Opus 5 --- app/Console/Commands/SyncStripeCatalogue.php | 38 +++++++----- app/Support/Readiness/BillingChecks.php | 34 +++++++---- app/Support/StripeCatalogueMode.php | 28 ++++++--- lang/de/readiness.php | 5 ++ lang/en/readiness.php | 4 ++ .../Billing/StripeCatalogueModeTest.php | 58 +++++++++++++++++++ tests/Feature/Readiness/BillingChecksTest.php | 29 +++++++++- 7 files changed, 162 insertions(+), 34 deletions(-) diff --git a/app/Console/Commands/SyncStripeCatalogue.php b/app/Console/Commands/SyncStripeCatalogue.php index 30e8776..d7ba574 100644 --- a/app/Console/Commands/SyncStripeCatalogue.php +++ b/app/Console/Commands/SyncStripeCatalogue.php @@ -93,7 +93,7 @@ class SyncStripeCatalogue extends Command return self::FAILURE; } - if (StripeCatalogueMode::hasStoredObjects() && ! StripeCatalogueMode::matchesActiveMode()) { + if (StripeCatalogueMode::hasStoredObjects() && StripeCatalogueMode::belongsToAnotherMode()) { $this->error($this->staleCatalogueMessage()); // Auch der Trockenlauf endet hier, und mit FAILURE: er zählt, was @@ -104,6 +104,23 @@ class SyncStripeCatalogue extends Command return self::FAILURE; } + // VOR der Anlegeschleife, nicht danach. Hier ist bereits bewiesen, dass + // entweder nichts liegt oder das Vorhandene dem aktiven Konto gehört — + // der Zeitpunkt trägt die Aussage also genauso gut. Am Ende von + // handle() trug er sie NICHT: createProduct() und ensure() werfen + // ungefangen, und ein Lauf, der nach dem ersten angelegten Objekt + // stirbt, hinterließ IDs ohne Herkunft. Der nächste Lauf hielt das für + // ein fremdes Konto und trug dem Betreiber auf, einen Katalog zu leeren, + // an dem laufende Verträge abgerechnet werden — statt schlicht + // wiederaufzusetzen, wofür die Idempotenzschlüssel unten gebaut sind. + if (! $dryRun) { + if (StripeCatalogueMode::recorded() === null && StripeCatalogueMode::hasStoredObjects()) { + $this->line(' note taking up a catalogue whose account was never recorded; nothing is replaced.'); + } + + StripeCatalogueMode::record(); + } + $created = 0; foreach (PlanFamily::query()->with('versions.prices')->orderBy('tier')->get() as $family) { @@ -143,15 +160,6 @@ class SyncStripeCatalogue extends Command $created += $this->syncModules($dryRun); - // Wessen Konto die IDs gehören, die jetzt in der Datenbank stehen. - // Erst HIER, und nur nach einem echten Lauf: der Trockenlauf legt - // nichts an, und die Weigerung oben ist es, die diese Zeile wahr hält — - // ohne sie könnte ein Lauf einen halb gemischten Katalog hinterlassen, - // über den ein einzelner Wert dann lügen müsste. - if (! $dryRun) { - StripeCatalogueMode::record(); - } - $this->newLine(); if ($created === 0) { @@ -181,18 +189,22 @@ class SyncStripeCatalogue extends Command * a registered row as proof on its own for the reverse-charge half, so a * cleared pointer with the register left behind would leave exactly that * half pointing at the old account, and nothing would say so. + * + * Only reached where the OTHER account is established fact. A catalogue + * whose account was never recorded is a different state with a different + * cure — see StripeCatalogueMode::belongsToAnotherMode(). Telling that + * operator to clear anything would be telling them to delete a catalogue + * their live contracts bill on. */ private function staleCatalogueMessage(): string { - $recorded = StripeCatalogueMode::recorded(); - return sprintf( 'The stored catalogue was created in the [%s] account; this installation is now in [%s] mode. ' .'Stripe objects belong to the account that issued them, and this command skips every row that ' .'already carries an id — so it cannot repair this by running again. Clear ' .'plan_families.stripe_product_id, plan_prices.stripe_price_id and the stripe_plan_prices / ' .'stripe_addon_prices registers first, then run it. Nothing was created.', - $recorded?->value ?? 'unknown', + StripeCatalogueMode::recorded()?->value, OperatingMode::current()->value, ); } diff --git a/app/Support/Readiness/BillingChecks.php b/app/Support/Readiness/BillingChecks.php index 1fdc041..1c196af 100644 --- a/app/Support/Readiness/BillingChecks.php +++ b/app/Support/Readiness/BillingChecks.php @@ -54,8 +54,17 @@ final class BillingChecks // dem Umschalten auf Live weiter „erfüllt", und die erste echte // Bestellung bekam von Stripe *No such price*. Ohne Netzverkehr // entschieden: die Bereitschaftsseite erreicht beim Aufruf nichts. - $wrongAccount = (clone $published)->whereNotNull('stripe_price_id')->exists() - && ! StripeCatalogueMode::matchesActiveMode(); + // + // ZWEI Fälle, nicht einer. „Steht fest: anderes Konto" verlangt das + // Leeren der Zeiger und Register; „Herkunft nie festgehalten" verlangt + // nur einen neuen Lauf. Der zweite entsteht ohne jeden Kontowechsel — + // ein abgebrochener Abgleich, oder eine Bestellung, die sich ihren + // fehlenden Preis selbst geholt hat — und die Leeranweisung wäre dort + // die Aufforderung, einen Katalog zu löschen, an dem laufende Verträge + // abgerechnet werden. + $somethingSynced = (clone $published)->whereNotNull('stripe_price_id')->exists(); + $wrongAccount = $somethingSynced && StripeCatalogueMode::belongsToAnotherMode(); + $unknownOrigin = $somethingSynced && StripeCatalogueMode::recorded() === null; return [ new Check( @@ -120,18 +129,21 @@ final class BillingChecks group: self::GROUP, severity: Check::SEVERITY_BLOCKING, label: __('readiness.billing.catalogue_synced'), - // Der Widerspruch zuerst: „noch nicht abgeglichen" wäre über - // einen Katalog, der abgeglichen IST — nur im anderen Konto — - // die falsche Anweisung, und es ist die teure Richtung + // Der bewiesene Widerspruch zuerst: „noch nicht abgeglichen" + // wäre über einen Katalog, der abgeglichen IST — nur im anderen + // Konto — die falsche Anweisung, und es ist die teure Richtung // (der Betreiber ruft den Befehl, bekommt „already in step" - // und glaubt, es sei erledigt). - breaks: $wrongAccount - ? __('readiness.billing.catalogue_account_breaks', [ + // und glaubt, es sei erledigt). Danach die unbekannte Herkunft, + // deren Kur ein Lauf ist und kein Löschen. Zuletzt die Lücke. + breaks: match (true) { + $wrongAccount => __('readiness.billing.catalogue_account_breaks', [ 'mode' => __('readiness.mode.'.$mode->value), - ]) - : __('readiness.billing.catalogue_synced_breaks'), + ]), + $unknownOrigin => __('readiness.billing.catalogue_origin_breaks'), + default => __('readiness.billing.catalogue_synced_breaks'), + }, tab: 'services', - satisfied: ! $unsyncedPrices && ! $wrongAccount, + satisfied: ! $unsyncedPrices && ! $wrongAccount && ! $unknownOrigin, ), ]; } diff --git a/app/Support/StripeCatalogueMode.php b/app/Support/StripeCatalogueMode.php index c2e6607..ba44db1 100644 --- a/app/Support/StripeCatalogueMode.php +++ b/app/Support/StripeCatalogueMode.php @@ -60,18 +60,28 @@ final class StripeCatalogueMode } /** - * Gehört der gespeicherte Katalog zum Konto des aktiven Modus? + * Steht fest, dass der gespeicherte Katalog einem ANDEREN Konto gehört? * - * „Noch nie abgeglichen" (null) ist hier bewusst KEINE Übereinstimmung: eine - * Installation, in der IDs liegen, über deren Herkunft nichts festgehalten - * ist, kann nicht behaupten, sie gehörten zum aktiven Konto. Die Migration, - * die die Plätze einführt, trägt für den einen Fall nach, den sie belegen - * kann (der vorhandene Schlüssel ist der einzige, mit dem je etwas angelegt - * worden sein kann). + * Nur dann — nicht schon, wenn die Herkunft unbekannt ist. Das sind zwei + * Zustände mit zwei Handlungsanweisungen, und sie zusammenzuwerfen war ein + * teurer Fehler: „Herkunft nie festgehalten" entsteht ganz ohne Kontowechsel, + * nämlich wenn ein Abgleich mitten im Anlegen abbricht oder wenn eine + * Bestellung sich ihren fehlenden Preis selbst holt (CheckoutController → + * PlanPrices::ensure(), BookAddon → SyncStripeAddonItems → + * AddonPrices::ensure() — keiner der beiden ruft record()). Die Objekte + * stammen dann aus genau dem geltenden Konto, und wer in diesem Zustand die + * Leeranweisung für einen Kontowechsel liest, löscht einen Katalog, an dem + * laufende Verträge abgerechnet werden. + * + * Unbekannte Herkunft ist trotzdem kein Freibrief: die Bereitschaftsseite + * meldet sie weiter blockierend, nur mit dem Satz, der dazu passt („lauf den + * Abgleich noch einmal"). Beweisen kann sie die Herkunft nicht. */ - public static function matchesActiveMode(): bool + public static function belongsToAnotherMode(): bool { - return self::recorded() === OperatingMode::current(); + $recorded = self::recorded(); + + return $recorded !== null && $recorded !== OperatingMode::current(); } /** diff --git a/lang/de/readiness.php b/lang/de/readiness.php index 2418df3..07527ae 100644 --- a/lang/de/readiness.php +++ b/lang/de/readiness.php @@ -61,6 +61,11 @@ return [ // die schon eine ID trägt, repariert das also NICHT von selbst. Ein // Satz, der nur „bitte neu abgleichen" sagte, schickte den Betreiber // auf einen Befehl, der ihm „already in step" antwortet. + // Der dritte Zustand, und ausdrücklich OHNE Leeranweisung: er entsteht + // ohne jeden Kontowechsel, und die Objekte gehören zum geltenden Konto. + // Wer hier zu löschen anfinge, löschte einen Katalog, an dem laufende + // Verträge abgerechnet werden. + 'catalogue_origin_breaks' => 'Zu den gespeicherten Stripe-IDs ist nicht festgehalten, in welchem Konto sie entstanden sind — etwa weil ein Abgleich mitten im Anlegen abgebrochen ist oder weil eine Bestellung sich ihren fehlenden Preis selbst geholt hat. Solange das offen ist, kann diese Seite nicht sagen, ob die Kasse mit ihnen verkaufen kann. Ein Lauf von stripe:sync-catalogue setzt auf dem Vorhandenen auf, legt nur das Fehlende nach und hält die Herkunft fest — gelöscht werden muss dafür nichts.', 'catalogue_account_breaks' => 'Die gespeicherten Stripe-Produkt- und Preis-IDs wurden in einem anderen Konto angelegt als dem, das jetzt gilt (:mode) — Stripe-Objekte gehören dem Konto, das sie ausgestellt hat. Jede Bestellung scheitert damit an der Kasse mit „No such price": kein Auftrag, keine Zahlung. Ein neuer Lauf von stripe:sync-catalogue allein genügt nicht, er überspringt jede Zeile, die schon eine ID trägt; erst nach dem Leeren von plan_families.stripe_product_id und plan_prices.stripe_price_id sowie der Register stripe_plan_prices und stripe_addon_prices legt er den Katalog im aktiven Konto neu an.', ], diff --git a/lang/en/readiness.php b/lang/en/readiness.php index e99f10d..f909d11 100644 --- a/lang/en/readiness.php +++ b/lang/en/readiness.php @@ -57,6 +57,10 @@ return [ // Says what happens and what actually helps, including the awkward // half: stripe:sync-catalogue skips every row that already carries an // id, so it does not repair this on its own. + // The third state, deliberately WITHOUT a clear-it-out instruction: it + // arises without any account change, and the objects belong to the + // account in force. + 'catalogue_origin_breaks' => 'Nothing records which account the stored Stripe ids were created in — a sync may have died part-way through, or an order may have minted its own missing price. Until that is settled this page cannot say whether the checkout can sell with them. Running stripe:sync-catalogue takes up what is already there, creates only what is missing, and records the account — nothing has to be deleted for it.', 'catalogue_account_breaks' => 'The stored Stripe product and price ids were created in a different account from the one now in force (:mode) — a Stripe object belongs to the account that issued it. Every order then fails at the checkout with "No such price": no order, no payment. Re-running stripe:sync-catalogue is not enough on its own, because it skips every row that already carries an id; only after clearing plan_families.stripe_product_id and plan_prices.stripe_price_id as well as the stripe_plan_prices and stripe_addon_prices registers does it recreate the catalogue in the active account.', ], diff --git a/tests/Feature/Billing/StripeCatalogueModeTest.php b/tests/Feature/Billing/StripeCatalogueModeTest.php index dbc468f..6aac026 100644 --- a/tests/Feature/Billing/StripeCatalogueModeTest.php +++ b/tests/Feature/Billing/StripeCatalogueModeTest.php @@ -4,7 +4,9 @@ use App\Models\PlanPrice; use App\Services\Stripe\FakeStripeClient; use App\Services\Stripe\StripeClient; use App\Support\OperatingMode; +use App\Support\Settings; use App\Support\StripeCatalogueMode; +use Illuminate\Contracts\Console\Kernel; /** * Der Abgleich sagt, in wessen Konto er gearbeitet hat — und weigert sich, in @@ -51,6 +53,62 @@ it('refuses to work into a catalogue that belongs to the other account', functio expect(StripeCatalogueMode::recorded())->toBe(OperatingMode::Test); }); +/** + * „Herkunft nie festgehalten" ist NICHT „fremdes Konto". + * + * Der Zustand entsteht ohne jeden Fehler: `CheckoutController` → `PlanPrices:: + * ensure()` und `BookAddon` → `SyncStripeAddonItems` → `AddonPrices::ensure()` + * legen fehlende Preise selbst an, und keiner der beiden ruft `record()`. Er + * entsteht auch nach einem Lauf, der mittendrin abgebrochen ist. + * + * Die Objekte stammen dann aus genau dem geltenden Konto. Diesen Zustand in die + * Konto-Weigerung zu schicken, hieße einem Betreiber das Leeren eines Katalogs + * aufzutragen, an dem laufende Verträge abgerechnet werden — für ein + * Wiederaufsetzen, für das die Idempotenzschlüssel dieses Befehls ausdrücklich + * gebaut sind. + */ +it('takes up a catalogue whose origin was never written down', function () { + OperatingMode::set(OperatingMode::Test); + $this->artisan('stripe:sync-catalogue')->assertSuccessful(); + + Settings::forget(StripeCatalogueMode::SETTING); + + $this->artisan('stripe:sync-catalogue')->assertSuccessful(); + + expect(StripeCatalogueMode::recorded())->toBe(OperatingMode::Test); +}); + +it('leaves no half-built catalogue that contradicts itself when a run dies', function () { + OperatingMode::set(OperatingMode::Test); + + // Stripe geht mitten im Lauf weg, nachdem das erste Produkt schon angelegt + // und seine ID gespeichert ist. `createProduct()` und `ensure()` werfen + // ungefangen — stünde record() am ENDE von handle(), bliebe genau der + // Zustand zurück, den der Test darüber beschreibt: IDs da, Herkunft leer. + app()->instance(StripeClient::class, new class extends FakeStripeClient + { + public function createPrice( + string $productId, + int $amountCents, + string $currency, + string $interval, + array $metadata = [], + ?string $idempotencyKey = null, + ): string { + throw new RuntimeException('Stripe went away mid-run'); + } + }); + + try { + app(Kernel::class)->call('stripe:sync-catalogue'); + } catch (RuntimeException) { + // Erwartet — der Abbruch IST der Testfall. + } + + expect(StripeCatalogueMode::hasStoredObjects())->toBeTrue() + ->and(StripeCatalogueMode::recorded())->toBe(OperatingMode::Test); +}); + it('syncs into the new account once the stale ids are cleared', function () { OperatingMode::set(OperatingMode::Test); $this->artisan('stripe:sync-catalogue')->assertSuccessful(); diff --git a/tests/Feature/Readiness/BillingChecksTest.php b/tests/Feature/Readiness/BillingChecksTest.php index 1ae7461..aee5a35 100644 --- a/tests/Feature/Readiness/BillingChecksTest.php +++ b/tests/Feature/Readiness/BillingChecksTest.php @@ -180,7 +180,34 @@ it('says the sale is refused and a fresh sync is needed, not that a column is em // trägt. Das muss dastehen, sonst schickt die Seite den Betreiber auf einen // Befehl, der ihm „already in step" antwortet. expect(checkFor('billing.catalogue_synced')->breaks) - ->not->toBe(__('readiness.billing.catalogue_synced_breaks')); + ->not->toBe(__('readiness.billing.catalogue_synced_breaks')) + // Nur DIESER Fall trägt die Leeranweisung. + ->and(checkFor('billing.catalogue_synced')->breaks)->toContain('stripe_plan_prices'); +}); + +/** + * Der dritte Zustand, und er darf nicht mit dem zweiten zusammenfallen. + * + * IDs liegen da, aber nichts sagt, aus welchem Konto — weil ein Abgleich + * mittendrin abgebrochen ist, oder weil eine Bestellung sich ihren fehlenden + * Preis selbst angelegt hat (CheckoutController → PlanPrices::ensure(), + * BookAddon → AddonPrices::ensure(); keiner der beiden ruft record()). Sie + * stammen aus genau dem geltenden Konto. Wer hier die Leeranweisung liest, + * löscht einen Katalog, an dem laufende Verträge abgerechnet werden. + */ +it('tells an unrecorded origin apart from a foreign account', function () { + PlanPrice::query()->update(['stripe_price_id' => 'price_minted_by_a_checkout']); + Settings::forget(StripeCatalogueMode::SETTING); + + // Blockierend bleibt es: beweisen lässt sich die Herkunft nicht, und die + // Seite behauptet nichts, was sie nicht weiß. + expect(checkFor('billing.catalogue_synced')->satisfied)->toBeFalse(); + + // Aber die Handlungsanweisung ist „lauf den Abgleich noch einmal", nicht + // „lösche deinen Katalog" — kein Registername in diesem Satz. + expect(checkFor('billing.catalogue_synced')->breaks) + ->not->toContain('stripe_plan_prices') + ->and(checkFor('billing.catalogue_synced')->breaks)->toContain('stripe:sync-catalogue'); }); it('is satisfied again once the catalogue was synced in the mode that is running', function () {