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

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/