Refactor price fields of package TPC

alexkuc commented 4 months ago

Scope of Work

This PR touches only price fields of the package eventespresso/tpc. Everything else inside that package is left as-is. The nature of this refactoring is 3-fold:

simplify structure of React components
address types that were previously silenced using @ts-ignore directive
become more familiar with the codebase

Additionally, I have enabled option argsIgnorePattern for ESLint rule no-unused-vars. This allows to skip or ignore properties when destructing an object. The following is supported and will not raise linter errors:

const object = {a: 1, b: 'string', c: false} as const;

const {a: ignore, ...rest} = object;
const {a: Ignore, ...rest} = object;
const {a: ignored, ...rest} = object;
const {a: Ignored, ...rest} = object;

Testing Notes

As this PR has updated the codebase for the Ticket Price Calculator fields (screenshot above), please check that the above fields behave as the did previously without any changes or similar.

General Points

please check all price types which are available under column Price Type (becomes available after you click on _any plus sign)
please set the label, description and the field's value accordingly
please check calculations from top to bottom (chevron icon points down) and from bottom to top (chevron icon points up)

Checklist

[ ] money fields to use 6 decimal places except for the field Total
[ ] the math behind calculations to remain the same i.e. as previously
[ ] fields to retain their value after saving the ticket (including page reload)
[ ] trash icon to function as expect
[ ] plus icon to function as expect
[ ] button Remove taxes to function as expect
[ ] button Delete all prices to function as expect
[ ] button for reversing direction of calculations (chevron icon) to function as expected

Details

Before PR

After PR

The most important change I believe is the correct enforcement of types. The component Factory based on internal* property _type will correct infer the HTML attributes and their associated types:

As you can see, even though both attributes are onChange, in the first case it comes from SelectHTMLAttributes while in the later case it comes from InputHTMLAttributes.

* internal meaning that in the final DOM the property _type will not be shown

alexkuc commented 4 months ago

Locally I have tested the following:

linter… ✅
unit tests… ✅
build assets… ✅
run end-to-end tests… ✅

alexkuc commented 4 months ago

@tn3rb In regards to the order of props, I'll update them manually now but will open a separate issue for ESLint to detect that and potentially fix it automatically - https://github.com/eventespresso/barista/issues/1313

Saam01 commented 3 months ago

@alexkuc Can you please give us some instructions about how and what to test here?

alexkuc commented 3 months ago

@Saam01 In the original description, there is section Testing Notes. I'll move it to the top now to make it more visible.

Edit: updated

Edit2: moved a bit down to allow section Scope of Work to be first

kingrio13 commented 3 months ago

General Points

[x] please check all price types which are available under column Price Type (becomes available after you click on _any plus sign)

all good

please set the label, description and the field's value accordingly

[x] please check calculations from top to bottom (chevron icon points down) and from bottom to top (chevron icon points up)

basing from Image above, computation is right.

@alexkuc since we already rounded this. to 2 digit, we should be using this rounded number when we add tickets during purchase?

I reported problem issue here, but will it be resolve in different PR? https://github.com/eventespresso/cafe/issues/1216

Checklist

[x] money fields to use 6 decimal places except for the field Total

All good

[x] the math behind calculations to remain the same i.e. as previously
[x] fields to retain their value after saving the ticket (including page reload)
[x] trash icon to function as expected
[x] plus icon to function as expected
[x] button Remove taxes to function as expected
[x] button Delete all prices to function as expected
[x] button for reversing direction of calculations (chevron icon) to function as expected

tn3rb commented 3 months ago

@kingrio13 re:

@alexkuc since we already rounded this. to 2 digit, we should be using this rounded number when we add tickets during purchase?

I reported problem issue here, but will it be resolve in different PR? https://github.com/eventespresso/cafe/issues/1216

that is an entirely different issue and will not be resolved by this PR. That rounding issue that you have reported a few times now is actually incredibly difficult to resolve because totals get calculated in different ways depending on what part of the process is occurring.

Saam01 commented 3 months ago

Testing Notes

As this PR has updated the codebase for the Ticket Price Calculator fields (screenshot above), please check that the above fields behave as the did previously without any changes or similar.

General Points

please check all price types which are available under column Price Type (becomes available after you click on _any plus sign)
please set the label, description and the field's value accordingly
please check calculations from top to bottom (chevron icon points down) and from bottom to top (chevron icon points up)

Checklist

[x] money fields to use 6 decimal places except for the field Total
[ ] the math behind calculations to remain the same i.e. as previously
[ ] fields to retain their value after saving the ticket (including page reload)
[x] trash icon to function as expect
[x] plus icon to function as expect
[x] button Remove taxes to function as expect
[x] button Delete all prices to function as expect
[ ] button for reversing direction of calculations (chevron icon) to function as expected

The total amount mismatches:

https://github.com/eventespresso/barista/assets/99984614/35294e34-fb24-4810-8586-c7e92b140c98

In the frontend the value also mismatches from the backend:

alexkuc commented 3 months ago

Thank you @kingrio13 and @Saam01 for the reports. @Saam01 I'll investigate the issue and let you know once I have something.

tn3rb commented 3 months ago

@Saam01

your onscreen calculation using the calculator is not correct

what the TPC is showing is correct:

price type	amount	runing total
base price	50	50.00
fed tax	15	57.50
$ surcharge	20	77.50
$ discount	10	67.50
% discount	5	64.125
reg tax	18	75.6675
total		75.67

it looks like you subtracted $5 for the 5% discount instead of subtracting $3.375

The difference on the frontend is because of a bug that has been reported several times already and is NOT part of this PR so @alexkuc plz disregard that aspect of @Saam01's feedback (the SPCO part)

alexkuc commented 3 months ago

@kingrio13 Are you finished with your testing? Is everything good? Just FYI, formatting for your comment broke.

@Saam01 Based on Brent's feedback, could you please continue with the rest of the testing points? Thank you

kingrio13 commented 3 months ago

@kingrio13 Are you finished with your testing? Is everything good? Just FYI, formatting for your comment broke.

@Saam01 Based on Brent's feedback, could you please continue with the rest of the testing points? Thank you

If i only based on the task list, all good in my end bud. @alexkuc

Saam01 commented 3 months ago

@tn3rb @alexkuc The price and price type seem to be working fine in the backend.

garthkoyle commented 3 months ago

I am unable to enter more than one digit in the bottom-up TCP (video): https://www.screencast.com/t/KRbvMLCJFu

To reproduce:

create a new ticket
Add a price e.g. 123
Change the price calculator FROM top down, TO bottom up.
I am unable to add more than one digit to the total to force the calcs to go from bottom up. For example, if you try to add the number 234, the field accepts the value 2, and then adds the decimal places and truncates any other numbers.

alexkuc commented 3 months ago

Going to fix this bug and open a new PR

alexkuc commented 3 months ago

Also, I'll look into making a checklist to improve testing scope and make it more standardised and comprehensive

alexkuc commented 3 months ago

Replicated locally, working on a fix

alexkuc commented 3 months ago

@tn3rb I'll bring the fix via #1319 so I believe this branch can be deleted now

eventespresso / barista