Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: remove listing of posts in the show discussion endpoint #4067

Merged
merged 3 commits into from
Oct 16, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions extensions/flags/extend.php
Original file line number Diff line number Diff line change
@@ -52,11 +52,6 @@
(new Extend\ApiResource(Resource\ForumResource::class))
->fields(ForumResourceFields::class),

(new Extend\ApiResource(Resource\DiscussionResource::class))
->endpoint(Endpoint\Show::class, function (Endpoint\Show $endpoint) {
return $endpoint->addDefaultInclude(['posts.flags', 'posts.flags.user']);
}),

(new Extend\ApiResource(Resource\PostResource::class))
->endpoint([Endpoint\Index::class, Endpoint\Show::class], function (Endpoint\Index|Endpoint\Show $endpoint) {
return $endpoint->addDefaultInclude(['flags', 'flags.user']);
5 changes: 0 additions & 5 deletions extensions/likes/extend.php
Original file line number Diff line number Diff line change
@@ -50,11 +50,6 @@ function (Endpoint\Index|Endpoint\Show|Endpoint\Create|Endpoint\Update $endpoint
}
),

(new Extend\ApiResource(Resource\DiscussionResource::class))
->endpoint(Endpoint\Show::class, function (Endpoint\Show $endpoint): Endpoint\Endpoint {
return $endpoint->addDefaultInclude(['posts.likes']);
}),

(new Extend\Event())
->listen(PostWasLiked::class, Listener\SendNotificationWhenPostIsLiked::class)
->listen(PostWasUnliked::class, Listener\SendNotificationWhenPostIsUnliked::class)
8 changes: 4 additions & 4 deletions extensions/likes/tests/integration/api/ListPostsTest.php
Original file line number Diff line number Diff line change
@@ -173,9 +173,10 @@ public function likes_relation_returns_limited_results_and_shows_only_visible_po
{
// Show discussion endpoint
$response = $this->send(
$this->request('GET', '/api/discussions/100', [
$this->request('GET', '/api/posts', [
'authenticatedAs' => 2,
])->withQueryParams([
'filter' => ['discussion' => 100],
'include' => $include,
])
);
@@ -184,7 +185,7 @@ public function likes_relation_returns_limited_results_and_shows_only_visible_po

$this->assertEquals(200, $response->getStatusCode(), $body);

$included = json_decode($body, true)['included'] ?? [];
$included = json_decode($body, true)['data'] ?? [];

$likes = collect($included)
->where('type', 'posts')
@@ -206,8 +207,7 @@ public function likes_relation_returns_limited_results_and_shows_only_visible_po
public static function likesIncludeProvider(): array
{
return [
['posts,posts.likes'],
['posts.likes'],
['likes'],
[null],
];
}
12 changes: 0 additions & 12 deletions extensions/mentions/extend.php
Original file line number Diff line number Diff line change
@@ -81,15 +81,6 @@
'lastPost.mentionsPosts.user', 'lastPost.mentionsPosts.discussion', 'lastPost.mentionsGroups',
],
]);
})
->endpoint(Endpoint\Show::class, function (Endpoint\Show $endpoint): Endpoint\Show {
return $endpoint->addDefaultInclude(['posts.mentionedBy', 'posts.mentionedBy.user', 'posts.mentionedBy.discussion'])
->eagerLoadWhenIncluded([
'posts' => [
'posts.mentionsUsers', 'posts.mentionsPosts', 'posts.mentionsPosts.user',
'posts.mentionsPosts.discussion', 'posts.mentionsGroups'
],
]);
}),

(new Extend\ApiResource(Resource\UserResource::class))
@@ -128,9 +119,6 @@
]),

(new Extend\ApiResource(Resource\DiscussionResource::class))
->endpoint(Endpoint\Show::class, function (Endpoint\Show $endpoint): Endpoint\Show {
return $endpoint->eagerLoadWhenIncluded(['posts' => ['posts.mentionsTags']]);
})
->endpoint(Endpoint\Index::class, function (Endpoint\Index $endpoint): Endpoint\Index {
return $endpoint->eagerLoadWhenIncluded(['firstPost' => ['firstPost.mentionsTags'], 'lastPost' => ['lastPost.mentionsTags']]);
}),
8 changes: 4 additions & 4 deletions extensions/mentions/tests/integration/api/ListPostsTest.php
Original file line number Diff line number Diff line change
@@ -207,14 +207,15 @@ public function mentioned_by_relation_returns_limited_results_and_shows_only_vis

// Show discussion endpoint
$response = $this->send(
$this->request('GET', '/api/discussions/100', [
$this->request('GET', '/api/posts', [
'authenticatedAs' => 2,
])->withQueryParams([
'filter' => ['discussion' => 100],
'include' => $include,
])
);

$included = json_decode($body = $response->getBody()->getContents(), true)['included'] ?? [];
$included = json_decode($body = $response->getBody()->getContents(), true)['data'] ?? [];

$this->assertEquals(200, $response->getStatusCode(), $body);

@@ -233,8 +234,7 @@ public function mentioned_by_relation_returns_limited_results_and_shows_only_vis
public static function mentionedByIncludeProvider(): array
{
return [
['posts,posts.mentionedBy'],
['posts.mentionedBy'],
['mentionedBy'],
[null],
];
}
6 changes: 0 additions & 6 deletions extensions/tags/extend.php
Original file line number Diff line number Diff line change
@@ -178,10 +178,4 @@ function (Endpoint\Index|Endpoint\Show|Endpoint\Create $endpoint) {
return $endpoint
->addDefaultInclude(['eventPostMentionsTags']);
}),

(new Extend\ApiResource(Resource\DiscussionResource::class))
->endpoint(Endpoint\Show::class, function (Endpoint\Show $endpoint) {
return $endpoint
->addDefaultInclude(['posts.eventPostMentionsTags']);
}),
];
67 changes: 14 additions & 53 deletions framework/core/src/Api/Resource/DiscussionResource.php
Original file line number Diff line number Diff line change
@@ -75,7 +75,6 @@ public function endpoints(): array
->authenticated()
->can('startDiscussion')
->defaultInclude([
'posts',
'user',
'lastPostedUser',
'firstPost',
@@ -89,12 +88,14 @@ public function endpoints(): array
Endpoint\Show::make()
->defaultInclude([
'user',
'posts',
'posts.discussion',
'posts.user',
'posts.user.groups',
'posts.editedUser',
'posts.hiddenUser'
'lastPostedUser',
'firstPost',
'firstPost.discussion',
'firstPost.user',
'firstPost.user.groups',
'firstPost.editedUser',
'firstPost.hiddenUser',
'lastPost'
]),
Endpoint\Index::make()
->defaultInclude([
@@ -206,54 +207,14 @@ public function fields(): array
->type('posts'),
Schema\Relationship\ToMany::make('posts')
->withLinkage(function (Context $context) {
return $context->showing(self::class);
return $context->showing(self::class)
|| $context->creating(self::class)
|| $context->creating(PostResource::class);
})
->includable()
// @todo: remove this, and send a second request from the frontend to /posts instead. Revert Serializer::addIncluded while you're at it.
->get(function (Discussion $discussion, Context $context) {
$showingDiscussion = $context->showing(self::class);

if (! $showingDiscussion) {
return fn () => $discussion->posts->all();
}

/** @var Endpoint\Show $endpoint */
$endpoint = $context->endpoint;

$actor = $context->getActor();

$limit = PostResource::$defaultLimit;

if (($near = Arr::get($context->request->getQueryParams(), 'page.near')) > 1) {
$offset = $this->posts->getIndexForNumber($discussion->id, $near, $actor);
$offset = max(0, $offset - $limit / 2);
} else {
$offset = $endpoint->extractOffsetValue($context, $endpoint->defaultExtracts($context));
}

/** @var Endpoint\Endpoint $endpoint */
$endpoint = $context->endpoint;

$posts = $discussion->posts()
->whereVisibleTo($actor)
->with($endpoint->getEagerLoadsFor('posts', $context))
->with($endpoint->getWhereEagerLoadsFor('posts', $context))
->orderBy('number')
->skip($offset)
->take($limit)
->get();

/** @var Post $post */
foreach ($posts as $post) {
$post->setRelation('discussion', $discussion);
}

$allPosts = $discussion->posts()->whereVisibleTo($actor)->orderBy('number')->pluck('id')->all();
$loadedPosts = $posts->all();

array_splice($allPosts, $offset, $limit, $loadedPosts);

return $allPosts;
// @todo: is it possible to refactor the frontend to not need all post IDs?
// some kind of percentage-based stream scrubber?
return $discussion->posts()->whereVisibleTo($context->getActor())->select('id')->get()->all();
}),
Schema\Relationship\ToOne::make('mostRelevantPost')
->visible(fn (Discussion $model, Context $context) => $context->listing())
1 change: 0 additions & 1 deletion framework/core/src/Api/Resource/PostResource.php
Original file line number Diff line number Diff line change
@@ -101,7 +101,6 @@ public function endpoints(): array
->defaultInclude([
'user',
'discussion',
'discussion.posts',
'discussion.lastPostedUser'
]),
Endpoint\Update::make()
21 changes: 7 additions & 14 deletions framework/core/src/Api/Serializer.php
Original file line number Diff line number Diff line change
@@ -146,24 +146,17 @@ private function whenResolved($value, $callback, bool $prepend = false): void
*/
public function addIncluded(Relationship $field, $model, ?array $include): array
{
if (is_object($model)) {
$relatedResource = $this->resourceForModel($field, $model);

if ($include === null) {
return [
'type' => $relatedResource->type(),
'id' => $relatedResource->getId($model, $this->context),
];
}
$relatedResource = $this->resourceForModel($field, $model);

$data = $this->addToMap($relatedResource, $model, $include);
} else {
$data = [
'type' => $field->collections[0],
'id' => (string) $model,
if ($include === null) {
return [
'type' => $relatedResource->type(),
'id' => $relatedResource->getId($model, $this->context),
];
}

$data = $this->addToMap($relatedResource, $model, $include);

return [
'type' => $data['type'],
'id' => $data['id'],
52 changes: 41 additions & 11 deletions framework/core/src/Forum/Content/Discussion.php
Original file line number Diff line number Diff line change
@@ -33,15 +33,7 @@ public function __invoke(Document $document, Request $request): Document
$near = intval(Arr::get($queryParams, 'near'));
$page = max(1, intval(Arr::get($queryParams, 'page')), 1 + intdiv($near, 20));

$params = [
'page' => [
'near' => $near,
'offset' => ($page - 1) * 20,
'limit' => 20
]
];

$apiDocument = $this->getApiDocument($request, $id, $params);
$apiDocument = $this->getApiDocument($request, $id);

$getResource = function ($link) use ($apiDocument) {
return Arr::first($apiDocument->included, function ($value) use ($link) {
@@ -64,9 +56,21 @@ public function __invoke(Document $document, Request $request): Document
($queryString ? '?'.$queryString : '');
};

$params = [
'filter' => [
'discussion' => intval($id),
],
'page' => [
'near' => $near,
'offset' => ($page - 1) * 20,
'limit' => 20,
],
];

$postsApiDocument = $this->getPostsApiDocument($request, $params);
$posts = [];

foreach ($apiDocument->included as $resource) {
foreach ($postsApiDocument->data as $resource) {
if ($resource->type === 'posts' && isset($resource->relationships->discussion) && isset($resource->attributes->contentHtml)) {
$posts[] = $resource;
}
@@ -77,6 +81,15 @@ public function __invoke(Document $document, Request $request): Document

$document->title = $apiDocument->data->attributes->title;
$document->content = $this->view->make('flarum.forum::frontend.content.discussion', compact('apiDocument', 'page', 'hasPrevPage', 'hasNextPage', 'getResource', 'posts', 'url'));

$apiDocument->included = array_values(array_filter($apiDocument->included, function ($value) {
return $value->type !== 'posts';
}));
$apiDocument->included = array_merge($apiDocument->included, $postsApiDocument->data, $postsApiDocument->included);
$apiDocument->included = array_values(array_filter($apiDocument->included, function ($value) use ($apiDocument) {
return $value->type !== 'discussions' || $value->id !== $apiDocument->data->id;
}));

$document->payload['apiDocument'] = $apiDocument;

$document->canonicalUrl = $url([]);
@@ -91,7 +104,7 @@ public function __invoke(Document $document, Request $request): Document
*
* @throws RouteNotFoundException
*/
protected function getApiDocument(Request $request, string $id, array $params): object
protected function getApiDocument(Request $request, string $id, array $params = []): object
{
$params['bySlug'] = true;

@@ -104,4 +117,21 @@ protected function getApiDocument(Request $request, string $id, array $params):
->getBody()
);
}

/**
* Get the result of an API request to list the posts of a discussion.
*
* @throws RouteNotFoundException
*/
protected function getPostsApiDocument(Request $request, array $params): object
{
return json_decode(
$this->api
->withoutErrorHandling()
->withParentRequest($request)
->withQueryParams($params)
->get('/posts')
->getBody()
);
}
}
13 changes: 9 additions & 4 deletions framework/core/tests/integration/api/discussions/ShowTest.php
Original file line number Diff line number Diff line change
@@ -88,26 +88,31 @@ public function guest_cannot_see_empty_discussion()
public function guest_cannot_see_hidden_posts()
{
$response = $this->send(
$this->request('GET', '/api/discussions/4')
$this->request('GET', '/api/posts')
->withQueryParams([
'filter' => ['discussion' => 4]
])
);

$json = json_decode($response->getBody()->getContents(), true);

$this->assertEmpty(Arr::get($json, 'data.relationships.posts.data'));
$this->assertEmpty(Arr::get($json, 'data'));
}

#[Test]
public function author_can_see_hidden_posts()
{
$response = $this->send(
$this->request('GET', '/api/discussions/4', [
$this->request('GET', '/api/posts', [
'authenticatedAs' => 2,
])->withQueryParams([
'filter' => ['discussion' => 4]
])
);

$json = json_decode($response->getBody()->getContents(), true);

$this->assertEquals(2, Arr::get($json, 'data.relationships.posts.data.0.id'), $response->getBody()->getContents());
$this->assertEquals(2, Arr::get($json, 'data.0.id'), $response->getBody()->getContents());
}

#[Test]
Original file line number Diff line number Diff line change
@@ -59,12 +59,15 @@ public function when_allowed_guests_can_see_hidden_posts()
);

$response = $this->send(
$this->request('GET', '/api/discussions/2')
$this->request('GET', '/api/posts')
->withQueryParams([
'filter' => ['discussion' => 2]
])
);

$json = json_decode($response->getBody()->getContents(), true);

$this->assertEquals(1, Arr::get($json, 'data.relationships.posts.data.0.id'));
$this->assertEquals(1, Arr::get($json, 'data.0.id'));
}

#[Test]