Unused fields should not show up in boolean charts

RESOLVED FIXED in Bugzilla 5.0

Status

()

Bugzilla
Query/Bug List
--
enhancement
RESOLVED FIXED
10 years ago
6 years ago

People

(Reporter: Jesse Clark, Assigned: sjoshi)

Tracking

Bugzilla 5.0
Bug Flags:
approval +

Details

Attachments

(1 attachment, 3 obsolete attachments)

1.17 KB, patch
Frédéric Buclin
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
The boolean charts show fields such as QA Contact, Target Milestone, and Votes even if these are turned off in params.
(Reporter)

Comment 1

10 years ago
Created attachment 327900 [details] [diff] [review]
v1
Assignee: query-and-buglist → jjclark1982
Status: NEW → ASSIGNED
Attachment #327900 - Flags: review?
(Reporter)

Updated

10 years ago
Attachment #327900 - Flags: review?

Comment 2

10 years ago
Hmm, on 3.2 tip? I seem to remember just fixing something about the boolean chart fields.

Comment 3

10 years ago
(In reply to comment #2)
> Hmm, on 3.2 tip? I seem to remember just fixing something about the boolean
> chart fields.

Wasn't it bug 435504, where we replaced hardcoded strings from the DB by those being in field-descs.none.html, for l10n purposes? Else the only other bug I have in mind is bug 287330 where obsolete fields are excluded.

Comment 4

10 years ago
Ah, yeah, it was probably the bug about obsolete fields. And actually, I had noticed the same bug that Jesse is reporting, myself, but I hadn't reported it. So at the least, I can confirm it. :-)
(Reporter)

Comment 5

10 years ago
Created attachment 328165 [details] [diff] [review]
v2

We felt it was important to screen out turned-off core fields from boolean charts in PRACA. Here is a patch that actually works on bugzilla.

It is also important to alphabetize boolean chart fields by final localized description instead of internal description, but that is probably a different bug.
Attachment #327900 - Attachment is obsolete: true
Attachment #328165 - Flags: review?(mkanat)

Comment 6

10 years ago
Comment on attachment 328165 [details] [diff] [review]
v2

This seems like it ought to live in a module somewhere, no? And it seems like it'd be useful elsewhere...?

Comment 7

10 years ago
Comment on attachment 328165 [details] [diff] [review]
v2

>+    @exclude_fields{qw(estimated_time remaining_time work_time
>+                             percentage_complete deadline)} = (1,1,1,1,1);

This looks good to me, but please write:

%exclude_fields = (estimated_time      => 1,
                   remaining_time      => 1,
                   work_time           => 1,
                   percentage_complete => 1,
                   deadline            => 1);

You are mixing hashes and arrays, and it's always a pain to read or modify.

Comment 8

10 years ago
Comment on attachment 328165 [details] [diff] [review]
v2

Please fix my previous comment (use a hash) and we are all good with it, I think.

Putting everything in a module can be done later.
Attachment #328165 - Flags: review?(mkanat) → review-
(Assignee)

Comment 9

6 years ago
Created attachment 684046 [details] [diff] [review]
Patch-v3

Taking it forward to closure.
Worked on top of Jesse's patch by incorporating the review comments.
Attachment #684046 - Flags: review?(LpSolit)

Comment 10

6 years ago
Comment on attachment 684046 [details] [diff] [review]
Patch-v3

>-    foreach my $tt_field (TIMETRACKING_FIELDS) {

Removing TIMETRACKING_FIELDS to restore a hardcoded list is a regression (that's the reason why this constant exists). Don't do that.


>+my %param_controlled_fields = ('useqacontact'        => 'qa_contact',
>+                               'usetargetmilestone'  => 'target_milestone',
>+                               'usevotes'            => 'votes',
>+                               'useclassification'   => 'classification',
>+                               'usestatuswhiteboard' => 'status_whiteboard',
>+                               'usebugaliases'       => 'alias');

usevotes and usebugaliases no longer exist.
Attachment #684046 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 11

6 years ago
Created attachment 684074 [details] [diff] [review]
Patch-v4

Incorporated review comments.
Attachment #684046 - Attachment is obsolete: true
Attachment #684074 - Flags: review?(LpSolit)

Updated

6 years ago
Attachment #328165 - Attachment is obsolete: true

Comment 12

6 years ago
Comment on attachment 684074 [details] [diff] [review]
Patch-v4

r=LpSolit
Attachment #684074 - Flags: review?(LpSolit) → review+

Comment 13

6 years ago
I don't want to take it for 4.4 in case we regress something.
Assignee: jjclark1982 → joshi_sunil
Severity: normal → enhancement
Flags: approval+
Target Milestone: --- → Bugzilla 5.0

Comment 14

6 years ago
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified query.cgi
Committed revision 8504.
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.