264601 - Entering an invalid bug number (or alias) should warn the user which field is incorrect

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[timeless]

Steps:
1. enter "testing" in the Blocks: field
2. enter a summary
3. click commit

or

click the url link.

Actual results:
Bugzilla Version 2.17.6
Invalid Bug ID
	  	
The 'bug number' is invalid. It is neither a bug number nor an alias to a bug
number. If you are trying to use QuickSearch, you need to enable JavaScript in
your browser. To help us fix this limitation, add your comments to bug 70907.

Expected results:
friendly error explaining that depends on and blocks are lists of bug numbers,
and you entered <what you entered> into <where you entered it>

Comment 1

[Dave Miller [:justdave]]

This is actually a serious usability issue.  Newbies run into it all the time. 
We need to kill it off.

Comment 2

[Shane H. W. Travis]

I fixed this in 2.18rc2, then went to ensure that my patch still worked for 
rc3... and it appears that someone has already dealt with this between rc2 and 
rc3 because my patch was pretty much a duplicate. 

I assume that the existing changes are acceptable (they exist in CGI.pl and 
globals/user-error.html.tmpl), but this is a 2.18 blocker so it needs to be 
closed off and moved out of the way.

Comment 3

[Frédéric Buclin]

(In reply to comment #0)
> Expected results:
> friendly error explaining that depends on and blocks are lists of bug numbers,
> and you entered <what you entered> into <where you entered it>

Aliases are also accepted if used by the installation. They are automatically
converted to IDs by ValidateBugID. Note that I updated the error message a bit,
see bug 269294. Now, the typical error message looks like:

'<what you entered>' is not a valid bug number nor an alias to a bug number.

timeless, justdave, is that enough for you or do you want a more complete error
message? In this last case, you need to give to ValidateBugID an additional
parameter to tell him which field is concerned.

You may assign this bug to me if nobody is working on it already and the actual
solution (the one mentioned above) gives you no satisfaction.

Comment 4

[timeless]

yeah, i'd kind of like to be told which field i botched.

Comment 5

[Frédéric Buclin]

Attachment: Patch for ValidateBugID + (lot of) cleanup, v1

This patch is much more general than just fixing the problem of the "Blocks"
field in post_bug.cgi.

Several points to note (to make the review easier):
- The second parameter of ValidateBugID has been changed to $field instead of
$skip_authorization. The point is that $skip_authorization == 1 only when
$field is "dependson" or "blocked". The $field param, if defined, is then used
in the error message, if tests fail.

- While checking for invalid bug number error messages, I found several
obsolete ones, see attachment.cgi (already checked by validateID()) and
process_bug.cgi (already checked by ValidateBugID()), as well as
user-error.html.tmpl (where I also found a dupe!!). I deleted them!

- I made some cleanup in process_bug.cgi (again :)) about duplicates as this
part is concerned by the error messages we are talking about here. Part of this
cleanup is already mentioned in bug 271023.

- The error message now correctly handles empty / invalid (non numeric) and
specifies which field is concerned, when appropriate.

Comment 6

[Frédéric Buclin]

Adding dependency to bug 271023 as this patch affects process_bug.cgi and fixes
some points mentioned there.

Comment 7

[Frédéric Buclin]

Comment on attachment 167768 [details] [diff] [review]
Patch for ValidateBugID + (lot of) cleanup, v1

myk, this patch is easier (and safer) to review than some others I already
asked you to review! ;)

Comment 8

[Frédéric Buclin]

Another suggestion is:

+    [% IF bug_id %]
+      The value '[% bug_id FILTER html %]' [% IF field %] you entered in the
+      '[% field_descs.$field FILTER html %]' field [% END %] is not a valid
+      [% terms.bug %] number
+      [% IF Param("usebugaliases") %]
+        nor an alias to [% terms.abug %] number
+      [% END %]. Please provide a valid one!
+    [% ELSE %]
+      [% IF field %]
+        The '[% field_descs.$field FILTER html %]' field cannot be empty.
+      [% END %]
+      You must enter a valid [% terms.bug %] number!
+    [% END %]

timeless, myk, which version do you prefer?

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 167768 [details] [diff] [review]
Patch for ValidateBugID + (lot of) cleanup, v1

>-# 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.
...
>     /^duplicate$/ && CheckonComment( "duplicate" ) && do {
...
>+        # Make sure we can change the original bug (issue A on bug 96085)

I believe this change means that the validation code only runs if users are
required to submit comments when marking duplicates, while it should actually
run whether or not the user is required to submit comments in that situation.

Comment 10

[Frédéric Buclin]

(In reply to comment #9)
> I believe this change means that the validation code only runs if users are
> required to submit comments when marking duplicates, while it should actually
> run whether or not the user is required to submit comments in that situation.
> 

Sorry, you are wrong. CheckonComment() checks if the user needs to enter a
comment and if yes, then checks than the user has given one. It returns 1 if no
comment is required or if a comment is required and the user has given one; else
this function calls ThrowUserError() (and returns 0). That means the validation
*is* done everytime, except if the program has stopped before due to
ThrowUserError()!

So there is no risk here! :)

Comment 11

[Frédéric Buclin]

Comment on attachment 167768 [details] [diff] [review]
Patch for ValidateBugID + (lot of) cleanup, v1

requesting myk's review again per my previous comment!

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 167768 [details] [diff] [review]
Patch for ValidateBugID + (lot of) cleanup, v1

Hmm, ok, r=myk.  Those CheckonComment calls should probably be the first item
in each block rather than part of the conditional for the blocks, but that's a
different bug.

Comment 13

[Jacob Steenhagen]

Checked into tip and 2.18.

*NOTE* 2.18 didn't have the "invalid_group_name" error in user-error.html.tmpl
so the part of the patch that removed it didn't apply.


Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.217; previous revision: 1.216
done
Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.63; previous revision: 1.62
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.92; previous revision: 1.91
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.221; previous revision: 1.220
done
Checking in template/en/default/global/field-descs.none.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/field-descs.none.tmpl,v
 <--  field-descs.none.tmpl
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v
 <--  user-error.html.tmpl
new revision: 1.78; previous revision: 1.77
done


Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.211.2.3; previous revision: 1.211.2.2
done
Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.58.2.1; previous revision: 1.58
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.88.2.2; previous revision: 1.88.2.1
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.205.2.11; previous revision: 1.205.2.10
done
Checking in template/en/default/global/field-descs.none.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/field-descs.none.tmpl,v
 <--  field-descs.none.tmpl
new revision: 1.2.2.1; previous revision: 1.2
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v
 <--  user-error.html.tmpl
new revision: 1.61.2.7; previous revision: 1.61.2.6
done