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.

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

SearchKit - Add ability to join on multi-select ContactRef fields #20764

Merged
merged 5 commits into from Jul 7, 2021

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Jul 4, 2021

Overview

Improves support for custom contactRef fields in APIv4, SearchKit, and Afform. Makes it possible to save, search for, and filter by serialized (multi-select) custom ContactRef fields.

Before

  • Bug prevents saving custom contactRef data in APIv4
  • Serialized custom contactRef outputs as numbers rather than display names in SearchKit
  • Not possible to create links for serialized contactRef fields in SearchKit
  • Bug gave incorrect results when filtering with a multiselect afform contactRef field

After

  • Saving custom contactRef data in APIv4 fixed, with test
  • Serialized custom contactRef outputs display names in SearchKit
  • Possible to create links for serialized contactRef fields in SearchKit
  • Fixed filtering with a multiselect afform contactRef field, added test
  • Also made it possible to use a text field instead of an autocomplete-select as a contactRef search filter in Afform

Comments

This also reverts #20156 as the limitation has been lifted.

This allows the API to return e.g. an array of display names from a serialized contact reference field.
@civibot
Copy link

civibot bot commented Jul 4, 2021

(Standard links)

@civibot civibot bot added the master label Jul 4, 2021
@colemanw colemanw changed the title Multi contact ref joins SearchKit - Add ability to join on multi-select ContactRef fields Jul 4, 2021
@eileenmcnaughton
Copy link
Contributor

@kurund @michaelmcandrew is this something you are able to test?

@kurund
Copy link
Contributor

kurund commented Jul 5, 2021

@kurund @michaelmcandrew is this something you are able to test?

Yup, will check it later today.

@eileenmcnaughton
Copy link
Contributor

thanks @kurund - it's good to merge once you OK it

@kurund
Copy link
Contributor

kurund commented Jul 6, 2021

@colemanw

I am getting this weird behavior after applying the patch on nightly build.

search-listing

I had to manually apply the patch as auto apply failed :(

I will recheck again if I missed something.

@colemanw
Copy link
Member Author

colemanw commented Jul 6, 2021

What nightly build did you use?
Have you cleared caches and run upgrade scripts?

@kurund
Copy link
Contributor

kurund commented Jul 6, 2021

What nightly build did you use?

civicrm-NIGHTLY-drupal.tar.gz ( 5.40.alpha1-202107051700 )

Have you cleared caches and run upgrade scripts?

Yes, cache cleared and upgrade went well.

@kurund
Copy link
Contributor

kurund commented Jul 6, 2021

I did a fresh setup and above errors are gone. Let me test the functionality and get back to you.

@kurund
Copy link
Contributor

kurund commented Jul 6, 2021

@colemanw

Overall patch is working as expected. I found few minor glitches.

a. Now we can add contact reference autocomplete as filter and is working fine. Only issue is that it's not respecting the configuration settings of autocomplete. eg: if the contact field has advanced filter 'action=get&contact_type=Organization' it still shows all the contacts.

b. I was trying to replicate activity tab and 'With Contact' does have individual clickable links but contact id is not evaluated. for example http://localhost/civicrm/contact/view?reset=1&cid=[Activity_ActivityContact_Contact_01.id]

c. For one of the search I am using contact sub type as display column. It is showing correct result in Compose Search however in the table display it just shows blank.

I will do few more tests later update here.

@colemanw
Copy link
Member Author

colemanw commented Jul 6, 2021

@kurund thanks for the testing

a. I think this is a related issue but should be a separate PR. It's a little tricky because traditional contact reference fields don't use the same callback as EntityRef fields, but afform uses EntityRef.

b. I couldn't reproduce this. Can you create a new search to demonstrate the problem and paste the url fragment here? For example when I tested, my search was
civicrm/admin/search#/create/Activity?params=%7B"version":4,"select":%5B"id","subject","Activity_ActivityContact_Contact_01.display_name"%5D,"orderBy":%7B%7D,"where":%5B%5D,"groupBy":%5B%5D,"join":%5B%5B"Contact%20AS%20Activity_ActivityContact_Contact_01","LEFT","ActivityContact",%5B"id","%3D","Activity_ActivityContact_Contact_01.activity_id"%5D,%5B"Activity_ActivityContact_Contact_01.record_type_id:name","%3D","%5C"Activity%20Targets%5C""%5D%5D%5D,"having":%5B%5D%7D

c. I couldn't reproduce this either. Can you paste a url fragment of a search to reproduce?

@colemanw
Copy link
Member Author

colemanw commented Jul 6, 2021

FYI I just noticed a problem where the admin search screen was formatting every field as a link instead of just label fields like display_name. Fixed and rebased.

@kurund
Copy link
Contributor

kurund commented Jul 7, 2021

c. I have created a contact list with related contacts and trying to display contact sub type of related contact.

Compose Search (where it is shown correctly)

{"version":4,"select":["row_count"],"orderBy":[],"where":[["contact_type:name","=","Organization"]],"groupBy":[],"join":[["Contact+AS+Contact_RelationshipCache_Contact_01","LEFT","RelationshipCache",["id","=","Contact_RelationshipCache_Contact_01.far_contact_id"],["Contact_RelationshipCache_Contact_01.near_relation:name","IN",["Manufactured+by","Supplied+by"]]]],"having":[],"limit":100,"offset":0}

Table view (not showing sub type)
{"return":"row_count","savedSearch":{"id":9,"name":"Contacts","label":"Contacts+Search","form_values":null,"mapping_id":null,"search_custom_id":null,"api_entity":"Contact","api_params":{"version":4,"select":["id","display_name","Contact_RelationshipCache_Contact_01.far_relation:label","Contact_RelationshipCache_Contact_01.display_name","Contact_RelationshipCache_Contact_01.contact_sub_type:label"],"orderBy":[],"where":[["contact_type:name","=","Organization"]],"groupBy":[],"join":[["Contact+AS+Contact_RelationshipCache_Contact_01","LEFT","RelationshipCache",["id","=","Contact_RelationshipCache_Contact_01.far_contact_id"],["Contact_RelationshipCache_Contact_01.near_relation:name","IN",["Manufactured+by","Supplied+by"]]]],"having":[]},"created_id":18742,"modified_id":18742,"expires_date":null,"created_date":"2021-07-07+14:14:15","modified_date":"2021-07-07+14:24:58","description":null,"groups":[],"displays":[{"id":11,"name":"Contact_Search","label":"Contact+Search","saved_search_id":9,"type":"table","settings":{"limit":50,"pager":true,"columns":[{"key":"id","label":"Contact+ID","dataType":"Integer","type":"field"},{"key":"display_name","label":"Display+Name","dataType":"String","type":"field"},{"key":"Contact_RelationshipCache_Contact_01.far_relation:label","label":"Contact+Related+Contacts:+Relationship+from+contact","dataType":"String","type":"field"},{"key":"Contact_RelationshipCache_Contact_01.display_name","label":"Contact+Related+Contacts:+Display+Name","dataType":"String","type":"field"},{"key":"Contact_RelationshipCache_Contact_01.contact_sub_type:label","label":"Contact+Related+Contacts:+Contact+Subtype","dataType":"String","type":"field"}]},"acl_bypass":false}]},"display":{"id":11,"name":"Contact_Search","label":"Contact+Search","saved_search_id":9,"type":"table","settings":{"limit":50,"pager":true,"columns":[{"key":"id","label":"Contact+ID","dataType":"Integer","type":"field"},{"key":"display_name","label":"Display+Name","dataType":"String","type":"field"},{"key":"Contact_RelationshipCache_Contact_01.far_relation:label","label":"Contact+Related+Contacts:+Relationship+from+contact","dataType":"String","type":"field"},{"key":"Contact_RelationshipCache_Contact_01.display_name","label":"Contact+Related+Contacts:+Display+Name","dataType":"String","type":"field"},{"key":"Contact_RelationshipCache_Contact_01.contact_sub_type:label","label":"Contact+Related+Contacts:+Contact+Subtype","dataType":"String","type":"field"}]},"acl_bypass":false},"sort":[],"filters":{},"afform":null}

@kurund
Copy link
Contributor

kurund commented Jul 7, 2021

b. I couldn't reproduce this. Can you create a new search to demonstrate the problem and paste the url fragment here?

Table view

{"return":"row_count","savedSearch":{"id":10,"name":"Transfer_Search","label":"Transfer+Search","form_values":null,"mapping_id":null,"search_custom_id":null,"api_entity":"Activity","api_params":{"version":4,"select":["id","subject","GROUP_CONCAT(DISTINCT+Activity_ActivityContact_Contact_01.display_name)+AS+GROUP_CONCAT_DISTINCT_Activity_ActivityContact_Contact_01_display_name"],"orderBy":[],"where":[["activity_type_id:name","=","Transfer"]],"groupBy":["id"],"join":[["Contact+AS+Activity_ActivityContact_Contact_01","INNER","ActivityContact",["id","=","Activity_ActivityContact_Contact_01.activity_id"],["Activity_ActivityContact_Contact_01.record_type_id:name","=","\"Activity+Targets\""]]],"having":[]},"created_id":18742,"modified_id":18742,"expires_date":null,"created_date":"2021-07-07+14:34:15","modified_date":"2021-07-07+14:37:06","description":null,"groups":[],"displays":[{"id":12,"name":"Transfer_Search","label":"Transfer+Search","saved_search_id":10,"type":"table","settings":{"limit":50,"pager":true,"columns":[{"key":"id","label":"Activity+ID","dataType":"Integer","type":"field"},{"key":"subject","label":"Subject","dataType":"String","type":"field"},{"key":"GROUP_CONCAT_DISTINCT_Activity_ActivityContact_Contact_01_display_name","label":"Activity+Contacts:+(List)+Display+Name","dataType":"String","type":"field","link":{"path":"civicrm/contact/view?reset=1&cid=[Activity_ActivityContact_Contact_01.id]"},"title":"View+Activity+Contacts"}]},"acl_bypass":false}]},"display":{"id":12,"name":"Transfer_Search","label":"Transfer+Search","saved_search_id":10,"type":"table","settings":{"limit":50,"pager":true,"columns":[{"key":"id","label":"Activity+ID","dataType":"Integer","type":"field"},{"key":"subject","label":"Subject","dataType":"String","type":"field"},{"key":"GROUP_CONCAT_DISTINCT_Activity_ActivityContact_Contact_01_display_name","label":"Activity+Contacts:+(List)+Display+Name","dataType":"String","type":"field","link":{"path":"civicrm/contact/view?reset=1&cid=[Activity_ActivityContact_Contact_01.id]"},"title":"View+Activity+Contacts"}]},"acl_bypass":false},"sort":[],"filters":{},"afform":null}

Each activity contacts have links like: http://localhost/civicrm/contact/view?reset=1&cid=[Activity_ActivityContact_Contact_01.id]

@kurund
Copy link
Contributor

kurund commented Jul 7, 2021

a. I think this is a related issue but should be a separate PR. It's a little tricky because traditional contact reference fields don't use the same callback as EntityRef fields, but afform uses EntityRef.

Thanks, that make sense.

@colemanw
Copy link
Member Author

colemanw commented Jul 7, 2021

@kurund I meant the url you see at the top of your screen when you compose a search. For a new search (before hitting Save) you'll see the url change as you update criteria.
For an existing search, you can click the "Clone" button and it will bring you to an unsaved search with those criteria in the url.

@kurund
Copy link
Contributor

kurund commented Jul 7, 2021

b

http://localhost/civicrm/admin/search#/create/Activity?params=%7B%22version%22:4,%22select%22:%5B%22id%22,%22subject%22,%22GROUP_CONCAT(DISTINCT%20Activity_ActivityContact_Contact_01.display_name)%20AS%20GROUP_CONCAT_DISTINCT_Activity_ActivityContact_Contact_01_display_name%22%5D,%22orderBy%22:%5B%5D,%22where%22:%5B%5B%22activity_type_id:name%22,%22%3D%22,%22Transfer%22%5D%5D,%22groupBy%22:%5B%22id%22%5D,%22join%22:%5B%5B%22Contact%20AS%20Activity_ActivityContact_Contact_01%22,%22INNER%22,%22ActivityContact%22,%5B%22id%22,%22%3D%22,%22Activity_ActivityContact_Contact_01.activity_id%22%5D,%5B%22Activity_ActivityContact_Contact_01.record_type_id:name%22,%22%3D%22,%22%5C%22Activity%20Targets%5C%22%22%5D%5D%5D,%22having%22:%5B%5D%7D

c.

http://localhost/civicrm/admin/search#/create/Contact?params=%7B%22version%22:4,%22select%22:%5B%22id%22,%22display_name%22,%22Contact_RelationshipCache_Contact_01.far_relation:label%22,%22Contact_RelationshipCache_Contact_01.display_name%22,%22Contact_RelationshipCache_Contact_01.contact_sub_type:label%22%5D,%22orderBy%22:%5B%5D,%22where%22:%5B%5B%22contact_type:name%22,%22%3D%22,%22Organization%22%5D%5D,%22groupBy%22:%5B%5D,%22join%22:%5B%5B%22Contact%20AS%20Contact_RelationshipCache_Contact_01%22,%22LEFT%22,%22RelationshipCache%22,%5B%22id%22,%22%3D%22,%22Contact_RelationshipCache_Contact_01.far_contact_id%22%5D,%5B%22Contact_RelationshipCache_Contact_01.near_relation:name%22,%22IN%22,%5B%22Manufactured%20by%22,%22Supplied%20by%22%5D%5D%5D%5D,%22having%22:%5B%5D%7D

Hope this helps!

@colemanw colemanw added the merge ready PR will be merged after a few days if there are no objections label Jul 7, 2021
@colemanw
Copy link
Member Author

colemanw commented Jul 7, 2021

Yes that helped. I can now reproduce b and c. On investigation, I don't think either should block this PR.

B. Is out-of-scope, as this PR fixes links for multi-select fields, not single fields aggregated by grouping. I do hope to tackle that issue at some point, but this PR is already quite large.

C. is a pre-existing condition. I was able to reproduce it in a test and will work on a fix.
UPDATE: here is the fix: #20799

Based on the review & testing I think this is merge-ready.

@colemanw
Copy link
Member Author

colemanw commented Jul 7, 2021

retest this please

@eileenmcnaughton
Copy link
Contributor

Thanks for the testing @kurund - I wonder how much it feels like things have moved on in your time away from Civi - hopefully a little bit :-)

Anyway - I'm upping this to merge-on-pass after reading through the above

@colemanw
Copy link
Member Author

colemanw commented Jul 7, 2021

Unrelated test fail.

@colemanw colemanw merged commit 2beecf9 into civicrm:master Jul 7, 2021
@eileenmcnaughton eileenmcnaughton deleted the multiContactRefJoins branch July 7, 2021 23:37
@kurund
Copy link
Contributor

kurund commented Jul 8, 2021

@colemanw @eileenmcnaughton

Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-test master merge on pass merge ready PR will be merged after a few days if there are no objections
Projects
None yet
3 participants