Merge lp:~cjwatson/launchpad/git-pending-merges-preload-messages into lp:launchpad

Proposed by Colin Watson on 2018-09-10
Status: Merged
Merged at revision: 18773
Proposed branch: lp:~cjwatson/launchpad/git-pending-merges-preload-messages
Merge into: lp:launchpad
Diff against target: 98 lines (+26/-7)
2 files modified
lib/lp/code/browser/tests/test_gitrepository.py (+15/-2)
lib/lp/code/model/branchmergeproposal.py (+11/-5)
To merge this branch: bzr merge lp:~cjwatson/launchpad/git-pending-merges-preload-messages
Reviewer Review Type Date Requested Status
Tom Wardill (community) Approve on 2018-09-10
Launchpad code reviewers 2018-09-10 Pending
Review via email: mp+354622@code.launchpad.net

Commit message

Preload review comments on merge proposals when preloading votes.

Description of the change

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.

To post a comment you must log in.
Tom Wardill (twom) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/browser/tests/test_gitrepository.py'
2--- lib/lp/code/browser/tests/test_gitrepository.py 2018-09-06 16:00:45 +0000
3+++ lib/lp/code/browser/tests/test_gitrepository.py 2018-09-10 15:49:30 +0000
4@@ -13,7 +13,10 @@
5
6 from fixtures import FakeLogger
7 import pytz
8-from testtools.matchers import DocTestMatches
9+from testtools.matchers import (
10+ DocTestMatches,
11+ Equals,
12+ )
13 import transaction
14 from zope.component import getUtility
15 from zope.formlib.itemswidgets import ItemDisplayWidget
16@@ -26,6 +29,7 @@
17 from lp.app.interfaces.services import IService
18 from lp.code.enums import (
19 BranchMergeProposalStatus,
20+ CodeReviewVote,
21 GitRepositoryType,
22 )
23 from lp.code.interfaces.revision import IRevisionSet
24@@ -296,6 +300,8 @@
25 target_ref=git_refs[0],
26 set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
27 self.factory.makePreviewDiff(merge_proposal=bmp)
28+ self.factory.makeCodeReviewComment(
29+ vote=CodeReviewVote.APPROVE, merge_proposal=bmp)
30
31 recorder1, recorder2 = record_two_runs(
32 login_and_view,
33@@ -339,6 +345,8 @@
34 source_ref=source_git_ref,
35 set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
36 self.factory.makePreviewDiff(merge_proposal=bmp)
37+ self.factory.makeCodeReviewComment(
38+ vote=CodeReviewVote.APPROVE, merge_proposal=bmp)
39
40 def login_and_view():
41 with FeatureFixture({"code.git.show_repository_mps": "on"}):
42@@ -352,7 +360,12 @@
43 login_and_view,
44 create_merge_proposal,
45 2)
46- self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
47+ # XXX cjwatson 2018-09-10: There is currently one extra
48+ # TeamParticipation query per reviewer (at least in this test setup)
49+ # due to GitRepository.isPersonTrustedReviewer. Fixing this
50+ # probably requires a suitable helper to update Person._inTeam_cache
51+ # in bulk.
52+ self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count + 2)))
53
54 def test_view_with_inactive_landing_targets(self):
55 product = self.factory.makeProduct(name="foo", vcs=VCSType.GIT)
56
57=== modified file 'lib/lp/code/model/branchmergeproposal.py'
58--- lib/lp/code/model/branchmergeproposal.py 2018-09-06 16:00:45 +0000
59+++ lib/lp/code/model/branchmergeproposal.py 2018-09-10 15:49:30 +0000
60@@ -124,6 +124,10 @@
61 from lp.services.job.model.job import Job
62 from lp.services.librarian.model import LibraryFileAlias
63 from lp.services.mail.sendmail import validate_message
64+from lp.services.messages.model.message import (
65+ Message,
66+ MessageChunk,
67+ )
68 from lp.services.propertycache import (
69 cachedproperty,
70 get_property_cache,
71@@ -1018,8 +1022,6 @@
72 if not subject.startswith('Re: '):
73 subject = 'Re: ' + subject
74
75- # Avoid circular dependencies.
76- from lp.services.messages.model.message import Message, MessageChunk
77 msgid = make_msgid('codereview')
78 message = Message(
79 parent=parent_message, owner=owner, rfc822msgid=msgid,
80@@ -1373,11 +1375,15 @@
81 # if we need to include a vote summary, we should precache
82 # that data too
83 if include_votes:
84- vote_list = list(load_referencing(
85+ votes = load_referencing(
86 CodeReviewVoteReference, branch_merge_proposals,
87- ['branch_merge_proposalID']))
88+ ['branch_merge_proposalID'])
89 for mp in branch_merge_proposals:
90- get_property_cache(mp).votes = vote_list
91+ get_property_cache(mp).votes = votes
92+ comments = load_related(CodeReviewComment, votes, ['commentID'])
93+ load_related(Message, comments, ['messageID'])
94+ list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
95+ [vote.reviewerID for vote in votes], need_validity=True))
96
97 # we also provide a summary of diffs, so load them
98 load_related(LibraryFileAlias, diffs, ['diff_textID'])