714343 - Add ability to get flag information for bugs and attachments via the web service

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[Reed Loden [:reed]]

In Bugzilla/WebService/Bug.pm,

sub _attachment_to_hash {
    my ($self, $attach, $filters) = @_;

    # Skipping attachment flags for now.
    delete $attach->{flags};

Need to add support for attachment flags, including creation time, modification time, etc.

Comment 1

[David Lawrence [:dkl]]

Will look at this the first week of the new year.

Comment 2

[David Lawrence [:dkl]]

Note to self: Make sure to add modification_date and creation_ts to the output hash for bug 714297

dkl

Comment 3

[David Lawrence [:dkl]]

Attachment: Patch to add flags to attachments data (v1)

New field returned by Bug.attachments that contains flag information for an attachment. 

Requires patch from bug 714370 to be applied first.

Please review
dkl

Comment 4

[David Lawrence [:dkl]]

Comment on attachment 587453 [details] [diff] [review]
Patch to add flags to attachments data (v1)

New patch coming. Need to update the POD docs.

dkl

Comment 5

[David Lawrence [:dkl]]

Attachment: Patch to add flags to attachments data (v1)

New field returned by Bug.attachments that contains flag information for an attachment. 

Requires patch from bug 714370 to be applied first.

Please review
dkl

Comment 6

[:glob ✱]

Comment on attachment 587467 [details] [diff] [review]
Patch to add flags to attachments data (v1)

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

::: Bugzilla/WebService/Bug.pm
@@ +959,5 @@
> +        my $field_id = $field . "_id";
> +        if ($flag->$field_id && filter_wants $filters, $field) {
> +            $item->{$field} = $self->type('string', $flag->$field->login);
> +        }
> +    }

you should always add the setting and requestee information when flags are requested; making them optional and disabled by default over-complicates things.

Comment 7

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #6)
> you should always add the setting and requestee information when flags are
> requested; making them optional and disabled by default over-complicates
> things.

It does return them by default. It is sort of misleading when they are broken out separately
like this as all it really does is allow them to be affected by include_fields and exclude_fields. If you do not specify include/exclude_fields in your params, all fields are returned by default.

dkl

Comment 8

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #7)
> It does return them by default. It is sort of misleading when they are
> broken out separately

ah, you're correct, sorry about that.

these filters don't appear to work however; setting exclude_fields to 'requestee' doesn't exclude the field.  maybe it's better to drop the filter_wants from the loop and always return the setter and requestee if flags are returned?

Comment 9

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #8)
> these filters don't appear to work however; setting exclude_fields to
> 'requestee' doesn't exclude the field.  maybe it's better to drop the
> filter_wants from the loop and always return the setter and requestee if
> flags are returned?

Yeah because in attachments() I am not passing $params to _flag_to_hash as it would interfere with the filtering of attachment key/values as well. But I coded it to
look at $params if passed in case other code wanted to use the _flag_to_hash function
such as maybe a future Bug.flags, etc. So the filter/filter_wants calls in _flag_to_hash are harmless. If it makes things better for now, I can remove the filters in _flag_to_hash and just return a simple hash.

dkl

Comment 10

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #9)
> If it makes things better for now, I can remove
> the filters in _flag_to_hash and just return a simple hash.

yeah, i think that's the best approach for now.

Comment 11

[David Lawrence [:dkl]]

Attachment: Patch to add flags to attachments data (v2)

Thanks for the review. Here is a new patch with suggested changes.
dkl

Comment 12

[:glob ✱]

Comment on attachment 587763 [details] [diff] [review]
Patch to add flags to attachments data (v2)

r=glob

Comment 13

[Josh Aas]

I think I was told this is fixed and live now, is that correct? Can we mark this bug fixed?

Comment 14

[David Lawrence [:dkl]]

(In reply to Josh Aas (Mozilla Corporation) from comment #13)
> I think I was told this is fixed and live now, is that correct? Can we mark
> this bug fixed?

The current implementation is live (BMO) allowing the BzAPI developer (gerv) to start adding support to use the new data if he prefers. Others using the XMLRPC/JSONRPC API directly can also take advantage of the new return data. There is discussion on the returned date format ongoing in bug 714370 that may or may not alter the way the dates are being returned. The BzAPI can make adjustments on its end to accommodate the different format if it changes.

That being said, this bug has not been closed as this is still not checked into the upstream repo pending the final decision of its blocker bug, bug 714370.

dkl

Comment 15

[Frédéric Buclin]

Just to be sure: why didn't you implement the ability to get bug flags too? It shouldn't require a lot more work. It seems a bit weird to be able to get flags for attachments only, but not for bugs. I tought about that while reading bug 506538.

Comment 16

[David Lawrence [:dkl]]

Attachment: Patch to add flags to bug and attachment data (v1)

Adding flag data to both Bug.get and Bug.attachments.

dkl

Comment 17

[:glob ✱]

Comment on attachment 593611 [details] [diff] [review]
Patch to add flags to bug and attachment data (v1)

r=glob by inspection

Comment 18

[Frédéric Buclin]

Comment on attachment 593611 [details] [diff] [review]
Patch to add flags to bug and attachment data (v1)

>+=item C<requestee>
>+
>+C<string> The login name of the user this flag has been requested to be granted or denied.

I think you should mention that this field is only returned if defined.


>+=item C<bug_id>
>+
>+C<int> The bug id the attachment for which this flag belongs to.

This information is already available in Bug.attachments and Bug.get. I think redundancy is bad here. Remove it as well as attachment_id, which is also already available when calling Bug.attachments.


>+=item C<requestee>
>+
>+C<string> The login name of the user this flag has been requested to be granted or denied.

Same here. Only returned if defined.


Everything else looks good and works fine.

Comment 19

[David Lawrence [:dkl]]

Attachment: Patch to add flags to bug and attachment data (v2)

Thanks for the review. This should address your issues.

dkl

Comment 20

[Frédéric Buclin]

Comment on attachment 597170 [details] [diff] [review]
Patch to add flags to bug and attachment data (v2)

Yes, thanks. r=LpSolit

Comment 21

[David Lawrence [:dkl]]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk
modified Bugzilla/WebService/Bug.pm
Committed revision 8108.

dkl

Comment 23

[Frédéric Buclin]

Added to the relnotes for 4.4.