diff --git a/core/server/api/canary/posts.js b/core/server/api/canary/posts.js index 831080ca7e..136b440047 100644 --- a/core/server/api/canary/posts.js +++ b/core/server/api/canary/posts.js @@ -3,6 +3,7 @@ const i18n = require('../../../shared/i18n'); const errors = require('@tryghost/errors'); const urlUtils = require('../../../shared/url-utils'); const {mega} = require('../../services/mega'); +const {BadRequestError} = require('@tryghost/errors'); const allowedIncludes = ['tags', 'authors', 'authors.roles', 'email']; const unsafeAttrs = ['status', 'authors', 'visibility']; @@ -141,9 +142,6 @@ module.exports = { source: { values: ['html'] }, - email_recipient_filter: { - values: ['none', 'free', 'paid', 'all'] - }, send_email_when_published: { values: [true, false] } @@ -183,7 +181,20 @@ module.exports = { } /**Handle newsletter email */ - if (model.get('email_recipient_filter') !== 'none') { + const emailRecipientFilter = model.get('email_recipient_filter'); + if (emailRecipientFilter !== 'none') { + if (emailRecipientFilter !== 'all') { + // check filter is valid + try { + await models.Member.findPage({filter: `subscribed:true+${emailRecipientFilter}`, limit: 1}); + } catch (err) { + return Promise.reject(new BadRequestError({ + message: i18n.t('errors.api.posts.invalidEmailRecipientFilter'), + context: err.message + })); + } + } + const postPublished = model.wasChanged() && (model.get('status') === 'published') && (model.previous('status') !== 'published'); if (postPublished) { let postEmail = model.relations.email; diff --git a/core/server/api/canary/utils/serializers/input/posts.js b/core/server/api/canary/utils/serializers/input/posts.js index f2a760e8ec..1364867ec0 100644 --- a/core/server/api/canary/utils/serializers/input/posts.js +++ b/core/server/api/canary/utils/serializers/input/posts.js @@ -102,6 +102,15 @@ const forceStatusFilter = (frame) => { } }; +const transformLegacyEmailRecipientFilters = (frame) => { + if (frame.options.email_recipient_filter === 'free') { + frame.options.email_recipient_filter = 'status:free'; + } + if (frame.options.email_recipient_filter === 'paid') { + frame.options.email_recipient_filter = 'status:-free'; + } +}; + module.exports = { browse(apiConfig, frame) { debug('browse'); @@ -196,6 +205,7 @@ module.exports = { }); } + transformLegacyEmailRecipientFilters(frame); handlePostsMeta(frame); defaultFormat(frame); defaultRelations(frame); @@ -205,6 +215,7 @@ module.exports = { debug('edit'); this.add(apiConfig, frame, {add: false}); + transformLegacyEmailRecipientFilters(frame); handlePostsMeta(frame); forceStatusFilter(frame); forcePageFilter(frame); diff --git a/core/server/api/v3/posts.js b/core/server/api/v3/posts.js index 498a05c2f5..e50a4a7fce 100644 --- a/core/server/api/v3/posts.js +++ b/core/server/api/v3/posts.js @@ -3,6 +3,7 @@ const i18n = require('../../../shared/i18n'); const errors = require('@tryghost/errors'); const urlUtils = require('../../../shared/url-utils'); const {mega} = require('../../services/mega'); +const {BadRequestError} = require('@tryghost/errors'); const allowedIncludes = ['tags', 'authors', 'authors.roles', 'email']; const unsafeAttrs = ['status', 'authors', 'visibility']; @@ -141,9 +142,6 @@ module.exports = { source: { values: ['html'] }, - email_recipient_filter: { - values: ['none', 'free', 'paid', 'all'] - }, send_email_when_published: { values: [true, false] } @@ -183,7 +181,20 @@ module.exports = { } /**Handle newsletter email */ - if (model.get('email_recipient_filter') !== 'none') { + const emailRecipientFilter = model.get('email_recipient_filter'); + if (emailRecipientFilter !== 'none') { + if (emailRecipientFilter !== 'all') { + // check filter is valid + try { + await models.Member.findPage({filter: `subscribed:true+${emailRecipientFilter}`, limit: 1}); + } catch (err) { + return Promise.reject(new BadRequestError({ + message: i18n.t('errors.api.posts.invalidEmailRecipientFilter'), + context: err.message + })); + } + } + const postPublished = model.wasChanged() && (model.get('status') === 'published') && (model.previous('status') !== 'published'); if (postPublished) { let postEmail = model.relations.email; diff --git a/core/server/api/v3/utils/serializers/input/posts.js b/core/server/api/v3/utils/serializers/input/posts.js index 2ef6a7b4a6..d1637a92a8 100644 --- a/core/server/api/v3/utils/serializers/input/posts.js +++ b/core/server/api/v3/utils/serializers/input/posts.js @@ -102,6 +102,15 @@ const forceStatusFilter = (frame) => { } }; +const transformLegacyEmailRecipientFilters = (frame) => { + if (frame.options.email_recipient_filter === 'free') { + frame.options.email_recipient_filter = 'status:free'; + } + if (frame.options.email_recipient_filter === 'paid') { + frame.options.email_recipient_filter = 'status:-free'; + } +}; + module.exports = { browse(apiConfig, frame) { debug('browse'); @@ -204,6 +213,7 @@ module.exports = { }); } + transformLegacyEmailRecipientFilters(frame); handlePostsMeta(frame); defaultFormat(frame); defaultRelations(frame); @@ -213,6 +223,7 @@ module.exports = { debug('edit'); this.add(apiConfig, frame, {add: false}); + transformLegacyEmailRecipientFilters(frame); handlePostsMeta(frame); forceStatusFilter(frame); forcePageFilter(frame); diff --git a/core/server/data/schema/schema.js b/core/server/data/schema/schema.js index 026961c7f7..d0a23df928 100644 --- a/core/server/data/schema/schema.js +++ b/core/server/data/schema/schema.js @@ -34,8 +34,7 @@ module.exports = { type: 'string', maxlength: 50, nullable: false, - defaultTo: 'none', - validations: {isIn: [['none', 'all', 'free', 'paid']]} + defaultTo: 'none' }, /** * @deprecated: single authors was superceded by multiple authors in Ghost 1.22.0 @@ -529,8 +528,7 @@ module.exports = { type: 'string', maxlength: 50, nullable: false, - defaultTo: 'paid', - validations: {isIn: [['all', 'free', 'paid']]} + defaultTo: 'status:-free' }, error: {type: 'string', maxlength: 2000, nullable: true}, error_data: {type: 'text', maxlength: 1000000000, fieldtype: 'long', nullable: true}, diff --git a/core/server/models/email.js b/core/server/models/email.js index 506842d6be..c990811b0d 100644 --- a/core/server/models/email.js +++ b/core/server/models/email.js @@ -8,7 +8,7 @@ const Email = ghostBookshelf.Model.extend({ return { uuid: uuid.v4(), status: 'pending', - recipient_filter: 'paid', + recipient_filter: 'status:-free', track_opens: false, delivered_count: 0, opened_count: 0, @@ -16,12 +16,40 @@ const Email = ghostBookshelf.Model.extend({ }; }, + parse() { + const attrs = ghostBookshelf.Model.prototype.parse.apply(this, arguments); + + // update legacy recipient_filter values to proper NQL + if (attrs.recipient_filter === 'free') { + attrs.recipient_filter = 'status:free'; + } + if (attrs.recipient_filter === 'paid') { + attrs.recipient_filter = 'status:-free'; + } + + return attrs; + }, + + formatOnWrite(attrs) { + // update legacy recipient_filter values to proper NQL + if (attrs.recipient_filter === 'free') { + attrs.recipient_filter = 'status:free'; + } + if (attrs.recipient_filter === 'paid') { + attrs.recipient_filter = 'status:-free'; + } + + return attrs; + }, + post() { return this.belongsTo('Post', 'post_id'); }, + emailBatches() { return this.hasMany('EmailBatch', 'email_id'); }, + recipients() { return this.hasMany('EmailRecipient', 'email_id'); }, diff --git a/core/server/models/post.js b/core/server/models/post.js index f59c7c721d..ecaa2eb83b 100644 --- a/core/server/models/post.js +++ b/core/server/models/post.js @@ -101,6 +101,14 @@ Post = ghostBookshelf.Model.extend({ } }); + // update legacy email_recipient_filter values to proper NQL + if (attrs.email_recipient_filter === 'free') { + attrs.email_recipient_filter = 'status:free'; + } + if (attrs.email_recipient_filter === 'paid') { + attrs.email_recipient_filter = 'status:-free'; + } + return attrs; }, @@ -139,6 +147,14 @@ Post = ghostBookshelf.Model.extend({ } }); + // update legacy email_recipient_filter values to proper NQL + if (attrs.email_recipient_filter === 'free') { + attrs.email_recipient_filter = 'status:free'; + } + if (attrs.email_recipient_filter === 'paid') { + attrs.email_recipient_filter = 'status:-free'; + } + return attrs; }, diff --git a/core/server/services/mega/mega.js b/core/server/services/mega/mega.js index 51a37c47b1..c36efc8004 100644 --- a/core/server/services/mega/mega.js +++ b/core/server/services/mega/mega.js @@ -117,19 +117,17 @@ const addEmail = async (postModel, options) => { const emailRecipientFilter = postModel.get('email_recipient_filter'); switch (emailRecipientFilter) { + // `paid` and `free` were swapped out for NQL filters in 4.5.0, we shouldn't see them here now case 'paid': - filterOptions.filter = 'subscribed:true+status:-free'; - break; case 'free': - filterOptions.filter = 'subscribed:true+status:free'; - break; + throw new Error(`Unexpected email_recipient_filter value "${emailRecipientFilter}", expected an NQL equivalent`); case 'all': filterOptions.filter = 'subscribed:true'; break; case 'none': throw new Error('Cannot sent email to "none" email_recipient_filter'); default: - throw new Error(`Unknown email_recipient_filter ${emailRecipientFilter}`); + filterOptions.filter = `subscribed:true+${emailRecipientFilter}`; } const startRetrieve = Date.now(); @@ -314,24 +312,22 @@ async function sendEmailJob({emailModel, options}) { // instantiations and associated processing and event loop blocking async function getEmailMemberRows({emailModel, options}) { const knexOptions = _.pick(options, ['transacting', 'forUpdate']); - - // TODO: this will clobber a user-assigned filter if/when we allow emails to be sent to filtered member lists const filterOptions = Object.assign({}, knexOptions); const recipientFilter = emailModel.get('recipient_filter'); switch (recipientFilter) { + // `paid` and `free` were swapped out for NQL filters in 4.5.0, we shouldn't see them here now case 'paid': - filterOptions.filter = 'subscribed:true+status:-free'; - break; case 'free': - filterOptions.filter = 'subscribed:true+status:free'; - break; + throw new Error(`Unexpected recipient_filter value "${recipientFilter}", expected an NQL equivalent`); case 'all': filterOptions.filter = 'subscribed:true'; break; + case 'none': + throw new Error('Cannot sent email to "none" recipient_filter'); default: - throw new Error(`Unknown recipient_filter ${recipientFilter}`); + filterOptions.filter = `subscribed:true+${recipientFilter}`; } const startRetrieve = Date.now(); diff --git a/core/shared/i18n/translations/en.json b/core/shared/i18n/translations/en.json index 8b95554146..3db36ea6bf 100644 --- a/core/shared/i18n/translations/en.json +++ b/core/shared/i18n/translations/en.json @@ -355,7 +355,8 @@ "notificationDoesNotExist": "Notification does not exist." }, "posts": { - "postNotFound": "Post not found." + "postNotFound": "Post not found.", + "invalidEmailRecipientFilter": "Invalid filter in email_recipient_filter param." }, "authors": { "notFound": "Author not found." diff --git a/test/regression/api/canary/admin/posts_spec.js b/test/regression/api/canary/admin/posts_spec.js index 460127005a..9347c22ce8 100644 --- a/test/regression/api/canary/admin/posts_spec.js +++ b/test/regression/api/canary/admin/posts_spec.js @@ -604,6 +604,40 @@ describe('Posts API (canary)', function () { should.equal(res.body.posts[0].plaintext, undefined); }); }); + + it('errors with invalid email recipient filter', function () { + return request + .post(localUtils.API.getApiQuery('posts/')) + .set('Origin', config.get('url')) + .send({ + posts: [{ + title: 'Ready to be emailed', + status: 'draft' + }] + }) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(201) + .then((res) => { + return request + .put(`${localUtils.API.getApiQuery(`posts/${res.body.posts[0].id}/`)}?email_recipient_filter=not a filter`) + .set('Origin', config.get('url')) + .send({ + posts: [{ + title: res.body.posts[0].title, + mobilecdoc: res.body.posts[0].mobilecdoc, + updated_at: res.body.posts[0].updated_at, + status: 'published' + }] + }) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(400); + }) + .then((res) => { + res.text.should.match(/invalid filter/i); + }); + }); }); describe('Destroy', function () { diff --git a/test/regression/api/v3/admin/posts_spec.js b/test/regression/api/v3/admin/posts_spec.js index 9719599164..144232ff7a 100644 --- a/test/regression/api/v3/admin/posts_spec.js +++ b/test/regression/api/v3/admin/posts_spec.js @@ -604,6 +604,40 @@ describe('Posts API (v3)', function () { should.equal(res.body.posts[0].plaintext, undefined); }); }); + + it('errors with invalid email recipient filter', function () { + return request + .post(localUtils.API.getApiQuery('posts/')) + .set('Origin', config.get('url')) + .send({ + posts: [{ + title: 'Ready to be emailed', + status: 'draft' + }] + }) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(201) + .then((res) => { + return request + .put(`${localUtils.API.getApiQuery(`posts/${res.body.posts[0].id}/`)}?email_recipient_filter=not a filter`) + .set('Origin', config.get('url')) + .send({ + posts: [{ + title: res.body.posts[0].title, + mobilecdoc: res.body.posts[0].mobilecdoc, + updated_at: res.body.posts[0].updated_at, + status: 'published' + }] + }) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(400); + }) + .then((res) => { + res.text.should.match(/invalid filter/i); + }); + }); }); describe('Destroy', function () { diff --git a/test/regression/models/model_posts_spec.js b/test/regression/models/model_posts_spec.js index 608afd1d59..981905a56b 100644 --- a/test/regression/models/model_posts_spec.js +++ b/test/regression/models/model_posts_spec.js @@ -60,6 +60,29 @@ describe('Post Model', function () { }); }); + describe('findOne', function () { + it('transforms legacy email_recipient_filter values on read', function (done) { + const postId = testUtils.DataGenerator.Content.posts[0].id; + + db.knex('posts').where({id: postId}).update({ + email_recipient_filter: 'paid' + }).then(() => { + return db.knex('posts').where({id: postId}); + }).then((knexResult) => { + const [knexPost] = knexResult; + knexPost.email_recipient_filter.should.equal('paid'); + + return models.Post.findOne({id: postId}); + }).then((result) => { + should.exist(result); + const post = result.toJSON(); + post.email_recipient_filter.should.equal('status:-free'); + + done(); + }).catch(done); + }); + }); + describe('findPage', function () { // @TODO: this test case fails for mysql currently if you run all regression tests, the test does not fail if you run this as a single test describe.skip('with more posts/tags', function () { @@ -692,6 +715,24 @@ describe('Post Model', function () { done(); }).catch(done); }); + + it('transforms legacy email_recipient_filter values on save', function (done) { + const postId = testUtils.DataGenerator.Content.posts[3].id; + + models.Post.findOne({id: postId}).then(() => { + return models.Post.edit({ + email_recipient_filter: 'free' + }, _.extend({}, context, {id: postId})); + }).then((edited) => { + edited.attributes.email_recipient_filter.should.equal('status:free'); + return db.knex('posts').where({id: edited.id}); + }).then((knexResult) => { + const [knexPost] = knexResult; + knexPost.email_recipient_filter.should.equal('status:free'); + + done(); + }).catch(done); + }); }); describe('add', function () { diff --git a/test/unit/api/canary/utils/serializers/input/posts_spec.js b/test/unit/api/canary/utils/serializers/input/posts_spec.js index 985c651ef2..13280023e4 100644 --- a/test/unit/api/canary/utils/serializers/input/posts_spec.js +++ b/test/unit/api/canary/utils/serializers/input/posts_spec.js @@ -361,5 +361,71 @@ describe('Unit: canary/utils/serializers/input/posts', function () { frame.data.posts[0].tags.should.eql([{name: 'name1'}, {name: 'name2'}]); }); }); + + describe('transforms legacy email recipient filter values', function () { + it('free becomes status:free', function () { + const frame = { + options: { + email_recipient_filter: 'free' + }, + data: { + posts: [{id: '1'}] + } + }; + + serializers.input.posts.edit({}, frame); + + frame.options.email_recipient_filter.should.eql('status:free'); + }); + + it('paid becomes status:-free', function () { + const frame = { + options: { + email_recipient_filter: 'paid' + }, + data: { + posts: [{id: '1'}] + } + }; + + serializers.input.posts.edit({}, frame); + + frame.options.email_recipient_filter.should.eql('status:-free'); + }); + }); + }); + + describe('add', function () { + describe('transforms legacy email recipient filter values', function () { + it('free becomes status:free', function () { + const frame = { + options: { + email_recipient_filter: 'free' + }, + data: { + posts: [{id: '1'}] + } + }; + + serializers.input.posts.add({}, frame); + + frame.options.email_recipient_filter.should.eql('status:free'); + }); + + it('paid becomes status:-free', function () { + const frame = { + options: { + email_recipient_filter: 'paid' + }, + data: { + posts: [{id: '1'}] + } + }; + + serializers.input.posts.add({}, frame); + + frame.options.email_recipient_filter.should.eql('status:-free'); + }); + }); }); }); diff --git a/test/unit/api/v3/utils/serializers/input/posts_spec.js b/test/unit/api/v3/utils/serializers/input/posts_spec.js index 6729018bba..09cdc6b171 100644 --- a/test/unit/api/v3/utils/serializers/input/posts_spec.js +++ b/test/unit/api/v3/utils/serializers/input/posts_spec.js @@ -361,5 +361,71 @@ describe('Unit: v3/utils/serializers/input/posts', function () { frame.data.posts[0].tags.should.eql([{name: 'name1'}, {name: 'name2'}]); }); }); + + describe('transforms legacy email recipient filter values', function () { + it('free becomes status:free', function () { + const frame = { + options: { + email_recipient_filter: 'free' + }, + data: { + posts: [{id: '1'}] + } + }; + + serializers.input.posts.edit({}, frame); + + frame.options.email_recipient_filter.should.eql('status:free'); + }); + + it('paid becomes status:-free', function () { + const frame = { + options: { + email_recipient_filter: 'paid' + }, + data: { + posts: [{id: '1'}] + } + }; + + serializers.input.posts.edit({}, frame); + + frame.options.email_recipient_filter.should.eql('status:-free'); + }); + }); + }); + + describe('add', function () { + describe('transforms legacy email recipient filter values', function () { + it('free becomes status:free', function () { + const frame = { + options: { + email_recipient_filter: 'free' + }, + data: { + posts: [{id: '1'}] + } + }; + + serializers.input.posts.add({}, frame); + + frame.options.email_recipient_filter.should.eql('status:free'); + }); + + it('paid becomes status:-free', function () { + const frame = { + options: { + email_recipient_filter: 'paid' + }, + data: { + posts: [{id: '1'}] + } + }; + + serializers.input.posts.add({}, frame); + + frame.options.email_recipient_filter.should.eql('status:-free'); + }); + }); }); }); diff --git a/test/unit/data/schema/integrity_spec.js b/test/unit/data/schema/integrity_spec.js index f70b923e22..06cb6b3997 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 = 'b7bca80554f3946cd2f83e0e99ff3532'; + const currentSchemaHash = 'bcb57235883ddb9765f9abf8ab878cd7'; const currentFixturesHash = '8671672598d2a62e53418c4b91aa79a3'; const currentSettingsHash = 'c202cf5780aa77d8730a82680e2b142e'; const currentRoutesHash = '3d180d52c663d173a6be791ef411ed01';