-
Notifications
You must be signed in to change notification settings - Fork 2
Allow admins to manage media files in Django admin #220
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
Conversation
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.
Pull request overview
This PR adds a FileBrowser section to the Django admin portal, enabling administrators to manage media files. The implementation uses django-filebrowser-no-grappelli and extends it with custom functionality to track and delete orphaned media files (files not associated with any Media record). The PR also updates the database export functionality to exclude filebrowser data.
- Adds FileBrowser integration with custom filtering and deletion capabilities
- Extends FileObject with methods to check and retrieve related Media records
- Updates database export to exclude filebrowser app data
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| TEKDB/requirements.txt | Adds django-filebrowser-no-grappelli dependency |
| TEKDB/TEKDB/views.py | Updates ExportDatabase to exclude apps via EXPORT_DUMP_EXCLUDE setting |
| TEKDB/TEKDB/urls.py | Registers filebrowser URLs under admin/filebrowser/ |
| TEKDB/TEKDB/tests/test_custom_filebrowser.py | Adds comprehensive test coverage for custom filebrowser functionality |
| TEKDB/TEKDB/templates/filebrowser/index.html | Main filebrowser listing template with custom "Clear Unused Media Files" action |
| TEKDB/TEKDB/templates/filebrowser/filter.html | Filter sidebar with custom "By Media Record" filter |
| TEKDB/TEKDB/templates/filebrowser/filelisting.html | File listing rows using original images instead of generated thumbnails |
| TEKDB/TEKDB/templates/filebrowser/detail.html | File detail view showing related Media records |
| TEKDB/TEKDB/templates/filebrowser/delete_no_media_record_confirm.html | Confirmation page for bulk deletion of orphaned files |
| TEKDB/TEKDB/tekdb_filebrowser.py | Custom FileBrowserSite implementation with filtering and deletion logic |
| TEKDB/TEKDB/settings.py | Configures filebrowser settings and EXPORT_DUMP_EXCLUDE |
| TEKDB/TEKDB/apps.py | Dynamically adds Media record tracking methods to FileObject |
Comments suppressed due to low confidence (2)
TEKDB/TEKDB/tekdb_filebrowser.py:177
- 'except' clause does nothing but pass and there is no explanatory comment.
except Exception:
TEKDB/TEKDB/tekdb_filebrowser.py:179
- 'except' clause does nothing but pass and there is no explanatory comment.
except Exception:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
TEKDB/TEKDB/templates/filebrowser/delete_no_media_record_confirm.html
Outdated
Show resolved
Hide resolved
| name = "TEKDB" | ||
| verbose_name = "Records" | ||
|
|
||
| def ready(self): |
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.
is this the right place for adding methods to FileObject?
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.
Hopefully @rhodges will know better than me.
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.
I have not seen anything like this before, but the more I dig into your code (and the FileBrowser source) this makes a lot of sense.
Ideally we'd want to just override FileObject to include these, but since we don't explicitly define any FileObjects, we'd need to override whichever parent or ancestor that we do touch, and by the end we may have a very difficult to maintain 'stack of crap' (my speciality) just to override one deeply embedded class. There is likely a lower-common-denominator where we could override the view that renders the template detail.html to insert the logic and context we need there, but in the end, if this works, I think it's just as elegant (though maybe harder to track & debug).
Knowing about this approach to modifying 3rd Party libraries is something I wish I'd known about long ago. If there are negative consequences, I don't know them.
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.
sounds like this is sticking around! 💩 thanks for the feedback!
TEKDB/TEKDB/settings.py
Outdated
|
|
||
| FILEBROWSER_LIST_PER_PAGE = 20 | ||
|
|
||
| FILEBROWSER_EXTENSIONS = { |
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.
Are these the right extensions we want to allow? I copied this over from the library: https://django-filebrowser.readthedocs.io/en/latest/settings.html#extensions
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.
🤔 I don't have a complete understanding of file extensions being uploaded to TEKDBs'. This feels pretty comprehensive, but is it possible to get by without choosing? If Django FileBrowser will work without FILEBROWSER_EXTENSIONS, maybe leave it out.
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.
I agree with @pollardld that not defining this for ourselves may be for the best, but that depends on the default behavior.
Looks like this is the default: https://django-filebrowser.readthedocs.io/en/latest/settings.html#extensions
The worry is that we cannot anticipate all of the file types we may see. It looks like this limits what people can upload (I'm not sure that's relevant) which I don't like (it'd be great if they could upload other data types, including GIS or metadata [JSON, XML, GeoJSON, SHP, ZIP, etc...]) and I'm hoping this doesn't constrain what files they can manipulate in the view (see above, or any upload 'parts').
Does this impact our use case?
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.
upon second thought, I think that this can be removed entirely since we are not allowing users to upload media through the filebrowser interface. I'll remove!
| @@ -0,0 +1,269 @@ | |||
| import os | |||
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.
is TEKDB/TEKDB/tekdb_filebrowser.py the correct location for this file? I wasn't totally sure.
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.
I'm not sure either, and have no objections.
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 works for now.
In the future, I recommend borrowing the convention used by the library you're overriding. Since this mostly functions to override components and logic from filebrowser.sites, you could call this something like TEKDB/TEKDB/filebrowser/sites.py or filebrowser_sites.py. This is anticipating the potential that we may need to MASSIVELY override multiple components of another tool that are broken up between named files that we cannot anticipate (beyond 'views', 'models', 'admin' or other Django defaults).
| with open(dumpfile_location, "w") as of: | ||
| management.call_command("dumpdata", "--indent=2", stdout=of) | ||
| excludes = getattr(settings, "EXPORT_DUMP_EXCLUDE", []) | ||
| management.call_command( |
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.
I believe that because we have filebrowser in INSTALLED_APPS we need to exclude it from dumpdata. Without it in the excludes list, dumpdata tries to dump it, which throws an error.
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.
Does FileBrowser have any tables in the DB that would be exported? Why would they fail? Why wouldn't we want them captured in the fixture if they are playing a role?
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.
I had a hard time understanding the root of the problem but here is my understanding after reading the code and chatting to copilot:
We include filebrowser in our INSTALLED_APPS the dumpdata command looks for all models in our registry, and Filebrowser is one of them. However, Filebrowser is defined in the library as
class FileBrowser(models.Model):
class Meta:
managed = False
verbose_name = _("FileBrowser")
verbose_name_plural = _("FileBrowser")and managed = False means that django will not create any tables or migrations for this model (which seems correct). So when we use dumpdata without the exclude list it is looking for a FileBrowser table that does not exist. I think keeping the exclude list is the way to go since we aren't actually excluding anything. I double checked, but we do need to keep filebrowser in the INSTALLED_APPS, so attacking the problem from that side is not viable.
pollardld
left a comment
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 great work! Adding a Comment, as opposed to Approve, bc I mentioned Ryan in a comment. I feel like he may think the comment is resolved if I Approve.
| name = "TEKDB" | ||
| verbose_name = "Records" | ||
|
|
||
| def ready(self): |
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.
Hopefully @rhodges will know better than me.
rhodges
left a comment
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.
Interesting stuff! I have a million questions, but no real red flags.
TEKDB/TEKDB/settings.py
Outdated
|
|
||
| FILEBROWSER_LIST_PER_PAGE = 20 | ||
|
|
||
| FILEBROWSER_EXTENSIONS = { |
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.
I agree with @pollardld that not defining this for ourselves may be for the best, but that depends on the default behavior.
Looks like this is the default: https://django-filebrowser.readthedocs.io/en/latest/settings.html#extensions
The worry is that we cannot anticipate all of the file types we may see. It looks like this limits what people can upload (I'm not sure that's relevant) which I don't like (it'd be great if they could upload other data types, including GIS or metadata [JSON, XML, GeoJSON, SHP, ZIP, etc...]) and I'm hoping this doesn't constrain what files they can manipulate in the view (see above, or any upload 'parts').
Does this impact our use case?
| @@ -0,0 +1,269 @@ | |||
| import os | |||
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 works for now.
In the future, I recommend borrowing the convention used by the library you're overriding. Since this mostly functions to override components and logic from filebrowser.sites, you could call this something like TEKDB/TEKDB/filebrowser/sites.py or filebrowser_sites.py. This is anticipating the potential that we may need to MASSIVELY override multiple components of another tool that are broken up between named files that we cannot anticipate (beyond 'views', 'models', 'admin' or other Django defaults).
| with open(dumpfile_location, "w") as of: | ||
| management.call_command("dumpdata", "--indent=2", stdout=of) | ||
| excludes = getattr(settings, "EXPORT_DUMP_EXCLUDE", []) | ||
| management.call_command( |
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.
Does FileBrowser have any tables in the DB that would be exported? Why would they fail? Why wouldn't we want them captured in the fixture if they are playing a role?
| management.call_command( | ||
| "dumpdata", | ||
| exclude=excludes, | ||
| indent=2, |
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.
Some commands "--indent=2" and indent=2 mean the same thing (depending on how robust the script being called is with handling arguments and flags as if they were the same thing, and frankly I don't understand the difference). Others don't. In your tests, is the 2-character indentation maintained in the dumped fixture file with this change?
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.
Yep, confirmed that that the 2-character indentation is maintained.
| django-tinymce | ||
| pillow | ||
| psycopg2-binary | ||
| django-filebrowser-no-grappelli>=4.0.0,<5.0.0 |
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.
Does this result in the need to run a migration when upgrading ITKDB to this new version (and including FileBrowser)?
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.
I don't think so because FileBrowser is not creating any DB tables
asana task
Description
Adds the following features to the Admin portal:
mediadirectoryScreenshots