Fix counter cache column name if :inverse_of is specified on belongs_to #4631

Closed
wants to merge 2 commits into
from

Projects

None yet

6 participants

Contributor
jm commented Jan 23, 2012

This gist explains what problem I'm fixing: https://gist.github.com/1665503

This patch adds a test and a small amount of code to fix the issue. Sorry for the stupid merging master commit. I can't seem to figure out how to get it to behave and don't have time to wrestle with git to fix it. :)

Member

This seems good to me but I am a little concerned about breaking existing code where people are relying on the current logic. @tenderlove wdyt?

Owner

Seems fine for master. I don't think we should backport to 3-2-stable though.

Member

Alrighty. @jm could you add something to the CHANGELOG please?

Contributor
jm commented Feb 19, 2012

Crap I didn't even see these comments until right now. I'll add something. Sorry about that.

@jm hey, would you mind adding that Changelog entry, so we can get this merged in? Thanks!

@carlosantoniodasilva carlosantoniodasilva commented on the diff May 1, 2012
...st/cases/associations/belongs_to_associations_test.rb
@@ -391,6 +391,20 @@ def test_custom_counter_cache
reply[:replies_count] = 17
assert_equal 17, reply.replies.size
end
+
+ def test_counter_cache_with_class_name
carlosantoniodasilva
carlosantoniodasilva May 1, 2012 Owner

I think the test could be test_counter_cache_with_inverse_of, wdyt?

jm
jm May 1, 2012 Contributor

Agreed. That's a crappily named test. :)

Member

@jm any chance you could update that test name and add a CHANGELOG entry so we can merge this?

Member

Oh, and removing the merge commit would be good too.

civrot commented Nov 16, 2012

Feeling a little out place, but trying to help out the community at the same time. I came across this PR from http://issuetriage.herokuapp.com/, and just adding a comment to bring it up again.

@jm have you lost the energy behind this change? Should the PR be closed?

Member

Yes, with two months and no answer, I'm giving it a close. @jm, you know how to re-open a new pull request if you think this is still a needed change.

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