From 8a40179ed232a6f5c9f1f2acddedf422a9e31b11 Mon Sep 17 00:00:00 2001 From: eileen Date: Fri, 15 Sep 2017 11:51:47 +1200 Subject: [PATCH] CRM-21187 Fix handling of currency when updating a contribution to be completed. This is replicable via the api & potentially other flows. The currency id of NULL is not accepted in financialTrxn BAO without it replacing with a default, which is not appropriate on an update. --- CRM/Contribute/BAO/Contribution.php | 16 +++++++++ CRM/Core/BAO/FinancialTrxn.php | 6 ++-- tests/phpunit/api/v3/ContributionTest.php | 42 ++++++++++++++++++++++- 3 files changed, 60 insertions(+), 4 deletions(-) diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php index 53b6448fb0..11760976aa 100644 --- a/CRM/Contribute/BAO/Contribution.php +++ b/CRM/Contribute/BAO/Contribution.php @@ -3467,6 +3467,9 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac * @param array $params * Contribution object, line item array and params for trxn. * + * @todo stop passing $params by reference. It is unclear the purpose of doing this & + * adds unpredictability. + * * @param string $context * Update scenarios. * @@ -3494,6 +3497,7 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac return; } if ($context == 'changedAmount' || $context == 'changeFinancialType') { + // @todo we should stop passing $params by reference - splitting this out would be a step towards that. $params['trxnParams']['total_amount'] = $params['trxnParams']['net_amount'] = ($params['total_amount'] - $params['prevContribution']->total_amount); } if ($context == 'changedStatus') { @@ -3501,6 +3505,7 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac && (self::isContributionStatusNegative($params['contribution']->contribution_status_id)) ) { $isARefund = TRUE; + // @todo we should stop passing $params by reference - splitting this out would be a step towards that. $params['trxnParams']['total_amount'] = -$params['total_amount']; if (empty($params['contribution']->creditnote_id) || $params['contribution']->creditnote_id == "null") { $creditNoteId = self::createCreditNoteId(); @@ -3514,6 +3519,7 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac $arAccountId = CRM_Contribute_PseudoConstant::getRelationalFinancialAccount($financialTypeID, 'Accounts Receivable Account is'); if ($currentContributionStatus == 'Cancelled') { + // @todo we should stop passing $params by reference - splitting this out would be a step towards that. $params['trxnParams']['to_financial_account_id'] = $arAccountId; $params['trxnParams']['total_amount'] = -$params['total_amount']; if (is_null($params['contribution']->creditnote_id) || $params['contribution']->creditnote_id == "null") { @@ -3522,6 +3528,7 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac } } else { + // @todo we should stop passing $params by reference - splitting this out would be a step towards that. $params['trxnParams']['from_financial_account_id'] = $arAccountId; } } @@ -3539,8 +3546,13 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac // & this can be removed return; } + // @todo we should stop passing $params by reference - splitting this out would be a step towards that. + // This is an update so original currency if none passed in. + $params['trxnParams']['currency'] = CRM_Utils_Array::value('currency', $params, $params['prevContribution']->currency); + self::recordAlwaysAccountsReceivable($params['trxnParams'], $params); $trxn = CRM_Core_BAO_FinancialTrxn::create($params['trxnParams']); + // @todo we should stop passing $params by reference - splitting this out would be a step towards that. $params['entity_id'] = self::$_trxnIDs[] = $trxn->id; $query = "UPDATE civicrm_financial_item SET status_id = %1 WHERE entity_id = %2 and entity_table = 'civicrm_line_item'"; $sql = "SELECT id, amount FROM civicrm_financial_item WHERE entity_id = %1 and entity_table = 'civicrm_line_item'"; @@ -3574,6 +3586,7 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac } $trxn = CRM_Core_BAO_FinancialTrxn::create($params['trxnParams']); + // @todo we should stop passing $params by reference - splitting this out would be a step towards that. $params['entity_id'] = $trxn->id; if ($context != 'changePaymentInstrument') { $itemParams['entity_table'] = 'civicrm_line_item'; @@ -3590,6 +3603,7 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac $financialAccount = self::getFinancialAccountForStatusChangeTrxn($params, CRM_Utils_Array::value('financial_account_id', $prevFinancialItem)); $currency = $params['prevContribution']->currency; + // @todo we should stop passing $params by reference - splitting this out would be a step towards that. if ($params['contribution']->currency) { $currency = $params['contribution']->currency; } @@ -3606,6 +3620,7 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac 'entity_id' => $lineItemDetails['id'], ); $financialItem = CRM_Financial_BAO_FinancialItem::create($itemParams, NULL, $trxnIds); + // @todo we should stop passing $params by reference - splitting this out would be a step towards that. $params['line_item'][$fieldId][$fieldValueId]['deferred_line_total'] = $itemParams['amount']; $params['line_item'][$fieldId][$fieldValueId]['financial_item_id'] = $financialItem->id; @@ -3634,6 +3649,7 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac } } if ($context == 'changeFinancialType') { + // @todo we should stop passing $params by reference - splitting this out would be a step towards that. $params['skipLineItem'] = FALSE; foreach ($params['line_item'] as &$lineItems) { foreach ($lineItems as &$line) { diff --git a/CRM/Core/BAO/FinancialTrxn.php b/CRM/Core/BAO/FinancialTrxn.php index 4ffd028a8e..06f1021d38 100644 --- a/CRM/Core/BAO/FinancialTrxn.php +++ b/CRM/Core/BAO/FinancialTrxn.php @@ -50,18 +50,18 @@ class CRM_Core_BAO_FinancialTrxn extends CRM_Financial_DAO_FinancialTrxn { * * @return CRM_Core_BAO_FinancialTrxn */ - public static function create(&$params) { + public static function create($params) { $trxn = new CRM_Financial_DAO_FinancialTrxn(); $trxn->copyValues($params); - if (!CRM_Utils_Rule::currencyCode($trxn->currency)) { + if (empty($params['id']) && !CRM_Utils_Rule::currencyCode($trxn->currency)) { $trxn->currency = CRM_Core_Config::singleton()->defaultCurrency; } $trxn->save(); - // We shoudn't proceed further to record related entity financial trxns if it's update. if (!empty($params['id'])) { + // For an update entity financial transaction record will already exist. Return early. return $trxn; } diff --git a/tests/phpunit/api/v3/ContributionTest.php b/tests/phpunit/api/v3/ContributionTest.php index ffc8271cdd..3166a5a9ff 100644 --- a/tests/phpunit/api/v3/ContributionTest.php +++ b/tests/phpunit/api/v3/ContributionTest.php @@ -1018,7 +1018,7 @@ class api_v3_ContributionTest extends CiviUnitTestCase { * Function tests that additional financial records are created for online contribution with pending option. */ public function testCreateContributionPendingOnline() { - $paymentProcessor = CRM_Financial_BAO_PaymentProcessor::create($this->_processorParams); + CRM_Financial_BAO_PaymentProcessor::create($this->_processorParams); $contributionPage = $this->callAPISuccess('contribution_page', 'create', $this->_pageParams); $this->assertAPISuccess($contributionPage); $params = array( @@ -1747,6 +1747,46 @@ class api_v3_ContributionTest extends CiviUnitTestCase { $this->revertTemplateToReservedTemplate(); } + /** + * Test completing a transaction via the API with a non-USD transaction. + */ + public function testCompleteTransactionEuro() { + $mut = new CiviMailUtils($this, TRUE); + $this->swapMessageTemplateForTestTemplate(); + $this->createLoggedInUser(); + $params = array_merge($this->_params, array('contribution_status_id' => 2, 'currency' => 'EUR')); + $contribution = $this->callAPISuccess('contribution', 'create', $params); + + $this->callAPISuccess('contribution', 'completetransaction', array( + 'id' => $contribution['id'], + )); + + $contribution = $this->callAPISuccess('contribution', 'getsingle', array('id' => $contribution['id'])); + $this->assertEquals('SSF', $contribution['contribution_source']); + $this->assertEquals('Completed', $contribution['contribution_status']); + $this->assertEquals(date('Y-m-d'), date('Y-m-d', strtotime($contribution['receipt_date']))); + + $entityFinancialTransactions = $this->getFinancialTransactionsForContribution($contribution['id']); + $entityFinancialTransaction = reset($entityFinancialTransactions); + $financialTrxn = $this->callAPISuccessGetSingle('FinancialTrxn', array('id' => $entityFinancialTransaction['financial_trxn_id'])); + $this->assertEquals('EUR', $financialTrxn['currency']); + + $mut->checkMailLog(array( + 'email:::anthony_anderson@civicrm.org', + 'is_monetary:::1', + 'amount:::100.00', + 'currency:::EUR', + 'receive_date:::' . date('Ymd', strtotime($contribution['receive_date'])), + "receipt_date:::\n", + 'contributeMode:::notify', + 'title:::Contribution', + 'displayName:::Mr. Anthony Anderson II', + 'contributionStatus:::Completed', + )); + $mut->stop(); + $this->revertTemplateToReservedTemplate(); + } + /** * Test to ensure mail is sent on chosing pay later */ -- 2.25.1