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

[GreenZero]

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.

[anonymous]

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.

[GreenZero]

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

[Pierre Phaneuf <pphaneuf@…>]

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).

[cboos]

See also #7200.

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

[rblank]

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

[rblank]

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

[rblank]

...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).

[rblank]

Updated patch for current 0.11-stable

[rblank]

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

[cboos]

I'll test it.

[cboos]

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.

[rblank]

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.

[rblank]

Patch applied in [7570].

[cboos]

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.