Bug 604388 - Filenames used to store data for Old Charts should be based on product IDs, not names (avoid dataloss when renaming products).

r/a=LpSolit

6 files changed, 118 insertions, 71 deletions

diff --git a/Bugzilla/Install/Filesystem.pm b/Bugzilla/Install/Filesystem.pm
index 6b768cbbb..678e208d4 100644
--- a/Bugzilla/Install/Filesystem.pm
+++ b/Bugzilla/Install/Filesystem.pm
@@ -29,6 +29,7 @@ use File::Find;
 use File::Path;
 use File::Basename;
 use File::Copy qw(move);
+use File::Spec;
 use IO::File;
 use POSIX ();
 
@@ -397,6 +398,13 @@ sub update_filesystem {
         _update_old_charts($datadir);
     }
 
+    # If there is a file named '-All-' in $datadir/mining, then we're still
+    # having mining files named by product name, and we need to convert them to
+    # files named by product ID.
+    if (-e File::Spec->catfile($datadir, 'mining', '-All-')) {
+        _update_old_mining_filenames(File::Spec->catdir($datadir, 'mining'));
+    }
+
     # By sorting the dirs, we assure that shorter-named directories
     # (meaning parent directories) are always created before their
     # child directories.
@@ -638,6 +646,59 @@ sub _update_old_charts {
     } 
 }
 
+# The old naming scheme has product names as mining file names; we rename them
+# to product IDs.
+sub _update_old_mining_filenames {
+    my ($miningdir) = @_;
+    my @conversion_errors;
+
+    require Bugzilla::Product;
+
+    # We use a dummy product instance with ID 0, representing all products
+    my $product_all = {id => 0, name => '-All-'};
+    bless($product_all, 'Bugzilla::Product');
+
+    print "Updating old charting data file names...";
+    my @products = Bugzilla::Product->get_all();
+    push(@products, $product_all);
+    foreach my $product (@products) {
+        if (-e File::Spec->catfile($miningdir, $product->id)) {
+            push(@conversion_errors,
+                 { product => $product,
+                   message => 'A file named "' . $product->id .
+                              '" already exists.' });
+        }
+    }
+
+    if (! @conversion_errors) {
+        # Renaming mining files should work now without a hitch.
+        foreach my $product (@products) {
+            if (! rename(File::Spec->catfile($miningdir, $product->name),
+                         File::Spec->catfile($miningdir, $product->id))) {
+                push(@conversion_errors,
+                     { product => $product,
+                       message => $! });
+            }
+        }
+    }
+
+    # Error reporting
+    if (! @conversion_errors) {
+        print " done.\n";
+    }
+    else {
+        print " FAILED:\n";
+        foreach my $error (@conversion_errors) {
+            printf "Cannot rename charting data file for product %d (%s): %s\n",
+                   $error->{product}->id, $error->{product}->name,
+                   $error->{message};
+        }
+        print "You need to empty the \"$miningdir\" directory, then run\n",
+              "   collectstats.pl --regenerate\n",
+              "in order to clean this up.\n";
+    }
+}
+
 sub fix_dir_permissions {
     my ($dir) = @_;
     return if ON_WINDOWS;
diff --git a/collectstats.pl b/collectstats.pl
index 7336714bb..e1a4603f3 100755
--- a/collectstats.pl
+++ b/collectstats.pl
@@ -38,6 +38,10 @@ $| = 1;
 my $datadir = bz_locations()->{'datadir'};
 my $graphsdir = bz_locations()->{'graphsdir'};
 
+# We use a dummy product instance with ID 0, representing all products
+my $product_all = {id => 0, name => '-All-'};
+bless($product_all, 'Bugzilla::Product');
+
 # Tidy up after graphing module
 my $cwd = Cwd::getcwd();
 if (chdir($graphsdir)) {
@@ -120,7 +124,7 @@ if ($switch{'regenerate'}) {
 my $tstart = time;
 
 my @myproducts = Bugzilla::Product->get_all;
-unshift(@myproducts, "-All-");
+unshift(@myproducts, $product_all);
 
 my $dir = "$datadir/mining";
 if (!-d $dir) {
@@ -149,18 +153,8 @@ sub collect_stats {
     my $product = shift;
     my $when = localtime (time);
     my $dbh = Bugzilla->dbh;
-    my $product_id;
-
-    if (ref $product) {
-        $product_id = $product->id;
-        $product = $product->name;
-    }
 
-    # NB: Need to mangle the product for the filename, but use the real
-    # product name in the query
-    my $file_product = $product;
-    $file_product =~ s/\//-/gs;
-    my $file = join '/', $dir, $file_product;
+    my $file = join '/', $dir, $product->id;
     my $exists = -f $file;
 
     # if the file exists, get the old status and resolution list for that product.
@@ -186,7 +180,7 @@ sub collect_stats {
     my $status_sql = q{SELECT COUNT(*) FROM bugs WHERE bug_status = ?};
     my $reso_sql   = q{SELECT COUNT(*) FROM bugs WHERE resolution = ?};
 
-    if ($product ne '-All-') {
+    if ($product->id) {
         $status_sql .= q{ AND product_id = ?};
         $reso_sql   .= q{ AND product_id = ?};
     }
@@ -197,13 +191,13 @@ sub collect_stats {
     my @values ;
     foreach my $status (@statuses) {
         @values = ($status);
-        push (@values, $product_id) if ($product ne '-All-');
+        push (@values, $product->id) if ($product->id);
         my $count = $dbh->selectrow_array($sth_status, undef, @values);
         push(@row, $count);
     }
     foreach my $resolution (@resolutions) {
         @values = ($resolution);
-        push (@values, $product_id) if ($product ne '-All-');
+        push (@values, $product->id) if ($product->id);
         my $count = $dbh->selectrow_array($sth_reso, undef, @values);
         push(@row, $count);
     }
@@ -216,7 +210,7 @@ sub collect_stats {
 # Do not edit me! This file is generated.
 #
 # fields: $fields
-# Product: $product
+# Product: $product->name
 # Created: $when
 FIN
     }
@@ -285,24 +279,14 @@ sub regenerate_stats {
     my $when = localtime(time());
     my $tstart = time();
 
-    # NB: Need to mangle the product for the filename, but use the real
-    # product name in the query
-    if (ref $product) {
-        $product = $product->name;
-    }
-    my $file_product = $product;
-    $file_product =~ s/\//-/gs;
-    my $file = join '/', $dir, $file_product;
+    my $file = join '/', $dir, $product->id;
 
     my $and_product = "";
-    my $from_product = "";
 
     my @values = ();
-    if ($product ne '-All-') {
-        $and_product = q{ AND products.name = ?};
-        $from_product = q{ INNER JOIN products 
-                          ON bugs.product_id = products.id};
-        push (@values, $product);
+    if ($product->id) {
+        $and_product = q{ AND product_id = ?};
+        push (@values, $product->id);
     }
 
     # Determine the start date from the date the first bug in the
@@ -312,7 +296,7 @@ sub regenerate_stats {
                 $dbh->sql_to_days('creation_ts') . q{ AS start_day, } .
                 $dbh->sql_to_days('current_date') . q{ AS end_day, } . 
                 $dbh->sql_to_days("'1970-01-01'") . 
-                 qq{ FROM bugs $from_product 
+                 qq{ FROM bugs
                    WHERE } . $dbh->sql_to_days('creation_ts') . 
                          qq{ IS NOT NULL $and_product 
                 ORDER BY start_day } . $dbh->sql_limit(1);
@@ -330,7 +314,7 @@ sub regenerate_stats {
 # Do not edit me! This file is generated.
 #
 # fields: $fields
-# Product: $product
+# Product: $product->name
 # Created: $when
 FIN
         # For each day, generate a line of statistics.
@@ -339,12 +323,13 @@ FIN
         for (my $day = $start + 1; $day <= $end; $day++) {
             # Some output feedback
             my $percent_done = ($day - $start - 1) * 100 / $total_days;
-            printf "\rRegenerating $product \[\%.1f\%\%]", $percent_done;
+            printf "\rRegenerating %s \[\%.1f\%\%]", $product->name,
+                                                     $percent_done;
 
             # Get a list of bugs that were created the previous day, and
             # add those bugs to the list of bugs for this product.
             $query = qq{SELECT bug_id 
-                          FROM bugs $from_product 
+                          FROM bugs
                          WHERE bugs.creation_ts < } . 
                          $dbh->sql_from_days($day - 1) . 
                          q{ AND bugs.creation_ts >= } . 
@@ -387,7 +372,8 @@ FIN
 
         # Finish up output feedback for this product.
         my $tend = time;
-        say "\rRegenerating $product \[100.0\%] - " . delta_time($tstart, $tend);
+        say "\rRegenerating " . $product->name . ' [100.0%] - ' .
+            delta_time($tstart, $tend);
 
         close DATA;
     }
diff --git a/reports.cgi b/reports.cgi
index c8c319f41..c931b1586 100755
--- a/reports.cgi
+++ b/reports.cgi
@@ -26,6 +26,10 @@ my $cgi = Bugzilla->cgi;
 my $template = Bugzilla->template;
 my $vars = {};
 
+# We use a dummy product instance with ID 0, representing all products
+my $product_all = {id => 0};
+bless($product_all, 'Bugzilla::Product');
+
 if (!Bugzilla->feature('old_charts')) {
     ThrowCodeError('feature_disabled', { feature => 'old_charts' });
 }
@@ -33,11 +37,11 @@ if (!Bugzilla->feature('old_charts')) {
 my $dir       = bz_locations()->{'datadir'} . "/mining";
 my $graph_dir = bz_locations()->{'graphsdir'};
 my $graph_url = basename($graph_dir);
-my $product_name = $cgi->param('product') || '';
+my $product_id = $cgi->param('product_id');
 
 Bugzilla->switch_to_shadow_db();
 
-if (!$product_name) {
+if (! defined($product_id)) {
     # Can we do bug charts?
     (-d $dir && -d $graph_dir) 
       || ThrowCodeError('chart_dir_nonexistent',
@@ -55,10 +59,10 @@ if (!$product_name) {
         push(@datasets, $datasets);
     }
 
-    # We only want those products that the user has permissions for.
-    my @myproducts = ('-All-');
-    # Extract product names from objects and add them to the list.
-    push( @myproducts, map { $_->name } @{$user->get_selectable_products} );
+    # Start our product list with an entry for all products, then add those
+    # products that the user has permissions for.
+    my @myproducts = ($product_all);
+    push( @myproducts, @{$user->get_selectable_products} );
 
     $vars->{'datasets'} = \@datasets;
     $vars->{'products'} = \@myproducts;
@@ -66,16 +70,21 @@ if (!$product_name) {
     print $cgi->header();
 }
 else {
-    # For security and correctness, validate the value of the "product" form variable.
-    # Valid values are those products for which the user has permissions which appear
-    # in the "product" drop-down menu on the report generation form.
-    my ($product) = grep { $_->name eq $product_name } @{$user->get_selectable_products};
-    ($product || $product_name eq '-All-')
-      || ThrowUserError('invalid_product_name', {product => $product_name});
-
-    # Product names can change over time. Their ID cannot; so use the ID
-    # to generate the filename.
-    my $prod_id = $product ? $product->id : 0;
+    my $product;
+    # For security and correctness, validate the value of the "product_id" form
+    # variable. Valid values are IDs of those products for which the user has
+    # permissions which appear in the "product_id" drop-down menu on the report
+    # generation form. The product_id 0 is a special case, meaning "All
+    # Products".
+    if ($product_id) {
+        $product = Bugzilla::Product->new($product_id);
+        $product && $user->can_see_product($product->name)
+            || ThrowUserError('product_access_denied',
+                              {id => $product_id});
+    }
+    else {
+        $product = $product_all;
+    }
 
     # Make sure there is something to plot.
     my @datasets = $cgi->param('datasets');
@@ -87,9 +96,9 @@ else {
 
     # Filenames must not be guessable as they can point to products
     # you are not allowed to see. Also, different projects can have
-    # the same product names.
+    # the same product IDs.
     my $project = bz_locations()->{'project'} || '';
-    my $image_file =  join(':', ($project, $prod_id, @datasets));
+    my $image_file =  join(':', ($project, $product->id, @datasets));
     my $key = Bugzilla->localconfig->{'site_wide_secret'};
     $image_file = hmac_sha256_base64($image_file, $key) . '.png';
     $image_file =~ s/\+/-/g;
@@ -116,8 +125,8 @@ sub get_data {
     my $dir = shift;
 
     my @datasets;
-    open(DATA, '<', "$dir/-All-")
-      || ThrowCodeError('chart_file_open_fail', {filename => "$dir/-All-"});
+    open(DATA, '<', "$dir/0")
+      || ThrowCodeError('chart_file_open_fail', {filename => "$dir/0"});
 
     while (<DATA>) {
         if (/^# fields?: (.+)\s*$/) {
@@ -131,18 +140,13 @@ sub get_data {
 
 sub generate_chart {
     my ($dir, $image_file, $product, $datasets) = @_;
-    $product = $product ? $product->name : '-All-';
-    my $data_file = $product;
-    $data_file =~ s/\//-/gs;
-    $data_file = $dir . '/' . $data_file;
+    my $data_file = $dir . '/' . $product->id;
 
     if (! open FILE, $data_file) {
-        if ($product eq '-All-') {
-            $product = '';
-        }
         ThrowCodeError('chart_data_not_generated', {'product' => $product});
     }
 
+    my $product_in_title = $product->id ? $product->name : 'All Products';
     my @fields;
     my @labels = qw(DATE);
     my %datasets = map { $_ => 1 } @$datasets;
@@ -205,7 +209,7 @@ sub generate_chart {
 
     my %settings =
         (
-         "title" => "Status Counts for $product",
+         "title" => "Status Counts for $product_in_title",
          "x_label" => "Dates",
          "y_label" => "Bug Counts",
          "legend_labels" => \@labels,
diff --git a/template/en/default/global/code-error.html.tmpl b/template/en/default/global/code-error.html.tmpl
index 9fd326051..8945ba445 100644
--- a/template/en/default/global/code-error.html.tmpl
+++ b/template/en/default/global/code-error.html.tmpl
@@ -55,8 +55,8 @@
     
   [% ELSIF error == "chart_data_not_generated" %]
     [% admindocslinks = {'extraconfig.html' => 'Setting up Charting'} %]
-    [% IF product %]
-      Charts for the <em>[% product FILTER html %]</em> product are not
+    [% IF product.id %]
+      Charts for the <em>[% product.name FILTER html %]</em> product are not
       available yet because no charting data has been collected for it since it
       was created.
     [% ELSE %]
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index 39d07653c..e7d3061a9 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -962,10 +962,6 @@
     [% title = "Invalid Parameter" %]
     The new value for [% name FILTER html %] is invalid: [% err FILTER html %].
 
-  [% ELSIF error == "invalid_product_name" %]
-    [% title = "Invalid Product Name" %]
-    The product name '[% product FILTER html %]' is invalid or does not exist.
-
   [% ELSIF error == "invalid_regexp" %]
     [% title = "Invalid regular expression" %]
     The regular expression you entered is invalid.
diff --git a/template/en/default/reports/old-charts.html.tmpl b/template/en/default/reports/old-charts.html.tmpl
index 12a0cdd83..29098c160 100644
--- a/template/en/default/reports/old-charts.html.tmpl
+++ b/template/en/default/reports/old-charts.html.tmpl
@@ -28,9 +28,9 @@
         <tr>
           <th>Product:</th>
           <td align="center">
-            <select id="product" name="product">
+            <select id="product_id" name="product_id">
               [% FOREACH product = products %]
-                <option value="[% product FILTER html %]">[% product FILTER html %]</option>
+                <option value="[% product.id FILTER html %]">[% product.name || '-All-' FILTER html %]</option>
               [% END %]
             </select>
           </td>