Bug 264601 - Bugzilla will now tell a user which field contained an "invalid bug number or alias."

Patch by Frédéric Buclin <LpSolit@netscape.net>
r=myk, a=justdave

6 files changed, 39 insertions, 59 deletions

diff --git a/CGI.pl b/CGI.pl
index 456022808..0d03562fe 100644
--- a/CGI.pl
+++ b/CGI.pl
@@ -148,7 +148,7 @@ sub ValidateBugID {
     # database, and that the user is authorized to access that bug.
     # We detaint the number here, too
 
-    my ($id, $skip_authorization) = @_;
+    my ($id, $field) = @_;
     
     # Get rid of white-space around the ID.
     $id = trim($id);
@@ -157,7 +157,9 @@ sub ValidateBugID {
     my $alias = $id;
     if (!detaint_natural($id)) {
         $id = BugAliasToID($alias);
-        $id || ThrowUserError("invalid_bug_id_or_alias", {'bug_id' => $alias});
+        $id || ThrowUserError("invalid_bug_id_or_alias",
+                              {'bug_id' => $alias,
+                               'field'  => $field });
     }
     
     # Modify the calling code's original variable to contain the trimmed,
@@ -170,7 +172,7 @@ sub ValidateBugID {
     FetchOneColumn()
       || ThrowUserError("invalid_bug_id_non_existent", {'bug_id' => $id});
 
-    return if $skip_authorization;
+    return if ($field eq "dependson" || $field eq "blocked");
     
     return if Bugzilla->user->can_see_bug($id);
 
diff --git a/attachment.cgi b/attachment.cgi
index 31dcd12e7..b486bf6db 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -1060,11 +1060,6 @@ sub update
   # Get the bug ID for the bug to which this attachment is attached.
   SendSQL("SELECT bug_id FROM attachments WHERE attach_id = $::FORM{'id'}");
   my $bugid = FetchSQLData();
-  unless ($bugid) 
-  {
-    ThrowUserError("invalid_bug_id",
-                   { bug_id => $bugid });
-  }
   
   # Lock database tables in preparation for updating the attachment.
   SendSQL("LOCK TABLES attachments WRITE , flags WRITE , " . 
diff --git a/post_bug.cgi b/post_bug.cgi
index 5fc50e66f..7282f8fa9 100755
--- a/post_bug.cgi
+++ b/post_bug.cgi
@@ -256,7 +256,7 @@ foreach my $field ("dependson", "blocked") {
         my @validvalues;
         foreach my $id (split(/[\s,]+/, $::FORM{$field})) {
             next unless $id;
-            ValidateBugID($id, 1);
+            ValidateBugID($id, $field);
             push(@validvalues, $id);
         }
         $::FORM{$field} = join(",", @validvalues);
diff --git a/process_bug.cgi b/process_bug.cgi
index 47957d193..6fb94711a 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -120,23 +120,13 @@ foreach my $field ("dependson", "blocked") {
         my @validvalues;
         foreach my $id (split(/[\s,]+/, $::FORM{$field})) {
             next unless $id;
-            ValidateBugID($id, 1);
+            ValidateBugID($id, $field);
             push(@validvalues, $id);
         }
         $::FORM{$field} = join(",", @validvalues);
     }
 }
 
-# If we are duping bugs, let's also make sure that we can change 
-# the original.  This takes care of issue A on bug 96085.
-if (defined $::FORM{'dup_id'} && $::FORM{'knob'} eq "duplicate") {
-    ValidateBugID($::FORM{'dup_id'});
-
-    # Also, let's see if the reporter has authorization to see the bug
-    # to which we are duping.  If not we need to prompt.
-    DuplicateUserConfirm();
-}
-
 # do a match on the fields if applicable
 
 # The order of these function calls is important, as both Flag::validate
@@ -490,8 +480,8 @@ sub DuplicateUserConfirm {
         return;
     }
 
-    my $dupe = trim($::FORM{'id'});
-    my $original = trim($::FORM{'dup_id'});
+    my $dupe = $::FORM{'id'};
+    my $original = $::FORM{'dup_id'};
     
     SendSQL("SELECT reporter FROM bugs WHERE bug_id = " . SqlQuote($dupe));
     my $reporter = FetchOneColumn();
@@ -520,7 +510,7 @@ sub DuplicateUserConfirm {
     $template->process("bug/process/confirm-duplicate.html.tmpl", $vars)
       || ThrowTemplateError($template->error());
     exit;
-} # end DuplicateUserConfirm()
+}
 
 if (defined $::FORM{'id'}) {
     # since this means that we were called from show_bug.cgi, now is a good
@@ -976,28 +966,22 @@ SWITCH: for ($::FORM{'knob'}) {
         last SWITCH;
     };
     /^duplicate$/ && CheckonComment( "duplicate" ) && do {
-        ChangeStatus('RESOLVED');
-        ChangeResolution('DUPLICATE');
-        CheckFormFieldDefined(\%::FORM,'dup_id');
-        my $num = trim($::FORM{'dup_id'});
-        SendSQL("SELECT bug_id FROM bugs WHERE bug_id = " . SqlQuote($num));
-        $num = FetchOneColumn();
-        if (!$num) {
-            ThrowUserError("dupe_invalid_bug_id")
-        }
-        if (!defined($::FORM{'id'}) || $num == $::FORM{'id'}) {
+        # Make sure we can change the original bug (issue A on bug 96085)
+        CheckFormFieldDefined(\%::FORM, 'dup_id');
+        ValidateBugID($::FORM{'dup_id'}, 'dup_id');
+
+        # Also, let's see if the reporter has authorization to see
+        # the bug to which we are duping. If not we need to prompt.
+        DuplicateUserConfirm();
+
+        $duplicate = $::FORM{'dup_id'};
+        if (!defined($::FORM{'id'}) || $duplicate == $::FORM{'id'}) {
             ThrowUserError("dupe_of_self_disallowed");
         }
-        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 });
-        }
-        $::FORM{'comment'} .= "\n\n*** This bug has been marked as a duplicate of $num ***";
-        $duplicate = $num;
-
+        ChangeStatus('RESOLVED');
+        ChangeResolution('DUPLICATE');
+        $::FORM{'comment'} .= "\n\n*** This bug has been marked " .
+                              "as a duplicate of $duplicate ***";
         last SWITCH;
     };
 
diff --git a/template/en/default/global/field-descs.none.tmpl b/template/en/default/global/field-descs.none.tmpl
index 1080e8e44..e49db6b5b 100644
--- a/template/en/default/global/field-descs.none.tmpl
+++ b/template/en/default/global/field-descs.none.tmpl
@@ -26,6 +26,7 @@
 [% field_descs = { "[Bug creation]"       => "[$terms.Bug creation]",
                    "alias"                => "Alias",
                    "assigned_to"          => "Assignee",
+                   "blocked"              => "Blocks",
                    "bug_file_loc"         => "URL",
                    "bug_id"               => "$terms.Bug ID",
                    "bug_severity"         => "Severity",
@@ -38,6 +39,8 @@
                    "component"            => "Component",
                    "creation_ts"          => "$terms.Bug Creation time",
                    "delta_ts"             => "Last Changed time",
+                   "dependson"            => "Depends on",
+                   "dup_id"               => "Duplicate",
                    "estimated_time"       => "Orig. Est.",
                    "everconfirmed"        => "Ever confirmed?",
                    "groupset"             => "Groupset",
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index affad39cc..ebd44906e 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -262,11 +262,6 @@
     [% title = "Description Required" %]
     You must provide a description of the [% terms.bug %].
 
-  [% ELSIF error == "dupe_invalid_bug_id" %]
-    [% title = BLOCK %]Valid [% terms.Bug %] Number Required[% END %]
-    You must specify a valid [% terms.bug %] number of which this 
-    [%+ terms.bug %] is a duplicate.  The [% terms.bug %] has not been changed.
-
   [% ELSIF error == "dupe_of_self_disallowed" %]
     [% title = "Cannot mark $terms.abug as a duplicate of itself" %]
     You can't mark [% terms.abug %] as a duplicate of itself.
@@ -468,19 +463,24 @@
     [% title = "Invalid Attachment ID" %]
     The attachment id [% attach_id FILTER html %] is invalid.
 
-  [% ELSIF error == "invalid_bug_id" %]
-    [% title = BLOCK %]Invalid [% terms.Bug %] ID[% END %]
-    The [% terms.bug %] id [% bug_id FILTER html %] is invalid.
-
   [% ELSIF error == "invalid_bug_id_non_existent" %]
     [% title = BLOCK %]Invalid [% terms.Bug %] ID[% END %]
     [% terms.Bug %] #[% bug_id FILTER html %] does not exist.
     
   [% ELSIF error == "invalid_bug_id_or_alias" %]
     [% title = BLOCK %]Invalid [% terms.Bug %] ID[% END %]
-    '[% bug_id FILTER html %]' is not a valid [% terms.bug %] number
-    [% IF Param("usebugaliases") %] nor an alias
-    to [% terms.abug %] number[% END %].
+    [% IF bug_id %]
+      '[% bug_id FILTER html %]' is not a valid [% terms.bug %] number
+      [% IF Param("usebugaliases") %]
+        nor an alias to [% terms.abug %] number
+      [% END %].
+    [% ELSE %]
+      [% IF field %]
+        The '[% field_descs.$field FILTER html %]' field
+        cannot be empty.
+      [% END %]
+      You must enter a valid [% terms.bug %] number!
+    [% END %]
     <noscript>
       If you are trying to use QuickSearch, you need to enable
       JavaScript in your browser.
@@ -523,10 +523,6 @@
     [% title = "Invalid group name" %]
     The group you specified, [% name FILTER html %], is not valid here.
 
-  [% ELSIF error == "invalid_group_name" %]
-    [% title = "Invalid group name" %]
-    The group you specified, [% name FILTER html %], is not valid here.
-
   [% ELSIF error == "invalid_maxrows" %]
     [% title = "Invalid Max Rows" %]
     The maximum number of rows, '[% maxrows FILTER html %]', must be