dev/core/#101: Allow further customization of search form in hooks #12078

mickadoo · 2018-05-03T15:56:01Z

Overview

The contact search form uses the template templates/CRM/Contact/Form/Search/Criteria/Basic.tpl. In the template there are already some conditionals to hide certain fields, such as

{if $form.group}
  <td>
  ...
{/if}

However only some fields are covered. For an extension developer who wants to hide more fields here it is difficult without causing warnings or breaking the layout.

To make it easier conditionals should be applied to each form element.

Before

Form conditionals were only available for some form elements in the templates/CRM/Contact/Form/Search/Criteria/Basic.tpl template

What it looked like before:

After

Form conditionals are available for all form elements in the templates/CRM/Contact/Form/Search/Criteria/Basic.tpl template

What it looked like after (with no fields changed, should be the same as "Before")

What it looked like after with the following fields removed

email
job_title
external_identifier
privacy_toggle
email_on_hold
preferred_communication_method

Notes

I reformatted the code a bit, fixing warnings from the IDE. I also changed some existing conditionals to not output anything if the element wasn't set. As it was before they would still show an empty td element leading to gaps in the form like:

Without these empty td elements the gaps are filled:

Gitlab Issue

https://lab.civicrm.org/dev/core/issues/101

reneolivo · 2018-05-03T18:30:12Z

Reviewing

reneolivo

Before

After

(please refer to comment about this issue)

Can you please provide some quick usage example on how you can hide these fields from a plugin? If you can do that I can replicate it locally.

Other than that the changes look good

reneolivo · 2018-05-03T18:37:23Z

templates/CRM/Contact/Form/Search/Criteria/Basic.tpl

-    <td>{$form.tag_search.label}  {help id="id-all-tags"}<br />{$form.tag_search.html}</td>
+    {if $form.tag_search}
+      <td>{$form.tag_search.label}  {help id="id-all-tags"}<br/>
+        {$form .tag_search.html}


⚠️ this is breaking the search because there's an extra space between $form and .tag_search.html. If you remove this space it fixes the issue.

eileenmcnaughton · 2018-05-03T21:21:15Z

@michaelmcandrew it's perhaps not possible here but where I have been updating forms to be more changeable by hooks I have been doing it by assigning the fields to use as an array - e.g

4fb5fcf#diff-7f0dd843b42789babf5e6a63cde98198L72

(part of the motivation for an array approach is they can also be re-ordered and others can be asserted - ie the default becomes to add an item in the array with

        {$form.$element.label}<br/>
        {$form.$element.lhtml}

if there is no special handling for that field

michaelmcandrew · 2018-05-04T13:10:10Z

@eileenmcnaughton, i guess you meant @mickadoo

reneolivo · 2018-05-08T02:48:56Z

Hello @eileenmcnaughton I was investigating the structure of the template and the form controller and even though I agree that looping through each field would be more extensible, I concluded that it would be a bit difficult to implement.

These are the issues I found:

All the fields are stored in the global $form variable and the display of each one of the fields is handled by individual templates:
https://github.com/compucorp/civicrm-core/tree/d9c5047f2c9640316ce8dcadacbd0f4ea005212f/templates/CRM/Contact/Form/Search/Criteria
As you can see all the sections share their fields in the global $form variable and are defined manually. There's no way to tell the section each field belongs to nor the ones that belong to the basic section.
Some fields have custom labels defined by the template:
https://github.com/compucorp/civicrm-core/blob/d9c5047f2c9640316ce8dcadacbd0f4ea005212f/templates/CRM/Contact/Form/Search/Criteria/Basic.tpl#L30-L31
A lot of them have custom logic, more than the ones that don't:
https://github.com/compucorp/civicrm-core/blob/d9c5047f2c9640316ce8dcadacbd0f4ea005212f/templates/CRM/Contact/Form/Search/Criteria/Basic.tpl#L59-L99

Since the solution you propose is not straight forward I suggest implementing this solution for now which works well with the current implementation of the advanced search template.

We could then draw a plan to make the advanced search templates more customizable as a whole.

eileenmcnaughton · 2018-05-08T03:51:45Z

@reneolivio - my main concern with this 'as is' is that there is a potential that anyone who took advantage of it would later feel burnt if we change it again.

Regarding "There's no way to tell the section each field belongs to nor the ones that belong to the basic section."

The way we've done it elsewhere (e.g. billing block) is more like
php

$this->assign('basicSearchFields' => ['sort_name', 'email', 'contact_type'....]);

and in the template (not correct code)

{foreach value=basicSearchField from=$basicSearchFields} 
  {if file_exists('CRM/Contact/SearchFieldTemplates/$basicSearchField)} 
    {include 'CRM/Contact/SearchFieldTemplates/$basicSearchField'}
  {/else}
    {$form.basicSearchField.label}<br/>
    {$form.basicSearchField.html}
  {/if}
{/foreach}

(I don't know if that file exists approach actually works - I was just thinking for a complex field it might be cleaner).

Regarding the labels - in my experience people don't define the labels in the tpl files because they couldn't put in the correct label when adding the field - they just copied another field done that way at some point in time. It shouldn't be too hard to fix those

reneolivo · 2018-05-09T03:18:14Z

Hello @eileenmcnaughton !!

I took a stab at your proposed solution, I'll update things here if you don't mind:

After

There's a bit of variation compared to the original, but it's minimal. Just the spacing between fields is about 5~10px different.

Technical details

Some of the labels were moved from the template to the controller so the implementation would depend less on the template.
The basic search fields were defined in the controller in the same order as the original implementation:

$form->assign('basicSearchFields', [
  'sort_name',
  'email',
  'contact_type',
  'group',
  'contact_tags',
  'tag_search',
  'all_tag_types',
  'phone_numeric',
  'phone_location_type_id',
  'phone_phone_type_id',
  'privacy_toggle',
  'preferred_communication_method',
  'contact_source',
  'job_title',
  'preferred_language',
  'contact_id',
  'external_identifier',
  'uf_user',
]);

A smarty's template_exists method was made available on the template as a modifier:

$smarty = CRM_Core_Smarty::singleton();
$smarty->register_modifier('template_exists', array(&$smarty, 'template_exists'));

The basic search template was changed to this:

<div class="advanced-search-fields basic-fields form-layout">
  {foreach from=$basicSearchFields item=basicSearchField}
    {assign var=field value=$form[$basicSearchField]}
    {if $field}
      {capture assign=fieldTemplatePath}CRM/Contact/Form/Search/Criteria/Fields/{$basicSearchField}.tpl{/capture}
      {if $fieldTemplatePath|template_exists}
        {include file=$fieldTemplatePath}
      {else}
        <div class="search-field">
          {$field.label}<br />
          {$field.html}
        </div>
      {/if}
    {/if}
  {/foreach}
</div>

Why divs ? Because when removing one of the fields it would leave a blank space at the end of the table. Also, like the contact_tags, would display a sibling field that could break the table as there was no planned space for such field.
I would have liked using something like Bootstrap, but it's still not supported. I decided to use CSS Grids instead:

.advanced-search-fields {
  display: grid;
  /* defines a column divided by 3 sections, each with -10px wide: */
  grid-template-columns: [col] repeat(3, calc(100% / 3 - 10px));
  width: 100%;
}

.advanced-search-fields .search-field {
  padding: 5px;
}

.advanced-search-fields .search-field__span-2 {
  grid-column: col / span 2;
}
.advanced-search-fields .search-field__span-3 {
  grid-column: col / span 3;
}

There's good support for CSS Grid across all major browsers, except for IE10 and lower. Not sure which version you are supporting.

If there's a custom template defined for the field it includes it, otherwise it displays a generic field:

{capture assign=fieldTemplatePath}CRM/Contact/Form/Search/Criteria/Fields/{$basicSearchField}.tpl{/capture}
{if $fieldTemplatePath|template_exists}
  {include file=$fieldTemplatePath}
{else}
  <div class="search-field">
    {$field.label}<br />
    {$field.html}
  </div>
{/if}

A sample field template would be like this:

<div class="search-field">
  {$form.contact_tags.label}<br />
  {$form.contact_tags.html}
</div>

{if $isTagset}
  <div class="search-field search-field__span-2">
    {include file="CRM/common/Tagset.tpl"}
  </div>
{/if}

Please let me know if this approach is acceptable. Also, please guide me on how to test this on another extension. I know that by removing things from the basicSearchFields array it can be done, but I'm not 100% how the implementation would look like.

eileenmcnaughton · 2018-05-09T03:20:25Z

Thanks @reneolivo - I'll loop in @colemanw re the css / layout & I'll take a close look at the code - with any luck I can figure out how to unit test it.

Also ping @monishdeb @seamuslee001 FYI

eileenmcnaughton · 2018-05-11T00:10:56Z

test this please

eileenmcnaughton · 2018-05-11T01:09:37Z

CRM/Contact/Form/Search/Criteria.php

@@ -35,6 +35,32 @@ class CRM_Contact_Form_Search_Criteria {
   * @param CRM_Core_Form $form
   */
  public static function basic(&$form) {
+    // Allows the template to determine if another template exists:
+    $smarty = CRM_Core_Smarty::singleton();


This didn't work for me. What did work was

a) adding a new file called modifier.template_exists.php in packages/Smarty/plugins
b) then clearing smarty caches

I tried using
https://gitlab.athlonsofia.com/athlon/silla.io/blob/3b669c42e34d72d62f4ac624244d979402c7a98e/resources/smarty_plugins/modifier.template_exists.php

but it didn't work for me so am using

function smarty_modifier_template_exists($template) { $templatePath = __DIR__ . '/../../../templates/' . $template; return is_file($templatePath); }

If you want to put up a PR for a new modifer against the packages repo I will merge it

eileenmcnaughton · 2018-05-11T02:53:51Z

@reneolivo I took a look at this & it seems pretty good. I have a proposal to move from flat field definitions to a fieldSpec - try compucorp@f4e095a

this allows definition of the help file & field in the definition & makes some of the field-specific templates redundant. It is also more extendable later.

Per my comment above - I had to add a modifier to packages to get the smarty include check working - but I'm leaning towards the field tpl being defined in the array (if needed ) ie.

      'preferred_language' => [],
      'contact_id' => ['help' => ['id' => 'id-contact-id', 'file' => 'CRM/Contact/Form/Contact']],
      'external_identifier' => ['help' => ['id' => 'id-external-id', 'file' => 'CRM/Contact/Form/Contact']],
      'uf_user' => ['tpl' => 'uf_user.tpl'],

It feels like a similar lever of complexity to doing the file_exists in smarty & would be overrideable from php which might be nice.

Other points

I'm finding the way email_on_hold is in communication_preferences a bit hacky- but I'm prepared to let that one slide.
I'm finding the {if $isTagset} a little confusing - I had it 'cut' with tag_search rather than contact_tags - but I think you may be right. However that would mean we don't need a field template for tag_search

Once you've checked the proposal / comments above we will ping Coleman to check the markup although it seems sane to me

…nsion. This is an effort at a way to genericise core forms that basically exist to do crud on an entity. We have a number of fairly straight forward forms of this type in core and, in order to allow extensions to use custom fields on a range of entities we should support editing them on these core crud forms. To add custom data support to an entity we need to a) add the custom data to the form using CRM_Custom_Form_CustomData::addToForm($this); b) ensure that the entity is saved using an api call not a BAO call c) add the custom data block to the tpl file - ie {include file="CRM/common/customDataBlock.tpl"} (the above is possible due to previous work to add support & simplify) In this PR the adding of the customData is done in the EntityFormTrait, the api is previously converted and the custom data is included by using a generic tpl to support metadata applied to the form. By using metadata on the form we can also give extension writers a lot more control over what is on the form as they can add to, alter, or remove the metadata in a php hook. This is the crux of this issue civicrm#12078 & it likewise is looking to use a generic field template to add fields based on metadata. A key difference between the 2 prs is that one uses divs & the other a table & that might preclude close sharing of the approach. Note this PR DOES have the impact of adding translate links to 2 localisable relationship type fields that did not currently have them - I think this is a good thing? Example of how to enable custom fields for RelationshipType in an extension. ``` civicrm_api3('OptionValue', 'create', [ 'option_group_id' => 'cg_extend_objects', 'name' => 'civicrm_relationship_type', 'label' => ts('Relationship Type'), 'value' => 'RelationshipType', ]); ```

JoeMurray · 2018-05-16T01:25:22Z

@Monish from perspective of wanting fields to define labels in html in way good for supporting a11y, do you see any issues here or have suggestions to make? Ideally these changes when rolled out gradually across the code base would eliminate/greatly reduce the special case handling we are seeing is needed on many fields/forms.

reneolivo · 2018-05-17T02:51:58Z

Hello @eileenmcnaughton, I cherry-picked your changes, but had to do some fixes too. Here are some pointers:

The field specs are much, much, better so we should go that way.
I'm not sure why the previous modifier didn't work for you, does $smarty->template_exists(...) work for you?
I'm not sure how to add a modifier to the package repo and see the changes locally. I found a Smarty/plugins/ in core, is it ok to place the modifier there? If not, please guide me on how to to test the modifier when used from the package repo.
Can we try to use $smarty->template_exists($templatePath) instead of:

$templatePath = __DIR__ . '/../../../../templates/' . $template;

return is_file($templatePath);

I don't feel __DIR__ is safe enough.

I changed the format for the template's help block a bit since it was a really long line.
There was a mismatched [] on the field specs.
email_on_hold was placed inside preferred_communication_method because it seems they are part of the same search field, see how they are defined in the same column: https://github.com/compucorp/civicrm-core/blob/master/templates/CRM/Contact/Form/Search/Criteria/Basic.tpl#L151-L156
This is the best solution I could think of, since it doesn't seem one can be displayed without the other. Please let me know if you have a better solution and I can implement it.
isTagset I think you may be right, we need to investigate this a bit more. Do you know how to activate this field so I can determine where to best place it?

eileenmcnaughton · 2018-05-17T05:15:12Z

@reneolivo did you read this? https://lab.civicrm.org/dev/core/issues/115

When I worked through #12128 to achieve a similar outcome I would up preferring to define the tpl rather than use the file_exists

 'weird_looking_field' => [
     'name' => 'weird_looking_field', 
     'template' => CRM/Member/Form/MembershipType/weird_looking_field.tpl',

seems more flexible for overwriting?

reneolivo · 2018-05-17T09:56:53Z

@eileenmcnaughton I'll have a look at the issue, I didn't get to see it, seems I'll need a bit of time to catch up to the thread. But I rather use convention over configuration: if we manage to make this field extensible it's one less field to configure.

I'll catch up to issue's thread and also run some tests and let you know.

reneolivo · 2018-05-18T17:05:16Z

@eileenmcnaughton I have seen the comments on the issue and your other PR as well. I approve of the changes, even the template field.

I'll wait until your PR gets approved before starting to change this one in case something changes from the spec so we don't duplicate efforts before everything is settled.

Is that ok?

eileenmcnaughton · 2018-05-20T22:32:12Z

@reneolivo thanks

reneolivo · 2018-06-07T00:03:00Z

@eileenmcnaughton I have implemented the following changes:

All the commits have been squashed into one.
Implemented the field spec pattern as detailed in dev/core#115 and also implemented in Permit UI editing of custom data on other entities (here relationship_type) when enabled in an extension #12128
Removed any references to the "template_exists" smarty method. The field spec "template" was favoured instead.

eileenmcnaughton · 2018-06-07T07:13:35Z

The css seems a bit wrong now

eileenmcnaughton · 2018-06-07T07:15:09Z

Also - either as this or as a follow up I think we should extract a form template - i.e we have this one

https://github.com/civicrm/civicrm-core/blob/master/templates/CRM/Core/Form/Field.tpl

but it uses not Div so perhaps

CRM/Core/Form/FieldDiv.tpl

too

eileenmcnaughton · 2018-06-07T07:18:08Z

@reneolivo I started an issue to document the fields format over here https://github.com/civicrm/civicrm-dev-docs/issues/534

eileenmcnaughton · 2018-06-07T07:32:57Z

Actually css might be OK - might have been cached

eileenmcnaughton · 2018-06-07T07:41:38Z

OK - I have found one issue but I feel like I looked fairly carefully & we are down to the following

If an activity tag type is defined then the 'Include tags used for Activities' doesn't show
I don't think we need an override for this field

Before

After

reneolivo · 2018-06-08T10:52:52Z

@eileenmcnaughton I worked on your feedback. Here are the changes:

The fields with custom templates were stripped of their wrapper div.search-field tag. That tag is now handled at the Basic.tpl level.
Implemented the field description for phone_numeric and uf_user which made their custom template fields unnecessary.
all_tag_types ("Include tags used for Activities") is displayed if the field is included in the basic search form.

Also, the tag set field is displayed when there is a tag set related to contacts. Since this field was previously displayed by checking if the template has the $isTagset variable set to true and it's not a field per se, a is_custom was added to the field spec of this field in order to display it. If this property is set to true, it will display the template for the field even if there are no form fields associated to the spec.

The field is displayed in a new row as opposed to how it was displayed before, in a 4th column, because the CSS grid was already fixed to 3 columns.

It's important to note that CSS grids were chosen over the previous table structure because the former is more flexible when removing a particular field. For example, if one would remove all the cells in a row, the row would still exist, occupying an empty space.

eileenmcnaughton · 2018-06-08T11:04:13Z

I think we are there on this. I've had a few looks at it & as of yesterday the items you have now addressed were the only outstanding ones. Let's merge -on-pass

eileenmcnaughton · 2018-06-08T11:24:48Z

@reneolivo jenkins failed on (one) style warning

By implementing the field spec pattern as detailed in dev/core#115 extension developers have more control over what is displayed in this template without needing to resort to overriding it

eileenmcnaughton · 2018-06-08T23:00:40Z

bang!

reneolivo · 2018-06-11T12:29:38Z

First try!

…nced search Included in CiviCRM 5.3.0 Core PR: civicrm#12078 Gitlab Issue: https://lab.civicrm.org/dev/core/issues/101

…nced search Included in CiviCRM 5.4.0 Core PR: civicrm#12078 Gitlab Issue: https://lab.civicrm.org/dev/core/issues/101

civicrm-builder added the master label May 3, 2018

reneolivo suggested changes May 3, 2018

View reviewed changes

reneolivo force-pushed the 101-conditionals-in-search-form branch 3 times, most recently from b4904b4 to 25ac93f Compare May 9, 2018 03:16

eileenmcnaughton reviewed May 11, 2018

View reviewed changes

eileenmcnaughton mentioned this pull request May 14, 2018

Permit UI editing of custom data on other entities (here relationship_type) when enabled in an extension #12128

Merged

reneolivo force-pushed the 101-conditionals-in-search-form branch 2 times, most recently from ebb4b2d to dbcbae0 Compare June 6, 2018 23:56

reneolivo force-pushed the 101-conditionals-in-search-form branch 2 times, most recently from 2a4718f to 1028e76 Compare June 8, 2018 09:50

eileenmcnaughton added merge on pass sig:extension support Supports being able to integrate extensions with CiviCRM sig:code maintenance readability testability labels Jun 8, 2018

dev/core#101 Implement field spec pattern for advanced search form.

a39762f

By implementing the field spec pattern as detailed in dev/core#115 extension developers have more control over what is displayed in this template without needing to resort to overriding it

reneolivo force-pushed the 101-conditionals-in-search-form branch from 1028e76 to a39762f Compare June 8, 2018 12:41

reneolivo mentioned this pull request Jun 8, 2018

PCHR-3563: Remove unused fields from contact advanced search compucorp/civihr#2678

Merged

eileenmcnaughton merged commit 3ffaaa3 into civicrm:master Jun 8, 2018

eileenmcnaughton mentioned this pull request Jun 9, 2018

CRM-21237: Activity search reset default values when using force=1 #11055

Closed

reneolivo deleted the 101-conditionals-in-search-form branch June 11, 2018 12:28

reneolivo mentioned this pull request Jun 11, 2018

PCHR-3563: Add field specs to the contact's advanced search compucorp/civicrm-core#25

Merged

eileenmcnaughton mentioned this pull request Jul 20, 2018

Shoreditch support: Fix following change in core that resulted in visual regression in shoreditch in advanced search #12524

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/core/#101: Allow further customization of search form in hooks #12078

dev/core/#101: Allow further customization of search form in hooks #12078

mickadoo commented May 3, 2018 •

edited

reneolivo commented May 3, 2018

reneolivo left a comment

reneolivo May 3, 2018

eileenmcnaughton commented May 3, 2018

michaelmcandrew commented May 4, 2018

reneolivo commented May 8, 2018

eileenmcnaughton commented May 8, 2018 •

edited

reneolivo commented May 9, 2018

eileenmcnaughton commented May 9, 2018

eileenmcnaughton commented May 11, 2018

eileenmcnaughton May 11, 2018

eileenmcnaughton commented May 11, 2018

JoeMurray commented May 16, 2018

reneolivo commented May 17, 2018

eileenmcnaughton commented May 17, 2018

reneolivo commented May 17, 2018

reneolivo commented May 18, 2018

eileenmcnaughton commented May 20, 2018

reneolivo commented Jun 7, 2018

eileenmcnaughton commented Jun 7, 2018

eileenmcnaughton commented Jun 7, 2018

eileenmcnaughton commented Jun 7, 2018

eileenmcnaughton commented Jun 7, 2018

eileenmcnaughton commented Jun 7, 2018 •

edited

reneolivo commented Jun 8, 2018

eileenmcnaughton commented Jun 8, 2018

eileenmcnaughton commented Jun 8, 2018

eileenmcnaughton commented Jun 8, 2018

reneolivo commented Jun 11, 2018

dev/core/#101: Allow further customization of search form in hooks #12078

dev/core/#101: Allow further customization of search form in hooks #12078

Conversation

mickadoo commented May 3, 2018 • edited

Overview

Before

What it looked like before:

After

What it looked like after (with no fields changed, should be the same as "Before")

What it looked like after with the following fields removed

Notes

Gitlab Issue

reneolivo commented May 3, 2018

reneolivo left a comment

Choose a reason for hiding this comment

Before

After

reneolivo May 3, 2018

Choose a reason for hiding this comment

eileenmcnaughton commented May 3, 2018

michaelmcandrew commented May 4, 2018

reneolivo commented May 8, 2018

eileenmcnaughton commented May 8, 2018 • edited

reneolivo commented May 9, 2018

After

Technical details

eileenmcnaughton commented May 9, 2018

eileenmcnaughton commented May 11, 2018

eileenmcnaughton May 11, 2018

Choose a reason for hiding this comment

eileenmcnaughton commented May 11, 2018

JoeMurray commented May 16, 2018

reneolivo commented May 17, 2018

eileenmcnaughton commented May 17, 2018

reneolivo commented May 17, 2018

reneolivo commented May 18, 2018

eileenmcnaughton commented May 20, 2018

reneolivo commented Jun 7, 2018

eileenmcnaughton commented Jun 7, 2018

eileenmcnaughton commented Jun 7, 2018

eileenmcnaughton commented Jun 7, 2018

eileenmcnaughton commented Jun 7, 2018

eileenmcnaughton commented Jun 7, 2018 • edited

reneolivo commented Jun 8, 2018

eileenmcnaughton commented Jun 8, 2018

eileenmcnaughton commented Jun 8, 2018

eileenmcnaughton commented Jun 8, 2018

reneolivo commented Jun 11, 2018

mickadoo commented May 3, 2018 •

edited

eileenmcnaughton commented May 8, 2018 •

edited

eileenmcnaughton commented Jun 7, 2018 •

edited