addons
addons copied to clipboard
mozilla
[Task]: Setting Needs Human Review flag during content review should automatically disable auto-approval in listed channel
Description
As a reviewer,
- when I mark a version for a full review by setting the Needs Human Review (NHR) flag
- during the content review of a first listed version submission of the add-on
- I almost always want the auto-approval to pause on the listed channel until the add-on has been manually reviewed
Outcome: I'd like the action to set NHR flag on a version to automatically disable auto-approval on listed channel on an add-on with newly submitted listed version awaiting content review.
Motivation:
- Reviewers flag an add-on for manual review when they suspect non-compliance in code during content review.
- Since there is already a suspicion, preventing the add-on from reaching AMO before the manual review is preferred.
- Typically [ideally], reviewers disable the auto-approval on the listed channel manually.
- But there have been instances when they forgot to do that. This resulted in the version (And any newer submissions) getting auto-approved and later having to be blocked, having already reached AMO audience.
- This can be prevented by automatically disabling auto-approval for the listed channels in case the reviewer sets NHR during content review of first listed version submission.
Tagging @wagnerand if there are any comments (and if there's any reason why this shouldn't be done)
Acceptance Criteria
### Acceptance Criteria
- [ ] When the NHR (needs human review) flag is set during content review of an add-on with first listed version submission, auto-approval should be disabled for the listed channel of the add-on
- [ ] The help text explicitly states that the add-on's auto-approval will be disabled, so that a reviewer is aware of this (side) effect
Checks
- [X] If the issue is ready to work on, I have removed the "needs:info" label.
┆Issue is synchronized with this Jira Task
I don't follow the motivation. Setting NHR is not an necessarily indication of potential noncompliance. If you have that suspicion, you can selectively disable auto-approval, but we shouldn't do this by default.
@wagnerand can you make a call on if what needs to be changed in the reviewer tools or close the issue if current behavior is what we want.
@eviljeff following offline agreement with Andreas, here's what we'd like to do here.
When the first listed version of a add-on is pending content review, and the reviewer decides to set Needs Human Review flag on it, we'd like the auto-approval of the listed channel to be disabled.~and the add-on to be moved out of the content review queue.~ (see https://github.com/mozilla/addons/issues/14889#issuecomment-2208672271)
This will mitigate the risk of a non-compliant version reaching more users than it has to.
An example scenario of where exactly this mitigation will help operations is:
- during a content review, a reviewer suspects that an add-on could be non-compliant and flags it for manual review
- the reviewer forgets to set the disable auto-approval flag
- between the time the version is flagged for manual review and the time when the manual review is done, more versions get submitted and auto-approved, potentially reaching AMO users
/cc @wagnerand
When the first listed version of a add-on is pending content review, and the reviewer decides to set Needs Human Review flag on it, we'd like the auto-approval of the listed channel to be disabled and the add-on to be moved out of the content review queue.
This is while the first pending version of the listed channel has auto-approval delayed for content-review.
and the add-on to be moved out of the content review queue.
We probably don't want to do this. Add-ons should only leave the queue for decisive actions and when explicitly dropping NHR.
and the add-on to be moved out of the content review queue.
We probably don't want to do this. Add-ons should only leave the queue for decisive actions and when explicitly dropping NHR.
I'll get rid of this requirement from here to keep this issue specific to an outcome, and file the quoted requirement as a separate issue given the finer details its definition will need.
auto-approval of the listed channel to be disabled
@abhn The title and acceptance criteria says all distribution channels; the latest comment says listed channel. Can you clarify and update?
When the first listed version of a add-on is pending content review
This is while the first pending version of the listed channel has auto-approval delayed for content-review.
I read this the first time as the review type was content_review, but now I'm not so sure what you mean?
@abhn The title and acceptance criteria says all distribution channels; the latest comment says listed channel. Can you clarify and update?
Done, thanks for pointing it out.
When the NHR (needs human review) flag is set during content review of an add-on with first listed version submission, auto-approval should be disabled for the listed channel of the add-on
The first listed version criterion was missed in the PR.
When the NHR (needs human review) flag is set during content review of an add-on with first listed version submission, auto-approval should be disabled for the listed channel of the add-on
The first listed version criterion was missed in the PR.
You'll have to be more specific - the PR closed the issue, IMO.
with first listed version submission
Content review is for the first listed submission so I'm missing some nuance here. Are you requesting that add-ons that submit multiple versions in quick succession, so the content review doesn't actually happen until the second, or later, version should be excluded from this?
Or is this about the edge-case when an add-on has the content review date cleared so it's up for another content review? (I figured this would also be a case where you'd also want to stop further auto-approvals?)
@wagnerand
From the testing instructions:
choose "set needs human review" action, choose a version, and submit with a comment navigate to the admin for that add-on (link in the right hand column) and scroll down to the add-on reviewer flags section to see Auto approval disabled until next approval set to yes
However, this should only happen if the content-review was caused by the initial (listed) submission for an add-on. Content-review can happen for other reasons as well, for example if the summary changed.
So following the conversation, I think I'll add my side and try to clarify it purely from a requirements perspective and we can weigh if it should've been spec'd or implemented differently.
During content reviews of add-ons with newly submitted listed channel version, reviewers flag the add-on's versions for a manual review if they deem it suspicious. It is currently a two step process (1). Set NHR (2). Disable auto-approval of listed channel
My motivation here is to combine (2) into (1), such that we a. Eliminate a common human error where the reviewer forgets to do (2) after (1) b. One fewer manual operational process to remember for the reviewer
Essentially, we'd not like to do (2) for an add-on that has already gone through a content review in the past (hence, the requirement said we'd like to do it for only the first listed version submission).
However, to answer Andrew's question in this thread
... the content review doesn't actually happen until the second, or later, version should be excluded from this?
I think in cases where the developer submits many versions in quick succession, we'd still like to do (2) as it is still the first content review for the add-on.
I figured this would also be a case where you'd also want to stop further auto-approvals?
This should be weighted for any potential cons. As far as reviewers' requirements go, I'd say this isn't a common scenario the we run into (that is, a subsequent content review prompting the reviewer to flag the add-on for manual review and needing to disable auto-approval).
Why would we want to disable auto-approval if the submission has already been auto-approved? Do we assume that a potential second version would be significantly more malicious than a first (auto-approved) one?
Also, why would we want to disable auto-approval permanently, rather than just until the next human approval?
@eviljeff please back out https://github.com/mozilla/addons-server/pull/22443 (https://github.com/mozilla/addons-server/commit/15e4bd0b8ee64da8384a5f15b281aa1af734e12c) while the requirements are re-evaluated and redefined. Thank you.
We'll revisit this in the future with a better, more detailed specification from ops (not high priority). As it stands we are not going forward with the approach highlighted here so I'm closing this ticket.
