Allow prices to be more precise

[joshfeck]

See https://events.codebasehq.com/projects/event-espresso/tickets/9414 for backstory

@mnelson4 The original branch got deleted from GitHub so I cut a new branch from master and re-applied the changes. Can you refresh the changes if needed, then request a review?

I think there are some good changes in here that will help many of the users of EE4 that need higher precision pricing and I don't think this should be held up until other price/tax/rounding issues get resolved.

Checklist

• [ ] I have read the documentation relating to systems affected by this pull request, see https://github.com/eventespresso/event-espresso-core/tree/master/docs
• [ ] User input is adequately validated and sanitized
• [ ] all publicly displayed strings are internationalized (usually using esc_html__(), see https://codex.wordpress.org/I18n_for_WordPress_Developers)
• [ ] My code is tested.
• [ ] My code follows the Event Espresso code style.
• [ ] My code has proper inline documentation.
• [ ] My code accounts for when the site is in Maintenance Mode (MM2 especially disallows usage of models)

[mnelson4]

ya kk! I'll do next time I pass attendee mover stuff off for review (unless you prefer I put that on hold for a bit to do this)

[joshfeck]

This can wait until the attendee importer is ready for its next review.

[mnelson4]

I noticed a few commits that got missed so added them here. (FYI I also found my original branch, I still had it locally, but it was conflicts all over with master, so I think we're best continuing with this branch).

The remaining issue, as before , is that the amounts are getting rounded on display in SPCO. Eg if you have two partial-penny price modifiers, they get rounded in ticket details and in the payment options step.

[mnelson4]

Summary

This allows price modifiers to be partial penny amounts, but not single ticket amounts. When displaying these partial penny amounts, we only show as many decimal places as needed (to a limit of 2 extra decimal places). This works better (and still works as gateways expect), but probably isn't satisfactory yet. We probably want to allow partial penny amounts on ticket amounts, and gateway code will just need to expect that we won't always be able to send an itemized list. Thoughts @joshfeck ?

Details

Here's $1 ticket with a 0.6% discount, 1% discount, and two half penny discounts. Here's how it looked before (notice that, according to how it was rounded, the total should be $0.96) and here's how it looks now (notice the extra precision only when necessary, although we don't go overboard, we add at most 2 extra decimal places).

Although here's a problem: TWO $1 tickets, 0.6& discount, 1% discount, and two half penny discounts: https://drive.google.com/a/eventespresso.com/file/d/1jb6mrw9ThcfCoLX6LCzD7_DfPVG2WN_o/view?usp=drivesdk. Notice the single ticket amount is correct ($0.97), and the total amount being paid is that single ticket amount times the quantity ($0.97 x 2 = $1.94), but then price modifiers "totals" column doesn't add to the final total: $2 -$0.012 - $0.0199 - $0.01 - $0.01 !- $1.94, it should be $1.95 going by that calculation.

The inconsistency is because the single ticket amount is rounded (from $0.974 to $0.97). Had the ticket amount not been rounded, we could have shown 2 x $0.974 = $1.948, and so charged $1.94.

In case it was missed, the trouble with not rounding single ticket amounts is that gateways expect order items to be rounded, and will give errors if the sum of the rounded single ticket amounts doesn't equal the total payment amount. (Our gateway code can KINDA handle this- it just skips sending the order items; WooCommerce solves this by sending a single item with the description being "2 x $0.974").

[joshfeck]

Rounding issues aside, there's a fatal error that's thrown when viewing a single event page, when a ticket is set be taxable:

Fatal error: Uncaught Error: Call to a member function is_percent() on null in /srv/www/wordpress-default/public_html/wp-content/plugins/32-core/caffeinated/modules/ticket_selector_caff/templates/ticket_selector_price_details.template.php:117 Stack trace: #0 /srv/www/wordpress-default/public_html/wp-content/plugins/32-core/caffeinated/modules/ticket_selector_caff/EED_Ticket_Selector_Caff.module.php(332): require() #1 /srv/www/wordpress-default/public_html/wp-includes/class-wp-hook.php(286): EED_Ticket_Selector_Caff::ticket_price_details(Object(EE_Ticket), 10.6625, true) #2 /srv/www/wordpress-default/public_html/wp-includes/class-wp-hook.php(310): WP_Hook->apply_filters('', Array) #3 /srv/www/wordpress-default/public_html/wp-includes/plugin.php(465): WP_Hook->do_action(Array) #4 /srv/www/wordpress-default/public_html/wp-content/plugins/32-core/modules/ticket_selector/templates/ticket_details.template.php(26): do_action('AHEE__ticket_se...', Object(EE_Ticket), 10.6625, true) #5 /srv/www/wordpress-default/public_html/wp-cont in /srv/www/wordpress-default/public_html/wp-content/plugins/32-core/caffeinated/modules/ticket_selector_caff/templates/ticket_selector_price_details.template.php on line 117

I think we should put this on hold for now until the Attendee Importer is finished.

[mnelson4]

oh that sucks, I didn't get that error. But ok we'll leave this for now.

[tn3rb]

@joshfeck we need this extra precision in the EDTR in order to accurately reverse calculate ticket base prices from the final total (see https://github.com/eventespresso/event-espresso-core/issues/2851). I looked at the template where that error was being thrown and it appeared to just be some copypasta as it was using a variable and some other code from a previous loop in the template.

Here's the actual change I made: https://github.com/eventespresso/event-espresso-core/pull/952/commits/bd14dee1d75d2240beb88606a2e39766374b2aff #diff-9d756049b88ea51c842ddc715e294f3eR121

but I also fixed some formatting in the file

[tn3rb]

unit tests were failing so had to apply some more changes, mostly because tests were based on decimal places getting rounded off at 2 (sometimes hard coded as such).

reg process and admin pages seem to be ok, plus the receipt and invoice look good, but still want to do some more digging to confirm that everything is going to be ok.

[tn3rb]

check out this error that happened during unit testing on TRavis:

There was 1 failure:
1) Events_Admin_Page_Test::test_delete_event
The following error(s) occurred during test "Events_Admin_Page_Test::test_delete_event()" : 

An error has occurred:
Unable to upload the file. Either the path given to download from is incorrect, or something else happened.

Here is the path given:
https://github.com/eventespresso/languages-ee4/blob/master/event_espresso.pot?raw=true
<span class="tiny-text">EEH_Sideloader - sideload - 247</span>

/home/travis/build/eventespresso/event-espresso-core/tests/includes/EE_UnitTestCase.class.php:172

😠

[garthkoyle]

Is the Ticket Price calculator supposed to support six decimal places? I'm seeing that it is rounding to two decimal places: https://www.screencast.com/t/SMs03WLLHTJ

[manzoorwanijk]

Is the Ticket Price calculator supposed to support six decimal places? I'm seeing that it is rounding to two decimal places:

That's because of your settings that is 2 decimal places by default. We wanted to store it upto 6 decimal places just so that if a user changes the settings from lower decimal places to higher decimal places, then the old values will be safe for new configuration.

[garthkoyle]

Do we need to change our settings to allow more than three digits? Event Espresso > General Settings > Countries

[tn3rb]

@garthkoyle

no, that setting in the Countries admin is just for display purposes, whereas the increased decimal precision added in this PR is for internal purposes only to reduce rounding errors caused during calculations.

[garthkoyle]

I was able to set up an event to get some of the calcs that Mike mentioned above:

the price modifiers do not allow more than two decimal places: https://www.screencast.com/t/WSAUE34jg

And adding two tickets gives me $1.95:

I'm uncertain if this work is finished given Mike's comments here and here

[tn3rb]

oh dang it... just rebased onto the wrong branch 🤦🏻‍♂️

welp... that explains all of the conflicts 😖

[tn3rb]

Keep getting these stupid JS unit test failures:

TypeError: (0 , _i18n.isRTL) is not a function

as you can see here: https://github.com/eventespresso/event-espresso-core/pull/952/checks?check_run_id=1666273126#step:6:2892

whose stack traces all end at:

node_modules/@wordpress/components/build/utils/@wordpress/components/src/utils/rtl.js:79:10

which points to this code: https://github.com/WordPress/gutenberg/blob/master/packages/components/src/utils/rtl.js#L79

and if you follow the import for isRTL it all looks kosher: https://github.com/WordPress/gutenberg/blob/master/packages/i18n/src/index.js#L3

[manzoorwanijk]

Keep getting these stupid JS unit test failures:

TypeError: (0 , _i18n.isRTL) is not a function

as you can see here: https://github.com/eventespresso/event-espresso-core/pull/952/checks?check_run_id=1666273126#step:6:2892

whose stack traces all end at:

node_modules/@wordpress/components/build/utils/@wordpress/components/src/utils/rtl.js:79:10

which points to this code: https://github.com/WordPress/gutenberg/blob/master/packages/components/src/utils/rtl.js#L79

and if you follow the import for isRTL it all looks kosher: https://github.com/WordPress/gutenberg/blob/master/packages/i18n/src/index.js#L3

earlier, wp.i18n.isRTL was not a function, rather a boolean property of the package, which resulted many runtime bugs, so they changed it to a function isRTL(). So, I think there may be some older version being installed somewhere by the package manager.

[garthkoyle]

https://garth.eventespresso.com/wp-admin/admin.php?page=espresso_events&action=edit&post=310 I've got that site set to show only 3 decimal places, but it's showing a lot of decimal places in some calculation places.

Alos, Invalid Integer error with Stripe:

JPN:

[joshfeck]

I've tested this and so far I can report this branch works very well.

A testing example that shows how well this works is to apply a 6.625% tax rate (required in New Jersey). We'll do a $50 ticket with a quantity of 5 tickets. Using this example, with the master branch, our tax rate rounds up to 6.63%. This gives us a total price of $266.58, and that's $0.02 more than what should be charged.

Switching to this PR's branch allows us to charge the correct rate of 6.625% for tax, and doing the same test as above gives us the correct total of $266.56. The prices displayed in the ticket selector, the last step of SPCO, the Invoice, and the Receipt are all correct. So we're good with the final result.

There are a few improvements that need to be made when it comes to consistency of display. Here's where there's room for improvement:

1. The ticket editor UI has a rounding error that's displayed immediately after checking the "This ticket is taxable." checkbox.

What's interesting is, after you publish/update the event, the rounding error goes away. I suspect there is some front end code that needs updating.

2. On SPCO's Attendee Information step there's a small table that displays some info about the ticket, including price per ticket. Here it shows the price to 4 decimal places:

This isn't a dealbreaker but it could be disorienting where it's usually expected to see a price with 2 decimal prices. Ideally, the price displayed here should use the decimal place option that's been set in EE's currency settings.

[tn3rb]

@joshfeck OK try that out now... basically I've gone through and found places where any kind of "unit price x quantity" type calculations were being performed and ensured that those unit prices were localized (rounded according to the set currency) PRIOR to performing the calculations.

This prevents cases where items with fractional subunits, like $1.995 from being used in calculations that result in totals that are off from what you would expect. Instead those unit prices now get rounded off prior to calculating the subtotal for more than one of them.

So instead of:

$1.995 x 2 = $3.99

we round first to get two items for $2.00 each, which obviously totals $4.00

One are of the code I have yet to crawl through are the payment gateways, mostly because I hardly have any installed, so if you do, then that would be a great place to perform some additional testing. PayPal works correctly now, which afaik was one of the main ones to be concerned about since they don't like when the line items don't add up correctly to the stated total.

[joshfeck]

It's probably not a deal-breaker but there is some inconsistency with how prices are displayed This is how they're displayed when you input the price: Please note the 5 decimal places where the price is shown.

Then after the event is published:

[tn3rb]

ok plz try this again Josh

[sethshoultes]

Are there testing notes for this issue?

[garthkoyle]

@sethshoultes not full testing notes because this development in core impacts almost all prices... so just look everywhere and test everything related to prices? :)

https://github.com/eventespresso/event-espresso-core/pull/952#issuecomment-759199580

[sethshoultes]

Testing notes:

• Under Your Organization, I set the Country to France
• I set the Country details to France and set the Currency Decimal Places to three, then set the currency thousands separator to Comma
• In my event, I set the price for a ticket to 10,198.
• • However, when saving the price became 10,198,000
• • Saved again the price became 10,198,000,000
• • Saved a third time and the price became 999,999,999,999:
• Viewing the event on the front end shows this:

Basically, it seems there's an issue with saving the event details, where it keeps increasing the price of the event.

[tn3rb]

@sethshoultes it appears you have found a get rich quick scheme!

good catch, will fix

[garthkoyle]

@sethshoultes and @joshfeck this could use some more testing now. Oops, I missed up the tags and it's not ready.

[sethshoultes]

Oops, I missed up the tags and it's not ready.

Ha ha. Ya, it's not working, yet.

[joshfeck]

In case it's not noted elsewhere, with this branch activated, this PHP notice is displayed in the legacy Event Tickets & Datetimes editor:

Notice: Undefined variable: include_taxes in /wp-content/plugins/32-core/core/db_models/EEM_Price.model.php on line 187