1396602 - Prompt user if they are trying to land a old diff id when there is a more recent one on Phabricator.

Status: RESOLVED FIXED

(Conduit :: Lando, defect)

Description

[Piotr Zalewa [:zalun]]

AC:

- POST to the /landings endpoint with the diff id that was given in the revision information when the page was loaded (which was the most recent diff id on page load).
- the POST will fail if the diff id given is not the most recent one anymore (as defined by https://trello.com/c/8N9BkSr2.
- Prompt the user about this discrepancy: inform them to re-review on phabricator, and give them an option to land the revision if they are cool with it.

Example case:
- I ask glob to land my revision.
- He loads the page, when diff id 10 was the most recent one.
- Oops! Trees are closed, glob says he will land first thing tomorrow morning. He goes to bed. He does not close the web page.
- I happen to find a bug with my patch so I fix it really quick. The most recent diff id is now 11.
- Glob wakes up and tries to land my revision. Lando UI passes diff id 10 because that is what was the most recent when the page was loaded.
- Lando API aborts the request because diff id 10 is no longer the most recent.
- Lando UI sees this, prompts glob about it, askes him to re-review, and gives him a button to continue the landing with diff id 11 if he is ok with it.

Comment 1

[:glob ✱]

(In reply to Piotr Zalewa [:zalun] from comment #0)
> - Oops! Trees are closed, glob says he will land first thing tomorrow
> morning. He goes to bed. He does not close the web page.

this isn't how lando should work.  if the trees are closed it *must* still allow submission to autoland-transplant.
autoland will queue the landing request and automatically land it when the trees are open.

Comment 2

[Steven MacLeod [:smacleod]]

(In reply to Piotr Zalewa [:zalun] from comment #0)
> ...
> - Lando API aborts the request because diff id 10 is no longer the most
> recent.
> - Lando UI sees this, prompts glob about it, askes him to re-review, and
> gives him a button to continue the landing with diff id 11 if he is ok with
> it.

I don't think we should just prompt to land the next diff, at least if we do we need to make sure the uploader of the new diff is the same user as the old. This also gets sketchy when it's not a well known contributor. I'd rather have the lander go back to verify the new diff is fine and then reclick the new autoland link.

Comment 3

[Piotr Zalewa [:zalun]]

(In reply to Steven MacLeod [:smacleod] from comment #2)
> (In reply to Piotr Zalewa [:zalun] from comment #0)
> > ...
> > - Lando API aborts the request because diff id 10 is no longer the most
> > recent.
> > - Lando UI sees this, prompts glob about it, askes him to re-review, and
> > gives him a button to continue the landing with diff id 11 if he is ok with
> > it.
> 
> I don't think we should just prompt to land the next diff, at least if we do
> we need to make sure the uploader of the new diff is the same user as the
> old. This also gets sketchy when it's not a well known contributor. I'd
> rather have the lander go back to verify the new diff is fine and then
> reclick the new autoland link.

Agreed.

In the database we will store the information of the failed Landing.
The Warning_Modal should have the link to load the Review information page.

We also will have an ability to confirm landing of the diff 10.
Lando API will allow to land an inactive diff if `force_inactive_diff` is set.

Comment 4

[Piotr Zalewa [:zalun]]

(In reply to Byron Jones ‹:glob› from comment #1)
> (In reply to Piotr Zalewa [:zalun] from comment #0)
> > - Oops! Trees are closed, glob says he will land first thing tomorrow
> > morning. He goes to bed. He does not close the web page.
> 
> this isn't how lando should work.  if the trees are closed it *must* still
> allow submission to autoland-transplant.
> autoland will queue the landing request and automatically land it when the
> trees are open.

Yes, the wording used was not right.
It should rather say "For whatever reason glob decided to land first thing tomorrow morning."

Comment 5

[Israel Madueme [:imadueme]]

This bug only tracks the API side of things, I will create a new bug and Trello card for the work needed on the UI side.

Comment 6

[Phabricator Automation]

Comment on attachment 8905414 [details]
Refuse to land an inactive revision unless forced.

Israel Madueme [:imadueme] has approved the revision.

https://phabricator.services.mozilla.com/D38#1446

Comment 7

[Phabricator Automation]

Comment on attachment 8905414 [details]
Refuse to land an inactive revision unless forced.

Israel Madueme [:imadueme] has been removed from the revision.

https://phabricator.services.mozilla.com/D38#2687

Comment 8

[Phabricator Automation]

Comment on attachment 8905414 [details]
Refuse to land an inactive revision unless forced.

Israel Madueme [:imadueme] has approved the revision.

https://phabricator.services.mozilla.com/D38#2798

Comment 9

[Phabricator Automation]

Comment on attachment 8905414 [details]
Refuse to land an inactive revision unless forced.

Israel Madueme [:imadueme] has requested changes to the revision.

https://phabricator.services.mozilla.com/D38#2917

Comment 10

[Phabricator Automation]

Comment on attachment 8905414 [details]
Refuse to land an inactive revision unless forced.

Israel Madueme [:imadueme] has approved the revision.

https://phabricator.services.mozilla.com/D38#3124

Comment 11

[Phabricator Automation]

Comment on attachment 8905414 [details]
Refuse to land an inactive revision unless forced.

Steven MacLeod [:smacleod] has approved the revision.

https://phabricator.services.mozilla.com/D38#3631