Launchpad itself

Merge lp:~james-w/launchpad/get-requested-reviews into lp:launchpad

  1. get-requested-reviews
  2. Merge into devel
Proposed by James Westby
Status: Merged
Merged at revision: not available
Proposed branch: lp:~james-w/launchpad/get-requested-reviews
Merge into: lp:launchpad
Diff against target: 186 lines (+79/-10)
6 files modified
lib/canonical/launchpad/interfaces/_schema_circular_imports.py (+6/-5)
lib/lp/code/interfaces/hasbranches.py (+31/-0)
lib/lp/code/model/hasbranches.py (+18/-1)
lib/lp/code/stories/webservice/xx-branchmergeproposal.txt (+17/-0)
lib/lp/registry/interfaces/person.py (+4/-2)
lib/lp/registry/model/person.py (+3/-2)
To merge this branch: bzr merge lp:~james-w/launchpad/get-requested-reviews
Reviewer Review Type Date Requested Status
Tim Penhey (community) Approve
Eleanor Berger (community) Needs Fixing
Review via email: mp+18592@code.launchpad.net

Commit message

Export a method on IPerson to get the reviews they have been requested to do.

To post a comment you must log in.
Revision history for this message
James Westby (james-w) wrote :

Hi,

This extends IPerson with a mixin similar to IHasMergeProposals that
doesn't get the owned merge proposals, but those that person has
been requested to review.

Thanks to Barry for his help.

Thanks,

James

Revision history for this message
Eleanor Berger (intellectronica) wrote :

(16:27:14) thekorn: james_w, not that I have review powers, but there is a typo in xx-branchmergeproposal.txt: `Gettting` ;)
(16:28:44) intellectronica: james_w: we don't use line continuation in the launchpad codebase. instead use parentheses to group different constituents of an expression appearing on different lines
(16:29:45) intellectronica: i can see that the same problem appears in an existing lines. no idea why the author of that line decided to format it like that. if you feel like fixing that too, i won't complain :) (but don't feel obliged)
https://code.edge.launchpad.net/launchpad/+activereviews
(16:31:52) intellectronica: james_w: the tuple on line 103 of the diff is formatted a bit funny. why not just put it on one line?
(16:32:56) intellectronica: james_w: also, maybe rename 'collection' in the same function to something more meaningful, like 'user_visible_branches'?
(16:34:08) intellectronica: james_w: using title capitalization, isn't "Getting Merge Proposals a Person has been Asked to Review" the correct form?
jamalta james_w
(16:35:23) intellectronica: james_w: why the final comma on line 129?
(16:35:53) intellectronica: james_w: other than that it all looks good

review: Needs Fixing
Revision history for this message
Tim Penhey (thumper) wrote :

This looks pretty good to me. Thanks James.

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/interfaces/_schema_circular_imports.py'
2--- lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2010-02-01 19:36:23 +0000
3+++ lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2010-02-04 16:54:18 +0000
4@@ -42,7 +42,8 @@
5 from lp.code.interfaces.codereviewcomment import ICodeReviewComment
6 from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
7 from lp.code.interfaces.diff import IPreviewDiff
8-from lp.code.interfaces.hasbranches import IHasBranches, IHasMergeProposals
9+from lp.code.interfaces.hasbranches import (
10+ IHasBranches, IHasMergeProposals, IHasRequestedReviews)
11 from lp.registry.interfaces.distribution import IDistribution
12 from lp.registry.interfaces.distributionmirror import IDistributionMirror
13 from lp.registry.interfaces.distributionsourcepackage import (
14@@ -116,11 +117,11 @@
15 IBranchMergeProposal['votes'].value_type.schema = ICodeReviewVoteReference
16
17 IHasBranches['getBranches'].queryTaggedValue(
18- LAZR_WEBSERVICE_EXPORTED)['return_type'].value_type.schema = \
19- IBranch
20+ LAZR_WEBSERVICE_EXPORTED)['return_type'].value_type.schema = IBranch
21 IHasMergeProposals['getMergeProposals'].queryTaggedValue(
22- LAZR_WEBSERVICE_EXPORTED)['return_type'].value_type.schema = \
23- IBranchMergeProposal
24+ LAZR_WEBSERVICE_EXPORTED)['return_type'].value_type.schema = IBranchMergeProposal
25+IHasRequestedReviews['getRequestedReviews'].queryTaggedValue(
26+ LAZR_WEBSERVICE_EXPORTED)['return_type'].value_type.schema = IBranchMergeProposal
27
28 # IBugTask
29
30
31=== modified file 'lib/lp/code/interfaces/hasbranches.py'
32--- lib/lp/code/interfaces/hasbranches.py 2009-06-30 16:56:07 +0000
33+++ lib/lp/code/interfaces/hasbranches.py 2010-02-04 16:54:18 +0000
34@@ -7,6 +7,7 @@
35 __all__ = [
36 'IHasBranches',
37 'IHasMergeProposals',
38+ 'IHasRequestedReviews',
39 ]
40
41
42@@ -71,3 +72,33 @@
43 :param visible_by_user: Normally the user who is asking.
44 :returns: A list of `IBranchMergeProposal`.
45 """
46+
47+
48+class IHasRequestedReviews(Interface):
49+ """IPersons can have reviews requested of them in merge proposals.
50+
51+ This interface defines the common methods for getting these merge proposals
52+ for a particular person.
53+ """
54+
55+ # In order to minimise dependancies the returns_collection is defined as
56+ # Interface here and defined fully in the circular imports file.
57+
58+ @operation_parameters(
59+ status=List(
60+ title=_("A list of merge proposal statuses to filter by."),
61+ value_type=Choice(vocabulary=BranchMergeProposalStatus)))
62+ @call_with(visible_by_user=REQUEST_USER)
63+ @operation_returns_collection_of(Interface) # Really IBranchMergeProposal.
64+ @export_read_operation()
65+ def getRequestedReviews(status=None, visible_by_user=None):
66+ """Returns merge proposals that a review was requested from the person.
67+
68+ This does not include merge proposals that were requested from
69+ teams that the person is part of. If status is not passed then
70+ it will return proposals that are in the "Needs Review" state.
71+
72+ :param status: A list of statuses to filter with.
73+ :param visible_by_user: Normally the user who is asking.
74+ :returns: A list of `IBranchMergeProposal`.
75+ """
76
77=== modified file 'lib/lp/code/model/hasbranches.py'
78--- lib/lp/code/model/hasbranches.py 2009-06-30 16:56:07 +0000
79+++ lib/lp/code/model/hasbranches.py 2010-02-04 16:54:18 +0000
80@@ -7,12 +7,15 @@
81 __all__ = [
82 'HasBranchesMixin',
83 'HasMergeProposalsMixin',
84+ 'HasRequestedReviewsMixin',
85 ]
86
87+from zope.component import getUtility
88
89 from lp.code.enums import BranchMergeProposalStatus
90 from lp.code.interfaces.branch import DEFAULT_BRANCH_STATUS_IN_LISTING
91-from lp.code.interfaces.branchcollection import IBranchCollection
92+from lp.code.interfaces.branchcollection import (
93+ IAllBranches, IBranchCollection)
94
95
96 class HasBranchesMixin:
97@@ -41,3 +44,17 @@
98
99 collection = IBranchCollection(self).visibleByUser(visible_by_user)
100 return collection.getMergeProposals(status)
101+
102+
103+class HasRequestedReviewsMixin:
104+ """A mixin implementation class for `IHasRequestedReviews`."""
105+
106+ def getRequestedReviews(self, status=None, visible_by_user=None):
107+ """See `IHasRequestedReviews`."""
108+ if not status:
109+ status = (BranchMergeProposalStatus.NEEDS_REVIEW,)
110+
111+ visible_branches = getUtility(IAllBranches).visibleByUser(
112+ visible_by_user)
113+ proposals = visible_branches.getMergeProposalsForPerson(self, status)
114+ return proposals
115
116=== modified file 'lib/lp/code/stories/webservice/xx-branchmergeproposal.txt'
117--- lib/lp/code/stories/webservice/xx-branchmergeproposal.txt 2009-12-06 23:33:54 +0000
118+++ lib/lp/code/stories/webservice/xx-branchmergeproposal.txt 2010-02-04 16:54:18 +0000
119@@ -456,3 +456,20 @@
120 >>> print_proposals(webservice, url='/widgets',
121 ... status=[BranchMergeProposalStatus.REJECTED.title])
122 http://.../~source/fooix/fix-it/+merge/... - Rejected
123+
124+== Getting Merge Proposals a Person has been Asked To Review ==
125+
126+It's good to be able to find out which proposals you have been asked to
127+review.
128+
129+ >>> login('admin@canonical.com')
130+ >>> from lp.code.enums import BranchMergeProposalStatus
131+ >>> fixit_proposal.requestReview()
132+ >>> logout()
133+
134+ >>> proposals = webservice.named_get('/~target', 'getRequestedReviews'
135+ ... ).jsonBody()
136+ >>> for proposal in proposals['entries']:
137+ ... print_proposal(proposal)
138+ http://.../~source/fooix/fix-it/+merge/2 - Needs review
139+
140
141=== modified file 'lib/lp/registry/interfaces/person.py'
142--- lib/lp/registry/interfaces/person.py 2010-01-15 01:37:34 +0000
143+++ lib/lp/registry/interfaces/person.py 2010-02-04 16:54:18 +0000
144@@ -72,7 +72,8 @@
145 from canonical.launchpad.interfaces.account import AccountStatus, IAccount
146 from canonical.launchpad.interfaces.emailaddress import IEmailAddress
147 from lp.app.interfaces.headings import IRootContext
148-from lp.code.interfaces.hasbranches import IHasBranches, IHasMergeProposals
149+from lp.code.interfaces.hasbranches import (
150+ IHasBranches, IHasMergeProposals, IHasRequestedReviews)
151 from lp.registry.interfaces.irc import IIrcID
152 from lp.registry.interfaces.jabber import IJabberID
153 from lp.services.worlddata.interfaces.language import ILanguage
154@@ -476,7 +477,8 @@
155
156 class IPersonPublic(IHasBranches, IHasSpecifications, IHasMentoringOffers,
157 IHasMergeProposals, IHasLogo, IHasMugshot, IHasIcon,
158- IHasLocation, IObjectWithLocation, IPrivacy):
159+ IHasLocation, IHasRequestedReviews, IObjectWithLocation,
160+ IPrivacy):
161 """Public attributes for a Person."""
162
163 id = Int(title=_('ID'), required=True, readonly=True)
164
165=== modified file 'lib/lp/registry/model/person.py'
166--- lib/lp/registry/model/person.py 2010-01-20 22:09:26 +0000
167+++ lib/lp/registry/model/person.py 2010-02-04 16:54:18 +0000
168@@ -81,7 +81,8 @@
169 from lp.soyuz.interfaces.archivepermission import (
170 IArchivePermissionSet)
171 from canonical.launchpad.interfaces.authtoken import LoginTokenType
172-from lp.code.model.hasbranches import HasBranchesMixin, HasMergeProposalsMixin
173+from lp.code.model.hasbranches import (
174+ HasBranchesMixin, HasMergeProposalsMixin, HasRequestedReviewsMixin)
175 from lp.bugs.interfaces.bugtask import (
176 BugTaskSearchParams, IBugTaskSet)
177 from lp.bugs.interfaces.bugtarget import IBugTarget
178@@ -212,7 +213,7 @@
179
180 class Person(
181 SQLBase, HasBugsBase, HasSpecificationsMixin, HasTranslationImportsMixin,
182- HasBranchesMixin, HasMergeProposalsMixin):
183+ HasBranchesMixin, HasMergeProposalsMixin, HasRequestedReviewsMixin):
184 """A Person."""
185
186 implements(IPerson, IHasIcon, IHasLogo, IHasMugshot)