956229 - develop a system to track the lifetime of review/feedback/needinfo requests

Status: RESOLVED FIXED

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

Description

[:glob ✱]

currently it's difficult to use bugzilla's flag ui/api to determine the full lifetime of a review/feedback/needinfo request.

issues:
- use of strings in bugs_activity causes several issues
  - hard to track if the requestee's login is changed
  - can't use database queries to lookup information
- lack of exposed id on attachment flags

as we rely on these flags for significant metrics, it would be valuable to develop a system which clearly tracks this information in an easily consumable way.

plan:
- extend the review extension to a table for a historical view of the flag's changes
- add api calls and html views of table

flag_state_activity table:
  id
  modification_ts
  flag_id
  requester_id
  requestee_id
  bug_id
  attachment_id
  flag_type_id
  creation_ts
  closed_ts
  status ('+', '-', 'X')

at creation time and after each change is made, a row is inserted into the flag_state_activity table with the timestamp and the current state of the flag. the most recent row in this table reflects the current state of the flag.
migration of existing flags into this table is outside of the scope of the initial development, and can be added later.

Comment 1

[Mark Côté [:mcote]]

dkl and I were thinking that we could do most of this by just fixing the bugs activity table to actually use IDs.  Then this extension could just be an API to various queries to get review information out.  Does that seem reasonable?  It would have the effect of both a generic fix applicable to other areas and wouldn't result in duplicated data.

Comment 2

[:glob ✱]

(In reply to Mark Côté ( :mcote ) from comment #1)
> dkl and I were thinking that we could do most of this by just fixing the
> bugs activity table to actually use IDs.

excellent point regarding reducing duplication, but just adding IDs to the activity table unfortunately won't help in all cases.

for example, it'll be useful for the requestee, however the flag name and status needs to be in individually queryable without requiring string manipulation, and linking to the flag's ID won't provide a point-in-time value for it's status.

this could be addressed by added multiple columns to the bugs_activity table, but i fear this may be a slippery slope, and would introduce performance issues on the table (i'm keen to avoid another "hot table with lots of columns" situation).


we could remove duplication by joining with bugs_activity...

flag_state_activity table:
  id
  activity_id (FK bugs_activity.id)
  flag_id
  requestee_id
  status ('+', '-', 'X')

Comment 3

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› (unavailable until Jan 12th) from comment #2)
> (In reply to Mark Côté ( :mcote ) from comment #1)
> > dkl and I were thinking that we could do most of this by just fixing the
> > bugs activity table to actually use IDs.
> 
> excellent point regarding reducing duplication, but just adding IDs to the
> activity table unfortunately won't help in all cases.
> 
> for example, it'll be useful for the requestee, however the flag name and
> status needs to be in individually queryable without requiring string
> manipulation, and linking to the flag's ID won't provide a point-in-time
> value for it's status.
> 
> this could be addressed by added multiple columns to the bugs_activity
> table, but i fear this may be a slippery slope, and would introduce
> performance issues on the table (i'm keen to avoid another "hot table with
> lots of columns" situation).


Here is the way I envisioned it:

    bugs_activity => {
        FIELDS => [
            id        => {TYPE => 'INTSERIAL', NOTNULL => 1,
                          PRIMARYKEY => 1},
            bug_id    => {TYPE => 'INT3', NOTNULL => 1,
                          REFERENCES => {TABLE  => 'bugs',
                                         COLUMN => 'bug_id',
                                         DELETE => 'CASCADE'}},
            who       => {TYPE => 'INT3', NOTNULL => 1,
                          REFERENCES => {TABLE  => 'profiles',
                                         COLUMN => 'userid'}},
            bug_when  => {TYPE => 'DATETIME', NOTNULL => 1},
            fieldid   => {TYPE => 'INT3', NOTNULL => 1,
                          REFERENCES => {TABLE  =>  'fielddefs',
                                         COLUMN =>  'id'}},
            added_string   => { TYPE => 'varchar(255)'},
            added_id       => { TYPE => 'INT3' },
            removed_string => { TYPE => 'varchar(255)'},
            removed_string => { TYPE => 'INT3' },
            attach_id => { TYPE => 'INT3',
                           REFERENCES => { TABLE  => 'attachments',
                                           COLUMN => 'attach_id',
                                           DELETE => 'CASCADE'}},
            requestee_id => { TYPE => 'INT3',
                              REFERENCES => { TABLE  => 'profiles',
                                              COLUMN => 'userid',
                                              DELETE => 'CASCADE'}},
            comment_id => { TYPE => 'INT4',
                            REFERENCES => { TABLE  => 'longdescs',
                                            COLUMN => 'comment_id',
                                            DELETE => 'CASCADE'}},
        ],

We would need to break all of the activity entries we currently combine into single rows such as flag changes, group changes, cc, etc. into individual rows. And each object class would need to have a function to help interpret it's own change row such as Bugzilla::Product, Bugzilla::User, etc. depending on fieldid.

For flags we could even involve both the added_string, added_id, and requestee_id fields together. Such as the added_id would be the flags type_id, the added_string would be for status and of course requestee_id for the requestee user_id if one exists.

Of course this is an oversimplification but is one way it could be done and would allow for more ease in implementing metrics going forward. The reason I brought this up is because currently for example we record the product or component names as strings. If the admin changes a component name, then the old name is still in record even though the bug displays the new name. If we used id's properly then that would be alleviated.

dkl

Comment 4

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #3)
> Here is the way I envisioned it:
> [snip]

i don't see any major issues with that as a standalone project, but it's a reasonable amount of refactoring which needs to happen upstream first.  

eg. the $changes hashref which is passed to extensions as well as internally will need to change structure to accommodate the extra fields, and Bugzilla::Object::update() will need to inspect the schema to determine which columns to treat as objects.

i'm surprised that proposal doesn't already exist as an upstream bug :)



perhaps it would be better to got with the augmentation table as a quick-win, while allowing the larger (and better) work to happen in an upstream bug?

Comment 5

[Mark Côté [:mcote]]

Yeah, now that I realize the scope of the changes to bugs activity that would have to happen, it probably does make sense to do this first.

Comment 6

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› (unavailable until Jan 12th) from comment #4) 
> perhaps it would be better to got with the augmentation table as a
> quick-win, while allowing the larger (and better) work to happen in an
> upstream bug?

<compliant>I agree my changes are not trivial and we need these metrics sooner,
so I also agree we can do the temporary solution now. As long as we eventually
get to the other.</compliant>

Comment 7

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› (unavailable until Jan 12th) from comment #4) 
> eg. the $changes hashref which is passed to extensions as well as internally
> will need to change structure to accommodate the extra fields, and
> Bugzilla::Object::update() will need to inspect the schema to determine
> which columns to treat as objects.
> 
> i'm surprised that proposal doesn't already exist as an upstream bug :)

I think that the ChangeSet.pm work that legneato did way back in bug 492674 would be the best fit for this and eventually we just take that effort over and incorporate the bugs_activity changes into that.

dkl

Comment 8

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

I will have a patch ready for review by the end of today. I've spent some time acquainting myself with the Bugzilla::Object "ORM" and creating a Bugzilla::Extension::Review::FlagStateActivity object and flag_state_activity table. This table is now being populated and what remains is writing a WebService to pull the data out. After someone reviews that, further work should be done on a UI to get at the data.

Comment 9

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

In my forth-coming patch, I ran into a problem: flag_state_activity references bugs_activity, but I can't figure out how to determine the what bugs_activity.id should be.

Comment 10

[David Lawrence [:dkl]]

(In reply to Dylan William Hardison [:dylan] from comment #9)
> In my forth-coming patch, I ran into a problem: flag_state_activity
> references bugs_activity, but I can't figure out how to determine the what
> bugs_activity.id should be.

Bugzilla/DB/Schema.pm line 343:

    bugs_activity => {
        FIELDS => [
            id        => {TYPE => 'INTSERIAL', NOTNULL => 1,
                          PRIMARYKEY => 1},

Bugzilla/DB/Schema/Mysql.pm line 114:

        INTSERIAL =>    'integer auto_increment',

So the field in flag_state_activity that references bugs_activity.id will need to be 'integer' 
as well or int(11) as reported by 'SHOW CREATE TABLE bugs_activity'.

Is that what you were asking?

dkl

Comment 11

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

So, in object_end_of_create() I want to insert a row into flag_state_activity (when $object is a Bugzilla::Flag object)

requestee_id is $object->requestee_id, flag_id is $object->id, status is $object->status, but I'm not sure where activity_id should come from.

Comment 12

[David Lawrence [:dkl]]

(In reply to Dylan William Hardison [:dylan] from comment #11)
> So, in object_end_of_create() I want to insert a row into
> flag_state_activity (when $object is a Bugzilla::Flag object)
> 
> requestee_id is $object->requestee_id, flag_id is $object->id, status is
> $object->status, but I'm not sure where activity_id should come from.

Sorry I didnt read the question earlier the right way.

This is not easy as Bugzilla::Flag::create which is called when a new flag is first created even during a bug update does not know about the bugs_activity table. And is called before LogActivityEntry is called in Bugzilla::Bug so the entry doesnt even exist yet really.

Somehow you would need to go back and insert the flag_state_changes.activity_id after the bugs_activity entry has been inserted for a flag change.

dkl

Comment 13

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

That's what I was afraid of.


The patch will be delayed while I dig into Bugzilla::Bug and figure out how to do this. I see there's a bz_last_key() method, which I'll need to call after the insert into bugs_activity...

Comment 14

[:glob ✱]

after poking around the code, i can see this quickly getting pretty messy and cumbersome.

one way of doing it would be to store the touched flag objects in an array in the request_cache, change LogActivityEntry to return an array of the IDs it inserted, add the IDs to the $changes object, and use bug level hooks to insert the rows into the flag_state_activity table.  it's unreasonably complicated with a lot of moving parts :(


an easier solution would be to go with the original schema proposal by writing all the data to the flag_state_activity table, and not join to the bugs_activity table.  yes, there'll be duplicate data, however it greatly simplifies the work required and the impact on the core code.

dkl, what are your thoughts on the original schema proposal?

Comment 15

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

I like the second option.

Comment 16

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #0)
> flag_state_activity table:
>   id
>   modification_ts
>   flag_id
>   requester_id

setter_id 

>   requestee_id
>   bug_id
>   attachment_id
>   flag_type_id

type_id

>   creation_ts
>   closed_ts
>   status ('+', '-', 'X')

Need '?' as well.

Wouldn't modification_ts and closed_ts be the same as the flag status would be final state of the flag anyway.

The rest looks fine to me.

dkl

Comment 17

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #16)
> Wouldn't modification_ts and closed_ts be the same as the flag status would
> be final state of the flag anyway.

good point.. let's drop the closed_ts column.

Comment 18

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

Under what conditions would the modification_ts be different from the creation_ts?

Comment 19

[:glob ✱]

(In reply to Dylan William Hardison [:dylan] from comment #18)
> Under what conditions would the modification_ts be different from the
> creation_ts?

when the flag's status or requestee changes.

hrm, it may be easier to track just the modification timestamp, and the creation_ts could be determined from the first row.

in summary:

flag_state_activity table:
  id             // autoinc
  flag_when      // insert/change timestamp (passed into hook)
  type_id        // fk
  flag_id        // fk
  setter_id      // fk
  requestee_id   // fk (nullable)
  bug_id         // fk
  attachment_id  // fk (nullable)
  status         // char ('?', '+', '-', 'X')

Comment 20

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

whee, patch coming very soon now. 

MariaDB [bmo]> select * from flag_state_activity;
+----+---------------------+---------+---------+-----------+--------------+--------+---------------+--------+
| id | flag_when           | type_id | flag_id | setter_id | requestee_id | bug_id | attachment_id | status |
+----+---------------------+---------+---------+-----------+--------------+--------+---------------+--------+
|  1 | 2014-01-31 10:21:19 |       4 |  727613 |    484240 |       484240 | 924031 |        814026 | ?      |
|  2 | 2014-01-31 10:21:19 |       4 |  727613 |    484240 |         NULL | 924031 |        814026 | +      |
+----+---------------------+---------+---------+-----------+--------------+--------+---------------+--------+
2 rows in set (0.00 sec)

Comment 21

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

So, in object_end_of_update(), the modification time passed in for the Flag isn't yet updated. What hook would be fired after the modification_date field is updated?

Comment 22

[David Lawrence [:dkl]]

(In reply to Dylan William Hardison [:dylan] from comment #21)
> So, in object_end_of_update(), the modification time passed in for the Flag
> isn't yet updated. What hook would be fired after the modification_date
> field is updated?

Add a new hook in Bugzilla::Flag called 'flag_end_of_update' and hook your extension into that.
Do not use monkey patches unless absolutely necessary. Also we do not want to call now() in the
extension as the timestamp should match modification_date in the flags table which also matches
the timestamps in bugs_activity.

    Bugzilla::Hook::process('flag_end_of_update',
        { flag => $self, changes => $changes });

dkl

Comment 23

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

Hmm, it turns out there is already a flag_end_of_update() called from Bugzilla::Flag->update_flags. Investigating.

Comment 24

[David Lawrence [:dkl]]

(In reply to Dylan William Hardison [:dylan] from comment #23)
> Hmm, it turns out there is already a flag_end_of_update() called from
> Bugzilla::Flag->update_flags. Investigating.

Hmm. Unfortunately do not think that will work as that hook does not have
access to the actual flag objects that have changed easily without making
some changes in the Bugzilla::Flag->update_flags subroutine. We would need
to know which flags were new, which were removed and which were updated.
You need actual flag objects to get the id information. The summaries
which are passed to the hook only contain text strings of the flags
that were created and changed and also just the login of the requestee
and not the actual user id.

I would suggest to change the hook name in update_flags() to 'flags_end_of_update'
and then have a new hook called 'flag_end_of_update' that will go in update().
You would need to update the extensions that currently call 'flag_end_of_update'
to the new hook name. 

Also note you will need a new hook in create() as well called 'flag_end_of_create'
that will need to be used. As you are inputting rows in your new table for new
flags as well as any flag changes and deletions.

dkl

Comment 25

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #24)
> I would suggest to change the hook name in update_flags() to
> 'flags_end_of_update'
> and then have a new hook called 'flag_end_of_update' that will go in
> update().

i'm not convinced this is the best course of action, as changing and moving that hook has the potential to break any other extensions using it.


i would address this issue by adding a single new hook into Bugzillla::Flag::update() inside the |if (changes)| block, which passes $self and $timestamp as params.  "flag_updated" sounds like a reasonable name.

we don't need to do anything for flag creation, as the timestamp is already in the params passed to the object_end_of_create hook.

Comment 26

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

I've renamed the existing hooks per dkl's comment #24 (and I will be reverting that and just adding a new hook per glob's comment #25) but a more interesting problem has become apparent:

When a flag is removed, the row from flags is DELETEd. If we set DELETE => 'CASCADE', then our flag activity state entry will disappear; if we set it to 'SET NULL', we must make flag_id nullable.

Is it acceptable to lose the information from the flags table when a flag is deleted?

Comment 27

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

(In reply to Dylan William Hardison [:dylan] from comment #26)
> I've renamed the existing hooks per dkl's comment #24 (and I will be
> reverting that and just adding a new hook per glob's comment #25) but a more
> interesting problem has become apparent:
> 
> When a flag is removed, the row from flags is DELETEd. If we set DELETE =>
> 'CASCADE', then our flag activity state entry will disappear; if we set it
> to 'SET NULL', we must make flag_id nullable.
> 
> Is it acceptable to lose the information from the flags table when a flag is
> deleted?
Actually, no information is lost as everything is in the flag_state_activity.
I would guess that we should either: 

1) not keep flag_id (bad idea)
2) not mark flag_id as a foreign key.

Comment 28

[:glob ✱]

(In reply to Dylan William Hardison [:dylan] from comment #27)
> 1) not keep flag_id (bad idea)
> 2) not mark flag_id as a foreign key.

making flag_state_activity.flag_id NULLable and doing a cascading SET NULL gets my vote.

Comment 29

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #28)
> (In reply to Dylan William Hardison [:dylan] from comment #27)
> > 1) not keep flag_id (bad idea)
> > 2) not mark flag_id as a foreign key.
> 
> making flag_state_activity.flag_id NULLable and doing a cascading SET NULL
> gets my vote.

+1

Comment 30

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

Attachment: bug-956229-v1.patch

I'm most sketchy about the WebService bit, so I look forward to seeing what will need to be changed there.

Comment 31

[David Lawrence [:dkl]]

Comment on attachment 8371112 [details] [diff] [review]
bug-956229-v1.patch

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

glob, should we just put this in it's own extension and track *all* flag changes, and not just review/feedback/needinfo? Could be useful for other people
needing this type of metric. Just curious on your opinion.

Also do you think we should allow more than one flag id to be specified at a time for the webservice API? I would think yes and we would just need to 
alter the @result returned to be a hash with the flag ids specified as the keys.

::: Bugzilla/Object.pm
@@ -432,4 @@
>      $dbh->bz_start_transaction();
>  
>      my $old_self = $self->new($self->id);
> -   

Same here. Do not remove whitespace in areas you are not modifying.

::: extensions/Example/Extension.pm
@@ -470,4 @@
>  
>  sub flag_end_of_update {
>      my ($self, $args) = @_;
> -    

No need to edit this file at all. As a general rule, only remove whitespace near/in the code area you are updating or adding to.

::: extensions/Review/lib/FlagStateActivity.pm
@@ +13,5 @@
> +use Bugzilla::Util qw(detaint_natural trim);
> +
> +use base qw( Bugzilla::Object );
> +
> +sub DB_TABLE {  'flag_state_activity' }

use constant DB_TABLE => 'flag_state_activity';

@@ +27,5 @@
> +        bug_id
> +        attachment_id
> +        status
> +    );
> +}

Just use a constant here unless you need to call subroutines, etc. to add/modify the column list.

use constant DB_COLUMNS => qw(
    id
    flag_when
    type_id
    flag_id
    setter_id
    requestee_id
    bug_id
    attachment_id
    status
);

@@ +29,5 @@
> +        status
> +    );
> +}
> +
> +sub LIST_ORDER { 'id' }

No need for this line as 'id' is default unless specified otherwise.

@@ +41,5 @@
> +        setter_id     => _check_required('setter_id'),
> +        bug_id        => _check_required('bug_id'),
> +        status        => _check_required('status'),
> +    };
> +}

use constant here as well. Also the subroutines should not have their parameters hardcoded as the code that uses VALIDATORS will pass in params of it's own.
each validator subroutine needs to be created specifically for the field unless you can reuse some of the built in validators such as Bugzilla::Object::check_boolean.

use constant VALIDATORS => {
    flag_when => \&_check_flag_when,
    [...]
};

sub _check_flag_when {
    my ($invocant, $date) = @_;
    $date = trim($date);
    return undef if !$date;
    datetime_from($date)
        || ThrowUserError('illegal_date', { date   => $date,
                                            format => 'YYYY-MM-DD HH24:MI:SS' });
    return $date;
}

and so on.

@@ +61,5 @@
> +sub setter_id     { $_[0]->{setter_id} }
> +sub bug_id        { $_[0]->{bug_id} }
> +sub requestee_id  { $_[0]->{requestee_id} }
> +sub attachment_id { $_[0]->{attachment_id} }
> +sub status        { $_[0]->{status} }

You should also other subroutines that return other useful data. Such as returning user objects for requestee and setter, FlagType objects that match type_id, and so on.
These will be helpful for the webservice API as well as when we do a UI for displaying this table later.

@@ +63,5 @@
> +sub requestee_id  { $_[0]->{requestee_id} }
> +sub attachment_id { $_[0]->{attachment_id} }
> +sub status        { $_[0]->{status} }
> +
> +1;

Will need POD here later down the road.

::: extensions/Review/lib/WebService.pm
@@ +76,5 @@
> +    my @result;
> +    if (my $flag_id = $params->{flag_id}) {
> +        if ($flag_id =~ /^(\d+)$/) {
> +            $flag_id = $1;
> +        }

Just use detaint_natural from Bugzilla::Util to do the check that it is an integer:

my $flag_id = $params->{'flag_id'};
detaint_natural($flag_id) || ThrowUserError(some_error);

@@ +79,5 @@
> +            $flag_id = $1;
> +        }
> +
> +        my $sth = $dbh->prepare('select * from flag_state_activity where flag_id = ?');
> +        $sth->execute($flag_id);

Use match() instead; 

       my $activity = Bugzilla::Extension::Review::FlagStateActivity::match({ flag_id => $flag_id });

This will return a list of FlagStateActivity objects.

@@ +87,5 @@
> +            my $type       = Bugzilla::FlagType->new( $object->type_id );
> +            my $setter     = Bugzilla::User->new( $object->setter_id );
> +            my $requestee  = Bugzilla::User->new( $object->requestee_id );
> +            my $bug        = Bugzilla::Bug->new( $object->bug_id );
> +            my $attachment = Bugzilla::Attachment->new( $object->attachment_id );

You cannot return objects in webservice data. You need to create hashes that represent the object data for each of the attributes as needed. And the values need to be properly converted to WebService types as well.

Such as:

foreach my $change (@$activity) {
    my $data = {
        type => {
            id   => $self->type('int', $change->type_id),
            name => $self->type('string', $change->type->name),
        }
        setter => {
            id        => $self->type('int', $change->setter_id),
            real_name => $self->type('string', $change->setter->name),
            name      => $self->type('email', $change->setter->login),
        },
        [...]
    };
    push(@result, $data);
}

You may want to create a separate function for doing this data hash to keep it clean such as _flag_change_to_hash() or similar. Look at the other Bugzilla/WebService/*.pm modules for examples.

@@ +141,5 @@
>          },
> +        qr{^/review/flag_activity/flag/(\d+)$}, {
> +            GET => {
> +                method => 'flag_activity',
> +                params => sub { +{ flag_id => $_[0] } },

Just curious as I have not used this in practice, what does it mean +{ ?

Comment 32

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #31)
> glob, should we just put this in it's own extension and track *all* flag
> changes, and not just review/feedback/needinfo? Could be useful for other
> people needing this type of metric. Just curious on your opinion.

no, i don't think it's worth extending this to all flags or splitting it into its own extension.

> Also do you think we should allow more than one flag id to be specified at a
> time for the webservice API? I would think yes and we would just need to 
> alter the @result returned to be a hash with the flag ids specified as the
> keys.

yes from me too.

> Just curious as I have not used this in practice, what does it mean +{ ?

it's a hint to the perl compiler that it's an anon hashref, not a code block.

Comment 33

[:glob ✱]

Comment on attachment 8371112 [details] [diff] [review]
bug-956229-v1.patch

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

looks like dkl jumped onto this review while i was doing it.  i've removed most of the duplicated points.

looks mostly good.

general points:
- don't touch files to just change whitespace (object.pm, example/extension.pm)
- don't use +{ unless the compiler is confused (it shouldn't be required anywhere here)
- follow the existing formatting conventions

::: Bugzilla/Flag.pm
@@ +481,5 @@
>                   undef, ($timestamp, $self->id));
>          $self->{'modification_date'} = format_time($timestamp,  '%Y.%m.%d %T');
> +
> +        Bugzilla::Hook::process( 'flag_updated',
> +            { flag => $self } );

this can go on a single line.

also add a comment to indicate it's a bmo specific hook, such as
# BMO - provide a hook which passes the flag object

::: extensions/Review/Extension.pm
@@ +256,4 @@
>      my ($self, $args) = @_;
>      my $object = $args->{object};
>  
> +

nit: adding a blank line here is unnecessary

@@ +286,5 @@
> +            bug_id        => $flag->bug_id,
> +            attachment_id => $flag->attach_id,
> +            status        => $status,
> +        }
> +    );

don't double-indent here.

..->create({
    flag_when => ..
    ..
});

@@ +615,5 @@
> +                }
> +            },
> +
> +            flag_id => {
> +                TYPE       => 'INT3',

should this be NOTNULL?  i don't see it being set to null anywhere

@@ +659,5 @@
> +                TYPE    => 'CHAR(1)',
> +                NOTNULL => 1,
> +            },
> +        ],
> +    };

nit: remove the blank lines between fields

::: extensions/Review/lib/WebService.pm
@@ +81,5 @@
> +
> +        my $sth = $dbh->prepare('select * from flag_state_activity where flag_id = ?');
> +        $sth->execute($flag_id);
> +
> +        while (my $row = $sth->fetchrow_hashref) {

as you've created a class for the table, you should use object methods to access.
Bugzilla::Extension::Review::FlagStateActivity->match({ flag_id => $flag_id })

even though it's going, for reference:
- uppercase your sql keywords
- never do |SELECT *|, explicitly name the fields
- use dbh->selectall_arrayref() instead of prepare/execute/fetch

@@ +87,5 @@
> +            my $type       = Bugzilla::FlagType->new( $object->type_id );
> +            my $setter     = Bugzilla::User->new( $object->setter_id );
> +            my $requestee  = Bugzilla::User->new( $object->requestee_id );
> +            my $bug        = Bugzilla::Bug->new( $object->bug_id );
> +            my $attachment = Bugzilla::Attachment->new( $object->attachment_id );

move the creation of each of these objects into the FlagStateActivity class.
you also need to enable caching here in the constructor:

sub type {
  my ($self) = @_;
  return Bugzilla::FlagType->new({ id => $self->{type_id}, cache => 1});
}

Comment 34

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

re: unary plus before a hashref -- I'd been in the habit of just using that in every case except for single scalar assignment -- I think it's a style I picked up from PBP. will fix that too.

Also I neglected to include the turning-of-of-auditing AUDIT_CREATES_*. Durp.

Comment 35

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

What's the rationale behind use constant? All use constant (should) be used is constants you want compiled away (which does not happen when they are called as methods).

Comment 36

[:glob ✱]

(In reply to Dylan William Hardison [:dylan] from comment #35)
> What's the rationale behind use constant? All use constant (should) be used
> is constants you want compiled away (which does not happen when they are
> called as methods).

doing things differently in a single file doesn't make any sense.  if bugzilla conventions are to be changed, then imho we should change all instances.  in this case i don't think it's worth the effort to make that size of change.

Comment 37

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

(In reply to Byron Jones ‹:glob› from comment #36)
> (In reply to Dylan William Hardison [:dylan] from comment #35)
> > What's the rationale behind use constant? All use constant (should) be used
> > is constants you want compiled away (which does not happen when they are
> > called as methods).
> 
> doing things differently in a single file doesn't make any sense.  if
> bugzilla conventions are to be changed, then imho we should change all
> instances.  in this case i don't think it's worth the effort to make that
> size of change.

Noted. I'll add that convention to my nebulous idea about adding more details to the bugzilla API documentation.

Comment 38

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

I'm about 1/2 done with the changes outlined by the reviews from glob and dkl. This is really a helpful process!

Comment 39

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

Attachment: bug-956229-v2.patch

Still needs pod. Not sure if I did the ThrowUserError/user-error template parts in the bugzilla-conventional way. 

How would I specify that the web service takes a list of flag_ids? (is there an existing web service that I can get an example from?)

Comment 40

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

Attachment: bug-956229-v2.5.patch

Comment 41

[David Lawrence [:dkl]]

Comment on attachment 8374156 [details] [diff] [review]
bug-956229-v2.5.patch

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

Looking better and does pass functional testing. Just needs a few more tweaks. r-

::: extensions/Review/Extension.pm
@@ +256,4 @@
>      my ($self, $args) = @_;
>      my $object = $args->{object};
>  
> +

Remove extra line

@@ +276,5 @@
> +sub _log_flag_state_activity {
> +    my ($self, $flag, $status) = @_;
> +
> +    Bugzilla::Extension::Review::FlagStateActivity->create(
> +        {

Nit-pick: 
    Bugzilla::Extension::Review::FlagStateActivity->create({

@@ +286,5 @@
> +            bug_id        => $flag->bug_id,
> +            attachment_id => $flag->attach_id,
> +            status        => $status,
> +        }
> +    );

nit-pick:
    });

::: extensions/Review/lib/FlagStateActivity.pm
@@ +49,5 @@
> +    flag_id   => _check_param_required('flag_id'),
> +    setter_id => _check_param_required('setter_id'),
> +    bug_id    => _check_param_required('bug_id'),
> +    status    => _check_param_required('status'),
> +};

I see why this is being done this way. One thing to keep in mind is that some of the fields
are different such as status and will have further validation that just whether
it is defined or not.

Example:

use constant VALIDATORS => {
    [...]
    status => \&_check_status
};


sub _check_status {
    my ($invocant, $status) = @_;
    $status = trim($status);
    if (!grep($status eq $_ , qw(X + - ?)) {
        ThrowUserError('flag_status_invalid', { status => $status });
    }
    return $status;
}

@@ +58,5 @@
> +sub setter_id     { $_[0]->{setter_id} }
> +sub bug_id        { $_[0]->{bug_id} }
> +sub requestee_id  { $_[0]->{requestee_id} }
> +sub attachment_id { $_[0]->{attachment_id} }
> +sub status        { $_[0]->{status} }

Add 'return' to be consistent with other Bugzilla coding style.

@@ +62,5 @@
> +sub status        { $_[0]->{status} }
> +
> +sub type {
> +    my ($self) = @_;
> +    $self->{type} //= Bugzilla::FlagType->new( { id => $self->type_id, cache => 1 } );

return  $self->{type} //= Bugzilla::FlagType->new( { id => $self->type_id, cache => 1 } );

And same for the others.

::: extensions/Review/lib/WebService.pm
@@ +73,5 @@
> +sub flag_activity {
> +    my ( $self, $params ) = @_;
> +    my $dbh = Bugzilla->switch_to_shadow_db();
> +
> +    my @result;

nit: @result above the foreach

@@ +79,5 @@
> +
> +    detaint_natural($flag_id)
> +        or ThrowUserError('invalid_flag_id', { flag_id => $flag_id });
> +
> +    my $matches = Bugzilla::Extension::Review::FlagStateActivity->match( { flag_id => $flag_id } );

nit: Remove extra space from match( { and } );

@@ +81,5 @@
> +        or ThrowUserError('invalid_flag_id', { flag_id => $flag_id });
> +
> +    my $matches = Bugzilla::Extension::Review::FlagStateActivity->match( { flag_id => $flag_id } );
> +    foreach my $fsa (@$matches) {
> +        push @result, $self->_flag_state_activity_as_hash($fsa);

nit: Parens around the args of push(). Also pass in $params to allow for filter()'ing the data fields returned.

@@ +124,5 @@
>          },
> +        qr{^/review/flag_activity/flag/(\d+)$}, {
> +            GET => {
> +                method => 'flag_activity',
> +                params => sub { +{ flag_id => $_[0] } },

s/\+{/{/

@@ +126,5 @@
> +            GET => {
> +                method => 'flag_activity',
> +                params => sub { +{ flag_id => $_[0] } },
> +            },
> +        },

I think it would be fine to omit /flag/ and just call it 

qr{^/review/flag_activity/(\d+)$}

Since we are it is already understood what is being returned by /flag_activity/

@@ +133,5 @@
>  
>  1;
>  
> +
> +sub _flag_state_activity_as_hash {

sub _flag_state_activity_to_hash

@@ +134,5 @@
>  1;
>  
> +
> +sub _flag_state_activity_as_hash {
> +    my ( $self, $fsa ) = @_;

my ($self, $fsa, $params) = @_;

Need to pass in $params to use for filter() when returning the data. This allows for specifying which fields to return using include_fields=id,status or exclude_fields=creation_time, etc.

@@ +137,5 @@
> +sub _flag_state_activity_as_hash {
> +    my ( $self, $fsa ) = @_;
> +
> +    return {
> +        flag_when => $self->type('string', $fsa->flag_when ),

Rename to 'when' instead of 'creation_time' to keep the user facing api keys simple and consistent with other webservice api names.

@@ +141,5 @@
> +        flag_when => $self->type('string', $fsa->flag_when ),
> +        type      => {
> +            id   => $self->type( 'int',    $fsa->type_id ),
> +            name => $self->type( 'string', $fsa->type->name ),
> +        },

Create a _flagtype_to_hash() sub. Use Bugzilla::WebService::Bug::_flagtype_to_hash as reference.

@@ +143,5 @@
> +            id   => $self->type( 'int',    $fsa->type_id ),
> +            name => $self->type( 'string', $fsa->type->name ),
> +        },
> +        setter     => $self->_api_user( $fsa->setter ),
> +        requestee  => $self->_api_user( $fsa->requestee ),

Only return 'requestee' if one exists, otherwise omit (more below)

@@ +148,5 @@
> +        bug        => { bug_id => $self->type( 'int', $fsa->bug_id ) },
> +        attachment => {
> +            id       => $self->type( 'int',    $fsa->attachment_id ),
> +            filename => $self->type( 'string', $fsa->attachment->filename ),
> +        },

I think just passing attachment_id only will be sufficient here and will get rid of the additional DB load of loading the attachment object data. There are webservice calls already that someone can use to get all of the attachment data for a given attachment id or ids.

@@ +150,5 @@
> +            id       => $self->type( 'int',    $fsa->attachment_id ),
> +            filename => $self->type( 'string', $fsa->attachment->filename ),
> +        },
> +        status => $self->type( 'string', $fsa->status ),
> +    };

continuation of requestee comment:

    my $data = {
        id        => $self->type('int', $fsa->id),
        creation_time => $self->type('string', $fsa->flag_when ),
        type      => $self->_flagtype_to_hash($fsa->type),
        setter    => $self->_user_to_hash($fsa->setter),
        requestee => $self->_user_to_hash($fsa->requestee),
        bug_id    => $self->type('int', $fsa->bug_id),
        attachment_id => $self->type('int', $fsa->attachment_id),
        status    => $self->type('string', $fsa->status),
    };

    if ($flag->requestee_id) {
        $data->{requestee} = $self->_user_to_hash($flag->requester);
    }
 
    return filter($params, $data);

@@ +153,5 @@
> +        status => $self->type( 'string', $fsa->status ),
> +    };
> +}
> +
> +sub _api_user {

sub _user_to_hash

@@ +165,5 @@
> +        };
> +    }
> +    else {
> +        return undef;
> +    }

No need for if () {} else {}

Comment 42

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

Attachment: bug-956229-v3.patch

Those tweaks made. Two questions I have are:

1) How do I handle multiple flag ids? comma-delimited?
2) for _flagtype_to_hash, do we need to include the possible values field?

Comment 43

[David Lawrence [:dkl]]

Comment on attachment 8377268 [details] [diff] [review]
bug-956229-v3.patch

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

::: Bugzilla/Flag.pm
@@ +481,5 @@
>                   undef, ($timestamp, $self->id));
>          $self->{'modification_date'} = format_time($timestamp,  '%Y.%m.%d %T');
> +
> +        # BMO - provide a hook which passes the flag object
> +        Bugzilla::Hook::process( 'flag_updated', { flag => $self } );

Nit-pick: remove extra whitespace after and before parentheses.

Thought about this some more. Please move this hook outside of the if (scalar(keys %$changes)) block and then pass in { changes => $changes } to the Hook itself as well.

This way extensions could make changes to a flag that is updated regardless if %$changes has values or not. Then in you extension code you can return early if not scalar(keys %$changes).

sub update {
    my $self = shift;
    my $dbh = Bugzilla->dbh;
    my $timestamp = shift || $dbh->selectrow_array('SELECT NOW()');

    my $changes = $self->SUPER::update(@_);

    if (scalar(keys %$changes)) {
        $dbh->do('UPDATE flags SET modification_date = ? WHERE id = ?',
                 undef, ($timestamp, $self->id));
        $self->{'modification_date'} = format_time($timestamp,  '%Y.%m.%d %T');
    }

    # BMO - provide a hook which passes the flag object
    Bugzilla::Hook::process( 'flag_updated', { flag    => $self,
                                               changes => $changes } );

    return $changes;
}

extensions/Review/Extension:

sub flag_updated {
    my ( $self, $args ) = @_;
    my $flag    = $args->{flag};
    my $changes = $args->{changes};
    return if !scalar(keys %$changes);
    if ( _is_countable_flag($flag) ) {
        $self->_log_flag_state_activity($flag, $flag->status);
    }
}

::: extensions/Review/lib/FlagStateActivity.pm
@@ +48,5 @@
> +    type_id   => _check_param_required('type_id'),
> +    flag_id   => _check_param_required('flag_id'),
> +    setter_id => _check_param_required('setter_id'),
> +    bug_id    => _check_param_required('bug_id'),
> +    status    => Bugzilla::Flag::UPDATE_VALIDATORS->{status},

You will need to make your own copy of Bugzilla::Flag::_check_status and reference that instead as Bugzilla::Flag::_check_status refers to
$self->status as it is an 'update' validator and not a 'create' validator. In our case $self->status is not populated yet and also I get the
following error when executing:

Can't use string ("Bugzilla::Extension::Review::Fla") as a HASH ref while "strict refs" in use at /loader/0x34efed0/Bugzilla/Extension/Review/FlagStateActivity.pm line 62. at /loader/0x34efed0/Bugzilla/Extension/Review/FlagStateActivity.pm line 62 \tBugzilla::Extension::Review::FlagStateActivity::status(...) called at Bugzilla/Flag.pm line 802 \tBugzilla::Flag::_check_status(...) called at Bugzilla/Object.pm line 651 \tBugzilla::Object::run_create_validators(...) called at Bugzilla/Object.pm line 600 \tBugzilla::Object::create(...) called at ./extensions/Review/Extension.pm line 277 \tBugzilla::Extension::Review::_log_flag_state_activity(...) called at ./extensions/Review/Extension.pm line 201 \tBugzilla::Extension::Review::object_end_of_create(...) called at Bugzilla/Hook.pm line 33 \tBugzilla::Hook::process(...) called at Bugzilla/Object.pm line 690 \tBugzilla::Object::insert_create_data(...) called at Bugzilla/Object.pm line 601 \tBugzilla::Object::create(...) called at Bugzilla/Flag.pm line 468 \tBugzilla::Flag::create(...) called at Bugzilla/Flag.pm line 528 \tBugzilla::Flag::update_flags(...) called at Bugzilla/Bug.pm line 940 \tBugzilla::Bug::update(...) called at /home/bugzilla/devel/htdocs/956229/process_bug.cgi line 410

@@ +62,5 @@
> +sub status        { $_[0]->{status} }
> +
> +sub type {
> +    my ($self) = @_;
> +    $self->{type} //= Bugzilla::FlagType->new( { id => $self->type_id, cache => 1 } );

nit-picks: 1) use return (same for all others) 2) remove extra whitespace between ( {

    return $self->{type} //= Bugzilla::FlagType->new({ id => $self->type_id, cache => 1 });

::: extensions/Review/lib/WebService.pm
@@ +124,5 @@
> +        qr{^/review/flag_activity/(\d+)$}, {
> +            GET => {
> +                method => 'flag_activity',
> +                params => sub {
> +                    return { flag_id => $_[0] } 

remove whitespace

@@ +164,5 @@
> +        type             => $self->type('string',  $flagtype->target_type),
> +        values           => \@values,
> +        is_active        => $self->type('boolean', $flagtype->is_active),
> +        is_requesteeble  => $self->type('boolean', $flagtype->is_requesteeble),
> +        is_multiplicable => $self->type('boolean', $flagtype->is_multiplicable)};

nit: move trailing }; to next line

Comment 44

[David Lawrence [:dkl]]

(In reply to Dylan William Hardison [:dylan] from comment #42)
> Created attachment 8377268 [details] [diff] [review]
> bug-956229-v3.patch
> 
> Those tweaks made. Two questions I have are:
> 
> 1) How do I handle multiple flag ids? comma-delimited?
> 2) for _flagtype_to_hash, do we need to include the possible values field?

Sorry that I missed this comment from before.

1) Normally we accept ids for the webservices only in list format and not a delimited string. For certain webservices that allow for more than one id you would just do &id=1&id=2&id=3... Then input_params would have converted that to a list reference already for you.

In this case, let's not worry about dealing with more than one id for now and we can add it later if the need arises.

2) Also to keep the code simple and quick, let's leave off values from the flag type object. As the webservice for the Review extension will not allow for adding/updating of flags for a bug/attachment, the client will not need to know the full list of possible values.

dkl

Comment 45

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

Ugh, the missing return statements was *supposed* to be in that patch, but I botched that up when preparing the patch. This workflow is still new to me. :)

Comment 46

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

Attachment: bug-956229-v4.patch

Comment 47

[David Lawrence [:dkl]]

Comment on attachment 8380641 [details] [diff] [review]
bug-956229-v4.patch

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

Looks good except for a couple errors. After those, a quick r+

::: extensions/Review/lib/FlagStateActivity.pm
@@ +9,5 @@
> +use strict;
> +use warnings;
> +
> +use Bugzilla::Util qw(trim validate_date);
> +use List::MoreUtils qw(none);

Need to also:

use Bugzilla::Error;

It dies when ThrowUserError was thrown in _check_date

@@ +55,5 @@
> +                date   => $date,
> +                format => 'YYYY-MM-DD'
> +            }
> +        );
> +    }

flag_when is the modification_time of the flag object so it is in the format YYYY-MM-DD HI24:MI:SS which causes this to error each time as validate_date only expects only YYYY-MM-DD.

Instead do:

sub _check_date {
    my ($invocant, $date) = @_;
    $date = trim($date);
    datetime_from($date)
        || ThrowUserError('illegal_date', { date   => $date,
                                            format => 'YYYY-MM-DD HH24:MI:SS' });
    return $date;
}

Of course you will need to change the use Bugzilla::Util line at the top.

Comment 48

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

Attachment: bug-956229-v5.patch

question -- dkl, do you like the removal of unused $self ($invocant) in this version?

Comment 49

[David Lawrence [:dkl]]

(In reply to Dylan William Hardison [:dylan] from comment #48)
> Created attachment 8383053 [details] [diff] [review]
> bug-956229-v5.patch
> 
> question -- dkl, do you like the removal of unused $self ($invocant) in this
> version?

I would leave it in for consistency with the other validators and for clarity on what the first param if we need to use it in this module for the future.

dkl

Comment 50

[David Lawrence [:dkl]]

Comment on attachment 8383053 [details] [diff] [review]
bug-956229-v5.patch

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

r=dkl with the comments fixed on commit

::: extensions/Review/lib/FlagStateActivity.pm
@@ +9,5 @@
> +use strict;
> +use warnings;
> +
> +use Bugzilla::Error qw(ThrowUserError);
> +use Bugzilla::Util qw(trim validate_date);

s/validate_date/datetime_from/

@@ +45,5 @@
> +    },
> +}
> +
> +sub _check_date {
> +    my (undef, $date) = @_;

I would leave it as $invocant for consistency and clarity.

Comment 51

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

Attachment: bug-956229-v6.patch

Final revision of patch for dkl to commit

Comment 52

[David Lawrence [:dkl]]

Attachment: 956229-schema-only_1.patch

Attaching the schema only patch for this bug. Going to commit this to bzr and glob can commit the full patch after the code has been pushed to production. Setting needinfo as reminder but feel free to remove it if you want.

dkl

Comment 53

[David Lawrence [:dkl]]

Schema changes only.

Committing to: bzr+ssh://[e-mail]/bmo/4.2
modified extensions/Review/Extension.pm
Committed revision 9257.

dkl

Comment 54

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bmo/4.2/
modified Bugzilla/Flag.pm
modified extensions/Review/Extension.pm
added extensions/Review/lib/FlagStateActivity.pm
modified extensions/Review/lib/WebService.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 9273.

Comment 55

[:glob ✱]

Comment on attachment 8383902 [details] [diff] [review]
bug-956229-v6.patch

bringing forward dkl's r+

Comment 56

[:glob ✱]

(In reply to Byron Jones ‹:glob› from comment #55)
> bringing forward dkl's r+

and here's the corresponding row from the flag_state_activity table for that change:

id,flag_when,type_id,flag_id,setter_id,requestee_id,bug_id,attachment_id,status
4,2014-03-06 00:25:24,4,823770,13647,,956229,8383902,+