If you're not allowed to change status or resolution, their values are being displayed unlocalized
RESOLVED
FIXED
in Bugzilla 4.2
Status
()
People
(Reporter: Wurblzap, Assigned: Wurblzap)
Tracking
Bug Flags:
Details
Attachments
(1 attachment, 2 obsolete attachments)
|
1.87 KB,
patch
|
Frédéric Buclin
:
review+
|
Details | Diff | Splinter Review |
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.
Description
•