Merge pull request #507 from gwasser/bugfix

Fix a Few Bugs and Update Mail Handling

.travis.yml

@@ -7,9 +7,10 @@ python:
     - "3.6"
 
 env:
-    - DJANGO=1.8.17
+    - DJANGO=1.8.18
-    - DJANGO=1.9.12
+    - DJANGO=1.9.13
-    - DJANGO=1.10.4
+    - DJANGO=1.10.7
+    - DJANGO=1.11
 
 install:
     - pip install -q Django==$DJANGO

docs/install.rst

@@ -159,6 +159,14 @@ You must create the database the holds the django-helpdesk tables using the
 UTF-8 collation; see the MySQL manual for more information:
 http://dev.mysql.com/doc/refman/5.1/en/charset-database.html
 
+You may be able to convert an existing MySQL database to use UTF-8 collation
+by using the following SQL commands::
+
+    ALTER DATABASE mydatabase CHARACTER SET utf8 COLLATE utf8_general_ci;
+    ALTER TABLE helpdesk_emailtemplate CONVERT TO CHARACTER SET utf8 COLLATE utf8_general_ci;
+
+Both ``utf8_general_ci`` or ``utf16_general_ci`` have been reported to work.
+
 If you do NOT do this step, and you only want to use English-language templates,
-you can continue however you will receive a warning when running the 'migrate'
+you may be able to continue however you will receive a warning when running the
-commands.
+'migrate' commands.

helpdesk/management/commands/get_email.py

@@ -34,7 +34,8 @@ from django.utils import encoding, six, timezone
 
 from helpdesk import settings
 from helpdesk.lib import send_templated_mail, safe_template_context, process_attachments
-from helpdesk.models import Queue, Ticket, FollowUp, IgnoreEmail
+from helpdesk.models import Queue, Ticket, TicketCC, FollowUp, IgnoreEmail
+from django.contrib.auth.models import User
 
 import logging
 
@@ -264,7 +265,7 @@ def decode_mail_headers(string):
 def ticket_from_message(message, queue, logger):
     # 'message' must be an RFC822 formatted message.
     message = email.message_from_string(message) if six.PY3 else email.message_from_string(message.encode('utf-8'))
-    subject = message.get('subject', _('Created from e-mail'))
+    subject = message.get('subject', _('Comment from e-mail'))
     subject = decode_mail_headers(decodeUnknown(message.get_charset(), subject))
     for affix in STRIPPED_SUBJECT_STRINGS:
         subject = subject.replace(affix, "")
@@ -274,6 +275,17 @@ def ticket_from_message(message, queue, logger):
     sender = decode_mail_headers(decodeUnknown(message.get_charset(), sender))
     sender_email = email.utils.parseaddr(sender)[1]
 
+    cc = message.get_all('cc', None)
+    if cc:
+        # first, fixup the encoding if necessary
+        cc = [decode_mail_headers(decodeUnknown(message.get_charset(), x)) for x in cc]
+        # get_all checks if multiple CC headers, but individual emails may be comma separated too
+        tempcc = []
+        for hdr in cc:
+            tempcc.extend(hdr.split(','))
+        # use a set to ensure no duplicates
+        cc = set([x.strip() for x in tempcc])
+
     for ignore in IgnoreEmail.objects.filter(Q(queues=queue) | Q(queues__isnull=True)):
         if ignore.test(sender_email):
             if ignore.keep_in_mailbox:
@@ -358,6 +370,44 @@ def ticket_from_message(message, queue, logger):
         )
         logger.debug("Created new ticket %s-%s" % (t.queue.slug, t.id))
 
+    if cc:
+        # get list of currently CC'd emails
+        current_cc = TicketCC.objects.filter(ticket=ticket)
+        current_cc_emails = [x.email for x in current_cc]
+        # get emails of any Users CC'd to email
+        current_cc_users = [x.user.email for x in current_cc]
+        # ensure submitter, assigned user, queue email not added
+        other_emails = [queue.email_address]
+        if t.submitter_email:
+            other_emails.append(t.submitter_email)
+        if t.assigned_to:
+            other_emails.append(t.assigned_to.email)
+        current_cc = set(current_cc_emails + current_cc_users + other_emails)
+        # first, add any User not previously CC'd (as identified by User's email)
+        all_users = User.objects.all()
+        all_user_emails = set([x.email for x in all_users])
+        users_not_currently_ccd = all_user_emails.difference(set(current_cc))
+        users_to_cc = cc.intersection(users_not_currently_ccd)
+        for user in users_to_cc:
+            tcc = TicketCC.objects.create(
+                ticket=t,
+                user=User.objects.get(email=user),
+                can_view=True,
+                can_update=False
+            )
+            tcc.save()
+        # then add remaining emails alphabetically, makes testing easy
+        new_cc = cc.difference(current_cc).difference(all_user_emails)
+        new_cc = sorted(list(new_cc))
+        for ccemail in new_cc:
+            tcc = TicketCC.objects.create(
+                ticket=t,
+                email=ccemail,
+                can_view=True,
+                can_update=False
+            )
+            tcc.save()
+
     f = FollowUp(
         ticket=t,
         title=_('E-Mail Received from %(sender_email)s' % {'sender_email': sender_email}),

helpdesk/tests/test_get_email.py

@@ -2,11 +2,13 @@
 
 from __future__ import unicode_literals
 
-from helpdesk.models import Queue, Ticket, FollowUp, Attachment
+from helpdesk.models import Queue, Ticket, TicketCC, FollowUp, Attachment
 from django.test import TestCase
 from django.core.management import call_command
 from django.utils import six
 from django.shortcuts import get_object_or_404
+from django.contrib.auth.models import User
+from django.contrib.auth.hashers import make_password
 import itertools
 from shutil import rmtree
 import sys
@@ -44,13 +46,13 @@ class GetEmailCommonTests(TestCase):
 
 
 class GetEmailParametricTemplate(object):
-    """TestCase that checks email functionality across methods and socks configs."""
+    """TestCase that checks basic email functionality across methods and socks configs."""
 
     def setUp(self):
 
         self.temp_logdir = mkdtemp()
         kwargs = {
-            "title": 'Queue 1',
+            "title": 'Basic Queue',
             "slug": 'QQ',
             "allow_public_submission": True,
             "allow_email_submission": True,
@@ -148,6 +150,79 @@ class GetEmailParametricTemplate(object):
             self.assertEqual(ticket2.title, test_email_subject)
             self.assertEqual(ticket2.description, test_email_body)
 
+    def test_read_email_with_template_tag(self):
+        """Tests reading plain text emails from a queue and creating tickets,
+           except this time the email body contains a Django template tag.
+           For each email source supported, we mock the backend to provide
+           authentically formatted responses containing our test data."""
+
+        # example email text from Django docs: https://docs.djangoproject.com/en/1.10/ref/unicode/
+        test_email_from = "Arnbjörg Ráðormsdóttir <arnbjorg@example.com>"
+        test_email_subject = "My visit to Sør-Trøndelag"
+        test_email_body = "Reporting some issue with the template tag: {% if helpdesk %}."
+        test_email = "To: helpdesk@example.com\nFrom: " + test_email_from + "\nSubject: " + test_email_subject + "\n\n" + test_email_body
+        test_mail_len = len(test_email)
+
+        if self.socks:
+            from socks import ProxyConnectionError
+            with self.assertRaisesRegexp(ProxyConnectionError, '%s:%s' % (unrouted_socks_server, unused_port)):
+                call_command('get_email')
+
+        else:
+            # Test local email reading
+            if self.method == 'local':
+                with mock.patch('helpdesk.management.commands.get_email.listdir') as mocked_listdir, \
+                        mock.patch('helpdesk.management.commands.get_email.isfile') as mocked_isfile, \
+                        mock.patch('builtins.open' if six.PY3 else '__builtin__.open', mock.mock_open(read_data=test_email)):
+                    mocked_isfile.return_value = True
+                    mocked_listdir.return_value = ['filename1', 'filename2']
+
+                    call_command('get_email')
+
+                    mocked_listdir.assert_called_with('/var/lib/mail/helpdesk/')
+                    mocked_isfile.assert_any_call('/var/lib/mail/helpdesk/filename1')
+                    mocked_isfile.assert_any_call('/var/lib/mail/helpdesk/filename2')
+
+            elif self.method == 'pop3':
+                # mock poplib.POP3's list and retr methods to provide responses as per RFC 1939
+                pop3_emails = {
+                    '1': ("+OK", test_email.split('\n')),
+                    '2': ("+OK", test_email.split('\n')),
+                }
+                pop3_mail_list = ("+OK 2 messages", ("1 %d" % test_mail_len, "2 %d" % test_mail_len))
+                mocked_poplib_server = mock.Mock()
+                mocked_poplib_server.list = mock.Mock(return_value=pop3_mail_list)
+                mocked_poplib_server.retr = mock.Mock(side_effect=lambda x: pop3_emails[x])
+                with mock.patch('helpdesk.management.commands.get_email.poplib', autospec=True) as mocked_poplib:
+                    mocked_poplib.POP3 = mock.Mock(return_value=mocked_poplib_server)
+                    call_command('get_email')
+
+            elif self.method == 'imap':
+                # mock imaplib.IMAP4's search and fetch methods with responses from RFC 3501
+                imap_emails = {
+                    "1": ("OK", (("1", test_email),)),
+                    "2": ("OK", (("2", test_email),)),
+                }
+                imap_mail_list = ("OK", ("1 2",))
+                mocked_imaplib_server = mock.Mock()
+                mocked_imaplib_server.search = mock.Mock(return_value=imap_mail_list)
+
+                # we ignore the second arg as the data item/mime-part is constant (RFC822)
+                mocked_imaplib_server.fetch = mock.Mock(side_effect=lambda x, _: imap_emails[x])
+                with mock.patch('helpdesk.management.commands.get_email.imaplib', autospec=True) as mocked_imaplib:
+                    mocked_imaplib.IMAP4 = mock.Mock(return_value=mocked_imaplib_server)
+                    call_command('get_email')
+
+            ticket1 = get_object_or_404(Ticket, pk=1)
+            self.assertEqual(ticket1.ticket_for_url, "QQ-%s" % ticket1.id)
+            self.assertEqual(ticket1.title, test_email_subject)
+            self.assertEqual(ticket1.description, test_email_body)
+
+            ticket2 = get_object_or_404(Ticket, pk=2)
+            self.assertEqual(ticket2.ticket_for_url, "QQ-%s" % ticket2.id)
+            self.assertEqual(ticket2.title, test_email_subject)
+            self.assertEqual(ticket2.description, test_email_body)
+
     def test_read_html_multipart_email(self):
         """Tests reading multipart MIME (HTML body and plain text alternative)
            emails from a queue and creating tickets.
@@ -160,6 +235,12 @@ class GetEmailParametricTemplate(object):
 
         me = "my@example.com"
         you = "your@example.com"
+        # NOTE: CC'd emails need to be alphabetical and tested as such!
+        # implementation uses sets, so only way to ensure tickets created
+        # in right order is to change set to list and sort it
+        cc_one = "nobody@example.com"
+        cc_two = "other@example.com"
+        cc = cc_one + ", " + cc_two
         subject = "Link"
 
         # Create message container - the correct MIME type is multipart/alternative.
@@ -167,6 +248,7 @@ class GetEmailParametricTemplate(object):
         msg['Subject'] = subject
         msg['From'] = me
         msg['To'] = you
+        msg['Cc'] = cc
 
         # Create the body of the message (a plain-text and an HTML version).
         text = "Hi!\nHow are you?\nHere is the link you wanted:\nhttps://www.python.org"
@@ -255,6 +337,11 @@ class GetEmailParametricTemplate(object):
             attach1 = get_object_or_404(Attachment, pk=1)
             self.assertEqual(attach1.followup.id, 1)
             self.assertEqual(attach1.filename, 'email_html_body.html')
+            cc1 = get_object_or_404(TicketCC, pk=1)
+            self.assertEqual(cc1.email, cc_one)
+            cc2 = get_object_or_404(TicketCC, pk=2)
+            self.assertEqual(cc2.email, cc_two)
+            self.assertEqual(len(TicketCC.objects.filter(ticket=1)), 2)
 
             ticket2 = get_object_or_404(Ticket, pk=2)
             self.assertEqual(ticket2.ticket_for_url, "QQ-%s" % ticket2.id)
@@ -269,6 +356,139 @@ class GetEmailParametricTemplate(object):
             self.assertEqual(attach2.filename, 'email_html_body.html')
 
 
+class GetEmailCCHandling(TestCase):
+    """TestCase that checks CC handling in email. Needs its own test harness."""
+
+    def setUp(self):
+
+        self.temp_logdir = mkdtemp()
+
+        kwargs = {
+            "title": 'CC Queue',
+            "slug": 'CC',
+            "allow_public_submission": True,
+            "allow_email_submission": True,
+            "email_address": 'queue@example.com',
+            "email_box_type": 'local',
+            "email_box_local_dir": '/var/lib/mail/helpdesk/',
+            "logging_dir": self.temp_logdir,
+            "logging_type": 'none'
+        }
+        self.queue_public = Queue.objects.create(**kwargs)
+
+        user1_kwargs = {
+            'username': 'staff',
+            'email': 'staff@example.com',
+            'password': make_password('Test1234'),
+            'is_staff': True,
+            'is_superuser': False,
+            'is_active': True
+        }
+        self.staff_user = User.objects.create(**user1_kwargs)
+
+        user2_kwargs = {
+            'username': 'assigned',
+            'email': 'assigned@example.com',
+            'password': make_password('Test1234'),
+            'is_staff': True,
+            'is_superuser': False,
+            'is_active': True
+        }
+        self.assigned_user = User.objects.create(**user2_kwargs)
+
+        user3_kwargs = {
+            'username': 'observer',
+            'email': 'observer@example.com',
+            'password': make_password('Test1234'),
+            'is_staff': True,
+            'is_superuser': False,
+            'is_active': True
+        }
+        self.observer_user = User.objects.create(**user3_kwargs)
+
+        ticket_kwargs = {
+            'title': 'Original Ticket',
+            'queue': self.queue_public,
+            'submitter_email': 'submitter@example.com',
+            'assigned_to': self.assigned_user,
+            'status': 1
+        }
+        self.original_ticket = Ticket.objects.create(**ticket_kwargs)
+
+        cc_kwargs = {
+            'ticket': self.original_ticket,
+            'user': self.staff_user,
+            'can_view': True,
+            'can_update': True
+        }
+        self.original_cc = TicketCC.objects.create(**cc_kwargs)
+
+    def tearDown(self):
+
+        rmtree(self.temp_logdir)
+
+    def test_read_email_cc(self):
+        """Tests reading plain text emails from a queue and adding to a ticket,
+           particularly to test appropriate handling of CC'd emails."""
+
+        # first, check that test ticket exists
+        ticket1 = get_object_or_404(Ticket, pk=1)
+        self.assertEqual(ticket1.ticket_for_url, "CC-1")
+        self.assertEqual(ticket1.title, "Original Ticket")
+        # only the staff_user is CC'd for now
+        self.assertEqual(len(TicketCC.objects.filter(ticket=1)), 1)
+        ccstaff = get_object_or_404(TicketCC, pk=1)
+        self.assertEqual(ccstaff.user, User.objects.get(username='staff'))
+        self.assertEqual(ticket1.assigned_to, User.objects.get(username='assigned'))
+
+        # example email text from Django docs: https://docs.djangoproject.com/en/1.10/ref/unicode/
+        test_email_from = "submitter@example.com"
+        # NOTE: CC emails are in alphabetical order and must be tested as such!
+        # implementation uses sets, so only way to ensure tickets created
+        # in right order is to change set to list and sort it
+        test_email_cc_one = "Alice Ráðormsdóttir <alice@example.com>"
+        test_email_cc_two = "nobody@example.com"
+        test_email_cc_three = "other@example.com"
+        test_email_cc_four = "someone@example.com"
+        ticket_user_emails = "assigned@example.com, staff@example.com, submitter@example.com, observer@example.com, queue@example.com"
+        test_email_subject = "[CC-1] My visit to Sør-Trøndelag"
+        test_email_body = "Unicode helpdesk comment with an s-hat (ŝ) via email."
+        test_email = "To: queue@example.com\nCc: " + test_email_cc_one + ", " + test_email_cc_one + ", " + test_email_cc_two + ", " + test_email_cc_three + "\nCC: " + test_email_cc_one + ", " + test_email_cc_three + ", " + test_email_cc_four + ", " + ticket_user_emails + "\nFrom: " + test_email_from + "\nSubject: " + test_email_subject + "\n\n" + test_email_body
+        test_mail_len = len(test_email)
+
+        with mock.patch('helpdesk.management.commands.get_email.listdir') as mocked_listdir, \
+                mock.patch('helpdesk.management.commands.get_email.isfile') as mocked_isfile, \
+                mock.patch('builtins.open' if six.PY3 else '__builtin__.open', mock.mock_open(read_data=test_email)):
+
+            mocked_isfile.return_value = True
+            mocked_listdir.return_value = ['filename1']
+
+            call_command('get_email')
+
+            mocked_listdir.assert_called_with('/var/lib/mail/helpdesk/')
+            mocked_isfile.assert_any_call('/var/lib/mail/helpdesk/filename1')
+
+        # ensure these 4 CCs (test_email_cc one thru four) are the only ones
+        # created and added to the existing staff_user that was CC'd,
+        # and the observer user that gets CC'd to new email.,
+        # and that submitter and assignee are not added as CC either
+        # (in other words, even though everyone was CC'd to this email,
+        #  we should come out with only 6 CCs after filtering)
+        self.assertEqual(len(TicketCC.objects.filter(ticket=1)), 6)
+        # next we make sure no duplicates were added, and the
+        # staff users nor submitter were not re-added as email TicketCCs
+        cc0 = get_object_or_404(TicketCC, pk=2)
+        self.assertEqual(cc0.user, User.objects.get(username='observer'))
+        cc1 = get_object_or_404(TicketCC, pk=3)
+        self.assertEqual(cc1.email, test_email_cc_one)
+        cc2 = get_object_or_404(TicketCC, pk=4)
+        self.assertEqual(cc2.email, test_email_cc_two)
+        cc3 = get_object_or_404(TicketCC, pk=5)
+        self.assertEqual(cc3.email, test_email_cc_three)
+        cc4 = get_object_or_404(TicketCC, pk=6)
+        self.assertEqual(cc4.email, test_email_cc_four)
+
+
 # build matrix of test cases
 case_methods = [c[0] for c in Queue._meta.get_field('email_box_type').choices]
 case_socks = [False] + [c[0] for c in Queue._meta.get_field('socks_proxy_type').choices]
@@ -287,7 +507,5 @@ for method, socks in case_matrix:
     test_name = str(
         "TestGetEmail%s%s" % (method.capitalize(), socks_str))
 
-    cl = type(test_name, (GetEmailParametricTemplate, TestCase,), {
+    cl = type(test_name, (GetEmailParametricTemplate, TestCase), {"method": method, "socks": socks})
-        "method": method,
-        "socks": socks})
     setattr(thismodule, test_name, cl)

helpdesk/views/staff.py

@@ -406,12 +406,14 @@ def update_ticket(request, ticket_id, public=False):
     # comment.
     context = safe_template_context(ticket)
 
-    # this line sometimes creates problems if code is sent as a comment.
-    # if comment contains some django code, like "why does {% if bla %} crash",
-    # then the following line will give us a crash, since django expects {% if %}
-    # to be closed with an {% endif %} tag.
     from django.template import engines
     template_func = engines['django'].from_string
+    # this prevents system from trying to render any template tags
+    # broken into two stages to prevent changes from first replace being themselves
+    # changed by the second replace due to conflicting syntax
+    comment = comment.replace('{%', 'X-HELPDESK-COMMENT-VERBATIM').replace('%}', 'X-HELPDESK-COMMENT-ENDVERBATIM')
+    comment = comment.replace('X-HELPDESK-COMMENT-VERBATIM', '{% verbatim %}{%').replace('X-HELPDESK-COMMENT-ENDVERBATIM', '%}{% endverbatim %}')
+    # render the neutralized template
     comment = template_func(comment).render(context)
 
     if owner is -1 and ticket.assigned_to: