-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 admin SQL issues with DISTINCT products #2025
Fix admin SQL issues with DISTINCT products #2025
Conversation
it "should be able to search and sort by price" do | ||
product = create(:product, name: 'apache baseball cap', sku: "A001") | ||
variant = create(:variant, product: product, sku: "A002") | ||
other_product = create(:product, name: 'zomg shirt', sku: "Z001") |
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.
Useless assignment to variable - other_product.
# Regression test for https://github.com/solidusio/solidus/issues/2016 | ||
it "should be able to search and sort by price" do | ||
product = create(:product, name: 'apache baseball cap', sku: "A001") | ||
variant = create(:variant, product: product, sku: "A002") |
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.
Useless assignment to variable - variant.
218a486
to
73fa313
Compare
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'm not sure about entirely removing the previous scope. It's common to import partials from core into a host apps code base, and I think deprecating the scope would be more cautious.
The actual fix looks great!
Woow, that's so elegant! and builds on top of the refactor in #1904 by @spaghetticode setting an excellent example for any such scopes. I've actually had to deal with this one just recently to handle searching through thousands of products, and I didn't know about the exists way .. will definitely use it next time around! Maybe it's a good time to ping on #1660. There was an excellent discussion there, and in my view we, the benefits of lookups and search working on large stores far outweight the usability negatives of not being able to do partial matching. And by large stores I don't even mean ours (4.5mil variants, 3mil) - I'm sure that even with a few thousand products, each with 8-10 variants .. the lookups become hardly usable. So if you do make a decision for better support of large stores, then we can add PRs similar to #1660 for variant SKU and others as well as conditionally add Postgres or MySQL specific indices. |
MySQL 5.7+ previously errored when trying to sort products by price. ORDER BY clause is not in SELECT list, references column 'spree_prices.amount' which is not in SELECT list; this is incompatible with DISTINCT Additionally, under PostgreSQL, searching for an SKU which matches multiple variants of the same product and then sorting by price, multiple of the same Product would be returned in the search. This commit removes the Product.distinct_by_product_ids hack replaces the join search on products->variants with a WHERE EXISTS subquery.
73fa313
to
7defeac
Compare
@mamhoff I have re-added the scope and deprecated it instead. |
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.
Thank you John!
Fixes #2016
MySQL 5.7+ previously errored when trying to sort products by price.
Additionally, under PostgreSQL, searching for an SKU which matches multiple variants of the same product and then sorting by price, multiple of the same Product would be returned in the search.
This commit removes the Product.distinct_by_product_ids hack replaces the join search on products->variants with a WHERE EXISTS subquery.