Closed Bug 1162667 Opened 9 years ago Closed 9 years ago

ReviewBoard comments that are posted to Bugzilla should include 3 lines of context around the line that was commented on

Categories

(MozReview Graveyard :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1255278

People

(Reporter: jaws, Unassigned)

References

Details

Splinter reviews include the previous 3 lines of context for review comments, but MozReview only includes the specific line being commented on. This often leaves out enough context to make sense of the review feedback purely from Bugzilla.
By clicking on the link, you can go to Review Board and view arbitrary amounts of context, along with a richer UI.  Is it just that this is too inconvenient for your work flow?
For me at least, yes, it is too inconvenient. The "richer UI" means it dumps you at the top-level view of the review request and then you have to go hunting around to find the comment and context. And obviously if there's not enough context posted to bugzilla it's harder to find.
(In reply to Mark Côté [:mcote] from comment #1)
> By clicking on the link, you can go to Review Board and view arbitrary
> amounts of context, along with a richer UI.  Is it just that this is too
> inconvenient for your work flow?

Yeah, I find it inconvenient especially when there is some of the context on Bugzilla but not enough to be useful.
Thanks, that's good to know.  The MozReview team's philosophy is trying to get people to stay inside of Review Board for all review-related activities--if there are blockers to that, we'd like to address them.

So first, there's always a link to the review itself in the Bugzilla comment.  For example see https://bugzilla.mozilla.org/show_bug.cgi?id=1159277#c9, which has the link https://reviewboard.mozilla.org/r/7885/#review6653 at the top of the comment.  From there, clicking on the file name takes you directly to the diff.  We could possibly even put links directly to the diff for each review comment too, although I'm not sure if that will make the Bugzilla comment itself less readable.
I don't think that the goal should be to always push people back to MozReview though.

When I get review feedback from someone I get bugmail which takes me to Bugzilla. I can then read through the review comments in the bug. So this is now asking me to take another step to review the comments that have been added. Splinter worked fine for this and rarely did I need the extra context beyond 3-4 lines. 

I think MozReview can improve here by including the 3-4 lines of context in the bug, but it still allows people to click a link to get more context when necessary. Most of the time though, 3-4 lines is plenty.
Long term we switch on MozReview rich email and limit Bugzilla updates to "review activity has occurred. see <URL>".

While we're not there yet, my inclination is to WONTFIX this request and not introduce more review activity in Bugzilla than we plan to support. It's always easier to add a new feature than to take one away. Let's focus on making MozReview usable rather than cargo culting existing sub-optimal workflows based around Bugzilla.
I'm with Kats and Jared on this one. I often read review comments directly in my email client, and having to open another website is a distraction.
It sounds like this would be solved by enabling Review Board emails, which are a lot richer and more useful.  We have some things to work out there, but we'll get to it.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
I'd rather have it in a bug than in emails, personally. But I guess having to hack around the new tools instead of the old tools isn't a big deal.
(In reply to Mark Côté [:mcote] from comment #9)
> It sounds like this would be solved by enabling Review Board emails, which
> are a lot richer and more useful.  We have some things to work out there,
> but we'll get to it.

Can we reconsider this? I think it's useful to have the context there in Bugzilla (as opposed to just in an email, which closing this as a dupe of bug 1092341 suggests), so that someone reading through a bug's comments on bugzilla can understand a comment containing review comments without having to separately load the review in MozReview.
Needinfo for comment 11. I continue to find the lack of context in bugzilla comments quite frustrating.
Flags: needinfo?(mcote)
As mentioned several months ago, Review Board provides a much richer context than Bugzilla ever will.

When this bug was originally filed, the diff context in Review Board was static. With our recent upgrade to 2.5, you can now expand context around each issue left in Review Board.

Per comment #3, perhaps we could address concerns by including a hyperlink in Bugzilla that takes you to the exact review comment/context.
What would be lost by including a few lines of context in the Bugzilla comment, for people who prefer reading the review comments in Bugzilla?
We'd anger the people who don't like the "spam." IIRC we once posted more context to Bugzilla and people complained that it was too verbose so we reduced it.
(In reply to Gregory Szorc [:gps] from comment #15)
> We'd anger the people who don't like the "spam."

But people already get that amount of "spam" with Splinter reviews. (Or were they counting on the Splinter -> MozReview transition to reduce it?)
I don't recall anyone complaining about extra context causing spam.  I recall people complaining about all the messages from, say, ReviewBot, but I can't imagine anyone saying 'no' about including, say, 5-10 extra lines in the diff.

My understanding is that it's actually a bit tricky to do this because of the way diff comments work in Review Board.  However I don't know if that's a bit tricky like an hour of work or a bit tricky like days of work.  Let me check and get back to you.
Flags: needinfo?(mcote)
Meant to leave that NI open...
Flags: needinfo?(mcote)
Product: Developer Services → MozReview
The tricky part here is with multi-line comments because of the way they are stored in Review Board.  However given that it seems (anecdotally, at least) that most comments are left on a single line, it makes sense to add extra lines just to them, and it should be easy as well.  If people want finer control over context, they can select exactly what they want.
Flags: needinfo?(mcote)
Sorry, this should have been duped to bug 1255278, which we deployed last week.  There are now five lines of context above the commented-line for the case in which only a single line was selected.  We didn't add any context below because it would be confusing as to where the comment belongs exactly (and 5 lines above is also how Splinter currently works).
\o/ Thanks for fixing this!
You need to log in before you can comment on or make changes to this bug.