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

[Tim Penhey]

[Tim Penhey (thumper)]

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

[Michael Hudson-Doyle (mwhudson)]

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.

[Michael Hudson-Doyle (mwhudson)]

review: Approve

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2009-11-11 02:33:20 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2009-11-27 03:48:14 +0000
@@ -502,13 +502,27 @@
         self.syncUpdate()
         return proposal
 
+    def _normalizeReviewType(self, review_type):
+        """Normalse the review type.
+
+        If review_type is None, it stays None.  Otherwise the review_type is
+        converted to lower case, and if the string is empty is gets changed to
+        None.
+        """
+        if review_type is not None:
+            review_type = review_type.strip()
+            if review_type == '':
+                review_type = None
+            else:
+                review_type = review_type.lower()
+        return review_type
+
     def nominateReviewer(self, reviewer, registrant, review_type=None,
                          _date_created=DEFAULT, _notify_listeners=True):
         """See `IBranchMergeProposal`."""
         # Return the existing vote reference or create a new one.
         # Lower case the review type.
-        if review_type is not None:
-            review_type = review_type.lower()
+        review_type = self._normalizeReviewType(review_type)
         vote_reference = self.getUsersVoteReference(reviewer, review_type)
         if vote_reference is None:
             vote_reference = CodeReviewVoteReference(
@@ -559,6 +573,7 @@
         #:param _date_created: The date the message was created.  Provided
         #    only for testing purposes, as it can break
         # BranchMergeProposal.root_message.
+        review_type = self._normalizeReviewType(review_type)
         assert owner is not None, 'Merge proposal messages need a sender'
         parent_message = None
         if parent is not None:
@@ -590,8 +605,7 @@
     def getUsersVoteReference(self, user, review_type=None):
         """Get the existing vote reference for the given user."""
         # Lower case the review type.
-        if review_type is not None:
-            review_type = review_type.lower()
+        review_type = self._normalizeReviewType(review_type)
         if user is None:
             return None
         if user.is_team:
@@ -656,9 +670,7 @@
         """See `IBranchMergeProposal`."""
         if _validate:
             validate_message(original_email)
-        # Lower case the review type.
-        if review_type is not None:
-            review_type = review_type.lower()
+        review_type = self._normalizeReviewType(review_type)
         code_review_message = CodeReviewComment(
             branch_merge_proposal=self, message=message, vote=vote,
             vote_tag=review_type)

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
--- lib/lp/code/model/tests/test_branchmergeproposals.py	2009-11-03 21:05:14 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposals.py	2009-11-27 03:48:14 +0000
@@ -1437,39 +1437,82 @@
         merge_proposal = self.factory.makeBranchMergeProposal()
         self.assertEqual([], list(merge_proposal.votes))
 
+    def makeProposalWithReviewer(self, reviewer=None, review_type=None,
+                                 registrant=None):
+        """Make a proposal and request a review from reviewer.
+
+        If no reviewer is passed in, make a reviewer.
+        """
+        if reviewer is None:
+            reviewer = self.factory.makePerson()
+        merge_proposal = self.factory.makeBranchMergeProposal()
+        if registrant is None:
+            registrant = merge_proposal.registrant
+        login_person(merge_proposal.source_branch.owner)
+        merge_proposal.nominateReviewer(
+            reviewer=reviewer, registrant=registrant, review_type=review_type)
+        return merge_proposal, reviewer
+
+    def test_pending_review_registrant(self):
+        # The registrant passed into the nominateReviewer call is the
+        # registrant of the vote reference.
+        registrant = self.factory.makePerson()
+        merge_proposal, reviewer = self.makeProposalWithReviewer(
+            registrant=registrant)
+        vote_reference = list(merge_proposal.votes)[0]
+        self.assertEqual(registrant, vote_reference.registrant)
+
+    def assertOneReviewPending(self, merge_proposal, reviewer, review_type):
+        # Check that there is one and only one review pending with the
+        # specified reviewer and review_type.
+        votes = list(merge_proposal.votes)
+        self.assertEqual(1, len(votes))
+        vote_reference = votes[0]
+        self.assertEqual(reviewer, vote_reference.reviewer)
+        if review_type is None:
+            self.assertIs(None, vote_reference.review_type)
+        else:
+            self.assertEqual(review_type, vote_reference.review_type)
+        self.assertIs(None, vote_reference.comment)
+
     def test_nominate_creates_reference(self):
-        """A new vote reference is created when a reviewer is nominated."""
-        merge_proposal = self.factory.makeBranchMergeProposal()
-        login_person(merge_proposal.source_branch.owner)
-        reviewer = self.factory.makePerson()
-        merge_proposal.nominateReviewer(
-            reviewer=reviewer,
-            registrant=merge_proposal.source_branch.owner,
+        # A new vote reference is created when a reviewer is nominated.
+        merge_proposal, reviewer = self.makeProposalWithReviewer(
             review_type='General')
-        votes = list(merge_proposal.votes)
-        self.assertEqual(1, len(votes))
-        vote_reference = votes[0]
-        self.assertEqual(reviewer, vote_reference.reviewer)
-        self.assertEqual(merge_proposal.source_branch.owner,
-                         vote_reference.registrant)
-        self.assertEqual('general', vote_reference.review_type)
+        self.assertOneReviewPending(merge_proposal, reviewer, 'general')
+
+    def test_nominate_with_None_review_type(self):
+        # Reviews nominated with a review type of None, make vote references
+        # with a review_type of None.
+        merge_proposal, reviewer = self.makeProposalWithReviewer(
+            review_type=None)
+        self.assertOneReviewPending(merge_proposal, reviewer, None)
+
+    def test_nominate_with_whitespace_review_type(self):
+        # A review nominated with a review type that just contains whitespace
+        # or the empty string, makes a vote reference with a review_type of
+        # None.
+        merge_proposal, reviewer = self.makeProposalWithReviewer(
+            review_type='')
+        self.assertOneReviewPending(merge_proposal, reviewer, None)
+        merge_proposal, reviewer = self.makeProposalWithReviewer(
+            review_type='    ')
+        self.assertOneReviewPending(merge_proposal, reviewer, None)
+        merge_proposal, reviewer = self.makeProposalWithReviewer(
+            review_type='\t')
+        self.assertOneReviewPending(merge_proposal, reviewer, None)
 
     def test_nominate_multiple_with_different_types(self):
         # While an individual can only be requested to do one review
         # (test_nominate_updates_reference) a team can have multiple
         # nominations for different review types.
-        merge_proposal = self.factory.makeBranchMergeProposal()
-        login_person(merge_proposal.source_branch.owner)
         reviewer = self.factory.makePerson()
         review_team = self.factory.makeTeam(owner=reviewer)
-        merge_proposal.nominateReviewer(
-            reviewer=review_team,
-            registrant=merge_proposal.source_branch.owner,
-            review_type='general-1')
-        # Second nomination of the same type fails.
-        merge_proposal.nominateReviewer(
-            reviewer=review_team,
-            registrant=merge_proposal.source_branch.owner,
+        merge_proposal, reviewer = self.makeProposalWithReviewer(
+            reviewer=review_team, review_type='general-1')
+        merge_proposal.nominateReviewer(
+            reviewer=review_team,
+            registrant=merge_proposal.registrant,
             review_type='general-2')
 
         votes = list(merge_proposal.votes)