From af38912df5c0f57ad40377119b91dd03d8f9b520 Mon Sep 17 00:00:00 2001 From: Marc Schumann Date: Tue, 16 Oct 2012 20:26:54 +0200 Subject: 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 --- Bugzilla/Install/Filesystem.pm | 61 ++++++++++++++++++++++++ collectstats.pl | 56 ++++++++-------------- reports.cgi | 60 ++++++++++++----------- template/en/default/global/code-error.html.tmpl | 4 +- template/en/default/global/user-error.html.tmpl | 4 -- template/en/default/reports/old-charts.html.tmpl | 4 +- 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 () { 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 [% product FILTER html %] product are not + [% IF product.id %] + Charts for the [% product.name FILTER html %] 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 @@ Product: - [% FOREACH product = products %] - + [% END %] -- cgit v1.2.1