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

RESOLVED FIXED in Bugzilla 2.22

Status

()

Bugzilla
Bugzilla-General
P2
enhancement
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: Frédéric Buclin, Assigned: Frédéric Buclin)

Tracking

2.19.2
Bugzilla 2.22
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(2 attachments)

(Assignee)

Description

13 years ago
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.

Updated

13 years ago
Blocks: 87411

Updated

13 years ago
Severity: normal → enhancement
Priority: -- → P2
Summary: GetSelectableProducts() is duplicated code of get_selectable_products() → Change all use of GetSelectableProducts() into Bugzilla::User::get_selectable_products()
Target Milestone: --- → Bugzilla 2.22
(Assignee)

Comment 1

12 years ago
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.
Assignee: general → LpSolit
(Assignee)

Comment 2

12 years ago
Created attachment 194450 [details] [diff] [review]
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.
Attachment #194450 - Flags: review?(bugreport)
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED

Comment 3

12 years ago
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

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

r=joel, but I'd like a 2xr on this.
Attachment #194450 - Flags: review?(bugreport)
Attachment #194450 - Flags: review?
Attachment #194450 - Flags: review+

Comment 5

12 years ago
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.
Attachment #194450 - Flags: review? → review+
(Assignee)

Updated

12 years ago
Flags: approval?
Flags: approval? → approval+
(Assignee)

Comment 6

12 years ago
(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. ;)
(Assignee)

Comment 7

12 years ago
Created attachment 194698 [details] [diff] [review]
patch, v1.1

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

Comment 8

12 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Blocks: 304067
You need to log in before you can comment on or make changes to this bug.