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
=== 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 @@
1Launchpad cannot create the branch requested by your merge directive:
2%(reason)s
03
=== 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 @@
37from lazr.uri import URI37from lazr.uri import URI
38from lp.code.enums import BranchType, CodeReviewVote38from lp.code.enums import BranchType, CodeReviewVote
39from lp.code.errors import BranchMergeProposalExists, UserNotBranchReviewer39from lp.code.errors import BranchMergeProposalExists, UserNotBranchReviewer
40from lp.code.interfaces.branch import BranchCreationException
40from lp.code.interfaces.branchlookup import IBranchLookup41from lp.code.interfaces.branchlookup import IBranchLookup
41from lp.code.interfaces.branchmergeproposal import (42from lp.code.interfaces.branchmergeproposal import (
42 IBranchMergeProposalGetter, ICreateMergeProposalJobSource)43 IBranchMergeProposalGetter, ICreateMergeProposalJobSource)
@@ -581,8 +582,13 @@
581 [message.get('from')],582 [message.get('from')],
582 'Error Creating Merge Proposal', body)583 'Error Creating Merge Proposal', body)
583 return584 return
584585 except BranchCreationException, e:
585586 body = get_error_message(
587 'branch-creation-exception.txt', reason=e)
588 simple_sendmail('merge@code.launchpad.net',
589 [message.get('from')],
590 'Error Creating Merge Proposal', body)
591 return
586 try:592 try:
587 bmp = source.addLandingTarget(submitter, target,593 bmp = source.addLandingTarget(submitter, target,
588 needs_review=True)594 needs_review=True)
589595
=== 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 @@
22from zope.security.proxy import removeSecurityProxy22from zope.security.proxy import removeSecurityProxy
2323
24from canonical.config import config24from canonical.config import config
25from canonical.launchpad.database import MessageSet
26from canonical.launchpad.interfaces.mail import (
27 EmailProcessingError, IWeaklyAuthenticatedPrincipal)
28from canonical.launchpad.mail.handlers import mail_handlers
29from canonical.launchpad.webapp import canonical_url
30from canonical.launchpad.webapp.authorization import LaunchpadSecurityPolicy
31from canonical.launchpad.webapp.interaction import (
32 get_current_principal, setupInteraction)
33from canonical.launchpad.webapp.interfaces import IPlacelessAuthUtility
34from canonical.testing import LaunchpadZopelessLayer, ZopelessAppServerLayer
35
25from lp.code.enums import (36from lp.code.enums import (
26 BranchMergeProposalStatus, BranchSubscriptionNotificationLevel,37 BranchMergeProposalStatus, BranchSubscriptionNotificationLevel,
27 BranchType, CodeReviewNotificationLevel, CodeReviewVote)38 BranchType, CodeReviewNotificationLevel, CodeReviewVote)
28from lp.codehosting.vfs import get_lp_server39from lp.code.enums import BranchVisibilityRule
29from lp.services.job.runner import JobRunner
30from lp.code.interfaces.branchlookup import IBranchLookup40from lp.code.interfaces.branchlookup import IBranchLookup
31from canonical.launchpad.database import MessageSet
32from lp.code.model.branchmergeproposaljob import (41from lp.code.model.branchmergeproposaljob import (
33 CreateMergeProposalJob, MergeProposalCreatedJob)42 CreateMergeProposalJob, MergeProposalCreatedJob)
34from lp.code.model.diff import PreviewDiff
35from canonical.launchpad.interfaces.mail import (
36 EmailProcessingError, IWeaklyAuthenticatedPrincipal)
37from lp.code.mail.codehandler import (43from lp.code.mail.codehandler import (
38 AddReviewerEmailCommand, CodeEmailCommands, CodeHandler,44 AddReviewerEmailCommand, CodeEmailCommands, CodeHandler,
39 CodeReviewEmailCommandExecutionContext,45 CodeReviewEmailCommandExecutionContext,
40 InvalidBranchMergeProposalAddress,46 InvalidBranchMergeProposalAddress,
41 MissingMergeDirective, NonLaunchpadTarget,47 MissingMergeDirective, NonLaunchpadTarget,
42 UpdateStatusEmailCommand, VoteEmailCommand)48 UpdateStatusEmailCommand, VoteEmailCommand)
43from canonical.launchpad.mail.handlers import mail_handlers49from lp.code.model.diff import PreviewDiff
50from lp.codehosting.vfs import get_lp_server
51from lp.registry.interfaces.person import IPersonSet
52from lp.services.job.runner import JobRunner
44from lp.testing import login, login_person, TestCase, TestCaseWithFactory53from lp.testing import login, login_person, TestCase, TestCaseWithFactory
45from lp.testing.mail_helpers import pop_notifications54from lp.testing.mail_helpers import pop_notifications
46from canonical.launchpad.webapp import canonical_url
47from canonical.launchpad.webapp.authorization import LaunchpadSecurityPolicy
48from canonical.launchpad.webapp.interaction import (
49 get_current_principal, setupInteraction)
50from canonical.launchpad.webapp.interfaces import IPlacelessAuthUtility
51from canonical.testing import LaunchpadZopelessLayer, ZopelessAppServerLayer
5255
5356
54class TestGetCodeEmailCommands(TestCase):57class TestGetCodeEmailCommands(TestCase):
@@ -1054,6 +1057,28 @@
1054 # The hosted copy has not been updated.1057 # The hosted copy has not been updated.
1055 self.assertEqual('rev2', hosted.last_revision())1058 self.assertEqual('rev2', hosted.last_revision())
10561059
1060 def test_forbidden_target(self):
1061 """Specifying a branch in a forbidden target generates email."""
1062 self.useBzrBranches(real_server=True)
1063 branch, source, message = self._createTargetSourceAndBundle(
1064 format="pack-0.92")
1065 branch.product.setBranchVisibilityTeamPolicy(
1066 None, BranchVisibilityRule.FORBIDDEN)
1067 result = self._processMergeDirective(message)
1068 self.assertIs(None, result)
1069 notifications = pop_notifications()
1070 self.assertEqual(1, len(notifications))
1071 self.assertEqual(
1072 'Error Creating Merge Proposal', notifications[0]['subject'])
1073 body = notifications[0].get_payload(decode=True)
1074 sender = getUtility(IPersonSet).getByEmail(message['from'])
1075 expected = (
1076 'Launchpad cannot create the branch requested by'
1077 ' your merge directive:\n'
1078 'You cannot create branches in "~%s/%s"\n' %
1079 (sender.name, branch.product.name))
1080 self.assertEqual(expected, body)
1081
10571082
1058class TestVoteEmailCommand(TestCase):1083class TestVoteEmailCommand(TestCase):
1059 """Test the vote and tag processing of the VoteEmailCommand."""1084 """Test the vote and tag processing of the VoteEmailCommand."""
10601085
=== 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 @@
138138
139 if not self.checkCreationPolicy(registrant):139 if not self.checkCreationPolicy(registrant):
140 raise BranchCreationForbidden(140 raise BranchCreationForbidden(
141 "You cannot create branches in %r" % self.name)141 'You cannot create branches in "%s"' % self.name)
142142
143 def validateBranchName(self, name):143 def validateBranchName(self, name):
144 """See `IBranchNamespace`."""144 """See `IBranchNamespace`."""