diff --git a/VERSION b/VERSION index 02c1916..fe0c4db 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.3.62 +1.3.63 diff --git a/app/Console/Commands/SweepOrphanStripePrices.php b/app/Console/Commands/SweepOrphanStripePrices.php new file mode 100644 index 0000000..8e2c75e --- /dev/null +++ b/app/Console/Commands/SweepOrphanStripePrices.php @@ -0,0 +1,156 @@ +option('dry-run'); + $archive = (bool) $this->option('archive'); + + // 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; + } + + $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; + } + + // 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. Once the sync has run, --archive stops selling what is still listed.", + }); + + 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/app/Console/Commands/SyncStripeCatalogue.php b/app/Console/Commands/SyncStripeCatalogue.php index f33270b..1876dd8 100644 --- a/app/Console/Commands/SyncStripeCatalogue.php +++ b/app/Console/Commands/SyncStripeCatalogue.php @@ -8,6 +8,8 @@ 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; use App\Services\Stripe\StripeClient; @@ -121,7 +123,32 @@ class SyncStripeCatalogue extends Command StripeCatalogueMode::record(); } + // Beide Seiten setzen hier an, und beide werden gebraucht. + // Die Modus-Sperre oben steht zuerst, weil sie VERWEIGERT: sie darf + // nicht hinter etwas laufen, das schon Zustand aufgebaut hat. Die + // Zähler darunter müssen nur vor der Anlegeschleife stehen, und das + // tun sie hier auch. + // 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; + // 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()); @@ -139,32 +166,32 @@ 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 = $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, - ['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]); } } @@ -180,22 +207,45 @@ class SyncStripeCatalogue extends Command $this->newLine(); + // $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); + + // 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. + $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. + // "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 or adopted. Run without --dry-run to do it." - : "{$created} object(s) created or adopted in Stripe."); + ? "{$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/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index 27c012d..5bdc8fb 100644 --- a/app/Providers/AppServiceProvider.php +++ b/app/Providers/AppServiceProvider.php @@ -13,6 +13,8 @@ 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; use App\Services\Dns\HetznerDnsClient; @@ -57,6 +59,17 @@ class AppServiceProvider extends ServiceProvider config('provisioning.pipelines', []), )); + // 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 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); + // 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/AdoptStripePrice.php b/app/Services/Billing/AdoptStripePrice.php index 41461d5..2b547a3 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; /** @@ -41,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) {} /** @@ -68,9 +79,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 +110,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; } @@ -104,7 +137,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); @@ -145,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 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/app/Services/Stripe/FakeStripeClient.php b/app/Services/Stripe/FakeStripeClient.php index 25d4ebc..8de2c57 100644 --- a/app/Services/Stripe/FakeStripeClient.php +++ b/app/Services/Stripe/FakeStripeClient.php @@ -16,10 +16,10 @@ class FakeStripeClient implements StripeClient { public bool $configured = true; - /** @var array */ + /** @var array */ public array $products = []; - /** @var array */ + /** @var array */ public array $prices = []; /** @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); @@ -218,15 +225,24 @@ 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 // 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, ]; @@ -258,15 +274,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,18 +286,57 @@ 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'], + // 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']), ]; } 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])) { - $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]; @@ -312,12 +358,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 94c6cf7..becdb79 100644 --- a/app/Services/Stripe/HttpStripeClient.php +++ b/app/Services/Stripe/HttpStripeClient.php @@ -177,43 +177,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, @@ -223,11 +204,66 @@ 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; } + 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 0ffed51..57195cb 100644 --- a/app/Services/Stripe/StripeClient.php +++ b/app/Services/Stripe/StripeClient.php @@ -125,42 +125,50 @@ 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; + /** + * 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/docs/handoffs/2026-07-30-real-run-handoff.md b/docs/handoffs/2026-07-30-real-run-handoff.md index 3364113..c341fc9 100644 --- a/docs/handoffs/2026-07-30-real-run-handoff.md +++ b/docs/handoffs/2026-07-30-real-run-handoff.md @@ -223,6 +223,23 @@ 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 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 + 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/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 new file mode 100644 index 0000000..d8b3bd1 --- /dev/null +++ b/docs/superpowers/specs/2026-07-30-stripe-adoption-followups-design.md @@ -0,0 +1,279 @@ +# 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. + +### 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 + +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 + **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. 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. + +**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 + +`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~~ — **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 + +- **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/Providers/AppServiceProvider.php` — beide Adoptions-Klassen als + 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`, + `StripeIdempotencyKeyTest.php` — angehängt +- `docs/superpowers/specs/2026-07-30-stripe-price-adoption-design.md` — §14 + abhaken, was hier erledigt wird 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/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 +
diff --git a/tests/Feature/Billing/StripeIdempotencyKeyTest.php b/tests/Feature/Billing/StripeIdempotencyKeyTest.php index efe72a0..e01f5d4 100644 --- a/tests/Feature/Billing/StripeIdempotencyKeyTest.php +++ b/tests/Feature/Billing/StripeIdempotencyKeyTest.php @@ -158,6 +158,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'], ]) @@ -173,73 +180,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 () { @@ -267,3 +241,92 @@ 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'); +}); + +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'); +}); diff --git a/tests/Feature/Billing/StripePriceAdoptionTest.php b/tests/Feature/Billing/StripePriceAdoptionTest.php index df68ab1..1a5075e 100644 --- a/tests/Feature/Billing/StripePriceAdoptionTest.php +++ b/tests/Feature/Billing/StripePriceAdoptionTest.php @@ -610,3 +610,100 @@ 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, per price, 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(); + + // 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 () { + // 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']); +}); diff --git a/tests/Feature/Billing/StripeProductAdoptionTest.php b/tests/Feature/Billing/StripeProductAdoptionTest.php new file mode 100644 index 0000000..9ea9275 --- /dev/null +++ b/tests/Feature/Billing/StripeProductAdoptionTest.php @@ -0,0 +1,184 @@ +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(); +}); + +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(); + $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'); +}); + +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'); +}); diff --git a/tests/Feature/Billing/SweepOrphanPricesTest.php b/tests/Feature/Billing/SweepOrphanPricesTest.php new file mode 100644 index 0000000..2b4a9fd --- /dev/null +++ b/tests/Feature/Billing/SweepOrphanPricesTest.php @@ -0,0 +1,123 @@ +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('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(); + + // 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([]); +}); + +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(); +});