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
1=== modified file 'lib/lp/code/mail/codehandler.py'
2--- lib/lp/code/mail/codehandler.py 2010-08-20 20:31:18 +0000
3+++ lib/lp/code/mail/codehandler.py 2010-09-23 20:47:45 +0000
4@@ -24,6 +24,7 @@
5 import transaction
6 from zope.component import getUtility
7 from zope.interface import implements
8+from zope.security.interfaces import Unauthorized
9
10 from canonical.launchpad.interfaces.mail import (
11 EmailProcessingError,
12@@ -206,7 +207,7 @@
13 command_name=self.name,
14 arguments='approved, rejected',
15 example_argument='approved'))
16- except UserNotBranchReviewer:
17+ except (UserNotBranchReviewer, Unauthorized):
18 raise EmailProcessingError(
19 get_error_message(
20 'user-not-reviewer.txt',
21
22=== modified file 'lib/lp/code/mail/tests/test_codehandler.py'
23--- lib/lp/code/mail/tests/test_codehandler.py 2010-08-20 20:31:18 +0000
24+++ lib/lp/code/mail/tests/test_codehandler.py 2010-09-23 20:47:45 +0000
25@@ -75,6 +75,7 @@
26 from lp.testing import (
27 login,
28 login_person,
29+ person_logged_in,
30 TestCase,
31 TestCaseWithFactory,
32 )
33@@ -1254,6 +1255,7 @@
34 # authorised to update the status.
35 self.context = CodeReviewEmailCommandExecutionContext(
36 self.merge_proposal, self.merge_proposal.target_branch.owner)
37+ self.jrandom = self.factory.makePerson()
38 transaction.commit()
39 self.layer.switchDbUser(config.processmail.dbuser)
40
41@@ -1339,11 +1341,24 @@
42 str(error))
43
44 def test_not_a_reviewer(self):
45- # If the user is not a reviewer, they can not update the status.
46+ # If the user is not a reviewer, they cannot update the status.
47+ self.context.user = self.jrandom
48+ command = UpdateStatusEmailCommand('status', ['approve'])
49+ with person_logged_in(self.context.user):
50+ error = self.assertRaises(
51+ EmailProcessingError, command.execute, self.context)
52+ target = self.merge_proposal.target_branch.bzr_identity
53+ self.assertEqual(
54+ "You are not a reviewer for the branch %s.\n" % target,
55+ str(error))
56+
57+ def test_registrant_not_a_reviewer(self):
58+ # If the registrant is not a reviewer, they cannot update the status.
59 self.context.user = self.context.merge_proposal.registrant
60 command = UpdateStatusEmailCommand('status', ['approve'])
61- error = self.assertRaises(
62- EmailProcessingError, command.execute, self.context)
63+ with person_logged_in(self.context.user):
64+ error = self.assertRaises(
65+ EmailProcessingError, command.execute, self.context)
66 target = self.merge_proposal.target_branch.bzr_identity
67 self.assertEqual(
68 "You are not a reviewer for the branch %s.\n" % target,