Bug 638489 - Make all boolean charts work with longdescs.isprivate

r=mkanat, a=mkanat (module owner)

3 files changed, 24 insertions, 58 deletions

diff --git a/Bugzilla/Search.pm b/Bugzilla/Search.pm
index 81b459ee5..2bbd4e451 100644
--- a/Bugzilla/Search.pm
+++ b/Bugzilla/Search.pm
@@ -309,9 +309,7 @@ use constant OPERATOR_FIELD_OVERRIDE => {
         changedto     => \&_invalid_combination,
         _default      => \&_long_descs_count,
     },
-    'longdescs.isprivate' => {
-        _default => \&_longdescs_isprivate,
-    },
+    'longdescs.isprivate' => MULTI_SELECT_OVERRIDE,
     owner_idle_time => {
         greaterthan   => \&_owner_idle_time_greater_less,
         greaterthaneq => \&_owner_idle_time_greater_less,
@@ -2269,25 +2267,6 @@ sub _content_matches {
     COLUMNS->{'relevance'}->{name} = $select_term;
 }
 
-sub _join_longdescs {
-    my ($self, $args) = @_;
-    my ($chart_id, $joins) = @$args{qw(chart_id joins)};
-    
-    my $table = "longdescs_$chart_id";
-    my $extra = $self->_user->is_insider ? [] : ["$table.isprivate = 0"];
-    my $join = {
-        table => 'longdescs',
-        as    => $table,
-        extra => $extra,
-    };
-    # We only want to do an INNER JOIN if we're not checking isprivate.
-    # Otherwise we'd exclude all bugs with only private comments from
-    # the search entirely.
-    $join->{join} = 'INNER' if $self->_user->is_insider;
-    push(@$joins, $join);
-    return $table;
-}
-
 sub _long_descs_count {
     my ($self, $args) = @_;
     my ($chart_id, $joins) = @$args{qw(chart_id joins)};
@@ -2302,12 +2281,6 @@ sub _long_descs_count {
     $args->{full_field} = "${table}.num";
 }
 
-sub _longdescs_isprivate {
-    my ($self, $args) = @_;
-    my $table = $self->_join_longdescs($args);
-    $args->{full_field} = "$table.isprivate";
-}
-
 sub _work_time_changedby {
     my ($self, $args) = @_;
     my ($chart_id, $joins, $value) = @$args{qw(chart_id joins value)};
@@ -2588,11 +2561,12 @@ sub _multiselect_multiple {
         push(@terms, $self->_multiselect_term($args));
     }
     
+    # The spacing in the joins helps make the resulting SQL more readable.
     if ($operator =~ /^any/) {
-        $args->{term} = join(" OR ", @terms);
+        $args->{term} = join("\n        OR ", @terms);
     }
     else {
-        $args->{term} = join(" AND ", @terms);
+        $args->{term} = join("\n        AND ", @terms);
     }
 }
 
@@ -2633,6 +2607,14 @@ sub _multiselect_table {
         $args->{full_field} = 'thetext';
         return "longdescs";
     }
+    elsif ($field eq 'longdescs.isprivate') {
+        ThrowUserError('auth_failure', { action => 'search',
+                                         object => 'bug_fields',
+                                         field => 'longdescs.isprivate' })
+            if !$self->_user->is_insider;
+        $args->{full_field} = 'isprivate';
+        return "longdescs";
+    }
     my $table = "bug_$field";
     $args->{full_field} = "bug_$field.value";
     return $table;
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index 47219e3d9..16b66ca84 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -149,6 +149,8 @@
       run
     [% ELSIF action == "schedule" %]
       schedule
+    [% ELSIF action == "search" %]
+      search
     [% ELSIF action == "use" %]
       use
     [% ELSIF action == "approve" %]
@@ -167,6 +169,8 @@
       [% END %]
     [% ELSIF object == "bugs" %]
       [%+ terms.bugs %]
+    [% ELSIF object == "bug_fields" %]
+      the [% field_descs.$field FILTER html %] field
     [% ELSIF object == "charts" %]
       the "New Charts" feature
     [% ELSIF object == "classifications" %]
diff --git a/xt/lib/Bugzilla/Test/Search/Constants.pm b/xt/lib/Bugzilla/Test/Search/Constants.pm
index ed7ff8f74..227203bc7 100644
--- a/xt/lib/Bugzilla/Test/Search/Constants.pm
+++ b/xt/lib/Bugzilla/Test/Search/Constants.pm
@@ -214,7 +214,6 @@ use constant NEGATIVE_BROKEN => (
     'attachments.mimetype'    => { contains => [5] },
     bug_file_loc => { contains => [5] },
     deadline     => { contains => [5] },
-    'longdescs.isprivate'   => { contains => [1] },
     # Custom fields are busted because they can be NULL.
     FIELD_TYPE_FREETEXT, { contains => [5] },
     FIELD_TYPE_BUG_ID,   { contains => [5] },
@@ -248,15 +247,9 @@ use constant ALLWORDS_BROKEN => (
 # nowords and nowordssubstr have these broken tests in common.
 #
 # flagtypes.name doesn't match bugs without flags.
-# longdescs.isprivate actually works properly in
-# terms of excluding bug 1 (since we exclude all values in the search,
-# on our test), but still fails at including bug 5.
-# The longdesc* fields, coincidentally, work completely
-# correctly, possibly because there's only one comment on bug 5.
 use constant NOWORDS_BROKEN => (
     NEGATIVE_BROKEN,
     'flagtypes.name' => { contains => [5] },
-    'longdescs.isprivate' => {},
 );
 
 # Fields that don't generally work at all with changed* searches, but
@@ -311,18 +304,6 @@ use constant KNOWN_BROKEN => {
     notsubstring => { NEGATIVE_BROKEN },
     notregexp    => { NEGATIVE_BROKEN },
 
-    # longdescs.isprivate matches if any comment matches, instead of if all
-    # comments match. (Commenter is probably
-    # also broken in this way, but all our comments come from the same user.) 
-    lessthan   => {
-        'longdescs.isprivate'   => { contains => [1] },
-    },
-    # The lessthaneq tests are broken for the same reasons, but they work
-    # slightly differently so they have a different set of broken tests.
-    lessthaneq => {
-        'longdescs.isprivate' => { contains => [1] },
-    },
-
     greaterthan   => { GREATERTHAN_BROKEN },
     greaterthaneq => { GREATERTHAN_BROKEN },
 
@@ -441,7 +422,6 @@ use constant COMMON_BROKEN_NOT => (
     "bug_file_loc"            => { contains => [5] },
     "deadline"                => { contains => [5] },
     "flagtypes.name"          => { contains => [5] },
-    "longdescs.isprivate"     => { contains => [1] },
     FIELD_TYPE_BUG_ID,           { contains => [5] },
     FIELD_TYPE_DATETIME,         { contains => [5] },
     FIELD_TYPE_FREETEXT,         { contains => [5] },
@@ -486,7 +466,6 @@ use constant BROKEN_NOT => {
         'attach_data.thedata' => { contains => [5] },
         cc => { },
         'flagtypes.name'      => { contains => [5] },
-        'longdescs.isprivate' => { },
     },
     allwordssubstr => {
         COMMON_BROKEN_NOT,
@@ -494,7 +473,6 @@ use constant BROKEN_NOT => {
     },
     'allwordssubstr-<1>,<2>' => {
         cc => { },
-        "longdescs.isprivate" => { },
     },
     anyexact => {
         COMMON_BROKEN_NOT,
@@ -560,11 +538,9 @@ use constant BROKEN_NOT => {
     },
     lessthan => {
         COMMON_BROKEN_NOT,
-        'longdescs.isprivate' => { },
     },
     lessthaneq => {
         COMMON_BROKEN_NOT,
-        'longdescs.isprivate' => { },
     },
     notequals      => { NEGATIVE_BROKEN_NOT },
     notregexp      => { NEGATIVE_BROKEN_NOT },
@@ -828,7 +804,7 @@ use constant TESTS => {
               cclist_accessible        => { value => 1, contains => [2,3,4,5] },
               reporter_accessible      => { value => 1, contains => [2,3,4,5] },
               'longdescs.count'        => { value => 3, contains => [2,3,4,5] },
-              'longdescs.isprivate'    => { value => 1, contains => [2,3,4,5] },
+              'longdescs.isprivate'    => { value => 1, contains => [1,2,3,4,5] },
               everconfirmed            => { value => 1, contains => [2,3,4,5] },
               creation_ts => { value => '2037-01-02', contains => [1,5] },
               delta_ts    => { value => '2037-01-02', contains => [1,5] },
@@ -852,7 +828,7 @@ use constant TESTS => {
               cclist_accessible        => { value => 0, contains => [2,3,4,5] },
               reporter_accessible      => { value => 0, contains => [2,3,4,5] },
               'longdescs.count'        => { value => 2, contains => [2,3,4,5] },
-              'longdescs.isprivate'    => { value => 0, contains => [2,3,4,5] },
+              'longdescs.isprivate'    => { value => -1, contains => [] },
               everconfirmed            => { value => 0, contains => [2,3,4,5] },
               blocked   => { contains => [1,2] },
               dependson => { contains => [1,3] },
@@ -932,7 +908,9 @@ use constant TESTS => {
           override => {
               dependson => { value => '<1-id> <3-id>', contains => [] },
               # bug 3 has the value "21" here, so matches "2,1"
-              percentage_complete => { value => '<2>,<3>', contains => [3] }
+              percentage_complete => { value => '<2>,<3>', contains => [3] },
+              # 1 0 matches bug 1, which has both public and private comments.
+             'longdescs.isprivate' => { contains => [1] },              
           }
         },
     ],
@@ -966,7 +944,9 @@ use constant TESTS => {
           override => { MULTI_BOOLEAN_OVERRIDE } },
         { contains => [], value => '<1> <2>',
           override => {
-            dependson => { contains => [], value => '<2-id> <3-id>' }
+            dependson => { contains => [], value => '<2-id> <3-id>' },
+            # 1 0 matches bug 1, which has both public and private comments.
+            'longdescs.isprivate' => { contains => [1] },
           }
         },
     ],