diff --git a/app/Console/Commands/SweepOrphanStripePrices.php b/app/Console/Commands/SweepOrphanStripePrices.php index cecd28f..2dbdeee 100644 --- a/app/Console/Commands/SweepOrphanStripePrices.php +++ b/app/Console/Commands/SweepOrphanStripePrices.php @@ -18,10 +18,33 @@ use Illuminate\Console\Command; * was the right call and it left an operator with a log line and no way to act * on it. * + * **What this lists is wider than that log line.** Every active Price at one of + * our Products that no row of ours knows — adoptable or not. The only question + * asked below is whether a register row holds the id; nothing here re-asks + * AdoptStripePrice whether it would have taken the Price over. + * + * **So stripe:sync-catalogue runs first.** Until it has, an orphan listed here + * may be one the next sync would simply have adopted, and --archive on that one + * is worse than it looks. Archiving it makes it invisible to the very step that + * would have rescued it: activePricesFor() asks Stripe for ACTIVE prices only, + * so adoption finds nothing and the sync falls through to createPrice() — under + * the same idempotency key the crashed run used, because the key is built from + * what is being sold and everything the call puts on the wire (see + * App\Services\Stripe\IdempotencyKey), and nothing about either has changed. For + * twenty-four hours Stripe answers a repeated key by replaying the response it + * stored rather than doing anything: the archived Price's id, in the body it had + * while it was still live. The register writes that id down as the live Price + * for the row, and a checkout on it is refused. + * * Without --archive this reports and touches nothing. With it, the orphans are - * archived at Stripe, which is harmless for the reason it always is in this - * codebase: Stripe goes on billing every subscription already on an archived - * Price. It stops being SOLD, and nothing here is sold on a Price no row knows. + * archived at Stripe, which is harmless for everything THIS codebase sells, for + * the reason it always is here: Stripe goes on billing every subscription + * already on an archived Price, it only stops being SOLD — and nothing this + * codebase sells is sold on a Price no row knows. That reasoning reaches no + * further than this codebase. A Price an operator wired up by hand — a payment + * link built in Stripe's own dashboard against one of our Products — is exactly + * the Price carrying none of our metadata described above, and --archive + * withdraws it too. */ class SweepOrphanStripePrices extends Command { @@ -29,7 +52,7 @@ class SweepOrphanStripePrices extends Command {--archive : Stop selling the orphans instead of only listing them} {--dry-run : Show what would be archived without touching Stripe}'; - protected $description = 'List the Stripe prices at our products that no row knows, and optionally archive them'; + protected $description = 'List the Stripe prices at our products that no row knows, and optionally archive them — run stripe:sync-catalogue first'; public function handle(StripeClient $stripe): int { @@ -87,10 +110,18 @@ class SweepOrphanStripePrices extends Command return self::SUCCESS; } + // Said on every path that found something, including after --archive + // has already run: the only question asked above is whether a row holds + // the id, so an orphan in this list may be one stripe:sync-catalogue + // would have adopted. Archiving that one is the failure this class's + // docblock describes, and an operator who has just done it needs to + // know as much as one who is about to. + $this->warn(' Run stripe:sync-catalogue first — an orphan listed here may be one the next sync would adopt.'); + $this->info(match (true) { $dryRun && $archive => "{$found} orphan(s) would be archived. Run without --dry-run to archive them.", $archive => "{$found} orphan(s) archived. Existing subscriptions on them keep billing.", - default => "{$found} orphan(s) found. Run with --archive to stop selling them.", + default => "{$found} orphan(s) found. Once the sync has run, --archive stops selling what is still listed.", }); return self::SUCCESS; diff --git a/app/Console/Commands/SyncStripeCatalogue.php b/app/Console/Commands/SyncStripeCatalogue.php index f880966..3bae730 100644 --- a/app/Console/Commands/SyncStripeCatalogue.php +++ b/app/Console/Commands/SyncStripeCatalogue.php @@ -194,8 +194,14 @@ class SyncStripeCatalogue extends Command return self::SUCCESS; } + // "or adopted" on the dry run, and only there. A dry run counts intents + // without ever calling ensure(), so it cannot know which of them Stripe + // already holds a Price for — and a dry run is the first thing an + // operator runs after an interrupted one, which is exactly when some of + // these will be adopted rather than minted. The live line below has + // both figures because by then the adoption step has answered. $this->info($dryRun - ? "{$created} object(s) would be created. Run without --dry-run to create them." + ? "{$created} object(s) would be created or adopted. Run without --dry-run to create them." : "{$minted} object(s) created, {$adopted} adopted in Stripe."); return self::SUCCESS; diff --git a/docs/handoffs/2026-07-30-real-run-handoff.md b/docs/handoffs/2026-07-30-real-run-handoff.md index 3364113..ab88736 100644 --- a/docs/handoffs/2026-07-30-real-run-handoff.md +++ b/docs/handoffs/2026-07-30-real-run-handoff.md @@ -223,6 +223,20 @@ Kette an einer anderen Stelle, und mehrere brechen sie **nach** der Zahlung. `stripe:reprice-subscriptions` (je zuerst `--dry-run`). Der Abgleich legt jetzt **zwei** Preise je Paket an; bis er lief, kann ein geprüfter EU-Firmenkunde nicht bestellen — das ist Absicht. +- `stripe:sweep-orphan-prices` listet jeden **aktiven** Stripe-Preis an unseren + Produkten, den **keine** Zeile in `stripe_addon_prices` oder + `stripe_plan_prices` kennt — mit ID, Betrag, Währung, Intervall und Metadaten. + Ohne Argument wird nichts angefasst; mit `--archive` werden die aufgelisteten + Preise bei Stripe stillgelegt, also **nicht mehr verkauft** (laufende Abos + darauf verrechnet Stripe weiter). **Zuerst `stripe:sync-catalogue` laufen + lassen:** das Kommando prüft nur, ob eine Zeile die ID kennt, nicht ob der + Wiedererkennungsschritt die Waise übernommen hätte. Eine übernehmbare Waise + vorher stillzulegen macht sie für den Abgleich unsichtbar (er fragt Stripe nur + nach aktiven Preisen) — er fällt dann auf `createPrice()` mit demselben + Idempotenz-Schlüssel zurück, den der abgebrochene Lauf benutzt hat, und Stripe + antwortet 24 Stunden lang mit der gespeicherten Antwort: der ID des + stillgelegten Preises. Danach steht im Register ein Preis, auf den Stripe + keinen Checkout mehr öffnet. --- diff --git a/docs/superpowers/specs/2026-07-30-stripe-adoption-followups-design.md b/docs/superpowers/specs/2026-07-30-stripe-adoption-followups-design.md index 89677e8..1e235c6 100644 --- a/docs/superpowers/specs/2026-07-30-stripe-adoption-followups-design.md +++ b/docs/superpowers/specs/2026-07-30-stripe-adoption-followups-design.md @@ -138,9 +138,25 @@ kann etwas mit ihr tun, ohne von Hand in Stripes Oberfläche zu gehen. - **`--dry-run`** wie bei `stripe:sync-catalogue`, damit die beiden Kommandos sich gleich anfühlen. -Eine Waise, die der Wiedererkennungsschritt hätte übernehmen können, taucht hier -gar nicht erst auf — sie wurde beim letzten Abgleich übernommen. Was übrig -bleibt, ist genau das, was §12 als „nicht übernehmbar" beschrieben hat. +**Berichtigt am 2026-07-30.** Hier stand: „Eine Waise, die der +Wiedererkennungsschritt hätte übernehmen können, taucht hier gar nicht erst auf +— sie wurde beim letzten Abgleich übernommen." Das stimmt nur, **wenn seit dem +Auftauchen der Waise ein Abgleich gelaufen ist**, und genau diese Vorbedingung +erzwingt das Kommando nicht. Es fragt ausschließlich, ob eine Zeile die ID +kennt; es fragt `AdoptStripePrice` nicht, ob die Waise übernehmbar wäre. Die +Liste ist deshalb **weiter** als das, was §12 „nicht übernehmbar" nannte, und +die Implementierung hat den falschen Satz getreu nachgebaut. + +Die Vorbedingung lautet: **zuerst `stripe:sync-catalogue`, dann dieses +Kommando.** Sie steht im Klassenkommentar, in `$description`, in der +Berichtsausgabe und im Betriebsteil des Handoffs — nicht als Ordnungsliebe, +sondern weil `--archive` auf eine übernehmbare Waise sie für den Abgleich +unsichtbar macht (`activePricesFor()` fragt nur aktive Preise ab). Der Abgleich +fällt dann auf `createPrice()` mit demselben Idempotenz-Schlüssel zurück, den +der abgebrochene Lauf benutzt hat, und Stripe antwortet 24 Stunden lang mit der +gespeicherten Antwort — der ID des stillgelegten Preises, im Zustand von +damals. Das Register hält den Tarif dann für live auf einem Preis, auf den +Stripe keinen Checkout öffnet. ## 5. Die Warnung wird gedrosselt @@ -250,7 +266,10 @@ Und an `StripeIdempotencyKeyTest.php`: Blättern **wirft** bei fehlender `id` - `app/Services/Billing/AddonPrices.php` — Produkt-Wiedererkennung in `product()` - `app/Providers/AppServiceProvider.php` — beide Adoptions-Klassen als - Singleton, damit ihr Lauf-Protokoll (`$duplicates`) trägt + Singleton, damit ihr Lauf-Protokoll über den ganzen Lauf trägt. Es heißt in + den beiden Klassen **nicht** gleich: `AdoptStripePrice::$adoptions` zählt die + übernommenen Preise, `AdoptStripeProduct::$duplicates` sammelt die IDs der + doppelten Produkte. - `app/Console/Commands/SyncStripeCatalogue.php` — Produkt-Wiedererkennung, getrennte Zählung, Kommentar berichtigt - `tests/Feature/Billing/StripePriceAdoptionTest.php`, diff --git a/tests/Feature/Billing/StripePriceAdoptionTest.php b/tests/Feature/Billing/StripePriceAdoptionTest.php index 6a6f75f..1a5075e 100644 --- a/tests/Feature/Billing/StripePriceAdoptionTest.php +++ b/tests/Feature/Billing/StripePriceAdoptionTest.php @@ -655,7 +655,7 @@ it('still adopts a price that carries every one of Stripe\'s defaults', function expect(adoptModulePrice())->toBe('price_ordinary'); }); -it('warns about an unexplained price once a day, not once a booking', function () { +it('warns about an unexplained price once a day, per price, not once a booking', function () { Log::spy(); $this->stripe->plantPrice('price_by_hand', 'prod_support', 3480, 'EUR', 'month', []); @@ -667,6 +667,32 @@ it('warns about an unexplained price once a day, not once a booking', function ( adoptModulePrice(); Log::shouldHaveReceived('warning')->once(); + + // PER PRICE. A second unexplained orphan is a second thing to say, not a + // repeat of the first — and "once" alone does not pin that: a key mutated + // to a constant would silence every orphan after this file's first one and + // leave the assertion above green. That is the silent no-op this whole + // branch is built against. + $this->stripe->plantPrice('price_by_hand_too', 'prod_support', 3480, 'EUR', 'month', []); + + adoptModulePrice(); + + Log::shouldHaveReceived('warning')->twice(); + + // PER DAY. An orphan is a STATE, so the line is due again tomorrow — the + // throttle exists to thin it out, not to say it once and never again. Past + // the TTL by a minute rather than exactly on it, so this asserts the expiry + // and not a boundary comparison. + $this->travelTo(now()->addDay()->addMinute()); + + adoptModulePrice(); + + // Asked of the FIRST price by name: after the travel both orphans warn + // again, and a count over all warnings could not tell "price_by_hand spoke + // twice" from "price_by_hand_too spoke twice and the first stayed silent". + Log::shouldHaveReceived('warning') + ->withArgs(fn (string $message, array $context) => $context['price'] === 'price_by_hand') + ->twice(); }); it('breaks a tie on created by the price id, so two runs agree', function () { diff --git a/tests/Feature/Billing/SweepOrphanPricesTest.php b/tests/Feature/Billing/SweepOrphanPricesTest.php index 1ab3771..4d2937d 100644 --- a/tests/Feature/Billing/SweepOrphanPricesTest.php +++ b/tests/Feature/Billing/SweepOrphanPricesTest.php @@ -1,6 +1,8 @@ assertSuccessful(); }); +it('sees the plan side too: a family product orphan is reported, a known plan price never', function () { + // Every other test in this file plants its orphan under the MODULE Product, + // so both halves of the plan side went untested: the PlanFamily half of + // products() and the StripePlanPrice half of the $known set. Blind to + // either, the command misses the LARGER half — a family Product collects a + // Price per version, term, treatment and rate change, a module Product four. + // + // Both assertions sit in one test on purpose: each fails for one of the two + // lines. Drop the PlanFamily pluck from products() and the family Product is + // never visited, so the orphan below goes unreported. Drop the + // StripePlanPrice pluck from $known and the row's own Price becomes an + // orphan the command names. + $row = PlanPrice::query()->firstOrFail(); + $familyProduct = (string) $row->version->family->stripe_product_id; + + $knownPlanPrice = (string) StripePlanPrice::query() + ->where('plan_price_id', $row->id) + ->where('reverse_charge', false) + ->value('stripe_price_id'); + + // At the family's Product and at an amount no row of ours holds — what the + // sync leaves behind when it dies between Stripe's create and our insert, + // seen from the sweep's side. + $this->stripe->plantPrice('price_family_orphan', $familyProduct, + 12300, (string) $row->currency, 'month', []); + + $this->artisan('stripe:sweep-orphan-prices') + ->expectsOutputToContain('price_family_orphan') + ->doesntExpectOutputToContain($knownPlanPrice) + ->assertSuccessful(); + + expect($familyProduct)->not->toBe('') + ->and($knownPlanPrice)->not->toBe('') + // Nothing was touched: this is the reporting path, not --archive. + ->and($this->stripe->archived)->toBe([]); +}); + it('stops selling the orphan when asked to', function () { $this->artisan('stripe:sweep-orphan-prices', ['--archive' => true]) ->assertSuccessful();