dev/financial#34 : Incorrect allocation of payment on an edited multi-line item event registration #13114

monishdeb · 2018-11-16T19:22:25Z

Overview

Edit of one line item previously led to changed allocations on more than one line item. This has been fixed so only the edited line item has new financial entries associated with it.

Steps to replicate:

Create new Financial Type Event Fees 2.
At Administer > Financial Accounts, navigate to the Financial Account this creates also called Event Fees 2 and enter 4301 as the Accounting Code.
Create an event with priceset with two fields, first with financial Type Event fees, second with FT of Event Fees 2.
Register in event with payment of say $25 for first ticket (Event Fees) and $10 for second ticket (Event fees 2), total $35.
Edit Event registration. Click Change selections. Change quantity for second field from 1 to 5, line item total of $50, contribution total of $75, total paid $35. Save. Balance is $40.
Click Record Payment and pay the balance.

Before

At Contributions > Accounting Batches > Open Batches, select the batch just created and export as csv, open and view the resulting transactions: Financial_Transactions_3_20180807210702

After

Financial_Transactions_3_20181119192008.csv

RESULT: If you compare both the CSV results, each file show same number of records in which first two records represent the financial entries for respective tickets ($25 and $10), third entry is the financial entry created to indicate the outstanding balance of $40 and is payable (i.e. is_payment = FALSE). For the last two entries the amount differ whereas now after the fix $40 is being assigned to Ticket2 (as qty is changed from 1 to 5, thus amount $10 is changed to $50) and $0 to Ticket 1 unlike earlier where amount is proportionally distributed to two tickets on basis of total contribution amount, instead of financial item's amount

Technical Details

I have changed the algorithm to do allocation on basis of financial_item.amount. Ensured that proportional allocation of financial items, in case of partial payment is not changed and we already have existing UTs for this.
In addition I have made changes in various unit test which does not follow the standard approach to change amount of a existing contribution (in order to calculate proportional amount allocation to different line-items).

Comments

ping @JoeMurray @kcristiano @pradpnayak @eileenmcnaughton @mattwire

civibot · 2018-11-16T19:22:27Z

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

JoeMurray · 2018-11-19T19:45:14Z

@monishdeb failing check...

kcristiano · 2018-11-28T17:31:07Z

@monishdeb I've just followed the steps detailed above on an existing event registration.

I made a change to reduce the quantity. The event transaction looks correct (they have a quantity of 1 not 2). However the refund was pro rated against all financial types in the contribution record as oppose to applying to the financial type that was reduced.

I also get a line item on the bookkeeping report with a Debit to Accounts Receivable and no credit account.

If I add to the event registration, I still get the odd line item with a debit to A/R but no credit account. The additional item is recorded properly with the correct financial account and Type.

Let me know what more details i can share with you so you can replicate the issue.

cc @JoeMurray

monishdeb · 2018-11-28T17:47:13Z

Thanks, @kcristiano for notifying that odd behavior. Will let you know after the fix.

JoeMurray · 2018-12-04T19:25:52Z

@monishdeb could you update here?

monishdeb · 2018-12-04T21:21:08Z

@kcristiano @JoeMurray sorry for the delay. I have fixed the issue on refund and updated this PR Here's a CSV file of financial entries created after doing a refund on event fee change - Financial_Transactions_2_20181205023626.csv
In the CSV:

The first two rows show the respective entries of ticket 1($15x1) and ticket2 ($10x2) created during event registration.
Later via 'Change Fee selections page' - ticket 2 quantity is reduced to 1 and the 3rd entry in the CSV indicate this change of pending refund to -$10.
Then on paying the refund of $10 via 'Record Refund' page creates two proportional entries of $0 and -$10 for respective tickets. The fourth and fifth row in the CSV represent those two entries.

Please have a look.

kcristiano · 2018-12-16T14:44:18Z

@monishdeb I have applied the patch and tested again.

I am still not getting the desired results. The refund is allocating across all financial Types in the transaction

Removing an event option (decrease):

Original Export

After Edit

Adding an event option (decrease)

Original Export Simple

After adding option

monishdeb · 2018-12-16T15:47:04Z

Hi @kcristiano, thanks for your feedback. I have tried again with decreasing the event ticket by choosing the lesser amount. And I did not found the proportional allocation of the amount across financial types. Here's a screencast, which shows the database entries, esp. the financial item records where after refund only one financial record receive the refund (-$15 here) and other $0:

Can you please help me replicating the refund scenario, by mentioning the steps? Please feel free to ping me (alias @Monish) in mattermost.

kcristiano · 2018-12-16T15:54:39Z

The difference I can see is that I am changing the options (Price set line items) by selecting/deselecting and not changing quantities.

I can provide a copy of the database(s) I am suing to test. Ping me if you want a download link.

monishdeb · 2018-12-16T16:20:11Z

Oh ok, I have tried this scenario now. In this case, I have changed the ticket to from $100 to $50. And then refunded. I can see now three financial transactions where the 2nd item is the reversal of 1st ticket:

kcristiano · 2018-12-16T16:23:55Z

Thank you for continuing to test @monishdeb let me know if there is anything else I can share to help you troubleshoot.

We are using Price sets with multiple items. The first item is the event registration. the rest are 'add-ons' which will have different financial types. They are part of the event and changing these (adding/removing) causes the issue.

monishdeb · 2018-12-16T16:30:45Z

@kcristiano thanks for your support. I'll look into this issue and will update this PR. However I was thinking, wouldn't it be appropriate if there is a separate sub ticket in gitlab? because the original gitlab ticket is about fixing the incorrect proportional allocation of amounts after doing partial payment. And the new issue which we came to know here is a similar issue with the financial transaction after doing refund payment. So should I create a separate sub-ticket and therefore submit a new PR which will dependent on this PR to keep things simple and it would easier for core team to review?

kcristiano · 2018-12-16T16:38:13Z

@monishdeb I don't think that https://lab.civicrm.org/dev/financial/issues/34 which is incorrect allocation is linked to partial payments at all. At least not my reading of this issue. The problems we have seen are due to allocations of edited items, not partial payments.

monishdeb · 2018-12-16T18:56:22Z

My apologies if I am drawing the current issue into respective separate tasks based on different use-cases. As in https://lab.civicrm.org/dev/financial/issues/34 explain a use-case where, after increasing the quantity of an item, changes the original completed contribution to a partial payment, completing which leads to incorrect allocation of payments to multiple transactions. On further testing, we found another use-case where on decreasing/changing the item to a lesser amount also leads to the same issue (after paying refund). I will treat those as a common issue which is about incorrect allocations of payment after fee-change. Will let you know after I fix that last issue. Thanks!

kcristiano · 2018-12-17T13:06:46Z

@monishdeb Thank you for the further testing. The issue we had did not involve a partial payment. I do see where this issue is speaking to an incorrect allocation after the contribution changes to a partial payment status. I agree that these are a common issue and based on the testing we've both done, the allocation occurs regardless of the partial payment or refund pending status.

kcristiano · 2018-12-24T17:11:36Z

@monishdeb have you been able to replicate the issue we have? Please let us know if there is anything we can do to help with this.

monishdeb · 2019-01-02T04:14:33Z

Sorry @kcristiano for the delay (as I was on vacation). I have updated the PR, can you please check now?

seamuslee001 · 2019-01-02T05:42:35Z

@monishdeb test fails look related https://test.civicrm.org/job/CiviCRM-Core-PR/23833/testReport/junit/(root)/CRM_Event_Form_ParticipantTest/testPaymentAllocationOnMultiLineItemEvent/ and https://test.civicrm.org/job/CiviCRM-Core-PR/23833/testReport/junit/(root)/api_v3_PaymentTest/testUpdatePayment/

kcristiano · 2019-01-02T13:04:08Z

@monishdeb Thank You - I will test the functionality and report back here. I do agree that the above test failure will need to be addressed. But I'll focus on the functionality.

kcristiano · 2019-01-04T16:13:47Z

@monishdeb I just applied the patch and attempted to remove a line item.

Rather than affect only the line item removed, the amounts were prorated.

Steps:

Register for an event with multiple options and a price set
Cancel one item
edit contribution

monishdeb · 2019-03-20T13:00:16Z

@kcristiano I have rebased this PR. Waiting for CT to merge it as the test build passed earlier.

kcristiano · 2019-03-20T15:23:15Z

Thanks @monishdeb Can you check the current test failure?

monishdeb · 2019-04-05T15:27:06Z

Jenkins test this please

mlutfy · 2019-04-05T16:20:13Z

This PR is causing some pretty weird errors with the tests: https://chat.civicrm.org/civicrm/pl/z1pwd9rhrjrydp8ta8senmhfta

mlutfy · 2019-04-05T16:21:16Z

CRM/Core/BAO/FinancialTrxn.php

@@ -726,8 +726,8 @@ public static function updateFinancialAccountsOnPaymentInstrumentChange($inputPa
      $trxnParams = [
        'total_amount' => $trxn->total_amount,
        'contribution_id' => $currentContribution->id,
-      ];
-      CRM_Contribute_BAO_Contribution::assignProportionalLineItems($trxnParams, $trxn->id, $prevContribution->total_amount);
+      );


@monishdeb Syntax error ^

mlutfy · 2019-04-05T16:21:29Z

CRM/Financial/Form/PaymentEdit.php

-        ];
-        $contributionTotalAmount = CRM_Core_DAO::getFieldValue('CRM_Contribute_BAO_Contribution', $this->_contributionID, 'total_amount');
-        CRM_Contribute_BAO_Contribution::assignProportionalLineItems($trxnParams, $financialTrxn['id'], $contributionTotalAmount);
+        );


…ine item event registration

eileenmcnaughton · 2019-04-06T22:49:01Z

The reason I've been staying away from this PR is the addPayments function seems like 'yet another way to add payments to a contribution that is inconsistent with other methods'. I'm not reviewing any work around payments /partial payments / financials until we resolve consolidating the back office form payment add with the api payment.create (currently stalled on this PR #13689 )

I would think after that we'd want to try to get addPayments to call payment.create

monishdeb · 2019-04-25T11:21:52Z

Jenkins test this please

kcristiano · 2019-05-16T16:47:48Z

@eileenmcnaughton as #13689 has been merged, what is the status of this PR.

We've had this in production for about 3 months (5.10.x) and it's critical to our clients.

kcristiano · 2019-06-13T18:43:33Z

@eileenmcnaughton Just updating here based on our conversation.

The issue we have with clients is that with a price set where Financial Types/Accounts are different for each line item (which seems to be common for our use cases) the following happens on editing say an event registration

User needs to cancel one line item, but not participation in the vent
We cancel this via the UI - change selections for the event or cancel for the line item
Instead of having the accounting only affect the one line item (as it should) the amount refunded/cancelled) is pro rated against all line items

Obviously this PR must now be rebased, but beyond that, what needs to be done to be merge ready?

JoeMurray · 2019-06-13T18:44:39Z

We're highly interested in moving this forward.

martinh-pw · 2019-06-13T19:19:12Z

Very interested on our end as well :)

eileenmcnaughton · 2019-06-22T03:56:00Z

This has some content from this PR (ie from the test) #14589

kcristiano · 2019-07-05T00:10:58Z

@JoeMurray The changes through #14589 (comment) have made this PR "unmergable" #14589 moves us forward. I think that we may want to look at what needs to be done based on that PR in order to move this forward

eileenmcnaughton · 2019-07-05T00:13:28Z

@kcristiano @JoeMurray I think the related changes have got a clear path through to completion - I've been chipping away at . it - but only as fast as I've been able to get review on the pieces along the way - but basically we get Payment.create right and then call that from the additional payment form

eileenmcnaughton · 2019-07-12T00:07:01Z

Current movement on this issue is in #14763 - agreed with @kcristiano to close this one

civibot bot added the master label Nov 16, 2018

monishdeb force-pushed the dev-core-310 branch from 4e59d09 to 61feab9 Compare November 19, 2018 13:42

monishdeb changed the title ~~[WIP] dev/financial#34 : Incorrect allocation of payment on an edited multi-line item event registration~~ dev/financial#34 : Incorrect allocation of payment on an edited multi-line item event registration Nov 19, 2018

monishdeb force-pushed the dev-core-310 branch from 61feab9 to 1db6762 Compare November 19, 2018 14:28

monishdeb force-pushed the dev-core-310 branch 3 times, most recently from 3f07d19 to 20b31bb Compare November 21, 2018 08:03

monishdeb force-pushed the dev-core-310 branch from 20b31bb to fc83bdb Compare December 4, 2018 21:09

monishdeb force-pushed the dev-core-310 branch 2 times, most recently from 0ea6fb8 to 8d9ba8d Compare January 2, 2019 04:12

monishdeb force-pushed the dev-core-310 branch 4 times, most recently from 0831a6f to f5a6506 Compare April 5, 2019 13:21

mlutfy reviewed Apr 5, 2019

View reviewed changes

monishdeb force-pushed the dev-core-310 branch from f5a6506 to 4cc11d5 Compare April 5, 2019 17:46

monishdeb added 2 commits April 6, 2019 16:14

(dev/core#310) : Incorrect allocation of payment on an edited multi-l…

18eb5cf

…ine item event registration

Add more unit tests

68784fb

monishdeb force-pushed the dev-core-310 branch from 4cc11d5 to 68784fb Compare April 6, 2019 10:45

eileenmcnaughton mentioned this pull request Jun 5, 2019

Fix proportional test to test Payment.create & for the test to make more sense #14436

Merged

kcristiano mentioned this pull request Jul 2, 2019

Fix creation of additional zero value line item when changing fee selection in edge case #14589

Merged

eileenmcnaughton closed this Jul 12, 2019

monishdeb mentioned this pull request Sep 3, 2019

dev/financial#34 Fix line allocation in Payment.create #14763

Merged

dev/financial#34 : Incorrect allocation of payment on an edited multi-line item event registration #13114

dev/financial#34 : Incorrect allocation of payment on an edited multi-line item event registration #13114

Conversation

monishdeb commented Nov 16, 2018 • edited by JoeMurray Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Nov 16, 2018

JoeMurray commented Nov 19, 2018

kcristiano commented Nov 28, 2018

monishdeb commented Nov 28, 2018

JoeMurray commented Dec 4, 2018

monishdeb commented Dec 4, 2018 • edited Loading

kcristiano commented Dec 16, 2018

monishdeb commented Dec 16, 2018

kcristiano commented Dec 16, 2018

monishdeb commented Dec 16, 2018

kcristiano commented Dec 16, 2018

monishdeb commented Dec 16, 2018 • edited Loading

kcristiano commented Dec 16, 2018

monishdeb commented Dec 16, 2018 • edited Loading

kcristiano commented Dec 17, 2018

kcristiano commented Dec 24, 2018

monishdeb commented Jan 2, 2019

seamuslee001 commented Jan 2, 2019

kcristiano commented Jan 2, 2019

kcristiano commented Jan 4, 2019

monishdeb commented Mar 20, 2019

kcristiano commented Mar 20, 2019

monishdeb commented Apr 5, 2019

mlutfy commented Apr 5, 2019

mlutfy Apr 5, 2019

Choose a reason for hiding this comment

mlutfy Apr 5, 2019

Choose a reason for hiding this comment

eileenmcnaughton commented Apr 6, 2019 • edited Loading

monishdeb commented Apr 25, 2019

kcristiano commented May 16, 2019

kcristiano commented Jun 13, 2019

JoeMurray commented Jun 13, 2019

martinh-pw commented Jun 13, 2019

eileenmcnaughton commented Jun 22, 2019

kcristiano commented Jul 5, 2019

eileenmcnaughton commented Jul 5, 2019

eileenmcnaughton commented Jul 12, 2019

monishdeb commented Nov 16, 2018 •

edited by JoeMurray

Loading

monishdeb commented Dec 4, 2018 •

edited

Loading

monishdeb commented Dec 16, 2018 •

edited

Loading

monishdeb commented Dec 16, 2018 •

edited

Loading

eileenmcnaughton commented Apr 6, 2019 •

edited

Loading