UI to view a user's review history

RESOLVED FIXED

Status

()

P1
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mcote, Assigned: dylan)

Tracking

(Blocks: 1 bug, {bmo-goal})

Production
bmo-goal
Dependency tree / graph

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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).
(Reporter)

Updated

5 years ago
Duplicate of this bug: 1021834
(Reporter)

Updated

5 years ago
Assignee: nobody → dylan
Keywords: bmo-goal
(Reporter)

Updated

5 years ago
Priority: -- → P1
(Assignee)

Comment 2

5 years ago
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?
(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
(Assignee)

Comment 4

4 years ago
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?
(Assignee)

Comment 5

4 years ago
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.
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.
(Assignee)

Comment 7

4 years ago
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).
(Assignee)

Updated

4 years ago
Depends on: 913647
(Assignee)

Comment 8

4 years ago
Created attachment 8469749 [details] [diff] [review]
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.
Attachment #8469749 - Flags: review?(glob)
Comment on attachment 8469749 [details] [diff] [review]
bug-1021902-v1.patch

'review' means "this code is ready to ship".  changing to feedback.
Attachment #8469749 - Flags: review?(glob) → feedback?(glob)
(Assignee)

Comment 10

4 years ago
It will be interesting to see how my code handles a flag changing like this.
(Reporter)

Comment 11

4 years ago
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.
(Reporter)

Updated

4 years ago
Depends on: 1050639
Depends on: 1050628
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
Attachment #8469749 - Flags: feedback?(glob) → feedback-
(Assignee)

Comment 13

4 years ago
Created attachment 8480988 [details] [diff] [review]
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.
Attachment #8469749 - Attachment is obsolete: true
Attachment #8480988 - Flags: review?(glob)
(Assignee)

Comment 14

4 years ago
(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?
Status: NEW → ASSIGNED
Flags: needinfo?(mcote)
(Assignee)

Comment 15

4 years ago
This would be a place where I would want ninja edits.
(Reporter)

Comment 16

4 years ago
Good idea.  Filed as bug 1060436.
Flags: needinfo?(mcote)
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
Attachment #8480988 - Flags: review?(glob) → review-
(Assignee)

Comment 18

4 years ago
Created attachment 8486038 [details] [diff] [review]
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
Attachment #8480988 - Attachment is obsolete: true
Attachment #8486038 - Flags: review?(glob)
(Assignee)

Comment 19

4 years ago
Created attachment 8486040 [details] [diff] [review]
bug-1021902-v3.1.patch

Truncated css file when adding license, fixed.
Attachment #8486038 - Attachment is obsolete: true
Attachment #8486038 - Flags: review?(glob)
Attachment #8486040 - Flags: review?(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>
Attachment #8486040 - Flags: review?(glob) → review-
(Assignee)

Comment 21

4 years ago
Created attachment 8486954 [details] [diff] [review]
bug-1021902-v4.patch

Fixed indentations in templates (tabstop=2) & js (tabstop=4), remove unneeded "width" and &nbsp;
Attachment #8486040 - Attachment is obsolete: true
Attachment #8486954 - Flags: review?(glob)
Comment on attachment 8486954 [details] [diff] [review]
bug-1021902-v4.patch

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

r=glob
Attachment #8486954 - Flags: review?(glob) → review+
(Assignee)

Comment 23

4 years ago
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   97384ad..5b25f5c  master -> master
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Blocks: 1067808
No longer depends on: 1067808

Updated

4 years ago
Blocks: 1071108
You need to log in before you can comment on or make changes to this bug.