From 6ac768e7c3f499c062029c29f87bc06c29706a85 Mon Sep 17 00:00:00 2001 From: eileen Date: Sat, 2 Nov 2019 15:39:16 +1300 Subject: [PATCH] Fix Payment.create with a negative value to create the correct financial items The job of a refund in this function turns out to be exactly the same as the a payment. I originally thought the from & to accounts would be reversed but in fact because we are recording a negative amount that is not the case. We need the from-to accounts to be the same. The only difference turns out to be the status of the financialTrxn (Completed vs Refunded it seems). Without this change the refund code is calling the recordFinancialAccounts function but after stepping through it I'm sure the only thing that function is doing for it is creating the FinancialTrxn. It *should* be creating the EntityFinancialItem too but it is not and the best place for that to be done is in the payment class not in a spaghetti function. Note that after this we need some robust testing & probably a follow up patch on the handling of tax items - which is unlikely to be correct since it wasn't in the past AFAIK and is unchanged..... However, this fixes an important bug & should be merged as soon as it is confirmed non-regressive to allow the next one to be investigated m --- CRM/Financial/BAO/Payment.php | 158 +++++++++++++-------------- api/v3/Payment.php | 6 + tests/phpunit/api/v3/PaymentTest.php | 1 + 3 files changed, 80 insertions(+), 85 deletions(-) diff --git a/CRM/Financial/BAO/Payment.php b/CRM/Financial/BAO/Payment.php index 6303c90328..ef6de745f3 100644 --- a/CRM/Financial/BAO/Payment.php +++ b/CRM/Financial/BAO/Payment.php @@ -49,7 +49,6 @@ class CRM_Financial_BAO_Payment { * * @return \CRM_Financial_DAO_FinancialTrxn * - * @throws \API_Exception * @throws \CRM_Core_Exception * @throws \CiviCRM_API3_Exception */ @@ -81,76 +80,58 @@ class CRM_Financial_BAO_Payment { $paymentTrxnParams['trxn_id'] = $paymentTrxnParams['contribution_trxn_id']; } + $paymentTrxnParams['currency'] = $contribution['currency']; + + $accountsReceivableAccount = CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship($contribution['financial_type_id'], 'Accounts Receivable Account is'); + $paymentTrxnParams['to_financial_account_id'] = CRM_Contribute_BAO_Contribution::getToFinancialAccount($contribution, $params); + $paymentTrxnParams['from_financial_account_id'] = $accountsReceivableAccount; + if ($params['total_amount'] > 0) { - $paymentTrxnParams['to_financial_account_id'] = CRM_Contribute_BAO_Contribution::getToFinancialAccount($contribution, $params); - $paymentTrxnParams['from_financial_account_id'] = CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship($contribution['financial_type_id'], 'Accounts Receivable Account is'); - $paymentTrxnParams['currency'] = $contribution['currency']; $paymentTrxnParams['status_id'] = CRM_Core_PseudoConstant::getKey('CRM_Core_BAO_FinancialTrxn', 'status_id', 'Completed'); + } + elseif ($params['total_amount'] < 0) { + $paymentTrxnParams['status_id'] = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Refunded'); + } - $trxn = CRM_Core_BAO_FinancialTrxn::create($paymentTrxnParams); - list($ftIds, $taxItems) = CRM_Contribute_BAO_Contribution::getLastFinancialItemIds($params['contribution_id']); - - foreach ($lineItems as $key => $value) { - if ($value['qty'] == 0 || $value['allocation'] === (float) 0) { - continue; - } - $eftParams = [ - 'entity_table' => 'civicrm_financial_item', - 'financial_trxn_id' => $trxn->id, - 'entity_id' => $ftIds[$value['price_field_value_id']], - 'amount' => $value['allocation'], - ]; + $trxn = CRM_Core_BAO_FinancialTrxn::create($paymentTrxnParams); - civicrm_api3('EntityFinancialTrxn', 'create', $eftParams); - - if (array_key_exists($value['price_field_value_id'], $taxItems)) { - // @todo - this is expected to be broken - it should be fixed to - // a) have the getPayableLineItems add the amount to allocate for tax - // b) call EntityFinancialTrxn directly - per above. - // - see https://github.com/civicrm/civicrm-core/pull/14763 - $entityParams = [ - 'contribution_total_amount' => $contribution['total_amount'], - 'trxn_total_amount' => $params['total_amount'], - 'trxn_id' => $trxn->id, - 'line_item_amount' => $taxItems[$value['price_field_value_id']]['amount'], - ]; - $eftParams['entity_id'] = $taxItems[$value['price_field_value_id']]['financial_item_id']; - CRM_Contribute_BAO_Contribution::createProportionalEntry($entityParams, $eftParams); - } - } + if ($params['total_amount'] < 0 && !empty($params['cancelled_payment_id'])) { + self::reverseAllocationsFromPreviousPayment($params, $trxn->id); + return $trxn; } - elseif ($params['total_amount'] < 0) { - list($contributionDAO, $params) = self::getContributionAndParamsInFormatForRecordFinancialTransaction($params['contribution_id']); - $trxnData = $params; - $params['payment_instrument_id'] = CRM_Utils_Array::value('payment_instrument_id', $trxnData, CRM_Utils_Array::value('payment_instrument_id', $params)); - - $arAccountId = CRM_Contribute_PseudoConstant::getRelationalFinancialAccount($contributionDAO->financial_type_id, 'Accounts Receivable Account is'); - - $trxnData['total_amount'] = $trxnData['net_amount'] = $trxnData['total_amount']; - $trxnData['from_financial_account_id'] = $arAccountId; - $trxnData['status_id'] = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Refunded'); - // record the entry - $financialTrxn = CRM_Contribute_BAO_Contribution::recordFinancialAccounts($params, $trxnData); - if (!empty($params['cancelled_payment_id'])) { - // Do a direct reversal of any entity_financial_trxn records being cancelled. - $entityFinancialTrxns = civicrm_api3('EntityFinancialTrxn', 'get', [ - 'entity_table' => 'civicrm_financial_item', - 'options' => ['limit' => 0], - 'financial_trxn_id.id' => $params['cancelled_payment_id'], - ])['values']; - foreach ($entityFinancialTrxns as $entityFinancialTrxn) { - civicrm_api3('EntityFinancialTrxn', 'create', [ - 'entity_table' => 'civicrm_financial_item', - 'entity_id' => $entityFinancialTrxn['entity_id'], - 'amount' => -$entityFinancialTrxn['amount'], - 'financial_trxn_id' => $financialTrxn->id, - ]); - } + list($ftIds, $taxItems) = CRM_Contribute_BAO_Contribution::getLastFinancialItemIds($params['contribution_id']); + + foreach ($lineItems as $key => $value) { + if ($value['qty'] == 0 || $value['allocation'] === (float) 0) { + continue; + } + $eftParams = [ + 'entity_table' => 'civicrm_financial_item', + 'financial_trxn_id' => $trxn->id, + 'entity_id' => $ftIds[$value['price_field_value_id']], + 'amount' => $value['allocation'], + ]; + + civicrm_api3('EntityFinancialTrxn', 'create', $eftParams); + + if (array_key_exists($value['price_field_value_id'], $taxItems)) { + // @todo - this is expected to be broken - it should be fixed to + // a) have the getPayableLineItems add the amount to allocate for tax + // b) call EntityFinancialTrxn directly - per above. + // - see https://github.com/civicrm/civicrm-core/pull/14763 + $entityParams = [ + 'contribution_total_amount' => $contribution['total_amount'], + 'trxn_total_amount' => $params['total_amount'], + 'trxn_id' => $trxn->id, + 'line_item_amount' => $taxItems[$value['price_field_value_id']]['amount'], + ]; + $eftParams['entity_id'] = $taxItems[$value['price_field_value_id']]['financial_item_id']; + CRM_Contribute_BAO_Contribution::createProportionalEntry($entityParams, $eftParams); } } if ($isPaymentCompletesContribution) { - if ($contributionStatus == 'Pending refund') { + if ($contributionStatus === 'Pending refund') { // Ideally we could still call completetransaction as non-payment related actions should // be outside this class. However, for now we just update the contribution here. // Unit test cover in CRM_Event_BAO_AdditionalPaymentTest::testTransactionInfo. @@ -173,7 +154,7 @@ class CRM_Financial_BAO_Payment { $trxn = CRM_Core_BAO_FinancialTrxn::retrieve($ftParams); } } - elseif ($contributionStatus === 'Pending') { + elseif ($contributionStatus === 'Pending' && $params['total_amount'] > 0) { self::updateContributionStatus($contribution['id'], 'Partially Paid'); } CRM_Contribute_BAO_Contribution::recordPaymentActivity($params['contribution_id'], CRM_Utils_Array::value('participant_id', $params), $params['total_amount'], $trxn->currency, $trxn->trxn_date); @@ -287,6 +268,8 @@ class CRM_Financial_BAO_Payment { * @param int $contributionID * * @return int + * @throws \CiviCRM_API3_Exception + * @throws \CiviCRM_API3_Exception */ public static function getPaymentContactID($contributionID) { $contribution = civicrm_api3('Contribution', 'getsingle', [ @@ -382,27 +365,6 @@ class CRM_Financial_BAO_Payment { return $filteredParams; } - /** - * The recordFinancialTransactions function has capricious requirements for input parameters - load them. - * - * The function needs rework but for now we need to give it what it wants. - * - * @param int $contributionId - * - * @return array - */ - protected static function getContributionAndParamsInFormatForRecordFinancialTransaction($contributionId) { - $getInfoOf['id'] = $contributionId; - $defaults = []; - $contributionDAO = CRM_Contribute_BAO_Contribution::retrieve($getInfoOf, $defaults); - - // build params for recording financial trxn entry - $params['contribution'] = $contributionDAO; - $params = array_merge($defaults, $params); - $params['skipLineItem'] = TRUE; - return [$contributionDAO, $params]; - } - /** * Does this payment complete the contribution * @@ -451,7 +413,6 @@ class CRM_Financial_BAO_Payment { * @param $params * * @return array - * @throws \CiviCRM_API3_Exception */ protected static function getPayableLineItems($params): array { $lineItems = CRM_Price_BAO_LineItem::getLineItemsByContributionID($params['contribution_id']); @@ -515,4 +476,31 @@ class CRM_Financial_BAO_Payment { return (float) $paid; } + /** + * Reverse the entity financial transactions associated with the cancelled payment. + * + * The reversals are linked to the new payemnt. + * + * @param array $params + * @param int $trxnID + * + * @throws \CiviCRM_API3_Exception + */ + protected static function reverseAllocationsFromPreviousPayment($params, $trxnID) { + // Do a direct reversal of any entity_financial_trxn records being cancelled. + $entityFinancialTrxns = civicrm_api3('EntityFinancialTrxn', 'get', [ + 'entity_table' => 'civicrm_financial_item', + 'options' => ['limit' => 0], + 'financial_trxn_id.id' => $params['cancelled_payment_id'], + ])['values']; + foreach ($entityFinancialTrxns as $entityFinancialTrxn) { + civicrm_api3('EntityFinancialTrxn', 'create', [ + 'entity_table' => 'civicrm_financial_item', + 'entity_id' => $entityFinancialTrxn['entity_id'], + 'amount' => -$entityFinancialTrxn['amount'], + 'financial_trxn_id' => $trxnID, + ]); + } + } + } diff --git a/api/v3/Payment.php b/api/v3/Payment.php index 1aa7711626..5c490bd865 100644 --- a/api/v3/Payment.php +++ b/api/v3/Payment.php @@ -440,4 +440,10 @@ function _civicrm_api3_payment_sendconfirmation_spec(&$params) { 'title' => ts('From email; an email string or the id of a valid email'), 'type' => CRM_Utils_Type::T_STRING, ]; + $params['is_send_contribution_notification'] = [ + 'title' => ts('Send any event or contribution confirmations triggered by this payment'), + 'description' => ts('If this payment completes a contribution it may mean receipts will go out according to busines logic if thie is set to TRUE'), + 'type' => CRM_Utils_Type::T_BOOLEAN, + 'api.default' => 0, + ]; } diff --git a/tests/phpunit/api/v3/PaymentTest.php b/tests/phpunit/api/v3/PaymentTest.php index 69bdb70677..4c95a02eed 100644 --- a/tests/phpunit/api/v3/PaymentTest.php +++ b/tests/phpunit/api/v3/PaymentTest.php @@ -621,6 +621,7 @@ class api_v3_PaymentTest extends CiviUnitTestCase { CRM_Core_Config::singleton()->userPermissionClass->permissions = ['administer CiviCRM', 'access CiviContribute', 'access CiviCRM', 'edit contributions']; $payment = $this->callAPIAndDocument('payment', 'create', $params, __FUNCTION__, __FILE__, 'Update Payment', 'UpdatePayment'); + $this->validateAllPayments(); // Check for proportional cancelled payment against lineitems. $minParams = [ 'entity_table' => 'civicrm_financial_item', -- 2.25.1