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
civicrm · Jul 8, 2021 · 76f9166 · 76f9166
1 parent ed8a9f9
commit 76f9166
Show file tree

Hide file tree

Showing 6 changed files with 112 additions and 85 deletions.
diff --git 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
@@ -453,16 +453,23 @@ 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']);
+      $order->setOverrideFinancialTypeID($overrides['financial_type_id'] ?? NULL);
+      $order->setOverrideTotalAmount($overrides['total_amount'] ?? NULL);
+      $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 +1000,4 @@ public static function buildOptions($fieldName, $context = NULL, $props = []) {
     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
@@ -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
    */
@@ -190,9 +212,9 @@ public function getOverrideTotalAmount() {
    *
    * @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;
   }
 
@@ -215,9 +237,9 @@ public function getOverrideFinancialTypeID() {
    *
    * @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;
   }
 
@@ -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
@@ -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
@@ -2558,11 +2558,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',
@@ -2818,11 +2816,9 @@ protected function createPriceSet($component = 'contribution_page', $componentId
     $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 @@ protected function createPriceSet($component = 'contribution_page', $componentId
     ], $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
@@ -2410,19 +2410,21 @@ public function testRepeatTransactionLineItems(): void {
     ];
     $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;