From 6c6ff2c16dc7b05bc256416417d74f91bc48984b Mon Sep 17 00:00:00 2001 From: shadrak gurupnor Date: Tue, 31 Aug 2021 19:41:22 +0530 Subject: [PATCH] fix: overriding of owner of doc feat: Fixed the dilemma of owner field in ToDo document --- .../assignment_rule/assignment_rule.py | 2 +- .../assignment_rule/test_assignment_rule.py | 28 +++++++++---------- frappe/core/doctype/user/test_user.py | 2 +- frappe/core/doctype/user/user.py | 2 +- frappe/core/notifications.py | 2 +- frappe/desk/doctype/event/test_event.py | 2 +- frappe/desk/doctype/todo/todo.json | 22 +++++++-------- frappe/desk/doctype/todo/todo.py | 18 ++++++------ frappe/desk/form/assign_to.py | 28 +++++++++---------- frappe/desk/listview.py | 6 ++-- frappe/model/document.py | 12 ++++++-- frappe/tests/test_assign.py | 2 +- frappe/tests/test_document.py | 19 +++++++++++++ 13 files changed, 85 insertions(+), 60 deletions(-) diff --git a/frappe/automation/doctype/assignment_rule/assignment_rule.py b/frappe/automation/doctype/assignment_rule/assignment_rule.py index a3e27d4da5..50a6f8b17e 100644 --- a/frappe/automation/doctype/assignment_rule/assignment_rule.py +++ b/frappe/automation/doctype/assignment_rule/assignment_rule.py @@ -109,7 +109,7 @@ class AssignmentRule(Document): user = d.user, count = frappe.db.count('ToDo', dict( reference_type = self.document_type, - owner = d.user, + allocated_to = d.user, status = "Open")) )) diff --git a/frappe/automation/doctype/assignment_rule/test_assignment_rule.py b/frappe/automation/doctype/assignment_rule/test_assignment_rule.py index 1c9e177f94..84b8dbd3c8 100644 --- a/frappe/automation/doctype/assignment_rule/test_assignment_rule.py +++ b/frappe/automation/doctype/assignment_rule/test_assignment_rule.py @@ -30,7 +30,7 @@ class TestAutoAssign(unittest.TestCase): reference_type = 'Note', reference_name = note.name, status = 'Open' - ), 'owner'), 'test@example.com') + ), 'allocated_to'), 'test@example.com') note = make_note(dict(public=1)) @@ -39,7 +39,7 @@ class TestAutoAssign(unittest.TestCase): reference_type = 'Note', reference_name = note.name, status = 'Open' - ), 'owner'), 'test1@example.com') + ), 'allocated_to'), 'test1@example.com') clear_assignments() @@ -51,7 +51,7 @@ class TestAutoAssign(unittest.TestCase): reference_type = 'Note', reference_name = note.name, status = 'Open' - ), 'owner'), 'test2@example.com') + ), 'allocated_to'), 'test2@example.com') # check loop back to first user note = make_note(dict(public=1)) @@ -60,7 +60,7 @@ class TestAutoAssign(unittest.TestCase): reference_type = 'Note', reference_name = note.name, status = 'Open' - ), 'owner'), 'test@example.com') + ), 'allocated_to'), 'test@example.com') def test_load_balancing(self): self.assignment_rule.rule = 'Load Balancing' @@ -71,11 +71,11 @@ class TestAutoAssign(unittest.TestCase): # check if each user has 10 assignments (?) for user in ('test@example.com', 'test1@example.com', 'test2@example.com'): - self.assertEqual(len(frappe.get_all('ToDo', dict(owner = user, reference_type = 'Note'))), 10) + self.assertEqual(len(frappe.get_all('ToDo', dict(allocated_to = user, reference_type = 'Note'))), 10) # clear 5 assignments for first user # can't do a limit in "delete" since postgres does not support it - for d in frappe.get_all('ToDo', dict(reference_type = 'Note', owner = 'test@example.com'), limit=5): + for d in frappe.get_all('ToDo', dict(reference_type = 'Note', allocated_to = 'test@example.com'), limit=5): frappe.db.delete("ToDo", {"name": d.name}) # add 5 more assignments @@ -84,7 +84,7 @@ class TestAutoAssign(unittest.TestCase): # check if each user still has 10 assignments for user in ('test@example.com', 'test1@example.com', 'test2@example.com'): - self.assertEqual(len(frappe.get_all('ToDo', dict(owner = user, reference_type = 'Note'))), 10) + self.assertEqual(len(frappe.get_all('ToDo', dict(allocated_to = user, reference_type = 'Note'))), 10) def test_based_on_field(self): self.assignment_rule.rule = 'Based on Field' @@ -119,7 +119,7 @@ class TestAutoAssign(unittest.TestCase): reference_type = 'Note', reference_name = note.name, status = 'Open' - ), 'owner'), None) + ), 'allocated_to'), None) def test_clear_assignment(self): note = make_note(dict(public=1)) @@ -132,7 +132,7 @@ class TestAutoAssign(unittest.TestCase): ))[0] todo = frappe.get_doc('ToDo', todo['name']) - self.assertEqual(todo.owner, 'test@example.com') + self.assertEqual(todo.allocated_to, 'test@example.com') # test auto unassign note.public = 0 @@ -154,7 +154,7 @@ class TestAutoAssign(unittest.TestCase): ))[0] todo = frappe.get_doc('ToDo', todo['name']) - self.assertEqual(todo.owner, 'test@example.com') + self.assertEqual(todo.allocated_to, 'test@example.com') note.content="Closed" note.save() @@ -164,7 +164,7 @@ class TestAutoAssign(unittest.TestCase): # check if todo is closed self.assertEqual(todo.status, 'Closed') # check if closed todo retained assignment - self.assertEqual(todo.owner, 'test@example.com') + self.assertEqual(todo.allocated_to, 'test@example.com') def check_multiple_rules(self): note = make_note(dict(public=1, notify_on_login=1)) @@ -174,7 +174,7 @@ class TestAutoAssign(unittest.TestCase): reference_type = 'Note', reference_name = note.name, status = 'Open' - ), 'owner'), 'test@example.com') + ), 'allocated_to'), 'test@example.com') def check_assignment_rule_scheduling(self): frappe.db.delete("Assignment Rule") @@ -192,7 +192,7 @@ class TestAutoAssign(unittest.TestCase): reference_type = 'Note', reference_name = note.name, status = 'Open' - ), 'owner'), ['test@example.com', 'test1@example.com', 'test2@example.com']) + ), 'allocated_to'), ['test@example.com', 'test1@example.com', 'test2@example.com']) frappe.flags.assignment_day = "Friday" note = make_note(dict(public=1)) @@ -201,7 +201,7 @@ class TestAutoAssign(unittest.TestCase): reference_type = 'Note', reference_name = note.name, status = 'Open' - ), 'owner'), ['test3@example.com']) + ), 'allocated_to'), ['test3@example.com']) def test_assignment_rule_condition(self): frappe.db.delete("Assignment Rule") diff --git a/frappe/core/doctype/user/test_user.py b/frappe/core/doctype/user/test_user.py index b3c85b22a1..d1291acfc4 100644 --- a/frappe/core/doctype/user/test_user.py +++ b/frappe/core/doctype/user/test_user.py @@ -70,7 +70,7 @@ class TestUser(unittest.TestCase): delete_contact("_test@example.com") delete_doc("User", "_test@example.com") - self.assertTrue(not frappe.db.sql("""select * from `tabToDo` where owner=%s""", + self.assertTrue(not frappe.db.sql("""select * from `tabToDo` where allocated_to=%s""", ("_test@example.com",))) from frappe.core.doctype.role.test_role import test_records as role_records diff --git a/frappe/core/doctype/user/user.py b/frappe/core/doctype/user/user.py index 2d2ad1fed9..ef7845d3b0 100644 --- a/frappe/core/doctype/user/user.py +++ b/frappe/core/doctype/user/user.py @@ -363,7 +363,7 @@ class User(Document): frappe.local.login_manager.logout(user=self.name) # delete todos - frappe.db.delete("ToDo", {"owner": self.name}) + frappe.db.delete("ToDo", {"allocated_to": self.name}) todo_table = DocType("ToDo") ( frappe.qb.update(todo_table) diff --git a/frappe/core/notifications.py b/frappe/core/notifications.py index 939cf52911..be3e723af6 100644 --- a/frappe/core/notifications.py +++ b/frappe/core/notifications.py @@ -23,7 +23,7 @@ def get_things_todo(as_list=False): data = frappe.get_list("ToDo", fields=["name", "description"] if as_list else "count(*)", filters=[["ToDo", "status", "=", "Open"]], - or_filters=[["ToDo", "owner", "=", frappe.session.user], + or_filters=[["ToDo", "allocated_to", "=", frappe.session.user], ["ToDo", "assigned_by", "=", frappe.session.user]], as_list=True) diff --git a/frappe/desk/doctype/event/test_event.py b/frappe/desk/doctype/event/test_event.py index 6b7f6ee471..b0269a80cc 100644 --- a/frappe/desk/doctype/event/test_event.py +++ b/frappe/desk/doctype/event/test_event.py @@ -93,7 +93,7 @@ class TestEvent(unittest.TestCase): # Remove an assignment todo = frappe.get_doc("ToDo", {"reference_type": ev.doctype, "reference_name": ev.name, - "owner": self.test_user}) + "allocated_to": self.test_user}) todo.status = "Cancelled" todo.save() diff --git a/frappe/desk/doctype/todo/todo.json b/frappe/desk/doctype/todo/todo.json index 15e0e4abe1..e6a1671c19 100644 --- a/frappe/desk/doctype/todo/todo.json +++ b/frappe/desk/doctype/todo/todo.json @@ -13,7 +13,7 @@ "column_break_2", "color", "date", - "owner", + "allocated_to", "description_section", "description", "section_break_6", @@ -69,15 +69,6 @@ "oldfieldname": "date", "oldfieldtype": "Date" }, - { - "fieldname": "owner", - "fieldtype": "Link", - "ignore_user_permissions": 1, - "in_global_search": 1, - "in_standard_filter": 1, - "label": "Allocated To", - "options": "User" - }, { "fieldname": "description_section", "fieldtype": "Section Break" @@ -153,12 +144,21 @@ "label": "Assignment Rule", "options": "Assignment Rule", "read_only": 1 + }, + { + "fieldname": "allocated_to", + "fieldtype": "Link", + "ignore_user_permissions": 1, + "in_global_search": 1, + "in_standard_filter": 1, + "label": "Allocated To", + "options": "User" } ], "icon": "fa fa-check", "idx": 2, "links": [], - "modified": "2020-01-14 17:04:36.971002", + "modified": "2021-09-02 16:27:32.173875", "modified_by": "Administrator", "module": "Desk", "name": "ToDo", diff --git a/frappe/desk/doctype/todo/todo.py b/frappe/desk/doctype/todo/todo.py index 6f3f4160e6..eabb28a6f3 100644 --- a/frappe/desk/doctype/todo/todo.py +++ b/frappe/desk/doctype/todo/todo.py @@ -16,10 +16,10 @@ class ToDo(Document): self._assignment = None if self.is_new(): - if self.assigned_by == self.owner: + if self.assigned_by == self.allocated_to: assignment_message = frappe._("{0} self assigned this task: {1}").format(get_fullname(self.assigned_by), self.description) else: - assignment_message = frappe._("{0} assigned {1}: {2}").format(get_fullname(self.assigned_by), get_fullname(self.owner), self.description) + assignment_message = frappe._("{0} assigned {1}: {2}").format(get_fullname(self.assigned_by), get_fullname(self.allocated_to), self.description) self._assignment = { "text": assignment_message, @@ -29,12 +29,12 @@ class ToDo(Document): else: # NOTE the previous value is only available in validate method if self.get_db_value("status") != self.status: - if self.owner == frappe.session.user: + if self.allocated_to == frappe.session.user: removal_message = frappe._("{0} removed their assignment.").format( get_fullname(frappe.session.user)) else: removal_message = frappe._("Assignment of {0} removed by {1}").format( - get_fullname(self.owner), get_fullname(frappe.session.user)) + get_fullname(self.allocated_to), get_fullname(frappe.session.user)) self._assignment = { "text": removal_message, @@ -75,7 +75,7 @@ class ToDo(Document): "reference_name": self.reference_name, "status": ("!=", "Cancelled") }, - fields=["owner"], as_list=True)] + fields=["allocated_to"], as_list=True)] assignments.reverse() frappe.db.set_value(self.reference_type, self.reference_name, @@ -98,8 +98,8 @@ class ToDo(Document): def get_owners(cls, filters=None): """Returns list of owners after applying filters on todo's. """ - rows = frappe.get_all(cls.DocType, filters=filters or {}, fields=['owner']) - return [parse_addr(row.owner)[1] for row in rows if row.owner] + rows = frappe.get_all(cls.DocType, filters=filters or {}, fields=['allocated_to']) + return [parse_addr(row.allocated_to)[1] for row in rows if row.allocated_to] # NOTE: todo is viewable if a user is an owner, or set as assigned_to value, or has any role that is allowed to access ToDo doctype. def on_doctype_update(): @@ -115,7 +115,7 @@ def get_permission_query_conditions(user): if any(check in todo_roles for check in frappe.get_roles(user)): return None else: - return """(`tabToDo`.owner = {user} or `tabToDo`.assigned_by = {user})"""\ + return """(`tabToDo`.allocated_to = {user} or `tabToDo`.assigned_by = {user})"""\ .format(user=frappe.db.escape(user)) def has_permission(doc, ptype="read", user=None): @@ -127,7 +127,7 @@ def has_permission(doc, ptype="read", user=None): if any(check in todo_roles for check in frappe.get_roles(user)): return True else: - return doc.owner==user or doc.assigned_by==user + return doc.allocated_to==user or doc.assigned_by==user @frappe.whitelist() def new_todo(description): diff --git a/frappe/desk/form/assign_to.py b/frappe/desk/form/assign_to.py index bf77170eeb..50e86b75d6 100644 --- a/frappe/desk/form/assign_to.py +++ b/frappe/desk/form/assign_to.py @@ -19,7 +19,7 @@ def get(args=None): if not args: args = frappe.local.form_dict - return frappe.get_all('ToDo', fields=['owner', 'name'], filters=dict( + return frappe.get_all('ToDo', fields=['allocated_to', 'name'], filters=dict( reference_type = args.get('doctype'), reference_name = args.get('name'), status = ('!=', 'Cancelled') @@ -48,7 +48,7 @@ def add(args=None): "reference_type": args['doctype'], "reference_name": args['name'], "status": "Open", - "owner": assign_to + "allocated_to": assign_to } if frappe.get_all("ToDo", filters=filters): @@ -61,7 +61,7 @@ def add(args=None): d = frappe.get_doc({ "doctype": "ToDo", - "owner": assign_to, + "allocated_to": assign_to, "reference_type": args['doctype'], "reference_name": args['name'], "description": args.get('description'), @@ -87,7 +87,7 @@ def add(args=None): follow_document(args['doctype'], args['name'], assign_to) # notify - notify_assignment(d.assigned_by, d.owner, d.reference_type, d.reference_name, action='ASSIGN', + notify_assignment(d.assigned_by, d.allocated_to, d.reference_type, d.reference_name, action='ASSIGN', description=args.get("description")) if shared_with_users: @@ -112,13 +112,13 @@ def add_multiple(args=None): add(args) def close_all_assignments(doctype, name): - assignments = frappe.db.get_all('ToDo', fields=['owner'], filters = + assignments = frappe.db.get_all('ToDo', fields=['allocated_to'], filters = dict(reference_type = doctype, reference_name = name, status=('!=', 'Cancelled'))) if not assignments: return False for assign_to in assignments: - set_status(doctype, name, assign_to.owner, status="Closed") + set_status(doctype, name, assign_to.allocated_to, status="Closed") return True @@ -130,13 +130,13 @@ def set_status(doctype, name, assign_to, status="Cancelled"): """remove from todo""" try: todo = frappe.db.get_value("ToDo", {"reference_type":doctype, - "reference_name":name, "owner":assign_to, "status": ('!=', status)}) + "reference_name":name, "allocated_to":assign_to, "status": ('!=', status)}) if todo: todo = frappe.get_doc("ToDo", todo) todo.status = status todo.save(ignore_permissions=True) - notify_assignment(todo.assigned_by, todo.owner, todo.reference_type, todo.reference_name) + notify_assignment(todo.assigned_by, todo.allocated_to, todo.reference_type, todo.reference_name) except frappe.DoesNotExistError: pass @@ -150,25 +150,25 @@ def clear(doctype, name): ''' Clears assignments, return False if not assigned. ''' - assignments = frappe.db.get_all('ToDo', fields=['owner'], filters = + assignments = frappe.db.get_all('ToDo', fields=['allocated_to'], filters = dict(reference_type = doctype, reference_name = name)) if not assignments: return False for assign_to in assignments: - set_status(doctype, name, assign_to.owner, "Cancelled") + set_status(doctype, name, assign_to.allocated_to, "Cancelled") return True -def notify_assignment(assigned_by, owner, doc_type, doc_name, action='CLOSE', +def notify_assignment(assigned_by, allocated_to, doc_type, doc_name, action='CLOSE', description=None): """ Notify assignee that there is a change in assignment """ - if not (assigned_by and owner and doc_type and doc_name): return + if not (assigned_by and allocated_to and doc_type and doc_name): return # return if self assigned or user disabled - if assigned_by == owner or not frappe.db.get_value('User', owner, 'enabled'): + if assigned_by == allocated_to or not frappe.db.get_value('User', allocated_to, 'enabled'): return # Search for email address in description -- i.e. assignee @@ -194,7 +194,7 @@ def notify_assignment(assigned_by, owner, doc_type, doc_name, action='CLOSE', 'email_content': description_html } - enqueue_create_notification(owner, notification_doc) + enqueue_create_notification(allocated_to, notification_doc) def format_message_for_assign_to(users): return "

" + "
".join(users) \ No newline at end of file diff --git a/frappe/desk/listview.py b/frappe/desk/listview.py index 43ad104f0d..3d6f1254a2 100644 --- a/frappe/desk/listview.py +++ b/frappe/desk/listview.py @@ -29,16 +29,16 @@ def get_group_by_count(doctype, current_filters, field): subquery = frappe.get_all(doctype, filters=current_filters, run=False) if field == 'assigned_to': subquery_condition = ' and `tabToDo`.reference_name in ({subquery})'.format(subquery = subquery) - return frappe.db.sql("""select `tabToDo`.owner as name, count(*) as count + return frappe.db.sql("""select `tabToDo`.allocated_to as name, count(*) as count from `tabToDo`, `tabUser` where `tabToDo`.status!='Cancelled' and - `tabToDo`.owner = `tabUser`.name and + `tabToDo`.allocated_to = `tabUser`.name and `tabUser`.user_type = 'System User' {subquery_condition} group by - `tabToDo`.owner + `tabToDo`.allocated_to order by count desc limit 50""".format(subquery_condition = subquery_condition), as_dict=True) diff --git a/frappe/model/document.py b/frappe/model/document.py index 1f079feedc..4312378995 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -470,8 +470,8 @@ class Document(BaseDocument): self.modified_by = frappe.session.user if not self.creation: self.creation = self.modified - if not self.owner: - self.owner = self.modified_by + if self.is_new(): + self.owner = self.flags.owner or self.modified_by for d in self.get_all_children(): d.modified = self.modified @@ -501,6 +501,7 @@ class Document(BaseDocument): self._sanitize_content() self._save_passwords() self.validate_workflow() + self.validate_owner() children = self.get_all_children() for d in children: @@ -543,6 +544,11 @@ class Document(BaseDocument): if not self._action == 'save': set_workflow_state_on_action(self, workflow, self._action) + def validate_owner(self): + """Validate if the owner of the Document has changed""" + if not self.is_new() and self.has_value_changed('owner'): + frappe.throw(_('Document owner cannot be changed')) + def validate_set_only_once(self): """Validate that fields are not changed if not in insert""" set_only_once_fields = self.meta.get_set_only_once_fields() @@ -1342,7 +1348,7 @@ class Document(BaseDocument): def get_assigned_users(self): assignments = frappe.get_all('ToDo', - fields=['owner'], + fields=['allocated_to'], filters={ 'reference_type': self.doctype, 'reference_name': self.name, diff --git a/frappe/tests/test_assign.py b/frappe/tests/test_assign.py index 05bf7e2fb3..971f9ce071 100644 --- a/frappe/tests/test_assign.py +++ b/frappe/tests/test_assign.py @@ -13,7 +13,7 @@ class TestAssign(unittest.TestCase): added = assign(todo, "test@example.com") - self.assertTrue("test@example.com" in [d.owner for d in added]) + self.assertTrue("test@example.com" in [d.allocated_to for d in added]) removed = frappe.desk.form.assign_to.remove(todo.doctype, todo.name, "test@example.com") diff --git a/frappe/tests/test_document.py b/frappe/tests/test_document.py index 29cec8b230..46638f5bf2 100644 --- a/frappe/tests/test_document.py +++ b/frappe/tests/test_document.py @@ -252,3 +252,22 @@ class TestDocument(unittest.TestCase): 'currency': 100000 }) self.assertEquals(d.get_formatted('currency', currency='INR', format="#,###.##"), '₹ 100,000.00') + + def test_owner_changed(self): + frappe.delete_doc_if_exists("User", "hello@example.com") + frappe.set_user("Administrator") + + d = frappe.get_doc({ + "doctype": "User", + "email": "hello@example.com", + "first_name": "John" + }) + d.insert() + self.assertEqual(frappe.db.get_value("User", d.owner), d.owner) + + d.set("owner", "johndoe@gmail.com") + self.assertRaises(frappe.ValidationError, d.save) + + d.reload() + d.save() + self.assertEqual(frappe.db.get_value("User", d.owner), d.owner)