From 362f37fc946519771aa4ec3e5783c46aa5a1e40b Mon Sep 17 00:00:00 2001 From: eileen Date: Sat, 29 Jun 2019 11:46:26 +1200 Subject: [PATCH] Further work on payment.create consolidation - always handle financials from payment.create This fixes a 2-track processing in payment.create whereby financial transactions for most payments were being handled by payment.create but for payments transitioning from 'Pending' to 'Completed' the contribution bao was handling them (via completetransaction). I've added a new parameter to allow opting out of recording payments in completetransation (is_post_payment_create). I believe ideally our eventual position is that this would be the case whenever a payment is added. I exposed the parameter to getfields in the spec for completetransaction but not contribution.create. It kind of is an internal / transitional param but the eventual logic is that completetransaction would never handle financials whereas the eventual for contribution.create is more muddy. This surfaced what I believe is a bug in the getToFinancialAccount function. This is only called when adding payments so I believe it should never transfer TO accounts receivable (which it was doing). In practical terms this would only have been hit (prior to this) when adding a payment to a partially paid transaction --- CRM/Contribute/BAO/Contribution.php | 33 +++++++++++++++++------------ CRM/Financial/BAO/Payment.php | 12 +++++------ api/v3/Contribution.php | 11 +++++++++- 3 files changed, 35 insertions(+), 21 deletions(-) diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php index 5997ce9e87..838bc9768e 100644 --- a/CRM/Contribute/BAO/Contribution.php +++ b/CRM/Contribute/BAO/Contribution.php @@ -241,7 +241,16 @@ class CRM_Contribute_BAO_Contribution extends CRM_Contribute_DAO_Contribution { //add Account details $params['contribution'] = $contribution; - self::recordFinancialAccounts($params); + if (empty($params['is_post_payment_create'])) { + // If this is being called from the Payment.create api/ BAO then that Entity + // takes responsibility for the financial transactions. In fact calling Payment.create + // to add payments & having it call completetransaction and / or contribution.create + // to update related entities is the preferred flow. + // Note that leveraging this parameter for any other code flow is not supported and + // is likely to break in future and / or cause serious problems in your data. + // https://github.com/civicrm/civicrm-core/pull/14673 + self::recordFinancialAccounts($params); + } if (self::isUpdateToRecurringContribution($params)) { CRM_Contribute_BAO_ContributionRecur::updateOnNewPayment( @@ -946,19 +955,10 @@ class CRM_Contribute_BAO_Contribution extends CRM_Contribute_DAO_Contribution { * @return int */ public static function getToFinancialAccount($contribution, $params) { - $contributionStatuses = CRM_Contribute_PseudoConstant::contributionStatus(NULL, 'name'); - CRM_Contribute_PseudoConstant::contributionStatus(NULL, 'name'); - $pendingStatus = [ - array_search('Pending', $contributionStatuses), - array_search('In Progress', $contributionStatuses), - ]; - if (in_array(CRM_Utils_Array::value('contribution_status_id', $contribution), $pendingStatus)) { - return CRM_Contribute_PseudoConstant::getRelationalFinancialAccount($contribution['financial_type_id'], 'Accounts Receivable Account is'); - } - elseif (!empty($params['payment_processor'])) { + if (!empty($params['payment_processor'])) { return CRM_Contribute_PseudoConstant::getRelationalFinancialAccount($contribution['payment_processor'], NULL, 'civicrm_payment_processor'); } - elseif (!empty($params['payment_instrument_id'])) { + if (!empty($params['payment_instrument_id'])) { return CRM_Financial_BAO_FinancialTypeAccount::getInstrumentFinancialAccount($contribution['payment_instrument_id']); } else { @@ -4490,10 +4490,16 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac * @param CRM_Core_Transaction $transaction * @param int $recur * @param CRM_Contribute_BAO_Contribution $contribution + * @param bool $isPostPaymentCreate + * Is this being called from the payment.create api. If so the api has taken care of financial entities. + * Note that our goal is that this would only ever be called from payment.create and never handle financials (only + * transitioning related elements). * * @return array + * @throws \CRM_Core_Exception + * @throws \CiviCRM_API3_Exception */ - public static function completeOrder(&$input, &$ids, $objects, $transaction, $recur, $contribution) { + public static function completeOrder(&$input, &$ids, $objects, $transaction, $recur, $contribution, $isPostPaymentCreate = FALSE) { $primaryContributionID = isset($contribution->id) ? $contribution->id : $objects['first_contribution']->id; // The previous details are used when calculating line items so keep it before any code that 'does something' if (!empty($contribution->id)) { @@ -4613,6 +4619,7 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac } $contributionParams['id'] = $contribution->id; + $contributionParams['is_post_payment_create'] = $isPostPaymentCreate; // CRM-19309 - if you update the contribution here with financial_type_id it can/will mess with $lineItem // unsetting it here does NOT cause any other contribution test to fail! diff --git a/CRM/Financial/BAO/Payment.php b/CRM/Financial/BAO/Payment.php index d3f5dc3119..7b9ca33336 100644 --- a/CRM/Financial/BAO/Payment.php +++ b/CRM/Financial/BAO/Payment.php @@ -59,12 +59,7 @@ class CRM_Financial_BAO_Payment { $isPaymentCompletesContribution = self::isPaymentCompletesContribution($params['contribution_id'], $params['total_amount']); - // For legacy reasons Pending payments are completed through completetransaction. - // @todo completetransaction should transition components but financial transactions - // should be handled through Payment.create. - $isSkipRecordingPaymentHereForLegacyHandlingReasons = ($contributionStatus == 'Pending' && $isPaymentCompletesContribution); - - if (!$isSkipRecordingPaymentHereForLegacyHandlingReasons && $params['total_amount'] > 0) { + if ($params['total_amount'] > 0) { $balanceTrxnParams['to_financial_account_id'] = CRM_Contribute_BAO_Contribution::getToFinancialAccount($contribution, $params); $balanceTrxnParams['from_financial_account_id'] = CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship($contribution['financial_type_id'], 'Accounts Receivable Account is'); $balanceTrxnParams['total_amount'] = $params['total_amount']; @@ -148,7 +143,10 @@ class CRM_Financial_BAO_Payment { ); } else { - civicrm_api3('Contribution', 'completetransaction', ['id' => $contribution['id']]); + civicrm_api3('Contribution', 'completetransaction', [ + 'id' => $contribution['id'], + 'is_post_payment_create' => TRUE, + ]); // Get the trxn $trxnId = CRM_Core_BAO_FinancialTrxn::getFinancialTrxnId($contribution['id'], 'DESC'); $ftParams = ['id' => $trxnId['financialTrxnId']]; diff --git a/api/v3/Contribution.php b/api/v3/Contribution.php index f59951194c..2e4a027d6e 100644 --- a/api/v3/Contribution.php +++ b/api/v3/Contribution.php @@ -611,6 +611,15 @@ function _civicrm_api3_contribution_completetransaction_spec(&$params) { 'optionGroupName' => 'accept_creditcard', ], ]; + // At some point we will deprecate this api in favour of calling payment create which will in turn call this + // api if appropriate to transition related entities and send receipts - ie. financial responsibility should + // not exist in completetransaction. For now we just need to allow payment.create to force a bypass on the + // things it does itself. + $params['is_post_payment_create'] = [ + 'title' => 'Is this being called from payment create?', + 'type' => CRM_Utils_Type::T_BOOLEAN, + 'description' => 'The \'correct\' flow is to call payment.create for the financial side & for that to call completecontribution for the entity & receipt management. However, we need to still support completetransaction directly for legacy reasons', + ]; } /** @@ -727,7 +736,7 @@ function _ipn_process_transaction(&$params, $contribution, $input, $ids, $firstC $input['pan_truncation'] = CRM_Utils_Array::value('pan_truncation', $params); $transaction = new CRM_Core_Transaction(); return CRM_Contribute_BAO_Contribution::completeOrder($input, $ids, $objects, $transaction, - !empty($contribution->contribution_recur_id), $contribution); + !empty($contribution->contribution_recur_id), $contribution, CRM_Utils_Array::value('is_post_payment_create', $params)); } /** -- 2.25.1