Add priority reordering function for BRR#367
Conversation
…nality - 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.
Confidence Score: 5/5This looks safe to merge.
Reviews (4): Last reviewed commit: "Merge branch 'version-15-hotfix' into br..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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", | ||
| ) |
There was a problem hiding this comment.
| 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", | |
| ) | |
| def get_bank_accounts_with_rules() -> list[str]: | |
| """Bank accounts that have at least one non-cancelled reconciliation rule.""" | |
| return frappe.get_list( | |
| "Bank Reconciliation Rule", | |
| filters={"docstatus": ("!=", 2)}, | |
| pluck="bank_account", | |
| group_by="bank_account", | |
| order_by="bank_account asc", | |
| ) |
Any particular reason you chose this permission structure? get_list seems more suitable at first glance.
-> Applies to the other whitelisted methods as well.
| ) | ||
|
|
||
|
|
||
| @frappe.whitelist() |
There was a problem hiding this comment.
| @frappe.whitelist() | |
| @frappe.whitelist(methods=["GET"]) | |
| @frappe.read_only() |
Security: whitelisted methods should specify allowed HTTP verbs (GET for read-requests, POST for write requests).
Performance: read_only methods can read from a database replica, if configured.
-> Applies to the other whitelisted methods as well.
|
|
||
| @frappe.whitelist() | ||
| def reorder_bank_reconciliation_rule_priorities( | ||
| bank_account: str | None = None, ordered_names: str | list | None = None |
There was a problem hiding this comment.
| bank_account: str | None = None, ordered_names: str | list | None = None | |
| bank_account: str, ordered_names: str | list |
Why are these parameters advertised as optional in the signature but required in the function body?
There was a problem hiding this comment.
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.
2 participants
Uh oh!
There was an error while loading. Please reload this page.