[REF] Move addSelectWhere-like function to be located on BAO_Contribution

[eileenmcnaughton]

Overview

This is a code relocation / standardisation to use the std location for applying acls to contributions

Before

Function in a random place

After

Function in the std place

Technical Details

Having a function to add ACLs in addSelectWhere on the BAO is the standard. There is a question as to whether this goes on the contribution BAO or on the financialType BAO & we put something in the parent to call in the financialType acl whenever it's an FK on the entity. However, that could be resolved in a follow up

Comments

Note this is covered by the recently added annualQuery test

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

[colemanw]

@eileenmcnaughton I'm confused about what this code is doing. It assigns a variable to another variable, and then immediately checks if they are equal.

[eileenmcnaughton]

@colemanw did you read the comment below? Basically we have an issue where we have financial acls in the hook AND the extension & we don't want both to apply. Originally the only places that called the hook DIDN'T apply the financial acls & vice versa.

Long term we want to only use the extension or only call the lines below this return. Joe doesn't yet know that he agrees with me on which one :-)

[colemanw]

I did read the comment but the code still doesn't make logical sense to me. It's basically saying:

function checkIfFooEqualsFoo() {
  $foo = rand();
  $bar = $foo;
  if ($bar !== $foo) {
    // this will never happen. Why are we checking for this?

[eileenmcnaughton]

@colemanw it WOULD happen if the parent 's call to the financialACLHook hit the financialaclreport extension

[colemanw]

Sorry I still don't see how. Even if the rand() function in my example were to call a hook, it's still assigning the output of that hook to two variables which will always be exactly the same no matter what, and then comparing them to see if they are exactly the same (they are).

[eileenmcnaughton]

ah - I mis-ordered when I brought across - I see now - fixed

[colemanw]

Ok now the first line of the function seems to be trying to access an undefined variable $whereClauses

[eileenmcnaughton]

this time?

[colemanw]

Now it makes sense :)

[eileenmcnaughton]

thanks for persevering with me!

[seamuslee001]

Jenkins re test this please

[eileenmcnaughton]

@colemanw - it's passing now - this is tested from the getAnnualQuery test

[colemanw]

LGTM. As Eileen and I have discussed, this could still use some more refactoring but this PR is a good first step.