-
Notifications
You must be signed in to change notification settings - Fork 23
feat!: Add Container.container_code field #545
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: kdmccormick/keys-component
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,8 +11,9 @@ | |
| from django.db import models | ||
| from typing_extensions import deprecated | ||
|
|
||
| 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, | ||
|
|
@@ -171,6 +172,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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very much non-blocking: I wish we had some way to enforce the integrity of these denormalized relationships. One idea could be in the future to write a system check that inspects the database and verifies everything. System checks which depend on the database do not normally run on startup (for performance) but can by run manually with the |
||
|
|
||
| # The type of the container. Cannot be changed once the container is created. | ||
| container_type = models.ForeignKey( | ||
| ContainerType, | ||
|
|
@@ -179,6 +186,11 @@ 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() | ||
|
|
||
| @property | ||
| def id(self) -> ID: | ||
| return cast(Container.ID, self.publishable_entity_id) | ||
|
|
@@ -194,6 +206,14 @@ def pk(self): | |
| # override this with a deprecated marker, so it shows a warning in developer's IDEs like VS Code. | ||
| return self.id | ||
|
|
||
| class Meta: | ||
| constraints = [ | ||
| models.UniqueConstraint( | ||
| fields=["learning_package", "container_code"], | ||
| name="oel_container_uniq_lp_cc", | ||
| ), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optional: Suggest adding code constraint here too. |
||
| ] | ||
|
|
||
| @classmethod | ||
| def validate_entity(cls, entity: PublishableEntity) -> None: | ||
| """ | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -33,6 +33,7 @@ def create_unit_and_version( | |||||||||||
| learning_package_id: LearningPackage.ID, | ||||||||||||
| key: str, | ||||||||||||
| *, | ||||||||||||
| container_code: str, | ||||||||||||
|
Comment on lines
34
to
+36
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This would make more sense to me, and then pass But if this is definitely just a temporary thing until the next PR merges, no problem. |
||||||||||||
| title: str, | ||||||||||||
| components: Iterable[Component | ComponentVersion] | None = None, | ||||||||||||
| created: datetime, | ||||||||||||
|
|
@@ -49,6 +50,7 @@ def create_unit_and_version( | |||||||||||
| unit, uv = containers_api.create_container_and_version( | ||||||||||||
| learning_package_id, | ||||||||||||
| key=key, | ||||||||||||
| container_code=container_code, | ||||||||||||
| title=title, | ||||||||||||
| entities=components, | ||||||||||||
| created=created, | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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"]) | ||
|
Comment on lines
+18
to
+22
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Comment, no action needed] It's really unfortunate that we can't use |
||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ("openedx_content", "0009_rename_component_local_key_to_component_code"), | ||
| ] | ||
|
|
||
| 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, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not necessary to explicitly add
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ormsbee oof, thank you for catching that. All of collection_code, container_code, and component_code are missing |
||
| 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", | ||
| ), | ||
| ), | ||
| ] | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that this has
keyandcontainer_codeis just from how the PRs are split up right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'm not sure why we have them as separate arguments here instead of just using
container_code = keyand then later renaming thekeyarg tocontainer_code, but it really doesn't matter if it's just for this PR and it all get sorted out in the end.