Merge lp:~allenap/launchpad/bug-mail-case-sensitive-domain-bug-48464-fix into lp:launchpad

[Gavin Panella]

[r=abentley][ui=none][bug=48464] Treat target domains on incoming mail without regard to case.

This makes MailHandler case-insensitive to domains, moves its doctests into unit tests, and updates the integration test, incomingmail.txt, to demonstrate case-insensitivity.

[Aaron Bentley (abentley)]

The code looks good.

Please add docs for all the tests. The copyright date for test_handlers should be 2009-2010, not just 2010.

review: Approve

=== modified file 'lib/lp/services/mail/handlers.py'
--- lib/lp/services/mail/handlers.py	2010-12-13 20:43:45 +0000
+++ lib/lp/services/mail/handlers.py	2010-12-13 20:43:46 +0000
@@ -2,6 +2,9 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
+__all__ = [
+    "mail_handlers",
+    ]
 
 from canonical.config import config
 from lp.answers.mail.handler import AnswerTrackerHandler
@@ -13,50 +16,31 @@
 class MailHandlers:
     """All the registered mail handlers."""
 
+    DEFAULT = (
+        (config.launchpad.bugs_domain, MaloneHandler),
+        (config.launchpad.specs_domain, SpecificationHandler),
+        (config.answertracker.email_domain, AnswerTrackerHandler),
+        # XXX flacoste 2007-04-23 Backward compatibility for old domain.
+        # We probably want to remove it in the future.
+        ('support.launchpad.net', AnswerTrackerHandler),
+        (config.launchpad.code_domain, CodeHandler),
+        )
+
     def __init__(self):
-        self._handlers = {
-            config.launchpad.bugs_domain: MaloneHandler(),
-            config.launchpad.specs_domain: SpecificationHandler(),
-            config.answertracker.email_domain: AnswerTrackerHandler(),
-            # XXX flacoste 2007-04-23 Backward compatibility for old domain.
-            # We probably want to remove it in the future.
-            'support.launchpad.net': AnswerTrackerHandler(),
-            config.launchpad.code_domain: CodeHandler(),
-            }
+        self._handlers = {}
+        for domain, handler_factory in self.DEFAULT:
+            self.add(domain, handler_factory())
 
     def get(self, domain):
         """Return the handler for the given email domain.
 
         Return None if no such handler exists.
-
-            >>> handlers = MailHandlers()
-            >>> handlers.get('bugs.launchpad.net') #doctest: +ELLIPSIS
-            <...MaloneHandler...>
-            >>> handlers.get('no.such.domain') is None
-            True
         """
-        return self._handlers.get(domain)
+        return self._handlers.get(domain.lower())
 
     def add(self, domain, handler):
-        """Adds a handler for a domain.
-
-            >>> handlers = MailHandlers()
-            >>> handlers.get('some.domain') is None
-            True
-            >>> handler = object()
-            >>> handlers.add('some.domain', handler)
-            >>> handlers.get('some.domain') is handler
-            True
-
-        If there already is a handler for the domain, the old one will
-        get overwritten:
-
-            >>> new_handler = object()
-            >>> handlers.add('some.domain', new_handler)
-            >>> handlers.get('some.domain') is new_handler
-            True
-        """
-        self._handlers[domain] = handler
+        """Adds a handler for a domain."""
+        self._handlers[domain.lower()] = handler
 
 
 mail_handlers = MailHandlers()

=== modified file 'lib/lp/services/mail/tests/incomingmail.txt'
--- lib/lp/services/mail/tests/incomingmail.txt	2010-12-13 20:43:45 +0000
+++ lib/lp/services/mail/tests/incomingmail.txt	2010-12-13 20:43:46 +0000
@@ -50,17 +50,19 @@
     >>> from canonical.testing.layers import LaunchpadZopelessLayer
     >>> from canonical.launchpad.mail import sendmail as original_sendmail
 
-For these examples, we don't want the Precedence header added.
+For these examples, we don't want the Precedence header added. Domains
+are treated without regard to case: for incoming mail, foo.com and
+FOO.COM are treated equivalently.
 
     >>> def sendmail(msg, to_addrs=None):
     ...     return original_sendmail(msg, to_addrs=to_addrs, bulk=False)
 
     >>> LaunchpadZopelessLayer.switchDbUser('launchpad')
     >>> msgids = {'foo.com': [], 'bar.com': [], 'baz.com': []}
-    >>> for domain in ('foo.com', 'bar.com', 'foo.com', 'baz.com'):
+    >>> for domain in ('foo.com', 'bar.com', 'FOO.COM', 'baz.com'):
     ...     msg = read_test_message('signed_detached.txt')
     ...     msg.replace_header('To', '123@%s' % domain)
-    ...     msgids[domain].append("<%s>" % sendmail(msg))
+    ...     msgids[domain.lower()].append("<%s>" % sendmail(msg))
 
 handleMail will check the timestamp on signed messages, but the signatures
 on our testdata are old, and in these tests we don't care to be told.

=== modified file 'lib/lp/services/mail/tests/test_handlers.py'
--- lib/lp/services/mail/tests/test_handlers.py	2010-12-13 20:43:45 +0000
+++ lib/lp/services/mail/tests/test_handlers.py	2010-12-13 20:43:46 +0000
@@ -1,10 +1,45 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
 
-from doctest import DocTestSuite
-
-
-def test_suite():
-    return DocTestSuite('lp.services.mail.handlers')
+from lp.services.mail.handlers import MailHandlers
+from lp.testing import TestCase
+
+
+class TestMailHandlers(TestCase):
+    """Tests for the `MailHandlers` class."""
+
+    def test_get(self):
+        # MailHandlers.get() should return the registered handler for the
+        # given domain.
+        handlers = MailHandlers()
+        self.assertIsNot(None, handlers.get("bugs.launchpad.net"))
+        self.assertIs(None, handlers.get("no.such.domain"))
+
+    def test_get_is_case_insensitive(self):
+        # The domain passed to get() is treated case-insentitively.
+        handlers = MailHandlers()
+        handler = object()
+        handlers.add("some.domain", handler)
+        self.assertIs(handler, handlers.get("some.domain"))
+        self.assertIs(handler, handlers.get("SOME.DOMAIN"))
+        self.assertIs(handler, handlers.get("Some.Domain"))
+
+    def test_add_for_new_domain(self):
+        # MailHandlers.add() registers a handler for the given domain.
+        handlers = MailHandlers()
+        self.assertIs(None, handlers.get("some.domain"))
+        handler = object()
+        handlers.add("some.domain", handler)
+        self.assertIs(handler, handlers.get("some.domain"))
+
+    def test_add_for_existing_domain(self):
+        # When adding a new handler for an already congfigured domain, the
+        # existing handler is overwritten.
+        handlers = MailHandlers()
+        handler1 = object()
+        handlers.add("some.domain", handler1)
+        handler2 = object()
+        handlers.add("some.domain", handler2)
+        self.assertIs(handler2, handlers.get("some.domain"))