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

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

Merged
merged 3 commits into from
Apr 8, 2020
Merged

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9cf08be
dev/core#1659: Set the param type for client Id.
tunbola Mar 18, 2020
d32a853
dev/core#1659: Implement logic case client condition SQL condition.
tunbola Mar 24, 2020
22d70ba
dev/core#1659: Add test to check that Case clients are not returned i…
tunbola Mar 24, 2020
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 @@ -1176,9 +1176,11 @@ public static function getRelatedContacts($caseID, $includeDetails = TRUE) {
AND cr.is_active
AND cc.id NOT IN (%2)
HERESQL;

$clientIdType = !empty($caseInfo['client_id']) ? 'CommaSeparatedIntegers' : 'String';
$params = [
1 => [$caseID, 'Integer'],
2 => [implode(',', $caseInfo['client_id']), 'String'],
2 => [implode(',', $caseInfo['client_id']), $clientIdType],
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@tunbola tunbola Mar 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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'];
}

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
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');
    }

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense

];
$dao = CRM_Core_DAO::executeQuery($query, $params);

Expand Down