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
=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml 2009-11-16 22:53:42 +0000
+++ lib/lp/code/configure.zcml 2009-12-07 04:01:14 +0000
@@ -28,9 +28,13 @@
28 provides="lp.code.interfaces.branchmergequeue.IBranchMergeQueueSet">28 provides="lp.code.interfaces.branchmergequeue.IBranchMergeQueueSet">
29 <allow interface="lp.code.interfaces.branchmergequeue.IBranchMergeQueueSet"/>29 <allow interface="lp.code.interfaces.branchmergequeue.IBranchMergeQueueSet"/>
30 </securedutility>30 </securedutility>
31
31 <class class="lp.code.model.codereviewvote.CodeReviewVoteReference">32 <class class="lp.code.model.codereviewvote.CodeReviewVoteReference">
32 <allow interface="lp.code.interfaces.codereviewvote.ICodeReviewVoteReference"/>33 <allow interface="lp.code.interfaces.codereviewvote.ICodeReviewVoteReferencePublic"/>
33 <require permission="launchpad.Edit" set_attributes="reviewer" />34 <require
35 permission="launchpad.Edit"
36 interface="lp.code.interfaces.codereviewvote.ICodeReviewVoteReferenceEdit"
37 set_attributes="reviewer" />
34 </class>38 </class>
35 <subscriber39 <subscriber
36 for="lp.code.interfaces.codereviewvote.ICodeReviewVoteReference40 for="lp.code.interfaces.codereviewvote.ICodeReviewVoteReference
@@ -257,8 +261,7 @@
257 updatePreviewDiff"/>261 updatePreviewDiff"/>
258 <require262 <require
259 permission="launchpad.AnyPerson"263 permission="launchpad.AnyPerson"
260 attributes="264 attributes="createComment
261 createComment
262 createCommentFromMessage"/>265 createCommentFromMessage"/>
263 </class>266 </class>
264 <adapter267 <adapter
265268
=== modified file 'lib/lp/code/errors.py'
--- lib/lp/code/errors.py 2009-12-07 04:01:13 +0000
+++ lib/lp/code/errors.py 2009-12-07 04:01:14 +0000
@@ -8,6 +8,7 @@
8 'BadBranchMergeProposalSearchContext',8 'BadBranchMergeProposalSearchContext',
9 'BadStateTransition',9 'BadStateTransition',
10 'BranchMergeProposalExists',10 'BranchMergeProposalExists',
11 'ClaimReviewFailed',
11 'InvalidBranchMergeProposal',12 'InvalidBranchMergeProposal',
12 'UserNotBranchReviewer',13 'UserNotBranchReviewer',
13 'WrongBranchMergeProposal',14 'WrongBranchMergeProposal',
@@ -22,6 +23,10 @@
22 """The user requested a state transition that is not possible."""23 """The user requested a state transition that is not possible."""
2324
2425
26class ClaimReviewFailed(Exception):
27 """The user cannot claim the pending review."""
28
29
25class InvalidBranchMergeProposal(Exception):30class InvalidBranchMergeProposal(Exception):
26 """Raised during the creation of a new branch merge proposal.31 """Raised during the creation of a new branch merge proposal.
2732
@@ -44,5 +49,3 @@
4449
45class WrongBranchMergeProposal(Exception):50class WrongBranchMergeProposal(Exception):
46 """The comment requested is not associated with this merge proposal."""51 """The comment requested is not associated with this merge proposal."""
47
48
4952
=== modified file 'lib/lp/code/interfaces/codereviewvote.py'
--- lib/lp/code/interfaces/codereviewvote.py 2009-10-09 04:15:55 +0000
+++ lib/lp/code/interfaces/codereviewvote.py 2009-12-07 04:01:14 +0000
@@ -9,7 +9,7 @@
9 ]9 ]
1010
11from zope.interface import Interface11from zope.interface import Interface
12from zope.schema import Datetime, Int, TextLine12from zope.schema import Bool, Datetime, Int, TextLine
1313
14from canonical.launchpad import _14from canonical.launchpad import _
15from canonical.launchpad.fields import PublicPersonChoice15from canonical.launchpad.fields import PublicPersonChoice
@@ -19,16 +19,12 @@
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 export_as_webservice_entry, exported)22 call_with, export_as_webservice_entry, export_write_operation, exported,
2323 REQUEST_USER)
2424
25class ICodeReviewVoteReference(Interface):25
26 """A reference to a vote on a IBranchMergeProposal.26class ICodeReviewVoteReferencePublic(Interface):
2727 """The public attributes for code review vote references."""
28 There is at most one reference to a vote for each reviewer on a given
29 branch merge proposal.
30 """
31 export_as_webservice_entry()
3228
33 id = Int(29 id = Int(
34 title=_("The ID of the vote reference"))30 title=_("The ID of the vote reference"))
@@ -66,3 +62,37 @@
66 "The code review comment that contains the most recent vote."62 "The code review comment that contains the most recent vote."
67 ),63 ),
68 required=True, schema=ICodeReviewComment))64 required=True, schema=ICodeReviewComment))
65
66 is_pending = exported(
67 Bool(title=_("Is the pending?"), required=True, readonly=True))
68
69
70class ICodeReviewVoteReferenceEdit(Interface):
71 """Method that require edit permissions."""
72
73 @call_with(claimant=REQUEST_USER)
74 @export_write_operation()
75 def claimReview(claimant):
76 """Change a pending review into a review for claimant.
77
78 Pending team reviews can be claimed by members of that team. This
79 allows reviews to be moved of the general team todo list, and onto a
80 personal todo list.
81
82 :param claimant: The person claiming the team review.
83 :raises ClaimReviewFailed: If the claimant already has a
84 personal review, if the reviewer is not a team, if the
85 claimant is not in the reviewer team, or if the review is
86 not pending.
87 """
88
89
90class ICodeReviewVoteReference(ICodeReviewVoteReferencePublic,
91 ICodeReviewVoteReferenceEdit):
92 """A reference to a vote on a IBranchMergeProposal.
93
94 There is at most one reference to a vote for each reviewer on a given
95 branch merge proposal.
96 """
97
98 export_as_webservice_entry()
6999
=== modified file 'lib/lp/code/model/codereviewvote.py'
--- lib/lp/code/model/codereviewvote.py 2009-06-25 04:06:00 +0000
+++ lib/lp/code/model/codereviewvote.py 2009-12-07 04:01:14 +0000
@@ -15,6 +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 ClaimReviewFailed
18from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference19from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
1920
2021
@@ -36,3 +37,31 @@
36 review_type = StringCol(default=None)37 review_type = StringCol(default=None)
37 comment = ForeignKey(38 comment = ForeignKey(
38 dbName='vote_message', foreignKey='CodeReviewComment', default=None)39 dbName='vote_message', foreignKey='CodeReviewComment', default=None)
40
41 @property
42 def is_pending(self):
43 """See `ICodeReviewVote`"""
44 # Reviews are pending if there is no associated comment.
45 return self.comment is None
46
47 def claimReview(self, claimant):
48 """See `ICodeReviewVote`"""
49 if not self.is_pending:
50 raise ClaimReviewFailed('The review is not pending.')
51 if not self.reviewer.is_team:
52 raise ClaimReviewFailed('Cannot claim non-team reviews.')
53 if not claimant.inTeam(self.reviewer):
54 raise ClaimReviewFailed(
55 '%s is not a member of %s' %
56 (claimant.unique_displayname,
57 self.reviewer.unique_displayname))
58 claimant_review = (
59 self.branch_merge_proposal.getUsersVoteReference(claimant))
60 if claimant_review is not None:
61 if claimant_review.is_pending:
62 error_str = '%s has an existing pending review'
63 else:
64 error_str = '%s has an existing personal review'
65 raise ClaimReviewFailed(
66 error_str % claimant.unique_displayname)
67 self.reviewer = claimant
3968
=== modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
--- lib/lp/code/model/tests/test_branchmergeproposals.py 2009-12-07 04:01:13 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposals.py 2009-12-07 04:01:14 +0000
@@ -1683,6 +1683,7 @@
1683 # Still only one vote.1683 # Still only one vote.
1684 self.assertEqual(1, len(list(merge_proposal.votes)))1684 self.assertEqual(1, len(list(merge_proposal.votes)))
16851685
1686
1686class TestBranchMergeProposalResubmit(TestCaseWithFactory):1687class TestBranchMergeProposalResubmit(TestCaseWithFactory):
16871688
1688 layer = DatabaseFunctionalLayer1689 layer = DatabaseFunctionalLayer
16891690
=== modified file 'lib/lp/code/model/tests/test_codereviewvote.py'
--- lib/lp/code/model/tests/test_codereviewvote.py 2009-06-25 04:06:00 +0000
+++ lib/lp/code/model/tests/test_codereviewvote.py 2009-12-07 04:01:14 +0000
@@ -3,30 +3,112 @@
33
4from unittest import TestLoader4from unittest import TestLoader
55
6from zope.security.interfaces import Unauthorized
7
6from canonical.database.constants import UTC_NOW8from canonical.database.constants import UTC_NOW
9from canonical.testing import DatabaseFunctionalLayer
10
11from lp.code.enums import CodeReviewVote
12from lp.code.errors import ClaimReviewFailed
7from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference13from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
8from lp.testing import TestCaseWithFactory14from lp.testing import login_person, TestCaseWithFactory
9from canonical.launchpad.webapp.testing import verifyObject15
10from canonical.testing import LaunchpadZopelessLayer
1116
12class TestCodeReviewVote(TestCaseWithFactory):17class TestCodeReviewVote(TestCaseWithFactory):
1318
14 layer = LaunchpadZopelessLayer19 layer = DatabaseFunctionalLayer
1520
16 def test_create_vote(self):21 def test_create_vote(self):
17 """CodeReviewVotes can be created"""22 """CodeReviewVotes can be created"""
18 merge_proposal = self.factory.makeBranchMergeProposal()23 merge_proposal = self.factory.makeBranchMergeProposal()
19 reviewer = self.factory.makePerson()24 reviewer = self.factory.makePerson()
20 registrant = self.factory.makePerson()25 login_person(merge_proposal.registrant)
21 vote = merge_proposal.nominateReviewer(reviewer, registrant)26 vote = merge_proposal.nominateReviewer(
27 reviewer, merge_proposal.registrant)
22 self.assertEqual(reviewer, vote.reviewer)28 self.assertEqual(reviewer, vote.reviewer)
23 self.assertEqual(registrant, vote.registrant)29 self.assertEqual(merge_proposal.registrant, vote.registrant)
24 self.assertEqual(merge_proposal, vote.branch_merge_proposal)30 self.assertEqual(merge_proposal, vote.branch_merge_proposal)
25 self.assertEqual([vote], list(merge_proposal.votes))31 self.assertEqual([vote], list(merge_proposal.votes))
26 self.assertSqlAttributeEqualsDate(32 self.assertSqlAttributeEqualsDate(
27 vote, 'date_created', UTC_NOW)33 vote, 'date_created', UTC_NOW)
28 assert verifyObject(ICodeReviewVoteReference, vote), ('Implements the'34 self.assertProvides(vote, ICodeReviewVoteReference)
29 ' expected interface.')35
36
37class TestCodeReviewVoteReferenceClaimReview(TestCaseWithFactory):
38 """Tests for CodeReviewVoteReference.claimReview."""
39
40 layer = DatabaseFunctionalLayer
41
42 def setUp(self):
43 TestCaseWithFactory.setUp(self)
44 # Setup the proposal, claimant and team reviewer.
45 self.bmp = self.factory.makeBranchMergeProposal()
46 self.claimant = self.factory.makePerson()
47 self.review_team = self.factory.makeTeam()
48
49 def _addPendingReview(self):
50 """Add a pending review for the review_team."""
51 login_person(self.bmp.registrant)
52 return self.bmp.nominateReviewer(
53 reviewer=self.review_team,
54 registrant=self.bmp.registrant)
55
56 def _addClaimantToReviewTeam(self):
57 """Add the claimant to the review team."""
58 login_person(self.review_team.teamowner)
59 self.review_team.addMember(
60 person=self.claimant, reviewer=self.review_team.teamowner)
61
62 def test_personal_completed_review(self):
63 # If the claimant has a personal review already, then they can't claim
64 # a pending team review.
65 login_person(self.claimant)
66 # Make sure that the personal review is done before the pending team
67 # review, otherwise the pending team review will be claimed by this
68 # one.
69 self.bmp.createComment(
70 self.claimant, 'Message subject', 'Message content',
71 vote=CodeReviewVote.APPROVE)
72 review = self._addPendingReview()
73 self._addClaimantToReviewTeam()
74 self.assertRaises(
75 ClaimReviewFailed, review.claimReview, self.claimant)
76
77 def test_personal_pending_review(self):
78 # If the claimant has a pending review already, then they can't claim
79 # a pending team review.
80 review = self._addPendingReview()
81 self._addClaimantToReviewTeam()
82 login_person(self.bmp.registrant)
83 self.bmp.nominateReviewer(
84 reviewer=self.claimant, registrant=self.bmp.registrant)
85 login_person(self.claimant)
86 self.assertRaises(
87 ClaimReviewFailed, review.claimReview, self.claimant)
88
89 def test_personal_not_in_review_team(self):
90 # If the claimant is not in the review team, an error is raised.
91 review = self._addPendingReview()
92 # Since the claimant isn't in the review team, they don't have
93 # launchpad.Edit on the review itself, hence Unauthorized.
94 login_person(self.claimant)
95 # Actually accessing claimReview triggers the security proxy.
96 self.assertRaises(
97 Unauthorized, getattr, review, 'claimReview')
98 # The merge proposal registrant however does have edit permissions,
99 # but isn't in the team, so they get ClaimReviewFailed.
100 login_person(self.bmp.registrant)
101 self.assertRaises(
102 ClaimReviewFailed, review.claimReview, self.bmp.registrant)
103
104 def test_success(self):
105 # If the claimant is in the review team, and does not have a personal
106 # review, pending or completed, then they can claim the team review.
107 review = self._addPendingReview()
108 self._addClaimantToReviewTeam()
109 login_person(self.claimant)
110 review.claimReview(self.claimant)
111 self.assertEqual(self.claimant, review.reviewer)
30112
31113
32def test_suite():114def test_suite():
33115
=== modified file 'lib/lp/code/stories/webservice/xx-branchmergeproposal.txt'
--- lib/lp/code/stories/webservice/xx-branchmergeproposal.txt 2009-11-05 21:36:14 +0000
+++ lib/lp/code/stories/webservice/xx-branchmergeproposal.txt 2009-12-07 04:01:14 +0000
@@ -79,6 +79,7 @@
79 branch_merge_proposal_link: u'http://api.launchpad.dev/beta/~.../+merge/...'79 branch_merge_proposal_link: u'http://api.launchpad.dev/beta/~.../+merge/...'
80 comment_link: None80 comment_link: None
81 date_created: u'...'81 date_created: u'...'
82 is_pending: True
82 registrant_link: u'http://api.launchpad.dev/beta/~person-name...'83 registrant_link: u'http://api.launchpad.dev/beta/~person-name...'
83 resource_type_link: u'http://api.launchpad.dev/beta/#code_review_vote_reference'84 resource_type_link: u'http://api.launchpad.dev/beta/#code_review_vote_reference'
84 review_type: u'green'85 review_type: u'green'
@@ -194,6 +195,7 @@
194 branch_merge_proposal_link: u'http://.../~source/fooix/fix-it/+merge/...'195 branch_merge_proposal_link: u'http://.../~source/fooix/fix-it/+merge/...'
195 comment_link: u'http://.../~source/fooix/fix-it/+merge/.../comments/...'196 comment_link: u'http://.../~source/fooix/fix-it/+merge/.../comments/...'
196 date_created: u'...'197 date_created: u'...'
198 is_pending: False
197 registrant_link: u'http://.../~person-name...'199 registrant_link: u'http://.../~person-name...'
198 resource_type_link: u'http://.../#code_review_vote_reference'200 resource_type_link: u'http://.../#code_review_vote_reference'
199 review_type: u'code'201 review_type: u'code'
@@ -221,6 +223,7 @@
221 branch_merge_proposal_link: u'http://.../~source/fooix/fix-it/+merge/...'223 branch_merge_proposal_link: u'http://.../~source/fooix/fix-it/+merge/...'
222 comment_link: None224 comment_link: None
223 date_created: u'...'225 date_created: u'...'
226 is_pending: True
224 registrant_link: u'http://.../~target'227 registrant_link: u'http://.../~target'
225 resource_type_link: u'http://.../#code_review_vote_reference'228 resource_type_link: u'http://.../#code_review_vote_reference'
226 review_type: u'code'229 review_type: u'code'

Subscribers

People subscribed via source and target branches

to status/vote changes: