Merge pull request civicrm#27496 from eileenmcnaughton/activity_view

Fix activity view bug where an activity type id in the url overrides …

CRM/Activity/Form/Activity.php

@@ -20,6 +20,8 @@
  */
 class CRM_Activity_Form_Activity extends CRM_Contact_Form_Task {
 
+  use CRM_Activity_Form_ActivityFormTrait;
+
   /**
    * The id of the object being edited / created
    *
@@ -65,7 +67,6 @@ class CRM_Activity_Form_Activity extends CRM_Contact_Form_Task {
    */
   protected $_sourceContactId;
   protected $_targetContactId;
-  protected $_asigneeContactId;
 
   protected $_single;
 
@@ -266,32 +267,18 @@ public function preProcess() {
     $this->assign('context', $this->_context);
 
     $this->_action = CRM_Utils_Request::retrieve('action', 'String', $this);
-
-    if ($this->_action & CRM_Core_Action::DELETE) {
+    if ($this->getAction() & CRM_Core_Action::DELETE) {
       if (!CRM_Core_Permission::check('delete activities')) {
         CRM_Core_Error::statusBounce(ts('You do not have permission to access this page.'));
       }
     }
 
-    // CRM-6957
-    // When we come from contact search, activity id never comes.
-    // So don't try to get from object, it might gives you wrong one.
-
-    // if we're not adding new one, there must be an id to
-    // an activity we're trying to work on.
-    if ($this->_action != CRM_Core_Action::ADD &&
-      get_class($this->controller) !== 'CRM_Contact_Controller_Search'
-    ) {
-      $this->_activityId = CRM_Utils_Request::retrieve('id', 'Positive', $this);
-    }
-
-    $this->_activityTypeId = CRM_Utils_Request::retrieve('atype', 'Positive', $this);
+    $this->_activityTypeId = $this->getActivityValue('activity_type_id') ?: CRM_Utils_Request::retrieve('atype', 'Positive', $this);
     $this->assign('atype', $this->_activityTypeId);
-
     $this->assign('activityId', $this->_activityId);
 
     // Check for required permissions, CRM-6264.
-    if ($this->_activityId &&
+    if ($this->getActivityID() &&
       in_array($this->_action, [CRM_Core_Action::UPDATE, CRM_Core_Action::VIEW]) &&
       !CRM_Activity_BAO_Activity::checkPermission($this->_activityId, $this->_action)
     ) {
@@ -304,13 +291,6 @@ public function preProcess() {
       $this->assign('allow_edit_inbound_emails', CRM_Activity_BAO_Activity::checkEditInboundEmailsPermissions());
     }
 
-    if (!$this->_activityTypeId && $this->_activityId) {
-      $this->_activityTypeId = CRM_Core_DAO::getFieldValue('CRM_Activity_DAO_Activity',
-        $this->_activityId,
-        'activity_type_id'
-      );
-    }
-
     $this->assignActivityType();
 
     // Check the mode when this form is called either single or as
@@ -373,8 +353,10 @@ public function preProcess() {
       [$this->_activityTypeName, $activityTypeDescription] = CRM_Core_BAO_OptionValue::getActivityTypeDetails($this->_activityTypeId);
     }
 
-    // Set activity type name and description to template.
-    $this->assign('activityTypeName', $this->_activityTypeName ?? FALSE);
+    // Set activity type name and description to template. Type should no longer be used anywhere
+    // except the case_activity workflow template - unsure how to test that... We want to remove
+    // it due to mis-naming of the variable. The workflow template can use a token...
+    $this->assign('activityTypeName', $this->_activityTypeName);
     $this->assign('activityTypeDescription', $activityTypeDescription ?? FALSE);
 
     // set user context
@@ -1263,8 +1245,29 @@ public function assignActivityType() {
         }
       }
     }
-
     $this->assign('activityTypeNameAndLabel', $activityTypeNameAndLabel);
   }
 
+  /**
+   * Get the activity ID.
+   *
+   * @api This function will not change in a minor release and is supported for
+   * use outside of core. This annotation / external support for properties
+   * is only given where there is specific test cover.
+   *
+   * @noinspection PhpUnhandledExceptionInspection
+   */
+  public function getActivityID(): ?int {
+    // CRM-6957
+    // When we come from contact search, activity id never comes.
+    // So don't try to get from object, it might gives you wrong one.
+    if ($this->controller instanceof \CRM_Contact_Controller_Search) {
+      return NULL;
+    }
+    if (!isset($this->_activityId) && $this->_action != CRM_Core_Action::ADD) {
+      $this->_activityId = CRM_Utils_Request::retrieve('id', 'Positive', $this);
+    }
+    return $this->_activityId ? (int) $this->_activityId : FALSE;
+  }
+
 }

CRM/Activity/Form/ActivityFormTrait.php

@@ -0,0 +1,51 @@
+<?php
+
+use Civi\API\EntityLookupTrait;
+
+/**
+ * Trait implements functions to retrieve activity related values.
+ */
+trait CRM_Activity_Form_ActivityFormTrait {
+
+  use EntityLookupTrait;
+
+  /**
+   * Get the value for a field relating to the activity.
+   *
+   * All values returned in apiv4 format. Escaping may be required.
+   *
+   * @api This function will not change in a minor release and is supported for
+   * use outside of core. This annotation / external support for properties
+   * is only given where there is specific test cover.
+   *
+   * @param string $fieldName
+   *
+   * @return mixed
+   * @throws \CRM_Core_Exception
+   */
+  public function getActivityValue(string $fieldName) {
+    if ($this->isDefined('Activity')) {
+      return $this->lookup('Activity', $fieldName);
+    }
+    $id = $this->getActivityID();
+    if ($id) {
+      $this->define('Activity', 'Activity', ['id' => $id]);
+      return $this->lookup('Activity', $fieldName);
+    }
+    return NULL;
+  }
+
+  /**
+   * Get the selected Activity ID.
+   *
+   * @api This function will not change in a minor release and is supported for
+   * use outside of core. This annotation / external support for properties
+   * is only given where there is specific test cover.
+   *
+   * @noinspection PhpUnhandledExceptionInspection
+   */
+  public function getActivityID(): ?int {
+    throw new CRM_Core_Exception('`getActivityID` must be implemented');
+  }
+
+}

Civi/Test/FormWrapper.php

@@ -107,6 +107,7 @@ public function __construct(string $formName, array $formValues = [], array $url
    * @return \Civi\Test\FormWrapper
    */
   public function processForm(int $state = self::SUBMITTED): self {
+    \CRM_Core_Smarty::singleton()->pushScope([]);
     if ($state > self::CONSTRUCTED) {
       $this->form->preProcess();
     }
@@ -119,7 +120,8 @@ public function processForm(int $state = self::SUBMITTED): self {
     if ($state > self::VALIDATED) {
       $this->postProcess();
     }
-    $this->templateVariables = $this->form->get_template_vars();
+    $this->templateVariables = \CRM_Core_Smarty::singleton()->get_template_vars();
+    \CRM_Core_Smarty::singleton()->popScope([]);
     return $this;
   }
 
@@ -161,7 +163,7 @@ public function addSubsequentForm(string $formName, array $formValues = []): For
    */
   public function __call(string $name, array $arguments) {
     if (!empty(ReflectionUtils::getCodeDocs((new \ReflectionMethod($this->form, $name)), 'Method')['api'])) {
-      return call_user_func([$this->form, $name], $arguments);
+      return call_user_func_array([$this->form, $name], $arguments);
     }
     throw new \CRM_Core_Exception($name . ' method not supported for external use');
   }
@@ -377,4 +379,16 @@ public function getDeprecatedProperty(string $property) {
     throw new \CRM_Core_Exception('Deprecation should have been triggered');
   }
 
+  /**
+   * @param string $name
+   * @param mixed $value
+   *
+   * @throws \CRM_Core_Exception
+   */
+  public function checkTemplateVariable(string $name, $value): void {
+    if ($this->templateVariables[$name] !== $value) {
+      throw new \CRM_Core_Exception("Template variable $name not set to " . print_r($value, TRUE) . ' actual value: ' . print_r($this->templateVariables[$name], TRUE));
+    }
+  }
+
 }

templates/CRM/Activity/Form/Activity.tpl

@@ -12,7 +12,7 @@
     <div class="crm-block crm-content-block crm-activity-view-block">
   {else}
     {if $activityTypeDescription}
-      <div class="help">{$activityTypeDescription}</div>
+      <div class="help">{$activityTypeDescription|purify}</div>
     {/if}
     <div class="crm-block crm-form-block crm-activity-form-block">
   {/if}
@@ -29,7 +29,7 @@
 
   {if $action eq 4}
     {if $activityTypeDescription}
-    <div class="help">{$activityTypeDescription}</div>
+    <div class="help">{$activityTypeDescription|purify}</div>
     {/if}
   {else}
     {if $context eq 'standalone' or $context eq 'search' or $context eq 'smog'}

templates/CRM/Activity/Form/ActivityView.tpl

@@ -9,7 +9,7 @@
 *}
 <div class="crm-block crm-content-block crm-activity-view-block">
       {if $activityTypeDescription}
-        <div class="help">{$activityTypeDescription}</div>
+        <div class="help">{$activityTypeDescription|purify}</div>
       {/if}
       <table class="crm-info-panel">
         <tr>

templates/CRM/Case/Form/Activity.tpl

@@ -23,7 +23,7 @@
   <table class="form-layout">
     {if $activityTypeDescription}
       <tr>
-        <div class="help">{$activityTypeDescription}</div>
+        <div class="help">{$activityTypeDescription|purify}</div>
       </tr>
     {/if}
     {* Block for change status, case type and start date. *}

templates/CRM/Case/Form/Case.tpl

@@ -27,7 +27,7 @@
 <table class="form-layout">
     {if $activityTypeDescription}
         <tr>
-            <div class="help">{$activityTypeDescription}</div>
+            <div class="help">{$activityTypeDescription|purify}</div>
         </tr>
     {/if}
 {if $clientName}

tests/phpunit/CRM/Activity/Form/ActivityTest.php

@@ -1,5 +1,8 @@
 <?php
 
+use Civi\Api4\OptionValue;
+use Civi\Test\FormTrait;
+use Civi\Test\FormWrapper;
 use Civi\Test\Invasive;
 
 /**
@@ -8,6 +11,8 @@
  */
 class CRM_Activity_Form_ActivityTest extends CiviUnitTestCase {
 
+  use FormTrait;
+
   protected $assignee1;
   protected $assignee2;
   protected $target;
@@ -16,19 +21,27 @@ class CRM_Activity_Form_ActivityTest extends CiviUnitTestCase {
   public function setUp():void {
     parent::setUp();
     $this->assignee1 = $this->individualCreate([
-      'first_name' => 'testassignee1',
-      'last_name' => 'testassignee1',
-      'email' => 'testassignee1@gmail.com',
+      'first_name' => 'test_assignee1',
+      'last_name' => 'test_assignee1',
+      'email' => 'test_assignee1@gmail.com',
     ]);
     $this->assignee2 = $this->individualCreate([
-      'first_name' => 'testassignee2',
-      'last_name' => 'testassignee2',
+      'first_name' => 'test_assignee2',
+      'last_name' => 'test_assignee2',
       'email' => 'testassignee2@gmail.com',
     ]);
     $this->target = $this->individualCreate();
     $this->source = $this->individualCreate();
   }
 
+  public function tearDown(): void {
+    if (!empty($this->ids['OptionValue'])) {
+      OptionValue::delete(FALSE)->addWhere('id', 'IN', $this->ids['OptionValue'])->execute();
+      unset($this->ids['OptionValue']);
+    }
+    parent::tearDown();
+  }
+
   public function testActivityCreate(): void {
     Civi::settings()->set('activity_assignee_notification', TRUE);
     //Reset filter to none.
@@ -240,34 +253,27 @@ private function deleteActivity($activityId, $mode = NULL) {
   }
 
   /**
-   * This is a bit messed up having a variable called name that means label but we don't want to fix it because it's a form member variable _activityTypeName that might be used in form hooks, so just make sure it doesn't flip between name and label. dev/core#1116
+   * Test that the correct variables are assigned for the activity type.
+   *
+   * Sadly for historial reasons this means 'activityTypeName' is actually the label.
+   *
+   * @throws \CRM_Core_Exception
    */
   public function testActivityTypeNameIsReallyLabel(): void {
-    $form = new CRM_Activity_Form_Activity();
-
-    // the actual value is irrelevant we just need something for the tested function to act on
-    $form->_currentlyViewedContactId = $this->source;
-
     // Let's make a new activity type that has a different name from its label just to be sure.
-    $actParams = [
-      'option_group_id' => 'activity_type',
+    $this->createTestEntity('OptionValue', [
+      'option_group_id:name' => 'activity_type',
       'name' => 'wp1234',
       'label' => 'Water Plants',
       'is_active' => 1,
+      'value' => 800,
       'is_default' => 0,
-    ];
-    $result = $this->callAPISuccess('option_value', 'create', $actParams);
-
-    $form->_activityTypeId = $result['values'][$result['id']]['value'];
-    $this->assertNotEmpty($form->_activityTypeId);
-
-    // Do the thing we want to test
-    $form->assignActivityType();
-
-    $this->assertEquals('Water Plants', $form->_activityTypeName);
+    ]);
 
-    // cleanup
-    $this->callAPISuccess('option_value', 'delete', ['id' => $result['id']]);
+    $form = $this->getTestForm('CRM_Activity_Form_Activity', [], ['atype' => 800, 'cid' => $this->source, 'action' => 'add']);
+    $form->processForm(FormWrapper::BUILT);
+    $form->checkTemplateVariable('activityTypeName', 'Water Plants');
+    $form->checkTemplateVariable('activityTypeNameAndLabel', ['machineName' => 'wp1234', 'displayLabel' => 'Water Plants', 'id' => 800]);
   }
 
   /**

tests/phpunit/CRM/Case/BAO/CaseTest.php

@@ -357,7 +357,7 @@ public function testGetRelatedCases(): void {
     $client_id_2 = $this->individualCreate([], 1);
     $caseObj_2 = $this->createCase($client_id_2, $loggedInUser);
     $case_id_2 = $caseObj_2->id;
-
+    $_REQUEST['action'] = 'add';
     $form = $this->getFormObject('CRM_Case_Form_Activity', [
       'activity_type_id' => CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_type_id', 'Link Cases'),
       'link_to_case_id' => $case_id_2,