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

fixes #3603: replaced a relation with a collection in 'where' conditions #1021

Closed
wants to merge 1 commit into from

Conversation

dmitri-d
Copy link
Member

No description provided.

@domcleal
Copy link
Contributor

Could you add some tests please?

@@ -14,7 +14,7 @@ def index
values = Puppetclass.search_for ""
end
@puppetclasses = values.paginate(:page => params[:page])
@host_counter = Host.group(:puppetclass_id).joins(:puppetclasses).where(:puppetclasses => {:id => @puppetclasses.pluck(:id)}).count
@host_counter = Host.group(:puppetclass_id).joins(:puppetclasses).where(:puppetclasses => {:id => @puppetclasses.collect(&:id)}).count
Copy link
Member

Choose a reason for hiding this comment

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

IMHO, if we are changing it already, we might as well change it to an INNER SELECT style which rails 3.2+ supports.

this was done prior to that.

Copy link
Member Author

Choose a reason for hiding this comment

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

no point, as @puppetclasses is used in the view.

Copy link
Member

Choose a reason for hiding this comment

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

not sure i follow, afair, you can just change

where(:puppetclasses => @puppetclasses)

Copy link
Member Author

Choose a reason for hiding this comment

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

Nods, I suppose that is more concise. I'll see if sub select works with this syntax (the original issue was AR stripping all joins from the relation).

@dmitri-d
Copy link
Member Author

@domcleal: now with tests.
@ohadlevy: same issue as before -- it looks like subqueries get stripped of joins (probably all parts but "where" predicates get stripped)

@domcleal
Copy link
Contributor

Merged as 9e08f30 for Foreman 1.3.2, thanks @witlessbird.

@domcleal domcleal closed this Nov 14, 2013
@dmitri-d dmitri-d deleted the 3603 branch December 13, 2017 20:24
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.

3 participants