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
=== added file 'database/schema/patch-2207-17-0.sql'
--- database/schema/patch-2207-17-0.sql 1970-01-01 00:00:00 +0000
+++ database/schema/patch-2207-17-0.sql 2009-12-06 22:41:14 +0000
@@ -0,0 +1,13 @@
1-- Copyright 2009 Canonical Ltd. This software is licensed under the
2-- GNU Affero General Public License version 3 (see the file LICENSE).
3
4SET client_min_messages=ERROR;
5
6-- There is no real reason to have limits on review_type, and it is just
7-- adding an artificial constraint that just gets in the way.
8
9-- Drop the UNIQUE indices
10DROP INDEX codereviewvote__branch_merge_proposal__reviewer__key;
11DROP INDEX codereviewvote__branch_merge_proposal__reviewer__review_type__k;
12
13INSERT INTO LaunchpadDatabaseRevision VALUES (2207, 17, 0);
014
=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py 2009-12-01 01:09:38 +0000
+++ lib/lp/code/model/branchmergeproposal.py 2009-12-06 22:41:14 +0000
@@ -523,7 +523,11 @@
523 # Lower case the review type.523 # Lower case the review type.
524 review_type = self._normalizeReviewType(review_type)524 review_type = self._normalizeReviewType(review_type)
525 vote_reference = self.getUsersVoteReference(reviewer, review_type)525 vote_reference = self.getUsersVoteReference(reviewer, review_type)
526 if vote_reference is None:526 # If there is no existing review for the reviewer, then create a new
527 # one. If the reviewer is a team, then we don't care if there is
528 # already an existing pending review, as some projects expect multiple
529 # reviews from a team.
530 if vote_reference is None or reviewer.is_team:
527 vote_reference = CodeReviewVoteReference(531 vote_reference = CodeReviewVoteReference(
528 branch_merge_proposal=self,532 branch_merge_proposal=self,
529 registrant=registrant,533 registrant=registrant,
@@ -615,7 +619,7 @@
615 return Store.of(self).find(619 return Store.of(self).find(
616 CodeReviewVoteReference,620 CodeReviewVoteReference,
617 CodeReviewVoteReference.branch_merge_proposal == self,621 CodeReviewVoteReference.branch_merge_proposal == self,
618 query).one()622 query).order_by(CodeReviewVoteReference.date_created).first()
619623
620 def _getTeamVoteReference(self, user, review_type):624 def _getTeamVoteReference(self, user, review_type):
621 """Get a vote reference where the user is in the review team.625 """Get a vote reference where the user is in the review team.
@@ -626,7 +630,8 @@
626 CodeReviewVoteReference,630 CodeReviewVoteReference,
627 CodeReviewVoteReference.branch_merge_proposal == self,631 CodeReviewVoteReference.branch_merge_proposal == self,
628 CodeReviewVoteReference.review_type == review_type,632 CodeReviewVoteReference.review_type == review_type,
629 CodeReviewVoteReference.comment == None)633 CodeReviewVoteReference.comment == None
634 ).order_by(CodeReviewVoteReference.date_created)
630 for ref in refs:635 for ref in refs:
631 if user.inTeam(ref.reviewer):636 if user.inTeam(ref.reviewer):
632 return ref637 return ref
633638
=== modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
--- lib/lp/code/model/tests/test_branchmergeproposals.py 2009-12-01 01:09:38 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposals.py 2009-12-06 22:41:14 +0000
@@ -1527,6 +1527,38 @@
1527 ['general-1', 'general-2'],1527 ['general-1', 'general-2'],
1528 sorted([review.review_type for review in votes]))1528 sorted([review.review_type for review in votes]))
15291529
1530 def test_nominate_multiple_with_same_types(self):
1531 # There can be multiple reviews for a team with the same review_type.
1532 reviewer = self.factory.makePerson()
1533 review_team = self.factory.makeTeam(owner=reviewer)
1534 merge_proposal, reviewer = self.makeProposalWithReviewer(
1535 reviewer=review_team, review_type='general')
1536 merge_proposal.nominateReviewer(
1537 reviewer=review_team,
1538 registrant=merge_proposal.registrant,
1539 review_type='general')
1540
1541 votes = list(merge_proposal.votes)
1542 self.assertEqual(
1543 [(review_team, 'general'), (review_team, 'general')],
1544 [(review.reviewer, review.review_type) for review in votes])
1545
1546 def test_nominate_multiple_team_reviews_with_no_type(self):
1547 # There can be multiple reviews for a team with no review type set.
1548 reviewer = self.factory.makePerson()
1549 review_team = self.factory.makeTeam(owner=reviewer)
1550 merge_proposal, reviewer = self.makeProposalWithReviewer(
1551 reviewer=review_team, review_type=None)
1552 merge_proposal.nominateReviewer(
1553 reviewer=review_team,
1554 registrant=merge_proposal.registrant,
1555 review_type=None)
1556
1557 votes = list(merge_proposal.votes)
1558 self.assertEqual(
1559 [(review_team, None), (review_team, None)],
1560 [(review.reviewer, review.review_type) for review in votes])
1561
1530 def test_nominate_updates_reference(self):1562 def test_nominate_updates_reference(self):
1531 """The existing reference is updated on re-nomination."""1563 """The existing reference is updated on re-nomination."""
1532 merge_proposal = self.factory.makeBranchMergeProposal()1564 merge_proposal = self.factory.makeBranchMergeProposal()

Subscribers

People subscribed via source and target branches

to status/vote changes: