Deprecate calls to createCreditNoteId

createCreditNoteId is called from BAO_Contribution::add whenever the contribution
status is 'Cancelled' or 'Refunded.

A few lines later recordFinancialAccounts is called, so by the time recordFinancialAccounts is called
it is already set & the empty check will prevent these lines being hit.

All 4 places where updateFinancialAccounts are called in the
code are from recordFinancialAccounts.

Record financial accounts is called from Contribution Create & Payment create.

contribution.create does not rely on these lines to set the creditnote_id, as discussed.

In the BAO_Payment there are 2 scenarios
1) payment is positive
2) payment is negative - in this case recordRefundPayment is called.

It's logically OK for the code to  NOT to create a credit note when being called from payment.create
 because payment.create is NOT cancelling the contribution or refunding it and the funtion NEVER changes
 the contribution status to Refunded, Cancelled, Chargeback (those are 'business' statuses).

Payment.create updates contribution status to reflect a payment has been made - eg change from
Pending to partially paid or various payment-related-statuses to 'Completed'.

ie Payment.create is
- only adding payments (or refunds) to an order
- and the decision as to whether something is being credited back & warrants a credit note id or
whether the payment is being returned due to overpayment or other payment related concerns does
not belong in the payment code.

Payment create never changes contribution
status to Refunded, Cancelled or ChargeBack so the lines would never be hit from payment.create

This leaves us with the conclusion the lines are never hit. For safety/sanity we can deprectate them rather
than remove them by now. That way if the above analysis is wrong a test will fail.

Less likely (given financial test coverage) the deprecation notice will show up in the wild in the next few months.

(It's rather unclear how it DOES fit in since it feels like creditnote_id only applies if the entire
contribution is being refunded & really should be generated at the point at which, for example, the event
fees associated with it are changed - however, complaints to date about creditnote_id have all focussed
on performance issues, not logic issues

diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php
index 7259c633a016a279443fbdac99076c35875a882e..11ce9e61cf8b3b9b4530d5a7ad6cb123782c17ac 100644 (file)
--- a/CRM/Contribute/BAO/Contribution.php
+++ b/CRM/Contribute/BAO/Contribution.php
@@ -150,17 +150,14 @@ class CRM_Contribute_BAO_Contribution extends CRM_Contribute_DAO_Contribution {
 
     $contributionStatus = CRM_Contribute_PseudoConstant::contributionStatus(NULL, 'name');
     //if contribution is created with cancelled or refunded status, add credit note id
-    if (!empty($params['contribution_status_id'])) {
-      // @todo - should we include Chargeback? If so use self::isContributionStatusNegative($params['contribution_status_id'])
-      if (($params['contribution_status_id'] == array_search('Refunded', $contributionStatus)
-        || $params['contribution_status_id'] == array_search('Cancelled', $contributionStatus))
-      ) {
-        if (empty($params['creditnote_id'])) {
-          $params['creditnote_id'] = self::createCreditNoteId();
-        }
+    // do the same for chargeback - this entered the code 'accidentally' but moving it to here
+    // as part of cleanup maintains consistency.
+    if (self::isContributionStatusNegative(CRM_Utils_Array::value('contribution_status_id', $params))) {
+      if (empty($params['creditnote_id'])) {
+        $params['creditnote_id'] = self::createCreditNoteId();
       }
     }
-    else {
+    if (empty($params['contribution_status_id'])) {
       // Since the fee amount is expecting this (later on) ensure it is always set.
       // It would only not be set for an update where it is unchanged.
       $params['contribution_status_id'] = civicrm_api3('Contribution', 'getvalue', [
@@ -1126,6 +1123,8 @@ class CRM_Contribute_BAO_Contribution extends CRM_Contribute_DAO_Contribution {
       // @todo we should stop passing $params by reference - splitting this out would be a step towards that.
       $params['trxnParams']['total_amount'] = -$params['total_amount'];
       if (empty($params['contribution']->creditnote_id)) {
+        // This is always set in the Contribution::create function.
+        CRM_Core_Error::deprecatedFunctionWarning('Logic says this line is never reached & can be removed');
         $creditNoteId = self::createCreditNoteId();
         CRM_Core_DAO::setFieldValue('CRM_Contribute_DAO_Contribution', $params['contribution']->id, 'creditnote_id', $creditNoteId);
       }
@@ -1141,6 +1140,8 @@ class CRM_Contribute_BAO_Contribution extends CRM_Contribute_DAO_Contribution {
         $params['trxnParams']['to_financial_account_id'] = $arAccountId;
         $params['trxnParams']['total_amount'] = -$params['total_amount'];
         if (empty($params['contribution']->creditnote_id)) {
+          // This is always set in the Contribution::create function.
+          CRM_Core_Error::deprecatedFunctionWarning('Logic says this line is never reached & can be removed');
           $creditNoteId = self::createCreditNoteId();
           CRM_Core_DAO::setFieldValue('CRM_Contribute_DAO_Contribution', $params['contribution']->id, 'creditnote_id', $creditNoteId);
         }

diff --git a/CRM/Contribute/Form/Task/Invoice.php b/CRM/Contribute/Form/Task/Invoice.php
index 23cea476ede62e9a501765bbbae9561dc8a93cf2..3798d6dae178c51680dd9bd3372841678b40c4ac 100644 (file)
--- a/CRM/Contribute/Form/Task/Invoice.php
+++ b/CRM/Contribute/Form/Task/Invoice.php
@@ -285,6 +285,7 @@ class CRM_Contribute_Form_Task_Invoice extends CRM_Contribute_Form_Task {
 
       if ($contribution->contribution_status_id == $refundedStatusId || $contribution->contribution_status_id == $cancelledStatusId) {
         if (is_null($contribution->creditnote_id)) {
+          CRM_Core_Error::deprecatedFunctionWarning('This it the wrong place to add a credit note id since the id is added when the status is changed in the Contribution::Create function- hopefully it is never hit');
           $creditNoteId = CRM_Contribute_BAO_Contribution::createCreditNoteId();
           CRM_Core_DAO::setFieldValue('CRM_Contribute_DAO_Contribution', $contribution->id, 'creditnote_id', $creditNoteId);
         }

diff --git a/tests/phpunit/api/v3/ContributionTest.php b/tests/phpunit/api/v3/ContributionTest.php
index a63b33bb9a45ba4a4da491b61d2aea4bfe60fdf5..8641cf181901cb1bca6bb770ad2dad980fb4396e 100644 (file)
--- a/tests/phpunit/api/v3/ContributionTest.php
+++ b/tests/phpunit/api/v3/ContributionTest.php
@@ -1328,6 +1328,8 @@ class api_v3_ContributionTest extends CiviUnitTestCase {
    * CRM-17951 the contra account is a financial account with a relationship to a
    * financial type. It is not always configured but should be reflected
    * in the financial_trxn & financial_item table if it is.
+   *
+   * @throws \CRM_Core_Exception
    */
   public function testCreateUpdateChargebackContributionDefaultAccount() {
     $contribution = $this->callAPISuccess('Contribution', 'create', $this->_params);
@@ -1552,6 +1554,10 @@ class api_v3_ContributionTest extends CiviUnitTestCase {
    * Function tests that financial records are added when Pending Contribution is Canceled.
    */
   public function testCreateUpdateContributionCancelPending() {
+    // Enable & disable invoicing just to standardise the credit note id setting.
+    // Longer term we want to separate that setting from 'taxAndInvoicing'.
+    // and / or remove from core.
+    $this->enableTaxAndInvoicing();
     $contribParams = [
       'contact_id' => $this->_individualId,
       'receive_date' => '2012-01-01',
@@ -1575,6 +1581,8 @@ class api_v3_ContributionTest extends CiviUnitTestCase {
     $contribution = $this->callAPISuccess('contribution', 'create', $newParams);
     $this->_checkFinancialTrxn($contribution, 'cancelPending', NULL, $checkTrxnDate);
     $this->_checkFinancialItem($contribution['id'], 'cancelPending');
+    $this->assertEquals('CN_1', $contribution['values'][$contribution['id']]['creditnote_id']);
+    $this->disableTaxAndInvoicing();
   }
 
   /**