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

[eileenmcnaughton]

Overview

Removes code that attempts to apply acls at the line item level but actually does not filter anything out, but instead results in the total being wrong if multiple permitted lines are present

Before

Users subject to financial ACLS will see all contributions that have a financial type that they have permission to, regardless of whether they are permitted to see the financial types in the line items. In addition where they have permissions to see the line items the left join means that if there are multiple permitted rows the total will be incorrectly inflated

(note that if they have the financialreportacls extension installed they will not hit this bug subsequent to #13319 as that causes the line item filter to work)

After

Users subject to financial ACLS will see all contributions that have a financial type that they have permission to, regardless of whether they are permitted to see the financial types in the line items. However, the total will be correct.

As before the filtering by acls will work if financialreportacls extension is installed

Technical Details

In both cases users with the financialreportacl extension should get their financial type filtering from there once #13319 is merged - however, they will still get rows counted twice until this is merged.

Comments

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

[pradpnayak]

Changes look good to me and even tested using selectWhereClause hook where it allows you to alter/add where clause.

Good to Merge if there is no test failure!

[eileenmcnaughton]

thanks @pradpnayak

[eileenmcnaughton]

test this please

[eileenmcnaughton]

@monishdeb @JoeMurray now that the dependent PR is merged I think this is good to merge based on @pradpnayak's review - summary is that this filtering by line item never filtered contributions out - it just distorted the total. I'm removing the lines that cause this.

Subesquent to #13319 it DOES filter out when the financialreportacls extension is installed (regardless of this patch)

I don't believe it makes sense to 'fix' this to do what it was intended to do but never did at this stage because I don't think we've done enough tidy up as yet to do that in a non-hacky way and the status quo post this patch (& the previous patch) is better than prior in all cases.

[JoeMurray]

So from my perspective, I don't care what a report shows if Financial ACLs are enabled in core but the extension to support financial acls in reports is not enabled. I don't want to put effort into that.

From one perspective, the ACL permissions should ignore the contribution financial type completely, since it is irrelevant to the financial information which is all in the line items. However, I seem to recall that the funder thought the existence of a financial type for bequests should be hidden from non-permissioned users. So there was a decision to exclude contributions if there was a view permission missing for any financial type of the line types OR the contribution's. Note that this means that if only one line item was not viewable, then the whole contribution is not viewable.

I will try to do some testing on a system with the extension installed.

[eileenmcnaughton]

@JoeMurray so the above use case is the query that shows the calculation of $this year on a contribution tab.

Without this PR it miscalculates it if financial ACLs are enabled, regardless of whether you have the extension or not, if there is more than one line item per contribution.

[JoeMurray]

Got it. I'll review on the assumption that the extension is irrelevant.

[eileenmcnaughton]

So @JoeMurray it WILL work better WITH the extension due to #13319 - which would filter out some contributions

[JoeMurray]

Hmm. I have configured a price set, used it to create a donation, and the contributions tab won't render: 'Unable to reach the server. Please refresh this page in your browser and try again.'

http://core-13469-5g0c5.test-1.civicrm.org:8001/civicrm/contact/view?reset=1&cid=202&selectedChild=contribute

This is logged in as admin, but both admin and demo are permissioned to see the contribution and its line item. I have installed the financial report acl extension on the test instance.

[eileenmcnaughton]

hmm will test more locally - this is with the extension enabled?

[eileenmcnaughton]

hmm so locally I installed the extension & it works - I can't see any other config changes you made on the demo site

[JoeMurray]

no just created simple price set, enabled extension, configured permissions (is that different??), and created the contribution.

[eileenmcnaughton]

hmm -maybe I haven't configured permissions - what did you configure there?

[eileenmcnaughton]

@JoeMurray I captured the error - maybe an old version of the extension?? No - it might be php tolerance - hang fire I have a PR for your extension

[eileenmcnaughton]

test this please

[eileenmcnaughton]

test this please

[eileenmcnaughton]

test this please

[eileenmcnaughton]

test this please

[eileenmcnaughton]

@JoeMurray I think if you merge this & we get that update onto the test box it will fix it https://github.com/JMAConsulting/biz.jmaconsulting.financialaclreport/pulls

[JoeMurray]

I've merged the fix on the extension and tagged a new v1.1 release. Will update/reinstall extension on test instance associated with this PR.

[JoeMurray]

Hmm. Not sure how long it will take for new tag to propagate through c.o so I can refresh extensions and get v1.1. Pausing for now. NB: I am worried I merged a different related PR that might depend on this one. :(

[eileenmcnaughton]

@JoeMurray well we can keep testing now it's merged

[JoeMurray]

Just reporting everything here from testing. All registrants are frontend for another user, pay later, with Record payment done in backend with Cash. V1.1 of financial acl reports extension enabled.

With both core Financial ACL and extension v1.1 enabled

When first logged in as Demo user which has no FT ACL perms, then on CiviCRM home dashboard http://core-13469-5huxo.test-1.civicrm.org:8001/civicrm/dashboard got:
Notice: Undefined index: status in CRM_Utils_Check_Component_FinancialTypeAcls::checkFinancialAclReport() (line 39 of /home/jenkins/bknix-dfl/build/core-13469-5huxo/sites/all/modules/civicrm/CRM/Utils/Check/Component/FinancialTypeAcls.php).

Got spurious info message when logging in as admin with extension already enabled:
CiviCRM will in the future require the extension biz.jmaconsulting.financialaclreport for CiviCRM Reports to work correctly with the Financial Type ACLs. The extension can be downloaded here
This should be suppressed in cases where the extension is installed and enabled.

When user has permission for contribution FT but not for one of the line item FTs, then even though the contribution is correctly not shown, the Contributions tab incorrectly shows 1 in tab, and 1 in the Contributions subtab:

Confirmed summary info shown correctly for event registrations with both FTs for line items, and one with campaign and one with donation as contribution FT.

If there are no other contributions than an unpermissioned one, then no contribution summary info is displayed. Similarly, if an permissioned contribution is combined with a contribution with an unpermissioned FT and a combination of permissioned and unpermissioned line item FTs, then the contribution summary info is correct:

However, if a permissioned contribution is combined with an contribution with a permissioned FT and a mix of permissioned and unpermissioned line item FTs, then contribution summary info is incorrect:

It seems that the filtering that produces the incorrect number of contributions in the tab is an indicator that the filtering for the summary info will also be wrong.

With neither core financial ACL enabled nor financial ACL extension enabled

Summary figures are correct.

[JoeMurray]

@eileenmcnaughton ^

[eileenmcnaughton]

@JoeMurray to be clear - this PR only touches the numbers in this row

The count on the tab is unrelated to any changes made here as is the decision about what line items to display.

I AM concerned about the fact the rows are empty in your tests & will try to replicate - the other issues are unrelated to this PR (although I acknowledge they are real and worth fixing & have thoughts about how to tackle them but we should move that elsewhere)

EDIT - the rows weren't empty - they were just wrapped :-)

[eileenmcnaughton]

@JoeMurray OK - I've confirmed the situation here - the behaviour WRT the acls & the annual totals is not correct but it is ** unchanged by this PR - ie. in 5.9, 5.10 AND current master it's the same**

if we have a contribution where

• the contribution financial type is permitted
• any or no line items are permitted
• but not ALL the line items are

it is

• included in the annuals total but
• excluded from the listing
• excluded from the tab count
• excluded from the main contact totals.

This is consistent with the comments in this PR - ie there was an attempt in the code to do filtering but it was flawed and causing damage without achieving what it was attempting.

The question is why it STILL doesn't work in master with the extension

The reason for this is that the extension code requires that the contribution type is permitted OR all the lines in it are permitted so it's possible to see a contribution that has a permitted financial type but NO permitted line items or a non-permitted financial type if ALL line items are permitted.

This seems different to core assumptions and I'm inclined to think it was unintended - if the intention is that in order to see a contribution you need to have access to view ALL the line items then the patch below would make that true. Otherwise there is a gap in the assumptions in core code & the extension that I don't know how to reconcile

diff --git a/financialaclreport.php b/financialaclreport.php
index d8a02c7..2bade83 100644
--- a/financialaclreport.php
+++ b/financialaclreport.php
@@ -129,8 +129,7 @@ function financialaclreport_civicrm_selectWhereClause($entity, &$clauses) {
             LEFT JOIN civicrm_line_item  civicrm_line_item_ft
               ON civicrm_contribution_ft.id = civicrm_line_item_ft.contribution_id
               AND civicrm_line_item_ft.financial_type_id IN (" . implode(', ', array_keys($financialTypes)) . ")
-            WHERE civicrm_contribution_ft.financial_type_id IN (" . implode(', ', array_keys($financialTypes)) . ")
-            AND civicrm_line_item_ft.id IS NOT NULL
+            WHERE civicrm_line_item_ft.id IS NOT NULL
             GROUP BY civicrm_contribution_ft.id";
   CRM_Core_DAO::executeQuery($sql);
   $clauses['id'] = "NOT IN (SELECT id FROM $tempTable)";

[JoeMurray]

@Stoob would it be okay to modify the criteria for filtering so that the contribution's financial type and all of its line items' financial types are permissioned for the user before the user gets to see the contribution, or for it to be counted on the tabs or to show in the contributions tab? Just checking...I think you'll agree.

[Stoob]

1. It's certainly the most restrictive that that permission to all its line items are a requirement before you get to the see contribution. But I can think of a couple use cases where this wouldn't be ideal. Example: a 'member services' user has permissions to see Member Dues financial type, but does not have permission to see Donation financial type. A member has made a $120 contribution which consists of a Member Dues line item for $100 and a $20 Donation line item. In this case would the user not be able to see that the contribution exists at all? The same could apply to a 'event services' user use-case.

2. If you could see a contribution if you had permissions for ANY of the line items, here's what could happen. A user could infer that the extra $20 they cannot see a line item for "must have been a donation" but they would not be able to examine or modify that donation.

3. In my client use cases this question is mainly moot - financial type permission is required because of large-dollar bequests and 'planned giving', which are seldom done in conjunction with any other transactions, much less small dollar transactions, and are mostly entered by hand by donor services reps who specialize in planned giving.

[JoeMurray]

I think it's a bug right now, but we don't have time/budget to build out new functionality to filter displays by line item. And like your point 2, I'd be worried that people could interpolate them if overall total was displayed but some line items hidden. Given this doesn't affect your clients who funded the original work, I think the best fix is to require all of the FTs (contribution and line items) to be permissioned for any viewing/inclusion in calculated amounts.
@monishdeb could you create a lab issue and work on this as a medium priority, maybe for 5.12? Thx.