CRM-20750 follow up fix, reversal should be against original account.

On trying to investigate why a test change was required in the original PR I came to the following conclusions: 1) the test function was causing problems by being hard to read. As a result it was too easy to see it as 'accounts magic' - I started a cleanup here 2) Once I understood what was being tested & failing I concluded the change in the PR was in fact wrong as it was putting a negative entry against the new financial account rather than the original one.
civicrm · Sep 11, 2017 · b0e806f · b0e806f
1 parent 5ca657d
commit b0e806f
Show file tree

Hide file tree

Showing 2 changed files with 82 additions and 8 deletions.
diff --git a/CRM/Core/BAO/FinancialTrxn.php b/CRM/Core/BAO/FinancialTrxn.php
@@ -744,6 +744,7 @@ public static function updateFinancialAccountsOnPaymentInstrumentChange($inputPa
     $lastFinancialTrxnId = self::getFinancialTrxnId($prevContribution->id, 'DESC', FALSE, NULL, $deferredFinancialAccount);
 
     // there is no point to proceed as we can't find the last payment made
+    // @todo we should throw an exception here rather than return false.
     if (empty($lastFinancialTrxnId['financialTrxnId'])) {
       return FALSE;
     }
@@ -756,7 +757,6 @@ public static function updateFinancialAccountsOnPaymentInstrumentChange($inputPa
     $lastFinancialTrxn['total_amount'] = -$inputParams['trxnParams']['total_amount'];
     $lastFinancialTrxn['net_amount'] = -$inputParams['trxnParams']['net_amount'];
     $lastFinancialTrxn['fee_amount'] = -$inputParams['trxnParams']['fee_amount'];
-    $lastFinancialTrxn['to_financial_account_id'] = CRM_Financial_BAO_FinancialTypeAccount::getInstrumentFinancialAccount($currentContribution->payment_instrument_id);
     $lastFinancialTrxn['contribution_id'] = $prevContribution->id;
     foreach (array($lastFinancialTrxn, $inputParams['trxnParams']) as $financialTrxnParams) {
       $trxn = CRM_Core_BAO_FinancialTrxn::create($financialTrxnParams);

diff --git a/tests/phpunit/api/v3/ContributionTest.php b/tests/phpunit/api/v3/ContributionTest.php
@@ -1147,7 +1147,7 @@ public function testCreateUpdateContributionPaymentInstrument() {
     );
     $contribution = $this->callAPISuccess('contribution', 'create', $newParams);
     $this->assertAPISuccess($contribution);
-    $this->_checkFinancialTrxn($contribution, 'paymentInstrument', $instrumentId);
+    $this->checkFinancialTrxnPaymentInstrumentChange($contribution['id'], 4, $instrumentId);
 
     // cleanup - delete created payment instrument
     $this->_deletedAddedPaymentInstrument();
@@ -1175,7 +1175,7 @@ public function testCreateUpdateNegativeContributionPaymentInstrument() {
     );
     $contribution = $this->callAPISuccess('contribution', 'create', $newParams);
     $this->assertAPISuccess($contribution);
-    $this->_checkFinancialTrxn($contribution, 'paymentInstrument', $instrumentId, array('total_amount' => '-100.00'));
+    $this->checkFinancialTrxnPaymentInstrumentChange($contribution['id'], 4, $instrumentId, -100);
 
     // cleanup - delete created payment instrument
     $this->_deletedAddedPaymentInstrument();
@@ -3333,6 +3333,59 @@ public function _checkFinancialItem($contId, $context) {
     }
   }
 
+  /**
+   * Check correct financial transaction entries were created for the change in payment instrument.
+   *
+   * @param int $contributionID
+   * @param int $originalInstrumentID
+   * @param int $newInstrumentID
+   */
+  public function checkFinancialTrxnPaymentInstrumentChange($contributionID, $originalInstrumentID, $newInstrumentID, $amount = 100) {
+
+    $entityFinancialTrxns = $this->getFinancialTransactionsForContribution($contributionID);
+
+    $originalTrxnParams = array(
+      'to_financial_account_id' => CRM_Financial_BAO_FinancialTypeAccount::getInstrumentFinancialAccount($originalInstrumentID),
+      'payment_instrument_id' => $originalInstrumentID,
+      'amount' => $amount,
+      'status_id' => 1,
+    );
+
+    $reversalTrxnParams = array(
+      'to_financial_account_id' => CRM_Financial_BAO_FinancialTypeAccount::getInstrumentFinancialAccount($originalInstrumentID),
+      'payment_instrument_id' => $originalInstrumentID,
+      'amount' => -$amount,
+      'status_id' => 1,
+    );
+
+    $newTrxnParams = array(
+      'to_financial_account_id' => CRM_Financial_BAO_FinancialTypeAccount::getInstrumentFinancialAccount($newInstrumentID),
+      'payment_instrument_id' => $newInstrumentID,
+      'amount' => $amount,
+      'status_id' => 1,
+    );
+
+    foreach (array($originalTrxnParams, $reversalTrxnParams, $newTrxnParams) as $index => $transaction) {
+      $entityFinancialTrxn = $entityFinancialTrxns[$index];
+      $this->assertEquals($entityFinancialTrxn['amount'], $transaction['amount']);
+
+      $financialTrxn = $this->callAPISuccessGetSingle('FinancialTrxn', array(
+        'id' => $entityFinancialTrxn['financial_trxn_id'],
+      ));
+      $this->assertEquals($transaction['status_id'], $financialTrxn['status_id']);
+      $this->assertEquals($transaction['amount'], $financialTrxn['total_amount']);
+      $this->assertEquals($transaction['amount'], $financialTrxn['net_amount']);
+      $this->assertEquals(0, $financialTrxn['fee_amount']);
+      $this->assertEquals($transaction['payment_instrument_id'], $financialTrxn['payment_instrument_id']);
+      $this->assertEquals($transaction['to_financial_account_id'], $financialTrxn['to_financial_account_id']);
+
+      // Generic checks.
+      $this->assertEquals(1, $financialTrxn['is_payment']);
+      $this->assertEquals('USD', $financialTrxn['currency']);
+      $this->assertEquals(date('Y-m-d'), date('Y-m-d', strtotime($financialTrxn['trxn_date'])));
+    }
+  }
+
   /**
    * Check financial transaction.
    *
@@ -3344,11 +3397,9 @@ public function _checkFinancialItem($contId, $context) {
    * @param array $extraParams
    */
   public function _checkFinancialTrxn($contribution, $context, $instrumentId = NULL, $extraParams = array()) {
-    $trxnParams = array(
-      'entity_id' => $contribution['id'],
-      'entity_table' => 'civicrm_contribution',
-    );
-    $trxn = current(CRM_Financial_BAO_FinancialItem::retrieveEntityFinancialTrxn($trxnParams, TRUE));
+    $financialTrxns = $this->getFinancialTransactionsForContribution($contribution['id']);
+    $trxn = array_pop($financialTrxns);
+
     $params = array(
       'id' => $trxn['financial_trxn_id'],
     );
@@ -3375,6 +3426,10 @@ public function _checkFinancialTrxn($contribution, $context, $instrumentId = NUL
       );
     }
     elseif ($context == 'changeFinancial' || $context == 'paymentInstrument') {
+      // @todo checkFinancialTrxnPaymentInstrumentChange instead for paymentInstrument.
+      // It does the same thing with greater readability.
+      // @todo remove handling for
+
       $entityParams = array(
         'entity_id' => $contribution['id'],
         'entity_table' => 'civicrm_contribution',
@@ -3910,4 +3965,23 @@ public function testRepeatTransactionWithDifferenceCurrency() {
     $this->assertEquals('AUD', $contribution['values'][$contribution['id']]['currency']);
   }
 
+  /**
+   * Get the financial items for the contribution.
+   *
+   * @param int $contributionID
+   *
+   * @return array
+   *   Array of associated financial items.
+   */
+  protected function getFinancialTransactionsForContribution($contributionID) {
+    $trxnParams = array(
+      'entity_id' => $contributionID,
+      'entity_table' => 'civicrm_contribution',
+    );
+    // @todo the following function has naming errors & has a weird signature & appears to
+    // only be called from test classes. Move into test suite & maybe just use api
+    // from this function.
+    return array_merge(CRM_Financial_BAO_FinancialItem::retrieveEntityFinancialTrxn($trxnParams, FALSE, array()));
+  }
+
 }