Activity tab code refactor #12557

eileenmcnaughton · 2018-07-25T02:56:04Z

Overview

Extract out the main part of getActivities so that getActivitiesCount can share the common parts without doing weird stuff

Before

Code winds itself in knots to be shared

After

Code has sensible points of being shared.

Technical Details

A while back @monishdeb refactored the function to get the count of activities associated with a contact to use the api eileenmcnaughton@466e3a5#diff-a35044abd0f538e4af551c5ae039692d

However we hit a performance snaffu - #10261 and we bypassed the new code in order to get the rc stabilised.

However since then @mattwire has worked on the performance of the new functions (#10909) and they have gotten better under most circumstances. WMF has switched over to the new functions as some contacts would no longer load under the old functions.

There are 2 new functions getActivities and getActivitiesCount. The former have also been switched over in core. The later still needs some work for permissioned users & hence this PR has been reduced to a refactor towards switching rather than the original switcharoo.

Comments

There is good test coverage on the new function as that was part of it's development.

I am continuing on with fixing up the api handling of permissions to the point where we can switch this across in core - current PR for that is #12995

Note I have updated this description to reflect the final PR - some of the comments reflect the original intent to do a switcharoo & were made before either function was switched

civibot · 2018-07-25T02:56:06Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

eileenmcnaughton · 2018-07-25T21:45:03Z

@seamuslee001 any chance you can test this & #12559 on AUG - under permissioned user preferably

monishdeb · 2018-07-26T08:39:16Z

@eileenmcnaughton @mattwire @davejenx I found a way to use Activity.getcount instead of iterate each activity record and exclude the civicase activities if the Case component is not enabled. I found this is the only reason why we are not able to call the getcount action which will further boost the performance if we can use it. I prepared a patch below, which consider api3('Activity', 'get', ['case_id' => ['IS NULL' => 1]]) works:

diff --git a/CRM/Activity/BAO/Activity.php b/CRM/Activity/BAO/Activity.php
index 61568a9..d044c9f 100644
--- a/CRM/Activity/BAO/Activity.php
+++ b/CRM/Activity/BAO/Activity.php
@@ -700,8 +700,6 @@ class CRM_Activity_BAO_Activity extends CRM_Activity_DAO_Activity {
         'subject',
         'activity_type_id',
         'activity_type',
-        'case_id',
-        'campaign_id',
       ),
       'check_permissions' => 1,
       'options' => array(
@@ -709,11 +707,20 @@ class CRM_Activity_BAO_Activity extends CRM_Activity_DAO_Activity {
       ),
     );
 
+    foreach (['case_id' => 'CiviCase', 'campaign_id' => 'CiviCampaign'] as $attr => $component) {
+      if (in_array($component, $enabledComponents)) {
+        $activityParams['return'][] = $attr;
+      }
+      else {
+        $activityParams[$attr] = ['IS NULL' => 1];
+      }
+    }
+
     if (!empty($params['activity_status_id'])) {
       $activityParams['activity_status_id'] = array('IN' => explode(',', $params['activity_status_id']));
     }

@@ -740,6 +747,10 @@ class CRM_Activity_BAO_Activity extends CRM_Activity_DAO_Activity {
     // 1. we should use Activity.Getcount for fetching count only, but  in order to check that
     //    current logged in user has permission to view Case activities we are performing filtering out those activities from list (see below).
     //    This logic need to be incorporated in Activity.get definition
+    if ($getCount) {
+      return civicrm_api3('Activity', 'getcount', $activityParams);
+    }
+
     $result = civicrm_api3('Activity', 'Get', $activityParams);
 
     $enabledComponents = self::activityComponents();
@@ -768,18 +779,8 @@ class CRM_Activity_BAO_Activity extends CRM_Activity_DAO_Activity {
     );
 
     foreach ($result['values'] as $id => $activity) {
-      // skip case activities if CiviCase is not enabled OR those actvities which are
-      if (!empty($activity['case_id']) && !in_array('CiviCase', $enabledComponents)) {
-        continue;
-      }
-
       $activities[$id] = array();
 
-      // if count is needed, no need to populate the array list with attributes
-      if ($getCount) {
-        continue;
-      }
-
       $isBulkActivity = (!$bulkActivityTypeID || ($bulkActivityTypeID != $activity['activity_type_id']));
       foreach ($mappingParams as $apiKey => $expectedName) {
         if (in_array($apiKey, array('assignee_contact_name', 'target_contact_name'))) {

eileenmcnaughton · 2018-07-26T09:19:00Z

Thanks @monishdeb looks good.

I think for separation of concerns rather than have getActivitiesCount call getActivities it should be lik

  public static function getActivitiesCount($input) {
  
    return civicrm_api3('Activity', 'getcount', self::getActivityParams($input));
  }

And we can share the code that derives those params.

Should I pull your patch into this PR?

monishdeb · 2018-07-26T09:23:14Z

I am not sure if there is any special case inside the foreach loop which excludes any activity (as I didn't found any). Apart from that it's ok to include the patch in this PR but I need assurance from others that the activity count and result don't differ.

eileenmcnaughton · 2018-07-26T09:23:53Z

@monishdeb do you prefer to leave it out & we can do as a follow up?

monishdeb · 2018-07-26T09:24:07Z

Yes sure

eileenmcnaughton · 2018-08-03T00:22:35Z

@monishdeb I have updated this with fixes similar to those you proposed. We deployed this & found that contacts with large numbers of records (e.g > 20000) couldn't render within our 30 sec time out (and in fact even after we doubled it some would not render).

monishdeb · 2018-08-03T06:43:25Z

@eileenmcnaughton, unfortunately, I cannot have Civi instance with large DB to do performance testing for this patch. But the code looks goods and doesn't cause any unintended regression

eileenmcnaughton · 2018-08-03T08:40:46Z

@monishdeb I realise there might be some issues in the permissions :-( The current api loads every activity & then iterates it to check permissions - but not really (now or with this patch) for getcount.

I think we should merge PR #12559 and I'll dig for a bit longer on this one.

I think we could probably filter the activity types by component before the query runs - ie. all of this part

    $activity = new CRM_Activity_DAO_Activity();
    $activity->id = $activityId;
    if (!$activity->find(TRUE)) {
      return $allow;
    }

    // Component related permissions.
    $compPermissions = array(
      'CiviCase' => array(
        'administer CiviCase',
        'access my cases and activities',
        'access all cases and activities',
      ),
      'CiviMail' => array('access CiviMail'),
      'CiviEvent' => array('access CiviEvent'),
      'CiviGrant' => array('access CiviGrant'),
      'CiviPledge' => array('access CiviPledge'),
      'CiviMember' => array('access CiviMember'),
      'CiviReport' => array('access CiviReport'),
      'CiviContribute' => array('access CiviContribute'),
      'CiviCampaign' => array('administer CiviCampaign'),
    );

    // Return early when it is case activity.
    $isCaseActivity = CRM_Case_BAO_Case::isCaseActivity($activityId);
    // Check for civicase related permission.
    if ($isCaseActivity) {
      $allow = FALSE;
      foreach ($compPermissions['CiviCase'] as $per) {
        if (CRM_Core_Permission::check($per)) {
          $allow = TRUE;
          break;
        }
      }

      // Check for case specific permissions.
      if ($allow) {
        $oper = 'view';
        if ($action == CRM_Core_Action::UPDATE) {
          $oper = 'edit';
        }
        $allow = CRM_Case_BAO_Case::checkPermission($activityId,
          $oper,
          $activity->activity_type_id
        );
      }

      return $allow;
    }

…o getActivitiesCount ditto for getActivities. This is a combination of 2 upstream PRs civicrm#12557 and civicrm#12559 Note - when trying to test use contact 72 - pretty sure at the moment the new code is on staging & old on live Bug: T199753 Bug: T191867 Change-Id: I1624b70eca431c018727618ebc7e8c35dc2121d4

Per civicrm#12557 Bug: T201008 Change-Id: I9694c6319f4491a48a8ce535ac4223b54b480437

…o getActivitiesCount ditto for getActivities. This is a combination of 2 upstream PRs civicrm#12557 and civicrm#12559 Note - when trying to test use contact 72 - pretty sure at the moment the new code is on staging & old on live Bug: T199753 Bug: T191867 Change-Id: I1624b70eca431c018727618ebc7e8c35dc2121d4

seamuslee001 · 2018-09-21T22:12:55Z

@eileenmcnaughton just pining given this is still open not sure what we should be doing here

eileenmcnaughton · 2018-10-18T04:10:09Z

@seamuslee001 I'm inclined to merge the extraction at this stage & pull out the 'switch over' commit - I want to do some more digging / testing on performance before the switch over (which we have in production & which helps us performance wise overall but we still get poor performance when one activity has a large number (e.g 30000) attached contacts. Also currently api calls with check_permissions + getcount fail hard. The non-switch over commit fixes that but I still want to dig some more into what is happening with permission checks (I believe they are duplicated)

If you are OK to merge I'll fix per ^^

seamuslee001 · 2018-10-18T05:47:39Z

Yep i think so

eileenmcnaughton · 2018-10-18T06:03:41Z

api/v3/Activity.php

@@ -341,6 +341,10 @@ function civicrm_api3_activity_get($params) {
  }

  $activities = _civicrm_api3_basic_get(_civicrm_api3_get_BAO(__FUNCTION__), $params, FALSE, 'Activity', $sql);
+  if ($options['is_count']) {


Note that this change takes us from a fatal error when we do getcount + check_permissions to a count that does not properly apply permissions. I think this is arguably better & the count doesn't seem to be highly sensitive info.

More importantly I think it's going to take a few steps to resolve this & I need to be able to get changes that don't break the tests.

The next steps to resolve this issue are

resolve https://lab.civicrm.org/dev/core/issues/454 and

get this extraction reviewed & merged [NFC] Extract case activity permission check. #12949 (I have a follow on for that commit still in the tidying realm)

update the extraction in 2 is merged - now this is the current PR #12995

Can we call Activity.getcount directly here?

this is for when getcount has been called - we have to return here as the next loop hard fails if we don't :-( I'm working to improve the next loop but need this to get tests running - it's moved up from 3 rows further down

eileenmcnaughton · 2018-10-18T06:05:10Z

@seamuslee001 ok done

eileenmcnaughton · 2018-10-25T04:52:04Z

@seamuslee001 @monishdeb I think this is mergeable now - I updated to pr template to reflect the reduced scope of this PR (ie. the function is getActivitiesCount function is still not used yet)

mattwire · 2018-10-29T20:43:42Z

CRM/Activity/BAO/Activity.php

+
+    $activityParams['activity_type_id'] = self::filterActivityTypes($params);
+    $enabledComponents = self::activityComponents();
+    // @todo - should we move this to activity get api.


Add comment // Exclude disabled components

mattwire · 2018-10-29T20:45:39Z

CRM/Activity/BAO/Activity.php

+      'case_id',
+      'campaign_id',
+    ];
+    foreach (['case_id' => 'CiviCase', 'campaign_id' => 'CiviCampaign'] as $attr => $component) {


As this array is used more than once in this class, it probably makes sense to split it out into a separate function?

mattwire · 2018-10-29T20:46:25Z

CRM/Activity/BAO/Activity.php

+      'activity_type',
+      'case_id',
+      'campaign_id',
+    ];


Yes. This is just moving the assignment of ['return'] to a bit further down.

mattwire · 2018-10-29T20:47:51Z

CRM/Activity/BAO/Activity.php

+   * @return array
+   */
+  protected static function getActivityParamsForDashboardFunctions($params) {
+    $activityParams = [


Yes. "Simple" function extraction with no changes

mattwire · 2018-10-29T20:51:55Z

api/v3/Activity.php

@@ -341,6 +341,10 @@ function civicrm_api3_activity_get($params) {
  }

  $activities = _civicrm_api3_basic_get(_civicrm_api3_get_BAO(__FUNCTION__), $params, FALSE, 'Activity', $sql);
+  if ($options['is_count']) {


Can we call Activity.getcount directly here?

seamuslee001 · 2018-11-14T22:06:49Z

Re re-reviewing, looks like Matt had in essence approved this had a couple of minor suggestions which i think can be dealt with in the next PR which shouldn't hold up this PR Merging based on Matt's review

eileenmcnaughton · 2018-11-14T22:11:10Z

thanks @seamuslee001

…o getActivitiesCount ditto for getActivities. This is a combination of 2 upstream PRs civicrm#12557 and civicrm#12559 Note - when trying to test use contact 72 - pretty sure at the moment the new code is on staging & old on live Bug: T199753 Bug: T191867 Change-Id: I1624b70eca431c018727618ebc7e8c35dc2121d4

This as a patch we had applied over 5.4 but it got missed in the re-cut of 5.6 - presumably because it was partially merged To test compare viewing contact 671068 on staging & on live upstream PR Per civicrm#12557 Note, it seems that the test contact has an activity that is very slow to render but it is not one of the 'first' activities. This patch stops activities not being displayed from being rendered (only show first 25). I'm digging into the slow activity still Bug: T201008 Bug: T204731 Change-Id: Iabb336153dac47d66b8ca7477b6678d0a4c93bb5

civicrm-builder added the master label Jul 25, 2018

eileenmcnaughton mentioned this pull request Jul 25, 2018

Activity display, switch to more performance getActivities function (from deprecatedGetActivities) #12559

Merged

eileenmcnaughton force-pushed the activity branch 2 times, most recently from 0d9b799 to 1d449f3 Compare August 3, 2018 00:21

eileenmcnaughton force-pushed the activity branch from 1d449f3 to a81d468 Compare August 3, 2018 03:47

eileenmcnaughton force-pushed the activity branch from a81d468 to 2c2abdb Compare August 19, 2018 22:14

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Sep 4, 2018

Port activity tab performance fix

bc4d87e

Per civicrm#12557 Bug: T201008 Change-Id: I9694c6319f4491a48a8ce535ac4223b54b480437

eileenmcnaughton force-pushed the activity branch from 2c2abdb to 8c9b1a7 Compare October 18, 2018 05:59

eileenmcnaughton commented Oct 18, 2018

View reviewed changes

Extract getcount for performance

84be264

eileenmcnaughton force-pushed the activity branch from 8c9b1a7 to 84be264 Compare October 25, 2018 04:40

eileenmcnaughton changed the title ~~Activity tab performance - switch from deprecatedGetActivitiesCount to getActivitiesCount~~ Activity tab code refactor Oct 25, 2018

mattwire requested changes Oct 29, 2018

View reviewed changes

seamuslee001 merged commit 945744e into civicrm:master Nov 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Activity tab code refactor #12557

Activity tab code refactor #12557

eileenmcnaughton commented Jul 25, 2018 •

edited

Loading

civibot bot commented Jul 25, 2018

eileenmcnaughton commented Jul 25, 2018

monishdeb commented Jul 26, 2018 •

edited

Loading

eileenmcnaughton commented Jul 26, 2018

monishdeb commented Jul 26, 2018 •

edited

Loading

eileenmcnaughton commented Jul 26, 2018

monishdeb commented Jul 26, 2018

eileenmcnaughton commented Aug 3, 2018

monishdeb commented Aug 3, 2018

eileenmcnaughton commented Aug 3, 2018

seamuslee001 commented Sep 21, 2018

eileenmcnaughton commented Oct 18, 2018

seamuslee001 commented Oct 18, 2018

eileenmcnaughton Oct 18, 2018

eileenmcnaughton Oct 25, 2018

mattwire Oct 29, 2018

eileenmcnaughton Oct 29, 2018

eileenmcnaughton commented Oct 18, 2018

eileenmcnaughton commented Oct 25, 2018

mattwire Oct 29, 2018

mattwire Oct 29, 2018

mattwire Oct 29, 2018

mattwire Oct 29, 2018

mattwire Oct 29, 2018

seamuslee001 commented Nov 14, 2018

eileenmcnaughton commented Nov 14, 2018

Activity tab code refactor #12557

Activity tab code refactor #12557

Conversation

eileenmcnaughton commented Jul 25, 2018 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Jul 25, 2018

eileenmcnaughton commented Jul 25, 2018

monishdeb commented Jul 26, 2018 • edited Loading

eileenmcnaughton commented Jul 26, 2018

monishdeb commented Jul 26, 2018 • edited Loading

eileenmcnaughton commented Jul 26, 2018

monishdeb commented Jul 26, 2018

eileenmcnaughton commented Aug 3, 2018

monishdeb commented Aug 3, 2018

eileenmcnaughton commented Aug 3, 2018

seamuslee001 commented Sep 21, 2018

eileenmcnaughton commented Oct 18, 2018

seamuslee001 commented Oct 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

eileenmcnaughton commented Oct 18, 2018

eileenmcnaughton commented Oct 25, 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

seamuslee001 commented Nov 14, 2018

eileenmcnaughton commented Nov 14, 2018

eileenmcnaughton commented Jul 25, 2018 •

edited

Loading

monishdeb commented Jul 26, 2018 •

edited

Loading

monishdeb commented Jul 26, 2018 •

edited

Loading