Clientside validation module: Configuration should be limited to 2 forms, not globally enabled

jilladams commented 4 months ago

Describe the defect

Public Websites has been working to fix a defect where users are able to submit an Event "at a VA location" without populating Facility location field that should be required, to populate addresses on Events. Issue: https://github.com/department-of-veterans-affairs/va.gov-cms/issues/17426 PR: https://github.com/department-of-veterans-affairs/va.gov-cms/pull/17859

During PR testing, we found that when the Clientside validation module is enabled (as it is on Prod), validation doesn't work as expected. For "conditionally required" fields, we have to require them on the client side. This means we rely on javascript to set the required property on such fields, and then the browser's native validation kicks in automatically to prevent submission of empty, required input fields. It is that native validation that is being short-circuited by Clientside Validation, somehow. There are several conditional field requirements on Events. (Client-side validation is required because Drupal isn't aware of conditional requirements.)

The problem is present for all required fields on the Events edit form (not the Event create form though, interestingly) if the Clientside Validation module is enabled (or configured to affect all Drupal forms as it is set in prod)

Per Edmund, Clientside validation should only be enabled for 2 image forms where alt text exists, so at some point new config got imported that turned it on for all forms.

To Reproduce

Events PR includes QA steps for repro. Christian's PR comment includes a video capture of reproduction: https://github.com/department-of-veterans-affairs/va.gov-cms/pull/17859#pullrequestreview-2003952724

Related issues

Original module implementation: https://github.com/department-of-veterans-affairs/va.gov-cms/issues/15436
Facilities work to work around this module's behavior on a Facilities form: https://github.com/department-of-veterans-affairs/va.gov-cms/issues/17018

Acceptance criteria

[ ] Clientside validation module config is applied only to image forms with alt text

jilladams commented 4 months ago

FYI @gracekretschmer-metrostar for your consideration. (cc @edmund-dunn )

edmund-dunn commented 4 months ago

@gracekretschmer-metrostar My memory was faulty; this was turned on for all forms. After conferring, we think this is the wrong path to follow. It has been enabled and disabling it now could have unintended consequences. That being said, it is possible to integrate clientside validation into this work.

See https://jqueryvalidation.org/category/plugin/

I found this:

$("#myform").validate({
  rules: {
    contact: {
      required: true,
      email: {
        depends: function(element) {
          return $("#contactform_email").is(":checked");
        }
      }
    }
  }
});

As a possible example to be used. My work at docroot/modules/custom/va_gov_media/js/alt_text_validation.es6.js can also be reviewed for further assistance.

We should encourage the use of clientside validation when possible and the use of #state and twig for more of the logic and less use of javascript whenever possible.

FYSA @timcosgrove

jilladams commented 2 months ago

@gracekretschmer-metrostar not urgent, just checking in on some older tickets with blockers. I know Edmund has been out. Wondering where this ticket sits in y'all's backlog / if you have a sense when it might be possible to look over? (Feel free to say: not high priority right now, ask us again later.)

jilladams commented 1 month ago

@gracekretschmer-metrostar @michelle-dooley hiya. FYI that in addition to #17424, Sitewide has a new use case for Client-side validation that would ideally depend on the fix from this ticket: https://github.com/department-of-veterans-affairs/va.gov-cms/issues/18280.

In #18280, we want to add new client-side validations on the phone paragraph type field for the following scenarios:

Phone number field
- Invalid format
- Invalid length
- Invalid characters
Extension number field
- Invalid characters

In order to do so, we will need some help from y'all. As far as I'm aware, the client-side validation implementation isn't documented yet, so we aren't sure how to go about using it correctly. And, theoretically because client-side validation is currently enabled for all forms, but won't be after this ticket merges, it would help us to understand when this work can be scheduled. (If we implement before this ticket closes, this ticket will regress our changes by limiting scope for client-side validation to just 2 forms, I think.)

Thanks for any insights!

gracekretschmer-metrostar commented 1 month ago

Slack thread: https://dsva.slack.com/archives/CT4GZBM8F/p1721406079937019

jilladams commented 3 weeks ago

Per meeting with Edmund, Grace and Daniel:

We'll rewrite this ticket to recap our actual intentions, which are no longer to limit the module to 2 forms.
Instead we'll hook into the module to provide an exception list for forms where it should not run. (I think.)
Daniel to help rewrite the ticket and verify the technical approach.
Either Sitewide or CMS teams can implement the changes, when either team is able to prioritize the work. Sitewide labels are added, for that reason.

FYI @FranECross that means this will become Drupal work we could opt to pull in at some point.

gracekretschmer-metrostar commented 3 weeks ago

@jilladams thanks for the write up. Confirming that your second bullet is correct, the exact vocabulary that Daniel used was to create a blacklist for forms to be excluded from the clientside validation module. Two other bullets I would add:

@edmund-dunn will be available to answer any questions about the clientside validation module for sitewide developers.
With this blacklist work, a readme file will be created, summarizing this work.

department-of-veterans-affairs / va.gov-cms