Edgewall Software
Modify

Opened 2 years ago

Closed 2 years ago

Last modified 12 months ago

#13013 closed defect (fixed)

ConfigurableTicketWorkflow should not handle other operations

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.0.17
Component: ticket system Version:
Severity: normal Keywords: workflow
Cc: Branch:
Release Notes:

ConfigurableTicketWorkflow skips actions that use operations defined by other ITicketActionControllers.

API Changes:
Internal Changes:

Description (last modified by 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:

Attachments (2)

Screen Shot 2018-04-21 at 12.03.41.png (10.4 KB ) - added by Ryan J Ollos 2 years ago.
Screen Shot 2018-04-21 at 12.04.02.png (8.7 KB ) - added by Ryan J Ollos 2 years ago.

Download all attachments as: .zip

Change History (12)

by Ryan J Ollos, 2 years ago

by Ryan J Ollos, 2 years ago

comment:1 by Ryan J Ollos, 2 years ago

Description: modified (diff)

comment:2 by Ryan J Ollos, 2 years ago

Release Notes: modified (diff)

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

comment:3 by Jun Omae, 2 years ago

Looks good to me.

comment:4 by Ryan J Ollos, 2 years ago

Resolution: fixed
Status: assignedclosed

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

comment:5 by Ryan J Ollos, 23 months ago

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

     
    9898    def _format_author(self, req, author):
    9999        return Chrome(self.env).format_author(req, author)
    100100
     101    def _get_new_status(self, action):
     102        actions = self.get_configurable_workflow().actions
     103        return actions[action]['newstate']
     104
    101105
    102106class TicketWorkflowOpOwnerReporter(TicketWorkflowOpBase):
    103107    """Sets the owner to the reporter of the ticket.
     
    127131
    128132    def get_ticket_changes(self, req, ticket, action):
    129133        """Returns the change of owner."""
    130         return {'owner': ticket['reporter']}
     134        return {
     135            'owner': ticket['reporter'],
     136            'status': self._get_new_status(action),
     137        }
    131138
    132139
    133140class TicketWorkflowOpOwnerComponent(TicketWorkflowOpBase):

comment:6 by Ryan J Ollos, 23 months ago

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.

Last edited 23 months ago by Ryan J Ollos (previous) (diff)

in reply to:  6 ; comment:7 by Jun Omae, 23 months ago

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.

in reply to:  7 comment:8 by Ryan J Ollos, 23 months ago

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.

comment:9 by Ryan J Ollos, 23 months ago

#13086, #13087.

comment:10 by Ryan J Ollos, 12 months ago

Related discussion in #13199.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos 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.