Prevent financial transactions from being saved with no payment instrument

[eileenmcnaughton]

Overview

Port of #12502

Since time is tight I've created this in advance of @pradpnayak confirming 5.4 is good to merge - get those tests run

https://lab.civicrm.org/dev/core/issues/264

[civibot]

(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).

[eileenmcnaughton]

test this please

[monishdeb]

@eileenmcnaughton I have tested the issue on my local and here's a screencast:

I noticed that

1. Not only contribution's payment_instrument is stored in the new financial transaction but also the additional data like check_number, cc type etc. In payment details it show two payment records which got same additional data but I think its ok.

2. Corresponding line-item, financial item is not updated.

3. Need to extend any backoffice contribution UT to replicate this use-case and check the financial transaction details.

[eileenmcnaughton]

@monishdeb so 5.3.1 is to be cut tomorrow - I understand from your comment we should not attempt to get this into 5.3.1 but rather see if a 5.4 rc fix is viable?

Or is it the case that 'this is a little bit better but not perfect' - in which case we should perhaps

• merge to 5.3 & 5.4
• look to freeze the fields in 5.5 with the option to use line item editor or delete & recreate

[eileenmcnaughton]

@monishdeb AND further to the above - would loading payment_instrument_id from prevContribution make this any more mergeable from your POV

[monishdeb]

I think we shouldn't introduce this patch in 5.3 but in 5.4-rc for sure as I believe it's not a regression and allowing the fix without updating the line-item will affect the financial reports.

And yes we will then look forward to freeze those 3 fields in 5.5

[eileenmcnaughton]

@monishdeb so I understand this as an existing problem that is exposed as of 5.1 due to the payment form being elevated - in fact prior to that I believe a payment_instrument_id would have been enforced at the form level

Does this change alter the line item updates of does core fail to do it properly before AND after?

[monishdeb]

I believe its a core issue before 5.1 and its an inefficiency of the contribution BAO create() to update the linked financial records. So I would core fails to update the line-item before/after when total_amount is either decreased or increased.

[pradpnayak]

I don't think its a core bug. The amounts do get update in all tables when someone updates amount from contribution form and is been working for a quite a long time. I tried @eileenmcnaughton patch on D7 with no extension and it seems to work as per CiviAccounts Data flow. Here's the proof

Before applying patch :

After applying patch :

+1, We should freeze amount for non-pending contributions but won't that be 2-3 step process to actually update a amount for contribution? Won't it make sense for Edit line item extension to freeze this fields?

[eileenmcnaughton]

@monishdeb @pradpnayak so "I don't think it's a core bug" refers to the line items?

We have 2 potential bugs being discussed here

1. a contribution is saved with no payment_instrument_id AND that means the payment_instrument_id is empty on the financial_trxn & cannot be updated

2. Corresponding line-item, financial item is not updated.

For each of these do we

a) agree it's a core bug
b) agree that the impact of this patch is a) no change b) improvement c) regression

[pradpnayak]

@eileenmcnaughton here is a reply for each of your points

a contribution is saved with no payment_instrument_id AND that means the payment_instrument_id is empty on the financial_trxn & cannot be updated

Yes, this a bug and your patch fixes the problem. financial_trxn.payment_instrument_id should not be NULL in case of payment status is Completed.

Corresponding line-item, financial item is not updated.

When a amount is changed new entry is created in financial item table with difference amount and line item amount is updated to total amount. This is working now and is been working for quite a long time. You can see in the gif image that line item is updated with correct amount and additional financial item is also created.

a) agree it's a core bug

1. For payment instrument set to NULL in financial trxn table - Yes
2. Corresponding line-item, financial item is not updated. - No , It does get updated and created.
3. Not only contribution's payment_instrument is stored in the new financial transaction but also the additional data like check_number, cc type etc. In payment details it show two payment records which got same additional data but I think its ok. - No , its a right behaviour.

b) agree that the impact of this patch is a) no change b) improvement c) regression

For payment instrument set to NULL in financial trxn table - Yes, this a regression since payment instrument is moved from contribution form to payment edit form.

@monishdeb is there any specific scenario where you feel 'line-item, financial item is not updated' which i am missing to test it? like additional configuration in CiviContribute settings, Financial type etc

Ref: https://wiki.civicrm.org/confluence/display/CRM/CiviAccounts+Data+Flow#CiviAccountsDataFlow-Changecontributionamount(quickconfig/singleline_item)

@eileenmcnaughton but its worth adding unit test for this to capture such scenario in future. Thoughts?

[monishdeb]

My bad, its something to do with line-item editor extension which prevents linked line-item to be updated. When I disabled the extension it worked fine. Also I checked with @eileenmcnaughton to ensure that it worked fine after applying the patch.

And I can see why its a regression as now we can't submit previous selected payment_instrument value on Edit form as its been moved to payment edit form. Thanks @pradpnayak for your detail investigation.

I would say it is ok to merge against 5.3.

[monishdeb]

Will it be ok to merge it against 5.3 without UT that replicate this scenario?

[eileenmcnaughton]

I think we can't get a unit test for 5.3 but should aim to do one for master (any volunteers?)

@monishdeb @pradpnayak so we are agreed on merging this to 5.3 & 5.4?

I also note that earlier in the same function I see

    $balanceTrxnParams['payment_instrument_id'] = $params['contribution']->payment_instrument_id;

• could this also have the same bug associated? If so we could do the load immediately after contribution->save()

or we could load from prevContribution but I worry about

    $setPrevContribution = TRUE;
    // CRM-13964 partial payment
    if (!empty($params['partial_payment_total']) && !empty($params['partial_amount_to_pay'])) {
      $partialAmtTotal = $params['partial_payment_total'];
      $partialAmtPay = $params['partial_amount_to_pay'];
      $params['total_amount'] = $partialAmtTotal;
      if ($partialAmtPay < $partialAmtTotal) {
        $params['contribution_status_id'] = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Partially paid');
        $params['is_pay_later'] = 0;
        $setPrevContribution = FALSE;
      }
    }

• happy to merge this against 5.3 & 5.4 & we can dig into that plus UT on master

[monishdeb]

Sure I will merge it and will submit a PR for UT against 5.4

[pradpnayak]

Thanks @eileenmcnaughton for the fix and @monishdeb for adding UT.

+1 on loading $params['contribution'] after $contribution->save();

[eileenmcnaughton]

@monishdeb I'll merge to 5.4 & you can do the UT on master

@pradpnayak @monishdeb REALLY appreciate you guys for this & all the other stuff you have been doing