Merge pull request #1805 from overleaf/csh-issue-1457-recurly-xml-builder

Don't Build Recurly XML Requests Manually

GitOrigin-RevId: bebe7f747715a33e681dc58fb89b529411a13860

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

@@ -32,31 +32,6 @@ module.exports = RecurlyWrapper = {
       x => x.url
     ) || 'https://api.recurly.com/v2',
 
-  _addressToXml(address) {
-    const allowedKeys = [
-      'address1',
-      'address2',
-      'city',
-      'country',
-      'state',
-      'zip',
-      'postal_code'
-    ]
-    let resultString = '<billing_info>\n'
-    for (let k in address) {
-      const v = address[k]
-      if (k === 'postal_code') {
-        k = 'zip'
-      }
-      if (v && Array.from(allowedKeys).includes(k)) {
-        resultString += `<${k}${k === 'address2' ? ' nil="nil"' : ''}>${v ||
-          ''}</${k}>\n`
-      }
-    }
-    resultString += '</billing_info>\n'
-    return resultString
-  },
-
   _paypal: {
     checkAccountExists(cache, next) {
       const { user } = cache
@@ -132,22 +107,22 @@ module.exports = RecurlyWrapper = {
         { user_id: user._id, recurly_token_id },
         'creating user in recurly'
       )
-      const requestBody = `\
-<account>
-	<account_code>${user._id}</account_code>
-	<email>${user.email}</email>
-	<first_name>${user.first_name}</first_name>
-	<last_name>${user.last_name}</last_name>
-	<address>
-		<address1>${address.address1}</address1>
-		<address2>${address.address2}</address2>
-		<city>${address.city || ''}</city>
-		<state>${address.state || ''}</state>
-		<zip>${address.zip || ''}</zip>
-		<country>${address.country}</country>
-	</address>
-</account>\
-`
+      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
+        }
+      }
+      const requestBody = RecurlyWrapper._buildXml('account', data)
+
       return RecurlyWrapper.apiRequest(
         {
           url: 'accounts',
@@ -194,11 +169,8 @@ module.exports = RecurlyWrapper = {
       if (!accountCode) {
         return next(new Error('no account code at createBillingInfo stage'))
       }
-      const requestBody = `\
-<billing_info>
-	<token_id>${recurly_token_id}</token_id>
-</billing_info>\
-`
+      const data = { token_id: recurly_token_id }
+      const requestBody = RecurlyWrapper._buildXml('billing_info', data)
       return RecurlyWrapper.apiRequest(
         {
           url: `accounts/${accountCode}/billing_info`,
@@ -252,7 +224,16 @@ module.exports = RecurlyWrapper = {
           new Error('no address in subscriptionDetails at setAddress stage')
         )
       }
-      const requestBody = RecurlyWrapper._addressToXml(address)
+      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)
+
       return RecurlyWrapper.apiRequest(
         {
           url: `accounts/${accountCode}/billing_info`,
@@ -292,16 +273,16 @@ module.exports = RecurlyWrapper = {
         { user_id: user._id, recurly_token_id },
         'creating subscription in recurly'
       )
-      const requestBody = `\
-<subscription>
-	<plan_code>${subscriptionDetails.plan_code}</plan_code>
-	<currency>${subscriptionDetails.currencyCode}</currency>
-	<coupon_code>${subscriptionDetails.coupon_code}</coupon_code>
-	<account>
-		<account_code>${user._id}</account_code>
-	</account>
-</subscription>\
-` // TODO: check account details and billing
+      const data = {
+        plan_code: subscriptionDetails.plan_code,
+        currency: subscriptionDetails.currencyCode,
+        coupon_code: subscriptionDetails.coupon_code,
+        account: {
+          account_code: user._id
+        }
+      }
+      const requestBody = RecurlyWrapper._buildXml('subscription', data)
+
       return RecurlyWrapper.apiRequest(
         {
           url: 'subscriptions',
@@ -388,22 +369,22 @@ module.exports = RecurlyWrapper = {
     recurly_token_id,
     callback
   ) {
-    const requestBody = `\
-<subscription>
-  <plan_code>${subscriptionDetails.plan_code}</plan_code>
-  <currency>${subscriptionDetails.currencyCode}</currency>
-  <coupon_code>${subscriptionDetails.coupon_code}</coupon_code>
-  <account>
-  	<account_code>${user._id}</account_code>
-  	<email>${user.email}</email>
-  	<first_name>${user.first_name}</first_name>
-  	<last_name>${user.last_name}</last_name>
-  	<billing_info>
-  		<token_id>${recurly_token_id}</token_id>
-  	</billing_info>
-  </account>
-</subscription>\
-`
+    const data = {
+      plan_code: subscriptionDetails.plan_code,
+      currency: subscriptionDetails.currencyCode,
+      coupon_code: subscriptionDetails.coupon_code,
+      account: {
+        account_code: user._id,
+        email: user.email,
+        first_name: user.first_name,
+        last_name: user.last_name,
+        billing_info: {
+          token_id: recurly_token_id
+        }
+      }
+    }
+    const requestBody = RecurlyWrapper._buildXml('subscription', data)
+
     return RecurlyWrapper.apiRequest(
       {
         url: 'subscriptions',
@@ -667,12 +648,12 @@ module.exports = RecurlyWrapper = {
       { subscriptionId, options },
       'telling recurly to update subscription'
     )
-    const requestBody = `\
-<subscription>
-  <plan_code>${options.plan_code}</plan_code>
-  <timeframe>${options.timeframe}</timeframe>
-</subscription>\
-`
+    const data = {
+      plan_code: options.plan_code,
+      timeframe: options.timeframe
+    }
+    const requestBody = RecurlyWrapper._buildXml('subscription', data)
+
     return RecurlyWrapper.apiRequest(
       {
         url: `subscriptions/${subscriptionId}`,
@@ -696,20 +677,19 @@ module.exports = RecurlyWrapper = {
     plan_code,
     callback
   ) {
-    const requestBody = `\
-<coupon>
-	<coupon_code>${coupon_code}</coupon_code>
-	<name>${name}</name>
-	<discount_type>dollars</discount_type>
-	<discount_in_cents>
-		<${currencyCode}>${discount_in_cents}</${currencyCode}>
-	</discount_in_cents>
-	<plan_codes>
-		<plan_code>${plan_code}</plan_code>
-	</plan_codes>
-	<applies_to_all_plans>false</applies_to_all_plans>
-</coupon>\
-`
+    const data = {
+      coupon_code,
+      name,
+      discount_type: 'dollars',
+      discount_in_cents: {},
+      plan_codes: {
+        plan_code
+      },
+      applies_to_all_plans: false
+    }
+    data.discount_in_cents[currencyCode] = discount_in_cents
+    const requestBody = RecurlyWrapper._buildXml('coupon', data)
+
     logger.log({ coupon_code, requestBody }, 'creating coupon')
     return RecurlyWrapper.apiRequest(
       {
@@ -787,12 +767,12 @@ module.exports = RecurlyWrapper = {
   },
 
   redeemCoupon(account_code, coupon_code, callback) {
-    const requestBody = `\
-<redemption>
-	<account_code>${account_code}</account_code>
-	<currency>USD</currency>
-</redemption>\
-`
+    const data = {
+      account_code,
+      currency: 'USD'
+    }
+    const requestBody = RecurlyWrapper._buildXml('redemption', data)
+
     logger.log(
       { account_code, coupon_code, requestBody },
       'redeeming coupon for user'
@@ -969,6 +949,19 @@ module.exports = RecurlyWrapper = {
       const result = convertDataTypes(data)
       return callback(null, result)
     })
+  },
+
+  _buildXml(rootName, data) {
+    const options = {
+      headless: true,
+      renderOpts: {
+        pretty: true,
+        indent: '\t'
+      },
+      rootName
+    }
+    const builder = new xml2js.Builder(options)
+    return builder.buildObject(data)
   }
 }
 

services/web/npm-shrinkwrap.json

@@ -20240,11 +20240,19 @@
       }
     },
     "xml2js": {
-      "version": "0.2.0",
-      "resolved": "https://registry.npmjs.org/xml2js/-/xml2js-0.2.0.tgz",
-      "integrity": "sha1-99pSJ33rtkeYMFOtti2XLe5loaw=",
+      "version": "0.4.19",
+      "resolved": "https://registry.npmjs.org/xml2js/-/xml2js-0.4.19.tgz",
+      "integrity": "sha512-esZnJZJOiJR9wWKMyuvSE1y6Dq5LCuJanqhxslH2bxM6duahNZ+HMpCLhBQGZkbX6xRf8x1Y2eJlgt2q3qo49Q==",
       "requires": {
-        "sax": ">=0.1.1"
+        "sax": ">=0.6.0",
+        "xmlbuilder": "~9.0.1"
+      },
+      "dependencies": {
+        "xmlbuilder": {
+          "version": "9.0.7",
+          "resolved": "https://registry.npmjs.org/xmlbuilder/-/xmlbuilder-9.0.7.tgz",
+          "integrity": "sha1-Ey7mPS7FVlxVfiD0wi35rKaGsQ0="
+        }
       }
     },
     "xmlbuilder": {

services/web/package.json

@@ -108,7 +108,7 @@
     "uuid": "^3.0.1",
     "v8-profiler-node8": "^6.0.1",
     "valid-url": "^1.0.9",
-    "xml2js": "0.2.0",
+    "xml2js": "^0.4.19",
     "xregexp": "^4.2.4",
     "yauzl": "^2.10.0"
   },

services/web/test/unit/src/Subscription/RecurlyWrapperTests.js

@@ -151,7 +151,8 @@ describe('RecurlyWrapper', function() {
             error: sinon.stub(),
             log: sinon.stub()
           },
-          request: sinon.stub()
+          request: sinon.stub(),
+          xml2js: require('xml2js')
         }
       }
     ))
@@ -280,12 +281,13 @@ describe('RecurlyWrapper', function() {
       return this.RecurlyWrapper.apiRequest.restore()
     })
 
-    it('should send an update request to the API', function() {
+    it('sends correct XML', function() {
       this.apiRequest.called.should.equal(true)
-      this.requestOptions.body.should.equal(`\
+      const { body } = this.apiRequest.lastCall.args[0]
+      expect(body).to.equal(`\
 <subscription>
-  <plan_code>silver</plan_code>
-  <timeframe>now</timeframe>
+	<plan_code>silver</plan_code>
+	<timeframe>now</timeframe>
 </subscription>\
 `)
       this.requestOptions.url.should.equal(
@@ -395,10 +397,6 @@ describe('RecurlyWrapper', function() {
         'apiRequest',
         (options, callback) => {
           options.url.should.equal(`coupons/${this.coupon_code}/redeem`)
-          options.body
-            .indexOf(`<account_code>${this.recurlyAccountId}</account_code>`)
-            .should.not.equal(-1)
-          options.body.indexOf('<currency>USD</currency>').should.not.equal(-1)
           options.method.should.equal('post')
           return callback()
         }
@@ -414,37 +412,63 @@ describe('RecurlyWrapper', function() {
       return this.RecurlyWrapper.apiRequest.restore()
     })
 
-    return it('should send the request to redem the coupon', function() {
-      return this.apiRequest.called.should.equal(true)
+    return it('sends correct XML', function() {
+      this.apiRequest.called.should.equal(true)
+      const { body } = this.apiRequest.lastCall.args[0]
+      return expect(body).to.equal(`\
+<redemption>
+	<account_code>account-id-123</account_code>
+	<currency>USD</currency>
+</redemption>\
+`)
     })
   })
 
-  describe('_addressToXml', function() {
-    beforeEach(function() {
-      return (this.address = {
-        address1: 'addr_one',
-        address2: 'addr_two',
-        country: 'some_country',
-        state: 'some_state',
-        postal_code: 'some_zip',
-        nonsenseKey: 'rubbish'
-      })
+  describe('createFixedAmmountCoupon', function() {
+    beforeEach(function(done) {
+      this.couponCode = 'a-coupon-code'
+      this.couponName = 'a-coupon-name'
+      this.currencyCode = 'EUR'
+      this.discount = 1337
+      this.planCode = 'a-plan-code'
+      this.apiRequest = sinon.stub(
+        this.RecurlyWrapper,
+        'apiRequest',
+        (options, callback) => {
+          return callback()
+        }
+      )
+      return this.RecurlyWrapper.createFixedAmmountCoupon(
+        this.couponCode,
+        this.couponName,
+        this.currencyCode,
+        this.discount,
+        this.planCode,
+        done
+      )
     })
 
-    return it('should generate the correct xml', function() {
-      const result = this.RecurlyWrapper._addressToXml(this.address)
-      return should.equal(
-        result,
-        `\
-<billing_info>
-<address1>addr_one</address1>
-<address2 nil="nil">addr_two</address2>
-<country>some_country</country>
-<state>some_state</state>
-<zip>some_zip</zip>
-</billing_info>\n\
-`
-      )
+    afterEach(function() {
+      return this.RecurlyWrapper.apiRequest.restore()
+    })
+
+    return it('sends correct XML', function() {
+      this.apiRequest.called.should.equal(true)
+      const { body } = this.apiRequest.lastCall.args[0]
+      return expect(body).to.equal(`\
+<coupon>
+	<coupon_code>a-coupon-code</coupon_code>
+	<name>a-coupon-name</name>
+	<discount_type>dollars</discount_type>
+	<discount_in_cents>
+		<EUR>1337</EUR>
+	</discount_in_cents>
+	<plan_codes>
+		<plan_code>a-plan-code</plan_code>
+	</plan_codes>
+	<applies_to_all_plans>false</applies_to_all_plans>
+</coupon>\
+`)
     })
   })
 
@@ -639,6 +663,29 @@ describe('RecurlyWrapper', function() {
       return this._parseSubscriptionXml.restore()
     })
 
+    it('sends correct XML', function(done) {
+      return this.call((err, result) => {
+        const { body } = this.apiRequest.lastCall.args[0]
+        expect(body).to.equal(`\
+<subscription>
+	<plan_code>some_plan_code</plan_code>
+	<currency>EUR</currency>
+	<coupon_code/>
+	<account>
+		<account_code>some_id</account_code>
+		<email>user@example.com</email>
+		<first_name/>
+		<last_name/>
+		<billing_info>
+			<token_id>a-token-id</token_id>
+		</billing_info>
+	</account>
+</subscription>\
+`)
+        return done()
+      })
+    })
+
     it('should not produce an error', function(done) {
       return this.call((err, sub) => {
         expect(err).to.not.be.instanceof(Error)
@@ -875,7 +922,12 @@ describe('RecurlyWrapper', function() {
         '_parseSubscriptionXml'
       )
       return (this.cache = {
-        user: (this.user = { _id: 'some_id' }),
+        user: (this.user = {
+          _id: 'some_id',
+          email: 'foo@bar.com',
+          first_name: 'Foo',
+          last_name: 'Bar'
+        }),
         recurly_token_id: (this.recurly_token_id = 'some_token'),
         subscriptionDetails: (this.subscriptionDetails = {
           currencyCode: 'EUR',
@@ -885,6 +937,7 @@ describe('RecurlyWrapper', function() {
           address: {
             address1: 'addr_one',
             address2: 'addr_two',
+            city: 'some_city',
             country: 'some_country',
             state: 'some_state',
             zip: 'some_zip'
@@ -1091,6 +1144,29 @@ describe('RecurlyWrapper', function() {
           )
         })
 
+        it('sends correct XML', function(done) {
+          return this.call((err, result) => {
+            const { body } = this.apiRequest.lastCall.args[0]
+            expect(body).to.equal(`\
+<account>
+	<account_code>some_id</account_code>
+	<email>foo@bar.com</email>
+	<first_name>Foo</first_name>
+	<last_name>Bar</last_name>
+	<address>
+		<address1>addr_one</address1>
+		<address2>addr_two</address2>
+		<city>some_city</city>
+		<state>some_state</state>
+		<zip>some_zip</zip>
+		<country>some_country</country>
+	</address>
+</account>\
+`)
+            return done()
+          })
+        })
+
         it('should not produce an error', function(done) {
           return this.call((err, result) => {
             expect(err).to.not.be.instanceof(Error)
@@ -1165,6 +1241,18 @@ describe('RecurlyWrapper', function() {
           )
         })
 
+        it('sends correct XML', function(done) {
+          return this.call((err, result) => {
+            const { body } = this.apiRequest.lastCall.args[0]
+            expect(body).to.equal(`\
+<billing_info>
+	<token_id>some_token</token_id>
+</billing_info>\
+`)
+            return done()
+          })
+        })
+
         it('should not produce an error', function(done) {
           return this.call((err, result) => {
             expect(err).to.not.be.instanceof(Error)
@@ -1259,6 +1347,23 @@ describe('RecurlyWrapper', function() {
           )
         })
 
+        it('sends correct XML', function(done) {
+          return this.call((err, result) => {
+            const { body } = this.apiRequest.lastCall.args[0]
+            expect(body).to.equal(`\
+<billing_info>
+	<address1>addr_one</address1>
+	<address2>addr_two</address2>
+	<city>some_city</city>
+	<state>some_state</state>
+	<zip>some_zip</zip>
+	<country>some_country</country>
+</billing_info>\
+`)
+            return done()
+          })
+        })
+
         it('should not produce an error', function(done) {
           return this.call((err, result) => {
             expect(err).to.not.be.instanceof(Error)
@@ -1330,6 +1435,23 @@ describe('RecurlyWrapper', function() {
           )
         })
 
+        it('sends correct XML', function(done) {
+          return this.call((err, result) => {
+            const { body } = this.apiRequest.lastCall.args[0]
+            expect(body).to.equal(`\
+<subscription>
+	<plan_code>some_plan_code</plan_code>
+	<currency>EUR</currency>
+	<coupon_code/>
+	<account>
+		<account_code>some_id</account_code>
+	</account>
+</subscription>\
+`)
+            return done()
+          })
+        })
+
         it('should not produce an error', function(done) {
           return this.call((err, result) => {
             expect(err).to.not.be.instanceof(Error)