CRM-20276 update line item financial amount when updating line item.

We don't have to leave the e-notice in, but I suspect that line is never called & it would be good
to figure that out rather than leave it 'in case', hence the enotice

diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php
index 5e7dd3f2ff716d13d841b5e2e8a0a0659fb4132c..d12ced5518642b8c8c05916a63a8fbcedd1c016d 100644 (file)
--- a/CRM/Contribute/BAO/Contribution.php
+++ b/CRM/Contribute/BAO/Contribution.php
@@ -3439,41 +3439,38 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac
    * @param string $context
    *   Update scenarios.
    *
-   * @param null $skipTrxn
-   *
    */
-  public static function updateFinancialAccounts(&$params, $context = NULL, $skipTrxn = NULL) {
-    $itemAmount = $trxnID = NULL;
-    //get all the statuses
-    $contributionStatus = CRM_Contribute_PseudoConstant::contributionStatus(NULL, 'name');
+  public static function updateFinancialAccounts(&$params, $context = NULL) {
+    $trxnID = NULL;
+    $inputParams = $params;
+    $isARefund = FALSE;
+    $currentContributionStatus = CRM_Core_PseudoConstant::getLabel('CRM_Contribute_BAO_Contribution', 'contribution_status_id', $params['contribution']->contribution_status_id);
     $previousContributionStatus = CRM_Contribute_PseudoConstant::contributionStatus($params['prevContribution']->contribution_status_id, 'name');
+
     if (($previousContributionStatus == 'Pending'
         || $previousContributionStatus == 'In Progress')
-      && $params['contribution']->contribution_status_id == array_search('Completed', $contributionStatus)
+      && $currentContributionStatus == 'Completed'
       && $context == 'changePaymentInstrument'
     ) {
       return;
     }
     if ((($previousContributionStatus == 'Partially paid'
-      && $params['contribution']->contribution_status_id == array_search('Completed', $contributionStatus))
+      && $currentContributionStatus == 'Completed')
       || ($previousContributionStatus == 'Pending' && $params['prevContribution']->is_pay_later == TRUE
-      && $params['contribution']->contribution_status_id == array_search('Partially paid', $contributionStatus)))
+      && $currentContributionStatus == 'Partially paid'))
       && $context == 'changedStatus'
     ) {
       return;
     }
     if ($context == 'changedAmount' || $context == 'changeFinancialType') {
-      $itemAmount = $params['trxnParams']['total_amount'] = $params['trxnParams']['net_amount'] = $params['total_amount'] - $params['prevContribution']->total_amount;
+      $params['trxnParams']['total_amount'] = $params['trxnParams']['net_amount'] = ($params['total_amount'] - $params['prevContribution']->total_amount);
     }
     if ($context == 'changedStatus') {
-      //get all the statuses
-      $contributionStatus = CRM_Contribute_PseudoConstant::contributionStatus(NULL, 'name');
-      $cancelledTaxAmount = 0;
       if ($previousContributionStatus == 'Completed'
         && (self::isContributionStatusNegative($params['contribution']->contribution_status_id))
       ) {
+        $isARefund = TRUE;
         $params['trxnParams']['total_amount'] = -$params['total_amount'];
-        $cancelledTaxAmount = CRM_Utils_Array::value('tax_amount', $params, '0.00');
         if (empty($params['contribution']->creditnote_id) || $params['contribution']->creditnote_id == "null") {
           $creditNoteId = self::createCreditNoteId();
           CRM_Core_DAO::setFieldValue('CRM_Contribute_DAO_Contribution', $params['contribution']->id, 'creditnote_id', $creditNoteId);
@@ -3485,7 +3482,7 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac
         $financialTypeID = CRM_Utils_Array::value('financial_type_id', $params) ? $params['financial_type_id'] : $params['prevContribution']->financial_type_id;
         $arAccountId = CRM_Contribute_PseudoConstant::getRelationalFinancialAccount($financialTypeID, 'Accounts Receivable Account is');
 
-        if ($params['contribution']->contribution_status_id == array_search('Cancelled', $contributionStatus)) {
+        if ($currentContributionStatus == 'Cancelled') {
           $params['trxnParams']['to_financial_account_id'] = $arAccountId;
           $params['trxnParams']['total_amount'] = -$params['total_amount'];
           if (is_null($params['contribution']->creditnote_id) || $params['contribution']->creditnote_id == "null") {
@@ -3497,7 +3494,6 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac
           $params['trxnParams']['from_financial_account_id'] = $arAccountId;
         }
       }
-      $itemAmount = $params['trxnParams']['total_amount'] + $cancelledTaxAmount;
     }
     elseif ($context == 'changePaymentInstrument') {
       $params['trxnParams']['net_amount'] = $params['trxnParams']['total_amount'];
@@ -3521,7 +3517,7 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac
     if ($context == 'changedStatus') {
       if (($previousContributionStatus == 'Pending'
           || $previousContributionStatus == 'In Progress')
-        && ($params['contribution']->contribution_status_id == array_search('Completed', $contributionStatus))
+        && ($currentContributionStatus == 'Completed')
       ) {
         if (empty($params['line_item'])) {
           //CRM-15296
@@ -3583,25 +3579,12 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac
           if ($params['contribution']->currency) {
             $currency = $params['contribution']->currency;
           }
-          $diff = 1;
-          if ($context == 'changeFinancialType' || self::isContributionStatusNegative($params['contribution']->contribution_status_id)) {
-            $diff = -1;
-          }
-          if (!empty($params['is_quick_config'])) {
-            $amount = $itemAmount;
-            if (!$amount) {
-              $amount = $params['total_amount'];
-            }
-          }
-          else {
-            $amount = $diff * $lineItemDetails['line_total'];
-          }
 
           $itemParams = array(
             'transaction_date' => $receiveDate,
             'contact_id' => $params['prevContribution']->contact_id,
             'currency' => $currency,
-            'amount' => $amount,
+            'amount' => self::getFinancialItemAmountFromParams($inputParams, $context, $lineItemDetails, $isARefund),
             'description' => $prevFinancialItem['description'],
             'status_id' => $prevFinancialItem['status_id'],
             'financial_account_id' => $financialAccount,
@@ -3609,13 +3592,13 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac
             'entity_id' => $lineItemDetails['id'],
           );
           $financialItem = CRM_Financial_BAO_FinancialItem::create($itemParams, NULL, $trxnIds);
-          $params['line_item'][$fieldId][$fieldValueId]['deferred_line_total'] = $amount;
+          $params['line_item'][$fieldId][$fieldValueId]['deferred_line_total'] = $itemParams['amount'];
           $params['line_item'][$fieldId][$fieldValueId]['financial_item_id'] = $financialItem->id;
 
-          if ($lineItemDetails['tax_amount']) {
+          if ($lineItemDetails['tax_amount'] && $lineItemDetails['tax_amount'] !== 'null') {
             $invoiceSettings = Civi::settings()->get('contribution_invoice_settings');
             $taxTerm = CRM_Utils_Array::value('tax_term', $invoiceSettings);
-            $itemParams['amount'] = $diff * $lineItemDetails['tax_amount'];
+            $itemParams['amount'] = self::getMultiplier($params['contribution']->contribution_status_id, $context) * $lineItemDetails['tax_amount'];
             $itemParams['description'] = $taxTerm;
             if ($lineItemDetails['financial_type_id']) {
               $itemParams['financial_account_id'] = self::getFinancialAccountId($lineItemDetails['financial_type_id']);
@@ -5258,6 +5241,78 @@ LEFT JOIN  civicrm_contribution on (civicrm_contribution.contact_id = civicrm_co
     return self::getValues(array('id' => $contributionID), CRM_Core_DAO::$_nullArray, CRM_Core_DAO::$_nullArray);
   }
 
+  /**
+   * Get the amount for the financial item row.
+   *
+   * Helper function to start to break down recordFinancialTransactions for readability.
+   *
+   * The logic is more historical than .. logical. Paths other than the deprecated one are tested.
+   *
+   * Codewise, several somewhat disimmilar things have been squished into recordFinancialAccounts
+   * for historical reasons. Going forwards we can hope to add tests & improve readibility
+   * of that function
+   *
+   * @todo move recordFinancialAccounts & helper functions to their own class?
+   *
+   * @param array $params
+   *   Params as passed to contribution.create
+   *
+   * @param string $context
+   *   changeFinancialType| changedAmount
+   * @param array $lineItemDetails
+   *   Line items.
+   * @param bool $isARefund
+   *   Is this a refund / negative transaction.
+   *
+   * @return float
+   */
+  protected static function getFinancialItemAmountFromParams($params, $context, $lineItemDetails, $isARefund) {
+    if ($context == 'changedAmount' || $context == 'changeFinancialType') {
+      return $params['total_amount'] - $params['prevContribution']->total_amount;
+    }
+    elseif ($context == 'changedStatus') {
+      $cancelledTaxAmount = 0;
+      if ($isARefund) {
+        $cancelledTaxAmount = CRM_Utils_Array::value('tax_amount', $params, '0.00');
+      }
+      return self::getMultiplier($params['contribution']->contribution_status_id, $context) * ($params['trxnParams']['total_amount'] + $cancelledTaxAmount);
+    }
+    elseif ($context === NULL) {
+      // erm, yes because? but, hey, it's tested.
+      return $params['total_amount'];
+    }
+    elseif (empty($lineItemDetails['line_total'])) {
+      // follow legacy code path
+      Civi::log()
+        ->warning('Deprecated bit of code, please log a ticket explaining how you got here!', array('civi.tag' => 'deprecated'));
+      return $params['total_amount'];
+    }
+    else {
+      return self::getMultiplier($params['contribution']->contribution_status_id, $context) * $lineItemDetails['line_total'];
+    }
+  }
+
+  /**
+   * Get the multiplier for adjusting rows.
+   *
+   * If we are dealing with a refund or cancellation then it will be a negative
+   * amount to reflect the negative transaction.
+   *
+   * If we are changing Financial Type it will be a negative amount to
+   * adjust down the old type.
+   *
+   * @param int $contribution_status_id
+   * @param string $context
+   *
+   * @return int
+   */
+  protected static function getMultiplier($contribution_status_id, $context) {
+    if ($context == 'changeFinancialType' || self::isContributionStatusNegative($contribution_status_id)) {
+      return -1;
+    }
+    return 1;
+  }
+
   /**
    * Assign Test Value.
    *

diff --git a/tests/phpunit/CRM/Contribute/Form/ContributionTest.php b/tests/phpunit/CRM/Contribute/Form/ContributionTest.php
index 6f36045db57f5f3f5a29fabcbcecca6a2f013726..2f56ee12c246918fe3b4b36580fce29ae79ad358 100644 (file)
--- a/tests/phpunit/CRM/Contribute/Form/ContributionTest.php
+++ b/tests/phpunit/CRM/Contribute/Form/ContributionTest.php
@@ -965,9 +965,7 @@ Price Field - Price Field 1        1   $ 100.00      $ 100.00
 
     $this->assertEquals(100, $items['values'][0]['amount']);
     $this->assertEquals(10, $items['values'][1]['amount']);
-    // @todo what should the amount BE? I believe this is incorrect elsewhere too.
-    // currently it is $120 - ie the first one not incremented. This is consistent
-    // with my testing on CRM-19723
+    $this->assertEquals(200, $items['values'][2]['amount']);
     $this->assertEquals(20, $items['values'][3]['amount']);
   }