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

[eileenmcnaughton]

Overview

Updated non-stale version of #13521 - fixes a missing line item when adjusting a radio line item - refer to that PR for more detail

Before

When adjusting a line item of radio type the changed item doesn't have an entity_financial_item record & line item

After

The above are created

Technical Details

This is largely the same as the original one but it addresses the mislinking of the financial item with the wrong trxn_id

Comments

I had some doubts about bringing this back to life since most work in this areas is stalled on @monishdeb availability at the moment. However, I was reluctant to throw away the tests. There is ALSO a larger question which stalled that PR & might stall this one which comes down to 'do a big audit' - I think this gets us to a better position (better test cover, imbalance fixed) without addressing some larger questions, but if it stalls again I'll close & we can track through gitlab.

Note that the behaviour when changing a text field line item is different to a radio field. For text fields there is an adjustment amount, for the others a reversal & a new row.

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

[seamuslee001]

@eileenmcnaughton jenkins doesn't like the style

[eileenmcnaughton]

thanks @seamuslee001 - cheered up now

[JoeMurray]

@monishdeb please spend up to 1h tomorrow on this, stealing as necessary from assigned projects.

Note that the behaviour when changing a text field line item is different to a radio field. For text fields there is an adjustment amount, for the others a reversal & a new row.

Please get rid of long obsoleted adjustment amount and make text fields consistent with the approved general pattern of reversal and new row.

[eileenmcnaughton]

Let's keep the change in behaviour for a follow up rather than try to scope creep this PR

[monishdeb]

Hi @eileenmcnaughton I found an issue on how entity_financial_trxns record register the change in amounts of radio options. Let me explain with an example.

1. Registered a event with radio option 'a' - $1
2. Changed fee selection and chosen option 'b' - $2
3. Recorded additional $1 amount.

Financial items: (3 records, including reversal amount - expected)

Financial trxns - (2 records, each with $1 amount - expected)

Entity Financial trxns - (4 records - wrong)

As per screenshot there is an additional record of $1 at the end (id - 255), which is wrong.

Can you please fix this issue and extend unit test to assert the entity_financial_trxns records for the adjustment made ?

[eileenmcnaughton]

@monishdeb I added a check to the test - but it's only recording 3 lines - not 4 like you say - are you saying this causes a regression in the scenario you tested - or that there is an additional broken scenario?

[monishdeb]

I found it as an additional broken scenario, as I checked in a clean setup. Ok on second thought I will let it go and will try to fix it in a followup PR, but strange that UT isn't able to capture this odd behavior in entity_financial_trxns records.

I am marking with MoP tag as I am happy with the final patch, also the added UTs captures the desired change-fee use-cases for radip price options, correctly.

[eileenmcnaughton]

thanks @monishdeb I think we can also merge #14408 now but I'm still struggling with the line item allocation #14763 since it fails a test but as I documented in #15143 the test doesn't really make sense to me in terms of final result

[monishdeb]

Jenkins test this please