634531 - add product/component watching extension to bmo

Status: RESOLVED FIXED

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

Description

[:glob ✱]

until bug 76794 has been resolved, we can implement very simple product and/or component watching functionality as an extension.

Comment 1

[:glob ✱]

Attachment: component watching extension, v1

Comment 2

[Reed Loden [:reed]]

Comment on attachment 512719 [details] [diff] [review]
component watching extension, v1

>+# The Original Code is the BMO Watching Ex Extension

"Ex Extension" ?

>+# The Initial Developer of the Original Code is Byron Jones

"the Mozilla Foundation"

>+sub end {
>+    my ($self, $args) = @_;
>+    use Data::Dumper;
>+    die Dumper($args);
>+}

This looks debuggish. Probably shouldn't be here. :)

Comment 3

[:glob ✱]

Attachment: component watching extension, v2

addresses issues from reed's drive-by

Comment 4

[David Lawrence [:dkl]]

Comment on attachment 512780 [details] [diff] [review]
component watching extension, v2

>=== added file 'extensions/ComponentWatching/Extension.pm'
>+#
>+# preferences
>+#
>+
>+sub user_preferences {
>+    my ($self, $args) = @_;
>+    my $tab = $args->{'current_tab'};
>+    return unless $tab eq 'componentwatch_tab';

nit-pick: s/componentwatch_tab/component_watch/

+# we have to monkeypatch $user->wants_bug_mail
+our $_wants_bug_mail;
+BEGIN {
+    no strict 'refs';
+    no warnings 'redefine';
+    $_wants_bug_mail = *Bugzilla::User::wants_bug_mail;
+    *Bugzilla::User::wants_bug_mail = *Bugzilla::Extension::ComponentWatching::componentWatch_wants_bug_mail;
+}
+
+sub componentWatch_wants_bug_mail {
+    my ($self, $bug_id, $relationship, $fieldDiffs, $comments, $dependencyText,
+        $changer, $bug_is_new) = @_;
+
+    if (+$relationship == REL_COMPONENT_WATCHER) {
+        return 1;
+    } else {
+        return &$_wants_bug_mail(@_);
+    }
+}

Would it be possible to just add some hooks in Bugzilla::User::wants_bug_mail and Bugzilla::User::wants_mail to accomplish this?
Seems it could be useful for others to have that ability. We can oush the hooks upstream once they are working.

>--- extensions/ComponentWatching/template/en/default/account/prefs/componentwatch_tab.html.tmpl	1970-01-01 00:00:00 +0000
>+++ extensions/ComponentWatching/template/en/default/account/prefs/componentwatch_tab.html.tmpl	2011-02-16 08:03:59 +0000

Nit: s/componentwatch_tab/component_watch/

>--- extensions/ComponentWatching/template/en/default/hook/account/prefs/prefs-tabs.html.tmpl	1970-01-01 00:00:00 +0000
>+++ extensions/ComponentWatching/template/en/default/hook/account/prefs/prefs-tabs.html.tmpl	2011-02-16 08:04:24 +0000

>+[% tabs = tabs.import([{
>+    name => "componentwatch_tab",
>+    label => "Component/Product Watching",
>+    link => "userprefs.cgi?tab=componentwatch_tab",
>+    saveable => 1
>+    }]) %]

Same name change here. The extension is effectively about component watching and selecting a Product/__Any__ means watch *all* components. 
So using the word Product everywhere is not really necessary. 

Looks good so far.

Comment 5

[:glob ✱]

Attachment: component watching extension, v3

addresses review points

Comment 6

[:glob ✱]

dkl: can i get a review on this please?

Comment 7

[David Lawrence [:dkl]]

Comment on attachment 513034 [details] [diff] [review]
component watching extension, v3

>=== modified file 'Bugzilla/BugMail.pm'
>--- Bugzilla/BugMail.pm	2011-02-15 21:30:47 +0000
>+++ Bugzilla/BugMail.pm	2011-02-17 05:09:27 +0000
>@@ -400,6 +400,19 @@
>                     $rels_which_want{$relationship} = 
>                         $recipients{$user_id}->{$relationship};
>                 }
>+                # Upstreaming: TODO
>+                Bugzilla::Hook::process('bugmail_user_wants', {
>+                    relationship_mail => \%rels_which_want,
>+                    user => $user,
>+                    bug_id => $id,
>+                    relationship => $relationship,
>+                    field_diffs => $diffs,
>+                    comments => $comments,
>+                    dependency_text => $deptext,
>+                    changer => $changer,
>+                    bug_is_new => !$start
>+                });
>+

Refresh my memory. Would this be better suited in the end of Bugzilla::User::wants_bug_mail instead? 
Could be called user_wants_bug_mail then. Suppose technically works in either place but maybe would
cleaner.

>=== added file 'extensions/ComponentWatching/Extension.pm'

>+use Bugzilla::Util qw(diff_arrays html_quote);
>+use Bugzilla::Status qw(is_open_state);

Doesn't seem like any of these are used anywhere.

>+sub user_preferences {
>+    my ($self, $args) = @_;
>+    my $tab = $args->{'current_tab'};
>+    return unless $tab eq 'component_watch';
>+
>+    my $save = $args->{'save_changes'};
>+    my $handled = $args->{'handled'};
>+    my $user = Bugzilla->user;
>+
>+    if ($save) {
>+        my ($sth, $sthAdd, $sthDel);
>+
>+        if (Bugzilla->input_params->{'add'}) {
>+            # add watch
>+
>+            my $productName = Bugzilla->input_params->{'add_product'};
>+            my $componentName = Bugzilla->input_params->{'add_component'};
>+
>+            # load product and verify access
>+            my $product = Bugzilla::Product->new({ name => $productName });
>+            unless ($product && $user->can_access_product($product)) {
>+                ThrowUserError('product_access_denied', { product => $productName });
>+            }
>+
>+            my $component;
>+            if ($componentName) {
>+
>+                # watching a specific component
>+                $component = Bugzilla::Component->new({ name => $componentName, product => $product });
>+                unless ($component) {
>+                    ThrowUserError('product_access_denied', { product => $productName });
>+                }
>+                _addComponentWatch($user, $component);
>+
>+            } else {
>+                # watching a product
>+                _addProductWatch($user, $product);
>+            }
>+
>+        } else {
>+            # remove watch(s)
>+
>+            foreach my $name (keys %{Bugzilla->input_params}) {
>+                if ($name =~ /^del_(\d+)$/) {
>+                    _deleteProductWatch($user, $1);
>+                } elsif ($name =~ /^del_(\d+)_(\d+)$/) {
>+                    _deleteComponentWatch($user, $1, $2);
>+                }
>+            }
>+        }
>+    }
>+
>+    $args->{'vars'}->{'watches'} = _getWatches($user);
>+
>+    $$handled = 1;
>+}
>+
>+#
>+# bugmail
>+#
>+
>+sub bugmail_recipients {
>+    my ($self, $args) = @_;
>+    my $bug = $args->{'bug'};
>+    my $recipients = $args->{'recipients'};
>+    my $diffs = $args->{'diffs'};
>+
>+    my ($oldProductId, $newProductId) = ($bug->product_id, $bug->product_id);
>+    my ($oldComponentId, $newComponentId) = ($bug->component_id, $bug->component_id);
>+
>+    # notify when the product/component is switch from one being watched
>+    if (@$diffs) {
>+        # we need the product to process the component, so scan for that first
>+        my $product;
>+        foreach my $ra (@$diffs) {
>+            my (undef, undef, undef, undef, $old, $new, undef, $field) = @$ra;
>+            if ($field eq 'product') {
>+                $product = Bugzilla::Product->new({ name => $old });
>+                $oldProductId = $product->id;
>+            }
>+        }
>+        if (!$product) {
>+            $product = Bugzilla::Product->new($oldProductId);
>+        }
>+        foreach my $ra (@$diffs) {
>+            my (undef, undef, undef, undef, $old, $new, undef, $field) = @$ra;
>+            if ($field eq 'component') {
>+                my $component = Bugzilla::Component->new({ name => $old, product => $product });
>+                $oldComponentId = $component->id;
>+            }
>+        }
>+    }
>+
>+    my $dbh = Bugzilla->dbh;
>+    my $sth = $dbh->prepare("
>+        SELECT user_id
>+          FROM component_watch
>+         WHERE ((product_id = ? OR product_id = ?) AND component_id IS NULL)
>+               OR (component_id = ? OR component_id = ?)
>+    ");
>+    $sth->execute($oldProductId, $newProductId, $oldComponentId, $newComponentId);
>+    while (my ($uid) = $sth->fetchrow_array) {
>+        if (!exists $recipients->{$uid}) {
>+            $recipients->{$uid}->{+REL_COMPONENT_WATCHER} = 1;
>+        }
>+    }
>+}
>+
>+sub bugmail_user_wants {
>+    my ($self, $args) = @_;
>+    my $relationship = $args->{'relationship'};
>+
>+    if (+$relationship == REL_COMPONENT_WATCHER) {
>+        $args->{'relationship_mail'}{$relationship} = 1;
>+    }
>+}
>+
>+sub bugmail_relationships {
>+    my ($self, $args) = @_;
>+    my $relationships = $args->{relationships};
>+
>+    $relationships->{+REL_COMPONENT_WATCHER} = 'Component-Watcher';
>+}
>+
>+#
>+# db
>+#
>+
>+sub _getWatches {
>+    my ($user) = @_;
>+    my $dbh = Bugzilla->dbh;
>+
>+    my $sth = $dbh->prepare("
>+        SELECT product_id, component_id 
>+          FROM component_watch
>+         WHERE user_id = ?
>+    ");
>+    $sth->execute($user->id);
>+    my @watches;
>+    while (my ($productId, $componentId) = $sth->fetchrow_array) {
>+        my $product = Bugzilla::Product->new($productId);
>+        next unless $product && $user->can_access_product($product);
>+
>+        my %watch = ( product => $product );
>+        if ($componentId) {
>+            my $component = Bugzilla::Component->new($componentId);
>+            next unless $component;
>+            $watch{'component'} = $component;
>+        }
>+
>+        push @watches, \%watch;
>+    }
>+
>+    @watches = sort { 
>+        $a->{'product'}->name cmp $b->{'product'}->name
>+        || $a->{'component'}->name cmp $b->{'component'}->name
>+    } @watches;
>+
>+    return \@watches;
>+}
>+
>+sub _addProductWatch {
>+    my ($user, $product) = @_;
>+    my $dbh = Bugzilla->dbh;
>+
>+    my $sth = $dbh->prepare("
>+        SELECT 1 
>+          FROM component_watch
>+         WHERE user_id = ? AND product_id = ? AND component_id IS NULL
>+    ");
>+    $sth->execute($user->id, $product->id);
>+    return if $sth->fetchrow_array;
>+
>+    $sth = $dbh->prepare("
>+        DELETE FROM component_watch
>+              WHERE user_id = ? AND product_id = ?
>+    ");
>+    $sth->execute($user->id, $product->id);
>+
>+    $sth = $dbh->prepare("
>+        INSERT INTO component_watch(user_id, product_id)
>+             VALUES (?, ?)
>+    ");
>+    $sth->execute($user->id, $product->id);
>+}
>+
>+sub _addComponentWatch {
>+    my ($user, $component) = @_;
>+    my $dbh = Bugzilla->dbh;
>+
>+    my $sth = $dbh->prepare("
>+        SELECT 1 
>+          FROM component_watch
>+         WHERE user_id = ?
>+               AND (component_id = ?  OR (product_id = ? AND component_id IS NULL))
>+    ");
>+    $sth->execute($user->id, $component->id, $component->product_id);
>+    return if $sth->fetchrow_array;
>+
>+    $sth = $dbh->prepare("
>+        INSERT INTO component_watch(user_id, product_id, component_id)
>+             VALUES (?, ?, ?)
>+    ");
>+    $sth->execute($user->id, $component->product_id, $component->id);
>+}
>+
>+sub _deleteProductWatch {
>+    my ($user, $productId) = @_;
>+    my $dbh = Bugzilla->dbh;
>+
>+    my $sth = $dbh->prepare("
>+        DELETE FROM component_watch
>+              WHERE user_id = ? AND product_id = ? AND component_id IS NULL
>+    ");
>+    $sth->execute($user->id, $productId);
>+}
>+
>+sub _deleteComponentWatch {
>+    my ($user, $productId, $componentId) = @_;
>+    my $dbh = Bugzilla->dbh;
>+
>+    my $sth = $dbh->prepare("
>+        DELETE FROM component_watch
>+              WHERE user_id = ? AND product_id = ? AND component_id = ?
>+    ");
>+    $sth->execute($user->id, $productId, $componentId);
>+}
>+
>+__PACKAGE__->NAME;


>=== added file 'extensions/ComponentWatching/template/en/default/account/prefs/component_watch.html.tmpl'

>+[% FOREACH prod = Bugzilla.user.get_selectable_products %]

Nit: Just use user.get_selectable_products

>+  cpts['[% n %]'] = [
>+    [%- FOREACH comp = prod.components %]'[% comp.name FILTER js %]'[% ", " UNLESS loop.last %] [%- END -%] ];
>+  [% n = n+1 %]
>+[% END %]
>+</script>
>+<script type="text/javascript"
>+ src="[% 'js/productform.js' FILTER mtime FILTER html %]">

Nit: indent src=

>+    <select name="add_product" id="product" onChange="onSelectProduct()">
>+    [% FOREACH product IN Bugzilla.user.get_selectable_products %]
>+      <option>[% product.name FILTER html %]</option>
>+    [% END %]

Same nit: user.get_selectable_products 

>+    <select name="add_component" id="component">
>+      <option value="">__Any__</option>
>+    [% FOREACH product IN Bugzilla.user.get_selectable_products %]
>+      [% FOREACH component IN product.components %]
>+        <option>[% component.name FILTER html %]</option>
>+      [% END %]

And here. Maybe do [% SET selectable_products = user.get_selectable_products %] at the top 
of them template and use that?

>+  <td>&nbsp;</td>
>+  <td><input type="submit" name="add" value="Add"></td>

Needs id="commit" or similar for later automated testing.

>+[% FOREACH watch IN watches %]
>+  [% IF (watch.component) %]
>+    <tr>
>+      <td><input type="checkbox" name="del_[% watch.product.id %]_[% watch.component.id %]" value="1"></td>
>+      <td>[% watch.component.product.name FILTER html %]</td>
>+      <td>[% watch.component.name FILTER html %]</td>
>+    </tr>
>+  [% ELSE %]
>+    <tr>
>+      <td><input type="checkbox" name="del_[% watch.product.id %]" value="1"></td>
>+      <td>[% watch.product.name FILTER html %]</td>
>+      <td>__Any__</td>
>+    </tr>
>+  [% END %]

Nit: place <tr> and </tr> outside of the [% IF ELSE END %]

Another comment I had that may be misleading is that in the email headers, I get the following output:

X-Bugzilla-Reason: Component-Watcher
X-Bugzilla-Watch-Reason: None

It may make more sense for it to be:

X-Bugzilla-Watch-Reason: Component-Watcher 

instead but looking at the code in BugMail.pm, it may not be that simple to implement. 

Otherwise looks good. Minor changes. 

Dave

Comment 8

[:glob ✱]

Attachment: component watching extension, v4

thanks dave.

i can't recall exactly why the hook ended up in BugMail instead of User; i suspect it organically ended up there as i worked through the issues.  it is cleaner in User, so i've moved it there as per your suggestion.

this revision fixes that, and your other review points.

Comment 9

[David Lawrence [:dkl]]

Comment on attachment 521107 [details] [diff] [review]
component watching extension, v4


>-  <td><input type="submit" name="add" value="Add"></td>
>+  <td><input type="submit" id="commit" name="add" value="Add"></td>

Nit: Change to id="add" instead of id="commit" and this is good to check in.

r=dkl

Comment 10

[:glob ✱]

bzr threw an error while committing, however it looks ok:

$ bzr commit --fixes mozilla:634531 -m 'Bug 634531: Add component watching extension'
Committing to: bzr+ssh://[e-mail]/bmo/4.0/
modified Bugzilla/BugMail.pm
modified Bugzilla/User.pm
modified extensions/ComponentWatching/Extension.pm
modified extensions/ComponentWatching/template/en/default/account/prefs/component_watch.html.tmpl
bzr: ERROR: Server sent an unexpected error: ('error', 'math range error')

$ bzr commit --fixes mozilla:634531 -m 'Bug 634531: Add component watching extension'
bzr: ERROR: Bound branch BzrBranch7(file:///cygdrive/c/dev/bugzilla/repo/bmo/4.0/) is out of date with master branch RemoteBranch(bzr+ssh://[e-mail]/bmo/4.0/).
To commit to master branch, run update and then commit.
You can also pass --local to commit to continue working disconnected.

$ bzr up
All changes applied successfully.
Updated to revision 7577 of branch bzr+ssh://[e-mail]/bmo/4.0

byron@Glob-Laptop ~/bugzilla/repo/bmo/4.0
$ bzr st

byron@Glob-Laptop ~/bugzilla/repo/bmo/4.0
$