launchpad-reviewers team mailing list archive

[Celso Providelo <celso.providelo@xxxxxxxxxxxxx>]

Celso Providelo has proposed merging lp:~cprov/launchpad/ic-ux-stab-1 into lp:launchpad.

Commit message:
Anonymous users can use DiffNav() to navigate across existing diffs and view published inline comments on public MPs

Requested reviews:
  William Grant (wgrant): code

For more details, see:
https://code.launchpad.net/~cprov/launchpad/ic-ux-stab-1/+merge/218553

Anonymous users can use DiffNav() to navigate across existing diffs and view published inline comments on public MPs. IBMP.{getPreviewDiff,getInlineComments} do not require login anymore, only lp.View.
-- 
https://code.launchpad.net/~cprov/launchpad/ic-ux-stab-1/+merge/218553
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml	2014-02-27 17:45:09 +0000
+++ lib/lp/code/browser/configure.zcml	2014-05-07 06:34:42 +0000
@@ -839,7 +839,7 @@
         name="+diff"
         for="lp.code.interfaces.diff.IPreviewDiff"
         class="lp.code.browser.branchmergeproposal.FormatPreviewDiffView"
-        permission="launchpad.AnyPerson"
+        permission="launchpad.View"
         template="../templates/branchmergeproposal-diff.pt"/>
 
     <browser:menus

=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
--- lib/lp/code/interfaces/branchmergeproposal.py	2014-02-25 16:53:24 +0000
+++ lib/lp/code/interfaces/branchmergeproposal.py	2014-05-07 06:34:42 +0000
@@ -376,6 +376,26 @@
             Revisions.
         """
 
+    def getPreviewDiff(id):
+        """Return the preview diff with the given id.
+
+        :param id: The id of the target `PreviewDiff`.
+        """
+
+    @export_read_operation()
+    @operation_parameters(
+        previewdiff_id=Int())
+    @operation_for_version('devel')
+    def getInlineComments(previewdiff_id):
+        """Return a list of inline comments related to this MP.
+
+        The return value is an list of dictionaries (objects), each one
+        representing a comment with 'line_number', 'person', 'text' and
+        'date' attributes.
+
+        :param previewdiff_id: The ID of the target `PreviewDiff`.
+        """
+
 
 class IBranchMergeProposalEdit(Interface):
 
@@ -583,20 +603,6 @@
     @export_read_operation()
     @operation_parameters(
         previewdiff_id=Int())
-    @operation_for_version('devel')
-    def getInlineComments(previewdiff_id):
-        """Return a list of inline comments related to this MP.
-
-        The return value is an list of dictionaries (objects), each one
-        representing a comment with 'line_number', 'person', 'text' and
-        'date' attributes.
-
-        :param previewdiff_id: The ID of the target `PreviewDiff`.
-        """
-
-    @export_read_operation()
-    @operation_parameters(
-        previewdiff_id=Int())
     @call_with(person=REQUEST_USER)
     @operation_for_version('devel')
     def getDraftInlineComments(previewdiff_id, person):
@@ -610,12 +616,6 @@
         :param person: The `IPerson` owner of the draft comments.
         """
 
-    def getPreviewDiff(id):
-        """Return the preview diff with the given id.
-
-        :param id: The id of the target `PreviewDiff`.
-        """
-
     @export_write_operation()
     @operation_parameters(
         previewdiff_id=Int(),

=== modified file 'lib/lp/code/javascript/branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2014-05-07 02:35:56 +0000
+++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2014-05-07 06:34:42 +0000
@@ -158,13 +158,15 @@
     return headerrow;
 };
 
-namespace.populate_existing_comments = function() {
+namespace.cleanup_comments = function() {
     // Cleanup previous inline review comments.
     Y.all('.ict-header').each( function(line) {
         line.next().remove();
         line.remove();
     });
+};
 
+namespace.populate_comments = function() {
     var config_published = {
         on: {
             success: function (comments) {
@@ -182,7 +184,9 @@
     };
     namespace.lp_client.named_get(
         LP.cache.context.self_link, 'getInlineComments', config_published);
+};
 
+namespace.populate_drafts = function() {
     var config_draft = {
         on: {
             success: function (drafts) {
@@ -215,11 +219,21 @@
     }
     // Store the current diff ID locally.
     namespace.current_previewdiff_id = previewdiff_id;
-    // Add the double-click handler for each row in the diff. This needs
-    // to be done first since populating existing published and draft
-    // comments may add more rows.
-    namespace.add_doubleclick_handler();
-    namespace.populate_existing_comments();
+
+    namespace.cleanup_comments();
+
+    // Draft inline-comments and click-handlers do not need to be
+    // loaded for anonymous requests.
+    // In fact, if loading draft is attempted is will fail due to
+    // the LP permission checks.
+    if (LP.links['me'] !== undefined) {
+        // Add the double-click handler for each row in the diff. This needs
+        // to be done first since populating existing published and draft
+        // comments may add more rows.
+        namespace.add_doubleclick_handler();
+        namespace.populate_drafts();
+    }
+    namespace.populate_comments();
 };
 
 
@@ -381,7 +395,7 @@
         if (LP.cache.inline_diff_comments !== true) {
             return;
         }
-        namespace.populate_existing_comments();
+        namespace.populate_comments();
         this._connectScrollers();
     },
 

=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js	2014-05-07 02:35:56 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js	2014-05-07 06:34:42 +0000
@@ -16,7 +16,9 @@
                 self_link: ("https://code.launchpad.dev/api/devel/"; +
                             "~foo/bar/foobr/+merge/1")
             };
-
+            LP.links = {
+                me : 'something'
+            };
             LP.cache.inline_diff_comments = true;
         },
 
@@ -44,7 +46,8 @@
 
             // Populate the page.
             module.current_previewdiff_id = 1;
-            module.populate_existing_comments();
+            module.populate_comments();
+            module.populate_drafts();
 
             // LP was hit twice for fetching published and draft inline
             // comments
@@ -173,7 +176,6 @@
             module.current_previewdiff_id = null;
 
             LP.cache.inline_diff_comments = false;
-            module.current_previewdiff_id = null;
             module.setup_inline_comments(1);
 
             Y.Assert.isFalse(called);
@@ -192,6 +194,27 @@
 
             Y.Assert.isTrue(called);
             Y.Assert.areEqual(1, module.current_previewdiff_id);
+        },
+
+        test_feature_not_logged_in: function() {
+            // Draft inline-comments are not loaded and doubleclick-handlers
+            // are not set for anonymous requests.
+            var doubleclick_called = false;
+            var draft_called = false;
+            module.add_doubleclick_handler = function() {
+                doubleclick_called = true;
+            };
+            module.populate_draft = function() {
+                draft_called = true;
+            };
+            module.current_previewdiff_id = null;
+
+            LP.links = {};
+            module.setup_inline_comments(1);
+
+            Y.Assert.isFalse(doubleclick_called);
+            Y.Assert.isFalse(draft_called);
+            Y.Assert.areEqual(1, module.current_previewdiff_id);
         }
 
     }));
@@ -429,7 +452,7 @@
             Y.one('#scroll-two').insert(new_scroller, 'after');
             // and instrument the inline-comment updated function.
             var populated = false;
-            module.populate_existing_comments = function () {
+            module.populate_comments = function () {
                 populated = true;
             };
             // Once called, update_on_new_comment() integrates the

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py	2014-02-27 12:42:03 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py	2014-05-07 06:34:42 +0000
@@ -2363,15 +2363,17 @@
 
     def test_saveDraftInlineComment(self):
         # Creating and retrieving draft inline comments.
+        # These operations require an logged in user with permission
+        # to view the BMP.
 
         # Enabled inline_diff feature.
         self.useContext(feature_flags())
         set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
 
         previewdiff = self.factory.makePreviewDiff()
-        user = previewdiff.branch_merge_proposal.target_branch.owner
-        ws_bmp = self.wsObject(previewdiff.branch_merge_proposal, user=user)
+        proposal = previewdiff.branch_merge_proposal
 
+        ws_bmp = self.wsObject(proposal, user=proposal.target_branch.owner)
         ws_bmp.saveDraftInlineComment(
             previewdiff_id=previewdiff.id,
             comments={'2': 'foo'})
@@ -2389,16 +2391,25 @@
         set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
 
         previewdiff = self.factory.makePreviewDiff()
-        user = previewdiff.branch_merge_proposal.target_branch.owner
-        ws_bmp = self.wsObject(previewdiff.branch_merge_proposal, user=user)
+        proposal = previewdiff.branch_merge_proposal
+        user = proposal.target_branch.owner
 
+        # Publishing inline-comments requires an logged in user with
+        # lp.Edit permission on the MP, in this case, the branch owner.
+        ws_bmp = self.wsObject(proposal, user=user)
         review_comment = ws_bmp.createComment(
             subject='Testing!',
             previewdiff_id=previewdiff.id,
             inline_comments={u'2': u'foo'})
         transaction.commit()
 
-        inline_comments = ws_bmp.getInlineComments(
+        # Retrieving published inline comments requires only lp.View
+        # permission on the MP, since the testing MP is public, even
+        # an anonymous user can view published inline comments.
+        launchpad = launchpadlib_for(
+            'test', None, service_root=self.layer.appserver_root_url('api'))
+        anon_bmp = ws_object(launchpad, proposal)
+        inline_comments = anon_bmp.getInlineComments(
             previewdiff_id=previewdiff.id)
 
         self.assertEqual(1, len(inline_comments))