Incorrect Allocation of refunded line items on Multi Event Registrations

Follow up to #34 (closed)

See https://github.com/civicrm/civicrm-core/pull/14763

PR 14763 moves the issue forward, adds tests and fixes (for the most part) adding additional items to event registrations.

This issue is to track what work needs to be done to correct refunds and other issues we have uncovered. After PR 14763 is merged the gaps expected are:

• receive date is updated for all line items not just the affected line items
• refunds still need to be handled
• null accounts in bookkeeping report
• test with taxes
• test with live Payment processor to reflect fees

This is a ample bookig report that shows the issue with Null Accounts:

I have exported and reviewed the data in this spreadsheet:
Report_20191029-1047.ods

I ran an number of sql select statements to review the data after editing the transaction:
14763-contrb-line-ietems.txt

@eileen Please edit and add any comments.

mentioned in issue #34 (closed)

added sig:bug label

added Concept approved triaged type:request labels

@kcristiano my test did not create NULL items in the bookkeeping report so I need some steps to replicate

Steps to replicate Nulls:

• Create a Price Set with multiple line items with different Financial Types
  • Event Fee (Select)
  • Donation (Text/Numeric)
  • Campaign Contribution (Text/Numeric)
• Create an event to use the price set
• Register Contact to the event
  • Choose Event Fee
  • set Donation to 10
• Choose Payment by Cash/check etc
• Edit event
• Change selections
• Leave Event Line item alone
• Increase Donation
• Add a new Campaign Contribution
• Save
• Go to Contribution Tab
• Select Submit Credit Card Payment (This uses Test processor in buildkit installs)
• Use a test CC number
• Complete Payment

Just to add: Null Account codes in Bookkeeping report was an issue without https://github.com/civicrm/civicrm-core/pull/14763

Hmm - I just failed to replicate it - can you configure on demo?

https://wpmaster.demo.civicrm.org/wp-admin/admin.php?page=CiviCRM&q=civicrm/contact/view&reset=1&cid=194

https://wpmaster.demo.civicrm.org/wp-admin/admin.php?page=CiviCRM&q=civicrm/report/instance/20

Filters:

When I use the same 3 scenarios as recorded earlier in my screencast after the https://github.com/civicrm/civicrm-core/pull/14763#issuecomment-547601983 got merged I get the report below:

The event/participantListing report shows: All 3 scenarios should have the restult: amount 100, already paid 50, balance due 50 (When I edit all 3 contributions this shows perfectly, only the report is off) I know this wrong reporting is not the topic here, I wanted to post because I also get the empty account codes and missing financial type records in the bookkeeping report, as shown in my first screenshot.

@magnolia61 Did the PR introduce any new issues/regressions? I found it solved the allocation issue when adding to an event (additional payment needed), while not solving the issues mentioned at the top of this issue.

I did not find any regressions, but if you did please let us know.

OK - in my test I found that if I had the 3 price fields as described

• Event Fee - select
• Donation - text
• Campaign - text

and a fully paid registration with

• Event fee - selected $100
• Donation - qty = 10

I could edit Donation & all was good.

I then edited again & added a value for campaign - at this point I got a 'phantom payment' - which I think is the root of the issue you are describing

@kcristiano I have put up this PR https://github.com/civicrm/civicrm-core/pull/15664

ttps://github.com/civicrm/civicrm-core/pull/15664 solves all of my 3 scenarios of changing event price set fees, with same and different financial types as described here: core#375 (comment 25528) Thank you Eileen for this great step forward!

mentioned in commit core@fab405c8

mentioned in commit core@06eaefe4

It would be useful to create a clean-up procedure for payments already made and the extraneous items that were created. Please think along if the flowing is legit:

Firstly this query:

SELECT * FROM civicrm_financial_trxn ft 
LEFT JOIN civicrm_entity_financial_trxn eft 
ON ft.id = eft.financial_trxn_id 
AND eft.entity_table = 'civicrm_financial_item' 
WHERE eft.id IS NULL 
AND ft.is_payment  = 1;

and to delete those:

DELETE ft FROM civicrm_financial_trxn ft 
LEFT JOIN civicrm_entity_financial_trxn eft
ON ft.id = eft.financial_trxn_id 
AND eft.entity_table = 'civicrm_financial_item' 
WHERE eft.id IS NULL 
AND ft.is_payment  = 1;

After that I noticed more 'ghost' financial transactions without trnx_id.

SELECT *
FROM `civicrm_entity_financial_trxn`
WHERE `financial_trxn_id` IS NULL;

and to delete those:

DELETE
FROM `civicrm_entity_financial_trxn`
WHERE `financial_trxn_id` IS NULL;

I am not sure if this is the way to go to resolve it backwards. As Eileen suggest in https://github.com/civicrm/civicrm-core/pull/15664 it maybe would be a good idea to add it to the system checks.

I am wondering if we have rogue 'amount = 0' items that also clutter the bookkeeping. These were created during canceled registrations and refunds.

SELECT * FROM civicrm_financial_trxn ft 
LEFT JOIN civicrm_entity_financial_trxn eft 
ON ft.id = eft.financial_trxn_id 
AND eft.entity_table = 'civicrm_financial_item' 
WHERE ft.total_amount = 0 
AND ft.is_payment = 1;

DELETE ft FROM civicrm_financial_trxn ft 
LEFT JOIN civicrm_entity_financial_trxn eft 
ON ft.id = eft.financial_trxn_id 
AND eft.entity_table = 'civicrm_financial_item' 
WHERE ft.total_amount = 0 
AND ft.is_payment = 1;

WARNING: Of all these repair queries I am not sure yet if they are valid. Please comment and help come up with something solid. :-)

@kcristiano can you check this original report & cross out 1 & 3 on this list if you agree they are fixed

Related PRs

Fix Payment.create bug whereby payment_processor_id is not being used for the to_account_id #15640
Fix Payment.create with a negative value to create the correct financial items #15705
Fix double loop on lineItems #15686

[NFC] Test class preliminary clean up #15685 MERGED
Cleanup date handling on Payment.create #15687 MERGED

changed the description

closing based on merging of all above PRs

closed

mentioned in issue #98 (closed)

@eileen @kcristiano How about the mysql cleanup for civicrm_financial_trxn and civicrm_entity_financial_trxn? Would it be wise to include this in the update script or would another way of offering this cleanup be better?

@magnolia61 I guess we could add a status check for them. I think that is just the extraneous ones we can clean up - not all the other variants of problems here

@magnolia61 @eileen How about adding them via an API (similar to System.updateindexes) so that a sysadmin can run them if required.

I would not want to put the cleanup in an upgrade script as we cannot know what consequences that would have.

@mattwire I agree the best way is to allow the sysadmin would decide to run. The API method sounds like the best path.