Fix checking case sensitivity when value is case insensitive

[albertyw]

I ran into the same issue as #787 and fixed it here.

[houndci-bot]

Put a comma after the last parameter of a multiline method call.

[tovodeverett]

What if there are constraints on the value that make appending "_case" illegal?

[mcmire]

Maybe "when the value is the same as its swapcased version"?

Also add a line break above this line, per style.

[mcmire]

The current behavior is to test that the record either allows or disallows the swapcased version of the value whether case_insensitive is specified or not. I just read that issue and I think what we should do is not even bother making this assertion if the value is the same as its swapcased version instead of adding alpha characters onto the value (or raising an error as the aforementioned issue suggests), because in this case, the case_sensitive: false setting on the validation really isn't doing anything.

So what if you changed the validate_case_sensitivity? method to look like this?

if value.respond_to?(:swapcase) && value != value.swapcase
  # allow or disallow
else
  true
end

[tovodeverett]

From a testing standpoint, the default behavior is to verify that the uniqueness is indeed case sensitive (or to test that the uniqueness is indeed case insensitive if we declare case_insensitive). If I make a mistake and supply a model that has a value that is swapcase immutable, then we're no longer testing either of those, but as a user I wouldn't necessarily be aware of that change since it was made silently.

Perhaps we need an option for ignore_case_sensitivity. For instance, if I have a string where I store formatted phone numbers. I might have validation rules that make it impossible to store alpha characters. On the other hand, I might have a field where I want to test that it is case insensitive, but I was oblivious and set the value to '123'. It would be nice in the latter case to be informed, "Hey, if you want the test to validate case sensitivity, you had better pass a value that lets us test that!"

[mcmire]

@tovodeverett Okay, that's fair. I don't know if we need an option for it because then you'd have to remember to use that too. But that's a good warning to have.

I'm trying to understand your original use case... in your test, did you pre-create a record before using the matcher?

[albertyw]

In my tests, I was not pre-creating the object (shoulda-matchers does it internally though).

I think @tovodeverett's suggestion is best. The current status will fail with a non-useful error and there's no way modifying the value will work because of possible additional validations. Instead of defaulting to true if the value isn't case sensitive (which therefore doesn't test anything), perhaps a RuntimeError can be raised so that an extra option doesn't need to be added.

[mcmire]

Okay, so you're saying that if you as the developer accidentally use a value that wouldn't create a good test by being un-swapcase-able, then you'd want the test to fail so that you're forced to replace it with something better?

[albertyw]

Well, I guess to expand a bit, if there are validations to preclude changing the value to something swapcase-able (e.g. a phone number), then the test should be changed to be case insensitive. (i.e. if you hit 1, then an error message should appear to change your code to 2 or 3)

1. Validation - sensitive, test - sensitive, value - insensitive = throw error
2. Validation - sensitive, test - insensitive, value - insensitive = accepts
3. Validation - sensitive, test - sensitive, value - sensitive = accepts (works already)

[tovodeverett]

In my case, the underlying value is case-sensitively unique. It's a field where I store pattern matches (SQL patterns for LIKE). I'm using factory_girl, and the pattern was along the lines of '%-1-%', which is, of course, unchanging with swapcase! I realized that since my underlying implementation was case-sensitively unique, I didn't want to indicate to the test that it should be case insensitive (since that didn't match the implementation). So I changed the value in factory_girl to '%-1[-a]%' and thus ensured that I got a test that it was indeed case sensitive.

I'm 95% onboard with albertyw's suggestion - if the stored value cannot contain alpha characters, then suggest to the developer that they add case_insensitive to the test. Basically, the error message would guide the developer into one of two resolutions: Either supply a test value that mutates with swapcase, or explicitly make the test case_insensitive.

[mcmire]

Okay, just so I understand, it seems like there are two uses cases here:

1. The developer has a case-sensitive validation in the model and is using the matcher without case_insensitive, but the value the matcher is using isn't swapcase-able. To fix this, the developer should provide a record with a value that is swapcase-able.
2. The developer has a case-sensitive validation on an attribute that holds values which do not contain any alpha characters. The value the matcher is using also isn't swapcase-able, but changing the value won't work -- in this case the developer needs to change the validation (and the matcher) to be case-insensitive.

So as you say @tovodeverett we just need to explain to the developer why is this happening and guide them into one of these choices.

[tovodeverett]

Yup! Technically case 2 is a simplification, but I think it's good enough.

Technically, the attribute in question isn't really case-sensitive or case-insensitive - the concept of case sensitivity doesn't apply to an attribute which can't store alpha characters, but since calling it case-insensitive will work, I think that's a good solution (and definitely simpler than defining yet another concept). Don't get me started on I18N, Turkish i's, and collations!

[houndci-bot]

Line is too long. [127/80]
Space missing to the left of {.
Space missing inside {.
Space missing inside }.

[houndci-bot]

Avoid using {...} for multi-line blocks.

[albertyw]

@tovodeverett @mcmire I've updated the pull request based on your comments.

[mcmire]

Cool, thanks so much. I'll take a look at this tonight.

[mcmire]

@albertyw @tovodeverett Okay, I reworded the error message to explain this in more detail. What do you think about this? (the model name, attribute, and value will be filled in dynamically)

Your Example model has a uniqueness validation on :attr which is declared to be case-sensitive, but the value the uniqueness matcher used, "123", doesn't contain any alpha characters, so using it to test the case-sensitivity part of the validation is ineffective. There are two possible solutions for this depending on what you're trying to do here:

1. If you meant for the validation to be case-sensitive, then you need to give the uniqueness matcher a saved instance of Example with a value for :attr that contains alpha characters:

   # RSpec
   it do
     record = Example.create!(attr: "some value with alpha characters")
     expect(record).to validate_uniqueness_of(:attr)
   end
   
   # Minitest (Shoulda)
   should "validate uniqueness of :attr" do
     record = Example.create!(attr: "some value with alpha characters")
     assert_accepts validate_uniqueness_of(:attr), record
   end

2. If you meant for the validation to be case-insensitive, then you need to qualify the validation, as well as the matcher, appropriately:

   # Model
   class Example < ActiveRecord::Base
     validates_uniqueness_of :attr, case_sensitive: false
   end
   
   # RSpec
   describe Example do
     it { should validate_uniqueness_of(:attr).case_insensitive }
   end
   
   # Minitest (Shoulda)
   class ExampleTest < ActiveSupport::TestCase
     should validate_uniqueness_of(:attr).case_insensitive
   end

[tovodeverett]

I think that error message is insanely awesome!!!! In fact, it's so awesome that I'm going to purposely use a few test values that don't have alpha characters just so I can smile when it prints out! 👍

It does a great job of explaining to the developer what the problem is, what the alternatives are, and provides a good example of how to resolve the situation under each alternative.

Be careful - if every error message I got as a developer was this helpful, Stack Overflow would need a new business model!

[albertyw]

I like the error message too - it's very clear. However, I think it's a bit long to output to console (certainly can work, but at minimum the formatting would be stripped). Should the error message link to the text in the readme or the website?

[mcmire]

@tovodeverett Thanks for the kind words! 😊

@albertyw Yeah... I was starting to think that. I do love great error messages, but if you saw four of these in a row then it might be a bit much. Let's try this:

Your Example model has a uniqueness validation on :attr which is declared to be case-sensitive, but the value the uniqueness matcher used, "123", doesn't contain any alpha characters, so using it to test the case-sensitivity part of the validation is ineffective. There are two possible solutions for this depending on what you're trying to do here:

1. If you meant for the validation to be case-sensitive, then you need to give the uniqueness matcher a saved instance of Example with a value for :attr that contains alpha characters.
2. If you meant for the validation to be case-insensitive, then you need to add case_sensitive: false to the validation and add case_insensitive to the matcher.

For full examples, please see:

http://github.com/thoughtbot/shoulda-matchers/wiki/error-uniqcs

[houndci-bot]

Line is too long. [96/80]

[houndci-bot]

Line is too long. [84/80]

[albertyw]

I've updated the error with your new text.

@mcmire - I think your original text can be copied to the wiki but I can't copy your markdown. Can you do it? (I fixed your link typo to http://github.com/thoughtbot/shoulda-matchers/wiki/error-uniques)

[mcmire]

Cool. Thanks for the PR. I've merged your changes as ada9bd3. Instead of adding docs to the wiki (which I don't plan on maintaining -- really it's just user-submitted documentation), I opted to add them to the global docs. The change for that is in 80407e6.

Thanks for the discussion around this, folks!

[albertyw]

Great. Looking forward to seeing this in the next release!

[mcmire]

@albertyw @tovodeverett This is now fixed in 3.0.1.