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

@@ -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
 

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

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),
     ]
 

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

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