387586 - Implement the ability to get all bug changes made by some given user in some given time range

Status: ASSIGNED

(Bugzilla :: Query/Bug List, enhancement, P3)

Description

[Frédéric Buclin]

I wanted to track all bugs edited by some given user in the last x days, and realized I couldn't, because you have to tell which fields you are looking at. In my case, *any* field will do it (commenting, adding himself to the CC list, etc...), so "foo" "changed by" "[e-mail]" doesn't work as "foo" would be "any field".

I would like to be able to do this anyway. Note that the SQL query is very simple as you only have to worry about the 'who' and 'bug_when' columns of the 'bugs_activity' table.

Comment 1

[Wayne Mery (:wsmwk)]

similarish thought, Bug 121335 – Implement "last changed by"

Comment 2

[Wayne Mery (:wsmwk)]

ditto Bug 214851 – boolean charts: "Any field" [changed]

Comment 3

[Frédéric Buclin]

Max, Romaia, you implemented this as part of req7 on b.m.o, in canned reports. Could you imlement it upstream?

Comment 4

[Ronaldo Maia]

Attachment: v1 - romaia

Comment 5

[Frédéric Buclin]

Comment on attachment 317349 [details] [diff] [review]
v1 - romaia

This patch doesn't pass tests:

t/008filter..........NOK 61
#   Failed test '(en/default) template/en/default/bug/activity/show.html.tmpl has unfiltered directives:
#   67: bug_id
#   68:+ bug_id
# --ERROR'

t/009bugwords........NOK 60
#   Failed test 'template/en/default/bug/activity/table.html.tmpl contains invalid bare words (e.g. 'bug') --WARNING'

t/010dependencies....NOK 3
#   Failed test 'Dependency on Bugzilla::Util from Bugzilla::Error causes loop. --ERROR'
t/010dependencies....NOK 131
#   Failed test 'Dependency on Bugzilla::Error from Bugzilla::Util causes loop. --ERROR'


Failed Test         Stat Wstat Total Fail  Failed  List of Failed
-------------------------------------------------------------------------------
t/008filter.t          1   256   259    1   0.39%  61
t/009bugwords.t        1   256   259    1   0.39%  60
t/010dependencies.t    2   512   287    2   0.70%  3 131

The dependency loop is the more critical problem.

Comment 7

[u81239]

Instead of ‘in the last x days’ I’d prefer to be able to search for a range of dates. Basically, add a ‘by user’ field to the Bug Changes section of the search page.

Basically, of the bug activity information (e.g. https://bugzilla.mozilla.org/show_activity.cgi?id=440678 ), we can currently search on the When, What and Added columns. The ability to search by Who (and perhaps also Removed?) should be added.

Comment 8

[Frédéric Buclin]

bmo has this feature implemented as "User Activity Report", see https://bugzilla.mozilla.org/page.cgi?id=user_activity.html. This bug is about having this implemented upstream. glob, dkl: would you agree to contribute the code upstream (not as an extension)?

Comment 9

[:glob ✱]

frédéric, that makes sense, will do.

Comment 11

[Max Kanat-Alexander]

I'm guessing glob will do this since he commented above?

Comment 12

[:glob ✱]

Attachment: patch v1

Comment 13

[Max Kanat-Alexander]

You know, I really want this feature, but I'd rather see it as a search form that appears on show_activity.cgi. That is, let people load show_activity.cgi with no "id" parameter, and have the search form appear. And then have the search form pre-populated on show_activity.cgi when people come to it from a bug page.

Comment 14

[:glob ✱]

(In reply to comment #13)
> You know, I really want this feature, but I'd rather see it as a search form
> that appears on show_activity.cgi. That is, let people load
> show_activity.cgi with no "id" parameter, and have the search form appear.
> And then have the search form pre-populated on show_activity.cgi when people
> come to it from a bug page.

are you proposing mirroring query.cgi by having two tabs; one for bug history, and one for user history?

Comment 15

[:glob ✱]

Attachment: patch v2

merging the two reports from a code perspective was the right thing to do, thanks max.

i played around with merging the two reports from a ui point of view, but couldn't get it working with a result i was happy with, so i ditched that idea.

Comment 16

[:glob ✱]

Attachment: patch v3

patch v2 with correct boilerplate on table.html.tmpl, and with table.html.tmpl.old deleted.

Comment 17

[Max Kanat-Alexander]

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

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

::: Bugzilla/Users.pm
@@ +21,5 @@
> +#   Byron Jones <[e-mail]>
> +#
> +# ***** END LICENSE BLOCK *****
> +
> +package Bugzilla::Users;

This is a very confusing name for a module; developers won't understand the difference between this and Bugzilla::User. Could this possibly be Bugzilla::Bug::Activity instead?

Comment 18

[Max Kanat-Alexander]

(In reply to comment #14)
> are you proposing mirroring query.cgi by having two tabs; one for bug
> history, and one for user history?

  FWIW, what I was proposing here was that when you loaded show_activity.cgi?id=1234, you would see:

Bug ID: [1234     ] User: [       ] From: [       ][Cal] To: [        ][Cal]

And then the activity table below it. When a bug id was selected, the "bug id" column would not appear. When a user was selected, the "user" column would not appear.

Comment 19

[:glob ✱]

the problem with combining the two reports is the output from the reports differs in more than just the bug column:  the user activity report includes comments and attachments, which are not shown on the bug history.

because of this, it would be confusing to merge the two reports, and it doesn't make sense to include the comments and attachments on the bug history output.

(In reply to comment #17)
> This is a very confusing name for a module; developers won't understand the
> difference between this and Bugzilla::User. Could this possibly be
> Bugzilla::Bug::Activity instead?

if anything it would be Bugzilla::User::Activity.  i can easily make that change, but i'll hold off on doing so until there's a review on the current patch.

Comment 20

[Frédéric Buclin]

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

>=== added file 'Bugzilla/Users.pm'

I agree with mkanat that Bugzilla::User::Activity would make more sense. Please rename this module.


Also, when we look at the bug activity of a bug, there is no Bug ID column, which makes sense as it would always contain the same ID. We should do the same when looking at the user activity, i.e. do not display the "Who" column. Is there really no way to easily combine both templates? You could add a [% IF mode == 'user_activity' %] or [% IF mode == 'bug_activity' %] to decide which column or information to display.

Also, I'm denying review because the UI exploded when I did a query. Screenshot coming.

Comment 21

[Frédéric Buclin]

Attachment: screenshot

Here is what I see... :-/

Comment 22

[:glob ✱]

(In reply to Frédéric Buclin from comment #20)
> Comment on attachment 541329 [details] [diff] [review]
> Also, when we look at the bug activity of a bug, there is no Bug ID column,
> which makes sense as it would always contain the same ID. We should do the
> same when looking at the user activity, i.e. do not display the "Who"
> column.

yes, we can make the 'who' column visible if you're only reporting on a single user.

> Also, I'm denying review because the UI exploded when I did a query.

indeed.

Comment 23

[Max Kanat-Alexander]

Also, for the record, I'd much prefer to have objects that represent individual rows of bugs_activity and use those. That's part of the Bugzilla::ChangeList thing that LegNeato was working on.

Comment 26

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is too invasive or too risky to be accepted for 4.4 at this point. The target milestone is set to 5.0 which is our next major release.

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 on time for 5.0.

Comment 27

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

Attachment: patch v4

Comment 28

[Frédéric Buclin]

Comment on attachment 8346597 [details] [diff] [review]
patch v4

>+++ b/Bugzilla/User/Activity.pm

>+# defined by the Mozilla Public License, v. 2.0.
>+package Bugzilla::User::Activity;

Please add a newline before |package ....|.


>+use strict;

Move this line before |use Bugzilla::Util|. You must also add |use 5.10.1|.



>diff --git a/show_activity.cgi b/show_activity.cgi

> my $cgi = Bugzilla->cgi;
> my $template = Bugzilla->template;
>+my $input = Bugzilla->input_params;

I don't see why you use Bugzilla->input_params when you already have the data available with $cgi. $input->{foo} can be replaced by $cgi->param('foo'). No CGI script use Bugzilla->input_params.



>diff --git a/template/en/default/activity/show-user.html.tmpl b/template/en/default/activity/show-user.html.tmpl

>+[%# ***** BEGIN LICENSE BLOCK *****
>+  # Version: MPL 1.1
>+  #
>+  # The contents of this file are subject to the Mozilla Public License Version
>+  # 1.1 (the "License"); you may not use this file except in compliance with
>+  # the License. You may obtain a copy of the License at
>+  # http://www.mozilla.org/MPL/

You must use the MPL 2.0 license.



>diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl

>+      You must provided the period start date.

s/provided/provide/


>+      You must provided the period end date.

Same here.


Note that I didn't review your code at all. Was just looking at your patch globally. :)

Comment 29

[:glob ✱]

Comment on attachment 8346597 [details] [diff] [review]
patch v4

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

in addition to lpsolit's comment..

make sure you run tests prior to attaching a patch.
#   Failed test 'Bugzilla/User/Activity.pm DOES NOT require Perl 5.10.1 --WARNING'
#   at t/002goodperl.t line 105.
#   Failed test 'Bugzilla/User/Activity.pm POD coverage is 0%'
#   at t/011pod.t line 117.

there's a lot of trailing whitespace in this patch, please remove all occurrences.

the issue shown in lpsolit's screenshot is still present. looks like it's caused by operation.changes.size being zero.
i can replicate this using landfill-tip's database and
show_activity.cgi?action=run&format=user&who=landfilltip%40justdave.net&from=2003-11-07&to=2003-12-01

::: Bugzilla/User/Activity.pm
@@ +161,5 @@
> +            $activity_visible = Bugzilla->user->is_timetracker;
> +        }
> +        elsif ($fieldname eq 'longdescs.isprivate'
> +                && !Bugzilla->user->is_insider 
> +                && $added) 

nit: line up the && with the $

::: show_activity.cgi
@@ +88,5 @@
>  
> +    my $bug = Bugzilla::Bug->check($id);
> +    Bugzilla->switch_to_shadow_db;
> +    ($vars->{'operations'}, $vars->{'incomplete_data'}) = 
> +        Bugzilla::Bug::GetBugActivity($bug->id);

GetBugActivity doesn't exist.  you want $bug->get_activity

::: template/en/default/activity/show-user.html.tmpl
@@ +26,5 @@
> +[% INCLUDE global/header.html.tmpl
> +  title = "User Activity Report"
> +  yui = [ 'autocomplete', 'calendar' ]
> +  javascript_urls = [ "js/util.js", "js/field.js" ]
> +  style_urls = ['skins/standard/activity.css']

activity.css is missing from this patch

Comment 30

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

I'm making those changes and should have a revised patch up soon. My first attempt was a straight forward-port from the v3 patch to the bugzilla trunk.

Comment 31

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

hmm, I can't find the activity.css in the previous patches as well. Any idea what it should look like?

Comment 32

[:glob ✱]

(In reply to Dylan William Hardison [:dylan] from comment #31)
> hmm, I can't find the activity.css in the previous patches as well. Any idea
> what it should look like?

i don't have a copy of it any more, however i suspect it would be styling to make it look like bmo's user activity report.

Comment 33

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

Attachment: rowspan-at-least-1.png

Setting the rowspawn to changes.size || 1 seems to partially correct it. I wonder if it would be better to insert a "no changes" row in this case?

Comment 34

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

Attachment: patch v5

Updated patch; code is more tidy and changes.size == 0 no longer causes table formatting errors, although it may not be the optimum solution.

Comment 35

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

I still need to resurrect the missing .css, but I wanted to get my cleaned-up version of the code out there.

Comment 36

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

Attachment: patch v6

This patch contains the cleaned up perl code, the previous (v5) was a mistaken upload.

Comment 37

[Frédéric Buclin]

Comment on attachment 8350699 [details] [diff] [review]
patch v6

>a/Bugzilla/User/Activity.pm

>+    if (Bugzilla->params->{"insidergroup"} && !Bugzilla->user->is_insider) {
>+        $activity_joins = "LEFT JOIN attachments
>+                   ON attachments.attach_id = bugs_activity.attach_id";
>+        $activity_where = "AND COALESCE(attachments.isprivate, 0) = 0";
>+        $attachments_where = $activity_where;
>+    }

Move this call after global/confirm-user-match.html.tmpl as this block is useless till we have validated users. Also, write $user = Bugzilla->user and then use $user->foo instead of repeating Bugzilla->user all the time. That's cleaner. :)


>+        my %vars = (
>+            script        => $cgi->url( -relative => 1 ),
>+            fields        => {},
>+            matches       => [],
>+            matchsuccess  => 0,
>+            matchmultiple => 1,
>+        );
>+
>+        print $cgi->header();
>+        $template->process("global/confirm-user-match.html.tmpl", \%vars)
>+            or ThrowTemplateError($template->error());
>+
>+        exit;

Call Bugzilla::User::match_field() instead.


>+    my @params = map { ( @who, $from . '00:00:00', $to . ' 23:59:59' ) } 1 .. 4;

It's totally unclear why you use 1..4. Could you add a comment, please?


>+    my $comment_filter =
>+        Bugzilla->user->is_insider ? '' : 'AND longdescs.isprivate = 0';

Does it correctly take comment tags into account? See bug 952284.


>+     LEFT JOIN fielddefs

Are there fields which are not in fielddefs? If not, you should use INNER JOIN instead of LEFT JOIN.


>+    UNION ALL

Instead of all these UNION ALL, wouldn't it be more efficient to call them separately and let Perl concatenate data? This also means you wouldn't need to add fake columns to get exactly the same columns even when they are not needed.


>+            || $fieldname eq 'deadline')
>+        {
>+            $activity_visible = Bugzilla->user->is_timetracker;

The deadline is no longer restricted to members of the timetracking group, see bug 718289.


>+        elsif ($fieldname eq 'longdescs.isprivate'
>+               && !Bugzilla->user->is_insider
>+               && $added)
>+        {
>+            $activity_visible = 0;

Isn't this already taken into account thanks to $comment_filter?



>a/show_activity.cgi

>+    if ($action) {

I see nowhere where the value of $action matters. So what's the purpose of it??


>+        $from = ymd_of_date($from);

Why not using Bugzilla::Util::format_time()?


>+        Bugzilla->switch_to_shadow_db;

I think this should be moved into Bugzilla::User::Activity so that the caller doesn't have to remember to do it itself.


>+        $from ||= DateTime->now->truncate(to => 'month')->ymd;
>+        $to   ||= DateTime->now->truncate(to => 'day')->ymd;

Use Bugzilla::Util::format_time() if possible.


>+    die "invalid format: $format";

No die. Use ThrowUserError() instead.


>+my $templateFormat = $template->get_format( 'activity/show', $format, 'html' );

Use $foo_bar instead of $fooBar.



>a/template/en/default/activity/show-bug.html.tmpl

>+  style_urls = ['skins/standard/activity.css']

Please don't create a new CSS file. We already have way too many of them. This slows down the page load as the web browser can only fetch a few (typically: 4) files at once.


>+  Back to [% "$terms.bug $bug.bug_id" FILTER bug_link(bug) FILTER none %]

It would be good from a performance point of view to call it only once, and store it if needed.


>+[% IF operations.size > 0 %]
>+  <p>
>+    Back to [% "$terms.bug $bug.bug_id" FILTER bug_link(bug) FILTER none %]
>+  </p>
>+[% END %]

See the comment above.


>+++ b/template/en/default/activity/show-user.html.tmpl

>+[% PROCESS "global/field-descs.none.tmpl" %]
>+[% PROCESS bug/time.html.tmpl %]

You don't need them, AFAIK.


>+<form id="activity_form" name="activity_form" action="show_activity.cgi" method="get">
>+<input type="hidden" name="action" value="run">
>+<input type="hidden" name="format" value="user">
>+<table>
>+<tr>
>+  <td>

Fix the indentation. It must be:

<form ...>
  <input ...>
  <input ...>

  <table ...>
    <tr>
      <td>

Also, add an id to the table for styling.


>+  <td>
>+    <b>Who</b>:
>+  </td>

It's a header, so write <th>Who:</th> instead.


>+  <td>
>+    <b>Period</b>:
>+  </td>

Same here.


>+    <input type="text" id="from" name="from" size="11"
>+      align="right" value="[% from FILTER html %]" maxlength="10"

size should be set to 10 as the maxlength is 10. Also, drop align="right". They are illegal in HTML5, see bug 840407. You should wait for bug 840407 to be fixed before going further, so that you can copy the new "format" compatible with HTML5. In that bug, I also clean up templates a lot to remove hacks and old stuff.



>+++ b/template/en/default/activity/table.html.tmpl

>+  <table border="1" cellpadding="4" cellspacing="0" id="report">

cellpadding and cellspacing are illegal in HTML5, see bug 920681. Here too, you should wait for bug 840407 to be fixed before going further.


>+        <td rowspan="[% operation.changes.size || 1 %]" valign="top">

valign is illegal in HTML5 too. See above.


>+        [% FOREACH change = operation.changes %]
>+          [% "</tr><tr>" IF loop.index > 0 %]

This is really a hack. Each block should be self-consistent and should open and close its own <tr>.



>--- a/template/en/default/bug/activity/table.html.tmpl

>-  <table border cellpadding="4">

Your patch is bitrotten due to bug 920681.



>+++ b/template/en/default/reports/menu.html.tmpl

>+<h2>Historical</h2>

I'm not a fan of this title. Maybe it could be included in the existing "Change Over Time" section.


>+  <li id="report_useractivity">

"report_user_activity" to more easily parse this string.


Moreover, your patch doesn't pass tests:

t/008filter.t ........ 1/290 
#   Failed test '(en/default) template/en/default/activity/table.html.tmpl has unfiltered directives:
#   50: operation.changes.size || 1 
#   53: operation.changes.size || 1 
#   57: operation.changes.size || 1 
# --ERROR'
#   at t/008filter.t line 116.

t/011pod.t ........... 220/373 
#   Failed test 'Bugzilla/User/Activity.pm POD coverage is 0%'
#   at t/011pod.t line 117.


I applied your patch, and it crashed when displaying the activity of a user:

Insecure dependency in parameter 3 of DBI::db=HASH(0xa278c90)->selectall_arrayref method call while running with -T switch at Bugzilla/User/Activity.pm line 134.
 at Bugzilla/User/Activity.pm line 134.
	Bugzilla::User::Activity::get_users_activity('ARRAY(0xa0d85d8)', '2013-12-19', '2014-01-02') called at /var/www/html/bugzilla/show_activity.cgi line 51

Your data is tainted, which means input has not been validated.

Comment 38

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

Ah, my mod perl config wasn't enabling taint mode. That's fixed now.

Comment 39

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

Attachment: bug-387586-v7.patch

Not requesting review yet; I still need to make the table not rely on the loop.count hack and also replace the "die" with proper error handling (which I had to figure how that works in bugzilla, but I have a handle on it now).

That said, this v7 patch shows that I fixed the bit rot and applied the other suggestions. It also runs under taint mode.

Comment 40

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

Update: v8 of this patch (incorporating the cleaner table generation and Bugzilla-standard error handling should be done tomorrow mid-day Eastern/US.

Comment 41

[Frédéric Buclin]

(In reply to Dylan William Hardison [:dylan] from comment #40)
> Update: v8 of this patch (incorporating the cleaner table generation and
> Bugzilla-standard error handling should be done tomorrow mid-day Eastern/US.

tomorrow of which day? :)

Comment 42

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

Attachment: bug-387586-v8.patch

Sorry, everything is quite new and I was trying to gain some ground on another bug. It is now tomorrow.

Comment 43

[:glob ✱]

Comment on attachment 8373917 [details] [diff] [review]
bug-387586-v8.patch

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

there are a lot of issues from lpsolit's review in comment 37 that have not been addressed (mostly in Activity.pm).
i'll wait for an updated revision before doing a full review.

in a lot of places you have extra spaces within parentheses:
wrong: if ( $format eq 'user' ) {
right: if ($format eq 'user') {
this applies to conditional statements, method calls, anon arrays, etc.

::: template/en/default/activity/show-user.html.tmpl
@@ +16,5 @@
> +  <input type="hidden" name="action" value="run">
> +  <input type="hidden" name="format" value="user">
> +  <table>
> +    <tr>
> +      <th> Who: </th>

don't add the extra spaces within the <th>:  <th>Who:</th>.
this apples to all <th> elements in this template.

Comment 44

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

In the delay I must've overlooked the items from comment #37, going over those. Paren-spaces is another tweak to perltidyrc. Thanks!

Comment 45

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

Attachment: bug-387586-v9.patch

(In reply to Byron Jones ‹:glob› from comment #43)
> there are a lot of issues from lpsolit's review in comment 37 that have not
> been addressed (mostly in Activity.pm).
> i'll wait for an updated revision before doing a full review.
Fully reviewed, see below.

(In reply to Frédéric Buclin from comment #37)
> Comment on attachment 8350699 [details] [diff] [review]
> patch v6
> 
> >a/Bugzilla/User/Activity.pm
> 
> >+    if (Bugzilla->params->{"insidergroup"} && !Bugzilla->user->is_insider) {
> >+        $activity_joins = "LEFT JOIN attachments
> >+                   ON attachments.attach_id = bugs_activity.attach_id";
> >+        $activity_where = "AND COALESCE(attachments.isprivate, 0) = 0";
> >+        $attachments_where = $activity_where;
> >+    }
> 
> Move this call after global/confirm-user-match.html.tmpl as this block is
> useless till we have validated users. 
The routine (per its comment) doesn't need to perform validation at all, so this stayed.
But the block below was removed.

> Also, write $user = Bugzilla->user and
> then use $user->foo instead of repeating Bugzilla->user all the time. That's
> cleaner. :)

Definitely! Fixed.

> >+        my %vars = (
> >+            script        => $cgi->url( -relative => 1 ),
> >+            fields        => {},
> >+            matches       => [],
> >+            matchsuccess  => 0,
> >+            matchmultiple => 1,
> >+        );
> >+
> >+        print $cgi->header();
> >+        $template->process("global/confirm-user-match.html.tmpl", \%vars)
> >+            or ThrowTemplateError($template->error());
> >+
> >+        exit;
> 
> Call Bugzilla::User::match_field() instead.
We call that from the CGI script (show_activity.cgi) so that above block was useless.

> >+    my @params = map { ( @who, $from . '00:00:00', $to . ' 23:59:59' ) } 1 .. 4;
> 
> It's totally unclear why you use 1..4. Could you add a comment, please?

I added a verbose command and simplified the expression.

> 
> >+    my $comment_filter =
> >+        Bugzilla->user->is_insider ? '' : 'AND longdescs.isprivate = 0';
> 
> Does it correctly take comment tags into account? See bug 952284.

I spent a rather large amount of time trying to answer this question.
Anything that is in the collapsed tags configuration paramter will have isprivate set.

Currently the addition of comments does not appear in bugs_activity, so there is nothing to leak
there as far as information goes.

> 
> 
> >+     LEFT JOIN fielddefs
> 
> Are there fields which are not in fielddefs? If not, you should use INNER
> JOIN instead of LEFT JOIN.

There do not appear to be! So INNER JOIN it is.

> 
> 
> >+    UNION ALL
> 
> Instead of all these UNION ALL, wouldn't it be more efficient to call them
> separately and let Perl concatenate data? This also means you wouldn't need
> to add fake columns to get exactly the same columns even when they are not
> needed.

The UNION ALL is 13 to 20% faster than 4 select queries.  The biggest win seems
to be in the way the resulting array is allocated with the UNION ALL approach.

> The deadline is no longer restricted to members of the timetracking group,
> see bug 718289.
Fixed.

> 
> 
> >+        elsif ($fieldname eq 'longdescs.isprivate'
> >+               && !Bugzilla->user->is_insider
> >+               && $added)
> >+        {
> >+            $activity_visible = 0;
> 
> Isn't this already taken into account thanks to $comment_filter?




> >a/show_activity.cgi
> 
> >+    if ($action) {
> 
> I see nowhere where the value of $action matters. So what's the purpose of
> it??

$action is now the name of the submit button, and its purpose is to show that a form submission
has occured. I'm open to other ways of acheiving this.

> Why not using Bugzilla::Util::format_time()?
Silly old me, not aware of Bugzilla::Util's functions.
> >+        Bugzilla->switch_to_shadow_db;

> I think this should be moved into Bugzilla::User::Activity so that the
> caller doesn't have to remember to do it itself.
Agreed.

> Use Bugzilla::Util::format_time() if possible.
Fixed.

> No die. Use ThrowUserError() instead.
Fixed

> Please don't create a new CSS file. We already have way too many of them.
> This slows down the page load as the web browser can only fetch a few
> (typically: 4) files at once.
Fixed.

> >+  Back to [% "$terms.bug $bug.bug_id" FILTER bug_link(bug) FILTER none %]
> 
> It would be good from a performance point of view to call it only once, and
> store it if needed.

We only call it twice?
I wonder if we could benefit from memoizing bug_link()...

Fixed.

> >+++ b/template/en/default/activity/show-user.html.tmpl
> 
> >+[% PROCESS "global/field-descs.none.tmpl" %]
> >+[% PROCESS bug/time.html.tmpl %]
> 
> You don't need them, AFAIK.

Yep.

> Fix the indentation. It must be:
Fixed.

> This is really a hack. Each block should be self-consistent and should open
> and close its own <tr>.

I'm not really sure that any other solution involving a table is less hacky than this.
Suggestions for removing the hack?

> "report_user_activity" to more easily parse this string.
Fixed.

Comment 46

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

In the midst of making the query return comment tags (and thus debugging the original show_activity.cgi) I am beginning to think get_user_activity() should be a method of Bugzilla::User ($user->get_activity()), to mirror $bug->get_activity()...

Comment 47

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

The hold up on this bug has been three-fold:

1) POD documentation for the get_user_activity function
2) Thoughts on structuring this so it looks less grafted-on (comment 46)
3) Adding comment tags.

For the later, I was under the mistaken impression they were supposed to be added to the bugs_activity table. Finally, I really looked at the existing Bug->get_activity and noticed, much to my annoyance, that they are added to the activity stream by adding a UNION ALL select against the comment tags table.

*shakes head*

It means a simple matter of programming to fix that and get this into review.

Comment 48

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

In order to add comment tagging to the user activity stream, there are now 5 queries joined by UNION ALL,
and I have come to the conclusion that this is going to be hard to maintain.

In the comments query, for instance, we need to select from longdescs_tags_activity again and compare it to the Bugzilla->params->{collapse_tags} list to check for spam.
There is also a Bugzilla hook so that extensions can remove items from the activity stream (needed for BMO's delete comments feature, which is implemented with a tag).

I did note that using a UNION ALL was nearly a 20% performance increase, but I think an honest refactoring of the now-five queries will be better. This is a maintenance nightmare waiting to happen otherwise.

I'm not going to upload the patch right now because it sucks.

Comment 49

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

Never underestimate the query planner... Building up a list of user ids first and issuing five distinct queries is actually up to 21% slower, although I think much easier to maintain. I will proceed with this and ask for feedback on the revised patch.

Comment 50

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

Attachment: bug-387586-v10.patch

What's changed:

1) comment tags are now included in user activity (*and* bug activity, which was a regression introduced by this patch set).
2) We're still using a UNION ALL on five (5) queries, because it is really quite fast. But the way the query is built now is, in my humble opinion, easier to maintain / debug / reason about.
3) POD documentation for the (internal) function

Comment 51

[:glob ✱]

Comment on attachment 8408352 [details] [diff] [review]
bug-387586-v10.patch

(In reply to Dylan William Hardison [:dylan] from comment #49)
> Never underestimate the query planner... Building up a list of user ids
> first and issuing five distinct queries is actually up to 21% slower,
> although I think much easier to maintain. I will proceed with this and ask
> for feedback on the revised patch.

i don't think splitting of the query in this way is worth the 21% performance hit.  please revert back to using UNION ALL.

sql comments may help with the readability there (that said, i don't consider the UNION ALL approach to be unreadable or hard to maintain).

Comment 52

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

(In reply to Byron Jones ‹:glob› from comment #51)
> Comment on attachment 8408352 [details] [diff] [review]
> bug-387586-v10.patch
> 
> (In reply to Dylan William Hardison [:dylan] from comment #49)
> > Never underestimate the query planner... Building up a list of user ids
> > first and issuing five distinct queries is actually up to 21% slower,
> > although I think much easier to maintain. I will proceed with this and ask
> > for feedback on the revised patch.
> 
> i don't think splitting of the query in this way is worth the 21%
> performance hit.  please revert back to using UNION ALL.

It is using the UNION all (per comment 50), but the query is built up while checking that each part has the correct number of arguments. I will undo that part, but in my opinion it is likely that errors will be introduced any time the queries have to be changed, or additional clauses to the UNION ALL are added. (The current, verbose solution avoided this problem.)

Comment 53

[:glob ✱]

Comment on attachment 8408352 [details] [diff] [review]
bug-387586-v10.patch

(In reply to Dylan William Hardison [:dylan] from comment #52)
> It is using the UNION all (per comment 50), but the query is built up while
> checking that each part has the correct number of arguments.

i completely miss-read the patch during my pre-coffee haze this morning.
sorry about that :(

(restoring the original review?dkl request)

Comment 54

[:glob ✱]

Comment on attachment 8408352 [details] [diff] [review]
bug-387586-v10.patch

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

here's a review without running the code.
i'm happy to review future revisions.

t/012throwables.t .... 1/213
#   Failed test 'Bugzilla/User/Activity.pm has 1 error(s):
# code error tag 'sql_generation_bug' is used at line(s) (311) but not defined for language(s): any'
#   at t/012throwables.t line 199.

#   Failed test '--WARNING template/en/default/global/user-error.html.tmpl has 1 unused error tag(s):
# user error tag 'user_activity_invalid_date' is defined at line(s) (1865) but is not used anywhere'
#   at t/012throwables.t line 203.

::: Bugzilla.pm
@@ +9,5 @@
>  
>  use 5.10.1;
>  use strict;
> +use FindBin '$Bin';
> +use local::lib "/home/dylan/src/mozilla/bugzilla-trunk-git/extlib";

this doesn't belong here

::: Bugzilla/User/Activity.pm
@@ +26,5 @@
> +
> +    my $dbh     = Bugzilla->dbh;
> +    my $user    = Bugzilla->user;
> +    my %context = (dbh  => $dbh,
> +                   user => $user);

there's no need to pass this context into the _query_* subs, just call Bugzilla->dbh or Bugzilla->user from them directly as required

@@ +30,5 @@
> +                   user => $user);
> +
> +    # XXX 
> +    # Dear future contributor,
> +    #

i'm not a fan of patches with XXX, and we don't need to formally address comments.

@@ +35,5 @@
> +    # If more tables need to be in the activity report, just
> +    # define a _query_* function and add it to the list passed to
> +    # _query_union_all(). The SELECT query must return the same number of rows.
> +    #
> +    # Some day we might have a centralized history mechanism.

remove this last line of the comment, it doesn't provide any value

@@ +98,5 @@
> +                && ($who ne $operation->{'who'}
> +                    || $when ne $operation->{'when'}))
> +            {
> +                $operation->{'changes'} = $changes;
> +                push (@operations, $operation);

nit: either remove the space after 'push', or the parentheses.

@@ +113,5 @@
> +            $change{'removed'} = $removed;
> +            $change{'added'} = $added;
> +
> +            if ($comment_id) {
> +                $change{'comment'} = Bugzilla::Comment->new($comment_id);

(almost) always use the cache when creating new objects... ->new({ id => $comment_id, cache => 1 })

@@ +147,5 @@
> +    my ($context, $login_names, $from, $to) = @_;
> +    my $placeholder_login_names = ('?') x @$login_names;
> +
> +
> +    my ($activity_joins, $activity_where) = ('', '');

nit: remove one of these blank lines

@@ +148,5 @@
> +    my $placeholder_login_names = ('?') x @$login_names;
> +
> +
> +    my ($activity_joins, $activity_where) = ('', '');
> +    if (Bugzilla->params->{"insidergroup"} && !$context->{user}->is_insider) {

there's no need to check the insidergroup param, $user->is_insider already does that.
(this issue occurs multiple times)

@@ +219,5 @@
> +
> +sub _query_bugs {
> +    my ($context, $login_names, $from, $to) = @_;
> +
> +    my $placeholder_login_names = ('?') x @$login_names;

nit: in the other _query subs, the blank line is placed after my $placeholder_login_names.  you should do the same here.

@@ +316,5 @@
> +}
> +
> +1;
> +
> +# __END__

nit: don't comment out __END__

Comment 55

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

Attachment: bug-387586-v11.patch

This should have all issues resolved, in addition to one I found: show_activity.cgi was returning <missing> for the what field on comments and comment tags, because operation.bug was not defined in the case that show_activity is called for bug history.

Comment 56

[:glob ✱]

Comment on attachment 8430426 [details] [diff] [review]
bug-387586-v11.patch

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

t/012throwables.t .... 1/215
#   Failed test '--WARNING template/en/default/global/user-error.html.tmpl has 1 unused error tag(s):
# user error tag 'user_activity_invalid_date' is defined at line(s) (1865) but is not used anywhere'
#   at t/012throwables.t line 203.
# Looks like you failed 1 test of 215.

following comments from a quick reading of the code; will test it once it passes tests.

::: Bugzilla/User/Activity.pm
@@ +298,5 @@
> +}
> +
> +sub _query {
> +    my ($query, @params) = @_;
> +    my $count =()= $query =~ /\?/sg;

that regex doesn't need /s

::: show_activity.cgi
@@ +17,2 @@
>  
> +use Date::Parse 'str2time';

str2time isn't used here

@@ +48,3 @@
>  
> +        ThrowUserError('user_activity_missing_to_date') unless $to;
> +        $to = format_time($to, '%Y-%m-%d');

i haven't tested this, but the implementation on bmo adds 1 day to $to to ensure that all changes made on that day are included in this report.  is this no longer required?

::: template/en/default/global/user-error.html.tmpl
@@ +1851,5 @@
>      to access any user information.
>  
> +  [% ELSIF error == "user_activity_missing_username" %]
> +      [% title = "Missing Username" %]
> +      You must provide at least one email address to report on.

you can't use the term "email address" here because bugzilla doesn't require email addresses for login.

Comment 57

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

I will have to compare the patch I generated to what's in my branch, as I ran all tests and didn't get that test error.

Comment 58

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

Attachment: bug-387586-v12.patch

Cleaned up the nits, tests should now pass without warnings. Removed unused imports, and split the report into two scripts. I find the logic much easier to follow.

Comment 59

[:glob ✱]

Comment on attachment 8437192 [details] [diff] [review]
bug-387586-v12.patch

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

the following errors are defined but not used (perhaps due to the missing cgi?):
- user_activity_missing_username
- user_activity_missing_to_date
- user_activity_missing_from_date

::: template/en/default/global/user-error.html.tmpl
@@ +1851,5 @@
>      to access any user information.
>  
> +  [% ELSIF error == "user_activity_missing_username" %]
> +      [% title = "Missing Username" %]
> +      You must provide at least one email address to report on.

you still can't use the term "email address" here.
replace it with "user".

::: template/en/default/reports/menu.html.tmpl
@@ +74,5 @@
> +
> +<ul>
> +  <li id="report_user_activity">
> +    <strong>
> +      <a href="show_user_activity.cgi">User Activity</a>

show_user_activity.cgi isn't part of this patch

Comment 60

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

yeah, the errors are due to the missing CGI. *sigh*

Comment 61

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

Attachment: bug-387586-v13.patch

Coming in at lucky 13, with the missing cgi added.

Comment 62

[:glob ✱]

Comment on attachment 8442976 [details] [diff] [review]
bug-387586-v13.patch

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

i like the splitting - it's much easier to read now :)

searching for multiple users doesn't work ($login_names only ever contains the first user)

::: Bugzilla/User/Activity.pm
@@ +108,5 @@
> +
> +            if ($comment_id) {
> +                $change{'comment'} =
> +                  Bugzilla::Comment->new({ id => $comment_id, cache => 1 });
> +                next if $change{'comment'}->count == 0;

why are you skipping activity made against comment 0?
this results in weird output if you tag comment 0.

@@ +165,5 @@
> +    INNER JOIN fielddefs
> +            ON bugs_activity.fieldid = fielddefs.id
> +    INNER JOIN profiles
> +            ON profiles.userid = bugs_activity.who
> +         WHERE profiles.login_name IN ($placeholder_login_names)

for all IN queries you have to use $dbh->sql_in (to keep oracle happy).

::: template/en/default/activity/table.html.tmpl
@@ +43,5 @@
> +      <th>Added</th>
> +    </tr>
> +
> +    [% FOREACH operation = operations %]
> +    [% DEFAULT operation.bug = bug %]

nit: indent the [% DEFAULT line

::: template/en/default/global/user-error.html.tmpl
@@ +1851,5 @@
>      to access any user information.
>  
> +  [% ELSIF error == "user_activity_missing_username" %]
> +      [% title = "Missing Username" %]
> +      You must provide at least one email address to report on.

you *still* can't use the term "email address" here.
replace it with "user".

Comment 63

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

(In reply to Byron Jones ‹:glob› from comment #62)
> Comment on attachment 8442976 [details] [diff] [review]
> bug-387586-v13.patch
> 
> Review of attachment 8442976 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> i like the splitting - it's much easier to read now :)
> 
> searching for multiple users doesn't work ($login_names only ever contains
> the first user)
Hmm, I thought that part looked wrong.


> ::: Bugzilla/User/Activity.pm
> @@ +108,5 @@
> > +
> > +            if ($comment_id) {
> > +                $change{'comment'} =
> > +                  Bugzilla::Comment->new({ id => $comment_id, cache => 1 });
> > +                next if $change{'comment'}->count == 0;
> 
> why are you skipping activity made against comment 0?
> this results in weird output if you tag comment 0.
I never questioned that from the original code, and I never tagged comment 0. I guess that should be removed? I wonder why it was there to begin with...

> @@ +165,5 @@
> > +    INNER JOIN fielddefs
> > +            ON bugs_activity.fieldid = fielddefs.id
> > +    INNER JOIN profiles
> > +            ON profiles.userid = bugs_activity.who
> > +         WHERE profiles.login_name IN ($placeholder_login_names)
> 
> for all IN queries you have to use $dbh->sql_in (to keep oracle happy).
I blame the bmo version for using mysqlisms. 

> nit: indent the [% DEFAULT line
*nods*

> you *still* can't use the term "email address" here.
> replace it with "user".

Crap, I thought I had fixed that one. *facepalm*

Comment 64

[Frédéric Buclin]

Dylan, any progress?

Comment 65

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

I haven't poked at this in several months; I will attempt to find time for it this quarter.