diff --git a/erpnext/support/doctype/issue/issue.py b/erpnext/support/doctype/issue/issue.py index bb2c86c9fcf..77a84f2901a 100644 --- a/erpnext/support/doctype/issue/issue.py +++ b/erpnext/support/doctype/issue/issue.py @@ -117,12 +117,55 @@ class Issue(Document): communication.flags.ignore_mandatory = True communication.save() + def get_timeline_communications(self, after=None) -> tuple[set[str], set[str]]: + """Return the Communications on this Issue's timeline, split by how they are attached. + + Mirrors the two sources `frappe.desk.form.load.get_communication_data` reads, since + Split is offered on every timeline item. `after` matches `communication_date`, what + the timeline is ordered by, not `creation`: a pulled email is created when fetched. + """ + date_filter = {"communication_date": (">=", after)} if after else {} + + referenced = frappe.get_all( + "Communication", + filters={"reference_doctype": "Issue", "reference_name": self.name, **date_filter}, + pluck="name", + ) + + link_parents = frappe.get_all( + "Communication Link", + filters={"link_doctype": "Issue", "link_name": self.name}, + pluck="parent", + ) + linked = ( + frappe.get_all( + "Communication", + filters={"name": ("in", link_parents), **date_filter}, + pluck="name", + ) + if link_parents + else [] + ) + + return set(referenced), set(linked) + @frappe.whitelist(methods=["POST"]) def split_issue(self, subject: str, communication_id: str): from copy import deepcopy self.check_permission("write") + referenced, linked = self.get_timeline_communications() + if communication_id not in referenced | linked: + frappe.throw( + _("Communication {0} is not on the timeline of Issue {1}").format( + communication_id, self.name + ), + frappe.PermissionError, + ) + + comm_to_split_from = frappe.get_doc("Communication", communication_id) + replicated_issue = deepcopy(self) replicated_issue.subject = subject replicated_issue.issue_split_from = self.name @@ -141,21 +184,22 @@ class Issue(Document): frappe.get_doc(replicated_issue).insert() - # Replicate linked Communications - # TODO: get all communications in timeline before this, and modify them to append them to new doc - comm_to_split_from = frappe.get_doc("Communication", communication_id) - communications = frappe.get_all( - "Communication", - filters={ - "reference_doctype": "Issue", - "reference_name": comm_to_split_from.reference_name, - "creation": (">=", comm_to_split_from.creation), - }, - ) + # Move the whole timeline from the split point onwards, both the Communications that + # reference this Issue and the ones only joined to it through a Timeline Link. + referenced, linked = self.get_timeline_communications(after=comm_to_split_from.communication_date) + + for name in sorted(referenced | linked): + doc = frappe.get_doc("Communication", name) + + if name in referenced: + doc.reference_name = replicated_issue.name + + # A Timeline Link is this Issue's own handle on the Communication, so it moves with + # the split. Its reference belongs to some other document and is left alone. + for link in doc.timeline_links: + if link.link_doctype == "Issue" and link.link_name == self.name: + link.link_name = replicated_issue.name - for communication in communications: - doc = frappe.get_doc("Communication", communication.name) - doc.reference_name = replicated_issue.name doc.save(ignore_permissions=True) frappe.get_doc( diff --git a/erpnext/support/doctype/issue/test_issue.py b/erpnext/support/doctype/issue/test_issue.py index a8f7d402d64..5a2ac411b58 100644 --- a/erpnext/support/doctype/issue/test_issue.py +++ b/erpnext/support/doctype/issue/test_issue.py @@ -641,3 +641,201 @@ def create_communication(reference_name, sender, sent_or_received, creation): } ) communication.save() + + +class TestSplitIssue(ERPNextTestSuite): + def setUp(self): + frappe.set_user("Administrator") + frappe.db.set_single_value("Support Settings", "track_service_level_agreement", 0) + + def tearDown(self): + frappe.set_user("Administrator") + super().tearDown() + + def make_issue(self, subject): + issue = frappe.get_doc( + { + "doctype": "Issue", + "subject": subject, + "raised_by": "split-test@example.com", + "status": "Open", + } + ).insert(ignore_permissions=True) + + # split_issue() deepcopies self, so it has to start from a document loaded off the + # database the way the desk caller hands it one, not from the freshly inserted object + return frappe.get_doc("Issue", issue.name) + + def make_communication(self, subject, communication_date, reference=None, link_to=None, creation=None): + """A Communication attached to an Issue by reference, by Timeline Link, or by both.""" + communication = frappe.get_doc( + { + "doctype": "Communication", + "communication_type": "Communication", + "communication_medium": "Email", + "sent_or_received": "Received", + "subject": subject, + "content": subject, + "sender": "split-test@example.com", + "status": "Linked", + "communication_date": get_datetime(communication_date) if communication_date else None, + "reference_doctype": "Issue" if reference else None, + "reference_name": reference, + } + ).insert(ignore_permissions=True) + + if link_to: + communication.add_link("Issue", link_to, autosave=True) + + if not communication_date: + # the field defaults to Now whenever it is unset, so leaving it genuinely empty -- + # as an import or an API caller can -- means blanking it after the insert + frappe.db.set_value( + "Communication", communication.name, "communication_date", None, update_modified=False + ) + communication.reload() + + if creation: + # insert() always stamps creation with the current time, so a test that needs it to + # disagree with communication_date has to write it afterwards + frappe.db.set_value( + "Communication", + communication.name, + "creation", + get_datetime(creation), + update_modified=False, + ) + communication.reload() + + return communication + + def linked_issues(self, communication): + return [ + link.link_name + for link in frappe.get_doc("Communication", communication).timeline_links + if link.link_doctype == "Issue" + ] + + def test_split_moves_referenced_communications_from_the_split_point(self): + issue = self.make_issue("Split source") + first = self.make_communication("First", "2024-01-01 10:00:00", reference=issue.name) + second = self.make_communication("Second", "2024-01-01 11:00:00", reference=issue.name) + third = self.make_communication("Third", "2024-01-01 12:00:00", reference=issue.name) + + split = issue.split_issue(subject="Split target", communication_id=second.name) + + self.assertEqual(frappe.db.get_value("Communication", first.name, "reference_name"), issue.name) + self.assertEqual(frappe.db.get_value("Communication", second.name, "reference_name"), split) + self.assertEqual(frappe.db.get_value("Communication", third.name, "reference_name"), split) + self.assertEqual(frappe.db.get_value("Issue", split, "issue_split_from"), issue.name) + + def test_split_follows_the_timeline_order_not_the_insertion_order(self): + """The split point is read off the timeline, which is ordered by communication_date. + + A pulled email is created when it is fetched, so creation can run the other way. + """ + issue = self.make_issue("Split source") + first = self.make_communication( + "Sent first, fetched last", "2024-01-01 10:00:00", reference=issue.name, creation="2024-06-03" + ) + second = self.make_communication( + "Sent second", "2024-01-01 11:00:00", reference=issue.name, creation="2024-06-02" + ) + third = self.make_communication( + "Sent last, fetched first", "2024-01-01 12:00:00", reference=issue.name, creation="2024-06-01" + ) + + split = issue.split_issue(subject="Split target", communication_id=second.name) + + self.assertEqual(frappe.db.get_value("Communication", first.name, "reference_name"), issue.name) + self.assertEqual(frappe.db.get_value("Communication", second.name, "reference_name"), split) + self.assertEqual(frappe.db.get_value("Communication", third.name, "reference_name"), split) + + def test_split_treats_an_undated_communication_as_the_bottom_of_the_timeline(self): + """communication_date is not mandatory, and an undated item sits below every split point.""" + # Split from a dated item: the undated one is below the split point, so it stays behind. + issue = self.make_issue("Split source") + undated = self.make_communication("Undated", None, reference=issue.name) + first = self.make_communication("First", "2024-01-01 10:00:00", reference=issue.name) + second = self.make_communication("Second", "2024-01-01 11:00:00", reference=issue.name) + + split = issue.split_issue(subject="Split target", communication_id=first.name) + + self.assertEqual(frappe.db.get_value("Communication", undated.name, "reference_name"), issue.name) + self.assertEqual(frappe.db.get_value("Communication", first.name, "reference_name"), split) + self.assertEqual(frappe.db.get_value("Communication", second.name, "reference_name"), split) + + # Split from the undated item itself: a null split point drops the date filter, so the + # whole timeline above it -- which is everything -- moves. + source = self.make_issue("Undated split source") + from_undated = self.make_communication("Undated", None, reference=source.name) + dated = self.make_communication("Dated", "2024-01-01 10:00:00", reference=source.name) + + whole = source.split_issue(subject="Split target", communication_id=from_undated.name) + + self.assertEqual(frappe.db.get_value("Communication", from_undated.name, "reference_name"), whole) + self.assertEqual(frappe.db.get_value("Communication", dated.name, "reference_name"), whole) + + def test_split_moves_timeline_linked_communications(self): + """A Communication on the timeline only through a Timeline Link moves with the split.""" + issue = self.make_issue("Split source") + referenced = self.make_communication("Referenced", "2024-01-01 10:00:00", reference=issue.name) + linked = self.make_communication("Linked only", "2024-01-01 11:00:00", link_to=issue.name) + + split = issue.split_issue(subject="Split target", communication_id=referenced.name) + + self.assertEqual(frappe.db.get_value("Communication", referenced.name, "reference_name"), split) + self.assertEqual(self.linked_issues(linked.name), [split]) + + def test_split_from_a_timeline_linked_communication(self): + """The Split button is offered on link-only timeline items, so they are valid split points.""" + issue = self.make_issue("Split source") + linked = self.make_communication("Linked only", "2024-01-01 10:00:00", link_to=issue.name) + later = self.make_communication("Later", "2024-01-01 11:00:00", reference=issue.name) + + split = issue.split_issue(subject="Split target", communication_id=linked.name) + + self.assertEqual(self.linked_issues(linked.name), [split]) + self.assertEqual(frappe.db.get_value("Communication", later.name, "reference_name"), split) + + def test_split_leaves_the_reference_of_a_linked_communication_alone(self): + """Moving a Timeline Link must not rewrite a reference that belongs to another Issue.""" + issue = self.make_issue("Split source") + other = self.make_issue("Unrelated issue") + shared = self.make_communication( + "Shared", "2024-01-01 10:00:00", reference=other.name, link_to=issue.name + ) + + split = issue.split_issue(subject="Split target", communication_id=shared.name) + + self.assertEqual(self.linked_issues(shared.name), [split]) + self.assertEqual(frappe.db.get_value("Communication", shared.name, "reference_name"), other.name) + + def test_split_rejects_a_communication_from_another_issue(self): + issue = self.make_issue("Split source") + self.make_communication("Own", "2024-01-01 10:00:00", reference=issue.name) + + other = self.make_issue("Other issue") + theirs = self.make_communication("Theirs", "2024-01-01 09:00:00", reference=other.name) + also_theirs = self.make_communication("Also theirs", "2024-01-01 10:30:00", reference=other.name) + + self.assertRaises(frappe.PermissionError, issue.split_issue, "Split target", theirs.name) + + self.assertEqual(frappe.db.get_value("Communication", theirs.name, "reference_name"), other.name) + self.assertEqual(frappe.db.get_value("Communication", also_theirs.name, "reference_name"), other.name) + + def test_split_rejects_a_communication_that_is_not_on_the_timeline(self): + issue = self.make_issue("Split source") + unattached = self.make_communication("Unattached", "2024-01-01 10:00:00") + + self.assertRaises(frappe.PermissionError, issue.split_issue, "Split target", unattached.name) + + def test_split_requires_write_permission_on_the_issue(self): + issue = self.make_issue("Split source") + own = self.make_communication("Own", "2024-01-01 10:00:00", reference=issue.name) + + frappe.set_user(create_user("split-no-roles@example.com").email) + + self.assertRaises( + frappe.PermissionError, frappe.get_doc("Issue", issue.name).split_issue, "Split target", own.name + )