-
Notifications
You must be signed in to change notification settings - Fork 85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: added required field indicator #1184
base: main
Are you sure you want to change the base?
Conversation
@nvasilevski, could you review it? |
@@ -9,7 +9,7 @@ | |||
<% if !value.empty? %> | |||
<% if value.include?("\n") %> | |||
<pre><%= value %></pre> | |||
<% else %> | |||
<% else %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After changing how to render the space in the VSCode, I found the issue 🙏🏼 .
@adrianna-chang-shopify , could you review it? |
@etiennebarrie , could you review it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes sense, pretty straightforward improvement. Let's have one more review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes make sense, a couple small comments. Thanks!
def attribute_required?(task_data_show, attribute_name) | ||
model_class = task_data_show.name.constantize | ||
model_class.validators_on(attribute_name).any? do |validator| | ||
validator.kind == :presence |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might need to skip adding the class this if there are conditions like if:
, unless:
, allow_nil:
, allow_blank:
. I haven't checked how possible it is to do (I'm not sure the options are passed to the validator or just kept at the model level).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried your suggestion but I didn't find any method to check if the parameter is required. The list of methods below.
:unicode_normalized?, :delete_suffix!, :squeeze!, :each_line, :delete, :squeeze, :lstrip!, :rstrip, :delete_prefix!, :rjust, :rpartition, :each_byte, :each_char, :each_codepoint, :tr!, :tr_s!, :rstrip!, :shell_split, :last, :inquiry, :slice!, :delete!, :html_safe, :%, :exclude?, :*, :+, :ends_with?, :starts_with?, :indent!, :strip_heredoc, :dasherize, :acts_like_string?, :to, :truncate, :+@, :-@, :parse_csv, :<=>, :<<, :==, :===, :=~, :[], :[]=, :to_d, :include?, :empty?, :eql?, :hash, :freeze, :inspect, :intern, :indent, :length, :size, :succ, :in_time_zone, :to_blob, :to_str, :to_sym, :to_s, :to_i, :to_f, :to_r, :to_date, :to_datetime, :pretty_print, :from, :dup, :unpack, :unpack1, :to_c, :at, :ext, :shellescape, :count, :partition, :first, :parameterize, :pathmap, :mb_chars, :is_utf8?, :to_time, :camelcase, :titlecase, :shellsplit, :sum, :next, :underscore, :pluralize, :casecmp, :singularize, :bytesize, :casecmp?, :match, :match?, :squish, :camelize, :succ!, :next!, :upto, :insert, :byteindex, :rindex, :humanize, :upcase_first, :downcase_first, :titleize, :tableize, :clear, :classify, :byterindex, :demodulize, :deconstantize, :chr, :foreign_key, :constantize, :byteslice, :safe_constantize, :remove, :getbyte, :dedup, :dump, :bytesplice, :scrub, :scrub!, :index, :setbyte, :undump, :downcase, :replace, :swapcase, :upcase!, :upcase, :reverse, :swapcase!, :hex, :oct, :downcase!, :lines, :bytes, :capitalize, :crypt, :grapheme_clusters, :prepend, :end_with?, :blank?, :codepoints, :sub, :split, :present?, :concat, :encode, :encode!, :ljust, :as_json, :squish!, :remove!, :scan, :chars, :capitalize!, :strip, :start_with?, :reverse!, :truncate_bytes, :chomp, :truncate_words, :lstrip, :ord, :gsub, :chop, :sub!, :gsub!, :append_as_bytes, :center, :delete_prefix, :delete_suffix, :each_grapheme_cluster, :slice, :chop!, :chomp!, :encoding, :force_encoding, :b, :valid_encoding?, :ascii_only?, :tr, :tr_s, :strip!, :unicode_normalize, :unicode_normalize!, :to_json, :to_json_raw, :to_json_raw_object, :between?, :clamp, :<=, :>=, :<, :>, :require_dependency, :to_param, :to_query, :presence_in, :in?, :html_safe?, :acts_like?, :with, :instance_variable_names, :presence, :to_yaml, :instance_values, :deep_dup, :with_options, :duplicable?, :pretty_print_cycle, :pretty_print_inspect, :pretty_print_instance_variables, :try, :try!, :trap, :singleton_class, :itself, :methods, :singleton_methods, :protected_methods, :private_methods, :public_methods, :instance_variables, :instance_variable_get, :instance_variable_set, :instance_variable_defined?, :remove_instance_variable, :instance_of?, :kind_of?, :is_a?, :display, :frozen?, :class, :then, :yield_self, :tap, :gem, :pretty_inspect, :public_send, :class_eval, :extend, :clone, :debugger, :!~, :nil?, :method, :respond_to?, :public_method, :singleton_method, :define_singleton_method, :object_id, :send, :to_enum, :enum_for, :equal?, :!, :__send__, :!=, :__id__, :instance_eval, :instance_exec, :singleton_method_added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to check validator.options
here to accomplish this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried use but not works as expected:
def attribute_required?(task, parameter_name)
task.class.validators_on(parameter_name).any? do |validator|
puts "Validator: #{validator.options.inspect}"
validator.options[:presence] == true
end
end
I have inspected the options and it isn't showing presence
Validator: {}
Validator: {with: /\A(\s?\d+(,\s?\d+\s?)*)\z/, allow_blank: true}
Validator: {in: [100, 200, 300], allow_nil: true}
Validator: {in: #<Proc:0x0000000145b92760 /Users/candidogomes/Documents/Projects/maintenance_tasks/test/dummy/app/tasks/maintenance/params_task.rb:41>, allow_nil: true}
Validator: {in: #<Proc:0x0000000145b92328 /Users/candidogomes/Documents/Projects/maintenance_tasks/test/dummy/app/tasks/maintenance/params_task.rb:42>, allow_nil: true}
Validator: {in: :dropdown_attr_options, allow_nil: true}
Validator: {in: Maintenance::DropdownOptions, allow_nil: true}
Validator: {within: [true, false], allow_nil: true}
Validator: {in: 100.., allow_nil: true}
036167e
to
f8b72dc
Compare
850c2b8
to
8e60f60
Compare
@adrianna-chang-shopify , could you review it? 🙏🏼 |
def attribute_required?(task_data_show, attribute_name) | ||
model_class = task_data_show.name.constantize | ||
model_class.validators_on(attribute_name).any? do |validator| | ||
validator.kind == :presence |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to check validator.options
here to accomplish this.
task_class = Class.new do | ||
class << self | ||
def validators_on(attribute) | ||
[ActiveModel::Validations::PresenceValidator.new(attributes: [attribute])] | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use the validator DSL instead of handcrafting a validator here, ie.:
task_class = Class.new(MaintenanceTasks::Task) do
validates :required_attribute, presence: true
end
required
label in the required fields