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

fix property filters to work with Postgres #11708

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mrbrdo
Copy link
Contributor

@mrbrdo mrbrdo commented May 26, 2022

This should be DB-agnostic and also works with Postgres now.

@viezly
Copy link

viezly bot commented May 26, 2022

Changes preview:

Legend:

👀 Review pull request on Viezly

@mrbrdo
Copy link
Contributor Author

mrbrdo commented Oct 25, 2022

@rafalcymerys

@rafalcymerys
Copy link
Member

Hi @mrbrdo - what was the problem with postgres here? I have my intuition, but would like to confirm if I'm reading it correctly ;)

@mrbrdo
Copy link
Contributor Author

mrbrdo commented Oct 28, 2022

@rafalcymerys I think the problem was that the scope coming in has an order, and postgres needs you to order first by the columns you are selecting DISTINCT. therefore the query failed as the ORDER BY is by something other than what we are pluck-ing.
In any case it surely had to do with the ORDER BY part which Postgres didn't like in some combination of incoming scope. Either due to distinct query or group by or something like that.

@rafalcymerys
Copy link
Member

That makes sense. Was that an issue coming out of your customization (where the scope coming in had the order set) or an issue in core spree?

@mrbrdo
Copy link
Contributor Author

mrbrdo commented Nov 3, 2022

@rafalcymerys hard to remember after 6 months. I don't think I did any changes to properties, so I'd guess it was related to spree core. I have some vague recollection that in one case the scope coming in depended on selected filter/sort value, and only under a certain condition would it cause issues, I think it was when doing a search (Ransack). If I was providing said scope I would fix that code rather than spree core which I try to avoid when possible.

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.

None yet

2 participants