Merge lp:~abentley/launchpad/reassign into lp:launchpad

[Aaron Bentley]

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

 reviewer rockstar

= Summary =
This is a follow-up to my claim-review branch. Now that reviews can be
claimed, it must be possible to "un-claim" them, and being able to
reassign reviews is useful for delegation as well.

== Proposed fix ==
Provide an edit icon next to the name of the review that, when clicked,
allows the user to assign the review to someone else. Only the
registrant and the reviewer can see this icon. (But anyone who can
claim the review can claim it and then reassign it.)

== Pre-implementation notes ==
Discussion with the Code team. ui approved by beuno:
https://code.edge.launchpad.net/~abentley/launchpad/reassign/+merge/9463

== Implementation details ==
Can't think of anything unusual.

== Tests ==
bin/test -t test_branchmergeproposal -t test_codereviewvote -t
xx-branch-merge-proposals.txt

== Demo and Q/A ==
Create a branch merge proposal. Request a review. Reassign the review.

= 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/browser/configure.zcml
  lib/canonical/launchpad/pagetitles.py
  lib/lp/code/browser/tests/test_branchmergeproposal.py
  lib/lp/code/browser/codereviewvote.py
  lib/lp/code/stories/branches/xx-branch-merge-proposals.txt
  lib/lp/code/browser/tests/test_codereviewvote.py
  lib/lp/code/browser/branchmergeproposal.py
  lib/lp/code/templates/branchmergeproposal-votes.pt
  lib/lp/code/templates/reviewrequest-reassign.pt
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkp7Hi4ACgkQ0F+nu1YWqI2xSQCeL+UTWZs872RynfCXmieBAYU1
ab0An3LZo43n6ua1D/BAXO0Z1fjP1h/8
=axSL
-----END PGP SIGNATURE-----

[Paul Hummer (rockstar)]

review: Approve

=== modified file 'lib/canonical/launchpad/pagetitles.py'
--- lib/canonical/launchpad/pagetitles.py	2009-07-29 02:38:44 +0000
+++ lib/canonical/launchpad/pagetitles.py	2009-07-29 20:56:34 +0000
@@ -1549,6 +1549,8 @@
 translationgroup_reassignment = ContextTitle(smartquote(
     'Change the owner of "%s" translation group'))
 
+reviewrequest_reassign = 'Reassign review request'
+
 translationgroups_index = 'Launchpad translation groups'
 
 translationimportqueueentry_index = 'Translation import queue entry'

=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py	2009-08-05 14:47:07 +0000
+++ lib/lp/code/browser/branchmergeproposal.py	2009-08-06 18:09:19 +0000
@@ -290,6 +290,18 @@
 
     usedfor = IBranchMergeProposal
 
+    @stepthrough('reviews')
+    def traverse_review(self, id):
+        """Navigate to a CodeReviewVoteReference through its BMP."""
+        try:
+            id = int(id)
+        except ValueError:
+            return None
+        try:
+            return self.context.getVoteReference(id)
+        except WrongBranchMergeProposal:
+            return None
+
     @stepthrough('comments')
     def traverse_comment(self, id):
         try:
@@ -467,6 +479,10 @@
             self.user_can_claim = False
         else:
             self.user_can_claim = self.user_can_review
+        if user in (context.reviewer, context.registrant):
+            self.user_can_reassign = True
+        else:
+            self.user_can_reassign = False
 
     @property
     def show_date_requested(self):

=== added file 'lib/lp/code/browser/codereviewvote.py'
--- lib/lp/code/browser/codereviewvote.py	1970-01-01 00:00:00 +0000
+++ lib/lp/code/browser/codereviewvote.py	2009-08-06 18:09:19 +0000
@@ -0,0 +1,36 @@
+# 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 canonical.launchpad import _
+from canonical.launchpad.fields import PublicPersonChoice
+from canonical.launchpad.webapp import (
+    action, canonical_url, LaunchpadFormView)
+
+
+class ReassignSchema(Interface):
+    """Schema to use when reassigning the reviewer for a requested review."""
+
+    reviewer = PublicPersonChoice( title=_('Reviewer'), required=True,
+            description=_('A person who you want to review this.'),
+            vocabulary='ValidPersonOrTeam')
+
+
+class CodeReviewVoteReassign(LaunchpadFormView):
+    """View for reassinging the reviewer for a requested review."""
+
+    schema = ReassignSchema
+
+    @action('Reassign', name='reassign')
+    def reassign_action(self, action, data):
+        """Use the form data to change the review request reviewer."""
+        self.context.reviewer = data['reviewer']
+        self.next_url = canonical_url(self.context.branch_merge_proposal)

=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml	2009-07-29 02:38:44 +0000
+++ lib/lp/code/browser/configure.zcml	2009-08-06 14:28:41 +0000
@@ -61,6 +61,13 @@
         path_expression="string:+review/${id}"
         attribute_to_parent="branch_merge_proposal"
         rootsite="code"/>
+    <browser:page
+        for="lp.code.interfaces.codereviewvote.ICodeReviewVoteReference"
+        name="+reassign"
+        class="lp.code.browser.codereviewvote.CodeReviewVoteReassign"
+        facet="branches"
+        permission="launchpad.AnyPerson"
+        template="../templates/reviewrequest-reassign.pt"/>
     <facet
         facet="branches">
         <browser:url

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2009-08-06 17:51:02 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2009-08-06 18:09:19 +0000
@@ -180,6 +180,34 @@
         view = BranchMergeProposalVoteView(self.bmp, LaunchpadTestRequest())
         self.assertFalse(view.requested_reviews[0].user_can_claim)
 
+    def makeReviewRequest(self, viewer=None, registrant=None):
+        albert = self.factory.makePerson()
+        if registrant is None:
+            registrant = self.bmp.source_branch.owner
+        self._nominateReviewer(albert, registrant)
+        if viewer is None:
+            viewer = albert
+        login_person(viewer)
+        view = BranchMergeProposalVoteView(self.bmp, LaunchpadTestRequest())
+        return view.requested_reviews[0]
+
+    def test_user_can_reassign_assignee(self):
+        """The user can reassign if they are the assignee."""
+        review_request = self.makeReviewRequest()
+        self.assertTrue(review_request.user_can_reassign)
+
+    def test_user_can_reassign_registrant(self):
+        """The user can reassign if they are the registrant."""
+        registrant = self.factory.makePerson()
+        review_request = self.makeReviewRequest(registrant, registrant)
+        self.assertTrue(review_request.user_can_reassign)
+
+    def test_user_cannot_reassign_random_person(self):
+        """Random people cannot reassign reviews."""
+        viewer = self.factory.makePerson()
+        review_request = self.makeReviewRequest(viewer)
+        self.assertFalse(review_request.user_can_reassign)
+
     def testCurrentReviewOrdering(self):
         # Most recent first.
         # Request three reviews.

=== added file 'lib/lp/code/browser/tests/test_codereviewvote.py'
--- lib/lp/code/browser/tests/test_codereviewvote.py	1970-01-01 00:00:00 +0000
+++ lib/lp/code/browser/tests/test_codereviewvote.py	2009-08-06 18:09:19 +0000
@@ -0,0 +1,40 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+
+"""Unit tests for CodeReviewVoteReferences."""
+
+
+__metaclass__ = type
+
+
+import unittest
+
+from canonical.launchpad.webapp.servers import LaunchpadTestRequest
+from canonical.testing import DatabaseFunctionalLayer
+from lp.testing import (
+    login_person, TestCaseWithFactory)
+from lp.code.browser.codereviewvote import CodeReviewVoteReassign
+
+
+class TestReassignReviewer(TestCaseWithFactory):
+    """Test functionality for changing the reviewer."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_reassign(self):
+        """A reviewer can reassign their vote to someone else."""
+        bmp = self.factory.makeBranchMergeProposal()
+        reviewer = self.factory.makePerson()
+        login_person(bmp.registrant)
+        vote = bmp.nominateReviewer(
+            reviewer=reviewer, registrant=bmp.registrant)
+        new_reviewer = self.factory.makePerson()
+        login_person(reviewer)
+        view = CodeReviewVoteReassign(vote, LaunchpadTestRequest())
+        view.reassign_action.success({'reviewer': new_reviewer})
+        self.assertEqual(vote.reviewer, new_reviewer)
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/code/stories/branches/xx-branch-merge-proposals.txt'
--- lib/lp/code/stories/branches/xx-branch-merge-proposals.txt	2009-08-06 17:51:02 +0000
+++ lib/lp/code/stories/branches/xx-branch-merge-proposals.txt	2009-08-06 17:53:03 +0000
@@ -320,6 +320,22 @@
     Reviewer                         Review Type  Date Requested Status...
     Foo Bar                          claimable    ... ago        Pending ...
 
+The claimant can reassign the review to someone else.
+
+    >>> foobar_browser.getLink('Reassign').click()
+    >>> foobar_browser.getControl('Reviewer').value = 'hwdb-team'
+    >>> foobar_browser.getControl('Reassign').click()
+    >>> foobar_browser.open('http://code.launchpad.dev/~name12/gnome-terminal'
+    ...     '/klingon/+merge/1')
+    >>> pending = find_tag_by_id(
+    ...     foobar_browser.contents, 'code-review-votes')
+
+The review is now reassigned to the HWDB team.
+
+    >>> print extract_text(pending)
+    Reviewer                         Review Type  Date Requested Status...
+    HWDB Team                        claimable    ... ago        Pending ...
+
 
 == Marking as merged ==
 

=== modified file 'lib/lp/code/templates/branchmergeproposal-votes.pt'
--- lib/lp/code/templates/branchmergeproposal-votes.pt	2009-08-05 14:47:07 +0000
+++ lib/lp/code/templates/branchmergeproposal-votes.pt	2009-08-05 20:16:34 +0000
@@ -44,7 +44,13 @@
     <!-- Pending reviews -->
     <tr tal:repeat="review view/requested_reviews"
         tal:attributes="id string:review-${review/reviewer/name}">
-      <td tal:content="structure review/reviewer/fmt:link:mainsite" />
+      <td>
+        <tal:reviewer
+          tal:replace="structure review/reviewer/fmt:link:mainsite" />
+          <a tal:condition="review/user_can_reassign"
+             tal:attributes="href string:reviews/${review/id}/+reassign"><img
+             src="/@@/edit" title="Reassign reviewer" alt="Reassign" /></a>
+      </td>
       <td tal:content="review/review_type" />
       <td>
         <tal:date-requested condition="review/show_date_requested">

=== added file 'lib/lp/code/templates/reviewrequest-reassign.pt'
--- lib/lp/code/templates/reviewrequest-reassign.pt	1970-01-01 00:00:00 +0000
+++ lib/lp/code/templates/reviewrequest-reassign.pt	2009-07-29 20:56:34 +0000
@@ -0,0 +1,22 @@
+<html
+  xmlns="http://www.w3.org/1999/xhtml"
+  xmlns:tal="http://xml.zope.org/namespaces/tal"
+  xmlns:metal="http://xml.zope.org/namespaces/metal"
+  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
+  xml:lang="en"
+  lang="en"
+  dir="ltr"
+  metal:use-macro="view/macro:page/onecolumn"
+  i18n:domain="launchpad"
+>
+<body>
+<div metal:fill-slot="main"
+     tal:define="context_menu context/menu:context">
+
+  <h1>Reassign review request</h1>
+  <div metal:use-macro="context/@@launchpad_form/form">
+    <div metal:fill-slot="extra_info" style="clear: both" />
+  </div>
+</div>
+</body>
+</html>