Modify

Opened 2 years ago

Closed 2 years ago

#10263 closed defect (fixed)

Big, red Delete button in 0.11.6

Reported by: ChrisNelson Owned by: rjollos
Priority: normal Component: TicketDeletePlugin
Severity: normal Keywords: regression css
Cc: Trac Release: 0.11

Description

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

Attachments (5)

Delete failure.png (107.6 KB) - added by ChrisNelson 2 years ago.
Screen shot of failure to refresh after deletion
Change.png (7.0 KB) - added by anonymous 2 years ago.
ChangeDeleted.png (15.8 KB) - added by anonymous 2 years ago.
DeleteChange.png (16.9 KB) - added by anonymous 2 years ago.
DeletedChangesMessage.png (21.0 KB) - added by rjollos 2 years ago.

Download all attachments as: .zip

Change History (40)

comment:3 Changed 2 years ago by ChrisNelson

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

comment:6 Changed 2 years ago by rjollos

#10262 closed as a duplicate.

comment:7 Changed 2 years ago by rjollos

  • Owner changed from coderanger to rjollos
  • Status changed from new to assigned

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.

comment:8 Changed 2 years ago by rjollos

  • Keywords regression css added

comment:5 Changed 2 years ago by rjollos

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

comment:6 Changed 2 years ago by rjollos

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

comment:7 Changed 2 years ago by rjollos

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

comment:8 Changed 2 years ago by rjollos

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

comment:9 Changed 2 years ago by rjollos

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

Changed 2 years ago by ChrisNelson

Screen shot of failure to refresh after deletion

comment:10 Changed 2 years ago by ChrisNelson

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

comment:11 Changed 2 years ago by ChrisNelson

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

comment:12 follow-up: Changed 2 years ago by 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.

comment:13 in reply to: ↑ 12 ; follow-up: Changed 2 years ago by ChrisNelson

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?

comment:14 in reply to: ↑ 13 Changed 2 years ago by rjollos

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?

comment:15 Changed 2 years ago by ChrisNelson

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.

comment:16 Changed 2 years ago by rjollos

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

comment:17 Changed 2 years ago by 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.

Changed 2 years ago by anonymous

Changed 2 years ago by anonymous

Changed 2 years ago by anonymous

comment:18 Changed 2 years ago by rjollos

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

comment:19 Changed 2 years ago by rjollos



comment:20 Changed 2 years ago by ChrisNelson

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

comment:21 Changed 2 years ago by ChrisNelson

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.

comment:22 Changed 2 years ago by ChrisNelson

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.

comment:23 Changed 2 years ago by anonymous

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

comment:24 Changed 2 years ago by ChrisNelson

Our production version of the plugin has this local patch:

  • 0.11/ticketdelete/web_ui.py

    a b from trac.web.chrome import add_warning 
    1313from trac.web.chrome import ITemplateProvider
    1414from trac.ticket.web_ui import TicketModule
    1515from trac.util import sorted
    16 from trac.util.datefmt import to_datetime, utc, to_timestamp
     16from trac.util.datefmt import to_datetime, utc, to_utimestamp
    1717
    1818import re
    1919import traceback
    class TicketDeletePlugin(Component): 
    102102
    103103                    ticket_data = {}
    104104                    for time, author, field, oldvalue, newvalue, perm in t.get_changelog():
    105                         c_data = ticket_data.setdefault(to_timestamp(time), {})
     105                        c_data = ticket_data.setdefault(to_utimestamp(time), {})
    106106                        c_data.setdefault('fields', {})[field] = {'old': oldvalue, 'new': newvalue}
    107107                        c_data['author'] = author
    108108                        # FIXME: The datetime handling is not working - enable
    class TicketDeletePlugin(Component): 
    194194                cursor.execute("DELETE FROM attachment WHERE type = 'ticket' AND id = %s AND time = %s", (id, ts))
    195195            else:
    196196                custom_fields = [f['name'] for f in ticket.fields if f.get('custom')]
    197                 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]:
     197                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]:
    198198                    oldval = [old for _, _, field2, old, _, _ in ticket.get_changelog(to_datetime(int(ts))) if field2 == field][0]
    199199                    if field in custom_fields:
    200200                        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.

comment:25 Changed 2 years ago by ChrisNelson

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

comment:26 Changed 2 years ago by anonymous

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

comment:27 follow-up: Changed 2 years ago by 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

comment:28 in reply to: ↑ 27 Changed 2 years ago by ChrisNelson

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.

comment:29 Changed 2 years ago by rjollos

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.

comment:30 follow-up: Changed 2 years ago by 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. 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.

comment:31 in reply to: ↑ 30 Changed 2 years ago by rjollos

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:

Changed 2 years ago by rjollos

comment:32 Changed 2 years ago by rjollos

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

comment:33 Changed 2 years ago by ChrisNelson

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.

comment:34 Changed 2 years ago by ChrisNelson

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

comment:35 Changed 2 years ago by rjollos

  • Resolution set to fixed
  • Status changed from assigned to closed

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

Add Comment

Modify Ticket

Action
as closed The owner will remain rjollos.
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.