Refactored the imap transport based on the tests

Testing the switch to uid-based IMAP calls required better testing
The better testing revealed some code that wasn't so pretty

I fixed the tests and the code

django_mailbox/tests/base.py

@@ -8,6 +8,16 @@ from django.test import TestCase
 from django_mailbox import models
 from django_mailbox.models import Mailbox, Message
 
+def get_email_as_text(name):
+    with open(
+        os.path.join(
+            os.path.dirname(__file__),
+            'messages',
+            name,
+        ),
+        'rb'
+    ) as f:
+        return f.read()
 
 class EmailMessageTestCase(TestCase):
     ALLOWED_EXTRA_HEADERS = [

django_mailbox/tests/test_transports.py

@@ -1,12 +1,42 @@
 import mock
-import six
 
-from django_mailbox.tests.base import EmailMessageTestCase
+from django.test.utils import override_settings
+
+from django_mailbox.tests.base import EmailMessageTestCase, get_email_as_text
 from django_mailbox.transports import ImapTransport, Pop3Transport
 
+FAKE_UID_SEARCH_ANSWER = ('OK', ['18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44'])
+FAKE_UID_FETCH_SIZES = ('OK', ['1 (UID 18 RFC822.SIZE 58070000000)', '2 (UID 19 RFC822.SIZE 2593)'])
+FAKE_UID_FETCH_MSG = ('OK', [('1 (UID 18 RFC822 {5807}', get_email_as_text('generic_message.eml') ),])
+FAKE_UID_COPY_MSG = ('OK', ['[COPYUID 1 2 2] (Success)'])
+FAKE_LIST_ARCHIVE_FOLDERS_ANSWERS = ('OK', ['(\\HasNoChildren \\All) "/" "[Gmail]/All Mail"'])
 
-class TestImapTransport(EmailMessageTestCase):
+class IMAPTestCase(EmailMessageTestCase):
     def setUp(self):
+        def imap_server_uid_method(*args):
+            cmd = args[0]
+            arg2 = args[2]
+            if cmd == 'search':
+                return FAKE_UID_SEARCH_ANSWER
+            if cmd == 'copy':
+                return FAKE_UID_COPY_MSG
+            if cmd == 'fetch':
+                if arg2 == '(RFC822.SIZE)':
+                    return FAKE_UID_FETCH_SIZES
+                if arg2 == '(RFC822)':
+                    return FAKE_UID_FETCH_MSG
+        def imap_server_list_method(pattern=None):
+            return FAKE_LIST_ARCHIVE_FOLDERS_ANSWERS
+
+        self.imap_server = mock.Mock()
+        self.imap_server.uid = imap_server_uid_method
+        self.imap_server.list = imap_server_list_method
+        super(IMAPTestCase, self).setUp()
+
+
+class TestImapTransport(IMAPTestCase):
+    def setUp(self):
+        super(TestImapTransport, self).setUp()
         self.arbitrary_hostname = 'one.two.three'
         self.arbitrary_port = 100
         self.ssl = False
@@ -15,42 +45,18 @@ class TestImapTransport(EmailMessageTestCase):
             self.arbitrary_port,
             self.ssl
         )
-        self.transport.server = None
+        self.transport.server = self.imap_server
-        super(TestImapTransport, self).setUp()
 
     def test_get_email_message(self):
-        with mock.patch.object(self.transport, 'server') as server:
-            server.search.return_value = (
-                'OK',
-                [
-                    'One',  # This is totally arbitrary
-                ]
-            )
-            server.fetch.return_value = (
-                'OK',
-                [
-                    [
-                        '1 (RFC822 {8190}',  # Wat?
-                        self._get_email_as_text('generic_message.eml')
-                    ],
-                    ')',
-                ]
-            )
         actual_messages = list(self.transport.get_message())
-
+        self.assertEqual(len(actual_messages), 27)
-        self.assertEqual(len(actual_messages), 1)
-
         actual_message = actual_messages[0]
         expected_message = self._get_email_object('generic_message.eml')
-
         self.assertEqual(expected_message, actual_message)
 
-
+class TestImapArchivedTransport(TestImapTransport):
-class TestImapArchivedTransport(EmailMessageTestCase):
     def setUp(self):
-        self.arbitrary_hostname = 'one.two.three'
+        super(TestImapArchivedTransport, self).setUp()
-        self.arbitrary_port = 100
-        self.ssl = False
         self.archive = 'Archive'
         self.transport = ImapTransport(
             self.arbitrary_hostname,
@@ -58,50 +64,34 @@ class TestImapArchivedTransport(EmailMessageTestCase):
             self.ssl,
             self.archive
         )
-        self.transport.server = None
+        self.transport.server = self.imap_server
-        super(TestImapArchivedTransport, self).setUp()
+
+class TestMaxSizeImapTransport(TestImapTransport):
+
+    @override_settings(DJANGO_MAILBOX_MAX_MESSAGE_SIZE=5807)
+    def setUp(self):
+        super(TestMaxSizeImapTransport, self).setUp()
+
+        self.transport = ImapTransport(
+            self.arbitrary_hostname,
+            self.arbitrary_port,
+            self.ssl,
+        )
+        self.transport.server = self.imap_server
+
+    def test_size_limit(self):
+        all_message_ids = self.transport._get_all_message_ids()
+        small_message_ids = self.transport._get_small_message_ids(all_message_ids)
+        self.assertEqual(len(small_message_ids), 1)
 
     def test_get_email_message(self):
-        with mock.patch.object(self.transport, 'server') as server:
-            server.search.return_value = (
-                'OK',
-                [
-                    'One',  # This is totally arbitrary
-                ]
-            )
-            server.fetch.return_value = (
-                'OK',
-                [
-                    [
-                        '1 (RFC822 {8190}',  # Wat?
-                        self._get_email_as_text('generic_message.eml')
-                    ],
-                    ')',
-                ]
-            )
-            server.list.return_value = (
-                'OK',
-                [
-                    '(\\HasNoChildren) "/" "Archive"'
-                ]
-            )
-            server.copy.return_value = (
-                'OK',
-                [
-                    '[COPYUID 1 2 2] (Success)'
-                ]
-            )
         actual_messages = list(self.transport.get_message())
-
         self.assertEqual(len(actual_messages), 1)
-
         actual_message = actual_messages[0]
         expected_message = self._get_email_object('generic_message.eml')
-
         self.assertEqual(expected_message, actual_message)
 
 
-
 class TestPop3Transport(EmailMessageTestCase):
     def setUp(self):
         self.arbitrary_hostname = 'one.two.three'
@@ -139,3 +129,4 @@ class TestPop3Transport(EmailMessageTestCase):
         expected_message = self._get_email_object('generic_message.eml')
 
         self.assertEqual(expected_message, actual_message)
+

django_mailbox/transports/imap.py

@@ -1,17 +1,21 @@
+import logging
+
+logger = logging.getLogger(__name__)
+
 from imaplib import IMAP4, IMAP4_SSL
 
 from .base import EmailTransport, MessageParseError
 
 from django.conf import settings
 
-MAX_MESSAGE_SIZE = getattr(
-    settings,
-    'DJANGO_MAILBOX_MAX_MESSAGE_SIZE',
-    False
-)
 
 class ImapTransport(EmailTransport):
     def __init__(self, hostname, port=None, ssl=False, archive=''):
+        MAX_MESSAGE_SIZE = getattr(
+            settings,
+            'DJANGO_MAILBOX_MAX_MESSAGE_SIZE',
+            False
+        )
         self.hostname = hostname
         self.port = port
         self.archive = archive
@@ -30,10 +34,9 @@ class ImapTransport(EmailTransport):
         typ, msg = self.server.login(username, password)
         self.server.select()
 
-
     def _get_all_message_ids(self):
         # Fetch all the message uids
-        response, message_ids = self.server.uid('search', None, 'ALL', )
+        response, message_ids = self.server.uid('search', None, 'ALL')
         return message_ids[0].split(' ')
 
     def _get_small_message_ids(self, message_ids):
@@ -45,20 +48,17 @@ class ImapTransport(EmailTransport):
         status, data = self.server.uid(
             'fetch',
             ','.join(message_ids),
-            '(BODY.PEEK[HEADER] RFC822.SIZE BODYSTRUCTURE)'
+            '(RFC822.SIZE)'
         )
 
         for each_msg in data:
-            if isinstance(each_msg, tuple):
             try:
-                    metadata, structure = each_msg[0].split(' BODYSTRUCTURE ')
+                uid = each_msg.split(' ')[2]
-                    uid = metadata.split('(')[1].split(' ')[1]
+                size = each_msg.split(' ')[4].rstrip(')')
-                    size = metadata.split('(')[1].split(' ')[3]
                 if int(size) <= int(self.MAX_MSG_SIZE):
                     safe_message_ids.append(uid)
             except ValueError, e:
-                    print "ValueError: %s working on %s" % (e, each_msg[0])
+                logger.warning("ValueError: %s working on %s" % (e, each_msg[0]))
-                    print each_msg
                 pass
         return safe_message_ids
 
@@ -69,7 +69,6 @@ class ImapTransport(EmailTransport):
         if self.MAX_MSG_SIZE:
             message_ids = self._get_small_message_ids(message_ids)
 
-
         if not message_ids:
             return