-
Notifications
You must be signed in to change notification settings - Fork 38
Add priority reordering function for BRR #367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: version-15-hotfix
Are you sure you want to change the base?
Changes from all commits
1d051bf
05417fa
a6f7fe1
c42a755
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -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,90 @@ 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() | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Security: whitelisted methods should specify allowed HTTP verbs ( -> Applies to the other whitelisted methods as well. |
||||||||
| 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 | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Why are these parameters advertised as optional in the signature but required in the function body? |
||||||||
| ) -> 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 not frappe.has_permission("Bank Reconciliation Rule", "write", name): | ||||||||
| frappe.throw(_("Not permitted to update {0}.").format(name), frappe.PermissionError) | ||||||||
|
greptile-apps[bot] marked this conversation as resolved.
|
||||||||
|
|
||||||||
| # 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 | ||||||||
| frappe.db.set_value("Bank Reconciliation Rule", name, "priority", priority) | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider moving the reorder popup into a dedicated JavaScript bundle that's only loaded and initialized here. Because bundles are built, you can then keep html and css in separate (template) files and import them. That makes the whole code a lot cleaner. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason you chose this permission structure?
get_listseems more suitable at first glance.-> Applies to the other whitelisted methods as well.