Launchpad itself

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

  1. git-activereviews
  2. Merge into devel
Proposed by Colin Watson
Status: Merged
Merged at revision: 17500
Proposed branch: lp:~cjwatson/launchpad/git-activereviews
Merge into: lp:launchpad
Diff against target: 951 lines (+349/-104)
10 files modified
lib/lp/code/browser/branchmergeproposallisting.py (+18/-24)
lib/lp/code/browser/configure.zcml (+6/-0)
lib/lp/code/browser/tests/test_branchmergeproposallisting.py (+206/-60)
lib/lp/code/interfaces/gitref.py (+2/-1)
lib/lp/code/interfaces/hasbranches.py (+20/-5)
lib/lp/code/model/branchmergeproposal.py (+25/-7)
lib/lp/code/model/gitcollection.py (+15/-0)
lib/lp/code/model/hasbranches.py (+53/-3)
lib/lp/code/templates/branchmergeproposal-listing.pt (+2/-2)
lib/lp/code/templates/branchmergeproposal-macros.pt (+2/-2)
To merge this branch: bzr merge lp:~cjwatson/launchpad/git-activereviews
Reviewer Review Type Date Requested Status
William Grant (community) code Approve
Review via email: mp+258910@code.launchpad.net

Commit message

Extend merge proposal listings to cover Git, and add GitRef:+activereviews.

Description of the change

Extend merge proposal listings to cover Git, and add GitRef:+activereviews.

We should probably have GitRepository:+activereviews at some point too, but that can come in a separate branch.

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/browser/branchmergeproposallisting.py'
2--- lib/lp/code/browser/branchmergeproposallisting.py 2014-12-08 00:32:30 +0000
3+++ lib/lp/code/browser/branchmergeproposallisting.py 2015-05-13 09:36:31 +0000
4@@ -1,4 +1,4 @@
5-# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
6+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
7 # GNU Affero General Public License version 3 (see the file LICENSE).
8
9 """Base class view for branch merge proposal listings."""
10@@ -40,10 +40,6 @@
11 BranchMergeProposalStatus,
12 CodeReviewVote,
13 )
14-from lp.code.interfaces.branchcollection import (
15- IAllBranches,
16- IBranchCollection,
17- )
18 from lp.code.interfaces.branchmergeproposal import (
19 BRANCH_MERGE_PROPOSAL_FINAL_STATES,
20 IBranchMergeProposal,
21@@ -168,7 +164,7 @@
22
23 @cachedproperty
24 def proposals(self):
25- """Return a list of BranchListingItems."""
26+ """Return a list of BranchMergeProposalListingItems."""
27 proposals = self._proposals_for_current_batch
28 return [self._createItem(proposal) for proposal in proposals]
29
30@@ -287,12 +283,11 @@
31
32 def getProposals(self):
33 """Get the proposals for the view."""
34- collection = IBranchCollection(self.context)
35- collection = collection.visibleByUser(self.user)
36- proposals = collection.getMergeProposals(
37- [BranchMergeProposalStatus.CODE_APPROVED,
38- BranchMergeProposalStatus.NEEDS_REVIEW], eager_load=True)
39- return proposals
40+ return self.context.getMergeProposals(
41+ status=(
42+ BranchMergeProposalStatus.CODE_APPROVED,
43+ BranchMergeProposalStatus.NEEDS_REVIEW),
44+ visible_by_user=self.user, eager_load=True)
45
46 def _getReviewGroup(self, proposal, votes, reviewer):
47 """One of APPROVED, MINE, TO_DO, CAN_DO, ARE_DOING, OTHER or WIP.
48@@ -324,8 +319,8 @@
49 return self.WIP
50
51 if (reviewer is not None and
52- (proposal.source_branch.owner == reviewer or
53- (reviewer.inTeam(proposal.source_branch.owner) and
54+ (proposal.merge_source.owner == reviewer or
55+ (reviewer.inTeam(proposal.merge_source.owner) and
56 proposal.registrant == reviewer))):
57 return self.MINE
58
59@@ -437,15 +432,13 @@
60 def _getReviewer(self):
61 return self.context
62
63- def _getCollection(self):
64- return getUtility(IAllBranches)
65-
66- def getProposals(self):
67+ def getProposals(self, project=None):
68 """See `ActiveReviewsView`."""
69- collection = self._getCollection().visibleByUser(self.user)
70- return collection.getMergeProposalsForPerson(
71- self._getReviewer(), [BranchMergeProposalStatus.CODE_APPROVED,
72- BranchMergeProposalStatus.NEEDS_REVIEW], eager_load=True)
73+ return self._getReviewer().getOwnedAndRequestedReviews(
74+ status=(
75+ BranchMergeProposalStatus.CODE_APPROVED,
76+ BranchMergeProposalStatus.NEEDS_REVIEW),
77+ visible_by_user=self.user, project=project, eager_load=True)
78
79
80 class PersonProductActiveReviewsView(PersonActiveReviewsView):
81@@ -459,8 +452,9 @@
82 def _getReviewer(self):
83 return self.context.person
84
85- def _getCollection(self):
86- return getUtility(IAllBranches).inProduct(self.context.product)
87+ def getProposals(self):
88+ return super(PersonProductActiveReviewsView, self).getProposals(
89+ project=self.context.product)
90
91 @property
92 def no_proposal_message(self):
93
94=== modified file 'lib/lp/code/browser/configure.zcml'
95--- lib/lp/code/browser/configure.zcml 2015-04-28 16:48:22 +0000
96+++ lib/lp/code/browser/configure.zcml 2015-05-13 09:36:31 +0000
97@@ -844,6 +844,12 @@
98 name="+register-merge"
99 permission="launchpad.AnyPerson"
100 template="../templates/gitref-register-merge.pt"/>
101+ <browser:page
102+ for="lp.code.interfaces.gitref.IGitRef"
103+ class="lp.code.browser.branchmergeproposallisting.BranchActiveReviewsView"
104+ permission="zope.Public"
105+ name="+activereviews"
106+ template="../templates/active-reviews.pt"/>
107 <browser:menus
108 classes="GitRefContextMenu"
109 module="lp.code.browser.gitref"/>
110
111=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposallisting.py'
112--- lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2013-04-29 00:10:09 +0000
113+++ lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2015-05-13 09:36:31 +0000
114@@ -1,4 +1,4 @@
115-# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
116+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
117 # GNU Affero General Public License version 3 (see the file LICENSE).
118
119 """Unit tests for BranchMergeProposal listing views."""
120@@ -12,6 +12,7 @@
121 from testtools.content_type import UTF8_TEXT
122 from testtools.matchers import Equals
123 import transaction
124+from zope.component import getUtility
125 from zope.security.proxy import removeSecurityProxy
126
127 from lp.app.enums import InformationType
128@@ -23,8 +24,14 @@
129 BranchMergeProposalStatus,
130 CodeReviewVote,
131 )
132+from lp.code.interfaces.gitref import IGitRef
133+from lp.code.interfaces.gitrepository import (
134+ GIT_FEATURE_FLAG,
135+ IGitRepositorySet,
136+ )
137 from lp.registry.model.personproduct import PersonProduct
138 from lp.services.database.sqlbase import flush_database_caches
139+from lp.services.features.testing import FeatureFixture
140 from lp.testing import (
141 ANONYMOUS,
142 BrowserTestCase,
143@@ -45,7 +52,55 @@
144 _default = object()
145
146
147-class TestProposalVoteSummary(TestCaseWithFactory):
148+class BzrMixin:
149+ """Mixin for Bazaar-based tests."""
150+
151+ def _makeBranch(self, target=None, **kwargs):
152+ if target is not None:
153+ # This only handles projects at the moment.
154+ kwargs["product"] = target
155+ return self.factory.makeBranch(**kwargs)
156+
157+ def _makePackageBranch(self, **kwargs):
158+ return self.factory.makePackageBranch(**kwargs)
159+
160+ def _makeStackedOnBranchChain(self, target=None, **kwargs):
161+ if target is not None:
162+ # This only handles projects at the moment.
163+ kwargs["product"] = target
164+ return self.factory.makeStackedOnBranchChain(**kwargs)
165+
166+ def _makeBranchMergeProposal(self, target=None, merge_target=None,
167+ **kwargs):
168+ # This only handles projects at the moment.
169+ return self.factory.makeBranchMergeProposal(
170+ product=target, target_branch=merge_target, **kwargs)
171+
172+
173+class GitMixin:
174+ """Mixin for Git-based tests."""
175+
176+ def setUp(self, user=ANONYMOUS):
177+ super(GitMixin, self).setUp(user=user)
178+ self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
179+
180+ def _makeBranch(self, **kwargs):
181+ return self.factory.makeGitRefs(**kwargs)[0]
182+
183+ def _makePackageBranch(self, **kwargs):
184+ dsp = self.factory.makeDistributionSourcePackage()
185+ return self.factory.makeGitRefs(target=dsp, **kwargs)[0]
186+
187+ def _makeStackedOnBranchChain(self, depth=None, **kwargs):
188+ # Git doesn't have stacked branches. Just make an ordinary reference.
189+ return self._makeBranch(**kwargs)
190+
191+ def _makeBranchMergeProposal(self, merge_target=None, **kwargs):
192+ return self.factory.makeBranchMergeProposalForGit(
193+ target_ref=merge_target, **kwargs)
194+
195+
196+class TestProposalVoteSummaryMixin:
197 """The vote summary shows a summary of the current votes."""
198
199 layer = DatabaseFunctionalLayer
200@@ -53,7 +108,8 @@
201 def setUp(self):
202 # Use an admin so we don't have to worry about launchpad.Edit
203 # permissions on the merge proposals for adding comments.
204- TestCaseWithFactory.setUp(self, user="admin@canonical.com")
205+ super(TestProposalVoteSummaryMixin, self).setUp(
206+ user="admin@canonical.com")
207
208 def _createComment(self, proposal, reviewer=None, vote=None,
209 comment=_default):
210@@ -69,7 +125,7 @@
211 def _get_vote_summary(self, proposal):
212 """Return the vote summary string for the proposal."""
213 view = create_initialized_view(
214- proposal.source_branch.owner, '+merges', rootsite='code')
215+ proposal.merge_source.owner, '+merges', rootsite='code')
216 batch_navigator = view.proposals
217 # There will only be one item in the list of proposals.
218 [listing_item] = batch_navigator.proposals
219@@ -78,14 +134,14 @@
220
221 def test_no_votes_or_comments(self):
222 # If there are no votes or comments, then we show that.
223- proposal = self.factory.makeBranchMergeProposal()
224+ proposal = self._makeBranchMergeProposal()
225 summary, comment_count = self._get_vote_summary(proposal)
226 self.assertEqual([], summary)
227 self.assertEqual(0, comment_count)
228
229 def test_no_votes_with_comments(self):
230 # The comment count is shown.
231- proposal = self.factory.makeBranchMergeProposal()
232+ proposal = self._makeBranchMergeProposal()
233 self._createComment(proposal)
234 summary, comment_count = self._get_vote_summary(proposal)
235 self.assertEqual([], summary)
236@@ -93,7 +149,7 @@
237
238 def test_vote_without_comment(self):
239 # If there are no comments we don't show a count.
240- proposal = self.factory.makeBranchMergeProposal()
241+ proposal = self._makeBranchMergeProposal()
242 self._createComment(
243 proposal, vote=CodeReviewVote.APPROVE, comment=None)
244 summary, comment_count = self._get_vote_summary(proposal)
245@@ -104,7 +160,7 @@
246
247 def test_vote_with_comment(self):
248 # A vote with a comment counts as a vote and a comment.
249- proposal = self.factory.makeBranchMergeProposal()
250+ proposal = self._makeBranchMergeProposal()
251 self._createComment(proposal, vote=CodeReviewVote.APPROVE)
252 summary, comment_count = self._get_vote_summary(proposal)
253 self.assertEqual(
254@@ -114,7 +170,7 @@
255
256 def test_disapproval(self):
257 # Shown as Disapprove: <count>.
258- proposal = self.factory.makeBranchMergeProposal()
259+ proposal = self._makeBranchMergeProposal()
260 self._createComment(proposal, vote=CodeReviewVote.DISAPPROVE)
261 summary, comment_count = self._get_vote_summary(proposal)
262 self.assertEqual(
263@@ -124,7 +180,7 @@
264
265 def test_abstain(self):
266 # Shown as Abstain: <count>.
267- proposal = self.factory.makeBranchMergeProposal()
268+ proposal = self._makeBranchMergeProposal()
269 transaction.commit()
270 self._createComment(proposal, vote=CodeReviewVote.ABSTAIN)
271 summary, comment_count = self._get_vote_summary(proposal)
272@@ -135,7 +191,7 @@
273
274 def test_vote_ranking(self):
275 # Votes go from best to worst.
276- proposal = self.factory.makeBranchMergeProposal()
277+ proposal = self._makeBranchMergeProposal()
278 self._createComment(proposal, vote=CodeReviewVote.DISAPPROVE)
279 self._createComment(proposal, vote=CodeReviewVote.APPROVE)
280 summary, comment_count = self._get_vote_summary(proposal)
281@@ -158,7 +214,7 @@
282
283 def test_multiple_votes_for_type(self):
284 # Multiple votes of a type are aggregated in the summary.
285- proposal = self.factory.makeBranchMergeProposal()
286+ proposal = self._makeBranchMergeProposal()
287 self._createComment(proposal, vote=CodeReviewVote.DISAPPROVE)
288 self._createComment(proposal, vote=CodeReviewVote.APPROVE)
289 self._createComment(proposal, vote=CodeReviewVote.DISAPPROVE)
290@@ -178,6 +234,16 @@
291 self.assertEqual(4, comment_count)
292
293
294+class TestProposalVoteSummaryBzr(
295+ TestProposalVoteSummaryMixin, BzrMixin, TestCaseWithFactory):
296+ """Test the vote summary for Bazaar."""
297+
298+
299+class TestProposalVoteSummaryGit(
300+ TestProposalVoteSummaryMixin, GitMixin, TestCaseWithFactory):
301+ """Test the vote summary for Git."""
302+
303+
304 class TestMerges(BrowserTestCase):
305
306 layer = DatabaseFunctionalLayer
307@@ -193,7 +259,7 @@
308 package = self.factory.makeDistributionSourcePackage()
309 self.getViewBrowser(package, '+merges', rootsite='code')
310
311- def test_query_count(self):
312+ def test_query_count_bzr(self):
313 product = self.factory.makeProduct()
314 target = self.factory.makeBranch(
315 product=product, information_type=InformationType.USERDATA)
316@@ -209,24 +275,51 @@
317 product, '+merges', rootsite='code', user=product.owner)
318 self.assertThat(recorder, HasQueryCount(Equals(41)))
319
320- def test_productseries(self):
321+ def test_query_count_git(self):
322+ self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
323+ product = self.factory.makeProduct()
324+ [target] = self.factory.makeGitRefs(
325+ target=product, information_type=InformationType.USERDATA)
326+ for i in range(7):
327+ [source] = self.factory.makeGitRefs(
328+ target=product, information_type=InformationType.USERDATA)
329+ self.factory.makeBranchMergeProposalForGit(
330+ source_ref=source, target_ref=target)
331+ flush_database_caches()
332+ with StormStatementRecorder() as recorder:
333+ self.getViewBrowser(
334+ product, '+merges', rootsite='code', user=product.owner)
335+ self.assertThat(recorder, HasQueryCount(Equals(38)))
336+
337+ def test_productseries_bzr(self):
338 target = self.factory.makeBranch()
339- unique_name = target.unique_name
340 with person_logged_in(target.product.owner):
341 target.product.development_focus.branch = target
342+ identity = target.identity
343 self.factory.makeBranchMergeProposal(target_branch=target)
344 view = self.getViewBrowser(target, '+merges', rootsite='code')
345- self.assertIn(unique_name, view.contents)
346-
347-
348-class ActiveReviewGroupsTest(TestCaseWithFactory):
349- """Tests for groupings used in for active reviews."""
350+ self.assertIn(identity, view.contents)
351+
352+ def test_product_git(self):
353+ self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
354+ [target] = self.factory.makeGitRefs()
355+ with person_logged_in(target.target.owner):
356+ getUtility(IGitRepositorySet).setDefaultRepository(
357+ target.target, target.repository)
358+ identity = target.identity
359+ self.factory.makeBranchMergeProposalForGit(target_ref=target)
360+ view = self.getViewBrowser(target, '+merges', rootsite='code')
361+ self.assertIn(identity, view.contents)
362+
363+
364+class ActiveReviewGroupsTestMixin:
365+ """Tests for groupings used for active reviews."""
366
367 layer = DatabaseFunctionalLayer
368
369 def setUp(self):
370- super(ActiveReviewGroupsTest, self).setUp()
371- self.bmp = self.factory.makeBranchMergeProposal(
372+ super(ActiveReviewGroupsTestMixin, self).setUp()
373+ self.bmp = self._makeBranchMergeProposal(
374 set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
375
376 def assertReviewGroupForReviewer(self, reviewer, group):
377@@ -247,21 +340,21 @@
378
379 def test_approved(self):
380 # If the proposal is approved, then the group is approved.
381- self.bmp = self.factory.makeBranchMergeProposal(
382+ self.bmp = self._makeBranchMergeProposal(
383 set_state=BranchMergeProposalStatus.CODE_APPROVED)
384 self.assertReviewGroupForReviewer(None, ActiveReviewsView.APPROVED)
385
386 def test_work_in_progress(self):
387 # If the proposal is in progress, then the group is wip.
388- self.bmp = self.factory.makeBranchMergeProposal(
389+ self.bmp = self._makeBranchMergeProposal(
390 set_state=BranchMergeProposalStatus.WORK_IN_PROGRESS)
391 self.assertReviewGroupForReviewer(None, ActiveReviewsView.WIP)
392
393- def test_source_branch_owner(self):
394- # If the reviewer is the owner of the source branch, then the review
395+ def test_merge_source_owner(self):
396+ # If the reviewer is the owner of the merge source, then the review
397 # is MINE. This occurs whether or not the user is the logged in or
398 # not.
399- reviewer = self.bmp.source_branch.owner
400+ reviewer = self.bmp.merge_source.owner
401 self.assertReviewGroupForReviewer(reviewer, ActiveReviewsView.MINE)
402
403 def test_proposal_registrant(self):
404@@ -271,13 +364,17 @@
405 self.assertReviewGroupForReviewer(reviewer, ActiveReviewsView.OTHER)
406
407 team = self.factory.makeTeam(self.bmp.registrant)
408- removeSecurityProxy(self.bmp.source_branch).owner = team
409+ naked_merge_source = removeSecurityProxy(self.bmp.merge_source)
410+ if IGitRef.providedBy(naked_merge_source):
411+ naked_merge_source.repository.owner = team
412+ else:
413+ naked_merge_source.owner = team
414 self.assertReviewGroupForReviewer(reviewer, ActiveReviewsView.MINE)
415
416- def test_target_branch_owner(self):
417- # For the target branch owner, it is to_do since they are the default
418+ def test_merge_target_owner(self):
419+ # For the merge target owner, it is to_do since they are the default
420 # reviewer.
421- reviewer = self.bmp.target_branch.owner
422+ reviewer = self.bmp.merge_target.owner
423 self.assertReviewGroupForReviewer(reviewer, ActiveReviewsView.TO_DO)
424
425 def test_group_pending_review(self):
426@@ -299,7 +396,7 @@
427 def test_review_done(self):
428 # If the logged in user has a completed review, then the review is
429 # ARE_DOING.
430- reviewer = self.bmp.target_branch.owner
431+ reviewer = self.bmp.merge_target.owner
432 login_person(reviewer)
433 self.bmp.createComment(
434 reviewer, 'subject', vote=CodeReviewVote.APPROVE)
435@@ -307,7 +404,17 @@
436 reviewer, ActiveReviewsView.ARE_DOING)
437
438
439-class TestBranchMergeProposalListingItem(TestCaseWithFactory):
440+class ActiveReviewGroupsTestBzr(
441+ ActiveReviewGroupsTestMixin, BzrMixin, TestCaseWithFactory):
442+ """Tests for groupings used for active reviews for Bazaar."""
443+
444+
445+class ActiveReviewGroupsTestGit(
446+ ActiveReviewGroupsTestMixin, GitMixin, TestCaseWithFactory):
447+ """Tests for groupings used for active reviews for Git."""
448+
449+
450+class TestBranchMergeProposalListingItemMixin:
451 """Tests specifically relating to the BranchMergeProposalListingItem."""
452
453 layer = DatabaseFunctionalLayer
454@@ -358,7 +465,17 @@
455 self.assertEqual(bmp.date_created, item.sort_key)
456
457
458-class ActiveReviewSortingTest(TestCaseWithFactory):
459+class TestBranchMergeProposalListingItemBzr(
460+ TestBranchMergeProposalListingItemMixin, BzrMixin, TestCaseWithFactory):
461+ """Test BranchMergeProposalListingItem for Bazaar."""
462+
463+
464+class TestBranchMergeProposalListingItemGit(
465+ TestBranchMergeProposalListingItemMixin, GitMixin, TestCaseWithFactory):
466+ """Test BranchMergeProposalListingItem for Git."""
467+
468+
469+class ActiveReviewSortingTestMixin:
470 """Test the sorting of the active review groups."""
471
472 layer = DatabaseFunctionalLayer
473@@ -366,14 +483,14 @@
474 def test_oldest_first(self):
475 # The oldest requested reviews should be first.
476 product = self.factory.makeProduct()
477- bmp1 = self.factory.makeBranchMergeProposal(product=product)
478- login_person(bmp1.source_branch.owner)
479+ bmp1 = self._makeBranchMergeProposal(target=product)
480+ login_person(bmp1.merge_source.owner)
481 bmp1.requestReview(datetime(2009, 6, 1, tzinfo=pytz.UTC))
482- bmp2 = self.factory.makeBranchMergeProposal(product=product)
483- login_person(bmp2.source_branch.owner)
484+ bmp2 = self._makeBranchMergeProposal(target=product)
485+ login_person(bmp2.merge_source.owner)
486 bmp2.requestReview(datetime(2009, 3, 1, tzinfo=pytz.UTC))
487- bmp3 = self.factory.makeBranchMergeProposal(product=product)
488- login_person(bmp3.source_branch.owner)
489+ bmp3 = self._makeBranchMergeProposal(target=product)
490+ login_person(bmp3.merge_source.owner)
491 bmp3.requestReview(datetime(2009, 1, 1, tzinfo=pytz.UTC))
492 login(ANONYMOUS)
493 view = create_initialized_view(
494@@ -383,8 +500,18 @@
495 [item.context for item in view.review_groups[view.OTHER]])
496
497
498-class ActiveReviewsWithPrivateBranches(TestCaseWithFactory):
499- """Test the sorting of the active review groups."""
500+class ActiveReviewSortingTestBzr(
501+ ActiveReviewSortingTestMixin, BzrMixin, TestCaseWithFactory):
502+ """Test the sorting of the active review groups for Bazaar."""
503+
504+
505+class ActiveReviewSortingTestGit(
506+ ActiveReviewSortingTestMixin, GitMixin, TestCaseWithFactory):
507+ """Test the sorting of the active review groups for Git."""
508+
509+
510+class ActiveReviewsWithPrivateBranchesMixin:
511+ """Test reviews of private branches."""
512
513 layer = DatabaseFunctionalLayer
514
515@@ -392,32 +519,42 @@
516 # Merge proposals against private branches are visible to
517 # the branch owner.
518 product = self.factory.makeProduct()
519- branch = self.factory.makeBranch(
520- product=product, information_type=InformationType.USERDATA)
521+ branch = self._makeBranch(
522+ target=product, information_type=InformationType.USERDATA)
523 with person_logged_in(removeSecurityProxy(branch).owner):
524- mp = self.factory.makeBranchMergeProposal(target_branch=branch)
525+ mp = self._makeBranchMergeProposal(merge_target=branch)
526 view = create_initialized_view(
527 branch, name='+activereviews', rootsite='code')
528 self.assertEqual([mp], list(view.getProposals()))
529
530
531-class PersonActiveReviewsPerformance(TestCaseWithFactory):
532+class ActiveReviewsWithPrivateBranchesBzr(
533+ ActiveReviewsWithPrivateBranchesMixin, BzrMixin, TestCaseWithFactory):
534+ """Test reviews of private Bazaar branches."""
535+
536+
537+class ActiveReviewsWithPrivateBranchesGit(
538+ ActiveReviewsWithPrivateBranchesMixin, GitMixin, TestCaseWithFactory):
539+ """Test reviews of references in private Git repositories."""
540+
541+
542+class PersonActiveReviewsPerformanceMixin:
543 """Test the performance of the person's active reviews page."""
544
545 layer = LaunchpadFunctionalLayer
546
547 def setupBMP(self, bmp):
548 self.factory.makePreviewDiff(merge_proposal=bmp)
549- login_person(bmp.source_branch.owner)
550+ login_person(bmp.merge_source.owner)
551 bmp.requestReview()
552
553- def createUserBMP(self, reviewer=None, target_branch_owner=None):
554- target_branch = None
555- if target_branch_owner is not None:
556- target_branch = self.factory.makePackageBranch(
557- owner=target_branch_owner)
558- bmp = self.factory.makeBranchMergeProposal(
559- reviewer=reviewer, target_branch=target_branch)
560+ def createUserBMP(self, reviewer=None, merge_target_owner=None):
561+ merge_target = None
562+ if merge_target_owner is not None:
563+ merge_target = self._makePackageBranch(
564+ owner=merge_target_owner)
565+ bmp = self._makeBranchMergeProposal(
566+ reviewer=reviewer, merge_target=merge_target)
567 self.setupBMP(bmp)
568 return bmp
569
570@@ -431,10 +568,9 @@
571 # Create one of the two types of BMP which will be displayed
572 # on a person's +activereviews page:
573 # - A BMP for which the person is the reviewer.
574- # - A BMP for which the person is the owner of the target
575- # branch.
576+ # - A BMP for which the person is the owner of the merge target.
577 if i % 2 == 0:
578- self.createUserBMP(target_branch_owner=user)
579+ self.createUserBMP(merge_target_owner=user)
580 else:
581 self.createUserBMP(reviewer=user)
582 login_person(user)
583@@ -457,9 +593,9 @@
584 self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
585
586 def createProductBMP(self, product):
587- target_branch = self.factory.makeStackedOnBranchChain(product=product)
588- bmp = self.factory.makeBranchMergeProposal(
589- product=product, target_branch=target_branch)
590+ merge_target = self._makeStackedOnBranchChain(target=product)
591+ bmp = self._makeBranchMergeProposal(
592+ target=product, merge_target=merge_target)
593 self.setupBMP(bmp)
594 return bmp
595
596@@ -491,3 +627,13 @@
597 base_bmps + added_bmps)
598 self.assertEqual(base_bmps + added_bmps, view2.proposal_count)
599 self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
600+
601+
602+class PersonActiveReviewsPerformanceBzr(
603+ PersonActiveReviewsPerformanceMixin, BzrMixin, TestCaseWithFactory):
604+ """Test the performance of the person's active reviews page for Bazaar."""
605+
606+
607+class PersonActiveReviewsPerformanceGit(
608+ PersonActiveReviewsPerformanceMixin, GitMixin, TestCaseWithFactory):
609+ """Test the performance of the person's active reviews page for Git."""
610
611=== modified file 'lib/lp/code/interfaces/gitref.py'
612--- lib/lp/code/interfaces/gitref.py 2015-04-24 12:58:46 +0000
613+++ lib/lp/code/interfaces/gitref.py 2015-05-13 09:36:31 +0000
614@@ -43,11 +43,12 @@
615 BranchMergeProposalStatus,
616 GitObjectType,
617 )
618+from lp.code.interfaces.hasbranches import IHasMergeProposals
619 from lp.registry.interfaces.person import IPerson
620 from lp.services.webapp.interfaces import ITableBatchNavigator
621
622
623-class IGitRef(Interface):
624+class IGitRef(IHasMergeProposals):
625 """A reference in a Git repository."""
626
627 # XXX cjwatson 2015-01-19 bug=760849: "beta" is a lie to get WADL
628
629=== modified file 'lib/lp/code/interfaces/hasbranches.py'
630--- lib/lp/code/interfaces/hasbranches.py 2013-01-07 02:40:55 +0000
631+++ lib/lp/code/interfaces/hasbranches.py 2015-05-13 09:36:31 +0000
632@@ -1,4 +1,4 @@
633-# Copyright 2009 Canonical Ltd. This software is licensed under the
634+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
635 # GNU Affero General Public License version 3 (see the file LICENSE).
636
637 """Interface definitions for IHas<code related bits>."""
638@@ -56,7 +56,7 @@
639 title=_('Limit the branches to those modified since this date.'),
640 required=False))
641 @call_with(visible_by_user=REQUEST_USER)
642- @operation_returns_collection_of(Interface) # Really IBranch.
643+ @operation_returns_collection_of(Interface) # Really IBranch.
644 @export_read_operation()
645 @operation_for_version('beta')
646 def getBranches(status=None, visible_by_user=None,
647@@ -88,7 +88,7 @@
648 title=_("A list of merge proposal statuses to filter by."),
649 value_type=Choice(vocabulary=BranchMergeProposalStatus)))
650 @call_with(visible_by_user=REQUEST_USER)
651- @operation_returns_collection_of(Interface) # Really IBranchMergeProposal.
652+ @operation_returns_collection_of(Interface) # Really IBranchMergeProposal.
653 @export_read_operation()
654 @operation_for_version('beta')
655 def getMergeProposals(status=None, visible_by_user=None):
656@@ -115,7 +115,7 @@
657 title=_("A list of merge proposal statuses to filter by."),
658 value_type=Choice(vocabulary=BranchMergeProposalStatus)))
659 @call_with(visible_by_user=REQUEST_USER)
660- @operation_returns_collection_of(Interface) # Really IBranchMergeProposal.
661+ @operation_returns_collection_of(Interface) # Really IBranchMergeProposal.
662 @export_read_operation()
663 @operation_for_version('beta')
664 def getRequestedReviews(status=None, visible_by_user=None):
665@@ -130,6 +130,21 @@
666 :returns: A list of `IBranchMergeProposal`.
667 """
668
669+ def getOwnedAndRequestedReviews(status=None, visible_by_user=None,
670+ project=None):
671+ """Returns merge proposals for branches owned by a person, or where
672+ that person was asked to review.
673+
674+ This does not include merge proposals that were requested from
675+ teams that the person is part of. If status is not passed then
676+ it will return proposals that are in the "Needs Review" state.
677+
678+ :param status: A list of statuses to filter with.
679+ :param visible_by_user: Normally the user who is asking.
680+ :param project: Limit results to branches for this `IProduct`.
681+ :returns: A list of `IBranchMergeProposal`.
682+ """
683+
684
685 class IHasCodeImports(Interface):
686 """Some things can have code imports that target them.
687@@ -151,7 +166,7 @@
688 schema=Interface)
689 )
690 @call_with(registrant=REQUEST_USER)
691- @export_factory_operation(Interface, []) # Really ICodeImport.
692+ @export_factory_operation(Interface, []) # Really ICodeImport.
693 @operation_for_version('beta')
694 def newCodeImport(registrant=None, branch_name=None, rcs_type=None,
695 url=None, cvs_root=None, cvs_module=None, owner=None):
696
697=== modified file 'lib/lp/code/model/branchmergeproposal.py'
698--- lib/lp/code/model/branchmergeproposal.py 2015-04-22 16:42:57 +0000
699+++ lib/lp/code/model/branchmergeproposal.py 2015-05-13 09:36:31 +0000
700@@ -92,7 +92,10 @@
701 from lp.registry.interfaces.product import IProduct
702 from lp.registry.model.person import Person
703 from lp.services.config import config
704-from lp.services.database.bulk import load_related
705+from lp.services.database.bulk import (
706+ load,
707+ load_related,
708+ )
709 from lp.services.database.constants import (
710 DEFAULT,
711 UTC_NOW,
712@@ -1055,20 +1058,30 @@
713 from lp.code.model.branch import Branch
714 from lp.code.model.branchcollection import GenericBranchCollection
715 from lp.code.model.gitcollection import GenericGitCollection
716+ from lp.code.model.gitref import GitRef
717 from lp.code.model.gitrepository import GitRepository
718
719 ids = set()
720 source_branch_ids = set()
721- source_git_repository_ids = set()
722+ git_ref_keys = set()
723 person_ids = set()
724 for mp in branch_merge_proposals:
725 ids.add(mp.id)
726 if mp.source_branchID is not None:
727 source_branch_ids.add(mp.source_branchID)
728 if mp.source_git_repositoryID is not None:
729- source_git_repository_ids.add(mp.source_git_repositoryID)
730+ git_ref_keys.add(
731+ (mp.source_git_repositoryID, mp.source_git_path))
732+ git_ref_keys.add(
733+ (mp.target_git_repositoryID, mp.target_git_path))
734+ if mp.prerequisite_git_repositoryID is not None:
735+ git_ref_keys.add(
736+ (mp.prerequisite_git_repositoryID,
737+ mp.prerequisite_git_path))
738 person_ids.add(mp.registrantID)
739 person_ids.add(mp.merge_reporterID)
740+ git_repository_ids = set(
741+ repository_id for repository_id, _ in git_ref_keys)
742
743 branches = load_related(
744 Branch, branch_merge_proposals, (
745@@ -1078,9 +1091,11 @@
746 GitRepository, branch_merge_proposals, (
747 "target_git_repositoryID", "prerequisite_git_repositoryID",
748 "source_git_repositoryID"))
749+ load(GitRef, git_ref_keys)
750 # The stacked on branches are used to check branch visibility.
751 GenericBranchCollection.preloadVisibleStackedOnBranches(
752 branches, user)
753+ GenericGitCollection.preloadVisibleRepositories(repositories, user)
754
755 if len(branches) == 0 and len(repositories) == 0:
756 return
757@@ -1098,21 +1113,24 @@
758 cache.preview_diff = previewdiff
759
760 # Add source branch/repository owners' to the list of pre-loaded
761- # persons.
762+ # persons. We need the target repository owner as well; unlike
763+ # branches, repository unique names aren't trigger-maintained.
764 person_ids.update(
765 branch.ownerID for branch in branches
766 if branch.id in source_branch_ids)
767 person_ids.update(
768 repository.owner_id for repository in repositories
769- if repository.id in source_git_repository_ids)
770+ if repository.id in git_repository_ids)
771
772 # Pre-load Person and ValidPersonCache.
773 list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
774 person_ids, need_validity=True))
775
776 # Pre-load branches'/repositories' data.
777- GenericBranchCollection.preloadDataForBranches(branches)
778- GenericGitCollection.preloadDataForRepositories(repositories)
779+ if branches:
780+ GenericBranchCollection.preloadDataForBranches(branches)
781+ if repositories:
782+ GenericGitCollection.preloadDataForRepositories(repositories)
783
784
785 class BranchMergeProposalGetter:
786
787=== modified file 'lib/lp/code/model/gitcollection.py'
788--- lib/lp/code/model/gitcollection.py 2015-04-22 16:42:57 +0000
789+++ lib/lp/code/model/gitcollection.py 2015-05-13 09:36:31 +0000
790@@ -9,6 +9,7 @@
791 ]
792
793 from functools import partial
794+from operator import attrgetter
795
796 from lazr.uri import (
797 InvalidURIError,
798@@ -178,6 +179,20 @@
799 return []
800
801 @staticmethod
802+ def preloadVisibleRepositories(repositories, user=None):
803+ """Preload visibility for the given list of repositories."""
804+ if len(repositories) == 0:
805+ return
806+ expressions = [
807+ GitRepository.id.is_in(map(attrgetter("id"), repositories))]
808+ if user is None:
809+ collection = AnonymousGitCollection(filter_expressions=expressions)
810+ else:
811+ collection = VisibleGitCollection(
812+ user=user, filter_expressions=expressions)
813+ return list(collection.getRepositories())
814+
815+ @staticmethod
816 def preloadDataForRepositories(repositories):
817 """Preload repositories' cached associated targets."""
818 load_related(Distribution, repositories, ['distribution_id'])
819
820=== modified file 'lib/lp/code/model/hasbranches.py'
821--- lib/lp/code/model/hasbranches.py 2013-03-04 04:17:17 +0000
822+++ lib/lp/code/model/hasbranches.py 2015-05-13 09:36:31 +0000
823@@ -1,4 +1,4 @@
824-# Copyright 2009 Canonical Ltd. This software is licensed under the
825+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
826 # GNU Affero General Public License version 3 (see the file LICENSE).
827
828 """Mixin classes to implement methods for IHas<code related bits>."""
829@@ -11,7 +11,10 @@
830 'HasRequestedReviewsMixin',
831 ]
832
833+from functools import partial
834+
835 from zope.component import getUtility
836+from zope.security.proxy import removeSecurityProxy
837
838 from lp.code.enums import BranchMergeProposalStatus
839 from lp.code.interfaces.branch import DEFAULT_BRANCH_STATUS_IN_LISTING
840@@ -20,6 +23,11 @@
841 IBranchCollection,
842 )
843 from lp.code.interfaces.branchtarget import IBranchTarget
844+from lp.code.interfaces.gitcollection import (
845+ IAllGitRepositories,
846+ IGitCollection,
847+ )
848+from lp.services.database.decoratedresultset import DecoratedResultSet
849
850
851 class HasBranchesMixin:
852@@ -44,14 +52,28 @@
853 def getMergeProposals(self, status=None, visible_by_user=None,
854 eager_load=False):
855 """See `IHasMergeProposals`."""
856+ # Circular import.
857+ from lp.code.model.branchmergeproposal import BranchMergeProposal
858+
859 if not status:
860 status = (
861 BranchMergeProposalStatus.CODE_APPROVED,
862 BranchMergeProposalStatus.NEEDS_REVIEW,
863 BranchMergeProposalStatus.WORK_IN_PROGRESS)
864
865- collection = IBranchCollection(self).visibleByUser(visible_by_user)
866- return collection.getMergeProposals(status, eager_load=eager_load)
867+ def _getProposals(interface):
868+ collection = removeSecurityProxy(interface(self))
869+ collection = collection.visibleByUser(visible_by_user)
870+ return collection.getMergeProposals(status, eager_load=False)
871+
872+ proposals = _getProposals(IBranchCollection).union(
873+ _getProposals(IGitCollection))
874+ if not eager_load:
875+ return proposals
876+ else:
877+ loader = partial(
878+ BranchMergeProposal.preloadDataForBMPs, user=visible_by_user)
879+ return DecoratedResultSet(proposals, pre_iter_hook=loader)
880
881
882 class HasRequestedReviewsMixin:
883@@ -66,6 +88,34 @@
884 visible_by_user)
885 return visible_branches.getMergeProposalsForReviewer(self, status)
886
887+ def getOwnedAndRequestedReviews(self, status=None, visible_by_user=None,
888+ project=None, eager_load=False):
889+ """See `IHasRequestedReviews`."""
890+ # Circular import.
891+ from lp.code.model.branchmergeproposal import BranchMergeProposal
892+
893+ if not status:
894+ status = (BranchMergeProposalStatus.NEEDS_REVIEW,)
895+
896+ def _getProposals(collection):
897+ collection = collection.visibleByUser(visible_by_user)
898+ return collection.getMergeProposalsForPerson(
899+ self, status, eager_load=False)
900+
901+ bzr_collection = removeSecurityProxy(getUtility(IAllBranches))
902+ git_collection = removeSecurityProxy(getUtility(IAllGitRepositories))
903+ if project is not None:
904+ bzr_collection = bzr_collection.inProduct(project)
905+ git_collection = git_collection.inProject(project)
906+ proposals = _getProposals(bzr_collection).union(
907+ _getProposals(git_collection))
908+ if not eager_load:
909+ return proposals
910+ else:
911+ loader = partial(
912+ BranchMergeProposal.preloadDataForBMPs, user=visible_by_user)
913+ return DecoratedResultSet(proposals, pre_iter_hook=loader)
914+
915
916 class HasCodeImportsMixin:
917
918
919=== modified file 'lib/lp/code/templates/branchmergeproposal-listing.pt'
920--- lib/lp/code/templates/branchmergeproposal-listing.pt 2009-09-14 02:04:02 +0000
921+++ lib/lp/code/templates/branchmergeproposal-listing.pt 2015-05-13 09:36:31 +0000
922@@ -35,10 +35,10 @@
923 <td>
924 <a tal:attributes="href proposal/fmt:url">
925 <strong>
926- <tal:source-branch replace="proposal/source_branch/bzr_identity"/>
927+ <tal:merge-source replace="proposal/merge_source/identity"/>
928 </strong>
929 &rArr;
930- <tal:source-branch replace="proposal/target_branch/bzr_identity"/>
931+ <tal:merge-target replace="proposal/merge_target/identity"/>
932 </a>
933 </td>
934 <td tal:attributes="class string:mergestatus${proposal/queue_status/name}"
935
936=== modified file 'lib/lp/code/templates/branchmergeproposal-macros.pt'
937--- lib/lp/code/templates/branchmergeproposal-macros.pt 2010-01-10 21:40:41 +0000
938+++ lib/lp/code/templates/branchmergeproposal-macros.pt 2015-05-13 09:36:31 +0000
939@@ -45,10 +45,10 @@
940 <td>
941 <a tal:attributes="href proposal/fmt:url">
942 <strong>
943- <tal:source-branch replace="proposal/source_branch/bzr_identity"/>
944+ <tal:merge-source replace="proposal/merge_source/identity"/>
945 </strong>
946 &rArr;
947- <tal:source-branch replace="proposal/target_branch/bzr_identity"/>
948+ <tal:merge-target replace="proposal/merge_target/identity"/>
949 </a>
950 </td>
951 <td>