diff --git a/app/Actions/UpdateNotificationsReadAt.php b/app/Actions/UpdateNotificationsReadAt.php index f83e28531..540a0fdd2 100644 --- a/app/Actions/UpdateNotificationsReadAt.php +++ b/app/Actions/UpdateNotificationsReadAt.php @@ -9,13 +9,14 @@ final readonly class UpdateNotificationsReadAt { - public function handle(User $user): CarbonImmutable + public function handle(User $user, string $type): CarbonImmutable { $readAt = CarbonImmutable::now()->startOfSecond(); $user->notifications() ->relevant() ->unread() + ->ofType($type) ->update(['read_at' => $readAt]); return $readAt; diff --git a/app/Http/Controllers/Api/NotificationController.php b/app/Http/Controllers/Api/NotificationController.php index 264062eee..cd40d0a06 100644 --- a/app/Http/Controllers/Api/NotificationController.php +++ b/app/Http/Controllers/Api/NotificationController.php @@ -19,14 +19,15 @@ final readonly class NotificationController { /** - * The notifications grouped and filtered by ?type= like the web does it. As on the web, listing them marks every - * notification as read, whatever the filter, and read_at says when, so the ones read since then are the new ones. - * A commit comes with its author and the commit it replies to, when the user may see it. + * The notifications grouped and filtered by ?type= like the web does it. As on the web, listing them marks the + * notifications the filter shows as read, so the rest is still new under another filter, and read_at says when, so + * the ones read since then are the new ones. A commit comes with its author and the commit it replies to, when the + * user may see it. */ public function index(ListNotificationsRequest $request, #[CurrentUser] User $user, NotificationGroupsQuery $query, UpdateNotificationsReadAt $action): AnonymousResourceCollection { - $readAt = $action->handle($user); $type = $request->type($user); + $readAt = $action->handle($user, $type); $notifications = $query->paginate($user, $type, $request->through()); diff --git a/app/Http/Controllers/NotificationController.php b/app/Http/Controllers/NotificationController.php index deef67fe7..228b32789 100644 --- a/app/Http/Controllers/NotificationController.php +++ b/app/Http/Controllers/NotificationController.php @@ -17,8 +17,8 @@ { public function index(ListNotificationsRequest $request, #[CurrentUser] User $user, NotificationGroupsQuery $query, UpdateNotificationsReadAt $action): Response { - $readAt = $request->loadsNotifications() ? $action->handle($user) : null; $type = $request->type($user); + $readAt = $request->loadsNotifications() ? $action->handle($user, $type) : null; return Inertia::render('notification/index', [ 'notifications' => Inertia::scroll(fn (): CursorPaginator => $query->withStarredCommitImages($query->withQuotedCommits($user, $query->paginate($user, $type, $request->through()))))->matchOn('data.group_id'), diff --git a/app/Http/Requests/ListNotificationsRequest.php b/app/Http/Requests/ListNotificationsRequest.php index d250afe9a..71343c7af 100644 --- a/app/Http/Requests/ListNotificationsRequest.php +++ b/app/Http/Requests/ListNotificationsRequest.php @@ -15,9 +15,14 @@ final class ListNotificationsRequest extends FormRequest { public const string TYPE_RULE = 'regex:/\A(?:all|none|(?:replies|quotes|stars|mentions|follows|moderation)(?:,(?:replies|quotes|stars|mentions|follows|moderation))*)\z/'; + /** + * The filter the browser keeps, including one this response is about to set, so the unread badge counts under the + * same filter the inbox just marked read. + */ public static function rememberedType(Request $request, User $user): string { - $type = $request->cookie('notification_types_'.$user->id, 'all'); + $cookie = 'notification_types_'.$user->id; + $type = Cookie::queued($cookie)?->getValue() ?? $request->cookie($cookie, 'all'); return is_string($type) && preg_match(mb_substr(self::TYPE_RULE, 6), $type) === 1 ? $type : 'all'; } diff --git a/app/Queries/NotificationBuilder.php b/app/Queries/NotificationBuilder.php index b2abee21b..a810cdacc 100644 --- a/app/Queries/NotificationBuilder.php +++ b/app/Queries/NotificationBuilder.php @@ -42,6 +42,23 @@ public function unread(): self return $this->whereNull('read_at'); } + /** + * Narrows to the types the inbox filter shows: a comma-separated list of its categories, 'all' for every type, and + * 'none' for no type at all. + */ + public function ofType(string $type): self + { + return $this->when($type !== 'all', fn (self $query): self => $query->whereIn('type', array_merge(...array_map(fn (string $category): array => match ($category) { + 'replies' => [NotificationType::Replied], + 'quotes' => [NotificationType::Quoted], + 'stars' => [NotificationType::Starred], + 'mentions' => [NotificationType::Mentioned], + 'follows' => [NotificationType::Followed], + 'moderation' => [NotificationType::Approved, NotificationType::Rejected], + default => [], + }, explode(',', $type))))); + } + /** * Leaves out what actors the recipient mutes did, and anything from across a block. Moderation * notifications are their own actor, and nobody can mute or block themselves. diff --git a/app/Queries/NotificationGroupsQuery.php b/app/Queries/NotificationGroupsQuery.php index 927458fb8..a0edf1280 100644 --- a/app/Queries/NotificationGroupsQuery.php +++ b/app/Queries/NotificationGroupsQuery.php @@ -90,7 +90,7 @@ public function unreadCount(User $user, string $type = 'all'): int return 0; } - $oldestUnread = $this->visible($user)->unread()->oldest()->first(); + $oldestUnread = $this->visible($user)->unread()->ofType($type)->oldest()->first(); return $oldestUnread instanceof Notification ? $this->groupsSince($user, $oldestUnread->created_at, $type)->where('group_rank', 1)->where('unread_count', '>', 0)->count() @@ -132,15 +132,7 @@ private function group(NotificationBuilder $visible, string $type = 'all', ?Noti 'identified.group_id', fn (QueryBuilder $query): QueryBuilder => $query->from('identified')->where('id', $member->id)->select('group_id'), )) - ->when($type !== 'all', fn (NotificationBuilder $query): NotificationBuilder => $query->whereIn('type', array_merge(...array_map(fn (string $category): array => match ($category) { - 'replies' => ['replied'], - 'quotes' => ['quoted'], - 'stars' => ['starred'], - 'mentions' => ['mentioned'], - 'follows' => ['followed'], - 'moderation' => ['approved', 'rejected'], - default => [], - }, explode(',', $type))))); + ->ofType($type); $gap = match (DB::connection($visible->getModel()->getConnectionName())->getDriverName()) { 'mysql', 'mariadb' => 'TIMESTAMPDIFF(SECOND, previous_match_at, created_at)', diff --git a/tests/Feature/Controllers/Api/NotificationControllerTest.php b/tests/Feature/Controllers/Api/NotificationControllerTest.php index f1b015867..fac8e04c0 100644 --- a/tests/Feature/Controllers/Api/NotificationControllerTest.php +++ b/tests/Feature/Controllers/Api/NotificationControllerTest.php @@ -273,7 +273,7 @@ 'moderation' => ['moderation', ['rejected', 'approved']], ]); -it('marks every notification as read whatever the filter, as the web does', function (): void { +it('marks only what the filter shows as read, as the web does', function (): void { $followed = Notification::factory()->for($this->user, 'recipient')->create(); $replied = Notification::factory()->for($this->user, 'recipient')->replied()->create(); @@ -281,7 +281,8 @@ ->assertOk() ->assertJsonPath('data.*.id', [$followed->id]); - expect($replied->refresh()->read_at)->not->toBeNull(); + expect($followed->refresh()->read_at)->not->toBeNull() + ->and($replied->refresh()->read_at)->toBeNull(); }); it('validates the filter before marking anything as read', function (array $query, string $field): void { diff --git a/tests/Feature/NotificationInboxTest.php b/tests/Feature/NotificationInboxTest.php index 40d0b9284..7212e0eb8 100644 --- a/tests/Feature/NotificationInboxTest.php +++ b/tests/Feature/NotificationInboxTest.php @@ -77,6 +77,24 @@ 'partially invalid saved filter' => ['replies,invalid', 4], ]); +it('marks only what the filter shows as read, so the rest is still new under another filter', function (): void { + $user = User::factory()->create(); + $reply = Notification::factory()->for($user, 'recipient')->replied()->create(); + $follow = Notification::factory()->for($user, 'recipient')->create(); + + $this->actingAs($user)->get(route('notification.index', ['type' => 'replies']))->assertOk(); + + expect($reply->refresh()->read_at)->not->toBeNull() + ->and($follow->refresh()->read_at)->toBeNull(); + + $this->travel(1)->minute(); + + $this->get(route('notification.index', ['type' => 'all']))->assertOk() + ->assertInertia(fn ($page) => $page->where('readAt', $follow->refresh()->read_at?->toJSON())); + + expect($reply->refresh()->read_at)->toBeLessThan($follow->read_at); +}); + it('refreshes the entire loaded inbox window without losing older pages', function (): void { $user = User::factory()->create(); $notices = collect(); @@ -147,7 +165,7 @@ ->assertJsonPath('props.auth.unreadNotificationsCount', 0); expect($latest->refresh()->read_at)->not->toBeNull() - ->and($follow->refresh()->read_at)->not->toBeNull() + ->and($follow->refresh()->read_at)->toBeNull() ->and($other->refresh()->read_at)->toBeNull(); }); @@ -371,7 +389,7 @@ expect($one->refresh()->read_at->equalTo($readAt))->toBeTrue(); }); -it('marks all relevant activity read beyond the first page and selected type', function (): void { +it('marks all relevant activity of the selected type read beyond the first page', function (): void { $user = User::factory()->create(); $root = Commit::factory()->create(); $events = collect(); @@ -391,8 +409,8 @@ $this->actingAs($user)->get(route('notification.index', ['type' => 'replies'])) ->assertOk()->assertInertia(fn ($page) => $page->has('notifications.data', 20) ->where('auth.unreadNotificationsCount', 0)); - expect($user->notifications()->relevant()->unread()->count())->toBe(0) - ->and($follow->refresh()->read_at)->not->toBeNull() + expect($user->notifications()->relevant()->unread()->ofType('replies')->count())->toBe(0) + ->and($follow->refresh()->read_at)->toBeNull() ->and($events->last()->refresh()->read_at)->not->toBeNull() ->and($hidden->refresh()->read_at)->toBeNull() ->and($other->refresh()->read_at)->toBeNull(); @@ -410,6 +428,9 @@ expect($latest->refresh()->read_at)->toBeNull(); $this->get(route('notification.index', ['type' => 'follows']))->assertInertia(fn ($page) => $page ->where('auth.unreadNotificationsCount', 0)); + expect($latest->refresh()->read_at)->toBeNull(); + $this->get(route('notification.index', ['type' => 'all']))->assertInertia(fn ($page) => $page + ->where('auth.unreadNotificationsCount', 0)); expect($latest->refresh()->read_at)->not->toBeNull(); }); diff --git a/tests/Unit/Actions/UpdateNotificationsReadAtTest.php b/tests/Unit/Actions/UpdateNotificationsReadAtTest.php index 07729101d..db3bce60f 100644 --- a/tests/Unit/Actions/UpdateNotificationsReadAtTest.php +++ b/tests/Unit/Actions/UpdateNotificationsReadAtTest.php @@ -13,7 +13,7 @@ $action = resolve(UpdateNotificationsReadAt::class); - $action->handle($user); + $action->handle($user, 'all'); foreach ($notifications as $notification) { expect($notification->refresh()->read_at)->not->toBeNull(); @@ -25,7 +25,7 @@ it('returns the moment it marked notifications read', function (): void { $notification = Notification::factory()->create(); - $readAt = resolve(UpdateNotificationsReadAt::class)->handle($notification->recipient); + $readAt = resolve(UpdateNotificationsReadAt::class)->handle($notification->recipient, 'all'); expect($readAt->toJSON())->toBe(now()->startOfSecond()->toJSON()) ->and($notification->refresh()->read_at->equalTo($readAt))->toBeTrue(); @@ -36,7 +36,7 @@ $action = resolve(UpdateNotificationsReadAt::class); - $action->handle($notification->recipient); + $action->handle($notification->recipient, 'all'); expect($notification->refresh()->read_at->toDateTimeString())->toBe(now()->subDay()->toDateTimeString()); }); @@ -49,7 +49,7 @@ $otherCommit = Notification::factory()->for($starred->recipient, 'recipient')->starred()->create(); $follow = Notification::factory()->for($starred->recipient, 'recipient')->create(); - resolve(UpdateNotificationsReadAt::class)->handle($starred->recipient); + resolve(UpdateNotificationsReadAt::class)->handle($starred->recipient, 'all'); expect($sameCommit->refresh()->read_at)->not->toBeNull() ->and($replied->refresh()->read_at)->toBeNull() @@ -61,7 +61,28 @@ it('marks nothing for a user without notifications', function (): void { $notification = Notification::factory()->create(); - resolve(UpdateNotificationsReadAt::class)->handle(User::factory()->create()); + resolve(UpdateNotificationsReadAt::class)->handle(User::factory()->create(), 'all'); expect($notification->refresh()->read_at)->toBeNull(); }); + +it('marks only the types the filter shows, and nothing under none', function (): void { + $user = User::factory()->create(); + $reply = Notification::factory()->for($user, 'recipient')->replied()->create(); + $star = Notification::factory()->for($user, 'recipient')->starred()->create(); + $follow = Notification::factory()->for($user, 'recipient')->create(); + + $action = resolve(UpdateNotificationsReadAt::class); + + $action->handle($user, 'none'); + + expect($reply->refresh()->read_at)->toBeNull() + ->and($star->refresh()->read_at)->toBeNull() + ->and($follow->refresh()->read_at)->toBeNull(); + + $action->handle($user, 'replies,stars'); + + expect($reply->refresh()->read_at)->not->toBeNull() + ->and($star->refresh()->read_at)->not->toBeNull() + ->and($follow->refresh()->read_at)->toBeNull(); +});