684701 - add a "watch user" to components

Status: RESOLVED FIXED

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

Description

[:glob ✱]

from bug 684088 comment 14:

[..] extend the component-watching extension to add a new field to components: "watch user".  the username that we current put into the QA field will instead be put into this field.

when a change triggers a email to component watchers, we can use the bugmail_recipients hook to inject the watch-user account into the recipient list, and then anyone watching that user will also receive emails.  likewise if a watch-user is already on the recipient list (eg. via a CC), we inject all users watching the associated component(s).

the only upstream change required for this would be a hook to the "browse" report so the watch-users are discoverable.

Comment 1

[:glob ✱]

Attachment: patch v1

initial patch; i'm still working through some testing scenarios, putting it up here to get eyeballs on it early.

Comment 2

[David Lawrence [:dkl]]

Comment on attachment 558478 [details] [diff] [review]
patch v1

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

::: extensions/ComponentWatching/Extension.pm
@@ +281,5 @@
> +    $sth = $dbh->prepare("
> +        SELECT component_watch.user_id
> +          FROM components
> +               INNER JOIN component_watch ON component_watch.component_id = components.id
> +         WHERE watch_user in ($uidList)

WHERE components.watch_user IN ($uidList)

@@ +300,5 @@
> +    ");
> +    $sth->execute($oldComponentId, $newComponentId);
> +    while (my ($uid) = $sth->fetchrow_array) {
> +        if (!exists $recipients->{$uid}) {
> +            $recipients->{$uid}->{+REL_QA} = Bugzilla::BugMail::BIT_DIRECT();

Change to REL_COMPONENT_WATCHER so we don't have to worry with it in the future.

::: extensions/ComponentWatching/template/en/default/hook/admin/components/edit-common-rows.html.tmpl
@@ +5,5 @@
> +       name => "watch_user"
> +       id => "watch_user"
> +       value => comp.watch_user.login
> +       size => 64
> +       emptyok => 1

nit: line up the =>'s

::: extensions/ComponentWatching/template/en/default/hook/admin/components/list-before_table.html.tmpl
@@ +2,5 @@
> +
> +[% FOREACH my_component = product.components %]
> +  [% overrides.watch_user.name.${my_component.name} = {
> +        override_content => 1
> +        content => my_component.watch_user.login

nit: line up the =>'s

::: extensions/ComponentWatching/template/en/default/hook/global/user-error-errors.html.tmpl
@@ +1,5 @@
> +[% IF error == "component_watch_invalid_watch_user" %]
> +   [% title = "Invalid Watch User" %]
> +   The "Watch User" must be a <b>.bugs</b> email address.<br>
> +   For example: <i>[e-mail]</i>
> +[% END %]

Food for thought: Should we stick strictly to the old standard of *@*.bugs or should we allow any arbitrary bugzilla account? We already
require this user to be a valid account so technically it could be anyone. Historically I think *.bugs was chosen just to differentiate
between an internal user only and a normal user account. But do we really need or care to have that differentiation anymore?

Comment 3

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #2)
> Comment on attachment 558478 [details] [diff] [review]
> patch v1
>
> Historically I think *.bugs was chosen just to differentiate
> between an internal user only and a normal user account. But do we really
> need or care to have that differentiation anymore?

there's no technical reason why we can't make it any user, however we _always_ use a .bugs address currently, and i think it would be wise to continue that after we migrate.  as we now have the opportunity to validate this with code, i figure it's a good thing to do.

Comment 4

[David Lawrence [:dkl]]

Comment on attachment 558478 [details] [diff] [review]
patch v1

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

You also need to add code to Extension.pm for object_end_of_create that adds the new watch_user value (if changed) to the %changes hash. 
Once this is done you also need to create template hook file global/messages-messages.html.tmpl that shows that the watch_user value has been updated.

::: extensions/ComponentWatching/Extension.pm
@@ +126,5 @@
> +    my ($self, $args) = @_;
> +    my $object = $args->{object};
> +    my $columns = $args->{columns};
> +    return unless $object->isa('Bugzilla::Component');
> +

You also need here:

    my $input = Bugzilla->input_params;
    $object->set('watch_user', $input->{'watch_user'});

otherwise I was unable to get any changes to watch_user to stick when editing with editcomponents.cgi

Comment 5

[:glob ✱]

Attachment: patch v2

here's a working patch.

i decided to not override the browse product report, instead i display the watch-user on the component-watching preferences tab.

Comment 6

[David Lawrence [:dkl]]

Comment on attachment 559053 [details] [diff] [review]
patch v2

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

Some more small review. Running actual tests now and will finish this up tonight/tomorrow.

::: extensions/ComponentWatching/Extension.pm
@@ +164,5 @@
> +    my $old_id = $old_object->watch_user ? $old_object->watch_user->id : 0;
> +    my $new_id = $object->watch_user ? $object->watch_user->id : 0;
> +    return unless $old_id == $new_id;
> +
> +    $changes->{watch_user} = [ $old_id ? $old_id : undef, $new_id ? $new_id : undef ];

This can be shortened it to:

my $old_id = $old_object->watch_user ? $old_object->watch_user->id : undef;
my $new_id = $object->watch_user ? $object->watch_user->id : undef;
return unless $old_id == $new_id;
		 
$changes->{watch_user} = [ $old_id, $new_id ];

::: extensions/ComponentWatching/template/en/default/account/prefs/component_watch.html.tmpl
@@ +107,5 @@
>    </td>
> +  <td valign="top">
> +    <div id="watch-user-div"
> +         title="You can also watch a component by following this user. [% ~%]
> +                CC'ing this user on a bug will trigger notifications to all watchers of this component."

CC'ing this user on a [% terms.bug %] will trigger notifications to all watchers of this component."

Comment 7

[David Lawrence [:dkl]]

Comment on attachment 559053 [details] [diff] [review]
patch v2

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

Functionally this works for me based on my testing. r- for now because of the reports/components.html.tmpl and the needed foreign key.

dkl

::: extensions/ComponentWatching/Extension.pm
@@ +74,5 @@
>  }
>  
> +sub install_update_db {
> +    my $dbh = Bugzilla->dbh;
> +    $dbh->bz_add_column('components', 'watch_user', {TYPE => 'INT3'});

Need foreign key:

REFERENCES => {TABLE  => 'profiles',
               COLUMN => 'userid',
               DELETE => 'CASCADE'}},

@@ +94,5 @@
> +    my ($self, $args) = @_;
> +    my $file = $args->{file};
> +    return unless $file eq 'reports/components.html.tmpl';
> +
> +    $args->{file} = 'reports/components_ex.html.tmpl';

Do not see this file in the current patch. Also if you are redirecting to another file, why not just have an extensions/ComponentWatch/template/en/default/reports/components.html.tmpl which will be loaded instead of the standard one? I thought you were doing this before.

Ideally I would prefer hook(s) to be added to the standard components.html.tmpl that adds the data we need. That way we get any upstream changes to the standard template automatically.

Comment 8

[David Lawrence [:dkl]]

(In reply to David Lawrence [:dkl] from comment #7)
> > +    my ($self, $args) = @_;
> > +    my $file = $args->{file};
> > +    return unless $file eq 'reports/components.html.tmpl';
> > +
> > +    $args->{file} = 'reports/components_ex.html.tmpl';
> 
> Do not see this file in the current patch. Also if you are redirecting to
> another file, why not just have an
> extensions/ComponentWatch/template/en/default/reports/components.html.tmpl
> which will be loaded instead of the standard one? I thought you were doing
> this before.
> 
> Ideally I would prefer hook(s) to be added to the standard
> components.html.tmpl that adds the data we need. That way we get any
> upstream changes to the standard template automatically.

Ignore all of this. This is why I shouldn't let a lot of time pass after the initial review request that I forgot that you mentioned something about this in your comment. Your comment stated that you were just going to display the watch user on the component watch preferences tab. Which is fine by me. In that case you can omit this hook altogether, right?

dkl

Comment 9

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #6)
> > +    my $old_id = $old_object->watch_user ? $old_object->watch_user->id : 0;
> > +    my $new_id = $object->watch_user ? $object->watch_user->id : 0;
> > +    return unless $old_id == $new_id;
> > +
> > +    $changes->{watch_user} = [ $old_id ? $old_id : undef, $new_id ? $new_id : undef ];
> 
> This can be shortened it to:
> 
> my $old_id = $old_object->watch_user ? $old_object->watch_user->id : undef;
> my $new_id = $object->watch_user ? $object->watch_user->id : undef;
> return unless $old_id == $new_id;

unfortunately we can't because that comparison will trigger "Use of uninitialized value" warnings if either id is undef.

Comment 10

[:glob ✱]

Attachment: patch v3

updated patch, with FK and without reports hook.

Comment 11

[David Lawrence [:dkl]]

When running checksetup.pl on a fresh database, I get the following warning:

...
Creating initial dummy product 'TestProduct'...
Use of uninitialized value $value in string eq at ./extensions/ComponentWatching/Extension.pm line 177, <STDIN> line 4.
...

Do you see that as well or maybe something with my setup?

dkl

Comment 12

[David Lawrence [:dkl]]

Comment on attachment 561488 [details] [diff] [review]
patch v3

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

Looks good and works expected, even the foreign key. Can fixed the uninitialized value error on checkin. r=dkl

Comment 13

[:glob ✱]

Attachment: migrate_qa_to_component_watch.pl

this script updates the components configuration and moves all .bugs qa watchers to user watchers.

Comment 14

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #13)
> Created attachment 561989 [details]
> migrate_qa_to_component_watch.pl
> 
> this script updates the components configuration and moves all .bugs qa
> watchers to user watchers.

Probably doesnt even matter but it generates this error when you run the script and it is going to move 0 component watch users:

Number of components to update: 0

Press <Ctrl-C> to stop or <Enter> to continue...

Updating...
DBD::mysql::db do failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ')' at line 3 [for Statement "    UPDATE components
       SET watch_user=initialqacontact
     WHERE id IN ()
"] at extensions/ComponentWatching/migrate_qa_to_component_watch.pl line 43

But otherwise worked well for moving 902 watch users previously.

I assume we are not going to move the current qa contact users on current bug reports containing *.bugs to the cc list for now, right? I suppose it is ok to just leave old bugs alone and educate the users that if they clear out the qa contact field on old bugs they should move the current watch user to the cc list. Or we 
could go ahead and do the migration for them now. Thoughts?

dkl

Comment 15

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #14)
> I assume we are not going to move the current qa contact users on current
> bug reports containing *.bugs to the cc list for now, right?

the migration plan is on bug 684088 comment 14.
we will be updating the qa contact on all bugs.

Comment 16

[:glob ✱]

Attachment: screenshot of updated prefs

Comment 17

[:glob ✱]

Attachment: screenshot of updated component admin

Comment 18

[:glob ✱]

Attachment: patch v4

fix rot and switched to mpl2; carrying forward r+

Comment 19

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bmo/4.0/
modified extensions/ComponentWatching/Config.pm
modified extensions/ComponentWatching/Extension.pm
missing extensions/ComponentWatching/reset-watch-preferences.pl
modified extensions/ComponentWatching/reset-watch-preferences.pl
modified extensions/ComponentWatching/template/en/default/account/prefs/component_watch.html.tmpl
added extensions/ComponentWatching/template/en/default/hook/admin
modified extensions/ComponentWatching/template/en/default/hook/account/prefs/email-relationships.html.tmpl
modified extensions/ComponentWatching/template/en/default/hook/account/prefs/prefs-tabs.html.tmpl
added extensions/ComponentWatching/template/en/default/hook/admin/components
added extensions/ComponentWatching/template/en/default/hook/admin/components/edit-common-rows.html.tmpl
added extensions/ComponentWatching/template/en/default/hook/admin/components/list-before_table.html.tmpl
added extensions/ComponentWatching/template/en/default/hook/global/messages-component_updated_fields.html.tmpl
modified extensions/ComponentWatching/template/en/default/hook/global/reason-descs-end.none.tmpl
added extensions/ComponentWatching/template/en/default/hook/global/user-error-errors.html.tmpl
modified t/008filter.t
Committed revision 8106.

Comment 20

[:glob ✱]

i've committed the final part of this code, which makes "watch user" mandatory.

Committing to: bzr+ssh://[e-mail]/bmo/4.0/
modified extensions/ComponentWatching/Extension.pm
modified extensions/ComponentWatching/template/en/default/hook/global/user-error-errors.html.tmpl
Committed revision 8230.

Committing to: bzr+ssh://[e-mail]/bmo/4.2/
modified extensions/ComponentWatching/Extension.pm
modified extensions/ComponentWatching/template/en/default/hook/global/user-error-errors.html.tmpl
Committed revision 8230.