CommitTicketUpdater should call manipulators and listeners

[osimons]

I don't use this hook myself, but I frequently get questions about it on IRC and try to help out. One very frequent request is some or handling for additional fields or related plugins that need to act on what gets updated on the ticket.

Example can be also adjusting estimates or time spent on fixing bugs, or updating some other related fields and structures.

Anyway, I quickly scanned the source and see that we don't call ticket system manipulators or listeners? That would provide the hooks to either change fields directly together with commit-update, or be handle updating of related information after-the-fact via listener.

Any reason not to call manipulators and listeners?

[Remy Blank]

The listeners are called by the model in save_changes(), so they should be called when a ticket is updated by the commit updater.

To be honest, the manipulators were forgotten. Looking at the manipulator interface, the value of calling them seems rather limited: prepare_ticket() is advertised as "not currently called". We could call validate_ticket(), and not update the ticket if it doesn't validate. Is that the use case you were thinking of?

[osimons]

Replying to rblank:

The listeners are called by the model in save_changes(), so they should be called when a ticket is updated by the commit updater.

Really? Huh. So we do. Surprised that we do actually, it seems the wrong layer to me but I suppose that is why we never could manage to turn ticket notification into a listener seeing it comes completely without context or request information. Anyway, a topic for some other day so forget it for the scope of this ticket :-)

We could call validate_ticket(), and not update the ticket if it doesn't validate. Is that the use case you were thinking of?

It would allow the additional hook into possibly rejecting the change if some plugin so wish, but I think this would be a very rare use-case.

My main interest is that the manipulator gets passed the actual ticket object BEFORE it is saved. Other plugins could then adjust custom fields or whatever to align with information in the commit message as they wish. It will then be part of the same change when you save the ticket.

A snippet like "-2h" could for instance deduct 2 hours from the current estimate of the task. Or anything really, I'm sure you get the point.

[Remy Blank]

Replying to osimons:

My main interest is that the manipulator gets passed the actual ticket object BEFORE it is saved. Other plugins could then adjust custom fields or whatever to align with information in the commit message as they wish. It will then be part of the same change when you save the ticket.

That makes sense. So we should actually start calling prepare_ticket() (and remove the bad advertisement). I'm not sure what the initial intention was with that method, and the signature hints that it's possibly not the right function for this use case: prepare_ticket(req, ticket, fields, actions). fields is probably intended to be a dict of new field values, but I wouldn't know what to pass in actions.

We could also abuse validate_ticket(), but that feels wrong.

Christian, any comments on this?

[doug]

Replying to rblank:

We could also abuse validate_ticket(), but that feels wrong.

Like this?

from trac.core import *
from trac.ticket.api import ITicketManipulator

class ZeroHoursPlugin(Component):
  implements(ITicketManipulator)
  
  def prepare_ticket(self, req, ticket, fields, actions):
    pass
    
  def validate_ticket(self, req, ticket):
    if ticket['status'] == 'closed':
      ticket['remaining_time'] = 0
    return []

Any suggestions on how to do this more cleanly?

[Remy Blank]

Replying to doug:

Like this?

Yes, absolutely. And now that I see it in code, it feels even more wrong :)

[osimons]

Heh. Well, it isn't all wrong… :-)

It is called ITicketManipulator as it was intended to also manipulate the instance, and not just possibly veto changes. The naming of the method could of course be discussed, but you could also argue that the 'remaining_time' example provides a validation - it updates the information to assure the integrity of its own structures. If some validator for some reason can't do that, it needs to veto the change and raise the issue. Adjusting field content can be seen as just another aspect of 'validation', IMHO.

[Remy Blank]

I'm actually fine with the idea, only the method name is hurting my eyes. So let's start with calling validate_ticket() from the commit updater, and allow it to prevent adding the comment if the "validation" fails (and log the messages).

We can also add a call to prepare_ticket() if desired, once we find out what the call was intended for.

[osimons]

Replying to rblank:

So let's start with calling validate_ticket() from the commit updater

Yes.

We can also add a call to prepare_ticket() if desired, once we find out what the call was intended for.

No. Tomorrow it will be exactly 5 years since it was added to the API in [3145], and not once has it actually been used. I'd rather we removed it from the API completely.

[Christian Boos]

The problem with validate_ticket() modifying the ticket fields is that in a sequence of validation involving for example two Components, the latter might well modify the ticket in such a way that the former would have declared it invalid. Hence I think it's better to have a process in two steps, with a first method allowing for modifications, the other doing strict validation on "read-only" data. So we might find a use for this prepare_ticket method ;-)

[osimons]

Christian; I think you are constructing a problem that only complicates the API. It has worked like this for 5 years. With the loosely-coupled extension interface we have, it isn't all that hard to image how one component may interfere with another. The mechanism we already have for this is to add an OrderedExtensionsOption for them so that the order can be influenced if needed.

Anyway, to see how ticket manipulators were called today I found that it isn't attached to the api.TicketSystem as I had assumed, but web.TicketModule. Then it all came back to me - I have been here before when making the RPC ticket update code compatible with 0.11 listeners, manipulators, workflow and notification… The non-trivial steps needed to update a ticket correctly via interal API can be seen here:

​trachacks:source:/xmlrpcplugin/trunk/tracrpc/ticket.py@9912#L184

It is not pretty… :-)

[doug]

Replying to rblank:

We can also add a call to prepare_ticket() if desired, once we find out what the call was intended for.

source:sandbox/workflow/trac/ticket/api.py@3378#L99​ source:sandbox/workflow/trac/ticket/web_ui.py@3378#L523​ WorkFlow#ProposedUnifiedFlowchartExtensionPoints The original intention seems to have been preparing a ticket for render, rather than modification of the saved ticket data.

[Christian Boos]

As you probably found out as well by digging in the history, the original intentions were never really clearly explained. Anyway they're probably not that relevant anymore, so I'd rather model this manipulator interface after the wiki one, IWikiPageManipulator​.

[Christian Boos]

Replying to osimons:

… The non-trivial steps needed to update a ticket correctly via interal API can be seen here: […] It is not pretty… :-)

No question about this, but instead of working around these deficiencies in this plugin, it might have been a better long term investment to try to fix that in the ticket module itself.

Said differently: if the XMLPRC code hasn't been a plugin but rather part of the core (or even tracopt?), would you have done it the same way, or would you have tried to refactor the code?

[osimons]

Replying to cboos:

…so I'd rather model this manipulator interface after the wiki one…

Interesting. We call manipulator.prepare_wiki_page() exactly once in 0.12.x and that is in wiki.web.WikiModule when rendering the page - ie. a read-only situation that only has effect for wiki_view.html template output.

/me notes todo for RPC plugin to remember to call the manipulator to 'prepare' before doing wiki.getPageHTML()…

[osimons]

Replying to cboos:

Said differently: if the XMLPRC code hasn't been a plugin but rather part of the core (or even tracopt?), would you have done it the same way, or would you have tried to refactor the code?

Oh yes. Quite different. The way I do it now is a strictly layered model where any domain logic is always shared between outputs:

• 'model' is basic store & fetch, with close to no logic other than to ensure data integrity. It contains all SQL (not even a SELECT is allowed in higher levels).
• 'core' is where I implement the internal API, and this layer also handles permissions, resources, listeners, manipulators, notification, syntax providers, attachments and so on.
• 'web_ui' translates and serves web requests, add scripts, handles ajax and so on, but calls to core domain API to do any fetching or manipulating. It also currently contains implementations for search and timeline providers that are still tied to web-output in Trac (another discussion :-).
• 'rpc' is a quite thin layer that just documents and translates the core domain api into a public api where needed.

A plugin based on this method can be seen at ​https://www.coderesort.com/p/open/browser/trac-talkplugin/trunk - it provides a fair illustration, and if you skim the various modules in the source it is fairly obvious what they do. It is a full-scale module plugin much like the wiki and ticket modules in Trac, so it's quite easy to relate it to the extended discussion in this ticket.

As I've argued before, adding an RPC interface to Trac core would force us to redo all modules to support web and rpc equally well by disentangling the current code and making cleaner layers. No minor task really, which makes status-quo tweaks the easy option…

[Christian Boos]

Replying to osimons:

Replying to cboos:

Said differently: if the XMLPRC code hasn't been a plugin but rather part of the core (or even tracopt?), would you have done it the same way, or would you have tried to refactor the code?

Oh yes. Quite different. The way I do it now is a strictly layered model where any domain logic is always shared between outputs:

• 'model' is basic store & fetch, with close to no logic other than to ensure data integrity. It contains all SQL (not even a SELECT is allowed in higher levels).
• 'core' is where I implement the internal API, and this layer also handles permissions, resources, listeners, manipulators, notification, syntax providers, attachments and so on.

Yes, the branch I have started for #1132 faces the same needs.

• 'web_ui' translates and serves web requests, add scripts, handles ajax and so on, but calls to core domain API to do any fetching or manipulating. It also currently contains implementations for search and timeline providers that are still tied to web-output in Trac (another discussion :-).
• 'rpc' is a quite thin layer that just documents and translates the core domain api into a public api where needed.

Sure, that makes sense.

…

As I've argued before, adding an RPC interface to Trac core would force us to redo all modules to support web and rpc equally well by disentangling the current code and making cleaner layers. No minor task really, which makes status-quo tweaks the easy option…

I didn't know you were eventually interested to moving the RPC interface to Trac core. I also think this could help us to better restructure the current code.

[Remy Blank]

Here's a start, calling validate_ticket() before saving:

tracopt/ticket/commit_updater.py

@@ -57 +57 @@
 from trac.wiki.macros import WikiMacroBase
 
 
+class ValidationFailed(Exception):
+    """Exception thrown when ticket validation fails."""
+    def __init__(self, errors):
+        super(ValidationFailed, self).__init__()
+        self.errors = errors
+
+
 class CommitTicketUpdater(Component):
     """Update tickets based on commit messages.
     
@@ -216 +223 @@
                     ticket = Ticket(self.env, tkt_id, db)
                     for cmd in cmds:
                         cmd(ticket, changeset, perm(ticket.resource))
+                    self._validate(ticket)
                     ticket.save_changes(changeset.author, comment, date, db)
                 self._notify(ticket, date)
+            except ValidationFailed, e:
+                self.log.error("Validation failed on ticket #%d:\n  %s",
+                               tkt_id, '  \n'.join(e.errors))
             except Exception, e:
                 self.log.error("Unexpected error while processing ticket "
                                "#%s: %s", tkt_id, exception_to_unicode(e))
     
+    def _validate(self, ticket):
+        """Validate the modified ticket."""
+        errors = []
+        for manipulator in TicketModule(self.env).ticket_manipulators:
+            for field, message in manipulator.validate_ticket(None, ticket):
+                if field:
+                    errors.append("Invalid field '%s': %s" % (field, message))
+                else:
+                    errors.append(message)
+        if errors:
+            raise ValidationFailed(errors)
+        
     def _notify(self, ticket, date):
         """Send a ticket update notification."""
         if not self.notify:

Sadly, validate_ticket() takes a req as its first argument. I pass None above, which is clearly wrong, but what should I pass instead? We're not in a web request context, so either we use a fake Request, or we fix the ITicketManipulator interface so that req is optional.

Thoughts?

[osimons]

Thanks for picking this up.

The req is an integral part of all methods belonging to workflow too, and at some stage someone will no doubt request that the updater supports states and actions and applies side-effects from 'close' operation too.

Some methods work fine with req passed as None. Some don't, and for some plugins None may be both unexpected and plain wrong. Educating plugin authors to expect None as input may be OK for lack of better alternative, but some uses may really need some of the attributes we attach to req like .href, .perm, .authname and so on.

I don't think it is a good idea that each need creates it's own DummyRequest as it sees fit, as plugins potentially may have to deal with different dummy requests that contains varying bits of information. A Trac OfflineRequest class should support a minimum of required arguments that should always be populated - like required arguments to OfflineRequest.__init__(). That way if a future version of Trac requires more attributes, any plugin will be forced to update its usage. When receiving a req input you can at least always be sure of a set of basic attributes but will have to test for existence of others.

Outside of what is likely needed by commit updater, the req usage gets somewhat muddled by the Context and RenderingContext abstrations too. It is not quite clear to me what spawns and inherits data here, but the content of a dummy req may have quite far-reaching consequences. Christian can no doubt elaborate on this.

[Christian Boos]

Well, the RenderingContext is really there to help you to generate content the appropriate way (only show information you're allowed to see, links rooted with the correct "base", nature of the content fitting the expected mime-type, etc.), so it clearly doesn't fit here as a possible replacement for req.

OTOH, the Request is there to say "do something" (by whom, in which special way according to the parameters, to the session preferences, …). The problem is that it is currently tied to the web_ui as if this would be the only possible way to trigger something. We already have the trac-admin CLI, and we could imagine GUI on top of the Trac API, etc. So in the longer run it would make sense to have an AbstractRequest (or RequestBase) class not tied to the trac.web API but still with some basic attributes (perm, args and session come to mind, not sure yet about the others). Then indeed an OfflineRequest subclass could add more attributes (href, chrome) to "fake" a web-like behavior for the unsuspecting plugins.

[osimons]

Replying to cboos:

Well, the RenderingContext is really there to help you to generate content the appropriate way (only show information you're allowed to see, links rooted with the correct "base", nature of the content fitting the expected mime-type, etc.), so it clearly doesn't fit here as a possible replacement for req.

Right, I wasn't suggesting it should replace req. What I meant was that if req at some stage down the processing of a ticket change ends up in a workflow effect or side-effect that decides to render some content or perhaps send a notification, the necessary context + rendering context + formatter will all carry with it whatever we pass down with the req in the first place.

[Remy Blank]

Soo… For this particular issue, what's the suggested way forward? I'm trying to finish my 0.13 tickets, and for this one I'm at a loss.

[osimons]

We need a req. The way ticket validation, workflow and listeners currently works, passing None is just asking for trouble - and plugins will naturally start to hack their own 'fixes' for the missing information…

[Remy Blank]

Unclear how to move forward, moving post-1.0.

[ethan.jucovy@…]

Replying to osimons:

We need a req. The way ticket validation, workflow and listeners currently works, passing None is just asking for trouble - and plugins will naturally start to hack their own 'fixes' for the missing information…

I've been looking into the uses of the req object that's passed in to ITicketManipulator and ITicketActionController implementations that I know of on trac-hacks and elsewhere. In almost all cases, the only properties that are accessed on req in any of these implementations are req.authname, req.args, and req.perm. There are very rare exceptions for things like chrome.add_warning(req, [...]) but so far I haven't found any use of req.href or req.chrome, and only very rare use of req.session or any other request properties that are conceptually coupled with a web request.

Based on those observations, I think that an AbstractRequest or a DummyRequest would be overkill. The interfaces could just take individual arguments for the properties that are most likely to be used, which the caller could either get from the request, or construct on its own, depending on its context.

In ticket #11424 I proposed changing the ITicketActionController interface to replace the req in its method signatures with a combination of individual arguments authname (a string), args (a dict), and perm. I think it might make sense to do this for ITicketManipulator as well. If I'm understanding the discussion here correctly, those two modifications would help this ticket to move forward.

(ITicketManipulator implementations that I've looked through are the ones in:

• ​th:BlackMagicTicketTweaksPlugin
• ​​th:MasterTicketsPlugin
• ​​th:TracHoursPlugin
• ​​th:SubticketsPlugin
• ​​th:TimingAndEstimationPlugin
• ​th:TaskListPlugin
• ​th:BudgetingPlugin
• ​th:SensitiveTicketsPlugin
• ​th:ChildTicketsPlugin
• ​th:TracTicketConditionalValidatePlugin
• ​th:InputfieldTrapPlugin
• ​th:GeoTicketPlugin — this does use req.session in one place :-(
• ​th:MathCaptchaPlugin — this does use a number of properties that are only meaningful in a web request, but most of them are used only for server-side error logging: (req.remote_addr, req.remote_user, req.base_path, req.path_info); it also uses req.remote_addr for one essential piece of functionality

I'd like to look a bit more closely at the code of the last two plugins on this list, to see whether it would be possible to cleanly implement their features without access to a req in their validate_ticket methods.)

[anonymous]

I do not think that the ITicketManipulator interface should be called upon in this context as it is meant for processing requests to the ticket system.

Instead, I believe that simply changing the ticket and letting the ITicketChangeListenerS do their work would be the better solution and keep the existing interface intact.

Besides that, the ITicketManipulatorS would not care about changes applied to the ticket because of a commit, as these are normally only: close ticket (set to resolved) and add a comment, so they must not validate.

Rather, the ticket workflow must be obeyed and this is not the case when looking through the existing code.

[anonymous]

Replying to anonymous:

I do not think that the ITicketManipulator interface should be called upon in this context as it is meant for processing requests to the ticket system.

And, looking at the existing commit_updater code, this was actually resolved a long time ago and any dependency on the ITicketManipulatorS have been removed and the system has been working happily ever since.

Except for the neglected workflow, but then again, resolving a ticket is always a good thing and every workflow should obey that.