launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/preview-diff-none-type into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1020439 in Launchpad itself: "AttributeError: 'NoneType' object has no attribute 'items'  in +preview-diff"
  https://bugs.launchpad.net/launchpad/+bug/1020439

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/preview-diff-none-type/+merge/126601

IDiff.diffstat is exported as Dict(), so lazr.restful attempts to unmarshall it as so. This would be fine, except when the column in the DB is NULL and the method returns None. lazr.restful's dict marshaller then tries to treat None as a dict and responds by de-praming its toys.

I have clawed back the LoC by some refactoring of the tests and removing the now unneeded pylint garbage.
-- 
https://code.launchpad.net/~stevenk/launchpad/preview-diff-none-type/+merge/126601
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/preview-diff-none-type into lp:launchpad.

=== modified file 'lib/lp/code/interfaces/diff.py'
--- lib/lp/code/interfaces/diff.py	2011-12-24 16:54:44 +0000
+++ lib/lp/code/interfaces/diff.py	2012-09-27 05:33:40 +0000
@@ -1,8 +1,6 @@
 # Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-# pylint: disable-msg=E0211,E0213
-
 """Interfaces including and related to IDiff."""
 
 __metaclass__ = type

=== modified file 'lib/lp/code/model/diff.py'
--- lib/lp/code/model/diff.py	2011-12-30 06:14:56 +0000
+++ lib/lp/code/model/diff.py	2012-09-27 05:33:40 +0000
@@ -70,7 +70,7 @@
 
     def _get_diffstat(self):
         if self._diffstat is None:
-            return None
+            return {}
         return dict((key, tuple(value))
                     for key, value
                     in simplejson.loads(self._diffstat).items())

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py	2012-09-19 01:19:35 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py	2012-09-27 05:33:40 +0000
@@ -1,8 +1,6 @@
 # Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-# pylint: disable-msg=F0401
-
 """Tests for BranchMergeProposals."""
 
 __metaclass__ = type
@@ -1521,13 +1519,11 @@
         login_person(merge_proposal.source_branch.owner)
         reviewer = self.factory.makePerson()
         reference = merge_proposal.nominateReviewer(
-            reviewer=reviewer,
-            registrant=merge_proposal.source_branch.owner,
+            reviewer=reviewer, registrant=merge_proposal.source_branch.owner,
             review_type='General')
         self.assertEqual('general', reference.review_type)
         merge_proposal.nominateReviewer(
-            reviewer=reviewer,
-            registrant=merge_proposal.source_branch.owner,
+            reviewer=reviewer, registrant=merge_proposal.source_branch.owner,
             review_type='Specific')
         # Note we're using the reference from the first call
         self.assertEqual('specific', reference.review_type)
@@ -1539,11 +1535,9 @@
             BranchSubscriptionNotificationLevel.NOEMAIL,
             sub.notification_level)
         self.assertEqual(
-            BranchSubscriptionDiffSize.NODIFF,
-            sub.max_diff_lines)
+            BranchSubscriptionDiffSize.NODIFF, sub.max_diff_lines)
         self.assertEqual(
-            CodeReviewNotificationLevel.FULL,
-            sub.review_level)
+            CodeReviewNotificationLevel.FULL, sub.review_level)
         # The reviewer can see the branch.
         self.assertTrue(branch.visibleByUser(reviewer))
         if branch.stacked_on is not None:
@@ -1585,6 +1579,14 @@
             membership_policy=TeamMembershipPolicy.MODERATED)
         self._test_nominate_grants_visibility(reviewer)
 
+    def assertVoteReference(self, votes, reviewer, comment):
+        self.assertEqual(1, len(votes))
+        vote_reference = votes[0]
+        self.assertEqual(reviewer, vote_reference.reviewer)
+        self.assertEqual(reviewer, vote_reference.registrant)
+        self.assertIsNone(vote_reference.review_type)
+        self.assertEqual(comment, vote_reference.comment)
+
     def test_comment_with_vote_creates_reference(self):
         """A comment with a vote creates a vote reference."""
         reviewer = self.factory.makePerson()
@@ -1594,12 +1596,7 @@
             reviewer, 'Message subject', 'Message content',
             vote=CodeReviewVote.APPROVE)
         votes = list(merge_proposal.votes)
-        self.assertEqual(1, len(votes))
-        vote_reference = votes[0]
-        self.assertEqual(reviewer, vote_reference.reviewer)
-        self.assertEqual(reviewer, vote_reference.registrant)
-        self.assertTrue(vote_reference.review_type is None)
-        self.assertEqual(comment, vote_reference.comment)
+        self.assertVoteReference(votes, reviewer, comment)
 
     def test_comment_without_a_vote_does_not_create_reference(self):
         """A comment with a vote creates a vote reference."""
@@ -1621,12 +1618,7 @@
             reviewer, 'Message subject', 'Message content',
             vote=CodeReviewVote.APPROVE)
         votes = list(merge_proposal.votes)
-        self.assertEqual(1, len(votes))
-        vote_reference = votes[0]
-        self.assertEqual(reviewer, vote_reference.reviewer)
-        self.assertEqual(reviewer, vote_reference.registrant)
-        self.assertTrue(vote_reference.review_type is None)
-        self.assertEqual(comment2, vote_reference.comment)
+        self.assertVoteReference(votes, reviewer, comment2)
 
     def test_vote_by_nominated_reuses_reference(self):
         """A comment with a vote for a nominated reviewer alters reference."""
@@ -2086,3 +2078,11 @@
             BadRequest,
             '(.|\n)*Invalid state transition for merge proposal(.|\n)*'):
             ws_bmp.setStatus(status='Approved')
+
+    def test_previewdiff_with_null_diffstat(self):
+        # A previewdiff with an empty diffstat doesn't crash when fetched.
+        previewdiff = self.factory.makePreviewDiff()
+        previewdiff.diff.diffstat = None
+        user = previewdiff.branch_merge_proposal.target_branch.owner
+        ws_previewdiff = self.wsObject(previewdiff, user=user)
+        self.assertEqual({}, ws_previewdiff.diffstat)