-
Notifications
You must be signed in to change notification settings - Fork 51
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(recaptcha): improvements for reCAPTCHA v2 + modal checkout #3692
base: trunk
Are you sure you want to change the base?
Conversation
BREAKING CHANGE: shippable product orders will auto-complete by default after this change.
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.
Overall things look good! I ran into one maybe-issue that I included at the end.
I ran these checkouts with ELEX and Braintree, and ELEX and Stripe. In most cases, things work as described; I:
- Entered the wrong address fine on the first screen
- Got and passed reCAPTCHA v2 prompt, entered my Braintree test code when using that gateway, and got the Address failed message (not sure if we can change this to remove the colon?):
- I hit the back button, corrected it, entered the credit card info again.
- Got reCAPTCHA checked again, and then saw the email correction screen:
- I picked the original address, entered the Braintree code again when using that gateway, and the transaction went through.
- I also smoke tested without reCAPTCHA and with v3 with both gateways.
The only issue I ran into was using Braintree and subscription products with free trials. I get this error when I try to submit the first screen (it comes from the Braintree plugin):
I found this suggestion, and it does seem to fix the issue but it means no payment method is collected on checkout; best I can tell, readers would be prompted to add payment information later on, which seems subpar:
I don't think this will be an issue for publishers who currently need this support, but I wanted to flag in case it's something that should be fixed before this is merged. I'm making this as "request changes", but that's the only issue I ran into so it should be OK if that's OK!
@laurelfulford thanks for the testing and feedback!
The message can be customized in ELEX plugin settings ("Customise" tab). Re: the issue with Braintree + free trial subscription products, since this seems to be a bug in the Braintree plugin itself and isn't introduced by the modal checkout flows, I don't think think we should let that block us from moving ahead. We should ensure that sites who want to use Braintree are aware of this limitation. For the record, I did attempt to tweak the priority value on the Also noting a bug I noticed in the ELEX plugin, which you can actually see in the screenshots of the address validation popup above: validated addresses drop the "Street 2" field, so that
becomes
Again, this is a bug originating in the ELEX plugin, so fixing it isn't within the scope of our code. |
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 following up on all that stuff, @dkoo!
The message can be customized in ELEX plugin settings ("Customise" tab).
Ah! That's perfect -- nice that they can add some direction there, too 🙂
Re: the issue with Braintree + free trial subscription products, since this seems to be a bug in the Braintree plugin itself and isn't introduced by the modal checkout flows, I don't think think we should let that block us from moving ahead.
Agreed! Thanks for verifying this.
Also noting a bug I noticed in the ELEX plugin, which you can actually see in the screenshots of the address validation popup above... Again, this is a bug originating in the ELEX plugin, so fixing it isn't within the scope of our code.
Good catch, and totally fair! As an aside, Canada Post has an address autocomplete thing you can use here that always bungles my address 😆 These things aren't infallible!
This all looks good to me - giving it a 🚢
All Submissions:
Changes proposed in this Pull Request:
Along with Automattic/newspack-blocks#2028, implements improvements to the reCAPTCHA v2 integration in a modal checkout context. Also introduces a new option (via environment constant, for now) to allow third-party WooCommerce extensions to load their assets in modal checkout, to allow for greater flexibility. Not all Woo extensions are guaranteed to work with the modal checkout, but many will.
These changes came about due to issues some publishers saw when using the ELEX Address Verification plugin—this plugin triggers actions on the
click
event of the#place_order
button in checkout, which is also bound to our reCAPTCHA v2 widget execution. This causes a conflict where only one of these bound events will fire on click—but because both ELEX and reCAPTCHA can block a checkout request upon failure, enabling both will make it impossible to complete checkout.This PR avoids such conflicts by creating a clone of the
#place_order
button in our modal checkout template, hiding the original button element, and binding our reCAPTCHA v2 widget execution to the clone instead of the original. When the reCAPTCHA widget fires its success callback (either upon initial pass or challenge pass), it will trigger aclick
event on the "real"#place_order
button, which will allow other extensions with events bound to that button to do their own thing.To be clear, this PR doesn't indicate full Newspack support for the ELEX plugin—but it will allow for publishers to test and use plugins like ELEX alongside Newspack, if the resulting experience is acceptable.
How to test the changes in this Pull Request:
Pre-testing setup
define( 'NEWSPACK_ALLOW_ALL_CHECKOUT_SCRIPTS', true );
to wp-config.php to allow unsupported Woo extensions to load their assets in modal checkout.https://siteurl/wp-admin/admin.php?page=wc-settings&tab=elex_address_autocomplete_validation§ion=elex-api-credentials
). Also make sure to enable address validation via EasyPost in the Address Validation settings page:Testing steps
Address verification failed
Other information: