From 355da03bdae9ca789a2a2086268c4aa4abc469e4 Mon Sep 17 00:00:00 2001 From: Leonardo Beroes Date: Thu, 15 May 2025 11:00:15 -0400 Subject: [PATCH 1/3] feat: implement submission file workflow with comprehensive testing - Create Submission file model - Add custom storage to use ORA bucket - Develop SubmissionFileProcessor for create files in DB - Add TestSubmissionFileProcessor with extensive test coverage - Verify complete file processing workflow - Validation tests for file addition in create_external_grader_detail - Document architectural decisions with ADR - Add test queue folder in .gitignore --- .gitignore | 1 + .../0002-add-submission-file-model.rst | 77 +++++++++ submissions/api.py | 70 +++++++- submissions/errors.py | 11 ++ submissions/migrations/0005_submissionfile.py | 33 ++++ submissions/models.py | 99 ++++++++++++ submissions/tests/test_api.py | 152 +++++++++++++++++- submissions/tests/test_models.py | 152 +++++++++++++++++- 8 files changed, 589 insertions(+), 6 deletions(-) create mode 100644 docs/source/decisions/0002-add-submission-file-model.rst create mode 100644 submissions/migrations/0005_submissionfile.py diff --git a/.gitignore b/.gitignore index e5e81393..17a77391 100644 --- a/.gitignore +++ b/.gitignore @@ -63,3 +63,4 @@ test.txt.tmp # VSCode .vscode +test_queue/* diff --git a/docs/source/decisions/0002-add-submission-file-model.rst b/docs/source/decisions/0002-add-submission-file-model.rst new file mode 100644 index 00000000..bb6cadee --- /dev/null +++ b/docs/source/decisions/0002-add-submission-file-model.rst @@ -0,0 +1,77 @@ +2. File Handling Implementation for Submission System +##################################################### + +Status +****** + +**Provisional** *2025-02-14* + +Implemented by https://github.com/openedx/edx-submissions/pull/286 + +Context +******* + +As part of the XQueue migration effort detailed in ADR 0001, we need to implement a file handling system within edx-submissions. Currently, XQueue manages file submissions through a tightly coupled approach. + +### Current Limitations + +1. **Inadequate File Management**: XQueue's approach relies on JSON strings in character fields, with size constraints and manual URL manipulation for file handling. + +2. **Process Inefficiencies**: The current system uses synchronous HTTP for file retrieval, lacks proper validation, and tightly couples submission processing with file handling. + +3. **Integration Challenges**: External graders depend on specific URL formats with HTTP-based retrieval, embedding file information directly in submission payloads. + +Decision +******** + +We will implement a dedicated file management system for the assessment submission process, focusing on workflow and educational needs: + +1. **Centralized Storage**: Create a unified repository for student-submitted files, ensuring they are properly associated with their assessments and accessible throughout the grading process. + +2. **Streamlined Workflow**: Design a clear process where files are automatically processed during submission creation, securely stored, and efficiently delivered to grading systems. + +3. **Consistent Experience**: Maintain compatibility with existing systems to ensure a smooth transition, allowing instructors and external graders to access files without changes to their established workflows. + +Consequences +************ + +Positive: +--------- + +1. **Architecture**: Clean separation of concerns, improved maintainability, better error handling, optimized database access + +2. **Integration**: Seamless xqueue-watcher compatibility, support for existing workflows, minimal client code changes + +3. **Operations**: Robust file validation, improved tracking, better error visibility, simplified lifecycle management + +Negative: +--------- + +1. **Technical**: Additional database structures + +2. **Migration**: Temporary system complexity, additional testing needs + +3. **Performance**: File processing overhead + +References +********** + +Current System Documentation: + * XQueue Repository: https://github.com/openedx/xqueue + * XQueue Watcher Repository: https://github.com/openedx/xqueue-watcher + +Related Repositories: + * edx-submissions: https://github.com/openedx/edx-submissions + * edx-platform: https://github.com/openedx/edx-platform + * XQueue Repository: https://github.com/openedx/xqueue + +Related Documentation: + * ADR 0001: Creation of ExternalGraderDetail Model for XQueue Migration + +Future Event Integration: + * Open edX Events Framework: https://github.com/openedx/openedx-events + * Event Bus Documentation: https://openedx.atlassian.net/wiki/spaces/AC/pages/124125264/Event+Bus + +Related Architecture Documents: + * Open edX Architecture Guidelines: https://openedx.atlassian.net/wiki/spaces/AC/pages/124125264/Architecture+Guidelines + diff --git a/submissions/api.py b/submissions/api.py index d5eb4cda..e8075e04 100644 --- a/submissions/api.py +++ b/submissions/api.py @@ -15,6 +15,7 @@ # SubmissionError imported so that code importing this api has access from submissions.errors import ( # pylint: disable=unused-import ExternalGraderQueueEmptyError, + FileProcessingError, SubmissionError, SubmissionInternalError, SubmissionNotFoundError, @@ -28,6 +29,7 @@ ScoreSummary, StudentItem, Submission, + SubmissionFile, score_reset, score_set ) @@ -49,7 +51,6 @@ TOP_SUBMISSIONS_CACHE_TIMEOUT = 300 -# pylint: disable=unused-argument def create_external_grader_detail(student_item_dict, answer, queue_name: str, @@ -93,9 +94,13 @@ def create_external_grader_detail(student_item_dict, submission_uuid=submission_uuid, queue_name=queue_name, grader_file_name=grader_file_name, - points_possible=points_possible, + points_possible=points_possible) + + files_dict = external_grader_additional_data.get('files') + if files_dict: + file_processor = SubmissionFileProcessor(instance) + file_processor.process_files(files_dict) - ) return instance except DatabaseError as error: @@ -106,6 +111,65 @@ def create_external_grader_detail(student_item_dict, raise SubmissionInternalError(error_message) from error +class SubmissionFileProcessor: + """ + Process file operations for submissions + """ + + 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() + } + + def create_submission( student_item_dict, answer, diff --git a/submissions/errors.py b/submissions/errors.py index 38a1cf77..bde0a509 100644 --- a/submissions/errors.py +++ b/submissions/errors.py @@ -40,6 +40,17 @@ 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/migrations/0005_submissionfile.py b/submissions/migrations/0005_submissionfile.py new file mode 100644 index 00000000..ddb5e709 --- /dev/null +++ b/submissions/migrations/0005_submissionfile.py @@ -0,0 +1,33 @@ +# Generated by Django 4.2.19 on 2025-05-15 14:14 + +from django.db import migrations, models +import django.db.models.deletion +import django.utils.timezone +import submissions.models +import uuid + + +class Migration(migrations.Migration): + + dependencies = [ + ('submissions', '0004_externalgraderdetail'), + ] + + operations = [ + migrations.CreateModel( + name='SubmissionFile', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('uuid', models.UUIDField(default=uuid.uuid4, editable=False)), + ('file', models.FileField(max_length=512, storage=submissions.models.get_storage, + upload_to=submissions.models.submission_file_path)), + ('original_filename', models.CharField(max_length=255)), + ('created_at', models.DateTimeField(default=django.utils.timezone.now)), + ('external_grader', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, + related_name='files', to='submissions.externalgraderdetail')), + ], + options={ + 'indexes': [models.Index(fields=['external_grader', 'uuid'], name='submissions_externa_ff8089_idx')], + }, + ), + ] diff --git a/submissions/models.py b/submissions/models.py index 25bbfa0e..aff5c29a 100644 --- a/submissions/models.py +++ b/submissions/models.py @@ -9,12 +9,15 @@ ./manage.py makemigrations submissions """ +import functools import logging +import os from datetime import timedelta from uuid import uuid4 from django.conf import settings from django.contrib import auth +from django.core.files.storage import default_storage from django.db import DatabaseError, models, transaction from django.db.models.signals import post_save, pre_save from django.dispatch import Signal, receiver @@ -693,3 +696,99 @@ def update_status(self, new_status): def create_from_uuid(cls, submission_uuid, **kwargs): submission = Submission.objects.get(uuid=submission_uuid) return cls.objects.create(submission=submission, **kwargs) + + +def submission_file_path(instance, _): + """ + Generate file path for submission files. + Format: queue_name/uuid + The filename is replaced with the UUID to ensure uniqueness without preserving extension. + """ + return os.path.join( + instance.external_grader.queue_name, + f"{instance.uuid}" + ) + + +@functools.cache +def _get_storage_cached(): + """ + Cached implementation to get the configured storage backend. + This function is for internal use only. + """ + edx_submissions_config = getattr(settings, 'EDX_SUBMISSIONS', {}) + storage_config = edx_submissions_config.get('MEDIA') + + if storage_config: + return storage_config + + 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. + + 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 + EDX_SUBMISSIONS = { + 'MEDIA': S3Boto3Storage(bucket_name='my-bucket') + } + + # Then get_storage() will return the S3Boto3Storage instance + """ + return _get_storage_cached() # For performance while keeping this function serializable for migrations + + +class SubmissionFile(models.Model): + """ + Model to handle files associated with submissions + """ + uuid = models.UUIDField(default=uuid4, editable=False) # legacy S3 key + external_grader = models.ForeignKey( + 'submissions.ExternalGraderDetail', + on_delete=models.SET_NULL, + related_name='files', + null=True, + ) + file = models.FileField( + upload_to=submission_file_path, + max_length=512, + storage=get_storage + ) + original_filename = models.CharField(max_length=255) # This is necessary to send file name to xqueue-watcher + created_at = models.DateTimeField(default=now) + + class Meta: + indexes = [ + models.Index(fields=['external_grader', 'uuid']), + ] + + @property + def xqueue_url(self): + """ + Returns a URL in the XQueue-compatible format: /queue_name/uuid + + This format is used for file references in both the legacy XQueue system + and the new integrated standard. It maintains backward compatibility + while supporting the migration from the external XQueue API to the + integrated Open edX solution. + + The URL follows the pattern: /{queue_name}/{submission_uuid} + where: + - queue_name: identifies the external grader queue + - uuid: uniquely identifies this submission (legacy S3 key) + + Returns: + str: Formatted URL path following XQueue conventions + """ + return f"/{self.external_grader.queue_name}/{self.uuid}" diff --git a/submissions/tests/test_api.py b/submissions/tests/test_api.py index a64b659c..f0a7e7ed 100644 --- a/submissions/tests/test_api.py +++ b/submissions/tests/test_api.py @@ -10,6 +10,8 @@ import pytz # Django imports from django.core.cache import cache +from django.core.files.base import ContentFile +from django.core.files.uploadedfile import SimpleUploadedFile from django.db import DatabaseError, IntegrityError, connection, transaction from django.test import TestCase from django.utils.timezone import now @@ -17,8 +19,17 @@ # Local imports from submissions import api +from submissions.api import SubmissionFileProcessor from submissions.errors import ExternalGraderQueueEmptyError, SubmissionInternalError -from submissions.models import ExternalGraderDetail, ScoreAnnotation, ScoreSummary, StudentItem, Submission, score_set +from submissions.models import ( + ExternalGraderDetail, + ScoreAnnotation, + ScoreSummary, + StudentItem, + Submission, + SubmissionFile, + score_set +) from submissions.serializers import StudentItemSerializer STUDENT_ITEM = { @@ -923,3 +934,142 @@ def test_create_multiple_submission_external_grader_details(self): self.assertEqual(submission2.answer, ANSWER_TWO) self.assertNotEqual(external_grader_detail1.submission.uuid, external_grader_detail2.submission.uuid) + + def test_create_external_grader_detail_with_files(self): + """Test creating a queue record with file handling.""" + + class MockFileObj: + def __init__(self, file_content): + self.file = SimpleUploadedFile( + "test.txt", + file_content, + content_type="text/plain" + ) + + test_file = MockFileObj(b"test content") + + event_data = { + 'queue_name': 'test_queue', + 'files': {'test.txt': test_file} + } + + external_grader_detail = api.create_external_grader_detail(STUDENT_ITEM, ANSWER_ONE, **event_data) + + self.assertEqual(external_grader_detail.queue_name, 'test_queue') + self.assertEqual(external_grader_detail.files.count(), 1) + submission_file = external_grader_detail.files.first() + self.assertEqual(submission_file.original_filename, 'test.txt') + + def test_create_external_grader_detail_with_multiple_files(self): + """Test creating a queue record with multiple files.""" + + class MockFileObj: + def __init__(self, filename, content): + self.file = SimpleUploadedFile( + filename, + content, + content_type="text/plain" + ) + + test_files = { + 'test1.txt': MockFileObj("test1.txt", b"test content 1"), + 'test2.txt': MockFileObj("test2.txt", b"test content 2") + } + + external_grader_detail = { + 'queue_name': 'test_queue', + 'files': test_files + } + external_grader_detail = api.create_external_grader_detail(STUDENT_ITEM, ANSWER_ONE, **external_grader_detail) + + self.assertEqual(external_grader_detail.files.count(), 2) + filenames = set(external_grader_detail.files.values_list('original_filename', flat=True)) + self.assertEqual(filenames, {'test1.txt', 'test2.txt'}) + + def test_create_external_grader_detail_without_files(self): + """Test creating a queue record without any files still works.""" + external_grader_instance = api.create_external_grader_detail(STUDENT_ITEM, ANSWER_ONE, queue_name="test_queue") + self.assertEqual(external_grader_instance.queue_name, 'test_queue') + self.assertEqual(external_grader_instance.files.count(), 0) + + +class TestSubmissionFileProcessor(TestCase): + """ + Test the SubmissionFileProcessor functionality. + """ + + 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) + + def test_process_files_with_file_objects(self): + """Test processing files with file objects that have a file attribute.""" + + 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}/")) + + def test_get_files_for_grader(self): + """Test retrieving files in xwatcher format.""" + class FileObjWithFileAttr1: + def __init__(self): + self.file = SimpleUploadedFile("test1.txt", b"content1") + + class FileObjWithFileAttr2: + def __init__(self): + self.file = SimpleUploadedFile("test2.txt", b"content2") + + self.processor.process_files({ + "test1.txt": FileObjWithFileAttr1(), + "test2.txt": FileObjWithFileAttr2() + }) + + grader_files = self.processor.get_files_for_grader() + self.assertEqual(len(grader_files), 2) + self.assertTrue(all(isinstance(url, str) for url in grader_files.values())) + + def test_process_files_complete_flow(self): + """ + Test the complete flow of processing a file through to SubmissionFile creation. + """ + + class FileObjWithFileAttr: + def __init__(self): + self.file = ContentFile(b"test binary content", name="complete_test.txt") + + 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}/")) + + # Verify SubmissionFile was created correctly + submission_file = SubmissionFile.objects.get( + external_grader=self.external_grader_detail, + original_filename="complete_test.txt" + ) + self.assertEqual(submission_file.xqueue_url, file_url) diff --git a/submissions/tests/test_models.py b/submissions/tests/test_models.py index ddce5a0c..448b5802 100644 --- a/submissions/tests/test_models.py +++ b/submissions/tests/test_models.py @@ -7,7 +7,9 @@ import pytest from django.contrib import auth -from django.test import TestCase +from django.core.files.storage import FileSystemStorage +from django.core.files.uploadedfile import SimpleUploadedFile +from django.test import TestCase, override_settings from django.utils.timezone import now from pytz import UTC @@ -20,7 +22,10 @@ ScoreSummary, StudentItem, Submission, - TeamSubmission + SubmissionFile, + TeamSubmission, + _get_storage_cached, + get_storage ) User = auth.get_user_model() @@ -687,3 +692,146 @@ def test_submission_mutability(self): self.assertEqual(re_fetched.answer, new_answer) self.assertEqual(re_fetched.attempt_number, new_attempt) self.assertEqual(re_fetched.submitted_at, new_time) + + +class TestSubmissionFile(TestCase): + """ + Test the SubmissionFile model functionality. + """ + + 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.test_file = SimpleUploadedFile( + "test_file.txt", + b"test content", + content_type="text/plain" + ) + + self.submission_file = SubmissionFile.objects.create( + external_grader=self.external_grader_detail, + file=self.test_file, + original_filename="test_file.txt" + ) + + def test_create_submission_file(self): + """Test basic submission file creation.""" + self.assertIsNotNone(self.submission_file.uuid) + self.assertEqual(self.submission_file.original_filename, "test_file.txt") + self.assertEqual(self.submission_file.external_grader, self.external_grader_detail) + self.assertIsNotNone(self.submission_file.created_at) + + def test_xqueue_url_format(self): + """Test that xqueue_url property returns the correct format.""" + expected_url = f"/{self.external_grader_detail.queue_name}/{self.submission_file.uuid}" + self.assertEqual(self.submission_file.xqueue_url, expected_url) + + def test_multiple_files_per_submission(self): + """Test that multiple files can be associated with a single submission.""" + second_file = SimpleUploadedFile( + "second_file.txt", + b"more content", + content_type="text/plain" + ) + + second_submission_file = SubmissionFile.objects.create( + external_grader=self.external_grader_detail, + file=second_file, + original_filename="second_file.txt" + ) + + files = self.external_grader_detail.files.all() + self.assertEqual(files.count(), 2) + self.assertIn(self.submission_file, files) + self.assertIn(second_submission_file, files) + + def test_file_path_generation(self): + """Test that files are stored with the correct path structure.""" + file_path = self.submission_file.file.name + expected_parts = [ + self.external_grader_detail.queue_name, + str(self.submission_file.uuid) + ] + + for part in expected_parts: + self.assertIn(part, file_path) + + def test_unique_uids(self): + """Test that each file gets a unique UUID.""" + second_file = SimpleUploadedFile( + "another_file.txt", + b"content", + content_type="text/plain" + ) + + second_submission_file = SubmissionFile.objects.create( + external_grader=self.external_grader_detail, + file=second_file, + original_filename="another_file.txt" + ) + + self.assertNotEqual( + self.submission_file.uuid, + second_submission_file.uuid + ) + + def test_related_name_access(self): + """Test accessing files through the submission queue record.""" + files = self.external_grader_detail.files.all() + self.assertEqual(files.count(), 1) + self.assertEqual(files.first(), self.submission_file) + + def test_ordering(self): + """Test that files are ordered by created_at.""" + past_time = now() - timedelta(hours=1) + + older_file = SimpleUploadedFile( + "older_file.txt", + b"old content", + content_type="text/plain" + ) + older_submission_file = SubmissionFile.objects.create( + external_grader=self.external_grader_detail, + file=older_file, + original_filename="older_file.txt", + created_at=past_time + ) + + files = self.external_grader_detail.files.all() + self.assertEqual(files[0], self.submission_file) + self.assertEqual(files[1], older_submission_file) + + def test_get_storage_with_custom_config(self): + """Test that get_storage returns the custom storage from EDX_SUBMISSIONS if configured.""" + + # Create a custom storage for testing + custom_storage = FileSystemStorage(location='/tmp/test_storage') + + # Override settings to include our custom storage + with override_settings(EDX_SUBMISSIONS={'MEDIA': custom_storage}): + + # Clear any existing cache before testing + try: + # If we have direct access to the cached function + _get_storage_cached.cache_clear() + except (ImportError, AttributeError): + pass + + storage = get_storage() + self.assertEqual(storage, custom_storage) + self.assertEqual(storage.location, '/tmp/test_storage') From 108a7a9fe27ff190530b555c243ef0b1f8c57e53 Mon Sep 17 00:00:00 2001 From: Leonardo Beroes Date: Mon, 19 May 2025 08:59:10 -0400 Subject: [PATCH 2/3] feat: Bump version to 3.11.0 for Submission File support The addition of submission file functionality and storage enhancement warrants a minor version bump following semver conventions. --- submissions/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/submissions/__init__.py b/submissions/__init__.py index 9f433d6f..9ac2b64c 100644 --- a/submissions/__init__.py +++ b/submissions/__init__.py @@ -1,2 +1,2 @@ """ API for creating submissions and scores. """ -__version__ = '3.10.1' +__version__ = '3.11.0' From 275b6e7bfd98459708556b9b9119ee71150c3c42 Mon Sep 17 00:00:00 2001 From: Leonardo Beroes Date: Mon, 19 May 2025 09:00:11 -0400 Subject: [PATCH 3/3] feat: Implement submission file system with configurable storage - 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. --- settings.py | 8 ++ submissions/api.py | 91 ++++++++--------------- submissions/errors.py | 11 --- submissions/models.py | 29 ++++++-- submissions/tests/test_api.py | 122 +++++++++++++++++++++---------- submissions/tests/test_models.py | 28 ++----- 6 files changed, 153 insertions(+), 136 deletions(-) diff --git a/settings.py b/settings.py index d718f05c..07ec8b7c 100644 --- 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 index e8075e04..40f46985 100644 --- a/submissions/api.py +++ b/submissions/api.py @@ -15,8 +15,6 @@ # SubmissionError imported so that code importing this api has access from submissions.errors import ( # pylint: disable=unused-import ExternalGraderQueueEmptyError, - FileProcessingError, - SubmissionError, SubmissionInternalError, SubmissionNotFoundError, SubmissionRequestError @@ -59,7 +57,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 +72,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 +111,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 +131,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 index bde0a509..38a1cf77 100644 --- 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 index aff5c29a..78d3cdd4 100644 --- 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,13 +715,21 @@ 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 @@ -728,23 +737,29 @@ def _get_storage_cached(): 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 index f0a7e7ed..58a2b440 100644 --- 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,31 +993,24 @@ 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): @@ -1025,12 +1018,46 @@ def __init__(self): 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 index 448b5802..30ff51c4 100644 --- a/submissions/tests/test_models.py +++ b/submissions/tests/test_models.py @@ -4,12 +4,13 @@ import time from datetime import datetime, timedelta from unittest import mock +from unittest.mock import patch import pytest from django.contrib import auth -from django.core.files.storage import FileSystemStorage +from django.core.files.storage import default_storage from django.core.files.uploadedfile import SimpleUploadedFile -from django.test import TestCase, override_settings +from django.test import TestCase from django.utils.timezone import now from pytz import UTC @@ -816,22 +817,9 @@ def test_ordering(self): self.assertEqual(files[0], self.submission_file) self.assertEqual(files[1], older_submission_file) - def test_get_storage_with_custom_config(self): - """Test that get_storage returns the custom storage from EDX_SUBMISSIONS if configured.""" + def test_get_storage_default_with_mock(self): + """Test that get_storage returns default_storage when no custom configuration exists.""" + _get_storage_cached.cache_clear() - # Create a custom storage for testing - custom_storage = FileSystemStorage(location='/tmp/test_storage') - - # Override settings to include our custom storage - with override_settings(EDX_SUBMISSIONS={'MEDIA': custom_storage}): - - # Clear any existing cache before testing - try: - # If we have direct access to the cached function - _get_storage_cached.cache_clear() - except (ImportError, AttributeError): - pass - - storage = get_storage() - self.assertEqual(storage, custom_storage) - self.assertEqual(storage.location, '/tmp/test_storage') + with patch('submissions.models.getattr', return_value={}): + self.assertEqual(get_storage(), default_storage)