CRM-21598 - Case Activity issues with custom Completed Status Type #11456
Conversation
CRM/Case/BAO/Case.php
Outdated
| $compStatusNames = array_merge( | ||
| array('Completed', 'Left Message', 'Cancelled', 'Unreachable', 'Not Required'), | ||
| CRM_Activity_BAO_Activity::getStatusesByType(CRM_Activity_BAO_Activity::COMPLETED) | ||
| ); |
There was a problem hiding this comment.
@jitendrapurohit why are you merging these 2 arrays? I think just the statusesByType should provide the needed information, correct?
There was a problem hiding this comment.
doesn't seem so. 'Left Message', 'Unreachable' and 'Not Required' belongs to activity statuses of type INCOMPLETE and 'Cancelled' is of type CANCELLED.
But the current flow doesn't want to display them in red color if they have an overdue date(see comment in L1042). statusesByType will only return the completed activity status.
We can use the following API if we intend to use only one variable -
$compStatusValues = civicrm_api3('OptionValue', 'get', array(
'sequential' => 1,
'return' => array("value"),
'option_group_id' => "activity_status",
'name' => array('IN' => array("Left Message", "Cancelled", "Unreachable", "Not Required")),
'filter' => CRM_Activity_BAO_Activity::COMPLETED,
'options' => array('or' => array(array("name", "filter"))),
));
$compStatusValues = CRM_Utils_Array::collect('value', $compStatusValues['values']);
But seems it'll not be a good change w.r.t readability than the current one?
CRM/Case/BAO/Case.php
Outdated
| $compStatusNames = array_merge( | ||
| array('Completed', 'Left Message', 'Cancelled', 'Unreachable', 'Not Required'), | ||
| CRM_Activity_BAO_Activity::getStatusesByType(CRM_Activity_BAO_Activity::COMPLETED) | ||
| ); | ||
| foreach ($compStatusNames as $name) { | ||
| $compStatusValues[] = CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_status_id', $name); | ||
| } |
There was a problem hiding this comment.
I don't think this loop is necessary anymore. If you make the above suggested change, then this is equal to array_keys($compStatusNames) (see getStatusesByType return value). Probably a bit more code cleanup below would allow us to just use one variable called $completedStatuses instead of one for names and the other for values.
|
test this please |
|
jenkins, retest this please |
|
retest this please |
|
can the status of this PR be updated from |
CRM/Case/BAO/Case.php
Outdated
| @@ -1041,10 +1041,11 @@ public static function getCaseActivity($caseID, &$params, $contactID, $context = | |||
|
|
|||
| // define statuses which are handled like Completed status (others are assumed to be handled like Scheduled status) | |||
| $compStatusValues = array(); | |||
| $compStatusNames = array('Completed', 'Left Message', 'Cancelled', 'Unreachable', 'Not Required'); | |||
| $compStatusNames = array('Left Message', 'Cancelled', 'Unreachable', 'Not Required'); | |||
There was a problem hiding this comment.
I'm still not happy with this made-up list. IMO we should be using
CRM_Activity_BAO_Activity::getStatusesByType(CRM_Activity_BAO_Activity::COMPLETED) + CRM_Activity_BAO_Activity::getStatusesByType(CRM_Activity_BAO_Activity::CANCELLED)
There was a problem hiding this comment.
Basically whoever made up that list made some arbitrary decisions about some ambiguous statuses. Is "Left Message" considered completed, or not? Well it depends on the use-case really. Which is why we should allow it to be user-configurable and change the code to respect the configured options rather than a hard-coded list IMO.
There was a problem hiding this comment.
Updated the PR with the above changes. Only $compStatusValues is used now which hold the ids of COMPLETED and CANCELLED activity types 👍
| @@ -48,7 +48,7 @@ public function onCaseChange(\Civi\CCase\Event\CaseChangeEvent $event) { | |||
| } | |||
|
|
|||
| $actTypes = array_flip(\CRM_Core_PseudoConstant::activityType(TRUE, TRUE, FALSE, 'name')); | |||
| $actStatuses = array_flip(\CRM_Core_PseudoConstant::activityStatus('name')); | |||
| $actStatuses = array_flip(\CRM_Activity_BAO_Activity::getStatusesByType(\CRM_Activity_BAO_Activity::COMPLETED)); | |||
There was a problem hiding this comment.
What does this function do and does it also need to respond to CANCELLED activities?
There was a problem hiding this comment.
This function calls the next activity in sequence when the current activity is set to Completed. It was only checking for the single activity status before(Completed - line 62 below). Now, after the above changes, it triggers the creation of next activity when the current activity is updated to any of the statuses of TYPE = COMPLETED.
I'm not really sure if it makes sense to create next activity when the present activity is CANCELLED. But, this was not the case before this PR.
There was a problem hiding this comment.
Ok, can you add what you just said about what the function does into the docblock for future reference?
|
@colemanw this PR looks very much as though you have reviewed it & approved it, but not merged it |
|
@colemanw ping - what is the status here? |
|
I've read the code again. Because it has a unit test I'm comfortable merging at this point. |
Overview
Case Activity issues with Completed status types created by the user
Before
Two issues noticed when a new activity status eg "Skip Activity" is created with "Status Type" =
Completed.1/ Case Activity still shown as red when it is updated to the newly created "Skip Activity" status.
2/ Case activity sequence is not updated when status is changed to the Completed(Skip Activity).
Check JIRA to replicate them.
After
works fine.
Comments
Updated unit test to check for user-created
Completedstatus.