From 0516dbde319b0ddff5a3984baa5f25aa657fc6c7 Mon Sep 17 00:00:00 2001 From: Jeremy Woertink Date: Mon, 14 Sep 2020 11:18:08 -0700 Subject: [PATCH 1/3] Fixing issue where a required validation on a bool field would fail if the value was false. Fixes #457 --- spec/save_operation_spec.cr | 33 +++++++++++++++++++ .../boxes/model_with_default_values_box.cr | 4 +++ src/avram/save_operation.cr | 5 ++- 3 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 spec/support/boxes/model_with_default_values_box.cr diff --git a/spec/save_operation_spec.cr b/spec/save_operation_spec.cr index 420a900f3..c4984bf18 100644 --- a/spec/save_operation_spec.cr +++ b/spec/save_operation_spec.cr @@ -16,6 +16,14 @@ private class SaveUser < User::SaveOperation end end +private class SaveUserWithFalseValueValidations < User::SaveOperation + permit_columns :nickname, :available_for_hire + + before_save do + validate_required nickname, available_for_hire + end +end + private class SaveLimitedUser < User::SaveOperation permit_columns :name end @@ -428,6 +436,17 @@ describe "Avram::SaveOperation" do r.drafted_at.should eq drafted_at end end + + it "updates with a record that has defaults" do + model = ModelWithDefaultValuesBox.create + params = Avram::Params.new({"greeting" => "Hi"}) + OverrideDefaults.update(model, params) do |_operation, record| + record.should_not eq nil + r = record.not_nil! + r.greeting.should eq "Hi" + r.admin.should eq false + end + end end end @@ -553,6 +572,20 @@ describe "Avram::SaveOperation" do end end end + + context "when the default is false, but the field is required" do + it "passes required validation as 'false' is a valid Boolean value" do + user = UserBox.create &.nickname("oopsie").available_for_hire(false) + params = Avram::Params.new({"nickname" => "falsey mcfalserson"}) + SaveUserWithFalseValueValidations.update(user, params) do |operation, record| + record.should_not eq nil + r = record.not_nil! + operation.errors.should eq({} of Symbol => Array(String)) + r.nickname.should eq "falsey mcfalserson" + r.available_for_hire.should eq false + end + end + end end describe ".update!" do diff --git a/spec/support/boxes/model_with_default_values_box.cr b/spec/support/boxes/model_with_default_values_box.cr new file mode 100644 index 000000000..38df7167d --- /dev/null +++ b/spec/support/boxes/model_with_default_values_box.cr @@ -0,0 +1,4 @@ +class ModelWithDefaultValuesBox < BaseBox + def initialize + end +end diff --git a/src/avram/save_operation.cr b/src/avram/save_operation.cr index fbcbd198a..f6995c44e 100644 --- a/src/avram/save_operation.cr +++ b/src/avram/save_operation.cr @@ -120,10 +120,13 @@ abstract class Avram::SaveOperation(T) < Avram::Operation end private def _{{ attribute[:name] }} + record_value = @record.try(&.{{ attribute[:name] }}) + value = record_value.nil? ? default_value_for_{{ attribute[:name] }} : record_value + @_{{ attribute[:name] }} ||= Avram::Attribute({{ attribute[:type] }}?).new( name: :{{ attribute[:name].id }}, param: permitted_params["{{ attribute[:name] }}"]?, - value: @record.try(&.{{ attribute[:name] }}) || default_value_for_{{ attribute[:name] }}, + value: value, param_key: self.class.param_key) end From ac4915306fec82bd36902f5f55f6a00999014160 Mon Sep 17 00:00:00 2001 From: Jeremy Woertink Date: Fri, 18 Sep 2020 08:58:39 -0700 Subject: [PATCH 2/3] applying spec cleanup changes --- spec/save_operation_spec.cr | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/spec/save_operation_spec.cr b/spec/save_operation_spec.cr index c4984bf18..346298545 100644 --- a/spec/save_operation_spec.cr +++ b/spec/save_operation_spec.cr @@ -439,13 +439,9 @@ describe "Avram::SaveOperation" do it "updates with a record that has defaults" do model = ModelWithDefaultValuesBox.create - params = Avram::Params.new({"greeting" => "Hi"}) - OverrideDefaults.update(model, params) do |_operation, record| - record.should_not eq nil - r = record.not_nil! - r.greeting.should eq "Hi" - r.admin.should eq false - end + record = OverrideDefaults.update!(model, greeting: "Hi") + record.greeting.should eq "Hi" + record.admin.should eq false end end end @@ -573,14 +569,14 @@ describe "Avram::SaveOperation" do end end - context "when the default is false, but the field is required" do - it "passes required validation as 'false' is a valid Boolean value" do + context "when the default is false and the field is required" do + it "is valid since 'false' is a valid Boolean value" do user = UserBox.create &.nickname("oopsie").available_for_hire(false) params = Avram::Params.new({"nickname" => "falsey mcfalserson"}) SaveUserWithFalseValueValidations.update(user, params) do |operation, record| record.should_not eq nil r = record.not_nil! - operation.errors.should eq({} of Symbol => Array(String)) + operation.valid?.should be_true r.nickname.should eq "falsey mcfalserson" r.available_for_hire.should eq false end From 187ca9d91fb59d7c2c265954d8b21066a07178eb Mon Sep 17 00:00:00 2001 From: Jeremy Woertink Date: Fri, 18 Sep 2020 09:07:10 -0700 Subject: [PATCH 3/3] Removing this box since it's not actually needed for anything --- spec/save_operation_spec.cr | 2 +- spec/support/boxes/model_with_default_values_box.cr | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) delete mode 100644 spec/support/boxes/model_with_default_values_box.cr diff --git a/spec/save_operation_spec.cr b/spec/save_operation_spec.cr index 346298545..742606de8 100644 --- a/spec/save_operation_spec.cr +++ b/spec/save_operation_spec.cr @@ -438,7 +438,7 @@ describe "Avram::SaveOperation" do end it "updates with a record that has defaults" do - model = ModelWithDefaultValuesBox.create + model = ModelWithDefaultValues::SaveOperation.create! record = OverrideDefaults.update!(model, greeting: "Hi") record.greeting.should eq "Hi" record.admin.should eq false diff --git a/spec/support/boxes/model_with_default_values_box.cr b/spec/support/boxes/model_with_default_values_box.cr deleted file mode 100644 index 38df7167d..000000000 --- a/spec/support/boxes/model_with_default_values_box.cr +++ /dev/null @@ -1,4 +0,0 @@ -class ModelWithDefaultValuesBox < BaseBox - def initialize - end -end