25521 - Keyword field in new bug entry

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P2)

Description

[leger]

terry-

Can you make it so the keyword field is available to info entry in the 
http://bugzilla.mozilla.org/enter_bug.cgi page.

This was the eng could add "helpwanted" or whatever when writing the bug. 

thanks!

Comment 1

[leger]

terry, more engineers are asking for this, can you do when you get a chance..ok?

Comment 2

[Terry Weissman]

The bug entry form is one that we try to keep *simple*.  I am very very
reluctant to add new fields there.

Comment 3

[Terry Weissman]

*** Bug 27384 has been marked as a duplicate of this bug. ***

Comment 4

[Matthew Tuck [:CodeMachine]]

*** Bug 28345 has been marked as a duplicate of this bug. ***

Comment 5

[Matthew T (active 1999-2002)]

*** Bug 28345 has been marked as a duplicate of this bug. ***

Comment 6

[Matthew T (active 1999-2002)]

In bug 28345, I suggested that the Keywords field be available when reporting a 
new bug, but only for those users who have change-all-aspects-of-any-bug 
permissions. That way, engineers and QA people get access to the field, and 
novice bug reporters don't. Problem solved.

Comment 7

[Matthew Tuck [:CodeMachine]]

This is really a feature regression over description/whiteboard flags, so I
wouldn't describe it as a new field as such.

Comment 8

[Sebastian Späth]

Adding a crasher bug without being able to mark it as such in the keywords at 
the first pass is *really* hard. I agree do not add it to the new form to keep 
it simple, but add it to the advanced form (or have it only when reporter has 
"advanced" rights)

Comment 9

[Matthew Tuck [:CodeMachine]]

See the super dooper new bug #31144 for a proposal which would allow this and
more.

Comment 10

[Terry Weissman]

[e-mail] is the new owner of Bugzilla and Bonsai.  (For details,
see my posting in netscape.public.mozilla.webtools,
news://news.mozilla.org/38F5D90D.F40E8C1A%40geocast.com .)

Comment 11

[leger]

Updating QA Contact

Comment 12

[Jesse Ruderman]

*** Bug 40752 has been marked as a duplicate of this bug. ***

Comment 13

[Matthew Tuck [:CodeMachine]]

I don't particularly see why we can't add stuff to the new bug entry like so:

New Bug Entry

.... beginner stuff ...

[ Submit Bug ]

----- Anything below this line is optional and can be ignored by novice Bugzilla
users. -----

.... advanced stuff .....

[ Submit Bug ]

Comment 14

[Dave Miller [:justdave]]

*** Bug 70268 has been marked as a duplicate of this bug. ***

Comment 15

[timeless]

if i wrote a patch, would it be rejected?

Comment 16

[Daniel A. Steffen]

Attachment: unified diff against bugzilla TOT implementing keywords on the bug entry page

Comment 17

[timeless]

cool. hrm, can we have someone research the pro/cons of using a multiselect 
listbox instead of an edit field?

Comment 18

[Matthew Tuck [:CodeMachine]]

Personally, I wouldn't approve this patch without creating a new advanced entry
section that newbies don't have to worry about, as I have outlined.  We have
various fields that people want to add here as well.  We could probably move the
assignee into this section as well.

I have two problems with a listbox.  Firstly, on an installation with lots of
keywords, it's really hard to see what has been selected (you might not be sure
what you did if you lose your way).  And secondly, it's inconsistent with the
bug page.

Comment 19

[Matthew Tuck [:CodeMachine]]

OK, I'm about to attach my proposed patch for this, separating the enter_bug
page into two halves.  I'm not an experienced Bugzilla coder, so beware:

My indentation is probably backwards.
If I kept the table tags balanced it's a minor miracle.
The button list is repeated but is not in a Sub.  Same with some keyword code in
post_bug.cgi lifted from process_bug.cgi.
The layout is probably wrong with certain combinations of fields on.
You may disagree about what should be in the "advanced" section.
We may want to add an option such that certain installations would not get the
message indicating that bug reporters can ignore the advanced section, for
example if all of their users are well trained.
And let's not forget it will fry your hard drive.

So in summary, if you put this patch on your production system, you're likely to
get what you deserve, mmkay?

Comment 20

[Matthew Tuck [:CodeMachine]]

Attachment: Rough patch.

Comment 21

[Dave Miller [:justdave]]

*** Bug 95196 has been marked as a duplicate of this bug. ***

Comment 22

[Zach Lipton [:zach]]

moving

Comment 23

[Keith Kemp]

*** Bug 99567 has been marked as a duplicate of this bug. ***

Comment 24

[Gervase Markham [:gerv]]

I'm templatising enter_bug.cgi. I may be able to do this at the same time.

Gerv

Comment 25

[Dave Miller [:justdave]]

We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.

Comment 26

[Gervase Markham [:gerv]]

Anyone who cares about this should feel free to produce a patch for the new
templatised version.

Gerv

Comment 27

[Jeff Hedlund]

Attachment: patch v.3

Updated patch as requested from Gerv.

I did not add "status whiteboard" to the bug entry as "Rough Patch" did, since
that doesn't seem to be a part of this bug.

Comment 28

[Andreas Franke (gone)]

     $zz = @::legal_severity;
+        $zz = @::legal_keywords;
     $zz = %::target_milestone;

Is this an evil tab? :-)

Comment 29

[Gervase Markham [:gerv]]

Comment on attachment 70967 [details] [diff] [review]
patch v.3

>+# cut & paste from process_bug, better to reuse somehow?

No - because it's going to be different. See below :-)

>+my @keywordlist;
>+my %keywordseen;
>+
>+if ($::FORM{'keywords'}) {
>+  foreach my $keyword (split(/[\s,]+/, $::FORM{'keywords'})) {
>+    if ($keyword eq '') {
>+	   next;
>+	 }
>+	 my $i = GetKeywordIdFromName($keyword);
>+	 if (!$i) {
>+	   PuntTryAgain("Unknown keyword named <code>" .
>+	   				  html_quote($keyword) . "</code>. " .
>+	                 "<P>The legal keyword names are " .
>+				        "<A HREF=describekeywords.cgi>" .
>+				        "listed here</A>.");

I think that, for entering bugs, we should just silently ignore keywords which
aren't keywords. This is much nicer behaviour than slapping the user round the
face with their lack of knowledge. You see, I can see people filling this in
with "helpful" key words about their problem, which Bugzilla won't recognise
most of.

>+    }
>+	 if (!$keywordseen{$i}) {
>+	   push(@keywordlist, $i);
>+	   $keywordseen{$i} = 1;

You make @keywordlist, and never use it. You only need %keywordseen.

>+foreach my $keyword (keys %keywordseen) {
>+	 SendSQL("insert into keywords (bug_id, keywordid) values ($id, $keyword)");

Nit: please capitalise MySQL keywords - it makes the SQL easier to read.

Gerv

Comment 30

[Bradley Baetz (:bbaetz)]

>I think that, for entering bugs, we should just silently ignore keywords which
>aren't keywords. This is much nicer behaviour than slapping the user round the
>face with their lack of knowledge. You see, I can see people filling this in
>with "helpful" key words about their problem, which Bugzilla won't recognise
>most of.

I disagree. Typo's happen, + you get the problem of people typing stuff in, and
it magically vanishing. You can't change keywords unless you have editbugs, so
just require it for this, too. See mpt's comment #6.

Comment 31

[Gervase Markham [:gerv]]

Mpt is right - please implement his solution :-) A good name for the template
variable is "canedit".

Gerv

Comment 32

[Jeff Hedlund]

Attachment: Patch v.4

removed the evil tab
user must be in 'editbugs' group
this patch also includes patch from bug 127318 to enable the above fix
other code cleanup as suggested

Comment 33

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

Reassigning to patch author.

Comment 34

[xyzzy]

*** Bug 140921 has been marked as a duplicate of this bug. ***

Comment 35

[Jeff Hedlund]

Attachment: patch v.5

New patch for against the tip, should work fine patching against 2.16 as well.

Comment 36

[Bradley Baetz (:bbaetz)]

Comment on attachment 93446 [details] [diff] [review]
patch v.5

If an invalid keyword is entered, then bits of the bug will be submitted anyway
- see bug 154036.

You need to move the checks up higher in the routine.

Comment 37

[Jeff Hedlund]

Attachment: patch v.6

Moved keyword validity checks before any SQL INSERTs

Comment 38

[Gervase Markham [:gerv]]

Comment on attachment 93569 [details] [diff] [review]
patch v.6

>+# Check for valid keywords and create list of keywords to be added to db
>+# (validity routine copied from process_bug.cgi)

Can we not factor this out, then?

>+my @keywordlist;
>+my %keywordseen;
>+
>+if ($::FORM{'keywords'}) {
>+    foreach my $keyword (split(/[\s,]+/, $::FORM{'keywords'})) {
>+        if ($keyword eq '') {
>+           next;
>+        }
>+        my $i = GetKeywordIdFromName($keyword);
>+        if (!$i) {
>+            ThrowUserError("Unknown keyword named <code>" . html_quote($keyword)
>+                           .
>+                           "</code>. <p>The legal keyword names are
>+                           <a href=\"describekeywords.cgi\">listed here</a></p>
>+                           .");

Please use the new API for ThrowUserError: 
$vars->{'keyword'} = $keyword;
ThrowUserError("unknown_keyword");
And then put the error in global/messages.html.tmpl .

>+  [% IF UserInGroup('editbugs') %]
>+    <tr>
>+      <td align="right" valign="top">
>+        <strong>
>+          <a href="describekeywords.cgi">Keywords:</a>

Nit: The colon shouldn't be linked.

As it stands, the UI is protected with "editbugs" but the process_bug is not -
so people without this permission could add keywords by hacking the URL.

Gerv

Comment 39

[Bradley Baetz (:bbaetz)]

Factoring out the code really means removing post_bug and using process_bug
instead. Theres not much factoring out which can be done without doing that too
(or having the two files look virtually alike)

Comment 40

[Gervase Markham [:gerv]]

bbaetz: is there a plan to combine post_bug and process_bug? Or have some common
module?

Gerv

Comment 41

[Bradley Baetz (:bbaetz)]

I want there to be such a plan :) Its a prereq for any sensible working email
bug submission, really.

My point was that that issue shouldn't hold this patch up.

Comment 42

[Jeff Hedlund]

Attachment: patch v.7

Implements changes and brings up to date to current CVS.

Comment 43

[Gervase Markham [:gerv]]

Comment on attachment 98551 [details] [diff] [review]
patch v.7

2xr=gerv. This needed a "colspan=3" on the <td>, but was otherwise OK.

Gerv

Comment 44

[Gervase Markham [:gerv]]

Fixed. Finally :-)

Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.64; previous revision: 1.63
done
Checking in template/en/default/bug/create/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/create/create.html.tmpl,v
 <--  create.html.tmpl
new revision: 1.12; previous revision: 1.11
done

Gerv

Comment 45

[dan johnson]

I just upgraded our version of Bugzilla from 2.10 to 2.16.3 and I don't see 
this fix in.  Is it supposed to be?  Is there anything I need to do to turn it 
on?

Comment 46

[Dave Miller [:justdave]]

That was fixed in 2.17.1.  You'll need to upgrade again if you want it.