-
Notifications
You must be signed in to change notification settings - Fork 397
feat(payment): PI-4306 Switch ClearPay to use generic hosted payment … #2675
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?
Conversation
7a6ad74 to
80561ba
Compare
|
|
||
| export default toResolvableComponent<PaymentMethodProps, PaymentMethodResolveId>( | ||
| HostedPaymentMethod, | ||
| [{ gateway: 'afterpay' }, { id: 'quadpay' }, { id: 'sezzle' }, { id: 'zip' }], | ||
| [ | ||
| { gateway: 'afterpay' }, | ||
| { gateway: 'clearpay' }, | ||
| { id: 'quadpay' }, | ||
| { id: 'sezzle' }, | ||
| { id: 'zip' }, | ||
| ], | ||
| ); |
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.
Bug: HostedPaymentMethod incorrectly initializes Afterpay, Clearpay, Quadpay, and Sezzle with createZipPaymentStrategy.
Severity: CRITICAL | Confidence: 0.95
🔍 Detailed Analysis
The initializeHostedPaymentMethod function in HostedPaymentMethod.tsx unconditionally uses createZipPaymentStrategy for all payment methods it handles. This occurs when initializing Afterpay, Clearpay, Quadpay, or Sezzle payment methods. As a result, these payment methods are initialized with an incompatible strategy, which will likely cause the payment initialization process to fail or behave unexpectedly within the checkout SDK. This deviates from the established pattern where each payment method uses its specific strategy.
💡 Suggested Fix
Modify initializeHostedPaymentMethod to dynamically select and pass the correct payment strategy based on the options.methodId or options.gateway instead of hardcoding createZipPaymentStrategy.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/hosted-payment-integration/src/HostedPaymentMethod.tsx#L41-L51
Potential issue: The `initializeHostedPaymentMethod` function in
`HostedPaymentMethod.tsx` unconditionally uses `createZipPaymentStrategy` for all
payment methods it handles. This occurs when initializing Afterpay, Clearpay, Quadpay,
or Sezzle payment methods. As a result, these payment methods are initialized with an
incompatible strategy, which will likely cause the payment initialization process to
fail or behave unexpectedly within the checkout SDK. This deviates from the established
pattern where each payment method uses its specific strategy.
Did we get this right? 👍 / 👎 to inform future reviews.
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.
Hey @bc-sebastianszafraniec can you please take a look at this comment.
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.
Hi @animesh1987
I think it is due to your changes in this PR from yesterday #2666 and hardcoded integrations: [createZipPaymentStrategy]. Why did you hardcoded it like that?
However I tested it and clearpay-payment-strategy.ts is used properly for Clearpay (attached video in testing section). So despite that it seams to work fine.
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.
Hey @bc-sebastianszafraniec this is a change we are doing to pass integrations directly to components. So integrations can be lazy loaded. We are updating more here https://github.com/bigcommerce/checkout-js/pull/2679/files#diff-721f997276ab34f7235b5537c9bdbb98751a72ead6448a162d602b9f626a843aR27. So if you see that a component is used by an integration, we need to pass the strategy as well.
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.
Hi @animesh1987 Sorry for late response, I was on PTO.
I updated the code and passed createClearpayPaymentStrategy similar to other payment strategies. I also updated the attached screen recording from testing.
5eec1e5
80561ba to
5eec1e5
Compare
…method component
What/Why?
To avoid unnecessary code duplication, Clearpay payment method is moved from its own package to a generic hosted payment method, as it doesn't require any provider-specific logic. Clearpay-integration package is removed.
Rollout/Rollback
Revert this PR
Testing
Unit tests passed
Tested manually:
Clearpay.mov