1248996 - Commit table still shows r? despite a Ship It from the reviewer

Status: RESOLVED INVALID

(MozReview Graveyard :: General, defect)

Description

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

See https://reviewboard.mozilla.org/r/35261/

jdm gave a Ship It, but the commit still has "r?" set on it.

Comment 1

[Mark Côté [:mcote]]

My guess is that jdm has never pushed anything to MozReview via SSH, so we don't have his LDAP account associated with his MozReview account and ergo no level information, which in turns means that MozReview can't verify that this was an L3 user who gave the r+, which is why it has remained as r?.  Unfortunately with the current state of auth (in particular, BMO vs LDAP) there's not much we can do here aside from getting jdm to do a trivial push of something up to reviewboard-hg.

See also bug 1245577.

Comment 2

[Mark Côté [:mcote]]

Although this is admittedly not tremendously obvious, even after bug 1245577 lands, this is, for better or worse, the intended behaviour, so I'm closing this out.  Follow along in the aforementioned bug for how we'll make this situation cleaner.

Also see bug 1249149, which will make it possible to associate your LDAP account without having to submit a commit for review.

Comment 3

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

(In reply to Mark Côté [:mcote] from comment #2)
> Also see bug 1249149, which will make it possible to associate your LDAP
> account without having to submit a commit for review.

http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/install.html#manually-associating-your-ldap-account-with-mozreview

^-- instructions for how to do this

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This is not acceptable. This situation will be very likely when a contributor posts a patch using reviewboard. This happened to me as a reviewer in bug 1163862. A patch from a new contributor is precisely the situation where we want to act quickly and avoid losing/forgetting the patch (and the contributor).

If we can't fix this directly then we need a way to flag the reviewer that their r+ didn't actually "take" and that they need to take additional action.

Comment 5

[Mark Côté [:mcote]]

(In reply to Bill McCloskey (:billm) from comment #4)
> This is not acceptable. This situation will be very likely when a
> contributor posts a patch using reviewboard. This happened to me as a
> reviewer in bug 1163862. A patch from a new contributor is precisely the
> situation where we want to act quickly and avoid losing/forgetting the patch
> (and the contributor).
> 
> If we can't fix this directly then we need a way to flag the reviewer that
> their r+ didn't actually "take" and that they need to take additional action.

First let me say that I agree that the situation here is problematic; I probably should have duped this instead of resolving it as WORKSFORME.  In addition, I just filed bug 1251505 that will display a warning to reviewers if they have not associated their LDAP accounts.  Unfortunately one way or another user action is required because we (Mozilla at large) have no means to associate BMO accounts with LDAP accounts, hence the command to do so based on the user's private key.  I hope to have this and bug 1245577 resolved before too long.

I want to make sure this alleviates your concern because there may be some misunderstanding here.  The problem is not really that the r+ didn't "take"; it's still there in BMO if you were, say, to use checkin-needed.  The problem is that (a) the lack of knowledge of SCM level prevents the use of MozReview's Autoland feature, which needs to verify the reviewer's authority and (b) we misuse the term "r+" when we really mean "can be Autolanded".

Please let me know if there is still some confusion here.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

What happened in this case is that the patch author had updated a patch and wanted me to re-review before landing. In ReviewBoard the patch showed as r? so he was waiting on me to r+ it. I was never notified of a new review request because, in Bugzilla's view, the patch had an r+.

Eventually someone asked me about the bug and I asked the author why it hadn't landed yet. But it would have been easy to miss this.

Comment 7

[:glob ✱]

i had to figure out why this was happening so i thought it would be useful to document the current logic.

the following must be satisfied for a review to show as r+:
- the review request must have been pushed (not uploaded manually via the website)
- the review request must be public (not a draft)
- the number of issues must not be zero
- if the patch author has scm level 3:
  - at least one ship-it from any user
- if the patch author does not have scm level 3:
  - the review request must have a ship-it from a user with scm level 3


the difference in behaviour between l3 patch authors and those without l3 was the cause for the confusion (eg. "why did their ship-it 'stick' on request A but not request B?").

Comment 8

[Steven MacLeod [:smacleod]]

(In reply to Byron Jones ‹:glob› from comment #7)
> i had to figure out why this was happening so i thought it would be useful
> to document the current logic.
> 
> the following must be satisfied for a review to show as r+:
> - the review request must have been pushed (not uploaded manually via the
> website)
> - the review request must be public (not a draft)
> - the number of issues must not be zero
must *be* zero


> - if the patch author has scm level 3:
>   - at least one ship-it from any user
> - if the patch author does not have scm level 3:
>   - the review request must have a ship-it from a user with scm level 3
> 
> 
> the difference in behaviour between l3 patch authors and those without l3
> was the cause for the confusion (eg. "why did their ship-it 'stick' on
> request A but not request B?").

This is kind of documented[1]:
> For the Autoland option to be enabled, the current user must have L3 access and the following must be true for every commit in the series:
> 
>    The commit has been reviewed by someone with L3 access, or
>    The commit has been submitted (pushed to MozReview) by someone with L3 access.

The tooltip on the status column also indicates *why* the current status isn't "r+" -  a big problem is the fact that we use "r+" here to mean something people aren't used to. Also, "status" is a pretty nebulous name, but a proposed fix is in Bug 1245577


[1] https://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/autoland.html#landing-commits

Comment 10

[Nathan Froyd [:froydnj]]

Just ran into this today:

https://reviewboard.mozilla.org/r/61642/

I was extremely confused about why I couldn't trigger a try push or autoland a patch, despite having given review and despite the docs seemingly telling me that I could.  The status not actually changing to r+ is also peculiar.

Comment 11

[BMO Automation]

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