Edgewall Software
Modify

Opened 5 years ago

Closed 5 years ago

Last modified 5 months ago

#11828 closed enhancement (fixed)

Replace underscores with whitespace when creating action label from action name

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

Added label attribute to the TracWorkflow actions and deprecated the name attribute. If neither name nor label is specified, the label will be created from the action name by replacing underscores with whitespace.

If not specified, the label attribute of TracTicketsCustomFields will be created from the custom field name by replacing underscores with whitespace.

API Changes:

Description (last modified by Ryan J Ollos)

Consider a workflow action such as the following:

may_reassign = new,assigned,accepted,reopened -> assigned
may_reassign.operations = may_set_owner
may_reassign.permissions = TICKET_MODIFY 
may_reassign.set_owner =

If I want the action name to look nice, I must add another attribute:

may_reassign.name = may reassign

What would be the downside to doing a simple replacement?

  • trac/ticket/default_workflow.py

    diff --git a/trac/ticket/default_workflow.py b/trac/ticket/default_workflow.py
    index f9c595e..f35ca4f 100644
    a b def parse_workflow_config(rawactions):  
    6767    for action, attributes in actions.items():
    6868        # Default the 'name' attribute to the name used in the ini file
    6969        if 'name' not in attributes:
    70             attributes['name'] = action
     70            attributes['name'] = action.replace("_", " ")
    7171        # If not specified, an action is not the default.
    7272        attributes['default'] = int(attributes.get('default', 0))
    7373        # If operations are not specified, that means no operations

The only downside I can think of is when the user really wants an underscore in the name, but I guess that to be a rather infrequent occurrence.

The same replacement could be applied to TracTicketsCustomFields field name.

Speaking of which, it's a bit confusing that the attribute label is used for TracTicketsCustomFields, whereas name is used for TracWorkflow, considering the attributes describe similar behavior.

Anyway, those are just small things and not too important.

Attachments (0)

Change History (8)

comment:1 by Christian Boos, 5 years ago

I hope I'm not making this too complicated than it needs to be, but to me, name and label should be two different things:

  • 'name' names the thing, i.e. could be used as a way to identify it
  • 'label' is how it's displayed, i.e. the user facing value (could be localized, even)

In the ticket fields case, that's pretty clear, e.g. {'name': 'owner', 'label': N_('Owner')}. We don't have yet localization for the values themselves, but if we had (TracDev/Proposals/ConfigEnumTranslation), I think that clearly differentiating between 'name' and 'label' would be needed as well.

In the Workflow situation, we could have followed the above scheme by saying there's always an implicit <action>.name = <action> and there could be an explicit <action>.label = displayed name. Thinking about it, if we say that <action>.label defaults to <action>.name, we could switch to using .label and still support existing configurations, i.e. the <action>.name = without space would be only used to provide a default to <action>.label, not to change the action's name in the code.

And then, normalizing attributes['label'] = name.replace("_", " ") makes sense.

in reply to:  1 comment:2 by Ryan J Ollos, 5 years ago

Replying to cboos:

I hope I'm not making this too complicated than it needs to be, but to me, name and label should be two different things:

  • 'name' names the thing, i.e. could be used as a way to identify it
  • 'label' is how it's displayed, i.e. the user facing value (could be localized, even)

We appear to be thinking similarly. For the ticket workflow the name attribute currently behaves in the way that you suggest the label attribute should behave. That's why I felt the choice of name for the ticket workflow attribute was poor, and the code should have originally been written like this:

  • trac/ticket/default_workflow.py

    diff --git a/trac/ticket/default_workflow.py b/trac/ticket/default_workflow.py
    index 5bf01ec..8950cba 100644
    a b def parse_workflow_config(rawactions):  
    6666
    6767    for action, attributes in actions.items():
    68         # Default the 'name' attribute to the name used in the ini file
    69         if 'name' not in attributes:
    70             attributes['name'] = action
     68        # Default the 'label' attribute to the action name
     69        if 'label' not in attributes:
     70            attributes['label'] = action
    7171        # If not specified, an action is not the default.
    7272        attributes['default'] = int(attributes.get('default', 0))
    7373        # If operations are not specified, that means no operations

In the Workflow situation, we could have followed the above scheme by saying there's always an implicit <action>.name = <action> and there could be an explicit <action>.label = displayed name. Thinking about it, if we say that <action>.label defaults to <action>.name, we could switch to using .label and still support existing configurations, i.e. the <action>.name = without space would be only used to provide a default to <action>.label, not to change the action's name in the code.

And then, normalizing attributes['label'] = name.replace("_", " ") makes sense.

parse_workflow_config builds a dictionary with the action as the key.

{u'resolve': {u'operations': [u'set_resolution'], 'name': u'resolve', 'default': 0, 'newstate': u'closed', u'set_resolution': [u''], 'oldstates': [u'new', u'assigned', u'accepted', u'reopened'], u'permissions': [u'TICKET_MODIFY']},

Therefore I expect the following patch would preserve backwards-compatibility by reading the name attribute when present, but giving preference to the label attribute and generating the label from the action name when neither name or label is present.

  • trac/ticket/default_workflow.py

    diff --git a/trac/ticket/default_workflow.py b/trac/ticket/default_workflow.py
    index 7388b1e..e91cde0 100644
    a b def parse_workflow_config(rawactions):  
    6565        return [item for item in (x.strip() for x in value.split(',')) if item]
    6666
    6767    for action, attributes in actions.items():
    68         # Default the 'label' attribute to the name used in the ini file
    69         if 'name' not in attributes:
    70             attributes['name'] = action
     68        # Default the 'label' attribute to the action name
     69        if 'label' not in attributes:
     70            if 'name' in attributes:  # backwards-compatibility, #11828
     71                attributes['label'] = attributes['name']
     72            else:
     73                attributes['label'] = action.replace("_", " ")
    7174        # If not specified, an action is not the default.
    7275        attributes['default'] = int(attributes.get('default', 0))
    7376        # If operations are not specified, that means no operations
    class ConfigurableTicketWorkflow(Component):  
    133136            # exists, as no other action can then change its state. (#5307)
    134137            self.actions['_reset'] = {
    135138                'default': 0,
    136                 'name': 'reset',
     139                'label': 'reset',
    137140                'newstate': 'new',
    138141                'oldstates': [],  # Will not be invoked unless needed
    139142                'operations': ['reset_workflow'],
    Read TracWorkflow for more information (don't forget to 'wik  
    357360            if status != '*':
    358361                hints.append(tag_("Next status will be '%(name)s'",
    359362                                  name=status))
    360         return (this_action['name'], tag(separated(control, ' ')),
     363        return (this_action['label'], tag(separated(control, ' ')),
    361364                tag(separated(hints, '. ', '.') if hints else ''))
    362365
    363366    def get_ticket_changes(self, req, ticket, action):

This would allow us to deprecate the name attribute. If we want to eventually remove the compatibility code an upgrade step could be written, but I don't see any harm in leaving it in place.

comment:3 by Christian Boos, 5 years ago

Yes exactly, and that's even simpler than what I thought, as it looks like that the 'name' attribute unmodified is not needed.

comment:4 by Ryan J Ollos, 5 years ago

DONE Modify sample workflows in trunk/contrib/workflow.

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

comment:5 by Ryan J Ollos, 5 years ago

Milestone: next-dev-1.1.x1.1.3

comment:6 by Ryan J Ollos, 5 years ago

Status: newassigned

comment:7 by Ryan J Ollos, 5 years ago

Release Notes: modified (diff)

Proposed changes in log:rjollos.git:t11828. Documentation updated in 1.1/TracWorkflow@5 and 1.1/TracTicketsCustomFields@6.

comment:8 by Ryan J Ollos, 5 years ago

Description: modified (diff)
Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to trunk in [13431:13432].

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 as closed 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.