Edgewall Software
Modify

Opened 15 years ago

Closed 15 years ago

#5572 closed defect (fixed)

Roadmap Progress bar errors when using custom Workflow

Reported by: anonymous Owned by: Christian Boos
Priority: high Milestone: 0.11
Component: roadmap Version: devel
Severity: normal Keywords: roadmap workflow
Cc: matthew.block@…, Otavio Salvador, Eli Carter, regis@… Branch:
Release Notes:
API Changes:
Internal Changes:

Description

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.

Attachments (7)

milestone_groups-r5758.diff (8.0 KB ) - added by Christian Boos 15 years ago.
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).
milestone_groups-r5762.diff (9.2 KB ) - added by Christian Boos 15 years ago.
Updated patch, the previous one contained quite a number of bugs…
milestone_groups-r5765.diff (8.8 KB ) - added by Eli Carter 15 years ago.
Somewhat re-worked version of the previous patch
milestone_groups-r5786.diff (13.2 KB ) - added by Christian Boos 15 years ago.
Latest version of the patch
milestone_groups-r5842.diff (13.1 KB ) - added by Eli Carter 15 years ago.
rediff after minimalist fix for #5602
milestone_groups-r5946.diff (13.3 KB ) - added by Christian Boos 15 years ago.
updated patch, don't depend on the order of entries in trac.in and a few other fixes
milestone_groups-r6000.diff (14.2 KB ) - added by Christian Boos 15 years ago.
New version of the patch, taking cmlenz review into account

Download all attachments as: .zip

Change History (27)

comment:1 by anonymous, 15 years ago

Cc: matthew.block@… added

comment:2 by Eli Carter, 15 years ago

Owner: changed from Christopher Lenz to Eli Carter
Status: newassigned

comment:3 by Eli Carter, 15 years ago

Cc: Otavio Salvador added

Add another affected user to the CC list.

comment:4 by Christian Boos, 15 years ago

Milestone: 0.11
Owner: changed from Eli Carter to Christian Boos
Status: assignednew

#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

by Christian Boos, 15 years ago

Attachment: milestone_groups-r5758.diff added

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).

by Christian Boos, 15 years ago

Attachment: milestone_groups-r5762.diff added

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

comment:5 by Christian Boos, 15 years ago

Keywords: review added

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

comment:6 by matthew.block@…, 15 years ago

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.

by Eli Carter, 15 years ago

Attachment: milestone_groups-r5765.diff added

Somewhat re-worked version of the previous patch

comment:7 by Eli Carter, 15 years ago

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.

comment:8 by Eli Carter, 15 years ago

Cc: Eli Carter added

comment:9 by Christian Boos, 15 years ago

Milestone: 0.11.10.11

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.

comment:10 by djc@…, 15 years ago

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

by Christian Boos, 15 years ago

Attachment: milestone_groups-r5786.diff added

Latest version of the patch

in reply to:  9 comment:11 by Christian Boos, 15 years ago

Status: newassigned

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

comment:12 by djc@…, 15 years ago

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.

in reply to:  12 comment:13 by Christian Boos, 15 years ago

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.

comment:14 by Eli Carter, 15 years ago

Milestone: 0.110.11.1

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.

by Eli Carter, 15 years ago

Attachment: milestone_groups-r5842.diff added

rediff after minimalist fix for #5602

comment:15 by regis@…, 15 years ago

Cc: regis@… added

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

in reply to:  15 comment:16 by Christian Boos, 15 years ago

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.

by Christian Boos, 15 years ago

Attachment: milestone_groups-r5946.diff added

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

comment:17 by Christian Boos, 15 years ago

Some additional testing of this new version would be great.

comment:18 by Christian Boos, 15 years ago

Milestone: 0.11.10.11

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

comment:19 by Noah Kantrowitz, 15 years ago

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

by Christian Boos, 15 years ago

Attachment: milestone_groups-r6000.diff added

New version of the patch, taking cmlenz review into account

comment:20 by Christian Boos, 15 years ago

Keywords: review removed
Resolution: fixed
Status: assignedclosed

Slightly improved patch committed as r6011.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Christian Boos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.