ifix(email): Create the log file only if required

+ update some translations
+ update some tests to correctly mock unlink as well
+ fix flake8 errors and sort the imports in email module
+ update some log messages to pass base message + parameters instead of rendered string (works better with Sentry)

helpdesk/email.py

@@ -4,42 +4,35 @@ Django Helpdesk - A Django powered ticket tracker for small enterprise.
 (c) Copyright 2008 Jutda. Copyright 2018 Timothy Hobbs. All Rights Reserved.
 See LICENSE for details.
 """
-from django.core.exceptions import ValidationError
+# import base64
-from django.core.files.base import ContentFile
-from django.core.files.uploadedfile import SimpleUploadedFile
-from django.core.management.base import BaseCommand
-from django.db.models import Q
-from django.utils.translation import ugettext as _
-from django.utils import encoding, timezone
-from django.contrib.auth import get_user_model
-
-from helpdesk import settings
-from helpdesk.lib import safe_template_context, process_attachments
-from helpdesk.models import Queue, Ticket, TicketCC, FollowUp, IgnoreEmail
-
-from datetime import timedelta
-import base64
-import binascii
 import email
-from email.header import decode_header
-from email.utils import getaddresses, parseaddr, collapse_rfc2231_value
 import imaplib
+import logging
 import mimetypes
-from os import listdir, unlink
+import os
-from os.path import isfile, join
 import poplib
 import re
 import socket
 import ssl
 import sys
+from datetime import timedelta
+from email.utils import getaddresses
+from os.path import isfile, join
 from time import ctime
-from optparse import make_option
 
 from bs4 import BeautifulSoup
-
+from django.contrib.auth import get_user_model
+from django.core.exceptions import ValidationError
+from django.core.files.uploadedfile import SimpleUploadedFile
+from django.db.models import Q
+from django.utils import encoding, timezone
+from django.utils.translation import ugettext as _
 from email_reply_parser import EmailReplyParser
 
-import logging
+from helpdesk import settings
+from helpdesk.lib import safe_template_context, process_attachments
+from helpdesk.models import Queue, Ticket, TicketCC, FollowUp, IgnoreEmail
+
 
 # import User model, which may be a custom model
 User = get_user_model()
@@ -70,31 +63,38 @@ def process_email(quiet=False):
         if q.logging_type in logging_types:
             logger.setLevel(logging_types[q.logging_type])
         elif not q.logging_type or q.logging_type == 'none':
-            logging.disable(logging.CRITICAL)  # disable all messages
+            # disable all handlers so messages go to nowhere
+            logger.handlers = []
+            logger.propagate = False
         if quiet:
             logger.propagate = False  # do not propagate to root logger that would log to console
-        logdir = q.logging_dir or '/var/log/helpdesk/'
+
+        # Log messages to specific file only if the queue has it configured
+        if (q.logging_type in logging_types) and q.logging_dir:  # if it's enabled and the dir is set
+            log_file_handler = logging.FileHandler(join(q.logging_dir, q.slug + '_get_email.log'))
+            logger.addHandler(log_file_handler)
+        else:
+            log_file_handler = None
 
         try:
-            handler = logging.FileHandler(join(logdir, q.slug + '_get_email.log'))
-            logger.addHandler(handler)
-
             if not q.email_box_last_check:
                 q.email_box_last_check = timezone.now() - timedelta(minutes=30)
 
             queue_time_delta = timedelta(minutes=q.email_box_interval or 0)
-
             if (q.email_box_last_check + queue_time_delta) < timezone.now():
                 process_queue(q, logger=logger)
                 q.email_box_last_check = timezone.now()
                 q.save()
         finally:
+            # we must close the file handler correctly if it's created
             try:
-                handler.close()
+                if log_file_handler:
+                    log_file_handler.close()
             except Exception as e:
                 logging.exception(e)
             try:
-                logger.removeHandler(handler)
+                if log_file_handler:
+                    logger.removeHandler(log_file_handler)
             except Exception as e:
                 logging.exception(e)
 
@@ -144,11 +144,17 @@ def imap_sync(q, logger, server):
                      settings.QUEUE_EMAIL_BOX_PASSWORD)
         server.select(q.email_box_imap_folder)
     except imaplib.IMAP4.abort:
-        logger.error("IMAP login failed. Check that the server is accessible and that the username and password are correct.")
+        logger.error(
+            "IMAP login failed. Check that the server is accessible and that "
+            "the username and password are correct."
+        )
         server.logout()
         sys.exit()
     except ssl.SSLError:
-        logger.error("IMAP login failed due to SSL error. This is often due to a timeout. Please check your connection and try again.")
+        logger.error(
+            "IMAP login failed due to SSL error. This is often due to a timeout. "
+            "Please check your connection and try again."
+        )
         server.logout()
         sys.exit()
 
@@ -171,7 +177,10 @@ def imap_sync(q, logger, server):
                 else:
                     logger.warn("Message %s was not successfully processed, and will be left on IMAP server" % num)
     except imaplib.IMAP4.error:
-        logger.error("IMAP retrieve failed. Is the folder '%s' spelled correctly, and does it exist on the server?" % q.email_box_imap_folder)
+        logger.error(
+            "IMAP retrieve failed. Is the folder '%s' spelled correctly, and does it exist on the server?",
+            q.email_box_imap_folder
+        )
 
     server.expunge()
     server.close()
@@ -243,7 +252,7 @@ def process_queue(q, logger):
 
     elif email_box_type == 'local':
         mail_dir = q.email_box_local_dir or '/var/lib/mail/helpdesk/'
-        mail = [join(mail_dir, f) for f in listdir(mail_dir) if isfile(join(mail_dir, f))]
+        mail = [join(mail_dir, f) for f in os.listdir(mail_dir) if isfile(join(mail_dir, f))]
         logger.info("Found %d messages in local mailbox directory" % len(mail))
 
         logger.info("Found %d messages in local mailbox directory" % len(mail))
@@ -253,15 +262,15 @@ def process_queue(q, logger):
                 full_message = encoding.force_text(f.read(), errors='replace')
                 ticket = object_from_message(message=full_message, queue=q, logger=logger)
             if ticket:
-                logger.info("Successfully processed message %d, ticket/comment created." % i)
+                logger.info("Successfully processed message %d, ticket/comment created.", i)
                 try:
-                    unlink(m)  # delete message file if ticket was successful
+                    os.unlink(m)  # delete message file if ticket was successful
-                except OSError:
+                except OSError as e:
-                    logger.error("Unable to delete message %d." % i)
+                    logger.error("Unable to delete message %d (%s).", i, str(e))
                 else:
-                    logger.info("Successfully deleted message %d." % i)
+                    logger.info("Successfully deleted message %d.", i)
             else:
-                logger.warn("Message %d was not successfully processed, and will be left in local directory" % i)
+                logger.warn("Message %d was not successfully processed, and will be left in local directory", i)
 
 
 def decodeUnknown(charset, string):
@@ -277,7 +286,11 @@ def decodeUnknown(charset, string):
 
 def decode_mail_headers(string):
     decoded = email.header.decode_header(string)
-    return u' '.join([str(msg, encoding=charset, errors='replace') if charset else str(msg) for msg, charset in decoded])
+    return u' '.join([
+        str(msg, encoding=charset, errors='replace') if charset else str(msg)
+        for msg, charset
+        in decoded
+    ])
 
 
 def create_ticket_cc(ticket, cc_list):
@@ -305,7 +318,7 @@ def create_ticket_cc(ticket, cc_list):
         try:
             ticket_cc = subscribe_to_ticket_updates(ticket=ticket, user=user, email=cced_email)
             new_ticket_ccs.append(ticket_cc)
-        except ValidationError as err:
+        except ValidationError:
             pass
 
     return new_ticket_ccs
@@ -362,7 +375,6 @@ def create_object_from_email_message(message, ticket_id, payload, files, logger)
             logger.debug("Created new ticket %s-%s" % (ticket.queue.slug, ticket.id))
 
             new = True
-            update = ''
 
     # Old issue being re-opened
     elif ticket.status == Ticket.CLOSED_STATUS:
@@ -389,7 +401,10 @@ def create_object_from_email_message(message, ticket_id, payload, files, logger)
 
     attached = process_attachments(f, files)
     for att_file in attached:
-        logger.info("Attachment '%s' (with size %s) successfully added to ticket from email." % (att_file[0], att_file[1].size))
+        logger.info(
+            "Attachment '%s' (with size %s) successfully added to ticket from email.",
+            att_file[0], att_file[1].size
+        )
 
     context = safe_template_context(ticket)
 
@@ -402,8 +417,8 @@ def create_object_from_email_message(message, ticket_id, payload, files, logger)
 
         ticket_cc_list = TicketCC.objects.filter(ticket=ticket).all().values_list('email', flat=True)
 
-        for email in ticket_cc_list:
+        for email_address in ticket_cc_list:
-            notifications_to_be_sent.append(email)
+            notifications_to_be_sent.append(email_address)
 
     # send mail to appropriate people now depending on what objects
     # were created and who was CC'd
@@ -453,8 +468,6 @@ def object_from_message(message, queue, logger):
     # correctly. Not ideal, but this seems to work for now.
     sender_email = email.utils.getaddresses(['\"' + sender.replace('<', '\" <')])[0][1]
 
-    body_plain, body_html = '', ''
-
     cc = message.get_all('cc', None)
     if cc:
         # first, fixup the encoding if necessary
@@ -530,18 +543,23 @@ def object_from_message(message, queue, logger):
             if not name:
                 ext = mimetypes.guess_extension(part.get_content_type())
                 name = "part-%i%s" % (counter, ext)
+
+            # 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
+            # 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)
+                # payloadToWrite = base64.decodebytes(payload)
             except non_b64_err:
                 logger.debug("Payload was not base64 encoded, using raw bytes")
-                payloadToWrite = payload
+                # payloadToWrite = payload
             files.append(SimpleUploadedFile(name, part.get_payload(decode=True), mimetypes.guess_type(name)[0]))
             logger.debug("Found MIME attachment %s" % name)
 

helpdesk/locale/cs/LC_MESSAGES/django.po

@@ -502,7 +502,7 @@ msgstr "Složka pro log soubory"
 #: third_party/django-helpdesk/helpdesk/models.py:306
 msgid ""
 "If logging is enabled, what directory should we use to store log files for "
-"this queue? If no directory is set, default to /var/log/helpdesk/"
+"this queue? The standard logging mechanims are used if no directory is set"
 msgstr ""
 
 #: third_party/django-helpdesk/helpdesk/models.py:317

helpdesk/locale/fr/LC_MESSAGES/django.po

@@ -522,11 +522,10 @@ msgstr "Dossier de logs"
 #: .\models.py:308
 msgid ""
 "If logging is enabled, what directory should we use to store log files for "
-"this queue? If no directory is set, default to /var/log/helpdesk/"
+"this queue? The standard logging mechanims are used if no directory is set"
 msgstr ""
 "Si les logs sont activés, quel dossier doit être utilisé pour stocker les "
-"fichiers de logs pour cette file ? Si aucun dossier n'est défini, cela sera /"
+"fichiers de logs pour cette file?"
-"var/log/helpdesk/ par défaut"
 
 #: .\models.py:319
 msgid "Default owner"

helpdesk/locale/ru/LC_MESSAGES/django.po

@@ -531,13 +531,15 @@ msgstr ""
 
 #: models.py:247
 msgid "Logging Directory"
-msgstr ""
+msgstr "Директория логов"
 
 #: models.py:251
 msgid ""
 "If logging is enabled, what directory should we use to store log files for "
-"this queue? If no directory is set, default to /var/log/helpdesk/"
+"this queue? The standard logging mechanims are used if no directory is set"
 msgstr ""
+"Директория в которую будут сохраняться файлы с логами; стандартная конфигурация "
+"используется если ничего не указано"
 
 #: models.py:261
 msgid "Default owner"

helpdesk/locale/zh_CN/LC_MESSAGES/django.po

@@ -470,7 +470,7 @@ msgstr ""
 #: models.py:308
 msgid ""
 "If logging is enabled, what directory should we use to store log files for "
-"this queue? If no directory is set, default to /var/log/helpdesk/"
+"this queue? The standard logging mechanims are used if no directory is set"
 msgstr ""
 
 #: models.py:319

helpdesk/migrations/0013_email_box_local_dir_and_logging.py

@@ -18,7 +18,7 @@ class Migration(migrations.Migration):
         migrations.AddField(
             model_name='queue',
             name='logging_dir',
-            field=models.CharField(blank=True, help_text='If logging is enabled, what directory should we use to store log files for this queue? If no directory is set, default to /var/log/helpdesk/', max_length=200, null=True, verbose_name='Logging Directory'),
+            field=models.CharField(blank=True, help_text='If logging is enabled, what directory should we use to store log files for this queue? The standard logging mechanims are used if no directory is set', max_length=200, null=True, verbose_name='Logging Directory'),
         ),
         migrations.AddField(
             model_name='queue',

helpdesk/models.py

@@ -307,7 +307,7 @@ class Queue(models.Model):
         null=True,
         help_text=_('If logging is enabled, what directory should we use to '
                     'store log files for this queue? '
-                    'If no directory is set, default to /var/log/helpdesk/'),
+                    'The standard logging mechanims are used if no directory is set'),
     )
 
     default_owner = models.ForeignKey(

helpdesk/tests/test_get_email.py

@@ -153,9 +153,10 @@ class GetEmailParametricTemplate(object):
         else:
             # Test local email reading
             if self.method == 'local':
-                with mock.patch('helpdesk.email.listdir') as mocked_listdir, \
+                with mock.patch('os.listdir') as mocked_listdir, \
                         mock.patch('helpdesk.email.isfile') as mocked_isfile, \
-                        mock.patch('builtins.open', mock.mock_open(read_data=test_email)):
+                        mock.patch('builtins.open', mock.mock_open(read_data=test_email)), \
+                        mock.patch('os.unlink'):
                     mocked_isfile.return_value = True
                     mocked_listdir.return_value = ['filename1', 'filename2']
 
@@ -224,9 +225,10 @@ class GetEmailParametricTemplate(object):
         else:
             # Test local email reading
             if self.method == 'local':
-                with mock.patch('helpdesk.email.listdir') as mocked_listdir, \
+                with mock.patch('os.listdir') as mocked_listdir, \
                         mock.patch('helpdesk.email.isfile') as mocked_isfile, \
-                        mock.patch('builtins.open' if six.PY3 else '__builtin__.open', mock.mock_open(read_data=test_email)):
+                        mock.patch('builtins.open' if six.PY3 else '__builtin__.open', mock.mock_open(read_data=test_email)), \
+                        mock.patch('os.unlink'):
                     mocked_isfile.return_value = True
                     mocked_listdir.return_value = ['filename1', 'filename2']
 
@@ -299,9 +301,10 @@ class GetEmailParametricTemplate(object):
         else:
             # Test local email reading
             if self.method == 'local':
-                with mock.patch('helpdesk.email.listdir') as mocked_listdir, \
+                with mock.patch('os.listdir') as mocked_listdir, \
                         mock.patch('helpdesk.email.isfile') as mocked_isfile, \
-                        mock.patch('builtins.open', mock.mock_open(read_data=test_email)):
+                        mock.patch('builtins.open', mock.mock_open(read_data=test_email)), \
+                        mock.patch('os.unlink'):
                     mocked_isfile.return_value = True
                     mocked_listdir.return_value = ['filename1', 'filename2']
 
@@ -412,9 +415,10 @@ class GetEmailParametricTemplate(object):
         else:
             # Test local email reading
             if self.method == 'local':
-                with mock.patch('helpdesk.email.listdir') as mocked_listdir, \
+                with mock.patch('os.listdir') as mocked_listdir, \
                         mock.patch('helpdesk.email.isfile') as mocked_isfile, \
-                        mock.patch('builtins.open', mock.mock_open(read_data=msg.as_string())):
+                        mock.patch('builtins.open', mock.mock_open(read_data=msg.as_string())), \
+                        mock.patch('os.unlink'):
                     mocked_isfile.return_value = True
                     mocked_listdir.return_value = ['filename1', 'filename2']
 
@@ -502,9 +506,10 @@ class GetEmailParametricTemplate(object):
         else:
             # Test local email reading
             if self.method == 'local':
-                with mock.patch('helpdesk.email.listdir') as mocked_listdir, \
+                with mock.patch('os.listdir') as mocked_listdir, \
                         mock.patch('helpdesk.email.isfile') as mocked_isfile, \
-                        mock.patch('builtins.open', mock.mock_open(read_data=test_email)):
+                        mock.patch('builtins.open', mock.mock_open(read_data=test_email)), \
+                        mock.patch('os.unlink'):
                     mocked_isfile.return_value = True
                     mocked_listdir.return_value = ['filename1']
 
@@ -670,9 +675,10 @@ class GetEmailCCHandling(TestCase):
         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.email.listdir') as mocked_listdir, \
+        with mock.patch('os.listdir') as mocked_listdir, \
                 mock.patch('helpdesk.email.isfile') as mocked_isfile, \
-                mock.patch('builtins.open', mock.mock_open(read_data=test_email)):
+                mock.patch('builtins.open', mock.mock_open(read_data=test_email)), \
+                mock.patch('os.unlink'):
             mocked_isfile.return_value = True
             mocked_listdir.return_value = ['filename1']