Add ignoring case sensitivity to uniqueness validation matcher #840

dgmstuart · 2015-11-13T03:46:50Z

Fixes #836
also possibly #838?

Duplicates some of the code on the WIP PR #820 - will cause some conflicts with that, but hopefully nothing too tricky.

The issue is that the existing functionality was either actively
asserting case sensitivity or case insensitivity. This commit adds an
option to not assert either.

This allows handling of the scenario where the case of an attribute is
changed at some point before being assigned on the model.

Perhaps it now makes sense to have an explicit .case_sensitive option as well?

In preparation for adding a third option (:ignore), we can't have the case sensitivity option as a boolean anymore.

Fixes thoughtbot#836 also possibly thoughtbot#838 ? The issue is that the existing functionality was either actively asserting case sensitivity or case insensitivity. This commit adds an option to not assert either. This allows handling of the scenario where the case of an attribute is changed at some point before being assigned on the model.

There are 3 cases: assert case sensitive, assert case insensitive, don't assert case. The description message mentions when it's asserting, and doesn't say anything if nothing is asserted.

houndci-bot · 2015-11-13T03:46:57Z

spec/unit/shoulda/matchers/active_record/validate_uniqueness_of_matcher_spec.rb

+        end
+
+        expect(&assertion).to raise_error(
+          Shoulda::Matchers::ActiveModel::AllowValueMatcher::CouldNotSetAttributeError


Line is too long. [86/80]
Put a comma after the last parameter of a multiline method call.

dgmstuart · 2015-11-27T14:10:59Z

@mcmire Hi - if there's anything I can do to help get this reviewed and merged please let me know :)

mcmire · 2016-01-07T19:23:12Z

spec/unit/shoulda/matchers/active_record/validate_uniqueness_of_matcher_spec.rb

+    context 'and ignoring_case_sensitivity is specified' do
+      it 'accepts (and not raise an error)' do
+        model = define_model_validating_uniqueness(
+          attribute_name: :name,


I'm noticing that in the first test, you have the validation qualified with case_sensitive: false, but you don't have that here.

I'm kind of forgetting what the original bug was here. Is that something we need to have here, you think? Or, should we have two sets of tests, one where the validation is case_sensitive: true, one where it's case_sensitive: false?

mcmire · 2016-01-07T19:23:59Z

@dgmstuart Hey, sorry for the delay on getting back to you about this. I've finally resolved the issues with ignoring_interference_by_writer, so I'm turning my attention to this now. I had a quick question above, although I need to take a look at the original issues around this and review this further to see how we need to test this.

mcmire · 2016-01-07T21:19:11Z

By the way, I think this PR can incorporate an idea I had in #838, which is to have the uniqueness matcher, upon failure, suggest to the developer that he or she try using ignoring_case_sensitivity in the failure message. Unfortunately, there isn't a great way to add some text to the failure message. allow_value is the matcher that the uniqueness matcher uses internally, so if the uniqueness matcher were to fail, then it's allow_value that would be producing a failure message.

allow_value does provide a sort of extension point that lets you customize the failure message already: you can set failure_message_preface to a proc that returns some text (here's how that's used now). But as its name implies, it affects the beginning of the message, not the end.

We could add a similar extension point -- say, failure_message_suffix -- except we don't always want to add some text every time the uniqueness matcher fails, we want to add some text specifically when the attribute being tested changes incoming values. If that happens, there's another method in allow_value called attribute_changed_value_message that holds some additional text that will get shown to the user:

As indicated in the message above, :#{result.attribute_setter.attribute_name}
seems to be changing certain values as they are set, and this could have
something to do with why this test is failing. If you've overridden the writer
method for this attribute, then you may need to change it to make this test
pass, or do something else entirely.

I think it's here where we want to add some more text, something like:

Specifically, if the attribute changes the case of incoming values, try adding
`ignoring_case_sensitivity` to the end of the matcher.

mcmire · 2016-01-07T21:19:27Z

Also, I was going back through the issues and I think this PR should fix #786 as well.

mcmire · 2016-01-07T23:14:42Z

Hey - so I made some tweaks to your branch, and specifically made the change I mentioned above concerning the failure message. Here's those changes along with your original changes. What do you think?

mcmire · 2016-01-11T05:19:47Z

I'd like to get 3.0.2 out the door, so I went ahead and merged this into master as d7d96ad.

dgmstuart · 2016-01-16T14:39:16Z

Hi - great to gave this in the new release. Your changes look good to me - adding a message suggesting using the new option is nice!

Have a great weekend.

mcmire · 2016-01-16T15:46:21Z

Thanks!

@bar

# 3.1.2 ### Deprecations * This is the **last version** that supports Rails 4.0 and 4.1 and Ruby 2.0 and 2.1. ### Bug fixes * When the `permit` matcher was used without `#on`, the controller did not use `params#require`, the params object was duplicated, and the matcher did not recognize the `#permit` call inside the controller. This behavior happened because the matcher overwrote double registries with the same parameter hash whenever ActionController::Parameters was instantiated. * *Commit: [44c019]* * *Issue: [#899]* * *Pull request: [#902]* # 3.1.1 ### Bug fixes * Some matchers make use of ActiveSupport's `in?` method, but do not include the file where this is defined in ActiveSupport. This causes problems with projects using shoulda-matchers that do not include all of ActiveSupport by default. To fix this, replace `in?` with Ruby's builtin `include?`. * *Pull request: [#879]* * `validate_uniqueness_of` works by creating a record if it doesn't exist, and then testing against a new record with various attributes set that are equal to (or different than) corresponding attributes in the existing record. In 3.1.0 a change was made whereby when the uniqueness matcher is given a new record and creates an existing record out of it, it ensures that the record is valid before continuing on. This created a problem because if the subject, before it was saved, was empty and therefore in an invalid state, it could not effectively be saved. While ideally this should be enforced, doing so would be a backward-incompatible change, so this behavior has been rolled back. ([#880], [#884], [#885]) * *Commit: [45de869]* * *Issues: [#880], [#884], [#885]* * Fix an issue with `validate_uniqueness_of` + `scoped_to` when used against a model where the attribute has multiple uniqueness validations and each validation has a different set of scopes. In this case, a test written for the first validation (and its scopes) would pass, but tests for the other validations (and their scopes) would not, as the matcher only considered the first set of scopes as the *actual* set of scopes. * *Commit: [28bd9a1]* * *Issues: [#830]* ### Improvements * Update `validate_uniqueness_of` so that if an existing record fails to be created because a column is non-nullable and was not filled in, raise an ExistingRecordInvalid exception with details on how to fix the test. * *Commit: [78ccfc5]* [#879]: thoughtbot/shoulda-matchers#879 [45de869]: thoughtbot/shoulda-matchers@45de869 [#880]: thoughtbot/shoulda-matchers#880 [#884]: thoughtbot/shoulda-matchers#884 [#885]: thoughtbot/shoulda-matchers#885 [78ccfc5]: thoughtbot/shoulda-matchers@78ccfc5 [28bd9a1]: thoughtbot/shoulda-matchers@28bd9a1 [#830]: thoughtbot/shoulda-matchers#830 # 3.1.0 ### Bug fixes * Update `validate_numericality_of` so that submatchers are applied lazily instead of immediately. Previously, qualifiers were order-dependent, meaning that if you used `strict` before you used, say, `odd`, then `strict` wouldn't actually apply to `odd`. Now the order that you specify qualifiers doesn't matter. * *Source: [6c67a5e]* * Fix `allow_value` so that it does not raise an AttributeChangedValueError (formerly CouldNotSetAttributeError) when used against an attribute that is an enum in an ActiveRecord model. * *Source: [9e8603e]* * Add a `ignoring_interference_by_writer` qualifier to all matchers, not just `allow_value`. *This is enabled by default, which means that you should never get a CouldNotSetAttributeError again.* (You may get some more information if a test fails, however.) * *Source: [1189934], [5532f43]* * *Fixes: [#786], [#799], [#801], [#804], [#817], [#841], [#849], [#872], [#873], and [#874]* * Fix `validate_numericality_of` so that it does not blow up when used against a virtual attribute defined in an ActiveRecord model (that is, an attribute that is not present in the database but is defined using `attr_accessor`). * *Source: [#822]* * Update `validate_numericality_of` so that it no longer raises an IneffectiveTestError if used against a numeric column. * *Source: [5ed0362]* * *Fixes: [#832]* [6c67a5e]: thoughtbot/shoulda-matchers@6c67a5e [9e8603e]: thoughtbot/shoulda-matchers@9e8603e [1189934]: thoughtbot/shoulda-matchers@1189934 [5532f43]: thoughtbot/shoulda-matchers@5532f43 [#786]: thoughtbot/shoulda-matchers#786 [#799]: thoughtbot/shoulda-matchers#799 [#801]: thoughtbot/shoulda-matchers#801 [#804]: thoughtbot/shoulda-matchers#804 [#817]: thoughtbot/shoulda-matchers#817 [#841]: thoughtbot/shoulda-matchers#841 [#849]: thoughtbot/shoulda-matchers#849 [#872]: thoughtbot/shoulda-matchers#872 [#873]: thoughtbot/shoulda-matchers#873 [#874]: thoughtbot/shoulda-matchers#874 [#822]: thoughtbot/shoulda-matchers#822 [5ed0362]: thoughtbot/shoulda-matchers@5ed0362 [#832]: thoughtbot/shoulda-matchers#832 ### Features * Add a new qualifier, `ignoring_case_sensitivity`, to `validate_uniqueness_of`. This provides a way to test uniqueness of an attribute whose case is normalized, either in a custom writer method for that attribute, or in a custom `before_validation` callback. * *Source: [#840]* * *Fixes: [#836]* [#840]: thoughtbot/shoulda-matchers#840 [#836]: thoughtbot/shoulda-matchers#836 ### Improvements * Improve failure messages and descriptions of all matchers across the board so that it is easier to understand what the matcher was doing when it failed. (You'll see a huge difference in the output of the numericality and uniqueness matchers in particular.) * Matchers now raise an error if any attributes that the matcher is attempting to set do not exist on the model. * *Source: [2962112]* * Update `validate_numericality_of` so that it doesn't always run all of the submatchers, but stops on the first one that fails. Since failure messages now contain information as to what value the matcher set on the attribute when it failed, this change guarantees that the correct value will be shown. * *Source: [8e24a6e]* * Continue to detect if attributes change incoming values, but now instead of immediately seeing a CouldNotSetAttributeError, you will only be informed about it if the test you've written fails. * *Source: [1189934]* * Add an additional check to `define_enum_for` to ensure that the column that underlies the enum attribute you're testing is an integer column. * *Source: [68dd70a]* * Add a test for `validate_numericality_of` so that it officially supports money columns. * *Source: [a559713]* * *Refs: [#841]* [2962112]: thoughtbot/shoulda-matchers@2962112 [8e24a6e]: thoughtbot/shoulda-matchers@8e24a6e [68dd70a]: thoughtbot/shoulda-matchers@68dd70a [a559713]: thoughtbot/shoulda-matchers@a559713 # 3.0.1 ### Bug fixes * Fix `validate_inclusion_of` + `in_array` when used against a date or datetime column/attribute so that it does not raise a CouldNotSetAttributeError. ([#783], [8fa97b4]) * Fix `validate_numericality_of` when used against a numeric column so that it no longer raises a CouldNotSetAttributeError if the matcher has been qualified in any way (`only_integer`, `greater_than`, `odd`, etc.). ([#784], [#812]) ### Improvements * `validate_uniqueness_of` now raises a NonCaseSwappableValueError if the value the matcher is using to test uniqueness cannot be case-swapped -- in other words, if it doesn't contain any alpha characters. When this is the case, the matcher cannot work effectively. ([#789], [ada9bd3]) [#783]: thoughtbot/shoulda-matchers#783 [8fa97b4]: thoughtbot/shoulda-matchers@8fa97b4 [#784]: thoughtbot/shoulda-matchers#784 [#789]: thoughtbot/shoulda-matchers#789 [ada9bd3]: thoughtbot/shoulda-matchers@ada9bd3 [#812]: thoughtbot/shoulda-matchers#812 # 3.0.0 ### Backward-incompatible changes * We've dropped support for Rails 3.x, Ruby 1.9.2, and Ruby 1.9.3, and RSpec 2. All of these have been end-of-lifed. ([a4045a1], [b7fe87a], [32c0e62]) * The gem no longer detects the test framework you're using or mixes itself into that framework automatically. [History][no-auto-integration-1] has [shown][no-auto-integration-2] that performing any kind of detection is prone to bugs and more complicated than it should be. Here are the updated instructions: * You no longer need to say `require: false` in your Gemfile; you can include the gem as normal. * You'll need to add the following somewhere in your `rails_helper` (for RSpec) or `test_helper` (for Minitest / Test::Unit): ``` ruby Shoulda::Matchers.configure do |config| config.integrate do |with| # Choose a test framework: with.test_framework :rspec with.test_framework :minitest with.test_framework :minitest_4 with.test_framework :test_unit # Choose one or more libraries: with.library :active_record with.library :active_model with.library :action_controller # Or, choose the following (which implies all of the above): with.library :rails end end ``` ([1900071]) * Previously, under RSpec, all of the matchers were mixed into all of the example groups. This created a problem because some gems, such as [active_model_serializers-matchers], provide matchers that share the same name as some of our own matchers. Now, matchers are only mixed into whichever example group they belong to: * ActiveModel and ActiveRecord matchers are available only in model example groups. * ActionController matchers are available only in controller example groups. * The `route` matcher is available only in routing example groups. ([af98a23], [8cf449b]) * There are two changes to `allow_value`: * The negative form of `allow_value` has been changed so that instead of asserting that any of the given values is an invalid value (allowing good values to pass through), assert that *all* values are invalid values (allowing good values not to pass through). This means that this test which formerly passed will now fail: ``` ruby expect(record).not_to allow_value('good value', *bad_values) ``` ([19ce8a6]) * `allow_value` now raises a CouldNotSetAttributeError if in setting the attribute, the value of the attribute from reading the attribute back is different from the one used to set it. This would happen if the writer method for that attribute has custom logic to ignore certain incoming values or change them in any way. Here are three examples we've seen: * You're attempting to assert that an attribute should not allow nil, yet the attribute's writer method contains a conditional to do nothing if the attribute is set to nil: ``` ruby class Foo include ActiveModel::Model attr_reader :bar def bar=(value) return if value.nil? @bar = value end end describe Foo do it do foo = Foo.new foo.bar = "baz" # This will raise a CouldNotSetAttributeError since `foo.bar` is now "123" expect(foo).not_to allow_value(nil).for(:bar) end end ``` * You're attempting to assert that an numeric attribute should not allow a string that contains non-numeric characters, yet the writer method for that attribute strips out non-numeric characters: ``` ruby class Foo include ActiveModel::Model attr_reader :bar def bar=(value) @bar = value.gsub(/\D+/, '') end end describe Foo do it do foo = Foo.new # This will raise a CouldNotSetAttributeError since `foo.bar` is now "123" expect(foo).not_to allow_value("abc123").for(:bar) end end ``` * You're passing a value to `allow_value` that the model typecasts into another value: ``` ruby describe Foo do # Assume that `attr` is a string # This will raise a CouldNotSetAttributeError since `attr` typecasts `[]` to `"[]"` it { should_not allow_value([]).for(:attr) } end ``` With all of these failing examples, why are we making this change? We want to guard you (as the developer) from writing a test that you think acts one way but actually acts a different way, as this could lead to a confusing false positive or negative. If you understand the problem and wish to override this behavior so that you do not get a CouldNotSetAttributeError, you can add the `ignoring_interference_by_writer` qualifier like so. Note that this will not always cause the test to pass. ``` ruby it { should_not allow_value([]).for(:attr).ignoring_interference_by_writer } ``` ([9d9dc4e]) * `validate_uniqueness_of` is now properly case-sensitive by default, to match the default behavior of the validation itself. This is a backward-incompatible change because this test which incorrectly passed before will now fail: ``` ruby class Product < ActiveRecord::Base validates_uniqueness_of :name, case_sensitive: false end describe Product do it { is_expected.to validate_uniqueness_of(:name) } end ``` ([57a1922]) * `ensure_inclusion_of`, `ensure_exclusion_of`, and `ensure_length_of` have been removed in favor of their `validate_*` counterparts. ([55c8d09]) * `set_the_flash` and `set_session` have been changed to more closely align with each other: * `set_the_flash` has been removed in favor of `set_flash`. ([801f2c7]) * `set_session('foo')` is no longer valid syntax, please use `set_session['foo']` instead. ([535fe05]) * `set_session['key'].to(nil)` will no longer pass when the key in question has not been set yet. ([535fe05]) * Change `set_flash` so that `set_flash[:foo].now` is no longer valid syntax. You'll want to use `set_flash.now[:foo]` instead. This was changed in order to more closely align with how `flash.now` works when used in a controller. ([#755], [#752]) * Change behavior of `validate_uniqueness_of` when the matcher is not qualified with any scopes, but your validation is. Previously the following test would pass when it now fails: ``` ruby class Post < ActiveRecord::Base validate :slug, uniqueness: { scope: :user_id } end describe Post do it { should validate_uniqueness_of(:slug) } end ``` ([6ac7b81]) [active_model_serializers-matchers]: https://github.com/adambarber/active_model_serializers-matchers [no-auto-integration-1]: freerange/mocha@049080c [no-auto-integration-2]: rr/rr#29 [1900071]: thoughtbot/shoulda-matchers@1900071 [b7fe87a]: thoughtbot/shoulda-matchers@b7fe87a [a4045a1]: thoughtbot/shoulda-matchers@a4045a1 [57a1922]: thoughtbot/shoulda-matchers@57a1922 [19ce8a6]: thoughtbot/shoulda-matchers@19c38a6 [eaaa2d8]: thoughtbot/shoulda-matchers@eaaa2d8 [55c8d09]: thoughtbot/shoulda-matchers@55c8d09 [801f2c7]: thoughtbot/shoulda-matchers@801f2c7 [535fe05]: thoughtbot/shoulda-matchers@535fe05 [6ac7b81]: thoughtbot/shoulda-matchers@6ac7b81 [#755]: thoughtbot/shoulda-matchers#755 [#752]: thoughtbot/shoulda-matchers#752 [9d9dc4e]: thoughtbot/shoulda-matchers@9d9dc4e [32c0e62]: thoughtbot/shoulda-matchers@32c0e62 [af98a23]: thoughtbot/shoulda-matchers@af98a23 [8cf449b]: thoughtbot/shoulda-matchers@8cf449b ### Bug fixes * So far the tests for the gem have been running against only SQLite. Now they run against PostgreSQL, too. As a result we were able to fix some Postgres-related bugs, specifically around `validate_uniqueness_of`: * When scoped to a UUID column that ends in an "f", the matcher is able to generate a proper "next" value without erroring. ([#402], [#587], [#662]) * Support scopes that are PostgreSQL array columns. Please note that this is only supported for Rails 4.2 and greater, as versions before this cannot handle array columns correctly, particularly in conjunction with the uniqueness validator. ([#554]) * Fix so that when scoped to a text column and the scope is set to nil before running it through the matcher, the matcher does not fail. ([#521], [#607]) * Fix `define_enum_for` so that it actually tests that the attribute is present in the list of defined enums, as you could fool it by merely defining a class method that was the pluralized version of the attribute name. In the same vein, passing a pluralized version of the attribute name to `define_enum_for` would erroneously pass, and now it fails. ([#641]) * Fix `permit` so that it does not break the functionality of ActionController::Parameters#require. ([#648], [#675]) * Fix `validate_uniqueness_of` + `scoped_to` so that it does not raise an error if a record exists where the scoped attribute is nil. ([#677]) * Fix `route` matcher so if your route includes a default `format`, you can specify this as a symbol or string. ([#693]) * Fix `validate_uniqueness_of` so that it allows you to test against scoped attributes that are boolean columns. ([#457], [#694]) * Fix failure message for `validate_numericality_of` as it sometimes didn't provide the reason for failure. ([#699]) * Fix `shoulda/matchers/independent` so that it can be required independently, without having to require all of the gem. ([#746], [e0a0200]) ### Features * Add `on` qualifier to `permit`. This allows you to make an assertion that a restriction was placed on a slice of the `params` hash and not the entire `params` hash. Although we don't require you to use this qualifier, we do recommend it, as it's a more precise check. ([#675]) * Add `strict` qualifier to `validate_numericality_of`. ([#620]) * Add `on` qualifier to `validate_numericality_of`. ([9748869]; h/t [#356], [#358]) * Add `join_table` qualifier to `have_and_belong_to_many`. ([#556]) * `allow_values` is now an alias for `allow_value`. This makes more sense when checking against multiple values: ``` ruby it { should allow_values('this', 'and', 'that') } ``` ([#692]) [9748869]: thoughtbot/shoulda-matchers@9748869 [#402]: thoughtbot/shoulda-matchers#402 [#587]: thoughtbot/shoulda-matchers#587 [#662]: thoughtbot/shoulda-matchers#662 [#554]: thoughtbot/shoulda-matchers#554 [#641]: thoughtbot/shoulda-matchers#641 [#521]: thoughtbot/shoulda-matchers#521 [#607]: thoughtbot/shoulda-matchers#607 [#648]: thoughtbot/shoulda-matchers#648 [#675]: thoughtbot/shoulda-matchers#675 [#677]: thoughtbot/shoulda-matchers#677 [#620]: thoughtbot/shoulda-matchers#620 [#693]: thoughtbot/shoulda-matchers#693 [#356]: thoughtbot/shoulda-matchers#356 [#358]: thoughtbot/shoulda-matchers#358 [#556]: thoughtbot/shoulda-matchers#556 [#457]: thoughtbot/shoulda-matchers#457 [#694]: thoughtbot/shoulda-matchers#694 [#692]: thoughtbot/shoulda-matchers#692 [#699]: thoughtbot/shoulda-matchers#699 [#746]: thoughtbot/shoulda-matchers#746

dgmstuart added 3 commits November 13, 2015 03:24

(refactor): Case sensitivity option uses symbols

b5554ed

In preparation for adding a third option (:ignore), we can't have the case sensitivity option as a boolean anymore.

(feature): Explicitly mention case insensitive

fa058e4

There are 3 cases: assert case sensitive, assert case insensitive, don't assert case. The description message mentions when it's asserting, and doesn't say anything if nothing is asserted.

houndci-bot reviewed Nov 13, 2015
View reviewed changes

mcmire added this to the 3.0.2 milestone Nov 27, 2015

mcmire reviewed Jan 7, 2016
View reviewed changes

mcmire mentioned this pull request Jan 7, 2016

validates_uniqueness_of with custom setter fails with "should require case sensitive unique value for email" #836

Closed

mcmire added the 🏃‍♀️PR: In Progress label Jan 7, 2016

mcmire closed this Jan 11, 2016

mcmire removed the 🏃‍♀️PR: In Progress label Jan 11, 2016

mcmire mentioned this pull request Jul 12, 2016

allow_value should detect if values are changed post-validation #838

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ignoring case sensitivity to uniqueness validation matcher #840

Add ignoring case sensitivity to uniqueness validation matcher #840

dgmstuart commented Nov 13, 2015

houndci-bot Nov 13, 2015

dgmstuart commented Nov 27, 2015

mcmire Jan 7, 2016

mcmire commented Jan 7, 2016

mcmire commented Jan 7, 2016

mcmire commented Jan 7, 2016

mcmire commented Jan 7, 2016

mcmire commented Jan 11, 2016

dgmstuart commented Jan 16, 2016

mcmire commented Jan 16, 2016

Add ignoring case sensitivity to uniqueness validation matcher #840

Add ignoring case sensitivity to uniqueness validation matcher #840

Conversation

dgmstuart commented Nov 13, 2015

houndci-bot Nov 13, 2015

Choose a reason for hiding this comment

dgmstuart commented Nov 27, 2015

mcmire Jan 7, 2016

Choose a reason for hiding this comment

mcmire commented Jan 7, 2016

mcmire commented Jan 7, 2016

mcmire commented Jan 7, 2016

mcmire commented Jan 7, 2016

mcmire commented Jan 11, 2016

dgmstuart commented Jan 16, 2016

mcmire commented Jan 16, 2016