From f52694340988cd29f5f80b6392e7662bf33e8bf2 Mon Sep 17 00:00:00 2001 From: eileen Date: Wed, 20 Nov 2019 17:54:22 +1300 Subject: [PATCH] dev/core#1409 Remove net_amount from Addtional Payment form This is causing a validation problem. We used to have an issue on the contribution form which we eventually resolved by removing net_amount as it's best calculated anyway In order to make this changed I had to ensure Payment.create adds the net_amount & had to do a couple of tweaks for the test to pass --- CRM/Contribute/Form/AdditionalPayment.php | 12 ++--------- CRM/Core/BAO/FinancialTrxn.php | 8 ++++++++ api/v3/Payment.php | 20 ++++++++++++++++--- .../CRM/Contribute/Form/AdditionalPayment.tpl | 2 -- tests/phpunit/api/v3/PaymentTest.php | 11 ++++++++++ .../phpunit/api/v3/SyntaxConformanceTest.php | 6 ++++++ 6 files changed, 44 insertions(+), 15 deletions(-) diff --git a/CRM/Contribute/Form/AdditionalPayment.php b/CRM/Contribute/Form/AdditionalPayment.php index dbb3136f3f..49f7d01db1 100644 --- a/CRM/Contribute/Form/AdditionalPayment.php +++ b/CRM/Contribute/Form/AdditionalPayment.php @@ -242,7 +242,7 @@ class CRM_Contribute_Form_AdditionalPayment extends CRM_Contribute_Form_Abstract $js = NULL; // render backoffice payment fields only on offline mode if (!$this->_mode) { - $js = ['onclick' => "return verify( );"]; + $js = ['onclick' => 'return verify( );']; $this->add('select', 'payment_instrument_id', ts('Payment Method'), @@ -258,11 +258,6 @@ class CRM_Contribute_Form_AdditionalPayment extends CRM_Contribute_Form_Abstract $attributes['fee_amount'] ); $this->addRule('fee_amount', ts('Please enter a valid monetary value for Fee Amount.'), 'money'); - - $this->add('text', 'net_amount', ts('Net Amount'), - $attributes['net_amount'] - ); - $this->addRule('net_amount', ts('Please enter a valid monetary value for Net Amount.'), 'money'); } $buttonName = $this->_refund ? ts('Record Refund') : ts('Record Payment'); @@ -299,10 +294,7 @@ class CRM_Contribute_Form_AdditionalPayment extends CRM_Contribute_Form_Abstract if ($self->_paymentType == 'refund' && $fields['total_amount'] != abs($self->_refund)) { $errors['total_amount'] = ts('Refund amount must equal refund due amount.'); } - $netAmt = (float) $fields['total_amount'] - (float) CRM_Utils_Array::value('fee_amount', $fields, 0); - if (!empty($fields['net_amount']) && $netAmt != $fields['net_amount']) { - $errors['net_amount'] = ts('Net amount should be equal to the difference between payment amount and fee amount.'); - } + if ($self->_paymentProcessor['id'] === 0 && empty($fields['payment_instrument_id'])) { $errors['payment_instrument_id'] = ts('Payment method is a required field'); } diff --git a/CRM/Core/BAO/FinancialTrxn.php b/CRM/Core/BAO/FinancialTrxn.php index 9fe5b61e46..ac05ff750a 100644 --- a/CRM/Core/BAO/FinancialTrxn.php +++ b/CRM/Core/BAO/FinancialTrxn.php @@ -55,6 +55,14 @@ class CRM_Core_BAO_FinancialTrxn extends CRM_Financial_DAO_FinancialTrxn { $trxn = new CRM_Financial_DAO_FinancialTrxn(); $trxn->copyValues($params); + if (isset($params['fee_amount']) && is_numeric($params['fee_amount'])) { + if (!isset($params['total_amount'])) { + $trxn->fetch(); + $params['total_amount'] = $trxn->total_amount; + } + $trxn->net_amount = $params['total_amount'] - $params['fee_amount']; + } + if (empty($params['id']) && !CRM_Utils_Rule::currencyCode($trxn->currency)) { $trxn->currency = CRM_Core_Config::singleton()->defaultCurrency; } diff --git a/api/v3/Payment.php b/api/v3/Payment.php index 5c490bd865..afe22067b6 100644 --- a/api/v3/Payment.php +++ b/api/v3/Payment.php @@ -39,6 +39,8 @@ * * @return array * Array of financial transactions which are payments, if error an array with an error id and error message + * + * @throws \CiviCRM_API3_Exception */ function civicrm_api3_payment_get($params) { $financialTrxn = []; @@ -50,7 +52,10 @@ function civicrm_api3_payment_get($params) { if (isset($params['trxn_id'])) { $params['financial_trxn_id.trxn_id'] = $params['trxn_id']; } - $eft = civicrm_api3('EntityFinancialTrxn', 'get', $params); + $eftParams = $params; + unset($eftParams['return']); + // @todo - why do we fetch EFT params at all? + $eft = civicrm_api3('EntityFinancialTrxn', 'get', $eftParams); if (!empty($eft['values'])) { $eftIds = []; foreach ($eft['values'] as $efts) { @@ -83,9 +88,10 @@ function civicrm_api3_payment_get($params) { * @param array $params * Input parameters. * - * @throws API_Exception * @return array * Api result array + * + * @throws \CiviCRM_API3_Exception */ function civicrm_api3_payment_delete($params) { return civicrm_api3('FinancialTrxn', 'delete', $params); @@ -135,7 +141,6 @@ function civicrm_api3_payment_cancel($params) { * @return array * Api result array * - * @throws \API_Exception * @throws \CRM_Core_Exception * @throws \CiviCRM_API3_Exception */ @@ -189,6 +194,10 @@ function _civicrm_api3_payment_create_spec(&$params) { 'title' => ts('Total Payment Amount'), 'type' => CRM_Utils_Type::T_FLOAT, ], + 'fee_amount' => [ + 'title' => ts('Fee Amount'), + 'type' => CRM_Utils_Type::T_FLOAT, + ], 'payment_processor_id' => [ 'name' => 'payment_processor_id', 'type' => CRM_Utils_Type::T_INT, @@ -349,6 +358,11 @@ function _civicrm_api3_payment_get_spec(&$params) { 'title' => ts('Payment Date'), 'type' => CRM_Utils_Type::T_TIMESTAMP, ], + 'financial_trxn_id' => [ + 'title' => ts('Payment ID'), + 'type' => CRM_Utils_Type::T_INT, + 'api.aliases' => ['payment_id', 'id'], + ], ]; } diff --git a/templates/CRM/Contribute/Form/AdditionalPayment.tpl b/templates/CRM/Contribute/Form/AdditionalPayment.tpl index 23443f10fd..ac7feb83fe 100644 --- a/templates/CRM/Contribute/Form/AdditionalPayment.tpl +++ b/templates/CRM/Contribute/Form/AdditionalPayment.tpl @@ -110,8 +110,6 @@ {$form.fee_amount.label}{$form.fee_amount.html|crmMoney:$currency:'XXX':'YYY'}
{ts}Processing fee for this transaction (if applicable).{/ts} - {$form.net_amount.label}{$form.net_amount.html|crmMoney:$currency:'':1}
- {ts}Net value of the payment (Total Amount minus Fee).{/ts} {/if} diff --git a/tests/phpunit/api/v3/PaymentTest.php b/tests/phpunit/api/v3/PaymentTest.php index 5bc8ba4537..eb61907867 100644 --- a/tests/phpunit/api/v3/PaymentTest.php +++ b/tests/phpunit/api/v3/PaymentTest.php @@ -770,6 +770,17 @@ class api_v3_PaymentTest extends CiviUnitTestCase { $this->validateAllPayments(); } + /** + * Test net amount is set when fee amount is passed in. + * + * @throws \CRM_Core_Exception + */ + public function testNetAmount() { + $order = $this->createPendingParticipantOrder(); + $payment = $this->callAPISuccess('Payment', 'create', ['order_id' => $order['id'], 'total_amount' => 10, 'fee_amount' => .25]); + $this->assertEquals('9.75', $this->callAPISuccessGetValue('Payment', ['id' => $payment['id'], 'return' => 'net_amount'])); + } + /** * Test create payment api for pay later contribution with partial payment. * diff --git a/tests/phpunit/api/v3/SyntaxConformanceTest.php b/tests/phpunit/api/v3/SyntaxConformanceTest.php index c074d1a314..74bb295691 100644 --- a/tests/phpunit/api/v3/SyntaxConformanceTest.php +++ b/tests/phpunit/api/v3/SyntaxConformanceTest.php @@ -663,6 +663,12 @@ class api_v3_SyntaxConformanceTest extends CiviUnitTestCase { 'is_primary', ], ], + 'FinancialTrxn' => [ + 'cant_update' => [ + // Altering fee amount will also cause net_amount to be recalculated. + 'fee_amount', + ], + ], 'Navigation' => [ 'cant_update' => [ // Weight is deliberately altered when this is changed - skip. -- 2.25.1