Project

General

Profile

Bug #7450

We mark not-required fields as required

Added by Marek Hulán about 5 years ago. Updated about 1 year ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Web Interface
Target version:
Difficulty:
Triaged:
Bugzilla link:
Fixed in Releases:
Found in Releases:

Description

If the validator has some condition (:if => $smt, :unless => $smt) we shouldn't mark the field as required. In this case we simply can't tell without AJAX call because validity may depend on another field of the form. Btw there was a project translating AR validations to javascript (client side validations) but IIRC it's discontinued. Also it was quite big thing.


Related issues

Related to Foreman - Feature #6400: mark required fields in formClosed2014-06-26
Related to Foreman - Bug #7560: add :required => true on host fields that have conditional validation if host.managed?Closed2014-09-21
Related to Foreman - Feature #21678: Mark required fields conditionally based on other fieldsNew2017-11-15

Associated revisions

Revision 3b87444a (diff)
Added by Marek Hulán about 5 years ago

Fixes #7450 - do not mark non-required fields

If validation is conditional we don't mark fields as required by
default. Also fixes disabling by override using :required option.

History

#1 Updated by Marek Hulán about 5 years ago

#2 Updated by Dominic Cleal about 5 years ago

Do you have an example field where this is wrong? There's an override option which can be passed into the form helper to disable it, we just need to know which field(s) require it :)

#3 Updated by Marek Hulán about 5 years ago

I'm afraid we can just enforce required to be true. Looking to the code I see

required_mark = ' *' if is_required?(f, attr) || options[:required]

see https://github.com/theforeman/foreman/blob/develop/app/helpers/layout_helper.rb#L153 and https://github.com/theforeman/foreman/blob/develop/app/helpers/layout_helper.rb#L167 for more details.

I have no existing example since it's in form I'm currently working on for bonding.

something like

<%= text_f f, :attached_to, :help_inline => _("Identifier of the interface to which this interface belongs, e.g. eth1") %>

adding :required => false does not help obviously since is_required?(f, attr) is always true

#4 Updated by Dominic Cleal about 5 years ago

Indeed, that should use options[:required] unless it's nil, and only then fall back to is_required.

#5 Updated by Marek Hulán about 5 years ago

Yup, will send a PR shortly. I thought it would be also useful to make is_required? aware of conditions and if some is present, return false (probably makes sense in most cases?). Thoughts?

#6 Updated by Dominic Cleal about 5 years ago

Sounds good!

#7 Updated by The Foreman Bot about 5 years ago

  • Status changed from New to Ready For Testing
  • Target version set to 1.7.3
  • Pull request https://github.com/theforeman/foreman/pull/1766 added
  • Pull request deleted ()

#8 Updated by Marek Hulán about 5 years ago

  • Assignee set to Marek Hulán

#9 Updated by Marek Hulán about 5 years ago

  • Status changed from Ready For Testing to Closed
  • % Done changed from 0 to 100

#10 Updated by Dominic Cleal about 5 years ago

  • Legacy Backlogs Release (now unused) set to 21

#11 Updated by Dominic Cleal about 5 years ago

  • Related to Bug #7560: add :required => true on host fields that have conditional validation if host.managed? added

#12 Updated by Marek Hulán almost 2 years ago

  • Related to Feature #21678: Mark required fields conditionally based on other fields added

Also available in: Atom PDF