-
Notifications
You must be signed in to change notification settings - Fork 402
feat(checkout): Migrate CBA MPGS to Resolver Configuration #3075
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,9 @@ import { | |
| import { createSagePayPaymentStrategy } from '@bigcommerce/checkout-sdk/integrations/sagepay'; | ||
| import React, { type FunctionComponent, useCallback } from 'react'; | ||
|
|
||
| import { useCheckout } from '@bigcommerce/checkout/contexts'; | ||
|
|
||
| import { isExperimentEnabled } from '../../common/utility'; | ||
| import { | ||
| withHostedCreditCardFieldset, | ||
| type WithInjectedHostedCreditCardFieldsetProps, | ||
|
|
@@ -35,6 +38,13 @@ const HostedCreditCardPaymentMethod: FunctionComponent< | |
| initializePayment, | ||
| ...rest | ||
| }) => { | ||
| const { selectedState: config } = useCheckout(({ data }) => data.getConfig()); | ||
| const isCBAMPGSResolverEnabled = isExperimentEnabled( | ||
| config?.checkoutSettings, | ||
| 'PI-4748.cba_resolver_configuration', | ||
| false, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The purpose of this fallback value is for the time when
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please explain when the experiment will not appear in the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Before the experiment is surfaced/registered in LaunchDarkly (or whatever feature flag service is used), the key won't exist in features[] at all — features['PI-4748.cba_resolver_configuration'] will be undefined. In that case ?? fallbackValue kicks in. Since the default fallback in isExperimentEnabled is true, without explicitly passing false here, cba_mpgs would resolve through the new flow on day one — before we've intentionally enabled it for anyone. That's exactly what we want to avoid.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, this is intended use case when we introduced this fallback value. The experiment appears to be added on Jun 23, 2026 at https://app.launchdarkly.com/projects/default/flags/PI-4748.cba_resolver_configuration/targeting?env=production&selected-env=production. Is anything blocking us from showing it to Checkout FE now?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I turned on this flag if you about that. Or I miss understood |
||
| ); | ||
|
|
||
| const initializeHostedCreditCardPayment: CreditCardPaymentMethodProps['initializePayment'] = | ||
| useCallback( | ||
| async (options, selectedInstrument) => { | ||
|
|
@@ -46,14 +56,14 @@ const HostedCreditCardPaymentMethod: FunctionComponent< | |
| createCyberSourcePaymentStrategy, | ||
| createCyberSourceV2PaymentStrategy, | ||
| createSagePayPaymentStrategy, | ||
| createCBAMPGSPaymentStrategy, | ||
| ...(!isCBAMPGSResolverEnabled ? [createCBAMPGSPaymentStrategy] : []), | ||
| ], | ||
| creditCard: getHostedFormOptions && { | ||
| form: await getHostedFormOptions(selectedInstrument), | ||
| }, | ||
| }); | ||
| }, | ||
| [getHostedFormOptions, initializePayment], | ||
| [getHostedFormOptions, initializePayment, isCBAMPGSResolverEnabled], | ||
| ); | ||
|
|
||
| return ( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,41 @@ | ||
| import type { CheckoutSettings } from '@bigcommerce/checkout-sdk'; | ||
| import { type ComponentType } from 'react'; | ||
|
|
||
| import { | ||
| type PaymentMethodProps, | ||
| type PaymentMethodResolveId, | ||
| } from '@bigcommerce/checkout/payment-integration-api'; | ||
|
|
||
| import { isExperimentEnabled } from '../common/utility'; | ||
| import { resolveLazyComponent } from '../common/resolver'; | ||
| import * as lazyPaymentMethods from '../generated/paymentIntegrations'; | ||
|
|
||
| export default function resolvePaymentMethod( | ||
| query: PaymentMethodResolveId, | ||
| checkoutSettings?: CheckoutSettings, | ||
| ): ComponentType<PaymentMethodProps> | undefined { | ||
| const { ComponentRegistry, ...allExports } = lazyPaymentMethods; | ||
| const filteredMethods = Object.fromEntries( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we adding this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @animesh1987 we want to experiments for our PaymentMethods
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for behavior of this PR we set experiment name in config then in file resolvePaymentMethod we check is this experiment true or false if it is true pr if we don't have experiment then this method will be in result arr if not than we will not have this mthod in arr
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Attaching an experiment name to a payment method config requires some explanations on the purpose of this experiment. Could we use the experiment to control payment method identifiers instead of making it part of the payment method resolution logic?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This experiment is specifically needed to safely migrate the CBA MPGS and other payment providers that are using hosted fields from the legacy core flow to the new resolver-based package architecture.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My question is narrower: it's about where the gate lives, not whether it exists. Agreed on the rollout safety, the experiment, gradual rollout, and clean revert path. All make sense given the prior incident, no objection there. The point that the routing decision must happen at runtime in the resolver is right (the registry is generated at build time). But that doesn't require an If we do want a generic per-package migration-gate mechanism long term, I think that's worth its own small design rather than an experiment field.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is not long term, we need this for migration maybe 3 providers.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the context. That changes the perspective.
If possible, could you please add this context as comments? My concern is we will soon forget about it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I created task for remove this functional after migration will be completed PI-5464 |
||
| Object.entries(ComponentRegistry).map(([key, value]) => { | ||
| const methods = value.filter((resolveId) => { | ||
| const { experiment } = resolveId as PaymentMethodResolveId; | ||
|
|
||
| if (!experiment) { | ||
| return true; | ||
| } | ||
|
|
||
| // should be normalized after config | ||
| const normalizedKey = experiment.replace('_', '.'); | ||
|
|
||
| return isExperimentEnabled(checkoutSettings, normalizedKey, false); | ||
| }); | ||
|
|
||
| return [key, methods]; | ||
| }), | ||
| ); | ||
|
|
||
| const components = Object.fromEntries( | ||
| Object.keys(ComponentRegistry).map((key) => [ | ||
| Object.keys(filteredMethods).map((key) => [ | ||
|
cursor[bot] marked this conversation as resolved.
|
||
| key, | ||
| allExports[key as keyof typeof allExports], | ||
| ]), | ||
|
|
@@ -22,6 +44,6 @@ export default function resolvePaymentMethod( | |
| return resolveLazyComponent<PaymentMethodResolveId, PaymentMethodProps>( | ||
| query, | ||
| components, | ||
| ComponentRegistry, | ||
| filteredMethods, | ||
| ); | ||
| } | ||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍