Launchpad itself

Merge lp://qastaging/~thumper/launchpad/set-empty-review-type-as-none into lp://qastaging/launchpad

  1. set-empty-review-type-as-none
  2. Merge into devel
Proposed by Tim Penhey
Status: Merged
Merged at revision: 9960
Proposed branch: lp://qastaging/~thumper/launchpad/set-empty-review-type-as-none
Merge into: lp://qastaging/launchpad
Diff against target: 173 lines (+86/-31)
2 files modified
lib/lp/code/model/branchmergeproposal.py (+19/-7)
lib/lp/code/model/tests/test_branchmergeproposals.py (+67/-24)
To merge this branch: bzr merge lp://qastaging/~thumper/launchpad/set-empty-review-type-as-none
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle Approve
Review via email: mp+15302@code.qastaging.launchpad.net
To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

The failure to claim team reviews was primarily due to the mismatch between '' and None and the ways that the zope form machinery, the api and the email interface interact.

This branch normalizes the review_type at the model level to avoid this complication.

tests:
  TestBranchMergeProposalNominateReviewer

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

As said on IRC:

In test_pending_review_registrant it's not super obvious to me that the merge_proposal.registrant is who's passed into nominateReviewer, could you give makeProposalWithReviewer an optional "requester" argument?

All else fine, nice to get this little mystery under control.

Revision history for this message
Michael Hudson-Doyle (mwhudson) :
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/model/branchmergeproposal.py'
2--- lib/lp/code/model/branchmergeproposal.py 2009-11-11 02:33:20 +0000
3+++ lib/lp/code/model/branchmergeproposal.py 2009-11-27 03:48:14 +0000
4@@ -502,13 +502,27 @@
5 self.syncUpdate()
6 return proposal
7
8+ def _normalizeReviewType(self, review_type):
9+ """Normalse the review type.
10+
11+ If review_type is None, it stays None. Otherwise the review_type is
12+ converted to lower case, and if the string is empty is gets changed to
13+ None.
14+ """
15+ if review_type is not None:
16+ review_type = review_type.strip()
17+ if review_type == '':
18+ review_type = None
19+ else:
20+ review_type = review_type.lower()
21+ return review_type
22+
23 def nominateReviewer(self, reviewer, registrant, review_type=None,
24 _date_created=DEFAULT, _notify_listeners=True):
25 """See `IBranchMergeProposal`."""
26 # Return the existing vote reference or create a new one.
27 # Lower case the review type.
28- if review_type is not None:
29- review_type = review_type.lower()
30+ review_type = self._normalizeReviewType(review_type)
31 vote_reference = self.getUsersVoteReference(reviewer, review_type)
32 if vote_reference is None:
33 vote_reference = CodeReviewVoteReference(
34@@ -559,6 +573,7 @@
35 #:param _date_created: The date the message was created. Provided
36 # only for testing purposes, as it can break
37 # BranchMergeProposal.root_message.
38+ review_type = self._normalizeReviewType(review_type)
39 assert owner is not None, 'Merge proposal messages need a sender'
40 parent_message = None
41 if parent is not None:
42@@ -590,8 +605,7 @@
43 def getUsersVoteReference(self, user, review_type=None):
44 """Get the existing vote reference for the given user."""
45 # Lower case the review type.
46- if review_type is not None:
47- review_type = review_type.lower()
48+ review_type = self._normalizeReviewType(review_type)
49 if user is None:
50 return None
51 if user.is_team:
52@@ -656,9 +670,7 @@
53 """See `IBranchMergeProposal`."""
54 if _validate:
55 validate_message(original_email)
56- # Lower case the review type.
57- if review_type is not None:
58- review_type = review_type.lower()
59+ review_type = self._normalizeReviewType(review_type)
60 code_review_message = CodeReviewComment(
61 branch_merge_proposal=self, message=message, vote=vote,
62 vote_tag=review_type)
63
64=== modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
65--- lib/lp/code/model/tests/test_branchmergeproposals.py 2009-11-03 21:05:14 +0000
66+++ lib/lp/code/model/tests/test_branchmergeproposals.py 2009-11-27 03:48:14 +0000
67@@ -1437,39 +1437,82 @@
68 merge_proposal = self.factory.makeBranchMergeProposal()
69 self.assertEqual([], list(merge_proposal.votes))
70
71+ def makeProposalWithReviewer(self, reviewer=None, review_type=None,
72+ registrant=None):
73+ """Make a proposal and request a review from reviewer.
74+
75+ If no reviewer is passed in, make a reviewer.
76+ """
77+ if reviewer is None:
78+ reviewer = self.factory.makePerson()
79+ merge_proposal = self.factory.makeBranchMergeProposal()
80+ if registrant is None:
81+ registrant = merge_proposal.registrant
82+ login_person(merge_proposal.source_branch.owner)
83+ merge_proposal.nominateReviewer(
84+ reviewer=reviewer, registrant=registrant, review_type=review_type)
85+ return merge_proposal, reviewer
86+
87+ def test_pending_review_registrant(self):
88+ # The registrant passed into the nominateReviewer call is the
89+ # registrant of the vote reference.
90+ registrant = self.factory.makePerson()
91+ merge_proposal, reviewer = self.makeProposalWithReviewer(
92+ registrant=registrant)
93+ vote_reference = list(merge_proposal.votes)[0]
94+ self.assertEqual(registrant, vote_reference.registrant)
95+
96+ def assertOneReviewPending(self, merge_proposal, reviewer, review_type):
97+ # Check that there is one and only one review pending with the
98+ # specified reviewer and review_type.
99+ votes = list(merge_proposal.votes)
100+ self.assertEqual(1, len(votes))
101+ vote_reference = votes[0]
102+ self.assertEqual(reviewer, vote_reference.reviewer)
103+ if review_type is None:
104+ self.assertIs(None, vote_reference.review_type)
105+ else:
106+ self.assertEqual(review_type, vote_reference.review_type)
107+ self.assertIs(None, vote_reference.comment)
108+
109 def test_nominate_creates_reference(self):
110- """A new vote reference is created when a reviewer is nominated."""
111- merge_proposal = self.factory.makeBranchMergeProposal()
112- login_person(merge_proposal.source_branch.owner)
113- reviewer = self.factory.makePerson()
114- merge_proposal.nominateReviewer(
115- reviewer=reviewer,
116- registrant=merge_proposal.source_branch.owner,
117+ # A new vote reference is created when a reviewer is nominated.
118+ merge_proposal, reviewer = self.makeProposalWithReviewer(
119 review_type='General')
120- votes = list(merge_proposal.votes)
121- self.assertEqual(1, len(votes))
122- vote_reference = votes[0]
123- self.assertEqual(reviewer, vote_reference.reviewer)
124- self.assertEqual(merge_proposal.source_branch.owner,
125- vote_reference.registrant)
126- self.assertEqual('general', vote_reference.review_type)
127+ self.assertOneReviewPending(merge_proposal, reviewer, 'general')
128+
129+ def test_nominate_with_None_review_type(self):
130+ # Reviews nominated with a review type of None, make vote references
131+ # with a review_type of None.
132+ merge_proposal, reviewer = self.makeProposalWithReviewer(
133+ review_type=None)
134+ self.assertOneReviewPending(merge_proposal, reviewer, None)
135+
136+ def test_nominate_with_whitespace_review_type(self):
137+ # A review nominated with a review type that just contains whitespace
138+ # or the empty string, makes a vote reference with a review_type of
139+ # None.
140+ merge_proposal, reviewer = self.makeProposalWithReviewer(
141+ review_type='')
142+ self.assertOneReviewPending(merge_proposal, reviewer, None)
143+ merge_proposal, reviewer = self.makeProposalWithReviewer(
144+ review_type=' ')
145+ self.assertOneReviewPending(merge_proposal, reviewer, None)
146+ merge_proposal, reviewer = self.makeProposalWithReviewer(
147+ review_type='\t')
148+ self.assertOneReviewPending(merge_proposal, reviewer, None)
149
150 def test_nominate_multiple_with_different_types(self):
151 # While an individual can only be requested to do one review
152 # (test_nominate_updates_reference) a team can have multiple
153 # nominations for different review types.
154- merge_proposal = self.factory.makeBranchMergeProposal()
155- login_person(merge_proposal.source_branch.owner)
156 reviewer = self.factory.makePerson()
157 review_team = self.factory.makeTeam(owner=reviewer)
158- merge_proposal.nominateReviewer(
159- reviewer=review_team,
160- registrant=merge_proposal.source_branch.owner,
161- review_type='general-1')
162- # Second nomination of the same type fails.
163- merge_proposal.nominateReviewer(
164- reviewer=review_team,
165- registrant=merge_proposal.source_branch.owner,
166+ merge_proposal, reviewer = self.makeProposalWithReviewer(
167+ reviewer=review_team, review_type='general-1')
168+ merge_proposal.nominateReviewer(
169+ reviewer=review_team,
170+ registrant=merge_proposal.registrant,
171 review_type='general-2')
172
173 votes = list(merge_proposal.votes)