Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixes #40 - Shoulda raises exceptions when one is using %{attribute} … #206

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

ahey commented Dec 20, 2012

...%{model} in the I18n activerecord validation messages.

@gabebw gabebw and 1 other commented on an outdated diff Dec 20, 2012

lib/shoulda/matchers/active_model/allow_value_matcher.rb
@@ -145,7 +145,9 @@ def default_attribute_message
default_error_message(
@options[:expected_message],
:model_name => model_name,
- :attribute => @attribute
+ :attribute_name => @attribute,
@gabebw

gabebw Dec 20, 2012

Contributor

It looks like :attribute_name is set to the attribute and :attribute is set to the attribute name, which looks backwards to me. Maybe I'm missing something?

@ahey

ahey Dec 20, 2012

Contributor

I'm trying to be consistent with ActiveModel https://github.com/rails/rails/blob/master/activemodel/lib/active_model/errors.rb#L431 which treats :attribute and :model as the humanized versions. Perhaps there is a better way to do this - any suggestions? We do ultimately need to pass :attribute and :model values containing humanized strings into the translate call.

@gabebw

gabebw Dec 20, 2012

Contributor

That does make sense - but if you change this, you should also change default_error_message in lib/shoulda/matchers/active_model/helpers.rb.

@ahey

ahey Dec 20, 2012

Contributor

I did update default_error_message - i'm not sure what else should be changed?

@gabebw

gabebw Dec 20, 2012

Contributor

Yes you did - my mistake. Sorry!

@gabebw gabebw and 1 other commented on an outdated diff Dec 20, 2012

...shoulda/active_model/ensure_length_of_matcher_spec.rb
+ end
+
+ it "should not raise an exception" do
+ expect {
+ @model.should ensure_length_of(:attr).is_at_most(4)
+ }.to_not raise_exception
+ end
+ end
+
+ context "when using %{attribute}, %{model} in the too_short translation" do
+ before do
+ stub_translation(
+ "activerecord.errors.messages.too_short",
+ "The %{attribute} of your %{model} is too short (minimum is %{count} characters)")
+
+ #too_long: "is too long (maximum is %{count} characters)"
@gabebw

gabebw Dec 20, 2012

Contributor

Please remove these comments.

@ahey

ahey Dec 20, 2012

Contributor

Oops - agreed.

@gabebw gabebw and 1 other commented on an outdated diff Dec 20, 2012

...shoulda/active_model/ensure_length_of_matcher_spec.rb
+ #too_short: "is too short (minimum is %{count} characters)"
+ #wrong_length: "is the wrong length (should be %{count} characters)"
+
+ @model = define_model(:example, :attr => :string) do
+ validates_length_of :attr, :minimum => 4
+ end.new
+ @model = Example.new
+ end
+
+ after do
+ stub_translation(
+ "activerecord.errors.messages.too_long",
+ "is too short (minimum is %{count} characters)")
+ end
+
+ it "should not raise an exception" do
@gabebw

gabebw Dec 20, 2012

Contributor

Why would this raise an exception? What use case is being tested here? Same for the test on line 210.

@gabebw

gabebw Dec 20, 2012

Contributor

I think a better test would be to call .matches? and then check the failure message, like this: https://github.com/thoughtbot/shoulda-matchers/blob/master/spec/shoulda/active_model/allow_value_matcher_spec.rb#L123

@ahey

ahey Dec 20, 2012

Contributor

It raises an I18n::MissingInterpolationArgument exception - it happens when we include %{attribute} or %{model} in the translation string. ActiveModel passes these values into the translate call via the options hash. This pull request is to include these values in the shoulda matchers to prevent the exception being raised. The tests do fail without the patched code.

@ahey

ahey Dec 20, 2012

Contributor

I'll look into refactoring the test, thanks.

@gabebw

gabebw Dec 20, 2012

Contributor

It raises an I18n::MissingInterpolationArgument exception

That makes sense, thanks for the comment. In that case, can you check for that exception specifically? Like this:

expect { @model.should ensure_length_of(:attr).is_at_least(4) }.to_not raise_exception(I18n::MissingInterpolationArgument) }

That way, it's clearer what you're expecting, and if another random error gets raised, it's not swallowed.

@ahey

ahey Dec 20, 2012

Contributor

Yes will do.

@gabebw gabebw commented on an outdated diff Dec 20, 2012

...shoulda/active_model/ensure_length_of_matcher_spec.rb
@@ -135,4 +135,82 @@
end
end
+ context "when using %{attribute}, %{model} in the too_long translation" do
+ before do
+ stub_translation(
+ "activerecord.errors.messages.too_long",
+ "The %{attribute} of your %{model} is too long (maximum is %{count} characters)")
+
+ @model = define_model(:example, :attr => :string) do
+ validates_length_of :attr, :maximum => 4
+ end.new
+ @model = Example.new
@gabebw

gabebw Dec 20, 2012

Contributor

This line isn't necessary.

@gabebw gabebw commented on an outdated diff Dec 20, 2012

...shoulda/active_model/ensure_length_of_matcher_spec.rb
+ expect {
+ @model.should ensure_length_of(:attr).is_at_least(4)
+ }.to_not raise_exception
+ end
+ end
+
+ context "when using %{attribute}, %{model} in the wrong_length translation" do
+ before do
+ stub_translation(
+ "activerecord.errors.messages.wrong_length",
+ "The %{attribute} of your %{model} is the wrong length (should be %{count} characters)")
+
+ @model = define_model(:example, :attr => :string) do
+ validates_length_of :attr, :is => 4
+ end.new
+ @model = Example.new
@gabebw

gabebw Dec 20, 2012

Contributor

This line isn't needed.

@gabebw gabebw commented on an outdated diff Dec 20, 2012

...lda/active_model/validate_presence_of_matcher_spec.rb
@@ -132,4 +132,26 @@
end
end
+ context "when using %{attribute} and %{model} in an i18 translation" do
+ before do
+ stub_translation(
+ "activerecord.errors.messages.blank",
+ "Please enter a %{attribute} for your %{model}")
+
+ define_model :example, :attr => :string do
@gabebw

gabebw Dec 20, 2012

Contributor

Can you please set @model on the same line you call define_model, like you do elsewhere?

@gabebw gabebw commented on an outdated diff Dec 20, 2012

...lda/active_model/validate_presence_of_matcher_spec.rb
@@ -132,4 +132,26 @@
end
end
+ context "when using %{attribute} and %{model} in an i18 translation" do
+ before do
+ stub_translation(
+ "activerecord.errors.messages.blank",
+ "Please enter a %{attribute} for your %{model}")
+
+ define_model :example, :attr => :string do
+ validates_presence_of :attr, :with => /abc/
@gabebw

gabebw Dec 20, 2012

Contributor

Is :with => /abc/ necessary here or anywhere in your pull?

@gabebw gabebw and 1 other commented on an outdated diff Dec 20, 2012

...shoulda/active_model/ensure_length_of_matcher_spec.rb
@@ -135,4 +135,82 @@
end
end
+ context "when using %{attribute}, %{model} in the too_long translation" do
+ before do
+ stub_translation(
+ "activerecord.errors.messages.too_long",
+ "The %{attribute} of your %{model} is too long (maximum is %{count} characters)")
+
+ @model = define_model(:example, :attr => :string) do
+ validates_length_of :attr, :maximum => 4
+ end.new
+ @model = Example.new
+ end
+
+ after do
@gabebw

gabebw Dec 20, 2012

Contributor

I think there's an easier way to reset the I18n database in the after block:

after { I18n.backend.reload! }

Based on this line: https://github.com/thoughtbot/shoulda-matchers/blob/master/spec/shoulda/active_model/helpers_spec.rb#L63

@ahey

ahey Dec 20, 2012

Contributor

I'll have a look - this is a cleaner approach.

Contributor

gabebw commented Dec 20, 2012

Thanks for your PR! I'm sure anyone who's used config/locales/en.yml will thank you. I have a bunch of comments, a lot of them are style (we like a consistent codebase) plus I have some questions and concerns.

Contributor

gabebw commented Dec 20, 2012

I had a bunch of comments - but it occurs to me that you might only need to add tests in spec/shoulda/active_model/helpers_spec.rb, for the default_error_message method. Did you look into this? It'd be nice to only edit 1 file instead of 5.

Owner

drapergeek commented Dec 20, 2012

👏 Thanks again for this PR, I know a couple people have asked about this!

Contributor

ahey commented Jan 4, 2013

Hi @gabebw , regarding only testing default_error_message, given that I had to make changes to both allow_value_matcher.rb and ensure_length_of_matcher.rb to fix the bug, I have created the tests in their related specs. I've added some new commits to tidy up based on earlier feedback and refactored to the new spec style.

Contributor

ahey commented Jan 11, 2013

Hi @gabebw would you mind reviewing the latest changes when you have some time?

Owner

drapergeek commented Mar 8, 2013

@ahey This is looking good. Can you remove the before and after block calls. We've been avoiding their use when possible and this is a case where it's not necessary. It's more of a style preference but if you could please update and rebase I would appreciate it. Thanks.

Member

mxie commented Mar 25, 2013

@ahey Just wanted to check in on this...we'd still love to get this in if you have some updates for us based on the comments above! Definitely rebase since there have been many changes since your last ones.

@ahey ahey Allow the use of %{attribute} or %{model} in i18n translations.
Fix a bug where shoulda was raising an exception,  when a translation
in en.yml was modified to include %{attribute} or %{model}
3302c45
Contributor

ahey commented Mar 29, 2013

@mxie As you say, there have been many changes in the last three months. I have had a look into this issue, and found that some commits were made that partially address the problem.

I placed the specs from my old PR into the latest master branch, and found that some of them are now passing, with a few failing. I've now fixed these last few bugs so that all the specs are passing, can also confirm that this PR fixes the issue originally experienced in my project specs that were using shoulda-matchers.

This should fix the issues (or at least some of the issues) listed in #53

@drapergeek i'm not sure how I could remove the after block as it it required to reset the i18n translations after each spec.

Owner

drapergeek commented Mar 29, 2013

Given that you're using the after block in every one of your actions, it would be very simple to just move that to a method and call the method at the end of the test.

That being said, I think an after block would be ok if you were to put all of them into a single context block of something like 'using translations' and call it then. That would be a good of use of the after block.

@ahey ahey Refactor some ensure_length_of_matcher_specs to remove duplication
Moved translation specs into a context with a common after block
703cfb9
Contributor

ahey commented Apr 4, 2013

Hi @drapergeek @mxie i've submitted an update :)

@mxie mxie commented on an outdated diff Apr 5, 2013

...atchers/active_model/ensure_length_of_matcher_spec.rb
@@ -105,6 +105,63 @@
end
end
+ context 'using translations' do
+ after { I18n.backend.reload! }
+ context "a too_long translation containing %{attribute}, %{model}" do
+ before do
+ stub_translation(
+ "activerecord.errors.messages.too_long",
+ "The %{attribute} of your %{model} is too long (maximum is %{count} characters)")
+
+ @model = define_model(:example, :attr => :string) do
@mxie

mxie Apr 5, 2013

Member

Defining @model here (and in the rest of the file) is actually the same as using the validating_length method that's already available, so I think you can safely remove these definitions. After doing that, you could do:

validating_length(:maximum => 4).should ensure_length_of(:attr).is_at_most(4)

And the like for the remainder of the file.

@mxie mxie commented on an outdated diff Apr 5, 2013

...ers/active_model/validate_presence_of_matcher_spec.rb
@@ -82,6 +82,24 @@
end
end
+ context "an i18n translation containing %{attribute} and %{model}" do
+ before do
+ stub_translation(
+ "activerecord.errors.messages.blank",
+ "Please enter a %{attribute} for your %{model}")
+
+ @model = define_model :example, :attr => :string do
@mxie

mxie Apr 5, 2013

Member

Same comment as before, except it's validating_presence.

Contributor

ahey commented Apr 6, 2013

@mxie PR udpated

Member

mxie commented Apr 12, 2013

@ahey - Your changes have been merged. Thanks for your PR!

@mxie mxie closed this Apr 12, 2013

andyw8 commented May 3, 2013

Will there be a new release any time soon?

Owner

drapergeek commented May 3, 2013

@andyw8 We will probably drop a release later today.

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