Bug 240398: Make flagtypes.name work properly with all the boolean chart

operators.
r=mkanat, a=mkanat (module owner)

3 files changed, 11 insertions, 140 deletions

diff --git a/Bugzilla/Search.pm b/Bugzilla/Search.pm
index 57b8fe748..24abc4379 100644
--- a/Bugzilla/Search.pm
+++ b/Bugzilla/Search.pm
@@ -288,11 +288,9 @@ use constant OPERATOR_FIELD_OVERRIDE => {
     days_elapsed => {
         _default => \&_days_elapsed,
     },
-    dependson => MULTI_SELECT_OVERRIDE,
-    keywords  => MULTI_SELECT_OVERRIDE,
-    'flagtypes.name' => {
-        _default => \&_flagtypes_name,
-    },    
+    dependson        => MULTI_SELECT_OVERRIDE,
+    keywords         => MULTI_SELECT_OVERRIDE,
+    'flagtypes.name' => MULTI_SELECT_OVERRIDE,
     longdesc => {
         %{ MULTI_SELECT_OVERRIDE() },
         changedby     => \&_long_desc_changedby,
@@ -2355,57 +2353,6 @@ sub _join_flag_tables {
     push(@$joins, $attachments_join, $flags_join);
 }
 
-sub _flagtypes_name {
-    my ($self, $args) = @_;
-    my ($chart_id, $operator, $joins, $field, $having) = 
-        @$args{qw(chart_id operator joins field having)};
-    my $dbh = Bugzilla->dbh;
-    
-    # Matches bugs by flag name/status.
-    # Note that--for the purposes of querying--a flag comprises
-    # its name plus its status (i.e. a flag named "review" 
-    # with a status of "+" can be found by searching for "review+").
-    
-    # Don't do anything if this condition is about changes to flags,
-    # as the generic change condition processors can handle those.
-    return if $operator =~ /^changed/;
-    
-    # Add the flags and flagtypes tables to the query.  We do 
-    # a left join here so bugs without any flags still match 
-    # negative conditions (f.e. "flag isn't review+").
-    $self->_join_flag_tables($args);
-    my $flags = "flags_$chart_id";
-    my $flagtypes = "flagtypes_$chart_id";
-    my $flagtypes_join = {
-        table => 'flagtypes',
-        as    => $flagtypes,
-        from  => "$flags.type_id",
-        to    => 'id',
-    };
-    push(@$joins, $flagtypes_join);
-    
-    my $full_field = $dbh->sql_string_concat("$flagtypes.name",
-                                             "$flags.status");
-    $args->{full_field} = $full_field;
-    $self->_do_operator_function($args);
-    my $term = $args->{term};
-    
-    # If this is a negative condition (f.e. flag isn't "review+"),
-    # we only want bugs where all flags match the condition, not 
-    # those where any flag matches, which needs special magic.
-    # Instead of adding the condition to the WHERE clause, we select
-    # the number of flags matching the condition and the total number
-    # of flags on each bug, then compare them in a HAVING clause.
-    # If the numbers are the same, all flags match the condition,
-    # so this bug should be included.
-    if ($operator =~ /^not/) {
-       push(@$having,
-            "SUM(CASE WHEN $full_field IS NOT NULL THEN 1 ELSE 0 END) = " .
-            "SUM(CASE WHEN $term THEN 1 ELSE 0 END)");
-       $args->{term} = '';
-    }
-}
-
 sub _days_elapsed {
     my ($self, $args) = @_;
     my $dbh = Bugzilla->dbh;
@@ -2562,6 +2509,8 @@ sub _multiselect_nonchanged {
 sub _multiselect_table {
     my ($self, $args) = @_;
     my ($field, $chart_id) = @$args{qw(field chart_id)};
+    my $dbh = Bugzilla->dbh;
+    
     if ($field eq 'keywords') {
         $args->{full_field} = 'keyworddefs.name';
         return "keywords INNER JOIN keyworddefs".
@@ -2610,6 +2559,11 @@ sub _multiselect_table {
         return "attachments INNER JOIN attach_data "
                . " ON attachments.attach_id = attach_data.id"
     }
+    elsif ($field eq 'flagtypes.name') {
+        $args->{full_field} = $dbh->sql_string_concat("flagtypes.name",
+                                                      "flags.status");
+        return "flags INNER JOIN flagtypes ON flags.type_id = flagtypes.id";
+    }
     my $table = "bug_$field";
     $args->{full_field} = "bug_$field.value";
     return $table;
diff --git a/xt/lib/Bugzilla/Test/Search/Constants.pm b/xt/lib/Bugzilla/Test/Search/Constants.pm
index 353f9a3bf..deca77a72 100644
--- a/xt/lib/Bugzilla/Test/Search/Constants.pm
+++ b/xt/lib/Bugzilla/Test/Search/Constants.pm
@@ -46,7 +46,6 @@ our @EXPORT = qw(
     NUM_BUGS
     NUM_SEARCH_TESTS
     OR_BROKEN
-    OR_SKIP
     SKIP_FIELDS
     SPECIAL_PARAM_TESTS
     SUBSTR_NO_FIELD_ADD
@@ -134,13 +133,6 @@ use constant SKIP_FIELDS => qw(
     days_elapsed
 );
 
-# During OR tests, we skip these fields. They basically just don't work
-# right in OR tests, and it's too much work to document the exact tests
-# that they cause to fail.
-use constant OR_SKIP => qw(
-    flagtypes.name
-);
-
 # All the fields that represent users.
 use constant USER_FIELDS => qw(
     assigned_to
@@ -212,13 +204,6 @@ use constant ALLWORDS_BROKEN => (
     cc        => { contains => [1] },
 );
 
-# nowords and nowordssubstr have these broken tests in common.
-#
-# flagtypes.name doesn't match bugs without flags.
-use constant NOWORDS_BROKEN => (
-    'flagtypes.name' => { contains => [5] },
-);
-
 # Fields that don't generally work at all with changed* searches, but
 # probably should.
 use constant CHANGED_BROKEN => (
@@ -272,16 +257,10 @@ use constant KNOWN_BROKEN => {
     greaterthaneq => { GREATERTHAN_BROKEN },
 
     'allwordssubstr-<1>' => { ALLWORDS_BROKEN },
-    # flagtypes.name does not work here, probably because they all try to
-    # match against a single flag.
     'allwords-<1>' => {
         ALLWORDS_BROKEN,
-        'flagtypes.name' => { contains => [1] },
     },
 
-    nowordssubstr => { NOWORDS_BROKEN },
-    nowords => { NOWORDS_BROKEN },
-
     # setters.login_name and requestees.login name aren't tracked individually
     # in bugs_activity, so can't be searched using this method.
     #
@@ -357,12 +336,6 @@ use constant KNOWN_BROKEN => {
 # Broken NotTests #
 ###################
 
-# These are fields that are broken in the same way for pretty much every
-# NOT test that is broken.
-use constant COMMON_BROKEN_NOT => (
-    "flagtypes.name"          => { contains => [5] },
-);
-
 # Common BROKEN_NOT values for the changed* fields.
 use constant CHANGED_BROKEN_NOT => (
     "attach_data.thedata"   => { contains => [1] },
@@ -385,42 +358,20 @@ use constant CHANGED_FROM_TO_BROKEN_NOT => (
     FIELD_TYPE_MULTI_SELECT, { contains => [1] },
 );
 
-# Common broken tests for the "not" or "no" operators.
-use constant NEGATIVE_BROKEN_NOT => (
-    "flagtypes.name" => { contains => [1 .. 5] },
-);
-
 # These are field/operator combinations that are broken when run under NOT().
 use constant BROKEN_NOT => {
     allwords       => {
-        COMMON_BROKEN_NOT,
         cc => { contains => [1] },
-        "flagtypes.name"      => { contains => [1,5] },
     },
     'allwords-<1> <2>' => {
         cc => { },
-        'flagtypes.name'      => { contains => [5] },
     },
     allwordssubstr => {
-        COMMON_BROKEN_NOT,
         cc => { contains => [1] },
     },
     'allwordssubstr-<1>,<2>' => {
         cc => { },
     },
-    anyexact => {
-        COMMON_BROKEN_NOT,
-        "flagtypes.name" => { contains => [1, 2, 5] },
-    },
-    anywords => {
-        COMMON_BROKEN_NOT,
-    },
-    anywordssubstr => {
-        COMMON_BROKEN_NOT,
-    },
-    casesubstring => {
-        COMMON_BROKEN_NOT,
-    },
     changedafter => {
         "attach_data.thedata"   => { contains => [2, 3, 4] },
         "classification"        => { contains => [2, 3, 4] },
@@ -454,45 +405,11 @@ use constant BROKEN_NOT => {
         longdesc => { contains => [1] },
         "remaining_time" => { contains => [1] },
     },
-    equals => {
-        COMMON_BROKEN_NOT,
-        "flagtypes.name" => { contains => [1, 5] },
-    },
     greaterthan => {
-        COMMON_BROKEN_NOT,
         cc        => { contains => [1] },
     },
     greaterthaneq => {
-        COMMON_BROKEN_NOT,
         cc               => { contains => [1] },
-        "flagtypes.name" => { contains => [2, 5] },
-    },
-    lessthan => {
-        COMMON_BROKEN_NOT,
-    },
-    lessthaneq => {
-        COMMON_BROKEN_NOT,
-    },
-    notequals      => { NEGATIVE_BROKEN_NOT },
-    notregexp      => { NEGATIVE_BROKEN_NOT },
-    notsubstring   => { NEGATIVE_BROKEN_NOT },
-    nowords        => {
-        NEGATIVE_BROKEN_NOT,
-        "flagtypes.name" => { },
-    },
-    nowordssubstr  => {
-        NEGATIVE_BROKEN_NOT,
-        "flagtypes.name" => { },
-    },
-    regexp         => {
-        COMMON_BROKEN_NOT,
-        "flagtypes.name" => { contains => [1,5] },
-    },
-    'regexp-^1-' => {
-        "flagtypes.name" => { contains => [5] },
-    },
-    substring      => {
-        COMMON_BROKEN_NOT,
     },
 };
 
diff --git a/xt/lib/Bugzilla/Test/Search/OrTest.pm b/xt/lib/Bugzilla/Test/Search/OrTest.pm
index 35653d0f0..42dc30698 100644
--- a/xt/lib/Bugzilla/Test/Search/OrTest.pm
+++ b/xt/lib/Bugzilla/Test/Search/OrTest.pm
@@ -70,7 +70,7 @@ sub debug_value {
 # SKIP & TODO Messages #
 ########################
 
-sub _join_skip { OR_SKIP }
+sub _join_skip { () }
 sub _join_broken_constant { OR_BROKEN }
 
 sub field_not_yet_implemented {