dev/core#24 Passing an array for contact_id/client_id to Case.Create API when updating an existing case causes case to be "reassigned" #11830
Conversation
mattwire
changed the title
|
test fails are related but seem to be e-notices. Probably require a test but maybe it would be good to see @colemanw is bought into this change first |
mattwire
force-pushed
the
devcore24_case_api_contact_fix
branch
from
23cf034 to
0535447
Compare
|
@colemanw @eileenmcnaughton Test failures seem unrelated (authorizenet)? |
|
test this please |
|
(now we have guzzle we can use a mockHandler when the fails get annoying enough :-) |
mattwire
changed the title
|
Agree test fail is unrelated - @colemanw is probably the best person to consider this |
api/v3/Case.php
Outdated
| if (!empty($params['contact_id']) && !in_array($params['contact_id'], $origContactIds)) { | ||
| $mergedCaseId = CRM_Case_BAO_Case::mergeCases($params['contact_id'], $params['case_id'], $origContactId, NULL, TRUE); | ||
| if (!empty($params['contact_id'])) { | ||
| $mainContactId = CRM_Utils_Array::first($params['contact_id']); |
There was a problem hiding this comment.
This doesn't look right. You seem to be assuming the value is an array here, but it might not be.
mattwire
force-pushed
the
devcore24_case_api_contact_fix
branch
2 times, most recently
from
4a55576 to
b47433c
Compare
|
@colemanw I've updated this PR following your comments and tests are passing - would you mind reviewing? |
api/v3/Case.php
Outdated
| $mainContactId = CRM_Utils_Array::first($params['contact_id']); | ||
| if (!isset($mainContactId)) { | ||
| $mainContactId = $params['contact_id']; | ||
| } |
There was a problem hiding this comment.
I think you could just cast it to an array...
$mainContactId = CRM_Utils_Array::first((array) $params['contact_id']);
api/v3/Case.php
Outdated
| // Convert single value to array here to simplify processing in later functions which expect an array. | ||
| if (isset($params['contact_id']) && !is_array($params['contact_id'])) { | ||
| $params['contact_id'] = array($params['contact_id']); | ||
| } |
There was a problem hiding this comment.
Wait, if it's not set then this will throw an undefined index warning.
…s it to be an array in some places and a value in others, leading to cases being 'reassigned' to the default contact id (1)
mattwire
force-pushed
the
devcore24_case_api_contact_fix
branch
from
b47433c to
ea2aa8f
Compare
|
@colemanw looks like your previous comments have been responded to |
eileenmcnaughton
added
the
sig:extension support
| $mergedCaseId = CRM_Case_BAO_Case::mergeCases($params['contact_id'], $params['case_id'], $origContactId, NULL, TRUE); | ||
| } | ||
| // Get the specified main contact ID for the case | ||
| $mainContactId = CRM_Utils_Array::first($params['contact_id']); |
There was a problem hiding this comment.
@mattwire What guarantees that this value is an array?
There was a problem hiding this comment.
|
Ok this looks good now. |
Overview
https://lab.civicrm.org/dev/core/issues/24
The contact_id/client_id for the case supports multiple ids. It can be passed to the case.create API either as a single value or as an array. However, the code is inconsistent in it's handling of contact_id and expects it to be both an array and a single value. The result is that if an array is passed in it will fail to detect that an existing case contact_id already matches and it will "reassign" the case to the default contact id (1). This issue was identified because of inconsistent behaviour when submitting case/activity updates via webform_civicrm.
The proposed solution is to ensure that contact_id is always an array, and then check the first element against the original contact id.
Before
If an array is passed in for contact_id the case is reassigned to contact ID 1.
After
Case is not reassigned to contact ID 1. Activities are correctly filed on existing case.
Technical Details
For historical reasons retrieveContactIDsByCaseId() returns a 1-based array. The only place it needs to be 1-based is in the Case.Get API otherwise the API return values change (and it's possible that someone relies on those return values being 1-based). Ideally they should be coding to retrieve the first array element, instead of a hardcoded ID.
This PR for webform_civicrm removes the 1-based requirement from webform_civicrm: colemanw/webform_civicrm#118