diff --git a/app/Actions/BackfillRouteArticlesAction.php b/app/Actions/BackfillRouteArticlesAction.php new file mode 100644 index 00000000..ba56a09f --- /dev/null +++ b/app/Actions/BackfillRouteArticlesAction.php @@ -0,0 +1,35 @@ +is_active) { + return; + } + + // Articles already validated with content, but never routed to this + // route. Uses the stored content rather than re-fetching, so a large + // backlog cannot trigger a network storm (#157). + Article::query() + ->where('feed_id', $route->feed_id) + ->whereNotNull('content') + ->whereDoesntHave('routeArticles', function ($query) use ($route) { + $query->where('feed_id', $route->feed_id) + ->where('platform_channel_id', $route->platform_channel_id); + }) + ->lazy() + ->each(function (Article $article) use ($route) { + $this->createRouteArticles->createForRoute($article, $route, (string) $article->content); + }); + } +} diff --git a/app/Actions/CreateRouteAction.php b/app/Actions/CreateRouteAction.php index 8b9c9aed..1f8463b9 100644 --- a/app/Actions/CreateRouteAction.php +++ b/app/Actions/CreateRouteAction.php @@ -2,6 +2,7 @@ namespace App\Actions; +use App\Events\RouteActivated; use App\Models\Route; class CreateRouteAction @@ -12,7 +13,7 @@ class CreateRouteAction */ public function execute(int $feedId, int $platformChannelId, int $priority = 0, bool $isActive = true): Route { - return Route::firstOrCreate( + $route = Route::firstOrCreate( [ 'feed_id' => $feedId, 'platform_channel_id' => $platformChannelId, @@ -22,5 +23,11 @@ public function execute(int $feedId, int $platformChannelId, int $priority = 0, 'is_active' => $isActive, ] ); + + if ($route->wasRecentlyCreated && $route->is_active) { + RouteActivated::dispatch($route->feed_id, $route->platform_channel_id); + } + + return $route; } } diff --git a/app/Actions/CreateRouteArticlesAction.php b/app/Actions/CreateRouteArticlesAction.php index b7ecbd34..e129e7bb 100644 --- a/app/Actions/CreateRouteArticlesAction.php +++ b/app/Actions/CreateRouteArticlesAction.php @@ -18,36 +18,38 @@ public function execute(Article $article, string $content): void ->where('is_active', true) ->get(); - // Batch-load all active keywords for this feed, grouped by channel - $keywordsByChannel = Keyword::where('feed_id', $article->feed_id) + foreach ($activeRoutes as $route) { + $this->createForRoute($article, $route, $content); + } + } + + public function createForRoute(Article $article, Route $route, string $content): void + { + $routeKeywords = Keyword::where('feed_id', $route->feed_id) + ->where('platform_channel_id', $route->platform_channel_id) ->where('is_active', true) - ->get() - ->groupBy('platform_channel_id'); + ->get(); // Match keywords against full article content, title, and description $searchableContent = $content.' '.$article->title.' '.$article->description; + $status = $this->evaluateKeywords($routeKeywords, $searchableContent); - foreach ($activeRoutes as $route) { - $routeKeywords = $keywordsByChannel->get($route->platform_channel_id, collect()); - $status = $this->evaluateKeywords($routeKeywords, $searchableContent); - - if ($status === ApprovalStatusEnum::PENDING && $this->shouldAutoApprove($route)) { - $status = ApprovalStatusEnum::APPROVED; - } - - RouteArticle::firstOrCreate( - [ - 'feed_id' => $route->feed_id, - 'platform_channel_id' => $route->platform_channel_id, - 'article_id' => $article->id, - ], - [ - 'approval_status' => $status, - 'validated_at' => now(), - 'decided_at' => $status === ApprovalStatusEnum::PENDING ? null : now(), - ] - ); + if ($status === ApprovalStatusEnum::PENDING && $this->shouldAutoApprove($route)) { + $status = ApprovalStatusEnum::APPROVED; } + + RouteArticle::firstOrCreate( + [ + 'feed_id' => $route->feed_id, + 'platform_channel_id' => $route->platform_channel_id, + 'article_id' => $article->id, + ], + [ + 'approval_status' => $status, + 'validated_at' => now(), + 'decided_at' => $status === ApprovalStatusEnum::PENDING ? null : now(), + ] + ); } /** diff --git a/app/Events/RouteActivated.php b/app/Events/RouteActivated.php new file mode 100644 index 00000000..f258cef0 --- /dev/null +++ b/app/Events/RouteActivated.php @@ -0,0 +1,17 @@ +where('feed_id', $event->feedId) + ->where('platform_channel_id', $event->platformChannelId) + ->first(); + + if ($route === null || ! $route->is_active) { + return; + } + + $this->backfillRouteArticles->execute($route); + } +} diff --git a/app/Livewire/Routes.php b/app/Livewire/Routes.php index 64ee812f..b3960689 100644 --- a/app/Livewire/Routes.php +++ b/app/Livewire/Routes.php @@ -2,6 +2,7 @@ namespace App\Livewire; +use App\Events\RouteActivated; use App\Models\Feed; use App\Models\Keyword; use App\Models\PlatformChannel; @@ -72,6 +73,8 @@ public function createRoute(): void 'is_active' => true, ]); + RouteActivated::dispatch($this->newFeedId, $this->newChannelId); + $this->closeCreateModal(); } @@ -129,6 +132,10 @@ public function toggle(int $feedId, int $channelId): void $route->is_active = ! $route->is_active; $route->save(); + + if ($route->is_active) { + RouteActivated::dispatch($route->feed_id, $route->platform_channel_id); + } } public function delete(int $feedId, int $channelId): void diff --git a/app/Models/Article.php b/app/Models/Article.php index 46d07d53..e02a8803 100644 --- a/app/Models/Article.php +++ b/app/Models/Article.php @@ -22,6 +22,7 @@ * @property string $url * @property string $title * @property string|null $description + * @property string|null $content * @property Carbon|null $validated_at * @property Carbon $created_at * @property Carbon $updated_at diff --git a/config/feed.php b/config/feed.php index 78d23199..12a0b33c 100644 --- a/config/feed.php +++ b/config/feed.php @@ -45,7 +45,8 @@ 'type' => 'website', 'is_active' => true, 'languages' => [ - 'en' => ['url' => 'https://capi.belga.press/belgapress/api/public/pressreleases?offset=1&count=50&search=&start=&end=&newsroomId=70&language=EN'], + // offset is a 0-based item index; offset=1 skips the newest release (#158). + 'en' => ['url' => 'https://capi.belga.press/belgapress/api/public/pressreleases?offset=0&count=50&search=&start=&end=&newsroomId=70&language=EN'], ], 'parsers' => [ 'homepage' => BelgaHomepageParserAdapter::class, diff --git a/database/migrations/2024_01_01_000024_fix_belga_feed_offset.php b/database/migrations/2024_01_01_000024_fix_belga_feed_offset.php new file mode 100644 index 00000000..30e88483 --- /dev/null +++ b/database/migrations/2024_01_01_000024_fix_belga_feed_offset.php @@ -0,0 +1,65 @@ +where('provider', 'belga')->where('url', $url)->min('id') + ?? DB::table('feeds')->where('provider', 'belga')->min('id'); + + if ($keepId === null) { + return; + } + + DB::table('feeds') + ->where('provider', 'belga') + ->where('id', '!=', $keepId) + ->update([ + 'is_active' => false, + 'updated_at' => now(), + ]); + + DB::table('feeds') + ->where('id', $keepId) + ->update([ + 'url' => $url, + 'updated_at' => now(), + ]); + } + + public function down(): void + { + // No rollback: restoring offset=1 would reinstate a feed that silently + // drops the newest press release. + } +}; diff --git a/tests/Feature/BackfillRouteArticlesTest.php b/tests/Feature/BackfillRouteArticlesTest.php new file mode 100644 index 00000000..a54a1f1f --- /dev/null +++ b/tests/Feature/BackfillRouteArticlesTest.php @@ -0,0 +1,143 @@ +create([ + 'feed_id' => $route->feed_id, + 'content' => 'Some article content', + ]); + } + + private function makeRoute(bool $isActive): Route + { + $feed = Feed::factory()->create(); + $channel = PlatformChannel::factory()->create(); + + return Route::create([ + 'feed_id' => $feed->id, + 'platform_channel_id' => $channel->id, + 'priority' => 50, + 'is_active' => $isActive, + ]); + } + + public function test_listener_backfills_stranded_articles(): void + { + $route = $this->makeRoute(true); + $article = $this->strandedArticle($route); + + $this->listener()->handle(new RouteActivated($route->feed_id, $route->platform_channel_id)); + + $this->assertSame(1, RouteArticle::where('article_id', $article->id)->count()); + } + + public function test_listener_does_nothing_when_the_route_is_missing(): void + { + $this->listener()->handle(new RouteActivated(99999, 99999)); + + $this->assertSame(0, RouteArticle::count()); + } + + public function test_listener_does_nothing_when_the_route_is_inactive(): void + { + $route = $this->makeRoute(false); + $this->strandedArticle($route); + + $this->listener()->handle(new RouteActivated($route->feed_id, $route->platform_channel_id)); + + $this->assertSame(0, RouteArticle::count()); + } + + public function test_creating_a_route_backfills_articles_that_were_validated_with_no_route(): void + { + $feed = Feed::factory()->create(); + $channel = PlatformChannel::factory()->create(); + $article = Article::factory()->create([ + 'feed_id' => $feed->id, + 'content' => 'Some article content', + ]); + + (new CreateRouteAction)->execute($feed->id, $channel->id); + + $this->assertSame(1, RouteArticle::where('article_id', $article->id)->count()); + } + + public function test_toggling_a_route_inactive_then_active_backfills(): void + { + $route = $this->makeRoute(false); + $article = $this->strandedArticle($route); + + Livewire::test(Routes::class) + ->call('toggle', $route->feed_id, $route->platform_channel_id); + + $this->assertTrue($route->fresh()->is_active); + $this->assertSame(1, RouteArticle::where('article_id', $article->id)->count()); + } + + public function test_create_route_action_dispatches_route_activated(): void + { + Event::fake([RouteActivated::class]); + + $feed = Feed::factory()->create(); + $channel = PlatformChannel::factory()->create(); + + (new CreateRouteAction)->execute($feed->id, $channel->id); + + Event::assertDispatched( + RouteActivated::class, + fn (RouteActivated $event) => $event->feedId === $feed->id && $event->platformChannelId === $channel->id + ); + } + + public function test_create_route_action_does_not_dispatch_for_an_existing_route(): void + { + Event::fake([RouteActivated::class]); + + $feed = Feed::factory()->create(); + $channel = PlatformChannel::factory()->create(); + + (new CreateRouteAction)->execute($feed->id, $channel->id); + (new CreateRouteAction)->execute($feed->id, $channel->id); + + Event::assertDispatchedTimes(RouteActivated::class, 1); + } + + public function test_create_route_action_does_not_dispatch_for_an_inactive_route(): void + { + Event::fake([RouteActivated::class]); + + $feed = Feed::factory()->create(); + $channel = PlatformChannel::factory()->create(); + + (new CreateRouteAction)->execute($feed->id, $channel->id, 0, false); + + Event::assertNotDispatched(RouteActivated::class); + } +} diff --git a/tests/Feature/EventListenerRegistrationTest.php b/tests/Feature/EventListenerRegistrationTest.php index c5c0fe9a..a6fda0dc 100644 --- a/tests/Feature/EventListenerRegistrationTest.php +++ b/tests/Feature/EventListenerRegistrationTest.php @@ -6,6 +6,7 @@ use App\Events\ActivityLogged; use App\Events\ExceptionOccurred; use App\Events\NewArticleFetched; +use App\Events\RouteActivated; use App\Events\RouteArticleApproved; use Illuminate\Contracts\Debug\ExceptionHandler; use Illuminate\Support\Facades\Event; @@ -25,6 +26,7 @@ public static function eventProvider(): array 'ActivityLogged' => [ActivityLogged::class], 'ExceptionOccurred' => [ExceptionOccurred::class], 'NewArticleFetched' => [NewArticleFetched::class], + 'RouteActivated' => [RouteActivated::class], 'RouteArticleApproved' => [RouteArticleApproved::class], ]; } diff --git a/tests/Feature/FixBelgaFeedOffsetMigrationTest.php b/tests/Feature/FixBelgaFeedOffsetMigrationTest.php new file mode 100644 index 00000000..4e5c0921 --- /dev/null +++ b/tests/Feature/FixBelgaFeedOffsetMigrationTest.php @@ -0,0 +1,176 @@ +up(); + } + + private function findFeed(int $id): stdClass + { + $feed = DB::table('feeds')->find($id); + + $this->assertInstanceOf(stdClass::class, $feed); + + return $feed; + } + + private function targetUrl(): string + { + $url = config('feed.providers.belga.languages.en.url'); + + $this->assertIsString($url); + + return $url; + } + + private function seedFeed(string $url, bool $isActive = true, string $provider = 'belga', string $type = 'website'): int + { + return DB::table('feeds')->insertGetId([ + 'name' => 'Belga', + 'provider' => $provider, + 'type' => $type, + 'url' => $url, + 'is_active' => $isActive, + 'created_at' => now(), + 'updated_at' => now(), + ]); + } + + public function test_moves_a_feed_still_on_the_offset_one_url(): void + { + DB::table('feeds')->delete(); + $id = $this->seedFeed(self::OFFSET_ONE_URL); + + $this->runMigration(); + + $feed = $this->findFeed($id); + + $this->assertSame($this->targetUrl(), $feed->url); + $this->assertSame('website', $feed->type); + $this->assertTrue((bool) $feed->is_active); + } + + public function test_is_idempotent_when_the_feed_is_already_on_the_target_url(): void + { + DB::table('feeds')->delete(); + $id = $this->seedFeed($this->targetUrl()); + + $this->runMigration(); + $this->runMigration(); + + $this->assertSame(1, DB::table('feeds')->where('provider', 'belga')->count()); + + $feed = $this->findFeed($id); + $this->assertSame($this->targetUrl(), $feed->url); + $this->assertTrue((bool) $feed->is_active); + } + + public function test_deactivates_superseded_rows_instead_of_colliding_on_the_unique_url(): void + { + DB::table('feeds')->delete(); + $staleId = $this->seedFeed(self::OFFSET_ONE_URL); + $currentId = $this->seedFeed($this->targetUrl()); + + $this->runMigration(); + + // Both rows survive: routes.feed_id cascades on delete, so a superseded + // feed is deactivated rather than removed. + $this->assertSame(2, DB::table('feeds')->where('provider', 'belga')->count()); + + $current = $this->findFeed($currentId); + $this->assertTrue((bool) $current->is_active); + $this->assertSame($this->targetUrl(), $current->url); + + $stale = $this->findFeed($staleId); + $this->assertFalse((bool) $stale->is_active); + } + + public function test_adopts_the_oldest_row_when_none_is_on_the_target_url(): void + { + DB::table('feeds')->delete(); + $oldestId = $this->seedFeed(self::OFFSET_ONE_URL); + $newerId = $this->seedFeed('https://capi.belga.press/belgapress/api/public/pressreleases?offset=1&count=6'); + + $this->runMigration(); + + // Neither row matches config, so the migration falls back to the lowest + // id — the oldest row, which is the one routes are most likely tied to. + $oldest = $this->findFeed($oldestId); + $this->assertSame($this->targetUrl(), $oldest->url); + $this->assertTrue((bool) $oldest->is_active); + + $this->assertFalse((bool) $this->findFeed($newerId)->is_active); + } + + public function test_leaves_other_providers_untouched(): void + { + DB::table('feeds')->delete(); + $vrtId = $this->seedFeed('https://www.vrt.be/vrtnws/nl/', true, 'vrt'); + // Seed a belga row too, so the migration actually runs its updates + // rather than bailing out at the "no belga feed" guard. + $this->seedFeed(self::OFFSET_ONE_URL); + + $this->runMigration(); + + $vrt = $this->findFeed($vrtId); + + $this->assertSame('https://www.vrt.be/vrtnws/nl/', $vrt->url); + $this->assertTrue((bool) $vrt->is_active); + } + + public function test_down_is_a_deliberate_no_op(): void + { + DB::table('feeds')->delete(); + $id = $this->seedFeed($this->targetUrl()); + + $migration = require database_path('migrations/2024_01_01_000024_fix_belga_feed_offset.php'); + $migration->down(); + + // Rolling back must not restore the offset=1 url that drops the newest + // press release. + $feed = $this->findFeed($id); + $this->assertSame($this->targetUrl(), $feed->url); + $this->assertTrue((bool) $feed->is_active); + } + + public function test_does_nothing_when_no_belga_feed_exists(): void + { + DB::table('feeds')->delete(); + + $this->runMigration(); + + $this->assertSame(0, DB::table('feeds')->count()); + } + + public function test_does_nothing_when_the_configured_url_is_missing(): void + { + DB::table('feeds')->delete(); + config(['feed.providers.belga.languages.en.url' => '']); + $id = $this->seedFeed(self::OFFSET_ONE_URL); + + $this->runMigration(); + + $feed = $this->findFeed($id); + + $this->assertSame(self::OFFSET_ONE_URL, $feed->url); + } +} diff --git a/tests/Unit/Actions/BackfillRouteArticlesActionTest.php b/tests/Unit/Actions/BackfillRouteArticlesActionTest.php new file mode 100644 index 00000000..84fb960c --- /dev/null +++ b/tests/Unit/Actions/BackfillRouteArticlesActionTest.php @@ -0,0 +1,154 @@ +create(); + $channel = PlatformChannel::factory()->create(); + + return Route::create([ + 'feed_id' => $feed->id, + 'platform_channel_id' => $channel->id, + 'priority' => 50, + 'is_active' => $isActive, + ]); + } + + private function strandedArticle(Route $route): Article + { + return Article::factory()->create([ + 'feed_id' => $route->feed_id, + 'title' => 'A title', + 'description' => 'A description', + 'content' => 'Some article content', + ]); + } + + public function test_it_backfills_articles_missing_a_route_article(): void + { + $route = $this->route(); + $article = $this->strandedArticle($route); + + $this->action()->execute($route); + + $this->assertDatabaseHas('route_articles', [ + 'article_id' => $article->id, + 'feed_id' => $route->feed_id, + 'platform_channel_id' => $route->platform_channel_id, + ]); + } + + public function test_it_is_idempotent(): void + { + $route = $this->route(); + $this->strandedArticle($route); + + $this->action()->execute($route); + $this->action()->execute($route); + + $this->assertSame(1, RouteArticle::count()); + } + + public function test_it_skips_articles_already_routed_to_this_route(): void + { + $route = $this->route(); + $article = $this->strandedArticle($route); + + RouteArticle::create([ + 'feed_id' => $route->feed_id, + 'platform_channel_id' => $route->platform_channel_id, + 'article_id' => $article->id, + 'approval_status' => ApprovalStatusEnum::APPROVED, + ]); + + $this->action()->execute($route); + + $this->assertSame(1, RouteArticle::count()); + $this->assertSame(ApprovalStatusEnum::APPROVED, RouteArticle::first()->approval_status); + } + + public function test_it_skips_articles_without_stored_content(): void + { + $route = $this->route(); + Article::factory()->create([ + 'feed_id' => $route->feed_id, + 'content' => null, + ]); + + $this->action()->execute($route); + + $this->assertSame(0, RouteArticle::count()); + } + + public function test_it_does_nothing_for_an_inactive_route(): void + { + $route = $this->route(isActive: false); + $this->strandedArticle($route); + + $this->action()->execute($route); + + $this->assertSame(0, RouteArticle::count()); + } + + public function test_it_does_not_touch_articles_in_other_feeds(): void + { + $route = $this->route(); + $this->strandedArticle($route); + + $otherFeed = Feed::factory()->create(); + $otherArticle = Article::factory()->create([ + 'feed_id' => $otherFeed->id, + 'content' => 'Other content', + ]); + + $this->action()->execute($route); + + $this->assertSame(0, RouteArticle::where('article_id', $otherArticle->id)->count()); + } + + public function test_keyword_matching_uses_the_stored_content_without_refetching(): void + { + Setting::setBool('enable_publishing_approvals', true); + $route = $this->route(); + Article::factory()->create([ + 'feed_id' => $route->feed_id, + 'title' => 'A title', + 'description' => 'A description', + 'content' => 'news from Brussels today', + ]); + + Keyword::create([ + 'feed_id' => $route->feed_id, + 'platform_channel_id' => $route->platform_channel_id, + 'keyword' => 'brussels', + 'is_active' => true, + ]); + + $this->action()->execute($route); + + $this->assertSame(ApprovalStatusEnum::PENDING, RouteArticle::first()->approval_status); + } +} diff --git a/tests/Unit/Services/Parsers/BelgaHomepageParserTest.php b/tests/Unit/Services/Parsers/BelgaHomepageParserTest.php index 15b74342..12eae183 100644 --- a/tests/Unit/Services/Parsers/BelgaHomepageParserTest.php +++ b/tests/Unit/Services/Parsers/BelgaHomepageParserTest.php @@ -114,7 +114,8 @@ public function test_adapter_api_url_pins_load_bearing_query_params(): void $this->assertStringStartsWith('https://capi.belga.press/belgapress/api/public/pressreleases?', $url); $this->assertStringContainsString('newsroomId=70', $url); - $this->assertStringContainsString('offset=1', $url); + // 0-based item index: offset=1 skipped the newest release entirely (#158). + $this->assertStringContainsString('offset=0&', $url); $this->assertStringContainsString('count=50', $url); $this->assertStringContainsString('language=EN', $url); }