Skip to content
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

Afform - Implement client-side validation of required fields #23604

Merged
merged 2 commits into from
Sep 5, 2022

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented May 26, 2022

Overview

This is (so far) a client-side only implementation of required field validation for Afforms.

See https://lab.civicrm.org/dev/core/-/issues/3137

Before

Required fields not required.

After

image

@civibot
Copy link

civibot bot commented May 26, 2022

(Standard links)

@civibot civibot bot added the master label May 26, 2022
@@ -89,6 +92,10 @@
}

this.submit = function() {
if (!ctrl.ngForm.$valid) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colemanw just wondering if we need to be able to customise the alert here and does this work on front ends? I can't recall if CRM gets loaded correctly on front end

Copy link
Contributor

@kurund kurund Jun 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this logic will ever be invoked as required validation happens at field level.

Copy link
Contributor

@kurund kurund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colemanw

Required validation seems to be working fine on backend and frontend for standard fields.

It needs to be fixed for block like Email block.

@@ -89,6 +92,10 @@
}

this.submit = function() {
if (!ctrl.ngForm.$valid) {
Copy link
Contributor

@kurund kurund Jun 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this logic will ever be invoked as required validation happens at field level.

@aydun
Copy link
Contributor

aydun commented Jul 8, 2022

It's working for fields from the standard entities but not for fields from my custom entity. The fields show as required on the builder page but not on the display page.

To test with a custom entity, see https://lab.civicrm.org/dev/core/-/issues/3728

@eileenmcnaughton
Copy link
Contributor

@colemanw this needs a rebase - also not quite sure status otherwise but can get @kurund to recheck once it is not stale

@colemanw
Copy link
Member Author

I've rebased this @eileenmcnaughton @kurund

@kurund
Copy link
Contributor

kurund commented Aug 1, 2022

@colemanw

Below is the sample form with all the fields configured as required in the Formbuilder.
Screenshot from 2022-08-01 10-48-16

However, I was able to submit the form by filling only data in the above screenshot.

@kurund
Copy link
Contributor

kurund commented Aug 26, 2022

@colemanw

It looks like "required" param is missing in field definition for 'joins'. You can check the generated code. Hence, I the email validation is failing.

@colemanw
Copy link
Member Author

colemanw commented Sep 3, 2022

@kurund I've fixed setting default required value for e.g. Email joins.
I think there's still some issue with Select2, but maybe this is good enough to merge now and we can investigate Select2 in a different PR.

Copy link
Contributor

@kurund kurund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. ready to for merging.

TODO for separate PR

  • Select2 fields
  • RichText fields

@colemanw colemanw merged commit aae10e5 into civicrm:master Sep 5, 2022
@colemanw colemanw deleted the afformValidate branch September 5, 2022 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants