223570 - Creating a bug in a product with no versions results in internal error: A legal Product was not set.

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Jason Pyeron]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0; .NET CLR 1.0.3705; .NET CLR 1.1.4322)
Build Identifier: 

attemped Bug 190611, was not able to reproduce from cvs tip

but caused an error when creating a new bug for the product w/o versions.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Jason Pyeron]

Attachment: ProductGood function / enter_bug.cgi implementation

created a product good function in globals...

use it where you would like to exclude products from entry, query, etc

what they are malformed in the database.

Comment 2

[Christian Reis]

2 questions:

- Why not just alter enter_bug.cgi to do an additional check for versions as we
already do for no components (grep for no_components if you haven't seen this).
You should then add a message for the user in the line of the error
no_components has.

- If we can't (for some reason I can't see), is there a reason we don't want to
use CanEnterProduct to do this as an additional check? I.E. does it make sense
to have CanEnterProduct() return ok if there is no version or component in it?

nit: I would much rather we wrapped new code at 72 chars.

Comment 3

[Jason Pyeron]

re question 1:
This error is thrown to the user eventually, if entry is attempted, what I am 
trying to fix is a consistancy issue:

 Dont allow actions (through normal program flow) which are know to cause a 
runtime error.

re question 2:
I have seperated from CanEnterProduct since its logic (by name) is different 
and I have not analyzed each use of CanEnterProduct to see if an edit would not 
break usage.

re nit pick:

if I need to re-edit this patch for any other reason then I will wrap at my 
editors margin (i think 80), if you or any one else hase time then doit.

Comment 4

[Christian Reis]

Well, I *really* think that the no_versions check is required here, since it
short-circuits the error procedure -- the error is a lot clearer.

If you want to avoid the runtime error to show up *when somebody is posting to
post_bug.cgi directly* (such as via an external client) then your patch is
warranted, you're right.

I would be happy to review a patch that addresses both issues (and ideally
wrapped at 7x-79 columns -- I try to keep 72 as a rule, but 79 is fine).

Comment 5

[Jason Pyeron]

re comment 4:
> If you want to avoid the runtime error to show up *when somebody is posting to
> post_bug.cgi directly* (such as via an external client) then your patch is
> warranted, you're right.


actually it is the opposite issue, it is when the user is interactive, allowing 
them to click products which are known to not allow (by groups restriction or 
product validity) entry is improper.

a tool which accesses post_bug.cgi directly, can then handle the error from 
post_bug

Comment 6

[Christian Reis]

To summarize: what makes the approach used by no_components not applicable here,
then?

Comment 7

[Vlad Dascalu]

Comment on attachment 134052 [details] [diff] [review]
ProductGood function / enter_bug.cgi implementation

The identation used (one-space identation and more than 79 chars per line) make
this impossible to check in, in the current form.

Also see kiko's question(s) above.

Comment 8

[Jason Pyeron]

Attachment: bugzilla-223570-v1.04.diff

fixed some bit rot, word wrap, etc...

(In reply to comment #6)
> To summarize: what makes the approach used by no_components not applicable
here,
> then?

wrong place and time, the user should not see products which cannot have entry.


akin to closed for entry, no permissions, etc.

Comment 9

[Christian Reis]

Comment on attachment 160901 [details] [diff] [review]
bugzilla-223570-v1.04.diff

>Index: globals.pl
>+# This function determines if the product has versions/components hence it
>+# can have new bugs entered ...

This function shouldn't do two things at the same time, then. Why not something
like:

  GetVersionCount($productname)
  GetComponentCount($productname)

And have both of those called instead? It would make them more generally
useful, and things a lot more easier when moving Product to be a class instead
of a bunch of loosely tied functions in global.pl.

Comment 10

[Jason Pyeron]

one of the better nits I have seen in a long time, consider it done.

Comment 11

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

"If it's not a regression from 2.18 and it's not a critical problem with
something that's already landed, let's push it off." - Dave

Comment 12

[Frédéric Buclin]

Jason, ping? Implement the version check in CanEnterProduct and hack
CanEnterProductOrWarn accordingly.

Comment 13

[Jason Pyeron]

thanks for the ping, I am putting it on my calendar to work on.
Monday June 6, 2005 I will work on it.

Comment 14

[Max Kanat-Alexander]

Hey Jason. Have you done any work on it? If it's a simple patch, it would be
nice to have it for 2.20.

Comment 15

[A. Karl Kornel]

So does this mean that products absolutely must have at least one version
associated with them?  (see bug 295933)

Comment 16

[A. Karl Kornel]

Attachment: Patch v3

Takes attachment 160901 [details] [diff] [review] and modifies it according to comment 9 and comment 12. 
CanEnterProductOrWarn updated to throw a newly-created userError if there are
no versions defined for a product.

Re my comment 10, I'm assuming that for now the answer to my question is "yes,
ast least for now".

This should probably obsolete attachment 160901 [details] [diff] [review].

Comment 17

[Frédéric Buclin]

Comment on attachment 187227 [details] [diff] [review]
Patch v3

>+sub GetVersionCount {
>+    my ($productname) = @_;
>+    my $dbh = Bugzilla->dbh;
>+
>+    return 0 unless defined($productname);
>+    trick_taint($productname);
>+
>+    # Query for the number of versions associated with the given product
>+    my $query = 'SELECT COUNT(*) FROM versions ' .
>+                'LEFT JOIN products ON versions.product_id = products.id ' .
>+                'WHERE products.name = ' . $dbh->quote($productname);
>+
>+    my ($versions_for_product) =  $dbh->selectrow_array($query);
>+    return $versions_for_product;
>+}

1) Use the syntax $dbh->selectrow_array($query, undef, @params).
2) GetVersionCount() should either be part of CanEnterProduct() or you should
use the routine defined in Version.pm, see bug 294160.


>+sub GetComponentCount {

This routine is useless as CanEnterProduct() already includes this one.


>+  [% ELSIF error == "no_versions" %]
[...]
>   [% ELSIF error == "no_components" %]

These two error messages can certainly be merged into a single one.

Comment 18

[A. Karl Kornel]

Attachment: Patch v4

Modification of attachment 187227 [details] [diff] [review], taking into account comment 17.

GetComponentCount removed; 'no_versions' and 'no_components' user errors
consolidated into one error, with appropriate text; all enter_bug changes
removed (since all new code is in CanEnterProduct).  GetVersionCount is being
rolled into CanEnterProduct.

Requesting review.  LpSolit: Could you mark 'bugzilla-223570-v1.04.diff' as
obsolete?  Thanks!

Comment 19

[Frédéric Buclin]

Comment on attachment 187470 [details] [diff] [review]
Patch v4

>+    # If the user can enter bugs into this product, and at least one component 
>+    # exists for the product, see if at least one version exists for the 
>+    # product.

This sentence is wrong. Actually, we still don't know if the user can enter
bugs into that product. It depends whether the product has versions defined or
not.


>+    if (defined $allow_new_bugs) {
>+        $allow_new_bugs =
>+            $dbh->selectrow_array('SELECT product_id FROM versions LEFT JOIN ' .
>+                                  'products ON versions.product_id = ' .
>+                                  'products.id WHERE products.name = ? ' .
>+                                  $dbh->sql_limit(1), 
>+                                  undef, $productname);
>+        $allow_new_bugs = 1 if defined($allow_new_bugs);
>+    }

Don't write a new SQL query. Add the 'versions' table to the previous query
instead, using a LEFT JOIN, and check whether at least one version is returned
(versions.value IS NULL).


>     # Return 1 if the user can enter bugs into that product;
>     # return 0 if the product is closed for new bug entry;
>-    # return undef if the product has no component.
>+    # return undef if the product has no component or no version.

We want to distinguish between a product having no component and a product
having no version. My suggestion above allows you to know that.


>-        ThrowUserError("no_components", { product => $product});
>+        ThrowUserError("missing_version_or_component", { product => $product});

Again, a single message saying "maybe it's due to A, maybe due to B" is not
acceptable.


>+  [% ELSIF error == "missing_version_or_component" %]
>+    [% title = "Missing Version or Component" %]
>+    Sorry; there needs to be at least one version and at least one component 
>+    associated with the product [% product FILTER html %] in order to 
>+    create a new [% terms.bug %].  
>+    [% IF UserInGroup("editcomponents") %]
>+      <a href="editversions.cgi?product=[% product FILTER url_quote %]">Create
>+        a new version</a> or 
>+      <a href="editcomponents.cgi?product=[% product FILTER url_quote %]">Create
>+        a new component</a>.
>+    [% ELSE %]
>+       Please contact [% Param("maintainer") %], giving the name of
>+       the product in which you tried to create a new [% terms.bug %].
>+    [% END %]

When I told you to write a single error message, I had in mind to pass the
reason of the error:
ThrowUserError("<error_string>", { product => $product, <param_name> =>
'version'|'component' })

Comment 20

[A. Karl Kornel]

(In reply to comment #19)
> 
> This sentence is wrong. Actually, we still don't know if the user can enter
> bugs into that product. It depends whether the product has versions defined or
> not.

OK.  This will be updated.

> 
> Don't write a new SQL query. Add the 'versions' table to the previous query
> instead, using a LEFT JOIN, and check whether at least one version is returned
> (versions.value IS NULL).

OK.  This will be updated.

> >     # Return 1 if the user can enter bugs into that product;
> >     # return 0 if the product is closed for new bug entry;
> >-    # return undef if the product has no component.
> >+    # return undef if the product has no component or no version.
> 
> We want to distinguish between a product having no component and a product
> having no version. My suggestion above allows you to know that.

Calls to CanEnterProduct that are made by by Bugzilla/Bug.pm, enter_bug.cgi, and
describecomponents.cgi.  Those calls will need to be changed if the return
values of CanEnterProduct are changed.  Is that OK?

> <<<snip>>>
> 
> When I told you to write a single error message, I had in mind to pass the
> reason of the error:
> ThrowUserError("<error_string>", { product => $product, <param_name> =>
> 'version'|'component' })
> 

With that being the case, I would have thought that two error messages would be
preferred, but I'll make the change anyway.

Comment 21

[Frédéric Buclin]

(In reply to comment #20)
> (In reply to comment #19)
> > 
> > This sentence is wrong. Actually, we still don't know if the user can enter
> > bugs into that product. It depends whether the product has versions defined or
> > not.
> 
> OK.  This will be updated.

Remove this sentence completely!


> Calls to CanEnterProduct that are made by by Bugzilla/Bug.pm, enter_bug.cgi, and
> describecomponents.cgi.  Those calls will need to be changed if the return
> values of CanEnterProduct are changed.  Is that OK?

That's not OK! If CanEnterProduct() is called without the second parameter set
($verbose undefined), then it should return 0. No need to alter other files this
way.


> With that being the case, I would have thought that two error messages would be
> preferred, but I'll make the change anyway.

If the only change is the word 'version' vs 'component', write a single error
message. If there are more changes, write two distinct error messages.


I'm also reassigning the bug to you. ;)

Comment 22

[A. Karl Kornel]

Attachment: Patch v5

(In reply to comment #21)
> 
> Remove this sentence completely!

That's what I meant! 8-)

> That's not OK! If CanEnterProduct() is called without the second parameter
set
> ($verbose undefined), then it should return 0. No need to alter other files
this
> way.

Woah!  OK.  The return value is unchanged if !defined $verbose.

> If the only change is the word 'version' vs 'component', write a single error

> message. If there are more changes, write two distinct error messages.

Updated.

Comment 23

[Frédéric Buclin]

Comment on attachment 187855 [details] [diff] [review]
Patch v5

>+    # at least one component and has at least one version.

"at least one component and one version."


>+    my ($allow_new_bugs,$component_id,$version_id) =
>+        $dbh->selectrow_array('SELECT CASE WHEN disallownew = 0 THEN 1 ELSE 0 END, ' .
>+                              'components.product_id, versions.product_id ' .
>+                              'FROM products LEFT JOIN components ' .
>+                              'ON components.product_id = products.id ' .
>+                              'LEFT JOIN versions ' .
>+                              'ON versions.product_id = products.id ' .
>+                              'WHERE products.name = ? ' .
>                                $dbh->sql_limit(1),
>                                undef, $productname);

This SQL query is ugly. What I suggested is something like (pseudo-code):

$allow_new_bugs, $has_version =
SELECT CASE WHEN disallownew = 0 THEN 1 ELSE 0 END, versions.value IS NOT NULL
FROM products
INNER JOIN components ON components.product_id = products.id
LEFT JOIN versions ON versions.product_id = products.id
WHERE products.name = ? LIMIT 1;

This way, we have:
1) $allow_new_bugs == 1 && $has_version == 1 => allow new bugs
2) $allow_new_bugs == 0 => product closed for new bug entry
3) $allow_new_bugs undefined => product has no component

So this part does not change... but... what's new now is:
4) $allow_new_bugs == 1 && $has_version == 0 => product has no version

In order to return so much information at once, we need to do something like:

if (defined($verbose)) {
    return ($allow_new_bugs, $has_version);
else {
    return ($allow_new_bugs && $has_version);
}

and CanEnterProductOrWarn() would now take an array instead of a scalar. This
way, you don't need to change anything outside CanEnterProduct() and
CanEnterProductOrWarn(). I cannot think of a better solution right now.


>+  [% ELSIF error == "missing_version_or_component" %]
>+    [% title = "Missing Version or Component" %]
>+    Sorry; there needs to be at least 
>+    [% IF status == -2 %]
>+      one component 
>+    [% ELSIF status == -3 %]
>+      one version 
>+    [% ELSIF status == -4 %]
>+      one version and one component 
>+    [% END %]
>+    associated with the product [% product FILTER html %] in otder to create a 
>+    new [% terms.bug %].  
>+    [% IF UserInGroup("editcomponents") %]
>+      [% IF ((status == -4) || (status == -3)) %]
>+        <a href="editversions.cgi?product=[% product FILTER url_quote %]">Create 
>+        a new version</a>.
>+      [% END %]
>+      [% IF ((status == -4) || (status == -2)) %]
>+        <a href="editcomponents.cgi?product=[% product FILTER url_quote %]">Create 
>+        a new component</a>.
>+      [% END %]
>+    [% ELSE %]
>+       Please contact [% Param("maintainer") %], giving the name of
>+       the product in which you tried to create a new [% terms.bug %].
>+    [% END %]

Are you able to read this?? ;)

What I suggested is:

Sorry; there needs to be at least one [% name FILTER html %] associated with
the product [% product FILTER html %] in order to create a new [% terms.bug %].
[....] Create a new [% name FILTER html %].

Comment 24

[A. Karl Kornel]

Attachment: Patch v6

(In reply to comment #23)
> This SQL query is ugly. What I suggested is something like (pseudo-code):

I don't think it looks that ugly!  At least, it seems just as efficient as what
you list below.

> $allow_new_bugs, $has_version =
> SELECT CASE WHEN disallownew = 0 THEN 1 ELSE 0 END, versions.value IS NOT
NULL
> FROM products
> INNER JOIN components ON components.product_id = products.id
> LEFT JOIN versions ON versions.product_id = products.id
> WHERE products.name = ? LIMIT 1;

SQL updated.

> This way, we have:
> 1) $allow_new_bugs == 1 && $has_version == 1 => allow new bugs
> 2) $allow_new_bugs == 0 => product closed for new bug entry
> 3) $allow_new_bugs undefined => product has no component
> 
> So this part does not change... but... what's new now is:
> 4) $allow_new_bugs == 1 && $has_version == 0 => product has no version
> 
> In order to return so much information at once, we need to do something like:

> 
> if (defined($verbose)) {
>     return ($allow_new_bugs, $has_version);
> else {
>     return ($allow_new_bugs && $has_version);
> }

Updated.
 
> Are you able to read this?? ;)

Actually, yes!

> 
> What I suggested is:
> 

Updated.

Comment 25

[Frédéric Buclin]

Comment on attachment 187981 [details] [diff] [review]
Patch v6

>-    # Return 1 if the user can enter bugs into that product;
>-    # return 0 if the product is closed for new bug entry;
>-    # return undef if the product has no component.

We really need to make clear what is returned by this routine. Please update my
comment instead of removing it.


>+    my ($allow_new_bugs, $has_version) = 
>+        $dbh->selectrow_array('SELECT CASE WHEN disallownew = 0 THEN 1 ELSE 0 END, ' .
>+                              'versions.value IS NOT NULL ' .
>+                              'FROM products INNER JOIN components ' .
>+                              'ON components.product_id = products.id ' .
>+                              'LEFT JOIN versions ' .
>+                              'ON versions.product_id = products.id ' *MISSING DOT HERE*
>+                              'WHERE products.name = ? ' .
>+                              $dbh->sql_limit(1), undef, $productname);
>+
>+    if (defined $verbose) {
>+        return ($allow_new_bugs, $has_version);
>+    } else {
>+        return ($allow_new_bugs && $has_version);
      *MISSING BRACKET HERE*
> }

The patch generates errors. Please run ./runtests.pl before submitting patches
and fix all errors returned. Fix the two mentioned here. Btw, I see no reason
to add quotes on every line.


>+    if (!defined $allow_new_bugs) {
>+        ThrowUserError("missing_version_or_component", 
>+                       { product      => $product,
>+                         missing_item => 'component' })
>+    } elsif ($has_version == 0) {

Nit: for consistency, write (!$has_version).
Moreover, doing this test here returns the wrong error message when the user
hasn't access to the product. This test *must* be the last one.


>+  [% ELSIF error == "missing_version_or_component" %]
>+    [% title = "Missing Version or Component" %]

We know what the problem is. Only mention what is really missing:
[% title = BLOCK %]Missing [% missing_item FILTER none %][% END %]


>+    [% IF UserInGroup("editcomponents") %]
>+      <a href="edit[% missing_item FILTER none -%]
>+      s.cgi?product=[% product FILTER url_quote %]">Create a new 
>+      [% missing_item %]</a>.

To make things clearer, move "s.cgi?product=" on the previous line:
<a href="edit[% missing_item FILTER none %]s.cgi?product=
[%- product FILTER url_quote %]">Create a new [% missing_item FILTER none
%]</a>.

Note that you missed one "FILTER none"! ./runtests.pl complained again!


I tested your patch with all these issues fixed. Everything works well. So the
next one should be ok. :)

Comment 26

[A. Karl Kornel]

Attachment: Patch v7

(In reply to comment #25)
> We really need to make clear what is returned by this routine. Please update 

> my comment instead of removing it.

Updated & expanded.

> The patch generates errors. Please run ./runtests.pl before submitting
patches
> and fix all errors returned. Fix the two mentioned here. Btw, I see no reason

> to add quotes on every line.

Updated & tested.  I've got quotes there to keep newlines out of the SQL, which
makes 'SHOW PROCESSLIST' look ugly.
 
> 
> >+	if (!defined $allow_new_bugs) {
> >+	    ThrowUserError("missing_version_or_component", 
> >+			   { product	  => $product,
> >+			     missing_item => 'component' })
> >+	} elsif ($has_version == 0) {
> 
> Nit: for consistency, write (!$has_version).
> Moreover, doing this test here returns the wrong error message when the user
> hasn't access to the product. This test *must* be the last one.

Moved !$has_version test to the end.

Your quote refers to two different tests, but you say "this test".  I assume
you're referring to the latter.

> We know what the problem is. Only mention what is really missing:
> [% title = BLOCK %]Missing [% missing_item FILTER none %][% END %]

Updated, with a small modification.

> To make things clearer, move "s.cgi?product=" on the previous line:
> <a href="edit[% missing_item FILTER none %]s.cgi?product=
> [%- product FILTER url_quote %]">Create a new [% missing_item FILTER none
> %]</a>.

Updated.

Comment 27

[Frédéric Buclin]

Comment on attachment 188072 [details] [diff] [review]
Patch v7

>+    Sorry; there needs to be at least one
>+    [% missing_item FILTER lower FILTER html %] associated with the product
>+    [% product FILTER html %] in order to create a new [% terms.bug %].

There are a few minor things to fix:
1) remove trailing whitespaces at the end of lines.
2) remove "FILTER html" from [% missing_item FILTER lower %] as this string is
hardcoded.
3) to make things look prettier, enclose the product name between <em> </em>.
4) either move [% missing_item FILTER lower %] on the previous line, or write
[%+ missing_item FILTER lower %] in order to add a whitespace before it.

r=LpSolit already. But please update your patch anyway. ;)

Comment 28

[A. Karl Kornel]

Attachment: Patch v7.5

Modification of attachment 188072 [details] [diff] [review] with respect to comment 27.
(In reply to comment #27)
> 1) remove trailing whitespaces at the end of lines.
> 2) remove "FILTER html" from [% missing_item FILTER lower %] as this string
is
> hardcoded.
> 3) to make things look prettier, enclose the product name between <em> </em>.

> 4) either move [% missing_item FILTER lower %] on the previous line, or write

> [%+ missing_item FILTER lower %] in order to add a whitespace before it.

Updated.  I hope you just mean trailing whitespace in my edit, as opposed to
the entire file!

Comment 29

[Frédéric Buclin]

Comment on attachment 188077 [details] [diff] [review]
Patch v7.5

>+      [%- product FILTER url_quote %]">Create a new
>+      [% missing_item FILTER lower %]</a>.

There is a missing whitespace => [%+ missing_item FILTER lower %]. I will fix
that on checkin.

r=LpSolit

Comment 30

[Frédéric Buclin]

Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.326; previous revision: 1.325
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.114; previous revision: 1.113
done