Code fix and Test for dev/searchandreport#53

[alistairjames]

Overview

This change is to fix a problem in the Advanced Search described in this issue:
https://lab.civicrm.org/dev/report/-/issues/53

Before

An Advanced Search (constructed using the user interface) to collect Activities using the criteria:
Contact + ActivityType + ActivityStatus + CaseType
failed and collected all the Activities of the selected ActivityType and Status in any of the Cases involving the Contact.

The error was tracked to a missing join involving the civicrm_case_activity table. After manually testing, no other missing joins were found. (The same bug affected the results from searches constructed with the Search Builder).

After

The error was fixed by adding a helper method to append an additional join statement to the end of the fromClause when both civicrm_case and civicrm_activity were listed in the $tables array used to generate the fromClause in the method:
CRM/Contact/BAO/Query::fromClause()
The change also fixed the behaviour of the Search Builder, so that the correct list of Contacts was returned for searches of the same type as above.

Technical Details

Notes on the phpunit test created.

• The test checks more that just the construction of the fromClause in the SQL query, and includes running the query to check that the correct number of items is returned. This seemed a good idea even if it is therefore something more than a 'unit test'.
• The test is very dependent on the setup creating CaseTypes that contain default ActivityTypes when Cases are created from them.
• The test collects the different identifiers needed from the test database rather than hard coding them to try to make it tolerant of updates to the setup.
• No real attempt was made to get the use of exceptions correct.
• All the tests in tests/phpunit/CRM/Contact/AllTests.php still passed following these changes.

Comments

As someone quite new to PHP as a language. I am happy the fix works, and will not be at all offended if the code is radically restructured.

[civicrm-builder]

Can one of the admins verify this patch?

[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).

[eileenmcnaughton]

add to whitelist

[eileenmcnaughton]

I think I tried to add this one years ago & got told then I should do a local global gitignore https://docs.github.com/en/github/using-git/ignoring-files

[alistairjames]

Thank you for the guide. The suggested change was a sign of my lack of control over .gitignore rather than a serious recommendation.

[demeritcowboy]

@alistairjames There's a bunch of styling errors preventing the tests from running. Beside the red failure notice here click on Details and then click on warnings and then on details.

[alistairjames]

Ouch. I will work through this slowly and get it right - give me a little time.

[demeritcowboy]

I'm running into a crash when you choose to Display As Cases and then just expand the activities section (can choose something but don't even need to fill in any fields anywhere to see it). If I give the table an alias inside the added function it seems better, e.g. INNER JOIN civicrm_case_activity cca ON (cca.case_id = civicrm_case.id AND cca.activity_id = civicrm_activity.id). I haven't fully checked if that then changes something else.

And it's not technically wrong but style-wise usually civi doesn't have code that uses \n on multilines to split lines. Usually can just do something like this, without adding \n:

$join_statement = "
INNER JOIN civicrm_case_activity cca
  ON (cca.case_id = civicrm_case.id AND cca.activity_id = civicrm_activity.id)
";

[alistairjames]

I see the crash also. I am reasonably competent in SQL, but have never seen anything quite as involved as the query generated by the code, which I copied into issue #53 (https://lab.civicrm.org/dev/report/-/issues/53). So my proposed solution was always something of sticking plaster. I will step through to find where exactly the exception arises and report back.

The '\n' was just for readability of the SQL query generated and so not part of my code syntax, but it is helpful to know the standard PHP line continuation. I did get civilint to give a report of syntax errors, having failed to get PhpStorm configured correctly.

[eileenmcnaughton]

Getting phpstorm configured is definitely do-able - I've just reconfigured it for my docker instance - maybe something in here will help https://docs.civicrm.org/dev/en/latest/tools/phpstorm/

[alistairjames]

The SQL query generated with Display as Cases and the Activities section expanded fails when offered directly to the database in MySQL Workbench with Error Code 1066. Not unique table/alias: 'civicrm_case_activity'
This is because the table name appears in two different sections of the query. So the Query::fromClause() already inserts a join on civicrm_case_activity, before my extra addition.
I guess that means that a closer look at how the FROM clause is generated, and maybe a fix at a deeper level?
I will investigate.
Here is the failing SQL query (picked up from vendor/pear/db/DB/mysqli.php line 406 in simpleQuery()) reformatted for ease of reading and with civicrm_case_activity in bold:

SELECT count( DISTINCT civicrm_case.id ) as rowCount FROM civicrm_contact contact_a

LEFT JOIN civicrm_case_contact
ON civicrm_case_contact.contact_id = contact_a.id

INNER JOIN civicrm_case
ON civicrm_case_contact.case_id = civicrm_case.id

LEFT JOIN civicrm_relationship case_relationship
ON ( case_relationship.contact_id_a = civicrm_case_contact.contact_id
AND case_relationship.contact_id_b = 202
AND case_relationship.case_id = civicrm_case.id
OR case_relationship.contact_id_b = civicrm_case_contact.contact_id
AND case_relationship.contact_id_a = 202
AND case_relationship.case_id = civicrm_case.id )

LEFT JOIN civicrm_relationship_type case_relation_type
ON ( case_relation_type.id = case_relationship.relationship_type_id
AND case_relation_type.id = case_relationship.relationship_type_id )

LEFT JOIN civicrm_activity_contact
ON ( civicrm_activity_contact.contact_id = contact_a.id )

LEFT JOIN civicrm_activity
ON ( civicrm_activity.id = civicrm_activity_contact.activity_id
AND civicrm_activity.is_deleted = 0
AND civicrm_activity.is_current_revision = 1 )

INNER JOIN civicrm_contact
ON ( civicrm_activity_contact.contact_id = civicrm_contact.id and civicrm_contact.is_deleted != 1 )

INNER JOIN civicrm_case_activity
ON civicrm_case_activity.case_id = civicrm_case.id

INNER JOIN civicrm_activity case_activity
ON ( civicrm_case_activity.activity_id = case_activity.id
AND case_activity.is_current_revision = 1 )

LEFT JOIN civicrm_option_group option_group_case_status
ON (option_group_case_status.name = 'case_status')

LEFT JOIN civicrm_option_value case_status
ON (civicrm_case.status_id = case_status.value
AND option_group_case_status.id = case_status.option_group_id )

LEFT JOIN civicrm_case_type
ON civicrm_case.case_type_id = civicrm_case_type.id

INNER JOIN civicrm_case_activity
ON (civicrm_case_activity.case_id = civicrm_case.id
AND civicrm_case_activity.activity_id = civicrm_activity.id)

WHERE ( civicrm_activity.status_id IN ("1", "2")
AND civicrm_activity_contact.record_type_id = 3
AND civicrm_activity.is_test = 0
AND civicrm_case.is_deleted = 0
AND civicrm_case_contact.contact_id = contact_a.id )
AND ( 1 )
AND (contact_a.is_deleted = 0)
AND (civicrm_activity.activity_type_id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 58, 60, 62, 64, 66, 67, 68))

[alistairjames]

I have added a comment to https://lab.civicrm.org/dev/report/-/issues/53, and will revise this fix when I have shown there that I have found the correct place at which to amend the code.

I have added a request for suggestions now that I understand the underlying basis of the problem, and have some different ideas about how it could be fixed. I will update the code here after I have received some feedback and have something implemented.

[eileenmcnaughton]

Closing this as I think that was the conclusion over in gitlab