mozilla / addons

☂ Umbrella repository for Mozilla Addons ✨
Other
125 stars 41 forks source link

[Bug]: Flag for human review is no longer present after resolving only one of multiple escalations #14872

Closed ioanarusiczki closed 2 months ago

ioanarusiczki commented 3 months ago

What happened?

STR: Report an extension/theme , report went to Cinder themes queue From Cinder use Escalate to AMO (or a different one that would escalate it to AMO) Go to the review page observe there is an escalation Report again , repeating steps above In rev tools resolve one of the Escalations, not both (I tried with Ignore/Approve)

What did you expect to happen?

The addon/theme should still be in the queue - Manual review or Updated queues

Instead the flag for human review is no longer present

Examples: https://reviewers.addons-dev.allizom.org/en-US/reviewers/review-unlisted/631539 https://reviewers.addons-dev.allizom.org/en-US/reviewers/review/631538 https://reviewers.addons-dev.allizom.org/en-US/reviewers/review/631540

Is there an existing issue for this?

┆Issue is synchronized with this Jira Task

diox commented 2 months ago

Original idea to fix this was to only remove the NeedsHumanReview flag for a given reason (abuse report, appeal, escalation) if there aren't any unresolved jobs for that particular reason. https://github.com/mozilla/addons-server/pull/22401 is a draft PR implementing that.

The problem is, https://github.com/mozilla/addons/issues/1741 gets in the way: Cinder does not create a new job for abuse reports filed while an appeal is ongoing, they get collapsed in the same job. So for the same job, we have a NeedsHumanReview caused by an appeal, and another one caused by reports. That creates potential edge cases depending on what jobs exist for a particular add-on and what the reviewer does. For instance, if there is a job for a regular report and another for an appeal at the same time for whatever reason (no guarantee this can't happen):

Potential workarounds I see:

diox commented 2 months ago

Original idea to fix this was to only remove the NeedsHumanReview flag for a given reason (abuse report, appeal, escalation) if there aren't any unresolved jobs for that particular reason (...) Use my approach, but never add new NHR for abuse violation reason if there is an ongoing appeal

We're going with this.

ioanarusiczki commented 2 months ago

I tried some tests with multiple escalations, multiple appeals for an extension and for a theme.

Verified on -dev.