Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(payments): Add Upgrade - Purchase Details + Layout #18173

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat(payments): Add Upgrade - Purchase Details + Layout #18173

wants to merge 2 commits into from

Conversation

xlisachan
Copy link
Contributor

@xlisachan xlisachan commented Dec 30, 2024 •
edited
Loading

This pull request

  • Adds the UpgradePurchaseDetails component and revises PurchaseDetails to reduce number of components
  • Updates eligibilityMananger.compareOverlap to also return the offering id when status is upgrade
  • Updates eligibilityService.checkEligibility to also return offering id and StripePrice of the plan being upgraded from
  • Adds invoiceManager.previewUpcomingForUpgrade
  • Updates cartService.getCart and adds oneTimeCharge as part of invoicePreview
  • Updates action service to filter/check properties for upgrade cart
  • Creates upgrade layout, i.e. app/[locale]/[offeringId]/upgrade/[interval]/[cartId]/layout.tsx
  • Updates SubscriptionTitle and adds it and UpgradePurchaseDetails to upgrade layout

Issue that this pull request solves

Closes: FXA-7588

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

@xlisachan xlisachan force-pushed the FXA-7588 branch 8 times, most recently from bd90b6e to f24741a Compare January 6, 2025 18:40
@xlisachan xlisachan force-pushed the FXA-7588 branch 22 times, most recently from 66d42cc to f487fae Compare January 10, 2025 17:43
@xlisachan xlisachan force-pushed the FXA-7588 branch 4 times, most recently from c141087 to b50376a Compare January 13, 2025 16:13
StaberindeZA
StaberindeZA reviewed Jan 13, 2025
View reviewed changes
Copy link
Contributor

@StaberindeZA StaberindeZA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good. Just some minor comments.

libs/payments/cart/src/lib/cart.service.ts Outdated
@@ -423,12 +432,42 @@ export class CartService {
customer = await this.customerManager.retrieve(cart.stripeCustomerId);
}

const eligibility = await this.eligibilityService.checkEligibility(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it necessary to checkEligibility again? Eligibility had already been calculated as part of setupCart and the eligibilityStatus was stored in the Cart DB table.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing it's because eligibleSourcePrice is needed and that's currently only being returned by checkEligibility?

If it's known that the eligibilityStatus is UPGRADE, is there no easier/faster way to get the price the customer is upgrading from?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we leave this as is for now, and see if we can iterate on this at a later time. Happy to hear your or anyone else's thoughts on this.

libs/payments/customer/src/lib/invoice.manager.ts Outdated
const invoices = [upcomingInvoice];

let proratedInvoice;
if (isUpgrade) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thus far with SP3 we've been steering clear of conditionals in methods, instead favoring explicit methods per task. (See CheckoutService.payWithStripe and CheckoutService.payWithPaypal vs CheckoutService.pay(isStripe: boolean)). I think having a separate method fetching invoicePreview for upgrades makes sense here.

libs/payments/customer/src/lib/util/stripeInvoiceToInvoicePreviewDTO.ts Outdated
): InvoicePreview {
const taxAmounts = invoice.total_tax_amounts.map((amount) => ({
const taxAmounts = invoice[0].total_tax_amounts.map((amount) => ({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a minor note, and can be picked up as a polish PR, but it'd be great to be more explicit about which invoice is the current and which is prorated, instead of just assuming it's index 0 and index 1 respectively.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted this update as we only need the total of the prorated invoice (I did not see the prorated amount being used anywhere other than in the legacy tests and stories).

libs/payments/ui/src/lib/client/components/PurchaseDetails/index.tsx Outdated
Comment on lines 97 to 104
<Localized
id="list-positive-amount"
vars={{
amount: getLocalizedCurrency(listAmount, currency),
}}
>
<p>{getLocalizedCurrencyString(listAmount, currency)}</p>
</Localized>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for making all these changes as discussed last week.

Just as an FYI, separately @bcolsson had reached out to me last week questioning the need for localization of list-positive-amount and list-negative-amount. I had a quick look, and doesn't seem like fxa-payments-server currently has localization on these fields, so we should be able to simplify this a bit. But happy to do that as a polish PR once this lands.

libs/payments/cart/src/lib/cart.service.ts Outdated
@@ -472,6 +511,11 @@ export class CartService {
metricsOptedOut,
latestInvoicePreview,
paymentInfo,
eligibilityStatus: cartEligibilityStatus,
eligibleSourcePrice: currentPrice,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit. Could this be renamed to something more appropriate? AFAIK this price isn't used in anything related to eligibility, but instead is needed for the upgrade pages? If so, maybe something like upgradeFromPrice makes more sense?

@xlisachan xlisachan changed the title WIP - feat(payments): Add Upgrade - Purchase Details + Layout feat(payments): Add Upgrade - Purchase Details + Layout Jan 14, 2025
@xlisachan xlisachan force-pushed the FXA-7588 branch 2 times, most recently from 4fe8427 to 80e5b3e Compare January 14, 2025 22:12
@xlisachan xlisachan marked this pull request as ready for review January 14, 2025 22:37
@xlisachan xlisachan requested review from a team as code owners January 14, 2025 22:37
@xlisachan xlisachan force-pushed the FXA-7588 branch 3 times, most recently from 8cb99af to 6023739 Compare January 15, 2025 14:53
@xlisachan xlisachan force-pushed the FXA-7588 branch 4 times, most recently from 9ff5203 to 56dc183 Compare January 16, 2025 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StaberindeZA StaberindeZA StaberindeZA left review comments

@bcolsson bcolsson bcolsson approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@xlisachan @StaberindeZA @bcolsson