Merge lp:~thumper/launchpad/reassign-review-into-model into lp:launchpad/db-devel

[Tim Penhey]

Move the review reassignment logic into the model.

[Tim Penhey (thumper)]

This branch moves the logic for review reassignment into the model code.

tests:
  TestCodeReviewVoteReferenceReassignReview
  TestReassignReviewer

[Jonathan Lange (jml)]

On Fri, Dec 11, 2009 at 11:59 AM, Tim Penhey <email address hidden> wrote:
> Tim Penhey has proposed merging lp:~thumper/launchpad/reassign-review-into-model into lp:launchpad.
>
>    Requested reviews:
>    Canonical Launchpad Engineering (launchpad)
> Related bugs:
>  #495201 Review reassignment doesn't check for existing reviews
>  https://bugs.launchpad.net/bugs/495201
>
>
> This branch moves the logic for review reassignment into the model code.
>
> tests:
>  TestCodeReviewVoteReferenceReassignReview
>  TestReassignReviewer
>
> --
> https://code.launchpad.net/~thumper/launchpad/reassign-review-into-model/+merge/15991
> Your team Launchpad code reviewers from Canonical is subscribed to branch lp:launchpad.
>
> === modified file 'lib/lp/code/browser/codereviewvote.py'
> --- lib/lp/code/browser/codereviewvote.py       2009-12-10 20:46:32 +0000
> +++ lib/lp/code/browser/codereviewvote.py       2009-12-11 00:59:25 +0000
...
> @@ -35,8 +32,5 @@
>     @action('Reassign', name='reassign')
>     def reassign_action(self, action, data):
>         """Use the form data to change the review request reviewer."""
> -        # XXX TimPenhey 2009-12-11 bug=495201
> -        # This should check for existing reviews by the reviewer, and have
> -        # the logic moved into the model code.
> -        removeSecurityProxy(self.context).reviewer = data['reviewer']
> +        self.context.reassignReview(data['reviewer'])

This function can raise errors that simple assignment would not. How
are you handling those errors?

Hooray for removing the removeSecurityProxy call.

> === modified file 'lib/lp/code/errors.py'
> --- lib/lp/code/errors.py       2009-12-07 06:51:42 +0000
> +++ lib/lp/code/errors.py       2009-12-11 00:59:25 +0000
> @@ -10,6 +10,7 @@
>     'BranchMergeProposalExists',
>     'ClaimReviewFailed',
>     'InvalidBranchMergeProposal',
> +    'ReassignReviewFailed',
>     'ReviewNotPending',
>     'UserNotBranchReviewer',
>     'WrongBranchMergeProposal',
> @@ -39,6 +40,10 @@
>     """Raised if there is already a matching BranchMergeProposal."""
>
>
> +class ReassignReviewFailed(Exception):
> +    """Failed to reassign the review request."""
> +
> +
>  class ReviewNotPending(Exception):
>     """The requested review is not in a pending state."""
>
>
> === modified file 'lib/lp/code/interfaces/codereviewvote.py'
> --- lib/lp/code/interfaces/codereviewvote.py    2009-12-07 06:51:42 +0000
> +++ lib/lp/code/interfaces/codereviewvote.py    2009-12-11 00:59:25 +0000
> @@ -17,10 +17,11 @@
>     IBranchMergeProposal)
>  from lp.code.interfaces.codereviewcomment import (
>     ICodeReviewComment)
> +from lp.registry.interfaces.person import IPerson
>  from lazr.restful.fields import Reference
>  from lazr.restful.declarations import (
>     call_with, export_as_webservice_entry, export_destructor_operation,
> -    export_write_operation, exported, REQUEST_USER)
> +    export_write_operation, exported, operation_parameters, REQUEST_USER)
>
>
>  class ICodeReviewVoteReferencePublic(Interface):
> @@ -86,6 +87,22 @@
>             not pending.
>         """
>
> +    @operation_parameters(
> +        reviewer=Reference(
> +            title=_("The person or ...

[Jonathan Lange (jml)]

Hey Tim,

Thanks for doing this. It's encouraging to see XXXs and removeSecurityProxy calls disappear.

I've got a few questions, mostly around the terminology that the patch uses. I look forward to hearing back from you.

jml

review: Needs Fixing

[Tim Penhey (thumper)]

On Fri, 11 Dec 2009 15:36:17 Jonathan Lange wrote:
> This function can raise errors that simple assignment would not. How
> are you handling those errors?

Right now I'm not. We are only one step better where we are not getting the
data into an inconsistent state, but the error handling still needs work.

I could change the XXX and file another bug if you like. The same error
handling is needed for claiming reviews.

Moving in baby steps.

> Hooray for removing the removeSecurityProxy call.
>
> > === modified file 'lib/lp/code/errors.py'
> > --- lib/lp/code/errors.py 2009-12-07 06:51:42 +0000
> > +++ lib/lp/code/errors.py 2009-12-11 00:59:25 +0000
> > @@ -10,6 +10,7 @@
> > 'BranchMergeProposalExists',
> > 'ClaimReviewFailed',
> > 'InvalidBranchMergeProposal',
> > + 'ReassignReviewFailed',
> > 'ReviewNotPending',
> > 'UserNotBranchReviewer',
> > 'WrongBranchMergeProposal',
> > @@ -39,6 +40,10 @@
> > """Raised if there is already a matching BranchMergeProposal."""
> >
> >
> > +class ReassignReviewFailed(Exception):
> > + """Failed to reassign the review request."""
> > +
> > +
> > class ReviewNotPending(Exception):
> > """The requested review is not in a pending state."""
> >
> >
> > === modified file 'lib/lp/code/interfaces/codereviewvote.py'
> > --- lib/lp/code/interfaces/codereviewvote.py 2009-12-07 06:51:42 +0000
> > +++ lib/lp/code/interfaces/codereviewvote.py 2009-12-11 00:59:25 +0000
> > @@ -17,10 +17,11 @@
> > IBranchMergeProposal)
> > from lp.code.interfaces.codereviewcomment import (
> > ICodeReviewComment)
> > +from lp.registry.interfaces.person import IPerson
> > from lazr.restful.fields import Reference
> > from lazr.restful.declarations import (
> > call_with, export_as_webservice_entry, export_destructor_operation,
> > - export_write_operation, exported, REQUEST_USER)
> > + export_write_operation, exported, operation_parameters,
> > REQUEST_USER)
> >
> >
> > class ICodeReviewVoteReferencePublic(Interface):
> > @@ -86,6 +87,22 @@
> > not pending.
> > """
> >
> > + @operation_parameters(
> > + reviewer=Reference(
> > + title=_("The person or team to assign to do the review."),
> > + schema=IPerson))
> > + @export_write_operation()
> > + def reassignReview(reviewer):
> > + """Reassign a pending review to someone else.
> > +
> > + Pending reviews can be reassigned to someone else.
> > +
> > + :param reviewer: The person to assign the pending review to.
> > + :raises ReviewNotPending: If the review is not pending.
> > + :raises ReassignReviewFailed: If the reviewer is an individual
> > and + already has a personal review.
> > + """
> > +
> > @export_destructor_operation()
> > def delete():
> > """Delete the pending review.
> >
> > === modified file 'lib/lp/code/model/codereviewvote.py'
> > --- lib/lp/code/model/codereviewvote.py 2009-12-07 06:51:42 +0000
> > +++ lib/lp/code/model/codereviewvote.py 2009-12-11 00:59:25 +0000
>
> ...
>
> > @@ -66,6 +67,22 @@
> > error_str % claimant.unique_displayname)
> > ...

[Jonathan Lange (jml)]

On Fri, Dec 11, 2009 at 3:15 PM, Tim Penhey <email address hidden> wrote:
> On Fri, 11 Dec 2009 15:36:17 Jonathan Lange wrote:
>> This function can raise errors that simple assignment would not. How
>> are you handling those errors?
>
> Right now I'm not.  We are only one step better where we are not getting the
> data into an inconsistent state, but the error handling still needs work.
>
> I could change the XXX and file another bug if you like.  The same error
> handling is needed for claiming reviews.
>
> Moving in baby steps.
>

That means it will raise an OOPS, right? Is it really that difficult to fix it?

...
>> >  self.branch_merge_proposal.getUsersVoteReference(reviewer)) +
>> >  if existing_review is not None:
>> > +                if existing_review.is_pending:
>> > +                    error_str = '%s has an existing pending review'
>> > +                else:
>> > +                    error_str = '%s has an existing personal review'
>>
>> This code path is untested. AFAICT, you only test
>> ReassignReviewerFailed once, so you cannot possibly be testing both
>> strings. Can you please either drop the condition or add a test?
>
> Yeah, can do.  I was considering this.
>

Thanks.

>> > +                raise ReassignReviewFailed(
>> > +                    error_str % reviewer.unique_displayname)
>> > +        self.reviewer = reviewer
>> > +
>>
>> The logic here is straightforward, but it's hard to figure out what
>> the error messages actually mean.
>>
>> Why does "pending" matter? Do these error strings get displayed to the
>> user? If so, they don't give the user any indication as to what to do
>> about the problem.
>
> I wrote it with the intent of showing them to the user, and in fact they are
> shown to a user using the API.  Pending only matters for the readabiltiy of
> the error, and for no other reason.
>

In that case, the text needs to be changed. If I tried to reassign a
review to you and was told "Tim Penhey already has an existing
personal review", I wouldn't know what do about it, or really what a
personal review was.

Doesn't it really mean "Tim Penhey has already reviewed this" or "Tim
Penhey has already been asked to review this"?

...
>> > +    def test_reassign_to_user_existing(self):
>> > +        # If a user has an existing review, they cannot have another
>> > +        # pending review assigned to them.
>> > +        bmp, review = self.makeMergeProposalWithReview()
>> > +        reviewer = self.factory.makePerson()
>> > +        user_review = bmp.nominateReviewer(
>> > +            reviewer=reviewer, registrant=bmp.registrant)
>> > +        self.assertRaises(
>> > +            ReassignReviewFailed, review.reassignReview, reviewer)
>> > +
>> > +    def test_reassign_to_team_existing(self):
>> > +        # If a team has an existing review, they can have another
>> > pending +        # review assigned to them.
>>
>> It took me a while to grok that the important different between this
>> test and the previous test is that one is for users and the other is
>> for teams.
>>
>> It's odd that you use 'personal' in the code and 'user' in the tests.
>> Could you please pick one set of terms and stick with them?
>
> W...

[Tim Penhey (thumper)]

<jml> aiui, if I try to assign a thing to you for review, and you are already assigned to do it
<thumper> I'm not sure I'll get that finished tonight
<jml> the error is "Tim Penhey already has an existing pending review"
<jml> is that right?
<thumper> almost: "Tim Penhey (thumper) already has an existing pending review"
<jml> ahh ok.
<jml> how about "Tim Penhey (thumper) has already been asked to review this"
<thumper> sounds fine
<thumper> better even
<jml> and likewise, "Tim Penhey has already reviewed this"
<jml> rather than 'has a personal review'
<thumper> yeah, they sound good
<jml> cool.
<jml> makes it easier to understand what the code is for, also :)

[Tim Penhey (thumper)]

On Fri, 11 Dec 2009 18:09:12 Jonathan Lange wrote:
> That means it will raise an OOPS, right? Is it really that difficult to fix
> it?

Fixed, and updated the story.

> >> > self.branch_merge_proposal.getUsersVoteReference(reviewer)) +
> >> > if existing_review is not None:
> >> > + if existing_review.is_pending:
> >> > + error_str = '%s has an existing pending review'
> >> > + else:
> >> > + error_str = '%s has an existing personal review'
> >>
> >> This code path is untested. AFAICT, you only test
> >> ReassignReviewerFailed once, so you cannot possibly be testing both
> >> strings. Can you please either drop the condition or add a test?
> >
> > Yeah, can do. I was considering this.
>
> Thanks.

Done

> >> > + raise ReassignReviewFailed(
> >> > + error_str % reviewer.unique_displayname)
> >> > + self.reviewer = reviewer
> >> > +
> >>
> >> The logic here is straightforward, but it's hard to figure out what
> >> the error messages actually mean.
> >>
> >> Why does "pending" matter? Do these error strings get displayed to the
> >> user? If so, they don't give the user any indication as to what to do
> >> about the problem.
> >
> > I wrote it with the intent of showing them to the user, and in fact they
> > are shown to a user using the API. Pending only matters for the
> > readabiltiy of the error, and for no other reason.
>
> In that case, the text needs to be changed. If I tried to reassign a
> review to you and was told "Tim Penhey already has an existing
> personal review", I wouldn't know what do about it, or really what a
> personal review was.
>
> Doesn't it really mean "Tim Penhey has already reviewed this" or "Tim
> Penhey has already been asked to review this"?

Updated following our IRC discussion.

[Jonathan Lange (jml)]

Thanks Tim.

review: Approve

=== modified file 'lib/lp/code/browser/codereviewvote.py'
--- lib/lp/code/browser/codereviewvote.py	2009-12-10 20:46:32 +0000
+++ lib/lp/code/browser/codereviewvote.py	2009-12-14 09:24:24 +0000
@@ -1,20 +1,18 @@
 # Copyright 2009 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-
 """Views, navigation and actions for CodeReviewVotes."""
 
-
 __metaclass__ = type
 
 
 from zope.interface import Interface
-from zope.security.proxy import removeSecurityProxy
 
 from canonical.launchpad import _
 from canonical.launchpad.fields import PublicPersonChoice
 from canonical.launchpad.webapp import (
     action, canonical_url, LaunchpadFormView)
+from lp.code.errors import ReviewNotPending, UserHasExistingReview
 
 
 class ReassignSchema(Interface):
@@ -35,8 +33,14 @@
     @action('Reassign', name='reassign')
     def reassign_action(self, action, data):
         """Use the form data to change the review request reviewer."""
-        # XXX TimPenhey 2009-12-11 bug=495201
-        # This should check for existing reviews by the reviewer, and have
-        # the logic moved into the model code.
-        removeSecurityProxy(self.context).reviewer = data['reviewer']
+        self.context.reassignReview(data['reviewer'])
         self.next_url = canonical_url(self.context.branch_merge_proposal)
+
+    def validate(self, data):
+        """Make sure that the reassignment can happen."""
+        reviewer = data.get('reviewer')
+        if reviewer is not None:
+            try:
+                self.context.validateReasignReview(reviewer)
+            except (ReviewNotPending, UserHasExistingReview), e:
+                self.addError(str(e))

=== modified file 'lib/lp/code/errors.py'
--- lib/lp/code/errors.py	2009-12-07 06:51:42 +0000
+++ lib/lp/code/errors.py	2009-12-14 09:24:24 +0000
@@ -11,6 +11,7 @@
     'ClaimReviewFailed',
     'InvalidBranchMergeProposal',
     'ReviewNotPending',
+    'UserHasExistingReview',
     'UserNotBranchReviewer',
     'WrongBranchMergeProposal',
 ]
@@ -43,6 +44,10 @@
     """The requested review is not in a pending state."""
 
 
+class UserHasExistingReview(Exception):
+    """The user has an existing review."""
+
+
 class UserNotBranchReviewer(Exception):
     """The user who attempted to review the merge proposal isn't a reviewer.
 

=== modified file 'lib/lp/code/interfaces/codereviewvote.py'
--- lib/lp/code/interfaces/codereviewvote.py	2009-12-07 06:51:42 +0000
+++ lib/lp/code/interfaces/codereviewvote.py	2009-12-14 09:24:24 +0000
@@ -17,10 +17,11 @@
     IBranchMergeProposal)
 from lp.code.interfaces.codereviewcomment import (
     ICodeReviewComment)
+from lp.registry.interfaces.person import IPerson
 from lazr.restful.fields import Reference
 from lazr.restful.declarations import (
     call_with, export_as_webservice_entry, export_destructor_operation,
-    export_write_operation, exported, REQUEST_USER)
+    export_write_operation, exported, operation_parameters, REQUEST_USER)
 
 
 class ICodeReviewVoteReferencePublic(Interface):
@@ -70,6 +71,15 @@
 class ICodeReviewVoteReferenceEdit(Interface):
     """Method that require edit permissions."""
 
+    def validateClaimReview(claimant):
+        """Implements the validation for claiming a review.
+
+        :raises ClaimReviewFailed: If the claimant already has a
+            personal review, if the reviewer is not a team, if the
+            claimant is not in the reviewer team, or if the review is
+            not pending.
+        """
+
     @call_with(claimant=REQUEST_USER)
     @export_write_operation()
     def claimReview(claimant):
@@ -86,6 +96,30 @@
             not pending.
         """
 
+    def validateReasignReview(reviewer):
+        """Implements the validation for reassignment.
+
+        :raises ReviewNotPending: If the review is not pending.
+        :raises ReassignReviewFailed: If the reviewer is an individual and
+            already has a personal review.
+        """
+
+    @operation_parameters(
+        reviewer=Reference(
+            title=_("The person or team to assign to do the review."),
+            schema=IPerson))
+    @export_write_operation()
+    def reassignReview(reviewer):
+        """Reassign a pending review to someone else.
+
+        Pending reviews can be reassigned to someone else.
+
+        :param reviewer: The person to assign the pending review to.
+        :raises ReviewNotPending: If the review is not pending.
+        :raises ReassignReviewFailed: If the reviewer is an individual and
+            already has a personal review.
+        """
+
     @export_destructor_operation()
     def delete():
         """Delete the pending review.

=== modified file 'lib/lp/code/model/codereviewvote.py'
--- lib/lp/code/model/codereviewvote.py	2009-12-07 06:51:42 +0000
+++ lib/lp/code/model/codereviewvote.py	2009-12-14 09:24:24 +0000
@@ -15,7 +15,8 @@
 from canonical.database.constants import DEFAULT
 from canonical.database.datetimecol import UtcDateTimeCol
 from canonical.database.sqlbase import SQLBase
-from lp.code.errors import ClaimReviewFailed, ReviewNotPending
+from lp.code.errors import (
+    ClaimReviewFailed, ReviewNotPending, UserHasExistingReview)
 from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
 
 
@@ -44,10 +45,25 @@
         # Reviews are pending if there is no associated comment.
         return self.comment is None
 
-    def claimReview(self, claimant):
+    def _validatePending(self):
+        """Raise if the review is not pending."""
+        if not self.is_pending:
+            raise ReviewNotPending('The review is not pending.')
+
+    def _validateNoReviewForUser(self, user):
+        """Make sure there isn't an existing review for the user."""
+        bmp = self.branch_merge_proposal
+        existing_review = bmp.getUsersVoteReference(user)
+        if existing_review is not None:
+            if existing_review.is_pending:
+                error_str = '%s has already been asked to review this'
+            else:
+                error_str = '%s has already reviewed this'
+            raise UserHasExistingReview(error_str % user.unique_displayname)
+
+    def validateClaimReview(self, claimant):
         """See `ICodeReviewVote`"""
-        if not self.is_pending:
-            raise ClaimReviewFailed('The review is not pending.')
+        self._validatePending()
         if not self.reviewer.is_team:
             raise ClaimReviewFailed('Cannot claim non-team reviews.')
         if not claimant.inTeam(self.reviewer):
@@ -55,17 +71,24 @@
                 '%s is not a member of %s' %
                 (claimant.unique_displayname,
                  self.reviewer.unique_displayname))
-        claimant_review = (
-            self.branch_merge_proposal.getUsersVoteReference(claimant))
-        if claimant_review is not None:
-            if claimant_review.is_pending:
-                error_str = '%s has an existing pending review'
-            else:
-                error_str = '%s has an existing personal review'
-            raise ClaimReviewFailed(
-                error_str % claimant.unique_displayname)
+        self._validateNoReviewForUser(claimant)
+
+    def claimReview(self, claimant):
+        """See `ICodeReviewVote`"""
+        self.validateClaimReview(claimant)
         self.reviewer = claimant
 
+    def validateReasignReview(self, reviewer):
+        """See `ICodeReviewVote`"""
+        self._validatePending()
+        if not reviewer.is_team:
+            self._validateNoReviewForUser(reviewer)
+
+    def reassignReview(self, reviewer):
+        """See `ICodeReviewVote`"""
+        self.validateReasignReview(reviewer)
+        self.reviewer = reviewer
+
     def delete(self):
         """See `ICodeReviewVote`"""
         if not self.is_pending:

=== modified file 'lib/lp/code/model/tests/test_codereviewvote.py'
--- lib/lp/code/model/tests/test_codereviewvote.py	2009-12-07 06:51:42 +0000
+++ lib/lp/code/model/tests/test_codereviewvote.py	2009-12-14 09:24:24 +0000
@@ -9,7 +9,8 @@
 from canonical.testing import DatabaseFunctionalLayer
 
 from lp.code.enums import CodeReviewVote
-from lp.code.errors import ClaimReviewFailed, ReviewNotPending
+from lp.code.errors import (
+    ClaimReviewFailed, ReviewNotPending, UserHasExistingReview)
 from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
 from lp.testing import login_person, TestCaseWithFactory
 
@@ -43,7 +44,7 @@
         TestCaseWithFactory.setUp(self)
         # Setup the proposal, claimant and team reviewer.
         self.bmp = self.factory.makeBranchMergeProposal()
-        self.claimant = self.factory.makePerson()
+        self.claimant = self.factory.makePerson(name='eric')
         self.review_team = self.factory.makeTeam()
 
     def _addPendingReview(self):
@@ -71,8 +72,10 @@
             vote=CodeReviewVote.APPROVE)
         review = self._addPendingReview()
         self._addClaimantToReviewTeam()
-        self.assertRaises(
-            ClaimReviewFailed, review.claimReview, self.claimant)
+        self.assertRaisesWithContent(
+            UserHasExistingReview,
+            'Eric (eric) has already reviewed this',
+            review.claimReview, self.claimant)
 
     def test_personal_pending_review(self):
         # If the claimant has a pending review already, then they can't claim
@@ -83,8 +86,10 @@
         self.bmp.nominateReviewer(
             reviewer=self.claimant, registrant=self.bmp.registrant)
         login_person(self.claimant)
-        self.assertRaises(
-            ClaimReviewFailed, review.claimReview, self.claimant)
+        self.assertRaisesWithContent(
+            UserHasExistingReview,
+            'Eric (eric) has already been asked to review this',
+            review.claimReview, self.claimant)
 
     def test_personal_not_in_review_team(self):
         # If the claimant is not in the review team, an error is raised.
@@ -183,5 +188,75 @@
         self.assertRaises(ReviewNotPending, review.delete)
 
 
+class TestCodeReviewVoteReferenceReassignReview(TestCaseWithFactory):
+    """Tests for CodeReviewVoteReference.reassignReview."""
+
+    layer = DatabaseFunctionalLayer
+
+    def makeMergeProposalWithReview(self, completed=False):
+        """Return a new merge proposal with a review."""
+        bmp = self.factory.makeBranchMergeProposal()
+        reviewer = self.factory.makePerson()
+        if completed:
+            login_person(reviewer)
+            bmp.createComment(
+                reviewer, 'Message subject', 'Message content',
+                vote=CodeReviewVote.APPROVE)
+            [review] = list(bmp.votes)
+        else:
+            login_person(bmp.registrant)
+            review = bmp.nominateReviewer(
+                reviewer=reviewer, registrant=bmp.registrant)
+        return bmp, review
+
+    def test_reassign_pending(self):
+        # A pending review can be reassigned to someone else.
+        bmp, review = self.makeMergeProposalWithReview()
+        new_reviewer = self.factory.makePerson()
+        review.reassignReview(new_reviewer)
+        self.assertEqual(new_reviewer, review.reviewer)
+
+    def test_reassign_completed_review(self):
+        # A completed review cannot be reassigned
+        bmp, review = self.makeMergeProposalWithReview(completed=True)
+        self.assertRaises(
+            ReviewNotPending, review.reassignReview, bmp.registrant)
+
+    def test_reassign_to_user_existing_pending(self):
+        # If a user has an existing pending review, they cannot have another
+        # pending review assigned to them.
+        bmp, review = self.makeMergeProposalWithReview()
+        reviewer = self.factory.makePerson(name='eric')
+        user_review = bmp.nominateReviewer(
+            reviewer=reviewer, registrant=bmp.registrant)
+        self.assertRaisesWithContent(
+            UserHasExistingReview,
+            'Eric (eric) has already been asked to review this',
+            review.reassignReview, reviewer)
+
+    def test_reassign_to_user_existing_completed(self):
+        # If a user has an existing completed review, they cannot have another
+        # pending review assigned to them.
+        bmp, review = self.makeMergeProposalWithReview()
+        reviewer = self.factory.makePerson(name='eric')
+        bmp.createComment(
+            reviewer, 'Message subject', 'Message content',
+            vote=CodeReviewVote.APPROVE)
+        self.assertRaisesWithContent(
+            UserHasExistingReview,
+            'Eric (eric) has already reviewed this',
+            review.reassignReview, reviewer)
+
+    def test_reassign_to_team_existing(self):
+        # If a team has an existing review, they can have another pending
+        # review assigned to them.
+        bmp, review = self.makeMergeProposalWithReview()
+        reviewer_team = self.factory.makeTeam()
+        team_review = bmp.nominateReviewer(
+            reviewer=reviewer_team, registrant=bmp.registrant)
+        review.reassignReview(reviewer_team)
+        self.assertEqual(reviewer_team, review.reviewer)
+
+
 def test_suite():
     return TestLoader().loadTestsFromName(__name__)

=== renamed file 'lib/lp/code/stories/branches/xx-branch-merge-proposals.txt' => 'lib/lp/code/stories/branches/xx-branchmergeproposals.txt'
--- lib/lp/code/stories/branches/xx-branch-merge-proposals.txt	2009-11-27 01:29:10 +0000
+++ lib/lp/code/stories/branches/xx-branchmergeproposals.txt	2009-12-14 09:24:24 +0000
@@ -233,15 +233,23 @@
 The claimant can reassign the review to someone else.
 
     >>> foobar_browser.getLink('Reassign').click()
+    >>> foobar_browser.getControl('Reviewer').value = 'no-priv'
+    >>> foobar_browser.getControl('Reassign').click()
+
+If the person already has a review, the user gets an error...
+
+    >>> print_feedback_messages(foobar_browser.contents)
+    There is 1 error.
+    No Privileges Person (no-priv) has already reviewed this
+
+... if not, the review is reassigned.
+
     >>> foobar_browser.getControl('Reviewer').value = 'hwdb-team'
     >>> foobar_browser.getControl('Reassign').click()
-    >>> foobar_browser.open(klingon_proposal)
-    >>> pending = find_tag_by_id(
-    ...     foobar_browser.contents, 'code-review-votes')
 
 The review is now reassigned to the HWDB team.
 
-    >>> print extract_text(pending)
+    >>> print_tag_with_id(foobar_browser.contents, 'code-review-votes')
     Reviewer                         Review Type  Date Requested Status...
     HWDB Team                        claimable    ... ago        Pending ...
 
@@ -294,8 +302,7 @@
     >>> sample_browser.getControl('Merged Revision Number').value = '42'
     >>> sample_browser.getControl('Mark as Merged').click()
 
-    >>> for message in get_feedback_messages(sample_browser.contents):
-    ...     print extract_text(message)
+    >>> print_feedback_messages(sample_browser.contents)
     The proposal's merged revision has been updated.
     >>> print_summary(sample_browser)
     Status: Merged
@@ -436,8 +443,7 @@
 setting it gives an appropriate error.
 
     >>> nopriv_browser.getControl('Propose Merge').click()
-    >>> for message in get_feedback_messages(nopriv_browser.contents):
-    ...     print extract_text(message)
+    >>> print_feedback_messages(nopriv_browser.contents)
     There is 1 error.
     Required input is missing.
 
@@ -447,8 +453,7 @@
     ...     name='field.target_branch.target_branch').value = (
     ...         'fooix')
     >>> nopriv_browser.getControl('Propose Merge').click()
-    >>> for message in get_feedback_messages(nopriv_browser.contents):
-    ...     print extract_text(message)
+    >>> print_feedback_messages(nopriv_browser.contents)
     There is 1 error.
     Invalid value