Fix Payment.create bug whereby payment_processor_id is not being used for the to_acco...

When Payment.create is used to add a payment made by a payment processor then the processor id should be passed in as payment_processor_id

The to_financial_account_id should be the one configured by the processor. The code was eroneously looking in 2 invalid places

- an unused parameter (which I deprecated & moved out to be handled in the api layer which is more 'throw away'
- the contribution - which doesn't have a payment_processor_id field & which should not be the source.

I originally thought this was a regression and did a patch against the rc
https://github.com/civicrm/civicrm-core/pull/15574

I think there is still a case to be made for putting this against the rc as it might be a regression - I just felt I
needed to slow down & sort out a test first.

Note that I have left a couple more things out of scope
1) I believe that if payment_processor_id is passed in then should use the payment_instrument_id from the processor
and not permit it to be overridden by the payment_instrument_id parameter (as is currently the case)
2) I believe that we should deprecate calling Payment.create without one of payment_instrument_id or payment_processor_id
(we might need some sugar to carry these over if chaining from Order.create
3) I believe the payment_instrument_id on the Order should be updated when the first payment comes in to
reflect the actual payment instrument, once known (of course it will still be inaccurate when there
are multiple payments of different types but at least for single payments this gets us past the issue
of it being outright wrong.

diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php
index 8aafcee8eea5de98c5d5148e556ba3060c7381a4..3f610b9d0a76b7f10cd801bbae4960eb000d1d01 100644 (file)
--- a/CRM/Contribute/BAO/Contribution.php
+++ b/CRM/Contribute/BAO/Contribution.php
@@ -952,8 +952,8 @@ class CRM_Contribute_BAO_Contribution extends CRM_Contribute_DAO_Contribution {
    * @return int
    */
   public static function getToFinancialAccount($contribution, $params) {
-    if (!empty($params['payment_processor'])) {
-      return CRM_Contribute_PseudoConstant::getRelationalFinancialAccount($contribution['payment_processor'], NULL, 'civicrm_payment_processor');
+    if (!empty($params['payment_processor_id'])) {
+      return CRM_Contribute_PseudoConstant::getRelationalFinancialAccount($params['payment_processor_id'], NULL, 'civicrm_payment_processor');
     }
     if (!empty($params['payment_instrument_id'])) {
       return CRM_Financial_BAO_FinancialTypeAccount::getInstrumentFinancialAccount($contribution['payment_instrument_id']);

diff --git a/CRM/Financial/BAO/Payment.php b/CRM/Financial/BAO/Payment.php
index 1a9676379843904c2a557d4d666508111a2f6fbb..d6051d3bf283a46f468cf3bf07b4c05a5dfce298 100644 (file)
--- a/CRM/Financial/BAO/Payment.php
+++ b/CRM/Financial/BAO/Payment.php
@@ -62,12 +62,7 @@ class CRM_Financial_BAO_Payment {
     $whiteList = ['check_number', 'payment_processor_id', 'fee_amount', 'total_amount', 'contribution_id', 'net_amount', 'card_type_id', 'pan_truncation', 'trxn_result_code', 'payment_instrument_id', 'trxn_id'];
     $paymentTrxnParams = array_intersect_key($params, array_fill_keys($whiteList, 1));
     $paymentTrxnParams['is_payment'] = 1;
-    if (!empty($params['payment_processor'])) {
-      // I can't find evidence this is passed in - I was gonna just remove it but decided to deprecate  as I see getToFinancialAccount
-      // also anticipates it.
-      CRM_Core_Error::deprecatedFunctionWarning('passing payment_processor is deprecated - use payment_processor_id');
-      $paymentTrxnParams['payment_processor_id'] = $params['payment_processor'];
-    }
+
     if (isset($paymentTrxnParams['payment_processor_id']) && empty($paymentTrxnParams['payment_processor_id'])) {
       // Don't pass 0 - ie the Pay Later processor as it is  a pseudo-processor.
       unset($paymentTrxnParams['payment_processor_id']);

diff --git a/CRM/Financial/BAO/PaymentProcessor.php b/CRM/Financial/BAO/PaymentProcessor.php
index 27f563d5f1cdb271781ec1e2f5a54130e884b419..a0de2a420260daecb3bd986e1fa6b7f39a1c3cce 100644 (file)
--- a/CRM/Financial/BAO/PaymentProcessor.php
+++ b/CRM/Financial/BAO/PaymentProcessor.php
@@ -120,6 +120,26 @@ class CRM_Financial_BAO_PaymentProcessor extends CRM_Financial_DAO_PaymentProces
     return [];
   }
 
+  /**
+   * Get options for a given contribution field.
+   *
+   * @param string $fieldName
+   * @param string $context see CRM_Core_DAO::buildOptionsContext.
+   * @param array $props whatever is known about this dao object.
+   *
+   * @return array|bool
+   * @see CRM_Core_DAO::buildOptions
+   *
+   */
+  public static function buildOptions($fieldName, $context = NULL, $props = []) {
+    $params = [];
+    if ($fieldName === 'financial_account_id') {
+      // Pseudo-field - let's help out.
+      return CRM_Core_BAO_FinancialTrxn::buildOptions('to_financial_account_id', $context, $props);
+    }
+    return CRM_Core_PseudoConstant::get(__CLASS__, $fieldName, $params, $context);
+  }
+
   /**
    * Retrieve DB object based on input parameters.
    *

diff --git a/api/v3/Payment.php b/api/v3/Payment.php
index 8198d1e9c18f9107ddb74de117e408e8c45854cb..3460f95c6d145b2a4b6917b7cf645a0adfe79439 100644 (file)
--- a/api/v3/Payment.php
+++ b/api/v3/Payment.php
@@ -146,6 +146,12 @@ function civicrm_api3_payment_create($params) {
       }
     }
   }
+  if (!empty($params['payment_processor'])) {
+    // I can't find evidence this is passed in - I was gonna just remove it but decided to deprecate  as I see getToFinancialAccount
+    // also anticipates it.
+    CRM_Core_Error::deprecatedFunctionWarning('passing payment_processor is deprecated - use payment_processor_id');
+    $params['payment_processor_id'] = $params['payment_processor'];
+  }
   // Check if it is an update
   if (!empty($params['id'])) {
     $amount = $params['total_amount'];

diff --git a/api/v3/PaymentProcessor.php b/api/v3/PaymentProcessor.php
index 435fd97e977ea7779ec1167afe0c86e509f1fb6a..96c18f608aa9493a1e2c4fd1c39c07f3afeb041d 100644 (file)
--- a/api/v3/PaymentProcessor.php
+++ b/api/v3/PaymentProcessor.php
@@ -65,7 +65,13 @@ function _civicrm_api3_payment_processor_create_spec(&$params) {
   $params['domain_id']['api.default'] = CRM_Core_Config::domainID();
   $params['financial_account_id']['api.default'] = CRM_Financial_BAO_PaymentProcessor::getDefaultFinancialAccountID();
   $params['financial_account_id']['api.required'] = TRUE;
+  $params['financial_account_id']['type'] = CRM_Utils_Type::T_INT;
   $params['financial_account_id']['title'] = ts('Financial Account for Processor');
+  $params['financial_account_id']['pseudoconstant'] = [
+    'table' => 'civicrm_financial_account',
+    'keyColumn' => 'id',
+    'labelColumn' => 'name',
+  ];
 }
 
 /**

diff --git a/tests/phpunit/api/v3/PaymentTest.php b/tests/phpunit/api/v3/PaymentTest.php
index 1f6f3e2215cf3aa30405b925d6ae7e49bde1c74d..6cfc2dec20d9abe9ec378c2eb1c5283e50a8486a 100644 (file)
--- a/tests/phpunit/api/v3/PaymentTest.php
+++ b/tests/phpunit/api/v3/PaymentTest.php
@@ -761,6 +761,8 @@ class api_v3_PaymentTest extends CiviUnitTestCase {
 
   /**
    * Test create payment api for pay later contribution with partial payment.
+   *
+   * @throws \CRM_Core_Exception
    */
   public function testCreatePaymentPayLaterPartialPayment() {
     $this->createLoggedInUser();
@@ -772,7 +774,7 @@ class api_v3_PaymentTest extends CiviUnitTestCase {
       'contribution_status_id' => 2,
       'is_pay_later' => 1,
     ];
-    $contribution = $this->callAPISuccess('Contribution', 'create', $contributionParams);
+    $contribution = $this->callAPISuccess('Order', 'create', $contributionParams);
     //Create partial payment
     $params = [
       'contribution_id' => $contribution['id'],
@@ -847,10 +849,42 @@ class api_v3_PaymentTest extends CiviUnitTestCase {
     $this->callAPISuccessGetCount('Activity', ['target_contact_id' => $this->_individualId, 'activity_type_id' => 'Payment'], 2);
   }
 
+  /**
+   * Test that Payment.create uses the to_account of the payment processor.
+   *
+   * @throws \CiviCRM_API3_Exception
+   * @throws \CRM_Core_Exception
+   */
+  public function testPaymentWithProcessorWithOddFinancialAccount() {
+    $processor = $this->dummyProcessorCreate(['financial_account_id' => 'Deposit Bank Account', 'payment_instrument_id' => 'Cash']);
+    $processor2 = $this->dummyProcessorCreate(['financial_account_id' => 'Payment Processor Account', 'name' => 'p2', 'payment_instrument_id' => 'EFT']);
+    $contributionParams = [
+      'total_amount' => 100,
+      'currency' => 'USD',
+      'contact_id' => $this->_individualId,
+      'financial_type_id' => 1,
+      'contribution_status_id' => 'Pending',
+    ];
+    $order = $this->callAPISuccess('Order', 'create', $contributionParams);
+    $this->callAPISuccess('Payment', 'create', ['payment_processor_id' => $processor->getID(), 'total_amount' => 6, 'contribution_id' => $order['id']]);
+    $this->callAPISuccess('Payment', 'create', ['payment_processor_id' => $processor2->getID(), 'total_amount' => 15, 'contribution_id' => $order['id']]);
+    $payments = $this->callAPISuccess('Payment', 'get', ['sequential' => 1, 'contribution_id' => $order['id']])['values'];
+    $this->assertEquals('Deposit Bank Account', CRM_Core_PseudoConstant::getName('CRM_Core_BAO_FinancialTrxn', 'to_financial_account_id', $payments[0]['to_financial_account_id']));
+    $this->assertEquals('Payment Processor Account', CRM_Core_PseudoConstant::getName('CRM_Core_BAO_FinancialTrxn', 'to_financial_account_id', $payments[1]['to_financial_account_id']));
+    $this->assertEquals('Accounts Receivable', CRM_Core_PseudoConstant::getName('CRM_Core_BAO_FinancialTrxn', 'from_financial_account_id', $payments[0]['from_financial_account_id']));
+    $this->assertEquals('Accounts Receivable', CRM_Core_PseudoConstant::getName('CRM_Core_BAO_FinancialTrxn', 'from_financial_account_id', $payments[1]['from_financial_account_id']));
+    $this->assertEquals('Cash', CRM_Core_PseudoConstant::getName('CRM_Core_BAO_FinancialTrxn', 'payment_instrument_id', $payments[0]['payment_instrument_id']));
+    $this->assertEquals('EFT', CRM_Core_PseudoConstant::getName('CRM_Core_BAO_FinancialTrxn', 'payment_instrument_id', $payments[1]['payment_instrument_id']));
+    // $order = $this->callAPISuccessGetSingle('Order', ['id' => $processor->getID()]);
+    // $this->assertEquals('Cash', CRM_Core_PseudoConstant::getName('CRM_Core_BAO_FinancialTrxn', 'payment_instrument_id', $order['payment_instrument_id']));
+  }
+
   /**
    * Add a location to our event.
    *
    * @param int $eventID
+   *
+   * @throws \CRM_Core_Exception
    */
   protected function addLocationToEvent($eventID) {
     $addressParams = [