diff --git a/src/openedx_catalog/api_impl.py b/src/openedx_catalog/api_impl.py index 411da7f22..03c9a3ee7 100644 --- a/src/openedx_catalog/api_impl.py +++ b/src/openedx_catalog/api_impl.py @@ -30,11 +30,11 @@ def get_catalog_course(*, org_code: str, course_code: str) -> CatalogCourse: ... @overload def get_catalog_course(*, key_str: str) -> CatalogCourse: ... @overload -def get_catalog_course(*, pk: int) -> CatalogCourse: ... +def get_catalog_course(*, pk: CatalogCourse.ID) -> CatalogCourse: ... def get_catalog_course( - pk: int | None = None, + pk: CatalogCourse.ID | None = None, key_str: str = "", org_code: str = "", course_code: str = "", @@ -61,7 +61,7 @@ def get_catalog_course( def update_catalog_course( - catalog_course: CatalogCourse | int, + catalog_course: CatalogCourse | CatalogCourse.ID, *, title: str | None = None, # Specify a string to change the title (display name). # The short language code (one of settings.ALL_LANGUAGES), e.g. "en", "es", "zh_HANS" @@ -88,7 +88,7 @@ def update_catalog_course( cc.save(update_fields=update_fields) -def delete_catalog_course(catalog_course: CatalogCourse | int) -> None: +def delete_catalog_course(catalog_course: CatalogCourse | CatalogCourse.ID) -> None: """ Delete a `CatalogCourse`. This will fail with a `ProtectedError` if any runs exist. diff --git a/src/openedx_catalog/models/catalog_course.py b/src/openedx_catalog/models/catalog_course.py index ad928731f..a6b7d3fac 100644 --- a/src/openedx_catalog/models/catalog_course.py +++ b/src/openedx_catalog/models/catalog_course.py @@ -3,6 +3,7 @@ """ import logging +from typing import NewType from django.conf import settings from django.contrib import admin @@ -12,7 +13,7 @@ from django.utils.translation import gettext_lazy as _ from organizations.models import Organization # type: ignore[import] -from openedx_django_lib.fields import case_insensitive_char_field, case_sensitive_char_field +from openedx_django_lib.fields import TypedBigAutoField, case_insensitive_char_field, case_sensitive_char_field from openedx_django_lib.validators import validate_utc_datetime log = logging.getLogger(__name__) @@ -59,7 +60,13 @@ class CatalogCourse(models.Model): courses in all instances of Open edX will need.) """ - id = models.BigAutoField( + CatalogCourseID = NewType("CatalogCourseID", int) + type ID = CatalogCourseID + + class IDField(TypedBigAutoField[ID]): # Boilerplate for fully-typed ID field. + pass + + id = IDField( primary_key=True, verbose_name=_("Primary Key"), help_text=_("The internal database ID for this catalog course. Should not be exposed to users nor in APIs."), diff --git a/src/openedx_catalog/models/course_run.py b/src/openedx_catalog/models/course_run.py index 5db24b26e..3f298df7a 100644 --- a/src/openedx_catalog/models/course_run.py +++ b/src/openedx_catalog/models/course_run.py @@ -3,6 +3,7 @@ """ import logging +from typing import NewType from django.contrib import admin from django.core.exceptions import ValidationError @@ -15,7 +16,7 @@ from opaque_keys.edx.django.models import CourseKeyField from opaque_keys.edx.locator import CourseLocator -from openedx_django_lib.fields import case_insensitive_char_field, case_sensitive_char_field +from openedx_django_lib.fields import TypedBigAutoField, case_insensitive_char_field, case_sensitive_char_field from openedx_django_lib.validators import validate_utc_datetime from .catalog_course import CatalogCourse @@ -79,8 +80,14 @@ class CourseRun(models.Model): learning package. """ + CourseRunID = NewType("CourseRunID", int) + type ID = CourseRunID + + class IDField(TypedBigAutoField[ID]): # Boilerplate for fully-typed ID field. + pass + # Use this field for relationships within the database: - id = models.BigAutoField( + id = IDField( primary_key=True, verbose_name=_("Primary Key"), help_text=_("The internal database ID for this course. Should not be exposed to users nor in APIs."), diff --git a/src/openedx_content/applets/backup_restore/zipper.py b/src/openedx_content/applets/backup_restore/zipper.py index 29b9638e9..5f7b2457e 100644 --- a/src/openedx_content/applets/backup_restore/zipper.py +++ b/src/openedx_content/applets/backup_restore/zipper.py @@ -176,7 +176,7 @@ def get_publishable_entities(self) -> QuerySet[PublishableEntity]: Retrieve the publishable entities associated with the learning package. Prefetches related data for efficiency. """ - lp_id = self.learning_package.pk + lp_id = self.learning_package.id publishable_entities: QuerySet[PublishableEntity] = publishing_api.get_publishable_entities(lp_id) return ( publishable_entities # type: ignore[no-redef] @@ -210,7 +210,7 @@ def get_collections(self) -> QuerySet[Collection]: Get the collections associated with the learning package. """ return ( - collections_api.get_collections(self.learning_package.pk) + collections_api.get_collections(self.learning_package.id) .prefetch_related("entities") ) @@ -512,7 +512,7 @@ def __init__(self, zipf: zipfile.ZipFile, key: str | None = None, user: UserType self.user = user self.user_id = getattr(self.user, "id", None) self.lp_key = key # If provided, use this key for the restored learning package - self.learning_package_id: int | None = None # Will be set upon restoration + self.learning_package_id: LearningPackage.ID | None = None # Will be set upon restoration self.utc_now: datetime = datetime.now(timezone.utc) self.component_types_cache: dict[tuple[str, str], ComponentType] = {} self.errors: list[dict[str, Any]] = [] diff --git a/src/openedx_content/applets/collections/api.py b/src/openedx_content/applets/collections/api.py index 8ab8d9cbf..b57b3bea8 100644 --- a/src/openedx_content/applets/collections/api.py +++ b/src/openedx_content/applets/collections/api.py @@ -10,7 +10,7 @@ from ..publishing import api as publishing_api from ..publishing.models import PublishableEntity -from .models import Collection, CollectionPublishableEntity +from .models import Collection, CollectionPublishableEntity, LearningPackage # The public API that will be re-exported by openedx_content.api # is listed in the __all__ entries below. Internal helper functions that are @@ -33,7 +33,7 @@ def create_collection( - learning_package_id: int, + learning_package_id: LearningPackage.ID, key: str, *, title: str, @@ -55,7 +55,7 @@ def create_collection( return collection -def get_collection(learning_package_id: int, collection_key: str) -> Collection: +def get_collection(learning_package_id: LearningPackage.ID, collection_key: str) -> Collection: """ Get a Collection by ID """ @@ -63,7 +63,7 @@ def get_collection(learning_package_id: int, collection_key: str) -> Collection: def update_collection( - learning_package_id: int, + learning_package_id: LearningPackage.ID, key: str, *, title: str | None = None, @@ -89,7 +89,7 @@ def update_collection( def delete_collection( - learning_package_id: int, + learning_package_id: LearningPackage.ID, key: str, *, hard_delete=False, @@ -111,7 +111,7 @@ def delete_collection( def restore_collection( - learning_package_id: int, + learning_package_id: LearningPackage.ID, key: str, ) -> Collection: """ @@ -125,7 +125,7 @@ def restore_collection( def add_to_collection( - learning_package_id: int, + learning_package_id: LearningPackage.ID, key: str, entities_qset: QuerySet[PublishableEntity], created_by: int | None = None, @@ -145,7 +145,7 @@ def add_to_collection( invalid_entity = entities_qset.exclude(learning_package_id=learning_package_id).first() if invalid_entity: raise ValidationError( - f"Cannot add entity {invalid_entity.pk} in learning package {invalid_entity.learning_package_id} " + f"Cannot add entity {invalid_entity.id} in learning package {invalid_entity.learning_package_id} " f"to collection {key} in learning package {learning_package_id}." ) @@ -161,7 +161,7 @@ def add_to_collection( def remove_from_collection( - learning_package_id: int, + learning_package_id: LearningPackage.ID, key: str, entities_qset: QuerySet[PublishableEntity], ) -> Collection: @@ -183,7 +183,7 @@ def remove_from_collection( return collection -def get_entity_collections(learning_package_id: int, entity_key: str) -> QuerySet[Collection]: +def get_entity_collections(learning_package_id: LearningPackage.ID, entity_key: str) -> QuerySet[Collection]: """ Get all collections in the given learning package which contain this entity. @@ -196,7 +196,10 @@ def get_entity_collections(learning_package_id: int, entity_key: str) -> QuerySe return entity.collections.filter(enabled=True).order_by("pk") -def get_collection_entities(learning_package_id: int, collection_key: str) -> QuerySet[PublishableEntity]: +def get_collection_entities( + learning_package_id: LearningPackage.ID, + collection_key: str, +) -> QuerySet[PublishableEntity]: """ Returns a QuerySet of PublishableEntities in a Collection. @@ -208,7 +211,7 @@ def get_collection_entities(learning_package_id: int, collection_key: str) -> Qu ).order_by("pk") -def get_collections(learning_package_id: int, enabled: bool | None = True) -> QuerySet[Collection]: +def get_collections(learning_package_id: LearningPackage.ID, enabled: bool | None = True) -> QuerySet[Collection]: """ Get all collections for a given learning package diff --git a/src/openedx_content/applets/components/api.py b/src/openedx_content/applets/components/api.py index 46de5356e..6127cd3ec 100644 --- a/src/openedx_content/applets/components/api.py +++ b/src/openedx_content/applets/components/api.py @@ -25,7 +25,7 @@ from ..media import api as media_api from ..publishing import api as publishing_api -from .models import Component, ComponentType, ComponentVersion, ComponentVersionMedia +from .models import Component, ComponentType, ComponentVersion, ComponentVersionMedia, LearningPackage # The public API that will be re-exported by openedx_content.api # is listed in the __all__ entries below. Internal helper functions that are @@ -96,7 +96,7 @@ def get_or_create_component_type_by_entity_key(entity_key: str) -> tuple[Compone def create_component( - learning_package_id: int, + learning_package_id: LearningPackage.ID, /, component_type: ComponentType, local_key: str, @@ -127,7 +127,7 @@ def create_component( def create_component_version( - component_pk: int, + component_id: Component.ID, /, version_num: int, title: str, @@ -139,7 +139,7 @@ def create_component_version( """ with atomic(): publishable_entity_version = publishing_api.create_publishable_entity_version( - component_pk, + component_id, version_num=version_num, title=title, created=created, @@ -147,13 +147,13 @@ def create_component_version( ) component_version = ComponentVersion.objects.create( publishable_entity_version=publishable_entity_version, - component_id=component_pk, + component_id=component_id, ) return component_version def create_next_component_version( - component_pk: int, + component_id: Component.ID, /, media_to_replace: dict[str, int | None | bytes], created: datetime, @@ -167,7 +167,7 @@ def create_next_component_version( Create a new ComponentVersion based on the most recent version. Args: - component_pk (int): The primary key of the Component to version. + component_id (int): The primary key of the Component to version. media_to_replace (dict): Mapping of file keys to Media IDs, None (for deletion), or bytes (for new file media). created (datetime): The creation timestamp for the new version. @@ -218,7 +218,7 @@ def create_next_component_version( # should pick up from the last edited version. Likewise, a Draft might get # reverted to an earlier version, but we want the latest version_num when # creating the next version. - component = Component.objects.get(pk=component_pk) + component = Component.objects.get(pk=component_id) last_version = component.versioning.latest if last_version is None: next_version_num = 1 @@ -233,7 +233,7 @@ def create_next_component_version( with atomic(): publishable_entity_version = publishing_api.create_publishable_entity_version( - component_pk, + component_id, version_num=next_version_num, title=title, created=created, @@ -241,7 +241,7 @@ def create_next_component_version( ) component_version = ComponentVersion.objects.create( publishable_entity_version=publishable_entity_version, - component_id=component_pk, + component_id=component_id, ) # First copy the new stuff over... for key, media_pk_or_bytes in media_to_replace.items(): @@ -290,7 +290,7 @@ def create_next_component_version( def create_component_and_version( # pylint: disable=too-many-positional-arguments - learning_package_id: int, + learning_package_id: LearningPackage.ID, /, component_type: ComponentType, local_key: str, @@ -313,7 +313,7 @@ def create_component_and_version( # pylint: disable=too-many-positional-argumen can_stand_alone=can_stand_alone, ) component_version = create_component_version( - component.pk, + component.id, version_num=1, title=title, created=created, @@ -322,17 +322,17 @@ def create_component_and_version( # pylint: disable=too-many-positional-argumen return (component, component_version) -def get_component(component_pk: int, /) -> Component: +def get_component(component_id: Component.ID, /) -> Component: """ Get Component by its primary key. This is the same as the PublishableEntity's ID primary key. """ - return Component.with_publishing_relations.get(pk=component_pk) + return Component.with_publishing_relations.get(pk=component_id) def get_component_by_key( - learning_package_id: int, + learning_package_id: LearningPackage.ID, /, namespace: str, type_name: str, @@ -367,7 +367,7 @@ def get_component_version_by_uuid(uuid: UUID) -> ComponentVersion: def component_exists_by_key( - learning_package_id: int, + learning_package_id: LearningPackage.ID, /, namespace: str, type_name: str, @@ -392,7 +392,7 @@ def component_exists_by_key( def get_components( # pylint: disable=too-many-positional-arguments - learning_package_id: int, + learning_package_id: LearningPackage.ID, /, draft: bool | None = None, published: bool | None = None, @@ -435,7 +435,7 @@ def get_components( # pylint: disable=too-many-positional-arguments def get_collection_components( - learning_package_id: int, + learning_package_id: LearningPackage.ID, collection_key: str, ) -> QuerySet[Component]: """ diff --git a/src/openedx_content/applets/components/models.py b/src/openedx_content/applets/components/models.py index de93680fd..6fbab2ba3 100644 --- a/src/openedx_content/applets/components/models.py +++ b/src/openedx_content/applets/components/models.py @@ -17,15 +17,21 @@ """ from __future__ import annotations -from typing import ClassVar +from typing import ClassVar, NewType, cast from django.db import models +from typing_extensions import deprecated from openedx_django_lib.fields import case_sensitive_char_field, key_field from openedx_django_lib.managers import WithRelationsManager from ..media.models import Media -from ..publishing.models import LearningPackage, PublishableEntityMixin, PublishableEntityVersionMixin +from ..publishing.models import ( + LearningPackage, + PublishableEntity, + PublishableEntityMixin, + PublishableEntityVersionMixin, +) __all__ = [ "ComponentType", @@ -123,6 +129,25 @@ class Component(PublishableEntityMixin): Make a foreign key to the Component model when you need a stable reference that will exist for as long as the LearningPackage itself exists. """ + + ComponentID = NewType("ComponentID", PublishableEntity.ID) + type ID = ComponentID + + @property + def id(self) -> ID: + return cast(Component.ID, self.publishable_entity_id) + + @property + @deprecated("Use .id instead") + def pk(self): + """Mark the .pk attribute as deprecated""" + # Note: Django-Stubs forces mypy to identify the `.pk` attribute of this model as having 'Any' type (due to our + # use of a OneToOneField primary key), and this is impossible for us to override, so we prefer to use + # `.id` which we can control fully. + # Since Django uses '.pk' internally, we have to make sure it still works, however. So the best we can do is + # override this with a deprecated marker, so it shows a warning in developer's IDEs like VS Code. + return self.id + # Set up our custom manager. It has the same API as the default one, but selects related objects by default. objects: ClassVar[WithRelationsManager[Component]] = WithRelationsManager( # type: ignore[assignment] 'component_type' diff --git a/src/openedx_content/applets/containers/api.py b/src/openedx_content/applets/containers/api.py index 002b39926..9fe192ea8 100644 --- a/src/openedx_content/applets/containers/api.py +++ b/src/openedx_content/applets/containers/api.py @@ -103,7 +103,7 @@ class ParsedEntityReference: role, but is only used when reading data out, not mutating containers. """ - entity_pk: int + entity_pk: PublishableEntity.ID version_pk: int | None = None @staticmethod @@ -126,7 +126,7 @@ def parse(entities: EntityListInput) -> list[ParsedEntityReference]: obj = obj.publishable_entity_version if isinstance(obj, PublishableEntity): - new_list.append(ParsedEntityReference(entity_pk=obj.pk)) + new_list.append(ParsedEntityReference(entity_pk=obj.id)) elif isinstance(obj, PublishableEntityVersion): new_list.append(ParsedEntityReference(entity_pk=obj.entity_id, version_pk=obj.pk)) else: @@ -135,7 +135,7 @@ def parse(entities: EntityListInput) -> list[ParsedEntityReference]: def create_container( - learning_package_id: int, + learning_package_id: LearningPackage.ID, key: str, created: datetime, created_by: int | None, @@ -190,7 +190,7 @@ def create_entity_list() -> EntityList: def create_entity_list_with_rows( parsed_entities: list[ParsedEntityReference], *, - learning_package_id: int | None, + learning_package_id: LearningPackage.ID | None, ) -> EntityList: """ [ 🛑 UNSTABLE ] @@ -281,7 +281,7 @@ def _create_container_version( ) container_version = version_type.objects.create( publishable_entity_version=publishable_entity_version, - container_id=container.pk, + container_id=container.id, entity_list=entity_list, # This could accept **kwargs in the future if we have additional type-specific fields? ) @@ -290,7 +290,7 @@ def _create_container_version( def create_container_version( - container_id: int, + container_id: Container.ID, version_num: int, *, title: str, @@ -338,7 +338,7 @@ def create_container_version( def create_container_and_version( - learning_package_id: int, + learning_package_id: LearningPackage.ID, key: str, *, title: str, @@ -375,7 +375,7 @@ def create_container_and_version( container_cls=container_cls, ) container_version: ContainerVersionModel = create_container_version( # type: ignore[assignment] - container.pk, + container.id, 1, title=title, entities=entities or [], @@ -394,7 +394,7 @@ class ChildrenEntitiesAction(Enum): def create_next_entity_list( - learning_package_id: int, + learning_package_id: LearningPackage.ID, last_version: ContainerVersion, entities: EntityListInput, entities_action: ChildrenEntitiesAction = ChildrenEntitiesAction.REPLACE, @@ -452,7 +452,7 @@ def create_next_entity_list( def create_next_container_version( - container: Container | int, + container: Container | Container.ID, /, *, title: str | None = None, @@ -474,7 +474,7 @@ def create_next_container_version( * We pin to different versions of the Container. Args: - container_pk: The ID of the container to create the next version of. + id: The ID of the container to create the next version of. title: The title of the container. None to keep the current title. entities: List of the entities that will comprise the entity list, in order. Pass `PublishableEntityVersion` or objects that use @@ -534,7 +534,7 @@ def create_next_container_version( return next_container_version -def get_container(pk: int) -> Container: +def get_container(pk: Container.ID) -> Container: """ [ 🛑 UNSTABLE ] Get a container by its primary key. @@ -564,7 +564,7 @@ def get_container_version(container_version_pk: int) -> ContainerVersion: return ContainerVersion.objects.get(pk=container_version_pk) -def get_container_by_key(learning_package_id: int, /, key: str) -> Container: +def get_container_by_key(learning_package_id: LearningPackage.ID, /, key: str) -> Container: """ [ 🛑 UNSTABLE ] Get a container by its learning package and primary key. @@ -606,7 +606,7 @@ def get_container_subclass(type_code: str, /) -> ContainerSubclass: return Container.subclass_for_type_code(type_code) -def get_container_type_code_of(container: Container | int, /) -> str: +def get_container_type_code_of(container: Container | Container.ID, /) -> str: """Get the type of a container, as a string - e.g. "unit".""" if isinstance(container, int): container = get_container(container) @@ -614,7 +614,7 @@ def get_container_type_code_of(container: Container | int, /) -> str: return container.container_type.type_code -def get_container_subclass_of(container: Container | int, /) -> ContainerSubclass: +def get_container_subclass_of(container: Container | Container.ID, /) -> ContainerSubclass: """ Get the type of a container. @@ -628,7 +628,7 @@ def get_container_subclass_of(container: Container | int, /) -> ContainerSubclas def get_containers( - learning_package_id: int, + learning_package_id: LearningPackage.ID, include_deleted: bool | None = False, ) -> QuerySet[Container]: """ @@ -677,7 +677,7 @@ def get_entities_in_container( # Very minor optimization: reload the container with related 1:1 entities container = Container.objects.select_related( "publishable_entity__published__version__containerversion__entity_list" - ).get(pk=container.pk) + ).get(pk=container.id) container_version = container.versioning.published select_related = ["entity__published__version"] if select_related_version: @@ -686,7 +686,7 @@ def get_entities_in_container( # Very minor optimization: reload the container with related 1:1 entities container = Container.objects.select_related( "publishable_entity__draft__version__containerversion__entity_list" - ).get(pk=container.pk) + ).get(pk=container.id) container_version = container.versioning.draft select_related = ["entity__draft__version"] if select_related_version: @@ -751,7 +751,7 @@ def get_entities_in_container_as_of( return container_version, entity_list -def contains_unpublished_changes(container_or_pk: Container | int, /) -> bool: +def contains_unpublished_changes(container_or_pk: Container | Container.ID, /) -> bool: """ [ 🛑 UNSTABLE ] Check recursively if a container has any unpublished changes. @@ -773,7 +773,7 @@ def contains_unpublished_changes(container_or_pk: Container | int, /) -> bool: container_id = container_or_pk else: assert isinstance(container_or_pk, Container) - container_id = container_or_pk.pk + container_id = container_or_pk.id container = ( Container.objects.select_related("publishable_entity__draft__draft_log_record") .select_related("publishable_entity__published__publish_log_record") @@ -800,7 +800,7 @@ def contains_unpublished_changes(container_or_pk: Container | int, /) -> bool: def get_containers_with_entity( - publishable_entity_pk: int, + publishable_entity_pk: PublishableEntity.ID, *, ignore_pinned=False, published=False, diff --git a/src/openedx_content/applets/containers/models.py b/src/openedx_content/applets/containers/models.py index 3661f1f61..ece19c799 100644 --- a/src/openedx_content/applets/containers/models.py +++ b/src/openedx_content/applets/containers/models.py @@ -5,10 +5,11 @@ from __future__ import annotations from functools import cached_property -from typing import final +from typing import NewType, cast, final from django.core.exceptions import ValidationError from django.db import models +from typing_extensions import deprecated from openedx_django_lib.fields import case_sensitive_char_field @@ -161,6 +162,9 @@ class Container(PublishableEntityMixin): entities for different learners or at different times. """ + ContainerID = NewType("ContainerID", PublishableEntity.ID) + type ID = ContainerID + type_code: str # Subclasses must override this, e.g. "unit" # olx_code: the OLX for XML serialization. Subclasses _may_ override this. # Only used in openedx-platform at the moment. We'll likely have to replace this with something more sophisticated. @@ -175,6 +179,21 @@ class Container(PublishableEntityMixin): editable=False, ) + @property + def id(self) -> ID: + return cast(Container.ID, self.publishable_entity_id) + + @property + @deprecated("Use .id instead") + def pk(self): + """Mark the .pk attribute as deprecated""" + # Note: Django-Stubs forces mypy to identify the `.pk` attribute of this model as having 'Any' type (due to our + # use of a OneToOneField primary key), and this is impossible for us to override, so we prefer to use + # `.id` which we can control fully. + # Since Django uses '.pk' internally, we have to make sure it still works, however. So the best we can do is + # override this with a deprecated marker, so it shows a warning in developer's IDEs like VS Code. + return self.id + @classmethod def validate_entity(cls, entity: PublishableEntity) -> None: """ @@ -280,5 +299,5 @@ def clean(self): called if anything is edited via a ModelForm like the Django admin. """ super().clean() - if self.container_id != self.publishable_entity_version.entity.container.pk: # pylint: disable=no-member + if self.container_id != self.publishable_entity_version.entity.container.id: # pylint: disable=no-member raise ValidationError("Inconsistent foreign keys to Container") diff --git a/src/openedx_content/applets/media/api.py b/src/openedx_content/applets/media/api.py index c588f8a3f..90d0ff801 100644 --- a/src/openedx_content/applets/media/api.py +++ b/src/openedx_content/applets/media/api.py @@ -14,6 +14,7 @@ from openedx_django_lib.fields import create_hash_digest +from ..publishing.models import LearningPackage from .models import Media, MediaType # The public API that will be re-exported by openedx_content.api @@ -81,7 +82,7 @@ def get_media(media_id: int, /) -> Media: def get_or_create_text_media( - learning_package_id: int, + learning_package_id: LearningPackage.ID, media_type_id: int, /, text: str, @@ -135,7 +136,7 @@ def get_or_create_text_media( def get_or_create_file_media( - learning_package_id: int, + learning_package_id: LearningPackage.ID, media_type_id: int, /, data: bytes, diff --git a/src/openedx_content/applets/publishing/api.py b/src/openedx_content/applets/publishing/api.py index 88143f520..dee0e7370 100644 --- a/src/openedx_content/applets/publishing/api.py +++ b/src/openedx_content/applets/publishing/api.py @@ -69,7 +69,7 @@ ] -def get_learning_package(learning_package_id: int, /) -> LearningPackage: +def get_learning_package(learning_package_id: LearningPackage.ID, /) -> LearningPackage: """ Get LearningPackage by ID. """ @@ -114,7 +114,7 @@ def create_learning_package( def update_learning_package( - learning_package_id: int, + learning_package_id: LearningPackage.ID, /, key: str | None = None, title: str | None = None, @@ -158,7 +158,7 @@ def learning_package_exists(key: str) -> bool: def create_publishable_entity( - learning_package_id: int, + learning_package_id: LearningPackage.ID, /, key: str, created: datetime, @@ -183,7 +183,7 @@ def create_publishable_entity( def create_publishable_entity_version( - entity_id: int, + entity_id: PublishableEntity.ID, /, version_num: int, title: str, @@ -282,32 +282,32 @@ def set_version_dependencies( ) -def get_publishable_entity(publishable_entity_id: int, /) -> PublishableEntity: - return PublishableEntity.objects.get(id=publishable_entity_id) +def get_publishable_entity(publishable_entity_id: PublishableEntity.ID, /) -> PublishableEntity: + return PublishableEntity.objects.get(pk=publishable_entity_id) -def get_publishable_entity_by_key(learning_package_id, /, key) -> PublishableEntity: +def get_publishable_entity_by_key(learning_package_id: LearningPackage.ID, /, key: str) -> PublishableEntity: return PublishableEntity.objects.get( learning_package_id=learning_package_id, key=key, ) -def get_last_publish(learning_package_id: int, /) -> PublishLog | None: +def get_last_publish(learning_package_id: LearningPackage.ID, /) -> PublishLog | None: return PublishLog.objects \ .filter(learning_package_id=learning_package_id) \ .order_by('-id') \ .first() -def get_all_drafts(learning_package_id: int, /) -> QuerySet[Draft]: +def get_all_drafts(learning_package_id: LearningPackage.ID, /) -> QuerySet[Draft]: return Draft.objects.filter( entity__learning_package_id=learning_package_id, version__isnull=False, ) -def get_publishable_entities(learning_package_id: int, /) -> QuerySet[PublishableEntity]: +def get_publishable_entities(learning_package_id: LearningPackage.ID, /) -> QuerySet[PublishableEntity]: """ Get all entities in a learning package. """ @@ -322,7 +322,7 @@ def get_publishable_entities(learning_package_id: int, /) -> QuerySet[Publishabl def get_entities_with_unpublished_changes( - learning_package_id: int, + learning_package_id: LearningPackage.ID, /, include_deleted_drafts: bool = False ) -> QuerySet[PublishableEntity]: @@ -354,7 +354,7 @@ def get_entities_with_unpublished_changes( return entities_qs.exclude(draft__version__isnull=True) -def get_entities_with_unpublished_deletes(learning_package_id: int, /) -> QuerySet[PublishableEntity]: +def get_entities_with_unpublished_deletes(learning_package_id: LearningPackage.ID, /) -> QuerySet[PublishableEntity]: """ Something will become "deleted" if it has a null Draft version but a not-null Published version. (If both are null, it means it's already been @@ -368,7 +368,7 @@ def get_entities_with_unpublished_deletes(learning_package_id: int, /) -> QueryS def publish_all_drafts( - learning_package_id: int, + learning_package_id: LearningPackage.ID, /, message="", published_at: datetime | None = None, @@ -421,7 +421,7 @@ def _get_dependencies_with_unpublished_changes( def publish_from_drafts( - learning_package_id: int, # LearningPackage.id + learning_package_id: LearningPackage.ID, /, draft_qset: QuerySet[Draft], message: str = "", @@ -504,7 +504,10 @@ def publish_from_drafts( return publish_log -def get_draft_version(publishable_entity_or_id: PublishableEntity | int, /) -> PublishableEntityVersion | None: +def get_draft_version( + publishable_entity_or_id: PublishableEntity | PublishableEntity.ID, + / +) -> PublishableEntityVersion | None: """ Return current draft PublishableEntityVersion for this PublishableEntity. @@ -531,7 +534,10 @@ def get_draft_version(publishable_entity_or_id: PublishableEntity | int, /) -> P return draft.version -def get_published_version(publishable_entity_or_id: PublishableEntity | int, /) -> PublishableEntityVersion | None: +def get_published_version( + publishable_entity_or_id: PublishableEntity | PublishableEntity.ID, + / +) -> PublishableEntityVersion | None: """ Return current published PublishableEntityVersion for this PublishableEntity. @@ -557,7 +563,7 @@ def get_published_version(publishable_entity_or_id: PublishableEntity | int, /) def set_draft_version( - draft_or_id: Draft | int, + draft_or_id: Draft | PublishableEntity.ID, publishable_entity_version_pk: int | None, /, set_at: datetime | None = None, @@ -683,7 +689,7 @@ def set_draft_version( def _add_to_existing_draft_change_log( active_change_log: DraftChangeLog, - entity_id: int, + entity_id: PublishableEntity.ID, old_version_id: int | None, new_version_id: int | None, ) -> DraftChangeLogRecord | None: @@ -1167,7 +1173,7 @@ def hash_for_log_record( return digest -def soft_delete_draft(publishable_entity_id: int, /, deleted_by: int | None = None) -> None: +def soft_delete_draft(publishable_entity_id: PublishableEntity.ID, /, deleted_by: int | None = None) -> None: """ Sets the Draft version to None. @@ -1181,7 +1187,7 @@ def soft_delete_draft(publishable_entity_id: int, /, deleted_by: int | None = No def reset_drafts_to_published( - learning_package_id: int, + learning_package_id: LearningPackage.ID, /, reset_at: datetime | None = None, reset_by: int | None = None, # User.id @@ -1308,7 +1314,10 @@ def filter_publishable_entities( return entities -def get_published_version_as_of(entity_id: int, publish_log_id: int) -> PublishableEntityVersion | None: +def get_published_version_as_of( + entity_id: PublishableEntity.ID, + publish_log_id: int, +) -> PublishableEntityVersion | None: """ Get the published version of the given entity, at a specific snapshot in the history of this Learning Package, given by the PublishLog ID. @@ -1328,7 +1337,7 @@ def get_published_version_as_of(entity_id: int, publish_log_id: int) -> Publisha def bulk_draft_changes_for( - learning_package_id: int, + learning_package_id: LearningPackage.ID, changed_by: int | None = None, changed_at: datetime | None = None ) -> DraftChangeLogContext: diff --git a/src/openedx_content/applets/publishing/contextmanagers.py b/src/openedx_content/applets/publishing/contextmanagers.py index 64ca9fb1f..0dcdc1dbb 100644 --- a/src/openedx_content/applets/publishing/contextmanagers.py +++ b/src/openedx_content/applets/publishing/contextmanagers.py @@ -12,7 +12,7 @@ from django.db.transaction import Atomic -from .models import DraftChangeLog +from .models import DraftChangeLog, LearningPackage class DraftChangeLogContext(Atomic): @@ -55,7 +55,7 @@ class to essentially keep the active DraftChangeChangeLog in a global def __init__( self, - learning_package_id: int, + learning_package_id: LearningPackage.ID, changed_at: datetime | None = None, changed_by: int | None = None, exit_callbacks: list[Callable[[DraftChangeLog], None]] | None = None diff --git a/src/openedx_content/applets/publishing/models/learning_package.py b/src/openedx_content/applets/publishing/models/learning_package.py index 9e6d6a776..9266b3a99 100644 --- a/src/openedx_content/applets/publishing/models/learning_package.py +++ b/src/openedx_content/applets/publishing/models/learning_package.py @@ -1,10 +1,15 @@ """ LearningPackage model """ + +from typing import NewType + from django.db import models +from typing_extensions import deprecated from openedx_django_lib.fields import ( MultiCollationTextField, + TypedAutoField, case_insensitive_char_field, immutable_uuid_field, key_field, @@ -18,12 +23,25 @@ class LearningPackage(models.Model): Each PublishableEntity belongs to exactly one LearningPackage. """ + + LearningPackageID = NewType("LearningPackageID", int) + type ID = LearningPackageID + # Explictly declare a 4-byte ID instead of using the app-default 8-byte ID. # We do not expect to have more than 2 billion LearningPackages on a given # site. Furthermore, many, many things have foreign keys to this model and # uniqueness indexes on those foreign keys + their own fields, so the 4 # bytes saved will add up over time. - id = models.AutoField(primary_key=True) + + class IDField(TypedAutoField[ID]): # Note: this is ...AutoField not ...BigAutoField + pass + + id = IDField(primary_key=True) + + @property # type: ignore[no-redef] + @deprecated("Use .id instead") + def pk(self) -> ID: + return self.id uuid = immutable_uuid_field() diff --git a/src/openedx_content/applets/publishing/models/publishable_entity.py b/src/openedx_content/applets/publishing/models/publishable_entity.py index 42e9a4a9c..907b50248 100644 --- a/src/openedx_content/applets/publishing/models/publishable_entity.py +++ b/src/openedx_content/applets/publishing/models/publishable_entity.py @@ -1,19 +1,22 @@ """ PublishableEntity model and PublishableEntityVersion + mixins """ + from __future__ import annotations from datetime import datetime from functools import cached_property -from typing import ClassVar, Self +from typing import ClassVar, NewType, Self from django.conf import settings from django.core.exceptions import ImproperlyConfigured from django.core.validators import MinValueValidator from django.db import models from django.utils.translation import gettext as _ +from typing_extensions import deprecated from openedx_django_lib.fields import ( + TypedBigAutoField, case_insensitive_char_field, immutable_uuid_field, key_field, @@ -110,6 +113,14 @@ class PublishableEntity(models.Model): more convenient, like file access. """ + PublishableEntityID = NewType("PublishableEntityID", int) + type ID = PublishableEntityID + + class IDField(TypedBigAutoField[ID]): # Boilerplate for fully-typed ID field. + pass + + id = IDField(primary_key=True) + uuid = immutable_uuid_field() learning_package = models.ForeignKey( LearningPackage, @@ -134,6 +145,11 @@ class PublishableEntity(models.Model): help_text=_("Set to True when created independently, False when created as part of a container."), ) + @property # type: ignore[no-redef] + @deprecated("Use .id instead") + def pk(self) -> ID: + return self.id + class Meta: constraints = [ # Keys are unique within a given LearningPackage. @@ -334,6 +350,10 @@ class PublishableEntityMixin(models.Model): PublishableEntity, on_delete=models.CASCADE, primary_key=True ) + @property + def id(self) -> PublishableEntity.ID: + return self.publishable_entity_id + @cached_property def versioning(self): return self.VersioningHelper(self) @@ -414,7 +434,7 @@ class VersioningHelper: # You need to manually refetch it from the database to see the new # publish status: - component = get_component(component.pk) + component = get_component(component.id) # Now this will work: assert component.versioning.published == component_version diff --git a/src/openedx_content/applets/sections/api.py b/src/openedx_content/applets/sections/api.py index ff9b613de..0aee1c7bd 100644 --- a/src/openedx_content/applets/sections/api.py +++ b/src/openedx_content/applets/sections/api.py @@ -9,6 +9,7 @@ from ..containers import api as containers_api from ..containers.models import ContainerVersion +from ..publishing.models import LearningPackage from ..subsections.models import Subsection, SubsectionVersion from .models import Section, SectionVersion @@ -23,13 +24,13 @@ ] -def get_section(section_id: int, /): +def get_section(section_id: Section.ID, /): """Get a section""" return Section.objects.select_related("container").get(pk=section_id) def create_section_and_version( - learning_package_id: int, + learning_package_id: LearningPackage.ID, key: str, *, title: str, @@ -60,7 +61,7 @@ def create_section_and_version( def create_next_section_version( - section: Section | int, + section: Section | Section.ID, *, title: str | None = None, subsections: Iterable[Subsection | SubsectionVersion] | None = None, diff --git a/src/openedx_content/applets/sections/models.py b/src/openedx_content/applets/sections/models.py index 3aba6a4cc..53e648652 100644 --- a/src/openedx_content/applets/sections/models.py +++ b/src/openedx_content/applets/sections/models.py @@ -2,7 +2,7 @@ Models that implement sections """ -from typing import override +from typing import NewType, cast, override from django.core.exceptions import ValidationError from django.db import models @@ -27,6 +27,9 @@ class Section(Container): entities and can be added to other containers. """ + SectionID = NewType("SectionID", Container.ID) + type ID = SectionID + type_code = "section" olx_tag_name = "chapter" # Serializes to OLX as `...`. @@ -37,6 +40,10 @@ class Section(Container): primary_key=True, ) + @property + def id(self) -> ID: + return cast(Section.ID, self.publishable_entity_id) + @override @classmethod def validate_entity(cls, entity: PublishableEntity) -> None: diff --git a/src/openedx_content/applets/subsections/api.py b/src/openedx_content/applets/subsections/api.py index 85f3b73ca..5d2a6cdb0 100644 --- a/src/openedx_content/applets/subsections/api.py +++ b/src/openedx_content/applets/subsections/api.py @@ -9,6 +9,7 @@ from ..containers import api as containers_api from ..containers.models import ContainerVersion +from ..publishing.models import LearningPackage from ..units.models import Unit, UnitVersion from .models import Subsection, SubsectionVersion @@ -23,13 +24,13 @@ ] -def get_subsection(subsection_id: int, /): +def get_subsection(subsection_id: Subsection.ID, /): """Get a subsection""" return Subsection.objects.select_related("container").get(pk=subsection_id) def create_subsection_and_version( - learning_package_id: int, + learning_package_id: LearningPackage.ID, key: str, *, title: str, @@ -60,7 +61,7 @@ def create_subsection_and_version( def create_next_subsection_version( - subsection: Subsection | int, + subsection: Subsection | Subsection.ID, *, title: str | None = None, units: Iterable[Unit | UnitVersion] | None = None, diff --git a/src/openedx_content/applets/subsections/models.py b/src/openedx_content/applets/subsections/models.py index ee470b6e7..7fd8c52ba 100644 --- a/src/openedx_content/applets/subsections/models.py +++ b/src/openedx_content/applets/subsections/models.py @@ -2,7 +2,7 @@ Models that implement subsections """ -from typing import override +from typing import NewType, cast, override from django.core.exceptions import ValidationError from django.db import models @@ -27,6 +27,9 @@ class Subsection(Container): entities and can be added to other containers. """ + SubsectionID = NewType("SubsectionID", Container.ID) + type ID = SubsectionID + type_code = "subsection" olx_tag_name = "sequential" # Serializes to OLX as `...`. @@ -37,6 +40,10 @@ class Subsection(Container): primary_key=True, ) + @property + def id(self) -> ID: + return cast(Subsection.ID, self.publishable_entity_id) + @override @classmethod def validate_entity(cls, entity: PublishableEntity) -> None: diff --git a/src/openedx_content/applets/units/api.py b/src/openedx_content/applets/units/api.py index 4aad6dde8..cde86bf0b 100644 --- a/src/openedx_content/applets/units/api.py +++ b/src/openedx_content/applets/units/api.py @@ -10,6 +10,7 @@ from ..components.models import Component, ComponentVersion from ..containers import api as containers_api from ..containers.models import ContainerVersion +from ..publishing.models import LearningPackage from .models import Unit, UnitVersion # 🛑 UNSTABLE: All APIs related to containers are unstable until we've figured @@ -23,13 +24,13 @@ ] -def get_unit(unit_id: int, /): +def get_unit(unit_id: Unit.ID, /): """Get a unit""" return Unit.objects.select_related("container").get(pk=unit_id) def create_unit_and_version( - learning_package_id: int, + learning_package_id: LearningPackage.ID, key: str, *, title: str, @@ -60,7 +61,7 @@ def create_unit_and_version( def create_next_unit_version( - unit: Unit | int, + unit: Unit | Unit.ID, *, title: str | None = None, components: Iterable[Component | ComponentVersion] | None = None, diff --git a/src/openedx_content/applets/units/models.py b/src/openedx_content/applets/units/models.py index 3594acb7b..913ea4890 100644 --- a/src/openedx_content/applets/units/models.py +++ b/src/openedx_content/applets/units/models.py @@ -2,7 +2,7 @@ Models that implement units """ -from typing import override +from typing import NewType, cast, override from django.core.exceptions import ValidationError from django.db import models @@ -25,6 +25,9 @@ class Unit(Container): entities and can be added to other containers. """ + UnitID = NewType("UnitID", Container.ID) + type ID = UnitID + type_code = "unit" olx_tag_name = "vertical" # Serializes to OLX as `...`. @@ -35,6 +38,10 @@ class Unit(Container): primary_key=True, ) + @property + def id(self) -> ID: + return cast(Unit.ID, self.publishable_entity_id) + @override @classmethod def validate_entity(cls, entity: PublishableEntity) -> None: diff --git a/src/openedx_content/management/commands/add_assets_to_component.py b/src/openedx_content/management/commands/add_assets_to_component.py index 7078c7f73..fc207e20a 100644 --- a/src/openedx_content/management/commands/add_assets_to_component.py +++ b/src/openedx_content/management/commands/add_assets_to_component.py @@ -73,7 +73,7 @@ def handle(self, *args, **options): local_keys_to_content_bytes[local_key] = pathlib.Path(file_path).read_bytes() if file_path else None next_version = create_next_component_version( - component.pk, + component.id, media_to_replace=local_keys_to_content_bytes, created=created, ) diff --git a/src/openedx_content/migrations/0006_typed_ids.py b/src/openedx_content/migrations/0006_typed_ids.py new file mode 100644 index 000000000..e950e058a --- /dev/null +++ b/src/openedx_content/migrations/0006_typed_ids.py @@ -0,0 +1,29 @@ +# Generated by Django 5.2.11 on 2026-04-09 00:08 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + """ + PublishableEntity.id was previously auto-created by Django (hence auto_created=True, + verbose_name='ID' in the original migration). It is now explicitly declared on the + model. The database column is unchanged (still BIGINT AUTO_INCREMENT PRIMARY KEY), + so database_operations is empty. + """ + + dependencies = [ + ("openedx_content", "0005_containertypes"), + ] + + operations = [ + migrations.SeparateDatabaseAndState( + state_operations=[ + migrations.AlterField( + model_name="publishableentity", + name="id", + field=models.BigAutoField(primary_key=True, serialize=False), + ), + ], + database_operations=[], + ), + ] diff --git a/src/openedx_django_lib/fields.py b/src/openedx_django_lib/fields.py index c41678993..e0f4964ff 100644 --- a/src/openedx_django_lib/fields.py +++ b/src/openedx_django_lib/fields.py @@ -8,6 +8,7 @@ We have helpers to make case sensitivity consistent across backends. MySQL is case-insensitive by default, SQLite and Postgres are case-sensitive. """ + from __future__ import annotations import hashlib @@ -16,6 +17,8 @@ from django.db import models from .collations import MultiCollationMixin +# Re-export these fields which are in a separate file so we can use .pyi type stubs: +from .id_fields import TypedAutoField, TypedBigAutoField # pylint: disable=unused-import from .validators import validate_utc_datetime diff --git a/src/openedx_django_lib/id_fields.py b/src/openedx_django_lib/id_fields.py new file mode 100644 index 000000000..8b62fe3f4 --- /dev/null +++ b/src/openedx_django_lib/id_fields.py @@ -0,0 +1,57 @@ +""" +Fields for supporting fully-typed integer primary keys +""" + +from django.db import models + + +class TypedBigAutoField(models.BigAutoField): + """ + Generic helper for constructing a BigAutoField with strongly-typed primary + keys. Use it like this: + + ``` + class MyModel(models.Model): + MyModelID = NewType("MyModelID", int) + type ID = MyModelID + + class IDField(TypedBigAutoField[ID]): + pass + + id = IDField(primary_key=True) + + ... # rest of your model's fields... + ``` + """ + + # The actual typing + django-stubs "magic" is handled entirely by the .pyi file. + + @classmethod + def __class_getitem__(cls, _): + # At runtime, type parameters are ignored — this just lets `TypedBigAutoField[XxxID]` + # and `class XxxField(TypedBigAutoField[XxxID])` work without `Generic` in the MRO. + # (Including `Generic` as a superclass here breaks Django's field __deepcopy__.) + return cls + + def deconstruct(self): + # Return the standard Django class path so the migration autodetector sees + # `BigAutoField` rather than the inner `IDField` subclass — otherwise every + # rename/move of the subclass generates a spurious AlterField migration. + name, _path, args, kwargs = super().deconstruct() + return name, "django.db.models.BigAutoField", args, kwargs + + +class TypedAutoField(models.AutoField): + """ + Use sparingly. This is a 32-bit version of `TypedBigAutoField`, and you + should usually be using that instead of this for primary key fields. + """ + + @classmethod + def __class_getitem__(cls, _): # See explanation on the class above. + return cls + + def deconstruct(self): + # Same reasoning as TypedBigAutoField.deconstruct() above. + name, _path, args, kwargs = super().deconstruct() + return name, "django.db.models.AutoField", args, kwargs diff --git a/src/openedx_django_lib/id_fields.pyi b/src/openedx_django_lib/id_fields.pyi new file mode 100644 index 000000000..cb76de275 --- /dev/null +++ b/src/openedx_django_lib/id_fields.pyi @@ -0,0 +1,17 @@ +""" +Expanded type definitions to support fully-typed integer primary key fields +""" + +from typing import TypeVar + +from django.db import models + +_IDType = TypeVar("_IDType", bound=int) + +class TypedBigAutoField(models.BigAutoField[_IDType, _IDType]): + _pyi_private_set_type: _IDType | int + _pyi_private_get_type: _IDType + +class TypedAutoField(models.AutoField[_IDType, _IDType]): + _pyi_private_set_type: _IDType | int + _pyi_private_get_type: _IDType diff --git a/tests/openedx_catalog/test_api.py b/tests/openedx_catalog/test_api.py index 0506b04af..f29040f5e 100644 --- a/tests/openedx_catalog/test_api.py +++ b/tests/openedx_catalog/test_api.py @@ -4,6 +4,7 @@ import logging from datetime import datetime, timezone +from typing import cast import pytest from django.db import connection @@ -56,9 +57,7 @@ def _python100_summer26(python100: CatalogCourse): @pytest.fixture(name="python100_winter26") def _python100_winter26(python100: CatalogCourse): """Create a "Python100" "Winter 2026" course run for use in these tests""" - return CourseRun.objects.create( - catalog_course=python100, run_code="2026winter", title="Python 100 (Winter '26)" - ) + return CourseRun.objects.create(catalog_course=python100, run_code="2026winter", title="Python 100 (Winter '26)") # get_catalog_course @@ -69,10 +68,11 @@ def test_get_catalog_course(python100: CatalogCourse, csharp200: CatalogCourse) Test using get_catalog_course to get a course in various ways: """ # Retrieve by ID: - assert api.get_catalog_course(pk=python100.pk) == python100 - assert api.get_catalog_course(pk=csharp200.pk) == csharp200 + assert api.get_catalog_course(pk=python100.id) == python100 + assert api.get_catalog_course(pk=csharp200.id) == csharp200 + FAKE_ID = cast(CatalogCourse.ID, 8234758243) with pytest.raises(CatalogCourse.DoesNotExist): - api.get_catalog_course(pk=8234758243) + api.get_catalog_course(pk=FAKE_ID) # Retrieve by key_str: assert api.get_catalog_course(key_str="catalog-course:Org1:Python100") == python100 @@ -118,12 +118,12 @@ def test_update_title(python100: CatalogCourse, csharp200: CatalogCourse) -> Non csharp200_old_name = csharp200.title # Update title using a CatalogCourse object: api.update_catalog_course(python100, title="New name for Python 100") - assert api.get_catalog_course(pk=python100.pk).title == "New name for Python 100" - assert api.get_catalog_course(pk=csharp200.pk).title == csharp200_old_name # Unchanged + assert api.get_catalog_course(pk=python100.id).title == "New name for Python 100" + assert api.get_catalog_course(pk=csharp200.id).title == csharp200_old_name # Unchanged # Update display name using PK only: - api.update_catalog_course(csharp200.pk, title="New name for C# 200") - assert api.get_catalog_course(pk=python100.pk).title == "New name for Python 100" # Unchanged - assert api.get_catalog_course(pk=csharp200.pk).title == "New name for C# 200" + api.update_catalog_course(csharp200.id, title="New name for C# 200") + assert api.get_catalog_course(pk=python100.id).title == "New name for Python 100" # Unchanged + assert api.get_catalog_course(pk=csharp200.id).title == "New name for C# 200" def test_update_language(python100: CatalogCourse) -> None: diff --git a/tests/openedx_catalog/test_models.py b/tests/openedx_catalog/test_models.py index 905f3d362..6523b8f8d 100644 --- a/tests/openedx_catalog/test_models.py +++ b/tests/openedx_catalog/test_models.py @@ -85,7 +85,7 @@ def test_course_code_required(org1) -> None: cc = CatalogCourse.objects.create(org_code="Org1", course_code="Python100", title="Python 100") with pytest.raises(IntegrityError): # Using .update() will bypass all checks and defaults in save()/clean(), to see if the DB enforces this: - CatalogCourse.objects.filter(pk=cc.pk).update(course_code="") + CatalogCourse.objects.filter(pk=cc.id).update(course_code="") # key_str field tests: @@ -109,7 +109,7 @@ def test_title_required(org1) -> None: cc = CatalogCourse.objects.create(org_code="Org1", course_code="Python100", title="Python 100") with pytest.raises(IntegrityError): # Using .update() will bypass all checks and defaults in save()/clean(), to see if the DB enforces this: - CatalogCourse.objects.filter(pk=cc.pk).update(title="") + CatalogCourse.objects.filter(pk=cc.id).update(title="") def test_title_unicode(org1) -> None: @@ -291,7 +291,7 @@ def test_run_code_required(python100: CatalogCourse) -> None: course = CourseRun.objects.create(catalog_course=python100, run_code="fall2026") with pytest.raises(IntegrityError): # Using .update() will bypass all checks and defaults in save()/clean(), to see if the DB enforces this: - CourseRun.objects.filter(pk=course.pk).update(run_code="") + CourseRun.objects.filter(pk=course.id).update(run_code="") def test_run_code_exact(org1) -> None: @@ -313,17 +313,17 @@ def test_run_code_exact(org1) -> None: # Do not allow modifying the run so it's completely different from the run in the course ID with pytest.raises(IntegrityError, match="oex_catalog_courserun_course_key_run_code_match_exactly"): with transaction.atomic(): - CourseRun.objects.filter(pk=run.pk).update(run_code="foobar") + CourseRun.objects.filter(pk=run.id).update(run_code="foobar") # Do not allow modifying the run so it doesn't match the course ID: with pytest.raises(IntegrityError): with transaction.atomic(): - CourseRun.objects.filter(pk=run.pk).update(run_code="mixedcase") + CourseRun.objects.filter(pk=run.id).update(run_code="mixedcase") # Do not allow modifying the course ID so it doesn't match the run: with pytest.raises(IntegrityError): with transaction.atomic(): - CourseRun.objects.filter(pk=run.pk).update( + CourseRun.objects.filter(pk=run.id).update( course_key=CourseLocator(org="Org1", course="Test302", run="mixedcase"), ) diff --git a/tests/openedx_content/applets/backup_restore/test_backup.py b/tests/openedx_content/applets/backup_restore/test_backup.py index e7a69cff7..efff635de 100644 --- a/tests/openedx_content/applets/backup_restore/test_backup.py +++ b/tests/openedx_content/applets/backup_restore/test_backup.py @@ -94,14 +94,14 @@ def setUpTestData(cls): ) new_problem_version = api.create_next_component_version( - cls.published_component.pk, + cls.published_component.id, title="My published problem draft v2", media_to_replace={}, created=cls.now, ) new_txt_media = api.get_or_create_text_media( - cls.learning_package.pk, + cls.learning_package.id, text_media_type.id, text="This is some data", created=cls.now, @@ -123,7 +123,7 @@ def setUpTestData(cls): ) new_html_version = api.create_next_component_version( - cls.draft_component.pk, + cls.draft_component.id, title="My draft html v2", media_to_replace={}, created=cls.now, diff --git a/tests/openedx_content/applets/collections/test_api.py b/tests/openedx_content/applets/collections/test_api.py index c8d638eb8..f53d9fbb7 100644 --- a/tests/openedx_content/applets/collections/test_api.py +++ b/tests/openedx_content/applets/collections/test_api.py @@ -108,7 +108,7 @@ def test_get_collection(self): """ Test getting a single collection. """ - collection = api.get_collection(self.learning_package.pk, 'COL1') + collection = api.get_collection(self.learning_package.id, 'COL1') assert collection == self.collection1 def test_get_collection_not_found(self): @@ -116,14 +116,14 @@ def test_get_collection_not_found(self): Test getting a collection that doesn't exist. """ with self.assertRaises(ObjectDoesNotExist): - api.get_collection(self.learning_package.pk, '12345') + api.get_collection(self.learning_package.id, '12345') def test_get_collection_wrong_learning_package(self): """ Test getting a collection that doesn't exist in the requested learning package. """ with self.assertRaises(ObjectDoesNotExist): - api.get_collection(self.learning_package.pk, self.another_library_collection.key) + api.get_collection(self.learning_package.id, self.another_library_collection.key) def test_get_collections(self): """ @@ -284,25 +284,25 @@ def setUpTestData(cls) -> None: cls.collection1 = api.add_to_collection( cls.learning_package.id, key=cls.collection1.key, - entities_qset=PublishableEntity.objects.filter(id__in=[ - cls.published_component.pk, + entities_qset=PublishableEntity.objects.filter(pk__in=[ + cls.published_component.id, ]), created_by=cls.user.id, ) cls.collection2 = api.add_to_collection( cls.learning_package.id, key=cls.collection2.key, - entities_qset=PublishableEntity.objects.filter(id__in=[ - cls.published_component.pk, - cls.draft_component.pk, - cls.draft_unit.pk, + entities_qset=PublishableEntity.objects.filter(pk__in=[ + cls.published_component.id, + cls.draft_component.id, + cls.draft_unit.id, ]), ) cls.disabled_collection = api.add_to_collection( cls.learning_package.id, key=cls.disabled_collection.key, - entities_qset=PublishableEntity.objects.filter(id__in=[ - cls.published_component.pk, + entities_qset=PublishableEntity.objects.filter(pk__in=[ + cls.published_component.id, ]), ) @@ -336,9 +336,9 @@ def test_add_to_collection(self): self.collection1 = api.add_to_collection( self.learning_package.id, self.collection1.key, - PublishableEntity.objects.filter(id__in=[ - self.draft_component.pk, - self.draft_unit.pk, + PublishableEntity.objects.filter(pk__in=[ + self.draft_component.id, + self.draft_unit.id, ]), created_by=self.user.id, ) @@ -361,8 +361,8 @@ def test_add_to_collection_again(self): self.collection2 = api.add_to_collection( self.learning_package.id, self.collection2.key, - PublishableEntity.objects.filter(id__in=[ - self.published_component.pk, + PublishableEntity.objects.filter(pk__in=[ + self.published_component.id, ]), ) @@ -381,8 +381,8 @@ def test_add_to_collection_wrong_learning_package(self): api.add_to_collection( self.learning_package_2.id, self.another_library_collection.key, - PublishableEntity.objects.filter(id__in=[ - self.published_component.pk, + PublishableEntity.objects.filter(pk__in=[ + self.published_component.id, ]), ) @@ -397,9 +397,9 @@ def test_remove_from_collection(self): self.collection2 = api.remove_from_collection( self.learning_package.id, self.collection2.key, - PublishableEntity.objects.filter(id__in=[ - self.published_component.pk, - self.draft_unit.pk, + PublishableEntity.objects.filter(pk__in=[ + self.published_component.id, + self.draft_unit.id, ]), ) @@ -443,7 +443,7 @@ def test_get_collection_containers(self): """ Test using `get_collection_entities()` to get containers """ - def get_collection_containers(learning_package_id: int, collection_key: str): + def get_collection_containers(learning_package_id: LearningPackage.ID, collection_key: str): return ( pe.container for pe in api.get_collection_entities(learning_package_id, collection_key).exclude(container=None) diff --git a/tests/openedx_content/applets/components/test_api.py b/tests/openedx_content/applets/components/test_api.py index a9af5f65d..cbaa889b2 100644 --- a/tests/openedx_content/applets/components/test_api.py +++ b/tests/openedx_content/applets/components/test_api.py @@ -48,8 +48,8 @@ def publish_component(self, component: Component): Helper method to publish a single component. """ publishing_api.publish_from_drafts( - self.learning_package.pk, - draft_qset=publishing_api.get_all_drafts(self.learning_package.pk).filter( + self.learning_package.id, + draft_qset=publishing_api.get_all_drafts(self.learning_package.id).filter( entity=component.publishable_entity, ), ) @@ -92,14 +92,14 @@ def test_component_num_queries(self) -> None: created_by=None, ) publishing_api.publish_all_drafts( - self.learning_package.pk, + self.learning_package.id, published_at=self.now ) # We should be fetching all of this with a select-related, so only one # database query should happen here. with self.assertNumQueries(1): - component = components_api.get_component(component.pk) + component = components_api.get_component(component.id) draft = component.versioning.draft published = component.versioning.published assert draft.title == published.title @@ -145,7 +145,7 @@ def setUpTestData(cls) -> None: created_by=None, ) publishing_api.publish_all_drafts( - cls.learning_package.pk, + cls.learning_package.id, published_at=cls.now ) @@ -177,7 +177,7 @@ def setUpTestData(cls) -> None: created=cls.now, created_by=None, ) - publishing_api.soft_delete_draft(cls.deleted_video.pk) + publishing_api.soft_delete_draft(cls.deleted_video.id) def test_no_filters(self): """ @@ -338,7 +338,7 @@ def setUpTestData(cls) -> None: ) def test_simple_get(self): - assert components_api.get_component(self.problem.pk) == self.problem + assert components_api.get_component(self.problem.id) == self.problem with self.assertRaises(ObjectDoesNotExist): components_api.get_component(-1) @@ -407,14 +407,14 @@ def setUpTestData(cls) -> None: def test_add(self): new_version = components_api.create_component_version( - self.problem.pk, + self.problem.id, version_num=1, title="My Title", created=self.now, created_by=None, ) new_media = media_api.get_or_create_text_media( - self.learning_package.pk, + self.learning_package.id, self.text_media_type.id, text="This is some data", created=self.now, @@ -425,7 +425,7 @@ def test_add(self): key="my/path/to/hello.txt", ) # re-fetch from the database to check to see if we wrote it correctly - new_version = components_api.get_component(self.problem.pk) \ + new_version = components_api.get_component(self.problem.id) \ .versions \ .get(publishable_entity_version__version_num=1) assert ( @@ -440,7 +440,7 @@ def test_add(self): new_media.pk, key="//nested/path/hello.txt", ) - new_version = components_api.get_component(self.problem.pk) \ + new_version = components_api.get_component(self.problem.id) \ .versions \ .get(publishable_entity_version__version_num=1) assert ( @@ -452,7 +452,7 @@ def test_bytes_content(self): bytes_media = b'raw content' version_1 = components_api.create_next_component_version( - self.problem.pk, + self.problem.id, title="Problem Version 1", media_to_replace={ "raw.txt": bytes_media, @@ -494,7 +494,7 @@ def test_multiple_versions(self): # Two text files, hello.txt and goodbye.txt version_1 = components_api.create_next_component_version( - self.problem.pk, + self.problem.id, title="Problem Version 1", media_to_replace={ "hello.txt": hello_media.pk, @@ -520,7 +520,7 @@ def test_multiple_versions(self): # This should keep the old value for goodbye.txt, add blank.txt, and set # hello.txt to be a new value (blank). version_2 = components_api.create_next_component_version( - self.problem.pk, + self.problem.id, title="Problem Version 2", media_to_replace={ "hello.txt": blank_media.pk, @@ -549,7 +549,7 @@ def test_multiple_versions(self): # Now we're going to set "hello.txt" back to hello_content, but remove # blank.txt, goodbye.txt, and an unknown "nothere.txt". version_3 = components_api.create_next_component_version( - self.problem.pk, + self.problem.id, title="Problem Version 3", media_to_replace={ "hello.txt": hello_media.pk, @@ -570,7 +570,7 @@ def test_multiple_versions(self): def test_create_next_version_forcing_num_version(self): """Test creating a next version with a forced version number.""" version_1 = components_api.create_next_component_version( - self.problem.pk, + self.problem.id, title="Problem Version 1", media_to_replace={}, created=self.now, @@ -602,7 +602,7 @@ def test_create_multiple_next_versions_and_diff_content(self): 'static/new_file.webp': python_source_asset.pk, } version_1_published = components_api.create_next_component_version( - self.problem.pk, + self.problem.id, title="Problem Version 1", media_to_replace=media_to_replace_for_published, created=self.now, @@ -610,12 +610,12 @@ def test_create_multiple_next_versions_and_diff_content(self): assert version_1_published.version_num == 1 publishing_api.publish_all_drafts( - self.learning_package.pk, + self.learning_package.id, published_at=self.now ) version_2_draft = components_api.create_next_component_version( - self.problem.pk, + self.problem.id, title="Problem Version 2", media_to_replace=media_to_replace_for_draft, created=self.now, diff --git a/tests/openedx_content/applets/components/test_models.py b/tests/openedx_content/applets/components/test_models.py index 356346a6a..bdcdea725 100644 --- a/tests/openedx_content/applets/components/test_models.py +++ b/tests/openedx_content/applets/components/test_models.py @@ -58,14 +58,14 @@ def test_latest_version(self) -> None: ) assert component.versioning.draft == component_version assert component.versioning.published is None - publish_all_drafts(self.learning_package.pk, published_at=self.now) + publish_all_drafts(self.learning_package.id, published_at=self.now) # Publishing isn't immediately reflected in the component obj (it's # using a cached version). assert component.versioning.published is None # Re-fetching the component and the published version should be updated. - component = get_component(component.pk) + component = get_component(component.id) assert component.versioning.published == component_version # Grabbing the list of versions for this component @@ -104,8 +104,8 @@ def test_last_publish_log(self): publish_all_drafts(self.learning_package.id) # Refetch the entities to get latest versions - component_with_changes = get_component(component_with_changes.pk) - component_with_no_changes = get_component(component_with_no_changes.pk) + component_with_changes = get_component(component_with_changes.id) + component_with_no_changes = get_component(component_with_no_changes.id) # Fetch the most recent PublishLog for these components first_publish_log_for_component_with_changes = component_with_changes.versioning.last_publish_log @@ -133,8 +133,8 @@ def test_last_publish_log(self): publish_all_drafts(self.learning_package.id) # Refetch the entities to get latest versions - component_with_changes = get_component(component_with_changes.pk) - component_with_no_changes = get_component(component_with_no_changes.pk) + component_with_changes = get_component(component_with_changes.id) + component_with_no_changes = get_component(component_with_no_changes.id) # Re-fetch the most recent PublishLog for these components next_publish_log_for_component_with_changes = component_with_changes.versioning.last_publish_log diff --git a/tests/openedx_content/applets/containers/test_api.py b/tests/openedx_content/applets/containers/test_api.py index 50871d8c5..9a5fdb431 100644 --- a/tests/openedx_content/applets/containers/test_api.py +++ b/tests/openedx_content/applets/containers/test_api.py @@ -4,7 +4,7 @@ # pylint: disable=too-many-positional-arguments, unused-argument from datetime import datetime, timezone -from typing import Any, assert_type +from typing import TYPE_CHECKING, Any, assert_type, cast import pytest from django.core.exceptions import ValidationError @@ -234,7 +234,7 @@ def _container_of_uninstalled_type(lp: LearningPackage, child_entity1: TestEntit """ # First create a TestContainer, then we'll modify it to simulate it being from an uninstalled plugin container, _ = containers_api.create_container_and_version( - lp.pk, + lp.id, key="abandoned-container", title="Abandoned Container 1", entities=[child_entity1], @@ -243,8 +243,8 @@ def _container_of_uninstalled_type(lp: LearningPackage, child_entity1: TestEntit ) # Now create the plugin type (no public API for this; only do this in a test) ctr = ContainerType.objects.create(type_code="misc") - Container.objects.filter(pk=container.pk).update(container_type=ctr) - return Container.objects.get(pk=container.pk) # Reload and just use the base Container type + Container.objects.filter(pk=container.id).update(container_type=ctr) + return Container.objects.get(pk=container.id) # Reload and just use the base Container type @pytest.fixture(name="other_lp_parent") @@ -301,7 +301,7 @@ def test_create_generic_empty_container(lp: LearningPackage, admin_user) -> None Creating an empty TestContainer. It will have only a draft version. """ container, container_v1 = containers_api.create_container_and_version( - lp.pk, + lp.id, key="new-container-1", title="Test Container 1", container_cls=TestContainer, @@ -310,8 +310,24 @@ def test_create_generic_empty_container(lp: LearningPackage, admin_user) -> None can_stand_alone=False, ) - assert_type(container, TestContainer) - # assert_type(container_v1, TestContainerVersion) # FIXME: seems not possible yet as of Python 3.12 + if TYPE_CHECKING: + # `create_container_and_version()` should return the correct Container subclass, based on `container_cls=...`: + assert_type(container, TestContainer) + + # Documenting status quo. We want `container.pk` to have type 'Container.ID', but we cannot do that because of + # how django-stubs overrides 'pk' when a model uses a OneToOneField as primary key. However, you should see that + # `pk` below is crossed out (if you use an IDE), to remind you to use 'id' instead. + assert_type(container.pk, Any) + + # We use this to get the primary key instead. Properly typed. + assert_type(container.id, Container.ID) + + # As of Python 3.12, it's not yet possible to make the the returned version (`container_v1`) use a type that is + # computed based on `container_cls`. Ideally this would have type `TestContainerVersion`, not + # `ContainerVersion`. + # Revisit as we upgrade to later python versions, perhaps with something like PEP 827. + # assert_type(container_v1, TestContainerVersion) + # Note the assert_type() calls must come before 'assert isinstance()' or they'll have no effect. assert isinstance(container, TestContainer) assert isinstance(container_v1, TestContainerVersion) @@ -340,10 +356,10 @@ def test_create_container_queries(lp: LearningPackage, child_entity1: TestEntity } # The exact numbers here aren't too important - this is just to alert us if anything significant changes. with django_assert_num_queries(31): - containers_api.create_container_and_version(lp.pk, key="c1", **base_args) + containers_api.create_container_and_version(lp.id, key="c1", **base_args) # And try with a a container that has children: with django_assert_num_queries(32): - containers_api.create_container_and_version(lp.pk, key="c2", **base_args, entities=[child_entity1]) + containers_api.create_container_and_version(lp.id, key="c2", **base_args, entities=[child_entity1]) # versioning helpers @@ -490,7 +506,7 @@ def test_create_next_container_version_with_remove_1( ] # Remove "entity 1 unpinned" - should remove both: containers_api.create_next_container_version( - parent_of_six.pk, + parent_of_six.id, entities=[child_entity1], created=now, created_by=None, @@ -527,7 +543,7 @@ def test_create_next_container_version_with_remove_2( ] # Remove "entity 2 pinned" - should remove both: containers_api.create_next_container_version( - parent_of_six.pk, + parent_of_six.id, entities=[child_entity2.versioning.draft], # specify the version for "pinned" created=now, created_by=None, @@ -565,7 +581,7 @@ def test_create_next_container_version_with_remove_3( ] # Remove "entity 3 pinned" - should remove only one: containers_api.create_next_container_version( - parent_of_six.pk, + parent_of_six.id, entities=[child_entity3.versioning.draft], # specify the version for "pinned" created=now, created_by=None, @@ -603,7 +619,7 @@ def test_create_next_container_version_with_remove_4( ] # Remove "entity 3 unpinned" - should remove only one: containers_api.create_next_container_version( - parent_of_six.pk, + parent_of_six.id, entities=[child_entity3], created=now, created_by=None, @@ -630,7 +646,7 @@ def test_create_next_container_version_with_conflicting_version(parent_of_two: T def create_v5(): """Create a new version, specifying version number 5 and changing the title and the order of the children.""" containers_api.create_next_container_version( - parent_of_two.pk, + parent_of_two.id, title="New version - forced as v5", force_version_num=5, created=now, @@ -651,7 +667,7 @@ def test_create_next_container_version_uninstalled_plugin(container_of_uninstall """ with pytest.raises(containers_api.ContainerImplementationMissingError): containers_api.create_next_container_version( - container_of_uninstalled_type.pk, + container_of_uninstalled_type.id, title="New version of the container", created=now, created_by=None, @@ -664,7 +680,7 @@ def test_create_next_container_version_other_lp(parent_of_two: TestContainer, ot """ with pytest.raises(ValidationError, match="Container entities must be from the same learning package."): containers_api.create_next_container_version( - parent_of_two.pk, + parent_of_two.id, title="Bad Version with entities from another learning package", created=now, created_by=None, @@ -680,7 +696,7 @@ def test_get_container(parent_of_two: TestContainer, django_assert_num_queries) Test `get_container()` """ with django_assert_num_queries(1): - result = containers_api.get_container(parent_of_two.pk) + result = containers_api.get_container(parent_of_two.id) assert result == parent_of_two.container # Versioning data should be pre-loaded via the default select_related() of Container.objects used by get_container with django_assert_num_queries(0): @@ -691,20 +707,21 @@ def test_get_container_nonexistent() -> None: """ Test `get_container()` with an invalid ID. """ + FAKE_ID = cast(Container.ID, -500) with pytest.raises(Container.DoesNotExist): - containers_api.get_container(-5000) + containers_api.get_container(FAKE_ID) def test_get_container_soft_deleted(parent_of_two: TestContainer) -> None: """ Test `get_container()` with a soft deleted container """ - publishing_api.soft_delete_draft(parent_of_two.pk, deleted_by=None) + publishing_api.soft_delete_draft(parent_of_two.id, deleted_by=None) parent_of_two.refresh_from_db() assert parent_of_two.versioning.draft is None assert parent_of_two.versioning.published is None # Get the container - result = containers_api.get_container(parent_of_two.pk) + result = containers_api.get_container(parent_of_two.id) assert result == parent_of_two.container # It works fine! get_container() ignores publish/delete status. @@ -713,7 +730,7 @@ def test_get_container_uninstalled_type(container_of_uninstalled_type: Container Test `get_container()` with a container from an uninstalled plugin """ # Nothing special happens. It should work fine. - result = containers_api.get_container(container_of_uninstalled_type.pk) + result = containers_api.get_container(container_of_uninstalled_type.id) assert result == container_of_uninstalled_type @@ -744,7 +761,7 @@ def test_get_container_by_key(lp: LearningPackage, parent_of_two: TestContainer) """ Test getting a specific container by key """ - result = containers_api.get_container_by_key(lp.pk, parent_of_two.key) + result = containers_api.get_container_by_key(lp.id, parent_of_two.key) assert result == parent_of_two.container # The API always returns "Container", not specific subclasses like TestContainer: assert result.__class__ is Container @@ -754,11 +771,12 @@ def test_get_container_by_key_nonexistent(lp: LearningPackage) -> None: """ Test getting a specific container by key, where the key and/or learning package is invalid """ + FAKE_ID = cast(LearningPackage.ID, -500) with pytest.raises(LearningPackage.DoesNotExist): - containers_api.get_container_by_key(32874, "invalid-key") + containers_api.get_container_by_key(FAKE_ID, "invalid-key") with pytest.raises(Container.DoesNotExist): - containers_api.get_container_by_key(lp.pk, "invalid-key") + containers_api.get_container_by_key(lp.id, "invalid-key") # get_container_subclass @@ -875,7 +893,7 @@ def test_get_containers_soft_deleted( default, but can be included. """ # Soft delete `parent_of_two`: - publishing_api.soft_delete_draft(parent_of_two.pk) + publishing_api.soft_delete_draft(parent_of_two.id) # Now it should not be included in the result: assert list(containers_api.get_containers(lp.id)) == [ # parent_of_two is not returned. @@ -904,7 +922,7 @@ def test_contains_unpublished_changes_queries( with django_assert_num_queries(1): assert containers_api.contains_unpublished_changes(grandparent) with django_assert_num_queries(1): - assert containers_api.contains_unpublished_changes(grandparent.pk) + assert containers_api.contains_unpublished_changes(grandparent.id) # Publish grandparent and all its descendants: with django_assert_num_queries(135): # TODO: investigate as this seems high! @@ -940,7 +958,7 @@ def test_auto_publish_children( Test that publishing a container publishes its child components automatically. """ # At first, nothing is published: - assert containers_api.contains_unpublished_changes(parent_of_two.pk) + assert containers_api.contains_unpublished_changes(parent_of_two.id) assert child_entity1.versioning.published is None assert child_entity2.versioning.published is None assert child_entity3.versioning.published is None @@ -955,7 +973,7 @@ def test_auto_publish_children( assert parent_of_two.versioning.has_unpublished_changes is False # Shallow check assert child_entity1.versioning.has_unpublished_changes is False assert child_entity2.versioning.has_unpublished_changes is False - assert containers_api.contains_unpublished_changes(parent_of_two.pk) is False # Deep check + assert containers_api.contains_unpublished_changes(parent_of_two.id) is False # Deep check assert child_entity1.versioning.published == child_entity1_v1 # v1 is now the published version. # But our other component that's outside the container is not affected: @@ -997,14 +1015,14 @@ def test_add_entity_after_publish(lp: LearningPackage, parent_of_two: TestContai assert parent_of_two_v1.version_num == 1 assert parent_of_two.versioning.published is None # Publish everything in the learning package: - publishing_api.publish_all_drafts(lp.pk) + publishing_api.publish_all_drafts(lp.id) parent_of_two.refresh_from_db() # Reloading is necessary assert not parent_of_two.versioning.has_unpublished_changes # Shallow check assert not containers_api.contains_unpublished_changes(parent_of_two) # Deeper check # Add a published entity (child_entity3, unpinned): parent_of_two_v2 = containers_api.create_next_container_version( - parent_of_two.pk, + parent_of_two.id, entities=[child_entity3], created=now, created_by=None, @@ -1034,7 +1052,7 @@ def test_modify_unpinned_entity_after_publish( child_entity2_v1 = child_entity2.versioning.draft assert parent_of_two.versioning.has_unpublished_changes is False # Shallow check - assert containers_api.contains_unpublished_changes(parent_of_two.pk) is False # Deeper check + assert containers_api.contains_unpublished_changes(parent_of_two.id) is False # Deeper check assert child_entity1.versioning.has_unpublished_changes is False # Now modify the child entity (it remains a draft): @@ -1046,7 +1064,9 @@ def test_modify_unpinned_entity_after_publish( assert ( parent_of_two.versioning.has_unpublished_changes is False ) # Shallow check should be false - container is unchanged - assert containers_api.contains_unpublished_changes(parent_of_two.pk) # But the container DOES "contain" changes + assert containers_api.contains_unpublished_changes( + parent_of_two.id + ) # But the container DOES "contain" changes assert child_entity1.versioning.has_unpublished_changes # Since the child's changes haven't been published, they should only appear in the draft container @@ -1131,7 +1151,7 @@ def test_publishing_shared_component(lp: LearningPackage): c4_v1 = c4.versioning.draft c5_v1 = c5.versioning.draft unit1, _ = containers_api.create_container_and_version( - lp.pk, + lp.id, entities=[c1, c2, c3], title="Unit 1", key="unit:1", @@ -1140,7 +1160,7 @@ def test_publishing_shared_component(lp: LearningPackage): container_cls=TestContainer, ) unit2, _ = containers_api.create_container_and_version( - lp.pk, + lp.id, entities=[c2, c4, c5], title="Unit 2", key="unit:2", @@ -1148,15 +1168,15 @@ def test_publishing_shared_component(lp: LearningPackage): created_by=None, container_cls=TestContainer, ) - publishing_api.publish_all_drafts(lp.pk) - assert containers_api.contains_unpublished_changes(unit1.pk) is False - assert containers_api.contains_unpublished_changes(unit2.pk) is False + publishing_api.publish_all_drafts(lp.id) + assert containers_api.contains_unpublished_changes(unit1.id) is False + assert containers_api.contains_unpublished_changes(unit2.id) is False # 2️⃣ Then the author edits C2 inside of Unit 1 making C2v2. c2_v2 = modify_entity(c2) # This makes U1 and U2 both show up as Units that CONTAIN unpublished changes, because they share the component. - assert containers_api.contains_unpublished_changes(unit1.pk) - assert containers_api.contains_unpublished_changes(unit2.pk) + assert containers_api.contains_unpublished_changes(unit1.id) + assert containers_api.contains_unpublished_changes(unit2.id) # (But the units themselves are unchanged:) unit1.refresh_from_db() unit2.refresh_from_db() @@ -1186,8 +1206,8 @@ def test_publishing_shared_component(lp: LearningPackage): ] # Result: Unit 2 CONTAINS unpublished changes because of the modified C5. Unit 1 doesn't contain unpub changes. - assert containers_api.contains_unpublished_changes(unit1.pk) is False - assert containers_api.contains_unpublished_changes(unit2.pk) + assert containers_api.contains_unpublished_changes(unit1.id) is False + assert containers_api.contains_unpublished_changes(unit2.id) # 5️⃣ Publish component C5, which should be the only thing unpublished in the learning package publish_entity(c5) @@ -1197,7 +1217,7 @@ def test_publishing_shared_component(lp: LearningPackage): Entry(c4_v1), # still original version of C4 (it was never modified) Entry(c5_v2), # new published version of C5 ] - assert containers_api.contains_unpublished_changes(unit2.pk) is False + assert containers_api.contains_unpublished_changes(unit2.id) is False def test_shallow_publish_log( @@ -1401,7 +1421,7 @@ def test_snapshots_of_published_unit(lp: LearningPackage, child_entity1: TestEnt modify_entity(child_entity1, title="Component 1 as of checkpoint 3") modify_entity(child_entity2, title="Component 2 as of checkpoint 3") containers_api.create_next_container_version( - container.pk, + container.id, title="Unit title in checkpoint 3", entities=[child_entity1, child_entity2], created=now, @@ -1414,7 +1434,7 @@ def test_snapshots_of_published_unit(lp: LearningPackage, child_entity1: TestEnt # Now add a third component to the unit, a pinned 📌 version of component 1. # This will test pinned versions and also test adding at the beginning rather than the end of the unit. containers_api.create_next_container_version( - container.pk, + container.id, title="Unit title in checkpoint 4", entities=[child_entity1_v1, child_entity1, child_entity2], created=now, @@ -1477,7 +1497,7 @@ def test_get_containers_with_entity_draft( # "child_entity1" is found in three different containers: with django_assert_num_queries(1): - result = list(containers_api.get_containers_with_entity(child_entity1.publishable_entity.pk)) + result = list(containers_api.get_containers_with_entity(child_entity1.publishable_entity.id)) assert result == [ # Note: ordering is in order of container creation parent_of_two.container, parent_of_three.container, @@ -1486,7 +1506,7 @@ def test_get_containers_with_entity_draft( # "child_entity3" is found in two different containers: with django_assert_num_queries(1): - result = list(containers_api.get_containers_with_entity(child_entity3.publishable_entity.pk)) + result = list(containers_api.get_containers_with_entity(child_entity3.publishable_entity.id)) assert result == [ # Note: ordering is in order of container creation parent_of_three.container, # pinned in this container parent_of_six.container, # pinned and unpinned in this container @@ -1497,17 +1517,17 @@ def test_get_containers_with_entity_draft( with django_assert_num_queries(1): result = list( - containers_api.get_containers_with_entity(child_entity3.publishable_entity.pk, ignore_pinned=True) + containers_api.get_containers_with_entity(child_entity3.publishable_entity.id, ignore_pinned=True) ) assert result == [ # Note: ordering is in order of container creation parent_of_six.container, # it's pinned and unpinned in this container ] # Some basic tests of the other learning package: - assert list(containers_api.get_containers_with_entity(other_lp_child.publishable_entity.pk)) == [ + assert list(containers_api.get_containers_with_entity(other_lp_child.publishable_entity.id)) == [ other_lp_parent.container ] - assert not list(containers_api.get_containers_with_entity(other_lp_parent.publishable_entity.pk)) + assert not list(containers_api.get_containers_with_entity(other_lp_parent.publishable_entity.id)) # get_container_children_count @@ -1521,7 +1541,7 @@ def test_get_container_children_count( grandparent: ContainerContainer, ): """Test `get_container_children_count()`""" - publishing_api.publish_all_drafts(lp.pk) + publishing_api.publish_all_drafts(lp.id) assert containers_api.get_container_children_count(parent_of_two, published=False) == 2 assert containers_api.get_container_children_count(parent_of_two, published=True) == 2 @@ -1554,7 +1574,7 @@ def test_get_container_children_count_soft_deletion( child_entity2: TestEntity, ): """Test `get_container_children_count()` when an entity is soft deleted""" - publishing_api.publish_all_drafts(lp.pk) + publishing_api.publish_all_drafts(lp.id) publishing_api.soft_delete_draft(child_entity2.pk) # "parent_of_two" contains the soft deleted child, so its draft child count is decreased by one: assert containers_api.get_container_children_count(parent_of_two, published=False) == 1 @@ -1572,7 +1592,7 @@ def test_get_container_children_count_queries( django_assert_num_queries, ): """Test how many database queries `get_container_children_count()` needs""" - publishing_api.publish_all_drafts(lp.pk) + publishing_api.publish_all_drafts(lp.id) # The 6 queries are: # - Draft.objects.get() # - PublishableEntityVersion.objects.get() diff --git a/tests/openedx_content/applets/publishing/test_api.py b/tests/openedx_content/applets/publishing/test_api.py index 96012633d..8da51120a 100644 --- a/tests/openedx_content/applets/publishing/test_api.py +++ b/tests/openedx_content/applets/publishing/test_api.py @@ -927,7 +927,7 @@ def test_simple_publish_log(self) -> None: ) publish_e1_log = publishing_api.publish_from_drafts( self.learning_package_1.id, - Draft.objects.filter(pk=entity1.pk), + Draft.objects.filter(pk=entity1.id), ) assert publish_e1_log.records.count() == 1 e1_pub_record = publish_e1_log.records.get(entity=entity1) @@ -1081,7 +1081,7 @@ def test_parent_child_side_effects(self) -> None: container_cls=TestContainer, ) container_v1 = containers_api.create_container_version( - container.pk, + container.id, 1, title="My Container", entities=[ @@ -1161,7 +1161,7 @@ def test_bulk_parent_child_side_effects(self) -> None: container_cls=TestContainer, ) container_v1 = containers_api.create_container_version( - container.pk, + container.id, 1, title="My Container", entities=[child_1, child_2], @@ -1245,7 +1245,7 @@ def test_draft_dependency_multiple_parents(self) -> None: ) for unit in [unit_1, unit_2]: containers_api.create_container_version( - unit.pk, + unit.id, 1, title="My Unit", entities=[component], @@ -1299,7 +1299,7 @@ def test_multiple_layers_of_containers(self) -> None: container_cls=TestContainer, ) containers_api.create_container_version( - unit.pk, + unit.id, 1, title="My Unit", entities=[component], @@ -1314,7 +1314,7 @@ def test_multiple_layers_of_containers(self) -> None: container_cls=TestContainer, ) containers_api.create_container_version( - subsection.pk, + subsection.id, 1, title="My Subsection", entities=[unit], @@ -1350,7 +1350,7 @@ def test_multiple_layers_of_containers(self) -> None: assert publish_log.records.count() == 3 publishing_api.create_publishable_entity_version( - component.pk, + component.id, version_num=3, title="Component v2", created=self.now, @@ -1358,7 +1358,7 @@ def test_multiple_layers_of_containers(self) -> None: ) publish_log = publishing_api.publish_from_drafts( self.learning_package.id, - Draft.objects.filter(entity_id=component.pk), + Draft.objects.filter(entity_id=component.id), ) assert publish_log.records.count() == 3 component_publish = publish_log.records.get(entity=component) @@ -1398,7 +1398,7 @@ def test_publish_all_layers(self) -> None: container_cls=TestContainer, ) containers_api.create_container_version( - unit.pk, + unit.id, 1, title="My Unit", entities=[component], @@ -1413,7 +1413,7 @@ def test_publish_all_layers(self) -> None: container_cls=TestContainer, ) containers_api.create_container_version( - subsection.pk, + subsection.id, 1, title="My Subsection", entities=[unit], @@ -1422,7 +1422,7 @@ def test_publish_all_layers(self) -> None: ) publish_log = publishing_api.publish_from_drafts( self.learning_package.id, - Draft.objects.filter(pk=subsection.pk), + Draft.objects.filter(pk=subsection.id), ) # The component, unit, and subsection should all be accounted for in @@ -1446,7 +1446,7 @@ def test_container_next_version(self) -> None: ) assert container.versioning.latest is None v1 = containers_api.create_next_container_version( - container.pk, + container.id, title="My Container v1", entities=None, created=self.now, @@ -1455,7 +1455,7 @@ def test_container_next_version(self) -> None: assert v1.version_num == 1 assert container.versioning.latest == v1 v2 = containers_api.create_next_container_version( - container.pk, + container.id, title="My Container v2", entities=[child_1], created=self.now, @@ -1465,7 +1465,7 @@ def test_container_next_version(self) -> None: assert container.versioning.latest == v2 assert v2.entity_list.entitylistrow_set.count() == 1 v3 = containers_api.create_next_container_version( - container.pk, + container.id, title="My Container v3", entities=None, created=self.now, diff --git a/tests/openedx_content/applets/publishing/test_models.py b/tests/openedx_content/applets/publishing/test_models.py index 0a924fba9..ccfbd9fec 100644 --- a/tests/openedx_content/applets/publishing/test_models.py +++ b/tests/openedx_content/applets/publishing/test_models.py @@ -1,9 +1,14 @@ """ Tests related to the Publishing model mixins """ + from typing import TYPE_CHECKING, assert_type -from openedx_content.applets.publishing.models import PublishableEntityMixin, PublishableEntityVersionMixin +from openedx_content.applets.publishing.models import ( + PublishableEntity, + PublishableEntityMixin, + PublishableEntityVersionMixin, +) from openedx_django_lib.managers import WithRelationsManager if TYPE_CHECKING: @@ -19,3 +24,8 @@ class FooEntityVersion(PublishableEntityVersionMixin): assert_type(FooEntityVersion.objects.create(), FooEntityVersion) assert_type(FooEntityVersion.objects, WithRelationsManager[FooEntityVersion]) + + # Test typing of PublishableEntity identifiers. + pe = PublishableEntity() + assert_type(pe.pk, PublishableEntity.ID) + assert_type(pe.id, PublishableEntity.ID) # `id` should show as deprecated diff --git a/tests/openedx_content/applets/sections/test_api.py b/tests/openedx_content/applets/sections/test_api.py index ef631e884..d3464b6bb 100644 --- a/tests/openedx_content/applets/sections/test_api.py +++ b/tests/openedx_content/applets/sections/test_api.py @@ -2,7 +2,7 @@ Basic tests for the sections API. """ -from typing import Any +from typing import Any, cast import pytest from django.core.exceptions import ValidationError @@ -77,7 +77,7 @@ def create_section_with_subsections( ) return section - def test_create_empty_section_and_version(self): + def test_create_empty_section_and_version(self) -> None: """Test creating a section with no units. Expected results: @@ -87,7 +87,7 @@ def test_create_empty_section_and_version(self): 4. There is no published version of the section. """ section, section_version = content_api.create_section_and_version( - learning_package_id=self.learning_package.pk, + learning_package_id=self.learning_package.id, key="section:key", title="Section", created=self.now, @@ -103,7 +103,7 @@ def test_create_empty_section_and_version(self): assert section.versioning.published is None assert section.publishable_entity.can_stand_alone - def test_create_next_section_version_with_unpinned_subsections(self): + def test_create_next_section_version_with_unpinned_subsections(self) -> None: """Test creating a unit version with an unpinned unit. Expected results: @@ -134,19 +134,20 @@ def test_get_section(self) -> None: """Test `get_section()`""" section = self.create_section_with_subsections([self.subsection_1, self.subsection_2]) - section_retrieved = content_api.get_section(section.pk) + section_retrieved = content_api.get_section(section.id) assert isinstance(section_retrieved, Section) assert section_retrieved == section def test_get_section_nonexistent(self) -> None: """Test `get_section()` when the subsection doesn't exist""" + FAKE_ID = cast(Section.ID, -500) with pytest.raises(Section.DoesNotExist): - content_api.get_section(-500) + content_api.get_section(FAKE_ID) def test_get_section_other_container_type(self) -> None: - """Test `get_section()` when the provided PK is for a non-Subsection container""" + """Test `get_section()` when the provided ID is for a non-Subsection container""" with pytest.raises(Section.DoesNotExist): - content_api.get_section(self.unit_1.pk) + content_api.get_section(self.unit_1.id) # type: ignore[arg-type] def test_section_queries(self) -> None: """ @@ -157,7 +158,7 @@ def test_section_queries(self) -> None: with self.assertNumQueries(160): content_api.publish_from_drafts( self.learning_package.id, - draft_qset=content_api.get_all_drafts(self.learning_package.id).filter(entity=section.pk), + draft_qset=content_api.get_all_drafts(self.learning_package.id).filter(entity=section.id), ) with self.assertNumQueries(4): result = content_api.get_subsections_in_section(section, published=True) @@ -186,7 +187,7 @@ def test_create_section_with_invalid_children(self): ) # Check that a new version was not created: section.refresh_from_db() - assert content_api.get_section(section.pk).versioning.draft == section_version + assert content_api.get_section(section.id).versioning.draft == section_version assert section.versioning.draft == section_version # Also check that `create_section_with_subsections()` has the same restriction diff --git a/tests/openedx_content/applets/subsections/test_api.py b/tests/openedx_content/applets/subsections/test_api.py index a79c05d49..344951304 100644 --- a/tests/openedx_content/applets/subsections/test_api.py +++ b/tests/openedx_content/applets/subsections/test_api.py @@ -2,6 +2,8 @@ Basic tests for the subsections API. """ +from typing import cast + import pytest from django.core.exceptions import ValidationError @@ -63,7 +65,7 @@ def test_create_empty_subsection_and_version(self): 4. There is no published version of the subsection. """ subsection, subsection_version = content_api.create_subsection_and_version( - learning_package_id=self.learning_package.pk, + learning_package_id=self.learning_package.id, key="subsection:key", title="Subsection", created=self.now, @@ -110,19 +112,20 @@ def test_get_subsection(self) -> None: """Test `get_subsection()`""" subsection = self.create_subsection_with_units([self.unit_1]) - subsection_retrieved = content_api.get_subsection(subsection.pk) + subsection_retrieved = content_api.get_subsection(subsection.id) assert isinstance(subsection_retrieved, Subsection) assert subsection_retrieved == subsection def test_get_subsection_nonexistent(self) -> None: """Test `get_subsection()` when the subsection doesn't exist""" + FAKE_ID = cast(Subsection.ID, -500) with pytest.raises(Subsection.DoesNotExist): - content_api.get_subsection(-500) + content_api.get_subsection(FAKE_ID) def test_get_subsection_other_container_type(self) -> None: """Test `get_subsection()` when the provided PK is for a non-Subsection container""" with pytest.raises(Subsection.DoesNotExist): - content_api.get_subsection(self.unit_1.pk) + content_api.get_subsection(self.unit_1.id) # type: ignore[arg-type] def test_subsection_queries(self) -> None: """ @@ -133,7 +136,7 @@ def test_subsection_queries(self) -> None: with self.assertNumQueries(102): # TODO: this seems high? content_api.publish_from_drafts( self.learning_package.id, - draft_qset=content_api.get_all_drafts(self.learning_package.id).filter(entity=subsection.pk), + draft_qset=content_api.get_all_drafts(self.learning_package.id).filter(entity=subsection.id), ) with self.assertNumQueries(4): result = content_api.get_units_in_subsection(subsection, published=True) @@ -165,7 +168,7 @@ def test_create_subsection_with_invalid_children(self): # Check that a new version was not created: subsection.refresh_from_db() - assert content_api.get_subsection(subsection.pk).versioning.draft == subsection_version + assert content_api.get_subsection(subsection.id).versioning.draft == subsection_version assert subsection.versioning.draft == subsection_version # Also check that `create_subsection_with_units()` has the same restriction diff --git a/tests/openedx_content/applets/units/test_api.py b/tests/openedx_content/applets/units/test_api.py index 22b867b04..944c5d076 100644 --- a/tests/openedx_content/applets/units/test_api.py +++ b/tests/openedx_content/applets/units/test_api.py @@ -2,6 +2,8 @@ Basic tests for the units API. """ +from typing import cast + import pytest from django.core.exceptions import ValidationError @@ -56,7 +58,7 @@ def test_create_empty_unit_and_version(self): 4. There is no published version of the unit. """ unit, unit_version = content_api.create_unit_and_version( - learning_package_id=self.learning_package.pk, + learning_package_id=self.learning_package.id, key="unit:key", title="Unit", created=self.now, @@ -104,17 +106,18 @@ def test_get_unit(self) -> None: """Test `get_unit()`""" unit = self.create_unit_with_components([self.component_1]) - unit_retrieved = content_api.get_unit(unit.pk) + unit_retrieved = content_api.get_unit(unit.id) assert isinstance(unit_retrieved, Unit) assert unit_retrieved == unit def test_get_unit_nonexistent(self) -> None: """Test `get_unit()` when the unit doesn't exist""" + FAKE_ID = cast(Unit.ID, -500) with pytest.raises(Unit.DoesNotExist): - content_api.get_unit(-500) + content_api.get_unit(FAKE_ID) def test_get_unit_other_container_type(self) -> None: - """Test `get_unit()` when the provided PK is for a non-Unit container""" + """Test `get_unit()` when the provided ID is for a non-Unit container""" other_container = content_api.create_container( self.learning_package.id, key="test", @@ -123,7 +126,7 @@ def test_get_unit_other_container_type(self) -> None: container_cls=TestContainer, ) with pytest.raises(Unit.DoesNotExist): - content_api.get_unit(other_container.pk) + content_api.get_unit(other_container.id) # type: ignore[arg-type] def test_unit_queries(self) -> None: """ @@ -134,7 +137,7 @@ def test_unit_queries(self) -> None: with self.assertNumQueries(48): # TODO: this seems high? content_api.publish_from_drafts( self.learning_package.id, - draft_qset=content_api.get_all_drafts(self.learning_package.id).filter(entity=unit.pk), + draft_qset=content_api.get_all_drafts(self.learning_package.id).filter(entity=unit.id), ) with self.assertNumQueries(3): result = content_api.get_components_in_unit(unit, published=True) @@ -167,7 +170,7 @@ def test_create_unit_with_invalid_children(self): # Try adding a generic entity to a Unit pe = content_api.create_publishable_entity(self.learning_package.id, "pe", created=self.now, created_by=None) pev = content_api.create_publishable_entity_version( - pe.pk, version_num=1, title="t", created=self.now, created_by=None + pe.id, version_num=1, title="t", created=self.now, created_by=None ) with pytest.raises(ValidationError, match='The entity "pe" cannot be added to a "unit" container.') as err: content_api.create_next_unit_version( @@ -180,7 +183,7 @@ def test_create_unit_with_invalid_children(self): # Check that a new version was not created: unit.refresh_from_db() - assert content_api.get_unit(unit.pk).versioning.draft == unit_version + assert content_api.get_unit(unit.id).versioning.draft == unit_version assert unit.versioning.draft == unit_version # Also check that `create_unit_and_version()` has the same restriction (not just `create_next_unit_version()`) diff --git a/tox.ini b/tox.ini index e432cf83a..ad8736086 100644 --- a/tox.ini +++ b/tox.ini @@ -33,6 +33,8 @@ match-dir = (?!migrations) DJANGO_SETTINGS_MODULE = test_settings addopts = --cov src --cov tests --cov-report term-missing --cov-report xml norecursedirs = .* docs requirements site-packages +filterwarnings = + ignore:Use \.id instead:DeprecationWarning:django\..* [testenv] usedevelop = True # installs -e .