Handle ignored emails explicitly using exceptions.

Support the flag on IgnoreEmail model to control deleting the email if
ignored.

helpdesk/email.py

@@ -20,7 +20,7 @@ import email
 from email.utils import getaddresses
 from email_reply_parser import EmailReplyParser
 from helpdesk import settings
-from helpdesk.exceptions import IgnoreTicketException
+from helpdesk.exceptions import IgnoreTicketException, DeleteIgnoredTicketException
 from helpdesk.lib import process_attachments, safe_template_context
 from helpdesk.models import FollowUp, IgnoreEmail, Queue, Ticket
 import imaplib
@@ -143,14 +143,18 @@ def pop3_sync(q, logger, server):
         except IgnoreTicketException:
             logger.warn(
                 "Message %s was ignored and will be left on POP3 server" % msgNum)
-
-        if ticket:
-            server.dele(msgNum)
-            logger.info(
-                "Successfully processed message %s, deleted from POP3 server" % msgNum)
-        else:
+        except DeleteIgnoredTicketException:
             logger.warn(
-                "Message %s was not successfully processed, and will be left on POP3 server" % msgNum)
+                "Message %s was ignored and deleted from POP3 server" % msgNum)
+            server.dele(msgNum)
+        else:
+            if ticket:
+                server.dele(msgNum)
+                logger.info(
+                    "Successfully processed message %s, deleted from POP3 server" % msgNum)
+            else:
+                logger.warn(
+                    "Message %s was not successfully processed, and will be left on POP3 server" % msgNum)
 
     server.quit()
 
@@ -195,16 +199,19 @@ def imap_sync(q, logger, server):
                     ticket = object_from_message(message=full_message, queue=q, logger=logger)
                 except IgnoreTicketException:
                     logger.warn("Message %s was ignored and will be left on IMAP server" % num)
-                    return
-                except TypeError as te:
-                    logger.warn(f"Unexpected error processing message: {te}")
-                    ticket = None  # hotfix. Need to work out WHY.
-                if ticket:
+                except DeleteIgnoredTicketException:
                     server.store(num, '+FLAGS', '\\Deleted')
-                    logger.info(
-                        "Successfully processed message %s, deleted from IMAP server" % num)
+                    logger.warn("Message %s was ignored and deleted from IMAP server" % num)
+                except TypeError as te:
+                    # Log the error with stacktrace to help identify what went wrong
+                    logger.error(f"Unexpected error processing message: {te}", exc_info=True)
                 else:
-                    logger.warn(
+                    if ticket:
+                        server.store(num, '+FLAGS', '\\Deleted')
+                        logger.info(
+                            "Successfully processed message %s, deleted from IMAP server" % num)
+                    else:
+                        logger.warn(
                         "Message %s was not successfully processed, and will be left on IMAP server" % num)
     except imaplib.IMAP4.error:
         logger.error(
@@ -295,21 +302,24 @@ def process_queue(q, logger):
                     ticket = object_from_message(message=full_message, queue=q, logger=logger)
                 except IgnoreTicketException:
                     logger.warn("Message %d was ignored and will be left in local directory", i)
-                    return
-            if ticket:
-                logger.info(
-                    "Successfully processed message %d, ticket/comment created.", i)
-                try:
-                    # delete message file if ticket was successful
+                except DeleteIgnoredTicketException:
                     os.unlink(m)
-                except OSError as e:
-                    logger.error(
-                        "Unable to delete message %d (%s).", i, str(e))
+                    logger.warn("Message %d was ignored and deleted local directory", i)
                 else:
-                    logger.info("Successfully deleted message %d.", i)
-            else:
-                logger.warn(
-                    "Message %d was not successfully processed, and will be left in local directory", i)
+                    if ticket:
+                        logger.info(
+                            "Successfully processed message %d, ticket/comment created.", i)
+                        try:
+                            # delete message file if ticket was successful
+                            os.unlink(m)
+                        except OSError as e:
+                            logger.error(
+                                "Unable to delete message %d (%s).", i, str(e))
+                        else:
+                            logger.info("Successfully deleted message %d.", i)
+                    else:
+                        logger.warn(
+                            "Message %d was not successfully processed, and will be left in local directory", i)
 
 
 def decodeUnknown(charset, string):
@@ -700,7 +710,7 @@ def object_from_message(message: str,
 
     for ignore in IgnoreEmail.objects.filter(Q(queues=queue) | Q(queues__isnull=True)):
         if ignore.test(sender_email):
-            raise IgnoreTicketException()
+            raise IgnoreTicketException() if ignore.keep_in_mailbox else DeleteIgnoredTicketException()
 
     ticket_id: typing.Optional[int] = get_ticket_id_from_subject_slug(
         queue.slug,

helpdesk/exceptions.py

@@ -2,4 +2,12 @@ class IgnoreTicketException(Exception):
     """
     Raised when an email message is received from a sender who is marked to be ignored
     """
+    pass
+
+
+class DeleteIgnoredTicketException(Exception):
+    """
+    Raised when an email message is received from a sender who is marked to be ignored
+    and the record is tagged to delete the email from the inbox
+    """
     pass

helpdesk/tests/test_get_email.py

@@ -7,7 +7,8 @@ from django.shortcuts import get_object_or_404
 from django.test import override_settings, TestCase
 import helpdesk.email
 from helpdesk.management.commands.get_email import Command
-from helpdesk.models import FollowUp, FollowUpAttachment, Queue, Ticket, TicketCC
+from helpdesk.models import FollowUp, FollowUpAttachment, Queue, Ticket, TicketCC,\
+    IgnoreEmail
 import itertools
 import logging
 import os
@@ -16,6 +17,9 @@ import six
 import sys
 from tempfile import mkdtemp
 from unittest import mock
+from helpdesk.tests import utils 
+from helpdesk.exceptions import DeleteIgnoredTicketException, IgnoreTicketException
+from helpdesk.email import object_from_message
 
 
 THIS_DIR = os.path.dirname(os.path.abspath(__file__))
@@ -42,7 +46,7 @@ class GetEmailCommonTests(TestCase):
             with mock.patch.object(Command, 'handle', return_value=None) as mocked_handle:
                 call_command('get_email', quiet=quiet_test_value)
                 mocked_handle.assert_called_once()
-                for args, kwargs in mocked_handle.call_args_list:
+                for _, kwargs in mocked_handle.call_args_list:
                     self.assertEqual(quiet_test_value, (kwargs['quiet']))
 
     def test_email_with_blank_body_and_attachment(self):
@@ -132,7 +136,29 @@ class GetEmailCommonTests(TestCase):
         assert "Hello there!" in FollowUp.objects.filter(
             ticket=ticket).first().comment
 
+    def test_will_delete_ignored_email(self):
+        """
+        Tests if an email will be ignored if configured to do so and throws the correct exception
+        to ensure the email is deleted
+        """
+        message, from_meta, _ = utils.generate_text_email(locale="es_ES")
+        ignore = IgnoreEmail(name="Test Ignore", email_address=from_meta[1], keep_in_mailbox=False)
+        ignore.save()
+        with self.assertRaises(DeleteIgnoredTicketException):
+            object_from_message(message.as_string(), self.queue_public, self.logger);
 
+    def test_will_not_delete_ignored_email(self):
+        """
+        Tests if an email will be ignored if configured to do so and throws the correct exception
+        to ensure the email is NOT deleted
+        """
+        message, from_meta, _ = utils.generate_text_email(locale="es_ES")
+        ignore = IgnoreEmail(name="Test Ignore", email_address=from_meta[1], keep_in_mailbox=True)
+        ignore.save()
+        with self.assertRaises(IgnoreTicketException):
+            object_from_message(message.as_string(), self.queue_public, self.logger);
+
+        
 class GetEmailParametricTemplate(object):
     """TestCase that checks basic email functionality across methods and socks configs."""
 
@@ -258,12 +284,12 @@ class GetEmailParametricTemplate(object):
         """Tests correctly decoding mail headers when a comma is encoded into
            UTF-8. See bug report #832."""
 
-        # example email text from Django docs:
-        # https://docs.djangoproject.com/en/1.10/ref/unicode/
-        test_email_from = "Bernard-Bouissières, Benjamin <bbb@example.com>"
+        # Create the from using standard RFC required formats
+        # Override the last_name to ensure we get a non-ascii character in it
+        test_email_from_meta = utils.generate_email_address("fr_FR", last_name_override="Bouissières")
         test_email_subject = "Commas in From lines"
         test_email_body = "Testing commas in from email UTF-8."
-        test_email = "To: helpdesk@example.com\nFrom: " + test_email_from + \
+        test_email = "To: helpdesk@example.com\nFrom: " + test_email_from_meta[0] + \
             "\nSubject: " + test_email_subject + "\n\n" + test_email_body
         test_mail_len = len(test_email)
 
@@ -333,13 +359,13 @@ class GetEmailParametricTemplate(object):
 
             ticket1 = get_object_or_404(Ticket, pk=1)
             self.assertEqual(ticket1.ticket_for_url, "QQ-%s" % ticket1.id)
-            self.assertEqual(ticket1.submitter_email, 'bbb@example.com')
+            self.assertEqual(ticket1.submitter_email, test_email_from_meta[1])
             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.submitter_email, 'bbb@example.com')
+            self.assertEqual(ticket2.submitter_email, test_email_from_meta[1])
             self.assertEqual(ticket2.title, test_email_subject)
             self.assertEqual(ticket2.description, test_email_body)
 

helpdesk/tests/utils.py

@@ -1,5 +1,6 @@
 """UItility functions facilitate making unit testing easier and less brittle."""
 
+import email
 import factory
 import faker
 import random
@@ -11,7 +12,8 @@ from email.message import Message
 from email.mime.text import MIMEText
 from numpy.random import randint
 from PIL import Image
-from typing import Tuple, Any
+from typing import Tuple, Any, Optional
+import typing
 
 
 def strip_accents(text):
@@ -48,7 +50,7 @@ def text_to_id(text):
 
 def get_random_string(length: int=16) -> str:
     return "".join(
-        [random.choice(string.ascii_letters + string.digits) for n in range(length)]
+        [random.choice(string.ascii_letters + string.digits) for _ in range(length)]
     )
 
 
@@ -91,32 +93,47 @@ def get_fake(provider: str, locale: str = "en_US", min_length: int = 5) -> Any:
     return factory.Faker(provider).evaluate({}, None, {'locale': locale,})
 
 
-def generate_email_address(locale: str="en_US") -> Tuple[str, str, str]:
-    """
-    Generate an email address making sure that the email address itself contains only ascii
-    """
+def generate_email_address(
+        locale: str="en_US",
+        use_short_email: bool=False,
+        real_name_format: Optional[str]="{last_name}, {first_name}",
+        last_name_override: Optional[str]=None) -> Tuple[str, str, str, str]:
+    '''
+    Generate an RFC 2822 email address
+    
+    :param locale: change this to generate locale specific names
+    :param use_short_email: defaults to false. If true then does not include real name in email address
+    :param real_name_format: pass a different format if different than "{last_name}, {first_name}"
+    :param last_name_override: override the fake name if you want some special characters in the last name
+    :returns <RFC2822 formatted email for header>, <short email address>, <first name>, <last_name
+    '''
     fake = faker.Faker(locale=locale)
     first_name = fake.first_name()
-    last_name = fake.last_name()
-    first_name.replace(' ', '').encode("ascii", "ignore").lower()
+    last_name = last_name_override or fake.last_name()
+    real_name = None if use_short_email else real_name_format.format(first_name=first_name, last_name=last_name)
+    # Add a random string to ensure we do not generate a real domain name
     email_address = "{}.{}@{}".format(
         first_name.replace(' ', '').encode("ascii", "ignore").lower().decode(),
         last_name.replace(' ', '').encode("ascii", "ignore").lower().decode(),
         get_random_string(5) + fake.domain_name()
     )
-    return email_address, first_name, last_name
+    # format email address for RFC 2822 and return
+    return email.utils.formataddr((real_name, email_address)), email_address, first_name, last_name
 
 
-def generate_email(locale: str="en_US", content_type: str="text/html", use_short_email: bool=False) -> Message:
+def generate_text_email(locale: str="en_US",
+        content_type: str="text/plain",
+        use_short_email: bool=False
+        ) -> typing.Tuple[Message, typing.Tuple[str, str], typing.Tuple[str, str]]:
     """
-    Generates an email includng headers
+    Generates an email including headers
     """
-    to_meta = generate_email_address(locale)
-    from_meta = generate_email_address(locale)
+    to_meta = generate_email_address(locale, use_short_email=use_short_email)
+    from_meta = generate_email_address(locale, use_short_email=use_short_email)
     body = get_fake("text", locale=locale)
     
     msg = MIMEText(body)
     msg['Subject'] = get_fake("sentence", locale=locale)
-    msg['From'] = from_meta[0] if use_short_email else "{} {}<{}>".format(from_meta[1], from_meta[2], from_meta[0])
-    msg['To'] = to_meta[0] if use_short_email else "{} {}<{}>".format(to_meta[1], to_meta[2], to_meta[0])
-    return msg.as_string()
+    msg['From'] = from_meta[0]
+    msg['To'] = to_meta[0]
+    return msg, from_meta, to_meta