Setting different completion ratios on different ticket states in [milestone-groups]

[tjorvi@…]

It would make sense to be able to set [milestone-groups] ticketstate.overall_completion to a percentage rather than a boolean.

I have set up a ticket workflow that goes like this: open -> programmed -> reviewed -> tested (closed). I want to assign completion ratios something like this:

• programmed - 40%
• reviewed - 20%
• tested - 40%

That in effect means that if all tickets were programmed, but none open, reviewed or tested, the overall completion would stand at 40%.

[nkantrowitz]

Duplicate of #1534. Should be a plugin as well.

[anonymous]

I don't belive this is really a duplicate of #1534. #1534 talks about assigning completion ratios to individual tickets, while I mean't to assign pre-set completion ratios to ticket states.

You're right that this could be a plugin, but this plugin would be almost identical to the DefaultTicketGroupStatsProvider?.

I have modified DefaultTicketGroupStatsProvider? to do this on my setup, by modifying two lines:

Index: trac/ticket/roadmap.py =================================================================== --- trac/ticket/roadmap.py (revision 6080) +++ trac/ticket/roadmap.py (working copy) @@ -105,7 +105,7 @@

float(self.count) * 100))

total_percent = total_percent + intervalpercent? if intervaloverall_completion?:

- self.done_percent += intervalpercent? + self.done_percent += intervalpercent? * float(interval['over all_completion'])

self.done_count += intervalcount?

# We want the percentages to add up to 100%. To do that, we fudge the

@@ -240,7 +240,7 @@

query_args[k] = v

stat.add_interval(groupname?, group_cnt, query_args,

group.get('css_class', groupname?),

- bool(group.get('overall_completion'))) + group.get('overall_completion'))

stat.refresh_calcs() return stat

[cboos]

Replying to anonymous:

Index: trac/ticket/roadmap.py
===================================================================
--- trac/ticket/roadmap.py      (revision 6080)
+++ trac/ticket/roadmap.py      (working copy)
@@ -105,7 +105,7 @@
                                         float(self.count) * 100))
             total_percent = total_percent + interval['percent']
             if interval['overall_completion']:
-                self.done_percent += interval['percent']
+                self.done_percent += interval['percent'] * float(interval['over
all_completion'])
                 self.done_count += interval['count']

         # We want the percentages to add up to 100%.  To do that, we fudge the
@@ -240,7 +240,7 @@
                 query_args[k] = v
             stat.add_interval(group['name'], group_cnt, query_args,
                               group.get('css_class', group['name']),
-                              bool(group.get('overall_completion')))
+                              group.get('overall_completion'))
         stat.refresh_calcs()
         return stat

Fixing the patch presentation. Well, I have yet to try it myself, but that would probably need some additional work before getting accepted, like making it a bit fool-proof and compatible with the simpler bool usage.

[tjorvi@…]

Thanks for that.

It would probable also need to parse percentages as a string ("25%" etc.) instead of only floating point numbers. As it is in my code it will handle "0" and "1" correctly but not "true" and "false".

[nkantrowitz]

I'm really going to insist this not go in core. Its clearly not a minimalist feature in my mind. The new stats provider would be very similar to the default one, thats what subclassing is for. We need to focus more time in moving features out of core, not the other way around.

[anonymous]

That's a good point.

Maybe it would make sense to break down the routines in DefaultTicketGroupStatsProvider? more to make subclassing a better option. Routines like get_ticket_group_stats are very long and there's no way to hook into them in a subclass, other than replacing them completely. Ideally they should call other functions to do much of the work, which could then be modified individually in a subclass.

[nkantrowitz]

Replying to anonymous:

That's a good point. Maybe it would make sense to break down the routines in DefaultTicketGroupStatsProvider? more to make subclassing a better option. Routines like get_ticket_group_stats are very long and there's no way to hook into them in a subclass, other than replacing them completely. Ideally they should call other functions to do much of the work, which could then be modified individually in a subclass.

That I would be +1 on, as long as it isn't too complicating. As a fallback position, a cut n' paste of the default code into the plugin doesn't bother me all that much as the code in question is so likely to change that it needs huge levels of abstraction.

[tjorvi@…]

The docstrings say that ITicketGroupStatsProvider.get_ticket_group_stats() should return a valid TicketGroupStats? object. For this sort of plugin to be made it requires subclassing, or re-implementing, of TicketGroupStats? too.

What would be the definition of "a valid TicketGroupStats? object"? An object that has exactly the same set of member variables and methods as TicketGroupStats?? Or can it omit one or other of the methods?

[tjorvi@…]

Here is a patch that splits get_ticket_group_stats() into more managable chunks, without changing any functionality:

Index: trac/ticket/roadmap.py
===================================================================
--- trac/ticket/roadmap.py	(revision 6085)
+++ trac/ticket/roadmap.py	(working copy)
@@ -160,6 +160,8 @@
         {'name': 'active', 'status': '*', 'css_class': 'open'}
         ]
 
+    ticket_group_stat_class = TicketGroupStats
+
     def _get_ticket_groups(self):
         """Returns a list of dict describing the ticket groups
         in the expected order of appearance in the milestone progress bars.
@@ -180,9 +182,10 @@
         else:
             return self.default_milestone_groups
 
-    def get_ticket_group_stats(self, ticket_ids):
+    def _count_status(self, all_statuses, ticket_ids):
+        """Returns a dictionary holding the count of tickets for each status
+        group"""
         total_cnt = len(ticket_ids)
-        all_statuses = set(TicketSystem(self.env).get_all_status())
         status_cnt = {}
         for s in all_statuses:
             status_cnt[s] = 0
@@ -194,8 +197,15 @@
                            ",".join(str_ids))
             for s, cnt in cursor:
                 status_cnt[s] = cnt
+        return status_cnt
 
-        stat = TicketGroupStats('ticket status', 'ticket')
+    def _parse_group_statuses(self, group_statuses_str):
+        """Takes a line of the form "status1, status2, status3" and returns
+        a set of the statuses"""
+        return set([s.strip()
+                    for s in group_statuses_str.split(',')])
+
+    def _read_ticket_groups(self, all_statuses):
         remaining_statuses = set(all_statuses)
         groups =  self._get_ticket_groups()
         catch_all_group = None
@@ -212,9 +222,8 @@
                         group1=group['name'], group2=catch_all_group['name']))
                 catch_all_group = group
             else:
-                group_statuses = set([s.strip()
-                                      for s in status_str.split(',')]) \
-                                      & all_statuses
+                group_statuses = self._parse_group_statuses(status_str) \
+                                 & all_statuses
                 if group_statuses - remaining_statuses:
                     raise TracError(_(
                         "'%(groupname)s' milestone group reused status "
@@ -227,6 +236,16 @@
                 group['statuses'] = group_statuses
         if catch_all_group:
             catch_all_group['statuses'] = remaining_statuses
+
+        return groups
+
+    def get_ticket_group_stats(self, ticket_ids):
+        all_statuses = set(TicketSystem(self.env).get_all_status())
+        status_cnt = self._count_status(all_statuses, ticket_ids)
+
+        stat = self.ticket_group_stat_class('ticket status', 'ticket')
+        groups = self._read_ticket_groups(all_statuses)
+
         for group in groups:
             group_cnt = 0
             query_args = {}

[cboos]

Re-open as a reminder for the above refactoring patch.