From 9ff7423b2bf3384c58f1a06ccd31c945ad93409c Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Thu, 3 Dec 2020 20:13:37 +0000 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20Added=20`email.open=5Frate`=20order?= =?UTF-8?q?=20option=20to=20posts=20api=20(#12439)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refs https://github.com/TryGhost/Ghost/issues/12420 - updated `order` bookshelf plugin's `parseOrderOption()` method to return multiple order-related properties - `order` same as before, a key-value object of property-direction - `orderRaw` new property that is a raw SQL order string generated from `orderRawQuery()` method in models - `eagerLoad` new property that is an array of properties the `eagerLoad` plugin should use to join across - updated `pagination.fetchAll()` to apply normal order + raw order if both are available and to handle eager loading / joins when `options.eagerLoad` is populated - updated post model to include details for email relationship and to add `orderRawQuery()` that allows `email.open_rate` to be used as an order option --- core/server/models/base/index.js | 5 +- core/server/models/plugins/order.js | 35 +++++++--- core/server/models/plugins/pagination.js | 10 ++- core/server/models/post.js | 17 +++++ .../regression/api/canary/admin/posts_spec.js | 69 +++++++++++++++++++ test/utils/fixtures/data-generator.js | 13 +++- test/utils/index.js | 22 ++++++ 7 files changed, 156 insertions(+), 15 deletions(-) diff --git a/core/server/models/base/index.js b/core/server/models/base/index.js index c53fa02be6..852e1ad097 100644 --- a/core/server/models/base/index.js +++ b/core/server/models/base/index.js @@ -937,7 +937,10 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ } if (options.order) { - options.order = itemCollection.parseOrderOption(options.order, options.withRelated); + const {order, orderRaw, eagerLoad} = itemCollection.parseOrderOption(options.order, options.withRelated); + options.orderRaw = orderRaw; + options.order = order; + options.eagerLoad = eagerLoad; } else if (options.autoOrder) { options.orderRaw = options.autoOrder; } else if (this.orderDefaultRaw) { diff --git a/core/server/models/plugins/order.js b/core/server/models/plugins/order.js index b65c826abb..395d6cf86b 100644 --- a/core/server/models/plugins/order.js +++ b/core/server/models/plugins/order.js @@ -1,20 +1,21 @@ const _ = require('lodash'); -const order = function order(Bookshelf) { +const orderPlugin = function orderPlugin(Bookshelf) { Bookshelf.Model = Bookshelf.Model.extend({ orderAttributes() {}, + orderRawQuery() {}, parseOrderOption: function (orderQueryString, withRelated) { - let orderAttributes; - let result; - let rules = []; + const order = {}; + const orderRaw = []; + const eagerLoadArray = []; - orderAttributes = this.orderAttributes(); + const orderAttributes = this.orderAttributes(); if (withRelated && withRelated.indexOf('count.posts') > -1) { orderAttributes.push('count.posts'); } - result = {}; + let rules = []; // CASE: repeat order query parameter keys are present if (_.isArray(orderQueryString)) { orderQueryString.forEach((qs) => { @@ -24,7 +25,7 @@ const order = function order(Bookshelf) { rules = orderQueryString.split(','); } - _.each(rules, function (rule) { + _.each(rules, (rule) => { let match; let field; let direction; @@ -39,6 +40,16 @@ const order = function order(Bookshelf) { field = match[1].toLowerCase(); direction = match[2].toUpperCase(); + const orderRawQuery = this.orderRawQuery(field, direction, withRelated); + + if (orderRawQuery) { + orderRaw.push(orderRawQuery.orderByRaw); + if (orderRawQuery.eagerLoad) { + eagerLoadArray.push(orderRawQuery.eagerLoad); + } + return; + } + const matchingOrderAttribute = orderAttributes.find((orderAttribute) => { // NOTE: this logic assumes we use different field names for "parent" and "child" relations. // E.g.: ['parent.title', 'child.title'] and ['child.title', 'parent.title'] - would not @@ -51,12 +62,16 @@ const order = function order(Bookshelf) { return; } - result[matchingOrderAttribute] = direction; + order[matchingOrderAttribute] = direction; }); - return result; + return { + order, + orderRaw: orderRaw.join(', '), + eagerLoad: _.uniq(eagerLoadArray) + }; } }); }; -module.exports = order; +module.exports = orderPlugin; diff --git a/core/server/models/plugins/pagination.js b/core/server/models/plugins/pagination.js index 7dbf2a9acc..6b4c9f9ba4 100644 --- a/core/server/models/plugins/pagination.js +++ b/core/server/models/plugins/pagination.js @@ -207,12 +207,18 @@ const pagination = function pagination(bookshelf) { paginationUtils.handleRelation(self, property); } }); - } else if (options.orderRaw) { - self.query(function (qb) { + } + + if (options.orderRaw) { + self.query((qb) => { qb.orderByRaw(options.orderRaw); }); } + if (!_.isEmpty(options.eagerLoad)) { + options.eagerLoad.forEach(property => paginationUtils.handleRelation(self, property)); + } + if (options.groups && !_.isEmpty(options.groups)) { _.each(options.groups, function (group) { self.query('groupBy', group); diff --git a/core/server/models/post.js b/core/server/models/post.js index becaddceb3..e5c4b57f11 100644 --- a/core/server/models/post.js +++ b/core/server/models/post.js @@ -72,6 +72,10 @@ Post = ghostBookshelf.Model.extend({ posts_meta: { targetTableName: 'posts_meta', foreignKey: 'post_id' + }, + email: { + targetTableName: 'emails', + foreignKey: 'post_id' } }, @@ -99,6 +103,19 @@ Post = ghostBookshelf.Model.extend({ return [...keys, ...postsMetaKeys]; }, + orderRawQuery: function orderRawQuery(field, direction, withRelated) { + if (field === 'email.open_rate' && withRelated && withRelated.indexOf('email') > -1) { + return { + // *1.0 is needed on one of the columns to prevent sqlite from + // performing integer division rounding and always giving 0. + // Order by emails.track_opens desc first so we always tracked emails + // before untracked emails in the posts list + orderByRaw: `emails.track_opens desc, emails.opened_count * 1.0 / emails.email_count * 100 ${direction}`, + eagerLoad: 'email.open_rate' + }; + } + }, + filterExpansions: function filterExpansions() { const postsMetaKeys = _.without(ghostBookshelf.model('PostsMeta').prototype.orderAttributes(), 'posts_meta.id', 'posts_meta.post_id'); diff --git a/test/regression/api/canary/admin/posts_spec.js b/test/regression/api/canary/admin/posts_spec.js index 6f646b377f..afbda23035 100644 --- a/test/regression/api/canary/admin/posts_spec.js +++ b/test/regression/api/canary/admin/posts_spec.js @@ -1,3 +1,4 @@ +const _ = require('lodash'); const should = require('should'); const supertest = require('supertest'); const ObjectId = require('bson-objectid'); @@ -182,6 +183,74 @@ describe('Posts API', function () { done(); }); }); + + it('can order by email open rate', async function () { + try { + await testUtils.createEmailedPost({ + postOptions: { + post: { + slug: '80-open-rate' + } + }, + emailOptions: { + email: { + email_count: 100, + opened_count: 80, + track_opens: true + } + } + }); + + await testUtils.createEmailedPost({ + postOptions: { + post: { + slug: '60-open-rate' + } + }, + emailOptions: { + email: { + email_count: 100, + opened_count: 60, + track_opens: true + } + } + }); + } catch (err) { + if (_.isArray(err)) { + throw err[0]; + } + throw err; + } + + await request.get(localUtils.API.getApiQuery('posts/?order=email.open_rate%20DESC')) + .set('Origin', config.get('url')) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(200) + .then((res) => { + should.not.exist(res.headers['x-cache-invalidate']); + const jsonResponse = res.body; + should.exist(jsonResponse.posts); + localUtils.API.checkResponse(jsonResponse, 'posts'); + jsonResponse.posts.should.have.length(15); + + jsonResponse.posts[0].slug.should.equal('80-open-rate', 'DESC 1st'); + jsonResponse.posts[1].slug.should.equal('60-open-rate', 'DESC 2nd'); + + localUtils.API.checkResponse(jsonResponse.meta.pagination, 'pagination'); + }); + + await request.get(localUtils.API.getApiQuery('posts/?order=email.open_rate%20ASC')) + .set('Origin', config.get('url')) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(200) + .then((res) => { + const jsonResponse = res.body; + jsonResponse.posts[0].slug.should.equal('60-open-rate', 'ASC 1st'); + jsonResponse.posts[1].slug.should.equal('80-open-rate', 'ASC 2nd'); + }); + }); }); describe('Read', function () { diff --git a/test/utils/fixtures/data-generator.js b/test/utils/fixtures/data-generator.js index ddec87fe40..fc35f126d0 100644 --- a/test/utils/fixtures/data-generator.js +++ b/test/utils/fixtures/data-generator.js @@ -748,6 +748,14 @@ DataGenerator.forKnex = (function () { }); } + function createEmail(overrides) { + const newObj = _.cloneDeep(overrides); + + return _.defaults(createBasic(newObj), { + submitted_at: new Date() + }); + } + const posts = [ createPost(DataGenerator.Content.posts[0]), createPost(DataGenerator.Content.posts[1]), @@ -951,8 +959,8 @@ DataGenerator.forKnex = (function () { ]; const emails = [ - createBasic(DataGenerator.Content.emails[0]), - createBasic(DataGenerator.Content.emails[1]) + createEmail(DataGenerator.Content.emails[0]), + createEmail(DataGenerator.Content.emails[1]) ]; const members = [ @@ -1010,6 +1018,7 @@ DataGenerator.forKnex = (function () { createInvite, createWebhook, createIntegration, + createEmail, invites, posts, diff --git a/test/utils/index.js b/test/utils/index.js index 9e0eb045b1..6d22f691ef 100644 --- a/test/utils/index.js +++ b/test/utils/index.js @@ -122,6 +122,14 @@ fixtures = { }); }, + insertEmailedPosts: function insertEmailedPosts({postCount = 2} = {}) { + const posts = []; + + for (let i = 0; i < postCount; i++) { + posts.push(DataGenerator.forKnex.createGenericPost); + } + }, + insertExtraPosts: function insertExtraPosts(max) { let lang; let status; @@ -749,6 +757,19 @@ const createPost = function createPost(options) { return models.Post.add(post, module.exports.context.internal); }; +const createEmail = function createEmail(options) { + const email = DataGenerator.forKnex.createEmail(options.email); + return models.Email.add(email, module.exports.context.internal); +}; + +const createEmailedPost = async function createEmailedPost({postOptions, emailOptions}) { + const post = await createPost(postOptions); + emailOptions.email.post_id = post.id; + const email = await createEmail(emailOptions); + + return {post, email}; +}; + /** * Has to run in a transaction for MySQL, otherwise the foreign key check does not work. * Sqlite3 has no truncate command. @@ -1131,6 +1152,7 @@ module.exports = { setup: setup, createUser: createUser, createPost: createPost, + createEmailedPost, /** * renderObject: res.render(view, dbResponse)