340439 - Classifications should be included in config.cgi

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Jeff Pound]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en) AppleWebKit/418 (KHTML, like Gecko) Safari/417.9.3
Build Identifier: 

Currently classification info is not part of the config.cgi output. It would be handy if it was included either as an element of each product (indicating the classification to which it belongs) or, more intuitivly, as a parent node to products, encapsulating the products which belong to that classification.

Reproducible: Always

Steps to Reproduce:
View <bugzilla-install>/config.cgi
Actual Results:  
No classification info is present

Expected Results:  
Classification info included

Comment 1

[Frank Becker]

Attachment: patch V1

Comment 2

[Frédéric Buclin]

Comment on attachment 613607 [details] [diff] [review]
patch V1

>=== modified file 'config.cgi'

>+$vars->{'classifications'} = [map($_->name, Bugzilla::Classification->get_all)];

For security reasons, you must not display classifications for which the user cannot see any product in them. So collect classification names from $vars->{'products'}. See how we did it in enter_bug.cgi, lines 68-70, for an efficient way to do it.

Comment 3

[Frank Becker]

Attachment: patch V2

Comment 4

[Frédéric Buclin]

Comment on attachment 613919 [details] [diff] [review]
patch V2

>=== modified file 'config.cgi'

>+my @enterable_products = @{$user->get_enterable_products};    
>+foreach my $product (@enterable_products) {

As I said in my review, use products being in $vars->{'products'}. So you don't need to call get_enterable_products() at all. So your code should be right after preload() at line 70. Also, only run this code if the 'useclassification' parameter is on.


>+    $class->{$product->classification_id}->{'object'} ||=
>+        new Bugzilla::Classification($product->classification_id);

We don't need all the complexity of enter_bug.cgi as we don't want to store products per classification. Remove the ->{object} key. We don't need it here. Also, you can call $product->classification instead of |new Bugzilla::Classification|. This is cleaner.


>+    # Nice way to group products per classification, without querying
>+    # the DB again.
>+    push(@{$class->{$product->classification_id}->{'products'}}, $product);

This code is not needed here. We won't use it.



>=== modified file 'template/en/default/config.rdf.tmpl'

>+        [% IF Param('useclassification') %]
>+            <bz:classification>[% product.classification.name FILTER html %]</bz:classification>
>+        [% END %]

To avoid too many calls to the DB, you should get the classification object from the $class hashref defined in config.cgi. This way, we wouldn't have to create one classification object per product.

Comment 5

[Frank Becker]

Attachment: patch V3

(In reply to Frédéric Buclin from comment #4)
> Comment on attachment 613919 [details] [diff] [review]
> patch V2
> 
> >=== modified file 'config.cgi'
> 
> >+my @enterable_products = @{$user->get_enterable_products};    
> >+foreach my $product (@enterable_products) {
> 
> As I said in my review, use products being in $vars->{'products'}. So you
> don't need to call get_enterable_products() at all. So your code should be
> right after preload() at line 70. Also, only run this code if the
> 'useclassification' parameter is on.

Sorry I forgot this but now it is included.

> 
> 
> >+    $class->{$product->classification_id}->{'object'} ||=
> >+        new Bugzilla::Classification($product->classification_id);
> 
> We don't need all the complexity of enter_bug.cgi as we don't want to store
> products per classification. Remove the ->{object} key. We don't need it
> here. Also, you can call $product->classification instead of |new
> Bugzilla::Classification|. This is cleaner.
> 
> 
> >+    # Nice way to group products per classification, without querying
> >+    # the DB again.
> >+    push(@{$class->{$product->classification_id}->{'products'}}, $product);
> 
> This code is not needed here. We won't use it.

Removed!
> 
> 
> 
> >=== modified file 'template/en/default/config.rdf.tmpl'
> 
> >+        [% IF Param('useclassification') %]
> >+            <bz:classification>[% product.classification.name FILTER html %]</bz:classification>
> >+        [% END %]
> 
> To avoid too many calls to the DB, you should get the classification object
> from the $class hashref defined in config.cgi. This way, we wouldn't have to
> create one classification object per product.

Now I use the classification_id so we anne not to load the classification.

Comment 6

[Frédéric Buclin]

Comment on attachment 613984 [details] [diff] [review]
patch V3

This looks good. One last thing, though.


>=== modified file 'template/en/default/config.rdf.tmpl'

>+[% IF Param('useclassification') %]

For consistency, move this classification block later in the template, right before the product block.


>+  <bz:classification>
>+    <Seq>
>+      [% FOREACH item = classifications %]
>+      <li>

item should be class or classification, which are the variable names we usually use for classifications. This makes the code easier to read without context.


>+        <bz:classification rdf:about="[% escaped_urlbase %]classification.cgi?id=[% item.id FILTER uri %]">

Now the reason for my r-. Everywhere else, we pass ?name=[% object.name %] except here where you pass ?id=[% object.id %]. This is inconsistent. As I said, you simply have to pass the $class hashref defined in config.cgi to the template, and you get the classification name for free when looping over products (you would have [% class.${product.id}.name %]).


Otherwise looks great! :)

Comment 7

[Frank Becker]

Attachment: patch V4

rework as requested in comment#6

Comment 8

[Frédéric Buclin]

Comment on attachment 614325 [details] [diff] [review]
patch V4

>=== modified file 'config.cgi'

>+    $vars->{'product_classification_name'} = $class;    

$vars->{'class_names'} would be a shorter name.



>=== modified file 'template/en/default/config.rdf.tmpl'

>+  <bz:classification>

I just realized that it must be <bz:classifications>.


>+        <bz:classification rdf:about="[% escaped_urlbase %]classification.cgi?id=
>+            [% classification.id FILTER uri %]">
>+          <bz:id>[% classification.id FILTER html %]</bz:id>

For consistency, we want name= instead of id= in the URL. Also, no need for the internal classification ID; we don't need it.


Everything else looks good. Comments above can be fixed on checkin. Thanks for the patch! :) r=LpSolit

Comment 9

[Frédéric Buclin]

I also had to add a missing FILTER html on checkin.

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified config.cgi
modified template/en/default/config.rdf.tmpl
Committed revision 8186.

Comment 10

[Max Kanat-Alexander]

(In reply to Frédéric Buclin from comment #9)
> I also had to add a missing FILTER html on checkin.

  rdf should be using "FILTER xml" shouldn't it?

Comment 11

[Frédéric Buclin]

(In reply to Max Kanat-Alexander from comment #10)
>   rdf should be using "FILTER xml" shouldn't it?

That's what I thought too (and was going to use it), but FILTER html is used everywhere else in this template, and so I used it too for consistency. FILTER html is used since this template exists, see bug 72837. When this template has been originally written, gerv asked the same question as you, and myk replied that it was fine, see bug 72837 comment 38. But at that time, FILTER xml and FILTER html were doing mostly the same job, so the distinction wasn't very important. Since them, FILTER xml has been improved to exclude some forbidden characters, see bug 105960, so maybe it would now make sense to s/FILTER html/FILTER xml/ everywhere. That's a separate bug, though. :) Feel free to open it. ;)