InterTrac link to non-existent changeset results in inaccurate error message

[Ryan J Ollos]

For example, changeset/1000000 leads to a page displaying:

Error: Invalid Changeset Number

No changeset 1000000 in the repository

TracGuide — The Trac User and Administration Guide

However, [trac 1000000] leads to a page displaying:

Error: Forbidden

Can't view changeset:"1000000": privileges are required to perform this operation. You don't have the required permissions.

TracGuide — The Trac User and Administration Guide

[Ryan J Ollos]

The error message looks strange because the "message" is being passed as the first argument and is being assigned to the parameter action. We should at least be using a keyword argument for the msg parameter​.

We could try to check if the resource exists, and raise different exception depending on whether the issue is resource existence or authorization. That seems like a bit of work though, and for now I'd just propose the following:

trac/wiki/intertrac.py

@@ -91 +91 @@
         if isinstance(link_frag, (Element, Fragment)):
             elt = find_element(link_frag, 'href')
             if elt is None: # most probably no permissions to view
-                raise PermissionError(_("Can't view %(link)s:", link=link))
+                raise PermissionError(
+                    msg=_("Can't view %(link)s. Resource doesn't exist or "
+                          "you don't have the required permission.",
+                          link=link))
             href = elt.attrib.get('href')
         else:
             href = req.href(link.rstrip(':'))

This change results in:

Error: Forbidden

Can't view changeset:"1000000". Resource doesn't exist or you don't have the required permission.

TracGuide — The Trac User and Administration Guide

Maybe it is more appropriate to raise a TracError rather than a PermissionError.

trac/wiki/intertrac.py

@@ -20 +20 @@
 
 from trac.config import ConfigSection
 from trac.core import *
-from trac.perm import PermissionError
 from trac.util.html import find_element
 from trac.util.translation import _, N_
 from trac.web.api import IRequestHandler
@@ -90 +89 @@
         link_frag = extract_link(self.env, web_context(req), link)
         if isinstance(link_frag, (Element, Fragment)):
             elt = find_element(link_frag, 'href')
-            if elt is None: # most probably no permissions to view
-                raise PermissionError(_("Can't view %(link)s:", link=link))
+            if elt is None:
+                raise TracError(
+                    _("Can't view %(link)s. Resource doesn't exist or "
+                      "you don't have the required permission.", link=link))
             href = elt.attrib.get('href')
         else:
             href = req.href(link.rstrip(':'))

Trac Error

Can't view changeset:"1000000". Resource doesn't exist or you don't have the required permission.

TracGuide — The Trac User and Administration Guide

[Ryan J Ollos]

Should I commit this to trunk rather than 1.0-stable since it changes a translatable string and it's not a fix for a major functional issue?

[Jun Omae]

I think that pushing to 1.0-stable is okay because the changesets after 1.0.1, e.g. r11777, have been changed the translation messages.

[Ryan J Ollos]

Committed to 1.0-stable in [12048]. Merged to trunk in [12049].