Implementation Plan: Copy updates `mature` -> `sensitive` #2126

AetherUnbound · 2023-05-18T00:17:33Z

Due date:

2023-06-08

Assigned reviewers

@krysal (for API expertise)
@dhruvkb (for frontend expertise)

Description

Resolves: #1965

This is the implementation plan for copy/code changes necessary for updating references from mature to sensitive for various aspects of the frontend and API.

Admittedly I have very little familiarity with the frontend and translations specifically, so my understanding there may be inaccurate. Any guidance in that area would be welcome!

Current round

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.

This discussion is currently in the Decision round.

The deadline for review of this round is 2023-06-08

github-actions · 2023-05-18T00:26:15Z

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

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.

krysal

Thank you for writing this proposal. It looks quite detailed, so I just have a question on this round.

krysal · 2023-05-23T03:01:58Z

...rust_and_safety/content_report_moderation/20230517-implementation_plan_mature_copy_change.md

+The Elasticsearch model property
+[`mature`](https://github.com/WordPress/openverse/blob/2041c5df1e9d5d1f9f37e7c177f2e70f61ea5dba/ingestion_server/ingestion_server/elasticsearch_models.py#L118)
+will also not be affected.


Will the ES model property also change later in conjunction with the field addition to the serializer?

I'm not sure! I think sensitive is actually something we might end up deriving from a few different places in Elasticsearch, and might not be something we store on the indexed records directly. @sarayourfriend did you have a different impression?

mature is already a derived property. It theoretically can encode if a provider supplies the fact but also gets flipped to true for confirmed user reports. If we can update it to sensitive that would be ideal. It again would take more careful planning and potentially several PRs to do it in a backwards compatible way (similar to a database column rename, really).

Although, even more ideally would be to rename it to (something like) has_confirmed_sensitivity_report and then when we ingest sensitivity from providers in the future to use yet another field for that. If we can make the name more explicit and prevent overloading the meaning in the future, that would be 100% better than the current ambiguous name.

That makes sense! It seems like it might be outside the scope of this IP though, so I'll leave this as-is for now unless there are any objections.

Totally, I did not think it was something we would handle in this. Totally fine by me to continue excluding it from the scope.

krysal · 2023-05-23T03:22:26Z

...rust_and_safety/content_report_moderation/20230517-implementation_plan_mature_copy_change.md

+The following models will need to be updated. Since we want to avoid a migration
+with this work, they will need to
+[explicitly reference the old table names using `Meta::db_table`](https://docs.djangoproject.com/en/4.2/ref/models/options/#table-names).


I'll also support a version of this IP with migrations. No need to do it now but it would be a plus to keep the DB model updated too.

It would probably be better to do sooner than later, if it's decided that that is desirable. It'll only get more tedious as the tables grow in size, right? Specifically with respect to the fact that we'd need to keep two tables for the rename to work with zero-downtime and copying the data will take longer as the tables grow. They're all pretty small (relatively speaking) at the moment, however.

Agreed, if we were going to change them now would be the time. I can rework this a bit to include instructions for how to approach that.

sarayourfriend · 2023-05-24T02:03:51Z

...rust_and_safety/content_report_moderation/20230517-implementation_plan_mature_copy_change.md

+The
+[`mature` reason](https://github.com/WordPress/openverse/blob/2041c5df1e9d5d1f9f37e7c177f2e70f61ea5dba/api/api/models/media.py#L22-L21)
+(and
+[`mature_filtered`](https://github.com/WordPress/openverse/blob/2041c5df1e9d5d1f9f37e7c177f2e70f61ea5dba/api/api/models/media.py#L18-L17)
+value) in `AbstractMediaReport` definition will also need to be changed to
+`sensitive` and `sensitive_filtered` respectively. In order to remain backwards
+compatible with the frontend, incoming references on reports for `mature` will
+need to be mapped to the new value.


If I'm understanding the approach suggested here, this would have us update all the previous report values, right? I think that will require multiple steps including Django migrations (Django insists on encoding "choices" into migrations even though Postgres doesn't know anything about it) and (even if Django migrations aren't needed) a management command to update the old values (as per our documentation on data transformations). Is my understanding of the suggestion correct? If so I think it would be worth outlining the steps for that explicitly.

FWIW, I don't think this violates the no-migrations stipulation from the project proposal as that was aimed primarily at table and column name changes rather than data changes. Table and column name changes are just a lot harder to juggle and can be masked in code and UI using Django's tools. The data inside the columns, however, is easier to update and much peskier if we don't (easy to accidentally mess up queries, etc).

Oh that's very true, thank you for mentioning it! I'll add that to this IP 🙂

dhruvkb

LGTM. Apart from what seems like a typo, I have nothing more to add after what @krysal and @sarayourfriend have already covered.

...rust_and_safety/content_report_moderation/20230517-implementation_plan_mature_copy_change.md

AetherUnbound · 2023-05-25T18:41:48Z

Thanks for the review folks! I'm moving this into the Revision round while I make updates to the document 🚀

AetherUnbound · 2023-06-06T00:00:01Z

@krysal and @sarayourfriend - I've added what I believe are the necessary steps for modifying the underlying table names. This seemed like a good opportunity to modify the report table names as well, since we'd also be changing the reason value in the table anyway (and doing this via a different table seemed easiest anyway). Clearly that breaks our intention to not have this IP include migrations :smiling: But I think now is a good time as we have so few records (<700 reports on the image table for instance)! Please let me know if any of these steps could use more clarity; it was a lot to wrap my head around for what needed to be done in which order and get it all into the IP.

@dhruvkb do you mind also taking one more look at the new proposed approach to make sure it seems okay?

Given the revisions I'm moving this into the Decision Round.

sarayourfriend

One blocker, but just to add a critical step/detail to the migration to cover the ingestion server changes.

Really excited about this work, thanks for the careful planning, Madison!

sarayourfriend · 2023-06-06T00:53:40Z

...rust_and_safety/content_report_moderation/20230517-implementation_plan_mature_copy_change.md

+1. Add new models with updated names and fields for `SensitiveImage` and
+   `SensitiveAudio`. Also add new models for `ImageReport` and `AudioReport`.
+   This will require renaming code references from the existing `ImageReport`
+   and `AudioReport` models to `NsfwReport` and `NsfwReportAudio` temporarily
+   while maintaining the reference to the existing table name. Said another way:
+   - `ImageReport` -> `NsfwReport` _(code only, `db_table` should remain the
+     same)_
+   - `AudioReport` -> `NsfwReportAudio` _(code only, `db_table` should remain
+     the same)_
+   - Add `ImageReport` _(new model, `db_table` will not need to be overridden)_
+   - Add `AudioReport` _(new model, `db_table` will not need to be overridden)_
+2. Modify the API to write to both the old and new tables for each of the above
+   instances. On the new report tables, the `mature_filtered` reason should be
+   written as `sensitive_filtered`. Data should continue to be read from the
+   original tables during this time.
+3. Deploy this version of the API.
+4. Create a
+   [data management command](https://docs.openverse.org/general/zero_downtime_database_management.html#django-management-command-based-data-transformations)
+   which copies all data from the original tables to the new tables. The
+   `identifier` field can be used for determining which rows have already been
+   copied.
+5. Deploy this version of the API and run the data management command until no
+   more rows need to be copied for each of the columns.
+6. Remove all code references to the old model from the API, but leave the
+   models in place for the time being. Data should now only be written to and
+   read from the new tables.
+7. Deploy this version of the API so that the only versions currently deployed
+   are writing to the new tables.
+8. Remove the old models (`MatureImage`, `MatureAudio`, `NsfwReport`,
+   `NsfwReportAudio`) from the API codebase and deploy this version of the API.
+   This will remove the old tables from the database.


Excellent! This is a clear description of this process and I'm very glad we'll soon have a concrete example of it with real life PRs to point to for future projects to reference.

...rust_and_safety/content_report_moderation/20230517-implementation_plan_mature_copy_change.md

sarayourfriend · 2023-06-06T00:58:50Z

...rust_and_safety/content_report_moderation/20230517-implementation_plan_mature_copy_change.md

+This work will include several migrations for the API, which will need to be
+deployed one after another in production as described in
+[the API section](#api).


This will need an update to note the ingestion server timed deployment as well.

I suppose we'll need to coordinate around/pause data refresh for that, which would be good to call out, especially if we run any more experiments with the popularity calculations on the real catalog db.

sarayourfriend · 2023-06-06T01:01:03Z

...rust_and_safety/content_report_moderation/20230517-implementation_plan_mature_copy_change.md

+The frontend code work should wait until the API work is complete so the content
+reporting using `sensitive` rather than `mature` is available on the API prior
+to the frontend code changes.


This probably needs to take into account the changes that will happen in #2118, primarily to note that we'll need to coordinate around that.

I don't think the frontend copy changes need to be blocked on the API work though, which seems to be what the first sentence in this section says.

Not a blocker, just clarification. I think this coordination will happen naturally as the work on the frontend copy gets done. Maybe just adding a note to the frontend issues to be aware that the other project will also be making related changes and to be aware of potential overlaps/merge conflicts that might arise.

AetherUnbound · 2023-06-09T11:10:08Z

@sarayourfriend I believe I've addressed your concerns but let me know if there's anything else I'm missing! Thanks for your thorough review 🙌🏼

sarayourfriend

LGTM 🚀

Not a requested reviewer.

openverse-bot · 2023-06-19T00:00:14Z

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

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

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

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

Specifically, Saturday and Sunday. ↩
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. ↩

...rust_and_safety/content_report_moderation/20230517-implementation_plan_mature_copy_change.md

krysal

It's a pretty comprehensive plan and looks good!

I only have a question about one of the steps for the API, but it shouldn't be a blocker.

...rust_and_safety/content_report_moderation/20230517-implementation_plan_mature_copy_change.md

krysal · 2023-06-20T21:28:39Z

...rust_and_safety/content_report_moderation/20230517-implementation_plan_mature_copy_change.md

+   [`exists_in_mature` table check](https://github.com/WordPress/openverse/blob/765a24028baa922b27fc0c38b8ae4c69902eec40/ingestion_server/ingestion_server/queries.py#L33-L37)
+   (this will necessarily involve updating tests and initialization SQL as
+   well).
+7. Remove all code references to the old model from the API, but leave the


Which model exactly are we speaking of here? Is it in the code from the Ingestion server?

This refers to the MatureImage and MatureAudio models, which are replaced by SensitiveImage and SensitiveAudio`. I'll make that explicit here. All the steps here deal with the API, unless explicitly stated like in step 6.

...rust_and_safety/content_report_moderation/20230517-implementation_plan_mature_copy_change.md

Co-authored-by: Krystle Salazar <krystle.salazar@automattic.com>

AetherUnbound requested a review from a team as a code owner May 18, 2023 00:17

AetherUnbound requested review from krysal and dhruvkb May 18, 2023 00:17

github-actions bot added the 🧱 stack: documentation Related to Sphinx documentation label May 18, 2023

AetherUnbound added 🟨 priority: medium Not blocking but should be addressed soon 🌟 goal: addition Addition of new feature 📄 aspect: text Concerns the textual material in the repository 🧭 project: implementation plan An implementation plan for a project labels May 18, 2023

krysal reviewed May 23, 2023

View reviewed changes

sarayourfriend reviewed May 24, 2023

View reviewed changes

dhruvkb approved these changes May 25, 2023

View reviewed changes

...rust_and_safety/content_report_moderation/20230517-implementation_plan_mature_copy_change.md Show resolved Hide resolved

...rust_and_safety/content_report_moderation/20230517-implementation_plan_mature_copy_change.md Outdated Show resolved Hide resolved

AetherUnbound marked this pull request as draft May 25, 2023 18:41

sarayourfriend mentioned this pull request May 28, 2023

Filter and blur sensitive results by term matching #377

Closed

10 tasks

AetherUnbound added 2 commits June 5, 2023 15:58

Initial IP content

dd4f595

Add other details

7b95a4c

AetherUnbound force-pushed the projects/mature-to-sensitive-ip branch from e1e2376 to 7b95a4c Compare June 5, 2023 22:58

Add migrations to the IP, address feedback

765a240

AetherUnbound marked this pull request as ready for review June 5, 2023 23:56

sarayourfriend requested changes Jun 6, 2023

View reviewed changes

Add ingestion server changes as necessary, other feedback

28b2458

AetherUnbound requested review from sarayourfriend and krysal June 9, 2023 12:46

krysal requested a review from dhruvkb June 9, 2023 14:13

sarayourfriend previously approved these changes Jun 12, 2023

View reviewed changes

sarayourfriend requested review from sarayourfriend and removed request for sarayourfriend June 12, 2023 23:59

This was referenced Jun 14, 2023

Implementation Plan: Nuxt 3 migration #2350

Closed

Sensitive content report moderation workflow #383

Open

sarayourfriend approved these changes Jun 19, 2023

View reviewed changes

...rust_and_safety/content_report_moderation/20230517-implementation_plan_mature_copy_change.md Show resolved Hide resolved

krysal approved these changes Jun 20, 2023

View reviewed changes

AetherUnbound and others added 5 commits June 26, 2023 15:38

Use ful IP term

0520528

Co-authored-by: Krystle Salazar <krystle.salazar@automattic.com>

Use full IP term

a3ed57d

Co-authored-by: Krystle Salazar <krystle.salazar@automattic.com>

Linting

e2a0b2d

Make the model list explicit

e72162b

Add approvals

b98ec10

AetherUnbound merged commit 26c863c into main Jun 26, 2023

AetherUnbound deleted the projects/mature-to-sensitive-ip branch June 26, 2023 23:14

AetherUnbound mentioned this pull request Dec 21, 2023

Update copy changes implementation plan to avoid table rename #3568

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation Plan: Copy updates `mature` -> `sensitive` #2126

Implementation Plan: Copy updates `mature` -> `sensitive` #2126

AetherUnbound commented May 18, 2023 •

edited

Loading

github-actions bot commented May 18, 2023

krysal left a comment

krysal May 23, 2023

AetherUnbound May 24, 2023

sarayourfriend May 24, 2023

sarayourfriend May 25, 2023 •

edited

Loading

AetherUnbound Jun 3, 2023

sarayourfriend Jun 4, 2023

krysal May 23, 2023

sarayourfriend May 24, 2023

AetherUnbound May 24, 2023

sarayourfriend May 24, 2023

AetherUnbound May 24, 2023

dhruvkb left a comment

AetherUnbound commented May 25, 2023

AetherUnbound commented Jun 6, 2023

sarayourfriend left a comment

sarayourfriend Jun 6, 2023

sarayourfriend Jun 6, 2023

sarayourfriend Jun 6, 2023

AetherUnbound commented Jun 9, 2023

sarayourfriend left a comment

openverse-bot commented Jun 19, 2023

krysal left a comment

krysal Jun 20, 2023

AetherUnbound Jun 26, 2023

Implementation Plan: Copy updates mature -> sensitive #2126

Implementation Plan: Copy updates mature -> sensitive #2126

Conversation

AetherUnbound commented May 18, 2023 • edited Loading

Due date:

Assigned reviewers

Description

Current round

github-actions bot commented May 18, 2023

krysal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarayourfriend May 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhruvkb left a comment

Choose a reason for hiding this comment

AetherUnbound commented May 25, 2023

AetherUnbound commented Jun 6, 2023

sarayourfriend left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AetherUnbound commented Jun 9, 2023

sarayourfriend left a comment

Choose a reason for hiding this comment

openverse-bot commented Jun 19, 2023

Footnotes

krysal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Implementation Plan: Copy updates `mature` -> `sensitive` #2126

Implementation Plan: Copy updates `mature` -> `sensitive` #2126

AetherUnbound commented May 18, 2023 •

edited

Loading

sarayourfriend May 25, 2023 •

edited

Loading