CRM-21111 Refactor include/exclude activity types into it's own function and im… #10909

mattwire · 2017-08-28T12:46:18Z

…prove performance (don't retrieve excluded activity types)

Before

API retrieved all activities, excluded activity types were removed afterwards.

After

API doesn't retrieve excluded activity types.

Technical Details

Extract activity type filtering to a new function filterActivityTypes();

Comments

The getActivities function is not actually in use anywhere yet so should be a relatively safe merge.

mattwire · 2018-05-18T21:11:20Z

test this please

eileenmcnaughton · 2018-05-21T09:03:18Z

CRM/Activity/BAO/Activity.php

-    }
-    elseif (!empty($activityTypes) && count($activityTypes)) {
-      $activityParams['activity_type_id'] = array('IN' => array_keys($activityTypes));
+    if ($params['context'] != 'activity') {


not sure where $params['context'] came in from - original line is

- if (!empty($params['activity_status_id'])) { - $activityParams['activity_status_id'] = array('IN' => explode(',', $params['activity_status_id'])); - }

Now I'm not sure either. I've changed it back to the original.

eileenmcnaughton · 2018-05-21T09:06:12Z

CRM/Activity/BAO/Activity.php

@@ -840,10 +808,10 @@ public static function getActivities($params, $getCount = FALSE) {
        else {
          $activities[$id][$expectedName] = CRM_Utils_Array::value($apiKey, $activity);
          if ($apiKey == 'activity_type_id') {
-            $activities[$id]['activity_type'] = CRM_Utils_Array::value($activities[$id][$expectedName], $activityTypes);
+            $activities[$id]['activity_type'] = CRM_Core_PseudoConstant::getName(__CLASS__, 'activity_type_id', $activities[$id][$expectedName]);


I think it's better to put in the class name in case the function ever gets moved

Ok, changed all instances

eileenmcnaughton · 2018-05-21T09:07:19Z

CRM/Activity/BAO/Activity.php

          }
          elseif ($apiKey == 'campaign_id') {
-            $activities[$id]['campaign'] = CRM_Utils_Array::value($activities[$id][$expectedName], $allCampaigns);


I actually feel a bit nervous about this line - as an unrelated change I feel like I have to work quite hard to validate it or let it skate through (& the latter is not good)

Ok, changed back to original.

eileenmcnaughton · 2018-05-21T09:09:46Z

CRM/Activity/BAO/Activity.php

+      if (!empty($activityTypes) && count($activityTypes)) {
+        return array('IN' => array_keys($activityTypes));
+      }
+      return NULL;


So this would only be hit if there were NO enabled activity types configured for the site at all? (in which case only inactive ones would be displayed)

Pretty unlikely to hit NULL yes. As the return parameter is used as an API parameter it would return no activity types.

eileenmcnaughton · 2018-05-21T09:10:16Z

CRM/Activity/BAO/Activity.php

+
+    // If no activity types are specified or excluded, return the list of all active ones
+    if (empty($params['activity_type_id']) && empty($params['activity_type_exclude_id'])) {
+      if (!empty($activityTypes) && count($activityTypes)) {


I think !empty is enough without count

eileenmcnaughton · 2018-05-21T09:16:30Z

@mattwire I took a look at this & I get what you are doing and I think it makes sense. I checked and this function has heavy test coverage - in fact it is currently ONLY reachable from the unit tests. The function has / had performance issues so we switched back to the deprecated function.

Given the above I'm happy to merge this if you can respond to the in-line comments. At least we will be in a better place if we later re-enable

mattwire

@eileenmcnaughton Let me know if you are happy with the changes and I'll squash the commits

mattwire · 2018-05-27T14:29:29Z

CRM/Activity/BAO/Activity.php

+      if (!empty($activityTypes) && count($activityTypes)) {
+        return array('IN' => array_keys($activityTypes));
+      }
+      return NULL;


Pretty unlikely to hit NULL yes. As the return parameter is used as an API parameter it would return no activity types.

mattwire · 2018-05-27T14:29:31Z

CRM/Activity/BAO/Activity.php

+
+    // If no activity types are specified or excluded, return the list of all active ones
+    if (empty($params['activity_type_id']) && empty($params['activity_type_exclude_id'])) {
+      if (!empty($activityTypes) && count($activityTypes)) {


mattwire · 2018-05-27T14:29:34Z

CRM/Activity/BAO/Activity.php

          }
          elseif ($apiKey == 'campaign_id') {
-            $activities[$id]['campaign'] = CRM_Utils_Array::value($activities[$id][$expectedName], $allCampaigns);


Ok, changed back to original.

mattwire · 2018-05-27T14:29:35Z

CRM/Activity/BAO/Activity.php

@@ -840,10 +808,10 @@ public static function getActivities($params, $getCount = FALSE) {
        else {
          $activities[$id][$expectedName] = CRM_Utils_Array::value($apiKey, $activity);
          if ($apiKey == 'activity_type_id') {
-            $activities[$id]['activity_type'] = CRM_Utils_Array::value($activities[$id][$expectedName], $activityTypes);
+            $activities[$id]['activity_type'] = CRM_Core_PseudoConstant::getName(__CLASS__, 'activity_type_id', $activities[$id][$expectedName]);


Ok, changed all instances

mattwire · 2018-05-27T14:29:38Z

CRM/Activity/BAO/Activity.php

-    }
-    elseif (!empty($activityTypes) && count($activityTypes)) {
-      $activityParams['activity_type_id'] = array('IN' => array_keys($activityTypes));
+    if ($params['context'] != 'activity') {


Now I'm not sure either. I've changed it back to the original.

eileenmcnaughton · 2018-05-27T22:40:13Z

@mattwire sure - please squash & cheer up jenkins re e-notice & we can merge

…prove performance (don't retrieve excluded activity types)

mattwire · 2018-05-28T21:22:27Z

@eileenmcnaughton Squashed, pending tests

mattwire mentioned this pull request Aug 28, 2017

CRM-21065 Replace some CRM_Core_OptionGroup::values with CRM_Activity_BAO_Activ… #10860

Merged

mattwire force-pushed the CRM-21111_BAO_Activity_getActivities_cleanup branch from 30f54e9 to ebf3dd1 Compare August 28, 2017 17:28

totten added the master label Aug 30, 2017

mattwire force-pushed the CRM-21111_BAO_Activity_getActivities_cleanup branch from ebf3dd1 to fa4acd0 Compare March 16, 2018 16:58

eileenmcnaughton reviewed May 21, 2018

View reviewed changes

mattwire commented May 27, 2018

View reviewed changes

mattwire force-pushed the CRM-21111_BAO_Activity_getActivities_cleanup branch from fa4acd0 to cddf679 Compare May 27, 2018 14:30

Refactor include/exclude activity types into it's own function and im…

5b22d1b

…prove performance (don't retrieve excluded activity types)

mattwire force-pushed the CRM-21111_BAO_Activity_getActivities_cleanup branch from cddf679 to 5b22d1b Compare May 28, 2018 21:22

eileenmcnaughton added the merge on pass label May 28, 2018

eileenmcnaughton merged commit 3832b5e into civicrm:master May 28, 2018

eileenmcnaughton mentioned this pull request Jul 25, 2018

Activity tab code refactor #12557

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-21111 Refactor include/exclude activity types into it's own function and im… #10909

CRM-21111 Refactor include/exclude activity types into it's own function and im… #10909

mattwire commented Aug 28, 2017 •

edited

Loading

mattwire commented May 18, 2018

eileenmcnaughton May 21, 2018

mattwire May 27, 2018

eileenmcnaughton May 21, 2018

mattwire May 27, 2018

eileenmcnaughton May 21, 2018

mattwire May 27, 2018

eileenmcnaughton May 21, 2018

mattwire May 27, 2018

eileenmcnaughton May 21, 2018

mattwire May 27, 2018

eileenmcnaughton commented May 21, 2018

mattwire left a comment

mattwire May 27, 2018

mattwire May 27, 2018

mattwire May 27, 2018

mattwire May 27, 2018

mattwire May 27, 2018

eileenmcnaughton commented May 27, 2018

mattwire commented May 28, 2018

CRM-21111 Refactor include/exclude activity types into it's own function and im… #10909

CRM-21111 Refactor include/exclude activity types into it's own function and im… #10909

Conversation

mattwire commented Aug 28, 2017 • edited Loading

Before

After

Technical Details

Comments

mattwire commented May 18, 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

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

Choose a reason for hiding this comment

eileenmcnaughton commented May 21, 2018

mattwire left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented May 27, 2018

mattwire commented May 28, 2018

mattwire commented Aug 28, 2017 •

edited

Loading