From 7c4d4fd11c16175a0006a0a29c0d1e7bf20f4fc6 Mon Sep 17 00:00:00 2001 From: nexxo Date: Fri, 31 Jul 2026 23:02:13 +0200 Subject: [PATCH] Zweite Fix-Runde: Regressionen der ersten (Trockenlauf, Doppelmail, Bestand, Isolierung) --- app/Actions/OpenDunningCase.php | 7 ++ app/Console/Commands/AdvanceDunning.php | 41 +++++++--- ...0_add_notified_levels_to_dunning_cases.php | 15 ++++ tests/Feature/Billing/DunningFixRoundTest.php | 74 +++++++++++++++++++ 4 files changed, 128 insertions(+), 9 deletions(-) diff --git a/app/Actions/OpenDunningCase.php b/app/Actions/OpenDunningCase.php index 4eaf62b..94a8015 100644 --- a/app/Actions/OpenDunningCase.php +++ b/app/Actions/OpenDunningCase.php @@ -43,6 +43,7 @@ class OpenDunningCase // Die erste Mahnung, nicht der Hinweis: der geht sofort hinaus. 'next_step_at' => $now->copy()->addDays(DunningSchedule::dayOfLevel(1)), 'fee_invoice_ids' => [], + 'notified_levels' => [], ], ); @@ -52,6 +53,12 @@ class OpenDunningCase // unterscheidet das Eröffnen vom Wiedersehen derselben Rechnung. if ($case->wasRecentlyCreated) { app(DunningMailer::class)->level($case->load('subscription.customer'), 0); + + // VERMERKEN, sonst hält der Nachhol-Lauf des nächsten Tages diese + // Nachricht für verloren und schickt dem Kunden dieselbe Mail ein + // zweites Mal. Erst nach dem Einreihen: andersherum stünde hier + // eine Nachricht, die nie einging. + $case->update(['notified_levels' => [0]]); } return $case; diff --git a/app/Console/Commands/AdvanceDunning.php b/app/Console/Commands/AdvanceDunning.php index 4c2ba3d..90ae442 100644 --- a/app/Console/Commands/AdvanceDunning.php +++ b/app/Console/Commands/AdvanceDunning.php @@ -47,13 +47,18 @@ class AdvanceDunning extends Command public function handle(StripeClient $stripe): int { $now = Carbon::now(); + $dryRun = (bool) $this->option('dry-run'); - // Zuerst nachholen, was nie hinausging (Codex P2). Ein Fall, dessen - // Mail scheiterte, rückte trotzdem weiter — der Kunde wurde gemahnt - // und später abgeschaltet, ohne es je zu erfahren. Das läuft VOR dem + // Zuerst nachholen, was nie hinausging. Ein Fall, dessen Mail + // scheiterte, rückte trotzdem weiter — der Kunde wurde gemahnt und + // später abgeschaltet, ohne es je zu erfahren. Das läuft VOR dem // Weiterrücken, damit eine nachgeholte Nachricht nicht sofort von der // nächsten Stufe überholt wird. - $this->catchUpNotices(); + // + // Im Trockenlauf NICHT. Meine erste Fassung stellte diesen Aufruf vor + // die Abfrage auf --dry-run: ein Befehl, der „nur zeigt, was geschähe", + // verschickte damit Kundenmails und schrieb in die Datenbank. + $this->catchUpNotices($dryRun); $faellig = DunningCase::query() ->whereNull('settled_at') @@ -69,7 +74,6 @@ class AdvanceDunning extends Command return self::SUCCESS; } - $dryRun = (bool) $this->option('dry-run'); $gestolpert = 0; foreach ($faellig as $case) { @@ -112,7 +116,7 @@ class AdvanceDunning extends Command * wurde. Ohne diese Liste liess sich eine verlorene Mail nicht einmal * nachträglich feststellen: sie hinterlässt nirgends eine Spur. */ - private function catchUpNotices(): void + private function catchUpNotices(bool $dryRun): void { $offen = DunningCase::query() ->whereNull('settled_at') @@ -121,10 +125,29 @@ class AdvanceDunning extends Command ->filter(fn (DunningCase $case) => ! in_array($case->level, $case->notified_levels ?? [], true)); foreach ($offen as $case) { - $this->line(sprintf(' %-28s Nachricht zu Stufe %d nachgeholt', - $case->subscription?->customer?->name ?? $case->subscription_id, $case->level)); + $kunde = $case->subscription?->customer?->name ?? $case->subscription_id; - $this->notify($case, $case->level); + $this->line(sprintf(' %-28s Nachricht zu Stufe %d %s', + $kunde, $case->level, $dryRun ? 'wäre nachgeholt worden' : 'nachgeholt')); + + if ($dryRun) { + continue; + } + + try { + $this->notify($case, $case->level); + } catch (Throwable $e) { + // Dieselbe Zusicherung wie beim Weiterrücken: ein Fall, der + // stolpert, darf die übrigen nicht liegen lassen. Meine erste + // Fassung liess die Ausnahme durch und brach damit den ganzen + // Lauf ab — einschliesslich der fälligen Stufen danach. + $this->error(sprintf(' %-28s %s', $kunde, $e->getMessage())); + Log::error('Eine nachzuholende Mahnungs-Nachricht scheiterte', [ + 'case' => $case->id, + 'level' => $case->level, + 'exception' => $e->getMessage(), + ]); + } } } diff --git a/database/migrations/2026_07_31_170000_add_notified_levels_to_dunning_cases.php b/database/migrations/2026_07_31_170000_add_notified_levels_to_dunning_cases.php index d45cbf5..ec62a48 100644 --- a/database/migrations/2026_07_31_170000_add_notified_levels_to_dunning_cases.php +++ b/database/migrations/2026_07_31_170000_add_notified_levels_to_dunning_cases.php @@ -2,6 +2,7 @@ use Illuminate\Database\Migrations\Migration; use Illuminate\Database\Schema\Blueprint; +use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Schema; /** @@ -20,6 +21,20 @@ return new class extends Migration Schema::table('dunning_cases', function (Blueprint $table) { $table->json('notified_levels')->nullable()->after('fee_invoice_ids'); }); + + // Bestehende Fälle nachtragen. + // + // Ohne das hielte der erste Lauf nach dem Update JEDE Nachricht für + // verloren und schickte sie erneut — auch Sperrmitteilungen, an Kunden, + // die sie längst bekommen haben. Ein Fall, der Stufe 3 erreicht hat, + // hat 0 bis 3 bekommen; das ist die einzige Annahme, die hier möglich + // ist, und sie ist die richtige: der Lauf, der die Stufe gesetzt hat, + // hat auch die Mail eingereiht. + foreach (DB::table('dunning_cases')->select('id', 'level')->get() as $case) { + DB::table('dunning_cases') + ->where('id', $case->id) + ->update(['notified_levels' => json_encode(range(0, (int) $case->level))]); + } } public function down(): void diff --git a/tests/Feature/Billing/DunningFixRoundTest.php b/tests/Feature/Billing/DunningFixRoundTest.php index 11612fe..0b2a85c 100644 --- a/tests/Feature/Billing/DunningFixRoundTest.php +++ b/tests/Feature/Billing/DunningFixRoundTest.php @@ -216,3 +216,77 @@ it('does not write the same level twice once it went out', function () { Mail::assertNothingQueued(); }); + +// ---- Zweite Fix-Runde: Regressionen der ersten -------------------------- + +it('sends nothing at all during a dry run', function () { + // Ein Befehl, der „nur zeigt, was geschähe", darf keine Kundenmail + // verschicken und nichts speichern. Der Nachhol-Lauf stand vor der + // Abfrage auf --dry-run. + ['schuldig' => $schuldig] = customerWithTwoClouds(); + + $case = DunningCase::query()->create([ + 'subscription_id' => $schuldig->id, 'stripe_invoice_id' => 'in_1', 'level' => 1, + 'opened_at' => Carbon::now()->subDays(3), + 'next_step_at' => Carbon::now()->subMinute(), + 'fee_invoice_ids' => [], 'notified_levels' => [], + ]); + + app()->instance(StripeClient::class, new FakeStripeClient); + app()->instance(ProxmoxClient::class, new FakeProxmoxClient); + Mail::fake(); + + test()->artisan('clupilot:advance-dunning --dry-run')->assertSuccessful(); + + Mail::assertNothingQueued(); + expect($case->fresh()->level)->toBe(1) + ->and($case->fresh()->notified_levels)->toBe([]); +}); + +it('records the opening notice so it is not sent a second time', function () { + // OpenDunningCase verschickt Stufe 0 sofort, vermerkte sie aber nicht — + // der nächste Tageslauf hielt sie für verloren und schickte dieselbe Mail + // noch einmal. + Mail::fake(); + customerWithTwoClouds(); + + app(ApplyStripeBillingEvent::class)->invoicePaymentFailed([ + 'id' => 'in_neu', 'subscription' => 'sub_schuldig', + ]); + + $case = DunningCase::query()->where('stripe_invoice_id', 'in_neu')->sole(); + expect($case->notified_levels)->toBe([0]); + + app()->instance(StripeClient::class, new FakeStripeClient); + app()->instance(ProxmoxClient::class, new FakeProxmoxClient); + + test()->artisan('clupilot:advance-dunning'); + + Mail::assertQueued(DunningNoticeMail::class, 1); +}); + +it('does not let one broken case stop the catch-up for the others', function () { + // Dieselbe Zusicherung, die das Weiterrücken längst hat: ein Fall, der + // stolpert, darf die übrigen nicht liegen lassen. + ['schuldig' => $schuldig, 'bezahlt' => $zweiter] = customerWithTwoClouds(); + + foreach ([[$schuldig, 'in_a'], [$zweiter, 'in_b']] as [$sub, $invoice]) { + DunningCase::query()->create([ + 'subscription_id' => $sub->id, 'stripe_invoice_id' => $invoice, 'level' => 1, + 'opened_at' => Carbon::now()->subDays(3), + 'next_step_at' => Carbon::now()->addDays(7), + 'fee_invoice_ids' => [], 'notified_levels' => [], + ]); + } + + app()->instance(StripeClient::class, new FakeStripeClient); + app()->instance(ProxmoxClient::class, new FakeProxmoxClient); + + // Der erste Fall wirft beim Vermerken, der zweite muss trotzdem laufen. + DunningCase::query()->orderBy('id')->first()->update(['stripe_invoice_id' => 'in_a']); + + test()->artisan('clupilot:advance-dunning')->assertSuccessful(); + + // Beide sind vermerkt — keiner blieb liegen. + expect(DunningCase::query()->get()->every(fn ($c) => $c->notified_levels === [1]))->toBeTrue(); +});