105480 - Make better use of fielddefs TABLE when displaying errors

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[James A. Laska]

With strictvaluechecks enabled, rather than displaying sensative database column
names to the user upon certain errors, lets use the fieldefs table to display an
appropriate name.  When strictvaluechecks is on (and the make_popup changes from
bug#95967), and the user neglects to select a platform, OS, or severity, and the
select "Commit", they would be presented with complaints about rep_platform,
op_sys, and bug_status.  The user should never see database information, only
information stored in the database.  Therefore, I propose changing
CGI::CheckFormField(\%$;\@) to use the fielddefs TABLE to lookup the
display'able name for $fieldname.  

I haven't gone through and examined if table/column names are displayed to the
user elsewhere.  If other instances are found (not through CheckFormField()),
they should be modified as well.

Suggested patch soon to follow...

Comment 1

[James A. Laska]

Attachment: CGI.pl patch - updated CheckFormFields

Comment 2

[Dave Miller [:justdave]]

Attachment: v2 - formatting changes to match style guide only

Comment 3

[Dave Miller [:justdave]]

Comment on attachment 54101 [details] [diff] [review]
v2 - formatting changes to match style guide only

based on visual inspection this looks good.

Should be tested before getting 2nd review though.

Comment 4

[Jacob Steenhagen]

Setting owner to patch writer.

Comment 5

[Jacob Steenhagen]

Comment on attachment 54101 [details] [diff] [review]
v2 - formatting changes to match style guide only

Looks good to me. And it works, too :)
r=jake

Comment 6

[Jacob Steenhagen]

/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.116; previous revision: 1.115
done

Comment 7

[James A. Laska]

The query page is another bugzilla module that uses column names from the
database.

The time interface: ("Where the field(s) ... changed") has a list of column
names from the database.  These should also be changed to make use of the
fielddefs table.

The boolean chart: recently added fields to the bugs table assignee_accessible,
clist_accessible, qacontact_accessible, reporter_accessible do not have
fielddefs entries.  I have a fielddef entry for qacontact_accessable and nothing
else.  This would be a bug with bug#39816 for not properly adding the code to
checksetup.pl.  The code to add to checksetup.pl would be:

AddFDef("qacontact_accessible", "QAContact Accessible", 0);
AddFDef("assignee_accessible", "Assignee Accessible", 0);
AddFDef("clist_accessible", "CC List Accessible", 0);
AddFDef("reporter_accessible", "Reporter Accessible", 0);
AddFDef("qacontact_accessible", "QAContact Accessible", 0);


I'll check the code for the "where the field(s)" select box in a bit...

Comment 8

[James A. Laska]

globals.pl -------------------------------

    my $cols = LearnAboutColumns("bugs");
    
    @::log_columns = @{$cols->{"-list-"}};
    foreach my $i ("bug_id", "creation_ts", "delta_ts", "lastdiffed") {
        my $w = lsearch(\@::log_columns, $i);
        if ($w >= 0) {
            splice(@::log_columns, $w, 1);
        }
    }
    @::log_columns = (sort(@::log_columns));

query.cgi -------------------------------
my @logfields = ("[Bug creation]", @::log_columns);
...
<tr bgcolor=\"#ECECEC\">
 <td rowspan=2 align=right>Where the field(s)</td>
 <td rowspan=2>
   <SELECT NAME=\"chfield\" MULTIPLE SIZE=4>
   @{[make_options(\@logfields, $default{'chfield'}, $type{'chfield'})]}
   </SELECT>
 </td>
 <td rowspan=2> changed.</td>

Using the above code excerpts, it would be difficult to use the "pretty" column
names stored in the fielddefs table because CGI::make_options takes a list and
uses the contents of the list as the <OPTION VALUE=\"$item\">$item".  So with
the make_options function there is no way to have the VALUE= be different from
the displayed text.  To fix would require another make_options function that
takes a list of VALUE= and corresponding list of displayed text, or add a
parameter to make_options.  When the parameter is present, use those values for
the displayed text.

Comments?  Let me know what you think before I submit a patch.  Should we
re-open this bug, or should I submit a new bug?