adding “reviewer delegation” to MozReview


One of the first projects I worked on when I moved to the MozReview team was “review delegation”. The goal was to add the ability for a reviewer to redirect a review request to someone else. It turned out to be a change that was much more complicated than expected.

MozReview is a Mozilla developed extension to the open source Review Board product; with the primary focus on working with changes as a series of commits instead of as a single unified diff.  This requires more than just a Review Board extension, it also encompasses a review repository (where reviews are pushed to), as well as Mercurial and Git modules that drive the review publishing process.  Autoland is also part of our ecosystem, with work started on adding static analysis (eg. linters) and other automation to improve the code review workflow.

I inherited the bug with a patch that had undergone a single review. The patch worked by exposing the reviewers edit field to all users, and modifying the review request once the update was OK’d. This is mostly the correct approach, however it had two major issues:

  1. According to Review Board and Bugzilla, the change was always made by the review’s submitter, not the person who actually made the change
  2. Unlike other changes made in Review Board, the review request was updated immediately instead of using a draft which could then be published or discarded


Review Board has a simple permissions model – the review request’s submitter (aka patch author) can make any change, while the reviewers can pretty much only comment, raise issues, and mark a review as “ready to ship” / “fix it”. As you would expect, there are checks within the object layer to ensure that these permission boundaries are not overstepped.  Review Board’s extension system allows for custom authorisation and permissions check, however the granularity of these are course: you can only control if a user can edit a review request as a whole, not on a per-field level.

In order to allow the reviewer list to be changed, we need to tell Review Board that the submitter was making the change.

Performing the actions as the submitter instead of the authenticated user is easy enough, however when the changes were pushed to Bugzilla they were attributed to the wrong user. After a few false starts, I settled on storing the authenticated user in the request’s meta data, performing the changes as the submitter, and updating the routines that touch Bugzilla to first check for a stored user and make changes as that user instead of the submitter. Essentially “su”.

This worked well except for on the page where the meta data changes are displayed – here the change was incorrectly attributed to the review submitter. Remember that under Review Board’s permissions model only the review submitter can make these changes, so the name and gravatar were hard-coded in the django template to use the submitter. Given the constraints a Review Board extension has to operate in, this was a problem, and developing a full audit trail for Review Board would be too time consuming and invasive. The solution I went with was simple: hide the name and gravatar using CSS.

Drafts and Publishing

How Review Board works is, when you make a change, a draft is (almost) always created which you can then publish or discard. Under the hood this involves duplicating the review request into a draft object, against which modifications are made. A review request can only have one draft, which isn’t a problem because only the submitter can change the review.

Of course for reviewer delegation to work we needed a draft for each user. Tricky.

The fix ended up needing to take a different approach depending on if the submitter was making the change or not.

When the review submitter updates the reviewers, the normal review board draft is used, with a few tweaks that show the draft banner when viewing any revision in the series instead of just the one that was changed. This allows us to correctly cope with situations where the submitter makes changes that are broader than just the reviewers, such as pushing a revised patch, or attaching a screenshot.

When anyone else updates the reviewers, a “draft” is created in local storage in their browser, and a fake draft banner is displayed. Publishing from this fake draft banner calls the API endpoint that performs the permissions shenanigans mentioned earlier.

Reviewer Delegation - Local Draft

Overall it has been an interesting journey into how Review Board works, and highlighted some of the complications MozReview hits when we need to work against Review Board’s design. We’ve been working with the Review Board team to ease some of these issues, as well as deviating where required to deliver a Mozilla-focused user experience.


mozreview and inline comments on the diff view

when a comment is left on a review in review board/mozreview it is currently displayed as a small square in the left column.

comment indicator

our top reviewers have strongly indicated that this is suboptimal and would prefer to match what most other code review systems do in displaying comments as an inline block on the diff. i agree — review comments are important and deserve more attention in the user interface; they should be impossible to miss.

while the upstream review board team have long said that the current display is in need of fixing, there is minimal love for the inline comments approach.

recently we worked on a plan of attack to appease both our reviewers and upstream’s requirements.  :smacleod, :mconley and i talked through a wide assortment of design goals and potential issues.  we have a design document and i’ve started mocking up the approach in html:

inline comments mock-up

we also kicked off discussions with the upstream review board development team, and are hopeful that this is a feature that will be accepted upstream.

happy bmo push day!

the following changes have been pushed to

  • [1243246] Attachment data submitted via REST API must always be base64 encoded
  • [1213424] The Bugzilla autocomplete dropdown should expand the width to show the full text of a match
  • [1241667] Trying to report a bug traps the user in an infinite loop
  • [1188236] “Congratulations on having your first patch approved” email should be clearer about how to get the patch landed.
  • [1243051] Create one off script to output cpanfile with all modules and their current versions to be used for version pinning
  • [1244604] configure nagios alerting for the bmo/reviewboard connector
  • [1244996] add a script to manage a user’s settings

discuss these changes on

happy bmo push day!

the following changes have been pushed to

  • [1233878] tracking flags don’t show up in the view of the bug right after filing
  • [1224001] Add push connector for
  • [1237188] add support for servicenow to the ‘see also’ field
  • [1236955] [form.mdn] Please add component drop-down to custom form
  • [1232913] The attachment links don’t look like links
  • [1237185] hide ‘cab review’ custom field behind a “click through” to direct people to servicenow

discuss these changes on

moving from bugzilla to mozreview

for the next couple of quarters (at least) i’ll be shifting my attention full time from bugzilla to mozreview. this switch involves a change of language, frameworks, and of course teams. i’m looking forward to new challenges.

one of the first things i’ve done is sketch out a high level architectural diagram of mozreview and its prime dependencies:

MozReview Architectural Diagram

mozreview exists as an extension to reviewboard, using bugzilla for user authentication, ldap to check commit levels, with autoland pushing commits automatically to try (and to mozilla-central soon).  there’s mecurial extensions on both the client and server to make pushing things easer, and there are plans to perform static analysis with bots.

happy bmo push day!

the following changes have been pushed to : user fields are cleared when navigating back to a page (eg. after an error) : help docs not available for User Preferences : gravatar container for comment 0 can be too wide : Button edges are hard to see at most zoom-levels, with bmo “experimental user interface” : Buttons besides ‘User Story’ overflow if the user story is a one-liner : linkification isn’t applied to the user-story field : Make login UX mobile friendly to assist mobile authentication workflow : BMO login being reset for every browser session : make the TOTP MFA code field type=number on mobile : Release Tracking Report doesn’t return bugs with tracking flags set to — when searching for “not fixed” values : Update crash signatures to match new Socorro signature generation : cannot remove other people from the cc list : Remember expanded/collapsed sections : Always show “never email me about this bug” checkbox : needinfo requests where the current user is the requestee shouldn’t be editable : add support for duo-security

happy bmo push day!

the following changes have been pushed to

  • [1209332] Make the master kick-off bug “Confidential Mozilla Employee Bug” by default
  • [1209971] Suggested reviewers should exclude the current user from the list displayed
  • [1200958] group owners should always be able to view group membership reports for their groups
  • [1196620] support automatic removal of inactive users from groups
  • [1210654] “MozReview Requests” is not shown in the page after submitting a change
  • [1210762] User stories aren’t saved as part of the “remember values as bookmarkable template”
  • [1198519] Add link to bug history page to the top-right drop-down menu
  • [1210246] help link is busted
  • [1210690] Display only commits and only relevant data
  • [1205748] Can’t mark inaccessible bug dependent on a regression it caused
  • [1164063] show a warning near the attachments table for sec-high/sec-crit bugs without sec-approval? on patches
  • [1211750] Changing password with MFA turned on will not work

discuss these changes on

happy bmo push day!

the following changes have been pushed to

  • [1207926] change treeherder@bots.tld to orangefactor@bots.tld
  • [1204683] Add whoami endpoint
  • [1208135] security not being mailed when bugs change core-security-release state
  • [1199090] add printable recovery 2fa codes
  • [1204623] timestamp on flags should reference the latest updated activity, not the first
  • [1209745] Update get_permissions.html.tmpl to reflect new self-canconfirm process

discuss these changes on

happy bmo push day!

the following changes have been pushed to

  • [1203991] change the suggested fxos 2fa app (again)
  • [1204704] Backport upstream testsuite changes to test against bug 1202447
  • [1205683] Feature request: STR and regression-range pulldowns
  • [1199087] extend 2fa protection beyond login
  • [1204645] the ‘last seen’ value in the group membership report should use a profile’s last-seen date, not the cookie

discuss these changes on