launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-pending-merges-preload-messages into lp:launchpad.

Commit message:
Preload review comments on merge proposals when preloading votes.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1791668 in Launchpad itself: "GitRepository:++repository-pending-merges does not preload review comments"
  https://bugs.launchpad.net/launchpad/+bug/1791668

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-pending-merges-preload-messages/+merge/354622

There's still a small number of TeamParticipation queries (up to three) per distinct reviewer; but as far as I can see there's no precedent for preloading these, and it would require some new helpers to do the right thing.  IMO this isn't urgent, because the set of all active merge proposals targeted at a given repository will usually have only a small number of distinct reviewers.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-pending-merges-preload-messages into lp:launchpad.

=== modified file 'lib/lp/code/browser/tests/test_gitrepository.py'
--- lib/lp/code/browser/tests/test_gitrepository.py	2018-09-06 16:00:45 +0000
+++ lib/lp/code/browser/tests/test_gitrepository.py	2018-09-10 15:49:30 +0000
@@ -13,7 +13,10 @@
 
 from fixtures import FakeLogger
 import pytz
-from testtools.matchers import DocTestMatches
+from testtools.matchers import (
+    DocTestMatches,
+    Equals,
+    )
 import transaction
 from zope.component import getUtility
 from zope.formlib.itemswidgets import ItemDisplayWidget
@@ -26,6 +29,7 @@
 from lp.app.interfaces.services import IService
 from lp.code.enums import (
     BranchMergeProposalStatus,
+    CodeReviewVote,
     GitRepositoryType,
     )
 from lp.code.interfaces.revision import IRevisionSet
@@ -296,6 +300,8 @@
                 target_ref=git_refs[0],
                 set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
             self.factory.makePreviewDiff(merge_proposal=bmp)
+            self.factory.makeCodeReviewComment(
+                vote=CodeReviewVote.APPROVE, merge_proposal=bmp)
 
         recorder1, recorder2 = record_two_runs(
             login_and_view,
@@ -339,6 +345,8 @@
                 source_ref=source_git_ref,
                 set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
             self.factory.makePreviewDiff(merge_proposal=bmp)
+            self.factory.makeCodeReviewComment(
+                vote=CodeReviewVote.APPROVE, merge_proposal=bmp)
 
         def login_and_view():
             with FeatureFixture({"code.git.show_repository_mps": "on"}):
@@ -352,7 +360,12 @@
             login_and_view,
             create_merge_proposal,
             2)
-        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
+        # XXX cjwatson 2018-09-10: There is currently one extra
+        # TeamParticipation query per reviewer (at least in this test setup)
+        # due to GitRepository.isPersonTrustedReviewer.  Fixing this
+        # probably requires a suitable helper to update Person._inTeam_cache
+        # in bulk.
+        self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count + 2)))
 
     def test_view_with_inactive_landing_targets(self):
         product = self.factory.makeProduct(name="foo", vcs=VCSType.GIT)

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2018-09-06 16:00:45 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2018-09-10 15:49:30 +0000
@@ -124,6 +124,10 @@
 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.messages.model.message import (
+    Message,
+    MessageChunk,
+    )
 from lp.services.propertycache import (
     cachedproperty,
     get_property_cache,
@@ -1018,8 +1022,6 @@
             if not subject.startswith('Re: '):
                 subject = 'Re: ' + subject
 
-        # Avoid circular dependencies.
-        from lp.services.messages.model.message import Message, MessageChunk
         msgid = make_msgid('codereview')
         message = Message(
             parent=parent_message, owner=owner, rfc822msgid=msgid,
@@ -1373,11 +1375,15 @@
         # if we need to include a vote summary, we should precache
         # that data too
         if include_votes:
-            vote_list = list(load_referencing(
+            votes = load_referencing(
                 CodeReviewVoteReference, branch_merge_proposals,
-                ['branch_merge_proposalID']))
+                ['branch_merge_proposalID'])
             for mp in branch_merge_proposals:
-                get_property_cache(mp).votes = vote_list
+                get_property_cache(mp).votes = votes
+            comments = load_related(CodeReviewComment, votes, ['commentID'])
+            load_related(Message, comments, ['messageID'])
+            list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+                [vote.reviewerID for vote in votes], need_validity=True))
 
             # we also provide a summary of diffs, so load them
             load_related(LibraryFileAlias, diffs, ['diff_textID'])