launchpad-reviewers team mailing list archive

[Tom Wardill <tom.wardill@xxxxxxxxxxxxx>]

Tom Wardill has proposed merging lp:~twom/launchpad/precache-gitrepository-branch-queries into lp:launchpad.

Commit message:
Precache gitrepository voting summary data

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~twom/launchpad/precache-gitrepository-branch-queries/+merge/354347
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~twom/launchpad/precache-gitrepository-branch-queries into lp:launchpad.

=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py	2018-07-16 00:49:00 +0000
+++ lib/lp/code/browser/branchmergeproposal.py	2018-09-05 16:55:00 +0000
@@ -890,7 +890,12 @@
     @cachedproperty
     def reviews(self):
         """Return the decorated votes for the proposal."""
-        users_vote = self.context.getUsersVoteReference(self.user)
+
+        # this would use getUsersVoteReference, but we need to
+        # be able to cache the property. We dont' need to normalize
+        # the review types.
+        users_vote = [uv for uv in self.context.votes
+                      if uv.reviewer == self.user]
         return [DecoratedCodeReviewVoteReference(vote, self.user, users_vote)
                 for vote in self.context.votes
                 if check_permission('launchpad.LimitedView', vote.reviewer)]

=== modified file 'lib/lp/code/browser/tests/test_gitrepository.py'
--- lib/lp/code/browser/tests/test_gitrepository.py	2018-08-31 11:35:52 +0000
+++ lib/lp/code/browser/tests/test_gitrepository.py	2018-09-05 16:55:00 +0000
@@ -50,7 +50,10 @@
     record_two_runs,
     TestCaseWithFactory,
     )
-from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
+    )
 from lp.testing.matchers import (
     Contains,
     HasQueryCount,
@@ -96,7 +99,7 @@
 
 class TestGitRepositoryView(BrowserTestCase):
 
-    layer = DatabaseFunctionalLayer
+    layer = LaunchpadFunctionalLayer
 
     def test_clone_instructions(self):
         repository = self.factory.makeGitRepository()
@@ -275,6 +278,32 @@
         self.assertEqual('1 branch', view._getBranchCountText(1))
         self.assertEqual('2 branches', view._getBranchCountText(2))
 
+    def test_landing_candidate_query_count(self):
+        repository = self.factory.makeGitRepository()
+        git_refs = self.factory.makeGitRefs(
+            repository,
+            paths=["refs/heads/master", "refs/heads/1.0", "refs/tags/1.1"])
+
+        def login_and_view():
+            with FeatureFixture({"code.git.show_repository_mps": "on"}):
+                with person_logged_in(repository.owner):
+                    browser = self.getViewBrowser(repository)
+                    self.assertIsNotNone(
+                        find_tag_by_id(browser.contents, 'landing-candidates'))
+
+        def create_merge_proposal():
+            bmp = self.factory.makeBranchMergeProposalForGit(
+                target_ref=git_refs[0],
+                set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
+            self.factory.makePreviewDiff(merge_proposal=bmp)
+
+        create_merge_proposal()
+        recorder1, recorder2 = record_two_runs(
+            login_and_view,
+            create_merge_proposal,
+            10)
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
+
     def test_view_with_landing_targets(self):
         product = self.factory.makeProduct(name="foo", vcs=VCSType.GIT)
         target_repository = self.factory.makeGitRepository(target=product)
@@ -296,6 +325,38 @@
                 self.assertIsNotNone(
                     find_tag_by_id(browser.contents, 'landing-targets'))
 
+    def test_landing_target_query_count(self):
+        product = self.factory.makeProduct(name="foo", vcs=VCSType.GIT)
+        target_repository = self.factory.makeGitRepository(target=product)
+        source_repository = self.factory.makeGitRepository(target=product)
+
+        def create_merge_proposal():
+            target_git_refs = self.factory.makeGitRefs(
+                target_repository)
+            source_git_refs = self.factory.makeGitRefs(
+                source_repository)
+            bmp = self.factory.makeBranchMergeProposalForGit(
+                target_ref=target_git_refs[0],
+                source_ref=source_git_refs[0],
+                set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
+            self.factory.makePreviewDiff(merge_proposal=bmp)
+
+        def login_and_view():
+            with FeatureFixture({"code.git.show_repository_mps": "on"}):
+                with person_logged_in(target_repository.owner):
+                    browser = self.getViewBrowser(
+                        source_repository, user=source_repository.owner)
+                    self.assertIsNotNone(
+                        find_tag_by_id(browser.contents, 'landing-targets'))
+
+        create_merge_proposal()
+
+        recorder1, recorder2 = record_two_runs(
+            login_and_view,
+            create_merge_proposal,
+            10)
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
+
     def test_view_with_inactive_landing_targets(self):
         product = self.factory.makeProduct(name="foo", vcs=VCSType.GIT)
         target_repository = self.factory.makeGitRepository(target=product)

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2018-04-25 12:01:33 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2018-09-05 16:55:00 +0000
@@ -21,7 +21,6 @@
 from sqlobject import (
     ForeignKey,
     IntCol,
-    SQLMultipleJoin,
     StringCol,
     )
 from storm.expr import (
@@ -123,6 +122,7 @@
 from lp.services.helpers import shortlist
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
+from lp.services.librarian.model import LibraryFileAlias
 from lp.services.mail.sendmail import validate_message
 from lp.services.propertycache import (
     cachedproperty,
@@ -610,8 +610,11 @@
     def preview_diff(self):
         return self._preview_diffs.last()
 
-    votes = SQLMultipleJoin(
-        'CodeReviewVoteReference', joinColumn='branch_merge_proposal')
+    @cachedproperty
+    def votes(self):
+        return list(Store.of(self).find(
+            CodeReviewVoteReference,
+            CodeReviewVoteReference.branch_merge_proposal == self))
 
     def getNotificationRecipients(self, min_level):
         """See IBranchMergeProposal.getNotificationRecipients"""
@@ -1275,7 +1278,7 @@
         return [range_ for range_, diff in zip(ranges, diffs) if diff is None]
 
     @staticmethod
-    def preloadDataForBMPs(branch_merge_proposals, user):
+    def preloadDataForBMPs(branch_merge_proposals, user, include_summary=True):
         # Utility to load the data related to a list of bmps.
         # Circular imports.
         from lp.code.model.branch import Branch
@@ -1330,7 +1333,7 @@
                 PreviewDiff.branch_merge_proposal_id,
                 Desc(PreviewDiff.date_created)).config(
                     distinct=[PreviewDiff.branch_merge_proposal_id])
-        load_related(Diff, preview_diffs, ['diff_id'])
+        diffs = load_related(Diff, preview_diffs, ['diff_id'])
         preview_diff_map = {}
         for previewdiff in preview_diffs:
             preview_diff_map[previewdiff.branch_merge_proposal_id] = (
@@ -1367,6 +1370,19 @@
         if repositories:
             GenericGitCollection.preloadDataForRepositories(repositories)
 
+        # if we need to include a vote summary, we should precache
+        # that data too
+        if include_summary:
+            vote_list = list(load_referencing(
+                                CodeReviewVoteReference,
+                                branch_merge_proposals,
+                                ['branch_merge_proposalID']))
+            for mp in branch_merge_proposals:
+                get_property_cache(mp).votes = vote_list
+
+            # we also provide a summary of diffs, so load them
+            load_related(LibraryFileAlias, diffs, ['diff_textID'])
+
 
 @implementer(IBranchMergeProposalGetter)
 class BranchMergeProposalGetter:

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2018-08-31 13:14:39 +0000
+++ lib/lp/code/model/gitrepository.py	2018-09-05 16:55:00 +0000
@@ -922,7 +922,10 @@
 
     def getPrecachedLandingCandidates(self, user):
         """See `IGitRepository`."""
-        loader = partial(BranchMergeProposal.preloadDataForBMPs, user=user)
+        loader = partial(
+            BranchMergeProposal.preloadDataForBMPs,
+            user=user,
+            include_summary=True)
         return DecoratedResultSet(
             self.landing_candidates, pre_iter_hook=loader)