Skip to content

feat!: Collection.key -> Collection.collection_code#542

Merged
kdmccormick merged 10 commits intomainfrom
kdmccormick/keys-collection
Apr 17, 2026
Merged

feat!: Collection.key -> Collection.collection_code#542
kdmccormick merged 10 commits intomainfrom
kdmccormick/keys-collection

Conversation

@kdmccormick
Copy link
Copy Markdown
Member

@kdmccormick kdmccormick commented Apr 15, 2026

Description

Also, standardize internal usage of collection_key to collection_code.
This helps clarify that Collection.key is not an OpaqueKey, but is rather
a local slug, which can be combined with other identifiers to form a fully-
qualified LibraryCollectionKey instance.

BREAKING CHANGE: Collection.key has been renamed to Collection.collection_code.

BREAKING CHANGE: Collection.collection_code now validates that its contents
matches '[A-Za-z0-9-_.]+'. This was already effectively true, because
LibraryCollectionKey can only be built with slug-like parts, but we now
we explicitly raise ValiationError from create_collection.

Backup-restore still write and reads the collection_code to/from TOML files as key for backwards compatibility. This may change in a future "v2" restore format.

Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com

Part of:

Full series of PRs:

  1. feat!: Collection.key -> Collection.collection_code #542
  2. feat!: Component.local_key -> Component.component_code #544
  3. feat!: Add Container.container_code field #545
  4. feat!: Package and Entity keys are now opaque refs #546
  5. feat!: ComponentVersionMedia.key -> ComponentVersionMedia.path #547

Testing, AI Usage, and Merge Considerations

See #322

Comment thread src/openedx_content/applets/components/models.py Outdated
@ormsbee ormsbee assigned ormsbee and unassigned ormsbee Apr 15, 2026
Comment thread src/openedx_content/applets/units/models.py
Comment thread src/openedx_content/migrations/0008_rename_collection_key_to_code_in_db.py Outdated
Comment thread src/openedx_content/applets/backup_restore/toml.py Outdated
Comment thread src/openedx_django_lib/fields.py
Comment thread src/openedx_django_lib/fields.py Outdated
@kdmccormick kdmccormick force-pushed the kdmccormick/keys-collection branch from 7eb00fd to b3daece Compare April 16, 2026 15:27
kdmccormick and others added 8 commits April 16, 2026 12:49
Also, standardize internal usage of collection_key to collection_code.
This helps clarify that Collection.key is *not* an OpaqueKey, but is rather
a local slug, which can be combined with other identifiers to form a fully-
qualified LibraryCollectionKey instance.

BREAKING CHANGE: Collection.key has been renamed to Collection.collection_code.

BREAKING CHANGE: Collection.collection_code now validates that its contents
matches '[A-Za-z0-9\-\_\.]+'.  This was already effectively true, because
LibraryCollectionKey can only be built with slug-like parts, but we now
we explicitly raise ValiationError from create_collection.

Backup now writes both 'key' and 'collection_code' to collection TOML files,
so older software (which only knows 'key') can still read new archives.
Restore accepts either field, preferring 'collection_code' and falling back
to the legacy 'key'.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Part of: #322
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…llection_code'

The backup_restore archive format stays unchanged — collections are
still serialized with "key". A comment notes the divergence from the
model field name (collection_code) and that a future v2 format may
align them.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds a code_field_check(field_name, name=...) companion to code_field()
that returns a CheckConstraint using Django's Regex lookup. This enforces
the code regex at the database level (not just via validators), and
Django also runs it as a Python-level validator automatically.

Applied to Collection.collection_code as the first usage.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@kdmccormick kdmccormick force-pushed the kdmccormick/keys-collection branch from b3daece to d55764c Compare April 16, 2026 17:04
kdmccormick and others added 2 commits April 16, 2026 19:05
…zer.validate()

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@kdmccormick kdmccormick marked this pull request as ready for review April 17, 2026 17:49
Copy link
Copy Markdown
Contributor

@ormsbee ormsbee left a comment

Choose a reason for hiding this comment

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

Great stuff! Thank you!

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

Copy link
Copy Markdown
Contributor

@bradenmacdonald bradenmacdonald left a comment

Choose a reason for hiding this comment

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

I didn't test this but the code changes look great. Thanks!


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

@kdmccormick kdmccormick merged commit 2d5a17b into main Apr 17, 2026
6 checks passed
@kdmccormick kdmccormick deleted the kdmccormick/keys-collection branch April 17, 2026 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants