process jobs from legal escalations with no abuse reports #23024
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: src/olympia/abuse/models.py
Did you find this useful? React with a 👍 or 👎 |
eviljeff
requested review from
a team and
KevinMind
and removed request for
a team
| contain any attached abuse reports (i.e. if an add-on was forwared without a | ||
| job).""" | ||
| target = addon_factory() | ||
| cinder_job = CinderJob.objects.create(job_id='1234', target_addon=target) |
There was a problem hiding this comment.
Is it possible to explicitly set "None" to the values that we are testing are in fact None?
That's probably the default but it helps clarify the intentions of the test and protects against any weird scenario where those values are defined internally.
There was a problem hiding this comment.
the fields aren't on CinderJob - they're on AbuseReport and ContentDecision - it's that there aren't any AbuseReports or ContentDecisions (both have a foreign key to the CinderJob)
There was a problem hiding this comment.
Both of those models have database constraints to ensure that addon, collection, rating, user have exactly one non-None value (AbuseReport uses guid rather than an addon foreign key, but same restriction)
There was a problem hiding this comment.
So it would be inverted like
assert AbuseReport.objects.filter(cinder_job=cinder_job).count() == 0
Would that make sense?
There was a problem hiding this comment.
It's impossible for that assertion ever to fail, but as requested c88399c
| rating=abuse_report_or_decision.rating, | ||
| collection=abuse_report_or_decision.collection, | ||
| user=abuse_report_or_decision.user, | ||
| rating=getattr(abuse_report_or_decision, 'rating', None), |
There was a problem hiding this comment.
Before this patch, would calling this method just raise if any of those fields were None? Or how would it have behaved before. That would be good to add to the context as well as testing to know how you'd expect it to behave outside of this patchl.
There was a problem hiding this comment.
It raised because abuse_report_or_decision was None (there were no abuse reports or appealed decisions). I already wrote about in the PR context section, or did you mean write it as code comments?
There was a problem hiding this comment.
Ah I read it wrong then, I interpreted "failed" as the approach failed not the code.
Fixes: mozilla/addons#15278
Description
Handles jobs from Cinder that have no abuse report attached. Also correctly clears NHR from the add-on when the legal escalation is made (the other problem noted in the issue)
Context
CinderJobs in general don't have any target information directly fk'd to them - the information is on the AbuseReport, or appealed decision, that led to the job being created. A legal forward doesn't (necessarily) have any appeals or abuse reports associated with it. But for add-on, we do set
target_addon, so we can use that instead (which we did already as an optimization, but then failed because there was no abuse report or appeal to reference.rating, etc , which would always have beenNonefor an addon abuse report/appeal anyway).Testing
Checklist
#ISSUENUMat the top of your PR to an existing open issue in the mozilla/addons repository.