From 7a87a7a4dbc8e4573fb78ea9f07075e308074185 Mon Sep 17 00:00:00 2001 From: DemeritCowboy Date: Mon, 5 Aug 2019 18:12:25 -0400 Subject: [PATCH] seek and document activityTypeName --- CRM/Activity/Form/Activity.php | 67 ++++++++++++------- .../CRM/Activity/Form/ActivityTest.php | 28 ++++++++ 2 files changed, 71 insertions(+), 24 deletions(-) diff --git a/CRM/Activity/Form/Activity.php b/CRM/Activity/Form/Activity.php index dc6718406bf5..f8944d7c44f4 100644 --- a/CRM/Activity/Form/Activity.php +++ b/CRM/Activity/Form/Activity.php @@ -59,6 +59,8 @@ class CRM_Activity_Form_Activity extends CRM_Contact_Form_Task { /** * The name of activity type. + * Except it's actually label ha ha - dev/core#1116 + * But we don't want to touch this because it's a form member variable and so people might be using it in hooks and such. * * @var string */ @@ -323,30 +325,7 @@ public function preProcess() { ); } - // Assigning Activity type name. - if ($this->_activityTypeId) { - $activityTName = CRM_Core_OptionGroup::values('activity_type', FALSE, FALSE, FALSE, 'AND v.value = ' . $this->_activityTypeId, 'label'); - if ($activityTName[$this->_activityTypeId]) { - $this->_activityTypeName = $activityTName[$this->_activityTypeId]; - $this->assign('activityTName', $activityTName[$this->_activityTypeId]); - } - // Set title. - if (isset($activityTName)) { - $activityName = CRM_Utils_Array::value($this->_activityTypeId, $activityTName); - - if ($this->_currentlyViewedContactId) { - $displayName = CRM_Contact_BAO_Contact::displayName($this->_currentlyViewedContactId); - // Check if this is default domain contact CRM-10482. - if (CRM_Contact_BAO_Contact::checkDomainContact($this->_currentlyViewedContactId)) { - $displayName .= ' (' . ts('default organization') . ')'; - } - CRM_Utils_System::setTitle($displayName . ' - ' . $activityName); - } - else { - CRM_Utils_System::setTitle(ts('%1 Activity', [1 => $activityName])); - } - } - } + $this->assignActivityTypeName(); // Check the mode when this form is called either single or as // search task action. @@ -405,6 +384,8 @@ public function preProcess() { if ($this->_activityTypeId) { // Set activity type name and description to template. + // activityTypeName means label here not name, but it should be name (dev/core#1116-fixme) + // This is double-fun because we already assigned it above (incorrectly to label also) using an even different method. However note that parent::postProcess can get called in between the two places, which could theoretically change it, and so maybe at one time this was here to reset it again? list($this->_activityTypeName, $activityTypeDescription) = CRM_Core_BAO_OptionValue::getActivityTypeDetails($this->_activityTypeId); $this->assign('activityTypeName', $this->_activityTypeName); $this->assign('activityTypeDescription', $activityTypeDescription); @@ -530,6 +511,7 @@ public function preProcess() { $this->_values['details'] = CRM_Utils_String::stripAlternatives($this->_values['details']) ?: ''; } + // activityTypeName means label here not name, but it should be name (dev/core#1116-fixme) if ($this->_activityTypeName === 'Inbound Email' && !CRM_Core_Permission::check('edit inbound email basic information and content') ) { @@ -787,6 +769,7 @@ public function buildQuickForm() { // we need to hide activity tagset for special activities $specialActivities = ['Open Case']; + // activityTypeName means label here not name, but it should be name (dev/core#1116-fixme) if (!in_array($this->_activityTypeName, $specialActivities)) { // build tag widget $parentNames = CRM_Core_BAO_Tag::getTagSet('civicrm_activity'); @@ -1246,4 +1229,40 @@ public function endPostProcess(&$params, &$activity) { } } + /** + * assignActivityTypeName() + * Which really means label, but we don't want to fix it because it sets a form member variable _activityTypeName which might be used in form hooks. dev/core#1116 + * So just pull out this section which was in preprocess() so it can be tested at least. + */ + public function assignActivityTypeName() { + if ($this->_activityTypeId) { + // activityTName means label here not name, but it should be name (dev/core#1123-fixme). Except see the note just below where since _activityTypeName is built from it and we don't want to touch that much. + $activityTName = CRM_Core_OptionGroup::values('activity_type', FALSE, FALSE, FALSE, 'AND v.value = ' . $this->_activityTypeId, 'label'); + if ($activityTName[$this->_activityTypeId]) { + // activityTypeName means label here not name, but it should be name (dev/core#1116-fixme) BUT!!! This is a form member variable, and so people might be using it in hooks and such. So we're not going to fix this, just note that it is label. + $this->_activityTypeName = $activityTName[$this->_activityTypeId]; + $this->assign('activityTName', $activityTName[$this->_activityTypeId]); + } + // Set title. + if (isset($activityTName)) { + // activityName means label here not name, but it's ok because label is desired here (dev/core#1116-ok-label) + $activityName = CRM_Utils_Array::value($this->_activityTypeId, $activityTName); + + if ($this->_currentlyViewedContactId) { + $displayName = CRM_Contact_BAO_Contact::displayName($this->_currentlyViewedContactId); + // Check if this is default domain contact CRM-10482. + if (CRM_Contact_BAO_Contact::checkDomainContact($this->_currentlyViewedContactId)) { + $displayName .= ' (' . ts('default organization') . ')'; + } + // activityName means label here not name, but it's ok because label is desired here (dev/core#1116-ok-label) + CRM_Utils_System::setTitle($displayName . ' - ' . $activityName); + } + else { + // activityName means label here not name, but it's ok because label is desired here (dev/core#1116-ok-label) + CRM_Utils_System::setTitle(ts('%1 Activity', [1 => $activityName])); + } + } + } + } + } diff --git a/tests/phpunit/CRM/Activity/Form/ActivityTest.php b/tests/phpunit/CRM/Activity/Form/ActivityTest.php index eb62a16045c7..1caf2c257779 100644 --- a/tests/phpunit/CRM/Activity/Form/ActivityTest.php +++ b/tests/phpunit/CRM/Activity/Form/ActivityTest.php @@ -186,4 +186,32 @@ private function deleteActivity($activityId, $mode = NULL) { $form->postProcess(); } + /** + * 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 + */ + public function testActivityTypeNameIsReallyLabel() { + $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', + 'name' => 'wp1234', + 'label' => 'Water Plants', + 'is_active' => 1, + 'is_default' => 0, + ]; + $result = $this->callAPISuccess('option_value', 'create', $actParams); + + $form->_activityTypeId = $result['values'][$result['id']]['value']; + $this->assertNotEmpty($form->_activityTypeId); + + // Assign name, I mean label. + $form->assignActivityTypeName(); + + $this->assertEquals('Water Plants', $form->_activityTypeName); + } + }