diff --git a/erpnext/stock/doctype/quality_inspection/quality_inspection.py b/erpnext/stock/doctype/quality_inspection/quality_inspection.py index c1d2d831826..c00d1809868 100644 --- a/erpnext/stock/doctype/quality_inspection/quality_inspection.py +++ b/erpnext/stock/doctype/quality_inspection/quality_inspection.py @@ -2,13 +2,15 @@ # License: GNU General Public License v3. See license.txt +from math import isfinite from typing import Any import frappe from frappe import _ from frappe.model.document import Document from frappe.model.mapper import get_mapped_doc -from frappe.utils import cint, flt, get_link_to_form, get_number_format_info +from frappe.utils import cint, flt, get_link_to_form +from frappe.utils.number_format import NUMBER_FORMAT_MAP, NumberFormat from erpnext.stock.doctype.quality_inspection_template.quality_inspection_template import ( get_template_details, @@ -84,6 +86,7 @@ class QualityInspection(Document): reading.status = "Accepted" if self.readings: + self.validate_reading_number_format() self.inspect_and_set_status() self.validate_inspection_required() @@ -281,6 +284,47 @@ class QualityInspection(Document): ) break + def validate_reading_number_format(self): + """Reject newly entered readings that are not numbers in the user's format. + + They would otherwise be misread rather than refused, silently rejecting an + inspection whose readings are in fact within the acceptance range. Readings + already stored are left alone, so a document entered by a user in one locale + stays saveable and submittable by a user in another.""" + number_format = get_reading_number_format() + decimal_str, comma_str = get_reading_separators(number_format) + before_save = self.get_doc_before_save() + + for reading in self.readings: + if not cint(reading.numeric) or cint(reading.manual_inspection): + continue + + stored = before_save and before_save.get("readings", {"name": reading.name}) + stored = stored[0] if stored else None + + for i in range(1, 11): + field = "reading_" + str(i) + value = reading.get(field) + if value is None or not value.strip(): + continue + + if stored and stored.get(field) == value: + continue + + if parse_reading(value, decimal_str, comma_str) is None: + frappe.throw( + _( + "Row #{0}: Reading {1} {2} is not a valid number in the {3} number format. Use {4} as the decimal separator." + ).format( + reading.idx, + i, + frappe.bold(value), + frappe.bold(number_format.string), + frappe.bold(decimal_str), + ), + title=_("Invalid Reading"), + ) + def set_status_based_on_acceptance_values(self, reading): if not cint(reading.numeric): reading_value = reading.get("reading_value") or "" @@ -511,17 +555,61 @@ def make_quality_inspection(source_name: str, target_doc: str | dict | Document return doc +def get_reading_number_format() -> NumberFormat: + """Number format the user enters readings in. + + User defaults fall back to the global default, so this is the same format the + user's desk formats numbers with.""" + number_format = frappe.defaults.get_user_default("number_format") + if number_format not in NUMBER_FORMAT_MAP: + number_format = "#,###.##" + + return NumberFormat.from_string(number_format) + + +def get_reading_separators(number_format: NumberFormat) -> tuple[str, str]: + """Decimal and thousands separator a reading may be written with. + + A format with no decimal separator still has to accept decimal readings, so it + falls back to a dot and gives up any grouping that would collide with it.""" + decimal_str = number_format.decimal_separator or "." + comma_str = number_format.thousands_separator + + return decimal_str, "" if comma_str == decimal_str else comma_str + + +def parse_reading(value: str, decimal_str: str, comma_str: str) -> float | None: + """Reading as a float, or None when it is not a number in that format.""" + value = value.strip() + integer_part = value.partition(decimal_str)[0] + + if comma_str and comma_str in integer_part: + groups = integer_part.split(comma_str) + lead = groups[0][1:] if groups[0][:1] in ("+", "-") else groups[0] + if not 1 <= len(lead) <= 3 or len(groups[-1]) != 3: + return None + + if any(len(group) not in (2, 3) for group in groups[1:-1]): + return None + + value = value.replace(comma_str, "") + + if decimal_str != ".": + value = value.replace(decimal_str, ".") + + try: + number = float(value) + except ValueError: + return None + + return number if isfinite(number) else None + + def parse_float(num: str) -> float: """Since reading_# fields are `Data` field they might contain number which is representation in user's prefered number format instead of machine readable format. This function converts them to machine readable format.""" - number_format = frappe.db.get_default("number_format") or "#,###.##" - decimal_str, comma_str, _number_format_precision = get_number_format_info(number_format) + decimal_str, comma_str = get_reading_separators(get_reading_number_format()) - if decimal_str == "," and comma_str == ".": - num = num.replace(",", "#$") - num = num.replace(".", ",") - num = num.replace("#$", ".") - - return flt(num) + return flt(parse_reading(num, decimal_str, comma_str)) diff --git a/erpnext/stock/doctype/quality_inspection/test_quality_inspection.py b/erpnext/stock/doctype/quality_inspection/test_quality_inspection.py index 9445e5da94f..2ce6d4fe338 100644 --- a/erpnext/stock/doctype/quality_inspection/test_quality_inspection.py +++ b/erpnext/stock/doctype/quality_inspection/test_quality_inspection.py @@ -1,8 +1,11 @@ # Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors and Contributors # See license.txt +from contextlib import contextmanager + import frappe from frappe.utils import nowdate +from frappe.utils.number_format import NumberFormat from erpnext.controllers.stock_controller import ( QualityInspectionNotSubmittedError, @@ -12,10 +15,29 @@ from erpnext.controllers.stock_controller import ( ) from erpnext.stock.doctype.delivery_note.test_delivery_note import create_delivery_note from erpnext.stock.doctype.item.test_item import create_item +from erpnext.stock.doctype.quality_inspection.quality_inspection import ( + get_reading_separators, + parse_reading, +) from erpnext.stock.doctype.stock_entry.stock_entry_utils import make_stock_entry from erpnext.tests.utils import ERPNextTestSuite +@contextmanager +def user_number_format(number_format): + """Temporarily set the session user's own number format.""" + user = frappe.session.user + previous = frappe.db.get_value("DefaultValue", {"parent": user, "defkey": "number_format"}, "defvalue") + frappe.defaults.set_user_default("number_format", number_format) + try: + yield + finally: + if previous: + frappe.defaults.set_user_default("number_format", previous) + else: + frappe.defaults.clear_user_default("number_format") + + class TestQualityInspection(ERPNextTestSuite): def setUp(self): super().setUp() @@ -108,7 +130,6 @@ class TestQualityInspection(ERPNextTestSuite): "acceptance_formula": "mean < 0.9", "reading_1": "0.5", "reading_2": "0.7", - "reading_3": "random text", # check if random string input causes issues }, { "specification": "Calcium Content", # non-numeric reading @@ -252,6 +273,208 @@ class TestQualityInspection(ERPNextTestSuite): qa.delete() dn.delete() + def test_non_numeric_reading(self): + dn = create_delivery_note(item_code="_Test Item with QA", do_not_submit=True) + create_quality_inspection_parameter("Density") + + readings = [ + {"specification": "Density", "min_value": 1.15, "max_value": 1.20, "reading_1": "random text"} + ] + qa = create_quality_inspection( + reference_type="Delivery Note", reference_name=dn.name, readings=readings, do_not_save=True + ) + + self.assertRaises(frappe.ValidationError, qa.save) + + dn.delete() + + def test_non_numeric_reading_in_formula_based_criteria(self): + dn = create_delivery_note(item_code="_Test Item with QA", do_not_submit=True) + create_quality_inspection_parameter("Density") + + readings = [ + { + "specification": "Density", + "formula_based_criteria": 1, + "acceptance_formula": "mean < 0.9", + "reading_1": "0.5", + "reading_2": "0.7", + "reading_3": "random text", + } + ] + qa = create_quality_inspection( + reference_type="Delivery Note", reference_name=dn.name, readings=readings, do_not_save=True + ) + + self.assertRaises(frappe.ValidationError, qa.save) + + dn.delete() + + def test_manual_inspection_reading_is_not_number_checked(self): + dn = create_delivery_note(item_code="_Test Item with QA", do_not_submit=True) + create_quality_inspection_parameter("Density") + + readings = [ + { + "specification": "Density", + "manual_inspection": 1, + "status": "Accepted", + "min_value": 1.15, + "max_value": 1.20, + "reading_1": "1.15 g/cm3", + } + ] + qa = create_quality_inspection( + reference_type="Delivery Note", reference_name=dn.name, readings=readings, do_not_save=True + ) + qa.save() + + self.assertEqual(qa.readings[0].status, "Accepted") + + qa.delete() + dn.delete() + + def test_reading_in_comma_decimal_number_format(self): + dn = create_delivery_note(item_code="_Test Item with QA", do_not_submit=True) + create_quality_inspection_parameter("Density") + + readings = [{"specification": "Density", "min_value": 1.15, "max_value": 1.20, "reading_1": "1,15"}] + with user_number_format("#.###,##"): + qa = create_quality_inspection( + reference_type="Delivery Note", reference_name=dn.name, readings=readings, do_not_save=True + ) + qa.save() + + self.assertEqual(qa.readings[0].status, "Accepted") + self.assertEqual(qa.status, "Accepted") + + qa.delete() + dn.delete() + + def test_reading_in_space_grouped_number_format(self): + dn = create_delivery_note(item_code="_Test Item with QA", do_not_submit=True) + create_quality_inspection_parameter("Density") + + readings = [{"specification": "Density", "min_value": 1.15, "max_value": 1.20, "reading_1": "1,15"}] + with user_number_format("# ###,##"): + qa = create_quality_inspection( + reference_type="Delivery Note", reference_name=dn.name, readings=readings, do_not_save=True + ) + qa.save() + + self.assertEqual(qa.readings[0].status, "Accepted") + self.assertEqual(qa.status, "Accepted") + + qa.delete() + dn.delete() + + def test_reading_in_wrong_decimal_number_format(self): + dn = create_delivery_note(item_code="_Test Item with QA", do_not_submit=True) + create_quality_inspection_parameter("Density") + + readings = [{"specification": "Density", "min_value": 1.15, "max_value": 1.20, "reading_1": "1.15"}] + with user_number_format("#.###,##"): + qa = create_quality_inspection( + reference_type="Delivery Note", reference_name=dn.name, readings=readings, do_not_save=True + ) + + self.assertRaises(frappe.ValidationError, qa.save) + + dn.delete() + + def test_reading_with_comma_in_dot_decimal_number_format(self): + dn = create_delivery_note(item_code="_Test Item with QA", do_not_submit=True) + create_quality_inspection_parameter("Density") + + readings = [{"specification": "Density", "min_value": 1.15, "max_value": 1.20, "reading_1": "1,15"}] + with user_number_format("#,###.##"): + qa = create_quality_inspection( + reference_type="Delivery Note", reference_name=dn.name, readings=readings, do_not_save=True + ) + + self.assertRaises(frappe.ValidationError, qa.save) + + dn.delete() + + @ERPNextTestSuite.change_settings("System Settings", {"number_format": "#,###.##"}) + def test_reading_number_format_prefers_the_user_over_the_system(self): + dn = create_delivery_note(item_code="_Test Item with QA", do_not_submit=True) + create_quality_inspection_parameter("Density") + + readings = [{"specification": "Density", "min_value": 1.15, "max_value": 1.20, "reading_1": "1,15"}] + with user_number_format("#.###,##"): + qa = create_quality_inspection( + reference_type="Delivery Note", reference_name=dn.name, readings=readings, do_not_save=True + ) + qa.save() + + self.assertEqual(qa.readings[0].status, "Accepted") + + qa.delete() + dn.delete() + + def test_stored_reading_stays_submittable_in_another_number_format(self): + dn = create_delivery_note(item_code="_Test Item with QA", do_not_submit=True) + create_quality_inspection_parameter("Density") + + readings = [{"specification": "Density", "min_value": 1.15, "max_value": 1.20, "reading_1": "1,15"}] + with user_number_format("#.###,##"): + qa = create_quality_inspection( + reference_type="Delivery Note", reference_name=dn.name, readings=readings, do_not_save=True + ) + qa.save() + + with user_number_format("#,###.##"): + qa.reload() + qa.submit() + + self.assertEqual(qa.docstatus, 1) + + qa.cancel() + qa.delete() + dn.delete() + + def test_parse_reading_in_every_number_format(self): + accepted = [ + ("#,###.##", "1.15", 1.15), + ("#,###.##", "1,234.56", 1234.56), + ("#,##,###.##", "12,34,567.89", 1234567.89), + ("#,###.###", "1,234.567", 1234.567), + ("#.###,##", "1,15", 1.15), + ("#.###,##", "1.234,56", 1234.56), + ("# ###,##", "1,15", 1.15), + ("# ###,##", "1.15", 1.15), + ("# ###,##", "1 234,56", 1234.56), + ("# ###.##", "1 234.56", 1234.56), + ("#'###.##", "1'234.56", 1234.56), + ("#, ###.##", "1, 234.56", 1234.56), + ("#.########", "1.15", 1.15), + ("#,###", "1.5", 1.5), + ("#,###", "1,500", 1500.0), + ("#.###", "1.5", 1.5), + ("#.###", "1.500", 1.5), + ("#,###.##", "-1,234.56", -1234.56), + ] + refused = [ + ("#,###.##", "1,15"), + ("#.###,##", "1.15"), + ("#,###.##", "--1.15"), + ("#,###.##", "1²"), + ("#,###.##", "nan"), + ("#,###.##", "random text"), + ("#,###", "1,50"), + ] + + for number_format, value, expected in accepted: + decimal_str, comma_str = get_reading_separators(NumberFormat.from_string(number_format)) + with self.subTest(number_format=number_format, value=value): + self.assertEqual(parse_reading(value, decimal_str, comma_str), expected) + + for number_format, value in refused: + decimal_str, comma_str = get_reading_separators(NumberFormat.from_string(number_format)) + with self.subTest(number_format=number_format, value=value): + self.assertIsNone(parse_reading(value, decimal_str, comma_str)) + def test_delete_quality_inspection_linked_with_stock_entry(self): item_code = create_item("_Test Cicuular Dependecy Item with QA").name