Launchpad itself

Merge lp:~abentley/launchpad/unauthorized-email into lp:launchpad

  1. unauthorized-email
  2. Merge into devel
Proposed by Aaron Bentley
Status: Merged
Merged at revision: 11640
Proposed branch: lp:~abentley/launchpad/unauthorized-email
Merge into: lp:launchpad
Diff against target: 68 lines (+20/-4)
2 files modified
lib/lp/code/mail/codehandler.py (+2/-1)
lib/lp/code/mail/tests/test_codehandler.py (+18/-3)
To merge this branch: bzr merge lp:~abentley/launchpad/unauthorized-email
Reviewer Review Type Date Requested Status
Abel Deuring (community) code Approve
Review via email: mp+36504@code.launchpad.net

Commit message

Status change emails don't produce OOPses.

Description of the change

= Summary =
Fix Bug #605347: Unauthorized merge approval gives oops rather than
Unauthorized message.

== Proposed fix ==
Handle Unauthorized exceptions like UserNotBranchReviewer exceptions.

== Pre-implementation notes ==

== Implementation details ==
There are two reasons a user may be denied setting a status; They may not have
launchpad.Edit on the merge proposal, or they may not be permitted to set the
particular status they are trying to set.

The result is basically the same, so we can handle Zope's Unauthorized the same
as UserNotBranchReviewer.

== Tests ==
bin/test -t test_not_a_reviewer codehandler

== Demo and Q/A ==
Create a merge proposal. Create another user. Send an email as the second
userr, formatted to set the merge proposal status to 'approved.' You should
get an error message, not an OOPS.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/mail/codehandler.py
  lib/lp/code/mail/tests/test_codehandler.py

./lib/lp/code/mail/codehandler.py
      80: E302 expected 2 blank lines, found 1
      83: E302 expected 2 blank lines, found 1
      86: E302 expected 2 blank lines, found 1
     544: Line exceeds 78 characters.
./lib/lp/code/mail/tests/test_codehandler.py
     197: E201 whitespace after '('
     258: W291 trailing whitespace
     626: W291 trailing whitespace
     769: E202 whitespace before ')'
     858: E202 whitespace before ')'
     881: E202 whitespace before ')'
    1159: E301 expected 1 blank line, found 2
    1161: E301 expected 1 blank line, found 0
    1434: W391 blank line at end of file
     242: Line exceeds 78 characters.
     258: Line has trailing whitespace.
     616: Line exceeds 78 characters.
     622: Line exceeds 78 characters.
     626: Line has trailing whitespace.

To post a comment you must log in.
Revision history for this message
Abel Deuring (adeuring) :
review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/code/mail/codehandler.py'
--- lib/lp/code/mail/codehandler.py 2010-08-20 20:31:18 +0000
+++ lib/lp/code/mail/codehandler.py 2010-09-23 20:47:45 +0000
@@ -24,6 +24,7 @@
24import transaction24import transaction
25from zope.component import getUtility25from zope.component import getUtility
26from zope.interface import implements26from zope.interface import implements
27from zope.security.interfaces import Unauthorized
2728
28from canonical.launchpad.interfaces.mail import (29from canonical.launchpad.interfaces.mail import (
29 EmailProcessingError,30 EmailProcessingError,
@@ -206,7 +207,7 @@
206 command_name=self.name,207 command_name=self.name,
207 arguments='approved, rejected',208 arguments='approved, rejected',
208 example_argument='approved'))209 example_argument='approved'))
209 except UserNotBranchReviewer:210 except (UserNotBranchReviewer, Unauthorized):
210 raise EmailProcessingError(211 raise EmailProcessingError(
211 get_error_message(212 get_error_message(
212 'user-not-reviewer.txt',213 'user-not-reviewer.txt',
213214
=== modified file 'lib/lp/code/mail/tests/test_codehandler.py'
--- lib/lp/code/mail/tests/test_codehandler.py 2010-08-20 20:31:18 +0000
+++ lib/lp/code/mail/tests/test_codehandler.py 2010-09-23 20:47:45 +0000
@@ -75,6 +75,7 @@
75from lp.testing import (75from lp.testing import (
76 login,76 login,
77 login_person,77 login_person,
78 person_logged_in,
78 TestCase,79 TestCase,
79 TestCaseWithFactory,80 TestCaseWithFactory,
80 )81 )
@@ -1254,6 +1255,7 @@
1254 # authorised to update the status.1255 # authorised to update the status.
1255 self.context = CodeReviewEmailCommandExecutionContext(1256 self.context = CodeReviewEmailCommandExecutionContext(
1256 self.merge_proposal, self.merge_proposal.target_branch.owner)1257 self.merge_proposal, self.merge_proposal.target_branch.owner)
1258 self.jrandom = self.factory.makePerson()
1257 transaction.commit()1259 transaction.commit()
1258 self.layer.switchDbUser(config.processmail.dbuser)1260 self.layer.switchDbUser(config.processmail.dbuser)
12591261
@@ -1339,11 +1341,24 @@
1339 str(error))1341 str(error))
13401342
1341 def test_not_a_reviewer(self):1343 def test_not_a_reviewer(self):
1342 # If the user is not a reviewer, they can not update the status.1344 # If the user is not a reviewer, they cannot update the status.
1345 self.context.user = self.jrandom
1346 command = UpdateStatusEmailCommand('status', ['approve'])
1347 with person_logged_in(self.context.user):
1348 error = self.assertRaises(
1349 EmailProcessingError, command.execute, self.context)
1350 target = self.merge_proposal.target_branch.bzr_identity
1351 self.assertEqual(
1352 "You are not a reviewer for the branch %s.\n" % target,
1353 str(error))
1354
1355 def test_registrant_not_a_reviewer(self):
1356 # If the registrant is not a reviewer, they cannot update the status.
1343 self.context.user = self.context.merge_proposal.registrant1357 self.context.user = self.context.merge_proposal.registrant
1344 command = UpdateStatusEmailCommand('status', ['approve'])1358 command = UpdateStatusEmailCommand('status', ['approve'])
1345 error = self.assertRaises(1359 with person_logged_in(self.context.user):
1346 EmailProcessingError, command.execute, self.context)1360 error = self.assertRaises(
1361 EmailProcessingError, command.execute, self.context)
1347 target = self.merge_proposal.target_branch.bzr_identity1362 target = self.merge_proposal.target_branch.bzr_identity
1348 self.assertEqual(1363 self.assertEqual(
1349 "You are not a reviewer for the branch %s.\n" % target,1364 "You are not a reviewer for the branch %s.\n" % target,