Skip to content

Commit

Permalink
Standardise implementation of financial type acl in query object
Browse files Browse the repository at this point in the history
This fixes it to call the selectWhere in the BAO_Query, as apiv4 does. There is test
cover in the touched test.

The primary goal here is to get this logic out of core & to disabled this extension
on most sites with a view to moving the whole extension out of core in time
  • Loading branch information
eileenmcnaughton committed Feb 5, 2024
1 parent 4e8f1d9 commit 9cab235
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 34 deletions.
28 changes: 14 additions & 14 deletions CRM/Contact/BAO/Query.php
Original file line number Diff line number Diff line change
Expand Up @@ -5069,7 +5069,7 @@ public function generatePermissionClause($onlyDeleted = FALSE, $count = FALSE) {
if (!$this->_skipPermission) {
$permissionClauses = CRM_Contact_BAO_Contact_Permission::cacheClause();
$this->_permissionWhereClause = $permissionClauses[1];
$this->_permissionFromClause = $permissionClauses[0];
$this->_permissionFromClause = $permissionClauses[0] ?: '';

if (CRM_Core_Permission::check('access deleted contacts')) {
if (!$onlyDeleted) {
Expand All @@ -5081,13 +5081,19 @@ public function generatePermissionClause($onlyDeleted = FALSE, $count = FALSE) {
}

// Add ACL permission clause generated by the BAO. This is the same clause used by API::get.
// TODO: Why only activities?
// TODO: Why only activities and contributions?. Not likely to change now as we move away from this.
if (isset($this->_tables['civicrm_activity'])) {
$clauses = array_filter(CRM_Activity_BAO_Activity::getSelectWhereClause());
if ($clauses) {
$this->_permissionWhereClause .= ($this->_permissionWhereClause ? ' AND ' : '') . '(' . implode(' AND ', $clauses) . ')';
}
}
if (isset($this->_tables['civicrm_contribution'])) {
$contributionClauses = array_filter(CRM_Contribute_BAO_Contribution::getSelectWhereClause());
if (!empty($contributionClauses)) {
$this->_permissionWhereClause .= ($this->_permissionWhereClause ? ' AND ' : '') . '(' . implode(' AND ', $contributionClauses) . ')';
}
}
}
else {
// add delete clause if needed even if we are skipping permission
Expand All @@ -5112,25 +5118,17 @@ public function setSkipPermission($val) {
}

/**
* @param null $context
* @param string|null $context
*
* @return array
* @throws \CRM_Core_Exception
*/
public function summaryContribution($context = NULL) {
[$innerselect, $from, $where, $having] = $this->query(TRUE);
if (!empty($this->_permissionFromClause) && !stripos($from, 'aclContactCache')) {
$from .= " $this->_permissionFromClause";
}
if ($this->_permissionWhereClause) {
$where .= " AND " . $this->_permissionWhereClause;
}
if ($context == 'search') {
public function summaryContribution(?string $context = NULL): array {
[, $from, $where] = $this->query(TRUE);
if ($context === 'search') {
$where .= " AND contact_a.is_deleted = 0 ";
}

$this->appendFinancialTypeWhereAndFromToQueryStrings($where, $from);

$summary = ['total' => [], 'soft_credit' => ['count' => 0, 'avg' => 0, 'amount' => 0]];
$this->addBasicStatsToSummary($summary, $where, $from);

Expand All @@ -5146,10 +5144,12 @@ public function summaryContribution($context = NULL) {
/**
* Append financial ACL limits to the query from & where clauses, if applicable.
*
* @deprecated since 5.71 will be removed around 5.75
* @param string $where
* @param string $from
*/
public function appendFinancialTypeWhereAndFromToQueryStrings(&$where, &$from) {
CRM_Core_Error::deprecatedFunctionWarning('use selectWhere');
if (!CRM_Financial_BAO_FinancialType::isACLFinancialTypeStatus()) {
return;
}
Expand Down
22 changes: 22 additions & 0 deletions ext/financialacls/financialacls.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
require_once 'financialacls.civix.php';

use Civi\Api4\EntityFinancialAccount;
use Civi\Api4\FinancialType;
use Civi\Api4\MembershipType;
use CRM_Financialacls_ExtensionUtil as E;

Expand Down Expand Up @@ -95,6 +96,12 @@ function financialacls_civicrm_selectWhereClause($entity, &$clauses) {
case 'Contribution':
case 'Product':
$clauses['financial_type_id'][] = _financialacls_civicrm_get_type_clause();
if ($entity === 'Contribution') {
$unavailableTypes = _financialacls_civicrm_get_inaccessible_financial_types();
if (!empty($unavailableTypes)) {
$clauses['id'][] = 'NOT IN (SELECT contribution_id FROM civicrm_line_item WHERE financial_type_id IN (' . implode(',', $unavailableTypes) . '))';
}
}
break;

case 'Membership':
Expand Down Expand Up @@ -169,6 +176,21 @@ function _financialacls_civicrm_get_accessible_financial_types(): array {
return array_keys($types);
}

/**
* Get an array of the ids of accessible financial types.
*
* If none then it will be [0]
*
* @return int[]
*/
function _financialacls_civicrm_get_inaccessible_financial_types(): array {
$types = (array) FinancialType::get(FALSE)->addSelect('id')->execute()->indexBy('id');
foreach (_financialacls_civicrm_get_accessible_financial_types() as $accessibleFinancialType) {
unset($types[$accessibleFinancialType]);
}
return array_keys($types);
}

/**
* Get the clause to limit available membership types.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public function testLineItemApiPermissions($version): void {

$lineItems = $this->callAPISuccess('LineItem', 'get', ['sequential' => TRUE])['values'];
$this->assertCount(2, $lineItems);
$this->callAPISuccessGetCount('LineItem', ['check_permissions' => TRUE], 1);
$this->callAPISuccessGetCount('LineItem', ['check_permissions' => TRUE], 0);

$this->callAPISuccess('LineItem', 'Delete', ['check_permissions' => ($version == 3), 'id' => $lineItems[0]['id']]);
$this->callAPIFailure('LineItem', 'Delete', ['check_permissions' => TRUE, 'id' => $lineItems[1]['id']]);
Expand Down
20 changes: 1 addition & 19 deletions tests/phpunit/CRM/Contact/BAO/QueryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -1256,14 +1256,6 @@ public function getSortOptions(): array {
public function testGetSummaryQueryWithFinancialACLDisabled(): void {
$this->createContributionsForSummaryQueryTests();

// Test the function directly
$where = $from = NULL;
$queryObject = new CRM_Contact_BAO_Query();
$queryObject->appendFinancialTypeWhereAndFromToQueryStrings($where,
$from);
$this->assertEquals(NULL, $where);
$this->assertEquals(NULL, $from);

// Test the function in action
$queryObject = new CRM_Contact_BAO_Query([['contribution_source', '=', 'SSF', '', '']]);
$summary = $queryObject->summaryContribution();
Expand Down Expand Up @@ -1292,21 +1284,11 @@ public function testGetSummaryQueryWithFinancialACLDisabled(): void {
* @throws \CRM_Core_Exception
*/
public function testGetSummaryQueryWithFinancialACLEnabled(): void {
$where = $from = NULL;

$this->createContributionsForSummaryQueryTests();
$this->enableFinancialACLs();
$this->createLoggedInUserWithFinancialACL();

// Test the function directly
$queryObject = new CRM_Contact_BAO_Query();
$queryObject->appendFinancialTypeWhereAndFromToQueryStrings($where,
$from);
$donationTypeID = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'financial_type_id', 'Donation');
$this->assertEquals(
" LEFT JOIN civicrm_line_item li
ON civicrm_contribution.id = li.contribution_id AND
li.entity_table = 'civicrm_contribution' AND li.financial_type_id NOT IN ({$donationTypeID}) ", $from);

// Test the function in action
$queryObject = new CRM_Contact_BAO_Query([['contribution_source', '=', 'SSF', '', '']]);
$summary = $queryObject->summaryContribution();
Expand Down

0 comments on commit 9cab235

Please sign in to comment.