Edgewall Software
Modify

Opened 7 years ago

Closed 7 years ago

Last modified 3 years ago

#11368 closed enhancement (fixed)

TracError messages with markup are not enclosed in a p.message when rendered

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.0.2
Component: web frontend Version: 1.0-stable
Severity: normal Keywords: TracError
Cc: Branch:
Release Notes:

Fixed TracError message raised in trac.web.auth would not be rendered in a red box.

API Changes:

TracError messages that don't contain a p element will be enclosed in a p element with class message when rendered on the error page. The function find_element from trac.util.html is available in the template context.

Internal Changes:

Description

While trying to raise an exception in which the message contains a link, I found that the message was is not wrapped in a p.message. There's at least one existing case in the codebase where the message contains markup: tags/trac-1.0.1/trac/web/auth.py@:145-153#L130. By forcing the conditional to True, it can be seen to render as:

The behavior is readily explained by the rendering logic, and [5388#file4] indicates it was implemented that way intentionally to avoid nested paragraphs. I propose a minor change to incrementally improve on this: log:rjollos.git:t11368.

Attachments (2)

TracError.png (13.8 KB ) - added by Ryan J Ollos 7 years ago.
TracError2.png (14.9 KB ) - added by Ryan J Ollos 7 years ago.

Download all attachments as: .zip

Change History (12)

by Ryan J Ollos, 7 years ago

Attachment: TracError.png added

by Ryan J Ollos, 7 years ago

Attachment: TracError2.png added

comment:1 by Ryan J Ollos, 7 years ago

With the proposed change we get the expected:

comment:2 by Jun Omae, 7 years ago

The changes break error message from timeline view. See http://twitpic.com/dln1ed/full.

At least, TracError or subclasses with Genshi elements is raised from the following.

$ egrep -r '(TracError|AdminCommandError|InvalidAttachment|ConfigurationError|ResourceNotFound|QuerySyntaxError|QueryValueError|InvalidTicket|ProcessorError|InvalidWikiPage)\( *tag' trac tracopt
trac/versioncontrol/web_ui/util.py:        raise ResourceNotFound(tag(
trac/notification.py:            raise TracError(tag(
trac/timeline/web_ui.py:        raise TracError(tag(
trac/web/auth.py:            raise TracError(tag_("Authentication information not available. "

We should fix only trac/web/auth.py, I think.

  • branches/1.0-stable/trac/web/auth.py

     
    148148                             title=_("Configuring Authentication"),
    149149                             href=req.href.wiki('TracInstall')
    150150                                  + "#ConfiguringAuthentication")
    151             raise TracError(tag_("Authentication information not available. "
    152                                  "Please refer to the %(inst_doc)s.",
    153                                  inst_doc=inst_doc))
     151            raise TracError(tag.p(tag_("Authentication information not "
     152                                       "available. Please refer to the "
     153                                       "%(inst_doc)s.", inst_doc=inst_doc),
     154                                  class_='message'))
    154155        remote_user = req.remote_user
    155156        if self.ignore_case:
    156157            remote_user = remote_user.lower()
Last edited 7 years ago by Jun Omae (previous) (diff)

comment:3 by Ryan J Ollos, 7 years ago

It doesn't seem like very good behavior to make the user wrap every error message with containing in a paragraph in order to get consistent display of error messages. I will see if I can modify my patch so that it works correctly with error messages from the timeline as well.

Last edited 7 years ago by Ryan J Ollos (previous) (diff)

comment:4 by Ryan J Ollos, 7 years ago

Oh, I am dumb. I tested find_element, but I didn't test the usage of find_element for the case that the message contains a p element, and I wasn't calling it correctly. After [4a61f679/rjollos.git], the TracError from the timeline event that contains a paragraph will be rendered correctly. Do you see any problem with the changes now that the issue is fixed?

Last edited 7 years ago by Ryan J Ollos (previous) (diff)

comment:5 by Jun Omae, 7 years ago

Before the changes, if want to show directly a message on error view, TracError's message must have Genshi fragments or elements. After the changes, TracError's message must have Genshi p element.

No problem. Verified the following. Please commit.

  • trac/versioncontrol/web_ui/util.py
  • trac/timeline/web_ui.py
  • trac/web/auth.py

TracError from trac/notification.py would be shown with add_warning, not on error view. Also it has another issue, #11369.

in reply to:  5 comment:6 by Ryan J Ollos, 7 years ago

Replying to jomae:

Before the changes, if want to show directly a message on error view, TracError's message must have Genshi fragments or elements. After the changes, TracError's message must have Genshi p element.

Maybe I'm misunderstanding, but as far as I can see the only difference after the change is that if the error message doesn't contain a p element, the error message will be wrapped in a p.message: [dffc5587/rjollos.git#file0].

I did some testing this evening with the following plugin:

  • /throwerror
  • /throwerror?markup=true
  • /throwerror?markup=true&p=true
from genshi.builder import tag
from trac.core import Component, TracError, implements
from trac.web.api import IRequestHandler

class ThrowErrorPlugin(Component):
    implements(IRequestHandler)

    def match_request(self, req):
        if req.path_info.startswith('/throwerror'):
            return True

    def process_request(self, req):
        if 'markup' in req.args and req.args['markup'] == 'true':
            if 'p' in req.args and req.args['p'] == 'true':
                # Markup with p element should be rendered as is
                raise TracError(tag(tag.p("Error ", tag.strong("with markup"), " and it is already enclosed in a ", tag.em("p.message"), class_="message"), tag.p("There is also some ", tag.em("markup outside the p.message"), "including a link to ", tag.a("the Trac site", href='http://trac.edgewall.org'))))
            else:
                # Markup with no p element should be wrapped in a p.message
                raise TracError(tag("Error ", tag.strong("with markup."), " It should be wrapped in a ", tag.code("p.message")))
        else:
            raise TracError("Error in plain text.")

Maybe a functional test could be added to check these cases.

TracError from trac/notification.py would be shown with add_warning, not on error view. Also it has another issue, #11369.

Oh, that's great, thank you. I also encountered that issue while working on #11189, and I was planning to investigate after this ticket is resolved.

comment:7 by Ryan J Ollos, 7 years ago

I added a small refactoring in [22ab25dc/rjollos.git].

comment:8 by Ryan J Ollos, 7 years ago

Status: newassigned

comment:9 by Ryan J Ollos, 7 years ago

API Changes: modified (diff)
Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to 1.0-stable in [12260] and merged to trunk in [12261].

comment:10 by Ryan J Ollos, 3 years ago

#12916 is related.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos 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.