From e000fd4615fcc84d3794ec121ff8f04e0e7abcbf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Alby?= Date: Tue, 12 Nov 2019 15:56:08 +0700 Subject: [PATCH] Merge pull request #2343 from overleaf/ta-invoice-attempt-callback Collect Past Due Invoices on Paypal Billing Info Updates GitOrigin-RevId: 6a0d298db8589ae6ba7cb62e4dfd562a1f292db0 --- .../Features/Subscription/RecurlyWrapper.js | 49 +++++++--- .../Subscription/SubscriptionController.js | 14 ++- .../Subscription/SubscriptionHandler.js | 34 ++++++- .../collect_paypal_past_due_invoice.js | 90 +++++++++++++++++++ .../recurly/get_paypal_accounts_csv.js | 20 +++-- .../SubscriptionControllerTests.js | 43 ++++++++- .../Subscription/SubscriptionHandlerTests.js | 63 ++++++++++++- 7 files changed, 284 insertions(+), 29 deletions(-) create mode 100644 services/web/scripts/recurly/collect_paypal_past_due_invoice.js diff --git a/services/web/app/src/Features/Subscription/RecurlyWrapper.js b/services/web/app/src/Features/Subscription/RecurlyWrapper.js index 3252827ed3..b80499f6e4 100644 --- a/services/web/app/src/Features/Subscription/RecurlyWrapper.js +++ b/services/web/app/src/Features/Subscription/RecurlyWrapper.js @@ -529,12 +529,12 @@ module.exports = RecurlyWrapper = { ) }, - getAccounts(queryParams, callback) { + getPaginatedEndpoint(resource, queryParams, callback) { queryParams.per_page = queryParams.per_page || 200 - let allAccounts = [] - var getPageOfAccounts = (cursor = null) => { + let allItems = [] + var getPage = (cursor = null) => { const opts = { - url: 'accounts', + url: resource, qs: queryParams } if (cursor != null) { @@ -549,11 +549,10 @@ module.exports = RecurlyWrapper = { logger.warn({ err }, 'could not get accoutns') callback(err) } - allAccounts = allAccounts.concat(data.accounts) + const items = data[resource] + allItems = allItems.concat(items) logger.log( - `got another ${data.accounts.length}, total now ${ - allAccounts.length - }` + `got another ${items.length}, total now ${allItems.length}` ) cursor = __guard__( response.headers.link != null @@ -563,15 +562,15 @@ module.exports = RecurlyWrapper = { ) if (cursor != null) { cursor = decodeURIComponent(cursor) - return getPageOfAccounts(cursor) + return getPage(cursor) } else { - return callback(err, allAccounts) + return callback(err, allItems) } }) }) } - return getPageOfAccounts() + return getPage() }, getAccount(accountId, callback) { @@ -645,6 +644,30 @@ module.exports = RecurlyWrapper = { ) }, + getAccountPastDueInvoices(accountId, callback) { + RecurlyWrapper.apiRequest( + { + url: `accounts/${accountId}/invoices?state=past_due` + }, + (error, response, body) => { + if (error) { + return callback(error) + } + RecurlyWrapper._parseInvoicesXml(body, callback) + } + ) + }, + + attemptInvoiceCollection(invoiceId, callback) { + RecurlyWrapper.apiRequest( + { + url: `invoices/${invoiceId}/collect`, + method: 'put' + }, + callback + ) + }, + updateSubscription(subscriptionId, options, callback) { logger.log( { subscriptionId, options }, @@ -926,6 +949,10 @@ module.exports = RecurlyWrapper = { return RecurlyWrapper._parseXmlAndGetAttribute(xml, 'errors', callback) }, + _parseInvoicesXml(xml, callback) { + return RecurlyWrapper._parseXmlAndGetAttribute(xml, 'invoices', callback) + }, + _parseXmlAndGetAttribute(xml, attribute, callback) { return RecurlyWrapper._parseXml(xml, function(error, data) { if (error != null) { diff --git a/services/web/app/src/Features/Subscription/SubscriptionController.js b/services/web/app/src/Features/Subscription/SubscriptionController.js index eda63dfbdd..0f11563f45 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionController.js +++ b/services/web/app/src/Features/Subscription/SubscriptionController.js @@ -356,7 +356,6 @@ module.exports = SubscriptionController = { recurlyCallback(req, res, next) { logger.log({ data: req.body }, 'received recurly callback') - // we only care if a subscription has exipired const event = Object.keys(req.body)[0] const eventData = req.body[event] if ( @@ -367,7 +366,7 @@ module.exports = SubscriptionController = { ].includes(event) ) { const recurlySubscription = eventData.subscription - return SubscriptionHandler.recurlyCallback( + return SubscriptionHandler.syncSubscription( recurlySubscription, { ip: req.ip }, function(err) { @@ -377,6 +376,17 @@ module.exports = SubscriptionController = { return res.sendStatus(200) } ) + } else if (event === 'billing_info_updated_notification') { + const recurlyAccountCode = eventData.account.account_code + return SubscriptionHandler.attemptPaypalInvoiceCollection( + recurlyAccountCode, + function(err) { + if (err) { + return next(err) + } + return res.sendStatus(200) + } + ) } else { return res.sendStatus(200) } diff --git a/services/web/app/src/Features/Subscription/SubscriptionHandler.js b/services/web/app/src/Features/Subscription/SubscriptionHandler.js index 2e5f30aec4..cb7cd4877c 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionHandler.js +++ b/services/web/app/src/Features/Subscription/SubscriptionHandler.js @@ -230,7 +230,7 @@ const SubscriptionHandler = { }) }, - recurlyCallback(recurlySubscription, requesterData, callback) { + syncSubscription(recurlySubscription, requesterData, callback) { return RecurlyWrapper.getSubscription( recurlySubscription.uuid, { includeAccount: true }, @@ -259,6 +259,38 @@ const SubscriptionHandler = { ) }, + // attempt to collect past due invoice for customer. Only do that when a) the + // customer is using Paypal and b) there is only one past due invoice. + // This is used because Recurly doesn't always attempt collection of paast due + // invoices after Paypal billing info were updated. + attemptPaypalInvoiceCollection(recurlyAccountCode, callback) { + RecurlyWrapper.getBillingInfo(recurlyAccountCode, (error, billingInfo) => { + if (error) { + return callback(error) + } + if (!billingInfo.paypal_billing_agreement_id) { + // this is not a Paypal user + return callback() + } + RecurlyWrapper.getAccountPastDueInvoices( + recurlyAccountCode, + (error, pastDueInvoices) => { + if (error) { + return callback(error) + } + if (pastDueInvoices.length !== 1) { + // no past due invoices, or more than one. Ignore. + return callback() + } + RecurlyWrapper.attemptInvoiceCollection( + pastDueInvoices[0].invoice_number, + callback + ) + } + ) + }) + }, + extendTrial(subscription, daysToExend, callback) { return RecurlyWrapper.extendTrial( subscription.recurlySubscription_id, diff --git a/services/web/scripts/recurly/collect_paypal_past_due_invoice.js b/services/web/scripts/recurly/collect_paypal_past_due_invoice.js new file mode 100644 index 0000000000..ec91cb727e --- /dev/null +++ b/services/web/scripts/recurly/collect_paypal_past_due_invoice.js @@ -0,0 +1,90 @@ +const RecurlyWrapper = require('../../app/src/Features/Subscription/RecurlyWrapper') +const async = require('async') +const minimist = require('minimist') + +const slowCallback = (callback, error, data) => + setTimeout(() => callback(error, data), 80) + +const handleAPIError = (source, id, error, callback) => { + console.warn(`Errors in ${source} with id=${id}`, error) + if (typeof error === 'string' && error.match(/429$/)) { + return setTimeout(callback, 1000 * 60 * 5) + } + slowCallback(callback) +} + +const attemptInvoiceCollection = (invoice, callback) => { + isAccountUsingPaypal(invoice, (error, isPaypal) => { + if (error || !isPaypal) { + return callback(error) + } + const accountId = invoice.account.url.match(/accounts\/(.*)/)[1] + if (USERS_COLLECTED.indexOf(accountId) > -1) { + console.warn(`Skipping duplicate user ${accountId}`) + return callback() + } + INVOICES_COLLECTED.push(invoice.invoice_number) + USERS_COLLECTED.push(accountId) + if (DRY_RUN) { + return callback() + } + RecurlyWrapper.attemptInvoiceCollection( + invoice.invoice_number, + (error, response) => { + if (error) { + return handleAPIError( + 'attemptInvoiceCollection', + invoice.invoice_number, + error, + callback + ) + } + slowCallback(callback, null) + } + ) + }) +} + +const isAccountUsingPaypal = (invoice, callback) => { + const accountId = invoice.account.url.match(/accounts\/(.*)/)[1] + RecurlyWrapper.getBillingInfo(accountId, (error, response) => { + if (error) { + return handleAPIError('billing info', accountId, error, callback) + } + if (response.billing_info.paypal_billing_agreement_id) { + return slowCallback(callback, null, true) + } + slowCallback(callback, null, false) + }) +} + +const attemptInvoicesCollection = callback => { + RecurlyWrapper.getPaginatedEndpoint( + 'invoices', + { state: 'past_due' }, + (error, invoices) => { + console.log('invoices', invoices.length) + if (error) { + return callback(error) + } + async.eachSeries(invoices, attemptInvoiceCollection, callback) + } + ) +} + +const argv = minimist(process.argv.slice(2)) +const DRY_RUN = argv.n !== undefined +const INVOICES_COLLECTED = [] +const USERS_COLLECTED = [] +attemptInvoicesCollection(error => { + if (error) { + throw error + } + console.log( + `DONE (DRY_RUN=${DRY_RUN}). ${ + INVOICES_COLLECTED.length + } invoices collected for ${USERS_COLLECTED.length} users.` + ) + console.log({ INVOICES_COLLECTED, USERS_COLLECTED }) + process.exit() +}) diff --git a/services/web/scripts/recurly/get_paypal_accounts_csv.js b/services/web/scripts/recurly/get_paypal_accounts_csv.js index 60753aeeda..491bdfb437 100644 --- a/services/web/scripts/recurly/get_paypal_accounts_csv.js +++ b/services/web/scripts/recurly/get_paypal_accounts_csv.js @@ -73,15 +73,19 @@ const printAccountCSV = (account, callback) => { } const printAccountsCSV = callback => { - RecurlyWrapper.getAccounts({ state: 'subscriber' }, (error, accounts) => { - if (error) { - return callback(error) + RecurlyWrapper.getPaginatedEndpoint( + 'accounts', + { state: 'subscriber' }, + (error, accounts) => { + if (error) { + return callback(error) + } + async.mapSeries(accounts, printAccountCSV, (error, csvData) => { + csvData = csvData.filter(d => !!d) + callback(error, csvData) + }) } - async.mapSeries(accounts, printAccountCSV, (error, csvData) => { - csvData = csvData.filter(d => !!d) - callback(error, csvData) - }) - }) + ) } const csvFields = [ diff --git a/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js b/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js index eb009a9013..3f5a1aa1b5 100644 --- a/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js +++ b/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js @@ -60,7 +60,8 @@ describe('SubscriptionController', function() { updateSubscription: sinon.stub().callsArgWith(3), reactivateSubscription: sinon.stub().callsArgWith(1), cancelSubscription: sinon.stub().callsArgWith(1), - recurlyCallback: sinon.stub().yields(), + syncSubscription: sinon.stub().yields(), + attemptPaypalInvoiceCollection: sinon.stub().yields(), startFreeTrial: sinon.stub() } @@ -514,7 +515,7 @@ describe('SubscriptionController', function() { }) describe('recurly callback', function() { - describe('with a actionable request', function() { + describe('with a sync subscription request', function() { beforeEach(function(done) { this.req = { body: { @@ -535,7 +536,7 @@ describe('SubscriptionController', function() { }) it('should tell the SubscriptionHandler to process the recurly callback', function(done) { - this.SubscriptionHandler.recurlyCallback.called.should.equal(true) + this.SubscriptionHandler.syncSubscription.called.should.equal(true) return done() }) @@ -545,6 +546,39 @@ describe('SubscriptionController', function() { }) }) + describe('with a billing info updated request', function() { + beforeEach(function(done) { + this.req = { + body: { + billing_info_updated_notification: { + account: { + account_code: 'mock-account-code' + } + } + } + } + this.res = { + sendStatus() { + done() + } + } + sinon.spy(this.res, 'sendStatus') + this.SubscriptionController.recurlyCallback(this.req, this.res) + }) + + it('should call attemptPaypalInvoiceCollection', function(done) { + this.SubscriptionHandler.attemptPaypalInvoiceCollection + .calledWith('mock-account-code') + .should.equal(true) + done() + }) + + it('should send a 200', function(done) { + this.res.sendStatus.calledWith(200) + done() + }) + }) + describe('with a non-actionable request', function() { beforeEach(function(done) { this.user.id = this.activeRecurlySubscription.account.account_code @@ -567,7 +601,8 @@ describe('SubscriptionController', function() { }) it('should not call the subscriptionshandler', function() { - return this.SubscriptionHandler.recurlyCallback.called.should.equal( + this.SubscriptionHandler.syncSubscription.called.should.equal(false) + this.SubscriptionHandler.attemptPaypalInvoiceCollection.called.should.equal( false ) }) diff --git a/services/web/test/unit/src/Subscription/SubscriptionHandlerTests.js b/services/web/test/unit/src/Subscription/SubscriptionHandlerTests.js index 986a2e3660..979cce15b1 100644 --- a/services/web/test/unit/src/Subscription/SubscriptionHandlerTests.js +++ b/services/web/test/unit/src/Subscription/SubscriptionHandlerTests.js @@ -70,7 +70,10 @@ describe('SubscriptionHandler', function() { redeemCoupon: sinon.stub().callsArgWith(2), createSubscription: sinon .stub() - .callsArgWith(3, null, this.activeRecurlySubscription) + .callsArgWith(3, null, this.activeRecurlySubscription), + getBillingInfo: sinon.stub().yields(), + getAccountPastDueInvoices: sinon.stub().yields(), + attemptInvoiceCollection: sinon.stub().yields() } this.DropboxHandler = { unlinkAccount: sinon.stub().callsArgWith(1) } @@ -380,7 +383,7 @@ describe('SubscriptionHandler', function() { }) }) - describe('recurlyCallback', function() { + describe('syncSubscription', function() { describe('with an actionable request', function() { beforeEach(function(done) { this.user.id = this.activeRecurlySubscription.account.account_code @@ -389,7 +392,7 @@ describe('SubscriptionHandler', function() { userId.should.equal(this.user.id) return callback(null, this.user) } - return this.SubscriptionHandler.recurlyCallback( + return this.SubscriptionHandler.syncSubscription( this.activeRecurlySubscription, {}, done @@ -419,6 +422,60 @@ describe('SubscriptionHandler', function() { }) }) + describe('attemptPaypalInvoiceCollection', function() { + describe('for credit card users', function() { + beforeEach(function(done) { + this.RecurlyWrapper.getBillingInfo.yields(null, { + paypal_billing_agreement_id: null + }) + this.SubscriptionHandler.attemptPaypalInvoiceCollection( + this.activeRecurlySubscription.account.account_code, + done + ) + }) + + it('gets billing infos', function() { + sinon.assert.calledWith( + this.RecurlyWrapper.getBillingInfo, + this.activeRecurlySubscription.account.account_code + ) + }) + + it('skips user', function() { + sinon.assert.notCalled(this.RecurlyWrapper.getAccountPastDueInvoices) + }) + }) + + describe('for paypal users', function() { + beforeEach(function(done) { + this.RecurlyWrapper.getBillingInfo.yields(null, { + paypal_billing_agreement_id: 'mock-billing-agreement' + }) + this.RecurlyWrapper.getAccountPastDueInvoices.yields(null, [ + { invoice_number: 'mock-invoice-number' } + ]) + this.SubscriptionHandler.attemptPaypalInvoiceCollection( + this.activeRecurlySubscription.account.account_code, + done + ) + }) + + it('gets past due invoices', function() { + sinon.assert.calledWith( + this.RecurlyWrapper.getAccountPastDueInvoices, + this.activeRecurlySubscription.account.account_code + ) + }) + + it('calls attemptInvoiceCollection', function() { + sinon.assert.calledWith( + this.RecurlyWrapper.attemptInvoiceCollection, + 'mock-invoice-number' + ) + }) + }) + }) + describe('validateNoSubscriptionInRecurly', function() { beforeEach(function() { this.subscriptions = []