From b2d504f503c1824c441030dad7d07862d4a82f12 Mon Sep 17 00:00:00 2001 From: myrmidex Date: Sun, 2 Aug 2026 15:45:26 +0200 Subject: [PATCH 1/5] 123 - Prevent duplicate Lemmy posts from concurrent publish attempts --- app/Models/RouteArticle.php | 4 + .../Publishing/ArticlePublishingService.php | 38 ++++ tests/Feature/DuplicatePublishTest.php | 167 ++++++++++++++++++ tests/Unit/Models/RouteArticleTest.php | 30 ++++ .../ArticlePublishingServiceTest.php | 71 ++++++++ 5 files changed, 310 insertions(+) create mode 100644 tests/Feature/DuplicatePublishTest.php diff --git a/app/Models/RouteArticle.php b/app/Models/RouteArticle.php index 7e71b524..8b0bc42e 100644 --- a/app/Models/RouteArticle.php +++ b/app/Models/RouteArticle.php @@ -92,6 +92,10 @@ public function isRejected(): bool public function approve(): void { + if ($this->isApproved()) { + return; + } + $this->update(['approval_status' => ApprovalStatusEnum::APPROVED]); event(new RouteArticleApproved($this)); diff --git a/app/Services/Publishing/ArticlePublishingService.php b/app/Services/Publishing/ArticlePublishingService.php index 73079e1d..4d427fca 100644 --- a/app/Services/Publishing/ArticlePublishingService.php +++ b/app/Services/Publishing/ArticlePublishingService.php @@ -12,10 +12,16 @@ use App\Modules\Lemmy\Services\LemmyPublisher; use App\Services\Log\LogSaver; use Exception; +use Illuminate\Contracts\Cache\LockTimeoutException; +use Illuminate\Support\Facades\Cache; use RuntimeException; class ArticlePublishingService { + private const LOCK_TTL_SECONDS = 180; + + private const LOCK_WAIT_SECONDS = 15; + public function __construct(private LogSaver $logSaver) {} /** @@ -64,6 +70,38 @@ public function publishRouteArticle(RouteArticle $routeArticle, array $extracted * @param array $extractedData */ private function publishToChannel(Article $article, array $extractedData, PlatformChannel $channel, mixed $account): ?ArticlePublication + { + $lock = Cache::lock("publish:{$article->id}:{$channel->id}", self::LOCK_TTL_SECONDS); + + try { + return $lock->block(self::LOCK_WAIT_SECONDS, function () use ($article, $extractedData, $channel, $account) { + $alreadyPublished = ArticlePublication::where('article_id', $article->id) + ->where('platform_channel_id', $channel->id) + ->exists(); + + if ($alreadyPublished) { + $this->logSaver->info('Skipping duplicate: already published to channel', $channel, [ + 'article_id' => $article->id, + ]); + + return null; + } + + return $this->doPublishToChannel($article, $extractedData, $channel, $account); + }); + } catch (LockTimeoutException $e) { + $this->logSaver->info('Skipping publish: another worker holds the lock', $channel, [ + 'article_id' => $article->id, + ]); + + return null; + } + } + + /** + * @param array $extractedData + */ + private function doPublishToChannel(Article $article, array $extractedData, PlatformChannel $channel, mixed $account): ?ArticlePublication { try { // Check if this URL or title was already posted to this channel diff --git a/tests/Feature/DuplicatePublishTest.php b/tests/Feature/DuplicatePublishTest.php new file mode 100644 index 00000000..a1d37646 --- /dev/null +++ b/tests/Feature/DuplicatePublishTest.php @@ -0,0 +1,167 @@ +create(); + $instance = PlatformInstance::factory()->create(); + $channel = PlatformChannel::factory()->create(['platform_instance_id' => $instance->id]); + $account = PlatformAccount::factory()->create(); + + /** @var Route $route */ + $route = Route::factory()->active()->create([ + 'feed_id' => $feed->id, + 'platform_channel_id' => $channel->id, + ]); + + $channel->platformAccounts()->attach($account->id, ['is_active' => true, 'priority' => 50]); + + $article = Article::factory()->create(['feed_id' => $feed->id]); + + /** @var RouteArticle $routeArticle */ + $routeArticle = RouteArticle::factory()->forRoute($route)->create([ + 'article_id' => $article->id, + ]); + + $this->fixture = [$routeArticle, $channel, $article]; + } + + protected function tearDown(): void + { + Mockery::close(); + parent::tearDown(); + } + + /** + * Real publishing service with only the Lemmy call faked, counting how many + * posts would actually be created remotely. + */ + private function makeListener(): PublishApprovedArticleListener + { + $publisher = Mockery::mock(LemmyPublisher::class); + $publisher->shouldReceive('publishToChannel') + ->andReturnUsing(function () { + $this->remoteCalls++; + + return ['post_view' => ['post' => ['id' => 2000000 + $this->remoteCalls]]]; + }); + + $service = Mockery::mock( + ArticlePublishingService::class, + [app(LogSaver::class)] + )->makePartial(); + $service->shouldAllowMockingProtectedMethods(); + $service->shouldReceive('makePublisher')->andReturn($publisher); + + $fetcher = Mockery::mock(ArticleFetcher::class); + $fetcher->shouldReceive('fetchArticleData')->andReturn(['title' => 'Test Article']); + + return new PublishApprovedArticleListener($fetcher, $service, new NotificationService); + } + + public function test_clicking_approve_twice_creates_only_one_remote_post(): void + { + Event::fake([RouteArticleApproved::class]); + + [$routeArticle] = $this->fixture; + + // The double-click: approve() must not dispatch a second time. + $routeArticle->approve(); + $routeArticle->approve(); + + Event::assertDispatchedTimes(RouteArticleApproved::class, 1); + } + + public function test_two_queued_listeners_create_only_one_remote_post(): void + { + [$routeArticle, $channel, $article] = $this->fixture; + + // Two listeners already in flight. Running them back to back would not + // reproduce anything — the second would see the first's publication row + // and stop. The real race interleaves: the second listener reaches its + // duplicate check while the first is still inside its Lemmy call, before + // any row exists. That window is what the lock has to close. + $publisher = Mockery::mock(LemmyPublisher::class); + $publisher->shouldReceive('publishToChannel') + ->andReturnUsing(function () use ($routeArticle) { + $this->remoteCalls++; + + if ($this->remoteCalls === 1) { + $this->makeListener()->handle(new RouteArticleApproved($routeArticle->fresh())); + } + + return ['post_view' => ['post' => ['id' => 2000000 + $this->remoteCalls]]]; + }); + + $service = Mockery::mock( + ArticlePublishingService::class, + [app(LogSaver::class)] + )->makePartial(); + $service->shouldAllowMockingProtectedMethods(); + $service->shouldReceive('makePublisher')->andReturn($publisher); + + $fetcher = Mockery::mock(ArticleFetcher::class); + $fetcher->shouldReceive('fetchArticleData')->andReturn(['title' => 'Test Article']); + + $listener = new PublishApprovedArticleListener($fetcher, $service, new NotificationService); + $listener->handle(new RouteArticleApproved($routeArticle)); + + $this->assertSame(1, $this->remoteCalls, 'Two listeners must not both post to Lemmy.'); + + $this->assertSame(1, ArticlePublication::where('article_id', $article->id) + ->where('platform_channel_id', $channel->id) + ->count()); + } + + public function test_a_single_approval_publishes_exactly_once(): void + { + [$routeArticle, $channel, $article] = $this->fixture; + + $this->makeListener()->handle(new RouteArticleApproved($routeArticle)); + + $this->assertSame(1, $this->remoteCalls); + $this->assertSame(PublishStatusEnum::PUBLISHED, $routeArticle->fresh()->publish_status); + $this->assertSame(1, ArticlePublication::where('article_id', $article->id) + ->where('platform_channel_id', $channel->id) + ->count()); + } +} diff --git a/tests/Unit/Models/RouteArticleTest.php b/tests/Unit/Models/RouteArticleTest.php index 97b656ea..a08ee3de 100644 --- a/tests/Unit/Models/RouteArticleTest.php +++ b/tests/Unit/Models/RouteArticleTest.php @@ -3,6 +3,7 @@ namespace Tests\Unit\Models; use App\Enums\ApprovalStatusEnum; +use App\Events\RouteArticleApproved; use App\Models\Article; use App\Models\Feed; use App\Models\PlatformChannel; @@ -10,6 +11,7 @@ use App\Models\RouteArticle; use Illuminate\Database\QueryException; use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Support\Facades\Event; use Tests\TestCase; class RouteArticleTest extends TestCase @@ -60,6 +62,34 @@ public function test_route_article_can_be_approved(): void $this->assertEquals(ApprovalStatusEnum::APPROVED, $routeArticle->fresh()->approval_status); } + public function test_approving_dispatches_the_approved_event(): void + { + Event::fake([RouteArticleApproved::class]); + + /** @var RouteArticle $routeArticle */ + $routeArticle = RouteArticle::factory()->create(); + + $routeArticle->approve(); + + Event::assertDispatchedTimes(RouteArticleApproved::class, 1); + } + + public function test_re_approving_an_approved_article_does_not_dispatch_again(): void + { + Event::fake([RouteArticleApproved::class]); + + /** @var RouteArticle $routeArticle */ + $routeArticle = RouteArticle::factory()->create(); + + // A double-click, or a UI action racing an API call, calls approve() twice. + // The second must be a no-op: each dispatch queues a publish listener. + $routeArticle->approve(); + $routeArticle->approve(); + + Event::assertDispatchedTimes(RouteArticleApproved::class, 1); + $this->assertEquals(ApprovalStatusEnum::APPROVED, $routeArticle->fresh()->approval_status); + } + public function test_route_article_can_be_rejected(): void { /** @var RouteArticle $routeArticle */ diff --git a/tests/Unit/Services/Publishing/ArticlePublishingServiceTest.php b/tests/Unit/Services/Publishing/ArticlePublishingServiceTest.php index 61a8d17b..cfdfd9f4 100644 --- a/tests/Unit/Services/Publishing/ArticlePublishingServiceTest.php +++ b/tests/Unit/Services/Publishing/ArticlePublishingServiceTest.php @@ -4,6 +4,7 @@ use App\Enums\PlatformEnum; use App\Models\Article; +use App\Models\ArticlePublication; use App\Models\Feed; use App\Models\PlatformAccount; use App\Models\PlatformChannel; @@ -15,7 +16,10 @@ use App\Services\Log\LogSaver; use App\Services\Publishing\ArticlePublishingService; use Exception; +use Illuminate\Contracts\Cache\Lock; +use Illuminate\Contracts\Cache\LockTimeoutException; use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Support\Facades\Cache; use Mockery; use Tests\TestCase; @@ -123,6 +127,73 @@ public function test_publish_route_article_successfully_publishes(): void ]); } + public function test_concurrent_publishes_produce_only_one_remote_post(): void + { + [$routeArticle, $channel, , $article] = $this->createRouteArticleWithAccount(); + + $remoteCalls = 0; + + // A competing listener committed its publication while this one was + // between its duplicate check and its own insert. The second attempt + // must notice and skip — the unique index cannot retract a remote post. + $publisherDouble = Mockery::mock(LemmyPublisher::class); + $publisherDouble->shouldReceive('publishToChannel') + ->andReturnUsing(function () use (&$remoteCalls, $article, $channel) { + $remoteCalls++; + + ArticlePublication::create([ + 'article_id' => $article->id, + 'post_id' => 999, + 'platform_channel_id' => $channel->id, + 'published_by' => 'other-worker', + 'published_at' => now(), + 'platform' => $channel->platformInstance->platform->value, + 'publication_data' => [], + ]); + + return ['post_view' => ['post' => ['id' => 900 + $remoteCalls]]]; + }); + + $service = Mockery::mock(ArticlePublishingService::class, [$this->logSaver])->makePartial(); + $service->shouldAllowMockingProtectedMethods(); + $service->shouldReceive('makePublisher')->andReturn($publisherDouble); + + $service->publishRouteArticle($routeArticle, ['title' => 'Hello']); + $service->publishRouteArticle($routeArticle, ['title' => 'Hello']); + + $this->assertSame(1, $remoteCalls, 'The remote must be called once, not once per racing listener.'); + $this->assertSame(1, ArticlePublication::where('article_id', $article->id) + ->where('platform_channel_id', $channel->id) + ->count()); + } + + public function test_losing_the_lock_race_skips_without_publishing(): void + { + [$routeArticle, $channel, , $article] = $this->createRouteArticleWithAccount(); + + // Another worker holds the lock, so block() gives up and throws. Faked + // rather than genuinely contended, so the test does not sit out the wait. + $lock = Mockery::mock(Lock::class); + $lock->shouldReceive('block')->once()->andThrow(new LockTimeoutException); + Cache::shouldReceive('lock') + ->with("publish:{$article->id}:{$channel->id}", 180) + ->andReturn($lock); + + $publisherDouble = Mockery::mock(LemmyPublisher::class); + $publisherDouble->shouldNotReceive('publishToChannel'); + + $service = Mockery::mock(ArticlePublishingService::class, [$this->logSaver])->makePartial(); + $service->shouldAllowMockingProtectedMethods(); + $service->shouldReceive('makePublisher')->andReturn($publisherDouble); + + // Must decline rather than throw: a LockTimeoutException would reach the + // caller's catch block and be recorded as a publish failure. + $result = $service->publishRouteArticle($routeArticle, ['title' => 'Hello']); + + $this->assertNull($result); + $this->assertDatabaseCount('article_publications', 0); + } + public function test_publish_route_article_handles_publishing_failure_gracefully(): void { [$routeArticle] = $this->createRouteArticleWithAccount(); -- 2.45.2 From b527813721e76463babe432dd425818f590c02b8 Mon Sep 17 00:00:00 2001 From: myrmidex Date: Sun, 2 Aug 2026 17:16:30 +0200 Subject: [PATCH 2/5] 123 - Key duplicate mirror by local channel and distinguish skipped publishes --- app/Actions/PublishRouteArticleAction.php | 107 ++++++++++ app/Enums/PublishStatusEnum.php | 1 + app/Jobs/PublishNextArticleJob.php | 57 +---- app/Jobs/SyncChannelPostsJob.php | 2 +- .../PublishApprovedArticleListener.php | 60 +----- app/Models/PlatformChannelPost.php | 29 +-- .../Lemmy/Services/LemmyApiService.php | 16 +- .../Publishing/ArticlePublishingService.php | 25 +-- app/Services/Publishing/PublishOutcome.php | 49 +++++ ...latform_channel_posts_by_local_channel.php | 83 ++++++++ ...ipped_to_route_articles_publish_status.php | 35 ++++ tests/Feature/DuplicatePublishTest.php | 5 +- .../KeyPlatformChannelPostsMigrationTest.php | 130 ++++++++++++ .../PublishApprovedArticleListenerTest.php | 22 +- .../Feature/MirrorDuplicateDetectionTest.php | 197 ++++++++++++++++++ tests/Unit/Jobs/PublishNextArticleJobTest.php | 46 ++-- tests/Unit/Jobs/SyncChannelPostsJobTest.php | 4 +- .../Lemmy/Services/LemmyApiServiceTest.php | 23 +- .../ArticlePublishingServiceTest.php | 27 +-- 19 files changed, 711 insertions(+), 207 deletions(-) create mode 100644 app/Actions/PublishRouteArticleAction.php create mode 100644 app/Services/Publishing/PublishOutcome.php create mode 100644 database/migrations/2024_01_01_000013_key_platform_channel_posts_by_local_channel.php create mode 100644 database/migrations/2024_01_01_000014_add_skipped_to_route_articles_publish_status.php create mode 100644 tests/Feature/KeyPlatformChannelPostsMigrationTest.php create mode 100644 tests/Feature/MirrorDuplicateDetectionTest.php diff --git a/app/Actions/PublishRouteArticleAction.php b/app/Actions/PublishRouteArticleAction.php new file mode 100644 index 00000000..b6a2f204 --- /dev/null +++ b/app/Actions/PublishRouteArticleAction.php @@ -0,0 +1,107 @@ +article; + + $routeArticle->update(['publish_status' => PublishStatusEnum::PUBLISHING]); + + try { + $extractedData = $this->articleFetcher->fetchArticleData($article); + $outcome = $this->publishingService->publishRouteArticle($routeArticle, $extractedData); + } catch (Exception $e) { + $routeArticle->update(['publish_status' => PublishStatusEnum::ERROR]); + + ActionPerformed::dispatch('Failed to publish article', LogLevelEnum::ERROR, [ + 'article_id' => $article->id, + 'error' => $e->getMessage(), + ]); + + $this->notificationService->send( + NotificationTypeEnum::PUBLISH_FAILED, + NotificationSeverityEnum::ERROR, + "Publish failed: {$article->title}", + $e->getMessage(), + $article, + ); + + throw $e; + } + + match (true) { + $outcome->succeeded() => $this->recordPublished($routeArticle), + $outcome->wasSkipped() => $this->recordSkipped($routeArticle, $outcome), + default => $this->recordFailed($routeArticle, $outcome), + }; + + return $outcome; + } + + private function recordPublished(RouteArticle $routeArticle): void + { + $routeArticle->update(['publish_status' => PublishStatusEnum::PUBLISHED]); + + ActionPerformed::dispatch('Published article', LogLevelEnum::INFO, [ + 'article_id' => $routeArticle->article->id, + 'title' => $routeArticle->article->title, + ]); + } + + private function recordSkipped(RouteArticle $routeArticle, PublishOutcome $outcome): void + { + $routeArticle->update(['publish_status' => PublishStatusEnum::SKIPPED]); + + ActionPerformed::dispatch('Skipped publishing article', LogLevelEnum::INFO, [ + 'article_id' => $routeArticle->article->id, + 'title' => $routeArticle->article->title, + 'reason' => $outcome->reason, + ]); + } + + private function recordFailed(RouteArticle $routeArticle, PublishOutcome $outcome): void + { + $article = $routeArticle->article; + + $routeArticle->update(['publish_status' => PublishStatusEnum::ERROR]); + + ActionPerformed::dispatch('No publication created for article', LogLevelEnum::WARNING, [ + 'article_id' => $article->id, + 'title' => $article->title, + 'reason' => $outcome->reason, + ]); + + $this->notificationService->send( + NotificationTypeEnum::PUBLISH_FAILED, + NotificationSeverityEnum::WARNING, + "Publish failed: {$article->title}", + $outcome->reason ?? 'No publication was created for this article.', + $article, + ); + } +} diff --git a/app/Enums/PublishStatusEnum.php b/app/Enums/PublishStatusEnum.php index 03260e30..c4e49b2a 100644 --- a/app/Enums/PublishStatusEnum.php +++ b/app/Enums/PublishStatusEnum.php @@ -7,5 +7,6 @@ enum PublishStatusEnum: string case UNPUBLISHED = 'unpublished'; case PUBLISHING = 'publishing'; case PUBLISHED = 'published'; + case SKIPPED = 'skipped'; case ERROR = 'error'; } diff --git a/app/Jobs/PublishNextArticleJob.php b/app/Jobs/PublishNextArticleJob.php index fd9a8118..e4ce99c7 100644 --- a/app/Jobs/PublishNextArticleJob.php +++ b/app/Jobs/PublishNextArticleJob.php @@ -2,19 +2,14 @@ namespace App\Jobs; +use App\Actions\PublishRouteArticleAction; use App\Enums\ApprovalStatusEnum; use App\Enums\LogLevelEnum; -use App\Enums\NotificationSeverityEnum; -use App\Enums\NotificationTypeEnum; -use App\Enums\PublishStatusEnum; use App\Events\ActionPerformed; use App\Exceptions\PublishException; use App\Models\ArticlePublication; use App\Models\RouteArticle; use App\Models\Setting; -use App\Services\Article\ArticleFetcher; -use App\Services\Notification\NotificationService; -use App\Services\Publishing\ArticlePublishingService; use Illuminate\Contracts\Queue\ShouldBeUnique; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Queue\Queueable; @@ -38,7 +33,7 @@ public function __construct() * * @throws PublishException */ - public function handle(ArticleFetcher $articleFetcher, ArticlePublishingService $publishingService, NotificationService $notificationService): void + public function handle(PublishRouteArticleAction $publishRouteArticle): void { $interval = Setting::getArticlePublishingInterval(); @@ -72,52 +67,6 @@ public function handle(ArticleFetcher $articleFetcher, ArticlePublishingService 'route' => $routeArticle->feed_id.'-'.$routeArticle->platform_channel_id, ]); - $routeArticle->update(['publish_status' => PublishStatusEnum::PUBLISHING]); - - try { - $extractedData = $articleFetcher->fetchArticleData($article); - $publication = $publishingService->publishRouteArticle($routeArticle, $extractedData); - - if ($publication) { - $routeArticle->update(['publish_status' => PublishStatusEnum::PUBLISHED]); - - ActionPerformed::dispatch('Successfully published article', LogLevelEnum::INFO, [ - 'article_id' => $article->id, - 'title' => $article->title, - ]); - } else { - $routeArticle->update(['publish_status' => PublishStatusEnum::ERROR]); - - ActionPerformed::dispatch('No publication created for article', LogLevelEnum::WARNING, [ - 'article_id' => $article->id, - 'title' => $article->title, - ]); - - $notificationService->send( - NotificationTypeEnum::PUBLISH_FAILED, - NotificationSeverityEnum::WARNING, - "Publish failed: {$article->title}", - 'No publication was created for this article. Check channel routing configuration.', - $article, - ); - } - } catch (PublishException $e) { - $routeArticle->update(['publish_status' => PublishStatusEnum::ERROR]); - - ActionPerformed::dispatch('Failed to publish article', LogLevelEnum::ERROR, [ - 'article_id' => $article->id, - 'error' => $e->getMessage(), - ]); - - $notificationService->send( - NotificationTypeEnum::PUBLISH_FAILED, - NotificationSeverityEnum::ERROR, - "Publish failed: {$article->title}", - $e->getMessage(), - $article, - ); - - throw $e; - } + $publishRouteArticle->execute($routeArticle); } } diff --git a/app/Jobs/SyncChannelPostsJob.php b/app/Jobs/SyncChannelPostsJob.php index d598733f..96043a2e 100644 --- a/app/Jobs/SyncChannelPostsJob.php +++ b/app/Jobs/SyncChannelPostsJob.php @@ -70,7 +70,7 @@ private function syncLemmyChannelPosts(LogSaver $logSaver): void $communityId = $api->resolveCommunityId($this->channel->channel_id, $token); - $api->syncChannelPosts($token, $communityId, $this->channel->name); + $api->syncChannelPosts($token, $this->channel, $communityId); $logSaver->info('Channel posts synced successfully', $this->channel); } catch (Exception $e) { diff --git a/app/Listeners/PublishApprovedArticleListener.php b/app/Listeners/PublishApprovedArticleListener.php index b08e3b7e..ae9524e7 100644 --- a/app/Listeners/PublishApprovedArticleListener.php +++ b/app/Listeners/PublishApprovedArticleListener.php @@ -2,15 +2,8 @@ namespace App\Listeners; -use App\Enums\LogLevelEnum; -use App\Enums\NotificationSeverityEnum; -use App\Enums\NotificationTypeEnum; -use App\Enums\PublishStatusEnum; -use App\Events\ActionPerformed; +use App\Actions\PublishRouteArticleAction; use App\Events\RouteArticleApproved; -use App\Services\Article\ArticleFetcher; -use App\Services\Notification\NotificationService; -use App\Services\Publishing\ArticlePublishingService; use Exception; use Illuminate\Contracts\Queue\ShouldQueue; @@ -19,9 +12,7 @@ class PublishApprovedArticleListener implements ShouldQueue public string $queue = 'publishing'; public function __construct( - private ArticleFetcher $articleFetcher, - private ArticlePublishingService $publishingService, - private NotificationService $notificationService, + private PublishRouteArticleAction $publishRouteArticle, ) {} public function handle(RouteArticleApproved $event): void @@ -29,7 +20,6 @@ public function handle(RouteArticleApproved $event): void $routeArticle = $event->routeArticle; $article = $routeArticle->article; - // Skip if already published to this channel if ($article->articlePublications() ->where('platform_channel_id', $routeArticle->platform_channel_id) ->exists() @@ -37,50 +27,10 @@ public function handle(RouteArticleApproved $event): void return; } - $routeArticle->update(['publish_status' => PublishStatusEnum::PUBLISHING]); - try { - $extractedData = $this->articleFetcher->fetchArticleData($article); - $publication = $this->publishingService->publishRouteArticle($routeArticle, $extractedData); - - if ($publication) { - $routeArticle->update(['publish_status' => PublishStatusEnum::PUBLISHED]); - - ActionPerformed::dispatch('Published approved article', LogLevelEnum::INFO, [ - 'article_id' => $article->id, - 'title' => $article->title, - ]); - } else { - $routeArticle->update(['publish_status' => PublishStatusEnum::ERROR]); - - ActionPerformed::dispatch('No publication created for approved article', LogLevelEnum::WARNING, [ - 'article_id' => $article->id, - 'title' => $article->title, - ]); - - $this->notificationService->send( - NotificationTypeEnum::PUBLISH_FAILED, - NotificationSeverityEnum::WARNING, - "Publish failed: {$article->title}", - 'No publication was created for this article. Check channel routing configuration.', - $article, - ); - } - } catch (Exception $e) { - $routeArticle->update(['publish_status' => PublishStatusEnum::ERROR]); - - ActionPerformed::dispatch('Failed to publish approved article', LogLevelEnum::ERROR, [ - 'article_id' => $article->id, - 'error' => $e->getMessage(), - ]); - - $this->notificationService->send( - NotificationTypeEnum::PUBLISH_FAILED, - NotificationSeverityEnum::ERROR, - "Publish failed: {$article->title}", - $e->getMessage(), - $article, - ); + $this->publishRouteArticle->execute($routeArticle); + } catch (Exception) { + // The action has already recorded the failure and notified. } } } diff --git a/app/Models/PlatformChannelPost.php b/app/Models/PlatformChannelPost.php index a411b241..9b94da42 100644 --- a/app/Models/PlatformChannelPost.php +++ b/app/Models/PlatformChannelPost.php @@ -2,13 +2,12 @@ namespace App\Models; -use App\Enums\PlatformEnum; use Illuminate\Database\Eloquent\Factories\Factory; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; +use Illuminate\Database\Eloquent\Relations\BelongsTo; /** - * @method static where(string $string, PlatformEnum $platform) * @method static updateOrCreate(array $array, array $array1) */ class PlatformChannelPost extends Model @@ -17,9 +16,7 @@ class PlatformChannelPost extends Model use HasFactory; protected $fillable = [ - 'platform', - 'channel_id', - 'channel_name', + 'platform_channel_id', 'post_id', 'url', 'title', @@ -33,26 +30,24 @@ protected function casts(): array { return [ 'posted_at' => 'datetime', - 'platform' => PlatformEnum::class, ]; } - public static function urlExists(PlatformEnum $platform, string $channelId, string $url): bool + /** + * @return BelongsTo + */ + public function platformChannel(): BelongsTo { - return self::where('platform', $platform) - ->where('channel_id', $channelId) - ->where('url', $url) - ->exists(); + return $this->belongsTo(PlatformChannel::class); } - public static function duplicateExists(PlatformEnum $platform, string $channelId, ?string $url, ?string $title): bool + public static function duplicateExists(PlatformChannel $channel, ?string $url, ?string $title): bool { if (! $url && ! $title) { return false; } - return self::where('platform', $platform) - ->where('channel_id', $channelId) + return self::where('platform_channel_id', $channel->id) ->where(function ($query) use ($url, $title) { if ($url) { $query->orWhere('url', $url); @@ -64,16 +59,14 @@ public static function duplicateExists(PlatformEnum $platform, string $channelId ->exists(); } - public static function storePost(PlatformEnum $platform, string $channelId, ?string $channelName, string $postId, ?string $url, ?string $title, ?\DateTime $postedAt = null): self + public static function storePost(PlatformChannel $channel, string $postId, ?string $url, ?string $title, ?\DateTime $postedAt = null): self { return self::updateOrCreate( [ - 'platform' => $platform, - 'channel_id' => $channelId, + 'platform_channel_id' => $channel->id, 'post_id' => $postId, ], [ - 'channel_name' => $channelName, 'url' => $url, 'title' => $title, 'posted_at' => $postedAt ?? now(), diff --git a/app/Modules/Lemmy/Services/LemmyApiService.php b/app/Modules/Lemmy/Services/LemmyApiService.php index 0ab58d21..c3d23c5c 100644 --- a/app/Modules/Lemmy/Services/LemmyApiService.php +++ b/app/Modules/Lemmy/Services/LemmyApiService.php @@ -2,7 +2,7 @@ namespace App\Modules\Lemmy\Services; -use App\Enums\PlatformEnum; +use App\Models\PlatformChannel; use App\Models\PlatformChannelPost; use App\Modules\Lemmy\LemmyRequest; use Exception; @@ -117,12 +117,12 @@ public function getCommunityId(string $communityName, string $token): int } } - public function syncChannelPosts(string $token, int $platformChannelId, string $communityName): void + public function syncChannelPosts(string $token, PlatformChannel $channel, int $communityId): void { try { $request = new LemmyRequest($this->instance, $token); $response = $request->get('post/list', [ - 'community_id' => $platformChannelId, + 'community_id' => $communityId, 'limit' => 50, 'sort' => 'New', ]); @@ -130,7 +130,7 @@ public function syncChannelPosts(string $token, int $platformChannelId, string $ if (! $response->successful()) { logger()->warning('Failed to sync channel posts', [ 'status' => $response->status(), - 'platform_channel_id' => $platformChannelId, + 'platform_channel_id' => $channel->id, ]); return; @@ -143,9 +143,7 @@ public function syncChannelPosts(string $token, int $platformChannelId, string $ $post = $postData['post']; PlatformChannelPost::storePost( - PlatformEnum::LEMMY, - (string) $platformChannelId, - $communityName, + $channel, (string) $post['id'], $post['url'] ?? null, $post['name'] ?? null, @@ -154,14 +152,14 @@ public function syncChannelPosts(string $token, int $platformChannelId, string $ } logger()->info('Synced channel posts', [ - 'platform_channel_id' => $platformChannelId, + 'platform_channel_id' => $channel->id, 'posts_count' => count($posts), ]); } catch (Exception $e) { logger()->error('Exception while syncing channel posts', [ 'error' => $e->getMessage(), - 'platform_channel_id' => $platformChannelId, + 'platform_channel_id' => $channel->id, ]); } } diff --git a/app/Services/Publishing/ArticlePublishingService.php b/app/Services/Publishing/ArticlePublishingService.php index 4d427fca..622e5cb7 100644 --- a/app/Services/Publishing/ArticlePublishingService.php +++ b/app/Services/Publishing/ArticlePublishingService.php @@ -39,7 +39,7 @@ protected function makePublisher(mixed $account): LemmyPublisher * * @throws PublishException */ - public function publishRouteArticle(RouteArticle $routeArticle, array $extractedData): ?ArticlePublication + public function publishRouteArticle(RouteArticle $routeArticle, array $extractedData): PublishOutcome { $article = $routeArticle->article; $channel = $routeArticle->platformChannel; @@ -60,7 +60,7 @@ public function publishRouteArticle(RouteArticle $routeArticle, array $extracted 'route_article_id' => $routeArticle->id, ]); - return null; + return PublishOutcome::failure('No active account for channel'); } return $this->publishToChannel($article, $extractedData, $channel, $account); @@ -69,7 +69,7 @@ public function publishRouteArticle(RouteArticle $routeArticle, array $extracted /** * @param array $extractedData */ - private function publishToChannel(Article $article, array $extractedData, PlatformChannel $channel, mixed $account): ?ArticlePublication + private function publishToChannel(Article $article, array $extractedData, PlatformChannel $channel, mixed $account): PublishOutcome { $lock = Cache::lock("publish:{$article->id}:{$channel->id}", self::LOCK_TTL_SECONDS); @@ -84,7 +84,7 @@ private function publishToChannel(Article $article, array $extractedData, Platfo 'article_id' => $article->id, ]); - return null; + return PublishOutcome::skipped('Already published to this channel'); } return $this->doPublishToChannel($article, $extractedData, $channel, $account); @@ -94,31 +94,26 @@ private function publishToChannel(Article $article, array $extractedData, Platfo 'article_id' => $article->id, ]); - return null; + return PublishOutcome::skipped('Another worker is publishing this article'); } } /** * @param array $extractedData */ - private function doPublishToChannel(Article $article, array $extractedData, PlatformChannel $channel, mixed $account): ?ArticlePublication + private function doPublishToChannel(Article $article, array $extractedData, PlatformChannel $channel, mixed $account): PublishOutcome { try { // Check if this URL or title was already posted to this channel $title = $extractedData['title'] ?? $article->title; - if (PlatformChannelPost::duplicateExists( - $channel->platformInstance->platform, - (string) $channel->channel_id, - $article->url, - $title - )) { + if (PlatformChannelPost::duplicateExists($channel, $article->url, $title)) { $this->logSaver->info('Skipping duplicate: URL or title already posted to channel', $channel, [ 'article_id' => $article->id, 'url' => $article->url, 'title' => $title, ]); - return null; + return PublishOutcome::skipped('URL or title already posted to this channel'); } $publisher = $this->makePublisher($account); @@ -138,14 +133,14 @@ private function doPublishToChannel(Article $article, array $extractedData, Plat 'article_id' => $article->id, ]); - return $publication; + return PublishOutcome::published($publication); } catch (Exception $e) { $this->logSaver->warning('Failed to publish to channel', $channel, [ 'article_id' => $article->id, 'error' => $e->getMessage(), ]); - return null; + return PublishOutcome::failure($e->getMessage()); } } } diff --git a/app/Services/Publishing/PublishOutcome.php b/app/Services/Publishing/PublishOutcome.php new file mode 100644 index 00000000..4584908e --- /dev/null +++ b/app/Services/Publishing/PublishOutcome.php @@ -0,0 +1,49 @@ +publication !== null; + } + + public function wasSkipped(): bool + { + return $this->skipped; + } + + public function failed(): bool + { + return ! $this->succeeded() && ! $this->skipped; + } +} diff --git a/database/migrations/2024_01_01_000013_key_platform_channel_posts_by_local_channel.php b/database/migrations/2024_01_01_000013_key_platform_channel_posts_by_local_channel.php new file mode 100644 index 00000000..7adc6548 --- /dev/null +++ b/database/migrations/2024_01_01_000013_key_platform_channel_posts_by_local_channel.php @@ -0,0 +1,83 @@ +dropUnique('channel_post_unique'); + $table->dropIndex(['platform', 'channel_id', 'url']); + $table->dropIndex(['platform', 'channel_id', 'title']); + $table->unsignedBigInteger('platform_channel_id')->nullable()->after('id'); + }); + + // A name shared by two instances is ambiguous; those rows stay unmapped. + DB::table('platform_channel_posts')->orderBy('id')->chunkById(200, function ($rows) { + foreach ($rows as $row) { + $matches = DB::table('platform_channels') + ->where('name', $row->channel_name) + ->orWhere('channel_id', $row->channel_name) + ->pluck('id'); + + if ($matches->count() !== 1) { + continue; + } + + DB::table('platform_channel_posts') + ->where('id', $row->id) + ->update(['platform_channel_id' => $matches->first()]); + } + }); + + // Unmappable rows are discarded rather than guessed: the mirror is a + // cache SyncChannelPostsJob rebuilds every ten minutes. + DB::table('platform_channel_posts')->whereNull('platform_channel_id')->delete(); + + Schema::table('platform_channel_posts', function (Blueprint $table) { + $table->unsignedBigInteger('platform_channel_id')->nullable(false)->change(); + $table->dropColumn(['platform', 'channel_id', 'channel_name']); + }); + + Schema::table('platform_channel_posts', function (Blueprint $table) { + $table->foreign('platform_channel_id')->references('id')->on('platform_channels')->onDelete('cascade'); + $table->unique(['platform_channel_id', 'post_id'], 'channel_post_unique'); + $table->index(['platform_channel_id', 'url']); + $table->index(['platform_channel_id', 'title']); + }); + } + + public function down(): void + { + Schema::table('platform_channel_posts', function (Blueprint $table) { + $table->dropForeign(['platform_channel_id']); + $table->dropUnique('channel_post_unique'); + $table->dropIndex(['platform_channel_id', 'url']); + $table->dropIndex(['platform_channel_id', 'title']); + $table->string('platform')->default('lemmy'); + $table->string('channel_id')->default(''); + $table->string('channel_name')->nullable(); + }); + + DB::table('platform_channel_posts')->update([ + 'channel_id' => DB::raw('platform_channel_id'), + ]); + + Schema::table('platform_channel_posts', function (Blueprint $table) { + $table->dropColumn('platform_channel_id'); + $table->unique(['platform', 'channel_id', 'post_id'], 'channel_post_unique'); + $table->index(['platform', 'channel_id', 'url']); + $table->index(['platform', 'channel_id', 'title']); + }); + } +}; diff --git a/database/migrations/2024_01_01_000014_add_skipped_to_route_articles_publish_status.php b/database/migrations/2024_01_01_000014_add_skipped_to_route_articles_publish_status.php new file mode 100644 index 00000000..ae4bd839 --- /dev/null +++ b/database/migrations/2024_01_01_000014_add_skipped_to_route_articles_publish_status.php @@ -0,0 +1,35 @@ +enum('publish_status', ['unpublished', 'publishing', 'published', 'skipped', 'error']) + ->default('unpublished') + ->change(); + }); + } + + public function down(): void + { + DB::table('route_articles') + ->where('publish_status', 'skipped') + ->update(['publish_status' => 'unpublished']); + + Schema::table('route_articles', function (Blueprint $table) { + $table->enum('publish_status', ['unpublished', 'publishing', 'published', 'error']) + ->default('unpublished') + ->change(); + }); + } +}; diff --git a/tests/Feature/DuplicatePublishTest.php b/tests/Feature/DuplicatePublishTest.php index a1d37646..892a3b69 100644 --- a/tests/Feature/DuplicatePublishTest.php +++ b/tests/Feature/DuplicatePublishTest.php @@ -2,6 +2,7 @@ namespace Tests\Feature; +use App\Actions\PublishRouteArticleAction; use App\Enums\PublishStatusEnum; use App\Events\RouteArticleApproved; use App\Listeners\PublishApprovedArticleListener; @@ -95,7 +96,7 @@ private function makeListener(): PublishApprovedArticleListener $fetcher = Mockery::mock(ArticleFetcher::class); $fetcher->shouldReceive('fetchArticleData')->andReturn(['title' => 'Test Article']); - return new PublishApprovedArticleListener($fetcher, $service, new NotificationService); + return new PublishApprovedArticleListener(new PublishRouteArticleAction($fetcher, $service, new NotificationService)); } public function test_clicking_approve_twice_creates_only_one_remote_post(): void @@ -142,7 +143,7 @@ public function test_two_queued_listeners_create_only_one_remote_post(): void $fetcher = Mockery::mock(ArticleFetcher::class); $fetcher->shouldReceive('fetchArticleData')->andReturn(['title' => 'Test Article']); - $listener = new PublishApprovedArticleListener($fetcher, $service, new NotificationService); + $listener = new PublishApprovedArticleListener(new PublishRouteArticleAction($fetcher, $service, new NotificationService)); $listener->handle(new RouteArticleApproved($routeArticle)); $this->assertSame(1, $this->remoteCalls, 'Two listeners must not both post to Lemmy.'); diff --git a/tests/Feature/KeyPlatformChannelPostsMigrationTest.php b/tests/Feature/KeyPlatformChannelPostsMigrationTest.php new file mode 100644 index 00000000..1febf897 --- /dev/null +++ b/tests/Feature/KeyPlatformChannelPostsMigrationTest.php @@ -0,0 +1,130 @@ +up(); + } + + private function restoreLegacyTable(): void + { + Schema::dropIfExists('platform_channel_posts'); + + Schema::create('platform_channel_posts', function (Blueprint $table) { + $table->id(); + $table->string('platform'); + $table->string('channel_id'); + $table->string('channel_name')->nullable(); + $table->string('post_id'); + $table->string('title')->nullable(); + $table->string('url')->nullable(); + $table->timestamp('posted_at')->nullable(); + $table->timestamps(); + + $table->unique(['platform', 'channel_id', 'post_id'], 'channel_post_unique'); + $table->index(['platform', 'channel_id', 'url']); + $table->index(['platform', 'channel_id', 'title']); + }); + } + + private function seedLegacyRow(string $channelId, ?string $channelName, string $postId): void + { + DB::table('platform_channel_posts')->insert([ + 'platform' => 'lemmy', + 'channel_id' => $channelId, + 'channel_name' => $channelName, + 'post_id' => $postId, + 'url' => "https://news.test/{$postId}", + 'title' => "Post {$postId}", + 'posted_at' => now(), + 'created_at' => now(), + 'updated_at' => now(), + ]); + } + + public function test_maps_rows_to_the_local_channel_by_name(): void + { + $this->restoreLegacyTable(); + + $channel = PlatformChannel::factory()->create(['name' => 'newsbottest', 'channel_id' => 'newsbottest']); + $this->seedLegacyRow('217', 'newsbottest', '1'); + $this->seedLegacyRow('217', 'newsbottest', '2'); + + $this->runMigration(); + + $this->assertSame(2, DB::table('platform_channel_posts') + ->where('platform_channel_id', $channel->id)->count()); + } + + public function test_drops_rows_that_match_no_channel(): void + { + $this->restoreLegacyTable(); + + PlatformChannel::factory()->create(['name' => 'newsbottest', 'channel_id' => 'newsbottest']); + $this->seedLegacyRow('999', 'a-community-that-no-longer-exists', '1'); + + $this->runMigration(); + + $this->assertSame(0, DB::table('platform_channel_posts')->count()); + } + + public function test_drops_rows_whose_channel_name_is_ambiguous_across_instances(): void + { + $this->restoreLegacyTable(); + + $first = PlatformInstance::factory()->create(['url' => 'https://one.test']); + $second = PlatformInstance::factory()->create(['url' => 'https://two.test']); + + PlatformChannel::factory()->create([ + 'platform_instance_id' => $first->id, + 'name' => 'news', + 'channel_id' => 'news', + ]); + PlatformChannel::factory()->create([ + 'platform_instance_id' => $second->id, + 'name' => 'news', + 'channel_id' => 'news-two', + ]); + + $this->seedLegacyRow('8', 'news', '1'); + + $this->runMigration(); + + $this->assertSame(0, DB::table('platform_channel_posts')->count()); + } + + public function test_leaves_a_schema_the_new_code_can_use(): void + { + $this->restoreLegacyTable(); + + $channel = PlatformChannel::factory()->create(['name' => 'newsbottest', 'channel_id' => 'newsbottest']); + $this->seedLegacyRow('217', 'newsbottest', '1'); + + $this->runMigration(); + + $this->assertTrue(Schema::hasColumn('platform_channel_posts', 'platform_channel_id')); + $this->assertFalse(Schema::hasColumn('platform_channel_posts', 'channel_id')); + $this->assertFalse(Schema::hasColumn('platform_channel_posts', 'channel_name')); + $this->assertFalse(Schema::hasColumn('platform_channel_posts', 'platform')); + + $this->assertTrue( + PlatformChannelPost::duplicateExists($channel, 'https://news.test/1', 'Post 1') + ); + } +} diff --git a/tests/Feature/Listeners/PublishApprovedArticleListenerTest.php b/tests/Feature/Listeners/PublishApprovedArticleListenerTest.php index fa0bb857..fcb09735 100644 --- a/tests/Feature/Listeners/PublishApprovedArticleListenerTest.php +++ b/tests/Feature/Listeners/PublishApprovedArticleListenerTest.php @@ -2,6 +2,7 @@ namespace Tests\Feature\Listeners; +use App\Actions\PublishRouteArticleAction; use App\Enums\NotificationSeverityEnum; use App\Enums\NotificationTypeEnum; use App\Events\RouteArticleApproved; @@ -15,6 +16,7 @@ use App\Services\Article\ArticleFetcher; use App\Services\Notification\NotificationService; use App\Services\Publishing\ArticlePublishingService; +use App\Services\Publishing\PublishOutcome; use Exception; use Illuminate\Foundation\Testing\RefreshDatabase; use Mockery; @@ -53,7 +55,7 @@ public function test_exception_during_publishing_creates_error_notification(): v $publishingServiceMock = Mockery::mock(ArticlePublishingService::class); - $listener = new PublishApprovedArticleListener($articleFetcherMock, $publishingServiceMock, new NotificationService); + $listener = new PublishApprovedArticleListener(new PublishRouteArticleAction($articleFetcherMock, $publishingServiceMock, new NotificationService)); $listener->handle(new RouteArticleApproved($routeArticle)); $this->assertDatabaseHas('notifications', [ @@ -82,9 +84,9 @@ public function test_no_publication_created_creates_warning_notification(): void $publishingServiceMock = Mockery::mock(ArticlePublishingService::class); $publishingServiceMock->shouldReceive('publishRouteArticle') ->once() - ->andReturn(null); + ->andReturn(PublishOutcome::failure('No publication created')); - $listener = new PublishApprovedArticleListener($articleFetcherMock, $publishingServiceMock, new NotificationService); + $listener = new PublishApprovedArticleListener(new PublishRouteArticleAction($articleFetcherMock, $publishingServiceMock, new NotificationService)); $listener->handle(new RouteArticleApproved($routeArticle)); $this->assertDatabaseHas('notifications', [ @@ -112,9 +114,9 @@ public function test_successful_publish_does_not_create_notification(): void $publishingServiceMock = Mockery::mock(ArticlePublishingService::class); $publishingServiceMock->shouldReceive('publishRouteArticle') ->once() - ->andReturn(ArticlePublication::factory()->make()); + ->andReturn(PublishOutcome::published($this->makePublication())); - $listener = new PublishApprovedArticleListener($articleFetcherMock, $publishingServiceMock, new NotificationService); + $listener = new PublishApprovedArticleListener(new PublishRouteArticleAction($articleFetcherMock, $publishingServiceMock, new NotificationService)); $listener->handle(new RouteArticleApproved($routeArticle)); $this->assertDatabaseCount('notifications', 0); @@ -135,7 +137,7 @@ public function test_skips_already_published_to_channel(): void $publishingServiceMock = Mockery::mock(ArticlePublishingService::class); $publishingServiceMock->shouldNotReceive('publishRouteArticle'); - $listener = new PublishApprovedArticleListener($articleFetcherMock, $publishingServiceMock, new NotificationService); + $listener = new PublishApprovedArticleListener(new PublishRouteArticleAction($articleFetcherMock, $publishingServiceMock, new NotificationService)); $listener->handle(new RouteArticleApproved($routeArticle)); $this->assertTrue(true); @@ -146,4 +148,12 @@ protected function tearDown(): void Mockery::close(); parent::tearDown(); } + + private function makePublication(): ArticlePublication + { + /** @var ArticlePublication $publication */ + $publication = ArticlePublication::factory()->make(); + + return $publication; + } } diff --git a/tests/Feature/MirrorDuplicateDetectionTest.php b/tests/Feature/MirrorDuplicateDetectionTest.php new file mode 100644 index 00000000..73c84e11 --- /dev/null +++ b/tests/Feature/MirrorDuplicateDetectionTest.php @@ -0,0 +1,197 @@ +create(); + $instance = PlatformInstance::factory()->create(['url' => 'https://lemmy.test']); + $channel = PlatformChannel::factory()->create([ + 'platform_instance_id' => $instance->id, + 'channel_id' => 'news', + 'name' => 'news', + ]); + $account = PlatformAccount::factory()->create(['instance_url' => 'https://lemmy.test']); + + /** @var Route $route */ + $route = Route::factory()->active()->create([ + 'feed_id' => $feed->id, + 'platform_channel_id' => $channel->id, + ]); + + $channel->platformAccounts()->attach($account->id, ['is_active' => true, 'priority' => 50]); + + $article = Article::factory()->create([ + 'feed_id' => $feed->id, + 'url' => 'https://news.test/already-posted', + ]); + + /** @var RouteArticle $routeArticle */ + $routeArticle = RouteArticle::factory()->forRoute($route)->approved()->create([ + 'article_id' => $article->id, + ]); + + $this->fixture = [$routeArticle, $channel, $article]; + } + + protected function tearDown(): void + { + Mockery::close(); + parent::tearDown(); + } + + private function service(?LemmyPublisher $publisher = null): ArticlePublishingService + { + $service = Mockery::mock(ArticlePublishingService::class, [app(LogSaver::class)])->makePartial(); + $service->shouldAllowMockingProtectedMethods(); + $service->shouldReceive('makePublisher')->andReturn( + $publisher ?? Mockery::mock(LemmyPublisher::class) + ); + + return $service; + } + + public function test_sync_writes_the_key_the_duplicate_check_reads(): void + { + [$routeArticle, $channel, $article] = $this->fixture; + + // Populate the mirror through the real sync path rather than seeding a + // row by hand, so write and read cannot silently disagree. + Http::fake([ + '*/api/v3/post/list*' => Http::response([ + 'posts' => [[ + 'post' => [ + 'id' => 555, + 'url' => $article->url, + 'name' => 'Already Posted', + 'published' => '2026-08-02T10:00:00Z', + ], + ]], + ]), + ]); + + (new LemmyApiService('https://lemmy.test')) + ->syncChannelPosts('token', $channel, 8); + + $this->assertDatabaseCount('platform_channel_posts', 1); + + $publisher = Mockery::mock(LemmyPublisher::class); + $publisher->shouldNotReceive('publishToChannel'); + + $result = $this->service($publisher)->publishRouteArticle($routeArticle, ['title' => 'Already Posted']); + + $this->assertTrue($result->wasSkipped()); + $this->assertDatabaseCount('article_publications', 0); + } + + public function test_a_skipped_duplicate_is_not_reported_as_a_publish_failure(): void + { + [$routeArticle, $channel, $article] = $this->fixture; + + PlatformChannelPost::storePost($channel, '555', $article->url, 'Already Posted'); + + $fetcher = Mockery::mock(ArticleFetcher::class); + $fetcher->shouldReceive('fetchArticleData')->andReturn(['title' => 'Already Posted']); + + $publisher = Mockery::mock(LemmyPublisher::class); + $publisher->shouldNotReceive('publishToChannel'); + + (new PublishApprovedArticleListener(new PublishRouteArticleAction($fetcher, $this->service($publisher), new NotificationService))) + ->handle(new RouteArticleApproved($routeArticle)); + + $this->assertSame(PublishStatusEnum::SKIPPED, $routeArticle->fresh()->publish_status); + $this->assertDatabaseCount('notifications', 0); + } + + public function test_a_genuine_failure_is_still_reported(): void + { + [$routeArticle] = $this->fixture; + + $fetcher = Mockery::mock(ArticleFetcher::class); + $fetcher->shouldReceive('fetchArticleData')->andReturn(['title' => 'Some Title']); + + $publisher = Mockery::mock(LemmyPublisher::class); + $publisher->shouldReceive('publishToChannel')->andThrow(new \RuntimeException('Lemmy rejected the post')); + + (new PublishApprovedArticleListener(new PublishRouteArticleAction($fetcher, $this->service($publisher), new NotificationService))) + ->handle(new RouteArticleApproved($routeArticle)); + + $this->assertSame(PublishStatusEnum::ERROR, $routeArticle->fresh()->publish_status); + $this->assertDatabaseHas('notifications', [ + 'type' => NotificationTypeEnum::PUBLISH_FAILED->value, + ]); + } + + public function test_mirror_is_keyed_by_the_local_channel(): void + { + [, $channel, $article] = $this->fixture; + + PlatformChannelPost::storePost( + $channel, + '555', + $article->url, + 'Already Posted', + ); + + $this->assertDatabaseHas('platform_channel_posts', [ + 'platform_channel_id' => $channel->id, + 'post_id' => '555', + ]); + } + + public function test_a_second_instance_with_the_same_community_name_does_not_collide(): void + { + [, $channel, $article] = $this->fixture; + + $otherInstance = PlatformInstance::factory()->create(['url' => 'https://other.test']); + $otherChannel = PlatformChannel::factory()->create([ + 'platform_instance_id' => $otherInstance->id, + 'channel_id' => 'news', + 'name' => 'news', + ]); + + PlatformChannelPost::storePost($channel, '1', $article->url, 'Same Title'); + + // Same community name on a different instance is a different community. + $this->assertFalse( + PlatformChannelPost::duplicateExists($otherChannel, $article->url, 'Same Title') + ); + + $this->assertTrue( + PlatformChannelPost::duplicateExists($channel, $article->url, 'Same Title') + ); + } +} diff --git a/tests/Unit/Jobs/PublishNextArticleJobTest.php b/tests/Unit/Jobs/PublishNextArticleJobTest.php index 66145951..28046481 100644 --- a/tests/Unit/Jobs/PublishNextArticleJobTest.php +++ b/tests/Unit/Jobs/PublishNextArticleJobTest.php @@ -2,6 +2,7 @@ namespace Tests\Unit\Jobs; +use App\Actions\PublishRouteArticleAction; use App\Enums\NotificationSeverityEnum; use App\Enums\NotificationTypeEnum; use App\Exceptions\PublishException; @@ -16,6 +17,7 @@ use App\Services\Article\ArticleFetcher; use App\Services\Notification\NotificationService; use App\Services\Publishing\ArticlePublishingService; +use App\Services\Publishing\PublishOutcome; use Illuminate\Contracts\Queue\ShouldBeUnique; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Queue\Queueable; @@ -95,7 +97,7 @@ public function test_handle_returns_early_when_no_approved_route_articles(): voi $publishingServiceMock = Mockery::mock(ArticlePublishingService::class); $job = new PublishNextArticleJob; - $job->handle($articleFetcherMock, $publishingServiceMock, $this->notificationService); + $job->handle(new PublishRouteArticleAction($articleFetcherMock, $publishingServiceMock, $this->notificationService)); $this->assertTrue(true); } @@ -114,7 +116,7 @@ public function test_handle_returns_early_when_no_unpublished_approved_route_art $publishingServiceMock = Mockery::mock(ArticlePublishingService::class); $job = new PublishNextArticleJob; - $job->handle($articleFetcherMock, $publishingServiceMock, $this->notificationService); + $job->handle(new PublishRouteArticleAction($articleFetcherMock, $publishingServiceMock, $this->notificationService)); $this->assertTrue(true); } @@ -132,7 +134,7 @@ public function test_handle_skips_non_approved_route_articles(): void $publishingServiceMock = Mockery::mock(ArticlePublishingService::class); $job = new PublishNextArticleJob; - $job->handle($articleFetcherMock, $publishingServiceMock, $this->notificationService); + $job->handle(new PublishRouteArticleAction($articleFetcherMock, $publishingServiceMock, $this->notificationService)); $this->assertTrue(true); } @@ -170,10 +172,10 @@ public function test_handle_publishes_oldest_approved_route_article(): void Mockery::on(fn ($ra) => $ra->article_id === $olderArticle->id), $extractedData ) - ->andReturn(ArticlePublication::factory()->make()); + ->andReturn(PublishOutcome::published($this->makePublication())); $job = new PublishNextArticleJob; - $job->handle($articleFetcherMock, $publishingServiceMock, $this->notificationService); + $job->handle(new PublishRouteArticleAction($articleFetcherMock, $publishingServiceMock, $this->notificationService)); $this->assertTrue(true); } @@ -200,7 +202,7 @@ public function test_handle_throws_exception_on_publishing_failure(): void $this->expectException(PublishException::class); - $job->handle($articleFetcherMock, $publishingServiceMock, $this->notificationService); + $job->handle(new PublishRouteArticleAction($articleFetcherMock, $publishingServiceMock, $this->notificationService)); } public function test_handle_skips_publishing_when_last_publication_within_interval(): void @@ -219,7 +221,7 @@ public function test_handle_skips_publishing_when_last_publication_within_interv $publishingServiceMock->shouldNotReceive('publishRouteArticle'); $job = new PublishNextArticleJob; - $job->handle($articleFetcherMock, $publishingServiceMock, $this->notificationService); + $job->handle(new PublishRouteArticleAction($articleFetcherMock, $publishingServiceMock, $this->notificationService)); $this->assertTrue(true); } @@ -243,10 +245,10 @@ public function test_handle_publishes_when_last_publication_beyond_interval(): v $publishingServiceMock = Mockery::mock(ArticlePublishingService::class); $publishingServiceMock->shouldReceive('publishRouteArticle') ->once() - ->andReturn(ArticlePublication::factory()->make()); + ->andReturn(PublishOutcome::published($this->makePublication())); $job = new PublishNextArticleJob; - $job->handle($articleFetcherMock, $publishingServiceMock, $this->notificationService); + $job->handle(new PublishRouteArticleAction($articleFetcherMock, $publishingServiceMock, $this->notificationService)); $this->assertTrue(true); } @@ -270,10 +272,10 @@ public function test_handle_publishes_when_interval_is_zero(): void $publishingServiceMock = Mockery::mock(ArticlePublishingService::class); $publishingServiceMock->shouldReceive('publishRouteArticle') ->once() - ->andReturn(ArticlePublication::factory()->make()); + ->andReturn(PublishOutcome::published($this->makePublication())); $job = new PublishNextArticleJob; - $job->handle($articleFetcherMock, $publishingServiceMock, $this->notificationService); + $job->handle(new PublishRouteArticleAction($articleFetcherMock, $publishingServiceMock, $this->notificationService)); $this->assertTrue(true); } @@ -297,10 +299,10 @@ public function test_handle_publishes_when_last_publication_exactly_at_interval( $publishingServiceMock = Mockery::mock(ArticlePublishingService::class); $publishingServiceMock->shouldReceive('publishRouteArticle') ->once() - ->andReturn(ArticlePublication::factory()->make()); + ->andReturn(PublishOutcome::published($this->makePublication())); $job = new PublishNextArticleJob; - $job->handle($articleFetcherMock, $publishingServiceMock, $this->notificationService); + $job->handle(new PublishRouteArticleAction($articleFetcherMock, $publishingServiceMock, $this->notificationService)); $this->assertTrue(true); } @@ -321,10 +323,10 @@ public function test_handle_publishes_when_no_previous_publications_exist(): voi $publishingServiceMock = Mockery::mock(ArticlePublishingService::class); $publishingServiceMock->shouldReceive('publishRouteArticle') ->once() - ->andReturn(ArticlePublication::factory()->make()); + ->andReturn(PublishOutcome::published($this->makePublication())); $job = new PublishNextArticleJob; - $job->handle($articleFetcherMock, $publishingServiceMock, $this->notificationService); + $job->handle(new PublishRouteArticleAction($articleFetcherMock, $publishingServiceMock, $this->notificationService)); $this->assertTrue(true); } @@ -343,10 +345,10 @@ public function test_handle_creates_warning_notification_when_no_publication_cre $publishingServiceMock = Mockery::mock(ArticlePublishingService::class); $publishingServiceMock->shouldReceive('publishRouteArticle') ->once() - ->andReturn(null); + ->andReturn(PublishOutcome::failure('No publication created')); $job = new PublishNextArticleJob; - $job->handle($articleFetcherMock, $publishingServiceMock, $this->notificationService); + $job->handle(new PublishRouteArticleAction($articleFetcherMock, $publishingServiceMock, $this->notificationService)); $this->assertDatabaseHas('notifications', [ 'type' => NotificationTypeEnum::PUBLISH_FAILED->value, @@ -380,7 +382,7 @@ public function test_handle_creates_notification_on_publish_exception(): void $job = new PublishNextArticleJob; try { - $job->handle($articleFetcherMock, $publishingServiceMock, $this->notificationService); + $job->handle(new PublishRouteArticleAction($articleFetcherMock, $publishingServiceMock, $this->notificationService)); } catch (PublishException) { // Expected } @@ -413,4 +415,12 @@ protected function tearDown(): void Mockery::close(); parent::tearDown(); } + + private function makePublication(): ArticlePublication + { + /** @var ArticlePublication $publication */ + $publication = ArticlePublication::factory()->make(); + + return $publication; + } } diff --git a/tests/Unit/Jobs/SyncChannelPostsJobTest.php b/tests/Unit/Jobs/SyncChannelPostsJobTest.php index 62fcc10e..946c48a9 100644 --- a/tests/Unit/Jobs/SyncChannelPostsJobTest.php +++ b/tests/Unit/Jobs/SyncChannelPostsJobTest.php @@ -148,7 +148,7 @@ public function test_sync_resolves_non_numeric_channel_id_via_get_community_id() ->andReturn(42); $apiMock->shouldReceive('syncChannelPosts') ->once() - ->with('token', 42, $channel->name); + ->with('token', Mockery::on(fn ($arg) => $arg->is($channel)), 42); $logSaverMock = Mockery::mock(LogSaver::class); $logSaverMock->shouldReceive('info')->zeroOrMoreTimes(); @@ -179,7 +179,7 @@ public function test_sync_passes_resolved_community_id_to_sync_channel_posts(): ->andReturn(42); $apiMock->shouldReceive('syncChannelPosts') ->once() - ->with('token', 42, $channel->name); + ->with('token', Mockery::on(fn ($arg) => $arg->is($channel)), 42); $logSaverMock = Mockery::mock(LogSaver::class); $logSaverMock->shouldReceive('info')->zeroOrMoreTimes(); diff --git a/tests/Unit/Modules/Lemmy/Services/LemmyApiServiceTest.php b/tests/Unit/Modules/Lemmy/Services/LemmyApiServiceTest.php index 9d50ead0..f807bba1 100644 --- a/tests/Unit/Modules/Lemmy/Services/LemmyApiServiceTest.php +++ b/tests/Unit/Modules/Lemmy/Services/LemmyApiServiceTest.php @@ -2,7 +2,7 @@ namespace Tests\Unit\Modules\Lemmy\Services; -use App\Enums\PlatformEnum; +use App\Models\PlatformChannel; use App\Modules\Lemmy\Services\LemmyApiService; use Exception; use Illuminate\Foundation\Testing\RefreshDatabase; @@ -13,6 +13,13 @@ class LemmyApiServiceTest extends TestCase { use RefreshDatabase; + private ?PlatformChannel $channel = null; + + private function syncChannel(): PlatformChannel + { + return $this->channel ??= PlatformChannel::factory()->create(); + } + public function test_constructor_sets_instance(): void { $service = new LemmyApiService('lemmy.world'); @@ -248,7 +255,7 @@ public function test_sync_channel_posts_success(): void ]); $service = new LemmyApiService('lemmy.world'); - $service->syncChannelPosts('token', 42, 'test-community'); + $service->syncChannelPosts('token', $this->syncChannel(), 42); Http::assertSent(function ($request) { return str_contains($request->url(), '/api/v3/post/list') @@ -259,18 +266,14 @@ public function test_sync_channel_posts_success(): void // Verify posts were stored in the database $this->assertDatabaseHas('platform_channel_posts', [ - 'platform' => PlatformEnum::LEMMY->value, - 'channel_id' => '42', - 'channel_name' => 'test-community', + 'platform_channel_id' => $this->syncChannel()->id, 'post_id' => '1', 'url' => 'https://example.com/1', 'title' => 'Post 1', ]); $this->assertDatabaseHas('platform_channel_posts', [ - 'platform' => PlatformEnum::LEMMY->value, - 'channel_id' => '42', - 'channel_name' => 'test-community', + 'platform_channel_id' => $this->syncChannel()->id, 'post_id' => '2', 'url' => 'https://example.com/2', 'title' => 'Post 2', @@ -284,7 +287,7 @@ public function test_sync_channel_posts_handles_unsuccessful_response(): void ]); $service = new LemmyApiService('lemmy.world'); - $service->syncChannelPosts('token', 42, 'test-community'); + $service->syncChannelPosts('token', $this->syncChannel(), 42); Http::assertSentCount(1); $this->assertDatabaseCount('platform_channel_posts', 0); @@ -297,7 +300,7 @@ public function test_sync_channel_posts_handles_exception(): void }); $service = new LemmyApiService('lemmy.world'); - $service->syncChannelPosts('token', 42, 'test-community'); + $service->syncChannelPosts('token', $this->syncChannel(), 42); // Assert that the method completes without throwing $this->assertTrue(true); diff --git a/tests/Unit/Services/Publishing/ArticlePublishingServiceTest.php b/tests/Unit/Services/Publishing/ArticlePublishingServiceTest.php index cfdfd9f4..0ea074b2 100644 --- a/tests/Unit/Services/Publishing/ArticlePublishingServiceTest.php +++ b/tests/Unit/Services/Publishing/ArticlePublishingServiceTest.php @@ -2,7 +2,6 @@ namespace Tests\Unit\Services\Publishing; -use App\Enums\PlatformEnum; use App\Models\Article; use App\Models\ArticlePublication; use App\Models\Feed; @@ -99,7 +98,7 @@ public function test_publish_route_article_returns_null_when_no_active_account() $result = $this->service->publishRouteArticle($routeArticle, ['title' => 'Test']); - $this->assertNull($result); + $this->assertTrue($result->failed()); $this->assertDatabaseCount('article_publications', 0); } @@ -118,7 +117,7 @@ public function test_publish_route_article_successfully_publishes(): void $result = $service->publishRouteArticle($routeArticle, ['title' => 'Hello']); - $this->assertNotNull($result); + $this->assertTrue($result->succeeded()); $this->assertDatabaseHas('article_publications', [ 'article_id' => $article->id, 'platform_channel_id' => $channel->id, @@ -190,7 +189,7 @@ public function test_losing_the_lock_race_skips_without_publishing(): void // caller's catch block and be recorded as a publish failure. $result = $service->publishRouteArticle($routeArticle, ['title' => 'Hello']); - $this->assertNull($result); + $this->assertTrue($result->wasSkipped()); $this->assertDatabaseCount('article_publications', 0); } @@ -209,7 +208,7 @@ public function test_publish_route_article_handles_publishing_failure_gracefully $result = $service->publishRouteArticle($routeArticle, ['title' => 'Hello']); - $this->assertNull($result); + $this->assertTrue($result->failed()); $this->assertDatabaseCount('article_publications', 0); } @@ -219,9 +218,7 @@ public function test_publish_skips_duplicate_when_url_already_posted_to_channel( // Simulate the URL already being posted to this channel PlatformChannelPost::storePost( - PlatformEnum::LEMMY, - (string) $channel->channel_id, - $channel->name, + $channel, '999', $article->url, 'Different Title', @@ -236,7 +233,7 @@ public function test_publish_skips_duplicate_when_url_already_posted_to_channel( $result = $service->publishRouteArticle($routeArticle, ['title' => 'Some Title']); - $this->assertNull($result); + $this->assertTrue($result->wasSkipped()); $this->assertDatabaseCount('article_publications', 0); } @@ -246,9 +243,7 @@ public function test_publish_skips_duplicate_when_title_already_posted_to_channe // Simulate the same title already posted with a different URL PlatformChannelPost::storePost( - PlatformEnum::LEMMY, - (string) $channel->channel_id, - $channel->name, + $channel, '888', 'https://example.com/different-url', 'Breaking News', @@ -263,7 +258,7 @@ public function test_publish_skips_duplicate_when_title_already_posted_to_channe $result = $service->publishRouteArticle($routeArticle, ['title' => 'Breaking News']); - $this->assertNull($result); + $this->assertTrue($result->wasSkipped()); $this->assertDatabaseCount('article_publications', 0); } @@ -273,9 +268,7 @@ public function test_publish_proceeds_when_no_duplicate_exists(): void // Existing post in the channel has a completely different URL and title PlatformChannelPost::storePost( - PlatformEnum::LEMMY, - (string) $channel->channel_id, - $channel->name, + $channel, '777', 'https://example.com/other-article', 'Totally Different Title', @@ -292,7 +285,7 @@ public function test_publish_proceeds_when_no_duplicate_exists(): void $result = $service->publishRouteArticle($routeArticle, ['title' => 'Unique Title']); - $this->assertNotNull($result); + $this->assertTrue($result->succeeded()); $this->assertDatabaseHas('article_publications', [ 'article_id' => $article->id, 'post_id' => 456, -- 2.45.2 From 358171c8cc434bb7a973e3b498f581b6d3e65b9d Mon Sep 17 00:00:00 2001 From: myrmidex Date: Sun, 2 Aug 2026 17:57:12 +0200 Subject: [PATCH 3/5] 123 - Start the scheduler in the production container --- Dockerfile | 3 ++ Dockerfile.dev | 3 ++ docker/build/entrypoint.sh | 51 ---------------------------- docker/build/laravel.env | 59 --------------------------------- docker/build/wait-for-db.php | 14 -------- docker/build/wait-for-redis.php | 11 ------ 6 files changed, 6 insertions(+), 135 deletions(-) delete mode 100644 docker/build/entrypoint.sh delete mode 100644 docker/build/laravel.env delete mode 100644 docker/build/wait-for-db.php delete mode 100644 docker/build/wait-for-redis.php diff --git a/Dockerfile b/Dockerfile index 9fd071d8..9ce1a488 100644 --- a/Dockerfile +++ b/Dockerfile @@ -101,6 +101,9 @@ php artisan db:seed --force || echo "Seeders failed or already run" # Start Horizon in the background php artisan horizon & +# Start the scheduler in the background +php artisan schedule:work & + # Start FrankenPHP exec frankenphp run --config /etc/caddy/Caddyfile EOF diff --git a/Dockerfile.dev b/Dockerfile.dev index 124ace16..41c44083 100644 --- a/Dockerfile.dev +++ b/Dockerfile.dev @@ -114,6 +114,9 @@ npm run dev & # Start Horizon (queue worker) in background php artisan horizon & +# Scheduler left off in dev on purpose; run schedule:work by hand when needed. +# php artisan schedule:work & + # Start FrankenPHP exec frankenphp run --config /etc/caddy/Caddyfile EOF diff --git a/docker/build/entrypoint.sh b/docker/build/entrypoint.sh deleted file mode 100644 index 151124eb..00000000 --- a/docker/build/entrypoint.sh +++ /dev/null @@ -1,51 +0,0 @@ -#!/bin/sh - -# Exit on any error -set -e - -# Check required Lemmy environment variables -if [ -z "$LEMMY_INSTANCE" ] || [ -z "$LEMMY_USERNAME" ] || [ -z "$LEMMY_PASSWORD" ] || [ -z "$LEMMY_COMMUNITY" ]; then - echo "ERROR: Missing required Lemmy configuration variables:" - echo " LEMMY_INSTANCE=${LEMMY_INSTANCE:-'(not set)'}" - echo " LEMMY_USERNAME=${LEMMY_USERNAME:-'(not set)'}" - echo " LEMMY_PASSWORD=${LEMMY_PASSWORD:-'(not set)'}" - echo " LEMMY_COMMUNITY=${LEMMY_COMMUNITY:-'(not set)'}" - echo "Please set all required environment variables before starting the application." - exit 1 -fi - -# Wait for database to be ready -echo "Waiting for database connection..." -until php /docker/wait-for-db.php > /dev/null 2>&1; do - echo "Database not ready, waiting..." - sleep 5 -done -echo "Database connection established." - -# Wait for Redis to be ready -echo "Waiting for Redis connection..." -until php /docker/wait-for-redis.php > /dev/null 2>&1; do - echo "Redis not ready, waiting..." - sleep 2 -done -echo "Redis connection established." - -# Substitute environment variables in .env file -echo "Configuring environment variables..." -envsubst < .env > .env.tmp && mv .env.tmp .env - -# Run migrations and initial setup -echo "Running database migrations..." -php artisan migrate --force - -echo "Dispatching initial sync job..." -php artisan tinker --execute="App\\Jobs\\SyncChannelPostsJob::dispatchForLemmy();" - -# Start all services in single container -echo "Starting web server, scheduler, and Horizon..." -php artisan schedule:work & -php artisan horizon & -php artisan serve --host=0.0.0.0 --port=8000 & - -# Wait for any process to exit -wait \ No newline at end of file diff --git a/docker/build/laravel.env b/docker/build/laravel.env deleted file mode 100644 index fab9ef82..00000000 --- a/docker/build/laravel.env +++ /dev/null @@ -1,59 +0,0 @@ -APP_NAME="Lemmy Poster" -APP_ENV=production -APP_KEY= -APP_DEBUG=true -APP_URL=http://localhost - -APP_LOCALE=en -APP_FALLBACK_LOCALE=en -APP_FAKER_LOCALE=en_US - -APP_MAINTENANCE_DRIVER=file - -PHP_CLI_SERVER_WORKERS=4 - -BCRYPT_ROUNDS=12 - -LOG_CHANNEL=stack -LOG_STACK=single -LOG_DEPRECATIONS_CHANNEL=null -LOG_LEVEL=error - -DB_CONNECTION=mysql -DB_HOST=mysql -DB_PORT=3306 -DB_DATABASE=$DB_DATABASE -DB_USERNAME=$DB_USERNAME -DB_PASSWORD=$DB_PASSWORD - -SESSION_DRIVER=redis -SESSION_LIFETIME=120 -SESSION_ENCRYPT=false -SESSION_PATH=/ -SESSION_DOMAIN=null - -BROADCAST_CONNECTION=log -FILESYSTEM_DISK=local -QUEUE_CONNECTION=redis - -CACHE_STORE=redis - -REDIS_CLIENT=phpredis -REDIS_HOST=redis -REDIS_PASSWORD=null -REDIS_PORT=6379 - -MAIL_MAILER=log -MAIL_SCHEME=null -MAIL_HOST=127.0.0.1 -MAIL_PORT=2525 -MAIL_USERNAME=null -MAIL_PASSWORD=null -MAIL_FROM_ADDRESS="hello@example.com" -MAIL_FROM_NAME="${APP_NAME}" - -# LEMMY SETTINGS -LEMMY_INSTANCE= -LEMMY_USERNAME= -LEMMY_PASSWORD= -LEMMY_COMMUNITY= diff --git a/docker/build/wait-for-db.php b/docker/build/wait-for-db.php deleted file mode 100644 index 2427fa84..00000000 --- a/docker/build/wait-for-db.php +++ /dev/null @@ -1,14 +0,0 @@ -#!/usr/bin/env php -connect('redis', 6379); - echo 'Connected'; - exit(0); -} catch (Exception $e) { - exit(1); -} \ No newline at end of file -- 2.45.2 From d0d81e524ece6af129345bb59e529c9d9e98ae2e Mon Sep 17 00:00:00 2001 From: myrmidex Date: Thu, 6 Aug 2026 00:20:49 +0200 Subject: [PATCH 4/5] 114 - Select channel communities from the instance instead of typing a slug --- app/Actions/CreateChannelAction.php | 6 +- .../Api/V1/PlatformChannelsController.php | 8 +- .../Requests/StorePlatformChannelRequest.php | 48 +++-- app/Jobs/SyncChannelPostsJob.php | 4 +- app/Livewire/Channels.php | 69 ++++-- app/Livewire/Onboarding.php | 68 +++++- app/Models/PlatformChannel.php | 3 +- .../Lemmy/Services/LemmyApiService.php | 37 +++- app/Modules/Lemmy/Services/LemmyPublisher.php | 4 +- app/Services/Platform/CommunityDirectory.php | 51 +++++ database/factories/PlatformChannelFactory.php | 3 +- ...eric_community_id_on_platform_channels.php | 99 +++++++++ resources/views/livewire/channels.blade.php | 42 ++-- resources/views/livewire/onboarding.blade.php | 50 +++-- tests/Feature/CommunityDirectoryTest.php | 82 +++++++ tests/Feature/DuplicatePublishTest.php | 65 +++++- .../Api/V1/PlatformChannelsControllerTest.php | 71 ++++-- tests/Feature/Livewire/ChannelsTest.php | 80 +++++-- tests/Feature/Livewire/OnboardingTest.php | 202 ++++++++++++++++++ .../Feature/StoreCommunityIdMigrationTest.php | 110 ++++++++++ .../Unit/Actions/CreateChannelActionTest.php | 22 +- tests/Unit/Jobs/SyncChannelPostsJobTest.php | 21 +- tests/Unit/Models/PlatformChannelTest.php | 16 +- .../Lemmy/Services/LemmyApiServiceTest.php | 45 ++-- .../Lemmy/Services/LemmyPublisherTest.php | 30 +-- 25 files changed, 1043 insertions(+), 193 deletions(-) create mode 100644 app/Services/Platform/CommunityDirectory.php create mode 100644 database/migrations/2024_01_01_000015_store_numeric_community_id_on_platform_channels.php create mode 100644 tests/Feature/CommunityDirectoryTest.php create mode 100644 tests/Feature/Livewire/OnboardingTest.php create mode 100644 tests/Feature/StoreCommunityIdMigrationTest.php diff --git a/app/Actions/CreateChannelAction.php b/app/Actions/CreateChannelAction.php index 5fdd3183..bd7e4378 100644 --- a/app/Actions/CreateChannelAction.php +++ b/app/Actions/CreateChannelAction.php @@ -10,7 +10,7 @@ class CreateChannelAction { - public function execute(string $name, int $platformInstanceId, ?int $languageId = null, ?string $description = null): PlatformChannel + public function execute(string $name, int $communityId, int $platformInstanceId, ?int $languageId = null, ?string $description = null): PlatformChannel { $platformInstance = PlatformInstance::findOrFail($platformInstanceId); @@ -22,10 +22,10 @@ public function execute(string $name, int $platformInstanceId, ?int $languageId throw new RuntimeException('No active platform accounts found for this instance. Please create a platform account first.'); } - return DB::transaction(function () use ($name, $platformInstanceId, $languageId, $description, $activeAccounts) { + return DB::transaction(function () use ($name, $communityId, $platformInstanceId, $languageId, $description, $activeAccounts) { $channel = PlatformChannel::create([ 'platform_instance_id' => $platformInstanceId, - 'channel_id' => $name, + 'channel_id' => $communityId, 'name' => $name, 'display_name' => ucfirst($name), 'description' => $description, diff --git a/app/Http/Controllers/Api/V1/PlatformChannelsController.php b/app/Http/Controllers/Api/V1/PlatformChannelsController.php index 443ba540..22c349ea 100644 --- a/app/Http/Controllers/Api/V1/PlatformChannelsController.php +++ b/app/Http/Controllers/Api/V1/PlatformChannelsController.php @@ -7,6 +7,8 @@ use App\Http\Resources\PlatformChannelResource; use App\Models\PlatformAccount; use App\Models\PlatformChannel; +use App\Models\PlatformInstance; +use App\Services\Platform\CommunityDirectory; use Exception; use Illuminate\Database\UniqueConstraintViolationException; use Illuminate\Http\JsonResponse; @@ -40,8 +42,12 @@ public function store(StorePlatformChannelRequest $request, CreateChannelAction try { $validated = $request->validated(); + $instance = PlatformInstance::query()->findOrFail((int) $validated['platform_instance_id']); + $name = app(CommunityDirectory::class)->name($instance, (int) $validated['channel_id']); + $channel = $createChannelAction->execute( - $validated['name'], + $name, + (int) $validated['channel_id'], $validated['platform_instance_id'], $validated['language_id'] ?? null, $validated['description'] ?? null, diff --git a/app/Http/Requests/StorePlatformChannelRequest.php b/app/Http/Requests/StorePlatformChannelRequest.php index 0a1af8c6..03453bb8 100644 --- a/app/Http/Requests/StorePlatformChannelRequest.php +++ b/app/Http/Requests/StorePlatformChannelRequest.php @@ -2,6 +2,9 @@ namespace App\Http\Requests; +use App\Models\PlatformInstance; +use App\Services\Platform\CommunityDirectory; +use Exception; use Illuminate\Foundation\Http\FormRequest; use Illuminate\Validation\Rule; @@ -17,19 +20,22 @@ public function authorize(): bool */ public function rules(): array { + try { + $communityRules = [ + Rule::in($this->communityIds()), + Rule::unique('platform_channels', 'channel_id') + ->where('platform_instance_id', $this->input('platform_instance_id')), + ]; + } catch (Exception $e) { + // Falling through to Rule::in([]) would report the community as non-existent + // when the truth is we never reached the instance to check. + $message = 'Could not reach this instance to list its communities: '.$e->getMessage(); + $communityRules = [fn ($attribute, $value, $fail) => $fail($message)]; + } + return [ 'platform_instance_id' => 'required|exists:platform_instances,id', - // name doubles as the Lemmy community slug (CreateChannelAction copies it - // verbatim into channel_id for community lookup at publish time), so it must - // be slug format and unique per instance — matching the Livewire create form. - 'name' => [ - 'required', - 'string', - 'max:255', - 'regex:/^[a-z0-9_]+$/', - Rule::unique('platform_channels', 'name') - ->where('platform_instance_id', $this->input('platform_instance_id')), - ], + 'channel_id' => ['required', 'integer', ...$communityRules], 'language_id' => 'nullable|exists:languages,id', 'description' => 'nullable|string', ]; @@ -41,8 +47,24 @@ public function rules(): array public function messages(): array { return [ - 'name.regex' => 'The name must be a valid community slug (lowercase letters, numbers, and underscores only).', - 'name.unique' => 'A channel with this name already exists for this instance.', + 'channel_id.in' => 'That community does not exist on the selected instance.', + 'channel_id.unique' => 'A channel for this community already exists.', ]; } + + /** + * @return array + */ + private function communityIds(): array + { + $instance = PlatformInstance::query()->find((int) $this->input('platform_instance_id')); + + if (! $instance) { + return []; + } + + return collect(app(CommunityDirectory::class)->forInstance($instance)) + ->pluck('id') + ->all(); + } } diff --git a/app/Jobs/SyncChannelPostsJob.php b/app/Jobs/SyncChannelPostsJob.php index 96043a2e..ba1ebaf1 100644 --- a/app/Jobs/SyncChannelPostsJob.php +++ b/app/Jobs/SyncChannelPostsJob.php @@ -68,9 +68,7 @@ private function syncLemmyChannelPosts(LogSaver $logSaver): void $api = $this->makeApiService($this->channel->platformInstance->url); $token = $this->getAuthToken($api, $account); - $communityId = $api->resolveCommunityId($this->channel->channel_id, $token); - - $api->syncChannelPosts($token, $this->channel, $communityId); + $api->syncChannelPosts($token, $this->channel, $this->channel->channel_id); $logSaver->info('Channel posts synced successfully', $this->channel); } catch (Exception $e) { diff --git a/app/Livewire/Channels.php b/app/Livewire/Channels.php index 0b785186..ebb7099d 100644 --- a/app/Livewire/Channels.php +++ b/app/Livewire/Channels.php @@ -7,6 +7,8 @@ use App\Models\PlatformAccount; use App\Models\PlatformChannel; use App\Models\PlatformInstance; +use App\Services\Platform\CommunityDirectory; +use Exception; use Illuminate\Contracts\View\View; use Illuminate\Database\UniqueConstraintViolationException; use Illuminate\Validation\Rule; @@ -19,10 +21,15 @@ class Channels extends Component public bool $showCreateModal = false; - public string $newName = ''; + public ?int $newCommunityId = null; public ?int $newPlatformInstanceId = null; + /** @var array */ + public array $availableCommunities = []; + + public ?string $communityLoadError = null; + public ?int $newLanguageId = null; public string $newDescription = ''; @@ -36,11 +43,44 @@ public function toggle(int $channelId): void public function openCreateModal(): void { - $this->reset(['newName', 'newPlatformInstanceId', 'newLanguageId', 'newDescription']); + $this->reset(['newCommunityId', 'newPlatformInstanceId', 'newLanguageId', 'newDescription', 'availableCommunities', 'communityLoadError']); $this->resetErrorBag(); $this->showCreateModal = true; } + public function updatedNewPlatformInstanceId(?int $value): void + { + $this->reset(['newCommunityId', 'availableCommunities', 'communityLoadError']); + + if (! $value) { + return; + } + + $instance = PlatformInstance::find($value); + + if (! $instance) { + return; + } + + try { + $this->availableCommunities = app(CommunityDirectory::class)->forInstance($instance); + } catch (Exception $e) { + $this->communityLoadError = 'Could not reach this instance to list its communities: '.$e->getMessage(); + } + } + + public function refreshCommunities(): void + { + $instance = $this->newPlatformInstanceId ? PlatformInstance::find($this->newPlatformInstanceId) : null; + + if (! $instance) { + return; + } + + app(CommunityDirectory::class)->forget($instance); + $this->updatedNewPlatformInstanceId($this->newPlatformInstanceId); + } + public function closeCreateModal(): void { $this->showCreateModal = false; @@ -49,36 +89,33 @@ public function closeCreateModal(): void public function createChannel(CreateChannelAction $action): void { $this->validate([ - // name doubles as the Lemmy community slug (used verbatim as channel_id for - // community lookup at publish time), so it must be lowercase slug format. - 'newName' => [ + 'newCommunityId' => [ 'required', - 'string', - 'max:255', - 'regex:/^[a-z0-9_]+$/', - Rule::unique('platform_channels', 'name') + 'integer', + Rule::in(collect($this->availableCommunities)->pluck('id')->all()), + Rule::unique('platform_channels', 'channel_id') ->where('platform_instance_id', $this->newPlatformInstanceId), ], 'newPlatformInstanceId' => 'required|integer|exists:platform_instances,id', 'newLanguageId' => 'nullable|integer|exists:languages,id', ], [ - 'newName.regex' => 'The name must be a valid community slug (lowercase letters, numbers, and underscores only).', - 'newName.unique' => 'A channel with this name already exists for this instance.', + 'newCommunityId.in' => 'Select a community from this instance.', + 'newCommunityId.unique' => 'A channel for this community already exists.', ]); + $name = collect($this->availableCommunities)->firstWhere('id', $this->newCommunityId)['name'] ?? null; + try { $action->execute( - $this->newName, + $name, + $this->newCommunityId, $this->newPlatformInstanceId, $this->newLanguageId, // Blade textarea binds an empty string when blank; the action expects null for "no description". $this->newDescription !== '' ? $this->newDescription : null, ); } catch (UniqueConstraintViolationException $e) { - // Unreachable via this form (the unique rule above catches duplicates first), - // but the (platform_instance_id, channel_id) index can still fire if channel_id - // ever drifts from name. Surface it as a field error instead of a 500. - $this->addError('newName', 'A channel with this name already exists for this instance.'); + $this->addError('newCommunityId', 'A channel for this community already exists.'); return; } catch (RuntimeException $e) { diff --git a/app/Livewire/Onboarding.php b/app/Livewire/Onboarding.php index 919514ea..2e1a88a2 100644 --- a/app/Livewire/Onboarding.php +++ b/app/Livewire/Onboarding.php @@ -17,8 +17,10 @@ use App\Models\Route; use App\Models\Setting; use App\Services\OnboardingService; +use App\Services\Platform\CommunityDirectory; use Exception; use Illuminate\Contracts\View\View; +use Illuminate\Validation\Rule; use InvalidArgumentException; use Livewire\Attributes\Locked; use Livewire\Component; @@ -49,7 +51,12 @@ class Onboarding extends Component public string $feedDescription = ''; // Channel form - public string $channelName = ''; + public ?int $channelCommunityId = null; + + /** @var array */ + public array $availableCommunities = []; + + public ?string $communityLoadError = null; public ?int $platformInstanceId = null; @@ -117,10 +124,11 @@ public function mount(): void // Pre-fill channel form if exists $channel = PlatformChannel::where('is_active', true)->first(); if ($channel) { - $this->channelName = $channel->name; $this->platformInstanceId = $channel->platform_instance_id; $this->channelLanguageId = $channel->language_id; $this->channelDescription = $channel->description ?? ''; + $this->loadCommunities(); + $this->channelCommunityId = $channel->channel_id; } // Pre-fill route form if exists @@ -252,16 +260,61 @@ public function createFeed(): void } } + public function updatedPlatformInstanceId(?int $value): void + { + $this->reset(['channelCommunityId', 'availableCommunities', 'communityLoadError']); + + if ($value) { + $this->loadCommunities(); + } + } + + public function refreshCommunities(): void + { + $instance = $this->platformInstanceId ? PlatformInstance::find($this->platformInstanceId) : null; + + if (! $instance) { + return; + } + + app(CommunityDirectory::class)->forget($instance); + $this->loadCommunities(); + } + + private function loadCommunities(): void + { + $this->availableCommunities = []; + $this->communityLoadError = null; + + $instance = $this->platformInstanceId ? PlatformInstance::find($this->platformInstanceId) : null; + + if (! $instance) { + return; + } + + try { + $this->availableCommunities = app(CommunityDirectory::class)->forInstance($instance); + } catch (Exception $e) { + $this->communityLoadError = 'Could not reach this instance to list its communities: '.$e->getMessage(); + } + } + public function createChannel(): void { $this->formErrors = []; $this->isLoading = true; $this->validate([ - 'channelName' => 'required|string|max:255', + 'channelCommunityId' => [ + 'required', + 'integer', + Rule::in(collect($this->availableCommunities)->pluck('id')->all()), + ], 'platformInstanceId' => 'required|exists:platform_instances,id', 'channelLanguageId' => 'required|exists:languages,id', 'channelDescription' => 'nullable|string|max:1000', + ], [ + 'channelCommunityId.in' => 'Select a community from this instance.', ]); // If language changed, reset feed form @@ -274,11 +327,14 @@ public function createChannel(): void } $this->previousChannelLanguageId = $this->channelLanguageId; + $name = collect($this->availableCommunities)->firstWhere('id', $this->channelCommunityId)['name'] ?? null; + try { $channel = $this->createChannelAction->execute( - $this->channelName, - $this->platformInstanceId, - $this->channelLanguageId, + $name, + (int) $this->channelCommunityId, + (int) $this->platformInstanceId, + $this->channelLanguageId !== null ? (int) $this->channelLanguageId : null, $this->channelDescription ?: null, ); diff --git a/app/Models/PlatformChannel.php b/app/Models/PlatformChannel.php index 055b5f60..12f88fac 100644 --- a/app/Models/PlatformChannel.php +++ b/app/Models/PlatformChannel.php @@ -15,7 +15,7 @@ * @property int $id * @property int $platform_instance_id * @property PlatformInstance $platformInstance - * @property string $channel_id + * @property int $channel_id * @property string $name * @property int $language_id * @property Language|null $language @@ -40,6 +40,7 @@ class PlatformChannel extends Model protected $casts = [ 'is_active' => 'boolean', + 'channel_id' => 'integer', ]; /** diff --git a/app/Modules/Lemmy/Services/LemmyApiService.php b/app/Modules/Lemmy/Services/LemmyApiService.php index c3d23c5c..0a4dba47 100644 --- a/app/Modules/Lemmy/Services/LemmyApiService.php +++ b/app/Modules/Lemmy/Services/LemmyApiService.php @@ -84,18 +84,35 @@ public function login(string $username, string $password): ?string } /** - * Resolve a PlatformChannel.channel_id to a numeric Lemmy community id. - * - * channel_id holds either a community slug (the usual case — CreateChannelAction - * copies `name` into it) or an already-numeric community id. Callers that need the - * numeric id should use this rather than reimplementing the check, so the two forms - * stay handled identically everywhere. + * @return array */ - public function resolveCommunityId(string $channelId, string $token): int + public function listCommunities(?string $token = null): array { - return is_numeric($channelId) - ? (int) $channelId - : $this->getCommunityId($channelId, $token); + $request = new LemmyRequest($this->instance, $token); + $response = $request->get('community/list', [ + 'type_' => 'Local', + 'limit' => 50, + 'sort' => 'TopAll', + ]); + + if (! $response->successful()) { + throw new Exception('Failed to list communities: '.$response->status()); + } + + /** @var array> $communities */ + $communities = $response->json('communities') ?? []; + + return collect($communities) + ->pluck('community') + ->reject(fn ($community) => ($community['removed'] ?? false) || ($community['deleted'] ?? false)) + ->map(fn ($community) => [ + 'id' => (int) $community['id'], + 'name' => (string) $community['name'], + 'title' => (string) ($community['title'] ?? $community['name']), + ]) + ->sortBy('name') + ->values() + ->all(); } public function getCommunityId(string $communityName, string $token): int diff --git a/app/Modules/Lemmy/Services/LemmyPublisher.php b/app/Modules/Lemmy/Services/LemmyPublisher.php index be7855a0..11d7e300 100644 --- a/app/Modules/Lemmy/Services/LemmyPublisher.php +++ b/app/Modules/Lemmy/Services/LemmyPublisher.php @@ -54,13 +54,11 @@ private function createPost(string $token, array $extractedData, PlatformChannel { $languageId = $extractedData['language_id'] ?? null; - $communityId = $this->api->resolveCommunityId($channel->channel_id, $token); - return $this->api->createPost( $token, $extractedData['title'] ?? 'Untitled', $extractedData['description'] ?? '', - $communityId, + $channel->channel_id, $article->url, $extractedData['thumbnail'] ?? null, $languageId diff --git a/app/Services/Platform/CommunityDirectory.php b/app/Services/Platform/CommunityDirectory.php new file mode 100644 index 00000000..75359159 --- /dev/null +++ b/app/Services/Platform/CommunityDirectory.php @@ -0,0 +1,51 @@ + + */ + public function forInstance(PlatformInstance $instance): array + { + return Cache::remember( + self::cacheKey($instance), + self::TTL_SECONDS, + fn () => $this->makeApi($instance->url)->listCommunities() + ); + } + + public function forget(PlatformInstance $instance): void + { + Cache::forget(self::cacheKey($instance)); + } + + public function has(PlatformInstance $instance, int $communityId): bool + { + return collect($this->forInstance($instance)) + ->contains(fn (array $community) => $community['id'] === $communityId); + } + + public function name(PlatformInstance $instance, int $communityId): ?string + { + return collect($this->forInstance($instance)) + ->firstWhere('id', $communityId)['name'] ?? null; + } + + protected function makeApi(string $instanceUrl): LemmyApiService + { + return new LemmyApiService($instanceUrl); + } + + private static function cacheKey(PlatformInstance $instance): string + { + return "platform:communities:{$instance->id}"; + } +} diff --git a/database/factories/PlatformChannelFactory.php b/database/factories/PlatformChannelFactory.php index e087cd7a..c5e3bac0 100644 --- a/database/factories/PlatformChannelFactory.php +++ b/database/factories/PlatformChannelFactory.php @@ -18,7 +18,7 @@ public function definition(): array { return [ 'platform_instance_id' => PlatformInstance::factory(), - 'channel_id' => $this->faker->slug(2), + 'channel_id' => $this->faker->unique()->numberBetween(1, 999999), 'name' => $this->faker->words(2, true), 'display_name' => $this->faker->words(2, true), 'language_id' => Language::factory(), @@ -39,7 +39,6 @@ public function community(?string $name = null): static $communityName = $name ?: $this->faker->word(); return $this->state(fn (array $attributes) => [ - 'channel_id' => strtolower($communityName), 'name' => $communityName, 'display_name' => ucfirst($communityName), ]); diff --git a/database/migrations/2024_01_01_000015_store_numeric_community_id_on_platform_channels.php b/database/migrations/2024_01_01_000015_store_numeric_community_id_on_platform_channels.php new file mode 100644 index 00000000..4f907e4f --- /dev/null +++ b/database/migrations/2024_01_01_000015_store_numeric_community_id_on_platform_channels.php @@ -0,0 +1,99 @@ +orderBy('id') + ->get() + ->mapWithKeys(fn (object $channel) => [$channel->id => $this->resolve($channel)]); + + Schema::table('platform_channels', function (Blueprint $table) { + $table->dropUnique('platform_channels_channel_id_unique'); + }); + + Schema::table('platform_channels', function (Blueprint $table) { + $table->unsignedBigInteger('remote_community_id')->nullable()->after('channel_id'); + }); + + foreach ($resolved as $id => $communityId) { + DB::table('platform_channels') + ->where('id', $id) + ->update(['remote_community_id' => $communityId]); + } + + Schema::table('platform_channels', function (Blueprint $table) { + $table->dropColumn('channel_id'); + }); + + Schema::table('platform_channels', function (Blueprint $table) { + $table->renameColumn('remote_community_id', 'channel_id'); + }); + + Schema::table('platform_channels', function (Blueprint $table) { + $table->unsignedBigInteger('channel_id')->nullable(false)->change(); + $table->unique(['platform_instance_id', 'channel_id'], 'platform_channels_channel_id_unique'); + }); + } + + public function down(): void + { + Schema::table('platform_channels', function (Blueprint $table) { + $table->dropUnique('platform_channels_channel_id_unique'); + }); + + Schema::table('platform_channels', function (Blueprint $table) { + $table->string('channel_id')->change(); + }); + + DB::table('platform_channels')->update(['channel_id' => DB::raw('name')]); + + Schema::table('platform_channels', function (Blueprint $table) { + $table->unique(['platform_instance_id', 'channel_id'], 'platform_channels_channel_id_unique'); + }); + } + + private function resolve(object $channel): int + { + if (is_numeric($channel->channel_id)) { + return (int) $channel->channel_id; + } + + $instance = DB::table('platform_instances')->find($channel->platform_instance_id); + + if (! $instance) { + throw new RuntimeException("Channel {$channel->id} has no platform instance; cannot resolve its community id."); + } + + $account = PlatformAccount::where('instance_url', $instance->url) + ->where('is_active', true) + ->first(); + + if (! $account) { + throw new RuntimeException("No active account for {$instance->url}; cannot resolve community '{$channel->channel_id}'."); + } + + $api = new LemmyApiService($instance->url); + $token = $api->login($account->username, $account->password); + + if (! $token) { + throw new RuntimeException("Could not authenticate against {$instance->url} to resolve community '{$channel->channel_id}'."); + } + + return $api->getCommunityId($channel->channel_id, $token); + } +}; diff --git a/resources/views/livewire/channels.blade.php b/resources/views/livewire/channels.blade.php index 26140bae..88af793b 100644 --- a/resources/views/livewire/channels.blade.php +++ b/resources/views/livewire/channels.blade.php @@ -162,22 +162,11 @@ class="w-full inline-flex justify-center rounded-md border border-gray-300 shado @if ($showCreateModal)
-
- - - @error('newName')

{{ $message }}

@enderror -
-
@error('newPlatformInstanceId')

{{ $message }}

@enderror + @if ($communityLoadError) +

{{ $communityLoadError }}

+ @endif
+ @if ($availableCommunities) +
+
+ + +
+ + @error('newCommunityId')

{{ $message }}

@enderror +
+ @endif +
-

Enter the community name (without the @ or instance)

- @error('channelName')

{{ $message }}

@enderror -
-
@error('platformInstanceId')

{{ $message }}

@enderror + @if ($communityLoadError) +

{{ $communityLoadError }}

+ @endif
+ @if ($availableCommunities) +
+
+ + +
+ + @error('channelCommunityId')

{{ $message }}

@enderror +
+ @endif +