From b5fc527f8d4f9d01e9e8c50dc7dafbfdcf01b11a Mon Sep 17 00:00:00 2001 From: Simon Backx Date: Mon, 2 Oct 2023 16:51:03 +0200 Subject: [PATCH] Added order option to recommendations API with sorting on counts (#18417) refs https://github.com/TryGhost/Product/issues/3957 This changes how we fetch recommendations: - Relations can be included in one query instead of extra queries - Sorting is now possible by click or subscriber counts --- ghost/bookshelf-repository/package.json | 3 +- .../src/BookshelfRepository.ts | 82 +++++-- ghost/bookshelf-repository/src/libraries.d.ts | 2 + .../test/BookshelfRepository.test.ts | 86 ++++++- ghost/core/content/themes/casper | 2 +- .../server/api/endpoints/recommendations.js | 3 +- .../recommendations.test.js.snap | 220 ++++++++++++++++++ .../e2e-api/admin/recommendations.test.js | 52 +++++ .../recommendation-emails.test.js.snap | 214 +++++++++++++++++ .../src/BookshelfRecommendationRepository.ts | 30 ++- ghost/recommendations/src/Recommendation.ts | 36 ++- .../src/RecommendationController.ts | 107 ++++++--- .../src/RecommendationRepository.ts | 15 +- .../src/RecommendationService.ts | 115 ++------- 14 files changed, 790 insertions(+), 177 deletions(-) diff --git a/ghost/bookshelf-repository/package.json b/ghost/bookshelf-repository/package.json index c29a8fcbf4..e93cb560ec 100644 --- a/ghost/bookshelf-repository/package.json +++ b/ghost/bookshelf-repository/package.json @@ -22,7 +22,8 @@ "devDependencies": { "c8": "7.14.0", "mocha": "10.2.0", - "sinon": "15.2.0" + "sinon": "15.2.0", + "@tryghost/nql": "0.11.0" }, "dependencies": { "@tryghost/mongo-utils": "0.5.0", diff --git a/ghost/bookshelf-repository/src/BookshelfRepository.ts b/ghost/bookshelf-repository/src/BookshelfRepository.ts index 49f53d4b69..8de58dd92d 100644 --- a/ghost/bookshelf-repository/src/BookshelfRepository.ts +++ b/ghost/bookshelf-repository/src/BookshelfRepository.ts @@ -1,5 +1,6 @@ import {Knex} from 'knex'; import {mapKeys, chainTransformers} from '@tryghost/mongo-utils'; +import errors from '@tryghost/errors'; type Entity = { id: T; @@ -29,8 +30,18 @@ export type ModelInstance = { save(properties: object, options?: {autoRefresh?: boolean; method?: 'update' | 'insert'}): Promise>; } +type OptionalPropertyOf = Exclude<{ +[K in keyof T]: T extends Record> + ? never + : K +}[keyof T], undefined> + // eslint-disable-next-line @typescript-eslint/no-explicit-any export type OrderOption = any> = Order[]; +// eslint-disable-next-line @typescript-eslint/no-explicit-any +export type IncludeOption = any> = OptionalPropertyOf[]; +// eslint-disable-next-line @typescript-eslint/no-explicit-any +export type AllOptions = any> = { filter?: string; order?: OrderOption; page?: number; limit?: number, include?: IncludeOption } export abstract class BookshelfRepository> { protected Model: ModelClass; @@ -43,6 +54,14 @@ export abstract class BookshelfRepository> { protected abstract modelToEntity (model: ModelInstance): Promise | T | null protected abstract getFieldToColumnMap(): Record; + /** + * override this method to add custom query logic to knex queries + */ + // eslint-disable-next-line @typescript-eslint/no-unused-vars + applyCustomQuery(query: Knex.QueryBuilder, options: AllOptions) { + return; + } + #entityFieldToColumn(field: keyof T): string { const mapping = this.getFieldToColumnMap(); return mapping[field]; @@ -78,50 +97,71 @@ export abstract class BookshelfRepository> { } async getById(id: IDType): Promise { - const model = await this.Model.findOne({id}, {require: false}) as ModelInstance | null; - return model ? this.modelToEntity(model) : null; + const models = await this.#fetchAll({ + filter: `id:'${id}'`, + limit: 1 + }); + if (models.length === 1) { + return models[0]; + } + return null; } - async #fetchAll({filter, order, page, limit}: { filter?: string; order?: OrderOption; page?: number; limit?: number }): Promise { + async #fetchAll(options: AllOptions = {}): Promise { + const {filter, order, page, limit} = options; + if (page !== undefined) { + if (page < 1) { + throw new errors.BadRequestError({message: 'page must be greater or equal to 1'}); + } + if (limit !== undefined && limit < 1) { + throw new errors.BadRequestError({message: 'limit must be greater or equal to 1'}); + } + } + const collection = this.Model.getFilteredCollection({ filter, mongoTransformer: this.#getNQLKeyTransformer() }); const orderString = this.#orderToString(order); - if ((limit && page) || orderString) { - collection - .query((q) => { - if (limit && page) { - q.limit(limit); - q.offset(limit * (page - 1)); - } + collection + .query((q) => { + this.applyCustomQuery(q, options); - if (orderString) { - q.orderByRaw( - orderString - ); - } - }); - } + if (limit) { + q.limit(limit); + } + if (limit && page) { + q.limit(limit); + q.offset(limit * (page - 1)); + } + + if (orderString) { + q.orderByRaw( + orderString + ); + } + }); const models = await collection.fetchAll(); return (await Promise.all(models.map(model => this.modelToEntity(model)))).filter(entity => !!entity) as T[]; } - async getAll({filter, order}: { filter?: string; order?: OrderOption } = {}): Promise { + async getAll({filter, order, include}: Omit, 'page'|'limit'> = {}): Promise { return this.#fetchAll({ filter, - order + order, + include }); } - async getPage({filter, order, page, limit}: { filter?: string; order?: OrderOption; page: number; limit: number }): Promise { + async getPage({filter, order, page, limit, include}: AllOptions & Required, 'page'|'limit'>>): Promise { return this.#fetchAll({ filter, order, page, - limit + limit, + include }); } diff --git a/ghost/bookshelf-repository/src/libraries.d.ts b/ghost/bookshelf-repository/src/libraries.d.ts index 674771117f..d36faa0cfa 100644 --- a/ghost/bookshelf-repository/src/libraries.d.ts +++ b/ghost/bookshelf-repository/src/libraries.d.ts @@ -1 +1,3 @@ declare module '@tryghost/mongo-utils'; +declare module '@tryghost/errors'; +declare module '@tryghost/nql'; diff --git a/ghost/bookshelf-repository/test/BookshelfRepository.test.ts b/ghost/bookshelf-repository/test/BookshelfRepository.test.ts index dc5e0d6776..c46c3757e0 100644 --- a/ghost/bookshelf-repository/test/BookshelfRepository.test.ts +++ b/ghost/bookshelf-repository/test/BookshelfRepository.test.ts @@ -1,6 +1,7 @@ import assert from 'assert'; import {BookshelfRepository, ModelClass, ModelInstance} from '../src/index'; import {Knex} from 'knex'; +import nql from '@tryghost/nql'; type SimpleEntity = { id: string; @@ -87,6 +88,7 @@ class Model implements ModelClass { add(data: object): Promise> { const item = { id: (data as any).id, + ...data, get(field: string): unknown { return (data as any)[field]; }, @@ -106,8 +108,18 @@ class Model implements ModelClass { return Promise.resolve(item); } - getFilteredCollection() { - return this; + getFilteredCollection({filter, mongoTransformer}: {filter?: string, mongoTransformer?: unknown}) { + // Filter all items by filter and mongoTransformer + if (!filter) { + return this; + } + const n = nql(filter, { + transformer: mongoTransformer + }); + + const duplicate = new Model(); + duplicate.items = this.items.filter(item => n.queryJSON(item)); + return duplicate; } count() { @@ -343,6 +355,76 @@ describe('BookshelfRepository', function () { assert(result.length === 3); }); + it('Cannot retrieve zero page number', async function () { + const repository = new SimpleBookshelfRepository(new Model()); + const entities = [{ + id: '1', + deleted: false, + name: 'Kym', + age: 24, + birthday: new Date('2000-01-01').toISOString() + }, { + id: '2', + deleted: false, + name: 'John', + age: 30, + birthday: new Date('2000-01-01').toISOString() + }, { + id: '3', + deleted: false, + name: 'Kevin', + age: 5, + birthday: new Date('2000-01-01').toISOString() + }]; + + for (const entity of entities) { + await repository.save(entity); + } + + const result = repository.getPage({ + order: [], + limit: 5, + page: 0 + }); + + await assert.rejects(result, /page/); + }); + + it('Cannot retrieve zero limit', async function () { + const repository = new SimpleBookshelfRepository(new Model()); + const entities = [{ + id: '1', + deleted: false, + name: 'Kym', + age: 24, + birthday: new Date('2000-01-01').toISOString() + }, { + id: '2', + deleted: false, + name: 'John', + age: 30, + birthday: new Date('2000-01-01').toISOString() + }, { + id: '3', + deleted: false, + name: 'Kevin', + age: 5, + birthday: new Date('2000-01-01').toISOString() + }]; + + for (const entity of entities) { + await repository.save(entity); + } + + const result = repository.getPage({ + order: [], + limit: 0, + page: 5 + }); + + await assert.rejects(result, /limit/); + }); + it('Can retrieve count', async function () { const repository = new SimpleBookshelfRepository(new Model()); const entities = [{ diff --git a/ghost/core/content/themes/casper b/ghost/core/content/themes/casper index 276e2c9d01..4d3319d05c 160000 --- a/ghost/core/content/themes/casper +++ b/ghost/core/content/themes/casper @@ -1 +1 @@ -Subproject commit 276e2c9d0140c902e1c8d3760bc194790722fa71 +Subproject commit 4d3319d05ce92e7b0244e5608d3fc6cc9c86e735 diff --git a/ghost/core/core/server/api/endpoints/recommendations.js b/ghost/core/core/server/api/endpoints/recommendations.js index 4bcc3b3c9c..368156f120 100644 --- a/ghost/core/core/server/api/endpoints/recommendations.js +++ b/ghost/core/core/server/api/endpoints/recommendations.js @@ -11,7 +11,8 @@ module.exports = { 'limit', 'page', 'include', - 'filter' + 'filter', + 'order' ], permissions: true, validation: {}, diff --git a/ghost/core/test/e2e-api/admin/__snapshots__/recommendations.test.js.snap b/ghost/core/test/e2e-api/admin/__snapshots__/recommendations.test.js.snap index d7920f66bc..878943d833 100644 --- a/ghost/core/test/e2e-api/admin/__snapshots__/recommendations.test.js.snap +++ b/ghost/core/test/e2e-api/admin/__snapshots__/recommendations.test.js.snap @@ -1501,6 +1501,116 @@ Object { } `; +exports[`Recommendations Admin API browse Can include click and subscribe counts and order by clicks+subscribe count 1: [body] 1`] = ` +Object { + "meta": Object { + "pagination": Object { + "limit": 5, + "next": null, + "page": 1, + "pages": 1, + "prev": null, + "total": 5, + }, + }, + "recommendations": Array [ + Object { + "count": Object { + "clicks": 3, + "subscribers": 0, + }, + "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "excerpt": "Test excerpt", + "favicon": "https://recommendation3.com/favicon.ico", + "featured_image": "https://recommendation3.com/featured.jpg", + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "one_click_subscribe": true, + "reason": "Reason 3", + "title": "Recommendation 3", + "updated_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "url": "https://recommendation3.com/", + }, + Object { + "count": Object { + "clicks": 2, + "subscribers": 3, + }, + "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "excerpt": "Test excerpt", + "favicon": "https://recommendation4.com/favicon.ico", + "featured_image": "https://recommendation4.com/featured.jpg", + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "one_click_subscribe": true, + "reason": "Reason 4", + "title": "Recommendation 4", + "updated_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "url": "https://recommendation4.com/", + }, + Object { + "count": Object { + "clicks": 0, + "subscribers": 0, + }, + "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "excerpt": "Test excerpt", + "favicon": "https://recommendation0.com/favicon.ico", + "featured_image": "https://recommendation0.com/featured.jpg", + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "one_click_subscribe": true, + "reason": "Reason 0", + "title": "Recommendation 0", + "updated_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "url": "https://recommendation0.com/", + }, + Object { + "count": Object { + "clicks": 0, + "subscribers": 0, + }, + "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "excerpt": "Test excerpt", + "favicon": "https://recommendation1.com/favicon.ico", + "featured_image": "https://recommendation1.com/featured.jpg", + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "one_click_subscribe": true, + "reason": "Reason 1", + "title": "Recommendation 1", + "updated_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "url": "https://recommendation1.com/", + }, + Object { + "count": Object { + "clicks": 0, + "subscribers": 2, + }, + "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "excerpt": "Test excerpt", + "favicon": "https://recommendation2.com/favicon.ico", + "featured_image": "https://recommendation2.com/featured.jpg", + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "one_click_subscribe": true, + "reason": "Reason 2", + "title": "Recommendation 2", + "updated_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "url": "https://recommendation2.com/", + }, + ], +} +`; + +exports[`Recommendations Admin API browse Can include click and subscribe counts and order by clicks+subscribe count 2: [headers] 1`] = ` +Object { + "access-control-allow-origin": "http://127.0.0.1:2369", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "2103", + "content-type": "application/json; charset=utf-8", + "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "vary": "Accept-Version, Origin, Accept-Encoding", + "x-powered-by": "Express", +} +`; + exports[`Recommendations Admin API browse Can include only clicks 1: [body] 1`] = ` Object { "meta": Object { @@ -1711,6 +1821,116 @@ Object { } `; +exports[`Recommendations Admin API browse Can order by click and subscribe counts and they will be included by default 1: [body] 1`] = ` +Object { + "meta": Object { + "pagination": Object { + "limit": 5, + "next": null, + "page": 1, + "pages": 1, + "prev": null, + "total": 5, + }, + }, + "recommendations": Array [ + Object { + "count": Object { + "clicks": 3, + "subscribers": 0, + }, + "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "excerpt": "Test excerpt", + "favicon": "https://recommendation3.com/favicon.ico", + "featured_image": "https://recommendation3.com/featured.jpg", + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "one_click_subscribe": true, + "reason": "Reason 3", + "title": "Recommendation 3", + "updated_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "url": "https://recommendation3.com/", + }, + Object { + "count": Object { + "clicks": 2, + "subscribers": 3, + }, + "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "excerpt": "Test excerpt", + "favicon": "https://recommendation4.com/favicon.ico", + "featured_image": "https://recommendation4.com/featured.jpg", + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "one_click_subscribe": true, + "reason": "Reason 4", + "title": "Recommendation 4", + "updated_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "url": "https://recommendation4.com/", + }, + Object { + "count": Object { + "clicks": 0, + "subscribers": 0, + }, + "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "excerpt": "Test excerpt", + "favicon": "https://recommendation0.com/favicon.ico", + "featured_image": "https://recommendation0.com/featured.jpg", + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "one_click_subscribe": true, + "reason": "Reason 0", + "title": "Recommendation 0", + "updated_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "url": "https://recommendation0.com/", + }, + Object { + "count": Object { + "clicks": 0, + "subscribers": 0, + }, + "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "excerpt": "Test excerpt", + "favicon": "https://recommendation1.com/favicon.ico", + "featured_image": "https://recommendation1.com/featured.jpg", + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "one_click_subscribe": true, + "reason": "Reason 1", + "title": "Recommendation 1", + "updated_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "url": "https://recommendation1.com/", + }, + Object { + "count": Object { + "clicks": 0, + "subscribers": 2, + }, + "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "excerpt": "Test excerpt", + "favicon": "https://recommendation2.com/favicon.ico", + "featured_image": "https://recommendation2.com/featured.jpg", + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "one_click_subscribe": true, + "reason": "Reason 2", + "title": "Recommendation 2", + "updated_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "url": "https://recommendation2.com/", + }, + ], +} +`; + +exports[`Recommendations Admin API browse Can order by click and subscribe counts and they will be included by default 2: [headers] 1`] = ` +Object { + "access-control-allow-origin": "http://127.0.0.1:2369", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "2103", + "content-type": "application/json; charset=utf-8", + "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "vary": "Accept-Version, Origin, Accept-Encoding", + "x-powered-by": "Express", +} +`; + exports[`Recommendations Admin API browse Can request pages 1: [body] 1`] = ` Object { "meta": Object { diff --git a/ghost/core/test/e2e-api/admin/recommendations.test.js b/ghost/core/test/e2e-api/admin/recommendations.test.js index b4f105f67a..76ec7706e4 100644 --- a/ghost/core/test/e2e-api/admin/recommendations.test.js +++ b/ghost/core/test/e2e-api/admin/recommendations.test.js @@ -246,6 +246,58 @@ describe('Recommendations Admin API', function () { assert.equal(page1.recommendations[2].count.subscribers, 2); }); + it('Can include click and subscribe counts and order by clicks+subscribe count', async function () { + await addDummyRecommendations(5); + await addClicksAndSubscribers({memberId}); + + const {body: page1} = await agent.get('recommendations/?include=count.clicks,count.subscribers&order=' + encodeURIComponent('count.clicks desc, count.subscribers asc')) + .expectStatus(200) + .matchHeaderSnapshot({ + 'content-version': anyContentVersion, + etag: anyEtag + }) + .matchBodySnapshot({ + recommendations: new Array(5).fill({ + id: anyObjectId, + created_at: anyISODateTime, + updated_at: anyISODateTime + }) + }); + + assert.equal(page1.recommendations[0].count.clicks, 3); + assert.equal(page1.recommendations[1].count.clicks, 2); + + assert.equal(page1.recommendations[0].count.subscribers, 0); + assert.equal(page1.recommendations[1].count.subscribers, 3); + assert.equal(page1.recommendations[2].count.subscribers, 0); + }); + + it('Can order by click and subscribe counts and they will be included by default', async function () { + await addDummyRecommendations(5); + await addClicksAndSubscribers({memberId}); + + const {body: page1} = await agent.get('recommendations/?order=' + encodeURIComponent('count.clicks desc, count.subscribers asc')) + .expectStatus(200) + .matchHeaderSnapshot({ + 'content-version': anyContentVersion, + etag: anyEtag + }) + .matchBodySnapshot({ + recommendations: new Array(5).fill({ + id: anyObjectId, + created_at: anyISODateTime, + updated_at: anyISODateTime + }) + }); + + assert.equal(page1.recommendations[0].count.clicks, 3); + assert.equal(page1.recommendations[1].count.clicks, 2); + + assert.equal(page1.recommendations[0].count.subscribers, 0); + assert.equal(page1.recommendations[1].count.subscribers, 3); + assert.equal(page1.recommendations[2].count.subscribers, 0); + }); + it('Can fetch recommendations with relations when there are no recommendations', async function () { const recommendations = await recommendationsService.repository.getCount(); assert.equal(recommendations, 0, 'This test expects there to be no recommendations'); diff --git a/ghost/core/test/e2e-server/services/__snapshots__/recommendation-emails.test.js.snap b/ghost/core/test/e2e-server/services/__snapshots__/recommendation-emails.test.js.snap index 42cb8ceeef..72e9497286 100644 --- a/ghost/core/test/e2e-server/services/__snapshots__/recommendation-emails.test.js.snap +++ b/ghost/core/test/e2e-server/services/__snapshots__/recommendation-emails.test.js.snap @@ -1,5 +1,219 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`Incoming Recommendation Emails Sends a different email if we receive a recommendation back 1: [html 1] 1`] = ` +" + + + + + πŸ‘ New recommendation + + + + + + + + + + +
  +
+ + + + + + + + + + +
+ + + + + + + + + + + + + + +
+ +

Great news!

+

One of the sites you're recommending is now recommending you back:

+ +
+ +
+
+ +
Other Ghost Site
+
+
+
+ +
+ + + + + + + +
+ + + + + + +
View recommendations
+
+
+

You can also copy & paste this URL into your browser:

+

http://127.0.0.1:2369/ghost/#/settings-x/recommendations

+
+

This message was sent from 127.0.0.1 to jbloggs@example.com

+
+

Don’t want to receive these emails? Manage your preferences here.

+
+
+ + + +
+ 		 
+ + +" +`; + +exports[`Incoming Recommendation Emails Sends a different email if we receive a recommendation back 2: [text 1] 1`] = ` +" +You have been recommended by Other Ghost Site. + +--- + +Sent to jbloggs@example.com from 127.0.0.1. +If you would no longer like to receive these notifications you can adjust your settings at http://127.0.0.1:2369/ghost/#/settings-x/users/show/joe-bloggs. + " +`; + +exports[`Incoming Recommendation Emails Sends a different email if we receive a recommendation back 3: [metadata 1] 1`] = ` +Object { + "subject": "πŸ‘ New recommendation: Other Ghost Site", + "to": "jbloggs@example.com", +} +`; + exports[`Incoming Recommendation Emails Sends an email if we receive a recommendation 1: [html 1] 1`] = ` " diff --git a/ghost/recommendations/src/BookshelfRecommendationRepository.ts b/ghost/recommendations/src/BookshelfRecommendationRepository.ts index e424f46d8c..fa94a682b3 100644 --- a/ghost/recommendations/src/BookshelfRecommendationRepository.ts +++ b/ghost/recommendations/src/BookshelfRecommendationRepository.ts @@ -1,7 +1,8 @@ +import {AllOptions, BookshelfRepository, ModelClass, ModelInstance} from '@tryghost/bookshelf-repository'; +import logger from '@tryghost/logging'; +import {Knex} from 'knex'; import {Recommendation} from './Recommendation'; import {RecommendationRepository} from './RecommendationRepository'; -import {BookshelfRepository, ModelClass, ModelInstance} from '@tryghost/bookshelf-repository'; -import logger from '@tryghost/logging'; type Sentry = { captureException(err: unknown): void; @@ -24,6 +25,22 @@ export class BookshelfRecommendationRepository extends BookshelfRepository) { + query.select('recommendations.*'); + + if (options.include?.includes('clickCount') || options.order?.find(o => o.field === 'clickCount')) { + query.select((knex: Knex.QueryBuilder) => { + knex.count('*').from('recommendation_click_events').where('recommendation_click_events.recommendation_id', knex.client.raw('recommendations.id')).as('count__clicks'); + }); + } + + if (options.include?.includes('subscriberCount') || options.order?.find(o => o.field === 'subscriberCount')) { + query.select((knex: Knex.QueryBuilder) => { + knex.count('*').from('recommendation_subscribe_events').where('recommendation_subscribe_events.recommendation_id', knex.client.raw('recommendations.id')).as('count__subscribers'); + }); + } + } + toPrimitive(entity: Recommendation): object { return { id: entity.id, @@ -36,6 +53,7 @@ export class BookshelfRecommendationRepository extends BookshelfRepository; } diff --git a/ghost/recommendations/src/Recommendation.ts b/ghost/recommendations/src/Recommendation.ts index 29915850ab..533d581f3d 100644 --- a/ghost/recommendations/src/Recommendation.ts +++ b/ghost/recommendations/src/Recommendation.ts @@ -15,7 +15,13 @@ export type RecommendationPlain = { url: URL oneClickSubscribe: boolean, createdAt: Date, - updatedAt: Date|null + updatedAt: Date|null, + + /** + * These are read only, you cannot change them + */ + clickCount?: number + subscriberCount?: number } export type RecommendationCreateData = { @@ -28,7 +34,13 @@ export type RecommendationCreateData = { url: URL|string oneClickSubscribe: boolean createdAt?: Date - updatedAt?: Date|null + updatedAt?: Date|null, + + /** + * These are read only, you cannot change them + */ + clickCount?: number + subscriberCount?: number } export type AddRecommendation = Omit @@ -45,6 +57,8 @@ export class Recommendation { oneClickSubscribe: boolean; createdAt: Date; updatedAt: Date|null; + #clickCount: number|undefined; + #subscriberCount: number|undefined; #deleted: boolean; @@ -52,6 +66,14 @@ export class Recommendation { return this.#deleted; } + get clickCount() { + return this.#clickCount; + } + + get subscriberCount() { + return this.#subscriberCount; + } + private constructor(data: RecommendationPlain) { this.id = data.id; this.title = data.title; @@ -63,6 +85,8 @@ export class Recommendation { this.oneClickSubscribe = data.oneClickSubscribe; this.createdAt = data.createdAt; this.updatedAt = data.updatedAt; + this.#clickCount = data.clickCount; + this.#subscriberCount = data.subscriberCount; this.#deleted = false; } @@ -122,7 +146,9 @@ export class Recommendation { url: new UnsafeData(data.url).url, oneClickSubscribe: data.oneClickSubscribe, createdAt: data.createdAt ?? new Date(), - updatedAt: data.updatedAt ?? null + updatedAt: data.updatedAt ?? null, + clickCount: data.clickCount, + subscriberCount: data.subscriberCount }; this.validate(d); @@ -143,7 +169,9 @@ export class Recommendation { url: this.url, oneClickSubscribe: this.oneClickSubscribe, createdAt: this.createdAt, - updatedAt: this.updatedAt + updatedAt: this.updatedAt, + clickCount: this.clickCount, + subscriberCount: this.subscriberCount }; } diff --git a/ghost/recommendations/src/RecommendationController.ts b/ghost/recommendations/src/RecommendationController.ts index bb09200666..c885daeed7 100644 --- a/ghost/recommendations/src/RecommendationController.ts +++ b/ghost/recommendations/src/RecommendationController.ts @@ -1,8 +1,9 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ import errors from '@tryghost/errors'; -import {AddRecommendation, RecommendationPlain} from './Recommendation'; -import {RecommendationIncludeFields, RecommendationService, RecommendationWithIncludes} from './RecommendationService'; +import {AddRecommendation, Recommendation, RecommendationPlain} from './Recommendation'; +import {RecommendationService} from './RecommendationService'; import {UnsafeData} from './UnsafeData'; +import {OrderOption} from '@tryghost/bookshelf-repository'; type Frame = { data: unknown, @@ -11,6 +12,17 @@ type Frame = { member: unknown, }; +const RecommendationIncludesMap = { + 'count.clicks': 'clickCount' as const, + 'count.subscribers': 'subscriberCount' as const +}; + +const RecommendationOrderMap = { + 'count.clicks': 'clickCount' as const, + 'count.subscribers': 'subscriberCount' as const, + created_at: 'createdAt' as const +}; + export class RecommendationController { service: RecommendationService; @@ -76,23 +88,63 @@ export class RecommendationController { await this.service.deleteRecommendation(id); } + #stringToOrder(str?: string) { + if (!str) { + // Default order + return [ + { + field: 'createdAt' as const, + direction: 'desc' as const + } + ]; + } + + const parts = str.split(','); + const order: OrderOption = []; + for (const part of parts) { + const trimmed = part.trim(); + const fieldData = new UnsafeData(trimmed.split(' ')[0].trim()); + const directionData = new UnsafeData(trimmed.split(' ')[1]?.trim() ?? 'asc'); + + const validatedField = fieldData.enum( + Object.keys(RecommendationOrderMap) as (keyof typeof RecommendationOrderMap)[] + ); + const direction = directionData.enum(['asc' as const, 'desc' as const]); + + // Convert 'count.' and camelCase to snake_case + const field = RecommendationOrderMap[validatedField]; + order.push({ + field, + direction + }); + } + + if (order.length === 0) { + // Default order + return [ + { + field: 'createdAt' as const, + direction: 'desc' as const + } + ]; + } + return order; + } + async browse(frame: Frame) { const options = new UnsafeData(frame.options); const page = options.optionalKey('page')?.integer ?? 1; const limit = options.optionalKey('limit')?.integer ?? 5; - const include = options.optionalKey('withRelated')?.array.map(item => item.enum(['count.clicks', 'count.subscribers'])) ?? []; + const include = options.optionalKey('withRelated')?.array.map((item) => { + return RecommendationIncludesMap[item.enum( + Object.keys(RecommendationIncludesMap) as (keyof typeof RecommendationIncludesMap)[] + )]; + }) ?? []; const filter = options.optionalKey('filter')?.string; - const orderOption = options.optionalKey('order')?.regex(/^[a-zA-Z]+ (asc|desc)$/) ?? 'createdAt desc'; - const field = orderOption?.split(' ')[0] as keyof RecommendationPlain; - const direction = orderOption?.split(' ')[1] as 'asc'|'desc'; - const order = [ - { - field, - direction - } - ]; + const orderOption = options.optionalKey('order')?.string; + const order = this.#stringToOrder(orderOption); const count = await this.service.countRecommendations({}); const recommendations = (await this.service.listRecommendations({page, limit, filter, include, order})); @@ -154,7 +206,7 @@ export class RecommendationController { return null; } - #serialize(recommendations: RecommendationWithIncludes[], meta?: any) { + #serialize(recommendations: RecommendationPlain[], meta?: any) { return { data: recommendations.map((entity) => { const d = { @@ -168,33 +220,12 @@ export class RecommendationController { one_click_subscribe: entity.oneClickSubscribe, created_at: entity.createdAt, updated_at: entity.updatedAt, - count: undefined as undefined|{clicks?: number, subscribers?: number} + count: entity.clickCount !== undefined || entity.subscriberCount !== undefined ? { + clicks: entity.clickCount, + subscribers: entity.subscriberCount + } : undefined }; - for (const [key, value] of Object.entries(entity)) { - if (key === 'count.clicks') { - if (typeof value !== 'number') { - continue; - } - d.count = { - ...(d.count ?? {}), - clicks: value - }; - continue; - } - - if (key === 'count.subscribers') { - if (typeof value !== 'number') { - continue; - } - d.count = { - ...(d.count ?? {}), - subscribers: value - }; - continue; - } - } - return d; }), meta diff --git a/ghost/recommendations/src/RecommendationRepository.ts b/ghost/recommendations/src/RecommendationRepository.ts index a569a7f3aa..2daa706094 100644 --- a/ghost/recommendations/src/RecommendationRepository.ts +++ b/ghost/recommendations/src/RecommendationRepository.ts @@ -1,18 +1,13 @@ -import {OrderOption} from '@tryghost/bookshelf-repository'; +import {AllOptions} from '@tryghost/bookshelf-repository'; import {Recommendation} from './Recommendation'; export interface RecommendationRepository { save(entity: Recommendation): Promise; getById(id: string): Promise; - getByUrl(url: URL): Promise; - getAll({filter, order}?: {filter?: string, order?: OrderOption}): Promise; - getPage({filter, order, page, limit}: { - filter?: string; - order?: OrderOption; - page: number; - limit: number; - }): Promise; - getCount({filter}?: { + getByUrl(url: URL): Promise; + getAll(options: Omit, 'page'|'limit'>): Promise; + getPage(options: AllOptions & Required, 'page'|'limit'>>): Promise; + getCount(options: { filter?: string; }): Promise; }; diff --git a/ghost/recommendations/src/RecommendationService.ts b/ghost/recommendations/src/RecommendationService.ts index ee5405344c..44c9831dc3 100644 --- a/ghost/recommendations/src/RecommendationService.ts +++ b/ghost/recommendations/src/RecommendationService.ts @@ -1,27 +1,12 @@ -import {BookshelfRepository, OrderOption} from '@tryghost/bookshelf-repository'; -import {AddRecommendation, Recommendation, RecommendationPlain} from './Recommendation'; -import {RecommendationRepository} from './RecommendationRepository'; -import {WellknownService} from './WellknownService'; +import {BookshelfRepository, IncludeOption, OrderOption} from '@tryghost/bookshelf-repository'; import errors from '@tryghost/errors'; +import logging from '@tryghost/logging'; import tpl from '@tryghost/tpl'; import {ClickEvent} from './ClickEvent'; +import {AddRecommendation, Recommendation, RecommendationPlain} from './Recommendation'; +import {RecommendationRepository} from './RecommendationRepository'; import {SubscribeEvent} from './SubscribeEvent'; -import logging from '@tryghost/logging'; - -export type RecommendationIncludeTypes = { - 'count.clicks': number, - 'count.subscribers': number -}; -export type RecommendationIncludeFields = keyof RecommendationIncludeTypes; - -/** - * All includes are optional, but if they are explicitly loaded, they will not be optional in the result. - * - * E.g. RecommendationWithIncludes['count.clicks'|'count.subscribers']. - * - * When using methods like listRecommendations with the include option, the result will automatically return the correct relations - */ -export type RecommendationWithIncludes = RecommendationPlain & Partial & Record; +import {WellknownService} from './WellknownService'; type MentionSendingService = { sendAll(options: {url: URL, links: URL[]}): Promise @@ -164,85 +149,25 @@ export class RecommendationService { this.sendMentionToRecommendation(existing); } - async #listRecommendations({page, limit, filter, order}: { page: number; limit: number | 'all', filter?: string, order?: OrderOption} = {page: 1, limit: 'all'}): Promise { - let list: Recommendation[]; - if (limit === 'all') { - list = await this.repository.getAll({filter, order}); - } else { - if (page < 1) { - throw new errors.BadRequestError({message: 'page must be greater or equal to 1'}); - } - if (limit < 1) { - throw new errors.BadRequestError({message: 'limit must be greater or equal to 1'}); - } - list = await this.repository.getPage({page, limit, filter, order}); - } - return list; - } - /** - * Same as #listRecommendations, but with includes and returns a plain object for external use + * Sames as listRecommendations, but returns Entities instead of plain objects (Entities are only used internally) */ - async listRecommendations({page, limit, filter, order, include}: { page: number; limit: number | 'all', filter?: string, order?: OrderOption, include?: IncludeFields[] } = {page: 1, limit: 'all', include: []}): Promise[]> { - const list = await this.#listRecommendations({page, limit, filter, order}); - return await this.loadRelations(list, include); + async #listRecommendations(options: { filter?: string; order?: OrderOption; page?: number; limit?: number|'all', include?: IncludeOption } = {page: 1, limit: 'all'}): Promise { + if (options.limit === 'all') { + return await this.repository.getAll({ + ...options + }); + } + return await this.repository.getPage({ + ...options, + page: options.page || 1, + limit: options.limit || 15 + }); } - async loadRelations(list: Recommendation[], include?: IncludeFields[]): Promise[]> { - const plainList: RecommendationWithIncludes[] = list.map(e => e.plain); - - if (!include || !include.length) { - return plainList as RecommendationWithIncludes[]; - } - - if (list.length === 0) { - // Avoid doing queries with broken filters - return plainList as RecommendationWithIncludes[]; - } - - for (const relation of include) { - switch (relation) { - case 'count.clicks': - const clickCounts = await this.clickEventRepository.getGroupedCount({groupBy: 'recommendationId', filter: `recommendationId:[${list.map(entity => entity.id).join(',')}]`}); - - // Set all to zero by default - for (const entity of plainList) { - entity[relation] = 0; - } - - for (const r of clickCounts) { - const entity = plainList.find(e => e.id === r.recommendationId); - if (entity) { - entity[relation] = r.count; - } - } - - break; - - case 'count.subscribers': - const subscribersCounts = await this.subscribeEventRepository.getGroupedCount({groupBy: 'recommendationId', filter: `recommendationId:[${list.map(entity => entity.id).join(',')}]`}); - - // Set all to zero by default - for (const entity of plainList) { - entity[relation] = 0; - } - - for (const r of subscribersCounts) { - const entity = plainList.find(e => e.id === r.recommendationId); - if (entity) { - entity[relation] = r.count; - } - } - - break; - default: - // Should create a Type compile error in case we didn't catch all relations - const r: never = relation; - console.error(`Unknown relation ${r}`); // eslint-disable-line no-console - } - } - - return plainList as RecommendationWithIncludes[]; + async listRecommendations(options: { filter?: string; order?: OrderOption; page?: number; limit?: number|'all', include?: IncludeOption } = {page: 1, limit: 'all', include: []}): Promise { + const list = await this.#listRecommendations(options); + return list.map(e => e.plain); } async countRecommendations({filter}: { filter?: string }) {