Merge pull request #16178 from gavindsouza/email-signature-fix

fix: Double signature in composed Email

frappe/core/doctype/communication/communication.py

@@ -18,6 +18,7 @@ from urllib.parse import unquote
 from frappe.utils.user import is_system_user
 from frappe.contacts.doctype.contact.contact import get_contact_name
 from frappe.automation.doctype.assignment_rule.assignment_rule import apply as apply_assignment_rule
+from parse import compile
 
 exclude_from_linked_with = True
 
@@ -114,6 +115,44 @@ class Communication(Document, CommunicationEmailMixin):
 				frappe.publish_realtime('new_message', self.as_dict(),
 					user=self.reference_name, after_commit=True)
 
+	def set_signature_in_email_content(self):
+		"""Set sender's User.email_signature or default outgoing's EmailAccount.signature to the email
+		"""
+		if not self.content:
+			return
+
+		quill_parser = compile('<div class="ql-editor read-mode">{}</div>')
+		email_body = quill_parser.parse(self.content)
+
+		if not email_body:
+			return
+
+		email_body = email_body[0]
+
+		user_email_signature = frappe.db.get_value(
+			"User",
+			self.sender,
+			"email_signature",
+		) if self.sender else None
+
+		signature = user_email_signature or frappe.db.get_value(
+			"Email Account",
+			{"default_outgoing": 1, "add_signature": 1},
+			"signature",
+		)
+
+		if not signature:
+			return
+
+		_signature = quill_parser.parse(signature)[0] if "ql-editor" in signature else None
+
+		if (_signature or signature) not in self.content:
+			self.content = f'{self.content}</p><br><p class="signature">{signature}'
+
+	def before_save(self):
+		if not self.flags.skip_add_signature:
+			self.set_signature_in_email_content()
+
 	def on_update(self):
 		# add to _comment property of the doctype, so it shows up in
 		# comments count for the list view

frappe/core/doctype/communication/email.py

@@ -22,12 +22,30 @@ OUTGOING_EMAIL_ACCOUNT_MISSING = _("""
 
 
 @frappe.whitelist()
-def make(doctype=None, name=None, content=None, subject=None, sent_or_received = "Sent",
-	sender=None, sender_full_name=None, recipients=None, communication_medium="Email", send_email=False,
-	print_html=None, print_format=None, attachments='[]', send_me_a_copy=False, cc=None, bcc=None,
-	flags=None, read_receipt=None, print_letterhead=True, email_template=None, communication_type=None,
-	ignore_permissions=False) -> Dict[str, str]:
-	"""Make a new communication.
+def make(
+	doctype=None,
+	name=None,
+	content=None,
+	subject=None,
+	sent_or_received="Sent",
+	sender=None,
+	sender_full_name=None,
+	recipients=None,
+	communication_medium="Email",
+	send_email=False,
+	print_html=None,
+	print_format=None,
+	attachments="[]",
+	send_me_a_copy=False,
+	cc=None,
+	bcc=None,
+	read_receipt=None,
+	print_letterhead=True,
+	email_template=None,
+	communication_type=None,
+	**kwargs,
+) -> Dict[str, str]:
+	"""Make a new communication. Checks for email permissions for specified Document.
 
 	:param doctype: Reference DocType.
 	:param name: Reference Document name.
@@ -44,17 +62,71 @@ def make(doctype=None, name=None, content=None, subject=None, sent_or_received =
 	:param send_me_a_copy: Send a copy to the sender (default **False**).
 	:param email_template: Template which is used to compose mail .
 	"""
-	is_error_report = (doctype=="User" and name==frappe.session.user and subject=="Error Report")
-	send_me_a_copy = cint(send_me_a_copy)
+	if kwargs:
+		from frappe.utils.commands import warn
+		warn(
+			f"Options {kwargs} used in frappe.core.doctype.communication.email.make "
+			"are deprecated or unsupported",
+			category=DeprecationWarning
+		)
 
-	if not ignore_permissions:
-		if doctype and name and not is_error_report and not frappe.has_permission(doctype, "email", name) and not (flags or {}).get('ignore_doctype_permissions'):
-			raise frappe.PermissionError("You are not allowed to send emails related to: {doctype} {name}".format(
-				doctype=doctype, name=name))
+	if doctype and name and not frappe.has_permission(doctype=doctype, ptype="email", doc=name):
+		raise frappe.PermissionError(
+			f"You are not allowed to send emails related to: {doctype} {name}"
+		)
 
-	if not sender:
-		sender = get_formatted_email(frappe.session.user)
+	return _make(
+		doctype=doctype,
+		name=name,
+		content=content,
+		subject=subject,
+		sent_or_received=sent_or_received,
+		sender=sender,
+		sender_full_name=sender_full_name,
+		recipients=recipients,
+		communication_medium=communication_medium,
+		send_email=send_email,
+		print_html=print_html,
+		print_format=print_format,
+		attachments=attachments,
+		send_me_a_copy=cint(send_me_a_copy),
+		cc=cc,
+		bcc=bcc,
+		read_receipt=read_receipt,
+		print_letterhead=print_letterhead,
+		email_template=email_template,
+		communication_type=communication_type,
+		add_signature=False,
+	)
 
+
+def _make(
+	doctype=None,
+	name=None,
+	content=None,
+	subject=None,
+	sent_or_received="Sent",
+	sender=None,
+	sender_full_name=None,
+	recipients=None,
+	communication_medium="Email",
+	send_email=False,
+	print_html=None,
+	print_format=None,
+	attachments="[]",
+	send_me_a_copy=False,
+	cc=None,
+	bcc=None,
+	read_receipt=None,
+	print_letterhead=True,
+	email_template=None,
+	communication_type=None,
+	add_signature=True,
+) -> Dict[str, str]:
+	"""Internal method to make a new communication that ignores Permission checks.
+	"""
+
+	sender = sender or get_formatted_email(frappe.session.user)
 	recipients = list_to_str(recipients) if isinstance(recipients, list) else recipients
 	cc = list_to_str(cc) if isinstance(cc, list) else cc
 	bcc = list_to_str(bcc) if isinstance(bcc, list) else bcc
@@ -77,7 +149,9 @@ def make(doctype=None, name=None, content=None, subject=None, sent_or_received =
 		"read_receipt":read_receipt,
 		"has_attachment": 1 if attachments else 0,
 		"communication_type": communication_type,
-	}).insert(ignore_permissions=True)
+	})
+	comm.flags.skip_add_signature = not add_signature
+	comm.insert(ignore_permissions=True)
 
 	# if not committed, delayed task doesn't find the communication
 	if attachments:
@@ -87,17 +161,21 @@ def make(doctype=None, name=None, content=None, subject=None, sent_or_received =
 
 	if cint(send_email):
 		if not comm.get_outgoing_email_account():
-			frappe.throw(msg=OUTGOING_EMAIL_ACCOUNT_MISSING, exc=frappe.OutgoingEmailError)
+			frappe.throw(
+				msg=OUTGOING_EMAIL_ACCOUNT_MISSING, exc=frappe.OutgoingEmailError
+			)
 
-		comm.send_email(print_html=print_html, print_format=print_format,
-			send_me_a_copy=send_me_a_copy, print_letterhead=print_letterhead)
+		comm.send_email(
+			print_html=print_html,
+			print_format=print_format,
+			send_me_a_copy=send_me_a_copy,
+			print_letterhead=print_letterhead,
+		)
 
 	emails_not_sent_to = comm.exclude_emails_list(include_sender=send_me_a_copy)
 
-	return {
-		"name": comm.name,
-		"emails_not_sent_to": ", ".join(emails_not_sent_to)
-	}
+	return {"name": comm.name, "emails_not_sent_to": ", ".join(emails_not_sent_to)}
+
 
 def validate_email(doc: "Communication") -> None:
 	"""Validate Email Addresses of Recipients and CC"""

frappe/email/doctype/notification/notification.py

@@ -186,7 +186,7 @@ def get_context(context):
 
 	def send_an_email(self, doc, context):
 		from email.utils import formataddr
-		from frappe.core.doctype.communication.email import make as make_communication
+		from frappe.core.doctype.communication.email import _make as make_communication
 		subject = self.subject
 		if "{" in subject:
 			subject = frappe.render_template(self.subject, context)
@@ -216,7 +216,8 @@ def get_context(context):
 		# Add mail notification to communication list
 		# No need to add if it is already a communication.
 		if doc.doctype != 'Communication':
-			make_communication(doctype=doc.doctype,
+			make_communication(
+				doctype=doc.doctype,
 				name=doc.name,
 				content=message,
 				subject=subject,
@@ -228,7 +229,7 @@ def get_context(context):
 				cc=cc,
 				bcc=bcc,
 				communication_type='Automated Message',
-				ignore_permissions=True)
+			)
 
 	def send_a_slack_msg(self, doc, context):
 		send_slack_message(

frappe/email/email_body.py

@@ -259,17 +259,12 @@ def get_formatted_html(subject, message, footer=None, print_html=None,
 
 	email_account = email_account or EmailAccount.find_outgoing(match_by_email=sender)
 
-	signature = None
-	if "<!-- signature-included -->" not in message:
-		signature = get_signature(email_account)
-
 	rendered_email = frappe.get_template("templates/emails/standard.html").render({
 		"brand_logo": get_brand_logo(email_account) if with_container or header else None,
 		"with_container": with_container,
 		"site_url": get_url(),
 		"header": get_header(header),
 		"content": message,
-		"signature": signature,
 		"footer": get_footer(email_account, footer),
 		"title": subject,
 		"print_html": print_html,
@@ -281,8 +276,7 @@ def get_formatted_html(subject, message, footer=None, print_html=None,
 	if unsubscribe_link:
 		html = html.replace("<!--unsubscribe link here-->", unsubscribe_link.html)
 
-	html = inline_style_in_html(html)
-	return html
+	return inline_style_in_html(html)
 
 @frappe.whitelist()
 def get_email_html(template, args, subject, header=None, with_container=False):

frappe/templates/emails/standard.html

@@ -37,7 +37,6 @@
 								<tr>
 									<td valign="top">
 										<p>{{ content }}</p>
-										<p class="signature">{{ signature }}</p>
 									</td>
 								</tr>
 							</table>

requirements.txt

@@ -29,6 +29,7 @@ maxminddb-geolite2==2018.703
 num2words~=0.5.10
 oauthlib~=3.1.0
 openpyxl~=3.0.7
+parse~=1.19.0
 passlib~=1.7.4
 paytmchecksum~=1.7.0
 pdfkit~=0.6.1