[Task]: Change due date logic for items in human review queue

[abyrne-moz]

Description

Do not inherit the due date for items in the human review queue if the previous versions have only Reported for abuse within the add-on or Escalated for an abuse report, via Cinder as the needs human review reason.

Acceptance Criteria

• Do not inherit the due date for items in the human review queue if the previous versions have only Reported for abuse within the add-on or Escalated for an abuse report, via Cinder as the needs human review reason.
• Don’t break the developer reply date logic

┆Issue is synchronized with this Jira Task

[KevinMind]

A version can be in the queue for multiple reasons simultaneously. Given this the number of possible "reason states" is very large. To determine which combinations of reasons should be included in the "inherit due date" bucket would be very difficult.

Given this, the short term solution is to skip due date inheritance when due date on a past version is only coming from "Reported for abuse within the add-on" or "Escalated for an abuse report, via Cinder".

Longer term solution would be to define the logic inversely where we define the case we do want to inherit. But as stated above this is a long list of options.

Issues for the future:

• which versions should we look at for determining inheritance
• what possible reasons for being in the queue are relevant

[diox]

Preempting future comments about an edge case:

• Version A is auto-approved
• Version A gets reported for abuse within the add-on (it gets a due date)
• Later, Version A gets a NHR for another, non-abuse related reason (At that point that version won't get a new due date - it already has one)
• Reviewer resolves reports for Version A, but doesn't review the version. That leaves the non-abuse-related NHR active.

In that case if a future Version B is flagged for human review it could inherit from that due date of Version A. Technically the due date for version A came from the abuse reports first, but it doesn't matter, there is only one due date for a given Version, and it persists so long there are NHRs left on it.

[diox]

There is another scenario I need clarification on though:

• Version A is submitted and auto-approved.
• Version A gets reported for abuse within the add-on (gaining Reported for abuse within the add-on NHR and a due date)
• Version B is submitted and auto-approved. Because Version A had an active NHR and a due date, it gets a due date and a NHR Previous version in channel had needs human review set ¹.
• Version C is submitted
• Because Version A and B had active NHR and due dates, Version C gets a due date.

What's the expected due date for Version C? With the ticket as described for the moment, it won't inherit version A's due date, but it will inherit from version B, since the reason here is Previous version in channel had needs human review set, not one of the reasons we ignore for due date inheritance purposes.

Footnotes

1. Previous version in channel had needs human review set is a special NHR reason we automatically set at submission time when another version in the same channel had an active NHR, as a way to carry on flagging future versions as needing our attention: we don't know if it has the same potential issues as the older one, but it might, so we err on the side of caution and flag it as well. This is important because the older version might not stick in the queue depending on whether it had already been signed or not. ↩

[wagnerand]

I thought about this extensively and the only solution I could think of that doesn't outright create more problems would be to prevent Previous version in channel had needs human review set being set for version B in the first place.

And come to think of it, this makes more sense to me than not. If a specific version was flagged for review because of an abuse report, we shouldn't automatically flag future versions (upon upload) as well. The abuse report was against a specific version (or against the listing), so this shouldn't lead to multiple versions needing review, at least not automatically.

[diox]

That NHR is typically important if the previous NHR was on a version awaiting review. Consider the following scenario:

• Malicious actor submits listed version A, it gets flagged by a scanner and not auto-approved. It gets a Hit scanner rule NHR and a due date.
• Same actor submits new Version B while Version A is waiting for approval, hoping that they found a way to get auto-approved. That disables Version A, which loses its due date. The automatically NHR for Previous version in channel had needs human review set (and therefore due date) on Version B ensures we don't lose track of the fact that something fishy was going on and that we should review that add-on.

Same idea applies if the original NHR was Manually set as needing human review by a reviewer - it was set because someone thought it was important to look at this add-on, but if the version had not been signed yet, we'd lose it when it gets disabled, so carrying it over the newer version is important.

Edit: we could decide to only carry it over if the previous version that had the NHR was awaiting review though. If it was approved (and therefore signed) we wouldn't lose the old NHR so that's less of an issue.

[wagnerand]

Sorry, let me clarify what I meant:

...prevent Previous version in channel had needs human review set being set for version B in the first place

... IF the previous reason(s) was/were only Reported for abuse within the add-on or Escalated for an abuse report, via Cinder.

[diox]

Ah, got it. Sounds like a good idea, anything related to inheritance (of due date or of NHR) would follow the same rule then, and avoid versions that only have Reported for abuse within the add-on or Escalated for an abuse report, via Cinder. I can do that.