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

Preload using with_translations #92

Merged
merged 5 commits into from
Apr 6, 2022

Conversation

npflood
Copy link
Contributor

@npflood npflood commented Aug 27, 2021

Fixes #90

Navigating to a taxon view in spree_frontend (any URL like /t/taxon_name) where products should be returned raised PG::GroupingError: ERROR: column "spree_product_translations.id" must appear in the GROUP BY clause or be used in an aggregate function

Removing the .includes(:translations).references(:translations) from the spree_base_scopes method in SpreeGlobalize::Translatable allows the query to succeed, but causes n+1 queries.

Adding .with_translations to the spree_base_scopes method in SpreeGlobalize::Translatable allows the query to succeed, without n+1 queries.

Testing to see what is causing issues with the latest version of spree's product searcher in the taxon controller.
* Use the Globalize with_translations scope to preload translations

SpreeGlobalize::Translatable now uses the `with_translations` class method to set `spree_base_scopes` instead of the previous `.includes(:translations).references(:translations)`

This fixes an issue where Spree v6.3.0 built queries that could not be executed due to the additional tables that `.includes(:translations).references(:translations)` creates while still avoiding n+1 queries.

* Changes to Gemfile and Test Suite to support Spree v6.3.0
Reflect Github's change to main from master branch.
Reflect Github's change from master to main
@npflood
Copy link
Contributor Author

npflood commented Oct 7, 2021

This fixes an issue with a currently released version of Spree. Can someone review the changes and either merge this branch or indicate that the underlying issue has been resolved in another way?

@ajahongir
Copy link

is this going to be accept?

@mrbrdo
Copy link

mrbrdo commented Mar 23, 2022

I can also confirm this bug and the fix works, should be merged...

@damianlegawiec damianlegawiec merged commit 1c6eb9e into spree-contrib:master Apr 6, 2022
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.

SpreeGlobalize::Translatable preloading translations causes queries to fail in Spree 4.3.0.rc2
4 participants