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.

10 files changed, 255 insertions, 276 deletions

diff --git a/mail/src/leap/mail/adaptors/soledad.py b/mail/src/leap/mail/adaptors/soledad.py
index 46dbe4c..9f0bb30 100644
--- a/mail/src/leap/mail/adaptors/soledad.py
+++ b/mail/src/leap/mail/adaptors/soledad.py
@@ -19,7 +19,6 @@ Soledadad MailAdaptor module.
 import re
 from collections import defaultdict
 from email import message_from_string
-from functools import partial
 
 from pycryptopp.hash import sha256
 from twisted.internet import defer
@@ -72,6 +71,7 @@ class SoledadDocumentWrapper(models.DocumentWrapper):
     deletion.
     """
     # TODO we could also use a _dirty flag (in models)
+    # TODO add a get_count() method ??? -- that is extended over u1db.
 
     # We keep a dictionary with DeferredLocks, that will be
     # unique to every subclass of SoledadDocumentWrapper.
@@ -86,10 +86,9 @@ class SoledadDocumentWrapper(models.DocumentWrapper):
         """
         return cls._k_locks[cls.__name__]
 
-    def __init__(self, **kwargs):
-        doc_id = kwargs.pop('doc_id', None)
+    def __init__(self, doc_id=None, future_doc_id=None, **kwargs):
         self._doc_id = doc_id
-        self._future_doc_id = kwargs.pop('future_doc_id', None)
+        self._future_doc_id = future_doc_id
         self._lock = defer.DeferredLock()
         super(SoledadDocumentWrapper, self).__init__(**kwargs)
 
@@ -123,7 +122,7 @@ class SoledadDocumentWrapper(models.DocumentWrapper):
 
         def update_doc_id(doc):
             self._doc_id = doc.doc_id
-            self._future_doc_id = None
+            self.set_future_doc_id(None)
             return doc
 
         if self.future_doc_id is None:
@@ -201,6 +200,7 @@ class SoledadDocumentWrapper(models.DocumentWrapper):
 
     @classmethod
     def _get_or_create(cls, store, index, value):
+        # TODO shorten this method.
         assert store is not None
         assert index is not None
         assert value is not None
@@ -211,6 +211,7 @@ class SoledadDocumentWrapper(models.DocumentWrapper):
             except AttributeError:
                 raise RuntimeError("The model is badly defined")
 
+        # TODO separate into another method?
         def try_to_get_doc_from_index(indexes):
             values = []
             idx_def = dict(indexes)[index]
@@ -323,9 +324,6 @@ class SoledadDocumentWrapper(models.DocumentWrapper):
         d.addCallback(wrap_docs)
         return d
 
-    # TODO
-    # [ ] get_count() ???
-
     def __repr__(self):
         try:
             idx = getattr(self, self.model.__meta__.index)
@@ -442,29 +440,23 @@ class MessageWrapper(object):
         integers, beginning at one, and the values are dictionaries with the
         content of the content-docs.
         """
-        if isinstance(mdoc, SoledadDocument):
-            mdoc_id = mdoc.doc_id
-            mdoc = mdoc.content
-        else:
-            mdoc_id = None
-        if not mdoc:
-            mdoc = {}
-        self.mdoc = MetaMsgDocWrapper(doc_id=mdoc_id, **mdoc)
-
-        if isinstance(fdoc, SoledadDocument):
-            fdoc_id = fdoc.doc_id
-            fdoc = fdoc.content
-        else:
-            fdoc_id = None
-        self.fdoc = FlagsDocWrapper(doc_id=fdoc_id, **fdoc)
+
+        def get_doc_wrapper(doc, cls):
+            if isinstance(doc, SoledadDocument):
+                doc_id = doc.doc_id
+                doc = doc.content
+            else:
+                doc_id = None
+            if not doc:
+                doc = {}
+            return cls(doc_id=doc_id, **doc)
+
+        self.mdoc = get_doc_wrapper(mdoc, MetaMsgDocWrapper)
+
+        self.fdoc = get_doc_wrapper(fdoc, FlagsDocWrapper)
         self.fdoc.set_future_doc_id(self.mdoc.fdoc)
 
-        if isinstance(hdoc, SoledadDocument):
-            hdoc_id = hdoc.doc_id
-            hdoc = hdoc.content
-        else:
-            hdoc_id = None
-        self.hdoc = HeaderDocWrapper(doc_id=hdoc_id, **hdoc)
+        self.hdoc = get_doc_wrapper(hdoc, HeaderDocWrapper)
         self.hdoc.set_future_doc_id(self.mdoc.hdoc)
 
         if cdocs is None:
@@ -489,10 +481,6 @@ class MessageWrapper(object):
                     "Cannot create: fdoc has a doc_id")
 
         # TODO check that the doc_ids in the mdoc are coherent
-        # TODO I think we need to tolerate the no hdoc.doc_id case, for when we
-        # are doing a copy to another mailbox.
-        # leap_assert(self.hdoc.doc_id is None,
-        # "Cannot create: hdoc has a doc_id")
         d = []
         d.append(self.mdoc.create(store))
         d.append(self.fdoc.create(store))
@@ -566,8 +554,9 @@ class MessageWrapper(object):
 
     def get_subpart_dict(self, index):
         """
-        :param index: index, 1-indexed
+        :param index: the part to lookup, 1-indexed
         :type index: int
+        :rtype: dict
         """
         return self.hdoc.part_map[str(index)]
 
@@ -785,16 +774,6 @@ class SoledadMailAdaptor(SoledadIndexMixin):
         assert(MessageClass is not None)
         return MessageClass(MessageWrapper(mdoc, fdoc, hdoc, cdocs), uid=uid)
 
-    def _get_msg_from_variable_doc_list(self, doc_list, msg_class, uid=None):
-        if len(doc_list) == 3:
-            mdoc, fdoc, hdoc = doc_list
-            cdocs = None
-        elif len(doc_list) > 3:
-            fdoc, hdoc = doc_list[:3]
-            cdocs = dict(enumerate(doc_list[3:], 1))
-        return self.get_msg_from_docs(
-            msg_class, mdoc, fdoc, hdoc, cdocs, uid=uid)
-
     def get_msg_from_mdoc_id(self, MessageClass, store, mdoc_id,
                              uid=None, get_cdocs=False):
 
@@ -844,10 +823,21 @@ class SoledadMailAdaptor(SoledadIndexMixin):
         else:
             d = get_parts_doc_from_mdoc_id()
 
-        d.addCallback(partial(self._get_msg_from_variable_doc_list,
-                              msg_class=MessageClass, uid=uid))
+        d.addCallback(self._get_msg_from_variable_doc_list,
+                      msg_class=MessageClass, uid=uid)
         return d
 
+    def _get_msg_from_variable_doc_list(self, doc_list, msg_class, uid=None):
+        if len(doc_list) == 3:
+            mdoc, fdoc, hdoc = doc_list
+            cdocs = None
+        elif len(doc_list) > 3:
+            # XXX is this case used?
+            mdoc, fdoc, hdoc = doc_list[:3]
+            cdocs = dict(enumerate(doc_list[3:], 1))
+        return self.get_msg_from_docs(
+            msg_class, mdoc, fdoc, hdoc, cdocs, uid=uid)
+
     def get_flags_from_mdoc_id(self, store, mdoc_id):
         """
         # XXX stuff here...
@@ -875,7 +865,6 @@ class SoledadMailAdaptor(SoledadIndexMixin):
     def create_msg(self, store, msg):
         """
         :param store: an instance of soledad, or anything that behaves alike
-        :type store:
         :param msg: a Message object.
 
         :return: a Deferred that is fired when all the underlying documents
@@ -889,8 +878,6 @@ class SoledadMailAdaptor(SoledadIndexMixin):
         """
         :param msg: a Message object.
         :param store: an instance of soledad, or anything that behaves alike
-        :type store:
-        :param msg: a Message object.
         :return: a Deferred that is fired when all the underlying documents
                  have been updated (actually, it's only the fdoc that's allowed
                  to update).
diff --git a/mail/src/leap/mail/imap/account.py b/mail/src/leap/mail/imap/account.py
index 0cf583b..38df845 100644
--- a/mail/src/leap/mail/imap/account.py
+++ b/mail/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/mail/src/leap/mail/imap/mailbox.py b/mail/src/leap/mail/imap/mailbox.py
index 58fc514..52f4dd5 100644
--- a/mail/src/leap/mail/imap/mailbox.py
+++ b/mail/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/mail/src/leap/mail/imap/messages.py b/mail/src/leap/mail/imap/messages.py
index df50323..8f4c953 100644
--- a/mail/src/leap/mail/imap/messages.py
+++ b/mail/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/mail/src/leap/mail/imap/server.py b/mail/src/leap/mail/imap/server.py
index 23ddefc..027fd7a 100644
--- a/mail/src/leap/mail/imap/server.py
+++ b/mail/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/mail/src/leap/mail/imap/service/imap.py b/mail/src/leap/mail/imap/service/imap.py
index 93e4d62..cc76e3a 100644
--- a/mail/src/leap/mail/imap/service/imap.py
+++ b/mail/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/mail/src/leap/mail/imap/tests/test_imap.py b/mail/src/leap/mail/imap/tests/test_imap.py
index 6be41cd..67a24cd 100644
--- a/mail/src/leap/mail/imap/tests/test_imap.py
+++ b/mail/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.
diff --git a/mail/src/leap/mail/mail.py b/mail/src/leap/mail/mail.py
index 9b7a562..59fd57c 100644
--- a/mail/src/leap/mail/mail.py
+++ b/mail/src/leap/mail/mail.py
@@ -58,9 +58,16 @@ def _write_and_rewind(payload):
 
 class MessagePart(object):
 
-    # TODO pass cdocs in init
-
     def __init__(self, part_map, cdocs={}):
+        """
+        :param part_map: a dictionary mapping the subparts for
+                         this MessagePart (1-indexed).
+        :type part_map: dict
+        :param cdoc: optional, a dict of content documents
+        """
+        # TODO document the expected keys in the part_map dict.
+        # TODO add abstraction layer between the cdocs and this class. Only
+        # adaptor should know about the format of the cdocs.
         self._pmap = part_map
         self._cdocs = cdocs
 
@@ -266,6 +273,10 @@ class MessageCollection(object):
     # [ ] To guarantee synchronicity of the documents sent together during a
     #     sync, we could get hold of a deferredLock that inhibits
     #     synchronization while we are updating (think more about this!)
+    # [ ] review the serveral count_ methods. I think it's better to patch
+    #     server to accept deferreds.
+    # [ ] Use inheritance for the mailbox-collection instead of handling the
+    #     special cases everywhere?
 
     # Account should provide an adaptor instance when creating this collection.
     adaptor = None
@@ -285,9 +296,6 @@ class MessageCollection(object):
         self.mbox_indexer = mbox_indexer
         self.mbox_wrapper = mbox_wrapper
 
-        # TODO --- review this count shit. I think it's better to patch server
-        # to accept deferreds.
-
     def is_mailbox_collection(self):
         """
         Return True if this collection represents a Mailbox.
@@ -297,22 +305,26 @@ class MessageCollection(object):
 
     @property
     def mbox_name(self):
-        wrapper = getattr(self, "mbox_wrapper", None)
-        if not wrapper:
+        # TODO raise instead?
+        if self.mbox_wrapper is None:
             return None
-        return wrapper.mbox
+        return self.mbox_wrapper.mbox
 
     @property
     def mbox_uuid(self):
-        wrapper = getattr(self, "mbox_wrapper", None)
-        if not wrapper:
+        # TODO raise instead?
+        if self.mbox_wrapper is None:
             return None
-        return wrapper.uuid
+        return self.mbox_wrapper.uuid
 
     def get_mbox_attr(self, attr):
+        if self.mbox_wrapper is None:
+            raise RuntimeError("This is not a mailbox collection")
         return getattr(self.mbox_wrapper, attr)
 
     def set_mbox_attr(self, attr, value):
+        if self.mbox_wrapper is None:
+            raise RuntimeError("This is not a mailbox collection")
         setattr(self.mbox_wrapper, attr, value)
         return self.mbox_wrapper.update(self.store)
 
@@ -323,10 +335,10 @@ class MessageCollection(object):
         Retrieve a message by its content hash.
         :rtype: Deferred
         """
-
         if not self.is_mailbox_collection():
-            # instead of getting the metamsg by chash, query by (meta) index
-            # or use the internal collection of pointers-to-docs.
+            # TODO instead of getting the metamsg by chash, in this case we
+            # should query by (meta) index or use the internal collection of
+            # pointers-to-docs.
             raise NotImplementedError()
 
         metamsg_id = _get_mdoc_id(self.mbox_name, chash)
@@ -462,6 +474,9 @@ class MessageCollection(object):
             raise NotImplementedError()
 
         def insert_copied_mdoc_id(wrapper):
+            # TODO this needs to be implemented before the copy
+            # interface works.
+            newmailbox_uuid = get_mbox_uuid_from_msg_wrapper(wrapper)
             return self.mbox_indexer.insert_doc(
                 newmailbox_uuid, wrapper.mdoc.doc_id)
 
@@ -525,15 +540,6 @@ class MessageCollection(object):
         d.addCallback(del_all_uid)
         return d
 
-    def _update_flags_or_tags(self, old, new, mode):
-        if mode == Flagsmode.APPEND:
-            final = list((set(tuple(old) + new)))
-        elif mode == Flagsmode.REMOVE:
-            final = list(set(old).difference(set(new)))
-        elif mode == Flagsmode.SET:
-            final = new
-        return final
-
     def update_flags(self, msg, flags, mode):
         """
         Update flags for a given message.
@@ -563,6 +569,15 @@ class MessageCollection(object):
         d.addCallback(newtags)
         return d
 
+    def _update_flags_or_tags(self, old, new, mode):
+        if mode == Flagsmode.APPEND:
+            final = list((set(tuple(old) + new)))
+        elif mode == Flagsmode.REMOVE:
+            final = list(set(old).difference(set(new)))
+        elif mode == Flagsmode.SET:
+            final = new
+        return final
+
 
 class Account(object):
     """
@@ -573,7 +588,7 @@ class Account(object):
     basic collection handled by traditional MUAs, but it can also handle other
     types of Collections (tag based, for instance).
 
-    leap.mail.imap.SoledadBackedAccount partially proxies methods in this
+    leap.mail.imap.IMAPAccount partially proxies methods in this
     class.
     """
 
@@ -582,7 +597,6 @@ class Account(object):
     # the Account class.
 
     adaptor_class = SoledadMailAdaptor
-    store = None
 
     def __init__(self, store, ready_cb=None):
         self.store = store
@@ -612,6 +626,12 @@ class Account(object):
         return d
 
     def callWhenReady(self, cb, *args, **kw):
+        """
+        Execute the callback when the initialization of the Account is ready.
+        Note that the callback will receive a first meaningless parameter.
+        """
+        # TODO this should ignore the first parameter explicitely
+        # lambda _: cb(*args, **kw)
         self.deferred_initialization.addCallback(cb, *args, **kw)
         return self.deferred_initialization
 
diff --git a/mail/src/leap/mail/mailbox_indexer.py b/mail/src/leap/mail/mailbox_indexer.py
index 43a1f60..4eb0fa8 100644
--- a/mail/src/leap/mail/mailbox_indexer.py
+++ b/mail/src/leap/mail/mailbox_indexer.py
@@ -38,23 +38,23 @@ class WrongMetaDocIDError(Exception):
     pass
 
 
-def sanitize(mailbox_id):
-    return mailbox_id.replace("-", "_")
+def sanitize(mailbox_uuid):
+    return mailbox_uuid.replace("-", "_")
 
 
-def check_good_uuid(mailbox_id):
+def check_good_uuid(mailbox_uuid):
     """
     Check that the passed mailbox identifier is a valid UUID.
-    :param mailbox_id: the uuid to check
-    :type mailbox_id: str
+    :param mailbox_uuid: the uuid to check
+    :type mailbox_uuid: str
     :return: None
     :raises: AssertionError if a wrong uuid was passed.
     """
     try:
-        uuid.UUID(str(mailbox_id))
+        uuid.UUID(str(mailbox_uuid))
     except (AttributeError, ValueError):
         raise AssertionError(
-            "the mbox_id is not a valid uuid: %s" % mailbox_id)
+            "the mbox_id is not a valid uuid: %s" % mailbox_uuid)
 
 
 class MailboxIndexer(object):
@@ -88,51 +88,33 @@ class MailboxIndexer(object):
         assert self.store is not None
         return self.store.raw_sqlcipher_query(*args, **kw)
 
-    def create_table(self, mailbox_id):
+    def create_table(self, mailbox_uuid):
         """
         Create the UID table for a given mailbox.
         :param mailbox: the mailbox identifier.
         :type mailbox: str
         :rtype: Deferred
         """
-        check_good_uuid(mailbox_id)
+        check_good_uuid(mailbox_uuid)
         sql = ("CREATE TABLE if not exists {preffix}{name}( "
                "uid  INTEGER PRIMARY KEY AUTOINCREMENT, "
                "hash TEXT UNIQUE NOT NULL)".format(
-                   preffix=self.table_preffix, name=sanitize(mailbox_id)))
+                   preffix=self.table_preffix, name=sanitize(mailbox_uuid)))
         return self._query(sql)
 
-    def delete_table(self, mailbox_id):
+    def delete_table(self, mailbox_uuid):
         """
         Delete the UID table for a given mailbox.
         :param mailbox: the mailbox name
         :type mailbox: str
         :rtype: Deferred
         """
-        check_good_uuid(mailbox_id)
+        check_good_uuid(mailbox_uuid)
         sql = ("DROP TABLE if exists {preffix}{name}".format(
-            preffix=self.table_preffix, name=sanitize(mailbox_id)))
+            preffix=self.table_preffix, name=sanitize(mailbox_uuid)))
         return self._query(sql)
 
-    def rename_table(self, oldmailbox, newmailbox):
-        """
-        Delete the UID table for a given mailbox.
-        :param oldmailbox: the old mailbox name
-        :type oldmailbox: str
-        :param newmailbox: the new mailbox name
-        :type newmailbox: str
-        :rtype: Deferred
-        """
-        assert oldmailbox
-        assert newmailbox
-        assert oldmailbox != newmailbox
-        sql = ("ALTER TABLE {preffix}{old} "
-               "RENAME TO {preffix}{new}".format(
-                   preffix=self.table_preffix,
-                   old=sanitize(oldmailbox), new=sanitize(newmailbox)))
-        return self._query(sql)
-
-    def insert_doc(self, mailbox_id, doc_id):
+    def insert_doc(self, mailbox_uuid, doc_id):
         """
         Insert the doc_id for a MetaMsg in the UID table for a given mailbox.
 
@@ -148,11 +130,11 @@ class MailboxIndexer(object):
                  document.
         :rtype: Deferred
         """
-        check_good_uuid(mailbox_id)
+        check_good_uuid(mailbox_uuid)
         assert doc_id
-        mailbox_id = mailbox_id.replace('-', '_')
+        mailbox_uuid = mailbox_uuid.replace('-', '_')
 
-        if not re.findall(METAMSGID_RE.format(mbox_uuid=mailbox_id), doc_id):
+        if not re.findall(METAMSGID_RE.format(mbox_uuid=mailbox_uuid), doc_id):
             raise WrongMetaDocIDError("Wrong format for the MetaMsg doc_id")
 
         def get_rowid(result):
@@ -160,12 +142,12 @@ class MailboxIndexer(object):
 
         sql = ("INSERT INTO {preffix}{name} VALUES ("
                "NULL, ?)".format(
-                   preffix=self.table_preffix, name=sanitize(mailbox_id)))
+                   preffix=self.table_preffix, name=sanitize(mailbox_uuid)))
         values = (doc_id,)
 
         sql_last = ("SELECT MAX(rowid) FROM {preffix}{name} "
                     "LIMIT 1;").format(
-            preffix=self.table_preffix, name=sanitize(mailbox_id))
+            preffix=self.table_preffix, name=sanitize(mailbox_uuid))
 
         d = self._query(sql, values)
         d.addCallback(lambda _: self._query(sql_last))
@@ -173,25 +155,25 @@ class MailboxIndexer(object):
         d.addErrback(lambda f: f.printTraceback())
         return d
 
-    def delete_doc_by_uid(self, mailbox_id, uid):
+    def delete_doc_by_uid(self, mailbox_uuid, uid):
         """
         Delete the entry for a MetaMsg in the UID table for a given mailbox.
 
-        :param mailbox_id: the mailbox uuid
+        :param mailbox_uuid: the mailbox uuid
         :type mailbox: str
         :param uid: the UID of the message.
         :type uid: int
         :rtype: Deferred
         """
-        check_good_uuid(mailbox_id)
+        check_good_uuid(mailbox_uuid)
         assert uid
         sql = ("DELETE FROM {preffix}{name} "
                "WHERE uid=?".format(
-                   preffix=self.table_preffix, name=sanitize(mailbox_id)))
+                   preffix=self.table_preffix, name=sanitize(mailbox_uuid)))
         values = (uid,)
         return self._query(sql, values)
 
-    def delete_doc_by_hash(self, mailbox_id, doc_id):
+    def delete_doc_by_hash(self, mailbox_uuid, doc_id):
         """
         Delete the entry for a MetaMsg in the UID table for a given mailbox.
 
@@ -199,7 +181,7 @@ class MailboxIndexer(object):
 
             M-<mailbox_uuid>-<content-hash-of-the-message>
 
-        :param mailbox_id: the mailbox uuid
+        :param mailbox_uuid: the mailbox uuid
         :type mailbox: str
         :param doc_id: the doc_id for the MetaMsg
         :type doc_id: str
@@ -207,82 +189,80 @@ class MailboxIndexer(object):
                  document.
         :rtype: Deferred
         """
-        check_good_uuid(mailbox_id)
+        check_good_uuid(mailbox_uuid)
         assert doc_id
         sql = ("DELETE FROM {preffix}{name} "
                "WHERE hash=?".format(
-                   preffix=self.table_preffix, name=sanitize(mailbox_id)))
+                   preffix=self.table_preffix, name=sanitize(mailbox_uuid)))
         values = (doc_id,)
         return self._query(sql, values)
 
-    def get_doc_id_from_uid(self, mailbox_id, uid):
+    def get_doc_id_from_uid(self, mailbox_uuid, uid):
         """
         Get the doc_id for a MetaMsg in the UID table for a given mailbox.
 
-        :param mailbox_id: the mailbox uuid
+        :param mailbox_uuid: the mailbox uuid
         :type mailbox: str
         :param uid: the uid for the MetaMsg for this mailbox
         :type uid: int
         :rtype: Deferred
         """
-        check_good_uuid(mailbox_id)
-        mailbox_id = mailbox_id.replace('-', '_')
+        check_good_uuid(mailbox_uuid)
+        mailbox_uuid = mailbox_uuid.replace('-', '_')
 
         def get_hash(result):
             return _maybe_first_query_item(result)
 
         sql = ("SELECT hash from {preffix}{name} "
                "WHERE uid=?".format(
-                   preffix=self.table_preffix, name=sanitize(mailbox_id)))
+                   preffix=self.table_preffix, name=sanitize(mailbox_uuid)))
         values = (uid,)
         d = self._query(sql, values)
         d.addCallback(get_hash)
         return d
 
-    def get_uid_from_doc_id(self, mailbox_id, doc_id):
-        check_good_uuid(mailbox_id)
-        mailbox_id = mailbox_id.replace('-', '_')
+    def get_uid_from_doc_id(self, mailbox_uuid, doc_id):
+        check_good_uuid(mailbox_uuid)
+        mailbox_uuid = mailbox_uuid.replace('-', '_')
 
         def get_uid(result):
             return _maybe_first_query_item(result)
 
         sql = ("SELECT uid from {preffix}{name} "
                "WHERE hash=?".format(
-                   preffix=self.table_preffix, name=sanitize(mailbox_id)))
+                   preffix=self.table_preffix, name=sanitize(mailbox_uuid)))
         values = (doc_id,)
         d = self._query(sql, values)
         d.addCallback(get_uid)
         return d
 
-
-
-    def get_doc_ids_from_uids(self, mailbox_id, uids):
+    def get_doc_ids_from_uids(self, mailbox_uuid, uids):
         # For IMAP relative numbering /sequences.
         # XXX dereference the range (n,*)
         raise NotImplementedError()
 
-    def count(self, mailbox_id):
+    def count(self, mailbox_uuid):
         """
         Get the number of entries in the UID table for a given mailbox.
 
-        :param mailbox_id: the mailbox uuid
-        :type mailbox_id: str
+        :param mailbox_uuid: the mailbox uuid
+        :type mailbox_uuid: str
         :return: a deferred that will fire with an integer returning the count.
         :rtype: Deferred
         """
-        check_good_uuid(mailbox_id)
+        check_good_uuid(mailbox_uuid)
 
         def get_count(result):
             return _maybe_first_query_item(result)
 
         sql = ("SELECT Count(*) FROM {preffix}{name};".format(
-            preffix=self.table_preffix, name=sanitize(mailbox_id)))
+            preffix=self.table_preffix, name=sanitize(mailbox_uuid)))
         d = self._query(sql)
         d.addCallback(get_count)
         d.addErrback(lambda _: 0)
         return d
 
-    def get_next_uid(self, mailbox_id):
+    def get_next_uid(self, mailbox_uuid):
         """
         Get the next integer beyond the highest UID count for a given mailbox.
 
@@ -291,13 +271,13 @@ class MailboxIndexer(object):
         only thing that can be assured is that it will be equal or greater than
         the value returned.
 
-        :param mailbox_id: the mailbox uuid
+        :param mailbox_uuid: the mailbox uuid
         :type mailbox: str
         :return: a deferred that will fire with an integer returning the next
                  uid.
         :rtype: Deferred
         """
-        check_good_uuid(mailbox_id)
+        check_good_uuid(mailbox_uuid)
 
         def increment(result):
             uid = _maybe_first_query_item(result)
@@ -305,18 +285,18 @@ class MailboxIndexer(object):
                 return 1
             return uid + 1
 
-        d = self.get_last_uid(mailbox_id)
+        d = self.get_last_uid(mailbox_uuid)
         d.addCallback(increment)
         return d
 
-    def get_last_uid(self, mailbox_id):
+    def get_last_uid(self, mailbox_uuid):
         """
         Get the highest UID for a given mailbox.
         """
-        check_good_uuid(mailbox_id)
+        check_good_uuid(mailbox_uuid)
         sql = ("SELECT MAX(rowid) FROM {preffix}{name} "
                "LIMIT 1;").format(
-            preffix=self.table_preffix, name=sanitize(mailbox_id))
+            preffix=self.table_preffix, name=sanitize(mailbox_uuid))
 
         def getit(result):
             return _maybe_first_query_item(result)
@@ -325,17 +305,17 @@ class MailboxIndexer(object):
         d.addCallback(getit)
         return d
 
-    def all_uid_iter(self, mailbox_id):
+    def all_uid_iter(self, mailbox_uuid):
         """
         Get a sequence of all the uids in this mailbox.
 
-        :param mailbox_id: the mailbox uuid
-        :type mailbox_id: str
+        :param mailbox_uuid: the mailbox uuid
+        :type mailbox_uuid: str
         """
-        check_good_uuid(mailbox_id)
+        check_good_uuid(mailbox_uuid)
 
         sql = ("SELECT uid from {preffix}{name} ").format(
-            preffix=self.table_preffix, name=sanitize(mailbox_id))
+            preffix=self.table_preffix, name=sanitize(mailbox_uuid))
 
         def get_results(result):
             return [x[0] for x in result]
diff --git a/mail/src/leap/mail/walk.py b/mail/src/leap/mail/walk.py
index 8653a5f..891abdc 100644
--- a/mail/src/leap/mail/walk.py
+++ b/mail/src/leap/mail/walk.py
@@ -122,7 +122,7 @@ def walk_msg_tree(parts, body_phash=None):
     documents that will be stored in Soledad.
 
     It walks down the subparts in the parsed message tree, and collapses
-    the leaf docuents into a wrapper document until no multipart submessages
+    the leaf documents into a wrapper document until no multipart submessages
     are left. To achieve this, it iteratively calculates a wrapper vector of
     all documents in the sequence that have more than one part and have unitary
     documents to their right. To collapse a multipart, take as many
@@ -171,7 +171,7 @@ def walk_msg_tree(parts, body_phash=None):
             HEADERS: dict(parts[wind][HEADERS])
         }
 
-        # remove subparts and substitue wrapper
+        # remove subparts and substitute wrapper
         map(lambda i: parts.remove(i), slic)
         parts[wind] = cwra