[REF] Move wrangling of Front end form contribution param for autoRen…

…ew back to form The processAmount function is really problematic because it's trying to do several disparate things & is called from all over the place with unclear intent. From looking at it I see it does a few things. It reformats line items, it does some obscure & likely flawed filtering, it generates a total cost & a tax cost and it generates a very specifically formatted array of autoreneal properties, The way I see this going is 1) Move the formatting of the autorenewal back to the calling form (this PR) & simplify it 2) Split out the foreach so it goes through once & formats - this can be shoved out to a separate function - and then it goes through the formatted array & calculates total_amount & tax_amount - we should have a wrapper function that just returns these & we might see that is most of what is needed 3) Move all that awful partial_payment_total stuff back to the event form. Note that we are working to entirely remove that from here are it makes so much less than no sense. 4) Calculates amount_level text - that should have it's own function. It worth noting all of this does very little intensive work - a DB lookup or 2 that could be cached & an iteration through a very small array so it would be fine to have 3 functions - - getAmountTotal - getTaxTotal - getAmountText that each go through the same process of generating a formatted array from price_x => 5 etc rather than trying to pass the array around for 'performance' or to 'save work'. From previous refactorings I would suggest we add an Order class where by you set the price fields & then you can call 'getLineItems' - but that is a few steps after this....
civicrm · Nov 22, 2019 · bdcbbfe · bdcbbfe
1 parent 0be5362
commit bdcbbfe
Show file tree

Hide file tree

Showing 8 changed files with 42 additions and 48 deletions.
diff --git a/CRM/Contribute/Form/Contribution.php b/CRM/Contribute/Form/Contribution.php
@@ -1380,7 +1380,7 @@ protected function submit($submittedValues, $action, $pledgePaymentID) {
     // as a point of fragility rather than a logical 'if' clause.
     if ($priceSetId) {
       CRM_Price_BAO_PriceSet::processAmount($this->_priceSet['fields'],
-        $submittedValues, $lineItem[$priceSetId], NULL, $priceSetId);
+        $submittedValues, $lineItem[$priceSetId], $priceSetId);
       // Unset tax amount for offline 'is_quick_config' contribution.
       // @todo WHY  - quick config was conceived as a quick way to configure contribution forms.
       // this is an example of 'other' functionality being hung off it.

diff --git a/CRM/Contribute/Form/Contribution/Confirm.php b/CRM/Contribute/Form/Contribution/Confirm.php
@@ -1967,7 +1967,7 @@ public static function submit($params) {
     }
     $priceFields = $priceFields[$priceSetID]['fields'];
     $lineItems = [];
-    CRM_Price_BAO_PriceSet::processAmount($priceFields, $paramsProcessedForForm, $lineItems, 'civicrm_contribution', $priceSetID);
+    $form->processAmountAndGetAutoRenew($priceFields, $paramsProcessedForForm, $lineItems, $priceSetID);
     $form->_lineItem = [$priceSetID => $lineItems];
     $membershipPriceFieldIDs = [];
     foreach ((array) $lineItems as $lineItem) {

diff --git a/CRM/Contribute/Form/Contribution/Main.php b/CRM/Contribute/Form/Contribution/Main.php
@@ -1124,12 +1124,14 @@ public function submit($params) {
           }
         }
       }
-      $component = '';
+
       if ($this->_membershipBlock) {
-        $component = 'membership';
+        $this->processAmountAndGetAutoRenew($this->_values['fee'], $params, $lineItem[$priceSetId], $priceSetId);
+      }
+      else {
+        CRM_Price_BAO_PriceSet::processAmount($this->_values['fee'], $params, $lineItem[$priceSetId], $priceSetId);
       }
 
-      CRM_Price_BAO_PriceSet::processAmount($this->_values['fee'], $params, $lineItem[$priceSetId], $component, $priceSetId);
       if ($params['tax_amount']) {
         $this->set('tax_amount', $params['tax_amount']);
       }

diff --git a/CRM/Contribute/Form/ContributionBase.php b/CRM/Contribute/Form/ContributionBase.php
@@ -1403,4 +1403,33 @@ protected function getMainContributionAmount($params) {
     return $params['amount'] ?? 0;
   }
 
+  /**
+   * Wrapper for processAmount that also sets autorenew.
+   *
+   * @param $fields
+   *   This is the output of the function CRM_Price_BAO_PriceSet::getSetDetail($priceSetID, FALSE, FALSE);
+   *   And, it would make sense to introduce caching into that function and call it from here rather than
+   *   require the $fields array which is passed from pillar to post around the form in order to pass it in here.
+   * @param array $params
+   *   Params reflecting form input e.g with fields 'price_5' => 7, 'price_8' => array(7, 8)
+   * @param $lineItems
+   *   Line item array to be altered.
+   * @param int $priceSetID
+   */
+  public function processAmountAndGetAutoRenew($fields, &$params, &$lineItems, $priceSetID = NULL) {
+    CRM_Price_BAO_PriceSet::processAmount($fields, $params, $lineItems, $priceSetID);
+    $autoRenew = [];
+    $autoRenew[0] = $autoRenew[1] = $autoRenew[2] = 0;
+    foreach ($lineItems as $lineItem) {
+      if (!empty($lineItem['auto_renew']) &&
+        is_numeric($lineItem['auto_renew'])
+      ) {
+        $autoRenew[$lineItem['auto_renew']] += $lineItem['line_total'];
+      }
+    }
+    if (count($autoRenew) > 1) {
+      $params['autoRenew'] = $autoRenew;
+    }
+  }
+
 }
diff --git a/CRM/Event/Form/Registration/Confirm.php b/CRM/Event/Form/Registration/Confirm.php
@@ -375,6 +375,9 @@ public static function formRule($fields, $files, $self) {
 
   /**
    * Process the form submission.
+   *
+   * @throws \CiviCRM_API3_Exception
+   * @throws \CRM_Core_Exception
    */
   public function postProcess() {
     $now = date('YmdHis');

diff --git a/CRM/Member/Form/Membership.php b/CRM/Member/Form/Membership.php
@@ -1153,7 +1153,7 @@ public function submit() {
     // END Fix for dev/core/issues/860
 
     CRM_Price_BAO_PriceSet::processAmount($this->_priceSet['fields'],
-      $formValues, $lineItem[$this->_priceSetId], NULL, $this->_priceSetId);
+      $formValues, $lineItem[$this->_priceSetId], $this->_priceSetId);
 
     if (!empty($formValues['tax_amount'])) {
       $params['tax_amount'] = $formValues['tax_amount'];

diff --git a/CRM/Member/Form/MembershipRenewal.php b/CRM/Member/Form/MembershipRenewal.php
@@ -597,7 +597,7 @@ protected function submit() {
       $lineItem = [];
       $this->_params = $this->setPriceSetParameters($this->_params);
       CRM_Price_BAO_PriceSet::processAmount($this->_priceSet['fields'],
-        $this->_params, $lineItem[$this->_priceSetId], NULL, $this->_priceSetId
+        $this->_params, $lineItem[$this->_priceSetId], $this->_priceSetId
       );
       //CRM-11529 for quick config backoffice transactions
       //when financial_type_id is passed in form, update the

diff --git a/CRM/Price/BAO/PriceSet.php b/CRM/Price/BAO/PriceSet.php
@@ -660,12 +660,9 @@ public static function initSet(&$form, $entityTable = 'civicrm_event', $validOnl
    *   Params reflecting form input e.g with fields 'price_5' => 7, 'price_8' => array(7, 8)
    * @param $lineItem
    *   Line item array to be altered.
-   * @param string $component
-   *   This parameter appears to only be relevant to determining whether memberships should be auto-renewed.
-   *   (and is effectively a boolean for 'is_membership' which could be calculated from the line items.)
    * @param int $priceSetID
    */
-  public static function processAmount($fields, &$params, &$lineItem, $component = '', $priceSetID = NULL) {
+  public static function processAmount($fields, &$params, &$lineItem, $priceSetID = NULL) {
     // using price set
     $totalPrice = $totalTax = 0;
     // CRM-18701 Sometimes the amount in the price set is overridden by the amount on the form.
@@ -675,10 +672,6 @@ public static function processAmount($fields, &$params, &$lineItem, $component =
     // set up (which allows a free form field).
     $amount_override = NULL;
 
-    if ($component) {
-      $autoRenew = [];
-      $autoRenew[0] = $autoRenew[1] = $autoRenew[2] = 0;
-    }
     if ($priceSetID) {
       $priceFields = self::filterPriceFieldsFromParams($priceSetID, $params);
       if (count($priceFields) == 1) {
@@ -730,15 +723,6 @@ public static function processAmount($fields, &$params, &$lineItem, $component =
             }
           }
           $totalPrice += $lineItem[$optionValueId]['line_total'] + CRM_Utils_Array::value('tax_amount', $lineItem[$optionValueId]);
-          if (
-            $component &&
-            // auto_renew exists and is empty in some workflows, which php treat as a 0
-            // and hence we explicitly check to see if auto_renew is numeric
-            isset($lineItem[$optionValueId]['auto_renew']) &&
-            is_numeric($lineItem[$optionValueId]['auto_renew'])
-          ) {
-            $autoRenew[$lineItem[$optionValueId]['auto_renew']] += $lineItem[$optionValueId]['line_total'];
-          }
           break;
 
         case 'Select':
@@ -750,13 +734,6 @@ public static function processAmount($fields, &$params, &$lineItem, $component =
             $lineItem = self::setLineItem($field, $lineItem, $optionValueId, $totalTax);
           }
           $totalPrice += $lineItem[$optionValueId]['line_total'] + CRM_Utils_Array::value('tax_amount', $lineItem[$optionValueId]);
-          if (
-            $component &&
-            isset($lineItem[$optionValueId]['auto_renew']) &&
-            is_numeric($lineItem[$optionValueId]['auto_renew'])
-          ) {
-            $autoRenew[$lineItem[$optionValueId]['auto_renew']] += $lineItem[$optionValueId]['line_total'];
-          }
           break;
 
         case 'CheckBox':
@@ -767,13 +744,6 @@ public static function processAmount($fields, &$params, &$lineItem, $component =
               $lineItem = self::setLineItem($field, $lineItem, $optionId, $totalTax);
             }
             $totalPrice += $lineItem[$optionId]['line_total'] + CRM_Utils_Array::value('tax_amount', $lineItem[$optionId]);
-            if (
-              $component &&
-              isset($lineItem[$optionId]['auto_renew']) &&
-              is_numeric($lineItem[$optionId]['auto_renew'])
-            ) {
-              $autoRenew[$lineItem[$optionId]['auto_renew']] += $lineItem[$optionId]['line_total'];
-            }
           }
           break;
       }
@@ -819,16 +789,6 @@ public static function processAmount($fields, &$params, &$lineItem, $component =
 
     $params['amount'] = $totalPrice;
     $params['tax_amount'] = $totalTax;
-    if ($component) {
-      foreach ($autoRenew as $dontCare => $eachAmount) {
-        if (!$eachAmount) {
-          unset($autoRenew[$dontCare]);
-        }
-      }
-      if (count($autoRenew) > 1) {
-        $params['autoRenew'] = $autoRenew;
-      }
-    }
   }
 
   /**