Fix merging conditions for *_througn with STI case #12659

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
Contributor

iantropov commented Oct 27, 2013

Fix #12474, by preventing merging of conditions of target_scope for *_through case with STI

@pftg pftg and 2 others commented on an outdated diff Oct 27, 2013

activerecord/test/models/place.rb
@@ -0,0 +1,17 @@
+class Place < ActiveRecord::Base
@pftg

pftg Oct 27, 2013

Contributor

Are there any ways to use existed models, instead of adding new?

@iantropov

iantropov Oct 27, 2013

Contributor

No, I've tried this, but can't find appropriate chain of models.

@senny

senny Nov 7, 2013

Owner

can you extend existing models slightly to get the needed chain? Adding all those models to reproduce feels way over the top.

@iantropov

iantropov Nov 7, 2013

Contributor

Ok, I will try to find appropriate existed variants again, but matter is that, we need in STI chain with proper associations.

Contributor

iantropov commented Nov 2, 2013

@senny senny and 1 other commented on an outdated diff Nov 7, 2013

...es/associations/has_many_through_associations_test.rb
@@ -1085,4 +1086,12 @@ def test_has_many_through_obeys_order_on_through_association
readers(:michael_authorless).update(first_post_id: 1)
assert_equal [posts(:thinking)], person.reload.first_posts
end
+
+ test "has many through with STI" do
+ land = Land.create! name: "USA"
+ city = land.cities.create! name: "Seattle"
+ district = city.districts.create! name: "Capitol Hill"
+
+ assert_equal Land.first.cities.map(&:districts).flatten, Land.first.districts
@senny

senny Nov 7, 2013

Owner

This assertion is very unclear to me, what is it actually verifying? Can we hardcode the expected values?

@iantropov

iantropov Nov 7, 2013

Contributor

It verifies that districts from Land via cities are same as districts from Land.

Contributor

iantropov commented Nov 9, 2013

@senny Please take a look, I've got rid of new models and clarify assertions in the test

Owner

senny commented Nov 19, 2013

@pftg pftg commented on an outdated diff Nov 19, 2013

...es/associations/has_many_through_associations_test.rb
@@ -1085,4 +1086,12 @@ def test_has_many_through_obeys_order_on_through_association
readers(:michael_authorless).update(first_post_id: 1)
assert_equal [posts(:thinking)], person.reload.first_posts
end
+
+ test "has many through with STI" do
@pftg

pftg Nov 19, 2013

Contributor

Some style problems, IMHO:

I'll review your patch tomorrow

Contributor

pftg commented Nov 20, 2013

@iantropov I found failed tests for your PR: https://gist.github.com/pftg/7569547

Contributor

iantropov commented Nov 21, 2013

@pftg It is strange - I don't have these problems.
Please retest, and be sure, that you've done bundle install and run tests with bundle exec

Contributor

pftg commented Nov 21, 2013

@iantropov tests are passed for master, I do not see any connection with bundle. Will check your last updates

@pftg pftg commented on the diff Nov 21, 2013

activerecord/CHANGELOG.md
@@ -1,3 +1,9 @@
+* Prevent merging target_scope conditions for *_through with STI case
@pftg

pftg Nov 21, 2013

Contributor

Will be great to see the problem which you resolved, not just solution.

@iantropov

iantropov Nov 21, 2013

Contributor

You can see it in test case and in its associations

@sgrif

sgrif Feb 4, 2015

Member

People who read the change log do will not be reading the test code. It is intended for users or casual contributers, not the core team. You need to list why a change was made in the changelog for something to be merged.

Contributor

pftg commented Nov 21, 2013

Cool, tests are passed!

Contributor

iantropov commented Nov 21, 2013

@pftg I've only rebased changes...

Contributor

grosser commented Jan 28, 2014

👍 also ran into this

rafaelfranca was assigned Jan 29, 2014

@rafaelfranca rafaelfranca modified the milestone: 4.0.5, 4.0.4, 4.0.6 Mar 10, 2014

This looks like it works; did you submit a pull request for this one?

Owner

iantropov replied Apr 8, 2014

Yeah, I've already submitted PR for this - rails#12659

Awesome. I've just been trolling the issues looking for somewhere to start contributing. :)

This patch solves the issue perfectly in 4.2.0 but it has gone out of sync with the line numbering @iantropov is there anything that we can do to help with refreshing that pull request?

Contributor

moktin commented Apr 9, 2014

Great 👍, I am having the same issue and this pr fixes it.

@dmitry dmitry commented on the diff Jul 12, 2014

activerecord/test/schema/schema.rb
@@ -659,6 +659,9 @@ def create_table(*args, &block)
create_table :tasks, force: true do |t|
t.datetime :starting
t.datetime :ending
+ t.string :name
@dmitry

dmitry Jul 12, 2014

Contributor

Isn't it possible to reuse some of the existing tables without adding additional fields?

@rafaelfranca rafaelfranca modified the milestone: 4.0.10, 4.1.7 Aug 21, 2014

Contributor

eval commented Sep 26, 2014

Mergeconflicts :( @iantropov could you do a rebase?

@rafaelfranca rafaelfranca modified the milestone: 4.1.9, 4.0.13 Nov 19, 2014

@rafaelfranca rafaelfranca modified the milestone: 4.0.13, 4.1.9 Jan 2, 2015

Contributor

jnraine commented Feb 4, 2015

@iantropov this fixes an issue I'm experiencing. I'd love to see it merged.

I've forked it and resolved the merge conflicts with master: https://github.com/jnraine/rails/tree/activerecord_issue_12474

Thanks for your work on this. Let me know if there is anything I can do to help get this merged as quickly as possible.

@sgrif sgrif commented on the diff Feb 4, 2015

...lib/active_record/associations/through_association.rb
@@ -17,7 +17,7 @@ def target_scope
scope.merge!(
reflection.klass.all.
except(:select, :create_with, :includes, :preload, :joins, :eager_load)
- )
+ ) if reflection.table_name != scope.table.name
@sgrif

sgrif Feb 4, 2015

Member

Avoiding inverse conditionals in favor of the more explicit syntax would be preferable.

Member

sgrif commented Feb 4, 2015

@jnraine Can you fix the points of feedback I've left here, open a new PR, and ping me on it?

Member

sgrif commented Feb 4, 2015

Closing in favor of #18805 since this appears to be quite stale.

sgrif closed this Feb 4, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment