Edgewall Software
Modify

Opened 12 years ago

Closed 12 years ago

#6879 closed defect (fixed)

State changes are populated AFTER triggering validate_ticket

Reported by: mdraghi@… Owned by: Eli Carter
Priority: high Milestone: 0.11
Component: ticket system Version: 0.11b1
Severity: major Keywords:
Cc: trac@… Branch:
Release Notes:
API Changes:

Description

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.

Attachments (2)

change_but_rollback_on_preview_or_failure.patch (2.4 KB ) - added by trac@… 12 years ago.
Patch to demonstrate a fix (albeit in a particularly inelegant way)
change_but_rollback_on_preview_or_failure.2.patch (2.6 KB ) - added by trac@… 12 years ago.
Here is an ever so slightly nicer version of the patch. It's not much different but makes more appropriate use of copy()

Download all attachments as: .zip

Change History (7)

comment:1 by Eli Carter, 12 years ago

Owner: changed from Jonas Borgström to Eli Carter
Status: newassigned

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.

comment:2 by Christian Boos, 12 years ago

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?

comment:3 by Eli Carter, 12 years ago

One affected plugin is WorkLogPlugin

comment:4 by trac@…, 12 years ago

Cc: trac@… added

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.

by trac@…, 12 years ago

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

by trac@…, 12 years ago

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

comment:5 by Eli Carter, 12 years ago

Resolution: fixed
Status: assignedclosed

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Eli Carter.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Eli Carter to the specified user.

Add Comment


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