No more throws a error when use validate_uniqueness_of with scope of boolean column

[maurogeorge]

closes #457

[houndci-bot]

Rename is_a_boolean_scope? to a_boolean_scope?.

[mcmire]

Why do we need to destroy the record here? I would think we'd want to keep it around, that way we're properly testing the validation.

[maurogeorge]

When we are dealing with date, string and integers for example we do not have this problem because the next_value is always a unused value.

all_records
=> [#<Example:0x007f9a932a56b8 id: 1, attr: "dummy value", scope: 2>, #<Example:0x007f9a932a53e8 id: 2, attr: "dummy value", scope: 3>]

previous_value
=> 3

next_value
=> 4

allows_value_of(existing_value, @expected_message)
=> true

But when we are dealing with a boolean our next_value will always be equal the first one created by the matcher

all_records
=> [#<Example:0x007f9a96982d48 id: 1, attr: "dummy value", scope: false>, #<Example:0x007f9a969825c8 id: 2, attr: "dummy value", scope: true>]

previous_value
=> true

next_value
=> false

allows_value_of(existing_value, @expected_message)
=> true

So if I not delete the first record, the allows_value_of will always be false since the scope is already defined on the first record, and here we can define only two value, true and false.

I really don't know if this is the better approach, if you have any idea let me know. I will dig around to see if I found another way to do that.

[mcmire]

Gotcha. Yeah I was thinking of this exact case, I made some comments below.

[mcmire]

I think the tests for these are going to be a little different, because there's a limited number of values for a boolean column.

If you have one existing record where the scope attribute is either true or false, then you can simply flip that scope attribute. If you have two existing records where one of the record's scope attribute is true and the other is false... then you can't flip the scope attribute.

Say you have a validation on an attribute against a boolean scope:

class Person < ActiveRecord::Base
  validates :name, scope: :snowflake
end

Here's what a test would look like if there's only one existing record:

describe Person do
  it "is valid with a unique name when snowflake is true" do
    create(:person, name: "Steve", snowflake: true)

    person = build(:person, name: "Steve", snowflake: true)
    person.valid?
    expect(person).errors[:name].not_to be_empty

    person = build(:person, name: "Joe", snowflake: true)
    person.valid?
    expect(person).errors[:name].to be_empty

    # here we can test against the snowflake attribute -
    # i.e. if name stays same but snowflake is different
    person = build(:person, name: "Steve", snowflake: false)
    person.valid?
    expect(person).errors[:name].to be_empty
  end
end

But now consider if there are two existing records. Here's what those tests might look like:

describe Person do
  it "is valid with a unique name when snowflake is true" do
    create(:person, name: "Steve", snowflake: true)
    create(:person, name: "Steve", snowflake: false)

    person = build(:person, name: "Steve", snowflake: true)
    person.valid?
    expect(person).errors[:name].not_to be_empty

    person = build(:person, name: "Joe", snowflake: true)
    person.valid?
    expect(person).errors[:name].to be_empty

    # we can't really test what happens if name stays same and snowflake is different
    # b/c all values for snowflake are filled - so let's just test what happens if name is
    # different when snowflake is false
    person = build(:person, name: "Steve", snowflake: false)
    person.valid?
    expect(person).errors[:name].not_to be_empty

    person = build(:person, name: "Joe", snowflake: false)
    person.valid?
    expect(person).errors[:name].not_to be_empty
  end
end

So now the question is... how do we modify the matcher to run these tests. I don't have a clear solution for this, but it seems like as soon as one of the scopes is a boolean attribute, it kind of messes with the existing logic...

[houndci-bot]

Line is too long. [102/80]

[maurogeorge]

@mcmire I updated the code and skip the when more than one record exists that has the next version of the attribute's value if the value type is boolean.

My thought was, if the scope is a boolean and we define the first record as true the second one as false we do not need this test, since this expect a third valid option, and since we are dealing with booleans we can remove this test and have a test that rejects when we already have 2 records one as true and other as false.

This makes sense to you?

[mcmire]

This should actually accept, I think...

So... this is something I actually noticed a while back. We're actually missing a check here. If you're validating uniqueness of a given attribute and it's scoped, then the first thing you should check is, if I change the value of that attribute to a unique value, then it should make the record valid. But currently we just loop through all the scope attributes and assert that if we change those to unique values then it should make the record valid.

But even if we added this check then the matcher would still fail. This is because we are still trying to check that column within #validate_after_scope_change?. We need to somehow exclude the check in this case, since both true and false are taken as possible next values.

[maurogeorge]

@mcmire I updated, now the #validate_after_scope_change? will return true when the scope is a boolean.

[mcmire]

Thanks @maurogeorge, that looks better. I'll look at this in more detail later to make sure we're doing the right thing here.

[mcmire]

I think this looks correct. But it looks like now we need to add a test for "when there is more than one scoped attribute and all are boolean columns".

[mcmire]

Perhaps we can rename this to boolean_value?

[houndci-bot]

Line is too long. [91/80]

[maurogeorge]

@mcmire updated please take a look.

[mcmire]

Thanks! Merged as 7c8e3f5.