Merge pull request #3090 from overleaf/jel-async-setDefaultEmailAddress

Convert setDefaultEmailAddress to async

GitOrigin-RevId: 1f915af03c3dbe54b2cce439ecd55eeb3a3f35d3

services/web/app/src/Features/Subscription/RecurlyWrapper.js

@@ -16,7 +16,6 @@
  * DS207: Consider shorter variations of null checks
  * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
  */
-let RecurlyWrapper
 const OError = require('@overleaf/o-error')
 const querystring = require('querystring')
 const crypto = require('crypto')
@@ -27,8 +26,30 @@ const logger = require('logger-sharelatex')
 const Async = require('async')
 const Errors = require('../Errors/Errors')
 const SubscriptionErrors = require('./Errors')
+const { promisify } = require('util')
 
-module.exports = RecurlyWrapper = {
+function updateAccountEmailAddress(accountId, newEmail, callback) {
+  const data = {
+    email: newEmail
+  }
+  const requestBody = RecurlyWrapper._buildXml('account', data)
+
+  RecurlyWrapper.apiRequest(
+    {
+      url: `accounts/${accountId}`,
+      method: 'PUT',
+      body: requestBody
+    },
+    (error, response, body) => {
+      if (error != null) {
+        return callback(error)
+      }
+      RecurlyWrapper._parseAccountXml(body, callback)
+    }
+  )
+}
+
+const RecurlyWrapper = {
   apiUrl: Settings.apis.recurly.url || 'https://api.recurly.com/v2',
 
   _paypal: {
@@ -589,26 +610,7 @@ module.exports = RecurlyWrapper = {
     )
   },
 
-  updateAccountEmailAddress(accountId, newEmail, callback) {
-    const data = {
-      email: newEmail
-    }
-    const requestBody = RecurlyWrapper._buildXml('account', data)
-
-    RecurlyWrapper.apiRequest(
-      {
-        url: `accounts/${accountId}`,
-        method: 'PUT',
-        body: requestBody
-      },
-      (error, response, body) => {
-        if (error != null) {
-          return callback(error)
-        }
-        RecurlyWrapper._parseAccountXml(body, callback)
-      }
-    )
-  },
+  updateAccountEmailAddress,
 
   getAccountActiveCoupons(accountId, callback) {
     return RecurlyWrapper.apiRequest(
@@ -1055,6 +1057,12 @@ module.exports = RecurlyWrapper = {
   }
 }
 
+RecurlyWrapper.promises = {
+  updateAccountEmailAddress: promisify(updateAccountEmailAddress)
+}
+
+module.exports = RecurlyWrapper
+
 function getCustomFieldsFromSubscriptionDetails(subscriptionDetails) {
   if (!subscriptionDetails.ITMCampaign) {
     return null

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

@@ -121,7 +121,7 @@ const UserEmailsController = {
     if (!email) {
       return res.sendStatus(422)
     }
-    UserUpdater.setDefaultEmailAddress(userId, email, err => {
+    UserUpdater.setDefaultEmailAddress(userId, email, false, err => {
       if (err) {
         return UserEmailsController._handleEmailError(err, req, res, next)
       }

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

@@ -5,7 +5,7 @@ const metrics = require('metrics-sharelatex')
 const { db } = mongojs
 const async = require('async')
 const { ObjectId } = mongojs
-const { promisify } = require('util')
+const { callbackify, promisify } = require('util')
 const UserGetter = require('./UserGetter')
 const {
   addAffiliation,
@@ -18,6 +18,54 @@ const Errors = require('../Errors/Errors')
 const NewsletterManager = require('../Newsletter/NewsletterManager')
 const RecurlyWrapper = require('../Subscription/RecurlyWrapper')
 
+async function setDefaultEmailAddress(userId, email, allowUnconfirmed) {
+  email = EmailHelper.parseEmail(email)
+  if (email == null) {
+    throw new Error('invalid email')
+  }
+
+  const user = await UserGetter.promises.getUser(userId, {
+    email: 1,
+    emails: 1
+  })
+  if (!user) {
+    throw new Error('invalid userId')
+  }
+
+  const oldEmail = user.email
+  const userEmail = user.emails.find(e => e.email === email)
+  if (!userEmail) {
+    throw new Error('Default email does not belong to user')
+  }
+  if (!userEmail.confirmedAt && !allowUnconfirmed) {
+    throw new Errors.UnconfirmedEmailError()
+  }
+
+  const query = { _id: userId, 'emails.email': email }
+  const update = { $set: { email } }
+  const res = await UserUpdater.promises.updateUser(query, update)
+
+  // this should not happen
+  if (res.n === 0) {
+    throw new Error('email update error')
+  }
+
+  try {
+    await NewsletterManager.promises.changeEmail(user, email)
+  } catch (error) {
+    logger.warn(
+      { err: error, oldEmail, newEmail: email },
+      'Failed to change email in newsletter subscription'
+    )
+  }
+
+  try {
+    await RecurlyWrapper.promises.updateAccountEmailAddress(user._id, email)
+  } catch (error) {
+    // errors are ignored
+  }
+}
+
 const UserUpdater = {
   addAffiliationForNewUser(userId, email, callback) {
     addAffiliation(userId, email, error => {
@@ -166,55 +214,7 @@ const UserUpdater = {
 
   // set the default email address by setting the `email` attribute. The email
   // must be one of the user's multiple emails (`emails` attribute)
-  setDefaultEmailAddress(userId, email, allowUnconfirmed, callback) {
-    if (typeof allowUnconfirmed === 'function') {
-      callback = allowUnconfirmed
-      allowUnconfirmed = false
-    }
-    email = EmailHelper.parseEmail(email)
-    if (email == null) {
-      return callback(new Error('invalid email'))
-    }
-    UserGetter.getUser(userId, { email: 1, emails: 1 }, (err, user) => {
-      if (err) {
-        return callback(err)
-      }
-      if (!user) {
-        return callback(new Error('invalid userId'))
-      }
-      const oldEmail = user.email
-      const userEmail = user.emails.find(e => e.email === email)
-      if (!userEmail) {
-        return callback(new Error('Default email does not belong to user'))
-      }
-      if (!userEmail.confirmedAt && !allowUnconfirmed) {
-        return callback(new Errors.UnconfirmedEmailError())
-      }
-      const query = { _id: userId, 'emails.email': email }
-      const update = { $set: { email } }
-      UserUpdater.updateUser(query, update, (err, res) => {
-        if (err) {
-          return callback(err)
-        }
-        // this should not happen
-        if (res.n === 0) {
-          return callback(new Error('email update error'))
-        }
-        NewsletterManager.changeEmail(user, email, err => {
-          if (err != null) {
-            logger.warn(
-              { err, oldEmail, newEmail: email },
-              'Failed to change email in newsletter subscription'
-            )
-          }
-        })
-        RecurlyWrapper.updateAccountEmailAddress(user._id, email, _error => {
-          // errors are ignored
-        })
-        callback()
-      })
-    })
-  },
+  setDefaultEmailAddress: callbackify(setDefaultEmailAddress),
 
   confirmEmail(userId, email, confirmedAt, callback) {
     if (arguments.length === 3) {
@@ -285,6 +285,7 @@ const promises = {
   addAffiliationForNewUser: promisify(UserUpdater.addAffiliationForNewUser),
   addEmailAddress: promisify(UserUpdater.addEmailAddress),
   confirmEmail: promisify(UserUpdater.confirmEmail),
+  setDefaultEmailAddress,
   updateUser: promisify(UserUpdater.updateUser)
 }
 

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

@@ -21,7 +21,10 @@ describe('UserUpdater', function() {
     this.UserGetter = {
       getUserEmail: sinon.stub(),
       getUserByAnyEmail: sinon.stub(),
-      ensureUniqueEmailAddress: sinon.stub()
+      ensureUniqueEmailAddress: sinon.stub(),
+      promises: {
+        getUser: sinon.stub()
+      }
     }
     this.logger = {
       error: sinon.stub(),
@@ -31,8 +34,16 @@ describe('UserUpdater', function() {
     this.addAffiliation = sinon.stub().yields()
     this.removeAffiliation = sinon.stub().callsArgWith(2, null)
     this.refreshFeatures = sinon.stub().yields()
-    this.NewsletterManager = { changeEmail: sinon.stub() }
-    this.RecurlyWrapper = { updateAccountEmailAddress: sinon.stub() }
+    this.NewsletterManager = {
+      promises: {
+        changeEmail: sinon.stub()
+      }
+    }
+    this.RecurlyWrapper = {
+      promises: {
+        updateAccountEmailAddress: sinon.stub()
+      }
+    }
     this.UserUpdater = SandboxedModule.require(modulePath, {
       globals: {
         console: console
@@ -130,7 +141,7 @@ describe('UserUpdater', function() {
             .calledWith(this.stubbedUser._id, this.newEmail)
             .should.equal(true)
           this.UserUpdater.setDefaultEmailAddress
-            .calledWith(this.stubbedUser._id, this.newEmail)
+            .calledWith(this.stubbedUser._id, this.newEmail, true)
             .should.equal(true)
           this.UserUpdater.removeEmailAddress
             .calledWith(this.stubbedUser._id, this.stubbedUser.email)
@@ -345,20 +356,21 @@ describe('UserUpdater', function() {
           confirmedAt: new Date()
         }
       ]
-      this.UserGetter.getUser = sinon.stub().yields(null, this.stubbedUser)
-      this.NewsletterManager.changeEmail.callsArgWith(2, null)
-      this.RecurlyWrapper.updateAccountEmailAddress.yields(null)
+      this.UserGetter.promises.getUser.resolves(this.stubbedUser)
+      this.NewsletterManager.promises.changeEmail.callsArgWith(2, null)
+      this.RecurlyWrapper.promises.updateAccountEmailAddress.resolves()
     })
 
     it('set default', function(done) {
-      this.UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, { n: 1 })
+      this.UserUpdater.promises.updateUser = sinon.stub().resolves({ n: 1 })
 
       this.UserUpdater.setDefaultEmailAddress(
         this.stubbedUser._id,
         this.newEmail,
+        false,
         err => {
           should.not.exist(err)
-          this.UserUpdater.updateUser
+          this.UserUpdater.promises.updateUser
             .calledWith(
               { _id: this.stubbedUser._id, 'emails.email': this.newEmail },
               { $set: { email: this.newEmail } }
@@ -370,17 +382,18 @@ describe('UserUpdater', function() {
     })
 
     it('set changed the email in newsletter', function(done) {
-      this.UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, { n: 1 })
+      this.UserUpdater.promises.updateUser = sinon.stub().resolves({ n: 1 })
 
       this.UserUpdater.setDefaultEmailAddress(
         this.stubbedUser._id,
         this.newEmail,
+        false,
         err => {
           should.not.exist(err)
-          this.NewsletterManager.changeEmail
+          this.NewsletterManager.promises.changeEmail
             .calledWith(this.stubbedUser, this.newEmail)
             .should.equal(true)
-          this.RecurlyWrapper.updateAccountEmailAddress
+          this.RecurlyWrapper.promises.updateAccountEmailAddress
             .calledWith(this.stubbedUser._id, this.newEmail)
             .should.equal(true)
           done()
@@ -389,13 +402,12 @@ describe('UserUpdater', function() {
     })
 
     it('handle error', function(done) {
-      this.UserUpdater.updateUser = sinon
-        .stub()
-        .callsArgWith(2, new Error('nope'))
+      this.UserUpdater.promises.updateUser = sinon.stub().rejects(Error('nope'))
 
       this.UserUpdater.setDefaultEmailAddress(
         this.stubbedUser._id,
         this.newEmail,
+        false,
         err => {
           should.exist(err)
           done()
@@ -404,11 +416,12 @@ describe('UserUpdater', function() {
     })
 
     it('handle missed update', function(done) {
-      this.UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, { n: 0 })
+      this.UserUpdater.promises.updateUser = sinon.stub().resolves({ n: 0 })
 
       this.UserUpdater.setDefaultEmailAddress(
         this.stubbedUser._id,
         this.newEmail,
+        false,
         err => {
           should.exist(err)
           done()
@@ -420,6 +433,7 @@ describe('UserUpdater', function() {
       this.UserUpdater.setDefaultEmailAddress(
         this.stubbedUser._id,
         '.edu',
+        false,
         err => {
           should.exist(err)
           done()
@@ -435,42 +449,47 @@ describe('UserUpdater', function() {
             confirmedAt: null
           }
         ]
-        this.UserGetter.getUser = sinon.stub().yields(null, this.stubbedUser)
-        this.UserUpdater.updateUser = sinon.stub()
-        this.NewsletterManager.changeEmail = sinon.stub()
+        this.UserUpdater.promises.updateUser = sinon.stub()
       })
 
       it('should callback with error', function() {
         this.UserUpdater.setDefaultEmailAddress(
           this.stubbedUser._id,
           this.newEmail,
-          this.callback
-        )
-        this.callback.firstCall.args[0].name.should.equal(
-          'UnconfirmedEmailError'
+          false,
+          error => {
+            expect(error).to.exist
+            expect(error.name).to.equal('UnconfirmedEmailError')
+            this.UserUpdater.promises.updateUser.callCount.should.equal(0)
+            this.NewsletterManager.promises.changeEmail.callCount.should.equal(
+              0
+            )
+          }
         )
-        this.UserUpdater.updateUser.callCount.should.equal(0)
-        this.NewsletterManager.changeEmail.callCount.should.equal(0)
       })
     })
 
     describe('when email does not belong to user', function() {
       beforeEach(function() {
         this.stubbedUser.emails = []
-        this.UserGetter.getUser = sinon.stub().yields(null, this.stubbedUser)
-        this.UserUpdater.updateUser = sinon.stub()
-        this.NewsletterManager.changeEmail = sinon.stub()
+        this.UserGetter.promises.getUser.resolves(this.stubbedUser)
+        this.UserUpdater.promises.updateUser = sinon.stub()
       })
 
       it('should callback with error', function() {
         this.UserUpdater.setDefaultEmailAddress(
           this.stubbedUser._id,
           this.newEmail,
-          this.callback
+          false,
+          error => {
+            expect(error).to.exist
+            expect(error.name).to.equal('Error')
+            this.UserUpdater.promises.updateUser.callCount.should.equal(0)
+            this.NewsletterManager.promises.changeEmail.callCount.should.equal(
+              0
+            )
+          }
         )
-        this.callback.firstCall.args[0].name.should.equal('Error')
-        this.UserUpdater.updateUser.callCount.should.equal(0)
-        this.NewsletterManager.changeEmail.callCount.should.equal(0)
       })
     })
   })