addons [Bug]: Closing multiple abuse reports during a review rejection resulted in multiple rejections being sent

What happened?

When processing an add-on review with a rejection action and closing abuse reports sending out the review verdict, if multiple abuse reports are sent, then the version log shows multiple entries for the same review action (rejection, in this case)

Example AMO ID: 2756955

What did you expect to happen?

Irrespective of how many abuse reports are closed, one reviewer action should only result in one reviewer action log in the add-on's version review history.

Is there an existing issue for this?

[x] I have searched the existing issues

┆Issue is synchronized with this Jira Task

Mar 26 '25 17:03 abhn

@abhn: a single job with multiple abuse reports, or multiple jobs (with one or more abuse reports each)?

Mar 28 '25 11:03 eviljeff

Hmm, I don't know which one exactly right now.

Can there be multiple jobs with one or more abuse reports each attached to a single version? @eviljeff

Mar 28 '25 11:03 abhn

Hmm, I don't know which one exactly right now.

Can there be multiple jobs with one or more abuse reports each attached to a single version? @eviljeff

If they come in directly as abuse reports, no, Cinder will combine them. If they're forwarded by TaskUs moderators, yes.

If they're separate jobs they will have separate checkboxes to select to resolve; if they're multiple reports in a single job, it'll be a single checkbox

Mar 28 '25 12:03 eviljeff

So if I want to resolve multiple jobs (by selecting multiple checkboxes) when resolving them by submitting a review verdict, multiple reviews would get sent to the developer (with multiple log of the same review verdict in reviewer tools version review log)?

Mar 28 '25 13:03 abhn

So if I want to resolve multiple jobs (by selecting multiple checkboxes) when resolving them by submitting a review verdict, multiple reviews would get sent to the developer (with multiple log of the same review verdict in reviewer tools version review log)?

Yes to the review log - maybe to the developer, I'd have to double check - but they are processed individually, yes.

@abhn: a single job with multiple abuse reports, or multiple jobs (with one or more abuse reports each)?

I didn't see an explicit answer on this - do you remember?

Mar 28 '25 17:03 eviljeff

I didn't see an explicit answer on this - do you remember?

I don't, as I didn't process that review nor is that information exposed in reviewer tools.

Yes to the review log - maybe to the developer

Okay understood. It is suboptimal to have multiple review entries for what was a single action from the reviewer's PoV.

Given that we do not record what was closed during a review submission, and just log the reviewer's verdict, it makes it even less comprehensible what happened.

Should it log multiple times if a single action closed the multiple reports?

Apr 01 '25 09:04 abhn

It was multiple check boxes, each checkbox with several abuse reports attached to it, so definitely multiple jobs.

Apr 01 '25 11:04 ralli007

@abhn for clarification could you attach a screenshot of this (in jira)

Apr 01 '25 13:04 KevinMind

Need to investigate, are there multiple jobs ? I assume yes because of forwarded jobs ? Are there multiple decisions created then ? If so, I assume we don't want to link a single activity to multiple decisions, can we have a single decision closing multiple jobs ?

Apr 01 '25 13:04 diox

Are there multiple decisions created then ? If so, I assume we don't want to link a single activity to multiple decisions, can we have a single decision closing multiple jobs ?

In Cinder it's a single decision per job, so each job is resolved separately. So we need to create a ContentDecision for each CinderJob.

It is possible to have multiple ContentDecision instances associated with a single activity log on a technical level, and what emails we send is our choice - whether we add the extra complexity to create a single ActivityLog instance, and send out a single email to the developer (both in implementation time, and maintainability) is another question though.

Apr 02 '25 12:04 eviljeff

To investigate: can we do what we are doing now but somehow avoid duplicating emails to the owner(s) (still need to send emails to each reporter if necessary) and activities ?

How to deal with appeals is another question though, as they are tied to each decision

May 07 '25 14:05 diox

To investigate: can we do what we are doing now but somehow avoid duplicating emails to the owner(s) (still need to send emails to each reporter if necessary) and activities ?

Given that we currently have to use the activity log to try and measure performance, the impact is more than just duplicated emails.

May 07 '25 15:05 wagnerand

Yes, I know, my sentence had a "and activities" at the end to account for that.

May 07 '25 16:05 diox

I think the path forward is:

In the process_action() method for the various ContentAction classes, don't record a new ActivityLog if we didn't do anything (i.e. the action has already been carried out), and return None instead of the activity. That's already how it's implemented for many of them, including ContentActionDisableAddon which is used for Force Disable, but not for ContentActionRejectVersion/ContentActionRejectVersionDelayed. It's a little messier to figure out that we didn't do anything for those actions but it's doable.
In reviewers/utils.py record_decision(), handle the fact that log_entry could be None after calling execute_action(), and pass something to report_decision_to_cinder_and_notify() task indicating whether or not that decision had a log_entry.
In abuse/tasks.py report_decision_to_cinder_and_notify() task, pass down that information to send_notifications()
In abuse/models.py send_notifications(), if we didn't record a log entry, don't call notify_owners(), only notify reporters.

We would still record as many decisions as there are jobs to resolve, but only one would create a single ActivityLog and send a single email to owners. As mentioned above, that's already how it's implemented for Force Disable [^1].

A small implementation question to solve with this approach: would we want to attach the lone ActivityLog to each decision ? If so, we'd need to be careful do still not notify owners only once.

[^1]: Although fun fact, at the moment, if you force disable resolving 2 different jobs, we will raise a 500 error, because we attempt to create a ReviewActionReasonLog with activity_log=log_entry for all decisions, including those that result in log_entry being None.

May 15 '25 11:05 diox

We probably still want to associate all the ContentDecision instances with the activity log though - I guess we can just manually create ContentDecisionLog instances to link them?

May 15 '25 12:05 eviljeff

Yes, that was my question at the end. We could edit the activity details and relink any missing ContentDecision once they have all been created.

May 15 '25 13:05 diox

STR:

Report an add-on for policy violation on the website (leave your email as the reporter)
In Cinder, escalate that to reviewers
Report an add-on for policy violation on the website again (leave a different email as the reporter)
In Cinder, escalate that to reviewers again
In the review page, that add-on should now have 2 jobs to resolve when selecting an action
Disable the add-on while checking the checkboxes to resolve those 2 jobs
The developer should get a single email, each reporter should get one too. A single activity log should have been recorded and made visible in the review page

Repeat with a different add-on, this time rejecting multiple versions instead of disabling the add-on.

May 21 '25 11:05 diox

I tried this on dev and stage.

On dev I no longer see the email sent to the developer multiple times. Each reporter will get an email. In case a reporter has been reporting the add-on multiple times then it will get multiple emails about the content being disabled.

The review pages display the activity once scenario with reject from rev tools https://reviewers.addons-dev.allizom.org/en-US/reviewers/review-listed/637874 scenario with disable from rev tools https://reviewers.addons-dev.allizom.org/en-US/reviewers/review-listed/637848

On stage I could reproduce the scenario with reject -> multiple emails sent to developer + the activity displayed multiple times https://reviewers.addons.allizom.org/en-US/reviewers/review-listed/2248196

Also on -stage when disabling the add-on with this scenario I got a sever error but then checking things I see the addons disabled. This server error is not reproducible on dev.

I tried with https://reviewers.addons.allizom.org/en-US/reviewers/review/2248197

What I didn't see on -dev is reject and disable actions registered in reviewactionreasonlog admin. That's available now on -stage so it appears to be a regression.

I checked for activity logs in admin for this addon which has been rejected. Or this addon which has been disabled.

May 23 '25 11:05 ioanarusiczki

What I didn't see on -dev is reject and disable actions registered in reviewactionreasonlog admin. That's available now on -stage so it appears to be a regression.

Is that because of https://github.com/mozilla/addons/issues/15513 ? The policy should be attached still.

May 23 '25 13:05 diox

Could be caused by it , last review action reason on dev is from May 21.

May 23 '25 13:05 ioanarusiczki

Filed it https://github.com/mozilla/addons/issues/15622

May 23 '25 14:05 ioanarusiczki