diff --git a/olx_importer/management/commands/load_components.py b/olx_importer/management/commands/load_components.py index 6b8d2e42..575f01ed 100644 --- a/olx_importer/management/commands/load_components.py +++ b/olx_importer/management/commands/load_components.py @@ -158,7 +158,7 @@ def import_block_type(self, block_type, now): # , publish_log_entry): _component, component_version = components_api.create_component_and_version( self.learning_package.id, namespace=namespace, - type=block_type, + type_name=block_type, local_key=local_key, title=display_name, created=now, diff --git a/openedx_learning/core/components/admin.py b/openedx_learning/core/components/admin.py index c5fb9b16..1e84391b 100644 --- a/openedx_learning/core/components/admin.py +++ b/openedx_learning/core/components/admin.py @@ -35,16 +35,15 @@ class ComponentAdmin(ReadOnlyModelAdmin): """ Django admin configuration for Component """ - list_display = ("key", "uuid", "namespace", "type", "created") + list_display = ("key", "uuid", "component_type", "created") readonly_fields = [ "learning_package", "uuid", - "namespace", - "type", + "component_type", "key", "created", ] - list_filter = ("type", "learning_package") + list_filter = ("component_type", "learning_package") search_fields = ["publishable_entity__uuid", "publishable_entity__key"] inlines = [ComponentVersionInline] diff --git a/openedx_learning/core/components/api.py b/openedx_learning/core/components/api.py index 7891ed37..2bec7b34 100644 --- a/openedx_learning/core/components/api.py +++ b/openedx_learning/core/components/api.py @@ -18,15 +18,28 @@ from django.db.models import Q, QuerySet from django.db.transaction import atomic +from ...lib.cache import lru_cache from ..publishing import api as publishing_api -from .models import Component, ComponentVersion, ComponentVersionRawContent +from .models import Component, ComponentType, ComponentVersion, ComponentVersionRawContent + + +@lru_cache(maxsize=128) +def get_or_create_component_type_id(namespace: str, name: str) -> int: + """ + Get the ID of a ComponentType, and create if missing. + """ + component_type, _created = ComponentType.objects.get_or_create( + namespace=namespace, + name=name, + ) + return component_type.id def create_component( learning_package_id: int, /, namespace: str, - type: str, # pylint: disable=redefined-builtin + type_name: str, local_key: str, created: datetime, created_by: int | None, @@ -34,7 +47,7 @@ def create_component( """ Create a new Component (an entity like a Problem or Video) """ - key = f"{namespace}:{type}@{local_key}" + key = f"{namespace}:{type_name}@{local_key}" with atomic(): publishable_entity = publishing_api.create_publishable_entity( learning_package_id, key, created, created_by @@ -42,8 +55,7 @@ def create_component( component = Component.objects.create( publishable_entity=publishable_entity, learning_package_id=learning_package_id, - namespace=namespace, - type=type, + component_type_id=get_or_create_component_type_id(namespace, type_name), local_key=local_key, ) return component @@ -163,7 +175,7 @@ def create_component_and_version( learning_package_id: int, /, namespace: str, - type: str, # pylint: disable=redefined-builtin + type_name: str, local_key: str, title: str, created: datetime, @@ -174,7 +186,7 @@ def create_component_and_version( """ with atomic(): component = create_component( - learning_package_id, namespace, type, local_key, created, created_by + learning_package_id, namespace, type_name, local_key, created, created_by ) component_version = create_component_version( component.pk, @@ -199,7 +211,7 @@ def get_component_by_key( learning_package_id: int, /, namespace: str, - type: str, # pylint: disable=redefined-builtin + type_name: str, local_key: str, ) -> Component: """ @@ -208,8 +220,8 @@ def get_component_by_key( return Component.with_publishing_relations \ .get( learning_package_id=learning_package_id, - namespace=namespace, - type=type, + component_type__namespace=namespace, + component_type__name=type_name, local_key=local_key, ) @@ -218,7 +230,7 @@ def component_exists_by_key( learning_package_id: int, /, namespace: str, - type: str, # pylint: disable=redefined-builtin + type_name: str, local_key: str ) -> bool: """ @@ -228,10 +240,10 @@ def component_exists_by_key( no current Draft version for it), or if it's been unpublished. """ try: - _component = Component.objects.only('pk').get( + _component = Component.objects.only('pk', 'component_type').get( learning_package_id=learning_package_id, - namespace=namespace, - type=type, + component_type__namespace=namespace, + component_type__name=type_name, local_key=local_key, ) return True @@ -245,7 +257,7 @@ def get_components( draft: bool | None = None, published: bool | None = None, namespace: str | None = None, - types: list[str] | None = None, + type_names: list[str] | None = None, draft_title: str | None = None, published_title: str | None = None, ) -> QuerySet[Component]: @@ -265,9 +277,9 @@ def get_components( if published is not None: qset = qset.filter(publishable_entity__published__version__isnull=not published) if namespace is not None: - qset = qset.filter(namespace=namespace) - if types is not None: - qset = qset.filter(type__in=types) + qset = qset.filter(component_type__namespace=namespace) + if type_names is not None: + qset = qset.filter(component_type__name__in=type_names) if draft_title is not None: qset = qset.filter( publishable_entity__draft__version__title__icontains=draft_title diff --git a/openedx_learning/core/components/migrations/0001_initial.py b/openedx_learning/core/components/migrations/0001_initial.py index fdfb5e40..b72d1aee 100644 --- a/openedx_learning/core/components/migrations/0001_initial.py +++ b/openedx_learning/core/components/migrations/0001_initial.py @@ -1,4 +1,4 @@ -# Generated by Django 3.2.23 on 2024-01-22 00:38 +# Generated by Django 3.2.23 on 2024-01-31 05:34 import uuid @@ -13,8 +13,8 @@ class Migration(migrations.Migration): initial = True dependencies = [ - ('oel_contents', '0001_initial'), ('oel_publishing', '0001_initial'), + ('oel_contents', '0001_initial'), ] operations = [ @@ -22,16 +22,21 @@ class Migration(migrations.Migration): name='Component', fields=[ ('publishable_entity', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, serialize=False, to='oel_publishing.publishableentity')), - ('namespace', openedx_learning.lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, max_length=100)), - ('type', openedx_learning.lib.fields.MultiCollationCharField(blank=True, db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, max_length=100)), ('local_key', openedx_learning.lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, max_length=500)), - ('learning_package', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='oel_publishing.learningpackage')), ], options={ 'verbose_name': 'Component', 'verbose_name_plural': 'Components', }, ), + migrations.CreateModel( + name='ComponentType', + fields=[ + ('id', models.AutoField(primary_key=True, serialize=False)), + ('namespace', openedx_learning.lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, max_length=100)), + ('name', openedx_learning.lib.fields.MultiCollationCharField(blank=True, db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, max_length=100)), + ], + ), migrations.CreateModel( name='ComponentVersion', fields=[ @@ -59,6 +64,16 @@ class Migration(migrations.Migration): name='raw_contents', field=models.ManyToManyField(related_name='component_versions', through='oel_components.ComponentVersionRawContent', to='oel_contents.RawContent'), ), + migrations.AddField( + model_name='component', + name='component_type', + field=models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, to='oel_components.componenttype'), + ), + migrations.AddField( + model_name='component', + name='learning_package', + field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='oel_publishing.learningpackage'), + ), migrations.AddIndex( model_name='componentversionrawcontent', index=models.Index(fields=['raw_content', 'component_version'], name='oel_cvrawcontent_c_cv'), @@ -73,10 +88,10 @@ class Migration(migrations.Migration): ), migrations.AddIndex( model_name='component', - index=models.Index(fields=['learning_package', 'namespace', 'type', 'local_key'], name='oel_component_idx_lc_ns_t_lk'), + index=models.Index(fields=['component_type', 'local_key'], name='oel_component_idx_ct_lk'), ), migrations.AddConstraint( model_name='component', - constraint=models.UniqueConstraint(fields=('learning_package', 'namespace', 'type', 'local_key'), name='oel_component_uniq_lc_ns_t_lk'), + constraint=models.UniqueConstraint(fields=('learning_package', 'component_type', 'local_key'), name='oel_component_uniq_lc_ct_lk'), ), ] diff --git a/openedx_learning/core/components/models.py b/openedx_learning/core/components/models.py index b7eba42d..920b3efb 100644 --- a/openedx_learning/core/components/models.py +++ b/openedx_learning/core/components/models.py @@ -28,6 +28,45 @@ from ..publishing.models import LearningPackage +class ComponentType(models.Model): + """ + Normalized representation of a type of Component. + + The only namespace being used initially will be 'xblock.v1', but we will + probably add a few others over time, such as a component type to represent + packages of files for things like Files and Uploads or python_lib.zip files. + + Make a ForeignKey against this table if you have to set policy based on the + type of Components–e.g. marking certain types of XBlocks as approved vs. + experimental for use in libraries. + """ + id = models.AutoField(primary_key=True) + + # namespace and name work together to help figure out what Component needs + # to handle this data. A namespace is *required*. The namespace for XBlocks + # is "xblock.v1" (to match the setup.py entrypoint naming scheme). + namespace = case_sensitive_char_field(max_length=100, blank=False) + + # name is a way to help sub-divide namespace if that's convenient. This + # field cannot be null, but it can be blank if it's not necessary. For an + # XBlock, this corresponds to tag, e.g. "video". It's also the block_type in + # the UsageKey. + name = case_sensitive_char_field(max_length=100, blank=True) + + constraints = [ + models.UniqueConstraint( + fields=[ + "namespace", + "name", + ], + name="oel_component_type_uniq_ns_n", + ), + ] + + def __str__(self): + return f"{self.namespace}:{self.name}" + + class Component(PublishableEntityMixin): # type: ignore[django-manager-missing] """ This represents any Component that has ever existed in a LearningPackage. @@ -63,7 +102,7 @@ class Component(PublishableEntityMixin): # type: ignore[django-manager-missing] ----------------- The ``key`` field on Component's ``publishable_entity`` is dervied from the - ``(namespace, type, local_key)`` fields in this model. We don't support + ``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. How build on this model @@ -75,9 +114,12 @@ class Component(PublishableEntityMixin): # type: ignore[django-manager-missing] # Tell mypy what type our objects manager has. # It's actually PublishableEntityMixinManager, but that has the exact same # interface as the base manager class. - objects: models.Manager[Component] + objects: models.Manager[Component] = WithRelationsManager( + 'component_type' + ) - with_publishing_relations = WithRelationsManager( + with_publishing_relations: models.Manager[Component] = WithRelationsManager( + 'component_type', 'publishable_entity', 'publishable_entity__draft__version', 'publishable_entity__draft__version__componentversion', @@ -93,53 +135,43 @@ class Component(PublishableEntityMixin): # type: ignore[django-manager-missing] # columns from different tables). learning_package = models.ForeignKey(LearningPackage, on_delete=models.CASCADE) - # namespace and type work together to help figure out what Component needs - # to handle this data. A namespace is *required*. The namespace for XBlocks - # is "xblock.v1" (to match the setup.py entrypoint naming scheme). - namespace = case_sensitive_char_field(max_length=100, blank=False) + # What kind of Component are we? This will usually represent a specific + # XBlock block_type, but we want it to be more flexible in the long term. + component_type = models.ForeignKey(ComponentType, on_delete=models.PROTECT) - # type is a way to help sub-divide namespace if that's convenient. This - # field cannot be null, but it can be blank if it's not necessary. For an - # XBlock, type corresponds to tag, e.g. "video". It's also the block_type in - # the UsageKey. - type = case_sensitive_char_field(max_length=100, blank=True) - - # local_key is an identifier that is local to the (namespace, type). The - # publishable.key should be calculated as a combination of (namespace, type, - # local_key). + # 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() class Meta: constraints = [ - # The combination of (namespace, type, local_key) is unique within + # The combination of (component_type, local_key) is unique within # a given LearningPackage. Note that this means it is possible to - # have two Components that have the exact same local_key. An XBlock - # would be modeled as namespace="xblock.v1" with the type as the - # block_type, so the local_key would only be the block_id (the - # very last part of the UsageKey). + # 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". models.UniqueConstraint( fields=[ "learning_package", - "namespace", - "type", + "component_type", "local_key", ], - name="oel_component_uniq_lc_ns_t_lk", + name="oel_component_uniq_lc_ct_lk", ), ] indexes = [ - # Global Namespace/Type/Local-Key Index: + # Global Component-Type/Local-Key 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=[ - "learning_package", - "namespace", - "type", + "component_type", "local_key", ], - name="oel_component_idx_lc_ns_t_lk", + name="oel_component_idx_ct_lk", ), ] @@ -148,7 +180,7 @@ class Meta: verbose_name_plural = "Components" def __str__(self): - return f"{self.namespace}:{self.type}:{self.local_key}" + return f"{self.component_type.namespace}:{self.component_type.name}:{self.local_key}" class ComponentVersion(PublishableEntityVersionMixin): diff --git a/openedx_learning/core/contents/api.py b/openedx_learning/core/contents/api.py index 923dc119..0b27690a 100644 --- a/openedx_learning/core/contents/api.py +++ b/openedx_learning/core/contents/api.py @@ -12,9 +12,8 @@ from django.core.files.base import ContentFile from django.db.transaction import atomic -from openedx_learning.lib.cache import lru_cache -from openedx_learning.lib.fields import create_hash_digest - +from ...lib.cache import lru_cache +from ...lib.fields import create_hash_digest from .models import MediaType, RawContent, TextContent @@ -33,7 +32,7 @@ def create_raw_content( raw_content = RawContent.objects.create( learning_package_id=learning_package_id, - media_type_id=get_media_type_id(mime_type), + media_type_id=get_or_create_media_type_id(mime_type), hash_digest=hash_digest, size=len(data_bytes), created=created, @@ -58,7 +57,7 @@ def create_text_from_raw_content(raw_content: RawContent, encoding="utf-8-sig") @lru_cache(maxsize=128) -def get_media_type_id(mime_type: str) -> int: +def get_or_create_media_type_id(mime_type: str) -> int: """ Return the MediaType.id for the desired mime_type string. diff --git a/tests/openedx_learning/core/components/test_api.py b/tests/openedx_learning/core/components/test_api.py index 69e50364..b1d46929 100644 --- a/tests/openedx_learning/core/components/test_api.py +++ b/tests/openedx_learning/core/components/test_api.py @@ -46,7 +46,7 @@ def test_component_num_queries(self) -> None: component, _version = components_api.create_component_and_version( self.learning_package.id, namespace="xblock.v1", - type="problem", + type_name="problem", local_key="Query Counting", title="Querying Counting Problem", created=self.now, @@ -97,7 +97,7 @@ def setUpTestData(cls) -> None: cls.published_problem, _version = components_api.create_component_and_version( cls.learning_package.id, namespace="xblock.v2", - type="problem", + type_name="problem", local_key="published_problem", title="Published Problem", created=cls.now, @@ -106,7 +106,7 @@ def setUpTestData(cls) -> None: cls.published_html, _version = components_api.create_component_and_version( cls.learning_package.id, namespace="xblock.v1", - type="html", + type_name="html", local_key="published_html", title="Published HTML", created=cls.now, @@ -121,7 +121,7 @@ def setUpTestData(cls) -> None: cls.unpublished_problem, _version = components_api.create_component_and_version( cls.learning_package.id, namespace="xblock.v2", - type="problem", + type_name="problem", local_key="unpublished_problem", title="Unpublished Problem", created=cls.now, @@ -130,7 +130,7 @@ def setUpTestData(cls) -> None: cls.unpublished_html, _version = components_api.create_component_and_version( cls.learning_package.id, namespace="xblock.v1", - type="html", + type_name="html", local_key="unpublished_html", title="Unpublished HTML", created=cls.now, @@ -142,7 +142,7 @@ def setUpTestData(cls) -> None: cls.deleted_video, _version = components_api.create_component_and_version( cls.learning_package.id, namespace="xblock.v1", - type="html", + type_name="html", local_key="deleted_video", title="Deleted Video", created=cls.now, @@ -230,7 +230,7 @@ def test_types_filter(self): """ html_and_video_components = components_api.get_components( self.learning_package.id, - types=['html', 'video'] + type_names=['html', 'video'] ) assert list(html_and_video_components) == [ self.published_html, @@ -300,7 +300,7 @@ def setUpTestData(cls) -> None: cls.problem = components_api.create_component( cls.learning_package.id, namespace='xblock.v1', - type='problem', + type_name='problem', local_key='my_component', created=cls.now, created_by=None, @@ -308,7 +308,7 @@ def setUpTestData(cls) -> None: cls.html = components_api.create_component( cls.learning_package.id, namespace='xblock.v1', - type='html', + type_name='html', local_key='my_component', created=cls.now, created_by=None, @@ -323,14 +323,14 @@ def test_get_by_key(self): assert self.html == components_api.get_component_by_key( self.learning_package.id, namespace='xblock.v1', - type='html', + type_name='html', local_key='my_component', ) with self.assertRaises(ObjectDoesNotExist): components_api.get_component_by_key( self.learning_package.id, namespace='xblock.v1', - type='video', # 'video' doesn't match anything we have + type_name='video', # 'video' doesn't match anything we have local_key='my_component', ) @@ -338,13 +338,13 @@ def test_exists_by_key(self): assert components_api.component_exists_by_key( self.learning_package.id, namespace='xblock.v1', - type='problem', + type_name='problem', local_key='my_component', ) assert not components_api.component_exists_by_key( self.learning_package.id, namespace='xblock.v1', - type='problem', + type_name='problem', local_key='not_my_component', ) @@ -367,7 +367,7 @@ def setUpTestData(cls) -> None: cls.problem = components_api.create_component( cls.learning_package.id, namespace='xblock.v1', - type='problem', + type_name='problem', local_key='my_component', created=cls.now, created_by=None, diff --git a/tests/openedx_learning/core/components/test_models.py b/tests/openedx_learning/core/components/test_models.py index 9342e232..d4bb20e3 100644 --- a/tests/openedx_learning/core/components/test_models.py +++ b/tests/openedx_learning/core/components/test_models.py @@ -27,7 +27,7 @@ def test_latest_version(self) -> None: component, component_version = create_component_and_version( self.learning_package.id, namespace="xblock.v1", - type="problem", + type_name="problem", local_key="monty_hall", title="Monty Hall Problem", created=self.now, diff --git a/tests/openedx_learning/core/contents/test_media_types.py b/tests/openedx_learning/core/contents/test_media_types.py index c880a008..96390cb4 100644 --- a/tests/openedx_learning/core/contents/test_media_types.py +++ b/tests/openedx_learning/core/contents/test_media_types.py @@ -11,18 +11,18 @@ class MediaTypeCachingTestCase(TestCase): """ def test_media_query_caching(self): """Test MediaType queries auto-create and caching.""" - assert contents_api.get_media_type_id.cache_info().currsize == 0 + assert contents_api.get_or_create_media_type_id.cache_info().currsize == 0 mime_type_str = "application/vnd.openedx.xblock.v1.problem+xml" - media_type_id = contents_api.get_media_type_id(mime_type_str) + media_type_id = contents_api.get_or_create_media_type_id(mime_type_str) # Now it should be loaded in the cache - assert contents_api.get_media_type_id.cache_info().currsize == 1 + assert contents_api.get_or_create_media_type_id.cache_info().currsize == 1 # Second call pulls from cache instead of the database with self.assertNumQueries(0): # Should also return the same thing it did last time. - assert media_type_id == contents_api.get_media_type_id(mime_type_str) + assert media_type_id == contents_api.get_or_create_media_type_id(mime_type_str) def test_media_query_caching_reset(self): """ @@ -31,4 +31,4 @@ def test_media_query_caching_reset(self): This test method's *must* execute after test_media_query_caching to be meaningful (they execute in string sort order). """ - assert contents_api.get_media_type_id.cache_info().currsize == 0 + assert contents_api.get_or_create_media_type_id.cache_info().currsize == 0