[Bug]: Delayed rejected version is auto approved after removing NHR flag

[ralli007]

What happened?

For the following add-on https://reviewers.addons.mozilla.org/en-US/reviewers/review-unlisted/2731905 I manually approved version 2.51.4 after finishing code review, but forgot to remove the NHR flag for version 2.51.3 and the add-on was still showing up in the manual queue. So I came back to it, removed the NHR flag for version 2.51.3, which was already delayed rejected, and as a result 2.51.3 got auto approved.

What did you expect to happen?

Version 2.51.3 should have remained delayed rejected. Removing the NHR flag should have removed the add-on from the manual queue.

Is there an existing issue for this?

• [X] I have searched the existing issues

┆Issue is synchronized with this Jira Task

[diox]

We never considered pending rejections would be used for non-approved versions - initially it was not meant for that. So it's currently working by design, pending rejection doesn't block auto-approval, but it would still happen eventually.

This is not a problem for permanent pre-reviewed add-ons, but I can see how it can be confusing for others...

[diox]

We agreed in triage meeting we should prevent this in auto-approve code (it checks a bunch of different conditions, it can be one of them)

[diox]

STR:

• Submit a new add-on, selecting "On your own" as the distribution method (fastest way to get the "Reject multiple versions" action to show up in reviewer tools - it wouldn't show up for the initial listed version of an add-on - it would also have to wait 24h/a content review to be auto-approved in the later step below if it was listed)
• In reviewer tools unlisted review page for that add-on, select "Reject multiple versions", and make sure to choose "Delayed rejection". You'll need ReviewActionReason etc set up properly, since you need to select a reason when rejecting)
• Click Enable Unlisted Auto-Approval Until Next Manual Approval button
• Leave the review page (so you don't get a lock on the add-on)
• Let auto-approve cron run (locally, use manage.py auto_approve)
• Go back to the unlisted review page, the version should not have been auto-approved, and there should be a message next to the version indicating that's because it's delayed rejected.
• Bonus: clear the pending rejection though the corresponding reviewer action, leave the review page, wait for/trigger auto-approval again and go back to the review page: the version should have been auto-approved.

[KevinMind]

Question:

If there are lower, unapproved versions, and a higher version which is approved, as in this case; Is there ever a use case for eventually approving the lower version?

Could this ever happen realistically? If not, why not go a step further and immediately reject non approved versions lower than the last approved version? Should we be spending time reviewing, considering a lower version?

[diox]

On the listed channel if there is a version awaiting review and the developer submits a new one, the old one will be disabled, so only the latest one will be awaiting review. However, for unlisted, all versions are considered independent, as we don't control updates for unlisted (developer is free to do whatever they want and keep all versions completely separate).

Note that in any case, sometimes a human will review old versions anyway: reviewers might want to reject the latest version but keep older ones intact - those would typically be already auto-approved though.

[KevinMind]

reviewers might want to reject the latest version but keep older ones intact - those would typically be already auto-approved though.

No I get it. But IF they decide to approve version X then you could make the argument versions X-n could be disabled. I would guess the most efficient way to process a queue of multiple unapproved versions would be to look at the latest one and work backwards until an approv-able one is found, at least for listed.

for unlisted, all versions are considered independent, as we don't control updates for unlisted (developer is free to do whatever they want and keep all versions completely separate).

This is interesting. I wonder when was the last time we looked at how used this feature is. Adds considerable complexity. Would there be a way to determine if this is a heavily used feature for unlisted versions.? I'm not sure (from a product perspective) it really makes sense that Mozilla owns the burden of reviewing versions where "developer is free to do whatever they want". Especially when we are looking for every opportunity to free up resources on reviews.

Another question: Is the approach we take standard in the market? Is Mozilla going above and beyond in this level of review support or is everyone else also doing it this way?

I don't expect answers from you here now. I will share this internally and see if there is more feedback.

[KevinMind]

Enable Unlisted Auto-Approval Until Next Manual Approval

Are you talking about this? I want to enable "until" next manual review or "before" next manual review?

[KevinMind]

Could not verify the bonus condition.

Bonus: clear the pending rejection though the corresponding reviewer action, leave the review page, wait for/trigger auto-approval again and go back to the review page: the version should have been auto-approved.

Stuck in awaiting review.

[diox]

If you look at the reasons, it says:

• "Has auto-approval disabled/delayed flag set". You need to hit the button in your screenshot to "Enable" (this is confusing wording I know) - that's because when a reviewer rejects something, we automatically set that flag that prevents auto-approval until a version is manually approved. So you need to flip that flag to get back to auto-approval.
• "Is locked by a reviewer". You still had the review page opened when auto-approval ran. Close that tab or navigate away from it when running manage.py auto_approve.

[KevinMind]

Tried following those steps. Got new reason: