dev/core#1659: Fix Case.get API returning Case Clients As Part of Related Contacts

[tunbola]

Overview

The Case.get API returns case clients as part of related contacts. The case clients are not supposed to be returned as part of the related contacts.

Before

• Issue described above exists.

After

The Case.get API no longer returns case clients as part of related contacts. They are excluded.

The condition here: https://github.com/civicrm/civicrm-core/blob/master/CRM/Case/BAO/Case.php#L1160 is meant to exclude case clients but when the clients are more than one for a case, the parameter here is treated as a String rather than CommaSeparatedIntegers, so the SQL end up having conditions like NOT IN ('2,3') rather than NOT IN (2,3).

The fix was to treat the Case clients as CommaSeparatedIntegers

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[colemanw]

This fix looks like it's headed in the right direction, but also needs a unit test.

[colemanw]

It looks to me like you're also trying to handle the scenario where $caseInfo['client_id'] is an empty string. But in that case it would get injected into a sql query like AND cc.id NOT IN ("") which is bad if not invalid SQL. Is that a real possibility? In that case we'd need to handle it better, probably by altering the query to omit that clause.

[tunbola]

@colemanw , It is a possibility, when I changed the param type to CommaSeparatedIntegers directly, I got an error emanating from the civirules module. I described the error under the Proposed solution in the section here: https://lab.civicrm.org/dev/core/issues/1659, so the issue had always been there and the query will run apparently when $caseInfo['client_id'] is an empty string but it's bad SQL.

I can update the PR to check for this and exclude the condition from the SQL and also add a test. Can you point me to a test similar to what I need to add for this?

[colemanw]

Fortunately, that clause is literally the last line in the query. So you could delete that line from the query, and also delete key 2 from the $params array, then do a conditional afterwards:

if (!empty($caseInfo['client_id'])) {
  $query .= <<<HERESQL
AND cc.id NOT IN (%2)
HERESQL;
  $params[2] = [implode(',', $caseInfo['client_id']), 'CommaSeparatedIntegers'];
}

[tunbola]

@colemanw
The clause is actually in two places in the Query. Here #16837 and #16837.

Was thinking of fixing like this:

$caseClientCondition = !empty($caseInfo['client_id']) ? "AND cc.id NOT IN (%2)" : '';
    $query = <<<HERESQL
    SELECT cc.display_name as name, cc.sort_name as sort_name, cc.id, cr.relationship_type_id, crt.label_b_a as role, crt.name_b_a as role_name, ce.email, cp.phone
    FROM civicrm_relationship cr
    JOIN civicrm_relationship_type crt
     ON crt.id = cr.relationship_type_id
    JOIN civicrm_contact cc
     ON cc.id = cr.contact_id_a
     AND cc.is_deleted <> 1
    LEFT JOIN civicrm_email ce
     ON ce.contact_id = cc.id
     AND ce.is_primary= 1
    LEFT JOIN civicrm_phone cp
     ON cp.contact_id = cc.id
     AND cp.is_primary= 1
    WHERE cr.case_id =  %1
     AND cr.is_active
     {$caseClientCondition}
    UNION
    SELECT cc.display_name as name, cc.sort_name as sort_name, cc.id, cr.relationship_type_id, crt.label_a_b as role, crt.name_a_b as role_name, ce.email, cp.phone
    FROM civicrm_relationship cr
    JOIN civicrm_relationship_type crt
     ON crt.id = cr.relationship_type_id
    JOIN civicrm_contact cc
     ON cc.id = cr.contact_id_b
     AND cc.is_deleted <> 1
    LEFT JOIN civicrm_email ce
     ON ce.contact_id = cc.id
     AND ce.is_primary= 1
    LEFT JOIN civicrm_phone cp
     ON cp.contact_id = cc.id
     AND cp.is_primary= 1
    WHERE cr.case_id =  %1
     AND cr.is_active
     {$caseClientCondition}
HERESQL;

   if ($caseClientCondition) {
      $params[2] = array(implode(',', $caseInfo['client_id']), 'CommaSeparatedIntegers');
    }

[colemanw]

That makes sense

[colemanw]

Unit tests for the Case api are here: https://github.com/civicrm/civicrm-core/blob/master/tests/phpunit/api/v3/CaseTest.php#L779 - that's the only one that mentions multi-client cases.

[tunbola]

@colemanw , Can you check this again? Thanks

[tunbola]

@colemanw Bump.

[demeritcowboy]

Looks good. There's some comments about the added test but doesn't affect the patch itself.

• General standards
  • [r-explain] PASS
  • [r-user] PASS
  • [r-doc] PASS
  • [r-run] PASS
    • The only other place this would come up is the Send Copy section on case activities but it ends up doing its own thing.
• Developer standards
  • [r-tech] PASS
  • [r-code] PASS
  • [r-maint] PASS with comments
    • A unit test was added and it fails before the patch and works after. So can remove the needs-test label and add has-test.
    • There's some code quality issues in the test but it doesn't block merging, but could be cleaned up eventually, e.g.
      • line 850 and line 859 do almost the same thing. Line 850 isn't needed.
      • line 860 setting the $_REQUEST variable can inadvertently affect other tests - there are other ways to create a relationship
      • line 878 unnecessary brackets
  • [r-test] Pending It passed before but just rerunning test since it wasn't recent. Jenkins test this please.

[colemanw]

Agree the test isn't ideal, but the other tests in that class are bad patterns so the problem isn't originating here.