Merge branch 'develop' into discussions-component-redesign

frappe/model/document.py

@@ -1003,8 +1003,6 @@ class Document(BaseDocument):
 		- `on_cancel` for **Cancel**
 		- `update_after_submit` for **Update after Submit**"""
 
-		doc_before_save = self.get_doc_before_save()
-
 		if self._action=="save":
 			self.run_method("on_update")
 		elif self._action=="submit":

frappe/workflow/doctype/workflow/test_workflow.py

@@ -5,6 +5,7 @@ import unittest
 from frappe.utils import random_string
 from frappe.model.workflow import apply_workflow, WorkflowTransitionError, WorkflowPermissionError, get_common_transition_actions
 from frappe.test_runner import make_test_records
+from frappe.query_builder import DocType
 
 
 class TestWorkflow(unittest.TestCase):
@@ -15,9 +16,31 @@ class TestWorkflow(unittest.TestCase):
 	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'''
@@ -75,7 +98,7 @@ class TestWorkflow(unittest.TestCase):
 		actions = get_common_transition_actions([todo1, todo2], 'ToDo')
 		self.assertListEqual(actions, ['Review'])
 
-	def test_if_workflow_actions_were_processed(self):
+	def test_if_workflow_actions_were_processed_using_role(self):
 		frappe.db.delete("Workflow Action")
 		user = frappe.get_doc('User', 'test2@example.com')
 		user.add_roles('Test Approver', 'System Manager')
@@ -93,6 +116,32 @@ class TestWorkflow(unittest.TestCase):
 		self.assertEqual(workflow_actions[0].status, 'Completed')
 		frappe.set_user('Administrator')
 
+	def test_if_workflow_actions_were_processed_using_user(self):
+		frappe.db.delete("Workflow Action")
+
+		user = frappe.get_doc('User', 'test2@example.com')
+		user.add_roles('Test Approver', 'System Manager')
+		frappe.set_user('test2@example.com')
+
+		doc = self.test_default_condition()
+		workflow_actions = frappe.get_all('Workflow Action', fields=['*'])
+		self.assertEqual(len(workflow_actions), 1)
+
+		# test if status of workflow actions are updated on approval
+		WorkflowAction = DocType("Workflow Action")
+		WorkflowActionPermittedRole = DocType("Workflow Action Permitted Role")
+		frappe.qb.update(WorkflowAction).set(WorkflowAction.user, 'test2@example.com').run()
+		frappe.qb.update(WorkflowActionPermittedRole).set(WorkflowActionPermittedRole.role, '').run()
+
+		self.test_approve(doc)
+
+		user.remove_roles('Test Approver', 'System Manager')
+		workflow_actions = frappe.get_all('Workflow Action', fields=['status'])
+		self.assertEqual(len(workflow_actions), 1)
+		self.assertEqual(workflow_actions[0].status, 'Completed')
+		frappe.set_user('Administrator')
+
+
 	def test_update_docstatus(self):
 		todo = create_new_todo()
 		apply_workflow(todo, 'Approve')

frappe/workflow/doctype/workflow_action/workflow_action.json

@@ -8,9 +8,12 @@
   "status",
   "reference_name",
   "reference_doctype",
-  "user",
   "workflow_state",
-  "completed_by"
+  "column_break_5",
+  "completed_by_role",
+  "completed_by",
+  "permitted_roles",
+  "user"
  ],
  "fields": [
   {
@@ -24,16 +27,14 @@
    "fieldname": "reference_name",
    "fieldtype": "Dynamic Link",
    "label": "Reference Name",
-   "options": "reference_doctype",
-   "search_index": 1
+   "options": "reference_doctype"
   },
   {
    "fieldname": "reference_doctype",
    "fieldtype": "Link",
    "in_list_view": 1,
    "label": "Reference Document Type",
-   "options": "DocType",
-   "search_index": 1
+   "options": "DocType"
   },
   {
    "fieldname": "user",
@@ -47,18 +48,38 @@
    "fieldname": "workflow_state",
    "fieldtype": "Data",
    "hidden": 1,
-   "label": "Workflow State",
-   "search_index": 1
+   "label": "Workflow State"
   },
   {
+   "depends_on": "eval: doc.completed_by",
    "fieldname": "completed_by",
    "fieldtype": "Link",
-   "label": "Completed By",
-   "options": "User"
+   "label": "Completed By User",
+   "options": "User",
+   "read_only": 1
+  },
+  {
+   "fieldname": "column_break_5",
+   "fieldtype": "Column Break"
+  },
+  {
+   "depends_on": "eval: doc.completed_by_role",
+   "fieldname": "completed_by_role",
+   "fieldtype": "Link",
+   "label": "Completed By Role",
+   "options": "Role",
+   "read_only": 1
+  },
+  {
+   "fieldname": "permitted_roles",
+   "fieldtype": "Table MultiSelect",
+   "label": "Permitted Roles",
+   "options": "Workflow Action Permitted Role",
+   "read_only": 1
   }
  ],
  "links": [],
- "modified": "2021-07-01 09:07:52.848618",
+ "modified": "2022-02-23 21:06:45.122258",
  "modified_by": "Administrator",
  "module": "Workflow",
  "name": "Workflow Action",
@@ -72,6 +93,7 @@
  ],
  "sort_field": "modified",
  "sort_order": "DESC",
+ "states": [],
  "title_field": "reference_name",
  "track_changes": 1
 }

frappe/workflow/doctype/workflow_action/workflow_action.py

@@ -13,24 +13,46 @@ from frappe.model.workflow import apply_workflow, get_workflow_name, has_approva
 from frappe.desk.notifications import clear_doctype_notifications
 from frappe.utils.user import get_users_with_role
 from frappe.utils.data import get_link_to_form
+from frappe.query_builder import DocType
 
 class WorkflowAction(Document):
 	pass
 
 
 def on_doctype_update():
-	frappe.db.add_index("Workflow Action", ["status", "user"])
+	# The search order in any use case is no ["reference_name", "reference_doctype", "status"]
+	# The index scan would happen from left to right
+	# so even if status is not in the where clause the index will be used
+	frappe.db.add_index("Workflow Action", ["reference_name", "reference_doctype", "status"])
 
 def get_permission_query_conditions(user):
 	if not user: user = frappe.session.user
 
 	if user == "Administrator": return ""
 
-	return "(`tabWorkflow Action`.`user`='{user}')".format(user=user)
+	roles = frappe.get_roles(user)
+
+	WorkflowAction = DocType("Workflow Action")
+	WorkflowActionPermittedRole = DocType("Workflow Action Permitted Role")
+
+	permitted_workflow_actions = (frappe.qb.from_(WorkflowAction)
+		.join(WorkflowActionPermittedRole)
+		.on(WorkflowAction.name == WorkflowActionPermittedRole.parent)
+		.select(WorkflowAction.name)
+		.where(WorkflowActionPermittedRole.role.isin(roles))
+	).get_sql()
+
+	return f"""(`tabWorkflow Action`.`name` in ({permitted_workflow_actions})
+		or `tabWorkflow Action`.`user`='{user}')
+		and `tabWorkflow Action`.`status`='Open'"""
 
 def has_permission(doc, user):
-	if user not in ['Administrator', doc.user]:
-		return False
+
+	user_roles = set(frappe.get_roles(user))
+
+	permitted_roles = {permitted_role.role for permitted_role in doc.permitted_roles}
+
+	return user == "Administrator" or user_roles.intersection(permitted_roles)
 
 def process_workflow_actions(doc, state):
 	workflow = get_workflow_name(doc.get('doctype'))
@@ -42,19 +64,18 @@ def process_workflow_actions(doc, state):
 
 	if is_workflow_action_already_created(doc): return
 
-	clear_old_workflow_actions(doc)
-	update_completed_workflow_actions(doc)
+	update_completed_workflow_actions(doc, workflow=workflow, workflow_state=get_doc_workflow_state(doc))
 	clear_doctype_notifications('Workflow Action')
 
 	next_possible_transitions = get_next_possible_transitions(workflow, get_doc_workflow_state(doc), doc)
 
 	if not next_possible_transitions: return
 
-	user_data_map = get_users_next_action_data(next_possible_transitions, doc)
+	user_data_map, roles = get_users_next_action_data(next_possible_transitions, doc)
 
 	if not user_data_map: return
 
-	create_workflow_actions_for_users(user_data_map.keys(), doc)
+	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)
@@ -132,20 +153,85 @@ def return_link_expired_page(doc, doc_workflow_state):
 				frappe.bold(frappe.get_value('User', doc.get("modified_by"), 'full_name'))
 			), indicator_color='blue')
 
-def clear_old_workflow_actions(doc, user=None):
+
+def update_completed_workflow_actions(doc, user=None, workflow=None, workflow_state=None):
+	allowed_roles = get_allowed_roles(user, workflow, workflow_state)
+	# There is no transaction leading upto this state
+	# so no older actions to complete
+	if not allowed_roles:
+		return
+	if workflow_action := get_workflow_action_by_role(doc, allowed_roles):
+		update_completed_workflow_actions_using_role(doc, user, allowed_roles, workflow_action)
+	else:
+		# backwards compatibility
+		# for workflow actions saved using user
+		clear_old_workflow_actions_using_user(doc, user)
+		update_completed_workflow_actions_using_user(doc, user)
+
+def get_allowed_roles(user, workflow, workflow_state):
 	user = user if user else frappe.session.user
-	frappe.db.delete("Workflow Action", {
-		"reference_doctype": doc.get("doctype"),
-		"reference_name": doc.get("name"),
-		"user": ("!=", user),
-		"status": "Open"
-	})
 
-def update_completed_workflow_actions(doc, user=None):
+	allowed_roles = frappe.get_all('Workflow Transition',
+		fields='allowed',
+		filters=[
+			['parent', '=', workflow],
+			['next_state', '=', workflow_state]
+		],
+		pluck = 'allowed')
+
+	user_roles = set(frappe.get_roles(user))
+	return set(allowed_roles).intersection(user_roles)
+
+def get_workflow_action_by_role(doc, allowed_roles):
+	WorkflowAction = DocType("Workflow Action")
+	WorkflowActionPermittedRole = DocType("Workflow Action Permitted Role")
+	return (frappe.qb.from_(WorkflowAction).join(WorkflowActionPermittedRole)
+		.on(WorkflowAction.name == WorkflowActionPermittedRole.parent)
+		.select(WorkflowAction.name, WorkflowActionPermittedRole.role)
+		.where((WorkflowAction.reference_name == doc.get('name'))
+			& (WorkflowAction.reference_doctype == doc.get('doctype'))
+			& (WorkflowAction.status == 'Open')
+			& (WorkflowActionPermittedRole.role.isin(list(allowed_roles))))
+		.orderby(WorkflowActionPermittedRole.role).limit(1)).run(as_dict=True)
+
+def update_completed_workflow_actions_using_role(doc, user=None, allowed_roles = set(), workflow_action=None):
 	user = user if user else frappe.session.user
-	frappe.db.sql("""UPDATE `tabWorkflow Action` SET `status`='Completed', `completed_by`=%s
-		WHERE `reference_doctype`=%s AND `reference_name`=%s AND `user`=%s AND `status`='Open'""",
-		(user, doc.get('doctype'), doc.get('name'), user))
+	WorkflowAction = DocType("Workflow Action")
+
+	if not workflow_action:
+		return
+
+	(frappe.qb.update(WorkflowAction)
+		.set(WorkflowAction.status, 'Completed')
+		.set(WorkflowAction.completed_by, user)
+		.set(WorkflowAction.completed_by_role, workflow_action[0].role)
+		.where(WorkflowAction.name == workflow_action[0].name)
+	).run()
+
+def clear_old_workflow_actions_using_user(doc, user=None):
+	user = user if user else frappe.session.user
+
+	if frappe.db.has_column('Workflow Action', 'user'):
+		frappe.db.delete("Workflow Action", {
+			"reference_name": doc.get("name"),
+			"reference_doctype": doc.get("doctype"),
+			"status": "Open",
+			"user": ("!=", user)
+		})
+
+def update_completed_workflow_actions_using_user(doc, user=None):
+	user = user or frappe.session.user
+
+	if frappe.db.has_column('Workflow Action', 'user'):
+		WorkflowAction = DocType("Workflow Action")
+		(frappe.qb.update(WorkflowAction)
+			.set(WorkflowAction.status, 'Completed')
+			.set(WorkflowAction.completed_by, user)
+			.where((WorkflowAction.reference_name == doc.get('name'))
+				& (WorkflowAction.reference_doctype == doc.get('doctype'))
+				& (WorkflowAction.status == 'Open')
+				& (WorkflowAction.user == user))
+		).run()
 
 def get_next_possible_transitions(workflow_name, state, doc=None):
 	transitions = frappe.get_all('Workflow Transition',
@@ -167,8 +253,10 @@ def get_next_possible_transitions(workflow_name, state, doc=None):
 	return transitions_to_return
 
 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:
@@ -182,19 +270,24 @@ def get_users_next_action_data(transitions, doc):
 				'action_name': transition.action,
 				'action_link': get_workflow_action_url(transition.action, doc, user)
 			}))
-	return user_data_map
+	return user_data_map, roles
 
 
-def create_workflow_actions_for_users(users, doc):
-	for user in users:
-		frappe.get_doc({
-			'doctype': 'Workflow Action',
-			'reference_doctype': doc.get('doctype'),
-			'reference_name': doc.get('name'),
-			'workflow_state': get_doc_workflow_state(doc),
-			'status': 'Open',
-			'user': user
-		}).insert(ignore_permissions=True)
+def create_workflow_actions_for_roles(roles, doc):
+	workflow_action = frappe.get_doc({
+		'doctype': 'Workflow Action',
+		'reference_doctype': doc.get('doctype'),
+		'reference_name': doc.get('name'),
+		'workflow_state': get_doc_workflow_state(doc),
+		'status': 'Open',
+	})
+
+	for role in roles:
+		workflow_action.append('permitted_roles', {
+			'role': role
+		})
+
+	workflow_action.insert(ignore_permissions=True)
 
 def send_workflow_action_email(users_data, doc):
 	common_args = get_common_email_args(doc)
@@ -249,18 +342,20 @@ def get_confirm_workflow_action_url(doc, action, user):
 def is_workflow_action_already_created(doc):
 	return frappe.db.exists({
 		'doctype': 'Workflow Action',
-		'reference_doctype': doc.get('doctype'),
 		'reference_name': doc.get('name'),
-		'workflow_state': get_doc_workflow_state(doc)
+		'reference_doctype': doc.get('doctype'),
+		'workflow_state': get_doc_workflow_state(doc),
 	})
 
 def clear_workflow_actions(doctype, name):
 	if not (doctype and name):
 		return
-	frappe.db.delete("Workflow Action", {
-		"reference_doctype": doctype,
-		"reference_name": name
-	})
+	frappe.db.delete("Workflow Action", filters = {
+			"reference_name": name,
+			"reference_doctype": doctype,
+		}
+	)
+
 def get_doc_workflow_state(doc):
 	workflow_name = get_workflow_name(doc.get('doctype'))
 	workflow_state_field = get_workflow_state_field(workflow_name)

frappe/workflow/doctype/workflow_action_permitted_role/workflow_action_permitted_role.json

@@ -0,0 +1,33 @@
+{
+ "actions": [],
+ "allow_rename": 1,
+ "autoname": "hash",
+ "creation": "2022-02-21 20:28:05.662187",
+ "doctype": "DocType",
+ "editable_grid": 1,
+ "engine": "InnoDB",
+ "field_order": [
+  "role"
+ ],
+ "fields": [
+  {
+   "fieldname": "role",
+   "fieldtype": "Link",
+   "label": "Role",
+   "options": "Role"
+  }
+ ],
+ "index_web_pages_for_search": 1,
+ "istable": 1,
+ "links": [],
+ "modified": "2022-02-21 20:28:05.662187",
+ "modified_by": "Administrator",
+ "module": "Workflow",
+ "name": "Workflow Action Permitted Role",
+ "naming_rule": "Random",
+ "owner": "Administrator",
+ "permissions": [],
+ "sort_field": "modified",
+ "sort_order": "DESC",
+ "states": []
+}

frappe/workflow/doctype/workflow_action_permitted_role/workflow_action_permitted_role.py

@@ -0,0 +1,8 @@
+# Copyright (c) 2022, Frappe Technologies and contributors
+# For license information, please see license.txt
+
+# import frappe
+from frappe.model.document import Document
+
+class WorkflowActionPermittedRole(Document):
+	pass