diff --git a/app/Actions/CreateCommit.php b/app/Actions/CreateCommit.php index 74965228a..72e6d4958 100644 --- a/app/Actions/CreateCommit.php +++ b/app/Actions/CreateCommit.php @@ -54,13 +54,16 @@ public function handle(User $user, string $body, ?Commit $parent = null, ?Commit $linkPreview = $this->linkPreviewFor($url); $createdAt = now(); $hasGoodReputation = $user->hasGoodReputation(); + $status = $user->isAdmin() || ($hasGoodReputation && ! $user->hasRecentReports()) ? CommitStatus::Approved : CommitStatus::Pending; + $isModerated = $status === CommitStatus::Pending && ! $hasGoodReputation && filled(config('services.openai.key')); $commit = $user->commits()->create([ 'body' => $this->emojiShortcodes->replace($body), 'mentions' => $this->mentionedUsers->get($body), 'parent_id' => $parent?->id, 'root_id' => $parent?->threadRootId(), - 'status' => $user->isAdmin() || ($hasGoodReputation && ! $user->hasRecentReports()) ? CommitStatus::Approved : CommitStatus::Pending, + 'status' => $status, + 'ai_decision' => $isModerated ? CommitStatus::Pending : null, 'link_preview_id' => $linkPreview?->id, 'quoted_commit_id' => $quotedCommit?->id, 'hot_score' => $this->theAlgorithm->score($createdAt, 0, 0), @@ -86,7 +89,7 @@ public function handle(User $user, string $body, ?Commit $parent = null, ?Commit } $this->createWorldReply->handle($commit); - if ($commit->status === CommitStatus::Pending && ! $hasGoodReputation && filled(config('services.openai.key'))) { + if ($isModerated) { dispatch(new ModerateCommit($commit))->afterCommit(); } diff --git a/app/Jobs/ModerateCommit.php b/app/Jobs/ModerateCommit.php index 77a996a66..55a200213 100644 --- a/app/Jobs/ModerateCommit.php +++ b/app/Jobs/ModerateCommit.php @@ -27,12 +27,13 @@ public function __construct(public readonly Commit $commit) } /** - * A commit that was moderated or deleted in the meantime no longer needs the AI's decision. Once every attempt - * has failed, the commit is simply left without one. + * A commit that was moderated or deleted in the meantime no longer needs the AI's decision. */ public function handle(UpdateCommitAiDecision $action, UpdateCommitStatuses $updateCommitStatuses): void { if ($this->commit->status !== CommitStatus::Pending || $this->commit->trashed()) { + $this->leaveUnchecked(); + return; } @@ -41,9 +42,30 @@ public function handle(UpdateCommitAiDecision $action, UpdateCommitStatuses $upd $updateCommitStatuses->handle([$this->commit->id], CommitStatus::Approved, onlyPending: true); } + $this->leaveUnchecked(); + return; } $action->handle($this->commit); } + + /** + * Once every attempt has failed, the commit is left without a decision and simply waits for a moderator. + */ + public function failed(): void + { + $this->leaveUnchecked(); + } + + /** + * Clears only the pending AI decision, so a decision written in the meantime stays. + */ + private function leaveUnchecked(): void + { + Commit::withTrashed() + ->whereKey($this->commit->id) + ->where('ai_decision', CommitStatus::Pending) + ->update(['ai_decision' => null]); + } } diff --git a/app/Models/Commit.php b/app/Models/Commit.php index 0c0dd2bf6..a9cc9a00d 100644 --- a/app/Models/Commit.php +++ b/app/Models/Commit.php @@ -170,14 +170,13 @@ public function isVisibleTo(?User $user): bool } /** - * A pending commit is being checked until its moderation job leaves an AI decision behind. After that, or once every - * attempt has had its chance, it simply waits for a moderator. + * A pending commit is being checked while its moderation job holds a pending AI decision. The time bound covers a + * job that was lost before it could leave a decision or clear the pending one. */ public function isBeingChecked(): bool { return $this->status === CommitStatus::Pending - && $this->ai_decision === null - && filled(config('services.openai.key')) + && $this->ai_decision === CommitStatus::Pending && $this->created_at->greaterThan(now()->subMinutes(self::CHECKING_MINUTES)); } diff --git a/resources/js/types/commit.ts b/resources/js/types/commit.ts index 7ea9c940c..b9c44a3b4 100644 --- a/resources/js/types/commit.ts +++ b/resources/js/types/commit.ts @@ -82,7 +82,7 @@ export type Commit = { quoted_commit_id: string | null; quoted_commit?: QuotedCommit | DeletedCommit | null; images?: CommitImage[]; - ai_decision?: Exclude | null; + ai_decision?: CommitStatus | null; ai_categories?: string[] | null; }; diff --git a/tests/Feature/Controllers/CommitStatusControllerTest.php b/tests/Feature/Controllers/CommitStatusControllerTest.php index 64e0dd0df..7b1998ca2 100644 --- a/tests/Feature/Controllers/CommitStatusControllerTest.php +++ b/tests/Feature/Controllers/CommitStatusControllerTest.php @@ -12,7 +12,7 @@ it('returns that the authors own pending commit is being checked', function (): void { $author = User::factory()->create(); - $commit = Commit::factory()->for($author)->pending()->create(); + $commit = Commit::factory()->for($author)->pending()->create(['ai_decision' => CommitStatus::Pending]); $this->actingAs($author) ->getJson(route('commit-status.show', $commit)) @@ -34,6 +34,7 @@ it('returns that a pending commit is no longer being checked once every attempt has had its chance', function (): void { $author = User::factory()->create(); $commit = Commit::factory()->for($author)->pending()->create([ + 'ai_decision' => CommitStatus::Pending, 'created_at' => now()->subMinutes(Commit::CHECKING_MINUTES + 1), ]); @@ -43,9 +44,7 @@ ->assertExactJson(['status' => 'pending', 'is_checking' => false]); }); -it('returns that a pending commit is not being checked without moderation', function (): void { - config(['services.openai.key' => null]); - +it('returns that a pending commit is not being checked when no check was queued', function (): void { $author = User::factory()->create(); $commit = Commit::factory()->for($author)->pending()->create(); diff --git a/tests/Unit/Actions/CreateCommitTest.php b/tests/Unit/Actions/CreateCommitTest.php index b708c731a..abc400b7b 100644 --- a/tests/Unit/Actions/CreateCommitTest.php +++ b/tests/Unit/Actions/CreateCommitTest.php @@ -79,7 +79,7 @@ expect($commit->refresh()) ->status->toBe($approved ? CommitStatus::Approved : CommitStatus::Pending) - ->ai_decision->toBeNull() + ->ai_decision->toBe($approved ? null : CommitStatus::Pending) ->and($commit->poll->opened_at?->toIso8601String())->toBe($approved ? now()->toIso8601String() : null); if ($approved) { @@ -242,9 +242,12 @@ $action = resolve(CreateCommit::class); - $action->handle(User::factory()->create(), 'Initial commit'); + $commit = $action->handle(User::factory()->create(), 'Initial commit'); Queue::assertNotPushed(ModerateCommit::class); + expect($commit->refresh()) + ->ai_decision->toBeNull() + ->isBeingChecked()->toBeFalse(); }); it('attaches a link preview for the first url in the body', function (): void { @@ -487,7 +490,9 @@ $approved = $reputable && $dismissed; expect($commit->status)->toBe($approved ? CommitStatus::Approved : CommitStatus::Pending) - ->and($commit->poll->opened_at !== null)->toBe($approved); + ->and($commit->poll->opened_at !== null)->toBe($approved) + ->and($commit->ai_decision)->toBe($reputable ? null : CommitStatus::Pending) + ->and($commit->isBeingChecked())->toBe(! $reputable); if ($reputable) { Queue::assertNotPushed(ModerateCommit::class); diff --git a/tests/Unit/Jobs/ModerateCommitTest.php b/tests/Unit/Jobs/ModerateCommitTest.php index 7e023ec83..274d17095 100644 --- a/tests/Unit/Jobs/ModerateCommitTest.php +++ b/tests/Unit/Jobs/ModerateCommitTest.php @@ -361,10 +361,10 @@ function reviewResponses(array $responses = []): Closure ->ai_categories->toBe(['harmful-text']); })->with('unclear answers'); -it('skips commits that were moderated in the meantime', function (): void { +it('skips commits that were moderated in the meantime, and stops checking them', function (): void { Http::fake(); - $commit = Commit::factory()->create(); + $commit = Commit::factory()->create(['ai_decision' => CommitStatus::Pending]); dispatch_sync(new ModerateCommit($commit)); @@ -378,7 +378,7 @@ function reviewResponses(array $responses = []): Closure $user = User::factory()->create(['created_at' => now()->subHour()->subSecond()]); Commit::factory()->for($user)->create(); - $commit = Commit::factory()->for($user)->pending()->create(); + $commit = Commit::factory()->for($user)->pending()->create(['ai_decision' => CommitStatus::Pending]); followersOf($user, 10); dispatch_sync(new ModerateCommit($commit)); @@ -393,21 +393,24 @@ function reviewResponses(array $responses = []): Closure it('skips commits that were deleted in the meantime', function (): void { Http::fake(); - $commit = Commit::factory()->pending()->create(); + $commit = Commit::factory()->pending()->create(['ai_decision' => CommitStatus::Pending]); $commit->delete(); dispatch_sync(new ModerateCommit($commit)); Http::assertNothingSent(); + + expect($commit->refresh()->ai_decision)->toBeNull(); }); -it('throws when the ai is unavailable, so the job is retried', function (): void { +it('throws when the ai is unavailable, so the job is retried while the commit is still being checked', function (): void { Http::fake(['api.openai.com/v1/moderations' => Http::response([], 503)]); - $commit = Commit::factory()->pending()->create(); + $commit = Commit::factory()->pending()->create(['ai_decision' => CommitStatus::Pending]); - expect(fn () => dispatch_sync(new ModerateCommit($commit)))->toThrow(RequestException::class) - ->and($commit->refresh()->ai_decision)->toBeNull(); + expect(fn () => app()->call([new ModerateCommit($commit), 'handle']))->toThrow(RequestException::class) + ->and($commit->refresh()->ai_decision)->toBe(CommitStatus::Pending) + ->and($commit->isBeingChecked())->toBeTrue(); }); it('holds clean commits when their author has a recent undismissed report', function (int $reportAge, bool $dismissed, bool $deleted, bool $sameAuthor, bool $held): void { @@ -448,7 +451,7 @@ function reviewResponses(array $responses = []): Closure $user = User::factory()->create(['created_at' => now()->subHours(2)]); $reported = Commit::factory()->for($user)->create(); followersOf($user, 10); - $commit = Commit::factory()->for($user)->pending()->create(); + $commit = Commit::factory()->for($user)->pending()->create(['ai_decision' => CommitStatus::Pending]); CommitReport::factory()->for($reported)->create(); dispatch_sync(new ModerateCommit($commit)); @@ -477,3 +480,16 @@ function reviewResponses(array $responses = []): Closure ->ai_decision->toBe(CommitStatus::Approved) ->status->toBe(CommitStatus::Pending); }); + +it('stops checking a commit once every attempt has failed, keeping a decision written in the meantime', function (?CommitStatus $decision, ?CommitStatus $kept): void { + $commit = Commit::factory()->pending()->create(['ai_decision' => $decision]); + + new ModerateCommit($commit)->failed(); + + expect($commit->refresh()) + ->ai_decision->toBe($kept) + ->isBeingChecked()->toBeFalse(); +})->with([ + 'still pending' => [CommitStatus::Pending, null], + 'decided meanwhile' => [CommitStatus::Rejected, CommitStatus::Rejected], +]);