Bug 450403: Add ability to view comments via the web service (Bug.comments)

Patch By Max Kanat-Alexander <mkanat@bugzilla.org> r=dkl, a=mkanat

6 files changed, 278 insertions, 28 deletions

diff --git a/Bugzilla/WebService.pm b/Bugzilla/WebService.pm
index 438a66710..615abf68c 100755
--- a/Bugzilla/WebService.pm
+++ b/Bugzilla/WebService.pm
@@ -294,3 +294,65 @@ an error 302, there won't be an error -302.
 Sometimes a function will throw an error that doesn't have a specific
 error code. In this case, the code will be C<-32000> if it's a "fatal"
 error, and C<32000> if it's a "transient" error.
+
+=head1 COMMON PARAMETERS
+
+Many Webservice methods take similar arguments. Instead of re-writing
+the documentation for each method, we document the parameters here, once,
+and then refer back to this documentation from the individual methods
+where these parameters are used.
+
+=head2 Limiting What Fields Are Returned
+
+Many WebService methods return an array of structs with various
+fields in the structs. (For example, L<Bugzilla::WebService::Bug/get>
+returns a list of C<bugs> that have fields like C<id>, C<summary>, 
+C<creation_time>, etc.)
+
+These parameters allow you to limit what fields are present in
+the structs, to possibly improve performance or save some bandwidth.
+
+=over
+
+=item C<include_fields> (array)
+
+An array of strings, representing the (case-sensitive) names of fields.
+Only the fields specified in this hash will be returned, the rest will
+not be included.
+
+If you specify an empty array, then this function will return empty
+hashes.
+
+Invalid field names are ignored.
+
+Example:
+
+  User.get( ids => [1], include_fields => ['id', 'name'] )
+
+would return something like:
+
+  { users => [{ id => 1, name => 'user@domain.com' }] }
+
+=item C<exclude_fields> (array)
+
+An array of strings, representing the (case-sensitive) names of fields.
+The fields specified will not be included in the returned hashes.
+
+If you specify all the fields, then this function will return empty
+hashes.
+
+Invalid field names are ignored.
+
+Specifying fields here overrides C<include_fields>, so if you specify a
+field in both, it will be excluded, not included.
+
+Example:
+
+  User.get( ids => [1], exclude_fields => ['name'] )
+
+would return something like:
+
+  { users => [{ id => 1, real_name => 'John Smith' }] }
+
+=back
+
diff --git a/Bugzilla/WebService/Bug.pm b/Bugzilla/WebService/Bug.pm
index aaaf753a3..10ea1f886 100755
--- a/Bugzilla/WebService/Bug.pm
+++ b/Bugzilla/WebService/Bug.pm
@@ -27,6 +27,7 @@ use Bugzilla::Constants;
 use Bugzilla::Error;
 use Bugzilla::Field;
 use Bugzilla::WebService::Constants;
+use Bugzilla::WebService::Util qw(filter);
 use Bugzilla::Bug;
 use Bugzilla::BugMail;
 use Bugzilla::Util qw(trim);
@@ -58,6 +59,84 @@ BEGIN { *get_bugs = \&get }
 # Methods #
 ###########
 
+sub comments {
+    my ($self, $params) = @_;
+    if (!(defined $params->{bug_ids} || defined $params->{comment_ids})) {
+        ThrowCodeError('params_required',
+                       { function => 'Bug.comments',
+                         params   => ['bug_ids', 'comment_ids'] });
+    }
+
+    my $bug_ids = $params->{bug_ids} || [];
+    my $comment_ids = $params->{comment_ids} || [];
+
+    my $dbh  = Bugzilla->dbh;
+    my $user = Bugzilla->user;
+
+    my %bugs;
+    foreach my $bug_id (@$bug_ids) {
+        my $bug = Bugzilla::Bug->check($bug_id);
+        # We want the API to always return comments in the same order.
+        my $comments = Bugzilla::Bug::GetComments(
+            $bug->id, 'oldest_to_newest');
+        my @result;
+        foreach my $comment (@$comments) {
+            next if $comment->{isprivate} && !$user->is_insider;
+            $comment->{bug_id} = $bug->id;
+            push(@result, $self->_translate_comment($comment, $params));
+        }
+        $bugs{$bug->id}{'comments'} = \@result;
+    }
+
+    my %comments;
+    if (scalar @$comment_ids) {
+        my @ids = map { trim($_) } @$comment_ids;
+        my @sql_ids = map { $dbh->quote($_) } @ids;
+        my $comment_data = $dbh->selectall_arrayref(
+            'SELECT comment_id AS id, bug_id, who, bug_when AS time,
+                    isprivate, thetext AS body, type, extra_data
+               FROM longdescs WHERE ' . $dbh->sql_in('comment_id', \@sql_ids),
+            {Slice=>{}});
+
+        # See if we were passed any invalid comment ids.
+        my %got_ids = map { $_->{id} => 1 } @$comment_data;
+        foreach my $comment_id (@ids) {
+            if (!$got_ids{$comment_id}) {
+                ThrowUserError('comment_id_invalid', { id => $comment_id });
+            }
+        }
+ 
+        # Now make sure that we can see all the associated bugs.
+        my %got_bug_ids = map { $_->{bug_id} => 1 } @$comment_data;
+        Bugzilla::Bug->check($_) foreach (keys %got_bug_ids);
+
+        foreach my $comment (@$comment_data) {
+            if ($comment->{isprivate} && !$user->is_insider) {
+                ThrowUserError('comment_is_private', { id => $comment->{id} });
+            }
+            $comment->{author} = new Bugzilla::User($comment->{who});
+            $comment->{body} = Bugzilla::Bug::format_comment($comment);
+            $comments{$comment->{id}} =
+                $self->_translate_comment($comment, $params);
+        }
+    }
+
+    return { bugs => \%bugs, comments => \%comments };
+}
+
+# Helper for Bug.comments
+sub _translate_comment {
+    my ($self, $comment, $filters) = @_;
+    return filter $filters, {
+        id         => $self->type('int', $comment->{id}),
+        bug_id     => $self->type('int', $comment->{bug_id}),
+        author     => $self->type('string', $comment->{author}->login),
+        time       => $self->type('dateTime', $comment->{'time'}),
+        is_private => $self->type('boolean', $comment->{isprivate}),
+        text       => $self->type('string', $comment->{body}),
+    };
+}
+
 sub get {
     my ($self, $params) = @_;
     my $ids = $params->{ids};
@@ -326,6 +405,123 @@ You specified a field that doesn't exist or isn't a drop-down field.
 
 =over
 
+
+=item C<comments>
+
+B<UNSTABLE>
+
+=over
+
+=item B<Description>
+
+This allows you to get data about comments, given a list of bugs 
+and/or comment ids.
+
+=item B<Params>
+
+B<Note>: At least one of C<bug_ids> or C<comment_ids> is required.
+
+In addition to the parameters below, this method also accepts the
+standard L<include_fields|Bugzilla::WebService/include_fields> and
+L<exclude_fields|Bugzilla::WebService/exclude_fields> arguments.
+
+=over
+
+=item C<bug_ids> 
+
+C<array> An array that can contain both bug IDs and bug aliases.
+All of the comments (that are visible to you) will be returned for the
+specified bugs.
+
+=item C<comment_ids> 
+
+C<array> An array of integer comment_ids. These comments will be
+returned individually, separate from any other comments in their
+respective bugs.
+
+=back
+
+=item B<Returns>
+
+Two items are returned:
+
+=over
+
+=item C<bugs>
+
+This is used for bugs specified in C<bug_ids>. This is a hash,
+where the keys are the numeric ids of the bugs, and the value is
+a hash with a single key, C<comments>, which is an array of comments.
+(The format of comments is described below.)
+
+Note that any individual bug will only be returned once, so if you
+specify an id multiple times in C<bug_ids>, it will still only be
+returned once.
+
+=item C<comments>
+
+Each individual comment requested in C<comment_ids> is returned here,
+in a hash where the numeric comment id is the key, and the value
+is the comment. (The format of comments is described below.) 
+
+=back
+
+A "comment" as described above is a hash that contains the following
+keys:
+
+=over
+
+=item id
+
+C<int> The globally unique ID for the comment.
+
+=item bug_id
+
+C<int> The ID of the bug that this comment is on.
+
+=item text
+
+C<string> The actual text of the comment.
+
+=item author
+
+C<string> The login name of the comment's author.
+
+=item time
+
+C<dateTime> The time (in Bugzilla's timezone) that the comment was added.
+
+=item is_private
+
+C<boolean> True if this comment is private (only visible to a certain
+group called the "insidergroup"), False otherwise.
+
+=back
+
+=item B<Errors>
+
+This method can throw all the same errors as L</get>. In addition,
+it can also throw the following errors:
+
+=over
+
+=item 110 (Comment Is Private)
+
+You specified the id of a private comment in the C<comment_ids>
+argument, and you are not in the "insider group" that can see
+private comments.
+
+=item 111 (Invalid Comment ID)
+
+You specified an id in the C<comment_ids> argument that is invalid--either
+you specified something that wasn't a number, or there is no comment with
+that id.
+
+=back
+
+=back
+
+
 =item C<get> 
 
 B<EXPERIMENTAL>
diff --git a/Bugzilla/WebService/Constants.pm b/Bugzilla/WebService/Constants.pm
index 593801a6f..07b51e5f1 100755
--- a/Bugzilla/WebService/Constants.pm
+++ b/Bugzilla/WebService/Constants.pm
@@ -51,6 +51,7 @@ use constant WS_ERROR_CODE => {
     # Generic Bugzilla::Object errors are 50-99.
     object_not_specified        => 50,
     param_required              => 50,
+    params_required             => 50,
     object_does_not_exist       => 51,
     # Bug errors usually occupy the 100-200 range.
     improper_bug_id_field_value => 100,
@@ -79,6 +80,9 @@ use constant WS_ERROR_CODE => {
     invalid_field_name => 108,
     # Not authorized to edit the bug
     product_edit_denied => 109,
+    # Comment-related errors
+    comment_is_private => 110,
+    comment_id_invalid => 111,
 
     # Authentication errors are usually 300-400.
     invalid_username_or_password => 300,
diff --git a/Bugzilla/WebService/User.pm b/Bugzilla/WebService/User.pm
index 24b9be709..6283f55a1 100755
--- a/Bugzilla/WebService/User.pm
+++ b/Bugzilla/WebService/User.pm
@@ -451,6 +451,10 @@ B<Note>: At least one of C<ids>, C<names>, or C<match> must be specified.
 B<Note>: Users will not be returned more than once, so even if a user 
 is matched by more than one argument, only one user will be returned.
 
+In addition to the parameters below, this method also accepts the
+standard L<include_fields|Bugzilla::WebService/include_fields> and
+L<exclude_fields|Bugzilla::WebService/exclude_fields> arguments.
+
 =over
 
 =item C<ids> (array) 
@@ -482,34 +486,6 @@ if they try. (This is to make it harder for spammers to harvest email
 addresses from Bugzilla, and also to enforce the user visibility
 restrictions that are implemented on some Bugzillas.)
 
-=item C<include_fields> (array)
-
-An array of strings, representing the names of keys in the hashes 
-this function returns. Only the fields specified in this hash will be 
-returned, the rest will not be included.
-
-Essentially, this is a way to make the return value smaller, for performance
-or bandwidth reasons.
-
-If you specify an empty array, then this function will return empty hashes.
-
-Invalid field names are ignored.
-
-=item C<exclude_fields> (array)
-
-An array of strings, representing the names of keys in the hashes this 
-function returns. The fields specified will not be excluded from the 
-returned hashes.
-
-Essentially, this is a way to exclude certain fields from the returned
-hashes, for performance or bandwidth reasons.
-
-If you specify all the fields, then this function will return empty hashes.
-
-Invalid field names are ignored.
-
-This overrides C<include_fields>.
-
 =back
 
 =item B<Returns> 
diff --git a/template/en/default/global/code-error.html.tmpl b/template/en/default/global/code-error.html.tmpl
index 37e052f81..da8f902d4 100644
--- a/template/en/default/global/code-error.html.tmpl
+++ b/template/en/default/global/code-error.html.tmpl
@@ -345,6 +345,12 @@
     a <code>[% param FILTER html %]</code> argument, and that
     argument was not set.
 
+  [% ELSIF error == "params_required" %]
+    [% title = "Missing Parameter" %]
+    The function <code>[% function FILTER html %]</code> requires
+    that you set one of the following parameters:
+    <code>[% params.join(', ') FILTER html %]</code>
+
   [% ELSIF error == "product_empty_group_controls" %]
     [% title = "Missing Group Controls" %]
     New settings must be defined to edit group controls for
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index 191f6eebd..d92b04622 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -239,10 +239,16 @@
     [% title = "Chart Too Large" %]
     Sorry, but 2000 x 2000 is the maximum size for a chart.
 
+  [% ELSIF error == "comment_id_invalid" %]
+    [% id FILTER html %] is not a valid comment id.
+
   [% ELSIF error == "comment_invalid_isprivate" %]
     You tried to modify the privacy of comment id [% id FILTER html %],
     but that is not a valid comment on this [% terms.bug %].
 
+  [% ELSIF error == "comment_is_private" %]
+    Comment id [% id FILTER html %] is private.
+
   [% ELSIF error == "comment_required" %]
     [% title = "Comment Required" %]
     You have to specify a