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

fix: number format converting to decimals #13465

Merged
merged 10 commits into from Sep 10, 2021

Conversation

hasnain2808
Copy link
Contributor

@hasnain2808 hasnain2808 commented Jun 10, 2021

Issue
For number formats with commas used as decimals on click the number got converted to using dot as decimal. Even in view mode (after save)
This was happening since validate was getting called on focusout
Solution
No need to call validate on focus out since we already call it on change

Before
Kapture 2021-06-28 at 16 54 58

After:
Kapture 2021-06-28 at 17 05 13

@hasnain2808 hasnain2808 requested a review from a team as a code owner June 10, 2021 08:23
@hasnain2808 hasnain2808 requested review from surajshetty3416 and removed request for a team June 10, 2021 08:23
@hasnain2808 hasnain2808 marked this pull request as draft June 10, 2021 08:23
@coveralls
Copy link

coveralls commented Jun 10, 2021

Coverage Status

Coverage increased (+3.6%) to 46.621% when pulling 52941e5 on hasnain2808:fix-number-format-issues into 550bb64 on frappe:develop.

@stale
Copy link

stale bot commented Jun 17, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

@stale stale bot added the inactive label Jun 17, 2021
@hasnain2808 hasnain2808 marked this pull request as ready for review June 22, 2021 04:38
Copy link
Member

@surajshetty3416 surajshetty3416 left a comment

Choose a reason for hiding this comment

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

@hasnain2808 Please add screenshots or animated GIFs for UI-related fixes.

It helps the reviewer or QA rep to get the context of the fix.

@stale
Copy link

stale bot commented Jul 5, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

@stale stale bot added the inactive label Jul 5, 2021
@stale stale bot closed this Jul 8, 2021
@hasnain2808 hasnain2808 reopened this Jul 26, 2021
@stale stale bot removed the inactive label Jul 26, 2021
@stale
Copy link

stale bot commented Aug 2, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

@stale stale bot added the inactive label Aug 2, 2021
Copy link
Member

@surajshetty3416 surajshetty3416 left a comment

Choose a reason for hiding this comment

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

@hasnain2808 the fix should ideally be in the parse function in float.js to avoid such conversions on repeated validation.

@stale stale bot removed the inactive label Aug 4, 2021
@surajshetty3416
Copy link
Member

@hasnain2808 any update on this?

@hasnain2808
Copy link
Contributor Author

hasnain2808 commented Aug 9, 2021

@hasnain2808 the fix should ideally be in the parse function in float.js to avoid such conversions on repeated validation.

@surajshetty3416 I went through all our control files and found out that the parse, flt, format_number are doing their jobs fine

parse method takes the number format(or figures it out) parameter using which it determines the identifiers for decimals and group separators

The problem that was coming was because the lines I removed were calling the parse method and setting the value in the model. parse method's job is to find the actual number from the formatted number setting this in the model is incorrect. the change method calls format number before setting the value in the model

Next time we call the parse method it again gets the number format but this time the number is not formatted with the number format rather it is in normal js float hence it tries to parse it using the shared number format and messes it up

Another interesting block is this
image
I believe this condition was added to make sure that flt does not try to convert numbers. The challenge here is how to figure out what this is '65.76' before searching for the number format. If the number format uses ',' as the decimal separator this should actually be used as a currency type(calling any normal number conversion method would make it a normal number and it would not be parsed). If the number format uses '.' as the decimal separator this should be treated as a float and not parsed. Vice versa of these two cases is also confusing

Hence IMO the focus implementation was incorrect and repeated calls to parse and format number would yield correct results.
Repeated calls to parse would also yield correct result provided that the passed value is of the correct type(number for just doing round and string for doing the number format to float conversion)

Thanks

@stale
Copy link

stale bot commented Aug 16, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

@stale stale bot added the inactive label Aug 16, 2021
@stale stale bot closed this Aug 19, 2021
@hasnain2808 hasnain2808 reopened this Aug 21, 2021
@stale stale bot removed the inactive label Aug 21, 2021
@stale
Copy link

stale bot commented Aug 28, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

@stale
Copy link

stale bot commented Sep 6, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

@stale stale bot added the inactive label Sep 6, 2021
@stale stale bot removed the inactive label Sep 8, 2021
@mergify mergify bot merged commit 74ee14a into frappe:develop Sep 10, 2021
@hasnain2808
Copy link
Contributor Author

@Mergifyio backport version-13-hotfix

@mergify
Copy link
Contributor

mergify bot commented Sep 28, 2021

Command backport version-13-hotfix: success

Backports have been created

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[erpnext 13] Number format #.###,## corrupts data in form field
3 participants