Merge pull request #2454 from overleaf/ta-recurly-address-check

Check Country Presence when Creating New Subscription

GitOrigin-RevId: a92266cf2a304e3649ff3b02f9b47e76ae0f8523

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')
-        )
-      }
-      const data = {
-        address1: address.address1,
-        address2: address.address2 || '',
-        city: address.city || '',
-        state: address.state || '',
-        zip: address.zip || '',
-        country: address.country
+
+      let address
+      try {
+        address = getAddressFromSubscriptionDetails(subscriptionDetails)
+      } catch (error) {
+        return next(error)
       }
-      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)

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(

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()
           })
         })

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() {