1245577 - Make meaning of "Status" column more clear

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P1)

Description

[Mark Côté [:mcote]]

The fact that we use r? and r+ in the Status column is confusing because it's overloading the meaning of these flags.  In Bugzilla they are used to reflect a single review or review request; in MozReview, they are determined by the state of the reviews, namely, if a user with L3 has given a review.

What this column is really about is a suggestion of whether the commit is ready to land or not (although it is really up to the author to make that call, as per the long thread on dev.platform).  I suggest changing the column name to "Ready to Land" and using a green checkmark or something like that instead of r+.  We can probably just leave it blank if otherwise.

Also, while we're at it, bz thinks we should only mark a commit as ready to land if it has an L3 review *and* no other outstanding review requests.  If the event that an author has asked review from several people just to see who gets there first, the author can just remove the extra review requests after getting a review.

Comment 1

[Mark Côté [:mcote]]

A couple more points: we should keep the !<issue count> status that we have now when there are open issues.  bz raises a further point: if there were issues opened by the reviewer, but the reviewer also gave an r+, then if any of these issues were fixed (versus dropped), then really the commit is not ready to land; it'll need a new revision pushed.  So we should indicate this as well, some sort of status icon (maybe just a ! with no issues) with a tooltip reading something like "issues fixed; new patch needed for landing".

Comment 2

[Mark Côté [:mcote]]

Another question: if the reviewer doesn't have L3, but the author does, is it landable?  Is this typically done with Bugzilla patches now?

Comment 3

[Gregory Szorc [:gps]]

(In reply to Mark Côté [:mcote] from comment #2)
> Another question: if the reviewer doesn't have L3, but the author does, is
> it landable?  Is this typically done with Bugzilla patches now?

There is no formal policy with Bugzilla since the concept of L3 is not integrated with Bugzilla and the repo levels can be undermined with checkin-needed.

Another concern that was raised in the giant dev-platform thread was the author expressing "intent to land" versus "getting review." Right now, MozReview assumes that if the Ship Its and issue counts are in the right state then we are "ready to land." However, this assumes the author is done with the patch! What's missing is an explicit statement from the author that yes, things are ready to land once appropriate review is obtained.

I liked mconnor's suggestion of a "land?" or "l?" flag in the commit message to denote "ready to land" versus normal "review requested." This requires a minor change in workflow. But we either do this or prompt the user during review submission. I like things in the commit message because that metadata is strongly attached to the commit (and is harder to get out of sync) and because it doesn't add any additional steps to review submission. Regarding that second point, I envision we'll someday present a text editor to submitters that gives them an opportunity to e.g. add change comments. We could certainly add a "ready to land" flag there.

Comment 4

[Mark Côté [:mcote]]

Heard you were looking for UI stuff. :) Ping me if you have questions (I'll answer comment 3 at some point soon).

Comment 6

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

Alright, let me make sure I have the TL;DR of this bug:

1) We're changing the last column of the commits table so that the header is "Ready to Land"
2) When there are issues opened against a review request, continue to show the count of those issues in that column
3) When the issues are closed, but a new revision has not been posted, we need to show a state that means "issues fixed, needs new revision"
4) When an L3 reviewer[1] gives a Ship It, then the commit is ready to land, unless there are issues still open. Once those issues are closed, and a new revision is posted, then the commit is ready to land.

[1]: I think this is the fuzziest part. Do we want to allow #4 to work if the submitter is L3 as well?

Comment 7

[Mark Côté [:mcote]]

Yes, that's all correct.  And yes, I think it should be either L3 author or L3 reviewer (or both).  I'm told that sometimes L3 people will give reviews to non-L3 people as a way of training them.

To answer gps in comment 3, this is a good idea, but I think it's out of scope here and should be a separate bug.  Right now we have a UI that is meaningless to many people; we should fix that to make it clearer, and then add more stuff later.  That said, perhaps "Ready to Land" is misleading as well... but I'm not sure what a better title would be.  Open to suggestions.

Comment 8

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

Attachment: MozReview Request: mozreview: [WIP] Change Status column to Cleared for Landing (bug 1245577); r?mdoglio

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

Comment 9

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/41257/#review39537

Screenshots of any changes to the table would be awesome :)

::: pylib/mozreview/mozreview/hooks.py:93
(Diff revision 1)
>  
>                  return False, 'Commit %s is not approved.' % commit_id
>  
>          return True
>  
>      def is_approved_child(self, review_request):

Please request me as a reviewer for any changes to the approval logic

::: pylib/mozreview/mozreview/hooks.py:109
(Diff revision 1)
> +        # Comment.objects.filter(filediff__in=diffset.filediffs.all())
> +        # Comment.objects.filter(issue_status=Comment.RESOLVED & (Q(filediff__in=diffset.filediffs.all()) | Q(interfilediff__in=diffset.filediffs.all())))

I can see this getting confusing since comments can be on multiple diffs / interdiffs, with the actual review being published after a new diff is posted etc.

I think it might be better to stick with the current behaviour, of trusting the user and letting them click land if the issues have been fixed without a revision being posted - it keeps the logic much simpler.

Where I think sticking this logic makes sense though is in a warning on the landing box "This diff has not been updated since issues were fixed" etc. Sort of like how we warn when someone is landing with a carried forward r+.

::: pylib/mozreview/mozreview/templates/mozreview/commits.html:32
(Diff revision 1)
>        <th class="reviews">{% trans "Reviews" %}</th>
>        {% comment "TODO: show this column when the commit author will be available" %}
>        <th class="submitter">{% trans "Submitter" %}</th>
>        {% endcomment %}
>        <th class="reviewers">{% trans "Reviewers" %}</th>
> -      <th class="status">{% trans "Status" %}</th>
> +      <th class="status">{% trans "Cleared for Landing" %}</th>

This is really going to need a screenshot.

Comment 10

[Steven MacLeod [:smacleod]]

:mconley is very busy with important work so I'm going to steal this and push it through.

I do however think the proposed changes to approval logic are out of scope for this bug - I plan to simply fix how we currently present the information "status" vs "ready to land" etc.

Comment 11

[Steven MacLeod [:smacleod]]

The solution to this has morphed a little bit. The idea now is to call the column "Landable" ("ready to land" takes up far too much space), and provide a help indicator icon in the column header (a circled "?" or something). This icon will provide a tooltip explaining the meaning of the column, and suggest looking at the tooltips for a specific commits "landable" icon to provide information about that commits status.

The other piece is to change the overloaded "r?", "r+" indicators in this column to a simple "no", "yes". The other more obvious states such as having open issues should remain.

After all that is said and done we need to update the status indicator tooltips to provide more information making the reason for the status obvious. Our current tooltips do explain things, but leave a lot to be desired especially in the more confusing cases.

I've passed off my current code which implemented the help icon and tooltip support to :davidwalsh, who will polish things up and take this across the finish line.

Comment 12

[David Walsh :davidwalsh]

Attachment: MozReview: Implement tooltip for landable column cells (Bug 1245577).

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

Comment 13

[David Walsh :davidwalsh]

Attachment: MozReview: Implement tooltip for landable column cells (Bug 1245577).

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

Comment 14

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/56616/#review53236

I made the entire cell a hover target simply because it's easier for the user to hit.  We'll be able to easily change colors if necessary.  Let me know what you think.

Comment 15

[:glob ✱]

https://reviewboard.mozilla.org/r/56618/#review58056

remove this WIP commit please - you'd want to squash the two commits into one.

::: pylib/mozreview/mozreview/templates/mozreview/commits.html:34
(Diff revision 1)
>        <th class="submitter">{% trans "Submitter" %}</th>
>        {% endcomment %}
>        <th class="reviewers">{% trans "Reviewers" %}</th>
> -      <th class="status">{% trans "Status" %}</th>
> +      <th class="status">
> +        {% trans "Landable" %}
> +        <div class="help-icon help-tooltip" title="Here is our tooltip!"><p title="">?</p></div>

i suspect we want something more informative than "Here is our tooltip!" here.

Comment 16

[:glob ✱]

Comment on attachment 8758377 [details]
MozReview: Implement tooltip for landable column cells (Bug 1245577).

https://reviewboard.mozilla.org/r/56620/#review58052

i really like how this looks!

::: pylib/mozreview/mozreview/static/mozreview/css/review.less:158
(Diff revision 1)
> +    position: relative;
> +
> +    &:hover .review-tooltip {
> +        display: block;
> +    }

the indentation on all your changes to this file is wrong; please use 2 spaces, not 4 or 8.

::: pylib/mozreview/mozreview/static/mozreview/js/review.js:62
(Diff revision 1)
> +     var $tip = $('<div></div>').attr('class', 'review-tooltip').insertAfter($element);
> +     var $tipText = $('<div></div>').attr('class', 'review-tooltip-text').text(text).appendTo($tip);

nit: no need to create $tip and $tipText here, as they are not used

Comment 17

[:glob ✱]

https://reviewboard.mozilla.org/r/56618/#review58062

::: pylib/mozreview/mozreview/templates/mozreview/commits.html:34
(Diff revision 1)
>        <th class="submitter">{% trans "Submitter" %}</th>
>        {% endcomment %}
>        <th class="reviewers">{% trans "Reviewers" %}</th>
> -      <th class="status">{% trans "Status" %}</th>
> +      <th class="status">
> +        {% trans "Landable" %}
> +        <div class="help-icon help-tooltip" title="Here is our tooltip!"><p title="">?</p></div>

the text of this tooltip is bold (because it's inheriting from th).  it should have a normal weight to match the other tooltips.

Comment 18

[David Walsh :davidwalsh]

Comment on attachment 8758377 [details]
MozReview: Implement tooltip for landable column cells (Bug 1245577).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56620/diff/1-2/

Comment 20

[David Walsh :davidwalsh]

Comment on attachment 8758376 [details]
MozReview: Implement tooltip for landable column cells (Bug 1245577).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56618/diff/1-2/

Comment 21

[David Walsh :davidwalsh]

Comment on attachment 8758376 [details]
MozReview: Implement tooltip for landable column cells (Bug 1245577).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56618/diff/2-3/

Comment 22

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/56616/#review60402

I think it's better to post screenshots as part of a commit, rather than the summary, that way it'll show open issues in the commit table.

Comment 23

[David Walsh :davidwalsh]

Comment on attachment 8758376 [details]
MozReview: Implement tooltip for landable column cells (Bug 1245577).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56618/diff/3-4/

Comment 24

[David Walsh :davidwalsh]

Comment on attachment 8758376 [details]
MozReview: Implement tooltip for landable column cells (Bug 1245577).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56618/diff/4-5/

Comment 25

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/56616/#review60402

Sorry, will do that from now on.

Comment 26

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/56616/#review60402

Comment 27

[:glob ✱]

Comment on attachment 8758376 [details]
MozReview: Implement tooltip for landable column cells (Bug 1245577).

https://reviewboard.mozilla.org/r/56618/#review60534

these changes read well, but i want to run them locally before i give an r+.

::: pylib/mozreview/mozreview/static/mozreview/css/commits.less:17
(Diff revision 5)
>  @issue-opened-bg: #fff4B0;
>  @issue-opened-border-color: darken(@issue-opened-bg, 40%);
>  @issue-opened-link-color: darken(@issue-opened-bg, 60%);
>  
>  /*******************************************************/
>  

please rebase these changes on top of tip and resubmit; there's merge conflicts that need to be resolved.

Comment 28

[David Walsh :davidwalsh]

Comment on attachment 8758376 [details]
MozReview: Implement tooltip for landable column cells (Bug 1245577).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56618/diff/5-6/

Comment 29

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/56618/#review60534

> please rebase these changes on top of tip and resubmit; there's merge conflicts that need to be resolved.

Rebased, good to go!

Comment 30

[:glob ✱]

Comment on attachment 8758376 [details]
MozReview: Implement tooltip for landable column cells (Bug 1245577).

https://reviewboard.mozilla.org/r/56618/#review61902

this looks good, except the font weight in the header's tooltip still needs fixing.  will be a quick r+ once that's sorted.

::: pylib/mozreview/mozreview/templates/mozreview/commits.html:34
(Diff revision 6)
>        <th class="submitter">{% trans "Submitter" %}</th>
>        {% endcomment %}
>        <th class="reviewers">{% trans "Reviewers" %}</th>
> -      <th class="status">{% trans "Status" %}</th>
> +      <th class="status">
> +        {% trans "Landable" %}
> +        <div class="help-icon help-tooltip" title="{% trans "Hover over each commit's status to view landable status" %}"><span>?</span></div>

the text of this tooltip is still bold (because it's inheriting from th).  it should have a normal weight to match the other tooltips.

Comment 31

[David Walsh :davidwalsh]

Comment on attachment 8758376 [details]
MozReview: Implement tooltip for landable column cells (Bug 1245577).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56618/diff/6-7/

Comment 32

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/56618/#review61902

> the text of this tooltip is still bold (because it's inheriting from th).  it should have a normal weight to match the other tooltips.

Fixed!  Sorry!

Comment 33

[:glob ✱]

Comment on attachment 8758376 [details]
MozReview: Implement tooltip for landable column cells (Bug 1245577).

https://reviewboard.mozilla.org/r/56618/#review62184

yay!

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/b2434cbfec7c
MozReview: Implement tooltip for landable column cells . r=glob