From 901094ebcdaf355f254159721f30cc71f2cf418e Mon Sep 17 00:00:00 2001
From: eileen <emcnaughton@wikimedia.org>
Date: Tue, 29 Oct 2019 00:48:26 +1300
Subject: [PATCH] Move calls to CRM_Core_BAO_FinancialTrxn::createDeferredTrxn
 back to the calling functions.

Although this seems like we are increasing duplication - by having more calls to one line of code I feel like
we are better off moving it back to the calling functions as there is 'preparatory wrangling' that also belongs
in the calling functions - notably in changeFinancialType.

I believe we should be able to get to the point where the updateFinancialAccounts funcgtion does not
need to receive context as a parameter because all the context specific stuff will be done in the calling functions
and it will have a clear objective - which I have finally realised is creating the financial transaction
and the financial items - however in some cases it is called twice to do a reversal followed by a line item
and I believe the calling functions could be cleaned up a lot more by 'returning' code to them as
passing  by reference leaves us very unclear as to what is happening to them at each point in the process.
---
 CRM/Contribute/BAO/Contribution.php | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php
index 8aafcee8eea..3ddc2f17cfa 100644
--- a/CRM/Contribute/BAO/Contribution.php
+++ b/CRM/Contribute/BAO/Contribution.php
@@ -3669,10 +3669,18 @@ public static function recordFinancialAccounts(&$params, $financialTrxnValues =
               }
             }
             self::updateFinancialAccounts($params, 'changeFinancialType');
+            $params['skipLineItem'] = FALSE;
+            foreach ($params['line_item'] as &$lineItems) {
+              foreach ($lineItems as &$line) {
+                $line['financial_type_id'] = $params['financial_type_id'];
+              }
+            }
+            CRM_Core_BAO_FinancialTrxn::createDeferredTrxn(CRM_Utils_Array::value('line_item', $params), $params['contribution'], TRUE, 'changeFinancialType');
             /* $params['trxnParams']['to_financial_account_id'] = $trxnParams['to_financial_account_id']; */
             $params['financial_account_id'] = $newFinancialAccount;
             $params['total_amount'] = $params['trxnParams']['total_amount'] = $params['trxnParams']['net_amount'] = $trxnParams['total_amount'];
             self::updateFinancialAccounts($params);
+            CRM_Core_BAO_FinancialTrxn::createDeferredTrxn(CRM_Utils_Array::value('line_item', $params), $params['contribution'], TRUE);
             $params['trxnParams']['to_financial_account_id'] = $trxnParams['to_financial_account_id'];
             $updated = TRUE;
             $params['deferred_financial_account_id'] = $newFinancialAccount;
@@ -3694,6 +3702,7 @@ public static function recordFinancialAccounts(&$params, $financialTrxnValues =
           $callUpdateFinancialAccounts = self::updateFinancialAccountsOnContributionStatusChange($params);
           if ($callUpdateFinancialAccounts) {
             self::updateFinancialAccounts($params, 'changedStatus');
+            CRM_Core_BAO_FinancialTrxn::createDeferredTrxn(CRM_Utils_Array::value('line_item', $params), $params['contribution'], TRUE, 'changedStatus');
           }
           $updated = TRUE;
         }
@@ -3722,6 +3731,7 @@ public static function recordFinancialAccounts(&$params, $financialTrxnValues =
           //Update Financial Records
           $params['trxnParams']['from_financial_account_id'] = NULL;
           self::updateFinancialAccounts($params, 'changedAmount');
+          CRM_Core_BAO_FinancialTrxn::createDeferredTrxn(CRM_Utils_Array::value('line_item', $params), $params['contribution'], TRUE, 'changedAmount');
           $updated = TRUE;
         }
 
@@ -3826,18 +3836,6 @@ public static function updateFinancialAccounts(&$params, $context = NULL) {
     foreach ($params['line_item'] as $fieldId => $fields) {
       $params = self::createFinancialItemsForLine($params, $context, $fields, $previousLineItems, $inputParams, $isARefund, $trxnIds, $fieldId);
     }
-
-    if ($context == 'changeFinancialType') {
-      // @todo we should stop passing $params by reference - splitting this out would be a step towards that.
-      $params['skipLineItem'] = FALSE;
-      foreach ($params['line_item'] as &$lineItems) {
-        foreach ($lineItems as &$line) {
-          $line['financial_type_id'] = $params['financial_type_id'];
-        }
-      }
-    }
-
-    CRM_Core_BAO_FinancialTrxn::createDeferredTrxn(CRM_Utils_Array::value('line_item', $params), $params['contribution'], TRUE, $context);
   }
 
   /**