Skip to content
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

Merged
merged 1 commit into from
Jan 28, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions CRM/Contribute/BAO/Contribution.php
Original file line number Diff line number Diff line change
Expand Up @@ -5590,21 +5590,27 @@ public static function getAnnualQuery($contactIDs) {
$liWhere = " AND i.financial_type_id NOT IN (" . implode(',', array_keys($financialTypes)) . ")";
}
$whereClauses = [
'b.contact_id IN (' . $contactIDs . ')',
'b.contribution_status_id = ' . (int) CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Completed'),
'b.is_test = 0',
'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'),
'is_test' => ' = 0',
'receive_date' => ['>=' . $startDate, '< ' . $endDate],
];
CRM_Financial_BAO_FinancialType::buildPermissionedClause($whereClauses, NULL, 'b');
CRM_Financial_BAO_FinancialType::addACLClausesToWhereClauses($whereClauses);

$clauses = [];
foreach ($whereClauses as $key => $clause) {
$clauses[] = 'b.' . $key . " " . implode(' AND b.' . $key, (array) $clause);
}
$whereClauseString = implode(' AND ', $clauses);

$query = "
SELECT COUNT(*) as count,
SUM(total_amount) as amount,
AVG(total_amount) as average,
currency
FROM civicrm_contribution b
LEFT JOIN civicrm_line_item i ON i.contribution_id = b.id AND i.entity_table = 'civicrm_contribution' $liWhere
WHERE " . implode(' AND ', $whereClauses) . "
WHERE " . $whereClauseString . "
GROUP BY currency
";
return $query;
Expand Down
38 changes: 38 additions & 0 deletions CRM/Financial/BAO/FinancialType.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

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.?

Copy link
Contributor Author

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?

Copy link
Contributor Author

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

Copy link
Contributor

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.

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

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)) . ')'
];
}
Copy link
Member

Choose a reason for hiding this comment

The 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)) . ')'
      ];

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Member

@monishdeb monishdeb Jan 17, 2019

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The 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);

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The 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.
*
Expand Down
26 changes: 26 additions & 0 deletions tests/phpunit/CRM/Contribute/BAO/ContributionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,32 @@ public function testAnnualQueryWithFinancialACLsDisabled() {
CRM_Core_DAO::executeQuery($sql);
}

/**
* Test that financial type data is not added to the annual query if acls not enabled.
*/
public function testAnnualQueryWithFinancialHook() {
$this->hookClass->setHook('civicrm_selectWhereClause', array($this, 'aclIdNoZero'));
$sql = CRM_Contribute_BAO_Contribution::getAnnualQuery([1, 2, 3]);
$this->assertContains('SUM(total_amount) as amount,', $sql);
$this->assertContains('WHERE b.contact_id IN (1,2,3)', $sql);
$this->assertContains('b.id NOT IN (0)', $sql);
$this->assertNotContains('b.financial_type_id', $sql);
CRM_Core_DAO::executeQuery($sql);
}

/**
* Add ACL denying values LIKE '0'.
*
* @param string $entity
* @param string $clauses
*/
public function aclIdNoZero($entity, &$clauses) {
if ($entity != 'Contribution') {
return;
}
$clauses['id'] = "NOT IN (0)";
}

/**
* Display sort name during.
* Update multiple contributions
Expand Down
5 changes: 4 additions & 1 deletion tests/phpunit/CRMTraits/Financial/FinancialACLTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,14 @@ protected function disableFinancialACLs() {
* @param array $aclPermissions
* Array of ACL permissions in the format
* [[$action, $financialType], [$action, $financialType])
*
* @return int Contact ID
*/
protected function createLoggedInUserWithFinancialACL($aclPermissions = [['view', 'Donation']]) {
CRM_Core_Config::singleton()->userPermissionClass->permissions = ['access CiviCRM'];
$this->createLoggedInUser();
$contactID = $this->createLoggedInUser();
$this->addFinancialAclPermissions($aclPermissions);
return $contactID;
}

/**
Expand Down