Code review comment for lp:~bac/launchpad/bug-429455-team-pages

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)

« Back to merge proposal