-
Notifications
You must be signed in to change notification settings - Fork 174
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.
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
Alter Django Admin media views to surface content reports #4254
Conversation
Yes, this needs to be to the identifier, not the The fix is a two line change to add diff --git a/api/api/models/audio.py b/api/api/models/audio.py
index 917be115f..163bb4d58 100644
--- a/api/api/models/audio.py
+++ b/api/api/models/audio.py
@@ -325,6 +325,7 @@ class AudioDecision(AbstractMediaDecision):
media_objs = models.ManyToManyField(
to="Audio",
+ to_field="identifier",
db_constraint=False,
help_text="The audio items being moderated.",
)
diff --git a/api/api/models/image.py b/api/api/models/image.py
index d0ad5323a..2f349ab29 100644
--- a/api/api/models/image.py
+++ b/api/api/models/image.py
@@ -138,6 +138,7 @@ class ImageDecision(AbstractMediaDecision):
media_objs = models.ManyToManyField(
to="Image",
+ to_field="identifier",
db_constraint=False,
help_text="The image items being moderated.",
) Best to either add that in this PR or open an issue as a fast follow. Similarly, (and this is definintely for a separate issue), it'd be nice to set - Count(f"{self.media_type}_report__decision__pk") That would just need to be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM as is. I've left a comment describing the fix for the identifier issue you found. Either add that to this PR or create an issue as a fast-follow. Fixing that will block rolling the feature out, and it should be fixed as soon as possible to avoid introducing more code that will reference the incorrect identifier in queries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, seconding @sarayourfriend's comments about using identifier
field instead of id
.
Co-authored-by: sarayourfriend <git@sarayourfriend.pictures>
74c2fed
to
3337b46
Compare
@sarayourfriend It looks like
Since this is approved and the way to get reference fields working correctly isn't immediately obvious to me, I'm going to go ahead and merge this and create an issue for following up on the field name. |
* Use a more ambiguous related name for reports * Change the media views to order records by total report count * Use plural name for media_reports reference * Let media ID be used for autocomplete, and return unaltered queryset * Add more fields to sort on * Add links to the pending reports in a column * Rename to "total report count" * Expose the decisions tables (for now) * Add filtering based on pending reports * More tweaks to table * Make pending the default filter rather than all * Revert back to media-specific tables * Wording and whitespace corrections Co-authored-by: sarayourfriend <git@sarayourfriend.pictures> --------- Co-authored-by: sarayourfriend <git@sarayourfriend.pictures>
Fixes
Fixes #3635 by @sarayourfriend
Description
This PR modifies the base media list views to accomplish the following:
This work was admittedly pretty hairy 😅 I'm hoping that the changes I've made here make sense, I'm certainly pleased that I was able to get all the functionality set up correctly! I'm happy to make anything more idiomatically "Django" where it makes sense to.
I also exposed the decision views so we could more easily add decisions for media, at least for testing. I'm sure that will change, and I'm honestly a little unclear on all of the moving parts here and how decisions will actually be applied. This view currently links to each individual report, but since decisions can be applied to multiple reports and multiple media records, I'm just not sure what all the actions will look like. That to say, this view might change a little once that's built out, which is okay!
Screenshots
Pending only filtered (default)
All reported records
Testing Instructions
The easiest way to do this is the following:
just down -v && just api/init
just api/dbshell
and execute the following:Then you should be able to go to http://localhost:50280/admin/api/image/ and see the views in the screenshots above!
The actual
image_id
s in theapi_imagedecision_media_objs
table might be non-deterministic, I'm not sure. It makes me wonder too if we should alter that table to useidentifier
in its table reference rather than{media}_id
. It's what we do for the sensitive/deleted objects tables as well: https://github.com/WordPress/openverse/blob/0db784ebc4d4937c925dcb14c1465bafb3e20c69/api/api/models/image.py#L97-L96. CC @dhruvkb as you might be the best informed on that.Checklist
Update index.md
).main
) or a parent feature branch.just catalog/generate-docs
for catalogPRs) or the media properties generator (
just catalog/generate-docs media-props
for the catalog or
just api/generate-docs
for the API) where applicable.Developer Certificate of Origin
Developer Certificate of Origin