Bug 311422: describecomponents.cgi and enter_bug.cgi need some cleanup - Patch by Frédéric Buclin <LpSolit@gmail.com> r=wicked a=myk

5 files changed, 103 insertions, 158 deletions

diff --git a/describecomponents.cgi b/describecomponents.cgi
index c00f39bed..87a1eed32 100755
--- a/describecomponents.cgi
+++ b/describecomponents.cgi
@@ -20,6 +20,7 @@
 #
 # Contributor(s): Terry Weissman <terry@mozilla.org>
 #                 Bradley Baetz <bbaetz@student.usyd.edu.au>
+#                 Frédéric Buclin <LpSolit@gmail.com>
 
 use strict;
 use lib qw(.);
@@ -27,83 +28,59 @@ use lib qw(.);
 use Bugzilla;
 use Bugzilla::Constants;
 require "globals.pl";
-
-use vars qw(@legal_product);
+use Bugzilla::Product;
 
 my $user = Bugzilla->login();
 
-GetVersionTable();
-
 my $cgi = Bugzilla->cgi;
 my $dbh = Bugzilla->dbh;
 my $template = Bugzilla->template;
 my $vars = {};
-my $product = trim($cgi->param('product') || '');
-my $product_id = get_product_id($product);
 
-if (!$product_id || !$user->can_enter_product($product)) {
+print $cgi->header();
+
+my $product_name = trim($cgi->param('product') || '');
+my $product = new Bugzilla::Product({'name' => $product_name});
+
+unless ($product && $user->can_enter_product($product->name)) {
     # Products which the user is allowed to see.
-    my @products = @{$user->get_enterable_products()};
+    my @products = @{$user->get_enterable_products};
 
     if (scalar(@products) == 0) {
         ThrowUserError("no_products");
     }
-    elsif (scalar(@products) > 1) {
-        # XXX - For backwards-compatibility with old template
-        # interfaces, we now create a proddesc hash. This can go away
-        # once we update the templates.
-        my %product_desc;
-        foreach my $product (@products) {
-            $product_desc{$product->name} = $product->description;
-        }
-        $vars->{'proddesc'} = \%product_desc;
+    # If there is only one product available but the user entered
+    # another product name, we display a list with this single
+    # product only, to not confuse the user with components of a
+    # product he didn't request.
+    elsif (scalar(@products) > 1 || $product_name) {
+        $vars->{'products'} = \@products;
         $vars->{'target'} = "describecomponents.cgi";
         # If an invalid product name is given, or the user is not
         # allowed to access that product, a message is displayed
         # with a list of the products the user can choose from.
-        if ($product) {
+        if ($product_name) {
             $vars->{'message'} = "product_invalid";
-            $vars->{'product'} = $product;
+            # Do not use $product->name here, else you could use
+            # this way to determine whether the product exists or not.
+            $vars->{'product'} = $product_name;
         }
 
-        print $cgi->header();
         $template->process("global/choose-product.html.tmpl", $vars)
           || ThrowTemplateError($template->error());
         exit;
     }
 
-    # Else, if there is only one product:
-    $product = $products[0]->name;
-    $product_id = $products[0]->id;
+    # If there is only one product available and the user didn't specify
+    # any product name, we show this product.
+    $product = $products[0];
 }
 
 ######################################################################
 # End Data/Security Validation
 ######################################################################
 
-my @components;
-my $comps = $dbh->selectall_arrayref(
-                  q{SELECT name, initialowner, initialqacontact, description
-                      FROM components
-                     WHERE product_id = ?
-                  ORDER BY name}, undef, $product_id);
-foreach my $comp (@$comps) {
-    my ($name, $initialowner, $initialqacontact, $description) = @$comp;
-    my %component;
-
-    $component{'name'} = $name;
-    $component{'initialowner'} = $initialowner ?
-      DBID_to_name($initialowner) : '';
-    $component{'initialqacontact'} = $initialqacontact ?
-      DBID_to_name($initialqacontact) : '';
-    $component{'description'} = $description;
-
-    push @components, \%component;
-}
-
 $vars->{'product'} = $product;
-$vars->{'components'} = \@components;
 
-print $cgi->header();
 $template->process("reports/components.html.tmpl", $vars)
   || ThrowTemplateError($template->error());
diff --git a/enter_bug.cgi b/enter_bug.cgi
index d1d070148..529c4ce73 100755
--- a/enter_bug.cgi
+++ b/enter_bug.cgi
@@ -43,11 +43,11 @@ use Bugzilla::Bug;
 use Bugzilla::User;
 use Bugzilla::Hook;
 use Bugzilla::Product;
+use Bugzilla::Classification;
 use Bugzilla::Keyword;
 require "globals.pl";
 
 use vars qw(
-  @enterable_products
   @legal_opsys
   @legal_platform
   @legal_priority
@@ -66,95 +66,74 @@ my $cgi = Bugzilla->cgi;
 my $template = Bugzilla->template;
 my $vars = {};
 
-my $product = $cgi->param('product');
+my $product = trim($cgi->param('product') || '');
 
-if (!defined $product || $product eq "") {
-    GetVersionTable();
-    Bugzilla->login();
+if ($product eq '') {
+    my $user = Bugzilla->login();
 
-    if ( ! Param('useclassification') ) {
-        # Just use a fake value for the Classification.
-        $cgi->param(-name => 'classification', 
-                    -value => '__all');
-    }
+    # If the user cannot enter bugs in any product, stop here.
+    my @enterable_products = @{$user->get_enterable_products};
+    ThrowUserError('no_products') unless scalar(@enterable_products);
 
-    if (!$cgi->param('classification')) {
-        my $classifications = Bugzilla->user->get_selectable_classifications();
-        foreach my $classification (@$classifications) {
-            my $found = 0;
-            foreach my $p (@enterable_products) {
-               if (Bugzilla->user->can_enter_product($p)
-                   && IsInClassification($classification->{name},$p)) {
-                       $found = 1; 
-               }
-            }
-            if ($found == 0) {
-                @$classifications = grep($_->{name} ne $classification->{name},
-                                         @$classifications);
-            }
+    my $classification = Param('useclassification') ?
+        scalar($cgi->param('classification')) : '__all';
+
+    unless ($classification) {
+        my $class;
+        # Get all classifications with at least one enterable product.
+        foreach $product (@enterable_products) {
+            $class->{$product->classification_id} ||=
+                new Bugzilla::Classification($product->classification_id);
         }
+        my @classifications = sort {lc($a->name) cmp lc($b->name)} (values %$class);
 
-        if (scalar(@$classifications) == 0) {
-            ThrowUserError("no_products");
-        } 
-        elsif (scalar(@$classifications) > 1) {
-            $vars->{'classifications'} = $classifications;
+        # We know there is at least one classification available,
+        # else we would have stopped earlier.
+        if (scalar(@classifications) > 1) {
+            $vars->{'classifications'} = \@classifications;
 
             $vars->{'target'} = "enter_bug.cgi";
             $vars->{'format'} = $cgi->param('format');
-           
             $vars->{'cloned_bug_id'} = $cgi->param('cloned_bug_id');
 
             print $cgi->header();
             $template->process("global/choose-classification.html.tmpl", $vars)
                || ThrowTemplateError($template->error());
-            exit;        
+            exit;
         }
-        $cgi->param(-name => 'classification', -value => @$classifications[0]->name);
+        # If we come here, then there is only one classification available.
+        $classification = $classifications[0]->name;
     }
 
-    my %products;
-    # XXX - This loop should work in some more sensible, efficient way.
-    foreach my $p (@enterable_products) {
-        if (Bugzilla->user->can_enter_product($p)) {
-            if (IsInClassification(scalar $cgi->param('classification'),$p) ||
-                $cgi->param('classification') eq "__all") {
-                my $product_object = new Bugzilla::Product({name => $p});
-                $products{$p} = $product_object->description;
-            }
-        }
-    }
- 
-    my $prodsize = scalar(keys %products);
-    if ($prodsize == 0) {
-        ThrowUserError("no_products");
-    } 
-    elsif ($prodsize > 1) {
-        my %classifications;
-        if ( ! Param('useclassification') ) {
-            @{$classifications{"all"}} = keys %products;
+    # Keep only enterable products which are in the specified classification.
+    if ($classification ne "__all") {
+        my $class = new Bugzilla::Classification({'name' => $classification});
+        # If the classification doesn't exist, then there is no product in it.
+        if ($class) {
+            @enterable_products
+              = grep {$_->classification_id == $class->id} @enterable_products;
         }
-        elsif ($cgi->param('classification') eq "__all") {
-            %classifications = %::classifications;
-        } else {
-            $classifications{$cgi->param('classification')} =
-                $::classifications{$cgi->param('classification')};
+        else {
+            @enterable_products = ();
         }
-        $vars->{'proddesc'} = \%products;
-        $vars->{'classifications'} = \%classifications;
+    }
 
+    if (scalar(@enterable_products) == 0) {
+        ThrowUserError('no_products');
+    }
+    elsif (scalar(@enterable_products) > 1) {
+        $vars->{'products'} = \@enterable_products;
         $vars->{'target'} = "enter_bug.cgi";
         $vars->{'format'} = $cgi->param('format');
-
         $vars->{'cloned_bug_id'} = $cgi->param('cloned_bug_id');
-        
+
         print $cgi->header();
         $template->process("global/choose-product.html.tmpl", $vars)
           || ThrowTemplateError($template->error());
-        exit;        
+        exit;
     } else {
-        # Only one product exists
-        $product = (keys %products)[0];
+        # Only one product exists.
+        $product = $enterable_products[0]->name;
     }
 }
 
diff --git a/template/en/default/filterexceptions.pl b/template/en/default/filterexceptions.pl
index 3dd671b38..b7e43c3c6 100644
--- a/template/en/default/filterexceptions.pl
+++ b/template/en/default/filterexceptions.pl
@@ -96,7 +96,6 @@
 
 'reports/components.html.tmpl' => [
   'numcols',
-  'comp.description', 
 ],
 
 'reports/duplicates-table.html.tmpl' => [
@@ -240,7 +239,6 @@
 
 'global/choose-product.html.tmpl' => [
   'target',
-  'proddesc.$p', 
 ],
 
 # You are not permitted to add any values here. Everything in this file should 
diff --git a/template/en/default/global/choose-product.html.tmpl b/template/en/default/global/choose-product.html.tmpl
index 078b9b700..346a53751 100644
--- a/template/en/default/global/choose-product.html.tmpl
+++ b/template/en/default/global/choose-product.html.tmpl
@@ -20,8 +20,11 @@
   #%]
 
 [%# INTERFACE:
-  # proddesc: hash. May be empty. The hash keys are the products, and the values
-  # are their descriptions.
+  # products: array of product objects. The list of products
+  #           the user can enter bugs into.
+  # target:   the script that displays this template.
+  # cloned_bug_id: ID of the bug being cloned.
+  # format:   the desired format to display the target.
   #%]
 
 [% PROCESS global/variables.none.tmpl %]
@@ -39,18 +42,16 @@
 
 <table>
 
-[% FOREACH p = proddesc.keys.sort %]
+[% FOREACH p = products %]
   <tr>
     <th align="right" valign="top">
-      <a href="[% target %]?product=[% p FILTER url_quote -%]
+      <a href="[% target %]?product=[% p.name FILTER url_quote -%]
             [%- IF cloned_bug_id %]&amp;cloned_bug_id=[% cloned_bug_id FILTER url_quote %][% END -%] 
             [%- IF format %]&amp;format=[% format FILTER url_quote %][% END %]">
-      [% p FILTER html %]</a>:&nbsp;
+      [% p.name FILTER html %]</a>:&nbsp;
     </th>
 
-    [% IF proddesc.$p %]
-      <td valign="top">[% proddesc.$p %]</td>
-    [% END %]
+    <td valign="top">[% p.description FILTER none %]</td>
   </tr>
 [% END %]
 
diff --git a/template/en/default/reports/components.html.tmpl b/template/en/default/reports/components.html.tmpl
index 3578a86e4..3950932bd 100644
--- a/template/en/default/reports/components.html.tmpl
+++ b/template/en/default/reports/components.html.tmpl
@@ -20,18 +20,14 @@
   #%]
 
 [%# INTERFACE:
-  # product: string. The product this is the components list for.
-  # components: List of hashes. May be empty. Each hash has four members:
-  #   name: string. Name of the component.
-  #   description: string. Description of the component. May contain HTML.
-  #   initialowner: string. Component's default assignee.
-  #   initialqacontact: string. Component's default QA contact.
+  # product: object. The product for which we want to display component descriptions.
   #%]
-  
-[% filtered_product = product FILTER html %]
-[% 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 %]
 
 [% IF Param("useqacontact") %]
   [% numcols = 3 %]
@@ -39,28 +35,24 @@
   [% numcols = 2 %]
 [% END %]
 
-[% IF components.size == 0 %]
-  This product has no components.
-[% ELSE %]
-  <table>
-    <tr>
-      <th align="left">Component</th>
-      <th align="left">Default Assignee</th>
-      [% IF Param("useqacontact") %]
-        <th align="left">Default QA Contact</th>
-      [% END %]
-    </tr>
-
-    [% FOREACH comp = components %]
-      [% INCLUDE describe_comp %]
+<table>
+  <tr>
+    <th align="left">Component</th>
+    <th align="left">Default Assignee</th>
+    [% IF Param("useqacontact") %]
+      <th align="left">Default QA Contact</th>
     [% END %]
-    <tr>
-      <td colspan="[% numcols %]">
-        <hr>
-      </td>
-    </tr>
-  </table>
-[% END %]
+  </tr>
+
+  [% FOREACH comp = product.components %]
+    [% INCLUDE describe_comp %]
+  [% END %]
+  <tr>
+    <td colspan="[% numcols %]">
+      <hr>
+    </td>
+  </tr>
+</table>
 
 [% PROCESS global/footer.html.tmpl %]
 
@@ -79,21 +71,19 @@
       <a name="[% comp.name FILTER html %]">[% comp.name FILTER html %]</a>
     </td>
     <td>
-      <a href="mailto:[% comp.initialowner FILTER html %]
-                      [% Param('emailsuffix') %]">
-      [% comp.initialowner FILTER html %]</a>
+      <a href="mailto:[% comp.default_assignee.email FILTER html %]">
+      [% comp.default_assignee.login FILTER html %]</a>
     </td>
     [% IF Param("useqacontact") %]
       <td>
-        <a href="mailto:[% comp.initialqacontact FILTER html %]
-                        [% Param('emailsuffix') %]">
-        [% comp.initialqacontact FILTER html %]</a>
+        <a href="mailto:[% comp.default_qa_contact.email FILTER html %]">
+        [% comp.default_qa_contact.login FILTER html %]</a>
       </td>
     [% END %]
   </tr>
   <tr>
     <td colspan="[% numcols - 1 %]">
-      [% comp.description %]
+      [% comp.description FILTER none %]
     </td>
   </tr>
 [% END %]