306601 - Bugzilla::Classification needs a ->products method

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Frédéric Buclin]

While working on bug 286158 and the removal of GetSelectableClassifications()
from globals.pl, it appeared clear that classification objects need a ->products
method which returns all products associated with the specified classification.

This also implies:
- the removal of Bugzilla::Product::classification() which is useless (and will
avoid a module dependency loop);
- the rework of User::get_selectable_classifications();
- some cleanup in query.cgi due to the lack of this method (grep for
get_selectable_classifications).

Comment 1

[André Batosti]

Attachment: batosti_v1

Comment 2

[Max Kanat-Alexander]

Comment on attachment 197197 [details] [diff] [review]
batosti_v1

>Index: Bugzilla/Classification.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Classification.pm,v
>retrieving revision 1.6
>diff -u -p -r1.6 Classification.pm
>--- Bugzilla/Classification.pm	14 Aug 2005 22:34:32 -0000	1.6
>+++ Bugzilla/Classification.pm	23 Sep 2005 19:10:38 -0000
>@@ -92,6 +92,24 @@ sub product_count {
>     return $self->{'product_count'};
> }
> 
>+sub products {
>+    my $self = @_;

  Do my ($self) = @_;
     or
  my $self = shift;

  (For $self, I prefer the second one.)

>+    if (!$self->{products}) {
>+        my $ids = $dbh->selectcol_arrayref(q{
>+            SELECT id FROM products
>+            WHERE classification_id = ? ORDER by name}, undef, $self->id);

  I don't think you need that ORDER BY. I don't think you need to guarantee an
order.

  This needs POD docs. (That's why the r-.)

Comment 3

[André Batosti]

Attachment: batosti_v1_fix

Comment 4

[Max Kanat-Alexander]

Comment on attachment 197234 [details] [diff] [review]
batosti_v1_fix

This patch has a bunch of extra stuff in it.

>+        my $ids = $dbh->selectcol_arrayref(q{
>+            SELECT id FROM products
>+            WHERE classification_id = ? ORDER by name}, undef, $self->id);

  You kept the ORDER BY, which I don't think we should do.

>+ Returns:     Bugzilla::Product object list.

  This should be more specific, and say "A reference to an array of
Bugzilla::Product objects."

Comment 5

[André Batosti]

Attachment: batosti_v1_anotherfix

Comment 6

[Max Kanat-Alexander]

Comment on attachment 197261 [details] [diff] [review]
batosti_v1_anotherfix

OK, I see now why there's a lot of extra code in this, but I wish it wasn't
there. Couldn't we do some of these changes in another patch?

Also, you didn't add "use Bugzilla::Product" to Bugzilla::Classification, so I
don't see how your code will work, with the "new Bugzilla::Product."

Comment 7

[Frédéric Buclin]

(In reply to comment #4)

>   You kept the ORDER BY, which I don't think we should do.

Hum... it depends in which context this method is used. Products should still be
displayed alphabetically. batosti, make sure to check this point before
submitting a new patch (and please test it!).

Comment 8

[André Batosti]

Attachment: batosti_v2

Comment 9

[Frédéric Buclin]

Comment on attachment 197441 [details] [diff] [review]
batosti_v2

>Index: Bugzilla/Classification.pm

>+sub products {

>+        $self->{'products'} = \@products;
>+    }
>+    return $self->{'products'};
>+}

>+    my @products = $classification->products();

Nit: $classification->products returns a reference to an array, not an array
itself. You should then write $products instead of @products.



>Index: Bugzilla/Product.pm

>-sub classification {

>-=item C<get_products_by_classification($class_id)>

You remove the classification method but the POD of
get_products_by_classification() ???
Moreover, you have to remove the corresponding entry from the SYNOPSIS section
too.



>Index: template/en/default/admin/classifications/edit.html.tmpl

>+      <td><textarea rows=4 cols=64 name="description">
>+        [% description = classification.description %]
>+        [% description %]
>+      </textarea></TD>

Why not using 'classification.description' directly??


<input type=hidden name="classificationold" value="[% classification FILTER
html %]">

It should be classification.name.



>Index: template/en/default/admin/classifications/reclassify.html.tmpl

[% main_classification = classification %]

I think you should pass the classification object instead of its name and
description in two separate variables, so that you have now:
[% main_classification = classification.name %]


Also, as I said in my original comment, query.cgi (around line 300) has to be
updated as well in order to take the new $classification->products method into
account.

Comment 10

[André Batosti]

Attachment: batosti_v2.fix

Comment 11

[André Batosti]

Attachment: batosti_v3: query.cgi cleaned up

Comment 12

[André Batosti]

Attachment: batosti_v3.fix

Comment 13

[Frédéric Buclin]

Comment on attachment 198039 [details] [diff] [review]
batosti_v3.fix

>Index: editclassifications.cgi

>if ($action eq 'reclassify') {

>+    my @classifications = 
>+        Bugzilla::Classification::get_all_classifications;
>     $vars->{'selected_products'}   = \@selected_products;
>     $vars->{'unselected_products'} = \@unselected_products;
>+    $vars->{'classifications'} = \@classifications;
>     $vars->{'classification'} = $classification->name;

In templates, you use classification.name, but here classification is not an
object (it should be!), but its name!
Moreover, now that $classification has a 'products' method, you don't need to
care about (un)selected_products here anymore as you can already access them
using $classification->products.
Note that this means editclassifications.cgi doesn't use Bugzilla::Product
anymore.



>Index: template/en/default/admin/classifications/reclassify.html.tmpl

>       <td valign="top">Classification:</td>
>+      <td valign="top" colspan=3>[% classification.name FILTER html %]</td>

Read my comment above. classification is not an object here (but it should
be!).


>       <select name="prodlist" id="prodlist" multiple="multiple" size="20">
>         [% FOREACH product = unselected_products %]
>           <option value="[% product.name FILTER html %]">
>+            [[% PROCESS class_name
>+                id = product.classification_id %]]&nbsp;[% product.name FILTER html %]
>           </option>
>         [% END %]
>       </select></td>

Instead of looping over unselected_products, you should now loop over all
classifications, and if the classification is different from the current
classification, display its products.
Also, instead of selected_products, use classification.products. This way, you
don't need your 'class_name' block anymore



>Index: template/en/default/search/form.html.tmpl
>Index: template/en/default/search/search-specific.html.tmpl

I didn't look at them yet.

Comment 14

[André Batosti]

Attachment: batosti_v4

Comment 15

[Frédéric Buclin]

Comment on attachment 198628 [details] [diff] [review]
batosti_v4

>Index: editclassifications.cgi

'use Bugzilla::Product' is no longer required.

"$vars->{'classification'} = $class_name;" is missing in the 'new' section
(regression from bug 286294).



>Index: query.cgi

> if (Param('useclassification')) {
>     my $class = $user->get_selectable_classifications;
>+    $vars->{'classification'} = $class;
> }

Nit: you could write this as:
$vars->{'classification'} = $user->get_selectable_classifications;



>Index: Bugzilla/Product.pm

'use Bugzilla::Classification' is no longer required.



>Index: template/en/default/admin/classifications/edit.html.tmpl

>+      <td><textarea rows=4 cols=64 name="description">
>+        [% classification.description FILTER none %]
>+      </textarea></td>

</textarea> has to be on the same line as [% classification.description FILTER
none %] to prevent trailing whitespaces.


>-        [% IF products AND products.size > 0 %]
>+        [% IF classification.products AND classification.products.size > 0 %]

Nit: write [% IF classification.products.size > 0 %] directly.

Comment 16

[André Batosti]

Attachment: batosti_v4_fix

Comment 17

[Frédéric Buclin]

Comment on attachment 199209 [details] [diff] [review]
batosti_v4_fix

>Index: Bugzilla/Classification.pm

>+ Returns:     An reference to an array of Bugzilla::Product objects.

Nit: s/An/A/



>Index: template/en/default/admin/classifications/edit.html.tmpl

>       <th align="right">Description:</th>
>+      <td><textarea rows=4 cols=64 name="description">[% classification.description FILTER none %]</textarea></td>

Nit: [% classification.description FILTER none %]</textarea></td> should be on
its own line. This line here is a bit too long.


Both are nits. r=LpSolit

Comment 18

[Frédéric Buclin]

Checking in editclassifications.cgi;
/cvsroot/mozilla/webtools/bugzilla/editclassifications.cgi,v  <-- 
editclassifications.cgi
new revision: 1.18; previous revision: 1.17
done
Checking in query.cgi;
/cvsroot/mozilla/webtools/bugzilla/query.cgi,v  <--  query.cgi
new revision: 1.152; previous revision: 1.151
done
Checking in Bugzilla/Classification.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Classification.pm,v  <-- 
Classification.pm
new revision: 1.7; previous revision: 1.6
done
Checking in Bugzilla/Product.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Product.pm,v  <--  Product.pm
new revision: 1.12; previous revision: 1.11
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.88; previous revision: 1.87
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v 
<--  filterexceptions.pl
new revision: 1.56; previous revision: 1.55
done
Checking in template/en/default/admin/classifications/del.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/classifications/del.html.tmpl,v
 <--  del.html.tmpl
new revision: 1.3; previous revision: 1.2
done
Checking in template/en/default/admin/classifications/delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/classifications/delete.html.tmpl,v
 <--delete.html.tmpl
new revision: 1.2; previous revision: 1.1
done
Checking in template/en/default/admin/classifications/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/classifications/edit.html.tmpl,v
 <--  edit.html.tmpl
new revision: 1.5; previous revision: 1.4
done
Checking in template/en/default/admin/classifications/reclassify.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/classifications/reclassify.html.tmpl,v
 <--  reclassify.html.tmpl
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/default/search/form.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/search/form.html.tmpl,v 
<--  form.html.tmpl
new revision: 1.33; previous revision: 1.32
done
Checking in template/en/default/search/search-specific.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/search/search-specific.html.tmpl,v
 <--  search-specific.html.tmpl
new revision: 1.14; previous revision: 1.13
done