-
-
Notifications
You must be signed in to change notification settings - Fork 814
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
Add selectWhere hook call to the query that generates the 'annual' query - the 'amount this year' on a contact dash #13319
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -345,6 +345,44 @@ public static function getAvailableMembershipTypes(&$membershipTypes = NULL, $ac | |
return $membershipTypes; | ||
} | ||
|
||
/** | ||
* This function adds the Financial ACL clauses to the where clause. | ||
* | ||
* This is currently somewhat mocking the native hook implementation | ||
* for the acls that are in core. If the financialaclreport extension is installed | ||
* core acls are not applied as that would result in them being applied twice. | ||
* | ||
* Long term we should either consolidate the financial acls in core or use only the extension. | ||
* Both require substantial clean up before implementing and by the time the code is clean enough to | ||
* take the final step we should | ||
* be able to implement by removing one half of the other of this function. | ||
* | ||
* @param array $whereClauses | ||
*/ | ||
public static function addACLClausesToWhereClauses(&$whereClauses) { | ||
$originalWhereClauses = $whereClauses; | ||
CRM_Utils_Hook::selectWhereClause('Contribution', $whereClauses); | ||
if ($whereClauses !== $originalWhereClauses) { | ||
// In this case permisssions have been applied & we assume the | ||
// financialaclreport is applying these | ||
// https://github.com/JMAConsulting/biz.jmaconsulting.financialaclreport/blob/master/financialaclreport.php#L107 | ||
return; | ||
} | ||
|
||
if (!self::isACLFinancialTypeStatus()) { | ||
return; | ||
} | ||
$types = self::getAllEnabledAvailableFinancialTypes(); | ||
if (empty($types)) { | ||
$whereClauses['financial_type_id'] = 'IN (0)'; | ||
} | ||
else { | ||
$whereClauses['financial_type_id'] = [ | ||
'IN (' . implode(',', array_keys($types)) . ')' | ||
]; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how about $types = (array) self::getAllEnabledAvailableFinancialTypes();
$whereClauses['financial_type_id'] = [
'IN (' . implode(',', array_keys($types)) . ')'
]; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My bad, silly thought - Array casting would have provided array_keys($types) as [0]. please ignore There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see |
||
} | ||
|
||
/** | ||
* Function to build a permissioned sql where clause based on available financial types. | ||
* | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't it make sense to shift this line at 373 if someone wants to alter the financial type clause based on their requirements.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you are right, Financial Type ACL report extension also uses selectWhereClause() hook.
+1