launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04776
[Merge] lp:~abentley/launchpad/rollback-13716 into lp:launchpad
Aaron Bentley has proposed merging lp:~abentley/launchpad/rollback-13716 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~abentley/launchpad/rollback-13716/+merge/72900
= Summary =
Fix +activereviews for private branches
== Proposed fix ==
Roll back 13716 and 13779
== Pre-implementation notes ==
Proposed by wgrant
== Implementation details ==
== Tests ==
== Demo and Q/A ==
Go to +activereviews for a private branch. There should be stuff there.
= 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/model/branchcollection.py
155: Line exceeds 78 characters.
178: Line exceeds 78 characters.
208: Line exceeds 78 characters.
308: Line exceeds 78 characters.
669: Line exceeds 78 characters.
697: Line exceeds 78 characters.
720: Line exceeds 78 characters.
784: Line exceeds 78 characters.
799: Line exceeds 78 characters.
228: local variable 'branches' is assigned to but never used
288: E225 missing whitespace around operator
308: E501 line too long (80 characters)
516: E225 missing whitespace around operator
799: E501 line too long (96 characters)
./lib/lp/code/browser/branchmergeproposallisting.py
333: Line exceeds 78 characters.
285: E231 missing whitespace after ','
333: E501 line too long (82 characters)
--
https://code.launchpad.net/~abentley/launchpad/rollback-13716/+merge/72900
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/rollback-13716 into lp:launchpad.
=== modified file 'lib/lp/code/browser/branchmergeproposallisting.py'
--- lib/lp/code/browser/branchmergeproposallisting.py 2011-08-17 20:21:01 +0000
+++ lib/lp/code/browser/branchmergeproposallisting.py 2011-08-25 13:57:36 +0000
@@ -47,7 +47,6 @@
IBranchCollection,
)
from lp.code.interfaces.branchmergeproposal import (
- BRANCH_MERGE_PROPOSAL_FINAL_STATES,
IBranchMergeProposal,
IBranchMergeProposalGetter,
IBranchMergeProposalListingBatchNavigator,
@@ -420,11 +419,7 @@
def getProposals(self):
"""See `ActiveReviewsView`."""
- non_final = tuple(
- set(BranchMergeProposalStatus.items) -
- set(BRANCH_MERGE_PROPOSAL_FINAL_STATES))
- candidates = self.context.getMergeProposals(
- status=non_final, eager_load=True)
+ candidates = self.context.landing_candidates
return [proposal for proposal in candidates
if check_permission('launchpad.View', proposal)]
=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py 2011-08-22 17:40:02 +0000
+++ lib/lp/code/interfaces/branch.py 2011-08-25 13:57:36 +0000
@@ -621,7 +621,7 @@
@export_read_operation()
@operation_for_version('beta')
def getMergeProposals(status=None, visible_by_user=None,
- merged_revnos=None, eager_load=False):
+ merged_revnos=None):
"""Return matching BranchMergeProposals."""
def scheduleDiffUpdates():
=== modified file 'lib/lp/code/interfaces/branchcollection.py'
--- lib/lp/code/interfaces/branchcollection.py 2011-08-17 10:01:10 +0000
+++ lib/lp/code/interfaces/branchcollection.py 2011-08-25 13:57:36 +0000
@@ -70,7 +70,7 @@
"""
def getMergeProposals(statuses=None, for_branches=None,
- target_branch=None, eager_load=False):
+ target_branch=None):
"""Return a result set of merge proposals for the branches in this
collection.
@@ -81,8 +81,6 @@
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-24 16:21:07 +0000
+++ lib/lp/code/model/branch.py 2011-08-25 13:57:36 +0000
@@ -376,7 +376,7 @@
""" % sqlvalues(self, BRANCH_MERGE_PROPOSAL_FINAL_STATES))
def getMergeProposals(self, status=None, visible_by_user=None,
- merged_revnos=None, eager_load=False):
+ merged_revnos=None):
"""See `IBranch`."""
if not status:
status = (
@@ -386,8 +386,7 @@
collection = getUtility(IAllBranches).visibleByUser(visible_by_user)
return collection.getMergeProposals(
- status, target_branch=self, merged_revnos=merged_revnos,
- eager_load=eager_load)
+ status, target_branch=self, merged_revnos=merged_revnos)
def isBranchMergeable(self, target_branch):
"""See `IBranch`."""
=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py 2011-08-17 10:01:10 +0000
+++ lib/lp/code/model/branchcollection.py 2011-08-25 13:57:36 +0000
@@ -63,17 +63,12 @@
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
@@ -157,9 +152,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.
@@ -180,8 +175,7 @@
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__(
@@ -211,48 +205,12 @@
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(
@@ -267,23 +225,53 @@
branch_ids = set(branch.id for branch in rows)
if not branch_ids:
return
- self._preloadDataForBranches(rows)
+ 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)
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,
- eager_load=False):
+ target_branch=None, merged_revnos=None):
"""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, eager_load)
+ target_branch, merged_revnos)
else:
# When examining merge proposals in a scope, this is a moderately
# effective set of constrained queries. It is not effective when
@@ -291,47 +279,16 @@
return self._scopedGetMergeProposals(statuses)
def _naiveGetMergeProposals(self, statuses=None, for_branches=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_branch=None, merged_revnos=None):
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))
@@ -348,12 +305,7 @@
if statuses is not None:
expressions.append(
BranchMergeProposal.queue_status.is_in(statuses))
- resultset = self.store.using(*tables).find(
- BranchMergeProposal, *expressions)
- if not eager_load:
- return resultset
- else:
- return DecoratedResultSet(resultset, pre_iter_hook=do_eager_load)
+ return self.store.using(*tables).find(BranchMergeProposal, *expressions)
def _scopedGetMergeProposals(self, statuses):
scope_tables = [Branch] + self._tables.values()
@@ -561,7 +513,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)
@@ -714,8 +666,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 [
@@ -742,9 +694,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.
@@ -765,8 +717,7 @@
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__(
@@ -830,8 +781,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:
@@ -845,11 +796,10 @@
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)"""))
