Merge pull request #1689 from denkfabrik-li/fix/notification-preference-types

Accept only notification types that can actually notify

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<NotificationTypeDefinition>
+     */
+    private function emailable(): array
+    {
+        return array_values(array_filter(
+            $this->types->all(),
+            fn (NotificationTypeDefinition $type): bool => $type->mailNotification !== null || $type->digestMail !== null,
+        ));
+    }
+
+    /**
+     * @return list<string>
+     */
+    private function emailableKeys(): array
+    {
+        return array_map(fn (NotificationTypeDefinition $type): string => $type->key, $this->emailable());
+    }
 }

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);