lots of little fixes after meskio's review

mostly having to do with poor, missing or outdated documentation,
naming of confusing things and reordering of code blocks for improved
readability.

6 files changed, 118 insertions, 126 deletions

diff --git a/src/leap/mail/imap/account.py b/src/leap/mail/imap/account.py
index 0cf583b..38df845 100644
--- a/src/leap/mail/imap/account.py
+++ b/src/leap/mail/imap/account.py
@@ -49,9 +49,6 @@ if PROFILE_CMD:
 # Soledad IMAP Account
 #######################################
 
-# XXX watchout, account needs to be ready... so we should maybe return
-# a deferred to the IMAP service when it's initialized
-
 class IMAPAccount(object):
     """
     An implementation of an imap4 Account
@@ -67,8 +64,14 @@ class IMAPAccount(object):
         """
         Keeps track of the mailboxes and subscriptions handled by this account.
 
-        :param account: The name of the account (user id).
-        :type account: str
+        The account is not ready to be used, since the store needs to be
+        initialized and we also need to do some initialization routines.
+        You can either pass a deferred to this constructor, or use
+        `callWhenReady` method.
+
+        :param user_id: The name of the account (user id, in the form
+                        user@provider).
+        :type user_id: str
 
         :param store: a Soledad instance.
         :type store: Soledad
@@ -87,11 +90,6 @@ class IMAPAccount(object):
         self.user_id = user_id
         self.account = Account(store, ready_cb=lambda: d.callback(self))
 
-    def _return_mailbox_from_collection(self, collection, readwrite=1):
-        if collection is None:
-            return None
-        mbox = IMAPMailbox(collection, rw=readwrite)
-        return mbox
     def end_session(self):
         """
         Used to mark when the session has closed, and we should not allow any
@@ -103,6 +101,12 @@ class IMAPAccount(object):
         self.session_ended = True
 
     def callWhenReady(self, cb, *args, **kw):
+        """
+        Execute callback when the account is ready to be used.
+        XXX note that this callback will be called with a first ignored
+        parameter.
+        """
+        # TODO ignore the first parameter and change tests accordingly.
         d = self.account.callWhenReady(cb, *args, **kw)
         return d
 
@@ -129,6 +133,12 @@ class IMAPAccount(object):
         d.addCallback(self._return_mailbox_from_collection)
         return d
 
+    def _return_mailbox_from_collection(self, collection, readwrite=1):
+        if collection is None:
+            return None
+        mbox = IMAPMailbox(collection, rw=readwrite)
+        return mbox
+
     #
     # IAccount
     #
@@ -146,7 +156,7 @@ class IMAPAccount(object):
         :type creation_ts: int
 
         :returns: a Deferred that will contain the document if successful.
-        :rtype: bool
+        :rtype: defer.Deferred
         """
         name = normalize_mailbox(name)
 
@@ -190,25 +200,15 @@ class IMAPAccount(object):
 
         :return:
             A deferred that will fire with a true value if the creation
-            succeeds.
+            succeeds. The deferred might fail with a MailboxException
+            if the mailbox cannot be added.
         :rtype: Deferred
 
-        :raise MailboxException: Raised if this mailbox cannot be added.
         """
-        paths = filter(None, normalize_mailbox(pathspec).split('/'))
-        subs = []
-        sep = '/'
-
         def pass_on_collision(failure):
             failure.trap(imap4.MailboxCollision)
             return True
 
-        for accum in range(1, len(paths)):
-            partial_path = sep.join(paths[:accum])
-            d = self.addMailbox(partial_path)
-            d.addErrback(pass_on_collision)
-            subs.append(d)
-
         def handle_collision(failure):
             failure.trap(imap4.MailboxCollision)
             if not pathspec.endswith('/'):
@@ -216,19 +216,26 @@ class IMAPAccount(object):
             else:
                 return defer.succeed(True)
 
+        def all_good(result):
+            return all(result)
+
+        paths = filter(None, normalize_mailbox(pathspec).split('/'))
+        subs = []
+        sep = '/'
+
+        for accum in range(1, len(paths)):
+            partial_path = sep.join(paths[:accum])
+            d = self.addMailbox(partial_path)
+            d.addErrback(pass_on_collision)
+            subs.append(d)
+
         df = self.addMailbox(sep.join(paths))
         df.addErrback(handle_collision)
         subs.append(df)
 
-        def all_good(result):
-            return all(result)
-
-        if subs:
-            d1 = defer.gatherResults(subs)
-            d1.addCallback(all_good)
-            return d1
-        else:
-            return defer.succeed(False)
+        d1 = defer.gatherResults(subs)
+        d1.addCallback(all_good)
+        return d1
 
     def select(self, name, readwrite=1):
         """
@@ -285,8 +292,7 @@ class IMAPAccount(object):
             global _mboxes
             _mboxes = mailboxes
             if name not in mailboxes:
-                err = imap4.MailboxException("No such mailbox: %r" % name)
-                return defer.fail(err)
+                raise imap4.MailboxException("No such mailbox: %r" % name)
 
         def get_mailbox(_):
             return self.getMailbox(name)
@@ -303,10 +309,9 @@ class IMAPAccount(object):
                 # as part of their root.
                 for others in _mboxes:
                     if others != name and others.startswith(name):
-                        err = imap4.MailboxException(
+                        raise imap4.MailboxException(
                             "Hierarchically inferior mailboxes "
                             "exist and \\Noselect is set")
-                        return defer.fail(err)
             return mbox
 
         d = self.account.list_all_mailbox_names()
@@ -324,8 +329,6 @@ class IMAPAccount(object):
         # if self._inferiorNames(name) > 1:
         #   self._index.removeMailbox(name)
 
-    # TODO use mail.Account.rename_mailbox
-    # TODO finish conversion to deferreds
     def rename(self, oldname, newname):
         """
         Renames a mailbox.
@@ -460,6 +463,9 @@ class IMAPAccount(object):
         :type name: str
         :rtype: Deferred
         """
+        # TODO should raise MailboxException if attempted to unsubscribe
+        # from a mailbox that is not currently subscribed.
+        # TODO factor out with subscribe method.
         name = normalize_mailbox(name)
 
         def set_unsubscribed(mbox):
diff --git a/src/leap/mail/imap/mailbox.py b/src/leap/mail/imap/mailbox.py
index 58fc514..52f4dd5 100644
--- a/src/leap/mail/imap/mailbox.py
+++ b/src/leap/mail/imap/mailbox.py
@@ -91,8 +91,9 @@ class IMAPMailbox(object):
     implements(
         imap4.IMailbox,
         imap4.IMailboxInfo,
-        #imap4.ICloseableMailbox,
         imap4.ISearchableMailbox,
+        # XXX I think we do not need to implement CloseableMailbox, do we?
+        # imap4.ICloseableMailbox
         imap4.IMessageCopier)
 
     init_flags = INIT_FLAGS
@@ -108,8 +109,6 @@ class IMAPMailbox(object):
 
     def __init__(self, collection, rw=1):
         """
-        SoledadMailbox constructor.
-
         :param collection: instance of IMAPMessageCollection
         :type collection: IMAPMessageCollection
 
@@ -117,14 +116,10 @@ class IMAPMailbox(object):
         :type rw: int
         """
         self.rw = rw
-        self.closed = False
 
         self._uidvalidity = None
         self.collection = collection
 
-        if not self.getFlags():
-            self.setFlags(self.init_flags)
-
     @property
     def mbox_name(self):
         return self.collection.mbox_name
@@ -201,6 +196,7 @@ class IMAPMailbox(object):
                     "flags expected to be a tuple")
         return self.collection.set_mbox_attr("flags", flags)
 
+    # TODO -  not used?
     @property
     def is_closed(self):
         """
@@ -211,6 +207,7 @@ class IMAPMailbox(object):
         """
         return self.collection.get_mbox_attr("closed")
 
+    # TODO -  not used?
     def set_closed(self, closed):
         """
         Set the closed attribute for this mailbox.
@@ -448,9 +445,6 @@ class IMAPMailbox(object):
         d.addCallback(remove_mbox)
         return d
 
-    def _close_cb(self, result):
-        self.closed = True
-
     def expunge(self):
         """
         Remove all messages flagged \\Deleted
@@ -555,24 +549,23 @@ class IMAPMailbox(object):
 
         # for sequence numbers (uid = 0)
         if is_sequence:
-            logger.debug("Getting msg by index: INEFFICIENT call!")
             # TODO --- implement sequences in mailbox indexer
             raise NotImplementedError
 
         else:
-            d = self._get_sequence_of_messages(messages_asked)
+            d = self._get_messages_range(messages_asked)
             d.addCallback(get_imap_messages_for_sequence)
 
         # TODO -- call signal_to_ui
         # d.addCallback(self.cb_signal_unread_to_ui)
         return d
 
-    def _get_sequence_of_messages(self, messages_asked):
-        def get_sequence(messages_asked):
+    def _get_messages_range(self, messages_asked):
+        def get_range(messages_asked):
             return self._filter_msg_seq(messages_asked)
 
         d = defer.maybeDeferred(self._bound_seq, messages_asked)
-        d.addCallback(get_sequence)
+        d.addCallback(get_range)
         return d
 
     def fetch_flags(self, messages_asked, uid):
@@ -599,6 +592,10 @@ class IMAPMailbox(object):
                 MessagePart.
         :rtype: tuple
         """
+        is_sequence = True if uid == 0 else False
+        if is_sequence:
+            raise NotImplementedError
+
         d = defer.Deferred()
         reactor.callLater(0, self._do_fetch_flags, messages_asked, uid, d)
         if PROFILE_CMD:
@@ -649,7 +646,7 @@ class IMAPMailbox(object):
             generator = (item for item in result)
             d.callback(generator)
 
-        d_seq = self._get_sequence_of_messages(messages_asked)
+        d_seq = self._get_messages_range(messages_asked)
         d_seq.addCallback(get_flags_for_seq)
         return d_seq
 
@@ -677,7 +674,11 @@ class IMAPMailbox(object):
                 MessagePart.
         :rtype: tuple
         """
+        # TODO implement sequences
         # TODO how often is thunderbird doing this?
+        is_sequence = True if uid == 0 else False
+        if is_sequence:
+            raise NotImplementedError
 
         class headersPart(object):
             def __init__(self, uid, headers):
@@ -753,6 +754,12 @@ class IMAPMailbox(object):
         :raise ReadOnlyMailbox: Raised if this mailbox is not open for
                                 read-write.
         """
+        # TODO implement sequences
+        # TODO how often is thunderbird doing this?
+        is_sequence = True if uid == 0 else False
+        if is_sequence:
+            raise NotImplementedError
+
         if not self.isWriteable():
             log.msg('read only mailbox!')
             raise imap4.ReadOnlyMailbox
@@ -777,7 +784,7 @@ class IMAPMailbox(object):
                          done.
         :type observer: deferred
         """
-        # XXX implement also sequence (uid = 0)
+        # TODO implement also sequence (uid = 0)
         # TODO we should prevent client from setting Recent flag
         leap_assert(not isinstance(flags, basestring),
                     "flags cannot be a string")
@@ -800,7 +807,7 @@ class IMAPMailbox(object):
             got_flags_setted.addCallback(return_result_dict)
             return got_flags_setted
 
-        d_seq = self._get_sequence_of_messages(messages_asked)
+        d_seq = self._get_messages_range(messages_asked)
         d_seq.addCallback(set_flags_for_seq)
         return d_seq
 
diff --git a/src/leap/mail/imap/messages.py b/src/leap/mail/imap/messages.py
index df50323..8f4c953 100644
--- a/src/leap/mail/imap/messages.py
+++ b/src/leap/mail/imap/messages.py
@@ -36,16 +36,38 @@ logger = logging.getLogger(__name__)
 
 class IMAPMessage(object):
     """
-    The main representation of a message.
+    The main representation of a message as seen by the IMAP Server.
+    This class implements the semantics specific to IMAP specification.
     """
-
     implements(imap4.IMessage)
 
     def __init__(self, message, prefetch_body=True,
                  store=None, d=defer.Deferred()):
         """
-        Initializes a LeapMessage.
+        Get an IMAPMessage. A mail.Message is needed, since many of the methods
+        are proxied to that object.
+
+
+        If you do not need to prefetch the body of the message, you can set
+        `prefetch_body` to False, but the current imap server implementation
+        expect the getBodyFile method to return inmediately.
+
+        When the prefetch_body option is used, a deferred is also expected as a
+        parameter, and this will fire when the deferred initialization has
+        taken place, with this instance of IMAPMessage as a parameter.
+
+        :param message: the abstract message
+        :type message: mail.Message
+        :param prefetch_body: Whether to prefetch the content doc for the body.
+        :type prefetch_body: bool
+        :param store: an instance of soledad, or anything that behaves like it.
+        :param d: an optional deferred, that will be fired with the instance of
+                  the IMAPMessage being initialized
+        :type d: defer.Deferred
         """
+        # TODO substitute the use of the deferred initialization by a factory
+        # function, maybe.
+
         self.message = message
         self.__body_fd = None
         self.store = store
diff --git a/src/leap/mail/imap/server.py b/src/leap/mail/imap/server.py
index 23ddefc..027fd7a 100644
--- a/src/leap/mail/imap/server.py
+++ b/src/leap/mail/imap/server.py
@@ -500,13 +500,18 @@ class LEAPIMAPServer(imap4.IMAP4Server):
     select_DELETE = auth_DELETE
 
     # Need to override the command table after patching
-    # arg_astring and arg_literal
+    # arg_astring and arg_literal, except on the methods that we are already
+    # overriding.
 
+    # TODO --------------------------------------------
+    # Check if we really need to override these
+    # methods, or we can monkeypatch.
     # do_DELETE = imap4.IMAP4Server.do_DELETE
     # do_CREATE = imap4.IMAP4Server.do_CREATE
     # do_RENAME = imap4.IMAP4Server.do_RENAME
     # do_SUBSCRIBE = imap4.IMAP4Server.do_SUBSCRIBE
     # do_UNSUBSCRIBE = imap4.IMAP4Server.do_UNSUBSCRIBE
+    # -------------------------------------------------
     do_LOGIN = imap4.IMAP4Server.do_LOGIN
     do_STATUS = imap4.IMAP4Server.do_STATUS
     do_APPEND = imap4.IMAP4Server.do_APPEND
@@ -530,8 +535,11 @@ class LEAPIMAPServer(imap4.IMAP4Server):
     auth_EXAMINE = (_selectWork, arg_astring, 0, 'EXAMINE')
     select_EXAMINE = auth_EXAMINE
 
+    # TODO -----------------------------------------------
+    # re-add if we stop overriding DELETE
     # auth_DELETE = (do_DELETE, arg_astring)
     # select_DELETE = auth_DELETE
+    # ----------------------------------------------------
 
     auth_RENAME = (do_RENAME, arg_astring, arg_astring)
     select_RENAME = auth_RENAME
diff --git a/src/leap/mail/imap/service/imap.py b/src/leap/mail/imap/service/imap.py
index 93e4d62..cc76e3a 100644
--- a/src/leap/mail/imap/service/imap.py
+++ b/src/leap/mail/imap/service/imap.py
@@ -17,6 +17,7 @@
 """
 IMAP service initialization
 """
+# TODO: leave only an implementor of IService in here
 import logging
 import os
 
@@ -29,10 +30,9 @@ from twisted.python import log
 logger = logging.getLogger(__name__)
 
 from leap.common import events as leap_events
-from leap.common.check import leap_assert, leap_assert_type, leap_check
+from leap.common.check import leap_assert_type, leap_check
 from leap.mail.imap.account import IMAPAccount
 from leap.mail.imap.server import LEAPIMAPServer
-from leap.mail.incoming import IncomingMail
 from leap.soledad.client import Soledad
 
 from leap.common.events.events_pb2 import IMAP_SERVICE_STARTED
@@ -113,6 +113,10 @@ class LeapIMAPFactory(ServerFactory):
         """
         Stops imap service (fetcher, factory and port).
         """
+        # mark account as unusable, so any imap command will fail
+        # with unauth state.
+        self.theAccount.end_session()
+
         # TODO should wait for all the pending deferreds,
         # the twisted way!
         if DO_PROFILE:
@@ -123,23 +127,23 @@ class LeapIMAPFactory(ServerFactory):
         return ServerFactory.doStop(self)
 
 
-def run_service(*args, **kwargs):
+def run_service(store, **kwargs):
     """
     Main entry point to run the service from the client.
 
+    :param store: a soledad instance
+
     :returns: the port as returned by the reactor when starts listening, and
               the factory for the protocol.
     """
-    leap_assert(len(args) == 2)
-    soledad = args
-    leap_assert_type(soledad, Soledad)
+    leap_assert_type(store, Soledad)
 
     port = kwargs.get('port', IMAP_PORT)
     userid = kwargs.get('userid', None)
     leap_check(userid is not None, "need an user id")
 
-    uuid = soledad.uuid
-    factory = LeapIMAPFactory(uuid, userid, soledad)
+    uuid = store.uuid
+    factory = LeapIMAPFactory(uuid, userid, store)
 
     try:
         tport = reactor.listenTCP(port, factory,
diff --git a/src/leap/mail/imap/tests/test_imap.py b/src/leap/mail/imap/tests/test_imap.py
index 6be41cd..67a24cd 100644
--- a/src/leap/mail/imap/tests/test_imap.py
+++ b/src/leap/mail/imap/tests/test_imap.py
@@ -75,6 +75,7 @@ class TestRealm:
 # TestCases
 #
 
+# TODO rename to IMAPMessageCollection
 class MessageCollectionTestCase(IMAP4HelperMixin):
     """
     Tests for the MessageCollection class
@@ -87,6 +88,7 @@ class MessageCollectionTestCase(IMAP4HelperMixin):
         We override mixin method since we are only testing
         MessageCollection interface in this particular TestCase
         """
+        # FIXME -- return deferred
         super(MessageCollectionTestCase, self).setUp()
 
         # FIXME --- update initialization
@@ -1090,64 +1092,6 @@ class LEAPIMAP4ServerTestCase(IMAP4HelperMixin):
 
         # Okay, that was much fun indeed
 
-    # skipping close test: we just need expunge for now.
-    #def testClose(self):
-        #"""
-        #Test closing the mailbox. We expect to get deleted all messages flagged
-        #as such.
-        #"""
-        #acc = self.server.theAccount
-        #mailbox_name = 'mailbox-close'
-#
-        #def add_mailbox():
-            #return acc.addMailbox(mailbox_name)
-#
-        #def login():
-            #return self.client.login(TEST_USER, TEST_PASSWD)
-#
-        #def select():
-            #return self.client.select(mailbox_name)
-#
-        #def get_mailbox():
-            #def _save_mbox(mailbox):
-                #self.mailbox = mailbox
-            #d = self.server.theAccount.getMailbox(mailbox_name)
-            #d.addCallback(_save_mbox)
-            #return d
-#
-        #def add_messages():
-            #d1 = self.mailbox.addMessage(
-                #'test 1', flags=('\\Deleted', 'AnotherFlag'))
-            #d2 = self.mailbox.addMessage(
-                #'test 2', flags=('AnotherFlag',))
-            #d3 = self.mailbox.addMessage(
-                #'test 3', flags=('\\Deleted',))
-            #d = defer.gatherResults([d1, d2, d3])
-            #return d
-#
-        #def close():
-            #return self.client.close()
-#
-        #d = self.connected.addCallback(strip(add_mailbox))
-        #d.addCallback(strip(login))
-        #d.addCallbacks(strip(select), self._ebGeneral)
-        #d.addCallback(strip(get_mailbox))
-        #d.addCallbacks(strip(add_messages), self._ebGeneral)
-        #d.addCallbacks(strip(close), self._ebGeneral)
-        #d.addCallbacks(self._cbStopClient, self._ebGeneral)
-        #d2 = self.loopback()
-        #d1 = defer.gatherResults([d, d2])
-        #d1.addCallback(lambda _: self.mailbox.getMessageCount())
-        #d1.addCallback(self._cbTestClose)
-        #return d1
-#
-    #def _cbTestClose(self, count):
-        # TODO is this correct? count should not take into account those
-        # flagged as deleted???
-        #self.assertEqual(count, 1)
-        # TODO --- assert flags are those of the message #2
-        #self.failUnless(self.mailbox.closed)
-
     def testExpunge(self):
         """
         Test expunge command
@@ -1209,6 +1153,7 @@ class LEAPIMAP4ServerTestCase(IMAP4HelperMixin):
         self.assertItemsEqual(self.results, [1, 3])
 
 
+# TODO -------- Fix this testcase
 class AccountTestCase(IMAP4HelperMixin):
     """
     Test the Account.