Skip to content

Add support for Django 5.2 #298

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.

Sign up for GitHub

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

Merged
irtazaakram merged 3 commits into from
Apr 10, 2025
+78 −5
Merged

Add support for Django 5.2 #298

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7984bf9
fix: add support for django 5.2
irtazaakram Apr 8, 2025
79f7973
chore: bump version
irtazaakram Apr 8, 2025
8c04e28
fix: add PII for auth.User
irtazaakram Apr 9, 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
54 changes: 53 additions & 1 deletion .annotation_safe_list.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,59 @@ auth.Group:
".. no_pii:": "This model has no PII"
auth.Permission:
".. no_pii:": "This model has no PII"
auth.User:
".. no_pii:": "This model has no PII"

@bradenmacdonald bradenmacdonald Apr 8, 2025

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.

Does the User model really have no PII ? And why do we have to declare annotations for common dependencies like Django or waffle redundantly in this repo anyways?!

@irtazaakram irtazaakram Apr 9, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The auth.User model does have some PII (username, email, and password), so I’ve added the annotations for it.
I think get_models_requiring_annotations method in find_django.py seems to treats all models (including those from external dependencies) as requiring annotation.

pii_check: commands[0]> code_annotations django_find_annotations --config_file .pii_annotations.yml --lint --report --coverage
Configured for report path: pii_report
Configured for source path: ./
Found safelist at .annotation_safe_list.yml. Reading.

Performing linting checks...
Linting passed without errors.

Model coverage report
----------------------------------------
Found 35 total models.
31 were eligible for annotation, 5 were annotated.
Coverage is 16.1%

Coverage found 26 uncovered models:
   auth.User
   oel_collections.Collection
   oel_collections.CollectionPublishableEntity
   oel_components.Component
   oel_components.ComponentType
   oel_components.ComponentVersion
   oel_components.ComponentVersionContent
   oel_contents.Content
   oel_contents.MediaType
   oel_publishing.Container
   oel_publishing.ContainerVersion
   oel_publishing.Draft
   oel_publishing.EntityList
   oel_publishing.EntityListRow
   oel_publishing.LearningPackage
   oel_publishing.PublishLog
   oel_publishing.PublishLogRecord
   oel_publishing.PublishableEntity
   oel_publishing.PublishableEntityVersion
   oel_publishing.Published
   oel_tagging.ObjectTag
   oel_tagging.Tag
   oel_tagging.TagImportTask
   oel_tagging.Taxonomy
   oel_units.Unit
   oel_units.UnitVersion

Coverage threshold not met! Needed 100.0, actually 16.1!

contenttypes.ContentType:
".. no_pii:": "This model has no PII"
sessions.Session:
oel_collections.Collection:
".. no_pii:": "This model has no PII"
oel_collections.CollectionPublishableEntity:
".. no_pii:": "This model has no PII"
oel_components.Component:
".. no_pii:": "This model has no PII"
oel_components.ComponentType:
".. no_pii:": "This model has no PII"
oel_components.ComponentVersion:
".. no_pii:": "This model has no PII"
oel_components.ComponentVersionContent:
".. no_pii:": "This model has no PII"
oel_contents.Content:
".. no_pii:": "This model has no PII"
oel_contents.MediaType:
".. no_pii:": "This model has no PII"
oel_publishing.Container:
".. no_pii:": "This model has no PII"
oel_publishing.ContainerVersion:
".. no_pii:": "This model has no PII"
oel_publishing.Draft:
".. no_pii:": "This model has no PII"
oel_publishing.EntityList:
".. no_pii:": "This model has no PII"
oel_publishing.EntityListRow:
".. no_pii:": "This model has no PII"
oel_publishing.LearningPackage:
".. no_pii:": "This model has no PII"
oel_publishing.PublishLog:
".. no_pii:": "This model has no PII"
oel_publishing.PublishLogRecord:
".. no_pii:": "This model has no PII"
oel_publishing.PublishableEntity:
".. no_pii:": "This model has no PII"
oel_publishing.PublishableEntityVersion:
".. no_pii:": "This model has no PII"
oel_publishing.Published:
".. no_pii:": "This model has no PII"
oel_tagging.ObjectTag:
".. no_pii:": "This model has no PII"
oel_tagging.Tag:
".. no_pii:": "This model has no PII"
oel_tagging.TagImportTask:
".. no_pii:": "This model has no PII"
oel_tagging.Taxonomy:
".. no_pii:": "This model has no PII"
oel_units.Unit:
".. no_pii:": "This model has no PII"
oel_units.UnitVersion:
".. no_pii:": "This model has no PII"
social_django.Association:
".. no_pii:": "This model has no PII"
Expand All @@ -29,6 +79,8 @@ social_django.Partial:
".. no_pii:": "This model has no PII"
social_django.UserSocialAuth:
".. no_pii:": "This model has no PII"
sessions.Session:
".. no_pii:": "This model has no PII"
waffle.Flag:
".. no_pii:": "This model has no PII"
waffle.Sample:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ jobs:
matrix:
os: [ubuntu-latest] # Add macos-latest later?
python-version: ['3.11', '3.12']
toxenv: ["django42", "package", "quality"]
toxenv: ["django42", "django52", "package", "quality"]
# We're only testing against MySQL 8 right now because 5.7 is
# incompatible with Djagno 4.2. We'd have to make the tox.ini file more
# complicated than it's worth given the short expected shelf-life of
Expand Down
2 changes: 1 addition & 1 deletion openedx_learning/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
Open edX Learning ("Learning Core").
"""

__version__ = "0.19.2"
__version__ = "0.20.0"
17 changes: 17 additions & 0 deletions openedx_learning/apps/authoring/publishing/migrations/0005_alter_entitylistrow_options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Generated by Django 5.2 on 2025-04-08 10:50

from django.db import migrations


class Migration(migrations.Migration):

dependencies = [
('oel_publishing', '0004_publishableentity_can_stand_alone'),
]

operations = [
migrations.AlterModelOptions(
name='entitylistrow',
options={'ordering': ['order_num']},
),
Comment on lines +13 to +16

@bradenmacdonald bradenmacdonald Apr 8, 2025

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.

Where is this migration coming from when there's no changes to the model - is it related to Django 5.2 specifically?

@irtazaakram irtazaakram Apr 9, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This migration isn't related to Django 5.2 It reflects a change that was made to the entitylistrow model last week, https://github.com/openedx/openedx-learning/pull/292/files#diff-f814bfe5a65c31e277927ed66550877cb86f65b735347d1b478eae5f91825886

@navinkarkera navinkarkera Apr 10, 2025

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.

@irtazaakram Thanks!

]
2 changes: 1 addition & 1 deletion requirements/base.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ attrs # Reduces boilerplate code involving class attributes

celery # Asynchronous task execution library

Django<5.0 # Web application framework
Django # Web application framework

djangorestframework<4.0 # REST API
edx-drf-extensions # Extensions to the Django REST Framework used by Open edX
Expand Down
1 change: 1 addition & 0 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ def is_requirement(line):
'Development Status :: 3 - Alpha',
'Framework :: Django',
'Framework :: Django :: 4.2',
'Framework :: Django :: 5.2',
'Intended Audience :: Developers',
'License :: OSI Approved :: GNU Affero General Public License v3 or later (AGPLv3+)',
'Natural Language :: English',
Expand Down
3 changes: 2 additions & 1 deletion tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
[tox]
envlist = py{311,312}-django{42}, quality, docs, pii_check, lint-imports
envlist = py{311,312}-django{42,52}, quality, docs, pii_check, lint-imports

[doc8]
; D001 = Line too long
Expand Down Expand Up @@ -38,6 +38,7 @@ norecursedirs = .* docs requirements site-packages
deps =
setuptools
django42: Django>=4.2,<5.0
django52: Django>=5.2,<6.0
-r{toxinidir}/requirements/test.txt
setenv =
# Note that the django32/django42 targets use MySQL, but running pytest by
Expand Down