1102428 - Custom field for MozReview link, with dynamic info

Status: RESOLVED FIXED

(bugzilla.mozilla.org Graveyard :: Extensions: MozReview Integration, enhancement)

Description

[Mark Côté [:mcote]]

We're putting a lot of effort into making MozReview the center of the patch world at Mozilla, so I'd like to have better support in Bugzilla than just an attachment-as-link.

We can iterate on this, but the field should take a URL, as we post now, but then query Review Board for information (dynamically, after page load) including title, status, and perhaps number of reviews or issues or something like that.  The idea is to make it both clear that there's an associated MozReview review request as well as provide a bit of information as to its current status/level of activity.

Comment 1

[David Lawrence [:dkl]]

I would like to work on this. 

Proposal:

1. Create new custom field called cf_mozreview_link (freetext)
   Q1: Should this field be editable by BMO user or should be for internal use
       only and settable by the API?
   Q2: Should it be visible only to a small set of products or global to all?
2. Using extension similar to OrangeFactor, add a small section to show_bug.cgi 
   to display the current information/status of the reviewboard review. It would
   use mainly javascript and load async so as to not slow down display of the bug
   page and only if there is a valid value in the cf_mozreview_link field.
   Q1: Documentation on the RB API that can be used for the ajax call?
   Q2: Would the ajax call need any special permissions (use api key per user) 
       or would it just be pulling data from RB that anyone could see anyway?
   Q3: Should the review information display by default or only if a link/button
       is clicked?

dkl

Comment 2

[Mark Côté [:mcote]]

1.1: I can't think of a good reason that a user would want to edit this field in the UI, so let's keep it in the API, for now at least.
1.2: Should be global, but ideally hidden if not set (since the user can't edit it anyway).
2.1: https://www.reviewboard.org/docs/manual/2.0/webapi/2.0/resources/review-request/
     I'm thinking summary (as the link), status, last_updated, and maybe issue_open_count.
2.2: No, anonymous calls should be fine.
2.3: Might as well display by default, as you say, fetched asynchronously.

(CCing MozReview devs)

Comment 3

[Steven MacLeod [:smacleod]]

(In reply to David Lawrence [:dkl] from comment #1)
> 1. Create new custom field called cf_mozreview_link (freetext)
This field should support storing multiple Review Request ids, since each user is able to have their own set of commits associated with the bug.

Comment 4

[Gregory Szorc [:gps]]

Things get complicated because we have multiple review requests per bug. And with the RB API you need a separate request for each review request. I wonder if we should provide a custom API endpoint in RB for returning all metadata associated with a parent review request. That would further allow us to perform semi-complicated data aggregation on the RB side. This should help reduce load time and complication in the Bugzilla implementation.

I dare say I'm tempted to expose an API that emits HTML and have Bugzilla just insert an <iframe>.

Comment 5

[David Lawrence [:dkl]]

(In reply to Gregory Szorc [:gps] from comment #4)
> Things get complicated because we have multiple review requests per bug. And
> with the RB API you need a separate request for each review request. I
> wonder if we should provide a custom API endpoint in RB for returning all
> metadata associated with a parent review request. That would further allow
> us to perform semi-complicated data aggregation on the RB side. This should
> help reduce load time and complication in the Bugzilla implementation.

Ideally what we would want to store on the BMO side is just the parent review request for each of the requests attached to the bug.

I agree that adding a new API call using extension in ReviewBoard would be nice as I can make a single call per top-level request and get all of the statuses for the child reviews in one call instead of making many individual calls.

If we went the custom API method route, I could even just input the calling bug id, and have ReviewBoard do all of the date gathering for me and return it in a nice data structure. 

The only downside to that other than using the built in API is it is not as easy to request one or more attributes exported by the stock API without changes on both ends. But that may not change that often anyway.

> I dare say I'm tempted to expose an API that emits HTML and have Bugzilla
> just insert an <iframe>.

Nah don't really want to go that route. It's easy enough to include a small table of information using the same styling as other Bugzilla page elements.

dkl

Comment 6

[David Lawrence [:dkl]]

Propose plan:

Goals:

* Allow MozReview to add/remove top-level review request ids to a bug in Bugzilla using a special REST API call specifically for MozReview. 
* If one or more review request ids exist for a bug, and the user is viewing the bug, a small table of data about the review request(s) will be displayed showing the current summary (as the link), status, last_updated, and issue_open_count.
* The will not be any information displayed and be hidden if there are no requests for the bug.
* A Bugzilla user will not be able to manipulate to list of request ids for the bug using the UI and changes will only occur via the web API by an empowered user.
* All review requests will be considered public and Bugzilla will display whatever information is provided regardless of user's permission level.
* API calls to MozReview should not require to be logged in.
* The review information should be loaded after all other operations as to not hinder the performance of show_bug.cgi. 
* All information in Bugzilla is read-only and no changes can be made to any data in MozReview. User will need to click on the summary to go directly to the review and log in to MozReview.

Database Schema:

mozreview_bug_map:
* id PRIMARYKEY
* bug_id INTEGER FOREIGN KEY bugs(bug_id)
* review_id INTEGER
* UNIQUE (bug_id, review_id)

API:

MozReview.update (PUT /rest/mozreview/reviews/(bug_id))

{ "id": (bug_id), "reviews": { "add": [1,2,3], "remove": [4,5,6] } }

* Either "add" or "remove" can be empty lists or be not present but one or the other is required.
* If both missing, error is returned. If both present but empty, no action is made and no error is returned.
* If the "id" argument is missing on the bug id is not found, an error is returned.
* User must be authenticated to perform any changes and be properly empowered to do so.

UI Design:
            
Summary                                                      | Who | Status | Last Updated | Issues  
---------------------------------------------------------------------------------------------------
reviewboard: add a command for discarding a draft review request | gps | Closed    | 2014-10-29 | 0 
Bug 966545 - Refactor connection logic                           | gps | Discarded | 2014-09-09 | 0

Comment 7

[Mark Côté [:mcote]]

So I guess we'd be aggregating the issue count from all child requests, and using the time from the last-updated review request (child or parent)?  I think that's probably good to start with, although we probably want a new MozReview API to do the aggregation, as gps suggested.  I would also change "Who" to "Requester".

Comment 8

[Mark Côté [:mcote]]

Attachment: MozReview extension

This adds an extension (plus one necessary hook) that looks at the attachments in the current bug, and if there are any with content type text/x-review-board-request, inserts a table below the main bug data.  On page load, some JavaScript fetches info from Review Board and populates the table with basic information on the review request(s).

This requires the patch from bug 1124445 to be deployed to MozReview.  It is still under review, but I'll try to get it on reviewboard-dev (will comment here when it's up).

Comment 9

[Mark Côté [:mcote]]

Oh also, this is styled with the Mozilla skin in mind.  It wasn't obvious if I could have different style rules in my extensions for the different skins, and it looks only a bit incongruent in the Dusk skin anyway.  That said, if I should be using any standard classes to better adapt to the different skins, let me know, although it looks like many of the built-in classes are pretty specific to particular elements, and vice versa.

Comment 10

[:glob ✱]

Comment on attachment 8563637 [details] [diff] [review]
MozReview extension

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

this mostly looks sane from a quick read, mostly minor points to address.
is there a way for me to test this?  (ie. has the rb ext part of this been deployed somewhere?)

fails tests:

t/008filter.t ........ 178/643
#   Failed test '(en/default) extensions/MozReview/template/en/default/hook/bug/edit-after_bug_data.html.tmpl has unfiltered directives:
#   15: Bugzilla.params.mozreview_base_url
#   33: rrid
# --ERROR'
#   at t/008filter.t line 130.

::: extensions/MozReview/Extension.pm
@@ +31,5 @@
> +        my $attachments = Bugzilla::Attachment->get_attachments_by_bug($bug);
> +
> +        foreach my $attachment (@$attachments) {
> +            if ($attachment->contenttype eq 'text/x-review-board-request') {
> +                push @rrids, ($attachment->data =~ /\/r\/(\d+)\/?$/);

to improve regex readability use a different regex delimiter:
  =~ m#/r/(\d+)/?$#

::: extensions/MozReview/template/en/default/hook/admin/editparams-current_panel.html.tmpl
@@ +7,5 @@
> +  #%]
> +
> +[% IF panel.name == "advanced" %]
> +  [% panel.param_descs.mozreview_base_url =
> +    'MozReview Base URL' %]

nit: no need to wrap this line

::: extensions/MozReview/template/en/default/hook/bug/edit-after_bug_data.html.tmpl
@@ +8,5 @@
> +
> +[% USE Bugzilla %]
> +[% cgi = Bugzilla.cgi %]
> +
> +[% IF mozreview %]

instead of wrapping the whole template in an unindented IF block, you can do

[% RETURN UNLESS mozreview %]

::: extensions/MozReview/template/en/default/hook/bug/show-header-end.html.tmpl
@@ +6,5 @@
> +  # defined by the Mozilla Public License, v. 2.0.
> +  #%]
> +
> +[% style_urls.push('extensions/MozReview/web/style/mozreview.css') %]
> +[% javascript_urls.push('extensions/MozReview/web/js/mozreview.js') %]

these files should only be included if mozreview is configured/enabled

::: extensions/MozReview/web/js/mozreview.js
@@ +41,5 @@
> +  }
> +
> +  var years = months / 12;
> +  return MozReview.formatElapsedTime("year", years);
> +};

formatElapseTime and elapsedTime have 2 spaces for indentation; should be 4

@@ +50,5 @@
> +    var td = $('<td/>');
> +
> +    if (!hostUrl.endsWith('/')) {
> +        hostUrl += '/';
> +    }

fix the url once when the parameter is set instead of every time you get a review request.
(provide a "checker" in config_modify_panels).

::: extensions/MozReview/web/style/mozreview.css
@@ +10,5 @@
> +  border: none;
> +  border-collapse: collapse;
> +  border-bottom: 1px solid rgba(0, 0, 0, 0.2);
> +  box-shadow: 0 1px 1px rgba(0, 0, 0, 0.1);
> +}

4 spaces indentation for css please

::: template/en/default/bug/edit.html.tmpl
@@ +187,4 @@
>  
>    <table id="bz_big_form_parts" cellspacing="0" cellpadding="0"><tr>
>    <td>
> +    [% Hook.process("after_bug_data") %]

add a comment indicating this is a bmo custom hook

[%# BMO - blah blah %]
[% Hook.process.. %]

Comment 11

[:glob ✱]

(In reply to Mark Côté [:mcote] from comment #9)
> Oh also, this is styled with the Mozilla skin in mind.  It wasn't obvious if
> I could have different style rules in my extensions for the different skins,
> and it looks only a bit incongruent in the Dusk skin anyway.  That said, if
> I should be using any standard classes to better adapt to the different
> skins, let me know, although it looks like many of the built-in classes are
> pretty specific to particular elements, and vice versa.

it needs to look schmick with the mozilla skin, but reasonable in the others.
the body element has a skin-* class which you can use in your stylesheet to perform classic/dusk tweaking.

Comment 12

[Mark Côté [:mcote]]

Attachment: MozReview extension

Addressed all review points.

The panel looks slightly out of place in the Dusk skin but not terribly.  If you'd like me to fix it up I can.

The Review Board extension is currently deployed to https://reviewboard-dev.allizom.org/.  I've got some changes to it under review but they don't affect core functionality aside from adding a few more informative errors.

Comment 13

[Mark Côté [:mcote]]

Attachment: Screen shots

Comment 14

[:glob ✱]

Comment on attachment 8565136 [details] [diff] [review]
MozReview extension

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

the requests to RB are failing:

Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at https://reviewboard-dev.allizom.org/api/extensions/mozreview.extension.MozReviewExtension/summary/378/. This can be fixed by moving the resource to the same domain or enabling CORS.

additionally if an error happens when fetching the data from RB, the UI is stuck on 'loading..' instead of displaying the error message.

this is preventing me from reviewing this any further.

::: extensions/MozReview/template/en/default/hook/admin/editparams-current_panel.html.tmpl
@@ +1,1 @@
> +[%# This Source Code Form is subject to the terms of the Mozilla Public

this template is in the wrong location.  it should be in extensions/MozReview/template/en/default/hook/admin/params

::: extensions/MozReview/template/en/default/hook/bug/edit-after_bug_data.html.tmpl
@@ +28,5 @@
> +          <thead>
> +            <th>commit</th>
> +            <th>status</th>
> +            <th>open issues</th>
> +            <th>last updated</th>

title case these headers please

Comment 15

[Mark Côté [:mcote]]

Attachment: MozReview extension

Updated for review.  I also removed a CSS definition that wasn't working and probably isn't what I want anyway.

CORS support should be on reviewboard-dev soon; see blocker.

Comment 16

[:glob ✱]

Comment on attachment 8570065 [details] [diff] [review]
MozReview extension

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

if there's an error, the error message show is the HTTP error code instead of the response from rb:
> Error loading review request 378: BAD REQUEST
while the error message returned is
> Review request is not a parent
we should display the error message instead of BAD REQUEST

obsolete reviews are always loaded.  not sure if this is something that we should be doing.
perhaps obsolete attachments shouldn't be loaded/visible until the "show obsolete" link in the attachments table is clicked (or add a similar toggle to the reviews table?).

::: extensions/MozReview/template/en/default/hook/bug/edit-after_bug_data.html.tmpl
@@ +38,5 @@
> +              <tr class="mozreview-loading-row">
> +                <td>Loading...</td>
> +              </tr>
> +              <tr class="mozreview-loading-error-row bz_default_hidden">
> +                <td>Error loading review

this <td> should span all four columns to keep error messages from making the commit column too wide.

::: extensions/MozReview/web/js/mozreview.js
@@ +8,5 @@
> +
> +var MozReview = {};
> +
> +MozReview.formatElapsedTime = function(s, val) {
> +    return Math.floor(val) + ' ' + s + (val > 1 ? 's' : '') + ' ago';

this function results in durations such as "1 days ago" instead of "1 day ago" for values > 1 and < 2
also, 0 should be plural ("0 seconds ago", not "0 second ago").

@@ +62,5 @@
> +        var trCommit = tr.clone();
> +        var a = $('<a/>');
> +
> +        if (!isParent) {
> +            tdSummary.append('&nbsp;&nbsp;&nbsp;&nbsp;');

please implement padding using css instead of &nbsp;

@@ +70,5 @@
> +        a.text(rr.summary);
> +        tdSummary.append(a);
> +
> +        if (isParent) {
> +            tdSummary.append(' (' + rr.submitter + ')');

rr.submitter needs html escaping

Comment 17

[Mark Côté [:mcote]]

Attachment: MozReview extension v4

Comment 18

[:glob ✱]

Comment on attachment 8572777 [details] [diff] [review]
MozReview extension v4

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

r=glob

Comment 19

[Mark Côté [:mcote]]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   8d20ba6..7d75b6e  master -> master

Comment 20

[Ed Morley [:emorley]]

This is awesome btw! :-)