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
=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py 2009-11-19 15:14:53 +0000
+++ lib/canonical/launchpad/security.py 2009-12-10 20:48:30 +0000
@@ -1676,9 +1676,15 @@
1676 The registrant may reassign the request to another entity.1676 The registrant may reassign the request to another entity.
1677 A member of the review team may assign it to themselves.1677 A member of the review team may assign it to themselves.
1678 A person to whom it is assigned may delegate it to someone else.1678 A person to whom it is assigned may delegate it to someone else.
1679
1680 Anyone with edit permissions on the target branch of the merge
1681 proposal can also edit the reviews.
1679 """1682 """
1680 return (user.inTeam(self.obj.reviewer) or1683 if user.inTeam(self.obj.reviewer) or user.inTeam(self.obj.registrant):
1681 user.inTeam(self.obj.registrant))1684 return True
1685 target_access = EditBranch(
1686 self.obj.branch_merge_proposal.target_branch)
1687 return target_access.checkAuthenticated(user)
16821688
16831689
1684class CodeReviewCommentView(AuthorizationBase):1690class CodeReviewCommentView(AuthorizationBase):
16851691
=== modified file 'lib/lp/code/browser/codereviewvote.py'
--- lib/lp/code/browser/codereviewvote.py 2009-09-10 20:49:25 +0000
+++ lib/lp/code/browser/codereviewvote.py 2009-12-10 20:48:30 +0000
@@ -9,6 +9,7 @@
99
1010
11from zope.interface import Interface11from zope.interface import Interface
12from zope.security.proxy import removeSecurityProxy
1213
13from canonical.launchpad import _14from canonical.launchpad import _
14from canonical.launchpad.fields import PublicPersonChoice15from canonical.launchpad.fields import PublicPersonChoice
@@ -34,5 +35,8 @@
34 @action('Reassign', name='reassign')35 @action('Reassign', name='reassign')
35 def reassign_action(self, action, data):36 def reassign_action(self, action, data):
36 """Use the form data to change the review request reviewer."""37 """Use the form data to change the review request reviewer."""
37 self.context.reviewer = data['reviewer']38 # XXX TimPenhey 2009-12-11 bug=495201
39 # This should check for existing reviews by the reviewer, and have
40 # the logic moved into the model code.
41 removeSecurityProxy(self.context).reviewer = data['reviewer']
38 self.next_url = canonical_url(self.context.branch_merge_proposal)42 self.next_url = canonical_url(self.context.branch_merge_proposal)
3943
=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml 2009-12-07 03:56:18 +0000
+++ lib/lp/code/configure.zcml 2009-12-10 20:48:30 +0000
@@ -33,8 +33,7 @@
33 <allow interface="lp.code.interfaces.codereviewvote.ICodeReviewVoteReferencePublic"/>33 <allow interface="lp.code.interfaces.codereviewvote.ICodeReviewVoteReferencePublic"/>
34 <require 34 <require
35 permission="launchpad.Edit"35 permission="launchpad.Edit"
36 interface="lp.code.interfaces.codereviewvote.ICodeReviewVoteReferenceEdit"36 interface="lp.code.interfaces.codereviewvote.ICodeReviewVoteReferenceEdit"/>
37 set_attributes="reviewer" />
38 </class>37 </class>
39 <subscriber38 <subscriber
40 for="lp.code.interfaces.codereviewvote.ICodeReviewVoteReference39 for="lp.code.interfaces.codereviewvote.ICodeReviewVoteReference
4140
=== modified file 'lib/lp/code/errors.py'
--- lib/lp/code/errors.py 2009-12-07 03:56:18 +0000
+++ lib/lp/code/errors.py 2009-12-10 20:48:30 +0000
@@ -10,6 +10,7 @@
10 'BranchMergeProposalExists',10 'BranchMergeProposalExists',
11 'ClaimReviewFailed',11 'ClaimReviewFailed',
12 'InvalidBranchMergeProposal',12 'InvalidBranchMergeProposal',
13 'ReviewNotPending',
13 'UserNotBranchReviewer',14 'UserNotBranchReviewer',
14 'WrongBranchMergeProposal',15 'WrongBranchMergeProposal',
15]16]
@@ -38,6 +39,10 @@
38 """Raised if there is already a matching BranchMergeProposal."""39 """Raised if there is already a matching BranchMergeProposal."""
3940
4041
42class ReviewNotPending(Exception):
43 """The requested review is not in a pending state."""
44
45
41class UserNotBranchReviewer(Exception):46class UserNotBranchReviewer(Exception):
42 """The user who attempted to review the merge proposal isn't a reviewer.47 """The user who attempted to review the merge proposal isn't a reviewer.
4348
4449
=== modified file 'lib/lp/code/interfaces/codereviewvote.py'
--- lib/lp/code/interfaces/codereviewvote.py 2009-12-07 03:56:18 +0000
+++ lib/lp/code/interfaces/codereviewvote.py 2009-12-10 20:48:30 +0000
@@ -19,8 +19,8 @@
19 ICodeReviewComment)19 ICodeReviewComment)
20from lazr.restful.fields import Reference20from lazr.restful.fields import Reference
21from lazr.restful.declarations import (21from lazr.restful.declarations import (
22 call_with, export_as_webservice_entry, export_write_operation, exported,22 call_with, export_as_webservice_entry, export_destructor_operation,
23 REQUEST_USER)23 export_write_operation, exported, REQUEST_USER)
2424
2525
26class ICodeReviewVoteReferencePublic(Interface):26class ICodeReviewVoteReferencePublic(Interface):
@@ -86,6 +86,13 @@
86 not pending.86 not pending.
87 """87 """
8888
89 @export_destructor_operation()
90 def delete():
91 """Delete the pending review.
92
93 :raises ReviewNotPending: If the review is not pending.
94 """
95
8996
90class ICodeReviewVoteReference(ICodeReviewVoteReferencePublic,97class ICodeReviewVoteReference(ICodeReviewVoteReferencePublic,
91 ICodeReviewVoteReferenceEdit):98 ICodeReviewVoteReferenceEdit):
9299
=== modified file 'lib/lp/code/model/codereviewvote.py'
--- lib/lp/code/model/codereviewvote.py 2009-12-07 03:56:18 +0000
+++ lib/lp/code/model/codereviewvote.py 2009-12-10 20:48:30 +0000
@@ -15,7 +15,7 @@
15from canonical.database.constants import DEFAULT15from canonical.database.constants import DEFAULT
16from canonical.database.datetimecol import UtcDateTimeCol16from canonical.database.datetimecol import UtcDateTimeCol
17from canonical.database.sqlbase import SQLBase17from canonical.database.sqlbase import SQLBase
18from lp.code.errors import ClaimReviewFailed18from lp.code.errors import ClaimReviewFailed, ReviewNotPending
19from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference19from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
2020
2121
@@ -65,3 +65,9 @@
65 raise ClaimReviewFailed(65 raise ClaimReviewFailed(
66 error_str % claimant.unique_displayname)66 error_str % claimant.unique_displayname)
67 self.reviewer = claimant67 self.reviewer = claimant
68
69 def delete(self):
70 """See `ICodeReviewVote`"""
71 if not self.is_pending:
72 raise ReviewNotPending('The review is not pending.')
73 self.destroySelf()
6874
=== modified file 'lib/lp/code/model/tests/test_codereviewvote.py'
--- lib/lp/code/model/tests/test_codereviewvote.py 2009-12-07 03:56:18 +0000
+++ lib/lp/code/model/tests/test_codereviewvote.py 2009-12-10 20:48:30 +0000
@@ -9,7 +9,7 @@
9from canonical.testing import DatabaseFunctionalLayer9from canonical.testing import DatabaseFunctionalLayer
1010
11from lp.code.enums import CodeReviewVote11from lp.code.enums import CodeReviewVote
12from lp.code.errors import ClaimReviewFailed12from lp.code.errors import ClaimReviewFailed, ReviewNotPending
13from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference13from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
14from lp.testing import login_person, TestCaseWithFactory14from lp.testing import login_person, TestCaseWithFactory
1515
@@ -111,5 +111,77 @@
111 self.assertEqual(self.claimant, review.reviewer)111 self.assertEqual(self.claimant, review.reviewer)
112112
113113
114class TestCodeReviewVoteReferenceDelete(TestCaseWithFactory):
115 """Tests for CodeReviewVoteReference.delete."""
116
117 layer = DatabaseFunctionalLayer
118
119 def test_delete_pending_by_registrant(self):
120 # A pending review can be deleted by the person requesting the review.
121 reviewer = self.factory.makePerson()
122 bmp = self.factory.makeBranchMergeProposal()
123 login_person(bmp.registrant)
124 review = bmp.nominateReviewer(
125 reviewer=reviewer, registrant=bmp.registrant)
126 review.delete()
127 self.assertEqual([], list(bmp.votes))
128
129 def test_delete_pending_by_reviewer(self):
130 # A pending review can be deleted by the person requesting the review.
131 reviewer = self.factory.makePerson()
132 bmp = self.factory.makeBranchMergeProposal()
133 login_person(bmp.registrant)
134 review = bmp.nominateReviewer(
135 reviewer=reviewer, registrant=bmp.registrant)
136 login_person(reviewer)
137 review.delete()
138 self.assertEqual([], list(bmp.votes))
139
140 def test_delete_pending_by_review_team_member(self):
141 # A pending review can be deleted by the person requesting the review.
142 review_team = self.factory.makeTeam()
143 bmp = self.factory.makeBranchMergeProposal()
144 login_person(bmp.registrant)
145 review = bmp.nominateReviewer(
146 reviewer=review_team, registrant=bmp.registrant)
147 login_person(review_team.teamowner)
148 review.delete()
149 self.assertEqual([], list(bmp.votes))
150
151 def test_delete_pending_by_target_branch_owner(self):
152 # A pending review can be deleted by anyone with edit permissions on
153 # the target branch.
154 reviewer = self.factory.makePerson()
155 bmp = self.factory.makeBranchMergeProposal()
156 login_person(bmp.registrant)
157 review = bmp.nominateReviewer(
158 reviewer=reviewer, registrant=bmp.registrant)
159 login_person(bmp.target_branch.owner)
160 review.delete()
161 self.assertEqual([], list(bmp.votes))
162
163 def test_delete_by_others_unauthorized(self):
164 # A pending review can be deleted by the person requesting the review.
165 reviewer = self.factory.makePerson()
166 bmp = self.factory.makeBranchMergeProposal()
167 login_person(bmp.registrant)
168 review = bmp.nominateReviewer(
169 reviewer=reviewer, registrant=bmp.registrant)
170 login_person(self.factory.makePerson())
171 self.assertRaises(
172 Unauthorized, getattr, review, 'delete')
173
174 def test_delete_not_pending(self):
175 # A non-pending review reference cannot be deleted.
176 reviewer = self.factory.makePerson()
177 bmp = self.factory.makeBranchMergeProposal()
178 login_person(reviewer)
179 bmp.createComment(
180 reviewer, 'Message subject', 'Message content',
181 vote=CodeReviewVote.APPROVE)
182 [review] = list(bmp.votes)
183 self.assertRaises(ReviewNotPending, review.delete)
184
185
114def test_suite():186def test_suite():
115 return TestLoader().loadTestsFromName(__name__)187 return TestLoader().loadTestsFromName(__name__)

Subscribers

People subscribed via source and target branches

to status/vote changes: