1467833 - There should be a more visible warning for unresolved comments

Status: RESOLVED WONTFIX

(Conduit :: Phabricator, enhancement)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I just reviewed a patch, I left some comments, but since those were mostly NITS I still approved the patch for landing. That's quite usual in our workflow when working among collegues knowing the code, we don't go through the request of a further review.

The problem is that, it looks like once the diff is approved, one can just go to Lando and land it, not noticing the comments, that are in a small section under "Reviewer accepted this revision" and "Reviewer added inline comments."

It would be great to have a far more visible "There are pending and unresolved comments" warning, so that one notices it before landing.

Then, after submitting a new revision, one can't just mark a comment as resolved, but must reply to the comment to mark it as resolved?

Comment 1

[Mark Banner (:standard8)]

As submitter of the patch in this case, I can definitely confirm that it looked like Lando would let me land with no warning of the outstanding comments.

You really have to be aware of the content of the approval email, or read the history. There's no "stop! did you really mean to do this?".

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

An interesting difference with bug 1288136 is that mozreviews shows pending issues quite clearly, even if a patch is approved.

Comment 3

[Steven MacLeod [:smacleod]]

Many users don't bother with  marking comments as "Done", and Phabricator doesn't treat them nearly as seriously as Review Board issues (they don't factor into revision acceptance and you can't choose to open one or not etc.). Warning / blocking landing based on comments not marked as Done would probably do more harm than good.

Authors should be reading the contents of the reviews they receive before landing. If you don't believe the code author will do this you should probably "Request Changes" and re-review.

Comment 4

[Steven MacLeod [:smacleod]]

Just a quote from upstream Phabricator to clarify this a bit:
> The current design of "Done" is trying to make it an optional feature authors can use to help manage large amounts of feedback, not a required feature. We don't teach users to check the box, we don't prompt them if they don't, we don't suggest they do

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Steven MacLeod [:smacleod] from comment #3)
> Warning / blocking landing based on comments not marked as Done would
> probably do more harm than good.

Couldn't we just make it a bit more evident there are comments in the UI, maybe even through some theme change? Without warnings or alerts of any sort, just something more visible than a small grey box among grey boxes.

Comment 6

[Steven MacLeod [:smacleod]]

(In reply to Marco Bonardo [::mak] from comment #5)
> Couldn't we just make it a bit more evident there are comments in the UI,
> maybe even through some theme change? Without warnings or alerts of any
> sort, just something more visible than a small grey box among grey boxes.

Are you suggesting in Phabricator or Lando? This still feels like more of an adjustment to the new system problem than something worth prioritizing.

Comment 7

[Mark Banner (:standard8)]

I don't think it matters which, but it needs something. I almost landed something earlier because I hadn't realised the reviewer had commented on the request.

I'd seen the bug mail come in, but not sure I looked at the body. I'd gone to the bug & phabricator page and it looked all green, I was just about to land, and I just scrolled to double-check the diff, and then noticed the comment.

On my screen without scrolling, I see everything down to just before the history section, that top section also gives me the link to Lando, hence I can look at it, see everything is "fine" and just hit through.

There's absolutely nothing in the top section that gives you any hints about the fact that comments have been made.

Whilst I understand that Phabricator may have less emphasis on comments, if there's a done/not done status, you might have expected that to be reflected in the top status section...

Comment 8

[:glob ✱]

maybe there's value in a simple indicator in phabricator (perhaps next to the lando link) which shows the number of "not-done" comments, however that might be less than useful until bug 1469136 is addressed.