From 1d32931d0ae46dca7229ab508624eab5d583d84a Mon Sep 17 00:00:00 2001 From: Daniel Lockyer Date: Fri, 20 Jan 2023 18:05:36 +0100 Subject: [PATCH 1/7] Revert "Added email snapshots for `API versioning` tests (#16139)" - this reverts commit 85051199e3d0b42a6aa0d2671a2d185be84336a2 - the tests here rely on dynamic content (the Ghost version number) --- .../shared/__snapshots__/version.test.js.snap | 419 ------------------ .../core/test/e2e-api/shared/version.test.js | 28 +- 2 files changed, 15 insertions(+), 432 deletions(-) diff --git a/ghost/core/test/e2e-api/shared/__snapshots__/version.test.js.snap b/ghost/core/test/e2e-api/shared/__snapshots__/version.test.js.snap index 1cb665235c..0863d88131 100644 --- a/ghost/core/test/e2e-api/shared/__snapshots__/version.test.js.snap +++ b/ghost/core/test/e2e-api/shared/__snapshots__/version.test.js.snap @@ -328,178 +328,6 @@ Object { } `; -exports[`API Versioning Admin API responds with error and sends email ONCE when requested version is BEHIND and CANNOT respond multiple times 3: [html 1] 1`] = ` -" - - - - -Integration error - - - - - - - - - - -
  - -
- - - - - - - - - - - - - -
- - - - - - - - - - -
-

Uh-oh!

-
-

- Your Zapier integration is no longer working as expected. This integration must be updated by its developer to work with your version of Ghost. -

- -

- To help you get things fixed as quickly as possible, Ghost has automatically generated some helpful information about the error that you can share with the creator of the Zapier integration below: -

- -

- Integration expected Ghost version:  v3.5 -
- Current Ghost version:  v5.30 -
- Failed request URL:  /ghost/api/admin/removed_endpoint/ -

-
-
-
-

This email was sent from http://127.0.0.1:2369/ to jbloggs@example.com

-
-
- - -
- 		 
- - -" -`; - exports[`API Versioning Admin API responds with error and sends email ONCE when requested version is BEHIND and CANNOT respond multiple times 4: [headers] 1`] = ` Object { "access-control-allow-origin": "http://127.0.0.1:2369", @@ -513,59 +341,6 @@ Object { } `; -exports[`API Versioning Admin API responds with error and sends email ONCE when requested version is BEHIND and CANNOT respond multiple times 4: [metadata 1] 1`] = ` -Object { - "subject": "Attention required: Your Zapier integration has failed", - "text": " [https://static.ghost.org/v4.0.0/images/ghost-orb-1.png] Uh-oh! - -Your Zapier integration is no longer working as expected. This integration must -be updated by its developer to work with your version of Ghost. - -To help you get things fixed as quickly as possible, Ghost has automatically -generated some helpful information about the error that you can share with the -creator of the Zapier integration below: - - Integration expected Ghost version:v3.5 -Current Ghost version:v5.30 -Failed request URL:/ghost/api/admin/removed_endpoint/ - -This email was sent from http://127.0.0.1:2369/ [http://127.0.0.1:2369/] to -jbloggs@example.com [jbloggs@example.com]", - "to": "jbloggs@example.com", -} -`; - -exports[`API Versioning Admin API responds with error and sends email ONCE when requested version is BEHIND and CANNOT respond multiple times 5: [body] 1`] = ` -Object { - "errors": Array [ - Object { - "code": "UPDATE_CLIENT", - "context": StringMatching /Provided client accept-version v3\\.5 is behind current Ghost version v\\\\d\\+\\\\\\.\\\\d\\+/, - "details": null, - "ghostErrorCode": null, - "help": "Try upgrading your Ghost API client.", - "id": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/, - "message": "Request could not be served, the endpoint was not found.", - "property": null, - "type": "RequestNotAcceptableError", - }, - ], -} -`; - -exports[`API Versioning Admin API responds with error and sends email ONCE when requested version is BEHIND and CANNOT respond multiple times 6: [headers] 1`] = ` -Object { - "access-control-allow-origin": "http://127.0.0.1:2369", - "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", - "content-length": StringMatching /\\\\d\\+/, - "content-type": "application/json; charset=utf-8", - "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, - "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, - "vary": "Accept-Version, Origin, Accept-Encoding", - "x-powered-by": "Express", -} -`; - exports[`API Versioning Admin API responds with error requested version is AHEAD and CANNOT respond 1: [body] 1`] = ` Object { "errors": Array [ @@ -628,200 +403,6 @@ Object { } `; -exports[`API Versioning Admin API responds with error when requested version is BEHIND and CANNOT respond 3: [html 1] 1`] = ` -" - - - - -Integration error - - - - - - - - - - -
  - -
- - - - - - - - - - - - - -
- - - - - - - - - - -
-

Uh-oh!

-
-

- Your Zapier integration is no longer working as expected. This integration must be updated by its developer to work with your version of Ghost. -

- -

- To help you get things fixed as quickly as possible, Ghost has automatically generated some helpful information about the error that you can share with the creator of the Zapier integration below: -

- -

- Integration expected Ghost version:  v3.1 -
- Current Ghost version:  v5.30 -
- Failed request URL:  /ghost/api/admin/removed_endpoint/ -

-
-
-
-

This email was sent from http://127.0.0.1:2369/ to jbloggs@example.com

-
-
- - -
- 		 
- - -" -`; - -exports[`API Versioning Admin API responds with error when requested version is BEHIND and CANNOT respond 4: [metadata 1] 1`] = ` -Object { - "subject": "Attention required: Your Zapier integration has failed", - "text": " [https://static.ghost.org/v4.0.0/images/ghost-orb-1.png] Uh-oh! - -Your Zapier integration is no longer working as expected. This integration must -be updated by its developer to work with your version of Ghost. - -To help you get things fixed as quickly as possible, Ghost has automatically -generated some helpful information about the error that you can share with the -creator of the Zapier integration below: - - Integration expected Ghost version:v3.1 -Current Ghost version:v5.30 -Failed request URL:/ghost/api/admin/removed_endpoint/ - -This email was sent from http://127.0.0.1:2369/ [http://127.0.0.1:2369/] to -jbloggs@example.com [jbloggs@example.com]", - "to": "jbloggs@example.com", -} -`; - exports[`API Versioning Admin API responds with no content version header when accept version header is NOT PRESENT 1: [body] 1`] = ` Object { "site": Object { diff --git a/ghost/core/test/e2e-api/shared/version.test.js b/ghost/core/test/e2e-api/shared/version.test.js index 59f8ad4e8a..ce0da74a3e 100644 --- a/ghost/core/test/e2e-api/shared/version.test.js +++ b/ghost/core/test/e2e-api/shared/version.test.js @@ -10,7 +10,6 @@ const settingsMatcher = { describe('API Versioning', function () { describe('Admin API', function () { let agentAdminAPI; - let emailMockReceiver; before(async function () { agentAdminAPI = await agentProvider.getAdminAPIAgent(); @@ -19,7 +18,7 @@ describe('API Versioning', function () { }); beforeEach(function () { - emailMockReceiver = mockManager.mockMail(); + mockManager.mockMail(); }); afterEach(function () { @@ -131,9 +130,11 @@ describe('API Versioning', function () { }] }); - emailMockReceiver.sentEmailCount(1); - emailMockReceiver.matchHTMLSnapshot(); - emailMockReceiver.matchMetadataSnapshot(); + mockManager.assert.sentEmailCount(1); + mockManager.assert.sentEmail({ + subject: 'Attention required: Your Zapier integration has failed', + to: 'jbloggs@example.com' + }); }); it('responds with error and sends email ONCE when requested version is BEHIND and CANNOT respond multiple times', async function () { @@ -154,9 +155,11 @@ describe('API Versioning', function () { }] }); - emailMockReceiver.sentEmailCount(1); - emailMockReceiver.matchHTMLSnapshot(); - emailMockReceiver.matchMetadataSnapshot(); + mockManager.assert.sentEmailCount(1); + mockManager.assert.sentEmail({ + subject: 'Attention required: Your Zapier integration has failed', + to: 'jbloggs@example.com' + }); await agentAdminAPI .get('removed_endpoint') @@ -175,7 +178,7 @@ describe('API Versioning', function () { }] }); - emailMockReceiver.sentEmailCount(1); + mockManager.assert.sentEmailCount(1); }); it('responds with 404 error when the resource cannot be found', async function () { @@ -194,7 +197,7 @@ describe('API Versioning', function () { }] }); - emailMockReceiver.sentEmailCount(0); + mockManager.assert.sentEmailCount(0); }); it('Does an internal rewrite for canary URLs with accept version set', async function () { @@ -268,10 +271,9 @@ describe('API Versioning', function () { describe('Content API', function () { let agentContentAPI; - let emailMockReceiver; beforeEach(function () { - emailMockReceiver = mockManager.mockMail(); + mockManager.mockMail(); }); afterEach(function () { @@ -339,7 +341,7 @@ describe('API Versioning', function () { }] }); - emailMockReceiver.sentEmailCount(0); + mockManager.assert.sentEmailCount(0); }); }); }); From 503a9ebe51b2b6da22653ebb517f82263416857c Mon Sep 17 00:00:00 2001 From: Rishabh Garg Date: Wed, 25 Jan 2023 13:59:43 +0530 Subject: [PATCH 2/7] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20invalid=20expiry=20f?= =?UTF-8?q?or=20member=20tier=20subscriptions=20(#16174)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refs https://github.com/TryGhost/Team/issues/2476 When upgrading from a Complimentary subscription with an expiry, to a paid Subscription of the same Tier, the Member was eventually losing access to the Tier when the complimentary subscription expires as the `expiry_at` on the mapping was not removed. This change fixes the code by setting expiry as null when a member upgrades their subscription to paid. This also adds 2 migrations to fix any side-effects on existing sites - - Removed invalid expiry tier expiry date for paid members - Restored missing tier mapping for paid members --- ...ix-invalid-tier-expiry-for-paid-members.js | 35 ++++++++++++++ ...ore-incorrect-expired-tiers-for-members.js | 46 +++++++++++++++++++ ghost/core/core/server/models/member.js | 4 +- .../test/unit/server/models/member.test.js | 8 ++++ 4 files changed, 91 insertions(+), 2 deletions(-) create mode 100644 ghost/core/core/server/data/migrations/versions/5.32/2023-01-24-08-00-fix-invalid-tier-expiry-for-paid-members.js create mode 100644 ghost/core/core/server/data/migrations/versions/5.32/2023-01-24-08-09-restore-incorrect-expired-tiers-for-members.js diff --git a/ghost/core/core/server/data/migrations/versions/5.32/2023-01-24-08-00-fix-invalid-tier-expiry-for-paid-members.js b/ghost/core/core/server/data/migrations/versions/5.32/2023-01-24-08-00-fix-invalid-tier-expiry-for-paid-members.js new file mode 100644 index 0000000000..0ab8b5a30a --- /dev/null +++ b/ghost/core/core/server/data/migrations/versions/5.32/2023-01-24-08-00-fix-invalid-tier-expiry-for-paid-members.js @@ -0,0 +1,35 @@ +const logging = require('@tryghost/logging'); +const {createTransactionalMigration} = require('../../utils'); + +module.exports = createTransactionalMigration( + async function up(knex) { + logging.info('Removing expiry dates for paid members'); + try { + // Fetch all members with a paid status that have an expiry date + // Paid members should not have an expiry date + const invalidExpiryIds = await knex('members_products') + .select('members_products.id') + .leftJoin('members', 'members_products.member_id', 'members.id') + .where('members.status', '=', 'paid') + .whereNotNull('members_products.expiry_at').pluck('members_products.id'); + + logging.info(`Found ${invalidExpiryIds.length} paid members with expiry dates`); + + if (invalidExpiryIds.length === 0) { + return; + } + + logging.info(`Removing expiry dates for ${invalidExpiryIds.length} paid members`); + + await knex('members_products') + .update('expiry_at', null) + .whereIn('id', invalidExpiryIds); + } catch (err) { + logging.warn('Failed to remove expiry dates for paid members'); + logging.warn(err); + } + }, + async function down() { + // no-op: we don't want to reintroduce the incorrect expiry dates for member tiers + } +); diff --git a/ghost/core/core/server/data/migrations/versions/5.32/2023-01-24-08-09-restore-incorrect-expired-tiers-for-members.js b/ghost/core/core/server/data/migrations/versions/5.32/2023-01-24-08-09-restore-incorrect-expired-tiers-for-members.js new file mode 100644 index 0000000000..83e93ab6b2 --- /dev/null +++ b/ghost/core/core/server/data/migrations/versions/5.32/2023-01-24-08-09-restore-incorrect-expired-tiers-for-members.js @@ -0,0 +1,46 @@ +const logging = require('@tryghost/logging'); +const ObjectId = require('bson-objectid').default; +const {createTransactionalMigration} = require('../../utils'); + +module.exports = createTransactionalMigration( + async function up(knex) { + logging.info('Restoring member<>tier mapping for members with paid status'); + try { + // fetch all members with a paid status that don't have a members_products record + // and have a members_product_events record with an action of "added" + // and fetch the product_id from the most recent record for that member + const memberWithTiers = await knex.select('m.id as member_id', 'mpe.product_id as product_id') + .from('members as m') + .leftJoin('members_products as mp', 'm.id', 'mp.member_id') + .leftJoin('members_product_events as mpe', function () { + this.on('m.id', 'mpe.member_id') + .andOn(knex.raw('mpe.created_at = (SELECT max(created_at) FROM members_product_events WHERE member_id = mpe.member_id and action = "added")')); + }) + .where({'m.status': 'paid', 'mp.member_id': null, 'mpe.action': 'added'}); + + // create a new members_products record for each member with id, member_id and product_id + const toInsert = memberWithTiers.map((memberTier) => { + return { + ...memberTier, + id: ObjectId().toHexString() + }; + }).filter((memberTier) => { + // filter out any members that don't have a product_id for some reason + if (!memberTier.product_id) { + logging.warn(`Invalid record found - member_id: ${memberTier.member_id} is without product_id`); + return false; + } + return true; + }); + + logging.info(`Inserting ${toInsert.length} records into members_products`); + await knex.batchInsert('members_products', toInsert); + } catch (err) { + logging.warn('Failed to restore member<>tier mapping for members with paid status'); + logging.warn(err); + } + }, + async function down() { + // np-op: we don't want to delete the missing records we've just inserted + } +); diff --git a/ghost/core/core/server/models/member.js b/ghost/core/core/server/models/member.js index 455c4c063c..eeb357ed45 100644 --- a/ghost/core/core/server/models/member.js +++ b/ghost/core/core/server/models/member.js @@ -219,8 +219,8 @@ const Member = ghostBookshelf.Model.extend({ async updateTierExpiry(products = [], options = {}) { for (const product of products) { - if (product?.expiry_at) { - const expiry = new Date(product.expiry_at); + if (product?.id) { + const expiry = product.expiry_at ? new Date(product.expiry_at) : null; const queryOptions = _.extend({}, options, { query: {where: {product_id: product.id}} }); diff --git a/ghost/core/test/unit/server/models/member.test.js b/ghost/core/test/unit/server/models/member.test.js index 39aa548c06..dabbc52418 100644 --- a/ghost/core/test/unit/server/models/member.test.js +++ b/ghost/core/test/unit/server/models/member.test.js @@ -77,5 +77,13 @@ describe('Unit: models/member', function () { updatePivot.calledWith({expiry_at: new Date(expiry)}, {query: {where: {product_id: '1'}}}).should.be.true(); }); + + it('calls updatePivot on member products to remove expiry', function () { + memberModel.updateTierExpiry([{ + id: '1' + }]); + + updatePivot.calledWith({expiry_at: null}, {query: {where: {product_id: '1'}}}).should.be.true(); + }); }); }); From 27619dd8a72e9c5defa59eb67792c03e05d6b158 Mon Sep 17 00:00:00 2001 From: Simon Backx Date: Tue, 24 Jan 2023 18:02:10 +0100 Subject: [PATCH 3/7] =?UTF-8?q?=F0=9F=90=9B=20Reduced=20concurrency=20when?= =?UTF-8?q?=20fetching=20Mailgun=20events=20(#16176)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refs https://github.com/TryGhost/Team/issues/2482 This change adds a small sleep in between dispatching events in the worker thread that reads the events from Mailgun. That should reduce the amount of queries we fire parallel to each other and could cause the connection pool to run out of connections. It also reduces the amount of concurrent sending to 2 from 10. Also to make sure the connection pool doesn't run out of connections while sending emails, and to reduce the chance of new connections falling back on a (delayed) replicated database. --- .../bulk-email/bulk-email-processor.js | 2 +- .../email-service/lib/batch-sending-service.js | 6 ++++-- .../email-service/lib/email-event-processor.js | 18 ++++++++++++++++++ .../test/batch-sending-service.test.js | 8 ++++---- 4 files changed, 27 insertions(+), 7 deletions(-) diff --git a/ghost/core/core/server/services/bulk-email/bulk-email-processor.js b/ghost/core/core/server/services/bulk-email/bulk-email-processor.js index c6b52b1898..e8493039fd 100644 --- a/ghost/core/core/server/services/bulk-email/bulk-email-processor.js +++ b/ghost/core/core/server/services/bulk-email/bulk-email-processor.js @@ -93,7 +93,7 @@ module.exports = { } catch (error) { return new FailedBatch(emailBatchId, error); } - }, {concurrency: 10}); + }, {concurrency: 2}); const successes = batchResults.filter(response => (response instanceof SuccessfulBatch)); const failures = batchResults.filter(response => (response instanceof FailedBatch)); diff --git a/ghost/email-service/lib/batch-sending-service.js b/ghost/email-service/lib/batch-sending-service.js index ba0baa42ec..f9dfa35633 100644 --- a/ghost/email-service/lib/batch-sending-service.js +++ b/ghost/email-service/lib/batch-sending-service.js @@ -8,6 +8,8 @@ const messages = { emailError: 'Email failed to send' }; +const MAX_SENDING_CONCURRENCY = 2; + /** * @typedef {import('./sending-service')} SendingService * @typedef {import('./email-segmenter')} EmailSegmenter @@ -267,8 +269,8 @@ class BatchSendingService { } }; - // Run maximum 10 at the same time - await Promise.all(new Array(10).fill(0).map(() => runNext())); + // Run maximum MAX_SENDING_CONCURRENCY at the same time + await Promise.all(new Array(MAX_SENDING_CONCURRENCY).fill(0).map(() => runNext())); if (succeededCount < batches.length) { if (succeededCount > 0) { diff --git a/ghost/email-service/lib/email-event-processor.js b/ghost/email-service/lib/email-event-processor.js index 4130212dcc..5cd6cdd3ce 100644 --- a/ghost/email-service/lib/email-event-processor.js +++ b/ghost/email-service/lib/email-event-processor.js @@ -1,5 +1,11 @@ const {EmailDeliveredEvent, EmailOpenedEvent, EmailBouncedEvent, SpamComplaintEvent, EmailUnsubscribedEvent, EmailTemporaryBouncedEvent} = require('@tryghost/email-events'); +async function waitForEvent() { + return new Promise((resolve) => { + setTimeout(resolve, 100); + }); +} + /** * @typedef EmailIdentification * @property {string} email @@ -43,6 +49,8 @@ class EmailEventProcessor { emailId: recipient.emailId, timestamp })); + // We cannot await the dispatched domainEvent, but we need to limit the number of events thare are processed at the same time + await waitForEvent(); } return recipient; } @@ -61,6 +69,8 @@ class EmailEventProcessor { emailId: recipient.emailId, timestamp })); + // We cannot await the dispatched domainEvent, but we need to limit the number of events thare are processed at the same time + await waitForEvent(); } return recipient; } @@ -81,6 +91,8 @@ class EmailEventProcessor { emailRecipientId: recipient.emailRecipientId, timestamp })); + // We cannot await the dispatched domainEvent, but we need to limit the number of events thare are processed at the same time + await waitForEvent(); } return recipient; } @@ -101,6 +113,8 @@ class EmailEventProcessor { emailRecipientId: recipient.emailRecipientId, timestamp })); + // We cannot await the dispatched domainEvent, but we need to limit the number of events thare are processed at the same time + await waitForEvent(); } return recipient; } @@ -118,6 +132,8 @@ class EmailEventProcessor { emailId: recipient.emailId, timestamp })); + // We cannot await the dispatched domainEvent, but we need to limit the number of events thare are processed at the same time + await waitForEvent(); } return recipient; } @@ -135,6 +151,8 @@ class EmailEventProcessor { emailId: recipient.emailId, timestamp })); + // We cannot await the dispatched domainEvent, but we need to limit the number of events thare are processed at the same time + await waitForEvent(); } return recipient; } diff --git a/ghost/email-service/test/batch-sending-service.test.js b/ghost/email-service/test/batch-sending-service.test.js index fe0458f344..49df4d0199 100644 --- a/ghost/email-service/test/batch-sending-service.test.js +++ b/ghost/email-service/test/batch-sending-service.test.js @@ -440,7 +440,7 @@ describe('Batch Sending Service', function () { assert.equal(arg.batch, batches[0]); }); - it('Works for more than 10 batches', async function () { + it('Works for more than 2 batches', async function () { const service = new BatchSendingService({}); let runningCount = 0; let maxRunningCount = 0; @@ -461,7 +461,7 @@ describe('Batch Sending Service', function () { sinon.assert.callCount(sendBatch, 101); const sendBatches = sendBatch.getCalls().map(call => call.args[0].batch); assert.deepEqual(sendBatches, batches); - assert.equal(maxRunningCount, 10); + assert.equal(maxRunningCount, 2); }); it('Throws error if all batches fail', async function () { @@ -485,7 +485,7 @@ describe('Batch Sending Service', function () { sinon.assert.callCount(sendBatch, 101); const sendBatches = sendBatch.getCalls().map(call => call.args[0].batch); assert.deepEqual(sendBatches, batches); - assert.equal(maxRunningCount, 10); + assert.equal(maxRunningCount, 2); }); it('Throws error if a single batch fails', async function () { @@ -511,7 +511,7 @@ describe('Batch Sending Service', function () { sinon.assert.callCount(sendBatch, 101); const sendBatches = sendBatch.getCalls().map(call => call.args[0].batch); assert.deepEqual(sendBatches, batches); - assert.equal(maxRunningCount, 10); + assert.equal(maxRunningCount, 2); }); }); From fc990e856ae66c85d8d27590f2730e64474ec9c3 Mon Sep 17 00:00:00 2001 From: Simon Backx Date: Wed, 25 Jan 2023 13:20:50 +0100 Subject: [PATCH 4/7] Reduced email error messages (#16183) fixes https://github.com/TryGhost/Team/issues/2487 --- ghost/admin/app/services/feature.js | 1 + ghost/admin/app/templates/settings/labs.hbs | 13 +++++++++++++ .../utils/serializers/output/mappers/emails.js | 8 ++++++++ ghost/core/core/shared/labs.js | 3 ++- 4 files changed, 24 insertions(+), 1 deletion(-) diff --git a/ghost/admin/app/services/feature.js b/ghost/admin/app/services/feature.js index b924856b74..1fad5be987 100644 --- a/ghost/admin/app/services/feature.js +++ b/ghost/admin/app/services/feature.js @@ -66,6 +66,7 @@ export default class FeatureService extends Service { @feature('emailStability') emailStability; @feature('webmentions') webmentions; @feature('outboundLinkTagging') outboundLinkTagging; + @feature('emailErrors') emailErrors; _user = null; diff --git a/ghost/admin/app/templates/settings/labs.hbs b/ghost/admin/app/templates/settings/labs.hbs index 932116aeea..c3b746d231 100644 --- a/ghost/admin/app/templates/settings/labs.hbs +++ b/ghost/admin/app/templates/settings/labs.hbs @@ -239,6 +239,19 @@ +
+
+
+

Show email errors

+

+ This makes email errors visible in the UI. +

+
+
+ +
+
+
{{/if}} diff --git a/ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/emails.js b/ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/emails.js index bb7ceeee1e..fe3e3edb0d 100644 --- a/ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/emails.js +++ b/ghost/core/core/server/api/endpoints/utils/serializers/output/mappers/emails.js @@ -1,4 +1,6 @@ const mega = require('../../../../../../services/mega'); +const labs = require('../../../../../../../shared/labs'); +const config = require('../../../../../../../shared/config'); module.exports = (model, frame) => { const jsonModel = model.toJSON ? model.toJSON(frame.options) : model; @@ -13,5 +15,11 @@ module.exports = (model, frame) => { ); }); + if (!labs.isSet('emailErrors') && !!(config.get('bulkEmail') && config.get('bulkEmail').mailgun)) { + if (jsonModel.status === 'failed') { + jsonModel.status = 'submitted'; + } + } + return jsonModel; }; diff --git a/ghost/core/core/shared/labs.js b/ghost/core/core/shared/labs.js index 6d6e1ac39f..670eac2a45 100644 --- a/ghost/core/core/shared/labs.js +++ b/ghost/core/core/shared/labs.js @@ -26,7 +26,8 @@ const GA_FEATURES = [ // NOTE: this allowlist is meant to be used to filter out any unexpected // input for the "labs" setting value const BETA_FEATURES = [ - 'activitypub' + 'activitypub', + 'emailErrors' ]; const ALPHA_FEATURES = [ From 846d033e207385f02dbed7056b5db2c456166c1c Mon Sep 17 00:00:00 2001 From: Simon Backx Date: Wed, 25 Jan 2023 14:57:10 +0100 Subject: [PATCH 5/7] Improved email error logging (#16184) no issue Logs errors to Sentry and adds error codes. --- .../bulk-email/bulk-email-processor.js | 9 ++- .../server/services/email-service/wrapper.js | 3 +- .../lib/batch-sending-service.js | 35 ++++++++-- .../lib/mailgun-email-provider.js | 2 +- .../test/batch-sending-service.test.js | 68 ++++++++++++++++++- 5 files changed, 105 insertions(+), 12 deletions(-) diff --git a/ghost/core/core/server/services/bulk-email/bulk-email-processor.js b/ghost/core/core/server/services/bulk-email/bulk-email-processor.js index e8493039fd..7f94480ad8 100644 --- a/ghost/core/core/server/services/bulk-email/bulk-email-processor.js +++ b/ghost/core/core/server/services/bulk-email/bulk-email-processor.js @@ -196,8 +196,11 @@ module.exports = { // log any error that didn't come from the provider which would have already logged it if (!error.code || error.code !== 'BULK_EMAIL_SEND_FAILED') { - let ghostError = new errors.InternalServerError({ - err: error + let ghostError = new errors.EmailError({ + err: error, + code: 'BULK_EMAIL_SEND_FAILED', + message: `Error sending email batch ${emailBatchId}`, + context: error.message }); sentry.captureException(ghostError); logging.error(ghostError); @@ -274,7 +277,7 @@ module.exports = { }); sentry.captureException(ghostError); - logging.warn(ghostError); + logging.error(ghostError); debug(`failed to send message (${Date.now() - startTime}ms)`); throw ghostError; diff --git a/ghost/core/core/server/services/email-service/wrapper.js b/ghost/core/core/server/services/email-service/wrapper.js index 9b2cfcaf62..9bf3efe442 100644 --- a/ghost/core/core/server/services/email-service/wrapper.js +++ b/ghost/core/core/server/services/email-service/wrapper.js @@ -88,7 +88,8 @@ class EmailServiceWrapper { jobsService, emailSegmenter, emailRenderer, - db + db, + sentry }); this.service = new EmailService({ diff --git a/ghost/email-service/lib/batch-sending-service.js b/ghost/email-service/lib/batch-sending-service.js index f9dfa35633..fca0cc3a22 100644 --- a/ghost/email-service/lib/batch-sending-service.js +++ b/ghost/email-service/lib/batch-sending-service.js @@ -29,6 +29,7 @@ class BatchSendingService { #jobsService; #models; #db; + #sentry; /** * @param {Object} dependencies @@ -42,6 +43,7 @@ class BatchSendingService { * @param {Email} dependencies.models.Email * @param {object} dependencies.models.Member * @param {object} dependencies.db + * @param {object} [dependencies.sentry] */ constructor({ emailRenderer, @@ -49,7 +51,8 @@ class BatchSendingService { jobsService, emailSegmenter, models, - db + db, + sentry }) { this.#emailRenderer = emailRenderer; this.#sendingService = sendingService; @@ -57,6 +60,7 @@ class BatchSendingService { this.#emailSegmenter = emailSegmenter; this.#models = models; this.#db = db; + this.#sentry = sentry; } /** @@ -97,7 +101,17 @@ class BatchSendingService { error: null }, {patch: true, autoRefresh: false}); } catch (e) { - logging.error(`Error sending email ${email.id}: ${e.message}`); + const ghostError = new errors.EmailError({ + err: e, + code: 'BULK_EMAIL_SEND_FAILED', + message: `Error sending email ${email.id}` + }); + + logging.error(ghostError); + if (this.#sentry) { + // Log the original error to Sentry + this.#sentry.captureException(e); + } // Edge case: Store error in email model (that are not caught by the batch) await email.save({ @@ -324,8 +338,21 @@ class BatchSendingService { }, {patch: true, require: false, autoRefresh: false}); succeeded = true; } catch (err) { - logging.error(`Error sending email batch ${batch.id}`); - logging.error(err); + if (!err.code || err.code !== 'BULK_EMAIL_SEND_FAILED') { + // BULK_EMAIL_SEND_FAILED are already logged in mailgun-email-provider + const ghostError = new errors.EmailError({ + err, + code: 'BULK_EMAIL_SEND_FAILED', + message: `Error sending email batch ${batch.id}`, + context: err.message + }); + + logging.error(ghostError); + if (this.#sentry) { + // Log the original error to Sentry + this.#sentry.captureException(err); + } + } await batch.save({ status: 'failed', diff --git a/ghost/email-service/lib/mailgun-email-provider.js b/ghost/email-service/lib/mailgun-email-provider.js index 7712d0f32e..a26b0e40a3 100644 --- a/ghost/email-service/lib/mailgun-email-provider.js +++ b/ghost/email-service/lib/mailgun-email-provider.js @@ -168,7 +168,7 @@ class MailgunEmailProvider { }); } - logging.warn(ghostError); + logging.error(ghostError); debug(`failed to send message (${Date.now() - startTime}ms)`); // log error to custom error handler. ex sentry diff --git a/ghost/email-service/test/batch-sending-service.test.js b/ghost/email-service/test/batch-sending-service.test.js index 49df4d0199..1fdae467e2 100644 --- a/ghost/email-service/test/batch-sending-service.test.js +++ b/ghost/email-service/test/batch-sending-service.test.js @@ -125,8 +125,12 @@ describe('Batch Sending Service', function () { status: 'pending' } }); + const captureException = sinon.stub(); const service = new BatchSendingService({ - models: {Email} + models: {Email}, + sentry: { + captureException + } }); let emailModel; let afterEmailModel; @@ -141,6 +145,16 @@ describe('Batch Sending Service', function () { assert.equal(result, undefined); sinon.assert.calledOnce(errorLog); sinon.assert.calledOnce(sendEmail); + sinon.assert.calledOnce(captureException); + + // Check error code + const error = errorLog.firstCall.args[0]; + assert.equal(error.code, 'BULK_EMAIL_SEND_FAILED'); + + // Check error + const sentryError = captureException.firstCall.args[0]; + assert.equal(sentryError.message, ''); + assert.equal(emailModel.status, 'submitting', 'The email status is submitting while sending'); assert.equal(afterEmailModel.get('status'), 'failed', 'The email status is failed after sending'); assert.equal(afterEmailModel.get('error'), 'Something went wrong while sending the email'); @@ -621,7 +635,7 @@ describe('Batch Sending Service', function () { }); assert.equal(result, false); - sinon.assert.calledTwice(errorLog); + sinon.assert.calledOnce(errorLog); sinon.assert.calledOnce(sendingService.send); sinon.assert.calledOnce(findOne); @@ -632,6 +646,54 @@ describe('Batch Sending Service', function () { assert.equal(batch.get('error_data'), null); }); + it('Does log error to Sentry', async function () { + const EmailBatch = createModelClass({ + findOne: { + status: 'pending', + member_segment: null + } + }); + const sendingService = { + send: sinon.stub().rejects(new Error('Test error')) + }; + + const findOne = sinon.spy(EmailBatch, 'findOne'); + const captureException = sinon.stub(); + const service = new BatchSendingService({ + models: {EmailBatch, EmailRecipient}, + sendingService, + sentry: { + captureException + } + }); + + const result = await service.sendBatch({ + email: createModel({}), + batch: createModel({}), + post: createModel({}), + newsletter: createModel({}) + }); + + assert.equal(result, false); + sinon.assert.calledOnce(errorLog); + sinon.assert.calledOnce(sendingService.send); + sinon.assert.calledOnce(captureException); + const sentryExeption = captureException.firstCall.args[0]; + assert.equal(sentryExeption.message, 'Test error'); + + const loggedExeption = errorLog.firstCall.args[0]; + assert.match(loggedExeption.message, /Error sending email batch/); + assert.equal(loggedExeption.context, 'Test error'); + assert.equal(loggedExeption.code, 'BULK_EMAIL_SEND_FAILED'); + + sinon.assert.calledOnce(findOne); + const batch = await findOne.firstCall.returnValue; + assert.equal(batch.get('status'), 'failed'); + assert.equal(batch.get('error_status_code'), null); + assert.equal(batch.get('error_message'), 'Test error'); + assert.equal(batch.get('error_data'), null); + }); + it('Does save EmailError', async function () { const EmailBatch = createModelClass({ findOne: { @@ -664,7 +726,7 @@ describe('Batch Sending Service', function () { }); assert.equal(result, false); - sinon.assert.calledTwice(errorLog); + sinon.assert.notCalled(errorLog); sinon.assert.calledOnce(sendingService.send); sinon.assert.calledOnce(findOne); From 2d11c29695961eb60a5e8b8e4ed57b44e7b5206e Mon Sep 17 00:00:00 2001 From: Simon Backx Date: Wed, 25 Jan 2023 14:56:37 +0100 Subject: [PATCH 6/7] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20email=20segment=20ge?= =?UTF-8?q?neration=20(#16182)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../email-service/batch-sending.test.js | 305 +++++++++++------- ghost/email-service/lib/email-renderer.js | 24 +- .../email-service/test/email-renderer.test.js | 2 +- 3 files changed, 200 insertions(+), 131 deletions(-) diff --git a/ghost/core/test/integration/services/email-service/batch-sending.test.js b/ghost/core/test/integration/services/email-service/batch-sending.test.js index 843b3de7aa..0c7103d158 100644 --- a/ghost/core/test/integration/services/email-service/batch-sending.test.js +++ b/ghost/core/test/integration/services/email-service/batch-sending.test.js @@ -8,7 +8,13 @@ const MailgunClient = require('@tryghost/mailgun-client/lib/mailgun-client'); const jobManager = require('../../../../core/server/services/jobs/job-service'); const _ = require('lodash'); 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 mobileDocWithFreeMemberOnly = '{"version":"0.3.1","atoms":[],"cards":[["email-cta",{"showButton":false,"showDividers":true,"segment":"status:free","alignment":"left","html":"

This is for free members only

"}]],"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":"

This is for paid members only

"}]],"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":"

This is for free members only

"}],["email-cta",{"showButton":false,"showDividers":true,"segment":"status:-free","alignment":"left","html":"

This is for paid members only

"}]],"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":"

This is for free members only

"}],["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 {settingsCache} = require('../../../../core/server/services/settings-helpers'); 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 () { let linkRedirectService, linkRedirectRepository, linkTrackingService, linkClickRepository; let ghostServer; @@ -268,146 +329,150 @@ describe('Batch sending tests', function () { }); it('Splits recipients in free and paid batch', async function () { - // Prepare a post and email model - const completedPromise = jobManager.awaitCompletion('batch-sending-service-job'); - const emailModel = await createPublishedPostEmail({ - // Requires a paywall + await testEmailBatches({ + // Requires a paywall = different content for paid and free members mobiledoc: mobileDocWithPaywall, // Required to trigger the paywall visibility: 'paid' - }); - - 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'); - 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); + }, null, [ + {segment: 'status:free', recipients: 3}, + {segment: 'status:-free', recipients: 2} + ]); }); - it('Only sends to members in email recipient filter', async function () { - // Prepare a post and email model - const completedPromise = jobManager.awaitCompletion('batch-sending-service-job'); - const emailModel = await createPublishedPostEmail({ - // Requires a paywall - mobiledoc: mobileDocWithPaywall, + it('Splits recipients in free and paid batch when including free member only content', async function () { + await testEmailBatches({ + mobiledoc: mobileDocWithFreeMemberOnly // = different content for paid and free members (extra content for free 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: 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 visibility: 'paid' - }, 'status:-free'); + }, null, [ + {segment: 'status:free', recipients: 3}, + {segment: 'status:-free', recipients: 2} + ]); + }); - assert.equal(emailModel.get('source_type'), 'mobiledoc'); - assert(emailModel.get('subject')); - assert(emailModel.get('from')); + it('Does not split recipient in free and paid batch if email is identical', async function () { + await testEmailBatches({ + mobiledoc: mobileDocExample // = same content for free and paid, no need to split batches + }, null, [ + {segment: null, recipients: 5} + ]); + }); - // Await sending job - await completedPromise; + it('Only sends to paid members if recipient filter is applied', async function () { + await testEmailBatches({ + mobiledoc: mobileDocExample // = same content for free and paid, no need to split batches + }, 'status:-free', [ + {segment: null, recipients: 2} + ]); + }); - await emailModel.refresh(); - assert.equal(emailModel.get('status'), 'submitted'); - assert.equal(emailModel.get('email_count'), 2); + it('Only sends to members with a specific label', async function () { + await testEmailBatches({ + 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? - const batches = await models.EmailBatch.findAll({filter: `email_id:${emailModel.id}`}); - assert.equal(batches.models.length, 1); + it('Only sends to members with a specific label and paid content', async function () { + await testEmailBatches({ + 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 - 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); + it('Only sends to members with a specific label and paywall', async function () { + await testEmailBatches({ + mobiledoc: mobileDocWithPaywall, + visibility: 'paid' + }, 'label:label-1', [ + {segment: 'status:free', recipients: 1} // The only member with this label is a free member + ]); + }); - // Did we create recipients? - const emailRecipients = await models.EmailRecipient.findAll({filter: `email_id:${emailModel.id}`}); - assert.equal(emailRecipients.models.length, 2); + it('Can handle OR filter in email recipient filter and split content', async function () { + await testEmailBatches({ + 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 - const memberIds = emailRecipients.models.map(recipient => recipient.get('member_id')); - assert.equal(_.uniq(memberIds).length, 2); + it('Can handle OR filter in email recipient filter without split content', async function () { + await testEmailBatches({ + 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 () { 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 - const completedPromise = jobManager.awaitCompletion('batch-sending-service-job'); - const emailModel = await createPublishedPostEmail(); + it('Splits up in batches according to email provider batch size with paid and free segments', async function () { + MailgunEmailProvider.BATCH_SIZE = 1; + await testEmailBatches({ + mobiledoc: mobileDocWithPaidMemberOnly + }, null, [ + // 2 paid + {segment: 'status:-free', recipients: 1}, + {segment: 'status:-free', recipients: 1}, - assert.equal(emailModel.get('source_type'), 'mobiledoc'); - assert(emailModel.get('subject')); - assert(emailModel.get('from')); - - // 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); + // 3 free + {segment: 'status:free', recipients: 1}, + {segment: 'status:free', recipients: 1}, + {segment: 'status:free', recipients: 1} + ]); }); it('One failed batch marks the email as failed and allows for a retry', async function () { diff --git a/ghost/email-service/lib/email-renderer.js b/ghost/email-service/lib/email-renderer.js index 0b0ea00913..64ec24cc1e 100644 --- a/ghost/email-service/lib/email-renderer.js +++ b/ghost/email-service/lib/email-renderer.js @@ -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 @returns {Segment[]} */ @@ -149,6 +150,14 @@ class EmailRenderer { const allowedSegments = ['status:free', 'status:-free']; const html = this.renderPostBaseHtml(post); + /** + * Always add free and paid segments if email has paywall card + */ + if (html.indexOf('') !== -1) { + // We have different content between free and paid members + return allowedSegments; + } + const cheerio = require('cheerio'); const $ = cheerio.load(html); @@ -156,19 +165,14 @@ class EmailRenderer { .get() .map(el => el.attribs['data-gh-segment']); - /** - * Always add free and paid segments if email has paywall card - */ - if (html.indexOf('') !== -1) { - allSegments = allSegments.concat(['status:free', 'status:-free']); - } - const segments = [...new Set(allSegments)].filter(segment => allowedSegments.includes(segment)); if (segments.length === 0) { - // One segment to all members + // No difference in email content between free and paid return [null]; } - return segments; + + // We have different content between free and paid members + return allowedSegments; } renderPostBaseHtml(post) { diff --git a/ghost/email-service/test/email-renderer.test.js b/ghost/email-service/test/email-renderer.test.js index f811f62c6e..e13b4c5e6b 100644 --- a/ghost/email-service/test/email-renderer.test.js +++ b/ghost/email-service/test/email-renderer.test.js @@ -330,7 +330,7 @@ describe('Email renderer', function () { } }; let response = emailRenderer.getSegments(post); - response.should.eql(['status:-free']); + response.should.eql(['status:free', 'status:-free']); }); }); From b1b24a688405151e70297ce55b83682b354cc9f9 Mon Sep 17 00:00:00 2001 From: Ghost CI <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 25 Jan 2023 14:32:26 +0000 Subject: [PATCH 7/7] v5.32.0 --- ghost/admin/package.json | 2 +- ghost/core/package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ghost/admin/package.json b/ghost/admin/package.json index 55689dbaee..6f63803f63 100644 --- a/ghost/admin/package.json +++ b/ghost/admin/package.json @@ -1,6 +1,6 @@ { "name": "ghost-admin", - "version": "5.31.0", + "version": "5.32.0", "description": "Ember.js admin client for Ghost", "author": "Ghost Foundation", "homepage": "http://ghost.org", diff --git a/ghost/core/package.json b/ghost/core/package.json index 34d6366ad8..3715102553 100644 --- a/ghost/core/package.json +++ b/ghost/core/package.json @@ -1,6 +1,6 @@ { "name": "ghost", - "version": "5.31.0", + "version": "5.32.0", "description": "The professional publishing platform", "author": "Ghost Foundation", "homepage": "https://ghost.org",