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

Polyamorous module #1113

Merged
merged 4 commits into from Apr 4, 2020

Conversation

varyonic
Copy link
Contributor

@varyonic varyonic commented Apr 3, 2020

Resumes #1105 Closes #1099 Fixes #1081 ?

.to match(%r{LEFT OUTER JOIN articles ON (\('default_scope' = 'default_scope'\) AND )?articles.person_id = people.id})
expect(real_query)
.to match(%r{LEFT OUTER JOIN articles articles_people ON (\('default_scope' = 'default_scope'\) AND )?articles_people.person_id = parents_people.id})
end
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez Apr 3, 2020

Choose a reason for hiding this comment

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

If I recall correctly, this is the test that was failing on Rails 6-0-stable. Are you saying that the new query that ransack builds is correct? My understanding was that the CI failure was a real failure that should be fixed.

Copy link
Member

@scarroll32 scarroll32 Apr 3, 2020

Choose a reason for hiding this comment

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

@deivid-rodriguez @varyonic this does seem to be passing the 6.0 CI, great work.

Copy link

@nikajukic nikajukic May 20, 2020

Choose a reason for hiding this comment

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

I've added an example of a failing test in #1119 (comment)

If you change the order of your conditions, the test starts to fail and generates invalid SQL.

@scarroll32 scarroll32 merged commit ab8c34d into activerecord-hackery:master Apr 4, 2020
1 check was pending
@varyonic varyonic deleted the polyamorous-module branch Jul 19, 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