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.
2021-04-19 23:38:57 -04:00 · 2021-04-19 23:38:57 -04:00 · 81fae1aefe
commit 81fae1aefe
parent 012ba4f42d 2b4c82fd1b
8 changed files with 128 additions and 30 deletions
--- 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
--- a/helpdesk/.flake8
+++ b/helpdesk/.flake8
@ -0,0 +1,3 @@
+[flake8]
+max-line-length = 120
+import-order-style = pep8
--- a/helpdesk/email.py
+++ b/helpdesk/email.py
@ -21,6 +21,7 @@ from os.path import isfile, join
 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,11 +605,25 @@ 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'}
@ -593,6 +631,7 @@ def object_from_message(message, queue, logger):

    payload = {
        'body': body,
+        'full_body': full_body or body,
        'subject': subject,
        'queue': queue,
        'sender_email': sender_email,
--- a/helpdesk/settings.py
+++ b/helpdesk/settings.py
@ -162,3 +162,12 @@ HELPDESK_ENABLE_PER_QUEUE_STAFF_PERMISSION = getattr(

 # 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)
--- a/helpdesk/templates/helpdesk/ticket.html
+++ b/helpdesk/templates/helpdesk/ticket.html
@ -50,7 +50,7 @@
                            </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 %}
--- 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>
--- 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
+
--- 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 @@ class GetEmailCommonTests(TestCase):
        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 @@ class GetEmailCommonTests(TestCase):
        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 @@ class GetEmailParametricTemplate(object):
            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