1182683 - Workflow for posting patches for feedback

Status: RESOLVED INVALID

(MozReview Graveyard :: General, defect)

Description

[Botond Ballo [:botond] [away until Oct 19]]

MozReview is a great replacement for posting patches as bugzilla attachments for review.

It would be great if it also supported the workflow of posting patches for feedback, rather than review.

Comment 1

[Gregory Szorc [:gps]]

I challenge the assertion that feedback needs to be distinguished from review. If a commit isn't ready for full review:

1) Indicate as such in the commit message (e.g. "FEEDBACK ONLY") or in a "self review" on MozReview
2) Don't land the commit

What value do you feel feedback adds? Please justify the added complexity, especially from the perspective of first time contributors who don't yet know what they are doing.

Comment 2

[Mark Côté [:mcote]]

I think gps has a good point here.  In my experience, feedback? requests in BMO can be divided into two categories:

1. Requesting review of an unfinished patch
2. Requesting a quick look at a finished patch, in addition to other full review? requests

1. can be accomplished in the manner gps suggests.  2. is harder, but I question its validity.  If someone flagged for f? saw something wrong, they should change their flag to a r-.  I don't think f+s are particularly useful, as at best they confer "I glanced at this and it seems okay".  In this case, they could just leave a review without a "ship it", but without raising any issues, and they'll just have their r? cancelled in BMO--effectively the same as an f+.  The determining factor remains the presence of one or more r+s from qualified reviewers and any issues raised in reviews.

I may be overlooking other uses of f+, but hopefully even those could be solved in other ways to keep the simplicity Review Board has by default.

There's an older bug opened about this topic; I'm going to dupe it to this one, since we've started a broader discussion on the topic here.

Comment 4

[Botond Ballo [:botond] [away until Oct 19]]

I mostly use feedback to ask someone a targeted question about a patch, e.g. "will the approach of doing X shown here work?" or "do you foresee any problems with doing Y?". The request being "feedback" rather than "review" makes it clear that they needn't look at other parts of the patch, and that they needn't evaluate aspects like design and style (unless that's specifically what the targeted question is about).

Of course we can use "review" in place of "feedback" and write a comment to discriminate between the two, but by the same token we could abolish flags altogether and both request and grant reviews in prose in comments.

I view this as simply a "parity" feature request - allow doing the same things that can be done with the old system (attaching patch files), with the new system (MozReview).

Comment 5

[Mark Côté [:mcote]]

> Of course we can use "review" in place of "feedback" and write a comment to
> discriminate between the two, but by the same token we could abolish flags
> altogether and both request and grant reviews in prose in comments.

I disagree--the value of a r+/ship it is very high.  Not only is clear, unambiguous information important to communicate when granting review, but it will feed into a number of automated systems, particularly autoland.  The information relayed by f+ is much less important.

> I view this as simply a "parity" feature request - allow doing the same
> things that can be done with the old system (attaching patch files), with
> the new system (MozReview).

This is a good time to revisit the old system and figure out what is necessary and useful versus what could be accomplished in simpler ways (or even just thrown away).  I recognize that anything that falls into the latter category will take some adaptation, but in the long run it can lead to a system that is easier to learn, easier to use, easier to maintain, and easier to expand.

At the very least, I'd like to see if getting a proper, editable description field separate from the commit message (bug 1123139) would be sufficient.  It's going to land this quarter, so let's revisit this in a month or two (even if we agreed to implement a feedback flag, it wouldn't get done before then anyway).

Comment 7

[Mark Côté [:mcote]]

After some conversations in Mozlando, I'm now leaning toward actually implementing this, in some way, mainly because it seems inevitable that we'll have to support flags other than review, e.g. ui-review.  There may be a way we can support a feedback flag without copying the flag system in Bugzilla, though, which is not particularly intuitive for new users.  That's a big discussion though.

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

we either need to remove the feedback flag from bugzilla, or add support here for feedback.  When you go in and review a patch when asked for feedback in bugzilla- the flag is not cleared.  It seems that the argument here about the need for feedback? is unrelated to mozreview- either we remove it from our other systems, or we make mozreview with smoothly with workflows defined in other systems.

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

Got hit by it multiple times.

The very minimum I'd expect is for MR not to *remove* the f? flag when I update the patch that has no f=/r= field set.

STR:

1) Push to MR a patch without review/feedback fields.
2) Manually mark someone for f? in Bugzilla
3) Push an updated MR patch

Current result:
MR removes the f? flag

Expected result:
Primum non nocere

Comment 11

[BMO Automation]

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