Add selectWhere hook call to the query that generates the 'annual' query - the 'amount this year' on a contact dash #13319

eileenmcnaughton · 2018-12-19T06:33:36Z

Overview

Adds a call to the selectWhere hook to the getAnnualQuery function. This function is used to generate the amount & count values for donations this year.

Before

Hook not called, less tests

After

Hook called, more tests

Technical Details

When the financial acls were developed they were added into core - tacked onto various queries. Later @colemanw developed the selectWhere hook approach for CiviCase permissions. When extending financial acls to reports we utilised this approach that had been developed in the interim https://issues.civicrm.org/jira/browse/CRM-21106 and an extension was create called 'financialaclreport'.

We now have a situation where a site using the financial acls needs to

enable the financial acls
install financialaclreport extension
(if they only do 1 then they will not have ACLs in the reports but it's arguably possible that some sites have this).

If we now implement the hook approach that is our new standard then financial report acl users will get the query applied twice. We have 3 options here

in the function addACLClausesToWhereClauses that adds the financial type acl we could check if the extension is installed and if not apply the core financial acl permission
In the function addACLClausesToWhereClauses that adds the financial type acl we could check if the extension is installed and if not put out a warning message
assume that the system check has done it's work & we don't need to handle the possibility of the financialaclreport extension not being present

I would also note that this will fail unit tests at the moment. The reason is that my digging shows that rather than the ACL applied on the line items restricting the lines included in the total it just left joins them in such that the contribution will be counted a minimum of once up to the number of permitted line items - so the only thing it achieves is making the output inaccurate. I propose we remove the line item left join for now since it is completely broken (adding the test) and resolve the bigger issue first, coming back to that if the extension does not handle it

@monishdeb @JoeMurray @pradpnayak ping

Requires #13317

Comments

My interest in this query is that it is a slow query due to an index merge & re-writing it with a better use of subqueries would reduce the time from around 30 seconds on very slow contacts to .25 seconds. However, I need to resolve what our path is with the financial acls in order to fix the query and one or two others. There are also a lot of places where the financial acls are joining in the financial type table & sometimes the line items table when they are disabled - causing a needless hindrance to the query.

civibot · 2018-12-19T06:33:38Z

(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 · 2018-12-30T23:57:02Z

test this please

eileenmcnaughton · 2018-12-30T23:58:01Z

@monishdeb hey - I'm definitely wanting to talk through this with you - the current failing test is intentional - to display the fact that some of the logic currently in there is broken and giving incorrect results & I think the right fix is to switch to a hook here.

pradpnayak · 2019-01-15T19:54:59Z

CRM/Financial/BAO/FinancialType.php

+   * @param array $whereClauses
+   */
+  public static function addACLClausesToWhereClauses(&$whereClauses) {
+    CRM_Utils_Hook::selectWhereClause('Contribution', $whereClauses);


Won't it make sense to shift this line at 373 if someone wants to alter the financial type clause based on their requirements.?

@pradpnayak yes that makes sense - the big challenge is that I'm trying to figure out how to deal with the fact that some of the contribution financial type acls are in core & some are in the extension & if we standardise them more in core people with the extension will get both - my best guess is for now to only call the financial type acl stuff in core if the above call changes nothing - & we can move to phase our of core - do you have any thoughts?

@pradpnayak also note that the extension would correctly filter on line items - per comments this gives a wrong answer due to the way line items are added

Yes you are right, Financial Type ACL report extension also uses selectWhereClause() hook.

my best guess is for now to only call the financial type acl stuff in core if the above call changes nothing - & we can move to phase our of core

+1

pradpnayak · 2019-01-15T19:58:30Z

CRM/Contribute/BAO/Contribution.php

-      'b.receive_date >= ' . $startDate,
-      'b.receive_date <  ' . $endDate,
+      'contact_id' => 'IN (' . $contactIDs . ')',
+      'contribution_status_id' => '=' . (int) CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Completed'),


A space after =?

I'm not sure I follow that comment

I meant to change'=' . (intto '= ' . (int i.e add a space after =

oh right - would make the resuling sql look better

eileenmcnaughton · 2019-01-15T22:03:34Z

@pradpnayak I've made some updates based on your input & also have marked the multiple line item test incomplete as it was functioning to demonstrate that core actually does not work at the moment but I will re-enable that with a proposed fix separately

eileenmcnaughton · 2019-01-16T01:05:39Z

I've split the stuff relating to line item munging out to here #13469 - hopefully we can merge this once tests pass although I'd stil like @monishdeb @JoeMurray to check it.

I want to take this further but not too quickly since I'd rather changes are not too much at once

eileenmcnaughton · 2019-01-17T01:00:01Z

@monishdeb @JoeMurray Pradeep has reviewed this but I still want your 'ack' as it interacts with your extension. There is a follow up PR on related issues

monishdeb · 2019-01-17T08:22:31Z

CRM/Financial/BAO/FinancialType.php

+      $whereClauses['financial_type_id'] = [
+        'IN (' . implode(',', array_keys($types)) . ')'
+      ];
+    }


how about

$types = (array) self::getAllEnabledAvailableFinancialTypes(); $whereClauses['financial_type_id'] = [ 'IN (' . implode(',', array_keys($types)) . ')' ];

@monishdeb so if they have no financial type access then $types will be empty - so we would get incorrect sql if we don't have a handling for that. IN (0) means return nothing - which would be correct

My bad, silly thought - Array casting would have provided array_keys($types) as [0]. please ignore

@eileenmcnaughton just for reusability, can we call here:

- $types = self::getAllEnabledAvailableFinancialTypes(); - if (empty($types)) { - $whereClauses['financial_type_id'] = 'IN (0)'; - } - else { - $whereClauses['financial_type_id'] = [ - 'IN (' . implode(',', array_keys($types)) . ')' - ]; - } + CRM_Financial_BAO_FinancialType::buildPermissionedClause($whereClauses);

@monishdeb the syntax is - buildPermissionedWhereClause doesn't follow the hook format where the field name is in the key rather than the string - so the goal is to migrate away from that to this & to move the hook compatible code eventually to being on the BAO in a way that it is generically called (e.g. by the api) as works with activities for example

monishdeb · 2019-01-17T09:13:12Z

Agree with the patch. Points to ponder:

Hook is invoked at a correct point to allow the extension like Financial Type ACL report to manipulate FT ACL clause and thus affect the annual query to fetch correct contribution stats based on ACL permissioned FT.
Retained the core behavior if the whereClause is not changed via extension
Enables Financial Type ACL report to produce correct contribution stats to annual query in Contact dashboard.

As per query optimisation is concerened, this could be handled in a separate PR as in #13469 and current patch doesn't bring any other changes other then the selectWhere hook call.
Overall I am happy with this patch, also the added UTs passed.

@pradpnayak @JoeMurray do I have your approval to merge this PR?

JoeMurray · 2019-01-17T15:52:09Z

I would like to deprecate our extension ASAP and move its functionality back to core now that the performance problem is resolved. There would be a bit of removal of help text on CiviContribution Settings form when enabling Financial ACLs. We can on upgrade disable the extension and display a message about why. I never liked hiving part of this functionality into a separate extension.

Should we do all of that in a separate PR?

eileenmcnaughton · 2019-01-17T20:38:45Z

@JoeMurray I think we should be going the other way and making core properly support ACLs via hook and moving all the functionality into an extension - either your extension or one that is better named.

It's worth noting that once the new selectWhere hook infrastructure is working I would expect people to implement alternatives like acls by campaigns etc.

JoeMurray · 2019-01-23T15:23:23Z

I'm in favour of all in one place or the other. I don't like hiving off a bit into an extension, defeating the purpose of hiding the data.

It would be small change to put extension code for reports back into core. I'm less clear on effort to remove ACLs from core wrt everything other than reports. We don't have a client to pay.

eileenmcnaughton · 2019-01-26T23:02:05Z

@JoeMurray either way we have to do significant cleanup in core before we can get back to one place. I believe the extension is the correct 'one place' - however, let's not delay merging this cleanup on resolving that.

JoeMurray · 2019-01-28T17:17:05Z

If you weaken some of the comments about moving all financial acls out of core then I would be more comfortable endorsing a merge on this.

If the hook call changes the where cause we will not implement the core pseudo-hook-code for financial type acls. This allows us to not conflict with reportfinancialacls extension Eventually we will not have any of this in core other than the hook.

eileenmcnaughton · 2019-01-28T21:32:23Z

OK - I tweaked the text & added merge on pass. I did make it clear that if we see moving into core as less work than moving to the extension that would mean we are not doing it right/ haven't done the preliminary tidy up (I don't see value in consolidation unless it is done correctly)

JoeMurray · 2019-01-28T21:39:26Z

Merge on pass

eileenmcnaughton · 2019-01-28T23:36:53Z

unrelated fail

civibot bot added the master label Dec 19, 2018

eileenmcnaughton force-pushed the cont_queries branch from 2f34b04 to 1786ef5 Compare December 19, 2018 06:39

pradpnayak reviewed Jan 15, 2019

View reviewed changes

eileenmcnaughton force-pushed the cont_queries branch from 1786ef5 to 94fb8cd Compare January 15, 2019 22:02

eileenmcnaughton force-pushed the cont_queries branch 3 times, most recently from bf11657 to 4202902 Compare January 16, 2019 00:47

eileenmcnaughton mentioned this pull request Jan 16, 2019

Fix bug in Annual summary query (on contribution dashboard) whereby a left join gives the wrong results #13469

Merged

monishdeb reviewed Jan 17, 2019

View reviewed changes

eileenmcnaughton force-pushed the cont_queries branch from 4202902 to c77f866 Compare January 28, 2019 21:30

eileenmcnaughton added the merge on pass label Jan 28, 2019

eileenmcnaughton merged commit 96bc266 into civicrm:master Jan 28, 2019

eileenmcnaughton deleted the cont_queries branch January 28, 2019 23:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add selectWhere hook call to the query that generates the 'annual' query - the 'amount this year' on a contact dash #13319

Add selectWhere hook call to the query that generates the 'annual' query - the 'amount this year' on a contact dash #13319

eileenmcnaughton commented Dec 19, 2018 •

edited

Loading

civibot bot commented Dec 19, 2018

eileenmcnaughton commented Dec 30, 2018

eileenmcnaughton commented Dec 30, 2018

pradpnayak Jan 15, 2019

eileenmcnaughton Jan 15, 2019

eileenmcnaughton Jan 15, 2019

pradpnayak Jan 15, 2019

pradpnayak Jan 15, 2019

eileenmcnaughton Jan 15, 2019

pradpnayak Jan 15, 2019

eileenmcnaughton Jan 15, 2019

pradpnayak Jan 15, 2019

eileenmcnaughton commented Jan 15, 2019 •

edited

Loading

eileenmcnaughton commented Jan 16, 2019

eileenmcnaughton commented Jan 17, 2019

monishdeb Jan 17, 2019

eileenmcnaughton Jan 17, 2019

monishdeb Jan 17, 2019 •

edited

Loading

monishdeb Jan 17, 2019

eileenmcnaughton Jan 17, 2019

monishdeb Jan 17, 2019

monishdeb commented Jan 17, 2019

JoeMurray commented Jan 17, 2019

eileenmcnaughton commented Jan 17, 2019

JoeMurray commented Jan 23, 2019

eileenmcnaughton commented Jan 26, 2019

JoeMurray commented Jan 28, 2019

eileenmcnaughton commented Jan 28, 2019

JoeMurray commented Jan 28, 2019

eileenmcnaughton commented Jan 28, 2019

Add selectWhere hook call to the query that generates the 'annual' query - the 'amount this year' on a contact dash #13319

Add selectWhere hook call to the query that generates the 'annual' query - the 'amount this year' on a contact dash #13319

Conversation

eileenmcnaughton commented Dec 19, 2018 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Dec 19, 2018

eileenmcnaughton commented Dec 30, 2018

eileenmcnaughton commented Dec 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Jan 15, 2019 • edited Loading

eileenmcnaughton commented Jan 16, 2019

eileenmcnaughton commented Jan 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monishdeb Jan 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monishdeb commented Jan 17, 2019

JoeMurray commented Jan 17, 2019

eileenmcnaughton commented Jan 17, 2019

JoeMurray commented Jan 23, 2019

eileenmcnaughton commented Jan 26, 2019

JoeMurray commented Jan 28, 2019

eileenmcnaughton commented Jan 28, 2019

JoeMurray commented Jan 28, 2019

eileenmcnaughton commented Jan 28, 2019

eileenmcnaughton commented Dec 19, 2018 •

edited

Loading

eileenmcnaughton commented Jan 15, 2019 •

edited

Loading

monishdeb Jan 17, 2019 •

edited

Loading