State changes are populated AFTER triggering validate_ticket

[mdraghi@…]

When processing a ticket request, trac 0.11b1 calls validate_ticket() BEFORE populating the state (and other fields) changes related to the workflow in place. Thus, plug-ins implementing ITicketManipulator no longer has access to the real state of the ticket, because some of the fields (state, owner, …) can be changed after the validation.

This is a regression and a change on behaviour from the 0.10.x branch, which should be fixed or CLEARLY be documented as an API change. All our custom ITicketManipulator plug-ins that perform some validation when the ticket is being closed are broken on 0.11b1, because the ticket state is still "new" when the plug-in are called.

I think either TicketModule._process_ticket_request() should call _validate_ticket() AFTER calling _apply_ticket_changes(), or ITicketManipulators should be able to interact with the workflow and get a list of field changes.

[Eli Carter]

I agree this is a change and a regression that needs to be fixed. Unfortunately, it isn't as simple as moving the _apply_ticket_changes() call; it'll be a bit more invasive, I fear. I'm working on it.

[Christian Boos]

I'd prefer to keep things simple and the two phases distinct, if possible:

• ITicketManipulator: validate non-workflow related fields. This is still in line with the current API doc for that interface:

Validate a ticket after it's been populated from user input.

• ITicketActionController: use get_ticket_changes (which is also called in preview mode) to validate the ticket according to the new state and other changes due to the workflow. Again, this is conformant to the API doc for that interface:

This is mainly about controlling the changes to the ticket status, though not restricted to it.

We only have to check if it's possible to write ITicketActionController which are doing consistency checks only. I believe it's doable but a sample plugin illustrating this would be better. Does someone have an interesting use case?

[Eli Carter]

One affected plugin is ​WorkLogPlugin

[trac@…]

I've also been stung by this.

Just discussed with retracile on IRC.

I develop the ​th:WorkLogPlugin and as I adapt it more to suit 0.11 paradigms I want to integrate it into the new workflow.

So I define a state as the "in work" state (let's go out on a limb and call it "inwork"). If a user attempts to change the state of a ticket to inwork when they are already committed to working on another ticket I can take one of three actions.

1. Start Work anyway - e.g. Multitask.
2. Stop work on the other ticket in the background (e.g. change it's state) and accept the state transition for the current ticket.
3. Prevent the change from happening on the current ticket and give the user a warning.

Currently, the code does not allow multi-tasking (it's tightly integrated with clients and billing in our organisation so it's not really appropriate for our use of Trac tickets in this regard). I do however ofer a configuration option to chose which of the remaining two options is preferred (it's a trac-wide config right now but that's beside the point).

In an ideal world, I would just implement ITicketManipulator and investigate the state transitions in validate_ticket and take appropriate action. I've patched Trac to make this work (i.e. call the validate methods later on after applying the workflow changes), but as retracile said on IRC this may conflict with the preview code later on.

So I guess I could implement the ITicketActionController but I don't really want to do this as I don't want to poke about in a code-based fashion with the workflow, I want to leave that flexible for the users to choose as appropriate for them.

Personally, as the workflow can change the owner, status and resolution of a ticket, I feel it really should be passed to the validate_ticket method. The simplest solution I can see here that would not interfere with the preview but would be to simply clone the ticket prior to applying the changes and if everyuhting is good, then use this moving forward. If there are errors then they will be reported but the preview will be exactly as it is now.

I'll attach a patch that does this for review.

[trac@…]

Patch to demonstrate a fix (albeit in a particularly inelegant way)

[trac@…]

Here is an ever so slightly nicer version of the patch. It's not much different but makes more appropriate use of copy()

[Eli Carter]

A minimal fix committed in [6919]. This does open up some issues for workflow actions that modify fields the user can also modify; see #7176