Open review page can block auto-approval indefinitely

[jvillalobos]

The Firefox Release Team ran into this problem recently, where a language pack wasn't being signed due to its review page being opened by a user with review permissions. This blocked release of a beta version of Firefox.

The intermediate state between submission and auto-approval is just an artifact of the way auto-approval was implemented, and we have ways to turn off auto-approval for individual add-ons. So, I see no reason for the review page lock to block auto-approval, other than prevent unexpected behavior if an action is taken on that review page based on its previous state. My suggestion in that case would be to show an error, refresh the page state, and ask the reviewer to try again. Not great, but it's not a common problem, AFAIK.

/cc @diox @wagnerand @kewisch

[diox]

The problem is that "state" can be hard to define. As a short-term solution, if just looking at status is enough, then we can implement that. But if we want something more granular it's going to require digging deeper in reviewer tools.

Worth noting that we talked about firing the auto-approve task right away at submission time instead of relying solely on the cron to do it at fixed intervals, but the intermediary state will likely always be present, because signing can't happen synchronously, so even if it's just for a split second, we'll probably always have the awaiting review state.

[wagnerand]

What if we ignored that "reviewer lock" just for language packs and kept it for extensions?

[diox]

I can do that yes.

[diox]

QA: reviewer locks (having the review page opened by a reviewer) should now be ignored by auto_approve for language packs. The rest should work as before.

[jvillalobos]

That fixes the pressing problem for language packs, but not the larger problem posed in this issue, so I'm reopening. It could still be a problem if a reviewer unknowingly keeps a page open and blocks auto-approval for an add-on for a long time.

[jvillalobos]

Another possible solution that occurred to me is that we show a warning message in the review page if you open it when the add-on is waiting for auto-approval. I suppose we could also disable the Save button in that case, to prevent accidental submission of a review while in that state.

[diox]

You can't detect that state though, because the reviewer can open the page before a new version is submitted. Really the underlying problem is difficult to detect and solve, we'd need a deep refactor of the reviewer tools to properly detect state changes, what version is passed, etc. At the moment it does not care, directly acting on the last version regardless of what was the state before (See #2446)

Is blocking auto-approval of regular add-ons a big issue? Have we ever had a complain about it? For langpacks, there is an expectation that they are almost immediately signed, everything is automated, so it makes sense. But for the rest, it may delay them for a short while, but it should be acceptable ?

[jvillalobos]

We have had complaints about it, yes. I believe we've had this issue with some of our own extensions, where a reviewer forgets they have the review page open. But, since it's certainly a smaller problem I think it's okay to drop it for now.

[AlexandraMoga]

Verified fixed on amo -dev with FF65, Win10x64

Reviewer lock is no longer blocking language pack auto-approvals. For all the other addons that are auto-approved (extensions and dictionaries) the reviewer lock still applies,