launchpad-reviewers team mailing list archive

[Celso Providelo <celso.providelo@xxxxxxxxxxxxx>]

Celso Providelo has proposed merging lp:~cprov/launchpad/ic-ui-stab-two into lp:launchpad with lp:~cprov/launchpad/ic-ui-stab-one as a prerequisite.

Commit message:
Create *scroller* links to navigate from review comments to their related inline-comments.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cprov/launchpad/ic-ui-stab-two/+merge/213950

Creating *scroller* links to navigate from review comments to their related inline-comments.

It uses the existing link_scroller() implementation to change the page focus to the review-diff area and uses the DIffNav() to update the diff and comments to the related contents.

Issue: link_scoller() uses (fixed) time dependant Y.anim which makes it pretty much untestable atm.
-- 
https://code.launchpad.net/~cprov/launchpad/ic-ui-stab-two/+merge/213950
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cprov/launchpad/ic-ui-stab-two into lp:launchpad.

=== modified file 'lib/lp/code/browser/codereviewcomment.py'
--- lib/lp/code/browser/codereviewcomment.py	2014-02-25 16:53:24 +0000
+++ lib/lp/code/browser/codereviewcomment.py	2014-04-03 00:48:06 +0000
@@ -98,6 +98,14 @@
             return ''
 
     @cachedproperty
+    def previewdiff_id(self):
+        inline_comment = getUtility(
+            ICodeReviewInlineCommentSet).getByReviewComment(self.comment)
+        if inline_comment is not None:
+            return inline_comment.previewdiff.id
+        return None
+
+    @cachedproperty
     def body_text(self):
         """Get the body text for the message."""
         return self.comment.message_body

=== modified file 'lib/lp/code/browser/tests/test_codereviewcomment.py'
--- lib/lp/code/browser/tests/test_codereviewcomment.py	2012-12-26 01:32:19 +0000
+++ lib/lp/code/browser/tests/test_codereviewcomment.py	2014-04-03 00:48:06 +0000
@@ -10,11 +10,15 @@
     Tag,
     )
 from testtools.matchers import Not
+from zope.component import getUtility
 
 from lp.code.browser.codereviewcomment import (
     CodeReviewDisplayComment,
     ICodeReviewDisplayComment,
     )
+from lp.code.interfaces.codereviewinlinecomment import (
+    ICodeReviewInlineCommentSet,
+    )
 from lp.services.webapp import canonical_url
 from lp.services.webapp.interfaces import IPrimaryContext
 from lp.testing import (
@@ -23,7 +27,10 @@
     TestCaseWithFactory,
     verifyObject,
     )
-from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
+    )
 
 
 class TestCodeReviewComments(TestCaseWithFactory):
@@ -54,6 +61,37 @@
         verifyObject(ICodeReviewDisplayComment, display_comment)
 
 
+class TestCodeReviewCommentInlineComments(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def test_display_comment_inline_comment(self):
+        # The CodeReviewDisplayComment links to related inline comments
+        # when they exist.
+        person = self.factory.makePerson()
+        with person_logged_in(person):
+            comment = self.factory.makeCodeReviewComment()
+        # `CodeReviewDisplayComment.previewdiff_id` is None if there
+        # is no related inline-comments.
+        display_comment = CodeReviewDisplayComment(comment)
+        self.assertIsNone(display_comment.previewdiff_id)
+        # Create a `PreviewDiff` and add inline-comments in
+        # the context of this review comment.
+        with person_logged_in(person):
+            previewdiff = self.factory.makePreviewDiff()
+            getUtility(ICodeReviewInlineCommentSet).ensureDraft(
+                previewdiff, person, {'1': 'Foo'})
+            getUtility(ICodeReviewInlineCommentSet).publishDraft(
+                previewdiff, person, comment)
+        # 'previewdiff_id' property is cached, so its value did not
+        # change on the existing object.
+        self.assertIsNone(display_comment.previewdiff_id)
+        # On a new object, it successfully returns the `PreviewDiff.id`
+        # containing inline-comments related with this review comment.
+        display_comment = CodeReviewDisplayComment(comment)
+        self.assertEqual(previewdiff.id, display_comment.previewdiff_id)
+
+
 class TestCodeReviewCommentHtml(BrowserTestCase):
 
     layer = DatabaseFunctionalLayer

=== modified file 'lib/lp/code/javascript/branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2014-04-03 00:48:06 +0000
+++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2014-04-03 00:48:06 +0000
@@ -234,7 +234,7 @@
 
 Y.extend(DiffNav, Y.Widget, {
 
-    /*
+    /**
      * The initializer method that is called from the base Plugin class.
      *
      * @method initializer
@@ -249,7 +249,7 @@
         }
     },
 
-    /*
+    /**
      * Add the spinner image to the diff section title.
      *
      * @method set_status_updating
@@ -284,7 +284,7 @@
         this.get('srcNode').all('h2 img').remove();
     },
 
-    /*
+    /**
      * Helper method to update diff-content area according to given
      * diff content uri
      *
@@ -320,7 +320,22 @@
         this.get('lp_client').get(preview_diff_uri, config);
     },
 
-    /*
+    /**
+     * Helper method to update the diff-content according to the
+     * navigator selected value.
+     *
+     * @method _showCurrentDiff
+     */
+    _showCurrentDiff: function(navigator) {
+        var self = this;
+        var previewdiff_id = navigator.get('value');
+        var preview_diff_uri = (
+            LP.cache.context.web_link +
+            '/+preview-diff/' + previewdiff_id + '/+diff');
+        self._updateDiff(preview_diff_uri);
+    },
+
+    /**
      * Helper method to connect changes on the diff navigator (<select>)
      * to the diff-content updater.
      *
@@ -329,15 +344,29 @@
     _connectNavigator: function(navigator) {
         var self = this;
         navigator.on('change', function() {
-            var previewdiff_id = this.get('value');
-            var preview_diff_uri = (
-                LP.cache.context.web_link +
-                '/+preview-diff/' + previewdiff_id + '/+diff');
-            self._updateDiff(preview_diff_uri);
+            self._showCurrentDiff(this);
         });
     },
 
-    /*
+    /**
+     * Helper method to connect inline-comment-scroller links to the 
+     * to the diff navigator.
+     *
+     * @method _connectScrollers
+     */
+    _connectScrollers: function(navigator) {
+        var self = this;
+        var rc = Y.lp.code.branchmergeproposal.reviewcomment;
+	Y.all('.inline-comment-scroller').each( function(node) {
+             rc.link_scroller(node, '#review-diff', function() {
+                 var previewdiff_id = node.one('span').get('text');
+                 navigator.set('value', previewdiff_id);
+                 navigator.simulate('change');
+             });
+        })
+    },
+
+    /**
      * Render diff navigator contents (navigator and diff area).
      *
      * @method renderUI
@@ -378,11 +407,8 @@
                     });
                     container.append(navigator)
                     self._connectNavigator(navigator);
-                    var previewdiff_id = navigator.get('value');
-                    var preview_diff_uri = (
-                        LP.cache.context.web_link +
-                        '/+preview-diff/' + previewdiff_id + '/+diff');
-                    self._updateDiff(preview_diff_uri);
+                    self._connectScrollers(navigator);
+                    self._showCurrentDiff(navigator);
                 },
                 failure: function(ignore, response, args) {
                     Y.log('DiffNav: ' + preview_diffs_uri +

=== modified file 'lib/lp/code/templates/codereviewcomment-header.pt'
--- lib/lp/code/templates/codereviewcomment-header.pt	2011-12-22 09:05:46 +0000
+++ lib/lp/code/templates/codereviewcomment-header.pt	2014-04-03 00:48:06 +0000
@@ -26,6 +26,12 @@
               tal:attributes="href context/branch_merge_proposal/fmt:url">a
               previous version</a>
             of this proposal</span>
+        <span style="float: right;" tal:condition="context/previewdiff_id">
+          <a class="inline-comment-scroller" href="">
+            View inline comments
+            <span style="display: none;" tal:content="context/previewdiff_id"/>
+          </a>
+        </span>
       </td>
 
       <td class="bug-comment-index">