Code review comment for lp:~michael.nelson/launchpad/sprint-index-and-attend-3.0

Hi Michael,

Thanks for working on this branch. I'm psyched to see us nailing down these
blueprint pages as we finish up 3.0.

As mentioned in irc, sprint-index doesn't look quite right because of the
breadcrumbs that shouldn't be there. I have a hack, er, solution for that
attached below. It basically puts a null +hierarchy view on the page. We
might want to consider generalizing that or providing an easier way to do it.
If you want to open a bug asking for that, I might be able to fix it
post-3.0. For now, this hack will work.

First, let's look at the code. I'll omit the stuff that looks fine.

=== modified file 'lib/lp/blueprints/browser/sprint.py'
--- lib/lp/blueprints/browser/sprint.py 2009-09-16 18:18:49 +0000
+++ lib/lp/blueprints/browser/sprint.py 2009-09-18 10:49:12 +0000
> @@ -69,7 +70,7 @@
> usedfor = ISprint
>
>
> -class SprintOverviewMenu(ApplicationMenu):
> +class SprintOverviewMenu(NavigationMenu):

While you're here can you give this class a docstring?

=== modified file 'lib/lp/blueprints/stories/sprints/05-sprint-creation.txt'
--- lib/lp/blueprints/stories/sprints/05-sprint-creation.txt 2009-09-15 10:26:04 +0000
+++ lib/lp/blueprints/stories/sprints/05-sprint-creation.txt 2009-09-18 11:31:27 +0000
> @@ -80,14 +80,9 @@
> Since the sprint's time zone was set to UTC, the dates are displayed in
> that time zone:
>
> - >>> print find_portlet(user_browser.contents, 'Meeting details')
> - <...
> - ...Starts:...
> - 2006-10-10 09:15 UTC...
> - ...Ends:...
> - 2006-10-13 16:00 UTC...
> - ...
> -
> + >>> print extract_text(find_tag_by_id(user_browser.contents, 'start-end'))
> + Starts: 09:15 UTC on Tuesday, 2006-10-10
> + Ends: 16:00 UTC on Friday, 2006-10-13

Thanks for cleaning these up!

=== modified file 'lib/lp/blueprints/stories/sprints/10-sprint-editing.txt'
--- lib/lp/blueprints/stories/sprints/10-sprint-editing.txt 2009-09-16 17:32:51 +0000
+++ lib/lp/blueprints/stories/sprints/10-sprint-editing.txt 2009-09-18 10:49:12 +0000
> @@ -2,8 +2,8 @@
> available to those who have permissions to edit that sprint.
>
> >>> anon_browser.open('http://launchpad.dev/sprints/ubz')
> - >>> anon_browser.title
> - 'Ubuntu Below Zero (sprint or meeting)'
> + >>> print anon_browser.title
> + Ubuntu Below Zero : Meetings

And these. If you have the inclination, can you indent the interactive
prompts 4 spaces?

=== modified file 'lib/lp/blueprints/stories/sprints/sprint-settopics.txt'
--- lib/lp/blueprints/stories/sprints/sprint-settopics.txt 2009-08-13 19:03:36 +0000
+++ lib/lp/blueprints/stories/sprints/sprint-settopics.txt 2009-09-18 10:49:12 +0000
> @@ -55,8 +55,8 @@
> 'http://launchpad.dev/sprints/uds-guacamole'
>
> >>> ut = 'Ubuntu Team'
> - >>> meeting_portlet = find_portlet(browser.contents, 'Meeting details')
> - >>> ut in extract_text(meeting_portlet.find(text='Driver:').findNext('a'))
> + >>> meeting_drivers = find_tag_by_id(browser.contents, 'meeting-drivers')
> + >>> ut in extract_text(meeting_drivers.findNext('a'))
> True

Is it possible to just print the text and use ellipses to omit anything you
don't care about? That would be better for documentation/testing purposes
than just printing True.

Here's a diff that will fix the breadcrumbs. It might need a little cleaning
up, but should give you the idea.

With consideration of these comments, merge-conditional, r=me.

=== modified file 'lib/lp/blueprints/browser/configure.zcml'
--- lib/lp/blueprints/browser/configure.zcml 2009-09-17 21:20:14 +0000
+++ lib/lp/blueprints/browser/configure.zcml 2009-09-18 15:54:43 +0000
@@ -23,6 +23,13 @@
         for="lp.blueprints.interfaces.sprint.ISprint"
         path_expression="name"
         parent_utility="lp.blueprints.interfaces.sprint.ISprintSet"/>
+ <browser:page
+ for="lp.blueprints.interfaces.sprint.ISprint"
+ name="+hierarchy"
+ class="lp.blueprints.browser.sprint.SprintIndexHierarchy"
+ template="../../app/templates/launchpad-hierarchy.pt"
+ permission="zope.Public"
+ />
     <browser:pages
         for="lp.blueprints.interfaces.sprint.ISprint"
         class="lp.blueprints.browser.sprint.SprintView"

=== modified file 'lib/lp/blueprints/browser/sprint.py'
--- lib/lp/blueprints/browser/sprint.py 2009-09-18 10:49:12 +0000
+++ lib/lp/blueprints/browser/sprint.py 2009-09-18 15:56:04 +0000
@@ -10,6 +10,7 @@
     'SprintBrandingView',
     'SprintEditView',
     'SprintFacets',
+ 'SprintIndexHierarchy',
     'SprintMeetingExportView',
     'SprintNavigation',
     'SprintOverviewMenu',
@@ -47,6 +48,7 @@
     LaunchpadFormView, LaunchpadView, Link, Navigation, NavigationMenu,
     StandardLaunchpadFacets, action, canonical_url, custom_widget,
     enabled_with_permission)
+from canonical.launchpad.browser.launchpad import Hierarchy
 from canonical.launchpad.webapp.batching import BatchNavigator
 from canonical.launchpad.webapp.breadcrumb import Breadcrumb
 from canonical.launchpad.helpers import shortlist
@@ -151,12 +153,20 @@
     enable_only = ['overview', ]

+class SprintIndexHierarchy(Hierarchy):
+ @property
+ def display_breadcrumbs(self):
+ return False
+
+
 class SprintView(HasSpecificationsView, LaunchpadView):

     __used_for__ = ISprint

     implements(IMajorHeadingView)

+ label = 'Meeting overview'
+
     def initialize(self):
         self.notices = []
         self.latest_specs_limit = 5

=== modified file 'lib/lp/blueprints/templates/sprint-index.pt'
--- lib/lp/blueprints/templates/sprint-index.pt 2009-09-18 10:49:12 +0000
+++ lib/lp/blueprints/templates/sprint-index.pt 2009-09-18 15:58:21 +0000
@@ -22,8 +22,6 @@
                  has_any_specs view/has_any_specifications">

   <div class="top-portlet">
- <h2>Meeting overview</h2>
-
     <p>
       <span tal:replace="context/summary">
         Sprint Summary Goes Here