From 9ca2e3f1831553ebd71fa91a1c5dedce94a7f9c8 Mon Sep 17 00:00:00 2001
From: Simon Backx <simon@ghost.org>
Date: Tue, 10 Jan 2023 15:41:42 +0100
Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20storing=20email=20recipi?=
 =?UTF-8?q?ent=20failures?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

fixes https://github.com/TryGhost/Team/issues/2398

There was an error when fetching the existing email recipient failure. It ended up matching all recipient failures. The result was that only one failure was stored in the database.
---
 .../email-service/email-event-storage.test.js | 97 +++++++++++++++++++
 .../email-service/lib/email-event-storage.js  |  2 +-
 2 files changed, 98 insertions(+), 1 deletion(-)

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 bea035ce81..a30b43e55e 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
@@ -399,6 +399,103 @@ describe('EmailEventStorage', function () {
         assert.notEqual(permanentFailures.models[0].get('failed_at').toUTCString(), timestamp.toUTCString());
     });
 
+    it('Can handle permanent failure events for multiple recipients', async function () {
+        const emailBatch = fixtureManager.get('email_batches', 0);
+        const emailId = emailBatch.email_id;
+
+        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);
+
+        events = [{
+            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: 605,
+                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: emailRecipient.member_email,
+            'recipient-domain': 'mailgun.com',
+            campaigns: [],
+            tags: [],
+            'user-variables': {},
+            timestamp: Math.round(timestamp.getTime() / 1000)
+        }];
+
+        const initialModel = await models.EmailRecipient.findOne({
+            id: emailRecipient.id
+        }, {require: true});
+
+        assert.equal(initialModel.get('failed_at'), null);
+
+        // Fire event processing
+        // We use offloading to have correct coverage and usage of worker thread
+        const {eventStats: result} = await run({
+            domainEvents
+        });
+        assert.equal(result.permanentFailed, 1);
+        assert.deepEqual(result.emailIds, [emailId]);
+        assert.deepEqual(result.memberIds, [memberId]);
+
+        // Since this is all event based we should wait for all dispatched events to be completed.
+        await DomainEvents.allSettled();
+
+        // Check if status has changed to delivered, with correct timestamp
+        const updatedEmailRecipient = await models.EmailRecipient.findOne({
+            id: emailRecipient.id
+        }, {require: true});
+
+        assert.equal(updatedEmailRecipient.get('failed_at').toUTCString(), timestamp.toUTCString());
+
+        // Check we have a stored permanent failure
+        const permanentFailures = await models.EmailRecipientFailure.findAll({
+            filter: `email_recipient_id:${emailRecipient.id}`
+        });
+        assert.equal(permanentFailures.length, 1);
+
+        assert.equal(permanentFailures.models[0].get('message'), 'Not delivering to previously bounced address');
+        assert.equal(permanentFailures.models[0].get('code'), 605);
+        assert.equal(permanentFailures.models[0].get('enhanced_code'), null);
+        assert.equal(permanentFailures.models[0].get('email_id'), emailId);
+        assert.equal(permanentFailures.models[0].get('member_id'), memberId);
+        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());
+    });
+
     it('Can handle temporary failure events', async function () {
         const emailBatch = fixtureManager.get('email_batches', 0);
         const emailId = emailBatch.email_id;
diff --git a/ghost/email-service/lib/email-event-storage.js b/ghost/email-service/lib/email-event-storage.js
index 4d8272b046..6a862604c6 100644
--- a/ghost/email-service/lib/email-event-storage.js
+++ b/ghost/email-service/lib/email-event-storage.js
@@ -122,7 +122,7 @@ class EmailEventStorage {
 
         // Create a forUpdate transaction
         const existing = await this.#models.EmailRecipientFailure.findOne({
-            filter: `email_recipient_id:${event.emailRecipientId}`
+            email_recipient_id: event.emailRecipientId
         }, {...options, require: false, forUpdate: true});
 
         if (!existing) {