-
Notifications
You must be signed in to change notification settings - Fork 45
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: add supported gateways check #2009
base: trunk
Are you sure you want to change the base?
Conversation
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.
Ran into a few minor issues here. Aside from these, the redirect is working in all test scenarios.
-
After creating a new account, when being directed to the regular checkout page the
_newspack_checkout_registration
param is still appended to the URL. -
When logging into an existing account via OTP, if you click "Continue" without filling in the code or after entering an incorrect code, the auth modal still has a translucent overlay on it that doesn't go away:
- If I get redirected to the regular checkout page and then hit the "back" button in my browser, the Checkout Button or Donate block remains in the "loading" state and the extra params are appended to the URL.
src/modal-checkout/modal.js
Outdated
} ); | ||
|
||
// Generate URL for non-modal checkout | ||
const generateNonModalCheckoutUrl = ( url ) => { |
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.
Nit: rather than relying on regex and text manipulation of the URL, a cleaner way to do this might be to not append the modal checkout-related hidden fields if there's an unsupported payment gateway.
Thanks @dkoo! Something else I noticed while fixing these is that the Donate Button doesn't actually animate for me unless I also have the checkout button block on the same page. When you tested this, did you have both blocks on that donate page? I've made some changes and I'm trying to rule out if it's something I've introduced or something that was already there 😅 |
I figured this out - I was blocking the JS file from loading due to changes I made to the Donate Block 😅 I've tweaked things so it should now work. |
Thank you for testing this, @dkoo! I think everything should be fixed. When you re-check this, I could definitely use a gut-check about the changes in 6aa043d. This is the approach I used to fix the back button "still loading" issue -- I noticed when I started testing that, that the registration modals also would hang around when you clicked "Back". I ended up moving the "close" const to make it available outside of |
Yes, I did have both blocks on the same testing page! |
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.
I like the new method of not including hidden fields in the first place when they're not needed!
The Donate and Checkout Button blocks still get stuck in a "loading" state if I click back from the checkout page, but that's a minor issue and we can consider it non-blocking if we can't get it working in this PR.
src/modal-checkout/modal.js
Outdated
window.location.href = generateNonModalCheckoutUrl( url ); | ||
generateCart( formData ).then( url => { | ||
// Remove modal checkout query string and trailing question mark (if any). | ||
window.location.href = url.replace( /&?modal_checkout=1/, '' ).replace( /\?$/, '' ); |
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.
Ideally we'd also prevent the ?modal_checkout=1
param from being appended too so we wouldn't need regex to strip it. It might be possible to add a condition here for this, but if it adds too much complication then it's okay as-is.
Thanks @dkoo! I'm digging into the |
I'm testing on Chrome |
Thanks @dkoo! I think I've sorted out the query string issue, and I hope the back button issue, too 🤞 In retesting this, I ran into a new issue where, if you had a unsupported payment gateway enabled and then simply signed in using the button in the header, you couldn't close the modal by clicking the 'Continue' button, only the 'Close' button. That should also be fixed in the Newspack Plugin PR that goes with this one. Could you please retest when you have a moment and make sure things look good to you, too? I'm kind of paranoid about introducing some weird fresh issue 😬 Thank you! |
All Submissions:
Changes proposed in this Pull Request:
Along with Automattic/newspack-plugin#3650, adds a check to see if there's an unsupported payment gateway enabled in WooCommerce. If there is, the Donate Block and Checkout Button block will use the regular checkout instead of the modal checkout.
See 1205234045751551-as-1208992708114862
How to test the changes in this Pull Request:
Testing set up
npm run build
for each.Testing with unsupported gateways
It's a lot, but ideally most of these steps should be repeated with a couple different block types, including Checkout Button block + simple product, Checkout Button block + product with variations, Donate Block, and Donate Block with tiers.
Account creation required & create account
Please test this with a simple product, a product with variations, and one of the donate blocks.
after_success
params.Account creation required & log in
Please test this with a simple product, a product with variations, and one of the donate blocks.
after_success
params.Account creation required & already logged in
Please test this with a simple product, a product with variations, and both donate blocks.
Account creation required & login failures
Smoke test a few scenarios where the login is not successful, like:
... plus any other cases you can think of. Most of these are to spook out any issues with my messing with the registration modal (making it not close, or messing with the loading styles 🙂 ).
Account creation not required
Testing with supported gateways
Before testing this section, navigate to WooCommerce > Settings > Payments, and deactivate any unsupported payment gateways.
Run through the following with 1-2 blocks each, just to smoke test and make sure no new issues have been introduced:
Other information: