1021902 - UI to view a user's review history

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, defect, P1)

Description

[Mark Côté [:mcote]]

We should use the API built for bug 978773 to display a user's review history somewhere.  Even better would be to provide stats alongside (average time to review, maybe number of +s and -s, things like that).

Comment 2

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

I will use YUI3 in this, as the MyDashboard and ProductDashboard extensions do -- unless there are serious objections.

My idea being  preloading the data from JSON into the page template
but using the YUI DataTable to provide a table with sortable columns. No sense adding a new feature that looks old and crufty, right?

Comment 3

[David Lawrence [:dkl]]

(In reply to Dylan William Hardison [:dylan] from comment #2)
> I will use YUI3 in this, as the MyDashboard and ProductDashboard extensions
> do -- unless there are serious objections.
> 
> My idea being  preloading the data from JSON into the page template
> but using the YUI DataTable to provide a table with sortable columns. No
> sense adding a new feature that looks old and crufty, right?

YUI3 is definitely preferred as we will hopefully one have everything migrated over from YUI2 as part of the GSoC project and is an upstream goal. Doing the project dashboard and my dashboard in YUI3 gave me a good reason to use it.

dkl

Comment 4

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

Alright, so the original bug covered all flag changes (flag_state_activity is the name).
Originally I was going to display all flag types for a given user, but glob convinced me this is not what we want (and it makes the UI harder to figure out). 

Then, given that I want to use YUI3 DataTable, I decided to just make JSONRPC calls[1] at page load time, rather than encoding the data in the template. 

What I now question is:

1) The FlagStateActivity API allows one flag_id per request.
2) Flags are per-product, yet the form is about a "user's review history", leading to...
3) There are 4-5 flags called "review". Right now I'm just using id=4 [not hard coded] which is the common one. Should I make one API call for each and merge them?
4) Even if I did that, my reading of the Bug.flag_types method indicates a product is required. This makes sense, as flags are per-product. Should the landing page of the Review History request a product and as well as the user to report on?

Comment 5

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

I of course mean, per product and component, so that would be three fields (product, optional component, user) on the user review history report page.

Comment 6

[:glob ✱]

i don't think that asking the user to select the product is the right course of action -- the fact that there are multiple flags with the same name is an implementation detail that we don't need to expose to the user.

i suggest instead adding a "flag_name" argument to the webservice method which sets $match_criteria{flag_id] to an arrayref with flag_ids matching the provided name.

Comment 7

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

It turns out that the endpoint flag_id=? for the Review.flag_activity API method is *not* useless,
and without it the review history wouldn't be possible. :-)

After I stopped mocking my data, I realized that only r? has a requestee -- to do the history we need the concept of "originating requestee", and there are at least three solutions:

1) When a flag become not-?, record the requestee in a new original_requestee column.
2) Do some funky self-join, select this.*, original.requestee from flag_state_activity as this left join flag_state_activity as original on this.flag_id = original.flag_id && original.status = '?' where original.requestee_id = $PERSON_WE_WANT
3) Ask for all flags with with requestee = $PERSON_WE_WANT, then ask for all flags with flag_id = [flag ids of $PERSON_WE_WANT].

I've gone with #3. In order to keep the number of API calls down, I allowed flag_id to be an array. It's not clear what this means in the REST interface (using JSONRPC right now).

Comment 8

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

Attachment: bug-1021902-v1.patch

For preliminary review. This is a sort of "minimum viable product" patch as it will likely under go changes while being previewed bugzilla-dev. 

In particular, the code style will need to be revised to match our javascript standards (needs to be camelCased rather than under_scored, among others).

That said, I am very happy with the way the code looks at the moment, style changes aside.

Comment 9

[:glob ✱]

Comment on attachment 8469749 [details] [diff] [review]
bug-1021902-v1.patch

'review' means "this code is ready to ship".  changing to feedback.

Comment 10

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

It will be interesting to see how my code handles a flag changing like this.

Comment 11

[Mark Côté [:mcote]]

Testing this on bugzilla-dev will probably be easier if we were to have a very recent sanitized db dump from production.  I'll file a bug to refresh bugzilla-dev's db.

Comment 12

[:glob ✱]

Comment on attachment 8469749 [details] [diff] [review]
bug-1021902-v1.patch

notes from our irc conversation:
- remove the "admins only" restrict; this information is public
- add UI to load a report for another user (use bugzilla's user input field)
- display "loading.." text or similar while the table is initially being rendered, and while the data is being loaded
- don't use # to abbreviate "number", label that column "Bug" or "ID"
- show the bug summary, not just the ID
- change "setter" column label to "requester"
- as the rows are per-attachment, show attachment desc there too (no need to display the attach id)
- ensure the rows are sorted (by date?) and the sort criteria is a visible column
- display users as per $user->identity not $user->login
- all field exclusion so the first flag_activity call only returns the flag_id

Comment 13

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

Attachment: bug-1021902-v2.patch

(In reply to Byron Jones ‹:glob› from comment #12)
> Comment on attachment 8469749 [details] [diff] [review]
> bug-1021902-v1.patch
> 
> notes from our irc conversation:
> - remove the "admins only" restrict; this information is public
Done.

> - add UI to load a report for another user (use bugzilla's user input field)
Done.

> - display "loading.." text or similar while the table is initially being
Both done -- a bit of a tricky, I basically have a plain text "Loading..." that is hidden when the YUI DataTable is rendered, and I changed the default message of the YUI data table to be 'Loading...'
It will also say "No reviews" when there are no reviews to see.

> - don't use # to abbreviate "number", label that column "Bug" or "ID"
Done.

> - show the bug summary, not just the ID
Done.

> - change "setter" column label to "requester"
Done.

> - as the rows are per-attachment, show attachment desc there too (no need to
> display the attach id)
Done -- Perhaps should we also link to the attachment?

> - ensure the rows are sorted (by date?) and the sort criteria is a visible
> column
Column added so that this is more obvious.

> - display users as per $user->identity not $user->login
Done.
> - all field exclusion so the first flag_activity call only returns the
> flag_id
Field exclusion all around, for the attachment and bug summary requests too.

Note, to test this you will need to apply the patch from bug 913647 as well.

Comment 14

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

(In reply to Mark Côté [:mcote] from comment #0)
> [snip] Even better would be to provide stats alongside (average
> time to review, maybe number of +s and -s, things like that).

> Even better would be to provide stats alongside (average time to review, maybe number of +s and -s, things like that).

Stats like that could be collected as the other User Statistics are, and put on the User Profile page.
How do you feel about filing that as another bug?

Comment 15

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

This would be a place where I would want ninja edits.

Comment 16

[Mark Côté [:mcote]]

Good idea.  Filed as bug 1060436.

Comment 17

[:glob ✱]

Comment on attachment 8480988 [details] [diff] [review]
bug-1021902-v2.patch

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

most of this looks great.

::: extensions/Review/Extension.pm
@@ +658,5 @@
>      }
>  }
>  
> +sub review_history {
> +    my ($self, $args) = @_;

let's require login to view this report.
it simplifies the issue of hiding email addresses :)

::: extensions/Review/lib/WebService.pm
@@ +100,5 @@
>      }
>  
> +    if (my $type_name = $params->{type_name}) {
> +        trick_taint($type_name);
> +        my $flag_types = Bugzilla::FlagType::match({name => $type_name });

nit: missing a space after {

::: extensions/Review/template/en/default/pages/review_history.html.tmpl
@@ +14,5 @@
> +  javascript_urls = [ "js/yui3/yui/yui-min.js",
> +                      "extensions/Review/web/js/review_history.js",
> +                      "extensions/Review/web/js/moment.min.js",
> +                       'js/util.js',
> +                       'js/field.js' ]

nit: fix the indentation of the last two elements

@@ +38,5 @@
> +});
> +</script>
> +
> +<div>
> +    <form method="post">

make this GET instead of POST

::: extensions/Review/web/js/review_history.js
@@ +1,1 @@
> +(function () {

this file is missing a license

@@ +4,5 @@
> +  YUI.add('bz-review-history', function (Y) {
> +    function format_duration(o) {
> +      var secs = o.value;
> +
> +      return moment.duration(secs).humanize();

nit: no need for the 'secs' variable here

@@ +41,5 @@
> +          { key: 'creation_time' },
> +          { key: 'status' },
> +          { key: 'bug_id' },
> +          { key: 'type' },
> +          { key: 'attachment_id' },

remove trailing comma from the last element (breaks IE).

i started tagging all occurrences in this file, but there's quite a few, so instead i'm going to say "fix all of them" (i counted 17).

@@ +245,5 @@
> +      return history;
> +    }
> +
> +
> +

nit: only need one blank line here

::: extensions/Review/web/styles/review_history.css
@@ +1,1 @@
> +.yui3-skin-sam .yui3-datatable-table > table {

this file is missing a license

::: extensions/UserProfile/template/en/default/pages/user_profile.html.tmpl
@@ +93,5 @@
> +  <th>Review History</th>
> +  <td colspan="2">
> +      <a href="page.cgi?id=review_history.html&amp;requestee=[% target.login FILTER uri %]">Generate Report</a>
> +  </td>
> +</tr>

because this requires the email address to execute, this link should only be displayed to logged in users.

"generate report" doesn't make much sense.

move this link so it's within the "review queue" section, and display it just as a link called "Review History" next to the current queue count.

ie.

  review requests  13 (Review History)
feedback requests  0
needinfo requests  0

Comment 18

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

Attachment: bug-1021902-v3.patch

(In reply to Byron Jones ‹:glob› from comment #17)
> Comment on attachment 8480988 [details] [diff] [review]
> bug-1021902-v2.patch
> 
> Review of attachment 8480988 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> most of this looks great.

Thanks.

> ::: extensions/Review/Extension.pm
> nit: missing a space after {
Fixed.

> ::: extensions/Review/template/en/default/pages/review_history.html.tmpl
> nit: fix the indentation of the last two elements
Fixed

> @@ +38,5 @@
> > +});
> > +</script>
> > +
> > +<div>
> > +    <form method="post">
> 
> make this GET instead of POST
Done, not sure why I used POST to begin with.

> ::: extensions/Review/web/js/review_history.js
> this file is missing a license
Added.
> nit: no need for the 'secs' variable here
Removed.
> remove trailing comma from the last element (breaks IE).

Done, and I made this a highlight as a red Error in vim
:match Error /,\_s*[)}]/

> nit: only need one blank line here
Fixed.

> ::: extensions/Review/web/styles/review_history.css
> this file is missing a license
Added

> ::: extensions/UserProfile/template/en/default/pages/user_profile.html.tmpl
> because this requires the email address to execute, this link should only be
> displayed to logged in users.
> 
> "generate report" doesn't make much sense.
> 
> move this link so it's within the "review queue" section, and display it
> just as a link called "Review History" next to the current queue count.
> 
> ie.
> 
>   review requests  13 (Review History)
> feedback requests  0
> needinfo requests  0

Took a few tries to make this work in the table-based layout, but I agree it's very nice this way

Comment 19

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

Attachment: bug-1021902-v3.1.patch

Truncated css file when adding license, fixed.

Comment 20

[:glob ✱]

Comment on attachment 8486040 [details] [diff] [review]
bug-1021902-v3.1.patch

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

no issues with the code :)

just realised there's some inconsistencies with indentation which i should have picked up in my last review.

::: extensions/Review/template/en/default/pages/review_history.html.tmpl
@@ +59,5 @@
> +    <div id="history-loading">Loading...</div>
> +    <div id="history"></div>
> +</div>
> +
> +[% PROCESS global/footer.html.tmpl %]

use 2 spaces for indentation in templates (this applies to inline js as well)

::: extensions/Review/web/js/review_history.js
@@ +282,5 @@
> +      "datatable-datasource", "datasource-io", "datasource-jsonschema", "cookie",
> +      "gallery-datatable-row-expansion-bmo", "handlebars", "escape", "promise"
> +    ]
> +  });
> +}());

bugzilla (normally) uses 4 spaces to indent javascript files.
there are old files/functions with 2 spaces, but new code should be using 4.

::: extensions/UserProfile/template/en/default/pages/user_profile.html.tmpl
@@ +124,4 @@
>    <tr>
>      <td>&nbsp;</td>
>      <th>Review requests</th>
> +    <td class="numeric" width="10em">

use css to set widths (if required -- i don't see any difference without this element).

@@ +133,5 @@
>        [% "</a>" IF user.id %]
>      </td>
> +    [% IF user.id %]
> +      <td>
> +      (<a href="page.cgi?id=review_history.html&amp;requestee=[% target.login FILTER uri %]">Review&nbsp;History</a>)

it would be better to use css to control white-space wrapping, instead of &nbsp;

this line should be indented because it's inside a <td>

Comment 21

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

Attachment: bug-1021902-v4.patch

Fixed indentations in templates (tabstop=2) & js (tabstop=4), remove unneeded "width" and  

Comment 22

[:glob ✱]

Comment on attachment 8486954 [details] [diff] [review]
bug-1021902-v4.patch

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

r=glob

Comment 23

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

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   97384ad..5b25f5c  master -> master