Edgewall Software
Modify

Opened 4 years ago

Last modified 4 years ago

#11424 new enhancement

Remove `req` from ITicketActionController method signatures

Reported by: ethan.jucovy@… Owned by:
Priority: normal Milestone: unscheduled
Component: ticket system Version:
Severity: normal Keywords:
Cc: Ryan J Ollos
Release Notes:
API Changes:

Description

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:

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.

Attachments (1)

reqless-action-controllers.diff (13.1 KB ) - added by ethan.jucovy@… 4 years ago.

Download all attachments as: .zip

Change History (7)

Changed 4 years ago by ethan.jucovy@…

comment:1 Changed 4 years ago by Ryan J Ollos

Cc: Ryan J Ollos added
Milestone: unscheduled

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?

comment:2 Changed 4 years ago by 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_changesadd_warning pattern. But I should remember to update my sample patch to include those sample plugins.

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

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

comment:5 in reply to:  4 Changed 4 years ago by 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.

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned.
as The resolution will be set.
The owner will be changed from (none) to anonymous.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.