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'])
_______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp