CRM-21513: Change fee selection for text price field on backoffice Event registration Not Creating Correct Financial Items

[monishdeb]

Overview

Steps to replicate:

1. Create pending pay later event registration with text field price option say qty 1 x $10
2. Pending pay later contribution no showing any financial transactions as expected
3. Edit selections on event registration by changing qty to 3 (i.e. $30)
4. Pending pay later contribution STILL no showing any financial transactions as expected.
5. Edit selections on event registration by changing qty to 2 (i.e. $20)
6. Pending pay later contribution STILL no showing any financial transactions as expected.

Bug: There is only 1 financial item record that refers to previous line-item of $10 amount. However, financial trxn correctly records the surplus $10.
Expected: There should be 3 financial items refers to old line-item and other referers to the changes made in step 3 and 5. Please check the After screenshot.

Before

Only have 1 financial recrods

After

Technical Details

Added unit test to assert this scenario.

---

• CRM-21513: Change fee selection for text price field on backoffice Event registration Not Creating Correct Financial Items

[eileenmcnaughton]

test this please

[eileenmcnaughton]

@monishdeb I took a look at this but I guess it really is WIP as tests pass with or without the other changes :-)

[monishdeb]

@eileenmcnaughton the fix is complete but need to add comments, above my changes. Also, need to extend added UT, asserting against expected financial records :)

[eileenmcnaughton]

Ah ok - I'm not totally loving the fix - not that it's wrong but just that I'd prefer to take another step towards readability & this is a (slight) step backwards - but I guess given that the same code needs to be touched again for https://issues.civicrm.org/jira/browse/CRM-20676 I could let it through on this one

[eileenmcnaughton]

What do you think about changing Order.create api so that if it receives an id & a line items array it calls 'changeFeeSelections' - that would give us a more robust way to call the function

[monishdeb]

Excellent idea, will put some thoughts in this direction and will see what kind of adjustment it needs to call the changeFeeSelections. This fn is already handling some complex cases of membership and event fee-change so that would be cool to use the existing fn for Order APIs.

On otherhand, I knw the patch isn't clean enough, in terms of readbility, so will do whats needed. Also I think getReverseFinancialItemsToRecord name is not appropriate, may be getAdjustedFinancialItemsToRecord ?

[eileenmcnaughton]

I feel like the issue with the code (which is not caused by this patch) is that it still mixes up processes for no obvious reason - ie. the recordAdjustedAmt function should probably do all the financial trxn / financial item changes rather than some & some being in the parent function.

Also, I tried changing from one option value to an option value Plus a text value & that got wierder. I support further changes going into the rc in this area (I think 4.7.29 is cut)

[monishdeb]

@eileenmcnaughton I agree. I think we should first resolve all the issues on 1st round. Then on 2nd round we need to adjust Order API to use changeFeeSelections fn. And in 3rd around we need to refactor the fn to move all the financial trxn / financial item changes to recordAdjustedAmt fn. What do you think?

This PR is ready for review!

[eileenmcnaughton]

I'm merging this because after testing & code review I agree it takes us forwards, & locks it in with a test. I'm still not convinced we have all the issues in this code resolved / covered by tests - but I think we should merge this & then move onto trying to replicate https://issues.civicrm.org/jira/browse/CRM-21414 & https://issues.civicrm.org/jira/browse/CRM-20676

[vakeesan26]

when we doing change selection and then record payment it is creating two completed financial transactions. which is affecting the Accounting Batch.

[eileenmcnaughton]

let's track that in gitlab https://lab.civicrm.org/dev/core/issues/309