Bug 647649: Change the old "Boolean Charts" UI into the new AND/OR

Bugzilla/CGI.pm

@@ -149,9 +149,18 @@ sub clean_search_url {
             $self->delete("${param}_type");
         }
 
-        # Boolean Chart stuff is empty if it's "noop"
-        if ($param =~ /\d-\d-\d/ && defined $self->param($param)
-            && $self->param($param) eq 'noop')
+        # Custom Search stuff is empty if it's "noop". We also keep around
+        # the old Boolean Chart syntax for backwards-compatibility.
+        if (($param =~ /\d-\d-\d/ || $param =~ /^[[:alpha:]]\d+$/)
+            && defined $self->param($param) && $self->param($param) eq 'noop')
+        {
+            $self->delete($param);
+        }
+        
+        # Any "join" for custom search that's an AND can be removed, because
+        # that's the default.
+        if (($param =~ /^j\d+$/ || $param eq 'j_top')
+            && $self->param($param) eq 'AND')
         {
             $self->delete($param);
         }

buglist.cgi

@@ -68,19 +68,6 @@ if (length($buffer) == 0) {
     ThrowUserError("buglist_parameters_required");
 }
 
-# If a parameter starts with cmd-, this means the And or Or button has been
-# pressed in the advanced search page with JS turned off.
-if (grep { $_ =~ /^cmd\-/ } $cgi->param()) {
-    my $url = "query.cgi?$buffer#chart";
-    print $cgi->redirect(-location => $url);
-    # Generate and return the UI (HTML page) from the appropriate template.
-    $vars->{'message'} = "buglist_adding_field";
-    $vars->{'url'} = $url;
-    $template->process("global/message.html.tmpl", $vars)
-      || ThrowTemplateError($template->error());
-    exit;
-}
-
 $cgi->redirect_search_url();
 
 # Determine whether this is a quicksearch query.

js/custom-search.js

+/* The contents of this file are subject to the Mozilla Public
+ * License Version 1.1 (the "License"); you may not use this file
+ * except in compliance with the License. You may obtain a copy of
+ * the License at http://www.mozilla.org/MPL/
+ *
+ * Software distributed under the License is distributed on an "AS
+ * IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or
+ * implied. See the License for the specific language governing
+ * rights and limitations under the License.
+ *
+ * The Original Code is the Bugzilla Bug Tracking System.
+ *
+ * The Initial Developer of the Original Code is BugzillaSource, Inc.
+ * Portions created by the Initial Developer are Copyright (C) 2011 
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s): 
+ *   Max Kanat-Alexander <mkanat@bugzilla.org>
+ */
+
+var PAREN_INDENT_EM = 2;
+
+function custom_search_new_row() {
+    var row = document.getElementById('custom_search_last_row');
+    var clone = row.cloneNode(true);
+    
+    _cs_fix_ids(clone);
+   
+    // We only want one copy of the buttons, in the new row. So the old
+    // ones get deleted.
+    var op_button = document.getElementById('op_button');
+    row.removeChild(op_button);
+    var cp_button = document.getElementById('cp_container');
+    row.removeChild(cp_button);
+    var add_button = document.getElementById('add_button');
+    row.removeChild(add_button);
+    _remove_any_all(clone);
+
+    // Always make sure there's only one row with this id.
+    row.id = null;
+    row.parentNode.appendChild(clone);
+    return clone;
+}
+
+function custom_search_open_paren() {
+    var row = document.getElementById('custom_search_last_row');
+
+    // If there's an "Any/All" select in this row, it needs to stay as
+    // part of the parent paren set.
+    var any_all = _remove_any_all(row);
+    if (any_all) {
+        var any_all_row = row.cloneNode(false);
+        any_all_row.id = null;
+        any_all_row.appendChild(any_all);
+        row.parentNode.insertBefore(any_all_row, row);
+    }
+
+    // We also need a "Not" checkbox to stay in the parent paren set.
+    var new_not = YAHOO.util.Dom.getElementsByClassName(
+        'custom_search_not_container', null, row);
+    var not_for_paren = new_not[0].cloneNode(true);
+
+    // Preserve the values when modifying the row.
+    var id = _cs_fix_ids(row, true);
+    var prev_id = id - 1;
+
+    var paren_row = row.cloneNode(false);
+    paren_row.id = null;
+    paren_row.innerHTML = '(<input type="hidden" name="f' + prev_id
+                        + '" value="OP">';
+    paren_row.insertBefore(not_for_paren, paren_row.firstChild);
+    row.parentNode.insertBefore(paren_row, row);
+    
+    // New paren set needs a new "Any/All" select.
+    var j_top = document.getElementById('j_top');
+    var any_all_container = j_top.parentNode.cloneNode(true);
+    var any_all = YAHOO.util.Dom.getElementsBy(function() { return true },
+                                               'select', any_all_container);
+    any_all[0].name = 'j' + prev_id;
+    any_all[0].id = any_all[0].name;
+    row.insertBefore(any_all_container, row.firstChild);
+
+    var margin = YAHOO.util.Dom.getStyle(row, 'margin-left');
+    var int_match = margin.match(/\d+/);
+    var new_margin = parseInt(int_match[0]) + PAREN_INDENT_EM;
+    YAHOO.util.Dom.setStyle(row, 'margin-left', new_margin + 'em');
+    YAHOO.util.Dom.removeClass('cp_container', 'bz_default_hidden');
+}
+
+function custom_search_close_paren() {
+    var new_row = custom_search_new_row();
+    
+    // We need to up the new row's id by one more, because we're going
+    // to insert a "CP" before it.
+    var id = _cs_fix_ids(new_row);
+
+    var margin = YAHOO.util.Dom.getStyle(new_row, 'margin-left');
+    var int_match = margin.match(/\d+/);
+    var new_margin = parseInt(int_match[0]) - PAREN_INDENT_EM;
+    YAHOO.util.Dom.setStyle(new_row, 'margin-left', new_margin + 'em');
+
+    var paren_row = new_row.cloneNode(false);
+    paren_row.id = null;
+    paren_row.innerHTML = ')<input type="hidden" name="f' + (id - 1)
+                        + '" value="CP">';
+  
+    new_row.parentNode.insertBefore(paren_row, new_row);
+
+    if (new_margin == 0) {
+        YAHOO.util.Dom.addClass('cp_container', 'bz_default_hidden');
+    }
+}
+
+
+function _cs_fix_ids(parent, preserve_values) {
+    // Update the label of the checkbox.
+    var label = YAHOO.util.Dom.getElementBy(function() { return true },
+                                            'label', parent);
+    var id_match = label.htmlFor.match(/\d+$/);
+    var id = parseInt(id_match[0]) + 1;
+    label.htmlFor = label.htmlFor.replace(/\d+$/, id);
+
+    // Sets all the inputs in the parent back to their default
+    // and fixes their id.
+    var fields =
+        YAHOO.util.Dom.getElementsByClassName('custom_search_form_field', null,
+                                              parent);
+    for (var i = 0; i < fields.length; i++) {
+        var field = fields[i];
+
+        if (!preserve_values) {
+            if (field.type == "checkbox") {
+                field.checked = false;
+            }
+            else {
+                field.value = '';
+            }
+        }
+        
+        // Update the numeric id for the new row.
+        field.name = field.name.replace(/\d+$/, id);
+        field.id = field.name;
+    }
+    
+    return id;
+}
+
+function _remove_any_all(parent) {
+    var any_all = YAHOO.util.Dom.getElementsByClassName('any_all_select', null,
+                                                        parent);
+    if (any_all[0]) {
+        parent.removeChild(any_all[0]);
+        return any_all[0];
+    }
+    return null;
+}

query.cgi

@@ -75,71 +75,36 @@ local our %default;
 # Items which are single-valued, the template should only reference [0]
 # and ignore any multiple values.
 sub PrefillForm {
-    my ($buf) = (@_);
+    my ($buf) = @_;
     my $cgi = Bugzilla->cgi;
     $buf = new Bugzilla::CGI($buf);
     my $foundone = 0;
 
-    # Nothing must be undef, otherwise the template complains.
-    my @list = ("bug_status", "resolution", "assigned_to",
-                      "rep_platform", "priority", "bug_severity",
-                      "classification", "product", "reporter", "op_sys",
-                      "component", "version", "chfield", "chfieldfrom",
-                      "chfieldto", "chfieldvalue", "target_milestone",
-                      "email", "emailtype", "emailreporter",
-                      "emailassigned_to", "emailcc", "emailqa_contact",
-                      "emaillongdesc", "content",
-                      "changedin", "short_desc", "short_desc_type",
-                      "longdesc", "longdesc_type", "bug_file_loc",
-                      "bug_file_loc_type", "status_whiteboard",
-                      "status_whiteboard_type", "bug_id",
-                      "bug_id_type", "keywords", "keywords_type",
-                      "deadlinefrom", "deadlineto",
-                      "x_axis_field", "y_axis_field", "z_axis_field",
-                      "chart_format", "cumulate", "x_labels_vertical",
-                      "category", "subcategory", "name", "newcategory",
-                      "newsubcategory", "public", "frequency");
-    # These fields can also have default values. And because there are
-    # hooks in the advanced search page which let you add fields as
-    # discrete forms, we also need to retain the operators.
-    my @custom_fields = Bugzilla->active_custom_fields;
-    push(@list, map { $_->name } @custom_fields);
-    push(@list, map { $_->name . '_type'} @custom_fields);
-
-    foreach my $name (@list) {
-        $default{$name} = [];
-    }
- 
-    # we won't prefill the boolean chart data from this query if
-    # there are any being submitted via params
-    my $prefillcharts = (grep(/^field-/, $cgi->param)) ? 0 : 1;
- 
+    # Query parameters that don't represent form fields on this page.
+    my @skip = qw(format query_format list_id columnlist);
+
     # Iterate over the URL parameters
     foreach my $name ($buf->param()) {
+        next if grep { $_ eq $name } @skip;
+        $foundone = 1;
         my @values = $buf->param($name);
-
-        # If the name begins with the string 'field', 'type', 'value', or
-        # 'negate', then it is part of the boolean charts. Because
-        # these are built different than the rest of the form, we need
-        # to store these as parameters. We also need to indicate that
-        # we found something so the default query isn't added in if
-        # all we have are boolean chart items.
-        if ($name =~ m/^(?:field|type|value|negate)/) {
-            $cgi->param(-name => $name, -value => $values[0]) if ($prefillcharts);
-            $foundone = 1;
+        
+        # If the name is a single letter followed by numbers, it's part
+        # of Custom Search. We store these as an array of hashes.
+        if ($name =~ /^([[:lower:]])(\d+)$/) {
+            $default{'custom_search'}->[$2]->{$1} = $values[0];
         }
         # If the name ends in a number (which it does for the fields which
         # are part of the email searching), we use the array
         # positions to show the defaults for that number field.
-        elsif ($name =~ m/^(.+)(\d)$/ && defined($default{$1})) {
-            $foundone = 1;
+        elsif ($name =~ /^(\w)(\d)$/) {
             $default{$1}->[$2] = $values[0];
         }
-        elsif (exists $default{$name}) {
-            $foundone = 1;
-            push (@{$default{$name}}, @values);
+        else {
+            push (@{ $default{$name} }, @values);
         }
     }
+
     return $foundone;
 }
 
@@ -153,10 +118,6 @@ if (!PrefillForm($buffer)) {
     }
 }
 
-if (!scalar(@{$default{'chfieldto'}}) || $default{'chfieldto'}->[0] eq "") {
-    $default{'chfieldto'} = ["Now"];
-}
-
 # if using groups for entry, then we don't want people to see products they 
 # don't have access to. Remove them from the list.
 my @selectable_products = sort {lc($a->name) cmp lc($b->name)} 
@@ -240,43 +201,6 @@ if (!Bugzilla->user->is_timetracker) {
 unshift(@fields, { name => "noop", description => "---" });
 $vars->{'fields'} = \@fields;
 
-# Creating new charts - if the cmd-add value is there, we define the field
-# value so the code sees it and creates the chart. It will attempt to select
-# "xyzzy" as the default, and fail. This is the correct behaviour.
-foreach my $cmd (grep(/^cmd-/, $cgi->param)) {
-    if ($cmd =~ /^cmd-add(\d+)-(\d+)-(\d+)$/) {
-        $cgi->param(-name => "field$1-$2-$3", -value => "xyzzy");
-    }
-}
-
-if (!$cgi->param('field0-0-0')) {
-    $cgi->param(-name => 'field0-0-0', -value => "xyzzy");
-}
-
-# Create data structure of boolean chart info. It's an array of arrays of
-# arrays - with the inner arrays having three members - field, type and
-# value.
-my @charts;
-for (my $chart = 0; $cgi->param("field$chart-0-0"); $chart++) {
-    my @rows;
-    for (my $row = 0; $cgi->param("field$chart-$row-0"); $row++) {
-        my @cols;
-        for (my $col = 0; $cgi->param("field$chart-$row-$col"); $col++) {
-            my $value = $cgi->param("value$chart-$row-$col");
-            if (!defined($value)) {
-                $value = '';
-            }
-            push(@cols, { field => $cgi->param("field$chart-$row-$col"),
-                          type => $cgi->param("type$chart-$row-$col") || 'noop',
-                          value => $value });
-        }
-        push(@rows, \@cols);
-    }
-    push(@charts, {'rows' => \@rows, 'negate' => scalar($cgi->param("negate$chart")) });
-}
-
-$default{'charts'} = \@charts;
-
 # Named queries
 if ($userid) {
      $vars->{'namedqueries'} = $dbh->selectcol_arrayref(

template/en/default/filterexceptions.pl

@@ -58,12 +58,7 @@
 ],
 
 'search/boolean-charts.html.tmpl' => [
-  '"field${chartnum}-${rownum}-${colnum}"', 
-  'field.name', 
-  '"${chartnum}-${rownum}-${newor}"', 
-  '"${chartnum}-${newand}-0"', 
-  'newchart',
-  'jsmagic',
+  '"id=\"$id\"" IF id'
 ],
 
 'search/form.html.tmpl' => [

template/en/default/search/boolean-charts.html.tmpl

@@ -48,96 +48,124 @@
   "matches",
   "notmatches",
 ] %]
-<script type="text/javascript">
-  TUI_alternates['custom_search_query'] = '&#9658;';
-  TUI_hide_default('custom_search_query');
-</script>
+
 <div class="bz_section_title" id="custom_search_filter">
   <div id="custom_search_query_controller" class="arrow">&#9660;</div>
   <a id="chart" href="javascript:TUI_toggle_class('custom_search_query')" >
     Custom Search</a> <span class="section_help">Didn't find what 
       you're looking for above? This area allows for ANDs, ORs, 
       and other more complex searches.</span>
-  </div>
-  <div id="custom_search_filter_section" class="bz_search_section custom_search_query" >
-[%# Whoever wrote the original version of boolean charts had a seriously twisted mind %]
+</div>
+<div id="custom_search_filter_section" 
+     class="bz_search_section custom_search_query">
+  [% SET indent_level = 0 %]
+  [% FOREACH condition = default.custom_search %]
+    [% SET cond_num = loop.count - 1 %]
+    [% PROCESS one_condition with_buttons = 0 %]
+  [% END %]
+  [% PROCESS one_condition
+    with_buttons = 1
+    condition = { f => 'noop' }
+    cond_num = cond_num + 1 %]
+  <script type="text/javascript">
+    TUI_alternates['custom_search_query'] = '&#9658;';
+    TUI_hide_default('custom_search_query');
+  </script>
+  <script type="text/javascript" src="js/custom-search.js"></script>
+</div>
 
-[% jsmagic = "onclick=\"this.form.action='query.cgi#chart'; this.form.method='POST'; return 1;\"" %]
 
-[% FOREACH chart = default.charts %]
-  [% chartnum = loop.count - 1 %]
-  <table>
-    <tr>
-      <td>
-        <input type="checkbox" id="negate[% chartnum FILTER html %]"
-             name="negate[% chartnum FILTER html %]" value="1"
-            [%+ "checked" IF chart.negate %]>
-        <label for="negate[% chartnum FILTER html %]">
-          Not (negate this whole chart)
-        </label>
-      </td>
-    </tr>
-  [% FOREACH row = chart.rows %]
-    [% rownum = loop.count - 1 %]
-    <tr>
-    [% FOREACH col = row %]
-      [% colnum = loop.count - 1 %]
-      <td>
-        <select name="[% "field${chartnum}-${rownum}-${colnum}" %]">
-          [% FOREACH field = fields %]
-            <option value="[% field.name %]" [% "selected" IF field.name == col.field %]>
-              [% field_descs.${field.name} || field.description FILTER html %]
-            </option>
-          [% END %]
-        </select>
+[% BLOCK one_condition %]
+  [%# Skip any conditions that don't have a field defined. %]
+  [% RETURN IF !condition.f %]
+  
+  [% IF !top_level_any_shown %]
+    [% INCLUDE any_all_select
+      name = "j_top" id = "j_top" selected = default.j_top.0 %]
+    [% top_level_any_shown = 1 %]
+  [% END %]
 
-        [% INCLUDE "search/type-select.html.tmpl"
-           name = "type${chartnum}-${rownum}-${colnum}",
-           types = types, selected = col.type %]
-        <input name="[% "value${chartnum}-${rownum}-${colnum}" %]" 
-               value="[% col.value FILTER html %]">
-      </td>
-      
-      [% UNLESS loop.last %]
-        <td align="center"> 
-          Or 
-        </td>    
-      </tr>
-      <tr>
-      [% ELSE %]
-        <td>
-          [% newor = colnum + 1 %]
-          <input type="submit" value="Or" [% jsmagic %]
-                 name="cmd-add[% "${chartnum}-${rownum}-${newor}" %]"
-                 id="cmd-add[% "${chartnum}-${rownum}-${newor}" %]">
-        </td>
-      [% END %]
-      
-    [% END %]
-    </tr>
+  [% IF condition.f == "CP" %]
+    [% indent_level = indent_level - 1 %]
+  [% END %]
+
+  <div class="custom_search_condition"
+       [% ' style="margin-left: ' _ (indent_level * 2) _ 'em"' IF indent_level %]
+       [% ' id="custom_search_last_row"' IF with_buttons %]>
     
-    [% UNLESS loop.last %]
-    <tr>
-      <td>And</td>
-    </tr>    
+    [% IF previous_condition.f == "OP" %]
+      [% INCLUDE any_all_select
+        name = "j" _ (cond_num - 1) id = "j" _ (cond_num - 1)
+        selected = previous_condition.j %]
+    [% END %]
+
+    [% IF with_buttons %]
+      <button id="op_button" type="button" 
+              title="Start a new group of criteria, including this row"
+              onclick="custom_search_open_paren()">(</button>
+    [% END %]
+
+    [% UNLESS condition.f == "CP" %]
+      <span class="custom_search_not_container" 
+            title="Search for the opposite of the criteria here">
+        <input type="checkbox" id="n[% cond_num FILTER html %]"
+               class="custom_search_form_field"
+               name="n[% cond_num FILTER html %]" value="1"
+               [% ' checked="checked"' IF condition.n %]>
+        <label for="n[% cond_num FILTER html %]">Not</label>
+      </span>
+    [% END %]
+
+    [% IF condition.f == "OP" %]
+      <input type="hidden" name="f[% cond_num FILTER html %]"
+             id="f[% cond_num FILTER html %]" value="OP">
+      (
+      [% indent_level = indent_level + 1 %]
+    [% ELSIF condition.f == "CP" %]
+      <input type="hidden" name="f[% cond_num FILTER html %]" value="CP">
+      )
     [% ELSE %]
-    <tr>
-      <td>
-        [% newand = rownum + 1; newchart = chartnum + 1 %]
-        <input type="submit" value="And" [% jsmagic %]
-               name="cmd-add[% "${chartnum}-${newand}-0" %]"
-               id="cmd-add[% "${chartnum}-${newand}-0" %]">
-        &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;
-        <input type="submit" value="Add another boolean chart" [% jsmagic %]
-               name="cmd-add[% newchart %]-0-0"
-               id="cmd-add[% newchart %]-0-0">
-        &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;
-      </td>
-    </tr>   
+      <select name="f[% cond_num FILTER html %]" title="Field"
+              id="f[% cond_num FILTER html %]"
+              class="custom_search_form_field">
+        [% FOREACH field = fields %]
+          <option value="[% field.name FILTER html %]"
+                  [%~ ' selected="selected"' IF field.name == condition.f %]>
+            [% field_descs.${field.name} || field.description FILTER html %]
+          </option>
+        [% END %]
+      </select>
+    
+      [% INCLUDE "search/type-select.html.tmpl"
+         name = "o${cond_num}", class = "custom_search_form_field"
+         types = types, selected = condition.o %]
+    
+      <input name="v[% cond_num FILTER html %]" title="Value"
+             class="custom_search_form_field"
+             value="[% condition.v FILTER html %]">
     [% END %]
     
-  [% END %]
-  </table>
-  [% "<hr>" IF NOT loop.last %]
+    [% IF with_buttons %]
+      <button class="custom_search_add_button" type="button"
+              id="add_button" title="Add a new row"
+              onclick="custom_search_new_row()">+</button>
+      <span id="cp_container" [% ' class="bz_default_hidden"' IF !indent_level %]>
+        <button id="cp_button" type="button"
+                title="End this group of criteria"
+                onclick="custom_search_close_paren()">)</button>
+      </span>
+    [% END %]
+  </div>
+  
+  [% previous_condition = condition %]
+[% END %]
+
+[% BLOCK any_all_select %]
+  <div class="any_all_select">
+    <select name="[% name FILTER html %]" [% "id=\"$id\"" IF id %]>
+      <option value="AND">Match ALL of the following:</option>
+      <option value="OR" [% ' selected="selected"' IF selected == "OR" %]>
+        Match ANY of the following:</option>
+    </select>
+  </div>
 [% END %]
-</div>
\ No newline at end of file

template/en/default/search/form.html.tmpl

@@ -395,7 +395,8 @@ TUI_hide_default('information_query');
            and 
            <div id="con_calendar_chfieldfrom"></div>
           <input name="chfieldto" size="10" id="chfieldto" 
-          value="[% default.chfieldto.0 FILTER html %]" onchange="updateCalendarFromField(this)">
+                 value="[% default.chfieldto.0 || "Now" FILTER html %]" 
+                 onchange="updateCalendarFromField(this)">
           <button type="button" class="calendar_button"
                          id="button_calendar_chfieldto"
                          onclick="showCalendar('chfieldto')"><span>Calendar</span></button>

template/en/default/search/search-specific.html.tmpl

@@ -75,7 +75,7 @@ for "crash secure SSL flash".
             [% FOREACH p = user.get_selectable_products(c.id) %]
               [% IF p.components.size %]
                 <option value="[% p.name FILTER html %]"
-                  [% " selected" IF lsearch(default.product, p.name) != -1 %]>
+                  [% " selected" IF default.product.contains(p.name) %]>
                   [% p.name FILTER html %]
                 </option>
               [% END %]
@@ -85,7 +85,7 @@ for "crash secure SSL flash".
         [% ELSE %]
           [% FOREACH p = product %]
             <option value="[% p.name FILTER html %]"
-              [% " selected" IF lsearch(default.product, p.name) != -1 %]>
+              [% " selected" IF default.product.contains(p.name) %]>
               [% p.name FILTER html %]
             </option>
           [% END %]

template/en/default/search/type-select.html.tmpl

@@ -20,7 +20,8 @@
 
 [% PROCESS "global/field-descs.none.tmpl" %]
   
-<select name="[% name FILTER html %]">
+<select name="[% name FILTER html %]" title="Search type"
+        class="[% class FILTER css_class_quote %]">
   [% FOREACH type = types %]
     <option value="[% type FILTER html %]"
             [%- ' selected="selected"' IF type == selected %]>