From 4df23f2a270c08bc42fd1ff93fcfe318ecdc0e19 Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Wed, 7 Jul 2021 06:54:32 +1200 Subject: [PATCH] Fixes getTemplateContribution to use a more reliable way to load line items My efforts to add testing a 'deprecate weird stuff' have identified an odd and fragile flow for the line items in getTemplateContribution. It calls getLineItemsByContributionID which, as it turns out, substitues the actual entity table with 'civicrm_contribution'. Then this line of weird handling swoops in and saves the day. https://github.com/civicrm/civicrm-core/pull/20775/files#diff-a16d4d7449cf5f3a0616d1d282a32f27ab6d3f7d2726d076c02ad1d4d655af41R393 This switches us to something cleaner than just loads the line items (with v4 LineItem.get) and no weird handling --- CRM/Contribute/BAO/Contribution.php | 1 - CRM/Contribute/BAO/ContributionRecur.php | 62 +---- CRM/Financial/BAO/Order.php | 239 ++++++++++++++++-- .../Contribute/BAO/ContributionRecurTest.php | 1 - .../CRM/Core/Payment/PayPalIPNTest.php | 4 +- tests/phpunit/CiviTest/CiviUnitTestCase.php | 13 +- tests/phpunit/api/v3/ContributionTest.php | 24 +- 7 files changed, 241 insertions(+), 103 deletions(-) diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php index e0f140a7bd..0e3a8b0e7b 100644 --- a/CRM/Contribute/BAO/Contribution.php +++ b/CRM/Contribute/BAO/Contribution.php @@ -2497,7 +2497,6 @@ LEFT JOIN civicrm_contribution contribution ON ( componentPayment.contribution_ * The count is out on how correct related entities wind up in this case. */ protected static function repeatTransaction(array $input, array $contributionParams) { - $templateContribution = CRM_Contribute_BAO_ContributionRecur::getTemplateContribution( (int) $contributionParams['contribution_recur_id'], array_filter([ diff --git a/CRM/Contribute/BAO/ContributionRecur.php b/CRM/Contribute/BAO/ContributionRecur.php index 0e90e0d765..0fa3623657 100644 --- a/CRM/Contribute/BAO/ContributionRecur.php +++ b/CRM/Contribute/BAO/ContributionRecur.php @@ -453,16 +453,25 @@ INNER JOIN civicrm_contribution con ON ( con.id = mp.contribution_id ) } if ($templateContributions->count()) { $templateContribution = $templateContributions->first(); - $lineItems = CRM_Price_BAO_LineItem::getLineItemsByContributionID($templateContribution['id']); + $order = new CRM_Financial_BAO_Order(); + $order->setTemplateContributionID($templateContribution['id']); + $order->setOverrideFinancialTypeID($overrides['financial_type_id'] ?? NULL); + $order->setOverridableFinancialTypeID($templateContribution['financial_type_id']); + $order->setOverrideTotalAmount($overrides['total_amount'] ?? NULL); + $order->setIsPermitOverrideFinancialTypeForMultipleLines(FALSE); + $lineItems = $order->getLineItems(); // We only permit the financial type to be overridden for single line items. // Otherwise we need to figure out a whole lot of extra complexity. // It's not UI-possible to alter financial_type_id for recurring contributions // with more than one line item. + // The handling of the line items is managed in BAO_Order so this + // is whether we should override on the contribution. Arguably the 2 should + // be decoupled. if (count($lineItems) > 1 && isset($overrides['financial_type_id'])) { unset($overrides['financial_type_id']); } $result = array_merge($templateContribution, $overrides); - $result['line_item'] = self::reformatLineItemsForRepeatContribution($result['total_amount'], $result['financial_type_id'], $lineItems, (array) $templateContribution); + $result['line_item'][$order->getPriceSetID()] = $lineItems; // If the template contribution was made on-behalf then add the // relevant values to ensure the activity reflects that. $relatedContact = CRM_Contribute_BAO_Contribution::getOnbehalfIds($result['id']); @@ -993,53 +1002,4 @@ INNER JOIN civicrm_contribution con ON ( con.id = mp.contribution_id ) return CRM_Core_PseudoConstant::get(__CLASS__, $fieldName, $params, $context); } - /** - * Reformat line items for getTemplateContribution / repeat contribution. - * - * This is an extraction and may be subject to further cleanup. - * - * @param float $total_amount - * @param int $financial_type_id - * @param array $lineItems - * @param array $originalContribution - * - * @return array - */ - protected static function reformatLineItemsForRepeatContribution($total_amount, $financial_type_id, array $lineItems, array $originalContribution): array { - $lineSets = []; - if (count($lineItems) == 1) { - foreach ($lineItems as $index => $lineItem) { - if ($lineItem['financial_type_id'] != $originalContribution['financial_type_id']) { - // CRM-20685, Repeattransaction produces incorrect Financial Type ID (in specific circumstance) - if number of lineItems = 1, So this conditional will set the financial_type_id as the original if line_item and contribution comes with different data. - $financial_type_id = $lineItem['financial_type_id']; - } - if ($financial_type_id) { - // CRM-17718 allow for possibility of changed financial type ID having been set prior to calling this. - $lineItem['financial_type_id'] = $financial_type_id; - } - $taxAmountMatches = FALSE; - if ((!empty($lineItem['tax_amount']) && ($lineItem['line_total'] + $lineItem['tax_amount']) == $total_amount)) { - $taxAmountMatches = TRUE; - } - if ($lineItem['line_total'] != $total_amount && !$taxAmountMatches) { - // We are dealing with a changed amount! Per CRM-16397 we can work out what to do with these - // if there is only one line item, and the UI should prevent this situation for those with more than one. - $lineItem['line_total'] = $total_amount; - $lineItem['unit_price'] = round($total_amount / $lineItem['qty'], 2); - } - $priceField = new CRM_Price_DAO_PriceField(); - $priceField->id = $lineItem['price_field_id']; - $priceField->find(TRUE); - $lineSets[$priceField->price_set_id][$lineItem['price_field_id']] = $lineItem; - } - } - // CRM-19309 if more than one then just pass them through: - elseif (count($lineItems) > 1) { - foreach ($lineItems as $index => $lineItem) { - $lineSets[$index][$lineItem['price_field_id']] = $lineItem; - } - } - return $lineSets; - } - } diff --git a/CRM/Financial/BAO/Order.php b/CRM/Financial/BAO/Order.php index 2388c44526..5dfdc7d2dd 100644 --- a/CRM/Financial/BAO/Order.php +++ b/CRM/Financial/BAO/Order.php @@ -9,6 +9,7 @@ +--------------------------------------------------------------------+ */ +use Civi\Api4\LineItem; use Civi\Api4\PriceField; use Civi\Api4\PriceSet; @@ -58,6 +59,48 @@ class CRM_Financial_BAO_Order { */ protected $overrideFinancialTypeID; + /** + * Overridable financial type id. + * + * When this is set only this financial type will be overridden. + * + * This is relevant to repeat transactions where we want to + * override the type on the line items if it a different financial type has + * been saved against the recurring contribution. However, it the line item + * financial type differs from the contribution financial type then we + * treat this as deliberately uncoupled and don't flow through changes + * in financial type down to the line items. + * + * This is covered in testRepeatTransactionUpdatedFinancialTypeAndNotEquals. + * + * @var int + */ + protected $overridableFinancialTypeID; + + /** + * Get overridable financial type id. + * + * If only one financial type id can be overridden at the line item level + * then get it here, otherwise NULL. + * + * @return int|null + */ + public function getOverridableFinancialTypeID(): ?int { + return $this->overridableFinancialTypeID; + } + + /** + * Set overridable financial type id. + * + * If only one financial type id can be overridden at the line item level + * then set it here. + * + * @param int|null $overridableFinancialTypeID + */ + public function setOverridableFinancialTypeID(?int $overridableFinancialTypeID): void { + $this->overridableFinancialTypeID = $overridableFinancialTypeID; + } + /** * Financial type id to use for any lines where is is not provided. * @@ -65,6 +108,75 @@ class CRM_Financial_BAO_Order { */ protected $defaultFinancialTypeID; + /** + * ID of a contribution to be used as a template. + * + * @var int + */ + protected $templateContributionID; + + /** + * Should we permit the line item financial type to be overridden when there is more than one line. + * + * Historically the answer is 'yes' for v3 order api and 'no' for repeattransaction + * and backoffice forms. + * + * @var bool + */ + protected $isPermitOverrideFinancialTypeForMultipleLines = TRUE; + + /** + * @return bool + */ + public function isPermitOverrideFinancialTypeForMultipleLines(): bool { + return $this->isPermitOverrideFinancialTypeForMultipleLines; + } + + /** + * @param bool $isPermitOverrideFinancialTypeForMultipleLines + */ + public function setIsPermitOverrideFinancialTypeForMultipleLines(bool $isPermitOverrideFinancialTypeForMultipleLines): void { + $this->isPermitOverrideFinancialTypeForMultipleLines = $isPermitOverrideFinancialTypeForMultipleLines; + } + + /** + * Number of line items. + * + * @var int + */ + protected $lineItemCount; + + /** + * @return int + */ + public function getLineItemCount(): int { + if (!isset($this->lineItemCount)) { + $this->lineItemCount = count($this->getPriceOptions()) || count($this->lineItems); + } + return $this->lineItemCount; + } + + /** + * @param int $lineItemCount + */ + public function setLineItemCount(int $lineItemCount): void { + $this->lineItemCount = $lineItemCount; + } + + /** + * @return int|null + */ + public function getTemplateContributionID(): ?int { + return $this->templateContributionID; + } + + /** + * @param int $templateContributionID + */ + public function setTemplateContributionID(int $templateContributionID): void { + $this->templateContributionID = $templateContributionID; + } + /** * @return int */ @@ -179,20 +291,43 @@ class CRM_Financial_BAO_Order { public function getOverrideTotalAmount() { // The override amount is only valid for quick config price sets where more // than one field has not been selected. - if (!$this->overrideTotalAmount || !$this->supportsOverrideAmount() || count($this->getPriceOptions()) > 1) { + if (!$this->overrideTotalAmount || $this->getLineItemCount() > 1) { return FALSE; } return $this->overrideTotalAmount; } + /** + * Is the line item financial type to be overridden. + * + * We have a tested scenario for repeatcontribution where the line item + * does not match the top level financial type for the order. In this case + * any financial type override has been determined to NOT apply to the line items. + * + * This is locked in via testRepeatTransactionUpdatedFinancialTypeAndNotEquals. + * + * @param int $financialTypeID + * + * @return bool + */ + public function isOverrideLineItemFinancialType(int $financialTypeID) { + if (!$this->getOverrideFinancialTypeID()) { + return FALSE; + } + if (!$this->getOverridableFinancialTypeID()) { + return TRUE; + } + return $this->getOverridableFinancialTypeID() === $financialTypeID; + } + /** * Set override for total amount. * * @internal use in tested core code only. * - * @param float $overrideTotalAmount + * @param float|null $overrideTotalAmount */ - public function setOverrideTotalAmount(float $overrideTotalAmount): void { + public function setOverrideTotalAmount(?float $overrideTotalAmount): void { $this->overrideTotalAmount = $overrideTotalAmount; } @@ -204,7 +339,11 @@ class CRM_Financial_BAO_Order { * @return int| FALSE */ public function getOverrideFinancialTypeID() { - if (count($this->getPriceOptions()) !== 1) { + // We don't permit overrides if there is more than one line. + // The reason for this constraint may be more historical since + // the case could be made that if it is set it should be used and + // we have built out the tax calculations a lot now. + if (!$this->isPermitOverrideFinancialTypeForMultipleLines() && $this->getLineItemCount() > 1) { return FALSE; } return $this->overrideFinancialTypeID ?? FALSE; @@ -215,9 +354,9 @@ class CRM_Financial_BAO_Order { * * @internal use in tested core code only. * - * @param int $overrideFinancialTypeID + * @param int|null $overrideFinancialTypeID */ - public function setOverrideFinancialTypeID(int $overrideFinancialTypeID): void { + public function setOverrideFinancialTypeID(?int $overrideFinancialTypeID): void { $this->overrideFinancialTypeID = $overrideFinancialTypeID; } @@ -265,17 +404,6 @@ class CRM_Financial_BAO_Order { ->first()['id']; } - /** - * Is overriding the total amount valid for this price set. - * - * @internal tested. core code use only. - * - * @return bool - */ - public function supportsOverrideAmount(): bool { - return (bool) $this->getPriceSetMetadata()['is_quick_config']; - } - /** * Set price set ID based on the contribution page id. * @@ -283,7 +411,6 @@ class CRM_Financial_BAO_Order { * * @param int $contributionPageID * - * @throws \CiviCRM_API3_Exception */ public function setPriceSetIDByContributionPageID(int $contributionPageID): void { $this->setPriceSetIDByEntity('contribution_page', $contributionPageID); @@ -550,7 +677,8 @@ class CRM_Financial_BAO_Order { /** * @return array - * @throws \CiviCRM_API3_Exception + * + * @throws \API_Exception */ protected function calculateLineItems(): array { $lineItems = []; @@ -564,20 +692,33 @@ class CRM_Financial_BAO_Order { // Dummy value to prevent e-notice in getLine. We calculate tax in this class. $params['financial_type_id'] = 0; - foreach ($this->getPriceOptions() as $fieldID => $valueID) { - $this->setPriceSetIDFromSelectedField($fieldID); - $throwAwayArray = []; - // @todo - still using getLine for now but better to bring it to this class & do a better job. - $newLines = CRM_Price_BAO_PriceSet::getLine($params, $throwAwayArray, $this->getPriceSetID(), $this->getPriceFieldSpec($fieldID), $fieldID)[1]; - foreach ($newLines as $newLine) { - $lineItems[$newLine['price_field_value_id']] = $newLine; + if ($this->getTemplateContributionID()) { + $lineItems = $this->getLinesFromTemplateContribution(); + } + else { + foreach ($this->getPriceOptions() as $fieldID => $valueID) { + $this->setPriceSetIDFromSelectedField($fieldID); + $throwAwayArray = []; + // @todo - still using getLine for now but better to bring it to this class & do a better job. + $newLines = CRM_Price_BAO_PriceSet::getLine($params, $throwAwayArray, $this->getPriceSetID(), $this->getPriceFieldSpec($fieldID), $fieldID)[1]; + foreach ($newLines as $newLine) { + $lineItems[$newLine['price_field_value_id']] = $newLine; + } } } + // Set the line item count here because it is needed to determine whether + // we can use overrides and would not be set yet if we have loaded them from + // a template contribution. + $this->setLineItemCount(count($lineItems)); foreach ($lineItems as &$lineItem) { + // Set the price set id if not set above. Note that the above + // requires it for line retrieval but we want to fix that as it + // should not be required at that point. + $this->setPriceSetIDFromSelectedField($lineItem['price_field_id']); // Set any pre-calculation to zero as we will calculate. $lineItem['tax_amount'] = 0; - if ($this->getOverrideFinancialTypeID() !== FALSE) { + if ($this->isOverrideLineItemFinancialType($lineItem['financial_type_id']) !== FALSE) { $lineItem['financial_type_id'] = $this->getOverrideFinancialTypeID(); } $taxRate = $this->getTaxRate((int) $lineItem['financial_type_id']); @@ -801,4 +942,48 @@ class CRM_Financial_BAO_Order { } } + /** + * Get the line items from a template. + * + * @return \Civi\Api4\Generic\Result + * + * @throws \API_Exception + */ + protected function getLinesFromTemplateContribution(): array { + $lines = $this->getLinesForContribution(); + foreach ($lines as &$line) { + // The apiv4 insists on adding id - so let it get all the details + // and we will filter out those that are not part of a template here. + unset($line['id'], $line['contribution_id']); + } + return $lines; + } + + /** + * @return array + * @throws \API_Exception + * @throws \Civi\API\Exception\UnauthorizedException + */ + protected function getLinesForContribution(): array { + return (array) LineItem::get(FALSE) + ->addWhere('contribution_id', '=', $this->getTemplateContributionID()) + ->setSelect([ + 'contribution_id', + 'entity_id', + 'entity_table', + 'price_field_id', + 'price_field_value_id', + 'financial_type_id', + 'label', + 'qty', + 'unit_price', + 'line_total', + 'tax_amount', + 'non_deductible_amount', + 'participant_count', + 'membership_num_terms', + ]) + ->execute(); + } + } diff --git a/tests/phpunit/CRM/Contribute/BAO/ContributionRecurTest.php b/tests/phpunit/CRM/Contribute/BAO/ContributionRecurTest.php index c8904af7c0..108a540892 100644 --- a/tests/phpunit/CRM/Contribute/BAO/ContributionRecurTest.php +++ b/tests/phpunit/CRM/Contribute/BAO/ContributionRecurTest.php @@ -218,7 +218,6 @@ class CRM_Contribute_BAO_ContributionRecurTest extends CiviUnitTestCase { * Test that is_template contribution is used where available * * @throws \API_Exception - * @throws \CRM_Core_Exception * @throws \CiviCRM_API3_Exception * @throws \Civi\API\Exception\UnauthorizedException */ diff --git a/tests/phpunit/CRM/Core/Payment/PayPalIPNTest.php b/tests/phpunit/CRM/Core/Payment/PayPalIPNTest.php index 6541e9f5d1..e38822626e 100644 --- a/tests/phpunit/CRM/Core/Payment/PayPalIPNTest.php +++ b/tests/phpunit/CRM/Core/Payment/PayPalIPNTest.php @@ -129,7 +129,7 @@ class CRM_Core_Payment_PayPalIPNTest extends CiviUnitTestCase { $this->assertEquals(1, $contribution1['contribution_status_id']); $this->assertEquals('8XA571746W2698126', $contribution1['trxn_id']); // source gets set by processor - $this->assertTrue(substr($contribution1['contribution_source'], 0, 20) === 'Online Contribution:'); + $this->assertEquals('Online Contribution:', substr($contribution1['contribution_source'], 0, 20)); $contributionRecur = $this->callAPISuccess('contribution_recur', 'getsingle', ['id' => $this->_contributionRecurID]); $this->assertEquals(5, $contributionRecur['contribution_status_id']); $paypalIPN = new CRM_Core_Payment_PayPalIPN($this->getPaypalRecurSubsequentTransaction()); @@ -207,7 +207,7 @@ class CRM_Core_Payment_PayPalIPNTest extends CiviUnitTestCase { * @throws \CRM_Core_Exception * @throws \CiviCRM_API3_Exception */ - public function testIPNPaymentInputMembershipRecurSuccess() { + public function testIPNPaymentInputMembershipRecurSuccess(): void { $durationUnit = 'year'; $this->setupMembershipRecurringPaymentProcessorTransaction(['duration_unit' => $durationUnit, 'frequency_unit' => $durationUnit]); $membershipPayment = $this->callAPISuccessGetSingle('membership_payment', []); diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php index 1790e50ea5..324c08380c 100644 --- a/tests/phpunit/CiviTest/CiviUnitTestCase.php +++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php @@ -2558,11 +2558,9 @@ VALUES 'total_amount' => '200', 'invoice_id' => $this->_invoiceID, 'financial_type_id' => 'Donation', - 'contribution_status_id' => 'Pending', 'contact_id' => $this->_contactID, 'contribution_page_id' => $this->_contributionPageID, 'payment_processor_id' => $this->_paymentProcessorID, - 'is_test' => 0, 'receive_date' => '2019-07-25 07:34:23', 'skipCleanMoney' => TRUE, 'amount_level' => 'expensive', @@ -2818,11 +2816,9 @@ VALUES $paramsSet['financial_type_id'] = 'Event Fee'; $paramsSet['extends'] = 1; $priceSet = $this->callAPISuccess('price_set', 'create', $paramsSet); - $priceSetId = $priceSet['id']; - //Checking for priceset added in the table. - $this->assertDBCompareValue('CRM_Price_BAO_PriceSet', $priceSetId, 'title', - 'id', $paramsSet['title'], 'Check DB for created priceset' - ); + if ($componentId) { + CRM_Price_BAO_PriceSet::addTo('civicrm_' . $component, $componentId, $priceSet['id']); + } $paramsField = array_merge([ 'label' => 'Price Field', 'name' => CRM_Utils_String::titleToVar('Price Field'), @@ -2842,9 +2838,6 @@ VALUES ], $priceFieldOptions); $priceField = CRM_Price_BAO_PriceField::create($paramsField); - if ($componentId) { - CRM_Price_BAO_PriceSet::addTo('civicrm_' . $component, $componentId, $priceSetId); - } return $this->callAPISuccess('PriceFieldValue', 'get', ['price_field_id' => $priceField->id]); } diff --git a/tests/phpunit/api/v3/ContributionTest.php b/tests/phpunit/api/v3/ContributionTest.php index d8bd66522f..64ef45865c 100644 --- a/tests/phpunit/api/v3/ContributionTest.php +++ b/tests/phpunit/api/v3/ContributionTest.php @@ -2410,19 +2410,21 @@ class api_v3_ContributionTest extends CiviUnitTestCase { ]; $lineItem1 = $this->callAPISuccess('line_item', 'get', array_merge($lineItemParams, [ 'entity_id' => $originalContribution['id'], - ])); + 'options' => ['sort' => 'qty'], + ]))['values']; $lineItem2 = $this->callAPISuccess('line_item', 'get', array_merge($lineItemParams, [ 'entity_id' => $originalContribution['id'] + 1, - ])); + 'options' => ['sort' => 'qty'], + ]))['values']; // unset id and entity_id for all of them to be able to compare the lineItems: - unset($lineItem1['values'][0]['id'], $lineItem1['values'][0]['entity_id']); - unset($lineItem2['values'][0]['id'], $lineItem2['values'][0]['entity_id']); - $this->assertEquals($lineItem1['values'][0], $lineItem2['values'][0]); + unset($lineItem1[0]['id'], $lineItem1[0]['entity_id']); + unset($lineItem2[0]['id'], $lineItem2[0]['entity_id']); + $this->assertEquals($lineItem1[0], $lineItem2[0]); - unset($lineItem1['values'][1]['id'], $lineItem1['values'][1]['entity_id']); - unset($lineItem2['values'][1]['id'], $lineItem2['values'][1]['entity_id']); - $this->assertEquals($lineItem1['values'][1], $lineItem2['values'][1]); + unset($lineItem1[1]['id'], $lineItem1[1]['entity_id']); + unset($lineItem2[1]['id'], $lineItem2[1]['entity_id']); + $this->assertEquals($lineItem1[1], $lineItem2[1]); // CRM-19309 so in future we also want to: // check that financial_line_items have been created for entity_id 3 and 4; @@ -2892,9 +2894,9 @@ class api_v3_ContributionTest extends CiviUnitTestCase { ]); $lineItem1 = $this->callAPISuccess('line_item', 'get', array_merge($lineItemParams, [ 'entity_id' => $originalContribution['id'], - ])); + ]))['values'][0]; $expectedLineItem = array_merge( - $lineItem1['values'][0], [ + $lineItem1, [ 'line_total' => '100.00', 'unit_price' => '100.00', 'financial_type_id' => 2, @@ -3001,7 +3003,7 @@ class api_v3_ContributionTest extends CiviUnitTestCase { * * @throws \CRM_Core_Exception */ - public function testRepeatTransactionUpdatedFinancialTypeAndNotEquals() { + public function testRepeatTransactionUpdatedFinancialTypeAndNotEquals(): void { $originalContribution = $this->setUpRecurringContribution([], ['financial_type_id' => 2]); // This will made the trick to get the not equals behaviour. $this->callAPISuccess('line_item', 'create', ['id' => 1, 'financial_type_id' => 4]); -- 2.25.1