Merge lp:~adeuring/launchpad/bug-903359 into lp:launchpad

[Abel Deuring]

[r=deryck][bug=903359] configure ascending or descending default sort order for each OrderByBar sort option separately.

fault sort direction for each of the
sort options of a OrderByBar separately (bug 903359).

The implementation is simple: The constructor of the sort widget
has the parameter sort_keys, which was a sequences uf tuples
(name, title); I extended this to the tuple (name, title, direction).

The new field "direction" must have either the value "asc" or the value
"desc". (I considered to use a bool flag instead, but this would have
required more changes to the current implementation of OrderByBar,
which already uses these string -- and I wanted to finish this branch
today...)

I added the method OrderByWidget._setSortOrder() to remove duplicated
code in OrderByWidget.updateSortArrows() and OrderByWidget.updateSortArrows().

And I updated the constant SORT_KEYS in lp.bugs.browser.bugtask, which
the sort buttons to include the sort order.

Some choices of "asc" or "desc" in SORT_KEYS are candidates for
bikeshedding, especially: Should the default status sor direction
really be "asc", i.e. should the status "new" come first?

I don't want to discuss this here -- changing the default order for
each sort option requires only a one line change...

tests:

./bin/test app -vvt test_orderby_widget
./bin/test bugs -vvt lp.bugs.browser.tests.test_bugtask

no lint

[Deryck Hodge (deryck)]

review: Approve

=== modified file 'lib/lp/app/javascript/ordering/ordering.js'
--- lib/lp/app/javascript/ordering/ordering.js	2011-12-16 03:40:04 +0000
+++ lib/lp/app/javascript/ordering/ordering.js	2011-12-19 14:16:28 +0000
@@ -38,15 +38,15 @@
          */
         sort_keys: {
             value: [
-                ['bugnumber', 'Bug number'],
-                ['bugtitle', 'Bug title'],
-                ['status', 'Status'],
-                ['importance', 'Importance'],
-                ['bug-heat-icons', 'Bug heat'],
-                ['package', 'Package name'],
-                ['milestone', 'Milestone'],
-                ['assignee', 'Assignee'],
-                ['bug-age', 'Bug age']
+                ['bugnumber', 'Bug number', 'asc'],
+                ['bugtitle', 'Bug title', 'asc'],
+                ['status', 'Status', 'asc'],
+                ['importance', 'Importance', 'desc'],
+                ['bug-heat-icons', 'Bug heat', 'desc'],
+                ['package', 'Package name', 'asc'],
+                ['milestone', 'Milestone', 'asc'],
+                ['assignee', 'Assignee', 'asc'],
+                ['bug-age', 'Bug age', 'desc']
             ]
         },
 
@@ -209,6 +209,20 @@
             }
         },
 
+        _setSortOrder: function(value, arrow_span) {
+            if (value === 'asc') {
+                arrow_span.setContent(this.constructor.ASCENDING_ARROW);
+                arrow_span.addClass('asc');
+                arrow_span.removeClass('desc');
+                this.set('sort_order', 'asc');
+            } else {
+                arrow_span.setContent(this.constructor.DESCENDING_ARROW);
+                arrow_span.addClass('desc');
+                arrow_span.removeClass('asc');
+                this.set('sort_order', 'desc');
+            }
+        },
+
         /**
          * Method used to update the arrows used in the display.
          * This will also update the widget's attributes to match
@@ -229,15 +243,9 @@
                 // Handle the case where the button clicked is the current
                 // active sort order.  We change sort directions for it.
                 if (arrow_span.hasClass('desc')) {
-                    arrow_span.setContent(this.constructor.ASCENDING_ARROW);
-                    arrow_span.addClass('asc');
-                    arrow_span.removeClass('desc');
-                    this.set('sort_order', 'asc');
+                    this._setSortOrder('asc', arrow_span);
                 } else {
-                    arrow_span.setContent(this.constructor.DESCENDING_ARROW);
-                    arrow_span.addClass('desc');
-                    arrow_span.removeClass('asc');
-                    this.set('sort_order', 'desc');
+                    this._setSortOrder('desc', arrow_span);
                 }
             } else {
                 // We have a different sort order clicked and need to
@@ -251,9 +259,13 @@
                 var pre_click_sort_order = this.get('sort_order');
                 old_arrow_span.removeClass(pre_click_sort_order);
                 // Update current li span arrow and set new sort order.
-                arrow_span.addClass('asc');
-                this.set('sort_order', 'asc');
-                arrow_span.setContent(this.constructor.ASCENDING_ARROW);
+                var sort_order;
+                Y.each(this.get('sort_keys'), function(key_data) {
+                    if (key_data[0] === clicked_node_sort_key) {
+                        sort_order = key_data[2];
+                    }
+                });
+                this._setSortOrder(sort_order, arrow_span);
             }
         },
 
@@ -335,15 +347,8 @@
                 li_node = Y.Node.create(li_html);
                 if (this.get('active') === id) {
                     li_node.addClass('active-sort');
-                    li_node.one('span').addClass(this.get('sort_order'));
                     sort_order = this.get('sort_order');
-                    if (sort_order === 'asc') {
-                        li_node.one('span').setContent(
-                            this.constructor.ASCENDING_ARROW);
-                    } else if (sort_order === 'desc') {
-                        li_node.one('span').setContent(
-                            this.constructor.DESCENDING_ARROW);
-                    }
+                    this._setSortOrder(sort_order, li_node.one('span'));
                 }
                 orderby_ul.appendChild(li_node);
                 li_nodes.push(li_node);

=== modified file 'lib/lp/app/javascript/ordering/tests/test_orderby_widget.js'
--- lib/lp/app/javascript/ordering/tests/test_orderby_widget.js	2011-12-08 13:23:00 +0000
+++ lib/lp/app/javascript/ordering/tests/test_orderby_widget.js	2011-12-19 14:16:28 +0000
@@ -83,9 +83,9 @@
     test_user_supplied_sort_keys: function() {
         // Call sites can supply their own sort keys to a widget.
         var user_supplied_sort_keys = [
-            ['foo', 'Foo item'],
-            ['bar', 'Bar item'],
-            ['baz', 'Baz item']
+            ['foo', 'Foo item', 'asc'],
+            ['bar', 'Bar item', 'asc'],
+            ['baz', 'Baz item', 'asc']
         ];
         this.orderby = new Y.lp.ordering.OrderByBar({
             sort_keys: user_supplied_sort_keys});
@@ -100,8 +100,8 @@
         // created from sort keys, and the name should be used as
         // a button display name in HTML.
         var test_sort_keys = [
-            ['foo', 'Foo item'],
-            ['bar', 'Bar item']
+            ['foo', 'Foo item', 'asc'],
+            ['bar', 'Bar item', 'asc']
         ];
         this.makeSrcNode('test-div');
         this.orderby = new Y.lp.ordering.OrderByBar({
@@ -187,8 +187,8 @@
         // This should fail because we do not allow
         // a "active" value not found in sort_keys.
         var test_sort_keys = [
-            ['foo', 'Foo item'],
-            ['bar', 'Bar item']
+            ['foo', 'Foo item', 'asc'],
+            ['bar', 'Bar item', 'asc']
         ];
         this.orderby = new Y.lp.ordering.OrderByBar({
             sort_keys: test_sort_keys,
@@ -212,8 +212,8 @@
         // happen.
         this.makeSrcNode('test-div');
         var test_sort_keys = [
-            ['foo', 'Foo item'],
-            ['bar', 'Bar item']
+            ['foo', 'Foo item', 'asc'],
+            ['bar', 'Bar item', 'asc']
         ];
         this.orderby = new Y.lp.ordering.OrderByBar({
             srcNode: Y.one('#test-div'),
@@ -223,8 +223,10 @@
         });
         this.orderby.render();
         var foo_node = Y.one('#sort-foo');
-        var expected_starting_text = '<span class="sprite order-ascending"></span>';
-        var expected_ending_text = '<span class="sprite order-descending"></span>';
+        var expected_starting_text =
+            '<span class="sprite order-ascending"></span>';
+        var expected_ending_text =
+            '<span class="sprite order-descending"></span>';
         Assert.areEqual(
             expected_starting_text, foo_node.one('span').get('innerHTML'));
         Assert.isTrue(foo_node.one('span').hasClass('asc'));
@@ -240,8 +242,8 @@
         // change should happen.
         this.makeSrcNode('test-div');
         var test_sort_keys = [
-            ['foo', 'Foo item'],
-            ['bar', 'Bar item']
+            ['foo', 'Foo item', 'asc'],
+            ['bar', 'Bar item', 'asc']
         ];
         this.orderby = new Y.lp.ordering.OrderByBar({
             srcNode: Y.one('#test-div'),
@@ -261,6 +263,32 @@
         Assert.isFalse(Y.one('#sort-foo').one('span').hasClass('desc'));
     },
 
+    test_click_different_sort_arrows_change_default_order: function() {
+        // A newly active sort button has the order as specified by
+        // the constructor parameter sort_keys.
+        this.makeSrcNode('test-div');
+        var test_sort_keys = [
+            ['foo', 'Foo item', 'asc'],
+            ['bar', 'Bar item', 'desc'],
+            ['baz', 'Baz item', 'asc']
+        ];
+        this.orderby = new Y.lp.ordering.OrderByBar({
+            srcNode: Y.one('#test-div'),
+            sort_keys: test_sort_keys,
+            active: 'foo',
+            sort_order: 'asc'
+        });
+        this.orderby.render();
+        var bar_node = Y.one('#sort-bar');
+        bar_node.simulate('click');
+        Assert.isTrue(bar_node.one('span').hasClass('desc'));
+        Assert.areEqual('desc', this.orderby.get('sort_order'));
+        var baz_node = Y.one('#sort-baz');
+        baz_node.simulate('click');
+        Assert.isTrue(baz_node.one('span').hasClass('asc'));
+        Assert.areEqual('asc', this.orderby.get('sort_order'));
+    },
+
     test_sort_clause_default: function() {
         // sort_clause defaults to "importance".
         this.orderby = new Y.lp.ordering.OrderByBar();
@@ -275,8 +303,8 @@
         // a URL.
         this.makeSrcNode('test-div');
         var test_sort_keys = [
-            ['foo', 'Foo item'],
-            ['bar', 'Bar item']
+            ['foo', 'Foo item', 'asc'],
+            ['bar', 'Bar item', 'asc']
         ];
         this.orderby = new Y.lp.ordering.OrderByBar({
             srcNode: Y.one('#test-div'),

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-12-18 03:35:49 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-12-19 14:16:28 +0000
@@ -2494,27 +2494,27 @@
 # All sort orders supported by BugTaskSet.search() and a title for
 # them.
 SORT_KEYS = [
-    ('importance', 'Importance'),
-    ('status', 'Status'),
-    ('id', 'Bug number'),
-    ('title', 'Bug title'),
-    ('targetname', 'Package/Project/Series name'),
-    ('milestone_name', 'Milestone'),
-    ('date_last_updated', 'Date bug last updated'),
-    ('assignee', 'Assignee'),
-    ('reporter', 'Reporter'),
-    ('datecreated', 'Bug age'),
-    ('tag', 'Bug Tags'),
-    ('heat', 'Bug heat'),
-    ('date_closed', 'Date bug closed'),
-    ('dateassigned', 'Date when the bug task was assigned'),
-    ('number_of_duplicates', 'Number of duplicates'),
-    ('latest_patch_uploaded', 'Date latest patch uploaded'),
-    ('message_count', 'Number of comments'),
-    ('milestone', 'Milestone ID'),
-    ('specification', 'Linked blueprint'),
-    ('task', 'Bug task ID'),
-    ('users_affected_count', 'Number of affected users'),
+    ('importance', 'Importance', 'desc'),
+    ('status', 'Status', 'asc'),
+    ('id', 'Bug number', 'desc'),
+    ('title', 'Bug title', 'asc'),
+    ('targetname', 'Package/Project/Series name', 'asc'),
+    ('milestone_name', 'Milestone', 'asc'),
+    ('date_last_updated', 'Date bug last updated', 'desc'),
+    ('assignee', 'Assignee', 'asc'),
+    ('reporter', 'Reporter', 'asc'),
+    ('datecreated', 'Bug age', 'desc'),
+    ('tag', 'Bug Tags', 'asc'),
+    ('heat', 'Bug heat', 'desc'),
+    ('date_closed', 'Date bug closed', 'desc'),
+    ('dateassigned', 'Date when the bug task was assigned', 'desc'),
+    ('number_of_duplicates', 'Number of duplicates', 'desc'),
+    ('latest_patch_uploaded', 'Date latest patch uploaded', 'desc'),
+    ('message_count', 'Number of comments', 'desc'),
+    ('milestone', 'Milestone ID', 'desc'),
+    ('specification', 'Linked blueprint', 'asc'),
+    ('task', 'Bug task ID', 'desc'),
+    ('users_affected_count', 'Number of affected users', 'desc'),
     ]
 
 

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2011-12-15 18:55:46 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2011-12-19 14:16:28 +0000
@@ -2263,6 +2263,21 @@
             "keys present in JSON cache but not defined: %r"
             % (valid_keys - json_sort_keys, json_sort_keys - valid_keys))
 
+    def test_sort_keys_in_json_cache_data(self):
+        # The entry 'sort_keys' in the JSON cache of a search listing
+        # view is a sequence of 3-tuples (name, title, order), where
+        # order is one of the string 'asc' or 'desc'.
+        with dynamic_listings():
+            view = self.makeView()
+        cache = IJSONRequestCache(view.request)
+        json_sort_keys = cache.objects['sort_keys']
+        for key in json_sort_keys:
+            self.assertEqual(
+                3, len(key), 'Invalid key length: %r' % (key, ))
+            self.assertTrue(
+                key[2] in ('asc', 'desc'),
+                'Invalid order value: %r' % (key, ))
+
 
 class TestBugTaskExpirableListingView(BrowserTestCase):
     """Test BugTaskExpirableListingView."""

=== modified file 'lib/lp/bugs/javascript/buglisting_utils.js'
--- lib/lp/bugs/javascript/buglisting_utils.js	2011-12-15 15:11:50 +0000
+++ lib/lp/bugs/javascript/buglisting_utils.js	2011-12-19 14:16:28 +0000
@@ -344,7 +344,6 @@
         Y.each(orderbybar.always_display, function(sort_key) {
             orderby_visibility[sort_key] = true;
         });
-        Y.log(data_visibility);
         orderbybar.updateVisibility(orderby_visibility);
     }