Infrastructure/ghost
0
Fork 0
mirror of https://github.com/TryGhost/Ghost.git synced 2025-02-03 23:00:14 -05:00
Code Issues Activity

🐛 Fixed email segment generation (#16182)

Browse source 
fixes https://github.com/TryGhost/Team/issues/2484

The flow only send the email to segments that were targeted in the email
content. But if a part of the email is only visible for `status:free`,
that doesn't mean we don't want to send the email to `status:-free`.
This has been corrected in the new email flow.
This commit is contained in:
Simon Backx 2023-01-25 14:56:37 +01:00 • committed by GitHub
parent dd74f42376
commit f4c55d123c
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 200 additions and 131 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

305
ghost/core/test/integration/services/email-service/batch-sending.test.js
View file

@ -8,7 +8,13 @@ const MailgunClient = require('@tryghost/mailgun-client/lib/mailgun-client');
const jobManager = require('../../../../core/server/services/jobs/job-service'); const jobManager = require('../../../../core/server/services/jobs/job-service');
const _ = require('lodash'); const _ = require('lodash');
const {MailgunEmailProvider} = require('@tryghost/email-service'); const {MailgunEmailProvider} = require('@tryghost/email-service');
const mobileDocExample = '{"version":"0.3.1","atoms":[],"cards":[],"markups":[],"sections":[[1,"p",[[0,[],0,"Hello world"]]]],"ghostVersion":"4.0"}';
const mobileDocWithPaywall = '{"version":"0.3.1","markups":[],"atoms":[],"cards":[["paywall",{}]],"sections":[[1,"p",[[0,[],0,"Free content"]]],[10,0],[1,"p",[[0,[],0,"Members content"]]]]}'; const mobileDocWithPaywall = '{"version":"0.3.1","markups":[],"atoms":[],"cards":[["paywall",{}]],"sections":[[1,"p",[[0,[],0,"Free content"]]],[10,0],[1,"p",[[0,[],0,"Members content"]]]]}';
const mobileDocWithFreeMemberOnly = '{"version":"0.3.1","atoms":[],"cards":[["email-cta",{"showButton":false,"showDividers":true,"segment":"status:free","alignment":"left","html":"<p>This is for free members only</p>"}]],"markups":[],"sections":[[1,"p",[[0,[],0,"Hello world"]]],[10,0],[1,"p",[[0,[],0,"Bye."]]]],"ghostVersion":"4.0"}';
const mobileDocWithPaidMemberOnly = '{"version":"0.3.1","atoms":[],"cards":[["email-cta",{"showButton":false,"showDividers":true,"segment":"status:-free","alignment":"left","html":"<p>This is for paid members only</p>"}]],"markups":[],"sections":[[1,"p",[[0,[],0,"Hello world"]]],[10,0],[1,"p",[[0,[],0,"Bye."]]]],"ghostVersion":"4.0"}';
const mobileDocWithPaidAndFreeMemberOnly = '{"version":"0.3.1","atoms":[],"cards":[["email-cta",{"showButton":false,"showDividers":true,"segment":"status:free","alignment":"left","html":"<p>This is for free members only</p>"}],["email-cta",{"showButton":false,"showDividers":true,"segment":"status:-free","alignment":"left","html":"<p>This is for paid members only</p>"}]],"markups":[],"sections":[[1,"p",[[0,[],0,"Hello world"]]],[10,0],[10,1],[1,"p",[[0,[],0,"Bye."]]]],"ghostVersion":"4.0"}';
const mobileDocWithFreeMemberOnlyAndPaywall = '{"version":"0.3.1","atoms":[],"cards":[["email-cta",{"showButton":false,"showDividers":true,"segment":"status:free","alignment":"left","html":"<p>This is for free members only</p>"}],["paywall",{}]],"markups":[],"sections":[[1,"p",[[0,[],0,"Hello world"]]],[10,0],[1,"p",[[0,[],0,"Bye."]]],[10,1],[1,"p",[[0,[],0,"This is after the paywall."]]]],"ghostVersion":"4.0"}';
const configUtils = require('../../../utils/configUtils'); const configUtils = require('../../../utils/configUtils');
const {settingsCache} = require('../../../../core/server/services/settings-helpers'); const {settingsCache} = require('../../../../core/server/services/settings-helpers');
const DomainEvents = require('@tryghost/domain-events'); const DomainEvents = require('@tryghost/domain-events');
@ -111,6 +117,61 @@ async function getLastEmail() {
}; };
} }
/**
* Test amount of batches and segmenting for a given email
*
* @param {object} settings
* @param {string|null} email_recipient_filter
* @param {{recipients: number, segment: string | null}[]} expectedBatches
*/
async function testEmailBatches(settings, email_recipient_filter, expectedBatches) {
const completedPromise = jobManager.awaitCompletion('batch-sending-service-job');
const emailModel = await createPublishedPostEmail(settings, email_recipient_filter);
assert.equal(emailModel.get('source_type'), 'mobiledoc');
assert(emailModel.get('subject'));
assert(emailModel.get('from'));
// Await sending job
await completedPromise;
await emailModel.refresh();
assert(emailModel.get('status'), 'submitted');
const expectedTotal = expectedBatches.reduce((acc, batch) => acc + batch.recipients, 0);
assert.equal(emailModel.get('email_count'), expectedTotal, 'This email should have an email_count of ' + expectedTotal + ' recipients');
// Did we create batches?
const batches = await models.EmailBatch.findAll({filter: `email_id:${emailModel.id}`});
assert.equal(batches.models.length, expectedBatches.length);
const remainingBatches = batches.models.slice();
const emailRecipients = [];
for (const expectedBatch of expectedBatches) {
// Check all batches are in send state
const index = remainingBatches.findIndex(b => b.get('member_segment') === expectedBatch.segment);
assert(index !== -1, `Could not find batch with segment ${expectedBatch.segment}`);
const firstBatch = remainingBatches[index];
remainingBatches.splice(index, 1);
assert.equal(firstBatch.get('provider_id'), 'stubbed-email-id');
assert.equal(firstBatch.get('status'), 'submitted');
assert.equal(firstBatch.get('member_segment'), expectedBatch.segment);
assert.equal(firstBatch.get('error_status_code'), null);
assert.equal(firstBatch.get('error_message'), null);
assert.equal(firstBatch.get('error_data'), null);
// Did we create recipients?
const emailRecipientsFirstBatch = await models.EmailRecipient.findAll({filter: `email_id:${emailModel.id}+batch_id:${firstBatch.id}`});
assert.equal(emailRecipientsFirstBatch.models.length, expectedBatch.recipients);
emailRecipients.push(...emailRecipientsFirstBatch.models);
}
// Check members are unique in all batches
const memberIds = emailRecipients.map(recipient => recipient.get('member_id'));
assert.equal(memberIds.length, _.uniq(memberIds).length);
}
describe('Batch sending tests', function () { describe('Batch sending tests', function () {
let linkRedirectService, linkRedirectRepository, linkTrackingService, linkClickRepository; let linkRedirectService, linkRedirectRepository, linkTrackingService, linkClickRepository;
let ghostServer; let ghostServer;
@ -268,146 +329,150 @@ describe('Batch sending tests', function () {
}); });
it('Splits recipients in free and paid batch', async function () { it('Splits recipients in free and paid batch', async function () {
// Prepare a post and email model await testEmailBatches({
const completedPromise = jobManager.awaitCompletion('batch-sending-service-job'); // Requires a paywall = different content for paid and free members
const emailModel = await createPublishedPostEmail({
// Requires a paywall
mobiledoc: mobileDocWithPaywall, mobiledoc: mobileDocWithPaywall,
// Required to trigger the paywall // Required to trigger the paywall
visibility: 'paid' visibility: 'paid'
}); }, null, [
{segment: 'status:free', recipients: 3},
assert.equal(emailModel.get('source_type'), 'mobiledoc'); {segment: 'status:-free', recipients: 2}
assert(emailModel.get('subject')); ]);
assert(emailModel.get('from'));
// Await sending job
await completedPromise;
await emailModel.refresh();
assert(emailModel.get('status'), 'submitted');
assert.equal(emailModel.get('email_count'), 5);
// Did we create batches?
const batches = await models.EmailBatch.findAll({filter: `email_id:${emailModel.id}`});
assert.equal(batches.models.length, 2);
// Check all batches are in send state
const firstBatch = batches.models[0];
assert.equal(firstBatch.get('provider_id'), 'stubbed-email-id');
assert.equal(firstBatch.get('status'), 'submitted');
assert.equal(firstBatch.get('member_segment'), 'status:free');
assert.equal(firstBatch.get('error_status_code'), null);
assert.equal(firstBatch.get('error_message'), null);
assert.equal(firstBatch.get('error_data'), null);
const secondBatch = batches.models[1];
assert.equal(secondBatch.get('provider_id'), 'stubbed-email-id');
assert.equal(secondBatch.get('status'), 'submitted');
assert.equal(secondBatch.get('member_segment'), 'status:-free');
assert.equal(secondBatch.get('error_status_code'), null);
assert.equal(secondBatch.get('error_message'), null);
assert.equal(secondBatch.get('error_data'), null);
// Did we create recipients?
const emailRecipientsFirstBatch = await models.EmailRecipient.findAll({filter: `email_id:${emailModel.id}+batch_id:${firstBatch.id}`});
assert.equal(emailRecipientsFirstBatch.models.length, 3);
const emailRecipientsSecondBatch = await models.EmailRecipient.findAll({filter: `email_id:${emailModel.id}+batch_id:${secondBatch.id}`});
assert.equal(emailRecipientsSecondBatch.models.length, 2);
// Check members are unique
const memberIds = [...emailRecipientsFirstBatch.models, ...emailRecipientsSecondBatch.models].map(recipient => recipient.get('member_id'));
assert.equal(memberIds.length, _.uniq(memberIds).length);
}); });
it('Only sends to members in email recipient filter', async function () { it('Splits recipients in free and paid batch when including free member only content', async function () {
// Prepare a post and email model await testEmailBatches({
const completedPromise = jobManager.awaitCompletion('batch-sending-service-job'); mobiledoc: mobileDocWithFreeMemberOnly // = different content for paid and free members (extra content for free in this case)
const emailModel = await createPublishedPostEmail({ }, null, [
// Requires a paywall {segment: 'status:free', recipients: 3},
mobiledoc: mobileDocWithPaywall, {segment: 'status:-free', recipients: 2}
]);
});
it('Splits recipients in free and paid batch when including paid member only content', async function () {
await testEmailBatches({
mobiledoc: mobileDocWithPaidMemberOnly // = different content for paid and free members (extra content for paid in this case)
}, null, [
{segment: 'status:free', recipients: 3},
{segment: 'status:-free', recipients: 2}
]);
});
it('Splits recipients in free and paid batch when including paid member only content', async function () {
await testEmailBatches({
mobiledoc: mobileDocWithPaidAndFreeMemberOnly // = different content for paid and free members
}, null, [
{segment: 'status:free', recipients: 3},
{segment: 'status:-free', recipients: 2}
]);
});
it('Splits recipients in free and paid batch when including free members only content with paywall', async function () {
await testEmailBatches({
mobiledoc: mobileDocWithFreeMemberOnlyAndPaywall, // = different content for paid and free members (extra content for paid in this case + a paywall)
// Required to trigger the paywall // Required to trigger the paywall
visibility: 'paid' visibility: 'paid'
}, 'status:-free'); }, null, [
{segment: 'status:free', recipients: 3},
{segment: 'status:-free', recipients: 2}
]);
});
assert.equal(emailModel.get('source_type'), 'mobiledoc'); it('Does not split recipient in free and paid batch if email is identical', async function () {
assert(emailModel.get('subject')); await testEmailBatches({
assert(emailModel.get('from')); mobiledoc: mobileDocExample // = same content for free and paid, no need to split batches
}, null, [
{segment: null, recipients: 5}
]);
});
// Await sending job it('Only sends to paid members if recipient filter is applied', async function () {
await completedPromise; await testEmailBatches({
mobiledoc: mobileDocExample // = same content for free and paid, no need to split batches
}, 'status:-free', [
{segment: null, recipients: 2}
]);
});
await emailModel.refresh(); it('Only sends to members with a specific label', async function () {
assert.equal(emailModel.get('status'), 'submitted'); await testEmailBatches({
assert.equal(emailModel.get('email_count'), 2); mobiledoc: mobileDocExample // = same content for free and paid, no need to split batches
}, 'label:label-1', [
{segment: null, recipients: 1} // 1 member was subscribed, one member is not subscribed
]);
});
// Did we create batches? it('Only sends to members with a specific label and paid content', async function () {
const batches = await models.EmailBatch.findAll({filter: `email_id:${emailModel.id}`}); await testEmailBatches({
assert.equal(batches.models.length, 1); mobiledoc: mobileDocWithPaidMemberOnly // = different content for paid and free members (extra content for paid in this case)
}, 'label:label-1', [
{segment: 'status:free', recipients: 1} // The only member with this label is a free member
]);
});
// Check all batches are in send state it('Only sends to members with a specific label and paywall', async function () {
const firstBatch = batches.models[0]; await testEmailBatches({
assert.equal(firstBatch.get('provider_id'), 'stubbed-email-id'); mobiledoc: mobileDocWithPaywall,
assert.equal(firstBatch.get('status'), 'submitted'); visibility: 'paid'
assert.equal(firstBatch.get('member_segment'), 'status:-free'); }, 'label:label-1', [
assert.equal(firstBatch.get('error_status_code'), null); {segment: 'status:free', recipients: 1} // The only member with this label is a free member
assert.equal(firstBatch.get('error_message'), null); ]);
assert.equal(firstBatch.get('error_data'), null); });
// Did we create recipients? it('Can handle OR filter in email recipient filter and split content', async function () {
const emailRecipients = await models.EmailRecipient.findAll({filter: `email_id:${emailModel.id}`}); await testEmailBatches({
assert.equal(emailRecipients.models.length, 2); mobiledoc: mobileDocWithPaywall, // = content should be different for free and paid members
visibility: 'paid'
}, 'status:-free,label:label-1', [
{segment: 'status:free', recipients: 1}, // The only member with this label is a free member
{segment: 'status:-free', recipients: 2} // The only member with this label is a free member
]);
});
// Check members are unique it('Can handle OR filter in email recipient filter without split content', async function () {
const memberIds = emailRecipients.models.map(recipient => recipient.get('member_id')); await testEmailBatches({
assert.equal(_.uniq(memberIds).length, 2); mobiledoc: mobileDocExample // = content is same for free and paid members
}, 'status:-free,label:label-1', [
{segment: null, recipients: 3} // 2 paid members + 1 free member with the label
]);
});
it('Only sends to paid members if recipient filter is applied in combination with free member only content', async function () {
// Tests if the batch generator doesn't go insane if we include a free memebr only content for an email that is only send to paid members
await testEmailBatches({
mobiledoc: mobileDocWithFreeMemberOnly
}, 'status:-free', [
{segment: 'status:-free', recipients: 2}
]);
}); });
it('Splits up in batches according to email provider batch size', async function () { it('Splits up in batches according to email provider batch size', async function () {
MailgunEmailProvider.BATCH_SIZE = 1; MailgunEmailProvider.BATCH_SIZE = 1;
await testEmailBatches({
mobiledoc: mobileDocExample
}, null, [
{segment: null, recipients: 1},
{segment: null, recipients: 1},
{segment: null, recipients: 1},
{segment: null, recipients: 1},
{segment: null, recipients: 1}
]);
});
// Prepare a post and email model it('Splits up in batches according to email provider batch size with paid and free segments', async function () {
const completedPromise = jobManager.awaitCompletion('batch-sending-service-job'); MailgunEmailProvider.BATCH_SIZE = 1;
const emailModel = await createPublishedPostEmail(); await testEmailBatches({
mobiledoc: mobileDocWithPaidMemberOnly
}, null, [
// 2 paid
{segment: 'status:-free', recipients: 1},
{segment: 'status:-free', recipients: 1},
assert.equal(emailModel.get('source_type'), 'mobiledoc'); // 3 free
assert(emailModel.get('subject')); {segment: 'status:free', recipients: 1},
assert(emailModel.get('from')); {segment: 'status:free', recipients: 1},
{segment: 'status:free', recipients: 1}
// Await sending job ]);
await completedPromise;
await emailModel.refresh();
assert.equal(emailModel.get('status'), 'submitted');
assert.equal(emailModel.get('email_count'), 5);
// Did we create batches?
const batches = await models.EmailBatch.findAll({filter: `email_id:${emailModel.id}`});
assert.equal(batches.models.length, 5);
const emailRecipients = [];
// Check all batches are in send state
for (const batch of batches.models) {
assert.equal(batch.get('provider_id'), 'stubbed-email-id');
assert.equal(batch.get('status'), 'submitted');
assert.equal(batch.get('member_segment'), null);
assert.equal(batch.get('error_status_code'), null);
assert.equal(batch.get('error_message'), null);
assert.equal(batch.get('error_data'), null);
// Did we create recipients?
const batchRecipients = await models.EmailRecipient.findAll({filter: `email_id:${emailModel.id}+batch_id:${batch.id}`});
assert.equal(batchRecipients.models.length, 1);
emailRecipients.push(...batchRecipients.models);
}
// Check members are unique
const memberIds = emailRecipients.map(recipient => recipient.get('member_id'));
assert.equal(memberIds.length, _.uniq(memberIds).length);
}); });
it('One failed batch marks the email as failed and allows for a retry', async function () { it('One failed batch marks the email as failed and allows for a retry', async function () {

24
ghost/email-service/lib/email-renderer.js
View file

@ -141,7 +141,8 @@ class EmailRenderer {
} }
/** /**
Not sure about this, but we need a method that can tell us which member segments are needed for a given post/email. Returns all the segments that we need to render the email for because they have different content.
WARNING: The sum of all the returned segments should always include all the members. Those members are later limited if needed based on the recipient filter of the email.
@param {Post} post @param {Post} post
@returns {Segment[]} @returns {Segment[]}
*/ */
@ -149,6 +150,14 @@ class EmailRenderer {
const allowedSegments = ['status:free', 'status:-free']; const allowedSegments = ['status:free', 'status:-free'];
const html = this.renderPostBaseHtml(post); const html = this.renderPostBaseHtml(post);
/**
* Always add free and paid segments if email has paywall card
*/
if (html.indexOf('<!--members-only-->') !== -1) {
// We have different content between free and paid members
return allowedSegments;
}
const cheerio = require('cheerio'); const cheerio = require('cheerio');
const $ = cheerio.load(html); const $ = cheerio.load(html);
@ -156,19 +165,14 @@ class EmailRenderer {
.get() .get()
.map(el => el.attribs['data-gh-segment']); .map(el => el.attribs['data-gh-segment']);
/**
* Always add free and paid segments if email has paywall card
*/
if (html.indexOf('<!--members-only-->') !== -1) {
allSegments = allSegments.concat(['status:free', 'status:-free']);
}
const segments = [...new Set(allSegments)].filter(segment => allowedSegments.includes(segment)); const segments = [...new Set(allSegments)].filter(segment => allowedSegments.includes(segment));
if (segments.length === 0) { if (segments.length === 0) {
// One segment to all members // No difference in email content between free and paid
return [null]; return [null];
} }
return segments;
// We have different content between free and paid members
return allowedSegments;
} }
renderPostBaseHtml(post) { renderPostBaseHtml(post) {

2
ghost/email-service/test/email-renderer.test.js
View file

@ -330,7 +330,7 @@ describe('Email renderer', function () {
} }
}; };
let response = emailRenderer.getSegments(post); let response = emailRenderer.getSegments(post);
response.should.eql(['status:-free']); response.should.eql(['status:free', 'status:-free']);
}); });
}); });