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
1=== modified file 'lib/canonical/launchpad/pagetitles.py'
2--- lib/canonical/launchpad/pagetitles.py 2009-07-29 02:38:44 +0000
3+++ lib/canonical/launchpad/pagetitles.py 2009-07-29 20:56:34 +0000
4@@ -1549,6 +1549,8 @@
5 translationgroup_reassignment = ContextTitle(smartquote(
6 'Change the owner of "%s" translation group'))
7
8+reviewrequest_reassign = 'Reassign review request'
9+
10 translationgroups_index = 'Launchpad translation groups'
11
12 translationimportqueueentry_index = 'Translation import queue entry'
13
14=== modified file 'lib/canonical/launchpad/security.py'
15--- lib/canonical/launchpad/security.py 2009-07-19 04:41:14 +0000
16+++ lib/canonical/launchpad/security.py 2009-07-29 16:19:44 +0000
17@@ -38,6 +38,8 @@
18 ICodeImportMachine)
19 from lp.code.interfaces.codereviewcomment import (
20 ICodeReviewComment, ICodeReviewCommentDeletion)
21+from lp.code.interfaces.codereviewvote import (
22+ ICodeReviewVoteReference)
23 from lp.registry.interfaces.distribution import IDistribution
24 from lp.registry.interfaces.distributionmirror import (
25 IDistributionMirror)
26@@ -1670,6 +1672,21 @@
27 AccessBranch(self.obj.target_branch).checkUnauthenticated())
28
29
30+class CodeReviewVoteReferenceEdit(AuthorizationBase):
31+ permission = 'launchpad.Edit'
32+ usedfor = ICodeReviewVoteReference
33+
34+ def checkAuthenticated(self, user):
35+ """Only the affected teams may change the review request.
36+
37+ The registrant may reassign the request to another entity.
38+ A member of the review team may assign it to themselves.
39+ A person to whom it is assigned may delegate it to someone else.
40+ """
41+ return (user.inTeam(self.obj.reviewer) or
42+ user.inTeam(self.obj.registrant))
43+
44+
45 class CodeReviewCommentView(AuthorizationBase):
46 permission = 'launchpad.View'
47 usedfor = ICodeReviewComment
48
49=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
50--- lib/lp/code/browser/branchmergeproposal.py 2009-07-24 03:52:27 +0000
51+++ lib/lp/code/browser/branchmergeproposal.py 2009-07-30 14:08:44 +0000
52@@ -37,15 +37,14 @@
53 from zope.interface import Interface, implements
54 from zope.schema import Choice, Int, Text
55 from zope.schema.vocabulary import SimpleVocabulary, SimpleTerm
56-from zope.security.proxy import removeSecurityProxy
57
58 from lazr.lifecycle.event import ObjectModifiedEvent
59
60 from canonical.cachedproperty import cachedproperty
61
62 from canonical.launchpad import _
63+from canonical.launchpad.fields import PublicPersonChoice
64 from lp.code.adapters.branch import BranchMergeProposalDelta
65-from lp.code.browser.branch import DecoratedBug
66 from lp.code.browser.codereviewcomment import CodeReviewDisplayComment
67 from canonical.launchpad.fields import Summary, Whiteboard
68 from canonical.launchpad.interfaces.message import IMessageSet
69@@ -291,6 +290,18 @@
70
71 usedfor = IBranchMergeProposal
72
73+ @stepthrough('reviews')
74+ def traverse_review(self, id):
75+ try:
76+ id = int(id)
77+ except ValueError:
78+ return None
79+ try:
80+ return self.context.getVoteReference(id)
81+ except WrongBranchMergeProposal:
82+ return None
83+
84+
85 @stepthrough('comments')
86 def traverse_comment(self, id):
87 try:
88@@ -328,13 +339,28 @@
89 def __init__(self, comments):
90 self.comments = comments
91
92-
93-class BranchMergeProposalView(LaunchpadView, UnmergedRevisionsMixin,
94+class ClaimButton(Interface):
95+ """A simple interface to populate the form to enqueue a proposal."""
96+
97+ review_id = Int(required=True)
98+
99+
100+class BranchMergeProposalView(LaunchpadFormView, UnmergedRevisionsMixin,
101 BranchMergeProposalRevisionIdMixin):
102 """A basic view used for the index page."""
103
104 label = "Proposal to merge branches"
105 __used_for__ = IBranchMergeProposal
106+ schema = ClaimButton
107+
108+ @action('Claim', name='claim')
109+ def claim_action(self, action, data):
110+ """Claim this proposal."""
111+ request = self.context.getVoteReference(data['review_id'])
112+ if request.reviewer == self.user:
113+ return
114+ request.reviewer = self.user
115+ self.next_url = canonical_url(self.context)
116
117 @property
118 def comment_location(self):
119@@ -394,6 +420,8 @@
120 @cachedproperty
121 def linked_bugs(self):
122 """Return DecoratedBugs linked to the source branch."""
123+ # Avoid import loop
124+ from lp.code.browser.branch import DecoratedBug
125 branch = self.context.source_branch
126 return [DecoratedBug(bug, branch) for bug in branch.linked_bugs]
127
128@@ -429,6 +457,14 @@
129 self.user_can_review = (
130 is_mergable and (self.can_change_review or
131 (user.inTeam(context.reviewer) and (users_vote is None))))
132+ if context.reviewer == user:
133+ self.user_can_claim = False
134+ else:
135+ self.user_can_claim = self.user_can_review
136+ if user in (context.reviewer, context.registrant):
137+ self.user_can_reassign = True
138+ else:
139+ self.user_can_reassign = False
140
141 @property
142 def show_date_requested(self):
143@@ -457,6 +493,22 @@
144 """The text shown in the table of the users vote."""
145 return self.status_text_map[self.context.comment.vote]
146
147+class ReassignSchema(Interface):
148+
149+ reviewer = PublicPersonChoice( title=_('Reviewer'), required=True,
150+ description=_('A person who you want to review this.'),
151+ vocabulary='ValidPersonOrTeam')
152+
153+
154+class CodeReviewVoteReassign(LaunchpadFormView):
155+
156+ schema = ReassignSchema
157+
158+ @action('Reassign', name='reassign')
159+ def reassign_action(self, action, data):
160+ self.context.reviewer = data['reviewer']
161+ self.next_url = canonical_url(self.context.branch_merge_proposal)
162+
163
164 class BranchMergeProposalVoteView(LaunchpadView):
165 """The view used for the tables of votes and requested reviews."""
166@@ -1114,7 +1166,7 @@
167 # Claim this vote reference, i.e. say that the individual
168 # self. user is doing this review ond behalf of the 'reviewer'
169 # team.
170- removeSecurityProxy(vote_ref).reviewer = self.user
171+ vote_ref.reviewer = self.user
172
173 comment = self.context.createComment(
174 self.user, subject=None, content=data['comment'],
175
176=== modified file 'lib/lp/code/browser/configure.zcml'
177--- lib/lp/code/browser/configure.zcml 2009-07-29 02:38:44 +0000
178+++ lib/lp/code/browser/configure.zcml 2009-07-29 20:56:34 +0000
179@@ -61,6 +61,13 @@
180 path_expression="string:+review/${id}"
181 attribute_to_parent="branch_merge_proposal"
182 rootsite="code"/>
183+ <browser:page
184+ for="lp.code.interfaces.codereviewvote.ICodeReviewVoteReference"
185+ name="+reassign"
186+ class="lp.code.browser.branchmergeproposal.CodeReviewVoteReassign"
187+ facet="branches"
188+ permission="launchpad.AnyPerson"
189+ template="../templates/reviewrequest-reassign.pt"/>
190 <facet
191 facet="branches">
192 <browser:url
193
194=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
195--- lib/lp/code/browser/tests/test_branchmergeproposal.py 2009-07-17 00:26:05 +0000
196+++ lib/lp/code/browser/tests/test_branchmergeproposal.py 2009-07-29 20:56:34 +0000
197@@ -12,6 +12,7 @@
198
199 import transaction
200 from zope.component import getMultiAdapter
201+from zope.security.interfaces import Unauthorized
202
203 from lp.code.browser.branch import RegisterBranchMergeProposalView
204 from lp.code.browser.branchmergeproposal import (
205@@ -149,6 +150,61 @@
206 [charles, bob, albert],
207 [review.reviewer for review in requested_reviews])
208
209+ def test_user_can_claim_self(self):
210+ """Someone cannot claim a review already assigned to them."""
211+ albert = self.factory.makePerson()
212+ owner = self.bmp.source_branch.owner
213+ self._nominateReviewer(albert, owner)
214+ login_person(albert)
215+ view = BranchMergeProposalVoteView(self.bmp, LaunchpadTestRequest())
216+ self.assertFalse(view.requested_reviews[0].user_can_claim)
217+
218+ def test_user_can_claim_member(self):
219+ """Someone can claim a review already assigned their team."""
220+ albert = self.factory.makePerson()
221+ review_team = self.factory.makeTeam()
222+ albert.join(review_team)
223+ owner = self.bmp.source_branch.owner
224+ self._nominateReviewer(review_team, owner)
225+ login_person(albert)
226+ view = BranchMergeProposalVoteView(self.bmp, LaunchpadTestRequest())
227+ self.assertTrue(view.requested_reviews[0].user_can_claim)
228+
229+ def test_user_can_claim_nonmember(self):
230+ """A non-member cannot claim a team's review."""
231+ albert = self.factory.makePerson()
232+ review_team = self.factory.makeTeam()
233+ owner = self.bmp.source_branch.owner
234+ self._nominateReviewer(review_team, owner)
235+ login_person(albert)
236+ view = BranchMergeProposalVoteView(self.bmp, LaunchpadTestRequest())
237+ self.assertFalse(view.requested_reviews[0].user_can_claim)
238+
239+ def makeReviewRequest(self, viewer=None, registrant=None):
240+ albert = self.factory.makePerson()
241+ if registrant is None:
242+ registrant = self.bmp.source_branch.owner
243+ self._nominateReviewer(albert, registrant)
244+ if viewer is None:
245+ viewer = albert
246+ login_person(viewer)
247+ view = BranchMergeProposalVoteView(self.bmp, LaunchpadTestRequest())
248+ return view.requested_reviews[0]
249+
250+ def test_user_can_reassign_assignee(self):
251+ review_request = self.makeReviewRequest()
252+ self.assertTrue(review_request.user_can_reassign)
253+
254+ def test_user_can_reassign_registrant(self):
255+ registrant = self.factory.makePerson()
256+ review_request = self.makeReviewRequest(registrant, registrant)
257+ self.assertTrue(review_request.user_can_reassign)
258+
259+ def test_user_can_reassign_random_person(self):
260+ viewer = self.factory.makePerson()
261+ review_request = self.makeReviewRequest(viewer)
262+ self.assertFalse(review_request.user_can_reassign)
263+
264 def testCurrentReviewOrdering(self):
265 # Most recent first.
266 # Request three reviews.
267@@ -371,6 +427,30 @@
268 view.initialize()
269 return view
270
271+ def makeTeamReview(self):
272+ owner = self.bmp.source_branch.owner
273+ review_team = self.factory.makeTeam()
274+ return self.bmp.nominateReviewer(review_team, owner)
275+
276+ def test_claim_action_team_member(self):
277+ """Claiming a review works for members of the requested team."""
278+ review = self.makeTeamReview()
279+ albert = self.factory.makePerson()
280+ albert.join(review.reviewer)
281+ login_person(albert)
282+ view = self._createView()
283+ view.claim_action.success({'review_id': review.id})
284+ self.assertEqual(albert, review.reviewer)
285+
286+ def test_claim_action_non_member(self):
287+ """Claiming a review does not work for non-members."""
288+ review = self.makeTeamReview()
289+ albert = self.factory.makePerson()
290+ login_person(albert)
291+ view = self._createView()
292+ self.assertRaises(Unauthorized, view.claim_action.success,
293+ {'review_id': review.id})
294+
295 def test_review_diff_with_no_diff(self):
296 """review_diff should be None when there is no context.review_diff."""
297 view = self._createView()
298
299=== modified file 'lib/lp/code/configure.zcml'
300--- lib/lp/code/configure.zcml 2009-07-23 02:06:55 +0000
301+++ lib/lp/code/configure.zcml 2009-07-29 16:19:44 +0000
302@@ -30,6 +30,7 @@
303 </securedutility>
304 <class class="lp.code.model.codereviewvote.CodeReviewVoteReference">
305 <allow interface="lp.code.interfaces.codereviewvote.ICodeReviewVoteReference"/>
306+ <require permission="launchpad.Edit" set_attributes="reviewer" />
307 </class>
308 <subscriber
309 for="lp.code.interfaces.codereviewvote.ICodeReviewVoteReference
310
311=== modified file 'lib/lp/code/templates/branchmergeproposal-votes.pt'
312--- lib/lp/code/templates/branchmergeproposal-votes.pt 2009-07-17 17:59:07 +0000
313+++ lib/lp/code/templates/branchmergeproposal-votes.pt 2009-07-29 20:56:34 +0000
314@@ -53,9 +53,19 @@
315 </td>
316 <td>
317 <span class="votePENDING">Pending</span>
318+ <tal:reassign-link condition="review/user_can_reassign">
319+ <a tal:attributes="href
320+ string:reviews/${review/id}/+reassign">[Reassign]</a>
321+ </tal:reassign-link>
322 <tal:vote-link condition="review/user_can_review">
323 <a tal:attributes="href string:+review?claim=${review/reviewer/name}&amp;review_type=${review/review_type_str}">[Review]</a>
324 </tal:vote-link>
325+ <form method="POST" tal:condition="review/user_can_claim">
326+ <input type="submit" name="field.actions.claim"
327+ id="field.actions.claim" value="Claim review" />
328+ <input type="hidden" name="field.review_id" id="field.claim"
329+ tal:attributes="value string:${review/id}" />
330+ </form>
331 </td>
332 </tr>
333 <tr>
334
335=== added file 'lib/lp/code/templates/reviewrequest-reassign.pt'
336--- lib/lp/code/templates/reviewrequest-reassign.pt 1970-01-01 00:00:00 +0000
337+++ lib/lp/code/templates/reviewrequest-reassign.pt 2009-07-29 20:56:34 +0000
338@@ -0,0 +1,22 @@
339+<html
340+ xmlns="http://www.w3.org/1999/xhtml"
341+ xmlns:tal="http://xml.zope.org/namespaces/tal"
342+ xmlns:metal="http://xml.zope.org/namespaces/metal"
343+ xmlns:i18n="http://xml.zope.org/namespaces/i18n"
344+ xml:lang="en"
345+ lang="en"
346+ dir="ltr"
347+ metal:use-macro="view/macro:page/onecolumn"
348+ i18n:domain="launchpad"
349+>
350+<body>
351+<div metal:fill-slot="main"
352+ tal:define="context_menu context/menu:context">
353+
354+ <h1>Reassign review request</h1>
355+ <div metal:use-macro="context/@@launchpad_form/form">
356+ <div metal:fill-slot="extra_info" style="clear: both" />
357+ </div>
358+</div>
359+</body>
360+</html>