ConfigurableTicketWorkflow should not handle other operations

[Ryan J Ollos]

Issue reproduced by the following steps:

1. Enable CodeReviewActionController​
2. Put ticket in review state

The user that requested the review doesn't have permission to review the ticket​, but sees:

The user should see:

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t13013_ctw_other_ops. may_set_owner needs to be added to ConfigurableTicketWorkflow.operations on 1.2-stable.

[Jun Omae]

Looks good to me.

[Ryan J Ollos]

Thanks for reviewing. Committed in r16551, merged in r16552, r16553.

[Ryan J Ollos]

​th:#13484 resulted from these changes. Previously ConfigurableTicketWorkflow would be invoked for every action and status changes would be applied. After r16551, each ITicketActionController needs to apply status changes.

I'm considering modifying each controller implemented in ​AdvancedTicketWorkflow, with a change like the following for TicketWorkflowOpOwnerReporter:

advancedworkflow/controller.py

@@ -98 +98 @@
     def _format_author(self, req, author):
         return Chrome(self.env).format_author(req, author)
 
+    def _get_new_status(self, action):
+        actions = self.get_configurable_workflow().actions
+        return actions[action]['newstate']
+
 
 class TicketWorkflowOpOwnerReporter(TicketWorkflowOpBase):
     """Sets the owner to the reporter of the ticket.
@@ -127 +131 @@
 
     def get_ticket_changes(self, req, ticket, action):
         """Returns the change of owner."""
-        return {'owner': ticket['reporter']}
+        return {
+            'owner': ticket['reporter'],
+            'status': self._get_new_status(action),
+        }
 
 
 class TicketWorkflowOpOwnerComponent(TicketWorkflowOpBase):

[Ryan J Ollos]

To be clear about comment:5, I don't think the changes in r16551 are wrong. For a plugin like ​AdvancedTicketWorkflowPlugin it might be useful to have a way to add operations that use behaviors of ConfigurableTicketWorklow like implicit status change. I haven't taken the time to investigate, so I'm not sure exactly what form that takes, whether it's a base class or helper methods on the ConfigurableTicketWorklow component.

[Jun Omae]

Replying to Ryan J Ollos:

To be clear about comment:5, I don't think the changes in r16551 are wrong.

Reconsidering…

[ticket] workflow option normally accepts one or more of workflow controllers. If one of the controllers sets new status to a ticket, which of the controllers should set the status? All controllers should set? At least, I think that other controllers expect ConfigurableTicketWorkflow sets new status to a ticket even if the operation is not defined in the ConfigurableTicketWorkflow.

It might be good that the setting status should be ideally moved to TicketModule from ConfigurableTicketWorkflow.

[Ryan J Ollos]

Replying to Jun Omae:

[ticket] workflow option normally accepts one or more of workflow controllers. If one of the controllers sets new status to a ticket, which of the controllers should set the status? All controllers should set? At least, I think that other controllers expect ConfigurableTicketWorkflow sets new status to a ticket even if the operation is not defined in the ConfigurableTicketWorkflow.

It might be good that the setting status should be ideally moved to TicketModule from ConfigurableTicketWorkflow.

I've been considering that the status change behavior in ConfigurableTicketWorkflow might need to be an explicit operation, e.g. set_status.

CodeReviewActionController presents a complicated case in which the status is determined by an algorithm in the controller​. CodeReviewActionController wants to change the status using behavior different from ConfigurableTicketWorkflow's implicit "set_status" operation.

With set_status operation made explicit in ConfigurableTicketWorkflow, actions that rely on existing behavior could have the set_status operation added to operations in trac.ini. Operations that want more control over status, like code_review, could implement there own status change behavior.

I'm considering a simple revision to r16551 that would preserve behavior prior to r16551. Fixing this issue might involve simple changes on the stable branches and more complex changes on trunk.

I noticed that the hint "The/Next status will be …" is only added if no other operations are defined​. We may want to add it for all operations in which the status is changed.

[Ryan J Ollos]

⇒ #13086, #13087.

[Ryan J Ollos]

Related discussion in #13199.