Moar MozReview feedback

[Steve Fink]

Ok, I just did another MozReview review, which means I will subject this
list to another steaming pile of feedback:

All comments here pertain to my experiences with
<https://reviewboard.mozilla.org/r/20433/>. This is a stream of
consciousness; some things I figured out as I went along. I'm leaving it
as-is because I'm guessing my confusion accurately reflects some
non-negligible subset of other users'.

- the review status display is great!
- The review status button isn't. (Isn't a button, I mean.)
- clicked on "Reviews" link. Shows description. No patch. Confused.
- clicked on "view diff" in top right. Shows patch.
- can't figure out how to mark r+. No ship it buttons or anything.
- oh. "Complete Diff" link too that I didn't see at first. But it
doesn't get marked visited by the top right button. Not sure if it's the
same.
- ah! "Log in" is displayed. A "log in to review patch" note somewhere
would help. Or a big "Not logged in; viewing in read-only mode" header
or something.
- logged in. Status button still isn't. [a button]
- I guess I'm viewing the diff? No action buttons are available. Need to
choose from "Finish Review" and "View Reviews".
- "View Reviews" -- still no way to take action.
- "Finish Review" -- finally! But now I want to go back to the patch. Is
that "Save"? What's the difference between "Publish Review" and "Save"?
s/Save/Save For Later/ or s/Save/Keep As Draft/?
- "Publish Review" isn't describing my main purpose, which is more like
"Accept Patch".
- oh, and I remember the tooltip for the status non-button said "a
reviewer hasn't marked Ship It" or something, but on the "Finish Review"
page, the "Ship It" terminology is not used.
- Oh. Now it says "You have a pending review." So s/Save/Keep As Pending/?
- in general: why such a separation between reviews and diffs? I'm
happier thinking of them as more or less the same thing. I'd rather
eliminate the separate tabs, and have an expander button to view the
diff *right about where the diff will appear*.
- generally, the spatial flow is weird. I seem to keep going between
different parts of the screen. Where I select things is far from where
they appear or where I need to do the next thing. I'm not sure what
things are changing the display [a good use of tabs] and what things are
taking actions [a bad use of tabs].
- double-clicking on a line doesn't create an issue. I have to know to
double-click the line number. (Big difference from splinter.)
- highlighting a range of lines brings up a "Your comment" box, but
immediately un-highlights the lines I selected
- comment box jumps down as soon as I start typing
- oh! Now I get it. The review display is meant to accumulate my
comments, as opposed to the code display. Hmmm... maybe have
"Review"/"Diff" tabs down below the summary area? And please, when
selecting one or the other, have a header line saying which one you're
on. Right now, I have no idea which view I'm looking at, partly because
I was so unclear on what each of them meant.
- I don't like the tab-like things in the top right. They aren't tabs.
The header area stays constant and some of them are actions, not
alternate displays. The actions should be moved to buttons, preferably
more appropriately-placed, and the displays should be real tabs on top
of whatever they're displaying.
- after finishing my review, I'm left staring at the page wondering if
there's something left for me to do. Even a "Done for now" button that
takes me to my dashboard would help.
- speaking of the dashboard, having the parent request and the child
request as separate line items feels weird. If there were an indent
somewhere, and/or a collapse/expand twisty, it'd be fine.
- I reviewed a patch and finished the review with one issue open. It's
still set to r? in bugzilla, which means my bugzilla dashboard still has
it in my todo list. With splinter, I would've marked it r+ as in "r=me
after these issues are addressed". It would be fine if the review flag
were cleared in the meantime instead, if mozreview is going to remember
to mark it r+ after all issues are addressed.

Ok, now the author updated the patch. Or rather, he added on a followup
patch? At any rate, my workflow got really weird:

- my issue disappeared
- I expected to see the issue marked as fixed somewhere/somehow, and
for that to trigger an r+
- instead, when I went back to the review I saw two new patches
listed, both labeled r?, and the issue was gone
- I had to do "Ship It!" on *both* of these patches. I didn't want to
have to do that with either one -- I felt like I should have already
been done with this review already, since my intention in the first
place was to mark the whole thing "r=me, but fix this minor issue before
landing".
- perhaps I did something incorrectly? Is there some way of expressing
the above intention?
- alternatively, did the author do something incorrectly? Is he
pushing in a way that is destroying metadata that would have associated
a new patch with the old?
- wait, now when I go back to the review, I see the issue. It's
totally possible that I just missed it.
- now the page layout has the issues contained within the main box,
and then a series of boxes below that seem to represent an audit trail
of things that happened with the issue. Previously, I thought that when
in displaying-the-review mode, it was showing a summary box and then the
list of issues, and in diff-mode, it showed the summary box and then the
diffs. The data model is still unclear. Or maybe I mean that what the
view represents is unclear.
- right now, I'm thinking that I would prefer to have a summary box,
and below it tabs for selecting issues, history, or changes. If indeed
those are the 3 main things it's trying to convey. The tabs could have
counts in them -- although that would make the exact text variable,
which is a little sketchy, I think in this case the variation would be
minor enough.
- ok, while looking again at the review, it seems like the author
fixed the issue by posting a new follow-on patch, without marking the
issue resolved. Should I be marking it resolved? If I did, would it have
done the "Ship It" for me?
- who is intended to "own" the issue states? As in, is it meant to be
helpful checklist for the author? Or a way for the reviewer to break
down acceptance of a change into multiple issues -- as in, the reviewer
decides whether he feels the updates have adequately resolved the
issues? Is that intended to be a social convention, or is there an
intention one way or the other? (I could imagine separate things for the
author to mark an issue as "resolved as I understand it" vs the reviewer
marking an issue "I agree that the update resolved this issue", and
perhaps mozreview treating those as equivalent. But... well, that's just
one possibility.)

Thanks for listening!

[Mark Côté]

Hey Steve, thanks for the detailed feedback! This is a very detailed
user study. :)

One source of a lot of your confusion, which I know we absolutely need
to fix, is that you ended up leaving your first review on the
"parent"--the sort of meta review request that ties together all the
commits. This is where you end up from the "Complete Diff" and "Review
Summary" links--totally confusing, it turns out. :) The concept of a
parent review request is something we had to add to Review Board to
support a series of commits that fixes a single bug/problem (versus
large, monolithic commits), and it has become excruciatingly obvious
that we did this in a totally unintuitive way. It's doubly confusing to
reviewers when there is only one commit--the parent request was meant to
give an overview of all the commits together, in case you want to see
the "entire solution" as it were. But with one commit, well, the parent
and child diffs end up being the same, and it's totally not obvious
where you're supposed to leave your review. I imagine we'll probably
just turn off reviews on the parents altogether and maybe just leave the
complete diff for reference.

I'm still digesting the rest of your feedback, but this is really
valuable for Q4, which we are dedicating to improving MozReview's
usability, particularly for new users.

One aside: Review Board issues are, I believe, mainly a convenience for
the patch author--a to-do list of sorts. I'm not totally sure as to why
Review Board lets reviewers also mark them as Fixed and Dropped, except
maybe as a convenience or a way to support alternative work flows. I
agree that part is confusing; I'm torn between disabling them for
reviewers to make the suggested workflow clear vs. leaving it alone to
allow reviewers and authors to decide themselves how they want to
resolve issues.

Finally, in this case the author should have probably updated the commit
rather than submitting a follow up--we're trying to reinforce the idea
that commits should be small and standalone, which, among other
benefits, generally makes the reviewer's job easier (that's why we added
the aforementioned parent review requests to support commit series). But
this is just a sort of convention that needs to be communicated out
repeatedly; there's not much we can do technically to enforce this, if
we wanted to.

Mark

[Steve Fink]

On Friday, October 2, 2015 at 2:16:39 PM UTC-7, Mark Côté wrote:

Hey Steve, thanks for the detailed feedback! This is a very detailed
user study. :)

Oops, sorry, I didn't see this reply until now.
 

One source of a lot of your confusion, which I know we absolutely need
to fix, is that you ended up leaving your first review on the
"parent"--the sort of meta review request that ties together all the
commits. This is where you end up from the "Complete Diff" and "Review
Summary" links--totally confusing, it turns out. :) The concept of a

Oh... I see. I mean, I knew about that aspect of the data model, but I thought I had explicitly navigated to the right thing. You're probably right, I must've gone astray via those links.
 

parent review request is something we had to add to Review Board to
support a series of commits that fixes a single bug/problem (versus
large, monolithic commits), and it has become excruciatingly obvious
that we did this in a totally unintuitive way. It's doubly confusing to
reviewers when there is only one commit--the parent request was meant to
give an overview of all the commits together, in case you want to see
the "entire solution" as it were. But with one commit, well, the parent
and child diffs end up being the same, and it's totally not obvious
where you're supposed to leave your review. I imagine we'll probably
just turn off reviews on the parents altogether and maybe just leave the
complete diff for reference.

Generally speaking, I feel like the UI lacks a firm sense of "place" -- as in, what exactly it is that I'm looking at right now. Whether I'm on the parent request vs a child request and the diff view vs a request(?) view should be more immediately obvious, and definitely shouldn't require me to remember which button/link/tablike I clicked on last.
 

I'm still digesting the rest of your feedback, but this is really
valuable for Q4, which we are dedicating to improving MozReview's
usability, particularly for new users.

One aside: Review Board issues are, I believe, mainly a convenience for
the patch author--a to-do list of sorts. I'm not totally sure as to why
Review Board lets reviewers also mark them as Fixed and Dropped, except
maybe as a convenience or a way to support alternative work flows. I
agree that part is confusing; I'm torn between disabling them for
reviewers to make the suggested workflow clear vs. leaving it alone to
allow reviewers and authors to decide themselves how they want to
resolve issues.

That's a totally valid approach, but I guess it's not the one I would have expected by default. To me, a code review is a conversation (and sometimes negotiation) between the author and reviewer. I assumed an issue was like a "mini-bug" and, like a bug, it would be filed by one person [the reviewer], assigned to another [the patch author], record the discussion, and then record the outcome whether it's "fixed", "reviewer changed mind/is satisfied with current state", or whatever.

Hm... if the current setup has separate Fixed and Dropped states, then it sounds like that's more or less what it's trying to support?

Which then leaves open the ambiguity of who is supposed to mark the issues "Fixed". I can imagine that there would be two main initial review outcomes involving issues: either "r=me after you fix these things", meaning the reviewer trusts the author to make the changes, or "r- for now, these are the issues I want to see resolved". I guess if you wanted to fully capture the intent, the reviewer would mark the issues as blocking vs nonblocking, or "I trust you to resolve this somehow" vs "get my agreement on whatever resolution you come up with for this issue". But that starts to feel heavyweight.

Then there's the question of whether the author has to explicitly resolve each issue or whether they can upload an obsoleting changeset that erases all the previous issues. (And given that mozreview wants to eventually see the final version of all patches, and that it should be possible to use evolve or whatever to locally mutate things and then re-upload, there really isn't a good way for an author to specify an intent here. Bleh. Too much process, probably. Should pick one and at most provide a "blow away all previous issues and request a fresh review" button or something.)

Maybe my problem is that I expected issues to be a workflow management device, to avoid the fairly common current situation where some review comments are accidentally left unaddressed. (At least, I know I do that.) I was hoping as a reviewer to be able to say "r=me with issues A, B, and C" and have the tool automatically set r+ when the issues are handled. But perhaps issues are simply a way for the reviewer to give feedback in the form of bullet points instead of comment paragraphs.
 

Finally, in this case the author should have probably updated the commit
rather than submitting a follow up--we're trying to reinforce the idea
that commits should be small and standalone, which, among other
benefits, generally makes the reviewer's job easier (that's why we added
the aforementioned parent review requests to support commit series). But
this is just a sort of convention that needs to be communicated out
repeatedly; there's not much we can do technically to enforce this, if
we wanted to.

Yeah, I like the current data model, including the multiple changesets under a persistent umbrella request. Especially if I can have a 4-part patch series and upload newer versions of patches in the middle without having mozreview lose the connection between the old and new versions. It just needs to come through a little better in the UI.

[Mark Côté]

My turn to apologize for the delay. :)

On 2015-10-07 1:31 AM, Steve Fink wrote:
 

parent review request is something we had to add to Review Board to
support a series of commits that fixes a single bug/problem (versus
large, monolithic commits), and it has become excruciatingly obvious
that we did this in a totally unintuitive way. It's doubly confusing to
reviewers when there is only one commit--the parent request was meant to
give an overview of all the commits together, in case you want to see
the "entire solution" as it were. But with one commit, well, the parent
and child diffs end up being the same, and it's totally not obvious
where you're supposed to leave your review. I imagine we'll probably
just turn off reviews on the parents altogether and maybe just leave the
complete diff for reference.

Generally speaking, I feel like the UI lacks a firm sense of "place" -- as in, what exactly it is that I'm looking at right now. Whether I'm on the parent request vs a child request and the diff view vs a request(?) view should be more immediately obvious, and definitely shouldn't require me to remember which button/link/tablike I clicked on last.

Yup, totally understood. I just wrote up a blog post about some basic ideas for improving this--or at least some fundamentals that we'll settle on in order to build up a more intuitive UI. I'll put the link in another post shortly; your feedback would be appreciated. :)

 

I'm still digesting the rest of your feedback, but this is really
valuable for Q4, which we are dedicating to improving MozReview's
usability, particularly for new users.

One aside: Review Board issues are, I believe, mainly a convenience for
the patch author--a to-do list of sorts. I'm not totally sure as to why
Review Board lets reviewers also mark them as Fixed and Dropped, except
maybe as a convenience or a way to support alternative work flows. I
agree that part is confusing; I'm torn between disabling them for
reviewers to make the suggested workflow clear vs. leaving it alone to
allow reviewers and authors to decide themselves how they want to
resolve issues.

That's a totally valid approach, but I guess it's not the one I would have expected by default. To me, a code review is a conversation (and sometimes negotiation) between the author and reviewer. I assumed an issue was like a "mini-bug" and, like a bug, it would be filed by one person [the reviewer], assigned to another [the patch author], record the discussion, and then record the outcome whether it's "fixed", "reviewer changed mind/is satisfied with current state", or whatever.

Hm... if the current setup has separate Fixed and Dropped states, then it sounds like that's more or less what it's trying to support?

Which then leaves open the ambiguity of who is supposed to mark the issues "Fixed". I can imagine that there would be two main initial review outcomes involving issues: either "r=me after you fix these things", meaning the reviewer trusts the author to make the changes, or "r- for now, these are the issues I want to see resolved". I guess if you wanted to fully capture the intent, the reviewer would mark the issues as blocking vs nonblocking, or "I trust you to resolve this somehow" vs "get my agreement on whatever resolution you come up with for this issue". But that starts to feel heavyweight.

So you can "ship it" with open issues. It will say "Fix it, then ship it!" in the UI (in yellow I believe), which will then change to a green "Ship it!" when those issues are all marked fixed or dropped. This will also r+ the patch and leave a comment in Bugzilla as usual. So this is pretty much a "r+ with nits" flow as done in Bugzilla now.

Similarly, you can leave a review without a "ship it" and one or more open issues, which is the equivalent of "r-; please fix then resubmit for review".

Then there's the question of whether the author has to explicitly resolve each issue or whether they can upload an obsoleting changeset that erases all the previous issues. (And given that mozreview wants to eventually see the final version of all patches, and that it should be possible to use evolve or whatever to locally mutate things and then re-upload, there really isn't a good way for an author to specify an intent here. Bleh. Too much process, probably. Should pick one and at most provide a "blow away all previous issues and request a fresh review" button or something.)

Hm yeah, I can't remember what it does in this case. I'd be a bit concerned if it automatically closed all issues, since there's the possibility of the author forgetting about one, but I can also see how it would be confusing for there to be open issues on an older commit. As you say, we could prompt the user when they push if there are open issues, but I'm not sure if that's worth the complexity. Maybe. This is a bit tough to sort out.

Oh and yes, MozReview supports evolve, and it does indeed work really well. If you have evolve activated and reorder commits, MozReview will understand what happened and show you appropriate interdiffs and such.

Maybe my problem is that I expected issues to be a workflow management device, to avoid the fairly common current situation where some review comments are accidentally left unaddressed. (At least, I know I do that.) I was hoping as a reviewer to be able to say "r=me with issues A, B, and C" and have the tool automatically set r+ when the issues are handled. But perhaps issues are simply a way for the reviewer to give feedback in the form of bullet points instead of comment paragraphs.

So, I actually agree with your expectation. That's how I see those issues--I would hope that all issues on a given review are fixed or dropped before the commit is landed on production. We can't really enforce that, of course, because if the author is L3 they can technically land whenever they want, at least if the reviewer agrees, but we will be enforcing this with autoland (the autoland button will be disabled as long as there are open issues).

 

Finally, in this case the author should have probably updated the commit
rather than submitting a follow up--we're trying to reinforce the idea
that commits should be small and standalone, which, among other
benefits, generally makes the reviewer's job easier (that's why we added
the aforementioned parent review requests to support commit series). But
this is just a sort of convention that needs to be communicated out
repeatedly; there's not much we can do technically to enforce this, if
we wanted to.

Yeah, I like the current data model, including the multiple changesets under a persistent umbrella request. Especially if I can have a 4-part patch series and upload newer versions of patches in the middle without having mozreview lose the connection between the old and new versions. It just needs to come through a little better in the UI.

Yup, that should all work. As for the UI, please look out for my next post & check out some of the ideas we're lobbing about. :)

Thanks,
Mark