489028 - Record last-visited time of bugs when logged in

Status: RESOLVED FIXED

(Bugzilla :: User Interface, enhancement, P3)

Description

[Michael Thomas (Mockodin)]

This is something I found useful at my office. When returning a search list also display if the user has viewed the most recent version of a bug.

Allows for quick visual checks of searches to see if there are any updates to a ticket that have been missed. In environments where a user might receive hundreds of email a day from bugzilla (like I do) many are ignored as they are cc email for issues that I'm just being "kept aware" of. Using a visual indicator allows me to view my list and quickly choose the ones I have not already looked at. Hyper Links color change on visited doesn't really achieve this as it is looking at if you have visited the url before.

We went as far as to flag the comments not previously view and make them bold when displayed as well as add a anchor to each bug so clicking the indicator would take the user to the latest comment.

Comment 1

[Frédéric Buclin]

This information could only be stored in cookies, IMO. Having it in the DB would make it to be huge. If we are going to have one cookie per bug, this would mean hundreds to thousands of cookies stored by the web browser. IMO not a good idea. You could simply display the "Last Changed" column. Then it's up to you to visit the bug or not.

Comment 2

[Michael Thomas (Mockodin)]

(In reply to comment #1)
> This information could only be stored in cookies, IMO. Having it in the DB
> would make it to be huge.

Not really. If every user visited every bug then you would yes have one record per user per bug. XUsers * YBugs. However usually you have a number of power users who view everything and a limited number of users who view only bug assigned to them or if CC'd. Even in a extreme case where all users view all bugs and you have 400,000 bugs you would use have a table with millions of lines but the amount of data still being small. The only data collected is:

bug_id
who
last_viewed

There is only data if a user has visited a bug and the same row is simply updated on subsequent visits so the row count will never exceed the XUsers * YBugs calculation.

Look up speed is not a real factor as the complexity of data is low and easily indexed.

>If we are going to have one cookie per bug, this
> would mean hundreds to thousands of cookies stored by the web browser.

Saving to Cookies defeats the purpose as moving between machines you lose the information, or clearing the cache, or as mentioned a lot of cookies. Bad idea.

> IMO not
> a good idea. You could simply display the "Last Changed" column. Then it's up
> to you to visit the bug or not.

Thought of that but for me my memory of dates and times sucks, I have no idea if looked at a bug yesterday at 5:13pm or if it was at 5:12pm and the bug has been updated.

Comment 3

[Michael Thomas (Mockodin)]

oh also given the nature of the data it could be set to purge after 6 months as well which would serve as a remind for old data as it would appear new. Optional in a crontab of course.

I would also want to treat is like classifications, make it optional for the administrator to turn the feature on or off.

Comment 4

[Dan Wierenga]

Attachment: Screenshot of the "Changed Since Last Viewed" functionality

Here's a screenshot of my issue list.  You can see how much easier it is to find the 5 issues with the yellow "changed since I've viewed it" triangle than trying to read the Changed date one-by-one for the entire list.

FYI this was a customization that we've had since way-back-when, and it's an absolute can't-live-without feature for our users.  They've had a taste of how much easier this makes their lives and they'll never go back.

As Michael said it's not as much data as you'd fear; we've got ~150K issues and ~3000 users, and only ~800K last_viewed records.  A safe and easy way to purge that table would be to delete any records for a user when that user's account is disabled.

Comment 5

[Michael Thomas (Mockodin)]

Frédéric,

The changes required for this are not extensive is this something that can be considered? As Dan mentioned this has become a can't-live-without feature for our users, obviously it can just be reimplemented as a customization on my side but standardization is always a good thing.

Comment 6

[Michael Thomas (Mockodin)]

Attachment: Patch v1

Patch add the following:
Adds new table: bugs_last_viewed
Adds uselastviewed option (on | off) to editparams.cgi?section=bugfields
   defaults as off

Patch adds a column to the far left of the buglist.cgi when uselastviewed is enabled. The newchange.png file is displayed when a bug has change since the logged in user has last viewed a bug. If the viewer is not logged in then no icon is displayed.

An entry is added to bugs_last_viewed during processing of showbug.cgi.
Only one entry per user per bug is possible. If there is an existing record then an update is executed.

bug_last_viewed consists of three columns:
 bug_id - Which bug the record is logged for
 who - who was the viewer that we are tracking for
 last_viewed - when the last view occurred

Comment 7

[Frédéric Buclin]

Do I need to say your patch contains unrelated code? ;)

Comment 8

[Michael Thomas (Mockodin)]

Attachment: newchange image file

file should be copied to base images dir

Comment 9

[Michael Thomas (Mockodin)]

re comment 7

stop you're scaring me lol

in all honesty : bugzilla/Bugzilla/Config/BugFields.pm MIGHT
looking at the patch I see a change that might be the result of something recently checked in that I don't have integrated on my side. Will review that shortly.

Comment 10

[Michael Thomas (Mockodin)]

re comment 7,  nm looks ok the patch contains changes from:
Revision : 1.6.2.1
Date : 2009/4/17 22:30:30
Author : 'mkanat%bugzilla.org'

attachment 382145 [details] [diff] [review] and attachment 382146 [details] should be good to go, baring any requested change. This patch is more simplified than my production environment. We went so far as to include formatting during autolinkification of bug comments based on the last_viewed information. Which might be nice to add later but is far less important than the bug list changes.

Comment 11

[Michael Thomas (Mockodin)]

Above ref to rev 1.6.2.1 corresponds to bug 482584

Comment 12

[Frédéric Buclin]

Comment on attachment 382145 [details] [diff] [review]
Patch v1

>Index: buglist.cgi
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v
>retrieving revision 1.394
>diff -u -p -r1.394 buglist.cgi
>--- buglist.cgi	6 Mar 2009 20:34:54 -0000	1.394
>+++ buglist.cgi	24 Apr 2009 22:10:34 -0000
>@@ -729,6 +729,8 @@ $columns->{assigned_to_realname}->{title
> $columns->{reporter_realname}->{title} = "Reporter";
> $columns->{qa_contact_realname}->{title} = "QA Contact";
> 
>+$columns->{last_viewed}->{name} = "CASE WHEN last_viewed IS NOT NULL THEN 1 END as last_viewed";
>+
> Bugzilla::Hook::process("buglist-columns", {'columns' => $columns} );
> 
> ################################################################################
>@@ -830,6 +832,12 @@ if (lsearch(\@displaycolumns, "percentag
> # Display columns are selected because otherwise we could not display them.
> push (@selectcolumns, @displaycolumns);
> 
>+# if using last_viewed, we also need to look in bugs_last_viewed
>+if (Bugzilla->params->{"uselastviewed"}) {
>+    push (@selectcolumns,"last_viewed");
>+    $vars->{'last_view_enabled'} = 1;
>+}
>+
> # If the user is editing multiple bugs, we also make sure to select the product
> # and status because the values of those fields determine what options the user
> # has for modifying the bugs.
>Index: show_bug.cgi
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/show_bug.cgi,v
>retrieving revision 1.57
>diff -u -p -r1.57 show_bug.cgi
>--- show_bug.cgi	5 Feb 2009 18:40:13 -0000	1.57
>+++ show_bug.cgi	20 Apr 2009 18:53:40 -0000
>@@ -74,6 +74,13 @@ if ($single) {
>             }
>         }
>     }
>+
>+    # if using last_viewed, we also need to look in bugs_last_viewed
>+    if (Bugzilla->params->{"uselastviewed"}) {
>+        my @bugid = map {$_->bug_id} grep {!$_->error} @bugs;
>+        Bugzilla::Bug->Update_Viewed(@bugid);
>+    }
>+
> } else {
>     foreach my $id ($cgi->param('id')) {
>         # Be kind enough and accept URLs of the form: id=1,2,3.
>Index: Bugzilla/Bug.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v
>retrieving revision 1.277
>diff -u -p -r1.277 Bug.pm
>--- Bugzilla/Bug.pm	6 Apr 2009 20:02:51 -0000	1.277
>+++ Bugzilla/Bug.pm	24 Apr 2009 22:36:26 -0000
>@@ -3615,6 +3615,29 @@ sub ValidateDependencies {
>     return %deps;
> }
> 
>+#
>+#
>+# Record Last Viewed
>+#
>+#
>+sub Update_Viewed {
>+    my $self = shift;
>+    my $bug_id = shift;
>+    my $user = Bugzilla->user;
>+
>+    my $sth = Bugzilla->dbh->prepare("SELECT 1 AS 'Exists' FROM bugs_last_viewed WHERE bug_id = ? and who = ?");
>+    $sth->execute($bug_id, $user->id);
>+    my $exists = $sth->fetchrow_hashref();
>+
>+    if ($exists){
>+        my $sth2 = Bugzilla->dbh->prepare("UPDATE bugs_last_viewed SET last_viewed = NOW() WHERE bug_id = ? and who = ?");
>+           $sth2->execute($bug_id, $user->id);
>+    } else {
>+        my $sth2 = Bugzilla->dbh->prepare("INSERT INTO bugs_last_viewed (bug_id, who, last_viewed) values ( ? , ? , NOW())");
>+           $sth2->execute($bug_id, $user->id);
>+    }
>+
>+}
> 
> #####################################################################
> # Autoloaded Accessors
>Index: Bugzilla/Search.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v
>retrieving revision 1.173
>diff -u -p -r1.173 Search.pm
>--- Bugzilla/Search.pm	2 Mar 2009 20:23:34 -0000	1.173
>+++ Bugzilla/Search.pm	18 Apr 2009 05:21:34 -0000
>@@ -155,6 +155,16 @@ sub init {
>                           "ON ldtime.bug_id = bugs.bug_id");
>     }
> 
>+    if (grep(/last_viewed/, @$fieldsref)) {
>+        if ($self->{'user'}->id){
>+            my $userid = $self->{'user'}->id;
>+            push @supptables, "LEFT JOIN bugs_last_viewed ON bugs.bug_id = bugs_last_viewed.bug_id "
>+                                ."AND bugs_last_viewed.who = $userid "
>+                                ."AND bugs_last_viewed.last_viewed >= bugs.delta_ts";
>+        }
>+    }
>+    
>+
>     my $minvotes;
>     if (defined $params->param('votes')) {
>         my $c = trim($params->param('votes'));
>Index: Bugzilla/Config/BugFields.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/BugFields.pm,v
>retrieving revision 1.6
>diff -u -p -r1.6 BugFields.pm
>--- Bugzilla/Config/BugFields.pm	10 Dec 2008 18:43:28 -0000	1.6
>+++ Bugzilla/Config/BugFields.pm	24 Apr 2009 22:37:12 -0000
>@@ -84,6 +84,12 @@ sub get_param_list {
>   },
> 
>   {
>+   name => 'use_see_also',
>+   type => 'b',
>+   default => 1
>+  },
>+
>+  {
>    name => 'defaultpriority',
>    type => 's',
>    choices => \@legal_priorities,
>@@ -113,7 +119,14 @@ sub get_param_list {
>    choices => ['', @legal_OS],
>    default => '',
>    checker => \&check_opsys
>-  } );
>+  },
>+
>+  {
>+   name => 'uselastviewed',
>+   type => 'b',
>+   default => 0
>+  }
>+  );
>   return @param_list;
> }
> 
>Index: Bugzilla/DB/Schema.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v
>retrieving revision 1.116
>diff -u -p -r1.116 Schema.pm
>--- Bugzilla/DB/Schema.pm	6 Apr 2009 20:57:23 -0000	1.116
>+++ Bugzilla/DB/Schema.pm	18 Apr 2009 04:43:46 -0000
>@@ -352,6 +352,21 @@ use constant ABSTRACT_SCHEMA => {
>             bugs_activity_fieldid_idx => ['fieldid'],
>         ],
>     },
>+    bugs_last_viewed  => {
>+        FIELDS => [
>+            bug_id    => {TYPE => 'INT3', NOTNULL => 1,
>+                          REFERENCES    =>  {TABLE  =>  'bugs',
>+                                             COLUMN =>  'bug_id',
>+                                             DELETE => 'CASCADE'}},
>+            who       => {TYPE => 'INT3', NOTNULL => 1,
>+                          REFERENCES => {TABLE  => 'profiles',
>+                                         COLUMN => 'userid'}},
>+            last_viewed  => {TYPE => 'DATETIME', NOTNULL => 1},
>+	],
>+	INDEXES => [
>+	    bugs_last_viewed_bug_id_who_idx  => ['bug_id', 'who'],
>+	],
>+    },
> 
>     cc => {
>         FIELDS => [
>Index: template/en/default/admin/params/bugfields.html.tmpl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/bugfields.html.tmpl,v
>retrieving revision 1.5
>diff -u -p -r1.5 bugfields.html.tmpl
>--- template/en/default/admin/params/bugfields.html.tmpl	10 Dec 2008 18:43:29 -0000	1.5
>+++ template/en/default/admin/params/bugfields.html.tmpl	18 Apr 2009 03:44:38 -0000
>@@ -56,5 +56,7 @@
>                   "entry form.<br> " _
>                   "You can leave this empty: " _
>                   "$terms.Bugzilla will then use the operating system that the browser " _
>-                  "reports to be running on as the default." }
>-%]
>\ No newline at end of file
>+                  "reports to be running on as the default.",
>+  uselastviewed => "Enables Users to see if a bug has changed since last viewed"
>+}
>+%]
>Index: template/en/default/list/table.html.tmpl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/list/table.html.tmpl,v
>retrieving revision 1.42
>diff -u -p -r1.42 table.html.tmpl
>--- template/en/default/list/table.html.tmpl	4 Jan 2009 17:44:52 -0000	1.42
>+++ template/en/default/list/table.html.tmpl	24 Apr 2009 22:43:44 -0000
>@@ -85,6 +85,7 @@
>       [% IF dotweak %]
>       <th>&nbsp;</th>
>       [% END %]
>+      [% IF last_view_enabled %]<th>&nbsp;</th>[% END %]
>       <th colspan="[% splitheader ? 2 : 1 %]" class="first-child">
>         [% desc = '' %]
>         [% IF (om = order.match("^bugs\.bug_id( desc)?")) %]
>@@ -180,6 +181,14 @@
>       <input type="checkbox" name="id_[% bug.bug_id %]">
>     </td>
>     [% END %]
>+    [% IF last_view_enabled %]
>+       [% IF bug.last_viewed %]
>+          <td><a href="show_bug.cgi?id=[% bug.bug_id %]#lv"><img src="images/newchange.gif" no border></a></td>
>+       [% ELSE %]
>+          <td>&nbsp;</td>
>+       [% END %]
>+    [% END %]
>+
>     <td class="first-child">
>       <a name="b[% bug.bug_id %]"
>          href="show_bug.cgi?id=[% bug.bug_id %]">[% bug.bug_id %]</a>

Comment 13

[Max Kanat-Alexander]

Comment on attachment 382145 [details] [diff] [review]
Patch v1

Wow, this is pretty cool.

Okay:
* I don't want a parameter--seems like that's solving a problem before we know it exists.

* You'll have to redo the patch now since we have a COLUMNS constant in Search.pm instead of a columns item in buglist.cgi.

* I need some sort of scaling test done on this to make sure that this isn't going to kill large installations.

And of course, there's lots of little code consistency and standards comments to make, like:

* Should be update_last_viewed, not Update_viewed.

* You should be doing my $dbh = Bugzilla->dbh instead of calling Bugzilla->dbh over and over.

* You need to not be updating last_viewed if there is no logged-in user.

* The last_viewed index should be UNIQUE.

* "no border" is not valid HTML.

Comment 14

[Max Kanat-Alexander]

Comment on attachment 382146 [details]
newchange image file

I'd like something a little more subtle than this. Perhaps even just a glyph if we can find one that's reliably available on all systems.

Comment 15

[Michael Thomas (Mockodin)]

(In reply to comment #13)
> (From update of attachment 382145 [details] [diff] [review])
> Wow, this is pretty cool.
> 
> Okay:
> * I don't want a parameter--seems like that's solving a problem before we know
> it exists.

Ok

> * You'll have to redo the patch now since we have a COLUMNS constant in
> Search.pm instead of a columns item in buglist.cgi.

Ok
 
> * I need some sort of scaling test done on this to make sure that this isn't
> going to kill large installations.

Running 200k bugs here, we don't see any noticeable impact against a 2.22 build of Bugzilla.

> And of course, there's lots of little code consistency and standards comments
> to make, like:
> 
> * Should be update_last_viewed, not Update_viewed.

Corrected

> * You should be doing my $dbh = Bugzilla->dbh instead of calling Bugzilla->dbh
> over and over.
> 
> * You need to not be updating last_viewed if there is no logged-in user.

Hmm, missed a line, shouldn't have been.

> * The last_viewed index should be UNIQUE.

Aye

> * "no border" is not valid HTML.

It's Microsoftish for Border="0"
(In reply to comment #14)
> (From update of attachment 382146 [details])
> I'd like something a little more subtle than this. Perhaps even just a glyph if
> we can find one that's reliably available on all systems.

Hmm how about an html entity symbol, » (») in red bold maybe double size?

Comment 16

[Ashley Bischoff (blog at handcoding.com)]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 382146 [details] [details])
> > I'd like something a little more subtle than this. Perhaps even just a glyph if
> > we can find one that's reliably available on all systems.
> 
> Hmm how about an html entity symbol, » (») in red bold maybe double size?

From an accessibility point of view, » might be a bit verbose as screen readers read that as "right double angle bracket". (http://www.standards-schmandards.com/2004/title-text-separators/)

If attachment 382146 [details] represents what you're generally looking for, Δ (Δ) may be an option.

Comment 17

[Michael Thomas (Mockodin)]

Δ works and is a close representation to what I was going for originally.

Comment 18

[Dan Wierenga]

The problem with Δ is that it's "hollow" in the middle, making it far less visible and attention-grabbing.  Considering that's the entire point of the glyph, that's a big drawback.

How about something like:
★ (★) BLACK STAR 
◆ (◆) BLACK DIAMOND

which are solid shapes and hence far more visible, with the Unicode names still being rather reasonable, IMO, for a screen reader to rattle off.

Comment 19

[Michael Thomas (Mockodin)]

Attachment: v2

Updated for current CVS Tip and applied suggestions to date

Went with ★ (★) BLACK STAR however css will make it red

Comment 20

[Frédéric Buclin]

Comment on attachment 409021 [details] [diff] [review]
v2

>Index: bugzilla/show_bug.cgi

>+    Bugzilla::Bug->update_last_viewed($id) if $user->id;

Why doing this only when viewing one bug at once? Why not also doing this when viewing multiple bugs at once? Is that intentional?

Also, as the bug object is created a few lines above this one, you should use it to call $bug->update_...



>Index: bugzilla/Bugzilla/Bug.pm

>+sub update_last_viewed {
>+    my $self = shift;
>+    my $bug_id = shift;
>+    trick_taint($bug_id);

Why my suggestion above, the bug ID would already be available as $self->id (no need to revalidate it, and no risk to pass an invalid integer).


>+    my $sth = $dbh->prepare("SELECT 1 AS 'Exists' FROM bugs_last_viewed WHERE bug_id = ? and who = ?");
>+    $sth->execute($bug_id, $user->id);
>+    my $exists = $sth->fetchrow_hashref();

Do this all at once, using:
 my $exists = $dbh->selectrow_array('SELECT 1 FROM bugs_... WHERE ...');

Also, use user_id as a column name rather than "who". "who" doesn't tell us if it's a user ID or user login name or what.


>+        my $sth2 = $dbh->prepare("UPDATE bugs_last_viewed SET last_viewed = NOW() WHERE bug_id = ? and who = ?");
>+           $sth2->execute($bug_id, $user->id);

Do it all at once too, using $dbh->do(...).


>+        my $sth2 = $dbh->prepare("INSERT INTO bugs_last_viewed (bug_id, who, last_viewed) values ( ? , ? , NOW())");
>+           $sth2->execute($bug_id, $user->id);

Same here.



>Index: bugzilla/Bugzilla/Search.pm

>+    $columns{'last_viewed'} = { name => "bugs_last_viewed.last_viewed",
>+	                        title => "Last Viewed" };

That's not the right place for this. Look at %columns earlier in the code.


>+        push(@supptables, "LEFT JOIN bugs_last_viewed "
>+		          ."ON bugs_last_viewed.bug_id = bugs.bug_id AND who = " . $user->id);

Use a placeholder for $user->id.



>Index: bugzilla/Bugzilla/DB/Schema.pm

>+            who       => {TYPE => 'INT3', NOTNULL => 1,
>+                          REFERENCES => {TABLE  => 'profiles',
>+                                         COLUMN => 'userid'}},

As said, s/who/user_id/. Also, on DELETE CASCADE.



>Index: bugzilla/template/en/default/list/table.html.tmpl

>+    <td><a class="bug_updated_alert" href="show_bug.cgi?id=[% bug.bug_id %]">&#9733;</a></td>

It shouldn't be a link. Only the bug ID is clickable. Also, I didn't apply your patch, but I'm pretty sure that editing the column list has no name for this one.


Do you have a screenshot of how it looks like?

Comment 21

[Michael Thomas (Mockodin)]

(In reply to comment #20)
> Why doing this only when viewing one bug at once? Why not also doing this when
> viewing multiple bugs at once? Is that intentional?

It was intentional. But in reflection..

> >+        push(@supptables, "LEFT JOIN bugs_last_viewed "
> >+		          ."ON bugs_last_viewed.bug_id = bugs.bug_id AND who = " . $user->id);
> 
> Use a placeholder for $user->id.
 
How? Also this is consistent with usages of $user->id through Search.pm
 
> >Index: bugzilla/Bugzilla/DB/Schema.pm
> 
> >+            who       => {TYPE => 'INT3', NOTNULL => 1,
> >+                          REFERENCES => {TABLE  => 'profiles',
> >+                                         COLUMN => 'userid'}},
> 
> As said, s/who/user_id/. Also, on DELETE CASCADE.

Hmm, apparently my home workstation had a old version of that file, DELETE CASCADE should and does exist, I'll post an update for the field and FKey.
 
> >Index: bugzilla/template/en/default/list/table.html.tmpl
> 
> >+    <td><a class="bug_updated_alert" href="show_bug.cgi?id=[% bug.bug_id %]">&#9733;</a></td>
> 
> It shouldn't be a link. Only the bug ID is clickable.

I prefer clickable personally, but is there a particular reason we only want one clickable link?

> Also, I didn't apply your
> patch, but I'm pretty sure that editing the column list has no name for this
> one.

I was treating it similar to bug_id, always shown. Also I didn't want the column sortable. It is for display only, is it possible to add it to the selectable columns list without allowing it to be sorted by?

Comment 22

[Michael Thomas (Mockodin)]

Attachment: Updated Screenshot

Comment 23

[Michael Thomas (Mockodin)]

Attachment: v3

Updated for comment 20

Except for:
1. Use a placeholder for $user->id.
   -- This isn't being done else where in Search.pm for $user->id
2. editing the column list has no name for this one
   Two Reasons
     1: I don't want it sortable, though this is more due to the next issue.
     2: I want the column to appear before bug_id which the editable column
        list does not allow for.

Comment 24

[Frédéric Buclin]

(In reply to comment #23)
>      1: I don't want it sortable, though this is more due to the next issue.
>      2: I want the column to appear before bug_id which the editable column
>         list does not allow for.

I certainly do want it sortable. I don't want a magic column having its own rules.

Comment 25

[Michael Thomas (Mockodin)]

Can you suggest a manner in which to allow it to be placed before the bug_id then?

As I said the issue was more the column order.

Comment 26

[Michael Thomas (Mockodin)]

Attachment: v3

updated index name and removed some phantom code which shouldn't have been in this patch

Comment 27

[Michael Thomas (Mockodin)]

Attachment: v4

Take 4? Patch file should be ok now.

Comment 28

[Michael Thomas (Mockodin)]

MKanat/LpSolit

I found bug in my testing, the code relies on comparing changeddate and last_viewed, the problem being changeddate only is displayed when the 'Changed' column is displayed, which I had while testing originally. I have two choices... three actually.

Option 1, I can add changeddate to the field list in bug list, same manner as adding bug_id causing it to always appear. Or Option 2 I can add it to Search.pm where I add the joins for the 'bugs_last_viewed' table and add a push(@fields,... just have to check for preexistence there if we are already displaying the changeddate column. Or option 3 always include changedate but alias it under a different column name for use with last_viewed.

Thoughts?

Comment 29

[Frédéric Buclin]

(In reply to comment #28)
> Thoughts?

If you need some data, just add it to the list of columns in buglist.cgi. This doesn't mean it has to be displayed.

Comment 30

[Max Kanat-Alexander]

  You're going to want the on_main_db function from bug 24896. (I'll explain in my review.)

Comment 31

[Max Kanat-Alexander]

Comment on attachment 410107 [details] [diff] [review]
v4

  This is pretty well-written. :-)

>+    $bug->update_last_viewed($user->id) if $user->id;

  I think it would be best to pass the whole $user object.

  Also, I think we should limit this to just the HTML version of show_bug.cgi, perhaps? Not sure about that.

>@@ -80,6 +82,7 @@
>         my @ids = split(/,/, $id);
>         foreach (@ids) {
>             my $bug = new Bugzilla::Bug($_);
>+	    $bug->update_last_viewed($user->id) if $user->id;

  Nit: There's a tab in this line. Also, I think we might want to skip this for the multiple-bug view, although I'm not totally certain.

>Index: bugzilla/Bugzilla/Bug.pm
>+#
>+# Record Last Viewed
>+#

  Nit: Usually our section headers look more like:

######################
# Record Last Viewed #
######################

>+sub update_last_viewed {
>+    my $self = shift;
>+    my $user_id = shift;
>+    return unless $user_id;

  If you have this here, you don't have to check $user->id in show_bug.cgi.

>+    my $dbh = Bugzilla->dbh;
>+    my $exists = $dbh->selectrow_array("SELECT 1 AS 'Exists' FROM bugs_last_viewed WHERE bug_id = ? and user_id = ?",

  You don't need that AS.

  Also, nit: this line is too long.

  Also, let's make it bug_last_viewed.

>+    if ($exists){
>+        $dbh-do("UPDATE bugs_last_viewed SET last_viewed = NOW() WHERE bug_id = ? and user_id = ?",

  Nit: Long line, capitalize "and".

  Also, use LOCALTIMESTAMP(0) instead of NOW().

>+    } else {
>+        $dbh->do("INSERT INTO bugs_last_viewed (bug_id, user_id, last_viewed) values ( ? , ? , NOW())",

  Nit: Long line.

  Use LOCALTIMESTAMP(0) instead of NOW().

>Index: bugzilla/Bugzilla/Search.pm
>+        last_viewed          => { title => 'Last Viewed', name => 'bugs_last_viewed.last_viewed'},

  Nit: Long line.

  Should this be in fielddefs instead? Then we might also have to add Search.pm code for it, right? So it can live here for now, but it should have an XXX comment about being moved into fielddefs.

  Also, it should be called 'last_viewed' instead of bugs_last_viewed.last_viewed.

>+    if (grep($_ eq 'last_viewed', @fields)) {
>+        push(@supptables, "LEFT JOIN bugs_last_viewed "
>+		          ."ON bugs_last_viewed.bug_id = bugs.bug_id AND who = " . $user->id);

  Tabs.

  Also, the line is too long.

  Also, it should be called map_bugs_last_viewed, since that's what we call the other ones.

>Index: bugzilla/Bugzilla/DB/Schema.pm
>+            last_viewed  => {TYPE => 'DATETIME', NOTNULL => 1},
>+	],
>+	INDEXES => [
>+            bugs_last_viewed_last_viewed_idx => { FIELDS => ['last_viewed', 'user_id', 'bug_id'],
>+            TYPE => 'UNIQUE'},
>+            bugs_last_viewed_user_id_idx     => { FIELDS => ['user_id', 'bug_id', 'last_viewed'],
>+            TYPE => 'UNIQUE'},

  You don't need two unique indexes. If you're just searching by user_id, then just have an index with user_id. Having an index the size of the whole table actually isn't useful for anything but a constraint, really, because the optimizer would probably just choose to scan the table, since the index is the size of the table.


  I have some serious concerns about the growth of this table. For example, on bmo, with 500,000 bugs and over 100,000 users, the table could grow to be 50 *billion* rows. The performance and backup impact of that would be unmanageable. So what's your idea for handling that problem?

Comment 32

[Michael Thomas (Mockodin)]

(In reply to comment #31)
> (From update of attachment 410107 [details] [diff] [review])
>   This is pretty well-written. :-)
> 
> >+    $bug->update_last_viewed($user->id) if $user->id;
> 
>   I think it would be best to pass the whole $user object.

Always seem broken to me to move entire objects to get one field but sure.

>   Also, I think we should limit this to just the HTML version of show_bug.cgi,
> perhaps? Not sure about that.

I guess my response would be why? Is there a pressing need to not allow for it.

> >@@ -80,6 +82,7 @@
> >         my @ids = split(/,/, $id);
> >         foreach (@ids) {
> >             my $bug = new Bugzilla::Bug($_);
> >+	    $bug->update_last_viewed($user->id) if $user->id;
> 
>   Nit: There's a tab in this line. Also, I think we might want to skip this for
> the multiple-bug view, although I'm not totally certain.
> 
> >Index: bugzilla/Bugzilla/Bug.pm
> >+#
> >+# Record Last Viewed
> >+#
> 
>   Nit: Usually our section headers look more like:
> 
> ######################
> # Record Last Viewed #
> ######################
> 
> >+sub update_last_viewed {
> >+    my $self = shift;
> >+    my $user_id = shift;
> >+    return unless $user_id;
> 
>   If you have this here, you don't have to check $user->id in show_bug.cgi.
Hmm, I think I saw that and forgot to fix it.

> >+    my $dbh = Bugzilla->dbh;
> >+    my $exists = $dbh->selectrow_array("SELECT 1 AS 'Exists' FROM bugs_last_viewed WHERE bug_id = ? and user_id = ?",
> 
>   You don't need that AS.
Yes
>   Also, nit: this line is too long.
80 yes?
>   Also, let's make it bug_last_viewed.
Ok
> >+    if ($exists){
> >+        $dbh-do("UPDATE bugs_last_viewed SET last_viewed = NOW() WHERE bug_id = ? and user_id = ?",
> 
>   Nit: Long line, capitalize "and".
OK
>   Also, use LOCALTIMESTAMP(0) instead of NOW().
OK
> >+    } else {
> >+        $dbh->do("INSERT INTO bugs_last_viewed (bug_id, user_id, last_viewed) values ( ? , ? , NOW())",
> 
>   Nit: Long line.
OK
>   Use LOCALTIMESTAMP(0) instead of NOW().
OK
> >Index: bugzilla/Bugzilla/Search.pm
> >+        last_viewed          => { title => 'Last Viewed', name => 'bugs_last_viewed.last_viewed'},
> 
>   Nit: Long line.
OK
>   Should this be in fielddefs instead? Then we might also have to add Search.pm
> code for it, right? So it can live here for now, but it should have an XXX
> comment about being moved into fielddefs.
OK
>   Also, it should be called 'last_viewed' instead of
> bugs_last_viewed.last_viewed.
Didn't you just say 'bug_last_viewed' and last_viewed.last_viewed is confusing to look at.

> >+    if (grep($_ eq 'last_viewed', @fields)) {
> >+        push(@supptables, "LEFT JOIN bugs_last_viewed "
> >+		          ."ON bugs_last_viewed.bug_id = bugs.bug_id AND who = " . $user->id);
> 
>   Tabs.
OK
>   Also, the line is too long.
OK
>   Also, it should be called map_bugs_last_viewed, since that's what we call the
> other ones.
OK
> >Index: bugzilla/Bugzilla/DB/Schema.pm
> >+            last_viewed  => {TYPE => 'DATETIME', NOTNULL => 1},
> >+	],
> >+	INDEXES => [
> >+            bugs_last_viewed_last_viewed_idx => { FIELDS => ['last_viewed', 'user_id', 'bug_id'],
> >+            TYPE => 'UNIQUE'},
> >+            bugs_last_viewed_user_id_idx     => { FIELDS => ['user_id', 'bug_id', 'last_viewed'],
> >+            TYPE => 'UNIQUE'},
> 
>   You don't need two unique indexes. If you're just searching by user_id, then
> just have an index with user_id. Having an index the size of the whole table
> actually isn't useful for anything but a constraint, really, because the
> optimizer would probably just choose to scan the table, since the index is the
> size of the table.

Was intended as a constraint, I suppose we could use limit 1 on queries... or something.
> 
>   I have some serious concerns about the growth of this table. For example, on
> bmo, with 500,000 bugs and over 100,000 users, the table could grow to be 50
> *billion* rows. The performance and backup impact of that would be
> unmanageable. So what's your idea for handling that problem?

If we add a timestamp they could expire I guess. The issue you mentioned I think I commented on somewhere. Using your example as a worse case all 100k users viewed all 500k bugs at some point to hit 50 billion. Remember this is set when viewing a ticket, not viewing a bug list so it is highly unlikely. Pruning could be used, else partitioning by userid could be used. One somewhat assumes that if you have a 100k user base you have real DBAs on staff to address this. Also large companies, commonly archive or purge, in the US 3 or 7 years being common 'OK to purge point'. We're at 1.3Million record here on this table with no impact, granted a vast difference between 1.3M and 50B.

Comment 33

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.

Comment 35

[:glob ✱]

from bug 957234:
> In order to not slow down bug-load times by adding a database write, this should be done via AJAX
> after the page is loaded rather than during show_bug.


regarding the size of the table itself, we could use the user's last login date to drive the pruning and remove rows for users who haven't logged in to bugzilla for some time (eg. 6 months).

this data also needs to be exposed via the api.

Comment 36

[David Lawrence [:dkl]]

Attachment: 957234_1.patch

This was some work I did on bug 957234 (duped to this one) initially a couple weeks back. Just have the schema and webservices side done so far. Did this before I knew this bug existed but was working towards the ajax route of updating the DB using a background call from the client. I can work on this some more once our other goals are completed but I just wanted to show the patch I had so far.

dkl

Comment 37

[Mark Côté [:mcote]]

Reassigning to dkl, for now at least.

Comment 38

[Mark Côté [:mcote]]

I think this is something else dylan can work on in parallel with the review-tracking stuff. Assigning over. Dylan, please talk to dkl if you need some context over his work in progress.

Comment 39

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

Attachment: bz-replace-into.sql

This is a sketch of how I'm planning on handling the REPLACE INTO logic when using PostgreSQL. A plpgsql function is required in order to do the UPDATE, if not found INSERT logic.

Comment 40

[Frédéric Buclin]

(In reply to Dylan William Hardison [:dylan] from comment #39)
> This is a sketch of how I'm planning on handling the REPLACE INTO logic when
> using PostgreSQL.

You would need a similar procedure for Oracle, as it doesn't understand REPLACE INTO either, AFAIK. Maybe it's safer to use the traditionnal way?

if (SELECT ..) {
  UPDATE
else {
  INSERT
}

Comment 41

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

The syntax for Oracle uses MERGE INTO table USING (query) WHEN MATCH / WHEN NOT MATCH and has better gurantees of consistency.

Comment 42

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

The chosen answer on stack overflow gives favor to MERGE INTO: http://stackoverflow.com/questions/12274156/oracle-merge-vs-select-then-insert-or-update

Comment 43

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

Attachment: bug-489028-WIP-1-webservice-bits.diff

I have fixed the bit rot and un-extensionified the patch :dkl originally wrote. I have also taken a stab at abstracting the REPLACE INTO it was using. It's still a work in progress for a few reasons:

1) the client side code has to send the Bugzilla_login and Bugzilla_password parameters because cookie auth is forbidden over GET requests?

2) I'm not 100% sure my table-level lock and MERGE INTO syntax for oracle is correct. I have access to an oracle 11g environment and will test it tomorrow.

Comment 44

[:glob ✱]

Comment on attachment 8389619 [details] [diff] [review]
bug-489028-WIP-1-webservice-bits.diff

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

looks like a good start :)

::: Bugzilla/User.pm
@@ +735,5 @@
> +
> +        $self->{bugs_last_visited} = { map { ($_->{id}, $_->{last_visited}) } @$bugs_last_visited };
> +    }
> +
> +    return $self->{bugs_last_visited};

i don't see the benefit of caching this data in the user object.  it's mostly likely to be a rather large list, and it's unlikely to be called more than once per request.

::: Bugzilla/WebService/Server/REST/Resources/LastVisited.pm
@@ +4,5 @@
> +#
> +# This Source Code Form is "Incompatible With Secondary Licenses", as
> +# defined by the Mozilla Public License, v. 2.0.
> +
> +package Bugzilla::WebService::Server::REST::Resources::LastVisited;

it's incorrect to do this in a REST resources file; this file should only contain the _rest_resources sub -- move it to Bugzilla::WebService::LastVisited (including the POD).

the functional logic for all these methods should be moved out of the webservice class and into a base class, with the webservice class just wrapping the calls against our business logic layer.
it probably makes sense to have a Bugzilla::LastVisisted which ISA Bugzilla::Object interface to the last_visited table, and helper methods in Bugzilla::Bug to set and get using the current user.

@@ +32,5 @@
> +    Bugzilla->user->visible_bugs(\@int);
> +
> +    my @result;
> +    foreach my $bug_id (@$ids) {
> +        my $bug = Bugzilla::Bug->check($bug_id);

use the cache here .. Bugzilla::Bug->check({ id => $bug_id, cache => 1 });

@@ +36,5 @@
> +        my $bug = Bugzilla::Bug->check($bug_id);
> +
> +        $dbh->do( $dbh->sql_replace_into(
> +            'last_visited',
> +            {user_id => $user->id, bug_id => $bug->id, last_visited => \'now()'},

grab the timestamp outside of this foreach and pass it to last_visited - it's important to use the same timestamp for changes made at the same time.

nit: spaces after { and before }

@@ +43,5 @@
> +
> +        my $last_visited = $dbh->selectrow_array("
> +            SELECT last_visited FROM last_visited
> +             WHERE user_id = ? AND bug_id = ?",
> +            undef, $user->id, $bug->id);

there's no need to do this.

@@ +83,5 @@
> +    my ($self, $bug, $last_visited, $params) = @_;
> +    my $user = Bugzilla->user;
> +
> +    my $data = {
> +        id           => $self->type('int',      $bug->id),

for clarity, return the $bug->id as bug_id

@@ +90,5 @@
> +
> +    if ($user->can_see_bug($bug)) {
> +        $data->{status}  = $self->type('string', $bug->bug_status),
> +        $data->{summary} = $self->type('string', $bug->short_desc),
> +    }

you're already excluding bugs which the user can't see by in get(), so you don't need to check it again here.

that said, i'm not sure of the value of returning bug information there beyond the id.  it's reasonably expensive to create all those bugs objects, especially since the number of bugs a bugzilla user visits can be extremely large.

@@ +113,5 @@
> +                },
> +            },
> +        },
> +        # product/component
> +        qr{^/review/lastvisited$}, {

/review/lastvisited?

i suspect this isn't supposed to be here.

Comment 45

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

Additional notes from a chat with glob:

> [glob] make sure you pass cache=>1 to ->check()
> [glob] i'm not sure why you're selecting last_visited after updating it
> [glob] don't use now()
> [glob] instead do a select outside of the loop to get the current time from the db
> [glob] and pass that in
> [dylan] that also means I don't need my silly quote_ref() addition to Bugzilla::DB
> [glob] and it should also mean you do't need to select last_visited after setting it
> [dylan] I'm not even sure 'add' should return all that info
> [glob] also..
> [glob] the webservce is the wrong place for this logic
> [glob] the webservice stuff upstream should just call methods on base objects
> [dylan] so we should have a LastVisted object?
> [dylan] Or should this be part of Bug?
> [glob] maybe both..
> [glob] a package for the last_visited table, to abstract away the db calls
> [glob] and a method on a bug object for connivence

Comment 46

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

I guess if LastVisited acts like the rest of the Bugzilla::Objects, there will be no need for my cross-platform sql replace into code?

Comment 47

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

Attachment: bug-489028-WIP-2-webservice-bits.diff

I've reorganized the code per the feedback suggestions, which means we are no longer using the sql replace code -- although I did spend a fair amount of time testing it -- but I guess that's not much of a concern.

I was pretty close to having a working bugzilla install against an oracle DB, which could be useful for verifying oracle/bugzilla bugs in the future, so I'll keep that VM around just in case.

Comment 48

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

Attachment: bug-489028-WIP-3.diff

last_visited is updated from javascript and stars (unicode glyph, per the original bug 410107 v4 patch). Currently this doesn't work right as changeddate doesn't have the granularity of last_visited -- and this is probably not what we want anyway. 

I have another approach where I update the search results with a javascript API call, but I was sufficiently less happy with that approach -- more discussion is probably warranted.

In addition, I am very curious how we can do authenticated GET requests against the REST API. Currently I make use of the JSONRPC API...

Comment 49

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

And by "this is probably not what we want anyway", I mean adding a join to the already complex bugs table is probably a bad idea.

Comment 50

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

Attachment: bug-489028-v1.patch

I'm absolutely certain the way I've implemented the search/query portion of this is wrong -- but it shows intent. Basically I am passing last_visited=1 to buglist.cgi and when that paramter is passed last_visited is added to selected columns -- but not displayed columns. Then the template checks the columns template hash to see if last visited is available and compares it to changedtime. If changedtime is newer than last visited, we show a red star.

This search is discoverable through a link next to My Bugs.

I've set this to feedback one more time to signal a quick look-over so I can fix the way the query is performed before first round of actual review.

/ dylan

Comment 51

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

As it stands, with this in-progress patch we record the last time that any given user visits a bug,
and allow a predefined query similar to "My Bugs" (shown in the footer). When this query is used, if the last time a bug was changed is newer than the last time you've seen it, a star is shown before the bug id column.

I'd like to know if there are (additional) ways we could make use of this last visited data to help the user navigate between bugs. 

Cheers,

/ dylan

Comment 52

[:glob ✱]

Comment on attachment 8392062 [details] [diff] [review]
bug-489028-v1.patch

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

just quick read-only (untested) feedback:

> This search is discoverable through a link next to My Bugs.
i don't see that here

> I'm absolutely certain the way I've implemented the search/query portion of this is wrong...
add last_visited to the list of selectable columns (look at Bugzilla::Search::COLUMNS).
there shouldn't be any need for any other special checks.

we also need to prune that table, as it could get extremely large.  use the user's last login date to drive the pruning and remove rows for users who haven't logged in to bugzilla for some time (eg. 3 months).


i was giving the user experience of this some thought over the weekend, and i think it may be beneficial to change things slightly (looking for feedback).
currently we're tracking the last-visited for every bug you visit, and we can then generate a query of bugs updated since the last time you visited it.
personally i want the list of 'updated bugs' to be a list of bugs which i care about, however with the current implementation it would be easy to have a polluted list of bugs which i visited but don't want to follow, and there's no way to remove a bug from that list.

i think it may be more useful to track the last-visited only on bugs that you are directly involved in: bugs where you are the reporter, assignee, qa_contact, or CC'd.

::: Bugzilla/DB/Schema.pm
@@ +1715,5 @@
>  
> +    last_visited => {
> +        FIELDS => [
> +            id => {
> +                TYPE       => 'MEDIUMSERIAL',

this should be INTSERIAL

::: Bugzilla/LastVisited.pm
@@ +26,5 @@
> +use constant {
> +    AUDIT_CREATES => 0,
> +    AUDIT_UPDATES => 0,
> +    AUDIT_REMOVES => 0,
> +};

also disable MEMCACHED for these objects

::: Bugzilla/Search.pm
@@ +519,5 @@
> +            from  => 'bug_id',
> +            table => 'last_visited',
> +            to    => 'bug_id',
> +            as    => 'last_visited',
> +        },

also need to take the current user_id into account (use extra).

::: Bugzilla/WebService/LastVisited.pm
@@ +49,5 @@
> +
> +    if ($ids) {
> +        # Cache permissions for bugs. This highly reduces the number of calls to the DB.
> +        # visible_bugs() is only able to handle bug IDs, so we have to skip aliases.
> +        $user->visible_bugs([grep { looks_like_number($_) } @$ids]);

use !/\D/ instead of looks_like_number, because looks_like_number treats "Inf" and "Infinity" as numbers.

::: Bugzilla/WebService/Server/REST/Resources/LastVisited.pm
@@ +32,5 @@
> +                },
> +            },
> +        },
> +        # product/component
> +        qr{^/review/lastvisited$}, {

i suspect this doesn't belong here :)

::: js/last-visited.js
@@ +11,5 @@
> +
> +    YAHOO.bugzilla.lastVisited = {
> +
> +        add: function(bug_id) {
> +            /* Here we get to use the REST API, because we arn't using the GET verb... */

use JSONRPC for both until the REST authentication story has been resolved.
these calls will likely fail on a non-public bug.

@@ +15,5 @@
> +            /* Here we get to use the REST API, because we arn't using the GET verb... */
> +
> +            var callbacks = {
> +                success: function(res) { console.log("added last visit: " + res.responseText); },
> +                failure: function(res) { console.log("failed to update last visited: " + res.responseText); },

remove debug logging

@@ +35,5 @@
> +                params: { },
> +            });
> +            var callbacks = {
> +                success: function(res) { done(JSON.parse(res.responseText)) },
> +                failure: function(res) { console.log("failed to get last visited: " + res.responseText); },

remove debug logging

::: template/en/default/list/table.html.tmpl
@@ +183,5 @@
> +    <td class="bug_updated_alert">&#9733; </td>
> +    [% ELSE %]
> +    <td> </td>
> +    [% END %]
> +    [% END %]

the indentation is incorrect in this file.

Comment 53

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

(In reply to Byron Jones ‹:glob› from comment #52)
> Comment on attachment 8392062 [details] [diff] [review]
> bug-489028-v1.patch
> 
> Review of attachment 8392062 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> just quick read-only (untested) feedback:
> 
> > This search is discoverable through a link next to My Bugs.
> i don't see that here
woops, forgot to regenerate that patch with that change. It was the part I did sunday evening.

> > I'm absolutely certain the way I've implemented the search/query portion of this is wrong...
> add last_visited to the list of selectable columns (look at
> Bugzilla::Search::COLUMNS).
> there shouldn't be any need for any other special checks.
And when I do that, I just have to specify all of the columns I want in $params->param('columnlist')?

> we also need to prune that table, as it could get extremely large.  use the
> user's last login date to drive the pruning and remove rows for users who
> haven't logged in to bugzilla for some time (eg. 3 months).
Presumably in a script that is run from cron?

> i was giving the user experience of this some thought over the weekend, and
> i think it may be beneficial to change things slightly (looking for
> feedback).
> currently we're tracking the last-visited for every bug you visit, and we
> can then generate a query of bugs updated since the last time you visited it.
> personally i want the list of 'updated bugs' to be a list of bugs which i
> care about, however with the current implementation it would be easy to have
> a polluted list of bugs which i visited but don't want to follow, and
> there's no way to remove a bug from that list.
> 
> i think it may be more useful to track the last-visited only on bugs that
> you are directly involved in: bugs where you are the reporter, assignee,
> qa_contact, or CC'd.
That does seem useful. Could we do this client side in the API call?


> ::: Bugzilla/LastVisited.pm
> also disable MEMCACHED for these objects
will do

> ::: Bugzilla/Search.pm
> @@ +519,5 @@
> > +            from  => 'bug_id',
> > +            table => 'last_visited',
> > +            to    => 'bug_id',
> > +            as    => 'last_visited',
> > +        },
> 
> also need to take the current user_id into account (use extra).

Noted

> ::: Bugzilla/WebService/LastVisited.pm
> @@ +49,5 @@
> > +
> > +    if ($ids) {
> > +        # Cache permissions for bugs. This highly reduces the number of calls to the DB.
> > +        # visible_bugs() is only able to handle bug IDs, so we have to skip aliases.
> > +        $user->visible_bugs([grep { looks_like_number($_) } @$ids]);
> 
> use !/\D/ instead of looks_like_number, because looks_like_number treats
> "Inf" and "Infinity" as numbers.

Ick. I think [0-9] is then the most correct option; \d matches any digit character from any alphabet. It currently doesn't matter because utf8 strings are marked as bytes and not characters, but if someone ever fixes that. I'll use \d for now, but I'd like it to be noted somewhere. Or if we ever bump the minimum version of perl to 5.14, we can use the /a (ascii) modifier.

> ::: Bugzilla/WebService/Server/REST/Resources/LastVisited.pm
> @@ +32,5 @@
> > +                },
> > +            },
> > +        },
> > +        # product/component
> > +        qr{^/review/lastvisited$}, {
> 
> i suspect this doesn't belong here :)
Cruft that is now removed.

> ::: js/last-visited.js
> @@ +11,5 @@
> > +
> > +    YAHOO.bugzilla.lastVisited = {
> > +
> > +        add: function(bug_id) {
> > +            /* Here we get to use the REST API, because we arn't using the GET verb... */
> 
> use JSONRPC for both until the REST authentication story has been resolved.
> these calls will likely fail on a non-public bug.
Will do.

> remove debug logging
> 
> ::: template/en/default/list/table.html.tmpl
> @@ +183,5 @@
> > +    <td class="bug_updated_alert">&#9733; </td>
> > +    [% ELSE %]
> > +    <td> </td>
> > +    [% END %]
> > +    [% END %]
> 
> the indentation is incorrect in this file.

I literally stared at that block for 5 minutes deciding if I should indent or not.

Comment 54

[Frédéric Buclin]

Comment on attachment 8392062 [details] [diff] [review]
bug-489028-v1.patch

>diff --git Bugzilla/Bug.pm Bugzilla/Bug.pm

>+# Update last_visited table
>+sub update_last_visited {

This method has nothing to with Bugzilla::Bug. It's a Bugzilla::User property. A user is viewing bugs and his list of viewed bugs is updated. It's not a bug property as you don't alter a bug at all. So this method should be moved into Bugzilla::User. This also means you don't need to pass $user as argument as you would call it with $user->update_last_visited_bug($bug_id, $timestamp) or something similar.


>+    require Bugzilla::LastVisited;
>+    my $lv = Bugzilla::LastVisited->match({bug_id => $self->id, user_id => $user->id})->[0];

I'm not a fan of this code. There is already a Bugzilla::User method to get all visited bugs. If there is no entry for this bug, then you know it's not already in the DB. That's more efficient that calling the DB.



>diff --git Bugzilla/DB/Schema.pm Bugzilla/DB/Schema.pm

>+    last_visited => {
>+        FIELDS => [
>+            id => {
>+                TYPE       => 'MEDIUMSERIAL',

Please use the same indentation as we did everywhere else in this file, for consistency. Also, this table should be named visited_bugs. There is a plenty of things we could visit in Bugzilla, such as attachments or preferences, so _bugs make sense. And the fact that you want to store the last time you visited the bug is a property of the time column, not of the table itself (the 'profiles' table has a last_seen_date colum, also the logincookies table has a lastused column. The tables themselves don't contain last_ in their name).


>+            last_visited => {
>+                TYPE    => 'DATETIME',

Rename it to last_visited_date as _date clearly states that it's a time field. And here it makes sense to start with last_ as this column specifically records the last time you visited the bug.


>diff --git Bugzilla/LastVisited.pm Bugzilla/LastVisited.pm

I don't see the point to have a whole separate module for this. And assuming it's absolutely necessary, it should be a sub-module of Bugzilla::User, and should be moved into Bugzilla/User/VisitedBugs.pm.


>+package Bugzilla::LastVisited;

This module has no MPL 2.0 license.


>+sub id           { return $_[0]{id}           }
>+sub bug_id       { return $_[0]{bug_id}       }
>+sub user_id      { return $_[0]{user_id}      }
>+sub last_visited { return $_[0]{last_visited} }

$_[0] is a hashref, so you must call $_[0]->{id}. Same for other methods.



>diff --git Bugzilla/User.pm Bugzilla/User.pm

>+sub bugs_last_visited {
>+    my ($self) = @_;
>+    require Bugzilla::LastVisited;

Instead of this, you should simply |use Bugzilla::Foo|.


>+    my $matches = Bugzilla::LastVisited->match({ user_id => $self->id });

No need to pass user_id if you make it a Bugzilla::User method.



>diff --git Bugzilla/WebService/Constants.pm Bugzilla/WebService/Constants.pm

>+        'LastVisited'    => 'Bugzilla::WebService::LastVisited',

This is definitely a method of Bugzilla::WS::User. Nothing more.



>+++ Bugzilla/WebService/LastVisited.pm

>+sub add {

>+        $bug->update_last_visited($user, $last_visited);

There is no validation that the user is logged in. If he is not, then nothing should be recorded.



>diff --git js/last-visited.js js/last-visited.js

Please don't create a new JS file. See bug 956191 for the rationale. The existing js/bug.js file seems a better place for your code.


>diff --git template/en/default/list/table.html.tmpl template/en/default/list/table.html.tmpl

>+      [% IF columns.last_visited %]
>+      <th>&nbsp;</th>
>+      [% END %]

Please fix the indentation.


>+    [% IF columns.last_visited %]
>+    [% IF bug.last_visited.defined && bug.last_visited <= bug.changedtime  %]
>+    <td class="bug_updated_alert">&#9733; </td>
>+    [% ELSE %]
>+    <td> </td>
>+    [% END %]
>+    [% END %]

Same here. Also write <td></td> instead of a whitespace if you don't intend to put anything here (or use &nbsp; as you did above).


As glob said, the table can become incredibily huge. We should delete old entries after some time.

Comment 55

[(no longer active)]

(In reply to Dylan William Hardison [:dylan] from comment #51)
> As it stands, with this in-progress patch we record the last time that any
> given user visits a bug,
> and allow a predefined query similar to "My Bugs" (shown in the footer).
> When this query is used, if the last time a bug was changed is newer than
> the last time you've seen it, a star is shown before the bug id column.
> 
> I'd like to know if there are (additional) ways we could make use of this
> last visited data to help the user navigate between bugs. 

The use cases that I'm interested in here are:

1. What are the bugs reported by me which have changed since I last visited them?
2. What are the bugs assigned to me which have changed since I last visited them?

These two queries will basically let people open Bugzilla in the morning and quickly catch up with what they need to do without having to go through all of their bugmail.

I actually think it might make sense to expose these two queries separately.  For people with a lot of bugs reported by them or assigned to them (such as yours truly), going through that long list and looking for stars is going to be painful.  I'd prefer if I only saw the changed bugs, and had a different query that showed me all bugs reported by me or assigned to me irrespective of when I last visited them in case I have a specific need to look for them.

Please let me know if this sufficiently answers your question, and also if you have any questions.  Thanks!

Comment 56

[(no longer active)]

(Note that I can imagine others also caring about the bugs they're CCed on that have changed since they last visited them, but I personally can't imagine why it would be useful to my workflow.  Perhaps it would make sense to expose the above two queries first, get some feedback from people, and see if anyone asks for more.)

Comment 57

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

(In reply to Frédéric Buclin from comment #54)
> Comment on attachment 8392062 [details] [diff] [review]
> bug-489028-v1.patch
> 
> >diff --git Bugzilla/Bug.pm Bugzilla/Bug.pm
> 
> >+# Update last_visited table
> >+sub update_last_visited {
> 
> This method has nothing to with Bugzilla::Bug. It's a Bugzilla::User
> property. A user is viewing bugs and his list of viewed bugs is updated.
> It's not a bug property as you don't alter a bug at all. So this method
> should be moved into Bugzilla::User. This also means you don't need to pass
> $user as argument as you would call it with
> $user->update_last_visited_bug($bug_id, $timestamp) or something similar.

Well, I think it could go either way. If it is a method on user we have to pass the bug in. But I'm not fussed either way.

> >+    require Bugzilla::LastVisited;
> >+    my $lv = Bugzilla::LastVisited->match({bug_id => $self->id, user_id => $user->id})->[0];
> 
> I'm not a fan of this code. There is already a Bugzilla::User method to get
> all visited bugs. If there is no entry for this bug, then you know it's not
> already in the DB. That's more efficient that calling the DB.

I thought it would be more efficient to not request all the bugs (which could be quite large), but I can see your point.

> >diff --git Bugzilla/DB/Schema.pm Bugzilla/DB/Schema.pm
> 
> >+    last_visited => {
> >+        FIELDS => [
> >+            id => {
> >+                TYPE       => 'MEDIUMSERIAL',
> 
> Please use the same indentation as we did everywhere else in this file, for
> consistency. Also, this table should be named visited_bugs. There is a
> plenty of things we could visit in Bugzilla, such as attachments or
> preferences, so _bugs make sense. And the fact that you want to store the
> last time you visited the bug is a property of the time column, not of the
> table itself (the 'profiles' table has a last_seen_date colum, also the
> logincookies table has a lastused column. The tables themselves don't
> contain last_ in their name).
I'll fix the indentation, and I think I like the name "bugs_visited", I was already considering that name.

> 
> >+            last_visited => {
> >+                TYPE    => 'DATETIME',
> 
> Rename it to last_visited_date as _date clearly states that it's a time
> field. And here it makes sense to start with last_ as this column
> specifically records the last time you visited the bug.

It is both a date and a time, is it conventional for it to just be _date?

> >diff --git Bugzilla/LastVisited.pm Bugzilla/LastVisited.pm
> 
> I don't see the point to have a whole separate module for this. And assuming
> it's absolutely necessary, it should be a sub-module of Bugzilla::User, and
> should be moved into Bugzilla/User/VisitedBugs.pm.

Can do, I was directed on a previous feedback cycle to turn it into its own Bugzilla::Object, but we might re-think that in favor of doing it like the bug tag system. 

> >+package Bugzilla::LastVisited;
> 
> This module has no MPL 2.0 license.
> 
> 
> >+sub id           { return $_[0]{id}           }
> >+sub bug_id       { return $_[0]{bug_id}       }
> >+sub user_id      { return $_[0]{user_id}      }
> >+sub last_visited { return $_[0]{last_visited} }
> 
> $_[0] is a hashref, so you must call $_[0]->{id}. Same for other methods.

perldoc perlref, "The arrow is optional between brackets subscripts". If you mean the style with arrows is preferred, I can do that. (But both styles seem to exist in the codebase.)

> 
> 
> >diff --git Bugzilla/User.pm Bugzilla/User.pm
> 
> >+sub bugs_last_visited {
> >+    my ($self) = @_;
> >+    require Bugzilla::LastVisited;
> 
> Instead of this, you should simply |use Bugzilla::Foo|.
But then it will be moved up to the top of the file as use is a compile time directive. I was under the impression we tried to lazy load modules when possible for speed under CGI environments. 

> 
> >+    my $matches = Bugzilla::LastVisited->match({ user_id => $self->id });
> 
> No need to pass user_id if you make it a Bugzilla::User method.
> 
> 
> 
> >diff --git Bugzilla/WebService/Constants.pm Bugzilla/WebService/Constants.pm
> 
> >+        'LastVisited'    => 'Bugzilla::WebService::LastVisited',
> 
> This is definitely a method of Bugzilla::WS::User. Nothing more.
> 
> 
> 
> >+++ Bugzilla/WebService/LastVisited.pm
> 
> >+sub add {
> 
> >+        $bug->update_last_visited($user, $last_visited);
> 
> There is no validation that the user is logged in. If he is not, then
> nothing should be recorded.

I'll do that.

> >diff --git js/last-visited.js js/last-visited.js
> 
> Please don't create a new JS file. See bug 956191 for the rationale. The
> existing js/bug.js file seems a better place for your code.

Will do. I followed the recent example of comment-tagging.

> 
> Please fix the indentation.
> 
> Same here. Also write <td></td> instead of a whitespace if you don't intend
> to put anything here (or use &nbsp; as you did above).
> 
> 
> As glob said, the table can become incredibily huge. We should delete old
> entries after some time.

I'll fix those things, but first I need to fix this up to use search properly.

Comment 58

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

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #55)
> (In reply to Dylan William Hardison [:dylan] from comment #51)
> > As it stands, with this in-progress patch we record the last time that any
> > given user visits a bug,
> > and allow a predefined query similar to "My Bugs" (shown in the footer).
> > When this query is used, if the last time a bug was changed is newer than
> > the last time you've seen it, a star is shown before the bug id column.
> > 
> > I'd like to know if there are (additional) ways we could make use of this
> > last visited data to help the user navigate between bugs. 
> 
> The use cases that I'm interested in here are:
> 
> 1. What are the bugs reported by me which have changed since I last visited
> them?
> 2. What are the bugs assigned to me which have changed since I last visited
> them?
> 
> These two queries will basically let people open Bugzilla in the morning and
> quickly catch up with what they need to do without having to go through all
> of their bugmail.
> 
> I actually think it might make sense to expose these two queries separately.
> For people with a lot of bugs reported by them or assigned to them (such as
> yours truly), going through that long list and looking for stars is going to
> be painful.  I'd prefer if I only saw the changed bugs, and had a different
> query that showed me all bugs reported by me or assigned to me irrespective
> of when I last visited them in case I have a specific need to look for them.
> 
> Please let me know if this sufficiently answers your question, and also if
> you have any questions.  Thanks!

Thank you, that is very helpful!

Comment 59

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

I am somewhat happy that I have the following only slightly incorrect sql query:

    SELECT bugs.bug_id AS bug_id, last_visited.last_visited AS last_visited, bugs.bug_status AS bug_status, bugs.priority AS priority, map_assigned_to.login_name AS assigned_to
    FROM bugs
    LEFT JOIN bug_group_map AS security_map ON bugs.bug_id = security_map.bug_id AND NOT ( security_map.group_id IN (12,14,13,21,16,17,7,3,6,15,4,5,2,1,10,18,19,20,8,9) )
    LEFT JOIN cc AS security_cc ON bugs.bug_id = security_cc.bug_id AND security_cc.who = 786
    LEFT JOIN last_visited AS last_visited ON bugs.bug_id = last_visited.bug_id
    INNER JOIN profiles AS map_assigned_to ON bugs.assigned_to = map_assigned_to.userid
    INNER JOIN bug_status AS map_bug_status ON bugs.bug_status = map_bug_status.value
    INNER JOIN priority AS map_priority ON bugs.priority = map_priority.value
    WHERE bugs.creation_ts IS NOT NULL
    AND (security_map.group_id IS NULL
            OR (bugs.reporter_accessible = 1 AND bugs.reporter = 786)
            OR (bugs.cclist_accessible = 1 AND security_cc.who IS NOT NULL)
            OR bugs.assigned_to = 786
    )
    AND last_visited.last_visited = 'batman'
    GROUP BY bugs.bug_id
    ORDER BY map_bug_status.sortkey, map_bug_status.value, map_priority.sortkey, map_priority.value, assigned_to, bug_id
    LIMIT 500

To do this I have had to wade through so much of the Search.pm code... which is so large that I have to turn off syntax highlighting to edit it in vim.

Now, mostly for my benefit, what needs to be done to finish implementing the last_visited feature as a search clause. Now, for my custom fielddef and search operator, i need to
1) limit the strings to either assignee or reporter ($lv_type)
2) change the SQL generated to be last_visited.last_visited < createdtime AND last_visited.type = $lv_type.

After this I can worry about renaming last_visited, which remains undone as this was quite difficult to figure out!

Comment 60

[:glob ✱]

following a quick discussion with dylan about this over irc, the plan is to expose last_visited as a date field, and add a new SPECIAL_PARSING rule which expands to the bug's delta_ts.

you'll be able to construct queries such as
(last_visited) (is less than) [%last_updated%]

last_visited within the context of search should return the timestamp of when the current user last visited that bug.  for non-authenticated requests we can short-circuit that to "1=2".  we don't need to support the ability to determine when someone else last visited a bug.

Comment 61

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

Attachment: bug-489028-v2.patch

This is by no means done, but I'd like to discuss it on our weekly call. Searching works! I refactored the namings (bug_visits, bug_visists.last_visit, and Last Visit in the advanced search UI).

Last Visit takes a date, and can understand either the 1d, 1w, 1y format (as other fields do) or can take the expando pronoun %last_changed%.

Comment 62

[:glob ✱]

Comment on attachment 8395445 [details] [diff] [review]
bug-489028-v2.patch

as it's incomplete, switching from review? to feedback?

Comment 63

[:glob ✱]

Comment on attachment 8395445 [details] [diff] [review]
bug-489028-v2.patch

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

feedback from a quick read of the patch.
please make sure you address all prior review/feedback points prior to attaching a new patch.
while i haven't tested it yet, the search code looks great :)

::: Bugzilla/Bug.pm
@@ +4064,5 @@
>  
> +# Update bug_visits table
> +sub add_visitor {
> +    my ($self, $user, $last_visit) = @_;
> +    my $lv = Bugzilla::BugVisit->match({bug_id => $self->id, user_id => $user->id})->[0];

you need a space after { and before }: ->match({ ... })

this issue occurs many times in this patch.

::: Bugzilla/BugVisit.pm
@@ +8,5 @@
> +#####################################################################
> +# Overriden Constants that are used as methods
> +#####################################################################
> +
> +use constant DB_TABLE   => 'bug_visits';

i don't agree with this name, as it doesn't track "bug visits", which could easily be interpreted as "a list of all times a user has visited a bug".

bug_last_visited or just last_visited would be better.

@@ +26,5 @@
> +use constant {
> +    AUDIT_CREATES => 0,
> +    AUDIT_UPDATES => 0,
> +    AUDIT_REMOVES => 0,
> +};

as per comment 52, also disable MEMCACHED for these objects

::: Bugzilla/DB/Schema.pm
@@ +1714,5 @@
>      },
>  
> +    bug_visits => {
> +        FIELDS => [
> +            id           => { TYPE => 'MEDIUMSERIAL', NOTNULL => 1, PRIMARYKEY => 1 },

as per comment 52, this needs to be INTSERIAL

::: Bugzilla/Field.pm
@@ +256,5 @@
>       type => FIELD_TYPE_BUG_URLS},
>      {name => 'tag',                   desc => 'Tags', buglist => 1,
>       type => FIELD_TYPE_KEYWORDS},
> +    {name => 'last_visit',            desc => 'Last Visit', buglist => 1,
> +     type => FIELD_TYPE_DATE },

shouldn't this be FIELD_TYPE_DATETIME ?

::: Bugzilla/Search.pm
@@ +2161,5 @@
> +
> +    $self->_datetime_translate($args);
> +    if ($value eq $args->{value}) {
> +        # Failed to translate a datetime. let's try the pronoun expando.
> +        if ($value =~ /^%last_changed%$/) {

$value eq '%last_changed%'

::: Bugzilla/WebService/BugVisit.pm
@@ +28,5 @@
> +    $user->login(LOGIN_REQUIRED);
> +
> +    # Cache permissions for bugs. This highly reduces the number of calls to the DB.
> +    # visible_bugs() is only able to handle bug IDs, so we have to skip aliases.
> +    $user->visible_bugs([grep { looks_like_number($_) } @$ids]);

again, use !/\D/ instead of looks_like_number, because looks_like_number treats "Inf" and "Infinity" as numbers.

@@ +50,5 @@
> +
> +    if ($ids) {
> +        # Cache permissions for bugs. This highly reduces the number of calls to the DB.
> +        # visible_bugs() is only able to handle bug IDs, so we have to skip aliases.
> +        $user->visible_bugs([grep { looks_like_number($_) } @$ids]);

same here, don't use looks_like_number

::: Bugzilla/WebService/Server/REST/Resources/BugVisit.pm
@@ +32,5 @@
> +                },
> +            },
> +        },
> +        # product/component
> +        qr{^/bug_visit$}, {

again, i suspect this doesn't belong here.
this feature is unrelated to product/component, and this rest endpoint doesn't appear to offer anything over the other one

::: js/bug.js
@@ +124,5 @@
> +    var JSON = YAHOO.lang.JSON;
> +
> +    YAHOO.bugzilla.bugVisit = {
> +
> +        /* JSON RPC is deprecated but we use it until REST supports everything. */

this comment is not required

::: skins/standard/buglist.css
@@ +42,5 @@
>  .bz_sort_order_primary   { color: black; }
>  .bz_sort_order_secondary { color: #777;  }
>  
> +td.bug_updated_alert {
> +    text-decoration:none;

nit: missing a space after :

@@ +46,5 @@
> +    text-decoration:none;
> +    color: red;
> +    font-size: medium;
> +    text-align: right;
> +}

i don't see where this class is being used.

if you're thinking of displaying a message when the call to update the last visited timestamp is updated - don't.  there's no need to inform the user of this action.

Comment 64

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

Update: I have finished renaming everything (again), to bug_user_last_visited / BugUserLastVisited, and a stab at the cronjob (purge-old-bug-user-last-visited.pl, currently). 

I've also

* catch the errors when you use a changed* operator on the last visited field (untested)
* remove debugging console logging
* disable memcached for BugUserLastVisited

I have not been able to test this yet -- so as soon as I test that (and address some remaining points in the previous feedback reviews) we should be golden.

Comment 65

[Frédéric Buclin]

(In reply to Dylan William Hardison [:dylan] from comment #64)
> Update: I have finished renaming everything (again), to
> bug_user_last_visited

To me, this table name doesn't make sense. You are enumerating all columns of the table in its name. I still say that bugs_visited is the right name for this table. Contrary to bug_visits which may indicate that you log all visits, bugs_visited makes it clearer that it logs bugs which have been visited. And this is indeed what it does, so this name is accurate.

Comment 66

[:glob ✱]

(In reply to Frédéric Buclin from comment #65)
> To me, this table name doesn't make sense. You are enumerating all columns
> of the table in its name. I still say that bugs_visited is the right name
> for this table.

looks like we'll have to agree to disagree on this point :)

<executive_decision>
dylan: stay with bug_user_last_visited.
</executive_decision>

Comment 67

[Dave Miller [:justdave]]

(In reply to Frédéric Buclin from comment #65)
> To me, this table name doesn't make sense. You are enumerating all columns
> of the table in its name.

This is a table which links multiple entities together (users and bugs).  Usually such tables name the entities being linked together (see the groups-related tables) as part of the table name, so this makes good sense to me.

Comment 68

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

Attachment: bug-489028-v3.patch

Woo! This patch should meet the needs of the previous feedback comments... unless I have overlooked something. I've repainted the bikeshed so many times that I might have missed a corner, but the code definitely runs. It also catches errors when using incompatible operators. We might be able to add that check to _pick_override_function, but that's a thought for another time... It seems like the code silently ignores invalid operator functions most of the time (except when you do what I did..)

Comment 69

[:glob ✱]

Comment on attachment 8397445 [details] [diff] [review]
bug-489028-v3.patch

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

a quick mostly visual review..

> Woo! This patch should meet the needs of the previous feedback comments...
> unless I have overlooked something.

i don't see anything that removes old entries :)

this patch fails to apply to trunk:
patching file js/bug.js
Hunk #1 FAILED at 118.
1 out of 1 hunk FAILED -- saving rejects to file js/bug.js.rej

this patch fails tests:
t/011pod.t ........... 209/352
#   Failed test 'Bugzilla/Bug.pm POD coverage is 99%. Undocumented methods: update_user_last_visit'
#   at t/011pod.t line 103.
# Looks like you failed 1 test of 352.

lines longer that 80 characters should be wrapped.  i was going to point you to where it says this in our style guide, but it doesn't, so it's understandable that you weren't aware of this.  i've raised bug 988736 to get this added to the guide.  please update all code to ensure it wraps to 80 chars where possible.

::: Bugzilla/BugUserLastVisit.pm
@@ +17,5 @@
> +    last_visit_ts
> +);
> +use constant UPDATE_COLUMNS => qw( last_visit_ts );
> +
> +use constant LIST_ORDER => 'id';

you need to also set NAME_FIELD to 'id', as the default is 'name'.

@@ +21,5 @@
> +use constant LIST_ORDER => 'id';
> +
> +use constant VALIDATORS => {};
> +
> +# turn off auditing

"turn off auditing and exclude these objects from memcached"

::: Bugzilla/DB/Schema.pm
@@ +1719,5 @@
> +            user_id       => { TYPE => 'INT3', NOTNULL => 1,
> +                               REFERENCES => { TABLE => 'profiles', COLUMN => 'userid', DELETE => 'CASCADE' } },
> +            bug_id        => { TYPE => 'INT3', NOTNULL => 1,
> +                               REFERENCES => { TABLE => 'bugs', COLUMN => 'bug_id', DELETE => 'CASCADE' } },
> +            last_visit_ts => { TYPE => 'DATETIME', NOTNULL => 1 },

please reformat the references to match the other entries.
eg.

            user_id       => {TYPE => 'INT3', NOTNULL => 1,
                              REFERENCES => {TABLE  => 'profiles',
                                             COLUMN => 'userid',
                                             DELETE => 'CASCADE'}},

::: Bugzilla/Search.pm
@@ +525,5 @@
> +        last_visit_ts => {
> +            from  => 'bug_id',
> +            table => 'bug_user_last_visit',
> +            to    => 'bug_id',
> +            as    => 'bug_user_last_visit',

you also need to join using the current user's id.

::: Bugzilla/WebService/BugUserLastVisit.pm
@@ +21,5 @@
> +    my ($self, $params) = validate(@_, 'ids');
> +    my $user = Bugzilla->user;
> +    my $dbh  = Bugzilla->dbh;
> +
> +    # $user->login(LOGIN_REQUIRED);

oops, this shouldn't be commented out :)

@@ +33,5 @@
> +
> +    my @results;
> +    my $last_visit_ts = $dbh->selectrow_array('SELECT NOW()');
> +    foreach my $bug_id (@$ids) {
> +        my $bug = Bugzilla::Bug->check({id => $bug_id, cache => 1});

nit: space after { and before }

::: js/bug.js
@@ +121,5 @@
> +
> +if (typeof console === "undefined" || typeof console.log === "undefined") {
> +    console = {};
> +    console.log = function() {};
> +}

we don't hit console enough to warrant this; instead just add an if (console) to your failure code.

::: template/en/default/bug/show-header.html.tmpl
@@ +25,4 @@
>  [% title = title _ filtered_desc %]
>  [% yui = ['autocomplete', 'calendar'] %]
>  [% yui.push('container') IF user.can_tag_comments %]
> +[% javascript_urls = [ "js/util.js", "js/field.js", "js/bug.js" ] %]

only add js/bug.js to this list for authenticated show_bug requests:

[% javascript_urls.push('js/bug.js') IF user.id %]

@@ +52,5 @@
>      }
>      YAHOO.util.Event.onDOMReady(function() {
>        initDirtyFieldTracking();
> +
> +      [% IF (bug.assigned_to && bug.assigned_to.id == user.id) || (bug.reporter && bug.reporter.id == user.id) %]

assigned_to and reporter are mandatory fields, there's no need to check that they are set first.

as per comment 52, you also need to check qa_contact (which isn't mandatory, and may be disabled via a param), and the cc list.

i'd also like to see this wrapped in an outer [% IF user.id %] check.

@@ +53,5 @@
>      YAHOO.util.Event.onDOMReady(function() {
>        initDirtyFieldTracking();
> +
> +      [% IF (bug.assigned_to && bug.assigned_to.id == user.id) || (bug.reporter && bug.reporter.id == user.id) %]
> +        YAHOO.bugzilla.bugUserLastVisit.update([% bug.bug_id FILTER js %]);

nit: this can be FILTER none

Comment 70

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

Attachment: bug-489028-v4.patch

(In reply to Byron Jones ‹:glob› from comment #69)
> 
> i don't see anything that removes old entries :)
clean-bug-user-last-visit.pl added. I kinda feel that cron scripts should get their own folder, but I guess it's just this one and whine.pl

> this patch fails to apply to trunk:
> patching file js/bug.js
> Hunk #1 FAILED at 118.
> 1 out of 1 hunk FAILED -- saving rejects to file js/bug.js.rej
Rebased to current trunk, it should apply cleanly now.

> this patch fails tests:
That would be from the $!$#@ renaming, it passes now. 

> lines longer that 80 characters should be wrapped.  i was going to point you
> to where it says this in our style guide, but it doesn't, so it's
> understandable that you weren't aware of this.  i've raised bug 988736 to
> get this added to the guide.  please update all code to ensure it wraps to
> 80 chars where possible.
Code updated. There were only a few lines longer than 80.

> you need to also set NAME_FIELD to 'id', as the default is 'name'.
Noted.

> "turn off auditing and exclude these objects from memcached"
I copied this wording verbatim, thanks!

> please reformat the references to match the other entries.
Fixed.


> you also need to join using the current user's id.
Woops! I fixed this, and I created other users and verified that a given user only sees their own last visited bugs.



> we don't hit console enough to warrant this; instead just add an if
> (console) to your failure code.
Ok, fixed.

> only add js/bug.js to this list for authenticated show_bug requests:
Ok. 

> assigned_to and reporter are mandatory fields, there's no need to check that
> they are set first.
> 
> as per comment 52, you also need to check qa_contact (which isn't mandatory,
> and may be disabled via a param), and the cc list.
> 
> i'd also like to see this wrapped in an outer [% IF user.id %] check.

Done and done. Rather than write a large conditional in the template, I added a method to Bugzilla::User - $user->is_following_bug($bug), which encapsulates the described logic. Perhaps the name could be better.

> nit: this can be FILTER none

Done. The day someone alters bug_id to be a VARCHAR or TEXT and injects malicious javascript into show_bug, though... I'm going to point fingers.

Comment 71

[:glob ✱]

Comment on attachment 8398400 [details] [diff] [review]
bug-489028-v4.patch

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

::: Bugzilla/Field.pm
@@ +256,4 @@
>       type => FIELD_TYPE_BUG_URLS},
>      {name => 'tag',                   desc => 'Personal Tags', buglist => 1,
>       type => FIELD_TYPE_KEYWORDS},
> +    {name => 'last_visit_ts',         desc => 'Last Visit', buglist => 1,

you also need an entry in template/en/default/global/field-descs.none.tmpl for localisation.

::: Bugzilla/Search.pm
@@ +353,5 @@
>          creation_ts => \&_datetime_translate,
>          deadline    => \&_date_translate,
>          delta_ts    => \&_datetime_translate,
> +
> +        # last_visit field that accept both a 1d, 1w, 1m, 1y format and the %last_changed% pronoun.

wrap this comment to 80 chars

::: Bugzilla/User.pm
@@ +726,5 @@
> +
> +    return Bugzilla::BugUserLastVisit->match({ user_id => $self->id });
> +}
> +
> +sub is_following_bug {

"following" doesn't seem right.

TODO: come up with a better name, such as is_involved_in_bug.  we can brainstorm a name in this week's chat.

@@ +738,5 @@
> +        push(@user_ids, $bug->qa_contact->id);
> +    }
> +    push(@user_ids, map { $_->id } @{ $bug->cc_users });
> +
> +    return any { $self->id == $_ } @user_ids;

creating an array of user objects and greping it is unnecessary overhead.

instead i'd prefer to see:

my $user_id = $self->id;
return unless $user_id;
return 1 if $user_id == $bug->assigned_to->id;
...

calling $bug->cc_users creates user objects for everyone on the cc list, which we should avoid.  $bug->cc loads just the user's login, and will have already been initialised by the show_bug template:
my $login = $self->login;
return 1 if grep { $_ eq $login } @{ $bug->cc };

@@ +2791,5 @@
> +User is the reporter of the bug
> +
> +=item *
> +
> +User is the qa contact of the bug (if Bugzilla is configured to use a qa contact)

nit: "QA" not "qa"

::: Bugzilla/WebService/BugUserLastVisit.pm
@@ +33,5 @@
> +
> +    my @results;
> +    my $last_visit_ts = $dbh->selectrow_array('SELECT NOW()');
> +    foreach my $bug_id (@$ids) {
> +        # Should we check $user->is_following_bug() here in addition to the check in template-land?

nit: code isn't the place for these question :)  use bug comments.

answer: yes.  never rely on the client doing the right thing.  throw an error if called with a bug the user isn't associated with.

@@ +35,5 @@
> +    my $last_visit_ts = $dbh->selectrow_array('SELECT NOW()');
> +    foreach my $bug_id (@$ids) {
> +        # Should we check $user->is_following_bug() here in addition to the check in template-land?
> +        my $bug = Bugzilla::Bug->check({ id => $bug_id, cache => 1 });
> +        $bug->update_user_last_visit($user, $last_visit_ts);

you need a transaction block around update_user_last_visit(), as it makes multiple database calls.

::: Bugzilla/WebService/Server/REST/Resources/BugUserLastVisit.pm
@@ +46,5 @@
> +
> +This part of the Bugzilla REST API allows you to lookup and update the last time a user visited a bug.
> +
> +See L<Bugzilla::WebService::BugUserLastVisit> for more details on how to use this part of
> +the REST API.

wrap perldoc to 80 chars

::: clean-bug-user-last-visit.pl
@@ +7,5 @@
> +# defined by the Mozilla Public License, v. 2.0.
> +
> +################################################################################
> +# Script Initialization
> +################################################################################

nit: you don't need this header.
replace it with a small blurb that describes what this script does.

@@ +11,5 @@
> +################################################################################
> +use 5.10.1;
> +use strict;
> +use warnings;
> +use lib '.';

use lib qw(. lib);

@@ +14,5 @@
> +use warnings;
> +use lib '.';
> +
> +use Bugzilla;
> +

also set the usage_mode so things fail correctly:
Bugzilla->usage_mode(USAGE_MODE_CMDLINE);

@@ +16,5 @@
> +
> +use Bugzilla;
> +
> +my $dbh           =  Bugzilla->dbh;
> +my $keep_days     =  Bugzilla->params->{last_visit_keep_days} // 120;

i don't see any code where this param is created or added to the admin ui.
not sure what section it belongs in, maybe 'administrative policies'.

edit Bugzilla/Config/Admin.pm and template/en/default/admin/params/admin.html.tmpl.

@@ +20,5 @@
> +my $keep_days     =  Bugzilla->params->{last_visit_keep_days} // 120;
> +my $keep_days_ago =  $dbh->sql_date_math('NOW()', '-', $keep_days, 'DAY');
> +my $sql           =  qq{ DELETE FROM bug_user_last_visit
> +                         WHERE last_visit_ts < $keep_days_ago };
> +$dbh->do($sql);

nit: no need for $keep_day_ago, $sql, or qq{} here, just call $dbh->do() with the sql directly:
$dbh->do("DELETE FROM bug_user_last_visit WHERE last_visit_ts < " .
         $dbh->sql_date_math('NOW()', '-', $keep_days, 'DAY');

you also need to edit Bugzilla/Install/Filesystem.pm to ensure this file's permissions get set correctly (copy whine.pl's config).

::: js/bug.js
@@ +119,4 @@
>      }
>  };
>  
> +

nit: no need to add a blank line here.

@@ +202,5 @@
> +                method: 'BugUserLastVisit.update',
> +                params: { ids: bug_id },
> +            });
> +            var callbacks = {
> +                failure: function(res) { if (console) console.log("failed to update last visited: " + res.responseText); },

wrap to 80 chars

@@ +217,5 @@
> +                params: { },
> +            });
> +            var callbacks = {
> +                success: function(res) { done(JSON.parse(res.responseText)) },
> +                failure: function(res) { if (console) console.log("failed to get last visited: " + res.responseText); },

wrap to 80 chars

Comment 72

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

Attachment: bug-489028-v5.patch

(In reply to Byron Jones ‹:glob› from comment #71)

> wrap this comment to 80 chars
This and all the others done. 
:set textwidth=80 colorcolumn=+1,+2 makes this very obvious.

> TODO: come up with a better name, such as is_involved_in_bug.  we can
> brainstorm a name in this week's chat.
is_involved_in_bug() is now used.

> instead i'd prefer to see:
Done. I was going to suggest "return undef" for the case of a false user id,
because I dislike calling predicate functions in a constructor and having
nothing returned, but "return undef" is bad according to Perl::Critic, so I
went with it.

> answer: yes.  never rely on the client doing the right thing.  throw an
> error if called with a bug the user isn't associated with.
Okay, the error is "user_not_involved", but I feel you may have a better suggestion so I bring it up here.

> you need a transaction block around update_user_last_visit(), as it makes
> multiple database calls.
Done. Note that I am also careful to catch errors.

> nit: you don't need this header.
> replace it with a small blurb that describes what this script does.

Replaced with a POD comment.

> i don't see any code where this param is created or added to the admin ui.
> not sure what section it belongs in, maybe 'administrative policies'.
I have added it to that section and tested it.

> nit: no need for $keep_day_ago, $sql, or qq{} here, just call $dbh->do()
> with the sql directly:
> $dbh->do("DELETE FROM bug_user_last_visit WHERE last_visit_ts < " .
>          $dbh->sql_date_math('NOW()', '-', $keep_days, 'DAY');
Done, it was harder formatting this onto 80 lines (uglier).

> you also need to edit Bugzilla/Install/Filesystem.pm to ensure this file's
> permissions get set correctly (copy whine.pl's config).
With the same permissions as whine.pl

(anything that I have not replied to has been fixed but a reply was not warranted.)

Comment 73

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

> > instead i'd prefer to see:
> Done. I was going to suggest "return undef" for the case of a false user id,
> because I dislike calling predicate functions in a constructor and having
> nothing returned, but "return undef" is bad according to Perl::Critic, so I
> went with it.
it being a bare return.

Comment 74

[:glob ✱]

Comment on attachment 8399807 [details] [diff] [review]
bug-489028-v5.patch

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

very close now; r- due to the error handling in the webservice.

::: Bugzilla/Config/Admin.pm
@@ +42,1 @@
>    });

this needs a 'checker' to ensure the value is always numeric:

  checker => \&check_numeric,

::: Bugzilla/WebService/BugUserLastVisit.pm
@@ +54,5 @@
> +        1;
> +    } or do {
> +        my $err = $@;
> +        $dbh->bz_rollback_transaction();
> +        die $err;

you don't need to use eval{} and explicitly rollback - that's handled automatically by our database layer.  look at how transactions are used in the rest of the bugzilla code.

::: clean-bug-user-last-visit.pl
@@ +14,5 @@
> +
> +This utility script cleans out entries from the bug_user_last_visit table that
> +are older than (a configurable) number of days.
> +
> +It takes no arguments and produces no output except in the case of errors;

nit: s/;$/./

::: template/en/default/admin/params/admin.html.tmpl
@@ +24,4 @@
>                         "Bugzilla will issue a warning in case you'd run into inconsistencies " _
>                         "when you're about to do so, but such deletions remain kinda scary. " _
>                         "So, you have to turn on this option before any such deletions " _
> +                       "will ever happen." 

nit: remove trailing whitespace

Comment 75

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

Attachment: bug-489028-v6.patch

(In reply to Byron Jones ‹:glob› from comment #74)
>   checker => \&check_numeric,
Aah, that's why there is no "type" for numbers.

> you don't need to use eval{} and explicitly rollback - that's handled
> automatically by our database layer.  look at how transactions are used in
> the rest of the bugzilla code.
This is me with egg on my face. Fixed.

> nit: remove trailing whitespace
Like little gnats... I guess I need visible whitespace for html too.

Comment 76

[:glob ✱]

Comment on attachment 8402479 [details] [diff] [review]
bug-489028-v6.patch

obsoleting attachment per our discussion last night.

- comment tags need to be visible in the bug and user history
- pod docs required for the new webservice method

Comment 77

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

(In reply to Byron Jones ‹:glob› from comment #76)
> Comment on attachment 8402479 [details] [diff] [review]
> bug-489028-v6.patch
> 
> obsoleting attachment per our discussion last night.
> 
> - comment tags need to be visible in the bug and user history
> - pod docs required for the new webservice method

Wrong bug. un-obsoleted. My fault. :)

Comment 78

[:glob ✱]

Comment on attachment 8402479 [details] [diff] [review]
bug-489028-v6.patch

r=glob \o/

Comment 79

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

Does approval in this mean I should commit the patch to bugzilla/bugzilla? to master?

Comment 80

[Frédéric Buclin]

(In reply to Dylan William Hardison [:dylan] from comment #79)
> Does approval in this mean I should commit the patch to bugzilla/bugzilla?
> to master?

No wait, we started the release process. The patch will be committed only after releases are out. We don't want to take the risk to break something now.

Comment 81

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

To [e-mail]:bugzilla/bugzilla.git
   36f56bd..eab44b1  master -> master

Comment 82

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

*facepam*

Comment 83

[Boris Zbarsky [:bzbarsky]]

So is it expected that when the query is selected the little "subtitle" text above the buglist says: "Bugs updated since list visited"?  In particular "list" vs "last"?

Comment 84

[Mark Côté [:mcote]]

No, that was a mistake, now fixed; see bug 1020425.

Comment 85

[Frédéric Buclin]

Added to relnotes for 5.0rc1.