CRM-20750: Incorrect financial trxn entries when payment instrument i…

[eileenmcnaughton]

@monishdeb this is the portion of #10539 that I understand. Mostly I have removed the whitespace changes & the test changes (as opposed to the additional test). I expect this to fail & the reason for the test changes to become clearer to me.

Note that I did a further extraction of self::isPaymentInstrumentChange for readability.

In general I feel fairly good about test coverage of the function being changed & most of your change is just tidying up code (unscattering & moving to a function).

There is a change in here that is not spelt out - when altering a check number 2 transactions will no longer be created (a negative & a positive one). Much as I passionately agree with that change I think it needs to be spelt out in the spec & if Joe agrees to it I believe some documentation needs to be updated

---

• CRM-20750: Incorrect financial trxn entries when payment instrument is changed on backoffice Contribution edit form
  https://issues.civicrm.org/jira/browse/CRM-20750

Overview

Fix to incorrect financial items created when editing a payment through the back office contribution form

Before

After altering the credit card details are incorrectly assigned to the reversal entry
[https://issues.civicrm.org/jira/secure/attachment/61870/Screen%20Shot%202017-06-20%20at%205.35.02%20PM.png]

After

Reversal entry is 'clean' of the details from the new transaction. Code readability improved & unit test added

Technical Details

Comments

This is a reviewers commit of #10539

I have done an extraction and made changes to the test. I have not changed what the test tests, which was my concern in the first commit before review.

Notable changes

1. I removed this from the reversal entry. I believe the reversal should be reversing against the original account. If this value is to be used it would be a from_financial_account_id entry

$lastFinancialTrxn['to_financial_account_id'] = CRM_Financial_BAO_FinancialTypeAccount::getInstrumentFinancialAccount($currentContribution->payment_instrument_id);

2. I have changed from
   elseif ((!CRM_Utils_System::isNull($params['contribution']->payment_instrument_id) ||
   to

elseif ((!CRM_Utils_System::isNull($params['payment_instrument_id'])

& switched && rather than || in the logic. This was causing a test failure which was changed in the original PR by changes to the test. However, I felt that the logic in fact was wrong, rather than the test. We should check if there is an attempt to change the payment_instrument_id (as represented by the $params array) rather than whether the $params['contribution'] has one.

Unfortunately I think I might be personally guilty of having added $params['contribution'] to the $params array in about 4.2 as a hack to get some data onto the form :-(

[eileenmcnaughton]

test this please

[eileenmcnaughton]

@monishdeb can you review this as a proposed replacement for #10539 - note that your commit is more-or-less-yours with some refactoring added but my 2 are logic changes from yours.

[monishdeb]

@eileenmcnaughton I agree with the final patch. Thanks a lot :)

Tested on my local, working fine :

Sooner or later I will submit PR to make _checkFinancialTrxn(..) readable as per checkFinancialTrxnPaymentInstrumentChange(...)

Should I merge it then?

[sunilpawar]

checking non null value for payment_instrument_id from $params['payment_instrument_id'] and $params['prevContribution']->payment_instrument_id, but in next line comparing payment_instrument from
$params['contribution']->payment_instrument_id

[monishdeb]

@sunilpawar we are still comparing payment_instrument_id of 'contribution' and 'prevContribution' respectively. On which line we are comparing the payment_instrument_id with name payment_instrument ?

[sunilpawar]

@monishdeb can we connect Skype? i already send skype request.

[eileenmcnaughton]

I think this was something well spotted by @sunilpawar & this should be merged
#10980