Bug 26074 - Ability to limit search by number of Comments

Bugzilla/Comment.pm

@@ -56,7 +56,10 @@ use constant UPDATE_COLUMNS => qw(
 
 use constant DB_TABLE => 'longdescs';
 use constant ID_FIELD => 'comment_id';
-use constant LIST_ORDER => 'bug_when';
+# In some rare cases, two comments can have identical timestamps. If
+# this happens, we want to be sure that the comment added later shows up
+# later in the sequence.
+use constant LIST_ORDER => 'bug_when, comment_id';
 
 use constant VALIDATORS => {
     extra_data => \&_check_extra_data,

Bugzilla/Field.pm

@@ -218,6 +218,8 @@ use constant DEFAULT_FIELDS => (
      buglist => 1},
     {name => 'longdesc',              desc => 'Comment'},
     {name => 'longdescs.isprivate',   desc => 'Comment is private'},
+    {name => 'longdescs.count',       desc => 'Number of Comments',
+     buglist => 1},
     {name => 'alias',                 desc => 'Alias', buglist => 1},
     {name => 'everconfirmed',         desc => 'Ever Confirmed'},
     {name => 'reporter_accessible',   desc => 'Reporter Accessible'},

Bugzilla/Search.pm

@@ -272,6 +272,14 @@ use constant OPERATOR_FIELD_OVERRIDE => {
         changedafter  => \&_long_desc_changedbefore_after,
         _default      => \&_long_desc,
     },
+    'longdescs.count' => {
+        changedby     => \&_long_desc_changedby,
+        changedbefore => \&_long_desc_changedbefore_after,
+        changedafter  => \&_long_desc_changedbefore_after,
+        changedfrom   => \&_invalid_combination,
+        changedto     => \&_invalid_combination,
+        _default      => \&_long_descs_count,
+    },
     'longdescs.isprivate' => {
         _default => \&_longdescs_isprivate,
     },
@@ -466,6 +474,10 @@ use constant COLUMN_JOINS => {
             to    => 'id',
         },
     },
+    'longdescs.count' => {
+        table => 'longdescs',
+        join  => 'INNER',
+    },
 };
 
 # This constant defines the columns that can be selected in a query 
@@ -524,6 +536,8 @@ sub COLUMNS {
             . $dbh->sql_string_concat('map_flagtypes.name', 'map_flags.status')),
 
         'keywords' => $dbh->sql_group_concat('DISTINCT map_keyworddefs.name'),
+        
+        'longdescs.count' => 'COUNT(DISTINCT map_longdescs_count.comment_id)',
     );
 
     # Backward-compatibility for old field names. Goes new_name => old_name.
@@ -620,6 +634,7 @@ use constant GROUP_BY_SKIP => EMPTY_COLUMN, qw(
     bug_id
     flagtypes.name
     keywords
+    longdescs.count
     percentage_complete
 );
 
@@ -2171,7 +2186,7 @@ sub _content_matches {
     COLUMNS->{'relevance'}->{name} = $select_term;
 }
 
-sub _long_desc {
+sub _join_longdescs {
     my ($self, $args) = @_;
     my ($chart_id, $joins) = @$args{qw(chart_id joins)};
     
@@ -2182,22 +2197,37 @@ sub _long_desc {
         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_desc {
+    my ($self, $args) = @_;
+    my $table = $self->_join_longdescs($args);
     $args->{full_field} = "$table.thetext";
 }
 
-sub _longdescs_isprivate {
+sub _long_descs_count {
     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 $table = "longdescs_count_$chart_id";
+    my $extra =  $self->_user->is_insider ? "" : "WHERE isprivate = 0";
     my $join = {
-        table => 'longdescs',
+        table => "(SELECT bug_id, COUNT(*) AS num"
+                 . " FROM longdescs $extra GROUP BY bug_id)",
         as    => $table,
-        extra => $extra,
     };
     push(@$joins, $join);
+    $args->{full_field} = "${table}.num";
+}
+
+sub _longdescs_isprivate {
+    my ($self, $args) = @_;
+    my $table = $self->_join_longdescs($args);
     $args->{full_field} = "$table.isprivate";
 }
 

template/en/default/global/field-descs.none.tmpl

@@ -133,6 +133,7 @@
      "flagtypes.name"          => "Flags",
      "keywords"                => "Keywords",
      "longdesc"                => "Comment",
+     "longdescs.count"         => "Number of Comments",
      "longdescs.isprivate"     => "Comment is private",
      "newcc"                   => "CC",
      "op_sys"                  => "OS",

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

@@ -66,6 +66,7 @@
     "op_sys"            => { maxlength => 4 } , 
     "bug_file_loc"      => { maxlength => 30 } , 
     "target_milestone"  => { title => "TargetM" } , 
+    "longdescs.count"   => { title => "# Comments" },
     "percentage_complete" => { format_value => "%d %%" } , 
   }
 %]

xt/lib/Bugzilla/Test/Search.pm

@@ -667,6 +667,13 @@ sub _create_one_bug {
                       undef, $bug->id);
         }
         
+        # Bug 1 gets three comments, so that longdescs.count matches it
+        # uniquely. The third comment is added in the middle, so that the
+        # last comment contains all of the important data, like work_time.
+        if ($number == 1) {
+            $bug->add_comment("1-comment-" . random(100));
+        }
+        
         my %update_params = %{ $self->_bug_update_values->{$number} };
         my %reverse_map = reverse %{ Bugzilla::Bug->FIELD_MAP };
         foreach my $db_name (keys %reverse_map) {
@@ -712,7 +719,7 @@ sub _create_one_bug {
         $update_params{reporter_accessible} = $number == 1 ? 1 : 0;
         $update_params{cclist_accessible} = $number == 1 ? 1 : 0;
         $update_params{alias} = $update_alias;
-        
+
         $bug->set_all(\%update_params);
         my $flags = $self->bug_create_value($number, 'set_flags')->{b};
         $bug->set_flags([], $flags);

xt/lib/Bugzilla/Test/Search/Constants.pm

@@ -309,6 +309,7 @@ use constant CHANGED_VALUE_BROKEN => (
     estimated_time   => { contains => [1] },
     'flagtypes.name' => { contains => [1] },
     keywords  => { contains => [1] },
+    'longdescs.count' => { search => 1 },
     work_time => { contains => [1] },
     FIELD_TYPE_MULTI_SELECT, { contains => [1] },
 );
@@ -511,6 +512,7 @@ use constant CHANGED_BROKEN_NOT => (
 
 # For changedfrom and changedto.
 use constant CHANGED_FROM_TO_BROKEN_NOT => (
+    'longdescs.count' => { search => 1 },
     "bug_group" => { contains => [1] },
     "cc" => { contains => [1] },
     "cf_multi_select" => { contains => [1] },
@@ -737,6 +739,7 @@ use constant REGEX_OVERRIDE => {
     cclist_accessible        => { value => '^1' },
     reporter_accessible      => { value => '^1' },
     everconfirmed            => { value => '^1' },
+    'longdescs.count'        => { value => '^3' },
     'longdescs.isprivate'    => { value => '^1' },
     creation_ts => { value => '^2037-01-01' },
     delta_ts    => { value => '^2037-01-01' },
@@ -808,6 +811,7 @@ use constant NEGATIVE_MULTI_BOOLEAN_OVERRIDE => (
 
 # For anyexact and anywordssubstr
 use constant ANY_OVERRIDE => (
+    'longdescs.count' => { contains => [1,2,3,4] },
     'work_time' => { value => '1.0,2.0' },
     dependson => { value => '<1>,<3>', contains => [1,3] },
     MULTI_BOOLEAN_OVERRIDE,
@@ -944,6 +948,7 @@ use constant TESTS => {
               'attachments.ispatch'    => { value => 1, contains => [2,3,4] },
               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] },
               everconfirmed            => { value => 1, contains => [2,3,4,5] },
               creation_ts => { value => '2037-01-02', contains => [1,5] },
@@ -967,6 +972,7 @@ use constant TESTS => {
               'attachments.isprivate'  => { value => 0, contains => [2,3,4] },
               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] },
               everconfirmed            => { value => 0, contains => [2,3,4,5] },
               blocked   => { contains => [1,2] },
@@ -991,6 +997,7 @@ use constant TESTS => {
               'attachments.isprivate'  => { value => 0, contains => [1] },
               cclist_accessible        => { value => 0, contains => [1] },
               reporter_accessible      => { value => 0, contains => [1] },
+              'longdescs.count'        => { value => 2, contains => [1] },
               'longdescs.isprivate'    => { value => 0, contains => [1] },
               everconfirmed            => { value => 0, contains => [1] },
               'flagtypes.name'         => { value => 2, contains => [2,3,4] },
@@ -1006,6 +1013,7 @@ use constant TESTS => {
               'attachments.isprivate'  => { value => 1, contains => [1] },
               cclist_accessible        => { value => 1, contains => [1] },
               reporter_accessible      => { value => 1, contains => [1] },
+              'longdescs.count'        => { value => 3, contains => [1] },
               'longdescs.isprivate'    => { value => 1, contains => [1] },
               everconfirmed            => { value => 1, contains => [1] },
               dependson => { value => '<3>', contains => [1,3] },
@@ -1074,6 +1082,7 @@ use constant TESTS => {
           override => {
               MULTI_BOOLEAN_OVERRIDE,
               dependson => { value => '<1> <3>', contains => [1,3] },
+              'longdescs.count' => { contains => [1,2,3,4] },              
               work_time => { value => '1.0 2.0' },
           },
         },
@@ -1109,6 +1118,7 @@ use constant TESTS => {
               blocked   => { contains => [1,2] },
               dependson => { contains => [1,3] },
               longdesc => { contains => [1,5] },
+              'longdescs.count' => { contains => [1,5] },
           }
         },
     ],
@@ -1195,6 +1205,15 @@ use constant INJECTION_BROKEN_FIELD => {
     FIELD_TYPE_BUG_ID,          { db_skip => ['Pg'] },
     FIELD_TYPE_DATETIME,        { db_skip => ['Pg'] },
     owner_idle_time => { search => 1 },
+    'longdescs.count' => {
+        search => 1,
+        db_skip => ['Pg'],
+        operator_ok => [qw(allwordssubstr anywordssubstr casesubstring
+                           changedbefore changedafter greaterthan greaterthaneq
+                           lessthan lessthaneq notregexp notsubstring
+                           nowordssubstr regexp substring anywords
+                           notequals nowords)],
+    },
     keywords => {
         search => 1,
         operator_ok => [qw(allwordssubstr anywordssubstr casesubstring

xt/lib/Bugzilla/Test/Search/FieldTest.pm

@@ -332,6 +332,9 @@ sub _field_values_for_bug {
         # searches use the last comment.
         @values = reverse @values;
     }
+    elsif ($field eq 'longdescs.count') {
+        @values = scalar(@{ $self->bug($number)->comments });
+    }
     elsif ($field eq 'bug_group') {
         @values = $self->_values_for($number, 'groups_in', 'name');
     }
@@ -515,7 +518,7 @@ sub do_tests {
     my $search_broken = $self->search_known_broken;
     
     my $search = $self->_test_search_object_creation();
-    
+
     my $sql;
     TODO: {
         local $TODO = $search_broken if $search_broken;

xt/lib/Bugzilla/Test/Search/InjectionTest.pm

@@ -54,7 +54,11 @@ sub sql_error_ok { return $_[0]->_known_broken->{sql_error} }
 # Injection tests only skip fields on certain dbs.
 sub field_not_yet_implemented {
     my ($self) = @_;
-    my $skip_for_dbs = $self->_known_broken->{db_skip};
+    # We use the constant directly because we don't want operator_ok
+    # or field_ok to stop us.
+    my $broken = INJECTION_BROKEN_FIELD->{$self->field}
+                 || INJECTION_BROKEN_FIELD->{$self->field_object->type};
+    my $skip_for_dbs = $broken->{db_skip};
     return undef if !$skip_for_dbs;
     my $dbh = Bugzilla->dbh;
     if (my ($skip) = grep { $dbh->isa("Bugzilla::DB::$_") } @$skip_for_dbs) {