Launchpad itself

Merge lp:~bac/launchpad/bug-436980-mugshots into lp:launchpad

  1. bug-436980-mugshots
  2. Merge into devel
Proposed by Brad Crittenden
Status: Merged
Merged at revision: not available
Proposed branch: lp:~bac/launchpad/bug-436980-mugshots
Merge into: lp:launchpad
Diff against target: 294 lines
10 files modified
configs/development/launchpad-lazr.conf (+2/-0)
lib/canonical/config/schema-lazr.conf (+6/-0)
lib/canonical/launchpad/icing/style-3-0.css (+22/-0)
lib/lp/registry/browser/announcement.py (+2/-1)
lib/lp/registry/browser/person.py (+9/-5)
lib/lp/registry/browser/tests/announcement-views.txt (+19/-0)
lib/lp/registry/browser/tests/team-views.txt (+27/-0)
lib/lp/registry/stories/announcements/xx-announcements.txt (+19/-5)
lib/lp/registry/stories/foaf/xx-team-home.txt (+12/-6)
lib/lp/registry/templates/team-mugshots.pt (+26/-11)
To merge this branch: bzr merge lp:~bac/launchpad/bug-436980-mugshots
Reviewer Review Type Date Requested Status
Martin Albisetti (community) ui Approve
Michael Nelson (community) ui* Approve
Graham Binns (community) code Approve
Review via email: mp+13250@code.launchpad.net

Commit message

Batch team mugshots. If no mugshot is available show the default mugshot.

To post a comment you must log in.
Revision history for this message
Brad Crittenden (bac) wrote :

= Summary =

The team's +mugshots page currently shows all members of the team and for large teams
that causes problems as the queries frequently time out. Instead of showing all
members on one page the mugshot page should be batched.

== Proposed fix ==

The batch navigator with a size of 32 people (8 in dev) is used. A reasonably sized
browser will show about 4 images across so 4 x 8 seemed like a good number to pick.

The old page showed the mugshot if one was available and the person's name if not.
This was weird because the photos weren't labeled, unless you did a mouse over. The
layout was pretty ugly too.

This change uses the generic mugshot for people who don't have a customized one. It
may be monotonous for teams with a large number of people with no custom mugshot but
it seems like the right thing to do.

A screenshot in lp.dev can be seen at: http://people.canonical.com/~bac/mugshots.png
(None of the people in our sample data have mugshots so I only uploaded one.)

I apologize for the drive-by changes to announcements. While I was adding a
configuration value for the mugshot batch size I decided to do some clean up to a
change I made in the run up to 3.0 for announcements. I didn't like having the batch
size hard-coded in the view. Hope it doesn't add too much noise to the review.

== Pre-implementation notes ==

Chatted with Curtis and he supplied the CSS changes.

== Implementation details ==

As above.

== Tests ==

bin/test -vv -t xx-team-home.txt -t team-views.txt
bin/test -vv -t announcements-views.txt -t xx-announcements.txt

== Demo and Q/A ==

Go to https://launchpad.dev/~ubuntu-team and click on 'Show member photos'.

= 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/announcements/xx-announcements.txt
  lib/lp/registry/browser/announcement.py
  lib/canonical/config/schema-lazr.conf
  lib/lp/registry/browser/tests/announcement-views.txt
  lib/lp/registry/templates/team-mugshots.pt
  lib/canonical/launchpad/icing/style-3-0.css
  lib/lp/registry/stories/foaf/xx-team-home.txt
  lib/lp/registry/browser/tests/team-views.txt
  lib/lp/registry/browser/person.py
  configs/development/launchpad-lazr.conf

== Pylint notices ==

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

Blah.

Revision history for this message
Graham Binns (gmb) :
review: Approve (code)
Revision history for this message
Michael Nelson (michael.nelson) wrote :

Looks great Brad!

The only thing that I think could be improved is the spacing between the spans. Currently the displayname of each mugshot is wedged in between two mugshots. If there were 32 mugshots on the page, after scrolling it would be more difficult to recognise which displayname belonged to which mugshot.

I'd suggest adding:
 padding: 1em 1em 1em 0;
to the .gridflow li span. It seems to provide enough space inside each span so that the displayname is clearly associated with the mugshot above it. See what you think.

Oh, and I realise it wasn't changed as part of this branch, but is "Who's in this team?" a good h1/breadcrumb? Given that the link that I clicked on was "Show member photos" perhaps "Member photos" would be better?

... and one css-style point: you've indented the css with 8 spaces rather than 4.

Thanks!

review: Approve (ui*)
Revision history for this message
Martin Albisetti (beuno) wrote :

Hi Brad, Michael, it's great to see this page get some love.
Michael's comment on the needed padding is spot on, that would make the UI good to land for me as well.

I do however have 3 comments about this page, that if addressed, probably belong in a follow-up branch:

1) I see this page used frequently to show off the team. By showing it in batches of 8, it will not really be possible for a team larger than 8. Could we address that in any way? I understand that "see all" will timeout, but it's a shame we loose that "feature". Maybe middle ground would be larger batches. I don't think scrolling is an issue here, so 24 or even 32 wouldn't be too bad.

2) This page is only really useful if people have mugshots, otherwise it's pretty useless. Could we order alphabetically, but showing the people with pictures first?

3) Last but not least, if a user hasn't set their mugshot, it would be super cool if they could when they see they don't have one on this page. Maybe just a "Upload a picture" link under his empty mugshot?

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

Michael:

Thanks for your comments. Your CSS change does make it look much nicer. I've also changed the label to "Member photos" which does make more sense.

Martin:

Your suggestions are good ones but indeed outside the scope of what we wanted to accomplish with this branch. I'll open a bug for the follow-on work.

Incremental diff at http://pastebin.ubuntu.com/292380/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'configs/development/launchpad-lazr.conf'
2--- configs/development/launchpad-lazr.conf 2009-09-29 15:15:21 +0000
3+++ configs/development/launchpad-lazr.conf 2009-10-13 15:08:16 +0000
4@@ -123,6 +123,8 @@
5 default_batch_size: 5
6 max_attachment_size: 2097152
7 branchlisting_batch_size: 6
8+mugshot_batch_size: 8
9+announcement_batch_size: 4
10 openid_preauthorization_acl:
11 localhost http://launchpad.dev/
12 max_bug_feed_cache_minutes: 30
13
14=== modified file 'lib/canonical/config/schema-lazr.conf'
15--- lib/canonical/config/schema-lazr.conf 2009-10-01 12:06:50 +0000
16+++ lib/canonical/config/schema-lazr.conf 2009-10-13 15:08:16 +0000
17@@ -930,6 +930,12 @@
18 # datatype: integer
19 branchlisting_batch_size: 100
20
21+# The default size used in a batched display of team mugshots.
22+mugshot_batch_size: 32
23+
24+# The default size used in a batched display of announcements.
25+announcement_batch_size: 5
26+
27 # If restrict_to_team is set (such as on the beta
28 # website), then this indicates the hostname suffix for
29 # the non-restricted version of Launchpad. Replacing
30
31=== modified file 'lib/canonical/launchpad/icing/style-3-0.css'
32--- lib/canonical/launchpad/icing/style-3-0.css 2009-10-02 13:56:25 +0000
33+++ lib/canonical/launchpad/icing/style-3-0.css 2009-10-13 15:08:16 +0000
34@@ -941,3 +941,25 @@
35 margin-top: 2em;
36 clear: both;
37 }
38+.gridflow {
39+ margin: 0.0em;
40+ padding: 0.0em;
41+}
42+.gridflow ul {
43+ margin: 0.5em 0.0em 0.5em 0.0em;
44+ padding: 0.5em 0.0em 0.0em 0.0em;
45+}
46+.gridflow li {
47+ display: inline;
48+ margin: 0.0em;
49+ padding: 0.2em;
50+}
51+.gridflow li span {
52+ display: table-cell;
53+ display: inline-table;
54+ display: inline-block;
55+ width: 192px;
56+ text-align: center;
57+ vertical-align: middle;
58+ padding: 1em 1em 1em 0;
59+}
60
61=== modified file 'lib/lp/registry/browser/announcement.py'
62--- lib/lp/registry/browser/announcement.py 2009-10-05 22:14:18 +0000
63+++ lib/lp/registry/browser/announcement.py 2009-10-13 15:08:16 +0000
64@@ -24,6 +24,7 @@
65
66 from lp.registry.interfaces.announcement import IAnnouncement
67
68+from canonical.config import config
69 from canonical.launchpad import _
70 from canonical.launchpad.browser.feeds import (
71 AnnouncementsFeedLink, FeedsMixin, RootAnnouncementsFeedLink)
72@@ -263,7 +264,7 @@
73 """A view class for pillars which have announcements."""
74 implements(IAnnouncementCreateMenu)
75
76- batch_size = 5
77+ batch_size = config.launchpad.announcement_batch_size
78
79 @cachedproperty
80 def feed_url(self):
81
82=== modified file 'lib/lp/registry/browser/person.py'
83--- lib/lp/registry/browser/person.py 2009-10-07 21:45:58 +0000
84+++ lib/lp/registry/browser/person.py 2009-10-13 15:08:16 +0000
85@@ -4662,16 +4662,20 @@
86 class TeamMugshotView(LaunchpadView):
87 """A view for the team mugshot (team photo) page"""
88
89- label = "Who's in this team?"
90+ label = "Member photos"
91+ batch_size = config.launchpad.mugshot_batch_size
92
93 def initialize(self):
94 """Cache images to avoid dying from a million cuts."""
95- getUtility(IPersonSet).cacheBrandingForPeople(self.allmembers)
96-
97+ getUtility(IPersonSet).cacheBrandingForPeople(
98+ self.members.currentBatch())
99
100 @cachedproperty
101- def allmembers(self):
102- return list(self.context.allmembers)
103+ def members(self):
104+ """Get a batch of all members in the team."""
105+ batch_nav = BatchNavigator(
106+ self.context.allmembers, self.request, size=self.batch_size)
107+ return batch_nav
108
109
110 class TeamReassignmentView(ObjectReassignmentView):
111
112=== modified file 'lib/lp/registry/browser/tests/announcement-views.txt'
113--- lib/lp/registry/browser/tests/announcement-views.txt 2009-10-05 21:31:28 +0000
114+++ lib/lp/registry/browser/tests/announcement-views.txt 2009-10-13 15:08:16 +0000
115@@ -45,3 +45,22 @@
116
117 >>> check_menu_links(AnnouncementCreateNavigationMenu(product))
118 True
119+
120+
121+Batching
122+--------
123+
124+Announcements are presented in batches. For launchpad.dev the batch size is
125+smaller than in production to ease testing.
126+
127+ >>> from lp.registry.interfaces.announcement import IAnnouncementSet
128+ >>> announcement_set = getUtility(IAnnouncementSet)
129+ >>> view = create_initialized_view(
130+ ... announcement_set, '+announcements')
131+ >>> print len(list(view.announcements))
132+ 20
133+ >>> print view.batch_size
134+ 4
135+ >>> batch = view.announcement_nav.currentBatch()
136+ >>> print len(list(batch))
137+ 4
138
139=== modified file 'lib/lp/registry/browser/tests/team-views.txt'
140--- lib/lp/registry/browser/tests/team-views.txt 2009-08-24 14:26:34 +0000
141+++ lib/lp/registry/browser/tests/team-views.txt 2009-10-13 15:08:16 +0000
142@@ -229,3 +229,30 @@
143 Send an email to this team's owner through Launchpad
144 >>> print view.specific_contact_text
145 Contact this team's owner
146+
147+
148+== Mugshots ==
149+
150+The mugshots for all members of a team can be seen on the +mugshots
151+page. The display of mugshots is batched.
152+
153+ >>> ubuntu = person_set.getByName('ubuntu-team')
154+ >>> view = create_initialized_view(ubuntu, '+mugshots')
155+ >>> batch = view.members.currentBatch()
156+ >>> print len(list(ubuntu.allmembers))
157+ 10
158+ >>> print view.batch_size
159+ 8
160+ >>> print len(list(batch))
161+ 8
162+ >>> from zope.security.proxy import removeSecurityProxy
163+ >>> for person in list(batch):
164+ ... print removeSecurityProxy(person)
165+ <Person at ... limi (Alexander Limi)>
166+ <Person at ... cprov (Celso Providelo)>
167+ <Person at ... kamion (Colin Watson)>
168+ <Person at ... kinnison (Daniel Silverstone)>
169+ <Person at ... edgar (Edgar Bursic)>
170+ <Person at ... name16 (Foo Bar)>
171+ <Person at ... jdub (Jeff Waugh)>
172+ <Person at ... mark (Mark Shuttleworth)>
173
174=== modified file 'lib/lp/registry/stories/announcements/xx-announcements.txt'
175--- lib/lp/registry/stories/announcements/xx-announcements.txt 2009-10-05 21:35:18 +0000
176+++ lib/lp/registry/stories/announcements/xx-announcements.txt 2009-10-13 15:08:16 +0000
177@@ -450,12 +450,26 @@
178 >>> anon_browser.open('http://launchpad.dev/+announcements')
179 >>> 'Announcements from all projects' in anon_browser.title
180 True
181+ >>> 'Kubuntu announcement' in announcements(anon_browser.contents)
182+ True
183+ >>> 'RedHat announcement ' in announcements(anon_browser.contents)
184+ True
185+ >>> 'Derby announcement ' in announcements(anon_browser.contents)
186+ True
187+ >>> 'Apache announcement ' in announcements(anon_browser.contents)
188+ True
189+
190+The announcements are batched so only the latest four are shown,
191+leaving Tomcat out:
192+
193+ >>> print extract_text(anon_browser.contents)
194+ Announcements from all projects hosted in Launchpad
195+ ...
196+ 1...4 of 25 results
197+ ...
198+
199 >>> 'Tomcat announcement ' in announcements(anon_browser.contents)
200- True
201- >>> 'Apache announcement ' in announcements(anon_browser.contents)
202- True
203- >>> 'Kubuntu announcement' in announcements(anon_browser.contents)
204- True
205+ False
206
207 It excludes future announcements too:
208
209
210=== modified file 'lib/lp/registry/stories/foaf/xx-team-home.txt'
211--- lib/lp/registry/stories/foaf/xx-team-home.txt 2009-09-18 19:31:04 +0000
212+++ lib/lp/registry/stories/foaf/xx-team-home.txt 2009-10-13 15:08:16 +0000
213@@ -148,7 +148,7 @@
214 ... find_tag_by_id(sample_browser.contents, 'contact-user'))
215 Contact this team's members
216
217-As teams do not have OpenID Logins, there is not link in the Contact
218+As teams do not have OpenID Logins, there is no link in the Contact
219 details section for help.
220
221 >>> sample_browser.getLink('(What\xe2\x80\x99s\xc2\xa0this?)')
222@@ -166,13 +166,19 @@
223 You are an indirect member of this team:
224 Sample Person &rarr; Warty Security Team &rarr; Ubuntu Gnome Team...
225
226-It is also possible to view the set of mugshots of the people in the team.
227+It is also possible to view the set of mugshots of the people in the
228+team. Notice that the output of mugshots is batched.
229
230- >>> anon_browser.open('http://launchpad.dev/~ubuntu-team/+mugshots')
231- >>> print find_main_content(anon_browser.contents)
232+ >>> anon_browser.open('http://launchpad.dev/~ubuntu-team/')
233+ >>> anon_browser.getLink('Show member photos').click()
234+ >>> main_content = find_main_content(anon_browser.contents)
235+ >>> print main_content
236 <...
237- <h1>Who's in this team?</h1>
238- ...
239+ <h1>Member photos</h1>
240+ ...
241+ 1...10... of 10 results
242+ ...
243+
244
245 == Team admins ==
246
247
248=== modified file 'lib/lp/registry/templates/team-mugshots.pt'
249--- lib/lp/registry/templates/team-mugshots.pt 2009-09-14 19:32:10 +0000
250+++ lib/lp/registry/templates/team-mugshots.pt 2009-10-13 15:08:16 +0000
251@@ -11,17 +11,32 @@
252
253 <div metal:fill-slot="main">
254
255- <tal:per_member repeat="member view/allmembers">
256- <a tal:condition="member/mugshot"
257- tal:attributes="
258- href member/fmt:url;
259- title member/displayname">
260- <img tal:attributes="src member/mugshot/http_url"/>
261- </a>
262- <span style="white-space: nowrap;" tal:condition="not: member/mugshot">
263- [<a tal:content="member/displayname"
264- tal:attributes="href member/fmt:url">Foo Bar</a>]</span>
265- </tal:per_member>
266+ <div id="top-navigation" style="margin-bottom: 1em;">
267+ <tal:navigation content="structure view/members/@@+navigation-links-upper" />
268+ </div>
269+
270+ <div class="gridflow"
271+ tal:define="current_batch nocall:view/members/currentBatch">
272+
273+ <ul>
274+ <tal:per_member repeat="member current_batch">
275+ <li>
276+ <span>
277+ <a tal:attributes="href member/fmt:url;
278+ title member/displayname">
279+ <img tal:replace="structure member/image:mugshot" />
280+ <div tal:content="member/displayname" />
281+ </a>
282+ </span>
283+ </li>
284+ </tal:per_member>
285+ </ul>
286+
287+ </div>
288+
289+ <div>
290+ <tal:navigation content="structure view/members/@@+navigation-links-lower" />
291+ </div>
292
293 </div>
294 </body>