Skip to content

Commit

Permalink
Conditionals mark fields as optional (#1799)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Jonas Meinerz committed Oct 23, 2020
1 parent cbf9fd0 commit 6e94a04
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 8 deletions.
4 changes: 3 additions & 1 deletion lib/administrate/field/base.rb
Expand Up @@ -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

Expand Down
4 changes: 2 additions & 2 deletions spec/example_app/app/models/product.rb
Expand Up @@ -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 },
Expand Down
20 changes: 15 additions & 5 deletions spec/helpers/administrate/application_helper_spec.rb
Expand Up @@ -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

Expand Down

0 comments on commit 6e94a04

Please sign in to comment.