1000917 - Backport upstream bug 489028 to bmo/4.2 to allow user last visit searching

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: bug-1000917-v1.patch

Testing this patch first, afterwards will flag for review.

Comment 2

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

Attachment: bug-1000917-v2.patch

Yep, my first patch was definitely broken. This one works.

Comment 3

[:glob ✱]

Comment on attachment 8413467 [details] [diff] [review]
bug-1000917-v2.patch

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

the backport code looks great, however there's two more things i'd like to see landed at the same time:

- add the bug_user_last_visit table to the list of tables sanitised by contrib/sanitizeme.pl
- add a new query to 'my dashboard' for "Bugs updated since list visited" (or similar wording)

Comment 4

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

Attachment: bug-1000917-v3.patch

- add bug_user_last_visit to contrib/sanitizeme.pl -- used TRUNCATE TABLE.

- add a new query to 'my dashboard'

Note that the earlier version of this patch would have removed 'tag' from the default fields!

Comment 5

[:glob ✱]

Comment on attachment 8425316 [details] [diff] [review]
bug-1000917-v3.patch

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

r=glob with the change to the heading to be fixed on commit.

you also need to file a bug in infra:webops-bugzilla to schedule clean-bug-user-last-visit.pl to run nightly.

::: extensions/MyDashboard/lib/Queries.pm
@@ +92,5 @@
>              }
>          },
> +        {
> +            name => 'lastvisitedbugs',
> +            heading => 'Bugs Since Last Visit',

that heading doesn't make sense.

how about: "Updated Since Last Visit"

Comment 6

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

To [e-mail]:webtools/bmo/bugzilla.git
   038e685..b6b83df  master -> master

Comment 7

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

This needs to go in as two commits, with the schema change split out from the rest. Reverting the commit.
To [e-mail]:webtools/bmo/bugzilla.git
   b6b83df..7ea36c8  master -> master

Comment 8

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

To [e-mail]:webtools/bmo/bugzilla.git
   7ea36c8..24a16ae  master -> master

Schema change pushed.

Comment 9

[Philippe M. Chiasson (:gozer)]

Wouldn't it make more sense and remove the need for a cleanup cron-job if the various methods that update the bug_user_last_visit table also ran the deletion/cleanup query at the same time?

Something like:

DELETE FROM
    bug_user_last_visit 
WHERE
    user_id = ?
  AND
    last_visit_ts < NOW() - Bugzilla->params->{last_visit_keep_days} DAY

Comment 10

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

That wouldn't remove last visits for users that don't use bugzilla often, and would be more queries happening. One query every 24 hours vs. one query per show_bug page load...

Comment 11

[:glob ✱]

(In reply to Philippe M. Chiasson (:gozer) from comment #9)
> Wouldn't it make more sense and remove the need for a cleanup cron-job.

i don't see the benefit in taking the hit of running that with every request in comparison to a cronjob.
setting up a cronjob is trivial, and shifts that load outside of the functional code.

(reopening as a reminder to me to commit the 2nd part of the patch during the push).

Comment 12

[:glob ✱]

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

Comment 13

[:glob ✱]

this caused bugs with an empty cc list to throw errors:

Can't use an undefined value as an ARRAY reference (Bugzilla/User.pm:773)

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   1154c57..7cdfaf1  master -> master


filed upstream as bug 1019476