Add option to match belong_to with touch #222

Closed
wants to merge 2 commits into
from

Projects

None yet

3 participants

Contributor
panupan commented Jan 17, 2013

This option has been missing but is pretty much the same thing as matching the validate option. It could probably be refactored to prevent code duplication.

Owner

@panupan Thanks for the pull!

I like this idea. The only thing that I do not like is the fact that if I have a model where I have specified touch and I am not currently specifying that in my tests, this will cause my matcher to fail. If I haven't specified it, I don't think it should be tested.

I do see that it is handled this way in the validate option and I don't agree with that.

Can you remove that please and we'll review from that point.

Member
mxie commented Apr 4, 2013

Just wanted to check in and see if you had any updates on this. Please also rebase once you do since there have been many changes to master since this was last reviewed. Thanks!

@panupan panupan Add touch option to belongs_to matcher
Also fixes validate failing when specified on model but not on matcher.
6ad7547
Contributor
panupan commented Apr 10, 2013

@drapergeek Hello, I fixed touch and validate to work how you suggested, I also added some tests to verify it.

@mxie It should be up-to-date and good to go now.

@mxie mxie and 1 other commented on an outdated diff Apr 12, 2013
...shoulda/matchers/active_record/association_matcher.rb
@@ -258,10 +266,19 @@ def join_table_exists?
end
def validate_correct?
- if !@validate && !reflection.options[:validate] || @validate == reflection.options[:validate]
+ if !@options.key?(:validate) || @options[:validate] == (reflection.options[:validate] == true ? true : false)
mxie
mxie Apr 12, 2013 Member

The !@options.key?(:validate) change is good, but instead of:

@options[:validate] == (reflection.options[:validate] == true ? true : false)

I think you can just do:

@options[:validate] == reflection.options[:validate]
panupan
panupan Apr 12, 2013 Contributor

Hi, that was breaking some tests. Because Rails defaults :validate to false so reflection.options[:validate] could be nil.

panupan
panupan Apr 12, 2013 Contributor

Another option is:

@options[:validate] == !!reflection.options[:validate]

mxie
mxie Apr 12, 2013 Member

@options[:validate] == !!reflection.options[:validate] is better, but perhaps we should add back in the check for reflection.options[:validate]?

panupan
panupan Apr 15, 2013 Contributor

@mixie I'm not quite sure what adding that back in will work. It was causing the behavior drapergeek was describing above, where it would cause the matcher would fail if validate was specified in the model but not the matcher. BTW, I amended my last commit to use !!.

@mxie mxie commented on an outdated diff Apr 12, 2013
...shoulda/matchers/active_record/association_matcher.rb
@@ -258,10 +266,19 @@ def join_table_exists?
end
def validate_correct?
- if !@validate && !reflection.options[:validate] || @validate == reflection.options[:validate]
+ if !@options.key?(:validate) || @options[:validate] == (reflection.options[:validate] == true ? true : false)
+ true
+ else
+ @missing = "#{@name} should have :validate => #{@options[:validate]}"
+ false
+ end
+ end
+
+ def touch_correct?
+ if !@options.key?(:touch) || @options[:touch] == (reflection.options[:touch] == true ? true : false)
mxie
mxie Apr 12, 2013 Member

Same comment as above for the validate option.

@mxie mxie and 1 other commented on an outdated diff Apr 12, 2013
...da/matchers/active_record/association_matcher_spec.rb
@@ -121,6 +126,51 @@
end
end
+ context 'an association with a :touch option' do
+ [false, true].each do |touch_value|
+ context "when the model has :touch => #{touch_value}" do
+ it 'accepts a matching touch option' do
+ belonging_to_parent(:touch => touch_value).should
mxie
mxie Apr 12, 2013 Member

Please move the should to the next line. Otherwise, this will always pass. Same for the other shoulds and should_nots in the remainder of the file if they appear at the end of the line.

panupan
panupan Apr 12, 2013 Contributor

Yikes! Not sure how it happened.. thought I just copied and pasted. Should be fixed now. Good catch.

@mxie mxie commented on an outdated diff Apr 12, 2013
...da/matchers/active_record/association_matcher_spec.rb
+ belong_to(:parent).touch(!touch_value)
+ end
+
+ it 'defaults to touch(true)' do
+ if touch_value
+ belonging_to_parent(:touch => touch_value).should
+ belong_to(:parent).touch
+ else
+ belonging_to_parent(:touch => touch_value).should_not
+ belong_to(:parent).touch
+ end
+ end
+
+ it 'will not break matcher when touch option is unspecified' do
+ belonging_to_parent(:touch => touch_value).should
+ belong_to(:parent)
mxie
mxie Apr 12, 2013 Member

It's ok to just join this with the line above.

Contributor
panupan commented Apr 25, 2013

Hello, can someone review this again? It should be good to go.

Owner

Thanks for this @panupan.

🚀

Also I really appreciate you fixing up the validate option as well. Good job!

@drapergeek drapergeek closed this Apr 26, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment