fix: remove annotated models from safelist and annotate openedx models on PII by Akanshu-2u · Pull Request #38712 · openedx/openedx-platform

Akanshu-2u · 2026-06-04T13:04:10Z

Description:

This PR fixes pii_check for edx-platform by adding missing model annotations and correcting the safelist so the annotation tool passes cleanly.

Changes:

Added/fixed inline PII annotations in the targeted edx-platform models (including no_pii and pii metadata where required).
Cleaned safelist conflicts by removing entries for models that are now annotated inline.
Restored required safelist entries for non-local, generated, and historical models so coverage is complete.
Fixed malformed annotation syntax in one model block.

Validation:

Ran the pii annotation check with lint and coverage.
Result: lint passed and coverage reached 100% (all eligible models annotated).

Private JIRA Link:

BOMS-572

…s on PII

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds/standardizes PII annotation markers on several Django models and refreshes the repository annotation safelist to reflect current coverage.

Changes:

Add .. no_pii: markers to multiple models’ docstrings.
Add explicit .. pii, .. pii_types, and .. pii_retirement metadata to models storing Apple identifiers and employee email addresses.
Update .annotation_safe_list.yml by removing older entries and adding a larger set of non-local/generated models.

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
openedx/core/djangoapps/notifications/models.py	Adds `.. no_pii:` annotation to NotificationPreference model docstring.
openedx/core/djangoapps/embargo/models.py	Adds `.. no_pii:` annotation to GlobalRestrictedCountry model docstring.
openedx/core/djangoapps/discussions/models.py	Fixes `.. no_pii:` directive formatting to include a space.
common/djangoapps/third_party_auth/models.py	Annotates Apple migration identifiers as PII with type/retirement metadata.
common/djangoapps/student/models/user.py	Adds `.. no_pii:` to history model and adds PII metadata docstring to AllowedAuthUser.
.annotation_safe_list.yml	Removes older safelist blocks and adds/updates safelist coverage for many models.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Akanshu-2u · 2026-06-05T08:03:18Z

 # Via django-wiki https://github.com/openedx/django-wiki
-wiki.Article:
+
+# Additional non-local or generated models requiring safelist coverage
+agreements.HistoricalUserAgreement:
+  ".. no_pii:": "No PII"


implemented

Akanshu-2u · 2026-06-05T08:03:29Z

    Model to store users' Apple Unique Identifier during migration
    process of Apple team from edx Inc. to edx LLC.
+
+    .. pii: Contains Apple user identifiers (old_apple_id, transfer_id, new_apple_id).


not required

Akanshu-2u · 2026-06-05T08:03:35Z


 class AllowedAuthUser(TimeStampedModel):
+    """
+    Tracks which employee email addresses are allowed to log in with password on a given site.


not required

Copilot

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.

+# Additional non-local or generated models requiring safelist coverage
+agreements.HistoricalUserAgreement:
+  ".. no_pii:": "No PII"
+assessment.HistoricalSharedFileUpload:
+  ".. no_pii:": "No PII"
+casbin_adapter.CasbinRule:
+  ".. no_pii:": "No PII"
+channel_integration.ApiResponseRecord:
+  ".. no_pii:": "No PII"


    """
    Model to store users' Apple Unique Identifier during migration
    process of Apple team from edx Inc. to edx LLC.
+
+    .. pii: Contains Apple user identifiers (old_apple_id, transfer_id, new_apple_id).
+    .. pii_types: external_service
+    .. pii_retirement: local_api


ttak-apphelix · 2026-06-08T07:00:41Z

+    """
+    Tracks which employee email addresses are allowed to log in with password on a given site.
+
+    .. pii: Contains employee email address.


remove employee

Implemented

ttak-apphelix · 2026-06-08T07:02:29Z

    Model to store users' Apple Unique Identifier during migration
    process of Apple team from edx Inc. to edx LLC.
+
+    .. pii: Contains Apple user identifiers (old_apple_id, transfer_id, new_apple_id).


check if this change is required?

AppleMigrationUserIdInfo stores Apple user identifiers (old_apple_id, transfer_id, new_apple_id) which are external identity tokens uniquely tied to real users. Our policy requires all local models storing PII to be annotated inline. The external_service type and local_api retirement are consistent with how we annotate similar identity-linkage models.

bmtcril

This seems ok at a glance, but it doesn't look like the annotation check is working correctly so there may be syntax problems not being caught. The check is reporting 0 models found for cms and lms:

code_annotations django_find_annotations \
	--config_file .pii_annotations.yml \
	--app_name cms \
	--coverage \
	--lint
Configured for report path: pii_report
Configured for source path: ./
/opt/hostedtoolcache/Python/3.12.13/x64/lib/python3.12/site-packages/fs/__init__.py:4: UserWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html. The pkg_resources package is slated for removal as early as 2025-11-30. Refrain from using this package or pin to Setuptools<81.
  __import__("pkg_resources").declare_namespace(__name__)  # type: ignore
Key BLOCK_STRUCTURES_SETTINGS 'STORAGE_CLASS' not found in storage settings {'COURSE_PUBLISH_TASK_DELAY': 30, 'TASK_DEFAULT_RETRY_DELAY': 30, 'TASK_MAX_RETRIES': 5}.Using default storage path.
Found safelist at .annotation_safe_list.yml. Reading.

Performing linting checks...
Linting passed without errors.

Model coverage report
----------------------------------------
Found 0 total models.
0 were eligible for annotation, 0 were annotated.
Coverage is 100.0%

Akanshu-2u · 2026-06-10T05:31:18Z

This seems ok at a glance, but it doesn't look like the annotation check is working correctly so there may be syntax problems not being caught. The check is reporting 0 models found for cms and lms:

code_annotations django_find_annotations \
	--config_file .pii_annotations.yml \
	--app_name cms \
	--coverage \
	--lint
Configured for report path: pii_report
Configured for source path: ./
/opt/hostedtoolcache/Python/3.12.13/x64/lib/python3.12/site-packages/fs/__init__.py:4: UserWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html. The pkg_resources package is slated for removal as early as 2025-11-30. Refrain from using this package or pin to Setuptools<81.
  __import__("pkg_resources").declare_namespace(__name__)  # type: ignore
Key BLOCK_STRUCTURES_SETTINGS 'STORAGE_CLASS' not found in storage settings {'COURSE_PUBLISH_TASK_DELAY': 30, 'TASK_DEFAULT_RETRY_DELAY': 30, 'TASK_MAX_RETRIES': 5}.Using default storage path.
Found safelist at .annotation_safe_list.yml. Reading.

Performing linting checks...
Linting passed without errors.

Model coverage report
----------------------------------------
Found 0 total models.
0 were eligible for annotation, 0 were annotated.
Coverage is 100.0%

Yes, this issue was previously identified and tracked in #38646 to fix the pii_check syntax. However, the corresponding PR intended to address it, #38667, was closed without any explanation and without resolving the underlying issue.

I followed up on the original issue with a comment (#38646 (comment)) requesting clarification, but I have not yet received a response.

At the moment, the only reason this PR is marked as "Do not merge" is that we are waiting for the pii_check fix to be implemented. Once that fix is in place, we will be able to verify this PR through the CI checks and proceed accordingly.

Note: For local testing purposes, I applied a temporary fix to the pii_check syntax issue and ran the checks against these changes. After confirming that everything passed successfully, I proceeded to raise this PR.

bmtcril

👍 Thanks for taking this on!

Akanshu-2u · 2026-06-12T13:25:49Z

The CI got stuck so did a forced push.

Akanshu-2u added 2 commits June 4, 2026 12:57

fix: remove annotated models from safelist and annotate openedx model…

3a3e3c5

…s on PII

Merge branch 'master' into aaich/BOMS-572

0f6dcaf

Akanshu-2u changed the title ~~fix: remove annotated models from safelist and annotate openedx models on PII~~ [Do not merge] fix: remove annotated models from safelist and annotate openedx models on PII Jun 5, 2026

Akanshu-2u requested a review from Copilot June 5, 2026 07:04

Copilot AI reviewed Jun 5, 2026

View reviewed changes

fix: fixed multiple faulty annotations

dcc7ab3

Akanshu-2u requested a review from Copilot June 5, 2026 08:03

Copilot AI reviewed Jun 5, 2026

View reviewed changes

ttak-apphelix reviewed Jun 8, 2026

View reviewed changes

Akanshu-2u added 2 commits June 8, 2026 17:14

Merge branch 'master' into aaich/BOMS-572

b93cf89

fix: fixed the comments

4df8d45

Akanshu-2u requested a review from ttak-apphelix June 8, 2026 11:46

ttak-apphelix approved these changes Jun 9, 2026

View reviewed changes

Merge branch 'master' into aaich/BOMS-572

4d53517

bmtcril reviewed Jun 9, 2026

View reviewed changes

Akanshu-2u requested a review from bmtcril June 10, 2026 05:31

Akanshu-2u added 4 commits June 10, 2026 11:01

Merge branch 'master' into aaich/BOMS-572

28c094c

Merge branch 'master' into aaich/BOMS-572

0f88cd2

fix: removed --app-name from pii_check in makefile

8fc3588

Merge branch 'master' into aaich/BOMS-572

726c45a

Akanshu-2u changed the title ~~[Do not merge] fix: remove annotated models from safelist and annotate openedx models on PII~~ fix: remove annotated models from safelist and annotate openedx models on PII Jun 11, 2026

bmtcril approved these changes Jun 11, 2026

View reviewed changes

Akanshu-2u added 2 commits June 12, 2026 17:21

Merge branch 'master' into aaich/BOMS-572

458c592

fix: added annotations in cms models and coverage to 100%

325320f

Akanshu-2u force-pushed the aaich/BOMS-572 branch from af8fe58 to 325320f Compare June 12, 2026 13:25

bmtcril merged commit 1b338e7 into openedx:master Jun 12, 2026
41 of 42 checks passed

Conversation

Akanshu-2u commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Changes:

Validation:

Private JIRA Link:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Akanshu-2u Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

Akanshu-2u Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

Akanshu-2u Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

ttak-apphelix Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

Akanshu-2u Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

ttak-apphelix Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

Akanshu-2u Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

bmtcril left a comment

Choose a reason for hiding this comment

Uh oh!

Akanshu-2u commented Jun 10, 2026

Uh oh!

bmtcril left a comment

Choose a reason for hiding this comment

Uh oh!

Akanshu-2u commented Jun 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Akanshu-2u commented Jun 4, 2026 •

edited

Loading