-
Notifications
You must be signed in to change notification settings - Fork 81
Feat: Add support for django5.2 #2773
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
f5c49bb to
4a4a551
Compare
77c5b15 to
4a3b27a
Compare
credentials/settings/test.py
Outdated
| # Local Directories | ||
| TEST_ROOT = path("test_root") | ||
| DEFAULT_FILE_STORAGE = "django.core.files.storage.FileSystemStorage" | ||
| STORAGES = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will effect django42 behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DEFAULT_FILE_STORAGE can't be defined with STORAGES dict, so the only option left is to go with the format of django52 (STORAGES {}) and search for any breakage in code and update to STORAGES['default']['BACKEND'] from this DEFAULT_FILE_STORAGE.
Also same for STATICFILES_STORAGE
| serializer = UserGradeSerializer(context={"request": Request(site=self.site)}) | ||
| updated_at_dt = datetime.now() | ||
| updated_at_utc = updated_at_dt.replace(tzinfo=pytz.UTC) | ||
| updated_at_utc = updated_at_dt.replace(tzinfo=ZoneInfo("UTC")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from django.utils.timezone import datetime, timezone
datetime.now(timezone.utc)
requirements/base.in
Outdated
| pillow | ||
| pygments | ||
| python-memcached | ||
| python-slugify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this new package require here ?
0f4556e to
2ab3cf0
Compare
.github/workflows/ci.yml
Outdated
| make tests | ||
| - name: Run code coverage | ||
| if: matrix.python-version == '3.12' && matrix.django-version == 'django42' | ||
| if: matrix.python-version == '3.12' && matrix.django-version == 'django52' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use django42
ca0d71b to
d9ea704
Compare
credentials/settings/base.py
Outdated
| EXTRA_APPS = [] | ||
| SESSION_EXPIRE_AT_BROWSER_CLOSE = False | ||
| STATICFILES_STORAGE = "django.contrib.staticfiles.storage.ManifestStaticFilesStorage" | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should match the existing value here "django.contrib.staticfiles.storage.ManifestStaticFilesStorage"
d9ea704 to
66ca851
Compare
|
@feanil Please review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds compatibility for Django 5.2 alongside existing Django 4.2 support by updating CI/tox configurations, code syntax, and tests.
- Expanded CI and tox environments to include Django 5.2
- Applied
django-upgradesyntax changes (e.g.,re_path→path,@admin.display) - Refactored tests and code to remove deprecated settings and switch to timezone‐aware imports
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tox.ini | Added django52 to envlist and deps |
| credentials/urls.py | Swapped re_path for path in badge routes |
| credentials/settings/base.py | Removed deprecated USE_DEPRECATED_PYTZ and USE_L10N |
| credentials/apps/catalog/tests/factories.py | Switched to django.utils.timezone imports and added skip_postgeneration_save |
| credentials/apps/badges/admin.py | Replaced short_description with @admin.display |
| credentials/apps/api/v2/tests/test_views.py | Updated timezone logic to use datetime.now(timezone.utc) |
| credentials/apps/api/v2/tests/test_serializers.py | Same timezone import and usage update |
| credentials/apps/api/v2/decorators.py | Replaced request.META lookup with request.headers |
| .github/workflows/ci.yml | Expanded Django matrix to include 5.2 |
Comments suppressed due to low confidence (1)
credentials/apps/api/v2/decorators.py:29
- Accessing host via
request.headers.get('host')may not be reliable. Consider usingrequest.get_host()to properly obtain the host header and respect Django’s ALLOWED_HOSTS setting.
f"[{request.user.username}] originating from [{request.headers.get('host', 'Unknown')}] "
| # simulate updating the existing record with the new field in the data | ||
| dt = datetime.datetime.now() | ||
| last_updated_at = dt.replace(tzinfo=pytz.UTC) | ||
| last_updated_at = datetime.now(timezone.utc) |
Copilot
AI
Jun 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Importing datetime from django.utils.timezone may not provide the standard datetime class. Consider from datetime import datetime and either use timezone.now() or from django.utils.timezone import utc for clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, this is a good suggestion and we should switch to just calling timezone.now()
| serializer = UserGradeSerializer(context={"request": Request(site=self.site)}) | ||
| updated_at_dt = datetime.now() | ||
| updated_at_utc = updated_at_dt.replace(tzinfo=pytz.UTC) | ||
| updated_at_utc = datetime.now(timezone.utc) |
Copilot
AI
Jun 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same import concern here: datetime from django.utils.timezone may not map to the built-in datetime. Prefer from datetime import datetime or use timezone.now() to generate a UTC timestamp.
| updated_at_utc = datetime.now(timezone.utc) | |
| updated_at_utc = timezone.now() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good suggestion, using timezone.now will give you zone aware times in UTC and we don't have to use the passthrough import of datetime which may go away in the future.
| serializer = UserGradeSerializer(context={"request": Request(site=self.site)}) | ||
| updated_at_dt = datetime.now() | ||
| updated_at_utc = updated_at_dt.replace(tzinfo=pytz.UTC) | ||
| updated_at_utc = datetime.now(timezone.utc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good suggestion, using timezone.now will give you zone aware times in UTC and we don't have to use the passthrough import of datetime which may go away in the future.
| # simulate updating the existing record with the new field in the data | ||
| dt = datetime.datetime.now() | ||
| last_updated_at = dt.replace(tzinfo=pytz.UTC) | ||
| last_updated_at = datetime.now(timezone.utc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, this is a good suggestion and we should switch to just calling timezone.now()
tox.ini
Outdated
| [testenv] | ||
| deps = | ||
| django42: -r requirements/django.txt | ||
| django52: -r requirements/django.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't actually testing Django 5.2, the django.txt file only pins to the current version of django.
| django52: -r requirements/django.txt | |
| django52: django>=5.2 |
| class CourseFactory(factory.django.DjangoModelFactory): | ||
| class Meta: | ||
| model = Course | ||
| skip_postgeneration_save = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted. it was by the package.
|
I've updated all the code, and this PR looks good overall. However, the storage-related work is still pending. For openedx/edx-platform#36885 ( This PR will update the default file storage usage ) |
|
@awais786 that makes sense, should the helper function that Daniel added get moved to edx-django-utils? so it can also be used here? |
This repository doesn't use |
|
@feanil should we merge this now and fix the STORAGES later or wait for the owning team review ? |
|
PR's been open long enough, if you're happy with it, I think it's okay to merge. Looks like there are some conflicts and it needs a rebase. Let me know if you need me to press the merge button. |
|
@feanil I can't merge this. Please go ahead. |
* feat: Add support for django5.2 * fix: updated timezone utc * fix: storage compatibility issue for django42 * fix: storage compatibility issue for django42 * fix: reverting few changes. * fix: reverting few changes. * fix: fixing timezone.now() changes. * fix: fixing timezone.now() changes. * fix: fixing timezone.now() changes. * fix: fixing timezone.now() changes. * fix: Update factories.py * fix: Update factories.py --------- Co-authored-by: awais qureshi <[email protected]>
Resolves issue
Add Django 5.2 Support
This PR updates the codebase to be compatible with Django 5.2 while maintaining compatibility with Django 4.2.
Changes Made:
django-upgrade usage:
I ran command
git ls-files -z -- '*.py' | xargs -0r django-upgrade --target-version 5.2