From 52aa0dd7aa0e52d977613f5e0534317fa516bd83 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 8 Apr 2026 17:55:17 -0700 Subject: [PATCH 1/6] chore: update to use strongly-typed IDs from openedx_content --- cms/djangoapps/contentstore/models.py | 2 +- cms/djangoapps/contentstore/utils.py | 2 +- cms/djangoapps/modulestore_migrator/api/write_api.py | 4 +++- cms/djangoapps/modulestore_migrator/tasks.py | 6 +++--- .../djangoapps/content_libraries/api/container_metadata.py | 4 ++-- openedx/core/djangoapps/content_libraries/api/libraries.py | 2 +- 6 files changed, 11 insertions(+), 9 deletions(-) diff --git a/cms/djangoapps/contentstore/models.py b/cms/djangoapps/contentstore/models.py index 9bc241e5b34f..c39e04c299dc 100644 --- a/cms/djangoapps/contentstore/models.py +++ b/cms/djangoapps/contentstore/models.py @@ -485,7 +485,7 @@ def _annotate_query_with_ready_to_sync(cls, query_set: QuerySet["EntityLinkBase" @classmethod def update_or_create( cls, - upstream_container_id: int | None, + upstream_container_id: Container.ID | None, /, upstream_container_key: LibraryContainerLocator, upstream_context_key: str, diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index b0b37a4a13b2..35818125efab 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -2425,7 +2425,7 @@ def _create_or_update_container_link(created: datetime | None, xblock): """ upstream_container_key = LibraryContainerLocator.from_string(xblock.upstream) try: - lib_component = get_container(upstream_container_key).container_pk + lib_component = get_container(upstream_container_key).container_id except ObjectDoesNotExist: log.error(f"Library component not found for {upstream_container_key}") lib_component = None diff --git a/cms/djangoapps/modulestore_migrator/api/write_api.py b/cms/djangoapps/modulestore_migrator/api/write_api.py index 52a1d9ec45fe..b410ec552bc2 100644 --- a/cms/djangoapps/modulestore_migrator/api/write_api.py +++ b/cms/djangoapps/modulestore_migrator/api/write_api.py @@ -6,6 +6,7 @@ from celery.result import AsyncResult from opaque_keys.edx.locator import LibraryLocatorV2 from openedx_content.api import get_collection +from openedx_content.models_api import LearningPackage from openedx.core.djangoapps.content_libraries.api import get_library from openedx.core.types.user import AuthUser @@ -64,7 +65,8 @@ def start_bulk_migration_to_library( """ target_library = get_library(target_library_key) # get_library ensures that the library is connected to a learning package. - target_package_id: int = target_library.learning_package_id # type: ignore[assignment] + assert target_library.learning_package_id is not None + target_package_id: LearningPackage.ID = target_library.learning_package_id sources_pks: list[int] = [] for source_key in source_key_list: diff --git a/cms/djangoapps/modulestore_migrator/tasks.py b/cms/djangoapps/modulestore_migrator/tasks.py index 07fdee7008c3..17bd5dd5bc95 100644 --- a/cms/djangoapps/modulestore_migrator/tasks.py +++ b/cms/djangoapps/modulestore_migrator/tasks.py @@ -120,7 +120,7 @@ class _MigrationContext: # Fields that remain constant previous_block_migrations: dict[UsageKey, data.ModulestoreBlockMigrationResult] - target_package_id: int + target_package_id: LearningPackage.ID target_library_key: LibraryLocatorV2 source_context_key: SourceContextKey content_by_filename: dict[str, int] @@ -882,12 +882,12 @@ def _migrate_container( ) if container_exists and context.should_skip_strategy: return PublishableEntityVersion.objects.get( - entity_id=container.container_pk, + entity_id=container.container_id, version_num=container.draft_version_num, ), None container_publishable_entity_version = content_api.create_next_container_version( - container.container_pk, + container.container_id, title=title, entities=[child.entity for child in children], created=context.created_at, diff --git a/openedx/core/djangoapps/content_libraries/api/container_metadata.py b/openedx/core/djangoapps/content_libraries/api/container_metadata.py index 9ef548c1bbef..b154d7ab4fa2 100644 --- a/openedx/core/djangoapps/content_libraries/api/container_metadata.py +++ b/openedx/core/djangoapps/content_libraries/api/container_metadata.py @@ -71,7 +71,7 @@ class ContainerMetadata(PublishableItem): container_key: LibraryContainerLocator container_type_code: str - container_pk: int + container_id: Container.ID @classmethod def from_container(cls, library_key, container: Container, associated_collections=None): @@ -99,7 +99,7 @@ def from_container(cls, library_key, container: Container, associated_collection return cls( container_key=container_key, container_type_code=container_key.container_type, - container_pk=container.pk, + container_id=container.id, display_name=draft.title, created=container.created, modified=draft.created, diff --git a/openedx/core/djangoapps/content_libraries/api/libraries.py b/openedx/core/djangoapps/content_libraries/api/libraries.py index 4fdbcb95d5f4..cb232ae06f80 100644 --- a/openedx/core/djangoapps/content_libraries/api/libraries.py +++ b/openedx/core/djangoapps/content_libraries/api/libraries.py @@ -124,7 +124,7 @@ class ContentLibraryMetadata: Class that represents the metadata about a content library. """ key: LibraryLocatorV2 - learning_package_id: int | None + learning_package_id: LearningPackage.ID | None title: str = "" description: str = "" num_blocks: int = 0 From 698f12dc868500aa2e1eaa581d441f23a9892acb Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 9 Apr 2026 09:42:38 -0700 Subject: [PATCH 2/6] temp: use openedx-core branch with strongly-typed keys --- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 77a5f9e113a5..b6bae0c0a647 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -832,7 +832,7 @@ openedx-calc==5.0.0 # via # -r requirements/edx/kernel.in # xblocks-contrib -openedx-core==0.38.2 +openedx-core @ git+https://github.com/openedx/openedx-core.git@braden/typed-pks # via # -c requirements/constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 657ac94cc455..9a0a504d27c3 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1384,7 +1384,7 @@ openedx-calc==5.0.0 # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # xblocks-contrib -openedx-core==0.38.2 +openedx-core @ git+https://github.com/openedx/openedx-core.git@braden/typed-pks # via # -c requirements/constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 0cc6b952a9b0..1feb58ad2c0b 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -1009,7 +1009,7 @@ openedx-calc==5.0.0 # via # -r requirements/edx/base.txt # xblocks-contrib -openedx-core==0.38.2 +openedx-core @ git+https://github.com/openedx/openedx-core.git@braden/typed-pks # via # -c requirements/constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 252d9f6ff777..6c1945e256bc 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1056,7 +1056,7 @@ openedx-calc==5.0.0 # via # -r requirements/edx/base.txt # xblocks-contrib -openedx-core==0.38.2 +openedx-core @ git+https://github.com/openedx/openedx-core.git@braden/typed-pks # via # -c requirements/constraints.txt # -r requirements/edx/base.txt From b52adceabf806291ee5b03fe1ab996b3d6ff7488 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 9 Apr 2026 10:22:51 -0700 Subject: [PATCH 3/6] feat: fully typed primary keys for StagedContent model --- cms/djangoapps/contentstore/helpers.py | 10 +++++---- .../content_libraries/api/blocks.py | 5 +++-- .../core/djangoapps/content_staging/api.py | 21 ++++++++++++------- .../core/djangoapps/content_staging/data.py | 6 +++++- .../core/djangoapps/content_staging/models.py | 14 +++++++++---- .../core/djangoapps/content_staging/tasks.py | 2 +- .../content_staging/tests/test_clipboard.py | 3 ++- 7 files changed, 41 insertions(+), 20 deletions(-) diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index 8213eb9e2c56..1a0d697d7555 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -6,6 +6,7 @@ Only Studio-specfic helper functions should be added here. Platform-wide Python APIs should be added to an appropriate api.py file instead. """ + from __future__ import annotations import json @@ -33,6 +34,7 @@ from cms.djangoapps.models.settings.course_grading import CourseGradingModel from cms.lib.xblock.upstream_sync import UpstreamLink, UpstreamLinkException from cms.lib.xblock.upstream_sync_block import fetch_customizable_fields_from_block +from openedx.core.djangoapps.content_staging.api import StagedContentID from openedx.core.djangoapps.content_staging.data import LIBRARY_SYNC_PURPOSE from openedx.core.djangoapps.content_tagging.types import TagValuesByObjectIdDict from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers @@ -312,7 +314,7 @@ def _rewrite_static_asset_references(downstream_xblock: XBlock, substitutions: d def _insert_static_files_into_downstream_xblock( - downstream_xblock: XBlock, staged_content_id: int, request + downstream_xblock: XBlock, staged_content_id: StagedContentID, request ) -> StaticFileNotices: """ Gets static files from staged content, and inserts them into the downstream XBlock. @@ -635,7 +637,7 @@ def _import_xml_node_to_parent( def _import_files_into_course( course_key: CourseKey, - staged_content_id: int, + staged_content_id: StagedContentID, static_files: list[content_staging_api.StagedContentFileData], usage_key: UsageKey, ) -> tuple[StaticFileNotices, dict[str, str]]: @@ -700,7 +702,7 @@ def _import_files_into_course( def _import_file_into_course( course_key: CourseKey, - staged_content_id: int, + staged_content_id: StagedContentID, file_data_obj: content_staging_api.StagedContentFileData, usage_key: UsageKey, ) -> tuple[bool | None, dict]: @@ -760,7 +762,7 @@ def _import_file_into_course( def _import_transcripts( block: XBlock, - staged_content_id: int, + staged_content_id: StagedContentID, static_files: list[content_staging_api.StagedContentFileData], ): """ diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index 3678ea9f42d6..4b61c50ca89f 100644 --- a/openedx/core/djangoapps/content_libraries/api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/api/blocks.py @@ -41,6 +41,7 @@ ) from xblock.core import XBlock +from openedx.core.djangoapps.content_staging.data import StagedContentID from openedx.core.djangoapps.xblock.api import ( get_component_from_usage_key, get_xblock_app_config, @@ -382,7 +383,7 @@ def _import_staged_block( library_key: LibraryLocatorV2, source_context_key: LearningContextKey, user, - staged_content_id: int, + staged_content_id: StagedContentID, staged_content_files: list[StagedContentFileData], now: datetime, ) -> LibraryXBlockMetadata: @@ -517,7 +518,7 @@ def _import_staged_block_as_container( library_key: LibraryLocatorV2, source_context_key: LearningContextKey, user, - staged_content_id: int, + staged_content_id: StagedContentID, staged_content_files: list[StagedContentFileData], now: datetime, *, diff --git a/openedx/core/djangoapps/content_staging/api.py b/openedx/core/djangoapps/content_staging/api.py index eb7bf94b010f..1700f2358739 100644 --- a/openedx/core/djangoapps/content_staging/api.py +++ b/openedx/core/djangoapps/content_staging/api.py @@ -19,7 +19,14 @@ from xmodule.contentstore.content import StaticContent from xmodule.contentstore.django import contentstore -from .data import CLIPBOARD_PURPOSE, StagedContentData, StagedContentFileData, StagedContentStatus, UserClipboardData +from .data import ( + CLIPBOARD_PURPOSE, + StagedContentData, + StagedContentFileData, + StagedContentID, + StagedContentStatus, + UserClipboardData, +) from .models import StagedContent as _StagedContent from .models import StagedContentFile as _StagedContentFile from .models import UserClipboard as _UserClipboard @@ -315,26 +322,26 @@ def _user_clipboard_model_to_data(clipboard: _UserClipboard) -> UserClipboardDat ) -def get_staged_content_olx(staged_content_id: int) -> str | None: +def get_staged_content_olx(staged_content_id: StagedContentID) -> str | None: """ Get the OLX (as a string) for the given StagedContent. Does not check permissions! """ try: - sc = _StagedContent.objects.get(pk=staged_content_id) + sc = _StagedContent.objects.get(id=staged_content_id) return sc.olx except _StagedContent.DoesNotExist: return None -def get_staged_content_static_files(staged_content_id: int) -> list[StagedContentFileData]: +def get_staged_content_static_files(staged_content_id: StagedContentID) -> list[StagedContentFileData]: """ Get the filenames and metadata for any static files used by the given staged content. Does not check permissions! """ - sc = _StagedContent.objects.get(pk=staged_content_id) + sc = _StagedContent.objects.get(id=staged_content_id) def str_to_key(source_key_str: str): if not source_key_str: @@ -360,13 +367,13 @@ def str_to_key(source_key_str: str): ] -def get_staged_content_static_file_data(staged_content_id: int, filename: str) -> bytes | None: +def get_staged_content_static_file_data(staged_content_id: StagedContentID, filename: str) -> bytes | None: """ Get the data for the static asset associated with the given staged content. Does not check permissions! """ - sc = _StagedContent.objects.get(pk=staged_content_id) + sc = _StagedContent.objects.get(id=staged_content_id) file_data_obj = sc.files.filter(filename=filename).first() if file_data_obj: return file_data_obj.data_file.open().read() diff --git a/openedx/core/djangoapps/content_staging/data.py b/openedx/core/djangoapps/content_staging/data.py index cf311e081e8f..47572058a332 100644 --- a/openedx/core/djangoapps/content_staging/data.py +++ b/openedx/core/djangoapps/content_staging/data.py @@ -4,6 +4,7 @@ from __future__ import annotations from datetime import datetime +from typing import NewType from attrs import field, frozen, validators from django.db.models import TextChoices @@ -12,6 +13,9 @@ from openedx.core.djangoapps.content_tagging.api import TagValuesByObjectIdDict +# Typed primary key for the StagedContent model / StagedContentData +StagedContentID = NewType("StagedContentID", int) + class StagedContentStatus(TextChoices): """ The status of this staged content. """ @@ -42,7 +46,7 @@ class StagedContentData: (OLX content that isn't part of any course at the moment) """ - id: int = field(validator=validators.instance_of(int)) + id: StagedContentID = field(validator=validators.instance_of(int)) # type: ignore[assignment] user_id: int = field(validator=validators.instance_of(int)) created: datetime = field(validator=validators.instance_of(datetime)) purpose: str = field(validator=validators.instance_of(str)) diff --git a/openedx/core/djangoapps/content_staging/models.py b/openedx/core/djangoapps/content_staging/models.py index 5248b6ab2cc3..19639fbd0765 100644 --- a/openedx/core/djangoapps/content_staging/models.py +++ b/openedx/core/djangoapps/content_staging/models.py @@ -1,6 +1,7 @@ """ Models for content staging (and clipboard) """ + from __future__ import annotations import logging @@ -11,11 +12,11 @@ from django.utils.translation import gettext_lazy as _ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import ContainerKey, LearningContextKey, UsageKey -from openedx_django_lib.fields import MultiCollationTextField, case_insensitive_char_field +from openedx_django_lib.fields import MultiCollationTextField, TypedAutoField, case_insensitive_char_field from openedx.core.djangoapps.content.course_overviews.api import get_course_overview_or_none -from .data import CLIPBOARD_PURPOSE, StagedContentStatus +from .data import CLIPBOARD_PURPOSE, StagedContentID, StagedContentStatus log = logging.getLogger(__name__) @@ -42,9 +43,14 @@ class StagedContent(models.Model): class Meta: verbose_name_plural = _("Staged Content") - id = models.AutoField(primary_key=True) # noqa: DJ012 + type ID = StagedContentID + + class IDField(TypedAutoField[ID]): + pass + + id = IDField(primary_key=True) # The user that created and owns this staged content. Only this user can read it. - user = models.ForeignKey(User, null=False, on_delete=models.CASCADE) + user = models.ForeignKey(User, null=False, on_delete=models.CASCADE) # noqa: DJ012 created = models.DateTimeField(null=False, auto_now_add=True) # What this StagedContent is for (e.g. "clipboard" for clipboard) purpose = models.CharField(max_length=64) diff --git a/openedx/core/djangoapps/content_staging/tasks.py b/openedx/core/djangoapps/content_staging/tasks.py index afa0332dd237..d676ad6acbcc 100644 --- a/openedx/core/djangoapps/content_staging/tasks.py +++ b/openedx/core/djangoapps/content_staging/tasks.py @@ -17,7 +17,7 @@ @shared_task(base=LoggedTask) @set_code_owner_attribute -def delete_expired_clipboards(staged_content_ids: list[int]): +def delete_expired_clipboards(staged_content_ids: list[StagedContent.ID]): """ A Celery task to delete StagedContent clipboard entries that are no longer relevant. diff --git a/openedx/core/djangoapps/content_staging/tests/test_clipboard.py b/openedx/core/djangoapps/content_staging/tests/test_clipboard.py index 9be402dd7efa..36a55e0a9916 100644 --- a/openedx/core/djangoapps/content_staging/tests/test_clipboard.py +++ b/openedx/core/djangoapps/content_staging/tests/test_clipboard.py @@ -3,6 +3,7 @@ Tests for the clipboard functionality """ from textwrap import dedent +from typing import cast from xml.etree import ElementTree from rest_framework.test import APIClient @@ -294,7 +295,7 @@ def test_copy_static_assets(self): # Validate the response: assert response.status_code == 200 response_data = response.json() - staged_content_id = response_data["content"]["id"] + staged_content_id = cast(python_api.StagedContentID, response_data["content"]["id"]) olx_str = python_api.get_staged_content_olx(staged_content_id) assert '' in olx_str static_assets = python_api.get_staged_content_static_files(staged_content_id) From a680f189c76f305777ce6db14480f8ca8082b8a9 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 9 Apr 2026 10:46:16 -0700 Subject: [PATCH 4/6] chore: misc typing improvements + type-check `helpers.py` in CMS --- cms/djangoapps/contentstore/helpers.py | 5 ++-- mypy.ini | 1 + .../content_staging/tests/test_clipboard.py | 25 +++++++++++-------- openedx/core/types/__init__.py | 2 +- openedx/core/types/user.py | 8 +++--- 5 files changed, 23 insertions(+), 18 deletions(-) diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index 1a0d697d7555..7332fac06904 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -39,6 +39,7 @@ from openedx.core.djangoapps.content_tagging.types import TagValuesByObjectIdDict from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.video_config.transcripts_utils import Transcript, build_components_import_path +from openedx.core.types import AuthUser as UserType from xmodule.contentstore.content import StaticContent from xmodule.contentstore.django import contentstore from xmodule.exceptions import NotFoundError @@ -454,7 +455,7 @@ def _fetch_and_set_upstream_link( copied_from_block: str, copied_from_version_num: int, temp_xblock: XBlock, - user: User + user: UserType, ): """ Fetch and set upstream link for the given xblock which is being pasted. This function handles following cases: @@ -515,7 +516,7 @@ def _import_xml_node_to_parent( # The modulestore we're using store, # The user who is performing this operation - user: User, + user: UserType, # Hint to use as usage ID (block_id) for the new XBlock slug_hint: str | None = None, # Content tags applied to the source XBlock(s) diff --git a/mypy.ini b/mypy.ini index f45a46d14acb..6e36cde13e6c 100644 --- a/mypy.ini +++ b/mypy.ini @@ -9,6 +9,7 @@ files = cms/lib/xblock, cms/djangoapps/contentstore/rest_api/v2/views, cms/djangoapps/contentstore/xblock_storage_handlers, + cms/djangoapps/contentstore/helpers.py, cms/djangoapps/modulestore_migrator, openedx/core/djangoapps/content/learning_sequences, # FIXME: need to solve type issues and add 'search' app here: diff --git a/openedx/core/djangoapps/content_staging/tests/test_clipboard.py b/openedx/core/djangoapps/content_staging/tests/test_clipboard.py index 36a55e0a9916..4c26f2c97cca 100644 --- a/openedx/core/djangoapps/content_staging/tests/test_clipboard.py +++ b/openedx/core/djangoapps/content_staging/tests/test_clipboard.py @@ -35,7 +35,7 @@ class ClipboardTestCase(ModuleStoreTestCase): Test Clipboard functionality """ - def test_empty_clipboard(self): + def test_empty_clipboard(self) -> None: """ When a user has no content on their clipboard, we get an empty 200 response """ @@ -70,7 +70,7 @@ def _setup_course(self): return (course_key, client) - def test_copy_video(self): + def test_copy_video(self) -> None: """ Test copying a video from the course, and retrieve it using the REST API """ @@ -103,7 +103,7 @@ def test_copy_video(self): # Now if we GET the clipboard again, the GET response should exactly equal the last POST response: assert client.get(CLIPBOARD_ENDPOINT).json() == response_data - def test_copy_video_python_get(self): + def test_copy_video_python_get(self) -> None: """ Test copying a video from the course, and retrieve it using the python API """ @@ -126,9 +126,10 @@ def test_copy_video_python_get(self): assert clipboard_data.content.display_name == "default" # Test the actual OLX in the clipboard: olx_data = python_api.get_staged_content_olx(clipboard_data.content.id) + assert olx_data is not None self.assertXmlEqual(olx_data, SAMPLE_VIDEO_OLX) - def test_copy_html(self): + def test_copy_html(self) -> None: """ Test copying an HTML XBlock from the course """ @@ -166,7 +167,7 @@ def test_copy_html(self): # Now if we GET the clipboard again, the GET response should exactly equal the last POST response: assert client.get(CLIPBOARD_ENDPOINT).json() == response_data - def test_copy_unit(self): + def test_copy_unit(self) -> None: """ Test copying a unit (vertical block) from the course """ @@ -242,7 +243,7 @@ def test_copy_unit(self): # Now if we GET the clipboard again, the GET response should exactly equal the last POST response: assert client.get(CLIPBOARD_ENDPOINT).json() == response_data - def test_copy_several_things(self): + def test_copy_several_things(self) -> None: """ Test that the clipboard only holds one thing at a time. """ @@ -275,7 +276,7 @@ def test_copy_several_things(self): # The OLX link from the video will no longer work: assert client.get(old_olx_url).status_code == 404 - def test_copy_static_assets(self): + def test_copy_static_assets(self) -> None: """ Test copying an HTML from the course that references a static asset file. """ @@ -297,6 +298,7 @@ def test_copy_static_assets(self): response_data = response.json() staged_content_id = cast(python_api.StagedContentID, response_data["content"]["id"]) olx_str = python_api.get_staged_content_olx(staged_content_id) + assert olx_str is not None assert '' in olx_str static_assets = python_api.get_staged_content_static_files(staged_content_id) @@ -307,7 +309,7 @@ def test_copy_static_assets(self): data=None, )] - def test_copy_static_assets_nonexistent(self): + def test_copy_static_assets_nonexistent(self) -> None: """ Test copying a HTML block which references non-existent static assets. """ @@ -332,11 +334,12 @@ def test_copy_static_assets_nonexistent(self): response_data = response.json() staged_content_id = response_data["content"]["id"] olx_str = python_api.get_staged_content_olx(staged_content_id) + assert olx_str is not None assert '' in olx_str static_assets = python_api.get_staged_content_static_files(staged_content_id) assert static_assets == [] - def test_no_course_permission(self): + def test_no_course_permission(self) -> None: """ Test that a user without read access cannot copy items in a course """ @@ -353,7 +356,7 @@ def test_no_course_permission(self): response = nonstaff_client.get(CLIPBOARD_ENDPOINT) assert response.json()["content"] is None - def test_no_stealing_clipboard_content(self): + def test_no_stealing_clipboard_content(self) -> None: """ Test that a user cannot see another user's clipboard """ @@ -370,7 +373,7 @@ def test_no_stealing_clipboard_content(self): response = nonstaff_client.get(olx_url) assert response.status_code == 403 - def assertXmlEqual(self, xml_str_a: str, xml_str_b: str): + def assertXmlEqual(self, xml_str_a: str, xml_str_b: str) -> None: """ Assert that the given XML strings are equal, ignoring attribute order and some whitespace variations. """ a = ElementTree.canonicalize(xml_str_a, strip_text=True) b = ElementTree.canonicalize(xml_str_b, strip_text=True) diff --git a/openedx/core/types/__init__.py b/openedx/core/types/__init__.py index 96c043603c28..1ff8eba86c88 100644 --- a/openedx/core/types/__init__.py +++ b/openedx/core/types/__init__.py @@ -3,4 +3,4 @@ """ from django.contrib.admin import display as admin_display # noqa: F401 -from .user import User # noqa: F401 +from .user import AuthUser, User # noqa: F401 diff --git a/openedx/core/types/user.py b/openedx/core/types/user.py index e714029e2592..e54c01ba7847 100644 --- a/openedx/core/types/user.py +++ b/openedx/core/types/user.py @@ -3,9 +3,9 @@ """ from __future__ import annotations -import typing as t - import django.contrib.auth.models -AuthUser: t.TypeAlias = django.contrib.auth.models.User # noqa: UP040 -User: t.TypeAlias = django.contrib.auth.models.User | django.contrib.auth.models.AnonymousUser # noqa: UP040 +# base type for an authenticated user +type AuthUser = django.contrib.auth.models.User +# base type for a generic user making an HTTP request, which may or may not be authenticated: +type User = AuthUser | django.contrib.auth.models.AnonymousUser From 64db8380b0bc7975d40cb877cd9779dabc775e76 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 9 Apr 2026 12:14:33 -0700 Subject: [PATCH 5/6] chore: explain mypy error and suppress it for now --- cms/djangoapps/contentstore/helpers.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index 7332fac06904..8696e4f997c2 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -283,7 +283,10 @@ def create_usage(self, def_id) -> UsageKey: def create_definition(self, block_type, slug=None) -> DefinitionLocator: """ Generate a new definition_id for an XBlock """ # Note: Split modulestore will detect this temporary ID and create a new definition ID when the XBlock is saved. - return DefinitionLocator(block_type, LocalId(block_type)) + # FIXME: The DefinitionLocator technically only accepts an ObjectId (or a str representing an ObjectId), but + # this code relies on passing a LocalId and having it save the LocalId object as its `definition_id`. We should + # either change this in the future or update DefinitionLocator to support LocalId-typed definition IDs. + return DefinitionLocator(block_type, LocalId(block_type)) # type: ignore[arg-type] @frozen From dbe8482e80fb40cdd2354f93e126917fc4322504 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 9 Apr 2026 13:01:07 -0700 Subject: [PATCH 6/6] chore: use .id instead of .pk --- cms/djangoapps/modulestore_migrator/tasks.py | 6 +++--- cms/pytest.ini | 4 ++++ .../core/djangoapps/content_libraries/api/blocks.py | 12 ++++++------ .../content_libraries/api/container_metadata.py | 6 +++--- .../djangoapps/content_libraries/api/containers.py | 8 ++++---- .../djangoapps/content_libraries/api/libraries.py | 4 ++-- .../xblock/runtime/openedx_content_runtime.py | 2 +- pyproject.toml | 4 ++++ 8 files changed, 27 insertions(+), 19 deletions(-) diff --git a/cms/djangoapps/modulestore_migrator/tasks.py b/cms/djangoapps/modulestore_migrator/tasks.py index 17bd5dd5bc95..b54b9191e6ba 100644 --- a/cms/djangoapps/modulestore_migrator/tasks.py +++ b/cms/djangoapps/modulestore_migrator/tasks.py @@ -345,13 +345,13 @@ def _import_structure( used_component_keys=set( LibraryUsageLocatorV2(target_library.key, block_type, block_id) # type: ignore[abstract] for block_type, block_id - in content_api.get_components(migration.target.pk).values_list( + in content_api.get_components(migration.target.id).values_list( "component_type__name", "local_key" ) ), used_container_slugs=set( content_api.get_containers( - migration.target.pk + migration.target.id ).values_list("publishable_entity__key", flat=True) ), previous_block_migrations=( @@ -359,7 +359,7 @@ def _import_structure( if source_data.previous_migration else {} ), - target_package_id=migration.target.pk, + target_package_id=migration.target.id, target_library_key=target_library.key, source_context_key=source_data.source_root_usage_key.course_key, content_by_filename=content_by_filename, diff --git a/cms/pytest.ini b/cms/pytest.ini index 368ceb257099..b715fdb8e624 100644 --- a/cms/pytest.ini +++ b/cms/pytest.ini @@ -17,6 +17,10 @@ filterwarnings = # ABC deprecation Warning comes from libsass ignore:Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated.*:DeprecationWarning:sass ignore:'etree' is deprecated. Use 'xml.etree.ElementTree' instead.:DeprecationWarning:wiki + # We sometimes use DeprecationWarning to enourage `.id` instead of `.pk` since there are some cases where we cannot + # override the type of `.pk`, but we can always customize `.id` (e.g. using TypedBigAutoField). But we don't want to + # see those warnings when Django is using `.pk` internally, which is fine. + ignore:Use \.id instead:DeprecationWarning:django\..* norecursedirs = envs python_classes = diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index 4b61c50ca89f..2ddc06fde254 100644 --- a/openedx/core/djangoapps/content_libraries/api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/api/blocks.py @@ -244,7 +244,7 @@ def set_library_block_olx(usage_key: LibraryUsageLocatorV2, new_olx_str: str) -> created=now, ) new_component_version = content_api.create_next_component_version( - component.pk, + component.id, title=new_title, media_to_replace={ 'block.xml': new_content.pk, @@ -728,7 +728,7 @@ def send_block_deleted_signal(): affected_collections = content_api.get_entity_collections(component.learning_package_id, component.key) affected_containers = get_containers_contains_item(usage_key) - content_api.soft_delete_draft(component.pk, deleted_by=user_id) + content_api.soft_delete_draft(component.id, deleted_by=user_id) send_block_deleted_signal() @@ -774,7 +774,7 @@ def restore_library_block(usage_key: LibraryUsageLocatorV2, user_id: int | None # Set draft version back to the latest available component version id. content_api.set_draft_version( - component.pk, + component.id, component.versioning.latest.pk, set_by=user_id, ) @@ -910,7 +910,7 @@ def add_library_block_static_asset_file( with transaction.atomic(): component_version = content_api.create_next_component_version( - component.pk, + component.id, media_to_replace={file_path: file_content}, created=datetime.now(tz=timezone.utc), # noqa: UP017 created_by=user.id if user else None, @@ -957,8 +957,8 @@ def delete_library_block_static_asset_file(usage_key, file_path, user=None): now = datetime.now(tz=timezone.utc) # noqa: UP017 with transaction.atomic(): - component_version = content_api.create_next_component_version( # noqa: F841 - component.pk, + content_api.create_next_component_version( + component.id, media_to_replace={file_path: None}, created=now, created_by=user.id if user else None, diff --git a/openedx/core/djangoapps/content_libraries/api/container_metadata.py b/openedx/core/djangoapps/content_libraries/api/container_metadata.py index b154d7ab4fa2..5bb8bcd50ae4 100644 --- a/openedx/core/djangoapps/content_libraries/api/container_metadata.py +++ b/openedx/core/djangoapps/content_libraries/api/container_metadata.py @@ -110,7 +110,7 @@ def from_container(cls, library_key, container: Container, associated_collection published_by=published_by, last_draft_created=last_draft_created, last_draft_created_by=last_draft_created_by, - has_unpublished_changes=content_api.contains_unpublished_changes(container.pk), + has_unpublished_changes=content_api.contains_unpublished_changes(container.id), tags_count=tags.get(str(container_key), 0), collections=associated_collections or [], ) @@ -260,7 +260,7 @@ def _get_containers_with_entities( for member in members: qs = qs.union( content_api.get_containers_with_entity( - member.entity.pk, + member.entity.id, ignore_pinned=ignore_pinned, ) ) @@ -338,7 +338,7 @@ def create( container=entity, ), display_name=entity.versioning.draft.title, - has_unpublished_changes=content_api.contains_unpublished_changes(entity.pk), + has_unpublished_changes=content_api.contains_unpublished_changes(entity.id), container=entity, component=None, ) diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index 15d8d19833a5..5db34dc753da 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -234,7 +234,7 @@ def send_container_deleted_signal(): container_key, published=False, ) - content_api.soft_delete_draft(container.pk) + content_api.soft_delete_draft(container.id) send_container_deleted_signal() @@ -294,7 +294,7 @@ def restore_container(container_key: LibraryContainerLocator) -> None: container.key, ) - content_api.set_draft_version(container.pk, container.versioning.latest.pk) + content_api.set_draft_version(container.id, container.versioning.latest.pk) # Fetch related containers after restore affected_containers = get_containers_contains_item(container_key) # Get children containers or components to update their index data @@ -441,7 +441,7 @@ def get_containers_contains_item(key: LibraryUsageLocatorV2 | LibraryContainerLo [ 🛑 UNSTABLE ] Get containers that contains the item, that can be a component or another container. """ entity = get_entity_from_key(key) - containers = content_api.get_containers_with_entity(entity.pk).select_related("container_type") + containers = content_api.get_containers_with_entity(entity.id).select_related("container_type") return [ContainerMetadata.from_container(key.lib_key, container) for container in containers] @@ -460,7 +460,7 @@ def publish_container_changes( learning_package = content_library.learning_package assert learning_package # The core publishing API is based on draft objects, so find the draft that corresponds to this container: - drafts_to_publish = content_api.get_all_drafts(learning_package.id).filter(entity__pk=container.pk) + drafts_to_publish = content_api.get_all_drafts(learning_package.id).filter(entity__pk=container.id) # Publish the container, which will also auto-publish any unpublished child components: publish_log = content_api.publish_from_drafts( learning_package.id, diff --git a/openedx/core/djangoapps/content_libraries/api/libraries.py b/openedx/core/djangoapps/content_libraries/api/libraries.py index cb232ae06f80..87842e837750 100644 --- a/openedx/core/djangoapps/content_libraries/api/libraries.py +++ b/openedx/core/djangoapps/content_libraries/api/libraries.py @@ -410,7 +410,7 @@ def get_library(library_key: LibraryLocatorV2) -> ContentLibraryMetadata: license=ref.license, created=learning_package.created, updated=learning_package.updated, - learning_package_id=learning_package.pk, + learning_package_id=learning_package.id, ) @@ -494,7 +494,7 @@ def create_library( allow_public_learning=ref.allow_public_learning, allow_public_read=ref.allow_public_read, license=library_license, - learning_package_id=ref.learning_package.pk, # type: ignore[union-attr] + learning_package_id=ref.learning_package.id, # type: ignore[union-attr] ) diff --git a/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py b/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py index e943c85057ac..3a90fb27a5f7 100644 --- a/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py @@ -302,7 +302,7 @@ def save_block(self, block): created=now, ) content_api.create_next_component_version( - component.pk, + component.id, title=block.display_name, media_to_replace={ "block.xml": media.id, diff --git a/pyproject.toml b/pyproject.toml index ec310ef390cb..40f0d3b0fcb8 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -199,6 +199,10 @@ filterwarnings = [ # ABC deprecation Warning comes from libsass "ignore:Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated.*:DeprecationWarning:sass", "ignore:'etree' is deprecated. Use 'xml.etree.ElementTree' instead.:DeprecationWarning:wiki", + # We sometimes use DeprecationWarning to enourage `.id` instead of `.pk` since there are some cases where we cannot + # override the type of `.pk`, but we can always customize `.id` (e.g. using TypedBigAutoField). But we don't want to + # see those warnings when Django is using `.pk` internally, which is fine. + "ignore:Use \\.id instead:DeprecationWarning:django\\..*", ] junit_family = "xunit2" norecursedirs = ". .* *.egg build conf dist node_modules test_root cms/envs lms/envs openedx/envs"