diff --git a/.gitignore b/.gitignore index ea7591535..828320592 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ +.claude *.py[cod] __pycache__ .pytest_cache diff --git a/olx_importer/management/commands/load_components.py b/olx_importer/management/commands/load_components.py index 175175040..dae8ffeec 100644 --- a/olx_importer/management/commands/load_components.py +++ b/olx_importer/management/commands/load_components.py @@ -61,12 +61,12 @@ def init_known_types(self): def add_arguments(self, parser): parser.add_argument("course_data_path", type=pathlib.Path) - parser.add_argument("learning_package_key", type=str) + parser.add_argument("learning_package_ref", type=str) - def handle(self, course_data_path, learning_package_key, **options): + def handle(self, course_data_path, learning_package_ref, **options): self.course_data_path = course_data_path - self.learning_package_key = learning_package_key - self.load_course_data(learning_package_key) + self.learning_package_ref = learning_package_ref + self.load_course_data(learning_package_ref) def get_course_title(self): course_type_dir = self.course_data_path / "course" @@ -74,20 +74,20 @@ def get_course_title(self): course_root = ET.parse(course_xml_file).getroot() return course_root.attrib.get("display_name", "Unknown Course") - def load_course_data(self, learning_package_key): + def load_course_data(self, learning_package_ref): print(f"Importing course from: {self.course_data_path}") now = datetime.now(timezone.utc) title = self.get_course_title() - if content_api.learning_package_exists(learning_package_key): + if content_api.learning_package_exists(learning_package_ref): raise CommandError( - f"{learning_package_key} already exists. " + f"{learning_package_ref} already exists. " "This command currently only supports initial import." ) with transaction.atomic(): self.learning_package = content_api.create_learning_package( - learning_package_key, title, created=now, + learning_package_ref, title, created=now, ) for block_type in SUPPORTED_TYPES: self.import_block_type(block_type, now) #, publish_log_entry) @@ -140,7 +140,7 @@ def import_block_type(self, block_type_name, now): # , publish_log_entry): for xml_file_path in block_data_path.glob("*.xml"): components_found += 1 - local_key = xml_file_path.stem + component_code = xml_file_path.stem # Do some basic parsing of the content to see if it's even well # constructed enough to add (or whether we should skip/error on it). @@ -155,7 +155,7 @@ def import_block_type(self, block_type_name, now): # , publish_log_entry): _component, component_version = content_api.create_component_and_version( self.learning_package.id, component_type=block_type, - local_key=local_key, + component_code=component_code, title=display_name, created=now, created_by=None, @@ -173,7 +173,7 @@ def import_block_type(self, block_type_name, now): # , publish_log_entry): content_api.create_component_version_media( component_version, text_content.pk, - key="block.xml", + path="block.xml", ) # Cycle through static assets references and add those as well... diff --git a/src/openedx_content/applets/backup_restore/api.py b/src/openedx_content/applets/backup_restore/api.py index b4cda8828..1f768b0b0 100644 --- a/src/openedx_content/applets/backup_restore/api.py +++ b/src/openedx_content/applets/backup_restore/api.py @@ -5,26 +5,28 @@ from django.contrib.auth.models import User as UserType # pylint: disable=imported-auth-user -from ..publishing.api import get_learning_package_by_key +from ..publishing.api import get_learning_package_by_ref from .zipper import LearningPackageUnzipper, LearningPackageZipper -def create_zip_file(lp_key: str, path: str, user: UserType | None = None, origin_server: str | None = None) -> None: +def create_zip_file( + package_ref: str, path: str, user: UserType | None = None, origin_server: str | None = None +) -> None: """ Creates a dump zip file for the given learning package key at the given path. The zip file contains a TOML representation of the learning package and its contents. - Can throw a NotFoundError at get_learning_package_by_key + Can throw a NotFoundError at get_learning_package_by_ref """ - learning_package = get_learning_package_by_key(lp_key) + learning_package = get_learning_package_by_ref(package_ref) LearningPackageZipper(learning_package, user, origin_server).create_zip(path) -def load_learning_package(path: str, key: str | None = None, user: UserType | None = None) -> dict: +def load_learning_package(path: str, package_ref: str | None = None, user: UserType | None = None) -> dict: """ Loads a learning package from a zip file at the given path. Restores the learning package and its contents to the database. Returns a dictionary with the status of the operation and any errors encountered. """ with zipfile.ZipFile(path, "r") as zipf: - return LearningPackageUnzipper(zipf, key, user).load() + return LearningPackageUnzipper(zipf, package_ref, user).load() diff --git a/src/openedx_content/applets/backup_restore/serializers.py b/src/openedx_content/applets/backup_restore/serializers.py index d8f7a5c15..28b09af5c 100644 --- a/src/openedx_content/applets/backup_restore/serializers.py +++ b/src/openedx_content/applets/backup_restore/serializers.py @@ -12,15 +12,30 @@ class LearningPackageSerializer(serializers.Serializer): # pylint: disable=abst """ Serializer for learning packages. + Archives created in Verawood or later write ``package_ref``. Archives + created in Ulmo write ``key``. Both are accepted; ``package_ref`` takes + precedence. + Note: - The `key` field is serialized, but it is generally not trustworthy for restoration. - During restore, a new key may be generated or overridden. + The ref/key field is serialized but is generally not trustworthy for + restoration. During restore, a new ref may be generated or overridden. """ + title = serializers.CharField(required=True) - key = serializers.CharField(required=True) + package_ref = serializers.CharField(required=False) + key = serializers.CharField(required=False) description = serializers.CharField(required=True, allow_blank=True) created = serializers.DateTimeField(required=True, default_timezone=timezone.utc) + def validate(self, attrs): + package_ref = attrs.pop("package_ref", None) + legacy_key = attrs.pop("key", None) + ref = package_ref or legacy_key + if not ref: + raise serializers.ValidationError("Either 'package_ref' or 'key' is required.") + attrs["package_ref"] = ref # Normalise to 'package_ref' for create_learning_package. + return attrs + class LearningPackageMetadataSerializer(serializers.Serializer): # pylint: disable=abstract-method """ @@ -40,18 +55,33 @@ class LearningPackageMetadataSerializer(serializers.Serializer): # pylint: disa class EntitySerializer(serializers.Serializer): # pylint: disable=abstract-method """ Serializer for publishable entities. + + Archives created in Verawood or later write ``entity_ref``. Archives + created in Ulmo use ``key``. Both are accepted; ``entity_ref`` takes + precedence. """ + can_stand_alone = serializers.BooleanField(required=True) - key = serializers.CharField(required=True) + entity_ref = serializers.CharField(required=False) + key = serializers.CharField(required=False) created = serializers.DateTimeField(required=True, default_timezone=timezone.utc) + def validate(self, attrs): + entity_ref = attrs.pop("entity_ref", None) + legacy_key = attrs.pop("key", None) + ref = entity_ref or legacy_key + if not ref: + raise serializers.ValidationError("Either 'entity_ref' or 'key' is required.") + attrs["entity_ref"] = ref + return attrs + class EntityVersionSerializer(serializers.Serializer): # pylint: disable=abstract-method """ Serializer for publishable entity versions. """ title = serializers.CharField(required=True) - entity_key = serializers.CharField(required=True) + entity_ref = serializers.CharField(required=True) created = serializers.DateTimeField(required=True, default_timezone=timezone.utc) version_num = serializers.IntegerField(required=True) @@ -59,21 +89,57 @@ class EntityVersionSerializer(serializers.Serializer): # pylint: disable=abstra class ComponentSerializer(EntitySerializer): # pylint: disable=abstract-method """ Serializer for components. - Contains logic to convert entity_key to component_type and local_key. + + Extracts component_type and component_code from the [entity.component] + section if present (archives created in Verawood or later). Falls back to + parsing the entity key for archives created in Ulmo. """ + component = serializers.DictField(required=False) + def validate(self, attrs): """ - Custom validation logic: - parse the entity_key into (component_type, local_key). + Custom validation logic: resolve component_type and component_code. + + Archives created in Verawood or later supply an [entity.component] + section with ``component_type`` (e.g. "xblock.v1:problem") and + ``component_code`` (e.g. "my_example"). Archives created in Ulmo only + have the entity ``key`` in the format + ``"{namespace}:{type_name}:{component_code}"``, so we fall back to + parsing that for backwards compatibility. """ - entity_key = attrs["key"] - try: - component_type_obj, local_key = components_api.get_or_create_component_type_by_entity_key(entity_key) - attrs["component_type"] = component_type_obj - attrs["local_key"] = local_key - except ValueError as exc: - raise serializers.ValidationError({"key": str(exc)}) + super().validate(attrs) + component_section = attrs.pop("component", None) + if component_section: + # Verawood+ format: component_type and component_code are explicit. + component_type_str = component_section.get("component_type", "") + component_code = component_section.get("component_code", "") + try: + namespace, type_name = component_type_str.split(":", 1) + except ValueError as exc: + raise serializers.ValidationError( + {"component": f"Invalid component_type format: {component_type_str!r}. " + "Expected '{namespace}:{type_name}'."} + ) from exc + component_type_obj = components_api.get_or_create_component_type(namespace, type_name) + else: + # Ulmo (legacy) format: parse the entity_ref (which ws normalized + # from "key" in super.validate()) assuming the format: + # (namespace, type_name, component_code). This parsing is + # intentionally only here — entity_ref must not be parsed anywhere + # else in the codebase. Verawood+ archives may not follow this + # convention. + entity_ref = attrs["entity_ref"] + try: + namespace, type_name, component_code = entity_ref.split(":", 2) + except ValueError as exc: + raise serializers.ValidationError( + {"key": f"Invalid entity key format: {entity_ref!r}. " + "Expected '{namespace}:{type_name}:{component_code}'."} + ) from exc + component_type_obj = components_api.get_or_create_component_type(namespace, type_name) + attrs["component_type"] = component_type_obj + attrs["component_code"] = component_code return attrs @@ -86,35 +152,46 @@ class ComponentVersionSerializer(EntityVersionSerializer): # pylint: disable=ab class ContainerSerializer(EntitySerializer): # pylint: disable=abstract-method """ Serializer for containers. + + Extracts container_code from the [entity.container] section. + Archives created in Verawood or later include an explicit + ``container_code`` field. Archives created in Ulmo do not, so we + fall back to using the entity key as the container_code. """ + container = serializers.DictField(required=True) def validate_container(self, value): """ Custom validation logic for the container field. - Ensures that the container dict has exactly one key which is one of - "section", "subsection", or "unit" values. + Ensures that the container dict has exactly one type key ("section", + "subsection", or "unit"), optionally alongside "container_code". """ errors = [] - if not isinstance(value, dict) or len(value) != 1: - errors.append("Container must be a dict with exactly one key.") - if len(value) == 1: # Only check the key if there is exactly one - container_type = list(value.keys())[0] - if container_type not in ("section", "subsection", "unit"): - errors.append(f"Invalid container value: {container_type}") + type_keys = [k for k in value if k in ("section", "subsection", "unit")] + if len(type_keys) != 1: + errors.append( + "Container must have exactly one type key: 'section', 'subsection', or 'unit'." + ) if errors: raise serializers.ValidationError(errors) return value def validate(self, attrs): """ - Custom validation logic: - parse the container dict to extract the container type. + Custom validation logic: extract container_type and container_code. + + Archives created in Verawood or later supply an explicit + ``container_code`` field inside [entity.container]. Archives created + in Ulmo do not, so we fall back to using the entity key. """ - container = attrs["container"] - container_type = list(container.keys())[0] # It is safe to do this after validate_container + super().validate(attrs) + container = attrs.pop("container") + # It is safe to do this after validate_container + container_type = next(k for k in container if k in ("section", "subsection", "unit")) attrs["container_type"] = container_type - attrs.pop("container") # Remove the container field after processing + # Verawood+: container_code is explicit. Ulmo: fall back to entity_ref. + attrs["container_code"] = container.get("container_code") or attrs["entity_ref"] return attrs @@ -156,10 +233,24 @@ class CollectionSerializer(serializers.Serializer): # pylint: disable=abstract- Serializer for collections. """ title = serializers.CharField(required=True) - key = serializers.CharField(required=True) + # 'collection_code' is the current field name; 'key' is the old name kept for + # back-compat with archives written before the rename. At least one must be present. + collection_code = serializers.CharField(required=False) + key = serializers.CharField(required=False) description = serializers.CharField(required=True, allow_blank=True) entities = serializers.ListField( child=serializers.CharField(), required=True, allow_empty=True, ) + + def validate(self, attrs): + # Prefer 'collection_code'; fall back to legacy 'key'. Always remove + # both so only the normalised 'collection_code' key reaches the caller. + code = attrs.pop("collection_code", None) + legacy_key = attrs.pop("key", None) + code = code or legacy_key + if not code: + raise serializers.ValidationError("Either 'collection_code' or 'key' is required.") + attrs["collection_code"] = code + return attrs diff --git a/src/openedx_content/applets/backup_restore/toml.py b/src/openedx_content/applets/backup_restore/toml.py index d39861803..2baac11df 100644 --- a/src/openedx_content/applets/backup_restore/toml.py +++ b/src/openedx_content/applets/backup_restore/toml.py @@ -43,7 +43,9 @@ def toml_learning_package( # Learning package main info section = tomlkit.table() section.add("title", learning_package.title) - section.add("key", learning_package.key) + # Write package_ref (Verawood+) and key (Ulmo back-compat). + section.add("package_ref", learning_package.package_ref) + section.add("key", learning_package.package_ref) section.add("description", learning_package.description) section.add("created", learning_package.created) section.add("updated", learning_package.updated) @@ -89,8 +91,10 @@ def _get_toml_publishable_entity_table( """ entity_table = tomlkit.table() entity_table.add("can_stand_alone", entity.can_stand_alone) - # Add key since the toml filename doesn't show the real key - entity_table.add("key", entity.key) + # Write entity_ref (Verawood+) and key (Ulmo back-compat) so that older + # restore code can still read archives produced after this rename. + entity_table.add("entity_ref", entity.entity_ref) + entity_table.add("key", entity.entity_ref) entity_table.add("created", entity.created) if not include_versions: @@ -108,12 +112,25 @@ def _get_toml_publishable_entity_table( published_table.add(tomlkit.comment("unpublished: no published_version_num")) entity_table.add("published", published_table) + if hasattr(entity, "component"): + component = entity.component + component_table = tomlkit.table() + # Write component_type and component_code explicitly so that restore + # (Verawood and later) does not need to parse the entity key. + component_table.add("component_type", str(component.component_type)) + component_table.add("component_code", component.component_code) + entity_table.add("component", component_table) + if hasattr(entity, "container"): + container = entity.container container_table = tomlkit.table() + # Write container_code explicitly so that restore (Verawood and later) + # does not need to parse the entity key. + container_table.add("container_code", container.container_code) container_types = ["section", "subsection", "unit"] for container_type in container_types: - if hasattr(entity.container, container_type): + if hasattr(container, container_type): container_table.add(container_type, tomlkit.table()) break # stop after the first match @@ -191,13 +208,13 @@ def toml_publishable_entity_version(version: PublishableEntityVersion) -> tomlki if hasattr(version, 'containerversion'): # If the version has a container version, add its children container_table = tomlkit.table() - children = containers_api.get_container_children_entities_keys(version.containerversion) + children = containers_api.get_container_children_entity_refs(version.containerversion) container_table.add("children", children) version_table.add("container", container_table) return version_table -def toml_collection(collection: Collection, entity_keys: list[str]) -> str: +def toml_collection(collection: Collection, entity_refs: list[str]) -> str: """ Create a TOML representation of a collection. @@ -215,12 +232,15 @@ def toml_collection(collection: Collection, entity_keys: list[str]) -> str: doc = tomlkit.document() entities_array = tomlkit.array() - entities_array.extend(entity_keys) + entities_array.extend(entity_refs) entities_array.multiline(True) collection_table = tomlkit.table() collection_table.add("title", collection.title) - collection_table.add("key", collection.key) + # Write both names so that older software (which reads 'key') stays compatible + # with archives produced after the Collection.key -> Collection.collection_code rename. + collection_table.add("key", collection.collection_code) + collection_table.add("collection_code", collection.collection_code) collection_table.add("description", collection.description) collection_table.add("created", collection.created) collection_table.add("entities", entities_array) diff --git a/src/openedx_content/applets/backup_restore/zipper.py b/src/openedx_content/applets/backup_restore/zipper.py index 29b9638e9..c312d09d6 100644 --- a/src/openedx_content/applets/backup_restore/zipper.py +++ b/src/openedx_content/applets/backup_restore/zipper.py @@ -202,7 +202,7 @@ def get_publishable_entities(self) -> QuerySet[PublishableEntity]: to_attr="prefetched_media", ), ) - .order_by("key") + .order_by("entity_ref") ) def get_collections(self) -> QuerySet[Collection]: @@ -238,25 +238,25 @@ def get_versions_to_write( versions_to_write.append(published_version) return versions_to_write, draft_version, published_version - def get_entity_toml_filename(self, entity_key: str) -> str: + def get_entity_toml_filename(self, entity_ref: str) -> str: """ Generate a unique TOML filename for a publishable entity. Ensures that the filename is unique within the zip file. Behavior: - - If the slugified key has not been used yet, use it as the filename. + - If the slugified ref has not been used yet, use it as the filename. - If it has been used, append a short hash to ensure uniqueness. Args: - entity_key (str): The key of the publishable entity. + entity_ref (str): The entity_ref of the publishable entity. Returns: str: A unique TOML filename for the entity. """ - slugify_name = slugify(entity_key, allow_unicode=True) + slugify_name = slugify(entity_ref, allow_unicode=True) if slugify_name in self.entities_filenames_already_created: - filename = slugify_hashed_filename(entity_key) + filename = slugify_hashed_filename(entity_ref) else: filename = slugify_name @@ -316,7 +316,7 @@ def create_zip(self, path: str) -> None: ) if hasattr(entity, 'container'): - entity_filename = self.get_entity_toml_filename(entity.key) + entity_filename = self.get_entity_toml_filename(entity.entity_ref) entity_toml_filename = f"{entity_filename}.toml" entity_toml_path = entities_folder / entity_toml_filename self.add_file_to_zip(zipf, entity_toml_path, entity_toml_content, timestamp=latest_modified) @@ -332,7 +332,7 @@ def create_zip(self, path: str) -> None: # v1/ # static/ - entity_filename = self.get_entity_toml_filename(entity.component.local_key) + entity_filename = self.get_entity_toml_filename(entity.component.component_code) component_root_folder = ( # Example: "entities/xblock.v1/html/" @@ -381,9 +381,8 @@ def create_zip(self, path: str) -> None: for component_version_media in prefetched_media: media: Media = component_version_media.media - # Important: The component_version_media.key contains implicitly - # the file name and the file extension - file_path = version_folder / component_version_media.key + # component_version_media.path is the file name and extension + file_path = version_folder / component_version_media.path if media.has_file and media.path: # If has_file, we pull it from the file system @@ -401,13 +400,13 @@ def create_zip(self, path: str) -> None: collections = self.get_collections() for collection in collections: - collection_hash_slug = self.get_entity_toml_filename(collection.key) + collection_hash_slug = self.get_entity_toml_filename(collection.collection_code) collection_toml_file_path = collections_folder / f"{collection_hash_slug}.toml" - entity_keys_related = collection.entities.order_by("key").values_list("key", flat=True) + entity_refs_related = collection.entities.order_by("entity_ref").values_list("entity_ref", flat=True) self.add_file_to_zip( zipf, collection_toml_file_path, - toml_collection(collection, list(entity_keys_related)), + toml_collection(collection, list(entity_refs_related)), timestamp=collection.modified, ) @@ -418,10 +417,10 @@ class RestoreLearningPackageData: Data about the restored learning package. """ id: int # The ID of the restored learning package - key: str # The key of the restored learning package (may be different if staged) - archive_lp_key: str # The original key from the archive - archive_org_key: str # The original organization key from the archive - archive_slug: str # The original slug from the archive + package_ref: str # The package_ref of the restored learning package (may be different if staged) + archive_package_ref: str # The original package_ref from the archive + archive_org_code: str | None # The org code parsed from archive_package_ref, or None if unparseable + archive_package_code: str | None # The package code parsed from archive_package_ref, or None if unparseable title: str num_containers: int num_sections: int @@ -454,35 +453,43 @@ class RestoreResult: backup_metadata: BackupMetadata | None = None -def unpack_lp_key(lp_key: str) -> tuple[str, str]: +def unpack_package_ref(package_ref: str) -> tuple[str | None, str | None]: """ - Unpack a learning package key into its components. + Try to parse org_code and package_code from a package_ref. + + By convention, package_refs take the form ``"{prefix}:{org_code}:{package_code}"``, + but this is only a convention — package_ref is opaque and the parse may fail. + Returns ``(None, None)`` if the ref does not match the expected format. """ - parts = lp_key.split(":") + parts = package_ref.split(":") if len(parts) < 3: - raise ValueError(f"Invalid learning package key: {lp_key}") - _, org_key, lp_slug = parts[:3] - return org_key, lp_slug + return None, None + _, org_code, package_code = parts[:3] + return org_code, package_code -def generate_staged_lp_key(archive_lp_key: str, user: UserType) -> str: +def generate_staged_package_ref(archive_package_ref: str, user: UserType) -> str: """ - Generate a staged learning package key based on the given base key. + Generate a staged learning package ref based on the archive's package_ref. Arguments: - archive_lp_key (str): The original learning package key from the archive. + archive_package_ref (str): The original package_ref from the archive. user (UserType | None): The user performing the restore operation. Example: Input: "lib:WGU:LIB_C001" Output: "lp-restore:dave:WGU:LIB_C001:1728575321" - The timestamp at the end ensures the key is unique. + The timestamp at the end ensures the ref is unique. Falls back to using + the full archive_package_ref when the conventional format is not recognised. """ username = user.username - org_key, lp_slug = unpack_lp_key(archive_lp_key) + org_code, package_code = unpack_package_ref(archive_package_ref) timestamp = int(time.time() * 1000) # Current time in milliseconds - return f"lp-restore:{username}:{org_key}:{lp_slug}:{timestamp}" + if org_code and package_code: + return f"lp-restore:{username}:{org_code}:{package_code}:{timestamp}" + # Fallback for non-conventional package_refs + return f"lp-restore:{username}:{archive_package_ref}:{timestamp}" class LearningPackageUnzipper: @@ -507,21 +514,21 @@ class LearningPackageUnzipper: result = unzipper.load() """ - def __init__(self, zipf: zipfile.ZipFile, key: str | None = None, user: UserType | None = None): + def __init__(self, zipf: zipfile.ZipFile, package_ref: str | None = None, user: UserType | None = None): self.zipf = zipf 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.package_ref = package_ref # If provided, use this package_ref for the restored learning package self.learning_package_id: int | 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]] = [] # Maps for resolving relationships - self.components_map_by_key: dict[str, Any] = {} - self.units_map_by_key: dict[str, Any] = {} - self.subsections_map_by_key: dict[str, Any] = {} - self.sections_map_by_key: dict[str, Any] = {} - self.all_publishable_entities_keys: set[str] = set() + self.components_map_by_ref: dict[str, Any] = {} + self.units_map_by_ref: dict[str, Any] = {} + self.subsections_map_by_ref: dict[str, Any] = {} + self.sections_map_by_ref: dict[str, Any] = {} + self.all_publishable_entity_refs: set[str] = set() self.all_published_entities_versions: set[tuple[str, int]] = set() # To track published entity versions # -------------------------- @@ -574,7 +581,7 @@ def load(self) -> dict[str, Any]: # Step 3.2: Save everything to the DB # All validations passed, we can proceed to save everything # Save the learning package first to get its ID - archive_lp_key = learning_package_validated["key"] + archive_package_ref = learning_package_validated["package_ref"] learning_package = self._save( learning_package_validated, components_validated, @@ -588,16 +595,16 @@ def load(self) -> dict[str, Any]: for container_type in ["section", "subsection", "unit"] ) - org_key, lp_slug = unpack_lp_key(archive_lp_key) + org_code, package_code = unpack_package_ref(archive_package_ref) result = RestoreResult( status="success", log_file_error=None, lp_restored_data=RestoreLearningPackageData( id=learning_package.id, - key=learning_package.key, - archive_lp_key=archive_lp_key, # The original key from the backup archive - archive_org_key=org_key, # The original organization key from the backup archive - archive_slug=lp_slug, # The original slug from the backup archive + package_ref=learning_package.package_ref, + archive_package_ref=archive_package_ref, + archive_org_code=org_code, + archive_package_code=package_code, title=learning_package.title, num_containers=num_containers, num_sections=len(containers_validated.get("section", [])), @@ -678,7 +685,7 @@ def _extract_entities( continue entity_data = serializer.validated_data - self.all_publishable_entities_keys.add(entity_data["key"]) + self.all_publishable_entity_refs.add(entity_data["entity_ref"]) entity_type = entity_data.pop("container_type", "components") results[entity_type].append(entity_data) @@ -716,11 +723,14 @@ def _extract_collections( continue collection_validated = serializer.validated_data entities_list = collection_validated["entities"] - for entity_key in entities_list: - if entity_key not in self.all_publishable_entities_keys: + for entity_ref in entities_list: + if entity_ref not in self.all_publishable_entity_refs: self.errors.append({ "file": file, - "errors": f"Entity key {entity_key} not found for collection {collection_validated.get('key')}" + "errors": ( + f"Entity ref {entity_ref} not found for collection " + + collection_validated.get('collection_code') + ), }) results["collections"].append(collection_validated) @@ -743,16 +753,16 @@ def _save( # Important: If not using a specific LP key, generate a temporary one # We cannot use the original key because it may generate security issues - if not self.lp_key: - # Generate a tmp key for the staged learning package + if not self.package_ref: + # Generate a tmp ref for the staged learning package if not self.user: - raise ValueError("User is required to create lp_key") - learning_package["key"] = generate_staged_lp_key( - archive_lp_key=learning_package["key"], + raise ValueError("User is required to generate a staged package_ref") + learning_package["package_ref"] = generate_staged_package_ref( + archive_package_ref=learning_package["package_ref"], user=self.user ) else: - learning_package["key"] = self.lp_key + learning_package["package_ref"] = self.package_ref learning_package_obj = publishing_api.create_learning_package(**learning_package) self.learning_package_id = learning_package_obj.id @@ -779,26 +789,31 @@ def _save_collections(self, learning_package, collections): ) collection = collections_api.add_to_collection( learning_package_id=learning_package.id, - key=collection.key, - entities_qset=publishing_api.get_publishable_entities(learning_package.id).filter(key__in=entities) + collection_code=collection.collection_code, + entities_qset=publishing_api.get_publishable_entities(learning_package.id).filter( + entity_ref__in=entities + ) ) def _save_components(self, learning_package, components, component_static_files): """Save components and published component versions.""" for valid_component in components.get("components", []): - entity_key = valid_component.pop("key") + entity_ref = valid_component.pop("entity_ref") component = components_api.create_component(learning_package.id, created_by=self.user_id, **valid_component) - self.components_map_by_key[entity_key] = component + self.components_map_by_ref[entity_ref] = component for valid_published in components.get("components_published", []): - entity_key = valid_published.pop("entity_key") + entity_ref = valid_published.pop("entity_ref") version_num = valid_published["version_num"] # Should exist, validated earlier - media_to_replace = self._resolve_static_files(version_num, entity_key, component_static_files) + component = self.components_map_by_ref[entity_ref] + media_to_replace = self._resolve_static_files( + version_num, entity_ref, component.component_type, component_static_files + ) self.all_published_entities_versions.add( - (entity_key, version_num) + (entity_ref, version_num) ) # Track published version components_api.create_next_component_version( - self.components_map_by_key[entity_key].publishable_entity.id, + component.publishable_entity.id, media_to_replace=media_to_replace, force_version_num=valid_published.pop("version_num", None), created_by=self.user_id, @@ -817,23 +832,24 @@ def _save_container( """Internal logic for _save_units, _save_subsections, and _save_sections""" type_code = container_cls.type_code # e.g. "unit" for data in containers.get(type_code, []): - entity_key = data.get("key") + entity_ref = data.pop("entity_ref") container = containers_api.create_container( learning_package.id, **data, # should this be allowed to override any of the following fields? + entity_ref=entity_ref, created_by=self.user_id, container_cls=container_cls, ) - container_map[entity_key] = container # e.g. `self.units_map_by_key[entity_key] = unit` + container_map[entity_ref] = container # e.g. `self.units_map_by_ref[entity_ref] = unit` for valid_published in containers.get(f"{type_code}_published", []): - entity_key = valid_published.pop("entity_key") + entity_ref = valid_published.pop("entity_ref") children = self._resolve_children(valid_published, children_map) self.all_published_entities_versions.add( - (entity_key, valid_published.get('version_num')) + (entity_ref, valid_published.get('version_num')) ) # Track published version containers_api.create_next_container_version( - container_map[entity_key], + container_map[entity_ref], **valid_published, # should this be allowed to override any of the following fields? force_version_num=valid_published.pop("version_num", None), entities=children, @@ -846,8 +862,8 @@ def _save_units(self, learning_package, containers): learning_package, containers, container_cls=Unit, - container_map=self.units_map_by_key, - children_map=self.components_map_by_key, + container_map=self.units_map_by_ref, + children_map=self.components_map_by_ref, ) def _save_subsections(self, learning_package, containers): @@ -856,8 +872,8 @@ def _save_subsections(self, learning_package, containers): learning_package, containers, container_cls=Subsection, - container_map=self.subsections_map_by_key, - children_map=self.units_map_by_key, + container_map=self.subsections_map_by_ref, + children_map=self.units_map_by_ref, ) def _save_sections(self, learning_package, containers): @@ -866,20 +882,23 @@ def _save_sections(self, learning_package, containers): learning_package, containers, container_cls=Section, - container_map=self.sections_map_by_key, - children_map=self.subsections_map_by_key, + container_map=self.sections_map_by_ref, + children_map=self.subsections_map_by_ref, ) def _save_draft_versions(self, components, containers, component_static_files): """Save draft versions for all entity types.""" for valid_draft in components.get("components_drafts", []): - entity_key = valid_draft.pop("entity_key") + entity_ref = valid_draft.pop("entity_ref") version_num = valid_draft["version_num"] # Should exist, validated earlier - if self._is_version_already_exists(entity_key, version_num): + if self._is_version_already_exists(entity_ref, version_num): continue - media_to_replace = self._resolve_static_files(version_num, entity_key, component_static_files) + component = self.components_map_by_ref[entity_ref] + media_to_replace = self._resolve_static_files( + version_num, entity_ref, component.component_type, component_static_files + ) components_api.create_next_component_version( - self.components_map_by_key[entity_key].publishable_entity.id, + component.publishable_entity.id, media_to_replace=media_to_replace, force_version_num=valid_draft.pop("version_num", None), # Drafts can diverge from published, so we allow ignoring previous media @@ -895,23 +914,23 @@ def _process_draft_containers( children_map: dict, ): for valid_draft in containers.get(f"{container_cls.type_code}_drafts", []): - entity_key = valid_draft.pop("entity_key") + entity_ref = valid_draft.pop("entity_ref") version_num = valid_draft["version_num"] # Should exist, validated earlier - if self._is_version_already_exists(entity_key, version_num): + if self._is_version_already_exists(entity_ref, version_num): continue children = self._resolve_children(valid_draft, children_map) del valid_draft["version_num"] containers_api.create_next_container_version( - container_map[entity_key], + container_map[entity_ref], **valid_draft, # should this be allowed to override any of the following fields? entities=children, force_version_num=version_num, created_by=self.user_id, ) - _process_draft_containers(Unit, self.units_map_by_key, children_map=self.components_map_by_key) - _process_draft_containers(Subsection, self.subsections_map_by_key, children_map=self.units_map_by_key) - _process_draft_containers(Section, self.sections_map_by_key, children_map=self.subsections_map_by_key) + _process_draft_containers(Unit, self.units_map_by_ref, children_map=self.components_map_by_ref) + _process_draft_containers(Subsection, self.subsections_map_by_ref, children_map=self.units_map_by_ref) + _process_draft_containers(Section, self.sections_map_by_ref, children_map=self.subsections_map_by_ref) # -------------------------- # Utilities @@ -933,9 +952,9 @@ def _write_errors(self) -> StringIO | None: return None return StringIO(content) - def _is_version_already_exists(self, entity_key: str, version_num: int) -> bool: + def _is_version_already_exists(self, entity_ref: str, version_num: int) -> bool: """ - Check if a version already exists for a given entity key and version number. + Check if a version already exists for a given entity_ref and version number. Note: Skip creating draft if this version is already published @@ -944,20 +963,21 @@ def _is_version_already_exists(self, entity_key: str, version_num: int) -> bool: Otherwise, we will raise an IntegrityError on PublishableEntityVersion due to unique constraints between publishable_entity and version_num. """ - identifier = (entity_key, version_num) + identifier = (entity_ref, version_num) return identifier in self.all_published_entities_versions def _resolve_static_files( self, num_version: int, - entity_key: str, + entity_ref: str, + component_type, static_files_map: dict[str, List[str]] ) -> dict[str, bytes | int]: """Resolve static file paths into their binary media content.""" resolved_files: dict[str, bytes | int] = {} - static_file_key = f"{entity_key}:v{num_version}" # e.g., "xblock.v1:html:my_component_123456:v1" - block_type = entity_key.split(":")[1] # e.g., "html" + static_file_key = f"{entity_ref}:v{num_version}" # e.g., "xblock.v1:html:my_component_123456:v1" + block_type = component_type.name # e.g., "html" static_files = static_files_map.get(static_file_key, []) for static_file in static_files: local_key = static_file.split(f"v{num_version}/")[-1] @@ -980,9 +1000,9 @@ def _resolve_static_files( return resolved_files def _resolve_children(self, entity_data: dict[str, Any], lookup_map: dict[str, Any]) -> list[Any]: - """Resolve child entity keys into model instances.""" - children_keys = entity_data.pop("children", []) - return [lookup_map[key] for key in children_keys if key in lookup_map] + """Resolve child entity refs into model instances.""" + children_refs = entity_data.pop("children", []) + return [lookup_map[ref] for ref in children_refs if ref in lookup_map] def _load_entity_data( self, entity_file: str @@ -1002,7 +1022,7 @@ def _validate_versions(self, entity_data, draft, published, serializer_cls, *, f continue serializer = serializer_cls( data={ - "entity_key": entity_data["key"], + "entity_ref": entity_data["entity_ref"], "created": self.utc_now, "created_by": None, **version diff --git a/src/openedx_content/applets/collections/admin.py b/src/openedx_content/applets/collections/admin.py index eb0685a27..41db9e082 100644 --- a/src/openedx_content/applets/collections/admin.py +++ b/src/openedx_content/applets/collections/admin.py @@ -13,14 +13,14 @@ class CollectionAdmin(admin.ModelAdmin): Allows users to easily disable/enable (aka soft delete and restore) or bulk delete Collections. """ - readonly_fields = ["key", "learning_package"] + readonly_fields = ["collection_code", "learning_package"] list_filter = ["enabled"] - list_display = ["key", "title", "enabled", "modified"] + list_display = ["collection_code", "title", "enabled", "modified"] fieldsets = [ ( "", { - "fields": ["key", "learning_package"], + "fields": ["collection_code", "learning_package"], } ), ( diff --git a/src/openedx_content/applets/collections/api.py b/src/openedx_content/applets/collections/api.py index 8ab8d9cbf..759c2046c 100644 --- a/src/openedx_content/applets/collections/api.py +++ b/src/openedx_content/applets/collections/api.py @@ -34,7 +34,7 @@ def create_collection( learning_package_id: int, - key: str, + collection_code: str, *, title: str, created_by: int | None, @@ -44,35 +44,37 @@ def create_collection( """ Create a new Collection """ - collection = Collection.objects.create( + collection = Collection( learning_package_id=learning_package_id, - key=key, + collection_code=collection_code, title=title, created_by_id=created_by, description=description, enabled=enabled, ) + collection.full_clean() + collection.save() return collection -def get_collection(learning_package_id: int, collection_key: str) -> Collection: +def get_collection(learning_package_id: int, collection_code: str) -> Collection: """ Get a Collection by ID """ - return Collection.objects.get_by_key(learning_package_id, collection_key) + return Collection.objects.get_by_code(learning_package_id, collection_code) def update_collection( learning_package_id: int, - key: str, + collection_code: str, *, title: str | None = None, description: str | None = None, ) -> Collection: """ - Update a Collection identified by the learning_package_id + key. + Update a Collection identified by the learning_package_id + collection_code. """ - collection = get_collection(learning_package_id, key) + collection = get_collection(learning_package_id, collection_code) # If no changes were requested, there's nothing to update, so just return # the Collection as-is @@ -90,17 +92,17 @@ def update_collection( def delete_collection( learning_package_id: int, - key: str, + collection_code: str, *, hard_delete=False, ) -> Collection: """ - Disables or deletes a collection identified by the given learning_package + key. + Disables or deletes a collection identified by the given learning_package + collection_code. By default (hard_delete=False), the collection is "soft deleted", i.e disabled. Soft-deleted collections can be re-enabled using restore_collection. """ - collection = get_collection(learning_package_id, key) + collection = get_collection(learning_package_id, collection_code) if hard_delete: collection.delete() @@ -112,12 +114,12 @@ def delete_collection( def restore_collection( learning_package_id: int, - key: str, + collection_code: str, ) -> Collection: """ Undo a "soft delete" by re-enabling a Collection. """ - collection = get_collection(learning_package_id, key) + collection = get_collection(learning_package_id, collection_code) collection.enabled = True collection.save() @@ -126,7 +128,7 @@ def restore_collection( def add_to_collection( learning_package_id: int, - key: str, + collection_code: str, entities_qset: QuerySet[PublishableEntity], created_by: int | None = None, ) -> Collection: @@ -146,10 +148,10 @@ def add_to_collection( if invalid_entity: raise ValidationError( f"Cannot add entity {invalid_entity.pk} in learning package {invalid_entity.learning_package_id} " - f"to collection {key} in learning package {learning_package_id}." + f"to collection {collection_code} in learning package {learning_package_id}." ) - collection = get_collection(learning_package_id, key) + collection = get_collection(learning_package_id, collection_code) collection.entities.add( *entities_qset.all(), through_defaults={"created_by_id": created_by}, @@ -162,7 +164,7 @@ def add_to_collection( def remove_from_collection( learning_package_id: int, - key: str, + collection_code: str, entities_qset: QuerySet[PublishableEntity], ) -> Collection: """ @@ -174,7 +176,7 @@ def remove_from_collection( Returns the updated Collection. """ - collection = get_collection(learning_package_id, key) + collection = get_collection(learning_package_id, collection_code) collection.entities.remove(*entities_qset.all()) collection.modified = datetime.now(tz=timezone.utc) @@ -183,20 +185,20 @@ 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: int, entity_ref: str) -> QuerySet[Collection]: """ Get all collections in the given learning package which contain this entity. Only enabled collections are returned. """ - entity = publishing_api.get_publishable_entity_by_key( + entity = publishing_api.get_publishable_entity_by_ref( learning_package_id, - key=entity_key, + entity_ref=entity_ref, ) 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: int, collection_code: str) -> QuerySet[PublishableEntity]: """ Returns a QuerySet of PublishableEntities in a Collection. @@ -204,7 +206,7 @@ def get_collection_entities(learning_package_id: int, collection_key: str) -> Qu """ return PublishableEntity.objects.filter( learning_package_id=learning_package_id, - collections__key=collection_key, + collections__collection_code=collection_code, ).order_by("pk") diff --git a/src/openedx_content/applets/collections/models.py b/src/openedx_content/applets/collections/models.py index 2f315b247..3258ccc9c 100644 --- a/src/openedx_content/applets/collections/models.py +++ b/src/openedx_content/applets/collections/models.py @@ -70,7 +70,7 @@ from django.db import models from django.utils.translation import gettext_lazy as _ -from openedx_django_lib.fields import MultiCollationTextField, case_insensitive_char_field, key_field +from openedx_django_lib.fields import MultiCollationTextField, case_insensitive_char_field, code_field from openedx_django_lib.validators import validate_utc_datetime from ..publishing.models import LearningPackage, PublishableEntity @@ -85,12 +85,12 @@ class CollectionManager(models.Manager): """ Custom manager for Collection class. """ - def get_by_key(self, learning_package_id: int, key: str): + def get_by_code(self, learning_package_id: int, collection_code: str): """ - Get the Collection for the given Learning Package + key. + Get the Collection for the given Learning Package + collection code. """ return self.select_related('learning_package') \ - .get(learning_package_id=learning_package_id, key=key) + .get(learning_package_id=learning_package_id, collection_code=collection_code) class Collection(models.Model): @@ -105,10 +105,11 @@ class Collection(models.Model): learning_package = models.ForeignKey(LearningPackage, on_delete=models.CASCADE) # Every collection is uniquely and permanently identified within its learning package - # by a 'key' that is set during creation. Both will appear in the + # by a 'code' that is set during creation. Both will appear in the # collection's opaque key: - # e.g. "lib-collection:lib:key" is the opaque key for a library collection. - key = key_field(db_column='_key') + # e.g. "lib-collection:{org_code}:{library_code}:{collection_code}" + # is the opaque key for a library collection. + collection_code = code_field() title = case_insensitive_char_field( null=False, @@ -170,11 +171,11 @@ class Collection(models.Model): class Meta: verbose_name_plural = "Collections" constraints = [ - # Keys are unique within a given LearningPackage. + # Collection codes are unique within a given LearningPackage. models.UniqueConstraint( fields=[ "learning_package", - "key", + "collection_code", ], name="oel_coll_uniq_lp_key", ), @@ -196,7 +197,7 @@ def __str__(self) -> str: """ User-facing string representation of a Collection. """ - return f"<{self.__class__.__name__}> (lp:{self.learning_package_id} {self.key}:{self.title})" + return f"<{self.__class__.__name__}> (lp:{self.learning_package_id} {self.collection_code}:{self.title})" class CollectionPublishableEntity(models.Model): diff --git a/src/openedx_content/applets/components/admin.py b/src/openedx_content/applets/components/admin.py index 3899fa050..80ea61bb6 100644 --- a/src/openedx_content/applets/components/admin.py +++ b/src/openedx_content/applets/components/admin.py @@ -37,16 +37,16 @@ class ComponentAdmin(ReadOnlyModelAdmin): """ Django admin configuration for Component """ - list_display = ("key", "uuid", "component_type", "created") + list_display = ("entity_ref", "uuid", "component_type", "created") readonly_fields = [ "learning_package", "uuid", "component_type", - "key", + "entity_ref", "created", ] list_filter = ("component_type", "learning_package") - search_fields = ["publishable_entity__uuid", "publishable_entity__key"] + search_fields = ["publishable_entity__uuid", "publishable_entity__entity_ref"] inlines = [ComponentVersionInline] @@ -69,13 +69,13 @@ def get_queryset(self, request): ) fields = [ - "key", + "path", "format_size", "rendered_data", ] readonly_fields = [ "media", - "key", + "path", "format_size", "rendered_data", ] diff --git a/src/openedx_content/applets/components/api.py b/src/openedx_content/applets/components/api.py index 46de5356e..be9c9165a 100644 --- a/src/openedx_content/applets/components/api.py +++ b/src/openedx_content/applets/components/api.py @@ -34,16 +34,15 @@ # to be callable only by other apps in the authoring package. __all__ = [ "get_or_create_component_type", - "get_or_create_component_type_by_entity_key", "create_component", "create_component_version", "create_next_component_version", "create_component_and_version", "get_component", - "get_component_by_key", + "get_component_by_code", "get_component_by_uuid", "get_component_version_by_uuid", - "component_exists_by_key", + "component_exists_by_code", "get_collection_components", "get_components", "create_component_version_media", @@ -74,45 +73,29 @@ def get_or_create_component_type(namespace: str, name: str) -> ComponentType: return component_type -def get_or_create_component_type_by_entity_key(entity_key: str) -> tuple[ComponentType, str]: - """ - Get or create a ComponentType based on a full entity key string. - - The entity key is expected to be in the format - ``"{namespace}:{type_name}:{local_key}"``. This function will parse out the - ``namespace`` and ``type_name`` parts and use those to get or create the - ComponentType. - - Raises ValueError if the entity_key is not in the expected format. - """ - try: - namespace, type_name, local_key = entity_key.split(':', 2) - except ValueError as exc: - raise ValueError( - f"Invalid entity_key format: {entity_key!r}. " - "Expected format: '{namespace}:{type_name}:{local_key}'" - ) from exc - return get_or_create_component_type(namespace, type_name), local_key - - def create_component( learning_package_id: int, /, component_type: ComponentType, - local_key: str, + component_code: str, created: datetime, created_by: int | None, *, can_stand_alone: bool = True, + entity_ref: str | None = None, ) -> Component: """ - Create a new Component (an entity like a Problem or Video) + Create a new Component (an entity like a Problem or Video). + + If ``entity_ref`` is not provided, it defaults to + ``"{namespace}:{type_name}:{component_code}"``. """ - key = f"{component_type.namespace}:{component_type.name}:{local_key}" + if entity_ref is None: + entity_ref = f"{component_type.namespace}:{component_type.name}:{component_code}" with atomic(): publishable_entity = publishing_api.create_publishable_entity( learning_package_id, - key, + entity_ref, created, created_by, can_stand_alone=can_stand_alone @@ -121,7 +104,7 @@ def create_component( publishable_entity=publishable_entity, learning_package_id=learning_package_id, component_type=component_type, - local_key=local_key, + component_code=component_code, ) return component @@ -268,7 +251,7 @@ def create_next_component_version( ComponentVersionMedia.objects.create( media_id=media_pk, component_version=component_version, - key=key, + path=key, ) if ignore_previous_media: @@ -279,11 +262,11 @@ def create_next_component_version( last_version_media_mapping = ComponentVersionMedia.objects \ .filter(component_version=last_version) for cvrc in last_version_media_mapping: - if cvrc.key not in media_to_replace: + if cvrc.path not in media_to_replace: ComponentVersionMedia.objects.create( media_id=cvrc.media_id, component_version=component_version, - key=cvrc.key, + path=cvrc.path, ) return component_version @@ -293,24 +276,29 @@ def create_component_and_version( # pylint: disable=too-many-positional-argumen learning_package_id: int, /, component_type: ComponentType, - local_key: str, + component_code: str, title: str, created: datetime, created_by: int | None = None, *, can_stand_alone: bool = True, + entity_ref: str | None = None, ) -> tuple[Component, ComponentVersion]: """ - Create a Component and associated ComponentVersion atomically + Create a Component and associated ComponentVersion atomically. + + If ``entity_ref`` is not provided, it defaults to + ``"{namespace}:{type_name}:{component_code}"``. """ with atomic(): component = create_component( learning_package_id, component_type, - local_key, + component_code, created, created_by, can_stand_alone=can_stand_alone, + entity_ref=entity_ref, ) component_version = create_component_version( component.pk, @@ -331,22 +319,22 @@ def get_component(component_pk: int, /) -> Component: return Component.with_publishing_relations.get(pk=component_pk) -def get_component_by_key( +def get_component_by_code( learning_package_id: int, /, namespace: str, type_name: str, - local_key: str, + component_code: str, ) -> Component: """ - Get a Component by its unique (namespace, type, local_key) tuple. + Get a Component by its unique (namespace, type, component_code) tuple. """ return Component.with_publishing_relations \ .get( learning_package_id=learning_package_id, component_type__namespace=namespace, component_type__name=type_name, - local_key=local_key, + component_code=component_code, ) @@ -366,12 +354,12 @@ def get_component_version_by_uuid(uuid: UUID) -> ComponentVersion: ) -def component_exists_by_key( +def component_exists_by_code( learning_package_id: int, /, namespace: str, type_name: str, - local_key: str + component_code: str ) -> bool: """ Return True/False for whether a Component exists. @@ -384,7 +372,7 @@ def component_exists_by_key( learning_package_id=learning_package_id, component_type__namespace=namespace, component_type__name=type_name, - local_key=local_key, + component_code=component_code, ) return True except Component.DoesNotExist: @@ -423,12 +411,12 @@ def get_components( # pylint: disable=too-many-positional-arguments if draft_title is not None: qset = qset.filter( Q(publishable_entity__draft__version__title__icontains=draft_title) | - Q(local_key__icontains=draft_title) + Q(component_code__icontains=draft_title) ) if published_title is not None: qset = qset.filter( Q(publishable_entity__published__version__title__icontains=published_title) | - Q(local_key__icontains=published_title) + Q(component_code__icontains=published_title) ) return qset @@ -436,7 +424,7 @@ def get_components( # pylint: disable=too-many-positional-arguments def get_collection_components( learning_package_id: int, - collection_key: str, + collection_code: str, ) -> QuerySet[Component]: """ Returns a QuerySet of Components relating to the PublishableEntities in a Collection. @@ -445,18 +433,18 @@ def get_collection_components( """ return Component.objects.filter( learning_package_id=learning_package_id, - publishable_entity__collections__key=collection_key, + publishable_entity__collections__collection_code=collection_code, ).order_by('pk') def look_up_component_version_media( - learning_package_key: str, - component_key: str, + learning_package_ref: str, + entity_ref: str, version_num: int, - key: Path, + path: Path, ) -> ComponentVersionMedia: """ - Look up ComponentVersionMedia by human readable keys. + Look up ComponentVersionMedia by human readable identifiers. Can raise a django.core.exceptions.ObjectDoesNotExist error if there is no matching ComponentVersionMedia. @@ -465,10 +453,10 @@ def look_up_component_version_media( I don't know if we wantto make it a part of the public interface. """ queries = ( - Q(component_version__component__learning_package__key=learning_package_key) - & Q(component_version__component__publishable_entity__key=component_key) + Q(component_version__component__learning_package__package_ref=learning_package_ref) + & Q(component_version__component__publishable_entity__entity_ref=entity_ref) & Q(component_version__publishable_entity_version__version_num=version_num) - & Q(key=key) + & Q(path=path) ) return ComponentVersionMedia.objects \ .select_related( @@ -484,29 +472,29 @@ def create_component_version_media( component_version_id: int, media_id: int, /, - key: str, + path: str, ) -> ComponentVersionMedia: """ Add a Media to the given ComponentVersion - We don't allow keys that would be absolute paths, e.g. ones that start with + We don't allow paths that would be absolute, e.g. ones that start with '/'. Storing these causes headaches with building relative paths and because of mismatches with things that expect a leading slash and those that don't. So for safety and consistency, we strip off leading slashes and emit a warning when we do. """ - if key.startswith('/'): + if path.startswith('/'): logger.warning( "Absolute paths are not supported: " f"removed leading '/' from ComponentVersion {component_version_id} " - f"media key: {repr(key)} (media_id: {media_id})" + f"media path: {repr(path)} (media_id: {media_id})" ) - key = key.lstrip('/') + path = path.lstrip('/') cvrc, _created = ComponentVersionMedia.objects.get_or_create( component_version_id=component_version_id, media_id=media_id, - key=key, + path=path, ) return cvrc @@ -529,13 +517,13 @@ def _get_component_version_info_headers(component_version: ComponentVersion) -> learning_package = component.learning_package return { # Component - "X-Open-edX-Component-Key": component.publishable_entity.key, + "X-Open-edX-Component-Key": component.publishable_entity.entity_ref, "X-Open-edX-Component-Uuid": component.uuid, # Component Version "X-Open-edX-Component-Version-Uuid": component_version.uuid, "X-Open-edX-Component-Version-Num": str(component_version.version_num), # Learning Package - "X-Open-edX-Learning-Package-Key": learning_package.key, + "X-Open-edX-Learning-Package-Key": learning_package.package_ref, "X-Open-edX-Learning-Package-Uuid": learning_package.uuid, } @@ -621,7 +609,7 @@ def _error_header(error: AssetError) -> dict[str, str]: # Check: Does the ComponentVersion have the requested asset (Media)? try: - cv_media = component_version.componentversionmedia_set.get(key=asset_path) + cv_media = component_version.componentversionmedia_set.get(path=asset_path) except ComponentVersionMedia.DoesNotExist: logger.error(f"ComponentVersion {component_version_uuid} has no asset {asset_path}") info_headers.update( diff --git a/src/openedx_content/applets/components/models.py b/src/openedx_content/applets/components/models.py index de93680fd..10a57685b 100644 --- a/src/openedx_content/applets/components/models.py +++ b/src/openedx_content/applets/components/models.py @@ -1,5 +1,5 @@ """ -The model hierarchy is Component -> ComponentVersion -> Media. +The model hierarchy is Component -> ComponentVersion -> Content. A Component is an entity like a Problem or Video. It has enough information to identify the Component and determine what the handler should be (e.g. XBlock @@ -10,7 +10,7 @@ publishing app. Component maps 1:1 to PublishableEntity and ComponentVersion maps 1:1 to PublishableEntityVersion. -Multiple pieces of Media may be associated with a ComponentVersion, through +Multiple pieces of Content may be associated with a ComponentVersion, through the ComponentVersionMedia model. ComponentVersionMedia allows to specify a ComponentVersion-local identifier. We're using this like a file path by convention, but it's possible we might want to have special identifiers later. @@ -21,7 +21,7 @@ from django.db import models -from openedx_django_lib.fields import case_sensitive_char_field, key_field +from openedx_django_lib.fields import case_sensitive_char_field, code_field, ref_field from openedx_django_lib.managers import WithRelationsManager from ..media.models import Media @@ -91,7 +91,7 @@ class Component(PublishableEntityMixin): Problem), but little beyond that. A Component will have many ComponentVersions over time, and most metadata is - associated with the ComponentVersion model and the Media that + associated with the ComponentVersion model and the Content that ComponentVersions are associated with. A Component belongs to exactly one LearningPackage. @@ -113,9 +113,10 @@ class Component(PublishableEntityMixin): State Consistency ----------------- - The ``key`` field on Component's ``publishable_entity`` is dervied from the - ``component_type`` and ``local_key`` fields in this model. We don't support - changing the keys yet, but if we do, those values need to be kept in sync. + The ``key`` field on Component's ``publishable_entity`` is derived from the + ``component_type`` and ``component_code`` fields in this model. We don't + support changing the keys yet, but if we do, those values need to be kept + in sync. How build on this model ----------------------- @@ -151,37 +152,37 @@ class Component(PublishableEntityMixin): # XBlock block_type, but we want it to be more flexible in the long term. component_type = models.ForeignKey(ComponentType, on_delete=models.PROTECT) - # local_key is an identifier that is local to the learning_package and - # component_type. The publishable.key should be calculated as a - # combination of component_type and local_key. - local_key = key_field() + # component_code is an identifier that is local to the learning_package and + # component_type. The publishable.entity_ref is derived from component_type + # and component_code. + component_code = code_field() class Meta: constraints = [ - # The combination of (component_type, local_key) is unique within - # a given LearningPackage. Note that this means it is possible to - # have two Components in the same LearningPackage to have the same - # local_key if the component_types are different. So for example, - # you could have a ProblemBlock and VideoBlock that both have the - # local_key "week_1". + # The combination of (component_type, component_code) is unique + # within a given LearningPackage. Note that this means it is + # possible to have two Components in the same LearningPackage with + # the same component_code if their component_types differ. For + # example, a ProblemBlock and VideoBlock could both have the + # component_code "week_1". models.UniqueConstraint( fields=[ "learning_package", "component_type", - "local_key", + "component_code", ], name="oel_component_uniq_lc_ct_lk", ), ] indexes = [ - # Global Component-Type/Local-Key Index: + # Global Component-Type/Component-Code Index: # * Search by the different Components fields across all Learning # Packages on the site. This would be a support-oriented tool # from Django Admin. models.Index( fields=[ "component_type", - "local_key", + "component_code", ], name="oel_component_idx_ct_lk", ), @@ -192,14 +193,14 @@ class Meta: verbose_name_plural = "Components" def __str__(self) -> str: - return f"{self.component_type.namespace}:{self.component_type.name}:{self.local_key}" + return f"{self.component_type.namespace}:{self.component_type.name}:{self.component_code}" class ComponentVersion(PublishableEntityVersionMixin): """ A particular version of a Component. - This holds the media using a M:M relationship with Media via + This holds the media using a M:M relationship with Content via ComponentVersionMedia. """ @@ -225,37 +226,31 @@ class Meta: class ComponentVersionMedia(models.Model): """ - Determines the Media for a given ComponentVersion. + Determines the Content for a given ComponentVersion. An ComponentVersion may be associated with multiple pieces of binary data. For instance, a Video ComponentVersion might be associated with multiple transcripts in different languages. - When Media is associated with an ComponentVersion, it has some local - key that is unique within the the context of that ComponentVersion. This - allows the ComponentVersion to do things like store an image file and - reference it by a "path" key. + When Content is associated with a ComponentVersion, it has a ``path`` + that is unique within the context of that ComponentVersion. This is + used as a local file-path-like identifier, e.g. "static/image.png". - Media is immutable and sharable across multiple ComponentVersions. + Content is immutable and sharable across multiple ComponentVersions. """ component_version = models.ForeignKey(ComponentVersion, on_delete=models.CASCADE) media = models.ForeignKey(Media, on_delete=models.RESTRICT) - # "key" is a reserved word for MySQL, so we're temporarily using the column - # name of "_key" to avoid breaking downstream tooling. A possible - # alternative name for this would be "path", since it's most often used as - # an internal file path. However, we might also want to put special - # identifiers that don't map as cleanly to file paths at some point. - key = key_field(db_column="_key") + # path is a local file-path-like identifier for the media within a + # ComponentVersion. + path = ref_field() class Meta: constraints = [ - # Uniqueness is only by ComponentVersion and key. If for some reason - # a ComponentVersion wants to associate the same piece of Media - # with two different identifiers, that is permitted. + # Uniqueness is only by ComponentVersion and path. models.UniqueConstraint( - fields=["component_version", "key"], + fields=["component_version", "path"], name="oel_cvcontent_uniq_cv_key", ), ] diff --git a/src/openedx_content/applets/components/readme.rst b/src/openedx_content/applets/components/readme.rst index eb97d22f2..b79134bc6 100644 --- a/src/openedx_content/applets/components/readme.rst +++ b/src/openedx_content/applets/components/readme.rst @@ -1,7 +1,7 @@ Components App ============== -The ``components`` app holds the versioned data models for the lowest-level pieces of content that can be stored in Open edX: Components (e.g. XBlocks), as well as the individual pieces of raw data Media that they reference. +The ``components`` app holds the versioned data models for the lowest-level pieces of content that can be stored in Open edX: Components (e.g. XBlocks), as well as the individual pieces of raw data content that they reference. Motivation ---------- @@ -18,6 +18,6 @@ Architecture Guidelines * We're keeping nearly unlimited history, so per-version metadata (i.e. the space/time cost of making a new version) must be kept low. * Do not assume that all Components will be XBlocks. -* Encourage other apps to make models that join to (and add their own metadata to) Component, ComponentVersion, Media, etc. But it should be done in such a way that this app is not aware of them. +* Encourage other apps to make models that join to (and add their own metadata to) Component, ComponentVersion, Content, etc. But it should be done in such a way that this app is not aware of them. * Always preserve the most raw version of the data possible, e.g. OLX, even if XBlocks then extend that with more sophisticated data models. At some point those XBlocks will get deprecated/removed, and we will still want to be able to export the raw data. * Exports should be fast and *not* require the invocation of plugin code. \ No newline at end of file diff --git a/src/openedx_content/applets/containers/admin.py b/src/openedx_content/applets/containers/admin.py index df2a60ecd..ae86e83c7 100644 --- a/src/openedx_content/applets/containers/admin.py +++ b/src/openedx_content/applets/containers/admin.py @@ -89,11 +89,13 @@ class ContainerAdmin(ReadOnlyModelAdmin): Django admin configuration for Container """ - list_display = ("key", "container_type_display", "published", "draft", "created") + list_display = ("container_code", "container_type_display", "published", "draft", "created") fields = [ "pk", "publishable_entity", "learning_package", + "container_code", + "container_type_display", "published", "draft", "created", @@ -101,14 +103,15 @@ class ContainerAdmin(ReadOnlyModelAdmin): "see_also", "most_recent_parent_entity_list", ] + # container_code is a model field; container_type_display is a method readonly_fields = fields # type: ignore[assignment] - search_fields = ["publishable_entity__uuid", "publishable_entity__key"] + search_fields = ["publishable_entity__uuid", "publishable_entity__entity_ref", "container_code"] inlines = [ContainerVersionInlineForContainer] def learning_package(self, obj: Container) -> SafeText: return model_detail_link( obj.publishable_entity.learning_package, - obj.publishable_entity.learning_package.key, + obj.publishable_entity.learning_package.package_ref, ) def get_queryset(self, request): @@ -184,7 +187,7 @@ class ContainerVersionInlineForEntityList(admin.TabularInline): fields = [ "pk", "version_num", - "container_key", + "container_code", "title", "created", "created_by", @@ -203,8 +206,8 @@ def get_queryset(self, request): ) ) - def container_key(self, obj: ContainerVersion) -> SafeText: - return model_detail_link(obj.container, obj.container.key) + def container_code(self, obj: ContainerVersion) -> SafeText: + return model_detail_link(obj.container, obj.container.container_code) class EntityListRowInline(admin.TabularInline): @@ -238,7 +241,7 @@ def pinned_version_num(self, obj: EntityListRow): def entity_models(self, obj: EntityListRow): return format_html( "{}", - model_detail_link(obj.entity, obj.entity.key), + model_detail_link(obj.entity, obj.entity.entity_ref), one_to_one_related_model_html(obj.entity), ) @@ -301,7 +304,7 @@ def recent_container(self, obj: EntityList) -> SafeText | None: Link to the Container of the newest ContainerVersion that references this EntityList """ if latest := _latest_container_version(obj): - return format_html("of: {}", model_detail_link(latest.container, latest.container.key)) + return format_html("of: {}", model_detail_link(latest.container, latest.container.entity_ref)) else: return None @@ -314,7 +317,7 @@ def recent_container_package(self, obj: EntityList) -> SafeText | None: "in: {}", model_detail_link( latest.container.publishable_entity.learning_package, - latest.container.publishable_entity.learning_package.key, + latest.container.publishable_entity.learning_package.package_ref, ), ) else: diff --git a/src/openedx_content/applets/containers/api.py b/src/openedx_content/applets/containers/api.py index 002b39926..5ac1e2fc5 100644 --- a/src/openedx_content/applets/containers/api.py +++ b/src/openedx_content/applets/containers/api.py @@ -55,7 +55,7 @@ "create_next_container_version", "get_container", "get_container_version", - "get_container_by_key", + "get_container_by_ref", "get_all_container_subclasses", "get_container_subclass", "get_container_type_code_of", @@ -68,7 +68,7 @@ "contains_unpublished_changes", "get_containers_with_entity", "get_container_children_count", - "get_container_children_entities_keys", + "get_container_children_entity_refs", ] @@ -136,12 +136,13 @@ def parse(entities: EntityListInput) -> list[ParsedEntityReference]: def create_container( learning_package_id: int, - key: str, created: datetime, created_by: int | None, *, + container_code: str, container_cls: type[ContainerModel], can_stand_alone: bool = True, + entity_ref: str | None = None, ) -> ContainerModel: """ [ 🛑 UNSTABLE ] @@ -149,28 +150,36 @@ def create_container( Args: learning_package_id: The ID of the learning package that contains the container. - key: The key of the container. created: The date and time the container was created. created_by: The ID of the user who created the container + container_code: A local slug identifier for the container, unique within + the learning package (regardless of container type). container_cls: The subclass of container to create (e.g. `Unit`) can_stand_alone: Set to False when created as part of containers + entity_ref: Optional opaque reference string. Defaults to container_code. + # TODO: The dev team is considering revisiting the default container + # entity_ref derivation in the future. Returns: The newly created container as an instance of `container_cls`. """ assert issubclass(container_cls, Container) assert container_cls is not Container, "Creating plain containers is not allowed; use a subclass of Container" + if entity_ref is None: + entity_ref = container_code # TODO: The team may revisit this default derivation. with atomic(): publishable_entity = publishing_api.create_publishable_entity( learning_package_id, - key, + entity_ref, created, created_by, can_stand_alone=can_stand_alone, ) container = container_cls.objects.create( publishable_entity=publishable_entity, + learning_package_id=learning_package_id, container_type=container_cls.get_container_type(), + container_code=container_code, ) return container @@ -339,21 +348,23 @@ def create_container_version( def create_container_and_version( learning_package_id: int, - key: str, *, + container_code: str, title: str, container_cls: type[ContainerModel], entities: EntityListInput | None = None, created: datetime, created_by: int | None = None, can_stand_alone: bool = True, + entity_ref: str | None = None, ) -> tuple[ContainerModel, ContainerVersionModel]: """ [ 🛑 UNSTABLE ] Create a new container and its initial version. Args: learning_package_id: The learning package ID. - key: The key. + container_code: A local slug identifier for the container, unique within + the learning package (regardless of container type). title: The title of the new container. container_cls: The subclass of container to create (e.g. Unit) entities: List of the entities that will comprise the entity list, in @@ -364,15 +375,17 @@ def create_container_and_version( created: The creation date. created_by: The ID of the user who created the container. can_stand_alone: Set to False when created as part of containers + entity_ref: Optional opaque reference string. Defaults to container_code. """ with atomic(savepoint=False): container = create_container( learning_package_id, - key, created, created_by, + container_code=container_code, can_stand_alone=can_stand_alone, container_cls=container_cls, + entity_ref=entity_ref, ) container_version: ContainerVersionModel = create_container_version( # type: ignore[assignment] container.pk, @@ -564,22 +577,22 @@ 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_ref(learning_package_id: int, /, entity_ref: str) -> Container: """ [ 🛑 UNSTABLE ] - Get a container by its learning package and primary key. + Get a container by its learning package and entity ref. Args: learning_package_id: The ID of the learning package that contains the container. - key: The primary key of the container. + entity_ref: The entity ref of the container. Returns: - The container with the given primary key (as `Container`, not as its typed subclass). + The container with the given entity ref (as `Container`, not as its typed subclass). """ try: return Container.objects.select_related("container_type").get( publishable_entity__learning_package_id=learning_package_id, - publishable_entity__key=key, + publishable_entity__entity_ref=entity_ref, ) except Container.DoesNotExist: # Check if it's the container or the learning package that does not exist: @@ -867,15 +880,17 @@ def get_container_children_count( return container_version.entity_list.entitylistrow_set.filter(**filter_deleted).count() -def get_container_children_entities_keys(container_version: ContainerVersion) -> list[str]: +def get_container_children_entity_refs(container_version: ContainerVersion) -> list[str]: """ - Fetch the list of entity keys for all entities in the given container version. + Fetch the list of entity refs for all entities in the given container version. Args: - container_version: The ContainerVersion to fetch the entity keys for. + container_version: The ContainerVersion to fetch the entity refs for. Returns: - A list of entity keys for all entities in the container version, ordered by entity key. + A list of entity refs for all entities in the container version, ordered by position. """ return list( - container_version.entity_list.entitylistrow_set.values_list("entity__key", flat=True).order_by("order_num") + container_version.entity_list.entitylistrow_set + .values_list("entity__entity_ref", flat=True) + .order_by("order_num") ) diff --git a/src/openedx_content/applets/containers/models.py b/src/openedx_content/applets/containers/models.py index 3661f1f61..52a00b5a1 100644 --- a/src/openedx_content/applets/containers/models.py +++ b/src/openedx_content/applets/containers/models.py @@ -10,8 +10,9 @@ from django.core.exceptions import ValidationError from django.db import models -from openedx_django_lib.fields import case_sensitive_char_field +from openedx_django_lib.fields import case_sensitive_char_field, code_field +from ..publishing.models.learning_package import LearningPackage from ..publishing.models.publishable_entity import ( PublishableEntity, PublishableEntityMixin, @@ -167,6 +168,12 @@ class Container(PublishableEntityMixin): olx_tag_name: str = "" _type_instance: ContainerType # Cache used by get_container_type() + # This foreign key is technically redundant because we're already locked to + # a single LearningPackage through our publishable_entity relation. However, + # having this foreign key directly allows us to make indexes that efficiently + # query by other Container fields within a given LearningPackage. + learning_package = models.ForeignKey(LearningPackage, on_delete=models.CASCADE) + # The type of the container. Cannot be changed once the container is created. container_type = models.ForeignKey( ContainerType, @@ -175,6 +182,19 @@ class Container(PublishableEntityMixin): editable=False, ) + # container_code is an identifier that is local to the learning_package. + # Unlike component_code, it is unique across all container types within + # the same LearningPackage. + container_code = code_field() + + class Meta: + constraints = [ + models.UniqueConstraint( + fields=["learning_package", "container_code"], + name="oel_container_uniq_lp_cc", + ), + ] + @classmethod def validate_entity(cls, entity: PublishableEntity) -> None: """ diff --git a/src/openedx_content/applets/publishing/admin.py b/src/openedx_content/applets/publishing/admin.py index 64b0035fc..d5796d86f 100644 --- a/src/openedx_content/applets/publishing/admin.py +++ b/src/openedx_content/applets/publishing/admin.py @@ -26,10 +26,10 @@ class LearningPackageAdmin(ReadOnlyModelAdmin): """ Read-only admin for LearningPackage model """ - fields = ["key", "title", "uuid", "created", "updated"] - readonly_fields = ["key", "title", "uuid", "created", "updated"] - list_display = ["key", "title", "uuid", "created", "updated"] - search_fields = ["key", "title", "uuid"] + fields = ["package_ref", "title", "uuid", "created", "updated"] + readonly_fields = ["package_ref", "title", "uuid", "created", "updated"] + list_display = ["package_ref", "title", "uuid", "created", "updated"] + search_fields = ["package_ref", "title", "uuid"] class PublishLogRecordTabularInline(admin.TabularInline): @@ -101,7 +101,7 @@ class PublishableEntityVersionTabularInline(admin.TabularInline): def dependencies_list(self, version: PublishableEntityVersion): identifiers = sorted( - [str(dep.key) for dep in version.dependencies.all()] + [str(dep.entity_ref) for dep in version.dependencies.all()] ) return "\n".join(identifiers) @@ -151,7 +151,7 @@ class PublishableEntityAdmin(ReadOnlyModelAdmin): inlines = [PublishableEntityVersionTabularInline] list_display = [ - "key", + "entity_ref", "published_version", "draft_version", "uuid", @@ -161,10 +161,10 @@ class PublishableEntityAdmin(ReadOnlyModelAdmin): "can_stand_alone", ] list_filter = ["learning_package", PublishStatusFilter] - search_fields = ["key", "uuid"] + search_fields = ["entity_ref", "uuid"] fields = [ - "key", + "entity_ref", "published_version", "draft_version", "uuid", @@ -175,7 +175,7 @@ class PublishableEntityAdmin(ReadOnlyModelAdmin): "can_stand_alone", ] readonly_fields = [ - "key", + "entity_ref", "published_version", "draft_version", "uuid", @@ -294,7 +294,7 @@ class DraftChangeLogRecordTabularInline(admin.TabularInline): def get_queryset(self, request): queryset = super().get_queryset(request) return queryset.select_related("entity", "old_version", "new_version") \ - .order_by("entity__key") + .order_by("entity__entity_ref") def old_version_num(self, draft_change: DraftChangeLogRecord): if draft_change.old_version is None: diff --git a/src/openedx_content/applets/publishing/api.py b/src/openedx_content/applets/publishing/api.py index 88143f520..613f2fc60 100644 --- a/src/openedx_content/applets/publishing/api.py +++ b/src/openedx_content/applets/publishing/api.py @@ -43,14 +43,14 @@ # to be callable only by other apps in the authoring package. __all__ = [ "get_learning_package", - "get_learning_package_by_key", + "get_learning_package_by_ref", "create_learning_package", "update_learning_package", "learning_package_exists", "create_publishable_entity", "create_publishable_entity_version", "get_publishable_entity", - "get_publishable_entity_by_key", + "get_publishable_entity_by_ref", "get_publishable_entities", "get_last_publish", "get_all_drafts", @@ -76,22 +76,22 @@ def get_learning_package(learning_package_id: int, /) -> LearningPackage: return LearningPackage.objects.get(id=learning_package_id) -def get_learning_package_by_key(key: str) -> LearningPackage: +def get_learning_package_by_ref(package_ref: str) -> LearningPackage: """ - Get LearningPackage by key. + Get LearningPackage by its package_ref. Can throw a NotFoundError """ - return LearningPackage.objects.get(key=key) + return LearningPackage.objects.get(package_ref=package_ref) def create_learning_package( - key: str, title: str, description: str = "", created: datetime | None = None + package_ref: str, title: str, description: str = "", created: datetime | None = None ) -> LearningPackage: """ Create a new LearningPackage. - The ``key`` must be unique. + The ``package_ref`` must be unique. Errors that can be raised: @@ -101,7 +101,7 @@ def create_learning_package( created = datetime.now(tz=timezone.utc) package = LearningPackage( - key=key, + package_ref=package_ref, title=title, description=description, created=created, @@ -116,7 +116,7 @@ def create_learning_package( def update_learning_package( learning_package_id: int, /, - key: str | None = None, + package_ref: str | None = None, title: str | None = None, description: str | None = None, updated: datetime | None = None, @@ -130,11 +130,11 @@ def update_learning_package( # If no changes were requested, there's nothing to update, so just return # the LearningPackage as-is. - if all(field is None for field in [key, title, description, updated]): + if all(field is None for field in [package_ref, title, description, updated]): return lp - if key is not None: - lp.key = key + if package_ref is not None: + lp.package_ref = package_ref if title is not None: lp.title = title if description is not None: @@ -150,17 +150,17 @@ def update_learning_package( return lp -def learning_package_exists(key: str) -> bool: +def learning_package_exists(package_ref: str) -> bool: """ - Check whether a LearningPackage with a particular key exists. + Check whether a LearningPackage with a particular package_ref exists. """ - return LearningPackage.objects.filter(key=key).exists() + return LearningPackage.objects.filter(package_ref=package_ref).exists() def create_publishable_entity( learning_package_id: int, /, - key: str, + entity_ref: str, created: datetime, # User ID who created this created_by: int | None, @@ -175,7 +175,7 @@ def create_publishable_entity( """ return PublishableEntity.objects.create( learning_package_id=learning_package_id, - key=key, + entity_ref=entity_ref, created=created, created_by_id=created_by, can_stand_alone=can_stand_alone, @@ -286,10 +286,10 @@ def get_publishable_entity(publishable_entity_id: int, /) -> PublishableEntity: return PublishableEntity.objects.get(id=publishable_entity_id) -def get_publishable_entity_by_key(learning_package_id, /, key) -> PublishableEntity: +def get_publishable_entity_by_ref(learning_package_id, /, entity_ref) -> PublishableEntity: return PublishableEntity.objects.get( learning_package_id=learning_package_id, - key=key, + entity_ref=entity_ref, ) diff --git a/src/openedx_content/applets/publishing/models/learning_package.py b/src/openedx_content/applets/publishing/models/learning_package.py index 9e6d6a776..c2b565a5e 100644 --- a/src/openedx_content/applets/publishing/models/learning_package.py +++ b/src/openedx_content/applets/publishing/models/learning_package.py @@ -7,8 +7,8 @@ MultiCollationTextField, case_insensitive_char_field, immutable_uuid_field, - key_field, manual_date_time_field, + ref_field, ) @@ -27,19 +27,15 @@ class LearningPackage(models.Model): uuid = immutable_uuid_field() - # "key" is a reserved word for MySQL, so we're temporarily using the column - # name of "_key" to avoid breaking downstream tooling. There's an open - # question as to whether this field needs to exist at all, or whether the - # top level library key it's currently used for should be entirely in the - # LibraryContent model. - key = key_field(db_column="_key") + # package_ref is an opaque reference string for the LearningPackage. + package_ref = ref_field() title = case_insensitive_char_field(max_length=500, blank=False) # TODO: We should probably defer this field, since many things pull back # LearningPackage as select_related. Usually those relations only care about - # the UUID and key, so maybe it makes sense to separate the model at some - # point. + # the UUID and package_ref, so maybe it makes sense to separate the model at + # some point. description = MultiCollationTextField( blank=True, null=False, @@ -58,16 +54,13 @@ class LearningPackage(models.Model): updated = manual_date_time_field() def __str__(self): - return f"{self.key}" + return f"{self.package_ref}" class Meta: constraints = [ - # LearningPackage keys must be globally unique. This is something - # that might be relaxed in the future if this system were to be - # extensible to something like multi-tenancy, in which case we'd tie - # it to something like a Site or Org. + # package_refs must be globally unique. models.UniqueConstraint( - fields=["key"], + fields=["package_ref"], name="oel_publishing_lp_uniq_key", ) ] diff --git a/src/openedx_content/applets/publishing/models/publishable_entity.py b/src/openedx_content/applets/publishing/models/publishable_entity.py index 42e9a4a9c..683c39f84 100644 --- a/src/openedx_content/applets/publishing/models/publishable_entity.py +++ b/src/openedx_content/applets/publishing/models/publishable_entity.py @@ -16,8 +16,8 @@ from openedx_django_lib.fields import ( case_insensitive_char_field, immutable_uuid_field, - key_field, manual_date_time_field, + ref_field, ) from openedx_django_lib.managers import WithRelationsManager @@ -82,7 +82,7 @@ class PublishableEntity(models.Model): * Published things need to have the right identifiers so they can be used throughout the system, and the UUID is serving the role of ISBN in physical book publishing. - * We want to be able to enforce the idea that "key" is locally unique across + * We want to be able to enforce the idea that "entity_ref" is locally unique across all PublishableEntities within a given LearningPackage. Component and Unit can't do that without a shared model. @@ -117,10 +117,11 @@ class PublishableEntity(models.Model): related_name="publishable_entities", ) - # "key" is a reserved word for MySQL, so we're temporarily using the column - # name of "_key" to avoid breaking downstream tooling. Consider renaming - # this later. - key = key_field(db_column="_key") + # entity_ref is an opaque reference string assigned by the creator of this + # entity (e.g. derived from component_type + component_code for Components). + # Consumers must treat it as an atomic string — do not parse or reconstruct + # it. + entity_ref = ref_field() created = manual_date_time_field() created_by = models.ForeignKey( @@ -136,21 +137,19 @@ class PublishableEntity(models.Model): class Meta: constraints = [ - # Keys are unique within a given LearningPackage. + # entity_refs are unique within a given LearningPackage. models.UniqueConstraint( fields=[ "learning_package", - "key", + "entity_ref", ], name="oel_pub_ent_uniq_lp_key", ) ] indexes = [ - # Global Key Index: - # * Search by key across all PublishableEntities on the site. This - # would be a support-oriented tool from Django Admin. + # Global entity_ref index for support-oriented admin searches. models.Index( - fields=["key"], + fields=["entity_ref"], name="oel_pub_ent_idx_key", ), # LearningPackage (reverse) Created Index: @@ -167,7 +166,7 @@ class Meta: verbose_name_plural = "Publishable Entities" def __str__(self): - return f"{self.key}" + return f"{self.entity_ref}" class PublishableEntityVersion(models.Model): @@ -234,7 +233,7 @@ class PublishableEntityVersion(models.Model): ) def __str__(self): - return f"{self.entity.key} @ v{self.version_num} - {self.title}" + return f"{self.entity.entity_ref} @ v{self.version_num} - {self.title}" class Meta: constraints = [ @@ -347,8 +346,8 @@ def can_stand_alone(self) -> bool: return self.publishable_entity.can_stand_alone @property - def key(self) -> str: - return self.publishable_entity.key + def entity_ref(self) -> str: + return self.publishable_entity.entity_ref @property def created(self) -> datetime: @@ -395,7 +394,7 @@ class VersioningHelper: learning_package_id=learning_package.id, namespace="xblock.v1", type="problem", - local_key="monty_hall", + component_code="monty_hall", title="Monty Hall Problem", created=now, created_by=None, diff --git a/src/openedx_content/applets/sections/api.py b/src/openedx_content/applets/sections/api.py index ff9b613de..3e926dff9 100644 --- a/src/openedx_content/applets/sections/api.py +++ b/src/openedx_content/applets/sections/api.py @@ -30,13 +30,14 @@ def get_section(section_id: int, /): def create_section_and_version( learning_package_id: int, - key: str, *, + container_code: str, title: str, subsections: Iterable[Subsection | SubsectionVersion] | None = None, created: datetime, created_by: int | None = None, can_stand_alone: bool = True, + entity_ref: str | None = None, ) -> tuple[Section, SectionVersion]: """ See documentation of `content_api.create_container_and_version()` @@ -47,7 +48,8 @@ def create_section_and_version( """ section, sv = containers_api.create_container_and_version( learning_package_id, - key=key, + container_code=container_code, + entity_ref=entity_ref, title=title, entities=subsections, created=created, diff --git a/src/openedx_content/applets/subsections/api.py b/src/openedx_content/applets/subsections/api.py index 85f3b73ca..384124af6 100644 --- a/src/openedx_content/applets/subsections/api.py +++ b/src/openedx_content/applets/subsections/api.py @@ -30,13 +30,14 @@ def get_subsection(subsection_id: int, /): def create_subsection_and_version( learning_package_id: int, - key: str, *, + container_code: str, title: str, units: Iterable[Unit | UnitVersion] | None = None, created: datetime, created_by: int | None = None, can_stand_alone: bool = True, + entity_ref: str | None = None, ) -> tuple[Subsection, SubsectionVersion]: """ See documentation of `content_api.create_container_and_version()` @@ -47,7 +48,8 @@ def create_subsection_and_version( """ subsection, sv = containers_api.create_container_and_version( learning_package_id, - key=key, + container_code=container_code, + entity_ref=entity_ref, title=title, entities=units, created=created, diff --git a/src/openedx_content/applets/units/api.py b/src/openedx_content/applets/units/api.py index 4aad6dde8..fd75a2fe9 100644 --- a/src/openedx_content/applets/units/api.py +++ b/src/openedx_content/applets/units/api.py @@ -30,13 +30,14 @@ def get_unit(unit_id: int, /): def create_unit_and_version( learning_package_id: int, - key: str, *, + container_code: str, title: str, components: Iterable[Component | ComponentVersion] | None = None, created: datetime, created_by: int | None = None, can_stand_alone: bool = True, + entity_ref: str | None = None, ) -> tuple[Unit, UnitVersion]: """ See documentation of `content_api.create_container_and_version()` @@ -47,7 +48,8 @@ def create_unit_and_version( """ unit, uv = containers_api.create_container_and_version( learning_package_id, - key=key, + container_code=container_code, + entity_ref=entity_ref, title=title, entities=components, created=created, diff --git a/src/openedx_content/applets/units/models.py b/src/openedx_content/applets/units/models.py index 3594acb7b..acf5041fb 100644 --- a/src/openedx_content/applets/units/models.py +++ b/src/openedx_content/applets/units/models.py @@ -1,6 +1,7 @@ """ Models that implement units """ +from __future__ import annotations from typing import override 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..2b19be325 100644 --- a/src/openedx_content/management/commands/add_assets_to_component.py +++ b/src/openedx_content/management/commands/add_assets_to_component.py @@ -9,7 +9,7 @@ from django.core.management.base import BaseCommand -from ...api import create_next_component_version, get_component_by_key, get_learning_package_by_key +from ...api import create_next_component_version, get_component_by_code, get_learning_package_by_ref class Command(BaseCommand): @@ -26,14 +26,14 @@ class Command(BaseCommand): def add_arguments(self, parser): parser.add_argument( - "learning_package_key", + "learning_package_ref", type=str, - help="LearningPackage.key value for where the Component is located." + help="LearningPackage.package_ref value for where the Component is located." ) parser.add_argument( - "component_key", + "entity_ref", type=str, - help="Component.key that you want to add assets to." + help="Entity ref (e.g. 'xblock.v1:problem:my_component') of the Component to add assets to." ) parser.add_argument( "file_mappings", @@ -53,34 +53,34 @@ def handle(self, *args, **options): """ Add files to a Component as ComponentVersion -> Content associations. """ - learning_package_key = options["learning_package_key"] - component_key = options["component_key"] + learning_package_ref = options["learning_package_ref"] + entity_ref = options["entity_ref"] file_mappings = options["file_mappings"] - learning_package = get_learning_package_by_key(learning_package_key) + learning_package = get_learning_package_by_ref(learning_package_ref) # Parse something like: "xblock.v1:problem:area_of_circle_1" - namespace, type_name, local_key = component_key.split(":", 2) - component = get_component_by_key( - learning_package.id, namespace, type_name, local_key + namespace, type_name, component_code = entity_ref.split(":", 2) + component = get_component_by_code( + learning_package.id, namespace, type_name, component_code ) created = datetime.now(tz=timezone.utc) - local_keys_to_content_bytes = {} + media_path_to_content_bytes = {} for file_mapping in file_mappings: - local_key, file_path = file_mapping.split(":", 1) + media_path, file_path = file_mapping.split(":", 1) - local_keys_to_content_bytes[local_key] = pathlib.Path(file_path).read_bytes() if file_path else None + media_path_to_content_bytes[media_path] = pathlib.Path(file_path).read_bytes() if file_path else None next_version = create_next_component_version( component.pk, - media_to_replace=local_keys_to_content_bytes, + media_to_replace=media_path_to_content_bytes, created=created, ) self.stdout.write( f"Created v{next_version.version_num} of " - f"{next_version.component.key} ({next_version.uuid}):" + f"{next_version.component.entity_ref} ({next_version.uuid}):" ) for cvm in next_version.componentversionmedia_set.all(): - self.stdout.write(f"- {cvm.key} ({cvm.uuid})") + self.stdout.write(f"- {cvm.path} ({cvm.uuid})") diff --git a/src/openedx_content/management/commands/lp_dump.py b/src/openedx_content/management/commands/lp_dump.py index c23038b2d..af2c3dea7 100644 --- a/src/openedx_content/management/commands/lp_dump.py +++ b/src/openedx_content/management/commands/lp_dump.py @@ -24,7 +24,7 @@ class Command(BaseCommand): help = 'Export a learning package to a zip file.' def add_arguments(self, parser): - parser.add_argument('lp_key', type=str, help='The key of the LearningPackage to dump') + parser.add_argument('package_ref', type=str, help='The package_ref of the LearningPackage to dump') parser.add_argument('file_name', type=str, help='The name of the output zip file') parser.add_argument( '--username', @@ -40,7 +40,7 @@ def add_arguments(self, parser): ) def handle(self, *args, **options): - lp_key = options['lp_key'] + package_ref = options['package_ref'] file_name = options['file_name'] username = options['username'] origin_server = options['origin_server'] @@ -52,18 +52,18 @@ def handle(self, *args, **options): if username: user = User.objects.get(username=username) start_time = time.time() - create_zip_file(lp_key, file_name, user=user, origin_server=origin_server) + create_zip_file(package_ref, file_name, user=user, origin_server=origin_server) elapsed = time.time() - start_time - message = f'{lp_key} written to {file_name} (create_zip_file: {elapsed:.2f} seconds)' + message = f'{package_ref} written to {file_name} (create_zip_file: {elapsed:.2f} seconds)' self.stdout.write(self.style.SUCCESS(message)) except LearningPackage.DoesNotExist as exc: - message = f"Learning package with key {lp_key} not found" + message = f"Learning package {package_ref!r} not found" raise CommandError(message) from exc except Exception as e: - message = f"Failed to export learning package '{lp_key}': {e}" + message = f"Failed to export learning package {package_ref!r}: {e}" logger.exception( - "Failed to create zip file %s (learning‑package key %s)", + "Failed to create zip file %s (package_ref %s)", file_name, - lp_key, + package_ref, ) raise CommandError(message) from e diff --git a/src/openedx_content/migrations/0004_rename_collection_key_to_code_in_python.py b/src/openedx_content/migrations/0004_rename_collection_key_to_code_in_python.py new file mode 100644 index 000000000..dd40e17fd --- /dev/null +++ b/src/openedx_content/migrations/0004_rename_collection_key_to_code_in_python.py @@ -0,0 +1,28 @@ +# Generated by Django 5.2.12 on 2026-03-24 20:33 + +from django.conf import settings +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('openedx_content', '0003_rename_content_to_media'), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.RemoveConstraint( + model_name='collection', + name='oel_coll_uniq_lp_key', + ), + migrations.RenameField( + model_name='collection', + old_name='key', + new_name='collection_code', + ), + migrations.AddConstraint( + model_name='collection', + constraint=models.UniqueConstraint(fields=('learning_package', 'collection_code'), name='oel_coll_uniq_lp_key'), + ), + ] diff --git a/src/openedx_content/migrations/0005_rename_collection_key_to_code_in_db.py b/src/openedx_content/migrations/0005_rename_collection_key_to_code_in_db.py new file mode 100644 index 000000000..209851c58 --- /dev/null +++ b/src/openedx_content/migrations/0005_rename_collection_key_to_code_in_db.py @@ -0,0 +1,20 @@ +# Generated by Django 5.2.12 on 2026-03-24 20:33 + +from django.db import migrations + +import openedx_django_lib.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('openedx_content', '0004_rename_collection_key_to_code_in_python'), + ] + + operations = [ + migrations.AlterField( + model_name='collection', + name='collection_code', + field=openedx_django_lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, max_length=500), + ), + ] diff --git a/src/openedx_content/migrations/0006_add_collection_code_regex_validation.py b/src/openedx_content/migrations/0006_add_collection_code_regex_validation.py new file mode 100644 index 000000000..9e92fbc56 --- /dev/null +++ b/src/openedx_content/migrations/0006_add_collection_code_regex_validation.py @@ -0,0 +1,23 @@ +# Generated by Django 5.2.12 on 2026-03-24 20:33 + +import re + +import django.core.validators +from django.db import migrations + +import openedx_django_lib.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('openedx_content', '0005_rename_collection_key_to_code_in_db'), + ] + + operations = [ + migrations.AlterField( + model_name='collection', + name='collection_code', + field=openedx_django_lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, max_length=255, validators=[django.core.validators.RegexValidator(re.compile('^[a-zA-Z0-9\\-\\_\\.]+\\Z'), 'Enter a valid "code name" consisting of letters, numbers, underscores, hyphens, or periods.', 'invalid')]), + ), + ] diff --git a/src/openedx_content/migrations/0007_merge_collection_container_changes.py b/src/openedx_content/migrations/0007_merge_collection_container_changes.py new file mode 100644 index 000000000..1c306bbb3 --- /dev/null +++ b/src/openedx_content/migrations/0007_merge_collection_container_changes.py @@ -0,0 +1,14 @@ +# Generated by Django 5.2.12 on 2026-03-30 14:33 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('openedx_content', '0005_containertypes'), + ('openedx_content', '0006_add_collection_code_regex_validation'), + ] + + operations = [ + ] diff --git a/src/openedx_content/migrations/0008_rename_component_local_key_to_code_in_python.py b/src/openedx_content/migrations/0008_rename_component_local_key_to_code_in_python.py new file mode 100644 index 000000000..8cc11f2d8 --- /dev/null +++ b/src/openedx_content/migrations/0008_rename_component_local_key_to_code_in_python.py @@ -0,0 +1,38 @@ +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('openedx_content', '0007_merge_collection_container_changes'), + ] + + operations = [ + migrations.RemoveConstraint( + model_name='component', + name='oel_component_uniq_lc_ct_lk', + ), + migrations.RemoveIndex( + model_name='component', + name='oel_component_idx_ct_lk', + ), + migrations.RenameField( + model_name='component', + old_name='local_key', + new_name='component_code', + ), + migrations.AddConstraint( + model_name='component', + constraint=models.UniqueConstraint( + fields=('learning_package', 'component_type', 'component_code'), + name='oel_component_uniq_lc_ct_lk', + ), + ), + migrations.AddIndex( + model_name='component', + index=models.Index( + fields=['component_type', 'component_code'], + name='oel_component_idx_ct_lk', + ), + ), + ] diff --git a/src/openedx_content/migrations/0009_rename_component_local_key_to_code_in_db.py b/src/openedx_content/migrations/0009_rename_component_local_key_to_code_in_db.py new file mode 100644 index 000000000..6f9ae63da --- /dev/null +++ b/src/openedx_content/migrations/0009_rename_component_local_key_to_code_in_db.py @@ -0,0 +1,21 @@ +from django.db import migrations + +import openedx_django_lib.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('openedx_content', '0008_rename_component_local_key_to_code_in_python'), + ] + + operations = [ + migrations.AlterField( + model_name='component', + name='component_code', + field=openedx_django_lib.fields.MultiCollationCharField( + db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, + max_length=500, + ), + ), + ] diff --git a/src/openedx_content/migrations/0010_add_component_code_regex_validation.py b/src/openedx_content/migrations/0010_add_component_code_regex_validation.py new file mode 100644 index 000000000..81ffa068b --- /dev/null +++ b/src/openedx_content/migrations/0010_add_component_code_regex_validation.py @@ -0,0 +1,31 @@ +import re + +import django.core.validators +from django.db import migrations + +import openedx_django_lib.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('openedx_content', '0009_rename_component_local_key_to_code_in_db'), + ] + + operations = [ + migrations.AlterField( + model_name='component', + name='component_code', + field=openedx_django_lib.fields.MultiCollationCharField( + db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, + max_length=255, + validators=[ + django.core.validators.RegexValidator( + re.compile('^[a-zA-Z0-9\\-\\_\\.]+\\Z'), + 'Enter a valid "code name" consisting of letters, numbers, underscores, hyphens, or periods.', + 'invalid', + ), + ], + ), + ), + ] diff --git a/src/openedx_content/migrations/0011_add_container_code.py b/src/openedx_content/migrations/0011_add_container_code.py new file mode 100644 index 000000000..f8ad95a1b --- /dev/null +++ b/src/openedx_content/migrations/0011_add_container_code.py @@ -0,0 +1,88 @@ +import re + +import django.core.validators +import django.db.models.deletion +from django.db import migrations, models + +import openedx_django_lib.fields + + +def backfill_container_code(apps, schema_editor): + """ + Backfill container_code and learning_package from publishable_entity. + + For existing containers, container_code is set to the entity key (the + only identifier available at this point). Future containers will have + container_code set by the caller. + """ + Container = apps.get_model("openedx_content", "Container") + for container in Container.objects.select_related("publishable_entity__learning_package").all(): + container.learning_package = container.publishable_entity.learning_package + container.container_code = container.publishable_entity.key + container.save(update_fields=["learning_package", "container_code"]) + + +class Migration(migrations.Migration): + + dependencies = [ + ("openedx_content", "0010_add_component_code_regex_validation"), + ] + + operations = [ + # 1. Add learning_package FK (nullable initially for backfill) + migrations.AddField( + model_name="container", + name="learning_package", + field=models.ForeignKey( + null=True, + on_delete=django.db.models.deletion.CASCADE, + to="openedx_content.learningpackage", + ), + ), + # 2. Add container_code (nullable initially for backfill) + migrations.AddField( + model_name="container", + name="container_code", + field=openedx_django_lib.fields.MultiCollationCharField( + db_collations={"mysql": "utf8mb4_bin", "sqlite": "BINARY"}, + max_length=255, + null=True, + ), + ), + # 3. Backfill both fields from publishable_entity + migrations.RunPython(backfill_container_code, migrations.RunPython.noop), + # 4. Make both fields non-nullable and add regex validation to container_code + migrations.AlterField( + model_name="container", + name="learning_package", + field=models.ForeignKey( + null=False, + on_delete=django.db.models.deletion.CASCADE, + to="openedx_content.learningpackage", + ), + ), + migrations.AlterField( + model_name="container", + name="container_code", + field=openedx_django_lib.fields.MultiCollationCharField( + db_collations={"mysql": "utf8mb4_bin", "sqlite": "BINARY"}, + max_length=255, + validators=[ + django.core.validators.RegexValidator( + re.compile(r"^[a-zA-Z0-9\-\_\.]+\Z"), + "Enter a valid \"code name\" consisting of letters, numbers, " + "underscores, hyphens, or periods.", + "invalid", + ), + ], + ), + ), + # 5. Add uniqueness constraint + migrations.AddConstraint( + model_name="container", + constraint=models.UniqueConstraint( + fields=["learning_package", "container_code"], + name="oel_container_uniq_lp_cc", + ), + ), + ] diff --git a/src/openedx_content/migrations/0012_rename_publishableentity_key_to_entity_ref.py b/src/openedx_content/migrations/0012_rename_publishableentity_key_to_entity_ref.py new file mode 100644 index 000000000..2dc3fd265 --- /dev/null +++ b/src/openedx_content/migrations/0012_rename_publishableentity_key_to_entity_ref.py @@ -0,0 +1,40 @@ +from django.db import migrations, models + +import openedx_django_lib.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('openedx_content', '0011_add_container_code'), + ] + + operations = [ + migrations.RemoveConstraint( + model_name='publishableentity', + name='oel_pub_ent_uniq_lp_key', + ), + migrations.RemoveIndex( + model_name='publishableentity', + name='oel_pub_ent_idx_key', + ), + migrations.RenameField( + model_name='publishableentity', + old_name='key', + new_name='entity_ref', + ), + migrations.AddConstraint( + model_name='publishableentity', + constraint=models.UniqueConstraint( + fields=['learning_package', 'entity_ref'], + name='oel_pub_ent_uniq_lp_key', + ), + ), + migrations.AddIndex( + model_name='publishableentity', + index=models.Index( + fields=['entity_ref'], + name='oel_pub_ent_idx_key', + ), + ), + ] diff --git a/src/openedx_content/migrations/0013_rename_publishableentity_db_column_key_to_entity_ref.py b/src/openedx_content/migrations/0013_rename_publishableentity_db_column_key_to_entity_ref.py new file mode 100644 index 000000000..23f8e3976 --- /dev/null +++ b/src/openedx_content/migrations/0013_rename_publishableentity_db_column_key_to_entity_ref.py @@ -0,0 +1,45 @@ +from django.db import migrations + +import openedx_django_lib.fields + + +class Migration(migrations.Migration): + """ + Rename the underlying DB column for PublishableEntity.entity_ref from + '_key' to 'entity_ref'. + + Uses SeparateDatabaseAndState: the state operation tells Django the field + no longer has db_column='_key'; the database operation does the actual + column rename via RunSQL. + + The table is named 'openedx_content_publishableentity' (the Django default + after migration 0002 reset all oel_* table names). + """ + + dependencies = [ + ('openedx_content', '0012_rename_publishableentity_key_to_entity_ref'), + ] + + operations = [ + migrations.SeparateDatabaseAndState( + state_operations=[ + migrations.AlterField( + model_name='publishableentity', + name='entity_ref', + field=openedx_django_lib.fields.MultiCollationCharField( + db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, + max_length=500, + ), + ), + ], + database_operations=[ + # Rename the physical column from '_key' to 'entity_ref'. + # MySQL and SQLite both support RENAME COLUMN (SQLite >= 3.25.0, + # MySQL >= 8.0). Django's test runner uses SQLite >= 3.25. + migrations.RunSQL( + sql='ALTER TABLE openedx_content_publishableentity RENAME COLUMN _key TO entity_ref', + reverse_sql='ALTER TABLE openedx_content_publishableentity RENAME COLUMN entity_ref TO _key', + ), + ], + ), + ] diff --git a/src/openedx_content/migrations/0014_rename_learningpackage_key_to_package_ref.py b/src/openedx_content/migrations/0014_rename_learningpackage_key_to_package_ref.py new file mode 100644 index 000000000..feb7391ae --- /dev/null +++ b/src/openedx_content/migrations/0014_rename_learningpackage_key_to_package_ref.py @@ -0,0 +1,29 @@ +from django.db import migrations, models + +import openedx_django_lib.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('openedx_content', '0013_rename_publishableentity_db_column_key_to_entity_ref'), + ] + + operations = [ + migrations.RemoveConstraint( + model_name='learningpackage', + name='oel_publishing_lp_uniq_key', + ), + migrations.RenameField( + model_name='learningpackage', + old_name='key', + new_name='package_ref', + ), + migrations.AddConstraint( + model_name='learningpackage', + constraint=models.UniqueConstraint( + fields=['package_ref'], + name='oel_publishing_lp_uniq_key', + ), + ), + ] diff --git a/src/openedx_content/migrations/0015_rename_learningpackage_db_column_key_to_package_ref.py b/src/openedx_content/migrations/0015_rename_learningpackage_db_column_key_to_package_ref.py new file mode 100644 index 000000000..cd531c4f2 --- /dev/null +++ b/src/openedx_content/migrations/0015_rename_learningpackage_db_column_key_to_package_ref.py @@ -0,0 +1,39 @@ +from django.db import migrations + +import openedx_django_lib.fields + + +class Migration(migrations.Migration): + """ + Rename the underlying DB column for LearningPackage.package_ref from + '_key' to 'package_ref'. + + Uses SeparateDatabaseAndState with RunSQL (RENAME COLUMN) for + SQLite/MySQL compatibility. The table is named + 'openedx_content_learningpackage' (Django default after migration 0002). + """ + + dependencies = [ + ('openedx_content', '0014_rename_learningpackage_key_to_package_ref'), + ] + + operations = [ + migrations.SeparateDatabaseAndState( + state_operations=[ + migrations.AlterField( + model_name='learningpackage', + name='package_ref', + field=openedx_django_lib.fields.MultiCollationCharField( + db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, + max_length=500, + ), + ), + ], + database_operations=[ + migrations.RunSQL( + sql='ALTER TABLE openedx_content_learningpackage RENAME COLUMN _key TO package_ref', + reverse_sql='ALTER TABLE openedx_content_learningpackage RENAME COLUMN package_ref TO _key', + ), + ], + ), + ] diff --git a/src/openedx_content/migrations/0016_rename_componentversionmedia_key_to_path.py b/src/openedx_content/migrations/0016_rename_componentversionmedia_key_to_path.py new file mode 100644 index 000000000..700a56e0b --- /dev/null +++ b/src/openedx_content/migrations/0016_rename_componentversionmedia_key_to_path.py @@ -0,0 +1,29 @@ +from django.db import migrations, models + +import openedx_django_lib.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('openedx_content', '0015_rename_learningpackage_db_column_key_to_package_ref'), + ] + + operations = [ + migrations.RemoveConstraint( + model_name='componentversionmedia', + name='oel_cvcontent_uniq_cv_key', + ), + migrations.RenameField( + model_name='componentversionmedia', + old_name='key', + new_name='path', + ), + migrations.AddConstraint( + model_name='componentversionmedia', + constraint=models.UniqueConstraint( + fields=['component_version', 'path'], + name='oel_cvcontent_uniq_cv_key', + ), + ), + ] diff --git a/src/openedx_content/migrations/0017_rename_componentversionmedia_db_column_key_to_path.py b/src/openedx_content/migrations/0017_rename_componentversionmedia_db_column_key_to_path.py new file mode 100644 index 000000000..b4dc7c6a0 --- /dev/null +++ b/src/openedx_content/migrations/0017_rename_componentversionmedia_db_column_key_to_path.py @@ -0,0 +1,37 @@ +from django.db import migrations + +import openedx_django_lib.fields + + +class Migration(migrations.Migration): + """ + Rename the underlying DB column for ComponentVersionMedia.path from + '_key' to 'path'. Uses SeparateDatabaseAndState with RunSQL for + SQLite/MySQL compatibility. The table is named + 'openedx_content_componentversionmedia' (Django default). + """ + + dependencies = [ + ('openedx_content', '0016_rename_componentversionmedia_key_to_path'), + ] + + operations = [ + migrations.SeparateDatabaseAndState( + state_operations=[ + migrations.AlterField( + model_name='componentversionmedia', + name='path', + field=openedx_django_lib.fields.MultiCollationCharField( + db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, + max_length=500, + ), + ), + ], + database_operations=[ + migrations.RunSQL( + sql='ALTER TABLE openedx_content_componentversionmedia RENAME COLUMN _key TO path', + reverse_sql='ALTER TABLE openedx_content_componentversionmedia RENAME COLUMN path TO _key', + ), + ], + ), + ] diff --git a/src/openedx_django_lib/fields.py b/src/openedx_django_lib/fields.py index c41678993..1cecd6261 100644 --- a/src/openedx_django_lib/fields.py +++ b/src/openedx_django_lib/fields.py @@ -11,9 +11,12 @@ from __future__ import annotations import hashlib +import re import uuid +from django.core.validators import RegexValidator from django.db import models +from django.utils.translation import gettext_lazy as _ from .collations import MultiCollationMixin from .validators import validate_utc_datetime @@ -110,16 +113,36 @@ def immutable_uuid_field() -> models.UUIDField: ) -def key_field(**kwargs) -> MultiCollationCharField: +# Alphanumeric, hyphens, underscores, periods +CODE_REGEX = re.compile(r"^[a-zA-Z0-9\-\_\.]+\Z") + + +def code_field(**kwargs) -> MultiCollationCharField: + """ + Field to hold a 'code', i.e. a slug-like local identifier. """ - Externally created Identifier fields. + return case_sensitive_char_field( + max_length=255, + blank=False, + validators=[ + RegexValidator( + CODE_REGEX, + # Translators: "letters" means latin letters: a-z and A-Z. + _('Enter a valid "code name" consisting of letters, numbers, underscores, hyphens, or periods.'), + "invalid", + ), + ], + **kwargs, + ) - These will often be local to a particular scope, like within a - LearningPackage. It's up to the application as to whether they're - semantically meaningful or look more machine-generated. - Other apps should *not* make references to these values directly, since - these values may in theory change (even if this is rare in practice). +def ref_field(**kwargs) -> MultiCollationCharField: + """ + Opaque reference string fields. + + These hold externally-created identifiers that are local to a particular + scope, like within a LearningPackage. Consumers must treat the value as + an atomic string and must never parse or reconstruct it. """ return case_sensitive_char_field(max_length=500, blank=False, **kwargs) diff --git a/tests/openedx_content/applets/backup_restore/test_backup.py b/tests/openedx_content/applets/backup_restore/test_backup.py index e7a69cff7..0551b46f3 100644 --- a/tests/openedx_content/applets/backup_restore/test_backup.py +++ b/tests/openedx_content/applets/backup_restore/test_backup.py @@ -51,7 +51,7 @@ def setUpTestData(cls): # Create a Learning Package for the test cls.learning_package = api.create_learning_package( - key="ComponentTestCase-test-key", + package_ref="ComponentTestCase-test-key", title="Components Test Case Learning Package", description="This is a test learning package for components.", ) @@ -69,7 +69,7 @@ def setUpTestData(cls): cls.published_component, _ = api.create_component_and_version( cls.learning_package.id, cls.problem_type, - local_key="my_published_example", + component_code="my_published_example", title="My published problem", created=cls.now, created_by=cls.user.id, @@ -80,7 +80,7 @@ def setUpTestData(cls): cls.published_component2, _ = api.create_component_and_version( cls.learning_package.id, cls.problem_type, - local_key="My_published_example", + component_code="My_published_example", title="My published problem 2", created=cls.now, created_by=cls.user.id, @@ -109,14 +109,14 @@ def setUpTestData(cls): api.create_component_version_media( new_problem_version.pk, new_txt_media.pk, - key="hello.txt", + path="hello.txt", ) # Create a Draft component, one in each learning package cls.draft_component, _ = api.create_component_and_version( cls.learning_package.id, cls.html_type, - local_key="my_draft_example", + component_code="my_draft_example", title="My draft html", created=cls.now, created_by=cls.user.id, @@ -138,7 +138,7 @@ def setUpTestData(cls): api.create_component_version_media( new_html_version.pk, cls.html_asset_media.id, - key="static/other/subdirectory/hello.html", + path="static/other/subdirectory/hello.html", ) components = api.get_publishable_entities(cls.learning_package) @@ -146,7 +146,7 @@ def setUpTestData(cls): cls.collection = api.create_collection( cls.learning_package.id, - key="COL1", + collection_code="COL1", created_by=cls.user.id, title="Collection 1", description="Description of Collection 1", @@ -154,15 +154,15 @@ def setUpTestData(cls): api.add_to_collection( cls.learning_package.id, - cls.collection.key, + cls.collection.collection_code, components ) api.create_container( learning_package_id=cls.learning_package.id, - key="unit-1", created=cls.now, created_by=cls.user.id, + container_code="unit-1", container_cls=Unit, ) @@ -216,8 +216,8 @@ def check_zip_file_structure(self, zip_path: Path): self.assertIn(expected_path, zip_name_list) def test_lp_dump_command(self): - lp_key = self.learning_package.key - file_name = f"{lp_key}.zip" + package_ref = self.learning_package.package_ref + file_name = f"{package_ref}.zip" try: out = StringIO() @@ -225,7 +225,7 @@ def test_lp_dump_command(self): # Call the management command to dump the learning package call_command( - "lp_dump", lp_key, file_name, username=self.user.username, origin_server=origin_server, stdout=out + "lp_dump", package_ref, file_name, username=self.user.username, origin_server=origin_server, stdout=out ) # Check that the zip file was created @@ -242,7 +242,8 @@ def test_lp_dump_command(self): Path("package.toml"), [ '[learning_package]', - f'key = "{self.learning_package.key}"', + f'package_ref = "{self.learning_package.package_ref}"', + f'key = "{self.learning_package.package_ref}"', f'title = "{self.learning_package.title}"', f'description = "{self.learning_package.description}"', '[meta]', @@ -277,8 +278,19 @@ def test_lp_dump_command(self): for file_path, expected_content in expected_files.items(): self.check_toml_file(zip_path, Path(file_path), expected_content) + # Verify that collection TOMLs include both 'key' (legacy) and 'collection_code' + # (new name), so older software can still read archives produced after the rename. + self.check_toml_file( + zip_path, + Path("collections/col1.toml"), + [ + 'key = "COL1"', + 'collection_code = "COL1"', + ] + ) + # Check the output message - message = f'{lp_key} written to {file_name}' + message = f'{package_ref} written to {file_name}' self.assertIn(message, out.getvalue()) except Exception as e: # pylint: disable=broad-exception-caught self.fail(f"lp_dump command failed with error: {e}") @@ -289,11 +301,11 @@ def test_lp_dump_command(self): def test_dump_nonexistent_learning_package(self): out = StringIO() - lp_key = "nonexistent_lp" - file_name = f"{lp_key}.zip" + package_ref = "nonexistent_lp" + file_name = f"{package_ref}.zip" with self.assertRaises(CommandError): # Attempt to dump a learning package that does not exist - call_command("lp_dump", lp_key, file_name, stdout=out) + call_command("lp_dump", package_ref, file_name, stdout=out) self.assertIn("Learning package 'nonexistent_lp' does not exist", out.getvalue()) def test_queries_n_plus_problem(self): @@ -315,7 +327,7 @@ def test_queries_n_plus_problem(self): api.create_component_and_version( self.learning_package.id, self.problem_type, - local_key="my_published_example2", + component_code="my_published_example2", title="My published problem 2", created=self.now, created_by=self.user.id, diff --git a/tests/openedx_content/applets/backup_restore/test_restore.py b/tests/openedx_content/applets/backup_restore/test_restore.py index 0116731c4..fd1fd2e88 100644 --- a/tests/openedx_content/applets/backup_restore/test_restore.py +++ b/tests/openedx_content/applets/backup_restore/test_restore.py @@ -8,7 +8,13 @@ from django.core.management import call_command from django.test import TestCase -from openedx_content.applets.backup_restore.zipper import LearningPackageUnzipper, generate_staged_lp_key +from openedx_content.applets.backup_restore.serializers import ( + CollectionSerializer, + ComponentSerializer, + ContainerSerializer, + EntitySerializer, +) +from openedx_content.applets.backup_restore.zipper import LearningPackageUnzipper, generate_staged_package_ref from openedx_content.applets.collections import api as collections_api from openedx_content.applets.components import api as components_api from openedx_content.applets.containers import api as containers_api @@ -25,7 +31,7 @@ def setUp(self): super().setUp() self.fixtures_folder = os.path.join(os.path.dirname(__file__), "fixtures/library_backup") self.zip_file = folder_to_inmemory_zip(self.fixtures_folder) - self.lp_key = "lib:WGU:LIB_C001" + self.package_ref = "lib:WGU:LIB_C001" self.user = User.objects.create_user(username='lp_user', password='12345') @@ -42,14 +48,14 @@ def test_restore_command(self, mock_load_learning_package): # You can pass any dummy path, since load_learning_package is mocked call_command("lp_load", "dummy.zip", "lp_user", stdout=out) - lp = self.verify_lp(restore_result["lp_restored_data"]["key"]) + lp = self.verify_lp(restore_result["lp_restored_data"]["package_ref"]) self.verify_containers(lp) self.verify_components(lp) self.verify_collections(lp) - def verify_lp(self, key): + def verify_lp(self, package_ref): """Verify the learning package was restored correctly.""" - lp = publishing_api.LearningPackage.objects.filter(key=key).first() + lp = publishing_api.LearningPackage.objects.filter(package_ref=package_ref).first() assert lp is not None, "Learning package was not restored." assert lp.title == "Library test" assert lp.description == "" @@ -61,26 +67,26 @@ def verify_containers(self, lp): expected_container_keys = ["unit1-b7eafb", "subsection1-48afa3", "section1-8ca126"] for container in container_qs: - assert container.key in expected_container_keys + assert container.entity_ref in expected_container_keys draft_version = publishing_api.get_draft_version(container.publishable_entity.id) published_version = publishing_api.get_published_version(container.publishable_entity.id) assert container.created_by is not None assert container.created_by.username == "lp_user" - if container.key == "unit1-b7eafb": + if container.entity_ref == "unit1-b7eafb": assert containers_api.get_container_type_code_of(container) == "unit" assert draft_version is not None assert draft_version.version_num == 2 assert draft_version.created_by is not None assert draft_version.created_by.username == "lp_user" assert published_version is None - elif container.key == "subsection1-48afa3": + elif container.entity_ref == "subsection1-48afa3": assert containers_api.get_container_type_code_of(container) == "subsection" assert draft_version is not None assert draft_version.version_num == 2 assert draft_version.created_by is not None assert draft_version.created_by.username == "lp_user" assert published_version is None - elif container.key == "section1-8ca126": + elif container.entity_ref == "section1-8ca126": assert containers_api.get_container_type_code_of(container) == "section" assert draft_version is not None assert draft_version.version_num == 2 @@ -88,7 +94,7 @@ def verify_containers(self, lp): assert draft_version.created_by.username == "lp_user" assert published_version is None else: - assert False, f"Unexpected container key: {container.key}" + assert False, f"Unexpected container key: {container.entity_ref}" def verify_components(self, lp): # pylint: disable=too-many-statements @@ -104,12 +110,12 @@ def verify_components(self, lp): "xblock.v1:html:c22b9f97-f1e9-4e8f-87f0-d5a3c26083e2" ] for component in component_qs: - assert component.key in expected_component_keys + assert component.entity_ref in expected_component_keys draft_version = publishing_api.get_draft_version(component.publishable_entity.id) published_version = publishing_api.get_published_version(component.publishable_entity.id) assert component.created_by is not None assert component.created_by.username == "lp_user" - if component.key == "xblock.v1:drag-and-drop-v2:4d1b2fac-8b30-42fb-872d-6b10ab580b27": + if component.entity_ref == "xblock.v1:drag-and-drop-v2:4d1b2fac-8b30-42fb-872d-6b10ab580b27": assert component.component_type.name == "drag-and-drop-v2" assert component.component_type.namespace == "xblock.v1" assert draft_version is not None @@ -124,7 +130,7 @@ def verify_components(self, lp): assert " None: cls.learning_package = api.create_learning_package( - key="ComponentTestCase-test-key", + package_ref="ComponentTestCase-test-key", title="Components Test Case Learning Package", ) cls.learning_package_2 = api.create_learning_package( - key="ComponentTestCase-test-key-2", + package_ref="ComponentTestCase-test-key-2", title="Components Test Case another Learning Package", ) cls.now = datetime(2024, 8, 5, tzinfo=timezone.utc) @@ -64,35 +64,35 @@ def setUpTestData(cls) -> None: super().setUpTestData() cls.collection1 = api.create_collection( cls.learning_package.id, - key="COL1", + collection_code="COL1", created_by=None, title="Collection 1", description="Description of Collection 1", ) cls.collection2 = api.create_collection( cls.learning_package.id, - key="COL2", + collection_code="COL2", created_by=None, title="Collection 2", description="Description of Collection 2", ) cls.collection3 = api.create_collection( cls.learning_package.id, - key="COL3", + collection_code="COL3", created_by=None, title="Collection 3", description="Description of Collection 3", ) cls.another_library_collection = api.create_collection( cls.learning_package_2.id, - key="another_library", + collection_code="another_library", created_by=None, title="Collection 4", description="Description of Collection 4", ) cls.disabled_collection = api.create_collection( cls.learning_package.id, - key="disabled_collection", + collection_code="disabled_collection", created_by=None, title="Disabled Collection", description="Description of Disabled Collection", @@ -123,7 +123,7 @@ 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.pk, self.another_library_collection.collection_code) def test_get_collections(self): """ @@ -191,14 +191,14 @@ def test_create_collection(self): with freeze_time(created_time): collection = api.create_collection( self.learning_package.id, - key='MYCOL', + collection_code='MYCOL', title="My Collection", created_by=user.id, description="This is my collection", ) assert collection.title == "My Collection" - assert collection.key == "MYCOL" + assert collection.collection_code == "MYCOL" assert collection.description == "This is my collection" assert collection.enabled assert collection.created == created_time @@ -211,15 +211,35 @@ def test_create_collection_without_description(self): """ collection = api.create_collection( self.learning_package.id, - key='MYCOL', + collection_code='MYCOL', created_by=None, title="My Collection", ) assert collection.title == "My Collection" - assert collection.key == "MYCOL" + assert collection.collection_code == "MYCOL" assert collection.description == "" assert collection.enabled + def test_create_collection_invalid_code(self): + """ + collection_code must only contain alphanumerics, hyphens, underscores, and periods. + """ + invalid_codes = [ + "has space", + "has@symbol", + "has/slash", + "has#hash", + ] + for code in invalid_codes: + with self.subTest(code=code): + with self.assertRaises(ValidationError): + api.create_collection( + self.learning_package.id, + collection_code=code, + title="Test", + created_by=None, + ) + class CollectionEntitiesTestCase(CollectionsTestCase): """ @@ -249,9 +269,9 @@ def setUpTestData(cls) -> None: created_time = datetime(2025, 4, 1, tzinfo=timezone.utc) cls.draft_unit = api.create_container( learning_package_id=cls.learning_package.id, - key="unit-1", created=created_time, created_by=cls.user.id, + container_code="unit-1", container_cls=Unit, ) @@ -259,7 +279,7 @@ def setUpTestData(cls) -> None: cls.published_component, _ = api.create_component_and_version( cls.learning_package.id, cls.problem_type, - local_key="my_published_example", + component_code="my_published_example", title="My published problem", created=cls.now, created_by=cls.user.id, @@ -274,7 +294,7 @@ def setUpTestData(cls) -> None: cls.draft_component, _ = api.create_component_and_version( cls.learning_package.id, cls.html_type, - local_key="my_draft_example", + component_code="my_draft_example", title="My draft html", created=cls.now, created_by=cls.user.id, @@ -283,7 +303,7 @@ def setUpTestData(cls) -> None: # Add some shared components to the collections cls.collection1 = api.add_to_collection( cls.learning_package.id, - key=cls.collection1.key, + collection_code=cls.collection1.collection_code, entities_qset=PublishableEntity.objects.filter(id__in=[ cls.published_component.pk, ]), @@ -291,7 +311,7 @@ def setUpTestData(cls) -> None: ) cls.collection2 = api.add_to_collection( cls.learning_package.id, - key=cls.collection2.key, + collection_code=cls.collection2.collection_code, entities_qset=PublishableEntity.objects.filter(id__in=[ cls.published_component.pk, cls.draft_component.pk, @@ -300,7 +320,7 @@ def setUpTestData(cls) -> None: ) cls.disabled_collection = api.add_to_collection( cls.learning_package.id, - key=cls.disabled_collection.key, + collection_code=cls.disabled_collection.collection_code, entities_qset=PublishableEntity.objects.filter(id__in=[ cls.published_component.pk, ]), @@ -335,7 +355,7 @@ def test_add_to_collection(self): with freeze_time(modified_time): self.collection1 = api.add_to_collection( self.learning_package.id, - self.collection1.key, + self.collection1.collection_code, PublishableEntity.objects.filter(id__in=[ self.draft_component.pk, self.draft_unit.pk, @@ -360,7 +380,7 @@ def test_add_to_collection_again(self): with freeze_time(modified_time): self.collection2 = api.add_to_collection( self.learning_package.id, - self.collection2.key, + self.collection2.collection_code, PublishableEntity.objects.filter(id__in=[ self.published_component.pk, ]), @@ -380,7 +400,7 @@ def test_add_to_collection_wrong_learning_package(self): with self.assertRaises(ValidationError): api.add_to_collection( self.learning_package_2.id, - self.another_library_collection.key, + self.another_library_collection.collection_code, PublishableEntity.objects.filter(id__in=[ self.published_component.pk, ]), @@ -396,7 +416,7 @@ def test_remove_from_collection(self): with freeze_time(modified_time): self.collection2 = api.remove_from_collection( self.learning_package.id, - self.collection2.key, + self.collection2.collection_code, PublishableEntity.objects.filter(id__in=[ self.published_component.pk, self.draft_unit.pk, @@ -414,7 +434,7 @@ def test_get_entity_collections(self): """ collections = api.get_entity_collections( self.learning_package.id, - self.published_component.publishable_entity.key, + self.published_component.publishable_entity.entity_ref, ) assert list(collections) == [ self.collection1, @@ -424,46 +444,46 @@ def test_get_entity_collections(self): def test_get_collection_components(self): assert list(api.get_collection_components( self.learning_package.id, - self.collection1.key, + self.collection1.collection_code, )) == [self.published_component] assert list(api.get_collection_components( self.learning_package.id, - self.collection2.key, + self.collection2.collection_code, )) == [self.published_component, self.draft_component] assert not list(api.get_collection_components( self.learning_package.id, - self.collection3.key, + self.collection3.collection_code, )) assert not list(api.get_collection_components( self.learning_package.id, - self.another_library_collection.key, + self.another_library_collection.collection_code, )) 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: int, collection_code: str): return ( pe.container for pe in - api.get_collection_entities(learning_package_id, collection_key).exclude(container=None) + api.get_collection_entities(learning_package_id, collection_code).exclude(container=None) ) assert not list(get_collection_containers( self.learning_package.id, - self.collection1.key, + self.collection1.collection_code, )) assert list(get_collection_containers( self.learning_package.id, - self.collection2.key, + self.collection2.collection_code, )) == [self.draft_unit.container] assert not list(get_collection_containers( self.learning_package.id, - self.collection3.key, + self.collection3.collection_code, )) assert not list(get_collection_containers( self.learning_package.id, - self.another_library_collection.key, + self.another_library_collection.collection_code, )) @@ -481,7 +501,7 @@ def setUpTestData(cls) -> None: super().setUpTestData() cls.collection = api.create_collection( cls.learning_package.id, - key="MYCOL", + collection_code="MYCOL", title="Collection", created_by=None, description="Description of Collection", @@ -495,7 +515,7 @@ def test_update_collection(self): with freeze_time(modified_time): collection = api.update_collection( self.learning_package.id, - key=self.collection.key, + collection_code=self.collection.collection_code, title="New Title", description="", ) @@ -511,17 +531,19 @@ def test_update_collection_partial(self): """ collection = api.update_collection( self.learning_package.id, - key=self.collection.key, + collection_code=self.collection.collection_code, title="New Title", ) assert collection.title == "New Title" assert collection.description == self.collection.description # unchanged - assert f"{collection}" == f" (lp:{self.learning_package.id} {self.collection.key}:New Title)" + assert f"{collection}" == ( + f" (lp:{self.learning_package.id} {self.collection.collection_code}:New Title)" + ) collection = api.update_collection( self.learning_package.id, - key=self.collection.key, + collection_code=self.collection.collection_code, description="New description", ) @@ -536,7 +558,7 @@ def test_update_collection_empty(self): with freeze_time(modified_time): collection = api.update_collection( self.learning_package.id, - key=self.collection.key, + collection_code=self.collection.collection_code, ) assert collection.title == self.collection.title # unchanged @@ -550,7 +572,7 @@ def test_update_collection_not_found(self): with self.assertRaises(ObjectDoesNotExist): api.update_collection( self.learning_package.id, - key="12345", + collection_code="12345", title="New Title", ) @@ -568,13 +590,13 @@ def test_soft_delete(self): with freeze_time(modified_time): collection = api.delete_collection( self.learning_package.id, - key=self.collection2.key, + collection_code=self.collection2.collection_code, ) # Collection was disabled and still exists in the database assert not collection.enabled assert collection.modified == modified_time - assert collection == api.get_collection(self.learning_package.id, collection.key) + assert collection == api.get_collection(self.learning_package.id, collection.collection_code) # ...and the collection's entities remain intact. assert list(collection.entities.all()) == [ self.draft_unit.publishable_entity, @@ -590,7 +612,7 @@ def test_delete(self): with freeze_time(modified_time): collection = api.delete_collection( self.learning_package.id, - key=self.collection2.key, + collection_code=self.collection2.collection_code, hard_delete=True, ) @@ -598,15 +620,15 @@ def test_delete(self): assert collection.enabled assert not collection.id with self.assertRaises(ObjectDoesNotExist): - api.get_collection(self.learning_package.id, collection.key) + api.get_collection(self.learning_package.id, collection.collection_code) # ...and the entities have been removed from this collection assert list(api.get_entity_collections( self.learning_package.id, - self.published_component.publishable_entity.key, + self.published_component.publishable_entity.entity_ref, )) == [self.collection1] assert not list(api.get_entity_collections( self.learning_package.id, - self.draft_component.publishable_entity.key, + self.draft_component.publishable_entity.entity_ref, )) def test_restore(self): @@ -615,20 +637,20 @@ def test_restore(self): """ collection = api.delete_collection( self.learning_package.id, - key=self.collection2.key, + collection_code=self.collection2.collection_code, ) modified_time = datetime(2024, 8, 8, tzinfo=timezone.utc) with freeze_time(modified_time): collection = api.restore_collection( self.learning_package.id, - key=self.collection2.key, + collection_code=self.collection2.collection_code, ) # Collection was enabled and still exists in the database assert collection.enabled assert collection.modified == modified_time - assert collection == api.get_collection(self.learning_package.id, collection.key) + assert collection == api.get_collection(self.learning_package.id, collection.collection_code) # ...and the collection's entities remain intact. assert list(collection.entities.all()) == [ self.draft_unit.publishable_entity, @@ -714,12 +736,12 @@ def test_set_collection_wrong_learning_package(self): We cannot set collections with a different learning package than the component. """ learning_package_3 = api.create_learning_package( - key="ComponentTestCase-test-key-3", + package_ref="ComponentTestCase-test-key-3", title="Components Test Case Learning Package-3", ) collection = api.create_collection( learning_package_3.id, - key="MYCOL", + collection_code="MYCOL", title="My Collection", created_by=None, description="Description of Collection", diff --git a/tests/openedx_content/applets/components/test_api.py b/tests/openedx_content/applets/components/test_api.py index a9af5f65d..10f57d782 100644 --- a/tests/openedx_content/applets/components/test_api.py +++ b/tests/openedx_content/applets/components/test_api.py @@ -35,7 +35,7 @@ class ComponentTestCase(TestCase): @classmethod def setUpTestData(cls) -> None: cls.learning_package = publishing_api.create_learning_package( - key="ComponentTestCase-test-key", + package_ref="ComponentTestCase-test-key", title="Components Test Case Learning Package", ) cls.now = datetime(2023, 5, 8, tzinfo=timezone.utc) @@ -54,14 +54,14 @@ def publish_component(self, component: Component): ), ) - def create_component(self, *, title: str = "Test Component", key: str = "component:1") -> tuple[ + def create_component(self, *, title: str = "Test Component", component_code: str = "component_1") -> tuple[ Component, ComponentVersion ]: """ Helper method to quickly create a component """ return components_api.create_component_and_version( self.learning_package.id, component_type=self.problem_type, - local_key=key, + component_code=component_code, title=title, created=self.now, created_by=None, @@ -86,7 +86,7 @@ def test_component_num_queries(self) -> None: component, _version = components_api.create_component_and_version( self.learning_package.id, component_type=self.problem_type, - local_key="Query Counting", + component_code="Query_Counting", title="Querying Counting Problem", created=self.now, created_by=None, @@ -131,7 +131,7 @@ def setUpTestData(cls) -> None: cls.published_problem, _version = components_api.create_component_and_version( cls.learning_package.id, component_type=v2_problem_type, - local_key="pp_lk", + component_code="pp_lk", title="Published Problem", created=cls.now, created_by=None, @@ -139,7 +139,7 @@ def setUpTestData(cls) -> None: cls.published_html, _version = components_api.create_component_and_version( cls.learning_package.id, component_type=cls.html_type, - local_key="ph_lk", + component_code="ph_lk", title="Published HTML", created=cls.now, created_by=None, @@ -153,7 +153,7 @@ def setUpTestData(cls) -> None: cls.unpublished_problem, _version = components_api.create_component_and_version( cls.learning_package.id, component_type=v2_problem_type, - local_key="upp_lk", + component_code="upp_lk", title="Unpublished Problem", created=cls.now, created_by=None, @@ -161,7 +161,7 @@ def setUpTestData(cls) -> None: cls.unpublished_html, _version = components_api.create_component_and_version( cls.learning_package.id, component_type=cls.html_type, - local_key="uph_lk", + component_code="uph_lk", title="Unpublished HTML", created=cls.now, created_by=None, @@ -172,7 +172,7 @@ def setUpTestData(cls) -> None: cls.deleted_video, _version = components_api.create_component_and_version( cls.learning_package.id, component_type=cls.video_type, - local_key="dv_lk", + component_code="dv_lk", title="Deleted Video", created=cls.now, created_by=None, @@ -324,14 +324,14 @@ def setUpTestData(cls) -> None: cls.problem = components_api.create_component( cls.learning_package.id, component_type=cls.problem_type, - local_key='my_component', + component_code='my_component', created=cls.now, created_by=None, ) cls.html = components_api.create_component( cls.learning_package.id, component_type=cls.html_type, - local_key='my_component', + component_code='my_component', created=cls.now, created_by=None, can_stand_alone=False, @@ -342,47 +342,47 @@ def test_simple_get(self): with self.assertRaises(ObjectDoesNotExist): components_api.get_component(-1) - def test_publishing_entity_key_convention(self): - """Our mapping convention is {namespace}:{component_type}:{local_key}""" - assert self.problem.key == "xblock.v1:problem:my_component" + def test_publishing_entity_ref_convention(self): + """entity_ref convention: {namespace}:{component_type}:{component_code}""" + assert self.problem.entity_ref == "xblock.v1:problem:my_component" def test_stand_alone_flag(self): """Check if can_stand_alone flag is set""" - component = components_api.get_component_by_key( + component = components_api.get_component_by_code( self.learning_package.id, namespace='xblock.v1', type_name='html', - local_key='my_component', + component_code='my_component', ) assert not component.publishable_entity.can_stand_alone - def test_get_by_key(self): - assert self.html == components_api.get_component_by_key( + def test_get_by_code(self): + assert self.html == components_api.get_component_by_code( self.learning_package.id, namespace='xblock.v1', type_name='html', - local_key='my_component', + component_code='my_component', ) with self.assertRaises(ObjectDoesNotExist): - components_api.get_component_by_key( + components_api.get_component_by_code( self.learning_package.id, namespace='xblock.v1', type_name='video', # 'video' doesn't match anything we have - local_key='my_component', + component_code='my_component', ) - def test_exists_by_key(self): - assert components_api.component_exists_by_key( + def test_exists_by_code(self): + assert components_api.component_exists_by_code( self.learning_package.id, namespace='xblock.v1', type_name='problem', - local_key='my_component', + component_code='my_component', ) - assert not components_api.component_exists_by_key( + assert not components_api.component_exists_by_code( self.learning_package.id, namespace='xblock.v1', type_name='problem', - local_key='not_my_component', + component_code='not_my_component', ) @@ -399,7 +399,7 @@ def setUpTestData(cls) -> None: cls.problem = components_api.create_component( cls.learning_package.id, component_type=cls.problem_type, - local_key='my_component', + component_code='my_component', created=cls.now, created_by=None, ) @@ -422,7 +422,7 @@ def test_add(self): components_api.create_component_version_media( new_version.pk, new_media.pk, - key="my/path/to/hello.txt", + path="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) \ @@ -430,7 +430,7 @@ def test_add(self): .get(publishable_entity_version__version_num=1) assert ( new_media == - new_version.media.get(componentversionmedia__key="my/path/to/hello.txt") + new_version.media.get(componentversionmedia__path="my/path/to/hello.txt") ) # Write the same content again, but to an absolute path (should auto- @@ -438,14 +438,14 @@ def test_add(self): components_api.create_component_version_media( new_version.pk, new_media.pk, - key="//nested/path/hello.txt", + path="//nested/path/hello.txt", ) new_version = components_api.get_component(self.problem.pk) \ .versions \ .get(publishable_entity_version__version_num=1) assert ( new_media == - new_version.media.get(componentversionmedia__key="nested/path/hello.txt") + new_version.media.get(componentversionmedia__path="nested/path/hello.txt") ) def test_bytes_content(self): @@ -461,8 +461,8 @@ def test_bytes_content(self): created=self.now, ) - content_txt = version_1.media.get(componentversionmedia__key="raw.txt") - content_raw_txt = version_1.media.get(componentversionmedia__key="no_ext") + content_txt = version_1.media.get(componentversionmedia__path="raw.txt") + content_raw_txt = version_1.media.get(componentversionmedia__path="no_ext") assert content_txt.size == len(bytes_media) assert str(content_txt.media_type) == 'text/plain' @@ -509,12 +509,12 @@ def test_multiple_versions(self): assert ( hello_media == version_1.media - .get(componentversionmedia__key="hello.txt") + .get(componentversionmedia__path="hello.txt") ) assert ( goodbye_media == version_1.media - .get(componentversionmedia__key="goodbye.txt") + .get(componentversionmedia__path="goodbye.txt") ) # This should keep the old value for goodbye.txt, add blank.txt, and set @@ -533,17 +533,17 @@ def test_multiple_versions(self): assert ( blank_media == version_2.media - .get(componentversionmedia__key="hello.txt") + .get(componentversionmedia__path="hello.txt") ) assert ( goodbye_media == version_2.media - .get(componentversionmedia__key="goodbye.txt") + .get(componentversionmedia__path="goodbye.txt") ) assert ( blank_media == version_2.media - .get(componentversionmedia__key="blank.txt") + .get(componentversionmedia__path="blank.txt") ) # Now we're going to set "hello.txt" back to hello_content, but remove @@ -564,7 +564,7 @@ def test_multiple_versions(self): assert ( hello_media == version_3.media - .get(componentversionmedia__key="hello.txt") + .get(componentversionmedia__path="hello.txt") ) def test_create_next_version_forcing_num_version(self): @@ -626,17 +626,17 @@ def test_create_multiple_next_versions_and_diff_content(self): assert ( python_source_asset == version_2_draft.media.get( - componentversionmedia__key="static/profile.webp") + componentversionmedia__path="static/profile.webp") ) assert ( python_source_asset == version_2_draft.media.get( - componentversionmedia__key="static/new_file.webp") + componentversionmedia__path="static/new_file.webp") ) with self.assertRaises(ObjectDoesNotExist): # This file was in the published version, but not in the draft version # since we ignored previous content. - version_2_draft.media.get(componentversionmedia__key="static/background.webp") + version_2_draft.media.get(componentversionmedia__path="static/background.webp") class SetCollectionsTestCase(ComponentTestCase): @@ -659,28 +659,28 @@ def setUpTestData(cls) -> None: cls.published_problem, _ = components_api.create_component_and_version( cls.learning_package.id, component_type=v2_problem_type, - local_key="pp_lk", + component_code="pp_lk", title="Published Problem", created=cls.now, created_by=None, ) cls.collection1 = collection_api.create_collection( cls.learning_package.id, - key="MYCOL1", + collection_code="MYCOL1", title="Collection1", created_by=None, description="Description of Collection 1", ) cls.collection2 = collection_api.create_collection( cls.learning_package.id, - key="MYCOL2", + collection_code="MYCOL2", title="Collection2", created_by=None, description="Description of Collection 2", ) cls.collection3 = collection_api.create_collection( cls.learning_package.id, - key="MYCOL3", + collection_code="MYCOL3", title="Collection3", created_by=None, description="Description of Collection 3", @@ -696,31 +696,19 @@ class TestComponentTypeUtils(TestCase): Test the component type utility functions. """ - def test_get_or_create_component_type_by_entity_key_creates_new(self): - comp_type, local_key = components_api.get_or_create_component_type_by_entity_key( - "video:youtube:abcd1234" - ) + def test_get_or_create_component_type_creates_new(self): + comp_type = components_api.get_or_create_component_type("video", "youtube") assert isinstance(comp_type, ComponentType) assert comp_type.namespace == "video" assert comp_type.name == "youtube" - assert local_key == "abcd1234" assert ComponentType.objects.count() == 1 - def test_get_or_create_component_type_by_entity_key_existing(self): + def test_get_or_create_component_type_existing(self): ComponentType.objects.create(namespace="video", name="youtube") - comp_type, local_key = components_api.get_or_create_component_type_by_entity_key( - "video:youtube:efgh5678" - ) + comp_type = components_api.get_or_create_component_type("video", "youtube") assert comp_type.namespace == "video" assert comp_type.name == "youtube" - assert local_key == "efgh5678" assert ComponentType.objects.count() == 1 - - def test_get_or_create_component_type_by_entity_key_invalid_format(self): - with self.assertRaises(ValueError) as ctx: - components_api.get_or_create_component_type_by_entity_key("not-enough-parts") - - self.assertIn("Invalid entity_key format", str(ctx.exception)) diff --git a/tests/openedx_content/applets/components/test_assets.py b/tests/openedx_content/applets/components/test_assets.py index a9c378681..4de763f57 100644 --- a/tests/openedx_content/applets/components/test_assets.py +++ b/tests/openedx_content/applets/components/test_assets.py @@ -53,13 +53,13 @@ def setUpTestData(cls) -> None: cls.html_media_type = media_api.get_or_create_media_type("text/html") cls.learning_package = publishing_api.create_learning_package( - key="ComponentTestCase-test-key", + package_ref="ComponentTestCase-test-key", title="Components Test Case Learning Package", ) cls.component, cls.component_version = components_api.create_component_and_version( cls.learning_package.id, component_type=cls.problem_type, - local_key="my_problem", + component_code="my_problem", title="My Problem", created=cls.now, created_by=None, @@ -75,7 +75,7 @@ def setUpTestData(cls) -> None: components_api.create_component_version_media( cls.component_version.pk, cls.problem_media.id, - key="block.xml", + path="block.xml", ) # Python source file, stored as a file. This is hypothetical, as we @@ -89,7 +89,7 @@ def setUpTestData(cls) -> None: components_api.create_component_version_media( cls.component_version.pk, cls.python_source_asset.id, - key="src/grader.py", + path="src/grader.py", ) # An HTML file that is student downloadable @@ -102,7 +102,7 @@ def setUpTestData(cls) -> None: components_api.create_component_version_media( cls.component_version.pk, cls.html_asset_media.id, - key="static/hello.html", + path="static/hello.html", ) def test_no_component_version(self): @@ -125,11 +125,11 @@ def _assert_has_component_version_headers(self, headers): Note: The request header values in an HttpResponse will all have been serialized to strings. """ - assert headers["X-Open-edX-Component-Key"] == self.component.key + assert headers["X-Open-edX-Component-Key"] == self.component.entity_ref assert headers["X-Open-edX-Component-Uuid"] == str(self.component.uuid) assert headers["X-Open-edX-Component-Version-Uuid"] == str(self.component_version.uuid) assert headers["X-Open-edX-Component-Version-Num"] == str(self.component_version.version_num) - assert headers["X-Open-edX-Learning-Package-Key"] == self.learning_package.key + assert headers["X-Open-edX-Learning-Package-Key"] == self.learning_package.package_ref assert headers["X-Open-edX-Learning-Package-Uuid"] == str(self.learning_package.uuid) def test_404s_with_component_version_info(self): diff --git a/tests/openedx_content/applets/components/test_models.py b/tests/openedx_content/applets/components/test_models.py index 356346a6a..c955d5c1a 100644 --- a/tests/openedx_content/applets/components/test_models.py +++ b/tests/openedx_content/applets/components/test_models.py @@ -51,7 +51,7 @@ def test_latest_version(self) -> None: component, component_version = create_component_and_version( self.learning_package.id, component_type=self.problem_type, - local_key="monty_hall", + component_code="monty_hall", title="Monty Hall Problem", created=self.now, created_by=None, @@ -82,7 +82,7 @@ def test_last_publish_log(self): component_with_changes, _ = create_component_and_version( self.learning_package.id, component_type=self.problem_type, - local_key="with_changes", + component_code="with_changes", title="Component with changes v1", created=self.now, created_by=None, @@ -92,7 +92,7 @@ def test_last_publish_log(self): component_with_no_changes, _ = create_component_and_version( self.learning_package.id, component_type=self.problem_type, - local_key="with_no_changes", + component_code="with_no_changes", title="Component with no changes v1", created=self.now, created_by=None, diff --git a/tests/openedx_content/applets/containers/test_api.py b/tests/openedx_content/applets/containers/test_api.py index 50871d8c5..6026c17ee 100644 --- a/tests/openedx_content/applets/containers/test_api.py +++ b/tests/openedx_content/applets/containers/test_api.py @@ -92,18 +92,18 @@ def _other_user(django_user_model): @pytest.fixture(name="lp") def _lp() -> LearningPackage: """Get a Learning Package.""" - return publishing_api.create_learning_package(key="containers-test-lp", title="Testing Containers Main LP") + return publishing_api.create_learning_package(package_ref="containers-test-lp", title="Testing Containers Main LP") @pytest.fixture(name="lp2") def _lp2() -> LearningPackage: """Get a Second Learning Package.""" - return publishing_api.create_learning_package(key="containers-test-lp2", title="Testing Containers (📦 2)") + return publishing_api.create_learning_package(package_ref="containers-test-lp2", title="Testing Containers (📦 2)") -def create_test_entity(learning_package: LearningPackage, key: str, title: str) -> TestEntity: +def create_test_entity(learning_package: LearningPackage, ref: str, title: str) -> TestEntity: """Create a TestEntity with a draft version""" - pe = publishing_api.create_publishable_entity(learning_package.id, key, created=now, created_by=None) + pe = publishing_api.create_publishable_entity(learning_package.id, ref, created=now, created_by=None) new_entity = TestEntity.objects.create(publishable_entity=pe) pev = publishing_api.create_publishable_entity_version( new_entity.pk, @@ -119,35 +119,38 @@ def create_test_entity(learning_package: LearningPackage, key: str, title: str) @pytest.fixture(name="child_entity1") def _child_entity1(lp: LearningPackage) -> TestEntity: """An example entity, such as a component""" - return create_test_entity(lp, key="child_entity1", title="Child 1 🌴") + return create_test_entity(lp, ref="child_entity1", title="Child 1 🌴") @pytest.fixture(name="child_entity2") def _child_entity2(lp: LearningPackage) -> TestEntity: """An example entity, such as a component""" - return create_test_entity(lp, key="child_entity2", title="Child 2 🌈") + return create_test_entity(lp, ref="child_entity2", title="Child 2 🌈") @pytest.fixture(name="child_entity3") def _child_entity3(lp: LearningPackage) -> TestEntity: """An example entity, such as a component""" - return create_test_entity(lp, key="child_entity3", title="Child 3 ⛵️") + return create_test_entity(lp, ref="child_entity3", title="Child 3 ⛵️") @pytest.fixture(name="other_lp_child") def _other_lp_child(lp2: LearningPackage) -> TestEntity: """An example entity, such as a component""" - return create_test_entity(lp2, key="other_lp_child", title="Child in other Learning Package 📦") + return create_test_entity(lp2, ref="other_lp_child", title="Child in other Learning Package 📦") def create_test_container( - learning_package: LearningPackage, key: str, entities: containers_api.EntityListInput, title: str = "" + learning_package: LearningPackage, + container_code: str, + entities: containers_api.EntityListInput, + title: str = "", ) -> TestContainer: """Create a TestContainer with a draft version""" container, _version = containers_api.create_container_and_version( learning_package.id, - key=key, - title=title or f"Container ({key})", + container_code=container_code, + title=title or f"Container ({container_code})", entities=entities, container_cls=TestContainer, created=now, @@ -161,7 +164,7 @@ def _parent_of_two(lp: LearningPackage, child_entity1: TestEntity, child_entity2 """An TestContainer with two children""" return create_test_container( lp, - key="parent_of_two", + container_code="parent_of_two", title="Generic Container with Two Unpinned Children", entities=[child_entity1, child_entity2], ) @@ -177,7 +180,7 @@ def _parent_of_three( """An TestContainer with three children, two of which are pinned""" return create_test_container( lp, - key="parent_of_three", + container_code="parent_of_three", title="Generic Container with Two 📌 Pinned Children and One Unpinned", entities=[child_entity3.versioning.draft, child_entity2.versioning.draft, child_entity1], ) @@ -193,7 +196,7 @@ def _parent_of_six( """An TestContainer with six children, two of each entity, with different pinned combinations""" return create_test_container( lp, - key="parent_of_six", + container_code="parent_of_six", title="Generic Container with Two 📌 Pinned Children and One Unpinned", entities=[ # 1: both unpinned, 2: both pinned, and 3: pinned and unpinned @@ -216,7 +219,7 @@ def _grandparent( """An ContainerContainer with two unpinned children""" grandparent, _version = containers_api.create_container_and_version( lp.id, - key="grandparent", + container_code="grandparent", title="Generic Container with Two Unpinned TestContainer children", entities=[parent_of_two, parent_of_three], container_cls=ContainerContainer, @@ -235,7 +238,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, - key="abandoned-container", + container_code="abandoned-container", title="Abandoned Container 1", entities=[child_entity1], container_cls=TestContainer, @@ -252,7 +255,7 @@ def _other_lp_parent(lp2: LearningPackage, other_lp_child: TestEntity) -> TestCo """An TestContainer with one child""" other_lp_parent, _version = containers_api.create_container_and_version( lp2.id, - key="other_lp_parent", + container_code="other_lp_parent", title="Generic Container with One Unpinned Child Entity", entities=[other_lp_child], container_cls=TestContainer, @@ -302,7 +305,7 @@ def test_create_generic_empty_container(lp: LearningPackage, admin_user) -> None """ container, container_v1 = containers_api.create_container_and_version( lp.pk, - key="new-container-1", + container_code="new-container-1", title="Test Container 1", container_cls=TestContainer, created=now, @@ -317,7 +320,7 @@ def test_create_generic_empty_container(lp: LearningPackage, admin_user) -> None assert isinstance(container_v1, TestContainerVersion) assert container.versioning.draft == container_v1 assert container.versioning.published is None - assert container.key == "new-container-1" + assert container.entity_ref == "new-container-1" assert container.versioning.draft.title == "Test Container 1" assert container.created == now assert container.created_by == admin_user @@ -340,10 +343,12 @@ 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.pk, container_code="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.pk, container_code="c2", **base_args, entities=[child_entity1] + ) # versioning helpers @@ -401,9 +406,9 @@ def test_create_next_container_version_no_changes(parent_of_two: TestContainer, assert version_2.entity_list_id == original_version.entity_list_id assert version_2.created == v2_date assert version_2.created_by == other_user - assert containers_api.get_container_children_entities_keys( + assert containers_api.get_container_children_entity_refs( original_version - ) == containers_api.get_container_children_entities_keys(version_2) + ) == containers_api.get_container_children_entity_refs(version_2) def test_create_next_container_version_with_changes( @@ -435,7 +440,7 @@ def test_create_next_container_version_with_changes( assert version_5.created_by is None assert version_5.title == "New Title - children reversed" assert version_5.entity_list_id != original_version.entity_list_id - assert containers_api.get_container_children_entities_keys(version_5) == ["child_entity2", "child_entity1"] + assert containers_api.get_container_children_entity_refs(version_5) == ["child_entity2", "child_entity1"] def test_create_next_container_version_with_append( @@ -737,28 +742,28 @@ def test_get_container_version_nonexistent() -> None: containers_api.get_container_version(-500) -# get_container_by_key +# get_container_by_ref -def test_get_container_by_key(lp: LearningPackage, parent_of_two: TestContainer) -> None: +def test_get_container_by_ref(lp: LearningPackage, parent_of_two: TestContainer) -> None: """ - Test getting a specific container by key + Test getting a specific container by entity ref """ - result = containers_api.get_container_by_key(lp.pk, parent_of_two.key) + result = containers_api.get_container_by_ref(lp.pk, parent_of_two.entity_ref) assert result == parent_of_two.container # The API always returns "Container", not specific subclasses like TestContainer: assert result.__class__ is Container -def test_get_container_by_key_nonexistent(lp: LearningPackage) -> None: +def test_get_container_by_ref_nonexistent(lp: LearningPackage) -> None: """ - Test getting a specific container by key, where the key and/or learning package is invalid + Test getting a specific container by entity ref, where the ref and/or learning package is invalid """ with pytest.raises(LearningPackage.DoesNotExist): - containers_api.get_container_by_key(32874, "invalid-key") + containers_api.get_container_by_ref(32874, "invalid-ref") with pytest.raises(Container.DoesNotExist): - containers_api.get_container_by_key(lp.pk, "invalid-key") + containers_api.get_container_by_ref(lp.pk, "invalid-ref") # get_container_subclass @@ -969,7 +974,8 @@ def test_no_publish_parent(parent_of_two: TestContainer, child_entity1: TestEnti Test that publishing an entity does NOT publish changes to its parent containers """ # "child_entity1" is a child of "parent_of_two" - assert child_entity1.key in containers_api.get_container_children_entities_keys(parent_of_two.versioning.draft) + children_refs = containers_api.get_container_children_entity_refs(parent_of_two.versioning.draft) + assert child_entity1.entity_ref in children_refs # Neither are published: assert child_entity1.versioning.published is None assert parent_of_two.versioning.published is None @@ -1125,7 +1131,7 @@ def test_publishing_shared_component(lp: LearningPackage): Everything is "unpinned". """ # 1️⃣ Create the units and publish them: - c1, c2, c3, c4, c5 = [create_test_entity(lp, key=f"C{i}", title=f"Component {i}") for i in range(1, 6)] + c1, c2, c3, c4, c5 = [create_test_entity(lp, ref=f"C{i}", title=f"Component {i}") for i in range(1, 6)] c1_v1 = c1.versioning.draft c3_v1 = c3.versioning.draft c4_v1 = c4.versioning.draft @@ -1134,7 +1140,7 @@ def test_publishing_shared_component(lp: LearningPackage): lp.pk, entities=[c1, c2, c3], title="Unit 1", - key="unit:1", + container_code="unit-1", created=now, created_by=None, container_cls=TestContainer, @@ -1143,7 +1149,7 @@ def test_publishing_shared_component(lp: LearningPackage): lp.pk, entities=[c2, c4, c5], title="Unit 2", - key="unit:2", + container_code="unit-2", created=now, created_by=None, container_cls=TestContainer, @@ -1208,7 +1214,7 @@ def test_shallow_publish_log( ) -> None: """Simple test of publishing a container plus children and reviewing the publish log""" publish_log = publish_entity(parent_of_two) - assert list(publish_log.records.order_by("entity__pk").values_list("entity__key", flat=True)) == [ + assert list(publish_log.records.order_by("entity__pk").values_list("entity__entity_ref", flat=True)) == [ # The container and its two children should be the only things published: "child_entity1", "child_entity2", @@ -1227,7 +1233,7 @@ def test_uninstalled_publish( with django_assert_num_queries(49): publish_log = publish_entity(container_of_uninstalled_type) # Nothing else should have been affected by the publish: - assert list(publish_log.records.order_by("entity__pk").values_list("entity__key", flat=True)) == [ + assert list(publish_log.records.order_by("entity__pk").values_list("entity__entity_ref", flat=True)) == [ "child_entity1", "abandoned-container", ] @@ -1256,7 +1262,7 @@ def test_deep_publish_log( # Create a "great grandparent" container that contains "grandparent" great_grandparent = create_test_container( lp, - key="great_grandparent", + container_code="great_grandparent", title="Great-grandparent container", entities=[grandparent], ) @@ -1265,7 +1271,7 @@ def test_deep_publish_log( with django_assert_num_queries(49): publish_log = publish_entity(container_of_uninstalled_type) # Nothing else should have been affected by the publish: - assert list(publish_log.records.order_by("entity__pk").values_list("entity__key", flat=True)) == [ + assert list(publish_log.records.order_by("entity__pk").values_list("entity__entity_ref", flat=True)) == [ "child_entity1", "abandoned-container", ] @@ -1273,7 +1279,7 @@ def test_deep_publish_log( # Publish great_grandparent. Should publish the whole tree. with django_assert_num_queries(126): publish_log = publish_entity(great_grandparent) - assert list(publish_log.records.order_by("entity__pk").values_list("entity__key", flat=True)) == [ + assert list(publish_log.records.order_by("entity__pk").values_list("entity__entity_ref", flat=True)) == [ "child_entity2", "parent_of_two", "parent_of_three", @@ -1381,7 +1387,7 @@ def test_snapshots_of_published_unit(lp: LearningPackage, child_entity1: TestEnt child_entity1_v1 = child_entity1.versioning.draft # At first the container has one child (unpinned): - container = create_test_container(lp, key="c", entities=[child_entity1]) + container = create_test_container(lp, container_code="c", entities=[child_entity1]) modify_entity(child_entity1, title="Component 1 as of checkpoint 1") _, before_publish = containers_api.get_entities_in_container_as_of(container, 0) assert not before_publish # Empty list @@ -1592,21 +1598,21 @@ def test_get_container_children_count_queries( assert containers_api.get_container_children_count(parent_of_six, published=True) == 6 -# get_container_children_entities_keys +# get_container_children_entity_refs -def test_get_container_children_entities_keys(grandparent: ContainerContainer, parent_of_six: TestContainer) -> None: - """Test `get_container_children_entities_keys()`""" +def test_get_container_children_entity_refs(grandparent: ContainerContainer, parent_of_six: TestContainer) -> None: + """Test `get_container_children_entity_refs()`""" - # TODO: is get_container_children_entities_keys() a useful API method? It's not used in edx-platform. + # TODO: is get_container_children_entity_refs() a useful API method? It's not used in edx-platform. - assert containers_api.get_container_children_entities_keys(grandparent.versioning.draft) == [ + assert containers_api.get_container_children_entity_refs(grandparent.versioning.draft) == [ # These are the two children of "grandparent" - see diagram near the top of this file. "parent_of_two", "parent_of_three", ] - assert containers_api.get_container_children_entities_keys(parent_of_six.versioning.draft) == [ + assert containers_api.get_container_children_entity_refs(parent_of_six.versioning.draft) == [ "child_entity3", "child_entity2", "child_entity1", diff --git a/tests/openedx_content/applets/media/test_file_storage.py b/tests/openedx_content/applets/media/test_file_storage.py index 7cf6ea664..a4aec1ba5 100644 --- a/tests/openedx_content/applets/media/test_file_storage.py +++ b/tests/openedx_content/applets/media/test_file_storage.py @@ -29,7 +29,7 @@ def setUp(self) -> None: """ super().setUp() learning_package = publishing_api.create_learning_package( - key="ContentFileStorageTestCase-test-key", + package_ref="ContentFileStorageTestCase-test-key", title="Content File Storage Test Case Learning Package", ) self.html_media_type = media_api.get_or_create_media_type("text/html") diff --git a/tests/openedx_content/applets/publishing/test_api.py b/tests/openedx_content/applets/publishing/test_api.py index 96012633d..c84b3ba1f 100644 --- a/tests/openedx_content/applets/publishing/test_api.py +++ b/tests/openedx_content/applets/publishing/test_api.py @@ -38,13 +38,13 @@ def test_normal(self) -> None: # Note: we must specify '-> None' to opt in to t created = datetime(2023, 4, 2, 15, 9, 0, tzinfo=timezone.utc) description = "A fun Description!" package = publishing_api.create_learning_package( - key=key, + package_ref=key, title=title, description=description, created=created ) - assert package.key == "my_key" + assert package.package_ref == "my_key" assert package.title == "My Excellent Title with Emoji 🔥" assert package.description == "A fun Description!" assert package.created == created @@ -59,11 +59,11 @@ def test_normal(self) -> None: # Note: we must specify '-> None' to opt in to t # Now test editing the fields. updated_package = publishing_api.update_learning_package( package.id, - key="new_key", + package_ref="new_key", title="new title", description="new description", ) - assert updated_package.key == "new_key" + assert updated_package.package_ref == "new_key" assert updated_package.title == "new title" assert updated_package.description == "new description" assert updated_package.created == created @@ -77,7 +77,7 @@ def test_auto_datetime(self) -> None: title = "My Excellent Title with Emoji 🔥" package = publishing_api.create_learning_package(key, title) - assert package.key == "my_key" + assert package.package_ref == "my_key" assert package.title == "My Excellent Title with Emoji 🔥" # Auto-generated datetime checking... @@ -97,7 +97,7 @@ def test_non_utc_time(self) -> None: """ with pytest.raises(ValidationError) as excinfo: publishing_api.create_learning_package( - key="my_key", + package_ref="my_key", title="A Title", created=datetime(2023, 4, 2) ) @@ -115,7 +115,7 @@ def test_already_exists(self) -> None: with pytest.raises(ValidationError) as excinfo: publishing_api.create_learning_package("my_key", "Duplicate") message_dict = excinfo.value.message_dict - assert "key" in message_dict + assert "package_ref" in message_dict class DraftTestCase(TestCase): @@ -1075,9 +1075,9 @@ def test_parent_child_side_effects(self) -> None: ) container = containers_api.create_container( self.learning_package.id, - "my_container", created=self.now, created_by=None, + container_code="my_container", container_cls=TestContainer, ) container_v1 = containers_api.create_container_version( @@ -1155,9 +1155,9 @@ def test_bulk_parent_child_side_effects(self) -> None: ) container = containers_api.create_container( self.learning_package.id, - "my_container", created=self.now, created_by=None, + container_code="my_container", container_cls=TestContainer, ) container_v1 = containers_api.create_container_version( @@ -1231,16 +1231,16 @@ def test_draft_dependency_multiple_parents(self) -> None: ) unit_1 = containers_api.create_container( self.learning_package.id, - "unit_1", created=self.now, created_by=None, + container_code="unit_1", container_cls=TestContainer, ) unit_2 = containers_api.create_container( self.learning_package.id, - "unit_2", created=self.now, created_by=None, + container_code="unit_2", container_cls=TestContainer, ) for unit in [unit_1, unit_2]: @@ -1293,9 +1293,9 @@ def test_multiple_layers_of_containers(self) -> None: ) unit = containers_api.create_container( self.learning_package.id, - "unit_1", created=self.now, created_by=None, + container_code="unit_1", container_cls=TestContainer, ) containers_api.create_container_version( @@ -1308,9 +1308,9 @@ def test_multiple_layers_of_containers(self) -> None: ) subsection = containers_api.create_container( self.learning_package.id, - "subsection_1", created=self.now, created_by=None, + container_code="subsection_1", container_cls=TestContainer, ) containers_api.create_container_version( @@ -1392,9 +1392,9 @@ def test_publish_all_layers(self) -> None: ) unit = containers_api.create_container( self.learning_package.id, - "unit_1", created=self.now, created_by=None, + container_code="unit_1", container_cls=TestContainer, ) containers_api.create_container_version( @@ -1407,9 +1407,9 @@ def test_publish_all_layers(self) -> None: ) subsection = containers_api.create_container( self.learning_package.id, - "subsection_1", created=self.now, created_by=None, + container_code="subsection_1", container_cls=TestContainer, ) containers_api.create_container_version( @@ -1439,9 +1439,9 @@ def test_container_next_version(self) -> None: ) container = containers_api.create_container( self.learning_package.id, - "my_container", created=self.now, created_by=None, + container_code="my_container", container_cls=TestContainer, ) assert container.versioning.latest is None diff --git a/tests/openedx_content/applets/sections/test_api.py b/tests/openedx_content/applets/sections/test_api.py index ef631e884..55156ec3e 100644 --- a/tests/openedx_content/applets/sections/test_api.py +++ b/tests/openedx_content/applets/sections/test_api.py @@ -22,11 +22,11 @@ def setUp(self) -> None: """Create some potential desdendants for use in these tests.""" super().setUp() self.component_1, self.component_1_v1 = self.create_component( - key="component_1", + component_code="component_1", title="Great-grandchild component", ) self.component_2, self.component_2_v1 = self.create_component( - key="component_2", + component_code="component_2", title="Great-grandchild component", ) common_args: dict[str, Any] = { @@ -35,25 +35,25 @@ def setUp(self) -> None: "created_by": None, } self.unit_1, self.unit_1_v1 = content_api.create_unit_and_version( - key="unit_1", + container_code="unit_1", title="Grandchild Unit 1", components=[self.component_1, self.component_2], **common_args, ) self.unit_2, self.unit_2_v1 = content_api.create_unit_and_version( - key="unit_2", + container_code="unit_2", title="Grandchild Unit 2", components=[self.component_2, self.component_1], # Backwards order from Unit 1 **common_args, ) self.subsection_1, self.subsection_1_v1 = content_api.create_subsection_and_version( - key="subsection_1", + container_code="subsection_1", title="Child Subsection 1", units=[self.unit_1, self.unit_2], **common_args, ) self.subsection_2, self.subsection_2_v1 = content_api.create_subsection_and_version( - key="subsection_2", + container_code="subsection_2", title="Child Subsection 2", units=[self.unit_2, self.unit_1], # Backwards order from subsection 1 **common_args, @@ -69,7 +69,7 @@ def create_section_with_subsections( """Helper method to quickly create a section with some subsections""" section, _section_v1 = content_api.create_section_and_version( learning_package_id=self.learning_package.id, - key=key, + container_code=key, title=title, subsections=subsections, created=self.now, @@ -88,7 +88,7 @@ def test_create_empty_section_and_version(self): """ section, section_version = content_api.create_section_and_version( learning_package_id=self.learning_package.pk, - key="section:key", + container_code="section-key", title="Section", created=self.now, created_by=None, diff --git a/tests/openedx_content/applets/subsections/test_api.py b/tests/openedx_content/applets/subsections/test_api.py index a79c05d49..2a5bc4cda 100644 --- a/tests/openedx_content/applets/subsections/test_api.py +++ b/tests/openedx_content/applets/subsections/test_api.py @@ -19,16 +19,16 @@ class SubsectionsTestCase(ComponentTestCase): def setUp(self) -> None: super().setUp() self.component_1, self.component_1_v1 = self.create_component( - key="Query Counting", + component_code="Query_Counting", title="Querying Counting Problem", ) self.component_2, self.component_2_v1 = self.create_component( - key="Query Counting (2)", + component_code="Query_Counting_2", title="Querying Counting Problem (2)", ) self.unit_1, self.unit_1_v1 = content_api.create_unit_and_version( learning_package_id=self.learning_package.id, - key="unit1", + container_code="unit1", title="Unit 1", components=[self.component_1, self.component_2], created=self.now, @@ -45,7 +45,7 @@ def create_subsection_with_units( """Helper method to quickly create a unit with some units""" subsection, _subsection_v1 = content_api.create_subsection_and_version( learning_package_id=self.learning_package.id, - key=key, + container_code=key, title=title, units=units, created=self.now, @@ -64,7 +64,7 @@ def test_create_empty_subsection_and_version(self): """ subsection, subsection_version = content_api.create_subsection_and_version( learning_package_id=self.learning_package.pk, - key="subsection:key", + container_code="subsection-key", title="Subsection", created=self.now, created_by=None, @@ -153,7 +153,7 @@ def test_create_subsection_with_invalid_children(self): # Try adding a Component to a Subsection with pytest.raises( ValidationError, - match='The entity "xblock.v1:problem:Query Counting" cannot be added to a "subsection" container.', + match='The entity "xblock.v1:problem:Query_Counting" cannot be added to a "subsection" container.', ) as err: content_api.create_next_subsection_version( subsection, @@ -172,7 +172,7 @@ def test_create_subsection_with_invalid_children(self): # (not just `create_next_subsection_version()`) with pytest.raises( ValidationError, - match='The entity "xblock.v1:problem:Query Counting" cannot be added to a "subsection" container.', + match='The entity "xblock.v1:problem:Query_Counting" cannot be added to a "subsection" container.', ): self.create_subsection_with_units([self.component_1], key="unit:key3", title="Unit 3") diff --git a/tests/openedx_content/applets/units/test_api.py b/tests/openedx_content/applets/units/test_api.py index 22b867b04..140db44f8 100644 --- a/tests/openedx_content/applets/units/test_api.py +++ b/tests/openedx_content/applets/units/test_api.py @@ -20,11 +20,11 @@ class UnitsTestCase(ComponentTestCase): def setUp(self) -> None: super().setUp() self.component_1, self.component_1_v1 = self.create_component( - key="Query Counting", + component_code="Query_Counting", title="Querying Counting Problem", ) self.component_2, self.component_2_v1 = self.create_component( - key="Query Counting (2)", + component_code="Query_Counting_2", title="Querying Counting Problem (2)", ) @@ -38,7 +38,7 @@ def create_unit_with_components( """Helper method to quickly create a unit with some components""" unit, _unit_v1 = content_api.create_unit_and_version( learning_package_id=self.learning_package.id, - key=key, + container_code=key, title=title, components=components, created=self.now, @@ -57,7 +57,7 @@ def test_create_empty_unit_and_version(self): """ unit, unit_version = content_api.create_unit_and_version( learning_package_id=self.learning_package.pk, - key="unit:key", + container_code="unit-key", title="Unit", created=self.now, created_by=None, @@ -117,9 +117,9 @@ def test_get_unit_other_container_type(self) -> None: """Test `get_unit()` when the provided PK is for a non-Unit container""" other_container = content_api.create_container( self.learning_package.id, - key="test", created=self.now, created_by=None, + container_code="test", container_cls=TestContainer, ) with pytest.raises(Unit.DoesNotExist): diff --git a/tests/openedx_tagging/test_system_defined_models.py b/tests/openedx_tagging/test_system_defined_models.py index dc58e7b8b..5fd8565c9 100644 --- a/tests/openedx_tagging/test_system_defined_models.py +++ b/tests/openedx_tagging/test_system_defined_models.py @@ -42,7 +42,7 @@ def tag_class_model(self): @property def tag_class_value_field(self) -> str: - return "key" + return "package_ref" @property def tag_class_key_field(self) -> str: @@ -87,8 +87,8 @@ def _create_learning_pkg(**kwargs) -> LearningPackage: def setUpClass(cls): super().setUpClass() # Create two learning packages and a taxonomy that can tag any object using learning packages as tags: - cls.learning_pkg_1 = cls._create_learning_pkg(key="p1", title="Learning Package 1") - cls.learning_pkg_2 = cls._create_learning_pkg(key="p2", title="Learning Package 2") + cls.learning_pkg_1 = cls._create_learning_pkg(package_ref="p1", title="Learning Package 1") + cls.learning_pkg_2 = cls._create_learning_pkg(package_ref="p2", title="Learning Package 2") cls.lp_taxonomy = LPTaxonomyTest.objects.create( taxonomy_class=LPTaxonomyTest, name="LearningPackage Taxonomy", @@ -108,11 +108,11 @@ def test_lp_taxonomy_validation(self): Test that the validation methods of the Learning Package Taxonomy are working """ # Create a new LearningPackage - we know no Tag instances will exist for it yet. - valid_lp = self._create_learning_pkg(key="valid-lp", title="New Learning Packacge") + valid_lp = self._create_learning_pkg(package_ref="valid-lp", title="New Learning Packacge") # The taxonomy can validate tags by value which we've defined as they 'key' of the LearningPackage: - assert self.lp_taxonomy.validate_value(self.learning_pkg_2.key) is True - assert self.lp_taxonomy.validate_value(self.learning_pkg_2.key) is True - assert self.lp_taxonomy.validate_value(valid_lp.key) is True + assert self.lp_taxonomy.validate_value(self.learning_pkg_2.package_ref) is True + assert self.lp_taxonomy.validate_value(self.learning_pkg_2.package_ref) is True + assert self.lp_taxonomy.validate_value(valid_lp.package_ref) is True assert self.lp_taxonomy.validate_value("foo") is False # The taxonomy can also validate tags by external_id, which we've defined as the UUID of the LearningPackage: assert self.lp_taxonomy.validate_external_id(self.learning_pkg_2.uuid) is True