frappe: Incorrect number format handling leads to decimal point being shifted

First I thought that for some reason or other I missed the worst part of #22392.

But unfortunately – though even harder to understand – the problem with decimal separators got substantially worse during the last few weeks.

In the case of the number format being #.###,##, I‘m just tabbing my way through the fields, back and forth. Now see what happens:

https://github.com/frappe/frappe/assets/46800703/12202aba-5c5c-43cc-a9d8-ffb6db8a29ea

What was 2.5 becomes 25, all of a sudden and without any manual input. This is horrible and needs to be fixed asap.

This is my bench: v15

v14 comparison

I doublechecked the error on a fresh v14 bench, and there the bug isn‘t present, except for the mere formatting issue reported in #22392:

https://github.com/frappe/frappe/assets/46800703/0e01ddfd-fe9e-414b-9269-cab242e232a2

The corresponding bench:

v14

It would be very helpful to find out which PR may have caused the problem. Then it should be easier to tackle.

About this issue

Original URL
State: closed
Created 9 months ago
Comments: 20 (20 by maintainers)

Most upvoted comments

I will look into it, but it may take some time. Maybe this weekend? I can‘t promise.

bosue on Sep 28, 2023

TIL, for better debugging: https://stackoverflow.com/a/15923770

The issue (e.g. 3,500 becomes 3.5) is observed during this sequence:

blaggacao on Nov 7, 2023

Could <html lang="en"> in ./frappe/templates/base.html be responsible for that? See: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/lang?retiredLocale=de#:~:text=or the language that the editable elements should be written in by the user

Sidenote:

def get_boot_data():
	return {
		"lang": "en",
		...
        }

resulting in an indiscriminate frappe.boot.lang = "en" in the following cases:

frappe/website/utils.py
160:def get_boot_data():

frappe/website/doctype/website_settings/website_settings.py
10:from frappe.website.utils import get_boot_data
261:	context.boot = get_boot_data()

frappe/website/doctype/web_form/web_form.py
16:from frappe.website.utils import get_boot_data, get_comment_list, get_sidebar_items
248:		context.boot = get_boot_data()

while:

❯ rg 'language' frappe/website/website_components/metatags.py
28:		self.tags["language"] = frappe.local.lang or "en"

blaggacao on Nov 7, 2023

Then we need a failing integration test.

Have you been able to craft one, yet?

cypress/integration/control_float.js should catch it, see https://github.com/frappe/frappe/issues/22521#issuecomment-1733226198.

But it doesn‘t, see for example this test run here and the video.

focus() and blur() might not work properly. At least I can‘t see the field getting its :focus style in the step-by-step snapshots, while in the video, I can’t see the field losing its :focus style.

However, there‘s so much going on there – reformatting the number back and forth – that it‘s really hard to see what‘s wrong, and I may not be proficient enough in JS debugging… 💁🏻‍♂️

bosue on Oct 27, 2023

During Research I found:

It looks like a more reliable implementation than the custom made frappe stuff. 🤷 Support is almost ubiquitous as of recently and there’s a polyfill for older browsers.

blaggacao on Sep 25, 2023

Under which circumstances does 12050 become 12 with 4d0b0d3?

While the unit failure mode is clearly described, I can’t tell with 100% accuracy the side conditions to be met for an integration test to fail (although I had added an integration tes fixed by my PR). When the unit already fails, that’s a bit of a secondary concern, after all but I believe it may be reproducible at the integration level with precision set to 0 on a currency(float) field.

Since I could reproduce the issue described herein, I’m affected and the other investigation is still fresh, I’ll try to fix this today.

blaggacao on Sep 25, 2023

previously 12050 became 12 (with https://github.com/frappe/frappe/commit/4d0b0d35efa64a1692e075b71ae70c4969b81cda)

That wouldn’t be any better, true.

However I never hit that bug and didn‘t manage to reproduce on a clean v14 install. Under which circumstances does 12050 become 12 with 4d0b0d35efa64a1692e075b71ae70c4969b81cda?

Clearly there are layered issues in the code path and the only legit course of action may be to roll them up and fix them one by one. This is why a rigorous root cause analysis and understanding of the code path is key.

I’m absolutely with you. 👍

Without cypress properly working on PRs I fear we won‘t be able to ship a stable product, though. Cypress testing should probably be fixed first.

bosue on Sep 25, 2023

This regression goes back to the commit 0568795.

I‘ll create a (failing) cypress testcase to get this bug covered. We can‘t simply revert @blaggacao‘s revert though, as he found (and obviously fixed) another bug with that commit. Probably it‘s wrong both ways: with and without the code, so we need better code that manages to fix both of them at once.

edit: I looked at the cypress test cases, and they should have caught this. Problem is: Cypress currently doesn‘t work at all, so tests are either skipped or the setup fails anyway.

bosue on Sep 25, 2023