Merge lp:~cprov/launchpad/ic-ui-stab-one into lp:launchpad

[Celso Providelo]

Moving 'diff-content update' code from Updater() to the new DiffNav() widget (which works with and without inline-comments). The Updater() widget continues to handle diff-stats updates because it's not relevant for the inline-comments context.
Also rendering the spinner during DiffNav() actions, so users are not left uninformed.

[William Grant (wgrant)]

Some of the new code is indented with tabs rather than spaces, but otherwise this looks good.

review: Approve (code)

=== modified file 'lib/lp/code/javascript/branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2014-02-28 01:19:39 +0000
+++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2014-03-31 23:30:31 +0000
@@ -249,46 +249,107 @@
         }
     },
 
-    _connectNavigator: function(navigator) {
-        var self = this;
+    /*
+     * Add the spinner image to the diff section title.
+     *
+     * @method set_status_updating
+     */
+    set_status_updating: function() {
+       var image = Y.Node.create('<img />')
+           .set('src', '/@@/spinner')
+           .set('title', 'Updating diff ...');
+       this.get('srcNode').one('h2').append(image);
+    },
+
+    /**
+     * Run finishing tasks after the diff content is updated.
+     *
+     * @method updated
+     */
+    set_status_updated: function() {
         var rc = Y.lp.code.branchmergeproposal.reviewcomment;
         (new rc.NumberToggle()).render();
-        namespace.setup_inline_comments(navigator.get('value'));
-
+	var navigator = this.get('srcNode').one('select');
+        if (navigator) {
+            namespace.setup_inline_comments(navigator.get('value'));
+        }
+    },
+
+    /**
+     * Remove the spinner image from the diff section title.
+     *
+     * @method cleanup_status
+     */
+    cleanup_status: function() {
+        this.get('srcNode').all('h2 img').remove();
+    },
+
+    /*
+     * Helper method to update diff-content area according to given
+     * diff content uri
+     *
+     * @method _updateDiff
+     */
+    _updateDiff: function(preview_diff_uri) {
+        var self = this;
+        var container = self.get('srcNode').one('.diff-content');
+        var config = {
+            on: {
+                success: function (diff_html) {
+                    container.set('innerHTML', diff_html);
+                    self.set_status_updated();
+                },
+                failure: function(ignore, response, args) {
+                    Y.log('DiffNav: ' + preview_diff_uri +
+                          ' - ' + response.statusText);
+                    var error_note = Y.Node.create('<p />')
+                        .addClass('exception')
+                        .addClass('lowlight')
+                        .set('text', 'Failed to fetch diff content.');
+                    container.empty();
+                    container.append(error_note);
+                },
+                start: function() {
+                    self.set_status_updating();
+                },
+                end: function() {
+                    self.cleanup_status();
+                }
+            }
+        };
+        this.get('lp_client').get(preview_diff_uri, config);
+    },
+
+    /*
+     * Helper method to connect changes on the diff navigator (<select>)
+     * to the diff-content updater.
+     *
+     * @method _connectNavigator
+     */
+    _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');
-            var container = self.get('srcNode').one('.diff-content');
-            var config = {
-                on: {
-                    success: function (diff_html) {
-                        container.set('innerHTML', diff_html);
-                        (new rc.NumberToggle()).render();
-                        namespace.setup_inline_comments(previewdiff_id);
-                    },
-                    failure: function(ignore, response, args) {
-                        Y.log('DiffNav: ' + preview_diff_uri +
-                              ' - ' + response.statusText);
-                        var error_note = Y.Node.create('<p />')
-                            .addClass('exception')
-                            .addClass('lowlight')
-                            .set('text', 'Failed to fetch diff content.');
-                        container.empty();
-                        container.append(error_note);
-                    },
-                },
-            };
-            self.get('lp_client').get(preview_diff_uri, config);
+            self._updateDiff(preview_diff_uri);
         });
     },
 
+    /*
+     * Render diff navigator contents (navigator and diff area).
+     *
+     * @method renderUI
+     */
     renderUI: function() {
+        var self = this;
         if (LP.cache.inline_diff_comments !== true) {
-            return
+            var preview_diff_uri = (
+                LP.cache.context.web_link + '/++diff');
+            self._updateDiff(preview_diff_uri);
+	    return;
         }
-        var self = this;
         var container = self.get('srcNode').one('.diff-navigator');
         container.empty();
         var preview_diffs_uri = LP.cache.context.preview_diffs_collection_link;
@@ -317,6 +378,11 @@
                     });
                     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);
                 },
                 failure: function(ignore, response, args) {
                     Y.log('DiffNav: ' + preview_diffs_uri +
@@ -328,6 +394,12 @@
                     container.empty();
                     container.append(error_note);
                 },
+                start: function() {
+                    self.set_status_updating();
+                },
+                end: function() {
+                    self.cleanup_status();
+                }
             }
         };
         this.get('lp_client').get(preview_diffs_uri, config);

=== modified file 'lib/lp/code/javascript/branchmergeproposal.updater.js'
--- lib/lp/code/javascript/branchmergeproposal.updater.js	2014-02-27 19:26:31 +0000
+++ lib/lp/code/javascript/branchmergeproposal.updater.js	2014-03-31 23:30:31 +0000
@@ -77,28 +77,6 @@
                    'summary_node').one('#summary-row-b-diff').one('td');
                container.set('innerHTML', value);
             }
-        },
-
-        /**
-         * The HTML code for the diff.
-         *
-         * @attribute diff
-         */
-        diff: {
-            getter: function() {
-                if (this.get('pending')) {
-                    return '';
-                }
-                else {
-                    return this.get(
-                        'srcNode').one('.diff-content').get('innerHTML');
-                }
-            },
-            setter: function(value) {
-               this._setup_diff_container();
-               this.get(
-                   'srcNode').one('.diff-content').set('innerHTML', value);
-            }
         }
     }
 
@@ -214,7 +192,7 @@
                 .append(Y.Node.create('<h2 />')
                     .set("text", "Preview Diff "))
                 .append(Y.Node.create('<div />')
-                    .addClass("diff-content"))
+                    .addClass("diff-navigator"))
                 .append(Y.Node.create('<div />')
                     .addClass("diff-content"));
             this.get('srcNode').append(review_diff);
@@ -222,14 +200,12 @@
     },
 
     /*
-     * Update the page with the last version of the diff and update the
-     * stats.
+     * Update the page diff stats.
      *
      * @method update
      */
     update: function() {
         this.update_stats();
-        this.update_diff();
     },
 
     /*
@@ -248,35 +224,10 @@
                         '#proposal-summary a.diff-link', '#review-diff');
                     var node = self.get('summary_node');
                     Y.lp.anim.green_flash({node: node}).run();
-                },
-                failure: function() {
-                    var node = self.get('summary_node');
-                    Y.lp.anim.red_flash({node: node}).run();
-                }
-            }
-        };
-        var mp_uri = LP.cache.context.web_link;
-        this.get('lp_client').get(mp_uri + "/++diff-stats", config);
-     },
-
-
-    /*
-     * Update the diff content with the last version.
-     *
-     * @method update_diff
-     */
-    update_diff: function() {
-        var self = this;
-        var config = {
-            on: {
-                success: function(diff) {
-                    self.set('diff', diff);
-                    var node = self.get('srcNode').one('.diff-content');
-                    Y.lp.anim.green_flash({node: node}).run();
                     self.fire(self.NAME + '.updated');
                 },
                 failure: function() {
-                    var node = self.get('srcNode').one('.diff-content');
+                    var node = self.get('summary_node');
                     Y.lp.anim.red_flash({node: node}).run();
                 },
                 start: function() {
@@ -288,7 +239,7 @@
             }
         };
         var mp_uri = LP.cache.context.web_link;
-        this.get('lp_client').get(mp_uri + "/++diff", config);
+        this.get('lp_client').get(mp_uri + "/++diff-stats", config);
     }
 
 });

=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js	2014-02-28 03:32:53 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js	2014-03-31 23:30:31 +0000
@@ -8,20 +8,15 @@
     tests.suite = new Y.Test.Suite('branchmergeproposal.inlinecomments Tests');
 
     tests.suite.add(new Y.Test.Case({
-        name: 'code.branchmergeproposal.inlinecomments_tests',
+        name: 'code.branchmergeproposal.inlinecomments_comments_tests',
 
         setUp: function () {
             // Loads testing values into LP.cache.
             LP.cache.context = {
-                web_link: "https://launchpad.dev/~foo/bar/foobr/+merge/1",
                 self_link: "https://code.launchpad.dev/api/devel/~foo/bar/foobr/+merge/1",
-                preview_diff_link: "https://code.launchpad.dev/api/devel/~foo/bar/foobr/+merge/1/preview_diff",
-                preview_diffs_collection_link: "https://code.launchpad.dev/api/devel/~foo/bar/foobr/+merge/1/preview_diffs"
             };
 
             LP.cache.inline_diff_comments = true;
-            module.current_previewdiff_id = 1;
-            module.inlinecomments = {};
         },
 
         tearDown: function () {},
@@ -47,6 +42,7 @@
                 {io_provider: mockio});
 
             // Populate the page.
+            module.current_previewdiff_id = 1;
             module.populate_existing_comments();
 
             // LP was hit twice for fetching published and draft inline
@@ -176,8 +172,27 @@
 
             Y.Assert.isTrue(called);
             Y.Assert.areEqual(1, module.current_previewdiff_id);
+        }
+
+    }));
+
+    tests.suite.add(new Y.Test.Case({
+        name: 'code.branchmergeproposal.inlinecomments_diffnav_tests',
+
+        setUp: function () {
+            // Loads testing values into LP.cache.
+            LP.cache.context = {
+                web_link: "https://launchpad.dev/~foo/bar/foobr/+merge/1",
+                self_link: "https://code.launchpad.dev/api/devel/~foo/bar/foobr/+merge/1",
+                preview_diff_link: "https://code.launchpad.dev/api/devel/~foo/bar/foobr/+merge/1/preview_diff",
+                preview_diffs_collection_link: "https://code.launchpad.dev/api/devel/~foo/bar/foobr/+merge/1/preview_diffs"
+            };
+
+            LP.cache.inline_diff_comments = true;
         },
 
+        tearDown: function () {},
+
         test_diff_nav_feature_flag_disabled: function() {
             // Disable feature flag.
             LP.cache.inline_diff_comments = false;
@@ -194,8 +209,15 @@
             (new module.DiffNav(
                  {srcNode: Y.one('#review-diff'), lp_client: lp_client}
             )).render();
-            // Nothing actually happens.
-            Y.Assert.areEqual(0, mockio.requests.length);
+            // The lastest diff is retrieved.
+            Y.Assert.areEqual(1, mockio.requests.length);
+            Y.Assert.areSame(
+                "/~foo/bar/foobr/+merge/1/++diff",
+                mockio.last_request.url);
+            mockio.success({
+                responseText: Y.one('.diff-content').get('innerHTML'),
+                responseHeaders: {'Content-Type': 'text/html'}});
+	    // But no inline_comments are set up.
             Y.Assert.isNull(called_diff_id);
         },
 
@@ -235,6 +257,14 @@
             mockio.success({
                 responseText: Y.JSON.stringify(preview_diffs),
                 responseHeaders: {'Content-Type': 'application/json'}});
+            // The selected preview_diff content is retrieved.
+            Y.Assert.areEqual(2, mockio.requests.length);
+            Y.Assert.areSame(
+                "/~foo/bar/foobr/+merge/1/+preview-diff/101/+diff",
+                mockio.last_request.url);
+            mockio.success({
+                responseText: Y.one('.diff-content').get('innerHTML'),
+                responseHeaders: {'Content-Type': 'text/html'}});
             // The local (fake) setup_inline_comments function
             // was called with the selected diff_id value.
             Y.Assert.areEqual(101, called_diff_id);
@@ -252,7 +282,7 @@
             Y.one('select').set('value', 202);
             Y.one('select').simulate('change');
             // diff content was updated with the selected diff_id content.
-            Y.Assert.areEqual(2, mockio.requests.length);
+            Y.Assert.areEqual(3, mockio.requests.length);
             Y.Assert.areSame(
                 "/~foo/bar/foobr/+merge/1/+preview-diff/202/+diff",
                 mockio.last_request.url);
@@ -306,6 +336,21 @@
             Y.Assert.areEqual(
                 'Failed to fetch available diffs.',
                 Y.one('.diff-navigator').get('text'));
+        },
+
+        test_status_indicator: function() {
+            var diff_nav = new module.DiffNav(
+                {srcNode: Y.one('#review-diff')});
+            // Indicating progress with the spinner image.
+            diff_nav.set_status_updating();
+            Y.Assert.areEqual(
+                '/@@/spinner',
+                Y.one('h2').one('img').getAttribute('src'));
+            // Remove the spinner when the work is done.
+            diff_nav.cleanup_status();
+            Y.Assert.areEqual(
+                'Preview Diff',
+                Y.one('h2').get('innerHTML'));
         }
 
     }));

=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.updater.js'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.updater.js	2012-10-26 10:00:20 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.updater.js	2014-03-31 23:30:31 +0000
@@ -43,9 +43,7 @@
 
     test_default_values: function() {
         Y.Assert.isTrue(this.updater.get('pending'));
-        Y.Assert.areEqual(
-            '',
-            this.updater.get('diff'));
+        Y.Assert.isNull(this.updater.get('diff_stats'));
     },
 
     test__setup_diff_container: function() {
@@ -104,38 +102,6 @@
             Y.one('h2').get('innerHTML'));
     },
 
-    test_get_diff: function() {
-        this.updater._setup_diff_container();
-        Y.one('.diff-content').set(
-            'innerHTML', 'this is a <span>diff</span>');
-        Y.Assert.areEqual(
-            'this is a <span>diff</span>',
-            this.updater.get('diff'));
-    },
-
-    test_set_diff: function() {
-        this.updater.set('diff', 'this is a <span>diff</span>');
-        Y.Assert.areEqual(
-            'this is a <span>diff</span>',
-            Y.one('.diff-content').get('innerHTML'));
-    },
-
-    test_update_diff_success: function() {
-        var mockio = new Y.lp.testing.mockio.MockIo();
-        this.updater.get('lp_client').io_provider = mockio;
-        Y.Assert.areEqual(
-            '',
-            this.updater.get('diff'));
-        this.updater.update_diff();
-        mockio.success({
-            responseText: 'New <span>diff</span>',
-            responseHeaders: {'Content-Type': 'text/html'}});
-
-        Y.Assert.areEqual(
-            'New <span>diff</span>',
-            this.updater.get('diff'));
-    },
-
     test_update_stats_success: function() {
         var mockio = new Y.lp.testing.mockio.MockIo();
         this.updater.get('lp_client').io_provider = mockio;
@@ -150,61 +116,22 @@
             this.updater.get('diff_stats'));
     },
 
-    test_update_diff_fires_event: function() {
+    test_update_fires_event: function() {
         var fired = false;
         var mockio = new Y.lp.testing.mockio.MockIo();
         this.updater.get('lp_client').io_provider = mockio;
         this.updater.on(this.updater.NAME + '.updated', function() {
             fired = true;
         });
-        this.updater.update_diff();
+        this.updater.update();
         mockio.success({
-            responseText: 'New <span>diff</span>',
+            responseText: '13 lines (+4/-0) 1 file modified',
             responseHeaders: {'Content-Type': 'text/html'}});
-
         Y.Assert.isTrue(fired);
     }
 
 }));
 
-/*
- * Tests for when the updater is built on top of an existing diff.
- *
- */
-var current_mp = Y.one('#current-mp').getContent();
-
-tests.suite.add(new Y.Test.Case({
-
-    name: 'branchmergeproposal-updater-refresh-tests',
-
-    setUp: function() {
-        Y.one("#placeholder")
-            .empty()
-            .append(Y.Node.create(current_mp));
-        var diff_area = Y.one('#diff-area');
-        this.updater = new UpdaterWidget({srcNode: diff_area});
-    },
-
-    tearDown: function() {
-        this.updater.destroy();
-    },
-
-    test_default_values: function() {
-        Y.Assert.isFalse(this.updater.get('pending'));
-        Y.Assert.areEqual(
-            'Example diff',
-            this.updater.get('diff'));
-    },
-
-    test_get_diff: function() {
-        Y.one('.diff-content').set(
-            'innerHTML', 'this is a <span>diff</span>');
-        Y.Assert.areEqual(
-            'this is a <span>diff</span>',
-            this.updater.get('diff'));
-    }
-
-}));
 
 tests.suite.add(new Y.Test.Case({