Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
.claude
*.py[cod]
__pycache__
.pytest_cache
Expand Down
4 changes: 3 additions & 1 deletion src/openedx_content/applets/backup_restore/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,9 @@ class CollectionSerializer(serializers.Serializer): # pylint: disable=abstract-
Serializer for collections.
"""
title = serializers.CharField(required=True)
key = serializers.CharField(required=True)
# The model field is now Collection.collection_code, but the archive format
# still uses "key". A future v2 format may align the name.
key = serializers.CharField(required=True, source="collection_code")
description = serializers.CharField(required=True, allow_blank=True)
entities = serializers.ListField(
child=serializers.CharField(),
Expand Down
4 changes: 3 additions & 1 deletion src/openedx_content/applets/backup_restore/toml.py
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,9 @@ def toml_collection(collection: Collection, entity_keys: list[str]) -> str:

collection_table = tomlkit.table()
collection_table.add("title", collection.title)
collection_table.add("key", collection.key)
# Note: the model field is now Collection.collection_code, but the archive
# format still uses "key". A future v2 format may align the name.
collection_table.add("key", collection.collection_code)
collection_table.add("description", collection.description)
collection_table.add("created", collection.created)
collection_table.add("entities", entities_array)
Expand Down
4 changes: 2 additions & 2 deletions src/openedx_content/applets/backup_restore/zipper.py
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,7 @@ 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)
self.add_file_to_zip(
Expand Down Expand Up @@ -779,7 +779,7 @@ def _save_collections(self, learning_package, collections):
)
collection = collections_api.add_to_collection(
learning_package_id=learning_package.id,
key=collection.key,
collection_code=collection.collection_code,
entities_qset=publishing_api.get_publishable_entities(learning_package.id).filter(key__in=entities)
)

Expand Down
6 changes: 3 additions & 3 deletions src/openedx_content/applets/collections/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
}
),
(
Expand Down
42 changes: 22 additions & 20 deletions src/openedx_content/applets/collections/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@

def create_collection(
learning_package_id: LearningPackage.ID,
key: str,
collection_code: str,
*,
title: str,
created_by: int | None,
Expand All @@ -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: LearningPackage.ID, collection_key: str) -> Collection:
def get_collection(learning_package_id: LearningPackage.ID, 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: LearningPackage.ID,
key: str,
collection_code: str,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not at all a blocker, but something to consider for the future: our APIs are a bit inconsistent about how we identify the "thing" we're modifying in each API call.

My preference is this style where you can pass in an instance or a primary key:

def update_catalog_course(
catalog_course: CatalogCourse | CatalogCourse.ID,

def create_next_container_version(
container: Container | Container.ID,

But this collection API is now using a composite (lp_id, collection_code) pattern:

def update_collection(
learning_package_id: LearningPackage.ID,
collection_code: str,

And other APIs accept only an ID:

def create_next_component_version(
component_id: Component.ID,

And other APIs accept only an instance:

def add_tag_to_taxonomy(
taxonomy: Taxonomy,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this would be great to fix soon

*,
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
Expand All @@ -90,17 +92,17 @@ def update_collection(

def delete_collection(
learning_package_id: LearningPackage.ID,
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()
Expand All @@ -112,12 +114,12 @@ def delete_collection(

def restore_collection(
learning_package_id: LearningPackage.ID,
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()
Expand All @@ -126,7 +128,7 @@ def restore_collection(

def add_to_collection(
learning_package_id: LearningPackage.ID,
key: str,
collection_code: str,
entities_qset: QuerySet[PublishableEntity],
created_by: int | None = None,
) -> Collection:
Expand All @@ -146,10 +148,10 @@ def add_to_collection(
if invalid_entity:
raise ValidationError(
f"Cannot add entity {invalid_entity.id} in learning package {invalid_entity.learning_package_id} "
f"to collection {key} in learning package {learning_package_id}."
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},
Expand All @@ -162,7 +164,7 @@ def add_to_collection(

def remove_from_collection(
learning_package_id: LearningPackage.ID,
key: str,
collection_code: str,
entities_qset: QuerySet[PublishableEntity],
) -> Collection:
"""
Expand All @@ -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)
Expand All @@ -198,7 +200,7 @@ def get_entity_collections(learning_package_id: LearningPackage.ID, entity_key:

def get_collection_entities(
learning_package_id: LearningPackage.ID,
collection_key: str,
collection_code: str,
) -> QuerySet[PublishableEntity]:
"""
Returns a QuerySet of PublishableEntities in a Collection.
Expand All @@ -207,7 +209,7 @@ def get_collection_entities(
"""
return PublishableEntity.objects.filter(
learning_package_id=learning_package_id,
collections__key=collection_key,
collections__collection_code=collection_code,
).order_by("pk")


Expand Down
22 changes: 12 additions & 10 deletions src/openedx_content/applets/collections/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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, code_field_check
from openedx_django_lib.validators import validate_utc_datetime

from ..publishing.models import LearningPackage, PublishableEntity
Expand All @@ -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):
Expand All @@ -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,
Expand Down Expand Up @@ -170,14 +171,15 @@ 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",
),
code_field_check("collection_code", name="oel_coll_collection_code_regex"),
]
indexes = [
models.Index(
Expand All @@ -196,7 +198,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):
Expand Down
4 changes: 2 additions & 2 deletions src/openedx_content/applets/components/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ def get_components( # pylint: disable=too-many-positional-arguments

def get_collection_components(
learning_package_id: LearningPackage.ID,
collection_key: str,
collection_code: str,
) -> QuerySet[Component]:
"""
Returns a QuerySet of Components relating to the PublishableEntities in a Collection.
Expand All @@ -445,7 +445,7 @@ 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')


Expand Down
1 change: 1 addition & 0 deletions src/openedx_content/applets/units/models.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""
Models that implement units
"""
from __future__ import annotations
Comment thread
kdmccormick marked this conversation as resolved.

from typing import NewType, cast, override

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
"""
Rename Collection.key -> Collection.collection_code and change from key_field to code_field.
"""
import re

import django.core.validators
import django.db.models.lookups
from django.conf import settings
from django.db import migrations, models

import openedx_django_lib.fields


class Migration(migrations.Migration):

dependencies = [
('openedx_content', '0007_publishlogrecord_direct'),
migrations.swappable_dependency(settings.AUTH_USER_MODEL),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be another Claude artifact. This migration does not actually depend on the AUTH_USER_MODEL.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I presume we only need it for migrations which touch an FK to User?

Copy link
Copy Markdown
Contributor

@bradenmacdonald bradenmacdonald Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I looked and every migration in core that has this dependency has a reference to settings. AUTH_USER_MODEL elsewhere in the list of changes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted on #322 for followup

]

operations = [
# Drop old constraint (references the old field name).
migrations.RemoveConstraint(
model_name='collection',
name='oel_coll_uniq_lp_key',
),
# Rename the column.
migrations.RenameField(
model_name='collection',
old_name='key',
new_name='collection_code',
),
# Change from key_field (max_length=500, no validator) to code_field
# (max_length=255, with regex validator).
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',
),
],
),
),
# Re-add uniqueness constraint with the new field name.
migrations.AddConstraint(
model_name='collection',
constraint=models.UniqueConstraint(
fields=('learning_package', 'collection_code'),
name='oel_coll_uniq_lp_key',
),
),
# DB-level regex check constraint.
migrations.AddConstraint(
model_name='collection',
constraint=models.CheckConstraint(
condition=django.db.models.lookups.Regex(
models.F('collection_code'),
'^[a-zA-Z0-9_.-]+\\Z',
),
name='oel_coll_collection_code_regex',
violation_error_message=(
'Enter a valid "code name" consisting of letters, numbers,'
' underscores, hyphens, or periods.'
),
),
),
]
Loading