311422 - describecomponents.cgi and enter_bug.cgi need some cleanup

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Frédéric Buclin]

Product objects now exist, see Product.pm. This allow us to do some cleanup in
describecomponents.cgi.

Comment 1

[Frédéric Buclin]

Adding enter_bug.cgi too, which has to be fixed at the same time, due to choose-products.html.tmpl which is used by both scripts.

Comment 2

[Frédéric Buclin]

Attachment: patch, v1

This is a huge cleanup. :)

Comment 3

[Frédéric Buclin]

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

maybe wicked will be faster than mkanat... :-/

Comment 4

[Teemu Mannermaa (:wicked)]

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

r=wicked for tip, although I would like to see atleast non-nits below fixed before checkin.

>Index: describecomponents.cgi
>===================================================================

Nit: Uhh, this script has horribly broken validity checking.. It's possible to enter an invalid product name and 1) get error about no enterable products, or 2) even get a component list for a different product without any error message (when there's only one enterable product). Granted, these bugs were there before this rewrite (this makes this a nit only).

>Index: enter_bug.cgi

>@@ -43,10 +43,10 @@
> use Bugzilla::User;
> use Bugzilla::Hook;
> use Bugzilla::Product;
>+use Bugzilla::Classification;
> require "globals.pl";
> 
> use vars qw(
>-  @enterable_products
>   @legal_opsys
>   @legal_platform
>   @legal_priority

Nit: Smallish, easily fixable, bitrot on this hunk.

> my $product = $cgi->param('product');
> 
>-if (!defined $product || $product eq "") {

Nit: Adding a trim would be nice.

>Index: template/en/default/filterexceptions.pl
>===================================================================

Don't any changes to this file need a second review anyway?

>Index: template/en/default/global/choose-product.html.tmpl
>===================================================================
> [%# INTERFACE:
>+  # products: array of product objects. The list of product
>+  #           the user can enter bugs into.

"..list of products the.."

>+  # target:   the script which calls this template.

"the script that displays this template."

Also, please add descriptions for the cloned_bug_id and format params.

>Index: template/en/default/reports/components.html.tmpl
>===================================================================
>+  # product: object. The product for which we want to display component descriptions.

Nit: I somehow hate using "we" in comments.. So maybe "The product for which the component descriptions will be displayed for." or something like that.

>-[% PROCESS global/header.html.tmpl 
>-  title = "Components for $product"
>-  h2 = filtered_product %]
>+
>+[% title = BLOCK %]
>+  Components for [% product.name FILTER html %]
>+[% END %]
>+
>+[% PROCESS global/header.html.tmpl title = title %]

Nit: Would be better to not use a separate title block but rather format the lines like they were. Also, product description would be nice h2.

>+[% IF product.components.size == 0 %]
>+  <p>This product has no components.</p>

Nit: This IF is pretty much useless right now because componentless products are not considered to be enterable. This means product list to choose from is shown instead of this template been processed.

>+  <p>
>   <table>

Huh? A p can't contain a table so this results in "trimming empty p" error. Please, don't add the p tag here.

Comment 5

[Frédéric Buclin]

Attachment: patch, v2

several comments addressed.

Comment 6

[Frédéric Buclin]

Checking in describecomponents.cgi;
/cvsroot/mozilla/webtools/bugzilla/describecomponents.cgi,v  <--  describecomponents.cgi
new revision: 1.35; previous revision: 1.34
done
Checking in enter_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v  <--  enter_bug.cgi
new revision: 1.131; previous revision: 1.130
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.67; previous revision: 1.66
done
Checking in template/en/default/global/choose-product.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/choose-product.html.tmpl,v  <--  choose-product.html.tmpl
new revision: 1.14; previous revision: 1.13
done
Checking in template/en/default/reports/components.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/components.html.tmpl,v  <--  components.html.tmpl
new revision: 1.10; previous revision: 1.9
done