Merge pull request #1722 from denkfabrik-li/fix/portal-dashboard-visible-files

DashboardController::clientDashboard() built its own whereHas('assignments') query instead of using File::scopeVisibleToClient -- "the single source of truth for client file access", as that scope's own docblock puts it. The copy reproduced the assignment half and stopped there, so the page disagreed with the portal it introduces, in both directions. Over: the scope ends in notExpired(), so an expired file was gone from /my-files and refused on download while the dashboard went on counting it and printing its name. Under: a file in a folder shared with the client, a file the client uploaded through the portal themselves, and a revision -- which owns no assignment row and inherits its original's recipients through SharingIdentity -- were all missing from the count and the list.

The hand-rolled query is gone and the scope is used, one query object cloned for the count exactly as before. groups_count and the storage figures are untouched: they answer different questions and have their own tests.

Verified before merging: 19 passed on the trial-merge, 2 failed / 17 passed with app/ reset. The existing "clients get the portal dashboard with their own numbers" test is unchanged and green either way, so a directly assigned live file counts as it always did. PHPStan level 8 clean on the changed file.

Reported and fixed by @denkfabrik-li.

app/Modules/Audit/Http/Controllers/DashboardController.php

@@ -497,23 +497,25 @@ class DashboardController extends Controller
 
     private function clientDashboard(User $client): Response
     {
-        $assignedFiles = File::query()->whereHas('assignments', function ($query) use ($client): void {
-            $query->where(function ($direct) use ($client): void {
-                $direct->where('assignable_type', User::class)->where('assignable_id', $client->id);
-            })->orWhere(function ($viaGroup) use ($client): void {
-                $viaGroup->where('assignable_type', Group::class)
-                    ->whereIn('assignable_id', $client->memberOfGroups()->pluck('groups.id'));
-            });
-        });
+        // File::scopeVisibleToClient is the single source of truth for
+        // client file access, and this page has to agree with the portal it
+        // introduces. Restating the assignment half here made it disagree
+        // in both directions: it counted expired files, which the scope
+        // ends by excluding and /my-files therefore never shows, and it
+        // missed everything that reaches a client another way — a file in a
+        // folder shared with them, their own portal upload, and a revision,
+        // which owns no assignment row and inherits its original's
+        // recipients.
+        $visibleFiles = File::query()->visibleToClient($client);
 
         return Inertia::render('portal/dashboard', [
-            'files_count' => (clone $assignedFiles)->count(),
+            'files_count' => (clone $visibleFiles)->count(),
             'groups_count' => $client->memberOfGroups()->where('public', true)->count(),
             'storage' => [
                 'used_bytes' => $this->storageUsage->usedBytes($client),
                 'quota_bytes' => $this->storageUsage->quotaBytes($client) ?: null,
             ],
-            'latest_files' => $assignedFiles->orderByDesc('created_at')->limit(5)->get()
+            'latest_files' => $visibleFiles->orderByDesc('created_at')->limit(5)->get()
                 ->map(fn (File $file): array => [
                     'id' => $file->id,
                     'name' => $file->name,

tests/Feature/Audit/DashboardTest.php

@@ -7,6 +7,7 @@ use App\Modules\Audit\Action;
 use App\Modules\Audit\ActivityLog;
 use App\Modules\Audit\ActivityOrigin;
 use App\Modules\Files\Models\File;
+use App\Modules\Files\Models\FolderAssignment;
 use App\Modules\Files\Models\ShareLink;
 use App\Modules\Groups\Models\Group;
 use App\Modules\Identity\Models\Role;
@@ -317,6 +318,55 @@ test('clients get the portal dashboard with their own numbers', function () {
     expect((string) $response->getContent())->not->toContain('Hidden');
 });
 
+test('the portal dashboard does not count a file the client can no longer open', function () {
+    // File::scopeVisibleToClient ends in notExpired(), so /my-files stops
+    // listing an expired file and the download is refused. The dashboard
+    // restated the assignment half of that scope without its ending, and
+    // went on offering the file's name.
+    $client = User::factory()->client()->create();
+    $file = File::factory()->create([
+        'uploaded_by' => $this->admin->id,
+        'name' => 'old-offer',
+        'expires_at' => now()->subDay(),
+    ]);
+    shareFileWith($file, $client);
+
+    $this->actingAs($client)->get("/files/{$file->id}/download")->assertForbidden();
+
+    $this->actingAs($client)->get('/dashboard')->assertInertia(
+        fn (AssertableInertia $page) => $page
+            ->where('files_count', 0)
+            ->has('latest_files', 0),
+    );
+});
+
+test('the portal dashboard counts a file that reaches the client through a folder', function () {
+    // The other direction of the same substitution: a shared folder and a
+    // client's own upload are two of the three ways into
+    // scopeVisibleToClient that the hand-rolled query left out, so the
+    // number was under the truth as well as over it.
+    $client = User::factory()->client()->create();
+    $folder = makeFolder('Shared with them');
+    FolderAssignment::query()->create([
+        'folder_id' => $folder->id,
+        'assignable_type' => $client->getMorphClass(),
+        'assignable_id' => $client->id,
+    ]);
+
+    File::factory()->create([
+        'uploaded_by' => $this->admin->id,
+        'name' => 'in-the-folder',
+        'folder_id' => $folder->id,
+    ]);
+    File::factory()->create(['uploaded_by' => $client->id, 'name' => 'their-own-upload']);
+
+    $this->actingAs($client)->get('/dashboard')->assertInertia(
+        fn (AssertableInertia $page) => $page
+            ->where('files_count', 2)
+            ->has('latest_files', 2),
+    );
+});
+
 test('the recent-activity widget carries origin so an actorless entry is not mislabelled', function () {
     // An anonymous entry (a public/share-link download) and a system entry
     // are both actor_name null; only `origin` separates "Anonymous" from