launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/fix-mp-vote-preloading into lp:launchpad.

Commit message:
Fix assignment of votes to merge proposals when preloading data for multiple merge proposals.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/fix-mp-vote-preloading/+merge/355318
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/fix-mp-vote-preloading into lp:launchpad.

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2018-09-10 17:02:47 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2018-09-19 09:32:34 +0000
@@ -10,6 +10,7 @@
     'is_valid_transition',
     ]
 
+from collections import defaultdict
 from email.utils import make_msgid
 from operator import attrgetter
 import sys
@@ -1374,8 +1375,11 @@
             votes = load_referencing(
                 CodeReviewVoteReference, branch_merge_proposals,
                 ['branch_merge_proposalID'])
+            votes_map = defaultdict(list)
+            for vote in votes:
+                votes_map[vote.branch_merge_proposalID].append(vote)
             for mp in branch_merge_proposals:
-                get_property_cache(mp).votes = votes
+                get_property_cache(mp).votes = votes_map.get(mp.id)
             comments = load_related(CodeReviewComment, votes, ['commentID'])
             load_related(Message, comments, ['messageID'])
             person_ids.update(vote.reviewerID for vote in votes)

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py	2018-04-25 12:01:33 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py	2018-09-19 09:32:34 +0000
@@ -65,6 +65,7 @@
     notify_modified,
     )
 from lp.code.model.branchmergeproposal import (
+    BranchMergeProposal,
     BranchMergeProposalGetter,
     is_valid_transition,
     )
@@ -1740,7 +1741,7 @@
             merge_proposal.target_branch.owner, vote.reviewer)
 
     def makeProposalWithReviewer(self, reviewer=None, review_type=None,
-                                 registrant=None):
+                                 registrant=None, **kwargs):
         """Make a proposal and request a review from reviewer.
 
         If no reviewer is passed in, make a reviewer.
@@ -1750,7 +1751,7 @@
         if registrant is None:
             registrant = self.factory.makePerson()
         merge_proposal = make_merge_proposal_without_reviewers(
-            factory=self.factory, registrant=registrant)
+            factory=self.factory, registrant=registrant, **kwargs)
         login_person(merge_proposal.source_branch.owner)
         merge_proposal.nominateReviewer(
             reviewer=reviewer, registrant=registrant, review_type=review_type)
@@ -2015,6 +2016,27 @@
         # Still only one vote.
         self.assertEqual(1, len(list(merge_proposal.votes)))
 
+    def test_preloadDataForBMPs_maps_votes_to_proposals(self):
+        # When called on multiple merge proposals, preloadDataForBMPs
+        # assigns votes to the correct proposals.
+        merge_proposal_1, reviewer_1 = self.makeProposalWithReviewer(
+            set_state=BranchMergeProposalStatus.MERGED)
+        merge_proposal_2, _ = self.makeProposalWithReviewer(
+            target_branch=merge_proposal_1.target_branch,
+            source_branch=merge_proposal_1.source_branch)
+        merge_proposal_2.nominateReviewer(
+            reviewer=reviewer_1, registrant=merge_proposal_2.registrant)
+        votes_1 = list(merge_proposal_1.votes)
+        self.assertEqual(1, len(votes_1))
+        votes_2 = list(merge_proposal_2.votes)
+        self.assertEqual(2, len(votes_2))
+        BranchMergeProposal.preloadDataForBMPs(
+            [removeSecurityProxy(merge_proposal_1),
+             removeSecurityProxy(merge_proposal_2)],
+            reviewer_1)
+        self.assertContentEqual(votes_1, merge_proposal_1.votes)
+        self.assertContentEqual(votes_2, merge_proposal_2.votes)
+
 
 class TestBranchMergeProposalResubmit(TestCaseWithFactory):