Remove call to CRM_Contribute_BAO_Contribution::recordAdditionalPayme…

…nt in favour of payment create
civicrm · May 20, 2019 · db18668 · db18668
1 parent 0896a16
commit db18668
Show file tree

Hide file tree

Showing 4 changed files with 40 additions and 19 deletions.
diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php
@@ -3591,8 +3591,12 @@ public static function updateFinancialAccounts(&$params, $context = NULL) {
     ) {
       return;
     }
-    if ((($previousContributionStatus == 'Partially paid'
-      && $currentContributionStatus == 'Completed')
+    // The 'right' way to add payments or refunds is through the Payment.create api. That api
+    // then updates the contribution but this process shoud not also record another financial trxn.
+    if ((($previousContributionStatus == 'Partially paid' && $currentContributionStatus == 'Completed')
+      || ($previousContributionStatus == 'Pending refund' && $currentContributionStatus == 'Completed')
+      // This concept of pay_later as different to any other sort of pending is deprecated & it's unclear
+      // why it is here or where it is handled instead.
       || ($previousContributionStatus == 'Pending' && $params['prevContribution']->is_pay_later == TRUE
       && $currentContributionStatus == 'Partially paid'))
       && $context == 'changedStatus'

diff --git a/CRM/Financial/BAO/Payment.php b/CRM/Financial/BAO/Payment.php
@@ -51,6 +51,7 @@ class CRM_Financial_BAO_Payment {
    *
    * @throws \API_Exception
    * @throws \CRM_Core_Exception
+   * @throws \CiviCRM_API3_Exception
    */
   public static function create($params) {
     $contribution = civicrm_api3('Contribution', 'getsingle', ['id' => $params['contribution_id']]);
@@ -104,11 +105,24 @@ public static function create($params) {
     }
 
     if ($isPaymentCompletesContribution) {
-      civicrm_api3('Contribution', 'completetransaction', ['id' => $contribution['id']]);
-      // Get the trxn
-      $trxnId = CRM_Core_BAO_FinancialTrxn::getFinancialTrxnId($contribution['id'], 'DESC');
-      $ftParams = ['id' => $trxnId['financialTrxnId']];
-      $trxn = CRM_Core_BAO_FinancialTrxn::retrieve($ftParams, CRM_Core_DAO::$_nullArray);
+      if ($contributionStatus == 'Pending refund') {
+        // Ideally we could still call completetransaction as non-payment related actions should
+        // be outside this class. However, for now we just update the contribution here.
+        // Unit test cover in CRM_Event_BAO_AdditionalPaymentTest::testTransactionInfo.
+        civicrm_api3('Contribution', 'create',
+          [
+            'id' => $contribution['id'],
+            'contribution_status_id' => 'Completed',
+          ]
+        );
+      }
+      else {
+        civicrm_api3('Contribution', 'completetransaction', ['id' => $contribution['id']]);
+        // Get the trxn
+        $trxnId = CRM_Core_BAO_FinancialTrxn::getFinancialTrxnId($contribution['id'], 'DESC');
+        $ftParams = ['id' => $trxnId['financialTrxnId']];
+        $trxn = CRM_Core_BAO_FinancialTrxn::retrieve($ftParams, CRM_Core_DAO::$_nullArray);
+      }
     }
     elseif ($contributionStatus === 'Pending') {
       civicrm_api3('Contribution', 'create',

diff --git a/tests/phpunit/CRM/Event/BAO/AdditionalPaymentTest.php b/tests/phpunit/CRM/Event/BAO/AdditionalPaymentTest.php
@@ -214,20 +214,23 @@ public function testTransactionInfo() {
     $result = $this->addParticipantWithPayment($feeAmt, $amtPaid);
     $contributionID = $result['contribution']['id'];
 
-    //Complete the partial payment.
-    $submittedValues = array(
+    $this->callAPISuccess('Payment', 'create', [
+      'contribution_id' => $contributionID,
       'total_amount' => 20,
       'payment_instrument_id' => 3,
-    );
-    CRM_Contribute_BAO_Contribution::recordAdditionalPayment($contributionID, $submittedValues, 'owed', $result['participant']['id']);
+      'participant_id' => $result['participant']['id'],
+    ]);
 
     //Change selection to a lower amount.
     $params['price_2'] = 50;
     CRM_Price_BAO_LineItem::changeFeeSelections($params, $result['participant']['id'], 'participant', $contributionID, $result['feeBlock'], $result['lineItem']);
 
-    //Record a refund of the remaining amount.
-    $submittedValues['total_amount'] = 50;
-    CRM_Contribute_BAO_Contribution::recordAdditionalPayment($contributionID, $submittedValues, 'refund', $result['participant']['id']);
+    $this->callAPISuccess('Payment', 'create',[
+      'total_amount' => -50,
+      'contribution_id' => $contributionID,
+      'participant_id' => $result['participant']['id'],
+      'payment_instrument_id' => 3,
+    ]);
     $paymentInfo = CRM_Contribute_BAO_Contribution::getPaymentInfo($result['participant']['id'], 'event', TRUE);
     $transaction = $paymentInfo['transaction'];
 

diff --git a/tests/phpunit/CRM/Event/BAO/ChangeFeeSelectionTest.php b/tests/phpunit/CRM/Event/BAO/ChangeFeeSelectionTest.php
@@ -308,12 +308,12 @@ public function testCRM20611() {
     $contributionBalance = ($this->_cheapFee - $actualPaidAmount);
     $this->assertEquals($contributionBalance, CRM_Contribute_BAO_Contribution::getContributionBalance($this->_contributionId));
 
-    //Complete the refund payment.
-    $submittedValues = array(
-      'total_amount' => 120,
+    $this->callAPISuccess('Payment', 'create', [
+      'contribution_id' => $this->_contributionId,
+      'total_amount' => -120,
       'payment_instrument_id' => 3,
-    );
-    CRM_Contribute_BAO_Contribution::recordAdditionalPayment($this->_contributionId, $submittedValues, 'refund', $this->_participantId);
+      'participant_id' => $this->_participantId,
+    ]);
     $contributionBalance += 120;
     $this->assertEquals($contributionBalance, CRM_Contribute_BAO_Contribution::getContributionBalance($this->_contributionId));