dev/core#1049: Use FrontEndPaymentFormTrait to assign line items on Ev…

[agileware-fj]

… on Event Registration Confirm and ThankYou "forms"

Overview

We really want to see this in 5.15 :)

Code cleanup around tax assignments, following pattern on #13899 but for Event Confirmation and ThankYou pages
Agileware ref CIVICRM-1244

Before

Event Confirmation and ThankYou Forms are implementing their own line item assignments, causing inconsistency in line item display. This is most immediately obvious in the tax rate column when tax & invoicing is enabled

After

These Forms use the previously implemented trait to assign line items in a consistent manner.

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

[agileware-fj]

Also see #14562 for master PR

[agileware-fj]

Not sure if this will need testing re-trigged since I forgot to select the correct branch when submitting.

[eileenmcnaughton]

@agileware-fj this doesn't really count as a recent regression (which is the criteria for rc vs master) since it's about 9-12 months old as a regression. Was there a particular reason for this to go in the rc? (Note I also pinged @kcristiano for input - he is our main rc tester and one of our key concerns is not to invalidate his testing by making changes after he has tested)

[eileenmcnaughton]

I should also note from a review POV - the code looks good. I haven't done r-run as yet

[kcristiano]

@agileware-fj Are there steps to reproduce the issue? I'll rebuild 5.15 RC with this patch but I'd like to be able to quickly reproduce the issue.

[kcristiano]

@agileware-fj I do see the issue - it's only the sales tax rate that shows the extra decimals. I tested on 5.15 RC and the PR solves the issue.

Before Patch is applied

After Patch is applied

@eileenmcnaughton I did have one 'odd' issue where I could not print an invoice after applying this patch. However, I rebuilt 5.15 via buildkit and manually applied the patch. The 'issue' disappeared. I think it was unrelated, but will continue to test.

@jusfreeman Could we consider this for master to allow more time for testing? Is the sales tax display a recent regression?

[mattwire]

@agileware-fj I'm not sure this is something that should go in the RC as it's been like that in core for a long time so is definitely not a recent regression. It is something that might make sense for the ESR though.

[eileenmcnaughton]

@mattwire I think we talked about the ESR focussing on things that worked in 5.7 & don't in 5.13 - so this wouldn't qualify there either - let's focus on getting it fixed & tested in master then

The fix looks good

[agileware-fj]

Okay, I wasn't sure about the regression criteria. I'll close this one then; please direct all attention to #14562 against master, which is the same patch +/- line numbers, but against master.