Display Checkout block with a notice when adding a product sold individually that was already in the cart #2854
Conversation
Aljullu
added
type: bug
|
Size Change: 0 B Total Size: 1.66 MB ℹ️ View Unchanged
|
|
@Aljullu just so I understand this, is the notice thats showing in this example coming from code unrelated to blocks? i.e. core error notices from the query string? Would another way to solve this be to clear notices before running the checkout logic and converting notices to exceptions? |
Correct, that error is thrown by WC Core when a product is added to the Cart. The issue we are facing in #2670 is that checkout data is not returned by
It might, but this part of the codebase is totally new to me, so I trust you more. 😄 From my understading we are already converting notices to exceptions, right? Clearing notices before running the checkout logic would fix the issue of the Checkout data not being available, but we would not be displaying any feedback to the user that the product was not added to the cart. |
|
It's tricky because we don't want non-checkout notices to pollute the checkout. I also don't think it's desirable to this query string to be passed to the API requests (is that what is happening?)—I only see usage of the exception conversion in the API itself. Are we somehow passing the add-to-cart string to the API requests? That might be a bug? I think the purpose of |
|
Edited description to fix issue link (was linking to #2679).
Thanks for digging into this 👍 We could use a link in some cart error notices, e.g. "please log in to your existing account" - hit this in #2851 |
I don't think so, the initial contents of the Checkout page are hydrated directly via PHP and we don't pass any
If we plainly discarded them, there would be no feedback for the user that the product was not added to their cart. What I was trying to achieve with this PR is that notices thrown by internal logic are not converted to exceptions but are not discarded either, instead they would be passed in a new |
That would mean we need to define own custom message, no? The issue I see from that (if I'm understanding it right) is that all this logic is currently in WC Core, so Blocks doesn't really know what error happened. |
Good spot. It doesn't totally make sense to me where these notices are coming from in that case, I guess added just before that hydration occurs. If that is indeed the case, we could hydrate and clear existing notices to another property if that's easier. I defer to your judgement :) |
This comment should not block this PR at all – I was just mentioning a feature request / need that's possibly related. That being that error notices might need links. Specifically, some existing WC core error notices do contain links, and we might strip the links out when we render the notices (I'm stripping the links in #2851). Here's a link to the related comment thread: #2851 (review) But this is all pretty tangential (unrelated) to this PR and shouldn't block it, hopefully this clarifies things :) |
Aljullu
force-pushed
the
fix/2670-add-to-cart-checkout-error
branch
from
5234f0e
to
0395a49
Compare
Aljullu
force-pushed
the
fix/2670-add-to-cart-checkout-error
branch
from
0395a49
to
cb65fb0
Compare
|
Updated this PR so it should be ready for review again. I updated the first comment with details on what's changed.
Ah, yes. The In this PR specifically, I don't think it really matters because the link goes to the Cart page, but that notice will be shown in the Checkout, so I think it's better not to show it. |
There was a problem hiding this comment.
Thanks for figuring this out and implementing a robust solution @Aljullu !
First up a disclaimer - I'm conflicted about this, it feels like a bit of an edge case, or more specifically a general bug, not specific to cart or checkout.
I did some testing with some add-to-cart buttons with various page urls (cart block, checkout block, a regular page, cart shortcode, checkout shortcode. I wanted to explore how this works currently to fully understand the issue.
Here's what I found:
- If the destination is a regular page, the notice is not shown on the page. This feels like a bug, or more likely, is an edge-case (indicating that this is not typical setup).
- If you click the
Add to cartbutton multiple times, and the destination is a page, multiple notices stack up behind the scenes. - When a cart or checkout shortcode page is eventually viewed (maybe some time later), all these
You cannot addnotices are rendered at top. This is informative, but maybe unnecessary IMO - maybe it's more helpful to just show cart/checkout with the single item added and the customer can proceed to checkout :)
With the cart & checkout blocks, I confirmed that this PR improves things by showing the error as a notice above checkout (but not cart).
Long term, I'm thinking we'll provide new ways for merchants to build a landing page with an Add to cart button, and that might be the cleanest "fix" for these kinds of problems. The best time to show the You can't add error is as close to the user action as possible, so an AJAX error would be ideal.
Perhaps this PR is more of an interim solution, or polish to protect inappropriate errors from causing confusion in cart & checkout block.
With that in mind, perhaps it's less important to catch and replay ALL notices generically, and maybe simpler to catch and discard, since the notices may be from some time ago. Or catch and enqueue, so the client can opt-in to displaying notices that are relevant to it; for example, cart/checkout blocks could display a whitelist of relevant notices, and if there are duplicates, just show one copy.
Can you think of any other error notices that might "leak" into cart & checkout in this way? I'm guessing there are other ways to "trick" Woo by passing query params like add-to-cart to unusual pages.
Super tricky issue!
src/BlockTypes/Checkout.php
Outdated
| * @param string $text Text to remove tags from. | ||
| */ | ||
| protected function strip_tags_content( $text ) { | ||
| return preg_replace( '@<(\w+)\b.*?>.*?</\1>@si', '', $text ); |
There was a problem hiding this comment.
Is there a library routine we can use to strip html more reliably - e.g. wp_strip_all_tags? Seems risky to implement/maintain a custom regex for this.
There was a problem hiding this comment.
In the end I changed how this is done (80568e9). Now, HTML is stripped in the frontend using a library WC Admin seems to use too. In some way I think it's more correct to let the client decide whether to display HTML markup or not.
This raises the bundle size a bit, so I'm happy for other ideas.
| @@ -27,6 +27,7 @@ export const DEFAULT_STATE = { | |||
| calculatingCount: 0, | |||
| orderId: checkoutData.order_id, | |||
| customerId: checkoutData.customer_id, | |||
| notices: checkoutData.notices, | |||
There was a problem hiding this comment.
Can we make the intention/purpose of this more clear? It's not a generic notices api, as far as I understand, it's more of a special case, catching orphaned errors and allowing the client to optionally render them.
I don't have any great ideas for what to rename this ... will have a think :)
| @@ -99,6 +100,14 @@ const CheckoutProcessor = () => { | |||
| setIsSuppressed( expressPaymentMethodActive ); | |||
| }, [ expressPaymentMethodActive, setIsSuppressed ] ); | |||
|
|
|||
| useEffect( () => { | |||
| if ( Array.isArray( checkoutNotices ) ) { | |||
| checkoutNotices.forEach( ( notice ) => { | |||
There was a problem hiding this comment.
I think this needs a comment to clarify the special case in play here. Again, I'm struggling to come up with a good comment!
assets/js/type-defs/contexts.js
Outdated
| @@ -245,6 +245,8 @@ | |||
| * order if one exists. | |||
| * @property {boolean} hasOrder True when the checkout has a | |||
| * draft order from the API. | |||
| * @property {string} notices Notices associated to the | |||
There was a problem hiding this comment.
This is a little misleading. As I understand it, these are errors that were encountered at some point in the past, and are now getting rendered as part of this block (but might have nothing to do with it). So they aren't necessarily associated with checkout data (though that's the mechanism that sends them to client).
| @@ -106,6 +106,7 @@ Via `useCheckoutContext`, the following are exposed: | |||
| - `orderId`: The order id for the order attached to the current checkout. | |||
| - `isCart`: This is true if the cart is being viewed. Note: usage of `CheckoutProvider` will automatically set this to false. There is also a `CartProvider` that wraps children in the `ShippingDataProvider` and exposes the same api as checkout context. The `CartProvider` automatically sets `isCart` to true. This allows components that implement `useCheckoutContext` to use the same api in either the cart or checkout context but still have specific behaviour to whether `isCart` is true or not. | |||
| - `hasOrder`: This is true when orderId is truthy. | |||
| - `notices`: An array of notices or messages associated to the current checkout data. For example, errors from the server which are not blocking the purchase process but have to be shown to the user. | |||
There was a problem hiding this comment.
This a bit clearer - these notices could be anything really, they might be a general store problem/notice encountered in some previous request.
There was a problem hiding this comment.
Since the notices are captured as part of hydration (only), maybe we can call them captured_hydrate_notices. Again, this is not very clear at all 😄
src/BlockTypes/Checkout.php
Outdated
| /** | ||
| * Hydrate the checkout block with data from the API. | ||
| * | ||
| * @param AssetDataRegistry $data_registry Data registry instance. | ||
| */ | ||
| protected function hydrate_from_api( AssetDataRegistry $data_registry ) { | ||
| // Save previous notices so we can return them in a `notices` property to | ||
| // be used in the frontend. | ||
| $previous_notices = wc_get_notices( 'error' ); |
There was a problem hiding this comment.
Do we need to limit this to error? I think a similar situation could possibly occur with other notice types (maybe contrived/theoretical).
There was a problem hiding this comment.
I removed this limitation. 🙂
src/BlockTypes/Checkout.php
Outdated
| if ( ! $data_registry->exists( 'cartData' ) ) { | ||
| $data_registry->add( 'cartData', WC()->api->get_endpoint_data( '/wc/store/cart' ) ); | ||
| } | ||
| if ( ! $data_registry->exists( 'checkoutData' ) ) { | ||
| add_filter( 'woocommerce_store_api_disable_nonce_check', '__return_true' ); | ||
| $data_registry->add( 'checkoutData', WC()->api->get_endpoint_data( '/wc/store/checkout' ) ); | ||
| $checkout_data = WC()->api->get_endpoint_data( '/wc/store/checkout' ); | ||
| if ( is_array( $previous_notices ) && count( $previous_notices ) > 0 ) { |
There was a problem hiding this comment.
This code is the crux of this new feature - capture and then send any dangling/orphaned notices, to prevent them being misinterpreted in checkout processing.
If we think this is worthwhile, it might be worth refactoring the code to methods so it's clearly separated from the main checkout/cart assets logic. E.g.
// Capture notices, strip html, convert to array ready for enqueuing as script data.
$irrelevant_notice_data = capture_and_clear_notices();
// Then later, optionally include the notice data in response if useful.
$checkout_data = array_merge( $checkout_data, [ 'captured_notices' => $irrelevant_notice_data ] );There was a problem hiding this comment.
If we do this for checkout, we should do it for cart block too (and consider other cases).
There was a problem hiding this comment.
If we do this for checkout, we should do it for cart block too (and consider other cases).
The Cart block shouldn't be affected. The issue happened because Checkout is calling convert_notices_to_exceptions, that means any notice would prevent the Checkout block from rendering and instead an error would be shown. Cart doesn't call that method so it's not affected and it shows notices like other pages.
|
I grabbed this and reviewed since Mike may be out for a bit 🏖️ 👾 |
|
Tangentially related enhancement issue - ensure blocks notices support actions :) #2985 |
Aljullu
force-pushed
the
fix/2670-add-to-cart-checkout-error
branch
2 times, most recently
from
262dac8
to
80568e9
Compare
|
Love all the suggestions @haszari! I implemented most of them and I will answer some of your feedback.
The issue is only reproducible in the Checkout block because it calls
💯 However, there will still be the case of links in the form
Correct, I think WC should give users feedback as soon as possible without requiring a page reload. But that's not yet possible with notices generated in Core and there will still be cases in which notices are generated on page load, as described above. So we still need a system so previously-generated notices don't break our blocks.
I don't, but that's just because my WC knowledge is limited. 🙂 I'm pretty sure there must be some other ways to trigger errors or notices right before the block logic is run. I renamed several variables with your suggestions. In the API I named it This PR is ready for another look. |
Aljullu
force-pushed
the
fix/2670-add-to-cart-checkout-error
branch
2 times, most recently
from
f9c78e3
to
54d3d6b
Compare
Aljullu
force-pushed
the
fix/2670-add-to-cart-checkout-error
branch
from
54d3d6b
to
7289061
Compare
|
Thanks for the in-depth review @haszari! After digging a bit more into this, it looks like (maybe) this could be fixed with just one line. 🤦 Printing the notices before running the block logic was enough to ensure they are empty when reaching
I'm still unable to reproduce this. Let me know if you can still reproduce with the last changes in this pull. |
There was a problem hiding this comment.
This is looking great. Fantastic effort finding such a clean/lightweight fix 🏅
In my testing I still saw some quirky behaviour, but I think this might actually be a Twenty Twenty bug (maybe). I don't think this is a blocker, so I'm pre-approving, but we might need some follow up issues.
By the way – I think in my previous testing I ONLY tested Twenty Twenty. So possibly some of what I was reporting was due to that. This time I used Storefont too.
Note I also think it's useful to compare behaviour with / without blocks plugin active. I'm not certain, but it's possible that hydrate_from_api is getting called on all pages, possibly causing different notice behaviour with/without blocks plugin. (Sorry if this sounds like FUD/voodoo!)
In my test setup, I have a blog post with a bunch of "add belt to cart" buttons. Each one links to the respective url with the add-to-cart param:
- Block-based checkout page.
- Block-based cart page.
- Regular site page (e.g.
AboutorContact). - Shortcode cart page.
- Shortcode checkout page.
With your new changes, and Storefront theme, all the cases I tested work well.
- Clicking
Buy now pagebutton now correctly shows 1 "already in cart" notice on the page.- I can't get multiple "already in cart" notices stacked up behind the scenes (and then rendered on an unsuspecting checkout). I think this is because the notices are caught and rendered sooner, i.e. when the regular page is rendered.
- All other buttons also show 1x "already in cart" notice at top.
With Twenty Twenty (1.5) ...
I'm a bit baffled by these issues. Since it's working well with Storefront, perhaps we can merge and log follow up issues for Twenty Twenty. But can you investigate to confirm that the approach is robust, i.e. confirm that it's not a fluke that Storefront is working well? Maybe test with some other quality Woo themes (e.g. Threads/Block Shop/Mathew from WCCOM marketplace, or other WPORG themes) as as first step.
Here's what I saw ..
When I click Buy now page, the notice is NOT shown (in Twenty Twenty). So this is similar issue to what I was reporting before.
Those notices stack up in the background, and then render when they get a chance. On shortcode pages, all the dups are rendered:
Also, in the checkout block, the notice is styled left-aligned:
| @@ -185,6 +185,10 @@ protected function hydrate_customer_payment_methods( AssetDataRegistry $data_reg | |||
| * @param AssetDataRegistry $data_registry Data registry instance. | |||
| */ | |||
| protected function hydrate_from_api( AssetDataRegistry $data_registry ) { | |||
| // Print existing notices now, otherwise they are caught by the Cart | |||
| // Controller and converted to exceptions. | |||
| wc_print_notices(); | |||
|
@Aljullu is this ready to be merged? |
|
@haszari, thanks for writing the testing steps, I could reproduce what you mean. Let me split my response in several parts: Notices not always appearing in Twenty Twenty.This seems to be the expected behavior. Even though Storefront displays WC notices on top of each page, that's not the case for other themes like Twenty Twenty. In them, notices are only shown when WC Core prints them. That happens in some occasions like when rendering the Cart and Checkout blocks, but not on every page load. I think that's the cause of the confusing behavior you were experimenting: you were triggering the error several times, but since you were visiting a regular page, notices were not being printed but instead were 'accumulated'. Once you visited the Checkout shortcode, all notices appeared at once. I'm not sure how this should affect blocks, I think there are three possibilities:
I'm leaning towards option 1 or 2. Thoughts? In any case, I would open an issue for that since I don't think that's blocking this PR. Should blocks use an action instead of directly calling
|
|
Excellent investigation and analysis @Aljullu ! It seems to me you've hit on an area that's inconsistent across themes, and it's not clear what the "correct" behaviour is, or what Woo expects. Also we should keep in mind that the issue that triggered this is somewhat of an edge case, so the behaviour differences we're seeing might actually be low-impact. That said, this seems like an opportunity to improve the robustness of Woo + themes 😄 (at some point - not an urgent issue IMO).
Aha! That does explain things. I wonder if we (woo core) should advocate for more themes to take the Storefront approach, or implement it in core as default behaviour.
I agree this should not block this PR, and it's probably a wider (woo platform) discussion, as this is really a woo core issue that we're hitting in blocks. I could live with any those options you mention – let's see what others think. Option 3 seems the least "intrusive" to me; all options seem like a patch the symptom rather than addressing the cause.
I agree - my instinct is that we should avoid adding hooks (or triggering existing hooks) until it's we know it's essential and needed. |
|
I'm going to merge this as-is since it has already been approved. We can follow-up with printing (or not) notices in more blocks and extensibility in the future. |
Fixes #2670.
Some changes included in this PR:
NoticeHandler::convert_notices_to_exceptions()there are not old notices.CheckoutErrortoCheckoutOrderErrorso it matches its directory name. 2278519Tagging @mikejolley for review since you worked on this and will probably know if this approach makes sense or there is a better way I missed.
Screenshots
How to test the changes in this Pull Request:
There are steps to reproduce in #2670, but I think they can be simplified to:
https://one.wordpress.test/checkout/?add-to-cart=19(replacingone.wordpress.testwith your test site URL and19with the ID of the product you modified).Changelog