On Aug 19, 2009, at 07:30 PM, Curtis Hovey wrote: >This looks good. I see have a small concern about one menu and I think >some of the links need to be handled differently. Okay. Hopefully we're close! >>These look like the menus we want to build. >> > >> >{{{ >> > >> >class PersonSetNavigationMenu(NavigationMenu): >> > """Navigation menu for PersonSet actions""" >> > usedfor = IPersonSet >> > links = [ >> > register_team','adminpeoplemerge', 'adminteammerge', 'mergeaccounts'] >> > >> > >> >class IRegistryCollectionNavigationMenu(Interface): >> > """Marker interface for the registry collection navigation menu.""" >> > >> > >> >class RegistryCollectionNavigationMenu(NavigationMenu, TopLevelMenuMixin): >> > """Navigation menu for registry person search collection.""" >> > usedfor = IRegistryCollectionNavigationMenu >> > facet = 'overview' >> > links = ['products', 'distributions', 'people', 'meetings'] >> > >> > >> >class PeopleSearchView(LaunchpadView): >> > """Search for people and teams on the /people page.""" >> > implements(IRegistryCollectionNavigationMenu) >> > >> >}}} >> >> Nice. This works but with a small ugliness. Because `usedfor` in the action >> menu must correspond to the context object's interface, it can't be completely >> generalized. It will force each top level collection view to create a >> subclass containing only the `usedfor` attribute, set to the right interface. >> Icky, but close enough for jazz. >> >No. I do not see that. We created the one shared menu (RegistryCollectionNavigationMenu). All the views (that already exist) just need to add the implements. > >I am not sure of the value of RegistryCollectionActionMenuBase. I do not think those links are usable between collections. Those links belong to the IPersonSet only. You might as well move those links to the one true implementing class PersonSetNavigationMenu I'm not so sure about that. Certainly, the admin_merge_* links pertain only to IPersonSet, but that's easily controlled by setting the 'links' attribute. In fact, that's exactly the refactoring I've done in my /projects branch, so I don't want to do it here. But I do think the register_projects and register_teams link make sense on all the top level collections, and I don't think we need to filter that further for the context. Also, create_account when anonymous makes sense on all the collections, so I think that justifies RegistryCollectionActionMenuBase. > >Doing this does feel like a hack because it would be better to register both >> menus on the view instead of one on the view and one on the context object. >> But it's a hack that will live another day because I'm not going to worry >> about it any more. :) >> >Creating a base that has only one user is a waste, remove it. It won't be a waste when you see the /projects branch. >> === modified file 'lib/lp/registry/browser/menu.py' >> --- lib/lp/registry/browser/menu.py 2009-08-18 13:54:51 +0000 >> +++ lib/lp/registry/browser/menu.py 2009-08-18 23:27:23 +0000 > >... > >> @@ -27,14 +34,72 @@ >> def meetings(self): >> return Link('/sprints/', 'View meetings', icon='info') >> >> + def project_groups(self): >> + return Link('/projectgroups', 'View project groups', icon='info') >> + >> def register_project(self): >> text = 'Register a project' >> - return Link('/projects/+new', text, icon='add') >> + enabled = self.user is not None >> + return Link('/projects/+new', text, icon='add', enabled=enabled) > >I do not think we want to do this. An anonymous user should see this link. >he will be prompted to login/register as needed Seems like a tease, or maybe misdirection, but okay, I've removed this and let the register_* links appear for everyone. > >> def register_team(self): >> text = 'Register a team' >> - return Link('/people/+newteam', text, icon='add') >> + enabled = self.user is not None >> + return Link('/people/+newteam', text, icon='add', enabled=enabled) > >Sam as register_project. Same as above. > >> def create_account(self): >> text = 'Create an account' >> - return Link('/people/+login', text, icon='add') >> + # Only enable this link for anonymous users. >> + enabled = self.user is None >> + return Link('/people/+login', text, icon='add', enabled=enabled) >> + >> + def request_merge(self): >> + text = 'Request a merge' >> + enabled = self.user is not None >> + return Link('/people/+requestmerge', text, icon='edit', >> + enabled=enabled) >> + >> + def admin_merge_people(self): >> + text = 'Merge people' >> + enabled = (self.user is not None and >> + self.user.inTeam(getUtility(ILaunchpadCelebrities).admin)) >> + return Link('/people/+adminpeoplemerge', text, icon='edit', >> + enabled=enabled) >> + >> + def admin_merge_teams(self): >> + text = 'Merge teams' >> + enabled = (self.user is not None and >> + self.user.inTeam(getUtility(ILaunchpadCelebrities).admin)) >> + return Link('/people/+adminteammerge', text, icon='edit', >> + enabled=enabled) > >These 'admin' links are bad. This circumvents permissions. If you look at >other menus, the links use this decorator > > @enabled_with_permission('launchpad.Admin') > def admin_merge_people(self): > >and > > @enabled_with_permission('launchpad.Admin') > def admin_merge_teams(self): > >This leads me to ask. Did we ever develop a permission for a logged in user? > >/me looks in security.py. > >Yes, this /should/ work. > > @enabled_with_permission('launchpad.View') > def create_account(self): This one isn't right because we only want to show this link with an anonymous user. If they have launchpad.View permission, they're logged in. We don't have an @enabled_for_anonymous() decorator that I can see and I'm not sure it's worth adding one for this particular link. > > @enabled_with_permission('launchpad.View') > def request_merge(self): > >OMG! We do not have permissions on IPersonSet, or the other collections. My >suggestion may not work. Oh, I see that 'launchpad.Admin' is on Interface >by default. This /should/ work. It does! Changed. > >> +class IRegistryCollectionNavigationMenu(Interface): >> + """Marker interface for top level registry collection navigation menu.""" >> + >> + >> +class RegistryCollectionNavigationMenu(NavigationMenu, TopLevelMenuMixin): >> + """Navigation menu for top level registry collections.""" >> + >> + usedfor = IRegistryCollectionNavigationMenu >> + facet = 'overview' >> + >> + links = [ >> + 'projects', >> + 'project_groups', >> + 'distributions', >> + 'people', >> + 'meetings', >> + ] >> + >> + >> +class RegistryCollectionActionMenuBase(NavigationMenu, TopLevelMenuMixin): >> + """Action menu for top level registry collections. >> + >> + Because of the way menus work, you need to subclass this menu class and >> + set the `usedfor` attribute on the subclass. `usedfor` should point to >> + the interface of the context object, so we can't do that for you. >> + """ >> + facet = 'overview' >> + links = ['register_team', 'register_project', 'create_account', >> + 'request_merge', 'admin_merge_people', 'admin_merge_teams'] > > >I do not think this is necessary. The action menu links I see will only >every be used by the IPersonSet Menu. PersonSetActionNavigationMenu should >do the mixin, and facet, and links. See above. I think it's useful to keep. > > >> === modified file 'lib/lp/registry/browser/person.py' >> --- lib/lp/registry/browser/person.py 2009-08-18 15:28:28 +0000 >> +++ lib/lp/registry/browser/person.py 2009-08-18 23:27:23 +0000 > >... > >+class PersonSetNavigationMenu(RegistryCollectionActionMenuBase): >+ """Action menu for `PeopleSearchView`.""" >+ usedfor = IPersonSet > >The name is ambiguous and as I said above, I do not see a need for the base. >This menu is not registered on the view. I think this is the menu that all >view of IPersonSet will use. s/PersonSetNavigationMenu/PersonSetActionNavigationMenu/ > >{{{ > >class PersonSetActionNavigationMenu(NavigationMenu, TopLevelMenuMixin): > """Action menu for `IPersonSet`.""" > facet = 'overview' > usedfor = IPersonSet > links = ['register_team', 'register_project', 'create_account', > 'request_merge', 'admin_merge_people', 'admin_merge_teams'] > >}}} > Here's an incremental diff. === modified file 'lib/lp/registry/browser/configure.zcml' --- lib/lp/registry/browser/configure.zcml 2009-08-19 17:17:21 +0000 +++ lib/lp/registry/browser/configure.zcml 2009-08-19 21:09:36 +0000 @@ -783,7 +783,7 @@ TeamOverviewMenu TeamOverviewNavigationMenu TeamSpecsMenu - PersonSetNavigationMenu + PersonSetActionNavigationMenu "/>