-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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
StoreAPI: Query parameters via Collection Data endpoint are no longer working #42157
Comments
@nerrad Do we have news about this issue? |
cc @dinhtungdu is this something you could look into? |
I can confirm this. I checked the |
@nerrad I don't know about the context/reason for the decision made in woocommerce/woocommerce-blocks#8599, but looks like we removed by incident. IMO, we now have some options:
Why reverting instead of fixing? Because to me, the current approach introduced in #42466 is fundamentally different from the former one.
|
Thanks for your input @dinhtungdu , regarding:
No, not really: as you can see in the following discussion https://github.com/woocommerce/woocommerce-blocks/discussions/9023 , by unanimous consensus, we have decided to adopt the implementation path that is aligned with the following rule:
This removal was a deliberate change to prevent a critical problem: previously, we had an SQL query on the client side to fetch the IDs of all displayed products that was later on used as the value of a param to fetch the filter counts from store API. As you can imagine, this is highly inefficient: ideally, the API should be returning those results to us with all DB requests happening on the backend rather than on the client side. In fact, we have received complaints from external users regarding this client-side query in particular given the huge bottleneck it introduced in our codebase. More details are available on: woocommerce/woocommerce-blocks#10198 The reported issue has been documented on pdnLyh-3VR-p2 With that said, we are on the same page when it comes to re-enabling this feature, but I strongly disagree that this should be made by reverting all the changes made in the context of the Filter Data count mismatch project: not only because this way we would re-introduce a number of bugs that were effectively solved by that implementation, but also because by re-introducing those queries on the client side we would be, in addition to reintroducing a bug, also negatively impact performance for all stores. My recommendation is for us to update the parameters received by store API so it is aware of what products are being actively displayed on the frontend and use this information to update the relevant SQL queries and the returned results from the backend, rather than this hybrid approach where the displayed product IDs are fetched on the client side via a highly inefficient SQL query that has already been proven to cause problems. In essence, the backend would do what it is supposed to do, with an API that returns the results we need, given extra params provided. |
@nefeline thanks for the context and links.
I agree that we should remove the code grabbing all product IDs as you did in woocommerce/woocommerce-blocks#10198. But I don't see it should lead to the removal of query recreation using
If we have the performance issue with the former |
Personally, I don't see the method needing to change, but the data passed to the endpoint and the way That's why I used the nearly identical former method for the new filter blocks, the fundamental change is I passed the whole query arguments, so I can ensure I recreate the same query being displayed on the frontend, resulting in the correct filter count data for any query from simple to complex. @samueljseay we should profile the new blocks with thousands or even hundreds of thousands of products to see how they keep up with large amounts of data. |
@dinhtungdu regarding:
Do you mind clarifying what you mean here? What I'm advocating for is to rely exclusively on
Just to be clear, the performance issue was solved with the removal of the queries to fetch the product IDs from
Sounds like we are sharing the same point of view here :). This is what I meant with my initial recommendation:
The only point that sounds like we are still not on the same page is this one:
I don't think |
I meant this query recreation woocommerce/plugins/woocommerce/src/Internal/ProductQueryFilters/FilterDataProvider.php Lines 158 to 169 in 76e6d82
IMO, recreating the query is cheap because we don't execute the query, we recreate the query to get the SQL request, and then we feed that SQL request as the WHERE condition for the single count query. There aren't any additional SQL queries to be made. If the former
As I mentioned before, I agree with this. I also mentioned that we could solve this differently by passing additional parameters to StoreAPI, like
I was also talking about the I think we can still use the former method to benefit from context-aware ability (like taking category into account like OP uses) without worrying about the performance as the reason I explained above. The only concern I have with the former method is this line: woocommerce/plugins/woocommerce/src/Internal/ProductQueryFilters/FilterDataProvider.php Line 178 in 76e6d82
But does it hurt the performance, I'm not sure. So I went ahead to some (very basic) profiling. My setup:
From the results, I can see the former method performs better than the current one consistently. I don't have much experience in profiling MySQL performance, so I'm open to learning any better profiling methods. |
I tried another test with min price:
|
There was a mistake in my test branch that misses the last array map call of
|
FYI, I realized I misunderstood some of Patricia's comments, so I edited my response #42157 (comment), please give it another look. |
Hey @dinhtungdu, thanks. I have a few questions here:
I'm happy to help open a PR specifically to solve the problem described on this issue without going through this major refactor & risking re-introducing regressions for the filters. |
@nefeline thanks. Regarding your questions:
I don't think so. OP uses Store API directly to get the filter data, not through a filter block, so woocommerce/woocommerce-blocks#10198 isn't related.
We have already fixed data mismatch issues for the new filter blocks in #42811 (pdnLyh-530-p2). We didn't change the main logic of those count methods, but we changed the data passed to them. We won't fix the data mismatch issue for the current filter blocks, because:
The previous method used by the current attribute filter block doesn't return correct counts, but the new attribute filter block using a similar method does. In addition, in my test, the current ( I used this branch for testing, but As I mentioned in the P2:
I want to refactor the StoreAPI to use the new class (now being refactored to a service provider in #43772), I made a draft PR to demonstrate how it looks here. So we can consider another option, which is waiting for the new service provider to be merged. |
Unfortunately it looks like you accidentally removed an important piece of information from my previous response here, so I'm posting it here again to make sure we are on the same page: (although not recommended, the plan to pass over different params to the API for narrowing down the products at that stage of execution is a way better approach)
Sounds like a lot of work has already been made without the involvement of folks who worked on the attribute counts early in the process (@roykho and myself), that's unfortunate as we missed an invaluable opportunity to collaborate early. Regarding your video: On Minute 1:57 you are showing the product "Sleek Silk Plate", which is a variable product: Does this product have two variations? Do both have the "At" attribute? If the answer to both questions is yes, then the bug is on your proposed implementation, not the current one: all counts were tailor-made to account for product variations as well as parent products, that was one of the key changes made as part of the Filter Data Count attribute project. On Minute 4:40 you enable a combination of filters of products with the attribute equal to "Atque" + a price range: can you please check if there are really two products with the "Atque" attribute AND with a price between the range of 559 and 587? I strongly recommend you carefully read the details of the implementation on https://github.com/woocommerce/woocommerce-blocks/discussions/9023 as it explains how each one of the decisions were made and why. This also includes the explanation why you are seeing the "zeros" with certain filter combinations: this was all by design, not a regression or a bug. |
@nefeline I didn't miss that. I agree with your comment about passing over additional parameters. I was saying that we could solve woocommerce/woocommerce-blocks#7245 without modifying the original So, we're still not on the same page. I wonder how can we move this forward. Maybe a call could be a better way to clear up the confusion, what do you think?
I'm confused here. As a shopper, when I click on an attribute with 2 as the count, I'd expect to see 2 products. So, are you saying, it's expected to have the count is different from the actual number of filtered products? cc @jarekmorawski @nerrad can you comment on this?
Yes, I can confirm, I also clicked the
This is another point that makes me confused. In my test, I still got more filtered products if I clicked on those 'zero' attribute filters. |
I'm sorry, you didn't answer my previous question so I'll paste it here:
If the user is looking for blue products and a given product has 3 different blue variations, they are all unique/individual blue products, that the user can purchase, aren't they? We have discussed this with design prior to implementing the change, but I'll let @jarekmorawski chime in here for more context.
Sorry, I was probably not very clear: can you please open these two products individually, open their individual variations, and then share the screenshots of their prices and attributes here? what I'm aiming for is a comparison between the product variations, not the parent products.
This also has been discussed with design at the time of implementation, so I'll again defer to @jarekmorawski here. For reference, I'm copy-pasting the context from the original discussion: Filter by attribute, rating, and priceExample:
Gotcha: based on our conversation so far, everything indicates that there's a misunderstanding of how filter counts should work and what is a bug. All the changes made in the context of the Filter data count mismatch project were reviewed and approved by the design team and the leadership: you didn't share the details for each one of the individual product variations yet, but if my suspicions are accurate, what you are seeing is correct: we have made this decision very clear with the following statement: The filtered counts always match the searched results, independent of the combination of filters. They accurately represent what is available in the store for the given criteria.
I agree, especially because there seems to be a fundamental disagreement with what is and what isn't the expected functionality here. |
@nefeline I don't want to sidetrack too much but I just wanted to zero in on this, and forgive me I might be misunderstanding some nuance but its just how I see it at first glance.
I think this is confusing for variations? I can't think of a store where I wanted to purchase a blue t-shirt for example and there is one blue tshirt with variations for small/medium/large would I really want to see the count including those variations? not to mention when the product list is filtered in the UI its showing me one tshirt.
Sure, variations are purchaseable, but users don't care about that, variations are somewhat hidden from them because we don't show them every tshirt size for blue tshirt in the product collection UI. I can understand a lot of thought has gone into all of this, I read through a lot of the previous issues and such, but I think its ok, if we challenge some of this thinking again to make sure we're on the right track about what expected behaviour should be, because every time I encountered this behaviour I felt its not correct for shopper UI personally. |
@nefeline I'm sorry I missed this. The Sleek Silk Plate product is a variable product, It has 3 variation base on only one attribute which is Color. There is only one variation has
Here are screenshots I took for the filtered products of this combination. Both products have multiple variations, and both have more than two variations that have the
However, I don't see any variation with |
Just to quickly chime in regarding the UX issue, I also agree with @samueljseay that counting each variation as a separate product in the UX is confusing and potentially misleading to the users. The fact that they are separate purchasable entities (and I've dealt with some WooCommerce integrations which require to mark them as separate products, to be honest: so, speaking strictly technically, there is a case to be made for this), I think from both the user and merchant perspective, it makes sense to consider them as one product. Think more about “models” rather than purchasable entities. I also have tried to follow all the links, but I didn't manage to find where this decision was made. I might be missing it, but, to me, it seems that actually @jarekmorawski was, if anything, basically supporting the opposite of this: per0F9-Rh-p2#comment-899 in a related issue. But then, we proceeded with a different solution in the interest of time, more conversation pending. |
@sunyatasattva @dinhtungdu @samueljseay how about we move your concerns to https://github.com/woocommerce/woocommerce-blocks/discussions/9023 so we can re-open the discussion with the design team and the leadership? WDYT? |
@nefeline I just want to chime in and say that this API is not only used in the block editor, but also used as an independent API in decoupled WooCommerce sites. Removing the product query from the count broke the contract of the API and its documentation and should be fixed as soon as possible. |
@creative-andrew thanks for chiming in! We are on the same page here: this ended up being a bigger discussion as other aspects of the filter counts are being questioned that are not at all related to this issue in particular. Fixing the bug as described on this ticket with the current implementation is fairly simple, I'm glad to open a PR with the solution if we verify that the discussion that is now happening on https://github.com/woocommerce/woocommerce-blocks/discussions/9023 will take longer to conclude and/or if we the outcome of that conversation is that the design decision made for the current approach is the one we wanna keep. |
@creative-andrew @fabianmarz just a heads up that this issue was fixed in #45247, and it will be available in WooCommerce 8.9. Thank you for your patience with us while discussing this issue and sorry for the inconvenience for your stores! |
Thanks y'all for your hard work! |
Thank you guys for the work and the heads up! 💪 |
Hey there!
when updating to the latest WooCommerce version my StoreAPI request data doesn't seem to return the data it used to return in WC 7.6.
I'm using the collection-data API to get a pre-filtered result set based on a given categories in a custom ACF block which shows products on any page it is placed on. So the products are not part of the main query.
It doesn't seem to work properly anymore since the this PR seem to dropped support for
ProductQuery()
which will no longer take the category query parameter into account.Here's a screenshot of my
WP_REST_Request
instance but the request returns all attributes instead of the ones filtered for the given category termsI noticed the Collection API docs still state support for all the parameters
Is this a bug or am I missing something here? Any help would be appreciated here Thank you! 🙌
The text was updated successfully, but these errors were encountered: