From f3b5936071ec32eda96547e3e3dcb186a7c7a495 Mon Sep 17 00:00:00 2001
From: eileen <emcnaughton@wikimedia.org>
Date: Mon, 31 Dec 2018 11:33:12 +1300
Subject: [PATCH] [REF] simplify CRM_Activity_BAO_Activity function by using
 early returns.

This function returns a bool. In the process it sets the  variable, however, once set to false in two places
it can never be set to true, so I am simply replacing a big if block with an early return to simplify the code.

This is a further block that can go but it is a slightly different patter & would remove 2 levels of 'if'
so it feels like an easier review if I leave that out of scope
---
 CRM/Activity/BAO/Activity.php | 57 ++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 28 deletions(-)

diff --git a/CRM/Activity/BAO/Activity.php b/CRM/Activity/BAO/Activity.php
index b890a99c3e32..34996dfe27e4 100644
--- a/CRM/Activity/BAO/Activity.php
+++ b/CRM/Activity/BAO/Activity.php
@@ -2700,7 +2700,9 @@ public static function checkPermission($activityId, $action) {
     }
 
     // Component related permissions.
-    $allow = self::hasPermissionForActivityType($activity->activity_type_id);
+    if (!self::hasPermissionForActivityType($activity->activity_type_id)) {
+      return FALSE;
+    }
 
     // Check for this permission related to contact.
     $permission = CRM_Core_Permission::VIEW;
@@ -2714,43 +2716,42 @@ public static function checkPermission($activityId, $action) {
     $targetID = CRM_Utils_Array::key('Activity Targets', $activityContacts);
 
     // Check for source contact.
-    if ($allow) {
-      $sourceContactId = self::getActivityContact($activity->id, $sourceID);
-      // Account for possibility of activity not having a source contact (as it may have been deleted).
-      $allow = $sourceContactId ? CRM_Contact_BAO_Contact_Permission::allow($sourceContactId, $permission) : TRUE;
+    $sourceContactId = self::getActivityContact($activity->id, $sourceID);
+    // Account for possibility of activity not having a source contact (as it may have been deleted).
+    $allow = $sourceContactId ? CRM_Contact_BAO_Contact_Permission::allow($sourceContactId, $permission) : TRUE;
+    if (!$allow) {
+      return FALSE;
     }
 
     // Check for target and assignee contacts.
-    if ($allow) {
-      // First check for supper permission.
-      $supPermission = 'view all contacts';
-      if ($action == CRM_Core_Action::UPDATE) {
-        $supPermission = 'edit all contacts';
+    // First check for supper permission.
+    $supPermission = 'view all contacts';
+    if ($action == CRM_Core_Action::UPDATE) {
+      $supPermission = 'edit all contacts';
+    }
+    $allow = CRM_Core_Permission::check($supPermission);
+
+    // User might have sufficient permission, through acls.
+    if (!$allow) {
+      $allow = TRUE;
+      // Get the target contacts.
+      $targetContacts = CRM_Activity_BAO_ActivityContact::retrieveContactIdsByActivityId($activity->id, $targetID);
+      foreach ($targetContacts as $cnt => $contactId) {
+        if (!CRM_Contact_BAO_Contact_Permission::allow($contactId, $permission)) {
+          $allow = FALSE;
+          break;
+        }
       }
-      $allow = CRM_Core_Permission::check($supPermission);
 
-      // User might have sufficient permission, through acls.
-      if (!$allow) {
-        $allow = TRUE;
-        // Get the target contacts.
-        $targetContacts = CRM_Activity_BAO_ActivityContact::retrieveContactIdsByActivityId($activity->id, $targetID);
-        foreach ($targetContacts as $cnt => $contactId) {
+      // Get the assignee contacts.
+      if ($allow) {
+        $assigneeContacts = CRM_Activity_BAO_ActivityContact::retrieveContactIdsByActivityId($activity->id, $assigneeID);
+        foreach ($assigneeContacts as $cnt => $contactId) {
           if (!CRM_Contact_BAO_Contact_Permission::allow($contactId, $permission)) {
             $allow = FALSE;
             break;
           }
         }
-
-        // Get the assignee contacts.
-        if ($allow) {
-          $assigneeContacts = CRM_Activity_BAO_ActivityContact::retrieveContactIdsByActivityId($activity->id, $assigneeID);
-          foreach ($assigneeContacts as $cnt => $contactId) {
-            if (!CRM_Contact_BAO_Contact_Permission::allow($contactId, $permission)) {
-              $allow = FALSE;
-              break;
-            }
-          }
-        }
       }
     }