FormBuilder - DisplayOnly fields cannot be required

[colemanw]

Overview

Fixes dev/core#5710

Before

A field configured as "DisplayOnly" and also "Required" would cause a form validation error.

After

No error

[civibot]

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...

• If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
• A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
• A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
• If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
• Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
• A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
• To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
• For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers...

• ➡️ PR Demo site (can be rebuilt with the command test this please).
• 📖 Review standards
• 🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

[danielstrum]

Hi @colemanw, I tested this and it seems that it no longer throws an error but it also does not validate that it is required. I am able to submit even with the field being empty.

[colemanw]

@danielstrum yes, that's the idea. See https://lab.civicrm.org/dev/core/-/issues/5710#note_176913

[danielstrum]

Oh... I didn't see that. Thanks but this sort of defeats the purpose for me. I want them to not be able to submit if the required, display only field is empty.

[colemanw]

@danielstrum as I mentioned in my comment, I think the way you're enforcing your workflow is at odds with usability: telling the user that they must fill out a field, and then making the field impossible to fill out, is going to lead to confusion/frustration.

[danielstrum]

Yes... you are right but this was totally a compromise. If my workflow works correctly, this would never be triggered because there would always be a value in those fields. However, if there is not a value, it means the form is not populating correctly and the submission is worse then useless. I actually think there should be some sort of error if a form is submitted with required fields that go no place because the form didn't populate the required entities correctly. Right now, it just submits with no error but the values are completely lost. If you want to discuss this more, let's have a short talk.

[colemanw]

@danielstrum I get that, but the other piece of this is that it would not be an easy task on my end to get what you want working. TBH it never worked the way you want, and it only seemed to work because of a bug which has now been fixed.

[danielstrum]

@colemanw It is sort of funny because I did not start out trying to make those fields required. At some point I did it by mistake and realized it would be a great fail-safe. Maybe really what is needed is for a way to make entities required so, if you add (for example) a second contact, or a grant to a form, you could make it required and the form throws an error if it is not populated when it is submitted? Does something like that make sense?

[mattwire]

@danielstrum If I understand you I think what you really want is that you can specify an entity as "required" on the form. Currently you can set fields to required to force that the entity gets filled in. Selecting "update" for an entity instead of "create" helps with this because it will fail if you didn't pass in a entity ID.

So maybe what we really need is a "required" flag for each entity that you add to the form. An entity can be loaded via URL, via pre-fill or via other methods but we'd still need to understand the difference between a "required" entity in "create" mode and "update" mode. Perhaps you could open a lab issue where we can discuss - it's a separate issue to what this PR is solving.

[danielstrum]

@colemanw @mattwire
I created an issue here: https://lab.civicrm.org/dev/core/-/issues/5727

Thanks!

[colemanw]

Sounds good. I agree required entities are a separate issue.
So if no objections I think this PR is merge-ready.