Clear NeedsHumanReview selectively depending on job being resolved

[diox]

Fixes: mozilla/addons#14872

Description

If there are multiple jobs for a given add-on, when resolving a job we only want to clear the NeedsHumanReview flag if the job we're resolving is the last one of its kind: the add-on should stay in the queue until all jobs are resolved.

With that logic in place, because reports can be added to an ongoing appeal job, we need to also prevent NHR from being added following a report attached to an appeal job (otherwise we can't determine what NHR to clear). It's redundant anyway, there will already have a NHR for the appeal and that's enough.

[diox]

Depending on test ordering I managed to end up with that waffle inactive

[eviljeff]

just a coding nit

[eviljeff]

might be missing an edge case, but we could just not set reason to a non-None value unless the queryset exists, then we don't need this has_unresolved_jobs_with_similar_reason variable.

[diox]

Like this?

diff --git a/src/olympia/abuse/models.py b/src/olympia/abuse/models.py
index 0118d97338..f4da6a7cf7 100644
--- a/src/olympia/abuse/models.py
+++ b/src/olympia/abuse/models.py
@@ -302,26 +302,22 @@ class CinderJob(ModelBase):
                 .unresolved()
                 .resolvable_in_reviewer_tools()
             )
+            reason = None
             if was_escalated:
-                has_unresolved_jobs_with_similar_reason = (
-                    base_unresolved_jobs_qs.filter(
-                        decision__action=DECISION_ACTIONS.AMO_ESCALATE_ADDON
-                    ).exists()
-                )
-                reason = NeedsHumanReview.REASONS.CINDER_ESCALATION
+                if base_unresolved_jobs_qs.filter(
+                    decision__action=DECISION_ACTIONS.AMO_ESCALATE_ADDON
+                ).exists():
+                    reason = NeedsHumanReview.REASONS.CINDER_ESCALATION
             elif self.is_appeal:
-                has_unresolved_jobs_with_similar_reason = (
-                    base_unresolved_jobs_qs.filter(
-                        appealed_decisions__isnull=False
-                    ).exists()
-                )
-                reason = NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL
+                if base_unresolved_jobs_qs.filter(
+                    appealed_decisions__isnull=False
+                ).exists():
+                    reason = NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL
             else:
                 # If the job we're resolving was not an appeal or escalation
                 # then all abuse reports are considered dealt with.
-                has_unresolved_jobs_with_similar_reason = None
                 reason = NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION
-            if not has_unresolved_jobs_with_similar_reason:
+            if reason:
                 NeedsHumanReview.objects.filter(
                     version__addon_id=cinder_decision.addon_id,
                     is_active=True,

I'm not sure this is better, it feels harder to read for me (though I agree the has_unresolved_jobs_with_similar_reason = None in my original version is not ideal)

[eviljeff]

yeah, like that. But I do see it's not a whole lot less code with all the line wrapping and wordy queries and constants. It'd probably read better if we had queryset methods for .escalated and is_appeal but that's even more extra code... so 🤷 your choice on whatever you think is best.

[diox]

Ok, going to keep my original, slightly more verbose version for now. I suspect this is not the last time we'll need to refactor this code anyway...