Repository: frappe/frappe
Pull request: fix(UX): send out a system notification when email sending + retries fail
Resolves #21196 

The notification looks like this
![image](https://github.com/frappe/frappe/assets/10119037/ee174bd9-ec70-4d77-82d5-c39cb50daeff)

Clicking on it will take you to the email queue page so you can investigate further

![image](https://github.com/frappe/frappe/assets/10119037/36c07a8d-98f3-49a4-b05d-bae8a98103ea)

[no-docs]

Problem statement:
Send system notification for unsent email
Only way to find out that email I sent failed is to go to that document timeline again and see the status. This is impractical. 


We can just send a system notification for unsent email after all retries are completed.

Hints from issue discussion:

Commit message: refactor: handle notification failure

Patch to review:
diff --git a/frappe/email/doctype/email_queue/email_queue.py b/frappe/email/doctype/email_queue/email_queue.py
index 2dccd65b4e2b..5a4ac1ad7081 100644
--- a/frappe/email/doctype/email_queue/email_queue.py
+++ b/frappe/email/doctype/email_queue/email_queue.py
@@ -6,11 +6,12 @@
 import traceback
 from contextlib import suppress
 from email.parser import Parser
-from email.policy import SMTPUTF8
+from email.policy import SMTPUTF8, default
 
 import frappe
 from frappe import _, safe_encode, task
 from frappe.core.utils import html2text
+from frappe.database.database import savepoint
 from frappe.email.doctype.email_account.email_account import EmailAccount
 from frappe.email.email_body import add_attachment, get_email, get_formatted_html
 from frappe.email.queue import get_unsubcribed_url, get_unsubscribe_message
@@ -259,11 +260,27 @@ def __exit__(self, exc_type, exc_val, exc_tb):
 				)
 			else:
 				update_fields.update({"status": "Error"})
+				self.notify_failed_email()
 		else:
 			update_fields = {"status": "Sent"}
 
 		self.queue_doc.update_status(**update_fields, commit=True)
 
+	@savepoint(catch=Exception)
+	def notify_failed_email(self):
+		# Parse the email body to extract the subject
+		subject = Parser(policy=default).parsestr(self.queue_doc.message)["Subject"]
+
+		# Construct the notification
+		notification = frappe.new_doc("Notification Log")
+		notification.for_user = self.queue_doc.owner
+		notification.set("type", "Alert")
+		notification.from_user = self.queue_doc.owner
+		notification.document_type = self.queue_doc.doctype
+		notification.document_name = self.queue_doc.name
+		notification.subject = _("Failed to send email with subject:") + f" {subject}"
+		notification.insert()
+
 	def update_recipient_status_to_sent(self, recipient):
 		self.sent_to_atleast_one_recipient = True
 		recipient.update_db(status="Sent", commit=True)


Code review comments:
diff_hunk: @@ -39,3 +40,38 @@ def test_email_queue_deletion_based_on_modified_date(self):
 
 		self.assertTrue(frappe.db.exists("Email Queue", new_record.name))
 		self.assertTrue(frappe.db.exists("Email Queue Recipient", {"parent": new_record.name}))
+
+	def test_failed_email_notification(self):
+		subject = frappe.generate_hash()
+		email_record = frappe.new_doc("Email Queue")
+		email_record.sender = "Test <test@example.com>"
+		email_record.message = f"""MIME-Version: 1.0
+Message-Id: {frappe.generate_hash()}
+X-Original-From: Test <test@example.com>
+Subject: {subject}
+From: Test <test@example.com>
+To: <!--recipient-->
+Date: {frappe.utils.now_datetime().strftime('%a, %d %b %Y %H:%M:%S %z')}
+Reply-To: test@example.com
+X-Frappe-Site: {frappe.local.site}
+"""
+		email_record.status = "Error"
+		email_record.retry = get_email_retry_limit()
+		email_record.priority = 1
+		email_record.reference_doctype = "User"
+		email_record.reference_name = "Administrator"
+		email_record.insert()
+
+		# Simulate an exception so that we get a notification
+		try:
+			with SendMailContext(queue_doc=email_record):
+				raise Exception("Test Exception")
+		except Exception:
+			pass
+
+		notification_log = frappe.db.get_value(
+			"Notification Log",
+			{"subject": f"Failed to send email with subject: {subject}"},
+			fieldname="*",
+		)
+		assert notification_log is not None
path: frappe/email/doctype/email_queue/test_email_queue.py
text: @user1:
```suggestion
		self.assertTrue(notification_log)
```

This would work if we were using pytest :grimacing: 

@author:
Ah, judging by method name I had initially assumes it just checks for true explicitly, just saw its not that :P switched

diff_hunk: @@ -39,3 +40,38 @@ def test_email_queue_deletion_based_on_modified_date(self):
 
 		self.assertTrue(frappe.db.exists("Email Queue", new_record.name))
 		self.assertTrue(frappe.db.exists("Email Queue Recipient", {"parent": new_record.name}))
+
+	def test_failed_email_notification(self):
+		subject = frappe.generate_hash()
+		email_record = frappe.new_doc("Email Queue")
+		email_record.sender = "Test <test@example.com>"
+		email_record.message = f"""MIME-Version: 1.0
+Message-Id: {frappe.generate_hash()}
+X-Original-From: Test <test@example.com>
+Subject: {subject}
+From: Test <test@example.com>
+To: <!--recipient-->
+Date: {frappe.utils.now_datetime().strftime('%a, %d %b %Y %H:%M:%S %z')}
+Reply-To: test@example.com
+X-Frappe-Site: {frappe.local.site}
+"""
path: frappe/email/doctype/email_queue/test_email_queue.py
text: @user1:
use `textwrap.indent` to avoid writing indented strings like this. 

@author:
Didn't understand the usage of it here, did you mean to indent the string and use textwrap.dedent()?