199516 - Allow users to specify a component without specifying a product, on request.cgi

Status: NEW

(Bugzilla :: Attachments & Requests, enhancement, P4)

Description

[timeless]

problem summary: the component field seems to be ignored unless there's a
product field specified.

steps to reproduce, load:
http://bugzilla.mozilla.org/request.cgi?action=queue&component=Mail+Window+Front+End
expected results:
http://bugzilla.mozilla.org/request.cgi?action=queue&component=Mail+Window+Front+End&product=MailNews
until such a time when some other product gets a component by the same name.
actual results:
http://bugzilla.mozilla.org/request.cgi

Comment 1

[Vlad Dascalu]

Attachment: Patch for the problem described.

The code from the CVS tip right now (request.cgi) looks like this:

if (product selected) {
    add restrictions or error if it's not okay.
    if (component selected) {
	add restrictions or error if it's not okay.
    }
}

So it's normal to expect the behaviour described above. To fix it, we change
the last line quoted above into:

} elsif (component selected) {
    # We have a selected component but no product selected

    if there are 2 components with the same name in different products: error
    add restrictions or throwCodeError if it's not okay.
}

The patch has around dozen lines, I tested it and it fixes the problem
described. I've also echoed the $query and it looks okay in every case I could
think of.

Comment 2

[Vlad Dascalu]

<-- me

Comment 3

[Christian Reis]

Comment on attachment 127970 [details] [diff] [review]
Patch for the problem described.

Vlad, don't you think that the better solution would be to fix (or provide a
fixed alternative) get_component_id() to allow only a component name to be
passed in? It would be nice to avoid extra SQL in this code.

That way we could avoid  the code duplication here (even though it's not too
much).

Comment 4

[Dave Miller [:justdave]]

get_component_id needs to do what it does.  This is a special situation, similar
to the query page, perhaps a trick similar to what the query page uses would be
in order (I think it uses build_subselect())

Comment 5

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

Comment on attachment 127970 [details] [diff] [review]
Patch for the problem described.

Works, looks good. r=myk

Comment 6

[Dave Miller [:justdave]]

Comment on attachment 127970 [details] [diff] [review]
Patch for the problem described.

Why are we using ThrowCodeError here?

ThrowCodeError implies a fatal situation in which we want the user to email the
sysadmin to bring it to their attention.

This is a situation where the user needs to clarify their text entry, not a
situation that needs to be brought to the admin's attention.

Please change it to use ThrowUserError.  (and that tag will need to be defined
in the user-error template).

Comment 7

[timeless]

Err. there's supposed to be a related bug. why are you requiring me to specify a
product? what if i want the queues for bugs from multiple products?

Comment 8

[Max Kanat-Alexander]

Any action on this?

Comment 9

[Max Kanat-Alexander]

This bug has not been touched by its owner in over six months, even though it is
targeted to 2.20, for which the freeze is 10 days away. Unsetting the target
milestone, on the assumption that nobody is actually working on it or has any
plans to soon.

If you are the owner, and you plan to work on the bug, please give it a real
target milestone. If you are the owner, and you do *not* plan to work on it,
please reassign it to [e-mail] or a .bugs component owner. If you are
*anybody*, and you get this comment, and *you* plan to work on the bug, please
reassign it to yourself if you have the ability.

Comment 10

[timeless]

Attachment: what i wanted

This is more what I had in mind, if it works, can we use it?

Comment 11

[Frédéric Buclin]

Comment on attachment 308514 [details] [diff] [review]
what i wanted

%::FORM no longer exists since Bugzilla 2.20 or 2.22, same for SqlQuote()!!!

Didn't we tell you to test your patches first?

Comment 12

[timeless]

Attachment: whatever

you had plenty of time to fix my bugs

Comment 13

[Frédéric Buclin]

Comment on attachment 308518 [details] [diff] [review]
whatever

>+        my $component = $dbh->quote($cgi->param('component'));

Don't do that. Use placeholders instead.

Comment 14

[timeless]

Attachment: placeholders?

Comment 15

[Frédéric Buclin]

Comment on attachment 308534 [details] [diff] [review]
placeholders?

>+        my $component_ids = $dbh->selectcol_arrayref('SELECT id FROM components ' .
>+                                                     'WHERE name = ?', $cgi->param('component'));

The syntax is incorrect and the component value is still tainted. Look at other SQL queries to see how it works.

Comment 16

[mikedoak]

Do what you need to do; permission granted to remove this and any other bugs.....

Comment 17

[mikedoak]

Permission granted....

Comment 18

[David Lawrence [:dkl]]

Attachment: 199516_1.patch

Attaching to correct bug :)

Comment 19

[Frédéric Buclin]

Comment on attachment 8363798 [details] [diff] [review]
199516_1.patch

>+    my $product;
>     if (defined $cgi->param('product') && $cgi->param('product') ne "") {
>+        $product = Bugzilla::Product->check(scalar $cgi->param('product'));

This code calls $cgi->param('product') 3 times. We waste cycles calling CGI.pm. You should write:

  my $product = $cgi->param('product') // '';
  if ($product ne '') {
      $product = Bugzilla::Product->check($product);



>+    if (defined $cgi->param('component') && $cgi->param('component') ne "") {
>+        my $component_name = scalar $cgi->param('component');

Same here, $cgi->param('component') is called 3 times too. Instead:

  my $component = $cgi->param('component') // '';
  if ($component ne '') {



>+            my @component_objs;
>+            foreach my $product (@{$user->get_selectable_products}) {
>+                foreach my $component (@{$product->components}) {

First of all, you want $user->get_accessible_products, not $user->get_selectable_products. Then it's a huge perf penalty to call each component of each product. This means several thousands of components on some installations. You should rather do a DB query to get the component IDs matching the component name, then call new_from_list(). As all component objects have a ->product method, it's easy to filter accessible components. If you think this code could be useful elsewhere, you could add a new method into Bugzilla::Component, e.g. get_components_by_name() or something like that.



>+                    push(@component_objs, $component) if $component->name eq $component_name;
>+                }
>+            }
>+            @component_ids = map { $_->id } @component_objs;

Simply merge both into a single: push(@component_ids, $component->id) if $component->name eq $component_name; But this code will probably go away with my proposal above.