Skip to content
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
567c1ca
feat!: upgrade codebase for compatibility with Django 4.2 and 5.2
awais786 Apr 29, 2025
1a76237
fix: updating code.
awais786 May 2, 2025
ef4fc25
fix: updating code.
awais786 May 2, 2025
7e27670
fix: adding tests.
awais786 May 3, 2025
5b70ad8
fix: adding tests.
awais786 May 3, 2025
8ba4bb1
fix: adding tests.
awais786 May 3, 2025
1075cc8
fix: Update image_helpers.py
awais786 May 6, 2025
05bd1d3
fix: fixing code.
awais786 May 6, 2025
03e8c83
fix: fixing code.
awais786 May 6, 2025
3f30323
fix: fixing code.
awais786 May 8, 2025
d9ec046
Merge branch 'master' into fixing-django-storages-dj52
awais786 May 13, 2025
b9d27be
chore: fix quality.
awais786 May 13, 2025
58ce3de
chore: Update image_helpers.py
awais786 May 13, 2025
04e341d
chore: Update image_helpers.py
awais786 May 13, 2025
54d3f24
chore: Update image_helpers.py
awais786 May 15, 2025
1561a84
chore: Update image_helpers.py
awais786 May 15, 2025
18bfa4c
chore: adding future settings options.
awais786 May 16, 2025
a7790f2
chore: fixing tests.
awais786 May 16, 2025
2918ecd
chore: Update image_helpers.py
awais786 May 16, 2025
df11963
chore: Update test_views.py
awais786 May 16, 2025
a8e5621
chore: fixing code.
awais786 May 16, 2025
3cc0856
chore: fixing code.
awais786 May 16, 2025
8a36323
Merge branch 'master' into fixing-django-storages-dj52
awais786 May 16, 2025
5893808
Merge branch 'master' into fixing-django-storages-dj52
awais786 May 19, 2025
07ac8cb
Merge branch 'master' into fixing-django-storages-dj52
awais786 May 21, 2025
9cbee76
feat!: changing the order to pick the storage backend.
awais786 Jun 2, 2025
33122fa
Merge branch 'master' into fixing-django-storages-dj52
awais786 Jun 2, 2025
defa634
Merge branch 'master' into fixing-django-storages-dj52
awais786 Jun 4, 2025
3f5555b
Merge branch 'master' into fixing-django-storages-dj52
awais786 Jun 5, 2025
9ab0900
Merge branch 'master' into fixing-django-storages-dj52
awais786 Jun 5, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 39 additions & 8 deletions openedx/core/djangoapps/user_api/accounts/image_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copilot AI Jun 2, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

from django.utils.module_loading import import_string

from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers
from common.djangoapps.student.models import UserProfile
from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers

from ..errors import UserNotFound

Expand All @@ -22,12 +23,42 @@

def get_profile_image_storage():
"""
Configures and returns a django Storage instance that can be used
to physically locate, read and write profile images.
"""
config = settings.PROFILE_IMAGE_BACKEND
storage_class = get_storage_class(config['class'])
return storage_class(**config['options'])
Returns an instance of the configured storage backend for profile images.

This function prioritizes different settings in the following order to determine
which storage class to use:

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

`'profile_image'` and uses its configured backend.
3. If that is also unavailable, it falls back to the `'default'` storage defined in `STORAGES`.
4. Finally, if no STORAGES are configured, it uses `DEFAULT_FILE_STORAGE` (legacy setting),
or falls back to `'django.core.files.storage.FileSystemStorage'` as the last resort.

Returns:
An instance of the configured storage backend for handling profile images.

Raises:
ImportError: If the specified storage class cannot be imported.
"""
config = getattr(settings, 'PROFILE_IMAGE_BACKEND', {})
storage_class_path = config.get('class')
options = config.get('options', {})
if not storage_class_path:
storages_config = getattr(settings, 'STORAGES', {})

if 'profile_image' in storages_config:
return storages['profile_image']
elif 'default' in storages_config:
return storages['default']
else:
storage_class_path = (getattr(settings, 'DEFAULT_FILE_STORAGE', None) or
'django.core.files.storage.FileSystemStorage')
options = {}

storage_class = import_string(storage_class_path)
Comment on lines +39 to +60

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}" )

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return storage_class(**options)


def _make_profile_image_name(username):
Expand Down
65 changes: 65 additions & 0 deletions openedx/core/djangoapps/user_api/accounts/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,13 @@
import ddt
import pytz
from django.conf import settings
from django.core.files.storage import FileSystemStorage
from django.test.testcases import TransactionTestCase
from django.test.utils import override_settings
from django.urls import reverse
from rest_framework import status
from rest_framework.test import APIClient, APITestCase
from storages.backends.s3boto3 import S3Boto3Storage

from common.djangoapps.student.models import PendingEmailChange, UserProfile
from common.djangoapps.student.models_api import do_name_change_request, get_pending_name_change
Expand All @@ -33,6 +35,7 @@
RetirementStateFactory,
UserRetirementStatusFactory
)
from openedx.core.djangoapps.user_api.accounts.image_helpers import get_profile_image_storage
from openedx.core.djangoapps.user_api.models import UserPreference, UserRetirementStatus
from openedx.core.djangoapps.user_api.preferences.api import set_user_preference
from openedx.core.djangoapps.waffle_utils.testutils import WAFFLE_TABLES
Expand Down Expand Up @@ -1156,6 +1159,37 @@ def test_patch_serializer_save_fails(self, serializer_save):
assert "Error thrown when saving account updates: 'bummer'" == error_response.data['developer_message']
assert error_response.data['user_message'] is None

def test_profile_image_backend(self):
# settings file contains the `VIDEO_IMAGE_SETTINGS` but dont'have STORAGE_CLASS
# so it returns the default storage.
storage = get_profile_image_storage()
storage_class = storage.__class__
self.assertEqual(
settings.PROFILE_IMAGE_BACKEND['class'],
f"{storage_class.__module__}.{storage_class.__name__}",
)
self.assertEqual(storage.base_url, settings.PROFILE_IMAGE_BACKEND['options']['base_url'])

@override_settings(PROFILE_IMAGE_BACKEND={
'class': 'storages.backends.s3boto3.S3Boto3Storage',
'options': {
'bucket_name': 'test',
'default_acl': 'public',
'location': 'abc/def'
}
})
def test_profile_backend_with_params(self):
storage = get_profile_image_storage()
self.assertIsInstance(storage, S3Boto3Storage)
self.assertEqual(storage.bucket_name, "test")
self.assertEqual(storage.default_acl, 'public')
self.assertEqual(storage.location, "abc/def")

@override_settings(PROFILE_IMAGE_BACKEND={'class': None, 'options': {}})
def test_profile_backend_without_backend(self):
storage = get_profile_image_storage()
self.assertIsInstance(storage, FileSystemStorage)

@override_settings(PROFILE_IMAGE_BACKEND=TEST_PROFILE_IMAGE_BACKEND)
def test_convert_relative_profile_url(self):
"""
Expand All @@ -1170,6 +1204,37 @@ def test_convert_relative_profile_url(self):
'image_url_full': 'http://testserver/static/default_50.png',
'image_url_small': 'http://testserver/static/default_10.png'}

@override_settings(
PROFILE_IMAGE_BACKEND={},
STORAGES={
'profile_image': {
'BACKEND': 'storages.backends.s3boto3.S3Boto3Storage',
'OPTIONS': {
'bucket_name': 'profiles',
'default_acl': 'public',
'location': 'profile/images',
}
}
}
)
def test_profile_backend_with_profile_image_settings(self):
""" It will use the storages dict with profile_images backend"""
storage = get_profile_image_storage()
self.assertIsInstance(storage, S3Boto3Storage)
self.assertEqual(storage.bucket_name, "profiles")
self.assertEqual(storage.default_acl, 'public')
self.assertEqual(storage.location, "profile/images")

@override_settings(
PROFILE_IMAGE_BACKEND={},
)
def test_profile_backend_with_default_hardcoded_backend(self):
""" In case of empty storages scenario uses the hardcoded backend."""
del settings.DEFAULT_FILE_STORAGE
del settings.STORAGES
storage = get_profile_image_storage()
self.assertIsInstance(storage, FileSystemStorage)

@ddt.data(
("client", "user", True),
("different_client", "different_user", False),
Expand Down