Allow configuring the default "retargeting" option when closing or deleting a milestone

[Yaniv Vararu <yvararu@…>]

It would be nice to have an option to configure the default milestone that appears in the "retargeting" option when closing a milestone.

Currently, the default is "None" (Which if not paying attention - might cause you to "lose" some tickets out in the open). I for example, would like to have my "Backlog" milestone be the default option for tickets not handled in a specific milestone. Adding such an option to the "trac.ini" file would be great.

[Christian Boos]

Nice idea, PatchWelcome.

[Ryan J Ollos]

What should the option be named? [milestone] default_retarget_to?

[Ryan J Ollos]

Proposed changes can be found in repos:rjollos.git:t10010.

[Ryan J Ollos]

The proposed changes are pretty straightforward, but I'd be interested to hear whether others think it makes sense to have the _default_retarget_to property for the purpose of logging a warning when default_retarget_to doesn't exist. Oh, and I had better modify the patch slightly so that the warning message is not logged when [milestone] default_target_to doesn't exist in trac.ini or its value is empty. I'll make the change in a moment.

[Ryan J Ollos]

I've fixed the logging issue in [a760b2c0/rjollos.git] and will squash the changes before committing.

[Jun Omae]

I've tried rjollos.git:t10010 branch. It works well.

I have the following suggestions.

• any() can accept an iterable object
• We should pass a format string and the parameters to log.warn() instead of the formatted text.

trac/ticket/roadmap.py

@@ -698 +698 @@
     @property
     def _default_retarget_to(self):
         if self.default_retarget_to and \
-           not any([self.default_retarget_to == m.name
-                    for m in Milestone.select(self.env)]):
+           not any(self.default_retarget_to == m.name
+                   for m in Milestone.select(self.env)):
             self.log.warn('Milestone "%s" does not exist. Update the '
                           '"default_retarget_to" option in the '
-                          '[milestone] section of trac.ini'
-                          % self.default_retarget_to)
+                          '[milestone] section of trac.ini',
+                          self.default_retarget_to)
         return self.default_retarget_to
 
     def _do_delete(self, req, milestone):

[Ryan J Ollos]

Thanks, Jun. I'll make those changes.

Would it make sense to replace default_retarget_to instead of adding _default_retarget_to, in order to make it unlikely that someone extending the class might call the attribute that doesn't do the logging rather than the attribute that does the logging?

trac/ticket/roadmap.py

@@ -695 +695 @@
 
     # Internal methods
 
+    _default_retarget_to = default_retarget_to
+
     @property
-    def _default_retarget_to(self):
-        if self.default_retarget_to and \
-           not any([self.default_retarget_to == m.name
+    def default_retarget_to(self):
+        if self._default_retarget_to and \
+           not any([self._default_retarget_to == m.name
                     for m in Milestone.select(self.env)]):
             self.log.warn('Milestone "%s" does not exist. Update the '
                           '"default_retarget_to" option in the '
                           '[milestone] section of trac.ini'
-                          % self.default_retarget_to)
-        return self.default_retarget_to
+                          % self._default_retarget_to)
+        return self._default_retarget_to
 
     def _do_delete(self, req, milestone):
         req.perm(milestone.resource).require('MILESTONE_DELETE')
@@ -803 +805 @@
             'milestone': milestone,
             'milestone_groups': group_milestones(milestones,
                 'TICKET_ADMIN' in req.perm),
-            'retarget_to': self._default_retarget_to
+            'retarget_to': self.default_retarget_to
         }
         return 'milestone_delete.html', data, None
 
@@ -829 +831 @@
                           and 'MILESTONE_VIEW' in req.perm(m.resource)]
             data['milestone_groups'] = group_milestones(milestones,
                 'TICKET_ADMIN' in req.perm)
-            data['retarget_to'] = self._default_retarget_to
+            data['retarget_to'] = self.default_retarget_to
         else:
             req.perm(milestone.resource).require('MILESTONE_CREATE')

[Ryan J Ollos]

Latest proposed changes (including changes discussed in comment:8 and comment:9) can be found in repos:rjollos.git:t10010.2.

[Ryan J Ollos]

Oh, I should add a (since 1.1.2) to the TracIni doc string before pushing the change.

[Ryan J Ollos]

Changes committed to trunk in [12121].

[Ryan J Ollos]

I thought it would be useful to allow the default_retarget_to option to be set from the milestone admin page.

The text Retarget may not be self-explanatory enough, despite having a title attribute, so I expanded the text a bit:

Any preferences?

One other nuance is that the Clear Default button added in #11300 will become Clear Defaults, and will clear both values. I suppose 2 buttons could be added, but it seems like a bit of clutter for something that's not too inconvenient.

[Remy Blank]

I have a slight preference for the former (short labels, "Default" to the left of "Retarget"). You could add tooltips to both column headings to explain them in more details.

[Ryan J Ollos]

Yeah, after looking at it for a while I also think that the former is better. Proposed changes can be found in log:rjollos.git:t10010.3. My primary concerns are:

• Is it worth changing the dictionary passed to the template just for making the variable name less ambiguous since this could affect a plugin?: [568f666c/rjollos.git]
• Are there too many tooltips? Should the tooltips be placed on the radio buttons rather or in addition to the headings?: [64a809e2/rjollos.git]

[Ryan J Ollos]

Committed to trunk in [12262].