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

Enable ability to require tags in profiles #23645

Merged
merged 1 commit into from
Jun 8, 2022

Conversation

jmcclelland
Copy link
Contributor

Overview

This PR enables the ability to make the Tag field required in a profile. For additional discussion, see this stack exchange post.

Before

If you:

  • Create a profile with stand alone checked
  • Include the Tags field
  • Marks the Tags field as required

When you display the profile in create mode:

  1. No red asterisk appears next to the Tags field
  2. You are allowed to submit it without a Tag selected

After

A red asterisk appears when the form is displayed and you are not able to submit the form without entering at least one tag.

Technical Details

It seems that this has simply been overlooked and nobody noticed or decided it should be fixed.

Comments

I tried to write a test to ensure it continues to work, but alas, the Api V3 profile submit code does, in fact, enforce the tags requirement. But, create mode does not seem to use that Api v3 function. I'm open to suggestions on how to properly test this feature.

@civibot
Copy link

civibot bot commented May 31, 2022

(Standard links)

@civibot civibot bot added the master label May 31, 2022
@@ -137,6 +137,9 @@ public static function buildQuickForm(

if (!empty($tags)) {
$form->add('select2', 'tag', ts('Tag(s)'), $tags, FALSE, ['class' => 'huge', 'placeholder' => ts('- select -'), 'multiple' => TRUE]);
if ($isRequired) {
$form->addRule('tag', ts('Tag is a required field.'), 'required');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you just change param 5 from FALSE to $isRequired? I haven't tested but it seems like that would do the same thing as the if and addRule.

For unit testing you could try using $form = $this->getFormObject('CRM_Profile_Form_Edit', [params]). I don't know offhand if there's an existing test that is similar - sometimes you have to fiddle a bit with the params and $_GET to make it work for the given form, but then you can do stuff like call $form->buildForm(), $form->postProcess(), etc.

@jmcclelland
Copy link
Contributor Author

@demeritcowboy Thanks for the tip on $isRequired - I just made that change.

But I'm banging my head against the wall with the test. I've added my work so far, but for some reason I can't seem to get the test for whether a required field is present to trigger.

The commented out formRule code was always coming back without errors.

Any suggestions?

$form = $this->getFormObject('CRM_Profile_Form_Edit', $formParams);
$form->set('gid', $profileID);
$form->preProcess();
$form->buildQuickForm();
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking maybe adding $errors = $form->validate() here after buildQuickForm might do it but doesn't seem to. Hmm.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see it. This form handles its own error handling slightly differently than other forms (because it's Civi!) It expects there to be an errorUrl param and then does a redirect. The return value from validate() is just a true/false here, so it actually does work to call $form->validate(), I was just expecting an array of error messages. So you can do $this->assertFalse($form->validate()); after the buildQuickForm(), and don't need the postProcess since then you're done with the test.

@jmcclelland jmcclelland force-pushed the profile-require-tag branch from d40e6a0 to 12d0654 Compare June 8, 2022 17:29
Prior to this change, it was not possible to make tags required in a
profile.
@jmcclelland jmcclelland force-pushed the profile-require-tag branch from 12d0654 to e661b0f Compare June 8, 2022 17:30
@jmcclelland
Copy link
Contributor Author

@demeritcowboy Thank you!!! I was tearing my hair out as I spiraled down a recursive grep rabbit hole...
Now it all seems to work as expected (and I fixed a parse error I had introduced).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants