Clean up & clarify code splitting line items in Contribution page membership flow

It was really confusing - but now it is separated into a function which explains it

Note there is specic test cover in
testSubmitMembershipComplexPriceSetPaymentPaymentProcessorRecurInstantPayment
and the changes to that test reflects the fact that the order in which
the line items are created changes slightly (in a fundamentally neutral way

diff --git a/CRM/Contribute/Form/Contribution/Confirm.php b/CRM/Contribute/Form/Contribution/Confirm.php
index 88bed9766ce99242928263a469a01b680da90995..c61c44f8ddc503286f7e576dc9616a9d4bc55132 100644 (file)
--- a/CRM/Contribute/Form/Contribution/Confirm.php
+++ b/CRM/Contribute/Form/Contribution/Confirm.php
@@ -1474,29 +1474,11 @@ class CRM_Contribute_Form_Contribution_Confirm extends CRM_Contribute_Form_Contr
     //@todo it should no longer be possible for it to get to this point & membership to not be an array
     if (is_array($membershipTypeIDs) && !empty($membershipContributionID)) {
       $typesTerms = $membershipParams['types_terms'] ?? [];
-
-      $membershipLines = $nonMembershipLines = [];
-      foreach ($unprocessedLineItems as $priceSetID => $lines) {
-        foreach ($lines as $line) {
-          if (!empty($line['membership_type_id'])) {
-            $membershipLines[$line['membership_type_id']] = $line['price_field_value_id'];
-          }
-        }
-      }
-
-      $i = 1;
       $this->_params['createdMembershipIDs'] = [];
-      foreach ($membershipTypeIDs as $memType) {
-        $membershipLineItems = [];
-        if ($i < count($membershipTypeIDs)) {
-          $membershipLineItems[$priceSetID][$membershipLines[$memType]] = $unprocessedLineItems[$priceSetID][$membershipLines[$memType]];
-          unset($unprocessedLineItems[$priceSetID][$membershipLines[$memType]]);
-        }
-        else {
-          $membershipLineItems = $unprocessedLineItems;
-        }
-        $i++;
-        $numTerms = $typesTerms[$memType] ?? 1;
+      $firstMembershipTypeID = reset($membershipTypeIDs);
+      foreach ($membershipTypeIDs as $membershipTypeID) {
+        $membershipLineItems = [$this->getPriceSetID() => $this->getLineItemsForMembershipCreate((int) $membershipTypeID, $firstMembershipTypeID)];
+        $numTerms = $typesTerms[$membershipTypeID] ?? 1;
         $contributionRecurID = $this->_params['contributionRecurID'] ?? NULL;
 
         $membershipSource = NULL;
@@ -1524,7 +1506,7 @@ class CRM_Contribute_Form_Contribution_Confirm extends CRM_Contribute_Form_Contr
         }
 
         [$membership, $renewalMode, $dates] = self::legacyProcessMembership(
-          $contactID, $memType, $isTest,
+          $contactID, $membershipTypeID, $isTest,
           date('YmdHis'), $membershipParams['cms_contactID'] ?? NULL,
           $customFieldsFormatted,
           $numTerms, $membershipID, $pending,
@@ -2951,4 +2933,36 @@ class CRM_Contribute_Form_Contribution_Confirm extends CRM_Contribute_Form_Contr
     return [$membership, $renewalMode, $dates];
   }
 
+  /**
+   * Get the line items for the membership create call.
+   *
+   * This form follows a legacy code path - rather than creating the membership
+   * and then creating the contribution / order with the right line items it
+   * creates the contribution and then the membership & uses some old code in
+   * the membership BAO to create the line items. In order to do this it needs to
+   * separate out the line items into the separate membership create calls.
+   * However, any non-membership lines need to be assigned to one & only one
+   * of these line item splits.
+   *
+   * Where we have separate payments enabled then we should ignore any non-membership
+   * line items as they will have been processed as part of the non-membership
+   * contribution.
+   *
+   * @param int $membershipTypeID
+   * @param int $defaultMembershipTypeID
+   *
+   * @return array
+   * @throws \CRM_Core_Exception
+   */
+  protected function getLineItemsForMembershipCreate(int $membershipTypeID, int $defaultMembershipTypeID): array {
+    $lineItemSplit = [];
+    foreach ($this->getLineItems() as $lineItem) {
+      if (empty($lineItem['membership_type_id']) && $this->isSeparateMembershipPayment()) {
+        continue;
+      }
+      $lineItemSplit[$lineItem['membership_type_id'] ?: $defaultMembershipTypeID][$lineItem['price_field_id']] = $lineItem;
+    }
+    return $lineItemSplit[$membershipTypeID];
+  }
+
 }

diff --git a/CRM/Financial/BAO/Order.php b/CRM/Financial/BAO/Order.php
index 7bc308338e0b5676468b2046bb46f46970330226..5be8addb19ad9f525d93e0f6efb3b5fd40c67a2a 100644 (file)
--- a/CRM/Financial/BAO/Order.php
+++ b/CRM/Financial/BAO/Order.php
@@ -947,6 +947,9 @@ class CRM_Financial_BAO_Order {
       elseif ($taxRate) {
         $lineItem['tax_amount'] = ($taxRate / 100) * $lineItem['line_total'];
       }
+      if (!empty($lineItem['membership_type_id'])) {
+        $lineItem['entity_table'] = 'civicrm_membership';
+      }
       $lineItem['title'] = $this->getLineItemTitle($lineItem);
       $lineItem['line_total_inclusive'] = $lineItem['line_total'] + $lineItem['tax_amount'];
     }

diff --git a/tests/phpunit/api/v3/ContributionPageTest.php b/tests/phpunit/api/v3/ContributionPageTest.php
index ba7adb63853a3ff33845f6d88256a828bc894424..982c6b71dd5d878317e0a929c74af572a52c7946 100644 (file)
--- a/tests/phpunit/api/v3/ContributionPageTest.php
+++ b/tests/phpunit/api/v3/ContributionPageTest.php
@@ -1683,10 +1683,10 @@ class api_v3_ContributionPageTest extends CiviUnitTestCase {
       'contribution_id' => $id,
     ])['values'];
     $this->assertCount(3, $lines);
-    $this->assertEquals('civicrm_membership', $lines[0]['entity_table']);
-    $this->assertEquals($preExistingMembershipID + 1, $lines[0]['entity_id']);
-    $this->assertEquals('civicrm_contribution', $lines[1]['entity_table']);
-    $this->assertEquals($id, $lines[1]['entity_id']);
+    $this->assertEquals('civicrm_membership', $lines[1]['entity_table']);
+    $this->assertEquals($preExistingMembershipID + 1, $lines[1]['entity_id']);
+    $this->assertEquals('civicrm_contribution', $lines[0]['entity_table']);
+    $this->assertEquals($id, $lines[0]['entity_id']);
     $this->assertEquals('civicrm_membership', $lines[2]['entity_table']);
     return $lines;
   }