CRM-17182 Fix Search on fee_level_id and ensure that participant.fee_amount isn't updated when label changes

[seamuslee001]

Overview

This aims to fix the issue in CRM-17182 by filtering on civicrm_line_item.price_field_value_id rather than civicrm_participant.fee_amount and remove the code introduced originally to fix CRM-17182 which changed the fee_amount column details based on label changes in price field values

Before

fee_amount changed whenever label is changed and searching done based on a regex on the fee_amount column

After

searching based on the price_field_value id and the fee_amount column is not changed

thoughts @JoeMurray @eileenmcnaughton @jitendrapurohit

[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]

Jenkins re test this please

[seamuslee001]

Jenkins re test this please

[eileenmcnaughton]

I caught some of this discussion on mattermost @JoeMurray are you in agreement with this change?

[seamuslee001]

To Elaborate further in CRM-17182 they found the problem if you changed the price_field_value.label then the fee level filter in searches broke. That is because it was doing regex search on the fee_amount column. Thus Jitendra’s fix for that was to re-write the contents of the fee amount column to match up with the new label. This reversed that fix but changes the search filter so it takes the id that has been passed in and then looks up the relevant rows through the line item table rather than regex on fee_amount. I suspect in the 4.7.alpha process line items may not have been getting created. Maybe. So hence the search code as it was.

[jitendrapurohit]

Hi @seamuslee001 The fix submitted for CRM-17182, #6743 does not aim to change the amount value in any way, it updates the older labels in amount_level and fee_level column. So not sure what the above comment actually means. Maybe, I'm missing some more discussion or the PR link/mm chat? Can you pls mention the steps where the amount is changed on updating the label value?

Also, I've applied this PR to see if it breaks the basic req of CRM-17182 and see an additional issue as follow -

• Create a priceset and add a checkbox price field with option label as foo => Save.
• Click on edit link for the above pricefield option and update the label from foo to bar.
• Instead of updating the same option, it adds a new option value to the checkbox.
• If I try to edit the field again, a new option value is added.

[seamuslee001]

@jitendrapurohit Joe Murray's view (and Mine) is that the data in the amount_level and fee_label columns of civicrm_contribution and civicrm_participant should not change whenever the price_field_value label changes as that data is meant to represent the fees as they are charged. I'll look into that other part. Note i am not talking about changing the amount at all, this is just focusing on the labels that are stored in those respective columns. This is a link to the chat https://chat.civicrm.org/civicrm/pl/8den9xeuu7botkie5yrynz9chr

The problem in my view is that the label change then can cause confusion for people as they may not see exactly what fee level someone registered at.

[jitendrapurohit]

Thus Jitendra’s fix for that was to re-write the contents of the fee amount column to match up with the new label

ok, thought the fee amount column from the above line was directed towards the amount value.

The requirement of this PR looks like CRM-17182 is not actually an issue and want to revert the fix completely? I haven't gone into this requirement from the time of that fix in 2016.

Can you update the PR fixing the above problem and I can do a full review of this change and maybe, discuss more with the author of that issue @JKingsnorth @kurund @yashodha @monishdeb

[seamuslee001]

@jitendrapurohit sorry poor set of words there.

This PR fully reverts the original fix. However it puts in what I think is a more proper fix which is when you select a fee to search on. It now would join to the line_item table and filter on the price_field_value.id column on the line item table. The $value in the search forms is price_field_value ids

[eileenmcnaughton]

@jitendrapurohit does the above make sense to you? It still seems to meet your original need (CRM-17182 )

[jitendrapurohit]

@eileenmcnaughton @seamuslee001 I'm waiting for the issue mentioned in #15064 (comment) to be fixed and then I can probably do a complete review of this PR :).

[eileenmcnaughton]

@jitendrapurohit my understanding is that @seamuslee001 & @JoeMurray feel we should not be updating the values in civicrm_participant. fee_label but instead we should be searching on the price option value labels (which are what is changed over time)

[jitendrapurohit]

yes, I understand that. The issue mentioned in the comment is something which looks like an additional separate regression caused by the changes made here.

i.e, on updating a price option, a new duplicate row is added instead of updating the same option value. I've listed the steps to reproduce in the comment.

[seamuslee001]

@jitendrapurohit i have fixed it now

[jitendrapurohit]

Thanks for the update @seamuslee001

Tested by adding multiple price options => registering contacts to the events => updating price option labels => and confirm that the main and the above additional issue is fixed.

After the option value update, any participant registering will show the updated label value.

The older participant displays the original fee level on the view participant page which is an expected behavior as per the discussions above.

The search functionality(using normal & advanced form) with the updated label works fine. The older participants are displayed in the results correctly.

Search Builder provides a textbox for the fee level field, so we need to input the older label in order to retrieve the participants.

👍 to get this merged @eileenmcnaughton

[eileenmcnaughton]

thanks @jitendrapurohit !