From 953f3856a842039cb5c66603c27d801460d17ab2 Mon Sep 17 00:00:00 2001 From: "Fabien \"egg\" O'Carroll" Date: Thu, 5 Jan 2023 16:24:15 +0700 Subject: [PATCH] Handled EmailBounceEvent with 605 error code When Mailgun fails to deliver an email to an address because the address has already bounced before, it gives us a permanent fail event with a 605 error code rather than a 5xx one. Because we want to "backfill" our suppressions data with previously bounced email addresses, we want to handle this specific error code. We may update this logic in the future based on new information from Mailgun with respect to their 6xx error codes and the meanings/underlying cause of theme. This also moves the tests which check for whether or not emails are suppressed into their own fail so that we do not pollute the event storage tests, and adds more tests cases. We also fix a leaky sinon stub which we were not resetting in the email event storage tests --- .../MailgunEmailSuppressionList.js | 2 +- .../email-service/email-event-storage.test.js | 18 +- .../mailgun-email-suppression-list.test.js | 241 ++++++++++++++++++ 3 files changed, 246 insertions(+), 15 deletions(-) create mode 100644 ghost/core/test/integration/services/mailgun-email-suppression-list.test.js diff --git a/ghost/core/core/server/services/email-suppression-list/MailgunEmailSuppressionList.js b/ghost/core/core/server/services/email-suppression-list/MailgunEmailSuppressionList.js index 102e92e255..26a863be99 100644 --- a/ghost/core/core/server/services/email-suppression-list/MailgunEmailSuppressionList.js +++ b/ghost/core/core/server/services/email-suppression-list/MailgunEmailSuppressionList.js @@ -101,7 +101,7 @@ class MailgunEmailSuppressionList extends AbstractEmailSuppressionList { if (!Number.isInteger(event.error?.code)) { return; } - if (event.error.code < 500 || event.error.code > 599) { + if (event.error.code < 500 || event.error.code > 599 && event.error.code !== 605) { return; } } diff --git a/ghost/core/test/integration/services/email-service/email-event-storage.test.js b/ghost/core/test/integration/services/email-service/email-event-storage.test.js index cf9c5d918d..bea035ce81 100644 --- a/ghost/core/test/integration/services/email-service/email-event-storage.test.js +++ b/ghost/core/test/integration/services/email-service/email-event-storage.test.js @@ -41,6 +41,10 @@ describe('EmailEventStorage', function () { }); }); + after(function () { + sinon.restore(); + }); + it('Can handle delivered events', async function () { const emailBatch = fixtureManager.get('email_batches', 0); const emailId = emailBatch.email_id; @@ -209,9 +213,6 @@ describe('EmailEventStorage', function () { const providerId = emailBatch.provider_id; const timestamp = new Date(2000, 0, 1); - const {body: {members: [member]}} = await agent.get(`/members/${memberId}`); - assert.equal(member.email_suppression.suppressed, false, 'This test requires a member that does not have a suppressed email'); - events = [{ event: 'failed', id: 'pl271FzxTTmGRW8Uj3dUWw', @@ -301,10 +302,6 @@ describe('EmailEventStorage', function () { assert.equal(permanentFailures.models[0].get('event_id'), 'pl271FzxTTmGRW8Uj3dUWw'); assert.equal(permanentFailures.models[0].get('severity'), 'permanent'); assert.equal(permanentFailures.models[0].get('failed_at').toUTCString(), timestamp.toUTCString()); - - const {body: {members: [memberAfter]}} = await agent.get(`/members/${memberId}`); - assert.equal(memberAfter.email_suppression.suppressed, false, 'The member should not have a suppressed email'); - assert.equal(memberAfter.email_suppression.info, null); }); it('Ignores permanent failures if already failed', async function () { @@ -756,9 +753,6 @@ describe('EmailEventStorage', function () { const existingSpamEvent = eventsBefore.find(event => event.type === 'email_complaint_event'); assert.equal(existingSpamEvent, null, 'This test requires a member that does not have a spam event'); - const {body: {members: [member]}} = await agent.get(`/members/${memberId}`); - assert.equal(member.email_suppression.suppressed, false, 'This test requires a member that does not have a suppressed email'); - events = [{ event: 'complained', recipient: emailRecipient.member_email, @@ -790,10 +784,6 @@ describe('EmailEventStorage', function () { const {body: {events: eventsAfter}} = await agent.get(eventsURI); const spamComplaintEvent = eventsAfter.find(event => event.type === 'email_complaint_event'); assert.equal(spamComplaintEvent.type, 'email_complaint_event'); - - const {body: {members: [memberAfter]}} = await agent.get(`/members/${memberId}`); - assert.equal(memberAfter.email_suppression.suppressed, true, 'The member should have a suppressed email'); - assert.equal(memberAfter.email_suppression.info.reason, 'spam'); }); it('Can handle unsubscribe events', async function () { diff --git a/ghost/core/test/integration/services/mailgun-email-suppression-list.test.js b/ghost/core/test/integration/services/mailgun-email-suppression-list.test.js new file mode 100644 index 0000000000..38690c284c --- /dev/null +++ b/ghost/core/test/integration/services/mailgun-email-suppression-list.test.js @@ -0,0 +1,241 @@ +const sinon = require('sinon'); +const {agentProvider, fixtureManager} = require('../../utils/e2e-framework'); +const assert = require('assert'); +const MailgunClient = require('@tryghost/mailgun-client'); +const DomainEvents = require('@tryghost/domain-events'); + +describe('MailgunEmailSuppressionList', function () { + let agent; + let events = []; + let run; + + before(async function () { + agent = await agentProvider.getAdminAPIAgent(); + await fixtureManager.init('newsletters', 'members:newsletters', 'members:emails'); + await agent.loginAsOwner(); + + // Only reference services after Ghost boot + run = require('../../../core/server/services/email-analytics/jobs/fetch-latest/run.js').run; + + sinon.stub(MailgunClient.prototype, 'fetchEvents').callsFake(async function (_, batchHandler) { + const normalizedEvents = (events.map(this.normalizeEvent) || []).filter(e => !!e); + return [await batchHandler(normalizedEvents)]; + }); + }); + + after(function () { + sinon.restore(); + }); + + it('Can handle permanent failure events with an error code of 605', async function () { + const emailBatch = fixtureManager.get('email_batches', 0); + + const emailRecipient = fixtureManager.get('email_recipients', 0); + assert(emailRecipient.batch_id === emailBatch.id); + const memberId = emailRecipient.member_id; + const providerId = emailBatch.provider_id; + const timestamp = new Date(2000, 0, 1); + const recipient = emailRecipient.member_email; + + const {body: {members: [member]}} = await agent.get(`/members/${memberId}`); + assert.equal(member.email_suppression.suppressed, false, 'This test requires a member that does not have a suppressed email'); + + events = [createPermanentFailedEvent({ + errorCode: 605, + providerId, + timestamp, + recipient + })]; + + await run({ + domainEvents: DomainEvents + }); + + await DomainEvents.allSettled(); + + const {body: {members: [memberAfter]}} = await agent.get(`/members/${memberId}`); + assert.equal(memberAfter.email_suppression.suppressed, true, 'The member should have a suppressed email'); + assert.equal(memberAfter.email_suppression.info.reason, 'fail'); + }); + + it('Can handle permanent failure events with an error code of 6xx', async function () { + const emailBatch = fixtureManager.get('email_batches', 0); + + const emailRecipient = fixtureManager.get('email_recipients', 1); + assert(emailRecipient.batch_id === emailBatch.id); + const memberId = emailRecipient.member_id; + const providerId = emailBatch.provider_id; + const timestamp = new Date(2000, 0, 1); + const recipient = emailRecipient.member_email; + + const {body: {members: [member]}} = await agent.get(`/members/${memberId}`); + assert.equal(member.email_suppression.suppressed, false, 'This test requires a member that does not have a suppressed email'); + + const errorCode = 600 + Math.floor(Math.random() * 100); + + events = [createPermanentFailedEvent({ + errorCode: errorCode === 605 ? 606 : errorCode, // Random number between 600-699, but not 605 + providerId, + timestamp, + recipient + })]; + + await run({ + domainEvents: DomainEvents + }); + + await DomainEvents.allSettled(); + + const {body: {members: [memberAfter]}} = await agent.get(`/members/${memberId}`); + assert.equal(memberAfter.email_suppression.suppressed, false, 'The member should not have a suppressed email'); + assert.equal(memberAfter.email_suppression.info, null); + }); + + it('Can handle permanent failure events with an error code of 4xx', async function () { + const emailBatch = fixtureManager.get('email_batches', 0); + + const emailRecipient = fixtureManager.get('email_recipients', 2); + assert(emailRecipient.batch_id === emailBatch.id); + const memberId = emailRecipient.member_id; + const providerId = emailBatch.provider_id; + const timestamp = new Date(2000, 0, 1); + const recipient = emailRecipient.member_email; + + const {body: {members: [member]}} = await agent.get(`/members/${memberId}`); + assert.equal(member.email_suppression.suppressed, false, 'This test requires a member that does not have a suppressed email'); + + events = [createPermanentFailedEvent({ + errorCode: 400 + Math.floor(Math.random() * 100), // Random number between 400-499 + providerId, + timestamp, + recipient + })]; + + await run({ + domainEvents: DomainEvents + }); + + await DomainEvents.allSettled(); + + const {body: {members: [memberAfter]}} = await agent.get(`/members/${memberId}`); + assert.equal(memberAfter.email_suppression.suppressed, false, 'The member should not have a suppressed email'); + assert.equal(memberAfter.email_suppression.info, null); + }); + + it('Can handle permanent failure events with an error code of 5xx', async function () { + const emailBatch = fixtureManager.get('email_batches', 0); + + const emailRecipient = fixtureManager.get('email_recipients', 3); + assert(emailRecipient.batch_id === emailBatch.id); + const memberId = emailRecipient.member_id; + const providerId = emailBatch.provider_id; + const timestamp = new Date(2000, 0, 1); + const recipient = emailRecipient.member_email; + + const {body: {members: [member]}} = await agent.get(`/members/${memberId}`); + assert.equal(member.email_suppression.suppressed, false, 'This test requires a member that does not have a suppressed email'); + + events = [createPermanentFailedEvent({ + errorCode: 500 + Math.floor(Math.random() * 100), // Random number between 500-599 + providerId, + timestamp, + recipient + })]; + + await run({ + domainEvents: DomainEvents + }); + + await DomainEvents.allSettled(); + + const {body: {members: [memberAfter]}} = await agent.get(`/members/${memberId}`); + assert.equal(memberAfter.email_suppression.suppressed, true, 'The member should have a suppressed email'); + assert.equal(memberAfter.email_suppression.info.reason, 'fail'); + }); + + it('Can handle complaint events', async function () { + const emailBatch = fixtureManager.get('email_batches', 0); + const emailId = emailBatch.email_id; + + const emailRecipient = fixtureManager.get('email_recipients', 4); + assert(emailRecipient.batch_id === emailBatch.id); + const memberId = emailRecipient.member_id; + const providerId = emailBatch.provider_id; + const timestamp = new Date(2000, 0, 1); + + const {body: {members: [member]}} = await agent.get(`/members/${memberId}`); + assert.equal(member.email_suppression.suppressed, false, 'This test requires a member that does not have a suppressed email'); + + events = [{ + event: 'complained', + recipient: emailRecipient.member_email, + 'user-variables': { + 'email-id': emailId + }, + message: { + headers: { + 'message-id': providerId + } + }, + timestamp: Math.round(timestamp.getTime() / 1000) + }]; + + await run({ + domainEvents: DomainEvents + }); + + await DomainEvents.allSettled(); + + const {body: {members: [memberAfter]}} = await agent.get(`/members/${memberId}`); + assert.equal(memberAfter.email_suppression.suppressed, true, 'The member should have a suppressed email'); + assert.equal(memberAfter.email_suppression.info.reason, 'spam'); + }); +}); + +function createPermanentFailedEvent({errorCode, providerId, timestamp, recipient}) { + return { + event: 'failed', + id: 'pl271FzxTTmGRW8Uj3dUWw', + 'log-level': 'error', + severity: 'permanent', + reason: 'suppress-bounce', + envelope: { + sender: 'john@example.org', + transport: 'smtp', + targets: 'joan@example.com' + }, + flags: { + 'is-routed': false, + 'is-authenticated': true, + 'is-system-test': false, + 'is-test-mode': false + }, + 'delivery-status': { + 'attempt-no': 1, + message: '', + code: errorCode, + description: 'Not delivering to previously bounced address', + 'session-seconds': 0.0 + }, + message: { + headers: { + to: 'joan@example.com', + 'message-id': providerId, + from: 'john@example.org', + subject: 'Test Subject' + }, + attachments: [], + size: 867 + }, + storage: { + url: 'https://se.api.mailgun.net/v3/domains/example.org/messages/eyJwI...', + key: 'eyJwI...' + }, + recipient: recipient, + 'recipient-domain': 'mailgun.com', + campaigns: [], + tags: [], + 'user-variables': {}, + timestamp: Math.round(timestamp.getTime() / 1000) + }; +}