Allow columns in relationship table to be modified by searchColumns hook

[mattwire]

Overview

Allow columns in relationship table to be modified by searchColumns hook. This allows us to modify the columns that are shown to the user on the Contact relationships tab.

Before

Not possible to modify relationship tab columns without changing templates/php.

After

Can modify columns by implementing searchColumns hook.

Technical Details

Implemented in standard way as done by other entities

Comments

@eileenmcnaughton There's a couple of bits in here that would probably be good to take further into an EntityPageTrait or similar. I've done a bit of work towards that here.

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

[jitendrapurohit]

Tested the PR. works great except that before this change the rows were sorted by relationship type id by default, but now it is sorted by relationship type labels? @mattwire

The addition of searchColumns hook works as desired.

Thanks.

[eileenmcnaughton]

Why do we need the hook here - in other places it is very centralised - but they do seem to be actually using the selector whereas I think you are mocking it?

[mattwire]

We are mocking it and there are two different functions - the page loads with the columns here. Later the datatable loads the rows via an ajax call.

[eileenmcnaughton]

Good spotting @pradpnayak - my instinct is that relationship type labels is at least as good as relationship_type_id - which seems kinda arbitrary.

My concern is the 2 different contexts - 'relationship.rows' & 'relationship.columns' - there doesn't seem to be a precedent for that?

The cleanup looks good

[yashodha]

@mattwire You might want to rebase the PR. +1 for the improvement :)

[mattwire]

Rebased!

[yashodha]

@mattwire Thanks! Do we have documentation for the hook to got with this?

[mattwire]

@yashodha Yes documentation is (accidently) already merged - see https://github.com/civicrm/civicrm-dev-docs/issues/609 If you are happy with those docs we could merge this and close that issue?

[yashodha]

@mattwire sure, will check

[mattwire]

I've flagged this merge-ready as I think we've got confirmation from reviewers that it's good to merge - just need the merger to confirm the documentation is ok.

[JoeMurray]

Matt, can you explain how you responded to @eileenmcnaughton 's question above

My concern is the 2 different contexts - 'relationship.rows' & 'relationship.columns' - there doesn't seem to be a precedent for that?

I didn't seek what the AddEntityPage stuff just under it meant.

[yashodha]

@mattwire Can you please rebase this PR?

[mattwire]

Matt, can you explain how you responded to @eileenmcnaughton 's question above

My concern is the 2 different contexts - 'relationship.rows' & 'relationship.columns' - there doesn't seem to be a precedent for that?

@JoeMurray @eileenmcnaughton Sorry :-) There probably isn't a precedent because this is the first time we're mocking the search selector instead of using it directly (as relationships is not using it).
Because we are using datatables for display there are two calls:

1. When loading the page we need to load the column headers but not the data - relationship.columns hook is triggered. We could detect that the rows variable is NULL but it seems cleaner to me to actually have a different selector.
2. When the page has loaded, the data is retrieved by an AJAX call - relationship.rows hook is triggered.

Is that ok?

[eileenmcnaughton]

We do have a precedent - it's just different - on the contribution tpl the headers are assigned to the template as an array & iterated - as you do. However, we don't provide a hook for that - we leave people to do it in another hook as a smarty assign. This feels like a compromise to me - you can achieve what you want but it's clear it is only quasi-supported - which I think is appropriate because I could see the code being cleaned up later.

If you ditched
a) the weird caching of a static array & just returned it
b) the hook

I would merge the cleanup to the tpl & you can see whether you really need this hook or whether you can assign the values to the tpl from an existing hook

[mattwire]

If you ditched
a) the weird caching of a static array & just returned it

Done

b) the hook

I would merge the cleanup to the tpl & you can see whether you really need this hook or whether you can assign the values to the tpl from an existing hook

@eileenmcnaughton relationship.rows is definitely required as it's an AJAX call and no forms/templates/smarty is involved.
relationship.columns could probably be fiddled into a template assign via hook_civicrm_pageRun() but that feels really un-intuitive and if the columns don't match the rows exactly the page will throw datatable errors.
The expectation from the searchColumns hooks is that it modifies both (https://docs.civicrm.org/dev/en/latest/hooks/hook_civicrm_searchColumns/):

This hook is called after a search is done, allowing you to modify the headers and/or the values that are displayed as part of the search.

If you prefer, we could simplify the selector and just use relationship instead of relationship.rows / relationship.columns as we can easily detect the empty or otherwise array of rows but I do think the handling of the columns / rows needs to be in the same place - ie. the same hook otherwise you end up with something required for the same functionality split across two seemingly unrelated hooks.

See here for an example implementation: https://github.com/mattwire/civicrm-hrgb/blob/master/hrgb.php#L160

[JoeMurray]

I'm okay with an implementation that makes use of params passed in via an existing hook to do more than one thing.

[yashodha]

@eileenmcnaughton @JoeMurray @mattwire
Are we ready to merge this or there are more changes to be done here?

[yashodha]

@mattwire Does this well with user dashboard as well?

[yashodha]

@jitendrapurohit Did you test user dashboard for this PR as well?

[mattwire]

@yashodha Where is this shown on userdashboard?

[yashodha]

@mattwire Please check screenshot

or this link https://dmaster.demo.civicrm.org/civicrm/user?reset=1&id=114&cid=114

[yashodha]

@mattwire
This piece of code is missing for this to work on user dashboard as well.

diff --git a/CRM/Contact/Page/View/UserDashBoard.php b/CRM/Contact/Page/View/UserDashBoard.php
index 7b98111ede..25787e6882 100644
--- a/CRM/Contact/Page/View/UserDashBoard.php
+++ b/CRM/Contact/Page/View/UserDashBoard.php
@@ -141,6 +141,11 @@ class CRM_Contact_Page_View_UserDashBoard extends CRM_Core_Page {
 
     // CRM-16512 - Hide related contact table if user lacks permission to view self
     if (!empty($this->_userOptions['Permissioned Orgs']) && CRM_Core_Permission::check('view my contact')) {
+     $columnHeaders = CRM_Contact_BAO_Relationship::getColumnHeaders();
+     $contactRelationships = $selector = NULL;
+     CRM_Utils_Hook::searchColumns('relationship.columns', $columnHeaders, $contactRelationships, $selector);
+     $this->assign('columnHeaders', $columnHeaders);
+
       $dashboardElements[] = array(
         'class' => 'crm-dashboard-permissionedOrgs',
         'templatePath' => 'CRM/Contact/Page/View/RelationshipSelector.tpl',

[yashodha]

@eileenmcnaughton I have removed merge ready tag as the proposed changes need to be done for this to work on userdashboard as well.

[mattwire]

@yashodha Thanks for review + patch. I've added and pushed your changes. @seamuslee001

[seamuslee001]

Given the discussion and the fact that Matt has reponded to Yash's review this looks fine to me. Merging

[yashodha]

@seamuslee001 Thanks! It works fine.