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

mattwire · 2017-07-23T10:18:33Z

This PR adds support for custom data for Case.Create API in line with other *.Create API functions. It refactors Case.Create API to support updating as the Case.Update API function is deprecated. This means a single codepath is now used for creation and updating, inline with other parts of CiviCRM API.

CRM-18082: CiviCase API Create ignores custom data

mattwire · 2017-07-23T10:26:25Z

@colemanw @aydun Would you mind a quick review of this when you get a moment, as you are linked on the original jira issue

davialexandre

@mattwire Left some inline comments. Besides that, it would be great if the PR description had details about the changes you did and their reasons. I tried to compare them to the suggestions made on the ticket's comments, but it seems you did a bit more than what is suggested there. Those details also helper reviewers to better understand what was the intent.

Finally, do you think you can add tests to this?

davialexandre · 2017-07-24T12:00:24Z

api/v3/Case.php

-    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, [


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

davialexandre · 2017-07-24T12:07:07Z

api/v3/Case.php

+      throw new API_Exception(ts('You cannot update creator id'));
+    }
+
+    $mCaseId = $origContactIds = array();


What does the m mean here?

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

davialexandre · 2017-07-24T12:08:41Z

api/v3/Case.php

-    'end_date' => CRM_Utils_Array::value('end_date', $params),
-    'subject' => $params['subject'],
-  );
+    // return error if modifying creator id


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

Removed comment

davialexandre · 2017-07-24T12:12:16Z

api/v3/Case.php

@@ -60,37 +60,67 @@
 *   api result array
 */
 function civicrm_api3_case_create($params) {
+  // Process params


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

Moved to _civicrm_api_case_format_params()

davialexandre · 2017-07-24T12:37:07Z

api/v3/Case.php

+    // 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];


Why 1 and not 0 or 2?

Also, what is the difference between origContactId and origContactIds?

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.

davialexandre · 2017-07-24T13:10:53Z

api/v3/Case.php

@@ -101,31 +131,46 @@ function civicrm_api3_case_create($params) {
    CRM_Case_BAO_CaseContact::create($contactParams);
  }

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


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

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.

davialexandre · 2017-07-24T13:11:16Z

api/v3/Case.php

+  return civicrm_api3_create_success($values, $params, 'Case', 'create', $caseBAO);
+}
+
+function _civicrm_api3_case_create_xmlProcessor($params, $caseBAO) {


What about a docblock for this function?

davialexandre · 2017-07-24T13:12:28Z

api/v3/Case.php

+    '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),


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

davialexandre · 2017-07-24T13:16:15Z

api/v3/Case.php

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


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?

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

davialexandre · 2017-07-24T13:21:11Z

api/v3/Case.php

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


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

Rearranged a little

mattwire · 2017-08-04T13:41:33Z

Pending unit test and response to @davialexandre (many of your comments are relating to existing code that I didn't modify, but you make some good observations, I'll review and update the PR appropriately).

mattwire · 2017-08-07T10:34:44Z

Unit tests now added

…g Case unit tests to pass

mattwire · 2017-08-07T12:23:35Z

@davejenx Could you do a quick review as it's in use on one of your sites :-)

davejenx · 2017-08-07T13:29:52Z

api/v3/Case.php

-  );
-  _civicrm_api3_case_format_params($params);
+    if (array_key_exists('creator_id', $params)) {
+      throw new API_Exception(ts('You cannot update creator id'));


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.

Thanks, that's now fixed

davejenx · 2017-08-07T13:58:43Z

@mattwire Haven't spotted any other issues - as I understand it, the change largely consists of incorporating the relevant parts of civicrm_api3_case_update() into civicrm_api3_case_create() so that it can serve both purposes, so much of it is existing code. If the existing & new unit tests are all passing, there's nothing else that jumps out at me.
@aydun made some good comments on the JIRA issue so as you said it would be good if he cast an eye over it.

colemanw · 2017-08-08T03:36:45Z

api/v3/Case.php

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


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.

@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.

Why is that necessary?

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

colemanw · 2017-08-09T18:23:19Z

api/v3/Case.php

+      // FIXME: CRM_Case_BAO_Case::retrieveContactIdsByCaseId returns a 1-based array (1 is the first element)
+      // It should really return a 0-based array for consistency.
+      $origContactIds = CRM_Case_BAO_Case::retrieveContactIdsByCaseId($params['id']);
+      $origContactId = $origContactIds[1];


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

Sounds good

mattwire · 2017-08-14T15:18:10Z

@colemanw Think this is now good to merge?

Allow case API create to work with custom data

da4d891

civicrm-builder added the master label Jul 23, 2017

davialexandre suggested changes Jul 24, 2017

View reviewed changes

mattwire changed the title ~~CRM-18082 - Allow case API create to work with custom data~~ WIP CRM-18082 - Allow case API create to work with custom data Aug 4, 2017

mattwire added 4 commits August 7, 2017 11:20

Refactor based on comments from @davialexandre

2a3c0d2

Add deprecated label to case update api function

3755d87

Fix style warnings

ebf4aea

Style warning

18174a4

mattwire added 2 commits August 7, 2017 12:20

contact_id is optional when updating case, this change allows existin…

791a8dc

…g Case unit tests to pass

Add unit tests for case create api custom data

c35f569

davejenx reviewed Aug 7, 2017

View reviewed changes

Don't use ts for exceptions

ef76301

mattwire changed the title ~~WIP CRM-18082 - Allow case API create to work with custom data~~ CRM-18082 - Allow case API create to work with custom data Aug 7, 2017

colemanw reviewed Aug 8, 2017

View reviewed changes

We don't need to return custom data on create

2e50efb

colemanw reviewed Aug 9, 2017

View reviewed changes

mattwire added 2 commits August 10, 2017 21:33

Use CRM_Utils_Array:first() for robustness

f37c1b4

Remove blank lines

ad851b1

colemanw merged commit 1a0ecaf into civicrm:master Aug 14, 2017

mattwire deleted the CRM-18082_civicase-api-custom-data branch August 28, 2017 12:51

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

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

Conversation

mattwire commented Jul 23, 2017 • edited Loading

mattwire commented Jul 23, 2017

davialexandre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattwire commented Aug 4, 2017

mattwire commented Aug 7, 2017 • edited Loading

mattwire commented Aug 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davejenx commented Aug 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattwire commented Aug 14, 2017

mattwire commented Jul 23, 2017 •

edited

Loading

mattwire commented Aug 7, 2017 •

edited

Loading

davejenx commented Aug 7, 2017 •

edited

Loading