From 1d051bf2ca69f419be2ebb1e0c8292245c4654ca Mon Sep 17 00:00:00 2001 From: 0xD0M1M0 <76812428+0xD0M1M0@users.noreply.github.com> Date: Sun, 17 May 2026 16:47:15 +0000 Subject: [PATCH 1/3] feat(Bank Reconciliation Rule): add priority-based reordering functionality - Enhanced the bank reconciliation rule list view to include a "Reorder by Priority" button. - Implemented backend methods to retrieve bank accounts with rules and reorder rules based on user-defined priorities. - Updated the bank reconciliation rule model to support priority management. - Added tests for the new reordering feature to ensure correct functionality. --- .../bank_reconciliation_rule.py | 93 ++++++++ .../bank_reconciliation_rule_list.js | 214 +++++++++++++++++- .../test_bank_reconciliation_rule.py | 25 ++ 3 files changed, 331 insertions(+), 1 deletion(-) diff --git a/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/bank_reconciliation_rule.py b/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/bank_reconciliation_rule.py index a2a3a6c7..950f7fc2 100644 --- a/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/bank_reconciliation_rule.py +++ b/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/bank_reconciliation_rule.py @@ -2,11 +2,13 @@ # For license information, please see license.txt import json +from typing import cast import frappe from frappe import _ from frappe.exceptions import ValidationError from frappe.model.document import Document +from frappe.utils import cint from banking.exceptions import CurrencyMismatchError @@ -49,3 +51,94 @@ def validate_account_currencies(self): def validate_filters(self): if not self.filters or not json.loads(self.filters): frappe.throw(_("Please define at least one filter!"), NoFiltersError) + + +@frappe.whitelist() +def get_bank_accounts_with_rules() -> list[str]: + """Bank accounts that have at least one non-cancelled reconciliation rule.""" + if not frappe.has_permission("Bank Reconciliation Rule", "read"): + frappe.throw(_("Not permitted to read the bank reconciliation rules"), frappe.PermissionError) + + return frappe.get_all( + "Bank Reconciliation Rule", + filters={"docstatus": ("!=", 2)}, + pluck="bank_account", + group_by="bank_account", + order_by="bank_account asc", + ) + + +@frappe.whitelist() +def get_rules_for_reorder(bank_account: str | None = None) -> list[dict]: + """Load non-cancelled rules for a bank account (list order = current evaluation order).""" + if not bank_account: + frappe.throw(_("Please set a Bank Account")) + if not frappe.has_permission("Bank Reconciliation Rule", "read"): + frappe.throw(_("Not permitted to read the bank reconciliation rules"), frappe.PermissionError) + + rules = frappe.get_all( + "Bank Reconciliation Rule", + filters={"bank_account": bank_account, "docstatus": ("!=", 2)}, + fields=["name", "target_account", "priority", "docstatus", "disabled"], + order_by="priority desc, creation asc", + ) + return list(rules) + + +@frappe.whitelist() +def reorder_bank_reconciliation_rule_priorities( + bank_account: str | None = None, ordered_names: str | list | None = None +) -> None: + """ + Persist **priority** from a full drag-and-drop order: first row = highest priority + (must match `order_by` in **Bank Transaction** automatic rules: `priority desc`). + Values are **10, 20, 30, …** so users can set intermediate priorities (e.g. 15) without + running the dialog again. + """ + if not bank_account: + frappe.throw(_("Please set a Bank Account")) + if not frappe.has_permission("Bank Reconciliation Rule", "write"): + frappe.throw(_("Not permitted to edit the bank reconciliation rules"), frappe.PermissionError) + + raw = frappe.parse_json(ordered_names) if isinstance(ordered_names, str) else ordered_names + if not raw: + frappe.throw(_("Please add at least one rule to the list")) + if not isinstance(raw, list | tuple) or not all(isinstance(n, str) for n in raw): + frappe.throw(_("Invalid rule order")) + names = cast("list[str] | tuple[str, ...]", raw) + ordered: list[str] = [str(n) for n in names] + if len(ordered) != len(set(ordered)): + frappe.throw(_("Each rule may only appear once")) + + expected = set( + frappe.get_all( + "Bank Reconciliation Rule", + filters={"bank_account": bank_account, "docstatus": ("!=", 2)}, + pluck="name", + ) + ) + if set(ordered) != expected: + frappe.throw( + _("The order must include every rule for this bank account. Reload the dialog and try again.") + ) + + n = len(ordered) + for name in ordered: + if frappe.db.get_value("Bank Reconciliation Rule", name, "bank_account") != bank_account: + frappe.throw(_("Rule {0} does not belong to the selected bank account.").format(name)) + if not frappe.has_permission("Bank Reconciliation Rule", "write", name): + frappe.throw(_("Not permitted to update {0}.").format(name), frappe.PermissionError) + + # Stagger so we do not hold duplicate priorities while renumbering submitted rules + for i, name in enumerate(ordered): + frappe.db.set_value( + "Bank Reconciliation Rule", name, "priority", 1_000_000 + i, update_modified=False + ) + + # 10, 20, 30, … so manually inserted rules can use intermediate priorities without renumbering + step = 10 + for idx, name in enumerate(ordered): + priority = cint(n - idx) * step + rule = cast(BankReconciliationRule, frappe.get_doc("Bank Reconciliation Rule", name)) + rule.priority = priority + rule.save() diff --git a/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/bank_reconciliation_rule_list.js b/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/bank_reconciliation_rule_list.js index f215c265..2ec79583 100644 --- a/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/bank_reconciliation_rule_list.js +++ b/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/bank_reconciliation_rule_list.js @@ -2,7 +2,7 @@ // For license information, please see license.txt frappe.listview_settings["Bank Reconciliation Rule"] = { - add_fields: ["docstatus", "disabled"], + add_fields: ["docstatus", "disabled", "priority"], has_indicator_for_draft: true, get_indicator: function (doc) { @@ -20,4 +20,216 @@ frappe.listview_settings["Bank Reconciliation Rule"] = { return [__("Draft"), "blue", "docstatus,=,0"]; } }, + + onload: function (listview) { + if (!frappe.model.can_write("Bank Reconciliation Rule")) { + return; + } + listview.page.add_inner_button(__("Reorder by Priority"), () => + banking_brr_open_reorder_dialog(listview) + ); + }, }; + +function banking_brr_open_reorder_dialog(listview) { + frappe.call({ + method: + "banking.klarna_kosma_integration.doctype.bank_reconciliation_rule.bank_reconciliation_rule.get_bank_accounts_with_rules", + callback(r) { + const accounts = r.message || []; + if (!accounts.length) { + frappe.msgprint({ + title: __("Reorder by Priority"), + message: __("No bank accounts with reconciliation rules."), + indicator: "orange", + }); + return; + } + banking_brr_show_reorder_dialog(listview, accounts); + }, + }); +} + +function banking_brr_show_reorder_dialog(listview, bank_accounts_with_rules) { + let sortable = null; + + const dialog = new frappe.ui.Dialog({ + title: __("Reorder bank reconciliation rules"), + fields: [ + { + fieldname: "info", + fieldtype: "HTML", + options: `

${__( + "The first row is handled first when several rules match. Drag to change the order (priority)." + )}

`, + }, + { + fieldname: "bank_account", + fieldtype: "Link", + label: __("Bank Account"), + options: "Bank Account", + reqd: 1, + }, + { + fieldname: "rules_list_html", + fieldtype: "HTML", + }, + ], + primary_action_label: __("Save order"), + primary_action: function () { + const account = dialog.get_value("bank_account"); + if (!account) { + return; + } + const ul = dialog.$wrapper.find(".brr-reorder__list").get(0); + if (!ul) { + frappe.show_alert({ + indicator: "orange", + message: __("No rules to save."), + }); + return; + } + const ordered = Array.from(ul.querySelectorAll("li[data-name]")).map( + (li) => li.getAttribute("data-name") + ); + if (!ordered.length) { + frappe.show_alert({ + indicator: "orange", + message: __("No rules to save."), + }); + return; + } + frappe.call({ + type: "POST", + freeze: true, + freeze_message: __("Saving..."), + method: + "banking.klarna_kosma_integration.doctype.bank_reconciliation_rule.bank_reconciliation_rule.reorder_bank_reconciliation_rule_priorities", + args: { + bank_account: account, + ordered_names: ordered, + }, + callback: (r) => { + if (!r.exc) { + banking_brr_reset_sortable(); + dialog.hide(); + listview.refresh(); + } + }, + }); + }, + }); + + function banking_brr_reset_sortable() { + if (sortable) { + try { + sortable.destroy(); + } catch (e) { + // ignore + } + } + sortable = null; + } + + function banking_brr_render(rules) { + const $wrap = dialog.fields_dict.rules_list_html.$wrapper; + $wrap.empty(); + if (!rules || !rules.length) { + $wrap.html( + `

${__("No rules for this bank account.")}

` + ); + return; + } + const items = rules + .map((r) => { + const name = frappe.utils.escape_html(r.name); + const ta = r.target_account + ? " — " + frappe.utils.escape_html(r.target_account) + : ""; + const p = Number(r.priority) || 0; + const st = + Number(r.docstatus) === 0 + ? "draft" + : Number(r.disabled) + ? "disabled" + : "submitted"; + return `
  • + ${frappe.utils.icon( + "drag", + "xs", + "", + "", + "sortable-handle" + )} + ${name}${ta} + P ${p} +
    • `; + }) + .join(""); + + $wrap.html( + `` + ); + } + + function banking_brr_init_sortable() { + if (typeof Sortable === "undefined") { + frappe.msgprint( + __( + "Sortable is not available. Please open the dialog from the Bank Reconciliation Rule list and try again." + ) + ); + return; + } + const el = dialog.$wrapper.find(".brr-reorder__list").get(0); + if (!el) { + return; + } + banking_brr_reset_sortable(); + sortable = new Sortable(el, { + handle: ".sortable-handle", + animation: 150, + }); + } + + function banking_brr_load_rules() { + const account = dialog.get_value("bank_account"); + if (!account) { + return; + } + banking_brr_reset_sortable(); + dialog.fields_dict.rules_list_html.$wrapper.html( + `

    ${__("Loading...")}

    ` + ); + frappe.call({ + type: "GET", + method: + "banking.klarna_kosma_integration.doctype.bank_reconciliation_rule.bank_reconciliation_rule.get_rules_for_reorder", + args: { bank_account: account }, + callback: function (r) { + if (r.message) { + banking_brr_render(r.message); + banking_brr_init_sortable(); + } + }, + }); + } + + dialog.$wrapper.addClass("brr-reorder-dialog"); + dialog.fields_dict.bank_account.get_query = () => ({ + filters: [["Bank Account", "name", "in", bank_accounts_with_rules]], + }); + dialog.show(); + const $ba = dialog.get_field("bank_account").$input; + $ba.on("change", () => banking_brr_load_rules()); + $ba.on("awesomplete-selectcomplete", () => banking_brr_load_rules()); + if (bank_accounts_with_rules.length === 1) { + dialog.set_value("bank_account", bank_accounts_with_rules[0]); + banking_brr_load_rules(); + } +} diff --git a/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/test_bank_reconciliation_rule.py b/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/test_bank_reconciliation_rule.py index 8d8652a5..d9926c48 100644 --- a/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/test_bank_reconciliation_rule.py +++ b/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/test_bank_reconciliation_rule.py @@ -7,6 +7,8 @@ from banking.exceptions import CurrencyMismatchError from banking.klarna_kosma_integration.doctype.bank_reconciliation_rule.bank_reconciliation_rule import ( NoFiltersError, + get_rules_for_reorder, + reorder_bank_reconciliation_rule_priorities, ) from banking.testing_utils import TEST_COMPANY, create_bank_account, create_currency_account @@ -19,6 +21,7 @@ def setUpClass(cls): parent_account = frappe.db.get_value("Account", {"is_group": 1, "company": TEST_COMPANY}) bank_account = create_currency_account("EUR", parent_account, "_Test_Account_EUR") cls.target_account = create_currency_account("USD", parent_account, "_Test_Account_USD") + cls.target_eur = create_currency_account("EUR", parent_account, "_Test_BRR_Target_EUR") cls.ba = create_bank_account(bank_account.name) def test_validate_account_currencies(self): @@ -38,3 +41,25 @@ def test_validate_filters(self): brr_doc.filters = "[]" with self.assertRaises(NoFiltersError): brr_doc.validate_filters() + + def _insert_draft_rule(self, description_value: str, priority: int): + r = frappe.new_doc("Bank Reconciliation Rule") + r.bank_account = self.ba.name + r.target_account = self.target_eur.name + r.filters = f'[["Bank Transaction","description","=","{description_value}",false]]' + r.priority = priority + r.insert(ignore_permissions=True, ignore_mandatory=True) + return r + + def test_reorder_priorities(self): + frappe.db.delete("Bank Reconciliation Rule", {"bank_account": self.ba.name}) + r1 = self._insert_draft_rule("BRR-REORDER-1", 2) + r2 = self._insert_draft_rule("BRR-REORDER-2", 1) + self.assertEqual( + [x["name"] for x in get_rules_for_reorder(bank_account=self.ba.name)], [r1.name, r2.name] + ) + reorder_bank_reconciliation_rule_priorities( + bank_account=self.ba.name, ordered_names=[r2.name, r1.name] + ) + self.assertEqual(frappe.db.get_value("Bank Reconciliation Rule", r2.name, "priority"), 20) + self.assertEqual(frappe.db.get_value("Bank Reconciliation Rule", r1.name, "priority"), 10) From 05417fa475537142432af9d7a453d763f3e942e2 Mon Sep 17 00:00:00 2001 From: 0xD0M1M0 <76812428+0xD0M1M0@users.noreply.github.com> Date: Sun, 17 May 2026 16:59:26 +0000 Subject: [PATCH 2/3] fix: update on redundant checks and infinite loading --- .../bank_reconciliation_rule.json | 3 ++- .../bank_reconciliation_rule/bank_reconciliation_rule.py | 2 -- .../bank_reconciliation_rule_list.js | 8 ++++++++ 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/bank_reconciliation_rule.json b/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/bank_reconciliation_rule.json index aa6e6b69..0238f962 100644 --- a/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/bank_reconciliation_rule.json +++ b/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/bank_reconciliation_rule.json @@ -84,6 +84,7 @@ }, { "description": "If multiple rules match a Bank Transaction, only the one with the highest priority is executed.", + "allow_on_submit": 1, "fieldname": "priority", "fieldtype": "Int", "in_list_view": 1, @@ -95,7 +96,7 @@ "index_web_pages_for_search": 1, "is_submittable": 1, "links": [], - "modified": "2026-03-28 02:28:06.918430", + "modified": "2026-05-17 18:58:20.426374", "modified_by": "Administrator", "module": "Klarna Kosma Integration", "name": "Bank Reconciliation Rule", diff --git a/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/bank_reconciliation_rule.py b/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/bank_reconciliation_rule.py index 950f7fc2..146b52c1 100644 --- a/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/bank_reconciliation_rule.py +++ b/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/bank_reconciliation_rule.py @@ -124,8 +124,6 @@ def reorder_bank_reconciliation_rule_priorities( n = len(ordered) for name in ordered: - if frappe.db.get_value("Bank Reconciliation Rule", name, "bank_account") != bank_account: - frappe.throw(_("Rule {0} does not belong to the selected bank account.").format(name)) if not frappe.has_permission("Bank Reconciliation Rule", "write", name): frappe.throw(_("Not permitted to update {0}.").format(name), frappe.PermissionError) diff --git a/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/bank_reconciliation_rule_list.js b/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/bank_reconciliation_rule_list.js index 2ec79583..18ca3ef5 100644 --- a/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/bank_reconciliation_rule_list.js +++ b/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/bank_reconciliation_rule_list.js @@ -212,6 +212,14 @@ function banking_brr_show_reorder_dialog(listview, bank_accounts_with_rules) { "banking.klarna_kosma_integration.doctype.bank_reconciliation_rule.bank_reconciliation_rule.get_rules_for_reorder", args: { bank_account: account }, callback: function (r) { + if (r.exc) { + dialog.fields_dict.rules_list_html.$wrapper.html( + `

    ${__( + "Failed to load rules. Please try again." + )}

    ` + ); + return; + } if (r.message) { banking_brr_render(r.message); banking_brr_init_sortable(); From a6f7fe1bf7209f91abacf596a4fde5b0106d5e64 Mon Sep 17 00:00:00 2001 From: 0xD0M1M0 <76812428+0xD0M1M0@users.noreply.github.com> Date: Sun, 17 May 2026 17:09:37 +0000 Subject: [PATCH 3/3] fix: avoid save() on priority reorder --- .../bank_reconciliation_rule.py | 4 +--- .../test_bank_reconciliation_rule.py | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/bank_reconciliation_rule.py b/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/bank_reconciliation_rule.py index 146b52c1..66ab22b0 100644 --- a/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/bank_reconciliation_rule.py +++ b/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/bank_reconciliation_rule.py @@ -137,6 +137,4 @@ def reorder_bank_reconciliation_rule_priorities( step = 10 for idx, name in enumerate(ordered): priority = cint(n - idx) * step - rule = cast(BankReconciliationRule, frappe.get_doc("Bank Reconciliation Rule", name)) - rule.priority = priority - rule.save() + frappe.db.set_value("Bank Reconciliation Rule", name, "priority", priority) diff --git a/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/test_bank_reconciliation_rule.py b/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/test_bank_reconciliation_rule.py index d9926c48..f828f9be 100644 --- a/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/test_bank_reconciliation_rule.py +++ b/banking/klarna_kosma_integration/doctype/bank_reconciliation_rule/test_bank_reconciliation_rule.py @@ -63,3 +63,20 @@ def test_reorder_priorities(self): ) self.assertEqual(frappe.db.get_value("Bank Reconciliation Rule", r2.name, "priority"), 20) self.assertEqual(frappe.db.get_value("Bank Reconciliation Rule", r1.name, "priority"), 10) + + def test_reorder_priorities_submitted(self): + frappe.db.delete("Bank Reconciliation Rule", {"bank_account": self.ba.name}) + r1 = self._insert_draft_rule("BRR-REORDER-SUB-1", 2) + r2 = self._insert_draft_rule("BRR-REORDER-SUB-2", 1) + r1.submit() + r2.submit() + self.assertEqual(r1.docstatus, 1) + self.assertEqual(r2.docstatus, 1) + + reorder_bank_reconciliation_rule_priorities( + bank_account=self.ba.name, ordered_names=[r2.name, r1.name] + ) + self.assertEqual(frappe.db.get_value("Bank Reconciliation Rule", r2.name, "priority"), 20) + self.assertEqual(frappe.db.get_value("Bank Reconciliation Rule", r1.name, "priority"), 10) + self.assertEqual(frappe.db.get_value("Bank Reconciliation Rule", r2.name, "docstatus"), 1) + self.assertEqual(frappe.db.get_value("Bank Reconciliation Rule", r1.name, "docstatus"), 1)