From 255167d6ba15725b1a9e7999c4269e69bc3195c2 Mon Sep 17 00:00:00 2001 From: Pablo Brasero <36066+pablobm@users.noreply.github.com> Date: Fri, 26 Feb 2021 16:20:35 +0000 Subject: [PATCH] Correct "required" asterisk when using validation option `:on` (#1872) We show a "required" asterisk when fields are required. However there are some edge cases, like when the requirement has an if/unless. We don't consider those actually required and we don't show the asterisk there. But there are other edge cases. Issue #1860 shows how the validation option :on can also create cases where an apparent requirement is not actually a requirement. Normally this option has the values :create or :update, but there are other possibilities. This PR tries to solve the issue. There are three commits, each an incremental improvement: 1. Consider :on to be the same as :if and :unless, and hide the asterisk. 2. Refactor specs to avoid the mystery guests involved in the examples for :if, :unless, and :on. This also allows for more scalable specs, should we decide to add more fine-grained handling of these options. 3. Consider the special (and most common) cases of on: :create and on: :update, and hide the asterisk for any other :on value. Fixes #1860 --- lib/administrate/field/base.rb | 22 ++- spec/example_app/app/models/product.rb | 5 +- spec/features/form_spec.rb | 2 + .../administrate/application_helper_spec.rb | 10 -- spec/lib/fields/base_spec.rb | 170 ++++++++++++++++++ 5 files changed, 194 insertions(+), 15 deletions(-) create mode 100644 spec/lib/fields/base_spec.rb diff --git a/lib/administrate/field/base.rb b/lib/administrate/field/base.rb index 0d56d0e254..5460ee07a0 100644 --- a/lib/administrate/field/base.rb +++ b/lib/administrate/field/base.rb @@ -52,9 +52,25 @@ def required? return false unless resource.class.respond_to?(:validators_on) resource.class.validators_on(attribute).any? do |v| - v.class == ActiveRecord::Validations::PresenceValidator && - !v.options.include?(:if) && - !v.options.include?(:unless) + next false unless v.class == ActiveRecord::Validations::PresenceValidator + + options = v.options + next false if options.include?(:if) + next false if options.include?(:unless) + + if on_option = options[:on] + if on_option == :create && !resource.persisted? + next true + end + + if on_option == :update && resource.persisted? + next true + end + + next false + end + + true end end diff --git a/spec/example_app/app/models/product.rb b/spec/example_app/app/models/product.rb index f8aaabe043..64e11b4154 100644 --- a/spec/example_app/app/models/product.rb +++ b/spec/example_app/app/models/product.rb @@ -11,16 +11,17 @@ def self.policy_class has_many :pages, dependent: :destroy has_one :product_meta_tag, dependent: :destroy - validates :description, presence: true, unless: -> { false } + validates :description, presence: true validates :image_url, presence: true validates :name, presence: true - validates :price, presence: true, if: -> { true } + validates :price, presence: true validates :release_year, numericality: { less_than_or_equal_to: ->(_product) { Time.current.year }, }, allow_blank: true validates :slug, uniqueness: true + validates :product_meta_tag, presence: true, on: :some_unclear_situation validate :valid_slug accepts_nested_attributes_for :product_meta_tag diff --git a/spec/features/form_spec.rb b/spec/features/form_spec.rb index 133ef005af..08f8a52425 100644 --- a/spec/features/form_spec.rb +++ b/spec/features/form_spec.rb @@ -34,6 +34,8 @@ required_field_translations = [ Product.human_attribute_name(:name), + Product.human_attribute_name(:description), + Product.human_attribute_name(:price), Product.human_attribute_name(:image_url), ] diff --git a/spec/helpers/administrate/application_helper_spec.rb b/spec/helpers/administrate/application_helper_spec.rb index 090188c32f..f2bbf42600 100644 --- a/spec/helpers/administrate/application_helper_spec.rb +++ b/spec/helpers/administrate/application_helper_spec.rb @@ -92,16 +92,6 @@ release_year = page.attributes.detect { |i| i.attribute == :release_year } expect(requireness(release_year)).to eq("optional") end - - it "is 'optional' for fields required only conditionally (with :unless)" do - description = page.attributes.detect { |i| i.attribute == :description } - expect(requireness(description)).to eq("optional") - end - - it "is 'optional' for fields required only conditionally (with :if)" do - price = page.attributes.detect { |i| i.attribute == :price } - expect(requireness(price)).to eq("optional") - end end describe "#sort_order" do diff --git a/spec/lib/fields/base_spec.rb b/spec/lib/fields/base_spec.rb new file mode 100644 index 0000000000..f1d9a2cc94 --- /dev/null +++ b/spec/lib/fields/base_spec.rb @@ -0,0 +1,170 @@ +require "administrate/field/url" +require "active_record" + +describe Administrate::Field::Base do + let(:field_class) { Class.new(Administrate::Field::Base) } + + describe "required?" do + it "is false by default" do + resource_class = class_double( + "ActiveRecord::Base", + validators_on: [], + ) + resource = instance_double( + "ActiveRecord::Base", + class: resource_class, + ) + field = field_class.new(:attribute, :date, :page, resource: resource) + + expect(field.required?).to eq(false) + end + + it "is true on an unconditional requirement for a value" do + validator = ActiveRecord::Validations::PresenceValidator.new( + attributes: [:foo], + options: {}, + ) + resource_class = class_double( + "ActiveRecord::Base", + validators_on: [validator], + ) + resource = instance_double( + "ActiveRecord::Base", + class: resource_class, + ) + field = field_class.new(:attribute, :date, :page, resource: resource) + + expect(field.required?).to eq(true) + end + + it "is false on a conditional requirement for a value (with :if)" do + validator = ActiveRecord::Validations::PresenceValidator.new( + attributes: [:foo], + if: -> { true }, + ) + resource_class = class_double( + "ActiveRecord::Base", + validators_on: [validator], + ) + resource = instance_double( + "ActiveRecord::Base", + class: resource_class, + ) + field = field_class.new(:attribute, :date, :page, resource: resource) + + expect(field.required?).to eq(false) + end + + it "is false on a conditional requirement for a value (with :unless)" do + validator = ActiveRecord::Validations::PresenceValidator.new( + attributes: [:foo], + unless: -> { true }, + ) + resource_class = class_double( + "ActiveRecord::Base", + validators_on: [validator], + ) + resource = instance_double( + "ActiveRecord::Base", + class: resource_class, + ) + field = field_class.new(:attribute, :date, :page, resource: resource) + + expect(field.required?).to eq(false) + end + + it "is true for an unpersisted record in only required on create" do + validator = ActiveRecord::Validations::PresenceValidator.new( + attributes: [:foo], + on: :create, + ) + resource_class = class_double( + "ActiveRecord::Base", + validators_on: [validator], + ) + resource = instance_double( + "ActiveRecord::Base", + class: resource_class, + persisted?: false, + ) + field = field_class.new(:attribute, :date, :page, resource: resource) + + expect(field.required?).to eq(true) + end + + it "is false for a persisted record if only required on create" do + validator = ActiveRecord::Validations::PresenceValidator.new( + attributes: [:foo], + on: :create, + ) + resource_class = class_double( + "ActiveRecord::Base", + validators_on: [validator], + ) + resource = instance_double( + "ActiveRecord::Base", + class: resource_class, + persisted?: true, + ) + field = field_class.new(:attribute, :date, :page, resource: resource) + + expect(field.required?).to eq(false) + end + + it "is true for a persisted record in only required on update" do + validator = ActiveRecord::Validations::PresenceValidator.new( + attributes: [:foo], + on: :update, + ) + resource_class = class_double( + "ActiveRecord::Base", + validators_on: [validator], + ) + resource = instance_double( + "ActiveRecord::Base", + class: resource_class, + persisted?: true, + ) + field = field_class.new(:attribute, :date, :page, resource: resource) + + expect(field.required?).to eq(true) + end + + it "is false for a persisted record in only required on update" do + validator = ActiveRecord::Validations::PresenceValidator.new( + attributes: [:foo], + on: :update, + ) + resource_class = class_double( + "ActiveRecord::Base", + validators_on: [validator], + ) + resource = instance_double( + "ActiveRecord::Base", + class: resource_class, + persisted?: false, + ) + field = field_class.new(:attribute, :date, :page, resource: resource) + + expect(field.required?).to eq(false) + end + + it "is false when required only on unstandard situations" do + validator = ActiveRecord::Validations::PresenceValidator.new( + attributes: [:foo], + on: :some_situation_or_the_other, + ) + resource_class = class_double( + "ActiveRecord::Base", + validators_on: [validator], + ) + resource = instance_double( + "ActiveRecord::Base", + class: resource_class, + ) + field = field_class.new(:attribute, :date, :page, resource: resource) + + expect(field.required?).to eq(false) + end + end +end