From c69cd9b1b5b8d0664a8bec880693599a970e49f6 Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Tue, 19 Sep 2023 09:29:23 +1200 Subject: [PATCH] Fix activity view bug where an activity type id in the url overrides the actual activity type id When we view an activity there is code in the template that renders differently depending on the activity type id. However, if the activity ID (atype) is wrong in the url it gives the url precedence This is a bit obscure - I hit it when I changed the activity type id in the database & the view did not update --- CRM/Activity/Form/Activity.php | 59 ++++++++++--------- CRM/Activity/Form/ActivityFormTrait.php | 51 ++++++++++++++++ Civi/Test/FormWrapper.php | 18 +++++- templates/CRM/Activity/Form/Activity.tpl | 4 +- templates/CRM/Activity/Form/ActivityView.tpl | 2 +- templates/CRM/Case/Form/Activity.tpl | 2 +- templates/CRM/Case/Form/Case.tpl | 2 +- .../CRM/Activity/Form/ActivityTest.php | 56 ++++++++++-------- tests/phpunit/CRM/Case/BAO/CaseTest.php | 2 +- 9 files changed, 135 insertions(+), 61 deletions(-) create mode 100644 CRM/Activity/Form/ActivityFormTrait.php diff --git a/CRM/Activity/Form/Activity.php b/CRM/Activity/Form/Activity.php index 35c4bfb936..c88631dceb 100644 --- a/CRM/Activity/Form/Activity.php +++ b/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 @@ class CRM_Activity_Form_Activity extends CRM_Contact_Form_Task { $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 @@ class CRM_Activity_Form_Activity extends CRM_Contact_Form_Task { $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 @@ class CRM_Activity_Form_Activity extends CRM_Contact_Form_Task { [$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 @@ class CRM_Activity_Form_Activity extends CRM_Contact_Form_Task { } } } - $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; + } + } diff --git a/CRM/Activity/Form/ActivityFormTrait.php b/CRM/Activity/Form/ActivityFormTrait.php new file mode 100644 index 0000000000..81f0195099 --- /dev/null +++ b/CRM/Activity/Form/ActivityFormTrait.php @@ -0,0 +1,51 @@ +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'); + } + +} diff --git a/Civi/Test/FormWrapper.php b/Civi/Test/FormWrapper.php index e20b031061..ac13407d34 100644 --- a/Civi/Test/FormWrapper.php +++ b/Civi/Test/FormWrapper.php @@ -107,6 +107,7 @@ class FormWrapper { * @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 @@ class FormWrapper { 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 @@ class FormWrapper { */ 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 @@ class FormWrapper { 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)); + } + } + } diff --git a/templates/CRM/Activity/Form/Activity.tpl b/templates/CRM/Activity/Form/Activity.tpl index bde0f91cb4..adbc8da900 100644 --- a/templates/CRM/Activity/Form/Activity.tpl +++ b/templates/CRM/Activity/Form/Activity.tpl @@ -12,7 +12,7 @@
{else} {if $activityTypeDescription} -
{$activityTypeDescription}
+
{$activityTypeDescription|purify}
{/if}
{/if} @@ -29,7 +29,7 @@ {if $action eq 4} {if $activityTypeDescription} -
{$activityTypeDescription}
+
{$activityTypeDescription|purify}
{/if} {else} {if $context eq 'standalone' or $context eq 'search' or $context eq 'smog'} diff --git a/templates/CRM/Activity/Form/ActivityView.tpl b/templates/CRM/Activity/Form/ActivityView.tpl index fa736453b2..9af13c0960 100644 --- a/templates/CRM/Activity/Form/ActivityView.tpl +++ b/templates/CRM/Activity/Form/ActivityView.tpl @@ -9,7 +9,7 @@ *}
{if $activityTypeDescription} -
{$activityTypeDescription}
+
{$activityTypeDescription|purify}
{/if} diff --git a/templates/CRM/Case/Form/Activity.tpl b/templates/CRM/Case/Form/Activity.tpl index ec74f69622..aa14680e2b 100644 --- a/templates/CRM/Case/Form/Activity.tpl +++ b/templates/CRM/Case/Form/Activity.tpl @@ -23,7 +23,7 @@
{if $activityTypeDescription} -
{$activityTypeDescription}
+
{$activityTypeDescription|purify}
{/if} {* Block for change status, case type and start date. *} diff --git a/templates/CRM/Case/Form/Case.tpl b/templates/CRM/Case/Form/Case.tpl index 92ab07e2fa..bb34a8ec51 100644 --- a/templates/CRM/Case/Form/Case.tpl +++ b/templates/CRM/Case/Form/Case.tpl @@ -27,7 +27,7 @@
{if $activityTypeDescription} -
{$activityTypeDescription}
+
{$activityTypeDescription|purify}
{/if} {if $clientName} diff --git a/tests/phpunit/CRM/Activity/Form/ActivityTest.php b/tests/phpunit/CRM/Activity/Form/ActivityTest.php index d527af19db..f64aaa37d1 100644 --- a/tests/phpunit/CRM/Activity/Form/ActivityTest.php +++ b/tests/phpunit/CRM/Activity/Form/ActivityTest.php @@ -1,5 +1,8 @@ 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 @@ class CRM_Activity_Form_ActivityTest extends CiviUnitTestCase { } /** - * 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]); } /** diff --git a/tests/phpunit/CRM/Case/BAO/CaseTest.php b/tests/phpunit/CRM/Case/BAO/CaseTest.php index c687ab90cc..26b288315a 100644 --- a/tests/phpunit/CRM/Case/BAO/CaseTest.php +++ b/tests/phpunit/CRM/Case/BAO/CaseTest.php @@ -357,7 +357,7 @@ class CRM_Case_BAO_CaseTest extends CiviUnitTestCase { $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, -- 2.25.1