launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #18551
Re: [Merge] lp:~cjwatson/launchpad/git-activereviews into lp:launchpad
Review: Approve code
Diff comments:
> === 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:31:11 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
> # GNU Affero General Public License version 3 (see the file LICENSE).
>
> """Base class view for branch merge proposal listings."""
> @@ -40,10 +40,6 @@
> BranchMergeProposalStatus,
> CodeReviewVote,
> )
> -from lp.code.interfaces.branchcollection import (
> - IAllBranches,
> - IBranchCollection,
> - )
> from lp.code.interfaces.branchmergeproposal import (
> BRANCH_MERGE_PROPOSAL_FINAL_STATES,
> IBranchMergeProposal,
> @@ -168,7 +164,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 +283,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 +319,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 +432,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 +452,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:31:11 +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:31:11 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
> # GNU Affero General Public License version 3 (see the file LICENSE).
>
> """Unit tests for BranchMergeProposal listing views."""
> @@ -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:31:11 +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:31:11 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009 Canonical Ltd. This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
> # GNU Affero General Public License version 3 (see the file LICENSE).
>
> """Interface definitions for IHas<code related bits>."""
> @@ -56,7 +56,7 @@
> title=_('Limit the branches to those modified since this date.'),
> required=False))
> @call_with(visible_by_user=REQUEST_USER)
> - @operation_returns_collection_of(Interface) # Really IBranch.
> + @operation_returns_collection_of(Interface) # Really IBranch.
> @export_read_operation()
> @operation_for_version('beta')
> def getBranches(status=None, visible_by_user=None,
> @@ -88,7 +88,7 @@
> title=_("A list of merge proposal statuses to filter by."),
> value_type=Choice(vocabulary=BranchMergeProposalStatus)))
> @call_with(visible_by_user=REQUEST_USER)
> - @operation_returns_collection_of(Interface) # Really IBranchMergeProposal.
> + @operation_returns_collection_of(Interface) # Really IBranchMergeProposal.
> @export_read_operation()
> @operation_for_version('beta')
> def getMergeProposals(status=None, visible_by_user=None):
> @@ -115,7 +115,7 @@
> title=_("A list of merge proposal statuses to filter by."),
> value_type=Choice(vocabulary=BranchMergeProposalStatus)))
> @call_with(visible_by_user=REQUEST_USER)
> - @operation_returns_collection_of(Interface) # Really IBranchMergeProposal.
> + @operation_returns_collection_of(Interface) # Really IBranchMergeProposal.
> @export_read_operation()
> @operation_for_version('beta')
> def getRequestedReviews(status=None, visible_by_user=None):
> @@ -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.
> @@ -151,7 +166,7 @@
> schema=Interface)
> )
> @call_with(registrant=REQUEST_USER)
> - @export_factory_operation(Interface, []) # Really ICodeImport.
> + @export_factory_operation(Interface, []) # Really ICodeImport.
> @operation_for_version('beta')
> def newCodeImport(registrant=None, branch_name=None, rcs_type=None,
> url=None, cvs_root=None, cvs_module=None, owner=None):
>
> === 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:31:11 +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))
The prereq also wants to be in git_repository_ids. You could also just derive git_repository_ids from git_ref_keys.
> 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:31:11 +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:31:11 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009 Canonical Ltd. This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
> # GNU Affero General Public License version 3 (see the file LICENSE).
>
> """Mixin classes to implement methods for IHas<code related bits>."""
> @@ -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:31:11 +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:31:11 +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>
>
--
https://code.launchpad.net/~cjwatson/launchpad/git-activereviews/+merge/258910
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
References
