From 16fa5ec9bd234f3080c81d3b192f97c4b3a04c08 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 12 Feb 2024 10:51:11 +0530 Subject: [PATCH 1/5] fix: enqueue_after_commit instead of instantly --- .../workflow/doctype/workflow_action/workflow_action.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/frappe/workflow/doctype/workflow_action/workflow_action.py b/frappe/workflow/doctype/workflow_action/workflow_action.py index c4180c47ba..e4623f3b17 100644 --- a/frappe/workflow/doctype/workflow_action/workflow_action.py +++ b/frappe/workflow/doctype/workflow_action/workflow_action.py @@ -116,7 +116,13 @@ def process_workflow_actions(doc, state): create_workflow_actions_for_roles(roles, doc) if send_email_alert(workflow): - enqueue(send_workflow_action_email, queue="short", users_data=list(user_data_map.values()), doc=doc) + enqueue( + send_workflow_action_email, + queue="short", + users_data=list(user_data_map.values()), + doc=doc, + enqueue_after_commit=True, + ) @frappe.whitelist(allow_guest=True) From 8344bce09ab2f068c835aa371cd832796698e392 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 12 Feb 2024 11:14:57 +0530 Subject: [PATCH 2/5] refactor: remove needless coupling of return values --- frappe/workflow/doctype/workflow_action/workflow_action.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/frappe/workflow/doctype/workflow_action/workflow_action.py b/frappe/workflow/doctype/workflow_action/workflow_action.py index e4623f3b17..500f8e210b 100644 --- a/frappe/workflow/doctype/workflow_action/workflow_action.py +++ b/frappe/workflow/doctype/workflow_action/workflow_action.py @@ -108,7 +108,8 @@ def process_workflow_actions(doc, state): if not next_possible_transitions: return - user_data_map, roles = get_users_next_action_data(next_possible_transitions, doc) + roles = {t.allowed for t in next_possible_transitions} + user_data_map = get_users_next_action_data(next_possible_transitions, doc) if not user_data_map: return @@ -327,10 +328,8 @@ def get_next_possible_transitions(workflow_name, state, doc=None): def get_users_next_action_data(transitions, doc): - roles = set() user_data_map = {} for transition in transitions: - roles.add(transition.allowed) users = get_users_with_role(transition.allowed) filtered_users = filter_allowed_users(users, doc, transition) for user in filtered_users: @@ -350,7 +349,7 @@ def get_users_next_action_data(transitions, doc): } ) ) - return user_data_map, roles + return user_data_map def create_workflow_actions_for_roles(roles, doc): From b0a4425230abf94f932b368cb785fad6e4e5ce1d Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 12 Feb 2024 11:20:17 +0530 Subject: [PATCH 3/5] perf: process workflow email receipients from background --- .../doctype/workflow_action/workflow_action.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/frappe/workflow/doctype/workflow_action/workflow_action.py b/frappe/workflow/doctype/workflow_action/workflow_action.py index 500f8e210b..9abac0bc3b 100644 --- a/frappe/workflow/doctype/workflow_action/workflow_action.py +++ b/frappe/workflow/doctype/workflow_action/workflow_action.py @@ -109,19 +109,14 @@ def process_workflow_actions(doc, state): return roles = {t.allowed for t in next_possible_transitions} - user_data_map = get_users_next_action_data(next_possible_transitions, doc) - - if not user_data_map: - return - create_workflow_actions_for_roles(roles, doc) if send_email_alert(workflow): enqueue( send_workflow_action_email, queue="short", - users_data=list(user_data_map.values()), doc=doc, + transitions=next_possible_transitions, enqueue_after_commit=True, ) @@ -353,6 +348,8 @@ def get_users_next_action_data(transitions, doc): def create_workflow_actions_for_roles(roles, doc): + if not roles: + return workflow_action = frappe.get_doc( { "doctype": "Workflow Action", @@ -369,7 +366,8 @@ def create_workflow_actions_for_roles(roles, doc): workflow_action.insert(ignore_permissions=True) -def send_workflow_action_email(users_data, doc): +def send_workflow_action_email(doc, transitions): + users_data = get_users_next_action_data(transitions, doc) common_args = get_common_email_args(doc) message = common_args.pop("message", None) for d in users_data: From 140a01e2cf66d38a2daa9ae6b23bf9f220f3a54d Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 12 Feb 2024 11:32:38 +0530 Subject: [PATCH 4/5] perf: cache permission results For each transition, we end up redoing perm check. Nothing has really changed there though. --- .../workflow_action/workflow_action.py | 26 ++++++++----------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/frappe/workflow/doctype/workflow_action/workflow_action.py b/frappe/workflow/doctype/workflow_action/workflow_action.py index 9abac0bc3b..1506e9311b 100644 --- a/frappe/workflow/doctype/workflow_action/workflow_action.py +++ b/frappe/workflow/doctype/workflow_action/workflow_action.py @@ -324,9 +324,19 @@ def get_next_possible_transitions(workflow_name, state, doc=None): def get_users_next_action_data(transitions, doc): user_data_map = {} + + @frappe.request_cache + def user_has_permission(user: str) -> bool: + from frappe.permissions import has_permission + + return has_permission(doctype=doc, user=user, print_logs=False) + for transition in transitions: users = get_users_with_role(transition.allowed) - filtered_users = filter_allowed_users(users, doc, transition) + filtered_users = [ + user for user in users if has_approval_access(user, doc, transition) and user_has_permission(user) + ] + for user in filtered_users: if not user_data_map.get(user): user_data_map[user] = frappe._dict( @@ -454,20 +464,6 @@ def get_doc_workflow_state(doc): return doc.get(workflow_state_field) -def filter_allowed_users(users, doc, transition): - """Filters list of users by checking if user has access to doc and - if the user satisfies 'workflow transision self approval' condition - """ - from frappe.permissions import has_permission - - return [ - user - for user in users - if has_approval_access(user, doc, transition) - and has_permission(doctype=doc, user=user, print_logs=False) - ] - - def get_common_email_args(doc): doctype = doc.get("doctype") docname = doc.get("name") From 8c5aaeb43782c2230ba324154987d330ead1be29 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 12 Feb 2024 12:38:05 +0530 Subject: [PATCH 5/5] test: avoid fiddling with workflow action columns --- .../doctype/workflow/test_workflow.py | 26 ------------------- 1 file changed, 26 deletions(-) diff --git a/frappe/workflow/doctype/workflow/test_workflow.py b/frappe/workflow/doctype/workflow/test_workflow.py index d8d30a5934..9df30d25b0 100644 --- a/frappe/workflow/doctype/workflow/test_workflow.py +++ b/frappe/workflow/doctype/workflow/test_workflow.py @@ -21,35 +21,9 @@ class TestWorkflow(FrappeTestCase): def setUp(self): self.workflow = create_todo_workflow() frappe.set_user("Administrator") - if self._testMethodName == "test_if_workflow_actions_were_processed_using_user": - if not frappe.db.has_column("Workflow Action", "user"): - # mariadb would raise this statement would create an implicit commit - # if we do not commit before alter statement - # nosemgrep - frappe.db.commit() - frappe.db.multisql( - { - "mariadb": "ALTER TABLE `tabWorkflow Action` ADD COLUMN user varchar(140)", - "postgres": 'ALTER TABLE "tabWorkflow Action" ADD COLUMN "user" varchar(140)', - } - ) - frappe.cache.delete_value("table_columns") def tearDown(self): frappe.delete_doc("Workflow", "Test ToDo") - if self._testMethodName == "test_if_workflow_actions_were_processed_using_user": - if frappe.db.has_column("Workflow Action", "user"): - # mariadb would raise this statement would create an implicit commit - # if we do not commit before alter statement - # nosemgrep - frappe.db.commit() - frappe.db.multisql( - { - "mariadb": "ALTER TABLE `tabWorkflow Action` DROP COLUMN user", - "postgres": 'ALTER TABLE "tabWorkflow Action" DROP COLUMN "user"', - } - ) - frappe.cache.delete_value("table_columns") def test_default_condition(self): """test default condition is set"""