CRM-20207 Added invocation for selectWhereClause hook in activity SQL

[Edzelopez]

---

• CRM-20207: Introduce selectWhereClause hook for activity results on activity tab contact summary page
  https://issues.civicrm.org/jira/browse/CRM-20207

[eileenmcnaughton]

I don't think we should be adding anything to that function - I think we should be trying to remove it in favour of more predictable api calls - eg. something along the lines of this

eileenmcnaughton@8dd8e64

(note that doesn't 100% work, the api join syntax would need to be used to get a few extra fields to display, + unit tests needed, however, I just wanted to try quickly to see how integral that function is)

[eileenmcnaughton]

@colemanw input welcome!

[eileenmcnaughton]

I changed to WIP because I think the approach here it to add a hook to a toxic function & we can call the api instead, (with a little more work) and remove the toxic code

[JoeMurray]

@eileenmcnaughton I think we need to have a common understanding of what is toxic code and what is reasonable to request in terms of a refactoring. I'm very comfortable if you have confidence in your approach to go that way. But I don't have a good enough appreciation for implications of taking out all that code. Are you requesting other folks do this kind of refactoring? @colemanw comments welcome

[eileenmcnaughton]

Yeah - I think we need to be very careful when adding hooks that we do research first to understand what way the code is going in, because hooks lock in bad code. A first step that I take when trying to assess if code is consistent with more recent efforts is a git blame, to see how recently it has been touched & who has touched it. Older code is more likely to be legacy. Code with lots of hand-written sql in it is likely to be legacy or undesirable, code using the api is likely to be recent, code written by Coleman is likely to be gold standard. It's worth asking the question when touching the form layer, in particular, if the work could be done using the api.

ACL hooks are a particular place where we don't want to go down the wrong path. Ideally when first looking into the code areas devs go on chat & say what they are trying to do & try to get ideas. It doesn't always work, but sometimes it does.

In this case there is a function with lots of hand-written sql code that was imported from svn, that is the subject of another open pr (where I said it should be re-written, although I didn't suggest switching to the api at that stage) & using api explorer it seems it we can do the same get with the api & in general, we know the api is where all the hook work will be directed.

The alternative I suggested was more of a quick draft than final code - as it does not include all the fields yet : eileenmcnaughton@8dd8e64

[JoeMurray]

Monish wrote me offline:

Hi Joe Murray
As per discussion on #9916 we need to refactor the CRM_Activity_BAO_Activity::getActivities(...) and getActivitiesCount(..) by removing SQL code and call API instead. With this, it will also resolve our purpose to call selectWhereClause hook as eventually all the hooks are directed through APIs. But in order to call selectWhereClause the get API must include the "check_permissons=1" parameter. However, I found 2 limitations in doing so, which are:

1. SQL code was written to fetch desired activities with some extra fields like case.*, mailing ID etc, Via Activity.get API it is not possible to fetch all the fields from single API.
2. There is no get API for civicrm_case_activity table so we need to implement that so to use for Activity.get API chain to fetch extra case fields.
   What needs to be done:
3. Remove SQL code and use Activity.get API instead
4. Use api chain to fetch extra fields like contact name, case ID/name etc.
5. Implement CaseActivity.get API
6. CRM unit test to call our desired BAO function.
   My rough estimate(including CRM unit tests) will be ~20hrs (2.5 days)

Edsel responded:
I think we would need to beef up the estimate if we are going to implement a new API since we would need a bunch of test cases to go with it, including adding tests for the new functions which will replace CRM_Activity_BAO_Activity::getActivities(...) and getActivitiesCount(..).

My sense:
We're not really in a position to fund this scope of work at this point as much of it is unrelated to our needs. Is there a way to scope this waay down to an initial step in this direction?

[JoeMurray]

I'm going to get an estimate for replacing SQL with existing API call and using SQL to retrieve missing pieces in the absence of a CaseActivity.get API.

[colemanw]

To a couple of the above points, I've recently implemented case_id as a param in the activity api.

[eileenmcnaughton]

@colemanw my understanding is that there effectively IS a caseActivity api based on the work you have done?

The existing code appears to go to great lengths to be efficient, but then includes unindexed joins on the option_value table

[eileenmcnaughton]

I guess the minimum here would be getting some unit tests in place to ensure the ACL works & also testing the options that can be passed into the function.

[eileenmcnaughton]

oh - & the tests should test getContactActivities & count - not the mysql function - that needs to be littered with comments saying - if you want to change this further you need to fix it to use the api

[JoeMurray]

We should really index the optionValue field! Actually, I confirmed https://github.com/civicrm/civicrm-core/blob/master/xml/schema/Core/OptionValue.xml#L204 both it and name are indexed, so I imagine the issue is that it is being joined to fields that are not indexed.

[JoeMurray]

FWIW, the JOINs at eileenmcnaughton@8dd8e64#diff-a35044abd0f538e4af551c5ae039692dL1088 and L1129 can be greatly improved by changing
left join civicrm_option_value on civicrm_activity.activity_type_id = civicrm_option_value.value
to
left join civicrm_option_value on civicrm_activity.activity_type_id = CAST(civicrm_option_value.value AS UNSIGNED). Still not optimal, but much better. A bigger but better change to the query would replace the civicrm_option_value table, unfortunately not aliased in the query, with
(SELECT CAST(civicrm_option_value.value AS UNSIGNED) as value_int, * FROM civicrm_option_value) as ov on civicrm_activity.activity_type_id = ov.value_int

[JoeMurray]

@Edzelopez and @monishdeb, could you provide updated estimate given @colemanw's work means it's not necessary to create an API for CaseActivity, nor the associated tests? Seems like we can retrieve all needed fields using chained API calls.

[eileenmcnaughton]

@JoeMurray the reason the join is not indexed is, as you identify, due to casting. Casting on the join resolution won't make it an indexed join :-(

We resolved this around 4.4 by switching to using pseudoconstants to resolve our values from the option_value table & then using those. In most cases simply switching to using the api solves the issue, since it was solved at the api layer a while back.

[monishdeb]

@JoeMurray yes now we can fetch all desired extra fields by using Activity.get with some api chaining and after discussing with @Edzelopez, my estimate will be 22-24 hr including CRM UT(unit test) on those two BAO functions and also one UT to ensure ACL works.

[JoeMurray]

None of this is paid work. Give it a medium priority.

[JoeMurray]

Actually, it's needed work just unfunded. So give it high priority.

[colemanw]

I just took a closer look and was surprised to discover that the function is only called in 2 places. The best course of action may be to just get rid of it and use the api from those 2 places instead.

@monishdeb @eileenmcnaughton

[monishdeb]

@colemanw are you referring to getActivitySQLClause(...) function?

In my recent commit 2a11177 I have replaced it with activity.get API call here with some desired filters, ensure that there aren't regression due to refactoring. However I was unable to fix few issues listed here

I have marked it with WIP since I am going to add associated CRM unit test for getActivities(...) and another UT to ensure selectWhereClause hook is supported on activity list through Activity.get api as check_permissions = 1 is used here

[colemanw]

@monishdeb I mean that PHPStorm's "Find Usages" of \CRM_Activity_BAO_Activity::getActivities turns up only 2 places where the function is used (outside of unit tests).
When I was doing my CiviCase BAO refactoring, if I came across a long, overcomplicated and rarely-used function like that, I would aim to just get rid of it.

[monishdeb]

@colemanw ah ok, got your point but as per my recent changes this function is used in 4 places

CRM/Activity/BAO/Activity.php:2211:    $activities = CRM_Activity_BAO_Activity::getActivities($params);
CRM/Activity/Selector/Activity.php:355:    return CRM_Activity_BAO_Activity::getActivities($params, TRUE);
CRM/Activity/Selector/Activity.php:389:    $rows = CRM_Activity_BAO_Activity::getActivities($params);
CRM/Contact/BAO/Contact.php:2619:        return CRM_Activity_BAO_Activity::getActivities($input, TRUE);

As now I have introduced a 2nd parameter to $getCount to fetch activity count in the same function here and this has replaced CRM_Activity_BAO_Activity::getActivitiesCount(...) which was earlier called in only 2 places too.
Apart from that it needs additional logic apart from API call, reason why I wrapped up the entire functionality inside CRM_Activity_BAO_Activity::getActivities(...)

So overall the final patch replaces getActivitiesCount(...) and getActivitySQLClause(...) with getActivities($params, $getCount = FALSE) and called in 4 places in Core

[JoeMurray]

Just to provide some context here: this is all unfunded work that has taken many times the original funding. We have been generous in my view in completely refactoring a few files. We are not interested at this point in doing further refactorings on this issue which was originally to provide proper permissioning for Activity types, however nice that might be, to improve core in general. Please create a separate unfunded issue for refactoring out those additional functions. Perhaps we need a process for handling scope escalations on these sorts of issues.

[monishdeb]

@eileenmcnaughton made the additional changes as per your comments

[monishdeb]

Test build passed

[eileenmcnaughton]

The code looks good to me ... @jitendrapurohit are you able to give this a bit of testing & then we can look to merge. I think you have a few open that @monishdeb might review in exchange...

[jitendrapurohit]

sure, will QA in a day or two.

[colemanw]

@eileenmcnaughton yes the join onto the option_value table is mismatched. We could fix that by changing the field to be varchar. Didn't you investigate that at one point?

[eileenmcnaughton]

@colemanw I did recommend changes along those lines but it was a bit complex. The approach adopted was the pseudoconstants. I think in the api we should add special handling for joins on the option_value table to resolve without the join

[jitendrapurohit]

I think we should include limit = 0 when we intend to fetch only count($getCount = 0). Similar to -

if ($getCount) {
  $activityParams['options']['limit'] = 0;
}

As I was able to retrieve a max of 25 count on a summary page of a contact having above 100 activities.

After patch -

[jitendrapurohit]

Also would like to discuss a change in behavior of filters(after this patch) :

On a contact summary page (Activities tab). The Include and Exclude Filters don't respect the other if either of it is selected. For eg:

• Select 'Meeting' in include_filter => would list all activities of type meeting in the list (correct).
• Select 'Bulk Email' in exclude_filter without changing the include_filter value => This would show all activities except Bulk Email without respecting the first filter.

Should we consider this as an improvement (Recent filter is respected) or as an introduced bug?

[monishdeb]

@jitendrapurohit I have updated the PR with additional fixes needed, please have a look 2d64829

[jitendrapurohit]

test this please

[eileenmcnaughton]

Just a note that I consider this to be in the last stages of QA so when @jitendrapurohit approves for merge you can go ahead & merge - preferably with commit squashing

[jitendrapurohit]

Tested this on Activity Tab(add/edit/delete operation), Activity Dashlet, add/edit custom fields for activities, invoke selectWhereClause hook on activity tabs, works well in all cases.

Not sure why the test fails - it works for me on local. So I think its unrelated.

[colemanw]

@jitendrapurohit it may not be related. Try doing a rebase & force push and see if that resolves it. Command (depending on your remote aliases):

git pull --rebase upstream master
git push -f JMAConsulting CRM-20207

[eileenmcnaughton]

Adding merge on pass per @jitendrapurohit review. I think the test issue is server - will try to kick it off again

[eileenmcnaughton]

test this please

[eileenmcnaughton]

test this please

[eileenmcnaughton]

yay!

[davejenx]

As discussed on Mattermost, this PR appears to cause a fatal error on contact view for ACL'd user when the viewed contact has activities.