From de7391fc1cb9b2722a6b0f129a741b5395f7a032 Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Sat, 24 Jul 2021 11:24:46 +1200 Subject: [PATCH] Fix Payment.create to update financial_item.status_id When I try to switch to the order->create flow for membership forms it turns out we are leaving the financial_item.status_id as 'unpaid' when adding a payment. No one has noticed because this field is kinda unused - but it needs to work to pass tests --- CRM/Financial/BAO/Payment.php | 78 ++++++++++++++----- .../CRM/Contribute/BAO/ContributionTest.php | 4 +- .../CRM/Event/Form/ParticipantTest.php | 2 +- tests/phpunit/api/v3/ContributionTest.php | 6 +- tests/phpunit/api/v3/OrderTest.php | 22 +++++- 5 files changed, 85 insertions(+), 27 deletions(-) diff --git a/CRM/Financial/BAO/Payment.php b/CRM/Financial/BAO/Payment.php index 02fbf35561..e24a4dfebc 100644 --- a/CRM/Financial/BAO/Payment.php +++ b/CRM/Financial/BAO/Payment.php @@ -15,16 +15,20 @@ * @copyright CiviCRM LLC https://civicrm.org/licensing */ +use Civi\Api4\FinancialItem; +use Civi\Api4\LineItem; + /** * This class contains payment related functions. */ class CRM_Financial_BAO_Payment { /** - * Function to process additional payment for partial and refund contributions. + * Function to process additional payment for partial and refund + * contributions. * - * This function is called via API payment.create function. All forms that add payments - * should use this. + * This function is called via API payment.create function. All forms that + * add payments should use this. * * @param array $params * - contribution_id @@ -35,6 +39,7 @@ class CRM_Financial_BAO_Payment { * * @throws \CRM_Core_Exception * @throws \CiviCRM_API3_Exception + * @throws \API_Exception */ public static function create(array $params): CRM_Financial_DAO_FinancialTrxn { $contribution = civicrm_api3('Contribution', 'getsingle', ['id' => $params['contribution_id']]); @@ -96,17 +101,37 @@ class CRM_Financial_BAO_Payment { self::reverseAllocationsFromPreviousPayment($params, $trxn->id); } else { - [$ftIds, $taxItems] = CRM_Contribute_BAO_Contribution::getLastFinancialItemIds($params['contribution_id']); + $salesTaxFinancialAccount = CRM_Contribute_BAO_Contribution::getSalesTaxFinancialAccounts(); + $financialItems = LineItem::get(FALSE) + ->addSelect('tax_amount', 'price_field_value_id', 'financial_item.id', 'financial_item.status_id:name', 'financial_item.financial_account_id') + ->addJoin( + 'FinancialItem AS financial_item', + 'INNER', + NULL, + ['financial_item.entity_table', '=', '"civicrm_line_item"'], + ['financial_item.entity_id', '=', 'id'] + ) + ->addOrderBy('financial_item.id', 'DESC') + ->addWhere('contribution_id', '=', (int) $params['contribution_id'])->execute(); foreach ($lineItems as $key => $value) { if ($value['allocation'] === (float) 0) { continue; } - - if (!empty($ftIds[$value['price_field_value_id']])) { - $financialItemID = $ftIds[$value['price_field_value_id']]; + $financialItemID = NULL; + $currentFinancialItemStatus = NULL; + foreach ($financialItems as $item) { + // We check against price_field_value_id rather than line item + // id because that is what the code did previously - but it's + // unclear whether this is for good reason or bad coding. + if ($item['price_field_value_id'] === (int) $value['price_field_value_id'] + && !in_array($item['financial_item.financial_account_id'], $salesTaxFinancialAccount, TRUE) + ) { + $financialItemID = $item['financial_item.id']; + $currentFinancialItemStatus = $item['financial_item.status_id:name']; + } } - else { + if (!$financialItemID) { $financialItemID = self::getNewFinancialItemID($value, $params['trxn_date'], $contribution['contact_id'], $paymentTrxnParams['currency']); } @@ -118,20 +143,31 @@ class CRM_Financial_BAO_Payment { ]; civicrm_api3('EntityFinancialTrxn', 'create', $eftParams); + if ($currentFinancialItemStatus && 'Paid' !== $currentFinancialItemStatus) { + $newStatus = $value['allocation'] < $value['balance'] ? 'Partially paid' : 'Paid'; + FinancialItem::update(FALSE) + ->addValue('status_id:name', $newStatus) + ->addWhere('id', '=', $financialItemID) + ->execute(); + } - 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); + foreach ($financialItems as $item) { + if ($item['price_field_value_id'] === (int) $value['price_field_value_id'] + && in_array($item['financial_item.financial_account_id'], $salesTaxFinancialAccount, TRUE) + ) { + // @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' => $item['tax_amount'], + ]; + $eftParams['entity_id'] = $item['financial_item.id']; + CRM_Contribute_BAO_Contribution::createProportionalEntry($entityParams, $eftParams); + } } } } diff --git a/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php b/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php index 17686476ac..62fe66a076 100644 --- a/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php +++ b/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php @@ -1147,7 +1147,7 @@ WHERE eft.entity_id = %1 AND ft.to_financial_account_id <> %2"; * to the original payment - ie. .0909 of the $110 is 10 & that * 50 is $4.54 (note the rounding seems wrong as it should be * saved un-rounded). */ - public function testCreateProportionalFinancialEntriesViaPaymentCreate() { + public function testCreateProportionalFinancialEntriesViaPaymentCreate(): void { [$contribution, $financialAccount] = $this->createContributionWithTax([], FALSE); $params = [ 'total_amount' => 50, @@ -1164,7 +1164,7 @@ WHERE eft.entity_id = %1 AND ft.to_financial_account_id <> %2"; 'financial_trxn_id' => $financialTrxn['id'], ]; $entityFinancialTrxn = $this->callAPISuccess('EntityFinancialTrxn', 'Get', $eftParams); - $this->assertEquals($entityFinancialTrxn['count'], 2, 'Invalid count.'); + $this->assertEquals(2, $entityFinancialTrxn['count'], 'Invalid count.'); $testAmount = [4.55, 45.45]; foreach ($entityFinancialTrxn['values'] as $value) { $this->assertEquals(array_pop($testAmount), $value['amount'], 'Invalid amount stored in civicrm_entity_financial_trxn.'); diff --git a/tests/phpunit/CRM/Event/Form/ParticipantTest.php b/tests/phpunit/CRM/Event/Form/ParticipantTest.php index f18c59c7b0..9ec0c4b44d 100644 --- a/tests/phpunit/CRM/Event/Form/ParticipantTest.php +++ b/tests/phpunit/CRM/Event/Form/ParticipantTest.php @@ -713,7 +713,7 @@ class CRM_Event_Form_ParticipantTest extends CiviUnitTestCase { 'contact_id' => $this->getContactID(), 'amount' => 1550.55, 'currency' => 'USD', - 'status_id' => CRM_Core_PseudoConstant::getKey('CRM_Financial_BAO_FinancialItem', 'status_id', 'Unpaid'), + 'status_id' => CRM_Core_PseudoConstant::getKey('CRM_Financial_BAO_FinancialItem', 'status_id', 'Partially paid'), 'entity_table' => 'civicrm_line_item', 'entity_id' => $lineItem['id'], 'financial_account_id' => 4, diff --git a/tests/phpunit/api/v3/ContributionTest.php b/tests/phpunit/api/v3/ContributionTest.php index 478b0f5911..717ce48361 100644 --- a/tests/phpunit/api/v3/ContributionTest.php +++ b/tests/phpunit/api/v3/ContributionTest.php @@ -4412,8 +4412,12 @@ class api_v3_ContributionTest extends CiviUnitTestCase { elseif ($flag === 'single') { $params = array_merge($this->_params, ['contribution_recur_id' => $contributionRecur['id']]); $params = array_merge($params, $contributionParams); - $params['api.Payment.create'] = ['total_amount' => $params['total_amount']]; $originalContribution = $this->callAPISuccess('Order', 'create', $params); + // Note the saved contribution amount will include tax. + $this->callAPISuccess('Payment', 'create', [ + 'contribution_id' => $originalContribution['id'], + 'total_amount' => $originalContribution['values'][$originalContribution['id']]['total_amount'], + ]); } $originalContribution['contribution_recur_id'] = $contributionRecur['id']; $originalContribution['payment_processor_id'] = $paymentProcessorID; diff --git a/tests/phpunit/api/v3/OrderTest.php b/tests/phpunit/api/v3/OrderTest.php index 4f83b9915d..e197268126 100644 --- a/tests/phpunit/api/v3/OrderTest.php +++ b/tests/phpunit/api/v3/OrderTest.php @@ -10,6 +10,7 @@ */ use Civi\Api4\Contribution; +use Civi\Api4\FinancialItem; /** * Test APIv3 civicrm_contribute_* functions @@ -198,7 +199,7 @@ class api_v3_OrderTest extends CiviUnitTestCase { /** * Test create order api for membership * - * @throws \CRM_Core_Exception + * @throws \API_Exception */ public function testAddOrderForMembership(): void { $membershipType = $this->membershipTypeCreate(); @@ -280,9 +281,26 @@ class api_v3_OrderTest extends CiviUnitTestCase { $paymentMembership = [ 'contribution_id' => $order['id'], ]; - $order = $this->callAPISuccess('order', 'get', $paymentMembership); + $order = $this->callAPISuccess('Order', 'get', $paymentMembership); $this->checkPaymentResult($order, $expectedResult); $this->callAPISuccessGetCount('MembershipPayment', $paymentMembership, 2); + $this->callAPISuccess('Payment', 'create', [ + 'contribution_id' => $order['id'], + 'payment_instrument_id' => 'Check', + 'total_amount' => 300, + ]); + foreach (FinancialItem::get(FALSE) + ->addJoin( + 'LineItem AS line_item', + 'INNER', + NULL, + ['entity_table', '=', '"civicrm_line_item"'], + ['entity_id', '=', 'line_item.id'], + ['line_item.contribution_id', '=', $order['id']] + ) + ->addSelect('status_id')->execute() as $item) { + $this->assertEquals('Paid', CRM_Core_PseudoConstant::getName('CRM_Financial_BAO_FinancialItem', 'status_id', $item['status_id'])); + } $this->callAPISuccess('Contribution', 'Delete', [ 'id' => $order['id'], ]); -- 2.25.1