Implementation Plan: Bulk moderation actions

[stacimc]

Fixes

Fixes #1967 by @sarayourfriend

Description

I've chosen @obulat and @sarayourfriend as reviewers. In particular I'd like to call out to @sarayourfriend (as I do in the document itself) that this plan build on and makes some slight change to the work in #3494 that should be carefully reviewed.

This discussion is following the Openverse decision-making process. Information about this process can be found on the Openverse documentation site.

Requested reviewers or participants will be following this process. If you are being asked to give input on a specific detail, you do not need to familiarise yourself with the process and follow it.

Current round

This discussion is currently in the Decision round.

The deadline for review of this round is 19 February 2024.

Checklist

• My pull request has a descriptive title (not a vague title likeUpdate index.md).
• My pull request targets the default branch of the repository (main) or a parent feature branch.
• My commit messages follow best practices.
• My code follows the established code style of the repository.
• I added or updated tests for the changes I made (if applicable).
• I added or updated documentation (if applicable).
• I tried running the project locally and verified that there are no visible errors.
• I ran the DAG documentation generator (if applicable).

Developer Certificate of Origin

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

[stacimc]

@sarayourfriend I'm very happy to extend the deadline for review given your schedule, please just let me know! 🙂

[github-actions]

Full-stack documentation: https://docs.openverse.org/_preview/3719

Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again.

You can check the GitHub pages deployment action list to see the current status of the deployments.

New files ➕:

• https://docs.openverse.org/_preview/3719/projects/proposals/trust_and_safety/content_report_moderation/20240122-implementation_plan_bulk_moderation_actions.html

[sarayourfriend]

5 Feb should be fine but I will let you know by Thursday if I am going to be late on this. At worst I would review this on my 5 Feb, which is before your 5 Feb, and you would never suspect that I'd done it last minute 🤡 But I'll let you know if that isn't going to work for whatever reason.

[sarayourfriend]

This looks really great, and I anticipate approving without issue, provided the usage of Elasticsearch or Postgres for searching records to bulk action against (and the rationale for the decision) is clarified. My hunch is that Elasticsearch is the safer option, and may even be the necessary option, considering creator is not indexed in the API Postgres. However, I haven't spent a great deal of time thinking about this, so I am really just asking for clarification on which we will use and why considering the limitations of each. Postgres would be a lot less work overall, I imagine, considering how easily Django admin already interacts with it. But is the performance hit worth that? Probably that balances against how much effort it takes to use Elasticsearch instead of Postgres for that view.

[obulat]

I will also approve this IP :)

My main 2 questions are:

1. Do we delete the item from the main media table, or only Elasticsearch? (see more details in the inline comment)
2. How will we proceed with updating the related IP? Will we open another issue for it?

[stacimc]

I am going to leave this in the Revision round for a few more days before moving to the final round, to give @sarayourfriend and @obulat another chance to respond to continue the discussion on a few comments, in particular:

• whether to change the behavior of admin deindexing to only remove records from Elasticsearch, to give a 'safety window' for reindexing

In response to @obulat's other question, How will we proceed with updating the related IP: I think it's sufficient to update the existing issue for creating the ModerationDecision. There are actually very few changes needed per this final draft. Curious what others think though!

[sarayourfriend]

whether to #3719 (comment) to only remove records from Elasticsearch, to give a 'safety window' for reindexing

To clarify my answer from the comment thread, my vote is a strong "no", it is not worth making a change for this. My reasons are in the thread.

[stacimc]

Re-requesting reviews as this has entered the Decision Round :)

[sarayourfriend]

LGTM! Nice work 👍

[dhruvkb]

While I'm not an assigned reviewer, I am still sharing some comments that came up during working on #3760. I'll add more comments if something comes up later, but these are the thoughts I have so far.

[dhruvkb]

Since we are using PostgreSQL media_identifiers can be implemented as a ArrayField in Django that allows the array to be stored in-table rather than as a separate join table. Also it allows us to have the weak constraints because they will just be strings to the ArrayField.

[stacimc]

I prefer the join table due to the assumption that bulk reports may affect large numbers of records. The size of the rows in the ModerationDecision table could get quite large and querying on the field could be very slow.

[dhruvkb]

Ah yes, that makes sense. I had not thought about the impact this would have on querying. But wouldn't DB-writes be incredibly slow if we have to write individual records for each media item in a bulk-moderation event?

[stacimc]

I don't believe so -- we can use RelatedManager.set to bulk set the list of related records, if I'm understanding your question!

[sarayourfriend]

@obulat are you able to review the implementation plan soon? It would be great to get this approved soon (with any changes needed, of course, if you have blockers to share) so that we can update the issues from the previous implementation plan with the new ModerationDecision model and other modifications to the existing plan.

[openverse-bot]

Based on the medium urgency of this PR, the following reviewers are being gently reminded to review this PR:

@obulat
This reminder is being automatically generated due to the urgency configuration.

Excluding weekend¹ days, this PR was ready for review 5 day(s) ago. PRs labelled with medium urgency are expected to be reviewed within 4 weekday(s)².

@stacimc, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings.

Footnotes

1. Specifically, Saturday and Sunday. ↩

2. For the purpose of these reminders we treat Monday - Friday as weekdays. Please note that the operation that generates these reminders runs at midnight UTC on Monday - Friday. This means that depending on your timezone, you may be pinged outside of the expected range. ↩

[stacimc]

I've just realized (prompted by @dhruvkb 's comment above) that I may need to clarify the specifics of the implementation for the many to many relationship. I likely do not have time to do so today so I'm going to draft this briefly.

[sarayourfriend]

@obulat Can you confirm you've seen the request to review this? Will you confirm if you will approve with the change Staci is making or if you'll require other changes?

[stacimc]

I was briefly concerned that there would be an issue with using the ManyToManyField as designed, due to the requirement that the record ids be non-constrained. TL;DR I had forgotten that this is possible using the db_constrained argument as we already do elsewhere in the code (for example, in the implementation of the MediaReports). I thought it was worth adding a specific mention of that argument to the IP and will also do so in the issues when they are updated/created. This should not require more work and I am undrafting now, apologies -- better to be safe 😅

[obulat]

The plan is solid and very detailed as it considers all of the edge cases, and also updates the related plan.
Sorry for the delay

[stacimc]

I've created issues under this milestone, and updated the relevant issue from the related IP.