Edgewall Software
Modify

Opened 6 years ago

Closed 6 years ago

Last modified 6 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:
Release Notes:

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

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

Download all attachments as: .zip

Change History (24)

Changed 6 years ago by Ryan J Ollos <ryan.j.ollos@…>

Attachment: BeforeEdit.png added

Changed 6 years ago by Ryan J Ollos <ryan.j.ollos@…>

Attachment: AfterEdit.png added

Changed 6 years ago by Ryan J Ollos <ryan.j.ollos@…>

Attachment: t10987-r11483-1.patch added

Patch against r11483 of the trunk.

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

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 Changed 6 years ago by Ryan J Ollos <ryan.j.ollos@…>

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 Changed 6 years ago by Remy Blank

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

Thanks for the patch.

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

Version: 1.0-stable

comment:5 Changed 6 years ago by Remy Blank

Resolution: fixed
Status: assignedclosed

Patch applied in [11494].

comment:6 Changed 6 years ago by Remy Blank

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

Changed 6 years ago by Ryan J Ollos <ryan.j.ollos@…>

Attachment: t10987-r11508-1.patch added

Patch against r11508 of the trunk.

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

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 Changed 6 years ago by Remy Blank

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

comment:9 Changed 6 years ago by Christian Boos

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

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

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 Changed 6 years ago by Christian Boos

Ah, I see, thanks for the clarification.

Changed 6 years ago by Remy Blank

Dedupe messages when loading from the session after a redirect.

comment:12 Changed 6 years ago by Remy Blank

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?

Changed 6 years ago by Remy Blank

Store messages as Markup.

comment:13 Changed 6 years ago by Remy Blank

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

Opinions?

comment:14 in reply to:  13 ; Changed 6 years ago by Christian Boos

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.

comment:15 in reply to:  14 ; Changed 6 years ago by Remy Blank

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 Changed 6 years ago by Remy Blank

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

comment:17 in reply to:  15 Changed 6 years ago by Christian Boos

Replying to rblank:

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

Oh, you're right.

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

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.
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.