273873 - bug-creation template for UNCONFIRMED makes NEW

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Nick Barnes]

If I make a bug-creation template by clicking on "Remember values as
bookmarkable template" having selected "UNCONFIRMED" as "Initial State", then
when I click on the template I get a bug entry form with "Initial State" of "NEW".

The URL in my bookmark is
enter_bug.cgi?product=TestProduct&version=other&component=TestComponent&rep_platform=PC&op_sys=FreeBSD&priority=P2&bug_severity=normal&bug_status=UNCONFIRMED&assigned_to=nb%2B271474%40ravenbrook.com&cc=&bug_file_loc=http%3A%2F%2F&short_desc=spong&comment=&dependson=&blocked=&maketemplate=Remember%20values%20as%20bookmarkable%20template&form_name=enter_bug

My Bugzilla was updated from the 2.18 branch yesterday, and I'm using Firefox
1.0 (not sure if that's relevant).

This seems pretty minor to me, and hopefully not a 2.18 blocker.

Comment 1

[Vlad Dascalu]

Bug 262141 rang a bell when I read this.

Comment 2

[Nick Barnes]

Attachment: Patch to 2.18 branch tip

Here's a patch for enter_bug.cgi which addresses this.	In a product which has
confirmation, a user who can confirm gets a pulldown with UNCONFIRMED and NEW,
and the default comes from the form if it's an allowable value.  A product
without confirmation gets a fixed NEW, a user who can't confirm gets a fixed
UNCONFIRMED.  A bit more complex than I would have liked, but it works.

Comment 3

[Nick Barnes]

Attachment: Second attempt at this patch

Comment 4

[Nick Barnes]

To clarify a bit of what's going on in this patch: the old code was pretty
bogus.  Here's a description of the old behaviour:

It makes a list of status values, which is either:
1. just "NEW", or
2. "NEW" and "UNCONFIRMED" if the product has confirmation and the user is in
editbugs and canconfirm.

Then it sets the default as the first member of this list (which is always "NEW").

Then the create template doesn't show the status ("Initial State") if the list
has fewer than two entries.

Then post_bug.cgi only takes the bug status from the form if the user is in
canconfirm or editbugs; otherwise it makes it NEW or UNCONFIRMED depending on
whether the product has confirmation.

Note that this is bogus, in passing a hidden bug_status of NEW if the product
has confirmation but the user can't confirm, but harmless as post_bug.cgi then
reverts it to UNCONFIRMED.

With the patch:

- enter_bug.cgi constructs a true list of available statuses;
- any status from a template gets treated as the default if its an available
status (*fixing this bug*);
- the form always has a visible "Initial State", which is read-only if there is
only one possible state and a drop-down otherwise.

Sorry about making two patches; the first one included a debugging print
statement and not enough comments (I think).  Also it defaulted to UNCONFIRMED
if the user can confirm, which is a change from previous behaviour; now it
defaults to NEW (like before).

Comment 5

[Dave Miller [:justdave]]

Comment on attachment 168300 [details] [diff] [review]
Second attempt at this patch

>-# There must be at least one status in @status.
>-my @status = "NEW";
>+# list of status values for drop-down
>+my @status;
> 
>-if (UserInGroup("editbugs") || UserInGroup("canconfirm")) {
>-    SendSQL("SELECT votestoconfirm FROM products WHERE name = " .
>-            SqlQuote($product));
>-    push(@status, $unconfirmedstate) if (FetchOneColumn());
>+# construct the list of allowable values.  If the product has
>+# confirmation, the list includes UNCONFIRMED, and NEW if the user can
>+# confirm.  If the product doesn't have confirmation, the list just
>+# includes NEW.
>+
>+SendSQL("SELECT votestoconfirm FROM products WHERE name = " .
>+        SqlQuote($product));
>+if (FetchOneColumn()) {
>+    if (UserInGroup("editbugs") || UserInGroup("canconfirm")) {
>+        push(@status, "NEW");
>+    }
>+    push(@status, $unconfirmedstate);
>+} else {
>+    push(@status, "NEW");
> }

I'm not sure what the benefit of the above change is, other than the more
complete comments.  It appears to be just a different way of doing exactly the
same thing.  (I'm not denying review for this, this is just a side comment)

>+if (formvalue('bug_status') && grep(formvalue('bug_status'), @status)) {

This is what I'm denying the review for.  The grep() function treats the first
argument as a regexp if it isn't a code block (even if it's in quotes instead
of //).  Since the form value for bug_status is untrusted (it came from the
user's browser) and we haven't sanitized it for regexp metacharacters, this is
a Bad Idea.  On second thought, re-reading the docs for grep(), that
formvalue() call is actually going to be seen as a code block (because it's a
function call), and it's only going to look at whether the result of it is true
or not.  So if the user passed ANYTHING, it'll always be true.	Or that's my
reading on it anyway.  Either way, that syntax spells trouble :)

You should do something like this:  grep({ $_ eq formvalue('bug_status') },
@status)

Comment 6

[Nick Barnes]

(In reply to comment #5)
> [snip]
> I'm not sure what the benefit of the above change is, other than the more
> complete comments.  It appears to be just a different way of doing exactly the
> same thing.  (I'm not denying review for this, this is just a side comment)

I tried to explain in comment 4.  I'll improve the comment in the code to
clarify it.  There are three cases:

1. product has confirmation, user cannot confirm
2. product has confirmation, user can confirm
3. product does not have confirmation.

The old code produces these results:
   @status                           default
1. "NEW"                             "NEW"
2. "NEW", "UNCONFIRMED"              "NEW"
3. "NEW"                             "NEW"

Note that case 1 is all wrong.  It doesn't actually matter right now because the
old template doesn't show the bug_status if there's only one, and the post_bug
code then throws the field away in case 1.  But it's still wrong, and liable to
cause problems if either the template or post_bug gets changed.
My patch produces these results:

   @status                           default
1. "UNCONFIRMED"                     "UNCONFIRMED"
2. "NEW", "UNCONFIRMED"              "NEW"
3. "NEW"                             "NEW"

I had to make this change to fix the bug because otherwise a bookmarked
bug-creation template with status of "UNCONFIRMED" couldn't be used in case 1
(it would fail the "is the template status value in the list of allowable status
values" test).

And my patch to the template makes the status visible even when there's only one
(which I think is a pretty clear improvement).

> >+if (formvalue('bug_status') && grep(formvalue('bug_status'), @status)) {
> 
> This is what I'm denying the review for.

You're completely right, and this is where my ignorance of Perl is showing.  In
Python I would just say "if (form.has_key('bug_status') and form['bug_status']
in status_list)".  But Perl apparently doesn't give me a simple way of asking
"is this value in this list?".  I'll keep my opinion of that shortcoming to
myself, and rewrite it as you suggest.  New patch coming up.

By the way, the text box I'm typing this into is way too small.  I see that it's
hardwired to 10 rows.  Maybe it should be configurable.  Maybe I should enter a bug.

Comment 7

[Nick Barnes]

Attachment: new patch, tested on 2.18 branch and on trunk.

Aha! lsearch()! Also removed some redundant code from the template file.  Still
not tested against trunk.

Comment 8

[Nick Barnes]

Comment on attachment 168401 [details] [diff] [review]
new patch, tested on 2.18 branch and on trunk.

Some time later, after building a load of infrastructure to manage Bugzilla
source trees in Perforce, to branch fresh Bugzilla instances, to setup fresh
Bugzilla databases, and to automatically create patch files, I can confirm that
this patch works on the trunk as well as on the 2.18 branch.

Comment 9

[Dave Miller [:justdave]]

(In reply to comment #6)
> I tried to explain in comment 4.  I'll improve the comment in the code to
> clarify it.  There are three cases:

Aha, I get it now. :)

Comment 10

[Vlad Dascalu]

Checking in enter_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v  <--  enter_bug.cgi
new revision: 1.98; previous revision: 1.97
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.35; previous revision: 1.34
done

Checking in enter_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v  <--  enter_bug.cgi
new revision: 1.94.2.1; previous revision: 1.94
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.30.2.2; previous revision: 1.30.2.1
done