Standardise implementation of financial type acl in query object

[eileenmcnaughton]

Overview

Standardise implementation of financial type acl in query object

This fixes it to call the selectWhere in the BAO_Query, as apiv4 does. There is test cover in the touched test.

Before

Financial ACLs applied deep in the query object

After

Applied via the selectWhere hook

Technical Details

The primary goal here is to get this logic out of core & to disable this extension on most sites with a view to moving the whole extension out of core in time - especially as we have just hit an issue with this extension requiring civi-contribution & causing problems with sites that disable civi-contribute

Comments

[civibot]

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...

• If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
• A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
• A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
• If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
• Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
• A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
• To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
• For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers...

• ➡️ PR Demo site (can be rebuilt with the command test this please).
• 📖 Review standards
• 🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

[seamuslee001]

@eileenmcnaughton these test fails seem to relate

Test Result (2 failures / +2)
Civi\Financialacls\LineItemTest.testLineItemApiPermissions with data set "APIv3"
Civi\Financialacls\LineItemTest.testLineItemApiPermissions with data set "APIv4"

[eileenmcnaughton]

OK - so this changes the behaviour a little

status quo
apiv4

• can load line items if the specific line item AND the contribution is permitted
• contribution is permitted if the financial type is permitted

apiv3 & BAO select

• • can load line items if the specific line item AND the contribution has a permitted function type, ie some but not all line items would be loaded from a permitted contribution
• contribution is permitted if the financial type is permitted AND ALL the line items are permitted (possibly inconsistently but at least in some usages of it)

This makes them the same ie LineItem get only works if ALL line items on the contribution are accessible & contribution get only retrieves if ALL line items are accessible

I think we need to standardize on one of 3 options

1. LineItem get gets the line items, regardless of whether the Contribution in it's entirity is accessible (makes more sense for reporting to me, Contribution.get requires all line items to be accessible.
2. Contribution.get does not require all line items to be accessible to return the contribution but rather at least one
3. Contribution.get does not require all line items to be accessible to return the contribution but rather the financial type id of the contribution is used

The amount of work to go down any of these 3 paths at this point is similar - since 1 is the hardest & it is done in this PR & only requires a test tweak to get it through

I do note however that they are not equivalent performance wise.

I think the best is to make 1 work / ie edit the tests for it & merge this & then actual users of the extension can tweak to one of the other options if they experience performance issues

Note that the goal now is to get this code out of core code to the extension, with the goal being the extension would eventually be community maintained outside of core so I don't want to get into any feature requests here

@JoeMurray @seamuslee001

[eileenmcnaughton]

Per my last comment I have fixed the test to acknowledge that this PR makes the strict behaviour that was inconsistently applied, consistently applied

[andyburnsco]

Tested on WP 5.70 single and multisite for both front-end forms and back end access in conjunction with #29369 which effectively does produce the outcome below which appears to be the most comprehensive and preferred option IMO:

LineItem get gets the line items, regardless of whether the Contribution in it's entity is accessible (makes more sense for reporting to me, Contribution.get requires all line items to be accessible.

I can see how the performance could be quite different looking thru line items and might take the trade off for the 3rd option (assume this will be the fastest) if noticeably different:

Contribution.get does not require all line items to be accessible to return the contribution but rather the financial type id of the contribution is used.

[seamuslee001]

Yeh I think 1 is probably best and given that this standardised on that seems good. I'm going to get jenkins to re-run again

[seamuslee001]

Jenkins re test this please