Roadmap Progress bar errors when using custom Workflow

[anonymous]

In the 0.11 version, with custom configurable workflows, the Roadmap queries that create the counts for the status bars are not always correct. The Roadmap code is assuming you are using the default workflow. For example, it is assuming the only closed status is 'closed' (which isn't too bad of an assumption). Even worse, it is assuming the only active statuses are 'new', 'assigned', and 'reopened'. Actually, with the first assumption, the number of requests get calculated correctly, however the link does not necessarily show the same number. For example, I'm using a workflow with SEVERAL other statuses and thus when I click on the active link very few of my tickets actually show up.

Need a configuration screen such that you can tell the Roadmap module which statuses are considered Active versus which are considered Closed.

[Eli Carter]

Add another affected user to the CC list.

[Christian Boos]

#5602 was closed as duplicate. It contained a patch which hard-coded the list of "active" states, which is not OK, of course.

The DefaultTicketGroupStatsProvider should have a more generic behavior, configurable in the TracIni so that it's easy to configure at the same time as the workflow when using the ConfigurableTicketWorkflow.

One example corresponding to the basic-workflow.ini:

[milestone-groups]
closed = closed
closed.order = 0
closed.overall_completion = true

active = assigned,accepted
active.order = 1
active.css = open

new = new,reopened
new.order = 2

[Christian Boos]

Make the default ITicketGroupStatsProvider configurable. Default configuration is generic enough to accomodate all configurable workflows having at least a closed status (which they should have anyway).

[Christian Boos]

Updated patch, the previous one contained quite a number of bugs…

[Christian Boos]

I'd like to get some feedback on this before committing. Thanks!

[matthew.block@…]

The concept looks good though I haven't had time to test it. I'll try to test it out sometime this week and add another comment.

[Eli Carter]

Somewhat re-worked version of the previous patch

[Eli Carter]

Overall, I like this change. I do have a number of comments; some of these I changed in the diff I attached, the rest are still open…

• For the config, document the *.css default value.

• Changed "if ticket_ids > ticket_cnt / 4" to "if len(ticket_ids) > …" (Though the next bullet makes this irrelevant.)

• "# then it's probably faster to get the status for all tickets" I removed this optimization.

I really don't like this approach. The optimization path will only trigger when there are a lot of tickets on the milestone, and not many tickets in the database. In otherwords, it's an optimization for the small case. But in all cases we have to query the database for a total number of tickets just to check if we want to do the optimization.

For those with a large number of tickets in the database, this won't help, but we incur the cost of a query to check it.

• 'id' is going to shadow the builtin; changed to 'tid'

• The implementation of _get_ticket_groups() will silently fail if the config has 'something.whatever' before 'something'. Fix or document?

• Nothing prevents the user from setting up a set of intervals in a way that some tickets show up in more than one interval. Do we need to do something about that?

• 3 unittests error with the patch: test_closed_interval, test_open_interval, test_stats. Looks like the stub environment doesn't have a self.config.

[Christian Boos]

Thanks for the review and fixes.

Replying to ecarter:

Overall, I like this change. I do have a number of comments; some of these I changed in the diff I attached, the rest are still open…

• For the config, document the *.css default value.

The place for documenting this is not obvious… we used to have a way to document configuration sections, maybe I should revive this in some form. That would be useful for the [ticket-workflow] section as well.

• "# then it's probably faster to get the status for all tickets" I removed this optimization.

(snip)

OK, good points. I was worried with having to build lots of queries with a potentially large in (...) part, but that was already the case before. One day this could become a problem and we'll probably have to propagate and somehow reuse the explicit query for each group instead of the list of ticket ids.

• The implementation of _get_ticket_groups() will silently fail if the config has 'something.whatever' before 'something'. Fix or document?

Document and complain if misconfigured.

• Nothing prevents the user from setting up a set of intervals in a way that some tickets show up in more than one interval. Do we need to do something about that?

Yes, I think it would be good if each group "consumes" some of the available status, and an error should be raised if a group mistakenly accepts a status already taken.

• 3 unittests error with the patch: test_closed_interval, test_open_interval, test_stats. Looks like the stub environment doesn't have a self.config.

ok, I'll have a look.

[djc@…]

I have created a trac-hack that does what you are asking.

​http://trac-hacks.org/wiki/CustomRoadmapPlugin

Unfortunately you need to apply a small patch to make it work

​http://trac-hacks.org/browser/customroadmapplugin/0.11/trac-0.11dev.patch

[Christian Boos]

Latest version of the patch

[Christian Boos]

Ok, above points implemented in the latest iteration of the patch. I'll commit that if no big concern is raised.

[djc@…]

The only problem I have with the patch is that it relies on extra style being added to trac/htdocs/css/roadmap.css. So while you have made the basic workflow look better in the progress bars there is still no way to include extra divisions in the progress bars if you configure a custom workflow.

I ended up in a situation in my plugin where I created a new request path (/customroadmap/roadmap.css) just so I could serve up a Genshi generated stylesheet.

Rather than making modifications to trac/htdocs/css/roadmap.css or using Genshi to dynamically generate a stylesheet, it would be better to be able to configure Trac to place <head> links to extra content based on the request path, then have that content served from the project environment.

[Christian Boos]

Replying to djc@object-craft.com.au:

The only problem I have with the patch is that it relies on extra style being added to trac/htdocs/css/roadmap.css.

No, the extra style can be added anywhere, typically it would have to be done in a style.css file linked from the site.html template. See TracInterfaceCustomization#Trac0.11andabove.

So basically when you configure a custom workflow:

• you'll need to define the workflow in [ticket-workflow] in TracIni
• you can eventually enhance the default grouping of ticket status in milestone progress bar (this is optional as the new default handles well closed vs. non-closed):
  • edit [milestone-groups] in TracIni, and make use of the .css qualifier
  • edit your local CSS file to define the classes used, besides the default new, open and closed ones. Of course, if this is the first CSS tweak you're making, then you need to setup the site.html first as explained above.

[Eli Carter]

I fixed #5602 using a minimal change to trunk so the regression part of this is fixed. Given that, I'm bumping this to 0.11.1; if we get this completed sooner, that would be great too. I'll re-diff the latest patch for this ticket.

[Eli Carter]

rediff after minimalist fix for #5602

[regis@…]

Hi,

I've been testing this patch since yesterday, and I keep having the "milestone group qualified before being defined" error with it, even with a copy/past of the [milestone-groups] from basic-workflow.ini. Also, If if disable the test returning the error and set 4 groups without .order option (closed, coded, assigned, new), they appear in the order assigned, coded, closed, new.

It looks to me like the call to self.config.options() doesn't return the options in their original order.

For information, I'm running r5892 on Windows with Python 2.5.1

[Christian Boos]

Replying to regis@boudin.name:

Hi,

I've been testing this patch since yesterday, and I keep having the "milestone group qualified before being defined" error with it … It looks to me like the call to self.config.options() doesn't return the options in their original order.

Thanks for the testing. I forgot that there's indeed no guarantee about the ordering of options. I'll look into fixing the patch.

[Christian Boos]

updated patch, don't depend on the order of entries in trac.in and a few other fixes

[Christian Boos]

Some additional testing of this new version would be great.

[Christian Boos]

Ok, if nobody speaks up against the last patch, I'll commit that tomorrow.

[Noah Kantrowitz]

I really think this is too big to go in core. It should be its own plugin.

[Christian Boos]

New version of the patch, taking cmlenz review into account

[Christian Boos]

Slightly improved patch committed as r6011.