Stop adding a refund payment from creating extraneous financial items

[eileenmcnaughton]

Overview

Stop adding a refund payment from creating extraneous financial items

Per
https://github.com/civicrm/civicrm-core/pull/15143#issuecomment-527245150

and https://github.com/civicrm/civicrm-core/pull/13114/files#diff-8967f6e57d6209aa277ded6512413608L326

Before

Financial items created when adding a refund to a contribution but @monishdeb - see https://github.com/civicrm/civicrm-core/pull/13114/files#diff-8967f6e57d6209aa277ded6512413608L326 and @pradpnayak - see comment below - have both said they should not be.

In this test we have the following events

1. Contribution is created for $300 with a partial payment of $150. Line items are $100 & $200
2. A payment is added for $50
3. the payment is update to be $100

These payments look as expected -

civicrm_financial_trxn

The final outcome is that the allocation in the civicrm_financial_item table is equal to the sum of payments $250 and the total of the entity_financial_trxns for this exceeds the amount paid

civicrm_financial_item

However, BEFORE we started adding payments the allocation was actually equal to the sum of the contribution.

civicrm_entity_financial_trxn - entity_table = civicrm_financial_item

civicrm_entity_financial_trxn - entity_table = civicrm_contribution

So prior to adding payments the financial_items allocated the total amount to pay (paid and unpaid) to the line items via financial items but in the end we would up with only the amount paid allocated. This feels wonky.

After

Only the original 2 financial items created. Test locks this change in.

Technical Details

Comments

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

[pradpnayak]

Hi @eileenmcnaughton

Looking closely from your attached image in description it looks like the data are stored incorrectly in some tables

1. civicrm_financial_trxn
   Entries looks good for me. (Except 4 and 5, if both needs to be combined together and stored as diff amount i.e when payment was updated from 50 to 100 there should be one entry for 50 instead of -50 and 100, @JoeMurray can you please confirm? )

2. civicrm_financial_item
   Entry 1-2 are correct, 3 and 4 shouldn't be created. Entry in financial items are only created when a new contribution is created or line item amount or Financial type is changed or contribution(not payment) status is set to refunded or cancelled not in other cases.

3. civicrm_entity_financial_trxn - entity_table = civicrm_financial_item

a . Entries for trxn_id = 2 is missing.
b. Entries for 9-13 for entity_id should 1 and 2 instead 3 and 4 resp.

4. civicrm_entity_financial_trxn - entity_table = civicrm_contribution
   Looks good. (If 4 and 5 needs to combined as per my above comment than here will only one entry with $50 instead -50 and 100)

Hope my points make sense.
Pradeep

[eileenmcnaughton]

Thanks @pradpnayak and thanks for your guidance on the other issue. I'll read through them later and mull them

[eileenmcnaughton]

@monishdeb @pradpnayak based on the above conversation (& elsewhere) I have updated this PR to remove the extraneous line item creations.

This was actually originally proposed by @monishdeb as part of 'the larger change' - but in this context I think it stands alone & is mergeable as an agreed change & simplifies the larger change

Note I have NOT addressed all @pradpnayak's points above here - just this one as it appears to be separable from the others

[monishdeb]

I agree with this rightful change and does avoid creating those extraneous items. I am marking with MoP tag.

[JoeMurray]

This is a very significant change.

Before we move away from doing reversal transactions followed by posting of new transaction, I think we need:

1. some feedback from accountants in the ecosystem to validate the approach of a single difference transaction,
2. significant additional tests to ensure that difference transactions are accurately created in cases where there are various more complex histories for a contribution, including various combinations of changes, partial payments, refunds and use of credit notes,
3. some analysis of how this will affect reports
4. some analysis of whether anything is needed for upgrades given this switch to how bookkeeping data will be stored.

My sense is that this is likely the right direction, but it is wrong to treat this as just a small change that can be discussed by developers on a pull request. @lcdservices who do you think could help with the discussion regarding accounting?

[JoeMurray]

@KarinG ^

[JoeMurray]

Does anyone know if Jamie Novick has a nick here in github so we can reference him?

[JoeMurray]

Here is a site that suggests when it is appropriate to use an adjusting entry (which we have called a difference transaction) and when it is more appropriate from an accounting perspective to use a reversal transaction followed by a new transaction:
https://www.patriotsoftware.com/accounting/training/blog/what-is-correcting-entries-journal-examples/

I think most of our changes are not related to correcting entries but to posting changes to the transaction in the regular course of its lifecycle.

[eileenmcnaughton]

@JoeMurray in what way is not creating financial items on adding a refund payment a significant change? It was included in the earlier PR by @monishdeb as a minor sub part, it doesn't happen on payment, only refund and after reviewing the spec after reading @pradpnayak's comments I looked at the confluence page (which one again I've lost the link to) and it talks about updating the status of financial items when a refund is received, not adding new ones - so this seems to bring us to the behaviour described in confluence rather than be a signficant change

[eileenmcnaughton]

Note that your discussion is all about financial transactions - this PR does not change financial transaction creation

[JoeMurray]

By financial transaction in this context, I meant bookkeeping entries in civicrm_financial_trxn, civicrm_financial_item and related tables including civicrm_entity_financial_trxn.

I relied on @monishdeb 's explanation that due to refactoring that has occurred, what was being discussed would modify how Civi handles any change to a bookkeeping entry so that there would be no more reversal of old (ie backing out the current status of the bookkeeping entry) followed by insertion of new, but instead a single insertion of difference transaction.

Are you referring to https://wiki.civicrm.org/confluence/display/CRM/CiviAccounts+Data+Flow ?

The pattern at one time in 4.2 was to do 'difference' transactions only. Then dgg pushed successfully for us to always do reversal and post new. That was the general pattern, except for this recently noticed outlier that did a difference transaction.

[eileenmcnaughton]

@JoeMurray thanks - if you look at the code & overview this is about financial items

[monishdeb]

@JoeMurray yes we are referring https://wiki.civicrm.org/confluence/display/CRM/CiviAccounts+Data+Flow here. And this PR is fixing a bug we encountered during the refund, which records few extraneous financial item that is causing an issue in bookkeeping entry (shown under Before section in 'financial_item' records ) Also an existing UT is extended to ensure the fix made in this PR

We are also ensuring there is a reversal amount recorded whenever the price-option is changed that leads to refund (if its a lower price option then earlier) or partial payment (if its a higher price option then earlier) or financial type change as per dataflow

[monishdeb]

I did r-run again and happy with the patch. Merging now.