If you're not allowed to change status or resolution, their values are being displayed unlocalized

RESOLVED FIXED in Bugzilla 4.2

Status

()

Bugzilla
Creating/Changing Bugs
--
minor
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Wurblzap, Assigned: Wurblzap)

Tracking

Bugzilla 4.2
Bug Flags:
approval +
approval4.2 +

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 610930 [details] [diff] [review]
Patch

If you're not allowed to change status or resolution, their values are being displayed unlocalized below the comment textarea.
Attachment #610930 - Flags: review?(LpSolit)

Comment 1

6 years ago
4.0 and older are restricted to security bugs only.
Target Milestone: Bugzilla 3.6 → Bugzilla 4.2

Comment 2

6 years ago
Comment on attachment 610930 [details] [diff] [review]
Patch

> [% ELSE %]
>+  [% FOREACH val = value %]
>+    [% display_value(field.name, val) FILTER html %]
>+    [% ', ' UNLESS loop.last() %]
>+  [% END %]
> [% END %]

Hum, I think you should check field.type and only call display_value() for single-select and multi-select fields. Other fields do not need to call display_value(). This would avoid useless calls to display_value() and would be consistent with the editable part of the template.
Attachment #610930 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 3

6 years ago
Created attachment 611227 [details] [diff] [review]
Patch 1.1

I'm not really in agreement, because it's pure chance that display_value does not need to be called for other field types. For maintainability, we should call display_value no matter what.

In order to get in a fix, here's a modified patch anyway.
Attachment #610930 - Attachment is obsolete: true
Attachment #611227 - Flags: review?(LpSolit)

Comment 4

6 years ago
Comment on attachment 611227 [details] [diff] [review]
Patch 1.1

It's not pure chance. Select fields are the only ones with a defined list of values. Why would you localize e.g. a free text field? This doesn't really make sense. Anyway, could you please use ELSIF instead of SWITCH to remain consistent with the code above your code? IF ELSIF ELSIF ELSE SWITCH doesn't make sense.
(Assignee)

Comment 5

6 years ago
Created attachment 611230 [details] [diff] [review]
Patch 1.2

All right, it doesn't make sense for all field types.

Here's an updated patch. For consistency with the [% IF editable %] part, I moved to SWITCH/CASE instead of ELSIF.
Attachment #611227 - Attachment is obsolete: true
Attachment #611227 - Flags: review?(LpSolit)
Attachment #611230 - Flags: review?(LpSolit)

Comment 6

6 years ago
Comment on attachment 611230 [details] [diff] [review]
Patch 1.2

Works fine, nice cleanup. :) r=LpSolit
Attachment #611230 - Flags: review?(LpSolit) → review+

Updated

6 years ago
Flags: approval4.2+
Flags: approval+
(Assignee)

Comment 7

6 years ago
Committing to: bzr+ssh://wurblzap%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified template/en/default/bug/field.html.tmpl
Committed revision 8181.

Committing to: bzr+ssh://wurblzap%40gmail.com@bzr.mozilla.org/bugzilla/4.2/
modified template/en/default/bug/field.html.tmpl
Committed revision 8067.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.