diff --git a/services/web/app/src/Features/Subscription/RecurlyWrapper.js b/services/web/app/src/Features/Subscription/RecurlyWrapper.js index d84b00c686..90e3d4e5ca 100644 --- a/services/web/app/src/Features/Subscription/RecurlyWrapper.js +++ b/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 diff --git a/services/web/app/src/Features/User/UserEmailsController.js b/services/web/app/src/Features/User/UserEmailsController.js index ce3041b9a2..b79b261d00 100644 --- a/services/web/app/src/Features/User/UserEmailsController.js +++ b/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) } diff --git a/services/web/app/src/Features/User/UserUpdater.js b/services/web/app/src/Features/User/UserUpdater.js index 24eedb18c5..204322bfcb 100644 --- a/services/web/app/src/Features/User/UserUpdater.js +++ b/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) } diff --git a/services/web/test/unit/src/User/UserUpdaterTests.js b/services/web/test/unit/src/User/UserUpdaterTests.js index a2ef0fe57e..41221873b0 100644 --- a/services/web/test/unit/src/User/UserUpdaterTests.js +++ b/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 + 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.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', 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) }) }) })