From 4604ba1587d679bac181ec6325786dcdc96bc17b Mon Sep 17 00:00:00 2001 From: Fabien 'egg' O'Carroll Date: Wed, 11 Nov 2020 13:03:41 +0000 Subject: [PATCH] Fixed backward compatibility for `send_email_when_published` (#12357) no-issue * Handled send_email_when_published in Posts API This restores backwards compatibility of the Posts API allowing existing clients to continue to use the `send_email_when_published` flag. This change uses two edits, which is unfortunate. The reason being is that this is an API compatibility issue, not a model issue, so we shouldn't introduce code to the model layer to handle it. The visibility property of the model is used to determine how to fall back, and because it can be left out of the API request, and relies on a default in the settings, we require that the model decide on the `visibility` before we run our fallback logic (or we duplicate the `visibility` default at the cost of maintenance in the future) * Dropped send_email_when_published column from posts Since this column is not used any more, we can drop it from the table. We include an extra migration to repopulate the column in the event of a rollback * Updated importer to handle send_email_when_published Because we currently export this value from Ghost, we should correctly import it. This follows the same logic as the migrations for this value. * Included send_email_when_published in API response As our v3 API documentation includes `send_email_when_published` we must retain backward compatibility by calculating the property. * Fixed fields filter with send_email_when_published * Added safety checks to frame properties Some parts of the code pass a manually created "frame" which is missing lots of properties, so we check for the existence of all of them before using them. * Fixed 3.1 migration to include columnDefinition We require that migrations have all the information they need contained within them as they run in an unknown state of the codebase, which could be from the commit they are introduced, to any future commit. In this case the column definition is removed from the schema in 3.38 and the migration would fail when run in this version or later. --- core/server/api/canary/posts.js | 11 +++++++- .../canary/utils/serializers/input/posts.js | 8 ++++++ .../utils/serializers/output/utils/clean.js | 12 +++++++- .../utils/serializers/output/utils/mapper.js | 8 ++++++ .../data/importer/importers/data/posts.js | 9 ++++++ ...-add-send-email-when-published-to-posts.js | 7 ++++- ...end-email-when-published-down-migration.js | 25 +++++++++++++++++ ...remove-send-email-when-published-column.js | 28 +++++++++++++++++++ core/server/data/schema/schema.js | 1 - core/server/models/post.js | 1 - test/api-acceptance/admin/utils.js | 2 +- test/api-acceptance/content/posts_spec.js | 21 ++++++++++++++ test/api-acceptance/content/utils.js | 1 + test/regression/api/canary/admin/utils.js | 1 + test/regression/api/canary/content/utils.js | 1 + test/regression/api/v2/admin/utils.js | 1 - test/regression/api/v2/content/utils.js | 1 - test/regression/api/v3/admin/utils.js | 1 + test/regression/api/v3/content/utils.js | 1 + test/unit/data/schema/integrity_spec.js | 2 +- 20 files changed, 133 insertions(+), 9 deletions(-) create mode 100644 core/server/data/migrations/versions/3.38/08-repopulate-send-email-when-published-down-migration.js create mode 100644 core/server/data/migrations/versions/3.38/09-remove-send-email-when-published-column.js diff --git a/core/server/api/canary/posts.js b/core/server/api/canary/posts.js index 890e074fee..4f8dd5eaa6 100644 --- a/core/server/api/canary/posts.js +++ b/core/server/api/canary/posts.js @@ -125,6 +125,7 @@ module.exports = { 'formats', 'source', 'email_recipient_filter', + 'send_email_when_published', 'force_rerender', // NOTE: only for internal context 'forUpdate', @@ -143,6 +144,9 @@ module.exports = { }, email_recipient_filter: { values: ['none', 'free', 'paid', 'all'] + }, + send_email_when_published: { + values: [true, false] } } }, @@ -151,12 +155,17 @@ module.exports = { }, async query(frame) { /**Check host limits for members when send email is true**/ - if (frame.options.email_recipient_filter && frame.options.email_recipient_filter !== 'none') { + if ((frame.options.email_recipient_filter && frame.options.email_recipient_filter !== 'none') || frame.options.send_email_when_published) { await membersService.checkHostLimit(); } let model = await models.Post.edit(frame.data.posts[0], frame.options); + if (!frame.options.email_recipient_filter && frame.options.send_email_when_published) { + frame.options.email_recipient_filter = model.get('visibility') === 'paid' ? 'paid' : 'all'; + model = await models.Post.edit(frame.data.posts[0], frame.options); + } + /**Handle newsletter email */ if (model.get('email_recipient_filter') !== 'none') { const postPublished = model.wasChanged() && (model.get('status') === 'published') && (model.previous('status') !== 'published'); diff --git a/core/server/api/canary/utils/serializers/input/posts.js b/core/server/api/canary/utils/serializers/input/posts.js index f2a760e8ec..190bdb3b91 100644 --- a/core/server/api/canary/utils/serializers/input/posts.js +++ b/core/server/api/canary/utils/serializers/input/posts.js @@ -108,6 +108,10 @@ module.exports = { forcePageFilter(frame); + if (frame.options.columns && frame.options.columns.includes('send_email_when_published')) { + frame.options.columns.push('email_recipient_filter'); + } + /** * ## current cases: * - context object is empty (functional call, content api access) @@ -136,6 +140,10 @@ module.exports = { forcePageFilter(frame); + if (frame.options.columns && frame.options.columns.includes('send_email_when_published')) { + frame.options.columns.push('email_recipient_filter'); + } + /** * ## current cases: * - context object is empty (functional call, content api access) diff --git a/core/server/api/canary/utils/serializers/output/utils/clean.js b/core/server/api/canary/utils/serializers/output/utils/clean.js index 8877ff2481..3481532665 100644 --- a/core/server/api/canary/utils/serializers/output/utils/clean.js +++ b/core/server/api/canary/utils/serializers/output/utils/clean.js @@ -70,6 +70,8 @@ const author = (attrs, frame) => { }; const post = (attrs, frame) => { + const columns = frame && frame.options && frame.options.columns || null; + const fields = frame && frame.original && frame.original.query && frame.original.query.fields || null; if (localUtils.isContentAPI(frame)) { // @TODO: https://github.com/TryGhost/Ghost/issues/10335 // delete attrs.page; @@ -95,13 +97,21 @@ const post = (attrs, frame) => { attrs.og_description = null; } // NOTE: the visibility column has to be always present in Content API response to perform content gating - if (frame.options.columns && frame.options.columns.includes('visibility') && !frame.original.query.fields.includes('visibility')) { + if (columns && columns.includes('visibility') && fields && !fields.includes('visibility')) { delete attrs.visibility; } } else { delete attrs.page; } + if (columns && columns.includes('email_recipient_filter') && fields && !fields.includes('email_recipient_filter')) { + delete attrs.email_recipient_filter; + } + + if (fields && !fields.includes('send_email_when_published')) { + delete attrs.send_email_when_published; + } + if (!attrs.tags) { delete attrs.primary_tag; } diff --git a/core/server/api/canary/utils/serializers/output/utils/mapper.js b/core/server/api/canary/utils/serializers/output/utils/mapper.js index d6deaac5fb..778e8744e7 100644 --- a/core/server/api/canary/utils/serializers/output/utils/mapper.js +++ b/core/server/api/canary/utils/serializers/output/utils/mapper.js @@ -47,6 +47,14 @@ const mapPost = (model, frame) => { gating.forPost(jsonModel, frame); } + if (typeof jsonModel.email_recipient_filter === 'undefined') { + jsonModel.send_email_when_published = null; + } else if (jsonModel.email_recipient_filter === 'none') { + jsonModel.send_email_when_published = false; + } else { + jsonModel.send_email_when_published = true; + } + clean.post(jsonModel, frame); if (frame.options && frame.options.withRelated) { diff --git a/core/server/data/importer/importers/data/posts.js b/core/server/data/importer/importers/data/posts.js index a53185f082..18dd333a59 100644 --- a/core/server/data/importer/importers/data/posts.js +++ b/core/server/data/importer/importers/data/posts.js @@ -33,6 +33,15 @@ class PostsImporter extends BaseImporter { } delete obj.page; } + + if (_.has(obj, 'send_email_when_published')) { + if (obj.send_email_when_published) { + obj.email_recipient_filter = obj.visibility === 'paid' ? 'paid' : 'all'; + } else { + obj.email_recipient_filter = 'none'; + } + delete obj.send_email_when_published; + } }); } diff --git a/core/server/data/migrations/versions/3.1/01-add-send-email-when-published-to-posts.js b/core/server/data/migrations/versions/3.1/01-add-send-email-when-published-to-posts.js index 6e54de0a2a..7bfb09b0d4 100644 --- a/core/server/data/migrations/versions/3.1/01-add-send-email-when-published-to-posts.js +++ b/core/server/data/migrations/versions/3.1/01-add-send-email-when-published-to-posts.js @@ -7,7 +7,12 @@ module.exports.up = commands.createColumnMigration({ return columnExists === true; }, operation: commands.addColumn, - operationVerb: 'Adding' + operationVerb: 'Adding', + columnDefinition: { + type: 'bool', + nullable: true, + defaultTo: false + } }); module.exports.down = commands.createColumnMigration({ diff --git a/core/server/data/migrations/versions/3.38/08-repopulate-send-email-when-published-down-migration.js b/core/server/data/migrations/versions/3.38/08-repopulate-send-email-when-published-down-migration.js new file mode 100644 index 0000000000..93b53dee3e --- /dev/null +++ b/core/server/data/migrations/versions/3.38/08-repopulate-send-email-when-published-down-migration.js @@ -0,0 +1,25 @@ +const logging = require('../../../../../shared/logging'); +const {createTransactionalMigration} = require('../../utils'); + +module.exports = createTransactionalMigration( + async function up() {}, + + async function down(connection) { + logging.info('Setting "send_email_when_published" based on "email_recipient_filter"'); + await connection('posts') + .update({ + send_email_when_published: true + }) + .whereNot({ + email_recipient_filter: 'none' + }); + + await connection('posts') + .update({ + send_email_when_published: false + }) + .where({ + email_recipient_filter: 'none' + }); + } +); diff --git a/core/server/data/migrations/versions/3.38/09-remove-send-email-when-published-column.js b/core/server/data/migrations/versions/3.38/09-remove-send-email-when-published-column.js new file mode 100644 index 0000000000..f9174a1a57 --- /dev/null +++ b/core/server/data/migrations/versions/3.38/09-remove-send-email-when-published-column.js @@ -0,0 +1,28 @@ +const {createColumnMigration, addColumn, dropColumn} = require('../../../schema/commands'); + +module.exports = { + up: createColumnMigration({ + table: 'posts', + column: 'send_email_when_published', + dbIsInCorrectState(columnExists) { + return columnExists === false; + }, + operation: dropColumn, + operationVerb: 'Removing' + }), + + down: createColumnMigration({ + table: 'posts', + column: 'send_email_when_published', + dbIsInCorrectState(columnExists) { + return columnExists === true; + }, + operation: addColumn, + operationVerb: 'Adding', + columnDefinition: { + type: 'bool', + nullable: true, + defaultTo: false + } + }) +}; diff --git a/core/server/data/schema/schema.js b/core/server/data/schema/schema.js index 51b6e54d9f..d9bf13a936 100644 --- a/core/server/data/schema/schema.js +++ b/core/server/data/schema/schema.js @@ -29,7 +29,6 @@ module.exports = { defaultTo: 'public', validations: {isIn: [['public', 'members', 'paid']]} }, - send_email_when_published: {type: 'bool', nullable: true, defaultTo: false}, email_recipient_filter: { type: 'string', maxlength: 50, diff --git a/core/server/models/post.js b/core/server/models/post.js index dea20aba53..3933c75415 100644 --- a/core/server/models/post.js +++ b/core/server/models/post.js @@ -50,7 +50,6 @@ Post = ghostBookshelf.Model.extend({ } return { - send_email_when_published: false, uuid: uuid.v4(), status: 'draft', featured: false, diff --git a/test/api-acceptance/admin/utils.js b/test/api-acceptance/admin/utils.js index b777c14a73..faeb712b78 100644 --- a/test/api-acceptance/admin/utils.js +++ b/test/api-acceptance/admin/utils.js @@ -44,6 +44,7 @@ const expectedProperties = { .concat( ..._(schema.posts_meta).keys().without('post_id', 'id') ) + .concat('send_email_when_published') , page: _(schema.posts) @@ -57,7 +58,6 @@ const expectedProperties = { // deprecated .without('author_id', 'author') // pages are not sent as emails - .without('send_email_when_published') .without('email_recipient_filter') // always returns computed properties .concat('url', 'primary_tag', 'primary_author', 'excerpt') diff --git a/test/api-acceptance/content/posts_spec.js b/test/api-acceptance/content/posts_spec.js index adbaa160e1..43de452da2 100644 --- a/test/api-acceptance/content/posts_spec.js +++ b/test/api-acceptance/content/posts_spec.js @@ -202,6 +202,27 @@ describe('Posts Content API', function () { }); }); + it('Can request send_email_when_published calculated field of posts', function (done) { + request.get(localUtils.API.getApiQuery(`posts/?key=${validKey}&fields=url&fields=send_email_when_published`)) + .set('Origin', testUtils.API.getURL()) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(200) + .end(function (err, res) { + if (err) { + return done(err); + } + const jsonResponse = res.body; + + should.exist(jsonResponse.posts); + + localUtils.API.checkResponse(jsonResponse.posts[0], 'post', false, false, ['url', 'send_email_when_published']); + res.body.posts[0].url.should.eql('http://127.0.0.1:2369/welcome/'); + res.body.posts[0].send_email_when_published.should.eql(false); + done(); + }); + }); + it('Can include relations', function (done) { request.get(localUtils.API.getApiQuery(`posts/?key=${validKey}&include=tags,authors`)) .set('Origin', testUtils.API.getURL()) diff --git a/test/api-acceptance/content/utils.js b/test/api-acceptance/content/utils.js index 7e56248ecf..1b13ba5ada 100644 --- a/test/api-acceptance/content/utils.js +++ b/test/api-acceptance/content/utils.js @@ -36,6 +36,7 @@ const expectedProperties = { ..._(schema.posts_meta).keys().without('post_id', 'id') ) .concat('reading_time') + .concat('send_email_when_published') , author: _(schema.users) .keys() diff --git a/test/regression/api/canary/admin/utils.js b/test/regression/api/canary/admin/utils.js index f52b7cf101..aae318a491 100644 --- a/test/regression/api/canary/admin/utils.js +++ b/test/regression/api/canary/admin/utils.js @@ -36,6 +36,7 @@ const expectedProperties = { .concat( ..._(schema.posts_meta).keys().without('post_id', 'id') ) + .concat('send_email_when_published') , user: _(schema.users) .keys() diff --git a/test/regression/api/canary/content/utils.js b/test/regression/api/canary/content/utils.js index 4fac2a65ca..a6c9a33774 100644 --- a/test/regression/api/canary/content/utils.js +++ b/test/regression/api/canary/content/utils.js @@ -35,6 +35,7 @@ const expectedProperties = { ..._(schema.posts_meta).keys().without('post_id', 'id') ) .concat('reading_time') + .concat('send_email_when_published') , author: _(schema.users) .keys() diff --git a/test/regression/api/v2/admin/utils.js b/test/regression/api/v2/admin/utils.js index 36049916ae..5c144cc9ee 100644 --- a/test/regression/api/v2/admin/utils.js +++ b/test/regression/api/v2/admin/utils.js @@ -27,7 +27,6 @@ const expectedProperties = { .without('page') .without('author_id', 'author') // emails are not supported in API v2 - .without('send_email_when_published') .without('email_recipient_filter') // always returns computed properties .concat('url', 'primary_tag', 'primary_author', 'excerpt') diff --git a/test/regression/api/v2/content/utils.js b/test/regression/api/v2/content/utils.js index 915a78ff65..d5d140fb63 100644 --- a/test/regression/api/v2/content/utils.js +++ b/test/regression/api/v2/content/utils.js @@ -22,7 +22,6 @@ const expectedProperties = { // v2 API doesn't return unused fields .without('locale', 'visibility') // emails are not supported in API v2 - .without('send_email_when_published') .without('email_recipient_filter') // These fields aren't useful as they always have known values .without('status') diff --git a/test/regression/api/v3/admin/utils.js b/test/regression/api/v3/admin/utils.js index b98c48483e..2427c6e7f1 100644 --- a/test/regression/api/v3/admin/utils.js +++ b/test/regression/api/v3/admin/utils.js @@ -35,6 +35,7 @@ const expectedProperties = { .concat( ..._(schema.posts_meta).keys().without('post_id', 'id') ) + .concat('send_email_when_published') , user: _(schema.users) .keys() diff --git a/test/regression/api/v3/content/utils.js b/test/regression/api/v3/content/utils.js index 5895ad0fc9..956bf64432 100644 --- a/test/regression/api/v3/content/utils.js +++ b/test/regression/api/v3/content/utils.js @@ -35,6 +35,7 @@ const expectedProperties = { ..._(schema.posts_meta).keys().without('post_id', 'id') ) .concat('reading_time') + .concat('send_email_when_published') , author: _(schema.users) .keys() diff --git a/test/unit/data/schema/integrity_spec.js b/test/unit/data/schema/integrity_spec.js index 1e1438e854..0a1148774c 100644 --- a/test/unit/data/schema/integrity_spec.js +++ b/test/unit/data/schema/integrity_spec.js @@ -32,7 +32,7 @@ const defaultSettings = require('../../../../core/server/data/schema/default-set */ describe('DB version integrity', function () { // Only these variables should need updating - const currentSchemaHash = '97705c7f5ae33414fcdb009c143480a8'; + const currentSchemaHash = '102b04bbd38cd2451fbf0957ffc35b30'; const currentFixturesHash = 'd46d696c94d03e41a5903500547fea77'; const currentSettingsHash = '229360069a9c77a945727a3c5869c3c6'; const currentRoutesHash = '3d180d52c663d173a6be791ef411ed01';