Further work on payment.create consolidation - always handle financials from payment...

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

diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php
index 5997ce9e87f19a6aac378160bd1ce3f9cbf46ae7..838bc9768e851f9236061c15b848ac23df4b3e07 100644 (file)
--- 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 d3f5dc3119d0a214ad919fd0b8c0abd76197e433..7b9ca3333634feeaa174e2554a94c36a1a29ae4c 100644 (file)
--- 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 f59951194cb7a278f41774fd31ae6c2a19594cc6..2e4a027d6e07d1556469c5e7695bb0fb86cf033c 100644 (file)
--- 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));
 }
 
 /**