515210 - Review Board Integration

Status: RESOLVED FIXED

(MozReview Graveyard :: General, enhancement)

Description

[Max Kanat-Alexander]

I want to replace our PatchReader/textarea code review system with Review Board integration. Review Board's website is at: http://www.review-board.org/

Review Board is really nice because you can do "graphical" code reviews (that is, imagine being able to select a section in the Diff Viewer and comment on it directly), but it can also export these reviews out to plain text.

Review Board already has support for various VCSes, including svn, hg, and bzr.

The *minimal* integration system that I imagine would work something like this:

* A user can attach a patch in Bugzilla, and specify a repository (and
  optionally a revision, if the patch is not against trunk when attached)
  from a drop-down that is limited per-product. Possibly this information
  could come from review-board (since it already tracks repositories), but
  I'd need some way to limit the list ofrepositories per-product (without
  having to set up a separate RB instance
  for each product in Bugzilla).

* Add a "Code Review" button on Bugzilla's Attachment Details page. This 
  would bring up the Review Board review interface, probably in an iframe,
  with the attached patch right there. It would do this by creating a review
  request in RB and then displaying it in review mode immediately.

* All review requests and reviews would at first probably be performed by 
  a single "Bugzilla" user in RB, unless I could implement some way in RB
  to re-use Bugzilla accounts.

* Once the user's review was complete, they would click "submit" in Bugzilla,
  which would submit the review in RB (not quite sure how to do this, 
  given browser XSS restrictions, but I'm sure I could figure out some 
  way), then extract the review as plain text and submit it as a comment
  to Bugzilla with a read-only link to the full "graphical" review in RB.

Comment 1

[Christian Hammond]

Hi,

I'm Christian Hammond, one of the founders/lead developers of Review Board. Someone pointed this out to me today, and I think it'd be awesome :)


(In reply to comment #0)
> * A user can attach a patch in Bugzilla, and specify a repository (and
>   optionally a revision, if the patch is not against trunk when attached)
>   from a drop-down that is limited per-product. Possibly this information
>   could come from review-board (since it already tracks repositories), but
>   I'd need some way to limit the list ofrepositories per-product (without
>   having to set up a separate RB instance
>   for each product in Bugzilla).

Repository info can be queried from Review Board. We don't have a mechanism today for categorizing repositories, though. Could something be done on the 
Bugzilla end to map one or more repositories to a Product?


> * Add a "Code Review" button on Bugzilla's Attachment Details page. This 
>   would bring up the Review Board review interface, probably in an iframe,
>   with the attached patch right there. It would do this by creating a review
>   request in RB and then displaying it in review mode immediately.

This should all be pretty easy through our API. It would work much like our post-review tool does (and really, could even use post-review if you wanted to put it all together quickly, perhaps as a proof of concept), and just take the resulting URL and display that in a browser window/iframe.


> * All review requests and reviews would at first probably be performed by 
>   a single "Bugzilla" user in RB, unless I could implement some way in RB
>   to re-use Bugzilla accounts.

Review Board today supports multiple auth backends: Built-in auth, LDAP, NIS, Active Directory, and soon OpenID. New auth backends could be written and used without modifying the Review Board codebase. One could be written for authenticating against Bugzilla, provided that there's a good mechanism on the Bugzilla side that can be used from Review Board for that.


> * Once the user's review was complete, they would click "submit" in Bugzilla,
>   which would submit the review in RB (not quite sure how to do this, 
>   given browser XSS restrictions, but I'm sure I could figure out some 
>   way), then extract the review as plain text and submit it as a comment
>   to Bugzilla with a read-only link to the full "graphical" review in RB.

Submitting can be done by an HTTP POST to some URL (I can't recall the address off the top of my head).

You can do a GET on /api/json/reviewrequests/<id>/ to get info on the review request and all reviews.

I don't know the architecture you're planning here, so I don't know what all is implemented client-side and what's server-side, but the server-side can always place the API calls needed on the Review Board server.

You might be interested in subscribing to the [e-mail] mailing list. A number of people are on there (both developers and end-users) who would find this interesting and may be able to recommend things. Let us know if you're stuck on something and need something from our side.

Comment 2

[Max Kanat-Alexander]

(In reply to comment #1)
> Repository info can be queried from Review Board. We don't have a mechanism
> today for categorizing repositories, though. Could something be done on the 
> Bugzilla end to map one or more repositories to a Product?

  Ah, okay. Yeah, we could offer a UI for selecting which repositories are available per-product, and if people don't limit the list, then we'd just make all repos available.

> This should all be pretty easy through our API. It would work much like our
> post-review tool does (and really, could even use post-review if you wanted to
> put it all together quickly, perhaps as a proof of concept), and just take the
> resulting URL and display that in a browser window/iframe.

  Cool. Perhaps I'll check that out.

> Review Board today supports multiple auth backends: Built-in auth, LDAP, NIS,
> Active Directory, and soon OpenID. New auth backends could be written and used
> without modifying the Review Board codebase. One could be written for
> authenticating against Bugzilla, provided that there's a good mechanism on the
> Bugzilla side that can be used from Review Board for that.

  Yeah, there's two ways it could be done. Bugzilla uses a salted SHA-256 hash for storing passwords in the database, so you could just auth against the Bugzilla DB directly. Also, there's an XML-RPC User.login method (doc'ed here: http://www.bugzilla.org/docs/3.4/en/html/api/Bugzilla/WebService/User.html ) that could be used to validate users, and which even returns a cookie for Bugzilla.

> I don't know the architecture you're planning here, so I don't know what all is
> implemented client-side and what's server-side, but the server-side can always
> place the API calls needed on the Review Board server.

  Yeah, I'll just have to figure out some good solution.

> You might be interested in subscribing to the [e-mail]
> mailing list. A number of people are on there (both developers and end-users)
> who would find this interesting and may be able to recommend things. Let us
> know if you're stuck on something and need something from our side.

  Cool! Will definitely let you know! :-)

Comment 3

[Gervase Markham [:gerv]]

Relevant:
http://www.visophyte.org/blog/2010/02/13/review-board-and-bugzilla-reviews-take-3/

Gerv

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

would the solution to this be to add a setup/config option to bugzilla to point to an external review-board server and if that is used, we could add a few integration points to the review process in bugzilla?  This would esentially be setting up a local review-board server and using the jetpack (referenced in Comment #3 above) until those few adjustments are integrated into bugzilla officially.

Comment 5

[Max Kanat-Alexander]

(In reply to comment #4)
> would the solution to this be to add a setup/config option to bugzilla to point
> to an external review-board server and if that is used, we could add a few
> integration points to the review process in bugzilla? 

  Yes, somewhat. The solution has to encompass comment 0 and should take into account the discussion after that, also.

Comment 6

[Mark Côté [:mcote]]

I have been working on this, starting with authentication.  The work was originally in bug 894537.  I am making some changes to ReviewBoard itself to allow this to become a proper extension instead of a fork.

Comment 7

[:glob ✱]

as this bug is for "bugzilla the product" integration, bug 943747 shouldn't be a blocker for this work.

Comment 8

[(no longer active)]

What's our plans to integrate the Bugzilla users with ReviewBoard?  ReviewBoard also has the notion of user accounts, and it seems like people won't be treated as reviewers in RB unless there is an associated user for them that RB knows about.

Comment 9

[Mark Côté [:mcote]]

We already support that; that was done when we first set up Review Board in the fall of 2013.  Authentication and user-search queries (e.g. for autosuggest) are all referred to Bugzilla via XMLRPC.  The results of any such query is stored in the Review Board user database, but any queries are always referred to Bugzilla to keep it up to date.

The alternative was to mirror the databases, but to mitigate the possibility of lag we'd have to have a notification when Bugzilla users are changed.  I think just using Bugzilla's APIs for all user-related functions is the only way to get proper synchronization.

Comment 10

[(no longer active)]

(In reply to comment #9)
> We already support that; that was done when we first set up Review Board in the
> fall of 2013.  Authentication and user-search queries (e.g. for autosuggest)
> are all referred to Bugzilla via XMLRPC.  The results of any such query is
> stored in the Review Board user database, but any queries are always referred
> to Bugzilla to keep it up to date.
> 
> The alternative was to mirror the databases, but to mitigate the possibility of
> lag we'd have to have a notification when Bugzilla users are changed.  I think
> just using Bugzilla's APIs for all user-related functions is the only way to
> get proper synchronization.

That sounds great!  Do you happen to have a pointer to the code behind this so that I can point our contractor working on the rest of the review tool to the work already done here?  Thanks!

Comment 11

[Mark Côté [:mcote]]

Yessir, it's at https://github.com/mozilla/rbbz

The only thing I have left to do is change the user search to match anywhere in the name instead of just the beginning, so that we can use our :irc_nick autosuggest convention.  I was going to put that in core, but it seems that I'll be adding a hook and doing it in the extension instead (https://reviews.reviewboard.org/r/5503/).  Should be done that this week (after the Bugzilla bzr->git migration, yay!).

Comment 12

[Mark Côté [:mcote]]

I'd like to chat soonish about what kind of work you foresee needing to happen on Bugzilla itself.  We're trying to assemble next quarter's goals, and I'm pretty sure they should involve some Review Board integration work.

Comment 13

[(no longer active)]

Sure, ni?ing myself to remember to get back to you.  At most by Monday.

Comment 14

[(no longer active)]

OK, let's book a meeting sometime this week?  I won't be around on Thursday and my calendar should be up to date and relatively empty for the rest of the week.  :-)  Can you please send me an invite for the time that works best for you?

Comment 15

[Mark Côté [:mcote]]

Pursuant to our discussion, here are the tasks we've identified for phase one of integration.  All of these can, I believe, be done to the rbbz Review Board extension.  I'll file individual bugs for them all and make this the tracker.

* Only allow users with editbugs to log in, since only they can set attachment flags.
** New development in Bugzilla since this meeting: flag requestees can now set the flag even if they don't have editbugs. For now, we'll stick to editbugs only to avoid complexity.

* Disallow creation of reviews for confidential bugs, for security reasons, and delete (completely) review if associated bug is marked confidential.

* When bug ID is set, set the RB link in Bugzilla as an attachment when review is published.

* Integrate suggested reviewers.

* Display error when auth token expires.

* Mirror comments back to Bugzilla (only one way, RB->BZ).

* Allow Bugzilla auth tokens to be used with Review Board web API calls

* Mirror flag changes back to Bugzilla (when RB supports Bugzilla-like flags).

Comment 16

[Mark Côté [:mcote]]

We have basic integration now, so I think this bug can be resolved.  Removing all blockers since we don't need it as a general tracker either (most issues will go into Developer Services :: MozReview).