Review Board and Bugzilla Interaction

[Gregory Szorc]

We intentionally put off the discussion about Review Board emails and
Bugzilla integration until after MozReview was launched. Now that we're
past that initial milestone and feedback is coming in, I'd like to have
that conversation.

As is currently implemented:

* Review Board emails are disabled
* Reviews on Review Board require an associated bug
* Reviews are posted to Bugzilla as comments coming from the reviewer

There are a few problems/shortcomings with this approach. In the
following sections I'll describe them and possible solutions.

(These are only my feelings. I welcome contrasting opinions.)

Duplication of review content on Bugzilla
=========================================

Review content is canonically stored in Review Board and accessible
there via a rich, HTML interface. Review content cross-posted to
Bugzilla is duplicated and rendered as plain text.

I have concerns about cross-posting content to Bugzilla.

By posting content to 2 mediums, it raises confusion on where to post
replies and follow-ups. The comment box is Bugzilla is very tempting.
And that temptation is reinforced by years of existing behavior.

Furthermore, the plain text review comments on Bugzilla are not as
powerful as they are in Review Board. For example, individual comments
in Review Board are hyperlinked and allow you to go directly to the diff
view and the code.

I like the idea of posting comments to Bugzilla when review activity
occurs. But I think it should be a high-level summary, rather than the
low-level content. I think this clearly establishes the boundary between
code review activity and bug tracking / progress activity. As I blogged
about recently [1], I think having everything under one roof just adds
chaos and confusion.

A downside is some people may lament the loss of Bugzilla comments and a
departure from "the way it has always been done." This may hurt adoption
of MozReview. A subset of those users may object on the basis of losing
flat text comments. Those users should be placated by adding a "plain
text view" of comments on Review Board.

Review Board Emails Can be Useful
=================================

Review Board supports rich HTML emails (or plain text if users don't
want HTML). Like the review comments in the web interface, these feature
hyperlinks that allow users to perform common actions quicker. This
saves a click or two and allows people to perform actions quicker. i.e.
increasing developer productivity. On this ground, enabling Review Board
emails sounds like something we should do.

Another problem to consider is review requests not attached to bugs. See
the use case in bug 1092341. Essentially an image review is conducted by
manually uploading images to Review Board. None of our on-push bug #
enforcement comes into play, so there is nowhere to send review request
updates to. Emails would send them somewhere, which is better than nowhere.

A con for emails is that it is yet another source of email for
developers. I think Mozillians are conditioned to seeing Bugzilla email
and that's it. Although, the GitHub crowd is likely seeing GitHub emails
and I'm sure they've learned to adapt. But the combination of Review
Board *and* Bugzilla email updates for a single review could be too
much. I know I wouldn't like receiving 2 emails for the same event!
People would just end up resenting Review Board because it creates
extra, unwanted spam.

I wonder if we could add functionality to Bugzilla that didn't send the
emails if Review Board already has them covered. This would likely
require a special API in Bugzilla "postReviewComment(text,
review_request, list_of_review_subscribers)" or something similar. Feels
a bit hacky. But it makes the email overload go away.

Another problem that needs solved is subscribing to notifications.
Bugzilla has this solved today via component watching. It is self
service. It is nice. Review Board kinda/sorta has this through default
reviewers and review groups. Although I *think* it requires admin
privileges to update. That's no good.

We need a self-service mechanism in Review Board where people can
subscribe to reviews much like they subscribe to reviews. I'm not sure
how we do this. Expose UI to manipulate review groups and/or default
reviewers outside of the admin interface? Somehow query Bugzilla for
watchers of a component and update the reviewer list in Review Board
with that account list? I dunno.

A related problem is people wanting to subscribe to repository changes.
I know a few people run custom email/RSS services that allow them to
follow actual code changes (as opposed to just Bugzilla activity). This
has significant overlap with "review watching." Justification for a
shared database or code watching mechanism?

Requiring a Bug for Reviews
===========================

We currently require a bug number for all pushed reviews. As I've said
before, I think this is unnecessary overhead. I think people should be
able to push code to initiate review. If a module member thinks code is
acceptable, we should be able to land without a bug. Less overhead
through optional bugs makes us move faster.

I understand not all projects may be able to partake in this brave new
world. Firefox release process is heavily dependent on bugs. They like
bugs as anchors both for reference and tracking. For people that
absolutely, positively must have a bug, I think MozReview should create
the bug # as a formality when the bugless review request is submitted.
This fulfills the requirements of the bug and removes some overhead for
code authors. (I think GitHub does something like this.)

I don't think people are yet ready for a bugless world. I don't think we
should be actively pushing people to this future just yet. But what I do
know is a) the bugless world can't be achieved if we don't have non-bug
notifications of review activity b) people will be very angry if they
can't subscribe to review activity like they can in Bugzilla today.

I'm not saying we should design things with a bugless future in mind.
But it is certainly worth considering when we think about user
interaction and how to further remove developer hurdles when writing
code and conducting code review.

My Recommendations
==================

In order:

1) We change Bugzilla comments to be high-level summaries of review
activity. "gps added 2 comments to the review at XXX." Emphasize Review
Board, not Bugzilla, as the home for review content.
2a) We enable email from Review Board.
2b) We figure out a way to prevent double emails of review activity.
3) Start thinking about self-service mechanism for subscribing to review
activity / emails
4) (way down the line) think about making bugs optional and more
frictionless.

[1]
http://gregoryszorc.com/blog/2014/10/27/implications-of-using-bugzilla-for-firefox-patch-development/

[Mark Côté]

I actually have very little to add. I think it probably does make sense
to keep the code review itself inside Review Board, with a summary. The
fact that the comment box is tempting is something that shouldn't be
taken too lightly, but it's pretty much unavoidable that going from the
bug to a review reply will involve a few clicks and some loading time.
We just have to make the code-review experience awesome enough that this
inconvenience will be forgiven by developers. Regardless, this is the
time to try it, when we're just getting started; removing comment
mirroring later will undoubtedly face more resistance.

I don't have much of an opinion on reviews without bugs. I'm sure that
project managers/leads/heavily involved developers may have opinions
with regards to their own projects though. Maybe it's worth doing on a
repo-by-repo basis.

One way to suppress email would be by tagging a comment upon creation,
e.g. with "mozreview", and making Bugzilla ignore email for those
comments. This runs the risk of abuse, but we could maybe do something
with IP addresses or some other mechanism to ensure that email
suppression would only take effect from comments posted by the MozReview
system. I'll bring this up at the BMO meeting tomorrow.

Subscribing/CCing to reviews is an interesting point. I have no thoughts
on this at the moment, and it's not urgent, but we should hash something
out.

In sum, I fully agree with your 4-point plan and believe we should start
on it more-or-less immediately.

Mark

[Mark Côté]

To follow up, I talked to the BMO devs, and we're already close to
having what we need to suppress emails for Review Board comments via the
WebServie APIs (bug 1062718).

On the same subject, should we bother posting an attachment for review?
As was pointed out today in IRC, it's an extra step to r- in Bugzilla,
since there's no related status in Review Board, and it's not really the
same since the review process in Review Board is on going. But keeping
an r? open for the entire review cycle messes with tools that report on
reviews right now (like the daily reminders), since that's not what it
means in Splinter review land. Maybe we could add another field in
Bugzilla (nothing I would recommend lightly).

Mark

[Gregory Szorc]

On 11/7/14 6:52 PM, Mark Côté wrote:
> On the same subject, should we bother posting an attachment for review?
> As was pointed out today in IRC, it's an extra step to r- in Bugzilla,
> since there's no related status in Review Board, and it's not really the
> same since the review process in Review Board is on going. But keeping
> an r? open for the entire review cycle messes with tools that report on
> reviews right now (like the daily reminders), since that's not what it
> means in Splinter review land. Maybe we could add another field in
> Bugzilla (nothing I would recommend lightly).

The big thing attachments have going in their favor is that people are
accustomed to them. Furthermore, people are accustomed to the dashboards
on Bugzilla for seeing what actions they need to take. I think having
Review Board stand on its own at this early juncture may hurt adoption.
It might be too unsettling for some.

I agree that mapping MozReview workflows to review flags doesn't really
work currently. As an alternative to changing Bugzilla, we could change
Review Board. We could add an additional review flag state to Review
Board. For example, we could have "review pending" and "review complete"
to track whether an individual has attended to a review request. Simply
publishing a new or updated review request would constitute a "Review
pending" and would map to r?. "Ship It" reviews would map to r+ (like
today). And "Review complete" (without a Ship It) would map to
cancelling the review flag. There would be no mapping to r-. "Review
complete" also partially helps solve the problem of "do I or don't I
need to do anything with this review request."

But like adding Bugzilla fields, I'm also a bit reluctant to add fields
to Review Board. I feel like we need to think this one through.

[Andreas Tolfsen]

On Sat, Nov 8, 2014 at 5:50 AM, Gregory Szorc <g...@mozilla.com> wrote:
> "Ship It" reviews would map to r+ (like today). And "Review complete"
> (without a Ship It) would map to cancelling the review flag. There would
> be no mapping to r-.

I think this makes sense. This addresses the problem of potentially
lengthy reviews getting stuck in your review queue.

[Mark Côté]

On 2014-11-08 12:50 AM, Gregory Szorc wrote:
> I agree that mapping MozReview workflows to review flags doesn't
> really work currently. As an alternative to changing Bugzilla, we
> could change Review Board. We could add an additional review flag
> state to Review Board. For example, we could have "review pending" and
> "review complete" to track whether an individual has attended to a
> review request. Simply publishing a new or updated review request
> would constitute a "Review pending" and would map to r?. "Ship It"
> reviews would map to r+ (like today). And "Review complete" (without a
> Ship It) would map to cancelling the review flag. There would be no
> mapping to r-. "Review complete" also partially helps solve the
> problem of "do I or don't I need to do anything with this review
> request."
>
> But like adding Bugzilla fields, I'm also a bit reluctant to add
> fields to Review Board. I feel like we need to think this one through.

Since you have to actually publish a review for it to be seen, maybe we
could treat the review as "review complete" if "ship it" isn't selected
when it's published? Or if we want to be extra explicit, we could add
options alongside the "Publish" button indicating if the review is
complete (and maybe add the Ship It button there too). We wouldn't have
to track any internal state; publishing would just trigger a Bugzilla
attachment flag-state change, as we do now when publishing review
requests and with the "ship it" option.

Mark

[James Graham]

On 08/11/14 02:52, Mark Côté wrote:
> On the same subject, should we bother posting an attachment for review?
> As was pointed out today in IRC, it's an extra step to r- in Bugzilla,
> since there's no related status in Review Board, and it's not really the
> same since the review process in Review Board is on going. But keeping
> an r? open for the entire review cycle messes with tools that report on
> reviews right now (like the daily reminders), since that's not what it
> means in Splinter review land. Maybe we could add another field in
> Bugzilla (nothing I would recommend lightly).

I approve of any solution that moves us away from having an r- marker.
Being expected to put r- on a patch that the reviewer fully expects to
land is demotivating for the patch author. Long time contributers are
used to this of course, but for others it is a very negative response to
their hard work.

Eventually, of course, I hope that people get used to the idea of using
the review system to manage code reviews and we can ditch the idea of
duplicating bits of code review state onto to the bug tracker altogether.

[Gregory Szorc]

I like the simplicity of this.

But one thing it doesn't buy us is the partial review use case. People
sometimes want to publish a review over say half the files and continue
the review later. Having the r? is a good reminder that work needs to be
done. A similar use case is a reviewer wanting to get extra context
before finishing the review. That question would constitute a review
from Review Board's perspective, removing r?.

I support adopting the simple solution (removing r? on review publish
that doesn't include Ship It) and solving the "review not complete" use
case later.

[Mark Côté]

Yeah, that would be solved by my second suggestion, having check boxes
beside the Publish button, indicating if you want to close the review
(r+ or remove r?).

> I support adopting the simple solution (removing r? on review publish
> that doesn't include Ship It) and solving the "review not complete"
> use case later.

Works for me; that requires no UI changes.

Mark