1278386 - Consider changing the wording when a review requires fixes

Status: RESOLVED INVALID

(MozReview Graveyard :: Review Board: User Interface, defect)

Description

[Eric Rahm [:erahm]]

Currently there's a comment with my face shouting 'Fix it!' if I flagged an issue to be dealt with (even if I gave the review an r+). As a reviewer I try to make all of my comments constructive and positive, I'd prefer that my avatar convey that same tone.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Maybe something like this instead?

http://i.imgur.com/gA42Rc7.png

Comment 2

[Eric Rahm [:erahm]]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #1)
> Maybe something like this instead?
> 
> http://i.imgur.com/gA42Rc7.png

That seems reasonable to me.

Comment 3

[David Walsh :davidwalsh]

Attachment: MozReview: Add files in anticipation of 'Fix It' label change (Bug 1278386).

Review commit: https://reviewboard.mozilla.org/r/78210/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78210/

Comment 4

[David Walsh :davidwalsh]

Attachment: MozReview: Update 'Fix It!' label to say 'Feedback' (Bug 1278386).

Review commit: https://reviewboard.mozilla.org/r/78212/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78212/

Comment 5

[:glob ✱]

sorry about doing this, but this bug sparked quite a discussion in the mozreview team about if this change should happen.
the final consensus is that we will leave the existing text as-is.

the text needs to clearly and concisely convey that action is required by the patch author before it will be good to land, something that "fix it" achieves.

Comment 6

[Eric Rahm [:erahm]]

(In reply to Byron Jones ‹:glob› from comment #5)
> sorry about doing this, but this bug sparked quite a discussion in the
> mozreview team about if this change should happen.
> the final consensus is that we will leave the existing text as-is.
> 
> the text needs to clearly and concisely convey that action is required by
> the patch author before it will be good to land, something that "fix it"
> achieves.

Perhaps you should include your users in the conversation as well, there's a reason this bug was filed.

Comment 7

[Mark Côté [:mcote]]

(In reply to Eric Rahm [:erahm] from comment #6)
> Perhaps you should include your users in the conversation as well, there's a
> reason this bug was filed.

The conversation was partly about technical complexity, and partly about clarity.  Feel free to argue your case further; we have been known to change our minds in the past. :)

Comment 8

[Eric Rahm [:erahm]]

(In reply to Mark Côté [:mcote] from comment #7)
> (In reply to Eric Rahm [:erahm] from comment #6)
> > Perhaps you should include your users in the conversation as well, there's a
> > reason this bug was filed.
> 
> The conversation was partly about technical complexity, and partly about
> clarity.  Feel free to argue your case further; we have been known to change
> our minds in the past. :)

Okay see comment 0, I don't want to use a tool that makes me seem actively hostile to the person I'm reviewing.

Comment 9

[Mark Côté [:mcote]]

Right, we get that.  Unfortunately this is pretty ingrained in Review Board and would involve a nontrivial amount of work to change.  Is there some way we can measure if this actively turns people off?  I know I've heard similar ideas with "r-" but to my knowledge no one has ever tried to quantify the effects.  As this is the first time I've heard of an issue with "Fix it", I would want to make sure the work involved would be justified (we have quite a number of high-priority UI and usability issues we're working on right now).

To be clear, I'm not saying you are wrong, but we have to balance this with the other 500+ bugs filed against MozReview (in addition to larger feature work).

Comment 10

[Eric Rahm [:erahm]]

(In reply to Mark Côté [:mcote] from comment #9)
> Right, we get that.  Unfortunately this is pretty ingrained in Review Board
> and would involve a nontrivial amount of work to change.  Is there some way
> we can measure if this actively turns people off?  I know I've heard similar
> ideas with "r-" but to my knowledge no one has ever tried to quantify the
> effects.  As this is the first time I've heard of an issue with "Fix it", I
> would want to make sure the work involved would be justified (we have quite
> a number of high-priority UI and usability issues we're working on right
> now).
> 
> To be clear, I'm not saying you are wrong, but we have to balance this with
> the other 500+ bugs filed against MozReview (in addition to larger feature
> work).

I don't know the best way for you to prioritize this, perhaps ask in a dev-platform post or maybe ask members of our community focused on bringing in a new users? It's possible you already did, but it was done outside of the scope of this bug so I don't know.

The existing patch indicates it's not a heavy burden to change (a one phrase change if you will), I suppose the real work is getting a reasonable result out of any ensuing bikeshedding.

Comment 11

[Mark Côté [:mcote]]

I know both mhoye and jdm work a lot with new contributors.  Do either of you have opinions on this?  I'm not crazy about "Feedback" because (a) it's not a clear instruction as to the next step like "Fix it" is, and (b) it clashes with the Feedback flag (which we will most likely support in MozReview at some point).  I think it's arguable if "Fix it" is aggressive, but maybe you've heard some feedback and/or have other ideas.

Comment 12

[Mike Hoye [:mhoye]]

I'm strongly opposed to talking to people who are volunteering their time and effort in the imperative. 

What to do about the patch should be in the comments, as I understand it, and given that we're space-limited to... what, eight or nine characters? Ten maybe? Then "Feedback" seems like a better choice, but if we've got the space "Revision Needed" would also be good. 

How much space do we actually have here?

Comment 13

[Mark Côté [:mcote]]

(In reply to Mike Hoye [:mhoye] from comment #12)
> What to do about the patch should be in the comments, as I understand it,
> and given that we're space-limited to... what, eight or nine characters? Ten
> maybe? Then "Feedback" seems like a better choice, but if we've got the
> space "Revision Needed" would also be good. 
> 
> How much space do we actually have here?

I think it's pretty flexible; there's usually a lot of space underneath.

I still think "Feedback" would cause more confusion than its worth, given the feedback flag in BMO.  "Revision needed" isn't bad, though not great... but I like that general sentiment.

Comment 14

[Josh Matthews [:jdm]]

We could follow github's lead and phrase it as "Requested changes".

Comment 15

[Mark Côté [:mcote]]

Reopening this since there has been good discussion.

Comment 16

[BMO Automation]

MozReview is now obsolete. Please use Phabricator instead. Closing this bug.