diff --git a/icd_tz/hooks.py b/icd_tz/hooks.py index 9d471445..3e96d1e2 100644 --- a/icd_tz/hooks.py +++ b/icd_tz/hooks.py @@ -155,6 +155,7 @@ "on_cancel": "icd_tz.icd_tz.api.purchase_order.on_cancel", }, "Purchase Invoice": { + "before_validate": "icd_tz.icd_tz.api.purchase_invoice.set_wip_account", "on_submit": "icd_tz.icd_tz.api.purchase_invoice.on_submit", "on_cancel": "icd_tz.icd_tz.api.purchase_invoice.on_cancel", }, diff --git a/icd_tz/icd_tz/api/purchase_invoice.py b/icd_tz/icd_tz/api/purchase_invoice.py index 7c7d7078..b59b70d9 100644 --- a/icd_tz/icd_tz/api/purchase_invoice.py +++ b/icd_tz/icd_tz/api/purchase_invoice.py @@ -1,6 +1,34 @@ import frappe -from icd_tz.icd_tz.api.purchase_order import get_expense_coverage, set_rows +from icd_tz.icd_tz.api.purchase_order import ( + get_expense_coverage, + get_expense_items_by_type, + get_wip_account, + set_rows, +) + + +def set_wip_account(doc, method=None): + """Hold port expense lines on the work in progress account + + The mapper copies the account from the order the invoice was made from, but an + invoice raised on its own has none, and an invoice can be edited after it is made. + The expense is released to cost of goods sold when the container is invoiced, so a + line that slipped onto another account would never be released. + + Only a configured port expense item on a container line is held: a container can be + tagged on any purchase, and the rest of the invoice is ERPNext's to account for. + Runs before validate so ERPNext still has its say on every other line. + """ + + wip_account = get_wip_account(doc.company) + if not wip_account: + return + + expense_items = get_expense_items_by_type() + for item in doc.items: + if item.get("icd_container") and item.item_code in expense_items: + item.expense_account = wip_account def on_submit(doc, method): diff --git a/icd_tz/icd_tz/api/purchase_order.py b/icd_tz/icd_tz/api/purchase_order.py index 0affe840..0fdf664f 100644 --- a/icd_tz/icd_tz/api/purchase_order.py +++ b/icd_tz/icd_tz/api/purchase_order.py @@ -87,6 +87,49 @@ def validate_no_draft_purchase_order(manifest: str, rows: list): ) +def get_wip_account(company: str) -> str | None: + """Work in progress account port expenses of this company are booked to, or None + + None means leave the expense account alone, so ERPNext keeps choosing it from the + item and company defaults as it always has. An account belongs to one company, and + the settings hold one account, so another company's expenses are left alone too + rather than made unsavable. + """ + + settings_doc = frappe.get_cached_doc("ICD TZ Settings") + if not settings_doc.enable_wip_for_expenses or not settings_doc.wip_account: + return None + + if frappe.get_cached_value("Account", settings_doc.wip_account, "company") != company: + return None + + return settings_doc.wip_account + + +def get_required_wip_account(company: str) -> str | None: + """The WIP account for a port expense order, or None while the ICD does not use one + + Where the ICD is deliberately creating a port expense order, a configured account + that cannot serve the company is said out loud rather than passed over, which is + what would otherwise send the expense quietly back to the ERPNext default. + """ + + settings_doc = frappe.get_cached_doc("ICD TZ Settings") + if not settings_doc.enable_wip_for_expenses: + return None + + wip_account = get_wip_account(company) + if not wip_account: + frappe.throw( + _("Port expenses are booked to work in progress, but {0} cannot be used for {1}").format( + frappe.bold(settings_doc.wip_account or _("no WIP Account")), frappe.bold(company) + ), + title=_("WIP Account Not Usable"), + ) + + return wip_account + + def build_purchase_order(manifest: str, buying_price_list: str, supplier: str, rows: list): """Draft order carrying one line per container, so cost lands on the right dimension""" @@ -94,6 +137,7 @@ def build_purchase_order(manifest: str, buying_price_list: str, supplier: str, r frappe.throw(_("Select a Supplier before creating the Purchase Order")) header = get_manifest_header(manifest) + wip_account = get_required_wip_account(header.company) purchase_order = frappe.new_doc("Purchase Order") purchase_order.update( @@ -110,7 +154,7 @@ def build_purchase_order(manifest: str, buying_price_list: str, supplier: str, r for row in rows: for container in row["containers"]: - add_container_line(purchase_order, manifest, row, container) + add_container_line(purchase_order, manifest, row, container, wip_account) purchase_order.flags.ignore_permissions = True purchase_order.insert() @@ -118,12 +162,13 @@ def build_purchase_order(manifest: str, buying_price_list: str, supplier: str, r return purchase_order -def add_container_line(purchase_order, manifest: str, row: dict, container: dict): +def add_container_line(purchase_order, manifest: str, row: dict, container: dict, wip_account: str | None): """One order line for one container, stamped with its accounting dimensions""" purchase_order.append( "items", { + "expense_account": wip_account, "item_code": row["item_code"], "qty": container["qty"], "rate": row["rate"], diff --git a/icd_tz/icd_tz/doctype/icd_tz_settings/icd_tz_settings.js b/icd_tz/icd_tz/doctype/icd_tz_settings/icd_tz_settings.js index 8af71205..e97c55fd 100644 --- a/icd_tz/icd_tz/doctype/icd_tz_settings/icd_tz_settings.js +++ b/icd_tz/icd_tz/doctype/icd_tz_settings/icd_tz_settings.js @@ -25,6 +25,17 @@ frappe.ui.form.on("ICD TZ Settings", { }; }); + for (const field of ["wip_account", "cogs_account"]) { + frm.set_query(field, () => { + return { + filters: { + is_group: 0, + company: frappe.defaults.get_user_default("Company"), + }, + }; + }); + } + frm.set_query("expense_item", "expense_types", () => { return { filters: { diff --git a/icd_tz/icd_tz/doctype/icd_tz_settings/icd_tz_settings.json b/icd_tz/icd_tz/doctype/icd_tz_settings/icd_tz_settings.json index f9d51809..e10a5c7b 100644 --- a/icd_tz/icd_tz/doctype/icd_tz_settings/icd_tz_settings.json +++ b/icd_tz/icd_tz/doctype/icd_tz_settings/icd_tz_settings.json @@ -7,6 +7,7 @@ "editable_grid": 1, "engine": "InnoDB", "field_order": [ + "section_break_qvvx", "icd_code", "default_price_list", "column_break_8yjq8", @@ -28,6 +29,10 @@ "expenses_tab", "expense_defaults_section", "default_buying_price_list", + "wip_account", + "column_break_wip", + "enable_wip_for_expenses", + "cogs_account", "expense_pricing_criteria_section", "expense_types", "port_storage_section", @@ -181,6 +186,33 @@ "fieldtype": "Tab Break", "label": "Expenses" }, + { + "default": "0", + "description": "Book port expenses to a work in progress account instead of letting ERPNext choose the expense account. The COGS Account is held for releasing them when the container is invoiced.", + "fieldname": "enable_wip_for_expenses", + "fieldtype": "Check", + "label": "Enable WIP for Port Expenses" + }, + { + "depends_on": "enable_wip_for_expenses", + "fieldname": "wip_account", + "fieldtype": "Link", + "label": "WIP Account", + "mandatory_depends_on": "enable_wip_for_expenses", + "options": "Account" + }, + { + "fieldname": "column_break_wip", + "fieldtype": "Column Break" + }, + { + "depends_on": "enable_wip_for_expenses", + "fieldname": "cogs_account", + "fieldtype": "Link", + "label": "COGS Account", + "mandatory_depends_on": "enable_wip_for_expenses", + "options": "Account" + }, { "fieldname": "expense_defaults_section", "fieldtype": "Section Break", @@ -220,12 +252,16 @@ "fieldname": "port_storage_example", "fieldtype": "HTML", "options": "

Example

\n\n\n\n \n \n \n \n \n \n \n \n \n \n \n \n \n \n \n \n
ChargeFromTo
Single17
Double89999999
\n" + }, + { + "fieldname": "section_break_qvvx", + "fieldtype": "Section Break" } ], "index_web_pages_for_search": 1, "issingle": 1, "links": [], - "modified": "2026-09-22 12:00:00.000000", + "modified": "2026-09-23 23:35:21.302602", "modified_by": "Administrator", "module": "Icd Tz", "name": "ICD TZ Settings", @@ -247,4 +283,4 @@ "sort_order": "DESC", "states": [], "track_changes": 1 -} +} \ No newline at end of file diff --git a/icd_tz/icd_tz/doctype/icd_tz_settings/icd_tz_settings.py b/icd_tz/icd_tz/doctype/icd_tz_settings/icd_tz_settings.py index b86a53a6..1f80db37 100644 --- a/icd_tz/icd_tz/doctype/icd_tz_settings/icd_tz_settings.py +++ b/icd_tz/icd_tz/doctype/icd_tz_settings/icd_tz_settings.py @@ -20,6 +20,58 @@ def before_save(self): self.validate_storage_days() self.validate_expense_types() self.validate_port_storage_days() + self.validate_wip_accounts() + + def validate_wip_accounts(self): + """What the two accounts must be before expenses can be booked to work in progress + + The form makes them mandatory and filters out group accounts, but neither + reaches an import, a patch or the API, and a group account fails only later at + posting time. + """ + + if not self.enable_wip_for_expenses: + return + + missing = [ + self.meta.get_label(field) for field in ("wip_account", "cogs_account") if not self.get(field) + ] + if missing: + frappe.throw( + _("{0} is required while port expenses are booked to work in progress").format( + frappe.bold(" and ".join(missing)) + ), + title=_("WIP Accounts Not Set"), + ) + + if self.wip_account == self.cogs_account: + frappe.throw( + _("WIP Account and COGS Account must differ, or releasing an expense would post nothing"), + title=_("WIP Accounts Not Set"), + ) + + companies = set() + for field in ("wip_account", "cogs_account"): + account = frappe.get_cached_value( + "Account", self.get(field), ["is_group", "company"], as_dict=True + ) + if account.is_group: + frappe.throw( + _("{0} is a group account, which cannot be posted to").format( + frappe.bold(self.get(field)) + ), + title=_("WIP Accounts Not Set"), + ) + + companies.add(account.company) + + if len(companies) > 1: + frappe.throw( + _("WIP Account and COGS Account belong to different companies: {0}").format( + frappe.bold(", ".join(sorted(companies))) + ), + title=_("WIP Accounts Not Set"), + ) def validate_storage_days(self): storage_days = [] diff --git a/icd_tz/tests/test_expense_wip_account.py b/icd_tz/tests/test_expense_wip_account.py new file mode 100644 index 00000000..d434eed1 --- /dev/null +++ b/icd_tz/tests/test_expense_wip_account.py @@ -0,0 +1,228 @@ +# Copyright (c) 2026, elius mgani and Contributors +# See license.txt + +from types import SimpleNamespace + +import frappe +from frappe.tests.utils import FrappeTestCase + +from icd_tz.icd_tz.api.purchase_invoice import set_wip_account +from icd_tz.icd_tz.api.purchase_order import ( + add_container_line, + get_required_wip_account, + get_wip_account, +) + +test_ignore = ["Company", "Cost Center"] + +EXPENSE_ITEM = "_Test WIP Expense Item" + + +def get_accounts(company): + return frappe.get_all( + "Account", {"company": company, "is_group": 0, "root_type": "Expense"}, pluck="name", limit=2 + ) + + +def make_expense_item(): + if not frappe.db.exists("Item", EXPENSE_ITEM): + frappe.get_doc( + { + "doctype": "Item", + "item_code": EXPENSE_ITEM, + "item_name": EXPENSE_ITEM, + "item_group": frappe.db.get_value("Item Group", {"is_group": 0}, "name"), + "is_stock_item": 0, + } + ).insert(ignore_permissions=True) + + return EXPENSE_ITEM + + +class WipTestCase(FrappeTestCase): + def setUp(self): + self.company = frappe.db.get_value("Company", {}, "name") + accounts = get_accounts(self.company) + if len(accounts) < 2: + self.skipTest(f"{self.company} has fewer than two postable expense accounts") + + self.wip, self.cogs = accounts + self.other_company = frappe.db.get_value("Company", {"name": ("!=", self.company)}, "name") + settings_doc = frappe.get_single("ICD TZ Settings") + if settings_doc.expense_types: + self.item = settings_doc.expense_types[0].expense_item + else: + self.item = make_expense_item() + settings_doc.append("expense_types", {"expense_type": "Shore", "expense_item": self.item}) + settings_doc.flags.ignore_mandatory = True + settings_doc.save(ignore_permissions=True) + + def tearDown(self): + frappe.clear_document_cache("ICD TZ Settings", "ICD TZ Settings") + + def enable_wip(self, wip_account=None): + frappe.db.set_single_value( + "ICD TZ Settings", + { + "enable_wip_for_expenses": 1, + "wip_account": wip_account or self.wip, + "cogs_account": self.cogs, + }, + ) + + def disable_wip(self): + frappe.db.set_single_value("ICD TZ Settings", "enable_wip_for_expenses", 0) + + +class TestWipAccountSetting(WipTestCase): + """Which account port expenses are booked to""" + + def test_no_account_is_chosen_while_the_setting_is_off(self): + # ERPNext keeps choosing the expense account from the item and company defaults + self.disable_wip() + + self.assertIsNone(get_wip_account(self.company)) + get_required_wip_account(self.company) + + def test_the_wip_account_is_used_while_the_setting_is_on(self): + self.enable_wip() + + self.assertEqual(get_wip_account(self.company), self.wip) + + def test_another_company_is_left_alone_rather_than_blocked(self): + # one settings record holds one account, and an account belongs to one company + if not self.other_company: + self.skipTest("this test needs a second company") + + self.enable_wip() + + self.assertIsNone(get_wip_account(self.other_company)) + + def test_an_order_for_a_company_the_account_cannot_serve_is_refused(self): + if not self.other_company: + self.skipTest("this test needs a second company") + + self.enable_wip() + + self.assertRaises(frappe.ValidationError, get_required_wip_account, self.other_company) + + def test_an_order_is_refused_while_the_account_is_missing(self): + frappe.db.set_single_value("ICD TZ Settings", {"enable_wip_for_expenses": 1, "wip_account": None}) + + self.assertRaises(frappe.ValidationError, get_required_wip_account, self.company) + + def test_turning_it_on_without_the_accounts_is_refused(self): + settings_doc = frappe.get_single("ICD TZ Settings") + settings_doc.update({"enable_wip_for_expenses": 1, "wip_account": None, "cogs_account": None}) + + self.assertRaises(frappe.ValidationError, settings_doc.validate_wip_accounts) + + def test_the_two_accounts_must_differ(self): + settings_doc = frappe.get_single("ICD TZ Settings") + settings_doc.update({"enable_wip_for_expenses": 1, "wip_account": self.wip, "cogs_account": self.wip}) + + self.assertRaises(frappe.ValidationError, settings_doc.validate_wip_accounts) + + def test_two_accounts_of_different_companies_are_refused(self): + if not self.other_company: + self.skipTest("this test needs a second company") + + other_accounts = get_accounts(self.other_company) + if not other_accounts: + self.skipTest(f"{self.other_company} has no postable expense account") + + settings_doc = frappe.get_single("ICD TZ Settings") + settings_doc.update( + { + "enable_wip_for_expenses": 1, + "wip_account": self.wip, + "cogs_account": other_accounts[0], + } + ) + + self.assertRaises(frappe.ValidationError, settings_doc.validate_wip_accounts) + + def test_a_group_account_is_refused(self): + group = frappe.db.get_value("Account", {"company": self.company, "is_group": 1}, "name") + settings_doc = frappe.get_single("ICD TZ Settings") + settings_doc.update({"enable_wip_for_expenses": 1, "wip_account": group, "cogs_account": self.cogs}) + + self.assertRaises(frappe.ValidationError, settings_doc.validate_wip_accounts) + + +class TestOrderAndInvoiceLines(WipTestCase): + """Holding the expense on WIP from the order through to the invoice""" + + def make_order_line(self, wip_account): + purchase_order = frappe.new_doc("Purchase Order") + purchase_order.schedule_date = frappe.utils.nowdate() + row = { + "item_code": self.item, + "rate": 100, + "expense_type": "Shore", + "size": "20ft", + "cargo_type": None, + "destination": None, + "port": None, + } + container = { + "qty": 1, + "container_no": "TEST1234567", + "day_rows": [], + "icd_master_bl": "MBL-1", + "icd_container": "CNT-1", + } + add_container_line(purchase_order, "ICD-M-0001", row, container, wip_account) + + return purchase_order.items[0] + + def make_invoice(self, item_code, icd_container="CNT-1", expense_account="SOMETHING ELSE"): + # frappe._dict cannot carry an "items" attribute, it shadows the dict method + return SimpleNamespace( + company=self.company, + items=[ + frappe._dict( + item_code=item_code, icd_container=icd_container, expense_account=expense_account + ) + ], + ) + + def test_an_order_line_carries_the_wip_account(self): + self.assertEqual(self.make_order_line(self.wip).expense_account, self.wip) + + def test_an_order_line_carries_no_account_while_the_setting_is_off(self): + self.assertIsNone(self.make_order_line(None).expense_account) + + def test_an_invoice_line_for_a_port_expense_is_held_on_wip(self): + self.enable_wip() + doc = self.make_invoice(self.item) + + set_wip_account(doc) + + self.assertEqual(doc.items[0].expense_account, self.wip) + + def test_a_line_that_is_not_a_configured_expense_is_left_alone(self): + # a container can be tagged on any purchase, and the rest of that invoice is + # ERPNext's to account for, stock items above all + self.enable_wip() + doc = self.make_invoice("_Test Item") + + set_wip_account(doc) + + self.assertEqual(doc.items[0].expense_account, "SOMETHING ELSE") + + def test_a_line_with_no_container_is_left_alone(self): + self.enable_wip() + doc = self.make_invoice(self.item, icd_container=None) + + set_wip_account(doc) + + self.assertEqual(doc.items[0].expense_account, "SOMETHING ELSE") + + def test_an_invoice_is_left_alone_while_the_setting_is_off(self): + self.disable_wip() + doc = self.make_invoice(self.item) + + set_wip_account(doc) + + self.assertEqual(doc.items[0].expense_account, "SOMETHING ELSE")