1595047 - moz-phab submit should not block updating patches with busy/unavailable reviewers if that reviewer has already approved the patch

Status: RESOLVED FIXED

(Conduit :: moz-phab, defect, P2)

Description

[:Gijs (he/him)]

STR:

1. write patch, request review from X
2. get review from X with a nit or two
3. try to re-moz-phab submit with the nits fixed

ER:
not blocked, because it's already been approved

AR:
blocked. :-(

Comment 1

[:Gijs (he/him)]

FWIW, I saw this with https://phabricator.services.mozilla.com/D52002 and so the reviewers were blocking reviewers - unsure if that makes a difference.

Comment 2

[Piotr Zalewa [:zalun]]

What do you mean by "blocked"?

We can't check if the changes introduced with the patch are the same as requested by the reviewer.
Since the code is changed the status of the revision changes as well. The reviewer did not accept the changed revision.

I think one of the slutions would be to add a switch where the user would force Phabricator to leave the revision accepted.

Comment 3

[:Gijs (he/him)]

(In reply to Piotr Zalewa [:zalun] from comment #2)

What do you mean by "blocked"?

I mean the code doesn't leave my machine and end up on phabricator:

Unable to submit commits:

563342:1c5cb1eb1ebb Bug 1593487 - tighten up registerWindowActor's handling of nonsensical actor specifications and remove cruft, r?nika!,mconley!
- nika is not available until 2019-11-30 05:00

We can't check if the changes introduced with the patch are the same as requested by the reviewer.
Since the code is changed the status of the revision changes as well. The reviewer did not accept the changed revision.

I'm not asking for a change in behaviour in how "approved" is handled by phabricator/lando. I'm asking for a change in moz-phab, which is what's blocking the update here.

I'm saying, given that:

1. I can land revisions if they have previously been approved.
2. an updated revision does not re-appear in reviewers' review queues if previously approved.
3. after such updates, if I as a patch author do want another review, I can already explicitly ask for a re-review from the phabricator UI using "Request review" in the actions dropdown

moz-phab should not take the availability or otherwise of reviewers who have accepted the diff into account when updating that diff, because I am not asking for review.

I think one of the slutions would be to add a switch where the user would force Phabricator to leave the revision accepted.

I don't think this would help fix the issue as filed, nor in fact do I think it's a good idea. :-)

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

I just hit this now due to holiday. Note that the issue is overridable by --force, but I agree with Gijs that this is not desirable default behaviour.

Comment 5

[Riya]

Can I work on this issue?

Comment 6

[Zeid [:zeid]]

Hi Riya -- sure thing. Let me know if you have any questions about this.

Comment 7

[Riya]

I have delved into the codebase, but I am not sure where the repos for moz-phab is located. Can you please help me out?
Thanks!

Comment 8

[Zeid [:zeid]]

Riya -- the repo is hosted on GitHub, and you can find some further instructions for contributing here: https://github.com/mozilla-conduit/review#development. Note that you can submit revisions via moz-phab for review on Phabricator.

Comment 9

[Riya]

Attachment: Bug 1595047 - moz-phab submit should not block updating patches if reviewer has already approved. r?zeid,Gijs

Comment 11

[Connor Sheehan [:sheehan]]

In bug 1706667 we added a hint about using -f to override this check, which is a reasonable enough workaround to not call this an S2 bug.

We should be able to check if the reviewer who is away has already given an r+ by checking the reviewers metadata on the revision, and then skip the warning/error if they have approved.

Comment 12

[Mathew Hodson]

Attachment: submit: move repo independent checks to one place (Bug 1595047) r=sheehan

Comment 13

[Mathew Hodson]

Attachment: submit: fix inaccurate "--no-wip" help text (Bug 1595047) r=sheehan

Comment 14

[Mathew Hodson]

Attachment: submit: avoid invalid reviewer error for existing revision (Bug 1595047) r=sheehan

If a revision already has reviewers, then don't check the reviewers
in the commit message. They won't be used anyway.

Comment 15

[Mathew Hodson]

I updated the patches based on your suggestions. This is ready to review now.

Comment 16

[Connor Sheehan [:sheehan]]

(In reply to Mathew Hodson from comment #15)

I updated the patches based on your suggestions. This is ready to review now.

Looks great, I will land and release a new version tomorrow. :)

Comment 17

[BMO Github Automation]

Authored by https://github.com/mathewhodson
https://github.com/mozilla-conduit/review/commit/ced85c38f9025532a83d48ea9b6a1dac143fe696
[main] submit: fix inaccurate "--no-wip" help text (Bug 1595047) r=sheehan

Authored by https://github.com/mathewhodson
https://github.com/mozilla-conduit/review/commit/9dc6ccb8875f396f91e03f9d52cfb8e426de6d21
[main] submit: move repo independent checks to one place (Bug 1595047) r=sheehan

Authored by https://github.com/mathewhodson
https://github.com/mozilla-conduit/review/commit/04473e861b9548387b21115d9646d22a92fb36fe
[main] submit: avoid invalid reviewer error for existing revision (Bug 1595047) r=sheehan

Comment 18

[Mathew Hodson]

Attachment: tests: avoid mocking logger in integration tests (Bug 1595047) r=sheehan

Comment 19

[BMO Github Automation]

Authored by https://github.com/mathewhodson
https://github.com/mozilla-conduit/review/commit/b949168661e6ff6e95bb9783caaaf8d2b02b05d4
[main] tests: avoid mocking logger in integration tests (Bug 1595047) r=sheehan