diff --git a/app/Modules/Notifications/Http/Controllers/NotificationPreferencesController.php b/app/Modules/Notifications/Http/Controllers/NotificationPreferencesController.php index 90822abc..60dedee4 100644 --- a/app/Modules/Notifications/Http/Controllers/NotificationPreferencesController.php +++ b/app/Modules/Notifications/Http/Controllers/NotificationPreferencesController.php @@ -7,9 +7,11 @@ namespace App\Modules\Notifications\Http\Controllers; use App\Http\Controllers\Controller; use App\Modules\Notifications\NotificationPreference; use App\Modules\Notifications\NotificationPreferences; +use App\Modules\Notifications\NotificationTypeDefinition; use App\Modules\Notifications\NotificationTypeRegistry; use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; +use Illuminate\Validation\Rule; use Inertia\Inertia; use Inertia\Response; @@ -30,21 +32,12 @@ class NotificationPreferencesController extends Controller $user = $request->user(); assert($user !== null); - // Only types that can email at all have anything to opt in or out - // of — a pure in-app type has no toggle to show. Either route - // counts: Notifier sending a mail class directly, or the digest - // buffering and sending one. - $emailable = array_values(array_filter( - $this->types->all(), - fn ($type) => $type->mailNotification !== null || $type->digestMail !== null, - )); - return Inertia::render('settings/notifications', [ - 'types' => array_map(fn ($type) => [ + 'types' => array_map(fn (NotificationTypeDefinition $type): array => [ 'key' => $type->key, 'label' => $type->label, 'email_enabled' => $this->preferences->emailEnabledFor($user, $type), - ], $emailable), + ], $this->emailable()), ]); } @@ -55,7 +48,11 @@ class NotificationPreferencesController extends Controller $validated = $request->validate([ 'preferences' => ['required', 'array'], - 'preferences.*.type' => ['required', 'string'], + // Against the registry, not merely "a string": a preference row + // for a type nothing can send is a row that will never be read + // again, and the screen only ever offers back what edit() gave + // it. + 'preferences.*.type' => ['required', 'string', Rule::in($this->emailableKeys())], 'preferences.*.email_enabled' => ['required', 'boolean'], ]); @@ -68,4 +65,31 @@ class NotificationPreferencesController extends Controller return back(); } + + /** + * Only types that can email at all have anything to opt in or out of — + * a pure in-app type has no toggle to show. Either route counts: + * Notifier sending a mail class directly, or the digest buffering and + * sending one. + * + * Shared by both halves on purpose, so what the screen offers and what + * it accepts back cannot drift apart. + * + * @return list + */ + private function emailable(): array + { + return array_values(array_filter( + $this->types->all(), + fn (NotificationTypeDefinition $type): bool => $type->mailNotification !== null || $type->digestMail !== null, + )); + } + + /** + * @return list + */ + private function emailableKeys(): array + { + return array_map(fn (NotificationTypeDefinition $type): string => $type->key, $this->emailable()); + } } diff --git a/tests/Feature/Notifications/NotificationPreferencesTest.php b/tests/Feature/Notifications/NotificationPreferencesTest.php index 747c2de6..caaaf4dd 100644 --- a/tests/Feature/Notifications/NotificationPreferencesTest.php +++ b/tests/Feature/Notifications/NotificationPreferencesTest.php @@ -59,6 +59,46 @@ test('updating preferences via the settings page persists a row per type', funct expect($row->email_enabled)->toBeFalse(); }); +test('a preference for a type nothing has registered is refused', function () { + $client = User::factory()->client()->create(); + + $this->actingAs($client)->put('/settings/notifications', [ + 'preferences' => [ + ['type' => 'not.a.real.type', 'email_enabled' => false], + ], + ])->assertInvalid(['preferences.0.type']); + + expect(NotificationPreference::query()->where('user_id', $client->id)->exists())->toBeFalse(); +}); + +test('a preference for a registered type that cannot email is refused', function () { + $client = User::factory()->client()->create(); + + // A real key, but one the screen never offers a toggle for — a row for + // it could never change what anybody receives. + $this->actingAs($client)->put('/settings/notifications', [ + 'preferences' => [ + ['type' => 'client_uploaded', 'email_enabled' => true], + ], + ])->assertInvalid(['preferences.0.type']); + + expect(NotificationPreference::query()->where('user_id', $client->id)->exists())->toBeFalse(); +}); + +test('one bad key rejects the whole submission, leaving no half-applied state', function () { + $client = User::factory()->client()->create(); + + $this->actingAs($client)->put('/settings/notifications', [ + 'preferences' => [ + ['type' => 'group.membership_approved', 'email_enabled' => false], + ['type' => 'not.a.real.type', 'email_enabled' => false], + ], + ])->assertInvalid(['preferences.1.type']); + + // Validation runs before the loop, so the good row is not written either. + expect(NotificationPreference::query()->where('user_id', $client->id)->exists())->toBeFalse(); +}); + test('the master switch off suppresses email regardless of an explicit per-user opt-in', function () { Notification::fake(); app(Settings::class)->set(Setting::EmailNotificationsEnabled, false);