Launchpad itself

Merge lp:~thumper/launchpad/claim-review-into-model into lp:launchpad/db-devel

  1. claim-review-into-model
  2. Merge into db-devel
Proposed by Tim Penhey
Status: Merged
Merged at revision: not available
Proposed branch: lp:~thumper/launchpad/claim-review-into-model
Merge into: lp:launchpad/db-devel
Diff against target: 96 lines (+53/-3)
3 files modified
database/schema/patch-2207-17-0.sql (+13/-0)
lib/lp/code/model/branchmergeproposal.py (+8/-3)
lib/lp/code/model/tests/test_branchmergeproposals.py (+32/-0)
To merge this branch: bzr merge lp:~thumper/launchpad/claim-review-into-model
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle Approve
Jonathan Lange (community) db Approve
Stuart Bishop (community) db Approve
Guilherme Salgado release-critical Pending
Review via email: mp+15524@code.launchpad.net

Commit message

Add db-patch to remove the restriction of only having one team review of a particular type.

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

The branch name came about because I wanted to move the claim review logic into the model code - and I still do, but the minimal fix needed for the release critical is this. Moving the logic will happen shortly (along with deleting pending reviews).

The way things are written at the moment, it is expected that there are only going to be one review of any particular type by any person or team. This means that if a project was wanting two team reviews, then they'd have to artificially give them different review types. This is no longer the case.

The particular problem this is solving is the case where a person ends up with to rows in the code review vote table when they should only have one. The use of the storm result set ".one()" method was causing this. There is still the possibility of an individual having two vote references if they are using multiple windows and claiming reviews in stale windows - that is to be fixed by moving the code into the model. This will reduce the window of opportunity for it to happen to simultaneous transactions. It is also possible for an individual to get two entries due to a person merge.

Revision history for this message
Stuart Bishop (stub) wrote :

Remove the two CREATE INDEX statements - they are not needed as we already have all the indexes needed for queries in place.

With this change, approved as patch-2207-17-0.sql

review: Approve (db)
Revision history for this message
Jonathan Lange (jml) wrote :

First, my peanut gallery comments:
 * In the SQL patch, you spell "UNIQUE" as "UNQIUE".
 * When you say "if vote_reference is None or reviewer.is_team:", you should probably have a comment explaining what this actually means.

I think that the db patch is sound. I can quite clearly see cases where it's desirable to request two code reviews from a particular team.

review: Approve (db)
Revision history for this message
Jonathan Lange (jml) wrote :

BTW, I can't figure out from the cover letter why this is a potential RC fix.

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

Looks fine to me.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'database/schema/patch-2207-17-0.sql'
2--- database/schema/patch-2207-17-0.sql 1970-01-01 00:00:00 +0000
3+++ database/schema/patch-2207-17-0.sql 2009-12-06 22:41:14 +0000
4@@ -0,0 +1,13 @@
5+-- Copyright 2009 Canonical Ltd. This software is licensed under the
6+-- GNU Affero General Public License version 3 (see the file LICENSE).
7+
8+SET client_min_messages=ERROR;
9+
10+-- There is no real reason to have limits on review_type, and it is just
11+-- adding an artificial constraint that just gets in the way.
12+
13+-- Drop the UNIQUE indices
14+DROP INDEX codereviewvote__branch_merge_proposal__reviewer__key;
15+DROP INDEX codereviewvote__branch_merge_proposal__reviewer__review_type__k;
16+
17+INSERT INTO LaunchpadDatabaseRevision VALUES (2207, 17, 0);
18
19=== modified file 'lib/lp/code/model/branchmergeproposal.py'
20--- lib/lp/code/model/branchmergeproposal.py 2009-12-01 01:09:38 +0000
21+++ lib/lp/code/model/branchmergeproposal.py 2009-12-06 22:41:14 +0000
22@@ -523,7 +523,11 @@
23 # Lower case the review type.
24 review_type = self._normalizeReviewType(review_type)
25 vote_reference = self.getUsersVoteReference(reviewer, review_type)
26- if vote_reference is None:
27+ # If there is no existing review for the reviewer, then create a new
28+ # one. If the reviewer is a team, then we don't care if there is
29+ # already an existing pending review, as some projects expect multiple
30+ # reviews from a team.
31+ if vote_reference is None or reviewer.is_team:
32 vote_reference = CodeReviewVoteReference(
33 branch_merge_proposal=self,
34 registrant=registrant,
35@@ -615,7 +619,7 @@
36 return Store.of(self).find(
37 CodeReviewVoteReference,
38 CodeReviewVoteReference.branch_merge_proposal == self,
39- query).one()
40+ query).order_by(CodeReviewVoteReference.date_created).first()
41
42 def _getTeamVoteReference(self, user, review_type):
43 """Get a vote reference where the user is in the review team.
44@@ -626,7 +630,8 @@
45 CodeReviewVoteReference,
46 CodeReviewVoteReference.branch_merge_proposal == self,
47 CodeReviewVoteReference.review_type == review_type,
48- CodeReviewVoteReference.comment == None)
49+ CodeReviewVoteReference.comment == None
50+ ).order_by(CodeReviewVoteReference.date_created)
51 for ref in refs:
52 if user.inTeam(ref.reviewer):
53 return ref
54
55=== modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
56--- lib/lp/code/model/tests/test_branchmergeproposals.py 2009-12-01 01:09:38 +0000
57+++ lib/lp/code/model/tests/test_branchmergeproposals.py 2009-12-06 22:41:14 +0000
58@@ -1527,6 +1527,38 @@
59 ['general-1', 'general-2'],
60 sorted([review.review_type for review in votes]))
61
62+ def test_nominate_multiple_with_same_types(self):
63+ # There can be multiple reviews for a team with the same review_type.
64+ reviewer = self.factory.makePerson()
65+ review_team = self.factory.makeTeam(owner=reviewer)
66+ merge_proposal, reviewer = self.makeProposalWithReviewer(
67+ reviewer=review_team, review_type='general')
68+ merge_proposal.nominateReviewer(
69+ reviewer=review_team,
70+ registrant=merge_proposal.registrant,
71+ review_type='general')
72+
73+ votes = list(merge_proposal.votes)
74+ self.assertEqual(
75+ [(review_team, 'general'), (review_team, 'general')],
76+ [(review.reviewer, review.review_type) for review in votes])
77+
78+ def test_nominate_multiple_team_reviews_with_no_type(self):
79+ # There can be multiple reviews for a team with no review type set.
80+ reviewer = self.factory.makePerson()
81+ review_team = self.factory.makeTeam(owner=reviewer)
82+ merge_proposal, reviewer = self.makeProposalWithReviewer(
83+ reviewer=review_team, review_type=None)
84+ merge_proposal.nominateReviewer(
85+ reviewer=review_team,
86+ registrant=merge_proposal.registrant,
87+ review_type=None)
88+
89+ votes = list(merge_proposal.votes)
90+ self.assertEqual(
91+ [(review_team, None), (review_team, None)],
92+ [(review.reviewer, review.review_type) for review in votes])
93+
94 def test_nominate_updates_reference(self):
95 """The existing reference is updated on re-nomination."""
96 merge_proposal = self.factory.makeBranchMergeProposal()

Subscribers

People subscribed via source and target branches

to status/vote changes: