From 554be73a361e37a7bd2a515ff3f085a1ddbd491e Mon Sep 17 00:00:00 2001 From: Antoine Clausse Date: Thu, 23 May 2024 09:06:46 +0200 Subject: [PATCH] In `collect_paypal_past_due_invoice.js`, iterate over each page instead of gathering data from all pages at first (#18414) * Create `getPaginatedEndpointIterator` to iterate each page * Create `waitMs` util, it will replace `slowCallback` * Make `handleAPIError` async * Make `isAccountUsingPaypal` async * Make `attemptInvoiceCollection` async * Make `attemptInvoicesCollection` async * Use `await` instead of `new Promise` * Remove unused callbackified `attemptInvoiceCollection` * Run `attemptInvoiceCollection` for each page instead of gathering all pages in the beginning * Add test on fetching multiple pages of invoice GitOrigin-RevId: 2674b18c6ca5732b873fb2bc71b515909006f93d --- .../Features/Subscription/RecurlyWrapper.js | 51 +++--- .../collect_paypal_past_due_invoice.js | 164 +++++++++--------- .../src/CollectPayPalPastDueInvoiceTest.js | 95 +++++++--- 3 files changed, 183 insertions(+), 127 deletions(-) diff --git a/services/web/app/src/Features/Subscription/RecurlyWrapper.js b/services/web/app/src/Features/Subscription/RecurlyWrapper.js index dc0d5ed4ca..8a9e62f7b2 100644 --- a/services/web/app/src/Features/Subscription/RecurlyWrapper.js +++ b/services/web/app/src/Features/Subscription/RecurlyWrapper.js @@ -467,9 +467,32 @@ const promises = { } }, + /** + * @typedef {{getNextPage: () => Promise, items: any[]}} PageData + */ + async getPaginatedEndpoint(resource, queryParams) { - queryParams.per_page = queryParams.per_page || 200 let allItems = [] + let items + + /** @type {() => Promise} */ + let getNextPage = promises.getPaginatedEndpointIterator( + resource, + queryParams + ) + while (getNextPage) { + ;({ items, getNextPage } = await getNextPage()) + allItems = allItems.concat(items) + logger.debug(`total now ${allItems.length}`) + } + return allItems + }, + + /** + * @returns {() => Promise} + */ + getPaginatedEndpointIterator(resource, queryParams) { + queryParams.per_page = queryParams.per_page || 200 const getPage = async (cursor = null) => { const opts = { url: resource, @@ -483,21 +506,16 @@ const promises = { const data = await RecurlyWrapper.promises._parseXml(body) const items = data[resource] - allItems = allItems.concat(items) - logger.debug(`got another ${items.length}, total now ${allItems.length}`) + logger.debug(`got ${items.length} items in this page`) const match = response.headers.link?.match(/cursor=([0-9.]+%3A[0-9.]+)&/) - cursor = match && match[1] - if (cursor) { - cursor = decodeURIComponent(cursor) - return getPage(cursor) - } else { - return allItems + const nextCursor = match && match[1] + return { + items, + getNextPage: + nextCursor && (() => getPage(decodeURIComponent(nextCursor))), } } - - await getPage() - - return allItems + return getPage }, async getAccount(accountId) { @@ -860,13 +878,6 @@ const RecurlyWrapper = { apiUrl: Settings.apis.recurly.url || 'https://api.recurly.com/v2', _buildXml, _parseXml: callbackify(promises._parseXml), - // This one needs to be callbackified manually because we need to transform {response, body} to (err, response, body) - attemptInvoiceCollection: (invoiceId, callback) => { - promises - .attemptInvoiceCollection(invoiceId) - .then(({ response, body }) => callback(null, response, body)) - .catch(err => callback(err)) - }, createFixedAmmountCoupon: callbackify(promises.createFixedAmmountCoupon), getAccountActiveCoupons: callbackify(promises.getAccountActiveCoupons), getBillingInfo: callbackify(promises.getBillingInfo), diff --git a/services/web/scripts/recurly/collect_paypal_past_due_invoice.js b/services/web/scripts/recurly/collect_paypal_past_due_invoice.js index 9dbe32744b..00007eb266 100644 --- a/services/web/scripts/recurly/collect_paypal_past_due_invoice.js +++ b/services/web/scripts/recurly/collect_paypal_past_due_invoice.js @@ -1,20 +1,19 @@ const RecurlyWrapper = require('../../app/src/Features/Subscription/RecurlyWrapper') -const async = require('async') const minimist = require('minimist') const logger = require('@overleaf/logger') -const slowCallback = +const waitMs = require.main === module - ? (callback, error, data) => setTimeout(() => callback(error, data), 80) - : (callback, error, data) => callback(error, data) + ? timeout => new Promise(resolve => setTimeout(() => resolve(), timeout)) + : () => Promise.resolve() // NOTE: Errors are not propagated to the caller -const handleAPIError = (source, id, error, callback) => { +const handleAPIError = async (source, id, error) => { logger.warn(`Errors in ${source} with id=${id}`, error) if (typeof error === 'string' && error.match(/429$/)) { - return setTimeout(callback, 1000 * 60 * 5) + return waitMs(1000 * 60 * 5) } - slowCallback(callback) + await waitMs(80) } /** @@ -25,100 +24,95 @@ const handleAPIError = (source, id, error, callback) => { * }>} */ const main = async () => { - 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) { - logger.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 - ) - } - INVOICES_COLLECTED_SUCCESS.push(invoice.invoice_number) - slowCallback(callback, null) - } - ) - }) - } + const attemptInvoiceCollection = async invoice => { + const isPaypal = await isAccountUsingPaypal(invoice) - const isAccountUsingPaypal = (invoice, callback) => { + if (!isPaypal) { + return + } 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) - }) + if (USERS_COLLECTED.indexOf(accountId) > -1) { + logger.warn(`Skipping duplicate user ${accountId}`) + return + } + INVOICES_COLLECTED.push(invoice.invoice_number) + USERS_COLLECTED.push(accountId) + if (DRY_RUN) { + return + } + try { + await RecurlyWrapper.promises.attemptInvoiceCollection( + invoice.invoice_number + ) + INVOICES_COLLECTED_SUCCESS.push(invoice.invoice_number) + await waitMs(80) + } catch (error) { + return handleAPIError( + 'attemptInvoiceCollection', + invoice.invoice_number, + error + ) + } } - const attemptInvoicesCollection = callback => { - RecurlyWrapper.getPaginatedEndpoint( - 'invoices', - { state: 'past_due' }, - (error, invoices) => { - logger.info('invoices', invoices?.length) - if (error) { - return callback(error) - } - async.eachSeries(invoices, attemptInvoiceCollection, callback) - } - ) + const isAccountUsingPaypal = async invoice => { + const accountId = invoice.account.url.match(/accounts\/(.*)/)[1] + try { + const response = await RecurlyWrapper.promises.getBillingInfo(accountId) + await waitMs(80) + return !!response.billing_info.paypal_billing_agreement_id + } catch (error) { + return handleAPIError('billing info', accountId, error) + } } + + const attemptInvoicesCollection = async () => { + let getPage = await RecurlyWrapper.promises.getPaginatedEndpointIterator( + 'invoices', + { state: 'past_due' } + ) + + while (getPage) { + const { items, getNextPage } = await getPage() + logger.info('invoices', items?.length) + for (const invoice of items) { + await attemptInvoiceCollection(invoice) + } + getPage = getNextPage + } + } + const argv = minimist(process.argv.slice(2)) const DRY_RUN = argv.n !== undefined const INVOICES_COLLECTED = [] const INVOICES_COLLECTED_SUCCESS = [] const USERS_COLLECTED = [] - return new Promise((resolve, reject) => { - attemptInvoicesCollection(error => { - logger.info( - `DONE (DRY_RUN=${DRY_RUN}). ${INVOICES_COLLECTED.length} invoices collection attempts for ${USERS_COLLECTED.length} users. ${INVOICES_COLLECTED_SUCCESS.length} successful collections` - ) - console.dir( - { - INVOICES_COLLECTED, - INVOICES_COLLECTED_SUCCESS, - USERS_COLLECTED, - }, - { maxArrayLength: null } - ) + try { + await attemptInvoicesCollection() - if (error) { - reject(error) - } + if (INVOICES_COLLECTED_SUCCESS.length === 0) { + throw new Error('No invoices collected') + } - if (INVOICES_COLLECTED_SUCCESS.length === 0) { - reject(new Error('No invoices collected')) - } - - resolve({ + return { + INVOICES_COLLECTED, + INVOICES_COLLECTED_SUCCESS, + USERS_COLLECTED, + } + } finally { + logger.info( + `DONE (DRY_RUN=${DRY_RUN}). ${INVOICES_COLLECTED.length} invoices collection attempts for ${USERS_COLLECTED.length} users. ${INVOICES_COLLECTED_SUCCESS.length} successful collections` + ) + console.dir( + { INVOICES_COLLECTED, INVOICES_COLLECTED_SUCCESS, USERS_COLLECTED, - }) - }) - }) + }, + { maxArrayLength: null } + ) + } } if (require.main === module) { diff --git a/services/web/test/acceptance/src/CollectPayPalPastDueInvoiceTest.js b/services/web/test/acceptance/src/CollectPayPalPastDueInvoiceTest.js index cfb200d0cf..e0689b850c 100644 --- a/services/web/test/acceptance/src/CollectPayPalPastDueInvoiceTest.js +++ b/services/web/test/acceptance/src/CollectPayPalPastDueInvoiceTest.js @@ -155,34 +155,55 @@ const invoiceCollectXml = ` ` +const ITEMS_PER_PAGE = 3 + +const getInvoicePage = fullInvoicesIds => queryOptions => { + const cursor = queryOptions.qs.cursor + const startEnd = cursor?.split(':').map(Number) || [] + const start = startEnd[0] || 0 + const end = startEnd[1] || ITEMS_PER_PAGE + const body = invoicesXml(fullInvoicesIds.slice(start, end)) + const hasMore = end < fullInvoicesIds.length + const nextPageCursor = hasMore ? `${end}%3A${end + ITEMS_PER_PAGE}&v=2` : null + const response = { + statusCode: 200, + headers: { + link: hasMore + ? `https://fakerecurly.com/v2/invoices?cursor=${nextPageCursor}` + : undefined, + }, + } + + return { response, body } +} + describe('CollectPayPalPastDueInvoice', function () { let apiRequestStub - const fakeApiRequests = invoiceIdsAndReturnCode => { + const fakeApiRequests = invoiceIds => { apiRequestStub = sinon.stub(RecurlyWrapper.promises, 'apiRequest') apiRequestStub.callsFake(options => { - switch (options.url) { - case 'invoices': - return { - response: { statusCode: 200, headers: {} }, - body: invoicesXml(invoiceIdsAndReturnCode), - } - case 'accounts/200/billing_info': - case 'accounts/404/billing_info': - return { - response: { statusCode: 200, headers: {} }, - body: billingInfoXml, - } - case 'invoices/200/collect': + if (options.url === 'invoices') { + return getInvoicePage(invoiceIds)(options) + } + + if (/accounts\/(\d+)\/billing_info/.test(options.url)) { + return { + response: { statusCode: 200, headers: {} }, + body: billingInfoXml, + } + } + + if (/invoices\/(\d+)\/collect/.test(options.url)) { + const invoiceId = options.url.match(/invoices\/(\d+)\/collect/)[1] + if (invoiceId < 400) { return { response: { statusCode: 200, headers: {} }, body: invoiceCollectXml, } - case 'invoices/404/collect': - throw new OError(`Recurly API returned with status code: 404`, { - statusCode: 404, - }) - default: - throw new Error(`Unexpected URL: ${options.url}`) + } + throw new OError(`Recurly API returned with status code: 404`, { + statusCode: 404, + }) } }) } @@ -194,13 +215,43 @@ describe('CollectPayPalPastDueInvoice', function () { it('collects one valid invoice', async function () { fakeApiRequests([200]) const r = await main() - await expect(r).to.eql({ + expect(r).to.eql({ INVOICES_COLLECTED: [200], INVOICES_COLLECTED_SUCCESS: [200], USERS_COLLECTED: ['200'], }) }) + it('collects several pages', async function () { + // 10 invoices, from 200 to 209 + fakeApiRequests([...Array(10).keys()].map(i => i + 200)) + const r = await main() + + expect(r).to.eql({ + INVOICES_COLLECTED: [200, 201, 202, 203, 204, 205, 206, 207, 208, 209], + INVOICES_COLLECTED_SUCCESS: [ + 200, 201, 202, 203, 204, 205, 206, 207, 208, 209, + ], + USERS_COLLECTED: [ + '200', + '201', + '202', + '203', + '204', + '205', + '206', + '207', + '208', + '209', + ], + }) + + // 4 calls to get the invoices + // 10 calls to get the billing info + // 10 calls to collect the invoices + expect(apiRequestStub.callCount).to.eql(24) + }) + it('rejects with no invoices are processed because of errors', async function () { fakeApiRequests([404]) await expect(main()).to.be.rejectedWith('No invoices collected') @@ -214,7 +265,7 @@ describe('CollectPayPalPastDueInvoice', function () { it('resolves when some invoices are partially successful', async function () { fakeApiRequests([200, 404]) const r = await main() - await expect(r).to.eql({ + expect(r).to.eql({ INVOICES_COLLECTED: [200, 404], INVOICES_COLLECTED_SUCCESS: [200], USERS_COLLECTED: ['200', '404'],