443306 - Unused fields should not show up in boolean charts

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, enhancement)

Description

[Jesse Clark]

The boolean charts show fields such as QA Contact, Target Milestone, and Votes even if these are turned off in params.

Comment 1

[Jesse Clark]

Attachment: v1

Comment 2

[Max Kanat-Alexander]

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

Comment 3

[Frédéric Buclin]

(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

[Max Kanat-Alexander]

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

Comment 5

[Jesse Clark]

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

Comment 6

[Max Kanat-Alexander]

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

[Frédéric Buclin]

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

[Frédéric Buclin]

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.

Comment 9

[Sunil Joshi [:sjoshi]]

Attachment: Patch-v3

Taking it forward to closure.
Worked on top of Jesse's patch by incorporating the review comments.

Comment 10

[Frédéric Buclin]

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.

Comment 11

[Sunil Joshi [:sjoshi]]

Attachment: Patch-v4

Incorporated review comments.

Comment 12

[Frédéric Buclin]

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

r=LpSolit

Comment 13

[Frédéric Buclin]

I don't want to take it for 4.4 in case we regress something.

Comment 14

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified query.cgi
Committed revision 8504.