From 6db7cc81569191f2e1057f1344689b816313725e Mon Sep 17 00:00:00 2001
From: Sag <guptazy@gmail.com>
Date: Mon, 13 Nov 2023 16:56:37 -0300
Subject: [PATCH] Removed email from Mailgun's suppression list on unsubscribe
(#18922)
closes https://github.com/TryGhost/Product/issues/4075
- when a member clicks on "Unsubscribe from that list" from Apple Mail,
the member's email is put into Mailgun's Unsubscribe suppression list.
Ghost listens for "Unsubscribe" events from Mailgun, and unsubscribes
the member from all the newsletters
- now, the member is only unsubscribed from the newsletter they
unsubscribe to (not all of them)
- now, the email is also deleted from Mailgun's suppression list, so
that it doesn't affect any other membership
---
.../EmailAnalyticsServiceWrapper.js | 7 ++-
.../MailgunEmailSuppressionList.js | 9 ++++
.../email-service/email-event-storage.test.js | 34 ++++++++++----
.../test/utils/fixtures/data-generator.js | 6 ++-
ghost/email-service/lib/EmailEventStorage.js | 30 ++++++++++--
.../test/email-event-storage.test.js | 46 ++++++++++++++++++-
6 files changed, 115 insertions(+), 17 deletions(-)
diff --git a/ghost/core/core/server/services/email-analytics/EmailAnalyticsServiceWrapper.js b/ghost/core/core/server/services/email-analytics/EmailAnalyticsServiceWrapper.js
index cfa52a84aa..c8892f5809 100644
--- a/ghost/core/core/server/services/email-analytics/EmailAnalyticsServiceWrapper.js
+++ b/ghost/core/core/server/services/email-analytics/EmailAnalyticsServiceWrapper.js
@@ -9,7 +9,7 @@ class EmailAnalyticsServiceWrapper {
const {EmailAnalyticsService} = require('@tryghost/email-analytics-service');
const {EmailEventStorage, EmailEventProcessor} = require('@tryghost/email-service');
const MailgunProvider = require('@tryghost/email-analytics-provider-mailgun');
- const {EmailRecipientFailure, EmailSpamComplaintEvent} = require('../../models');
+ const {EmailRecipientFailure, EmailSpamComplaintEvent, Email} = require('../../models');
const StartEmailAnalyticsJobEvent = require('./events/StartEmailAnalyticsJobEvent');
const domainEvents = require('@tryghost/domain-events');
@@ -19,14 +19,17 @@ class EmailAnalyticsServiceWrapper {
const queries = require('./lib/queries');
const membersService = require('../members');
const membersRepository = membersService.api.members;
+ const emailSuppressionList = require('../email-suppression-list');
this.eventStorage = new EmailEventStorage({
db,
membersRepository,
models: {
+ Email,
EmailRecipientFailure,
EmailSpamComplaintEvent
- }
+ },
+ emailSuppressionList
});
// Since this is running in a worker thread, we cant dispatch directly
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 802763b004..6831574cd5 100644
--- a/ghost/core/core/server/services/email-suppression-list/MailgunEmailSuppressionList.js
+++ b/ghost/core/core/server/services/email-suppression-list/MailgunEmailSuppressionList.js
@@ -47,6 +47,15 @@ class MailgunEmailSuppressionList extends AbstractEmailSuppressionList {
return true;
}
+ async removeUnsubscribe(email) {
+ try {
+ await this.apiClient.removeUnsubscribe(email);
+ } catch (err) {
+ logging.error(err);
+ return false;
+ }
+ }
+
async getSuppressionData(email) {
try {
const model = await this.Suppression.findOne({
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 186baa24d9..964a002b7d 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
@@ -1028,31 +1028,41 @@ describe('EmailEventStorage', function () {
});
it('Can handle unsubscribe events', async function () {
+ const newsletterToRemove = fixtureManager.get('newsletters', 0).id;
+ const newsletterToKeep = fixtureManager.get('newsletters', 1).id;
+
+ const email = fixtureManager.get('emails', 0);
+ await models.Email.edit({newsletter_id: newsletterToRemove}, {id: email.id});
+
const emailBatch = fixtureManager.get('email_batches', 0);
- const emailId = emailBatch.email_id;
+ assert(emailBatch.email_id === email.id);
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);
- // Reset
+ // Initialise member with 2 newsletters
await membersService.api.members.update({newsletters: [
{
- id: fixtureManager.get('newsletters', 0).id
+ id: newsletterToRemove
+ },
+ {
+ id: newsletterToKeep
}
]}, {id: memberId});
- // Check not unsubscribed
+ // Check that the member is subscribed to 2 newsletters
const memberInitial = await membersService.api.members.get({id: memberId}, {withRelated: ['newsletters']});
- assert.notEqual(memberInitial.related('newsletters').length, 0, 'This test requires a member that is subscribed to at least one newsletter');
+ assert.equal(memberInitial.related('newsletters').length, 2, 'This test requires a member that is subscribed to at least one newsletter');
events = [{
event: 'unsubscribed',
recipient: emailRecipient.member_email,
'user-variables': {
- 'email-id': emailId
+ 'email-id': email.id
},
message: {
headers: {
@@ -1070,9 +1080,17 @@ describe('EmailEventStorage', function () {
// Since this is all event based we should wait for all dispatched events to be completed.
await DomainEvents.allSettled();
- // Check if unsubscribed
+ // The member should be unsubscribed from the specific newsletter
const member = await membersService.api.members.get({id: memberId}, {withRelated: ['newsletters']});
- assert.equal(member.related('newsletters').length, 0);
+
+ // The member is now subscribed to 1 newsletter
+ assert.equal(member.related('newsletters').length, 1);
+
+ // The member is now unsubscribed from newsletter 0
+ assert(!member.related('newsletters').models.some(newsletter => newsletter.id === newsletterToRemove));
+
+ // But the member is still subscribed to newsletter 1
+ assert(member.related('newsletters').models.some(newsletter => newsletter.id === newsletterToKeep));
});
it('Can handle unknown events', async function () {
diff --git a/ghost/core/test/utils/fixtures/data-generator.js b/ghost/core/test/utils/fixtures/data-generator.js
index fec0b903aa..0e4eece6ae 100644
--- a/ghost/core/test/utils/fixtures/data-generator.js
+++ b/ghost/core/test/utils/fixtures/data-generator.js
@@ -736,7 +736,8 @@ DataGenerator.Content = {
html: '<p>Look! I\'m an email</p>',
plaintext: 'Waba-daba-dab-da',
track_opens: false,
- submitted_at: moment().toDate()
+ submitted_at: moment().toDate(),
+ newsletter_id: null // newsletter[0] relation added later
},
{
id: ObjectId().toHexString(),
@@ -748,7 +749,8 @@ DataGenerator.Content = {
html: '<p>What\'s that? Another email!</p>',
plaintext: 'yes this is an email',
track_opens: false,
- submitted_at: moment().toDate()
+ submitted_at: moment().toDate(),
+ newsletter_id: null // newsletter[1] relation added later
}
],
diff --git a/ghost/email-service/lib/EmailEventStorage.js b/ghost/email-service/lib/EmailEventStorage.js
index 749b4edeb1..55b5187b6c 100644
--- a/ghost/email-service/lib/EmailEventStorage.js
+++ b/ghost/email-service/lib/EmailEventStorage.js
@@ -5,11 +5,13 @@ class EmailEventStorage {
#db;
#membersRepository;
#models;
+ #emailSuppressionList;
- constructor({db, models, membersRepository}) {
+ constructor({db, models, membersRepository, emailSuppressionList}) {
this.#db = db;
this.#models = models;
this.#membersRepository = membersRepository;
+ this.#emailSuppressionList = emailSuppressionList;
}
async handleDelivered(event) {
@@ -111,7 +113,16 @@ class EmailEventStorage {
}
async handleUnsubscribed(event) {
- return this.unsubscribeFromNewsletters(event);
+ try {
+ // Unsubscribe member from the specific newsletter
+ const newsletters = await this.findNewslettersToKeep(event);
+ await this.#membersRepository.update({newsletters}, {id: event.memberId});
+
+ // Remove member from Mailgun's suppression list
+ await this.#emailSuppressionList.removeUnsubscribe(event.email);
+ } catch (err) {
+ logging.error(err);
+ }
}
async handleComplained(event) {
@@ -128,11 +139,22 @@ class EmailEventStorage {
}
}
- async unsubscribeFromNewsletters(event) {
+ async findNewslettersToKeep(event) {
try {
- await this.#membersRepository.update({newsletters: []}, {id: event.memberId});
+ const member = await this.#membersRepository.get({email: event.email}, {
+ withRelated: ['newsletters']
+ });
+ const existingNewsletters = member.related('newsletters');
+
+ const email = await this.#models.Email.findOne({id: event.emailId});
+ const newsletterToRemove = email.get('newsletter_id');
+
+ return existingNewsletters.models.filter(newsletter => newsletter.id !== newsletterToRemove).map((n) => {
+ return {id: n.id};
+ });
} catch (err) {
logging.error(err);
+ return [];
}
}
}
diff --git a/ghost/email-service/test/email-event-storage.test.js b/ghost/email-service/test/email-event-storage.test.js
index 770c06c722..a2db2b278a 100644
--- a/ghost/email-service/test/email-event-storage.test.js
+++ b/ghost/email-service/test/email-event-storage.test.js
@@ -456,14 +456,20 @@ describe('Email Event Storage', function () {
const update = sinon.stub().resolves();
+ const emailSuppressionList = {
+ removeUnsubscribe: sinon.stub().resolves()
+ };
+
const eventHandler = new EmailEventStorage({
membersRepository: {
update
- }
+ },
+ emailSuppressionList
});
await eventHandler.handleUnsubscribed(event);
assert(update.calledOnce);
assert(update.firstCall.args[0].newsletters.length === 0);
+ assert(emailSuppressionList.removeUnsubscribe.calledOnce);
});
it('Handles unsubscribe with a non-existent member', async function () {
@@ -487,6 +493,44 @@ describe('Email Event Storage', function () {
assert(update.firstCall.args[0].newsletters.length === 0);
});
+ it('Finds newsletters to keep during an unsubscribe', async function () {
+ const event = EmailUnsubscribedEvent.create({
+ email: 'example@example.com',
+ memberId: '123',
+ emailId: '456',
+ timestamp: new Date(0)
+ });
+
+ const Email = {
+ findOne: sinon.stub().resolves({
+ get: sinon.stub().returns('newsletter_1')
+ })
+ };
+
+ const membersRepository = {
+ get: sinon.stub().resolves({
+ related: sinon.stub().returns({
+ models: [
+ {id: 'newsletter_1'},
+ {id: 'newsletter_2'}
+ ]
+ })
+ })
+ };
+
+ const eventHandler = new EmailEventStorage({
+ membersRepository,
+ models: {
+ Email
+ }
+ });
+
+ const result = await eventHandler.findNewslettersToKeep(event);
+
+ assert(result.length === 1);
+ assert(result[0].id === 'newsletter_2');
+ });
+
it('Handles complaints', async function () {
const event = SpamComplaintEvent.create({
email: 'example@example.com',