Fix implementation of jQuery.validate

[mattwire]

Overview

Fix implementation of jQuery validate in CiviCRM so that it actually uses the CRM settings, validates select2 fields and uses Civi/bootstrap classes for errors.

Partial from #16488

Before

jQuery validate in CiviCRM did not work with select2. In Common.js there was code to load CiviCRM specific settings but they weren't actually being loaded!

After

select2 is validated (note that the error message is currently the label - that could be improved as could the positioning of some of the error messages - that's all possible but out of scope for this PR!):

Technical Details

If the PR involves technical details/changes/considerations which would not be manifest to a casual developer skimming the above sections, please describe the details here.

Comments

Testing

For testing you can do in the browser console on a contribution page:
var validator = CRM.$('#Main').validate();
validator.settings - to show the settings that are loaded for the page.
CRM.$('#Main').valid(); to check if the form is "valid" - returns true/false.

Currently this will return valid for many required fields on the form if they are empty. This is because CiviCRM is not setting them as required before creating the form - that will be fixed in #16488.
So to test you have to add the class "required" to the form fields once the page has loaded:

This is particularly a problem for any processor that has "billing address" fields.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[seamuslee001]

@mattwire can you rebase this please

[mattwire]

Thanks @seamuslee001 rebased. Note that I found something similar here https://github.com/coopsymbiotic/coop.symbiotic.symbioticux/blob/master/templates/CRM/Symbioticux/Contribute/Form/Contribution/Main.validate.tpl so would be great if @mlutfy was able to comment on this PR.

Also note that originally I added in crm-inline-error alert alert-danger but later changed to error alert-danger. This is because by default alert adds padding round the element which looks pretty ugly in some cases whereas alert-danger just changes the background color. And using error instead of crm-inline-error seems more universal.

[mlutfy]

I have to admit I don't really remember why that file was forked in symbioticux some time ago. It may have been to remove the alerts, or add CSS classes, although I've noticed a few times that select2 fields were not correctly validated before submit, and that can be a huge annoyance.

The code seems OK, but I have to admit that I'm not the most qualified to comment here. @colemanw ?

Also added sig:interface because this might have an impact on the CSS of existing sites?

[seamuslee001]

I think this all looks good to me haven't r-run it yet tho

[seamuslee001]

@mattwire are you able to rebase this please.

[mattwire]

@seamuslee001 Thanks - now rebased

[mattwire]

@seamuslee001 @colemanw I've updated based on comments and think this is now ok to merge?

[jaapjansma]

Betty and I are reviewing PR's and we came across this one.

We are struggling with testing this PR. We checked a contribution page and the javascript CRM.$('#Main').valid(); gives true altough all our fields are empty and required.

See screenshot:

Also when we press review your contribution we get different error messages then in the screenshot provided by after. See our screenshot

[mattwire]

@jaapjansma Sorry for not explaining better :-) Currently there is another problem with the CiviCRM forms that means they don't get detected by jQuery validate - that will be fixed in #16488 but it is not yet ready. I have added above in the PR description some more information.

[artfulrobot]

• General standards
  • (r-explain) PASS
  • (r-user) PASS
  • (r-doc) PASS
  • (r-run) CONDITIONAL PASS - see below
• Developer standards
  • (r-tech) PASS
  • (r-code) PASS
  • (r-maint) PASS
  • (r-test) UNTESTED

(tested on 5.26 buildkit)

I ran this by looking at a contribution form as anon user and clicking submit. Fields lit up as errors.

I added required to an input and to the hidden-by-select2 select element and hit validate again and then saw errors on those fields, too.

The conditional pass is that:

• I believe the non-validated fields are related to the other PR Pass required attribute for quickform fields through to form #16488
• There's an odd behaviour around hidden fields that may be unrelated to this or not. Hidden fields are not validated, but neither is any existing validation ignored.
  1. click submit on an empty form to make a required field show as invalid.
  2. edit the field to set it to hidden.
  3. click submit again.
     The error does not go away, even though the field is now hidden. I'm not sure if there's any cases where this could cause problems - e.g. if some fields are hidden conditionally based on other inputs, then could it result in their error messages persisting?

[jaapjansma]

@jaapjansma Sorry for not explaining better :-) Currently there is another problem with the CiviCRM forms that means they don't get detected by jQuery validate - that will be fixed in #16488 but it is not yet ready. I have added above in the PR description some more information.

So basically this PR is not yet ready for review that is what I understand.

[mattwire]

@jaapjansma We have been shipping the same thing (via an override) in Stripe for a long time. The PR, in my opinion, is ready for review and merge because it improves the form validation. But we need to follow up with additional PRs to make further changes.
For testing we should check that it validates some of the fields and does not fail validation when it should pass. There will be a lot of things that should fail but don't but this PR is not fixing those. It is just putting in place the ability to fix them.

[jaapjansma]

@eileenmcnaughton @colemanw what do you think? Betty and I have reviewed it on March 23 and @artfulrobot on May 26th. So I would say you should have a call on this whether this could be merged or not as we are not able to review it any further.

[mattwire]

#16488 is a much bigger change - it actually changes the way the form fields are rendered (so validation will behave differently). This PR (#16495) does not depend on #16488 as it is a frontend-only (javascript) change which improves the way validation works on the form if it has the right information (and that information will come from a version of #16488). So we need to review/merge this one before we can work on #16488 (#16488 has a much bigger chance to break things..)

[eileenmcnaughton]

@colemanw will have to be the final merger on this one as those are very central js functions and I don't feel able to assess whether all the implications of touching them have been addressed in the review

[eileenmcnaughton]

@colemanw are you able to check this one for final merge?

[mattwire]

Merged via #16488