Don't leave dangling users in mailchimp when change-address fails (#2165)

* Don't leave dangling users in mailchimp when change-address fails

Also prevents upserting of users when unsubscribing

bug: overleaf/issues#2220
bug: overleaf/issues#2301
bug: overleaf/issues#2302

* Tidy up NewsletterManager. Enable deletion of contacts on account delete.

GitOrigin-RevId: ab7cde7a7b7018b37dea54ffa154e02c5aea0244

services/web/app/src/Features/Newsletter/NewsletterManager.js

@@ -56,68 +56,120 @@ function makeMailchimpProvider() {
     }
   }
 
-  async function unsubscribe(user) {
+  async function unsubscribe(user, options = {}) {
     try {
       const path = getSubscriberPath(user.email)
-      await mailchimp.put(path, {
-        email_address: user.email,
+      if (options.delete) {
+        await mailchimp.delete(path)
+      } else {
+        await mailchimp.patch(path, {
           status: 'unsubscribed',
-        status_if_new: 'unsubscribed',
           merge_fields: getMergeFields(user)
         })
-      logger.info({ user }, 'finished unsubscribing user from newsletter')
+      }
+      logger.info(
+        { user, options },
+        'finished unsubscribing user from newsletter'
+      )
     } catch (err) {
+      if (err.status === 404 || err.status === 405) {
+        // silently ignore users who were never subscribed (404) or previously deleted (405)
+        return
+      }
+
       if (err.message.includes('looks fake or invalid')) {
         logger.info(
-          { err, user },
+          { err, user, options },
           'Mailchimp declined to unsubscribe user because it finds the email looks fake'
         )
-      } else {
+        return
+      }
+
       throw new OError({
         message: 'error unsubscribing user from newsletter',
         info: { userId: user._id }
       }).withCause(err)
     }
   }
+
+  async function changeEmail(user, newEmail) {
+    const oldEmail = user.email
+
+    try {
+      await updateEmailInMailchimp(user, newEmail)
+    } catch (updateError) {
+      // if we failed to update the user, delete their old email address so that
+      // we don't leave it stuck in mailchimp
+      logger.info(
+        { oldEmail, newEmail, updateError },
+        'unable to change email in newsletter, removing old mail'
+      )
+
+      try {
+        await unsubscribe(user, { delete: true })
+      } catch (unsubscribeError) {
+        // something went wrong removing the user's address
+        throw new OError({
+          message:
+            'error unsubscribing old email in response to email change failure',
+          info: { oldEmail, newEmail, updateError }
+        }).withCause(unsubscribeError)
+      }
+
+      // throw the error, unless it was an expected one that we can ignore
+      if (!updateError.info || !updateError.info.nonFatal) {
+        throw updateError
+      }
+    }
+  }
+
+  async function updateEmailInMailchimp(user, newEmail) {
+    const oldEmail = user.email
+
+    // mailchimp doesn't give us error codes, so we have to parse the message :'(
+    const errors = {
+      'merge fields were invalid': 'user has never subscribed',
+      'could not be validated':
+        'user has previously unsubscribed or new email already exist on list',
+      'is already a list member': 'new email is already on mailing list',
+      'looks fake or invalid': 'mail looks fake to mailchimp'
     }
 
-  async function changeEmail(oldEmail, newEmail) {
     try {
       const path = getSubscriberPath(oldEmail)
-      await mailchimp.put(path, {
+      await mailchimp.patch(path, {
         email_address: newEmail,
-        status_if_new: 'unsubscribed'
+        merge_fields: getMergeFields(user)
       })
       logger.info('finished changing email in the newsletter')
     } catch (err) {
-      if (err.message.includes('merge fields were invalid')) {
-        logger.info(
-          { oldEmail, newEmail },
-          'unable to change email in newsletter, user has never subscribed'
-        )
-      } else if (err.message.includes('could not be validated')) {
-        logger.info(
-          { oldEmail, newEmail },
-          'unable to change email in newsletter, user has previously unsubscribed or new email already exist on list'
-        )
-      } else if (err.message.includes('is already a list member')) {
-        logger.info(
-          { oldEmail, newEmail },
-          'unable to change email in newsletter, new email is already on mailing list'
-        )
-      } else if (err.message.includes('looks fake or invalid')) {
-        logger.info(
-          { oldEmail, newEmail },
-          'unable to change email in newsletter, email looks fake to mailchimp'
-        )
-      } else {
+      // silently ignore users who were never subscribed
+      if (err.status === 404) {
+        return
+      }
+
+      // look through expected mailchimp errors and log if we find one
+      Object.keys(errors).forEach(key => {
+        if (err.message.includes(key)) {
+          const message = `unable to change email in newsletter, ${errors[key]}`
+
+          logger.info({ oldEmail, newEmail }, message)
+
+          // throw a non-fatal error
+          throw new OError({
+            message: message,
+            info: { oldEmail, newEmail, nonFatal: true }
+          }).withCause(err)
+        }
+      })
+
+      // if we didn't find an expected error, generate something to throw
       throw new OError({
         message: 'error changing email in newsletter',
         info: { oldEmail, newEmail }
       }).withCause(err)
     }
   }
-  }
 
   function getSubscriberPath(email) {
     const emailHash = hashEmail(email)

services/web/app/src/Features/User/UserDeleter.js

@@ -110,7 +110,7 @@ async function _cleanupUser(user) {
   if (user == null) {
     throw new Error('no user supplied')
   }
-  await NewsletterManager.promises.unsubscribe(user)
+  await NewsletterManager.promises.unsubscribe(user, { delete: true })
   await SubscriptionHandler.promises.cancelSubscription(user)
   await InstitutionsAPI.promises.deleteAffiliations(user._id)
   await SubscriptionUpdater.promises.removeUserFromAllGroups(user._id)

services/web/app/src/Features/User/UserUpdater.js

@@ -172,7 +172,7 @@ const UserUpdater = {
         if (res.n === 0) {
           return callback(new Error('email update error'))
         }
-        NewsletterManager.changeEmail(oldEmail, email, err => {
+        NewsletterManager.changeEmail(user, email, err => {
           if (err != null) {
             logger.warn(
               { err, oldEmail, newEmail: email },

services/web/test/unit/src/Newsletter/NewsletterManagerTests.js

@@ -13,15 +13,24 @@ describe('NewsletterManager', function() {
       }
     }
     this.mailchimp = {
-      put: sinon.stub()
+      put: sinon.stub(),
+      patch: sinon.stub(),
+      delete: sinon.stub()
     }
     this.Mailchimp = sinon.stub().returns(this.mailchimp)
 
+    this.mergeFields = {
+      FNAME: 'Overleaf',
+      LNAME: 'Duck',
+      MONGO_ID: 'user_id'
+    }
+
     this.NewsletterManager = SandboxedModule.require(MODULE_PATH, {
       requires: {
         'mailchimp-api-v3': this.Mailchimp,
         'settings-sharelatex': this.Settings
-      }
+      },
+      globals: { console: console }
     }).promises
 
     this.user = {
@@ -43,36 +52,27 @@ describe('NewsletterManager', function() {
           email_address: this.user.email,
           status: 'subscribed',
           status_if_new: 'subscribed',
-          merge_fields: {
-            FNAME: 'Overleaf',
-            LNAME: 'Duck',
-            MONGO_ID: 'user_id'
-          }
+          merge_fields: this.mergeFields
         }
       )
     })
   })
 
   describe('unsubscribe', function() {
+    describe('when unsubscribing normally', function() {
       it('calls Mailchimp to unsubscribe the user', async function() {
         await this.NewsletterManager.unsubscribe(this.user)
-      expect(this.mailchimp.put).to.have.been.calledWith(
+        expect(this.mailchimp.patch).to.have.been.calledWith(
           `/lists/list_id/members/${this.emailHash}`,
           {
-          email_address: this.user.email,
             status: 'unsubscribed',
-          status_if_new: 'unsubscribed',
-          merge_fields: {
-            FNAME: 'Overleaf',
-            LNAME: 'Duck',
-            MONGO_ID: 'user_id'
-          }
+            merge_fields: this.mergeFields
           }
         )
       })
 
       it('ignores a Mailchimp error about fake emails', async function() {
-      this.mailchimp.put.rejects(
+        this.mailchimp.patch.rejects(
           new Error(
             'overleaf.duck@example.com looks fake or invalid, please enter a real email address'
           )
@@ -82,26 +82,77 @@ describe('NewsletterManager', function() {
       })
 
       it('rejects on other errors', async function() {
-      this.mailchimp.put.rejects(
+        this.mailchimp.patch.rejects(
           new Error('something really wrong is happening')
         )
-      await expect(this.NewsletterManager.unsubscribe(this.user)).to.be.rejected
+        await expect(this.NewsletterManager.unsubscribe(this.user)).to.be
+          .rejected
+      })
+    })
+
+    describe('when deleting', function() {
+      it('calls Mailchimp to delete the user', async function() {
+        await this.NewsletterManager.unsubscribe(this.user, { delete: true })
+        expect(this.mailchimp.delete).to.have.been.calledWith(
+          `/lists/list_id/members/${this.emailHash}`
+        )
+      })
+
+      it('ignores a Mailchimp error about fake emails', async function() {
+        this.mailchimp.delete.rejects(
+          new Error(
+            'overleaf.duck@example.com looks fake or invalid, please enter a real email address'
+          )
+        )
+        await expect(
+          this.NewsletterManager.unsubscribe(this.user, { delete: true })
+        ).to.be.fulfilled
+      })
+
+      it('rejects on other errors', async function() {
+        this.mailchimp.delete.rejects(
+          new Error('something really wrong is happening')
+        )
+        await expect(
+          this.NewsletterManager.unsubscribe(this.user, { delete: true })
+        ).to.be.rejected
+      })
     })
   })
 
   describe('changeEmail', function() {
     it('calls Mailchimp to change the subscriber email', async function() {
       await this.NewsletterManager.changeEmail(
-        this.user.email,
+        this.user,
         'overleaf.squirrel@example.com'
       )
-      expect(this.mailchimp.put).to.have.been.calledWith(
+      expect(this.mailchimp.patch).to.have.been.calledWith(
         `/lists/list_id/members/${this.emailHash}`,
         {
           email_address: 'overleaf.squirrel@example.com',
-          status_if_new: 'unsubscribed'
+          merge_fields: this.mergeFields
         }
       )
     })
+
+    it('deletes the old email if changing the address fails', async function() {
+      this.mailchimp.patch
+        .withArgs(`/lists/list_id/members/${this.emailHash}`, {
+          email_address: 'overleaf.squirrel@example.com',
+          merge_fields: this.mergeFields
+        })
+        .rejects(new Error('that did not work'))
+
+      await expect(
+        this.NewsletterManager.changeEmail(
+          this.user,
+          'overleaf.squirrel@example.com'
+        )
+      ).to.be.rejected
+
+      expect(this.mailchimp.delete).to.have.been.calledWith(
+        `/lists/list_id/members/${this.emailHash}`
+      )
+    })
   })
 })

services/web/test/unit/src/User/UserDeleterTests.js

@@ -159,11 +159,11 @@ describe('UserDeleter', function() {
             this.UserMock.verify()
           })
 
-          it('should unsubscribe the user from the news letter', async function() {
+          it('should delete the user from mailchimp', async function() {
             await this.UserDeleter.promises.deleteUser(this.userId)
             expect(
               this.NewsletterManager.promises.unsubscribe
-            ).to.have.been.calledWith(this.user)
+            ).to.have.been.calledWith(this.user, { delete: true })
           })
 
           it('should delete all the projects of a user', async function() {

services/web/test/unit/src/User/UserUpdaterTests.js

@@ -366,7 +366,7 @@ describe('UserUpdater', function() {
         err => {
           should.not.exist(err)
           this.NewsletterManager.changeEmail
-            .calledWith(this.stubbedUser.email, this.newEmail)
+            .calledWith(this.stubbedUser, this.newEmail)
             .should.equal(true)
           done()
         }