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

CRM-18082 - Allow case API create to work with custom data #10728

Merged
merged 11 commits into from
Aug 14, 2017
Merged

CRM-18082 - Allow case API create to work with custom data #10728

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
da4d891
Allow case API create to work with custom data
mattwire Jul 23, 2017
2a3c0d2
Refactor based on comments from @davialexandre
mattwire Aug 7, 2017
3755d87
Add deprecated label to case update api function
mattwire Aug 7, 2017
ebf4aea
Fix style warnings
mattwire Aug 7, 2017
18174a4
Style warning
mattwire Aug 7, 2017
791a8dc
contact_id is optional when updating case, this change allows existin…
mattwire Aug 7, 2017
c35f569
Add unit tests for case create api custom data
mattwire Aug 7, 2017
ef76301
Don't use ts for exceptions
mattwire Aug 7, 2017
2e50efb
We don't need to return custom data on create
mattwire Aug 9, 2017
f37c1b4
Use CRM_Utils_Array:first() for robustness
mattwire Aug 10, 2017
ad851b1
Remove blank lines
mattwire Aug 10, 2017
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
4 changes: 3 additions & 1 deletion CRM/Case/BAO/Case.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,9 @@ public static function enabled() {
public static function add(&$params) {
$caseDAO = new CRM_Case_DAO_Case();
$caseDAO->copyValues($params);
return $caseDAO->save();
$result = $caseDAO->save();
$caseDAO->find(TRUE); // Get other case values (required by XML processor), this adds to $result array
return $result;
}

/**
Expand Down
152 changes: 100 additions & 52 deletions api/v3/Case.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,37 +60,67 @@
* api result array
*/
function civicrm_api3_case_create($params) {
// Process params
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding this comment here, I think it would be better to extract the logic to a function with a descriptive name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to _civicrm_api_case_format_params()

$values = array();
_civicrm_api3_custom_format_params($params, $values, 'Case');
$params = array_merge($params, $values);

if (!empty($params['id'])) {
return civicrm_api3_case_update($params);
}

civicrm_api3_verify_mandatory($params, NULL, array(
'contact_id',
'subject',
array('case_type', 'case_type_id'))
);
_civicrm_api3_case_format_params($params);

// If format_params didn't find what it was looking for, return error
if (empty($params['case_type_id'])) {
throw new API_Exception('Invalid case_type. No such case type exists.');
}
if (empty($params['case_type'])) {
throw new API_Exception('Invalid case_type_id. No such case type exists.');
if (empty($params['id'])) {
// Creating a new case, so make sure we have the necessary parameters
civicrm_api3_verify_mandatory($params, NULL, [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since PHP 5.3 is still supported, we can't use short array syntax yet

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

'contact_id',
'subject',
['case_type', 'case_type_id']
]
);
}
else {
// Update an existing case
// FIXME: Some of this logic should move to the BAO object?
// FIXME: Should we check if case with ID actually exists?
if (!isset($params['case_id']) && isset($params['id'])) {
$params['case_id'] = $params['id'];
}

// Fixme: can we safely pass raw params to the BAO?
$newParams = array(
'case_type_id' => $params['case_type_id'],
'creator_id' => $params['creator_id'],
'status_id' => $params['status_id'],
'start_date' => $params['start_date'],
'end_date' => CRM_Utils_Array::value('end_date', $params),
'subject' => $params['subject'],
);
// return error if modifying creator id
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the code is quite self-explanatory, so there's no need to this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed comment

if (array_key_exists('creator_id', $params)) {
throw new API_Exception(ts('You cannot update creator id'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny picky point: I believe ts() isn't generally used for API exceptions. Discussed at https://chat.civicrm.org/civicrm/pl/qiotz5x6p7fxdegbiece7xhhmc . ts() isn't used for the other exceptions in this file, apart from an identical instance in civicrm_api3_case_update() which presumably this was copied from.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that's now fixed

}

$mCaseId = $origContactIds = array();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does the m mean here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not my code :-) But agreed, it means merged so I've renamed to $mergedCaseId


// get original contact id and creator id of case
if (!empty($params['contact_id'])) {
$origContactIds = CRM_Case_BAO_Case::retrieveContactIdsByCaseId($params['id']);
$origContactId = $origContactIds[1];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 1 and not 0 or 2?

Also, what is the difference between origContactId and origContactIds?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a comment, again not my code, but copied from the update function. That function returns a 1-based array, as it's used elsewhere it's not in scope for me to refactor right now! But I've added a FIXME comment there explaining. origContactId is an integer, origContactIds an array of contactIds, again copied from update function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use CRM_Utils_Array::first() so you don't have to worry about how the array is indexed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

}

if (count($origContactIds) > 1) {
// check valid orig contact id
if (!empty($params['orig_contact_id']) && !in_array($params['orig_contact_id'], $origContactIds)) {
throw new API_Exception('Invalid case contact id (orig_contact_id)');
}
elseif (empty($params['orig_contact_id'])) {
throw new API_Exception('Case is linked with more than one contact id. Provide the required params orig_contact_id to be replaced');
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually, I find early returns easier to read than if/else/elseif. Since you're throwing exceptions here, you can reorganize this as:

if (empty($params['orig_contact_id'])) {
  throw new API_Exception('Case is linked with more than one contact id. Provide the required params orig_contact_id to be replaced');
}

if (!empty($params['orig_contact_id']) && !in_array($params['orig_contact_id'], $origContactIds)) {
  throw new API_Exception('Invalid case contact id (orig_contact_id)');
}

$origContactId = $params['orig_contact_id'];

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, done

$origContactId = $params['orig_contact_id'];
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this logic to get the original contact ID should be extracted to a separate function of even to a BAO method. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I've added a FIXME comment to that effect


$caseBAO = CRM_Case_BAO_Case::create($newParams);
// check for same contact id for edit Client
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it would be better to use the comment to explain why do we need this check and the merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes... Though I'm not sure myself, again it's copied from the update function

if (!empty($params['contact_id']) && !in_array($params['contact_id'], $origContactIds)) {
$mCaseId = CRM_Case_BAO_Case::mergeCases($params['contact_id'], $params['case_id'], $origContactId, NULL, TRUE);
}

// If we merged cases then update the merged case
if (!empty($mCaseId[0])) {
$params['id'] = $mCaseId[0];
}
}

// Create/update the case
$caseBAO = CRM_Case_BAO_Case::create($params);

if (!$caseBAO) {
throw new API_Exception('Case not created. Please check input params.');
Expand All @@ -101,31 +131,46 @@ function civicrm_api3_case_create($params) {
CRM_Case_BAO_CaseContact::create($contactParams);
}

if (!isset($params['id'])) {
// Creating a new case
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment correct? Are we really creating a case here? If not, please use the comment to explain why do we need this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are, I don't really understand the xml stuff very well, except that cases are defined in xml files, and when creating a new one you have to read the xml file to get case params/setup the case. I've added a comment to that effect - the code in _civicrm_api3_case_create_xmlProcessor is simply extracted code from the original api_v3_case_create function.

_civicrm_api3_case_create_xmlProcessor($params, $caseBAO);
}

// return case
$values = array();
_civicrm_api3_object_to_array($caseBAO, $values[$caseBAO->id]);

// Add custom data
if (isset($params['custom'])) {
_civicrm_api3_custom_data_get($values[$caseBAO->id],TRUE,'case',$caseBAO->id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there should be spaces between each of the params here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an extra database hit, which we normally don't do for create operations. There is an api option called something like "force_reload" which takes care of this type of thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colemanw Ok, I've had a good look through the code for various other APIs and I can't find any reference to this. Agreed that it's not ideal calling get again, but not sure what else I can do here to return back the customdata if it was passed in on create.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is that necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's not :-) Removed it now, all tests still passing


return civicrm_api3_create_success($values, $params, 'Case', 'create', $caseBAO);
}

function _civicrm_api3_case_create_xmlProcessor($params, $caseBAO) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about a docblock for this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

// Format params for xmlProcessor
if (isset($caseBAO->id)) { $params['id'] = $caseBAO->id; }

// Initialize XML processor with $params
$xmlProcessor = new CRM_Case_XMLProcessor_Process();
$xmlProcessorParams = array(
'clientID' => $params['contact_id'],
'creatorID' => $params['creator_id'],
'clientID' => CRM_Utils_Array::value('contact_id',$params),
'creatorID' => CRM_Utils_Array::value('creator_id',$params),
'standardTimeline' => 1,
'activityTypeName' => 'Open Case',
'caseID' => $caseBAO->id,
'subject' => $params['subject'],
'location' => CRM_Utils_Array::value('location', $params),
'activity_date_time' => $params['start_date'],
'duration' => CRM_Utils_Array::value('duration', $params),
'medium_id' => CRM_Utils_Array::value('medium_id', $params),
'details' => CRM_Utils_Array::value('details', $params),
'caseID' => CRM_Utils_Array::value('id',$params),
'subject' => CRM_Utils_Array::value('subject',$params),
'location' => CRM_Utils_Array::value('location',$params),
'activity_date_time' => CRM_Utils_Array::value('start_date',$params),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a space between the params of every CRM_Utils_Array::value() call here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

'duration' => CRM_Utils_Array::value('duration',$params),
'medium_id' => CRM_Utils_Array::value('medium_id',$params),
'details' => CRM_Utils_Array::value('details',$params),
'custom' => array(),
);

// Do it! :-D
$xmlProcessor->run($params['case_type'], $xmlProcessorParams);

// return case
$values = array();
_civicrm_api3_object_to_array($caseBAO, $values[$caseBAO->id]);

return civicrm_api3_create_success($values, $params, 'Case', 'create', $caseBAO);
}

/**
Expand Down Expand Up @@ -645,23 +690,26 @@ function _civicrm_api3_case_read(&$cases, $options) {
* @param array $params
*/
function _civicrm_api3_case_format_params(&$params) {
// figure out case type id from case type and vice-versa
$caseTypes = CRM_Case_PseudoConstant::caseType('name', FALSE);
if (empty($params['case_type_id'])) {
$params['case_type_id'] = array_search($params['case_type'], $caseTypes);

// DEPRECATED: lookup by label for backward compatibility
if (!$params['case_type_id']) {
$caseTypeLabels = CRM_Case_PseudoConstant::caseType('title', FALSE);
$params['case_type_id'] = array_search($params['case_type'], $caseTypeLabels);
if (!empty($params['case_type_id']) || !empty($params['case_type'])) {
// figure out case type id from case type and vice-versa
$caseTypes = CRM_Case_PseudoConstant::caseType('name', FALSE);
if (empty($params['case_type_id'])) {
$params['case_type_id'] = array_search($params['case_type'], $caseTypes);

// DEPRECATED: lookup by label for backward compatibility
if (!$params['case_type_id']) {
$caseTypeLabels = CRM_Case_PseudoConstant::caseType('title', FALSE);
$params['case_type_id'] = array_search($params['case_type'], $caseTypeLabels);
$params['case_type'] = $caseTypes[$params['case_type_id']];
}
}
elseif (empty($params['case_type'])) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Early returns would improve this method's readability and considerably reduce the indentation level in this method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rearranged a little

$params['case_type'] = $caseTypes[$params['case_type_id']];
}
}
elseif (empty($params['case_type'])) {
$params['case_type'] = $caseTypes[$params['case_type_id']];
}
}


/**
* It actually works a lot better to use the CaseContact api instead of the Case api
* for entityRef fields so we can perform the necessary joins,
Expand Down