Merge pull request #958 from koriaf/master

fix(email): Add ability to attach full first email text to avoid losing forwards, and to save .eml files for any incoming mesages, plus fix tests and some minor bugs.
django-helpdesk · Apr 20, 2021 · 81fae1a · 81fae1a
2 parents 012ba4f + 2b4c82f
commit 81fae1a
Show file tree

Hide file tree

Showing 8 changed files with 128 additions and 30 deletions.
diff --git a/docs/settings.rst b/docs/settings.rst
@@ -223,3 +223,7 @@ The following settings were defined in previous versions and are no longer suppo
 - **HELPDESK_FOOTER_SHOW_CHANGE_LANGUAGE_LINK** Is never shown. Use your own template if required.
 
 - **HELPDESK_ENABLE_PER_QUEUE_MEMBERSHIP** Discontinued in favor of HELPDESK_ENABLE_PER_QUEUE_STAFF_PERMISSION.
+
+- **HELPDESK_FULL_FIRST_MESSAGE_FROM_EMAIL** Do not ignore fowarded and replied text from the email messages which create a new ticket; useful for cases when customer forwards some email (error from service or something) and wants support to see that
+
+- **HELPDESK_ALWAYS_SAVE_INCOMING_EMAIL_MESSAGE** Any incoming .eml message is saved and available, helps when customer spent some time doing fancy markup which has been corrupted during the email-to-ticket-comment translate process
diff --git a/helpdesk/.flake8 b/helpdesk/.flake8
@@ -0,0 +1,3 @@
+[flake8]
+max-line-length = 120
+import-order-style = pep8
diff --git a/helpdesk/email.py b/helpdesk/email.py
@@ -21,6 +21,7 @@
 from time import ctime
 
 from bs4 import BeautifulSoup
+from django.conf import settings as django_settings
 from django.contrib.auth import get_user_model
 from django.core.exceptions import ValidationError
 from django.core.files.uploadedfile import SimpleUploadedFile
@@ -394,7 +395,7 @@ def create_object_from_email_message(message, ticket_id, payload, files, logger)
         title=_('E-Mail Received from %(sender_email)s' % {'sender_email': sender_email}),
         date=now,
         public=True,
-        comment=payload['body'],
+        comment=payload.get('full_body', payload['body']) or "",
         message_id=message_id
     )
 
@@ -505,6 +506,7 @@ def object_from_message(message, queue, logger):
         ticket = None
 
     body = None
+    full_body = None
     counter = 0
     files = []
 
@@ -523,7 +525,19 @@ def object_from_message(message, queue, logger):
                 if part['Content-Transfer-Encoding'] == '8bit' and part.get_content_charset() == 'utf-8':
                     body = body.decode('unicode_escape')
                 body = decodeUnknown(part.get_content_charset(), body)
-                body = EmailReplyParser.parse_reply(body)
+                # have to use django_settings here so overwritting it works in tests
+                # the default value is False anyway
+                if ticket is None and getattr(django_settings, 'HELPDESK_FULL_FIRST_MESSAGE_FROM_EMAIL', False):
+                    # first message in thread, we save full body to avoid losing forwards and things like that
+                    body_parts = []
+                    for f in EmailReplyParser.read(body).fragments:
+                        body_parts.append(f.content)
+                    full_body = '\n\n'.join(body_parts)
+                    body = EmailReplyParser.parse_reply(body)
+                else:
+                    # second and other reply, save only first part of the message
+                    body = EmailReplyParser.parse_reply(body)
+                    full_body = body
                 # workaround to get unicode text out rather than escaped text
                 try:
                     body = body.encode('ascii').decode('unicode_escape')
@@ -536,13 +550,23 @@ def object_from_message(message, queue, logger):
                 except UnicodeDecodeError:
                     email_body = encoding.smart_text(part.get_payload(decode=False))
 
-                payload = """
-<html>
-<head>
-<meta charset="utf-8"/>
-</head>
-%s
-</html>""" % email_body
+                if not body and not full_body:
+                    # no text has been parsed so far - try such deep parsing for some messages
+                    altered_body = email_body.replace("</p>", "</p>\n").replace("<br", "\n<br")
+                    mail = BeautifulSoup(str(altered_body), "html.parser")
+                    full_body = mail.get_text()
+
+                if "<body" not in email_body:
+                    email_body = f"<body>{email_body}</body>"
+
+                payload = (
+                    '<html>'
+                    '<head>'
+                    '<meta charset="utf-8" />'
+                    '</head>'
+                    '%s'
+                    '</html>'
+                ) % email_body
                 files.append(
                     SimpleUploadedFile(_("email_html_body.html"), payload.encode("utf-8"), 'text/html')
                 )
@@ -554,22 +578,22 @@ def object_from_message(message, queue, logger):
             else:
                 name = ("part-%i_" % counter) + name
 
-            # FIXME: this code gets the paylods, then does something with it and then completely ignores it
-            # writing the part.get_payload(decode=True) instead; and then the payload variable is
-            # replaced by some dict later.
-            # the `payloadToWrite` has been also ignored so was commented
-            payload = part.get_payload()
-            if isinstance(payload, list):
-                payload = payload.pop().as_string()
-            # payloadToWrite = payload
-            # check version of python to ensure use of only the correct error type
-            non_b64_err = TypeError
-            try:
-                logger.debug("Try to base64 decode the attachment payload")
-                # payloadToWrite = base64.decodebytes(payload)
-            except non_b64_err:
-                logger.debug("Payload was not base64 encoded, using raw bytes")
-                # payloadToWrite = payload
+            # # FIXME: this code gets the paylods, then does something with it and then completely ignores it
+            # # writing the part.get_payload(decode=True) instead; and then the payload variable is
+            # # replaced by some dict later.
+            # # the `payloadToWrite` has been also ignored so was commented
+            # payload = part.get_payload()
+            # if isinstance(payload, list):
+            #     payload = payload.pop().as_string()
+            # # payloadToWrite = payload
+            # # check version of python to ensure use of only the correct error type
+            # non_b64_err = TypeError
+            # try:
+            #     logger.debug("Try to base64 decode the attachment payload")
+            #     # payloadToWrite = base64.decodebytes(payload)
+            # except non_b64_err:
+            #     logger.debug("Payload was not base64 encoded, using raw bytes")
+            #     # payloadToWrite = payload
             files.append(SimpleUploadedFile(name, part.get_payload(decode=True), mimetypes.guess_type(name)[0]))
             logger.debug("Found MIME attachment %s" % name)
 
@@ -581,18 +605,33 @@ def object_from_message(message, queue, logger):
         if beautiful_body:
             try:
                 body = beautiful_body.text
+                full_body = body
             except AttributeError:
                 pass
         if not body:
             body = ""
 
+    if getattr(django_settings, 'HELPDESK_ALWAYS_SAVE_INCOMING_EMAIL_MESSAGE', False):
+        # save message as attachment in case of some complex markup renders wrong
+        files.append(
+            SimpleUploadedFile(
+                _("original_message.eml").replace(
+                    ".eml",
+                    timezone.localtime().strftime("_%d-%m-%Y_%H:%M") + ".eml"
+                ),
+                str(message).encode("utf-8"),
+                'text/plain'
+            )
+        )
+
     smtp_priority = message.get('priority', '')
     smtp_importance = message.get('importance', '')
     high_priority_types = {'high', 'important', '1', 'urgent'}
     priority = 2 if high_priority_types & {smtp_priority, smtp_importance} else 3
 
     payload = {
         'body': body,
+        'full_body': full_body or body,
         'subject': subject,
         'queue': queue,
         'sender_email': sender_email,

diff --git a/helpdesk/settings.py b/helpdesk/settings.py
@@ -162,3 +162,12 @@
 
 # use https in the email links
 HELPDESK_USE_HTTPS_IN_EMAIL_LINK = getattr(settings, 'HELPDESK_USE_HTTPS_IN_EMAIL_LINK', False)
+
+# Include all signatures and forwards in the first ticket message if set
+# Useful if you get forwards dropped from them while they are useful part of request
+HELPDESK_FULL_FIRST_MESSAGE_FROM_EMAIL = getattr(settings, 'HELPDESK_FULL_FIRST_MESSAGE_FROM_EMAIL', False)
+
+# If set then we always save incoming emails as .eml attachments
+# which is quite noisy but very helpful for complicated markup, forwards and so on
+# (which gets stripped/corrupted otherwise)
+HELPDESK_ALWAYS_SAVE_INCOMING_EMAIL_MESSAGE = getattr(settings, "HELPDESK_ALWAYS_SAVE_INCOMING_EMAIL_MESSAGE", False)
diff --git a/helpdesk/templates/helpdesk/ticket.html b/helpdesk/templates/helpdesk/ticket.html
@@ -50,7 +50,7 @@ <h5 class="mb-1">{{ followup.title|num_to_link }}</h5>
                             </div>
                             <p class="mb-1">
                                 {% if followup.comment %}
-                                    <p>{{ followup.get_markdown|urlizetrunc:50|num_to_link|linebreaksbr }}</p>
+                                    <p>{{ followup.get_markdown|urlizetrunc:50|num_to_link }}</p>
                                 {% endif %}
                                 {% for change in followup.ticketchange_set.all %}
                                     {% if forloop.first %}<div class='changes'><ul>{% endif %}

diff --git a/helpdesk/templates/helpdesk/ticket_desc_table.html b/helpdesk/templates/helpdesk/ticket_desc_table.html
@@ -56,7 +56,7 @@
                         <td class="{% if ticket.priority < 3 %}table-warning{% endif %}">{{ ticket.get_priority_display }}
                         </td>
                         <th class="table-active">{% trans "Copies To" %}</th>
-                        <td>{{ ticketcc_string }} <a data-toggle='tooltip' href='{% url 'helpdesk:ticket_cc' ticket.id %}' title='{% trans "Click here to add / remove people who should receive an e-mail whenever this ticket is updated." %}'><strong><button type="button" class="btn btn-warning btn-sm float-right"><i class="fa fa-share"></i></button></strong></a>{% if SHOW_SUBSCRIBE %}, <strong><a data-toggle='tooltip' href='?subscribe' title='{% trans "Click here to subscribe yourself to this ticket, if you want to receive an e-mail whenever this ticket is updated." %}'><button type="button" class="btn btn-warning btn-sm float-right"><i class="fas fa-rss-square"></i></button></a></strong>{% endif %}</td>
+                        <td>{{ ticketcc_string }} <a data-toggle='tooltip' href='{% url 'helpdesk:ticket_cc' ticket.id %}' title='{% trans "Click here to add / remove people who should receive an e-mail whenever this ticket is updated." %}'><strong><button type="button" class="btn btn-warning btn-sm float-right"><i class="fa fa-share"></i></button></strong></a>{% if SHOW_SUBSCRIBE %} <strong><a data-toggle='tooltip' href='?subscribe' title='{% trans "Click here to subscribe yourself to this ticket, if you want to receive an e-mail whenever this ticket is updated." %}'><button type="button" class="btn btn-warning btn-sm float-right"><i class="fas fa-rss-square"></i></button></a></strong>{% endif %}</td>
                     </tr>
 
                     <tr>
@@ -99,7 +99,7 @@
                     <tr>
                         <td id="ticket-description" colspan='4'>
                             <h4>{% trans "Description" %}</h4>
-                            {{ ticket.get_markdown|urlizetrunc:50|num_to_link|linebreaksbr }}</td>
+                            {{ ticket.get_markdown|urlizetrunc:50|num_to_link }}</td>
                     </tr>
 
                     {% if ticket.resolution %}<tr>

diff --git a/helpdesk/tests/test_files/forwarded-message.eml b/helpdesk/tests/test_files/forwarded-message.eml
@@ -0,0 +1,27 @@
+Return-Path: sender@domain.tld
+To: recipient@domain.tld
+From: The Sender <sender@domain.tld>
+Subject: Test with original message from GitHub
+Message-ID: <0d3a17d5-7136-75c0-c8ba-a7698c57ac42@gmail.com>
+Date: Tue, 13 Apr 2021 12:56:39 +0200
+User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
+ Thunderbird/78.8.1
+MIME-Version: 1.0
+Content-Type: text/plain; charset=utf-8; format=flowed
+Content-Transfer-Encoding: 7bit
+Content-Language: en-US
+
+This is email body
+
+-----Original Message-----
+From: GitHub <noreply@github.com>
+Sent: Tuesday, 6 April 2021 9:09 AM
+Subject: [GitHub API] Update notice for access token [SEC=UNOFFICIAL]
+
+Hello there!
+
+We noticed that, at April 5th, 2021 at 23:09 (UTC) your application accessed the GitHub API with a token that has an outdated format.
+
+Thanks,
+The GitHub Team
+
diff --git a/helpdesk/tests/test_get_email.py b/helpdesk/tests/test_get_email.py
@@ -1,5 +1,5 @@
 # -*- coding: utf-8 -*-
-from django.test import TestCase
+from django.test import TestCase, override_settings
 from django.core.management import call_command
 from django.shortcuts import get_object_or_404
 from django.contrib.auth.models import User
@@ -84,9 +84,11 @@ def test_email_with_8bit_encoding_and_utf_8(self):
         self.assertEqual(ticket.title, "Testovácí email")
         self.assertEqual(ticket.description, "íářčšáíéřášč")
 
+    @override_settings(HELPDESK_FULL_FIRST_MESSAGE_FROM_EMAIL=True)
     def test_email_with_utf_8_non_decodable_sequences(self):
         """
         Tests that emails with utf-8 non-decodable sequences are parsed correctly
+        The message is fowarded as well
         """
         with open(os.path.join(THIS_DIR, "test_files/utf-nondecodable.eml")) as fd:
             test_email = fd.read()
@@ -99,6 +101,20 @@ def test_email_with_utf_8_non_decodable_sequences(self):
         attachment = attachments[0]
         self.assertIn('prosazuje lepší', attachment.file.read().decode("utf-8"))
 
+    @override_settings(HELPDESK_FULL_FIRST_MESSAGE_FROM_EMAIL=True)
+    def test_email_with_forwarded_message(self):
+        """
+        Forwarded message of that format must be still attached correctly
+        """
+        with open(os.path.join(THIS_DIR, "test_files/forwarded-message.eml")) as fd:
+            test_email = fd.read()
+        ticket = helpdesk.email.object_from_message(test_email, self.queue_public, self.logger)
+        self.assertEqual(ticket.title, "Test with original message from GitHub")
+        self.assertIn("This is email body", ticket.description)
+        assert "Hello there!" not in ticket.description, ticket.description
+        assert FollowUp.objects.filter(ticket=ticket).count() == 1
+        assert "Hello there!" in FollowUp.objects.filter(ticket=ticket).first().comment
+
 
 class GetEmailParametricTemplate(object):
     """TestCase that checks basic email functionality across methods and socks configs."""
@@ -555,7 +571,7 @@ def test_read_pgp_signed_email(self):
             self.assertEqual(followup1.ticket.id, 1)
             attach1 = get_object_or_404(FollowUpAttachment, pk=1)
             self.assertEqual(attach1.followup.id, 1)
-            self.assertEqual(attach1.filename, 'signature.asc')
+            self.assertEqual(attach1.filename, 'part-1_signature.asc')
             self.assertEqual(attach1.file.read(), b"""-----BEGIN PGP SIGNATURE-----
 
 iQIcBAEBCAAGBQJaA3dnAAoJELBLc7QPITnLN54P/3Zsu7+AIQWDFTvziJfCqswG