Validate ticket comment changes

[Ryan J Ollos]

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.

[Ryan J Ollos]

WIP changes in log:rjollos.git:t12725_validate_comment.

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

[Ryan J Ollos]

Yes, req.args.get('comment') returns None for comment editing. I haven't checked, but it probably return an empty string for a ticket property change with no comment.

A plugin could also detect a comment edit by checking for 'edited_comment' in req.args.

[Ryan J Ollos]

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

[Ryan J Ollos]

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:

[Ryan J Ollos]

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

[Ryan J Ollos]

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.

[Ryan J Ollos]

Committed to trunk in r15719.

[Ryan J Ollos]

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.

[Ryan J Ollos]

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

[anonymous]

comment:10 changes committed in r15743.

[Ryan J Ollos]

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

[Ryan J Ollos]

Replying to Ryan J Ollos:

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

Corrected documentation in TracDev/PluginDevelopment/ExtensionPoints/trac.ticket.api.ITicketManipulator@8. See also #11723.