978773 - API to query flag history by user

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, defect)

Description

[Mark Côté [:mcote]]

Now that bug 956229 added the ability to properly track flag activity, we should add an endpoint to query for flag activity for a given user.  Probably worth being able to specify the particular flag(s) as well, optionally.

Comment 1

[David Lawrence [:dkl]]

Dylan, IMO this should implemented as thus:

1) REST endpoint changes:

qr{^/review/flag_activity$}, {
    GET => {
	method => 'flag_activity',
    },
},
qr{^/review/flag_activity/user/(.*)$}, {
    GET => {
	method => 'flag_activity',
	params => sub {
	    return { user => $_[0] }
        },
    },
},

The first one will allow for querying based on flag id *and* user such as 
/rest/review/flag_activity?flag_id=1234&user=[e-mail]

The second is a shortcut for just specifying all flags for a given user
/rest/review/flag_activity/user/[e-mail]

2) Review.flag_activity will look for the additional 'user' parameter and filter on that obviously in addition to the flag_id param if provided.

In hindsight I wish we originally named flag_id as type_id as for consistency and less confusion as internally we have type ids and flag ids and are different. As I can't see when we would ever need to query on a individual flag id then I suppose we can let it ride.

dkl

Comment 2

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

The flag states have one or two users associated with them -- the setter and the requestee. When we search for events for a particular user, do we want to consider both?

Comment 3

[Mark Côté [:mcote]]

Ah good question.  The use case I have in mind is "how long does it take person X to complete a review", so in this case it would be the requestee.  However I can see the setter perhaps being useful in other situations... so maybe just name this API appropriately, flag_requestee or something like that.  We can add more APIs as needed, since there are many ways to digest this info.

Comment 4

[David Lawrence [:dkl]]

(In reply to Mark Côté ( :mcote ) from comment #3)
> Ah good question.  The use case I have in mind is "how long does it take
> person X to complete a review", so in this case it would be the requestee. 
> However I can see the setter perhaps being useful in other situations... so
> maybe just name this API appropriately, flag_requestee or something like
> that.  We can add more APIs as needed, since there are many ways to digest
> this info.

Goog points. Revised proposal if you choose to accept it:

The first one will allow for querying based on flag id *and* user such as 
/rest/review/flag_activity?flag_id=1234&requestee=[e-mail]

/rest/review/flag_activity?setter=[e-mail]

The second is a shortcut for just specifying all flags for a given user
/rest/review/flag_activity/requestee/[e-mail]

or 

/rest/review/flag_activity/setter/[e-mail]

dkl

Comment 5

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

Attachment: bug-978773-v1.patch

Here's the patch for requestee/setter flag status viewing.

Comment 6

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

For Oracle, the MERGE INTO statenebt would be used.

Comment 7

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

ignore my previous comment, it was for an unrelated bug.

Comment 8

[David Lawrence [:dkl]]

Comment on attachment 8389153 [details] [diff] [review]
bug-978773-v1.patch

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

Please add POD for the new method and REST endpoints. Also sorry didn't think of it before, but please add the ability to search on type_id, similar to flag_id, as I could see a use case for showing me "all 'review' flag history for dkl@mozilla" for example. Rest of code mostly looks fine so far.

dkl

::: extensions/Review/lib/WebService.pm
@@ +145,5 @@
> +        qr{^/review/flag_activity/(requestee|setter)/(.*)$}, {
> +            GET => {
> +                method => 'flag_activity',
> +                params => sub {
> +                    return {$_[0] => $_[1]};

nit: space after { and before }

Comment 9

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

Attachment: bug-978773-v2.patch

Documentation & search by type_id done. I also made the whitespace change -- I am a little confused as to when we want white spaces in hashref constructors vs. when we do not, as I've now had to correct it both ways.

Comment 10

[David Lawrence [:dkl]]

Comment on attachment 8392040 [details] [diff] [review]
bug-978773-v2.patch

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

Another thing we should consider is adding MAX_RESULTS constant and pass that into Bugzilla::Extension::Review::FlagStateActivity->match() as limit
if $params->{limit} is not provided as this table will grow rather large over time. See what we do in Bugzilla::WebService::Bug::search as to how to
determine what the value of limit should be for match(). Ping me if you are not sure what I mean.

::: extensions/Review/lib/WebService.pm
@@ +84,5 @@
> +    }
> +
> +    if (my $type_id = $params->{type_id}) {
> +        detaint_natural($type_id)
> +          or ThrowUserError('invalid_flag_type_id', {type_id => $flag_id});

ThrowUserError('invalid_flag_type_id', {type_id => $type_id});

@@ +92,5 @@
> +
> +    for my $user_field (qw( requestee setter )) {
> +        if (my $user_name = $params->{$user_field}) {
> +            my $user = Bugzilla::User->check({name => $user_name, cache => 1})
> +                or ThrowUserError('invalid_username', {name => $user_name});

Bugzilla::User->check will throw it's own 'object_does_not_exist' before it gets to your ThrowUserError(). You can pass in an error that you prefer check() to use as a param to check() or just use Bugzilla::User->new and then use your ThrowUserError().
The former may be cleaner IMO.

my $user = Bugzilla::User->check({ name => $user_name, cache => 1, _error => 'invalid_username' });

@@ +148,5 @@
>                  },
>              },
>          },
> +        # flag activity by user
> +        qr{^/review/flag_activity/(requestee|setter|type)/(.*)$}, {

Change type to type_id:

qr{^/review/flag_activity/(requestee|setter|type_id)/(.*)$}, {

@@ +302,5 @@
> +=over
> +
> +=item B<Description>
> +
> +Returns the history of flag status changes

Returns the history of flag status changes based on requestee, setter, flag_id, type_id, or all.

@@ +324,5 @@
> +Query by flag:
> +
> +=over
> +
> +=over

Looks fine without using the extra =over =back IMO.

@@ +328,5 @@
> +=over
> +
> +=item C<flag_id> (integer) - The flag ID.
> +
> +Note that searching by C<flag_id> is not reliable because when flags are removed, flag_ids cease to exist.

flag_ids cease to exist in the C<flags> table.

@@ +364,5 @@
> +=over
> +
> +=over
> +
> +=item C<type_id> (int) - The flag type id of a change

The flag type id of the flag changed.

@@ +402,5 @@
> +=item C<email> (string) - The user's email address (aka login).
> +
> +=item C<name> (string) - The user's display name (may not match the Bugzilla "real name").
> +
> +=item C<review_count> (string) - The number of "review" and "feedback" requests in the user's queue.

The above is not complete. What you have pertains to the user objects (setter/requestee) but doesn't have anything about the flag data itself such as type, status, creation_time, bug_id, attachment_id, etc.

::: template/en/default/global/user-error.html.tmpl
@@ +997,5 @@
>  
> +  [% ELSIF error == "invalid_flag_type_id" %]
> +    [% title = "Invalid Flag Type ID" %]
> +    The flag type id [% type_id FILTER html %] is invalid.
> +

If we are only using this for the extension currently, we should move it to extenstions/Review/template/en/default/hook/global/user-error-errors.html.tmpl.

Comment 11

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

Attachment: bug-978773-v3.patch

Sorry this took longer than I said, my brain was really dragging ass on the documentation part yesterday.

Comment 12

[David Lawrence [:dkl]]

Comment on attachment 8393616 [details] [diff] [review]
bug-978773-v3.patch

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

*** ERROR: =over on line 313 without closing =back at line EOF in file extensions/Review/lib/WebService.pm
not ok 494 - extensions/Review/lib/WebService.pm has incorrect POD syntax --ERROR
#   Failed test 'extensions/Review/lib/WebService.pm has incorrect POD syntax --ERROR'
#   at t/011pod.t line 56.

Otherwise works well in testing. Quick r+ after the issues are resolved.

dkl

::: extensions/Review/lib/WebService.pm
@@ +323,5 @@
> +GET /rest/review/flag_activity/requestee/C<requestee>
> +
> +GET /rest/review/flag_activity/setter/C<setter>
> +
> +GET /rest/review/flag_activity/type/C<type-id>

s/type/type_id/

@@ +331,5 @@
> +The returned data format is the same as below.
> +
> +=item B<Params>
> +
> +Query by flag:

I would just list all of the params one by one and leave out the "Query by .." portions. No need for the multiple section as it is redundant. Above below B<Params> I would just say something like 

Use one or more of the following parameters to find specific flag status changes.

@@ +387,5 @@
> +An array of hashes with the following keys/values:
> +
> +=over
> +
> +=item flag_id

=item C<flag_id> (integer) 

same format for the other return values.

@@ +453,5 @@
> +=back
> +
> +=item requestee
> +
> +The requestee is the bugzilla user that is specified by the flag. Optional - absent if there is no requestee.

Make note that the requestee is a user object same as 'setter' with the same keys/values.

Comment 13

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

Attachment: bug-978773-v3.1.patch

Comment 14

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

Attachment: bug-978773-v3.2.patch

Comment 15

[David Lawrence [:dkl]]

Comment on attachment 8393938 [details] [diff] [review]
bug-978773-v3.2.patch

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

Nice. r=dkl

::: extensions/Review/lib/WebService.pm
@@ +367,5 @@
> +An object with the following fields:
> +
> +=over
> +
> +=item id (integer)

Enclose the return key names with C<> such as C<id> to match the B<Params> keys above on commit.

@@ +409,5 @@
> +The id of the bugzilla user. A unique integer value.
> +
> +=item real_name (string)
> +
> +The real name of the bugzilla user. 

nit: trailing whitespace

Comment 16

[David Lawrence [:dkl]]

Good work Dylan. Committed.

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   99457cc..8ba59b5  4.2 -> 4.2

dkl