launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/unauthorized-email into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #605347 Unauthorized merge approval gives oops rather than Unauthorized message.
  https://bugs.launchpad.net/bugs/605347


= 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.
-- 
https://code.launchpad.net/~abentley/launchpad/unauthorized-email/+merge/36504
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/unauthorized-email into lp:launchpad/devel.

=== 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 @@
 import transaction
 from zope.component import getUtility
 from zope.interface import implements
+from zope.security.interfaces import Unauthorized
 
 from canonical.launchpad.interfaces.mail import (
     EmailProcessingError,
@@ -206,7 +207,7 @@
                         command_name=self.name,
                         arguments='approved, rejected',
                         example_argument='approved'))
-        except UserNotBranchReviewer:
+        except (UserNotBranchReviewer, Unauthorized):
             raise EmailProcessingError(
                 get_error_message(
                     'user-not-reviewer.txt',

=== 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 @@
 from lp.testing import (
     login,
     login_person,
+    person_logged_in,
     TestCase,
     TestCaseWithFactory,
     )
@@ -1254,6 +1255,7 @@
         # authorised to update the status.
         self.context = CodeReviewEmailCommandExecutionContext(
             self.merge_proposal, self.merge_proposal.target_branch.owner)
+        self.jrandom = self.factory.makePerson()
         transaction.commit()
         self.layer.switchDbUser(config.processmail.dbuser)
 
@@ -1339,11 +1341,24 @@
             str(error))
 
     def test_not_a_reviewer(self):
-        # If the user is not a reviewer, they can not update the status.
+        # If the user is not a reviewer, they cannot update the status.
+        self.context.user = self.jrandom
+        command = UpdateStatusEmailCommand('status', ['approve'])
+        with person_logged_in(self.context.user):
+            error = self.assertRaises(
+                EmailProcessingError, command.execute, self.context)
+        target = self.merge_proposal.target_branch.bzr_identity
+        self.assertEqual(
+            "You are not a reviewer for the branch %s.\n" % target,
+            str(error))
+
+    def test_registrant_not_a_reviewer(self):
+        # If the registrant is not a reviewer, they cannot update the status.
         self.context.user = self.context.merge_proposal.registrant
         command = UpdateStatusEmailCommand('status', ['approve'])
-        error = self.assertRaises(
-            EmailProcessingError, command.execute, self.context)
+        with person_logged_in(self.context.user):
+            error = self.assertRaises(
+                EmailProcessingError, command.execute, self.context)
         target = self.merge_proposal.target_branch.bzr_identity
         self.assertEqual(
             "You are not a reviewer for the branch %s.\n" % target,