launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05980
[Merge] lp:~adeuring/launchpad/bug-903359 into lp:launchpad
Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-903359 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #903359 in Launchpad itself: "Default ordering in bug listings is typically unhelpful"
https://bugs.launchpad.net/launchpad/+bug/903359
For more details, see:
https://code.launchpad.net/~adeuring/launchpad/bug-903359/+merge/86244
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
--
https://code.launchpad.net/~adeuring/launchpad/bug-903359/+merge/86244
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/bug-903359 into lp:launchpad.
=== 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);
}
