feat!: upgrade code and fix get_storage_class ( compatibility django42 and django52) #36628

awais786 · 2025-04-29T15:19:25Z

Fixing breaking change for django52.

Verify the image functionality on sandbox.

https://app.pr-36628-139931.sandboxes.opencraft.hosting/profile/u/openedx
opened/openedx

Replaced usage of `get_storage_class`, which was deprecated in Django 4.2 and removed in 5.1, with `import_string` to support dynamic storage class loading.

musanaeem · 2025-04-30T07:54:21Z

openedx/core/djangoapps/user_api/accounts/image_helpers.py

+    storage_class_path = config.get('class')
+    storage_class = import_string(storage_class_path)


Unsure if this case could ever occur but if the contents of storage_class_path are incorrect, there could be an error which we should catch e.g

try: storage_class = import_string(storage_class_path) except ImportError as e: raise ImproperlyConfigured( f"Could not import storage class '{storage_class_path}': {e}" )

https://github.com/carltongibson/django/blob/5b1099323288b15a4c23cba88eb4dcdfa824b3d4/django/core/files/storage/__init__.py#L37C5-L37C71

musanaeem · 2025-04-30T07:54:45Z

LGTM!

hinakhadim · 2025-04-30T09:50:36Z

openedx/core/djangoapps/user_api/accounts/image_helpers.py

+    storage_class_path = config.get('class')
+    storage_class = import_string(storage_class_path)
+    return storage_class(**config.get('options', {}))


Why are not we using the below one?

from django.core.files.storage import storages storage_class = storages(config.get('class')) return storage_class(**config.get('options', {}))

https://github.com/carltongibson/django/blob/5b1099323288b15a4c23cba88eb4dcdfa824b3d4/django/core/files/storage/__init__.py#L37C5-L37C71

awais786 · 2025-05-13T13:36:36Z

@feanil Please review

feanil · 2025-05-14T15:15:45Z

openedx/core/djangoapps/user_api/accounts/image_helpers.py

+    if not storage_class_path:
+        storage_class_path = (
+            getattr(settings, 'DEFAULT_FILE_STORAGE', None) or
+            getattr(settings, 'STORAGES', {}).get('default', {}).get('BACKEND') or
+            'django.core.files.storage.FileSystemStorage'
+        )


Before we fallback to the default storage class, I think we should also have a named storage class in the STORAGES dictionary. Maybe named profile_images?

This way, operators could migrate to defining their various storages config via the new STORAGES config and we can eventually deprecate the old configuration settings.

awais786 · 2025-05-15T10:26:51Z

@feanil I have verified on django42 this solution will not work. These settings cannot declare together.

STORAGES = {
    "profile_images": {
        "BACKEND": "openedx.core.storage.OverwriteStorage",
        "OPTIONS": {
            "location": "profile-images/",
            "base_url": "profile-images/",
        },
    },
    'default': {
        'BACKEND': 'django.core.files.storage.FileSystemStorage'
    }
}

DEFAULT_FILE_STORAGE = 'django.core.files.storage.FileSystemStorage'

These cannot declared together. It raises error

django.core.exceptions.ImproperlyConfigured: DEFAULT_FILE_STORAGE/STORAGES are mutually exclusive.

Django code reference

Right now DEFAULT_FILE_STORAGE is declared at multiple places and this change will increase the scope of this PR. We can add followup ticket for cleaning purpose. openedx/public-engineering#369

awais786 · 2025-05-16T20:15:10Z

@feanil its ready for final review.

awais786 · 2025-05-19T17:00:17Z

@feanil its ready.

Copilot

Pull Request Overview

This PR refactors the profile image storage helper to support both the existing PROFILE_IMAGE_BACKEND setting and the new Django 5+ STORAGES configuration, and adds tests in test_views.py to validate each fallback path.

Migrate from get_storage_class to import_string and explicit resolution order for storage backends.
Introduce support for the new STORAGES setting (with profile_image and default keys) before falling back to DEFAULT_FILE_STORAGE or FileSystemStorage.
Add comprehensive tests in test_views.py to verify default, parameterized, STORAGES-based, and hardcoded fallback behaviors.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
openedx/core/djangoapps/user_api/accounts/tests/test_views.py	New tests covering `get_profile_image_storage` under various `PROFILE_IMAGE_BACKEND` and `STORAGES` scenarios
openedx/core/djangoapps/user_api/accounts/image_helpers.py	Refactored `get_profile_image_storage` to use `import_string`, support Django 5+ `STORAGES`, and drop `get_storage_class`

Comments suppressed due to low confidence (1)

openedx/core/djangoapps/user_api/accounts/tests/test_views.py:1173

[nitpick] Test method names are inconsistent: the first test is test_profile_image_backend, but subsequent tests use test_profile_backend_*. For clarity, rename them to test_profile_image_backend_with_params, test_profile_image_backend_without_backend, etc.

@override_settings(PROFILE_IMAGE_BACKEND={

Copilot · 2025-06-02T17:12:18Z

openedx/core/djangoapps/user_api/accounts/image_helpers.py

@@ -8,10 +8,11 @@
 from django.conf import settings
 from django.contrib.staticfiles.storage import staticfiles_storage
 from django.core.exceptions import ObjectDoesNotExist
-from django.core.files.storage import get_storage_class
+from django.core.files.storage import storages


Importing storages directly will break on Django versions before 5.0, where this attribute doesn't exist. Wrap this import in a try/except ImportError and fall back to a compatible approach (e.g., using import_string or the legacy get_storage_class) for Django<5.0.

This comment is wrong, django had the storages import available in 4.2, it just drops the get_storage_class function in 5.0 which is what we're trying to fix here.

feanil

This looks good, just one more change and then I think it's good to merge.

FYI, I've added a note to the Ulmo release page for operators so they know about the new storage class name and can switch to it after Ulmo lands so we can remove support for the conversion from the old way to the new way.

@awais786 once you've made the final change, I don't need to re-review it if it looks good to you and we have test coverage testing the priority order is working correctly.

feanil · 2025-06-02T17:52:17Z

openedx/core/djangoapps/user_api/accounts/image_helpers.py

+
+    1. If the `PROFILE_IMAGE_BACKEND` setting is defined and includes a `'class'` key,
+       it uses the specified storage backend and options.
+    2. If not, it checks the Django 5+ `STORAGES` setting for a named storage called


This should be the highest priority option, we should also make the newest way to do things the highest priority, falling back to the older ways for compatibility. This makes it easier to read and also easier to cleanup in the future when we want to drop all the old ways of doing this.

open-craft-grove · 2025-06-04T01:01:55Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2025-06-04T22:07:31Z

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2025-06-05T13:10:16Z

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

edx-pipeline-bot · 2025-06-05T15:49:24Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2025-06-05T16:11:40Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2025-06-05T16:11:40Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2025-06-05T16:19:03Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2025-06-05T16:42:32Z

2U Release Notice: This PR has been deployed to the edX production environment.

feat!: upgrade codebase for compatibility with Django 4.2 and 5.2

567c1ca

Replaced usage of `get_storage_class`, which was deprecated in Django 4.2 and removed in 5.1, with `import_string` to support dynamic storage class loading.

awais786 changed the title ~~feat!: upgrade codebase for compatibility with Django 4.2 and 5.2~~ feat!: upgrade code and fix get_storage_class ( compatibility django42 and django52) Apr 29, 2025

musanaeem reviewed Apr 30, 2025

View reviewed changes

hinakhadim reviewed Apr 30, 2025

View reviewed changes

awais786 added 8 commits May 2, 2025 18:11

fix: updating code.

1a76237

fix: updating code.

ef4fc25

fix: adding tests.

7e27670

fix: adding tests.

5b70ad8

fix: adding tests.

8ba4bb1

fix: Update image_helpers.py

1075cc8

fix: fixing code.

05bd1d3

fix: fixing code.

03e8c83

awais786 force-pushed the fixing-django-storages-dj52 branch from 2ffa43e to 03e8c83 Compare May 6, 2025 11:17

fix: fixing code.

3f30323

awais786 mentioned this pull request May 9, 2025

upgrade get_storage_class method in profile image in edx-platform openedx/public-engineering#364

Open

awais786 added 4 commits May 13, 2025 15:43

Merge branch 'master' into fixing-django-storages-dj52

d9ec046

chore: fix quality.

b9d27be

chore: Update image_helpers.py

58ce3de

chore: Update image_helpers.py

04e341d

irtazaakram approved these changes May 13, 2025

View reviewed changes

awais786 marked this pull request as ready for review May 13, 2025 16:20

awais786 requested a review from a team as a code owner May 13, 2025 16:20

feanil reviewed May 14, 2025

View reviewed changes

awais786 added 4 commits May 15, 2025 17:03

chore: Update image_helpers.py

54d3f24

chore: Update image_helpers.py

1561a84

chore: adding future settings options.

18bfa4c

chore: fixing tests.

a7790f2

Merge branch 'master' into fixing-django-storages-dj52

8a36323

Merge branch 'master' into fixing-django-storages-dj52

5893808

Merge branch 'master' into fixing-django-storages-dj52

07ac8cb

feanil requested a review from Copilot June 2, 2025 17:10

Copilot AI reviewed Jun 2, 2025

View reviewed changes

feanil reviewed Jun 2, 2025

View reviewed changes

awais786 added 2 commits June 2, 2025 16:21

feat!: changing the order to pick the storage backend.

9cbee76

Merge branch 'master' into fixing-django-storages-dj52

33122fa

awais786 added the create-sandbox open-craft-grove should create a sandbox environment from this PR label Jun 3, 2025

feanil approved these changes Jun 4, 2025

View reviewed changes

Merge branch 'master' into fixing-django-storages-dj52

defa634

Merge branch 'master' into fixing-django-storages-dj52

3f5555b

Merge branch 'master' into fixing-django-storages-dj52

9ab0900

awais786 merged commit 291c5c3 into master Jun 5, 2025
49 checks passed

awais786 deleted the fixing-django-storages-dj52 branch June 5, 2025 14:11

		storage_class_path = config.get('class')
		storage_class = import_string(storage_class_path)

feat!: upgrade code and fix get_storage_class ( compatibility django42 and django52) #36628

feat!: upgrade code and fix get_storage_class ( compatibility django42 and django52) #36628

Conversation

awais786 commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

musanaeem Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

awais786 May 2, 2025

Choose a reason for hiding this comment

Uh oh!

musanaeem commented Apr 30, 2025

Uh oh!

hinakhadim Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

awais786 May 2, 2025

Choose a reason for hiding this comment

Uh oh!

awais786 commented May 13, 2025

Uh oh!

feanil May 14, 2025

Choose a reason for hiding this comment

Uh oh!

awais786 commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

awais786 commented May 16, 2025

Uh oh!

awais786 commented May 19, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

feanil Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

feanil left a comment

Choose a reason for hiding this comment

Uh oh!

feanil Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

open-craft-grove commented Jun 4, 2025

Uh oh!

open-craft-grove commented Jun 4, 2025

Uh oh!

open-craft-grove commented Jun 5, 2025

Uh oh!

Uh oh!

edx-pipeline-bot commented Jun 5, 2025

Uh oh!

edx-pipeline-bot commented Jun 5, 2025

Uh oh!

edx-pipeline-bot commented Jun 5, 2025

Uh oh!

edx-pipeline-bot commented Jun 5, 2025

Uh oh!

edx-pipeline-bot commented Jun 5, 2025

Uh oh!

Uh oh!

awais786 commented Apr 29, 2025 •

edited

Loading

awais786 commented May 15, 2025 •

edited

Loading