Edgewall Software
Modify

Opened 3 years ago

Last modified 7 months ago

#10125 new enhancement

CommitTicketUpdater should call manipulators and listeners

Reported by: osimons Owned by: rblank
Priority: normal Milestone: next-major-releases
Component: ticket system Version: 0.12-stable
Severity: normal Keywords: CommitTicketUpdater
Cc: ethan.jucovy@…, rjollos
Release Notes:
API Changes:

Description

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?

Attachments (0)

Change History (29)

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

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?

comment:2 in reply to: ↑ 1 ; follow-up: Changed 3 years ago by 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.

comment:3 in reply to: ↑ 2 ; follow-up: Changed 3 years ago by rblank

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?

comment:4 in reply to: ↑ 3 ; follow-up: Changed 3 years ago by 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?

comment:5 in reply to: ↑ 4 Changed 3 years ago by rblank

Replying to doug:

Like this?

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

comment:6 Changed 3 years ago by 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.

comment:7 follow-ups: Changed 3 years ago by rblank

  • Milestone set to 0.13
  • Owner set to rblank

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.

comment:8 in reply to: ↑ 7 Changed 3 years ago by 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.

comment:9 Changed 3 years ago by cboos

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

comment:10 follow-up: Changed 3 years ago by 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… :-)

comment:11 in reply to: ↑ 7 Changed 3 years ago by 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.

comment:12 Changed 3 years ago by cboos

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.

comment:13 in reply to: ↑ 10 ; follow-up: Changed 3 years ago by cboos

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?

comment:14 Changed 3 years ago by 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()

comment:15 in reply to: ↑ 13 ; follow-up: Changed 3 years ago by 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…

comment:16 in reply to: ↑ 15 Changed 3 years ago by cboos

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.

comment:17 follow-up: Changed 3 years ago by rblank

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

  • tracopt/ticket/commit_updater.py

    diff --git a/tracopt/ticket/commit_updater.py b/tracopt/ticket/commit_updater.py
    a b from trac.wiki.formatter import format_t 
    5757from trac.wiki.macros import WikiMacroBase 
    5858 
    5959 
     60class ValidationFailed(Exception): 
     61    """Exception thrown when ticket validation fails.""" 
     62    def __init__(self, errors): 
     63        super(ValidationFailed, self).__init__() 
     64        self.errors = errors 
     65 
     66 
    6067class CommitTicketUpdater(Component): 
    6168    """Update tickets based on commit messages. 
    6269     
    In [%s]: 
    216223                    ticket = Ticket(self.env, tkt_id, db) 
    217224                    for cmd in cmds: 
    218225                        cmd(ticket, changeset, perm(ticket.resource)) 
     226                    self._validate(ticket) 
    219227                    ticket.save_changes(changeset.author, comment, date, db) 
    220228                self._notify(ticket, date) 
     229            except ValidationFailed, e: 
     230                self.log.error("Validation failed on ticket #%d:\n  %s", 
     231                               tkt_id, '  \n'.join(e.errors)) 
    221232            except Exception, e: 
    222233                self.log.error("Unexpected error while processing ticket " 
    223234                               "#%s: %s", tkt_id, exception_to_unicode(e)) 
    224235     
     236    def _validate(self, ticket): 
     237        """Validate the modified ticket.""" 
     238        errors = [] 
     239        for manipulator in TicketModule(self.env).ticket_manipulators: 
     240            for field, message in manipulator.validate_ticket(None, ticket): 
     241                if field: 
     242                    errors.append("Invalid field '%s': %s" % (field, message)) 
     243                else: 
     244                    errors.append(message) 
     245        if errors: 
     246            raise ValidationFailed(errors) 
     247         
    225248    def _notify(self, ticket, date): 
    226249        """Send a ticket update notification.""" 
    227250        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?

comment:18 in reply to: ↑ 17 Changed 3 years ago by 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.

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

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.

comment:20 in reply to: ↑ 19 Changed 3 years ago by 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.

comment:21 Changed 3 years ago by rblank

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.

comment:22 follow-up: Changed 3 years ago by 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…

comment:23 Changed 2 years ago by rblank

  • Milestone changed from 1.0 to next-major-0.1X

Unclear how to move forward, moving post-1.0.

comment:24 Changed 9 months ago by ethan.jucovy@…

  • Cc ethan.jucovy@… added

comment:25 in reply to: ↑ 22 Changed 9 months ago by 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:

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

comment:26 follow-up: Changed 7 months ago by 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.

comment:27 in reply to: ↑ 26 Changed 7 months ago by 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.

comment:28 Changed 7 months ago by jomae

  • Keywords CommitTicketUpdater added

comment:29 Changed 7 months ago by rjollos

  • Cc rjollos added

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new The owner will remain rblank.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from rblank to anonymous. Next status will be 'assigned'.
Author


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

 
Note: See TracTickets for help on using tickets.