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 8aafcee8eea5..3f610b9d0a76 100644 --- a/CRM/Contribute/BAO/Contribution.php +++ b/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']); diff --git a/CRM/Financial/BAO/Payment.php b/CRM/Financial/BAO/Payment.php index 1a9676379843..d6051d3bf283 100644 --- a/CRM/Financial/BAO/Payment.php +++ b/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']); diff --git a/CRM/Financial/BAO/PaymentProcessor.php b/CRM/Financial/BAO/PaymentProcessor.php index 27f563d5f1cd..a0de2a420260 100644 --- a/CRM/Financial/BAO/PaymentProcessor.php +++ b/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. * diff --git a/api/v3/Payment.php b/api/v3/Payment.php index 8198d1e9c18f..3460f95c6d14 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 435fd97e977e..96c18f608aa9 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 1f6f3e2215cf..6cfc2dec20d9 100644 --- a/tests/phpunit/api/v3/PaymentTest.php +++ b/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 = [