launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #17551
[Merge] lp:~cjwatson/launchpad/sort-structsub-recipients into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/sort-structsub-recipients into lp:launchpad.
Commit message:
Use unique_displayname rather than title to render administered teams for structural subscriptions, and sort them by team name.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1337329 in Launchpad itself: "Structural subscription recipient picker shows unsorted list of teams"
https://bugs.launchpad.net/launchpad/+bug/1337329
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/sort-structsub-recipients/+merge/242670
== Summary ==
The team recipient picker for new structural subscriptions is unsorted, and thus very hard to navigate.
== Proposed fix ==
Use team.unique_displayname rather than team.title to render the team names; this gets rid of the '"Team name" team' business, which is redundant for something that's explicitly a list of teams anyway. Since team.name is now part of the displayed text, it's reasonable to sort by that.
== Tests ==
bin/with-xvfb bin/test -vvct test_administrated_teams -t lp.bugs.browser.tests.test_expose -t lp.registry.javascript.tests.test_structural_subscription
== Demo and Q/A ==
Go to a DSP page, select "Subscribe to bug mail", and check that the picker contents are reasonable.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/sort-structsub-recipients into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/structuralsubscription.py'
--- lib/lp/bugs/browser/structuralsubscription.py 2013-05-22 02:24:34 +0000
+++ lib/lp/bugs/browser/structuralsubscription.py 2014-11-24 15:26:26 +0000
@@ -478,14 +478,14 @@
precache_permission_for_objects(
None, perm, administers_and_in)
- for team in administers_and_in:
+ for team in sorted(administers_and_in, key=attrgetter('name')):
if (bug_supervisor is not None and
not team.inTeam(bug_supervisor)):
continue
info.append({
'has_preferredemail': team.preferredemail is not None,
'link': absoluteURL(team, api_request),
- 'title': team.title,
+ 'title': team.unique_displayname,
'url': canonical_url(team),
})
objects['administratedTeams'] = info
=== modified file 'lib/lp/bugs/browser/tests/test_expose.py'
--- lib/lp/bugs/browser/tests/test_expose.py 2013-06-20 05:50:00 +0000
+++ lib/lp/bugs/browser/tests/test_expose.py 2014-11-24 15:26:26 +0000
@@ -3,8 +3,6 @@
"""Tests for helpers that expose data about a user to on-page JavaScript."""
-from operator import itemgetter
-
from lazr.enum import (
DBEnumeratedType,
DBItem,
@@ -74,7 +72,7 @@
def fake_absoluteURL(ob, request):
"""An absoluteURL implementation that doesn't require ZTK for testing."""
- return 'http://example.com/' + ob.title.replace(' ', '')
+ return 'http://example.com/' + ob.name.replace(' ', '')
class DemoEnum(DBEnumeratedType):
@@ -121,9 +119,6 @@
bug_super_subteam.join(
self.bug_super_team, self.bug_super_team.teamowner)
- def _sort(self, team_info, key='title'):
- return sorted(team_info, key=itemgetter(key))
-
def test_teams_preferredemail(self):
# The function includes information about whether the team has a
# preferred email. This gives us information in JavaScript that tells
@@ -139,13 +134,10 @@
expose_user_administered_teams_to_js(self.request, self.user, context,
absoluteURL=fake_absoluteURL)
- team_info = self._sort(self.request.objects['administratedTeams'])
- self.assertThat(
- team_info[0]['title'], Equals(u'\u201cTeam 1\u201d team'))
+ team_info = self.request.objects['administratedTeams']
+ self.assertThat(team_info[0]['title'], Equals(u'Team 1 (team-1)'))
self.assertThat(team_info[0]['has_preferredemail'], Equals(False))
- self.assertThat(
- team_info[1]['title'],
- Equals(u'\u201cTeam 2\u201d team'))
+ self.assertThat(team_info[1]['title'], Equals(u'Team 2 (team-2)'))
self.assertThat(team_info[1]['has_preferredemail'], Equals(True))
def test_teams_for_non_distro(self):
@@ -161,7 +153,7 @@
# The team information should have been added to the request.
self.assertThat(self.request.objects, Contains('administratedTeams'))
- team_info = self._sort(self.request.objects['administratedTeams'])
+ team_info = self.request.objects['administratedTeams']
# Since there are three teams, there should be three items in the
# list of team info.
expected_number_teams = 3
@@ -171,18 +163,18 @@
self.assertThat(
team_info[i],
KeysEqual('has_preferredemail', 'link', 'title', 'url'))
- # The link is the title of the team.
+ # The link is the unique display name of the team.
self.assertThat(
team_info[0]['title'],
- Equals(u'\u201cBug Supervisor Sub Team\u201d team'))
+ Equals(u'Bug Supervisor Sub Team (bug-supervisor-sub-team)'))
self.assertThat(
team_info[1]['title'],
- Equals(u'\u201cBug Supervisor Team\u201d team'))
+ Equals(u'Bug Supervisor Team (bug-supervisor-team)'))
self.assertThat(
- team_info[2]['title'], Equals(u'\u201cUnrelated Team\u201d team'))
+ team_info[2]['title'], Equals(u'Unrelated Team (unrelated-team)'))
# The link is the API link to the team.
self.assertThat(team_info[0]['link'],
- Equals(u'http://example.com/\u201cBugSupervisorSubTeam\u201dteam'))
+ Equals(u'http://example.com/bug-supervisor-sub-team'))
def test_query_count(self):
# The function issues a constant number of queries regardless of
@@ -269,7 +261,7 @@
# The team information should have been added to the request.
self.assertThat(self.request.objects, Contains('administratedTeams'))
- team_info = self._sort(self.request.objects['administratedTeams'])
+ team_info = self.request.objects['administratedTeams']
# Since the distro only returns teams that are members of the bug
# supervisor team, we only expect two.
@@ -280,16 +272,16 @@
self.assertThat(
team_info[i],
KeysEqual('has_preferredemail', 'link', 'title', 'url'))
- # The link is the title of the team.
+ # The link is the unique display name of the team.
self.assertThat(
team_info[0]['title'],
- Equals(u'\u201cBug Supervisor Sub Team\u201d team'))
+ Equals(u'Bug Supervisor Sub Team (bug-supervisor-sub-team)'))
self.assertThat(
team_info[1]['title'],
- Equals(u'\u201cBug Supervisor Team\u201d team'))
+ Equals(u'Bug Supervisor Team (bug-supervisor-team)'))
# The link is the API link to the team.
self.assertThat(team_info[0]['link'],
- Equals(u'http://example.com/\u201cBugSupervisorSubTeam\u201dteam'))
+ Equals(u'http://example.com/bug-supervisor-sub-team'))
def test_teams_for_distro_with_no_bug_super(self):
self._setup_teams(self.user)
@@ -301,7 +293,7 @@
# The team information should have been added to the request.
self.assertThat(self.request.objects, Contains('administratedTeams'))
- team_info = self._sort(self.request.objects['administratedTeams'])
+ team_info = self.request.objects['administratedTeams']
# Since the distro has no bug supervisor set, all administered teams
# are returned.
@@ -312,18 +304,18 @@
self.assertThat(
team_info[i],
KeysEqual('has_preferredemail', 'link', 'title', 'url'))
- # The link is the title of the team.
+ # The link is the unique display name of the team.
self.assertThat(
team_info[0]['title'],
- Equals(u'\u201cBug Supervisor Sub Team\u201d team'))
+ Equals(u'Bug Supervisor Sub Team (bug-supervisor-sub-team)'))
self.assertThat(
team_info[1]['title'],
- Equals(u'\u201cBug Supervisor Team\u201d team'))
+ Equals(u'Bug Supervisor Team (bug-supervisor-team)'))
self.assertThat(
- team_info[2]['title'], Equals(u'\u201cUnrelated Team\u201d team'))
+ team_info[2]['title'], Equals(u'Unrelated Team (unrelated-team)'))
# The link is the API link to the team.
self.assertThat(team_info[0]['link'],
- Equals(u'http://example.com/\u201cBugSupervisorSubTeam\u201dteam'))
+ Equals(u'http://example.com/bug-supervisor-sub-team'))
class TestStructuralSubscriptionHelpers(TestCase):
Follow ups