TryGhost/Ghost
1
0
Fork 0
mirror of https://github.com/TryGhost/Ghost.git synced 2024-11-23 11:55:01 +03:00
Code Issues Projects Releases Wiki Activity

Filtered member email recipients based on the newsletter subscriptions (#14489)

Browse Source 
refs https://github.com/TryGhost/Team/issues/1524

- We need to fetch the post newsletter to grab the slug as it's needed for the member NQL filter.
- We can then use the newsletter slug and append it in the existing member NQL filter.
- Removed `subscribed:true` when an email is sent to a newsletter and replaced it with the newsletter id
- Added `status:-free` when an email is sent to a newsletter with `visibility` set to `paid`
- Added tests what happens when you publish without newsletter_id
- Added tests what happens when you publish with newsletter_id

Co-authored-by: Simon Backx <simon@ghost.org>
This commit is contained in:
Thibaut Patel 2022-04-25 11:25:49 +02:00 committed by Matt Hanley
parent 9da57fa6bb
commit ed29c7addf
7 changed files with 268 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
core/server/models/email.js
View File

@ -54,6 +54,10 @@ const Email = ghostBookshelf.Model.extend({
return this.hasMany('EmailRecipient', 'email_id');
},
newsletter() {
return this.belongsTo('Newsletter', 'newsletter_id');
},
emitChange: function emitChange(event, options) {
const eventToTrigger = 'email' + '.' + event;
ghostBookshelf.Model.prototype.emitChange.bind(this)(this, eventToTrigger, options);

4
core/server/models/post.js
View File

@ -828,6 +828,10 @@ Post = ghostBookshelf.Model.extend({
return this.hasOne('Email', 'post_id');
},
newsletter: function newsletter() {
return this.belongsTo('Newsletter', 'newsletter_id');
},
/**
* @NOTE:
* If you are requesting models with `columns`, you try to only receive some fields of the model/s.

53
core/server/services/mega/mega.js
View File

@ -17,6 +17,7 @@ const models = require('../../models');
const postEmailSerializer = require('./post-email-serializer');
const labs = require('../../../shared/labs');
const {getSegmentsFromHtml} = require('./segment-parser');
const labsService = require('../../../shared/labs');
// Used to listen to email.added and email.edited model events originally, I think to offload this - ideally would just use jobs now if possible
const events = require('../../lib/common/events');
@ -26,7 +27,8 @@ const messages = {
unexpectedFilterError: 'Unexpected {property} value "{value}", expected an NQL equivalent',
noneFilterError: 'Cannot send email to "none" {property}',
emailSendingDisabled: `Email sending is temporarily disabled because your account is currently in review. You should have an email about this from us already, but you can also reach us any time at support@ghost.org`,
sendEmailRequestFailed: 'The email service was unable to send an email batch.'
sendEmailRequestFailed: 'The email service was unable to send an email batch.',
newsletterVisibilityError: 'Unexpected visibility value "{value}". Use one of the valid: "members", "paid".'
};
const getFromAddress = () => {
@ -127,7 +129,15 @@ const sendTestEmail = async (postModel, toEmails, apiVersion, memberSegment) =>
* @param {string} emailRecipientFilter NQL filter for members
* @param {object} options
*/
const transformEmailRecipientFilter = (emailRecipientFilter, {errorProperty = 'email_recipient_filter'} = {}) => {
const transformEmailRecipientFilter = (emailRecipientFilter, {errorProperty = 'email_recipient_filter'} = {}, newsletter = null) => {
let filter = [];
if (!newsletter) {
filter.push(`subscribed:true`);
} else {
filter.push(`newsletters.id:${newsletter.id}`);
}
switch (emailRecipientFilter) {
// `paid` and `free` were swapped out for NQL filters in 4.5.0, we shouldn't see them here now
case 'paid':
@ -139,7 +149,7 @@ const transformEmailRecipientFilter = (emailRecipientFilter, {errorProperty = 'e
})
});
case 'all':
return 'subscribed:true';
break;
case 'none':
throw new errors.InternalServerError({
message: tpl(messages.noneFilterError, {
@ -147,8 +157,29 @@ const transformEmailRecipientFilter = (emailRecipientFilter, {errorProperty = 'e
})
});
default:
return `subscribed:true+(${emailRecipientFilter})`;
filter.push(`(${emailRecipientFilter})`);
break;
}
if (newsletter) {
const visibility = newsletter.get('visibility');
switch (visibility) {
case 'members':
// No need to add a member status filter as the email is available to all members
break;
case 'paid':
filter.push(`status:-free`);
break;
default:
throw new errors.InternalServerError({
message: tpl(messages.newsletterVisibilityError, {
value: visibility
})
});
}
}
return filter.join('+');
};
/**
@ -177,12 +208,16 @@ const addEmail = async (postModel, options) => {
const knexOptions = _.pick(options, ['transacting', 'forUpdate']);
const filterOptions = Object.assign({}, knexOptions, {limit: 1});
let newsletter;
if (labsService.isSet('multipleNewsletters')) {
newsletter = await postModel.related('newsletter').fetch(Object.assign({}, {require: false}, _.pick(options, ['transacting'])));
}
const emailRecipientFilter = postModel.get('email_recipient_filter');
filterOptions.filter = transformEmailRecipientFilter(emailRecipientFilter, {errorProperty: 'email_recipient_filter'});
filterOptions.filter = transformEmailRecipientFilter(emailRecipientFilter, {errorProperty: 'email_recipient_filter'}, newsletter);
const startRetrieve = Date.now();
debug('addEmail: retrieving members count');
const {meta: {pagination: {total: membersCount}}} = await membersService.api.members.list(Object.assign({}, knexOptions, filterOptions));
const {meta: {pagination: {total: membersCount}}} = await membersService.api.members.list({...knexOptions, ...filterOptions});
debug(`addEmail: retrieved members count - ${membersCount} members (${Date.now() - startRetrieve}ms)`);
// NOTE: don't create email object when there's nobody to send the email to
@ -385,7 +420,11 @@ async function getEmailMemberRows({emailModel, memberSegment, options}) {
const knexOptions = _.pick(options, ['transacting', 'forUpdate']);
const filterOptions = Object.assign({}, knexOptions);
const recipientFilter = transformEmailRecipientFilter(emailModel.get('recipient_filter'), {errorProperty: 'recipient_filter'});
let newsletter = null;
if (labsService.isSet('multipleNewsletters')) {
newsletter = await emailModel.related('newsletter').fetch(Object.assign({}, {require: false}, _.pick(options, ['transacting'])));
}
const recipientFilter = transformEmailRecipientFilter(emailModel.get('recipient_filter'), {errorProperty: 'recipient_filter'}, newsletter);
filterOptions.filter = recipientFilter;
if (memberSegment) {

4
core/server/services/posts/posts-service.js
View File

@ -22,7 +22,7 @@ class PostsService {
// Make sure the newsletter_id is matching an active newsletter
if (frame.options.newsletter_id) {
const newsletter = await this.models.Newsletter.findOne({id: frame.options.newsletter_id, status: 'active'}, {transacting: frame.options.transacting});
const newsletter = await this.models.Newsletter.findOne({id: frame.options.newsletter_id, filter: 'status:active'}, {transacting: frame.options.transacting});
if (!newsletter) {
throw new BadRequestError({
message: messages.invalidNewsletterId
@ -30,7 +30,7 @@ class PostsService {
}
} else {
// Set the newsletter_id if it isn't passed to the API
const newsletters = await this.models.Newsletter.findPage({status: 'active', limit: 1, columns: ['id']}, {transacting: frame.options.transacting});
const newsletters = await this.models.Newsletter.findPage({filter: 'status:active', limit: 1, columns: ['id']}, {transacting: frame.options.transacting});
if (newsletters.data.length > 0) {
frame.options.newsletter_id = newsletters.data[0].id;
}

24
test/e2e-api/admin/posts.test.js
View File

@ -601,7 +601,10 @@ describe('Posts API', function () {
id: id,
status: 'draft'
}, testUtils.context.internal);
should(model.get('newsletter_id')).eql(newsletterId);
// The newsletter id is back to null here, because no email was sent...
// This is expected behaviour
should(model.get('newsletter_id')).eql(null);
const republished = {
status: 'published',
@ -620,7 +623,24 @@ describe('Posts API', function () {
id: id,
status: 'published'
}, testUtils.context.internal);
should(model.get('newsletter_id')).eql(newsletterId);
should(model.get('newsletter_id')).eql(newsletterId2);
// Should not change if status remains published
await request
.put(localUtils.API.getApiQuery('posts/' + id + '/?email_recipient_filter=all&newsletter_id=' + newsletterId))
.set('Origin', config.get('url'))
.send({posts: [republished]})
.expect('Content-Type', /json/)
.expect('Cache-Control', testUtils.cacheRules.private)
.expect(200);
model = await models.Post.findOne({
id: id,
status: 'published'
}, testUtils.context.internal);
// Test if the newsletter_id option was ignored
should(model.get('newsletter_id')).eql(newsletterId2);
});
it('Can destroy a post', async function () {

148
test/regression/api/admin/posts.test.js
View File

@ -8,13 +8,21 @@ const config = require('../../../../core/shared/config');
const models = require('../../../../core/server/models');
const localUtils = require('./utils');
const default_newsletter_id = testUtils.DataGenerator.Content.newsletters[0].id;
const second_newsletter_id = testUtils.DataGenerator.Content.newsletters[1].id;
describe('Posts API (canary)', function () {
let request;
before(async function () {
await localUtils.startGhost();
request = supertest.agent(config.get('url'));
await localUtils.doAuth(request, 'users:extra', 'posts', 'emails', 'members');
// Archive the default newsletter fixture
const defaultNewsletter = await models.Newsletter.findOne({status: 'active'});
await models.Newsletter.edit({status: 'archived'}, {id: defaultNewsletter.id});
await localUtils.doAuth(request, 'users:extra', 'posts', 'emails', 'newsletters', 'members:newsletters');
});
describe('Browse', function () {
@ -563,7 +571,7 @@ describe('Posts API (canary)', function () {
});
});
it('publishes a post with email_only and sends email', async function () {
it('publishes a post with email_only and sends email to all without specifying the default newsletter_id', async function () {
const res = await request
.post(localUtils.API.getApiQuery('posts/'))
.set('Origin', config.get('url'))
@ -604,10 +612,10 @@ describe('Posts API (canary)', function () {
publishedRes.body.posts[0].status.should.equal('sent');
should.exist(publishedRes.body.posts[0].email);
publishedRes.body.posts[0].email.email_count.should.equal(6);
publishedRes.body.posts[0].email.email_count.should.equal(4);
});
it('publishes a post while setting email_only flag sends an email', async function () {
it('publishes a post while setting email_only flag sends an email to paid without specifying the default newsletter_id', async function () {
const res = await request
.post(localUtils.API.getApiQuery('posts/'))
.set('Origin', config.get('url'))
@ -647,7 +655,137 @@ describe('Posts API (canary)', function () {
publishedRes.body.posts[0].status.should.equal('sent');
should.exist(publishedRes.body.posts[0].email);
publishedRes.body.posts[0].email.email_count.should.equal(3);
publishedRes.body.posts[0].email.email_count.should.equal(2);
});
it('publishes a post with email_only and sends email to all', async function () {
const res = await request
.post(localUtils.API.getApiQuery('posts/'))
.set('Origin', config.get('url'))
.send({
posts: [{
title: 'Email me',
email_only: true
}]
})
.expect('Content-Type', /json/)
.expect('Cache-Control', testUtils.cacheRules.private)
.expect(201);
should.exist(res.body.posts);
should.exist(res.body.posts[0].title);
res.body.posts[0].title.should.equal('Email me');
res.body.posts[0].email_only.should.be.true();
res.body.posts[0].status.should.equal('draft');
should.exist(res.headers.location);
res.headers.location.should.equal(`http://127.0.0.1:2369${localUtils.API.getApiQuery('posts/')}${res.body.posts[0].id}/`);
const publishedRes = await request
.put(localUtils.API.getApiQuery(`posts/${res.body.posts[0].id}/?email_recipient_filter=all&newsletter_id=${default_newsletter_id}`))
.set('Origin', config.get('url'))
.send({
posts: [{
status: 'published',
updated_at: res.body.posts[0].updated_at
}]
})
.expect('Content-Type', /json/)
.expect('Cache-Control', testUtils.cacheRules.private)
.expect(200);
should.exist(publishedRes.body.posts);
res.body.posts[0].email_only.should.be.true();
publishedRes.body.posts[0].status.should.equal('sent');
should.exist(publishedRes.body.posts[0].email);
publishedRes.body.posts[0].email.email_count.should.equal(4);
});
it('publishes a post while setting email_only flag sends an email to paid', async function () {
const res = await request
.post(localUtils.API.getApiQuery('posts/'))
.set('Origin', config.get('url'))
.send({
posts: [{
title: 'Email me'
}]
})
.expect('Content-Type', /json/)
.expect('Cache-Control', testUtils.cacheRules.private)
.expect(201);
should.exist(res.body.posts);
should.exist(res.body.posts[0].title);
res.body.posts[0].title.should.equal('Email me');
res.body.posts[0].email_only.should.be.false();
res.body.posts[0].status.should.equal('draft');
should.exist(res.headers.location);
res.headers.location.should.equal(`http://127.0.0.1:2369${localUtils.API.getApiQuery('posts/')}${res.body.posts[0].id}/`);
const publishedRes = await request
.put(localUtils.API.getApiQuery(`posts/${res.body.posts[0].id}/?email_recipient_filter=paid&newsletter_id=${default_newsletter_id}`))
.set('Origin', config.get('url'))
.send({
posts: [{
status: 'published',
email_only: true,
updated_at: res.body.posts[0].updated_at
}]
})
.expect('Content-Type', /json/)
.expect('Cache-Control', testUtils.cacheRules.private)
.expect(200);
should.exist(publishedRes.body.posts);
publishedRes.body.posts[0].status.should.equal('sent');
should.exist(publishedRes.body.posts[0].email);
publishedRes.body.posts[0].email.email_count.should.equal(2);
});
it('only send an email to paid subscribed members of the selected newsletter', async function () {
const res = await request
.post(localUtils.API.getApiQuery('posts/'))
.set('Origin', config.get('url'))
.send({
posts: [{
title: 'Email me'
}]
})
.expect('Content-Type', /json/)
.expect('Cache-Control', testUtils.cacheRules.private)
.expect(201);
should.exist(res.body.posts);
should.exist(res.body.posts[0].title);
res.body.posts[0].title.should.equal('Email me');
res.body.posts[0].email_only.should.be.false();
res.body.posts[0].status.should.equal('draft');
should.exist(res.headers.location);
res.headers.location.should.equal(`http://127.0.0.1:2369${localUtils.API.getApiQuery('posts/')}${res.body.posts[0].id}/`);
const publishedRes = await request
.put(localUtils.API.getApiQuery(`posts/${res.body.posts[0].id}/?email_recipient_filter=paid&newsletter_id=${second_newsletter_id}`))
.set('Origin', config.get('url'))
.send({
posts: [{
status: 'published',
email_only: true,
updated_at: res.body.posts[0].updated_at
}]
})
.expect('Content-Type', /json/)
.expect('Cache-Control', testUtils.cacheRules.private)
.expect(200);
should.exist(publishedRes.body.posts);
publishedRes.body.posts[0].status.should.equal('sent');
should.exist(publishedRes.body.posts[0].email);
publishedRes.body.posts[0].email.email_count.should.equal(2);
});
it('read-only value do not cause errors when edited', function () {

50
test/unit/server/services/mega/mega.test.js
View File

@ -1,13 +1,17 @@
const should = require('should');
const sinon = require('sinon');
const errors = require('@tryghost/errors');
const labs = require('../../../../../core/shared/labs');
const {addEmail, _partitionMembersBySegment, _getEmailMemberRows, _transformEmailRecipientFilter, handleUnsubscribeRequest} = require('../../../../../core/server/services/mega/mega');
const membersService = require('../../../../../core/server/services/members');
const labs = require('../../../../../core/shared/labs');
describe('MEGA', function () {
describe('addEmail', function () {
afterEach(function () {
sinon.restore();
});
// via transformEmailRecipientFilter
it('throws when "free" or "paid" strings are used as a email_recipient_filter', async function () {
const postModel = {
@ -37,6 +41,26 @@ describe('MEGA', function () {
err.message.should.equal('Cannot send email to "none" email_recipient_filter');
}
});
// via transformEmailRecipientFilter
it('throws when "public" is used as newsletter.visibility', async function () {
const postModel = {
get: sinon.stub().returns('status:free'),
fetch: sinon.stub().returns(Promise.resolve({
get: () => 'public'
}))
};
postModel.related = sinon.stub().returns(postModel);
sinon.stub(labs, 'isSet').returns(true);
try {
await addEmail(postModel);
should.fail('addEmail did not throw');
} catch (err) {
should.equal(errors.utils.isGhostError(err), true);
err.message.should.equal('Unexpected visibility value "public". Use one of the valid: "members", "paid".');
}
});
});
describe('transformEmailRecipientFilter', function () {
@ -44,6 +68,16 @@ describe('MEGA', function () {
const transformedFilter = _transformEmailRecipientFilter('status:free,status:-free');
transformedFilter.should.equal('subscribed:true+(status:free,status:-free)');
});
it('doesn\'t enforce subscribed:true when sending an email to a newsletter', function () {
const transformedFilter = _transformEmailRecipientFilter('status:free,status:-free', {}, {id: 'test', get: () => 'members'});
transformedFilter.should.equal('newsletters.id:test+(status:free,status:-free)');
});
it('combines successfully with the newsletter paid-only visibility', function () {
const transformedFilter = _transformEmailRecipientFilter('status:free,status:-free', {}, {id: 'test', get: () => 'paid'});
transformedFilter.should.equal('newsletters.id:test+(status:free,status:-free)+status:-free');
});
});
describe('handleUnsubscribeRequest', function () {
@ -87,7 +121,12 @@ describe('MEGA', function () {
describe('getEmailMemberRows', function () {
it('addEmail throws when "free" or "paid" strings are used as a recipient_filter', async function () {
const emailModel = {
get: sinon.stub().returns('paid')
get: sinon.stub().returns('paid'),
related: sinon.stub().returns({
fetch: sinon.stub().returns({
id: 'test'
})
})
};
try {
@ -101,7 +140,12 @@ describe('MEGA', function () {
it('addEmail throws when "none" is used as a recipient_filter', async function () {
const emailModel = {
get: sinon.stub().returns('none')
get: sinon.stub().returns('none'),
related: sinon.stub().returns({
fetch: sinon.stub().returns({
id: 'test'
})
})
};
try {