Bug 232749 - fix various charting problems revealed by b.m.o. upgrade, including…

Bugzilla/Series.pm

@@ -47,9 +47,14 @@ sub new {
 
     my $arg_count = scalar(@_);
     
+    # There are three ways of creating Series objects. Two (CGI and Parameters)
+    # are for use when creating a new series. One (Database) is for retrieving
+    # information on existing series.
     if ($arg_count == 1) {
         if (ref($_[0])) {
             # We've been given a CGI object to create a new Series from.
+            # This series may already exist - external code needs to check
+            # before it calls writeToDatabase().
             $self->initFromCGI($_[0]);
         }
         else {
@@ -60,6 +65,8 @@ sub new {
     }
     elsif ($arg_count >= 6 && $arg_count <= 8) {
         # We've been given a load of parameters to create a new Series from.
+        # Currently, undef is always passed as the first parameter; this allows
+        # you to call writeToDatabase() unconditionally. 
         $self->initFromParameters(@_);
     }
     else {
@@ -146,7 +153,6 @@ sub initFromCGI {
 sub writeToDatabase {
     my $self = shift;
 
-    # Lock some tables
     my $dbh = Bugzilla->dbh;
     $dbh->do("LOCK TABLES series_categories WRITE, series WRITE, " .
              "user_series_map WRITE");
@@ -154,8 +160,15 @@ sub writeToDatabase {
     my $category_id = getCategoryID($self->{'category'});
     my $subcategory_id = getCategoryID($self->{'subcategory'});
 
+    my $exists;
+    if ($self->{'series_id'}) { 
+        $exists = 
+            $dbh->selectrow_array("SELECT series_id FROM series
+                                   WHERE series_id = $self->{'series_id'}");
+    }
+    
     # Is this already in the database?                              
-    if ($self->existsInDatabase()) {
+    if ($exists) {
         # Update existing series
         my $dbh = Bugzilla->dbh;
         $dbh->do("UPDATE series SET " .
@@ -196,7 +209,7 @@ sub writeToDatabase {
 }
 
 # Check whether a series with this name, category and subcategory exists in
-# the DB and, if so, sets series_id to its series_id.
+# the DB and, if so, returns its series_id.
 sub existsInDatabase {
     my $self = shift;
     my $dbh = Bugzilla->dbh;
@@ -205,12 +218,12 @@ sub existsInDatabase {
     my $subcategory_id = getCategoryID($self->{'subcategory'});
     
     trick_taint($self->{'name'});
-    $self->{'series_id'} = $dbh->selectrow_array("SELECT series_id " .
+    my $series_id = $dbh->selectrow_array("SELECT series_id " .
                               "FROM series WHERE category = $category_id " .
                               "AND subcategory = $subcategory_id AND name = " .
                               $dbh->quote($self->{'name'}));
                               
-    return(defined($self->{'series_id'}));
+    return($series_id);
 }
 
 # Get a category or subcategory IDs, creating the category if it doesn't exist.

buglist.cgi

@@ -275,12 +275,14 @@ if ($::FORM{'cmdtype'} eq "dorem") {
     if ($::FORM{'remaction'} eq "run") {
         $::buffer = LookupNamedQuery($::FORM{"namedcmd"});
         $vars->{'searchname'} = $::FORM{'namedcmd'};
+        $vars->{'searchtype'} = "saved";
         $params = new Bugzilla::CGI($::buffer);
         $order = $params->param('order') || $order;
     }
     elsif ($::FORM{'remaction'} eq "runseries") {
         $::buffer = LookupSeries($::FORM{"series_id"});
         $vars->{'searchname'} = $::FORM{'namedcmd'};
+        $vars->{'searchtype'} = "series";
         $params = new Bugzilla::CGI($::buffer);
         $order = $params->param('order') || $order;
     }

chart.cgi

@@ -21,10 +21,12 @@
 # Contributor(s): Gervase Markham <gerv@gerv.net>
 
 # Glossary:
-# series:  An individual, defined set of data plotted over time.
-# line:    A set of one or more series, to be summed and drawn as a single
-#          line when the series is plotted.
-# chart:   A set of lines
+# series:   An individual, defined set of data plotted over time.
+# data set: What a series is called in the UI.
+# line:     A set of one or more series, to be summed and drawn as a single
+#           line when the series is plotted.
+# chart:    A set of lines
+#
 # So when you select rows in the UI, you are selecting one or more lines, not
 # series.
 
@@ -34,7 +36,6 @@
 # Broken image on error or no data - need to do much better.
 # Centralise permission checking, so UserInGroup('editbugs') not scattered
 #   everywhere.
-# Better protection on collectstats.pl for second run in a day
 # User documentation :-)
 #
 # Bonus:
@@ -84,6 +85,9 @@ ConnectToDatabase();
 
 confirm_login();
 
+# Only admins may create public queries
+UserInGroup('admin') || $cgi->delete('public');
+
 # All these actions relate to chart construction.
 if ($action =~ /^(assemble|add|remove|sum|subscribe|unsubscribe)$/) {
     # These two need to be done before the creation of the Chart object, so
@@ -120,6 +124,7 @@ elsif ($action eq "wrap") {
 }
 elsif ($action eq "create") {
     assertCanCreate($cgi);
+    
     my $series = new Bugzilla::Series($cgi);
 
     if (!$series->existsInDatabase()) {
@@ -127,7 +132,7 @@ elsif ($action eq "create") {
         $vars->{'message'} = "series_created";
     }
     else {
-        $vars->{'message'} = "series_already_exists";
+        ThrowUserError("series_already_exists", {'series' => $series});
     }
 
     $vars->{'series'} = $series;
@@ -150,7 +155,20 @@ elsif ($action eq "alter") {
     assertCanEdit($series_id);
 
     my $series = new Bugzilla::Series($cgi);
+
+    # We need to check if there is _another_ series in the database with
+    # our (potentially new) name. So we call existsInDatabase() to see if
+    # the return value is us or some other series we need to avoid stomping
+    # on.
+    my $id_of_series_in_db = $series->existsInDatabase();
+    if (defined($id_of_series_in_db) && 
+        $id_of_series_in_db != $series->{'series_id'}) 
+    {
+        ThrowUserError("series_already_exists", {'series' => $series});
+    }
+    
     $series->writeToDatabase();
+    $vars->{'changes_saved'} = 1;
     
     edit($series);
 }
@@ -193,9 +211,6 @@ sub assertCanCreate {
     
     UserInGroup("editbugs") || ThrowUserError("illegal_series_creation");
 
-    # Only admins may create public queries
-    UserInGroup('admin') || $cgi->delete('public');
-    
     # Check permission for frequency
     my $min_freq = 7;
     if ($cgi->param('frequency') < $min_freq && !UserInGroup("admin")) {
@@ -231,25 +246,7 @@ sub edit {
 
     $vars->{'category'} = Bugzilla::Chart::getVisibleSeries();
     $vars->{'creator'} = new Bugzilla::User($series->{'creator'});
-
-    # If we've got any parameters, use those in preference to the values
-    # read from the database. This is a bit ugly, but I can't see a better
-    # way to make this work in the no-JS situation.
-    if ($cgi->param('category'))
-    {
-        $vars->{'default'} = new Bugzilla::Series($series->{'series_id'},
-          $cgi->param('category')    || $series->{'category'},
-          $cgi->param('subcategory') || $series->{'subcategory'},
-          $cgi->param('name')        || $series->{'name'},
-          $series->{'creator'},
-          $cgi->param('frequency')   || $series->{'frequency'});
-
-        $vars->{'default'}{'public'}
-                                = $cgi->param('public') || $series->{'public'};
-    }
-    else {
-        $vars->{'default'} = $series;
-    }
+    $vars->{'default'} = $series;
 
     print "Content-Type: text/html\n\n";
     $template->process("reports/edit-series.html.tmpl", $vars)

template/en/default/global/messages.html.tmpl

@@ -135,17 +135,6 @@
       <a href="editflagtypes.cgi">Back to flag types.</a>
     </p>
     
-  [% ELSIF message_tag == "series_already_exists" %]
-    [% title = "Series Already Exists" %]
-      A series <em>[% series.category FILTER html %] /
-      [%+ series.subcategory FILTER html %] / 
-      [%+ series.name FILTER html %]</em>
-      already exists. If you want to create this series, you will need to give
-      it a different name.
-      <br><br>
-      Go back or 
-      <a href="query.cgi?format=create-series">create another series</a>.
-    
   [% ELSIF message_tag == "series_created" %]
     [% title = "Series Created" %]
       The series <em>[% series.category FILTER html %] /

template/en/default/global/user-error.html.tmpl

@@ -657,6 +657,13 @@
     [% title = "Access Denied" %]
     You do not have the permissions necessary to run a sanity check.
 
+  [% ELSIF error == "series_already_exists" %]
+    [% title = "Series Already Exists" %]
+      A series named <em>[% series.category FILTER html %] /
+      [%+ series.subcategory FILTER html %] / 
+      [%+ series.name FILTER html %]</em>
+      already exists.
+    
   [% ELSIF error == "sidebar_supports_mozilla_only" %]
     Sorry - sidebar.cgi currently only supports Mozilla based web browsers.
     <a href="http://www.mozilla.org">Upgrade today</a>. :-)

template/en/default/list/list.html.tmpl

@@ -19,6 +19,11 @@
   # Contributor(s): Myk Melez <myk@mozilla.org>
   #%]
 
+[%# INTERFACE:
+  # searchtype: string. Type of search - either "series", "saved" or undef.
+  # ...
+  #%]
+
 [%############################################################################%]
 [%# Template Initialization                                                  #%]
 [%############################################################################%]
@@ -155,7 +160,7 @@
       <a href="query.cgi?[% urlquerypart FILTER html %]">Edit&nbsp;Search</a>
     </td>
       
-    [% IF searchname %]
+    [% IF searchtype == "saved" %]
       <td valign="middle" nowrap="nowrap">
         |
         <a href="buglist.cgi?cmdtype=dorem&amp;remaction=forget&amp;namedcmd=

template/en/default/reports/create-chart.html.tmpl

@@ -190,7 +190,7 @@ function subcatSelected() {
             </td>
 
             <td align="center">
-              [% IF series.creator != 0 %]
+              [% IF NOT series.public %]
                 [% IF series.isSubscribed(user.id) %]
                   <input type="submit" value="Unsubscribe" style="width: 12ex;"
                          name="action-unsubscribe[% series.series_id %]">

template/en/default/reports/edit-series.html.tmpl

@@ -28,12 +28,19 @@
 
 [% PROCESS global/header.html.tmpl %]
 
+[% IF changes_saved %]
+  <p>
+    <font color="red">
+      Series updated.
+    </font>
+  </p>
+[% END %]
+
 <form method="get" action="chart.cgi" name="chartform">
   
-  [% button_name = "Change" %]
-
-  [% PROCESS reports/series.html.tmpl %]
-  <input type="hidden" name="action-alter" value="1">
+  [% PROCESS reports/series.html.tmpl 
+     button_name = "Change Data Set" %]
+  <input type="hidden" name="action" value="alter">
   
   [% IF default.series_id %]
     <input type="hidden" name="series_id" value="[% default.series_id %]">
@@ -50,8 +57,12 @@
   [% END %]
 </p>
 
+<p>Note: it is not yet possible to edit the search associated with this data
+set.
+</p>
+
 <p>
-  <a href="query.cgi?[% default.query FILTER html%]">View 
+  <a href="query.cgi?[% default.query FILTER html %]">View 
     series search parameters</a> |
   <a href="buglist.cgi?cmdtype=dorem&amp;namedcmd=
     [% default.category FILTER url_quote %]-

template/en/default/reports/series.html.tmpl

@@ -86,14 +86,9 @@
       </td>
       <td></td>
       <td>
-        <input type="submit" name="action-create" value="Create Data Set">
+        <input type="submit" name="submit-button" 
+               value="[% button_name FILTER html %]">
       </td>
     </tr>
   </tbody>
 </table>
-
-<script language="JavaScript" type="text/javascript">
-  document.chartform.category[0].selected = true;
-  catSelected();
-  checkNewState();
-</script>

template/en/default/search/search-create-series.html.tmpl

@@ -34,7 +34,7 @@
 
 <p style="color: red">
   Note: there is currently a restriction that data sets will only count public
-  [% terms.bugs %] (those not in any group).
+  [%+ terms.bugs %] (those not in any group).
 </p>
 
 <form method="get" action="chart.cgi" name="chartform">
@@ -43,12 +43,20 @@
 
 <p>
   <input type="submit" name="action-search" value="Run Search">
-  to see which [% terms.bugs %] would be included in this series.
+  to see which [% terms.bugs %] would be included in this data set.
 </p>
     
 <h3>Data Set Parameters</h3>
       
-[% INCLUDE reports/series.html.tmpl %]
+[% PROCESS reports/series.html.tmpl 
+   button_name = "Create Data Set" %]
+  <input type="hidden" name="action" value="create">
+
+<script language="JavaScript" type="text/javascript">
+  document.chartform.category[0].selected = true;
+  catSelected();
+  checkNewState();
+</script>
       
 <hr>