From 76606e5ec9a5844b4e8d7ec1aff34ae284dac0fc Mon Sep 17 00:00:00 2001 From: Davey Alvarez Date: Tue, 11 Aug 2026 19:31:30 -0400 Subject: [PATCH] fix(payments): expand discount coupons on cached invoices and subscriptions Because: * The SDK 22 upgrade rewrote discount reads to `discounts[0].source.coupon` without requesting the expansion, so a cached invoice holding a discount id threw `Cannot read properties of undefined (reading 'coupon')` on `/v1/account`. * Expanding `discounts` does not expand the coupon under it, so reads asking only for `discounts` dropped promotion details. This commit: * Requests `discounts.source.coupon` on the reads and Firestore writes that asked only for `discounts`. * Adds discountsNeedExpansion, and re-fetches a discount that arrives as an id rather than dropping it. * Reports the discount amount with null coupon fields when a coupon is still unreadable. * Types `source.coupon` as expanded or null, never an id. Closes PAY-3893 --- .../customer/src/lib/subscription.manager.ts | 2 +- .../stripeInvoiceToFirstInvoicePreviewDTO.ts | 4 +- .../src/lib/subscriptionManagement.service.ts | 2 +- .../payments-metrics-aggregator.service.ts | 2 +- .../stripe/src/lib/stripe.client.spec.ts | 23 ++- libs/payments/stripe/src/lib/stripe.client.ts | 4 +- .../stripe/src/lib/stripe.client.types.ts | 5 +- .../lib/payments/stripe-firestore.spec.ts | 16 +- .../lib/payments/stripe-formatter.spec.ts | 67 +++++++ .../lib/payments/stripe-formatter.ts | 13 +- .../lib/payments/stripe.spec.ts | 181 +++++++++++++++++- .../fxa-auth-server/lib/payments/stripe.ts | 60 ++++-- .../fxa-shared/payments/stripe-firestore.ts | 4 +- packages/fxa-shared/payments/stripe.ts | 23 ++- 14 files changed, 358 insertions(+), 48 deletions(-) diff --git a/libs/payments/customer/src/lib/subscription.manager.ts b/libs/payments/customer/src/lib/subscription.manager.ts index cd704f9e847..737070d6a52 100644 --- a/libs/payments/customer/src/lib/subscription.manager.ts +++ b/libs/payments/customer/src/lib/subscription.manager.ts @@ -191,7 +191,7 @@ export class SubscriptionManager { const subscription = await this.retrieve(subscriptionId); return subscription.discounts.some( - (discount) => discount.source?.coupon.id === stripeCouponId + (discount) => discount.source?.coupon?.id === stripeCouponId ); } } diff --git a/libs/payments/customer/src/lib/util/stripeInvoiceToFirstInvoicePreviewDTO.ts b/libs/payments/customer/src/lib/util/stripeInvoiceToFirstInvoicePreviewDTO.ts index 6d4730f6838..0d246cf1dc1 100644 --- a/libs/payments/customer/src/lib/util/stripeInvoiceToFirstInvoicePreviewDTO.ts +++ b/libs/payments/customer/src/lib/util/stripeInvoiceToFirstInvoicePreviewDTO.ts @@ -71,7 +71,7 @@ export function stripeInvoiceToInvoicePreviewDTO( discountAmount, subtotal: invoice.subtotal, discountEnd: invoice.discounts[0]?.end, - discountType: invoice.discounts[0]?.source?.coupon.duration, + discountType: invoice.discounts[0]?.source?.coupon?.duration, number: invoice.number, paypalTransactionId: invoice.metadata?.[STRIPE_INVOICE_METADATA.PaypalTransactionId], @@ -82,7 +82,7 @@ export function stripeInvoiceToInvoicePreviewDTO( creditApplied: invoice.ending_balance ? invoice.starting_balance - invoice.ending_balance : invoice.starting_balance, - promotionName: invoice.discounts[0]?.source?.coupon.name, + promotionName: invoice.discounts[0]?.source?.coupon?.name, remainingAmountTotal, startingBalance: invoice.starting_balance, totalExcludingTax: invoice.total_excluding_tax, diff --git a/libs/payments/management/src/lib/subscriptionManagement.service.ts b/libs/payments/management/src/lib/subscriptionManagement.service.ts index 00000dc5054..8a70347c483 100644 --- a/libs/payments/management/src/lib/subscriptionManagement.service.ts +++ b/libs/payments/management/src/lib/subscriptionManagement.service.ts @@ -561,7 +561,7 @@ export class SubscriptionManagementService { } = upcomingInvoice; const nextPromotionName = - subscription.discounts[0]?.source?.coupon.name ?? null; + subscription.discounts[0]?.source?.coupon?.name ?? null; const totalExclusiveTax = taxAmounts .filter((tax) => !tax.inclusive) diff --git a/libs/payments/metrics-aggregator/src/lib/payments-metrics-aggregator.service.ts b/libs/payments/metrics-aggregator/src/lib/payments-metrics-aggregator.service.ts index b5f082996bf..405eecfb969 100644 --- a/libs/payments/metrics-aggregator/src/lib/payments-metrics-aggregator.service.ts +++ b/libs/payments/metrics-aggregator/src/lib/payments-metrics-aggregator.service.ts @@ -60,7 +60,7 @@ export class PaymentsMetricsAggregatorService { return { customerId: customer?.id, - couponCode: subscription?.discounts[0]?.source?.coupon.id, + couponCode: subscription?.discounts[0]?.source?.coupon?.id, currency: customer?.currency || undefined, taxAddress: taxAddress, productId: price?.product, diff --git a/libs/payments/stripe/src/lib/stripe.client.spec.ts b/libs/payments/stripe/src/lib/stripe.client.spec.ts index 0192b39acc4..62722919c77 100644 --- a/libs/payments/stripe/src/lib/stripe.client.spec.ts +++ b/libs/payments/stripe/src/lib/stripe.client.spec.ts @@ -317,7 +317,7 @@ describe('StripeClient', () => { expect(result).toEqual(mockResponse); }); - it('expands confirmation_secret, discounts, and nested tax rates', async () => { + it('expands confirmation_secret, discount coupons, and nested tax rates', async () => { const mockInvoice = StripeInvoiceFactory(); const mockResponse = StripeResponseFactory(mockInvoice); @@ -328,7 +328,7 @@ describe('StripeClient', () => { expect(mockStripeInvoicesRetrieve).toHaveBeenCalledWith(mockInvoice.id, { expand: [ 'confirmation_secret', - 'discounts', + 'discounts.source.coupon', 'lines.data.taxes.tax_rate_details.tax_rate', 'total_taxes.tax_rate_details.tax_rate', ], @@ -350,6 +350,25 @@ describe('StripeClient', () => { expect(result).toEqual(mockResponse); }); + + it('expands discount coupons and nested tax rates', async () => { + const mockCustomer = StripeCustomerFactory(); + const mockInvoice = StripeUpcomingInvoiceFactory(); + const mockResponse = StripeResponseFactory(mockInvoice); + + mockStripeCreatePreviewInvoice.mockResolvedValue(mockResponse); + + await stripeClient.invoicesCreatePreview({ customer: mockCustomer.id }); + + expect(mockStripeCreatePreviewInvoice).toHaveBeenCalledWith({ + customer: mockCustomer.id, + expand: [ + 'discounts.source.coupon', + 'lines.data.taxes.tax_rate_details.tax_rate', + 'total_taxes.tax_rate_details.tax_rate', + ], + }); + }); }); describe('invoicesFinalizeInvoice', () => { diff --git a/libs/payments/stripe/src/lib/stripe.client.ts b/libs/payments/stripe/src/lib/stripe.client.ts index 4f1c184dea0..2926a508037 100644 --- a/libs/payments/stripe/src/lib/stripe.client.ts +++ b/libs/payments/stripe/src/lib/stripe.client.ts @@ -238,7 +238,7 @@ export class StripeClient { ...params, expand: [ 'confirmation_secret', - 'discounts', + 'discounts.source.coupon', 'lines.data.taxes.tax_rate_details.tax_rate', 'total_taxes.tax_rate_details.tax_rate', ], @@ -251,7 +251,7 @@ export class StripeClient { const result = await this.stripe.invoices.createPreview({ ...params, expand: [ - 'discounts', + 'discounts.source.coupon', 'lines.data.taxes.tax_rate_details.tax_rate', 'total_taxes.tax_rate_details.tax_rate', ], diff --git a/libs/payments/stripe/src/lib/stripe.client.types.ts b/libs/payments/stripe/src/lib/stripe.client.types.ts index 8f2bdacb11d..e98320c4c6c 100644 --- a/libs/payments/stripe/src/lib/stripe.client.types.ts +++ b/libs/payments/stripe/src/lib/stripe.client.types.ts @@ -107,7 +107,10 @@ export type StripeDiscount = NegotiateExpanded< DeepOverride< Stripe.Discount, { - source: DeepOverride; + source: DeepOverride< + Stripe.Discount.Source, + { coupon: StripeCoupon | null } + >; } >, 'customer' | 'promotion_code' diff --git a/packages/fxa-auth-server/lib/payments/stripe-firestore.spec.ts b/packages/fxa-auth-server/lib/payments/stripe-firestore.spec.ts index 6e268d23ad9..b6e423d928d 100644 --- a/packages/fxa-auth-server/lib/payments/stripe-firestore.spec.ts +++ b/packages/fxa-auth-server/lib/payments/stripe-firestore.spec.ts @@ -672,7 +672,9 @@ describe('StripeFirestore', () => { await stripeFirestore.fetchAndInsertInvoice(invoiceId, eventTime); expect(stripe.invoices.retrieve).toHaveBeenCalledTimes(1); - expect(stripe.invoices.retrieve).toHaveBeenCalledWith(invoiceId); + expect(stripe.invoices.retrieve).toHaveBeenCalledWith(invoiceId, { + expand: ['discounts.source.coupon'], + }); expect( stripeFirestore.customerCollectionDbRef.where ).toHaveBeenCalledTimes(1); @@ -718,7 +720,9 @@ describe('StripeFirestore', () => { expect(result).toEqual(mockInvoice); expect(stripe.invoices.retrieve).toHaveBeenCalledTimes(1); - expect(stripe.invoices.retrieve).toHaveBeenCalledWith(invoiceId); + expect(stripe.invoices.retrieve).toHaveBeenCalledWith(invoiceId, { + expand: ['discounts.source.coupon'], + }); expect( stripeFirestore.customerCollectionDbRef.where ).toHaveBeenCalledTimes(1); @@ -742,7 +746,9 @@ describe('StripeFirestore', () => { expect(result).toEqual(mockInvoiceWithoutSubscription); expect(stripe.invoices.retrieve).toHaveBeenCalledTimes(1); - expect(stripe.invoices.retrieve).toHaveBeenCalledWith(invoiceId); + expect(stripe.invoices.retrieve).toHaveBeenCalledWith(invoiceId, { + expand: ['discounts.source.coupon'], + }); expect( stripeFirestore.customerCollectionDbRef.where ).toHaveBeenCalledTimes(0); @@ -802,7 +808,9 @@ describe('StripeFirestore', () => { expect(result).toEqual(mockInvoice); expect(stripe.invoices.retrieve).toHaveBeenCalledTimes(1); - expect(stripe.invoices.retrieve).toHaveBeenCalledWith(invoiceId); + expect(stripe.invoices.retrieve).toHaveBeenCalledWith(invoiceId, { + expand: ['discounts.source.coupon'], + }); expect( stripeFirestore.customerCollectionDbRef.where ).toHaveBeenCalledTimes(1); diff --git a/packages/fxa-auth-server/lib/payments/stripe-formatter.spec.ts b/packages/fxa-auth-server/lib/payments/stripe-formatter.spec.ts index b1b2cce47f0..5e7f17ce0cf 100644 --- a/packages/fxa-auth-server/lib/payments/stripe-formatter.spec.ts +++ b/packages/fxa-auth-server/lib/payments/stripe-formatter.spec.ts @@ -71,6 +71,60 @@ describe('stripeInvoiceToFirstInvoicePreviewDTO', () => { ); }); + it('keeps the discount amount when the invoice discount is an unexpanded id', () => { + const invoicePreview = deepCopy(previewInvoiceWithDiscountAndTax); + invoicePreview.discounts = ['di_1234567890abcdef']; + + const invoice = stripeInvoiceToFirstInvoicePreviewDTO([ + invoicePreview, + undefined, + ]); + + expect(invoice.discount).toEqual({ + amount: previewInvoiceWithDiscountAndTax.total_discount_amounts[0].amount, + amount_off: null, + percent_off: null, + }); + }); + + it('keeps the discount amount when the discount has no source', () => { + const invoicePreview = deepCopy(previewInvoiceWithDiscountAndTax); + invoicePreview.discounts = [ + { + id: 'di_1234567890abcdef', + object: 'discount', + coupon: { id: 'co_1234567890abcdef', percent_off: 20 }, + }, + ]; + + const invoice = stripeInvoiceToFirstInvoicePreviewDTO([ + invoicePreview, + undefined, + ]); + + expect(invoice.discount).toEqual({ + amount: previewInvoiceWithDiscountAndTax.total_discount_amounts[0].amount, + amount_off: null, + percent_off: null, + }); + }); + + it('keeps the discount amount when the coupon on the discount source is an unexpanded id', () => { + const invoicePreview = deepCopy(previewInvoiceWithDiscountAndTax); + invoicePreview.discounts[0].source.coupon = 'co_1234567890abcdef'; + + const invoice = stripeInvoiceToFirstInvoicePreviewDTO([ + invoicePreview, + undefined, + ]); + + expect(invoice.discount).toEqual({ + amount: previewInvoiceWithDiscountAndTax.total_discount_amounts[0].amount, + amount_off: null, + percent_off: null, + }); + }); + it('formats an invoice where tax display_name is an empty string', () => { const invoicePreview = deepCopy(previewInvoiceWithTax); invoicePreview.total_taxes[0].tax_rate_details.tax_rate.display_name = ''; @@ -161,4 +215,17 @@ describe('stripeInvoiceToLatestInvoiceItemsDTO', () => { previewInvoiceWithDiscountAndTax.discounts[0].source.coupon.percent_off ); }); + + it('keeps the discount amount when the invoice discount is an unexpanded id', () => { + const invoicePreview = deepCopy(previewInvoiceWithDiscountAndTax); + invoicePreview.discounts = ['di_1234567890abcdef']; + + const invoice = stripeInvoiceToLatestInvoiceItemsDTO(invoicePreview); + + expect(invoice.discount).toEqual({ + amount: previewInvoiceWithDiscountAndTax.total_discount_amounts[0].amount, + amount_off: null, + percent_off: null, + }); + }); }); diff --git a/packages/fxa-auth-server/lib/payments/stripe-formatter.ts b/packages/fxa-auth-server/lib/payments/stripe-formatter.ts index 0072d3d463f..6f7710e32a2 100644 --- a/packages/fxa-auth-server/lib/payments/stripe-formatter.ts +++ b/packages/fxa-auth-server/lib/payments/stripe-formatter.ts @@ -44,13 +44,16 @@ export function stripeInvoiceToFirstInvoicePreviewDTO( } // Add discount if it exists - const discount = invoice[0].discounts?.[0]; - const coupon = discount?.source.coupon; - if (coupon && invoice[0].total_discount_amounts) { + const firstDiscount = invoice[0].discounts?.[0]; + const discount = + typeof firstDiscount === 'object' ? firstDiscount : undefined; + const rawCoupon = discount?.source?.coupon; + const coupon = typeof rawCoupon === 'object' ? rawCoupon : undefined; + if (invoice[0].total_discount_amounts?.length) { invoicePreview.discount = { amount: invoice[0].total_discount_amounts[0].amount, - amount_off: coupon.amount_off, - percent_off: coupon.percent_off, + amount_off: coupon?.amount_off ?? null, + percent_off: coupon?.percent_off ?? null, }; } diff --git a/packages/fxa-auth-server/lib/payments/stripe.spec.ts b/packages/fxa-auth-server/lib/payments/stripe.spec.ts index 062d677bbc5..f0ab54d87bf 100644 --- a/packages/fxa-auth-server/lib/payments/stripe.spec.ts +++ b/packages/fxa-auth-server/lib/payments/stripe.spec.ts @@ -891,7 +891,7 @@ describe('StripeHelper', () => { }); describe('getInvoiceWithDiscount', () => { - it('returns an invoice with discounts expanded', async () => { + it('returns an invoice with discounts and their coupons expanded', async () => { const invoice = { id: 'invoiceId' }; jest .spyOn(stripeHelper.stripe.invoices, 'retrieve') @@ -901,7 +901,7 @@ describe('StripeHelper', () => { expect(stripeHelper.stripe.invoices.retrieve).toHaveBeenCalledTimes(1); expect(stripeHelper.stripe.invoices.retrieve).toHaveBeenCalledWith( invoice.id, - { expand: ['discounts'] } + { expand: ['discounts.source.coupon'] } ); }); }); @@ -1087,7 +1087,10 @@ describe('StripeHelper', () => { expect(stripeStub).toHaveBeenCalledTimes(1); expect(stripeStub).toHaveBeenCalledWith({ subscription: 'sub123', - expand: ['discounts', 'lines.data.taxes.tax_rate_details.tax_rate'], + expand: [ + 'discounts.source.coupon', + 'lines.data.taxes.tax_rate_details.tax_rate', + ], }); }); @@ -1102,7 +1105,10 @@ describe('StripeHelper', () => { expect(stripeStub).toHaveBeenCalledTimes(1); expect(stripeStub).toHaveBeenCalledWith({ subscription: 'sub123', - expand: ['discounts', 'lines.data.taxes.tax_rate_details.tax_rate'], + expand: [ + 'discounts.source.coupon', + 'lines.data.taxes.tax_rate_details.tax_rate', + ], subscription_details: { cancel_at_period_end: false }, }); }); @@ -2999,6 +3005,80 @@ describe('StripeHelper', () => { ); }); + it('re-fetches the invoice from Stripe when the cached discount is an unexpanded id', async () => { + const cachedInvoice = deepCopy(invoicePaidSubscriptionCreate); + cachedInvoice.discounts = ['di_1234567890abcdef']; + const expandedInvoice = deepCopy(invoicePaidSubscriptionCreate); + expandedInvoice.discounts = [ + { + id: 'di_1234567890abcdef', + source: { coupon: { id: 'co_1234567890abcdef', percent_off: 20 } }, + }, + ]; + stripeFirestore.retrieveInvoice = jest + .fn() + .mockResolvedValue(cachedInvoice); + stripeHelper.stripe.invoices.retrieve = jest + .fn() + .mockResolvedValue(expandedInvoice); + + const result = await stripeHelper.expandResource( + cachedInvoice.id, + INVOICES_RESOURCE + ); + + expect(result).toEqual(expandedInvoice); + expect(stripeHelper.stripe.invoices.retrieve).toHaveBeenCalledWith( + cachedInvoice.id, + { expand: ['discounts.source.coupon'] } + ); + }); + + it('returns the cached invoice when the discount has no source', async () => { + const cachedInvoice = deepCopy(invoicePaidSubscriptionCreate); + cachedInvoice.discounts = [ + { + id: 'di_1234567890abcdef', + object: 'discount', + coupon: { id: 'co_1234567890abcdef', percent_off: 20 }, + }, + ]; + stripeFirestore.retrieveInvoice = jest + .fn() + .mockResolvedValue(cachedInvoice); + stripeHelper.stripe.invoices.retrieve = jest.fn(); + + const result = await stripeHelper.expandResource( + cachedInvoice.id, + INVOICES_RESOURCE + ); + + expect(result).toEqual(cachedInvoice); + expect(stripeHelper.stripe.invoices.retrieve).not.toHaveBeenCalled(); + }); + + it('does not re-fetch the invoice when the cached discount is already expanded', async () => { + const cachedInvoice = deepCopy(invoicePaidSubscriptionCreate); + cachedInvoice.discounts = [ + { + id: 'di_1234567890abcdef', + source: { coupon: { id: 'co_1234567890abcdef', percent_off: 20 } }, + }, + ]; + stripeFirestore.retrieveInvoice = jest + .fn() + .mockResolvedValue(cachedInvoice); + stripeHelper.stripe.invoices.retrieve = jest.fn(); + + const result = await stripeHelper.expandResource( + cachedInvoice.id, + INVOICES_RESOURCE + ); + + expect(result).toEqual(cachedInvoice); + expect(stripeHelper.stripe.invoices.retrieve).not.toHaveBeenCalled(); + }); + it('expands invoice when invoice isnt found and inserts it', async () => { stripeFirestore.retrieveInvoice = jest .fn() @@ -3663,6 +3743,73 @@ describe('StripeHelper', () => { expect(actual).toEqual(expected); }); + it('re-fetches the subscription for its promotion values when the cached discount is an unexpanded id', async () => { + const subscription = deepCopy(subscriptionCouponForever); + subscription.discounts = ['di_1234567890abcdef']; + const input = { data: [subscription] }; + jest + .spyOn(stripeHelper.stripe.invoices, 'retrieve') + .mockResolvedValue(paidInvoice); + const callback = jest.spyOn(stripeHelper, 'expandResource'); + callback.mockResolvedValueOnce(paidInvoice); + callback.mockResolvedValueOnce({ id: productId, name: productName }); + jest + .spyOn(stripeHelper, 'getSubscriptionWithDiscount') + .mockResolvedValue(deepCopy(subscriptionCouponForever)); + + const actual = await stripeHelper.subscriptionsToResponse(input); + + expect(stripeHelper.getSubscriptionWithDiscount).toHaveBeenCalledWith( + subscription.id + ); + expect(actual[0].promotion_name).toBe( + subscriptionCouponForever.discounts[0].source.coupon.name + ); + expect(actual[0].promotion_duration).toBe('forever'); + expect(actual[0].promotion_percent_off).toBe( + subscriptionCouponForever.discounts[0].source.coupon.percent_off + ); + }); + + it('omits the promotion values when the cached discount has no source', async () => { + const subscription = deepCopy(subscriptionCouponForever); + subscription.discounts = [ + { + id: 'di_1234567890abcdef', + object: 'discount', + coupon: { id: 'co_1234567890abcdef', percent_off: 20 }, + }, + ]; + const input = { data: [subscription] }; + jest + .spyOn(stripeHelper.stripe.invoices, 'retrieve') + .mockResolvedValue(paidInvoice); + const callback = jest.spyOn(stripeHelper, 'expandResource'); + callback.mockResolvedValueOnce(paidInvoice); + callback.mockResolvedValueOnce({ id: productId, name: productName }); + + const actual = await stripeHelper.subscriptionsToResponse(input); + + expect(actual[0].promotion_name).toBeNull(); + expect(actual[0].promotion_percent_off).toBeNull(); + }); + + it('does not re-fetch the subscription when the cached discount is already expanded', async () => { + const subscription = deepCopy(subscriptionCouponForever); + const input = { data: [subscription] }; + jest + .spyOn(stripeHelper.stripe.invoices, 'retrieve') + .mockResolvedValue(paidInvoice); + const callback = jest.spyOn(stripeHelper, 'expandResource'); + callback.mockResolvedValueOnce(paidInvoice); + callback.mockResolvedValueOnce({ id: productId, name: productName }); + jest.spyOn(stripeHelper, 'getSubscriptionWithDiscount'); + + await stripeHelper.subscriptionsToResponse(input); + + expect(stripeHelper.getSubscriptionWithDiscount).not.toHaveBeenCalled(); + }); + it('repeating coupon includes the promotion values in the returned value', async () => { const subscription = deepCopy(subscriptionCouponRepeating); const input = { data: [subscription] }; @@ -5059,6 +5206,32 @@ describe('StripeHelper', () => { expect(actual.discountDuration).toBeNull(); }); + it('extracts the correct discount type when only the coupon needs to be expanded', async () => { + const fixtureUnexpandedCoupon = deepCopy(fixture); + fixtureUnexpandedCoupon.discounts = [ + { source: { coupon: 'couponId' } }, + ]; + jest.spyOn(stripeHelper, 'getInvoiceWithDiscount').mockResolvedValue({ + ...fixtureUnexpandedCoupon, + discounts: [ + { + source: { + coupon: { + duration: 'repeating', + duration_in_months: 3, + }, + }, + }, + ], + }); + const actual = await stripeHelper.extractInvoiceDetailsForEmail( + fixtureUnexpandedCoupon + ); + expect(stripeHelper.getInvoiceWithDiscount).toHaveBeenCalledTimes(1); + expect(actual.discountType).toBe('repeating'); + expect(actual.discountDuration).toBe(3); + }); + it('uses and includes Firestore based configs when available', async () => { jest .spyOn(stripeHelper, 'maybeGetPlanConfig') diff --git a/packages/fxa-auth-server/lib/payments/stripe.ts b/packages/fxa-auth-server/lib/payments/stripe.ts index ac1c1f1c104..2d4b8876cf1 100644 --- a/packages/fxa-auth-server/lib/payments/stripe.ts +++ b/packages/fxa-auth-server/lib/payments/stripe.ts @@ -26,6 +26,7 @@ import { import { CHARGES_RESOURCE, CUSTOMER_RESOURCE, + discountsNeedExpansion, INVOICES_RESOURCE, PAYMENT_METHOD_RESOURCE, PLAN_RESOURCE, @@ -403,7 +404,10 @@ export class StripeHelper extends StripeHelperBase { }) { return this.stripe.invoices.createPreview({ subscription: subscriptionId, - expand: ['discounts', 'lines.data.taxes.tax_rate_details.tax_rate'], + expand: [ + 'discounts.source.coupon', + 'lines.data.taxes.tax_rate_details.tax_rate', + ], ...(includeCanceled && { subscription_details: { cancel_at_period_end: false }, }), @@ -671,19 +675,23 @@ export class StripeHelper extends StripeHelperBase { } /* - * Expand the discounts property of an invoice - * TODO: We may be able to remove this method in the future if we want to add logic - * to expandResource to check if the discounts property is expanded. + * Expand the discounts property of an invoice, and the coupon nested under it */ async getInvoiceWithDiscount( invoiceId: string ): Promise> { const invoice = await this.stripe.invoices.retrieve(invoiceId, { - expand: ['discounts'], + expand: ['discounts.source.coupon'], }); return invoice as StripeResponse; } + async getSubscriptionWithDiscount(subscriptionId: string) { + return this.stripe.subscriptions.retrieve(subscriptionId, { + expand: ['discounts.source.coupon'], + }); + } + /** * Finalizes an invoice and marks auto_advance as false. */ @@ -1752,12 +1760,22 @@ export class StripeHelper extends StripeHelperBase { continue; } - let latestInvoice = sub.latest_invoice; - if (typeof latestInvoice === 'string') { - latestInvoice = await this.expandResource( - latestInvoice, + let latestInvoice: StripeInvoice | null; + if (typeof sub.latest_invoice === 'string') { + latestInvoice = await this.expandResource( + sub.latest_invoice, + INVOICES_RESOURCE + ); + } else if ( + sub.latest_invoice && + discountsNeedExpansion(sub.latest_invoice.discounts) + ) { + latestInvoice = await this.expandResource( + sub.latest_invoice.id, INVOICES_RESOURCE ); + } else { + latestInvoice = sub.latest_invoice as StripeInvoice | null; } if (!latestInvoice) { @@ -1772,7 +1790,8 @@ export class StripeHelper extends StripeHelperBase { // to get details of why it failed. The caller should expand the last_invoice // calls by passing ['data.subscriptions.data.latest_invoice'] to `fetchCustomer` // as the `expand` argument or this will not fetch the failure code/message. - let charge = latestInvoice.payments?.data[0]?.payment.charge; + let charge: string | Stripe.Charge | null | undefined = + latestInvoice.payments?.data[0]?.payment.charge; if (this.checkSubscriptionPastDue(sub) && latestInvoice && charge) { if (typeof charge === 'string') { charge = await this.stripe.charges.retrieve(charge); @@ -1784,10 +1803,13 @@ export class StripeHelper extends StripeHelperBase { } } - const firstDiscount = sub.discounts?.[0]; + const discounts = discountsNeedExpansion(sub.discounts) + ? (await this.getSubscriptionWithDiscount(sub.id)).discounts + : sub.discounts; + const firstDiscount = discounts?.[0]; const discount = typeof firstDiscount === 'object' ? firstDiscount : undefined; - const rawCoupon = discount?.source.coupon; + const rawCoupon = discount?.source?.coupon; const coupon = typeof rawCoupon === 'object' ? rawCoupon : undefined; // This type inconsistency runs quite deep, but plan does exist on the subscription here @@ -1813,9 +1835,8 @@ export class StripeHelper extends StripeHelperBase { cancel_at_period_end: sub.cancel_at_period_end, end_at: sub.ended_at, latest_invoice: latestInvoice.number, - latest_invoice_items: stripeInvoiceToLatestInvoiceItemsDTO( - latestInvoice as StripeInvoice - ), + latest_invoice_items: + stripeInvoiceToLatestInvoiceItemsDTO(latestInvoice), plan_id: plan.id, product_name, product_id, @@ -1988,16 +2009,17 @@ export class StripeHelper extends StripeHelperBase { let discountDuration: number | null = null; if (invoice.id && !!invoice.discounts?.length && invoice.discounts.length === 1) { - // The discount may arrive as a string id, in which case expand it. + // The discount, or its coupon, may arrive as an id, in which case expand it. let discount = invoice.discounts[0] as string | StripeDiscount; - if (typeof discount === 'string') { + if (discountsNeedExpansion(invoice.discounts)) { const invoiceWithDiscount = await this.getInvoiceWithDiscount( invoice.id ); discount = invoiceWithDiscount.discounts[0]; } - const coupon = - typeof discount === 'object' ? discount.source.coupon : undefined; + const rawCoupon = + typeof discount === 'object' ? discount.source?.coupon : undefined; + const coupon = typeof rawCoupon === 'object' ? rawCoupon : undefined; discountType = coupon?.duration ?? null; discountDuration = coupon?.duration_in_months ?? null; } diff --git a/packages/fxa-shared/payments/stripe-firestore.ts b/packages/fxa-shared/payments/stripe-firestore.ts index 25e3afb63d0..9eb82aa5898 100644 --- a/packages/fxa-shared/payments/stripe-firestore.ts +++ b/packages/fxa-shared/payments/stripe-firestore.ts @@ -514,7 +514,9 @@ export class StripeFirestore { eventTime: number, ignoreErrors: boolean = false, ) { - const invoice = await this.stripe.invoices.retrieve(invoiceId); + const invoice = await this.stripe.invoices.retrieve(invoiceId, { + expand: ['discounts.source.coupon'], + }); const subscriptionId = invoice.parent?.subscription_details?.subscription; if (subscriptionId == null) { // We can only insert invoices with a subscription for caching, but we diff --git a/packages/fxa-shared/payments/stripe.ts b/packages/fxa-shared/payments/stripe.ts index 59e51d388bb..c1feb149f28 100644 --- a/packages/fxa-shared/payments/stripe.ts +++ b/packages/fxa-shared/payments/stripe.ts @@ -31,6 +31,16 @@ import { ProductConfigurationManager } from '@fxa/shared/cms'; import { StripeMapperService } from '@fxa/payments/legacy'; import * as Sentry from '@sentry/node'; +export function discountsNeedExpansion( + discounts?: Array | null +) { + return !!discounts?.some( + (discount) => + typeof discount === 'string' || + typeof discount.source?.coupon === 'string' + ); +} + export const CHARGES_RESOURCE = 'charges'; export const COUPON_RESOURCE = 'coupons'; export const CREDIT_NOTE_RESOURCE = 'creditNotes'; @@ -609,16 +619,19 @@ export abstract class StripeHelper { ); case INVOICES_RESOURCE: try { - // TODO we could remove the getInvoiceWithDiscount method if we add logic - // here to check if the discounts field is expanded but it would mean - // adding another stipe call to get discounts even when unnecessary const invoice = await this.stripeFirestore.retrieveInvoice(resource); + if (!discountsNeedExpansion(invoice.discounts)) { + // @ts-ignore + return invoice; + } // @ts-ignore - return invoice; + return this.stripe.invoices.retrieve(resource, { + expand: ['discounts.source.coupon'], + }); } catch (err) { if (err.name === FirestoreStripeError.FIRESTORE_INVOICE_NOT_FOUND) { const invoice = await this.stripe.invoices.retrieve(resource, { - expand: ['discounts'], + expand: ['discounts.source.coupon'], }); await this.stripeFirestore.retrieveAndFetchCustomer( invoice.customer as string,