Merge lp:~bac/launchpad/bug-429455-team-pages into lp:launchpad

Proposed by Brad Crittenden
Status: Merged
Approved by: Curtis Hovey
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~bac/launchpad/bug-429455-team-pages
Merge into: lp:launchpad
Diff against target: None lines
To merge this branch: bzr merge lp:~bac/launchpad/bug-429455-team-pages
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Approve
Canonical Launchpad Engineering Pending
Review via email: mp+11739@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Brad Crittenden (bac) wrote :

= Summary =

Bug 429455 requires the conversion of three team pages to UI 3.0:
team-map.pt
team-mugshots.pt
team-members.pt

== Proposed fix ==

Provide changes to fulfill the new UI requirements.

== Pre-implementation notes ==

N/A

== Implementation details ==

* team-map.pt
  * Convert to main_only.
  * Remove from pagetitles.py
  * Create label property
  * Remove h1
  * Remove the section that displays team members with no location
    data and invites anyone to set it for them - Bug 262193

* team-mugshots.pt
  * Convert to main_only.
  * Remove from pagetitles.py
  * Create label property
  * Remove h1

* team-members.pt

== Tests ==

bin/test -vvm lp.registry -t team-map.txt
     -t stories/mailinglists/moderation.txt

== Demo and Q/A ==

Screenshots are available at
http://people.canonical.com/~bac/bug-429455-team-pages

* As Mark, go to: https://launchpad.dev/~hwdb-team/+map
  * Note page title, label, and breadcrumbs.

* Go to https://launchpad.dev/~hwdb-team/+mugshots
  * Note page title, label, and breadcrumbs.
  * Without the side there is much more real estate for photos.

* Go to https://launchpad.dev/~hwdb-team/+members
  * Note page title, label, and breadcrumbs.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/registry/stories/foaf/xx-team-membership.txt
  lib/canonical/config/schema-lazr.conf
  lib/canonical/launchpad/pagetitles.py
  lib/canonical/launchpad/database/librarian.py
  lib/lp/registry/browser/team.py
  lib/lp/registry/templates/team-mugshots.pt
  lib/lp/registry/templates/team-members.pt
  lib/lp/registry/templates/team-map.pt
  lib/lp/registry/browser/person.py
  lib/lp/registry/stories/location/team-map.txt
  configs/development/launchpad-lazr.conf

== Pylint notices ==

lib/lp/registry/browser/person.py
    119: [F0401] Unable to import 'lazr.delegates' (No module named delegates)
    120: [F0401] Unable to import 'lazr.config' (No module named config)
    121: [F0401] Unable to import 'lazr.restful.interface' (No module named restful)

--
Brad Crittenden
<email address hidden>

Revision history for this message
Curtis Hovey (sinzui) wrote :

This looks pretty good. I have a few remarks that I think will improve the code before you land it.

> === modified file 'lib/canonical/launchpad/pagetitles.py'
> --- lib/canonical/launchpad/pagetitles.py 2009-09-14 15:16:12 +0000
> +++ lib/canonical/launchpad/pagetitles.py 2009-09-14 19:38:57 +0000
> @@ -1033,8 +1033,6 @@
> team_mailinglist_subscribers = ContextBrowsername(
> 'Mailing list subscribers for the %s team')
>
> -team_map = ContextBrowsername('Map of %s participants')
> -
> team_members = ContextBrowsername(smartquote('"%s" members'))
>
> team_mugshots = ContextBrowsername(smartquote('Mugshots in the "%s" team'))

Why didn't you remove these? IS it because base-layout still requires a
page_title. I think you can do this in the page views.

    page_title = label

So that you can remove the entries from pagetitles.py

> === modified file 'lib/lp/registry/browser/person.py'
> --- lib/lp/registry/browser/person.py 2009-09-14 14:10:10 +0000
> +++ lib/lp/registry/browser/person.py 2009-09-14 19:32:10 +0000

...

> @@ -4564,6 +4568,10 @@
> getUtility(IPersonSet).cacheBrandingForPeople(self.allmembers)
>
> @cachedproperty
> + def label(self):
> + return "Who's in this team?"

This does not need to be a cachedproperty.

    label = "Who's in this team?"

> === modified file 'lib/lp/registry/browser/team.py'
> --- lib/lp/registry/browser/team.py 2009-09-01 19:43:30 +0000
> +++ lib/lp/registry/browser/team.py 2009-09-14 19:32:10 +0000
> @@ -1019,6 +1019,10 @@
> self.request.needs_gmap2 = True
>
> @cachedproperty
> + def label(self):
> + return "Team member locations"

This does not need to be a cachedproperty.

    label = "Team member locations"

review: Approve (code)
Revision history for this message
Barry Warsaw (barry) wrote :

On Sep 14, 2009, at 5:35 PM, Curtis Hovey wrote:

> Why didn't you remove these? IS it because base-layout still
> requires a
> page_title. I think you can do this in the page views.
>
> page_title = label
>
> So that you can remove the entries from pagetitles.py

base-layout only requires page_title in some limited circumstances.

https://bugs.edge.launchpad.net/launchpad-foundations/+bug/429473/comments/1

Revision history for this message
Brad Crittenden (bac) wrote :

On Sep 14, 2009, at 17:35 , Curtis Hovey wrote:

> Review: Approve code
> This looks pretty good. I have a few remarks that I think will
> improve the code before you land it.
>
>> === modified file 'lib/canonical/launchpad/pagetitles.py'
>> --- lib/canonical/launchpad/pagetitles.py 2009-09-14 15:16:12 +0000
>> +++ lib/canonical/launchpad/pagetitles.py 2009-09-14 19:38:57 +0000
>> @@ -1033,8 +1033,6 @@
>> team_mailinglist_subscribers = ContextBrowsername(
>> 'Mailing list subscribers for the %s team')
>>
>> -team_map = ContextBrowsername('Map of %s participants')
>> -
>> team_members = ContextBrowsername(smartquote('"%s" members'))
>>
>> team_mugshots = ContextBrowsername(smartquote('Mugshots in the "%s"
>> team'))
>
> Why didn't you remove these? IS it because base-layout still
> requires a
> page_title. I think you can do this in the page views.

I just forgot. Thanks for catching them.

>
> page_title = label
>
> So that you can remove the entries from pagetitles.py

These views did not require a page_title.

>
>> === modified file 'lib/lp/registry/browser/person.py'
>> --- lib/lp/registry/browser/person.py 2009-09-14 14:10:10 +0000
>> +++ lib/lp/registry/browser/person.py 2009-09-14 19:32:10 +0000
>
> ...
>
>> @@ -4564,6 +4568,10 @@
>> getUtility(IPersonSet).cacheBrandingForPeople
>> (self.allmembers)
>>
>> @cachedproperty
>> + def label(self):
>> + return "Who's in this team?"
>
> This does not need to be a cachedproperty.
>
> label = "Who's in this team?"

Of course. Done.

>
>> === modified file 'lib/lp/registry/browser/team.py'
>> --- lib/lp/registry/browser/team.py 2009-09-01 19:43:30 +0000
>> +++ lib/lp/registry/browser/team.py 2009-09-14 19:32:10 +0000
>> @@ -1019,6 +1019,10 @@
>> self.request.needs_gmap2 = True
>>
>> @cachedproperty
>> + def label(self):
>> + return "Team member locations"
>
> This does not need to be a cachedproperty.
>
> label = "Team member locations"
>

Done.

Diff at http://pastebin.ubuntu.com/271285/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'configs/development/launchpad-lazr.conf'
--- configs/development/launchpad-lazr.conf 2009-08-19 12:28:32 +0000
+++ configs/development/launchpad-lazr.conf 2009-09-14 19:32:10 +0000
@@ -145,6 +145,7 @@
145restricted_upload_port: 58095145restricted_upload_port: 58095
146restricted_download_port: 58085146restricted_download_port: 58085
147restricted_download_url: http://launchpad.dev:58085/147restricted_download_url: http://launchpad.dev:58085/
148use_https = False
148149
149[librarian_server]150[librarian_server]
150root: /var/tmp/fatsam151root: /var/tmp/fatsam
151152
=== modified file 'lib/canonical/config/schema-lazr.conf'
--- lib/canonical/config/schema-lazr.conf 2009-09-14 09:12:58 +0000
+++ lib/canonical/config/schema-lazr.conf 2009-09-14 19:32:10 +0000
@@ -1144,6 +1144,8 @@
1144# datatype: urlbase1144# datatype: urlbase
1145restricted_download_url: http://restricted-librarian.launchpad.net/1145restricted_download_url: http://restricted-librarian.launchpad.net/
11461146
1147use_https = True
1148
1147[librarian_gc]1149[librarian_gc]
1148# The database user which will be used by this process.1150# The database user which will be used by this process.
1149# datatype: string1151# datatype: string
11501152
=== modified file 'lib/canonical/launchpad/database/librarian.py'
--- lib/canonical/launchpad/database/librarian.py 2009-08-27 01:32:01 +0000
+++ lib/canonical/launchpad/database/librarian.py 2009-09-14 19:32:10 +0000
@@ -98,7 +98,7 @@
9898
99 def getURL(self):99 def getURL(self):
100 """See ILibraryFileAlias.getURL"""100 """See ILibraryFileAlias.getURL"""
101 if config.vhosts.use_https:101 if config.librarian.use_https:
102 return self.https_url102 return self.https_url
103 else:103 else:
104 return self.http_url104 return self.http_url
105105
=== modified file 'lib/canonical/launchpad/pagetitles.py'
--- lib/canonical/launchpad/pagetitles.py 2009-09-14 15:16:12 +0000
+++ lib/canonical/launchpad/pagetitles.py 2009-09-14 19:38:57 +0000
@@ -1033,8 +1033,6 @@
1033team_mailinglist_subscribers = ContextBrowsername(1033team_mailinglist_subscribers = ContextBrowsername(
1034 'Mailing list subscribers for the %s team')1034 'Mailing list subscribers for the %s team')
10351035
1036team_map = ContextBrowsername('Map of %s participants')
1037
1038team_members = ContextBrowsername(smartquote('"%s" members'))1036team_members = ContextBrowsername(smartquote('"%s" members'))
10391037
1040team_mugshots = ContextBrowsername(smartquote('Mugshots in the "%s" team'))1038team_mugshots = ContextBrowsername(smartquote('Mugshots in the "%s" team'))
10411039
=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py 2009-09-14 14:10:10 +0000
+++ lib/lp/registry/browser/person.py 2009-09-14 19:32:10 +0000
@@ -1327,7 +1327,11 @@
13271327
13281328
1329class TeamMembershipView(LaunchpadView):1329class TeamMembershipView(LaunchpadView):
1330 """The view behins ITeam/+members."""1330 """The view behind ITeam/+members."""
1331
1332 @cachedproperty
1333 def label(self):
1334 return smartquote('Members of "%s"' % self.context.displayname)
13311335
1332 @cachedproperty1336 @cachedproperty
1333 def active_memberships(self):1337 def active_memberships(self):
@@ -4564,6 +4568,10 @@
4564 getUtility(IPersonSet).cacheBrandingForPeople(self.allmembers)4568 getUtility(IPersonSet).cacheBrandingForPeople(self.allmembers)
45654569
4566 @cachedproperty4570 @cachedproperty
4571 def label(self):
4572 return "Who's in this team?"
4573
4574 @cachedproperty
4567 def allmembers(self):4575 def allmembers(self):
4568 return list(self.context.allmembers)4576 return list(self.context.allmembers)
45694577
45704578
=== modified file 'lib/lp/registry/browser/team.py'
--- lib/lp/registry/browser/team.py 2009-09-01 19:43:30 +0000
+++ lib/lp/registry/browser/team.py 2009-09-14 19:32:10 +0000
@@ -1019,6 +1019,10 @@
1019 self.request.needs_gmap2 = True1019 self.request.needs_gmap2 = True
10201020
1021 @cachedproperty1021 @cachedproperty
1022 def label(self):
1023 return "Team member locations"
1024
1025 @cachedproperty
1022 def mapped_participants(self):1026 def mapped_participants(self):
1023 """Participants with locations."""1027 """Participants with locations."""
1024 return self.context.getMappedParticipants(limit=self.limit)1028 return self.context.getMappedParticipants(limit=self.limit)
10251029
=== modified file 'lib/lp/registry/stories/foaf/xx-team-membership.txt'
--- lib/lp/registry/stories/foaf/xx-team-membership.txt 2009-01-03 19:32:48 +0000
+++ lib/lp/registry/stories/foaf/xx-team-membership.txt 2009-09-14 20:37:47 +0000
@@ -3,8 +3,6 @@
3 >>> browser.open('http://launchpad.dev/~ubuntu-team/+members')3 >>> browser.open('http://launchpad.dev/~ubuntu-team/+members')
4 >>> 'Ubuntu Team' in browser.contents4 >>> 'Ubuntu Team' in browser.contents
5 True5 True
6 >>> 'Mark Shuttleworth' in browser.contents
7 True
86
9Let's take a look at Colin's subscription page. Colin is an7Let's take a look at Colin's subscription page. Colin is an
10administrator and his subscription never expires.8administrator and his subscription never expires.
@@ -240,4 +238,3 @@
240 >>> indirect = find_portlet(browser.contents, 'Indirect membership')238 >>> indirect = find_portlet(browser.contents, 'Indirect membership')
241 >>> print indirect239 >>> print indirect
242 None240 None
243
244241
=== modified file 'lib/lp/registry/stories/location/team-map.txt'
--- lib/lp/registry/stories/location/team-map.txt 2009-06-12 16:36:02 +0000
+++ lib/lp/registry/stories/location/team-map.txt 2009-09-14 21:01:18 +0000
@@ -15,16 +15,12 @@
15 >>> 'build/lp/mapping.js' in markup15 >>> 'build/lp/mapping.js' in markup
16 True16 True
1717
18You should also be able to see a map of the team, with links to the people18You should also be able to see a map of the team.
19who need to have location data added to complete the team.
2019
21 >>> maplink = nopriv_browser.getLink('View map and time zones')20 >>> maplink = nopriv_browser.getLink('View map and time zones')
22 >>> maplink.click()21 >>> maplink.click()
23 >>> nopriv_browser.url22 >>> nopriv_browser.url
24 'http://launchpad.dev/~guadamen/+map'23 'http://launchpad.dev/~guadamen/+map'
25 >>> link = nopriv_browser.getLink('Edgar Bursic')
26 >>> link.url
27 'http://launchpad.dev/~edgar/+editlocation?for_team=guadamen'
2824
29The map depends on a stream of XML-formatted data, giving the locations of25The map depends on a stream of XML-formatted data, giving the locations of
30all members of the team. We show that this stream works for teams with, and26all members of the team. We show that this stream works for teams with, and
@@ -60,24 +56,6 @@
60 </participants>56 </participants>
61 <BLANKLINE>57 <BLANKLINE>
6258
63We have a nice way to add locations for all the members of a team. We look
64at a page which shows all the members of the team on a map, together with
65links to edit the location of folks who have not yet been mapped. When we
66click on those links, we pass the name of the team, so when the location is
67added, we come back to the team map, and from there we can click on the next
68person.
69
70 >>> admin_browser.open('http://launchpad.dev/~guadamen/+map')
71 >>> admin_browser.getLink('Edgar Bursic').click()
72 >>> admin_browser.url
73 'http://launchpad.dev/~edgar/+editlocation?for_team=guadamen'
74 >>> admin_browser.getControl(name='field.location.latitude').value = '37.3'
75 >>> admin_browser.getControl(name='field.location.longitude').value = '2.45'
76 >>> admin_browser.getControl(name='field.location.time_zone').value = ['Europe/Madrid']
77 >>> admin_browser.getControl('Update').click()
78 >>> admin_browser.url
79 'http://launchpad.dev/~guadamen/+map'
80
81It doesn't make sense to edit the location of the team itself, not even59It doesn't make sense to edit the location of the team itself, not even
82if we are an admin, so a team's +editlocation page will simply redirect60if we are an admin, so a team's +editlocation page will simply redirect
83to +map.61to +map.
@@ -92,13 +70,13 @@
92The display name of all team participants will be escaped to prevent70The display name of all team participants will be escaped to prevent
93XSS attacks on any callsite of +mapdata.71XSS attacks on any callsite of +mapdata.
9472
95 >>> admin_browser.open('http://launchpad.dev/~edgar/+edit')73 >>> admin_browser.open('http://launchpad.dev/~kamion/+edit')
96 >>> admin_browser.getControl('Display Name').value = (74 >>> admin_browser.getControl('Display Name').value = (
97 ... "<script>alert('John \"nasty\"');</script>")75 ... "<script>alert('Colin \"nasty\"');</script>")
98 >>> admin_browser.getControl('Save Changes').click()76 >>> admin_browser.getControl('Save Changes').click()
9977
100 >>> anon_browser.open('http://launchpad.dev/~guadamen/+mapdata')78 >>> anon_browser.open('http://launchpad.dev/~guadamen/+mapdata')
101 >>> print anon_browser.contents79 >>> print anon_browser.contents
102 <?xml version="1.0"...80 <?xml version="1.0"...
103 ...displayname="&amp;lt;script&amp;gt;alert('John &amp;quot;nasty&amp;quot;');&amp;lt;/script&amp;gt;"81 ...displayname="&amp;lt;script&amp;gt;alert('Colin &amp;quot;nasty&amp;quot;');&amp;lt;/script&amp;gt;"
104 ...82 ...
10583
=== modified file 'lib/lp/registry/templates/team-map.pt'
--- lib/lp/registry/templates/team-map.pt 2009-07-17 17:59:07 +0000
+++ lib/lp/registry/templates/team-map.pt 2009-09-14 20:48:51 +0000
@@ -3,10 +3,7 @@
3 xmlns:tal="http://xml.zope.org/namespaces/tal"3 xmlns:tal="http://xml.zope.org/namespaces/tal"
4 xmlns:metal="http://xml.zope.org/namespaces/metal"4 xmlns:metal="http://xml.zope.org/namespaces/metal"
5 xmlns:i18n="http://xml.zope.org/namespaces/i18n"5 xmlns:i18n="http://xml.zope.org/namespaces/i18n"
6 xml:lang="en"6 metal:use-macro="view/macro:page/main_only"
7 lang="en"
8 dir="ltr"
9 metal:use-macro="view/macro:page/onecolumn"
10 i18n:domain="launchpad"7 i18n:domain="launchpad"
11>8>
129
@@ -14,28 +11,6 @@
1411
15<div metal:fill-slot="main">12<div metal:fill-slot="main">
1613
17 <h1>Members' location</h1>
18
19 <tal:unmapped condition="view/unmapped_participants_count">
20 <div style="margin: 4px;">
21 <h2>Complete the map!</h2>
22 Do you know the current location for these team members? Set their
23 locations!
24 </div>
25
26 <div style="margin-bottom: 2em;">
27 <span style="float: left; width: 15em"
28 tal:repeat="person view/unmapped_participants">
29 <a tal:condition="person/name|nothing"
30 tal:attributes="
31 href string:${person/fmt:url}/+editlocation?for_team=${context/name}"
32 title="Set this person's location"
33 ><img src="/@@/edit" />
34 <tal:person replace="person/displayname" /></a>
35 </span>
36 </div>
37 </tal:unmapped>
38
39 <div style="height:420px;">14 <div style="height:420px;">
40 <div tal:condition="not: view/has_mapped_participants">15 <div tal:condition="not: view/has_mapped_participants">
41 None of the current participants in16 None of the current participants in
4217
=== modified file 'lib/lp/registry/templates/team-members.pt'
--- lib/lp/registry/templates/team-members.pt 2009-07-17 17:59:07 +0000
+++ lib/lp/registry/templates/team-members.pt 2009-09-14 19:32:10 +0000
@@ -3,27 +3,16 @@
3 xmlns:tal="http://xml.zope.org/namespaces/tal"3 xmlns:tal="http://xml.zope.org/namespaces/tal"
4 xmlns:metal="http://xml.zope.org/namespaces/metal"4 xmlns:metal="http://xml.zope.org/namespaces/metal"
5 xmlns:i18n="http://xml.zope.org/namespaces/i18n"5 xmlns:i18n="http://xml.zope.org/namespaces/i18n"
6 xml:lang="en"6 metal:use-macro="view/macro:page/main_only"
7 lang="en"
8 dir="ltr"
9 metal:use-macro="context/@@main_template/master"
10 i18n:domain="launchpad"7 i18n:domain="launchpad"
11>8>
129
13<body>10<body>
1411
15<metal:portlets fill-slot="portlets">
16 <div tal:replace="structure context/@@+portlet-details" />
17</metal:portlets>
18
19<div metal:fill-slot="main"12<div metal:fill-slot="main"
20 tal:define="user_can_edit_memberships context/required:launchpad.Edit;13 tal:define="user_can_edit_memberships context/required:launchpad.Edit;
21 active_member_count context/active_member_count">14 active_member_count context/active_member_count">
2215
23 <h1>Members of
24 &#8220;<span tal:replace="context/displayname" />&#8221;
25 </h1>
26
27 <p>Active, pending and former members of this team.</p>16 <p>Active, pending and former members of this team.</p>
2817
29 <ul>18 <ul>
@@ -34,7 +23,7 @@
3423
35 <p>24 <p>
36 There are <span tal:replace="active_member_count">15</span>25 There are <span tal:replace="active_member_count">15</span>
37 direct members of the 26 direct members of the
38 "<span tal:replace="context/displayname">Ubuntu Members</span>" team,27 "<span tal:replace="context/displayname">Ubuntu Members</span>" team,
39 and <span tal:replace="context/all_member_count">23</span> people are28 and <span tal:replace="context/all_member_count">23</span> people are
40 members in total, directly and indirectly through other team29 members in total, directly and indirectly through other team
@@ -103,7 +92,7 @@
10392
104 <li tal:condition="not: active_member_count">93 <li tal:condition="not: active_member_count">
105 <h2>94 <h2>
106 &#8220;<span tal:replace="context/displayname" />&#8221; has no 95 &#8220;<span tal:replace="context/displayname" />&#8221; has no
107 members96 members
108 </h2>97 </h2>
10998
11099
=== modified file 'lib/lp/registry/templates/team-mugshots.pt'
--- lib/lp/registry/templates/team-mugshots.pt 2009-07-17 17:59:07 +0000
+++ lib/lp/registry/templates/team-mugshots.pt 2009-09-14 19:32:10 +0000
@@ -3,32 +3,25 @@
3 xmlns:tal="http://xml.zope.org/namespaces/tal"3 xmlns:tal="http://xml.zope.org/namespaces/tal"
4 xmlns:metal="http://xml.zope.org/namespaces/metal"4 xmlns:metal="http://xml.zope.org/namespaces/metal"
5 xmlns:i18n="http://xml.zope.org/namespaces/i18n"5 xmlns:i18n="http://xml.zope.org/namespaces/i18n"
6 xml:lang="en"6 metal:use-macro="view/macro:page/main_only"
7 lang="en"
8 dir="ltr"
9 metal:use-macro="context/@@main_template/master"
10 i18n:domain="launchpad"7 i18n:domain="launchpad"
11>8>
129
13<body>10<body>
1411
15<metal:heading fill-slot="pageheading">
16 <h1>Who's in this team?</h1>
17</metal:heading>
18
19<div metal:fill-slot="main">12<div metal:fill-slot="main">
2013
21<tal:per_member repeat="member view/allmembers">14 <tal:per_member repeat="member view/allmembers">
22 <a tal:condition="member/mugshot"15 <a tal:condition="member/mugshot"
23 tal:attributes="16 tal:attributes="
24 href member/fmt:url;17 href member/fmt:url;
25 title member/displayname">18 title member/displayname">
26 <img tal:attributes="src member/mugshot/http_url"/>19 <img tal:attributes="src member/mugshot/http_url"/>
27 </a>20 </a>
28 <span style="white-space: nowrap;" tal:condition="not: member/mugshot">21 <span style="white-space: nowrap;" tal:condition="not: member/mugshot">
29 [<a tal:content="member/displayname"22 [<a tal:content="member/displayname"
30 tal:attributes="href member/fmt:url">Foo Bar</a>]</span>23 tal:attributes="href member/fmt:url">Foo Bar</a>]</span>
31</tal:per_member>24 </tal:per_member>
3225
33</div>26</div>
34</body>27</body>