launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

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):