Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Create sensitive and deleted media models for decisions #4544

Merged
merged 11 commits into from
Jun 26, 2024
Merged

Create sensitive and deleted media models for decisions #4544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
610622a
Record models for media, sensitive media and deleted media in decisio…
dhruvkb Jun 24, 2024
86d013a
Create deleted media and sensitive media models on save
dhruvkb Jun 24, 2024
b6d944f
Create through models when performing moderation from the admin
dhruvkb Jun 24, 2024
bcb9417
Update backfill command to perform action associated with through models
dhruvkb Jun 24, 2024
c9a1fdf
All media objects to be deleted without affecting past decisions
dhruvkb Jun 24, 2024
8cea252
Update tests to account for media items being deleted
dhruvkb Jun 24, 2024
7737df2
Mock ES to prevent breaking other tests
dhruvkb Jun 24, 2024
9e2e54e
Merge branch 'main' of https://github.com/WordPress/openverse into de…
dhruvkb Jun 24, 2024
c5adb41
Undo fixes to the backfill command
dhruvkb Jun 25, 2024
fb8495f
Avoid unnecessary queries
dhruvkb Jun 25, 2024
63ce31c
Add helpful messages when redirecting
dhruvkb Jun 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 30 additions & 2 deletions api/api/admin/media_report.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
from elasticsearch import NotFoundError
from elasticsearch_dsl import Search

from api.constants.moderation import DecisionAction
from api.models import (
Audio,
AudioDecision,
Expand Down Expand Up @@ -377,6 +378,9 @@ def change_view(self, request, object_id, form_url="", extra_context=None):
media_obj = self.get_object(request, object_id)
if media_obj:
extra_context["media_obj"] = media_obj
else:
messages.warning(request, f"No media object found with ID {object_id}.")
return redirect(f"admin:api_{self.media_type}_changelist")

tags_by_provider = {}
if tags := media_obj.tags:
Expand Down Expand Up @@ -440,6 +444,10 @@ def decision_create_view(self, request, object_id):
if request.method == "POST":
media_obj = self.get_object(request, object_id)

through_model = {
"image": ImageDecisionThrough,
"audio": AudioDecisionThrough,
}[self.media_type]
Comment on lines +447 to +450
Copy link
Collaborator

@sarayourfriend sarayourfriend Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bit of a nit, but I've seen this pattern a few times in our code and I don't really understand it. Why not use match/case or if/else for this? The inline object approach is a little too clever (and I never understood defining a static object inline of a function body like this either).

Both match and if/else require explicitly raising if self.media_type doesn't match... but isn't that better? It's certainly easier to read and understand to me (and you know the old phrase about how often code is read compared to written, I'm sure).

Suggested change
through_model = {
"image": ImageDecisionThrough,
"audio": AudioDecisionThrough,
}[self.media_type]
match self.media_type:
case IMAGE:
through_model = ImageDecisionThrough
case AUDIO:
through_model = AudioDecisionThrough
case _:
raise ValueError(f"Unknown media type {self.media_type}")
Suggested change
through_model = {
"image": ImageDecisionThrough,
"audio": AudioDecisionThrough,
}[self.media_type]
if self.media_type == IMAGE:
through_model = ImageDecisionThrough
elif self.media_type == AUDIO:
through_model = AudioDecisionThrough
else:
raise ValueError(f"Unknown media type {self.media_type}")

Even better would be to configure it on the admin itself, along with the media type.

Alternatively, if you want to remove all explicit configuration:

Suggested change
through_model = {
"image": ImageDecisionThrough,
"audio": AudioDecisionThrough,
}[self.media_type]
through_model = getattr(media_obj, f"{self.media_type}decisionthrough_set").model

But it really would be better if it was just a @property of media_obj._meta or something...

Suggested change
through_model = {
"image": ImageDecisionThrough,
"audio": AudioDecisionThrough,
}[self.media_type]
through_model = media_obj._meta.decision_through_model

Anyway, any of those would be expected and easier to understand when reading, I think. (Except the getattr one, that's similarly too clever and it's basically not even worth including as is, but would be improved if it didn't need gettatr and could just be media_obj.decisionthrough_set.model if the media type was removed from the name, which would simplify other code too, not just here).

Copy link
Member Author

@dhruvkb dhruvkb Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I write use the = {}[] pattern is because it is the most compact among all the alternatives and also raises an exception when none of the keys match the input.

I've done this a few times in this file itself. Would you prefer I change this pattern across the entire file, or keep this as is is in the interest of consistency, or just change it here to one of the alternatives you've suggested?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the dict-key-access approach is preferred for any reason at all (it is nice it implicitly throws if the key doesn't exist), I'd at the very least think defining the dictionary outside the runtime scope of the function is a reasonable requirement, if only so that it's in a shared location. It's far-fetched to me to establish a pattern of defining otherwise static dictionaries, especially one encoding relationships between static objects, entirely dynamically in the runtime of a function. From a performance perspective it's fine here, but in a tight loop it's just silly, right? From a shared data/relationship encoding perspective (and discoverability, clarity, etc) it's definitely the worst option I can think of 😕. Just seems like an antipattern to me 🤷 I also don't think compactness is necessarily a virtue, and certainly not in Python, which actively resists compactness in my experience.

Copy link
Collaborator

@sarayourfriend sarayourfriend Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So my request here, to clarify, is to move the dictionary to a static location, out of the function, or define the relationship in some other concrete manner that isn't specific to this function. That can be either: changing the field names on the models so that they can be referenced generically (without the media type prefix), or adding a class property to the base class that resolves these models for the media type based on the fields, or something else like that. Also, a follow-up issue to remove that pattern anywhere it's been added and replace it with the generic approach (whether that's statically defined dicts or the dynamic-but-shared approach of class properties).

In general: these static relationships between media type and the data models should not be defined within a local function context, even ignoring all issues with legibility, ergonomics, and performance of this local dict approach. At the very least, this static relationship should be defined statically, and in a shared location, so that new code automatically references it, and reducing the risk of someone just copy/pasting this function-local definition of the relationship.

The dict-accessor pattern is fine on its own, it's the inline dict definition I think is an anti-pattern (though I think match/case and an explicit raise of ValueError is clearer than KeyError, but that's an aesthetic judgement, I know, as at the end of the day it's more or less intelligible as the same underlying problem).

Edit: I realise I'm blocking this PR that fixes a bug in the admin on a code-style/quality issue. I do think this needs to change and believe it's an anti-pattern, but won't block the PR. I'll write an issue to address this more widely later today instead.

form = get_decision_form(self.media_type)(request.POST)
if form.is_valid():
decision = form.save(commit=False)
Expand All @@ -454,9 +462,13 @@ def decision_create_view(self, request, object_id):
moderator=request.user.get_username(),
)

decision.media_objs.add(media_obj)
through = through_model.objects.create(
decision=decision,
media_obj=media_obj,
)
logger.info(
"Media linked to decision",
"Through model created",
through=through.id,
decision=decision.id,
media_obj=media_obj.id,
)
Expand All @@ -468,6 +480,22 @@ def decision_create_view(self, request, object_id):
report_count=count,
decision=decision.id,
)

if decision.action in {
DecisionAction.DEINDEXED_COPYRIGHT,
DecisionAction.DEINDEXED_SENSITIVE,
}:
messages.info(
request,
"The media object has been deindexed from ES and deleted from DB.",
)
return redirect(f"admin:api_{self.media_type}_changelist")

if decision.action == DecisionAction.MARKED_SENSITIVE:
messages.info(
request,
"The media object has been marked as sensitive.",
)
else:
logger.warning(
"Form is invalid",
Expand Down
4 changes: 4 additions & 0 deletions api/api/models/audio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,10 @@ class AudioDecisionThrough(AbstractMediaDecisionThrough):
be referenced by `identifier` rather than an arbitrary `id`.
"""

media_class = Audio
sensitive_media_class = SensitiveAudio
deleted_media_class = DeletedAudio

media_obj = models.ForeignKey(
Audio,
to_field="identifier",
Expand Down
4 changes: 4 additions & 0 deletions api/api/models/image.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,10 @@ class ImageDecisionThrough(AbstractMediaDecisionThrough):
be referenced by `identifier` rather than an arbitrary `id`.
"""

media_class = Image
sensitive_media_class = SensitiveImage
deleted_media_class = DeletedImage

media_obj = models.ForeignKey(
Image,
to_field="identifier",
Expand Down
24 changes: 22 additions & 2 deletions api/api/models/media.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,8 +335,6 @@ class AbstractMediaDecision(OpenLedgerModel):
class Meta:
abstract = True

# TODO: Implement ``clean`` and ``save``, if needed.


class AbstractMediaDecisionThrough(models.Model):
"""
Expand All @@ -348,6 +346,10 @@ class AbstractMediaDecisionThrough(models.Model):

media_class: type[models.Model] = None
"""the model class associated with this media type e.g. ``Image`` or ``Audio``"""
sensitive_media_class: type[models.Model] = None
"""the model class associated with this media type e.g. ``SensitiveImage`` or ``SensitiveAudio``"""
deleted_media_class: type[models.Model] = None
"""the model class associated with this media type e.g. ``DeletedImage`` or ``DeletedAudio``"""

media_obj = models.ForeignKey(
AbstractMedia,
Expand All @@ -361,6 +363,24 @@ class AbstractMediaDecisionThrough(models.Model):
class Meta:
abstract = True

def perform_action(self, action=None):
"""Perform the action specified in the decision."""

action = self.decision.action if action is None else action

if action in {
DecisionAction.DEINDEXED_SENSITIVE,
DecisionAction.DEINDEXED_COPYRIGHT,
}:
self.deleted_media_class.objects.create(media_obj_id=self.media_obj_id)

if action == DecisionAction.MARKED_SENSITIVE:
self.sensitive_media_class.objects.create(media_obj_id=self.media_obj_id)

def save(self, *args, **kwargs):
super().save(*args, **kwargs)
self.perform_action()


class PerformIndexUpdateMixin:
@property
Expand Down