Interruptible uploads for Media and MediaBulkUpload #229
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces interruptible/resumable file upload functionality for Media and MediaBulkUpload in the Django admin panel using the django-resumable-async-upload package. This is part 1 of a two-part implementation.
Changes:
- Replaces the standard Django FileField with AsyncFileField for the Media model's mediafile field
- Updates MediaBulkUpload admin to handle file uploads as comma-separated file path strings instead of file objects
- Adds middleware and admin mixins for orphaned file cleanup
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| docker/docker-compose.yml | Adds volume mount for local django-resumable-async-upload development |
| TEKDB/requirements.txt | Minor formatting cleanup (removed blank lines) |
| TEKDB/entrypoint.sh | Adds conditional installation of django-resumable-async-upload in editable mode |
| TEKDB/Dockerfile | Adds comment about editable package installation at runtime |
| TEKDB/TEKDB/urls.py | Adds URL pattern for admin_async_upload endpoints |
| TEKDB/TEKDB/settings.py | Adds admin_async_upload app and middleware, plus configuration settings |
| TEKDB/TEKDB/models.py | Changes Media.mediafile from FileField to AsyncFileField with max_files=1 |
| TEKDB/TEKDB/forms.py | Replaces MultipleFileField with FormResumableFileField in MediaBulkUploadForm |
| TEKDB/TEKDB/admin.py | Adds AsyncFileCleanupMixin to admin classes and refactors file processing to handle comma-separated file paths |
| TEKDB/TEKDB/tests/test_admin.py | Updates tests to use string file paths instead of SimpleUploadedFile objects |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test_file_paths = [ | ||
| "thumbnail_test_image.jpg, thumbnail_test_video.mp4, thumbnail_test_audio.mp3, thumbnail_test_text.txt, thumbnail_test_thing.shp, thumbnail_test_unknown.xyz" | ||
| ] |
There was a problem hiding this comment.
The test data is a list containing a single string with comma-separated file paths. This is inconsistent with how the data should be structured. Based on the form field definition (FormResumableFileField) and the admin processing logic (line 632 in admin.py), the cleaned_data should be a simple comma-separated string, not a list. This test should use a plain string: test_file_paths = "thumbnail_test_image.jpg,thumbnail_test_video.mp4,..." instead of wrapping it in a list.
| test_file_paths = [ | |
| "thumbnail_test_image.jpg, thumbnail_test_video.mp4, thumbnail_test_audio.mp3, thumbnail_test_text.txt, thumbnail_test_thing.shp, thumbnail_test_unknown.xyz" | |
| ] | |
| test_file_paths = ( | |
| "thumbnail_test_image.jpg,thumbnail_test_video.mp4,thumbnail_test_audio.mp3," | |
| "thumbnail_test_text.txt,thumbnail_test_thing.shp,thumbnail_test_unknown.xyz" | |
| ) |
| if uploaded_file_paths: | ||
| import os | ||
|
|
||
| # split the comma-separated string into a list | ||
| uploaded_file_paths_list = uploaded_file_paths.split(",") | ||
|
|
||
| for uploaded_file_path in uploaded_file_paths_list: | ||
| # Extract just the filename from the path | ||
| file_name = os.path.basename(uploaded_file_path) | ||
|
|
||
| # Guess MIME type from filename | ||
| mime_type, _ = guess_type(file_name) | ||
| if mime_type: | ||
| file_mime_type = mime_type.split("/")[0] | ||
| media_type_instance = LookupMediaType.objects.filter( | ||
| mediatype__startswith=file_mime_type | ||
| ).first() | ||
| else: | ||
| media_type_instance = None | ||
|
|
||
| if not media_type_instance: | ||
| media_type_instance = LookupMediaType.objects.filter( | ||
| mediatype__startswith="other" | ||
| ).first() | ||
|
|
||
| mediatype = media_type_instance | ||
| filename = file.name.split(".")[0] | ||
| filename = file_name.rsplit(".", 1)[0] # Remove extension | ||
|
|
||
| media_instance = Media( | ||
| medianame=filename, | ||
| mediadescription=f'Part of the "{obj.mediabulkname}" Media Bulk Upload that was uploaded on {obj.mediabulkdate}', | ||
| mediafile=file, | ||
| mediatype=mediatype, | ||
| ) | ||
| media_instance.save() | ||
| obj.mediabulkupload.add(media_instance) | ||
|
|
||
| # Add relationships | ||
| if places: | ||
| for place in places: | ||
| PlacesMediaEvents.objects.create( | ||
| placeid=place, mediaid=media_instance | ||
| ) | ||
| if resources: | ||
| for resource in resources: | ||
| ResourcesMediaEvents.objects.create( | ||
| resourceid=resource, mediaid=media_instance | ||
| ) | ||
| if citations: | ||
| for citation in citations: | ||
| MediaCitationEvents.objects.create( | ||
| citationid=citation, mediaid=media_instance | ||
| ) | ||
| if activities: | ||
| for activity in activities: | ||
| ResourceActivityMediaEvents.objects.create( | ||
| resourceactivityid=activity, mediaid=media_instance | ||
| ) | ||
| if placesresources: | ||
| for placeresource in placesresources: | ||
| PlacesResourceMediaEvents.objects.create( | ||
| placeresourceid=placeresource, mediaid=media_instance | ||
| ) | ||
| media_instance = Media( | ||
| medianame=filename, | ||
| mediadescription=f'Part of the "{obj.mediabulkname}" Media Bulk Upload that was uploaded on {obj.mediabulkdate}', | ||
| mediafile=uploaded_file_path, | ||
| mediatype=mediatype, | ||
| ) | ||
| print(f"[DEBUG]: Creating Media instance: {filename}") | ||
| media_instance.save() | ||
| obj.mediabulkupload.add(media_instance) | ||
|
|
||
| # Add relationships | ||
| if places: | ||
| for place in places: | ||
| PlacesMediaEvents.objects.create( | ||
| placeid=place, mediaid=media_instance | ||
| ) | ||
| if resources: | ||
| for resource in resources: | ||
| ResourcesMediaEvents.objects.create( | ||
| resourceid=resource, mediaid=media_instance | ||
| ) | ||
| if citations: | ||
| for citation in citations: | ||
| MediaCitationEvents.objects.create( | ||
| citationid=citation, mediaid=media_instance | ||
| ) | ||
| if activities: | ||
| for activity in activities: | ||
| ResourceActivityMediaEvents.objects.create( | ||
| resourceactivityid=activity, mediaid=media_instance | ||
| ) | ||
| if placesresources: | ||
| for placeresource in placesresources: | ||
| PlacesResourceMediaEvents.objects.create( | ||
| placeresourceid=placeresource, mediaid=media_instance | ||
| ) |
There was a problem hiding this comment.
The entire file upload processing logic lacks error handling. If any step fails (e.g., database save fails, relationship creation fails), the uploaded files may be left in an inconsistent state and the error won't be properly communicated to the user. Consider wrapping the processing logic in a try-except block and using Django's messages framework to communicate errors to the admin user.
| # Install editable package if the directory is mounted | ||
| if [ -d "/django-resumable-async-upload" ]; then | ||
| echo "Installing django-resumable-async-upload in editable mode for local development..." | ||
| pip install -e /django-resumable-async-upload | ||
| else | ||
| echo "django-resumable-async-upload directory not found, skipping editable install" | ||
| fi |
There was a problem hiding this comment.
The editable package installation in entrypoint.sh happens on every container start, which could slow down container startup in production. Consider documenting that this is for development only, or conditionally installing based on an environment variable (e.g., DJANGO_ENV=development).
| mediafile = AsyncFileField( | ||
| db_column="mediafile", | ||
| max_length=255, | ||
| blank=True, | ||
| null=True, | ||
| verbose_name="file", | ||
| max_files=1, |
There was a problem hiding this comment.
The Media.mediafile field has been changed from FileField to AsyncFileField, but there is no corresponding database migration included in this PR. This will cause the application to fail when deployed, as Django will expect the field to match the model definition. Run "python manage.py makemigrations" to generate the required migration file.
| mediafile = AsyncFileField( | |
| db_column="mediafile", | |
| max_length=255, | |
| blank=True, | |
| null=True, | |
| verbose_name="file", | |
| max_files=1, | |
| mediafile = models.FileField( | |
| db_column="mediafile", | |
| max_length=255, | |
| blank=True, | |
| null=True, | |
| verbose_name="file", |
TEKDB/TEKDB/admin.py
Outdated
| mediafile=uploaded_file_path, | ||
| mediatype=mediatype, | ||
| ) | ||
| print(f"[DEBUG]: Creating Media instance: {filename}") |
There was a problem hiding this comment.
Debug print statement should be removed before merging to production. Consider using Django's logging framework (e.g., logger.debug) if debugging information is needed in production.
| # split the comma-separated string into a list | ||
| uploaded_file_paths_list = uploaded_file_paths.split(",") |
There was a problem hiding this comment.
The code doesn't filter out empty strings after splitting, which could occur if the uploaded_file_paths string has trailing commas or consecutive commas. This could cause the loop to process empty paths. Consider filtering empty strings: [path for path in uploaded_file_paths.split(",") if path.strip()].
| # split the comma-separated string into a list | |
| uploaded_file_paths_list = uploaded_file_paths.split(",") | |
| # split the comma-separated string into a list and filter out empty entries | |
| uploaded_file_paths_list = [ | |
| path.strip() for path in uploaded_file_paths.split(",") if path.strip() | |
| ] |
|
|
||
| mediatype = media_type_instance | ||
| filename = file.name.split(".")[0] | ||
| filename = file_name.rsplit(".", 1)[0] # Remove extension |
There was a problem hiding this comment.
The filename extraction using rsplit with maxsplit=1 will fail if the filename has no extension (e.g., "README"). This will result in the full filename being used, which may not be the desired behavior. Consider using os.path.splitext(file_name)[0] instead, which is more robust and handles edge cases like files without extensions.
| filename = file_name.rsplit(".", 1)[0] # Remove extension | |
| filename = os.path.splitext(file_name)[0] # Remove extension |
| # split the comma-separated string into a list | ||
| uploaded_file_paths_list = uploaded_file_paths.split(",") |
There was a problem hiding this comment.
The code doesn't handle potential whitespace around file paths after splitting. If the uploaded_file_paths string contains spaces (e.g., "file1.jpg, file2.jpg"), the paths will include leading/trailing whitespace which could cause issues. Consider using uploaded_file_paths.split(",") followed by stripping whitespace from each path, or use a list comprehension like: [path.strip() for path in uploaded_file_paths.split(",")].
| # split the comma-separated string into a list | |
| uploaded_file_paths_list = uploaded_file_paths.split(",") | |
| # split the comma-separated string into a list and strip whitespace from each path | |
| uploaded_file_paths_list = [ | |
| path.strip() for path in uploaded_file_paths.split(",") if path.strip() | |
| ] |
| media_type_instance = LookupMediaType.objects.filter( | ||
| mediatype__startswith="other" | ||
| ).first() | ||
|
|
There was a problem hiding this comment.
If no media type can be determined (both mime_type lookup and "other" fallback fail), mediatype will be None. This will cause a database constraint error when saving the Media instance if the mediatype field is required. Consider adding error handling or a default value to prevent the save operation from failing silently or with an unclear error.
| # If no media type can be determined, skip creating this Media instance | |
| if not media_type_instance: | |
| print( | |
| f"[ERROR]: Could not determine media type for file '{file_name}'. " | |
| "Skipping Media instance creation." | |
| ) | |
| continue |
| "./test_image.jpg", b"\x00\x00\x00\x00", content_type="image" | ||
| ) | ||
|
|
||
| test_image_paths = "test_image.jpg,test_image.jpg" |
There was a problem hiding this comment.
The test data format has changed from actual file objects to comma-separated string paths, but the tests don't verify that the actual files exist at these paths or that the file upload mechanism works correctly. The tests are now essentially testing string manipulation rather than the file upload functionality. Consider adding proper test fixtures with actual file handling or mocking the async upload mechanism appropriately.
Part 1 of allowing for interruptible/resumable uploads for Media in the Admin panel. Part 2 is in the django-resumable-async-upload repo.
Description
Uses the django-resumable-async-upload package to allow for uploads of media files to be interruptible. Details on how that works are in that packages repo.
The package is used in TEKDB in the following ways:
Mediarecords, uses theAsyncFileFieldmodel instead of theFileFieldMediaadmin form uses theAsyncFileCleanupMixinto clean up orphaned files in the case of a user uploads a file but does not save the record.MediaBulkUploadsadmin form, uses theFormResumableFileFieldandAsyncFileCleanupMixinto allow for resumable uploads, and clean up of orphaned filesmax_filesto be passed to theAsyncFileFieldto determine how many files can be uploaded to a file inputOrphanedFileCleanupMiddlewareto check for orphaned files and delete them if presentTo do:
AsyncFileFieldLinks