Merge lp:~danilo/launchpad/bug-826692-take2 into lp:launchpad

[Данило Шеган]

[r=jtv][bug=826692] Re-land the fix for bug 826692 with the fix for private branches included: makes +activereviews page render faster by eager loading relevant data in getMergeProposals().

= 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

[Jeroen T. Vermeulen (jtv)]

The incremental diff from the pastebin looks fine (apart from a missing "the" in the test comment ☺).

I wish visible_by_user were called visible_to_user, but that's irrelevant for this branch.

review: Approve

=== 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:36:42 +0000
@@ -47,6 +47,7 @@
     IBranchCollection,
     )
 from lp.code.interfaces.branchmergeproposal import (
+    BRANCH_MERGE_PROPOSAL_FINAL_STATES,
     IBranchMergeProposal,
     IBranchMergeProposalGetter,
     IBranchMergeProposalListingBatchNavigator,
@@ -282,7 +283,7 @@
         collection = collection.visibleByUser(self.user)
         proposals = collection.getMergeProposals(
             [BranchMergeProposalStatus.CODE_APPROVED,
-             BranchMergeProposalStatus.NEEDS_REVIEW,])
+             BranchMergeProposalStatus.NEEDS_REVIEW, ])
         return proposals
 
     def _getReviewGroup(self, proposal, votes, reviewer):
@@ -330,8 +331,8 @@
                     else:
                         return self.ARE_DOING
                 # Since team reviews are always pending, and we've eliminated
-                # the case where the reviewer is ther person, then if the reviewer
-                # is in the reviewer team, it is a can do.
+                # the case where the reviewer is ther person, then if
+                # the reviewer is in the reviewer team, it is a can do.
                 if reviewer.inTeam(vote.reviewer):
                     result = self.CAN_DO
         return result
@@ -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:36:42 +0000
@@ -28,6 +28,7 @@
     BrowserTestCase,
     login,
     login_person,
+    person_logged_in,
     TestCaseWithFactory,
     )
 from lp.testing.views import create_initialized_view
@@ -359,3 +360,20 @@
         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
+        # the 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:36:42 +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:36:42 +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:36:42 +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:36:42 +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)"""))