Fix a regression whereby payment details are not saved from the AdditionalPayment form #15537

eileenmcnaughton · 2019-10-18T06:42:04Z

Overview

I discovered the Payment.create api is not respecting a bunch of parameters - since this is now called from the AdditionalPaymentForm they are not being saved.

payment_instrument_id, card_type_id, pan_truncation & other field were not being passed through - resulting in incorrect payment saves

Before

Add a pending contribution & then submit a credit card payment using the additional payment form

After

Technical Details

This looks worse than it is because I also had to pull in my upstream param renaming commit #15535

Comments

@monishdeb @jitendrapurohit can one of you check

civibot · 2019-10-18T06:42:06Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

mattwire · 2019-10-18T08:50:44Z

@eileenmcnaughton test fails look related - they're all returning

CiviCRM_API3_Exception: DB Error: constraint violation

mattwire · 2019-10-18T09:32:55Z

api/v3/utils.php

@@ -390,6 +390,9 @@ function _civicrm_api3_get_BAO($name) {
    // not the other cache info like search results (which could in fact be in Redis or another cache engine)
    $name = 'PrevNextCache';
  }
+  if ($name === 'Payment') {
+    $name = 'FinancialTrxn';
+  }


@eileenmcnaughton Is this change required for this PR? As I understand it Payment does not map directly to FinancialTrxn but wraps the financial entities.

@mattwire yes - otherwise the pseudoconstant doesn't work.

Payment is a psuedo-api that represents one specific type of FinancialTrxn - hence the presence of 'is_payment' as a fixed parameter when creating a financial_trxn from the payment api.

Ie it's a subset of the FinancialTrxn entity

mattwire · 2019-10-18T09:34:06Z

api/v3/Payment.php

+    'order_reference' => [
+      'name' => 'order_reference',
+      'type' => CRM_Utils_Type::T_STRING,
+      'title' => ts('Order Reference'),
+      'description' => ts('Payment Processor external order reference'),
+      'maxlength' => 255,
+      'size' => 25,
+      'where' => 'civicrm_financial_trxn.order_reference',
+      'table_name' => 'civicrm_financial_trxn',
+      'entity' => 'FinancialTrxn',
+      'bao' => 'CRM_Financial_DAO_FinancialTrxn',
+      'localizable' => 0,
+      'html' => [
+        'type' => 'Text',
+      ],
+    ],


@eileenmcnaughton order_reference was only added in 5.20 - will this cause problems on 5.19 as the column won't exist in the DB?

Yeah - I think it's the cause of the test fails - I just pulled it out again

mattwire · 2019-10-18T09:35:19Z

api/v3/Payment.php

      'type' => CRM_Utils_Type::T_INT,
-      'description' => ts('Payment processor ID - required for payment processor payments'),
+      'title' => ts('Payment Processor'),
+      'description' => ts('Payment Processor for this financial transaction'),


Can we use Payment processor for this payment. As the payment API will be creating one or more financial transactions depending on the action.

Yes - good point

mattwire · 2019-10-18T09:36:17Z

CRM/Financial/BAO/Payment.php

@@ -59,27 +59,36 @@ public static function create($params) {

    $isPaymentCompletesContribution = self::isPaymentCompletesContribution($params['contribution_id'], $params['total_amount']);

+    $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', 'order_reference', 'trxn_id'];


@eileenmcnaughton See comment on order_reference which is only available from 5.20 - though I don't think it will cause problems in this list.

yep - removed

mattwire

@eileenmcnaughton I've added a couple of comments. The main concern is around order_reference which did not exist in 5.19.
Is there a minimum version of this PR without the variable rename that could be merged for 5.19 and then do the rest for 5.20?
As this is modifying the internals of CRM_Financial_BAO_Payment::create it has the potential to break badly if we've missed anything!

eileenmcnaughton · 2019-10-18T21:41:47Z

@mattwire I've fixed the order_reference part. To be honest I started working on 5.20 & then moved back to 5.19 once I realised I'd hit a regression.

The variable rename is 'safe' & hopefully my comments on the other PR address your concerns. If so we can merge that to sync this with current master & rebase this to be cleaner

eileenmcnaughton · 2019-10-18T23:48:32Z

test this please

eileenmcnaughton · 2019-10-19T01:17:46Z

@magnolia61

mattwire · 2019-10-20T18:05:30Z

@eileenmcnaughton Please rebase and I'll take another look

eileenmcnaughton · 2019-10-20T23:22:32Z

@mattwire done - also FYI - the reason I moved the param gathering outside the IF is that the same params are actually required if total_amount < 0 - that code is for historical reasons in it's own function but over a few refactors it would start to share that paymentParams array

eileenmcnaughton · 2019-10-21T01:17:51Z

Hopefully that sorts the test. Along with #15561 we should aim for 5.18

mattwire · 2019-10-21T09:45:42Z

test this please

jitendrapurohit · 2019-10-21T12:12:23Z

I've tested this on contribution, event payments and confirm that the params are correctly set after the submission. The payment.create api call can also set these values in the record.

Failing test is SyntaxConformanceTest.php::testLimit for Payment Entity. Should this entity be added to static::toBeSkipped_getlimit() as well?

On some digging I found that various payment related fields like pan_truncation & card_type_id were being quietly ignored rather than saved by Payment.create The additional payment form now uses this api so it is a regression on that form. This fixes the metadata, tests & support for payment-related trxn fields

Agreed elsewhere that the param name change is fine

seamuslee001 · 2019-10-21T23:47:51Z

merging as per the collective reviews

civibot bot added the 5.19 label Oct 18, 2019

eileenmcnaughton force-pushed the pay_check branch from ab1552f to 366d2f5 Compare October 18, 2019 06:53

mattwire reviewed Oct 18, 2019

View reviewed changes

mattwire previously requested changes Oct 18, 2019

View reviewed changes

eileenmcnaughton force-pushed the pay_check branch 2 times, most recently from 72b09f5 to 275b4c9 Compare October 18, 2019 21:40

eileenmcnaughton force-pushed the pay_check branch from 275b4c9 to 3085d15 Compare October 19, 2019 23:46

eileenmcnaughton force-pushed the pay_check branch from 3085d15 to cab89cc Compare October 20, 2019 23:20

eileenmcnaughton force-pushed the pay_check branch 2 times, most recently from 69c028d to 3643945 Compare October 20, 2019 23:40

eileenmcnaughton force-pushed the pay_check branch 2 times, most recently from d4cbc42 to 7d628f4 Compare October 21, 2019 07:20

eileenmcnaughton force-pushed the pay_check branch from 7d628f4 to a494d7a Compare October 21, 2019 21:44

seamuslee001 merged commit f8fdba9 into civicrm:5.19 Oct 21, 2019

seamuslee001 deleted the pay_check branch October 21, 2019 23:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a regression whereby payment details are not saved from the AdditionalPayment form #15537

Fix a regression whereby payment details are not saved from the AdditionalPayment form #15537

eileenmcnaughton commented Oct 18, 2019 •

edited

Loading

civibot bot commented Oct 18, 2019

mattwire commented Oct 18, 2019

mattwire Oct 18, 2019

eileenmcnaughton Oct 18, 2019

mattwire Oct 18, 2019

eileenmcnaughton Oct 18, 2019

mattwire Oct 18, 2019

eileenmcnaughton Oct 18, 2019

mattwire Oct 18, 2019

eileenmcnaughton Oct 18, 2019

mattwire left a comment

eileenmcnaughton commented Oct 18, 2019

eileenmcnaughton commented Oct 18, 2019

eileenmcnaughton commented Oct 19, 2019

mattwire commented Oct 20, 2019

eileenmcnaughton commented Oct 20, 2019

eileenmcnaughton commented Oct 21, 2019

mattwire commented Oct 21, 2019

jitendrapurohit commented Oct 21, 2019

seamuslee001 commented Oct 21, 2019

		@@ -59,27 +59,36 @@ public static function create($params) {

		$isPaymentCompletesContribution = self::isPaymentCompletesContribution($params['contribution_id'], $params['total_amount']);

		$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', 'order_reference', 'trxn_id'];

Fix a regression whereby payment details are not saved from the AdditionalPayment form #15537

Fix a regression whereby payment details are not saved from the AdditionalPayment form #15537

Conversation

eileenmcnaughton commented Oct 18, 2019 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Oct 18, 2019

mattwire commented Oct 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattwire left a comment

Choose a reason for hiding this comment

eileenmcnaughton commented Oct 18, 2019

eileenmcnaughton commented Oct 18, 2019

eileenmcnaughton commented Oct 19, 2019

mattwire commented Oct 20, 2019

eileenmcnaughton commented Oct 20, 2019

eileenmcnaughton commented Oct 21, 2019

mattwire commented Oct 21, 2019

jitendrapurohit commented Oct 21, 2019

seamuslee001 commented Oct 21, 2019

eileenmcnaughton commented Oct 18, 2019 •

edited

Loading