From 88ee5b4d289e395825a2829105674f723989160b Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Wed, 21 May 2025 13:38:30 -0600 Subject: [PATCH] fix: resolve feedback comments --- .../export_course_metadata/test_signals.py | 2 +- common/djangoapps/util/storage.py | 47 +++++++++---------- 2 files changed, 24 insertions(+), 25 deletions(-) diff --git a/cms/djangoapps/export_course_metadata/test_signals.py b/cms/djangoapps/export_course_metadata/test_signals.py index 1fc007dd39..67bbfec219 100644 --- a/cms/djangoapps/export_course_metadata/test_signals.py +++ b/cms/djangoapps/export_course_metadata/test_signals.py @@ -78,6 +78,7 @@ class TestExportCourseMetadata(SharedModuleStoreTestCase): self.assertEqual(storage.__class__.__name__, "CourseMetadataExportS3Storage") self.assertEqual(storage.bucket_name, "bucket_name_test") + @override_settings() def test_resolve_storage_with_no_config(self): """ If no storage setup is defined, we get FileSystemStorage by default """ del settings.DEFAULT_FILE_STORAGE @@ -114,7 +115,6 @@ class TestExportCourseMetadata(SharedModuleStoreTestCase): ) def test_resolve_storage_using_django5_settings_with_options(self): """ Ensure we call the storage class with the correct parameters and Django 5 setup """ - del settings.DEFAULT_FILE_STORAGE del settings.COURSE_METADATA_EXPORT_STORAGE del settings.COURSE_METADATA_EXPORT_BUCKET storage = resolve_storage_backend("COURSE_METADATA_EXPORT_STORAGE") diff --git a/common/djangoapps/util/storage.py b/common/djangoapps/util/storage.py index b7bdbdc291..2609c9f26f 100644 --- a/common/djangoapps/util/storage.py +++ b/common/djangoapps/util/storage.py @@ -1,24 +1,28 @@ """ Utility functions related to django storages """ -import django +from typing import Optional from django.conf import settings -from django.core.files.storage import default_storage +from django.core.files.storage import default_storage, storages from django.utils.module_loading import import_string -def resolve_storage_backend(storage_key, options=None): +def resolve_storage_backend(storage_key: str, options: Optional[dict] = None): """ Configures and returns a Django `Storage` instance, compatible with both Django 4 and Django 5. - - Main goal: - Deprecate the use of `django.core.files.storage.get_storage_class`. - How: - Replace `get_storage_class` with direct configuration logic, - ensuring backward compatibility with both Django 4 and Django 5 storage settings. + Params: + storage_key = The key name saved in Django settings. + options = Kwargs for the storage class Returns: An instance of the configured storage backend. Raises: ImportError: If the specified storage class cannot be imported. + + Main goal: + Deprecate the use of `django.core.files.storage.get_storage_class`. + + How: + Replace `get_storage_class` with direct configuration logic, + ensuring backward compatibility with both Django 4 and Django 5 storage settings. """ storage_path = getattr(settings, storage_key, None) @@ -29,33 +33,28 @@ def resolve_storage_backend(storage_key, options=None): if storage_key == "default": # Use case 1: Default storage - # Works consistently across Django 4.2 and 5.x - # In Django 4.2, default_storage uses DEFAULT_FILE_STORAGE - # In Django 5.x, it uses STORAGES['default'] + # Works consistently across Django 4.2 and 5.x. + # In Django 4.2 and above, `default_storage` uses + # either `DEFAULT_FILE_STORAGE` or `STORAGES['default']`. return default_storage - if django.VERSION >= (5, 0) and storage_key in storages_config: - # Use case 2: Django 5+ with STORAGES defined + if storage_key in storages_config: + # Use case 2: STORAGES is defined + # If STORAGES is present, we retrieve it through the storages API # settings.py must define STORAGES like: # STORAGES = { # "default": {"BACKEND": "...", "OPTIONS": {...}}, # "custom": {"BACKEND": "...", "OPTIONS": {...}}, # } # See: https://docs.djangoproject.com/en/5.2/ref/settings/#std-setting-STORAGES - from django.core.files.storage import storages return storages[storage_key] - if not storage_path and storage_key in storages_config: - # Use case 3: Transitional setup - # Running on Django 4.x but using Django 5-style STORAGES config - # Manually load the backend and options - storage_path = storages_config.get(storage_key, {}).get("BACKEND") - options = storages_config.get(storage_key, {}).get("OPTIONS", {}) - if not storage_path: - # if no specific storage was resolved, use the default storage + # Use case 3: No storage settings defined + # If no storage settings are defined anywhere, use the default storage return default_storage - # Fallback to import the storage_path manually + # Use case 4: Import storage_path manually + # Fallback to import the storage_path (Obtained from django settings) manually StorageClass = import_string(storage_path) return StorageClass(**options)