Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #9789 and Fixed #10088 and Fixed [fd23442] - Fix currency problems especially with European currency format #10141

Merged
merged 3 commits into from
Sep 29, 2021

Conversation

uberbrady
Copy link
Collaborator

@uberbrady uberbrady commented Sep 29, 2021

We've had a few reports now of problems when people pick one of the common European currency formats -

1.234.567,89

We've tried to make small piecemeal changes here and there, but this is most comprehensive version I can come up with so far. The logic is, every time we display a currency we use a special helper to do so, and every time we take currency as input we use a special helper to parse the value.

Here's a breakdown of the changes:

  • A new ParseCurrency helper now respects the Snipe-IT settings, and is used in most places of ParseFloat, which was tied to the system's locale
  • The LicenseTransformer was not formatting currencies on output, as the other transformers do. To match the rest of the API's, I made it do so. But we also added a new purchase_cost_numeric field which is in computer-format rather than human-readable. We will probably eventually want to add that everywhere. This could be a breaking change for some people, but it seems more like a bug on our part? Happy to discuss and come up with alternatives if we prefer.
  • I added the app/Helpers/Helper.php class to the alias portion of the app-config, to make it easier to use the main Helper in most blades. I didn't bother going back through all the blades to remove the now-extraneous namespace reference though. That would've added way too much noise.
  • I couldn't find anywhere that resources/views/hardware/qr-view.blade.php was in actual use anymore, so I deleted it.
  • Any place that outputs currency in a 'classic' blade style I made sure was using the Helper::formatCurrencyOutput() method, to be consistent throughout the system.
  • Any place that uses AJAX to populate views, using bootstrap tables and AJAX'ing to our API, tended to already work.
  • I had an idea to make the sumFormatter() Javascript method able to handle correctly-formatted or misformatted currency values, but I decided against doing that - I'd rather have any places that remain where we do not correctly format the currency stick out so we can fix them, if I missed any of them.

Assorted miscellaneous notes

  • This is targeted towards master, which is unusual - but develop has all the v6 stuff in it right now.
  • For the API, I used the ParseCurrency helper but that seems dangerous and weird, and I don't know if that's the right move. I'm happy to pull it. The big problem here would be if someone POST'ed something with 1.234,56 and we parsed it incorrectly. I think the previous application of ParseFloat was incorrect here, and we shouldn't have been doing any parsing, since this is an API. That's weird, though, in that we return values in a parsed format.
  • I found a bug in the ReportsController, around the custom asset report. So I just fixed that - it looks like we shouldn't have to e() the variable that was input. It's actually possible that this isn't even a bug - e('1') is going to be the same as 1, right? I might have been confused. I can certainly pull it.
  • The cleanFloat() method only yanked out the first thousands-indicator (comma for us, period for some of the EU) - so I changed it to yank them all. Some of my test purchase prices were in the millions :) I doubt this would've ever come up, but I figured it's better to be correct.
  • I have tested the heck out of everything when the format is set to the format 1.234,56 - but I haven't even looked at how it all works with the US format. I should probably do that, but I'm a little fried right now.

@snipe
Copy link
Owner

snipe commented Sep 29, 2021

This looks great, thanks @uberbrady!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants