From 390c8641da842a311a3ede1bd03dd3d95b9df8d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Alby?= Date: Mon, 16 Dec 2019 16:22:21 +0530 Subject: [PATCH] Merge pull request #2454 from overleaf/ta-recurly-address-check Check Country Presence when Creating New Subscription GitOrigin-RevId: a92266cf2a304e3649ff3b02f9b47e76ae0f8523 --- .../Features/Subscription/RecurlyWrapper.js | 69 +++++++++++-------- .../Subscription/SubscriptionController.js | 5 ++ .../src/Subscription/RecurlyWrapperTests.js | 23 +++++-- .../SubscriptionControllerTests.js | 16 +++++ 4 files changed, 81 insertions(+), 32 deletions(-) diff --git a/services/web/app/src/Features/Subscription/RecurlyWrapper.js b/services/web/app/src/Features/Subscription/RecurlyWrapper.js index f3cd21c1e8..eac574ca0d 100644 --- a/services/web/app/src/Features/Subscription/RecurlyWrapper.js +++ b/services/web/app/src/Features/Subscription/RecurlyWrapper.js @@ -24,6 +24,7 @@ const Settings = require('settings-sharelatex') const xml2js = require('xml2js') const logger = require('logger-sharelatex') const Async = require('async') +const Errors = require('../Errors/Errors') const SubscriptionErrors = require('./Errors') module.exports = RecurlyWrapper = { @@ -79,28 +80,22 @@ module.exports = RecurlyWrapper = { createAccount(cache, next) { const { user } = cache const { subscriptionDetails } = cache - const { address } = subscriptionDetails - if (!address) { - return next( - new Error('no address in subscriptionDetails at createAccount stage') - ) - } if (cache.userExists) { return next(null, cache) } + + let address + try { + address = getAddressFromSubscriptionDetails(subscriptionDetails) + } catch (error) { + return next(error) + } const data = { account_code: user._id, email: user.email, first_name: user.first_name, last_name: user.last_name, - address: { - address1: address.address1, - address2: address.address2 || '', - city: address.city || '', - state: address.state || '', - zip: address.zip || '', - country: address.country - } + address } const requestBody = RecurlyWrapper._buildXml('account', data) @@ -189,21 +184,14 @@ module.exports = RecurlyWrapper = { if (!accountCode) { return next(new Error('no account code at setAddress stage')) } - const { address } = subscriptionDetails - if (!address) { - return next( - new Error('no address in subscriptionDetails at setAddress stage') - ) + + let address + try { + address = getAddressFromSubscriptionDetails(subscriptionDetails) + } catch (error) { + return next(error) } - const data = { - address1: address.address1, - address2: address.address2 || '', - city: address.city || '', - state: address.state || '', - zip: address.zip || '', - country: address.country - } - const requestBody = RecurlyWrapper._buildXml('billing_info', data) + const requestBody = RecurlyWrapper._buildXml('billing_info', address) return RecurlyWrapper.apiRequest( { @@ -1051,6 +1039,31 @@ function getCustomFieldsFromSubscriptionDetails(subscriptionDetails) { return { custom_field: customFields } } +function getAddressFromSubscriptionDetails(subscriptionDetails) { + const { address } = subscriptionDetails + if (!address || !address.country) { + throw new Errors.InvalidError({ + message: 'Invalid country', + info: { + public: { + message: 'Invalid country' + } + } + }) + } + + const addressObject = { + address1: address.address1, + address2: address.address2 || '', + city: address.city || '', + state: address.state || '', + zip: address.zip || '', + country: address.country + } + + return addressObject +} + function __guard__(value, transform) { return typeof value !== 'undefined' && value !== null ? transform(value) diff --git a/services/web/app/src/Features/Subscription/SubscriptionController.js b/services/web/app/src/Features/Subscription/SubscriptionController.js index 26a7d33aee..d561a20cac 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionController.js +++ b/services/web/app/src/Features/Subscription/SubscriptionController.js @@ -29,6 +29,7 @@ const FeaturesUpdater = require('./FeaturesUpdater') const planFeatures = require('./planFeatures') const GroupPlansData = require('./GroupPlansData') const V1SubscriptionManager = require('./V1SubscriptionManager') +const Errors = require('../Errors/Errors') const SubscriptionErrors = require('./Errors') const HttpErrors = require('@overleaf/o-error/http') @@ -219,6 +220,10 @@ module.exports = SubscriptionController = { return next( new HttpErrors.UnprocessableEntityError({}).withCause(err) ) + } else if (err instanceof Errors.InvalidError) { + return next( + new HttpErrors.UnprocessableEntityError({}).withCause(err) + ) } logger.warn( diff --git a/services/web/test/unit/src/Subscription/RecurlyWrapperTests.js b/services/web/test/unit/src/Subscription/RecurlyWrapperTests.js index 2375cca8fc..6b3265dd38 100644 --- a/services/web/test/unit/src/Subscription/RecurlyWrapperTests.js +++ b/services/web/test/unit/src/Subscription/RecurlyWrapperTests.js @@ -21,6 +21,7 @@ const querystring = require('querystring') const modulePath = '../../../../app/src/Features/Subscription/RecurlyWrapper' const SandboxedModule = require('sandboxed-module') const tk = require('timekeeper') +const Errors = require('../../../../app/src/Features/Errors/Errors') const SubscriptionErrors = require('../../../../app/src/Features/Subscription/Errors') const fixtures = { @@ -158,7 +159,8 @@ describe('RecurlyWrapper', function() { }, request: sinon.stub(), xml2js: require('xml2js'), - './Errors': SubscriptionErrors + './Errors': SubscriptionErrors, + '../Errors/Errors': Errors } } )) @@ -1148,6 +1150,19 @@ describe('RecurlyWrapper', function() { }) }) + describe('when country is missing from address', function() { + beforeEach(function() { + return (this.cache.subscriptionDetails.address = {}) + }) + + it('should produce an error', function(done) { + return this.call((err, result) => { + expect(err).to.be.instanceof(Errors.InvalidError) + return done() + }) + }) + }) + describe('when account already exists', function() { beforeEach(function() { this.cache.userExists = true @@ -1378,14 +1393,14 @@ describe('RecurlyWrapper', function() { }) }) - describe('when address is missing from subscriptionDetails', function() { + describe('when country is missing', function() { beforeEach(function() { - return (this.cache.subscriptionDetails.address = null) + return (this.cache.subscriptionDetails.address = { country: '' }) }) it('should produce an error', function(done) { return this.call((err, result) => { - expect(err).to.be.instanceof(Error) + expect(err).to.be.instanceof(Errors.InvalidError) return done() }) }) diff --git a/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js b/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js index 3f5a1aa1b5..85de32c838 100644 --- a/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js +++ b/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js @@ -19,6 +19,7 @@ const MockRequest = require('../helpers/MockRequest') const MockResponse = require('../helpers/MockResponse') const modulePath = '../../../../app/src/Features/Subscription/SubscriptionController' +const Errors = require('../../../../app/src/Features/Errors/Errors') const SubscriptionErrors = require('../../../../app/src/Features/Subscription/Errors') const OError = require('@overleaf/o-error') const HttpErrors = require('@overleaf/o-error/http') @@ -118,6 +119,7 @@ describe('SubscriptionController', function() { './FeaturesUpdater': (this.FeaturesUpdater = {}), './GroupPlansData': (this.GroupPlansData = {}), './V1SubscriptionManager': (this.V1SubscriptionManager = {}), + '../Errors/Errors': Errors, './Errors': SubscriptionErrors, '@overleaf/o-error/http': HttpErrors } @@ -433,6 +435,20 @@ describe('SubscriptionController', function() { }) return done() }) + + it('should handle validation errors', function(done) { + this.next = sinon.stub() + this.LimitationsManager.userHasV1OrV2Subscription.yields(null, false) + this.SubscriptionHandler.createSubscription.yields( + new Errors.InvalidError({}) + ) + this.SubscriptionController.createSubscription(this.req, null, error => { + expect(error).to.exist + expect(error).to.be.instanceof(HttpErrors.UnprocessableEntityError) + expect(OError.hasCauseInstanceOf(error, Errors.InvalidError)).to.be.true + }) + return done() + }) }) describe('updateSubscription via post', function() {