launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05930
[Merge] lp:~adeuring/launchpad/bug-903852 into lp:launchpad
Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-903852 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #903852 in Launchpad itself: "sort widget on bug listings pages breaks when a sort order is selected via the URL that has no related displayed data"
https://bugs.launchpad.net/launchpad/+bug/903852
For more details, see:
https://code.launchpad.net/~adeuring/launchpad/bug-903852/+merge/85651
The new JS widgets to change the sort order and to select the displyed data
break at present when a bug listing page is accessed with an "unexpected"
sort order. This can happen for example when a user selects "sort by
numbers of users affected" in the "Advanced search" page. An example URL
for the bug:
https://bugs.launchpad.net/launchpad/+bugs?orderby=-users_affected_count
(make sure that the feature bugs.dynamic_bug_listings.enabled is enabled.)
The cause is simple: The JS code in
lp/bugs/templates/bugtask-macros-tableview.pt, macro activate_listing_js,
does not define sort buttons for all sort orders implemented by
BugTaskSet.search()
A related problem: Generally, we want to show only sort buttons for data
that is also displayed on the page. A strict implementation of this rule
would be confusing for users: If the data for current sort order is not
displayed, users would have no indication what the sort order is (well,
the< could have a look at the URL, but that's not obvious to all users,
and it can be cumbersome to find the "orderby" parameter in complex
queries).
Hence there is already an exemption to this rule: The button for the
current sort order is never hidden. But the button disappears when
the users selects a different sort order.
This branch adds another exemption: If the data for the sort order
selected during page load can never be displayed, the sort button will
always be displayed during the "Lifetime" of the page view.
Implementation details:
I moved the definition of the set of all sort buttons (parameter
sort_keys for Y.lp.ordering.OrderByBar()) to
lp.browser.bugtask; this data is added in an initialize() call to the
JSON cache.
This makes it possible to write a test that sort_keys (or more precisely,
the new constant lp.browser.bugtask.SORT_KEYS) has entries for all
sort options implemented by BugTaskSet.search()
(test_sort_keys_in_json_cache()).
This required another, mostly mechanical, change: The SQL expressions
for the ORDER BY clause were stored in BugTaskSet._ORDERBY_COLUMN,
which is not initialized during module load to avoid circular imports.
Instead, this dictionary was initilalized in the method
BugTaskSet.getOrderByColumn(). I replaced this method with the
cached property BugTaskSet.orderby_expression, so that
test_sort_keys_in_json_cache() can access all keys of this dictionary.
tests:
./bin/test bugs -vvt test_bugtask.*test_sort_keys_in_json_cache
./bin/test bugs -vvt test_buglisting_utils
lint:
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/bugs/browser/bugtask.py
lib/lp/bugs/browser/tests/test_bugtask.py
lib/lp/bugs/interfaces/bugtask.py
lib/lp/bugs/javascript/buglisting_utils.js
lib/lp/bugs/javascript/tests/test_buglisting_utils.js
lib/lp/bugs/model/bugtask.py
lib/lp/bugs/templates/bugtask-macros-tableview.pt
./lib/lp/bugs/templates/bugtask-macros-tableview.pt
674: not well-formed (invalid token)
make: *** [lint] Fehler 1
This is caused by the '<' in
for (var i = 0; i < sort_keys.length; i++)
This complaint could be avoided by
for (var i = 0; sort_keys.length > i; i++)
but at least for me the former variant is easier for "mental parsing".
--
https://code.launchpad.net/~adeuring/launchpad/bug-903852/+merge/85651
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/bug-903852 into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py 2011-12-14 03:19:49 +0000
+++ lib/lp/bugs/browser/bugtask.py 2011-12-14 12:44:18 +0000
@@ -2478,6 +2478,33 @@
return Link('+nominations', 'Review nominations', icon='bug')
+# All sort orders supported by BugTaskSet.search() and a title for
+# them.
+SORT_KEYS = [
+ ('id', 'Bug number'),
+ ('title', 'Bug title'),
+ ('importance', 'Importance'),
+ ('status', 'Status'),
+ ('heat', 'Bug heat'),
+ ('reporter', 'Reporter'),
+ ('assignee', 'Assignee'),
+ ('targetname', 'Package/Project/Series name'),
+ ('milestone_name', 'Milestone'),
+ ('datecreated', 'Bug age'),
+ ('date_last_updated', 'Date bug last updated'),
+ ('tag', 'Bug Tags'),
+ ('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'),
+ ]
+
+
class BugTaskSearchListingView(LaunchpadFormView, FeedsMixin, BugsInfoMixin):
"""View that renders a list of bugs for a given set of search criteria."""
@@ -2672,6 +2699,7 @@
last_batch = batch_navigator.batch.lastBatch()
cache.objects['last_start'] = last_batch.startNumber() - 1
cache.objects.update(_getBatchInfo(batch_navigator.batch))
+ cache.objects['sort_keys'] = SORT_KEYS
@property
def show_config_portlet(self):
@@ -2743,7 +2771,7 @@
orderby_col = orderby_col[1:]
try:
- bugset.getOrderByColumnDBName(orderby_col)
+ bugset.orderby_expression[orderby_col]
except KeyError:
raise UnexpectedFormData(
"Unknown sort column '%s'" % orderby_col)
=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py 2011-12-13 15:32:36 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py 2011-12-14 12:44:18 +0000
@@ -2246,6 +2246,22 @@
mustache_model['bugtasks'][0]['show_date_last_updated'] = True
self.assertIn('Last updated updated1', navigator.mustache)
+ def test_sort_keys_in_json_cache(self):
+ # The JSON cache of a search listing view provides a sequence
+ # that describes all sort orders implemented by
+ # BugTaskSet.search() and no sort orders that are not implemented.
+ with self.dynamic_listings():
+ view = self.makeView()
+ cache = IJSONRequestCache(view.request)
+ json_sort_keys = cache.objects['sort_keys']
+ json_sort_keys = set(key[0] for key in json_sort_keys)
+ valid_keys = set(getUtility(IBugTaskSet).orderby_expression.keys())
+ self.assertEqual(
+ valid_keys, json_sort_keys,
+ "Existing sort order values not available in JSON cache: %r; "
+ "keys present in JSON cache but not defined: %r"
+ % (valid_keys - json_sort_keys, json_sort_keys - valid_keys))
+
class TestBugTaskExpirableListingView(BrowserTestCase):
"""Test BugTaskExpirableListingView."""
=== modified file 'lib/lp/bugs/interfaces/bugtask.py'
--- lib/lp/bugs/interfaces/bugtask.py 2011-12-12 04:16:26 +0000
+++ lib/lp/bugs/interfaces/bugtask.py 2011-12-14 12:44:18 +0000
@@ -920,7 +920,7 @@
"""
def userHasBugSupervisorPrivileges(user):
- """Is the user a privledged one, allowed to changed details on a
+ """Is the user a privledged one, allowed to changed details on a
bug?
:return: A boolean.
@@ -1483,6 +1483,8 @@
class IBugTaskSet(Interface):
"""A utility to retrieving BugTasks."""
title = Attribute('Title')
+ orderby_expression = Attribute(
+ "The SQL expression for a sort key")
def get(task_id):
"""Retrieve a BugTask with the given id.
@@ -1639,12 +1641,6 @@
<user> is None, no private bugtasks will be returned.
"""
- def getOrderByColumnDBName(col_name):
- """Get the database name for col_name.
-
- If the col_name is unrecognized, a KeyError is raised.
- """
-
def getBugCountsForPackages(user, packages):
"""Return open bug counts for the list of packages.
=== modified file 'lib/lp/bugs/javascript/buglisting_utils.js'
--- lib/lp/bugs/javascript/buglisting_utils.js 2011-12-09 14:20:52 +0000
+++ lib/lp/bugs/javascript/buglisting_utils.js 2011-12-14 12:44:18 +0000
@@ -338,8 +338,12 @@
orderby_visibility[order_key] = data_visibility[data_key];
}
}
- // Never hide the button for the current sort order.
+ // Never hide the button for the current sort order...
orderby_visibility[orderbybar.get('active')] = true;
+ // ...and for the sort order that should always be displayed.
+ if (orderbybar.always_display !== undefined) {
+ orderby_visibility[orderbybar.always_display] = true;
+ }
orderbybar.updateVisibility(orderby_visibility);
}
=== modified file 'lib/lp/bugs/javascript/tests/test_buglisting_utils.js'
--- lib/lp/bugs/javascript/tests/test_buglisting_utils.js 2011-12-11 15:48:11 +0000
+++ lib/lp/bugs/javascript/tests/test_buglisting_utils.js 2011-12-14 12:44:18 +0000
@@ -436,43 +436,58 @@
Assert.isNull(Y.Cookie.get(this.cookie_name));
},
+ getDefaultDataVisibility: function() {
+ return {
+ show_title: true,
+ show_id: false,
+ show_importance: true,
+ show_status: true,
+ show_heat: true,
+ show_targetname: true,
+ show_datecreated: false,
+ show_date_last_updated: false,
+ show_assignee: false,
+ show_reporter: false,
+ show_milestone_name: false,
+ show_tag: true
+ };
+ },
+
+ SORT_KEYS: [
+ ['id', 'Bug number'],
+ ['title', 'Bug title'],
+ ['importance', 'Importance'],
+ ['status', 'Status'],
+ ['heat', 'Bug heat'],
+ ['reporter', 'Reporter'],
+ ['assignee', 'Assignee'],
+ ['targetname', 'Package/Project/Series name'],
+ ['milestone_name', 'Milestone'],
+ ['datecreated', 'Bug age'],
+ ['date_last_updated', 'Date bug last updated'],
+ ['tag', 'Bug Tags'],
+ ['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']
+ ],
+
test_update_sort_button_visibility: function() {
// update_sort_button_visibility() hides sort buttons for
// data that is not displayed and shown sort buttons for other
// data.
var orderby = new Y.lp.ordering.OrderByBar({
- sort_keys: [
- ['id', 'Bug number'],
- ['title', 'Bug title'],
- ['importance', 'Importance'],
- ['status', 'Status'],
- ['heat', 'Bug heat'],
- ['reporter', 'Reporter'],
- ['assignee', 'Assignee'],
- ['targetname', 'Package/Project/Series name'],
- ['milestone_name', 'Milestone'],
- ['datecreated', 'Bug age'],
- ['date_last_updated', 'Date bug last updated'],
- ['tag', 'Bug Tags']
- ],
+ sort_keys: this.SORT_KEYS,
active: 'importance',
sort_order: 'desc'
});
orderby.render();
- var data_visibility = {
- show_title: true,
- show_id: false,
- show_importance: true,
- show_status: true,
- show_heat: true,
- show_targetname: true,
- show_datecreated: false,
- show_date_last_updated: false,
- show_assignee: false,
- show_reporter: false,
- show_milestone_name: false,
- show_tag: true
- };
+ var data_visibility = this.getDefaultDataVisibility();
Y.lp.buglisting_utils.update_sort_button_visibility(
orderby, data_visibility);
Y.each(orderby.get('li_nodes'), function(li_node) {
@@ -483,16 +498,41 @@
Assert.isTrue(li_node._isHidden());
}
});
+ },
+ test_update_sort_button_visibility_current_sort_order: function() {
// The current sort order (importance for this test) is
// never hidden, even if the bug importance is not displayed.
+ var orderby = new Y.lp.ordering.OrderByBar({
+ sort_keys: this.SORT_KEYS,
+ active: 'importance',
+ sort_order: 'desc'
+ });
+ orderby.render();
+ var data_visibility = this.getDefaultDataVisibility();
data_visibility.show_importance = false;
Y.lp.buglisting_utils.update_sort_button_visibility(
orderby, data_visibility);
- importance_button = Y.one('#sort-importance');
+ var importance_button = Y.one('#sort-importance');
Assert.isFalse(importance_button._isHidden());
+ },
+
+ test_update_sort_button_visibility_extra_sort_order: function() {
+ // If the orderby widget has a property "always_display",
+ // the button for this sort order is never hidden.
+ var orderby = new Y.lp.ordering.OrderByBar({
+ sort_keys: this.SORT_KEYS,
+ active: 'importance',
+ sort_order: 'desc'
+ });
+ orderby.render();
+ orderby.always_display = 'users_affected_count';
+ var data_visibility = this.getDefaultDataVisibility();
+ Y.lp.buglisting_utils.update_sort_button_visibility(
+ orderby, data_visibility);
+ var users_affected_button = Y.one('#sort-users_affected_count');
+ Assert.isFalse(users_affected_button._isHidden());
}
-
});
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2011-12-12 04:16:26 +0000
+++ lib/lp/bugs/model/bugtask.py 2011-12-14 12:44:18 +0000
@@ -162,7 +162,10 @@
from lp.registry.model.pillar import pillar_sort_key
from lp.registry.model.sourcepackagename import SourcePackageName
from lp.services import features
-from lp.services.propertycache import get_property_cache
+from lp.services.propertycache import (
+ cachedproperty,
+ get_property_cache,
+ )
from lp.soyuz.enums import PackagePublishingStatus
from lp.blueprints.model.specification import Specification
@@ -3206,95 +3209,93 @@
# which will be converted into key-value pairs in the dictionary.
return dict(result)
- def getOrderByColumnDBName(self, col_name):
- """See `IBugTaskSet`."""
- if BugTaskSet._ORDERBY_COLUMN is None:
- # Avoid circular imports.
- from lp.bugs.model.bug import (
- Bug,
- BugTag,
- )
- from lp.registry.model.milestone import Milestone
- from lp.registry.model.person import Person
- Assignee = ClassAlias(Person)
- Reporter = ClassAlias(Person)
- BugTaskSet._ORDERBY_COLUMN = {
- "task": (BugTask.id, []),
- "id": (BugTask.bugID, []),
- "importance": (BugTask.importance, []),
- # TODO: sort by their name?
- "assignee": (
- Assignee.name,
- [
- (Assignee,
- LeftJoin(Assignee, BugTask.assignee == Assignee.id))
- ]),
- "targetname": (BugTask.targetnamecache, []),
- "status": (BugTask._status, []),
- "title": (Bug.title, []),
- "milestone": (BugTask.milestoneID, []),
- "dateassigned": (BugTask.date_assigned, []),
- "datecreated": (BugTask.datecreated, []),
- "date_last_updated": (Bug.date_last_updated, []),
- "date_closed": (BugTask.date_closed, []),
- "number_of_duplicates": (Bug.number_of_duplicates, []),
- "message_count": (Bug.message_count, []),
- "users_affected_count": (Bug.users_affected_count, []),
- "heat": (BugTask.heat, []),
- "latest_patch_uploaded": (Bug.latest_patch_uploaded, []),
- "milestone_name": (
- Milestone.name,
- [
- (Milestone,
- LeftJoin(Milestone,
- BugTask.milestone == Milestone.id))
- ]),
- "reporter": (
- Reporter.name,
- [
- (Bug, Join(Bug, BugTask.bug == Bug.id)),
- (Reporter, Join(Reporter, Bug.owner == Reporter.id))
- ]),
- "tag": (
- BugTag.tag,
- [
- (Bug, Join(Bug, BugTask.bug == Bug.id)),
- (BugTag,
- LeftJoin(
- BugTag,
- BugTag.bug == Bug.id and
- # We want at most one tag per bug. Select the
- # tag that comes first in alphabetic order.
- BugTag.id == SQL("""
- SELECT id FROM BugTag AS bt
- WHERE bt.bug=bug.id ORDER BY bt.name LIMIT 1
- """))),
- ]
- ),
- "specification": (
- Specification.name,
- [
- (Bug, Join(Bug, BugTask.bug == Bug.id)),
- (Specification,
- LeftJoin(
- Specification,
- # We want at most one specification per bug.
- # Select the specification that comes first
- # in alphabetic order.
- Specification.id == SQL("""
- SELECT Specification.id
- FROM SpecificationBug
- JOIN Specification
- ON SpecificationBug.specification=
- Specification.id
- WHERE SpecificationBug.bug=Bug.id
- ORDER BY Specification.name
- LIMIT 1
- """))),
- ]
- ),
- }
- return BugTaskSet._ORDERBY_COLUMN[col_name]
+ @cachedproperty
+ def orderby_expression(self):
+ # Avoid circular imports.
+ from lp.bugs.model.bug import (
+ Bug,
+ BugTag,
+ )
+ from lp.registry.model.milestone import Milestone
+ from lp.registry.model.person import Person
+ Assignee = ClassAlias(Person)
+ Reporter = ClassAlias(Person)
+ return {
+ "task": (BugTask.id, []),
+ "id": (BugTask.bugID, []),
+ "importance": (BugTask.importance, []),
+ # TODO: sort by their name?
+ "assignee": (
+ Assignee.name,
+ [
+ (Assignee,
+ LeftJoin(Assignee, BugTask.assignee == Assignee.id))
+ ]),
+ "targetname": (BugTask.targetnamecache, []),
+ "status": (BugTask._status, []),
+ "title": (Bug.title, []),
+ "milestone": (BugTask.milestoneID, []),
+ "dateassigned": (BugTask.date_assigned, []),
+ "datecreated": (BugTask.datecreated, []),
+ "date_last_updated": (Bug.date_last_updated, []),
+ "date_closed": (BugTask.date_closed, []),
+ "number_of_duplicates": (Bug.number_of_duplicates, []),
+ "message_count": (Bug.message_count, []),
+ "users_affected_count": (Bug.users_affected_count, []),
+ "heat": (BugTask.heat, []),
+ "latest_patch_uploaded": (Bug.latest_patch_uploaded, []),
+ "milestone_name": (
+ Milestone.name,
+ [
+ (Milestone,
+ LeftJoin(Milestone,
+ BugTask.milestone == Milestone.id))
+ ]),
+ "reporter": (
+ Reporter.name,
+ [
+ (Bug, Join(Bug, BugTask.bug == Bug.id)),
+ (Reporter, Join(Reporter, Bug.owner == Reporter.id))
+ ]),
+ "tag": (
+ BugTag.tag,
+ [
+ (Bug, Join(Bug, BugTask.bug == Bug.id)),
+ (BugTag,
+ LeftJoin(
+ BugTag,
+ BugTag.bug == Bug.id and
+ # We want at most one tag per bug. Select the
+ # tag that comes first in alphabetic order.
+ BugTag.id == SQL("""
+ SELECT id FROM BugTag AS bt
+ WHERE bt.bug=bug.id ORDER BY bt.name LIMIT 1
+ """))),
+ ]
+ ),
+ "specification": (
+ Specification.name,
+ [
+ (Bug, Join(Bug, BugTask.bug == Bug.id)),
+ (Specification,
+ LeftJoin(
+ Specification,
+ # We want at most one specification per bug.
+ # Select the specification that comes first
+ # in alphabetic order.
+ Specification.id == SQL("""
+ SELECT Specification.id
+ FROM SpecificationBug
+ JOIN Specification
+ ON SpecificationBug.specification=
+ Specification.id
+ WHERE SpecificationBug.bug=Bug.id
+ ORDER BY Specification.name
+ LIMIT 1
+ """))),
+ ]
+ ),
+ }
def _processOrderBy(self, params):
"""Process the orderby parameter supplied to search().
@@ -3362,11 +3363,11 @@
orderby_arg.append(orderby_col)
continue
if orderby_col.startswith("-"):
- col, sort_joins = self.getOrderByColumnDBName(orderby_col[1:])
+ col, sort_joins = self.orderby_expression[orderby_col[1:]]
extra_joins.extend(sort_joins)
order_clause = Desc(col)
else:
- col, sort_joins = self.getOrderByColumnDBName(orderby_col)
+ col, sort_joins = self.orderby_expression[orderby_col]
extra_joins.extend(sort_joins)
order_clause = col
if col in unambiguous_cols:
=== modified file 'lib/lp/bugs/templates/bugtask-macros-tableview.pt'
--- lib/lp/bugs/templates/bugtask-macros-tableview.pt 2011-12-08 13:23:00 +0000
+++ lib/lp/bugs/templates/bugtask-macros-tableview.pt 2011-12-14 12:44:18 +0000
@@ -662,6 +662,7 @@
if (Y.Lang.isNull(navigator)){
return;
}
+ var sort_keys = LP.cache.sort_keys;
var active_sort_key = LP.cache.order_by;
var sort_order = 'asc';
if (active_sort_key.charAt(0) === '-') {
@@ -669,22 +670,19 @@
1, active_sort_key.length);
sort_order = 'desc';
}
+ var unknown_sort_key = true;
+ for (var i = 0; i < sort_keys.length; i++) {
+ if (sort_keys[i][0] === active_sort_key) {
+ unknown_sort_key = false;
+ break;
+ }
+ }
+ if (unknown_sort_key) {
+ active_sort_key = 'importance';
+ }
var orderby = new Y.lp.ordering.OrderByBar({
srcNode: Y.one('#bugs-orderby'),
- sort_keys: [
- ['id', 'Bug number'],
- ['title', 'Bug title'],
- ['importance', 'Importance'],
- ['status', 'Status'],
- ['heat', 'Bug heat'],
- ['reporter', 'Reporter'],
- ['assignee', 'Assignee'],
- ['targetname', 'Package/Project/Series name'],
- ['milestone_name', 'Milestone'],
- ['datecreated', 'Bug age'],
- ['date_last_updated', 'Date bug last updated'],
- ['tag', 'Bug Tags']
- ],
+ sort_keys: sort_keys,
active: active_sort_key,
sort_order: sort_order,
config_slot: true
@@ -693,7 +691,8 @@
Y.on('orderbybar:sort', function(e) {
navigator.first_batch(e);
});
- navigator.get('model').get('history').after(
+ var model = navigator.get('model');
+ model.get('history').after(
'change', function(e) {
Y.lp.buglisting_utils.update_sort_button_visibility(
orderby, e.newVal);
@@ -701,14 +700,22 @@
var config_node = orderby.get('config_node');
var list_util = new Y.lp.buglisting_utils.BugListingConfigUtil({
srcNode: config_node,
- model: navigator.get('model')
+ model: model
});
list_util.render();
Y.on('buglisting-config-util:fields-changed', function(e) {
navigator.change_fields(list_util.get('field_visibility'));
});
+ var field_visibility =
+ navigator.get('model').get_field_visibility();
+ // The advanced search page contains sort options that have
+ // no related data fields we can display. If a user has selected
+ // such a sort order, this sort option should always be visible.
+ if (field_visibility['show_' + active_sort_key] === undefined) {
+ orderby.always_display = active_sort_key;
+ }
Y.lp.buglisting_utils.update_sort_button_visibility(
- orderby, navigator.get('model').get_field_visibility());
+ orderby, field_visibility);
});
});
</script>
