Code review comment for lp:~james-w/launchpad-work-items-tracker/workitem-list-pages

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>