Launchpad itself

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

  1. reassign
  2. Merge into devel
Proposed by Aaron Bentley
Status: Superseded
Proposed branch: lp:~abentley/launchpad/reassign
Merge into: lp:launchpad
Diff against target: None lines
To merge this branch: bzr merge lp:~abentley/launchpad/reassign
Reviewer Review Type Date Requested Status
Martin Albisetti (community) ui Approve
Review via email: mp+9463@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Aaron Bentley (abentley) wrote :

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

 reviewer beuno ui

= Summary =
Allow review requests to be claimed, which can avoid two people working
on a review in parallel. Allow reviews to be reassigned, in case the
claimant changes their mind.

== Proposed fix ==
When a review is assigned to a team, any member of the team can hit the
"claim review" button to claim it.

The person assigned to perform a review can reassign it, and the person
who requested the review can also reassign it.

== Pre-implementation notes ==
Pre- and mid-implementation discussion was with thumper and rockstar

== Implementation details ==
The ability to change a reviewer is now granted by launchpad.Edit on
CodeReviewVoteReference

== Tests ==

== Demo and Q/A ==
Log in as no privilages person. Create a merge proposal and request
Mark Shuttleworth to review it. Reassign it to hwdb-team.

Log in as Foo Bar. Go to the merge proposal page. Claim the review.
Reassign it to Mark Shuttleworth. Note that you can no longer reassign
it, because you are not the requested reviewer.

= 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/lp/code/configure.zcml
  lib/canonical/launchpad/security.py
  lib/canonical/launchpad/pagetitles.py
  lib/lp/code/browser/tests/test_branchmergeproposal.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

iEYEARECAAYFAkpxrD0ACgkQ0F+nu1YWqI3BXwCeM0cbcTuN51JG/aVFNCtm5t6W
KksAniigxjSXdkZ4KKqQ4fUbR+HvNJXC
=FmfX
-----END PGP SIGNATURE-----

Revision history for this message
Martin Albisetti (beuno) wrote :

Hi Aaron, this change is great.

I would like to propose a different UI for it though: place an edit icon next to the reviewers name instead of a link on the far-right.

Is there any reason why you didn't go down that path initially?

review: Needs Fixing (ui)
Revision history for this message
Aaron Bentley (abentley) wrote :

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

Martin Albisetti wrote:
> Review: Needs Fixing ui
> Hi Aaron, this change is great.
>
> I would like to propose a different UI for it though: place an edit icon next to the reviewers name instead of a link on the far-right.
>
> Is there any reason why you didn't go down that path initially?

Good idea. I didn't think of that.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkp54aIACgkQ0F+nu1YWqI22vgCeJC8eJfBWRNpyS50OcBdYFDt4
lSEAniEuAhNpEeAsTD0n911TFfaj7UY8
=R7DN
-----END PGP SIGNATURE-----

Revision history for this message
Aaron Bentley (abentley) wrote :

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

Martin Albisetti wrote:
> Review: Needs Fixing ui
> Hi Aaron, this change is great.
>
> I would like to propose a different UI for it though: place an edit icon next to the reviewers name instead of a link on the far-right.

I've updated this as requested. What do you think?

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkp5684ACgkQ0F+nu1YWqI2aUwCbBzdZI8facO1Dg93OpXcWfyo/
oJYAn2JnIKS3ZzhrGLInZBlu74OON5El
=q6kJ
-----END PGP SIGNATURE-----

Revision history for this message
Martin Albisetti (beuno) wrote :

Fantastic. Thank you for the change, looks good to land for me.

review: Approve (ui)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
1549translationgroup_reassignment = ContextTitle(smartquote(1549translationgroup_reassignment = ContextTitle(smartquote(
1550 'Change the owner of "%s" translation group'))1550 'Change the owner of "%s" translation group'))
15511551
1552reviewrequest_reassign = 'Reassign review request'
1553
1552translationgroups_index = 'Launchpad translation groups'1554translationgroups_index = 'Launchpad translation groups'
15531555
1554translationimportqueueentry_index = 'Translation import queue entry'1556translationimportqueueentry_index = 'Translation import queue entry'
15551557
=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py 2009-07-19 04:41:14 +0000
+++ lib/canonical/launchpad/security.py 2009-07-29 16:19:44 +0000
@@ -38,6 +38,8 @@
38 ICodeImportMachine)38 ICodeImportMachine)
39from lp.code.interfaces.codereviewcomment import (39from lp.code.interfaces.codereviewcomment import (
40 ICodeReviewComment, ICodeReviewCommentDeletion)40 ICodeReviewComment, ICodeReviewCommentDeletion)
41from lp.code.interfaces.codereviewvote import (
42 ICodeReviewVoteReference)
41from lp.registry.interfaces.distribution import IDistribution43from lp.registry.interfaces.distribution import IDistribution
42from lp.registry.interfaces.distributionmirror import (44from lp.registry.interfaces.distributionmirror import (
43 IDistributionMirror)45 IDistributionMirror)
@@ -1670,6 +1672,21 @@
1670 AccessBranch(self.obj.target_branch).checkUnauthenticated())1672 AccessBranch(self.obj.target_branch).checkUnauthenticated())
16711673
16721674
1675class CodeReviewVoteReferenceEdit(AuthorizationBase):
1676 permission = 'launchpad.Edit'
1677 usedfor = ICodeReviewVoteReference
1678
1679 def checkAuthenticated(self, user):
1680 """Only the affected teams may change the review request.
1681
1682 The registrant may reassign the request to another entity.
1683 A member of the review team may assign it to themselves.
1684 A person to whom it is assigned may delegate it to someone else.
1685 """
1686 return (user.inTeam(self.obj.reviewer) or
1687 user.inTeam(self.obj.registrant))
1688
1689
1673class CodeReviewCommentView(AuthorizationBase):1690class CodeReviewCommentView(AuthorizationBase):
1674 permission = 'launchpad.View'1691 permission = 'launchpad.View'
1675 usedfor = ICodeReviewComment1692 usedfor = ICodeReviewComment
16761693
=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py 2009-07-24 03:52:27 +0000
+++ lib/lp/code/browser/branchmergeproposal.py 2009-07-30 14:08:44 +0000
@@ -37,15 +37,14 @@
37from zope.interface import Interface, implements37from zope.interface import Interface, implements
38from zope.schema import Choice, Int, Text38from zope.schema import Choice, Int, Text
39from zope.schema.vocabulary import SimpleVocabulary, SimpleTerm39from zope.schema.vocabulary import SimpleVocabulary, SimpleTerm
40from zope.security.proxy import removeSecurityProxy
4140
42from lazr.lifecycle.event import ObjectModifiedEvent41from lazr.lifecycle.event import ObjectModifiedEvent
4342
44from canonical.cachedproperty import cachedproperty43from canonical.cachedproperty import cachedproperty
4544
46from canonical.launchpad import _45from canonical.launchpad import _
46from canonical.launchpad.fields import PublicPersonChoice
47from lp.code.adapters.branch import BranchMergeProposalDelta47from lp.code.adapters.branch import BranchMergeProposalDelta
48from lp.code.browser.branch import DecoratedBug
49from lp.code.browser.codereviewcomment import CodeReviewDisplayComment48from lp.code.browser.codereviewcomment import CodeReviewDisplayComment
50from canonical.launchpad.fields import Summary, Whiteboard49from canonical.launchpad.fields import Summary, Whiteboard
51from canonical.launchpad.interfaces.message import IMessageSet50from canonical.launchpad.interfaces.message import IMessageSet
@@ -291,6 +290,18 @@
291290
292 usedfor = IBranchMergeProposal291 usedfor = IBranchMergeProposal
293292
293 @stepthrough('reviews')
294 def traverse_review(self, id):
295 try:
296 id = int(id)
297 except ValueError:
298 return None
299 try:
300 return self.context.getVoteReference(id)
301 except WrongBranchMergeProposal:
302 return None
303
304
294 @stepthrough('comments')305 @stepthrough('comments')
295 def traverse_comment(self, id):306 def traverse_comment(self, id):
296 try:307 try:
@@ -328,13 +339,28 @@
328 def __init__(self, comments):339 def __init__(self, comments):
329 self.comments = comments340 self.comments = comments
330341
331342class ClaimButton(Interface):
332class BranchMergeProposalView(LaunchpadView, UnmergedRevisionsMixin,343 """A simple interface to populate the form to enqueue a proposal."""
344
345 review_id = Int(required=True)
346
347
348class BranchMergeProposalView(LaunchpadFormView, UnmergedRevisionsMixin,
333 BranchMergeProposalRevisionIdMixin):349 BranchMergeProposalRevisionIdMixin):
334 """A basic view used for the index page."""350 """A basic view used for the index page."""
335351
336 label = "Proposal to merge branches"352 label = "Proposal to merge branches"
337 __used_for__ = IBranchMergeProposal353 __used_for__ = IBranchMergeProposal
354 schema = ClaimButton
355
356 @action('Claim', name='claim')
357 def claim_action(self, action, data):
358 """Claim this proposal."""
359 request = self.context.getVoteReference(data['review_id'])
360 if request.reviewer == self.user:
361 return
362 request.reviewer = self.user
363 self.next_url = canonical_url(self.context)
338364
339 @property365 @property
340 def comment_location(self):366 def comment_location(self):
@@ -394,6 +420,8 @@
394 @cachedproperty420 @cachedproperty
395 def linked_bugs(self):421 def linked_bugs(self):
396 """Return DecoratedBugs linked to the source branch."""422 """Return DecoratedBugs linked to the source branch."""
423 # Avoid import loop
424 from lp.code.browser.branch import DecoratedBug
397 branch = self.context.source_branch425 branch = self.context.source_branch
398 return [DecoratedBug(bug, branch) for bug in branch.linked_bugs]426 return [DecoratedBug(bug, branch) for bug in branch.linked_bugs]
399427
@@ -429,6 +457,14 @@
429 self.user_can_review = (457 self.user_can_review = (
430 is_mergable and (self.can_change_review or458 is_mergable and (self.can_change_review or
431 (user.inTeam(context.reviewer) and (users_vote is None))))459 (user.inTeam(context.reviewer) and (users_vote is None))))
460 if context.reviewer == user:
461 self.user_can_claim = False
462 else:
463 self.user_can_claim = self.user_can_review
464 if user in (context.reviewer, context.registrant):
465 self.user_can_reassign = True
466 else:
467 self.user_can_reassign = False
432468
433 @property469 @property
434 def show_date_requested(self):470 def show_date_requested(self):
@@ -457,6 +493,22 @@
457 """The text shown in the table of the users vote."""493 """The text shown in the table of the users vote."""
458 return self.status_text_map[self.context.comment.vote]494 return self.status_text_map[self.context.comment.vote]
459495
496class ReassignSchema(Interface):
497
498 reviewer = PublicPersonChoice( title=_('Reviewer'), required=True,
499 description=_('A person who you want to review this.'),
500 vocabulary='ValidPersonOrTeam')
501
502
503class CodeReviewVoteReassign(LaunchpadFormView):
504
505 schema = ReassignSchema
506
507 @action('Reassign', name='reassign')
508 def reassign_action(self, action, data):
509 self.context.reviewer = data['reviewer']
510 self.next_url = canonical_url(self.context.branch_merge_proposal)
511
460512
461class BranchMergeProposalVoteView(LaunchpadView):513class BranchMergeProposalVoteView(LaunchpadView):
462 """The view used for the tables of votes and requested reviews."""514 """The view used for the tables of votes and requested reviews."""
@@ -1114,7 +1166,7 @@
1114 # Claim this vote reference, i.e. say that the individual1166 # Claim this vote reference, i.e. say that the individual
1115 # self. user is doing this review ond behalf of the 'reviewer'1167 # self. user is doing this review ond behalf of the 'reviewer'
1116 # team.1168 # team.
1117 removeSecurityProxy(vote_ref).reviewer = self.user1169 vote_ref.reviewer = self.user
11181170
1119 comment = self.context.createComment(1171 comment = self.context.createComment(
1120 self.user, subject=None, content=data['comment'],1172 self.user, subject=None, content=data['comment'],
11211173
=== 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-07-29 20:56:34 +0000
@@ -61,6 +61,13 @@
61 path_expression="string:+review/${id}"61 path_expression="string:+review/${id}"
62 attribute_to_parent="branch_merge_proposal"62 attribute_to_parent="branch_merge_proposal"
63 rootsite="code"/>63 rootsite="code"/>
64 <browser:page
65 for="lp.code.interfaces.codereviewvote.ICodeReviewVoteReference"
66 name="+reassign"
67 class="lp.code.browser.branchmergeproposal.CodeReviewVoteReassign"
68 facet="branches"
69 permission="launchpad.AnyPerson"
70 template="../templates/reviewrequest-reassign.pt"/>
64 <facet71 <facet
65 facet="branches">72 facet="branches">
66 <browser:url73 <browser:url
6774
=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py 2009-07-17 00:26:05 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py 2009-07-29 20:56:34 +0000
@@ -12,6 +12,7 @@
1212
13import transaction13import transaction
14from zope.component import getMultiAdapter14from zope.component import getMultiAdapter
15from zope.security.interfaces import Unauthorized
1516
16from lp.code.browser.branch import RegisterBranchMergeProposalView17from lp.code.browser.branch import RegisterBranchMergeProposalView
17from lp.code.browser.branchmergeproposal import (18from lp.code.browser.branchmergeproposal import (
@@ -149,6 +150,61 @@
149 [charles, bob, albert],150 [charles, bob, albert],
150 [review.reviewer for review in requested_reviews])151 [review.reviewer for review in requested_reviews])
151152
153 def test_user_can_claim_self(self):
154 """Someone cannot claim a review already assigned to them."""
155 albert = self.factory.makePerson()
156 owner = self.bmp.source_branch.owner
157 self._nominateReviewer(albert, owner)
158 login_person(albert)
159 view = BranchMergeProposalVoteView(self.bmp, LaunchpadTestRequest())
160 self.assertFalse(view.requested_reviews[0].user_can_claim)
161
162 def test_user_can_claim_member(self):
163 """Someone can claim a review already assigned their team."""
164 albert = self.factory.makePerson()
165 review_team = self.factory.makeTeam()
166 albert.join(review_team)
167 owner = self.bmp.source_branch.owner
168 self._nominateReviewer(review_team, owner)
169 login_person(albert)
170 view = BranchMergeProposalVoteView(self.bmp, LaunchpadTestRequest())
171 self.assertTrue(view.requested_reviews[0].user_can_claim)
172
173 def test_user_can_claim_nonmember(self):
174 """A non-member cannot claim a team's review."""
175 albert = self.factory.makePerson()
176 review_team = self.factory.makeTeam()
177 owner = self.bmp.source_branch.owner
178 self._nominateReviewer(review_team, owner)
179 login_person(albert)
180 view = BranchMergeProposalVoteView(self.bmp, LaunchpadTestRequest())
181 self.assertFalse(view.requested_reviews[0].user_can_claim)
182
183 def makeReviewRequest(self, viewer=None, registrant=None):
184 albert = self.factory.makePerson()
185 if registrant is None:
186 registrant = self.bmp.source_branch.owner
187 self._nominateReviewer(albert, registrant)
188 if viewer is None:
189 viewer = albert
190 login_person(viewer)
191 view = BranchMergeProposalVoteView(self.bmp, LaunchpadTestRequest())
192 return view.requested_reviews[0]
193
194 def test_user_can_reassign_assignee(self):
195 review_request = self.makeReviewRequest()
196 self.assertTrue(review_request.user_can_reassign)
197
198 def test_user_can_reassign_registrant(self):
199 registrant = self.factory.makePerson()
200 review_request = self.makeReviewRequest(registrant, registrant)
201 self.assertTrue(review_request.user_can_reassign)
202
203 def test_user_can_reassign_random_person(self):
204 viewer = self.factory.makePerson()
205 review_request = self.makeReviewRequest(viewer)
206 self.assertFalse(review_request.user_can_reassign)
207
152 def testCurrentReviewOrdering(self):208 def testCurrentReviewOrdering(self):
153 # Most recent first.209 # Most recent first.
154 # Request three reviews.210 # Request three reviews.
@@ -371,6 +427,30 @@
371 view.initialize()427 view.initialize()
372 return view428 return view
373429
430 def makeTeamReview(self):
431 owner = self.bmp.source_branch.owner
432 review_team = self.factory.makeTeam()
433 return self.bmp.nominateReviewer(review_team, owner)
434
435 def test_claim_action_team_member(self):
436 """Claiming a review works for members of the requested team."""
437 review = self.makeTeamReview()
438 albert = self.factory.makePerson()
439 albert.join(review.reviewer)
440 login_person(albert)
441 view = self._createView()
442 view.claim_action.success({'review_id': review.id})
443 self.assertEqual(albert, review.reviewer)
444
445 def test_claim_action_non_member(self):
446 """Claiming a review does not work for non-members."""
447 review = self.makeTeamReview()
448 albert = self.factory.makePerson()
449 login_person(albert)
450 view = self._createView()
451 self.assertRaises(Unauthorized, view.claim_action.success,
452 {'review_id': review.id})
453
374 def test_review_diff_with_no_diff(self):454 def test_review_diff_with_no_diff(self):
375 """review_diff should be None when there is no context.review_diff."""455 """review_diff should be None when there is no context.review_diff."""
376 view = self._createView()456 view = self._createView()
377457
=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml 2009-07-23 02:06:55 +0000
+++ lib/lp/code/configure.zcml 2009-07-29 16:19:44 +0000
@@ -30,6 +30,7 @@
30 </securedutility>30 </securedutility>
31 <class class="lp.code.model.codereviewvote.CodeReviewVoteReference">31 <class class="lp.code.model.codereviewvote.CodeReviewVoteReference">
32 <allow interface="lp.code.interfaces.codereviewvote.ICodeReviewVoteReference"/>32 <allow interface="lp.code.interfaces.codereviewvote.ICodeReviewVoteReference"/>
33 <require permission="launchpad.Edit" set_attributes="reviewer" />
33 </class>34 </class>
34 <subscriber35 <subscriber
35 for="lp.code.interfaces.codereviewvote.ICodeReviewVoteReference36 for="lp.code.interfaces.codereviewvote.ICodeReviewVoteReference
3637
=== modified file 'lib/lp/code/templates/branchmergeproposal-votes.pt'
--- lib/lp/code/templates/branchmergeproposal-votes.pt 2009-07-17 17:59:07 +0000
+++ lib/lp/code/templates/branchmergeproposal-votes.pt 2009-07-29 20:56:34 +0000
@@ -53,9 +53,19 @@
53 </td>53 </td>
54 <td>54 <td>
55 <span class="votePENDING">Pending</span>55 <span class="votePENDING">Pending</span>
56 <tal:reassign-link condition="review/user_can_reassign">
57 <a tal:attributes="href
58 string:reviews/${review/id}/+reassign">[Reassign]</a>
59 </tal:reassign-link>
56 <tal:vote-link condition="review/user_can_review">60 <tal:vote-link condition="review/user_can_review">
57 <a tal:attributes="href string:+review?claim=${review/reviewer/name}&amp;review_type=${review/review_type_str}">[Review]</a>61 <a tal:attributes="href string:+review?claim=${review/reviewer/name}&amp;review_type=${review/review_type_str}">[Review]</a>
58 </tal:vote-link>62 </tal:vote-link>
63 <form method="POST" tal:condition="review/user_can_claim">
64 <input type="submit" name="field.actions.claim"
65 id="field.actions.claim" value="Claim review" />
66 <input type="hidden" name="field.review_id" id="field.claim"
67 tal:attributes="value string:${review/id}" />
68 </form>
59 </td>69 </td>
60 </tr>70 </tr>
61 <tr>71 <tr>
6272
=== 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 @@
1<html
2 xmlns="http://www.w3.org/1999/xhtml"
3 xmlns:tal="http://xml.zope.org/namespaces/tal"
4 xmlns:metal="http://xml.zope.org/namespaces/metal"
5 xmlns:i18n="http://xml.zope.org/namespaces/i18n"
6 xml:lang="en"
7 lang="en"
8 dir="ltr"
9 metal:use-macro="view/macro:page/onecolumn"
10 i18n:domain="launchpad"
11>
12<body>
13<div metal:fill-slot="main"
14 tal:define="context_menu context/menu:context">
15
16 <h1>Reassign review request</h1>
17 <div metal:use-macro="context/@@launchpad_form/form">
18 <div metal:fill-slot="extra_info" style="clear: both" />
19 </div>
20</div>
21</body>
22</html>