Hi Aaron,

It's a shame the error message isn't even more informative, but I
guess this is outside of your responsibility here, and this branch is
a great improvement over the current situation as it is.

One minor/trivial comment.

Gavin.


> === modified file 'lib/canonical/launchpad/mail/incoming.py'
> --- lib/canonical/launchpad/mail/incoming.py	2009-06-25 05:30:52 +0000
> +++ lib/canonical/launchpad/mail/incoming.py	2009-08-04 20:43:38 +0000
> @@ -177,6 +177,12 @@
>                  msg)
>          trans.commit()
>
> +    def _handle_user_error(error, mail):
> +        mailbox.delete(mail_id)
> +        send_process_error_notification(
> +            mail['From'], 'Submit Request Failure', str(error), mail)
> +        trans.commit()
> +
>      log = getLogger('process-mail')
>      mailbox = getUtility(IMailBox)
>      log.info("Opening the mail box.")
> @@ -250,10 +256,7 @@
>                  try:
>                      principal = authenticateEmail(mail)
>                  except InvalidSignature, error:
> -                    _handle_error(
> -                        "Invalid signature for %s:\n    %s" % (mail['From'],
> -                                                               str(error)),
> -                        file_alias_url)
> +                    _handle_user_error(error, mail)
>                      continue
>                  except InactiveAccount:
>                      _handle_error(
>
> === modified file 'lib/canonical/launchpad/mail/tests/test_incoming.py'
> --- lib/canonical/launchpad/mail/tests/test_incoming.py	2009-06-25 05:30:52 +0000
> +++ lib/canonical/launchpad/mail/tests/test_incoming.py	2009-08-04 20:43:38 +0000
> @@ -5,9 +5,55 @@
>
>  from zope.testing.doctest import DocTestSuite
>
> +from canonical.launchpad.mail.incoming import handleMail, MailErrorUtility
> +from canonical.testing import LaunchpadZopelessLayer
> +from lp.testing import TestCaseWithFactory
> +from lp.testing.mail_helpers import pop_notifications
> +from lp.services.mail.sendmail import MailController
> +
> +
> +class TestIncoming(TestCaseWithFactory):
> +
> +    layer = LaunchpadZopelessLayer
> +
> +    def test_invalid_signature(self):
> +        """Invalid signature should not be handled as an OOPs.
> +
> +        It should produce a message explaining to the user what went wrong.
> +        """
> +        person = self.factory.makePerson()
> +        email_address = person.preferredemail.email
> +        invalid_body = (
> +            '-----BEGIN PGP SIGNED MESSAGE-----\n'
> +            'Hash: SHA1\n\n'
> +            'Body\n'
> +            '-----BEGIN PGP SIGNATURE-----\n'
> +            'Not a signature.\n'
> +            '-----END PGP SIGNATURE-----\n')
> +        ctrl = MailController(
> +            email_address, '<email address hidden>', 'subject', invalid_body,
> +            bulk=False)
> +        ctrl.send()
> +        error_utility = MailErrorUtility()
> +        old_oops = error_utility.getLastOopsReport()
> +        handleMail()
> +        current_oops = error_utility.getLastOopsReport()
> +        if old_oops is None:
> +            self.assertIs(None, current_oops)
> +        else:
> +            self.assertEqual(old_oops.id, current_oops.id)
> +        (notification,) = pop_notifications()

IIRC, there was a long debate about this, the result of which was that
we should unpack using list notation:

       [notification] = pop_notifications()

> +        body = notification.get_payload()[0].get_payload(decode=True)
> +        self.assertIn(
> +            "An error occurred while processing a mail you sent to "
> +            "Launchpad's email\ninterface.\n\n\n"
> +            "Error message:\n\nSignature couldn't be verified: No data",
> +            body)
> +
>
>  def test_suite():
>      suite = unittest.TestSuite()
> +    suite.addTest(unittest.TestLoader().loadTestsFromName(__name__))
>      suite.addTest(DocTestSuite('canonical.launchpad.mail.incoming'))
>      return suite
>
>