launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/remove-inline-comment-id-leftovers into lp:launchpad.

Commit message:
Remove data-comment-id from inline comments.  It used to be needed to keep track of scrollers, but is no longer.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/remove-inline-comment-id-leftovers/+merge/327875

Spotted by eslint.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/remove-inline-comment-id-leftovers into lp:launchpad.

=== modified file 'lib/lp/code/javascript/branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2015-06-23 17:42:59 +0000
+++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2017-07-21 12:15:50 +0000
@@ -566,7 +566,6 @@
                 return;
             }
             var previewdiff_id = td.getData('previewdiff-id');
-            var comment_id = td.getData('comment-id');
             // We have to remove the old scrollers otherwise they will
             // fire multiple 'click' handlers (and animations).
             var scroller = td.one('.ic-scroller');

=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html	2015-04-09 03:36:08 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html	2017-07-21 12:15:50 +0000
@@ -82,20 +82,16 @@
 
       <table class="conversation">
       <tr>
-        <td data-previewdiff-id="102" data-from-superseded="True"
-            data-comment-id="10">
+        <td data-previewdiff-id="102" data-from-superseded="True">
            Comment from superseded </td>
       </tr><tr>
-        <td data-previewdiff-id="101" data-from-superseded="False"
-            data-comment-id="1">
+        <td data-previewdiff-id="101" data-from-superseded="False">
            Comment One </td>
       </tr><tr>
-        <td data-previewdiff-id="202" data-from-superseded="False"
-            data-comment-id="2">
+        <td data-previewdiff-id="202" data-from-superseded="False">
            Comment Two </td>
       </tr><tr>
-        <td data-from-superseded="False"
-            data-comment-id="3">
+        <td data-from-superseded="False">
            No inlines </td>
       </tr>
       </table>

=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js	2015-06-23 12:33:26 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js	2017-07-21 12:15:50 +0000
@@ -494,9 +494,7 @@
                  'Comment Two Show diff comments',
                  'No inlines'],
                 lines);
-            // Scroller are identified as:
-            // 'ic-scroller-<previewdiff_id>-<comment_id>'.
-            var comment = Y.one('[data-comment-id="2"]');
+            var comment = Y.one('[data-previewdiff-id="202"]');
             var scroller = comment.one('.ic-scroller');
             // We need to re-instrument the scroller in other to
             // instantly fire the 'end' event (which runs the code
@@ -527,8 +525,7 @@
             this.diffnav.set('navigator', Y.one('select'));
             // Let's create add a new scroller to the page.
             var new_comment = Y.Node.create(
-                '<tr><td data-previewdiff-id="202" data-comment-id="3">' +
-                'Comment Three</td></tr>');
+                '<tr><td data-previewdiff-id="202">Comment Three</td></tr>');
             Y.one('.conversation').append(new_comment);
             // and instrument the inline-comment functions to remove
             // and populate comments.

=== modified file 'lib/lp/code/stories/branches/xx-code-review-comments.txt'
--- lib/lp/code/stories/branches/xx-code-review-comments.txt	2016-09-07 11:12:58 +0000
+++ lib/lp/code/stories/branches/xx-code-review-comments.txt	2017-07-21 12:15:50 +0000
@@ -263,8 +263,6 @@
     >>> transaction.commit()
     >>> previewdiff_id = previewdiff.id
     >>> new_previewdiff_id = new_previewdiff.id
-    >>> comment1_id = comment1.id
-    >>> comment2_id = comment2.id
     >>> logout()
 
     >>> def get_comment_attributes(contents):
@@ -275,27 +273,25 @@
     ...         if len(tds) == 0:
     ...            continue
     ...         td = tds[0]
-    ...         result[td['data-comment-id']] = (
-    ...              td.get('data-previewdiff-id', '-'),
-    ...              td.get('data-from-superseded', '-'))
+    ...         if td.get('data-previewdiff-id'):
+    ...             result[td['data-previewdiff-id']] = (
+    ...                 td.get('data-from-superseded', '-'))
     ...     return result
 
 The 'data-' attributes:
 
- * 'comment-id': The review comment ID, used to highlight related
- *               inline comments.
  * 'previewdiff-id': used to load the corresponding `PreviewDiff`.
  * 'from_superseded': 'True' or 'False' whether the context MP is
-                      superseded by another. Used to supress context
+                      superseded by another. Used to suppress context
                       handlers on superseded comments.
 
 They are always available in `BranchMergeProposal` pages.
 
     >>> anon_browser.open(merge_proposal_url)
     >>> comments = get_comment_attributes(anon_browser.contents)
-    >>> comments[str(comment1_id)] == (str(previewdiff_id), 'False')
+    >>> comments[str(previewdiff_id)] == 'False'
     True
-    >>> str(comment2_id) in comments
+    >>> str(new_previewdiff_id) in comments
     False
 
 When visualized in the new merge proposal the comments from the original
@@ -304,7 +300,7 @@
 
     >>> anon_browser.open(new_merge_proposal_url)
     >>> comments = get_comment_attributes(anon_browser.contents)
-    >>> comments[str(comment1_id)] == (str(previewdiff_id), 'True')
+    >>> comments[str(previewdiff_id)] == 'True'
     True
-    >>> comments[str(comment2_id)] == (str(new_previewdiff_id), 'False')
+    >>> comments[str(new_previewdiff_id)] == 'False'
     True

=== modified file 'lib/lp/code/templates/codereviewcomment-header.pt'
--- lib/lp/code/templates/codereviewcomment-header.pt	2014-05-14 20:03:00 +0000
+++ lib/lp/code/templates/codereviewcomment-header.pt	2017-07-21 12:15:50 +0000
@@ -7,8 +7,7 @@
   <tbody>
     <tr>
     <td tal:attributes="data-previewdiff-id context/previewdiff_id;
-                        data-from-superseded context/from_superseded;
-                        data-comment-id context/id">
+                        data-from-superseded context/from_superseded">
       <span
         itemprop="creator"
         tal:content="structure context/comment_author/fmt:link-display-name-id"/>