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

Refactor payment method buttons and separators rendering #5472

Closed
timur27 opened this issue Jan 27, 2023 · 2 comments · Fixed by #5937
Closed

Refactor payment method buttons and separators rendering #5472

timur27 opened this issue Jan 27, 2023 · 2 comments · Fixed by #5937
Assignees
Labels
category: projects For any issues which are part of any project, including bugs, enhancements, etc. category: refactor The issue/PR is related to refactoring. component: checkout Issues related to Checkout type: technical debt This issue/PR represents/solves the technical debt of the project.

Comments

@timur27
Copy link
Contributor

timur27 commented Jan 27, 2023

Why is the refactoring needed?

There was a fix implemented here and here that resolves the issue of displaying multiple separators on the checkout, cart, product details pages.

After the fix, the button and separator displaying implementations are separated from each other. With that, a few drawbacks are currently visible, and these should be resolved once the refactoring is completed:

  1. In a nutshell, the logic deciding whether we should render the button and the separator is duplicated (see WC_Payments::display_express_checkout_separator_if_necessary for the separator and WC_Payments_*_Button_Handler::init for Platform_Checkout, Payment_Request and Stripe_Link accordingly. This will lead to situations when e.g. the WooPay button logic is changed in the future, we might easily forget to update the logic in the separator rendering class, which will immediately result in displaying the separator alone when the button is disabled.
  2. Separator rendering logic is located in class-wc-payments.php, which doesn't seem to be its direct responsibility.
  3. There's limited test coverage due to static methods in the class-wc-payments.php class.

Ideally, we could create a new class that will merge and orchestrate all the logic from both button and separator rendering in one place and test it out properly.

Acceptance criteria

  1. Button and separator displaying logic is merged into one place.
  2. The bug with the multiple separators doesn't appear (no regression after the changes within this issue).
  3. All the logic is covered with tests.
@timur27 timur27 added type: bug The issue is a confirmed bug. category: refactor The issue/PR is related to refactoring. and removed type: bug The issue is a confirmed bug. labels Jan 27, 2023
@zmaglica zmaglica added type: technical debt This issue/PR represents/solves the technical debt of the project. component: checkout Issues related to Checkout labels Feb 1, 2023
@jessy-p
Copy link
Contributor

jessy-p commented Feb 2, 2023

@Automattic/pulsar

@timur27
Copy link
Contributor Author

timur27 commented Feb 2, 2023

Hey @zmaglica @jessy-p. Currently, this issue can be addressed within the split-UPE project only as the split UPE branch wasn't merged yet. After it's merged, this can be taken basically by any team to work on. Before this happened, I think adding category:projects label and possibly addressing this by @Automattic/heisenberg-woopay-upe-compatibility would be the best option. Sorry for not providing this information earlier.

@timur27 timur27 added the category: projects For any issues which are part of any project, including bugs, enhancements, etc. label Feb 2, 2023
@mdmoore mdmoore self-assigned this Feb 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: projects For any issues which are part of any project, including bugs, enhancements, etc. category: refactor The issue/PR is related to refactoring. component: checkout Issues related to Checkout type: technical debt This issue/PR represents/solves the technical debt of the project.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants