[OSEDV-814] Upgrade Django and Python#846
Conversation
React App | Jest test suite - Code coverage reportTotal: 40.32%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
📝 WalkthroughWalkthroughBumps Python and many dependencies; migrates rest_auth → dj_rest_auth and django-ckeditor → django-ckeditor-5 with compatibility shims and migrations; converts Event index declaration to models.Index; standardizes timezone handling to Django default; updates Dockerfile and release notes. Changes
Sequence Diagram(s)(Skipped) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/django/api/throttles.py (1)
5-9: Upgrade djangorestframework to 3.16.x or later for Django 5.1.3 compatibility.The project pins
djangorestframework==3.15.2insrc/django/requirements.txt, but Django REST Framework 3.15.x does not have official support for Django 5.1. The package is configured to use Django 5.1.3 with Python 3.11, which requires DRF 3.16.x or later. Update the dependency to a compatible version.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/django/Dockerfilesrc/django/api/apps.pysrc/django/api/migrations/0190_add_event_index.pysrc/django/api/models/event.pysrc/django/api/processing.pysrc/django/api/serializers/facility/utils.pysrc/django/api/serializers/user/user_password_reset_confirm_serializer.pysrc/django/api/serializers/user/user_password_reset_serializer.pysrc/django/api/throttles.pysrc/django/api/views/auth/login_to_oar_client.pysrc/django/api/views/auth/logout_to_oar_client.pysrc/django/oar/settings.pysrc/django/oar/urls.pysrc/django/requirements.txt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-14T08:13:20.055Z
Learnt from: VadimKovalenkoSNF
Repo: opensupplyhub/open-supply-hub PR: 717
File: src/django/requirements.txt:48-52
Timestamp: 2025-08-14T08:13:20.055Z
Learning: In the Open Supply Hub project, heavy ML/data processing dependencies (splink, geopy, numpy, pandas) are kept in the main requirements.txt file rather than being split into separate worker requirements because the record linkage feature is controlled by a runtime-toggleable Django waffle switch (`enable_record_linkage`) that can be enabled/disabled through Django admin. Separate images would overcomplicate the CI/CD process and could cause runtime failures if the feature is enabled but dependencies are missing.
Applied to files:
src/django/requirements.txt
🧬 Code graph analysis (1)
src/django/api/processing.py (1)
src/django/api/models/facility/facility_list_item.py (1)
FacilityListItem(10-195)
🪛 Ruff (0.14.10)
src/django/api/apps.py
13-13: Consider (*tuple(names), 'index_together') instead of concatenation
Replace with (*tuple(names), 'index_together')
(RUF005)
src/django/api/models/event.py
10-15: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
src/django/api/migrations/0190_add_event_index.py
5-7: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
9-21: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
src/django/oar/settings.py
27-27: Consider (*options.DEFAULT_NAMES, 'index_together') instead of concatenation
Replace with (*options.DEFAULT_NAMES, 'index_together')
(RUF005)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-dd-code-quality
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-flake8-linter
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: run-django-code-quality
- GitHub Check: get-base-branch-django-cov
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: run-countries-code-quality
- GitHub Check: run-fe-code-quality
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-contricleaner-cov
🔇 Additional comments (14)
src/django/api/processing.py (1)
81-82: Good refactoring to useisinstance()for type checking.This change follows Python best practices. The
isinstance()check is more Pythonic and flexible than strict type equality, as it properly handles inheritance hierarchies. The codebase contains no subclasses ofFacilityListItem, so this is a safe modernization that improves code maintainability.src/django/api/throttles.py (1)
14-14: Docstring improvement: removes redundancy and clarifies wording.The updated docstring is clearer, removing the redundant "custom" and simplifying the phrasing. The change improves code clarity without affecting functionality.
src/django/api/views/auth/login_to_oar_client.py (1)
4-4: Import migration looks correct.The migration from
rest_auth.views.LoginViewtodj_rest_auth.views.LoginViewis appropriate. Since the custompost()method handles authentication independently using Django'sauthenticate()andlogin(), this change should be compatible with dj-rest-auth 7.x.src/django/api/models/event.py (1)
10-15: Good migration fromindex_togetherto explicitindexes.This correctly updates from the deprecated
index_togethersyntax to explicitmodels.Index(), which is the recommended approach for Django 4.2+ and required for Django 5.x whereindex_togetheris fully removed.src/django/api/serializers/facility/utils.py (1)
3-3: Timezone change is functionally equivalent.Using
datetime.timezone.utcinstead ofdjango.utils.timezone.utcworks identically for theastimezone()call. Both represent UTC. This is a valid change, thoughdjango.utils.timezoneremains fully supported in Django 5.x if consistency with other Django code is preferred.Also applies to: 267-267
src/django/Dockerfile (1)
1-1: Python 3.11 base image upgrade looks good.The upgrade to
python:3.11-slim-bookwormaligns with Django 5.1.3 requirements. The removal of the aiokafka workaround simplifies the build process.src/django/api/views/auth/logout_to_oar_client.py (1)
3-3: Import migration looks correct.The migration from
rest_auth.views.LogoutViewtodj_rest_auth.views.LogoutViewis appropriate. The custompost()method uses Django'slogout()directly, so it's independent of the parent class's implementation details.src/django/api/serializers/user/user_password_reset_confirm_serializer.py (1)
1-1: Verifyself.userandself.set_password_formattribute availability in dj-rest-auth 7.x.The import migration is correct, but dj-rest-auth 7.x's
PasswordResetConfirmSerializermay have different internal attributes. The customsave()method relies onself.userandself.set_password_formbeing populated by the parent class's validation. Verify these attributes exist after calling the parent's validation methods.dj-rest-auth PasswordResetConfirmSerializer user set_password_form attributesAlso applies to: 5-10
src/django/api/serializers/user/user_password_reset_serializer.py (1)
1-1: Verify customsave()method compatibility with dj-rest-auth 7.x.The import migration to dj-rest-auth is correct, but dj-rest-auth 7.x PasswordResetSerializer.save() requires a specific parameter signature. Ensure the custom save() method at lines 25-47 implements the correct signature and passes compatible parameters to reset_form.save().
src/django/api/migrations/0190_add_event_index.py (1)
1-22: LGTM! Migration correctly transitions to explicit index declaration.The migration properly uses
AlterModelOptionsto add an explicit index, which is the modern approach for Django 5.x. This replaces the deprecatedindex_togetherMeta option.Note: The static analysis warnings (RUF012) about
ClassVarannotations are false positives—Django's migration framework requiresdependenciesandoperationsas standard class attributes.src/django/oar/settings.py (2)
15-15: Approve compatibility workaround for legacy migrations.The import and compatibility code correctly addresses Django 5.x's removal of
index_togetherfromDEFAULT_NAMES. This allows existing migrations that reference the deprecatedindex_togetherMeta option to continue working during the upgrade transition.Note: The static analysis hint (RUF005) suggesting tuple unpacking is valid but represents a minor style improvement that can be optionally addressed.
Optional style improvement
- options.DEFAULT_NAMES = options.DEFAULT_NAMES + ('index_together',) + options.DEFAULT_NAMES = (*options.DEFAULT_NAMES, 'index_together')Also applies to: 25-27
131-131: LGTM! Migration from django-rest-auth to dj-rest-auth is necessary.The package swap from
rest_authtodj_rest_authis required because django-rest-auth is no longer maintained. The dj-rest-auth fork provides Django 5.x compatibility and active maintenance.Also applies to: 135-135
src/django/oar/urls.py (1)
148-149: LGTM! URL patterns updated for dj-rest-auth migration.The URL includes are correctly updated from
rest_authtodj_rest_auth, aligning with the changes in settings.py and the overall migration to the maintained authentication package.src/django/requirements.txt (1)
2-2: Verify Django 5.1.3 compatibility and security advisories for major upgrades.This PR performs major framework upgrades including Django 3.2.17 → 5.1.3 (skipping 4.x) and implicitly Python 3.8 → 3.11. While the commit history shows an intermediate Django 4.2.6 step, the final jump to 5.1.3 introduces significant changes.
Key concerns:
- Django 5.1.3 verification: Confirm this version exists and check for security advisories
- Breaking changes: Django 5.x has numerous breaking changes that must be addressed
- Python 3.11 compatibility: Verify all dependencies support Python 3.11
- Security advisories: Check for known vulnerabilities in pinned versions
Note: The PR description appears to have swapped version numbers—it states "Python: 3.2.17 → 5.1.3; Django: 3.8 → 3.11" when it should be "Django: 3.2.17 → 5.1.3; Python: 3.8 → 3.11".
What is the latest stable version of Django 5.1 and are there any security advisories for Django 5.1.3?What are the major breaking changes in Django 5.0 and Django 5.1 compared to Django 3.2?Does djangorestframework 3.15.2 support Django 5.1 and Python 3.11?Also applies to: 4-4, 6-8, 10-10, 13-13, 15-16, 18-19, 21-34, 36-39, 41-44, 48-50
Dedupe Hub App | Unittest test suite - Code coverage reportTotal: 55.73%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Contricleaner App | Unittest test suite - Code coverage reportTotal: 98.75%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Countries App | Unittest test suite - Code coverage reportTotal: 100%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/django/oar/settings.py (1)
409-409:STATICFILES_STORAGEis ignored in Django 5.2; local dev loses SPA behavior.
STATICFILES_STORAGEwas removed in Django 5.1—this line has no effect. TheSTORAGESdict at lines 610-617 is only defined whennot DEBUG or (AWS_S3_ENDPOINT_URL and not TESTING). In local DEBUG mode withoutAWS_S3_ENDPOINT_URL,STORAGESis never set, so Django defaults toStaticFilesStorageinstead ofSPAStaticFilesStorage, breaking SPA routing (index.html at root, fallback behavior).Define
STORAGESunconditionally with the SPA staticfiles backend, then conditionally override the"default"backend for S3:🔧 Proposed fix
-STATICFILES_STORAGE = 'spa.storage.SPAStaticFilesStorage' +# Django 5.2+ requires STORAGES dict; STATICFILES_STORAGE is removed. +STORAGES = { + "default": { + "BACKEND": "django.core.files.storage.FileSystemStorage", + }, + "staticfiles": { + "BACKEND": "spa.storage.SPAStaticFilesStorage", + }, +}Then update the conditional block to only override the
"default"backend:if not DEBUG or (AWS_S3_ENDPOINT_URL and not TESTING): - STORAGES = { - "default": { - "BACKEND": "storages.backends.s3boto3.S3Boto3Storage", - }, - "staticfiles": { - "BACKEND": "spa.storage.SPAStaticFilesStorage", - }, - } + STORAGES["default"] = { + "BACKEND": "storages.backends.s3boto3.S3Boto3Storage", + }Also applies to: 609-617
🧹 Nitpick comments (1)
src/django/oar/settings.py (1)
376-376: Remove deprecatedUSE_L10Nsetting.
USE_L10Nwas deprecated in Django 4.0 and removed in Django 5.0. In Django 5.2, localization is always enabled, making this setting a no-op. Consider removing it to clean up dead code.♻️ Proposed fix
USE_I18N = True -USE_L10N = True - USE_TZ = True
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/django/oar/settings.py`:
- Line 409: Remove the deprecated STATICFILES_STORAGE assignment and replace it
by unconditionally defining the STORAGES dict in settings.py; keep conditional
logic only for the "default" backend (e.g., choose between the SPA backend
'oar.storage.SPAStaticFilesStorage' and whatever other default you had) while
always providing a "staticfiles" or equivalent key in STORAGES so Django has a
staticfiles backend fallback; update references to STATICFILES_STORAGE
accordingly and ensure STORAGES is declared at module scope (replace the
STATICFILES_STORAGE = 'oar.storage.SPAStaticFilesStorage' line and ensure the
existing conditional that previously set STORAGES at line ~610 is consolidated
into a single unconditional STORAGES definition that uses a conditional
expression for the "default" entry.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/django/oar/settings.py (1)
17-30: Remove theindex_togethercompatibility shim—it is ineffective and relies on unstable internals.In Django 5.2,
index_togetherwas removed from runtime Meta support (Django 5.1+). The shim does not help with old migrations: Django's migration framework handles backward compatibility through dedicated operations (AlterIndexTogether,RenameIndex), not via runtime Meta validation. Monkey-patchingDEFAULT_NAMES(an internal, unstable API with no public guarantees) only prevents a TypeError if code tries to defineindex_togetherin Meta, but does nothing to actually support the removed feature. Delete lines 26–29.
🤖 Fix all issues with AI agents
In `@src/django/oar/settings.py`:
- Around line 37-55: Update CKEDITOR_5_CONFIGS by removing invalid plugin names
from the "removePlugins" list so only supported identifiers are used; locate the
CKEDITOR_5_CONFIGS dict and the "removePlugins" key and replace the entries that
are not part of django-ckeditor-5 0.2.19 (e.g., remove CKBox, EasyImage,
ImageUpload) leaving only valid plugin names such as CKFinderUploadAdapter,
Image, ImageCaption, ImageStyle, ImageToolbar, MediaEmbed, Table, and
TableToolbar so the build will actually disable the intended features.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/django/oar/settings.py`:
- Around line 609-617: STORAGES is currently defined conditionally and uses
django.contrib.staticfiles.storage.ManifestStaticFilesStorage for "staticfiles",
which breaks SPA behavior; change the configuration so STORAGES is defined
unconditionally and set the "staticfiles" backend to
spa.storage.SPAStaticFilesStorage (replace ManifestStaticFilesStorage with
spa.storage.SPAStaticFilesStorage) and keep the "default" S3 backend as-is
(S3Boto3Storage) so that SPA routing and asset serving work in DEBUG and
production regardless of AWS_S3_ENDPOINT_URL or TESTING.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/django/oar/settings.py (1)
409-409: Critical:STATICFILES_STORAGEis ignored in Django 5.1+ — remove and migrate toSTORAGES.
STATICFILES_STORAGEwas removed in Django 5.1. This line is now a no-op and the SPA staticfiles backend (spa.storage.SPAStaticFilesStorage) is not being used. This breaks SPA functionality:
index.htmlnot served at root- Client-side routing fallback not working
- Static assets not served at both
/static/...and/...pathsRemove this line and update the
STORAGESconfiguration (lines 609-623) to usespa.storage.SPAStaticFilesStoragefor thestaticfilesbackend.Suggested fix
Remove line 409 entirely, then update
STORAGES(see next comment).
…orage in Docker build
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/django/oar/settings.py`:
- Around line 607-621: The DEBUG branch of the STORAGES dict is missing the
"default" backend which Django 5.2 no longer provides implicitly; update the
DEBUG branch STORAGES to include a "default" key using a filesystem storage
backend (e.g., django.core.files.storage.FileSystemStorage) so default_storage
and FileField work in development, keeping the existing "staticfiles" entry
intact.
protsack-stephan
left a comment
There was a problem hiding this comment.
Great work, one small comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/django/oar/settings.py (1)
376-376: Remove deprecatedUSE_L10Nsetting.
USE_L10Nwas deprecated in Django 4.0 and removed in Django 5.0. The project uses Django 5.2.10, where this setting no longer has any effect and may cause configuration warnings. Remove this line entirely, as localization is always enabled in Django 5.0+.
🧹 Nitpick comments (1)
src/django/oar/settings.py (1)
26-29: Acceptable workaround, but consider migration squashing long-term.This monkey-patch allows legacy migrations using
index_togetherto load without editing historical migration files. While this works, it relies on Django internals that could change. Consider usingsquashmigrationsto consolidate old migrations and convertindex_togethertoindexesin the squashed migration, removing the need for this hack in future releases.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/django/oar/settings.py`:
- Around line 153-165: The migration file 0185_add_source_by_to_partner_field.py
still imports the legacy ckeditor package (relying on the compatibility shim in
the ckeditor package __init__), so update the migration to stop depending on the
shim: replace the legacy import from "ckeditor" with the correct import from the
new package (django_ckeditor_5) or the exact class/field module provided by
django_ckeditor_5 used in that migration, ensuring the migration references the
new package names directly (or, if impossible, add a one-line compatibility
alias inside the migration to map the old name to the new one) and then remove
reliance on the ckeditor compatibility shim.
- Around line 37-55: Update the CKEDITOR_5_CONFIGS mapping in settings.py to
remove the invalid plugin identifier "CKFinderUploadAdapter" from the
"removePlugins" list in the "default" config of CKEDITOR_5_CONFIGS; replace it
with the valid identifier "UploadAdapter" if you intend to disable upload
adapters, or simply delete that entry entirely so only valid plugin names
("Image", "ImageCaption", "ImageStyle", "ImageToolbar", "MediaEmbed", "Table",
"TableToolbar") remain, ensuring CKEditor 5 initializes correctly.
|
|
@coderabbitai resolve |
✅ Actions performedComments resolved. |



Fix OSDEV-814
3.8→3.113.2.17→5.2.10Note: DedupeHub was not affected as it is a separate service.