feat: Implement submission file system with configurable storage

leoaulasneo98 · leoaulasneo98 · commit d99a8f2b4505 · 2025-05-19T13:11:20.000-04:00
- Replace SubmissionFileProcessor with direct file handling in create_external_grader_detail
- Implement flexible storage configuration via EDX_SUBMISSIONS['MEDIA'] dictionary
- Support dynamic backend loading with proper Django settings conventions
- Add cached storage implementation with serialization-safe public interface
- Configure InMemoryStorage for tests to ensure reliable test execution
- Update tests to accommodate new storage patterns and configuration
- Ensure backward compatibility with existing file operations

This implementation follows Django best practices for storage configuration
while maintaining performance through caching and avoiding serialization
issues with migrations.
diff --git a/settings.py b/settings.py
@@ -82,3 +82,11 @@
 )
 
 TEST_APPS = ('submissions',)
+
+EDX_SUBMISSIONS = {
+    'MEDIA': {
+        'BACKEND': 'django.core.files.storage.InMemoryStorage',
+        'OPTIONS': {
+        }
+    }
+}
diff --git a/submissions/api.py b/submissions/api.py
@@ -59,7 +59,7 @@ def create_external_grader_detail(student_item_dict,
                                   **external_grader_additional_data
                                   ):
     """
-    Creates a submission and an associated ExternalGraderDetail record.
+    Creates a submission and an associated ExternalGraderDetail record with optional file attachments.
 
     Args:
         student_item_dict (dict): The student_item this submission is associated with.
@@ -74,13 +74,28 @@ def create_external_grader_detail(student_item_dict,
         points_possible (int, optional): The maximum possible points for this submission. Defaults to 1.
 
         external_grader_additional_data: Additional keyword arguments that may be used for the external grader.
+            If a 'files' dictionary is included, files will be processed and stored as SubmissionFile objects.
+            The 'files' dictionary should map filenames to file objects (typically FileObjForWebobFiles).
 
     Returns:
         ExternalGraderDetail: The created external grader detail record that references the submission.
+            Any processed files are accessible through the `files` related_name on this object.
 
     Raises:
         ExternalGraderQueueEmptyError: If queue_name is empty.
         SubmissionInternalError: If there's an error creating the submission or external grader detail.
+
+    Example:
+        >>> files = {'assignment.py': file_obj}
+        >>> external_grader = create_external_grader_detail(
+        ...     student_item_dict,
+        ...     answer,
+        ...     'python_grader',
+        ...     files=files
+        ... )
+        >>> # Access files through external_grader.files.all()
+        >>> # Get URLs for external systems:
+        >>> urls = {f.original_filename: f.xqueue_url for f in external_grader.files.all()}
     """
 
     submission = create_submission(student_item_dict, answer)
@@ -98,8 +113,15 @@ def create_external_grader_detail(student_item_dict,
 
         files_dict = external_grader_additional_data.get('files')
         if files_dict:
-            file_processor = SubmissionFileProcessor(instance)
-            file_processor.process_files(files_dict)
+
+            files_urls = {}
+            for filename, file_obj in files_dict.items():
+                submission_file = SubmissionFile.objects.create(
+                    external_grader=instance,
+                    file=file_obj.file,
+                    original_filename=filename
+                )
+                files_urls[filename] = submission_file.xqueue_url
 
         return instance
 
@@ -111,63 +133,14 @@ def create_external_grader_detail(student_item_dict,
         raise SubmissionInternalError(error_message) from error
 
 
-class SubmissionFileProcessor:
+def get_files_for_grader(external_grader):
     """
-    Process file operations for submissions
+    Returns files in format expected by xqueue-watcher when is calling by get_submission endpoint.
     """
-
-    def __init__(self, external_grader):
-        self.external_grader = external_grader
-
-    def process_files(self, files_dict):
-        """
-        Process uploaded files from an Open edX environment and store them as SubmissionFile objects.
-
-        This method specifically handles native OpenedX FileObjForWebobFiles objects.
-
-        The method performs the following operations:
-        1. Stores each file directly as a SubmissionFile object in the database
-        2. Returns URLs in xqueue-compatible format
-
-        Args:
-            files_dict (dict): Dictionary mapping filenames to file objects.
-                              Format: {filename: file_object, ...}
-
-        Returns:
-            dict: Dictionary mapping original filenames to xqueue URLs.
-                  Format: {filename: "/queue_name/uuid", ...}
-
-        Example:
-            >>> external_grader = ExternalGraderDetail.create_from_uuid(
-            submission_uuid=submission_uuid,
-            queue_name=queue_name,
-            grader_file_name=grader_file_name,
-            points_possible=points_possible)
-            >>> file_processor = SubmissionFileProcessor(external_grader)
-            >>> files = {'assignment.py': file_obj}
-            >>> urls = file_processor.process_files(files)
-            >>> print(urls)
-            {'assignment.py': '/my_queue/550e8400-e29b-41d4-a716-446655440000'}
-        """
-        files_urls = {}
-        for filename, file_obj in files_dict.items():
-            submission_file = SubmissionFile.objects.create(
-                external_grader=self.external_grader,
-                file=file_obj.file,
-                original_filename=filename
-            )
-            files_urls[filename] = submission_file.xqueue_url
-
-        return files_urls
-
-    def get_files_for_grader(self):
-        """
-        Returns files in format expected by xqueue-watcher
-        """
-        return {
-            file.original_filename: file.file.url
-            for file in self.external_grader.files.all()
-        }
+    return {
+        file.original_filename: file.file.url
+        for file in external_grader.files.all()
+    }
 
 
 def create_submission(
diff --git a/submissions/errors.py b/submissions/errors.py
@@ -40,17 +40,6 @@ class ExternalGraderQueueEmptyError(SubmissionError):
     """
 
 
-class FileProcessingError(SubmissionError):
-    """
-    Exception raised when there's an error reading or processing a file.
-
-    This exception is raised when file operations fail, such as:
-    - I/O errors when reading file content
-    - OS errors during file operations
-    - Unicode decoding errors when processing file content
-    """
-
-
 class SubmissionRequestError(SubmissionError):
     """
     This error is raised when there was a request-specific error
diff --git a/submissions/models.py b/submissions/models.py
@@ -21,6 +21,7 @@
 from django.db import DatabaseError, models, transaction
 from django.db.models.signals import post_save, pre_save
 from django.dispatch import Signal, receiver
+from django.utils.module_loading import import_string
 from django.utils.timezone import now
 from jsonfield import JSONField
 from model_utils.models import TimeStampedModel
@@ -714,37 +715,51 @@ def submission_file_path(instance, _):
 def _get_storage_cached():
     """
     Cached implementation to get the configured storage backend.
-    This function is for internal use only.
+
+    This private function loads storage configuration from settings and
+    dynamically instantiates the specified storage backend. It expects
+    EDX_SUBMISSIONS['MEDIA'] to be a dict with 'BACKEND' (string path to
+    storage class) and optional 'OPTIONS' (dict of parameters).
+
+    This function is for internal use only and is cached to improve performance.
     """
     edx_submissions_config = getattr(settings, 'EDX_SUBMISSIONS', {})
     storage_config = edx_submissions_config.get('MEDIA')
 
     if storage_config:
-        return storage_config
+        storage_cls = import_string(storage_config['BACKEND'])
+        options = storage_config.get('OPTIONS', {})
+        return storage_cls(**options)
 
     return default_storage
 
 
 def get_storage():
     """
     Get the configured storage backend or fallback to default storage.
-    Private helper with caching to avoid Django migration serialization errors.
 
     This function checks for a storage configuration in the Django settings.
     It first looks for 'MEDIA' in the 'EDX_SUBMISSIONS' configuration dictionary.
 
+    The function uses an internal cached implementation while remaining
+    serializable for Django migrations, avoiding "Cannot serialize" errors.
+
     Returns:
         Storage instance: Returns the configured storage if found in EDX_SUBMISSIONS['MEDIA'],
                          otherwise returns Django's default_storage.
 
     Example:
-        # In settings
-        from storages.backends.s3boto3 import S3Boto3Storage
+        # In settings.py
         EDX_SUBMISSIONS = {
-            'MEDIA': S3Boto3Storage(bucket_name='my-bucket')
+            'MEDIA': {
+                'BACKEND': 'storages.backends.s3boto3.S3Boto3Storage',
+                'OPTIONS': {
+                    'bucket_name': 'my-bucket'
+                }
+            }
         }
 
-        # Then get_storage() will return the S3Boto3Storage instance
+        # Then get_storage() will return an S3Boto3Storage instance
     """
     return _get_storage_cached()  # For performance while keeping this function serializable for migrations
 
diff --git a/submissions/tests/test_api.py b/submissions/tests/test_api.py
@@ -19,7 +19,7 @@
 
 # Local imports
 from submissions import api
-from submissions.api import SubmissionFileProcessor
+from submissions.api import create_external_grader_detail, get_files_for_grader
 from submissions.errors import ExternalGraderQueueEmptyError, SubmissionInternalError
 from submissions.models import (
     ExternalGraderDetail,
@@ -993,44 +993,71 @@ def test_create_external_grader_detail_without_files(self):
         self.assertEqual(external_grader_instance.files.count(), 0)
 
 
-class TestSubmissionFileProcessor(TestCase):
+class TestExternalGraderFileProcessing(TestCase):
     """
-    Test the SubmissionFileProcessor functionality.
+    Test the file processing functionality for external graders.
     """
 
     def setUp(self):
         """Set up common test data."""
-        self.student_item = StudentItem.objects.create(
-            student_id="test_student",
-            course_id="test_course",
-            item_id="test_item"
-        )
-        self.submission = Submission.objects.create(
-            student_item=self.student_item,
-            answer="test answer",
-            attempt_number=1
-        )
-        self.external_grader_detail = ExternalGraderDetail.objects.create(
-            submission=self.submission,
-            queue_name="test_queue"
-        )
-        self.processor = SubmissionFileProcessor(self.external_grader_detail)
+        self.student_item_dict = {
+            "student_id": "test_student",
+            "course_id": "test_course",
+            "item_id": "test_item",
+            "item_type": "test_type"
+        }
+        self.answer = "test answer"
+        self.queue_name = "test_queue"
 
-    def test_process_files_with_file_objects(self):
-        """Test processing files with file objects that have a file attribute."""
+    def test_create_external_grader_with_files(self):
+        """Test processing files within create_external_grader_detail."""
 
         class FileObjWithFileAttr:
             def __init__(self):
                 self.file = SimpleUploadedFile("test.txt", b"test content")
 
         file_obj = FileObjWithFileAttr()
         files_dict = {"test.txt": file_obj}
-        urls = self.processor.process_files(files_dict)
-        self.assertEqual(len(urls), 1)
-        self.assertTrue(urls["test.txt"].startswith(f"/{self.external_grader_detail.queue_name}/"))
+
+        # Create external grader with files
+        instance = create_external_grader_detail(
+            student_item_dict=self.student_item_dict,
+            answer=self.answer,
+            queue_name=self.queue_name,
+            files=files_dict
+        )
+
+        # Verify SubmissionFile was created correctly
+        submission_file = SubmissionFile.objects.get(
+            external_grader=instance,
+            original_filename="test.txt"
+        )
+        self.assertTrue(submission_file.xqueue_url.startswith(f"/{self.queue_name}/"))
 
     def test_get_files_for_grader(self):
-        """Test retrieving files in xwatcher format."""
+        """Test the standalone get_files_for_grader function."""
+
+        class FileObjWithFileAttr:
+            def __init__(self):
+                self.file = SimpleUploadedFile("test.txt", b"test content")
+
+        # Create external grader with a file
+        instance = create_external_grader_detail(
+            student_item_dict=self.student_item_dict,
+            answer=self.answer,
+            queue_name=self.queue_name,
+            files={"test.txt": FileObjWithFileAttr()}
+        )
+
+        # Test get_files_for_grader function
+        grader_files = get_files_for_grader(instance)
+        self.assertEqual(len(grader_files), 1)
+        self.assertIn("test.txt", grader_files)
+        self.assertTrue(isinstance(grader_files["test.txt"], str))
+
+    def test_multiple_files_processing(self):
+        """Test processing multiple files through create_external_grader_detail."""
+
         class FileObjWithFileAttr1:
             def __init__(self):
                 self.file = SimpleUploadedFile("test1.txt", b"content1")
@@ -1039,16 +1066,30 @@ class FileObjWithFileAttr2:
             def __init__(self):
                 self.file = SimpleUploadedFile("test2.txt", b"content2")
 
-        self.processor.process_files({
+        files_dict = {
             "test1.txt": FileObjWithFileAttr1(),
             "test2.txt": FileObjWithFileAttr2()
-        })
+        }
 
-        grader_files = self.processor.get_files_for_grader()
+        # Create external grader with multiple files
+        instance = create_external_grader_detail(
+            student_item_dict=self.student_item_dict,
+            answer=self.answer,
+            queue_name=self.queue_name,
+            files=files_dict
+        )
+
+        # Verify both files were created
+        submission_files = SubmissionFile.objects.filter(external_grader=instance)
+        self.assertEqual(submission_files.count(), 2)
+
+        # Test get_files_for_grader
+        grader_files = get_files_for_grader(instance)
         self.assertEqual(len(grader_files), 2)
-        self.assertTrue(all(isinstance(url, str) for url in grader_files.values()))
+        self.assertIn("test1.txt", grader_files)
+        self.assertIn("test2.txt", grader_files)
 
-    def test_process_files_complete_flow(self):
+    def test_complete_file_processing_flow(self):
         """
         Test the complete flow of processing a file through to SubmissionFile creation.
         """
@@ -1059,17 +1100,22 @@ def __init__(self):
 
         files_dict = {"complete_test.txt": FileObjWithFileAttr()}
 
-        # Process the file
-        urls = self.processor.process_files(files_dict)
-
-        # Verify URL was created
-        self.assertEqual(len(urls), 1)
-        file_url = urls["complete_test.txt"]
-        self.assertTrue(file_url.startswith(f"/{self.external_grader_detail.queue_name}/"))
+        # Create external grader with file
+        instance = create_external_grader_detail(
+            student_item_dict=self.student_item_dict,
+            answer=self.answer,
+            queue_name=self.queue_name,
+            files=files_dict
+        )
 
         # Verify SubmissionFile was created correctly
         submission_file = SubmissionFile.objects.get(
-            external_grader=self.external_grader_detail,
+            external_grader=instance,
             original_filename="complete_test.txt"
         )
-        self.assertEqual(submission_file.xqueue_url, file_url)
+
+        # Verify URL format
+        self.assertTrue(submission_file.xqueue_url.startswith(f"/{self.queue_name}/"))
+
+        # Verify file content is accessible
+        self.assertEqual(submission_file.file.read(), b"test binary content")
diff --git a/submissions/tests/test_models.py b/submissions/tests/test_models.py
