Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Do not merge] [NFC] dev/core#1116 - document where activityTypeName is name and where it's label #14972

Closed
wants to merge 1 commit into from
Closed

[Do not merge] [NFC] dev/core#1116 - document where activityTypeName is name and where it's label #14972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 43 additions & 24 deletions CRM/Activity/Form/Activity.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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')
) {
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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]));
}
}
}
}

}
28 changes: 28 additions & 0 deletions tests/phpunit/CRM/Activity/Form/ActivityTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

}