ability to set and retain preferred comment order

[jeremy.j.dunn@…]

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

[Jun Omae]

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

[jeremy.j.dunn@…]

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?

[jeremy.j.dunn@…]

ajax action to save ticket comment order preference

[Jun Omae]

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.

[jeremy.j.dunn@…]

Replying to jomae:

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

[Jun Omae]

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?

[jeremy.j.dunn@…]

sequence of actions:

1. SQLite query: select * from session_attribute where sid = 'jeremydunn' and name = 'ticket_comments_order' result: newest

2. open Firebug
3. 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

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

[jeremy.j.dunn@…]

AccountManager plugin config

[Jun Omae]

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.

[jeremy.j.dunn@…]

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?

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

[jeremy.j.dunn@…]

Trac log with DEBUG and SQL

[jeremy.j.dunn@…]

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

[Ryan J Ollos]

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

[Ryan J Ollos]

#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?

[Ryan J Ollos]

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.

[Ryan J Ollos]

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

@@ -546 +546 @@
             # Permissions for anonymous users remain unchanged.
             return handler
         elif req.path_info == '/prefs' and req.method == 'POST' and \
-                not 'restore' in req.args:
+                not 'restore' in req.args and \
+                not req.get_header('X-Requested-With') == 'XMLHttpRequest':
             try:
                 AccountManager(self.env).validate_account(req)
                 # Check passed without error: New email address seems good.

[Jun Omae]

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.

[Ryan J Ollos]

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

[Ryan J Ollos]

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

@@ -140 +140 @@
 
     def _do_save_xhr(self, req):
         for key in req.args:
-            if not key in ['save_prefs', 'panel_id']:
+            if not key in ['save_prefs', 'panel_id', '__FORM_TOKEN']:
                 req.session[key] = req.args[key]
         req.session.save()
         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?

[Ryan J Ollos]

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.

[Peter Suter]

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.

[Jun Omae]

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

[Ryan J Ollos]

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.

[Ryan J Ollos]

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.

[Ryan J Ollos]

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.

[Ryan J Ollos]

Milestone renamed