From 572ffd5acf09bd664c26ab868e63fc0abf0e5971 Mon Sep 17 00:00:00 2001 From: chrisbroderick Date: Tue, 6 Sep 2022 19:40:35 +0100 Subject: [PATCH] Handle ignored emails explicitly using exceptions. Support the flag on IgnoreEmail model to control deleting the email if ignored. --- helpdesk/email.py | 70 ++++++++++++++++++-------------- helpdesk/exceptions.py | 8 ++++ helpdesk/tests/test_get_email.py | 42 +++++++++++++++---- helpdesk/tests/utils.py | 49 ++++++++++++++-------- 4 files changed, 115 insertions(+), 54 deletions(-) diff --git a/helpdesk/email.py b/helpdesk/email.py index 7a4962e2..7c83f7db 100644 --- a/helpdesk/email.py +++ b/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, diff --git a/helpdesk/exceptions.py b/helpdesk/exceptions.py index de35b9c4..d87f74fd 100644 --- a/helpdesk/exceptions.py +++ b/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 \ No newline at end of file diff --git a/helpdesk/tests/test_get_email.py b/helpdesk/tests/test_get_email.py index 89afa95d..df59fe80 100644 --- a/helpdesk/tests/test_get_email.py +++ b/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 " + # 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) diff --git a/helpdesk/tests/utils.py b/helpdesk/tests/utils.py index 2e36f04d..2cd4dd50 100644 --- a/helpdesk/tests/utils.py +++ b/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 , , , 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