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

ACSS: Refactor unsupported deferred intent in the blocks checkout #3866

Merged
merged 49 commits into from
Feb 26, 2025

Conversation

ricardo
Copy link
Member

@ricardo ricardo commented Feb 11, 2025

Fixes #3851

This PR is complementary to #3805.

Changes proposed in this Pull Request:

  • Add the required changes for mounting non-deferred intent payment methods in the blocks checkout.
  • Fix minor issue with fonts in the blocks checkout.
  • Implement loading state for non-deferred intent payment methods while the PI is being created.

Testing instructions

Follow testing instructions from #3805, but using the blocks checkout.


  • 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

ricardo and others added 30 commits January 22, 2025 15:40
…merce/woocommerce-gateway-stripe into fix/3804-refactor-deferred-intent
Co-authored-by: César Costa <10233985+cesarcosta99@users.noreply.github.com>
Co-authored-by: César Costa <10233985+cesarcosta99@users.noreply.github.com>
appearance,
paymentMethodCreation: 'manual',
fonts: getFontRulesFromPage(),
Copy link
Member Author

Choose a reason for hiding this comment

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

Here we're also fixing a missing fonts prop which resulted in a minor style issue:

Before:
no-fonts

After:
fonts

@ricardo ricardo marked this pull request as ready for review February 11, 2025 14:17
@ricardo
Copy link
Member Author

ricardo commented Feb 11, 2025

Assigned @cesarcosta99 and @wjrosa as reviewers since this PR is a follow up from #3805.

Copy link
Contributor

@wjrosa wjrosa left a comment

Choose a reason for hiding this comment

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

Code is good and works as expected! I was able to place an order following the test steps.

One bug I noticed (maybe it is something related to my environment only) is that if I don't provide a phone number I get the following error:

Screenshot 2025-02-11 at 15 49 21

If it is something happening to everyone, we need to make the phone number required for this method.

Base automatically changed from fix/3804-refactor-deferred-intent to develop February 13, 2025 10:02
Copy link
Contributor

@cesarcosta99 cesarcosta99 left a comment

Choose a reason for hiding this comment

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

Code looks good and tests well 🎉 I spotted some issues, though:

  1. I experienced the same as @wjrosa and get the error Please provide complete payment details. when I don't provide a phone number. I tested this in both classic and Blocks checkouts and both errored. We'll likely need to enforce a phone requirement in the forms, maybe not something to address in this PR, but at least something to add to ACSS: Handle Errors and Edge Cases #3830.
  2. When trying to checkout with ACSS in a clean session (e.g. incognito mode), the payment method doesn't display as I fill out the form. Instead, I need to refresh the page to be able to see and select the method. I only experienced this in Blocks checkout:
output.mp4

@ricardo
Copy link
Member Author

ricardo commented Feb 22, 2025

When trying to checkout with ACSS in a clean session (e.g. incognito mode), the payment method doesn't display as I fill out the form.

@cesarcosta99 This was fixed by @asumaran here, so now it should work, can you try one more time?

@cesarcosta99
Copy link
Contributor

@cesarcosta99 This was fixed by @asumaran here, so now it should work, can you try one more time?

That fixed it 👍 It was not clear if we're going to address the phone requirement here, so I held off the full review for now.

Also, we have a new process for handling changelogs and you need to run npm run changelog to provide one.

@ricardo ricardo requested a review from cesarcosta99 February 25, 2025 18:49
@ricardo
Copy link
Member Author

ricardo commented Feb 25, 2025

@cesarcosta99 I fixed the phone number issue in e09db26. Can you give a final review?

Here's what was happening:

  • In the classic checkout, we check if a field is required by checking if it's visible (src). This makes sense since there are extensions to hide fields from the classic checkout.
  • However, the phone number field can be optional in WooCommerce's native settings but isn't hidden by default. In case we pass phone: never to the PI creation, it will expect a phone number in the payment method creation (which inherits the PI that was created, so this is only valid for ACSS and non-deferred intent PMs).
  • If phone: auto, the phone number parameter also can't be an empty string.

As far as I know, this change shouldn't affect deferred intent payment methods, but phone: auto should fix it for ACSS.

cc @wjrosa in case you have thoughts about e09db26 since you mentioned this as well.

Copy link
Contributor

@cesarcosta99 cesarcosta99 left a comment

Choose a reason for hiding this comment

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

LGTM and tests well! Thank you for working on the changes!

  • Test 1: Successful checkout ✅
    • Blocks and shortcode with no phone provided
  • Test 2: Checkout with micro deposits verification + delayed notification ✅
    • Blocks and shortcode with no phone provided
  • Test 3: Disable feature flag ✅

@ricardo ricardo enabled auto-merge (squash) February 26, 2025 16:05
@ricardo ricardo merged commit 6b6cf50 into develop Feb 26, 2025
41 checks passed
@ricardo ricardo deleted the fix/3851-non-di-blocks-pm branch February 26, 2025 16:07
@wjrosa
Copy link
Contributor

wjrosa commented Feb 26, 2025

@cesarcosta99 I fixed the phone number issue in e09db26. Can you give a final review?

Here's what was happening:

  • In the classic checkout, we check if a field is required by checking if it's visible (src). This makes sense since there are extensions to hide fields from the classic checkout.
  • However, the phone number field can be optional in WooCommerce's native settings but isn't hidden by default. In case we pass phone: never to the PI creation, it will expect a phone number in the payment method creation (which inherits the PI that was created, so this is only valid for ACSS and non-deferred intent PMs).
  • If phone: auto, the phone number parameter also can't be an empty string.

As far as I know, this change shouldn't affect deferred intent payment methods, but phone: auto should fix it for ACSS.

cc @wjrosa in case you have thoughts about e09db26 since you mentioned this as well.

I think it is a nice solution. Thanks for looking into it 👍

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.

ACSS: Refactor unsupported deferred intent in the blocks checkout
3 participants