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

eileenmcnaughton · 2019-07-09T11:29:25Z

Overview

Fixes the Payment.create api to correctly allocate payments to the right line items.

Note that this is not exposed in the UI without merging #14408 (which would make the additional payment form call payment.create and it does not apply to payments that update the status from Pending to Completed without this #14673

However, I think it gets us close to having the right code in this path.

Before

Create a price set with 2 text fields.
create an event registration
Edit the fees for the event registration such that one field now has a greater quantity & it is now 'Partially Paid'
add a payment via the api - this creates 3 extra EntityFinancialTrxn records

After

As above but only one EntityFinancialTrxn record is created when the Payment.create api is used to add the payment

Technical Details

This fix takes the approach of determining what % of the outstanding balance has been paid and then for each line item determinine the outstanding balance and multiplying it by that ration to assign 'allocation' field in the line item array. This is the amount used to allocate the line item between financial items. In the above example 100% of the balance is paid so 100%
of the line item is allocated

I don't hold any illusions this is the end of the line for locking down line item allocation. Once Merged I think we should

rebase & retest Switch recordAdditionalPayment fully over to api #14408 - that will cause a swag more tests to pass through this code and is likely to expose more
rebase & retest Further work on payment.create consolidation - always handle financials from payment.create #14673 - which will also extend test coverage
figure out the inconsistency (heavily commented in this PR) between the 2 methods used to getLastFinancialItem in payment.create and resolve on one, consolidate the 2 code paths
I haven't attempted to fix the sales tax part of the allocation here - if the above does not expose that we need to write a test & fix
extend test coverage to address when the payment is a refund rather than a payment and also what happens if the payment is an overpayment. Possibly by going through https://github.com/civicrm/civicrm-core/pull/13114/files#diff-8f648ba27ad1578ef45b6004ccf8a39b & pulling across what we can (the test this is based on came from there)

Comments

@monishdeb @kcristiano

civibot · 2019-07-09T11:29: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).

eileenmcnaughton · 2019-07-11T09:51:51Z

Test fail relates but from the one I looked at I think the test might be setting it up wrong

eileenmcnaughton · 2019-08-02T21:51:54Z

This is currently blocked on clarifying correct behaviour when line items are overpaid - from #14408 (comment)

I've encountered a specific issue with the financial items - namely what should happen when the payment exceeds the line items total. This can happen when
a) the price set selections are altered, downwards and
b) an overpayment is received.
b) A payment is updated - in effect this means it is cancelled & a new financial_trxn created

This means that the line item total is less than the contribution total

There are 3 options

the overage is allocated proportionally across the line items - so that the amount paid is equal to the sum of the financial items for line items for the given contribution or
the overage is not allocated and the sum of the financial items for the line items for the contribution is less than the amount paid against the contribution
we continue to do different things in different places

The former is locked in & tested in UpdatePaymentTest whereas the tests @monishdeb wrote for dev/core#310 assume the latter is the case

One for @JoeMurray @monishdeb @pradpnayak to have thoughts on...

kcristiano · 2019-08-28T12:15:46Z

@eileenmcnaughton Now that the related PRs are merged, what can I do to move this PR forward?

eileenmcnaughton · 2019-08-28T20:57:25Z

@kcristiano definite progress - I still have questions on whether this is right & i should change the failing test to match it - I annotated the test here #15143

Fundamentally the question is 'what is the point of the financial items' - I had understood them as being about linking payments to the things they are paying. However, as we see in that test it is allocating the intention to pay and SOME of the payments - I feel like I still need some guidance from @JoeMurray & @lcdservices as to what we are seeking to achieve here - I did get some but when I tried to act off it I still found gaps between what I felt like they were saying and what I was seeing 'locked in'

kcristiano · 2019-08-28T21:36:03Z

Thanks Eileen. I will do a run test and see what happens in the use cases that have given us issues.

monishdeb · 2019-08-30T13:24:03Z

So here's my test result. I have configured a price-set for Contribution with 3 price fields as Text -$1, radio (a-$1, b-$2,c-$3), checkbox (c1-$10, c2-$20) and this is how it behaves on partial payment and refund.

PARTIAL PAYMENT:

Do a contribution with price options : Text - $1, radio- a, checkbox - c1 and c2. Contribution total - $32
On edit contribution, using line-item . editor, increase the Text quantity to 5 from 1 and save.
Contribution status updates to Partially Paid, owed amount is $4 and contribution total is $36 4. Record partial payment.
After recording partial payment, a new financial item entry of $4 amount is made and is linked to corresponding financial trxn via entity_financial_trxn mapping.
Result - Correct behavior

REFUND

Do a contribution with price options : Text - $5, radio- a, checkbox - c1 and c2. Contribution total - $36
On edit contribution, using line-item . editor, decrease the Text quantity to 1 from 5 and save.
Contribution status updates to Pending Refund and refund amount is -$4. Record refund
After recording partial payment, a 5 new financial item entry of proportionate amount are created and is linked to corresponding financial trxn via entity_financial_trxn mapping. Also its strange that financial item labels are empty.

Here it should create the 4 FI entries with respective proportionate amount (ID between 189 to 192)

Result - Wrong behavior
Expected - There should be single financial item to record the refund amount -$4 like we recording the overage amount in case of partial payment.

@eileenmcnaughton @kcristiano

kcristiano · 2019-08-30T14:12:46Z

Thanks @monishdeb I'll take a look as well, but the data you have shows we haven't gotten this solved yet

eileenmcnaughton · 2019-08-30T23:13:24Z

@monishdeb this PR doesn't touch what happens in the refund - they use separate code - so I'd like to worry about that as a follow up.

A more specific blocking issue for this PR is that I can't get the api_v3_PaymentTest::testUpdatePayment to pass without changing assumptions - either in that test or in this PR (other fails are not notices etc)

I added some comments here

#15143

but basically this PR assumes that all PAYMENTS are allocated via financial items to line items. The test assumes that intended payments are allocated (ie accounts receivable transactions). If that is correct it complicates things but it also means I fundamentally don't understand what's going on / what's being achieved here & need that documented before this can be resolved.

The complexity of the test is how it sets up the original transaction - it is done with some magic code that handles a part payment at the time of creating the contribution - I don't know where the UI calls it & I discourage the approach & encourage creating a pending payment and THEN adding a payment (partial or otherwise)

pradpnayak · 2019-09-02T20:54:29Z

CRM/Financial/BAO/Payment.php

+        $entityFinancialTrxns = civicrm_api3('EntityFinancialTrxn', 'get', [
+          'entity_table' => 'civicrm_financial_item',
+          'entity_id' => ['IN' => array_keys($financialItems)],
+        ])['values'];


@eileenmcnaughton not all data stored in EntityFinancialTrxn are stored as paid some are pending incase of 'Accounts receivable' so probably you may need to join civicrm_financial_trxn table to check is_payment = 1 (i.e 'financial_trxn_id.is_payment' => 1)

Do we need to set limit => 0?

Yes - well spotted - fixed

hmm. is_payment is also set true for refunds. I'm going to play around with output on different use cases.

pradpnayak · 2019-09-02T20:55:01Z

CRM/Financial/BAO/Payment.php

+      $financialItems = civicrm_api3('FinancialItem', 'get', [
+        'entity_id' => $lineItemID,
+        'entity_table' => 'civicrm_line_item',
+        'options' => ['sort' => 'id DESC'],


Do we need to set limit => 0?

Yes - fixed

eileenmcnaughton · 2019-09-03T07:58:36Z

@pradpnayak thanks for your input - making the change you suggested seems to have solved my updatePaymentTest problem. It DOES mean we now wind up with 4 EntityFinancialTrxns in the end rather than 3 - I'm assuming you are saying that is a good thing.

If this passes tests can you or @kcristiano or @monishdeb do some tests to check the (slightly changed) results.

Note the handling of refunds is unchanged and out of scope

monishdeb · 2019-09-03T08:59:56Z

Jenkins test this please

monishdeb · 2019-09-03T09:05:50Z

The latest patch doesn't change any behavior, and matches with my earlier test results, where it resolves the partial payment financial handling and refund is still broken. But then it's not in the current scope of this PR so I am happy with the code fix. Am afraid there will be related test failures because now we are not proportionately assigning amounts to line-items in case of partial payment. Also, it seems bookeeping report is not displaying the result correctly.

@eileenmcnaughton have you checked the bookeeping report why the query is behaving strangely after this change?

eileenmcnaughton · 2019-09-03T09:30:34Z

@monishdeb no -- this change DOES change something - ie I added this line

https://github.com/civicrm/civicrm-core/pull/14763/files#diff-8967f6e57d6209aa277ded6512413608R613

On @pradpnayak's suggestion

Neither " Am afraid there will be related test failures because now we are not proportionately assigning amounts to line-items in case of partial payment" or "Also, it seems bookeeping report is not displaying the result correctly" should be the case - can you elaborate?

monishdeb · 2019-09-03T09:56:17Z

@eileenmcnaughton here's a screencast which show the financial adjustments in bookeeping report after doing partial payment. You will find Financial Type is missing against the adjusted amount $4.

In addition it will also cause some issue in Financial Batch Export. @kcristiano can you please help me to provide results of batch export for partial payment?

eileenmcnaughton · 2019-09-03T09:59:14Z

@monishdeb is this different to before I made that last change suggesed by Pradeep? Can you see what entry is missing / wrong to cause it?

eileenmcnaughton · 2019-09-03T10:39:08Z

OK - I see that without the change recommended by @pradpnayak but not with it.

However, I still see 2 test fails & am still stuck on what the logic should be. It's probably better discussed over on #15143 though as @pradpnayak has already commented there & resolving that should clarify what needs to happen here

monishdeb · 2019-09-03T10:58:15Z

@eileenmcnaughton I went through the same test issues and some oddity in financial batch export result. In order to fulfill every requirement, I need to follow back the proportional line-item allocation when there is a partial or refund payment. And this PR #13114 has all the answers. I was wondering if we can use the logic in this patch to adjust the financial records.

eileenmcnaughton · 2019-09-03T11:00:40Z

Gawd - I had my head around this when I did it but it's months ago now - trying to get my head into it again

monishdeb · 2019-09-03T11:09:34Z

Yeah :( Should I submit a new PR where I will cherry-pick your commits and then change the code adopt the #13114 changes? which was earlier tested by @kcristiano @pradpnayak
.

eileenmcnaughton · 2019-10-22T21:23:04Z

@kcristiano yeah keep this open a little longer. I don't like people using the PR queue as their personal to do list & keeping this open kinda flies in the face of that - but on the other hand this is still active & high priority (even if it doesn't feel like it). I'm going to try a bit more on the docs - the issue is really that with docs being poor shared understanding / expectations are poor.

Unfortunately I think in some ways some community issues a couple of years dampened docs efforts & related comms & this is in some ways a fallout

magnolia61 · 2019-10-22T21:30:53Z

I do not want to spam here (as you want to pause this issue) but would https://lab.civicrm.org/dev/core/issues/375 be related? Also a consequence of different financial type line items. Apologies but if this is totally something different. In that case I am glad to delete this comment.

kcristiano · 2019-10-22T21:59:20Z

@magnolia61 I commented at https://lab.civicrm.org/dev/core/issues/375 and will take a look, it does look similar.

eileenmcnaughton · 2019-10-25T00:21:23Z

@kcristiano @JoeMurray I've added a section on financial items here civicrm/civicrm-dev-docs#719 - I think it makes them relatively clear (I hope) - it also finally documents the schema issue that has been causing us problems - I'll add a gitlab for that too

eileenmcnaughton · 2019-10-25T03:32:35Z

I've just rebased this - and tested the following steps as described by @monishdeb above

create a fully paid event contribution with a mix of text & option values ie

Text box with quantity $1
option group with radio box - values 1,2,3
option group with checkbox - 10, 20

edit the value for the text box from $1 to 5 using edit participant fees
add a partial payment for the remaining $4

The outcome actually looks OK to me

The code has changed quite a bit in the meantime which could explain the difference (I did also use the code functionality not the lineitemeditor to change the line item)

@kcristiano my feeling is that if your testing shows this as fixing a problem without creating a new one we should merge it based on your testing. If it fixes a problem but leaves another one outstanding then we should create a follow up with instructions to replicate.

This DOES lock in the fix with a test (originally written by @monishdeb )

kcristiano · 2019-10-26T17:22:33Z

@eileenmcnaughton Thanks for the work.

Test failure looks related but I did a number or r-run anyway.

On the demo setup (buildkit master) i had the same results you did.

I also took the patch and applied to 5.18.4 and used a DB that has rather complex events (11 different Financial Types across over 25 line items).

The only issue I found is that when adding the second payment the received date changed on the original item.

As many of our clients use cash basis accounting, they rely on the receive date.

This is an improvement.

As far as steps to replicate the change in receive date, the only difference is on the initial registration, use a date in the past and then when changing the selections use the current date.

eileenmcnaughton · 2019-10-26T21:26:57Z

argh the tests - now I remember - will have to revist them

After ongoing issues resolving civicrm#14763 I have concluded the underlying issue on the failing tests is the test setup. Specifically the use of 'partial_amount_to_pay' does not create the correct underlying entities - it 'sort of' creates a payment without linking it to the financial items. These parameters are part of our first attempt at partial payments. I am removing them from the test here but will deprecate later from other places in the code

eileenmcnaughton · 2019-10-26T23:37:57Z

After re-digging I've concluded the tests are flawed - in not using the order api & instead using the 'partial_amount_to_pay' parameter they are miscreating the initial order #15620

After ongoing issues resolving civicrm#14763 I have concluded the underlying issue on the failing tests is the test setup. Specifically the use of 'partial_amount_to_pay' does not create the correct underlying entities - it 'sort of' creates a payment without linking it to the financial items. These parameters are part of our first attempt at partial payments. I am removing them from the test here but will deprecate later from other places in the code

eileenmcnaughton · 2019-10-26T23:42:58Z

This is the other cause of failures #15621

eileenmcnaughton · 2019-10-26T23:46:32Z

Further docs updates here civicrm/civicrm-dev-docs#722

After ongoing issues resolving civicrm#14763 I have concluded the underlying issue on the failing tests is the test setup. Specifically the use of 'partial_amount_to_pay' does not create the correct underlying entities - it 'sort of' creates a payment without linking it to the financial items. These parameters are part of our first attempt at partial payments. I am removing them from the test here but will deprecate later from other places in the code

Cast result of getContributionBalance to float to match comment block. Without this it is possible for it to return 'null' which is not useful / in keeping. I did a bit of an audit & did not find places where this would cause an === comparison to fail

eileenmcnaughton · 2019-10-27T23:00:38Z

OK - the current blocker on this is the fact the Update is not correctly creating the entity_financial_trxn records for the negative payment / cancel & the tests were missing that because of the way the tests were written :-( I've added #15630 which will fix the cancel action but not address the refund action as yet

eileenmcnaughton · 2019-10-28T19:36:32Z

test this please

eileenmcnaughton · 2019-10-28T22:16:30Z

@kcristiano so this is finally passing!

I think it's OK to merge this if we figure out the remaining gaps & log issues over on financial - https://lab.civicrm.org/dev/financial/issues

I think the gaps are

something to do with receive_date
refunds are not allocating proportionally
what happens when sales taxes are in play?
what happens when fees are in play

kcristiano · 2019-10-29T18:42:21Z

@eileenmcnaughton I agree this is ready to merge.

I ran another set of tests and this is a big improvement.

I agree the gaps 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 (image A and Report A )
test with taxes
test with live Payment processor to reflect fees

I'll open an issue with these items.

Image A

Report A
Report_20191029-1047.zip

kcristiano · 2019-10-29T18:57:05Z

https://lab.civicrm.org/dev/financial/issues/94

eileenmcnaughton · 2019-10-29T19:07:15Z

@kcristiano thanks I'll merge this & we'll look at the others - 3 in the list seems like the highest priority - can you write steps to replicate

kcristiano · 2019-10-29T19:55:27Z

Steps to replicate in https://lab.civicrm.org/dev/financial/issues/94 (fixed link)

eileenmcnaughton · 2019-10-29T19:56:33Z

link goes the wrong place - https://lab.civicrm.org/dev/financial/issues/94

civibot bot added the master label Jul 9, 2019

eileenmcnaughton force-pushed the payment_3 branch from dda2e58 to ac78942 Compare July 9, 2019 11:31

eileenmcnaughton mentioned this pull request Jul 12, 2019

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

Closed

kcristiano mentioned this pull request Jul 16, 2019

Switch recordAdditionalPayment fully over to api #14408

Merged

eileenmcnaughton added the has-test label Aug 2, 2019

eileenmcnaughton force-pushed the payment_3 branch from ac78942 to db7a8dd Compare August 27, 2019 06:31

eileenmcnaughton mentioned this pull request Aug 28, 2019

dev/financial#40 add missing financial item when altering a radio amount #14892

Merged

pradpnayak reviewed Sep 2, 2019

View reviewed changes

eileenmcnaughton force-pushed the payment_3 branch 2 times, most recently from 2d691b8 to d902e31 Compare September 3, 2019 07:54

eileenmcnaughton force-pushed the payment_3 branch from d902e31 to 8881981 Compare October 25, 2019 03:14

eileenmcnaughton force-pushed the payment_3 branch from 8881981 to 4f8f2d2 Compare October 26, 2019 23:06

eileenmcnaughton mentioned this pull request Oct 26, 2019

Fix test setup Function to use order api #15620

Merged

eileenmcnaughton force-pushed the payment_3 branch from 4f8f2d2 to d985845 Compare October 27, 2019 04:26

Extract code to calculate how much of a line item has been paid

4f6e08d

eileenmcnaughton force-pushed the payment_3 branch from d985845 to 4f6e08d Compare October 27, 2019 04:45

eileenmcnaughton merged commit 10cde98 into civicrm:master Oct 29, 2019

eileenmcnaughton deleted the payment_3 branch October 29, 2019 19:07

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

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

Conversation

eileenmcnaughton commented Jul 9, 2019

Overview

Before

After

Technical Details

Comments

civibot bot commented Jul 9, 2019

eileenmcnaughton commented Jul 11, 2019

eileenmcnaughton commented Aug 2, 2019

kcristiano commented Aug 28, 2019

eileenmcnaughton commented Aug 28, 2019

kcristiano commented Aug 28, 2019

monishdeb commented Aug 30, 2019 • edited Loading

kcristiano commented Aug 30, 2019

eileenmcnaughton commented Aug 30, 2019 • edited Loading

pradpnayak Sep 2, 2019

Choose a reason for hiding this comment

pradpnayak Sep 2, 2019

Choose a reason for hiding this comment

eileenmcnaughton Sep 3, 2019

Choose a reason for hiding this comment

JoeMurray Sep 11, 2019

Choose a reason for hiding this comment

pradpnayak Sep 2, 2019

Choose a reason for hiding this comment

eileenmcnaughton Sep 3, 2019

Choose a reason for hiding this comment

eileenmcnaughton commented Sep 3, 2019

monishdeb commented Sep 3, 2019

monishdeb commented Sep 3, 2019

eileenmcnaughton commented Sep 3, 2019

monishdeb commented Sep 3, 2019

eileenmcnaughton commented Sep 3, 2019

eileenmcnaughton commented Sep 3, 2019

monishdeb commented Sep 3, 2019 • edited Loading

eileenmcnaughton commented Sep 3, 2019

monishdeb commented Sep 3, 2019

eileenmcnaughton commented Oct 22, 2019

magnolia61 commented Oct 22, 2019 • edited Loading

kcristiano commented Oct 22, 2019

eileenmcnaughton commented Oct 25, 2019

eileenmcnaughton commented Oct 25, 2019

kcristiano commented Oct 26, 2019

eileenmcnaughton commented Oct 26, 2019

eileenmcnaughton commented Oct 26, 2019

eileenmcnaughton commented Oct 26, 2019

eileenmcnaughton commented Oct 26, 2019

eileenmcnaughton commented Oct 27, 2019

eileenmcnaughton commented Oct 28, 2019

eileenmcnaughton commented Oct 28, 2019 • edited Loading

kcristiano commented Oct 29, 2019

kcristiano commented Oct 29, 2019

eileenmcnaughton commented Oct 29, 2019

kcristiano commented Oct 29, 2019 • edited Loading

eileenmcnaughton commented Oct 29, 2019

monishdeb commented Aug 30, 2019 •

edited

Loading

eileenmcnaughton commented Aug 30, 2019 •

edited

Loading

monishdeb commented Sep 3, 2019 •

edited

Loading

magnolia61 commented Oct 22, 2019 •

edited

Loading

eileenmcnaughton commented Oct 28, 2019 •

edited

Loading

kcristiano commented Oct 29, 2019 •

edited

Loading