launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04784
[Merge] lp:~danilo/launchpad/bug-826692-take2 into lp:launchpad
Данило Шеган has proposed merging lp:~danilo/launchpad/bug-826692-take2 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #826692 in Launchpad itself: "+activereviews times out with a lot of merge proposals"
https://bugs.launchpad.net/launchpad/+bug/826692
For more details, see:
https://code.launchpad.net/~danilo/launchpad/bug-826692-take2/+merge/72996
= Bug 826692: resubmit =
This has mostly been reviewed in https://code.launchpad.net/~danilo/launchpad/bug-826692/+merge/71836
However, that didn't work for private branches. The only change (other than a few more lint fixes) is the one below:
== Proposed fix ==
http://paste.ubuntu.com/675046/
== Tests ==
bin/test -cvvt ActiveReviewsWithPrivateBranches -t TestLandingCandidates
(TestLandingCandidates was introduced when the landing was reverted)
== Demo and Q/A ==
Go to a Landscape branch lp:landscape and look at merge proposals page for it (note, go to the branch, not the project page). The number of proposals there should match the number listed on the branch page.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/code/model/branchcollection.py
lib/lp/code/interfaces/branch.py
lib/lp/code/interfaces/branchcollection.py
lib/lp/code/model/branch.py
lib/lp/code/browser/branchmergeproposallisting.py
lib/lp/code/browser/tests/test_branchmergeproposallisting.py
--
https://code.launchpad.net/~danilo/launchpad/bug-826692-take2/+merge/72996
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/bug-826692-take2 into lp:launchpad.
=== modified file 'lib/lp/code/browser/branchmergeproposallisting.py'
--- lib/lp/code/browser/branchmergeproposallisting.py 2011-08-25 13:49:26 +0000
+++ lib/lp/code/browser/branchmergeproposallisting.py 2011-08-26 06:10:01 +0000
@@ -47,6 +47,7 @@
IBranchCollection,
)
from lp.code.interfaces.branchmergeproposal import (
+ BRANCH_MERGE_PROPOSAL_FINAL_STATES,
IBranchMergeProposal,
IBranchMergeProposalGetter,
IBranchMergeProposalListingBatchNavigator,
@@ -419,7 +420,11 @@
def getProposals(self):
"""See `ActiveReviewsView`."""
- candidates = self.context.landing_candidates
+ non_final = tuple(
+ set(BranchMergeProposalStatus.items) -
+ set(BRANCH_MERGE_PROPOSAL_FINAL_STATES))
+ candidates = self.context.getMergeProposals(
+ status=non_final, eager_load=True, visible_by_user=self.user)
return [proposal for proposal in candidates
if check_permission('launchpad.View', proposal)]
=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposallisting.py'
--- lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2011-08-12 11:37:08 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2011-08-26 06:10:01 +0000
@@ -28,6 +28,7 @@
BrowserTestCase,
login,
login_person,
+ person_logged_in,
TestCaseWithFactory,
)
from lp.testing.views import create_initialized_view
@@ -359,3 +360,19 @@
self.assertEqual(
[bmp3, bmp2, bmp1],
[item.context for item in view.review_groups[view.OTHER]])
+
+
+class ActiveReviewsWithPrivateBranches(TestCaseWithFactory):
+ """Test the sorting of the active review groups."""
+
+ layer = DatabaseFunctionalLayer
+
+ def test_private_branch_owner(self):
+ # Merge proposals against private branches are visible to branch owner.
+ product = self.factory.makeProduct()
+ branch = self.factory.makeBranch(private=True, product=product)
+ with person_logged_in(removeSecurityProxy(branch).owner):
+ mp = self.factory.makeBranchMergeProposal(target_branch=branch)
+ view = create_initialized_view(
+ branch, name='+activereviews', rootsite='code')
+ self.assertEqual([mp], list(view.getProposals()))
=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py 2011-08-25 13:49:26 +0000
+++ lib/lp/code/interfaces/branch.py 2011-08-26 06:10:01 +0000
@@ -621,7 +621,7 @@
@export_read_operation()
@operation_for_version('beta')
def getMergeProposals(status=None, visible_by_user=None,
- merged_revnos=None):
+ merged_revnos=None, eager_load=False):
"""Return matching BranchMergeProposals."""
def scheduleDiffUpdates():
=== modified file 'lib/lp/code/interfaces/branchcollection.py'
--- lib/lp/code/interfaces/branchcollection.py 2011-08-25 13:49:26 +0000
+++ lib/lp/code/interfaces/branchcollection.py 2011-08-26 06:10:01 +0000
@@ -70,7 +70,7 @@
"""
def getMergeProposals(statuses=None, for_branches=None,
- target_branch=None):
+ target_branch=None, eager_load=False):
"""Return a result set of merge proposals for the branches in this
collection.
@@ -81,6 +81,8 @@
branch is one of the branches specified.
:param target_branch: If specified, only return merge proposals
that target the specified branch.
+ :param eager_load: If True, preloads all the related information for
+ merge proposals like PreviewDiffs and Branches.
"""
def getMergeProposalsForPerson(person, status=None):
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py 2011-08-25 13:49:26 +0000
+++ lib/lp/code/model/branch.py 2011-08-26 06:10:01 +0000
@@ -376,7 +376,7 @@
""" % sqlvalues(self, BRANCH_MERGE_PROPOSAL_FINAL_STATES))
def getMergeProposals(self, status=None, visible_by_user=None,
- merged_revnos=None):
+ merged_revnos=None, eager_load=False):
"""See `IBranch`."""
if not status:
status = (
@@ -386,7 +386,8 @@
collection = getUtility(IAllBranches).visibleByUser(visible_by_user)
return collection.getMergeProposals(
- status, target_branch=self, merged_revnos=merged_revnos)
+ status, target_branch=self, merged_revnos=merged_revnos,
+ eager_load=eager_load)
def isBranchMergeable(self, target_branch):
"""See `IBranch`."""
=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py 2011-08-25 13:49:26 +0000
+++ lib/lp/code/model/branchcollection.py 2011-08-26 06:10:01 +0000
@@ -63,12 +63,17 @@
from lp.code.model.codeimport import CodeImport
from lp.code.model.codereviewcomment import CodeReviewComment
from lp.code.model.codereviewvote import CodeReviewVoteReference
+from lp.code.model.diff import (
+ Diff,
+ PreviewDiff,
+ )
from lp.code.model.seriessourcepackagebranch import SeriesSourcePackageBranch
from lp.registry.model.distribution import Distribution
from lp.registry.model.distroseries import DistroSeries
from lp.registry.model.person import (
Owner,
Person,
+ ValidPersonCache,
)
from lp.registry.model.product import Product
from lp.registry.model.sourcepackagename import SourcePackageName
@@ -152,9 +157,9 @@
exclude_from_search=None, symmetric=True):
"""Return a subset of this collection, filtered by 'expressions'.
- :param symmetric: If True this filter will apply to both sides of merge
- proposal lookups and any other lookups that join Branch back onto
- Branch.
+ :param symmetric: If True this filter will apply to both sides
+ of merge proposal lookups and any other lookups that join
+ Branch back onto Branch.
"""
# NOTE: JonathanLange 2009-02-17: We might be able to avoid the need
# for explicit 'tables' by harnessing Storm's table inference system.
@@ -175,7 +180,8 @@
if table is not None:
asymmetric_tables[table] = join
symmetric_expr = list(self._branch_filter_expressions)
- asymmetric_expr = self._asymmetric_filter_expressions + expressions
+ asymmetric_expr = (
+ self._asymmetric_filter_expressions + expressions)
if exclude_from_search is None:
exclude_from_search = []
return self.__class__(
@@ -205,12 +211,48 @@
def _getCandidateBranchesWith(self):
"""Return WITH clauses defining candidate branches.
- These are defined in terms of scope_branches which should be separately
- calculated.
+ These are defined in terms of scope_branches which should be
+ separately calculated.
"""
return [
With("candidate_branches", SQL("SELECT id from scope_branches"))]
+ def _preloadDataForBranches(self, branches):
+ """Preload branches cached associated product series and
+ suite source packages."""
+ caches = dict((branch.id, get_property_cache(branch))
+ for branch in branches)
+ branch_ids = caches.keys()
+ for cache in caches.values():
+ if not safe_hasattr(cache, '_associatedProductSeries'):
+ cache._associatedProductSeries = []
+ if not safe_hasattr(cache, '_associatedSuiteSourcePackages'):
+ cache._associatedSuiteSourcePackages = []
+ if not safe_hasattr(cache, 'code_import'):
+ cache.code_import = None
+ # associatedProductSeries
+ # Imported here to avoid circular import.
+ from lp.registry.model.productseries import ProductSeries
+ for productseries in self.store.find(
+ ProductSeries,
+ ProductSeries.branchID.is_in(branch_ids)):
+ cache = caches[productseries.branchID]
+ cache._associatedProductSeries.append(productseries)
+ # associatedSuiteSourcePackages
+ series_set = getUtility(IFindOfficialBranchLinks)
+ # Order by the pocket to get the release one first. If changing
+ # this be sure to also change BranchCollection.getBranches.
+ links = series_set.findForBranches(branches).order_by(
+ SeriesSourcePackageBranch.pocket)
+ for link in links:
+ cache = caches[link.branchID]
+ cache._associatedSuiteSourcePackages.append(
+ link.suite_sourcepackage)
+ for code_import in IStore(CodeImport).find(
+ CodeImport, CodeImport.branchID.is_in(branch_ids)):
+ cache = caches[code_import.branchID]
+ cache.code_import = code_import
+
def getBranches(self, eager_load=False):
"""See `IBranchCollection`."""
all_tables = set(
@@ -225,53 +267,23 @@
branch_ids = set(branch.id for branch in rows)
if not branch_ids:
return
- branches = dict((branch.id, branch) for branch in rows)
- caches = dict((branch.id, get_property_cache(branch))
- for branch in rows)
- for cache in caches.values():
- if not safe_hasattr(cache, '_associatedProductSeries'):
- cache._associatedProductSeries = []
- if not safe_hasattr(cache, '_associatedSuiteSourcePackages'):
- cache._associatedSuiteSourcePackages = []
- if not safe_hasattr(cache, 'code_import'):
- cache.code_import = None
- # associatedProductSeries
- # Imported here to avoid circular import.
- from lp.registry.model.productseries import ProductSeries
- for productseries in self.store.find(
- ProductSeries,
- ProductSeries.branchID.is_in(branch_ids)):
- cache = caches[productseries.branchID]
- cache._associatedProductSeries.append(productseries)
- # associatedSuiteSourcePackages
- series_set = getUtility(IFindOfficialBranchLinks)
- # Order by the pocket to get the release one first. If changing
- # this be sure to also change BranchCollection.getBranches.
- links = series_set.findForBranches(rows).order_by(
- SeriesSourcePackageBranch.pocket)
- for link in links:
- cache = caches[link.branchID]
- cache._associatedSuiteSourcePackages.append(
- link.suite_sourcepackage)
+ self._preloadDataForBranches(rows)
load_related(Product, rows, ['productID'])
# So far have only needed the persons for their canonical_url - no
# need for validity etc in the /branches API call.
load_related(Person, rows,
['ownerID', 'registrantID', 'reviewerID'])
- for code_import in IStore(CodeImport).find(
- CodeImport, CodeImport.branchID.is_in(branch_ids)):
- cache = caches[code_import.branchID]
- cache.code_import = code_import
load_referencing(BugBranch, rows, ['branchID'])
return DecoratedResultSet(resultset, pre_iter_hook=do_eager_load)
def getMergeProposals(self, statuses=None, for_branches=None,
- target_branch=None, merged_revnos=None):
+ target_branch=None, merged_revnos=None,
+ eager_load=False):
"""See `IBranchCollection`."""
if (self._asymmetric_filter_expressions or for_branches or
target_branch or merged_revnos):
return self._naiveGetMergeProposals(statuses, for_branches,
- target_branch, merged_revnos)
+ target_branch, merged_revnos, eager_load)
else:
# When examining merge proposals in a scope, this is a moderately
# effective set of constrained queries. It is not effective when
@@ -279,16 +291,47 @@
return self._scopedGetMergeProposals(statuses)
def _naiveGetMergeProposals(self, statuses=None, for_branches=None,
- target_branch=None, merged_revnos=None):
+ target_branch=None, merged_revnos=None, eager_load=False):
+
+ def do_eager_load(rows):
+ branch_ids = set()
+ person_ids = set()
+ diff_ids = set()
+ for mp in rows:
+ branch_ids.add(mp.target_branchID)
+ branch_ids.add(mp.source_branchID)
+ person_ids.add(mp.registrantID)
+ person_ids.add(mp.merge_reporterID)
+ diff_ids.add(mp.preview_diff_id)
+ if not branch_ids:
+ return
+
+ # Pre-load Person and ValidPersonCache.
+ list(self.store.find(
+ (Person, ValidPersonCache),
+ ValidPersonCache.id == Person.id,
+ Person.id.is_in(person_ids),
+ ))
+
+ # Pre-load PreviewDiffs and Diffs.
+ list(self.store.find(
+ (PreviewDiff, Diff),
+ PreviewDiff.id.is_in(diff_ids),
+ Diff.id == PreviewDiff.diff_id))
+
+ branches = set(
+ self.store.find(Branch, Branch.id.is_in(branch_ids)))
+ self._preloadDataForBranches(branches)
+
Target = ClassAlias(Branch, "target")
extra_tables = list(set(
self._tables.values() + self._asymmetric_tables.values()))
tables = [Branch] + extra_tables + [
Join(BranchMergeProposal, And(
- Branch.id==BranchMergeProposal.source_branchID,
+ Branch.id == BranchMergeProposal.source_branchID,
*(self._branch_filter_expressions +
self._asymmetric_filter_expressions))),
- Join(Target, Target.id==BranchMergeProposal.target_branchID)
+ Join(Target, Target.id == BranchMergeProposal.target_branchID),
]
expressions = self._getBranchVisibilityExpression()
expressions.extend(self._getBranchVisibilityExpression(Target))
@@ -305,7 +348,12 @@
if statuses is not None:
expressions.append(
BranchMergeProposal.queue_status.is_in(statuses))
- return self.store.using(*tables).find(BranchMergeProposal, *expressions)
+ resultset = self.store.using(*tables).find(
+ BranchMergeProposal, *expressions)
+ if not eager_load:
+ return resultset
+ else:
+ return DecoratedResultSet(resultset, pre_iter_hook=do_eager_load)
def _scopedGetMergeProposals(self, statuses):
scope_tables = [Branch] + self._tables.values()
@@ -513,7 +561,7 @@
"""See `IBranchCollection`."""
subquery = Select(
TeamParticipation.teamID,
- where=TeamParticipation.personID==person.id)
+ where=TeamParticipation.personID == person.id)
filter = [In(Branch.ownerID, subquery)]
return self._filterBy(filter, symmetric=False)
@@ -666,8 +714,8 @@
def _getCandidateBranchesWith(self):
"""Return WITH clauses defining candidate branches.
- These are defined in terms of scope_branches which should be separately
- calculated.
+ These are defined in terms of scope_branches which should be
+ separately calculated.
"""
# Anonymous users get public branches only.
return [
@@ -694,9 +742,9 @@
exclude_from_search=None, symmetric=True):
"""Return a subset of this collection, filtered by 'expressions'.
- :param symmetric: If True this filter will apply to both sides of merge
- proposal lookups and any other lookups that join Branch back onto
- Branch.
+ :param symmetric: If True this filter will apply to both sides
+ of merge proposal lookups and any other lookups that join
+ Branch back onto Branch.
"""
# NOTE: JonathanLange 2009-02-17: We might be able to avoid the need
# for explicit 'tables' by harnessing Storm's table inference system.
@@ -717,7 +765,8 @@
if table is not None:
asymmetric_tables[table] = join
symmetric_expr = list(self._branch_filter_expressions)
- asymmetric_expr = self._asymmetric_filter_expressions + expressions
+ asymmetric_expr = (
+ self._asymmetric_filter_expressions + expressions)
if exclude_from_search is None:
exclude_from_search = []
return self.__class__(
@@ -781,8 +830,8 @@
def _getCandidateBranchesWith(self):
"""Return WITH clauses defining candidate branches.
- These are defined in terms of scope_branches which should be separately
- calculated.
+ These are defined in terms of scope_branches which should be
+ separately calculated.
"""
person = self._user
if person is None:
@@ -796,10 +845,11 @@
TeamParticipation.personID == person.id)._get_select()),
With("private_branches", SQL("""
SELECT scope_branches.id FROM scope_branches WHERE
- scope_branches.private AND ((scope_branches.owner in (select team from teams) OR
- EXISTS(SELECT true from BranchSubscription, teams WHERE
- branchsubscription.branch = scope_branches.id AND
- branchsubscription.person = teams.team)))""")),
+ scope_branches.private AND (
+ (scope_branches.owner in (select team from teams) OR
+ EXISTS(SELECT true from BranchSubscription, teams WHERE
+ branchsubscription.branch = scope_branches.id AND
+ branchsubscription.person = teams.team)))""")),
With("candidate_branches", SQL("""
(SELECT id FROM private_branches) UNION
(select id FROM scope_branches WHERE not private)"""))
Follow ups
