Mark Côté [:mcote] Reporter
	Description • 10 years ago

The REST version of the bug-history API doesn't allow multiple IDs to be specified. This would be really useful for quickly getting recent changes to bugs, e.g. watch lists, last-visited, etc.

Also I'd like to see since/until params or the equivalent, with until defaulting to now.

	Mark Côté [:mcote] Reporter
	Comment 1 • 10 years ago

Oh also, we should include changes to comments and attachments as well, perhaps optionally, rather than making users do three separate calls to get full history.

	Frédéric Buclin
	Comment 2 • 10 years ago

(In reply to Mark Côté ( :mcote ) from comment #0)
> The REST version of the bug-history API doesn't allow multiple IDs to be
> specified. This would be really useful for quickly getting recent changes to
> bugs, e.g. watch lists, last-visited, etc.

Isn't this a dupe of bug 987690, which you already filed?

	Mark Côté [:mcote] Reporter
	Comment 3 • 10 years ago

Kind of. I'm going to change the other one to just be about a UI; I filed that before I fully understood the problem.

	David Lawrence [:dkl]
	Comment 4 • 10 years ago

(In reply to Mark Côté ( :mcote ) from comment #1)
> Oh also, we should include changes to comments and attachments as well,
> perhaps optionally, rather than making users do three separate calls to get
> full history.

I have had this on my list to file but I think this should be a separate bug to add the comments and attachment metadata to to the history list. Also I am proposing that we merge the extra data into the history list similar to inline history. Note to self: the comments and attachments should be off by default and use include_fields=comments,attachments to get the extra data to save on overhead.

As for this bug, I may transition it into a broader bug as the more I think of it, we should have relevant REST resources to allow all of our methods to work in this way. We have several other REST resources that only work with one ID at a time and we should have consistency across the board.

So I propose:

        qr{^/bug/([^/]+)/history$}, {
            GET => {
                method => 'history',
                params => sub {
                    return { ids => [ $_[0] ] };
                },
            }
        },
        qr{^/bug/history$}, {
            GET => {
                method => 'history',
            }
        },

So you can either do /rest/bug/<id>/history for a single bug or do /rest/bug/history?ids=<id1>&ids=<ids2> etc.

We should also (a different bug) allow for ids=<id1>,<id2>,... as shortcut specifically as it relates to IDs only. Code-wise it should be simple as we run the params through Bugzilla::WebService::Util::validate() which can do the fixup for us.

dkl

	Kohei Yoshino [:kohei]
	Comment 5 • 10 years ago

(In reply to Mark Côté ( :mcote ) from comment #1)
> Oh also, we should include changes to comments and attachments as well,
> perhaps optionally, rather than making users do three separate calls to get
> full history.

+1

Once it's resolved, my BzDeck app will migrate from BzAPI to the native REST API.
https://github.com/kyoshino/bzdeck/wiki/Tech-Notes#enhancement-requests

	Mark Côté [:mcote] Reporter
	Comment 6 • 10 years ago

Sure, spin off as you see fit. :) And a big +1 to adding support for multiple ids across the board.

	:glob ✱
	Comment 7 • 10 years ago

can't you do:

             GET => {
                 method => 'history',
                 params => sub {
-                    return { ids => [ $_[0] ] };
+                    return { ids => [ split(/,/ $_[0]) ] };
                 },
             }
         },


?

	David Lawrence [:dkl]
	Comment 8 • 10 years ago

(In reply to Byron Jones ‹:glob› from comment #7)
> can't you do:
> 
>              GET => {
>                  method => 'history',
>                  params => sub {
> -                    return { ids => [ $_[0] ] };
> +                    return { ids => [ split(/,/ $_[0]) ] };
>                  },
>              }
>          },
> 
> 
> ?

That would definitely work for REST, I would like it to be more universal in order to work for JSONRPC and XMLRPC as well even though they won't be around forever. So best place to add it would be in validate() IMO.

dkl

	:glob ✱
	Comment 9 • 10 years ago

(In reply to David Lawrence [:dkl] from comment #8)
> That would definitely work for REST, I would like it to be more universal in
> order to work for JSONRPC and XMLRPC as well even though they won't be
> around forever. So best place to add it would be in validate() IMO.

i'm not sure it makes sense for those other endpoints, given how easy it is to pass in an array to them.  

however i do see the need to be consistent across all endpoints here, so if it's easy to do it in a universal way i'm all for it.

	:glob ✱
	Comment 11 • 10 years ago

Comment on attachment 8400137 [details] [diff] [review]
987950_1.patch

Review of attachment 8400137 [details] [diff] [review]:
-----------------------------------------------------------------

this change means that comments and attachments are returned unless explicitly excluded (by using include_fields or exclude_fields).
for performance reasons i think this is a mistake - the comments and attachments should only be returned if explicitly requested.
this probably means fixing bug 540818 first.

the perldoc for the history method needs to be updated to reflect the new optional params, as well as the support for comma separated ids.
would it be bad form if the splitting of ids happened automatically in the rest server to make it applicable to all requests that accept ids?

::: Bugzilla/Attachment.pm
@@ +657,2 @@
>      my $attach_ids = $dbh->selectcol_arrayref("SELECT attach_id FROM attachments
> +                                               WHERE bug_id = ? $and_restriction $date_part",

this throws an undef error if you don't set 'after'

::: Bugzilla/WebService/Bug.pm
@@ +438,5 @@
> +                                         new_since => $params->{new_since} });
> +        foreach my $bug_id (keys %{ $comments->{bugs} }) {
> +            $comments_by_date{$bug_id} = {};
> +            foreach my $comment (@{ $comments->{bugs}->{$bug_id}->{comments} }) {
> +                $comments_by_date{$bug_id}->{$comment->{creation_time}} = $comment;

as it's possible for two comments to be created at the same time, this should be an array

@@ +493,5 @@
> +
> +            if (my $attachment = $attachments_by_date{$bug_history{when}}) {
> +                $bug_history{attachment} = $attachment;
> +                delete $attachments_by_date{$bug_id}->{$bug_history{when}};
> +            }

due to the "two comments at the same time" issue, also check the comment/attachment author here

	David Lawrence [:dkl]
	Comment 12 • 10 years ago

(In reply to Byron Jones ‹:glob› from comment #11)
> Comment on attachment 8400137 [details] [diff] [review]
> 987950_1.patch
> 
> Review of attachment 8400137 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this change means that comments and attachments are returned unless
> explicitly excluded (by using include_fields or exclude_fields).
> for performance reasons i think this is a mistake - the comments and
> attachments should only be returned if explicitly requested.
> this probably means fixing bug 540818 first

Yes unfortunately. This patch is in the theme of the way current webservices work. To do this one
before 540818, I could simple grep($_ eq 'comments', @{ $params->{include_fields} || [] }) and 
only include if explicitly asked for. But finishing 540818 would make this much simpler.

> the perldoc for the history method needs to be updated to reflect the new
> optional params, as well as the support for comma separated ids.
> would it be bad form if the splitting of ids happened automatically in the
> rest server to make it applicable to all requests that accept ids?

I would need to tell the REST code which paths could be comma delimited and which to leave alone as each REST path could have different variables types in it, such as usernames, etc. 

Maybe one more reason to do this in validate() and only if 'ids' or 'id' is a scalar and not already a list.

dkl

	David Lawrence [:dkl]
	Comment 13 • 10 years ago

The amount of change needed to do the multiple ID support began to grow as I started to make it work universally for all methods that take the params, 'ids', 'attach_ids', 'comment_ids', etc. so I think it would be best from a review/approval standpoint that we treat that separate from this bug. So I will file a different bug for that multiple ID support and finish this one to support attachments and comments as part of Bug.history.

dkl