Validate enum column is an integer in `define_enum_for` #829

geoffharcourt · 2015-10-30T15:41:36Z

If your ActiveRecord model stores its enum data in a non-integer
column, ActiveRecord will save the data without error. However, when you
access the attribute on the record after saving, AR will look for the
string to what it expects to be a list of numbers and return nil
rather than the mapped value.

This change adds a third criterion to the define_enum_for matcher,
verifying that the underlying database column has a sql_type of
"integer".

houndci-bot · 2015-10-30T15:41:38Z

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

+        :record_with_hash_values,
+        {
+          _enum_attribute => { type: :integer },
+        }


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

houndci-bot · 2015-10-30T15:44:47Z

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

+          enum(_enum_attribute => %w(published unpublished draft))
+        end
+
+        message = "Expected #{model} to define :statuses as an enum and store the value in a column with an integer type"


Line is too long. [121/80]

houndci-bot · 2015-10-30T15:46:55Z

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

-        message = "Expected #{record.class} to define :#{plural_enum_attribute} as an enum"
-        assertion = -> {
+        message = "Expected #{record.class} to define :#{plural_enum_attribute} as an enum and store the value in a column with an integer type"
+        assertion = ->  do


Unnecessary spacing detected.

I think the style I've adopted (mostly across the board, except in a couple spots) has been to use lambda do instead of -> do, for a multi-line lambda.

houndci-bot · 2015-10-30T16:42:17Z

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

-        expect do
-          expect(record_with_array_values).to_not define_enum_for(enum_attribute)
-        end.to fail_with_message(message)
+        expect {


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

mcmire · 2015-10-30T17:26:22Z

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

-        assertion = -> {
+        message = "Expected #{record.class} to define :#{plural_enum_attribute} as an enum and store the value in a column with an integer type"
+
+        assertion = lamdba do


lambda here as well.

mcmire · 2015-11-02T03:47:55Z

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

+          :record_with_array_values,
+          _enum_attribute => { type: :string },
+        ) do |model|
+          enum enum_attribute => %w(published unpublished draft)


It looks like enum should be model.enum.

geoffharcourt · 2016-01-08T19:44:35Z

@mcmire, other than Hound warnings, is this GTG? I'd like to ship it today if possible.

mcmire · 2016-01-10T05:31:54Z

lib/shoulda/matchers/active_record/define_enum_for_matcher.rb

@@ -111,6 +113,18 @@ def enum_values_match?
          expected_enum_values.empty? || actual_enum_values == expected_enum_values
        end

+        def matched_column
+          @_matched_column ||= begin


ActiveRecord provides a columns_hash method you can use if you want the Column object for a particular attribute. So you can replace this with @model.class.columns_hash[attribute_name.to_s] instead, here.

mcmire · 2016-01-10T05:42:09Z

Hey @geoffharcourt, sorry for the lack of attention on your PR. I've been preoccupied with fixing some regressions with 3.0 so I'm afraid I forgot about this 😞

I had a few more comments above. If you would like to add an entry to the NEWS section and rebase this against master, you are free to merge this in at will.

houndci-bot · 2016-01-11T04:26:27Z

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

+
+          assertion = lambda do
+            expect(record_with_array_values).
+              to define_enum_for(non_enum_attribute).with(["open", "close"])


Prefer single-quoted strings when you don't need string interpolation or special symbols.

If your ActiveRecord model stores its `enum` data in a non-integer column, ActiveRecord will save the data without error. However, when you access the attribute on the record after saving, AR will look for the string to what it expects to be a list of numbers and return `nil` rather than the mapped value. This change adds a third criterion to the `define_enum_for` matcher, verifying that the underlying database column has a `sql_type` of `"integer"`. Fix #827.

mcmire · 2016-01-11T04:43:12Z

Hey, sorry for stepping in, but I made these changes and merged this in because I wanted to release 3.0.2. As a plus, this change will be available probably by the time you read this :)

geoffharcourt · 2016-01-11T14:09:22Z

@mcmire no need to apologize, excited to see this get merged!

houndci-bot reviewed Oct 30, 2015
View reviewed changes

geoffharcourt force-pushed the gh-validate-enum-has-integer-column branch 2 times, most recently from 1343ad5 to ab347d4 Compare October 30, 2015 15:44

houndci-bot reviewed Oct 30, 2015
View reviewed changes

geoffharcourt force-pushed the gh-validate-enum-has-integer-column branch from ab347d4 to 0d3ad55 Compare October 30, 2015 15:46

houndci-bot reviewed Oct 30, 2015
View reviewed changes

geoffharcourt force-pushed the gh-validate-enum-has-integer-column branch from ac6aedc to 27877ba Compare October 30, 2015 16:47

mcmire reviewed Oct 30, 2015
View reviewed changes

geoffharcourt force-pushed the gh-validate-enum-has-integer-column branch 2 times, most recently from ee43f19 to cac5bf9 Compare October 30, 2015 18:40

mcmire reviewed Nov 2, 2015
View reviewed changes

geoffharcourt force-pushed the gh-validate-enum-has-integer-column branch from cac5bf9 to 9055174 Compare November 2, 2015 14:26

mcmire mentioned this pull request Nov 20, 2015

should define_enum_for should validate column is an integer #827

Closed

mcmire reviewed Jan 10, 2016
View reviewed changes

mcmire force-pushed the gh-validate-enum-has-integer-column branch 2 times, most recently from 0339edc to 69786b9 Compare January 11, 2016 04:26

houndci-bot reviewed Jan 11, 2016
View reviewed changes

mcmire force-pushed the gh-validate-enum-has-integer-column branch from 69786b9 to 68dd70a Compare January 11, 2016 04:40

mcmire merged commit 68dd70a into master Jan 11, 2016

mcmire deleted the gh-validate-enum-has-integer-column branch January 11, 2016 04:40

mcmire added this to the 3.0.2 milestone Jan 11, 2016

begedin mentioned this pull request Mar 9, 2016

PR #829 breaks "define_enum_for" matcher for enums that do not actually use integer columns #912

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate enum column is an integer in `define_enum_for` #829

Validate enum column is an integer in `define_enum_for` #829

geoffharcourt commented Oct 30, 2015

houndci-bot Oct 30, 2015

houndci-bot Oct 30, 2015

houndci-bot Oct 30, 2015

mcmire Oct 30, 2015

houndci-bot Oct 30, 2015

mcmire Oct 30, 2015

mcmire Nov 2, 2015

geoffharcourt commented Jan 8, 2016

mcmire Jan 10, 2016

mcmire commented Jan 10, 2016

houndci-bot Jan 11, 2016

mcmire commented Jan 11, 2016

geoffharcourt commented Jan 11, 2016

Validate enum column is an integer in define_enum_for #829

Validate enum column is an integer in define_enum_for #829

Conversation

geoffharcourt commented Oct 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffharcourt commented Jan 8, 2016

Choose a reason for hiding this comment

mcmire commented Jan 10, 2016

Choose a reason for hiding this comment

mcmire commented Jan 11, 2016

geoffharcourt commented Jan 11, 2016

Validate enum column is an integer in `define_enum_for` #829

Validate enum column is an integer in `define_enum_for` #829