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
=== modified file 'configs/development/launchpad-lazr.conf'
--- configs/development/launchpad-lazr.conf 2009-09-29 15:15:21 +0000
+++ configs/development/launchpad-lazr.conf 2009-10-13 15:08:16 +0000
@@ -123,6 +123,8 @@
123default_batch_size: 5123default_batch_size: 5
124max_attachment_size: 2097152124max_attachment_size: 2097152
125branchlisting_batch_size: 6125branchlisting_batch_size: 6
126mugshot_batch_size: 8
127announcement_batch_size: 4
126openid_preauthorization_acl:128openid_preauthorization_acl:
127 localhost http://launchpad.dev/129 localhost http://launchpad.dev/
128max_bug_feed_cache_minutes: 30130max_bug_feed_cache_minutes: 30
129131
=== modified file 'lib/canonical/config/schema-lazr.conf'
--- lib/canonical/config/schema-lazr.conf 2009-10-01 12:06:50 +0000
+++ lib/canonical/config/schema-lazr.conf 2009-10-13 15:08:16 +0000
@@ -930,6 +930,12 @@
930# datatype: integer930# datatype: integer
931branchlisting_batch_size: 100931branchlisting_batch_size: 100
932932
933# The default size used in a batched display of team mugshots.
934mugshot_batch_size: 32
935
936# The default size used in a batched display of announcements.
937announcement_batch_size: 5
938
933# If restrict_to_team is set (such as on the beta939# If restrict_to_team is set (such as on the beta
934# website), then this indicates the hostname suffix for940# website), then this indicates the hostname suffix for
935# the non-restricted version of Launchpad. Replacing941# the non-restricted version of Launchpad. Replacing
936942
=== modified file 'lib/canonical/launchpad/icing/style-3-0.css'
--- lib/canonical/launchpad/icing/style-3-0.css 2009-10-02 13:56:25 +0000
+++ lib/canonical/launchpad/icing/style-3-0.css 2009-10-13 15:08:16 +0000
@@ -941,3 +941,25 @@
941 margin-top: 2em;941 margin-top: 2em;
942 clear: both;942 clear: both;
943}943}
944.gridflow {
945 margin: 0.0em;
946 padding: 0.0em;
947}
948.gridflow ul {
949 margin: 0.5em 0.0em 0.5em 0.0em;
950 padding: 0.5em 0.0em 0.0em 0.0em;
951}
952.gridflow li {
953 display: inline;
954 margin: 0.0em;
955 padding: 0.2em;
956}
957.gridflow li span {
958 display: table-cell;
959 display: inline-table;
960 display: inline-block;
961 width: 192px;
962 text-align: center;
963 vertical-align: middle;
964 padding: 1em 1em 1em 0;
965}
944966
=== modified file 'lib/lp/registry/browser/announcement.py'
--- lib/lp/registry/browser/announcement.py 2009-10-05 22:14:18 +0000
+++ lib/lp/registry/browser/announcement.py 2009-10-13 15:08:16 +0000
@@ -24,6 +24,7 @@
2424
25from lp.registry.interfaces.announcement import IAnnouncement25from lp.registry.interfaces.announcement import IAnnouncement
2626
27from canonical.config import config
27from canonical.launchpad import _28from canonical.launchpad import _
28from canonical.launchpad.browser.feeds import (29from canonical.launchpad.browser.feeds import (
29 AnnouncementsFeedLink, FeedsMixin, RootAnnouncementsFeedLink)30 AnnouncementsFeedLink, FeedsMixin, RootAnnouncementsFeedLink)
@@ -263,7 +264,7 @@
263 """A view class for pillars which have announcements."""264 """A view class for pillars which have announcements."""
264 implements(IAnnouncementCreateMenu)265 implements(IAnnouncementCreateMenu)
265266
266 batch_size = 5267 batch_size = config.launchpad.announcement_batch_size
267268
268 @cachedproperty269 @cachedproperty
269 def feed_url(self):270 def feed_url(self):
270271
=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py 2009-10-07 21:45:58 +0000
+++ lib/lp/registry/browser/person.py 2009-10-13 15:08:16 +0000
@@ -4662,16 +4662,20 @@
4662class TeamMugshotView(LaunchpadView):4662class TeamMugshotView(LaunchpadView):
4663 """A view for the team mugshot (team photo) page"""4663 """A view for the team mugshot (team photo) page"""
46644664
4665 label = "Who's in this team?"4665 label = "Member photos"
4666 batch_size = config.launchpad.mugshot_batch_size
46664667
4667 def initialize(self):4668 def initialize(self):
4668 """Cache images to avoid dying from a million cuts."""4669 """Cache images to avoid dying from a million cuts."""
4669 getUtility(IPersonSet).cacheBrandingForPeople(self.allmembers)4670 getUtility(IPersonSet).cacheBrandingForPeople(
46704671 self.members.currentBatch())
46714672
4672 @cachedproperty4673 @cachedproperty
4673 def allmembers(self):4674 def members(self):
4674 return list(self.context.allmembers)4675 """Get a batch of all members in the team."""
4676 batch_nav = BatchNavigator(
4677 self.context.allmembers, self.request, size=self.batch_size)
4678 return batch_nav
46754679
46764680
4677class TeamReassignmentView(ObjectReassignmentView):4681class TeamReassignmentView(ObjectReassignmentView):
46784682
=== modified file 'lib/lp/registry/browser/tests/announcement-views.txt'
--- lib/lp/registry/browser/tests/announcement-views.txt 2009-10-05 21:31:28 +0000
+++ lib/lp/registry/browser/tests/announcement-views.txt 2009-10-13 15:08:16 +0000
@@ -45,3 +45,22 @@
4545
46 >>> check_menu_links(AnnouncementCreateNavigationMenu(product))46 >>> check_menu_links(AnnouncementCreateNavigationMenu(product))
47 True47 True
48
49
50Batching
51--------
52
53Announcements are presented in batches. For launchpad.dev the batch size is
54smaller than in production to ease testing.
55
56 >>> from lp.registry.interfaces.announcement import IAnnouncementSet
57 >>> announcement_set = getUtility(IAnnouncementSet)
58 >>> view = create_initialized_view(
59 ... announcement_set, '+announcements')
60 >>> print len(list(view.announcements))
61 20
62 >>> print view.batch_size
63 4
64 >>> batch = view.announcement_nav.currentBatch()
65 >>> print len(list(batch))
66 4
4867
=== modified file 'lib/lp/registry/browser/tests/team-views.txt'
--- lib/lp/registry/browser/tests/team-views.txt 2009-08-24 14:26:34 +0000
+++ lib/lp/registry/browser/tests/team-views.txt 2009-10-13 15:08:16 +0000
@@ -229,3 +229,30 @@
229 Send an email to this team's owner through Launchpad229 Send an email to this team's owner through Launchpad
230 >>> print view.specific_contact_text230 >>> print view.specific_contact_text
231 Contact this team's owner231 Contact this team's owner
232
233
234== Mugshots ==
235
236The mugshots for all members of a team can be seen on the +mugshots
237page. The display of mugshots is batched.
238
239 >>> ubuntu = person_set.getByName('ubuntu-team')
240 >>> view = create_initialized_view(ubuntu, '+mugshots')
241 >>> batch = view.members.currentBatch()
242 >>> print len(list(ubuntu.allmembers))
243 10
244 >>> print view.batch_size
245 8
246 >>> print len(list(batch))
247 8
248 >>> from zope.security.proxy import removeSecurityProxy
249 >>> for person in list(batch):
250 ... print removeSecurityProxy(person)
251 <Person at ... limi (Alexander Limi)>
252 <Person at ... cprov (Celso Providelo)>
253 <Person at ... kamion (Colin Watson)>
254 <Person at ... kinnison (Daniel Silverstone)>
255 <Person at ... edgar (Edgar Bursic)>
256 <Person at ... name16 (Foo Bar)>
257 <Person at ... jdub (Jeff Waugh)>
258 <Person at ... mark (Mark Shuttleworth)>
232259
=== modified file 'lib/lp/registry/stories/announcements/xx-announcements.txt'
--- lib/lp/registry/stories/announcements/xx-announcements.txt 2009-10-05 21:35:18 +0000
+++ lib/lp/registry/stories/announcements/xx-announcements.txt 2009-10-13 15:08:16 +0000
@@ -450,12 +450,26 @@
450 >>> anon_browser.open('http://launchpad.dev/+announcements')450 >>> anon_browser.open('http://launchpad.dev/+announcements')
451 >>> 'Announcements from all projects' in anon_browser.title451 >>> 'Announcements from all projects' in anon_browser.title
452 True452 True
453 >>> 'Kubuntu announcement' in announcements(anon_browser.contents)
454 True
455 >>> 'RedHat announcement ' in announcements(anon_browser.contents)
456 True
457 >>> 'Derby announcement ' in announcements(anon_browser.contents)
458 True
459 >>> 'Apache announcement ' in announcements(anon_browser.contents)
460 True
461
462The announcements are batched so only the latest four are shown,
463leaving Tomcat out:
464
465 >>> print extract_text(anon_browser.contents)
466 Announcements from all projects hosted in Launchpad
467 ...
468 1...4 of 25 results
469 ...
470
453 >>> 'Tomcat announcement ' in announcements(anon_browser.contents)471 >>> 'Tomcat announcement ' in announcements(anon_browser.contents)
454 True472 False
455 >>> 'Apache announcement ' in announcements(anon_browser.contents)
456 True
457 >>> 'Kubuntu announcement' in announcements(anon_browser.contents)
458 True
459473
460It excludes future announcements too:474It excludes future announcements too:
461475
462476
=== modified file 'lib/lp/registry/stories/foaf/xx-team-home.txt'
--- lib/lp/registry/stories/foaf/xx-team-home.txt 2009-09-18 19:31:04 +0000
+++ lib/lp/registry/stories/foaf/xx-team-home.txt 2009-10-13 15:08:16 +0000
@@ -148,7 +148,7 @@
148 ... find_tag_by_id(sample_browser.contents, 'contact-user'))148 ... find_tag_by_id(sample_browser.contents, 'contact-user'))
149 Contact this team's members149 Contact this team's members
150150
151As teams do not have OpenID Logins, there is not link in the Contact151As teams do not have OpenID Logins, there is no link in the Contact
152details section for help.152details section for help.
153153
154 >>> sample_browser.getLink('(What\xe2\x80\x99s\xc2\xa0this?)')154 >>> sample_browser.getLink('(What\xe2\x80\x99s\xc2\xa0this?)')
@@ -166,13 +166,19 @@
166 You are an indirect member of this team:166 You are an indirect member of this team:
167 Sample Person &rarr; Warty Security Team &rarr; Ubuntu Gnome Team...167 Sample Person &rarr; Warty Security Team &rarr; Ubuntu Gnome Team...
168168
169It is also possible to view the set of mugshots of the people in the team.169It is also possible to view the set of mugshots of the people in the
170team. Notice that the output of mugshots is batched.
170171
171 >>> anon_browser.open('http://launchpad.dev/~ubuntu-team/+mugshots')172 >>> anon_browser.open('http://launchpad.dev/~ubuntu-team/')
172 >>> print find_main_content(anon_browser.contents)173 >>> anon_browser.getLink('Show member photos').click()
174 >>> main_content = find_main_content(anon_browser.contents)
175 >>> print main_content
173 <...176 <...
174 <h1>Who's in this team?</h1>177 <h1>Member photos</h1>
175 ...178 ...
179 1...10... of 10 results
180 ...
181
176182
177== Team admins ==183== Team admins ==
178184
179185
=== modified file 'lib/lp/registry/templates/team-mugshots.pt'
--- lib/lp/registry/templates/team-mugshots.pt 2009-09-14 19:32:10 +0000
+++ lib/lp/registry/templates/team-mugshots.pt 2009-10-13 15:08:16 +0000
@@ -11,17 +11,32 @@
1111
12<div metal:fill-slot="main">12<div metal:fill-slot="main">
1313
14 <tal:per_member repeat="member view/allmembers">14 <div id="top-navigation" style="margin-bottom: 1em;">
15 <a tal:condition="member/mugshot"15 <tal:navigation content="structure view/members/@@+navigation-links-upper" />
16 tal:attributes="16 </div>
17 href member/fmt:url;17
18 title member/displayname">18 <div class="gridflow"
19 <img tal:attributes="src member/mugshot/http_url"/>19 tal:define="current_batch nocall:view/members/currentBatch">
20 </a>20
21 <span style="white-space: nowrap;" tal:condition="not: member/mugshot">21 <ul>
22 [<a tal:content="member/displayname"22 <tal:per_member repeat="member current_batch">
23 tal:attributes="href member/fmt:url">Foo Bar</a>]</span>23 <li>
24 </tal:per_member>24 <span>
25 <a tal:attributes="href member/fmt:url;
26 title member/displayname">
27 <img tal:replace="structure member/image:mugshot" />
28 <div tal:content="member/displayname" />
29 </a>
30 </span>
31 </li>
32 </tal:per_member>
33 </ul>
34
35 </div>
36
37 <div>
38 <tal:navigation content="structure view/members/@@+navigation-links-lower" />
39 </div>
2540
26</div>41</div>
27</body>42</body>