Milestone edit page should more clearly show when milestone doesn't exist

[Ryan J Ollos]

When navigating from the Add new milestone button on the Roadmap page the path will be /milestone?action=new and the user should have a good idea that the milestone does not yet exist (although it might be less clear when &name=milestoneX is appended to the path resulting in the milestone name input having text).

In the case that a link is followed to a non-existing milestone the path will be /milestone/milestoneX and it might be less clear that the milestone does not yet exist. We could give a clear indicator in this case:

The patch would be simple, and would not add a notice for the case that was first described, when the user follows the Add new milestone link.

trac/ticket/roadmap.py

@@ -860 +860 @@
             data['milestone_groups'] = group_milestones(milestones,
                 'TICKET_ADMIN' in req.perm)
         else:
+            if milestone.name:
+                add_notice(req, _("Milestone %(name)s does not exist. You can"
+                                  " create it here.", name=milestone.name))
             req.perm(milestone.resource).require('MILESTONE_CREATE')
 
         chrome = Chrome(self.env)

[Ryan J Ollos]

One side note: I noticed a while back at a few places in the codebase some multiline strings that always placed the whitespace on the left hand side (like shown in my patch above):

"This is a multi-line string"
" with the whitespace on the"
" left on each continuation line"
" which seems to make it less likely to omit"
" a whitespace and inadvertently join two words."

as opposed to,

"This is a multi-line string "
"with the whitespace on the "
"left on each continuation line "
"which seems to make it less likely to omit "
"a whitespace and inadvertently join two words."

Maybe that is a good practice to add to TracDev/CodingStyle#Strings? Any opinions?

[Jun Omae]

I usually use the latter. It is commonly-used in the codebase.

$ grep -rE "^ *u?['\"] " --include='*.py' trac tracopt | wc -l
47
$ grep -rE " ['\"] *\\\\?$" --include='*.py' trac tracopt | wc -l
483

[Ryan J Ollos]

I have usually used the later as well. That doesn't form an argument for why it's a better style though.

[Jun Omae]

Adding the notice sounds good to me. But I guess that we should add it after checking priviledges. Otherwise, it would be shown in PermissionError page.

             req.perm(milestone.resource).require('MILESTONE_CREATE')
+            if milestone.name:
+                add_notice(req, _("Milestone %(name)s does not exist. You can"
+                                  " create it here.", name=milestone.name))

[Ryan J Ollos]

Thanks for spotting that issue. Proposed changes in log:rjollos.git:t11663. I'm anticipating that RequestHandlerPermissionsTestCaseBase will be reused in numerous test case modules as we write more unit tests and depend less on functional tests.

[Ryan J Ollos]

Committed to 1.0-stable in [12910:12911], merged to trunk in [12912:12913].