Merge lp:~abentley/launchpad/display-cleanup into lp:launchpad

[Aaron Bentley]

[r=adeuring][bug=882795] Polish ajax bug listing navigation.

= Summary =
Fix bug #882795: ajax bug listing navigation needs polish

== Proposed fix ==
Replace all navigation nodes with hyperlink nodes.
Mark first/previous disabled on first batch.
Mark last/next disabled on last batch.
Update batch info (1 -> 5 of 10) on navigation.

== Pre-implementation notes ==
None

== Implementation details ==
None

== Tests ==
bin/test -t test_buglisting.html

== Demo and Q/A ==
- Go to a listing page. first & previous should be disabled.
- Click last. next & last should be disabled, and first & previous should be green. The info about the batch should be updated.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/javascript/buglisting.js
  lib/lp/bugs/templates/bugs-listing-table.pt
  lib/lp/bugs/templates/buglisting-default.pt
  lib/lp/bugs/browser/tests/test_bugtask.py
  lib/lp/bugs/browser/bugtask.py
  lib/lp/bugs/templates/bugs-listing-table-without-navlinks.pt
  lib/lp/bugs/javascript/tests/test_buglisting.js

[Abel Deuring (adeuring)]

(16:40:10) adeuring: abentley: I think "this.current_batch.mustache_model.bugtasks.length + 1" in line 113 is wrong. Shouldn't that be "... -1"?
(16:40:35) adeuring: (line 113 of the diff)
(16:41:09) adeuring: abentley: no, that should be "+0"...
(16:41:13) abentley: adeuring: So, the +1 is trying to compensate for start being 0-indexed.
(16:41:47) adeuring: abentley: right. But for 5 results and start = 0, we want to show "results 1-> 5"
(16:41:56) adeuring: while we get "1 to 6"
(16:42:08) adeuring: abentley: just click on any of the links ;)
(16:42:15) abentley: Hmm, so why does it look right in use?
(16:42:36) adeuring: and compare the number in "n->m of k" with the real number of results ;)
(16:42:49) abentley: adeuring: that's an estimate, though.
(16:43:26) adeuring: abentley: well, k is an estimate (at least for StormRangeFactory), but we known the number of results in the batch precisely
(16:44:07) abentley: adeuring: You're right, I'll fix it.
(16:44:15) adeuring: abentley: cool, thanks
(16:46:07) adeuring: abentley: another quirk: For the first batch, the "first" and "previous" links are gray, but still active. "unlinking" them is something for antother branch, I assume?
(16:46:30) adeuring: (same for the last batch and the last/next links)
(16:47:39) abentley: adeuring: I would prefer if the CSS was fixed so that inactive links don't show as links.
(16:48:33) adeuring: abentley: right, makes sense. So, r=me, and file a bug about the links?
(16:49:02) abentley: adeuring: Thanks. I'll file a bug.

review: Approve

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-10-28 15:09:16 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-10-28 15:09:17 +0000
@@ -2495,6 +2495,7 @@
             cache.objects['next'] = _getBatchInfo(next_batch)
             prev_batch = batch_navigator.batch.prevBatch()
             cache.objects['prev'] = _getBatchInfo(prev_batch)
+            cache.objects['total'] = batch_navigator.batch.total()
             cache.objects['order_by'] = ','.join(
                 get_sortorder_from_request(self.request))
             cache.objects['forwards'] = batch_navigator.batch.range_forwards
 
=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2011-10-28 15:09:16 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2011-10-28 15:09:17 +0000
@@ -1424,6 +1424,7 @@
         self.assertIs(None, cache.objects['memo'])
         self.assertEqual(0, cache.objects['start'])
         self.assertTrue(cache.objects['forwards'])
+        self.assertEqual(1, cache.objects['total'])
 
     def test_cache_has_all_batch_vars_specified(self):
         """Cache has all the needed variables.
 
=== modified file 'lib/lp/bugs/javascript/buglisting.js'
--- lib/lp/bugs/javascript/buglisting.js	2011-10-28 15:09:16 +0000
+++ lib/lp/bugs/javascript/buglisting.js	2011-10-28 15:09:17 +0000
@@ -18,11 +18,13 @@
  * cache is the JSONRequestCache for the batch.
  * template is the template to use for rendering batches.
  * target is a YUI node to update when rendering batches.
+ * navigation_indices is a YUI NodeList of nodes to update with the current
+ * batch info.
  * io_provider is something providing the Y.io interface, typically used for
  * testing.  Defaults to Y.io.
  */
 namespace.ListingNavigator = function(current_url, cache, template, target,
-                                      io_provider) {
+                                      navigation_indices, io_provider) {
     var lp_client = new Y.lp.client.Launchpad();
     this.search_params = namespace.get_query(current_url);
     delete this.search_params.start;
@@ -34,6 +36,14 @@
     this.template = template;
     this.target = target;
     this.batches = {};
+    this.backwards_navigation = new Y.NodeList([]);
+    this.forwards_navigation = new Y.NodeList([]);
+    if (!Y.Lang.isValue(navigation_indices)){
+        navigation_indices = new Y.NodeList([]);
+    }
+    this.navigation_indices = navigation_indices;
+    this.batch_info_template = '<strong>{{start}}</strong> &rarr; ' +
+        '<strong>{{end}}</strong> of {{total}} results';
 };
 
 
@@ -53,18 +63,38 @@
     nodes.addClass('js-action');
 };
 
+/**
+ * Rewrite all nodes with navigation classes so that they are hyperlinks.
+ * Content is retained.
+ */
+namespace.linkify_navigation = function(){
+    Y.each(['previous', 'next', 'first', 'last'], function(class_name){
+        Y.all('.' + class_name).each(function(node){
+            new_node = Y.Node.create('<a href="#"></a>');
+            new_node.addClass(class_name);
+            new_node.setContent(node.getContent());
+            node.replace(new_node);
+        });
+    });
+};
 
 /**
  * Factory to return a ListingNavigator for the given page.
  */
 namespace.ListingNavigator.from_page = function(){
     var target = Y.one('#client-listing');
+    var navigation_indices = Y.all('.batch-navigation-index');
     var navigator = new namespace.ListingNavigator(
-        window.location, LP.cache, LP.mustache_listings, target);
+        window.location, LP.cache, LP.mustache_listings, target,
+        navigation_indices);
+    namespace.linkify_navigation();
+    navigator.backwards_navigation = Y.all('.first,.previous');
+    navigator.forwards_navigation = Y.all('.last,.next');
     navigator.clickAction('.first', navigator.first_batch);
     navigator.clickAction('.next', navigator.next_batch);
     navigator.clickAction('.previous', navigator.prev_batch);
     navigator.clickAction('.last', navigator.last_batch);
+    navigator.render_navigation();
     return navigator;
 };
 
@@ -77,13 +107,31 @@
  *
  * The template is always LP.mustache_listings.
  */
-namespace.ListingNavigator.prototype.rendertable = function(){
+namespace.ListingNavigator.prototype.render = function(){
     if (! Y.Lang.isValue(this.target)){
         return;
     }
     var model = this.current_batch.mustache_model;
-    var txt = Mustache.to_html(this.template, model);
-    this.target.set('innerHTML', txt);
+    var batch_info = Mustache.to_html(this.batch_info_template, {
+        start: this.current_batch.start + 1,
+        end: this.current_batch.start +
+            this.current_batch.mustache_model.bugtasks.length,
+        total: this.current_batch.total
+    });
+    this.target.setContent(Mustache.to_html(this.template, model));
+    this.navigation_indices.setContent(batch_info);
+    this.render_navigation();
+};
+
+
+/**
+ * Enable/disable navigation links as appropriate.
+ */
+namespace.ListingNavigator.prototype.render_navigation = function(){
+    this.backwards_navigation.toggleClass(
+        'inactive', this.current_batch.prev === null);
+    this.forwards_navigation.toggleClass(
+        'inactive', this.current_batch.next === null);
 };
 
 
@@ -105,7 +153,7 @@
     var key = namespace.ListingNavigator.get_batch_key(model);
     this.batches[key] = model;
     this.current_batch = model;
-    this.rendertable();
+    this.render();
 };
 
 
@@ -139,7 +187,7 @@
     var cached_batch = this.batches[key];
     if (Y.Lang.isValue(cached_batch)){
         this.current_batch = cached_batch;
-        this.rendertable();
+        this.render();
     }
     else {
         this.load_model(config);
 
=== modified file 'lib/lp/bugs/javascript/tests/test_buglisting.js'
--- lib/lp/bugs/javascript/tests/test_buglisting.js	2011-10-28 15:09:16 +0000
+++ lib/lp/bugs/javascript/tests/test_buglisting.js	2011-10-28 15:09:17 +0000
@@ -21,26 +21,132 @@
 }));
 
 suite.add(new Y.Test.Case({
-    name: 'rendertable',
-
-    test_rendertable_no_client_listing: function() {
+    name: 'render',
+
+    tearDown: function(){
+        Y.one('#fixture').setContent('');
+    },
+
+    test_render_no_client_listing: function() {
         // Rendering should not error with no #client-listing.
         var navigator = new module.ListingNavigator();
-        navigator.rendertable();
+        navigator.render();
     },
-    test_rendertable: function() {
-        // Rendering should work with #client-listing supplied.
+    get_render_navigator: function() {
         var target = Y.Node.create('<div id="client-listing"></div>');
         var lp_cache = {
             mustache_model: {
-                foo: 'bar'
-            }
+                foo: 'bar',
+                bugtasks: ['a', 'b', 'c']
+            },
+            next: null,
+            prev: null,
+            start: 5,
+            total: 256
         };
         var template = "{{foo}}";
-        var navigator = new module.ListingNavigator(
+        var navigator =  new module.ListingNavigator(
             null, lp_cache, template, target);
-        navigator.rendertable();
-        Y.Assert.areEqual('bar', navigator.target.get('innerHTML'));
+        var index = Y.Node.create(
+            '<div><strong>3</strong> &rarr; <strong>4</strong>' +
+            ' of 512 results</div>');
+        navigator.navigation_indices.push(index);
+        navigator.backwards_navigation.push(Y.Node.create('<div></div>'));
+        navigator.forwards_navigation.push(Y.Node.create('<div></div>'));
+        return navigator;
+    },
+    test_render: function() {
+        // Rendering should work with #client-listing supplied.
+        var navigator = this.get_render_navigator();
+        navigator.render();
+        Y.Assert.areEqual('bar', navigator.target.getContent());
+    },
+    /**
+     * render_navigation should disable "previous" and "first" if there is
+     * no previous batch (i.e. we're at the beginning.)
+     */
+    test_render_navigation_disables_backwards_navigation_if_no_prev:
+    function() {
+        var navigator = this.get_render_navigator();
+        var action = navigator.backwards_navigation.item(0);
+        navigator.render_navigation();
+        Y.Assert.isTrue(action.hasClass('inactive'));
+    },
+    /**
+     * render_navigation should enable "previous" and "first" if there is
+     * a previous batch (i.e. we're not at the beginning.)
+     */
+    test_render_navigation_enables_backwards_navigation_if_prev:
+    function() {
+        var navigator = this.get_render_navigator();
+        var action = navigator.backwards_navigation.item(0);
+        action.addClass('inactive');
+        navigator.current_batch.prev = {
+            start: 1, memo: 'pi'
+        };
+        navigator.render_navigation();
+        Y.Assert.isFalse(action.hasClass('inactive'));
+    },
+    /**
+     * render_navigation should disable "next" and "last" if there is
+     * no next batch (i.e. we're at the end.)
+     */
+    test_render_navigation_disables_forwards_navigation_if_no_next:
+    function() {
+        var navigator = this.get_render_navigator();
+        var action = navigator.forwards_navigation.item(0);
+        navigator.render_navigation();
+        Y.Assert.isTrue(action.hasClass('inactive'));
+    },
+    /**
+     * render_navigation should enable "next" and "last" if there is a next
+     * batch (i.e. we're not at the end.)
+     */
+    test_render_navigation_enables_forwards_navigation_if_next: function() {
+        var navigator = this.get_render_navigator();
+        var action = navigator.forwards_navigation.item(0);
+        action.addClass('inactive');
+        navigator.current_batch.next = {
+            start: 1, memo: 'pi'
+        };
+        navigator.render_navigation();
+        Y.Assert.isFalse(action.hasClass('inactive'));
+    },
+    /**
+     * linkify_navigation should convert previous, next, first last into
+     * hyperlinks, while retaining the original content.
+     */
+    test_linkify_navigation: function(){
+        Y.one('#fixture').setContent(
+            '<span class="previous">PreVious</span>' +
+            '<span class="next">NeXt</span>' +
+            '<span class="first">FiRST</span>' +
+            '<span class="last">lAst</span>');
+        module.linkify_navigation();
+        function checkNav(selector, content){
+            var node = Y.one(selector);
+            Y.Assert.areEqual('a', node.get('tagName').toLowerCase());
+            Y.Assert.areEqual(content, node.getContent());
+            Y.Assert.areEqual('#', node.get('href').substr(-1, 1));
+        }
+        checkNav('.previous', 'PreVious');
+        checkNav('.next', 'NeXt');
+        checkNav('.first', 'FiRST');
+        checkNav('.last', 'lAst');
+    },
+    /**
+     * Render should update the navigation_indices with the result info.
+     */
+    test_render_navigation_indices: function(){
+        var navigator = this.get_render_navigator();
+        index = navigator.navigation_indices.item(0);
+        Y.Assert.areEqual(
+            '<strong>3</strong> \u2192 <strong>4</strong> of 512 results',
+            index.getContent());
+        navigator.render();
+        Y.Assert.areEqual(
+            '<strong>6</strong> \u2192 <strong>8</strong> of 256 results',
+            index.getContent());
     }
 }));
 
@@ -58,26 +164,29 @@
                 web_link: 'http://foo/bar'
             },
             mustache_model: {
-                foo: 'bar'
+                foo: 'bar',
+                bugtasks: []
             }
         };
         var target = Y.Node.create('<div id="client-listing"></div>');
         var navigator = new module.ListingNavigator(
             "http://yahoo.com?start=5&memo=6&direction=backwards",
             lp_cache, "<ol>" + "{{#item}}<li>{{name}}</li>{{/item}}</ol>",
-            target, mock_io);
+            target, null, mock_io);
         navigator.first_batch('intensity');
-        Y.Assert.areEqual('', navigator.target.get('innerHTML'));
+        Y.Assert.areEqual('', navigator.target.getContent());
         mock_io.last_request.successJSON({
             context: {
                 resource_type_link: 'http://foo_type',
                 web_link: 'http://foo/bar'
             },
             mustache_model:
-            {item: [
+            {
+                item: [
                 {name: 'first'},
-                {name: 'second'}
-            ]},
+                {name: 'second'}],
+                bugtasks: []
+            },
             order_by: 'intensity',
             start: 0,
             forwards: true,
@@ -91,7 +200,7 @@
         var navigator = this.get_intensity_listing();
         var mock_io = navigator.io_provider;
         Y.Assert.areEqual('<ol><li>first</li><li>second</li></ol>',
-            navigator.target.get('innerHTML'));
+            navigator.target.getContent());
         Y.Assert.areEqual('/bar/+bugs/++model++?orderby=intensity&start=0',
             mock_io.last_request.url);
     },
@@ -247,7 +356,8 @@
         order_by: 'foo',
         last_start: 23
     };
-    return new module.ListingNavigator(url, lp_cache, null, null, mock_io);
+    return new module.ListingNavigator(url, lp_cache, null, null, null,
+                                       mock_io);
 };
 
 suite.add(new Y.Test.Case({