1074586 - Add "Bugs of Interest" to the dashboard

Status: RESOLVED FIXED

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

Description

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

We need to collect the following types of events in a 'bug_interest' table:

e1) Alice adds Bob to the CC list of bug. Bob is now interested in that bug (Ref: Bug 1059417)
e2) Bob files new bugs or bugs moved to a component that Alice watches. Alice is now interested in those bugs. (Ref: Bug 1056445)
e3) Alice is involved with Bug $A, and Bug $B is added as a dependency of Bug $A. Alice is now interested in Bug $B, which she may or may not be involved with.

Except for the transitive relationship between bugs, the events that trigger
bug interest would also cause the bugs to show up in the "involved with and
never visited" query.  It is simpler to say "if a user becomes involved with a
bug, or a bug they are involved with establishes a relationship with another
bug, that bug is of interest".

Parts of Bugzilla that will need to interact with this feature:

p1) New table: bug_interest, defined as (id, bug_id, user_id, timestamp)
p2) Hooks after bug creation and bug update to record the events into bug_interest.
p3) A new search term will be added to Search.pm
p4) The MyDashboard extension will get a query that is the union of "Updated
   since last visit" and new query "interesting bugs" which will pull from the
   events in the bugs_interest table. This query will be referred to in this document as "all interesting bugs".
p5) A new webservice end point will be added, along the lines of BugUserLastVisit.
p6) This API could be read-only or read-write, an UI could be added for the user to arbitarily mark a bug as interesting.
    This is outside of the scope of this feature, however.
p7) There must be a way of clearing interest. Visiting the bug or using "Mark as Visited"
   (Bug 1023405) feature My Dashboard could be used for this, perhaps renamed as "Mark as Read".

Notes:

Making the distinction between "clear last visit" and "clear interest" is potentially confusing.
I suppose when the current query is "all interesting bugs" the "Mark as Read" button would do both.

I have a more advanced proposal, that would add a scoring system to bug_interest events. That will be outlined in separate bug to not bog down this one.

Comment 1

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

Ehsan, do the above points (e1, e2, and e3) meet your requirements for an extended "last visited"-type dashboard?

Comment 2

[(no longer active)]

e1 and e2 are fantastic, and I believe they are the last pieces that we need in order to be able to completely survive without reading any bugmail.

However, I'm not sure if e3 is what I would want.  I think in practice, in most cases I wouldn't be interested in getting bug $B on my list when it gets added as a dependency for $A.  Because $A gets modified during the dependency setting, it will already be on my list so I can CC myself on $B if desired.  However, if $A depends on $B and $B's resolution changes, I would want $A to appear in my list again because I may be interested to know that for example $A can now make progress since $B is fixed.  And it would be awesome if going to $A at this point would somehow make it obvious that the bug was changed because $B's resolution changes (right now it can be kind of difficult to know that for things that we don't display in the inline history fields on the bug page, such as the CC list changing, or a dependency getting resolved.

Another thing which would be nice to have (but with a lower priority) would be if there was a way for me to mark a bug so that it never shows up in my "bugs of interest" list.  That use case becomes more important when we fix e2, of course, since the user may watch components as a way to get notified about interesting bugs in that component, but they may not actually be interested in every single bug in that component.

Comment 3

[(no longer active)]

Oh, another thing that would be nice would be if we extended e2 to cover the case where a bug was filed in a component that I watch before the "bugs of interest" feature gets deployed.  As things stand right now, such bugs won't appear on the "changed since last visit" dashboard, and I think they will not appear on the "bugs of interest" dashboard either, so there would be no way of knowing about those bugs...

Comment 4

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

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #3)
> Oh, another thing that would be nice would be if we extended e2 to cover the
> case where a bug was filed in a component that I watch before the "bugs of
> interest" feature gets deployed.  As things stand right now, such bugs won't
> appear on the "changed since last visit" dashboard, and I think they will
> not appear on the "bugs of interest" dashboard either, so there would be no
> way of knowing about those bugs...

It is conceivable to build this information as a one-time custom script. No promises, but I'll consider this after the other goals are met.

Comment 5

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

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #2)
> [snip]
> However, I'm not sure if e3 is what I would want.  I think in practice, in
> most cases I wouldn't be interested in getting bug $B on my list when it
> gets added as a dependency for $A.  Because $A gets modified during the
> dependency setting, it will already be on my list so I can CC myself on $B
> if desired.  However, if $A depends on $B and $B's resolution changes, I
> would want $A to appear in my list again because I may be interested to know
> that for example $A can now make progress since $B is fixed.
Ah, I believe I misunderstood in the original bug for this. I have collapsed the bug description, and put the revised summary in the User Story field.

> And it would
> be awesome if going to $A at this point would somehow make it obvious that
> the bug was changed because $B's resolution changes (right now it can be
> kind of difficult to know that for things that we don't display in the
> inline history fields on the bug page, such as the CC list changing, or a
> dependency getting resolved.

I believe this is a separate concern, which can be addressed later.

> Another thing which would be nice to have (but with a lower priority) would
> be if there was a way for me to mark a bug so that it never shows up in my
> "bugs of interest" list.  That use case becomes more important when we fix
> e2, of course, since the user may watch components as a way to get notified
> about interesting bugs in that component, but they may not actually be
> interested in every single bug in that component.

Possibly this can be done if the advanced version of this feature is implemented: Bug 1074589.
I believe some sort of scoring system will eventually be required to sort through the deluge of bugs.

Comment 6

[(no longer active)]

This all looks great, Dylan.  Thanks!

Comment 7

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

Attachment: bug-1074586-feedback.patch

Working data colllection, Search term, and MyDashboard query

Comment 8

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

Attachment: bug-1074586-v1.patch

Bug Interest feature in for review.

Comment 9

[David Lawrence [:dkl]]

Comment on attachment 8500851 [details] [diff] [review]
bug-1074586-v1.patch

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

Watch you line lengths :)

Mostly works well. Just a few little things to revise and then quick r+.

::: extensions/MyDashboard/Extension.pm
@@ +69,5 @@
> +            modification_time => { TYPE    => 'DATETIME',
> +                                   NOTNULL => 1 }
> +        ],
> +        INDEXES => [
> +            bug_interest_idx         => { FIELDS => [qw(bug_id user_id)], },

TYPE => 'UNIQUE'

@@ +138,5 @@
> +                               WHERE product_id = ?
> +                                 AND (component_id = ?
> +                                  OR  component_id IS NULL
> +                                  OR  ? LIKE CONCAT(component_prefix, '%'))";
> +    my ($user_ids) = $dbh->selectcol_arrayref($query, undef, $self->product_id, $self->id, $self->name);

cache the results in case bug_end_of_update is called after bug_end_of_create one or more times for the same component

::: extensions/MyDashboard/lib/BugInterest.pm
@@ +35,5 @@
> +
> +sub id            { return $_[0]->{id}            }
> +sub bug_id        { return $_[0]->{bug_id}        }
> +sub user_id       { return $_[0]->{user_id}       }
> +sub modification_time { return $_[0]->{modification_time} }

nit: line up {

@@ +47,5 @@
> +        $interest->update();
> +        return $interest;
> +    }
> +    else {
> +        return $class->create({ user_id => $user_id, bug_id => $bug_id, modification_time => $timestamp });

nit:

return $class->create({
    user_id           => $user_id,
    bug_id            => $bug_id,
    modification_time => $timestamp
});

::: extensions/MyDashboard/lib/WebService.pm
@@ +134,5 @@
> +
> +    my $bug_ids = $params->{bug_ids}
> +        or ThrowCodeError('param_required', { function => 'MyDashboard.bug_interest_unmark', param => 'type' });
> +
> +    my @bug_ids = ref($bug_ids) ? @$bug_ids : ( $bug_ids );

s/type/bug_ids/

Also shorten to:

$params->{bug_ids}
    || ThrowCodeError('param_required', { function => 'MyDashboard.bug_interest_unmark', param => 'bug_ids' });
my @bug_ids = ref($params->{bug_ids}) ? @{ $params->{bug_ids} } : ( $params->{bug_ids} );

::: extensions/MyDashboard/web/styles/mydashboard.css
@@ +51,1 @@
>  

nit: remove blank line

::: js/bug.js
@@ +188,5 @@
> +                        console.log("failed to unmark interest: "
> +                            + res.responseText);
> +                },
> +            };
> +

nit: remove extra line

@@ +191,5 @@
> +            };
> +
> +            YAHOO.util.Connect.setDefaultPostHeader('application/json', true);
> +            YAHOO.util.Connect.asyncRequest('POST', 'jsonrpc.cgi', callbacks,
> +                args)

bring args back up to the line above

Comment 10

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

Attachment: bug-1074586-v2.patch

(In reply to David Lawrence [:dkl] from comment #9)
> Watch you line lengths :)

Currently, set to 120 based on previous conversations.
Possibly 100 is more consistent? I know the line length is longer than the 80 we have for upstream. I see the specific constructs that should be shorter, however. :)

> TYPE => 'UNIQUE'
Woops!

> @@ +138,5 @@
> > +                               WHERE product_id = ?
> > +                                 AND (component_id = ?
> > +                                  OR  component_id IS NULL
> > +                                  OR  ? LIKE CONCAT(component_prefix, '%'))";
> > +    my ($user_ids) = $dbh->selectcol_arrayref($query, undef, $self->product_id, $self->id, $self->name);
> 
> cache the results in case bug_end_of_update is called after
> bug_end_of_create one or more times for the same component

Added. I also removed the unused method above this. Maybe we'll need it later, but for right now it was bloat. :)

> ::: extensions/MyDashboard/lib/BugInterest.pm
fixed.

> 
> @@ +47,5 @@
> > +        $interest->update();
> ...
> nit:
fixed.

> 
> ::: extensions/MyDashboard/lib/WebService.pm
> s/type/bug_ids/
Fixed. 

> Also shorten to:

Fixed, but I prefer using postfix "unless" in this case (reads better)

> ::: extensions/MyDashboard/web/styles/mydashboard.css
> nit: remove blank line
Fixed.

> ::: js/bug.js
> @@ +188,5 @@
> > +                        console.log("failed to unmark interest: "
> > +                            + res.responseText);
> > +                },
> > +            };
> > +
> 
> nit: remove extra line
> bring args back up to the line above

Fixed, and the adjacent code too for consistency.

Comment 11

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

Attachment: bug-1074586-v2.1.patch

There was some noise relating to the double-encoding email bug; this version has been corrected.

Comment 12

[David Lawrence [:dkl]]

Comment on attachment 8502791 [details] [diff] [review]
bug-1074586-v2.1.patch

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

Can fix on commit. r=dkl

::: Bugzilla/Search.pm
@@ +359,5 @@
> +    bug_interest_ts => {
> +        _non_changed => \&_bug_interest_ts,
> +        _default     => \&_invalid_operator,
> +    },
> + 

nit: remove whitespace on commit

::: extensions/MyDashboard/Extension.pm
@@ +103,5 @@
> +    my $query = "SELECT user_id FROM component_watch
> +                               WHERE product_id = ?
> +                                 AND (component_id = ?
> +                                  OR  component_id IS NULL
> +                                  OR  ? LIKE CONCAT(component_prefix, '%'))";

nit:

my $query = "SELECT user_id FROM component_watch
              WHERE product_id = ?
		    AND (component_id = ?
		         OR component_id IS NULL
		         OR ? LIKE CONCAT(component_prefix, '%'))";

@@ +192,5 @@
> +    my %old_cc = map { $_->id => $_ } grep { defined } @{ $old_bug->cc_users };
> +    my @added = grep { not $old_cc{ $_->id } } grep { defined } @{ $bug->cc_users };
> +    foreach my $user (@added) {
> +        Bugzilla::Extension::MyDashboard::BugInterest->mark($user->id, $bug->id, $timestamp);
> +    }

Lets omit this if the changer is the one being added to the cc list.

::: extensions/MyDashboard/lib/WebService.pm
@@ +128,4 @@
>      return { result => { $type => $results }};
>  }
>  
> +sub bug_interest_unmark {

Sorry I missed before. Need a REST API method for this. Not sure if we should use PUT or DELETE for this. PUT is normally for updating a record and DELETE for totally removing a record. Technically we are removing the entries but I suppose in the abstract case we are merely updating our interesting bugs list.

sub rest_resources {
    return [
        qr{^/bug_interest_unmark$}, {
            PUT => {
                method => 'bug_interest_unmark'
            }
        }
    ];
}

Comment 13

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

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   1f84551..ccea670  master -> master

Comment 14

[Gregory Szorc [:gps]]

This commit broke our Dockerized BMO for ReviewBoard. It is now failing with errors like:

   $ $TESTDIR/testing/bugzilla.py create-bug TestProduct TestComponent bug5
+  Traceback (most recent call last):
+    File "/home/vagrant/version-control-tools/testing/bugzilla.py", line 96, in <module>
+      sys.exit(main(sys.argv[1:]))
+    File "/home/vagrant/version-control-tools/testing/bugzilla.py", line 37, in main
+      client.put(bug)
+    File "/home/vagrant/version-control-tools/pylib/Bugsy/bugsy/bugsy.py", line 109, in put
+      raise BugsyException(result['message'])
+  bugsy.bugsy.BugsyException: Message: DBD::mysql::db selectcol_arrayref failed: Table 'bugs.component_watch' doesn't exist [for Statement "SELECT user_id FROM component_watch
+                    WHERE product_id = ?
+                      AND (component_id = ?
+                           OR component_id IS NULL
+                           OR ? LIKE CONCAT(component_prefix, '%'))"] at /var/lib/bugzilla/bugzilla/extensions/MyDashboard/Extension.pm line 109.

Because the component watching feature doesn't work with a fresh database on the BMO code base (not sure if there is a bug on that), we have disabled the component watching feature in our Dockerized BMO.

Unfortunately, it appears this patch assumes the component watching feature is enabled. When run against an install without component watching, it fails.

I can pin our Dockerized BMO install to the commit before this patch as a stop-gap to get our tests to pass.

Potential solutions:

1) Change this patch so it doesn't require component watching (possibly while this patch is backed out)
2) Fix BMO code base to work with component watching from a fresh database

Comment 15

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

Backing out this commit. We either move it to the component watching component or make dependency on component watching optional.

That said, we really shouldn't have to disable component watching. If there isn't already a bug for that, one should be filed.

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   ccea670..2f5045a  master -> master

Comment 16

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

It is looking like dkl will have fixex the problems that require component watching to be disabled.
I'm resolving this bug (and re-applying the new feature) so that this gets out in the next push.
Additional discussion about ComponentWatching causing a clean install to fail can be directed to Bug 1082113, which is fixing this.

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   41e58e2..b348b6a  master -> master

Comment 17

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

Fixed typo / syntax error.

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   b348b6a..6d2defe  master -> master