Hi James,

I think this looks good but the new functions/methods would benefit from
some docstrings.  I also have a few comments below.

 review approve

On Sat, 2011-02-26 at 01:02 +0000, James Westby wrote:
[...]
> === modified file 'generate-all'
> --- generate-all	2011-02-06 20:00:12 +0000
> +++ generate-all	2011-02-26 01:01:57 +0000
> @@ -133,6 +133,11 @@
>  basename = os.path.join(opts.output_dir, "about")
>  report_tools.about_page(my_path, opts.database, basename, opts.config, root=opts.root)
>  
> +# workitems in status pages
> +for status in report_tools.valid_states:
> +    basename = os.path.join(opts.output_dir, status)
> +    report_tools.workitem_list(my_path, opts.database, basename, opts.config, status, root=opts.root)
> +
>  # front page
>  basename = os.path.join(opts.output_dir, 'index')
>  report_tools.status_overview(my_path, opts.database, basename, opts.config, root=opts.root)
> 
> === modified file 'html-report'
> --- html-report	2011-02-25 02:56:48 +0000
> +++ html-report	2011-02-26 01:01:57 +0000
> @@ -47,6 +47,7 @@
>          self.priority = priority
>          self.status = status
>  
> +
>  class Assignee(WorkitemTarget):
>  
>      def __init__(self, name, url, complexity=0, todo_wis=[],
> @@ -90,6 +91,19 @@
>          return any([g.priority for g in self.groups])
>  
> 
> +class Workitem(object):
> +
> +    def __init__(self, description, status, blueprint, assignee=None):
> +        self.description = description
> +        self.status = status
> +        self.blueprint = blueprint
> +        self._assignee = assignee
> +
> +    @property
> +    def assignee(self):
> +        return self._assignee or self.blueprint.assignee
> +
> +
>  def spec_group_completion(db, team, milestone):
>      data = report_tools.spec_group_completion(db, team, milestone)
>      if not data:
> @@ -286,6 +300,21 @@
>      return group
>  
> 
> +def workitems_in_status(db, status, team=None, milestone=None):
> +    data = report_tools.assignee_completion(
> +        db, team=team, milestone=milestone)
> +    workitems = []
> +    for assignee in data:
> +        if status in data[assignee]:
> +            for wi_info in data[assignee][status]:
> +                bp_name, description, priority, url = wi_info
> +                blueprint = Blueprint(bp_name, url, priority=priority)
> +                workitems.append(
> +                    Workitem(description, status, blueprint,
> +                        assignee=assignee))
> +    return workitems
> +
> +
>  def list_people(db, team=None):
>      team_info = report_tools.team_information(db, team=team)
>      if team is None:
> @@ -398,6 +427,17 @@
>          data.update(dict(page_type="about"))
>          print report_tools.fill_template("about.html", data)
>  
> +    def workitem_list(self, db, opts):
> +        assert opts.status is not None, (
> +            "Must pass --status for workitem_list report-type")
> +        workitems = workitems_in_status(
> +            db, opts.status, team=opts.team, milestone=opts.milestone)
> +        data = self.template_data(db, opts)
> +        data.update(dict(status=opts.status))
> +        data.update(dict(workitems=workitems))
> +        data.update(dict(page_type="overview"))

Why 3 update() calls instead of just one passing a dict combining
everything?

> +        print report_tools.fill_template("workitem_list.html", data)
> +
>  
>  #
>  # main
> @@ -426,6 +466,8 @@
>              help="Select the group for a group_overview report.")
>      optparser.add_option('--root', dest="root",
>              help="Root URL for the charts")
> +    optparser.add_option('--status', dest="status",
> +            help="Workitem status to consider for workitem_list report-type")
>  
>      (opts, args) = optparser.parse_args()
>      if not opts.database:
> 
> === modified file 'report_tools.py'
> --- report_tools.py	2011-02-24 01:33:40 +0000
> +++ report_tools.py	2011-02-26 01:01:57 +0000
> @@ -76,6 +76,25 @@
>      proc.wait()
>  
> 
> +def workitem_list(my_path, database, basename, config, status, root=None):
> +    cfg = load_config(config)
> +    team = cfg.get("primary_team", None)
> +    fh = open(basename + '.html', 'w')
> +    chartname = os.path.basename(basename)
> +    try:
> +        args = [os.path.join(my_path, 'html-report'), '-d', database, '--chart', '%s.svg' % chartname]
> +        args += ['--report-type', 'group_overview']
> +        args += ['--status', status]
> +        if root:
> +            args += ['--root', root]
> +        report_args(args, team=team)
> +        proc = Popen(args, stdout=fh)
> +        print basename + '.html'
> +        proc.wait()
> +    finally:
> +        fh.close()
> +
> +
>  def team_list_page(my_path, database, basename, config, root=None):
>      cfg = load_config(config)
>      team = cfg.get("primary_team", None)
> 
> === modified file 'templates/base.html'
> --- templates/base.html	2011-02-25 23:03:45 +0000
> +++ templates/base.html	2011-02-26 01:01:57 +0000
> @@ -19,11 +19,11 @@
>      td.priority_Essential, th.priority_Essential { color: red; }
>      td span.implementation_status { font-size: 70%; }
>  
> -    .status-todo {color: orange;}
> -    .status-inprogress {color: gray;}
> -    .status-done {color: green;}
> -    .status-blocked {color: red;}
> -    .status-postponed {color: purple;}
> +    .status-todo a {color: orange;}
> +    .status-inprogress a {color: gray;}
> +    .status-done a {color: green;}
> +    .status-blocked a {color: red;}
> +    .status-postponed a {color: purple;}
>  
>      .interesting {
>          font-weight: bold;
> @@ -340,6 +340,13 @@
>              }
>          });
>  
> +        $(".workitems_in_status").tablesorter({
> +            sortList: [[1,1],[0,0],[2,0]],

Maybe a comment explaining these magic numbers?

> +                headers: {
> +                    1: { sorter:'priority' }
> +                }
> +        });
> +
>          // with the 'todo/done' and assignee not appearing on every
>          // row, this does not lend itself well to sorting
>          //$("#byworkitem").tablesorter();
> 
> === modified file 'templates/overview.html'
> --- templates/overview.html	2011-02-16 21:53:06 +0000
> +++ templates/overview.html	2011-02-26 01:01:57 +0000
> @@ -44,26 +44,12 @@
>        <th>Count</th>
>    </tr>
>  </thead>
> -<tr class="status-todo">
> -    <td>Todo</td>
> -    <td>${all_workitems.todo}</td>
> -</tr>
> -<tr class="status-blocked">
> -    <td>Blocked</td>
> -    <td>${all_workitems.blocked}</td>
> -</tr>
> -<tr class="status-inprogress">
> -    <td>In Progress</td>
> -    <td>${all_workitems.inprogress}</td>
> -</tr>
> -<tr class="status-done">
> -    <td>Done</td>
> -    <td>${all_workitems.done}</td>
> -</tr>
> -<tr class="status-postponed">
> -    <td>Postponed</td>
> -    <td>${all_workitems.postponed}</td>
> -</tr>
> +% for status, label in [('todo', 'Todo'), ('blocked', 'Blocked'), ('inprogress', 'In Progress'), ('done', 'Done'), ('postponed', 'Postponed')]:
> +<tr class="status-${status}">
> +    <td><a href="${base.url(status + '.html')}">${label}</a></td>
> +    <td><a href="${base.url(status + '.html')}">${getattr(all_workitems,status)}</a></td>
> +</tr>
> +% endfor

To make this even nicer you could add something like a
valid_states_with_labels to report_tools (by zip()ping a list containing
the lables with the existing valid_states there). :)

>  </table>
>  <%def name="workitems_per_day(workitems, days)">
>  % if days < 1:
> 
> === added file 'templates/workitem_list.html'
> --- templates/workitem_list.html	1970-01-01 00:00:00 +0000
> +++ templates/workitem_list.html	2011-02-26 01:01:57 +0000
> @@ -0,0 +1,29 @@
> +<%inherit file="base.html"/>
> +<%namespace name="base" file="base.html"/>
> +
> +<%def name="title()">
> +Work items in ${status} status
> +</%def>
> +
> +<h1>Work items in ${status} status</h1>
> +
> +<p>This page shows the full list of workitems in a particular status. This can be useful to see a list of workitems that are blocked or postponed for instance.</p>
> +
> +<table class="workitems_in_status">
> +  <thead>
> +    <tr>
> +        <th>Blueprint</th>
> +        <th>Priority</th>
> +        <th>Assignee</th>
> +        <th>Description</th>
> +    </tr>
> +  </thead>
> +% for workitem in workitems:
> +    <tr>
> +        <td><a href="{workitem.blueprint.url}">${workitem.blueprint.name}</a></td>
> +        <td class="priority_${workitem.blueprint.priority}">${workitem.blueprint.priority}</td>
> +        <td>${base.real_name(workitem.assignee)}</td>
> +        <td>${workitem.description}</td>
> +    </tr>
> +% endfor
> +</table>
> 

-- 
Guilherme Salgado <https://launchpad.net/~salgado>