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

Fix legacy method names in order meta #2934

Merged

Conversation

Mayisha
Copy link
Contributor

@Mayisha Mayisha commented Feb 23, 2024

Fixes #2932

Changes proposed in this Pull Request:

  • In 3fbd3d5 I have reverted the subscription check for Multibanco. I had added this check in Move Alipay and Multibanco inside Stripe settings when UPE is enabled #2870 to hide Multibanco as it was showing up as a payment method while purchasing a subscription when UPE was enabled. However, upon testing in trunk I found that trunk has the same behavior for Multibanco in UPE and all other methods when UPE is disabled. In short, the existing behavior in the latest releases are, when you have a subscription product in cart, the non-UPE methods using Sources API is available as an option to checkout but the subscription will be created as a manually renewed subscription.
  • In 25594a8 reverted the changes I made in PR Update the interface for customizing Stripe payment methods #2839. I removed all the stripe methods from add_gateways method which was called by woocommerce_payment_gateways so that they don't appear on the WooCommerce > Settings > Payments page. As a result WC()->payment_gateways->payment_gateways() does not have these Stripe methods listed. As a result, here the order can not find the stripe payment method object (just has the id) and finally just renders the id here. This is the reason behind the order meta payment method name issue in New settings page issue #2932. (There could be other places implicated by WC()->payment_gateways->payment_gateways() not having the stripe payment methods, though I have not found others yet)
  • As in 25594a8 I am adding the stripe payment methods to the list again, they are visible on the WooCommerce > Settings > Payments page. So in 803835f I am hiding them using css.
  • To show the payment methods in the order saved inside Stripe settings page and not in the order of the outer settings page (where they are hidden), in baf0589 I am unsettling them from the gateway object before using them to the expected position.

Testing instructions

Reproduce the bug

  • Go to the develop branch.
  • Disable the New checkout experience from the Stripe settings page.
  • Set Euro as store currency and enable a few payment methods on the settings page (like EPS, giropay etc.)
  • Click the customize button, set a title for the enabled methods or keep it as before.
  • As a shopper, add a product to the cart and checkout with any Stripe enabled methods except card.
  • After placing the order, in your wp dashboard go to this order edit page.
  • Notice that the payment method name is displayed as stripe_xyz instead of the method title.

wpm

Test the fix

  • Go to the fix/2932-non-upe-method-in-order-and-checkout branch (this branch).
  • Disable the New checkout experience from the Stripe settings page.
  • Set Euro as store currency and enable a few payment methods on the settings page (like EPS, giropay etc.)
  • Click the customize button, set a title for the enabled methods or keep it as before.
  • As a shopper, add a product to the cart and checkout with any Stripe enabled methods except card.
  • After placing the order, in your wp dashboard go to this order edit page.
  • Make sure that the payment method name is displayed as the one you have set as the title.

cpm

Payment method order in checkout page

  • Go to the fix/2932-non-upe-method-in-order-and-checkout branch (this branch).
  • Disable the New checkout experience from the Stripe settings page.
  • Reorder payment methods and customize some of their names.
  • Reorder the main Stripe method in WooCommerce > Settings > Payments. (cod, stripe, cheque)
  • As a shopper, go to the checkout page and ensure that the ordering is correct. Expected ordering would be cod, all the Stripe methods as ordered inside the Stripe settings page, cheque.
  • Reorder payment methods in Stripe settings page, refresh the checkout page as a shopper and make sure the order is correct.

Add payment method page

  • Enable UPE
  • Enable a few methods including Card, Sepa, iDeal and Bancontact.
  • Go to My Account > Add payment method page.
  • When you try to add a payment method Card, Sepa, iDeal and Bancontact should be available as options.
  • Disable UPE
  • Enable a few methods including Card and Sepa
  • Go to My Account > Add payment method page.
  • When you try to add a payment method Card and Sepa.

@Mayisha Mayisha requested review from a team and wjrosa and removed request for a team February 23, 2024 08:50
@a-danae a-danae self-requested a review February 24, 2024 03:01
Copy link
Contributor

@a-danae a-danae left a comment

Choose a reason for hiding this comment

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

Thanks for these changes, @Mayisha! Glad we caught this!

Here's the summary from a a partial round of testing.

I was able to replicate the issue in develop and successfully tested these flows:

  • Payment gateway name in order details. The APM’s name is displayed in the order details, not its gateway ID
  • Reordering the payment methods in WooCommerce > Payments. All of the payment methods from Stripe are displayed together. This group is ordered as expected in relationship with the payment methods introduced by WooCommerce and other extensions, like PayPal Payments, on the checkout pages.
  • Reordering the Stripe Payment methods. They are ordered as expected in relationship with each other on the classic checkout page.
  • SEPA and Cards are displayed on the Add new payment method page when UPE is disabled.

🔴 I also noticed the following unexpected behavior. There’s a fatal error when visiting the Add new payment method page with UPE enabled.

Undefined variable: gateway in /var/www/html/wp-content/plugins/woocommerce-gateway-stripe/includes/payment-methods/class-wc-stripe-upe-payment-gateway.php on line 196
Uncaught Error: Call to a member function supports() on null in /var/www/html/wp-content/plugins/woocommerce-gateway-stripe/includes/payment-methods/class-wc-stripe-upe-payment-gateway.php:196
Stack trace:
#0 /var/www/html/wp-includes/class-wp-hook.php(324): WC_Stripe_UPE_Payment_Gateway->get_available_payment_gateways(Array)
#1 /var/www/html/wp-includes/plugin.php(205): WP_Hook->apply_filters(Array, Array)
#2 /var/www/html/wp-content/plugins/woocommerce/includes/class-wc-payment-gateways.php(333): apply_filters('woocommerce_ava...', Array)
#3 /var/www/html/wp-content/plugins/woocommerce-paypal-payments/modules/ppcp-button/src/Assets/SmartButton.php(535): WC_Payment_Gateways->get_available_payment_gateways()
#4 /var/www/html/wp-content/plugins/woocommerce-paypal-payments/modules/ppcp-button/src/Assets/SmartButton.php(279): WooCommerce\PayPalCommerce\Button\Assets\SmartButton->render_button_wrapper_registrar()
#5 /var/www/html/wp-content/plugins/woocommerce-paypal-payments/modules/ppcp-button/src/ButtonModule. in /var/www/html/wp-content/plugins/woocommerce-gateway-stripe/includes/payment-methods/class-wc-stripe-upe-payment-gateway.php on line 196

I'll check out this error, check other flows, and review the code next.

@a-danae a-danae changed the base branch from develop to add/deferred-intent February 25, 2024 14:11
@a-danae
Copy link
Contributor

a-danae commented Feb 25, 2024

So, the error I mentioned before should have been fixed by 4f01c4b.

I updated the branch this PR points to from develop to add/deferred-intent because I think it'll be better to solve the incoming conflicts while testing this PR, rather than when updating add/deferred-intent. Hope it's okay.

Copy link
Contributor

@a-danae a-danae left a comment

Choose a reason for hiding this comment

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

Things tests well, thanks @Mayisha 🙂

I've pushed an update and will proceed to merge it so GlobalStep can include these changes in their test round on Monday. Please feel free to review those when you're back!

I think it'd be good to revisit it, since hiding the gateways from the Payment Methods page with CSS seems a bit fragile. I'd be curious if there's a more robust approach but I couldn't dive there. I wonder if WooPayments needed to handle a similar scenario.

Approving and proceeding to merge.

@a-danae a-danae merged commit 8f0db94 into add/deferred-intent Feb 25, 2024
32 checks passed
@a-danae a-danae deleted the fix/2932-non-upe-method-in-order-and-checkout branch February 25, 2024 23:04
@mattallan
Copy link
Contributor

Nice PR @Mayisha! These changes also fixed refunding via Multibanco 🎉

Previously if you purchased an order with Multibanco, there was no "Refund $$ via Multibanco" button on the Edit Order page, only manual refunds were supported.

The required change to fix this is to add Multibanco to our add_gateways() function so that it's returned when WooCommerce Core calls WC()->payment_gateways->payment_gateways().

I think it'd be good to revisit it, since hiding the gateways from the Payment Methods page with CSS seems a bit fragile. I'd be curious if there's a more robust approach but I couldn't dive there. I wonder if WooPayments needed to handle a similar scenario.

@a-danae @Mayisha James added a new function recently to hide payment methods on the WooCommerce payment methods list page here:

public static function hide_gateways_on_settings_page() {

We should be able to replace the CSS approach in this PR by adding Multibanco and any other non-UPE classes to the hide_gateways_on_settings_page() function. I don't think it's not a priority to get this fixed before GlobalStep testing, but I'll open up a PR with this change

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.

New settings page issue
3 participants