Remove `req` from ITicketActionController method signatures

[ethan.jucovy@…]

Currently all methods of the ITicketActionController interface require a req request parameter. This couples the ITicketActionController system tightly with web requests, and makes it very difficult to use workflow components at all in contexts other than a web request where the requesting user is directly viewing or acting on a ticket. This is because a Request object is difficult to instantiate directly (needs an environ and start_response); has an indeterminate interface (environ keys, request callbacks, etc); and must be capable of handling exceptional situations like req.send and req.redirect.

If this coupling were looser, it would become much easier to use workflow in situations like:

• workflow-aware actions triggered by commit messages (#10755)
• workflow-aware actions in ITicketChangeListener components that subscribe to ticket_change events, where a req is not available
• directly through any Python code that has a Trac environment, a ticket, and a (real or simulated) initiator for a workflow action

I looked through the code of the following ITicketActionController implementations:

• trac.ticket.default_workflow
• ​th:AdvancedTicketWorkflowPlugin
• ​th:WorkflowNotificationPlugin
• ​th:MultipleWorkflowPlugin
• ​th:TypedTicketWorkflowPlugin
• ​th:TestManagerForTracPlugin / ​th:TracGenericWorkflowPlugin

As far as I can see, the only request properties that are ever accessed in any of these implementations are req.perm, req.args and req.authname. There are only two exceptions, neither of which seems critical. (One is discussed below; the other is a hack in my own ​th:WorkflowNotificationPlugin which I'd prefer to clean up anyway.)

So, I'm proposing to replacing the req argument in ITicketActionController method signatures with individual arguments perm (a Permission object), args (a dict), and author (a string). An illustration is provided in the attached patch.

Unlike a Request object, these three parameters are trivial to construct from any calling context and to instantiate directly.

Aside from backwards compatibility concerns, the only potential problem I see, based on the documented interfaces and their implementations in the wild, is that it's recommended to use trac.web.chrome.add_warning(req, ...) within a component's get_ticket_changes method if args['preview'] is set and if the component will have any side effects in apply_action_side_effects. In practice, I can only find one use of this recommendation, in ​th:AdvancedTicketWorkflowPlugin's TicketWorkflowOpXRef component. This could be solved by extending the get_ticket_changes method with an optional add_warning callback/accumulator (which could be set to lambda *x: chrome.add_warning(req, *x) in web contexts, or e.g. a logging.warn call in other contexts) or by extending the interface with a get_ticket_change_warnings method.

[Ryan J Ollos]

In order to keep the API stable, instead of directly removing req, we could deprecate it in 1.2 for removal in 1.4. The signatures would be:

def render_ticket_action_control(self, req, ticket, action, perm=None, author=None, args=None):
     if req is None and not any(perm, author, args):
         raise TracError(...)

… and similarly for get_ticket_actions, get_ticket_changes, apply_action_side_effects. The any(perm, author, args) may not be the correct approach depending on which arguments should be required in various contexts. I haven't studied the code yet to try and figure out the correct way.

A caller wishing to use the "newer" signature would call it as:

render_ticket_action_control(None, ticket, action, perm, author, args):

What do you think?

[ethan.jucovy@…]

I just noticed there are a number of ITicketActionController implementations in the trac distribution's sample-plugins/workflow/ directory, which I hadn't looked through when I filed this ticket. From a quick glance through those files, it looks like the observation still holds true that (perm, authname, args) are all that are ever used in practice. There's also one more use of the get_ticket_changes⇒add_warning pattern. But I should remember to update my sample patch to include those sample plugins.

[ethan.jucovy@…]

#10125 is indirectly related to this ticket; the discussion there, starting around comment:17:ticket:10125, runs into the problem of a req being needed to call ITicketActionController methods, as well as ITicketManipulator needing a req in validate_ticket.

For what it's worth, an initial glance at some well-known ITicketManipulator implementations turns up similar information: almost no validate_ticket implementations use req at all, and the few uses I am seeing are limited to req.perm and req.args. (I looked at ​th:BlackMagicTicketTweaksPlugin, ​th:MasterTicketsPlugin, ​th:TracHoursPlugin, ​th:SubticketsPlugin and ​th:TimingAndEstimationPlugin to start.)

There's a proposal on #10125 to formalize a minimal request-like interface (AbstractRequest) and to build an OfflineRequest class that can be passed through these interfaces. That would be an alternate way of addressing the needs described here, but based on the usage patterns I'm seeing for both ticket manipulators and ticket action controllers, it seems like adjusting the interfaces to take individual arguments is a simpler solution to the same problem.

[anonymous]

ITicketManipulator is part of the IRequestHandler request handling process. And so is the ITicketActionController interface. See TicketModule#process_request and delegated to methods.

And, having the request object available for checking on the raw parameters and other stuff, for example request headers, is a nice to have feature.

So I am in favour of keeping the old interface intact.

[anonymous]

Replying to anonymous:

And, having the request object available for checking on the raw parameters and other stuff, for example request headers, is a nice to have feature.

My use case here would be having the ticket system accept tickets from automated systems rather than from a user. For this, the automation tool could set the header X-Trac-Ticket-Automaton and my ticket manipulator would recognize that and would add additional fields to the ticket from the otherwise not recognized other raw fields in the request and other stuff like that.

[anonymous]

@ethan: how about making the code that actually validates the ticket separatedly availabe in a different class or callable and then call that instead. That way, you can reuse the validation logic and call upon that validation logic from your ITicketManipulator by simply delegating to it, e.g.

class MyTicketManipulator implements ITicketManipulator:

  def ticket_validate(req, ... ticket):
    return MyTicketValidationLogic().validate(req.perms, ... ticket)


class MyTicketValidationLogic:

  def validate(perms, parms, ..., ticket):
     ...

That way, your proposed changes would not limit other users of the ITicketManipulator interface and you would only have to provide a small shim around your actual, and now reusable, validation logic.