Edgewall Software
Modify

Opened 7 years ago

Closed 7 years ago

Last modified 4 years ago

#12725 closed enhancement (fixed)

Validate ticket comment changes

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.3.2
Component: ticket system Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:
  • Ticket comment edits are validated. ITicketManipulators are called and [ticket] max_comment_size is enforced.
  • Added ticket warnings above the preview.
API Changes:
Internal Changes:

Description

Discussed in #12722.

_validate_ticket should probably be called because currently max_comment_size is not enforced for a comment edit: tags/trac-1.2/trac/ticket/web_ui.py#L1336. _validate_ticket calls ITicketManipulator, which would allow additional rules to be enforced for ticket comment changes.

ICommentManipulator (comment:2:ticket:12722) might provide a nicer interface, but I'm unsure if it's necessary.

As suggested in #12642 for ticket workflow warnings, and similar to what is discussed in #11269, the warnings should be rendered near the comment edit box.

Attachments (5)

Screen Shot 2017-03-15 at 11.52.41.png (265.3 KB ) - added by Ryan J Ollos 7 years ago.
Screen Shot 2017-03-15 at 11.52.17.png (126.4 KB ) - added by Ryan J Ollos 7 years ago.
Screen Shot 2017-03-15 at 11.55.49.png (113.8 KB ) - added by Ryan J Ollos 7 years ago.
Screen Shot 2017-03-15 at 12.00.49.png (117.2 KB ) - added by Ryan J Ollos 7 years ago.
Screen Shot 2017-04-02 at 03.57.13.png (70.0 KB ) - added by Ryan J Ollos 7 years ago.

Download all attachments as: .zip

Change History (19)

comment:2 by Peter Suter, 7 years ago

Some plugins (th:TracHoursPlugin, th:PullRequestsPlugin) implement ITicketManipulator mainly to manipulate / cause side-effects, not to validate. Will there still be a way for these plugins to avoid these side-effects on comment editing? (It looks like req.args.get('comment') might return None for comment editing, correct? That might conveniently keep them working as-is.)

comment:3 by Ryan J Ollos, 7 years ago

Yes, req.args.get('comment') returns None for comment editing. A plugin could also detect a comment edit by checking for 'edited_comment' in req.args.

Version 0, edited 7 years ago by Ryan J Ollos (next)

comment:4 by Ryan J Ollos, 7 years ago

Milestone: 1.2.11.3.2

I'll target next-major-release since there is impact on plugins.

by Ryan J Ollos, 7 years ago

by Ryan J Ollos, 7 years ago

by Ryan J Ollos, 7 years ago

by Ryan J Ollos, 7 years ago

comment:5 by Ryan J Ollos, 7 years ago

Part of #11269 will also be addressed in this ticket. I chose to place the warnings directly above the preview so that it doesn't interfere with the ticket property changes box. Here is an example of the interference:

With warnings placed above the preview:

For consistency I added a warning above the New Ticket preview:

For consistency I moved the edit conflict warnings and added a warning box for the Ticket Change preview:

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

comment:6 by Ryan J Ollos, 7 years ago

Proposed changes in log:rjollos.git:t12725_validate_comment.1. I will add test coverage before committing.

in reply to:  2 ; comment:7 by Ryan J Ollos, 7 years ago

Replying to Peter Suter:

Some plugins (th:TracHoursPlugin, th:PullRequestsPlugin) implement ITicketManipulator mainly to manipulate / cause side-effects, not to validate. Will there still be a way for these plugins to avoid these side-effects on comment editing? (It looks like req.args.get('comment') might return None for comment editing, correct? That might conveniently keep them working as-is.)

Documented in TracDev/PluginDevelopment/ExtensionPoints/trac.ticket.api.ITicketManipulator@7.

comment:8 by Ryan J Ollos, 7 years ago

Release Notes: modified (diff)
Status: assignedclosed

Committed to trunk in r15719.

comment:9 by Ryan J Ollos, 7 years ago

I was seeing an error during autopreview:

21:04:14 Trac[main] ERROR: [127.0.0.1] Internal Server Error: <RequestWithSession "POST '/newticket'">, referrer 'http://localhost:8000/proj-1.3/newticket'
Traceback (most recent call last):
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/web/main.py", line 636, in _dispatch_request
    dispatcher.dispatch(req)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/web/main.py", line 259, in dispatch
    method=method)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/web/chrome.py", line 1377, in render_template
    fragment, iterable, method)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/web/chrome.py", line 1469, in _render_jinja_template
    iterable)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/web/chrome.py", line 1583, in generate_template_stream
    bytes = template.render(data).encode('utf-8')
  File "/Users/rjollos/Documents/Workspace/trac-dev/pve/lib/python2.7/site-packages/jinja2/environment.py", line 1008, in render
    return self.environment.handle_exception(exc_info, True)
  File "/Users/rjollos/Documents/Workspace/trac-dev/pve/lib/python2.7/site-packages/jinja2/environment.py", line 780, in handle_exception
    reraise(exc_type, exc_value, tb)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/ticket/templates/ticket_box.html", line 25, in top-level template code
    ${jmacros.warnings(chrome.warnings)}
  File "/Users/rjollos/Documents/Workspace/trac-dev/pve/lib/python2.7/site-packages/jinja2/environment.py", line 430, in getattr
    return getattr(obj, attribute)
UndefinedError: 'jmacros' is undefined

Should be fixed in r15742.

by Ryan J Ollos, 7 years ago

comment:10 by Ryan J Ollos, 7 years ago

Another issue that I didn't notice prior to r15742: on autosubmit the div#ticket is replaced and the warnings are outside the div#ticket so another warning is added is added on each autosubmit.

The only way I can see to fix this is to add another div: [5db6a53bc/rjollos.git].

comment:11 by anonymous, 7 years ago

comment:10 changes committed in r15743.

comment:12 by Ryan J Ollos, 7 years ago

Resolution: fixed

comment:13 by Ryan J Ollos, 7 years ago

r15719 results in duplicated div#warning elements when the conflict warning is displayed on the page. The issue will be fixed in #12914.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from 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.