🐛 Fixed setting delivered_at to null after hard bounce (#15942)

refs https://ghost.slack.com/archives/C02G9E68C/p1670075366333929?thread_ts=1669963540.980309&cid=C02G9E68C

When we receive a permanent bounce/failure, we set delivered_at to null.
But we don't want to lose this information.

Instead we should be able to handle recipients that both have failed_at
and delivered_at set.

ghost/core/test/integration/services/email-service/email-event-storage.test.js

@@ -18,7 +18,7 @@ async function resetFailures(emailId) {
     await models.EmailRecipientFailure.destroy({
         destroyBy: {
             email_id: emailId
-        }   
+        }
     });
 }
 
@@ -282,9 +282,9 @@ describe('EmailEventStorage', function () {
         }, {require: true});
 
         assert.equal(updatedEmailRecipient.get('failed_at').toUTCString(), timestamp.toUTCString());
-        
-        // Check delivered at is reset back to null
-        assert.equal(updatedEmailRecipient.get('delivered_at'), null);
+
+        // Check delivered at is NOT reset back to null
+        assert.notEqual(updatedEmailRecipient.get('delivered_at'), null);
 
         // Check we have a stored permanent failure
         const permanentFailures = await models.EmailRecipientFailure.findAll({
@@ -300,31 +300,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());
-
-        // Sometimes we emit events outside of order beacuse of the TRUST_THRESHOLD of the provider-mailgun class.
-        // Check if we handle this correctly.
-        // Manually emit the delivered event again, and see if it is ignored correctly
-        // @ts-ignore
-        domainEvents.dispatch(EmailDeliveredEvent.create({
-            email: emailRecipient.member_email,
-            emailRecipientId: emailRecipient.id,
-            memberId: memberId,
-            emailId: emailId,
-            timestamp
-        }));
-
-        // Now wait for events processed
-        await sleep(200);
-
-        // Check delivered at is not set again
-        const updatedEmailRecipient2 = await models.EmailRecipient.findOne({
-            id: emailRecipient.id
-        }, {require: true});
-
-        assert.equal(updatedEmailRecipient2.get('failed_at').toUTCString(), timestamp.toUTCString());
-        
-        // Check delivered at is reset back to null
-        assert.equal(updatedEmailRecipient2.get('delivered_at'), null, 'A delivered event after a permanent failure event should be ignored');
     });
 
     it('Ignores permanent failures if already failed', async function () {

ghost/email-service/lib/email-event-storage.js

@@ -68,17 +68,17 @@ class EmailEventStorage {
 
     async handleDelivered(event) {
         // To properly handle events that are received out of order (this happens because of polling)
-        // we only can set an email recipient to delivered if they are not already marked as failed
-        // Why handle this her?  An email can be 'delivered' and later have a delayed bounce event. So we need to prevent that delivered_at is set again.
+        // only set if delivered_at is null
         await this.#db.knex('email_recipients')
             .where('id', '=', event.emailRecipientId)
-            .whereNull('failed_at')
             .update({
                 delivered_at: this.#db.knex.raw('COALESCE(delivered_at, ?)', [moment.utc(event.timestamp).format('YYYY-MM-DD HH:mm:ss')])
             });
     }
 
     async handleOpened(event) {
+        // To properly handle events that are received out of order (this happens because of polling)
+        // only set if opened_at is null
         await this.#db.knex('email_recipients')
             .where('id', '=', event.emailRecipientId)
             .update({
@@ -87,11 +87,12 @@ class EmailEventStorage {
     }
 
     async handlePermanentFailed(event) {
+        // To properly handle events that are received out of order (this happens because of polling)
+        // only set if failed_at is null
         await this.#db.knex('email_recipients')
             .where('id', '=', event.emailRecipientId)
             .update({
-                failed_at: this.#db.knex.raw('COALESCE(failed_at, ?)', [moment.utc(event.timestamp).format('YYYY-MM-DD HH:mm:ss')]),
-                delivered_at: null // Reset in case we have a delayed bounce event
+                failed_at: this.#db.knex.raw('COALESCE(failed_at, ?)', [moment.utc(event.timestamp).format('YYYY-MM-DD HH:mm:ss')])
             });
         await this.saveFailure('permanent', event);
     }

ghost/email-service/test/email-event-storage.test.js

@@ -128,7 +128,6 @@ describe('Email event storage', function () {
         await waitPromise;
         sinon.assert.calledOnce(db.update);
         assert(!!db.update.firstCall.args[0].failed_at);
-        assert(db.update.firstCall.args[0].delivered_at === null);
         assert(existing.save.calledOnce);
     });
 
@@ -174,7 +173,6 @@ describe('Email event storage', function () {
         await waitPromise;
         sinon.assert.calledOnce(db.update);
         assert(!!db.update.firstCall.args[0].failed_at);
-        assert(db.update.firstCall.args[0].delivered_at === null);
         assert(EmailRecipientFailure.add.calledOnce);
     });
 
@@ -231,7 +229,6 @@ describe('Email event storage', function () {
         await waitPromise;
         sinon.assert.calledOnce(db.update);
         assert(!!db.update.firstCall.args[0].failed_at);
-        assert(db.update.firstCall.args[0].delivered_at === null);
         assert(EmailRecipientFailure.findOne.called);
         assert(!existing.save.called);
     });