Fix Payment.create bug whereby payment_processor_id is not being used…

… for the to_account_id

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
#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.

CRM/Contribute/BAO/Contribution.php

@@ -952,8 +952,8 @@ public static function recordPaymentActivity($contributionId, $participantId, $t
    * @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']);

CRM/Financial/BAO/Payment.php

@@ -62,12 +62,7 @@ public static function create($params) {
     $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']);

CRM/Financial/BAO/PaymentProcessor.php

@@ -120,6 +120,26 @@ public static function getCreditCards($paymentProcessorID = NULL) {
     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.
    *

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'];

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',
+  ];
 }
 
 /**

tests/phpunit/api/v3/PaymentTest.php

@@ -761,6 +761,8 @@ public function testCreatePaymentIncompletePaymentPartialPayment() {
 
   /**
    * Test create payment api for pay later contribution with partial payment.
+   *
+   * @throws \CRM_Core_Exception
    */
   public function testCreatePaymentPayLaterPartialPayment() {
     $this->createLoggedInUser();
@@ -772,7 +774,7 @@ public function testCreatePaymentPayLaterPartialPayment() {
       '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 @@ public function testCreatePaymentPayLaterPartialPayment() {
     $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 = [