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

[Cleanup] Number Precision And Decimal Separator Adjusting #2069

Merged

Conversation

Civolilah
Copy link
Collaborator

@beganovich @turbo124 The PR includes adjustments for decimal precision and separators on various functionalities such as invoicing products, projects, tasks, etc. The value used for decimal precision will be the one entered in User Details => Preferences. If no value is entered, the default precision will be 2. Another part of the adjustment concerns separators. For example, when creating notes while invoicing a project, we will use the separator as defined in the Localization settings. Let me know your thoughts.

Closes #2057

@Civolilah
Copy link
Collaborator Author

Civolilah commented Sep 24, 2024

@turbo124 A separate PR will be created for fixes on the Preview table in Reports. We should make corrections there for money number values.

@Civolilah
Copy link
Collaborator Author

@turbo124 The adjustment has been made here, so formatting numbers on values such as descriptions, where it is something that the API cannot format but is also important to send to the client's customer, is formatted at the client level. However, when the formatting is not related to the client's customer, we will format values according to company settings. Let me know your thoughts.

@turbo124
Copy link
Member

image

When are you format the time in the task log (see image) it must be localized to the client. note here this client is DEU + EUR, so comma separator is used for currency, but not for time duration.

@Civolilah
Copy link
Collaborator Author

image

When are you format the time in the task log (see image) it must be localized to the client. note here this client is DEU + EUR, so comma separator is used for currency, but not for time duration.

@turbo124 Ah, you are right! I missed it. So right now, the zones, date, and time formats should be adjusted on invoicing projects, invoicing tasks, and adding tasks to an invoice. Let me know your thoughts.

@Civolilah
Copy link
Collaborator Author

@turbo124 As we discussed on Slack, the offset addition has been removed here. Only formatting should be applied for all date/time values, along with number separator and precision formatting. Let me know your thoughts.

Comment on lines 25 to 37
return numericFormatter(numStr, {
thousandSeparator: thousandSeparator
? thousandSeparator
: company?.use_comma_as_decimal_place
? '.'
: ',',
decimalSeparator: decimalSeparator
? decimalSeparator
: company?.use_comma_as_decimal_place
? ','
: '.',
decimalScale: precision || userNumberPrecision,
});
Copy link
Member

Choose a reason for hiding this comment

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

Please refactor with nice if statements, this is really unreadable with so many nesting levels.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@beganovich Sure thing, the logic has been separated into functions and then applied to the logic. Let me know your thoughts.

@beganovich beganovich merged commit 1e12f07 into invoiceninja:develop Oct 2, 2024
2 of 3 checks passed
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.

3 participants