launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #15266
[Merge] lp:~stevenk/launchpad/product-merges-preload into lp:launchpad
Steve Kowalik has proposed merging lp:~stevenk/launchpad/product-merges-preload into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1135518 in Launchpad itself: "IHasMergeProposals:+merges times out"
https://bugs.launchpad.net/launchpad/+bug/1135518
For more details, see:
https://code.launchpad.net/~stevenk/launchpad/product-merges-preload/+merge/151410
Make use of the existing eager loading support for Product:+merges, by teaching HasMergeProposalsMixin.getMergeProposals about it.
Perform a massive clean-up in test_branchmergeproposallisting, and a small one in IHasBranches mostly to force this branch to be net-negative.
--
https://code.launchpad.net/~stevenk/launchpad/product-merges-preload/+merge/151410
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/product-merges-preload into lp:launchpad.
=== modified file 'lib/lp/code/browser/branchmergeproposallisting.py'
--- lib/lp/code/browser/branchmergeproposallisting.py 2012-01-01 02:58:52 +0000
+++ lib/lp/code/browser/branchmergeproposallisting.py 2013-03-04 04:42:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2013 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."""
@@ -142,8 +142,8 @@
implements(IBranchMergeProposalListingBatchNavigator)
def __init__(self, view):
- TableBatchNavigator.__init__(
- self, view.getVisibleProposalsForUser(), view.request,
+ super(BranchMergeProposalListingBatchNavigator, self).__init__(
+ view.getVisibleProposalsForUser(), view.request,
columns_to_show=view.extra_columns,
size=config.launchpad.branchlisting_batch_size)
self.view = view
@@ -216,9 +216,7 @@
@property
def initial_values(self):
- return {
- 'status': FilterableStatusValues.ALL,
- }
+ return {'status': FilterableStatusValues.ALL}
@cachedproperty
def status_value(self):
@@ -248,8 +246,8 @@
def getVisibleProposalsForUser(self):
"""Branch merge proposals that are visible by the logged in user."""
- has_proposals = IHasMergeProposals(self.context)
- return has_proposals.getMergeProposals(self.status_filter, self.user)
+ return IHasMergeProposals(self.context).getMergeProposals(
+ self.status_filter, self.user, eager_load=True)
@cachedproperty
def proposal_count(self):
@@ -286,8 +284,7 @@
collection = collection.visibleByUser(self.user)
proposals = collection.getMergeProposals(
[BranchMergeProposalStatus.CODE_APPROVED,
- BranchMergeProposalStatus.NEEDS_REVIEW, ],
- eager_load=True)
+ BranchMergeProposalStatus.NEEDS_REVIEW], eager_load=True)
return proposals
def _getReviewGroup(self, proposal, votes, reviewer):
@@ -445,13 +442,9 @@
def getProposals(self):
"""See `ActiveReviewsView`."""
collection = self._getCollection().visibleByUser(self.user)
- proposals = collection.getMergeProposalsForPerson(
- self._getReviewer(),
- [BranchMergeProposalStatus.CODE_APPROVED,
- BranchMergeProposalStatus.NEEDS_REVIEW],
- eager_load=True)
-
- return proposals
+ return collection.getMergeProposalsForPerson(
+ self._getReviewer(), [BranchMergeProposalStatus.CODE_APPROVED,
+ BranchMergeProposalStatus.NEEDS_REVIEW], eager_load=True)
class PersonProductActiveReviewsView(PersonActiveReviewsView):
=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposallisting.py'
--- lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2012-09-28 02:41:52 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2013-03-04 04:42:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2013 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."""
@@ -18,7 +18,6 @@
from lp.code.browser.branchmergeproposallisting import (
ActiveReviewsView,
BranchMergeProposalListingItem,
- BranchMergeProposalListingView,
)
from lp.code.enums import (
BranchMergeProposalStatus,
@@ -26,7 +25,6 @@
)
from lp.registry.model.personproduct import PersonProduct
from lp.services.database.sqlbase import flush_database_caches
-from lp.services.webapp.servers import LaunchpadTestRequest
from lp.testing import (
ANONYMOUS,
BrowserTestCase,
@@ -65,17 +63,13 @@
if comment is _default:
comment = self.factory.getUniqueString()
proposal.createComment(
- owner=reviewer,
- subject=self.factory.getUniqueString('subject'),
- content=comment,
- vote=vote)
+ owner=reviewer, subject=self.factory.getUniqueString('subject'),
+ content=comment, vote=vote)
def _get_vote_summary(self, proposal):
"""Return the vote summary string for the proposal."""
- view_context = proposal.source_branch.owner
- view = BranchMergeProposalListingView(
- view_context, LaunchpadTestRequest())
- view.initialize()
+ view = create_initialized_view(
+ proposal.source_branch.owner, '+merges', rootsite='code')
batch_navigator = view.proposals
# There will only be one item in the list of proposals.
[listing_item] = batch_navigator.proposals
@@ -105,8 +99,7 @@
summary, comment_count = self._get_vote_summary(proposal)
self.assertEqual(
[{'name': 'APPROVE', 'title':'Approve', 'count':1,
- 'reviewers': ''}],
- summary)
+ 'reviewers': ''}], summary)
self.assertEqual(0, comment_count)
def test_vote_with_comment(self):
@@ -116,8 +109,7 @@
summary, comment_count = self._get_vote_summary(proposal)
self.assertEqual(
[{'name': 'APPROVE', 'title':'Approve', 'count':1,
- 'reviewers': ''}],
- summary)
+ 'reviewers': ''}], summary)
self.assertEqual(1, comment_count)
def test_disapproval(self):
@@ -127,8 +119,7 @@
summary, comment_count = self._get_vote_summary(proposal)
self.assertEqual(
[{'name': 'DISAPPROVE', 'title':'Disapprove', 'count':1,
- 'reviewers': ''}],
- summary)
+ 'reviewers': ''}], summary)
self.assertEqual(1, comment_count)
def test_abstain(self):
@@ -139,8 +130,7 @@
summary, comment_count = self._get_vote_summary(proposal)
self.assertEqual(
[{'name': 'ABSTAIN', 'title':'Abstain', 'count':1,
- 'reviewers': ''}],
- summary)
+ 'reviewers': ''}], summary)
self.assertEqual(1, comment_count)
def test_vote_ranking(self):
@@ -153,8 +143,7 @@
[{'name': 'APPROVE', 'title':'Approve', 'count':1,
'reviewers': ''},
{'name': 'DISAPPROVE', 'title':'Disapprove', 'count':1,
- 'reviewers': ''}],
- summary)
+ 'reviewers': ''}], summary)
self.assertEqual(2, comment_count)
self._createComment(proposal, vote=CodeReviewVote.ABSTAIN)
summary, comment_count = self._get_vote_summary(proposal)
@@ -164,8 +153,7 @@
{'name': 'ABSTAIN', 'title':'Abstain', 'count':1,
'reviewers': ''},
{'name': 'DISAPPROVE', 'title':'Disapprove', 'count':1,
- 'reviewers': ''}],
- summary)
+ 'reviewers': ''}], summary)
self.assertEqual(3, comment_count)
def test_multiple_votes_for_type(self):
@@ -186,8 +174,7 @@
{'name': 'ABSTAIN', 'title':'Abstain', 'count':1,
'reviewers': ''},
{'name': 'DISAPPROVE', 'title':'Disapprove', 'count':2,
- 'reviewers': ''}],
- summary)
+ 'reviewers': ''}], summary)
self.assertEqual(4, comment_count)
@@ -206,6 +193,22 @@
package = self.factory.makeDistributionSourcePackage()
self.getViewBrowser(package, '+merges', rootsite='code')
+ def test_query_count(self):
+ product = self.factory.makeProduct()
+ target = self.factory.makeBranch(
+ product=product, information_type=InformationType.USERDATA)
+ for i in range(7):
+ source = self.factory.makeBranch(
+ product=product, information_type=InformationType.USERDATA)
+ self.factory.makeBranchMergeProposal(
+ source_branch=removeSecurityProxy(source),
+ target_branch=target)
+ flush_database_caches()
+ with StormStatementRecorder() as recorder:
+ self.getViewBrowser(
+ product, '+merges', rootsite='code', user=product.owner)
+ self.assertThat(recorder, HasQueryCount(Equals(40)))
+
class ActiveReviewGroupsTest(TestCaseWithFactory):
"""Tests for groupings used in for active reviews."""
@@ -222,8 +225,8 @@
login(ANONYMOUS)
# The actual context of the view doesn't matter here as all the
# parameters are passed in.
- view = ActiveReviewsView(
- self.factory.makeProduct(), LaunchpadTestRequest())
+ view = create_initialized_view(
+ self.factory.makeProduct(), '+activereviews', rootsite='code')
self.assertEqual(
group, view._getReviewGroup(self.bmp, self.bmp.votes, reviewer))
@@ -405,8 +408,7 @@
target_branch = self.factory.makePackageBranch(
owner=target_branch_owner)
bmp = self.factory.makeBranchMergeProposal(
- reviewer=reviewer,
- target_branch=target_branch)
+ reviewer=reviewer, target_branch=target_branch)
self.setupBMP(bmp)
return bmp
@@ -430,8 +432,7 @@
flush_database_caches()
with StormStatementRecorder() as recorder:
view = create_initialized_view(
- user, name='+activereviews', rootsite='code',
- principal=user)
+ user, name='+activereviews', rootsite='code', principal=user)
view.render()
return recorder, view
=== modified file 'lib/lp/code/model/hasbranches.py'
--- lib/lp/code/model/hasbranches.py 2011-03-03 01:13:47 +0000
+++ lib/lp/code/model/hasbranches.py 2013-03-04 04:42:21 +0000
@@ -41,7 +41,8 @@
class HasMergeProposalsMixin:
"""A mixin implementation class for `IHasMergeProposals`."""
- def getMergeProposals(self, status=None, visible_by_user=None):
+ def getMergeProposals(self, status=None, visible_by_user=None,
+ eager_load=False):
"""See `IHasMergeProposals`."""
if not status:
status = (
@@ -50,7 +51,7 @@
BranchMergeProposalStatus.WORK_IN_PROGRESS)
collection = IBranchCollection(self).visibleByUser(visible_by_user)
- return collection.getMergeProposals(status)
+ return collection.getMergeProposals(status, eager_load=eager_load)
class HasRequestedReviewsMixin:
@@ -63,9 +64,7 @@
visible_branches = getUtility(IAllBranches).visibleByUser(
visible_by_user)
- proposals = visible_branches.getMergeProposalsForReviewer(
- self, status)
- return proposals
+ return visible_branches.getMergeProposalsForReviewer(self, status)
class HasCodeImportsMixin:
