diff --git a/app/Console/Commands/Concerns/FindsUsersWithLegacyEncryption.php b/app/Console/Commands/Concerns/FindsUsersWithLegacyEncryption.php index 4aa40eac..53edaed2 100644 --- a/app/Console/Commands/Concerns/FindsUsersWithLegacyEncryption.php +++ b/app/Console/Commands/Concerns/FindsUsersWithLegacyEncryption.php @@ -5,7 +5,6 @@ namespace App\Console\Commands\Concerns; use App\Models\User; use Illuminate\Database\Eloquent\Builder; use Illuminate\Support\Collection; -use Laravel\Cashier\Subscription; trait FindsUsersWithLegacyEncryption { @@ -34,32 +33,17 @@ trait FindsUsersWithLegacyEncryption * Drop users who are still being billed. These accounts must never be emailed a * deletion warning nor deleted while a subscription or trial is active. * - * Unlike {@see User::hasActiveSubscriptionOrTrial()}, this check is intentionally - * independent of the `subscriptions.enabled` feature flag: that flag controls whether - * the paywall is enforced, not whether real Stripe subscriptions exist. A destructive - * command must never delete a paying customer just because the flag is off in the - * runtime that happens to execute it. + * {@see User::isBilled()} is used instead of {@see User::hasActiveSubscriptionOrTrial()} + * because it is independent of the `subscriptions.enabled` feature flag: that flag + * controls whether the paywall is enforced, not whether real Stripe subscriptions + * exist. A destructive command must never delete a paying customer just because the + * flag is off in the runtime that happens to execute it. * * @param Collection $users * @return Collection */ protected function excludeBilledUsers(Collection $users): Collection { - return $users->reject(fn (User $user): bool => $this->isBilled($user))->values(); - } - - /** - * Whether the user is on a generic trial or holds a subscription that is still - * valid and not merely coasting through its grace period (it will not renew). - */ - private function isBilled(User $user): bool - { - if ($user->onGenericTrial()) { - return true; - } - - return $user->subscriptions->contains( - fn (Subscription $subscription): bool => $subscription->valid() && ! $subscription->onGracePeriod() - ); + return $users->reject(fn (User $user): bool => $user->isBilled())->values(); } } diff --git a/app/Models/User.php b/app/Models/User.php index 27d4a66b..d8379cb9 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -409,10 +409,21 @@ class User extends Authenticatable implements HasLocalePreference, MustVerifyEma */ public function hasActiveSubscriptionOrTrial(): bool { - if (! config('subscriptions.enabled')) { - return false; - } + return config('subscriptions.enabled') && $this->isBilled(); + } + /** + * Whether the user is on a generic trial or holds a subscription that is still + * valid and not merely coasting through its grace period (it will not renew). + * + * Unlike hasActiveSubscriptionOrTrial() this is independent of the + * `subscriptions.enabled` flag: that flag controls whether the paywall is + * enforced, not whether real Stripe subscriptions exist. Destructive flows that + * must never touch a paying customer (the encryption cleanup commands) rely on + * this flag-independent check. + */ + public function isBilled(): bool + { if ($this->onGenericTrial()) { return true; } diff --git a/tests/Feature/Console/DeleteEncryptedDataAccountsCommandTest.php b/tests/Feature/Console/DeleteEncryptedDataAccountsCommandTest.php index ab5fcc99..019e4e28 100644 --- a/tests/Feature/Console/DeleteEncryptedDataAccountsCommandTest.php +++ b/tests/Feature/Console/DeleteEncryptedDataAccountsCommandTest.php @@ -45,6 +45,59 @@ test('it never deletes a subscribed user even when the subscriptions flag is off expect(User::whereKey($subscribed->id)->exists())->toBeTrue(); }); +test('it never deletes a trialing subscriber', function () { + config(['subscriptions.enabled' => false]); + + $trialing = User::factory()->create(['last_active_at' => now()->subDays(30)]); + Transaction::factory()->for($trialing)->create(); + $trialing->subscriptions()->create([ + 'type' => 'default', + 'stripe_id' => 'sub_trial123', + 'stripe_status' => 'trialing', + 'stripe_price' => 'price_test123', + 'trial_ends_at' => now()->addDays(5), + ]); + + artisan('encryption:delete-accounts', ['--force' => true])->assertSuccessful(); + + expect(User::whereKey($trialing->id)->exists())->toBeTrue(); +}); + +test('it never deletes a past-due subscriber', function () { + config(['subscriptions.enabled' => false]); + + $pastDue = User::factory()->create(['last_active_at' => now()->subDays(30)]); + Transaction::factory()->for($pastDue)->create(); + $pastDue->subscriptions()->create([ + 'type' => 'default', + 'stripe_id' => 'sub_pastdue123', + 'stripe_status' => 'past_due', + 'stripe_price' => 'price_test123', + ]); + + artisan('encryption:delete-accounts', ['--force' => true])->assertSuccessful(); + + expect(User::whereKey($pastDue->id)->exists())->toBeTrue(); +}); + +test('it deletes a subscriber who is only in the cancellation grace period', function () { + config(['subscriptions.enabled' => false]); + + $grace = User::factory()->create(['last_active_at' => now()->subDays(30)]); + Transaction::factory()->for($grace)->create(); + $grace->subscriptions()->create([ + 'type' => 'default', + 'stripe_id' => 'sub_grace123', + 'stripe_status' => 'active', + 'stripe_price' => 'price_test123', + 'ends_at' => now()->addDays(5), + ]); + + artisan('encryption:delete-accounts', ['--force' => true])->assertSuccessful(); + + expect(User::whereKey($grace->id)->exists())->toBeFalse(); +}); + test('it never deletes the demo account', function () { $demo = User::factory()->create([ 'email' => config('app.demo.email'),