ReviewBoard feedback

[Steve Fink]

Since I used it for real for the first time, I thought I'd write up my
current feedback. Obviously, this is from a naive user's first
encounter, and you know about most/all of these already, but perhaps
it'll still be helpful:

- "Depends On" list should cross out the ones that are reviewed, or
otherwise indicate that the user is "done" for now.

- If there are new comments somewhere, that should probably be indicated
in either the Depends On list or the parent thing's list.

- Should be able to comment on code regions. Oops! gps tells me you can.
So: commenting on code regions should be more discoverable. (I double
clicked as per Splinter. Er... maybe a tooltip?)

- Ship It button should be available in the screen containing the
reviewed code

- Parent ...thing?... should show state of each subthing (r=somebody,
somebody r-'d, etc.)

- Should be able to r- as well as r+ (though perhaps we should use
something else that doesn't risk inferred value judgements that were not
intended)

- The "Ship It!" tabbutton should not be a tabbutton. That isn't a thing
and shouldn't be. It should be a button. Or a state indicator.

- it would be nice if RB urls had a bug number in them, even if it
wasn't used. https://rb.moz.org/r/1234/?bug=4321 maybe.

- the sub-things should perhaps be displayed like hg log --graph or
somesuch. Assuming that matches the data model. I can't figure out the
RB data model from the UI.

- the subthings should be labeled with the requested reviewer

- the "ship it" button should be available when adding a comment

- "ship it" shouldn't be called "ship it". r+ is more meaningful. Just
because I'm ok with a code change doesn't mean I necessarily want it to
ship, or land. (Especially when it's only for a subthing that doesn't
make sense without the other subthings.)

- what *are* the things, anyway? They're not bugs, nor issues.
Changesets? If I obsolete a changeset, does the newer one get its own
thing id?

- having a review "tab" that just brings up an empty screen with just a
checkbox and a textbox is just obnoxious

- the top "Reviews/Diff" tabs don't indicate which is active

- there are multiple people listed under the "Reviewers" list for my
parent thing. I'd like an expander or popup to say which of them are
associated with which subthings.

- The "Expand All" is unclear about what it's expanding. It seems to be
comments? Except it sticks around after expanding them.

- The ship it status should be indicated on the overall thing, not just
on one comment. (Both would be best.)

- The extra draft/public stuff for individual comments seems like
overkill. I guess the functionality is cool, but the default action
should just be to publish. Perhaps we could have an additional "safe as
draft" button. "Publish/Save As Draft/Cancel".

[Gregory Szorc]

Thanks for taking the time to write this feedback!

We generally know how lacking parts of the UI are. We have plans to improve them. Look for more in a few weeks. More responses inline.

On Thu, Jan 8, 2015 at 9:58 PM, Steve Fink <sf...@mozilla.com> wrote:

Since I used it for real for the first time, I thought I'd write up my
current feedback. Obviously, this is from a naive user's first
encounter, and you know about most/all of these already, but perhaps
it'll still be helpful:

- "Depends On" list should cross out the ones that are reviewed, or
otherwise indicate that the user is "done" for now.

I agree.

 

- If there are new comments somewhere, that should probably be indicated
in either the Depends On list or the parent thing's list.

This exists in "My Dashboard." We need to add something to the parent review to also expose this.

 

- Should be able to comment on code regions. Oops! gps tells me you can.
So: commenting on code regions should be more discoverable. (I double
clicked as per Splinter. Er... maybe a tooltip?)

- Ship It button should be available in the screen containing the
reviewed code

I agree. Having to load another page adds unnecessary overhead.

 

- Parent ...thing?... should show state of each subthing (r=somebody,
somebody r-'d, etc.)

I agree.

 

- Should be able to r- as well as r+ (though perhaps we should use
something else that doesn't risk inferred value judgements that were not
intended)

I'm not sure what we're going to do here. There is something nice to be said about a binary yes/no flag where absence of "ship ip" means no. I also like Gerrit's approach where they have +2, +1, 0, -1, and -2 to indicate a spectrum from "land this" to "never land this." That more clearly indicates reviewer wishes. Although it is slightly more complicated. We'll probably talk about this at the MozReview work week in a few weeks.

 

- The "Ship It!" tabbutton should not be a tabbutton. That isn't a thing
and shouldn't be. It should be a button. Or a state indicator.

Yeah, this UI is kinda bad. It will be improved.

 

- it would be nice if RB urls had a bug number in them, even if it
wasn't used. https://rb.moz.org/r/1234/?bug=4321 maybe.

I disagree. Bugs are metadata attached to commits and should not be part of the URL. Now, if we want to expose special URLs that allow us to display all reviews associated with a bug, that is another matter entirely. I would support this.

 

- the sub-things should perhaps be displayed like hg log --graph or
somesuch. Assuming that matches the data model. I can't figure out the
RB data model from the UI.

- the subthings should be labeled with the requested reviewer

I think you are referring to "issues?" https://www.reviewboard.org/docs/manual/2.0/users/reviews/issue-tracking/

Adding reviewer is a good idea.

Not sure what you mean by "hg log --graph" display. Can you explain?

 

- the "ship it" button should be available when adding a comment

- "ship it" shouldn't be called "ship it". r+ is more meaningful. Just
because I'm ok with a code change doesn't mean I necessarily want it to
ship, or land. (Especially when it's only for a subthing that doesn't
make sense without the other subthings.)

This is a carryover from Review Board. Bikeshedding over better terminology and workflow is definitely warranted.

 

- what *are* the things, anyway? They're not bugs, nor issues.
Changesets? If I obsolete a changeset, does the newer one get its own
thing id?

Review requests. See Review Board User Guide. https://www.reviewboard.org/docs/manual/2.0/users/

 

- having a review "tab" that just brings up an empty screen with just a
checkbox and a textbox is just obnoxious

I agree. Pretty sure we have a bug on file.

 

- the top "Reviews/Diff" tabs don't indicate which is active

We already have plans to refactor these tabs.

 

- there are multiple people listed under the "Reviewers" list for my
parent thing. I'd like an expander or popup to say which of them are
associated with which subthings.

This would be useful. Probably part of a refactored display on the review series.

 

- The "Expand All" is unclear about what it's expanding. It seems to be
comments? Except it sticks around after expanding them.

Which UI element is this? URL?

 

- The ship it status should be indicated on the overall thing, not just
on one comment. (Both would be best.)

We'll be adding better UI around granting review.

 

- The extra draft/public stuff for individual comments seems like
overkill. I guess the functionality is cool, but the default action
should just be to publish. Perhaps we could have an additional "safe as
draft" button. "Publish/Save As Draft/Cancel".

An additional button to go straight to publish is probably warranted.

Having publish after every comment is definitely not warranted. This results in GitHub-like notifications after every little change, which is crazy annoying and one of my least liked features about GitHub's code review. I'm sure you agree with me :)

[Steve Fink]

On 01/09/2015 01:21 PM, Gregory Szorc wrote:

Thanks for taking the time to write this feedback!

We generally know how lacking parts of the UI are. We have plans to improve them. Look for more in a few weeks. More responses inline.

On Thu, Jan 8, 2015 at 9:58 PM, Steve Fink <sf...@mozilla.com> wrote:- Ship It button should be available in the screen containing the

- Should be able to r- as well as r+ (though perhaps we should use
something else that doesn't risk inferred value judgements that were not
intended)

I'm not sure what we're going to do here. There is something nice to be said about a binary yes/no flag where absence of "ship ip" means no. I also like Gerrit's approach where they have +2, +1, 0, -1, and -2 to indicate a spectrum from "land this" to "never land this." That more clearly indicates reviewer wishes. Although it is slightly more complicated. We'll probably talk about this at the MozReview work week in a few weeks.

A numeric spectrum doesn't do much for me. I'd prefer named resolutions:

 1. it's good, no changes needed
 2. good with the changes requested in a comment, I don't need to re-review
 3. good with the changes requested in a comment, but I want to re-review before landing
 4. requires some changes, show me another version and I'll re-review
 5. the implementation may or may not be fine, but I don't want this functionality or change
 6. implementation strategy is not going to work out. You're headed in the wrong direction. We need to discuss a different approach.
 7. whatever this is about is invalid for some reason

#3 and #4 might be the same. Or perhaps #4 and #6 aren't different enough. Anyway, I see the appeal of the single bit, and yes you could describe the nuances in comments, but personally I'd like something like the 3-bit breakdown listed above since they're all pretty commonly used and so it'd be nice to not have to describe the exact meaning of the review flag all the time.

Note that the difference between #2 and #3 might be meaningful to the tooling -- if the final patches always end up going through RB, then #2 implies that a changeset that obsoletes a reviewed changeset is automatically marked as passing review, while that is not the case with #3.

Or at least, the above is my current leaning.

 

- it would be nice if RB urls had a bug number in them, even if it
wasn't used. https://rb.moz.org/r/1234/?bug=4321 maybe.

I disagree. Bugs are metadata attached to commits and should not be part of the URL. Now, if we want to expose special URLs that allow us to display all reviews associated with a bug, that is another matter entirely. I would support this.

Hm, I can see that. But let me describe what triggered this. I see people pasting RB urls into the IRC channel, and they're completely opaque. They're like pastebin urls. Especially with the current bug-centric workflow, that makes using RB feel clunkier than going through bugzilla. For the specific case of IRC, this could probably be solved by having firebot (or something) retrieve the title (do these have titles?) Or if it *does* have bugs associated with it, display those. That doesn't solve other channels for passing these around, but maybe they aren't worth worrying about.

Links to bugzilla attachments generally have the (redundant) bug number in them, which I use fairly often.

 

- the sub-things should perhaps be displayed like hg log --graph or
somesuch. Assuming that matches the data model. I can't figure out the
RB data model from the UI.

- the subthings should be labeled with the requested reviewer

I think you are referring to "issues?" https://www.reviewboard.org/docs/manual/2.0/users/reviews/issue-tracking/

I've seen an issue. That's kinda cool, but it'll take some getting used to. It was pretty cryptic what it was and what it meant when I encountered it. "Is it referring to the overall request? Or is this a new thing? How is it associated with the comment?"

You're right, though, I haven't read any of the RB documentation. I'm the norm there, though. :-)

I *think* I'm talking about what is in the manual toc as "Review Requests". They have a parent/child relationship? (Sorry, I'd read the manual and answer myself, but I'm in a rush atm.)

Not sure what you mean by "hg log --graph" display. Can you explain?

Do these review request things correspond to changesets? If so, I guess I'd like to see them chained together so I know which one comes first, which one obsoleted which other one, etc. And if they come from different repos, they would be separated out.

I don't know. Have to think more about it.

 

- the "ship it" button should be available when adding a comment

- "ship it" shouldn't be called "ship it". r+ is more meaningful. Just
because I'm ok with a code change doesn't mean I necessarily want it to
ship, or land. (Especially when it's only for a subthing that doesn't
make sense without the other subthings.)

This is a carryover from Review Board. Bikeshedding over better terminology and workflow is definitely warranted.

 

- what *are* the things, anyway? They're not bugs, nor issues.
Changesets? If I obsolete a changeset, does the newer one get its own
thing id?

Review requests. See Review Board User Guide. https://www.reviewboard.org/docs/manual/2.0/users/

Oh. You answered me. I think review requests are the only thing I've been talking about.

- The "Expand All" is unclear about what it's expanding. It seems to be

comments? Except it sticks around after expanding them.

Which UI element is this? URL?

Uh, it is just on a parent review request. It expands the comments. I'll try to dig up a url later.

 

- The extra draft/public stuff for individual comments seems like
overkill. I guess the functionality is cool, but the default action
should just be to publish. Perhaps we could have an additional "safe as
draft" button. "Publish/Save As Draft/Cancel".

An additional button to go straight to publish is probably warranted.

Having publish after every comment is definitely not warranted. This results in GitHub-like notifications after every little change, which is crazy annoying and one of my least liked features about GitHub's code review. I'm sure you agree with me :)

I need to think about that one.

[Gregory Szorc]

On Fri, Jan 9, 2015 at 2:03 PM, Steve Fink <sf...@mozilla.com> wrote:

On 01/09/2015 01:21 PM, Gregory Szorc wrote:

Thanks for taking the time to write this feedback!

We generally know how lacking parts of the UI are. We have plans to improve them. Look for more in a few weeks. More responses inline.

On Thu, Jan 8, 2015 at 9:58 PM, Steve Fink <sf...@mozilla.com> wrote:- Ship It button should be available in the screen containing the

- Should be able to r- as well as r+ (though perhaps we should use
something else that doesn't risk inferred value judgements that were not
intended)

I'm not sure what we're going to do here. There is something nice to be said about a binary yes/no flag where absence of "ship ip" means no. I also like Gerrit's approach where they have +2, +1, 0, -1, and -2 to indicate a spectrum from "land this" to "never land this." That more clearly indicates reviewer wishes. Although it is slightly more complicated. We'll probably talk about this at the MozReview work week in a few weeks.

A numeric spectrum doesn't do much for me. I'd prefer named resolutions:

 1. it's good, no changes needed
 2. good with the changes requested in a comment, I don't need to re-review
 3. good with the changes requested in a comment, but I want to re-review before landing
 4. requires some changes, show me another version and I'll re-review
 5. the implementation may or may not be fine, but I don't want this functionality or change
 6. implementation strategy is not going to work out. You're headed in the wrong direction. We need to discuss a different approach.
 7. whatever this is about is invalid for some reason

#3 and #4 might be the same. Or perhaps #4 and #6 aren't different enough. Anyway, I see the appeal of the single bit, and yes you could describe the nuances in comments, but personally I'd like something like the 3-bit breakdown listed above since they're all pretty commonly used and so it'd be nice to not have to describe the exact meaning of the review flag all the time.

Note that the difference between #2 and #3 might be meaningful to the tooling -- if the final patches always end up going through RB, then #2 implies that a changeset that obsoletes a reviewed changeset is automatically marked as passing review, while that is not the case with #3.

Or at least, the above is my current leaning.

There is infinite bikeshedding potential here.

I've also talked with some security people about implementing cryptographic signing to convey "r+." A technical solution has been devised. But it would require additional actions on reviewers (reviewers must sign commits as they are before they can land). We're not yet sure how we want to roll this out. Leaning towards only enforcing it for security components like NSS and cert validation first.

 

 

- it would be nice if RB urls had a bug number in them, even if it
wasn't used. https://rb.moz.org/r/1234/?bug=4321 maybe.

I disagree. Bugs are metadata attached to commits and should not be part of the URL. Now, if we want to expose special URLs that allow us to display all reviews associated with a bug, that is another matter entirely. I would support this.

Hm, I can see that. But let me describe what triggered this. I see people pasting RB urls into the IRC channel, and they're completely opaque. They're like pastebin urls. Especially with the current bug-centric workflow, that makes using RB feel clunkier than going through bugzilla. For the specific case of IRC, this could probably be solved by having firebot (or something) retrieve the title (do these have titles?) Or if it *does* have bugs associated with it, display those. That doesn't solve other channels for passing these around, but maybe they aren't worth worrying about.

Links to bugzilla attachments generally have the (redundant) bug number in them, which I use fairly often.

We should have an IRC bot that posts review info like we have for bugs. Please file a bug. If it is under the Firebot component (is there one?), please CC me, smacleod, mconley, and mcote.

For the purposes of friendly URLs, sure, we could probably make something with a bug available. But the bug part of the URL would probably do nothing. This is because in RB, bug #s are metadata and can change.

 

 

- the sub-things should perhaps be displayed like hg log --graph or
somesuch. Assuming that matches the data model. I can't figure out the
RB data model from the UI.

- the subthings should be labeled with the requested reviewer

I think you are referring to "issues?" https://www.reviewboard.org/docs/manual/2.0/users/reviews/issue-tracking/

I've seen an issue. That's kinda cool, but it'll take some getting used to. It was pretty cryptic what it was and what it meant when I encountered it. "Is it referring to the overall request? Or is this a new thing? How is it associated with the comment?"

You're right, though, I haven't read any of the RB documentation. I'm the norm there, though. :-)

I *think* I'm talking about what is in the manual toc as "Review Requests". They have a parent/child relationship? (Sorry, I'd read the manual and answer myself, but I'm in a rush atm.)

The review series parent-child foo is something we hacked RB to do until RB grows proper support for multi-commit reviewers.

Review requests traditionally map to changesets/commits. We've created a special "parent review request" entity that is essentially an umbrella entity. The RB docs won't mention this, of course. https://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview.html may :)