launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/ic-fixes into lp:launchpad.

Commit message:
Turn inline comments publish checkbox label into a real label, ensure that drafts are refreshed after publishing, and fix off-by-one error in inline comment email formatting.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/ic-fixes/+merge/220410

Fix three easy bugs in inline comments:

 - Turn 'Include X diff comments' into a label for the checkbox so it's possible to click without incredible precision.

 - Ensure that drafts are refreshed from the server after publishing them, as we otherwise end up with weird ghost drafts that either don't appear or disappear until refresh.

 - Fix inline comment off-by-one error. Comment dicts are 1-indexed, but the email formatter was accidentally just skipping line 0 instead of numbering from 1.

-- 
https://code.launchpad.net/~wgrant/launchpad/ic-fixes/+merge/220410
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/ic-fixes into lp:launchpad.

=== modified file 'lib/lp/code/javascript/branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2014-05-21 03:49:52 +0000
+++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2014-05-21 09:37:00 +0000
@@ -272,17 +272,17 @@
             Y.fire('CodeReviewComment.SET_DISABLED', true);
             return;
         }
-        var text = 'Include ' + n_drafts + ' diff comment';
+        var text = ' Include ' + n_drafts + ' diff comment';
         if (n_drafts > 1) text += 's';
-        var label = Y.Node.create('<a />')
-            .set('text', text);
         var checkbox = Y.Node.create(
             '<input id="field.publish_inline_comments"' +
             'name="field.publish_inline_comments"' +
             'type="checkbox" class="checkboxType"' +
             'checked=""></input>')
-        this.get('contentBox').append(checkbox);
-        this.get('contentBox').append(label);
+        var label = Y.Node.create(
+            '<label for="field.publish_inline_comments" />')
+            .set('text', text);
+        this.get('contentBox').append(checkbox).append(label);
         Y.fire('CodeReviewComment.SET_DISABLED', false);
     },
 
@@ -463,6 +463,7 @@
         }
         namespace.cleanup_comments();
         namespace.populate_comments();
+        namespace.populate_drafts();
         this._connectScrollers();
     },
 

=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js	2014-05-21 01:58:44 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js	2014-05-21 09:37:00 +0000
@@ -425,13 +425,13 @@
             // A event is fired when the inlinecomments set changes.
             Y.fire('inlinecomment.UPDATED');
             Y.Assert.areEqual(
-                 'Include 1 diff comment', container.get('text'));
+                 ' Include 1 diff comment', container.get('text'));
             Y.Assert.isNotNull(container.one('[type=checkbox]'));
             // Adding another draft.
             module.create_row({'line_number': '2', 'text': 'another'});
             Y.fire('inlinecomment.UPDATED');
             Y.Assert.areEqual(
-                 'Include 2 diff comments', container.get('text'));
+                 ' Include 2 diff comments', container.get('text'));
             Y.Assert.isNotNull(container.one('[type=checkbox]'));
             // Removing all drafts.
             module.cleanup_comments();

=== modified file 'lib/lp/code/mail/codereviewcomment.py'
--- lib/lp/code/mail/codereviewcomment.py	2014-05-21 01:42:39 +0000
+++ lib/lp/code/mail/codereviewcomment.py	2014-05-21 09:37:00 +0000
@@ -173,10 +173,9 @@
     """Return a formatted text section with contextualized comments."""
     result_lines = []
     diff_lines = diff_text.splitlines()
-    for i in range(1, len(diff_lines)):
-        result_lines.append(
-            u'> {0}'.format(diff_lines[i].decode('utf-8', 'replace')))
-        comment = comments.get(str(i))
+    for num, line in enumerate(diff_lines, 1):
+        result_lines.append(u'> {0}'.format(line.decode('utf-8', 'replace')))
+        comment = comments.get(str(num))
         if comment is not None:
             result_lines.append('')
             result_lines.extend(comment.splitlines())

=== modified file 'lib/lp/code/mail/tests/test_codereviewcomment.py'
--- lib/lp/code/mail/tests/test_codereviewcomment.py	2014-05-21 01:42:39 +0000
+++ lib/lp/code/mail/tests/test_codereviewcomment.py	2014-05-21 09:37:00 +0000
@@ -245,7 +245,7 @@
         set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
 
         comment = self.makeCommentWithInlineComments(
-            inline_comments={'1': u'Is this from Planet Earth\xa9 ?'})
+            inline_comments={'2': u'Is this from Planet Earth\xa9 ?'})
         mailer = CodeReviewCommentMailer.forCreation(comment)
         commenter = comment.branch_merge_proposal.registrant
         ctrl = mailer.generateEmail(
@@ -255,6 +255,7 @@
             '',
             'Diff comments:',
             '',
+            "> === zbqvsvrq svyr 'yvo/yc/pbqr/vagresnprf/qvss.cl'",
             '> --- yvo/yc/pbqr/vagresnprf/qvss.cl      '
             '2009-10-01 13:25:12 +0000',
             '',
@@ -263,7 +264,7 @@
             '> +++ yvo/yc/pbqr/vagresnprf/qvss.cl      '
             '2010-02-02 15:48:56 +0000'
         ]
-        self.assertEqual(expected_lines, ctrl.body.splitlines()[1:9])
+        self.assertEqual(expected_lines, ctrl.body.splitlines()[1:10])
 
     def test_generateEmailWithInlineComments_feature_disabled(self):
         """Inline comments are not considered if the flag is not enabled."""
@@ -434,7 +435,7 @@
              '',
              'Diff comments:',
              '',
-             '> +++ bar\t1969-12-31 19:00:00.000000000 -0500'],
+             '> --- bar\t2009-08-26 15:53:34.000000000 -0400'],
             header)
         footer = section[-2:]
         self.assertEqual(
@@ -445,7 +446,7 @@
     def test_single_line_comment(self):
         # The inline comments are correctly contextualized in the diff.
         # and prefixed with '>>> '
-        comments = {'1': u'\u03b4\u03bf\u03ba\u03b9\u03bc\u03ae'}
+        comments = {'2': u'\u03b4\u03bf\u03ba\u03b9\u03bc\u03ae'}
         self.assertEqual(
             ['> +++ bar\t1969-12-31 19:00:00.000000000 -0500',
              '',
@@ -453,11 +454,11 @@
              '',
              '> @@ -1,3 +0,0 @@',
              u'> -\xe5'],
-            self.getSection(comments).splitlines()[4:10])
+            self.getSection(comments).splitlines()[5:11])
 
     def test_multi_line_comment(self):
         # Inline comments with multiple lines are rendered appropriately.
-        comments = {'1': 'Foo\nBar'}
+        comments = {'2': 'Foo\nBar'}
         self.assertEqual(
             ['> +++ bar\t1969-12-31 19:00:00.000000000 -0500',
              '',
@@ -465,11 +466,11 @@
              'Bar',
              '',
              '> @@ -1,3 +0,0 @@'],
-            self.getSection(comments).splitlines()[4:10])
+            self.getSection(comments).splitlines()[5:11])
 
     def test_multiple_comments(self):
         # Multiple inline comments are redered appropriately.
-        comments = {'1': 'Foo', '2': 'Bar'}
+        comments = {'2': 'Foo', '3': 'Bar'}
         self.assertEqual(
             ['> +++ bar\t1969-12-31 19:00:00.000000000 -0500',
              '',
@@ -479,4 +480,4 @@
              '',
              'Bar',
              ''],
-            self.getSection(comments).splitlines()[4:12])
+            self.getSection(comments).splitlines()[5:13])