From 928a340b95528a9c44dfd335d6d31b183d910763 Mon Sep 17 00:00:00 2001 From: eileen Date: Wed, 5 Sep 2018 13:40:57 +1200 Subject: [PATCH] Use cached function for financialAccount retrieval. In performance testing I find that when inserting 100 contributions this change saves 198 queries by using an alternative that is cached --- CRM/Contribute/BAO/Contribution.php | 10 +- CRM/Contribute/PseudoConstant.php | 2 + CRM/Core/BAO/FinancialTrxn.php | 2 +- CRM/Financial/BAO/FinancialAccount.php | 4 +- CRM/Financial/BAO/FinancialItem.php | 2 +- .../CRM/Contribute/PseudoConstantTest.php | 93 +++++++++++++++++++ 6 files changed, 105 insertions(+), 8 deletions(-) create mode 100644 tests/phpunit/CRM/Contribute/PseudoConstantTest.php diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php index bd1d1d452e..ceb09eb5d9 100644 --- a/CRM/Contribute/BAO/Contribution.php +++ b/CRM/Contribute/BAO/Contribution.php @@ -3208,14 +3208,14 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac $partialAmtPay = CRM_Utils_Rule::cleanMoney($params['partial_amount_to_pay']); $partialAmtTotal = CRM_Utils_Rule::cleanMoney($params['partial_payment_total']); - $fromFinancialAccountId = CRM_Contribute_PseudoConstant::getRelationalFinancialAccount($params['financial_type_id'], 'Accounts Receivable Account is'); + $fromFinancialAccountId = CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship($params['financial_type_id'], 'Accounts Receivable Account is'); $statusId = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Completed'); $params['total_amount'] = $partialAmtPay; $balanceTrxnInfo = CRM_Core_BAO_FinancialTrxn::getBalanceTrxnAmt($params['contribution']->id, $params['financial_type_id']); if (empty($balanceTrxnInfo['trxn_id'])) { // create new balance transaction record - $toFinancialAccount = CRM_Contribute_PseudoConstant::getRelationalFinancialAccount($params['financial_type_id'], 'Accounts Receivable Account is'); + $toFinancialAccount = CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship($params['financial_type_id'], 'Accounts Receivable Account is'); $balanceTrxnParams['total_amount'] = $partialAmtTotal; $balanceTrxnParams['to_financial_account_id'] = $toFinancialAccount; @@ -3356,12 +3356,12 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac if (!empty($params['revenue_recognition_date']) || $params['prevContribution']->revenue_recognition_date) { $accountRelationship = 'Deferred Revenue Account is'; } - $oldFinancialAccount = CRM_Contribute_PseudoConstant::getRelationalFinancialAccount($params['prevContribution']->financial_type_id, $accountRelationship); - $newFinancialAccount = CRM_Contribute_PseudoConstant::getRelationalFinancialAccount($params['financial_type_id'], $accountRelationship); + $oldFinancialAccount = CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship($params['prevContribution']->financial_type_id, $accountRelationship); + $newFinancialAccount = CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship($params['financial_type_id'], $accountRelationship); if ($oldFinancialAccount != $newFinancialAccount) { $params['total_amount'] = 0; if (in_array($params['contribution']->contribution_status_id, $pendingStatus)) { - $params['trxnParams']['to_financial_account_id'] = CRM_Contribute_PseudoConstant::getRelationalFinancialAccount( + $params['trxnParams']['to_financial_account_id'] = CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship( $params['prevContribution']->financial_type_id, $accountRelationship); } else { diff --git a/CRM/Contribute/PseudoConstant.php b/CRM/Contribute/PseudoConstant.php index 8373687ac8..b406f3d65f 100644 --- a/CRM/Contribute/PseudoConstant.php +++ b/CRM/Contribute/PseudoConstant.php @@ -380,6 +380,8 @@ class CRM_Contribute_PseudoConstant extends CRM_Core_PseudoConstant { /** * Get financial account for a Financial type. * + * @deprecated use the alternative with caching + * CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship * * @param int $entityId * @param string $accountRelationType diff --git a/CRM/Core/BAO/FinancialTrxn.php b/CRM/Core/BAO/FinancialTrxn.php index 393714f3b4..78c51313ac 100644 --- a/CRM/Core/BAO/FinancialTrxn.php +++ b/CRM/Core/BAO/FinancialTrxn.php @@ -405,7 +405,7 @@ WHERE ceft.entity_id = %1"; else { $financialTypeId = $params['financial_type_id']; } - $financialAccount = CRM_Contribute_PseudoConstant::getRelationalFinancialAccount($financialTypeId, 'Expense Account is'); + $financialAccount = CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship($financialTypeId, 'Expense Account is'); $params['trxnParams']['from_financial_account_id'] = $params['to_financial_account_id']; $params['trxnParams']['to_financial_account_id'] = $financialAccount; diff --git a/CRM/Financial/BAO/FinancialAccount.php b/CRM/Financial/BAO/FinancialAccount.php index 9e37d4ceaf..c23542e997 100644 --- a/CRM/Financial/BAO/FinancialAccount.php +++ b/CRM/Financial/BAO/FinancialAccount.php @@ -263,7 +263,9 @@ WHERE cft.id = %1 Civi::$statics[__CLASS__]['entity_financial_account'][$financialTypeID][$accountRelationshipID] = $incomeAccountFinancialAccountID; } } - + if (!isset(Civi::$statics[__CLASS__]['entity_financial_account'][$financialTypeID][$relationTypeId])) { + Civi::$statics[__CLASS__]['entity_financial_account'][$financialTypeID][$relationTypeId] = NULL; + } } return Civi::$statics[__CLASS__]['entity_financial_account'][$financialTypeID][$relationTypeId]; } diff --git a/CRM/Financial/BAO/FinancialItem.php b/CRM/Financial/BAO/FinancialItem.php index 35e6b448d9..4e09466d9b 100644 --- a/CRM/Financial/BAO/FinancialItem.php +++ b/CRM/Financial/BAO/FinancialItem.php @@ -114,7 +114,7 @@ class CRM_Financial_BAO_FinancialItem extends CRM_Financial_DAO_FinancialItem { } } if ($lineItem->financial_type_id) { - $params['financial_account_id'] = CRM_Contribute_PseudoConstant::getRelationalFinancialAccount( + $params['financial_account_id'] = CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship( $lineItem->financial_type_id, $accountRelName ); diff --git a/tests/phpunit/CRM/Contribute/PseudoConstantTest.php b/tests/phpunit/CRM/Contribute/PseudoConstantTest.php new file mode 100644 index 0000000000..4ce5fbde63 --- /dev/null +++ b/tests/phpunit/CRM/Contribute/PseudoConstantTest.php @@ -0,0 +1,93 @@ +quickCleanUpFinancialEntities(); + parent::tearDown(); + } + + /** + * Test that getRelationalFinancialAccount works and returns the same as the performant alternative. + * + * Note this is to be changed to be a deprecated wrapper function. + * + * Future is CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship + */ + public function testGetRelationalFinancialAccount() { + $financialTypes = $this->callAPISuccess('FinancialType', 'get', [])['values']; + $financialAccounts = $this->callAPISuccess('FinancialAccount', 'get', [])['values']; + foreach ($financialTypes as $financialType) { + $accountID = CRM_Contribute_PseudoConstant::getRelationalFinancialAccount($financialType['id'], 'Accounts Receivable Account is'); + $this->assertEquals('Accounts Receivable', $financialAccounts[$accountID]['name']); + $accountIDFromBetterFunction = CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship( + $financialType['id'], + 'Accounts Receivable Account is' + ); + $this->assertEquals($accountIDFromBetterFunction, $accountID); + + $accountID = CRM_Contribute_PseudoConstant::getRelationalFinancialAccount($financialType['id'], 'Income Account is'); + $this->assertEquals($financialType['name'], $financialAccounts[$accountID]['name']); + $accountIDFromBetterFunction = CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship( + $financialType['id'], + 'Income Account is' + ); + $this->assertEquals($accountIDFromBetterFunction, $accountID); + } + + } + + /** + * Test that getRelationalFinancialAccount works and returns the same as the performant alternative. + * + * Note this is to be changed to be a deprecated wrapper function. + * + * Future is CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship + */ + public function testGetRelationalFinancialAccountForPaymentInstrument() { + $paymentInstruments = $this->callAPISuccess('Contribution', 'getoptions', ['field' => 'payment_instrument_id'])['values']; + $financialAccounts = $this->callAPISuccess('FinancialAccount', 'get', [])['values']; + foreach ($paymentInstruments as $paymentInstrumentID => $paymentInstrumentName) { + $financialAccountID = CRM_Financial_BAO_FinancialTypeAccount::getInstrumentFinancialAccount($paymentInstrumentID); + if (in_array($paymentInstrumentName, ['Credit Card', 'Debit Card'])) { + $this->assertEquals('Payment Processor Account', $financialAccounts[$financialAccountID]['name']); + } + else { + $this->assertEquals('Deposit Bank Account', $financialAccounts[$financialAccountID]['name']); + } + } + } + +} -- 2.25.1