From 6e94a047ee2ad4e5a7f351bef66630d57570ca14 Mon Sep 17 00:00:00 2001
From: Jonas Meinerz <jonas@thoughtbot.com>
Date: Fri, 23 Oct 2020 14:16:30 +0100
Subject: [PATCH] Conditionals mark fields as optional (#1799)

We were assuming that all fields were required when a presence
validation existed. While that makes sense, it's also possible for
validations to be conditional.

Take the following validation as an example:

        validates :phone_number, presence: true, if: :egyptian?

Before this commit, the UI would flag phone_number as required, even for
records who were not egyptian.

We now always flag these fields as optional. This is a bit misleading
too, but it's impossible to know these things when the page is rendered,
and marking them as optional makes for a slightly better user interface,
as the user will most likely be prompted with detailed validation errors
after trying to persist an invalid item, rather than be led to fill some
fields that are, in fact, not mandatory.
---
 lib/administrate/field/base.rb                |  4 +++-
 spec/example_app/app/models/product.rb        |  4 ++--
 .../administrate/application_helper_spec.rb   | 20 ++++++++++++++-----
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/lib/administrate/field/base.rb b/lib/administrate/field/base.rb
index 8522fd91c9..0d56d0e254 100644
--- a/lib/administrate/field/base.rb
+++ b/lib/administrate/field/base.rb
@@ -52,7 +52,9 @@ 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.class == ActiveRecord::Validations::PresenceValidator &&
+            !v.options.include?(:if) &&
+            !v.options.include?(:unless)
         end
       end
 
diff --git a/spec/example_app/app/models/product.rb b/spec/example_app/app/models/product.rb
index 3df36e6037..f8aaabe043 100644
--- a/spec/example_app/app/models/product.rb
+++ b/spec/example_app/app/models/product.rb
@@ -11,10 +11,10 @@ def self.policy_class
   has_many :pages, dependent: :destroy
   has_one :product_meta_tag, dependent: :destroy
 
-  validates :description, presence: true
+  validates :description, presence: true, unless: -> { false }
   validates :image_url, presence: true
   validates :name, presence: true
-  validates :price, presence: true
+  validates :price, presence: true, if: -> { true }
   validates :release_year,
             numericality: {
               less_than_or_equal_to: ->(_product) { Time.current.year },
diff --git a/spec/helpers/administrate/application_helper_spec.rb b/spec/helpers/administrate/application_helper_spec.rb
index 493058ef83..322d6486dd 100644
--- a/spec/helpers/administrate/application_helper_spec.rb
+++ b/spec/helpers/administrate/application_helper_spec.rb
@@ -80,17 +80,27 @@
 
   describe "#requireness" do
     let(:page) do
-      Administrate::Page::Form.new(Blog::PostDashboard.new, Blog::Post.new)
+      Administrate::Page::Form.new(ProductDashboard.new, Product.new)
     end
 
     it "returns 'required' if field is required" do
-      title = page.attributes.detect { |i| i.attribute == :title }
-      expect(requireness(title)).to eq("required")
+      name = page.attributes.detect { |i| i.attribute == :name }
+      expect(requireness(name)).to eq("required")
     end
 
     it "returns 'optional' if field is not required" do
-      publish_at = page.attributes.detect { |i| i.attribute == :published_at }
-      expect(requireness(publish_at)).to eq("optional")
+      release_year = page.attributes.detect { |i| i.attribute == :release_year }
+      expect(requireness(release_year)).to eq("optional")
+    end
+
+    it "returns 'optional' if field is required if condition is met" do
+      description = page.attributes.detect { |i| i.attribute == :description }
+      expect(requireness(description)).to eq("optional")
+    end
+
+    it "returns 'optional' if field is required unless condition is met" do
+      price = page.attributes.detect { |i| i.attribute == :price }
+      expect(requireness(price)).to eq("optional")
     end
   end