From 3ceec8fe5eb0286154e92a87523074bcc3ae53ee Mon Sep 17 00:00:00 2001 From: nexxo Date: Thu, 30 Jul 2026 16:45:39 +0200 Subject: [PATCH 01/14] Design the nine things the reviews found and nobody built MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The one that matters is orphan PRODUCTS. A price orphan is recognised now; a product orphan is not, and it is worse — a second product makes every activePricesFor() ask about the wrong one, so price recognition goes blind for that whole family and the guard disables itself through the gap beside it. Duplicated products are reported, not deactivated. That is the owner's call and it is the careful one: an archived PRICE is provably harmless, which is what the grandfathering rule has always rested on, but deactivating a PRODUCT makes its prices unsellable and contracts can be running on those. What was proven for one is not proven for the other. The money gate moves into the class whose docblock promises it. Today AdoptStripePrice can compare three properties and the other four are filtered in one client, which the fake cannot express — so no test can reach the promise the class makes. Also: a sweep command for orphans nobody can adopt, a day's throttle on the warning that currently fires on every customer booking, and six mechanical fixes. Self-review caught one of my own: the plan to tell "created" from "adopted" by comparing the price id before and after the call cannot work — there is no id before, in either case. AdoptStripePrice counts its own adoptions instead, which needs it bound as a singleton, and the spec now says why. Co-Authored-By: Claude Opus 5 --- ...-07-30-stripe-adoption-followups-design.md | 243 ++++++++++++++++++ 1 file changed, 243 insertions(+) create mode 100644 docs/superpowers/specs/2026-07-30-stripe-adoption-followups-design.md 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 new file mode 100644 index 0000000..47d2eba --- /dev/null +++ b/docs/superpowers/specs/2026-07-30-stripe-adoption-followups-design.md @@ -0,0 +1,243 @@ +# Spec — Die offenen Punkte der Preis-Wiedererkennung + +**Datum:** 2026-07-30 +**Status:** entworfen, noch nicht umgesetzt +**Vorgänger:** `docs/superpowers/specs/2026-07-30-stripe-price-adoption-design.md`, §14 + +--- + +## 1. Ausgangslage + +Die Preis-Wiedererkennung ist gebaut und gemergt: ein Lauf, der zwischen Stripes +Anlage und unserem Schreiben abbricht, hinterlässt keine zweite Preis-Zeile +mehr, und eine Änderung an den Metadaten blockiert nichts mehr für 24 Stunden. + +§14 des Vorgängers hat aufgeschrieben, was die Prüfungen dabei fanden und +niemand behoben hat. Das wird hier gebaut. Drei Punkte mit Entwurfsentscheidung, +sechs mechanische. + +## 2. Waisen-**Produkte** — die eine echte Lücke + +`activePricesFor()` erkennt einen verwaisten *Preis* wieder. Für ein verwaistes +**Produkt** gibt es kein Gegenstück, und dort ist der Schaden größer: ein +zweites Produkt lässt jedes `activePricesFor($neueId)` leer zurückkommen. Damit +ist die Wiedererkennung für dieses Modul oder diese Familie **dauerhaft blind** +und jede Waise am ersten Produkt unerreichbar. Der Zweig deaktiviert seine eigene +Sicherung durch die Lücke, die er nicht geschlossen hat. + +Betroffen sind zwei Stellen, beide mit demselben Muster — Produkt anlegen, ID in +einem **zweiten** Schreibvorgang speichern: + +- `SyncStripeCatalogue::handle()` für Paketfamilien (`$family->update([…])`), +- `AddonPrices::product()` für Module (die ID landet erst über `remember()` in + einer Preiszeile). + +### Wie es gebaut wird + +`StripeClient::activeProducts(): array` — alle aktiven Produkte des Kontos, nach +dem Blätter-Muster von `activePricesFor()`. **Ohne** Filter auf Metadaten: Stripe +kann danach nicht suchen, und ein Konto hat eine Handvoll Produkte, keine +Tausende. + +`App\Services\Billing\AdoptStripeProduct` — der Wiedererkennungsschritt, nach +dem Vorbild von `AdoptStripePrice`, aber deutlich kürzer. Ein Produkt hat **keinen +Betrag**, also gibt es hier **kein Geld-Tor**: der Beweis liegt allein in den +Metadaten. Übernommen wird ein Produkt, das + +1. bei Stripe **aktiv** ist, +2. **mindestens einen tragenden Metadatenschlüssel bestätigt** — + `plan_family_id` bei Familien, `addon` bei Modulen — und +3. **keinem** unserer Metadatenschlüssel widerspricht, den es trägt. + +Eine Beanspruchungs-Prüfung wie bei den Preisen braucht es nicht: die +tragenden Schlüssel sind je Familie und je Modul eindeutig, ein Produkt kann +also gar nicht zu zweien gehören. + +### Doppelte Produkte werden gemeldet, nicht stillgelegt + +Entscheidung des Auftraggebers vom 2026-07-30, und sie ist die vorsichtige: +**das älteste wird übernommen, die übrigen werden gemeldet und nicht angefasst.** + +Der Grund ist ein echter Unterschied zur Preis-Seite. Ein archivierter *Preis* +ist nachweislich harmlos — Stripe verrechnet laufende Abos weiter, nur verkauft +wird er nicht mehr, und darauf beruht die Grandfathering-Regel dieses Projekts +schon lange. Ein stillgelegtes *Produkt* macht dagegen **seine Preise +unverkäuflich**, und auf denen können Verträge laufen. Was bei Preisen bewiesen +harmlos war, ist hier nicht bewiesen — also wird es nicht getan. + +Gemeldet wird über `Log::warning` **und** in der Ausgabe von +`stripe:sync-catalogue`, weil die Produkt-Anlage im Gegensatz zu `ensure()` nur +im Kommando läuft und ein Betreiber dort hinsieht. + +## 3. Das Geld-Tor zieht dorthin, wo es zugesagt wird + +`AdoptStripePrice` verspricht im Kopfkommentar, keine Übernahme könne Geld +verschieben — vergleicht aber nur Betrag, Währung und Intervall. Die vier +übrigen Eigenschaften, die entscheiden, was ein Preis verrechnet +(`interval_count`, `usage_type`, `transform_quantity`, `billing_scheme`), +filtert `HttpStripeClient::activePricesFor()`. + +Zwei Folgen, und die zweite ist die schlimmere: + +- der `FakeStripeClient` kann diese Felder gar nicht ausdrücken, also kann + **kein Test von `AdoptStripePrice` das Geld-Tor erreichen** — genau die + Zusage, um die es geht; +- eine dritte Implementierung von `StripeClient` ließe die Prüfung still + fallen. + +### Wie es gebaut wird + +`activePricesFor()` **filtert nicht mehr**, sondern gibt die vier Eigenschaften +im Rückgabe-Array mit: `interval_count`, `usage_type`, `transform_quantity` +(als `bool`: trägt der Preis eine?), `billing_scheme`. Absente Schlüssel werden +weiterhin auf Stripes Vorgaben gesetzt — `1`, `licensed`, `false`, `per_unit` — +und das bleibt die Richtung, auf die es ankommt: andersherum gelesen würde der +Filter jeden legitimen Preis ablehnen und die Wiedererkennung still zu einem +Nichts machen. + +`AdoptStripePrice` prüft sie neben Betrag, Währung und Intervall. Der +`FakeStripeClient` bekommt die vier Felder in `plantPrice()`, damit ein Test +einen vierteljährlichen oder mengenteilenden Preis überhaupt hinstellen kann. + +`StripeClient::activePricesFor()`s Vertrag wird ehrlich: er liefert aktive, +wiederkehrende Preise **mit** ihren preisbestimmenden Eigenschaften; was davon +zulässig ist, entscheidet der Aufrufer. + +**Verwandt, hier mit erledigt:** `PlanPrices::ensure()` bekommt die +`<= 0`-Schranke, die `AddonPrices::ensure()` schon hat. Sie ist das Einzige, was +einen `billing_scheme: tiered`-Preis (bei dem Stripe `unit_amount: null` +liefert, hier als 0 gelesen) auf der Paketseite überhaupt erreichbar macht. + +## 4. `stripe:sweep-orphan-prices` — das Aufräum-Kommando + +Der Vorgänger hat es bewusst ausgelassen (§12) und dafür die Log-Zeile +begründet. Die Log-Zeile allein reicht nicht: sie nennt die Waise, aber niemand +kann etwas mit ihr tun, ohne von Hand in Stripes Oberfläche zu gehen. + +- **Ohne Argument:** ein Bericht. Für jedes unserer Produkte werden die aktiven + Preise geholt und die aufgelistet, die **keine Zeile** in + `stripe_addon_prices` oder `stripe_plan_prices` kennt — mit ID, Betrag, + Währung, Intervall und Metadaten. Nichts wird angefasst. +- **Mit `--archive`:** dieselbe Liste wird stillgelegt. Das ist harmlos, und + zwar aus demselben Grund wie überall sonst in diesem Projekt: ein + archivierter Preis verrechnet laufende Abos weiter, er wird nur nicht mehr + verkauft. +- **`--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. + +## 5. Die Warnung wird gedrosselt + +`AdoptStripePrice` meldet einen unerklärlichen aktiven Preis mit +`Log::warning` — bei **jedem** Abgleich und **jeder** Kundenbuchung, solange die +Waise existiert. Der Widerspruchs-Pfad wurde aus genau diesem Grund still +gestellt; der Bestätigungs-Pfad bekam dieselbe Überlegung nicht. + +Höchstens einmal pro Preis-ID und Tag, über `Cache::add()` — dasselbe Muster wie +`App\Livewire\Billing:377` und `App\Livewire\Admin\Vpn:329`. Die Drosselung gilt +**nur** dieser wiederkehrenden Warnung, nicht der Meldung über ein doppeltes +Produkt und nicht der über einen archivierten Doppel-Preis: die stehen für ein +Ereignis, nicht für einen Zustand. + +## 6. Die sechs mechanischen Punkte + +Ohne Entwurfsentscheidung — bauen, Test, weiter (R22.5): + +1. `FakeStripeClient::updatePriceMetadata()` **ersetzt**, wo Stripe + **zusammenführt**. Bestehende Schlüssel bleiben künftig stehen. +2. `FakeStripeClient::activePricesFor()` gibt Metadaten ungecastet zurück, wo + `HttpStripeClient` auf String castet. Angleichen. +3. `created` im Fake kann gleichstehen: `createPrice()` stempelt + `count($prices) + 1`, `plantPrice()` hat die Vorgabe `1`. Gleichstand wird + über die Preis-ID aufgelöst, damit die Wahl über Seitengrenzen hinweg + reproduzierbar ist. +4. `HttpStripeClient::activePricesFor()` bricht das Blättern **still** ab, wenn + dem letzten Eintrag einer nicht-letzten Seite die `id` fehlt. Künftig wirft + es. Begründung im Kommentar: bei `invoiceLines()` entsteht dadurch ein zu + kurzer Belegtext, hier eine ungesehene Waise und ein zweiter aktiver Preis — + also der Vorfall selbst. +5. `stripe:sync-catalogue` zählt „angelegt" und „übernommen" **getrennt** statt + die Wortwahl zu entschärfen. + + Der naheliegende Weg funktioniert nicht: die Preis-ID vor und nach dem + Aufruf zu vergleichen unterscheidet die beiden Fälle *nicht* — vorher gibt es + in beiden keine, nachher in beiden eine. Stattdessen zählt + `AdoptStripePrice` seine eigenen Übernahmen mit, und das Kommando liest den + Zähler am Anfang und am Ende. Dafür wird die Klasse in `AppServiceProvider` + als **Singleton** gebunden: sie wird heute je Aufruf frisch aufgelöst + (`SyncStripeCatalogue` holt `PlanPrices` pro Zeile neu), ein Zähler auf der + Instanz käme also nie über eins hinaus. Sie hat sonst keinen Zustand, und + der Zähler ist das, was den Bericht wahr macht. +6. `PlanPrices::ensure()` bekommt die `<= 0`-Schranke — siehe §3. + +## 7. Was nicht dazugehört + +- **Der sprunghafte `PlanCatalogueTest`.** Ursachensuche, keine Bauarbeit, und + sie braucht ein anderes Vorgehen als dieser Plan. +- **`CLUPILOT_DNS_ZONE` in `phpunit.xml` festnageln.** Fremder Fehler, + inzwischen anders gelöst — die zweite Session hat die Zone im Test selbst + festgenagelt (`a842512`). +- **Alles, was laufende Verträge bewegt.** Wie im Vorgänger: dieser Entwurf + bewegt keinen einzigen. + +## 8. Tests + +`tests/Feature/Billing/StripeProductAdoptionTest.php` (neu) + +| Prüfung | Deckt ab | +|---|---| +| Verwaistes Produkt bei Stripe → wird übernommen, `createProduct` **nicht** aufgerufen (Familie **und** Modul) | §2 | +| Produkt ohne unsere Metadaten → **nicht** übernommen, neues angelegt, Warnung | §2, Bedingung 2 | +| Produkt, das einem Metadatenschlüssel widerspricht → nicht übernommen | §2, Bedingung 3 | +| Zwei passende Produkte → ältestes übernommen, das andere **gemeldet und nicht stillgelegt** | §2 | + +An `StripePriceAdoptionTest.php` angehängt: + +| Prüfung | Deckt ab | +|---|---| +| Ein Preis mit `interval_count: 3` wird **von `AdoptStripePrice`** abgelehnt — über den Fake, nicht über `Http::fake` | §3 | +| Dasselbe für `transform_quantity`, `usage_type: metered`, `billing_scheme: tiered` | §3 | +| Zweite Warnung zur selben Preis-ID innerhalb eines Tages unterbleibt | §5 | +| Gleichstand bei `created` → die kleinere Preis-ID gewinnt, zweimal gelaufen dasselbe Ergebnis | §6.3 | +| Ein Lauf, der eine Waise übernimmt, meldet „übernommen" und **nicht** „angelegt" | §6.5 | + +`tests/Feature/Billing/SweepOrphanPricesTest.php` (neu) + +| Prüfung | Deckt ab | +|---|---| +| Bericht nennt die Waise und lässt sie in Ruhe | §4 | +| `--archive` legt sie still, eine Zeile mit Preis bleibt unberührt | §4 | +| `--dry-run` mit `--archive` legt nichts still | §4 | + +Und an `StripeIdempotencyKeyTest.php`: Blättern **wirft** bei fehlender `id` +(§6.4); der Fake führt Metadaten zusammen statt sie zu ersetzen (§6.1). + +## 9. Berührte Dateien + +**Neu** +- `app/Services/Billing/AdoptStripeProduct.php` +- `app/Console/Commands/SweepOrphanStripePrices.php` +- `tests/Feature/Billing/StripeProductAdoptionTest.php` +- `tests/Feature/Billing/SweepOrphanPricesTest.php` + +**Geändert** +- `app/Services/Stripe/StripeClient.php` — `activeProducts()`, Vertrag von + `activePricesFor()` +- `app/Services/Stripe/HttpStripeClient.php` — Produkte auflisten, Filter + auflösen, Blättern wirft +- `app/Services/Stripe/FakeStripeClient.php` — Produkte auflisten, vier Felder + in `plantPrice()`, Metadaten zusammenführen, `created`-Gleichstand +- `app/Services/Billing/AdoptStripePrice.php` — die vier Eigenschaften prüfen, + Warnung drosseln +- `app/Services/Billing/AddonPrices.php` — Produkt-Wiedererkennung in + `product()` +- `app/Services/Billing/PlanPrices.php` — `<= 0`-Schranke +- `app/Console/Commands/SyncStripeCatalogue.php` — Produkt-Wiedererkennung, + getrennte Zählung, Kommentar berichtigt +- `tests/Feature/Billing/StripePriceAdoptionTest.php`, + `StripeIdempotencyKeyTest.php` — angehängt +- `docs/superpowers/specs/2026-07-30-stripe-price-adoption-design.md` — §14 + abhaken, was hier erledigt wird From 7d66a01a405df9b5642b0c1b878971a004e3e3bf Mon Sep 17 00:00:00 2001 From: nexxo Date: Thu, 30 Jul 2026 16:56:08 +0200 Subject: [PATCH 02/14] Break the follow-ups into six testable steps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six tasks, each ending on a green billing folder and its own commit: move the money gate into the class that promises it, four fixes that make the fake behave like Stripe, ask Stripe for its products, recognise an orphaned one, count adopted apart from created, and hand the operator a sweep for the orphans nobody can adopt. Self-review dropped one item the spec had carried over from the last review: the zero guard in PlanPrices::ensure(). Its only purpose was to keep a tiered price unreachable, and the gate move rejects those directly — so it would be a behaviour change with no remaining benefit, and it would make a free plan unsellable. Recorded as deliberately not built, with the reason, rather than quietly dropped. Also corrected: a test that counted every product where the run mints one per family, and a step that named the test it replaces only by description. Co-Authored-By: Claude Opus 5 --- .../2026-07-30-stripe-adoption-followups.md | 1680 +++++++++++++++++ ...-07-30-stripe-adoption-followups-design.md | 28 +- 2 files changed, 1702 insertions(+), 6 deletions(-) create mode 100644 docs/superpowers/plans/2026-07-30-stripe-adoption-followups.md diff --git a/docs/superpowers/plans/2026-07-30-stripe-adoption-followups.md b/docs/superpowers/plans/2026-07-30-stripe-adoption-followups.md new file mode 100644 index 0000000..9ede35c --- /dev/null +++ b/docs/superpowers/plans/2026-07-30-stripe-adoption-followups.md @@ -0,0 +1,1680 @@ +# Stripe Adoption Follow-ups — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Die neun Punkte aus §14 der Preis-Wiedererkennung bauen — allen voran die Wiedererkennung verwaister Stripe-**Produkte**, ohne die die schon gebaute Preis-Wiedererkennung für eine Familie dauerhaft blind werden kann. + +**Architecture:** Das Geld-Tor zieht aus `HttpStripeClient::activePricesFor()` in `AdoptStripePrice` um — der Client liefert die preisbestimmenden Eigenschaften, der Aufrufer entscheidet, was zulässig ist. Daneben entsteht `AdoptStripeProduct` nach demselben Muster wie `AdoptStripePrice`, aber ohne Geld-Tor (ein Produkt hat keinen Betrag) und ohne Stilllegen von Doppeln (ein stillgelegtes Produkt macht seine Preise unverkäuflich). Dazu ein Aufräum-Kommando, eine Tagesdrosselung der Waisen-Warnung und vier mechanische Korrekturen an Client und Fake. + +**Tech Stack:** Laravel 11, Pest, Stripe REST über `Illuminate\Http\Client`, MariaDB in Produktion / SQLite `:memory:` in Tests. PHP läuft im Container: `docker exec -w /var/www/html/.claude/worktrees/cool-sammet-368fee clupilot-app-1 php artisan test …` + +**Spec:** `docs/superpowers/specs/2026-07-30-stripe-adoption-followups-design.md` +**Vorgänger:** `docs/superpowers/specs/2026-07-30-stripe-price-adoption-design.md` §14 + +## Global Constraints + +- **Commit-Disziplin (Nutzervorgabe, nicht verhandelbar):** eine zweite Session arbeitet im selben Repository. **Immer** `git add -- ` und `git commit -F - -- `. **Nie** `git add -A`, `git add .`, `git commit -a`. Diese Regel wurde am 2026-07-30 gebrochen und hat 3526 Zeilen fertiger Arbeit aus `main` gelöscht. +- **Es wird ausschließlich im Worktree gearbeitet** (`/home/nexxo/clupilot/.claude/worktrees/cool-sammet-368fee`, Zweig `claude/cool-sammet-368fee`). Das Haupt-Repo `/home/nexxo/clupilot` wird **nicht angefasst** — dort läuft die zweite Session. +- **Kein `migrate:fresh`, kein `db:wipe`, kein `--env=testing`.** Es gibt keine `.env.testing`; solche Befehle treffen die laufende Entwicklungsdatenbank. Die Suite migriert sich selbst nach SQLite. +- Code, Kommentare, Log-Meldungen und Commit-Botschaften **englisch**; Spec, Plan und Handoffs **deutsch**. Kommentare erklären das *Warum*. +- **Adoption darf niemals Geld verschieben.** Übernommen wird nur bei identischem Betrag, identischer Währung, identischem Intervall — und nach Task 1 auch nur bei `interval_count` 1, `usage_type` `licensed`, ohne `transform_quantity` und mit `billing_scheme` `per_unit`. +- **Absente Stripe-Felder sind Stripes Vorgaben**: `interval_count` 1, `usage_type` `licensed`, `transform_quantity` keine, `billing_scheme` `per_unit`. Andersherum gelesen würde jede Prüfung jeden legitimen Preis ablehnen und die Wiedererkennung still zu einem Nichts machen. +- **Ein Produkt wird nie stillgelegt.** Ein archivierter *Preis* ist nachweislich harmlos; ein stillgelegtes *Produkt* macht seine Preise unverkäuflich, und darauf können Verträge laufen. +- **Die `<= 0`-Schranke in `PlanPrices::ensure()` wird NICHT gebaut** — siehe Spec §3. Der Umzug des Geld-Tors nimmt ihr den Zweck, und sie würde einen kostenlosen Tarif unverkäuflich machen. +- Jede Task endet mit einem grünen `tests/Feature/Billing`, bevor committet wird. + +--- + +## Dateistruktur + +**Neu** + +| Datei | Verantwortung | +|---|---| +| `app/Services/Billing/AdoptStripeProduct.php` | Erkennt ein verwaistes Stripe-Produkt als unseres wieder. Kein Geld-Tor, kein Stilllegen — nur Metadaten-Beweis, ältestes gewinnt, Doppel werden gemeldet. | +| `app/Console/Commands/SweepOrphanStripePrices.php` | Listet aktive Preise an unseren Produkten, die keine Zeile kennt; auf Wunsch legt es sie still. | +| `tests/Feature/Billing/StripeProductAdoptionTest.php` | Die Produkt-Regeln und beide Verdrahtungen. | +| `tests/Feature/Billing/SweepOrphanPricesTest.php` | Bericht, `--archive`, `--dry-run`. | + +**Geändert** + +| Datei | Änderung | +|---|---| +| `app/Services/Stripe/StripeClient.php` | `activeProducts()`; Vertrag von `activePricesFor()` wird ehrlich | +| `app/Services/Stripe/HttpStripeClient.php` | Filter auflösen, vier Felder mitliefern, Produkte auflisten, Blättern wirft | +| `app/Services/Stripe/FakeStripeClient.php` | vier Felder in `createPrice`/`plantPrice`/`activePricesFor`, Metadaten zusammenführen und casten, `activeProducts()`, `plantProduct()`, `created` an Produkten | +| `app/Services/Billing/AdoptStripePrice.php` | vier Eigenschaften prüfen, Warnung drosseln, Gleichstand über die ID, Übernahmen zählen | +| `app/Services/Billing/AddonPrices.php` | Produkt-Wiedererkennung in `product()` | +| `app/Console/Commands/SyncStripeCatalogue.php` | Produkt-Wiedererkennung, getrennte Zählung | +| `app/Providers/AppServiceProvider.php` | beide Adoptions-Klassen als Singleton | +| `tests/Feature/Billing/StripeIdempotencyKeyTest.php`, `StripePriceAdoptionTest.php` | angehängt bzw. umgehängt | +| `docs/superpowers/specs/2026-07-30-stripe-price-adoption-design.md` | §14 abhaken | + +**Abhängigkeit:** Task 1 und 2 sind unabhängig. Task 3 ist unabhängig. Task 4 braucht 3. Task 5 braucht 4 (Singleton-Muster). Task 6 ist unabhängig, kommt aber zuletzt. + +--- + +### Task 1: Das Geld-Tor zieht dorthin, wo es zugesagt wird + +**Files:** +- Modify: `app/Services/Stripe/HttpStripeClient.php` (`activePricesFor()`, der Filterblock und das Rückgabe-Array) +- Modify: `app/Services/Stripe/FakeStripeClient.php` (`createPrice()`, `activePricesFor()`, `plantPrice()`) +- Modify: `app/Services/Stripe/StripeClient.php` (Vertrag von `activePricesFor()`) +- Modify: `app/Services/Billing/AdoptStripePrice.php` (Geld-Tor, Drosselung) +- Test: `tests/Feature/Billing/StripePriceAdoptionTest.php` (neu angehängt), `tests/Feature/Billing/StripeIdempotencyKeyTest.php` (bestehender Test umgestellt) + +**Interfaces:** +- Produces: `activePricesFor()` liefert je Preis zusätzlich + `interval_count: int`, `usage_type: string`, `transform_quantity: bool`, `billing_scheme: string`. + `FakeStripeClient::plantPrice()` bekommt vier optionale Parameter am Ende: + `int $intervalCount = 1, string $usageType = 'licensed', bool $transformQuantity = false, string $billingScheme = 'per_unit'`. + +- [ ] **Step 1: Write the failing test** + +An `tests/Feature/Billing/StripePriceAdoptionTest.php` anhängen: + +```php +it('refuses a price whose recurrence is not one we could have minted', function () { + // The money gate, asked of AdoptStripePrice itself rather than of the + // client's filter. Until this moved, these four properties were rejected + // inside HttpStripeClient — and FakeStripeClient could not express them, so + // no test of this class could reach the promise its docblock makes. + $this->stripe->plantPrice('price_quarterly', 'prod_support', 3480, 'EUR', 'month', + moduleMetadata(), intervalCount: 3); + + expect(adoptModulePrice())->toBeNull(); +}); + +it('refuses a price that meters usage instead of selling a licence', function () { + $this->stripe->plantPrice('price_metered', 'prod_support', 3480, 'EUR', 'month', + moduleMetadata(), usageType: 'metered'); + + expect(adoptModulePrice())->toBeNull(); +}); + +it('refuses a price that divides the quantity before charging', function () { + // Modules are billed BY quantity — SyncStripeAddonItems sums a pack into one + // item at quantity n — so a divide_by price would charge a customer holding + // three for one. + $this->stripe->plantPrice('price_divided', 'prod_support', 3480, 'EUR', 'month', + moduleMetadata(), transformQuantity: true); + + expect(adoptModulePrice())->toBeNull(); +}); + +it('refuses a price that keeps its money in tiers', function () { + $this->stripe->plantPrice('price_tiered', 'prod_support', 3480, 'EUR', 'month', + moduleMetadata(), billingScheme: 'tiered'); + + expect(adoptModulePrice())->toBeNull(); +}); + +it('still adopts a price that carries every one of Stripe\'s defaults', function () { + // The direction that matters more than the four above: read the wrong way + // round, the gate refuses every legitimate price and recognition becomes a + // permanent no-op that nothing would ever alert on. + $this->stripe->plantPrice('price_ordinary', 'prod_support', 3480, 'EUR', 'month', moduleMetadata()); + + expect(adoptModulePrice())->toBe('price_ordinary'); +}); + +it('warns about an unexplained price once a day, not once a booking', function () { + Log::spy(); + + $this->stripe->plantPrice('price_by_hand', 'prod_support', 3480, 'EUR', 'month', []); + + // ensure() runs inside a customer's module booking, so without a throttle + // this line is written every time anybody books anything. + adoptModulePrice(); + adoptModulePrice(); + adoptModulePrice(); + + Log::shouldHaveReceived('warning')->once(); +}); +``` + +In `tests/Feature/Billing/StripeIdempotencyKeyTest.php` **zwei bestehende Tests +durch einen ersetzen**. Es sind genau diese beiden, sie prüfen das Wegfiltern und +haben nach dem Umzug keinen Gegenstand mehr: + +- `it('skips a price it could not have created itself, so the money gate never trusts a partial recurrence', …)` (ab `:173`) +- `it('skips a price that would charge our figure for the wrong quantity', …)` (ab `:203`) + +An ihre Stelle tritt einer, der prüft, dass die Eigenschaften **mitgeliefert** +statt weggefiltert werden — das Ablehnen selbst prüfen jetzt die vier neuen +Tests in `StripePriceAdoptionTest.php` oben, also dort, wo die Zusage gemacht +wird: + +```php +it('reports the properties that decide what a price charges, and filters none of them', function () { + Http::fake([ + 'api.stripe.com/*' => Http::response([ + 'data' => [ + ['id' => 'price_ordinary', 'unit_amount' => 3480, 'currency' => 'eur', + 'created' => 100, 'recurring' => ['interval' => 'month'], 'metadata' => []], + ['id' => 'price_quarterly', 'unit_amount' => 3480, 'currency' => 'eur', + 'created' => 101, 'recurring' => ['interval' => 'month', 'interval_count' => 3], + 'metadata' => []], + ['id' => 'price_divided', 'unit_amount' => 3480, 'currency' => 'eur', + 'created' => 102, 'recurring' => ['interval' => 'month'], + 'transform_quantity' => ['divide_by' => 10, 'round' => 'up'], 'metadata' => []], + ['id' => 'price_tiered', 'unit_amount' => null, 'currency' => 'eur', + 'created' => 103, 'recurring' => ['interval' => 'month', 'usage_type' => 'metered'], + 'billing_scheme' => 'tiered', 'metadata' => []], + ], + 'has_more' => false, + ]), + ]); + + $prices = collect((new HttpStripeClient)->activePricesFor('prod_1'))->keyBy('id'); + + // All four come back. Deciding which are usable is AdoptStripePrice's job — + // it is the class that promises adoption cannot move money, and it could not + // keep that promise while the fields lived only here. + expect($prices)->toHaveCount(4) + ->and($prices['price_ordinary'])->toMatchArray([ + 'interval_count' => 1, 'usage_type' => 'licensed', + 'transform_quantity' => false, 'billing_scheme' => 'per_unit', + ]) + ->and($prices['price_quarterly']['interval_count'])->toBe(3) + ->and($prices['price_divided']['transform_quantity'])->toBeTrue() + ->and($prices['price_tiered']['usage_type'])->toBe('metered') + ->and($prices['price_tiered']['billing_scheme'])->toBe('tiered'); +}); +``` + +- [ ] **Step 2: Run tests to verify they fail** + +```bash +docker exec -w /var/www/html/.claude/worktrees/cool-sammet-368fee clupilot-app-1 php artisan test tests/Feature/Billing/StripePriceAdoptionTest.php tests/Feature/Billing/StripeIdempotencyKeyTest.php +``` + +Expected: FAIL — `plantPrice()` kennt die vier benannten Argumente nicht (`Unknown named parameter $intervalCount`), und der umgestellte Client-Test bekommt nur einen Preis zurück statt vier. + +- [ ] **Step 3: `HttpStripeClient::activePricesFor()` filtert nicht mehr** + +Den ganzen `if (…) { continue; }`-Block samt seinem Kommentar **entfernen** und das Rückgabe-Array erweitern: + +```php + $prices[] = [ + 'id' => (string) ($price['id'] ?? ''), + 'unit_amount' => (int) ($price['unit_amount'] ?? 0), + 'currency' => strtoupper((string) ($price['currency'] ?? '')), + 'interval' => (string) ($recurring['interval'] ?? ''), + // The four properties that decide what a Price CHARGES + // beyond its amount. createPrice() sends none of them, so + // Stripe defaults every one — and an ABSENT key is that + // default, never a reason to reject. Reported rather than + // filtered: the class that promises adoption cannot move + // money is AdoptStripePrice, and it could not keep that + // promise while these lived here, where FakeStripeClient + // cannot express them and no test of that class could reach + // them. + 'interval_count' => (int) ($recurring['interval_count'] ?? 1), + 'usage_type' => (string) ($recurring['usage_type'] ?? 'licensed'), + 'transform_quantity' => (array) ($price['transform_quantity'] ?? []) !== [], + 'billing_scheme' => (string) ($price['billing_scheme'] ?? 'per_unit'), + 'created' => (int) ($price['created'] ?? 0), + 'metadata' => array_map( + fn ($value) => (string) $value, + (array) ($price['metadata'] ?? []), + ), + ]; +``` + +- [ ] **Step 4: Den Vertrag ehrlich machen** + +`app/Services/Stripe/StripeClient.php`, Docblock von `activePricesFor()` — die Aufzählung der vier Eigenschaften bleibt, aber die Aussage dreht sich um: + +```php + /** + * Every active recurring Price of a Product, as Stripe holds them, with the + * properties that decide what each one CHARGES. + * + * `interval_count` multiplies the period — 3 bills every three months while + * `interval` still reads 'month'. `usage_type` other than 'licensed' bills + * metered usage. `transform_quantity` divides the quantity before charging, + * and modules are billed BY quantity, so a divide_by of 10 would charge a + * customer holding three for one. `billing_scheme` 'tiered' keeps the money + * in tiers and leaves `unit_amount` null, which reads as 0 here. + * + * All four are REPORTED, not filtered. Which of them is acceptable is the + * caller's decision, and the caller is App\Services\Billing\AdoptStripePrice + * — the class whose docblock promises that adoption cannot move money. That + * promise has to be testable in the class that makes it, and it was not + * while these fields were dropped here: FakeStripeClient cannot express + * them. + * + * An absent key is Stripe's own default — 1, 'licensed', none, 'per_unit'. + * + * Active ones only, because the question being asked is "is there something + * here I can sell on?". `currency` comes back UPPER CASE, the way our own + * tables hold it. + * + * @return array}> + */ + public function activePricesFor(string $productId): array; +``` + +- [ ] **Step 5: Der Fake kann die vier Eigenschaften ausdrücken** + +In `FakeStripeClient::createPrice()` das gespeicherte Array um die vier Vorgaben ergänzen: + +```php + $this->prices[$id] = [ + 'product' => $productId, + 'amount' => $amountCents, + 'currency' => $currency, + 'interval' => $interval, + // Stripe's defaults, and the only thing createPrice() can produce: + // it sends none of these four, so every Price this platform mints + // carries exactly them. + 'interval_count' => 1, + 'usage_type' => 'licensed', + 'transform_quantity' => false, + 'billing_scheme' => 'per_unit', + 'metadata' => $metadata, + 'created' => count($this->prices) + 1, + ]; +``` + +`plantPrice()` bekommt die vier als benannte Parameter mit denselben Vorgaben: + +```php + public function plantPrice( + string $id, + string $productId, + int $amountCents, + string $currency, + string $interval, + array $metadata = [], + int $created = 1, + int $intervalCount = 1, + string $usageType = 'licensed', + bool $transformQuantity = false, + string $billingScheme = 'per_unit', + ): void { + $this->prices[$id] = [ + 'product' => $productId, + 'amount' => $amountCents, + 'currency' => $currency, + 'interval' => $interval, + // Defaulted to what we could have minted ourselves, so a test that + // does not care says nothing — and a test about the money gate says + // exactly which property it is about. + 'interval_count' => $intervalCount, + 'usage_type' => $usageType, + 'transform_quantity' => $transformQuantity, + 'billing_scheme' => $billingScheme, + 'metadata' => $metadata, + 'created' => $created, + ]; + } +``` + +`activePricesFor()` im Fake gibt sie zurück; der Kommentarabsatz, der erklärt, +warum der Fake nicht filtert, wird **entfernt** — er beschreibt einen Zustand, +den es nicht mehr gibt: + +```php + $found[] = [ + 'id' => $id, + 'unit_amount' => $price['amount'], + 'currency' => strtoupper($price['currency']), + 'interval' => $price['interval'], + 'interval_count' => $price['interval_count'], + 'usage_type' => $price['usage_type'], + 'transform_quantity' => $price['transform_quantity'], + 'billing_scheme' => $price['billing_scheme'], + 'created' => $price['created'], + 'metadata' => $price['metadata'], + ]; +``` + +Das `@var` von `$prices` (`:22`) um die vier Schlüssel ergänzen. + +- [ ] **Step 6: Das Geld-Tor in `AdoptStripePrice`** + +```php + foreach ($this->stripe->activePricesFor($productId) as $price) { + // The money gate, and the whole of it — this class promises that no + // adoption can move money, and until these four moved here it could + // compare only the first three. A Price at our figure with + // interval_count 3 bills quarterly; one with transform_quantity + // divide_by 10 charges a customer holding three packs for one; a + // metered or tiered one does not charge an amount at all. Stripe's + // dashboard copies metadata when it duplicates a Price, so such a + // Price can carry our own identifying key and be indistinguishable + // from ours by every other test here. + if ($price['unit_amount'] !== $amountCents + || $price['currency'] !== strtoupper($currency) + || $price['interval'] !== $interval + || $price['interval_count'] !== 1 + || $price['usage_type'] !== 'licensed' + || $price['transform_quantity'] + || $price['billing_scheme'] !== 'per_unit') { + continue; + } +``` + +- [ ] **Step 7: Die Warnung wird gedrosselt** + +`use Illuminate\Support\Facades\Cache;` ergänzen und den Warn-Zweig umbauen: + +```php + if (! $this->confirms($price['metadata'], $metadata, $identifying)) { + // Once a day per Price. An orphan nobody can adopt is a STATE, + // not an event, and this runs in a customer's module booking as + // well as in the sweep — undrosselt it writes a line every time + // anybody books anything, for as long as the orphan exists, and + // the clean-up command that would end it is a separate one. + // Cache::add is this repo's throttle; see App\Livewire\Billing. + if (Cache::add('stripe:unexplained-price:'.$price['id'], true, now()->addDay())) { + Log::warning('stripe: left an unexplained active price alone rather than adopting it', [ + 'price' => $price['id'], + 'product' => $productId, + 'amount_cents' => $amountCents, + 'currency' => strtoupper($currency), + 'interval' => $interval, + ]); + } + + continue; + } +``` + +- [ ] **Step 8: Run the tests** + +```bash +docker exec -w /var/www/html/.claude/worktrees/cool-sammet-368fee clupilot-app-1 php artisan test tests/Feature/Billing +``` + +Expected: PASS. Fällt hier ein bestehender Test, ist das ein echter Fund über den Umzug — melden, nicht den Test aufweichen. + +- [ ] **Step 9: Commit** + +```bash +git add -- app/Services/Stripe/HttpStripeClient.php app/Services/Stripe/FakeStripeClient.php app/Services/Stripe/StripeClient.php app/Services/Billing/AdoptStripePrice.php tests/Feature/Billing/StripePriceAdoptionTest.php tests/Feature/Billing/StripeIdempotencyKeyTest.php +``` + +```bash +git commit -F - -- app/Services/Stripe/HttpStripeClient.php app/Services/Stripe/FakeStripeClient.php app/Services/Stripe/StripeClient.php app/Services/Billing/AdoptStripePrice.php tests/Feature/Billing/StripePriceAdoptionTest.php tests/Feature/Billing/StripeIdempotencyKeyTest.php <<'MSG' +Put the money gate in the class that promises it + +AdoptStripePrice says in its own docblock that no adoption can move money, and +could compare three properties. The four that also decide what a Price charges +were filtered in HttpStripeClient — where FakeStripeClient cannot express them, +so no test of the class could reach the promise it makes, and a third client +implementation would drop the check in silence. + +The client reports them now and the caller decides. Absent keys stay Stripe's +defaults, which is the direction that matters: read the other way round the gate +refuses every legitimate price and recognition becomes a no-op nothing alerts on. + +And the unexplained-orphan warning is a state, not an event. It fired on every +sweep and every customer booking for as long as the orphan existed; once a day +per price is enough to act on. + +Co-Authored-By: Claude Opus 5 +MSG +``` + +--- + +### Task 2: Vier mechanische Korrekturen an Client und Fake + +**Files:** +- Modify: `app/Services/Stripe/FakeStripeClient.php` (`updatePriceMetadata()`, `activePricesFor()`) +- Modify: `app/Services/Stripe/HttpStripeClient.php` (`activePricesFor()`, Blätter-Schleife) +- Modify: `app/Services/Billing/AdoptStripePrice.php` (`usort`) +- Test: `tests/Feature/Billing/StripeIdempotencyKeyTest.php`, `tests/Feature/Billing/StripePriceAdoptionTest.php` + +**Interfaces:** +- Consumes: die vier Felder aus Task 1. +- Produces: nichts Neues nach außen. + +- [ ] **Step 1: Write the failing tests** + +An `tests/Feature/Billing/StripeIdempotencyKeyTest.php`: + +```php +it('merges metadata onto a price the way Stripe does, rather than replacing it', function () { + $fake = new FakeStripeClient; + $fake->plantPrice('price_a', 'prod_1', 3480, 'EUR', 'month', + ['addon' => 'priority_support', 'internal_note' => 'kept']); + + $fake->updatePriceMetadata('price_a', ['tax_treatment' => 'domestic']); + + // Stripe merges: a key you do not send stays. A fake that replaced would let + // a test prove the opposite of production — and it is exactly this merging + // that AdoptStripePrice's "write only when it differs" comparison is built + // around. + expect($fake->activePricesFor('prod_1')[0]['metadata'])->toBe([ + 'addon' => 'priority_support', + 'internal_note' => 'kept', + 'tax_treatment' => 'domestic', + ]); +}); + +it('hands metadata back as strings, the way the wire does', function () { + $fake = new FakeStripeClient; + $fake->plantPrice('price_a', 'prod_1', 3480, 'EUR', 'month', ['plan_price_id' => 42]); + + // HttpStripeClient casts; the fake did not. An un-cast int is what makes + // confirms()'s strict === fail forever for that price. + expect($fake->activePricesFor('prod_1')[0]['metadata']['plan_price_id'])->toBe('42'); +}); + +it('refuses to stop half-read when Stripe says there is another page', function () { + Http::fake([ + 'api.stripe.com/*' => Http::response([ + // has_more, and the last item carries no id to page after. Stopping + // here leaves an orphan unseen — which is a second live Price for one + // figure, the incident this whole feature exists to end. + 'data' => [['unit_amount' => 3480, 'currency' => 'eur', 'recurring' => ['interval' => 'month']]], + 'has_more' => true, + ]), + ]); + + expect(fn () => (new HttpStripeClient)->activePricesFor('prod_1')) + ->toThrow(RuntimeException::class, 'half-read'); +}); +``` + +An `tests/Feature/Billing/StripePriceAdoptionTest.php`: + +```php +it('breaks a tie on created by the price id, so two runs agree', function () { + // plantPrice defaults created to 1 and createPrice stamps count+1, so a + // planted orphan can tie with a minted price. A tie decided by Stripe's list + // order would adopt one price today and the other tomorrow. + $this->stripe->plantPrice('price_bbb', 'prod_support', 3480, 'EUR', 'month', + moduleMetadata(), created: 7); + $this->stripe->plantPrice('price_aaa', 'prod_support', 3480, 'EUR', 'month', + moduleMetadata(), created: 7); + + expect(adoptModulePrice())->toBe('price_aaa') + ->and($this->stripe->archived)->toBe(['price_bbb']); +}); +``` + +- [ ] **Step 2: Run tests to verify they fail** + +```bash +docker exec -w /var/www/html/.claude/worktrees/cool-sammet-368fee clupilot-app-1 php artisan test tests/Feature/Billing/StripeIdempotencyKeyTest.php tests/Feature/Billing/StripePriceAdoptionTest.php +``` + +Expected: FAIL — Metadaten werden ersetzt statt zusammengeführt, `42` bleibt Integer, das Blättern wirft nicht, und der Gleichstand fällt auf die Einfügereihenfolge. + +- [ ] **Step 3: Fake führt Metadaten zusammen** + +```php + public function updatePriceMetadata(string $priceId, array $metadata): void + { + if (isset($this->prices[$priceId])) { + // Merged, not replaced — Stripe leaves a key you do not send where + // it is. A fake that replaced would let a test prove the opposite of + // production, and "write only when it differs" in AdoptStripePrice + // is built around exactly this merging. + $this->prices[$priceId]['metadata'] = [ + ...$this->prices[$priceId]['metadata'], + ...$metadata, + ]; + } + + $this->metadataUpdates[] = ['price' => $priceId, 'metadata' => $metadata]; + } +``` + +- [ ] **Step 4: Fake castet Metadaten** + +Im Rückgabe-Array von `FakeStripeClient::activePricesFor()`: + +```php + // Cast like the wire does: Stripe hands metadata back as + // strings, and an un-cast int here would let a test pass where + // production's strict === fails. + 'metadata' => array_map(fn ($value) => (string) $value, $price['metadata']), +``` + +- [ ] **Step 5: Blättern wirft, statt still abzubrechen** + +In `HttpStripeClient::activePricesFor()`, nach dem Setzen von `$after`: + +```php + $after = $data === [] ? null : ($data[array_key_last($data)]['id'] ?? null); + + // invoiceLines() stops here, and there that costs a short document. + // Here it costs an unseen orphan and a second live Price for one + // figure — the incident. So it is an error, not a stopping point. + if (($page['has_more'] ?? false) === true && $after === null) { + throw new RuntimeException( + 'Stripe reported another page of prices and the last item of this one carried no id; ' + .'refusing to stop half-read.' + ); + } + } while (($page['has_more'] ?? false) === true); +``` + +`use RuntimeException;` steht bereits oben in der Datei. + +- [ ] **Step 6: Gleichstand über die Preis-ID** + +In `AdoptStripePrice`: + +```php + // Oldest first, and on a tie the lower id — so the same two orphans give + // the same answer on every run, whatever order Stripe lists them in. + usort($candidates, fn (array $a, array $b) => [$a['created'], $a['id']] <=> [$b['created'], $b['id']]); +``` + +Den Kommentar am `created`-Feld des Fakes berichtigen: er darf nicht mehr +behaupten, ein Gleichstand sei unmöglich — er ist möglich und wird jetzt +aufgelöst. + +- [ ] **Step 7: Run the tests** + +```bash +docker exec -w /var/www/html/.claude/worktrees/cool-sammet-368fee clupilot-app-1 php artisan test tests/Feature/Billing +``` + +Expected: PASS. + +- [ ] **Step 8: Commit** + +```bash +git add -- app/Services/Stripe/FakeStripeClient.php app/Services/Stripe/HttpStripeClient.php app/Services/Billing/AdoptStripePrice.php tests/Feature/Billing/StripeIdempotencyKeyTest.php tests/Feature/Billing/StripePriceAdoptionTest.php +``` + +```bash +git commit -F - -- app/Services/Stripe/FakeStripeClient.php app/Services/Stripe/HttpStripeClient.php app/Services/Billing/AdoptStripePrice.php tests/Feature/Billing/StripeIdempotencyKeyTest.php tests/Feature/Billing/StripePriceAdoptionTest.php <<'MSG' +Make the fake behave like Stripe where it matters + +Three ways it did not. It replaced metadata where Stripe merges — and merging is +the property AdoptStripePrice's "write only when it differs" comparison is built +around. It handed metadata back uncast where the wire returns strings, so a test +could pass on an int that production's strict === rejects forever. And a tie on +`created` fell to insertion order, so two runs could adopt different prices. + +Paging now throws instead of stopping when Stripe says there is another page and +the last item carries no id. invoiceLines() stops there and it costs a short +document; here it costs an unseen orphan and a second live price for one figure. + +Co-Authored-By: Claude Opus 5 +MSG +``` + +--- + +### Task 3: Stripe nach seinen Produkten fragen + +**Files:** +- Modify: `app/Services/Stripe/StripeClient.php` (nach `activePricesFor()`) +- Modify: `app/Services/Stripe/HttpStripeClient.php` +- Modify: `app/Services/Stripe/FakeStripeClient.php` +- Test: `tests/Feature/Billing/StripeIdempotencyKeyTest.php` + +**Interfaces:** +- Produces: + `StripeClient::activeProducts(): array` — Liste von + `array{id: string, name: string, created: int, metadata: array}`. + `FakeStripeClient::plantProduct(string $id, string $name, array $metadata = [], int $created = 1): void`. + `FakeStripeClient::$products` bekommt zusätzlich `created: int`. + +- [ ] **Step 1: Write the failing test** + +```php +it('pages through every active product of the account', function () { + Http::fake([ + 'api.stripe.com/*' => Http::sequence() + ->push([ + 'data' => [ + ['id' => 'prod_a', 'name' => 'Team', 'created' => 100, + 'metadata' => ['plan_family' => 'team', 'plan_family_id' => '3']], + ['id' => 'prod_b', 'name' => 'Priority Support', 'created' => 101, + 'metadata' => ['addon' => 'priority_support']], + ], + 'has_more' => true, + ]) + ->push([ + 'data' => [['id' => 'prod_c', 'name' => 'Etwas anderes', 'created' => 102, 'metadata' => []]], + 'has_more' => false, + ]), + ]); + + $products = (new HttpStripeClient)->activeProducts(); + + expect($products)->toHaveCount(3) + ->and($products[0])->toBe([ + 'id' => 'prod_a', + 'name' => 'Team', + 'created' => 100, + 'metadata' => ['plan_family' => 'team', 'plan_family_id' => '3'], + ]) + ->and($products[2]['id'])->toBe('prod_c'); + + // No metadata filter is sent: Stripe cannot search by it, so the whole + // active list comes back and the matching happens here. An account holds a + // handful of products, not thousands. + Http::assertSent(fn ($request) => str_contains($request->url(), 'active=true') + && ! str_contains($request->url(), 'metadata')); + Http::assertSent(fn ($request) => str_contains($request->url(), 'starting_after=prod_b')); +}); + +it('lets the fake answer with the products it holds', function () { + $fake = new FakeStripeClient; + + $minted = $fake->createProduct('Team', ['plan_family_id' => '3']); + $fake->plantProduct('prod_orphan', 'Team', ['plan_family_id' => '3'], created: 0); + + expect(collect($fake->activeProducts())->pluck('id')->all()) + ->toContain($minted, 'prod_orphan'); +}); +``` + +- [ ] **Step 2: Run test to verify it fails** + +```bash +docker exec -w /var/www/html/.claude/worktrees/cool-sammet-368fee clupilot-app-1 php artisan test tests/Feature/Billing/StripeIdempotencyKeyTest.php +``` + +Expected: FAIL — `Call to undefined method App\Services\Stripe\HttpStripeClient::activeProducts()`. + +- [ ] **Step 3: Den Vertrag ergänzen** + +`app/Services/Stripe/StripeClient.php`, nach `activePricesFor()`: + +```php + /** + * Every active Product on the account, with its metadata. + * + * The other half of the orphan question, and the more consequential one. A + * run that created a Product and died before storing its id leaves an orphan + * exactly as a Price does — but a second Product is worse than a duplicate + * Price, because activePricesFor() is then asked about the wrong one and the + * Price recognition goes blind for that whole family. + * + * The WHOLE active list, with no metadata filter: Stripe cannot search + * metadata, and an account holds a handful of Products. Which of them is + * ours is App\Services\Billing\AdoptStripeProduct's decision. + * + * @return array}> + */ + public function activeProducts(): array; +``` + +- [ ] **Step 4: Im `HttpStripeClient` umsetzen** + +```php + public function activeProducts(): array + { + $products = []; + $after = null; + + do { + $page = $this->request() + ->get($this->url('products'), array_filter([ + 'active' => 'true', + 'limit' => 100, + 'starting_after' => $after, + ], fn ($value) => $value !== null)) + ->throw() + ->json(); + + $data = (array) ($page['data'] ?? []); + + foreach ($data as $product) { + $products[] = [ + 'id' => (string) ($product['id'] ?? ''), + 'name' => (string) ($product['name'] ?? ''), + 'created' => (int) ($product['created'] ?? 0), + 'metadata' => array_map( + fn ($value) => (string) $value, + (array) ($product['metadata'] ?? []), + ), + ]; + } + + $after = $data === [] ? null : ($data[array_key_last($data)]['id'] ?? null); + + // Same reason as activePricesFor(): a half-read list here means a + // Product we own goes unrecognised, and the next run mints a second + // one — which blinds the Price recognition for the whole family. + if (($page['has_more'] ?? false) === true && $after === null) { + throw new RuntimeException( + 'Stripe reported another page of products and the last item of this one carried no id; ' + .'refusing to stop half-read.' + ); + } + } while (($page['has_more'] ?? false) === true); + + return $products; + } +``` + +- [ ] **Step 5: Im `FakeStripeClient` umsetzen** + +`createProduct()` stempelt einen Zähler mit: + +```php + $id = 'prod_'.substr(sha1($name.count($this->products)), 0, 12); + $this->products[$id] = [ + 'name' => $name, + 'metadata' => $metadata, + // Stripe stamps a creation time and AdoptStripeProduct takes the + // OLDEST of several. A counter cannot drift the way a clock in a + // test can. + 'created' => count($this->products) + 1, + ]; +``` + +Das `@var` von `$products` entsprechend erweitern. Dazu: + +```php + public function activeProducts(): array + { + $found = []; + + foreach ($this->products as $id => $product) { + $found[] = [ + 'id' => $id, + 'name' => $product['name'], + 'created' => $product['created'], + 'metadata' => array_map(fn ($value) => (string) $value, $product['metadata']), + ]; + } + + return $found; + } + + /** + * A Product that exists at Stripe and in no row of ours — the orphan a run + * leaves when it dies between Stripe's create and our storing its id. + */ + public function plantProduct(string $id, string $name, array $metadata = [], int $created = 1): void + { + $this->products[$id] = ['name' => $name, 'metadata' => $metadata, 'created' => $created]; + } +``` + +- [ ] **Step 6: Run the tests** + +```bash +docker exec -w /var/www/html/.claude/worktrees/cool-sammet-368fee clupilot-app-1 php artisan test tests/Feature/Billing +``` + +Expected: PASS. + +- [ ] **Step 7: Commit** + +```bash +git add -- app/Services/Stripe/StripeClient.php app/Services/Stripe/HttpStripeClient.php app/Services/Stripe/FakeStripeClient.php tests/Feature/Billing/StripeIdempotencyKeyTest.php +``` + +```bash +git commit -F - -- app/Services/Stripe/StripeClient.php app/Services/Stripe/HttpStripeClient.php app/Services/Stripe/FakeStripeClient.php tests/Feature/Billing/StripeIdempotencyKeyTest.php <<'MSG' +Let the catalogue ask Stripe what products it already has + +The price half of this question was answered; the product half was left open and +named as a known gap. It is the more consequential one: a second product makes +every activePricesFor() ask about the wrong one, so price recognition goes blind +for that whole family and every orphan on the first product is unreachable. + +The whole active list comes back with no metadata filter, because Stripe cannot +search metadata and an account holds a handful of products. Deciding which are +ours belongs to the next commit. + +Co-Authored-By: Claude Opus 5 +MSG +``` + +--- + +### Task 4: `AdoptStripeProduct` und beide Verdrahtungen + +**Files:** +- Create: `app/Services/Billing/AdoptStripeProduct.php` +- Modify: `app/Console/Commands/SyncStripeCatalogue.php` (Produkt-Anlage der Familie) +- Modify: `app/Services/Billing/AddonPrices.php` (`product()`) +- Modify: `app/Providers/AppServiceProvider.php` (Singleton-Bindung) +- Test: `tests/Feature/Billing/StripeProductAdoptionTest.php` + +**Interfaces:** +- Consumes: `StripeClient::activeProducts()` aus Task 3, `FakeStripeClient::plantProduct()`. +- Produces: + ```php + public function __invoke(array $metadata, array $identifying): ?string + ``` + Gibt die ID eines zu übernehmenden Produkts zurück oder `null`. + `public array $duplicates = []` — Produkt-IDs, die als Doppel erkannt und **nicht** angefasst wurden, für die Ausgabe des Kommandos. + In `AppServiceProvider::register()` als **Singleton** gebunden, damit `$duplicates` über einen ganzen Lauf trägt. + +- [ ] **Step 1: Write the failing test** + +`tests/Feature/Billing/StripeProductAdoptionTest.php`: + +```php +stripe = new FakeStripeClient; + app()->instance(StripeClient::class, $this->stripe); +}); + +it('adopts an orphaned family product instead of minting a second one', function () { + // A run that got as far as creating the Product and died before storing its + // id: Stripe has it, plan_families does not. + $family = PlanFamily::query()->orderBy('tier')->firstOrFail(); + $family->update(['stripe_product_id' => null]); + + $this->stripe->plantProduct('prod_orphan_family', $family->name, [ + 'plan_family' => $family->key, + 'plan_family_id' => (string) $family->id, + ]); + + app(Kernel::class)->call('stripe:sync-catalogue'); + + // Counting every product would prove nothing — the same run mints one for + // each of the OTHER families. What must hold is that this family's id is + // carried by exactly one product, and that it is the planted one. + expect($family->refresh()->stripe_product_id)->toBe('prod_orphan_family') + ->and(collect($this->stripe->products) + ->filter(fn (array $p) => ($p['metadata']['plan_family_id'] ?? null) === (string) $family->id) + ->keys()->all())->toBe(['prod_orphan_family']); +}); + +it('adopts an orphaned module product instead of minting a second one', function () { + $this->stripe->plantProduct('prod_orphan_module', 'Priority Support', [ + 'addon' => 'priority_support', + ]); + + $before = count($this->stripe->products); + + app(AddonPrices::class)->ensure( + 'priority_support', 2900, 'EUR', Subscription::TERM_MONTHLY, TaxTreatment::domestic(), + ); + + expect(StripeAddonPrice::query() + ->where('addon_key', 'priority_support') + ->value('stripe_product_id'))->toBe('prod_orphan_module') + ->and(count($this->stripe->products))->toBe($before); +}); + +it('adopts nothing a product does not prove about itself', function () { + // What an operator selling something else through the same Stripe account + // leaves lying around. Silent, unlike the Price side: we list every product + // on the account, so warning about each foreign one would fire on every run + // for as long as it exists. + Log::spy(); + + $this->stripe->plantProduct('prod_stranger', 'Etwas ganz anderes', []); + + expect(app(AdoptStripeProduct::class)( + ['addon' => 'priority_support'], ['addon'], + ))->toBeNull(); + + Log::shouldNotHaveReceived('warning'); +}); + +it('adopts nothing that contradicts what we would have written', function () { + $this->stripe->plantProduct('prod_other_module', 'Collabora Pro', ['addon' => 'collabora_pro']); + + expect(app(AdoptStripeProduct::class)( + ['addon' => 'priority_support'], ['addon'], + ))->toBeNull(); +}); + +it('takes the oldest of two matching products and leaves the other alone', function () { + Log::spy(); + + $this->stripe->plantProduct('prod_second', 'Priority Support', ['addon' => 'priority_support'], created: 200); + $this->stripe->plantProduct('prod_first', 'Priority Support', ['addon' => 'priority_support'], created: 100); + + $adopt = app(AdoptStripeProduct::class); + + expect($adopt(['addon' => 'priority_support'], ['addon']))->toBe('prod_first') + // NOT deactivated. Its prices would become unsellable, and a contract + // can be running on one of them — the asymmetry with archivePrice(). + ->and($adopt->duplicates)->toBe(['prod_second']); + + Log::shouldHaveReceived('warning')->once(); +}); +``` + +- [ ] **Step 2: Run test to verify it fails** + +```bash +docker exec -w /var/www/html/.claude/worktrees/cool-sammet-368fee clupilot-app-1 php artisan test tests/Feature/Billing/StripeProductAdoptionTest.php +``` + +Expected: FAIL — `Target class [App\Services\Billing\AdoptStripeProduct] does not exist.` + +- [ ] **Step 3: `AdoptStripeProduct` schreiben** + +```php + + */ + public array $duplicates = []; + + public function __construct(private readonly StripeClient $stripe) {} + + /** + * The id of an existing Stripe Product to use instead of creating one. + * + * @param array $metadata what the create call would send + * @param array $identifying metadata keys that mark a Product as ours + */ + public function __invoke(array $metadata, array $identifying): ?string + { + $metadata = array_map(fn ($value) => (string) $value, $metadata); + + $candidates = []; + + foreach ($this->stripe->activeProducts() as $product) { + if ($this->contradicts($product['metadata'], $metadata)) { + continue; + } + + if (! $this->confirms($product['metadata'], $metadata, $identifying)) { + continue; + } + + $candidates[] = $product; + } + + if ($candidates === []) { + return null; + } + + // Oldest first, and on a tie the lower id, so two runs agree. + usort($candidates, fn (array $a, array $b) => [$a['created'], $a['id']] <=> [$b['created'], $b['id']]); + + $adopted = array_shift($candidates); + + foreach ($candidates as $duplicate) { + $this->duplicates[] = $duplicate['id']; + + Log::warning('stripe: a second product claims to be the same thing — left alone, not deactivated', [ + 'product' => $duplicate['id'], + 'adopted' => $adopted['id'], + 'metadata' => $metadata, + ]); + } + + Log::info('stripe: adopted an existing product instead of creating a second one', [ + 'product' => $adopted['id'], + 'metadata' => $metadata, + ]); + + return $adopted['id']; + } + + /** + * @param array $found + * @param array $expected + */ + private function contradicts(array $found, array $expected): bool + { + foreach ($expected as $key => $value) { + if (array_key_exists($key, $found) && $found[$key] !== $value) { + return true; + } + } + + return false; + } + + /** + * Failing to contradict is not enough — an empty metadata bag contradicts + * nothing, and this list holds every Product on the account. + * + * @param array $found + * @param array $expected + * @param array $identifying + */ + private function confirms(array $found, array $expected, array $identifying): bool + { + foreach ($identifying as $key) { + if (isset($found[$key]) && $found[$key] === ($expected[$key] ?? null)) { + return true; + } + } + + return false; + } +} +``` + +- [ ] **Step 4: Als Singleton binden** + +In `app/Providers/AppServiceProvider::register()`, bei den anderen Bindungen: + +```php + // Singletons because both carry a record across one run that the caller + // reads afterwards: which products were duplicates, and how many prices + // were adopted rather than created. Resolved per call they would each + // start empty — SyncStripeCatalogue asks the container for PlanPrices + // once per catalogue row. + $this->app->singleton(AdoptStripeProduct::class); +``` + +`use App\Services\Billing\AdoptStripeProduct;` oben ergänzen. + +- [ ] **Step 5: Die Familie fragt zuerst** + +`SyncStripeCatalogue::handle()`, im Block `if ($productId === null)`: + +```php + if (! $dryRun) { + $metadata = ['plan_family' => $family->key, 'plan_family_id' => (string) $family->id]; + + // Asked BEFORE minting, for the same reason the Price side + // asks: a run that died between Stripe's create and our + // update left a Product this row does not know, and the key + // below stops protecting it after twenty-four hours. + // plan_family_id is what proves such a Product is this + // family's. + $productId = app(AdoptStripeProduct::class)($metadata, ['plan_family_id']); + + $productId ??= $stripe->createProduct( + $family->name, + $metadata, + // Covers a RETRY, for twenty-four hours, and nothing + // beyond them. What stops a second Product for one + // family is the adoption step above. + idempotencyKey: "clupilot-product-{$family->id}", + ); + + $family->update(['stripe_product_id' => $productId]); + } +``` + +- [ ] **Step 6: Das Modul fragt zuerst** + +`AddonPrices::product()`: + +```php + private function product(string $addonKey): string + { + $existing = StripeAddonPrice::query() + ->where('addon_key', $addonKey) + ->value('stripe_product_id'); + + if (is_string($existing) && $existing !== '') { + return $existing; + } + + $metadata = ['addon' => $addonKey]; + + // Asked before minting. A run that created this Product and died before + // any row carried its id leaves an orphan — and a second Product would + // make activePricesFor() ask about the wrong one, blinding the Price + // recognition for this module entirely. + $adopted = app(AdoptStripeProduct::class)($metadata, ['addon']); + + return $adopted ?? $this->stripe->createProduct( + app(AddonCatalogue::class)->name($addonKey), + $metadata, + // A retry's guard for twenty-four hours and nothing beyond them; + // what stops a second Product is the adoption step above. + idempotencyKey: "clupilot-addon-product-{$addonKey}", + ); + } +``` + +- [ ] **Step 7: Run the tests** + +```bash +docker exec -w /var/www/html/.claude/worktrees/cool-sammet-368fee clupilot-app-1 php artisan test tests/Feature/Billing +``` + +Expected: PASS. `ReverseChargePriceTest` ist auch hier der Zeuge: es zählt nach einem doppelten Sync 16 Register-Zeilen und 16 verschiedene Preis-IDs. + +- [ ] **Step 8: Commit** + +```bash +git add -- app/Services/Billing/AdoptStripeProduct.php app/Providers/AppServiceProvider.php app/Console/Commands/SyncStripeCatalogue.php app/Services/Billing/AddonPrices.php tests/Feature/Billing/StripeProductAdoptionTest.php +``` + +```bash +git commit -F - -- app/Services/Billing/AdoptStripeProduct.php app/Providers/AppServiceProvider.php app/Console/Commands/SyncStripeCatalogue.php app/Services/Billing/AddonPrices.php tests/Feature/Billing/StripeProductAdoptionTest.php <<'MSG' +Recognise the product Stripe already has + +The gap beside the guard. A run that creates a product and dies before storing +its id leaves an orphan exactly as a price does, and past the key's twenty-four +hours the next run makes a second one — after which activePricesFor() is asked +about the wrong product, price recognition goes blind for the whole family, and +every orphaned price on the first product is unreachable. + +Two things are unlike the price side, deliberately. No money gate: a product +carries no amount, so its metadata is the whole of the proof. And a duplicate is +reported, never deactivated — archiving a price is provably harmless because +Stripe keeps billing subscriptions already on it, but deactivating a product +makes its prices unsellable and contracts can be running on those. + +Silent about strangers, too. This asks for every active product on the account, +so an operator selling something else through it is an ordinary state, not a +finding worth a line on every run. + +Co-Authored-By: Claude Opus 5 +MSG +``` + +--- + +### Task 5: Der Abgleich zählt „angelegt" und „übernommen" getrennt + +**Files:** +- Modify: `app/Services/Billing/AdoptStripePrice.php` (Zähler) +- Modify: `app/Providers/AppServiceProvider.php` (Singleton) +- Modify: `app/Console/Commands/SyncStripeCatalogue.php` (Zählung und Schlusszeile, Doppel-Produkte melden) +- Test: `tests/Feature/Billing/StripeProductAdoptionTest.php` + +**Interfaces:** +- Consumes: `AdoptStripeProduct::$duplicates` aus Task 4. +- Produces: `AdoptStripePrice::$adoptions` (`int`), als Singleton gebunden. + +- [ ] **Step 1: Write the failing test** + +An `tests/Feature/Billing/StripeProductAdoptionTest.php`: + +```php +it('says it adopted, not that it created', function () { + // The one signal a human reads after an incident. The count is taken before + // ensure() runs, so without this the run reports objects created in Stripe + // when it made none — and telling those two apart is the whole point of the + // recognition step. + app(Kernel::class)->call('stripe:sync-catalogue'); + + $row = PlanPrice::query()->firstOrFail(); + $charged = PlanPrices::chargedCents($row, TaxTreatment::domestic()); + $orphan = (string) StripePlanPrice::query() + ->where('plan_price_id', $row->id)->where('reverse_charge', false) + ->value('stripe_price_id'); + StripePlanPrice::query()->where('stripe_price_id', $orphan)->delete(); + $this->stripe->keys = []; + + $this->artisan('stripe:sync-catalogue') + ->expectsOutputToContain('1 adopted') + ->assertSuccessful(); +}); + +it('names a duplicate product in its report, without touching it', function () { + $family = PlanFamily::query()->whereNotNull('stripe_product_id')->firstOrFail(); + $family->update(['stripe_product_id' => null]); + + $metadata = ['plan_family' => $family->key, 'plan_family_id' => (string) $family->id]; + $this->stripe->plantProduct('prod_first', $family->name, $metadata, created: 100); + $this->stripe->plantProduct('prod_second', $family->name, $metadata, created: 200); + + $this->artisan('stripe:sync-catalogue') + ->expectsOutputToContain('prod_second') + ->assertSuccessful(); + + expect($family->refresh()->stripe_product_id)->toBe('prod_first'); +}); +``` + +Die zusätzlichen `use`-Zeilen (`App\Models\PlanPrice`, `App\Models\StripePlanPrice`, `App\Services\Billing\PlanPrices`) oben in der Datei ergänzen. + +- [ ] **Step 2: Run test to verify it fails** + +```bash +docker exec -w /var/www/html/.claude/worktrees/cool-sammet-368fee clupilot-app-1 php artisan test tests/Feature/Billing/StripeProductAdoptionTest.php +``` + +Expected: FAIL — die Ausgabe sagt „created", und die Doppel-Produkt-ID steht nirgends. + +- [ ] **Step 3: `AdoptStripePrice` zählt seine Übernahmen** + +Eigenschaft und Inkrement: + +```php + /** + * How many Prices this run took over rather than minted. + * + * Read by stripe:sync-catalogue, which counts an intent BEFORE calling + * ensure() and so cannot tell the two apart on its own — comparing the Price + * id before and after does not distinguish them either, because there is no + * id before in either case. + */ + public int $adoptions = 0; +``` + +und, unmittelbar vor dem `return $adopted['id'];`: + +```php + $this->adoptions++; +``` + +- [ ] **Step 4: Als Singleton binden** + +In `AppServiceProvider::register()`, neben der Bindung aus Task 4: + +```php + $this->app->singleton(AdoptStripePrice::class); +``` + +`use App\Services\Billing\AdoptStripePrice;` oben ergänzen. + +- [ ] **Step 5: Das Kommando zählt und meldet getrennt** + +In `SyncStripeCatalogue::handle()`, ganz am Anfang von `handle()`: + +```php + $adoptPrices = app(AdoptStripePrice::class); + $adoptProducts = app(AdoptStripeProduct::class); + $adoptedBefore = $adoptPrices->adoptions; +``` + +und die Schlussausgabe ersetzen: + +```php + $adopted = $adoptPrices->adoptions - $adoptedBefore; + $minted = max(0, $created - $adopted); + + foreach ($adoptProducts->duplicates as $duplicate) { + // Named here as well as in the log: an operator running the sweep + // reads this, and a second Product for one family is something only + // a person can resolve — we deliberately do not deactivate it. + $this->warn(" duplicate product {$duplicate} — left active, a product's prices become unsellable if it is deactivated"); + } + + if ($created === 0) { + $this->info('Stripe is already in step with the catalogue.'); + + return self::SUCCESS; + } + + $this->info($dryRun + ? "{$created} object(s) would be created. Run without --dry-run to create them." + : "{$minted} object(s) created, {$adopted} adopted in Stripe."); + + return self::SUCCESS; +``` + +`use App\Services\Billing\AdoptStripePrice;` und `use App\Services\Billing\AdoptStripeProduct;` oben ergänzen. + +- [ ] **Step 6: Run the tests** + +```bash +docker exec -w /var/www/html/.claude/worktrees/cool-sammet-368fee clupilot-app-1 php artisan test tests/Feature/Billing +``` + +Expected: PASS. `ReverseChargePriceTest:335` prüft `'already in step'` — diese Zeile bleibt unverändert. + +- [ ] **Step 7: Commit** + +```bash +git add -- app/Services/Billing/AdoptStripePrice.php app/Providers/AppServiceProvider.php app/Console/Commands/SyncStripeCatalogue.php tests/Feature/Billing/StripeProductAdoptionTest.php +``` + +```bash +git commit -F - -- app/Services/Billing/AdoptStripePrice.php app/Providers/AppServiceProvider.php app/Console/Commands/SyncStripeCatalogue.php tests/Feature/Billing/StripeProductAdoptionTest.php <<'MSG' +Say what was adopted, not that it was created + +The count is taken before ensure() runs, so the sweep reported objects created in +Stripe when it had made none — and after the next interrupted run, that line is +the first thing a human reads. Telling the two apart is the whole point of the +recognition step. + +Comparing the price id before and after the call does not distinguish them +either: there is no id before, in either case. AdoptStripePrice counts its own +adoptions instead, which is why it and its product sibling are now singletons — +resolved per call, a counter on the instance would never pass one. + +Duplicate products are named in the report as well as the log. We deliberately do +not deactivate them, so only a person can resolve one, and a person reads this. + +Co-Authored-By: Claude Opus 5 +MSG +``` + +--- + +### Task 6: `stripe:sweep-orphan-prices` + +**Files:** +- Create: `app/Console/Commands/SweepOrphanStripePrices.php` +- Test: `tests/Feature/Billing/SweepOrphanPricesTest.php` +- Modify: `docs/superpowers/specs/2026-07-30-stripe-price-adoption-design.md` (§14 abhaken) + +**Interfaces:** +- Consumes: `StripeClient::activePricesFor()`, `archivePrice()`. +- Produces: das Kommando `stripe:sweep-orphan-prices {--archive} {--dry-run}`. + +- [ ] **Step 1: Write the failing test** + +`tests/Feature/Billing/SweepOrphanPricesTest.php`: + +```php +stripe = new FakeStripeClient; + app()->instance(StripeClient::class, $this->stripe); + app(Kernel::class)->call('stripe:sync-catalogue'); + + $this->product = (string) StripeAddonPrice::query() + ->where('addon_key', 'priority_support') + ->value('stripe_product_id'); + + $this->known = (string) StripeAddonPrice::query() + ->where('addon_key', 'priority_support') + ->where('interval', 'month') + ->where('reverse_charge', false) + ->value('stripe_price_id'); + + $this->stripe->plantPrice('price_orphan', $this->product, 9900, 'EUR', 'month', []); +}); + +it('reports the orphan and leaves it alone', function () { + $this->artisan('stripe:sweep-orphan-prices') + ->expectsOutputToContain('price_orphan') + ->assertSuccessful(); + + expect($this->stripe->archived)->toBe([]); +}); + +it('never reports a price a row knows', function () { + $this->artisan('stripe:sweep-orphan-prices') + ->doesntExpectOutputToContain($this->known) + ->assertSuccessful(); +}); + +it('stops selling the orphan when asked to', function () { + $this->artisan('stripe:sweep-orphan-prices', ['--archive' => true]) + ->assertSuccessful(); + + // Archiving is harmless for the same reason it always is here: Stripe goes + // on billing every subscription already on an archived Price, it merely + // stops being sold. + expect($this->stripe->archived)->toBe(['price_orphan']); +}); + +it('touches nothing on a dry run, even when asked to archive', function () { + $this->artisan('stripe:sweep-orphan-prices', ['--archive' => true, '--dry-run' => true]) + ->expectsOutputToContain('price_orphan') + ->assertSuccessful(); + + expect($this->stripe->archived)->toBe([]); +}); +``` + +- [ ] **Step 2: Run test to verify it fails** + +```bash +docker exec -w /var/www/html/.claude/worktrees/cool-sammet-368fee clupilot-app-1 php artisan test tests/Feature/Billing/SweepOrphanPricesTest.php +``` + +Expected: FAIL — `The command "stripe:sweep-orphan-prices" does not exist.` + +- [ ] **Step 3: Das Kommando schreiben** + +`app/Console/Commands/SweepOrphanStripePrices.php`: + +```php +option('dry-run'); + $archive = (bool) $this->option('archive'); + + if (! $dryRun && ! $stripe->isConfigured()) { + $this->error('Stripe is not configured (STRIPE_SECRET is empty). Nothing was read.'); + + return self::FAILURE; + } + + $known = StripeAddonPrice::query()->pluck('stripe_price_id') + ->merge(StripePlanPrice::query()->pluck('stripe_price_id')) + ->filter() + ->flip(); + + $found = 0; + + foreach ($this->products() as $productId) { + foreach ($stripe->activePricesFor($productId) as $price) { + if ($known->has($price['id'])) { + continue; + } + + $found++; + + $this->line(sprintf( + ' orphan %s %d %s %s product %s %s', + $price['id'], + $price['unit_amount'], + $price['currency'], + $price['interval'], + $productId, + $price['metadata'] === [] ? 'no metadata' : json_encode($price['metadata']), + )); + + if ($archive && ! $dryRun) { + $stripe->archivePrice($price['id']); + } + } + } + + $this->newLine(); + + if ($found === 0) { + $this->info('Every active price at our products is accounted for.'); + + return self::SUCCESS; + } + + $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.", + }); + + return self::SUCCESS; + } + + /** + * Every Stripe Product this platform owns: one per plan family, one per + * module. Read from our own rows rather than from Stripe, because a Product + * we do not know is not one whose Prices we can judge. + * + * @return array + */ + private function products(): array + { + return PlanFamily::query()->whereNotNull('stripe_product_id')->pluck('stripe_product_id') + ->merge(StripeAddonPrice::query()->pluck('stripe_product_id')) + ->filter() + ->unique() + ->values() + ->all(); + } +} +``` + +- [ ] **Step 4: Run the tests** + +```bash +docker exec -w /var/www/html/.claude/worktrees/cool-sammet-368fee clupilot-app-1 php artisan test tests/Feature/Billing/SweepOrphanPricesTest.php +``` + +Expected: PASS, vier Tests. + +- [ ] **Step 5: §14 des Vorgängers abhaken** + +In `docs/superpowers/specs/2026-07-30-stripe-price-adoption-design.md` §14 wird +Punkt für Punkt abgehakt, jeweils mit der Datei, die ihn schließt: + +| §14-Eintrag | Ergebnis | +|---|---| +| Waisen-Produkte | **erledigt** — `AdoptStripeProduct`, beide Verdrahtungen | +| Geld-Tor an der falschen Stelle | **erledigt** — geprüft jetzt in `AdoptStripePrice` | +| Null-Schranke in `PlanPrices` | **bewusst nicht gebaut** — Begründung in der neuen Spec §3: der Umzug nimmt ihr den Zweck, und sie würde einen kostenlosen Tarif unverkäuflich machen | +| Abgleich zählt „angelegt" für Übernommenes | **erledigt** — getrennte Zählung | +| Warnung ohne Drosselung | **erledigt** — einmal pro Preis und Tag | +| Fake ersetzt Metadaten / castet nicht | **erledigt** | +| `created` kann gleichstehen | **erledigt** — Gleichstand über die Preis-ID | +| Blättern bricht still ab | **erledigt** — wirft, für Preise und Produkte | +| Fingerabdruck und POST-Rumpf müssen zusammenbleiben | **bleibt offen** — es ist eine Naht, kein Fehler; der Test darüber existiert | +| `HostStepsTest` (DNS-Zone) | **erledigt durch die zweite Session** (`a842512`), Eintrag streichen | +| Sprunghafter `PlanCatalogueTest` | **bleibt offen** — Ursachensuche, eigene Sitzung | + +Der Abschnitt bekommt einen Kopfsatz, der sagt, wo die Fortsetzung steht: +`docs/superpowers/specs/2026-07-30-stripe-adoption-followups-design.md`. + +- [ ] **Step 6: Voller Testlauf** + +```bash +docker exec -w /var/www/html/.claude/worktrees/cool-sammet-368fee clupilot-app-1 php artisan test +``` + +Expected: PASS. Sollte etwas außerhalb von `tests/Feature/Billing` fallen, ist das ein echter Fund — melden. + +- [ ] **Step 7: Commit** + +```bash +git add -- app/Console/Commands/SweepOrphanStripePrices.php tests/Feature/Billing/SweepOrphanPricesTest.php docs/superpowers/specs/2026-07-30-stripe-price-adoption-design.md +``` + +```bash +git commit -F - -- app/Console/Commands/SweepOrphanStripePrices.php tests/Feature/Billing/SweepOrphanPricesTest.php docs/superpowers/specs/2026-07-30-stripe-price-adoption-design.md <<'MSG' +Give the operator something to do about an orphan nobody can adopt + +The recognition step refuses a price that proves nothing about itself, and that +is right — adopting a stranger's price is worse than minting a second one. It +left an operator with a log line and no way to act on it. + +Reports by default and touches nothing. With --archive the orphans stop being +sold, which is harmless for the reason it always is here: Stripe goes on billing +every subscription already on an archived price, and nothing is sold on a price +no row knows. + +Co-Authored-By: Claude Opus 5 +MSG +``` + +--- + +## Nach dem letzten Task + +- [ ] **Voller Testlauf im Worktree**, nicht im Haupt-Repo. +- [ ] **Ein Review über den Gesamtdiff**, dann **eine** Fix-Runde, dann **ein** Re-Review über den Fix-Diff (R22.1). Danach werden offene Befunde mit schriftlicher Begründung geparkt. +- [ ] **Nicht mergen, solange die zweite Session arbeitet.** Anweisung des Nutzers vom 2026-07-30. Der Merge nach `main` wartet, bis das Haupt-Repo einen sauberen Baum hat. 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 47d2eba..f246df0 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 @@ -103,10 +103,23 @@ einen vierteljährlichen oder mengenteilenden Preis überhaupt hinstellen kann. wiederkehrende Preise **mit** ihren preisbestimmenden Eigenschaften; was davon zulässig ist, entscheidet der Aufrufer. -**Verwandt, hier mit erledigt:** `PlanPrices::ensure()` bekommt die -`<= 0`-Schranke, die `AddonPrices::ensure()` schon hat. Sie ist das Einzige, was -einen `billing_scheme: tiered`-Preis (bei dem Stripe `unit_amount: null` -liefert, hier als 0 gelesen) auf der Paketseite überhaupt erreichbar macht. +### Die Null-Schranke in `PlanPrices` wird **nicht** gebaut + +Der Abschluss-Review hatte sie vorgeschlagen, und §14 des Vorgängers hat sie +notiert: `AddonPrices::ensure()` hat eine `<= 0`-Schranke, `PlanPrices::ensure()` +nicht, und das sei das Einzige, was einen `billing_scheme: tiered`-Preis (bei dem +Stripe `unit_amount: null` liefert, hier als 0 gelesen) auf der Paketseite +erreichbar macht. + +Das stimmte — **bis zu diesem Umzug**. Sobald `AdoptStripePrice` selbst +`billing_scheme !== 'per_unit'` ablehnt, ist ein gestufter Preis unabhängig vom +Betrag des Aufrufers ausgeschlossen. Die Schranke hätte dann keinen Zweck mehr, +wäre aber eine Verhaltensänderung: ein Paket zu null — ein kostenloser Tarif, +heute nicht im Katalog, aber jederzeit anlegbar — bekäme keinen Stripe-Preis +mehr und wäre nicht verkäuflich. Eine Schranke ohne Nutzen, die einen künftigen +Fall bricht, wird nicht eingebaut. + +Der §14-Eintrag wird entsprechend abgehakt: nicht gebaut, und warum. ## 4. `stripe:sweep-orphan-prices` — das Aufräum-Kommando @@ -171,7 +184,9 @@ Ohne Entwurfsentscheidung — bauen, Test, weiter (R22.5): (`SyncStripeCatalogue` holt `PlanPrices` pro Zeile neu), ein Zähler auf der Instanz käme also nie über eins hinaus. Sie hat sonst keinen Zustand, und der Zähler ist das, was den Bericht wahr macht. -6. `PlanPrices::ensure()` bekommt die `<= 0`-Schranke — siehe §3. +6. ~~`PlanPrices::ensure()` bekommt die `<= 0`-Schranke~~ — **entfällt**, siehe + §3: der Umzug des Geld-Tors nimmt ihr den Zweck, und sie würde einen + kostenlosen Tarif unverkäuflich machen. ## 7. Was nicht dazugehört @@ -234,7 +249,8 @@ Und an `StripeIdempotencyKeyTest.php`: Blättern **wirft** bei fehlender `id` Warnung drosseln - `app/Services/Billing/AddonPrices.php` — Produkt-Wiedererkennung in `product()` -- `app/Services/Billing/PlanPrices.php` — `<= 0`-Schranke +- `app/Providers/AppServiceProvider.php` — beide Adoptions-Klassen als + Singleton, damit ihr Lauf-Protokoll (`$duplicates`, `$adoptions`) trägt - `app/Console/Commands/SyncStripeCatalogue.php` — Produkt-Wiedererkennung, getrennte Zählung, Kommentar berichtigt - `tests/Feature/Billing/StripePriceAdoptionTest.php`, From 40532c6e0267aaf040104d42da2b08a52aebde20 Mon Sep 17 00:00:00 2001 From: nexxo Date: Thu, 30 Jul 2026 17:04:25 +0200 Subject: [PATCH 03/14] Put the money gate in the class that promises it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AdoptStripePrice says in its own docblock that no adoption can move money, and could compare three properties. The four that also decide what a Price charges were filtered in HttpStripeClient — where FakeStripeClient cannot express them, so no test of the class could reach the promise it makes, and a third client implementation would drop the check in silence. The client reports them now and the caller decides. Absent keys stay Stripe's defaults, which is the direction that matters: read the other way round the gate refuses every legitimate price and recognition becomes a no-op nothing alerts on. And the unexplained-orphan warning is a state, not an event. It fired on every sweep and every customer booking for as long as the orphan existed; once a day per price is enough to act on. Co-Authored-By: Claude Opus 5 --- app/Services/Billing/AdoptStripePrice.php | 39 +++++++-- app/Services/Stripe/FakeStripeClient.php | 33 +++++--- app/Services/Stripe/HttpStripeClient.php | 45 +++------- app/Services/Stripe/StripeClient.php | 45 ++++------ .../Billing/StripeIdempotencyKeyTest.php | 82 +++++++------------ .../Billing/StripePriceAdoptionTest.php | 58 +++++++++++++ 6 files changed, 171 insertions(+), 131 deletions(-) diff --git a/app/Services/Billing/AdoptStripePrice.php b/app/Services/Billing/AdoptStripePrice.php index 41461d5..1e7d4b7 100644 --- a/app/Services/Billing/AdoptStripePrice.php +++ b/app/Services/Billing/AdoptStripePrice.php @@ -3,6 +3,7 @@ namespace App\Services\Billing; use App\Services\Stripe\StripeClient; +use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Log; /** @@ -68,9 +69,22 @@ final class AdoptStripePrice $candidates = []; foreach ($this->stripe->activePricesFor($productId) as $price) { + // The money gate, and the whole of it — this class promises that no + // adoption can move money, and until these four moved here it could + // compare only the first three. A Price at our figure with + // interval_count 3 bills quarterly; one with transform_quantity + // divide_by 10 charges a customer holding three packs for one; a + // metered or tiered one does not charge an amount at all. Stripe's + // dashboard copies metadata when it duplicates a Price, so such a + // Price can carry our own identifying key and be indistinguishable + // from ours by every other test here. if ($price['unit_amount'] !== $amountCents || $price['currency'] !== strtoupper($currency) - || $price['interval'] !== $interval) { + || $price['interval'] !== $interval + || $price['interval_count'] !== 1 + || $price['usage_type'] !== 'licensed' + || $price['transform_quantity'] + || $price['billing_scheme'] !== 'per_unit') { continue; } @@ -86,13 +100,22 @@ final class AdoptStripePrice } if (! $this->confirms($price['metadata'], $metadata, $identifying)) { - Log::warning('stripe: left an unexplained active price alone rather than adopting it', [ - 'price' => $price['id'], - 'product' => $productId, - 'amount_cents' => $amountCents, - 'currency' => strtoupper($currency), - 'interval' => $interval, - ]); + // Once a day per Price. An orphan nobody can adopt is a STATE, + // not an event, and this runs in a customer's module booking as + // well as in the sweep — without a throttle it writes a line + // every time anybody books anything, for as long as the orphan + // exists, and the clean-up command that would end it is a + // separate one. Cache::add is this repo's throttle; see + // App\Livewire\Billing. + if (Cache::add('stripe:unexplained-price:'.$price['id'], true, now()->addDay())) { + Log::warning('stripe: left an unexplained active price alone rather than adopting it', [ + 'price' => $price['id'], + 'product' => $productId, + 'amount_cents' => $amountCents, + 'currency' => strtoupper($currency), + 'interval' => $interval, + ]); + } continue; } diff --git a/app/Services/Stripe/FakeStripeClient.php b/app/Services/Stripe/FakeStripeClient.php index 25d4ebc..262f8d7 100644 --- a/app/Services/Stripe/FakeStripeClient.php +++ b/app/Services/Stripe/FakeStripeClient.php @@ -19,7 +19,7 @@ class FakeStripeClient implements StripeClient /** @var array */ public array $products = []; - /** @var array */ + /** @var array */ public array $prices = []; /** @var array */ @@ -218,6 +218,13 @@ class FakeStripeClient implements StripeClient 'amount' => $amountCents, 'currency' => $currency, 'interval' => $interval, + // Stripe's defaults, and the only thing createPrice() can produce: + // it sends none of these four, so every Price this platform mints + // carries exactly them. + 'interval_count' => 1, + 'usage_type' => 'licensed', + 'transform_quantity' => false, + 'billing_scheme' => 'per_unit', 'metadata' => $metadata, // Stripe stamps every object with its creation time and // AdoptStripePrice takes the OLDEST of several orphans. Counted @@ -258,15 +265,6 @@ class FakeStripeClient implements StripeClient // No failIfAsked(): createPrice(), the call this one stands in front of, // does not fail either, and a test that scripts an outage for a refund // must not have its catalogue sync change behaviour underneath it. - // - // None of HttpStripeClient's charging-property filter either — the four - // fields it names (interval_count, usage_type, transform_quantity, - // billing_scheme). This fake only ever holds prices its own - // createPrice()/plantPrice() put here, and neither takes an argument for - // any of them: there is nothing non-standard a test could plant, so the - // filter would have nothing to do. Not an oversight. What that filter - // does is proven where it lives, over Http::fake — see - // tests/Feature/Billing/StripeIdempotencyKeyTest.php. $found = []; foreach ($this->prices as $id => $price) { @@ -279,6 +277,10 @@ class FakeStripeClient implements StripeClient 'unit_amount' => $price['amount'], 'currency' => strtoupper($price['currency']), 'interval' => $price['interval'], + 'interval_count' => $price['interval_count'], + 'usage_type' => $price['usage_type'], + 'transform_quantity' => $price['transform_quantity'], + 'billing_scheme' => $price['billing_scheme'], 'created' => $price['created'], 'metadata' => $price['metadata'], ]; @@ -312,12 +314,23 @@ class FakeStripeClient implements StripeClient string $interval, array $metadata = [], int $created = 1, + int $intervalCount = 1, + string $usageType = 'licensed', + bool $transformQuantity = false, + string $billingScheme = 'per_unit', ): void { $this->prices[$id] = [ 'product' => $productId, 'amount' => $amountCents, 'currency' => $currency, 'interval' => $interval, + // Defaulted to what we could have minted ourselves, so a test that + // does not care says nothing — and a test about the money gate says + // exactly which property it is about. + 'interval_count' => $intervalCount, + 'usage_type' => $usageType, + 'transform_quantity' => $transformQuantity, + 'billing_scheme' => $billingScheme, 'metadata' => $metadata, 'created' => $created, ]; diff --git a/app/Services/Stripe/HttpStripeClient.php b/app/Services/Stripe/HttpStripeClient.php index fc90f32..b90cbe9 100644 --- a/app/Services/Stripe/HttpStripeClient.php +++ b/app/Services/Stripe/HttpStripeClient.php @@ -170,43 +170,24 @@ class HttpStripeClient implements StripeClient foreach ($data as $price) { $recurring = (array) ($price['recurring'] ?? []); - // Only prices we could have minted ourselves. createPrice() sends - // product, unit_amount, currency, recurring[interval] and metadata - // and nothing else, so Stripe defaults every other field that - // decides what a Price CHARGES — and anything but those defaults - // is a Price this platform did not write. Skipped here rather than - // returned, because AdoptStripePrice compares amount, currency and - // interval only and trusts that triple as the whole of what a - // Price charges: - // - // - interval_count multiplies the period — 3 bills every three - // months — while `interval` still reads 'month'; - // - usage_type other than 'licensed' bills metered usage; - // - a transform_quantity divides the quantity before charging, - // and modules are billed BY quantity — SyncStripeAddonItems - // sums a pack into one item at quantity n — so divide_by 10 - // would charge a customer holding three for one; - // - billing_scheme 'tiered' keeps the money in tiers, leaving - // unit_amount null. Read as 0 below, so such a Price fails the - // amount match only while the caller's own figure is not 0 — - // and PlanPrices::ensure() has no zero guard, so that is not a - // condition to rest on. - // - // An ABSENT key is Stripe's default and no reason to reject. - // Read the other way round this filter would refuse every - // legitimate price and turn recognition into a permanent no-op. - if ((int) ($recurring['interval_count'] ?? 1) !== 1 - || ($recurring['usage_type'] ?? 'licensed') !== 'licensed' - || (array) ($price['transform_quantity'] ?? []) !== [] - || ($price['billing_scheme'] ?? 'per_unit') !== 'per_unit') { - continue; - } - $prices[] = [ 'id' => (string) ($price['id'] ?? ''), 'unit_amount' => (int) ($price['unit_amount'] ?? 0), 'currency' => strtoupper((string) ($price['currency'] ?? '')), 'interval' => (string) ($recurring['interval'] ?? ''), + // The four properties that decide what a Price CHARGES + // beyond its amount. createPrice() sends none of them, so + // Stripe defaults every one — and an ABSENT key is that + // default, never a reason to reject. Reported rather than + // filtered: the class that promises adoption cannot move + // money is AdoptStripePrice, and it could not keep that + // promise while these lived here, where FakeStripeClient + // cannot express them and no test of that class could reach + // them. + 'interval_count' => (int) ($recurring['interval_count'] ?? 1), + 'usage_type' => (string) ($recurring['usage_type'] ?? 'licensed'), + 'transform_quantity' => (array) ($price['transform_quantity'] ?? []) !== [], + 'billing_scheme' => (string) ($price['billing_scheme'] ?? 'per_unit'), 'created' => (int) ($price['created'] ?? 0), 'metadata' => array_map( fn ($value) => (string) $value, diff --git a/app/Services/Stripe/StripeClient.php b/app/Services/Stripe/StripeClient.php index 0ffed51..ab7908d 100644 --- a/app/Services/Stripe/StripeClient.php +++ b/app/Services/Stripe/StripeClient.php @@ -125,39 +125,30 @@ interface StripeClient ): string; /** - * Every active recurring Price of a Product, as Stripe holds them. + * Every active recurring Price of a Product, as Stripe holds them, with the + * properties that decide what each one CHARGES. * - * The half of the catalogue mirror that was missing: nothing here ever asked - * Stripe what it already had. A run that created a Price and died before the - * row was written left an orphan our table never learned about, and once the - * idempotency key expired the next run made a SECOND live Price for the same - * money — the exact duplicate the key exists to prevent. See - * App\Services\Billing\AdoptStripePrice, which is the only caller. + * `interval_count` multiplies the period — 3 bills every three months while + * `interval` still reads 'month'. `usage_type` other than 'licensed' bills + * metered usage. `transform_quantity` divides the quantity before charging, + * and modules are billed BY quantity, so a divide_by of 10 would charge a + * customer holding three for one. `billing_scheme` 'tiered' keeps the money + * in tiers and leaves `unit_amount` null, which reads as 0 here. + * + * All four are REPORTED, not filtered. Which of them is acceptable is the + * caller's decision, and the caller is App\Services\Billing\AdoptStripePrice + * — the class whose docblock promises that adoption cannot move money. That + * promise has to be testable in the class that makes it, and it was not + * while these fields were dropped here: FakeStripeClient cannot express + * them. + * + * An absent key is Stripe's own default — 1, 'licensed', none, 'per_unit'. * * Active ones only, because the question being asked is "is there something * here I can sell on?". `currency` comes back UPPER CASE, the way our own * tables hold it. * - * FOUR properties are checked, and they are named because the list is what - * the contract is: `recurring.interval_count` is 1, `recurring.usage_type` - * is `licensed`, there is no `transform_quantity`, and `billing_scheme` is - * `per_unit`. Those are Stripe's own defaults, and createPrice() sets none of - * the four — so anything else is a Price this platform did not write, and is - * filtered out here rather than returned. - * - * The reason it is filtered at all: AdoptStripePrice compares the amount, - * currency and interval of the shape below and trusts that triple as the - * WHOLE of what a Price charges. A Price billed every three months, on - * metered usage, dividing the quantity before charging, or pricing in tiers - * would otherwise pass that triple on `interval => 'month'` alone and be - * adopted as if it charged our figure per unit per month — which is adoption - * moving money, the one thing nothing here may do. - * - * Not a list of everything Stripe can put on a Price: it is the list of - * fields known to change what one CHARGES. A fifth would have to be added - * here, at the boundary, before the shape below is handed on. - * - * @return array}> + * @return array}> */ public function activePricesFor(string $productId): array; diff --git a/tests/Feature/Billing/StripeIdempotencyKeyTest.php b/tests/Feature/Billing/StripeIdempotencyKeyTest.php index ead4498..47686e0 100644 --- a/tests/Feature/Billing/StripeIdempotencyKeyTest.php +++ b/tests/Feature/Billing/StripeIdempotencyKeyTest.php @@ -155,6 +155,13 @@ it('pages through every active price of a product', function () { // side is which. 'currency' => 'EUR', 'interval' => 'month', + // Stripe's own defaults, absent from every price in this response — + // reported here rather than filtered, which is the whole point of + // this test's neighbour below. + 'interval_count' => 1, + 'usage_type' => 'licensed', + 'transform_quantity' => false, + 'billing_scheme' => 'per_unit', 'created' => 100, 'metadata' => ['addon' => 'priority_support'], ]) @@ -170,73 +177,40 @@ it('pages through every active price of a product', function () { Http::assertSent(fn ($request) => str_contains($request->url(), 'active=true')); }); -it('skips a price it could not have created itself, so the money gate never trusts a partial recurrence', function () { +it('reports the properties that decide what a price charges, and filters none of them', function () { Http::fake([ 'api.stripe.com/*' => Http::response([ 'data' => [ - ['id' => 'price_monthly', 'unit_amount' => 3480, 'currency' => 'eur', - 'created' => 100, 'recurring' => ['interval' => 'month'], - 'metadata' => ['addon' => 'priority_support']], + ['id' => 'price_ordinary', 'unit_amount' => 3480, 'currency' => 'eur', + 'created' => 100, 'recurring' => ['interval' => 'month'], 'metadata' => []], ['id' => 'price_quarterly', 'unit_amount' => 3480, 'currency' => 'eur', 'created' => 101, 'recurring' => ['interval' => 'month', 'interval_count' => 3], - 'metadata' => ['addon' => 'priority_support']], - ['id' => 'price_metered', 'unit_amount' => 3480, 'currency' => 'eur', - 'created' => 102, 'recurring' => ['interval' => 'month', 'usage_type' => 'metered'], - 'metadata' => ['addon' => 'priority_support']], - ], - 'has_more' => false, - ]), - ]); - - $prices = (new HttpStripeClient)->activePricesFor('prod_1'); - - // A hand-duplicated dashboard price keeps our metadata, so nothing - // downstream could tell it apart, and the module would bill quarterly. - // - // None of the three planted prices carries `transform_quantity` or - // `billing_scheme` at all, and the ordinary one still comes back — so this - // also holds the other direction of the next test's filter: an absent key is - // Stripe's default, not a reason to reject. - expect(collect($prices)->pluck('id')->all())->toBe(['price_monthly']); -}); - -it('skips a price that would charge our figure for the wrong quantity', function () { - Http::fake([ - 'api.stripe.com/*' => Http::response([ - 'data' => [ - // Spelled out as Stripe actually sends them for an ordinary - // price: transform_quantity null, billing_scheme per_unit. - ['id' => 'price_ordinary', 'unit_amount' => 3480, 'currency' => 'eur', - 'created' => 100, 'recurring' => ['interval' => 'month'], - 'transform_quantity' => null, 'billing_scheme' => 'per_unit', - 'metadata' => ['addon' => 'priority_support']], + 'metadata' => []], ['id' => 'price_divided', 'unit_amount' => 3480, 'currency' => 'eur', - 'created' => 101, 'recurring' => ['interval' => 'month'], - 'transform_quantity' => ['divide_by' => 10, 'round' => 'up'], - 'billing_scheme' => 'per_unit', - 'metadata' => ['addon' => 'priority_support']], - ['id' => 'price_tiered', 'unit_amount' => null, 'currency' => 'eur', 'created' => 102, 'recurring' => ['interval' => 'month'], - 'billing_scheme' => 'tiered', - 'metadata' => ['addon' => 'priority_support']], + 'transform_quantity' => ['divide_by' => 10, 'round' => 'up'], 'metadata' => []], + ['id' => 'price_tiered', 'unit_amount' => null, 'currency' => 'eur', + 'created' => 103, 'recurring' => ['interval' => 'month', 'usage_type' => 'metered'], + 'billing_scheme' => 'tiered', 'metadata' => []], ], 'has_more' => false, ]), ]); - $prices = (new HttpStripeClient)->activePricesFor('prod_1'); + $prices = collect((new HttpStripeClient)->activePricesFor('prod_1'))->keyBy('id'); - // The concrete harm: modules are billed BY quantity — SyncStripeAddonItems - // sums a pack into ONE item at quantity n — so price_divided is our exact - // figure, on our Product, carrying our `addon` key, and adoption would take - // it. A customer holding three would then be charged ceil(3/10) = 1. - // - // price_tiered is refused by name rather than by luck: Stripe reports - // unit_amount null for a tiered price, activePricesFor() casts that to 0, and - // the amount match alone only rejects it while the caller's own figure is not - // 0 — which PlanPrices::ensure(), unlike AddonPrices::ensure(), does not - // guarantee. - expect(collect($prices)->pluck('id')->all())->toBe(['price_ordinary']); + // All four come back. Deciding which are usable is AdoptStripePrice's job — + // it is the class that promises adoption cannot move money, and it could not + // keep that promise while the fields lived only here. + expect($prices)->toHaveCount(4) + ->and($prices['price_ordinary'])->toMatchArray([ + 'interval_count' => 1, 'usage_type' => 'licensed', + 'transform_quantity' => false, 'billing_scheme' => 'per_unit', + ]) + ->and($prices['price_quarterly']['interval_count'])->toBe(3) + ->and($prices['price_divided']['transform_quantity'])->toBeTrue() + ->and($prices['price_tiered']['usage_type'])->toBe('metered') + ->and($prices['price_tiered']['billing_scheme'])->toBe('tiered'); }); it('writes metadata onto a price that already exists', function () { diff --git a/tests/Feature/Billing/StripePriceAdoptionTest.php b/tests/Feature/Billing/StripePriceAdoptionTest.php index df68ab1..ca10ff2 100644 --- a/tests/Feature/Billing/StripePriceAdoptionTest.php +++ b/tests/Feature/Billing/StripePriceAdoptionTest.php @@ -610,3 +610,61 @@ it('keeps only the lowest-id row in every group of duplicates, and leaves the re 'created_at' => now(), 'updated_at' => now(), ]))->toThrow(UniqueConstraintViolationException::class); }); + +it('refuses a price whose recurrence is not one we could have minted', function () { + // The money gate, asked of AdoptStripePrice itself rather than of the + // client's filter. Until this moved, these four properties were rejected + // inside HttpStripeClient — and FakeStripeClient could not express them, so + // no test of this class could reach the promise its docblock makes. + $this->stripe->plantPrice('price_quarterly', 'prod_support', 3480, 'EUR', 'month', + moduleMetadata(), intervalCount: 3); + + expect(adoptModulePrice())->toBeNull(); +}); + +it('refuses a price that meters usage instead of selling a licence', function () { + $this->stripe->plantPrice('price_metered', 'prod_support', 3480, 'EUR', 'month', + moduleMetadata(), usageType: 'metered'); + + expect(adoptModulePrice())->toBeNull(); +}); + +it('refuses a price that divides the quantity before charging', function () { + // Modules are billed BY quantity — SyncStripeAddonItems sums a pack into one + // item at quantity n — so a divide_by price would charge a customer holding + // three for one. + $this->stripe->plantPrice('price_divided', 'prod_support', 3480, 'EUR', 'month', + moduleMetadata(), transformQuantity: true); + + expect(adoptModulePrice())->toBeNull(); +}); + +it('refuses a price that keeps its money in tiers', function () { + $this->stripe->plantPrice('price_tiered', 'prod_support', 3480, 'EUR', 'month', + moduleMetadata(), billingScheme: 'tiered'); + + expect(adoptModulePrice())->toBeNull(); +}); + +it('still adopts a price that carries every one of Stripe\'s defaults', function () { + // The direction that matters more than the four above: read the wrong way + // round, the gate refuses every legitimate price and recognition becomes a + // permanent no-op that nothing would ever alert on. + $this->stripe->plantPrice('price_ordinary', 'prod_support', 3480, 'EUR', 'month', moduleMetadata()); + + expect(adoptModulePrice())->toBe('price_ordinary'); +}); + +it('warns about an unexplained price once a day, not once a booking', function () { + Log::spy(); + + $this->stripe->plantPrice('price_by_hand', 'prod_support', 3480, 'EUR', 'month', []); + + // ensure() runs inside a customer's module booking, so without a throttle + // this line is written every time anybody books anything. + adoptModulePrice(); + adoptModulePrice(); + adoptModulePrice(); + + Log::shouldHaveReceived('warning')->once(); +}); From 4b99c6d5c6ef6639dca6e6f7ecd2929402cfb1e6 Mon Sep 17 00:00:00 2001 From: nexxo Date: Thu, 30 Jul 2026 17:14:46 +0200 Subject: [PATCH 04/14] Make the fake behave like Stripe where it matters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three ways it did not. It replaced metadata where Stripe merges — and merging is the property AdoptStripePrice's "write only when it differs" comparison is built around. It handed metadata back uncast where the wire returns strings, so a test could pass on an int that production's strict === rejects forever. And a tie on `created` fell to insertion order, so two runs could adopt different prices. Paging now throws instead of stopping when Stripe says there is another page and the last item carries no id. invoiceLines() stops there and it costs a short document; here it costs an unseen orphan and a second live price for one figure. Co-Authored-By: Claude Opus 5 --- app/Services/Billing/AdoptStripePrice.php | 4 +- app/Services/Stripe/FakeStripeClient.php | 22 +++++++--- app/Services/Stripe/HttpStripeClient.php | 12 +++++- .../Billing/StripeIdempotencyKeyTest.php | 42 +++++++++++++++++++ .../Billing/StripePriceAdoptionTest.php | 13 ++++++ 5 files changed, 86 insertions(+), 7 deletions(-) diff --git a/app/Services/Billing/AdoptStripePrice.php b/app/Services/Billing/AdoptStripePrice.php index 1e7d4b7..3c1e8ce 100644 --- a/app/Services/Billing/AdoptStripePrice.php +++ b/app/Services/Billing/AdoptStripePrice.php @@ -127,7 +127,9 @@ final class AdoptStripePrice return null; } - usort($candidates, fn (array $a, array $b) => $a['created'] <=> $b['created']); + // Oldest first, and on a tie the lower id — so the same two orphans give + // the same answer on every run, whatever order Stripe lists them in. + usort($candidates, fn (array $a, array $b) => [$a['created'], $a['id']] <=> [$b['created'], $b['id']]); $adopted = array_shift($candidates); diff --git a/app/Services/Stripe/FakeStripeClient.php b/app/Services/Stripe/FakeStripeClient.php index 262f8d7..1db81fa 100644 --- a/app/Services/Stripe/FakeStripeClient.php +++ b/app/Services/Stripe/FakeStripeClient.php @@ -231,9 +231,11 @@ class FakeStripeClient implements StripeClient // rather than clocked, because two Prices minted in the same second // would share a timestamp — but the two counters are independent: // plantPrice() stamps `created: 1` of its own accord, which ties with - // a Price minted here while this list was still empty. Any test whose - // outcome depends on which of two prices is older passes `created:` - // itself, as the adoption tests do. + // a Price minted here while this list was still empty. A tie is + // possible, not merely theoretical — which is why AdoptStripePrice's + // usort breaks one by price id rather than leaving it to Stripe's + // list order, and a test can plant two prices at the same `created` + // on purpose to prove it. 'created' => count($this->prices) + 1, ]; @@ -282,7 +284,10 @@ class FakeStripeClient implements StripeClient 'transform_quantity' => $price['transform_quantity'], 'billing_scheme' => $price['billing_scheme'], 'created' => $price['created'], - 'metadata' => $price['metadata'], + // Cast like the wire does: Stripe hands metadata back as + // strings, and an un-cast int here would let a test pass where + // production's strict === fails. + 'metadata' => array_map(fn ($value) => (string) $value, $price['metadata']), ]; } @@ -292,7 +297,14 @@ class FakeStripeClient implements StripeClient public function updatePriceMetadata(string $priceId, array $metadata): void { if (isset($this->prices[$priceId])) { - $this->prices[$priceId]['metadata'] = $metadata; + // Merged, not replaced — Stripe leaves a key you do not send where + // it is. A fake that replaced would let a test prove the opposite of + // production, and "write only when it differs" in AdoptStripePrice + // is built around exactly this merging. + $this->prices[$priceId]['metadata'] = [ + ...$this->prices[$priceId]['metadata'], + ...$metadata, + ]; } $this->metadataUpdates[] = ['price' => $priceId, 'metadata' => $metadata]; diff --git a/app/Services/Stripe/HttpStripeClient.php b/app/Services/Stripe/HttpStripeClient.php index b90cbe9..dc08835 100644 --- a/app/Services/Stripe/HttpStripeClient.php +++ b/app/Services/Stripe/HttpStripeClient.php @@ -197,7 +197,17 @@ class HttpStripeClient implements StripeClient } $after = $data === [] ? null : ($data[array_key_last($data)]['id'] ?? null); - } while (($page['has_more'] ?? false) === true && $after !== null); + + // invoiceLines() stops here, and there that costs a short document. + // Here it costs an unseen orphan and a second live Price for one + // figure — the incident. So it is an error, not a stopping point. + if (($page['has_more'] ?? false) === true && $after === null) { + throw new RuntimeException( + 'Stripe reported another page of prices and the last item of this one carried no id; ' + .'refusing to stop half-read.' + ); + } + } while (($page['has_more'] ?? false) === true); return $prices; } diff --git a/tests/Feature/Billing/StripeIdempotencyKeyTest.php b/tests/Feature/Billing/StripeIdempotencyKeyTest.php index 47686e0..1f8a9df 100644 --- a/tests/Feature/Billing/StripeIdempotencyKeyTest.php +++ b/tests/Feature/Billing/StripeIdempotencyKeyTest.php @@ -238,3 +238,45 @@ it('lets the fake answer with the prices it holds, minus the archived ones', fun expect($found)->toContain($kept, 'price_orphan') ->and($found)->not->toContain($gone, $other); }); + +it('merges metadata onto a price the way Stripe does, rather than replacing it', function () { + $fake = new FakeStripeClient; + $fake->plantPrice('price_a', 'prod_1', 3480, 'EUR', 'month', + ['addon' => 'priority_support', 'internal_note' => 'kept']); + + $fake->updatePriceMetadata('price_a', ['tax_treatment' => 'domestic']); + + // Stripe merges: a key you do not send stays. A fake that replaced would let + // a test prove the opposite of production — and it is exactly this merging + // that AdoptStripePrice's "write only when it differs" comparison is built + // around. + expect($fake->activePricesFor('prod_1')[0]['metadata'])->toBe([ + 'addon' => 'priority_support', + 'internal_note' => 'kept', + 'tax_treatment' => 'domestic', + ]); +}); + +it('hands metadata back as strings, the way the wire does', function () { + $fake = new FakeStripeClient; + $fake->plantPrice('price_a', 'prod_1', 3480, 'EUR', 'month', ['plan_price_id' => 42]); + + // HttpStripeClient casts; the fake did not. An un-cast int is what makes + // confirms()'s strict === fail forever for that price. + expect($fake->activePricesFor('prod_1')[0]['metadata']['plan_price_id'])->toBe('42'); +}); + +it('refuses to stop half-read when Stripe says there is another page', function () { + Http::fake([ + 'api.stripe.com/*' => Http::response([ + // has_more, and the last item carries no id to page after. Stopping + // here leaves an orphan unseen — which is a second live Price for one + // figure, the incident this whole feature exists to end. + 'data' => [['unit_amount' => 3480, 'currency' => 'eur', 'recurring' => ['interval' => 'month']]], + 'has_more' => true, + ]), + ]); + + expect(fn () => (new HttpStripeClient)->activePricesFor('prod_1')) + ->toThrow(RuntimeException::class, 'half-read'); +}); diff --git a/tests/Feature/Billing/StripePriceAdoptionTest.php b/tests/Feature/Billing/StripePriceAdoptionTest.php index ca10ff2..6a6f75f 100644 --- a/tests/Feature/Billing/StripePriceAdoptionTest.php +++ b/tests/Feature/Billing/StripePriceAdoptionTest.php @@ -668,3 +668,16 @@ it('warns about an unexplained price once a day, not once a booking', function ( Log::shouldHaveReceived('warning')->once(); }); + +it('breaks a tie on created by the price id, so two runs agree', function () { + // plantPrice defaults created to 1 and createPrice stamps count+1, so a + // planted orphan can tie with a minted price. A tie decided by Stripe's list + // order would adopt one price today and the other tomorrow. + $this->stripe->plantPrice('price_bbb', 'prod_support', 3480, 'EUR', 'month', + moduleMetadata(), created: 7); + $this->stripe->plantPrice('price_aaa', 'prod_support', 3480, 'EUR', 'month', + moduleMetadata(), created: 7); + + expect(adoptModulePrice())->toBe('price_aaa') + ->and($this->stripe->archived)->toBe(['price_bbb']); +}); From fcea7ff3a8352bd966aec6b881c53044e478c863 Mon Sep 17 00:00:00 2001 From: nexxo Date: Thu, 30 Jul 2026 17:25:38 +0200 Subject: [PATCH 05/14] Let the catalogue ask Stripe what products it already has The price half of this question was answered; the product half was left open and named as a known gap. It is the more consequential one: a second product makes every activePricesFor() ask about the wrong one, so price recognition goes blind for that whole family and every orphan on the first product is unreachable. The whole active list comes back with no metadata filter, because Stripe cannot search metadata and an account holds a handful of products. Deciding which are ours belongs to the next commit. Co-Authored-By: Claude Opus 5 --- app/Services/Stripe/FakeStripeClient.php | 36 +++++++++++++- app/Services/Stripe/HttpStripeClient.php | 45 ++++++++++++++++++ app/Services/Stripe/StripeClient.php | 17 +++++++ .../Billing/StripeIdempotencyKeyTest.php | 47 +++++++++++++++++++ 4 files changed, 143 insertions(+), 2 deletions(-) diff --git a/app/Services/Stripe/FakeStripeClient.php b/app/Services/Stripe/FakeStripeClient.php index 1db81fa..8de2c57 100644 --- a/app/Services/Stripe/FakeStripeClient.php +++ b/app/Services/Stripe/FakeStripeClient.php @@ -16,7 +16,7 @@ class FakeStripeClient implements StripeClient { public bool $configured = true; - /** @var array */ + /** @var array */ public array $products = []; /** @var array */ @@ -184,7 +184,14 @@ class FakeStripeClient implements StripeClient } $id = 'prod_'.substr(sha1($name.count($this->products)), 0, 12); - $this->products[$id] = ['name' => $name, 'metadata' => $metadata]; + $this->products[$id] = [ + 'name' => $name, + 'metadata' => $metadata, + // Stripe stamps a creation time and AdoptStripeProduct takes the + // OLDEST of several. A counter cannot drift the way a clock in a + // test can. + 'created' => count($this->products) + 1, + ]; $this->rememberKey($key, $id, $parameters); @@ -294,6 +301,31 @@ class FakeStripeClient implements StripeClient return $found; } + public function activeProducts(): array + { + $found = []; + + foreach ($this->products as $id => $product) { + $found[] = [ + 'id' => $id, + 'name' => $product['name'], + 'created' => $product['created'], + 'metadata' => array_map(fn ($value) => (string) $value, $product['metadata']), + ]; + } + + return $found; + } + + /** + * A Product that exists at Stripe and in no row of ours — the orphan a run + * leaves when it dies between Stripe's create and our storing its id. + */ + public function plantProduct(string $id, string $name, array $metadata = [], int $created = 1): void + { + $this->products[$id] = ['name' => $name, 'metadata' => $metadata, 'created' => $created]; + } + public function updatePriceMetadata(string $priceId, array $metadata): void { if (isset($this->prices[$priceId])) { diff --git a/app/Services/Stripe/HttpStripeClient.php b/app/Services/Stripe/HttpStripeClient.php index dc08835..b562a7a 100644 --- a/app/Services/Stripe/HttpStripeClient.php +++ b/app/Services/Stripe/HttpStripeClient.php @@ -212,6 +212,51 @@ class HttpStripeClient implements StripeClient return $prices; } + public function activeProducts(): array + { + $products = []; + $after = null; + + do { + $page = $this->request() + ->get($this->url('products'), array_filter([ + 'active' => 'true', + 'limit' => 100, + 'starting_after' => $after, + ], fn ($value) => $value !== null)) + ->throw() + ->json(); + + $data = (array) ($page['data'] ?? []); + + foreach ($data as $product) { + $products[] = [ + 'id' => (string) ($product['id'] ?? ''), + 'name' => (string) ($product['name'] ?? ''), + 'created' => (int) ($product['created'] ?? 0), + 'metadata' => array_map( + fn ($value) => (string) $value, + (array) ($product['metadata'] ?? []), + ), + ]; + } + + $after = $data === [] ? null : ($data[array_key_last($data)]['id'] ?? null); + + // Same reason as activePricesFor(): a half-read list here means a + // Product we own goes unrecognised, and the next run mints a second + // one — which blinds the Price recognition for the whole family. + if (($page['has_more'] ?? false) === true && $after === null) { + throw new RuntimeException( + 'Stripe reported another page of products and the last item of this one carried no id; ' + .'refusing to stop half-read.' + ); + } + } while (($page['has_more'] ?? false) === true); + + return $products; + } + public function updatePriceMetadata(string $priceId, array $metadata): void { $this->request() diff --git a/app/Services/Stripe/StripeClient.php b/app/Services/Stripe/StripeClient.php index ab7908d..57195cb 100644 --- a/app/Services/Stripe/StripeClient.php +++ b/app/Services/Stripe/StripeClient.php @@ -152,6 +152,23 @@ interface StripeClient */ public function activePricesFor(string $productId): array; + /** + * Every active Product on the account, with its metadata. + * + * The other half of the orphan question, and the more consequential one. A + * run that created a Product and died before storing its id leaves an orphan + * exactly as a Price does — but a second Product is worse than a duplicate + * Price, because activePricesFor() is then asked about the wrong one and the + * Price recognition goes blind for that whole family. + * + * The WHOLE active list, with no metadata filter: Stripe cannot search + * metadata, and an account holds a handful of Products. Which of them is + * ours is App\Services\Billing\AdoptStripeProduct's decision. + * + * @return array}> + */ + public function activeProducts(): array; + /** * Write metadata onto a Price that already exists. * diff --git a/tests/Feature/Billing/StripeIdempotencyKeyTest.php b/tests/Feature/Billing/StripeIdempotencyKeyTest.php index 1f8a9df..db5d53d 100644 --- a/tests/Feature/Billing/StripeIdempotencyKeyTest.php +++ b/tests/Feature/Billing/StripeIdempotencyKeyTest.php @@ -280,3 +280,50 @@ it('refuses to stop half-read when Stripe says there is another page', function expect(fn () => (new HttpStripeClient)->activePricesFor('prod_1')) ->toThrow(RuntimeException::class, 'half-read'); }); + +it('pages through every active product of the account', function () { + Http::fake([ + 'api.stripe.com/*' => Http::sequence() + ->push([ + 'data' => [ + ['id' => 'prod_a', 'name' => 'Team', 'created' => 100, + 'metadata' => ['plan_family' => 'team', 'plan_family_id' => '3']], + ['id' => 'prod_b', 'name' => 'Priority Support', 'created' => 101, + 'metadata' => ['addon' => 'priority_support']], + ], + 'has_more' => true, + ]) + ->push([ + 'data' => [['id' => 'prod_c', 'name' => 'Etwas anderes', 'created' => 102, 'metadata' => []]], + 'has_more' => false, + ]), + ]); + + $products = (new HttpStripeClient)->activeProducts(); + + expect($products)->toHaveCount(3) + ->and($products[0])->toBe([ + 'id' => 'prod_a', + 'name' => 'Team', + 'created' => 100, + 'metadata' => ['plan_family' => 'team', 'plan_family_id' => '3'], + ]) + ->and($products[2]['id'])->toBe('prod_c'); + + // No metadata filter is sent: Stripe cannot search by it, so the whole + // active list comes back and the matching happens here. An account holds a + // handful of products, not thousands. + Http::assertSent(fn ($request) => str_contains($request->url(), 'active=true') + && ! str_contains($request->url(), 'metadata')); + Http::assertSent(fn ($request) => str_contains($request->url(), 'starting_after=prod_b')); +}); + +it('lets the fake answer with the products it holds', function () { + $fake = new FakeStripeClient; + + $minted = $fake->createProduct('Team', ['plan_family_id' => '3']); + $fake->plantProduct('prod_orphan', 'Team', ['plan_family_id' => '3'], created: 0); + + expect(collect($fake->activeProducts())->pluck('id')->all()) + ->toContain($minted, 'prod_orphan'); +}); From d6ec09a9b40e98131b72246bd2c8499d86bff53d Mon Sep 17 00:00:00 2001 From: nexxo Date: Thu, 30 Jul 2026 17:39:15 +0200 Subject: [PATCH 06/14] Recognise the product Stripe already has MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gap beside the guard. A run that creates a product and dies before storing its id leaves an orphan exactly as a price does, and past the key's twenty-four hours the next run makes a second one — after which activePricesFor() is asked about the wrong product, price recognition goes blind for the whole family, and every orphaned price on the first product is unreachable. Two things are unlike the price side, deliberately. No money gate: a product carries no amount, so its metadata is the whole of the proof. And a duplicate is reported, never deactivated — archiving a price is provably harmless because Stripe keeps billing subscriptions already on it, but deactivating a product makes its prices unsellable and contracts can be running on those. Silent about strangers, too. This asks for every active product on the account, so an operator selling something else through it is an ordinary state, not a finding worth a line on every run. Co-Authored-By: Claude Opus 5 --- app/Console/Commands/SyncStripeCatalogue.php | 38 ++--- app/Providers/AppServiceProvider.php | 8 + app/Services/Billing/AddonPrices.php | 19 ++- app/Services/Billing/AdoptStripeProduct.php | 137 ++++++++++++++++++ .../Billing/StripeProductAdoptionTest.php | 113 +++++++++++++++ 5 files changed, 286 insertions(+), 29 deletions(-) create mode 100644 app/Services/Billing/AdoptStripeProduct.php create mode 100644 tests/Feature/Billing/StripeProductAdoptionTest.php diff --git a/app/Console/Commands/SyncStripeCatalogue.php b/app/Console/Commands/SyncStripeCatalogue.php index b34071c..7f32e41 100644 --- a/app/Console/Commands/SyncStripeCatalogue.php +++ b/app/Console/Commands/SyncStripeCatalogue.php @@ -8,6 +8,7 @@ use App\Models\PlanVersion; use App\Models\Subscription; use App\Services\Billing\AddonCatalogue; use App\Services\Billing\AddonPrices; +use App\Services\Billing\AdoptStripeProduct; use App\Services\Billing\PlanPrices; use App\Services\Billing\TaxTreatment; use App\Services\Stripe\StripeClient; @@ -101,32 +102,25 @@ class SyncStripeCatalogue extends Command $created++; if (! $dryRun) { - $productId = $stripe->createProduct( + $metadata = ['plan_family' => $family->key, 'plan_family_id' => (string) $family->id]; + + // Asked BEFORE minting, for the same reason the Price side + // asks: a run that died between Stripe's create and our + // update left a Product this row does not know, and the key + // below stops protecting it after twenty-four hours. + // plan_family_id is what proves such a Product is this + // family's. + $productId = app(AdoptStripeProduct::class)($metadata, ['plan_family_id']); + + $productId ??= $stripe->createProduct( $family->name, - ['plan_family' => $family->key, 'plan_family_id' => (string) $family->id], + $metadata, // Covers a RETRY, for twenty-four hours, and nothing - // after that: Stripe forgets a key at the end of them. - // A crash between Stripe creating the Product and us - // storing its id therefore leaves an orphan, and the - // next run past the expiry makes a second Product for - // this family. There is no recognition step for Products - // — App\Services\Billing\AdoptStripePrice covers Prices - // only — so nothing here ever asks Stripe what Products - // it already has. A KNOWN GAP, not something this key - // closes, and a worse one than a duplicate Price: - // activePricesFor() would then be asked about the new - // Product, and the Price recognition goes blind for the - // whole family. - // - // IdempotencyKey::forProduct() folds the name and the - // metadata into what goes on the wire, so a renamed - // family under an unstored id now mints a second Product - // where it used to answer HTTP 400 for a day. That is - // the trade this branch chose deliberately: a blockade - // reaches a paying customer, a duplicate Product does - // not. + // beyond them. What stops a second Product for one + // family is the adoption step above. idempotencyKey: "clupilot-product-{$family->id}", ); + $family->update(['stripe_product_id' => $productId]); } } diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index 27c012d..1c5d75f 100644 --- a/app/Providers/AppServiceProvider.php +++ b/app/Providers/AppServiceProvider.php @@ -13,6 +13,7 @@ use App\Mail\Transport\MailboxTransport; use App\Models\Customer; use App\Models\MaintenanceNotification; use App\Provisioning\PipelineRegistry; +use App\Services\Billing\AdoptStripeProduct; use App\Services\Billing\CustomDomainAccess; use App\Services\Dns\FileHostDnsDirectory; use App\Services\Dns\HetznerDnsClient; @@ -57,6 +58,13 @@ class AppServiceProvider extends ServiceProvider config('provisioning.pipelines', []), )); + // Singletons because both carry a record across one run that the caller + // reads afterwards: which products were duplicates, and how many prices + // were adopted rather than created. Resolved per call they would each + // start empty — SyncStripeCatalogue asks the container for PlanPrices + // once per catalogue row. + $this->app->singleton(AdoptStripeProduct::class); + // Real I/O implementations; tests swap in fakes via app()->instance(). $this->app->bind(RemoteShell::class, PhpseclibRemoteShell::class); $this->app->bind(WireguardHub::class, LocalWireguardHub::class); diff --git a/app/Services/Billing/AddonPrices.php b/app/Services/Billing/AddonPrices.php index ecb49bd..7f01d10 100644 --- a/app/Services/Billing/AddonPrices.php +++ b/app/Services/Billing/AddonPrices.php @@ -284,14 +284,19 @@ final class AddonPrices return $existing; } - return $this->stripe->createProduct( + $metadata = ['addon' => $addonKey]; + + // Asked before minting. A run that created this Product and died before + // any row carried its id leaves an orphan — and a second Product would + // make activePricesFor() ask about the wrong one, blinding the Price + // recognition for this module entirely. + $adopted = app(AdoptStripeProduct::class)($metadata, ['addon']); + + return $adopted ?? $this->stripe->createProduct( app(AddonCatalogue::class)->name($addonKey), - ['addon' => $addonKey], - // A retry's guard for twenty-four hours and nothing beyond them, the - // same as the plan side's in SyncStripeCatalogue — and the same known - // gap: there is no recognition step for Products, so a run that - // created this one and died before any row carried its id leaves an - // orphan nothing here will ever ask Stripe about. + $metadata, + // A retry's guard for twenty-four hours and nothing beyond them; + // what stops a second Product is the adoption step above. idempotencyKey: "clupilot-addon-product-{$addonKey}", ); } diff --git a/app/Services/Billing/AdoptStripeProduct.php b/app/Services/Billing/AdoptStripeProduct.php new file mode 100644 index 0000000..4f5f46a --- /dev/null +++ b/app/Services/Billing/AdoptStripeProduct.php @@ -0,0 +1,137 @@ + + */ + public array $duplicates = []; + + public function __construct(private readonly StripeClient $stripe) {} + + /** + * The id of an existing Stripe Product to use instead of creating one. + * + * @param array $metadata what the create call would send + * @param array $identifying metadata keys that mark a Product as ours + */ + public function __invoke(array $metadata, array $identifying): ?string + { + $metadata = array_map(fn ($value) => (string) $value, $metadata); + + $candidates = []; + + foreach ($this->stripe->activeProducts() as $product) { + if ($this->contradicts($product['metadata'], $metadata)) { + continue; + } + + if (! $this->confirms($product['metadata'], $metadata, $identifying)) { + continue; + } + + $candidates[] = $product; + } + + if ($candidates === []) { + return null; + } + + // Oldest first, and on a tie the lower id, so two runs agree. The tie is + // not merely theoretical: createProduct()'s counter and plantProduct()'s + // default both start at 1, so a minted Product and a planted orphan can + // share a `created` — the same collision AdoptStripePrice's usort breaks + // the same way, documented at FakeStripeClient::createPrice(). + usort($candidates, fn (array $a, array $b) => [$a['created'], $a['id']] <=> [$b['created'], $b['id']]); + + $adopted = array_shift($candidates); + + foreach ($candidates as $duplicate) { + $this->duplicates[] = $duplicate['id']; + + Log::warning('stripe: a second product claims to be the same thing — left alone, not deactivated', [ + 'product' => $duplicate['id'], + 'adopted' => $adopted['id'], + 'metadata' => $metadata, + ]); + } + + Log::info('stripe: adopted an existing product instead of creating a second one', [ + 'product' => $adopted['id'], + 'metadata' => $metadata, + ]); + + return $adopted['id']; + } + + /** + * @param array $found + * @param array $expected + */ + private function contradicts(array $found, array $expected): bool + { + foreach ($expected as $key => $value) { + if (array_key_exists($key, $found) && $found[$key] !== $value) { + return true; + } + } + + return false; + } + + /** + * Failing to contradict is not enough — an empty metadata bag contradicts + * nothing, and this list holds every Product on the account. + * + * @param array $found + * @param array $expected + * @param array $identifying + */ + private function confirms(array $found, array $expected, array $identifying): bool + { + foreach ($identifying as $key) { + if (isset($found[$key]) && $found[$key] === ($expected[$key] ?? null)) { + return true; + } + } + + return false; + } +} diff --git a/tests/Feature/Billing/StripeProductAdoptionTest.php b/tests/Feature/Billing/StripeProductAdoptionTest.php new file mode 100644 index 0000000..3379629 --- /dev/null +++ b/tests/Feature/Billing/StripeProductAdoptionTest.php @@ -0,0 +1,113 @@ +stripe = new FakeStripeClient; + app()->instance(StripeClient::class, $this->stripe); +}); + +it('adopts an orphaned family product instead of minting a second one', function () { + // A run that got as far as creating the Product and died before storing its + // id: Stripe has it, plan_families does not. + $family = PlanFamily::query()->orderBy('tier')->firstOrFail(); + $family->update(['stripe_product_id' => null]); + + $this->stripe->plantProduct('prod_orphan_family', $family->name, [ + 'plan_family' => $family->key, + 'plan_family_id' => (string) $family->id, + ]); + + app(Kernel::class)->call('stripe:sync-catalogue'); + + // Counting every product would prove nothing — the same run mints one for + // each of the OTHER families. What must hold is that this family's id is + // carried by exactly one product, and that it is the planted one. + expect($family->refresh()->stripe_product_id)->toBe('prod_orphan_family') + ->and(collect($this->stripe->products) + ->filter(fn (array $p) => ($p['metadata']['plan_family_id'] ?? null) === (string) $family->id) + ->keys()->all())->toBe(['prod_orphan_family']); +}); + +it('adopts an orphaned module product instead of minting a second one', function () { + $this->stripe->plantProduct('prod_orphan_module', 'Priority Support', [ + 'addon' => 'priority_support', + ]); + + $before = count($this->stripe->products); + + app(AddonPrices::class)->ensure( + 'priority_support', 2900, 'EUR', Subscription::TERM_MONTHLY, TaxTreatment::domestic(), + ); + + expect(StripeAddonPrice::query() + ->where('addon_key', 'priority_support') + ->value('stripe_product_id'))->toBe('prod_orphan_module') + ->and(count($this->stripe->products))->toBe($before); +}); + +it('adopts nothing a product does not prove about itself', function () { + // What an operator selling something else through the same Stripe account + // leaves lying around. Silent, unlike the Price side: we list every product + // on the account, so warning about each foreign one would fire on every run + // for as long as it exists. + Log::spy(); + + $this->stripe->plantProduct('prod_stranger', 'Etwas ganz anderes', []); + + expect(app(AdoptStripeProduct::class)( + ['addon' => 'priority_support'], ['addon'], + ))->toBeNull(); + + Log::shouldNotHaveReceived('warning'); +}); + +it('adopts nothing that contradicts what we would have written', function () { + $this->stripe->plantProduct('prod_other_module', 'Collabora Pro', ['addon' => 'collabora_pro']); + + expect(app(AdoptStripeProduct::class)( + ['addon' => 'priority_support'], ['addon'], + ))->toBeNull(); +}); + +it('takes the oldest of two matching products and leaves the other alone', function () { + Log::spy(); + + $this->stripe->plantProduct('prod_second', 'Priority Support', ['addon' => 'priority_support'], created: 200); + $this->stripe->plantProduct('prod_first', 'Priority Support', ['addon' => 'priority_support'], created: 100); + + $adopt = app(AdoptStripeProduct::class); + + expect($adopt(['addon' => 'priority_support'], ['addon']))->toBe('prod_first') + // NOT deactivated. Its prices would become unsellable, and a contract + // can be running on one of them — the asymmetry with archivePrice(). + ->and($adopt->duplicates)->toBe(['prod_second']); + + Log::shouldHaveReceived('warning')->once(); +}); From df7da334eeaf8637385d14dd1e7b1bdbc15e9053 Mon Sep 17 00:00:00 2001 From: nexxo Date: Thu, 30 Jul 2026 17:56:13 +0200 Subject: [PATCH 07/14] Say what was adopted, not that it was created MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The count is taken before ensure() runs, so the sweep reported objects created in Stripe when it had made none — and after the next interrupted run, that line is the first thing a human reads. Telling the two apart is the whole point of the recognition step. Comparing the price id before and after the call does not distinguish them either: there is no id before, in either case. AdoptStripePrice counts its own adoptions instead, which is why it and its product sibling are now singletons — resolved per call, a counter on the instance would never pass one. Duplicate products are named in the report as well as the log. We deliberately do not deactivate them, so only a person can resolve one, and a person reads this. Co-Authored-By: Claude Opus 5 --- app/Console/Commands/SyncStripeCatalogue.php | 40 +++++++++++++----- app/Providers/AppServiceProvider.php | 14 ++++--- app/Services/Billing/AdoptStripePrice.php | 12 ++++++ app/Services/Billing/AdoptStripeProduct.php | 12 ++++++ .../Billing/StripeProductAdoptionTest.php | 41 +++++++++++++++++++ 5 files changed, 104 insertions(+), 15 deletions(-) diff --git a/app/Console/Commands/SyncStripeCatalogue.php b/app/Console/Commands/SyncStripeCatalogue.php index 7f32e41..c18f3b3 100644 --- a/app/Console/Commands/SyncStripeCatalogue.php +++ b/app/Console/Commands/SyncStripeCatalogue.php @@ -8,6 +8,7 @@ use App\Models\PlanVersion; use App\Models\Subscription; use App\Services\Billing\AddonCatalogue; use App\Services\Billing\AddonPrices; +use App\Services\Billing\AdoptStripePrice; use App\Services\Billing\AdoptStripeProduct; use App\Services\Billing\PlanPrices; use App\Services\Billing\TaxTreatment; @@ -84,6 +85,14 @@ class SyncStripeCatalogue extends Command return self::FAILURE; } + // Resolved once, as singletons: each counts across the WHOLE run, and a + // count taken before ensure()/the adoption call runs can never itself + // distinguish "created" from "adopted" — see AdoptStripePrice. + $adoptPrices = app(AdoptStripePrice::class); + $adoptProducts = app(AdoptStripeProduct::class); + $adoptedPricesBefore = $adoptPrices->adoptions; + $adoptedProductsBefore = $adoptProducts->adoptions; + $created = 0; foreach (PlanFamily::query()->with('versions.prices')->orderBy('tier')->get() as $family) { @@ -110,7 +119,7 @@ class SyncStripeCatalogue extends Command // below stops protecting it after twenty-four hours. // plan_family_id is what proves such a Product is this // family's. - $productId = app(AdoptStripeProduct::class)($metadata, ['plan_family_id']); + $productId = $adoptProducts($metadata, ['plan_family_id']); $productId ??= $stripe->createProduct( $family->name, @@ -136,22 +145,33 @@ class SyncStripeCatalogue extends Command $this->newLine(); + // Both counted the same way and for the same reason: $created is an + // INTENT taken before we know whether ensure()/the adoption call will + // mint or adopt, for a price as well as for a family's product — see + // AdoptStripePrice. Combined into one "adopted" figure because an + // operator reading the summary line cares whether Stripe made anything + // NEW, not which kind of object it would have been. + $adoptedPrices = $adoptPrices->adoptions - $adoptedPricesBefore; + $adoptedProducts = $adoptProducts->adoptions - $adoptedProductsBefore; + $adopted = $adoptedPrices + $adoptedProducts; + $minted = max(0, $created - $adopted); + + foreach ($adoptProducts->duplicates as $duplicate) { + // Named here as well as in the log: an operator running the sweep + // reads this, and a second Product for one family is something only + // a person can resolve — we deliberately do not deactivate it. + $this->warn(" duplicate product {$duplicate} — left active, a product's prices become unsellable if it is deactivated"); + } + if ($created === 0) { $this->info('Stripe is already in step with the catalogue.'); return self::SUCCESS; } - // "or adopted", because the count is taken BEFORE ensure() runs and - // ensure() may find the Price already at Stripe and adopt it instead of - // creating anything — see App\Services\Billing\AdoptStripePrice. Telling - // which of the two happened is the whole purpose of that step, so the - // first thing an operator reads after an interrupted run must not assert - // a creation that did not take place. The log entry adoption writes names - // the Price it took over. $this->info($dryRun - ? "{$created} object(s) would be created or adopted. Run without --dry-run to do it." - : "{$created} object(s) created or adopted in Stripe."); + ? "{$created} object(s) would be created. Run without --dry-run to create them." + : "{$minted} object(s) created, {$adopted} adopted in Stripe."); return self::SUCCESS; } diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index 1c5d75f..865a1df 100644 --- a/app/Providers/AppServiceProvider.php +++ b/app/Providers/AppServiceProvider.php @@ -13,6 +13,7 @@ use App\Mail\Transport\MailboxTransport; use App\Models\Customer; use App\Models\MaintenanceNotification; use App\Provisioning\PipelineRegistry; +use App\Services\Billing\AdoptStripePrice; use App\Services\Billing\AdoptStripeProduct; use App\Services\Billing\CustomDomainAccess; use App\Services\Dns\FileHostDnsDirectory; @@ -58,12 +59,15 @@ class AppServiceProvider extends ServiceProvider config('provisioning.pipelines', []), )); - // Singletons because both carry a record across one run that the caller - // reads afterwards: which products were duplicates, and how many prices - // were adopted rather than created. Resolved per call they would each - // start empty — SyncStripeCatalogue asks the container for PlanPrices - // once per catalogue row. + // Both singletons because each carries a record across one run that the + // caller reads only after it finishes: which products were duplicates, + // and how many products and prices were adopted rather than created. + // Resolved per call they would each start empty on every read. PlanPrices + // is deliberately NOT a singleton — SyncStripeCatalogue asks the container + // for it once per catalogue row — which is exactly why the counter has to + // live here instead: on a per-call instance it could never pass one. $this->app->singleton(AdoptStripeProduct::class); + $this->app->singleton(AdoptStripePrice::class); // Real I/O implementations; tests swap in fakes via app()->instance(). $this->app->bind(RemoteShell::class, PhpseclibRemoteShell::class); diff --git a/app/Services/Billing/AdoptStripePrice.php b/app/Services/Billing/AdoptStripePrice.php index 3c1e8ce..2b547a3 100644 --- a/app/Services/Billing/AdoptStripePrice.php +++ b/app/Services/Billing/AdoptStripePrice.php @@ -42,6 +42,16 @@ use Illuminate\Support\Facades\Log; */ final class AdoptStripePrice { + /** + * How many Prices this run took over rather than minted. + * + * Read by stripe:sync-catalogue, which counts an intent BEFORE calling + * ensure() and so cannot tell the two apart on its own — comparing the Price + * id before and after does not distinguish them either, because there is no + * id before in either case. + */ + public int $adoptions = 0; + public function __construct(private readonly StripeClient $stripe) {} /** @@ -170,6 +180,8 @@ final class AdoptStripePrice 'interval' => $interval, ]); + $this->adoptions++; + return $adopted['id']; } diff --git a/app/Services/Billing/AdoptStripeProduct.php b/app/Services/Billing/AdoptStripeProduct.php index 4f5f46a..3145fd5 100644 --- a/app/Services/Billing/AdoptStripeProduct.php +++ b/app/Services/Billing/AdoptStripeProduct.php @@ -44,6 +44,16 @@ final class AdoptStripeProduct */ public array $duplicates = []; + /** + * How many Products this run took over rather than minted. + * + * Mirrors AdoptStripePrice::$adoptions, and for the same reason: + * stripe:sync-catalogue counts an intent to create a Product BEFORE it + * knows whether this class will hand back an existing one, so a Product + * that was adopted must not silently count as one that was created. + */ + public int $adoptions = 0; + public function __construct(private readonly StripeClient $stripe) {} /** @@ -98,6 +108,8 @@ final class AdoptStripeProduct 'metadata' => $metadata, ]); + $this->adoptions++; + return $adopted['id']; } diff --git a/tests/Feature/Billing/StripeProductAdoptionTest.php b/tests/Feature/Billing/StripeProductAdoptionTest.php index 3379629..2820bc0 100644 --- a/tests/Feature/Billing/StripeProductAdoptionTest.php +++ b/tests/Feature/Billing/StripeProductAdoptionTest.php @@ -1,10 +1,13 @@ once(); }); + +it('says it adopted, not that it created', function () { + // The one signal a human reads after an incident. The count is taken before + // ensure() runs, so without this the run reports objects created in Stripe + // when it made none — and telling those two apart is the whole point of the + // recognition step. + app(Kernel::class)->call('stripe:sync-catalogue'); + + $row = PlanPrice::query()->firstOrFail(); + $charged = PlanPrices::chargedCents($row, TaxTreatment::domestic()); + $orphan = (string) StripePlanPrice::query() + ->where('plan_price_id', $row->id)->where('reverse_charge', false) + ->value('stripe_price_id'); + StripePlanPrice::query()->where('stripe_price_id', $orphan)->delete(); + $this->stripe->keys = []; + + $this->artisan('stripe:sync-catalogue') + ->expectsOutputToContain('1 adopted') + ->assertSuccessful(); +}); + +it('names a duplicate product in its report, without touching it', function () { + // Nothing has been synced yet, same as the orphan test above — the family + // has no product of its own, and two stray ones claiming to be it are the + // whole of what Stripe has for it. + $family = PlanFamily::query()->orderBy('tier')->firstOrFail(); + $family->update(['stripe_product_id' => null]); + + $metadata = ['plan_family' => $family->key, 'plan_family_id' => (string) $family->id]; + $this->stripe->plantProduct('prod_first', $family->name, $metadata, created: 100); + $this->stripe->plantProduct('prod_second', $family->name, $metadata, created: 200); + + $this->artisan('stripe:sync-catalogue') + ->expectsOutputToContain('prod_second') + ->assertSuccessful(); + + expect($family->refresh()->stripe_product_id)->toBe('prod_first'); +}); From 5883989552e6ef2db6c1503069a909ee8ca05ab4 Mon Sep 17 00:00:00 2001 From: nexxo Date: Thu, 30 Jul 2026 17:59:16 +0200 Subject: [PATCH 08/14] Give settings the shape a settings page actually has MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four attempts, and this is what was wrong with the last one: full-width panels stacked down the page, each carrying its own title INSIDE it. Every group had two frames — the title bar and the border — so the page read as a column of long boxes with nothing saying where one topic ended and the next began, and a phone number got a field a thousand pixels wide. The shape now: - **A section list on the left**, sticky, so the sections stay reachable however far a form runs. Below `lg` it becomes a scrolling strip of chips, because a vertical list on a telephone is four rows of nothing. Under it, who is signed in — on a shared office machine that is a real question, and a settings page is where it gets asked. - **A content column of 820px**, not the whole 1240 shell. - **The heading OUTSIDE the card it belongs to.** A section is a heading, a sentence, and then a card of rows. That single move is what stops them reading as boxes: the border now frames the fields, not the topic. - **What cannot be undone is last, in a card whose border says so** — the deletion deadlines and the close button together, at the foot of the contract section, in danger colours. The rows are unchanged (label left, control right) and so is every binding; what changed is the frame around them. Co-Authored-By: Claude Opus 5 --- lang/de/dpa.php | 2 + lang/de/settings.php | 1 + lang/en/dpa.php | 2 + lang/en/settings.php | 1 + resources/views/livewire/settings.blade.php | 913 +++++++++++--------- 5 files changed, 507 insertions(+), 412 deletions(-) diff --git a/lang/de/dpa.php b/lang/de/dpa.php index 6f79b85..6d69a3d 100644 --- a/lang/de/dpa.php +++ b/lang/de/dpa.php @@ -15,4 +15,6 @@ return [ 'in_force_note' => 'Dieser Vertrag ist mit Ihrer Bestellung Bestandteil unserer Vereinbarung; eine gesonderte Unterschrift ist nicht nötig. Wenn Sie für Ihre Unterlagen eine ausdrückliche Bestätigung möchten, halten wir Fassung, Zeitpunkt und IP-Adresse fest.', 'download_agreement' => 'AV-Vertrag laden', 'download_measures' => 'TOM laden', + 'open' => 'Öffnen', + 'state_label' => 'Status', ]; diff --git a/lang/de/settings.php b/lang/de/settings.php index 098d932..c1136a4 100644 --- a/lang/de/settings.php +++ b/lang/de/settings.php @@ -11,6 +11,7 @@ return [ 'branding' => 'Erscheinungsbild', 'contract' => 'Vertrag', ], + 'signed_in_as' => 'Angemeldet als', 'title' => 'Einstellungen', 'subtitle' => 'Alles, was zu Ihrem Konto gehört — nach Themen getrennt.', 'save' => 'Speichern', diff --git a/lang/en/dpa.php b/lang/en/dpa.php index 4c0b3a5..1a95a8f 100644 --- a/lang/en/dpa.php +++ b/lang/en/dpa.php @@ -15,4 +15,6 @@ return [ 'in_force_note' => 'This agreement became part of our contract with your order; a separate signature is not required. If you would like an explicit confirmation for your records, we note the version, the moment and the IP address.', 'download_agreement' => 'Download agreement', 'download_measures' => 'Download measures', + 'open' => 'Open', + 'state_label' => 'State', ]; diff --git a/lang/en/settings.php b/lang/en/settings.php index e72e94f..25f2292 100644 --- a/lang/en/settings.php +++ b/lang/en/settings.php @@ -11,6 +11,7 @@ return [ 'branding' => 'Appearance', 'contract' => 'Contract', ], + 'signed_in_as' => 'Signed in as', 'title' => 'Settings', 'subtitle' => 'Everything that belongs to your account, grouped by subject.', 'save' => 'Save', diff --git a/resources/views/livewire/settings.blade.php b/resources/views/livewire/settings.blade.php index 2298e20..32fcf95 100644 --- a/resources/views/livewire/settings.blade.php +++ b/resources/views/livewire/settings.blade.php @@ -1,27 +1,46 @@ {{-- The customer's own settings. - ## What was wrong, three times over + ## Four attempts, and what was wrong with each - First it was one column 768px wide in a 1240px shell, holding everything a - customer might ever change, two thousand pixels deep. Then it became four - tabs — better — but the cards inside them were still boxes of different - heights in a grid, which on the contract tab produced a short one, a wide one - and a short one again: a staircase with no rhythm and half the width empty - beside it. + 1. One column 768px wide in a 1240px shell, everything in it, two thousand + pixels deep. + 2. Four tabs — better — but cards of different heights in a grid, which on + the contract tab read as a staircase. + 3. Panels and rows: right idea, wrong proportions. Full-width slabs stacked + down the page, each with its title INSIDE it, so the page was a column of + long boxes and nothing said where one topic ended. ## What it is now - Panels and rows (x-ui.panel, x-ui.row). A group is a card with a header that - names it; a setting is a ROW inside that card — what it is on the left, the - control on the right, dividers between. That is the shape every settings page - worth copying uses, and it is the shape that uses the width: a label column - of 280px with the control beside it fills the line, where a stack of - full-width inputs leaves two thirds of it empty. + The shape serious settings pages actually use: a section list on the left, + a measured content column on the right, and the heading OUTSIDE the card it + belongs to. - Below `sm` the rows stack, because on a telephone a label belongs above its - field. + - The nav is the structure. It is sticky, so the sections stay reachable + however far down a form goes, and it says which one you are in without + making you read a row of tabs. + - 820px is the content column. Nobody types a phone number into a field + 1100px wide; a settings page that lets them looks like a spreadsheet. + - A section is a heading, a sentence, and then a card of rows. Putting the + heading inside the card gave every group two frames — the title bar and + the border — and that is what made them read as boxes rather than as + topics. + - What cannot be undone lives at the end, in a card that says so with its + border. + + Below `lg` the nav becomes a scrolling strip of chips above the content, and + the rows stack, because on a telephone a label belongs above its field. --}} +@php + $sections = [ + 'profile' => 'users', + 'security' => 'shield-check', + 'branding' => 'pen', + 'contract' => 'receipt', + ]; +@endphp +
@@ -32,425 +51,495 @@

{{ __('settings.subtitle') }}

- {{-- The same tab bar the console uses, so one vocabulary holds on both sides - of the login. The choice lives in the query string — see the #[Url] - attribute on the component. --}} -
- @foreach ($tabs as $name) - - @endforeach -
+
- {{-- ══ Ihre Daten ═══════════════════════════════════════════════════════ --}} - @if ($tab === 'profile') -
- - - - - - - - - - - - - + {{-- ── The section list ──────────────────────────────────────────── + Sticky on a wide screen: a long form must not scroll its own + navigation away. A strip of chips below `lg`, because a vertical + list on a telephone is four rows of nothing. --}} + - {{-- A FACT for a record that has one. It decides the fourteen-day - right of withdrawal: a business able to set itself to - "Privatperson" here is a business able to withdraw from a - contract it may not withdraw from. Registration asks; this - page reports. The lock is in Settings::saveProfile, not here - — a form that only hides a control has never stopped anybody - who can post to /livewire/update. --}} - - @if ($customer?->customer_type) -

- - {{ __('settings.customer_type_'.$customer->customer_type) }} + {{-- ── The content column ─────────────────────────────────────────── + 820px, not the whole shell. Nobody types a postcode into a field a + thousand pixels wide. --}} +

+ + {{-- ══ Ihre Daten ═══════════════════════════════════════════════ --}} + @if ($tab === 'profile') + +
+

{{ __('settings.company_title') }}

+

{{ __('settings.company_sub') }}

+ + + + + + + + + + + + + + + + {{-- Field by field, not a paragraph: what landed in + the textarea was whatever somebody typed — a + postcode on the street line, a town with no + postcode, no country at all — and an invoice has + to name its recipient precisely enough to be a + document. --}} + +
+ +
+ + +
+ +
+ + + + + {{ __('settings.save') }} + + + +
+ + {{-- A FACT for a record that has one. It decides the + fourteen-day right of withdrawal: a business able to set + itself to "Privatperson" here is a business able to + withdraw from a contract it may not withdraw from. + Registration asks; this page reports. The lock is in + Settings::saveProfile, not here — a form that only hides + a control has never stopped anybody who can post to + /livewire/update. --}} +
+

{{ __('settings.customer_type') }}

+

+ {{ $customer?->customer_type ? __('settings.customer_type_locked') : __('settings.customer_type_once') }}

- @else -
- @foreach ($customerTypes as $type) - - @endforeach -
- @error('customerType')

{{ $message }}

@enderror - @endif - - - - {{ __('settings.save') }} - - - - - @endif + +
+ @if ($customer?->customer_type) +

+ + {{ __('settings.customer_type_'.$customer->customer_type) }} +

+ @else +
+ @foreach ($customerTypes as $type) + + @endforeach +
+ @error('customerType')

{{ $message }}

@enderror + @endif +
+ +
+ + @endif - {{-- ══ Sicherheit ═══════════════════════════════════════════════════════ --}} - @if ($tab === 'security') -
+ {{-- ══ Sicherheit ═══════════════════════════════════════════════ --}} + @if ($tab === 'security') +
- {{-- Own password. There was no way to change one at all — Fortify's - updatePasswords feature was switched off. --}} -
- - - - - -
- - -
- + {{-- Own password. There was no way to change one at all — + Fortify's updatePasswords feature was switched off. --}} +
+

{{ __('admin_settings.password_title') }}

+

{{ __('admin_settings.password_sub') }}

- - - {{ __('admin_settings.password_save') }} - - - - +
+ + + + + +
+ + +
+ - {{-- Two-factor. Every action is re-checked server-side against a - recent password confirmation — hiding the buttons stops nobody - who can post to /livewire/update. --}} - - -
+ + + {{ __('admin_settings.password_save') }} + + + + +
+ + {{-- Two-factor. Every action is re-checked server-side + against a recent password confirmation — hiding the + buttons stops nobody who can post to /livewire/update. --}} +
-

{{ __('settings.twofa_title') }}

+

{{ __('settings.twofa_title') }}

{{ $twoFactorOn ? __('settings.twofa_state_on') : __('settings.twofa_state_off') }}
-

{{ __('settings.twofa_sub') }}

-
- +

{{ __('settings.twofa_sub') }}

- @if (! $passwordConfirmed) - {{-- The gate. A signed-in session is not enough to change how - the account is protected — the risk is an unlocked - machine. --}} -
- -
-
- -
- - {{ __('settings.twofa_confirm_button') }} - -
- -
- @elseif ($twoFactorOn) - -
- - {{ __('settings.twofa_new_codes') }} - - - {{ __('settings.twofa_disable') }} - -
- - @elseif ($twoFactorPending) - -
-
{!! $twoFactorQr !!}
-
-
- -
- - {{ __('settings.twofa_activate') }} - -
-
- - @else - - - {{ __('settings.twofa_enable') }} - - - @endif - - @if ($recoveryCodes) - {{-- Shown once, on purpose: nobody writes down what they were - never shown, and these are the way back in when the phone - is gone. --}} - -
    - @foreach ($recoveryCodes as $code)
  • {{ $code }}
    • @endforeach -
- - @endif - - - {{-- The devices. It answers the same question two-factor does — who - can get into this account — and it is a table of its own. --}} - @livewire('sessions') -
- @endif - - {{-- ══ Erscheinungsbild ═════════════════════════════════════════════════ --}} - @if ($tab === 'branding') -
- - - - - - -
- - -
- @error('brandPrimary')

{{ $message }}

@enderror - - - -
- - -
- @error('brandAccent')

{{ $message }}

@enderror - - - -
-
- @if ($logo) - - @elseif ($logoUrl) - + + @if (! $passwordConfirmed) + {{-- The gate. A signed-in session is not enough + to change how the account is protected — the + risk is an unlocked machine. --}} + + +
+
+ +
+ + {{ __('settings.twofa_confirm_button') }} + +
+ + + @elseif ($twoFactorOn) + +
+ + {{ __('settings.twofa_new_codes') }} + + + {{ __('settings.twofa_disable') }} + +
+ + @elseif ($twoFactorPending) + +
+
{!! $twoFactorQr !!}
+
+
+ +
+ + {{ __('settings.twofa_activate') }} + +
+
+ @else - {{ __('settings.brand_default') }} - @endif -
-
- - @if ($logoUrl) - - @endif - @error('logo')

{{ $message }}

@enderror -
-
- - - - @if ($branding['is_default'] ?? false) -

{{ __('settings.brand_using_default') }}

- @endif - - {{ __('settings.save') }} - - -
- - @endif - - {{-- ══ Vertrag ══════════════════════════════════════════════════════════ --}} - @if ($tab === 'contract') -
- - - - {{-- The package, and the way out of it. --}} - -
-

- @if ($cancellationScheduled) - {{ __('settings.cancel_scheduled_body', ['date' => $instance?->service_ends_at?->local()->isoFormat('LL')]) }} - @elseif ($hasActivePackage) - {{ __('settings.package_active', ['plan' => $instance ? __('billing.plan.'.$instance->plan) : '—']) }} - @else - {{ __('settings.no_package') }} - @endif -

- - @if ($hasActivePackage && ! $cancellationScheduled) - - {{ __('settings.cancel_cta') }} - - @elseif (! $hasActivePackage && ! $cancellationScheduled) - - {{ __('settings.to_packages') }} - - @endif -
- - - {{-- ── Das Widerrufsrecht ──────────────────────────────────── - Shown to every CONSUMER with a contract, open or not — not - only while it is open. It vanished entirely once the - fourteen days ran out, which is indistinguishable from a - feature that was never built: the owner went looking for it - and reported it missing from an account that has no contract - at all, so there was nothing for it to be about. - - A business never sees it, and the server refuses them again - in Settings::withdraw() — a card that is not rendered has - never stopped anybody who can post to /livewire/update. - - Not a variant of the cancellation above: that one ends a - contract validly concluded and keeps the term already paid - for; this one unwinds the contract, ends the service the same - day and sends the whole amount back. --}} - @if ($withdrawal->applies) - -
-

- @if ($withdrawal->open) - {{ __('withdrawal.card_sub', [ - 'date' => $withdrawal->endsAt->local()->isoFormat('LL'), - 'days' => $withdrawal->daysLeft(), - ]) }} - @else - {{ $withdrawal->refusal }} - @endif -

- @if ($withdrawal->open) - - {{ __('withdrawal.cta') }} - - @endif -
- - @endif - - {{-- ── AV-Vertrag & TOM ────────────────────────────────────── - Art. 28 DSGVO wants a contract wherever personal data is - processed on somebody else's behalf. It is concluded WITH the - terms at checkout (see the AGB), so this is not a second - signing ceremony: the document has to be available, current - and retrievable. The extra confirmation is offered because a - practice or a firm that gets audited often wants one on file. - - Nothing renders until an operator has published a version. --}} - @if ($dpa !== null) - -
-
- - {{ __('dpa.read_agreement') }} - - - {{ __('dpa.download') }} - - @if ($dpa->measures_path) - - - {{ __('dpa.read_measures') }} + + + {{ __('settings.twofa_enable') }} - - {{ __('dpa.download') }} - - @endif - {{ __('dpa.version', ['version' => $dpa->version]) }} -
- - @if ($dpaAcceptance) - {{-- R19: stored in UTC, read on the wall clock. --}} -

- - {{ __('dpa.accepted_on', [ - 'version' => $dpa->version, - 'when' => $dpaAcceptance->accepted_at->local()->isoFormat('LL, LT'), - ]) }} -

- @else -
-

{{ __('dpa.in_force_note') }}

- - {{ __('dpa.accept_cta') }} - -
+ @endif -
- - @endif - {{-- Closing the account. Last row of the panel, where the - irreversible things belong. --}} - -
- - {{ __('settings.close_cta') }} - -
- - + @if ($recoveryCodes) + {{-- Shown once, on purpose: nobody writes down + what they were never shown, and these are the + way back in when the phone is gone. --}} + +
    + @foreach ($recoveryCodes as $code)
  • {{ $code }}
    • @endforeach +
+ + @endif + + - {{-- The deadlines, said where somebody asks the question. "Nach fünf - Tagen gelöscht" on the verification page reads as if it applied - to every account; it applies to a registration nobody confirmed. - The other rule — a year without a package — was nowhere at all. - Both come from the commands that enforce them, so the page cannot - drift from what actually happens. --}} - -
-
    -
  • {{ __('settings.lifecycle_unverified', ['days' => App\Console\Commands\PruneUnverifiedAccounts::AFTER_DAYS]) }}
    • -
  • {{ __('settings.lifecycle_dormant', ['warn' => App\Console\Commands\PruneDormantAccounts::WARN_DAYS_BEFORE]) }}
    • -
  • {{ __('settings.lifecycle_kept') }}
    • -
- - {{ __('settings.lifecycle_terms') }} - + {{-- The devices. It answers the same question two-factor does + — who can get into this account. --}} +
+ @livewire('sessions') +
- + @endif + + {{-- ══ Erscheinungsbild ═════════════════════════════════════════ --}} + @if ($tab === 'branding') +
+
+

{{ __('settings.branding_title') }}

+

{{ __('settings.branding_sub') }}

+ + + + + + + +
+ + +
+ @error('brandPrimary')

{{ $message }}

@enderror + + + +
+ + +
+ @error('brandAccent')

{{ $message }}

@enderror + + + +
+
+ @if ($logo) + + @elseif ($logoUrl) + + @else + {{ __('settings.brand_default') }} + @endif +
+
+ + @if ($logoUrl) + + @endif + @error('logo')

{{ $message }}

@enderror +
+
+ + + + @if ($branding['is_default'] ?? false) +

{{ __('settings.brand_using_default') }}

+ @endif + + {{ __('settings.save') }} + + + +
+
+ @endif + + {{-- ══ Vertrag ══════════════════════════════════════════════════ --}} + @if ($tab === 'contract') +
+ +
+

{{ __('settings.package_title') }}

+

{{ __('settings.contract_sub') }}

+ + + +
+

+ @if ($cancellationScheduled) + {{ __('settings.cancel_scheduled_body', ['date' => $instance?->service_ends_at?->local()->isoFormat('LL')]) }} + @elseif ($hasActivePackage) + {{ __('settings.package_active', ['plan' => $instance ? __('billing.plan.'.$instance->plan) : '—']) }} + @else + {{ __('settings.no_package') }} + @endif +

+ + @if ($hasActivePackage && ! $cancellationScheduled) + + {{ __('settings.cancel_cta') }} + + @elseif (! $hasActivePackage && ! $cancellationScheduled) + + {{ __('settings.to_packages') }} + + @endif +
+ + + {{-- ── Das Widerrufsrecht ──────────────────────── + Shown to every CONSUMER with a contract, open or + not. It used to render only while the window was + open, so once the fourteen days expired the block + vanished — indistinguishable from a feature + nobody built. + + A business never sees it, and Settings::withdraw() + refuses them again on the server: a card that is + not rendered has never stopped anybody who can + post to /livewire/update. + + Not a variant of the cancellation above it: that + one ends a contract validly concluded and keeps + the term already paid for; this one unwinds the + contract, ends the service the same day and sends + the whole amount back. --}} + @if ($withdrawal->applies) + +
+

+ @if ($withdrawal->open) + {{ __('withdrawal.card_sub', [ + 'date' => $withdrawal->endsAt->local()->isoFormat('LL'), + 'days' => $withdrawal->daysLeft(), + ]) }} + @else + {{ $withdrawal->refusal }} + @endif +

+ @if ($withdrawal->open) + + {{ __('withdrawal.cta') }} + + @endif +
+ + @endif + +
+ + {{-- ── AV-Vertrag & TOM ────────────────────────────────── + Art. 28 DSGVO wants a contract wherever personal data is + processed on somebody else's behalf. It is concluded WITH + the terms at checkout (see the AGB), so this is not a + second signing ceremony: the document has to be + available, current and retrievable. The extra + confirmation is offered because a practice or a firm that + gets audited often wants one on file. + + Nothing renders until an operator has published a + version. --}} + @if ($dpa !== null) +
+
+

{{ __('dpa.title') }}

+ {{ __('dpa.version', ['version' => $dpa->version]) }} +
+

{{ __('dpa.sub') }}

+ + + +
+ + {{ __('dpa.open') }} + + + {{ __('dpa.download') }} + +
+ + + @if ($dpa->measures_path) + +
+ + {{ __('dpa.open') }} + + + {{ __('dpa.download') }} + +
+ + @endif + + + @if ($dpaAcceptance) + {{-- R19: stored in UTC, read on the wall clock. --}} +

+ + {{ __('dpa.accepted_on', [ + 'version' => $dpa->version, + 'when' => $dpaAcceptance->accepted_at->local()->isoFormat('LL, LT'), + ]) }} +

+ @else +
+

{{ __('dpa.in_force_note') }}

+ + {{ __('dpa.accept_cta') }} + +
+ @endif + + +
+ @endif + + {{-- ── What ends an account ────────────────────────────── + The deadlines and the button that closes it, together and + last, in a card whose border says what kind of thing this + is. Everything irreversible on this page lives here. + + The deadlines come from the commands that enforce them, + so the page cannot drift from what actually happens. --}} +
+

{{ __('settings.close_account_title') }}

+

{{ __('settings.close_account_sub') }}

+ + + +
    +
  • {{ __('settings.lifecycle_unverified', ['days' => App\Console\Commands\PruneUnverifiedAccounts::AFTER_DAYS]) }}
    • +
  • {{ __('settings.lifecycle_dormant', ['warn' => App\Console\Commands\PruneDormantAccounts::WARN_DAYS_BEFORE]) }}
    • +
  • {{ __('settings.lifecycle_kept') }}
    • +
+ + {{ __('settings.lifecycle_terms') }} + + + + +
+ + {{ __('settings.close_cta') }} + +
+ + +
+
+ @endif
- @endif +
From 69a9322a4efb1af9d10026363c0b20db55403ad2 Mon Sep 17 00:00:00 2001 From: nexxo Date: Thu, 30 Jul 2026 17:59:16 +0200 Subject: [PATCH 09/14] Release v1.3.63 Co-Authored-By: Claude Opus 5 --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index 02c1916..fe0c4db 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.3.62 +1.3.63 From 048e5ba81f87a56cfc72d085d4547e87b191016b Mon Sep 17 00:00:00 2001 From: nexxo Date: Thu, 30 Jul 2026 18:20:27 +0200 Subject: [PATCH 10/14] Subtract only what was counted, not everything a singleton adopted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A figure may only subtract what it counted as an intent in the first place. The family loop counts a Product before knowing whether it will be adopted or minted, so an adoption there could honestly be subtracted back out — but syncModules() has never counted a module's own Product as an intent at all; it only counts a module's Prices. Reading AdoptStripeProduct::adoptions as one run-wide delta could not tell the two apart, so a module Product adopted from an interrupted run silently inflated "adopted" and shrank "created" by exactly one, for a Product the command never claimed to have made in the first place. The fix counts the family side locally, at the one call site that already counts the intent, and leaves a module's Product out of both figures entirely — adopted or minted, it was never counted, so neither number may move for it. AdoptStripeProduct::$adoptions is gone with it: nothing reads a singleton-wide total that cannot be attributed to one side or the other, and keeping it around unread would be exactly the kind of state this codebase does not leave lying about. The duplicate-product report had the same shape of bug one line down: it read straight off the singleton's list with no before/after snapshot, so a second handle() call in one process would reprint a duplicate an earlier run already named. Sliced to what this run itself added, the same way the counters beside it already were. Also: the unread `$charged` line in the adoption test that TDD had already exercised as dead weight, and its now-unused PlanPrices import. Co-Authored-By: Claude Opus 5 --- app/Console/Commands/SyncStripeCatalogue.php | 47 ++++++++++++++----- app/Providers/AppServiceProvider.php | 11 +++-- app/Services/Billing/AdoptStripeProduct.php | 12 ----- .../Billing/StripeProductAdoptionTest.php | 34 +++++++++++++- 4 files changed, 74 insertions(+), 30 deletions(-) diff --git a/app/Console/Commands/SyncStripeCatalogue.php b/app/Console/Commands/SyncStripeCatalogue.php index c18f3b3..f880966 100644 --- a/app/Console/Commands/SyncStripeCatalogue.php +++ b/app/Console/Commands/SyncStripeCatalogue.php @@ -91,9 +91,21 @@ class SyncStripeCatalogue extends Command $adoptPrices = app(AdoptStripePrice::class); $adoptProducts = app(AdoptStripeProduct::class); $adoptedPricesBefore = $adoptPrices->adoptions; - $adoptedProductsBefore = $adoptProducts->adoptions; + // Duplicates accumulate on the SAME singleton across the whole process, + // not only this run — without this, a second handle() in one process + // would print a duplicate a previous run already reported. + $duplicatesBefore = count($adoptProducts->duplicates); $created = 0; + // Counted here, locally, rather than on AdoptStripeProduct itself: the + // very same class also adopts a MODULE's Product, from + // AddonPrices::product() inside syncModules() below, and a module's + // Product is never counted as an intent at all — unlike a family's, + // which gets its own "product …" line and $created++ right where this + // is incremented. A single counter on the class could not tell the two + // apart; counting only at the one call site that also counts the + // intent avoids the question entirely. + $adoptedFamilyProducts = 0; foreach (PlanFamily::query()->with('versions.prices')->orderBy('tier')->get() as $family) { $published = $family->versions->filter(fn (PlanVersion $version) => $version->isPublished()); @@ -121,6 +133,13 @@ class SyncStripeCatalogue extends Command // family's. $productId = $adoptProducts($metadata, ['plan_family_id']); + if ($productId !== null) { + // The intent was counted three lines up, so — unlike a + // module's Product — an adoption here may honestly be + // subtracted back out of it below. + $adoptedFamilyProducts++; + } + $productId ??= $stripe->createProduct( $family->name, $metadata, @@ -145,18 +164,24 @@ class SyncStripeCatalogue extends Command $this->newLine(); - // Both counted the same way and for the same reason: $created is an - // INTENT taken before we know whether ensure()/the adoption call will - // mint or adopt, for a price as well as for a family's product — see - // AdoptStripePrice. Combined into one "adopted" figure because an - // operator reading the summary line cares whether Stripe made anything - // NEW, not which kind of object it would have been. - $adoptedPrices = $adoptPrices->adoptions - $adoptedPricesBefore; - $adoptedProducts = $adoptProducts->adoptions - $adoptedProductsBefore; - $adopted = $adoptedPrices + $adoptedProducts; + // $adopted may only subtract what $created actually counted as an + // intent. Every price is one, family or module — syncPrice() and + // syncModules() both increment $created before knowing whether ensure() + // will mint or adopt, so the full price-adoption delta is honest. A + // family's Product is one too, counted above, so $adoptedFamilyProducts + // is honest for the same reason. A module's Product is NOT one — it + // prints no line of its own and increments nothing in syncModules(), + // however AdoptStripeProduct answers it — so its adoption stays out of + // both figures entirely. Subtracting it would report fewer objects + // created than Stripe actually gained. + $adopted = ($adoptPrices->adoptions - $adoptedPricesBefore) + $adoptedFamilyProducts; $minted = max(0, $created - $adopted); - foreach ($adoptProducts->duplicates as $duplicate) { + // Sliced to what THIS run added — the list lives on the same + // process-wide singleton as the count above, so without the same + // before/after care a second handle() in one process would print a + // duplicate a previous run already reported. + foreach (array_slice($adoptProducts->duplicates, $duplicatesBefore) as $duplicate) { // Named here as well as in the log: an operator running the sweep // reads this, and a second Product for one family is something only // a person can resolve — we deliberately do not deactivate it. diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index 865a1df..5bdc8fb 100644 --- a/app/Providers/AppServiceProvider.php +++ b/app/Providers/AppServiceProvider.php @@ -61,11 +61,12 @@ class AppServiceProvider extends ServiceProvider // Both singletons because each carries a record across one run that the // caller reads only after it finishes: which products were duplicates, - // and how many products and prices were adopted rather than created. - // Resolved per call they would each start empty on every read. PlanPrices - // is deliberately NOT a singleton — SyncStripeCatalogue asks the container - // for it once per catalogue row — which is exactly why the counter has to - // live here instead: on a per-call instance it could never pass one. + // and how many prices were adopted rather than created. Resolved per + // call they would each start empty on every read. PlanPrices is + // deliberately NOT a singleton — SyncStripeCatalogue asks the container + // for it once per catalogue row — which is exactly why a record like + // this has to live on the adoption classes instead: on a per-call + // instance it could never carry across the row it started on. $this->app->singleton(AdoptStripeProduct::class); $this->app->singleton(AdoptStripePrice::class); diff --git a/app/Services/Billing/AdoptStripeProduct.php b/app/Services/Billing/AdoptStripeProduct.php index 3145fd5..4f5f46a 100644 --- a/app/Services/Billing/AdoptStripeProduct.php +++ b/app/Services/Billing/AdoptStripeProduct.php @@ -44,16 +44,6 @@ final class AdoptStripeProduct */ public array $duplicates = []; - /** - * How many Products this run took over rather than minted. - * - * Mirrors AdoptStripePrice::$adoptions, and for the same reason: - * stripe:sync-catalogue counts an intent to create a Product BEFORE it - * knows whether this class will hand back an existing one, so a Product - * that was adopted must not silently count as one that was created. - */ - public int $adoptions = 0; - public function __construct(private readonly StripeClient $stripe) {} /** @@ -108,8 +98,6 @@ final class AdoptStripeProduct 'metadata' => $metadata, ]); - $this->adoptions++; - return $adopted['id']; } diff --git a/tests/Feature/Billing/StripeProductAdoptionTest.php b/tests/Feature/Billing/StripeProductAdoptionTest.php index 2820bc0..9ea9275 100644 --- a/tests/Feature/Billing/StripeProductAdoptionTest.php +++ b/tests/Feature/Billing/StripeProductAdoptionTest.php @@ -7,7 +7,6 @@ use App\Models\StripePlanPrice; use App\Models\Subscription; use App\Services\Billing\AddonPrices; use App\Services\Billing\AdoptStripeProduct; -use App\Services\Billing\PlanPrices; use App\Services\Billing\TaxTreatment; use App\Services\Stripe\FakeStripeClient; use App\Services\Stripe\StripeClient; @@ -123,7 +122,6 @@ it('says it adopted, not that it created', function () { app(Kernel::class)->call('stripe:sync-catalogue'); $row = PlanPrice::query()->firstOrFail(); - $charged = PlanPrices::chargedCents($row, TaxTreatment::domestic()); $orphan = (string) StripePlanPrice::query() ->where('plan_price_id', $row->id)->where('reverse_charge', false) ->value('stripe_price_id'); @@ -152,3 +150,35 @@ it('names a duplicate product in its report, without touching it', function () { expect($family->refresh()->stripe_product_id)->toBe('prod_first'); }); + +it('does not fold a module Product adoption into the created/minted count', function () { + // A module's prices are all genuinely new, and its Product exists at + // Stripe already — unlinked from our own register, the same shape as a + // family's orphaned Product one level up (AddonPrices::product() asks + // AdoptStripeProduct exactly as the family loop does). The difference that + // makes this dangerous: syncModules() below counts a module's PRICES as + // intents, but it has never counted a module's PRODUCT as one — adopted or + // minted, that Product prints no line and increments nothing — so + // adopting it here must neither inflate "adopted" nor shrink "created". + $this->stripe->plantProduct('prod_priority_support_orphan', 'Priority Support', [ + 'addon' => 'priority_support', + ]); + + // Computed from the catalogue rather than hardcoded, the way + // ReverseChargePriceTest sizes a rate-change sweep: one Product per family, + // two treatments per priced row, and monthly+yearly at both treatments for + // every module — all of it newly minted here, nothing pre-existing to + // adopt except the one Product just planted above. + $modules = count(array_merge(array_keys((array) config('provisioning.addons')), ['storage'])); + $expectedCreated = PlanFamily::query()->count() + PlanPrice::query()->count() * 2 + $modules * 4; + + $this->artisan('stripe:sync-catalogue') + ->expectsOutputToContain("{$expectedCreated} object(s) created, 0 adopted in Stripe.") + ->assertSuccessful(); + + // And the orphan really was taken over rather than merely ignored — the + // Product this test is about, not a no-op that would pass it by accident. + expect(StripeAddonPrice::query() + ->where('addon_key', 'priority_support') + ->value('stripe_product_id'))->toBe('prod_priority_support_orphan'); +}); From 277a6bab830d4f4d70ef9f300a89dc33a47c2178 Mon Sep 17 00:00:00 2001 From: nexxo Date: Thu, 30 Jul 2026 18:36:06 +0200 Subject: [PATCH 11/14] Give the operator something to do about an orphan nobody can adopt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The recognition step refuses a price that proves nothing about itself, and that is right — adopting a stranger's price is worse than minting a second one. It left an operator with a log line and no way to act on it. Reports by default and touches nothing. With --archive the orphans stop being sold, which is harmless for the reason it always is here: Stripe goes on billing every subscription already on an archived price, and nothing is sold on a price no row knows. Co-Authored-By: Claude Opus 5 --- .../Commands/SweepOrphanStripePrices.php | 109 ++++++++++++++++++ ...-07-30-stripe-adoption-followups-design.md | 2 +- ...2026-07-30-stripe-price-adoption-design.md | 46 +++++--- .../Feature/Billing/SweepOrphanPricesTest.php | 66 +++++++++++ 4 files changed, 206 insertions(+), 17 deletions(-) create mode 100644 app/Console/Commands/SweepOrphanStripePrices.php create mode 100644 tests/Feature/Billing/SweepOrphanPricesTest.php diff --git a/app/Console/Commands/SweepOrphanStripePrices.php b/app/Console/Commands/SweepOrphanStripePrices.php new file mode 100644 index 0000000..edc355f --- /dev/null +++ b/app/Console/Commands/SweepOrphanStripePrices.php @@ -0,0 +1,109 @@ +option('dry-run'); + $archive = (bool) $this->option('archive'); + + if (! $dryRun && ! $stripe->isConfigured()) { + $this->error('Stripe is not configured (STRIPE_SECRET is empty). Nothing was read.'); + + return self::FAILURE; + } + + $known = StripeAddonPrice::query()->pluck('stripe_price_id') + ->merge(StripePlanPrice::query()->pluck('stripe_price_id')) + ->filter() + ->flip(); + + $found = 0; + + foreach ($this->products() as $productId) { + foreach ($stripe->activePricesFor($productId) as $price) { + if ($known->has($price['id'])) { + continue; + } + + $found++; + + $this->line(sprintf( + ' orphan %s %d %s %s product %s %s', + $price['id'], + $price['unit_amount'], + $price['currency'], + $price['interval'], + $productId, + $price['metadata'] === [] ? 'no metadata' : json_encode($price['metadata']), + )); + + if ($archive && ! $dryRun) { + $stripe->archivePrice($price['id']); + } + } + } + + $this->newLine(); + + if ($found === 0) { + $this->info('Every active price at our products is accounted for.'); + + return self::SUCCESS; + } + + $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.", + }); + + return self::SUCCESS; + } + + /** + * Every Stripe Product this platform owns: one per plan family, one per + * module. Read from our own rows rather than from Stripe, because a Product + * we do not know is not one whose Prices we can judge. + * + * @return array + */ + private function products(): array + { + return PlanFamily::query()->whereNotNull('stripe_product_id')->pluck('stripe_product_id') + ->merge(StripeAddonPrice::query()->pluck('stripe_product_id')) + ->filter() + ->unique() + ->values() + ->all(); + } +} 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 f246df0..89677e8 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 @@ -250,7 +250,7 @@ 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`, `$adoptions`) trägt + Singleton, damit ihr Lauf-Protokoll (`$duplicates`) trägt - `app/Console/Commands/SyncStripeCatalogue.php` — Produkt-Wiedererkennung, getrennte Zählung, Kommentar berichtigt - `tests/Feature/Billing/StripePriceAdoptionTest.php`, diff --git a/docs/superpowers/specs/2026-07-30-stripe-price-adoption-design.md b/docs/superpowers/specs/2026-07-30-stripe-price-adoption-design.md index be50139..d53c1e4 100644 --- a/docs/superpowers/specs/2026-07-30-stripe-price-adoption-design.md +++ b/docs/superpowers/specs/2026-07-30-stripe-price-adoption-design.md @@ -277,10 +277,11 @@ Zwei Regeldatei-Tests nach Repo-Sitte, getrennt danach, **gegen wen** sie prüfe ## 14. Folgepunkte, die aus der Umsetzung entstanden sind -Nachgetragen nach dem Abschluss-Review. Alle Befunde sind offengelegt, keiner -blockiert den Merge — aber keiner ist erledigt. +Nachgetragen nach dem Abschluss-Review. Die Fortsetzung steht in +`docs/superpowers/specs/2026-07-30-stripe-adoption-followups-design.md`; jeder +Punkt unten ist abgehakt, mit der Datei, die ihn schließt. -### Die eine echte Lücke: Waisen-**Produkte** +### Die eine echte Lücke: Waisen-**Produkte** — erledigt `activePricesFor()` erkennt einen verwaisten *Preis* wieder. Für ein verwaistes **Produkt** gibt es kein Gegenstück, und dort ist der Schaden größer: ein @@ -295,7 +296,11 @@ benennen die Lücke jetzt; der Fix wäre ein `productsFor()` nach dem Muster von `activePricesFor()`, das Stripe nach den Produkten fragt und über die Metadaten wiedererkennt. -### Das Geld-Tor sitzt an der falschen Stelle +**Geschlossen durch:** `App\Services\Billing\AdoptStripeProduct`, beide +Verdrahtungen (`SyncStripeCatalogue::handle()` für eine Familie, +`AddonPrices::product()` für ein Modul). + +### Das Geld-Tor sitzt an der falschen Stelle — erledigt `AdoptStripePrice` verspricht in seinem Kopfkommentar, keine Übernahme könne Geld verschieben — kann aber nur Betrag, Währung und Intervall vergleichen. Was @@ -307,53 +312,62 @@ diese Felder gar nicht ausdrücken, also kann **kein Test von `StripeClient` ließe es still fallen. Die Prüfung gehört in die Klasse, die sie zusagt. +**Geschlossen durch:** die Prüfung sitzt jetzt in `AdoptStripePrice` selbst; +`activePricesFor()`s Vertrag liefert die vier preisbestimmenden Eigenschaften, +statt selbst zu filtern. + Verwandt: `PlanPrices::ensure()` hat keine `<= 0`-Schranke, wo `AddonPrices::ensure()` eine hat. Das ist das Einzige, was einen `billing_scheme: tiered`-Preis (bei dem Stripe `unit_amount: null` liefert, hier als 0 gelesen) auf der Paketseite überhaupt erreichbar macht. +**Bewusst nicht gebaut.** Begründung in der neuen Spec §3: sobald +`AdoptStripePrice` selbst `billing_scheme !== 'per_unit'` ablehnt, hat die +Schranke keinen Zweck mehr — sie würde nur noch ein Paket zu null (ein +kostenloser Tarif, heute nicht im Katalog, aber jederzeit anlegbar) treffen und +es unverkäuflich machen. + ### Kleineres, benannt statt vergessen - **Der Abgleich zählt „angelegt", auch wo er übernommen hat.** Die Wortwahl der Konsolenausgabe ist entschärft („created or adopted"), gezählt wird weiterhin vor dem Aufruf. Eine echte Trennung der beiden Zahlen fehlt — und das ist die eine Zahl, die ein Mensch nach einem Vorfall zuerst liest. + **Erledigt** — getrennte Zählung in `SyncStripeCatalogue::handle()`. - **Die Warnung über einen unerklärlichen Preis hat keine Drosselung.** Sie feuert bei jedem Abgleich *und* jeder Kundenbuchung, solange die Waise existiert — und weil das Aufräum-Kommando (§12) ein Folgepunkt ist, auf unbestimmte Zeit. Der Widerspruchs-Pfad wurde aus genau diesem Grund still gestellt; der Bestätigungs-Pfad bekam dieselbe Überlegung nicht. + **Erledigt** — einmal pro Preis und Tag, über `Cache::add()`. - **`FakeStripeClient::updatePriceMetadata()` ersetzt, wo Stripe zusammenführt**, und `activePricesFor()` gibt Metadaten ungecastet zurück, wo der HTTP-Client auf String castet. Heute nicht beobachtbar, aber es ist genau die Sorte Untreue des Fakes, gegen die §9 geschrieben wurde. + **Erledigt.** - **`created` im Fake kann gleichstehen**: `createPrice()` stempelt `count($prices) + 1`, `plantPrice()` hat die Vorgabe `1`. Der Kommentar sagt das jetzt zu, statt es zu bestreiten; ein Gleichstand-Tiebreak auf die ID fehlt weiter. + **Erledigt** — Gleichstand wird über die Preis-ID aufgelöst. - **`HttpStripeClient::activePricesFor()` bricht das Blättern still ab**, wenn dem letzten Eintrag einer nicht-letzten Seite die `id` fehlt — getreue Kopie desselben Verhaltens in `invoiceLines()`. Die Folge hat sich aber geändert: in `invoiceLines()` entsteht ein zu kurzer Belegtext, hier eine ungesehene Waise und ein zweiter aktiver Preis, also der Vorfall. Wer es anfasst, soll werfen statt anhalten. + **Erledigt** — wirft jetzt, für Preise und Produkte. - **Der Idempotenz-Fingerabdruck und der POST-Rumpf müssen zusammenbleiben.** Ein Feld, das künftig in `createPrice()` gesendet, aber nicht in `IdempotencyKey::priceParameters()` aufgenommen wird, öffnet den Vorfall wieder. Der Test „does not block a booking because the metadata format moved" wacht über diese Nahtstelle — mit einem gesetzten Ledger-Eintrag, weil der Zustand heute nicht mehr fahrbar ist. + **Bleibt offen** — es ist eine Naht, kein Fehler; der Test darüber existiert. -### Zwei fremde, vorbestehende Testfehler, die dieser Zweig aufgedeckt hat +### Ein fremder, vorbestehender Testfehler, den dieser Zweig aufgedeckt hat -Keiner davon wird von diesem Zweig verursacht. - -1. **`HostStepsTest.php:1064`** schreibt `fsn-01.node.clupilot.com` hart hinein, - während `.env:107` `CLUPILOT_DNS_ZONE=clupilot.cloud` setzt und `phpunit.xml` - neun andere Variablen mit `force="true"` festnagelt, diese aber nicht. Eine - Zeile in `phpunit.xml`. Derselbe Punkt steht als offene Aufgabe in - `docs/handoffs/2026-07-30-real-run-handoff.md`. -2. **`PlanCatalogueTest.php`** ist einmal ausgefallen und lief beim - Wiederholen durch. „Beim zweiten Mal grün" ist keine Diagnose, und ein - sprunghafter Fehler in den Katalogpreis-Tests liegt im Wirkungskreis dieses - Zweigs, auch wenn er nicht von ihm kommt. +**`PlanCatalogueTest.php`** ist einmal ausgefallen und lief beim Wiederholen +durch. „Beim zweiten Mal grün" ist keine Diagnose, und ein sprunghafter Fehler +in den Katalogpreis-Tests liegt im Wirkungskreis dieses Zweigs, auch wenn er +nicht von ihm kommt. **Bleibt offen** — Ursachensuche, eigene Sitzung. diff --git a/tests/Feature/Billing/SweepOrphanPricesTest.php b/tests/Feature/Billing/SweepOrphanPricesTest.php new file mode 100644 index 0000000..c33f453 --- /dev/null +++ b/tests/Feature/Billing/SweepOrphanPricesTest.php @@ -0,0 +1,66 @@ +stripe = new FakeStripeClient; + app()->instance(StripeClient::class, $this->stripe); + app(Kernel::class)->call('stripe:sync-catalogue'); + + $this->product = (string) StripeAddonPrice::query() + ->where('addon_key', 'priority_support') + ->value('stripe_product_id'); + + $this->known = (string) StripeAddonPrice::query() + ->where('addon_key', 'priority_support') + ->where('interval', 'month') + ->where('reverse_charge', false) + ->value('stripe_price_id'); + + $this->stripe->plantPrice('price_orphan', $this->product, 9900, 'EUR', 'month', []); +}); + +it('reports the orphan and leaves it alone', function () { + $this->artisan('stripe:sweep-orphan-prices') + ->expectsOutputToContain('price_orphan') + ->assertSuccessful(); + + expect($this->stripe->archived)->toBe([]); +}); + +it('never reports a price a row knows', function () { + $this->artisan('stripe:sweep-orphan-prices') + ->doesntExpectOutputToContain($this->known) + ->assertSuccessful(); +}); + +it('stops selling the orphan when asked to', function () { + $this->artisan('stripe:sweep-orphan-prices', ['--archive' => true]) + ->assertSuccessful(); + + // Archiving is harmless for the same reason it always is here: Stripe goes + // on billing every subscription already on an archived Price, it merely + // stops being sold. + expect($this->stripe->archived)->toBe(['price_orphan']); +}); + +it('touches nothing on a dry run, even when asked to archive', function () { + $this->artisan('stripe:sweep-orphan-prices', ['--archive' => true, '--dry-run' => true]) + ->expectsOutputToContain('price_orphan') + ->assertSuccessful(); + + expect($this->stripe->archived)->toBe([]); +}); From 571acd0013812d525d9b67d6e94b79b9b01dbe2d Mon Sep 17 00:00:00 2001 From: nexxo Date: Thu, 30 Jul 2026 18:48:49 +0200 Subject: [PATCH 12/14] Check Stripe is configured on every path the sweep actually uses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dry-run guard was copied from stripe:sync-catalogue, where it is right: that command's --dry-run touches nothing, because everything it compares already lives in our own tables. This command has no such local-only mode — the report IS a live activePricesFor() call, dry-run or not — so `! $dryRun &&` made the guard fire only on the one path that would have worked anyway. A --dry-run against an unconfigured account fell straight into HttpStripeClient's own exception instead of the clean error and self::FAILURE. Co-Authored-By: Claude Opus 5 --- app/Console/Commands/SweepOrphanStripePrices.php | 8 +++++++- tests/Feature/Billing/SweepOrphanPricesTest.php | 13 +++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/app/Console/Commands/SweepOrphanStripePrices.php b/app/Console/Commands/SweepOrphanStripePrices.php index edc355f..cecd28f 100644 --- a/app/Console/Commands/SweepOrphanStripePrices.php +++ b/app/Console/Commands/SweepOrphanStripePrices.php @@ -36,7 +36,13 @@ class SweepOrphanStripePrices extends Command $dryRun = (bool) $this->option('dry-run'); $archive = (bool) $this->option('archive'); - if (! $dryRun && ! $stripe->isConfigured()) { + // Checked on EVERY path, unlike stripe:sync-catalogue's --dry-run, + // which touches nothing because everything it compares is already in + // our own tables. This command has no local-only mode: the report — + // the whole point of it, dry-run or not — is activePricesFor(), a + // live call. Skipping this check under --dry-run would not skip the + // call, only the graceful error in front of it. + if (! $stripe->isConfigured()) { $this->error('Stripe is not configured (STRIPE_SECRET is empty). Nothing was read.'); return self::FAILURE; diff --git a/tests/Feature/Billing/SweepOrphanPricesTest.php b/tests/Feature/Billing/SweepOrphanPricesTest.php index c33f453..1ab3771 100644 --- a/tests/Feature/Billing/SweepOrphanPricesTest.php +++ b/tests/Feature/Billing/SweepOrphanPricesTest.php @@ -64,3 +64,16 @@ it('touches nothing on a dry run, even when asked to archive', function () { expect($this->stripe->archived)->toBe([]); }); + +it('fails cleanly on a dry run when Stripe is not configured, rather than throwing', function () { + // Unlike stripe:sync-catalogue, this command has no local-only path: the + // report itself is a live activePricesFor() call, dry-run or not. A guard + // that only fired outside --dry-run would let this fall straight into + // HttpStripeClient's own "not configured" exception instead of the clean + // error below. + $this->stripe->configured = false; + + $this->artisan('stripe:sweep-orphan-prices', ['--dry-run' => true]) + ->expectsOutputToContain('Stripe is not configured (STRIPE_SECRET is empty). Nothing was read.') + ->assertFailed(); +}); From a43be4186a41b974a73d38cfa0a437786196dd92 Mon Sep 17 00:00:00 2001 From: nexxo Date: Thu, 30 Jul 2026 19:12:57 +0200 Subject: [PATCH 13/14] Say what the sweep did not check, before telling anyone to archive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit stripe:sweep-orphan-prices described itself as listing "what AdoptStripePrice will not take over". It computes nothing of the sort: it lists every active Price at one of our Products that no register row knows, adoptable or not — the only question it asks is whether a row holds the id. And then it invited the destructive path, with no word anywhere about running stripe:sync-catalogue first. Following that advice can make a plan unsellable. Archive an orphan the next sync would have adopted and the sync can no longer see it: activePricesFor() asks Stripe for active prices only, so adoption returns null and createPrice() fires under the same key the crashed run used. For twenty-four hours Stripe answers a repeated key by replaying the stored response — the archived Price's id, in the body it had while it was live. The register writes it down as live, and the checkout is refused. This is the same class of defect the branch exists against: a comment that was eloquent and wrong. - The docblock now says what the command lists, names the twenty-four-hour replay so nobody has to rediscover it, and the ordering requirement is in $description and printed on every path that found something, --archive included. The reassurance that archiving is harmless is now scoped to what THIS codebase sells; a payment link an operator built by hand against one of our Products is withdrawn all the same, and says so. - stripe:sync-catalogue --dry-run says "created or adopted" again. A dry run counts intents without calling ensure(), so it cannot know which Stripe already holds — and a dry run is the first thing an operator runs after an interrupted one. The live line keeps both figures, where they are knowable. - The sweep's plan side gets a test: an orphan at a family Product is reported and a known plan Price never is. Deleting either half of the lookup turns it red; the five tests it joins all stayed green for the products() half. - The throttle test pinned "once" and neither "per price" nor "per day". A key mutated to a constant would have silenced every orphan after the first and kept it green — the silent no-op the branch is built against. It now plants a second orphan, and travels a day to hear the first one again. - Two of my own documents corrected. The spec sentence claiming an adoptable orphan never appears in this list is what the implementation faithfully built; it holds only if a sync has run since the orphan appeared, which nothing enforced. And the two adoption classes do not carry the same run log: $adoptions on the Price side, $duplicates on the Product side. Co-Authored-By: Claude Opus 5 --- .../Commands/SweepOrphanStripePrices.php | 41 ++++++++++++++++--- app/Console/Commands/SyncStripeCatalogue.php | 8 +++- docs/handoffs/2026-07-30-real-run-handoff.md | 14 +++++++ ...-07-30-stripe-adoption-followups-design.md | 27 ++++++++++-- .../Billing/StripePriceAdoptionTest.php | 28 ++++++++++++- .../Feature/Billing/SweepOrphanPricesTest.php | 39 ++++++++++++++++++ 6 files changed, 146 insertions(+), 11 deletions(-) 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(); From 79654333a8eccc0c6c003f096c2f82f21a67144f Mon Sep 17 00:00:00 2001 From: nexxo Date: Thu, 30 Jul 2026 19:24:18 +0200 Subject: [PATCH 14/14] Name the recurring filter, and stop the spec contradicting itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three sentences left over from the wave before, all of the kind this branch exists to end. - "Every active Price" was one word short. activePricesFor() sends `type: recurring` as well as `active: true`, so a one-time Price is never listed — which made the hand-built-payment-link sentence false for a one-time link: neither listed nor archived. Both places say `recurring` now, and the docblock says why the filter is not a gap: createPrice() always sends `recurring[interval]` and is the only call that mints a Price at one of our Products, so the filter can only ever hide somebody else's one-time Price. A filter left out of a sentence is a gap the next reader goes looking for. - The sweep test file still opened "The orphans nobody can adopt" — the exact narrowing the command's own docblock had to be corrected away from. It happens to describe the fixtures, which are all metadata-less, so it now says that instead of claiming it of the command. - The design doc called --archive "harmlos" seven lines above the correction establishing that it is not, for an adoptable orphan. A document that contradicts itself seven lines apart teaches nobody anything. Co-Authored-By: Claude Opus 5 --- .../Commands/SweepOrphanStripePrices.php | 26 +++++++++++++------ docs/handoffs/2026-07-30-real-run-handoff.md | 9 ++++--- ...-07-30-stripe-adoption-followups-design.md | 11 ++++---- .../Feature/Billing/SweepOrphanPricesTest.php | 19 +++++++++----- 4 files changed, 42 insertions(+), 23 deletions(-) diff --git a/app/Console/Commands/SweepOrphanStripePrices.php b/app/Console/Commands/SweepOrphanStripePrices.php index 2dbdeee..8e2c75e 100644 --- a/app/Console/Commands/SweepOrphanStripePrices.php +++ b/app/Console/Commands/SweepOrphanStripePrices.php @@ -18,10 +18,18 @@ 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. + * **What this lists is wider than that log line.** Every active RECURRING 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. + * + * Recurring, because activePricesFor() sends `type: recurring` as well as + * `active: true`. That filter can hide somebody else's one-time Price from the + * list; it can never hide one of ours. createPrice() always sends + * `recurring[interval]`, and it is the only call that mints a Price at one of + * our Products at all — the setup fee is priced inline at the checkout, under an + * ad-hoc Product of its own. So the filter errs toward listing too little, never + * toward missing an orphan of ours. * * **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 @@ -41,10 +49,12 @@ use Illuminate\Console\Command; * 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. + * further than this codebase. A Price an operator wired up by hand — a + * SUBSCRIPTION 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. A one-time link is the other side of the + * recurring filter: neither listed nor archived, and so neither protected nor + * disturbed by this command. */ class SweepOrphanStripePrices extends Command { diff --git a/docs/handoffs/2026-07-30-real-run-handoff.md b/docs/handoffs/2026-07-30-real-run-handoff.md index ab88736..c341fc9 100644 --- a/docs/handoffs/2026-07-30-real-run-handoff.md +++ b/docs/handoffs/2026-07-30-real-run-handoff.md @@ -223,9 +223,12 @@ 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. +- `stripe:sweep-orphan-prices` listet jeden **aktiven wiederkehrenden** + 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. Einmalige Preise sieht es nicht; von uns + angelegt ist nie einer davon, `createPrice()` schickt immer + `recurring[interval]`. 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 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 1e235c6..d8b3bd1 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 @@ -128,13 +128,14 @@ begründet. Die Log-Zeile allein reicht nicht: sie nennt die Waise, aber niemand kann etwas mit ihr tun, ohne von Hand in Stripes Oberfläche zu gehen. - **Ohne Argument:** ein Bericht. Für jedes unserer Produkte werden die aktiven - Preise geholt und die aufgelistet, die **keine Zeile** in + **wiederkehrenden** Preise geholt und die aufgelistet, die **keine Zeile** in `stripe_addon_prices` oder `stripe_plan_prices` kennt — mit ID, Betrag, Währung, Intervall und Metadaten. Nichts wird angefasst. -- **Mit `--archive`:** dieselbe Liste wird stillgelegt. Das ist harmlos, und - zwar aus demselben Grund wie überall sonst in diesem Projekt: ein - archivierter Preis verrechnet laufende Abos weiter, er wird nur nicht mehr - verkauft. +- **Mit `--archive`:** dieselbe Liste wird stillgelegt. Für alles, was **dieses + Projekt** verkauft, ist das harmlos, und zwar aus demselben Grund wie überall + sonst hier: ein archivierter Preis verrechnet laufende Abos weiter, er wird + nur nicht mehr verkauft. Für eine **übernehmbare** Waise gilt das ausdrücklich + **nicht** — siehe die Berichtigung unten. - **`--dry-run`** wie bei `stripe:sync-catalogue`, damit die beiden Kommandos sich gleich anfühlen. diff --git a/tests/Feature/Billing/SweepOrphanPricesTest.php b/tests/Feature/Billing/SweepOrphanPricesTest.php index 4d2937d..2b4a9fd 100644 --- a/tests/Feature/Billing/SweepOrphanPricesTest.php +++ b/tests/Feature/Billing/SweepOrphanPricesTest.php @@ -8,14 +8,19 @@ use App\Services\Stripe\StripeClient; use Illuminate\Contracts\Console\Kernel; /** - * The orphans nobody can adopt. + * The orphans an operator can finally act on. * - * A Price at one of our Products that no row knows, and that AdoptStripePrice - * refuses — because nothing in its metadata proves it is ours, or because it - * charges in a way we never mint. The recognition step names it in the log and - * leaves it, deliberately: adopting a stranger's Price is worse than minting a - * second one. This is the other half of that decision, the one an operator can - * act with. + * The command lists EVERY active recurring Price at one of our Products that no + * row knows — adoptable or not; the only question it asks is whether a register + * row holds the id. That is wider than the log line AdoptStripePrice writes, and + * it is why stripe:sync-catalogue has to run first: see the command's own + * docblock, which had to be corrected away from the narrower claim. + * + * The fixtures below are all at the narrow end anyway — planted with no metadata + * at all, so nothing could have adopted them, which is what a person clicking + * through Stripe's dashboard leaves behind. That keeps each test about the + * sweep's own question (does a row know this id?) rather than about what the + * recognition step would have done with it. */ beforeEach(function () { $this->stripe = new FakeStripeClient;