Fix Payment.create with a negative value to create the correct financial items

[eileenmcnaughton]

Overview

Fixes a bug whereby refund payments are not resulting in entity_financial_trxn records being created. This is a regression but I don't know how recent - the tests that should have caught it did not as they were only checking that they were created correctly IF created. Due to complexity I am targetting 5.20 not 5.19

Before

Record a refund payment using api or Additional Payment form - no entity-financial items created with table = 'civicrm_financial_items'

After

Record a refund payment using api or Additional Payment form - proportional entity-financial items created

Technical Details

This PR contains the contents of a bunch of other PRs that would ideally be merged & resolved first but which are still hanging around in review - I was going to wait to put this up but there is some urgency that it's merged for 5.20

The job of a refund in this function turns out to be exactly the same as the a payment. I originally thought the
from & to accounts would be reversed but in fact because we are recording a negative amount that is not the case.
We need the from-to accounts to be the same. The only difference turns out to be the status
of the financialTrxn (Completed vs Refunded it seems).

Without this change the refund code is calling the recordFinancialAccounts function but after stepping
through it I'm sure the only thing that function is doing for it is creating the
FinancialTrxn. It should be creating the EntityFinancialItem too but it is not
and the best place for that to be done is in the payment class not in a spaghetti function.

Note that after this we need some robust testing & probably a follow up
patch on the handling of tax items - which is unlikely to be correct since it wasn't in the past
AFAIK and is unchanged.....

However, this fixes an important bug & should be merged as soon as it is confirmed non-regressive
to allow the next one to be investigated

Comments

@JoeMurray @kcristiano this is the most important PR.

Please be very clear on steps to replicate for any problems you find as the last PR got stalled for a few months over lack of clarity over steps to replicate a possible issue (which I think turned out to be the bug in changing line items that we fixed)

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

[magnolia61]

I did some testing with this patch. The scenario I used was:

1. register for an event with event fee of 100 euro
2. pay the 100 euro with "record contribution" (payment type: bank)
3. change the event registration so that the fee is now 50 euro
4. record a refund of 50 euro (payment type: cash)

Without this patch these are the results of the civicrm_financial_trxn & civicrm_entity_financial_trxn tables

With this patch the results are as below:


The only difference I can see is that the to_financial_account_id
without the patch = 9
with the patch = 15

---

A side-catch I discovered is a wrong amount for the amount paid in the notification mail

This should be:

Total amount:   50
You paid:      100
Refund amount: -50

[eileenmcnaughton]

@magnolia61 the change in financial account should have come from the top of that chain - ie this change

#15640

When you make a payment the money should go 'to' the account associated with the way you made the payment - so from payment processors that is configured by processor but is normally the 'Payment Processor account'. When no processor is in play then it is configured by payment type. (Usually Bank deposit). The key think is it should go to the account associated with the actual payment details. If you do further checks on this you might want to comment on #15640 which is specifically on that issue (but included in this chain)

Regarding your other non-change diagnosis. There are 2 types of entity_financial_trxn records. I'm been so focussed on the financial_item ones I didn't disambiguate them. However, I think you are looking at the ones with 'civicrm_contribution' in the entity_table & not the ones with 'civicrm_financial_items'?

[seamuslee001]

Jenkins re test this please

[seamuslee001]

@eileenmcnaughton can you rebase this please

[eileenmcnaughton]

@kcristiano I've rebased this now that the other one you reviewed is merged

[kcristiano]

• (r-run) Exception

Created event with Price set that had:

Event Fee (100 and 500 USD Options dropdown select)
Donation (Text)
Campaign Contribution (Text)

Entered Registration
Fee - 500 by dropdown select
Donation 10
Campaign 10

$210 Contribution recorded

Edited Event
Changed Selections
Event Fee changed to 100
No other changes
Saved

Recorded Refund

Worked perfectly

Event Fee (100 and 500 USD Options dropdown select)
Donation
Campaign Contribution

Entered Registration
Fee - 100 by dropdown select
Donation 200
Campaign 200

$500 Contribution recorded

Edited Event
Changed Selections
Event Fee unchanged
Changed Campaign Contribution to 0
Saved

Recorded Refund

Worked perfectly

Last test case:

Using local client DB with an event with over 50 line items

Registered for event:

Choose Basic Registration
Price set line line items options for Event, Guest Fee, and Extra Meals -- All Checkboxes

Event 820
Guest 100
Meals 225
Total 1,145.00

Edited Event
Changed selections
Remove meals (Checkboxes)
Recorded refund $225

Contribution status shows $920

However, Bookkeeping report shows -225

In SQL

The contribution looks as expected:

select c.id,c.contact_id,c.total_amount,c.fee_amount,c.net_amount,c.trxn_id,c.receive_date,c.contribution_status_id,cov.name 
from civicrm_contribution c 
join civicrm_option_value cov on cov.value = c.contribution_status_id and cov.option_group_id = 11 and cov.value = c.contribution_status_id where c.id = 16095

The Line Items are fine

select li.id,li.entity_table,li.entity_id,li.contribution_id,li.label,li.line_total,li.financial_type_id,ft.name 
from civicrm_line_item li join civicrm_financial_type ft on ft.id = li.financial_type_id where li.contribution_id = 16095

civicrm_financial_trxn appears to be where we start to go off

select cft.id,cft.from_financial_account_id, cft.to_financial_account_id,cft.trxn_date,cft.trxn_id, cft.total_amount,cft.status_id 
from civicrm_financial_trxn cft 
where cft.id >= '18121'

The issue appears to be in civicrm_entity_financial_trxn

join civicrm_financial_trxn cft on cft.id = eft.financial_trxn_id and cft.trxn_date >= '2019-11-04'
where eft.id >= 77057
order by eft.financial_trxn_id

This may be an issue with Data Types (Checkboxes) or an issue with this client Database.

@eileenmcnaughton what do you think? Better in than out for more testing?

I can attempt to recreate on a clean install if you want, but I will need some more time and we are up against the RC being cut.

[eileenmcnaughton]

@kcristiano if you agree if fixes a known issue then we should merge IMHO

[kcristiano]

This fixes a known issue. I think it should go in and we can continue testing this exception.

[eileenmcnaughton]

@kcristiano great - merging - please lot a new gitlab with steps to replicate issue in the next few days

[eileenmcnaughton]

@magnolia61 - I added https://lab.civicrm.org/dev/financial/issues/99 to track the template issue - not sure you got the ping https://lab.civicrm.org/dev/financial/issues/99