286158 - Change all use of GetSelectableProducts() into Bugzilla::User::get_selectable_products()

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement, P2)

Description

[Frédéric Buclin]

We have a get_selectable_products() sub in User.pm *and* a
GetSelectableProducts() sub in globals.pl, doing almost the same thing except
for the classification part. IMO, we should remove the one in globals.pl and
update the one in User.pm.

The function in User.pm has been introduced by bug 186093.

Comment 1

[Frédéric Buclin]

GetSelectableProducts(), GetSelectableProductHash() and
GetSelectableClassifications() should be moved together into User.pm as they are
user-specific. Note that two of these three routines already exist in User.pm.

Comment 2

[Frédéric Buclin]

Attachment: patch, v1

the code inside query.cgi could be optimized by adding a new
$classification->products method. But this also requires
$user->get_selectable_classifications to be rewritten and I prefer to fix this
point in another bug.

Comment 3

[Joel Peshkin]

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


-		      join(', ', (-1, values(%{Bugzilla->user->groups}))) . ")
+		      join(', ', (-1, values(%{$user->groups}))) . ")

You don't need to do this anymore.  You can use $user->groups_as_string.

Aside from that, this looks fine, but I still need to make another pass at it
and test it.

Comment 4

[Joel Peshkin]

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

r=joel, but I'd like a 2xr on this.

Comment 5

[Christian Reis]

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

Beautiful cleanup, wow.

>Index: enter_bug.cgi
>+       my $classifications = Bugzilla->user->get_selectable_classifications();

I wonder if you would be better off with using a real @classifications array
here instead of an arrayref; would avoid the @$ on all but one callsites. Nit,
though, and I don't know if this is standard in our code.

>Index: query.cgi
>+# extract product names
>+my @products = map { $_->name } @selectable_product_objects;

Please use @product_names

>+
>+foreach my $p (@products) {

And $p_name or similar.

>Index: Bugzilla/Product.pm
>-        $self->{components} =
>+        my @components =
>             Bugzilla::Component::get_components_by_product($self->id);

I thought we had used a Product instance as an argument instead of the product
id, how odd.

>+    my $groups_controls = $product->group_controls();
>+    my @milestones      = $product->milestones();
>+    my @versions        = $product->versions();

It's unfortunate that this API is slightly inconsistent, isn't it? How about we
keep it all nice and rounded?

>-    my $hash_ref = Bugzilla::Component::get_components_by_product(1);
>-    my $component = $hash_ref->{1};
>+    my @components = Bugzilla::Component::get_components_by_product($id);

How odd. Was this broken before?

>Index: Bugzilla/User.pm
>+    return $self->{SelectableProducts};

[...]

>     return $self->{selectable_classifications};

Any reason for the apparent inconsistency? Perhaps fixing this would be nice.

>Index: template/en/default/request/queue.html.tmpl

I hope we will be able to factor out the duplication in this code someday.

>-          [% FOREACH item = components %]
>-            <option value="[% item FILTER html %]" [% "selected" IF cgi.param('component') == item %]>
>-              [% item FILTER html %]</option>
>+          [% FOREACH prod = products %]
>+            [% FOREACH comp = prod.components %]

Ah, so there is no longer $components in the template interface? No need,
right?

>+        [% FOREACH product = products %]
>+          [% FOREACH milestone = product.milestones %]

We are using this sort of construct in so many places I ask myself if an API to
simply grab everything for all products is warranted. This is just a remark for
the future, not something that needs addressing here.

No showstoppers here -- fix as you like and land, thanks for the patch.

Comment 6

[Frédéric Buclin]

(In reply to comment #5)
> >Index: query.cgi
> >+# extract product names
> >+my @products = map { $_->name } @selectable_product_objects;
> 
> Please use @product_names

@products is used in many places and I don't want to be too intrusive with my
patch. I keep it as is.


> >+foreach my $p (@products) {
> 
> And $p_name or similar.

Fixed! I renamed it as $prod_name.


> >Index: Bugzilla/User.pm
> >+    return $self->{SelectableProducts};
> 
> [...]
> 
> >     return $self->{selectable_classifications};
> 
> Any reason for the apparent inconsistency? Perhaps fixing this would be nice.

Fixed! I renamed it as $self->{selectable_products}.


> >Index: template/en/default/request/queue.html.tmpl

> Ah, so there is no longer $components in the template interface? No need,
> right?

This file has no INTERFACE. ;)

Comment 7

[Frédéric Buclin]

Attachment: patch, v1.1

The patch I'm going to check in and which fixes the two nits mentioned in my
previous comment.

Comment 8

[Frédéric Buclin]

Checking in config.cgi;
/cvsroot/mozilla/webtools/bugzilla/config.cgi,v  <--  config.cgi
new revision: 1.11; previous revision: 1.10
done
Checking in duplicates.cgi;
/cvsroot/mozilla/webtools/bugzilla/duplicates.cgi,v  <--  duplicates.cgi
new revision: 1.47; previous revision: 1.46
done
Checking in enter_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v  <--  enter_bug.cgi
new revision: 1.119; previous revision: 1.118
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.338; previous revision: 1.337
done
Checking in query.cgi;
/cvsroot/mozilla/webtools/bugzilla/query.cgi,v  <--  query.cgi
new revision: 1.151; previous revision: 1.150
done
Checking in reports.cgi;
/cvsroot/mozilla/webtools/bugzilla/reports.cgi,v  <--  reports.cgi
new revision: 1.79; previous revision: 1.78
done
Checking in request.cgi;
/cvsroot/mozilla/webtools/bugzilla/request.cgi,v  <--  request.cgi
new revision: 1.26; previous revision: 1.25
done
Checking in Bugzilla/Component.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Component.pm,v  <--  Component.pm
new revision: 1.6; previous revision: 1.5
done
Checking in Bugzilla/Product.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Product.pm,v  <--  Product.pm
new revision: 1.9; previous revision: 1.8
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.78; previous revision: 1.77
done
Checking in template/en/default/config.js.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/config.js.tmpl,v  <-- 
config.js.tmpl
new revision: 1.6; previous revision: 1.5
done
Checking in template/en/default/config.rdf.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/config.rdf.tmpl,v  <-- 
config.rdf.tmpl
new revision: 1.6; previous revision: 1.5
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v 
<--  filterexceptions.pl
new revision: 1.52; previous revision: 1.51
done
Checking in template/en/default/global/choose-classification.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/choose-classification.html.tmpl,v
 <--choose-classification.html.tmpl
new revision: 1.5; previous revision: 1.4
done
Checking in template/en/default/reports/duplicates.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/duplicates.html.tmpl,v
 <--  duplicates.html.tmpl
new revision: 1.15; previous revision: 1.14
done
Checking in template/en/default/request/queue.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/request/queue.html.tmpl,v
 <--  queue.html.tmpl
new revision: 1.14; previous revision: 1.13
done