994896 - Add the ability to get comments, attachments, and history using Bug.get

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: API, enhancement, P2)

Description

[David Lawrence [:dkl]]

Once bug 540818 is done we will have the ability to add fields to the WebService calls that are not always returned by default. This will allow people to get comments and attachments using Bug.get if they explicitly ask for them. This bug is about adding that ability and i will post a patch for review soon.

dkl

Comment 1

[David Lawrence [:dkl]]

Attachment: 994986_1.patch

Adding this patch here for now. Once 540818 is committed, I may have to update this patch a little to allow it to apply cleanly.

dkl

Comment 2

[Kohei Yoshino [:kohei]]

It would be nice if we can get the history also.

Comment 3

[David Lawrence [:dkl]]

Attachment: 994896_1.patch

Comment 4

[Gervase Markham [:gerv]]

Comment on attachment 8405473 [details] [diff] [review]
994896_1.patch

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

::: Bugzilla/WebService/Bug.pm
@@ +1481,5 @@
> +        changes => []
> +    };
> +
> +    my %api_name = reverse %{ Bugzilla::Bug::FIELD_MAP() };
> +    $api_name{'bug_group'} = 'groups';

Can we have a comment to explain this line? Why is this not in FIELD_MAP() itself?

@@ +1492,5 @@
> +        }
> +        $change->{removed}    = $self->type('string', $change->{removed});
> +        $change->{added}      = $self->type('string', $change->{added});
> +        $change->{field_name} = $self->type('string', $api_field);
> +        delete $change->{fieldname};

It looked to me here like you set a value then immediately delete it, but then I noticed the lack of an _. This is probably worth a comment, though. Or do the delete further up, where you extract the value. That would make it clearer.

@@ +2748,5 @@
> +=over
> +
> +=item C<id>
> +
> +C<int> The id of the flag.

By this, you actually mean the id of this particular instance of this flag? I.e. a unique-across-Bugzilla ID for this particular flag set this particular time on this particular bug?

@@ +2756,5 @@
> +C<string> The name of the flag.
> +
> +=item C<type_id>
> +
> +C<int> The type id of the flag.

Is it worth noting how one can use the API to convert this into a useful flag name?

@@ +2806,5 @@
> +
> +=item count
> +
> +C<int> The number of the comment local to the bug. The Description is 0,
> +comments start with 1.

Add a note that the numbers may not be consecutive if you are unable to see some comments? (I hope that's true, otherwise the comment numbers will be wrong!)

@@ +2825,5 @@
> +
> +C<dateTime> This is exactly same as the C<time> key. Use this field instead of
> +C<time> for consistency with other methods including L</get> and L</attachments>.
> +For compatibility, C<time> is still usable. However, please note that C<time>
> +may be deprecated and removed in a future release.

We should set specific deprecation version numbers rather than this vagueness, because otherwise we will either not remove them for fear of upsetting someone, or we will remove them and someone will be upset with us.

If API breaking changes are only on major releases, then you should mention Bugzilla 6.0 here.

Comment 5

[Gervase Markham [:gerv]]

There's an inconsistency in your treatment of attach_id. On comments, if there's no attach_id, the field is null. On history entries, if there's no attach_id, the field is not present. I suggest you adopt the same strategy for both; I'd recommend the second one.

+For compatibility, C<time> is still usable. However, please note that C<time>
+may be deprecated and removed in a future release.

Can we get rid of it immediately on the REST API (just so we don't introduce already-deprecated things) or is that complicated?

Also, we should specifically document that you can get attachment metadata without the data (which can be big) using:

http://bugzilla/rest/bug/1?include_fields=attachments&exclude_fields=attachments.data

This is commonly-requested thing, so I think it makes sense to say something specifically.

Gerv

Comment 8

[David Lawrence [:dkl]]

Attachment: 994896_2.patch

Comment 9

[Dylan Hardison [:dylan] (he/him)]

I suppose to review this properly, we need to green-up bugzilla-master: https://treeherder.mozilla.org/#/jobs?repo=bugzilla-master

Comment 10

[Dylan Hardison [:dylan] (he/him)]

I actually got this mostly reviewed today. I know this will make kohei happy :-)

Comment 11

[Kohei Yoshino [:kohei]]

\o/

Comment 12

[Leif Gruenwoldt]

Awesome.

What release is this expected to land in?

Comment 13

[Dylan Hardison [:dylan] (he/him)]

(In reply to Leif Gruenwoldt from comment #12)
> Awesome.
> 
> What release is this expected to land in?

In 6.0

Comment 14

[Kohei Yoshino [:kohei]]

Is the patch ready? If so, I want to see a PR for BMO so it can be included later in Bugzilla 6.0 :) If :dkl doesn’t have time now, I can take over.

Comment 15

[Kohei Yoshino [:kohei]]

Looks like the patch has to be updated anyway for the current, unversioned BMO code. Taking.

Comment 16

[Kohei Yoshino [:kohei]]

Attachment: GitHub Pull Request

Comment 17

[Kohei Yoshino [:kohei]]

Merged to master.