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.

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

feat(payments): Do not allow duplicate subscriptions to the same offering and interval #18322

Merged
merged 1 commit into from
Feb 7, 2025

Conversation

xlisachan
Copy link
Contributor

@xlisachan xlisachan commented Feb 5, 2025

This pull request

  • fixes checkEligibility to return correct status
  • prevents customer from creating duplicate subscriptions to the same offering and interval

Issue that this pull request solves

Closes: FXA-10994

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.

Screenshots

Invalid eligibility status

Screenshot 2025-02-06 at 10 31 53 AM

Downgrade eligibility status

Screenshot 2025-02-07 at 3 41 00 PM

Example of Error page without Button

Screenshot 2025-02-06 at 10 32 07 AM

@xlisachan xlisachan force-pushed the FXA-10994 branch 4 times, most recently from 9676c5f to eb1d061 Compare February 6, 2025 16:30
@xlisachan xlisachan marked this pull request as ready for review February 6, 2025 17:03
@xlisachan xlisachan requested review from a team as code owners February 6, 2025 17:03
@@ -6,6 +6,7 @@ import { headers } from 'next/headers';
import Image from 'next/image';
import Link from 'next/link';

import { SUPPORT_URL } from '@fxa/payments/ui';
Copy link
Contributor

Choose a reason for hiding this comment

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

Could the support url be read from config instead please?

It should be defined in apps/payments/next/config/index.ts, similar to contentServerUrl and can then be used by importing the config object that's being exported.

@@ -35,7 +35,8 @@ export class EligibilityManager {
*/
async getOfferingOverlap(
priceIds: string[],
targetPriceId: string
targetPriceId: string,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. Seems like this change makes targetPriceId optional as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or rather I suppose either targetPriceId or targetPriceId should be provided. Could a check for this be added to this method?

Comment on lines 506 to 507
state: CartState.FAIL,
errorReasonId: CartErrorReasonId.CartEligibilityStatusInvalid,
Copy link
Contributor

Choose a reason for hiding this comment

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

If the eligibility is INVALID, I'd expect the cart to be set to error state.

A cart in error state would have cart.state: CartState.FAIL and errorReasonId: CartErrorReasonId.xxx

This current change only returns valid values for state and errorReasonId but doesn't update the underlying Cart DB record. Since our goal with SP3 is for the frontend to be a reflection of the Cart, its important for Cart in the DB to match what the frontend is showing.

@xlisachan xlisachan force-pushed the FXA-10994 branch 4 times, most recently from 7ae1284 to 0bdf13d Compare February 7, 2025 20:40
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.

r+. Thank you Lisa

A nit. but not for this PR, we should probably update the Title of the page to something more generic.Error confirming subscription… doesn't quite fit here.

@xlisachan xlisachan merged commit 818d7d7 into main Feb 7, 2025
23 checks passed
@xlisachan xlisachan deleted the FXA-10994 branch February 7, 2025 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants