Merge pull request #2089 from overleaf/em-mailchimp-unsubscribe

Handle error on Mailchimp unsubscribe when deleting users

GitOrigin-RevId: 8923480e6d50de45003fd7741610f995753a412b

services/web/app/src/Features/Institutions/InstitutionsAPI.js

@@ -11,14 +11,14 @@
  * DS207: Consider shorter variations of null checks
  * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
  */
-let InstitutionsAPI
 const logger = require('logger-sharelatex')
 const metrics = require('metrics-sharelatex')
 const settings = require('settings-sharelatex')
 const request = require('request')
+const { promisifyAll } = require('../../util/promises')
 const NotificationsBuilder = require('../Notifications/NotificationsBuilder')
 
-module.exports = InstitutionsAPI = {
+const InstitutionsAPI = {
   getInstitutionAffiliations(institutionId, callback) {
     if (callback == null) {
       callback = function(error, body) {}
@@ -223,3 +223,6 @@ var makeAffiliationRequest = function(requestOptions, callback) {
     logger
   )
 )
+
+InstitutionsAPI.promises = promisifyAll(InstitutionsAPI)
+module.exports = InstitutionsAPI

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

@@ -1,182 +1,170 @@
-/* eslint-disable
-    camelcase,
-    max-len,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * DS103: Rewrite code to no longer use __guard__
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-let mailchimp
-const async = require('async')
+const { callbackify } = require('util')
 const logger = require('logger-sharelatex')
 const Settings = require('settings-sharelatex')
 const crypto = require('crypto')
 const Mailchimp = require('mailchimp-api-v3')
+const OError = require('@overleaf/o-error')
 
-if (
-  (Settings.mailchimp != null ? Settings.mailchimp.api_key : undefined) == null
-) {
-  logger.info('Using newsletter provider: none')
-  mailchimp = {
-    request(opts, cb) {
-      return cb()
-    }
+const provider = getProvider()
+
+module.exports = {
+  subscribe: callbackify(provider.subscribe),
+  unsubscribe: callbackify(provider.unsubscribe),
+  changeEmail: callbackify(provider.changeEmail),
+  promises: provider
+}
+
+function getProvider() {
+  if (mailchimpIsConfigured()) {
+    logger.info('Using newsletter provider: mailchimp')
+    return makeMailchimpProvider()
+  } else {
+    logger.info('Using newsletter provider: none')
+    return makeNullProvider()
   }
-} else {
-  logger.info('Using newsletter provider: mailchimp')
-  mailchimp = new Mailchimp(
-    Settings.mailchimp != null ? Settings.mailchimp.api_key : undefined
-  )
 }
 
-module.exports = {
-  subscribe(user, callback) {
-    if (callback == null) {
-      callback = function() {}
-    }
-    const options = buildOptions(user, true)
-    logger.log(
-      { options, user, email: user.email },
-      'subscribing user to the mailing list'
-    )
-    return mailchimp.request(options, function(err) {
-      if (err != null) {
-        logger.warn({ err, user }, 'error subscribing person to newsletter')
-      } else {
-        logger.log({ user }, 'finished subscribing user to the newsletter')
-      }
-      return callback(err)
-    })
-  },
+function mailchimpIsConfigured() {
+  return Settings.mailchimp != null && Settings.mailchimp.api_key != null
+}
+
+function makeMailchimpProvider() {
+  const mailchimp = new Mailchimp(Settings.mailchimp.api_key)
+  const MAILCHIMP_LIST_ID = Settings.mailchimp.list_id
+
+  return {
+    subscribe,
+    unsubscribe,
+    changeEmail
+  }
 
-  unsubscribe(user, callback) {
-    if (callback == null) {
-      callback = function() {}
+  async function subscribe(user) {
+    try {
+      const path = getSubscriberPath(user.email)
+      await mailchimp.put(path, {
+        email_address: user.email,
+        status: 'subscribed',
+        status_if_new: 'subscribed',
+        merge_fields: getMergeFields(user)
+      })
+      logger.info({ user }, 'finished subscribing user to newsletter')
+    } catch (err) {
+      throw new OError({
+        message: 'error subscribing user to newsletter',
+        info: { userId: user._id }
+      }).withCause(err)
     }
-    logger.log(
-      { user, email: user.email },
-      'trying to unsubscribe user to the mailing list'
-    )
-    const options = buildOptions(user, false)
-    return mailchimp.request(options, function(err) {
-      if (err != null) {
-        logger.warn({ err, user }, 'error unsubscribing person to newsletter')
+  }
+
+  async function unsubscribe(user) {
+    try {
+      const path = getSubscriberPath(user.email)
+      await mailchimp.put(path, {
+        email_address: user.email,
+        status: 'unsubscribed',
+        status_if_new: 'unsubscribed',
+        merge_fields: getMergeFields(user)
+      })
+      logger.info({ user }, 'finished unsubscribing user from newsletter')
+    } catch (err) {
+      if (err.message.includes('looks fake or invalid')) {
+        logger.info(
+          { err, user },
+          'Mailchimp declined to unsubscribe user because it finds the email looks fake'
+        )
       } else {
-        logger.log({ user }, 'finished unsubscribing user to the newsletter')
+        throw new OError({
+          message: 'error unsubscribing user from newsletter',
+          info: { userId: user._id }
+        }).withCause(err)
       }
-      return callback(err)
-    })
-  },
-
-  changeEmail(oldEmail, newEmail, callback) {
-    if (callback == null) {
-      callback = function() {}
     }
-    const options = buildOptions({ email: oldEmail })
-    delete options.body.status
-    options.body.email_address = newEmail
-    logger.log({ oldEmail, newEmail, options }, 'changing email in newsletter')
-    return mailchimp.request(options, function(err) {
-      if (
-        err != null &&
-        __guard__(err != null ? err.message : undefined, x =>
-          x.indexOf('merge fields were invalid')
-        ) !== -1
-      ) {
-        logger.log(
+  }
+
+  async function changeEmail(oldEmail, newEmail) {
+    try {
+      const path = getSubscriberPath(oldEmail)
+      await mailchimp.put(path, {
+        email_address: newEmail,
+        status_if_new: 'unsubscribed'
+      })
+      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'
         )
-        return callback()
-      } else if (
-        err != null &&
-        __guard__(err != null ? err.message : undefined, x1 =>
-          x1.indexOf('could not be validated')
-        ) !== -1
-      ) {
-        logger.log(
+      } 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'
         )
-        return callback()
-      } else if (
-        err != null &&
-        err.message.indexOf('is already a list member') !== -1
-      ) {
-        logger.log(
+      } 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'
         )
-        return callback()
-      } else if (
-        err != null &&
-        __guard__(err != null ? err.message : undefined, x2 =>
-          x2.indexOf('looks fake or invalid')
-        ) !== -1
-      ) {
-        logger.log(
+      } else if (err.message.includes('looks fake or invalid')) {
+        logger.info(
           { oldEmail, newEmail },
           'unable to change email in newsletter, email looks fake to mailchimp'
         )
-        return callback()
-      } else if (err != null) {
-        logger.warn(
-          { err, oldEmail, newEmail },
-          'error changing email in newsletter'
-        )
-        return callback(err)
       } else {
-        logger.log('finished changing email in the newsletter')
-        return callback()
+        throw new OError({
+          message: 'error changing email in newsletter',
+          info: { oldEmail, newEmail }
+        }).withCause(err)
       }
-    })
+    }
   }
-}
 
-const hashEmail = email =>
-  crypto
-    .createHash('md5')
-    .update(email.toLowerCase())
-    .digest('hex')
-
-var buildOptions = function(user, is_subscribed) {
-  const subscriber_hash = hashEmail(user.email)
-  const status = is_subscribed ? 'subscribed' : 'unsubscribed'
-  const opts = {
-    method: 'PUT',
-    path: `/lists/${
-      Settings.mailchimp != null ? Settings.mailchimp.list_id : undefined
-    }/members/${subscriber_hash}`,
-    body: {
-      email_address: user.email,
-      status_if_new: status
-    }
+  function getSubscriberPath(email) {
+    const emailHash = hashEmail(email)
+    return `/lists/${MAILCHIMP_LIST_ID}/members/${emailHash}`
   }
 
-  // only set status if we explictly want to set it
-  if (is_subscribed != null) {
-    opts.body.status = status
+  function hashEmail(email) {
+    return crypto
+      .createHash('md5')
+      .update(email.toLowerCase())
+      .digest('hex')
   }
 
-  if (user._id != null) {
-    opts.body.merge_fields = {
+  function getMergeFields(user) {
+    return {
       FNAME: user.first_name,
       LNAME: user.last_name,
-      MONGO_ID: user._id
+      MONGO_ID: user._id.toString()
     }
   }
-
-  return opts
 }
 
-function __guard__(value, transform) {
-  return typeof value !== 'undefined' && value !== null
-    ? transform(value)
-    : undefined
+function makeNullProvider() {
+  return {
+    subscribe,
+    unsubscribe,
+    changeEmail
+  }
+
+  async function subscribe(user) {
+    logger.info(
+      { user },
+      'Not subscribing user to newsletter because no newsletter provider is configured'
+    )
+  }
+
+  async function unsubscribe(user) {
+    logger.info(
+      { user },
+      'Not unsubscribing user from newsletter because no newsletter provider is configured'
+    )
+  }
+
+  async function changeEmail(oldEmail, newEmail) {
+    logger.info(
+      { oldEmail, newEmail },
+      'Not changing email in newsletter for user because no newsletter provider is configured'
+    )
+  }
 }

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

@@ -17,6 +17,7 @@ const async = require('async')
 const RecurlyWrapper = require('./RecurlyWrapper')
 const Settings = require('settings-sharelatex')
 const { User } = require('../../models/User')
+const { promisifyAll } = require('../../util/promises')
 const logger = require('logger-sharelatex')
 const SubscriptionUpdater = require('./SubscriptionUpdater')
 const LimitationsManager = require('./LimitationsManager')
@@ -24,7 +25,7 @@ const EmailHandler = require('../Email/EmailHandler')
 const Events = require('../../infrastructure/Events')
 const Analytics = require('../Analytics/AnalyticsManager')
 
-module.exports = {
+const SubscriptionHandler = {
   validateNoSubscriptionInRecurly(user_id, callback) {
     if (callback == null) {
       callback = function(error, valid) {}
@@ -57,39 +58,38 @@ module.exports = {
   },
 
   createSubscription(user, subscriptionDetails, recurlyTokenIds, callback) {
-    const self = this
     const clientTokenId = ''
-    return this.validateNoSubscriptionInRecurly(user._id, function(
-      error,
-      valid
-    ) {
-      if (error != null) {
-        return callback(error)
-      }
-      if (!valid) {
-        return callback(new Error('user already has subscription in recurly'))
-      }
-      return RecurlyWrapper.createSubscription(
-        user,
-        subscriptionDetails,
-        recurlyTokenIds,
-        function(error, recurlySubscription) {
-          if (error != null) {
-            return callback(error)
-          }
-          return SubscriptionUpdater.syncSubscription(
-            recurlySubscription,
-            user._id,
-            function(error) {
-              if (error != null) {
-                return callback(error)
-              }
-              return callback()
-            }
-          )
+    return SubscriptionHandler.validateNoSubscriptionInRecurly(
+      user._id,
+      function(error, valid) {
+        if (error != null) {
+          return callback(error)
         }
-      )
-    })
+        if (!valid) {
+          return callback(new Error('user already has subscription in recurly'))
+        }
+        return RecurlyWrapper.createSubscription(
+          user,
+          subscriptionDetails,
+          recurlyTokenIds,
+          function(error, recurlySubscription) {
+            if (error != null) {
+              return callback(error)
+            }
+            return SubscriptionUpdater.syncSubscription(
+              recurlySubscription,
+              user._id,
+              function(error) {
+                if (error != null) {
+                  return callback(error)
+                }
+                return callback()
+              }
+            )
+          }
+        )
+      }
+    )
   },
 
   updateSubscription(user, plan_code, coupon_code, callback) {
@@ -245,3 +245,6 @@ module.exports = {
     )
   }
 }
+
+SubscriptionHandler.promises = promisifyAll(SubscriptionHandler)
+module.exports = SubscriptionHandler

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

@@ -12,14 +12,14 @@
  * DS207: Consider shorter variations of null checks
  * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
  */
-let SubscriptionLocator
+const { promisify } = require('util')
 const { Subscription } = require('../../models/Subscription')
 const logger = require('logger-sharelatex')
 const { ObjectId } = require('mongoose').Types
 
-module.exports = SubscriptionLocator = {
+const SubscriptionLocator = {
   getUsersSubscription(user_or_id, callback) {
-    const user_id = this._getUserId(user_or_id)
+    const user_id = SubscriptionLocator._getUserId(user_or_id)
     logger.log({ user_id }, 'getting users subscription')
     return Subscription.findOne({ admin_id: user_id }, function(
       err,
@@ -39,7 +39,7 @@ module.exports = SubscriptionLocator = {
     if (callback == null) {
       callback = function(error, managedSubscriptions) {}
     }
-    const user_id = this._getUserId(user_or_id)
+    const user_id = SubscriptionLocator._getUserId(user_or_id)
     return Subscription.find({
       manager_ids: user_or_id,
       groupPlan: true
@@ -49,7 +49,7 @@ module.exports = SubscriptionLocator = {
   },
 
   getMemberSubscriptions(user_or_id, callback) {
-    const user_id = this._getUserId(user_or_id)
+    const user_id = SubscriptionLocator._getUserId(user_or_id)
     logger.log({ user_id }, 'getting users group subscriptions')
     return Subscription.find({ member_ids: user_id })
       .populate('admin_id')
@@ -92,3 +92,26 @@ module.exports = SubscriptionLocator = {
     }
   }
 }
+
+SubscriptionLocator.promises = {
+  getUsersSubscription: promisify(SubscriptionLocator.getUsersSubscription),
+  findManagedSubscription: promisify(
+    SubscriptionLocator.findManagedSubscription
+  ),
+  getManagedGroupSubscriptions: promisify(
+    SubscriptionLocator.getManagedGroupSubscriptions
+  ),
+  getMemberSubscriptions: promisify(SubscriptionLocator.getMemberSubscriptions),
+  getSubscription: promisify(SubscriptionLocator.getSubscription),
+  getSubscriptionByMemberIdAndId: promisify(
+    SubscriptionLocator.getSubscriptionByMemberIdAndId
+  ),
+  getGroupSubscriptionsMemberOf: promisify(
+    SubscriptionLocator.getGroupSubscriptionsMemberOf
+  ),
+  getGroupsWithEmailInvite: promisify(
+    SubscriptionLocator.getGroupsWithEmailInvite
+  ),
+  getGroupWithV1Id: promisify(SubscriptionLocator.getGroupWithV1Id)
+}
+module.exports = SubscriptionLocator

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

@@ -1,5 +1,6 @@
 const async = require('async')
 const _ = require('underscore')
+const { promisifyAll } = require('../../util/promises')
 const { Subscription } = require('../../models/Subscription')
 const SubscriptionLocator = require('./SubscriptionLocator')
 const UserGetter = require('../User/UserGetter')
@@ -77,11 +78,11 @@ const SubscriptionUpdater = {
   },
 
   addUserToGroup(subscriptionId, userId, callback) {
-    this.addUsersToGroup(subscriptionId, [userId], callback)
+    SubscriptionUpdater.addUsersToGroup(subscriptionId, [userId], callback)
   },
 
   addUsersToGroup(subscriptionId, memberIds, callback) {
-    this.addUsersToGroupWithoutFeaturesRefresh(
+    SubscriptionUpdater.addUsersToGroupWithoutFeaturesRefresh(
       subscriptionId,
       memberIds,
       function(err) {
@@ -238,4 +239,5 @@ const SubscriptionUpdater = {
   }
 }
 
+SubscriptionUpdater.promises = promisifyAll(SubscriptionUpdater)
 module.exports = SubscriptionUpdater

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

@@ -1,119 +1,114 @@
-/* eslint-disable
-    camelcase,
-    handle-callback-err,
-    max-len,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-let UserController
 const UserHandler = require('./UserHandler')
 const UserDeleter = require('./UserDeleter')
 const UserGetter = require('./UserGetter')
 const { User } = require('../../models/User')
-const newsLetterManager = require('../Newsletter/NewsletterManager')
+const NewsletterManager = require('../Newsletter/NewsletterManager')
 const UserRegistrationHandler = require('./UserRegistrationHandler')
 const logger = require('logger-sharelatex')
 const metrics = require('metrics-sharelatex')
-const Url = require('url')
 const AuthenticationManager = require('../Authentication/AuthenticationManager')
 const AuthenticationController = require('../Authentication/AuthenticationController')
 const UserSessionsManager = require('./UserSessionsManager')
 const UserUpdater = require('./UserUpdater')
 const SudoModeHandler = require('../SudoMode/SudoModeHandler')
-const settings = require('settings-sharelatex')
 const Errors = require('../Errors/Errors')
 const OError = require('@overleaf/o-error')
 const HttpErrors = require('@overleaf/o-error/http')
 const EmailHandler = require('../Email/EmailHandler')
 
-module.exports = UserController = {
+const UserController = {
   tryDeleteUser(req, res, next) {
-    const user_id = AuthenticationController.getLoggedInUserId(req)
+    const userId = AuthenticationController.getLoggedInUserId(req)
     const { password } = req.body
-    logger.log({ user_id }, 'trying to delete user account')
+    logger.log({ userId }, 'trying to delete user account')
     if (password == null || password === '') {
       logger.err(
-        { user_id },
+        { userId },
         'no password supplied for attempt to delete account'
       )
       return res.sendStatus(403)
     }
-    AuthenticationManager.authenticate({ _id: user_id }, password, function(
-      err,
-      user
-    ) {
-      if (err != null) {
-        logger.warn(
-          { user_id },
-          'error authenticating during attempt to delete account'
-        )
-        return next(err)
-      }
-      if (!user) {
-        logger.err({ user_id }, 'auth failed during attempt to delete account')
-        return res.sendStatus(403)
-      }
-      UserDeleter.deleteUser(
-        user_id,
-        { deleterUser: user, ipAddress: req.ip },
-        function(err) {
-          if (err) {
-            let errorData = {
-              message: 'error while deleting user account',
-              info: { user_id }
-            }
-            if (err instanceof Errors.SubscriptionAdminDeletionError) {
-              // set info.public.error for JSON response so frontend can display
-              // a specific message
-              errorData.info.public = {
-                error: 'SubscriptionAdminDeletionError'
+    AuthenticationManager.authenticate(
+      { _id: userId },
+      password,
+      (err, user) => {
+        if (err != null) {
+          logger.warn(
+            { userId },
+            'error authenticating during attempt to delete account'
+          )
+          return next(err)
+        }
+        if (!user) {
+          logger.err({ userId }, 'auth failed during attempt to delete account')
+          return res.sendStatus(403)
+        }
+        UserDeleter.deleteUser(
+          userId,
+          { deleterUser: user, ipAddress: req.ip },
+          err => {
+            if (err) {
+              let errorData = {
+                message: 'error while deleting user account',
+                info: { userId }
               }
-              return next(
-                new HttpErrors.UnprocessableEntityError(errorData).withCause(
-                  err
+              if (err instanceof Errors.SubscriptionAdminDeletionError) {
+                // set info.public.error for JSON response so frontend can display
+                // a specific message
+                errorData.info.public = {
+                  error: 'SubscriptionAdminDeletionError'
+                }
+                return next(
+                  new HttpErrors.UnprocessableEntityError(errorData).withCause(
+                    err
+                  )
                 )
-              )
-            } else {
-              return next(new OError(errorData).withCause(err))
+              } else {
+                return next(new OError(errorData).withCause(err))
+              }
             }
-          }
-          const sessionId = req.sessionID
-          if (typeof req.logout === 'function') {
-            req.logout()
-          }
-          req.session.destroy(function(err) {
-            if (err != null) {
-              logger.warn({ err }, 'error destorying session')
-              return next(err)
+            const sessionId = req.sessionID
+            if (typeof req.logout === 'function') {
+              req.logout()
             }
-            UserSessionsManager.untrackSession(user, sessionId)
-            return res.sendStatus(200)
-          })
-        }
-      )
-    })
+            req.session.destroy(err => {
+              if (err != null) {
+                logger.warn({ err }, 'error destorying session')
+                return next(err)
+              }
+              UserSessionsManager.untrackSession(user, sessionId)
+              res.sendStatus(200)
+            })
+          }
+        )
+      }
+    )
   },
 
-  unsubscribe(req, res) {
-    const user_id = AuthenticationController.getLoggedInUserId(req)
-    return UserGetter.getUser(user_id, (err, user) =>
-      newsLetterManager.unsubscribe(user, () => res.send())
-    )
+  unsubscribe(req, res, next) {
+    const userId = AuthenticationController.getLoggedInUserId(req)
+    UserGetter.getUser(userId, (err, user) => {
+      if (err != null) {
+        return next(err)
+      }
+      NewsletterManager.unsubscribe(user, err => {
+        if (err != null) {
+          logger.warn(
+            { err, user },
+            'Failed to unsubscribe user from newsletter'
+          )
+        }
+        res.send()
+      })
+    })
   },
 
   updateUserSettings(req, res, next) {
-    const user_id = AuthenticationController.getLoggedInUserId(req)
-    logger.log({ user_id }, 'updating account settings')
-    return User.findById(user_id, function(err, user) {
+    const userId = AuthenticationController.getLoggedInUserId(req)
+    logger.log({ userId }, 'updating account settings')
+    User.findById(userId, (err, user) => {
       if (err != null || user == null) {
-        logger.err({ err, user_id }, 'problem updaing user settings')
+        logger.err({ err, userId }, 'problem updaing user settings')
         return res.sendStatus(500)
       }
 
@@ -163,7 +158,10 @@ module.exports = UserController = {
         user.ace.lineHeight = req.body.lineHeight
       }
 
-      return user.save(function(err) {
+      user.save(err => {
+        if (err != null) {
+          return next(err)
+        }
         const newEmail =
           req.body.email != null
             ? req.body.email.trim().toLowerCase()
@@ -178,19 +176,17 @@ module.exports = UserController = {
             first_name: user.first_name,
             last_name: user.last_name
           })
-          return res.sendStatus(200)
+          res.sendStatus(200)
         } else if (newEmail.indexOf('@') === -1) {
           // email invalid
-          return res.sendStatus(400)
+          res.sendStatus(400)
         } else {
           // update the user email
-          return UserUpdater.changeEmailAddress(user_id, newEmail, function(
-            err
-          ) {
+          UserUpdater.changeEmailAddress(userId, newEmail, err => {
             if (err) {
               let errorData = {
                 message: 'problem updaing users email address',
-                info: { user_id, newEmail, public: {} }
+                info: { userId, newEmail, public: {} }
               }
               if (err instanceof Errors.EmailExistsError) {
                 errorData.info.public.message = req.i18n.translate(
@@ -208,10 +204,10 @@ module.exports = UserController = {
                 )
               }
             }
-            return User.findById(user_id, function(err, user) {
+            User.findById(userId, (err, user) => {
               if (err != null) {
                 logger.err(
-                  { err, user_id },
+                  { err, userId },
                   'error getting user for email update'
                 )
                 return res.send(500)
@@ -221,12 +217,12 @@ module.exports = UserController = {
                 first_name: user.first_name,
                 last_name: user.last_name
               })
-              return UserHandler.populateTeamInvites(user, function(err) {
+              UserHandler.populateTeamInvites(user, err => {
                 // need to refresh this in the background
                 if (err != null) {
                   logger.err({ err }, 'error populateTeamInvites')
                 }
-                return res.sendStatus(200)
+                res.sendStatus(200)
               })
             })
           })
@@ -236,9 +232,6 @@ module.exports = UserController = {
   },
 
   _doLogout(req, cb) {
-    if (cb == null) {
-      cb = function(err) {}
-    }
     metrics.inc('user.logout')
     const user = AuthenticationController.getSessionUser(req)
     logger.log({ user }, 'logging out')
@@ -246,26 +239,26 @@ module.exports = UserController = {
     if (typeof req.logout === 'function') {
       req.logout()
     } // passport logout
-    return req.session.destroy(function(err) {
+    req.session.destroy(err => {
       if (err) {
         logger.warn({ err }, 'error destorying session')
-        cb(err)
+        return cb(err)
       }
       if (user != null) {
         UserSessionsManager.untrackSession(user, sessionId)
         SudoModeHandler.clearSudoMode(user._id)
       }
-      return cb()
+      cb()
     })
   },
 
   logout(req, res, next) {
-    return UserController._doLogout(req, function(err) {
+    UserController._doLogout(req, err => {
       if (err != null) {
         return next(err)
       }
-      const redirect_url = '/login'
-      return res.redirect(redirect_url)
+      const redirectUrl = '/login'
+      res.redirect(redirectUrl)
     })
   },
 
@@ -291,21 +284,17 @@ module.exports = UserController = {
   },
 
   register(req, res, next) {
-    if (next == null) {
-      next = function(error) {}
-    }
     const { email } = req.body
     if (email == null || email === '') {
-      res.sendStatus(422) // Unprocessable Entity
-      return
+      return res.sendStatus(422) // Unprocessable Entity
     }
-    return UserRegistrationHandler.registerNewUserAndSendActivationEmail(
+    UserRegistrationHandler.registerNewUserAndSendActivationEmail(
       email,
-      function(error, user, setNewPasswordUrl) {
+      (error, user, setNewPasswordUrl) => {
         if (error != null) {
           return next(error)
         }
-        return res.json({
+        res.json({
           email: user.email,
           setNewPasswordUrl
         })
@@ -314,22 +303,15 @@ module.exports = UserController = {
   },
 
   clearSessions(req, res, next) {
-    if (next == null) {
-      next = function(error) {}
-    }
     metrics.inc('user.clear-sessions')
     const user = AuthenticationController.getSessionUser(req)
-    logger.log({ user_id: user._id }, 'clearing sessions for user')
-    return UserSessionsManager.revokeAllUserSessions(
-      user,
-      [req.sessionID],
-      function(err) {
-        if (err != null) {
-          return next(err)
-        }
-        return res.sendStatus(201)
+    logger.log({ userId: user._id }, 'clearing sessions for user')
+    UserSessionsManager.revokeAllUserSessions(user, [req.sessionID], err => {
+      if (err != null) {
+        return next(err)
       }
-    )
+      res.sendStatus(201)
+    })
   },
 
   changePassword(req, res, next) {
@@ -337,9 +319,9 @@ module.exports = UserController = {
     const internalError = {
       message: { type: 'error', text: req.i18n.translate('internal_error') }
     }
-    const user_id = AuthenticationController.getLoggedInUserId(req)
+    const userId = AuthenticationController.getLoggedInUserId(req)
     AuthenticationManager.authenticate(
-      { _id: user_id },
+      { _id: userId },
       req.body.currentPassword,
       (err, user) => {
         if (err) {
@@ -411,3 +393,5 @@ module.exports = UserController = {
     )
   }
 }
+
+module.exports = UserController

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

@@ -1,20 +1,18 @@
+const { callbackify } = require('util')
+const logger = require('logger-sharelatex')
+const moment = require('moment')
 const { User } = require('../../models/User')
 const { DeletedUser } = require('../../models/DeletedUser')
 const NewsletterManager = require('../Newsletter/NewsletterManager')
-const ProjectDeleterPromises = require('../Project/ProjectDeleter').promises
-const logger = require('logger-sharelatex')
-const moment = require('moment')
+const ProjectDeleter = require('../Project/ProjectDeleter')
 const SubscriptionHandler = require('../Subscription/SubscriptionHandler')
 const SubscriptionUpdater = require('../Subscription/SubscriptionUpdater')
 const SubscriptionLocator = require('../Subscription/SubscriptionLocator')
 const UserMembershipsHandler = require('../UserMembership/UserMembershipsHandler')
-const async = require('async')
 const InstitutionsAPI = require('../Institutions/InstitutionsAPI')
 const Errors = require('../Errors/Errors')
-const { promisify, callbackify } = require('util')
 
-let UserDeleter
-module.exports = UserDeleter = {
+module.exports = {
   deleteUser: callbackify(deleteUser),
   expireDeletedUser: callbackify(expireDeletedUser),
   ensureCanDeleteUser: callbackify(ensureCanDeleteUser),
@@ -38,10 +36,10 @@ async function deleteUser(userId, options = {}) {
     let user = await User.findById(userId).exec()
     logger.log({ user }, 'deleting user')
 
-    await UserDeleter.promises.ensureCanDeleteUser(user)
-    await _createDeletedUser(user, options)
+    await ensureCanDeleteUser(user)
     await _cleanupUser(user)
-    await ProjectDeleterPromises.deleteUsersProjects(user._id)
+    await _createDeletedUser(user, options)
+    await ProjectDeleter.promises.deleteUsersProjects(user._id)
     await User.deleteOne({ _id: userId }).exec()
   } catch (error) {
     logger.warn({ error, userId }, 'something went wrong deleting the user')
@@ -76,26 +74,17 @@ async function expireDeletedUsersAfterDuration() {
   }
 
   for (let i = 0; i < deletedUsers.length; i++) {
-    await UserDeleter.promises.expireDeletedUser(
-      deletedUsers[i].deleterData.deletedUserId
-    )
+    await expireDeletedUser(deletedUsers[i].deleterData.deletedUserId)
   }
 }
 
 async function ensureCanDeleteUser(user) {
-  await new Promise((resolve, reject) => {
-    SubscriptionLocator.getUsersSubscription(user, (error, subscription) => {
-      if (error) {
-        return reject(error)
-      }
-
-      if (subscription) {
-        return reject(new Errors.SubscriptionAdminDeletionError({}))
-      }
-
-      resolve()
-    })
-  })
+  const subscription = await SubscriptionLocator.promises.getUsersSubscription(
+    user
+  )
+  if (subscription) {
+    throw new Errors.SubscriptionAdminDeletionError({})
+  }
 }
 
 async function _createDeletedUser(user, options) {
@@ -121,21 +110,9 @@ async function _cleanupUser(user) {
   if (user == null) {
     throw new Error('no user supplied')
   }
-
-  const runInSeries = promisify(async.series)
-
-  await runInSeries([
-    cb =>
-      NewsletterManager.unsubscribe(user, err => {
-        logger.err('Failed to unsubscribe user from newsletter', {
-          user_id: user._id,
-          error: err
-        })
-        cb()
-      }),
-    cb => SubscriptionHandler.cancelSubscription(user, cb),
-    cb => InstitutionsAPI.deleteAffiliations(user._id, cb),
-    cb => SubscriptionUpdater.removeUserFromAllGroups(user._id, cb),
-    cb => UserMembershipsHandler.removeUserFromAllEntities(user._id, cb)
-  ])
+  await NewsletterManager.promises.unsubscribe(user)
+  await SubscriptionHandler.promises.cancelSubscription(user)
+  await InstitutionsAPI.promises.deleteAffiliations(user._id)
+  await SubscriptionUpdater.promises.removeUserFromAllGroups(user._id)
+  await UserMembershipsHandler.promises.removeUserFromAllEntities(user._id)
 }

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

@@ -1,21 +1,8 @@
-/* eslint-disable
-    handle-callback-err,
-    max-len,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-let UserRegistrationHandler
 const { User } = require('../../models/User')
 const UserCreator = require('./UserCreator')
 const UserGetter = require('./UserGetter')
 const AuthenticationManager = require('../Authentication/AuthenticationManager')
-const NewsLetterManager = require('../Newsletter/NewsletterManager')
+const NewsletterManager = require('../Newsletter/NewsletterManager')
 const async = require('async')
 const logger = require('logger-sharelatex')
 const crypto = require('crypto')
@@ -25,7 +12,7 @@ const Analytics = require('../Analytics/AnalyticsManager')
 const settings = require('settings-sharelatex')
 const EmailHelper = require('../Helpers/EmailHelper')
 
-module.exports = UserRegistrationHandler = {
+const UserRegistrationHandler = {
   _registrationRequestIsValid(body, callback) {
     const invalidEmail = AuthenticationManager.validateEmail(body.email || '')
     const invalidPassword = AuthenticationManager.validatePassword(
@@ -41,7 +28,7 @@ module.exports = UserRegistrationHandler = {
   _createNewUserIfRequired(user, userDetails, callback) {
     if (user == null) {
       userDetails.holdingAccount = false
-      return UserCreator.createNewUser(
+      UserCreator.createNewUser(
         {
           holdingAccount: false,
           email: userDetails.email,
@@ -51,7 +38,7 @@ module.exports = UserRegistrationHandler = {
         callback
       )
     } else {
-      return callback(null, user)
+      callback(null, user)
     }
   },
 
@@ -62,21 +49,18 @@ module.exports = UserRegistrationHandler = {
       return callback(new Error('request is not valid'))
     }
     userDetails.email = EmailHelper.parseEmail(userDetails.email)
-    return UserGetter.getUserByAnyEmail(userDetails.email, (err, user) => {
+    UserGetter.getUserByAnyEmail(userDetails.email, (err, user) => {
       if (err != null) {
         return callback(err)
       }
       if ((user != null ? user.holdingAccount : undefined) === false) {
         return callback(new Error('EmailAlreadyRegistered'), user)
       }
-      return self._createNewUserIfRequired(user, userDetails, function(
-        err,
-        user
-      ) {
+      self._createNewUserIfRequired(user, userDetails, (err, user) => {
         if (err != null) {
           return callback(err)
         }
-        return async.series(
+        async.series(
           [
             cb =>
               User.update(
@@ -90,17 +74,24 @@ module.exports = UserRegistrationHandler = {
                 userDetails.password,
                 cb
               ),
-            function(cb) {
+            cb => {
               if (userDetails.subscribeToNewsletter === 'true') {
-                NewsLetterManager.subscribe(user, function() {})
+                NewsletterManager.subscribe(user, err => {
+                  if (err != null) {
+                    logger.warn(
+                      { err, user },
+                      'Failed to subscribe user to newsletter'
+                    )
+                  }
+                })
               }
-              return cb()
+              cb()
             } // this can be slow, just fire it off
           ],
-          function(err) {
+          err => {
             logger.log({ user }, 'registered')
             Analytics.recordEvent(user._id, 'user-registered')
-            return callback(err, user)
+            callback(err, user)
           }
         )
       })
@@ -108,16 +99,13 @@ module.exports = UserRegistrationHandler = {
   },
 
   registerNewUserAndSendActivationEmail(email, callback) {
-    if (callback == null) {
-      callback = function(error, user, setNewPasswordUrl) {}
-    }
     logger.log({ email }, 'registering new user')
-    return UserRegistrationHandler.registerNewUser(
+    UserRegistrationHandler.registerNewUser(
       {
         email,
         password: crypto.randomBytes(32).toString('hex')
       },
-      function(err, user) {
+      (err, user) => {
         if (
           err != null &&
           (err != null ? err.message : undefined) !== 'EmailAlreadyRegistered'
@@ -132,11 +120,11 @@ module.exports = UserRegistrationHandler = {
         }
 
         const ONE_WEEK = 7 * 24 * 60 * 60 // seconds
-        return OneTimeTokenHandler.getNewToken(
+        OneTimeTokenHandler.getNewToken(
           'password',
           user._id,
           { expiresIn: ONE_WEEK },
-          function(err, token) {
+          (err, token) => {
             if (err != null) {
               return callback(err)
             }
@@ -151,13 +139,15 @@ module.exports = UserRegistrationHandler = {
                 to: user.email,
                 setNewPasswordUrl
               },
-              function() {}
+              () => {}
             )
 
-            return callback(null, user, setNewPasswordUrl)
+            callback(null, user, setNewPasswordUrl)
           }
         )
       }
     )
   }
 }
+
+module.exports = UserRegistrationHandler

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

@@ -1,19 +1,3 @@
-/* eslint-disable
-    camelcase,
-    handle-callback-err,
-    max-len,
-    no-undef,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS101: Remove unnecessary use of Array.from
- * DS102: Remove unnecessary code created because of implicit returns
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-let UserUpdater
 const logger = require('logger-sharelatex')
 const mongojs = require('../../infrastructure/mongojs')
 const metrics = require('metrics-sharelatex')
@@ -30,10 +14,10 @@ const EmailHelper = require('../Helpers/EmailHelper')
 const Errors = require('../Errors/Errors')
 const NewsletterManager = require('../Newsletter/NewsletterManager')
 
-module.exports = UserUpdater = {
+const UserUpdater = {
   updateUser(query, update, callback) {
     if (callback == null) {
-      callback = function(error) {}
+      callback = () => {}
     }
     if (typeof query === 'string') {
       query = { _id: ObjectId(query) }
@@ -43,7 +27,7 @@ module.exports = UserUpdater = {
       query._id = ObjectId(query._id)
     }
 
-    return db.users.update(query, update, callback)
+    db.users.update(query, update, callback)
   },
 
   //
@@ -61,12 +45,12 @@ module.exports = UserUpdater = {
     logger.log({ userId, newEmail }, 'updaing email address of user')
 
     let oldEmail = null
-    return async.series(
+    async.series(
       [
         cb =>
-          UserGetter.getUserEmail(userId, function(error, email) {
+          UserGetter.getUserEmail(userId, (error, email) => {
             oldEmail = email
-            return cb(error)
+            cb(error)
           }),
         cb => UserUpdater.addEmailAddress(userId, newEmail, cb),
         cb => UserUpdater.setDefaultEmailAddress(userId, newEmail, true, cb),
@@ -89,12 +73,12 @@ module.exports = UserUpdater = {
       return callback(new Error('invalid email'))
     }
 
-    return UserGetter.ensureUniqueEmailAddress(newEmail, error => {
+    UserGetter.ensureUniqueEmailAddress(newEmail, error => {
       if (error != null) {
         return callback(error)
       }
 
-      return addAffiliation(userId, newEmail, affiliationOptions, error => {
+      addAffiliation(userId, newEmail, affiliationOptions, error => {
         if (error != null) {
           logger.warn(
             { error },
@@ -113,12 +97,12 @@ module.exports = UserUpdater = {
             emails: { email: newEmail, createdAt: new Date(), reversedHostname }
           }
         }
-        return UserUpdater.updateUser(userId, update, function(error) {
+        UserUpdater.updateUser(userId, update, error => {
           if (error != null) {
             logger.warn({ error }, 'problem updating users emails')
             return callback(error)
           }
-          return callback()
+          callback()
         })
       })
     })
@@ -131,7 +115,7 @@ module.exports = UserUpdater = {
     if (email == null) {
       return callback(new Error('invalid email'))
     }
-    return removeAffiliation(userId, email, error => {
+    removeAffiliation(userId, email, error => {
       if (error != null) {
         logger.warn({ error }, 'problem removing affiliation')
         return callback(error)
@@ -139,7 +123,7 @@ module.exports = UserUpdater = {
 
       const query = { _id: userId, email: { $ne: email } }
       const update = { $pull: { emails: { email } } }
-      return UserUpdater.updateUser(query, update, function(error, res) {
+      UserUpdater.updateUser(query, update, (error, res) => {
         if (error != null) {
           logger.warn({ error }, 'problem removing users email')
           return callback(error)
@@ -147,7 +131,7 @@ module.exports = UserUpdater = {
         if (res.n === 0) {
           return callback(new Error('Cannot remove email'))
         }
-        return callback()
+        callback()
       })
     })
   },
@@ -188,8 +172,14 @@ module.exports = UserUpdater = {
         if (res.n === 0) {
           return callback(new Error('email update error'))
         }
-        // do not wait - this will log its own errors
-        NewsletterManager.changeEmail(oldEmail, email, () => {})
+        NewsletterManager.changeEmail(oldEmail, email, err => {
+          if (err != null) {
+            logger.warn(
+              { err, oldEmail, newEmail: email },
+              'Failed to change email in newsletter subscription'
+            )
+          }
+        })
         callback()
       })
     })
@@ -205,7 +195,7 @@ module.exports = UserUpdater = {
       return callback(new Error('invalid email'))
     }
     logger.log({ userId, email }, 'confirming user email')
-    return addAffiliation(userId, email, { confirmedAt }, error => {
+    addAffiliation(userId, email, { confirmedAt }, error => {
       if (error != null) {
         logger.warn(
           { error },
@@ -223,7 +213,7 @@ module.exports = UserUpdater = {
           'emails.$.confirmedAt': confirmedAt
         }
       }
-      return UserUpdater.updateUser(query, update, function(error, res) {
+      UserUpdater.updateUser(query, update, (error, res) => {
         if (error != null) {
           return callback(error)
         }
@@ -233,14 +223,14 @@ module.exports = UserUpdater = {
             new Errors.NotFoundError('user id and email do no match')
           )
         }
-        return FeaturesUpdater.refreshFeatures(userId, callback)
+        FeaturesUpdater.refreshFeatures(userId, callback)
       })
     })
   },
 
-  removeReconfirmFlag(user_id, callback) {
-    return UserUpdater.updateUser(
-      user_id.toString(),
+  removeReconfirmFlag(userId, callback) {
+    UserUpdater.updateUser(
+      userId.toString(),
       {
         $set: { must_reconfirm: false }
       },
@@ -258,3 +248,5 @@ module.exports = UserUpdater = {
 ].map(method =>
   metrics.timeAsyncMethod(UserUpdater, method, 'mongo.UserUpdater', logger)
 )
+
+module.exports = UserUpdater

services/web/app/src/Features/UserMembership/UserMembershipsHandler.js

@@ -10,8 +10,8 @@
  * DS207: Consider shorter variations of null checks
  * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
  */
-let UserMembershipsHandler
 const async = require('async')
+const { promisifyAll } = require('../../util/promises')
 const EntityModels = {
   Institution: require('../../models/Institution').Institution,
   Subscription: require('../../models/Subscription').Subscription,
@@ -19,7 +19,7 @@ const EntityModels = {
 }
 const UserMembershipEntityConfigs = require('./UserMembershipEntityConfigs')
 
-module.exports = UserMembershipsHandler = {
+const UserMembershipsHandler = {
   removeUserFromAllEntities(userId, callback) {
     // get all writable entity types
     if (callback == null) {
@@ -83,3 +83,6 @@ module.exports = UserMembershipsHandler = {
     })
   }
 }
+
+UserMembershipsHandler.promises = promisifyAll(UserMembershipsHandler)
+module.exports = UserMembershipsHandler

services/web/app/src/util/promises.js

@@ -0,0 +1,31 @@
+const { promisify } = require('util')
+
+module.exports = { promisifyAll }
+
+/**
+ * Promisify all functions in a module.
+ *
+ * This is meant to be used only when all functions in the module are async
+ * callback-style functions.
+ *
+ * It's very much tailored to our current module structure. In particular, it
+ * binds `this` to the module when calling the function in order not to break
+ * modules that call sibling functions using `this`.
+ *
+ * This will not magically fix all modules. Special cases should be promisified
+ * manually.
+ */
+function promisifyAll(module, opts = {}) {
+  const { without = [] } = opts
+  const promises = {}
+  for (const propName of Object.getOwnPropertyNames(module)) {
+    if (without.includes(propName)) {
+      continue
+    }
+    const propValue = module[propName]
+    if (typeof propValue === 'function') {
+      promises[propName] = promisify(propValue).bind(module)
+    }
+  }
+  return promises
+}

services/web/app/views/user/settings.pug

@@ -268,6 +268,8 @@ block content
 						| #{translate('email_or_password_wrong_try_again')}
 					span(ng-switch-when="SubscriptionAdminDeletionError")
 						| #{translate('subscription_admins_cannot_be_deleted')}
+					span(ng-switch-when="UserDeletionError")
+						| #{translate('user_deletion_error')}
 					span(ng-switch-default)
 						| #{translate('generic_something_went_wrong')}
 			if settings.createV1AccountOnLogin && settings.overleaf

services/web/package-lock.json

@@ -4184,6 +4184,14 @@
         "type-detect": "^1.0.0"
       }
     },
+    "chai-as-promised": {
+      "version": "7.1.1",
+      "resolved": "https://registry.npmjs.org/chai-as-promised/-/chai-as-promised-7.1.1.tgz",
+      "integrity": "sha512-azL6xMoi+uxu6z4rhWQ1jbdUhOMhis2PvscD/xjLqNMkv3BPPp2JyyuTHOrf9BOosGpNQ11v6BKv/g57RXbiaA==",
+      "requires": {
+        "check-error": "^1.0.2"
+      }
+    },
     "chalk": {
       "version": "1.1.3",
       "resolved": "https://registry.npmjs.org/chalk/-/chalk-1.1.3.tgz",
@@ -4211,6 +4219,11 @@
       "integrity": "sha512-j/Toj7f1z98Hh2cYo2BVr85EpIRWqUi7rtRSGxh/cqUjqrnJe9l9UE7IUGd2vQ2p+kSHLkSzObQPZPLUC6TQwg==",
       "dev": true
     },
+    "check-error": {
+      "version": "1.0.2",
+      "resolved": "https://registry.npmjs.org/check-error/-/check-error-1.0.2.tgz",
+      "integrity": "sha1-V00xLt2Iu13YkS6Sht1sCu1KrII="
+    },
     "chokidar": {
       "version": "1.7.0",
       "resolved": "https://registry.npmjs.org/chokidar/-/chokidar-1.7.0.tgz",
@@ -7696,11 +7709,13 @@
         },
         "balanced-match": {
           "version": "1.0.0",
-          "bundled": true
+          "bundled": true,
+          "optional": true
         },
         "brace-expansion": {
           "version": "1.1.11",
           "bundled": true,
+          "optional": true,
           "requires": {
             "balanced-match": "^1.0.0",
             "concat-map": "0.0.1"
@@ -7717,7 +7732,8 @@
         },
         "concat-map": {
           "version": "0.0.1",
-          "bundled": true
+          "bundled": true,
+          "optional": true
         },
         "console-control-strings": {
           "version": "1.1.0",
@@ -7846,6 +7862,7 @@
         "minimatch": {
           "version": "3.0.4",
           "bundled": true,
+          "optional": true,
           "requires": {
             "brace-expansion": "^1.1.7"
           }

services/web/package.json

@@ -40,6 +40,7 @@
     "bcrypt": "^3.0.4",
     "body-parser": "^1.13.1",
     "bufferedstream": "1.6.0",
+    "chai-as-promised": "^7.1.1",
     "codemirror": "^5.33.0",
     "connect-redis": "^3.1.0",
     "contentful": "^6.1.1",

services/web/public/src/main/account-settings.js

@@ -1,23 +1,9 @@
-/* eslint-disable
-    camelcase,
-    max-len,
-    no-return-assign,
-    no-undef,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 define(['base'], function(App) {
   App.controller('AccountSettingsController', function(
     $scope,
     $http,
     $modal,
+    // eslint-disable-next-line camelcase
     event_tracking,
     UserAffiliationsDataService
   ) {
@@ -34,14 +20,13 @@ define(['base'], function(App) {
       })
         .then(function() {
           $scope.unsubscribing = false
-          return ($scope.subscribed = false)
+          $scope.subscribed = false
         })
         .catch(() => ($scope.unsubscribing = true))
     }
 
     $scope.deleteAccount = function() {
-      let modalInstance
-      return (modalInstance = $modal.open({
+      $modal.open({
         templateUrl: 'deleteAccountModalTemplate',
         controller: 'DeleteAccountModalController',
         resolve: {
@@ -56,14 +41,14 @@ define(['base'], function(App) {
               .catch(() => null)
           }
         }
-      }))
+      })
     }
 
-    return ($scope.upgradeIntegration = service =>
-      event_tracking.send('subscription-funnel', 'settings-page', service))
+    $scope.upgradeIntegration = service =>
+      event_tracking.send('subscription-funnel', 'settings-page', service)
   })
 
-  return App.controller('DeleteAccountModalController', function(
+  App.controller('DeleteAccountModalController', function(
     $scope,
     $modalInstance,
     $timeout,
@@ -114,19 +99,21 @@ define(['base'], function(App) {
           $modalInstance.close()
           $scope.state.inflight = false
           $scope.state.error = null
-          return setTimeout(() => (window.location = '/login'), 1000)
+          setTimeout(() => (window.location = '/login'), 1000)
         })
         .catch(function(response) {
           const { data, status } = response
           $scope.state.inflight = false
           if (status === 403) {
             $scope.state.error = { code: 'InvalidCredentialsError' }
-          } else {
+          } else if (data.error) {
             $scope.state.error = { code: data.error }
+          } else {
+            $scope.state.error = { code: 'UserDeletionError' }
           }
         })
     }
 
-    return ($scope.cancel = () => $modalInstance.dismiss('cancel'))
+    $scope.cancel = () => $modalInstance.dismiss('cancel')
   })
 })

services/web/test/unit/bootstrap.js

@@ -5,6 +5,9 @@ require('sinon')
 // has a nicer failure messages
 chai.use(require('sinon-chai'))
 
+// Load promise support for chai
+chai.use(require('chai-as-promised'))
+
 // add support for promises in sinon
 require('sinon-as-promised')
 // add support for mongoose in sinon

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

@@ -0,0 +1,107 @@
+const { expect } = require('chai')
+const sinon = require('sinon')
+const SandboxedModule = require('sandboxed-module')
+
+const MODULE_PATH = '../../../../app/src/Features/Newsletter/NewsletterManager'
+
+describe('NewsletterManager', function() {
+  beforeEach('setup mocks', function() {
+    this.Settings = {
+      mailchimp: {
+        api_key: 'api_key',
+        list_id: 'list_id'
+      }
+    }
+    this.mailchimp = {
+      put: sinon.stub()
+    }
+    this.Mailchimp = sinon.stub().returns(this.mailchimp)
+
+    this.NewsletterManager = SandboxedModule.require(MODULE_PATH, {
+      requires: {
+        'mailchimp-api-v3': this.Mailchimp,
+        'settings-sharelatex': this.Settings
+      }
+    }).promises
+
+    this.user = {
+      _id: 'user_id',
+      email: 'overleaf.duck@example.com',
+      first_name: 'Overleaf',
+      last_name: 'Duck'
+    }
+    // MD5 sum of the user email
+    this.emailHash = 'c02f60ed0ef51818186274e406c9a48f'
+  })
+
+  describe('subscribe', function() {
+    it('calls Mailchimp to subscribe the user', async function() {
+      await this.NewsletterManager.subscribe(this.user)
+      expect(this.mailchimp.put).to.have.been.calledWith(
+        `/lists/list_id/members/${this.emailHash}`,
+        {
+          email_address: this.user.email,
+          status: 'subscribed',
+          status_if_new: 'subscribed',
+          merge_fields: {
+            FNAME: 'Overleaf',
+            LNAME: 'Duck',
+            MONGO_ID: 'user_id'
+          }
+        }
+      )
+    })
+  })
+
+  describe('unsubscribe', function() {
+    it('calls Mailchimp to unsubscribe the user', async function() {
+      await this.NewsletterManager.unsubscribe(this.user)
+      expect(this.mailchimp.put).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'
+          }
+        }
+      )
+    })
+
+    it('ignores a Mailchimp error about fake emails', async function() {
+      this.mailchimp.put.rejects(
+        new Error(
+          'overleaf.duck@example.com looks fake or invalid, please enter a real email address'
+        )
+      )
+      await expect(this.NewsletterManager.unsubscribe(this.user)).to.be
+        .fulfilled
+    })
+
+    it('rejects on other errors', async function() {
+      this.mailchimp.put.rejects(
+        new Error('something really wrong is happening')
+      )
+      await expect(this.NewsletterManager.unsubscribe(this.user)).to.be.rejected
+    })
+  })
+
+  describe('changeEmail', function() {
+    it('calls Mailchimp to change the subscriber email', async function() {
+      await this.NewsletterManager.changeEmail(
+        this.user.email,
+        'overleaf.squirrel@example.com'
+      )
+      expect(this.mailchimp.put).to.have.been.calledWith(
+        `/lists/list_id/members/${this.emailHash}`,
+        {
+          email_address: 'overleaf.squirrel@example.com',
+          status_if_new: 'unsubscribed'
+        }
+      )
+    })
+  })
+})

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

@@ -38,7 +38,11 @@ describe('UserDeleter', function() {
     )
     this.user = this.mockedUser.object
 
-    this.NewsletterManager = { unsubscribe: sinon.stub().yields() }
+    this.NewsletterManager = {
+      promises: {
+        unsubscribe: sinon.stub().resolves()
+      }
+    }
 
     this.ProjectDeleter = {
       promises: {
@@ -47,23 +51,33 @@ describe('UserDeleter', function() {
     }
 
     this.SubscriptionHandler = {
-      cancelSubscription: sinon.stub().yields()
+      promises: {
+        cancelSubscription: sinon.stub().resolves()
+      }
     }
 
     this.SubscriptionUpdater = {
-      removeUserFromAllGroups: sinon.stub().yields()
+      promises: {
+        removeUserFromAllGroups: sinon.stub().resolves()
+      }
     }
 
     this.SubscriptionLocator = {
-      getUsersSubscription: sinon.stub().yields()
+      promises: {
+        getUsersSubscription: sinon.stub().resolves()
+      }
     }
 
     this.UserMembershipsHandler = {
-      removeUserFromAllEntities: sinon.stub().yields()
+      promises: {
+        removeUserFromAllEntities: sinon.stub().resolves()
+      }
     }
 
     this.InstitutionsApi = {
-      deleteAffiliations: sinon.stub().yields()
+      promises: {
+        deleteAffiliations: sinon.stub().resolves()
+      }
     }
 
     this.UserDeleter = SandboxedModule.require(modulePath, {
@@ -98,8 +112,6 @@ describe('UserDeleter', function() {
 
   describe('deleteUser', function() {
     beforeEach(function() {
-      this.UserDeleter.promises.ensureCanDeleteUser = sinon.stub().resolves()
-
       this.UserMock.expects('findById')
         .withArgs(this.userId)
         .chain('exec')
@@ -124,11 +136,6 @@ describe('UserDeleter', function() {
             deletedUserOverleafId: this.user.overleaf.id
           }
         }
-
-        this.UserMock.expects('deleteOne')
-          .withArgs({ _id: this.userId })
-          .chain('exec')
-          .resolves()
       })
 
       describe('when no options are passed', function() {
@@ -139,97 +146,103 @@ describe('UserDeleter', function() {
             .resolves()
         })
 
-        it('should find and the user in mongo by its id', async function() {
-          await this.UserDeleter.promises.deleteUser(this.userId)
-          this.UserMock.verify()
-        })
+        describe('when unsubscribing in Mailchimp succeeds', function() {
+          beforeEach(function() {
+            this.UserMock.expects('deleteOne')
+              .withArgs({ _id: this.userId })
+              .chain('exec')
+              .resolves()
+          })
 
-        it('should unsubscribe the user from the news letter', async function() {
-          await this.UserDeleter.promises.deleteUser(this.userId)
-          expect(this.NewsletterManager.unsubscribe).to.have.been.calledWith(
-            this.user
-          )
-        })
+          it('should find and the user in mongo by its id', async function() {
+            await this.UserDeleter.promises.deleteUser(this.userId)
+            this.UserMock.verify()
+          })
 
-        it('should delete all the projects of a user', async function() {
-          await this.UserDeleter.promises.deleteUser(this.userId)
-          expect(
-            this.ProjectDeleter.promises.deleteUsersProjects
-          ).to.have.been.calledWith(this.userId)
-        })
+          it('should unsubscribe the user from the news letter', async function() {
+            await this.UserDeleter.promises.deleteUser(this.userId)
+            expect(
+              this.NewsletterManager.promises.unsubscribe
+            ).to.have.been.calledWith(this.user)
+          })
 
-        it("should cancel the user's subscription", async function() {
-          await this.UserDeleter.promises.deleteUser(this.userId)
-          expect(
-            this.SubscriptionHandler.cancelSubscription
-          ).to.have.been.calledWith(this.user)
-        })
+          it('should delete all the projects of a user', async function() {
+            await this.UserDeleter.promises.deleteUser(this.userId)
+            expect(
+              this.ProjectDeleter.promises.deleteUsersProjects
+            ).to.have.been.calledWith(this.userId)
+          })
 
-        it('should delete user affiliations', async function() {
-          await this.UserDeleter.promises.deleteUser(this.userId)
-          expect(
-            this.InstitutionsApi.deleteAffiliations
-          ).to.have.been.calledWith(this.userId)
-        })
+          it("should cancel the user's subscription", async function() {
+            await this.UserDeleter.promises.deleteUser(this.userId)
+            expect(
+              this.SubscriptionHandler.promises.cancelSubscription
+            ).to.have.been.calledWith(this.user)
+          })
 
-        it('should remove user from group subscriptions', async function() {
-          await this.UserDeleter.promises.deleteUser(this.userId)
-          expect(
-            this.SubscriptionUpdater.removeUserFromAllGroups
-          ).to.have.been.calledWith(this.userId)
-        })
+          it('should delete user affiliations', async function() {
+            await this.UserDeleter.promises.deleteUser(this.userId)
+            expect(
+              this.InstitutionsApi.promises.deleteAffiliations
+            ).to.have.been.calledWith(this.userId)
+          })
 
-        it('should remove user memberships', async function() {
-          await this.UserDeleter.promises.deleteUser(this.userId)
-          expect(
-            this.UserMembershipsHandler.removeUserFromAllEntities
-          ).to.have.been.calledWith(this.userId)
-        })
+          it('should remove user from group subscriptions', async function() {
+            await this.UserDeleter.promises.deleteUser(this.userId)
+            expect(
+              this.SubscriptionUpdater.promises.removeUserFromAllGroups
+            ).to.have.been.calledWith(this.userId)
+          })
 
-        it('ensures user can be deleted', async function() {
-          await this.UserDeleter.promises.deleteUser(this.userId)
-          expect(
-            this.UserDeleter.promises.ensureCanDeleteUser
-          ).to.have.been.calledWith(this.user)
-        })
+          it('should remove user memberships', async function() {
+            await this.UserDeleter.promises.deleteUser(this.userId)
+            expect(
+              this.UserMembershipsHandler.promises.removeUserFromAllEntities
+            ).to.have.been.calledWith(this.userId)
+          })
 
-        it('should create a deletedUser', async function() {
-          await this.UserDeleter.promises.deleteUser(this.userId)
-          this.DeletedUserMock.verify()
+          it('rejects if the user is a subscription admin', async function() {
+            this.SubscriptionLocator.promises.getUsersSubscription.rejects({
+              _id: 'some-subscription'
+            })
+            await expect(this.UserDeleter.promises.deleteUser(this.userId)).to
+              .be.rejected
+          })
+
+          it('should create a deletedUser', async function() {
+            await this.UserDeleter.promises.deleteUser(this.userId)
+            this.DeletedUserMock.verify()
+          })
         })
 
         describe('when unsubscribing from mailchimp fails', function() {
           beforeEach(function() {
-            this.NewsletterManager.unsubscribe = sinon
-              .stub()
-              .yields(new Error('something went wrong'))
-          })
-
-          it('should not return an error', async function() {
-            try {
-              await this.UserDeleter.promises.deleteUser(this.userId)
-            } catch (error) {
-              expect(error).not.to.exist
-              expect.fail()
-            }
-            // check that we called `unsubscribe` to generate the error
-            expect(this.NewsletterManager.unsubscribe).to.have.been.calledWith(
-              this.user
+            this.NewsletterManager.promises.unsubscribe.rejects(
+              new Error('something went wrong')
             )
           })
 
-          it('should delete the user', async function() {
-            await this.UserDeleter.promises.deleteUser(this.userId)
+          it('should return an error and not delete the user', async function() {
+            await expect(this.UserDeleter.promises.deleteUser(this.userId)).to
+              .be.rejected
             this.UserMock.verify()
           })
 
-          it('should log an error', async function() {
-            await this.UserDeleter.promises.deleteUser(this.userId)
-            sinon.assert.called(this.logger.err)
+          it('should log a warning', async function() {
+            await expect(this.UserDeleter.promises.deleteUser(this.userId)).to
+              .be.rejected
+            sinon.assert.called(this.logger.warn)
           })
         })
 
         describe('when called as a callback', function() {
+          beforeEach(function() {
+            this.UserMock.expects('deleteOne')
+              .withArgs({ _id: this.userId })
+              .chain('exec')
+              .resolves()
+          })
+
           it('should delete the user', function(done) {
             this.UserDeleter.deleteUser(this.userId, err => {
               expect(err).not.to.exist
@@ -253,6 +266,10 @@ describe('UserDeleter', function() {
             .withArgs(this.deletedUser)
             .chain('exec')
             .resolves()
+          this.UserMock.expects('deleteOne')
+            .withArgs({ _id: this.userId })
+            .chain('exec')
+            .resolves()
         })
 
         it('should add the deleted user id and ip address to the deletedUser', async function() {
@@ -285,45 +302,34 @@ describe('UserDeleter', function() {
 
     describe('when the user cannot be deleted because they are a subscription admin', function() {
       beforeEach(function() {
-        this.UserDeleter.promises.ensureCanDeleteUser.rejects(
-          new Errors.SubscriptionAdminDeletionError()
-        )
+        this.SubscriptionLocator.promises.getUsersSubscription.resolves({
+          _id: 'some-subscription'
+        })
       })
 
       it('fails with a SubscriptionAdminDeletionError', async function() {
-        let error
-        try {
-          await this.UserDeleter.promises.deleteUser(this.userId)
-        } catch (e) {
-          error = e
-        } finally {
-          expect(error).to.be.instanceof(Errors.SubscriptionAdminDeletionError)
-        }
+        await expect(
+          this.UserDeleter.promises.deleteUser(this.userId)
+        ).to.be.rejectedWith(Errors.SubscriptionAdminDeletionError)
       })
 
       it('should not create a deletedUser', async function() {
-        try {
-          await this.UserDeleter.promises.deleteUser(this.userId)
-        } catch (e) {
-        } finally {
-          this.DeletedUserMock.verify()
-        }
+        await expect(this.UserDeleter.promises.deleteUser(this.userId)).to.be
+          .rejected
+        this.DeletedUserMock.verify()
       })
 
       it('should not remove the user from mongo', async function() {
-        try {
-          await this.UserDeleter.promises.deleteUser(this.userId)
-        } catch (e) {
-        } finally {
-          this.UserMock.verify()
-        }
+        await expect(this.UserDeleter.promises.deleteUser(this.userId)).to.be
+          .rejected
+        this.UserMock.verify()
       })
     })
   })
 
   describe('ensureCanDeleteUser', function() {
     it('should not return error when user can be deleted', async function() {
-      this.SubscriptionLocator.getUsersSubscription.yields(null, null)
+      this.SubscriptionLocator.promises.getUsersSubscription.resolves(null)
       let error
       try {
         await this.UserDeleter.promises.ensureCanDeleteUser(this.user)
@@ -335,7 +341,7 @@ describe('UserDeleter', function() {
     })
 
     it('should return custom error when user is group admin', async function() {
-      this.SubscriptionLocator.getUsersSubscription.yields(null, {
+      this.SubscriptionLocator.promises.getUsersSubscription.resolves({
         _id: '123abc'
       })
       let error
@@ -349,7 +355,7 @@ describe('UserDeleter', function() {
     })
 
     it('propagates errors', async function() {
-      this.SubscriptionLocator.getUsersSubscription.yields(
+      this.SubscriptionLocator.promises.getUsersSubscription.rejects(
         new Error('Some error')
       )
       let error
@@ -364,16 +370,20 @@ describe('UserDeleter', function() {
   })
 
   describe('expireDeletedUsersAfterDuration', function() {
+    const userId1 = new ObjectId()
+    const userId2 = new ObjectId()
+
     beforeEach(function() {
-      this.UserDeleter.promises.expireDeletedUser = sinon.stub().resolves()
       this.deletedUsers = [
         {
-          user: { _id: 'wombat' },
-          deleterData: { deletedUserId: 'wombat' }
+          user: { _id: userId1 },
+          deleterData: { deletedUserId: userId1 },
+          save: sinon.stub().resolves()
         },
         {
-          user: { _id: 'potato' },
-          deleterData: { deletedUserId: 'potato' }
+          user: { _id: userId2 },
+          deleterData: { deletedUserId: userId2 },
+          save: sinon.stub().resolves()
         }
       ]
 
@@ -388,16 +398,22 @@ describe('UserDeleter', function() {
         })
         .chain('exec')
         .resolves(this.deletedUsers)
+      for (const deletedUser of this.deletedUsers) {
+        this.DeletedUserMock.expects('findOne')
+          .withArgs({
+            'deleterData.deletedUserId': deletedUser.deleterData.deletedUserId
+          })
+          .chain('exec')
+          .resolves(deletedUser)
+      }
     })
 
-    it('calls expireDeletedUser for each user', async function() {
+    it('clears data from all deleted users', async function() {
       await this.UserDeleter.promises.expireDeletedUsersAfterDuration()
-      expect(
-        this.UserDeleter.promises.expireDeletedUser
-      ).to.have.been.calledWith('wombat')
-      expect(
-        this.UserDeleter.promises.expireDeletedUser
-      ).to.have.been.calledWith('potato')
+      for (const deletedUser of this.deletedUsers) {
+        expect(deletedUser.user).to.be.undefined
+        expect(deletedUser.save.called).to.be.true
+      }
     })
   })
 

services/web/test/unit/src/util/promisesTests.js

@@ -0,0 +1,60 @@
+const { expect } = require('chai')
+const { promisifyAll } = require('../../../../app/src/util/promises')
+
+describe('promisifyAll', function() {
+  describe('basic functionality', function() {
+    before(function() {
+      this.module = {
+        SOME_CONSTANT: 1,
+        asyncAdd(a, b, callback) {
+          callback(null, a + b)
+        },
+        asyncDouble(x, callback) {
+          this.asyncAdd(x, x, callback)
+        }
+      }
+      this.promisified = promisifyAll(this.module)
+    })
+
+    it('promisifies functions in the module', async function() {
+      const sum = await this.promisified.asyncAdd(29, 33)
+      expect(sum).to.equal(62)
+    })
+
+    it('binds this to the original module', async function() {
+      const sum = await this.promisified.asyncDouble(38)
+      expect(sum).to.equal(76)
+    })
+
+    it('does not copy over non-functions', async function() {
+      expect(this.promisified).not.to.have.property('SOME_CONSTANT')
+    })
+
+    it('does not modify the prototype of the module', async function() {
+      expect(this.promisified.toString()).to.equal('[object Object]')
+    })
+  })
+
+  describe('without option', function() {
+    before(function() {
+      this.module = {
+        asyncAdd(a, b, callback) {
+          callback(null, a + b)
+        },
+        syncAdd(a, b) {
+          return a + b
+        }
+      }
+      this.promisified = promisifyAll(this.module, { without: 'syncAdd' })
+    })
+
+    it('does not promisify excluded functions', function() {
+      expect(this.promisified.syncAdd).not.to.exist
+    })
+
+    it('promisifies other functions', async function() {
+      const sum = await this.promisified.asyncAdd(12, 89)
+      expect(sum).to.equal(101)
+    })
+  })
+})