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

[coldrunKacper]

Overview

This is a supplement to https://issues.civicrm.org/jira/projects/CRM/issues/CRM-20630.

Before

When doing Activity Search with force=1 mode we are able to pass URL parameters into search form. However some of fields have their defaults set which are populated into the form in case we don't pass their values by URL parameters (for example 'activity_role' is set to "with" and 'activity_status' is set to "Scheduled" and "Completed" by default).

We don't want to populate default values when using force=1. We assume that there shouldn't be any of value set if it it is not specified in URL parameter.

After

We fix this issue implementing setDefaultValues() method in CRM_Activity_Form_Search class. This method is called in parent class (CRM_Core_Form::buildForm()) after form's default values are set. So we are able to reset them if force=1 mode is used.

Comments

Additionally, the PR adds 'contact_id' form's hidden field to allow passing Contact ID parameter to the form. It's handled automatically and allow searching for Activities of specified Contact. Previously the 'contact_id' parameter worked but didn't get passed further when re-submitting search form.

---

• CRM-21237: Activity Search: reset default values when using force=1
• CRM-20630: Find Activities: search criteria passing with URL parameters

[colemanw]

Interesting. I don't really understand how those defaults were getting set in the first place. Where are they being set? And instead of unsetting them here, could we just avoid setting them in the first place when fore=1?

[coldrunKacper]

Here are relevant steps of search flow:

1). Execute CRM_Core_Form::buildForm() method.

2). The CRM_Core_Form::buildForm() executes $form->preProcess() method (in our case it's CRM_Activity_Form_Search::preProcess()).
Here we handle URL parameters and pass them into the search values.

3). The CRM_Core_Form::buildForm() executes $form->buildQuickForm() metod (in our case it's CRM_Activity_Form_Search::buildQuickForm()).

4). The CRM_Activity_Form_Search::buildQuickForm() method executes CRM_Activity_BAO_Query::buildSearchForm() method
which sets some of the fields default values whether or not we pass the values by URL. This causes overriding of URL parameters with setDefaults() calls made inside CRM_Activity_BAO_Query::buildSearchForm().

5). The CRM_Core_Form::buildForm() executes checking defaults by calling $form->setDefaultValues() (in our case it's CRM_Activity_Form_Search::setDefaultValues()).

6). If setDefaultValues() method returns an array then it's used to set the form values to the array.
So here we are able to override the form values in case they were set in step 4).

So originally the defaults are set at 4) inside CRM_Activity_BAO_Query::buildSearchForm(), for particular form's field, for example:
$form->addRadio('activity_role', NULL, $activityRoles, array('allowClear' => TRUE));
$form->setDefaults(array('activity_role' => 3));

So I guess the only way to avoid setting them is to wrap setDefaults() for each field with "if (!$force)" check.

[coldrunKacper]

Or we could group all 'setDefaults()' parameters from CRM_Activity_BAO_Query::buildSearchForm() (https://github.com/civicrm/civicrm-core/blob/master/CRM/Activity/BAO/Query.php#L423) into single array and then call setDefaults() wrapped by "if (!$force)" at the bottom of CRM_Activity_BAO_Query::buildSearchForm() method.

[colemanw]

@civicrm-builder retest this please

[colemanw]

@coldrunKacper I agree, wrapping the default setting with if (!$force) would make the most sense.

[eileenmcnaughton]

@colemanw is this good to merge? Note that if the patch IS ok as submitted I think it should go in the parent class if possible. Seems like a generic search thing

[colemanw]

@coldrunKacper what do you think of my most recent suggestion of wrapping default settings in a conditional?

[eileenmcnaughton]

What about putting the code in a function on the parent (setForcedDefaults or setURLDefaults) & either moving the setDefaults to the parent or calling it from other functions that inherit from the parent. It seems generic enough that it could apply to all search classes.

OTOH is there any risk here (@totten ) I know we have discussed in the past whether a url could then be constructed that was in some way unsafe - perhaps sanitising the $values makes sense?

[eileenmcnaughton]

I'm actually deeply uncomfortable with this. I believe there was original discussion about setting which fields defaults were gettable for & it was deemed too hard to whitelist but on the basis it was not very safe to do all fields the conclusion was to do all fields for this search only. I don't want to extend that without revisiting the original decision - in the context of changes that have taken place since them - specifically a protocol to define the fields implemented by @reneolivo in #12078 and a proposal on how to extend that to get data from the url in
#12185

I'd like to close that & move instead to a combo of those approaches for this form

Ping @colemanw @guanhuan

[eileenmcnaughton]

I don't think this a safe way to do it - closing - per above I think we should use a metadata based approach