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

Improve reference calculator #836

Merged

Conversation

twothreenine
Copy link
Contributor

@twothreenine twothreenine commented Feb 20, 2021

  • fix rounding error: previously, some numbers like 38.20 (which is something like 38.2000000001) were ceiled to 38.21 (was there a specific reason to use "ceil" instead of "round"?)
  • always display final amount with two decimals
  • add currency symbol to input fields and final amount
  • better(?) headline for transaction types - you could also use "type" (as in transaction type) which would be closer to the model, but I think "purpose" is better understandable and fitting in this context.

fix rounding error, add currency symbol, better(?) translation for headline
@twothreenine twothreenine marked this pull request as ready for review February 20, 2021 01:11
Copy link
Member

@paroga paroga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, beside the Math.ceil

@@ -12,7 +12,7 @@
$('input[data-has-name-short]').each(function() {
var bank_account = $(this).attr('data-bank-account') || default_bank_account;
var name = $(this).attr('name');
var value = Math.ceil(parseFloat($(this).val()) * 100) / 100;
var value = Math.round(parseFloat($(this).val()) * 100) / 100;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the rational for this change? I choose ceil to make sure, that people always get an amount which includes all of the required money. If 3 people they to pay 1/3 of 10 entering 3.333 will be rounded to 3.33 and results in 9.99. So the foodcoop misses 0.01 otherwise it gets 0.02 plus (in case people do not enter the exact values anyway).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I already mentioned, with ceil some numbers with two decimals are wrongly ceiled to the next 0.01 step. Try 1.09, 1.10, 1.11, 1.12 for example. The reason is that they are handled as float and not cut off at the last digit entered.
refcalc

If 3 people they to pay 1/3 of 10 entering 3.333

I don't understand that sentence, why should several people (an ordergroup consisting of 3 people?) enter a single fraction into the calculator? Or do you mean if someone wants to pay 10.00 in total, split evenly on 3 transaction types?
I don't see why this calculator should handle more than two decimals in the first place. The input field turns red as a warning if more than two decimals are entered.

@paroga paroga merged commit 226c117 into foodcoops:master Feb 22, 2021
@twothreenine twothreenine deleted the feature/improve-reference-calculator branch February 25, 2021 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants