Launchpad itself

Merge lp:~thumper/launchpad/delete-pending-reviews into lp:launchpad/db-devel

  1. delete-pending-reviews
  2. Merge into db-devel
Proposed by Tim Penhey
Status: Merged
Merged at revision: 8777
Proposed branch: lp:~thumper/launchpad/delete-pending-reviews
Merge into: lp:launchpad/db-devel
Prerequisite: lp:~thumper/launchpad/claim-review-into-model-browser-integration
Diff against target: 223 lines (+108/-9)
7 files modified
lib/canonical/launchpad/security.py (+8/-2)
lib/lp/code/browser/codereviewvote.py (+5/-1)
lib/lp/code/configure.zcml (+1/-2)
lib/lp/code/errors.py (+5/-0)
lib/lp/code/interfaces/codereviewvote.py (+9/-2)
lib/lp/code/model/codereviewvote.py (+7/-1)
lib/lp/code/model/tests/test_codereviewvote.py (+73/-1)
To merge this branch: bzr merge lp:~thumper/launchpad/delete-pending-reviews
Reviewer Review Type Date Requested Status
Paul Hummer (community) Approve
Review via email: mp+15739@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

Adds the ability to delete pending reviews into the model code.

tests:
 TestCodeReviewVoteReferenceDelete

Revision history for this message
Paul Hummer (rockstar) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/security.py'
2--- lib/canonical/launchpad/security.py 2009-11-19 15:14:53 +0000
3+++ lib/canonical/launchpad/security.py 2009-12-10 20:48:30 +0000
4@@ -1676,9 +1676,15 @@
5 The registrant may reassign the request to another entity.
6 A member of the review team may assign it to themselves.
7 A person to whom it is assigned may delegate it to someone else.
8+
9+ Anyone with edit permissions on the target branch of the merge
10+ proposal can also edit the reviews.
11 """
12- return (user.inTeam(self.obj.reviewer) or
13- user.inTeam(self.obj.registrant))
14+ if user.inTeam(self.obj.reviewer) or user.inTeam(self.obj.registrant):
15+ return True
16+ target_access = EditBranch(
17+ self.obj.branch_merge_proposal.target_branch)
18+ return target_access.checkAuthenticated(user)
19
20
21 class CodeReviewCommentView(AuthorizationBase):
22
23=== modified file 'lib/lp/code/browser/codereviewvote.py'
24--- lib/lp/code/browser/codereviewvote.py 2009-09-10 20:49:25 +0000
25+++ lib/lp/code/browser/codereviewvote.py 2009-12-10 20:48:30 +0000
26@@ -9,6 +9,7 @@
27
28
29 from zope.interface import Interface
30+from zope.security.proxy import removeSecurityProxy
31
32 from canonical.launchpad import _
33 from canonical.launchpad.fields import PublicPersonChoice
34@@ -34,5 +35,8 @@
35 @action('Reassign', name='reassign')
36 def reassign_action(self, action, data):
37 """Use the form data to change the review request reviewer."""
38- self.context.reviewer = data['reviewer']
39+ # XXX TimPenhey 2009-12-11 bug=495201
40+ # This should check for existing reviews by the reviewer, and have
41+ # the logic moved into the model code.
42+ removeSecurityProxy(self.context).reviewer = data['reviewer']
43 self.next_url = canonical_url(self.context.branch_merge_proposal)
44
45=== modified file 'lib/lp/code/configure.zcml'
46--- lib/lp/code/configure.zcml 2009-12-07 03:56:18 +0000
47+++ lib/lp/code/configure.zcml 2009-12-10 20:48:30 +0000
48@@ -33,8 +33,7 @@
49 <allow interface="lp.code.interfaces.codereviewvote.ICodeReviewVoteReferencePublic"/>
50 <require
51 permission="launchpad.Edit"
52- interface="lp.code.interfaces.codereviewvote.ICodeReviewVoteReferenceEdit"
53- set_attributes="reviewer" />
54+ interface="lp.code.interfaces.codereviewvote.ICodeReviewVoteReferenceEdit"/>
55 </class>
56 <subscriber
57 for="lp.code.interfaces.codereviewvote.ICodeReviewVoteReference
58
59=== modified file 'lib/lp/code/errors.py'
60--- lib/lp/code/errors.py 2009-12-07 03:56:18 +0000
61+++ lib/lp/code/errors.py 2009-12-10 20:48:30 +0000
62@@ -10,6 +10,7 @@
63 'BranchMergeProposalExists',
64 'ClaimReviewFailed',
65 'InvalidBranchMergeProposal',
66+ 'ReviewNotPending',
67 'UserNotBranchReviewer',
68 'WrongBranchMergeProposal',
69 ]
70@@ -38,6 +39,10 @@
71 """Raised if there is already a matching BranchMergeProposal."""
72
73
74+class ReviewNotPending(Exception):
75+ """The requested review is not in a pending state."""
76+
77+
78 class UserNotBranchReviewer(Exception):
79 """The user who attempted to review the merge proposal isn't a reviewer.
80
81
82=== modified file 'lib/lp/code/interfaces/codereviewvote.py'
83--- lib/lp/code/interfaces/codereviewvote.py 2009-12-07 03:56:18 +0000
84+++ lib/lp/code/interfaces/codereviewvote.py 2009-12-10 20:48:30 +0000
85@@ -19,8 +19,8 @@
86 ICodeReviewComment)
87 from lazr.restful.fields import Reference
88 from lazr.restful.declarations import (
89- call_with, export_as_webservice_entry, export_write_operation, exported,
90- REQUEST_USER)
91+ call_with, export_as_webservice_entry, export_destructor_operation,
92+ export_write_operation, exported, REQUEST_USER)
93
94
95 class ICodeReviewVoteReferencePublic(Interface):
96@@ -86,6 +86,13 @@
97 not pending.
98 """
99
100+ @export_destructor_operation()
101+ def delete():
102+ """Delete the pending review.
103+
104+ :raises ReviewNotPending: If the review is not pending.
105+ """
106+
107
108 class ICodeReviewVoteReference(ICodeReviewVoteReferencePublic,
109 ICodeReviewVoteReferenceEdit):
110
111=== modified file 'lib/lp/code/model/codereviewvote.py'
112--- lib/lp/code/model/codereviewvote.py 2009-12-07 03:56:18 +0000
113+++ lib/lp/code/model/codereviewvote.py 2009-12-10 20:48:30 +0000
114@@ -15,7 +15,7 @@
115 from canonical.database.constants import DEFAULT
116 from canonical.database.datetimecol import UtcDateTimeCol
117 from canonical.database.sqlbase import SQLBase
118-from lp.code.errors import ClaimReviewFailed
119+from lp.code.errors import ClaimReviewFailed, ReviewNotPending
120 from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
121
122
123@@ -65,3 +65,9 @@
124 raise ClaimReviewFailed(
125 error_str % claimant.unique_displayname)
126 self.reviewer = claimant
127+
128+ def delete(self):
129+ """See `ICodeReviewVote`"""
130+ if not self.is_pending:
131+ raise ReviewNotPending('The review is not pending.')
132+ self.destroySelf()
133
134=== modified file 'lib/lp/code/model/tests/test_codereviewvote.py'
135--- lib/lp/code/model/tests/test_codereviewvote.py 2009-12-07 03:56:18 +0000
136+++ lib/lp/code/model/tests/test_codereviewvote.py 2009-12-10 20:48:30 +0000
137@@ -9,7 +9,7 @@
138 from canonical.testing import DatabaseFunctionalLayer
139
140 from lp.code.enums import CodeReviewVote
141-from lp.code.errors import ClaimReviewFailed
142+from lp.code.errors import ClaimReviewFailed, ReviewNotPending
143 from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
144 from lp.testing import login_person, TestCaseWithFactory
145
146@@ -111,5 +111,77 @@
147 self.assertEqual(self.claimant, review.reviewer)
148
149
150+class TestCodeReviewVoteReferenceDelete(TestCaseWithFactory):
151+ """Tests for CodeReviewVoteReference.delete."""
152+
153+ layer = DatabaseFunctionalLayer
154+
155+ def test_delete_pending_by_registrant(self):
156+ # A pending review can be deleted by the person requesting the review.
157+ reviewer = self.factory.makePerson()
158+ bmp = self.factory.makeBranchMergeProposal()
159+ login_person(bmp.registrant)
160+ review = bmp.nominateReviewer(
161+ reviewer=reviewer, registrant=bmp.registrant)
162+ review.delete()
163+ self.assertEqual([], list(bmp.votes))
164+
165+ def test_delete_pending_by_reviewer(self):
166+ # A pending review can be deleted by the person requesting the review.
167+ reviewer = self.factory.makePerson()
168+ bmp = self.factory.makeBranchMergeProposal()
169+ login_person(bmp.registrant)
170+ review = bmp.nominateReviewer(
171+ reviewer=reviewer, registrant=bmp.registrant)
172+ login_person(reviewer)
173+ review.delete()
174+ self.assertEqual([], list(bmp.votes))
175+
176+ def test_delete_pending_by_review_team_member(self):
177+ # A pending review can be deleted by the person requesting the review.
178+ review_team = self.factory.makeTeam()
179+ bmp = self.factory.makeBranchMergeProposal()
180+ login_person(bmp.registrant)
181+ review = bmp.nominateReviewer(
182+ reviewer=review_team, registrant=bmp.registrant)
183+ login_person(review_team.teamowner)
184+ review.delete()
185+ self.assertEqual([], list(bmp.votes))
186+
187+ def test_delete_pending_by_target_branch_owner(self):
188+ # A pending review can be deleted by anyone with edit permissions on
189+ # the target branch.
190+ reviewer = self.factory.makePerson()
191+ bmp = self.factory.makeBranchMergeProposal()
192+ login_person(bmp.registrant)
193+ review = bmp.nominateReviewer(
194+ reviewer=reviewer, registrant=bmp.registrant)
195+ login_person(bmp.target_branch.owner)
196+ review.delete()
197+ self.assertEqual([], list(bmp.votes))
198+
199+ def test_delete_by_others_unauthorized(self):
200+ # A pending review can be deleted by the person requesting the review.
201+ reviewer = self.factory.makePerson()
202+ bmp = self.factory.makeBranchMergeProposal()
203+ login_person(bmp.registrant)
204+ review = bmp.nominateReviewer(
205+ reviewer=reviewer, registrant=bmp.registrant)
206+ login_person(self.factory.makePerson())
207+ self.assertRaises(
208+ Unauthorized, getattr, review, 'delete')
209+
210+ def test_delete_not_pending(self):
211+ # A non-pending review reference cannot be deleted.
212+ reviewer = self.factory.makePerson()
213+ bmp = self.factory.makeBranchMergeProposal()
214+ login_person(reviewer)
215+ bmp.createComment(
216+ reviewer, 'Message subject', 'Message content',
217+ vote=CodeReviewVote.APPROVE)
218+ [review] = list(bmp.votes)
219+ self.assertRaises(ReviewNotPending, review.delete)
220+
221+
222 def test_suite():
223 return TestLoader().loadTestsFromName(__name__)

Subscribers

People subscribed via source and target branches

to status/vote changes: