-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Fixes 422 error due to bad sql building #12704
Fixes 422 error due to bad sql building #12704
Conversation
Hello @rioug , I uploaded a bug here in the I have working on the elsif sorting_with_supplier_properties?(supplier_properties)
order_by_property = distributor.producer_properties
.map { |pp| "producer_properties.property_id=#{pp.property_id} DESC" }
.join(", ")
"#{order_by_property}, spree_products.name ASC, spree_products.id ASC" def sorting_with_supplier_properties?(supplier_properties)
supplier_properties && distributor.producer_properties.present?
end I am not sure whether my use of What do you think ? |
If I am following correctly what you are saying, I think you are trying to something we don't need. There is no need to order by I think what we need to do here, it's to get rid of the Good job on cleaning up |
As a first step you should try to add the missing spec in here :
We need the scenario where preferred_shopfront_taxon_order is set on the distributor and we are filtering by producer properties.
|
- first part of query use supplier_properties parameter, but not second part, that can leads to mismatch between the 2 parts. Remove supplier_properties parameter + modify SQL to get it right. - spec tests category filtering & sorting + producer properties
91f804f
to
05ed463
Compare
Hello @rioug , Thanks for the tip. Checks have failed, but I cannot reproduce one and the 2nd fail is a redis issue ... |
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.
Awesome, thanks for fixing this @cyrillefr 🙏 ! I had a feeling it wasn't going to be this simple but I am glad to be proven wrong 😄
Thanks so much @cyrillefr , I could verify the bug is fixed indeed in staging. Before this PR, the bug is reproducible: After this PR the bug is gone, and the correct results are returned: Merging! |
What? Why?
When some properties are defined at the enterprise level, there is a 422 return code as well as a "Products loading ..." displayed when customer filters products.
Reason is a bug in SQL Query building.
First part of query use
supplier_properties
parameter, but not second part, that can leads to mismatch between the 2 parts:openfoodnetwork/app/services/order_cycles/distributed_products_service.rb
Lines 37 to 40 in dfea0cd
The 2nd bit:
openfoodnetwork/app/services/order_cycles/distributed_products_service.rb
Lines 81 to 83 in dfea0cd
What should we test?
See § "Steps to Reproduce" from #12656
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates