661476 - sanitycheck.cgi should check if all products have components defined

Status: RESOLVED FIXED

(Bugzilla :: Database, enhancement, P3)

Description

[Mariusz Smykuła]

User-Agent:       Mozilla/5.0 (X11; Linux i686) AppleWebKit/534.24 (KHTML, like Gecko) Chrome/11.0.696.71 Safari/534.24
Build Identifier: 4.0.1

After migration from old version 3.0 some of products was migrated without any components defined. Migration procedure raises problem with bugs without components but was silent about products without components. 

This is important, because if some products dont have defined components, then on advanced search page list of components is incorrect (products always has components, if components list is empty then visible is components list from next product and last product has empty components list).

Reproducible: Always

Steps to Reproduce:
1. Remove all components from product (manual from db?)
2. Run Sanity Check


Actual Results:  
All is OK

Expected Results:  
Error should be raised about missing components in products

Comment 1

[Mariusz Smykuła]

Example:

If we have in database

Product    Component

A          A1
           A2
B          (null)
C          C1
           C2

Then on Advanced Search page we have something like this 

Product    Component
A          A1
           A2
B          C1
           C2
C          (null)

Comment 2

[Frédéric Buclin]

It's legal to have products without components (though that's not very useful, but that's another story), so sanitycheck.cgi shouldn't complain about this.

The problem you describe about the Advanced Search page is already reported in bug 622487.

Comment 3

[Max Kanat-Alexander]

No, I actually think we *should* check for this in sanitycheck. There's no reason to have a product with no components and it may cause problems in various places.

Comment 4

[Mariusz Smykuła]

If bug without component is error (migration treat this as error) then howto add bug to product without component defined? 

I think this was something like default component in 3.0 but this was lost after migration to 4.x.

Comment 5

[Mariusz Smykuła]

In 4.0.1 if there is no component defined in product, then in Edit Product page word "missing" is printed as RED:

Edit components:	missing

This not look like normal situation for me.

Comment 7

[Frédéric Buclin]

Attachment: patch, v1

I also make sure that you cannot delete the last component or version in remove_from_db(). Milestones are not affected as you cannot delete the default milestone, and so we are sure there is always at least one milestone defined.

Comment 8

[Frédéric Buclin]

FYI, the Product.create webservice method implemented in 4.2 still create products with no components (making this method pretty useless, to be honest). Creating components using the API will be done in bug 419568. The goal of this bug is not to let someone also create a component using Product.create, but to make life easier when using our UI, and to make sanitycheck.cgi detect products with no component or version.

Comment 9

[Max Kanat-Alexander]

  Hmm. I think perhaps a better solution that would catch all possible use-cases would just be to create a default component always, and call it something like "General", perhaps from a template string. You could make the initialowner the product creator. Then we would also handle WebService users and people who are using url-hacking to script Bugzilla.

Comment 10

[Frédéric Buclin]

(In reply to comment #9)
>   Hmm. I think perhaps a better solution that would catch all possible
> use-cases would just be to create a default component always, and call it
> something like "General", perhaps from a template string. You could make the
> initialowner the product creator.

This would catch all possible use-cases, I agree, but it would also most of the time require the product creator fix the component attributes as the admin of an installation is rarely a developer and so is probably not the assignee. At least, your solution would be less useful than mine for at least the GCC, Mandriva and Mageia teams. So I still prefer my solution. :)

Comment 11

[Max Kanat-Alexander]

(In reply to comment #10)
> This would catch all possible use-cases, I agree, but it would also most of
> the time require the product creator fix the component attributes as the
> admin of an installation is rarely a developer and so is probably not the
> assignee. At least, your solution would be less useful than mine for at
> least the GCC, Mandriva and Mageia teams. So I still prefer my solution. :)

  Okay, how about this? In the event that there are no component-creation parameters passed to Product->create, create a default component.

Comment 12

[Bernhard Merkle]

(In reply to comment #9)
>   Hmm. I think perhaps a better solution that would catch all possible
> use-cases would just be to create a default component always, and call it
> something like "General", perhaps from a template string. You could make the
> initialowner the product creator. Then we would also handle WebService users
> and people who are using url-hacking to script Bugzilla.

I would not vote for a "General" component. And it does not fix the initial problem. Still people/admin can delete all components for a product and then there is a problem which i would expect that sanitycheck will find it.

Comment 13

[Frédéric Buclin]

(In reply to comment #12)
> problem. Still people/admin can delete all components for a product

No, my patch prevents that. You no longer can delete all components or versions.

Comment 14

[Bernhard Merkle]

(In reply to comment #13)
> (In reply to comment #12)
> > problem. Still people/admin can delete all components for a product
> 
> No, my patch prevents that. You no longer can delete all components or
> versions.

ok, understood, but the patch is avail. since which version of bugzilla ?
and think about:
- older installations which have products without components
- database migrations, done by admins directly via SQL on the db which may result in products without componentns
in such usecases it would be very convenient if sanitycheck gives a hint.

I usually rely on sanitycheck to tell me: "your system is ok and consistent"

Comment 15

[Frédéric Buclin]

(In reply to comment #14)
> ok, understood, but the patch is avail. since which version of bugzilla ?

It works with 4.2 (and probably with 4.0 too, but I didn't check), but it will only be checked in in Bugzilla 5.0. Bugzilla 4.2 is closed for new enhancements.


> in such usecases it would be very convenient if sanitycheck gives a hint.

Did you read my patch? That's what sanitycheck.cgi does. My patch does a lot of things. :)

Comment 16

[Bernhard Merkle]

(In reply to comment #15)

> Did you read my patch? That's what sanitycheck.cgi does. My patch does a lot
> of things. :)

now i read it. great. exactly what we need. :-)

Comment 17

[:glob ✱]

Comment on attachment 548833 [details] [diff] [review]
patch, v1

Review of attachment 548833 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

::: Bugzilla/Component.pm
@@ +157,5 @@
>  
> +    # Products must have at least one component.
> +    if (scalar(@{$self->product->components}) == 1) {
> +        ThrowUserError('component_is_last', { comp => $self });
> +    }

ideally this should happen on the delete confirmation screen; similar to the how we block component deletion when the component contains bugs, however this is not a show-stopper.

::: Bugzilla/Version.pm
@@ +147,5 @@
> +
> +    # Products must have at least one version.
> +    if (scalar(@{$self->product->versions}) == 1) {
> +        ThrowUserError('version_is_last', { version => $self });
> +    }

ditto for versions.

Comment 18

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified editproducts.cgi
modified sanitycheck.cgi
modified Bugzilla/Component.pm
modified Bugzilla/Version.pm
modified template/en/default/admin/components/edit-common.html.tmpl
modified template/en/default/admin/products/create.html.tmpl
modified template/en/default/admin/products/edit-common.html.tmpl
modified template/en/default/admin/sanitycheck/messages.html.tmpl
modified template/en/default/global/messages.html.tmpl
modified template/en/default/global/user-error.html.tmpl
Committed revision 7935.

Comment 19

[Frédéric Buclin]

Added to relnotes for 4.4.