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

CRM-20169 (possibly wanting refactor of whole report) Added alterReportVar hook support to activity report #9886

Merged
merged 2 commits into from
Jun 26, 2017

Conversation

Edzelopez
Copy link
Contributor


@Edzelopez Edzelopez changed the title CRM-20169 Added alterReportVar hook support to activity report CRM-20169 [ready for core team review] Added alterReportVar hook support to activity report Feb 23, 2017
@seamuslee001
Copy link
Contributor

Jenkins test this please

@seamuslee001
Copy link
Contributor

@eileenmcnaughton what do you think of this?

@eileenmcnaughton
Copy link
Contributor

I have a generalised hatred for that hook because it accepts multiple parameters (rows, columns & sql) and does different things depending on what gets passed out - so it appears to me to be vague & fragile.

https://docs.civicrm.org/dev/en/master/hooks/hook_civicrm_alterReportVar/

It's also altering a report that due to the weird way the $from clause is written has had to be excluded from unit testing. I can't personally understand the point of the 'triple from', as opposed to joining on the activity contact 3 times, once for each purpose.

Probably more currently, I am expecting some other work to be done on this report to support ACLs. The first step towards that would be to get this PR reviewed and merged:
https://github.com/civicrm/civicrm-core/pull/9570/files

All of which makes me very hesitant to merge this - what was the end use case for the PR? I just hate the idea we are locking in weirdness, or alternatively next point release we might break the usage of this hook by fixing up the report.

@Edzelopez
Copy link
Contributor Author

Hi @eileenmcnaughton

All very valid points. The end use case for this PR was for us to be able to get an ACL for activity based on activity type working via an extension here

I know you expressed your concern with using this hook before, but we hadn't gone about refactoring the use of a new hook yet. Good to know that there is something new being done with respect to modifying the reports using the new selectWhereClause hook in PR #9570

@eileenmcnaughton
Copy link
Contributor

@Edzelopez If you are able to review #9570 then we can get that small step closer. It seems that in conjunction with that adding calls to buildPermissionsHook will add acl strings - although I have only done a cursory review myself. You might be able to get that working with the activity report?

BTW changing the activity report to declare the 3 contacts separately in $columns rather than use the weird triple-from structure might help

@Edzelopez
Copy link
Contributor Author

Thanks @eileenmcnaughton

I will review and refactor the PR to get the activity report working with this hook.

----------------------------------------
* CRM-20169: Add support for alterReportVar hook in Activity Report
  https://issues.civicrm.org/jira/browse/CRM-20169
@Edzelopez
Copy link
Contributor Author

@eileenmcnaughton Could you please review the changes to the report now?
I am not sure if I needed to modify this report to accept the clauses that the selectWhereClause was introducing but it turns out I needed to in order to get the report to play nicely with the aclWhere that buildPermissionClause modified.

----------------------------------------
* CRM-20169: Add support for alterReportVar hook in Activity Report
  https://issues.civicrm.org/jira/browse/CRM-20169
@Edzelopez
Copy link
Contributor Author

@eileenmcnaughton Could you please also let me know if core is planning on moving away from using the alterReportVar hook to modify filters available to reports? We are in the process of pushing out an extension which introduces ACLs for activity types and would like to know if the filters for the reports can be modified in a way other than using the alterReportVar hook.

@eileenmcnaughton
Copy link
Contributor

I think if you take a look at #9570 it introduces methodology for adding acls by hook

@eileenmcnaughton
Copy link
Contributor

Regarding "ACLs for activity types and would like to know if the filters for the reports can be modified in a way other than using the alterReportVar hook."

Do you mean - which activity types are available in the drop down to filter by?

Currently the report establishes activity types using the deprecated function

              $this->activityTypes = CRM_Core_PseudoConstant::activityType(TRUE, FALSE, FALSE, 'label', TRUE);

I'm pretty sure there is a function that applies ACLs to the list - @colemanw might confirm but I thought the api getfields function filters the types by applying ACLs? I think ACL work on activities is reasonably advanced.

@colemanw I think you need to document & share the way you have been doing ACLs

@Edzelopez
Copy link
Contributor Author

Jenkins, test this please

@Edzelopez
Copy link
Contributor Author

Edzelopez commented Mar 2, 2017

@eileenmcnaughton yes, I used the selectWhereClause hook which is called from the buildPermissionClause function introduced in the PR. (here) is the relevant hook implementation. I was just wondering if the joins added to the report now are the best way to go forward considering the limitation that the hook provides in accessing aliases and other clauses.

@Edzelopez
Copy link
Contributor Author

Edzelopez commented Mar 2, 2017

@eileenmcnaughton I also found out that core invokes the optionValues hook on

CRM_Core_PseudoConstant::activityType(TRUE, FALSE, FALSE, 'label', TRUE);

when instead it should be invoking hook_civicrm_fieldOptions.
According to the documentation here the optionValues hook is deprecated.

@eileenmcnaughton
Copy link
Contributor

The comments block on

CRM_Core_PseudoConstant::activityType

says

  • @deprecated Please use the buildOptions() method in the appropriate BAO object.

It looks like buildOptions DOES call the new hook, so replacing calls to activityType with buildOptions is probably the way to go.

Note the helpers CRM_Core_PseudoConstant::getLabel, getKey & getValue leverage buildOptions, although I'm not sure that is terribly helpful here,

@Edzelopez
Copy link
Contributor Author

Edzelopez commented Mar 10, 2017

Hi @eileenmcnaughton
Sorry for the delay in replying to this, it slipped off my to-do list with the other fixes being higher priority.
I agree that the buildOptions() function should be used instead of CRM_Core_PseudoConstant::activityType, but that is not the case here. This report does not use the Pseudoconstant method to retrieve the activity types, instead it uses the CRM_Core_OptionGroup::values('activity_type', FALSE, FALSE, FALSE, $condition)
function.

I was just bringing the invalidity of the deprecated hook to you notice but it seems the buildOptions function was designed for just that :)

Also, I would like to apologize if you were indicating I should review PR #9570
I interpreted your request as something I should review and apply in my PR rather than review and reply to that PR.

Could you please review the rest of the changes are merge if appropriate? Thanks!

@eileenmcnaughton
Copy link
Contributor

@Edzelopez I'm really deeply uncomfortable with the way this report has been written, and I'm reluctant to add to it without a unit test.

It's not testable like the others yet because it's kinda-weird. My suspicion is the weirdness is not required & it was written that way because of the way activity data was stored prior to CiviCRM 4.4 - if that's the case it could be stripped back to being 'like a normal report'.

I want to do some performance tests on the query to answer that question - and i added this small PR (which you could review) to add the developer tab to make it easier to do that #9965

I'll try to do the testing early next week to figure out whether it makes sense to 'add the temp table better so it can be tested' or just skip the temp table & make the report simple (and tested)

@eileenmcnaughton
Copy link
Contributor

As an aside I discovered that the address fields on the report show the most recent contact

@JoeMurray
Copy link
Contributor

JoeMurray commented Mar 13, 2017

Just trying to see what you would like made simpler, @eileenmcnaughton .The select() seems quite unusual eg if at https://github.com/civicrm/civicrm-core/blob/master/CRM/Report/Form/Activity.php#L399 . But I think the replication of source, target, assignee is needed, final seems to be doing something different - I would have used a different word that was clearer. This affects the from(). Creating the temp table at https://github.com/civicrm/civicrm-core/blob/master/CRM/Report/Form/Activity.php#L805 if needed could all be done in a single query. But I agree, this is not logically needed and is probably just being done in this odd way as a result of optimization. Jitendra did some Group By optimization last year, and not the odd stuff re: mysql versions and options at b708c08#diff-e54381bfdf51e31cab376c71ca0d66ffL4533. So, in general, you're proposing to refactor to remove temp table and just use regular joins. My guess is that the performance issue comes from joining repeatedly against a large contact table. You might want to consider comparing versions of SQL depending on how you structure query as 5.7 has better optimizations in various places.

@JoeMurray JoeMurray changed the title CRM-20169 [ready for core team review] Added alterReportVar hook support to activity report CRM-20169 WIP (possibly wanting refactor of whole report) Added alterReportVar hook support to activity report Mar 13, 2017
@colemanw
Copy link
Member

@eileenmcnaughton I'd like to accommodate JMA's client deadline with this while still pushing for needed refactors. @JoeMurray has agreed to add your request to his backlog; meanwhile do you see any problems with merging this PR for now?

@JoeMurray
Copy link
Contributor

Here is a CRM issue for the backlog https://issues.civicrm.org/jira/browse/CRM-20407

@monishdeb
Copy link
Member

I have tested this patch and it works for me in local. Agree with the changes as, this is what we can do, least to support alterReportVar hook in Activity Report.

@eileenmcnaughton can you merge it?

@colemanw colemanw merged commit 5502c5c into civicrm:master Jun 26, 2017
@pradpnayak pradpnayak deleted the CRM-20169 branch June 26, 2017 21:15
@eileenmcnaughton eileenmcnaughton changed the title CRM-20169 WIP (possibly wanting refactor of whole report) Added alterReportVar hook support to activity report CRM-20169 (possibly wanting refactor of whole report) Added alterReportVar hook support to activity report Sep 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants