From b0e806fac15b880e3bebd553151d1ce0503acb7f Mon Sep 17 00:00:00 2001 From: eileen Date: Mon, 11 Sep 2017 14:37:50 +1200 Subject: [PATCH] CRM-20750 follow up fix, reversal should be against original account. On trying to investigate why a test change was required in the original PR I came to the following conclusions: 1) the test function was causing problems by being hard to read. As a result it was too easy to see it as 'accounts magic' - I started a cleanup here 2) Once I understood what was being tested & failing I concluded the change in the PR was in fact wrong as it was putting a negative entry against the new financial account rather than the original one. --- CRM/Core/BAO/FinancialTrxn.php | 2 +- tests/phpunit/api/v3/ContributionTest.php | 88 +++++++++++++++++++++-- 2 files changed, 82 insertions(+), 8 deletions(-) diff --git a/CRM/Core/BAO/FinancialTrxn.php b/CRM/Core/BAO/FinancialTrxn.php index ebdbfb68e8..4ffd028a8e 100644 --- a/CRM/Core/BAO/FinancialTrxn.php +++ b/CRM/Core/BAO/FinancialTrxn.php @@ -744,6 +744,7 @@ WHERE ft.is_payment = 1 $lastFinancialTrxnId = self::getFinancialTrxnId($prevContribution->id, 'DESC', FALSE, NULL, $deferredFinancialAccount); // there is no point to proceed as we can't find the last payment made + // @todo we should throw an exception here rather than return false. if (empty($lastFinancialTrxnId['financialTrxnId'])) { return FALSE; } @@ -756,7 +757,6 @@ WHERE ft.is_payment = 1 $lastFinancialTrxn['total_amount'] = -$inputParams['trxnParams']['total_amount']; $lastFinancialTrxn['net_amount'] = -$inputParams['trxnParams']['net_amount']; $lastFinancialTrxn['fee_amount'] = -$inputParams['trxnParams']['fee_amount']; - $lastFinancialTrxn['to_financial_account_id'] = CRM_Financial_BAO_FinancialTypeAccount::getInstrumentFinancialAccount($currentContribution->payment_instrument_id); $lastFinancialTrxn['contribution_id'] = $prevContribution->id; foreach (array($lastFinancialTrxn, $inputParams['trxnParams']) as $financialTrxnParams) { $trxn = CRM_Core_BAO_FinancialTrxn::create($financialTrxnParams); diff --git a/tests/phpunit/api/v3/ContributionTest.php b/tests/phpunit/api/v3/ContributionTest.php index b43a99e257..ffc8271cdd 100644 --- a/tests/phpunit/api/v3/ContributionTest.php +++ b/tests/phpunit/api/v3/ContributionTest.php @@ -1147,7 +1147,7 @@ class api_v3_ContributionTest extends CiviUnitTestCase { ); $contribution = $this->callAPISuccess('contribution', 'create', $newParams); $this->assertAPISuccess($contribution); - $this->_checkFinancialTrxn($contribution, 'paymentInstrument', $instrumentId); + $this->checkFinancialTrxnPaymentInstrumentChange($contribution['id'], 4, $instrumentId); // cleanup - delete created payment instrument $this->_deletedAddedPaymentInstrument(); @@ -1175,7 +1175,7 @@ class api_v3_ContributionTest extends CiviUnitTestCase { ); $contribution = $this->callAPISuccess('contribution', 'create', $newParams); $this->assertAPISuccess($contribution); - $this->_checkFinancialTrxn($contribution, 'paymentInstrument', $instrumentId, array('total_amount' => '-100.00')); + $this->checkFinancialTrxnPaymentInstrumentChange($contribution['id'], 4, $instrumentId, -100); // cleanup - delete created payment instrument $this->_deletedAddedPaymentInstrument(); @@ -3333,6 +3333,59 @@ class api_v3_ContributionTest extends CiviUnitTestCase { } } + /** + * Check correct financial transaction entries were created for the change in payment instrument. + * + * @param int $contributionID + * @param int $originalInstrumentID + * @param int $newInstrumentID + */ + public function checkFinancialTrxnPaymentInstrumentChange($contributionID, $originalInstrumentID, $newInstrumentID, $amount = 100) { + + $entityFinancialTrxns = $this->getFinancialTransactionsForContribution($contributionID); + + $originalTrxnParams = array( + 'to_financial_account_id' => CRM_Financial_BAO_FinancialTypeAccount::getInstrumentFinancialAccount($originalInstrumentID), + 'payment_instrument_id' => $originalInstrumentID, + 'amount' => $amount, + 'status_id' => 1, + ); + + $reversalTrxnParams = array( + 'to_financial_account_id' => CRM_Financial_BAO_FinancialTypeAccount::getInstrumentFinancialAccount($originalInstrumentID), + 'payment_instrument_id' => $originalInstrumentID, + 'amount' => -$amount, + 'status_id' => 1, + ); + + $newTrxnParams = array( + 'to_financial_account_id' => CRM_Financial_BAO_FinancialTypeAccount::getInstrumentFinancialAccount($newInstrumentID), + 'payment_instrument_id' => $newInstrumentID, + 'amount' => $amount, + 'status_id' => 1, + ); + + foreach (array($originalTrxnParams, $reversalTrxnParams, $newTrxnParams) as $index => $transaction) { + $entityFinancialTrxn = $entityFinancialTrxns[$index]; + $this->assertEquals($entityFinancialTrxn['amount'], $transaction['amount']); + + $financialTrxn = $this->callAPISuccessGetSingle('FinancialTrxn', array( + 'id' => $entityFinancialTrxn['financial_trxn_id'], + )); + $this->assertEquals($transaction['status_id'], $financialTrxn['status_id']); + $this->assertEquals($transaction['amount'], $financialTrxn['total_amount']); + $this->assertEquals($transaction['amount'], $financialTrxn['net_amount']); + $this->assertEquals(0, $financialTrxn['fee_amount']); + $this->assertEquals($transaction['payment_instrument_id'], $financialTrxn['payment_instrument_id']); + $this->assertEquals($transaction['to_financial_account_id'], $financialTrxn['to_financial_account_id']); + + // Generic checks. + $this->assertEquals(1, $financialTrxn['is_payment']); + $this->assertEquals('USD', $financialTrxn['currency']); + $this->assertEquals(date('Y-m-d'), date('Y-m-d', strtotime($financialTrxn['trxn_date']))); + } + } + /** * Check financial transaction. * @@ -3344,11 +3397,9 @@ class api_v3_ContributionTest extends CiviUnitTestCase { * @param array $extraParams */ public function _checkFinancialTrxn($contribution, $context, $instrumentId = NULL, $extraParams = array()) { - $trxnParams = array( - 'entity_id' => $contribution['id'], - 'entity_table' => 'civicrm_contribution', - ); - $trxn = current(CRM_Financial_BAO_FinancialItem::retrieveEntityFinancialTrxn($trxnParams, TRUE)); + $financialTrxns = $this->getFinancialTransactionsForContribution($contribution['id']); + $trxn = array_pop($financialTrxns); + $params = array( 'id' => $trxn['financial_trxn_id'], ); @@ -3375,6 +3426,10 @@ class api_v3_ContributionTest extends CiviUnitTestCase { ); } elseif ($context == 'changeFinancial' || $context == 'paymentInstrument') { + // @todo checkFinancialTrxnPaymentInstrumentChange instead for paymentInstrument. + // It does the same thing with greater readability. + // @todo remove handling for + $entityParams = array( 'entity_id' => $contribution['id'], 'entity_table' => 'civicrm_contribution', @@ -3910,4 +3965,23 @@ class api_v3_ContributionTest extends CiviUnitTestCase { $this->assertEquals('AUD', $contribution['values'][$contribution['id']]['currency']); } + /** + * Get the financial items for the contribution. + * + * @param int $contributionID + * + * @return array + * Array of associated financial items. + */ + protected function getFinancialTransactionsForContribution($contributionID) { + $trxnParams = array( + 'entity_id' => $contributionID, + 'entity_table' => 'civicrm_contribution', + ); + // @todo the following function has naming errors & has a weird signature & appears to + // only be called from test classes. Move into test suite & maybe just use api + // from this function. + return array_merge(CRM_Financial_BAO_FinancialItem::retrieveEntityFinancialTrxn($trxnParams, FALSE, array())); + } + } -- 2.25.1