From d3e6e9a47496ff046056c589b18f0279e70264e3 Mon Sep 17 00:00:00 2001 From: eileen Date: Fri, 31 Jan 2020 21:27:18 +1300 Subject: [PATCH] Use correct...er flow to create partial payments on events This fixes the methodology for recording partial payments on the backoffice contribution form to create a pending contribution (order) and then add a payment. The previous method was our first cut at supporting partial payments (before the Order+Payment flow) and relies on some handling in the BAO which is crufty & has proven unreliable. This allows us to deprecate that handling & remove the now unused addPayments function. Prior to this PR I wrote a test for current behaviour & I had to alter that test for this as follows 1) prior to this change net_amount was clearly miscalculated & hence this fixes & I updated the test 2) prior to this change a financial transaction was created for the contribution. Now only the payment financial transaction is made. 3) prior to this change the financial item was set to partially paid, not it is still 'unpaid' Items 1 is clearly a bug fix whereas 2 & 3 are a bit borderline. Item 2 is the fact that for pending contributions (in general) we don't create financial items when creating them. We do for (some) other statuses. This was discussed here https://github.com/civicrm/civicrm-dev-docs/issues/712#issuecomment-545164618 & the summary was 'it would be better to change this but this is what was the scope /how it is' Item 3 is discussed in https://docs.civicrm.org/dev/en/latest/financial/financialentities/ with the summary being that in any other path to creating a partial payment the financial item would be left at 'Unpaid' - hopefully until it's fully paid. Item 1 is a clear bug fix & I think the fact no-one picked it up is material because it gives us a clue as to how many people use this rather obscure flow that requires an admin user to alter the status to 'partially paid' and alter the amount. Given the above we have a choice 1) Make this flow behave like other flows, despite this flow being arguably correct or 2) Hack away to keep the current quirks I've come down on the side of 1 as a standardisation. I think changing it so items 2 & 3 behave differently is worth pursuing but it's better for this edge case flow to do the same as our approved flow & if we want to iterate on that we can --- CRM/Contribute/BAO/Contribution.php | 34 ----------- CRM/Event/Form/Participant.php | 61 ++++++++++++++++--- CRM/Financial/BAO/Payment.php | 2 + .../CRM/Contribute/BAO/ContributionTest.php | 15 ----- .../CRM/Event/Form/ParticipantTest.php | 16 +---- 5 files changed, 58 insertions(+), 70 deletions(-) diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php index 325b7ff962..105c396ed3 100644 --- a/CRM/Contribute/BAO/Contribution.php +++ b/CRM/Contribute/BAO/Contribution.php @@ -4828,40 +4828,6 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac return ''; } - /** - * Function to add payments for contribution for Partially Paid status - * - * @deprecated this is known to be flawed and possibly buggy. - * - * Replace with Order.create->Payment.create flow. - * - * @param array $contribution - * - * @throws \CiviCRM_API3_Exception - */ - public static function addPayments($contribution) { - // get financial trxn which is a payment - $ftSql = "SELECT ft.id, ft.total_amount - FROM civicrm_financial_trxn ft - INNER JOIN civicrm_entity_financial_trxn eft ON eft.financial_trxn_id = ft.id AND eft.entity_table = 'civicrm_contribution' - WHERE eft.entity_id = %1 AND ft.is_payment = 1 ORDER BY ft.id DESC LIMIT 1"; - - $ftDao = CRM_Core_DAO::executeQuery($ftSql, [ - 1 => [ - $contribution->id, - 'Integer', - ], - ]); - $ftDao->fetch(); - - // store financial item Proportionaly. - $trxnParams = [ - 'total_amount' => $ftDao->total_amount, - 'contribution_id' => $contribution->id, - ]; - self::assignProportionalLineItems($trxnParams, $ftDao->id, $contribution->total_amount); - } - /** * Function use to store line item proportionally in in entity financial trxn table * diff --git a/CRM/Event/Form/Participant.php b/CRM/Event/Form/Participant.php index 1da70c725f..37faf681f1 100644 --- a/CRM/Event/Form/Participant.php +++ b/CRM/Event/Form/Participant.php @@ -226,6 +226,31 @@ class CRM_Event_Form_Participant extends CRM_Contribute_Form_AbstractEditPayment */ protected $participantRecord; + /** + * Params for creating a payment to add to the contribution. + * + * @var array + */ + protected $createPaymentParams = []; + + /** + * Get params to create payments. + * + * @return array + */ + public function getCreatePaymentParams(): array { + return $this->createPaymentParams; + } + + /** + * Set params to create payments. + * + * @param array $createPaymentParams + */ + public function setCreatePaymentParams(array $createPaymentParams) { + $this->createPaymentParams = $createPaymentParams; + } + /** * Explicitly declare the entity api name. */ @@ -1360,10 +1385,10 @@ class CRM_Event_Form_Participant extends CRM_Contribute_Form_AbstractEditPayment // CRM-13964 partial_payment_total if ($amountOwed > $params['total_amount']) { // the owed amount - $contributionParams['partial_payment_total'] = $amountOwed; - // the actual amount paid - $contributionParams['partial_amount_to_pay'] = $params['total_amount']; - $this->assign('balanceAmount', $contributionParams['partial_payment_total'] - $contributionParams['partial_amount_to_pay']); + $contributionParams['total_amount'] = $amountOwed; + $contributionParams['contribution_status_id'] = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Pending'); + $this->assign('balanceAmount', $amountOwed - $params['total_amount']); + $this->storePaymentCreateParams($params); } } @@ -1429,8 +1454,8 @@ class CRM_Event_Form_Participant extends CRM_Contribute_Form_AbstractEditPayment } } foreach ($contributions as $contribution) { - if ('Partially paid' === CRM_Core_PseudoConstant::getName('CRM_Contribute_BAO_Contribution', 'contribution_status_id', $contribution->contribution_status_id)) { - CRM_Contribute_BAO_Contribution::addPayments($contribution); + if (!empty($this->getCreatePaymentParams())) { + civicrm_api3('Payment', 'create', array_merge(['contribution_id' => $contribution->id], $this->getCreatePaymentParams())); } } } @@ -1467,7 +1492,7 @@ class CRM_Event_Form_Participant extends CRM_Contribute_Form_AbstractEditPayment } } - $this->assign('totalAmount', $contributionParams['total_amount']); + $this->assign('totalAmount', $params['total_amount'] ?? $contributionParams['total_amount']); $this->assign('isPrimary', 1); $this->assign('checkNumber', CRM_Utils_Array::value('check_number', $params)); } @@ -2235,4 +2260,26 @@ class CRM_Event_Form_Participant extends CRM_Contribute_Form_AbstractEditPayment return ''; } + /** + * Store the parameters to create a payment, if approprite, on the form. + * + * @param array $params + * Params as submitted. + */ + protected function storePaymentCreateParams($params) { + if ('Completed' === CRM_Core_PseudoConstant::getName('CRM_Contribute_BAO_Contribution', 'contribution_status_id', $params['contribution_status_id'])) { + $this->setCreatePaymentParams([ + 'total_amount' => $params['total_amount'], + 'is_send_contribution_notification' => FALSE, + 'payment_instrument_id' => $params['payment_instrument_id'], + 'trxn_date' => $params['receive_date'] ?? date('Y-m-d'), + 'trxn_id' => $params['trxn_id'], + 'pan_truncation' => $params['pan_truncation'] ?? '', + 'card_type_id' => $params['card_type_id'] ?? '', + 'check_number' => $params['check_number'] ?? '', + 'skipCleanMoney' => TRUE, + ]); + } + } + } diff --git a/CRM/Financial/BAO/Payment.php b/CRM/Financial/BAO/Payment.php index 122f13cb91..3f654b0e7a 100644 --- a/CRM/Financial/BAO/Payment.php +++ b/CRM/Financial/BAO/Payment.php @@ -45,6 +45,8 @@ class CRM_Financial_BAO_Payment { $whiteList = ['check_number', 'payment_processor_id', 'fee_amount', 'total_amount', 'contribution_id', 'net_amount', 'card_type_id', 'pan_truncation', 'trxn_result_code', 'payment_instrument_id', 'trxn_id', 'trxn_date']; $paymentTrxnParams = array_intersect_key($params, array_fill_keys($whiteList, 1)); $paymentTrxnParams['is_payment'] = 1; + // Really we should have a DB default. + $paymentTrxnParams['fee_amount'] = $paymentTrxnParams['fee_amount'] ?? 0; if (isset($paymentTrxnParams['payment_processor_id']) && empty($paymentTrxnParams['payment_processor_id'])) { // Don't pass 0 - ie the Pay Later processor as it is a pseudo-processor. diff --git a/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php b/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php index 5c6d66f1a3..52be410429 100644 --- a/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php +++ b/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php @@ -672,21 +672,6 @@ class CRM_Contribute_BAO_ContributionTest extends CiviUnitTestCase { $this->assertEquals(2, $financialTrxn->N, 'Mismatch count for is payment flag.'); } - /** - * addPayments() method (add and edit modes of participant). - * - * Add Payments is part of an old, flawed, code flow. - */ - public function testAddPayments() { - $contribution = $this->addParticipantWithContribution(); - // Delete existing financial_trxns. This is because we are testing a code flow we - // want to deprecate & remove & the test relies on bad data asa starting point. - // End goal is the Order.create->Payment.create flow. - CRM_Core_DAO::executeQuery('DELETE FROM civicrm_entity_financial_trxn WHERE entity_table = "civicrm_financial_item"'); - CRM_Contribute_BAO_Contribution::addPayments($contribution); - $this->checkItemValues($contribution); - } - /** * checks db values for financial item */ diff --git a/tests/phpunit/CRM/Event/Form/ParticipantTest.php b/tests/phpunit/CRM/Event/Form/ParticipantTest.php index 399a8a927d..7e690e0dbb 100644 --- a/tests/phpunit/CRM/Event/Form/ParticipantTest.php +++ b/tests/phpunit/CRM/Event/Form/ParticipantTest.php @@ -589,7 +589,7 @@ class CRM_Event_Form_ParticipantTest extends CiviUnitTestCase { 'contact_id' => $form->_contactID, 'total_amount' => '1550.55', 'fee_amount' => '0.00', - 'net_amount' => '20.00', + 'net_amount' => '1550.55', 'contribution_source' => 'I wrote this', 'amount_level' => '', 'is_template' => '0', @@ -630,18 +630,6 @@ class CRM_Event_Form_ParticipantTest extends CiviUnitTestCase { 'tax_amount' => '0.00', ], $lineItem); - $financialTrxn = $this->callAPISuccessGetSingle('FinancialTrxn', ['is_payment' => 0]); - $this->assertAttributesEquals([ - 'to_financial_account_id' => '7', - 'total_amount' => '1550.55', - 'fee_amount' => '0.00', - 'net_amount' => '1550.55', - 'currency' => 'USD', - 'status_id' => '1', - 'payment_instrument_id' => $paymentInstrumentID, - 'check_number' => '879', - ], $financialTrxn); - $payment = $this->callAPISuccessGetSingle('FinancialTrxn', ['is_payment' => 1]); $this->assertAttributesEquals([ 'to_financial_account_id' => 6, @@ -661,7 +649,7 @@ class CRM_Event_Form_ParticipantTest extends CiviUnitTestCase { 'contact_id' => $form->_contactID, 'amount' => 1550.55, 'currency' => 'USD', - 'status_id' => 2, + 'status_id' => CRM_Core_PseudoConstant::getKey('CRM_Financial_BAO_FinancialItem', 'status_id', 'Unpaid'), 'entity_table' => 'civicrm_line_item', 'entity_id' => $lineItem['id'], 'financial_account_id' => 4, -- 2.25.1