271797 - target_milestone should never be empty if usetargetmilestone=1

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Frédéric Buclin]

When editing a single bug directly from an URL like
.../process_bug.cgi?product=XXX&component=YYY&....&target_milestone=&.....

Bugzilla does not complain and accepts target_milestone="" which is then saved
in the bugs table as an empty string. But when you try to edit this bug again,
it complains that Target Milestone is not defined. Unfortunalety, this field is
unavailable from the UI and you have to modifiy the bug using the URL above with
target_milestone set.

Bugzilla knows what are the legal milestones for a given product; it should look
at them! Patch coming...

Comment 1

[Frédéric Buclin]

Attachment: validate target_milestone

Check if target_milestone is legal.

Comment 2

[Frédéric Buclin]

Comment on attachment 167065 [details] [diff] [review]
validate target_milestone

Some cleanup to process_bug.cgi here! :)

Comment 3

[Dave Miller [:justdave]]

Comment on attachment 167065 [details] [diff] [review]
validate target_milestone

>Index: mozilla/webtools/bugzilla/process_bug.cgi
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v
>retrieving revision 1.217
>diff -u -r1.217 process_bug.cgi
>--- mozilla/webtools/bugzilla/process_bug.cgi        2004-11-20 18:37:27.000000000 +0100
>+++ mozilla/webtools/bugzilla/process_bug.cgi        2004-11-26 01:47:20.570713384 +0100
>@@ -202,12 +202,7 @@
> CheckFormFieldDefined(\%::FORM, 'version');
> CheckFormFieldDefined(\%::FORM, 'component');
> 
>-# check if target milestone is defined - [e-mail]
>-if ( Param("usetargetmilestone") ) {
>-  CheckFormFieldDefined(\%::FORM, 'target_milestone');
>-}
> 
>-#
> # This function checks if there is a comment required for a specific
> # function and tests, if the comment was given.
> # If comments are required for functions is defined by params.
>@@ -275,6 +270,7 @@
> 
>     my $mok = 1;   # so it won't affect the 'if' statement if milestones aren't used
>     if ( Param("usetargetmilestone") ) {
>+       CheckFormFieldDefined(\%::FORM, 'target_milestone');
>        $mok = lsearch($::target_milestone{$prod}, $::FORM{'target_milestone'}) >= 0;
>     }
> 
>@@ -292,23 +288,21 @@
>             # if its a valid option, otherwise we use the default where
>             # thats appropriate
>             $vars->{'versions'} = $::versions{$prod};
>-            if (lsearch($::versions{$prod}, $::FORM{'version'}) != -1) {
>+            if ($vok) {
>                 $defaults{'version'} = $::FORM{'version'};
>             }
>             $vars->{'components'} = $::components{$prod};
>-            if (lsearch($::components{$prod}, $::FORM{'component'}) != -1) {
>+            if ($cok) {
>                 $defaults{'component'} = $::FORM{'component'};
>             }
>-
>             if (Param("usetargetmilestone")) {
>                 $vars->{'use_target_milestone'} = 1;
>                 $vars->{'milestones'} = $::target_milestone{$prod};
>-                if (lsearch($::target_milestone{$prod},
>-                            $::FORM{'target_milestone'}) != -1) {
>+                if ($mok) {
>                     $defaults{'target_milestone'} = $::FORM{'target_milestone'};
>                 } else {
>-                    SendSQL("SELECT defaultmilestone FROM products WHERE " .
>-                            "name = " . SqlQuote($prod));
>+                    SendSQL("SELECT defaultmilestone FROM products " .
>+                            "WHERE name = " . SqlQuote($prod));
>                     $defaults{'target_milestone'} = FetchOneColumn();
>                 }
>             }
>@@ -318,7 +312,7 @@
>             $vars->{'defaults'} = \%defaults;
>         }
>         else {
>-            $vars->{"verify_fields"} = 0;
>+            $vars->{'verify_fields'} = 0;
>         }
>         
>         $vars->{'verify_bug_group'} = (AnyDefaultGroups() 
>@@ -536,19 +530,23 @@
>     # (XXX those error checks need to happen too, but implementing them 
>     # is more work in the current architecture of this script...)
>     #
>-    CheckFormField(\%::FORM, 'rep_platform', \@::legal_platform);
>-    CheckFormField(\%::FORM, 'priority', \@::legal_priority);
>-    CheckFormField(\%::FORM, 'bug_severity', \@::legal_severity);
>+    CheckFormField(\%::FORM, 'product', \@::legal_product);
>     CheckFormField(\%::FORM, 'component', 
>                    \@{$::components{$::FORM{'product'}}});
>-    CheckFormFieldDefined(\%::FORM, 'bug_file_loc');
>-    CheckFormFieldDefined(\%::FORM, 'short_desc');
>-    CheckFormField(\%::FORM, 'product', \@::legal_product);
>     CheckFormField(\%::FORM, 'version', 
>                    \@{$::versions{$::FORM{'product'}}});
>+    if ( Param("usetargetmilestone") ) {
>+        CheckFormField(\%::FORM, 'target_milestone', 
>+                       \@{$::target_milestone{$::FORM{'product'}}});
>+    }
>+    CheckFormField(\%::FORM, 'rep_platform', \@::legal_platform);
>     CheckFormField(\%::FORM, 'op_sys', \@::legal_opsys);
>+    CheckFormField(\%::FORM, 'priority', \@::legal_priority);
>+    CheckFormField(\%::FORM, 'bug_severity', \@::legal_severity);
>+    CheckFormFieldDefined(\%::FORM, 'bug_file_loc');
>+    CheckFormFieldDefined(\%::FORM, 'short_desc');
>     CheckFormFieldDefined(\%::FORM, 'longdesclength');
>-    
>+
>     if (trim($::FORM{'short_desc'}) eq "") {
>         ThrowUserError("require_summary");
>     }
>@@ -676,9 +674,8 @@
>     }
> }
> 
>-foreach my $field ("rep_platform", "priority", "bug_severity",          
>-                   "summary", "bug_file_loc", "short_desc",
>-                   "version", "op_sys",
>+foreach my $field ("rep_platform", "priority", "bug_severity",
>+                   "bug_file_loc", "short_desc", "version", "op_sys",
>                    "target_milestone", "status_whiteboard") {
>     if (defined $::FORM{$field}) {
>         if (!$::FORM{'dontchange'}
>@@ -1215,9 +1212,7 @@
>         SendSQL("SELECT defaultmilestone FROM products WHERE name = " .
>                 SqlQuote($oldhash{'product'}));
>         if ($value eq FetchOneColumn()) {
>-            ThrowUserError("milestone_required",
>-                           { bug_id => $id },
>-                           "abort");
>+            ThrowUserError("milestone_required", { bug_id => $id }, "abort");
>         }
>     }   
>     if (defined $::FORM{'delta_ts'} && $::FORM{'delta_ts'} ne $delta_ts) {

Comment 4

[Dave Miller [:justdave]]

errr, oops :)  sorry about that, didn't mean to "Edit as Comment" and not delete
it :)

Comment 5

[Frédéric Buclin]

I didn't test my patch on the 2.18 branch, but I'm quite sure process_bug.cgi
hasn't been modified a lot since we branched! ;)

Comment 6

[Frédéric Buclin]

(In reply to comment #5)
> I didn't test my patch on the 2.18 branch, but I'm quite sure process_bug.cgi
> hasn't been modified a lot since we branched! ;)
> 

OK, sounds good!

Comment 7

[Vlad Dascalu]

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.205.2.9; previous revision: 1.205.2.8
done

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.219; previous revision: 1.218
done