r6919 breaks custom workflow set_owner operation!

[robert.nadler@…]

We use a custom workflow that has worked fine through r6912. After installing rc1, actions that have a set_owner operation always fail to commit the ticket change and kick you into preview mode with this banner:

Warning: 'user' is not a valid value for the owner field.

This happens no matter which user is selected.

[Eli Carter]

Can you test before and after r6919? That is the changeset I would first suspect. Also, if you can post your custom workflow, that may prove useful. I'll try to reproduce this locally regardless.

[robert.nadler@…]

I switched back to the r6912 version of web_ui.py (previously changed at r6907) and everything works fine.

Our workflow uses a custom permissions provider (CDIC_*) and some custom actions. All of the set_owner operations below fail with the warning when the r6919 version of web_ui.py is used.

assign = open -> assigned
assign.name = Assign: Set owner
assign.operations = set_owner
assign.permissions = CDIC_MANAGER
assign.default = 1
assigntester = fixed -> fixed
assigntester.name = AssignTester
assigntester.operations = set_owner
assigntester.permissions = CDIC_MANAGER
close = new,reviewed -> closed
close.name = Close
close.operations = set_resolution
close.permissions = CDIC_DBA
leave = * -> *
leave.default = 2
leave.name = Leave
leave.operations = leave_status
open = new -> open
open.name = Open
open.permissions = CDIC_DBA
openagain = recommend_close,reviewed -> open
openagain.name = OpenAgain
openagain.operations = del_owner
openagain.permissions = CDIC_MANAGER
reassign = assigned -> assigned
reassign.name = ReAssign
reassign.operations = set_owner
reassign.permissions = CDIC_MANAGER
recommendclose = tested,open,fixed,assigned -> recommend_close
recommendclose.name = RecommendClose
recommendclose.operations = del_owner
recommendclose.permissions = CDIC_TESTER
reopen = closed -> open
reopen.name = ReOpen
reopen.operations = del_resolution,del_owner
reopen.permissions = CDIC_DBA
reviewed = recommend_close,tested -> reviewed
reviewed.name = Reviewed
reviewed.operations = leave_status
reviewed.permissions = CDIC_DBA
testpassed = fixed -> tested
testpassed.name = TestPassed
testpassed.operations = del_owner
testpassed.permissions = CDIC_TESTER
unassign = assigned -> open
unassign.name = UnAssign
unassign.operations = del_owner
unassign.permissions = CDIC_DEVELOPER

[Eli Carter]

Using the given configuration without the permission settings, I'm not able to reproduce the problem you're seeing. (tested with r6941)

r6919 changed the ITicketManipulator interface to match the behavior it had in 0.10.*. At this point, I suspect the error you are getting is from a plugin that uses that interface. What plugins are you using? Can you disable all the plugins, and verify that the issue goes away? Assuming it does, then reenable them one by one to determine which is the culprit.

From there we should be able to get your system running again.

[robert.nadler@…]

Disabling these did not solve the problem.

BatchModify 0.2.0
CDICProjects (WikiMacroBase)
TracSVNHooks 0.1
TracTicketDelete 2.0
excel-report-plugin

I couldn't disable our CdicWorkflow plugin because it's the permissions provider for our workflow. It only implements ITicketActionController and IPermissionRequestor.

I enabled the DEBUG log and didn't see anything informative.

Our system is running fine with the older version of web_ui.py, but I'm willing to help diagnosis this problem.

[Eli Carter]

Can you comment out the permissions in the config, disable that plugin, and try reproducing with that?

Are you able to post your ITicketActionController part of that plugin? I'm not seeing any custom operations in your config; is your ITicketActionController handling all actions?

Hm. Part of r6919 also changed when the get_ticket_changes was called relative to populating the changes made to the ticket. If you are using ITicketActionController to try to validate that changes are OK, you'll want to use the ITicketManipulator interface instead. Alternatively, make an argument why the _populate() call should be done before the get_ticket_changes() calls.

[robert.nadler@…]

I've attached our workflow plugin (CdicWorkflow.py). Most of the workflow is done through the standard trac.ini defined actions. The plugin is used to copy developer and tester custom fields to the owner for status changes between assigned/fixed/tested.

As you'll see, the plugin does not validate changes from the standard workflow.

Now that you have everything, you'll hopefully be able to reproduce the problem.

[Eli Carter]

Looking at your plugin, I'd suggest a different approach. If you make a set_owner_to_field operation, and had it understand a .owner_to_field option, the workflow you have implemented in this plugin can then be described in the .ini, and other workflows can as well.

The change in the location of the _populate() call may still be an issue; you wouldn't be able to set the developer field in the ticket and use the operation in the same step. If you move the _populate() call to just after the elif req.method == 'POST' line, does your problem go away?

[robert.nadler@…]

I've never heard of the set_owner_to_field operation. It's not in the documentation or default_workflow.py. In any case, we needed the action labels and behavior to change based on the custom field contents.

Our workflow plugin does not have a _populate() call or a elif req.method == 'POST' line, so I'm not sure what you're referring to.

Were you able to reproduce the problem with this workflow and plugin?

[Eli Carter]

Apparently my explanation was confusing; you weren't the only one confused.

The _populate() call I was talking about is in trac/ticket/web_ui.py in the Trac core. The call to that method used to occur before the call to all the get_ticket_changes() methods, but when fixing #6879, I moved the _populate() call to after that point [6919]. This means that the workflow plugins do not currently see the changes the user made, but they used to. I expect I will need to change it back to calling _populate() first.

As for my suggestion about a different approach, that was a suggestion about a different way to write your plugin. It would have required implementing a set_owner_to_field operation, and then using that. However, I went ahead and implemented that and a couple other operations as a plugin, ​AdvancedTicketWorkflowPlugin

Using that, you should be able to move your business logic out of the plugin, and into your trac.ini.

I have not tried reproducing the problem with your plugin yet.

[Eli Carter]

I convinced myself the _populate() call needs to move back to where it was before [6919], so that is back to the way it was. Done in 0.11-stable [7014] and trunk [7016]. Please verify that fixes the behavior of your current plugin. Then rework your workflow using the set_owner_to_field operation from the ​AdvancedTicketWorkflowPlugin.

[Eli Carter]

I made an attempt to reproduce the reported problem, but without success.

• Updated to trunk@6919
• Replaced tab characters in CdicWorkflow.py with 8 spaces.
• Replaced [ticket-workflow] section with provided config.
• Add CDICActionController to [ticket] workflow.
• Added custom fields:

  [ticket-custom]
  developer = text
  developer.label = CDIC developer
  tester = text
  tester.label = CDIC tester
  

• Logged in as user with TRAC_ADMIN.
• Set developer and tester fields to other users (alice and bob)
• Tried various actions such as AssignTester, TestFailed, etc.
• Did not reproduce the problem.

[Eli Carter]

At this point, the workflow you want to implement should be doable with ​wiki:AdvancedTicketWorkflowPlugin using its set_owner_to_field operation, and the IPermissionRequestor portion of your plugin.

If I don't hear anything on this, I'll close this ticket next week.

[robert.nadler@…]

It's too bad you weren't able to reproduce the problem. Your tests were probably complete, but just to be sure: The problem I had was with workflow actions (like assign) that had a set_owner operation, not the custom actions.

The new AdvancedTicketWorkflowPlugin looks great. Unfortunately we just completed a full process validation with our custom workflow and went live last week. Changes to the workflow implementation of this magnitude would require a significant re-validation. If I even suggested that to my team this early-on I'm sure they'd string me up. "If it ain't broke, don't fix it" will have to do for now.

Thanks for all your help on this!

[Eli Carter]

I'm pretty sure I did cover that in my testing; so I'm going to close this.