Launchpad itself

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

  1. forbidden-oops
  2. Merge into devel
Proposed by Aaron Bentley
Status: Merged
Approved by: Edwin Grubbs
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~abentley/launchpad/forbidden-oops
Merge into: lp:launchpad
Diff against target: 130 lines (+49/-16)
4 files modified
lib/canonical/launchpad/mail/errortemplates/branch-creation-exception.txt (+2/-0)
lib/lp/code/mail/codehandler.py (+8/-2)
lib/lp/code/mail/tests/test_codehandler.py (+38/-13)
lib/lp/code/model/branchnamespace.py (+1/-1)
To merge this branch: bzr merge lp:~abentley/launchpad/forbidden-oops
Reviewer Review Type Date Requested Status
Edwin Grubbs (community) code Approve
Review via email: mp+19021@code.launchpad.net

Commit message

Mail instead of oops when branch creation forbidden

To post a comment you must log in.
Revision history for this message
Aaron Bentley (abentley) wrote :

= 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)

Revision history for this message
Edwin Grubbs (edwin-grubbs) wrote :
Download full text (4.5 KiB)

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."""
>+ ...

Read more...

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'lib/canonical/launchpad/mail/errortemplates/branch-creation-exception.txt'
2--- lib/canonical/launchpad/mail/errortemplates/branch-creation-exception.txt 1970-01-01 00:00:00 +0000
3+++ lib/canonical/launchpad/mail/errortemplates/branch-creation-exception.txt 2010-02-10 17:09:26 +0000
4@@ -0,0 +1,2 @@
5+Launchpad cannot create the branch requested by your merge directive:
6+%(reason)s
7
8=== modified file 'lib/lp/code/mail/codehandler.py'
9--- lib/lp/code/mail/codehandler.py 2010-01-22 02:52:32 +0000
10+++ lib/lp/code/mail/codehandler.py 2010-02-10 17:09:26 +0000
11@@ -37,6 +37,7 @@
12 from lazr.uri import URI
13 from lp.code.enums import BranchType, CodeReviewVote
14 from lp.code.errors import BranchMergeProposalExists, UserNotBranchReviewer
15+from lp.code.interfaces.branch import BranchCreationException
16 from lp.code.interfaces.branchlookup import IBranchLookup
17 from lp.code.interfaces.branchmergeproposal import (
18 IBranchMergeProposalGetter, ICreateMergeProposalJobSource)
19@@ -581,8 +582,13 @@
20 [message.get('from')],
21 'Error Creating Merge Proposal', body)
22 return
23-
24-
25+ except BranchCreationException, e:
26+ body = get_error_message(
27+ 'branch-creation-exception.txt', reason=e)
28+ simple_sendmail('merge@code.launchpad.net',
29+ [message.get('from')],
30+ 'Error Creating Merge Proposal', body)
31+ return
32 try:
33 bmp = source.addLandingTarget(submitter, target,
34 needs_review=True)
35
36=== modified file 'lib/lp/code/mail/tests/test_codehandler.py'
37--- lib/lp/code/mail/tests/test_codehandler.py 2010-02-01 04:15:51 +0000
38+++ lib/lp/code/mail/tests/test_codehandler.py 2010-02-10 17:09:26 +0000
39@@ -22,33 +22,36 @@
40 from zope.security.proxy import removeSecurityProxy
41
42 from canonical.config import config
43+from canonical.launchpad.database import MessageSet
44+from canonical.launchpad.interfaces.mail import (
45+ EmailProcessingError, IWeaklyAuthenticatedPrincipal)
46+from canonical.launchpad.mail.handlers import mail_handlers
47+from canonical.launchpad.webapp import canonical_url
48+from canonical.launchpad.webapp.authorization import LaunchpadSecurityPolicy
49+from canonical.launchpad.webapp.interaction import (
50+ get_current_principal, setupInteraction)
51+from canonical.launchpad.webapp.interfaces import IPlacelessAuthUtility
52+from canonical.testing import LaunchpadZopelessLayer, ZopelessAppServerLayer
53+
54 from lp.code.enums import (
55 BranchMergeProposalStatus, BranchSubscriptionNotificationLevel,
56 BranchType, CodeReviewNotificationLevel, CodeReviewVote)
57-from lp.codehosting.vfs import get_lp_server
58-from lp.services.job.runner import JobRunner
59+from lp.code.enums import BranchVisibilityRule
60 from lp.code.interfaces.branchlookup import IBranchLookup
61-from canonical.launchpad.database import MessageSet
62 from lp.code.model.branchmergeproposaljob import (
63 CreateMergeProposalJob, MergeProposalCreatedJob)
64-from lp.code.model.diff import PreviewDiff
65-from canonical.launchpad.interfaces.mail import (
66- EmailProcessingError, IWeaklyAuthenticatedPrincipal)
67 from lp.code.mail.codehandler import (
68 AddReviewerEmailCommand, CodeEmailCommands, CodeHandler,
69 CodeReviewEmailCommandExecutionContext,
70 InvalidBranchMergeProposalAddress,
71 MissingMergeDirective, NonLaunchpadTarget,
72 UpdateStatusEmailCommand, VoteEmailCommand)
73-from canonical.launchpad.mail.handlers import mail_handlers
74+from lp.code.model.diff import PreviewDiff
75+from lp.codehosting.vfs import get_lp_server
76+from lp.registry.interfaces.person import IPersonSet
77+from lp.services.job.runner import JobRunner
78 from lp.testing import login, login_person, TestCase, TestCaseWithFactory
79 from lp.testing.mail_helpers import pop_notifications
80-from canonical.launchpad.webapp import canonical_url
81-from canonical.launchpad.webapp.authorization import LaunchpadSecurityPolicy
82-from canonical.launchpad.webapp.interaction import (
83- get_current_principal, setupInteraction)
84-from canonical.launchpad.webapp.interfaces import IPlacelessAuthUtility
85-from canonical.testing import LaunchpadZopelessLayer, ZopelessAppServerLayer
86
87
88 class TestGetCodeEmailCommands(TestCase):
89@@ -1054,6 +1057,28 @@
90 # The hosted copy has not been updated.
91 self.assertEqual('rev2', hosted.last_revision())
92
93+ def test_forbidden_target(self):
94+ """Specifying a branch in a forbidden target generates email."""
95+ self.useBzrBranches(real_server=True)
96+ branch, source, message = self._createTargetSourceAndBundle(
97+ format="pack-0.92")
98+ branch.product.setBranchVisibilityTeamPolicy(
99+ None, BranchVisibilityRule.FORBIDDEN)
100+ result = self._processMergeDirective(message)
101+ self.assertIs(None, result)
102+ notifications = pop_notifications()
103+ self.assertEqual(1, len(notifications))
104+ self.assertEqual(
105+ 'Error Creating Merge Proposal', notifications[0]['subject'])
106+ body = notifications[0].get_payload(decode=True)
107+ sender = getUtility(IPersonSet).getByEmail(message['from'])
108+ expected = (
109+ 'Launchpad cannot create the branch requested by'
110+ ' your merge directive:\n'
111+ 'You cannot create branches in "~%s/%s"\n' %
112+ (sender.name, branch.product.name))
113+ self.assertEqual(expected, body)
114+
115
116 class TestVoteEmailCommand(TestCase):
117 """Test the vote and tag processing of the VoteEmailCommand."""
118
119=== modified file 'lib/lp/code/model/branchnamespace.py'
120--- lib/lp/code/model/branchnamespace.py 2010-01-06 12:09:46 +0000
121+++ lib/lp/code/model/branchnamespace.py 2010-02-10 17:09:26 +0000
122@@ -138,7 +138,7 @@
123
124 if not self.checkCreationPolicy(registrant):
125 raise BranchCreationForbidden(
126- "You cannot create branches in %r" % self.name)
127+ 'You cannot create branches in "%s"' % self.name)
128
129 def validateBranchName(self, name):
130 """See `IBranchNamespace`."""