mirror of
https://github.com/TryGhost/Ghost.git
synced 2025-01-06 22:40:14 -05:00
Fixed wrong newsletter used when sending scheduled post (#14734)
refs https://ghost.slack.com/archives/C02G9E68C/p1651939076681719 Cause: - When a scheduled post was published via the post scheduler, no `newsletter_id` option is passed when editing the post. - When editing a post via the posts service, without the `newsletter_id` option, the `newsletter_id` option is automatically set to the default newsletter's id. - Inside the post model, this new `newsletter_id` was not saved, because it was already set, and changing it is prevented. - The `mega` service wasn't using the (unchanged) post's newsletter_id, but used the option instead, which contained the default newsletter's id. Fix: - Always using the newsletter_id from the post and requiring the newsletter associated with a post to exist. - This behaviour can be/is tested by publishing a scheduled post without any option. Also cleaned up some `Object.assign` usages.
This commit is contained in:
Simon Backx
GitHub
parent
4d6b3568c5
commit
5657019e47
3 changed files with 135 additions and 19 deletions
|
|
@ -211,12 +211,10 @@ const addEmail = async (postModel, options) => {
|
|||
}
|
||||
|
||||
const knexOptions = _.pick(options, ['transacting', 'forUpdate']);
|
||||
const filterOptions = Object.assign({}, knexOptions, {limit: 1});
|
||||
const filterOptions = {...knexOptions, limit: 1};
|
||||
|
||||
const newsletter = await postModel.related('newsletter').fetch({require: true, ..._.pick(options, ['transacting'])});
|
||||
|
||||
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'}, newsletter);
|
||||
|
||||
|
|
@ -254,7 +252,7 @@ const addEmail = async (postModel, options) => {
|
|||
submitted_at: moment().toDate(),
|
||||
track_opens: !!settingsCache.get('email_track_opens'),
|
||||
recipient_filter: emailRecipientFilter,
|
||||
newsletter_id: options.newsletter_id
|
||||
newsletter_id: newsletter.id
|
||||
}, knexOptions);
|
||||
} else {
|
||||
return existing;
|
||||
|
|
|
|||
|
|
@ -29,6 +29,8 @@ class PostsService {
|
|||
}
|
||||
} else {
|
||||
// Set the newsletter_id if it isn't passed to the API
|
||||
// NOTE: this option is ignored if the newsletter_id is already set on the post.
|
||||
// Never use frame.options.newsletter_id to do actual logic. Use model.newsletter_id after the edit.
|
||||
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;
|
||||
|
|
@ -84,7 +86,7 @@ class PostsService {
|
|||
let postEmail = model.relations.email;
|
||||
|
||||
if (!postEmail) {
|
||||
const email = await this.mega.addEmail(model, Object.assign({}, frame.options));
|
||||
const email = await this.mega.addEmail(model, frame.options);
|
||||
model.set('email', email);
|
||||
} else if (postEmail && postEmail.get('status') === 'failed') {
|
||||
const email = await this.mega.retryFailedEmail(postEmail);
|
||||
|
|
|
|||
|
|
@ -21,7 +21,7 @@ describe('Posts API', function () {
|
|||
* Members are needed to enable mega to create an email record so that we can test that newsletter_id
|
||||
* can't be overwritten after an email record is created.
|
||||
*/
|
||||
await localUtils.doAuth(request, 'users:extra', 'posts', 'emails', 'newsletters', 'members');
|
||||
await localUtils.doAuth(request, 'users:extra', 'posts', 'emails', 'newsletters', 'members:newsletters');
|
||||
|
||||
// Assign a newsletter to one of the posts
|
||||
const newsletterId = testUtils.DataGenerator.Content.newsletters[0].id;
|
||||
|
|
@ -589,6 +589,8 @@ describe('Posts API', function () {
|
|||
});
|
||||
|
||||
it('Can change the newsletter_id of a post when publishing', async function () {
|
||||
const newsletterId = testUtils.DataGenerator.Content.newsletters[2].id;
|
||||
|
||||
const post = {
|
||||
title: 'My newsletter_id post',
|
||||
status: 'draft',
|
||||
|
|
@ -618,8 +620,6 @@ describe('Posts API', function () {
|
|||
|
||||
updatedPost.status = 'published';
|
||||
|
||||
const newsletterId = testUtils.DataGenerator.Content.newsletters[0].id;
|
||||
|
||||
const finalPost = await request
|
||||
.put(localUtils.API.getApiQuery('posts/' + id + '/?email_recipient_filter=all&newsletter_id=' + newsletterId))
|
||||
.set('Origin', config.get('url'))
|
||||
|
|
@ -637,6 +637,104 @@ describe('Posts API', function () {
|
|||
}, testUtils.context.internal);
|
||||
|
||||
should(model.get('newsletter_id')).eql(newsletterId);
|
||||
|
||||
// Check email
|
||||
// Note: we only create an email if we have members susbcribed to the newsletter
|
||||
const email = await models.Email.findOne({
|
||||
post_id: id
|
||||
}, testUtils.context.internal);
|
||||
|
||||
should.exist(email);
|
||||
should(email.get('newsletter_id')).eql(newsletterId);
|
||||
should(email.get('status')).eql('pending');
|
||||
});
|
||||
|
||||
it('Can publish a scheduled post', async function () {
|
||||
const newsletterId = testUtils.DataGenerator.Content.newsletters[2].id;
|
||||
|
||||
const post = {
|
||||
title: 'My scheduled post',
|
||||
status: 'draft',
|
||||
feature_image_alt: 'Testing newsletter_id in scheduled posts',
|
||||
feature_image_caption: 'Testing <b>feature image caption</b>',
|
||||
mobiledoc: testUtils.DataGenerator.markdownToMobiledoc('my post'),
|
||||
created_at: moment().subtract(2, 'days').toDate(),
|
||||
updated_at: moment().subtract(2, 'days').toDate(),
|
||||
created_by: ObjectId().toHexString(),
|
||||
updated_by: ObjectId().toHexString()
|
||||
};
|
||||
|
||||
const res = await request.post(localUtils.API.getApiQuery('posts'))
|
||||
.set('Origin', config.get('url'))
|
||||
.send({posts: [post]})
|
||||
.expect('Content-Type', /json/)
|
||||
.expect('Cache-Control', testUtils.cacheRules.private)
|
||||
.expect(201);
|
||||
|
||||
const id = res.body.posts[0].id;
|
||||
|
||||
const updatedPost = res.body.posts[0];
|
||||
|
||||
updatedPost.status = 'scheduled';
|
||||
updatedPost.published_at = moment().add(2, 'days').toDate();
|
||||
|
||||
const scheduledRes = await request
|
||||
.put(localUtils.API.getApiQuery('posts/' + id + '/?email_recipient_filter=all&newsletter_id=' + newsletterId))
|
||||
.set('Origin', config.get('url'))
|
||||
.send({posts: [updatedPost]})
|
||||
.expect('Content-Type', /json/)
|
||||
.expect('Cache-Control', testUtils.cacheRules.private)
|
||||
.expect(200);
|
||||
|
||||
const scheduledPost = scheduledRes.body.posts[0];
|
||||
|
||||
scheduledPost.newsletter.id.should.eql(newsletterId);
|
||||
should.not.exist(scheduledPost.newsletter_id);
|
||||
|
||||
let model = await models.Post.findOne({
|
||||
id,
|
||||
status: 'scheduled'
|
||||
}, testUtils.context.internal);
|
||||
|
||||
should(model.get('newsletter_id')).eql(newsletterId);
|
||||
|
||||
// We should not have an email
|
||||
let email = await models.Email.findOne({
|
||||
post_id: id
|
||||
}, testUtils.context.internal);
|
||||
|
||||
should.not.exist(email);
|
||||
|
||||
// Publish now, without passing the newsletter_id or other options again!
|
||||
scheduledPost.status = 'published';
|
||||
scheduledPost.published_at = moment().toDate();
|
||||
|
||||
const publishedRes = await request
|
||||
.put(localUtils.API.getApiQuery('posts/' + id + '/'))
|
||||
.set('Origin', config.get('url'))
|
||||
.send({posts: [scheduledPost]})
|
||||
.expect('Content-Type', /json/)
|
||||
.expect('Cache-Control', testUtils.cacheRules.private)
|
||||
.expect(200);
|
||||
|
||||
const publishedPost = publishedRes.body.posts[0];
|
||||
|
||||
model = await models.Post.findOne({
|
||||
id
|
||||
}, testUtils.context.internal);
|
||||
|
||||
should(model.get('newsletter_id')).eql(newsletterId);
|
||||
|
||||
publishedPost.newsletter.id.should.eql(newsletterId);
|
||||
should.not.exist(publishedPost.newsletter_id);
|
||||
|
||||
// Check email is sent to the correct newsletter
|
||||
email = await models.Email.findOne({
|
||||
post_id: id
|
||||
}, testUtils.context.internal);
|
||||
|
||||
should(email.get('newsletter_id')).eql(newsletterId);
|
||||
should(email.get('status')).eql('pending');
|
||||
});
|
||||
|
||||
it('Defaults to the default newsletter when publishing without a newsletter_id', async function () {
|
||||
|
|
@ -688,6 +786,9 @@ describe('Posts API', function () {
|
|||
});
|
||||
|
||||
it('Can\'t change the newsletter_id once it has been set', async function () {
|
||||
// Note: this test only works if there are members subscribed to the initial newsletter
|
||||
// (so it won't get reset when changing the post status to draft again)
|
||||
|
||||
let model;
|
||||
const post = {
|
||||
title: 'My post without newsletter_id',
|
||||
|
|
@ -718,7 +819,7 @@ describe('Posts API', function () {
|
|||
};
|
||||
|
||||
const res2 = await request
|
||||
.put(localUtils.API.getApiQuery('posts/' + id + '/?email_recipient_filter=all&send_email_when_published=true&newsletter_id=' + newsletterId))
|
||||
.put(localUtils.API.getApiQuery('posts/' + id + '/?email_recipient_filter=status:-free&send_email_when_published=true&newsletter_id=' + newsletterId))
|
||||
.set('Origin', config.get('url'))
|
||||
.send({posts: [updatedPost]})
|
||||
.expect('Content-Type', /json/)
|
||||
|
|
@ -735,6 +836,14 @@ describe('Posts API', function () {
|
|||
}, testUtils.context.internal);
|
||||
should(model.get('newsletter_id')).eql(newsletterId);
|
||||
|
||||
// Check email is sent to the correct newsletter
|
||||
let email = await models.Email.findOne({
|
||||
post_id: id
|
||||
}, testUtils.context.internal);
|
||||
|
||||
should(email.get('newsletter_id')).eql(newsletterId);
|
||||
should(email.get('status')).eql('pending');
|
||||
|
||||
const unpublished = {
|
||||
status: 'draft',
|
||||
updated_at: res2.body.posts[0].updated_at
|
||||
|
|
@ -749,7 +858,7 @@ describe('Posts API', function () {
|
|||
.expect(200);
|
||||
|
||||
// Check newsletter relation is loaded in response
|
||||
should(res3.body.posts[0].newsletter).eql(null);
|
||||
should(res3.body.posts[0].newsletter.id).eql(newsletterId);
|
||||
should.not.exist(res3.body.posts[0].newsletter_id);
|
||||
|
||||
model = await models.Post.findOne({
|
||||
|
|
@ -757,9 +866,16 @@ describe('Posts API', function () {
|
|||
status: 'draft'
|
||||
}, testUtils.context.internal);
|
||||
|
||||
// The newsletter id is back to null here, because no email was sent...
|
||||
// This is expected behaviour
|
||||
should(model.get('newsletter_id')).eql(null);
|
||||
should(model.get('newsletter_id')).eql(newsletterId);
|
||||
|
||||
// Check email
|
||||
// Note: we only create an email if we have members susbcribed to the newsletter
|
||||
email = await models.Email.findOne({
|
||||
post_id: id
|
||||
}, testUtils.context.internal);
|
||||
|
||||
should.exist(email);
|
||||
should(email.get('newsletter_id')).eql(newsletterId);
|
||||
|
||||
const republished = {
|
||||
status: 'published',
|
||||
|
|
@ -776,14 +892,14 @@ describe('Posts API', function () {
|
|||
|
||||
// Check newsletter relation is loaded in response
|
||||
// + did update the newsletter id
|
||||
should(res4.body.posts[0].newsletter.id).eql(newsletterId2);
|
||||
should(res4.body.posts[0].newsletter.id).eql(newsletterId);
|
||||
should.not.exist(res4.body.posts[0].newsletter_id);
|
||||
|
||||
model = await models.Post.findOne({
|
||||
id: id,
|
||||
status: 'published'
|
||||
}, testUtils.context.internal);
|
||||
should(model.get('newsletter_id')).eql(newsletterId2);
|
||||
should(model.get('newsletter_id')).eql(newsletterId);
|
||||
|
||||
// Should not change if status remains published
|
||||
const res5 = await request
|
||||
|
|
@ -796,7 +912,7 @@ describe('Posts API', function () {
|
|||
|
||||
// Check newsletter relation is loaded in response
|
||||
// + did not update the newsletter id
|
||||
should(res5.body.posts[0].newsletter.id).eql(newsletterId2);
|
||||
should(res5.body.posts[0].newsletter.id).eql(newsletterId);
|
||||
should.not.exist(res5.body.posts[0].newsletter_id);
|
||||
|
||||
model = await models.Post.findOne({
|
||||
|
|
@ -805,7 +921,7 @@ describe('Posts API', function () {
|
|||
}, testUtils.context.internal);
|
||||
|
||||
// Test if the newsletter_id option was ignored
|
||||
should(model.get('newsletter_id')).eql(newsletterId2);
|
||||
should(model.get('newsletter_id')).eql(newsletterId);
|
||||
});
|
||||
|
||||
it('Can destroy a post', async function () {
|
||||
|
|
|
|||
Loading…
Reference in a new issue