From 62a8774bb10f6d122ffad1775a3f2848b3903607 Mon Sep 17 00:00:00 2001 From: Rushabh Mehta Date: Mon, 29 Jun 2020 08:13:23 +0530 Subject: [PATCH] feat(role permission): if desk access is removed from a role, attempt to remove it from users too (#10838) * feat(minor): if desk access is removed from a role, attempt to remove it from users too * fix(minor): exception for install; * fix(minor): exception for install --- frappe/core/doctype/role/role.py | 24 ++++++++++++++++++------ frappe/core/doctype/role/test_role.py | 25 +++++++++++++++++++++++++ frappe/model/document.py | 5 +++++ frappe/tests/test_document.py | 7 +++++++ 4 files changed, 55 insertions(+), 6 deletions(-) diff --git a/frappe/core/doctype/role/role.py b/frappe/core/doctype/role/role.py index 7ce2537da3..657340ec24 100644 --- a/frappe/core/doctype/role/role.py +++ b/frappe/core/doctype/role/role.py @@ -22,16 +22,28 @@ class Role(Document): frappe.db.sql("delete from `tabHas Role` where role = %s", self.name) frappe.clear_cache() + def on_update(self): + '''update system user desk access if this has changed in this update''' + if frappe.flags.in_install: return + if self.has_value_changed('desk_access'): + for user_name in get_users(self.name): + user = frappe.get_doc('User', user_name) + user_type = user.user_type + user.set_system_user() + if user_type != user.user_type: + user.save() + # Get email addresses of all users that have been assigned this role def get_emails_from_role(role): emails = [] - users = frappe.get_list("Has Role", filters={"role": role, "parenttype": "User"}, - fields=["parent"]) - - for user in users: - user_email, enabled = frappe.db.get_value("User", user.parent, ["email", "enabled"]) + for user in get_users(role): + user_email, enabled = frappe.db.get_value("User", user, ["email", "enabled"]) if enabled and user_email not in ["admin@example.com", "guest@example.com"]: emails.append(user_email) - return emails \ No newline at end of file + return emails + +def get_users(role): + return [d.parent for d in frappe.get_all("Has Role", filters={"role": role, "parenttype": "User"}, + fields=["parent"])] diff --git a/frappe/core/doctype/role/test_role.py b/frappe/core/doctype/role/test_role.py index 31efb5d4e8..6459a72c98 100644 --- a/frappe/core/doctype/role/test_role.py +++ b/frappe/core/doctype/role/test_role.py @@ -23,3 +23,28 @@ class TestUser(unittest.TestCase): frappe.get_doc("User", "test@example.com").add_roles("_Test Role 3") self.assertTrue("_Test Role 3" in frappe.get_roles("test@example.com")) + + def test_change_desk_access(self): + '''if we change desk acecss from role, remove from user''' + frappe.delete_doc_if_exists('User', 'test-user-for-desk-access@example.com') + frappe.delete_doc_if_exists('Role', 'desk-access-test') + user = frappe.get_doc(dict( + doctype='User', + email='test-user-for-desk-access@example.com', + first_name='test')).insert() + role = frappe.get_doc(dict( + doctype = 'Role', + role_name = 'desk-access-test', + desk_access = 0 + )).insert() + user.add_roles(role.name) + user.save() + self.assertTrue(user.user_type=='Website User') + role.desk_access = 1 + role.save() + user.reload() + self.assertTrue(user.user_type=='System User') + role.desk_access = 0 + role.save() + user.reload() + self.assertTrue(user.user_type=='Website User') diff --git a/frappe/model/document.py b/frappe/model/document.py index 24450f0cc6..30d3442954 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -396,6 +396,11 @@ class Document(BaseDocument): def get_doc_before_save(self): return getattr(self, '_doc_before_save', None) + def has_value_changed(self, fieldname): + '''Returns true if value is changed before and after saving''' + previous = self.get_doc_before_save() + return previous.get(fieldname)!=self.get(fieldname) if previous else True + def set_new_name(self, force=False, set_name=None, set_child_names=True): """Calls `frappe.naming.set_new_name` for parent and child docs.""" if self.flags.name_set and not force: diff --git a/frappe/tests/test_document.py b/frappe/tests/test_document.py index 470ab35fb6..c96076cfba 100644 --- a/frappe/tests/test_document.py +++ b/frappe/tests/test_document.py @@ -66,6 +66,13 @@ class TestDocument(unittest.TestCase): self.assertEqual(frappe.db.get_value(d.doctype, d.name, "subject"), "subject changed") + def test_value_changed(self): + d = self.test_insert() + d.subject = "subject changed again" + d.save() + self.assertTrue(d.has_value_changed('subject')) + self.assertFalse(d.has_value_changed('event_type')) + def test_mandatory(self): # TODO: recheck if it is OK to force delete frappe.delete_doc_if_exists("User", "test_mandatory@example.com", 1)