From bbaf615e925ff0408937a06267d64046d2cf5391 Mon Sep 17 00:00:00 2001 From: demeritcowboy Date: Tue, 23 Jun 2020 20:22:07 -0400 Subject: [PATCH] compare against name not label --- CRM/Core/PseudoConstant.php | 16 +++-- CRM/Financial/BAO/FinancialAccount.php | 15 +++-- .../Financial/BAO/FinancialAccountTest.php | 66 +++++++++++++++++++ 3 files changed, 85 insertions(+), 12 deletions(-) diff --git a/CRM/Core/PseudoConstant.php b/CRM/Core/PseudoConstant.php index eb77d5ff33..2fdc708682 100644 --- a/CRM/Core/PseudoConstant.php +++ b/CRM/Core/PseudoConstant.php @@ -1376,19 +1376,21 @@ WHERE id = %1 * The static array option values is returned * * - * @param bool $optionGroupName - * Get All Option Group values- default is to get only active ones. + * @param string $optionGroupName + * Name of option group * * @param int $id - * @param null $condition + * @param string $condition + * @param string $column + * Whether to return 'name' or 'label' * * @return array - * array reference of all Option Group Name + * array reference of all Option Values */ - public static function accountOptionValues($optionGroupName, $id = NULL, $condition = NULL) { - $cacheKey = $optionGroupName . '_' . $condition; + public static function accountOptionValues($optionGroupName, $id = NULL, $condition = NULL, $column = 'label') { + $cacheKey = $optionGroupName . '_' . $condition . '_' . $column; if (empty(self::$accountOptionValues[$cacheKey])) { - self::$accountOptionValues[$cacheKey] = CRM_Core_OptionGroup::values($optionGroupName, FALSE, FALSE, FALSE, $condition); + self::$accountOptionValues[$cacheKey] = CRM_Core_OptionGroup::values($optionGroupName, FALSE, FALSE, FALSE, $condition, $column); } if ($id) { return self::$accountOptionValues[$cacheKey][$id] ?? NULL; diff --git a/CRM/Financial/BAO/FinancialAccount.php b/CRM/Financial/BAO/FinancialAccount.php index 72b9b7a13a..32fe022f82 100644 --- a/CRM/Financial/BAO/FinancialAccount.php +++ b/CRM/Financial/BAO/FinancialAccount.php @@ -216,6 +216,8 @@ WHERE cft.id = %1 * * Note that we avoid the CRM_Core_PseudoConstant function as it stores one * account per financial type and is unreliable. + * @todo Not sure what the above comment means, and the function uses the + * PseudoConstant twice. Three times if you count the for loop. * * @param int $financialTypeID * @@ -224,7 +226,12 @@ WHERE cft.id = %1 * @return int */ public static function getFinancialAccountForFinancialTypeByRelationship($financialTypeID, $relationshipType) { - $relationTypeId = key(CRM_Core_PseudoConstant::accountOptionValues('account_relationship', NULL, " AND v.name LIKE '{$relationshipType}' ")); + // This is keyed on the `value` column from civicrm_option_value + $accountRelationshipsByValue = CRM_Core_PseudoConstant::accountOptionValues('account_relationship', NULL, NULL, 'name'); + // We look up by the name a couple times below, so flip it. + $accountRelationships = array_flip($accountRelationshipsByValue); + + $relationTypeId = $accountRelationships[$relationshipType] ?? NULL; if (!isset(Civi::$statics[__CLASS__]['entity_financial_account'][$financialTypeID][$relationTypeId])) { $accounts = civicrm_api3('EntityFinancialAccount', 'get', [ @@ -236,14 +243,12 @@ WHERE cft.id = %1 Civi::$statics[__CLASS__]['entity_financial_account'][$financialTypeID][$account['account_relationship']] = $account['financial_account_id']; } - $accountRelationships = CRM_Core_PseudoConstant::accountOptionValues('account_relationship', NULL); - - $incomeAccountRelationshipID = array_search('Income Account is', $accountRelationships); + $incomeAccountRelationshipID = $accountRelationships['Income Account is'] ?? FALSE; $incomeAccountFinancialAccountID = Civi::$statics[__CLASS__]['entity_financial_account'][$financialTypeID][$incomeAccountRelationshipID]; foreach (['Chargeback Account is', 'Credit/Contra Revenue Account is'] as $optionalAccountRelationship) { - $accountRelationshipID = array_search($optionalAccountRelationship, $accountRelationships); + $accountRelationshipID = $accountRelationships[$optionalAccountRelationship] ?? FALSE; if (empty(Civi::$statics[__CLASS__]['entity_financial_account'][$financialTypeID][$accountRelationshipID])) { Civi::$statics[__CLASS__]['entity_financial_account'][$financialTypeID][$accountRelationshipID] = $incomeAccountFinancialAccountID; } diff --git a/tests/phpunit/CRM/Financial/BAO/FinancialAccountTest.php b/tests/phpunit/CRM/Financial/BAO/FinancialAccountTest.php index 0b34e9a503..c2f7195b5e 100644 --- a/tests/phpunit/CRM/Financial/BAO/FinancialAccountTest.php +++ b/tests/phpunit/CRM/Financial/BAO/FinancialAccountTest.php @@ -162,6 +162,28 @@ class CRM_Financial_BAO_FinancialAccountTest extends CiviUnitTestCase { $this->assertEquals(2, CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship(2, 'Income Account is')); } + /** + * Test getting financial account for a given financial Type with a particular relationship with label changed. + */ + public function testGetFinancialAccountByFinancialTypeAndRelationshipBuiltInLabel() { + // change the label + $optionValue = $this->callAPISuccess('OptionValue', 'get', [ + 'option_group_id' => 'account_relationship', + 'name' => 'Income Account is', + ]); + $this->callAPISuccess('OptionValue', 'create', [ + 'id' => $optionValue['id'], + 'label' => 'Changed label', + ]); + // run test + $this->assertEquals(2, CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship(2, 'Income Account is')); + // restore label + $this->callAPISuccess('OptionValue', 'create', [ + 'id' => $optionValue['id'], + 'label' => 'Income Account is', + ]); + } + /** * Test getting financial account for a given financial Type with a particular relationship. */ @@ -169,6 +191,28 @@ class CRM_Financial_BAO_FinancialAccountTest extends CiviUnitTestCase { $this->assertEquals(2, CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship(2, 'Credit/Contra Revenue Account is')); } + /** + * Test getting financial account for a given financial Type with a particular relationship with label changed. + */ + public function testGetFinancialAccountByFinancialTypeAndRelationshipBuiltInRefundedLabel() { + // change the label + $optionValue = $this->callAPISuccess('OptionValue', 'get', [ + 'option_group_id' => 'account_relationship', + 'name' => 'Credit/Contra Revenue Account is', + ]); + $this->callAPISuccess('OptionValue', 'create', [ + 'id' => $optionValue['id'], + 'label' => 'Changed label', + ]); + // run test + $this->assertEquals(2, CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship(2, 'Credit/Contra Revenue Account is')); + // restore label + $this->callAPISuccess('OptionValue', 'create', [ + 'id' => $optionValue['id'], + 'label' => 'Credit/Contra Revenue Account is', + ]); + } + /** * Test getting financial account for a given financial Type with a particular relationship. */ @@ -176,6 +220,28 @@ class CRM_Financial_BAO_FinancialAccountTest extends CiviUnitTestCase { $this->assertEquals(2, CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship(2, 'Chargeback Account is')); } + /** + * Test getting financial account for a given financial Type with a particular relationship with label changed. + */ + public function testGetFinancialAccountByFinancialTypeAndRelationshipBuiltInChargeBackLabel() { + // change the label + $optionValue = $this->callAPISuccess('OptionValue', 'get', [ + 'option_group_id' => 'account_relationship', + 'name' => 'Chargeback Account is', + ]); + $this->callAPISuccess('OptionValue', 'create', [ + 'id' => $optionValue['id'], + 'label' => 'Changed label', + ]); + // run test + $this->assertEquals(2, CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship(2, 'Chargeback Account is')); + // restore label + $this->callAPISuccess('OptionValue', 'create', [ + 'id' => $optionValue['id'], + 'label' => 'Chargeback Account is', + ]); + } + /** * Test getting financial account for a given financial Type with a particular relationship. */ -- 2.25.1