Code review comment for lp:~bac/launchpad/bug-436980-mugshots

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.

« Back to merge proposal