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
=== modified file 'lib/canonical/launchpad/interfaces/_schema_circular_imports.py'
--- lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2010-02-01 19:36:23 +0000
+++ lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2010-02-04 16:54:18 +0000
@@ -42,7 +42,8 @@
42from lp.code.interfaces.codereviewcomment import ICodeReviewComment42from lp.code.interfaces.codereviewcomment import ICodeReviewComment
43from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference43from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
44from lp.code.interfaces.diff import IPreviewDiff44from lp.code.interfaces.diff import IPreviewDiff
45from lp.code.interfaces.hasbranches import IHasBranches, IHasMergeProposals45from lp.code.interfaces.hasbranches import (
46 IHasBranches, IHasMergeProposals, IHasRequestedReviews)
46from lp.registry.interfaces.distribution import IDistribution47from lp.registry.interfaces.distribution import IDistribution
47from lp.registry.interfaces.distributionmirror import IDistributionMirror48from lp.registry.interfaces.distributionmirror import IDistributionMirror
48from lp.registry.interfaces.distributionsourcepackage import (49from lp.registry.interfaces.distributionsourcepackage import (
@@ -116,11 +117,11 @@
116IBranchMergeProposal['votes'].value_type.schema = ICodeReviewVoteReference117IBranchMergeProposal['votes'].value_type.schema = ICodeReviewVoteReference
117118
118IHasBranches['getBranches'].queryTaggedValue(119IHasBranches['getBranches'].queryTaggedValue(
119 LAZR_WEBSERVICE_EXPORTED)['return_type'].value_type.schema = \120 LAZR_WEBSERVICE_EXPORTED)['return_type'].value_type.schema = IBranch
120 IBranch
121IHasMergeProposals['getMergeProposals'].queryTaggedValue(121IHasMergeProposals['getMergeProposals'].queryTaggedValue(
122 LAZR_WEBSERVICE_EXPORTED)['return_type'].value_type.schema = \122 LAZR_WEBSERVICE_EXPORTED)['return_type'].value_type.schema = IBranchMergeProposal
123 IBranchMergeProposal123IHasRequestedReviews['getRequestedReviews'].queryTaggedValue(
124 LAZR_WEBSERVICE_EXPORTED)['return_type'].value_type.schema = IBranchMergeProposal
124125
125# IBugTask126# IBugTask
126127
127128
=== modified file 'lib/lp/code/interfaces/hasbranches.py'
--- lib/lp/code/interfaces/hasbranches.py 2009-06-30 16:56:07 +0000
+++ lib/lp/code/interfaces/hasbranches.py 2010-02-04 16:54:18 +0000
@@ -7,6 +7,7 @@
7__all__ = [7__all__ = [
8 'IHasBranches',8 'IHasBranches',
9 'IHasMergeProposals',9 'IHasMergeProposals',
10 'IHasRequestedReviews',
10 ]11 ]
1112
1213
@@ -71,3 +72,33 @@
71 :param visible_by_user: Normally the user who is asking.72 :param visible_by_user: Normally the user who is asking.
72 :returns: A list of `IBranchMergeProposal`.73 :returns: A list of `IBranchMergeProposal`.
73 """74 """
75
76
77class IHasRequestedReviews(Interface):
78 """IPersons can have reviews requested of them in merge proposals.
79
80 This interface defines the common methods for getting these merge proposals
81 for a particular person.
82 """
83
84 # In order to minimise dependancies the returns_collection is defined as
85 # Interface here and defined fully in the circular imports file.
86
87 @operation_parameters(
88 status=List(
89 title=_("A list of merge proposal statuses to filter by."),
90 value_type=Choice(vocabulary=BranchMergeProposalStatus)))
91 @call_with(visible_by_user=REQUEST_USER)
92 @operation_returns_collection_of(Interface) # Really IBranchMergeProposal.
93 @export_read_operation()
94 def getRequestedReviews(status=None, visible_by_user=None):
95 """Returns merge proposals that a review was requested from the person.
96
97 This does not include merge proposals that were requested from
98 teams that the person is part of. If status is not passed then
99 it will return proposals that are in the "Needs Review" state.
100
101 :param status: A list of statuses to filter with.
102 :param visible_by_user: Normally the user who is asking.
103 :returns: A list of `IBranchMergeProposal`.
104 """
74105
=== modified file 'lib/lp/code/model/hasbranches.py'
--- lib/lp/code/model/hasbranches.py 2009-06-30 16:56:07 +0000
+++ lib/lp/code/model/hasbranches.py 2010-02-04 16:54:18 +0000
@@ -7,12 +7,15 @@
7__all__ = [7__all__ = [
8 'HasBranchesMixin',8 'HasBranchesMixin',
9 'HasMergeProposalsMixin',9 'HasMergeProposalsMixin',
10 'HasRequestedReviewsMixin',
10 ]11 ]
1112
13from zope.component import getUtility
1214
13from lp.code.enums import BranchMergeProposalStatus15from lp.code.enums import BranchMergeProposalStatus
14from lp.code.interfaces.branch import DEFAULT_BRANCH_STATUS_IN_LISTING16from lp.code.interfaces.branch import DEFAULT_BRANCH_STATUS_IN_LISTING
15from lp.code.interfaces.branchcollection import IBranchCollection17from lp.code.interfaces.branchcollection import (
18 IAllBranches, IBranchCollection)
1619
1720
18class HasBranchesMixin:21class HasBranchesMixin:
@@ -41,3 +44,17 @@
4144
42 collection = IBranchCollection(self).visibleByUser(visible_by_user)45 collection = IBranchCollection(self).visibleByUser(visible_by_user)
43 return collection.getMergeProposals(status)46 return collection.getMergeProposals(status)
47
48
49class HasRequestedReviewsMixin:
50 """A mixin implementation class for `IHasRequestedReviews`."""
51
52 def getRequestedReviews(self, status=None, visible_by_user=None):
53 """See `IHasRequestedReviews`."""
54 if not status:
55 status = (BranchMergeProposalStatus.NEEDS_REVIEW,)
56
57 visible_branches = getUtility(IAllBranches).visibleByUser(
58 visible_by_user)
59 proposals = visible_branches.getMergeProposalsForPerson(self, status)
60 return proposals
4461
=== modified file 'lib/lp/code/stories/webservice/xx-branchmergeproposal.txt'
--- lib/lp/code/stories/webservice/xx-branchmergeproposal.txt 2009-12-06 23:33:54 +0000
+++ lib/lp/code/stories/webservice/xx-branchmergeproposal.txt 2010-02-04 16:54:18 +0000
@@ -456,3 +456,20 @@
456 >>> print_proposals(webservice, url='/widgets',456 >>> print_proposals(webservice, url='/widgets',
457 ... status=[BranchMergeProposalStatus.REJECTED.title])457 ... status=[BranchMergeProposalStatus.REJECTED.title])
458 http://.../~source/fooix/fix-it/+merge/... - Rejected458 http://.../~source/fooix/fix-it/+merge/... - Rejected
459
460== Getting Merge Proposals a Person has been Asked To Review ==
461
462It's good to be able to find out which proposals you have been asked to
463review.
464
465 >>> login('admin@canonical.com')
466 >>> from lp.code.enums import BranchMergeProposalStatus
467 >>> fixit_proposal.requestReview()
468 >>> logout()
469
470 >>> proposals = webservice.named_get('/~target', 'getRequestedReviews'
471 ... ).jsonBody()
472 >>> for proposal in proposals['entries']:
473 ... print_proposal(proposal)
474 http://.../~source/fooix/fix-it/+merge/2 - Needs review
475
459476
=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py 2010-01-15 01:37:34 +0000
+++ lib/lp/registry/interfaces/person.py 2010-02-04 16:54:18 +0000
@@ -72,7 +72,8 @@
72from canonical.launchpad.interfaces.account import AccountStatus, IAccount72from canonical.launchpad.interfaces.account import AccountStatus, IAccount
73from canonical.launchpad.interfaces.emailaddress import IEmailAddress73from canonical.launchpad.interfaces.emailaddress import IEmailAddress
74from lp.app.interfaces.headings import IRootContext74from lp.app.interfaces.headings import IRootContext
75from lp.code.interfaces.hasbranches import IHasBranches, IHasMergeProposals75from lp.code.interfaces.hasbranches import (
76 IHasBranches, IHasMergeProposals, IHasRequestedReviews)
76from lp.registry.interfaces.irc import IIrcID77from lp.registry.interfaces.irc import IIrcID
77from lp.registry.interfaces.jabber import IJabberID78from lp.registry.interfaces.jabber import IJabberID
78from lp.services.worlddata.interfaces.language import ILanguage79from lp.services.worlddata.interfaces.language import ILanguage
@@ -476,7 +477,8 @@
476477
477class IPersonPublic(IHasBranches, IHasSpecifications, IHasMentoringOffers,478class IPersonPublic(IHasBranches, IHasSpecifications, IHasMentoringOffers,
478 IHasMergeProposals, IHasLogo, IHasMugshot, IHasIcon,479 IHasMergeProposals, IHasLogo, IHasMugshot, IHasIcon,
479 IHasLocation, IObjectWithLocation, IPrivacy):480 IHasLocation, IHasRequestedReviews, IObjectWithLocation,
481 IPrivacy):
480 """Public attributes for a Person."""482 """Public attributes for a Person."""
481483
482 id = Int(title=_('ID'), required=True, readonly=True)484 id = Int(title=_('ID'), required=True, readonly=True)
483485
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2010-01-20 22:09:26 +0000
+++ lib/lp/registry/model/person.py 2010-02-04 16:54:18 +0000
@@ -81,7 +81,8 @@
81from lp.soyuz.interfaces.archivepermission import (81from lp.soyuz.interfaces.archivepermission import (
82 IArchivePermissionSet)82 IArchivePermissionSet)
83from canonical.launchpad.interfaces.authtoken import LoginTokenType83from canonical.launchpad.interfaces.authtoken import LoginTokenType
84from lp.code.model.hasbranches import HasBranchesMixin, HasMergeProposalsMixin84from lp.code.model.hasbranches import (
85 HasBranchesMixin, HasMergeProposalsMixin, HasRequestedReviewsMixin)
85from lp.bugs.interfaces.bugtask import (86from lp.bugs.interfaces.bugtask import (
86 BugTaskSearchParams, IBugTaskSet)87 BugTaskSearchParams, IBugTaskSet)
87from lp.bugs.interfaces.bugtarget import IBugTarget88from lp.bugs.interfaces.bugtarget import IBugTarget
@@ -212,7 +213,7 @@
212213
213class Person(214class Person(
214 SQLBase, HasBugsBase, HasSpecificationsMixin, HasTranslationImportsMixin,215 SQLBase, HasBugsBase, HasSpecificationsMixin, HasTranslationImportsMixin,
215 HasBranchesMixin, HasMergeProposalsMixin):216 HasBranchesMixin, HasMergeProposalsMixin, HasRequestedReviewsMixin):
216 """A Person."""217 """A Person."""
217218
218 implements(IPerson, IHasIcon, IHasLogo, IHasMugshot)219 implements(IPerson, IHasIcon, IHasLogo, IHasMugshot)