From fda544f4242a97caf2d5a980c150a216db002bc6 Mon Sep 17 00:00:00 2001 From: Mohammad Hasnain Mohsin Rajan Date: Fri, 25 Mar 2022 02:01:15 +0530 Subject: [PATCH] refactor!: make automatically following documents optional (#16030) * fix: make automatically following documents optional * fix: optimize email triggers for document followed * test: add tests for document follow settings * test: sync global search before testing * fix: extend pypika's cast function to mimic varchar cast in MariaDB Co-authored-by: Suraj Shetty <13928957+surajshetty3416@users.noreply.github.com> Co-authored-by: phot0n --- frappe/core/doctype/user/user.json | 49 +++- frappe/desk/form/assign_to.py | 3 +- frappe/desk/form/document_follow.py | 63 +++-- frappe/desk/form/utils.py | 4 +- frappe/desk/like.py | 3 +- .../document_follow/test_document_follow.py | 223 ++++++++++++++++-- frappe/model/document.py | 6 +- frappe/query_builder/functions.py | 22 ++ frappe/share.py | 3 +- frappe/tests/test_global_search.py | 2 +- 10 files changed, 327 insertions(+), 51 deletions(-) diff --git a/frappe/core/doctype/user/user.json b/frappe/core/doctype/user/user.json index 9e9529cd5e..642a392a58 100644 --- a/frappe/core/doctype/user/user.json +++ b/frappe/core/doctype/user/user.json @@ -2,7 +2,7 @@ "actions": [], "allow_import": 1, "allow_rename": 1, - "creation": "2014-03-11 14:55:00", + "creation": "2022-01-10 17:29:51.672911", "description": "Represents a User in the system.", "doctype": "DocType", "engine": "InnoDB", @@ -48,6 +48,12 @@ "document_follow_notifications_section", "document_follow_notify", "document_follow_frequency", + "column_break_75", + "follow_created_documents", + "follow_commented_documents", + "follow_liked_documents", + "follow_assigned_documents", + "follow_shared_documents", "email_settings", "email_signature", "thread_notify", @@ -606,6 +612,45 @@ "fieldtype": "Link", "label": "Module Profile", "options": "Module Profile" + }, + { + "fieldname": "column_break_75", + "fieldtype": "Column Break" + }, + { + "default": "0", + "depends_on": "eval:(doc.document_follow_notify== 1)", + "fieldname": "follow_created_documents", + "fieldtype": "Check", + "label": "Auto follow documents that you create" + }, + { + "default": "0", + "depends_on": "eval:(doc.document_follow_notify== 1)", + "fieldname": "follow_commented_documents", + "fieldtype": "Check", + "label": "Auto follow documents that you comment on" + }, + { + "default": "0", + "depends_on": "eval:(doc.document_follow_notify== 1)", + "fieldname": "follow_liked_documents", + "fieldtype": "Check", + "label": "Auto follow documents that you Like" + }, + { + "default": "0", + "depends_on": "eval:(doc.document_follow_notify== 1)", + "fieldname": "follow_shared_documents", + "fieldtype": "Check", + "label": "Auto follow documents that are shared with you" + }, + { + "default": "0", + "depends_on": "eval:(doc.document_follow_notify== 1)", + "fieldname": "follow_assigned_documents", + "fieldtype": "Check", + "label": "Auto follow documents that are assigned to you" } ], "icon": "fa fa-user", @@ -704,4 +749,4 @@ "states": [], "title_field": "full_name", "track_changes": 1 -} \ No newline at end of file +} diff --git a/frappe/desk/form/assign_to.py b/frappe/desk/form/assign_to.py index 049d33c1ec..d79927a506 100644 --- a/frappe/desk/form/assign_to.py +++ b/frappe/desk/form/assign_to.py @@ -84,7 +84,8 @@ def add(args=None): shared_with_users.append(assign_to) # make this document followed by assigned user - follow_document(args['doctype'], args['name'], assign_to) + if frappe.get_cached_value("User", assign_to, "follow_assigned_documents"): + follow_document(args['doctype'], args['name'], assign_to) # notify notify_assignment(d.assigned_by, d.allocated_to, d.reference_type, d.reference_name, action='ASSIGN', diff --git a/frappe/desk/form/document_follow.py b/frappe/desk/form/document_follow.py index 14970092d0..7dd2b64c21 100644 --- a/frappe/desk/form/document_follow.py +++ b/frappe/desk/form/document_follow.py @@ -6,7 +6,7 @@ import frappe.utils from frappe.utils import get_url_to_form from frappe.model import log_types from frappe import _ -from itertools import groupby +from frappe.query_builder import DocType @frappe.whitelist() def update_follow(doctype, doc_name, following): @@ -94,33 +94,50 @@ def send_document_follow_mails(frequency): call method to send mail ''' - users = frappe.get_list("Document Follow", - fields=["*"]) + user_list = get_user_list(frequency) - sorted_users = sorted(users, key=lambda k: k['user']) + for user in user_list: + message, valid_document_follows = get_message_for_user(frequency, user) + if message: + send_email_alert(user, valid_document_follows, message) + # send an email if we have already spent resources creating the message + # nosemgrep + frappe.db.commit() - grouped_by_user = {} - for k, v in groupby(sorted_users, key=lambda k: k['user']): - grouped_by_user[k] = list(v) +def get_user_list(frequency): + DocumentFollow = DocType('Document Follow') + User = DocType('User') + return (frappe.qb.from_(DocumentFollow).join(User) + .on(DocumentFollow.user == User.name) + .where(User.document_follow_notify == 1) + .where(User.document_follow_frequency == frequency) + .select(DocumentFollow.user) + .groupby(DocumentFollow.user)).run(pluck="user") - for user in grouped_by_user: - user_frequency = frappe.db.get_value("User", user, "document_follow_frequency") - message = [] - valid_document_follows = [] - if user_frequency == frequency: - for d in grouped_by_user[user]: - content = get_message(d.ref_docname, d.ref_doctype, frequency, user) - if content: - message = message + content - valid_document_follows.append({ - "reference_docname": d.ref_docname, - "reference_doctype": d.ref_doctype, - "reference_url": get_url_to_form(d.ref_doctype, d.ref_docname) - }) +def get_message_for_user(frequency, user): + message = [] + latest_document_follows = get_document_followed_by_user(user) + valid_document_follows = [] - if message and frappe.db.get_value("User", user, "document_follow_notify", ignore=True): - send_email_alert(user, valid_document_follows, message) + for document_follow in latest_document_follows: + content = get_message(document_follow.ref_docname, document_follow.ref_doctype, frequency, user) + if content: + message = message + content + valid_document_follows.append({ + "reference_docname": document_follow.ref_docname, + "reference_doctype": document_follow.ref_doctype, + "reference_url": get_url_to_form(document_follow.ref_doctype, document_follow.ref_docname) + }) + return message, valid_document_follows +def get_document_followed_by_user(user): + DocumentFollow = DocType('Document Follow') + # at max 20 documents are sent for each user + return (frappe.qb.from_(DocumentFollow) + .where(DocumentFollow.user == user) + .select(DocumentFollow.ref_doctype, DocumentFollow.ref_docname) + .orderby(DocumentFollow.modified) + .limit(20)).run(as_dict=True) def get_version(doctype, doc_name, frequency, user): timeline = [] diff --git a/frappe/desk/form/utils.py b/frappe/desk/form/utils.py index 291767de10..f80b89d2b8 100644 --- a/frappe/desk/form/utils.py +++ b/frappe/desk/form/utils.py @@ -31,8 +31,8 @@ def add_comment(reference_doctype, reference_name, content, comment_email, comme reference_doc = frappe.get_doc(reference_doctype, reference_name) doc.content = extract_images_from_html(reference_doc, content, is_private=True) doc.insert(ignore_permissions=True) - - follow_document(doc.reference_doctype, doc.reference_name, frappe.session.user) + if frappe.get_cached_value("User", frappe.session.user, "follow_commented_documents"): + follow_document(doc.reference_doctype, doc.reference_name, frappe.session.user) return doc.as_dict() @frappe.whitelist() diff --git a/frappe/desk/like.py b/frappe/desk/like.py index 4480ed8a1e..5e5a789973 100644 --- a/frappe/desk/like.py +++ b/frappe/desk/like.py @@ -41,7 +41,8 @@ def _toggle_like(doctype, name, add, user=None): if user not in liked_by: liked_by.append(user) add_comment(doctype, name) - follow_document(doctype, name, user) + if frappe.get_cached_value("User", user, "follow_liked_documents"): + follow_document(doctype, name, user) else: if user in liked_by: liked_by.remove(user) diff --git a/frappe/email/doctype/document_follow/test_document_follow.py b/frappe/email/doctype/document_follow/test_document_follow.py index 050add65e9..0f6ff6c114 100644 --- a/frappe/email/doctype/document_follow/test_document_follow.py +++ b/frappe/email/doctype/document_follow/test_document_follow.py @@ -3,10 +3,18 @@ # License: MIT. See LICENSE import frappe import unittest +from dataclasses import dataclass import frappe.desk.form.document_follow as document_follow +from frappe.query_builder import DocType +from frappe.desk.form.utils import add_comment +from frappe.desk.form.document_follow import get_document_followed_by_user +from frappe.desk.like import toggle_like +from frappe.desk.form.assign_to import add +from frappe.share import add as share +from frappe.query_builder.functions import Cast_ class TestDocumentFollow(unittest.TestCase): - def test_document_follow(self): + def test_document_follow_version(self): user = get_user() event_doc = get_event() @@ -18,18 +26,173 @@ class TestDocumentFollow(unittest.TestCase): self.assertEqual(doc.user, user.name) document_follow.send_hourly_updates() + emails = get_emails(event_doc, '%This is a test description for sending mail%') + self.assertIsNotNone(emails) - email_queue_entry_name = frappe.get_all("Email Queue", limit=1)[0].name - email_queue_entry_doc = frappe.get_doc("Email Queue", email_queue_entry_name) - self.assertEqual((email_queue_entry_doc.recipients[0].recipient), user.name) + def test_document_follow_comment(self): + user = get_user() + event_doc = get_event() - self.assertIn(event_doc.doctype, email_queue_entry_doc.message) - self.assertIn(event_doc.name, email_queue_entry_doc.message) + add_comment(event_doc.doctype, event_doc.name, "This is a test comment", 'Administrator@example.com', 'Bosh') + + document_follow.unfollow_document("Event", event_doc.name, user.name) + doc = document_follow.follow_document("Event", event_doc.name, user.name) + self.assertEqual(doc.user, user.name) + + document_follow.send_hourly_updates() + emails = get_emails(event_doc, '%This is a test comment%') + self.assertIsNotNone(emails) + + + def test_follow_limit(self): + user = get_user() + for _ in range(25): + event_doc = get_event() + document_follow.unfollow_document("Event", event_doc.name, user.name) + doc = document_follow.follow_document("Event", event_doc.name, user.name) + self.assertEqual(doc.user, user.name) + self.assertEqual(len(get_document_followed_by_user(user.name)), 20) + + def test_follow_on_create(self): + user = get_user(DocumentFollowConditions(1)) + frappe.set_user(user.name) + event = get_event() + + event.description = "This is a test description for sending mail" + event.save(ignore_version=False) + + documents_followed = get_events_followed_by_user(event.name, user.name) + self.assertTrue(documents_followed) + + def test_do_not_follow_on_create(self): + user = get_user() + frappe.set_user(user.name) + + event = get_event() + + documents_followed = get_events_followed_by_user(event.name, user.name) + self.assertFalse(documents_followed) + + def test_do_not_follow_on_update(self): + user = get_user() + frappe.set_user(user.name) + event = get_event() + + event.description = "This is a test description for sending mail" + event.save(ignore_version=False) + + documents_followed = get_events_followed_by_user(event.name, user.name) + self.assertFalse(documents_followed) + + def test_follow_on_comment(self): + user = get_user(DocumentFollowConditions(0, 1)) + frappe.set_user(user.name) + event = get_event() + + add_comment(event.doctype, event.name, "This is a test comment", 'Administrator@example.com', 'Bosh') + + documents_followed = get_events_followed_by_user(event.name, user.name) + self.assertTrue(documents_followed) + + def test_do_not_follow_on_comment(self): + user = get_user() + frappe.set_user(user.name) + event = get_event() + + add_comment(event.doctype, event.name, "This is a test comment", 'Administrator@example.com', 'Bosh') + + documents_followed = get_events_followed_by_user(event.name, user.name) + self.assertFalse(documents_followed) + + def test_follow_on_like(self): + user = get_user(DocumentFollowConditions(0, 0, 1)) + frappe.set_user(user.name) + event = get_event() + + toggle_like(event.doctype, event.name, add="Yes") + + documents_followed = get_events_followed_by_user(event.name, user.name) + self.assertTrue(documents_followed) + + def test_do_not_follow_on_like(self): + user = get_user() + frappe.set_user(user.name) + event = get_event() + + toggle_like(event.doctype, event.name) + + documents_followed = get_events_followed_by_user(event.name, user.name) + self.assertFalse(documents_followed) + + def test_follow_on_assign(self): + user = get_user(DocumentFollowConditions(0, 0, 0, 1)) + event = get_event() + + add({ + 'assign_to': [user.name], + 'doctype': event.doctype, + 'name': event.name + }) + + documents_followed = get_events_followed_by_user(event.name, user.name) + self.assertTrue(documents_followed) + + def test_do_not_follow_on_assign(self): + user = get_user() + frappe.set_user(user.name) + event = get_event() + + add({ + 'assign_to': [user.name], + 'doctype': event.doctype, + 'name': event.name + }) + + documents_followed = get_events_followed_by_user(event.name, user.name) + self.assertFalse(documents_followed) + + def test_follow_on_share(self): + user = get_user(DocumentFollowConditions(0, 0, 0, 0, 1)) + event = get_event() + + share( + user= user.name, + doctype= event.doctype, + name= event.name + ) + + documents_followed = get_events_followed_by_user(event.name, user.name) + self.assertTrue(documents_followed) + + def test_do_not_follow_on_share(self): + user = get_user() + event = get_event() + + share( + user = user.name, + doctype = event.doctype, + name = event.name + ) + + documents_followed = get_events_followed_by_user(event.name, user.name) + self.assertFalse(documents_followed) def tearDown(self): frappe.db.rollback() + frappe.db.delete('Email Queue') + frappe.db.delete('Email Queue Recipient') + frappe.db.delete('Document Follow') + frappe.db.delete('Event') + +def get_events_followed_by_user(event_name, user_name): + DocumentFollow = DocType('Document Follow') + return (frappe.qb.from_(DocumentFollow) + .where(DocumentFollow.ref_doctype == 'Event') + .where(DocumentFollow.ref_docname == event_name) + .where(DocumentFollow.user == user_name) + .select(DocumentFollow.name)).run() def get_event(): doc = frappe.get_doc({ @@ -42,16 +205,40 @@ def get_event(): doc.insert() return doc -def get_user(): +def get_user(document_follow=None): + frappe.set_user("Administrator") if frappe.db.exists('User', 'test@docsub.com'): - doc = frappe.get_doc('User', 'test@docsub.com') - else: - doc = frappe.new_doc("User") - doc.email = "test@docsub.com" - doc.first_name = "Test" - doc.last_name = "User" - doc.send_welcome_email = 0 - doc.document_follow_notify = 1 - doc.document_follow_frequency = "Hourly" - doc.insert() - return doc \ No newline at end of file + doc = frappe.delete_doc('User', 'test@docsub.com') + doc = frappe.new_doc("User") + doc.email = "test@docsub.com" + doc.first_name = "Test" + doc.last_name = "User" + doc.send_welcome_email = 0 + doc.document_follow_notify = 1 + doc.document_follow_frequency = "Hourly" + doc.__dict__.update(document_follow.__dict__ if document_follow else {}) + doc.insert() + doc.add_roles('System Manager') + return doc + +def get_emails(event_doc, search_string): + EmailQueue = DocType('Email Queue') + EmailQueueRecipient = DocType('Email Queue Recipient') + + return (frappe.qb.from_(EmailQueue) + .join(EmailQueueRecipient) + .on(EmailQueueRecipient.parent == Cast_(EmailQueue.name, "varchar")) + .where(EmailQueueRecipient.recipient == 'test@docsub.com',) + .where(EmailQueue.message.like(f'%{event_doc.doctype}%')) + .where(EmailQueue.message.like(f'%{event_doc.name}%')) + .where(EmailQueue.message.like(search_string)) + .select(EmailQueue.message) + .limit(1)).run() + +@dataclass +class DocumentFollowConditions: + follow_created_documents: int = 0 + follow_commented_documents: int = 0 + follow_liked_documents: int = 0 + follow_assigned_documents: int = 0 + follow_shared_documents: int = 0 diff --git a/frappe/model/document.py b/frappe/model/document.py index 3c38ff3442..3848fa8029 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -276,7 +276,8 @@ class Document(BaseDocument): delattr(self, "__unsaved") if not (frappe.flags.in_migrate or frappe.local.flags.in_install or frappe.flags.in_setup_wizard): - follow_document(self.doctype, self.name, frappe.session.user) + if frappe.get_cached_value("User", frappe.session.user, "follow_created_documents"): + follow_document(self.doctype, self.name, frappe.session.user) return self def save(self, *args, **kwargs): @@ -1125,7 +1126,8 @@ class Document(BaseDocument): version.insert(ignore_permissions=True) if not frappe.flags.in_migrate: # follow since you made a change? - follow_document(self.doctype, self.name, frappe.session.user) + if frappe.get_cached_value("User", frappe.session.user, "follow_created_documents"): + follow_document(self.doctype, self.name, frappe.session.user) @staticmethod def hook(f): diff --git a/frappe/query_builder/functions.py b/frappe/query_builder/functions.py index 9d12358f0d..cf01e885ba 100644 --- a/frappe/query_builder/functions.py +++ b/frappe/query_builder/functions.py @@ -44,6 +44,28 @@ CombineDatetime = ImportMapper( ) +class Cast_(Function): + def __init__(self, value, as_type, alias=None): + if db_type_is.MARIADB and ( + (hasattr(as_type, "get_sql") and as_type.get_sql() == "varchar") or str(as_type).lower() == "varchar" + ): + # mimics varchar cast in mariadb + # as mariadb doesn't have varchar data cast + # https://mariadb.com/kb/en/cast/#description + + # ref: https://stackoverflow.com/a/32542095 + super().__init__("CONCAT", value, "", alias=alias) + else: + # from source: https://pypika.readthedocs.io/en/latest/_modules/pypika/functions.html#Cast + super().__init__("CAST", value, alias=alias) + self.as_type = as_type + + def get_special_params_sql(self, **kwargs): + if self.name.lower() == "cast": + type_sql = self.as_type.get_sql() if hasattr(self.as_type, "get_sql") else str(self.as_type).upper() + return "AS {type}".format(type=type_sql) + + def _aggregate(function, dt, fieldname, filters, **kwargs): return ( Query() diff --git a/frappe/share.py b/frappe/share.py index 9b33198c9b..917181e563 100644 --- a/frappe/share.py +++ b/frappe/share.py @@ -44,7 +44,8 @@ def add(doctype, name, user=None, read=1, write=0, submit=0, share=0, everyone=0 doc.save(ignore_permissions=True) notify_assignment(user, doctype, name, everyone, notify=notify) - follow_document(doctype, name, user) + if frappe.get_cached_value("User", user, "follow_shared_documents"): + follow_document(doctype, name, user) return doc diff --git a/frappe/tests/test_global_search.py b/frappe/tests/test_global_search.py index 9a86baa4e5..f3c0f90d71 100644 --- a/frappe/tests/test_global_search.py +++ b/frappe/tests/test_global_search.py @@ -83,7 +83,7 @@ class TestGlobalSearch(unittest.TestCase): def test_delete_doc(self): self.insert_test_events() - + global_search.sync_global_search() event_name = frappe.get_all('Event')[0].name event = frappe.get_doc('Event', event_name) test_subject = event.subject