-
Notifications
You must be signed in to change notification settings - Fork 212
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
Use explicit through table for media/decision many-to-many field #4310
Conversation
Full-stack documentation: https://docs.openverse.org/_preview/4310 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. Changed files 🔄: |
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! I'm not opposed to having it but the base model is kind of redundant here because all of its fields have to be overridden in the subclasses.
I wasn't sure how best to handle that case, since we have some other abstract models in a similar vein! Happy to remove it though 😄 Edit: Since it's referenced by another abstract model, I'd rather keep it currently. |
Based on the high urgency of this PR, the following reviewers are being gently reminded to review this PR: @obulat Excluding weekend1 days, this PR was ready for review 2 day(s) ago. PRs labelled with high urgency are expected to be reviewed within 2 weekday(s)2. @AetherUnbound, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
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 is just the Django Admin default, and it's temporary as well while we build out the rest of the features! openverse/api/api/admin/__init__.py Lines 57 to 59 in 0a7d31b
|
355fe47
to
55a88c5
Compare
This PR has migrations. Please rebase it before merging to ensure that conflicting migrations are not introduced. |
Fixes
Fixes #4286 by @AetherUnbound
Description
This PR changes the many-to-many relationship between media records and media decisions to reference media by the
identifier
rather than the Django default ID. This required creating a separate, explicit model and using thethrough
property ofManyToManyField
.This produces the following SQL for migrations:
Testing Instructions
Run
just down -v && just api/init
. Then open a connection to the database withjust api/dbshell
and run:This is similar to the testing described in #4254, but note the difference in the intermediary table. Visit http://localhost:50280/admin/api/image/ locally to see the reports & decisions.
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