From 5c35aae8767a0eb34da44f9a0e1b2a4ec2935f3b Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 31 May 2022 09:54:21 +0530 Subject: [PATCH] fix: accurate prefix parsing Previous version of prefix parsing relied on partial reimplemntation of naming series logic which was outdated and often incorrect. --- .../document_naming_settings.py | 45 +++++++------- frappe/model/naming.py | 59 +++++++++++++++++-- frappe/tests/test_naming.py | 18 +++++- 3 files changed, 90 insertions(+), 32 deletions(-) diff --git a/frappe/core/doctype/document_naming_settings/document_naming_settings.py b/frappe/core/doctype/document_naming_settings/document_naming_settings.py index 35f16f282e..1f6a71e567 100644 --- a/frappe/core/doctype/document_naming_settings/document_naming_settings.py +++ b/frappe/core/doctype/document_naming_settings/document_naming_settings.py @@ -7,7 +7,7 @@ import frappe from frappe import _ from frappe.core.doctype.doctype.doctype import validate_series from frappe.model.document import Document -from frappe.model.naming import make_autoname, parse_naming_series +from frappe.model.naming import get_naming_series_prefix, make_autoname from frappe.permissions import get_doctypes_with_read from frappe.utils import cint @@ -153,38 +153,35 @@ class DocumentNamingSettings(Document): return frappe.get_meta(doctype or self.transaction_type).get_field("naming_series").options @frappe.whitelist() - def get_current(self, arg=None): + def get_current(self): """get series current""" if self.prefix: - prefix = self.parse_naming_series() + prefix = get_naming_series_prefix(self.prefix) self.current_value = frappe.db.get_value("Series", prefix, "current", order_by="name") - def insert_series(self, series): - """insert series if missing""" - if frappe.db.get_value("Series", series, "name", order_by="name") == None: - frappe.db.sql("insert into tabSeries (name, current) values (%s, 0)", (series)) - @frappe.whitelist() def update_series_start(self): - if self.prefix: - prefix = self.parse_naming_series() - self.insert_series(prefix) - frappe.db.sql( - "update `tabSeries` set current = %s where name = %s", (cint(self.current_value), prefix) - ) - frappe.msgprint(_("Series Updated Successfully")) - else: - frappe.msgprint(_("Please select prefix first")) + if not self.prefix: + frappe.throw(_("Please select prefix first")) - def parse_naming_series(self): - parts = self.prefix.split(".") + series = frappe.qb.DocType("Series") - # Remove ### from the end of series - if parts[-1] == "#" * len(parts[-1]): - del parts[-1] + db_prefix = get_naming_series_prefix(self.prefix) - prefix = parse_naming_series(parts) - return prefix + if frappe.db.get_value("Series", db_prefix, "name", order_by="name") is None: + series.insert(db_prefix, 0).columns(series.name, series.current).run() + + ( + frappe.qb.update(series) + .set(series.current, cint(self.current_value)) + .where(series.name == db_prefix) + ).run() + + frappe.msgprint( + _("Series counter for {} updated to {} successfully").format(self.prefix, self.current_value), + alert=True, + indicator="green", + ) @frappe.whitelist() def preview_series(self) -> str: diff --git a/frappe/model/naming.py b/frappe/model/naming.py index ff7448e069..9ffb72b09c 100644 --- a/frappe/model/naming.py +++ b/frappe/model/naming.py @@ -2,7 +2,7 @@ # License: MIT. See LICENSE import re -from typing import TYPE_CHECKING, Optional, Union +from typing import TYPE_CHECKING, Callable, List, Optional, Union import frappe from frappe import _ @@ -11,6 +11,7 @@ from frappe.query_builder import DocType from frappe.utils import cint, cstr, now_datetime if TYPE_CHECKING: + from frappe.model.document import Document from frappe.model.meta import Meta @@ -189,10 +190,28 @@ def make_autoname(key="", doctype="", doc=""): return n -def parse_naming_series(parts, doctype="", doc=""): - n = "" +def parse_naming_series( + parts: Union[List[str], str], + doctype=None, + doc: Optional["Document"] = None, + number_generator: Optional[Callable[[str, int], str]] = None, +) -> str: + + """Parse the naming series and get next name. + + args: + parts: naming series parts (split by `.`) + doc: document to use for series that have parts using fieldnames + number_generator: Use different counter backend other than `tabSeries`. Primarily used for testing. + """ + + name = "" if isinstance(parts, str): parts = parts.split(".") + + if not number_generator: + number_generator = getseries + series_set = False today = now_datetime() for e in parts: @@ -200,7 +219,7 @@ def parse_naming_series(parts, doctype="", doc=""): if e.startswith("#"): if not series_set: digits = len(e) - part = getseries(n, digits) + part = number_generator(name, digits) series_set = True elif e == "YY": part = today.strftime("%y") @@ -225,9 +244,37 @@ def parse_naming_series(parts, doctype="", doc=""): part = e if isinstance(part, str): - n += part + name += part - return n + return name + + +def get_naming_series_prefix(series: str) -> str: + """Naming series stores prefix to maintain a counter in DB. This prefix can be used to update counter and/or other validations. + + e.g. `SINV-.YY.-.####` has prefix of `SINV-22-` in database for year 2022. + """ + + prefix = None + + if "#" not in series: + series += ".#####" + + def fake_counter_backend(partial_series, digits): + nonlocal prefix + prefix = partial_series + return "#" * digits + + # This function evaluates all parts till we hit numerical parts and then + # sends prefix + digits to DB to find next number. + # Instead of reimplemnted the whole parsing logic in multiple places we can + # just ask this function to give us the prefix. + parse_naming_series(series, number_generator=fake_counter_backend) + + if prefix is None: + frappe.throw(_("Invalid Naming Series")) + + return prefix def determine_consecutive_week_number(datetime): diff --git a/frappe/tests/test_naming.py b/frappe/tests/test_naming.py index 89266d92ea..cad91c11a3 100644 --- a/frappe/tests/test_naming.py +++ b/frappe/tests/test_naming.py @@ -1,13 +1,12 @@ # Copyright (c) 2018, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE -import unittest - import frappe from frappe.core.doctype.doctype.test_doctype import new_doctype from frappe.model.naming import ( append_number_if_name_exists, determine_consecutive_week_number, + get_naming_series_prefix, getseries, revert_series_if_last, ) @@ -288,6 +287,21 @@ class TestNaming(FrappeTestCase): dt.delete(ignore_permissions=True) + def test_naming_series_prefix(self): + today = now_datetime() + year = today.strftime("%y") + month = today.strftime("%m") + + prefix_test_cases = { + "SINV-.YY.-.####": f"SINV-{year}-", + "SINV-.YY.-.MM.-.####": f"SINV-{year}-{month}-", + "SINV": "SINV", + "SINV-.": "SINV-", + } + + for series, prefix in prefix_test_cases.items(): + self.assertEqual(prefix, get_naming_series_prefix(series)) + def make_invalid_todo(): frappe.get_doc({"doctype": "ToDo", "description": "Test"}).insert(set_name="ToDo")