-
Notifications
You must be signed in to change notification settings - Fork 217
Moving OC card method logic to the new OC payment method class #4579
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
Moving OC card method logic to the new OC payment method class #4579
Conversation
….com/woocommerce/woocommerce-gateway-stripe into fix/removing-card-requirement-for-oc
…c-card-method-logic-to-single-method-class
Good catch! That was a new validation we recently added for the payment intent. I was able to reproduce it here and fixed it in ce4817a. |
Thanks for the steps to reproduce the issue! I haved fixed it in c28b903
This specific behavior will be handled in STRIPE-678 |
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.
Thanks for the updates @wjrosa. The result of functional testing with OC enabled is as following-
✅ Settings looks good.
✅ Enabled methods are rendered correctly on shortcode checkout page.
✅ Enabled methods are rendered correctly on block checkout page
✅ Card payment is working on both checkout pages.
✅ Redirect payment is working on both checkout pages.
🔴 When all the payment methods are disabled I see an error on shortcode checkout page. Block checkout looks good.
🔴 Payment method name is wrong when I pay with Google Pay
page | develop |
this branch |
---|---|---|
Order received page | ![]() |
![]() |
Order edit page | ![]() |
![]() |
includes/payment-methods/class-wc-stripe-upe-payment-method-oc.php
Outdated
Show resolved
Hide resolved
includes/payment-methods/class-wc-stripe-upe-payment-method-oc.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Mayisha <[email protected]>
… of https://github.com/woocommerce/woocommerce-gateway-stripe into dev/moving-oc-card-method-logic-to-single-method-class
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.
Thanks @wjrosa for the fixes. Confirming that I don't see any of the errors/issues I mentioned in my previous comments.
Settings
✅ OC option is rendered correctly irrespective of the enabled payment methods.
Block checkout
✅ OC element is rendered when there are enabled payment methods.
✅ OC element is not rendered when there are no enabled payment methods. The express methods are also hidden.
✅ Checkout with card is successful.
✅ Checkout with Google Pay is successful and the payment method title is correct.
✅ Checkout with redirect payment method (iDeal/EPS) is successful.
Shortcode checkout
✅ OC element is rendered when there are enabled payment methods.
✅ OC element is not rendered when there are no enabled payment methods. The express methods are also hidden.
✅ Checkout with card is successful.
✅ Checkout with Google Pay is successful and the payment method title is correct.
✅ Checkout with redirect payment method (iDeal/EPS) is successful.

Thanks, Mayisha! I have fixed the availability for the block checkout and created STRIPE-687 to address the same for the shortcode checkout. |
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.
Looks good to me. I left some comments, but they are not merge blockers.
@@ -33,6 +33,7 @@ class WC_Stripe_Payment_Methods { | |||
const SEPA_DEBIT = 'sepa_debit'; | |||
const SOFORT = 'sofort'; | |||
const WECHAT_PAY = 'wechat_pay'; | |||
const OC = 'card'; // This is a special case for the Optimized Checkout |
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.
nit: To avoid any ambiguity, should we set this to its own ID (eg: oc_container
) and then map it to card
where frontend/Stripe expects it?
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.
I tried that, but it caused a lot of bugs (tried it again now and still couldn't do it). We still need some stuff from the card payment method class (even when inactive). That was the way I could make it as independent as possible. We can revisit this later
* @inheritDoc | ||
*/ | ||
public function is_available() { | ||
return true; |
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.
Should we do something like
return true; | |
if ( ! parent::is_available() ) return false; | |
return true; |
I see the is_available
method in WC_Payment_Gateway
(which was in the previous inheritance chain) has some additional checks that might be useful.
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.
Sure. Added it in f58a3fe. I didn't apply your suggestion directly due to linting.
Fixes STRIPE-667
Changes proposed in this Pull Request:
This PR moves all the specific Optimized Checkout logic of the card payment method to a new payment method class. The goal is to improve the code's organization and properly separate its concerns.
Testing instructions
dev/moving-oc-card-method-logic-to-single-method-class
)Changelog entry
Changelog Entry Comment
Comment
Post merge