fixes after review

* Some more docstring completion/fixes.
* Removed unneeded str coertion.
* Handle mailbox name in logs.
* Separate manhole boilerplate into its own file.

6 files changed, 146 insertions, 130 deletions

diff --git a/mail/src/leap/mail/imap/mailbox.py b/mail/src/leap/mail/imap/mailbox.py
index 40d3420c..c6825786 100644
--- a/mail/src/leap/mail/imap/mailbox.py
+++ b/mail/src/leap/mail/imap/mailbox.py
@@ -52,11 +52,6 @@ notifying clients of new messages. Use during stress tests.
 NOTIFY_NEW = not os.environ.get('LEAP_SKIPNOTIFY', False)
 
 
-class MessageCopyError(Exception):
-    """
-    """
-
-
 class SoledadMailbox(WithMsgFields, MBoxParser):
     """
     A Soledad-backed IMAP mailbox.
@@ -802,7 +797,7 @@ class SoledadMailbox(WithMsgFields, MBoxParser):
         # probably to FETCH...
         if message is None:
             log.msg("BUG: COPY found a None in passed message")
-            d.calback(None)
+            d.callback(None)
         deferLater(reactor, 0, self._do_copy, message, d)
         return d
 
@@ -849,7 +844,6 @@ class SoledadMailbox(WithMsgFields, MBoxParser):
             # errback. This actually rases an ugly warning
             # in some muas like thunderbird. I guess the user does
             # not deserve that.
-            #observer.errback(MessageCopyError("Already exists!"))
             observer.callback(True)
         else:
             mbox = self.mbox
diff --git a/mail/src/leap/mail/imap/memorystore.py b/mail/src/leap/mail/imap/memorystore.py
index 0632d1cc..195cef79 100644
--- a/mail/src/leap/mail/imap/memorystore.py
+++ b/mail/src/leap/mail/imap/memorystore.py
@@ -475,12 +475,13 @@ class MemoryStore(object):
 
     def get_last_uid(self, mbox):
         """
-        Get the highest UID for a given mbox.
+        Return the highest UID for a given mbox.
         It will be the highest between the highest uid in the message store for
         the mailbox, and the soledad integer cache.
 
         :param mbox: the mailbox
         :type mbox: str or unicode
+        :rtype: int
         """
         uids = self.get_uids(mbox)
         last_mem_uid = uids and max(uids) or 0
diff --git a/mail/src/leap/mail/imap/messages.py b/mail/src/leap/mail/imap/messages.py
index 6f822dbb..25fc55fe 100644
--- a/mail/src/leap/mail/imap/messages.py
+++ b/mail/src/leap/mail/imap/messages.py
@@ -328,7 +328,7 @@ class LeapMessage(fields, MailParser, MBoxParser):
         # We are still returning funky characters from here.
         else:
             logger.warning("No BDOC found for message.")
-            return write_fd(str(""))
+            return write_fd("")
 
     @memoized_method
     def _get_charset(self, stuff):
@@ -945,9 +945,7 @@ class MessageCollection(WithMsgFields, IndexedDB, MailParser, MBoxParser):
         hd = stringify_parts_map(hd)
 
         # The MessageContainer expects a dict, one-indexed
-        # XXX review-me
-        cdocs = dict(((key + 1, doc) for key, doc in
-                     enumerate(walk.get_raw_docs(msg, parts))))
+        cdocs = dict(enumerate(walk.get_raw_docs(msg, parts), 1))
 
         self.set_recent_flag(uid)
         msg_container = MessageWrapper(fd, hd, cdocs)
diff --git a/mail/src/leap/mail/imap/service/imap.py b/mail/src/leap/mail/imap/service/imap.py
index 8b95f75f..5487cfc8 100644
--- a/mail/src/leap/mail/imap/service/imap.py
+++ b/mail/src/leap/mail/imap/service/imap.py
@@ -66,6 +66,8 @@ except Exception:
 ######################################################
 
 DO_MANHOLE = os.environ.get("LEAP_MAIL_MANHOLE", None)
+if DO_MANHOLE:
+    from leap.mail.imap.service import manhole
 
 
 class IMAPAuthRealm(object):
@@ -121,118 +123,6 @@ class LeapIMAPFactory(ServerFactory):
         return imapProtocol
 
 
-MANHOLE_PORT = 2222
-
-
-def getManholeFactory(namespace, user, secret):
-    """
-    Get an administrative manhole into the application.
-
-    :param namespace: the namespace to show in the manhole
-    :type namespace: dict
-    :param user: the user to authenticate into the administrative shell.
-    :type user: str
-    :param secret: pass for this manhole
-    :type secret: str
-    """
-    import string
-
-    from twisted.cred.portal import Portal
-    from twisted.conch import manhole, manhole_ssh
-    from twisted.conch.insults import insults
-    from twisted.cred.checkers import (
-        InMemoryUsernamePasswordDatabaseDontUse as MemoryDB)
-
-    from rlcompleter import Completer
-
-    class EnhancedColoredManhole(manhole.ColoredManhole):
-        """
-        A Manhole with some primitive autocomplete support.
-        """
-        # TODO use introspection to make life easier
-
-        def find_common(self, l):
-            """
-            find common parts in thelist items
-            ex: 'ab' for ['abcd','abce','abf']
-            requires an ordered list
-            """
-            if len(l) == 1:
-                return l[0]
-
-            init = l[0]
-            for item in l[1:]:
-                for i, (x, y) in enumerate(zip(init, item)):
-                    if x != y:
-                        init = "".join(init[:i])
-                        break
-
-                if not init:
-                    return None
-            return init
-
-        def handle_TAB(self):
-            """
-            Trap the TAB keystroke
-            """
-            necessarypart = "".join(self.lineBuffer).split(' ')[-1]
-            completer = Completer(globals())
-            if completer.complete(necessarypart, 0):
-                matches = list(set(completer.matches))  # has multiples
-
-                if len(matches) == 1:
-                    length = len(necessarypart)
-                    self.lineBuffer = self.lineBuffer[:-length]
-                    self.lineBuffer.extend(matches[0])
-                    self.lineBufferIndex = len(self.lineBuffer)
-                else:
-                    matches.sort()
-                    commons = self.find_common(matches)
-                    if commons:
-                        length = len(necessarypart)
-                        self.lineBuffer = self.lineBuffer[:-length]
-                        self.lineBuffer.extend(commons)
-                        self.lineBufferIndex = len(self.lineBuffer)
-
-                    self.terminal.nextLine()
-                    while matches:
-                        matches, part = matches[4:], matches[:4]
-                        for item in part:
-                            self.terminal.write('%s' % item.ljust(30))
-                            self.terminal.write('\n')
-                            self.terminal.nextLine()
-
-                self.terminal.eraseLine()
-                self.terminal.cursorBackward(self.lineBufferIndex + 5)
-                self.terminal.write("%s %s" % (
-                    self.ps[self.pn], "".join(self.lineBuffer)))
-
-        def keystrokeReceived(self, keyID, modifier):
-            """
-            Act upon any keystroke received.
-            """
-            self.keyHandlers.update({'\b': self.handle_BACKSPACE})
-            m = self.keyHandlers.get(keyID)
-            if m is not None:
-                m()
-            elif keyID in string.printable:
-                self.characterReceived(keyID, False)
-
-    sshRealm = manhole_ssh.TerminalRealm()
-
-    def chainedProtocolFactory():
-        return insults.ServerProtocol(EnhancedColoredManhole, namespace)
-
-    sshRealm = manhole_ssh.TerminalRealm()
-    sshRealm.chainedProtocolFactory = chainedProtocolFactory
-
-    portal = Portal(
-        sshRealm, [MemoryDB(**{user: secret})])
-
-    f = manhole_ssh.ConchFactory(portal)
-    return f
-
-
 def run_service(*args, **kwargs):
     """
     Main entry point to run the service from the client.
@@ -281,12 +171,12 @@ def run_service(*args, **kwargs):
 
         if DO_MANHOLE:
             # TODO get pass from env var.too.
-            manhole_factory = getManholeFactory(
+            manhole_factory = manhole.getManholeFactory(
                 {'f': factory,
                  'a': factory.theAccount,
                  'gm': factory.theAccount.getMailbox},
                 "boss", "leap")
-            reactor.listenTCP(MANHOLE_PORT, manhole_factory,
+            reactor.listenTCP(manhole.MANHOLE_PORT, manhole_factory,
                               interface="127.0.0.1")
         logger.debug("IMAP4 Server is RUNNING in port  %s" % (port,))
         leap_events.signal(IMAP_SERVICE_STARTED, str(port))
diff --git a/mail/src/leap/mail/imap/service/manhole.py b/mail/src/leap/mail/imap/service/manhole.py
new file mode 100644
index 00000000..c83ae899
--- /dev/null
+++ b/mail/src/leap/mail/imap/service/manhole.py
@@ -0,0 +1,130 @@
+# -*- coding: utf-8 -*-
+# manhole.py
+# Copyright (C) 2014 LEAP
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+"""
+Utilities for enabling the manhole administrative interface into the
+LEAP Mail application.
+"""
+MANHOLE_PORT = 2222
+
+
+def getManholeFactory(namespace, user, secret):
+    """
+    Get an administrative manhole into the application.
+
+    :param namespace: the namespace to show in the manhole
+    :type namespace: dict
+    :param user: the user to authenticate into the administrative shell.
+    :type user: str
+    :param secret: pass for this manhole
+    :type secret: str
+    """
+    import string
+
+    from twisted.cred.portal import Portal
+    from twisted.conch import manhole, manhole_ssh
+    from twisted.conch.insults import insults
+    from twisted.cred.checkers import (
+        InMemoryUsernamePasswordDatabaseDontUse as MemoryDB)
+
+    from rlcompleter import Completer
+
+    class EnhancedColoredManhole(manhole.ColoredManhole):
+        """
+        A Manhole with some primitive autocomplete support.
+        """
+        # TODO use introspection to make life easier
+
+        def find_common(self, l):
+            """
+            find common parts in thelist items
+            ex: 'ab' for ['abcd','abce','abf']
+            requires an ordered list
+            """
+            if len(l) == 1:
+                return l[0]
+
+            init = l[0]
+            for item in l[1:]:
+                for i, (x, y) in enumerate(zip(init, item)):
+                    if x != y:
+                        init = "".join(init[:i])
+                        break
+
+                if not init:
+                    return None
+            return init
+
+        def handle_TAB(self):
+            """
+            Trap the TAB keystroke.
+            """
+            necessarypart = "".join(self.lineBuffer).split(' ')[-1]
+            completer = Completer(globals())
+            if completer.complete(necessarypart, 0):
+                matches = list(set(completer.matches))  # has multiples
+
+                if len(matches) == 1:
+                    length = len(necessarypart)
+                    self.lineBuffer = self.lineBuffer[:-length]
+                    self.lineBuffer.extend(matches[0])
+                    self.lineBufferIndex = len(self.lineBuffer)
+                else:
+                    matches.sort()
+                    commons = self.find_common(matches)
+                    if commons:
+                        length = len(necessarypart)
+                        self.lineBuffer = self.lineBuffer[:-length]
+                        self.lineBuffer.extend(commons)
+                        self.lineBufferIndex = len(self.lineBuffer)
+
+                    self.terminal.nextLine()
+                    while matches:
+                        matches, part = matches[4:], matches[:4]
+                        for item in part:
+                            self.terminal.write('%s' % item.ljust(30))
+                            self.terminal.write('\n')
+                            self.terminal.nextLine()
+
+                self.terminal.eraseLine()
+                self.terminal.cursorBackward(self.lineBufferIndex + 5)
+                self.terminal.write("%s %s" % (
+                    self.ps[self.pn], "".join(self.lineBuffer)))
+
+        def keystrokeReceived(self, keyID, modifier):
+            """
+            Act upon any keystroke received.
+            """
+            self.keyHandlers.update({'\b': self.handle_BACKSPACE})
+            m = self.keyHandlers.get(keyID)
+            if m is not None:
+                m()
+            elif keyID in string.printable:
+                self.characterReceived(keyID, False)
+
+    sshRealm = manhole_ssh.TerminalRealm()
+
+    def chainedProtocolFactory():
+        return insults.ServerProtocol(EnhancedColoredManhole, namespace)
+
+    sshRealm = manhole_ssh.TerminalRealm()
+    sshRealm.chainedProtocolFactory = chainedProtocolFactory
+
+    portal = Portal(
+        sshRealm, [MemoryDB(**{user: secret})])
+
+    f = manhole_ssh.ConchFactory(portal)
+    return f
diff --git a/mail/src/leap/mail/imap/soledadstore.py b/mail/src/leap/mail/imap/soledadstore.py
index 82f27e7e..8e22f26b 100644
--- a/mail/src/leap/mail/imap/soledadstore.py
+++ b/mail/src/leap/mail/imap/soledadstore.py
@@ -253,9 +253,11 @@ class SoledadStore(ContentDedup):
         """
         Consume each document wrapper in a separate thread.
 
-        :param doc_wrapper:
-        :type doc_wrapper:
-        :param deferred:
+        :param doc_wrapper: a MessageWrapper or RecentFlagsDoc instance
+        :type doc_wrapper: MessageWrapper or RecentFlagsDoc
+        :param deferred: a deferred that will be fired when the write operation
+                         has finished, either calling its callback or its
+                         errback depending on whether it succeed.
         :type deferred: Deferred
         """
         items = self._process(doc_wrapper)
@@ -415,6 +417,7 @@ class SoledadStore(ContentDedup):
         :param uid: the UID for the message
         :type uid: int
         """
+        result = None
         try:
             flag_docs = self._soledad.get_from_index(
                 fields.TYPE_MBOX_UID_IDX,
@@ -447,7 +450,7 @@ class SoledadStore(ContentDedup):
             mbox_doc = self._get_mbox_document(mbox)
             old_val = mbox_doc.content[key]
             if value < old_val:
-                logger.error("%s:%s Tried to write a UID lesser than what's "
+                logger.error("%r:%s Tried to write a UID lesser than what's "
                              "stored!" % (mbox, value))
             mbox_doc.content[key] = value
             self._soledad.put_doc(mbox_doc)