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 error when discarding changes to the display order #3698

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

malithsen
Copy link
Contributor

@malithsen malithsen commented Jan 9, 2025

Fixes #3697

Changes proposed in this Pull Request:

When changes to the payment method order list are discarded via the cancel button, it leads to an empty settings screen. This happens because it tries to find a non-existent Link payment method in the mapping list. This PR fixes it by returning early if the Icon and label fields for a payment method are not found on the mapping.

Before
Image

Now
CleanShot 2025-01-09 at 11 17 53

Testing instructions

Test the fix

  1. Go to WooCommerce -> Settings -> Payments -> Stripe
  2. Click on "Change display order" button at the top right.
  3. Click the cancel button.
  4. Settings page should not be an empty screen.

Regression tests

  • Make sure changes to the payment method list are retained and reflected on the checkout page.

  • Covered with tests (or have a good reason not to test in description ☝️)
  • Added changelog entry in both changelog.txt and readme.txt (or does not apply)
  • Tested on mobile (or does not apply)

Post merge

When changes to the payment method order list are discarded via the
cancel button, it leads to an empty settings screen. This happens
because it tries to find the Link payment method in the mapping list but
fails and throw an error. We fix it by returning early if the Icon and
label fields for a payment method are not found on the mapping.
@malithsen malithsen requested review from a team and Mayisha and removed request for a team January 9, 2025 17:29
Copy link
Contributor

@Mayisha Mayisha left a comment

Choose a reason for hiding this comment

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

Thank you @malithsen for the fix. With these additional checks, I don't see the empty screen issue anymore. 👍

But there is another issue. It shows a wrong list after I click the cancel button.

Screen.Recording.2025-01-10.at.11.58.38.AM.mov

We should fix it here as well to render the correct order.

The empty screen error is happening because upePaymentMethods in the above line includes link in the list. Also, the upePaymentMethods list is different than the orderedPaymentMethodIds list in terms of ordering.
Using setOrderedPaymentMethodIds( orderedPaymentMethodIds ); here won't help as we are updating orderedPaymentMethodIds when user drags a method to reorder.

We have to either store the initial data of orderedPaymentMethodIds in this component to use them when cancel button is clicked or keep the list in different state variable in this component. You may have some other better solutions too.

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.

Cancelling changes to the payment method display order results in an empty screen
2 participants