CRM-21720 Cleanup search classes to use enumerators instead of hardcoded values #11600

mattwire · 2018-01-28T15:44:13Z

Overview

Follow on from CRM-21391 to tidy up code for search classes (specifically in relation to search tasks)

eileenmcnaughton · 2018-01-28T20:25:27Z

CRM/Contact/Form/Search.php

@@ -326,25 +337,15 @@ public static function getModeSelect() {
   */
  public function buildTaskList() {
    if ($this->_context !== 'amtg') {


If you figure out what amtg stands for & when it arises please comment the code. My best guess at the moment is that it is 'add member to group'

@eileenmcnaughton i think your right as per

civicrm-core/CRM/Contact/Form/Search.php

Line 180 in a4aae59

'amtg' => 'Add members to group',

Yes, the search classes have a few of these strings - see https://github.com/civicrm/civicrm-core/blob/master/CRM/Contact/Form/Search.php#L176

Anyway, I've inserted a comment just above that line

eileenmcnaughton · 2018-01-28T20:35:03Z

(CiviCRM Review Template WORD-1.0)

(r-jira) PASS
(r-test) Undecided
(r-code) PASS - clear improvement. Thanks!
(r-doc) Pass no user facing change
(r-maint) PASS code manageablily improved
(r-run) Undecided:
(r-user) PASS: no user facing change
(r-tech) PASS: code cleanup

eileenmcnaughton · 2018-01-28T20:47:15Z

CRM/Contact/Form/Search.php


    if (isset($this->_ssID)) {
-      if ($permission == CRM_Core_Permission::EDIT) {
-        $tasks = $tasks + CRM_Contact_Task::optionalTaskTitle();


there are still 2 places where optionalTaskTitle() is referred to that can go

I think those are removed via #11764

eileenmcnaughton · 2018-01-28T20:47:55Z

CRM/Contact/Form/Search.php

@@ -464,7 +452,7 @@ public function buildQuickForm() {
          'class' => 'crm-form-submit',
        )
      );
-      $this->add('hidden', 'task', CRM_Contact_Task::GROUP_CONTACTS);
+      $this->add('hidden', 'task', CRM_Contact_Task::GROUP_ADD);


I'm still reading but GROUP_ADD doesn't exist on my version that I can see

It exists once #11240 is merged

It is now in CRM/Core/Task.php which is part of core.

eileenmcnaughton · 2018-01-28T20:54:40Z

On contact search I just found the tasks missing -
before

after

Also note that they are not alphabetically sorted - I noticed an 'asort' was removed

Some thoughts about things to verify before merge

count of tasks - on master with a basic install I get 22 tasks before hand on contact search
alpha sorting of tasks
check for presence of 'update smart group' when in smart group context (viewing contacts in an existing smart group)
specifically check the Add to group works. The constant has been changed here and I'm not sure about it.
test creating a group & saving with different group types (set/ blank)

eileenmcnaughton · 2018-01-28T20:55:07Z

CRM/Contact/Form/Task/SaveSearch.php

-    if (isset($formValues['group_type']) &&
-      is_array($formValues['group_type'])
-    ) {
+    if (isset($formValues['group_type']) && is_array($formValues['group_type']) && count($formValues['group_type'])) {


I think this is an unrelated change. It makes sense though from what I can see - if there is no group_type but it is set & is an array save as 'no group type'

Yes, it fixes a PHP notice as the code was not checking for an empty array, but looking at it I believe it intended to as it doesn't make sense otherwise. It's pulling a group type from the form into a variable, if there isn't one it can't do that so sets an empty string in the else below.

eileenmcnaughton · 2018-01-28T20:55:56Z

CRM/Contact/StateMachine/Search.php

@@ -103,15 +103,10 @@ public function taskName($controller, $formName = 'Search') {
    }
    $this->_controller->set('task', $value);

-    if ($value) {


Any idea how to get to this line without $value to test?

This was causing a bug in some situations, as it was falling back to the contact task and would try and return the default task for contact tasks no matter what context we were in (eg. in member context). I think you could trigger it by editing a smart search and changing the "View results as" to be something other than contacts. But it should not be possible to get there with no value for $value unless you are doing something wierd and it doesn't make sense (in my opinion) to fall back to the contact task handler, as it defines a different set of tasks which are potentially not applicable to the current context.

By removing the check for $value we are always calling the relevant getTask() handler for the context.

mattwire · 2018-01-29T01:35:29Z

@eileenmcnaughton What I failed to mention was that this should be merged AFTER #11240 as it relies on changes in that PR (eg. GROUP_ADD is defined in CRM_Core_Task). Sorry!

eileenmcnaughton · 2018-03-11T21:32:24Z

Per comments on #11764 this works in conjunction with that so merging both

civicrm-builder added the master label Jan 28, 2018

mattwire mentioned this pull request Jan 28, 2018

CRM-21391 Refactor form search to use a base class, resolve issues with action … #11240

Closed

eileenmcnaughton reviewed Jan 28, 2018

View reviewed changes

mattwire force-pushed the CRM-21720_core_task_search_classes branch from fb13128 to 9d1ed2c Compare January 29, 2018 01:41

mattwire mentioned this pull request Mar 5, 2018

CRM-21391 Convert Contact to use core Task class #11764

Merged

Cleanup search classes to use enumerators instead of hardcoded values

eda34f9

mattwire force-pushed the CRM-21720_core_task_search_classes branch from 9d1ed2c to eda34f9 Compare March 5, 2018 12:33

eileenmcnaughton merged commit 7dd2478 into civicrm:master Mar 11, 2018

eileenmcnaughton mentioned this pull request Apr 19, 2018

core/issues/64 - In custom searches, column headings are being ignored #12001

Merged

mattwire deleted the CRM-21720_core_task_search_classes branch September 25, 2018 11:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-21720 Cleanup search classes to use enumerators instead of hardcoded values #11600

CRM-21720 Cleanup search classes to use enumerators instead of hardcoded values #11600

mattwire commented Jan 28, 2018 •

edited by civicrm-builder

Loading

eileenmcnaughton Jan 28, 2018

seamuslee001 Jan 28, 2018

mattwire Jan 29, 2018

mattwire Jan 29, 2018

eileenmcnaughton commented Jan 28, 2018 •

edited

Loading

eileenmcnaughton Jan 28, 2018

mattwire Mar 5, 2018

eileenmcnaughton Jan 28, 2018

mattwire Jan 29, 2018

mattwire Mar 5, 2018

eileenmcnaughton commented Jan 28, 2018

eileenmcnaughton Jan 28, 2018

mattwire Mar 5, 2018

eileenmcnaughton Jan 28, 2018

mattwire Mar 5, 2018

mattwire commented Jan 29, 2018

eileenmcnaughton commented Mar 11, 2018

CRM-21720 Cleanup search classes to use enumerators instead of hardcoded values #11600

CRM-21720 Cleanup search classes to use enumerators instead of hardcoded values #11600

Conversation

mattwire commented Jan 28, 2018 • edited by civicrm-builder Loading

Overview

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Jan 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Jan 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattwire commented Jan 29, 2018

eileenmcnaughton commented Mar 11, 2018

mattwire commented Jan 28, 2018 •

edited by civicrm-builder

Loading

eileenmcnaughton commented Jan 28, 2018 •

edited

Loading