CRM-20097 - Tweaks and fixes #10288
CRM-20097 - Tweaks and fixes #10288
Conversation
colemanw
commented
May 1, 2017
•
colemanw
commented
- CRM-20097: Case action menu
PR civicrm#10288 fixes an edge-case in plugging in a chained-value. The scenario originated in `org.civicrm.civicase` Angular UI. I just copied/trimmed it to reproduce the bug.
| @@ -136,6 +136,10 @@ function _civicrm_api_replace_variables(&$params, &$parentResult, $separator = ' | |||
| elseif (is_array($value) && is_string(reset($value)) && substr(reset($value), 0, 6) == '$value') { | |||
| $key = key($value); | |||
| $value[$key] = _civicrm_api_replace_variable($value[$key], $parentResult, $separator); | |||
| // A null value with an operator will cause an error, so remove it. | |||
| if ($value[$key] === NULL) { | |||
| $value = ''; | |||
There was a problem hiding this comment.
It seems this merits a test-case. I copied the API call used by org.civicrm.civicase, trimmed it down a bit, and put that in a new test (on your branch).
| @@ -471,7 +471,7 @@ function _civicrm_api3_activity_get_formatResult($params, $activities, $options) | |||
| $dao = CRM_Core_DAO::executeQuery("SELECT activity_id, case_id FROM civicrm_case_activity WHERE activity_id IN (%1)", | |||
| array(1 => array(implode(',', array_keys($activities)), 'String', CRM_Core_DAO::QUERY_FORMAT_NO_QUOTES))); | |||
| while ($dao->fetch()) { | |||
| $activities[$dao->activity_id]['case_id'] = $dao->case_id; | |||
| $activities[$dao->activity_id]['case_id'][] = $dao->case_id; | |||
There was a problem hiding this comment.
OK, so repeating aloud to check my comprehension:
- Activities are normally associated with zero or one cases, but in the scenario of linked-cases, you can have an activity which lives in multiple cases.
- The old/existing API contract returns an
intor null, which is fine for the normal scenario. The new/proposed API contract returns anarrayofints, which is more generally correct. - Problem: Changing the data-structure of
case_idwill break any API consumers who use it. - Suggestion: The
ActivityAPI should support two different return notations,case_id(singular, legacy behavior) orcase_ids(array, new behavior). - Suggestion: Whether returning
case_idorcase_ids, the query which readscivicrm_case_activityshould include anORDER BYclause to ensure that the result is deterministic.
There was a problem hiding this comment.
The case_id return value is brand-new and I don't think it's even been documented yet, so most likely the only consumer so far is the civicase extension. IMO it's not too late to change it now.
There was a problem hiding this comment.
Oh, I see. It's only been in 4.7.18 and the upcoming 4.7.19:
git show bc4b6f0f5694d1a5b8e2151e6dcf9a394c300df4 | head
commit bc4b6f0f5694d1a5b8e2151e6dcf9a394c300df4
Author: Coleman Watts <coleman@civicrm.org>
Date: Mon Mar 6 22:49:00 2017 -0500
CRM-20102 - Return case_id from activity api
$ git tag --contains bc4b6f0f5694d1a5b8e2151e6dcf9a394c300df4
4.7.18
There was a problem hiding this comment.
Right, I highly doubt anyone has used it let alone noticed it yet.
There was a problem hiding this comment.
FYI in theory we might start using multiple case ids per activity in more than just the "link cases" scenario. For example sending a mail blast across multiple cases.
There was a problem hiding this comment.
If you break something that did not have a unit test, it ain't broken ;)
Question on create, does it work with both case_id =int or case_id=array? Breaking on read is IMO not a huge issue, breaking on create risks loosing data.
There was a problem hiding this comment.
Good point @tttp I've just updated the PR to cover that.
|
@colemanw I did a general sniff-test of using "Linked Cases" in the main UI, and this seems to still work. My only real hangup is changing the contract for the Tangentially, it seems weird that the "Linked Case" activity has status "Scheduled"... and that you can't change it. This is a pre-existing issue (and god knows there's probably someone who relies on it), but hopefully the new UI can default to status "Completed". |
The smaller getRelatedCaseIds() is used by the civicase extension's getdetails api action.
|
My previous concerns were addressed in the code-level discussion. 👍 |
