Merge lp:~abentley/launchpad/forbidden-oops into lp:launchpad

[Aaron Bentley]

Mail instead of oops when branch creation forbidden

[Aaron Bentley (abentley)]

= Summary =
This fixes bug #492275, "OOPS when submitting merge request for a project that
I don't have access to"

== Proposed fix ==
Catch BranchCreation exceptions and send email to the sender.

== Pre-implementation notes ==
None

== Implementation details ==
The actual exception was using %r, which was causing the name to be shown as
a unicode literal, e.g. u'~person/product'. End users shouldn't see unicode
literals, so I changed it to "%s".

Also, I sorted the imports.

== Tests ==
bin/test test_codehandler -t test_forbidden_target

== Demo and Q/A ==
Use bzr send to request a merge into a project that does not allow you to
create branches in it. If you get back an error, it works. If you get back
nothing or an OOPs, it's broken.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/code/mail/codehandler.py
  lib/lp/code/mail/tests/test_codehandler.py
  lib/canonical/launchpad/mail/errortemplates/branch-creation-exception.txt
  lib/lp/code/model/branchnamespace.py

== Pylint notices ==

lib/lp/code/mail/codehandler.py
    37: [F0401] Unable to import 'lazr.uri' (No module named uri)

lib/lp/code/model/branchnamespace.py
    20: [F0401] Unable to import 'lazr.lifecycle.event' (No module named lifecycle)

[Edwin Grubbs (edwin-grubbs)]

Hi Aaron,

This is a nice branch. I just have two formatting suggestions.

merge-approved

-Edwin

>=== modified file 'lib/lp/code/mail/tests/test_codehandler.py'
>--- lib/lp/code/mail/tests/test_codehandler.py 2010-02-01 04:15:51 +0000
>+++ lib/lp/code/mail/tests/test_codehandler.py 2010-02-10 16:04:07 +0000
>@@ -22,33 +22,35 @@
> from zope.security.proxy import removeSecurityProxy
>
> from canonical.config import config
>+from canonical.launchpad.database import MessageSet
>+from canonical.launchpad.interfaces.mail import (
>+ EmailProcessingError, IWeaklyAuthenticatedPrincipal)
>+from canonical.launchpad.mail.handlers import mail_handlers
>+from canonical.launchpad.webapp import canonical_url
>+from canonical.launchpad.webapp.authorization import LaunchpadSecurityPolicy
>+from canonical.launchpad.webapp.interaction import (
>+ get_current_principal, setupInteraction)
>+from canonical.launchpad.webapp.interfaces import IPlacelessAuthUtility
>+from canonical.testing import LaunchpadZopelessLayer, ZopelessAppServerLayer
> from lp.code.enums import (
> BranchMergeProposalStatus, BranchSubscriptionNotificationLevel,
> BranchType, CodeReviewNotificationLevel, CodeReviewVote)

I think it would be helpful to add a blank line between
the canonical imports and the lp imports.

>-from lp.codehosting.vfs import get_lp_server
>-from lp.services.job.runner import JobRunner
>+from lp.code.enums import BranchVisibilityRule
> from lp.code.interfaces.branchlookup import IBranchLookup
>-from canonical.launchpad.database import MessageSet
> from lp.code.model.branchmergeproposaljob import (
> CreateMergeProposalJob, MergeProposalCreatedJob)
>-from lp.code.model.diff import PreviewDiff
>-from canonical.launchpad.interfaces.mail import (
>- EmailProcessingError, IWeaklyAuthenticatedPrincipal)
> from lp.code.mail.codehandler import (
> AddReviewerEmailCommand, CodeEmailCommands, CodeHandler,
> CodeReviewEmailCommandExecutionContext,
> InvalidBranchMergeProposalAddress,
> MissingMergeDirective, NonLaunchpadTarget,
> UpdateStatusEmailCommand, VoteEmailCommand)
>-from canonical.launchpad.mail.handlers import mail_handlers
>+from lp.code.model.diff import PreviewDiff
>+from lp.codehosting.vfs import get_lp_server
>+from lp.registry.interfaces.person import IPersonSet
>+from lp.services.job.runner import JobRunner
> from lp.testing import login, login_person, TestCase, TestCaseWithFactory
> from lp.testing.mail_helpers import pop_notifications
>-from canonical.launchpad.webapp import canonical_url
>-from canonical.launchpad.webapp.authorization import LaunchpadSecurityPolicy
>-from canonical.launchpad.webapp.interaction import (
>- get_current_principal, setupInteraction)
>-from canonical.launchpad.webapp.interfaces import IPlacelessAuthUtility
>-from canonical.testing import LaunchpadZopelessLayer, ZopelessAppServerLayer
>
>
> class TestGetCodeEmailCommands(TestCase):
>@@ -1054,6 +1056,27 @@
> # The hosted copy has not been updated.
> self.assertEqual('rev2', hosted.last_revision())
>
>+ def test_forbidden_target(self):
>+ """Specifying a branch in a forbidden target generates email."""
>+ ...

review: Approve (code)

=== added file 'lib/canonical/launchpad/mail/errortemplates/branch-creation-exception.txt'
--- lib/canonical/launchpad/mail/errortemplates/branch-creation-exception.txt	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/mail/errortemplates/branch-creation-exception.txt	2010-02-10 17:09:26 +0000
@@ -0,0 +1,2 @@
+Launchpad cannot create the branch requested by your merge directive:
+%(reason)s

=== modified file 'lib/lp/code/mail/codehandler.py'
--- lib/lp/code/mail/codehandler.py	2010-01-22 02:52:32 +0000
+++ lib/lp/code/mail/codehandler.py	2010-02-10 17:09:26 +0000
@@ -37,6 +37,7 @@
 from lazr.uri import URI
 from lp.code.enums import BranchType, CodeReviewVote
 from lp.code.errors import BranchMergeProposalExists, UserNotBranchReviewer
+from lp.code.interfaces.branch import BranchCreationException
 from lp.code.interfaces.branchlookup import IBranchLookup
 from lp.code.interfaces.branchmergeproposal import (
     IBranchMergeProposalGetter, ICreateMergeProposalJobSource)
@@ -581,8 +582,13 @@
                 [message.get('from')],
                 'Error Creating Merge Proposal', body)
             return
-
-
+        except BranchCreationException, e:
+            body = get_error_message(
+                    'branch-creation-exception.txt', reason=e)
+            simple_sendmail('[e-mail]',
+                [message.get('from')],
+                'Error Creating Merge Proposal', body)
+            return
         try:
             bmp = source.addLandingTarget(submitter, target,
                                           needs_review=True)

=== modified file 'lib/lp/code/mail/tests/test_codehandler.py'
--- lib/lp/code/mail/tests/test_codehandler.py	2010-02-01 04:15:51 +0000
+++ lib/lp/code/mail/tests/test_codehandler.py	2010-02-10 17:09:26 +0000
@@ -22,33 +22,36 @@
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.config import config
+from canonical.launchpad.database import MessageSet
+from canonical.launchpad.interfaces.mail import (
+    EmailProcessingError, IWeaklyAuthenticatedPrincipal)
+from canonical.launchpad.mail.handlers import mail_handlers
+from canonical.launchpad.webapp import canonical_url
+from canonical.launchpad.webapp.authorization import LaunchpadSecurityPolicy
+from canonical.launchpad.webapp.interaction import (
+    get_current_principal, setupInteraction)
+from canonical.launchpad.webapp.interfaces import IPlacelessAuthUtility
+from canonical.testing import LaunchpadZopelessLayer, ZopelessAppServerLayer
+
 from lp.code.enums import (
     BranchMergeProposalStatus, BranchSubscriptionNotificationLevel,
     BranchType, CodeReviewNotificationLevel, CodeReviewVote)
-from lp.codehosting.vfs import get_lp_server
-from lp.services.job.runner import JobRunner
+from lp.code.enums import BranchVisibilityRule
 from lp.code.interfaces.branchlookup import IBranchLookup
-from canonical.launchpad.database import MessageSet
 from lp.code.model.branchmergeproposaljob import (
     CreateMergeProposalJob, MergeProposalCreatedJob)
-from lp.code.model.diff import PreviewDiff
-from canonical.launchpad.interfaces.mail import (
-    EmailProcessingError, IWeaklyAuthenticatedPrincipal)
 from lp.code.mail.codehandler import (
     AddReviewerEmailCommand, CodeEmailCommands, CodeHandler,
     CodeReviewEmailCommandExecutionContext,
     InvalidBranchMergeProposalAddress,
     MissingMergeDirective, NonLaunchpadTarget,
     UpdateStatusEmailCommand, VoteEmailCommand)
-from canonical.launchpad.mail.handlers import mail_handlers
+from lp.code.model.diff import PreviewDiff
+from lp.codehosting.vfs import get_lp_server
+from lp.registry.interfaces.person import IPersonSet
+from lp.services.job.runner import JobRunner
 from lp.testing import login, login_person, TestCase, TestCaseWithFactory
 from lp.testing.mail_helpers import pop_notifications
-from canonical.launchpad.webapp import canonical_url
-from canonical.launchpad.webapp.authorization import LaunchpadSecurityPolicy
-from canonical.launchpad.webapp.interaction import (
-    get_current_principal, setupInteraction)
-from canonical.launchpad.webapp.interfaces import IPlacelessAuthUtility
-from canonical.testing import LaunchpadZopelessLayer, ZopelessAppServerLayer
 
 
 class TestGetCodeEmailCommands(TestCase):
@@ -1054,6 +1057,28 @@
         # The hosted copy has not been updated.
         self.assertEqual('rev2', hosted.last_revision())
 
+    def test_forbidden_target(self):
+        """Specifying a branch in a forbidden target generates email."""
+        self.useBzrBranches(real_server=True)
+        branch, source, message = self._createTargetSourceAndBundle(
+            format="pack-0.92")
+        branch.product.setBranchVisibilityTeamPolicy(
+            None, BranchVisibilityRule.FORBIDDEN)
+        result = self._processMergeDirective(message)
+        self.assertIs(None, result)
+        notifications = pop_notifications()
+        self.assertEqual(1, len(notifications))
+        self.assertEqual(
+            'Error Creating Merge Proposal', notifications[0]['subject'])
+        body = notifications[0].get_payload(decode=True)
+        sender = getUtility(IPersonSet).getByEmail(message['from'])
+        expected = (
+            'Launchpad cannot create the branch requested by'
+            ' your merge directive:\n'
+            'You cannot create branches in "~%s/%s"\n' %
+            (sender.name, branch.product.name))
+        self.assertEqual(expected, body)
+
 
 class TestVoteEmailCommand(TestCase):
     """Test the vote and tag processing of the VoteEmailCommand."""

=== modified file 'lib/lp/code/model/branchnamespace.py'
--- lib/lp/code/model/branchnamespace.py	2010-01-06 12:09:46 +0000
+++ lib/lp/code/model/branchnamespace.py	2010-02-10 17:09:26 +0000
@@ -138,7 +138,7 @@
 
         if not self.checkCreationPolicy(registrant):
             raise BranchCreationForbidden(
-                "You cannot create branches in %r" % self.name)
+                'You cannot create branches in "%s"' % self.name)
 
     def validateBranchName(self, name):
         """See `IBranchNamespace`."""