From 5b6fa91f5710f101b48181941b4eaf9756d99730 Mon Sep 17 00:00:00 2001
From: Eileen McNaughton <emcnaughton@wikimedia.org>
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 | 4 +-
CRM/Financial/BAO/Order.php | 93 +++++++++++++++++--
.../CRM/Core/Payment/PayPalIPNTest.php | 4 +-
tests/phpunit/CiviTest/CiviUnitTestCase.php | 2 -
5 files changed, 89 insertions(+), 15 deletions(-)
diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php
index e0f140a7bde0..0e3a8b0e7b1b 100644
--- a/CRM/Contribute/BAO/Contribution.php
+++ b/CRM/Contribute/BAO/Contribution.php
@@ -2497,7 +2497,6 @@ public static function contributionCount($contactId, $includeSoftCredit = TRUE)
* 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 0e90e0d76523..0af060ca2b97 100644
--- a/CRM/Contribute/BAO/ContributionRecur.php
+++ b/CRM/Contribute/BAO/ContributionRecur.php
@@ -453,7 +453,9 @@ public static function getTemplateContribution(int $id, $overrides = []): array
}
if ($templateContributions->count()) {
$templateContribution = $templateContributions->first();
- $lineItems = CRM_Price_BAO_LineItem::getLineItemsByContributionID($templateContribution['id']);
+ $order = new CRM_Financial_BAO_Order();
+ $order->setTemplateContributionID($templateContribution['id']);
+ $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
diff --git a/CRM/Financial/BAO/Order.php b/CRM/Financial/BAO/Order.php
index 2388c445268c..5de2f7f8c94c 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;
@@ -65,6 +66,27 @@ class CRM_Financial_BAO_Order {
*/
protected $defaultFinancialTypeID;
+ /**
+ * ID of a contribution to be used as a template.
+ *
+ * @var int
+ */
+ protected $templateContributionID;
+
+ /**
+ * @return int|null
+ */
+ public function getTemplateContributionID(): ?int {
+ return $this->templateContributionID;
+ }
+
+ /**
+ * @param int $templateContributionID
+ */
+ public function setTemplateContributionID(int $templateContributionID): void {
+ $this->templateContributionID = $templateContributionID;
+ }
+
/**
* @return int
*/
@@ -283,7 +305,6 @@ public function supportsOverrideAmount(): bool {
*
* @param int $contributionPageID
*
- * @throws \CiviCRM_API3_Exception
*/
public function setPriceSetIDByContributionPageID(int $contributionPageID): void {
$this->setPriceSetIDByEntity('contribution_page', $contributionPageID);
@@ -550,7 +571,8 @@ public function getRenewableMembershipTypes(): array {
/**
* @return array
- * @throws \CiviCRM_API3_Exception
+ *
+ * @throws \API_Exception
*/
protected function calculateLineItems(): array {
$lineItems = [];
@@ -564,17 +586,26 @@ protected function calculateLineItems(): array {
// 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;
+ }
}
}
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) {
@@ -801,4 +832,48 @@ protected function addTotalsToLineBasedOnOverrideTotal(int $financialTypeID, arr
}
}
+ /**
+ * 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/Core/Payment/PayPalIPNTest.php b/tests/phpunit/CRM/Core/Payment/PayPalIPNTest.php
index 6541e9f5d164..e38822626ee2 100644
--- a/tests/phpunit/CRM/Core/Payment/PayPalIPNTest.php
+++ b/tests/phpunit/CRM/Core/Payment/PayPalIPNTest.php
@@ -129,7 +129,7 @@ public function testIPNPaymentRecurSuccess(): void {
$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 @@ public function testIPNPaymentMembershipRecurSuccess() {
* @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 144fe93c2817..d99bb2ee2915 100644
--- a/tests/phpunit/CiviTest/CiviUnitTestCase.php
+++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php
@@ -2557,11 +2557,9 @@ public function setupRecurringPaymentProcessorTransaction($recurParams = [], $co
'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',