Launchpad itself

Merge lp:~thumper/launchpad/claim-review-into-model-attempt2 into lp:launchpad/db-devel

  1. claim-review-into-model-attempt2
  2. Merge into db-devel
Proposed by Tim Penhey
Status: Merged
Merged at revision: 8763
Proposed branch: lp:~thumper/launchpad/claim-review-into-model-attempt2
Merge into: lp:launchpad/db-devel
Prerequisite: lp:~thumper/launchpad/lp-code-errors
Diff against target: 342 lines (+177/-26)
7 files modified
lib/lp/code/configure.zcml (+7/-4)
lib/lp/code/errors.py (+5/-2)
lib/lp/code/interfaces/codereviewvote.py (+41/-11)
lib/lp/code/model/codereviewvote.py (+29/-0)
lib/lp/code/model/tests/test_branchmergeproposals.py (+1/-0)
lib/lp/code/model/tests/test_codereviewvote.py (+91/-9)
lib/lp/code/stories/webservice/xx-branchmergeproposal.txt (+3/-0)
To merge this branch: bzr merge lp:~thumper/launchpad/claim-review-into-model-attempt2
Reviewer Review Type Date Requested Status
Paul Hummer (community) Approve
Review via email: mp+15728@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

Moves the logic of actually claiming a review into the model code itself and exposes it over the api.

tests:
  model.tests.test_codereviewvote

and for the is_pending bit:
  webservice/xx-branchmergeproposal

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/lp/code/configure.zcml'
2--- lib/lp/code/configure.zcml 2009-11-16 22:53:42 +0000
3+++ lib/lp/code/configure.zcml 2009-12-07 04:01:14 +0000
4@@ -28,9 +28,13 @@
5 provides="lp.code.interfaces.branchmergequeue.IBranchMergeQueueSet">
6 <allow interface="lp.code.interfaces.branchmergequeue.IBranchMergeQueueSet"/>
7 </securedutility>
8+
9 <class class="lp.code.model.codereviewvote.CodeReviewVoteReference">
10- <allow interface="lp.code.interfaces.codereviewvote.ICodeReviewVoteReference"/>
11- <require permission="launchpad.Edit" set_attributes="reviewer" />
12+ <allow interface="lp.code.interfaces.codereviewvote.ICodeReviewVoteReferencePublic"/>
13+ <require
14+ permission="launchpad.Edit"
15+ interface="lp.code.interfaces.codereviewvote.ICodeReviewVoteReferenceEdit"
16+ set_attributes="reviewer" />
17 </class>
18 <subscriber
19 for="lp.code.interfaces.codereviewvote.ICodeReviewVoteReference
20@@ -257,8 +261,7 @@
21 updatePreviewDiff"/>
22 <require
23 permission="launchpad.AnyPerson"
24- attributes="
25- createComment
26+ attributes="createComment
27 createCommentFromMessage"/>
28 </class>
29 <adapter
30
31=== modified file 'lib/lp/code/errors.py'
32--- lib/lp/code/errors.py 2009-12-07 04:01:13 +0000
33+++ lib/lp/code/errors.py 2009-12-07 04:01:14 +0000
34@@ -8,6 +8,7 @@
35 'BadBranchMergeProposalSearchContext',
36 'BadStateTransition',
37 'BranchMergeProposalExists',
38+ 'ClaimReviewFailed',
39 'InvalidBranchMergeProposal',
40 'UserNotBranchReviewer',
41 'WrongBranchMergeProposal',
42@@ -22,6 +23,10 @@
43 """The user requested a state transition that is not possible."""
44
45
46+class ClaimReviewFailed(Exception):
47+ """The user cannot claim the pending review."""
48+
49+
50 class InvalidBranchMergeProposal(Exception):
51 """Raised during the creation of a new branch merge proposal.
52
53@@ -44,5 +49,3 @@
54
55 class WrongBranchMergeProposal(Exception):
56 """The comment requested is not associated with this merge proposal."""
57-
58-
59
60=== modified file 'lib/lp/code/interfaces/codereviewvote.py'
61--- lib/lp/code/interfaces/codereviewvote.py 2009-10-09 04:15:55 +0000
62+++ lib/lp/code/interfaces/codereviewvote.py 2009-12-07 04:01:14 +0000
63@@ -9,7 +9,7 @@
64 ]
65
66 from zope.interface import Interface
67-from zope.schema import Datetime, Int, TextLine
68+from zope.schema import Bool, Datetime, Int, TextLine
69
70 from canonical.launchpad import _
71 from canonical.launchpad.fields import PublicPersonChoice
72@@ -19,16 +19,12 @@
73 ICodeReviewComment)
74 from lazr.restful.fields import Reference
75 from lazr.restful.declarations import (
76- export_as_webservice_entry, exported)
77-
78-
79-class ICodeReviewVoteReference(Interface):
80- """A reference to a vote on a IBranchMergeProposal.
81-
82- There is at most one reference to a vote for each reviewer on a given
83- branch merge proposal.
84- """
85- export_as_webservice_entry()
86+ call_with, export_as_webservice_entry, export_write_operation, exported,
87+ REQUEST_USER)
88+
89+
90+class ICodeReviewVoteReferencePublic(Interface):
91+ """The public attributes for code review vote references."""
92
93 id = Int(
94 title=_("The ID of the vote reference"))
95@@ -66,3 +62,37 @@
96 "The code review comment that contains the most recent vote."
97 ),
98 required=True, schema=ICodeReviewComment))
99+
100+ is_pending = exported(
101+ Bool(title=_("Is the pending?"), required=True, readonly=True))
102+
103+
104+class ICodeReviewVoteReferenceEdit(Interface):
105+ """Method that require edit permissions."""
106+
107+ @call_with(claimant=REQUEST_USER)
108+ @export_write_operation()
109+ def claimReview(claimant):
110+ """Change a pending review into a review for claimant.
111+
112+ Pending team reviews can be claimed by members of that team. This
113+ allows reviews to be moved of the general team todo list, and onto a
114+ personal todo list.
115+
116+ :param claimant: The person claiming the team review.
117+ :raises ClaimReviewFailed: If the claimant already has a
118+ personal review, if the reviewer is not a team, if the
119+ claimant is not in the reviewer team, or if the review is
120+ not pending.
121+ """
122+
123+
124+class ICodeReviewVoteReference(ICodeReviewVoteReferencePublic,
125+ ICodeReviewVoteReferenceEdit):
126+ """A reference to a vote on a IBranchMergeProposal.
127+
128+ There is at most one reference to a vote for each reviewer on a given
129+ branch merge proposal.
130+ """
131+
132+ export_as_webservice_entry()
133
134=== modified file 'lib/lp/code/model/codereviewvote.py'
135--- lib/lp/code/model/codereviewvote.py 2009-06-25 04:06:00 +0000
136+++ lib/lp/code/model/codereviewvote.py 2009-12-07 04:01:14 +0000
137@@ -15,6 +15,7 @@
138 from canonical.database.constants import DEFAULT
139 from canonical.database.datetimecol import UtcDateTimeCol
140 from canonical.database.sqlbase import SQLBase
141+from lp.code.errors import ClaimReviewFailed
142 from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
143
144
145@@ -36,3 +37,31 @@
146 review_type = StringCol(default=None)
147 comment = ForeignKey(
148 dbName='vote_message', foreignKey='CodeReviewComment', default=None)
149+
150+ @property
151+ def is_pending(self):
152+ """See `ICodeReviewVote`"""
153+ # Reviews are pending if there is no associated comment.
154+ return self.comment is None
155+
156+ def claimReview(self, claimant):
157+ """See `ICodeReviewVote`"""
158+ if not self.is_pending:
159+ raise ClaimReviewFailed('The review is not pending.')
160+ if not self.reviewer.is_team:
161+ raise ClaimReviewFailed('Cannot claim non-team reviews.')
162+ if not claimant.inTeam(self.reviewer):
163+ raise ClaimReviewFailed(
164+ '%s is not a member of %s' %
165+ (claimant.unique_displayname,
166+ self.reviewer.unique_displayname))
167+ claimant_review = (
168+ self.branch_merge_proposal.getUsersVoteReference(claimant))
169+ if claimant_review is not None:
170+ if claimant_review.is_pending:
171+ error_str = '%s has an existing pending review'
172+ else:
173+ error_str = '%s has an existing personal review'
174+ raise ClaimReviewFailed(
175+ error_str % claimant.unique_displayname)
176+ self.reviewer = claimant
177
178=== modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
179--- lib/lp/code/model/tests/test_branchmergeproposals.py 2009-12-07 04:01:13 +0000
180+++ lib/lp/code/model/tests/test_branchmergeproposals.py 2009-12-07 04:01:14 +0000
181@@ -1683,6 +1683,7 @@
182 # Still only one vote.
183 self.assertEqual(1, len(list(merge_proposal.votes)))
184
185+
186 class TestBranchMergeProposalResubmit(TestCaseWithFactory):
187
188 layer = DatabaseFunctionalLayer
189
190=== modified file 'lib/lp/code/model/tests/test_codereviewvote.py'
191--- lib/lp/code/model/tests/test_codereviewvote.py 2009-06-25 04:06:00 +0000
192+++ lib/lp/code/model/tests/test_codereviewvote.py 2009-12-07 04:01:14 +0000
193@@ -3,30 +3,112 @@
194
195 from unittest import TestLoader
196
197+from zope.security.interfaces import Unauthorized
198+
199 from canonical.database.constants import UTC_NOW
200+from canonical.testing import DatabaseFunctionalLayer
201+
202+from lp.code.enums import CodeReviewVote
203+from lp.code.errors import ClaimReviewFailed
204 from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
205-from lp.testing import TestCaseWithFactory
206-from canonical.launchpad.webapp.testing import verifyObject
207-from canonical.testing import LaunchpadZopelessLayer
208+from lp.testing import login_person, TestCaseWithFactory
209+
210
211 class TestCodeReviewVote(TestCaseWithFactory):
212
213- layer = LaunchpadZopelessLayer
214+ layer = DatabaseFunctionalLayer
215
216 def test_create_vote(self):
217 """CodeReviewVotes can be created"""
218 merge_proposal = self.factory.makeBranchMergeProposal()
219 reviewer = self.factory.makePerson()
220- registrant = self.factory.makePerson()
221- vote = merge_proposal.nominateReviewer(reviewer, registrant)
222+ login_person(merge_proposal.registrant)
223+ vote = merge_proposal.nominateReviewer(
224+ reviewer, merge_proposal.registrant)
225 self.assertEqual(reviewer, vote.reviewer)
226- self.assertEqual(registrant, vote.registrant)
227+ self.assertEqual(merge_proposal.registrant, vote.registrant)
228 self.assertEqual(merge_proposal, vote.branch_merge_proposal)
229 self.assertEqual([vote], list(merge_proposal.votes))
230 self.assertSqlAttributeEqualsDate(
231 vote, 'date_created', UTC_NOW)
232- assert verifyObject(ICodeReviewVoteReference, vote), ('Implements the'
233- ' expected interface.')
234+ self.assertProvides(vote, ICodeReviewVoteReference)
235+
236+
237+class TestCodeReviewVoteReferenceClaimReview(TestCaseWithFactory):
238+ """Tests for CodeReviewVoteReference.claimReview."""
239+
240+ layer = DatabaseFunctionalLayer
241+
242+ def setUp(self):
243+ TestCaseWithFactory.setUp(self)
244+ # Setup the proposal, claimant and team reviewer.
245+ self.bmp = self.factory.makeBranchMergeProposal()
246+ self.claimant = self.factory.makePerson()
247+ self.review_team = self.factory.makeTeam()
248+
249+ def _addPendingReview(self):
250+ """Add a pending review for the review_team."""
251+ login_person(self.bmp.registrant)
252+ return self.bmp.nominateReviewer(
253+ reviewer=self.review_team,
254+ registrant=self.bmp.registrant)
255+
256+ def _addClaimantToReviewTeam(self):
257+ """Add the claimant to the review team."""
258+ login_person(self.review_team.teamowner)
259+ self.review_team.addMember(
260+ person=self.claimant, reviewer=self.review_team.teamowner)
261+
262+ def test_personal_completed_review(self):
263+ # If the claimant has a personal review already, then they can't claim
264+ # a pending team review.
265+ login_person(self.claimant)
266+ # Make sure that the personal review is done before the pending team
267+ # review, otherwise the pending team review will be claimed by this
268+ # one.
269+ self.bmp.createComment(
270+ self.claimant, 'Message subject', 'Message content',
271+ vote=CodeReviewVote.APPROVE)
272+ review = self._addPendingReview()
273+ self._addClaimantToReviewTeam()
274+ self.assertRaises(
275+ ClaimReviewFailed, review.claimReview, self.claimant)
276+
277+ def test_personal_pending_review(self):
278+ # If the claimant has a pending review already, then they can't claim
279+ # a pending team review.
280+ review = self._addPendingReview()
281+ self._addClaimantToReviewTeam()
282+ login_person(self.bmp.registrant)
283+ self.bmp.nominateReviewer(
284+ reviewer=self.claimant, registrant=self.bmp.registrant)
285+ login_person(self.claimant)
286+ self.assertRaises(
287+ ClaimReviewFailed, review.claimReview, self.claimant)
288+
289+ def test_personal_not_in_review_team(self):
290+ # If the claimant is not in the review team, an error is raised.
291+ review = self._addPendingReview()
292+ # Since the claimant isn't in the review team, they don't have
293+ # launchpad.Edit on the review itself, hence Unauthorized.
294+ login_person(self.claimant)
295+ # Actually accessing claimReview triggers the security proxy.
296+ self.assertRaises(
297+ Unauthorized, getattr, review, 'claimReview')
298+ # The merge proposal registrant however does have edit permissions,
299+ # but isn't in the team, so they get ClaimReviewFailed.
300+ login_person(self.bmp.registrant)
301+ self.assertRaises(
302+ ClaimReviewFailed, review.claimReview, self.bmp.registrant)
303+
304+ def test_success(self):
305+ # If the claimant is in the review team, and does not have a personal
306+ # review, pending or completed, then they can claim the team review.
307+ review = self._addPendingReview()
308+ self._addClaimantToReviewTeam()
309+ login_person(self.claimant)
310+ review.claimReview(self.claimant)
311+ self.assertEqual(self.claimant, review.reviewer)
312
313
314 def test_suite():
315
316=== modified file 'lib/lp/code/stories/webservice/xx-branchmergeproposal.txt'
317--- lib/lp/code/stories/webservice/xx-branchmergeproposal.txt 2009-11-05 21:36:14 +0000
318+++ lib/lp/code/stories/webservice/xx-branchmergeproposal.txt 2009-12-07 04:01:14 +0000
319@@ -79,6 +79,7 @@
320 branch_merge_proposal_link: u'http://api.launchpad.dev/beta/~.../+merge/...'
321 comment_link: None
322 date_created: u'...'
323+ is_pending: True
324 registrant_link: u'http://api.launchpad.dev/beta/~person-name...'
325 resource_type_link: u'http://api.launchpad.dev/beta/#code_review_vote_reference'
326 review_type: u'green'
327@@ -194,6 +195,7 @@
328 branch_merge_proposal_link: u'http://.../~source/fooix/fix-it/+merge/...'
329 comment_link: u'http://.../~source/fooix/fix-it/+merge/.../comments/...'
330 date_created: u'...'
331+ is_pending: False
332 registrant_link: u'http://.../~person-name...'
333 resource_type_link: u'http://.../#code_review_vote_reference'
334 review_type: u'code'
335@@ -221,6 +223,7 @@
336 branch_merge_proposal_link: u'http://.../~source/fooix/fix-it/+merge/...'
337 comment_link: None
338 date_created: u'...'
339+ is_pending: True
340 registrant_link: u'http://.../~target'
341 resource_type_link: u'http://.../#code_review_vote_reference'
342 review_type: u'code'

Subscribers

People subscribed via source and target branches

to status/vote changes: