launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #18540
[Merge] lp:~cjwatson/launchpad/git-activereviews into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/git-activereviews into lp:launchpad.
Commit message:
Extend merge proposal listings to cover Git, and add GitRef:+activereviews.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1453020 in Launchpad itself: "Please add support for +activereviews for git branches"
https://bugs.launchpad.net/launchpad/+bug/1453020
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-activereviews/+merge/258910
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.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-activereviews into lp:launchpad.
=== modified file 'lib/lp/code/browser/branchmergeproposallisting.py'
--- lib/lp/code/browser/branchmergeproposallisting.py 2014-12-08 00:32:30 +0000
+++ lib/lp/code/browser/branchmergeproposallisting.py 2015-05-12 17:28:52 +0000
@@ -50,6 +50,10 @@
IBranchMergeProposalGetter,
IBranchMergeProposalListingBatchNavigator,
)
+from lp.code.interfaces.gitcollection import (
+ IAllGitRepositories,
+ IGitCollection,
+ )
from lp.code.interfaces.hasbranches import IHasMergeProposals
from lp.services.config import config
from lp.services.propertycache import (
@@ -168,7 +172,7 @@
@cachedproperty
def proposals(self):
- """Return a list of BranchListingItems."""
+ """Return a list of BranchMergeProposalListingItems."""
proposals = self._proposals_for_current_batch
return [self._createItem(proposal) for proposal in proposals]
@@ -287,12 +291,11 @@
def getProposals(self):
"""Get the proposals for the view."""
- collection = IBranchCollection(self.context)
- collection = collection.visibleByUser(self.user)
- proposals = collection.getMergeProposals(
- [BranchMergeProposalStatus.CODE_APPROVED,
- BranchMergeProposalStatus.NEEDS_REVIEW], eager_load=True)
- return proposals
+ return self.context.getMergeProposals(
+ status=(
+ BranchMergeProposalStatus.CODE_APPROVED,
+ BranchMergeProposalStatus.NEEDS_REVIEW),
+ visible_by_user=self.user, eager_load=True)
def _getReviewGroup(self, proposal, votes, reviewer):
"""One of APPROVED, MINE, TO_DO, CAN_DO, ARE_DOING, OTHER or WIP.
@@ -324,8 +327,8 @@
return self.WIP
if (reviewer is not None and
- (proposal.source_branch.owner == reviewer or
- (reviewer.inTeam(proposal.source_branch.owner) and
+ (proposal.merge_source.owner == reviewer or
+ (reviewer.inTeam(proposal.merge_source.owner) and
proposal.registrant == reviewer))):
return self.MINE
@@ -437,15 +440,13 @@
def _getReviewer(self):
return self.context
- def _getCollection(self):
- return getUtility(IAllBranches)
-
- def getProposals(self):
+ def getProposals(self, project=None):
"""See `ActiveReviewsView`."""
- collection = self._getCollection().visibleByUser(self.user)
- return collection.getMergeProposalsForPerson(
- self._getReviewer(), [BranchMergeProposalStatus.CODE_APPROVED,
- BranchMergeProposalStatus.NEEDS_REVIEW], eager_load=True)
+ return self._getReviewer().getOwnedAndRequestedReviews(
+ status=(
+ BranchMergeProposalStatus.CODE_APPROVED,
+ BranchMergeProposalStatus.NEEDS_REVIEW),
+ visible_by_user=self.user, project=project, eager_load=True)
class PersonProductActiveReviewsView(PersonActiveReviewsView):
@@ -459,8 +460,9 @@
def _getReviewer(self):
return self.context.person
- def _getCollection(self):
- return getUtility(IAllBranches).inProduct(self.context.product)
+ def getProposals(self):
+ return super(PersonProductActiveReviewsView, self).getProposals(
+ project=self.context.product)
@property
def no_proposal_message(self):
=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml 2015-04-28 16:48:22 +0000
+++ lib/lp/code/browser/configure.zcml 2015-05-12 17:28:52 +0000
@@ -844,6 +844,12 @@
name="+register-merge"
permission="launchpad.AnyPerson"
template="../templates/gitref-register-merge.pt"/>
+ <browser:page
+ for="lp.code.interfaces.gitref.IGitRef"
+ class="lp.code.browser.branchmergeproposallisting.BranchActiveReviewsView"
+ permission="zope.Public"
+ name="+activereviews"
+ template="../templates/active-reviews.pt"/>
<browser:menus
classes="GitRefContextMenu"
module="lp.code.browser.gitref"/>
=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposallisting.py'
--- lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2013-04-29 00:10:09 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2015-05-12 17:28:52 +0000
@@ -12,6 +12,7 @@
from testtools.content_type import UTF8_TEXT
from testtools.matchers import Equals
import transaction
+from zope.component import getUtility
from zope.security.proxy import removeSecurityProxy
from lp.app.enums import InformationType
@@ -23,8 +24,14 @@
BranchMergeProposalStatus,
CodeReviewVote,
)
+from lp.code.interfaces.gitref import IGitRef
+from lp.code.interfaces.gitrepository import (
+ GIT_FEATURE_FLAG,
+ IGitRepositorySet,
+ )
from lp.registry.model.personproduct import PersonProduct
from lp.services.database.sqlbase import flush_database_caches
+from lp.services.features.testing import FeatureFixture
from lp.testing import (
ANONYMOUS,
BrowserTestCase,
@@ -45,7 +52,55 @@
_default = object()
-class TestProposalVoteSummary(TestCaseWithFactory):
+class BzrMixin:
+ """Mixin for Bazaar-based tests."""
+
+ def _makeBranch(self, target=None, **kwargs):
+ if target is not None:
+ # This only handles projects at the moment.
+ kwargs["product"] = target
+ return self.factory.makeBranch(**kwargs)
+
+ def _makePackageBranch(self, **kwargs):
+ return self.factory.makePackageBranch(**kwargs)
+
+ def _makeStackedOnBranchChain(self, target=None, **kwargs):
+ if target is not None:
+ # This only handles projects at the moment.
+ kwargs["product"] = target
+ return self.factory.makeStackedOnBranchChain(**kwargs)
+
+ def _makeBranchMergeProposal(self, target=None, merge_target=None,
+ **kwargs):
+ # This only handles projects at the moment.
+ return self.factory.makeBranchMergeProposal(
+ product=target, target_branch=merge_target, **kwargs)
+
+
+class GitMixin:
+ """Mixin for Git-based tests."""
+
+ def setUp(self, user=ANONYMOUS):
+ super(GitMixin, self).setUp(user=user)
+ self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
+
+ def _makeBranch(self, **kwargs):
+ return self.factory.makeGitRefs(**kwargs)[0]
+
+ def _makePackageBranch(self, **kwargs):
+ dsp = self.factory.makeDistributionSourcePackage()
+ return self.factory.makeGitRefs(target=dsp, **kwargs)[0]
+
+ def _makeStackedOnBranchChain(self, depth=None, **kwargs):
+ # Git doesn't have stacked branches. Just make an ordinary reference.
+ return self._makeBranch(**kwargs)
+
+ def _makeBranchMergeProposal(self, merge_target=None, **kwargs):
+ return self.factory.makeBranchMergeProposalForGit(
+ target_ref=merge_target, **kwargs)
+
+
+class TestProposalVoteSummaryMixin:
"""The vote summary shows a summary of the current votes."""
layer = DatabaseFunctionalLayer
@@ -53,7 +108,8 @@
def setUp(self):
# Use an admin so we don't have to worry about launchpad.Edit
# permissions on the merge proposals for adding comments.
- TestCaseWithFactory.setUp(self, user="admin@xxxxxxxxxxxxx")
+ super(TestProposalVoteSummaryMixin, self).setUp(
+ user="admin@xxxxxxxxxxxxx")
def _createComment(self, proposal, reviewer=None, vote=None,
comment=_default):
@@ -69,7 +125,7 @@
def _get_vote_summary(self, proposal):
"""Return the vote summary string for the proposal."""
view = create_initialized_view(
- proposal.source_branch.owner, '+merges', rootsite='code')
+ proposal.merge_source.owner, '+merges', rootsite='code')
batch_navigator = view.proposals
# There will only be one item in the list of proposals.
[listing_item] = batch_navigator.proposals
@@ -78,14 +134,14 @@
def test_no_votes_or_comments(self):
# If there are no votes or comments, then we show that.
- proposal = self.factory.makeBranchMergeProposal()
+ proposal = self._makeBranchMergeProposal()
summary, comment_count = self._get_vote_summary(proposal)
self.assertEqual([], summary)
self.assertEqual(0, comment_count)
def test_no_votes_with_comments(self):
# The comment count is shown.
- proposal = self.factory.makeBranchMergeProposal()
+ proposal = self._makeBranchMergeProposal()
self._createComment(proposal)
summary, comment_count = self._get_vote_summary(proposal)
self.assertEqual([], summary)
@@ -93,7 +149,7 @@
def test_vote_without_comment(self):
# If there are no comments we don't show a count.
- proposal = self.factory.makeBranchMergeProposal()
+ proposal = self._makeBranchMergeProposal()
self._createComment(
proposal, vote=CodeReviewVote.APPROVE, comment=None)
summary, comment_count = self._get_vote_summary(proposal)
@@ -104,7 +160,7 @@
def test_vote_with_comment(self):
# A vote with a comment counts as a vote and a comment.
- proposal = self.factory.makeBranchMergeProposal()
+ proposal = self._makeBranchMergeProposal()
self._createComment(proposal, vote=CodeReviewVote.APPROVE)
summary, comment_count = self._get_vote_summary(proposal)
self.assertEqual(
@@ -114,7 +170,7 @@
def test_disapproval(self):
# Shown as Disapprove: <count>.
- proposal = self.factory.makeBranchMergeProposal()
+ proposal = self._makeBranchMergeProposal()
self._createComment(proposal, vote=CodeReviewVote.DISAPPROVE)
summary, comment_count = self._get_vote_summary(proposal)
self.assertEqual(
@@ -124,7 +180,7 @@
def test_abstain(self):
# Shown as Abstain: <count>.
- proposal = self.factory.makeBranchMergeProposal()
+ proposal = self._makeBranchMergeProposal()
transaction.commit()
self._createComment(proposal, vote=CodeReviewVote.ABSTAIN)
summary, comment_count = self._get_vote_summary(proposal)
@@ -135,7 +191,7 @@
def test_vote_ranking(self):
# Votes go from best to worst.
- proposal = self.factory.makeBranchMergeProposal()
+ proposal = self._makeBranchMergeProposal()
self._createComment(proposal, vote=CodeReviewVote.DISAPPROVE)
self._createComment(proposal, vote=CodeReviewVote.APPROVE)
summary, comment_count = self._get_vote_summary(proposal)
@@ -158,7 +214,7 @@
def test_multiple_votes_for_type(self):
# Multiple votes of a type are aggregated in the summary.
- proposal = self.factory.makeBranchMergeProposal()
+ proposal = self._makeBranchMergeProposal()
self._createComment(proposal, vote=CodeReviewVote.DISAPPROVE)
self._createComment(proposal, vote=CodeReviewVote.APPROVE)
self._createComment(proposal, vote=CodeReviewVote.DISAPPROVE)
@@ -178,6 +234,16 @@
self.assertEqual(4, comment_count)
+class TestProposalVoteSummaryBzr(
+ TestProposalVoteSummaryMixin, BzrMixin, TestCaseWithFactory):
+ """Test the vote summary for Bazaar."""
+
+
+class TestProposalVoteSummaryGit(
+ TestProposalVoteSummaryMixin, GitMixin, TestCaseWithFactory):
+ """Test the vote summary for Git."""
+
+
class TestMerges(BrowserTestCase):
layer = DatabaseFunctionalLayer
@@ -193,7 +259,7 @@
package = self.factory.makeDistributionSourcePackage()
self.getViewBrowser(package, '+merges', rootsite='code')
- def test_query_count(self):
+ def test_query_count_bzr(self):
product = self.factory.makeProduct()
target = self.factory.makeBranch(
product=product, information_type=InformationType.USERDATA)
@@ -209,24 +275,51 @@
product, '+merges', rootsite='code', user=product.owner)
self.assertThat(recorder, HasQueryCount(Equals(41)))
- def test_productseries(self):
+ def test_query_count_git(self):
+ self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
+ product = self.factory.makeProduct()
+ [target] = self.factory.makeGitRefs(
+ target=product, information_type=InformationType.USERDATA)
+ for i in range(7):
+ [source] = self.factory.makeGitRefs(
+ target=product, information_type=InformationType.USERDATA)
+ self.factory.makeBranchMergeProposalForGit(
+ source_ref=source, target_ref=target)
+ flush_database_caches()
+ with StormStatementRecorder() as recorder:
+ self.getViewBrowser(
+ product, '+merges', rootsite='code', user=product.owner)
+ self.assertThat(recorder, HasQueryCount(Equals(38)))
+
+ def test_productseries_bzr(self):
target = self.factory.makeBranch()
- unique_name = target.unique_name
with person_logged_in(target.product.owner):
target.product.development_focus.branch = target
+ identity = target.identity
self.factory.makeBranchMergeProposal(target_branch=target)
view = self.getViewBrowser(target, '+merges', rootsite='code')
- self.assertIn(unique_name, view.contents)
-
-
-class ActiveReviewGroupsTest(TestCaseWithFactory):
- """Tests for groupings used in for active reviews."""
+ self.assertIn(identity, view.contents)
+
+ def test_product_git(self):
+ self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
+ [target] = self.factory.makeGitRefs()
+ with person_logged_in(target.target.owner):
+ getUtility(IGitRepositorySet).setDefaultRepository(
+ target.target, target.repository)
+ identity = target.identity
+ self.factory.makeBranchMergeProposalForGit(target_ref=target)
+ view = self.getViewBrowser(target, '+merges', rootsite='code')
+ self.assertIn(identity, view.contents)
+
+
+class ActiveReviewGroupsTestMixin:
+ """Tests for groupings used for active reviews."""
layer = DatabaseFunctionalLayer
def setUp(self):
- super(ActiveReviewGroupsTest, self).setUp()
- self.bmp = self.factory.makeBranchMergeProposal(
+ super(ActiveReviewGroupsTestMixin, self).setUp()
+ self.bmp = self._makeBranchMergeProposal(
set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
def assertReviewGroupForReviewer(self, reviewer, group):
@@ -247,21 +340,21 @@
def test_approved(self):
# If the proposal is approved, then the group is approved.
- self.bmp = self.factory.makeBranchMergeProposal(
+ self.bmp = self._makeBranchMergeProposal(
set_state=BranchMergeProposalStatus.CODE_APPROVED)
self.assertReviewGroupForReviewer(None, ActiveReviewsView.APPROVED)
def test_work_in_progress(self):
# If the proposal is in progress, then the group is wip.
- self.bmp = self.factory.makeBranchMergeProposal(
+ self.bmp = self._makeBranchMergeProposal(
set_state=BranchMergeProposalStatus.WORK_IN_PROGRESS)
self.assertReviewGroupForReviewer(None, ActiveReviewsView.WIP)
- def test_source_branch_owner(self):
- # If the reviewer is the owner of the source branch, then the review
+ def test_merge_source_owner(self):
+ # If the reviewer is the owner of the merge source, then the review
# is MINE. This occurs whether or not the user is the logged in or
# not.
- reviewer = self.bmp.source_branch.owner
+ reviewer = self.bmp.merge_source.owner
self.assertReviewGroupForReviewer(reviewer, ActiveReviewsView.MINE)
def test_proposal_registrant(self):
@@ -271,13 +364,17 @@
self.assertReviewGroupForReviewer(reviewer, ActiveReviewsView.OTHER)
team = self.factory.makeTeam(self.bmp.registrant)
- removeSecurityProxy(self.bmp.source_branch).owner = team
+ naked_merge_source = removeSecurityProxy(self.bmp.merge_source)
+ if IGitRef.providedBy(naked_merge_source):
+ naked_merge_source.repository.owner = team
+ else:
+ naked_merge_source.owner = team
self.assertReviewGroupForReviewer(reviewer, ActiveReviewsView.MINE)
- def test_target_branch_owner(self):
- # For the target branch owner, it is to_do since they are the default
+ def test_merge_target_owner(self):
+ # For the merge target owner, it is to_do since they are the default
# reviewer.
- reviewer = self.bmp.target_branch.owner
+ reviewer = self.bmp.merge_target.owner
self.assertReviewGroupForReviewer(reviewer, ActiveReviewsView.TO_DO)
def test_group_pending_review(self):
@@ -299,7 +396,7 @@
def test_review_done(self):
# If the logged in user has a completed review, then the review is
# ARE_DOING.
- reviewer = self.bmp.target_branch.owner
+ reviewer = self.bmp.merge_target.owner
login_person(reviewer)
self.bmp.createComment(
reviewer, 'subject', vote=CodeReviewVote.APPROVE)
@@ -307,7 +404,17 @@
reviewer, ActiveReviewsView.ARE_DOING)
-class TestBranchMergeProposalListingItem(TestCaseWithFactory):
+class ActiveReviewGroupsTestBzr(
+ ActiveReviewGroupsTestMixin, BzrMixin, TestCaseWithFactory):
+ """Tests for groupings used for active reviews for Bazaar."""
+
+
+class ActiveReviewGroupsTestGit(
+ ActiveReviewGroupsTestMixin, GitMixin, TestCaseWithFactory):
+ """Tests for groupings used for active reviews for Git."""
+
+
+class TestBranchMergeProposalListingItemMixin:
"""Tests specifically relating to the BranchMergeProposalListingItem."""
layer = DatabaseFunctionalLayer
@@ -358,7 +465,17 @@
self.assertEqual(bmp.date_created, item.sort_key)
-class ActiveReviewSortingTest(TestCaseWithFactory):
+class TestBranchMergeProposalListingItemBzr(
+ TestBranchMergeProposalListingItemMixin, BzrMixin, TestCaseWithFactory):
+ """Test BranchMergeProposalListingItem for Bazaar."""
+
+
+class TestBranchMergeProposalListingItemGit(
+ TestBranchMergeProposalListingItemMixin, GitMixin, TestCaseWithFactory):
+ """Test BranchMergeProposalListingItem for Git."""
+
+
+class ActiveReviewSortingTestMixin:
"""Test the sorting of the active review groups."""
layer = DatabaseFunctionalLayer
@@ -366,14 +483,14 @@
def test_oldest_first(self):
# The oldest requested reviews should be first.
product = self.factory.makeProduct()
- bmp1 = self.factory.makeBranchMergeProposal(product=product)
- login_person(bmp1.source_branch.owner)
+ bmp1 = self._makeBranchMergeProposal(target=product)
+ login_person(bmp1.merge_source.owner)
bmp1.requestReview(datetime(2009, 6, 1, tzinfo=pytz.UTC))
- bmp2 = self.factory.makeBranchMergeProposal(product=product)
- login_person(bmp2.source_branch.owner)
+ bmp2 = self._makeBranchMergeProposal(target=product)
+ login_person(bmp2.merge_source.owner)
bmp2.requestReview(datetime(2009, 3, 1, tzinfo=pytz.UTC))
- bmp3 = self.factory.makeBranchMergeProposal(product=product)
- login_person(bmp3.source_branch.owner)
+ bmp3 = self._makeBranchMergeProposal(target=product)
+ login_person(bmp3.merge_source.owner)
bmp3.requestReview(datetime(2009, 1, 1, tzinfo=pytz.UTC))
login(ANONYMOUS)
view = create_initialized_view(
@@ -383,8 +500,18 @@
[item.context for item in view.review_groups[view.OTHER]])
-class ActiveReviewsWithPrivateBranches(TestCaseWithFactory):
- """Test the sorting of the active review groups."""
+class ActiveReviewSortingTestBzr(
+ ActiveReviewSortingTestMixin, BzrMixin, TestCaseWithFactory):
+ """Test the sorting of the active review groups for Bazaar."""
+
+
+class ActiveReviewSortingTestGit(
+ ActiveReviewSortingTestMixin, GitMixin, TestCaseWithFactory):
+ """Test the sorting of the active review groups for Git."""
+
+
+class ActiveReviewsWithPrivateBranchesMixin:
+ """Test reviews of private branches."""
layer = DatabaseFunctionalLayer
@@ -392,32 +519,42 @@
# Merge proposals against private branches are visible to
# the branch owner.
product = self.factory.makeProduct()
- branch = self.factory.makeBranch(
- product=product, information_type=InformationType.USERDATA)
+ branch = self._makeBranch(
+ target=product, information_type=InformationType.USERDATA)
with person_logged_in(removeSecurityProxy(branch).owner):
- mp = self.factory.makeBranchMergeProposal(target_branch=branch)
+ mp = self._makeBranchMergeProposal(merge_target=branch)
view = create_initialized_view(
branch, name='+activereviews', rootsite='code')
self.assertEqual([mp], list(view.getProposals()))
-class PersonActiveReviewsPerformance(TestCaseWithFactory):
+class ActiveReviewsWithPrivateBranchesBzr(
+ ActiveReviewsWithPrivateBranchesMixin, BzrMixin, TestCaseWithFactory):
+ """Test reviews of private Bazaar branches."""
+
+
+class ActiveReviewsWithPrivateBranchesGit(
+ ActiveReviewsWithPrivateBranchesMixin, GitMixin, TestCaseWithFactory):
+ """Test reviews of references in private Git repositories."""
+
+
+class PersonActiveReviewsPerformanceMixin:
"""Test the performance of the person's active reviews page."""
layer = LaunchpadFunctionalLayer
def setupBMP(self, bmp):
self.factory.makePreviewDiff(merge_proposal=bmp)
- login_person(bmp.source_branch.owner)
+ login_person(bmp.merge_source.owner)
bmp.requestReview()
- def createUserBMP(self, reviewer=None, target_branch_owner=None):
- target_branch = None
- if target_branch_owner is not None:
- target_branch = self.factory.makePackageBranch(
- owner=target_branch_owner)
- bmp = self.factory.makeBranchMergeProposal(
- reviewer=reviewer, target_branch=target_branch)
+ def createUserBMP(self, reviewer=None, merge_target_owner=None):
+ merge_target = None
+ if merge_target_owner is not None:
+ merge_target = self._makePackageBranch(
+ owner=merge_target_owner)
+ bmp = self._makeBranchMergeProposal(
+ reviewer=reviewer, merge_target=merge_target)
self.setupBMP(bmp)
return bmp
@@ -431,10 +568,9 @@
# Create one of the two types of BMP which will be displayed
# on a person's +activereviews page:
# - A BMP for which the person is the reviewer.
- # - A BMP for which the person is the owner of the target
- # branch.
+ # - A BMP for which the person is the owner of the merge target.
if i % 2 == 0:
- self.createUserBMP(target_branch_owner=user)
+ self.createUserBMP(merge_target_owner=user)
else:
self.createUserBMP(reviewer=user)
login_person(user)
@@ -457,9 +593,9 @@
self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
def createProductBMP(self, product):
- target_branch = self.factory.makeStackedOnBranchChain(product=product)
- bmp = self.factory.makeBranchMergeProposal(
- product=product, target_branch=target_branch)
+ merge_target = self._makeStackedOnBranchChain(target=product)
+ bmp = self._makeBranchMergeProposal(
+ target=product, merge_target=merge_target)
self.setupBMP(bmp)
return bmp
@@ -491,3 +627,13 @@
base_bmps + added_bmps)
self.assertEqual(base_bmps + added_bmps, view2.proposal_count)
self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
+
+
+class PersonActiveReviewsPerformanceBzr(
+ PersonActiveReviewsPerformanceMixin, BzrMixin, TestCaseWithFactory):
+ """Test the performance of the person's active reviews page for Bazaar."""
+
+
+class PersonActiveReviewsPerformanceGit(
+ PersonActiveReviewsPerformanceMixin, GitMixin, TestCaseWithFactory):
+ """Test the performance of the person's active reviews page for Git."""
=== modified file 'lib/lp/code/interfaces/gitref.py'
--- lib/lp/code/interfaces/gitref.py 2015-04-24 12:58:46 +0000
+++ lib/lp/code/interfaces/gitref.py 2015-05-12 17:28:52 +0000
@@ -43,11 +43,12 @@
BranchMergeProposalStatus,
GitObjectType,
)
+from lp.code.interfaces.hasbranches import IHasMergeProposals
from lp.registry.interfaces.person import IPerson
from lp.services.webapp.interfaces import ITableBatchNavigator
-class IGitRef(Interface):
+class IGitRef(IHasMergeProposals):
"""A reference in a Git repository."""
# XXX cjwatson 2015-01-19 bug=760849: "beta" is a lie to get WADL
=== modified file 'lib/lp/code/interfaces/hasbranches.py'
--- lib/lp/code/interfaces/hasbranches.py 2013-01-07 02:40:55 +0000
+++ lib/lp/code/interfaces/hasbranches.py 2015-05-12 17:28:52 +0000
@@ -130,6 +130,21 @@
:returns: A list of `IBranchMergeProposal`.
"""
+ def getOwnedAndRequestedReviews(status=None, visible_by_user=None,
+ project=None):
+ """Returns merge proposals for branches owned by a person, or where
+ that person was asked to review.
+
+ This does not include merge proposals that were requested from
+ teams that the person is part of. If status is not passed then
+ it will return proposals that are in the "Needs Review" state.
+
+ :param status: A list of statuses to filter with.
+ :param visible_by_user: Normally the user who is asking.
+ :param project: Limit results to branches for this `IProduct`.
+ :returns: A list of `IBranchMergeProposal`.
+ """
+
class IHasCodeImports(Interface):
"""Some things can have code imports that target them.
=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py 2015-04-22 16:42:57 +0000
+++ lib/lp/code/model/branchmergeproposal.py 2015-05-12 17:28:52 +0000
@@ -92,7 +92,10 @@
from lp.registry.interfaces.product import IProduct
from lp.registry.model.person import Person
from lp.services.config import config
-from lp.services.database.bulk import load_related
+from lp.services.database.bulk import (
+ load,
+ load_related,
+ )
from lp.services.database.constants import (
DEFAULT,
UTC_NOW,
@@ -1055,18 +1058,29 @@
from lp.code.model.branch import Branch
from lp.code.model.branchcollection import GenericBranchCollection
from lp.code.model.gitcollection import GenericGitCollection
+ from lp.code.model.gitref import GitRef
from lp.code.model.gitrepository import GitRepository
ids = set()
source_branch_ids = set()
- source_git_repository_ids = set()
+ git_repository_ids = set()
person_ids = set()
+ git_ref_keys = set()
for mp in branch_merge_proposals:
ids.add(mp.id)
if mp.source_branchID is not None:
source_branch_ids.add(mp.source_branchID)
if mp.source_git_repositoryID is not None:
- source_git_repository_ids.add(mp.source_git_repositoryID)
+ git_repository_ids.add(mp.source_git_repositoryID)
+ git_repository_ids.add(mp.target_git_repositoryID)
+ git_ref_keys.add(
+ (mp.source_git_repositoryID, mp.source_git_path))
+ git_ref_keys.add(
+ (mp.target_git_repositoryID, mp.target_git_path))
+ if mp.prerequisite_git_repositoryID is not None:
+ git_ref_keys.add(
+ (mp.prerequisite_git_repositoryID,
+ mp.prerequisite_git_path))
person_ids.add(mp.registrantID)
person_ids.add(mp.merge_reporterID)
@@ -1078,9 +1092,11 @@
GitRepository, branch_merge_proposals, (
"target_git_repositoryID", "prerequisite_git_repositoryID",
"source_git_repositoryID"))
+ load(GitRef, git_ref_keys)
# The stacked on branches are used to check branch visibility.
GenericBranchCollection.preloadVisibleStackedOnBranches(
branches, user)
+ GenericGitCollection.preloadVisibleRepositories(repositories, user)
if len(branches) == 0 and len(repositories) == 0:
return
@@ -1098,21 +1114,24 @@
cache.preview_diff = previewdiff
# Add source branch/repository owners' to the list of pre-loaded
- # persons.
+ # persons. We need the target repository owner as well; unlike
+ # branches, repository unique names aren't trigger-maintained.
person_ids.update(
branch.ownerID for branch in branches
if branch.id in source_branch_ids)
person_ids.update(
repository.owner_id for repository in repositories
- if repository.id in source_git_repository_ids)
+ if repository.id in git_repository_ids)
# Pre-load Person and ValidPersonCache.
list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
person_ids, need_validity=True))
# Pre-load branches'/repositories' data.
- GenericBranchCollection.preloadDataForBranches(branches)
- GenericGitCollection.preloadDataForRepositories(repositories)
+ if branches:
+ GenericBranchCollection.preloadDataForBranches(branches)
+ if repositories:
+ GenericGitCollection.preloadDataForRepositories(repositories)
class BranchMergeProposalGetter:
=== modified file 'lib/lp/code/model/gitcollection.py'
--- lib/lp/code/model/gitcollection.py 2015-04-22 16:42:57 +0000
+++ lib/lp/code/model/gitcollection.py 2015-05-12 17:28:52 +0000
@@ -9,6 +9,7 @@
]
from functools import partial
+from operator import attrgetter
from lazr.uri import (
InvalidURIError,
@@ -178,6 +179,20 @@
return []
@staticmethod
+ def preloadVisibleRepositories(repositories, user=None):
+ """Preload visibility for the given list of repositories."""
+ if len(repositories) == 0:
+ return
+ expressions = [
+ GitRepository.id.is_in(map(attrgetter("id"), repositories))]
+ if user is None:
+ collection = AnonymousGitCollection(filter_expressions=expressions)
+ else:
+ collection = VisibleGitCollection(
+ user=user, filter_expressions=expressions)
+ return list(collection.getRepositories())
+
+ @staticmethod
def preloadDataForRepositories(repositories):
"""Preload repositories' cached associated targets."""
load_related(Distribution, repositories, ['distribution_id'])
=== modified file 'lib/lp/code/model/hasbranches.py'
--- lib/lp/code/model/hasbranches.py 2013-03-04 04:17:17 +0000
+++ lib/lp/code/model/hasbranches.py 2015-05-12 17:28:52 +0000
@@ -11,7 +11,10 @@
'HasRequestedReviewsMixin',
]
+from functools import partial
+
from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
from lp.code.enums import BranchMergeProposalStatus
from lp.code.interfaces.branch import DEFAULT_BRANCH_STATUS_IN_LISTING
@@ -20,6 +23,11 @@
IBranchCollection,
)
from lp.code.interfaces.branchtarget import IBranchTarget
+from lp.code.interfaces.gitcollection import (
+ IAllGitRepositories,
+ IGitCollection,
+ )
+from lp.services.database.decoratedresultset import DecoratedResultSet
class HasBranchesMixin:
@@ -44,14 +52,28 @@
def getMergeProposals(self, status=None, visible_by_user=None,
eager_load=False):
"""See `IHasMergeProposals`."""
+ # Circular import.
+ from lp.code.model.branchmergeproposal import BranchMergeProposal
+
if not status:
status = (
BranchMergeProposalStatus.CODE_APPROVED,
BranchMergeProposalStatus.NEEDS_REVIEW,
BranchMergeProposalStatus.WORK_IN_PROGRESS)
- collection = IBranchCollection(self).visibleByUser(visible_by_user)
- return collection.getMergeProposals(status, eager_load=eager_load)
+ def _getProposals(interface):
+ collection = removeSecurityProxy(interface(self))
+ collection = collection.visibleByUser(visible_by_user)
+ return collection.getMergeProposals(status, eager_load=False)
+
+ proposals = _getProposals(IBranchCollection).union(
+ _getProposals(IGitCollection))
+ if not eager_load:
+ return proposals
+ else:
+ loader = partial(
+ BranchMergeProposal.preloadDataForBMPs, user=visible_by_user)
+ return DecoratedResultSet(proposals, pre_iter_hook=loader)
class HasRequestedReviewsMixin:
@@ -66,6 +88,34 @@
visible_by_user)
return visible_branches.getMergeProposalsForReviewer(self, status)
+ def getOwnedAndRequestedReviews(self, status=None, visible_by_user=None,
+ project=None, eager_load=False):
+ """See `IHasRequestedReviews`."""
+ # Circular import.
+ from lp.code.model.branchmergeproposal import BranchMergeProposal
+
+ if not status:
+ status = (BranchMergeProposalStatus.NEEDS_REVIEW,)
+
+ def _getProposals(collection):
+ collection = collection.visibleByUser(visible_by_user)
+ return collection.getMergeProposalsForPerson(
+ self, status, eager_load=False)
+
+ bzr_collection = removeSecurityProxy(getUtility(IAllBranches))
+ git_collection = removeSecurityProxy(getUtility(IAllGitRepositories))
+ if project is not None:
+ bzr_collection = bzr_collection.inProduct(project)
+ git_collection = git_collection.inProject(project)
+ proposals = _getProposals(bzr_collection).union(
+ _getProposals(git_collection))
+ if not eager_load:
+ return proposals
+ else:
+ loader = partial(
+ BranchMergeProposal.preloadDataForBMPs, user=visible_by_user)
+ return DecoratedResultSet(proposals, pre_iter_hook=loader)
+
class HasCodeImportsMixin:
=== modified file 'lib/lp/code/templates/branchmergeproposal-listing.pt'
--- lib/lp/code/templates/branchmergeproposal-listing.pt 2009-09-14 02:04:02 +0000
+++ lib/lp/code/templates/branchmergeproposal-listing.pt 2015-05-12 17:28:52 +0000
@@ -35,10 +35,10 @@
<td>
<a tal:attributes="href proposal/fmt:url">
<strong>
- <tal:source-branch replace="proposal/source_branch/bzr_identity"/>
+ <tal:merge-source replace="proposal/merge_source/identity"/>
</strong>
⇒
- <tal:source-branch replace="proposal/target_branch/bzr_identity"/>
+ <tal:merge-target replace="proposal/merge_target/identity"/>
</a>
</td>
<td tal:attributes="class string:mergestatus${proposal/queue_status/name}"
=== modified file 'lib/lp/code/templates/branchmergeproposal-macros.pt'
--- lib/lp/code/templates/branchmergeproposal-macros.pt 2010-01-10 21:40:41 +0000
+++ lib/lp/code/templates/branchmergeproposal-macros.pt 2015-05-12 17:28:52 +0000
@@ -45,10 +45,10 @@
<td>
<a tal:attributes="href proposal/fmt:url">
<strong>
- <tal:source-branch replace="proposal/source_branch/bzr_identity"/>
+ <tal:merge-source replace="proposal/merge_source/identity"/>
</strong>
⇒
- <tal:source-branch replace="proposal/target_branch/bzr_identity"/>
+ <tal:merge-target replace="proposal/merge_target/identity"/>
</a>
</td>
<td>
Follow ups
