diff --git a/KNOWLEDGE_BASE.md b/KNOWLEDGE_BASE.md index 4f575f267..376278dcd 100644 --- a/KNOWLEDGE_BASE.md +++ b/KNOWLEDGE_BASE.md @@ -539,7 +539,7 @@ An app can use the same `/api/v1` for you with signed requests. It keeps an SSH A signed request sees what the web shows you, such as your own pending commits. It can also, with the web's validation, moderation, and rate limits: -- read your account, logs, replies, notifications (including grouped activity), and starred commits; +- read your account, logs, replies, threads authors write as replies to themselves, notifications (including grouped activity), and starred commits; - commit, reply, and quote, with images, and delete your commits, with the same undo window as the web; - preview commit drafts without publishing them; - follow or unfollow people and organizations, mute or unmute, and block or unblock, and list who you muted and blocked; diff --git a/app/Http/Controllers/Api/CommitContinuationController.php b/app/Http/Controllers/Api/CommitContinuationController.php new file mode 100644 index 000000000..99a26bba0 --- /dev/null +++ b/app/Http/Controllers/Api/CommitContinuationController.php @@ -0,0 +1,48 @@ +get($request->continuedCommit() ?? $commit, $user); + $commits = $thread->commits->values(); + $nextIds = $commits->skip(1)->pluck('id')->push($thread->nextId)->values(); + + foreach ($commits as $index => $listed) { + $listed->setAttribute('continuation_id', $nextIds[$index]); + $listed->setAttribute('thread_id', $commit->id); + } + + // The paginator only links to a next page when it is given one item more than a page. + $page = new CursorPaginator( + $thread->nextId === null ? $commits : $commits->concat([new Commit]), + CommitContinuationQuery::PER_PAGE, + options: ['path' => $request->url(), 'parameters' => ['thread_id', 'id']], + ); + + return CommitResource::collection($page)->additional(['meta' => [ + 'position' => $thread->position, + 'length' => $thread->length, + 'is_capped' => $thread->isCapped, + ]]); + } +} diff --git a/app/Http/Controllers/Api/CommitReplyController.php b/app/Http/Controllers/Api/CommitReplyController.php index 7506a4a07..327595a67 100644 --- a/app/Http/Controllers/Api/CommitReplyController.php +++ b/app/Http/Controllers/Api/CommitReplyController.php @@ -16,6 +16,6 @@ { public function index(ListApiCommitRepliesRequest $request, #[CurrentUser] User $user, Commit $commit, CommitRepliesQuery $replies): AnonymousResourceCollection { - return CommitResource::collection($replies->paginate($commit, $user, $request->boolean('newest'))->withQueryString()); + return CommitResource::collection($replies->paginate($commit, $user, $request->boolean('newest'), except: $request->exceptedReplyId())->withQueryString()); } } diff --git a/app/Http/Requests/ListApiCommitContinuationRequest.php b/app/Http/Requests/ListApiCommitContinuationRequest.php new file mode 100644 index 000000000..0ae54e0dd --- /dev/null +++ b/app/Http/Requests/ListApiCommitContinuationRequest.php @@ -0,0 +1,67 @@ +user(); + assert($user instanceof User); + + $commit = $this->route('commit'); + assert($commit instanceof Commit); + + return $commit->isVisibleTo($user) && $this->continuedCommit()?->isVisibleTo($user) + ? Response::allow() + : Response::denyAsNotFound(); + } + + /** + * The commit whose next commits this page lists: the commit itself, or the one the cursor continues from. + */ + public function continuedCommit(): ?Commit + { + if ($this->continued instanceof Commit) { + return $this->continued; + } + + $commit = $this->route('commit'); + assert($commit instanceof Commit); + + $cursor = $this->query('cursor'); + $parameters = is_string($cursor) ? Cursor::fromEncoded($cursor)?->toArray() ?? [] : []; + $id = $parameters['id'] ?? null; + + if ($id === null) { + return $this->continued = $commit; + } + + return $this->continued = ($parameters['thread_id'] ?? null) === $commit->id && is_string($id) && Str::isUuid($id) + ? Commit::query()->find($id) + : null; + } + + /** + * @return array{} + */ + public function rules(): array + { + return []; + } +} diff --git a/app/Http/Requests/ListApiCommitRepliesRequest.php b/app/Http/Requests/ListApiCommitRepliesRequest.php index 359a8f9b0..707892f88 100644 --- a/app/Http/Requests/ListApiCommitRepliesRequest.php +++ b/app/Http/Requests/ListApiCommitRepliesRequest.php @@ -25,6 +25,14 @@ public function authorize(): Response return $commit->isVisibleTo($user) ? Response::allow() : Response::denyAsNotFound(); } + /** + * The reply the client shows elsewhere, such as the continuation of the commit, as on the web. + */ + public function exceptedReplyId(): ?string + { + return $this->string('except')->value() ?: null; + } + /** * Newest first continues the next_cursor of a reply preview, as on the web. * @@ -34,6 +42,7 @@ public function rules(): array { return [ 'newest' => ['sometimes', 'boolean'], + 'except' => ['sometimes', 'nullable', 'uuid'], ]; } } diff --git a/app/Http/Resources/CommitResource.php b/app/Http/Resources/CommitResource.php index f8340df0c..ec06cc5e8 100644 --- a/app/Http/Resources/CommitResource.php +++ b/app/Http/Resources/CommitResource.php @@ -24,10 +24,13 @@ * and how many pastéis it was given, which the app only shows in pastéis mode, as the web does. Like the web's cards, * it also has preview_segments, the excerpt a list shows of a long commit, the poll, with what the user may see of * its votes, and the commit it quotes, or null once the user may no longer see that one, while quoted_commit_id still - * says it quoted something. In a list, reply_preview is the newest reply or quote the web shows under it. As on the - * web, the counts, link_preview, images, starred, parent, poll, quoted_commit and reply_preview are only - * there when they were loaded, so ancestors and the commits of notifications come without some of them. The images of - * a commit that isn't published yet have temporary URLs, since the app has no web session to load them with. + * says it quoted something. In a list, reply_preview is the newest reply or quote the web shows under it. + * continuation_id is the author's earliest reply to their own commit, which the web shows as the next commit of their + * thread rather than among the replies, so replies_count leaves it out; it is null when there is none the user may see. + * As on the web, the counts, link_preview, images, starred, continuation_id, parent, poll, quoted_commit and + * reply_preview are only there when they were loaded, so ancestors and the commits of notifications come without some + * of them. The images of a commit that isn't published yet have temporary URLs, since the app has no web session to + * load them with. * * A deleted commit that still has replies stays in its thread as a tombstone: its id, parent_id, deleted_at and * replies_count, but never its author or body. Only signed requests are ever sent one. @@ -81,6 +84,7 @@ public function toArray(Request $request): array 'starred' => $this->whenHas('starred'), 'pasteis_count' => $this->pasteis_count, 'deleted_at' => null, + 'continuation_id' => $this->whenHas('continuation_id'), 'parent' => $this->whenLoaded('parent', fn (Commit $parent): self => self::make($parent)), 'quoted_commit_id' => $this->quoted_commit_id, 'quoted_commit' => $this->whenLoaded('quotedCommit', fn (Commit $quoted): self => self::make($quoted)), diff --git a/routes/api.php b/routes/api.php index 824068436..fc4ec4047 100644 --- a/routes/api.php +++ b/routes/api.php @@ -4,6 +4,7 @@ use App\Exceptions\ApiException; use App\Http\Controllers\Api\CallController; +use App\Http\Controllers\Api\CommitContinuationController; use App\Http\Controllers\Api\CommitController as ApiCommitController; use App\Http\Controllers\Api\CommitPastelController; use App\Http\Controllers\Api\CommitPinController; @@ -260,6 +261,8 @@ Route::get('commits/{commit}/replies', [CommitReplyController::class, 'index']) ->withTrashed() ->name('api-commit-reply.index'); + Route::get('commits/{commit}/continuation', [CommitContinuationController::class, 'index']) + ->name('api-commit-continuation.index'); // Commit Report: a commit that doesn't exist or was deleted gets the same answer as one the user may not see... Route::post('commits/{commit}/reports', [CommitReportController::class, 'store']) diff --git a/tests/Feature/Controllers/Api/CommitContinuationControllerTest.php b/tests/Feature/Controllers/Api/CommitContinuationControllerTest.php new file mode 100644 index 000000000..7d1ce77c9 --- /dev/null +++ b/tests/Feature/Controllers/Api/CommitContinuationControllerTest.php @@ -0,0 +1,174 @@ +user = User::factory()->create(); + $this->keyPair = apiKeyPairFor($this->user); +}); + +it('lists the next commits of the author thread as the API shows commits', function (): void { + $author = User::factory()->create(); + $commit = Commit::factory()->for($author)->create(['created_at' => now()->subMinutes(5)]); + $first = Commit::factory()->for($author)->for($commit, 'parent')->create(['created_at' => now()->subMinutes(4)]); + $second = Commit::factory()->for($author)->for($first, 'parent')->create(['created_at' => now()->subMinutes(3)]); + Commit::factory()->for($author)->for($commit, 'parent')->create(['created_at' => now()->subMinutes(2)]); + Commit::factory()->for($first, 'parent')->create(['created_at' => now()->subMinute()]); + + signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', $commit)) + ->assertOk() + ->assertJsonPath('data.*.id', [$first->id, $second->id]) + ->assertJsonPath('data.0.author.handle', $author->handle) + ->assertJsonPath('data.0.continuation_id', $second->id) + ->assertJsonPath('data.0.replies_count', 1) + ->assertJsonPath('data.1.continuation_id', null) + ->assertJsonPath('meta.position', 1) + ->assertJsonPath('meta.length', 2) + ->assertJsonPath('meta.is_capped', false) + ->assertJsonPath('meta.next_cursor', null) + ->assertJsonPath('links.next', null); +}); + +it('lists the thread 10 commits at a time with a cursor to the next page', function (): void { + $author = User::factory()->create(); + $commits = [Commit::factory()->for($author)->create()]; + + foreach (range(1, CommitContinuationQuery::PER_PAGE + 2) as $ignored) { + $commits[] = Commit::factory()->for($author)->for(end($commits), 'parent')->create(); + } + + $first = signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', $commits[0])); + + $first->assertOk() + ->assertJsonPath('data.*.id', array_map(fn (Commit $commit): string => $commit->id, array_slice($commits, 1, 10))) + ->assertJsonPath('data.9.continuation_id', $commits[11]->id) + ->assertJsonPath('meta.position', 1) + ->assertJsonPath('meta.length', 12); + + $second = signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', [$commits[0], 'cursor' => $first->json('meta.next_cursor')])); + + $second->assertOk() + ->assertJsonPath('data.*.id', [$commits[11]->id, $commits[12]->id]) + ->assertJsonPath('meta.position', 11) + ->assertJsonPath('meta.length', 2) + ->assertJsonPath('meta.next_cursor', null); + + signedApiRequest($this->keyPair, 'GET', (string) $first->json('links.next')) + ->assertOk() + ->assertJsonPath('data.*.id', [$commits[11]->id, $commits[12]->id]); +}); + +it('does not query each commit of the thread, its author or its stars', function (): void { + Queue::fake([DetectCommitLanguage::class]); + $author = User::factory()->create(); + $commits = [Commit::factory()->for($author)->create()]; + + $queries = function () use ($commits): int { + DB::flushQueryLog(); + DB::enableQueryLog(); + + signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', $commits[0]))->assertOk(); + + return count(DB::getQueryLog()); + }; + + $create = function (int $count) use (&$commits, $author): void { + foreach (range(1, $count) as $ignored) { + $commits[] = $commit = Commit::factory()->for($author)->for(end($commits), 'parent')->create(); + $commit->stars()->attach([$this->user->id, User::factory()->create()->id]); + } + }; + + $create(2); + + // The first signed request also syncs the user's keys with GitHub, and the first cards load the features... + $queries(); + + $few = $queries(); + + $create(8); + + expect($queries())->toBe($few); +}); + +it('lists the first page for a cursor that is not one', function (mixed $cursor): void { + $commit = Commit::factory()->create(); + $next = Commit::factory()->for($commit->user)->for($commit, 'parent')->create(); + + signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', [$commit, 'cursor' => $cursor])) + ->assertOk() + ->assertJsonPath('data.*.id', [$next->id]); +})->with([ + 'malformed' => ['not-a-cursor'], + 'array' => [['not-a-cursor']], +]); + +it('returns not found for a cursor on something that is not a commit', function (): void { + $commit = Commit::factory()->create(); + + signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', [$commit, 'cursor' => new Cursor(['thread_id' => $commit->id, 'id' => 'not-a-uuid'])->encode()])) + ->assertNotFound(); +}); + +it('returns not found for a cursor from the thread of another commit', function (): void { + $commit = Commit::factory()->create(); + Commit::factory()->for($commit->user)->for($commit, 'parent')->create(); + $other = [Commit::factory()->create()]; + + foreach (range(1, CommitContinuationQuery::PER_PAGE + 1) as $ignored) { + $other[] = Commit::factory()->for($other[0]->user)->for(end($other), 'parent')->create(); + } + + $cursor = signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', $other[0]))->json('meta.next_cursor'); + + signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', [$other[0], 'cursor' => $cursor]))->assertOk(); + signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', [$commit, 'cursor' => $cursor]))->assertNotFound(); +}); + +it('stops the thread at the first commit the user may not see', function (): void { + $author = User::factory()->create(); + $commit = Commit::factory()->for($author)->create(); + $visible = Commit::factory()->for($author)->for($commit, 'parent')->create(); + $pending = Commit::factory()->for($author)->for($visible, 'parent')->pending()->create(); + Commit::factory()->for($author)->for($pending, 'parent')->create(); + + signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', $commit)) + ->assertOk() + ->assertJsonPath('data.*.id', [$visible->id]) + ->assertJsonPath('data.0.continuation_id', null) + ->assertJsonPath('meta.length', 1); +}); + +it('returns not found for the thread of a commit the user may not see', function (): void { + $commit = Commit::factory()->pending()->create(); + + signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', $commit))->assertNotFound(); +}); + +it('returns not found for the thread of a commit across a block', function (): void { + $commit = Commit::factory()->create(); + Commit::factory()->for($commit->user)->for($commit, 'parent')->create(); + $this->user->blocking()->attach($commit->user); + + signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', $commit))->assertNotFound(); +}); + +it('returns not found for a cursor on a commit the user may not see', function (): void { + $commit = Commit::factory()->create(); + $hidden = Commit::factory()->pending()->create(); + + signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', [$commit, 'cursor' => new Cursor(['thread_id' => $commit->id, 'id' => $hidden->id])->encode()])) + ->assertNotFound(); +}); + +it('rejects requests that are not signed', function (): void { + $this->getJson(route('api-commit-continuation.index', Commit::factory()->create()))->assertUnauthorized(); +}); diff --git a/tests/Feature/Controllers/Api/CommitControllerTest.php b/tests/Feature/Controllers/Api/CommitControllerTest.php index f811fcac9..df3b92198 100644 --- a/tests/Feature/Controllers/Api/CommitControllerTest.php +++ b/tests/Feature/Controllers/Api/CommitControllerTest.php @@ -291,6 +291,7 @@ 'starred' => true, 'pasteis_count' => 4, 'deleted_at' => null, + 'continuation_id' => null, 'quoted_commit_id' => null, 'quoted_commit' => null, 'poll' => null, @@ -322,6 +323,24 @@ ]); }); +it('tells a signed request which reply continues the thread of its author, and leaves it out of the replies count', function (): void { + $keyPair = apiKeyPairFor(User::factory()->create()); + $author = User::factory()->create(); + $commit = Commit::factory()->for($author)->create(['created_at' => now()->subMinutes(4)]); + $continuation = Commit::factory()->for($author)->for($commit, 'parent')->create(['created_at' => now()->subMinutes(3)]); + Commit::factory()->for($author)->for($commit, 'parent')->create(['created_at' => now()->subMinutes(2)]); + Commit::factory()->for($commit, 'parent')->create(['created_at' => now()->subMinute()]); + + signedApiRequest($keyPair, 'GET', route('api-commit.show', $commit)) + ->assertOk() + ->assertJsonPath('data.continuation_id', $continuation->id) + ->assertJsonPath('data.replies_count', 2); + + $this->getJson(route('api-commit.show', $commit)) + ->assertOk() + ->assertJsonMissingPath('data.continuation_id'); +}); + it('shows a signed request the poll of a commit as the web does, with each share once the votes show', function (): void { $user = User::factory()->create(); $keyPair = apiKeyPairFor($user); diff --git a/tests/Feature/Controllers/Api/CommitReplyControllerTest.php b/tests/Feature/Controllers/Api/CommitReplyControllerTest.php index adfc109fc..6c5090532 100644 --- a/tests/Feature/Controllers/Api/CommitReplyControllerTest.php +++ b/tests/Feature/Controllers/Api/CommitReplyControllerTest.php @@ -36,6 +36,35 @@ ->assertJsonPath('meta.next_cursor', null); }); +it('tells which reply continues the thread of each reply', function (): void { + $parent = Commit::factory()->create(); + $author = User::factory()->create(); + $threaded = Commit::factory()->for($author)->for($parent, 'parent')->create(['created_at' => now()->subMinutes(3)]); + $continuation = Commit::factory()->for($author)->for($threaded, 'parent')->create(['created_at' => now()->subMinutes(2)]); + Commit::factory()->for($parent, 'parent')->create(['created_at' => now()->subMinute()]); + + signedApiRequest($this->keyPair, 'GET', route('api-commit-reply.index', $parent)) + ->assertOk() + ->assertJsonPath('data.0.continuation_id', $continuation->id) + ->assertJsonPath('data.1.continuation_id', null); +}); + +it('leaves out the reply the client shows elsewhere, such as the continuation, as the web does', function (): void { + $parent = Commit::factory()->create(['created_at' => now()->subMinutes(3)]); + $continuation = Commit::factory()->for($parent->user)->for($parent, 'parent')->create(['created_at' => now()->subMinutes(2)]); + $reply = Commit::factory()->for($parent, 'parent')->create(['created_at' => now()->subMinute()]); + + signedApiRequest($this->keyPair, 'GET', route('api-commit-reply.index', [$parent, 'except' => $continuation->id])) + ->assertOk() + ->assertJsonPath('data.*.id', [$reply->id]); +}); + +it('validates the reply to leave out', function (): void { + signedApiRequest($this->keyPair, 'GET', route('api-commit-reply.index', [Commit::factory()->create(), 'except' => 'not-a-uuid'])) + ->assertUnprocessable() + ->assertJsonValidationErrors('except'); +}); + it('shows the stars and saves of each reply as the user sees them', function (): void { $parent = Commit::factory()->create(); $starred = Commit::factory()->for($parent, 'parent')->create(['created_at' => now()->subMinutes(2)]); diff --git a/tests/Feature/Controllers/Api/LogControllerTest.php b/tests/Feature/Controllers/Api/LogControllerTest.php index 984afa307..76940da8c 100644 --- a/tests/Feature/Controllers/Api/LogControllerTest.php +++ b/tests/Feature/Controllers/Api/LogControllerTest.php @@ -82,6 +82,7 @@ 'starred' => false, 'pasteis_count' => 2, 'deleted_at' => null, + 'continuation_id' => null, 'quoted_commit_id' => null, 'quoted_commit' => null, 'poll' => null, diff --git a/tests/Feature/Controllers/Api/UserCommitControllerTest.php b/tests/Feature/Controllers/Api/UserCommitControllerTest.php index bf8c346ed..210fc0c1e 100644 --- a/tests/Feature/Controllers/Api/UserCommitControllerTest.php +++ b/tests/Feature/Controllers/Api/UserCommitControllerTest.php @@ -271,3 +271,16 @@ ->assertOk() ->assertJsonMissingPath('data.0.reply_preview'); }); + +it('tells a signed request which reply continues the thread of each commit it lists', function (): void { + $keyPair = apiKeyPairFor(User::factory()->create()); + $user = User::factory()->create(); + $commit = Commit::factory()->for($user)->create(['created_at' => now()->subMinutes(3)]); + $continuation = Commit::factory()->for($user)->for($commit, 'parent')->create(['created_at' => now()->subMinutes(2)]); + Commit::factory()->for($user)->create(['created_at' => now()->subMinutes(4)]); + + signedApiRequest($keyPair, 'GET', route('api-user-commit.index', $user)) + ->assertOk() + ->assertJsonPath('data.0.continuation_id', $continuation->id) + ->assertJsonPath('data.1.continuation_id', null); +});