Edgewall Software
Modify

Opened 13 years ago

Closed 11 years ago

Last modified 11 years ago

#4061 closed defect (fixed)

existing tickets should get the default value if value is not set in ticket

Reported by: GreenZero Owned by: Remy Blank
Priority: normal Milestone: 0.11.2
Component: ticket system Version: 0.10
Severity: major Keywords: ticket modify severity default custom field verify patch
Cc: oliver@…, p.phaneuf@… Branch:
Release Notes:
API Changes:

Description

actual situation:

  • creating a new ticket, severity will be set to the configured default severity
  • existing tickets with no severity set will get the first severity in configured order on modify after creating severities


required situation:

  • creating a new ticket, severity will be set to the configured default severity (already implemented = OK)
  • modify an existing ticket will set severity to configured default severity, too


this occurs after using trac in project where no severity was used at the begining. Later on severity was filled. Modifiing existing tickets causes them to be automatically set to highest priority if user did not pay attention to that ticket field and set it manually. It would be very nice, if you could fix this.

Attachments (2)

4061-standard-fields-default-r7502.patch (536 bytes ) - added by Remy Blank 11 years ago.
Patch against 0.11-stable avoiding to set unset standard fields to an empty string
4061-standard-fields-default-r7554.patch (536 bytes ) - added by Remy Blank 11 years ago.
Updated patch for current 0.11-stable

Download all attachments as: .zip

Change History (19)

comment:1 by anonymous, 13 years ago

After we tried a workaround with custom fields we found out, that the custom fields have the same behaviour. The default value of such a custom field is not respected if you try to modify an already existing ticket.

comment:2 by anonymous, 13 years ago

Keywords: custom added

comment:3 by anonymous, 13 years ago

Keywords: field added

comment:4 by anonymous, 13 years ago

Cc: oliver@… added

comment:5 by Emmanuel Blot, 13 years ago

Milestone: 0.10.1

comment:6 by GreenZero, 13 years ago

Summary: existing tickets should get the default severity if severity is not set in ticketexisting tickets should get the default value in custom fields if value is not set in ticket

summary changed because it is more related to custom fields than only to severity (severity is just a custom field)

comment:7 by anonymous, 13 years ago

Milestone: 0.10.3

comment:8 by Pierre Phaneuf <pphaneuf@…>, 12 years ago

Cc: p.phaneuf@… added
Summary: existing tickets should get the default value in custom fields if value is not set in ticketexisting tickets should get the default value if value is not set in ticket

It's not just on custom fields, actually, but on built-in fields that can be hidden (like severity or component). We just transitioned from not using the "component" field (so we had a bunch of tickets with a "null" component), and when we added components, we picked one as the default. On new tickets, this was working correctly, but when an ticket (that didn't have a component) was brought up, the component field would default to the first component in alphabetical order, rather than the default component.

So we had a bunch of tickets inadvertantly changed to a (most of the time) non-sensical component by people who just added a comment quickly.

The proper fix would be for the ticket display code to use the default component if it is not set, instead of letting the browser pick the first one (for the component field, of course, this could apply to other fields).

comment:9 by Christian Boos, 12 years ago

Keywords: verify added
Milestone: 0.10.50.11.1
Owner: changed from Jonas Borgström to Christian Boos

See also #7200.

The current behavior (0.11-stable) must be checked.

comment:10 by Remy Blank, 11 years ago

This behavior has been fixed for custom fields in #7465. But as far as I can tell, that fix should also correct the problem for built-in fields. Let me quickly check…

by Remy Blank, 11 years ago

Patch against 0.11-stable avoiding to set unset standard fields to an empty string

comment:11 by Remy Blank, 11 years ago

Keywords: patch added
Owner: changed from Christian Boos to Remy Blank

…not quite. Unset standard fields are set to the empty string on retrieval in Ticket._fetch_ticket(). The patch above fixes the problem for me with no ill effects (meaning, queries and ticket viewing and editing work, and the test suite passes).

by Remy Blank, 11 years ago

Updated patch for current 0.11-stable

comment:12 by Remy Blank, 11 years ago

I'm still looking for a volunteer to review this. Any takers?

comment:13 by Christian Boos, 11 years ago

I'll test it.

comment:14 by Christian Boos, 11 years ago

I had to verify this because I was a bit confused about how this change could work.

Looks like that when we set it to '', the

 value = ticket.get_value_or_default(field.name)

in the template will get '' (instead of the default when we don't set the field, like your patch does).

Then when filling the option list, in the first case no actual entry will match '', so the first one will be chosen, whereas in the second case, the appropriate default will be chosen.

Now while I'm at it, I see that the get_value_or_default is a bit heavy handed (try...except), maybe we could do a simple if name in self.values: ... instead.

in reply to:  14 ; comment:15 by Remy Blank, 11 years ago

Thanks for the review.

Replying to cboos:

Then when filling the option list, in the first case no actual entry will match '', so the first one will be chosen, whereas in the second case, the appropriate default will be chosen.

Correct. So… would you say that this is the right way to fix this issue?

Now while I'm at it, I see that the get_value_or_default is a bit heavy handed (try...except), maybe we could do a simple if name in self.values: ... instead.

There are two schools about this. Some people say "better ask for forgiveness than for permission" and they do it the try: except: way, and others do an in check before accessing the item. The argument for the former is that a try: except: block has almost no runtime overhead in the normal case (no exception), whereas an if ... in ... and accessing the item requires two lookups.

In this case, the difference is probably negligible, so if the latter is the preferred coding style in Trac, I'll change it.

comment:16 by Remy Blank, 11 years ago

Milestone: 0.11.30.11.2
Resolution: fixed
Status: newclosed

Patch applied in [7570].

in reply to:  15 comment:17 by Christian Boos, 11 years ago

Replying to rblank:

Correct. So… would you say that this is the right way to fix this issue?

Yes, indeed. I was not clear about this, sorry.

Modify Ticket

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