We only want to help here. But the indicated resolution does not fix the problem, as posted above.
A value “5.000,00” in some localisations is valid “5000.00”. So cropping the comma will only work for some currencies but is highly dangerous… There should be a test case which enters values in each localisation and checks the value that gets to the db. In a calm minute, I’ll try to think of something…
Precisely - the takeaway learning here is the need to write a test for say one or two locales. (Whoever cares to extend to their locale can do so.)
With the problem identified, the test would illustrate and reproduce the exact issue. Once the code is fixed, the test would validate the fix.
If that test ever fails again - say some proposed change breaks this - it would help identify and resolve the problem. And that would be cause to not commit that code, revisit whether the test is valid and so on.
Moreover tests show how code APIs work, capture business rule context, are change detectors, are key to enable code refactoring - that is essential to manage technical debt.
Dears,
not all who contribute or discuss here are coding savy. Some are business owners, experts, etc. @Saqib_Ansari a rule of thumb while dealing with an international system especially ERP involving financials is not to mess with it without proper design document or knowledge of the domain.
Same testing issue arises in the Currency Doctype. The remedy is already submitted in GitHub and still not committed. The design and tests are not in accordance so basically the Currency Doctype is not compliant with any business rules. fix: Currency Exchange for_selling and for_buying on the same day by toofun666 · Pull Request #19339 · frappe/erpnext · GitHub Just noticed my fix was held up due to space/tab issues. Correcting and submitting again.
I could not agree more. We need unit test, UI and integration tests. The more we can get, the better it will be for the overall quality of the system. And I agree, that it is the duty of our region to provide tests for such “comma issues” as discussed here.
I have created a separate thread on the UI and integration tests here so that this thread does not need to be highjacked:
Frappe Framework: v12.0.20 (version-12)
Tried on Purchase invoice, the value entered in Purchase Invoice item like 3,2 is converted to 32.
Better to revert all changes on these objects starting with this Excel related update.
updated my live system with:
bench update --reset
this morning. It is not working afterwards. I still have to manually revert all the changes these developers caused.
Reloaded built and all I will check once again tonight when there is no user load.
I don’t think they will nail it as they don’t get it in the first place. Sorry.
Dears,
the latest update is not working. Please find the forensics below:
the files changed since the update regarding the pasting from Excel files are below:
~/bench/apps/frappe/.eslintrc
~/bench/apps/frappe/frappe/geo/doctype/currency/currency.js
~/bench/apps/frappe/frappe/public/js/frappe/utils/number_format.js
~/bench/apps/frappe/frappe/public/js/frappe/form/controls/float.js
~/bench/apps/frappe/frappe/public/js/frappe/form/controls/currency.js
all of the files are recently updated via:
bench update --reset