Edgewall Software
Modify

Opened 6 years ago

Last modified 19 months ago

#11612 new defect

ability to set and retain preferred comment order

Reported by: jeremy.j.dunn@… Owned by:
Priority: normal Milestone: next-stable-1.2.x
Component: ticket system Version: 1.0.1
Severity: normal Keywords: comment
Cc: shoffman, Jun Omae, leho@… Branch:
Release Notes:
API Changes:

Description (last modified by anonymous)

r10989 added the OldestFirst/NewestFirst/Threaded option of viewing comments, which I generally like.

However if I want something other than the default behavior, there does not seem to be a way to save my preference: I need to keep changing it on every ticket.

It would be nice to specify my default-comment-sort-order in "Preferences".

Also, the ticket-comment-order CHANGES automatically when I add a comment or reply-to a comment. If I select "oldest first" then reply to a comment, it auto-changes to "newest first". Is this intentional?

I'm on Windows 7. I typically use Firefox (v29); but verified the same behavior in Chrome (v34) and Internet Explorer (v10)

Trac 1.0.1 using standalone 'tracd' ; Python 2.5.4 on Windows 2008 R2

Attachments (3)

AjaxSaveTicketCommentOrderPref.PNG (20.0 KB ) - added by jeremy.j.dunn@… 6 years ago.
ajax action to save ticket comment order preference
TracAcctManager_ConfigWorks_15May2014.PNG (47.5 KB ) - added by jeremy.j.dunn@… 6 years ago.
AccountManager plugin config
TracDebugLog-ChangeCommentOrder.txt (21.3 KB ) - added by jeremy.j.dunn@… 6 years ago.
Trac log with DEBUG and SQL

Download all attachments as: .zip

Change History (27)

in reply to:  description ; comment:1 by Jun Omae, 6 years ago

Keywords: comment added
Milestone: next-stable-1.0.x
Type: enhancementdefect

Thanks for the reporting!

However if I want something other than the default behavior, there does not seem to be a way to save my preference: I need to keep changing it on every ticket.

It would be nice to specify my default-comment-sort-order in "Preferences".

The user preferences have no settings for the comments order. However, clicking radio buttons in comments order of ticket view, the selection will be stored in the user's session. I don't think the setting is needed in preferences.

Also, the ticket-comment-order CHANGES automatically when I add a comment or reply-to a comment. If I select "oldest first" then reply to a comment, it auto-changes to "newest first". Is this intentional?

It seems to be a defect. Auto-preview for ticket's comments and form wrongly resets to "oldest".

in reply to:  1 ; comment:2 by jeremy.j.dunn@…, 6 years ago

Replying to jomae:

However, clicking radio buttons in comments order of ticket view, the selection will be stored in the user's session.

I was hoping for the same thing; but it doesn't seem to work. I just tried with Firebug, and do indeed see the ajax request to save my ticket-comment-order in session. (see attached screenshot)

However when I navigate to another ticket, again the ticket comment order is "newest first". Why is it not using the saved preference?

by jeremy.j.dunn@…, 6 years ago

ajax action to save ticket comment order preference

in reply to:  2 ; comment:3 by Jun Omae, 6 years ago

I was hoping for the same thing; but it doesn't seem to work. I just tried with Firebug, and do indeed see the ajax request to save my ticket-comment-order in session. (see attached screenshot)

However when I navigate to another ticket, again the ticket comment order is "newest first". Why is it not using the saved preference?

Auto-preview leads the issue as has been explained in comment:1.

  1. Visit demo-1.0/ticket/1. Comments are shown in "Oldest first".
  2. Click "Newest first" button.
  3. Visit demo-1.0/ticket/2. Comments are shown in "Newest first" as expected.
  4. Enter any text in comment textarea of demo-1.0/ticket/1 and wait auto-preview for the comment. Comments in the page are wrongly shown in "Oldest first".
  5. Visit demo-1.0/ticket/3. Comments are wrongly shown in "Oldest first".

The behaviors of 4th and 5th in the above list are unexpected and defects.

in reply to:  3 comment:4 by jeremy.j.dunn@…, 6 years ago

Replying to jomae:

  1. Visit demo-1.0/ticket/2. Comments are shown in "Newest first" as expected.

yes, the demo system behaves that way; but MY system does not.

My system:

  1. open ticket 1
  2. change comment sort order
  3. navigate to ticket 2
  4. sort order is NOT the sort order selected on the other ticket

Is it possibly because I'm using standalone tracd, rather than running Trac thru a webserver like apache? We've always (~8 years) run it this way and have found it perfectly satisfactory - I guess our load is light enough. I wonder if the session-storage mechanism in tracd is different and isn't storing the preference properly? Or could it be something in my configuration that is not saving the session prefs?

comment:5 by Jun Omae, 6 years ago

I wonder if the session-storage mechanism in tracd is different and isn't storing the preference properly? Or could it be something in my configuration that is not saving the session prefs?

No differences.

Hmmm. I cannot reproduce with tracd.exe of Trac 1.0.1 on Python 2.6.

The comments-order is stored in the session and is used for ticket view.

C:\> sqlite3 C:\usr\var\trac\1.0\db\trac.db
SQLite version 3.7.15.2 2013-01-09 11:53:05
Enter ".help" for instructions
Enter SQL statements terminated with a ";"
sqlite> .header on
sqlite> .mode column
sqlite> select * from session_attribute;
sid                       authenticated  name                   value
------------------------  -------------  ---------------------  ----------
d5ac2c5966f2115677ac5833  0              ticket_comments_order  threaded
d5ac2c5966f2115677ac5833  0              __FORM_TOKEN           ac573c5bf2

Could you please try to disable installed plugins and temporarily remove template files in $ENV/templates?

Last edited 6 years ago by Jun Omae (previous) (diff)

comment:6 by jeremy.j.dunn@…, 6 years ago

sequence of actions:

  1. SQLite query: select * from session_attribute where sid = 'jeremydunn' and name = 'ticket_comments_order' result: newest
  1. open Firebug
  2. open a Trac ticket. change comment order to 'oldest'. Watch the ajax action fire. similar to previously-attached screenshot: POST parameters: save_prefs: true, ticket_comment_order: oldest
  1. repeat SQL query from step (1). same result. database was not updated.

note: a difference from the example in previous comment is that session_attribute table authenticated=1, and sid=[myTracUserName].

I noticed that __FORM_TOKEN value in the database does not match __FORM_TOKEN in the POST variables, so as an experiment I did this:

  1. Logged out of trac
  2. shut down tracd service
  3. deleted the session_attribute row for my sid, name = __FORM_TOKEN
  4. restarted tracd service
  5. logged back in
  6. changed comment sort order again
  7. requeried database: which is still not updated

Next test: disable template:

  1. shut down tracd
  2. renamed $ENV/template/site.html as site.html.hide
  3. started tracd
  4. changed ticket comment order
  5. requeried database. ticket_comment_order row of session_attribute table for my sid, has not been updated

Next test: disabling plugins:

  1. disabled advancedTicketWorkflowPlugin (0.11 dev)
  2. disabled cc-selector (0.0.3dev)
  3. disabled tracVote (0.1.3dev-r11793)
  4. disabled tracXmlRpc (1.1.2-r13776)
  5. stop and restart tracd
  6. try the test again. session_attribute table is not updated

the only plugin I cannot disable is tracAccountManager (0.4.3) because if it's disabled I cannot login (there is no authentication method without the plugin). Attached is a screenshot of my tracAccountManager config in case that is relevant.

I've restored my plugins and site template for the time being.

Any further tests I can try to help isolate why the session_attribute table is not getting updated ? Should I completely clear the session and session_attribute tables ??

by jeremy.j.dunn@…, 6 years ago

AccountManager plugin config

comment:7 by anonymous, 6 years ago

Description: modified (diff)

comment:8 by Jun Omae, 6 years ago

Would you please check warnings and errors in your trac.log when comments order is changed? If it fails to save the session data, a warning is logged at tags/trac-1.0.1/trac/web/session.py@:157-158#L135.

Also, using [logging] log_level = DEBUG and [trac] debug_sql = enabled is able to log SQL queried to trac.log. cf. TracIni#trac-section.

in reply to:  8 comment:9 by jeremy.j.dunn@…, 6 years ago

Replying to jomae:

Would you please check warnings and errors in your trac.log when comments order is changed? If it fails to save the session data, a warning is logged at tags/trac-1.0.1/trac/web/session.py@:157-158#L135.

Also, using [logging] log_level = DEBUG and [trac] debug_sql = enabled is able to log SQL queried to trac.log. cf. TracIni#trac-section.

I do not see any errors or warnings in the log file (attached).

However, I do see some strange things:

  1. when I changed the comment-order to 'oldest', the insert statement still says 'newest'. I did this TWICE just to make sure there was no mistake.
  2. I'm not 100% sure, but it appears that the deletion of session_attributes is failing:
    2014-05-16 05:40:45,572 Trac[util] DEBUG: SQL: DELETE FROM session_attribute
                          WHERE sid=%s AND authenticated=%s
    2014-05-16 05:40:45,572 Trac[util] DEBUG: args: (u'jeremydunn', 1)
    2014-05-16 05:40:45,575 Trac[util] DEBUG: prefetch: 0 rows
    
    is 0 rows the result of the DELETE statement?
  1. I double-checked my session_attribute table in SQLite and verified that ALL rows for my sid ('jeremydunn') have authenticated=1. So they should have been deleted.

after disabling logging, I tried the following:

  1. manually deleted all session_attribute for sid='jeremydunn'.
  2. checked db after deletion: 0 rows for my sid
  3. reload the ticket page I was on.
  4. checked the database: 1 row ("shown_vote_message") for my sid
  5. clicked "newest first" in the comment section
  6. checked the database: STILL only 1 row
  7. clicked "oldest first" in the comment section
  8. checked the database: STILL only 1 row
  1. navigated to another ticket. comment order 'oldest'
  2. changed to 'newest'
  3. navigated to another ticket. comment order is OLDEST again. Now it appears that 'oldest' is the default
  4. checked database: still only 1 row in session_attribute for my sid

Bottom line: something is not working to save session_attributes in our database. I also looked at the whole session_attribute table: it has attributes for users who have not logged in, in a very long time; but does -not- have session-attributes from many current users. I'm guessing that some upgrade broke the ability to update session-attributes; but since there is no datestamp on the table, I cannot say when or what version caused this problem.

Any further debugging suggestions?

by jeremy.j.dunn@…, 6 years ago

Trac log with DEBUG and SQL

comment:10 by jeremy.j.dunn@…, 6 years ago

sessions_attribute table information as reported by SQLite manager:

/* Column information for - session_attribute */
------------------------------------------------

cid      name              type        notnull      dflt_value      pk   
------- ---------------- ---------- ----------- -------------- --- 
0        sid               text        0                            0    
1        authenticated     integer     0                            0    
2        name              text        0                            0    
3        value             text        0                            0    


/* DDL information for - session_attribute */
---------------------------------------------
CREATE TABLE session_attribute (
    sid text,
    authenticated integer,
    name text,
    value text,
    UNIQUE (sid,authenticated,name)
)

comment:11 by Ryan J Ollos, 5 years ago

Issue with Comments only not being retained in session data has been reported in #11808.

comment:12 by Ryan J Ollos, 5 years ago

#11808 says that the behavior is only seen when authenticated. Would you be able to test that scenario for the issue experienced in this ticket?

comment:13 by Ryan J Ollos, 5 years ago

Cc: shoffman Jun Omae added

I can reproduce the issue in this ticket as well as #11808 when AccountManagerPlugin is installed. Setting a breakpoint at the location the AJAX callback is processed, I see thatreq.method == 'GET' when AccountManagerPlugin is enabled, which is the direct cause for the preference not being saved.

As to the reason why this is a GET request rather than a POST, the only guess I have at the moment is that another handler in account manager matches the request and redirects, converting the jQuery AJAX POST request to a GET. I'll debug further when I have more time tomorrow.

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

comment:14 by Ryan J Ollos, 5 years ago

The problem is in EmailVerificationModule.pre_process_request(). When the user is authenticated the account validation is executed in response to an AJAX POST, which raises a RegistrationError exception with message You must specify a valid email address (which Jun noted as seeing in comment:7:ticket:11808): browser:/accountmanagerplugin/trunk/acct_mgr/register.py@14078:551,566#L542.

The following patch at least works around the issue:

  • accountmanagerplugin/trunk/acct_mgr/register.py

    diff --git a/accountmanagerplugin/trunk/acct_mgr/register.py b/accountmanagerplu
    index 5008350..4c2eec2 100644
    a b class EmailVerificationModule(CommonTemplateProvider):  
    546546            # Permissions for anonymous users remain unchanged.
    547547            return handler
    548548        elif req.path_info == '/prefs' and req.method == 'POST' and \
    549                 not 'restore' in req.args:
     549                not 'restore' in req.args and \
     550                not req.get_header('X-Requested-With') == 'XMLHttpRequest':
    550551            try:
    551552                AccountManager(self.env).validate_account(req)
    552553                # Check passed without error: New email address seems good.

comment:15 by Jun Omae, 5 years ago

The changes seems to be good.

BTW, I think storing #prefs form to user's session via XHR has two problems. One of the problems is that __FORM_TOKEN's value is always stored to the session when changing the form, see comment:5. Another one is that a user can store any value to user's session by changing elements in #prefs using Web Developer tools in browser.

comment:16 by Ryan J Ollos, 5 years ago

th:#12050 created to address issue for AccountManagerPlugin. This ticket will be left open to address the issue confirmed in comment:1.

Last edited 5 years ago by Jun Omae (previous) (diff)

in reply to:  15 ; comment:17 by Ryan J Ollos, 5 years ago

Replying to jomae:

The changes seems to be good.

BTW, I think storing #prefs form to user's session via XHR has two problems. One of the problems is that __FORM_TOKEN's value is always stored to the session when changing the form, see comment:5.

Is this patch sufficient to handle that issue?:

  • trac/prefs/web_ui.py

    diff --git a/trac/prefs/web_ui.py b/trac/prefs/web_ui.py
    index 0f1100d..897f8e2 100644
    a b class PreferencesModule(Component):  
    140140
    141141    def _do_save_xhr(self, req):
    142142        for key in req.args:
    143             if not key in ['save_prefs', 'panel_id']:
     143            if not key in ['save_prefs', 'panel_id', '__FORM_TOKEN']:
    144144                req.session[key] = req.args[key]
    145145        req.session.save()
    146146        req.send_no_content()

Another one is that a user can store any value to user's session by changing elements in #prefs using Web Developer tools in browser.

I'm not sure of the steps to reproduce the issue by editing the page markup, but I was able to replicate the effect by editing the JavaScript: Open threaded_comments.js in the browser tools and edit the data passed to ajax: tags/trac-1.0.2/trac/htdocs/js/threaded_comments.js@:94#L90. On autopreview the new key / value pairs are saved in the session attributes table. It seems like we need to register the allowed keys and filter the data received in the AJAX callback.

Or do you have a better overall approach in mind?

in reply to:  1 ; comment:18 by Ryan J Ollos, 5 years ago

Replying to jomae:

Also, the ticket-comment-order CHANGES automatically when I add a comment or reply-to a comment. If I select "oldest first" then reply to a comment, it auto-changes to "newest first". Is this intentional?

It seems to be a defect. Auto-preview for ticket's comments and form wrongly resets to "oldest".

It appears to be intentional that on autopreview the comment ordering reverts to Oldest first: tags/trac-1.0.2/trac/ticket/templates/ticket.html@:83-85#L76.

Rather than reverting the order on autopreview, perhaps we should reapply the preferred ordering.

in reply to:  18 comment:19 by Peter Suter, 5 years ago

Replying to rjollos:

It appears to be intentional that on autopreview the comment ordering reverts to Oldest first: tags/trac-1.0.2/trac/ticket/templates/ticket.html@:83-85#L76.

Rather than reverting the order on autopreview, perhaps we should reapply the preferred ordering.

See comment:22:ticket:3360 for the rationale:

The reason for the un-threading is that we fetch new comments and highlight them so the user sees that there are concurrent edits. If we didn't unthread, the new comments could end up anywhere in the changelog (or at the very top when sorting by descending time), so they would be invisible to the user.

in reply to:  17 ; comment:20 by Jun Omae, 5 years ago

Replying to rjollos:

Is this patch sufficient to handle that issue?: […]

Yes. That's good.

I'm not sure of the steps to reproduce the issue by editing the page markup, but I was able to replicate the effect by editing the JavaScript: Open threaded_comments.js in the browser tools and edit the data passed to ajax: tags/trac-1.0.2/trac/htdocs/js/threaded_comments.js@:94#L90.

Oh, sorry. I misunderstood. It's able to store any data by calling jQuery.ajax() from javascript debugger in the browser.

On autopreview the new key / value pairs are saved in the session attributes table. It seems like we need to register the allowed keys and filter the data received in the AJAX callback.

Or do you have a better overall approach in mind?

Yeah, I think we need interface like this.

class ISessionAttributeProvider(Interface):

    def get_session_attribute_names():
        """Return a list of supported names in session attribute."""

    def validate_session_attribute(name, value):
        """Validate the given attribute.""""

in reply to:  20 comment:21 by Ryan J Ollos, 5 years ago

Replying to jomae:

Replying to rjollos:

Is this patch sufficient to handle that issue?: […]

Yes. That's good.

Thanks, patch applied in [13260:13261].

I'll have a patch ready soon that adds ISessionAttributeProvider.

in reply to:  20 comment:22 by Ryan J Ollos, 5 years ago

Replying to jomae:

Or do you have a better overall approach in mind?

Yeah, I think we need interface like this.

class ISessionAttributeProvider(Interface):

    def get_session_attribute_names():
        """Return a list of supported names in session attribute."""

    def validate_session_attribute(name, value):
        """Validate the given attribute.""""

#11840 created for introducing the new interface.

comment:23 by lkraav <leho@…>, 5 years ago

Cc: leho@… added

comment:24 by Ryan J Ollos, 3 years ago

Milestone: next-stable-1.0.xnext-stable-1.2.x

Moved ticket assigned to next-stable-1.0.x since maintenance of 1.0.x is coming to a close. Please move the ticket back if it's critical to fix on 1.0.x.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned. Next status will be 'new'.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from (none) to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.