do not use v1 for setting default email

GitOrigin-RevId: 00d4610ae55c0a90699d4bc79e7d08d432087abe

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

@@ -87,18 +87,12 @@ module.exports = UserEmailsController = {
     if (email == null) {
       return res.sendStatus(422)
     }
-
-    return UserUpdater.updateV1AndSetDefaultEmailAddress(
-      userId,
-      email,
-      function(error) {
-        if (error != null) {
-          return UserEmailsController._handleEmailError(error, req, res, next)
-        } else {
-          return res.sendStatus(200)
-        }
+    UserUpdater.setDefaultEmailAddress(userId, email, err => {
+      if (err) {
+        return UserEmailsController._handleEmailError(err, req, res, next)
       }
-    )
+      res.sendStatus(200)
+    })
   },
 
   endorse(req, res, next) {

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

@@ -28,8 +28,6 @@ const {
 const FeaturesUpdater = require('../Subscription/FeaturesUpdater')
 const EmailHelper = require('../Helpers/EmailHelper')
 const Errors = require('../Errors/Errors')
-const Settings = require('settings-sharelatex')
-const request = require('request')
 const NewsletterManager = require('../Newsletter/NewsletterManager')
 
 module.exports = UserUpdater = {
@@ -71,7 +69,7 @@ module.exports = UserUpdater = {
             return cb(error)
           }),
         cb => UserUpdater.addEmailAddress(userId, newEmail, cb),
-        cb => UserUpdater.setDefaultEmailAddress(userId, newEmail, cb),
+        cb => UserUpdater.setDefaultEmailAddress(userId, newEmail, true, cb),
         cb => UserUpdater.removeEmailAddress(userId, oldEmail, cb)
       ],
       callback
@@ -156,109 +154,44 @@ module.exports = 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, callback) {
+  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'))
     }
-    return UserGetter.getUserEmail(userId, (error, oldEmail) => {
-      if (typeof err !== 'undefined' && err !== null) {
-        return callback(error)
+    UserGetter.getUser(userId, { email: 1, emails: 1 }, (err, user) => {
+      if (err) {
+        return callback(err)
       }
-      const query = { _id: userId, 'emails.email': email }
-      const update = { $set: { email } }
-      return this.updateUser(query, update, function(error, res) {
-        if (error != null) {
-          logger.warn({ error }, 'problem setting default emails')
-          return callback(error)
-        } else if (res.n === 0) {
-          // TODO: Check n or nMatched?
-          return callback(new Error('Default email does not belong to user'))
-        } else {
-          NewsletterManager.changeEmail(oldEmail, email, function() {})
-          return callback()
-        }
-      })
-    })
-  },
-
-  updateV1AndSetDefaultEmailAddress(userId, email, callback) {
-    return this.updateEmailAddressInV1(userId, email, error => {
-      if (error != null) {
-        return callback(error)
+      if (!user) {
+        return callback(new Error('invalid userId'))
       }
-      return this.setDefaultEmailAddress(userId, email, callback)
-    })
-  },
-
-  updateEmailAddressInV1(userId, newEmail, callback) {
-    if (!Settings.apis.v1.url) {
-      return callback()
-    }
-    return UserGetter.getUser(userId, { 'overleaf.id': 1, emails: 1 }, function(
-      error,
-      user
-    ) {
-      let email
-      if (error != null) {
-        return callback(error)
+      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 (user == null) {
-        return callback(new Errors.NotFoundError('no user found'))
-      }
-      if ((user.overleaf != null ? user.overleaf.id : undefined) == null) {
-        return callback()
+      if (!userEmail.confirmedAt && !allowUnconfirmed) {
+        return callback(new Errors.UnconfirmedEmailError())
       }
-      let newEmailIsConfirmed = false
-      for (email of Array.from(user.emails)) {
-        if (email.email === newEmail && email.confirmedAt != null) {
-          newEmailIsConfirmed = true
-          break
+      const query = { _id: userId, 'emails.email': email }
+      const update = { $set: { email } }
+      UserUpdater.updateUser(query, update, (err, res) => {
+        if (err) {
+          return callback(err)
         }
-      }
-      if (!newEmailIsConfirmed) {
-        return callback(
-          new Errors.UnconfirmedEmailError(
-            "can't update v1 with unconfirmed email"
-          )
-        )
-      }
-      return request(
-        {
-          baseUrl: Settings.apis.v1.url,
-          url: `/api/v1/sharelatex/users/${user.overleaf.id}/email`,
-          method: 'PUT',
-          auth: {
-            user: Settings.apis.v1.user,
-            pass: Settings.apis.v1.pass,
-            sendImmediately: true
-          },
-          json: {
-            user: {
-              email: newEmail
-            }
-          },
-          timeout: 5 * 1000
-        },
-        function(error, response, body) {
-          if (error != null) {
-            if (error.code === 'ECONNREFUSED') {
-              error = new Errors.V1ConnectionError('No V1 connection')
-            }
-            return callback(error)
-          }
-          if (response.statusCode === 409) {
-            // Conflict
-            return callback(new Errors.EmailExistsError('email exists in v1'))
-          } else if (response.statusCode >= 200 && response.statusCode < 300) {
-            return callback()
-          } else {
-            return callback(
-              new Error(`non-success code from v1: ${response.statusCode}`)
-            )
-          }
+        // this should not happen
+        if (res.n === 0) {
+          return callback(new Error('email update error'))
         }
-      )
+        // do not wait - this will log its own errors
+        NewsletterManager.changeEmail(oldEmail, email, () => {})
+        callback()
+      })
     })
   },
 

services/web/test/acceptance/src/UserEmailsTests.js

@@ -653,7 +653,7 @@ describe('UserEmails', function() {
       )
     })
 
-    it('should update the email in v1 if confirmed', function(done) {
+    it('should not update the email in v1', function(done) {
       const token = null
       return async.series(
         [
@@ -725,18 +725,13 @@ describe('UserEmails', function() {
           if (error != null) {
             return done(error)
           }
-          expect(
-            MockV1Api.updateEmail.calledWith(
-              42,
-              'new-confirmed-default-in-v1@example.com'
-            )
-          ).to.equal(true)
-          return done()
+          expect(MockV1Api.updateEmail.callCount).to.equal(0)
+          done()
         }
       )
     })
 
-    it('should return an error if the email exists in v1', function(done) {
+    it('should not return an error if the email exists in v1', function(done) {
       MockV1Api.existingEmails.push('exists-in-v1@example.com')
       return async.series(
         [
@@ -798,11 +793,8 @@ describe('UserEmails', function() {
                 if (error != null) {
                   return done(error)
                 }
-                expect(response.statusCode).to.equal(409)
-                expect(body).to.deep.equal({
-                  message: 'This email is already registered'
-                })
-                return cb()
+                expect(response.statusCode).to.equal(200)
+                cb()
               }
             )
           },
@@ -810,8 +802,8 @@ describe('UserEmails', function() {
             return this.user.request(
               { url: '/user/emails', json: true },
               function(error, response, body) {
-                expect(body[0].default).to.equal(true)
-                expect(body[1].default).to.equal(false)
+                expect(body[0].default).to.equal(false)
+                expect(body[1].default).to.equal(true)
                 return cb()
               }
             )

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

@@ -187,22 +187,22 @@ describe('UserEmailsController', function() {
     beforeEach(function() {
       this.email = 'email_to_set_default@bar.com'
       this.req.body.email = this.email
-      return this.EmailHelper.parseEmail.returns(this.email)
+      this.EmailHelper.parseEmail.returns(this.email)
     })
 
     it('sets default email', function(done) {
-      this.UserUpdater.updateV1AndSetDefaultEmailAddress.callsArgWith(2, null)
+      this.UserUpdater.setDefaultEmailAddress.yields()
 
-      return this.UserEmailsController.setDefault(this.req, {
+      this.UserEmailsController.setDefault(this.req, {
         sendStatus: code => {
           code.should.equal(200)
           assertCalledWith(this.EmailHelper.parseEmail, this.email)
           assertCalledWith(
-            this.UserUpdater.updateV1AndSetDefaultEmailAddress,
+            this.UserUpdater.setDefaultEmailAddress,
             this.user._id,
             this.email
           )
-          return done()
+          done()
         }
       })
     })
@@ -210,7 +210,7 @@ describe('UserEmailsController', function() {
     it('handles email parse error', function(done) {
       this.EmailHelper.parseEmail.returns(null)
 
-      return this.UserEmailsController.setDefault(this.req, {
+      this.UserEmailsController.setDefault(this.req, {
         sendStatus: code => {
           code.should.equal(422)
           assertNotCalled(this.UserUpdater.setDefaultEmailAddress)

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

@@ -74,7 +74,8 @@ describe('UserUpdater', function() {
       name: 'bob',
       email: 'hello@world.com'
     }
-    return (this.newEmail = 'bob@bob.com')
+    this.newEmail = 'bob@bob.com'
+    this.callback = sinon.stub()
   })
 
   afterEach(() => tk.reset())
@@ -83,7 +84,7 @@ describe('UserUpdater', function() {
     beforeEach(function() {
       this.UserGetter.getUserEmail.callsArgWith(1, null, this.stubbedUser.email)
       this.UserUpdater.addEmailAddress = sinon.stub().callsArgWith(2)
-      this.UserUpdater.setDefaultEmailAddress = sinon.stub().callsArgWith(2)
+      this.UserUpdater.setDefaultEmailAddress = sinon.stub().yields()
       return (this.UserUpdater.removeEmailAddress = sinon
         .stub()
         .callsArgWith(2))
@@ -325,14 +326,20 @@ describe('UserUpdater', function() {
 
   describe('setDefaultEmailAddress', function() {
     beforeEach(function() {
-      this.UserGetter.getUserEmail.callsArgWith(1, null, this.stubbedUser.email)
-      return this.NewsletterManager.changeEmail.callsArgWith(2, null)
+      this.stubbedUser.emails = [
+        {
+          email: this.newEmail,
+          confirmedAt: new Date()
+        }
+      ]
+      this.UserGetter.getUser = sinon.stub().yields(null, this.stubbedUser)
+      this.NewsletterManager.changeEmail.callsArgWith(2, null)
     })
 
     it('set default', function(done) {
       this.UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, { n: 1 })
 
-      return this.UserUpdater.setDefaultEmailAddress(
+      this.UserUpdater.setDefaultEmailAddress(
         this.stubbedUser._id,
         this.newEmail,
         err => {
@@ -343,7 +350,7 @@ describe('UserUpdater', function() {
               { $set: { email: this.newEmail } }
             )
             .should.equal(true)
-          return done()
+          done()
         }
       )
     })
@@ -351,7 +358,7 @@ describe('UserUpdater', function() {
     it('set changed the email in newsletter', function(done) {
       this.UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, { n: 1 })
 
-      return this.UserUpdater.setDefaultEmailAddress(
+      this.UserUpdater.setDefaultEmailAddress(
         this.stubbedUser._id,
         this.newEmail,
         err => {
@@ -359,7 +366,7 @@ describe('UserUpdater', function() {
           this.NewsletterManager.changeEmail
             .calledWith(this.stubbedUser.email, this.newEmail)
             .should.equal(true)
-          return done()
+          done()
         }
       )
     })
@@ -369,12 +376,12 @@ describe('UserUpdater', function() {
         .stub()
         .callsArgWith(2, new Error('nope'))
 
-      return this.UserUpdater.setDefaultEmailAddress(
+      this.UserUpdater.setDefaultEmailAddress(
         this.stubbedUser._id,
         this.newEmail,
         err => {
           should.exist(err)
-          return done()
+          done()
         }
       )
     })
@@ -382,26 +389,73 @@ describe('UserUpdater', function() {
     it('handle missed update', function(done) {
       this.UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, { n: 0 })
 
-      return this.UserUpdater.setDefaultEmailAddress(
+      this.UserUpdater.setDefaultEmailAddress(
         this.stubbedUser._id,
         this.newEmail,
         err => {
           should.exist(err)
-          return done()
+          done()
         }
       )
     })
 
     it('validates email', function(done) {
-      return this.UserUpdater.setDefaultEmailAddress(
+      this.UserUpdater.setDefaultEmailAddress(
         this.stubbedUser._id,
         '.edu',
         err => {
           should.exist(err)
-          return done()
+          done()
         }
       )
     })
+
+    describe('when email not confirmed', () => {
+      beforeEach(function() {
+        this.stubbedUser.emails = [
+          {
+            email: this.newEmail,
+            confirmedAt: null
+          }
+        ]
+        this.UserGetter.getUser = sinon.stub().yields(null, this.stubbedUser)
+        this.UserUpdater.updateUser = sinon.stub()
+        this.NewsletterManager.changeEmail = 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'
+        )
+        this.UserUpdater.updateUser.callCount.should.equal(0)
+        this.NewsletterManager.changeEmail.callCount.should.equal(0)
+      })
+    })
+
+    describe('when email does not belong to user', () => {
+      beforeEach(function() {
+        this.stubbedUser.emails = []
+        this.UserGetter.getUser = sinon.stub().yields(null, this.stubbedUser)
+        this.UserUpdater.updateUser = sinon.stub()
+        this.NewsletterManager.changeEmail = 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('Error')
+        this.UserUpdater.updateUser.callCount.should.equal(0)
+        this.NewsletterManager.changeEmail.callCount.should.equal(0)
+      })
+    })
   })
 
   describe('confirmEmail', function() {