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: '
Look! I\'m an email
', 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: 'What\'s that? Another email!
', 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',