From 6cc6cb8c60809716a793ee036915c2461a0b8c99 Mon Sep 17 00:00:00 2001 From: eileen Date: Tue, 29 Oct 2019 00:22:38 +1300 Subject: [PATCH] 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 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. --- CRM/Contribute/BAO/Contribution.php | 4 +-- CRM/Financial/BAO/Payment.php | 7 +---- CRM/Financial/BAO/PaymentProcessor.php | 20 ++++++++++++++ api/v3/Payment.php | 6 +++++ api/v3/PaymentProcessor.php | 6 +++++ tests/phpunit/api/v3/PaymentTest.php | 36 +++++++++++++++++++++++++- 6 files changed, 70 insertions(+), 9 deletions(-) diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php index 8aafcee8ee..3f610b9d0a 100644 --- 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 1a96763798..d6051d3bf2 100644 --- 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 27f563d5f1..a0de2a4202 100644 --- 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 8198d1e9c1..3460f95c6d 100644 --- 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 435fd97e97..96c18f608a 100644 --- 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 1f6f3e2215..6cfc2dec20 100644 --- 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 = [ -- 2.25.1