launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02794
[Merge] lp:~thumper/launchpad/blueprint-email-handler-bug-83414 into lp:launchpad
Tim Penhey has proposed merging lp:~thumper/launchpad/blueprint-email-handler-bug-83414 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#83414 Mail handler crashes if it receives a empty mail body
https://bugs.launchpad.net/bugs/83414
For more details, see:
https://code.launchpad.net/~thumper/launchpad/blueprint-email-handler-bug-83414/+merge/51488
This branch fixes a real old OOPS that we still get.
The blueprint mail handler didn't at all handle multipart email messages. When you call get_payload on a multipart message, you get an iterable of email parts back. This isn't what the method expected for a regex match, so an exception was raised.
I've added some unit tests around this area as there were none before.
--
https://code.launchpad.net/~thumper/launchpad/blueprint-email-handler-bug-83414/+merge/51488
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~thumper/launchpad/blueprint-email-handler-bug-83414 into lp:launchpad.
=== removed file 'lib/canonical/launchpad/mail/specexploder.py'
--- lib/canonical/launchpad/mail/specexploder.py 2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/mail/specexploder.py 1970-01-01 00:00:00 +0000
@@ -1,19 +0,0 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""Functions dealing with extracting information from spec notifications."""
-
-__metaclass__ = type
-
-import re
-
-
-_moin_url_re = re.compile(r'(https?://[^ \r\n]+)')
-
-def get_spec_url_from_moin_mail(moin_text):
- """Extract a specification URL from Moin change notification."""
- match = _moin_url_re.search(moin_text)
- if match:
- return match.group(1)
- else:
- return None
=== modified file 'lib/lp/blueprints/doc/spec-mail-exploder.txt'
--- lib/lp/blueprints/doc/spec-mail-exploder.txt 2010-12-22 20:46:21 +0000
+++ lib/lp/blueprints/doc/spec-mail-exploder.txt 2011-02-28 04:08:00 +0000
@@ -49,33 +49,12 @@
-----------------------------------------------------------------...
...
-So, we'll have to extract the specification URL from the message. To do
-this, we use get_spec_url_from_moin_mail():
-
- >>> from canonical.launchpad.mail.specexploder import (
- ... get_spec_url_from_moin_mail)
- >>> get_spec_url_from_moin_mail(moin_change.get_payload(decode=True))
- 'https://wiki.ubuntu.com/MediaIntegrityCheck'
-
-Even though it's name get_spec_url_from_moin_mail(), it doesn't really
-care if it's a moin notification or not, it simply extracts the first
-URL from the body.
-
- >>> get_spec_url_from_moin_mail(
- ... 'https://wiki.ubuntu.com/MediaIntegrityCheck')
- 'https://wiki.ubuntu.com/MediaIntegrityCheck'
-
-If no URL is found, None is returned.
-
- >>> get_spec_url_from_moin_mail('foo bar') is None
- True
-
The Mail Handler
----------------
The mail handler that handles mail on notifications@xxxxxxxxxxxxxxxxxxx
-is SpecificationHandler.
+is BlueprintHandler.
>>> from canonical.config import config
>>> from lp.services.mail.handlers import mail_handlers
=== modified file 'lib/lp/blueprints/mail/handler.py'
--- lib/lp/blueprints/mail/handler.py 2010-12-13 16:01:29 +0000
+++ lib/lp/blueprints/mail/handler.py 2011-02-28 04:08:00 +0000
@@ -5,7 +5,7 @@
__metaclass__ = type
__all__ = [
- "SpecificationHandler",
+ "BlueprintHandler",
]
import re
@@ -16,13 +16,27 @@
from canonical.config import config
from canonical.launchpad.interfaces.mail import IMailHandler
-from canonical.launchpad.mail.specexploder import get_spec_url_from_moin_mail
+from canonical.launchpad.mail.helpers import get_main_body
from canonical.launchpad.webapp import urlparse
from lp.blueprints.interfaces.specification import ISpecificationSet
from lp.services.mail.sendmail import sendmail
-class SpecificationHandler:
+MOIN_URL_RE = re.compile(r'(https?://[^ \r\n]+)')
+
+
+def get_spec_url_from_moin_mail(moin_text):
+ """Extract a specification URL from Moin change notification."""
+ if not isinstance(moin_text, basestring):
+ return None
+ match = MOIN_URL_RE.search(moin_text)
+ if match:
+ return match.group(1)
+ else:
+ return None
+
+
+class BlueprintHandler:
"""Handles emails sent to specs.launchpad.net."""
implements(IMailHandler)
@@ -56,6 +70,11 @@
else:
return getUtility(ISpecificationSet).getByURL(url)
+ def get_spec_url_from_email(self, signed_msg):
+ """Return the first url found in the email body."""
+ mail_body = get_main_body(signed_msg)
+ return get_spec_url_from_moin_mail(mail_body)
+
def process(self, signed_msg, to_addr, filealias=None, log=None):
"""See IMailHandler."""
match = self._spec_changes_address.match(to_addr)
@@ -82,8 +101,7 @@
# sender.
del signed_msg['Sender']
- mail_body = signed_msg.get_payload(decode=True)
- spec_url = get_spec_url_from_moin_mail(mail_body)
+ spec_url = self.get_spec_url_from_email(signed_msg)
if spec_url is not None:
if log is not None:
log.debug('Found a spec URL: %s' % spec_url)
=== added directory 'lib/lp/blueprints/mail/tests'
=== added file 'lib/lp/blueprints/mail/tests/__init__.py'
=== added file 'lib/lp/blueprints/mail/tests/test_handler.py'
--- lib/lp/blueprints/mail/tests/test_handler.py 1970-01-01 00:00:00 +0000
+++ lib/lp/blueprints/mail/tests/test_handler.py 2011-02-28 04:08:00 +0000
@@ -0,0 +1,80 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Testing the Blueprint email handler."""
+
+__metaclass__ = type
+
+from testtools.matchers import Equals, Is
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.blueprints.mail.handler import (
+ BlueprintHandler,
+ get_spec_url_from_moin_mail,
+ )
+from lp.testing import (
+ TestCase,
+ TestCaseWithFactory,
+ )
+
+
+class TestGetSpecUrlFromMoinMail(TestCase):
+ """Tests for get_spec_url_from_moin_mail."""
+
+ def test_invalid_params(self):
+ # Only strings and unicode are OK.
+ self.assertThat(get_spec_url_from_moin_mail(None), Is(None))
+ self.assertThat(get_spec_url_from_moin_mail(42), Is(None))
+ self.assertThat(get_spec_url_from_moin_mail(object()), Is(None))
+
+ def test_missing_urls(self):
+ # Strings with missing URLs also return None
+ self.assertThat(
+ get_spec_url_from_moin_mail('nothing here'),
+ Is(None))
+
+ def test_string_contains_url(self):
+ body = """
+ Testing a big string
+
+ An url, http://example.com/foo in a string
+ """
+ self.assertThat(
+ get_spec_url_from_moin_mail(body),
+ Equals('http://example.com/foo'))
+
+ def test_two_urls(self):
+ # Given two urls, only the first is returned.
+ body = """
+ Testing two urls:
+ http://example.com/first
+ http://example.com/second
+ """
+ self.assertThat(
+ get_spec_url_from_moin_mail(body),
+ Equals('http://example.com/first'))
+
+ def test_unicode(self):
+ # Given two urls, only the first is returned.
+ body = u"""
+ Testing unicode:
+ http://example.com/\N{SNOWMAN}
+ """
+ self.assertThat(
+ get_spec_url_from_moin_mail(body),
+ Equals(u'http://example.com/\N{SNOWMAN}'))
+
+
+class TestBlueprintEmailHandler(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def test_find_url_in_multipart_message(self):
+ """Multipart email is common, and we should be able to handle it."""
+ message = self.factory.makeSignedMessage(
+ body="An url http://example.com/foo in the body.",
+ attachment_contents="Nothing here either")
+ handler = BlueprintHandler()
+ self.assertThat(
+ handler.get_spec_url_from_email(message),
+ Equals('http://example.com/foo'))
=== modified file 'lib/lp/services/mail/handlers.py'
--- lib/lp/services/mail/handlers.py 2010-12-13 17:24:03 +0000
+++ lib/lp/services/mail/handlers.py 2011-02-28 04:08:00 +0000
@@ -8,7 +8,7 @@
from canonical.config import config
from lp.answers.mail.handler import AnswerTrackerHandler
-from lp.blueprints.mail.handler import SpecificationHandler
+from lp.blueprints.mail.handler import BlueprintHandler
from lp.bugs.mail.handler import MaloneHandler
from lp.code.mail.codehandler import CodeHandler
@@ -18,7 +18,7 @@
DEFAULT = (
(config.launchpad.bugs_domain, MaloneHandler),
- (config.launchpad.specs_domain, SpecificationHandler),
+ (config.launchpad.specs_domain, BlueprintHandler),
(config.answertracker.email_domain, AnswerTrackerHandler),
# XXX flacoste 2007-04-23 Backward compatibility for old domain.
# We probably want to remove it in the future.
