Edgewall Software
Modify

Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#10987 closed enhancement (fixed)

Avoid duplicate info and warning messages

Reported by: Ryan J Ollos <ryan.j.ollos@…> Owned by: Ryan J Ollos <ryan.j.ollos@…>
Priority: normal Milestone: 1.0.1
Component: web frontend Version: 1.0-stable
Severity: normal Keywords:
Cc: Branch:
Release Notes:

Prevent possibility of multiple identical info or warning messages being presented to the user.

API Changes:
Internal Changes:

Description

To reproduce the issue I've seen:

  • Create an instance of Trac without the SubversionConnector component enabled.
  • Observe that the Warning: Can't synchronize with repository … message is seen.
  • Edit a wiki page and save the change.
  • Observe that the Warning: Can't synchronize with repository … message is seen twice.

Here are screen captures before and after an edit:

Attachments (6)

BeforeEdit.png (19.8 KB ) - added by Ryan J Ollos <ryan.j.ollos@…> 11 years ago.
AfterEdit.png (23.9 KB ) - added by Ryan J Ollos <ryan.j.ollos@…> 11 years ago.
t10987-r11483-1.patch (3.3 KB ) - added by Ryan J Ollos <ryan.j.ollos@…> 11 years ago.
Patch against r11483 of the trunk.
t10987-r11508-1.patch (1.4 KB ) - added by Ryan J Ollos <ryan.j.ollos@…> 11 years ago.
Patch against r11508 of the trunk.
10987-dedupe-messages-r11531.patch (1.6 KB ) - added by Remy Blank 11 years ago.
Dedupe messages when loading from the session after a redirect.
10987-dedupe-markup-r11531.patch (1.7 KB ) - added by Remy Blank 11 years ago.
Store messages as Markup.

Download all attachments as: .zip

Change History (24)

by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

Attachment: BeforeEdit.png added

by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

Attachment: AfterEdit.png added

by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

Attachment: t10987-r11483-1.patch added

Patch against r11483 of the trunk.

comment:1 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

t10987-r11483-1.patch attempts to fix the issue by preventing duplicate entries in the req.chrome['warnings'] and req.chrome['notices'] lists.

comment:2 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

I went about trying to fix this under the assumption that we could end up with duplicate info or warning messages displayed under circumstances other than the repository not correctly configured. However, if that is not the case, then it is probably not too important to fix the issue.

comment:3 by Remy Blank, 11 years ago

Milestone: 1.0.1
Owner: set to Remy Blank
Status: newassigned

Thanks for the patch.

comment:4 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

Version: 1.0-stable

comment:5 by Remy Blank, 11 years ago

Resolution: fixed
Status: assignedclosed

Patch applied in [11494].

comment:6 by Remy Blank, 11 years ago

Owner: changed from Remy Blank to Ryan J Ollos <ryan.j.ollos@…>

by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

Attachment: t10987-r11508-1.patch added

Patch against r11508 of the trunk.

comment:7 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

Resolution: fixed
Status: closedreopened

I thought I had tested this patch pretty thoroughly, but I'm still seeing the issue described in comment:description. This seems to be due to the saving/restoring of messages saved in session data to handle the case of redirect. I've created t10987-r11508-1.patch to deal with the issue. Sorry for the error.

comment:8 by Remy Blank, 11 years ago

Owner: changed from Ryan J Ollos <ryan.j.ollos@…> to Remy Blank
Status: reopenedassigned

comment:9 by Christian Boos, 11 years ago

Wouldn't there be a way to make sure no duplicate messages get stored in the session in the first place?

comment:10 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

I'm seeing only one message stored in the session. There is a message popped from req.session, and another already existing in req.chrome at line 966 of trac.web.chrome:

                    for i in itertools.count():
                        message = req.session.pop('chrome.%s.%d' % (type_, i))
                        req.chrome[type_].append(Markup(message))

It looks like we need to make sure there aren't duplicates added when the messages in req.session are appended to req.chrome, which is what the patch tries to accomplish.

That said, I'm not claiming to have a big picture view of what's going on in this function, so I wouldn't be surprised if there is a better way to fix this.

comment:11 by Christian Boos, 11 years ago

Ah, I see, thanks for the clarification.

by Remy Blank, 11 years ago

Dedupe messages when loading from the session after a redirect.

comment:12 by Remy Blank, 11 years ago

Using unescape() in t10987-r11508-1.patch isn't quite correct, as req.chrome[type_] can contain strings or Markup instances, and the comparison will fail for the latter. We need to compare the messages read from the session to the escaped messages. 10987-dedupe-messages-r11531.patch does this.

But even this isn't completely correct. If a message is read from the session after a redirect, it's stored in req.chrome[type_] as a Markup instance. After that, if the same message is added again as a string, it will be duplicated.

How about escaping the messages in add_warning() and add_notice()? That way, req.chrome[type_] will only contain Markup instances, and the comparison should always work.

Opinions?

by Remy Blank, 11 years ago

Store messages as Markup.

comment:13 by Remy Blank, 11 years ago

10987-dedupe-markup-r11531.patch implements the suggestion from comment:12.

Opinions?

in reply to:  13 ; comment:14 by Christian Boos, 11 years ago

Replying to rblank:

10987-dedupe-markup-r11531.patch implements the suggestion from comment:12.

We should add a comment in add_warning saying that we normalize on Markup instances we have to be wary about the fact that '<a>' == Markup('<a>') (which is not completely obvious).

Then about the session, doing the escape(sessiondata) on save and Markup(sessiondata) on read seems to be a good thing to do. However, shouldn't we rather do that in general, for any session_attribute, and therefore in Session.get_session/set? And if we store the session attributes escaped, we should probably use full escaping (keep the default quote=True).

But if you want to do it the incremental way, we could leave the above suggestion for 1.1.x and fix the present issue like you did in the patch.

in reply to:  14 ; comment:15 by Remy Blank, 11 years ago

Resolution: fixed
Status: assignedclosed

10987-dedupe-markup-r11531.patch applied in [11532], with improved docstrings.

Replying to cboos:

Then about the session, doing the escape(sessiondata) on save and Markup(sessiondata) on read seems to be a good thing to do. However, shouldn't we rather do that in general, for any session_attribute, and therefore in Session.get_session/set? And if we store the session attributes escaped, we should probably use full escaping (keep the default quote=True).

I wouldn't do this for all the data stored in the session. Session attributes can be arbitrary strings (and the DB supports this), so there's no reason to encode or escape them. Why escape e.g. the last ticket query? It wouldn't even work, as escape() actually modifies the string value, so you would have to call unescape() when reading the value, not Markup().

Also, these messages are the only occurrence I know of of text stored in the session which is used afterward for rendering.

comment:16 by Remy Blank, 11 years ago

Owner: changed from Remy Blank to Ryan J Ollos <ryan.j.ollos@…>

in reply to:  15 comment:17 by Christian Boos, 11 years ago

Replying to rblank:

… Why escape e.g. the last ticket query? It wouldn't even work, …

Oh, you're right.

comment:18 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

Release Notes: modified (diff)

Modify Ticket

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