From 44e602b44778d325f61990fd5e51c4bd733f7f7d Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Wed, 28 Feb 2024 18:42:02 +0000 Subject: [PATCH] Switched to default ordering for comments API requests (#19774) closes ENG-681 There's no need to provide an `order` param with every request in Comments-UI if the API has default ordering that matches our requirements. The order param makes logs more noisy/harder to read than they need to be so we want to get rid of it. - modified comments API input serializer to add a default order param to the browse and replies endpoints when none is provided - removed order param from the requests that Comments-UI makes --- apps/comments-ui/src/utils/api.ts | 6 +- .../utils/serializers/input/comments.js | 14 +++ .../__snapshots__/comments.test.js.snap | 88 +++++++++++++++++++ .../e2e-api/members-comments/comments.test.js | 29 +++++- 4 files changed, 129 insertions(+), 8 deletions(-) diff --git a/apps/comments-ui/src/utils/api.ts b/apps/comments-ui/src/utils/api.ts index e26e3107da..a30903aa1c 100644 --- a/apps/comments-ui/src/utils/api.ts +++ b/apps/comments-ui/src/utils/api.ts @@ -127,12 +127,9 @@ function setupGhostApi({siteUrl = window.location.origin, apiUrl, apiKey}: {site filter = `created_at:<=${firstCommentCreatedAt}`; } - const order = 'created_at DESC, id DESC'; - const params = new URLSearchParams(); params.set('limit', '5'); - params.set('order', order); if (filter) { params.set('filter', filter); } @@ -166,9 +163,8 @@ function setupGhostApi({siteUrl = window.location.origin, apiUrl, apiKey}: {site }, async replies({commentId, afterReplyId, limit}: {commentId: string; afterReplyId: string; limit?: number | 'all'}) { const filter = encodeURIComponent(`id:>'${afterReplyId}'`); - const order = encodeURIComponent('created_at ASC, id ASC'); - const url = endpointFor({type: 'members', resource: `comments/${commentId}/replies`, params: `?limit=${limit ?? 5}&order=${order}&filter=${filter}`}); + const url = endpointFor({type: 'members', resource: `comments/${commentId}/replies`, params: `?limit=${limit ?? 5}&filter=${filter}`}); const res = await makeRequest({ url, method: 'GET', diff --git a/ghost/core/core/server/api/endpoints/utils/serializers/input/comments.js b/ghost/core/core/server/api/endpoints/utils/serializers/input/comments.js index ed60b58901..77a6b09aaa 100644 --- a/ghost/core/core/server/api/endpoints/utils/serializers/input/comments.js +++ b/ghost/core/core/server/api/endpoints/utils/serializers/input/comments.js @@ -14,5 +14,19 @@ module.exports = { } return relation; }); + }, + + browse(apiConfig, frame) { + // for top-level comments we show newest comments first and paginate to older + if (!frame.options.order) { + frame.options.order = 'created_at DESC, id DESC'; + } + }, + + replies(apiConfig, frame) { + // for replies we show the oldest comments first and paginate to newer + if (!frame.options.order) { + frame.options.order = 'created_at ASC, id ASC'; + } } }; diff --git a/ghost/core/test/e2e-api/members-comments/__snapshots__/comments.test.js.snap b/ghost/core/test/e2e-api/members-comments/__snapshots__/comments.test.js.snap index be79c1f039..327c3d6529 100644 --- a/ghost/core/test/e2e-api/members-comments/__snapshots__/comments.test.js.snap +++ b/ghost/core/test/e2e-api/members-comments/__snapshots__/comments.test.js.snap @@ -1844,6 +1844,94 @@ Object { } `; +exports[`Comments API when commenting enabled for all when authenticated Can browse all comments of a post with default order 1: [body] 1`] = ` +Object { + "comments": Array [ + Object { + "count": Object { + "likes": Any, + "replies": 0, + }, + "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "edited_at": null, + "html": "

This is a message

New line

", + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "liked": Any, + "member": Object { + "avatar_image": null, + "expertise": null, + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "name": null, + "uuid": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/, + }, + "replies": Array [], + "status": "published", + }, + Object { + "count": Object { + "likes": Any, + "replies": Any, + }, + "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "edited_at": null, + "html": "

First.

", + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "liked": Any, + "member": Object { + "avatar_image": null, + "expertise": null, + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "name": "Mr Egg", + "uuid": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/, + }, + "replies": Array [ + Object { + "count": Object { + "likes": Any, + }, + "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "edited_at": null, + "html": "

Really original

", + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "liked": Any, + "member": Object { + "avatar_image": null, + "expertise": null, + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "name": null, + "uuid": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/, + }, + "status": "published", + }, + ], + "status": "published", + }, + ], + "meta": Object { + "pagination": Object { + "limit": 15, + "next": null, + "page": 1, + "pages": 1, + "prev": null, + "total": 2, + }, + }, +} +`; + +exports[`Comments API when commenting enabled for all when authenticated Can browse all comments of a post with default order 2: [headers] 1`] = ` +Object { + "access-control-allow-origin": "*", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "1118", + "content-type": "application/json; charset=utf-8", + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "vary": "Accept-Encoding", + "x-powered-by": "Express", +} +`; + exports[`Comments API when commenting enabled for all when authenticated Can comment on a post 1: [body] 1`] = ` Object { "comments": Array [ diff --git a/ghost/core/test/e2e-api/members-comments/comments.test.js b/ghost/core/test/e2e-api/members-comments/comments.test.js index 475a4b0dcf..de681411aa 100644 --- a/ghost/core/test/e2e-api/members-comments/comments.test.js +++ b/ghost/core/test/e2e-api/members-comments/comments.test.js @@ -319,18 +319,38 @@ describe('Comments API', function () { }); it('Can browse all comments of a post (legacy)', async function () { + // uses explicit order to match db ordering await membersAgent - .get(`/api/comments/?filter=post_id:'${postId}'`) + .get(`/api/comments/?filter=post_id:'${postId}'&order=id%20ASC`) .expectStatus(200) .matchHeaderSnapshot({ etag: anyEtag }) .matchBodySnapshot({ - comments: [commentMatcherWithReplies({replies: 1}), commentMatcher] + comments: [ + commentMatcherWithReplies({replies: 1}), + commentMatcher + ] }); }); it('Can browse all comments of a post', async function () { + // uses explicit order to match db ordering + await membersAgent + .get(`/api/comments/post/${postId}/?order=id%20ASC`) + .expectStatus(200) + .matchHeaderSnapshot({ + etag: anyEtag + }) + .matchBodySnapshot({ + comments: [ + commentMatcherWithReplies({replies: 1}), + commentMatcher + ] + }); + }); + + it('Can browse all comments of a post with default order', async function () { await membersAgent .get(`/api/comments/post/${postId}/`) .expectStatus(200) @@ -338,7 +358,10 @@ describe('Comments API', function () { etag: anyEtag }) .matchBodySnapshot({ - comments: [commentMatcherWithReplies({replies: 1}), commentMatcher] + comments: [ + commentMatcher, + commentMatcherWithReplies({replies: 1}) + ] }); });