diff --git a/app/Http/Controllers/StoryComposeController.php b/app/Http/Controllers/StoryComposeController.php index 24ed2deee..dddc7412f 100644 --- a/app/Http/Controllers/StoryComposeController.php +++ b/app/Http/Controllers/StoryComposeController.php @@ -221,7 +221,7 @@ class StoryComposeController extends Controller $img = $this->imageManager->decodePath($path); $img = $img->crop($width, $height, $x, $y); - $img = $img->coverDown(1080, 1920); + $img = $img->cover(1080, 1920); if (in_array(strtolower($extension), ['jpg', 'jpeg'])) { $encoder = new JpegEncoder($quality); @@ -239,7 +239,7 @@ class StoryComposeController extends Controller $img = $this->imageManager->decodeBinary($fileContent); $img = $img->crop($width, $height, $x, $y); - $img = $img->coverDown(1080, 1920); + $img = $img->cover(1080, 1920); if (in_array(strtolower($extension), ['jpg', 'jpeg'])) { $encoder = new JpegEncoder($quality); diff --git a/app/Services/NotificationService.php b/app/Services/NotificationService.php index 3db108032..d35d02bc9 100644 --- a/app/Services/NotificationService.php +++ b/app/Services/NotificationService.php @@ -108,7 +108,11 @@ class NotificationService if (! $epoch) { NotificationEpochUpdatePipeline::dispatch(); - return 1; + $rec = Notification::whereDate('created_at', '>=', now()->subMonths($months)->format('Y-m-d')) + ->orderBy('id') + ->first(); + + return $rec ? $rec->id : 1; } return $epoch; @@ -142,12 +146,51 @@ class NotificationService */ public static function getMaxPage($id = false, $maxId = null, $limit = 10) { - return self::fetchPage($id, $maxId, 'max', $limit); + return self::fetchPage($id, $maxId, 'max', $limit, self::renderableFilter()); } public static function getMinPage($id = false, $minId = null, $limit = 10) { - return self::fetchPage($id, $minId, 'min', $limit); + return self::fetchPage($id, $minId, 'min', $limit, self::renderableFilter()); + } + + protected static function renderableFilter(): callable + { + // Notification types that are meaningless without an attached status. + $statusTypes = ['comment', 'mention', 'share', 'reblog', 'favourite']; + + // Notification types that don't have an attached status. + $otherTypes = array_merge($statusTypes, [ + 'follow', + 'follow_request', + 'direct', + 'tagged', + 'modlog', + 'group', + 'story:react', + 'story:comment', + ]); + + return function ($n) use ($statusTypes, $otherTypes) { + if (! isset($n['account']['id'])) { + return null; + } + + $type = $n['type'] ?? null; + + if ($type !== null && ! in_array($type, $otherTypes)) { + Log::warning('NotificationService: unexpected notification type in renderableFilter', [ + 'type' => $type, + 'notification_id' => $n['id'] ?? null, + ]); + } + + if (in_array($type, $statusTypes) && ! isset($n['status']['id'])) { + return null; + } + + return $n; + }; } /** @@ -527,7 +570,7 @@ class NotificationService return null; } - if ($n->item_id && $n->item_type === Status::class && ! $n->item) { + if ($n->item_id && in_array($n->item_type, ['App\Status', Status::class]) && ! $n->item) { return null; } diff --git a/app/Transformer/Api/Mastodon/v1/NotificationTransformer.php b/app/Transformer/Api/Mastodon/v1/NotificationTransformer.php index fb35fc36b..820533244 100644 --- a/app/Transformer/Api/Mastodon/v1/NotificationTransformer.php +++ b/app/Transformer/Api/Mastodon/v1/NotificationTransformer.php @@ -30,7 +30,7 @@ class NotificationTransformer extends Fractal\TransformerAbstract public function includeStatus(Notification $notification) { $item = $notification; - if ($item->item_id && $item->item_type == Status::class) { + if ($item->item_id && in_array($item->item_type, ['App\Status', Status::class])) { $status = Status::with('media')->find($item->item_id); if ($status) { return $this->item($status, new StatusTransformer); diff --git a/app/Transformer/Api/NotificationTransformer.php b/app/Transformer/Api/NotificationTransformer.php index 1883fd24a..2baee6361 100644 --- a/app/Transformer/Api/NotificationTransformer.php +++ b/app/Transformer/Api/NotificationTransformer.php @@ -30,7 +30,7 @@ class NotificationTransformer extends Fractal\TransformerAbstract } } - if ($n->item_id && $n->item_type == Status::class) { + if ($n->item_id && in_array($n->item_type, ['App\Status', Status::class])) { $res['status'] = StatusService::get($n->item_id, false); } diff --git a/tests/Feature/NotificationServiceTest.php b/tests/Feature/NotificationServiceTest.php index d0d356808..40f1a9774 100644 --- a/tests/Feature/NotificationServiceTest.php +++ b/tests/Feature/NotificationServiceTest.php @@ -7,9 +7,31 @@ use App\Models\Status; use App\Models\User; use App\Services\NotificationService; use Illuminate\Foundation\Testing\LazilyRefreshDatabase; +use Illuminate\Support\Facades\Log; uses(LazilyRefreshDatabase::class); +/** + * Persist a like notification while forcing the raw item_type string, so tests + * can reproduce legacy ('App\Status') vs current (App\Models\Status) rows. + */ +function makeLikeNotificationWithType(int $profileId, int $actorId, int $statusId, string $itemType): Notification +{ + $n = new Notification; + $n->profile_id = $profileId; + $n->actor_id = $actorId; + $n->action = 'like'; + $n->item_id = $statusId; + $n->item_type = $itemType; + $n->save(); + + // Mirror createNotification's cache registration so the page walk sees it. + NotificationService::setNotification($n); + NotificationService::set($n->profile_id, $n->id); + + return $n; +} + describe('NotificationService::createNotification', function () { it('creates a notification and persists it', function () { $user = User::factory()->create(); @@ -186,3 +208,187 @@ describe('NotificationService::firstOrCreateNotification', function () { expect($first->id)->toBe($second->id); }); }); + +describe('NotificationService::getMaxPage renderable filtering (pixelfed#7195)', function () { + it('excludes a like notification whose status has been deleted', function () { + $user = User::factory()->create(); + $user->refresh(); + $actor = User::factory()->create(); + $actor->refresh(); + + $status = Status::factory()->create(['profile_id' => $user->profile_id, 'scope' => 'public']); + + $n = NotificationService::createNotification( + $user->profile_id, + $actor->profile_id, + 'like', + $status->id, + Status::class + ); + + // Delete the underlying status so it can no longer be hydrated. + $status->delete(); + NotificationService::del($user->profile_id, $n->id); + + $page = NotificationService::getMaxPage($user->profile_id, $n->id + 1, 20); + $ids = collect($page['data'])->pluck('id')->map(fn ($v) => (string) $v)->all(); + + expect($ids)->not->toContain((string) $n->id); + }); + + it('includes a follow notification (no status required)', function () { + $user = User::factory()->create(); + $user->refresh(); + $actor = User::factory()->create(); + $actor->refresh(); + + $n = NotificationService::createNotification( + $user->profile_id, + $actor->profile_id, + 'follow', + $actor->profile_id, + Profile::class + ); + + $page = NotificationService::getMaxPage($user->profile_id, $n->id + 1, 20); + $ids = collect($page['data'])->pluck('id')->map(fn ($v) => (string) $v)->all(); + + expect($ids)->toContain((string) $n->id); + }); + + it('includes a like notification whose status still exists', function () { + $user = User::factory()->create(); + $user->refresh(); + $actor = User::factory()->create(); + $actor->refresh(); + + $status = Status::factory()->create(['profile_id' => $user->profile_id, 'scope' => 'public']); + + $n = NotificationService::createNotification( + $user->profile_id, + $actor->profile_id, + 'like', + $status->id, + Status::class + ); + + $page = NotificationService::getMaxPage($user->profile_id, $n->id + 1, 20); + $ids = collect($page['data'])->pluck('id')->map(fn ($v) => (string) $v)->all(); + + expect($ids)->toContain((string) $n->id); + }); +}); + +describe('NotificationService legacy item_type alias (pixelfed#7195)', function () { + it('hydrates and includes a legacy App\\Status like notification', function () { + $user = User::factory()->create(); + $user->refresh(); + $actor = User::factory()->create(); + $actor->refresh(); + + $status = Status::factory()->create(['profile_id' => $user->profile_id, 'scope' => 'public']); + + // Legacy row: item_type stored as the pre-migration 'App\Status'. + $n = makeLikeNotificationWithType($user->profile_id, $actor->profile_id, $status->id, 'App\Status'); + + $page = NotificationService::getMaxPage($user->profile_id, $n->id + 1, 20); + $item = collect($page['data'])->firstWhere('id', (string) $n->id) + ?? collect($page['data'])->firstWhere('id', $n->id); + + expect($item)->not->toBeNull(); + expect($item['status'] ?? null)->not->toBeNull(); + expect((string) $item['status']['id'])->toBe((string) $status->id); + }); + + it('excludes a legacy App\\Status like notification whose status was deleted', function () { + $user = User::factory()->create(); + $user->refresh(); + $actor = User::factory()->create(); + $actor->refresh(); + + $status = Status::factory()->create(['profile_id' => $user->profile_id, 'scope' => 'public']); + $n = makeLikeNotificationWithType($user->profile_id, $actor->profile_id, $status->id, 'App\Status'); + + $status->delete(); + NotificationService::del($user->profile_id, $n->id); + + $page = NotificationService::getMaxPage($user->profile_id, $n->id + 1, 20); + $ids = collect($page['data'])->pluck('id')->map(fn ($v) => (string) $v)->all(); + + expect($ids)->not->toContain((string) $n->id); + }); +}); + +describe('NotificationService::renderableFilter unexpected type warning', function () { + it('logs a warning for a notification type outside the known set', function () { + Log::spy(); + + $user = User::factory()->create(); + $user->refresh(); + $actor = User::factory()->create(); + $actor->refresh(); + + // An unknown action -> replaceTypeVerb passes it through unchanged, so + // the page walk sees a type not in the known list. + $n = NotificationService::createNotification( + $user->profile_id, + $actor->profile_id, + 'totally-unknown-type', + $actor->profile_id, + Profile::class + ); + + NotificationService::getMaxPage($user->profile_id, $n->id + 1, 20); + + Log::shouldHaveReceived('warning') + ->withArgs(fn ($message) => str_contains($message, 'unexpected notification type')) + ->atLeast()->once(); + }); + + it('does not warn for a known follow notification', function () { + Log::spy(); + + $user = User::factory()->create(); + $user->refresh(); + $actor = User::factory()->create(); + $actor->refresh(); + + $n = NotificationService::createNotification( + $user->profile_id, + $actor->profile_id, + 'follow', + $actor->profile_id, + Profile::class + ); + + NotificationService::getMaxPage($user->profile_id, $n->id + 1, 20); + + Log::shouldNotHaveReceived('warning'); + }); +}); + +describe('NotificationService pagination termination (pixelfed#7195)', function () { + it('returns empty data when every candidate is an unrenderable status notification', function () { + $user = User::factory()->create(); + $user->refresh(); + $actor = User::factory()->create(); + $actor->refresh(); + + // Two like notifications whose statuses are all deleted -> nothing is + // renderable, so the page must come back empty and let the client stop + // paginating instead of looping forever. + $notifs = []; + foreach (range(1, 2) as $i) { + $status = Status::factory()->create(['profile_id' => $user->profile_id, 'scope' => 'public']); + $n = makeLikeNotificationWithType($user->profile_id, $actor->profile_id, $status->id, Status::class); + $status->delete(); + NotificationService::del($user->profile_id, $n->id); + $notifs[] = $n; + } + + $topId = max(array_map(fn ($n) => $n->id, $notifs)) + 1; + $page = NotificationService::getMaxPage($user->profile_id, $topId, 20); + + expect($page['data'])->toBeEmpty(); + }); +}); diff --git a/tests/Feature/NotificationTransformerStatusHydrationTest.php b/tests/Feature/NotificationTransformerStatusHydrationTest.php new file mode 100644 index 000000000..1f761592c --- /dev/null +++ b/tests/Feature/NotificationTransformerStatusHydrationTest.php @@ -0,0 +1,114 @@ + App\Models\ namespace migration hold the legacy 'App\Status' +| value (now a morph-map alias), while new rows hold App\Models\Status. +| A strict `== Status::class` comparison silently skipped status hydration +| for the legacy rows, so favourite/comment/mention notifications came back +| with no attached status and the web UI dropped them (endless loading). +| +| These transform the notification directly (no Redis-backed service cache) +| so the assertion targets the transformer contract itself. +| +*/ + +function transformNotification(Notification $n): array +{ + $fractal = new Manager; + $fractal->setSerializer(new ArraySerializer); + + return $fractal->createData(new Item($n, new NotificationTransformer))->toArray(); +} + +/** + * Persist a like notification while forcing the raw item_type string, so we + * can reproduce legacy ('App\Status') vs current (App\Models\Status) rows. + */ +function makeLikeNotification(string $itemType): array +{ + $user = User::factory()->create(); + $user->refresh(); + $actor = User::factory()->create(); + $actor->refresh(); + + $status = Status::factory()->create([ + 'profile_id' => $user->profile_id, + 'scope' => 'public', + ]); + + $n = new Notification; + $n->profile_id = $user->profile_id; + $n->actor_id = $actor->profile_id; + $n->action = 'like'; + $n->item_id = $status->id; + $n->item_type = $itemType; + $n->save(); + + return [$n, $status]; +} + +it('hydrates status for a legacy App\\Status notification (regression)', function () { + [$n, $status] = makeLikeNotification('App\Status'); + + $res = transformNotification($n); + + expect($res['type'])->toBe('favourite'); + expect($res['status'])->not->toBeNull(); + expect((string) $res['status']['id'])->toBe((string) $status->id); +}); + +it('hydrates status for a current App\\Models\\Status notification', function () { + [$n, $status] = makeLikeNotification(Status::class); + + $res = transformNotification($n); + + expect($res['status'])->not->toBeNull(); + expect((string) $res['status']['id'])->toBe((string) $status->id); +}); + +it('returns a null status for a legacy notification whose status was deleted', function () { + [$n, $status] = makeLikeNotification('App\Status'); + $status->delete(); + + $res = transformNotification($n); + + // The comparison now matches, so hydration is attempted; a deleted status + // resolves to null rather than being skipped by an unmatched item_type. + expect($res['status'] ?? null)->toBeNull(); +}); + +it('does not attach a status for a follow notification', function () { + $user = User::factory()->create(); + $user->refresh(); + $actor = User::factory()->create(); + $actor->refresh(); + + $n = new Notification; + $n->profile_id = $user->profile_id; + $n->actor_id = $actor->profile_id; + $n->action = 'follow'; + $n->item_id = $actor->profile_id; + $n->item_type = 'App\Profile'; + $n->save(); + + $res = transformNotification($n); + + expect($res['type'])->toBe('follow'); + expect(array_key_exists('status', $res))->toBeFalse(); +}); diff --git a/tests/Feature/Services/NotificationEpochFallbackTest.php b/tests/Feature/Services/NotificationEpochFallbackTest.php new file mode 100644 index 000000000..8ccc41751 --- /dev/null +++ b/tests/Feature/Services/NotificationEpochFallbackTest.php @@ -0,0 +1,62 @@ + 1`, scanning the +| entire notifications table. +| +*/ + +beforeEach(function () { + Queue::fake(); + Cache::forget(NotificationService::EPOCH_CACHE_KEY.'6'); +}); + +it('returns a recent notification id on cache miss instead of 1', function () { + $old = Notification::create([ + 'profile_id' => 1, + 'actor_id' => 2, + 'action' => 'like', + 'created_at' => now()->subMonths(9), + ]); + + $recent = Notification::create([ + 'profile_id' => 1, + 'actor_id' => 2, + 'action' => 'like', + 'created_at' => now()->subMonth(), + ]); + + $epoch = NotificationService::getEpochId(); + + expect($epoch)->toBe($recent->id); + expect($epoch)->not->toBe(1); + expect($epoch)->toBeGreaterThan($old->id); + + Queue::assertPushed(NotificationEpochUpdatePipeline::class); +}); + +it('falls back to 1 when no recent notifications exist', function () { + Notification::create([ + 'profile_id' => 1, + 'actor_id' => 2, + 'action' => 'like', + 'created_at' => now()->subYear(), + ]); + + expect(NotificationService::getEpochId())->toBe(1); +});