Skip to content
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

Added select on product's scopes for price sorting #3620

Merged

Conversation

thomasrossetto
Copy link
Contributor

Description
You can find all details here

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @thomasrossetto, thanks for your contribution! 👋

I left a comment and I think it makes sense to also add a spec for this scope here, taking advantage of this change. I'm just afraid that this Postgres fix could break the same query on MySQL (even if I don't think it will happen). With a spec, the CI will take care of run it with both databases.

core/app/models/spree/product/scopes.rb Outdated Show resolved Hide resolved
core/spec/lib/search/base_spec.rb Outdated Show resolved Hide resolved
core/spec/lib/search/base_spec.rb Outdated Show resolved Hide resolved
core/spec/lib/search/base_spec.rb Outdated Show resolved Hide resolved
core/spec/lib/search/base_spec.rb Outdated Show resolved Hide resolved
core/spec/lib/search/base_spec.rb Outdated Show resolved Hide resolved
core/spec/lib/search/base_spec.rb Outdated Show resolved Hide resolved
core/spec/lib/search/base_spec.rb Outdated Show resolved Hide resolved
core/spec/lib/search/base_spec.rb Outdated Show resolved Hide resolved
core/spec/lib/search/base_spec.rb Outdated Show resolved Hide resolved
core/spec/lib/search/base_spec.rb Outdated Show resolved Hide resolved
@thomasrossetto
Copy link
Contributor Author

@kennyadsl Added test + commented intentionally the right logic. Now you can see postgresql and (maybe) mysql problems.

@thomasrossetto thomasrossetto force-pushed the fix_product_sort_with_postgresql branch from 638f8f9 to 0c377f6 Compare May 18, 2020 07:38
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me, thanks!

Copy link
Member

@aldesantis aldesantis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thomasrossetto thanks, just a couple of comments on the spec description format!

core/spec/lib/search/base_spec.rb Outdated Show resolved Hide resolved
core/spec/lib/search/base_spec.rb Outdated Show resolved Hide resolved
@thomasrossetto thomasrossetto force-pushed the fix_product_sort_with_postgresql branch from 0c377f6 to 2d3b3cc Compare May 22, 2020 13:47
@kennyadsl kennyadsl merged commit 064f6e3 into solidusio:master May 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants