Move code to assign tax information into shared parent

CRM/Contribute/Form/Contribution/Confirm.php

@@ -412,7 +412,7 @@ public function preProcess() {
             }
 
             $isPrimary = 1;
-            if (isset ($this->_params['onbehalf_location'][$blockName])
+            if (isset($this->_params['onbehalf_location'][$blockName])
               && count($this->_params['onbehalf_location'][$blockName]) > 0
             ) {
               $isPrimary = 0;
@@ -511,15 +511,20 @@ public function buildQuickForm() {
     // Make a copy of line items array to use for display only
     $tplLineItems = $this->_lineItem;
     if (CRM_Invoicing_Utils::isInvoicingEnabled()) {
-      list($getTaxDetails, $tplLineItems) = $this->alterLineItemsForTemplate($tplLineItems);
-      $this->assign('getTaxDetails', $getTaxDetails);
-      $this->assign('taxTerm', CRM_Invoicing_Utils::getTaxTerm());
+      // @todo $params seems like exactly the wrong place to get totalTaxAmount from
+      // this is a calculated variable so we it should be transparent how we
+      // calculated it rather than coming from 'params'
       $this->assign('totalTaxAmount', $params['tax_amount']);
     }
-    if ($this->_priceSetId && !CRM_Core_DAO::getFieldValue('CRM_Price_DAO_PriceSet', $this->_priceSetId, 'is_quick_config')) {
-      $this->assign('lineItem', $tplLineItems);
-    }
-    else {
+    $this->assignLineItemsToTemplate($tplLineItems);
+
+    $isDisplayLineItems = $this->_priceSetId && !CRM_Core_DAO::getFieldValue('CRM_Price_DAO_PriceSet', $this->_priceSetId, 'is_quick_config');
+
+    $this->assign('isDisplayLineItems', $isDisplayLineItems);
+
+    if (!$isDisplayLineItems) {
+      // quickConfig is deprecated in favour of isDisplayLineItems. Lots of logic has been harnessed to quick config
+      // whereas isDisplayLineItems is specific & clear.
       $this->assign('is_quick_config', 1);
       $this->_params['is_quick_config'] = 1;
     }
@@ -1811,8 +1816,7 @@ protected function getIsPending() {
     // The concept of contributeMode is deprecated.
     // the is_monetary concept probably should be too as it can be calculated from
     // the existence of 'amount' & seems fragile.
-    if (((isset($this->_contributeMode)) || !empty
-        ($this->_params['is_pay_later'])
+    if (((isset($this->_contributeMode)) || !empty($this->_params['is_pay_later'])
       ) &&
       (($this->_values['is_monetary'] && $this->_amount > 0.0))
     ) {

CRM/Financial/Form/FrontEndPaymentFormTrait.php

@@ -43,25 +43,40 @@ trait CRM_Financial_Form_FrontEndPaymentFormTrait {
    * CRM_Invoicing_Utils class with a potential end goal of moving this handling to an extension.
    *
    * @param $tplLineItems
-   *
-   * @return array
    */
-  protected function alterLineItemsForTemplate($tplLineItems) {
-    $getTaxDetails = FALSE;
+  protected function alterLineItemsForTemplate(&$tplLineItems) {
+    if (!CRM_Invoicing_Utils::isInvoicingEnabled()) {
+      return;
+    }
+    // @todo this should really be the first time we are determining
+    // the tax rates - we can calculate them from the financial_type_id
+    // & amount here so we didn't need a deeper function to semi-get
+    // them but not be able to 'format them right' because they are
+    // potentially being used for 'something else'.
+    // @todo invoicing code - please feel the hate. Also move this 'hook-like-bit'
+    // to the CRM_Invoicing_Utils class.
     foreach ($tplLineItems as $key => $value) {
       foreach ($value as $k => $v) {
         if (isset($v['tax_rate']) && $v['tax_rate'] != '') {
-          $getTaxDetails = TRUE;
+          // These only need assigning once, but code is more readable with them here
+          $this->assign('getTaxDetails', TRUE);
+          $this->assign('taxTerm', CRM_Invoicing_Utils::getTaxTerm());
           // Cast to float to display without trailing zero decimals
           $tplLineItems[$key][$k]['tax_rate'] = (float) $v['tax_rate'];
         }
       }
     }
-    // @todo fix this to only return $tplLineItems. Calling function can check for tax rate and
-    // do all invoicing related assigns
-    // another discrete function (it's just one more iteration through a an array with only a handful of
-    // lines so the separation of concerns is more important than 'efficiency'
-    return [$getTaxDetails, $tplLineItems];
+  }
+
+  /**
+   * Assign line items to the template.
+   *
+   * @param $tplLineItems
+   */
+  protected function assignLineItemsToTemplate($tplLineItems) {
+    // @todo this should be a hook that invoicing code hooks into rather than a call to it.
+    $this->alterLineItemsForTemplate($tplLineItems);
+    $this->assign('lineItem', $tplLineItems);
   }
 
 }

templates/CRM/Contribute/Form/Contribution/Confirm.tpl

@@ -44,16 +44,16 @@
 
   {include file="CRM/Contribute/Form/Contribution/MembershipBlock.tpl" context="confirmContribution"}
 
-  {if $amount GTE 0 OR $minimum_fee GTE 0 OR ( $priceSetID and $lineItem ) }
+  {if $amount GTE 0 OR $minimum_fee GTE 0 OR ( $isDisplayLineItems and $lineItem ) }
     <div class="crm-group amount_display-group">
       {if !$useForMember}
         <div class="header-dark">
-          {if !$membershipBlock AND $amount OR ( $priceSetID and $lineItem ) }{ts}Contribution Amount{/ts}{else}{ts}Membership Fee{/ts} {/if}
+          {if !$membershipBlock AND $amount OR ( $isDisplayLineItems and $lineItem ) }{ts}Contribution Amount{/ts}{else}{ts}Membership Fee{/ts} {/if}
         </div>
       {/if}
       <div class="display-block">
         {if !$useForMember}
-          {if $lineItem and $priceSetID}
+          {if $lineItem and $isDisplayLineItems}
             {if !$amount}{assign var="amount" value=0}{/if}
             {assign var="totalAmount" value=$amount}
             {include file="CRM/Price/Page/LineItem.tpl" context="Contribution"}