dev/report#53: search on relationship and case (2) #20002

jaapjansma · 2021-04-08T11:21:47Z

Overview

When doing an advanced search with case parameters set and displaying related contacts. Gives all clients of all the found cases which have this relationship. What we would expect is that related contact is linked to the found cases.

Steps to reproduce

Preparation:

Create a contact Contact A
Create another contact Contact B
Create a third contact Contact C
Create a case of type Housing Support for Contact A
On the case assign the role Benefit specialist is to Contact B
Create a second case of type Adult day care referral
On the case assign the role Benefit specialist is to Contact C

Searching:

Go to advanced search
Click on View contact as related contact
Select Benefit Specialist as relationship type
Go to tab cases and select Housing Support as case type

Expected results

I expected to see only Contact B as that one has a relationship on the case Housing Support.

Actual results

I see both Contact B and Contact C.

Comments

See also the discussion of a similar related topic: https://lab.civicrm.org/dev/report/-/issues/53

civibot · 2021-04-08T11:21:49Z

(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 · 2021-04-08T20:23:02Z

Can we get a unit test for this? We've been pretty strict for a long time about not having changes to advanced search without adding tests. I know we let your last one through without but we really should stick to it since this has been some of the hardest code to maintain.

The current failure should give a good starting point

CRM_Contact_SelectorTest::testSelectorQuery with data set #10 (array('Test display relationships', 'CRM_Contact_Selector', array(), array('1_b_a'), null, array(), 'advanced', 0, null, false, array('SELECT contact_a.id as contac...egion`', 'WHERE displayRelType.relation...d = 0)')))
different sql
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'WHERE displayRelType.relationship_type_id = 1 AND displayRelType.is_active = 1 AND ( 1 ) AND (contact_a.is_deleted = 0)'
+'AND displayRelType.relationship_type_id = 1 AND displayRelType.is_active = 1 AND ( 1 ) AND (contact_a.is_deleted = 0)'

/home/jenkins/bknix-dfl/build/core-20002-a4wl/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php:2667
/home/jenkins/bknix-dfl/build/core-20002-a4wl/web/sites/all/modules/civicrm/tests/phpunit/CRM/Contact/SelectorTest.php:62
/home/jenkins/bknix-dfl/build/core-20002-a4wl/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php:226
/home/jenkins/bknix-dfl/extern/phpunit7/phpunit7.phar:615

jaapjansma · 2021-04-09T13:13:51Z

I have fixed the failing test.
And I have added a test for this specific use case.

demeritcowboy · 2021-04-09T16:20:06Z

tests/phpunit/CRM/Case/BAO/QueryTest.php

+    $clientContactID = $this->individualCreate(['first_name' => 'John', 'last_name' => 'Smith']);
+    $benefitSpecialist1 = $this->individualCreate(['Individual', 'first_name' => 'Alexa', 'last_name' => 'Clarke']);
+    $benefitSpecialist2 = $this->individualCreate(['Individual', 'first_name' => 'Sandra', 'last_name' => 'Johnson']);
+    $housingSupportCase = $this->createCase($clientContactID, NULL, ['case_type_id' => 'housing_support', 'case_type_id' => 1]);


I think this is a typo: case_type_id used twice.

Yes indeed is a typo

demeritcowboy · 2021-04-09T16:26:41Z

CRM/Contact/BAO/Query.php

@@ -5837,10 +5837,11 @@ public function filterRelatedContacts(&$from, &$where, &$having) {
      else {
        $from .= $qcache['from'];
      }
-      $where = $qcache['where'];
+      $where .= $qcache['where'];


I kind of remember this code area from the earlier PR but I don't remember if there was ever a time when $where was passed in to the function and is blank or didn't have "WHERE" in it already. I wonder if it's worth adding a guard against that, e.g. if $where does not contain "WHERE", then $where = "WHERE (1) " . $qcache['where']. Or something like that - just an example.

I have added a check whether $where is empty.

demeritcowboy · 2021-04-18T00:20:38Z

In case the jenkins log expires:

CRM_Contact_SelectorTest::testSelectorQuery with data set #11 (array('Test display relationships', 'CRM_Contact_Selector', array(), array('1_b_a'), null, array(), 'advanced', 0, null, false, array('SELECT contact_a.id as contac...egion`', 'AND displayRelType.relationsh...d = 0)')))
different sql
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'AND displayRelType.relationship_type_id = 1 AND displayRelType.is_active = 1 AND ( 1 ) AND (contact_a.is_deleted = 0)'
+'WHERE 1 AND displayRelType.relationship_type_id = 1 AND displayRelType.is_active = 1 AND ( 1 ) AND (contact_a.is_deleted = 0)'

/home/jenkins/bknix-dfl/build/core-20002-h980/web/sites/all/modules/civicrm/tests/phpunit/CRM/Contact/SelectorTest.php:66

jaapjansma · 2021-04-19T10:58:51Z

Thanks for storing the jenkins log. I have fixed the failing test.

demeritcowboy · 2021-04-19T13:50:01Z

Thanks @jaapjansma I think this is ok to merge.

General standards
- [r-explain] PASS
- [r-user] PASS
- [r-doc] PASS
- [r-run] PASS
  - Can reproduce and this fixes it.
Developer standards
- [r-tech] PASS
  - I'm going to rely on my earlier look at this code area where I said:
    
    There's already code that does str_replace and if strpos, so it isn't doing something new. ... This code is only ever called when _displayRelationshipType is set to something, and I'm pretty sure, at least in core, that this only happens from the advanced search form (via CRM_Contact_Selector). It's technically possible to call CRM_Contact_BAO_Query::__construct in a way you could end up here, but just browsing a few I didn't see any. I didn't see a way to trigger this via api.
- [r-code] PASS
- [r-maint] PASS
- [r-test] PASS

eileenmcnaughton · 2021-04-19T19:42:39Z

merging based on @demeritcowboy review

eileenmcnaughton · 2021-08-02T19:38:48Z

Just noting this appears to have caused a regression - #20997

eileenmcnaughton · 2021-08-05T01:49:56Z

@jaapjansma we made the call to revert this for now in order to fix the regression & get the releases out. @jmcclelland added a test for the regression so we can be confident it won't regress again. Note your test was not reverted - just marked incomplete for now.

I will re-open in gitlab

eileenmcnaughton · 2021-08-05T01:55:14Z

Actually the original issue had a whole lot of stuff in it not just this so I opened https://lab.civicrm.org/dev/report/-/issues/72 to track

Issue 53 (2)

4f4fb80

civibot bot added the master label Apr 8, 2021

eileenmcnaughton added the needs-test label Apr 8, 2021

jaapjansma added 2 commits April 9, 2021 15:12

Fix failing test

c07cc62

Added new test for advanced search

2b59356

demeritcowboy reviewed Apr 9, 2021

View reviewed changes

eileenmcnaughton added has-test and removed needs-test labels Apr 9, 2021

Fixed typo and added check for whether is empty

afbc869

Fixed failing test

7ebe0c1

eileenmcnaughton merged commit 8d7a356 into civicrm:master Apr 19, 2021

This was referenced Aug 4, 2021

Ensure advanced search displaying related contacts works consitently #20997

Closed

Temporary revert of changes in 20002 which caused a regression #21016

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/report#53: search on relationship and case (2) #20002

dev/report#53: search on relationship and case (2) #20002

jaapjansma commented Apr 8, 2021

civibot bot commented Apr 8, 2021

eileenmcnaughton commented Apr 8, 2021

jaapjansma commented Apr 9, 2021

demeritcowboy Apr 9, 2021

jaapjansma Apr 12, 2021

demeritcowboy Apr 9, 2021

jaapjansma Apr 12, 2021

demeritcowboy commented Apr 18, 2021

jaapjansma commented Apr 19, 2021

demeritcowboy commented Apr 19, 2021

eileenmcnaughton commented Apr 19, 2021

eileenmcnaughton commented Aug 2, 2021

eileenmcnaughton commented Aug 5, 2021

eileenmcnaughton commented Aug 5, 2021

dev/report#53: search on relationship and case (2) #20002

dev/report#53: search on relationship and case (2) #20002

Conversation

jaapjansma commented Apr 8, 2021

Overview

Steps to reproduce

Expected results

Actual results

Comments

civibot bot commented Apr 8, 2021

eileenmcnaughton commented Apr 8, 2021

jaapjansma commented Apr 9, 2021

demeritcowboy Apr 9, 2021

Choose a reason for hiding this comment

jaapjansma Apr 12, 2021

Choose a reason for hiding this comment

demeritcowboy Apr 9, 2021

Choose a reason for hiding this comment

jaapjansma Apr 12, 2021

Choose a reason for hiding this comment

demeritcowboy commented Apr 18, 2021

jaapjansma commented Apr 19, 2021

demeritcowboy commented Apr 19, 2021

eileenmcnaughton commented Apr 19, 2021

eileenmcnaughton commented Aug 2, 2021

eileenmcnaughton commented Aug 5, 2021

eileenmcnaughton commented Aug 5, 2021