From 057139ea2eb7333575b5bea7a1c8e8977780f757 Mon Sep 17 00:00:00 2001 From: David Arnold Date: Thu, 14 Nov 2024 00:12:14 +0100 Subject: [PATCH] chore(communication): cleanup unused code (#28199) * chore: feedback doctype no longer exists missed from https://github.com/frappe/frappe/pull/17479 * chore: remove unused communication type This was removed and migrated already in v12: ``` frappe/patches/v12_0/setup_comments_from_communications.py: frappe.db.delete("Communication", {"communication_type": "Comment"}) ``` ... comming from https://github.com/frappe/frappe/commit/41d90fa6d1fda781f6c96493faa616a63c5e0eb3 which effectively reverted https://github.com/frappe/frappe/commit/465318878e15b96f4dbeb02c410db19d15fee817 --- .../doctype/communication/communication.json | 46 ++-------- .../doctype/communication/communication.py | 56 ++----------- frappe/core/doctype/user/user.py | 11 --- .../dashboard_chart/test_dashboard_chart.py | 83 ++++++++++++------- frappe/desk/form/load.py | 8 +- .../email_account/test_email_account.py | 27 ++++-- .../doctype/notification/test_notification.py | 3 +- .../doctype/notification/test_records.toml | 9 +- frappe/tests/test_query_builder.py | 39 +++++---- 9 files changed, 124 insertions(+), 158 deletions(-) diff --git a/frappe/core/doctype/communication/communication.json b/frappe/core/doctype/communication/communication.json index c8d58db89f..bc3496b4a9 100644 --- a/frappe/core/doctype/communication/communication.json +++ b/frappe/core/doctype/communication/communication.json @@ -24,7 +24,6 @@ "status_section", "text_content", "communication_type", - "comment_type", "column_break_5", "status", "sent_or_received", @@ -55,10 +54,7 @@ "uid", "imap_folder", "email_status", - "has_attachment", - "feedback_section", - "rating", - "feedback_request" + "has_attachment" ], "fields": [ { @@ -75,9 +71,10 @@ "label": "To and CC" }, { - "depends_on": "eval:doc.communication_type===\"Communication\"", "fieldname": "communication_medium", "fieldtype": "Select", + "in_list_view": 1, + "in_standard_filter": 1, "label": "Type", "options": "\nEmail\nChat\nPhone\nSMS\nEvent\nMeeting\nVisit\nOther" }, @@ -154,20 +151,12 @@ "default": "Communication", "fieldname": "communication_type", "fieldtype": "Select", - "label": "Communication Type", - "options": "Communication\nComment\nChat\nNotification\nFeedback\nAutomated Message", - "read_only": 1, - "reqd": 1 - }, - { - "fieldname": "comment_type", - "fieldtype": "Select", - "hidden": 1, "in_list_view": 1, "in_standard_filter": 1, - "label": "Comment Type", - "options": "\nComment\nLike\nInfo\nLabel\nWorkflow\nCreated\nSubmitted\nCancelled\nUpdated\nDeleted\nAssigned\nAssignment Completed\nAttachment\nAttachment Removed\nShared\nUnshared\nRelinked", - "read_only": 1 + "label": "Communication Type", + "options": "Communication\nAutomated Message", + "read_only": 1, + "reqd": 1 }, { "fieldname": "column_break_5", @@ -346,25 +335,6 @@ "hidden": 1, "label": "Has Attachment" }, - { - "collapsible": 1, - "depends_on": "eval: doc.rating > 0", - "fieldname": "feedback_section", - "fieldtype": "Section Break", - "label": "Feedback" - }, - { - "fieldname": "rating", - "fieldtype": "Int", - "label": "Rating", - "read_only": 1 - }, - { - "fieldname": "feedback_request", - "fieldtype": "Data", - "label": "Feedback Request", - "read_only": 1 - }, { "fieldname": "email_template", "fieldtype": "Link", @@ -466,4 +436,4 @@ "title_field": "subject", "track_changes": 1, "track_seen": 1 -} \ No newline at end of file +} diff --git a/frappe/core/doctype/communication/communication.py b/frappe/core/doctype/communication/communication.py index de629d04ca..ffd3cfe797 100644 --- a/frappe/core/doctype/communication/communication.py +++ b/frappe/core/doctype/communication/communication.py @@ -44,33 +44,11 @@ class Communication(Document, CommunicationEmailMixin): _user_tags: DF.Data | None bcc: DF.Code | None cc: DF.Code | None - comment_type: DF.Literal[ - "", - "Comment", - "Like", - "Info", - "Label", - "Workflow", - "Created", - "Submitted", - "Cancelled", - "Updated", - "Deleted", - "Assigned", - "Assignment Completed", - "Attachment", - "Attachment Removed", - "Shared", - "Unshared", - "Relinked", - ] communication_date: DF.Datetime | None communication_medium: DF.Literal[ "", "Email", "Chat", "Phone", "SMS", "Event", "Meeting", "Visit", "Other" ] - communication_type: DF.Literal[ - "Communication", "Comment", "Chat", "Notification", "Feedback", "Automated Message" - ] + communication_type: DF.Literal["Communication", "Automated Message"] content: DF.TextEditor | None delivery_status: DF.Literal[ "", @@ -92,13 +70,11 @@ class Communication(Document, CommunicationEmailMixin): email_account: DF.Link | None email_status: DF.Literal["Open", "Spam", "Trash"] email_template: DF.Link | None - feedback_request: DF.Data | None has_attachment: DF.Check imap_folder: DF.Data | None in_reply_to: DF.Link | None message_id: DF.SmallText | None phone_no: DF.Data | None - rating: DF.Int read_by_recipient: DF.Check read_by_recipient_on: DF.Datetime | None read_receipt: DF.Check @@ -218,19 +194,7 @@ class Communication(Document, CommunicationEmailMixin): if self.reference_doctype == "Communication" and self.sent_or_received == "Sent": frappe.db.set_value("Communication", self.reference_name, "status", "Replied") - if self.communication_type == "Communication": - self.notify_change("add") - - elif self.communication_type in ("Chat", "Notification"): - if self.reference_name == frappe.session.user: - message = self.as_dict() - message["broadcast"] = True - frappe.publish_realtime("new_message", message, after_commit=True) - else: - # reference_name contains the user who is addressed in the messages' page comment - frappe.publish_realtime( - "new_message", self.as_dict(), user=self.reference_name, after_commit=True - ) + self.notify_change("add") def set_signature_in_email_content(self): """Set sender's User.email_signature or default outgoing's EmailAccount.signature to the email""" @@ -284,13 +248,10 @@ class Communication(Document, CommunicationEmailMixin): if (method := getattr(parent, "on_communication_update", None)) and callable(method): parent.on_communication_update(self) return - - if self.comment_type != "Updated": - update_parent_document_on_communication(self) + update_parent_document_on_communication(self) def on_trash(self): - if self.communication_type == "Communication": - self.notify_change("delete") + self.notify_change("delete") @property def sender_mailid(self): @@ -342,10 +303,8 @@ class Communication(Document, CommunicationEmailMixin): def set_status(self): if self.reference_doctype and self.reference_name: self.status = "Linked" - elif self.communication_type == "Communication": - self.status = "Open" else: - self.status = "Closed" + self.status = "Open" if self.send_after and self.is_new(): self.delivery_status = "Scheduled" @@ -658,11 +617,6 @@ def update_parent_document_on_communication(doc): if not parent: return - # update parent mins_to_first_communication only if we create the Email communication - # ignore in case of only Comment is added - if doc.communication_type == "Comment": - return - status_field = parent.meta.get_field("status") if status_field: options = (status_field.options or "").splitlines() diff --git a/frappe/core/doctype/user/user.py b/frappe/core/doctype/user/user.py index 81b2bec376..395af1a1a0 100644 --- a/frappe/core/doctype/user/user.py +++ b/frappe/core/doctype/user/user.py @@ -547,17 +547,6 @@ class User(Document): # delete shares frappe.db.delete("DocShare", {"user": self.name}) - # delete messages - table = DocType("Communication") - frappe.db.delete( - table, - filters=( - (table.communication_type.isin(["Chat", "Notification"])) - & (table.reference_doctype == "User") - & ((table.reference_name == self.name) | table.owner == self.name) - ), - run=False, - ) # unlink contact table = DocType("Contact") frappe.qb.update(table).where(table.user == self.name).set(table.user, None).run() diff --git a/frappe/desk/doctype/dashboard_chart/test_dashboard_chart.py b/frappe/desk/doctype/dashboard_chart/test_dashboard_chart.py index 7d877a5280..5fda816a99 100644 --- a/frappe/desk/doctype/dashboard_chart/test_dashboard_chart.py +++ b/frappe/desk/doctype/dashboard_chart/test_dashboard_chart.py @@ -7,6 +7,7 @@ from unittest.mock import patch from dateutil.relativedelta import relativedelta import frappe +from frappe.core.doctype.doctype.test_doctype import new_doctype from frappe.desk.doctype.dashboard_chart.dashboard_chart import get from frappe.tests import IntegrationTestCase, UnitTestCase from frappe.utils import formatdate, get_last_day, getdate @@ -23,6 +24,32 @@ class UnitTestDashboardChart(UnitTestCase): class TestDashboardChart(IntegrationTestCase): + def setUp(self): + doc = new_doctype( + fields=[ + { + "fieldname": "title", + "fieldtype": "Text", + "label": "Title", + "reqd": 1, # mandatory + }, + { + "fieldname": "number", + "fieldtype": "Int", + "label": "Number", + "reqd": 1, # mandatory + }, + { + "fieldname": "date", + "fieldtype": "Date", + "label": "Date", + "reqd": 1, # mandatory + }, + ], + ) + doc.insert() + self.doctype_name = doc.name + def test_period_ending(self): self.assertEqual(get_period_ending("2019-04-10", "Daily"), getdate("2019-04-10")) @@ -147,7 +174,7 @@ class TestDashboardChart(IntegrationTestCase): self.assertEqual(result.get("datasets")[0].get("values")[0], todo_status_count) def test_daily_dashboard_chart(self): - insert_test_records() + insert_test_records(self.doctype_name) if frappe.db.exists("Dashboard Chart", "Test Daily Dashboard Chart"): frappe.delete_doc("Dashboard Chart", "Test Daily Dashboard Chart") @@ -156,9 +183,9 @@ class TestDashboardChart(IntegrationTestCase): doctype="Dashboard Chart", chart_name="Test Daily Dashboard Chart", chart_type="Sum", - document_type="Communication", - based_on="communication_date", - value_based_on="rating", + document_type=self.doctype_name, + based_on="date", + value_based_on="number", timespan="Select Date Range", time_interval="Daily", from_date=datetime(2019, 1, 6), @@ -176,7 +203,7 @@ class TestDashboardChart(IntegrationTestCase): ) def test_weekly_dashboard_chart(self): - insert_test_records() + insert_test_records(self.doctype_name) if frappe.db.exists("Dashboard Chart", "Test Weekly Dashboard Chart"): frappe.delete_doc("Dashboard Chart", "Test Weekly Dashboard Chart") @@ -185,9 +212,9 @@ class TestDashboardChart(IntegrationTestCase): doctype="Dashboard Chart", chart_name="Test Weekly Dashboard Chart", chart_type="Sum", - document_type="Communication", - based_on="communication_date", - value_based_on="rating", + document_type=self.doctype_name, + based_on="date", + value_based_on="number", timespan="Select Date Range", time_interval="Weekly", from_date=datetime(2018, 12, 30), @@ -203,7 +230,7 @@ class TestDashboardChart(IntegrationTestCase): self.assertEqual(result.get("labels"), ["12-30-2018", "01-06-2019", "01-13-2019", "01-20-2019"]) def test_avg_dashboard_chart(self): - insert_test_records() + insert_test_records(self.doctype_name) if frappe.db.exists("Dashboard Chart", "Test Average Dashboard Chart"): frappe.delete_doc("Dashboard Chart", "Test Average Dashboard Chart") @@ -212,9 +239,9 @@ class TestDashboardChart(IntegrationTestCase): doctype="Dashboard Chart", chart_name="Test Average Dashboard Chart", chart_type="Average", - document_type="Communication", - based_on="communication_date", - value_based_on="rating", + document_type=self.doctype_name, + based_on="date", + value_based_on="number", timespan="Select Date Range", time_interval="Weekly", from_date=datetime(2018, 12, 30), @@ -254,22 +281,22 @@ class TestDashboardChart(IntegrationTestCase): self.assertEqual(sorted(result.get("labels")), sorted(["01-19-2019", "01-05-2019", "01-12-2019"])) -def insert_test_records(): - create_new_communication("Communication 1", datetime(2018, 12, 30), 50) - create_new_communication("Communication 2", datetime(2019, 1, 4), 100) - create_new_communication("Communication 3", datetime(2019, 1, 6), 200) - create_new_communication("Communication 4", datetime(2019, 1, 7), 400) - create_new_communication("Communication 5", datetime(2019, 1, 8), 300) - create_new_communication("Communication 6", datetime(2019, 1, 10), 100) +def insert_test_records(doctype_name): + create_new_record(doctype_name, "Title 1", datetime(2018, 12, 30), 50) + create_new_record(doctype_name, "Title 2", datetime(2019, 1, 4), 100) + create_new_record(doctype_name, "Title 3", datetime(2019, 1, 6), 200) + create_new_record(doctype_name, "Title 4", datetime(2019, 1, 7), 400) + create_new_record(doctype_name, "Title 5", datetime(2019, 1, 8), 300) + create_new_record(doctype_name, "Title 6", datetime(2019, 1, 10), 100) -def create_new_communication(subject, date, rating): - communication = { - "doctype": "Communication", - "subject": subject, - "rating": rating, - "communication_date": date, +def create_new_record(doctype_name, title, date, number): + doc = { + "doctype": doctype_name, + "title": title, + "date": date, + "number": number, } - comm = frappe.get_doc(communication) - if not frappe.db.exists("Communication", {"subject": comm.subject}): - comm.insert() + doc = frappe.get_doc(doc) + if not frappe.db.exists(doctype_name, {"title": doc.title}): + doc.insert(ignore_mandatory=True) diff --git a/frappe/desk/form/load.py b/frappe/desk/form/load.py index 0a95293780..6f903980d5 100644 --- a/frappe/desk/form/load.py +++ b/frappe/desk/form/load.py @@ -292,11 +292,11 @@ def get_communication_data( if not fields: fields = """ C.name, C.communication_type, C.communication_medium, - C.comment_type, C.communication_date, C.content, + C.communication_date, C.content, C.sender, C.sender_full_name, C.cc, C.bcc, C.creation AS creation, C.subject, C.delivery_status, C._liked_by, C.reference_doctype, C.reference_name, - C.read_by_recipient, C.rating, C.recipients + C.read_by_recipient, C.recipients """ conditions = "" @@ -315,7 +315,7 @@ def get_communication_data( part1 = f""" SELECT {fields} FROM `tabCommunication` as C - WHERE C.communication_type IN ('Communication', 'Feedback', 'Automated Message') + WHERE C.communication_type IN ('Communication', 'Automated Message') AND (C.reference_doctype = %(doctype)s AND C.reference_name = %(name)s) {conditions} """ @@ -325,7 +325,7 @@ def get_communication_data( SELECT {fields} FROM `tabCommunication` as C INNER JOIN `tabCommunication Link` ON C.name=`tabCommunication Link`.parent - WHERE C.communication_type IN ('Communication', 'Feedback', 'Automated Message') + WHERE C.communication_type IN ('Communication', 'Automated Message') AND `tabCommunication Link`.link_doctype = %(doctype)s AND `tabCommunication Link`.link_name = %(name)s {conditions} """ diff --git a/frappe/email/doctype/email_account/test_email_account.py b/frappe/email/doctype/email_account/test_email_account.py index dbac77d408..18b1637845 100644 --- a/frappe/email/doctype/email_account/test_email_account.py +++ b/frappe/email/doctype/email_account/test_email_account.py @@ -168,16 +168,23 @@ class TestEmailAccount(IntegrationTestCase): ) def test_outgoing(self): - make( + comm_name = make( subject="test-mail-000", content="test mail 000", recipients="test_receiver@example.com", send_email=True, sender="test_sender@example.com", - ) + )["name"] - mail = email.message_from_string(frappe.get_last_doc("Email Queue").message) - self.assertTrue("test-mail-000" in mail.get("Subject")) + sent_mail = email.message_from_string( + frappe.get_doc( + "Email Queue", + { + "communication": comm_name, + }, + ).message + ) + self.assertTrue("test-mail-000" in sent_mail.get("Subject")) def test_sendmail(self): frappe.sendmail( @@ -192,7 +199,7 @@ class TestEmailAccount(IntegrationTestCase): self.assertTrue("test-mail-001" in sent_mail.get("Subject")) def test_print_format(self): - make( + comm_name = make( sender="test_sender@example.com", recipients="test_recipient@example.com", content="test mail 001", @@ -201,9 +208,15 @@ class TestEmailAccount(IntegrationTestCase): name="_Test Email Account 1", print_format="Standard", send_email=True, + )["name"] + sent_mail = email.message_from_string( + frappe.get_doc( + "Email Queue", + { + "communication": comm_name, + }, + ).message ) - - sent_mail = email.message_from_string(frappe.get_last_doc("Email Queue").message) self.assertTrue("test-mail-002" in sent_mail.get("Subject")) def test_threading(self): diff --git a/frappe/email/doctype/notification/test_notification.py b/frappe/email/doctype/notification/test_notification.py index 9c6d01591d..45a6a67ed3 100644 --- a/frappe/email/doctype/notification/test_notification.py +++ b/frappe/email/doctype/notification/test_notification.py @@ -68,7 +68,8 @@ class TestNotification(IntegrationTestCase): def test_new_and_save(self): """Check creating a new communication triggers a notification.""" communication = frappe.new_doc("Communication") - communication.communication_type = "Comment" + communication.communication_type = "Communication" + communication.sender_full_name = "__test_notification_sender__" communication.subject = "test" communication.content = "test" communication.insert(ignore_permissions=True) diff --git a/frappe/email/doctype/notification/test_records.toml b/frappe/email/doctype/notification/test_records.toml index 49e6aeaeef..f32b515703 100644 --- a/frappe/email/doctype/notification/test_records.toml +++ b/frappe/email/doctype/notification/test_records.toml @@ -3,8 +3,8 @@ subject = "_Test Notification 1" document_type = "Communication" event = "New" attach_print = 0 -message = "New comment {{ doc.content }} created" -condition = "doc.communication_type=='Comment'" +message = "New communication {{ doc.content }} created" +condition = "doc.communication_type=='Communication' and doc.sender_full_name=='__test_notification_sender__'" [[Notification.recipients]] receiver_by_document_field = "owner" @@ -14,8 +14,9 @@ subject = "_Test Notification 2" document_type = "Communication" event = "Save" attach_print = 0 -message = "New comment {{ doc.content }} saved" -condition = "doc.communication_type=='Comment'" +sender_full_name = "__testsender__" +message = "New communication {{ doc.content }} saved" +condition = "doc.communication_type=='Communication' and doc.sender_full_name=='__test_notification_sender__'" set_property_after_alert = "subject" property_value = "__testing__" [[Notification.recipients]] diff --git a/frappe/tests/test_query_builder.py b/frappe/tests/test_query_builder.py index 1be10d60dc..76b7add5aa 100644 --- a/frappe/tests/test_query_builder.py +++ b/frappe/tests/test_query_builder.py @@ -3,6 +3,7 @@ from collections.abc import Callable from datetime import time import frappe +from frappe.core.doctype.doctype.test_doctype import new_doctype from frappe.query_builder import Case from frappe.query_builder.builder import Function from frappe.query_builder.custom import ConstantColumn @@ -316,22 +317,32 @@ class TestBuilderBase: self.assertIsInstance(data, list) def test_agg_funcs(self): - frappe.db.truncate("Communication") + doc = new_doctype( + fields=[ + { + "fieldname": "number", + "fieldtype": "Int", + "label": "Number", + "reqd": 1, # mandatory + }, + ], + ) + doc.insert() + self.doctype_name = doc.name + frappe.db.truncate(self.doctype_name) sample_data = { - "doctype": "Communication", - "communication_type": "Communication", - "content": "testing", - "rating": 1, + "doctype": self.doctype_name, + "number": 1, } - frappe.get_doc(sample_data).insert() - sample_data["rating"] = 3 - frappe.get_doc(sample_data).insert() - sample_data["rating"] = 4 - frappe.get_doc(sample_data).insert() - self.assertEqual(frappe.qb.max("Communication", "rating"), 4) - self.assertEqual(frappe.qb.min("Communication", "rating"), 1) - self.assertAlmostEqual(frappe.qb.avg("Communication", "rating"), 2.666, places=2) - self.assertEqual(frappe.qb.sum("Communication", "rating"), 8.0) + frappe.get_doc(sample_data).insert(ignore_mandatory=True) + sample_data["number"] = 3 + frappe.get_doc(sample_data).insert(ignore_mandatory=True) + sample_data["number"] = 4 + frappe.get_doc(sample_data).insert(ignore_mandatory=True) + self.assertEqual(frappe.qb.max(self.doctype_name, "number"), 4) + self.assertEqual(frappe.qb.min(self.doctype_name, "number"), 1) + self.assertAlmostEqual(frappe.qb.avg(self.doctype_name, "number"), 2.666, places=2) + self.assertEqual(frappe.qb.sum(self.doctype_name, "number"), 8.0) frappe.db.rollback()