From f0d18278b3e869197321f315fcc3f13177f10d63 Mon Sep 17 00:00:00 2001 From: eileen Date: Thu, 10 Oct 2019 12:31:49 +0200 Subject: [PATCH] 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 --- CRM/Contribute/BAO/Contribution.php | 19 ++++++++++--------- CRM/Contribute/Form/Task/Invoice.php | 1 + tests/phpunit/api/v3/ContributionTest.php | 8 ++++++++ 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php index 7259c633a0..11ce9e61cf 100644 --- 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 23cea476ed..3798d6dae1 100644 --- 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 a63b33bb9a..8641cf1819 100644 --- 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(); } /** -- 2.25.1