271023 - process_bug.cgi needs some cleanup

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Frédéric Buclin]

There are several points in process_bug.cgi which I cannot understand why there
are here. I may misunderstand some of them, but globally, I think this should go
away or be simplified.

Among others:

    if (!CheckCanChangeField('product', $::FORM{'id'}, $::oldproduct,
$::FORM{'product'})) {
        $vars->{'oldvalue'} = $::oldproduct;
        $vars->{'newvalue'} = $::FORM{'product'};
        $vars->{'field'} = 'product';
        ThrowUserError("illegal_change",
                       { oldvalue => $::oldproduct,
                         newvalue => $::FORM{'product'},
                         field => 'product',
                       },
                       "abort");
    }

Instead of:

ThrowUserError("illegal_change", $vars, "abort");


Another example:

    if ($ownerid eq $whoid) {
    if (Param('useqacontact') && ($qacontactid eq $whoid)) {
    if ($reporterid eq $whoid) {

Here, we compare numbers, so why not:

    if ($ownerid == $whoid) {
    if (Param('useqacontact') && ($qacontactid == $whoid)) {
    if ($reporterid == $whoid) {


Another one:

        $::FORM{'resolution'} = $str; # Used later by CheckCanChangeField

But in CheckCanChangeField, we say: return 1 if ($field eq "resolution"). So why
defining a field nobody cares about?


Another one:

foreach my $field ("rep_platform", "priority", "bug_severity",          
                   "summary", "bug_file_loc", "short_desc",
                   "version", "op_sys",
                   "target_milestone", "status_whiteboard") {
    if (defined $::FORM{$field}) {
        if (!$::FORM{'dontchange'}
            || $::FORM{$field} ne $::FORM{'dontchange'}) {
            DoComma();
            $::query .= "$field = " . SqlQuote(trim($::FORM{$field}));
        }
    }
}

Here, $::query is used to update the bugs table, but "summary" is not a valid
attribute of bugs. Btw, this field is never defined (does not exist)!


Another one:

            $qacontact = 0 unless (defined $qacontact && $qacontact != 0);

Why not:

$qacontact = 0 unless defined $qacontact;


Another one:

    my $dupe = trim($::FORM{'id'});

But $::FORM{'id'} is already trimmed by ValidateBugID at the beginning of the file.


Another one:

        my $checkid = trim($::FORM{'id'});
        SendSQL("SELECT bug_id FROM bugs where bug_id = " .  SqlQuote($checkid));
        $checkid = FetchOneColumn();
        if (!$checkid) {
            ThrowUserError("invalid_bug_id",
                           { bug_id => $checkid });
        }

That's exactly what ValidateBugID does when process_bug.cgi is called. Why
checking this twice?

There may be some others... Do not hesitate to mention them.

Comment 1

[Frédéric Buclin]

All these dependent bugs, except one, are assigned to me.

Comment 2

[Dave Miller [:justdave]]

Most of the code in process_bug.cgi has been there since "The Early Days" and
many of those much more logical function calls you're suggesting have only been
recently introduced.  Nobody has yet gone back and retrofitted process_bug.cgi.
 It's definitely in sore need of a cleanup.  There's been talk of completely
rewriting it, but nobody's ever got around to it.

Comment 3

[Matthew Tuck [:CodeMachine]]

I think the summary of this bug is the biggest understatement of the year.

Comment 4

[Frédéric Buclin]

As far as I'm concerned, this bug is now fixed since the checkin of bug 412836.