-
Notifications
You must be signed in to change notification settings - Fork 17
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.
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
irtazaakram
commented
Apr 8, 2025
- Resolves Add Django 5.2 support to openedx-learning #296
.annotation_safe_list.yml
Outdated
| auth.User: | ||
| ".. no_pii:": "This model has no PII" |
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.
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?!
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.
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!
| migrations.AlterModelOptions( | ||
| name='entitylistrow', | ||
| options={'ordering': ['order_num']}, | ||
| ), |
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.
Where is this migration coming from when there's no changes to the model - is it related to Django 5.2 specifically?
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 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
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.
@irtazaakram Thanks!
bradenmacdonald
left a comment
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.
LGTM