Big, red Delete button in 0.11.6

[Chris Nelson]

After upgrading TicketDeletePlugin from 2.0 to 3.0.0dev, the button to delete a comment got bigger and changed color.

[Chris Nelson]

The <a> for the button appears to have lost the inlinklinkbutton style.

The old, nice button was built with JavaScript and poked into the following <div>

<div class="inlinebuttons">
                  <input type="hidden" name="replyto" value="1" />
                  <input type="submit" value="Reply" title="Reply to comment 1" /> 
<a class="inlinelinkbutton" href="../ticketchangecomment/8234?cnum=1">Change</a>
        </div>

The new one says:

<div class="inlinebuttons">
                  <input type="hidden" name="replyto" value="1" />
                  <input type="submit" value="Reply" title="Reply to comment 1" /> 
<a class="inlinelinkbutton" href="../ticketchangecomment/233?cnum=1">Change</a>
        <a href="../admin/ticket/comments/233?cnum=1" title="Delete this comment">Delete</a></div>

(It may be that inlinelinkbutton is a local style but I think we submitted a patch to get it into Trac's core CSS.)

[Ryan J Ollos]

#10262 closed as a duplicate.

[Ryan J Ollos]

The next check-in should fix the issue. I've eliminated the need for custom CSS in TicketDeletePlugin entirely. Please report back if it is working well for you after this checkin. The issue you raised in #840 will be dealt with shortly as well.

[Ryan J Ollos]

(In [12051]) Refs #10263: The plugin now implements the IRequestFilter interface. This allows for using an input element as the Delete button rather than a link element, which eliminates the need for custom CSS, since the Trac styling of the reply button applies to the Delete input element as well.

FIX: Previously, if the description was empty, the Delete button would not be shown, since it relied on the presence of the reply button, which doesn't exist when the description is empty. The Delete button is always shown now.

[Ryan J Ollos]

(In [12052]) Refs #10263: Added 3-Clause BSD license to source.

[Ryan J Ollos]

I discovered a few more issues today. You probably want to hold off another day or so until I get those fixed.

[Ryan J Ollos]

(In [12098]) Refs #840, #10263:

• Added check of Trac version. The plugin won't install in Trac 0.12 and later. In Trac 0.12 and later, enable the equivalent functionality by setting tracopt.ticket.deleter = enabled.
• When a "change" checkbox is selected, all of the child "field" checkboxes will be selected.
• Improved warning that is issued when an invalid ticket ID is entered. The left # characters in case the user enters #100 for a ticket ID, rather than 100.
• Fixed some regressions introduced in #840, when migrating from the javascript version of the plugin, to the genshi version.
  • Alert confirmation dialog has been restored.
  • Changes sort chronlogically.

Please test and report back, if you can.

[Ryan J Ollos]

(In [12099]) Refs #840, #10263: FIX: the reply buttons were redirecting to the delete page as well.

[Chris Nelson]

Screen shot of failure to refresh after deletion

[Chris Nelson]

OK up until I confirm deletion and see an error, attached.

[Chris Nelson]

Some possibly interesting detail from my trac.log:

Traceback (most recent call last):
  File "/usr/local/lib/python2.6/dist-packages/Trac-0.11.6-py2.6.egg/trac/web/main.py", line 455, in _dispatch_request
    dispatcher.dispatch(req)
  File "/usr/local/lib/python2.6/dist-packages/Trac-0.11.6-py2.6.egg/trac/web/main.py", line 206, in dispatch
    resp = chosen_handler.process_request(req)
  File "/usr/local/lib/python2.6/dist-packages/Trac-0.11.6-py2.6.egg/trac/admin/web_ui.py", line 114, in process_request
    path_info)
  File "build/bdist.linux-x86_64/egg/ticketdelete/web_ui.py", line 128, in render_admin_panel
    self._delete_change(t.id, ts, field)
  File "build/bdist.linux-x86_64/egg/ticketdelete/web_ui.py", line 215, in _delete_change
    oldval = [old for _, _, field2, old, _, _ in ticket.get_changelog(to_datetime(int(ts))) if field2 == field][0]
IndexError: list index out of range

[Ryan J Ollos]

Is it possible this is one of the more complex cases, like #840 or #4817? I hammered on the delete functionality quite a bit, but didn't get around to testing cases such as #840 and #4817.

[Chris Nelson]

Replying to rjollos:

Is it possible this is one of the more complex cases, like #840 or #4817? I hammered on the delete functionality quite a bit, but didn't get around to testing cases such as #840 and #4817.

The correct/expected item was checked on the delete page so I don't think it's 840.

The item I tried to delete was custom field setting but nothing to do with an attachment.

I can delete a change to estimatedhours so it's not a general problem with custom fields.

Note that the change I tried to delete was the initial setting of the value of a custom field displayed as a select box. Perhaps the plugin doesn't know how to go from set to unset on such fields?

[Ryan J Ollos]

Replying to ChrisNelson:

Note that the change I tried to delete was the initial setting of the value of a custom field displayed as a select box. Perhaps the plugin doesn't know how to go from set to unset on such fields?

I'm not entirely sure what you mean, but I think the picture will help. The attachment is having problems again. Could you email it to me?

[Chris Nelson]

I have a custom field:

• Type: Radio (I lied before when I said it was a Select)
• Default value: (blank)
• Options:
  • Low
  • Medium
  • High
  • Very high

When I create a new ticket, no radio buttons are selected.

Then I select one of the radio buttons and save my changes.

Then I click the Delete button in the ticket "comment" for setting the initial value of the field.

On the delete page, the "Old value" column is empty.

When I confirm the deletion, I get the error.

[Ryan J Ollos]

Thanks, I will try to reproduce in a dev environment and hope to have a fix available later today.

[anonymous]

I've tried reproducing with the following configuration:

[ticket-custom]
confidence = radio
confidence.label = confidence
confidence.options = Low|Medium|High|Very High

I haven't been able to reproduce. Could this problem have something to do with your microsecond patch? The datetime of the change in one of your screen captures is 12/31/69 19:22:29.

I'm adding some more debug info to the code, that might help isolate the issue. I'll also post some screen captures, so you can see what steps I took to try to reproduce the issue on Trac 0.11.7.

[Ryan J Ollos]

In ([12108]) Refs #10263: Made notice for deletion of a field different than notice for deletion of a ticket change, to aid in debugging.

[Chris Nelson]

With the latest in my environment, I still see an error. The log says:

2012-10-04 08:13:07,839 Trac[main] ERROR: Internal Server Error: 
Traceback (most recent call last):
  File "/usr/local/lib/python2.6/dist-packages/Trac-0.11.6-py2.6.egg/trac/web/main.py", line 455, in _dispatch_request
    dispatcher.dispatch(req)
  File "/usr/local/lib/python2.6/dist-packages/Trac-0.11.6-py2.6.egg/trac/web/main.py", line 206, in dispatch
    resp = chosen_handler.process_request(req)
  File "/usr/local/lib/python2.6/dist-packages/Trac-0.11.6-py2.6.egg/trac/admin/web_ui.py", line 114, in process_request
    path_info)
  File "build/bdist.linux-x86_64/egg/ticketdelete/web_ui.py", line 132, in render_admin_panel
    self._delete_change(t.id, ts, field)
  File "build/bdist.linux-x86_64/egg/ticketdelete/web_ui.py", line 222, in _delete_change
    oldval = [old for _, _, field2, old, _, _ in ticket.get_changelog(to_datetime(int(ts))) if field2 == field][0]
IndexError: list index out of range

[Chris Nelson]

The problem line is:

oldval = [old for _, _, field2, old, _, _ in ticket.get_changelog(to_datetime(int(ts))) if field2 == field][0]

and the only index there is 0 so that suggests that the get_changelog() is returning no data.

[Chris Nelson]

Trying to narrow it down, I made a local change:

220,222c220,232
<                 if field != 'comment' and not [1 for time, _, field2, _, _, _ in ticket.get_changelog()
<                                                if to_timestamp(time) > int(ts) and field == field2]:
<                     oldval = [old for _, _, field2, old, _, _ in ticket.get_changelog(to_datetime(int(ts))) if field2 == field][0]    
---
>                 if field != 'comment' \
>                         and not [1 for time, _, field2, _, _, _ \
>                                      in ticket.get_changelog()
>                                  if to_timestamp(time) > int(ts) \
>                                      and field == field2]:
>                     self.env.log.debug('ts:%s', ts)
>                     self.env.log.debug('  as int:%s' % int(ts))
>                     self.env.log.debug('  as dt:%s' % to_datetime(int(ts)))
>                     changes = ticket.get_changelog(to_datetime(int(ts)))
>                     self.env.log.debug('%s changes found' % len(changes))
>                     old = [old for _, _, field2, old, _, _ in changes  if field2 == field]
>                     self.env.log.debug('%s changes to %s' % (len(old), field))
>                     oldval = old[0]    

and my log says:

2012-10-04 08:25:57,046 Trac[web_ui] DEBUG: ts:1348502158
2012-10-04 08:25:57,046 Trac[web_ui] DEBUG:   as int:1348502158
2012-10-04 08:25:57,046 Trac[web_ui] DEBUG:   as dt:1970-01-01 00:22:28.502158+00:00
2012-10-04 08:25:57,048 Trac[web_ui] DEBUG: 0 changes found
2012-10-04 08:25:57,048 Trac[web_ui] DEBUG: 0 changes to confidence

My database shows:

trac=# select * from ticket_change where field = 'confidence';
 ticket |       time       | author |   field    | oldvalue | newvalue 
--------+------------------+--------+------------+----------+----------
    233 | 1348502158986154 | chrisn | confidence |          | High
    259 | 1349290421652076 | chrisn | confidence |          | Medium
(2 rows)

And I guess because we think this may be a millisecond patch problem, I'll show you:

trac=# \d ticket_change;
  Table "public.ticket_change"
  Column  |  Type   | Modifiers 
----------+---------+-----------
 ticket   | integer | not null
 time     | bigint  | not null
 author   | text    | 
 field    | text    | not null
 oldvalue | text    | 
 newvalue | text    | 
Indexes:
    "ticket_change_pk" PRIMARY KEY, btree (ticket, "time", field)
    "ticket_change_ticket_idx" btree (ticket)
    "ticket_change_time_idx" btree ("time")

Indeed, the ts variable from the log is the prefix of the time field in the db.

Hope this helps.

[anonymous]

Right. What about the potential microsecond timestamp patch issue i raised?

[Chris Nelson]

Our production version of the plugin has this local patch:

0.11/ticketdelete/web_ui.py

@@ -13 +13 @@
 from trac.web.chrome import ITemplateProvider
 from trac.ticket.web_ui import TicketModule 
 from trac.util import sorted
-from trac.util.datefmt import to_datetime, utc, to_timestamp
+from trac.util.datefmt import to_datetime, utc, to_utimestamp
 
 import re
 import traceback
@@ -102 +102 @@
 
                     ticket_data = {}
                     for time, author, field, oldvalue, newvalue, perm in t.get_changelog():
-                        c_data = ticket_data.setdefault(to_timestamp(time), {})
+                        c_data = ticket_data.setdefault(to_utimestamp(time), {})
                         c_data.setdefault('fields', {})[field] = {'old': oldvalue, 'new': newvalue}
                         c_data['author'] = author
                         # FIXME: The datetime handling is not working - enable
@@ -194 +194 @@
                 cursor.execute("DELETE FROM attachment WHERE type = 'ticket' AND id = %s AND time = %s", (id, ts))
             else:
                 custom_fields = [f['name'] for f in ticket.fields if f.get('custom')]
-                if field != "comment" and not [1 for time, author, field2, oldval, newval, _ in ticket.get_changelog() if to_timestamp(time) > int(ts) and field == field2]:
+                if field != "comment" and not [1 for time, author, field2, oldval, newval, _ in ticket.get_changelog() if to_utimestamp(time) > int(ts) and field == field2]:
                     oldval = [old for _, _, field2, old, _, _ in ticket.get_changelog(to_datetime(int(ts))) if field2 == field][0]
                     if field in custom_fields:
                         cursor.execute("UPDATE ticket_custom SET value=%s WHERE ticket=%s AND name=%s", (oldval, id, field))

which clearly affects the code that isn't working in the revision.

[Chris Nelson]

When I apply our local patch to the r12112, it works.

[anonymous]

Thanks. I will study the issue and get back to you later today.

[Ryan J Ollos]

I'm glad it's working for you now. If I may suggest a more minimal patch that will apply better after I commit additional refactoring,

from trac.util.datefmt import to_utimestamp as to_timestamp

[Chris Nelson]

Replying to rjollos:

I'm glad it's working for you now. If I may suggest a more minimal patch that will apply better after I commit additional refactoring,

from trac.util.datefmt import to_utimestamp as to_timestamp

I didn't know you could do that!?

Don't bother with more work for me. I can apply the local patch to your tip and be good until I move to Trac 1.0.

[Ryan J Ollos]

Okay, well I'll just commit my refactoring, and I found an interesting corner case that this plugin doesn't handle well. I'll open a ticket just so that you are aware of the issue. It will take some effort to fix correctly.

[Ryan J Ollos]

The next check-in should not functionally change anything, but the code is a lot cleaner and it will be easier to debug if we find any issues in the future. I'd like to make sure that I have all of the functionality restored since going to the Genshi-based version of the plugin, so if you could report back on that, then I will aim to get resolution on this ticket and move on to other things for a while.

[Ryan J Ollos]

Replying to rjollos:

The next check-in should not functionally change anything, but the code is a lot cleaner and it will be easier to debug if we find any issues in the future.

On second thought, this is not entirely correct. I improved the notice messages that are displayed when you delete fields. There is now a line in the notice for each field that is deleted, and you get a set of messages when deleting a group of changes, rather than the more generic "changes deleted" message.

The messages always look like this:

[Ryan J Ollos]

(In [12114]) Refs #10263: Refactored code and added comments. Improved notification messages on delete of a ticket change.

[Chris Nelson]

Without adding the microsecond patch to the 12114, I see a nice explanation when trying to delete a change:

Ticket change with timestamp 1348502158 (datetime: 1970-01-01 00:22:28.502158+00:00) not found in ticket #233 changelog.

[Chris Nelson]

With the microsecond patch applied, I can delete one or more changes and it works as expected.

[Ryan J Ollos]

Thanks for testing it out! Glad it is working well for you and we seem to have recovered from the earlier regressions.