Launchpad itself

Merge lp:~cjwatson/launchpad/git-getRequestedReviews into lp:launchpad

  1. git-getRequestedReviews
  2. Merge into devel
Proposed by Colin Watson
Status: Needs review
Proposed branch: lp:~cjwatson/launchpad/git-getRequestedReviews
Merge into: lp:launchpad
Diff against target: 61 lines (+32/-4)
2 files modified
lib/lp/code/model/hasbranches.py (+8/-3)
lib/lp/code/stories/webservice/xx-branchmergeproposal.txt (+24/-1)
To merge this branch: bzr merge lp:~cjwatson/launchpad/git-getRequestedReviews
Reviewer Review Type Date Requested Status
William Grant (community) Needs Information
Review via email: mp+271136@code.launchpad.net

Commit message

Also return Git-based merge proposals from IHasRequestedReviews.getRequestedReviews.

Description of the change

Also return Git-based merge proposals from IHasRequestedReviews.getRequestedReviews.

I made similar changes to nearby code in https://code.launchpad.net/~cjwatson/launchpad/git-activereviews/+merge/258910, but apparently missed the existence of getRequestedReviews.

To post a comment you must log in.
Revision history for this message
William Grant (wgrant) :
review: Approve (code)
Reply
Revision history for this message
William Grant (wgrant) wrote :

Actually, getMergeProposalsForReviewer sorts, and this will likely drop that sort. Is that a problem?

review: Needs Information
Reply
Revision history for this message
Colin Watson (cjwatson) wrote :

Hm. Possibly, and fixing that is difficult with the current structure (CodeReviewComment.vote isn't in the UNIONed result set). I'm thinking that the best way to tackle this might be to do something like what we did with Snap, and move the queries for merge proposals on collections to the BranchMergeProposal model so that we can use a simple OR here rather than a UNION. We would have to think carefully about how to design the bits that remain on *Collection - there'd have to be some reasonable way to extract symmetric vs. asymmetric expressions - but we'd probably still end up with a net reduction of code as well as being able to support this case. Does that sound reasonable?

Reply
Revision history for this message
Colin Watson (cjwatson) wrote :
Reply

Unmerged revisions

17739. By Colin Watson

Also return Git-based merge proposals from IHasRequestedReviews.getRequestedReviews.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/model/hasbranches.py'
2--- lib/lp/code/model/hasbranches.py 2015-05-12 17:30:40 +0000
3+++ lib/lp/code/model/hasbranches.py 2015-09-15 14:34:38 +0000
4@@ -84,9 +84,14 @@
5 if not status:
6 status = (BranchMergeProposalStatus.NEEDS_REVIEW,)
7
8- visible_branches = getUtility(IAllBranches).visibleByUser(
9- visible_by_user)
10- return visible_branches.getMergeProposalsForReviewer(self, status)
11+ def _getProposals(collection):
12+ collection = collection.visibleByUser(visible_by_user)
13+ return collection.getMergeProposalsForReviewer(self, status)
14+
15+ bzr_collection = removeSecurityProxy(getUtility(IAllBranches))
16+ git_collection = removeSecurityProxy(getUtility(IAllGitRepositories))
17+ return _getProposals(bzr_collection).union(
18+ _getProposals(git_collection))
19
20 def getOwnedAndRequestedReviews(self, status=None, visible_by_user=None,
21 project=None, eager_load=False):
22
23=== modified file 'lib/lp/code/stories/webservice/xx-branchmergeproposal.txt'
24--- lib/lp/code/stories/webservice/xx-branchmergeproposal.txt 2015-04-22 14:52:10 +0000
25+++ lib/lp/code/stories/webservice/xx-branchmergeproposal.txt 2015-09-15 14:34:38 +0000
26@@ -470,6 +470,29 @@
27 ... registrant=target_owner, source_branch=target_branch)
28 >>> proposal.nominateReviewer(branch_owner, target_owner)
29 <CodeReviewVoteReference at ...>
30+
31+Create a similar pair of proposals for Git.
32+
33+ >>> [source_ref] = factory.makeGitRefs(
34+ ... owner=branch_owner, target=blob, name=u"foo",
35+ ... paths=[u"refs/heads/master"])
36+ >>> [target_ref] = factory.makeGitRefs(
37+ ... owner=target_owner, target=blob, name=u"bar",
38+ ... paths=[u"refs/heads/fix"])
39+ >>> proposal = factory.makeBranchMergeProposalForGit(
40+ ... target_ref=target_ref,
41+ ... set_state=BranchMergeProposalStatus.NEEDS_REVIEW,
42+ ... registrant=branch_owner, source_ref=source_ref)
43+ >>> proposal.nominateReviewer(target_owner, branch_owner)
44+ <CodeReviewVoteReference at ...>
45+
46+ >>> proposal = factory.makeBranchMergeProposalForGit(
47+ ... target_ref=source_ref,
48+ ... set_state=BranchMergeProposalStatus.NEEDS_REVIEW,
49+ ... registrant=target_owner, source_ref=target_ref)
50+ >>> proposal.nominateReviewer(branch_owner, target_owner)
51+ <CodeReviewVoteReference at ...>
52+
53 >>> logout()
54
55 >>> proposals = webservice.named_get('/~target', 'getRequestedReviews'
56@@ -477,4 +500,4 @@
57 >>> for proposal in proposals['entries']:
58 ... print_proposal(proposal)
59 http://.../~source/blob/foo/+merge/4 - Needs review
60-
61+ http://.../~source/blob/+git/foo/+merge/6 - Needs review