From c23f373db478c16b81bfa3033044ae5e96c89c64 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 8 Jan 2024 17:34:05 -0500 Subject: [PATCH 01/23] feat: add features to support v2 libraries This commit adds a number of features needed to accomodate content libraries. It also tunes a number of queries to elminated n+1 fetch issues when tested with libraries, by using select_related more liberally, and following cached relationships instead of re-fetching things from the database. As part of this general tuning, PublishableEntityVersion.version_num was reduced to a 4-byte PositiveIntegerField instead of using the 8-byte PositiveBigIntegerField. This should save us a bit of space with our indexes while still allowing for over two billion versions for any given PublishableEntity. --- openedx_learning/core/components/api.py | 155 +++++++++++- .../components/migrations/0001_initial.py | 10 +- openedx_learning/core/components/models.py | 18 +- .../core/contents/migrations/0001_initial.py | 9 +- openedx_learning/core/contents/models.py | 10 +- openedx_learning/core/publishing/admin.py | 4 +- openedx_learning/core/publishing/api.py | 126 +++++++++- .../publishing/migrations/0001_initial.py | 21 +- .../migrations/0002_alter_fk_on_delete.py | 30 --- .../core/publishing/model_mixins.py | 136 +++++++--- openedx_learning/core/publishing/models.py | 29 ++- openedx_learning/lib/managers.py | 37 +++ .../core/components/test_api.py | 237 ++++++++++++++++++ .../core/components/test_models.py | 9 +- .../core/publishing/test_api.py | 14 +- 15 files changed, 728 insertions(+), 117 deletions(-) delete mode 100644 openedx_learning/core/publishing/migrations/0002_alter_fk_on_delete.py create mode 100644 openedx_learning/lib/managers.py create mode 100644 tests/openedx_learning/core/components/test_api.py diff --git a/openedx_learning/core/components/api.py b/openedx_learning/core/components/api.py index 4275c5d12..62b1556ae 100644 --- a/openedx_learning/core/components/api.py +++ b/openedx_learning/core/components/api.py @@ -15,10 +15,10 @@ from datetime import datetime from pathlib import Path -from django.db.models import Q +from django.db.models import Q, QuerySet from django.db.transaction import atomic -from ..publishing.api import create_publishable_entity, create_publishable_entity_version +from ..publishing import api as publishing_api from .models import Component, ComponentVersion, ComponentVersionRawContent @@ -35,7 +35,7 @@ def create_component( """ key = f"{namespace}:{type}@{local_key}" with atomic(): - publishable_entity = create_publishable_entity( + publishable_entity = publishing_api.create_publishable_entity( learning_package_id, key, created, created_by ) component = Component.objects.create( @@ -59,7 +59,7 @@ def create_component_version( Create a new ComponentVersion """ with atomic(): - publishable_entity_version = create_publishable_entity_version( + publishable_entity_version = publishing_api.create_publishable_entity_version( entity_id=component_pk, version_num=version_num, title=title, @@ -73,6 +73,72 @@ def create_component_version( return component_version +def create_next_version( + component_pk: int, + title: str, + content_to_replace: dict[str: int], + created: datetime, + created_by: int | None = None, +): + """ + Create a new ComponentVersion based on the most recent version. + + A very common pattern for making a new ComponentVersion is going to be "make + it just like the last version, except changing these one or two things". + Before calling this, you should create any new contents via the contents + API, since ``content_to_replace`` needs RawContent IDs for the values. + + TODO: Have to add learning_downloadable info to this when it comes time to + support static asset download. + """ + # This needs to grab the highest version_num for this Publishable Entity. + # This will often be the Draft version, but not always. For instance, if + # an entity was soft-deleted, the draft would be None, but the version_num + # should pick up from the last edited version. Likewise, a Draft might get + # reverted to an earlier version, but we want the latest version_num when + # creating the next version. + component = Component.objects.get(pk=component_pk) + last_version = component.versioning.latest + if last_version is None: + next_version_num = 1 + else: + next_version_num = last_version.version_num + 1 + + with atomic(): + publishable_entity_version = publishing_api.create_publishable_entity_version( + entity_id=component_pk, + version_num=next_version_num, + title=title, + created=created, + created_by=created_by, + ) + component_version = ComponentVersion.objects.create( + publishable_entity_version=publishable_entity_version, + component_id=component_pk, + ) + # First copy the new stuff over... + for key, raw_content_pk in content_to_replace.items(): + ComponentVersionRawContent.objects.create( + raw_content_id=raw_content_pk, + component_version=component_version, + key=key, + learner_downloadable=False, + ) + # Now copy any old associations that existed, as long as they don't + # conflict with the new stuff. + last_version_content_mapping = ComponentVersionRawContent.objects \ + .filter(component_version=component_version) \ + .all() + for cvrc in last_version_content_mapping: + if cvrc.key not in content_to_replace: + ComponentVersionRawContent.objects.create( + raw_content_id=cvrc.raw_content_pk, + component_version=component_version, + key=cvrc.key, + learner_downloadable=cvrc.learner_downloadable, + ) + + def create_component_and_version( learning_package_id: int, namespace: str, @@ -80,7 +146,7 @@ def create_component_and_version( local_key: str, title: str, created: datetime, - created_by: int | None, + created_by: int | None = None, ): """ Create a Component and associated ComponentVersion atomically @@ -98,10 +164,84 @@ def create_component_and_version( ) return (component, component_version) +def get_component(component_pk: int) -> Component: + """ + Get Component by its primary key. + + This is the same as the PublishableEntity's ID primary key. + """ + return Component.with_publishing_relations.get(pk=component_pk) + +def get_component_by_key( + learning_package_id: int, + namespace: str, + type: str, # pylint: disable=redefined-builtin + local_key: str, +): + """ + Get Componet by it unique namespace/type/local_key tuple. + """ + return Component.with_publishing_relations \ + .get( + learning_package_id=learning_package_id, + namespace=namespace, + type=type, + local_key=local_key, + ) -def get_component_by_pk(component_pk: int) -> Component: - return Component.objects.get(pk=component_pk) +def component_exists_by_key( + learning_package_id: int, + namespace: str, + type: str, # pylint: disable=redefined-builtin + local_key: str +): + """ + Return True/False for whether a Component exists. + + Note that a Component still exists even if it's been soft-deleted (there's + no current Draft version for it), or if it's been unpublished. + """ + try: + _component = Component.objects.only('pk').get( + learning_package_id=learning_package_id, + namespace=namespace, + type=type, + local_key=local_key, + ) + return True + except Component.DoesNotExist: + return False + +def get_components( + learning_package_id: int, + draft: bool | None = None, + published: bool | None = None, + namespace: str | None = None, + types: list[str] | None = None, + title: str | None = None, +) -> QuerySet: + """ + Fetch a QuerySet of Components for a LearningPackage using various filters. + + This method will pre-load all the relations that we need in order to get + info from the Component's draft and published versions, since we'll be + referencing these a lot. + """ + qset = Component.with_publishing_relations \ + .filter(learning_package_id=learning_package_id) \ + .order_by('pk') + if draft is not None: + qset = qset.filter(publishable_entity__draft__version__isnull = not draft) + 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) + if title is not None: + qset = qset.filter(publishable_entity__draft__version__title__icontains=title) + return qset def get_component_version_content( learning_package_key: str, @@ -118,6 +258,7 @@ def get_component_version_content( return ComponentVersionRawContent.objects.select_related( "raw_content", "raw_content__media_type", + "raw_content__textcontent", "component_version", "component_version__component", "component_version__component__learning_package", diff --git a/openedx_learning/core/components/migrations/0001_initial.py b/openedx_learning/core/components/migrations/0001_initial.py index bbe0e15d8..68a324f36 100644 --- a/openedx_learning/core/components/migrations/0001_initial.py +++ b/openedx_learning/core/components/migrations/0001_initial.py @@ -1,11 +1,9 @@ -# Generated by Django 3.2.23 on 2023-12-04 00:41 +# Generated by Django 3.2.23 on 2024-01-22 00:38 -import uuid - -import django.db.models.deletion from django.db import migrations, models - +import django.db.models.deletion import openedx_learning.lib.fields +import uuid class Migration(migrations.Migration): @@ -13,8 +11,8 @@ class Migration(migrations.Migration): initial = True dependencies = [ - ('oel_publishing', '0002_alter_fk_on_delete'), ('oel_contents', '0001_initial'), + ('oel_publishing', '0001_initial'), ] operations = [ diff --git a/openedx_learning/core/components/models.py b/openedx_learning/core/components/models.py index 2a24beb65..3cf81a687 100644 --- a/openedx_learning/core/components/models.py +++ b/openedx_learning/core/components/models.py @@ -20,7 +20,12 @@ from django.db import models -from openedx_learning.lib.fields import case_sensitive_char_field, immutable_uuid_field, key_field +from openedx_learning.lib.fields import ( + case_sensitive_char_field, + immutable_uuid_field, + key_field, +) +from openedx_learning.lib.managers import WithRelationsManager from ..contents.models import RawContent from ..publishing.model_mixins import PublishableEntityMixin, PublishableEntityVersionMixin @@ -44,6 +49,9 @@ class Component(PublishableEntityMixin): # type: ignore[django-manager-missing] A Component belongs to exactly one LearningPackage. + A Component's is 1:1 with the same primary key values as the + PublishableEntity it uses for draft/publishing operations. + Identifiers ----------- @@ -70,6 +78,14 @@ class Component(PublishableEntityMixin): # type: ignore[django-manager-missing] # interface as the base manager class. objects: models.Manager[Component] + with_publishing_relations = WithRelationsManager( + 'publishable_entity', + 'publishable_entity__draft__version', + 'publishable_entity__draft__version__componentversion', + 'publishable_entity__published__version', + 'publishable_entity__published__version__componentversion', + ) + # 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 diff --git a/openedx_learning/core/contents/migrations/0001_initial.py b/openedx_learning/core/contents/migrations/0001_initial.py index 7dac9a3f7..b7ea865a5 100644 --- a/openedx_learning/core/contents/migrations/0001_initial.py +++ b/openedx_learning/core/contents/migrations/0001_initial.py @@ -1,9 +1,8 @@ -# Generated by Django 3.2.23 on 2023-12-04 00:41 +# Generated by Django 3.2.23 on 2024-01-22 00:38 import django.core.validators -import django.db.models.deletion from django.db import migrations, models - +import django.db.models.deletion import openedx_learning.lib.fields import openedx_learning.lib.validators @@ -13,7 +12,7 @@ class Migration(migrations.Migration): initial = True dependencies = [ - ('oel_publishing', '0002_alter_fk_on_delete'), + ('oel_publishing', '0001_initial'), ] operations = [ @@ -46,7 +45,7 @@ class Migration(migrations.Migration): name='TextContent', fields=[ ('raw_content', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, related_name='text_content', serialize=False, to='oel_contents.rawcontent')), - ('text', openedx_learning.lib.fields.MultiCollationTextField(blank=True, db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, max_length=100000)), + ('text', openedx_learning.lib.fields.MultiCollationTextField(blank=True, db_collations={'mysql': 'utf8mb4_unicode_ci', 'sqlite': 'NOCASE'}, max_length=100000)), ('length', models.PositiveIntegerField()), ], ), diff --git a/openedx_learning/core/contents/models.py b/openedx_learning/core/contents/models.py index e7de73757..d90c2abdc 100644 --- a/openedx_learning/core/contents/models.py +++ b/openedx_learning/core/contents/models.py @@ -254,12 +254,12 @@ class TextContent(models.Model): text = MultiCollationTextField( blank=True, max_length=MAX_TEXT_LENGTH, - # We don't really expect to ever sort by the text column. This is here - # primarily to force the column to be created as utf8mb4 on MySQL. I'm - # using the binary collation because it's a little cheaper/faster. + # We don't really expect to ever sort by the text column, but we may + # want to do case-insensitive searches, so it's useful to have a case + # and accent insensitive collation. db_collations={ - "sqlite": "BINARY", - "mysql": "utf8mb4_bin", + "sqlite": "NOCASE", + "mysql": "utf8mb4_unicode_ci", } ) length = models.PositiveIntegerField(null=False) diff --git a/openedx_learning/core/publishing/admin.py b/openedx_learning/core/publishing/admin.py index 90f72bcc8..a2b5dbde1 100644 --- a/openedx_learning/core/publishing/admin.py +++ b/openedx_learning/core/publishing/admin.py @@ -149,7 +149,9 @@ def get_queryset(self, request): ) def version_num(self, published_obj): - return published_obj.version.version_num + if published_obj.version: + return published_obj.version.version_num + return None def previous(self, published_obj): """ diff --git a/openedx_learning/core/publishing/api.py b/openedx_learning/core/publishing/api.py index ab4099ad2..2a4f87c8a 100644 --- a/openedx_learning/core/publishing/api.py +++ b/openedx_learning/core/publishing/api.py @@ -24,8 +24,22 @@ ) +def get_learning_package(learning_package_id: int) -> LearningPackage: + """ + Get LearningPackage by ID. + """ + return LearningPackage.objects.get(id=learning_package_id) + +def get_learning_package_by_key(key: str) -> LearningPackage: + """ + Get LearningPackage by key. + + Can throw a NotFoundError + """ + return LearningPackage.objects.get(key=key) + def create_learning_package( - key: str, title: str, created: datetime | None = None + key: str, title: str, description: str = "", created: datetime | None = None ) -> LearningPackage: """ Create a new LearningPackage. @@ -42,6 +56,7 @@ def create_learning_package( package = LearningPackage( key=key, title=title, + description=description, created=created, updated=created, ) @@ -50,6 +65,41 @@ def create_learning_package( return package +def update_learning_package( + learning_package_id: int, + key: str | None = None, + title: str | None = None, + description: str | None = None, + updated: datetime | None = None, +) -> LearningPackage: + """ + Make an update to LearningPackage metadata. + + Note that LearningPackage itself is not versioned (only stuff inside it is). + """ + lp = LearningPackage.objects.get(id=learning_package_id) + + # 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]): + return lp + + if key is not None: + lp.key = key + if title is not None: + lp.title = title + if description is not None: + lp.description = description + + # updated is a bit different–we auto-generate it if it's not explicitly + # passed in. + if updated is None: + updated = datetime.now(tz=timezone.utc) + lp.updated = updated + + lp.save() + return lp + def create_publishable_entity( learning_package_id: int, @@ -99,6 +149,11 @@ def create_publishable_entity_version( ) return version +def get_publishable_entity_by_key(learning_package_id, key): + return PublishableEntity.objects.get( + learning_package_id=learning_package_id, + key=key, + ) def learning_package_exists(key: str) -> bool: """ @@ -107,6 +162,37 @@ def learning_package_exists(key: str) -> bool: return LearningPackage.objects.filter(key=key).exists() +def get_last_publish(learning_package_id: int) -> PublishLog | None: + return PublishLog.objects \ + .filter(learning_package_id=learning_package_id) \ + .order_by('-id') \ + .first() + +def get_all_drafts(learning_package_id: int): + return Draft.objects.filter( + entity__learning_package_id=learning_package_id, + version__isnull=False, + ) + +def get_entities_with_unpublished_changes(learning_package_id: int): + return PublishableEntity.objects \ + .filter(learning_package_id=learning_package_id) \ + .exclude(draft__version=F('published__version')) + +def get_entities_with_unpublished_deletes(learning_package_id: int): + """ + Something will become "deleted" if it has a null Draft version but a + not-null Published version. (If both are null, it means it's already been + deleted in a previous publish, or it was never published.) + """ + return PublishableEntity.objects \ + .filter( + learning_package_id=learning_package_id, + draft__version__isnull=True, + ) \ + .exclude(published__version__isnull=True) + + def publish_all_drafts( learning_package_id: int, message="", @@ -118,8 +204,8 @@ def publish_all_drafts( """ draft_qset = ( Draft.objects.select_related("entity__published") - .filter(entity__learning_package_id=learning_package_id) - .exclude(entity__published__version_id=F("version_id")) + .filter(entity__learning_package_id=learning_package_id) + .exclude(entity__published__version_id=F("version_id")) ) return publish_from_drafts( learning_package_id, draft_qset, message, published_at, published_by @@ -200,7 +286,7 @@ def get_draft_version(publishable_entity_id: int) -> PublishableEntityVersion | return draft.version -def set_draft_version(publishable_entity_id: int, publishable_entity_version_pk: int | None) -> None: +def set_draft_version(publishable_entity_id: int, publishable_entity_version_pk: int) -> None: """ Modify the Draft of a PublishableEntity to be a PublishableEntityVersion. @@ -216,6 +302,38 @@ def set_draft_version(publishable_entity_id: int, publishable_entity_version_pk: draft.save() +def soft_delete_draft(publishable_entity_id: int): + draft = Draft.objects.get(entity_id=publishable_entity_id) + draft.version_id = None + draft.save() + + +def reset_drafts_to_published(learning_package_id: int) -> None: + """ + Reset all Drafts to point to the most recently Published versions. + + This is a way to say "discard my unpublished changes" at the level of an + entire LearningPackage. + """ + # These are all the drafts that are different from the publisehd versions. + draft_qset = ( + Draft.objects.select_related("entity__published") + .filter(entity__learning_package_id=learning_package_id) + .exclude(entity__published__version_id=F("version_id")) + ) + # Note: We can't do an .update with a F() on a joined field in the ORM, so + # we have to loop through the drafts individually to reset them. We can + # rework this into a bulk update or custom SQL if it becomes a performance + # issue. + with atomic(): + for draft in draft_qset.all(): + if hasattr(draft.entity, 'published'): + draft.version_id = draft.entity.published.version_id + else: + draft.version = None + draft.save() + + def register_content_models( content_model_cls: type[PublishableEntityMixin], content_version_model_cls: type[PublishableEntityVersionMixin], diff --git a/openedx_learning/core/publishing/migrations/0001_initial.py b/openedx_learning/core/publishing/migrations/0001_initial.py index d7663748a..dcfcf5fd8 100644 --- a/openedx_learning/core/publishing/migrations/0001_initial.py +++ b/openedx_learning/core/publishing/migrations/0001_initial.py @@ -1,14 +1,12 @@ -# Generated by Django 3.2.19 on 2023-06-15 14:43 +# Generated by Django 3.2.23 on 2024-01-22 00:37 -import uuid - -import django.core.validators -import django.db.models.deletion from django.conf import settings +import django.core.validators from django.db import migrations, models - +import django.db.models.deletion import openedx_learning.lib.fields import openedx_learning.lib.validators +import uuid class Migration(migrations.Migration): @@ -27,6 +25,7 @@ class Migration(migrations.Migration): ('uuid', models.UUIDField(default=uuid.uuid4, editable=False, unique=True, verbose_name='UUID')), ('key', openedx_learning.lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, max_length=500)), ('title', openedx_learning.lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_unicode_ci', 'sqlite': 'NOCASE'}, max_length=500)), + ('description', openedx_learning.lib.fields.MultiCollationTextField(blank=True, db_collations={'mysql': 'utf8mb4_unicode_ci', 'sqlite': 'NOCASE'}, default='', max_length=10000)), ('created', models.DateTimeField(validators=[openedx_learning.lib.validators.validate_utc_datetime])), ('updated', models.DateTimeField(validators=[openedx_learning.lib.validators.validate_utc_datetime])), ], @@ -43,7 +42,7 @@ class Migration(migrations.Migration): ('key', openedx_learning.lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, max_length=500)), ('created', models.DateTimeField(validators=[openedx_learning.lib.validators.validate_utc_datetime])), ('created_by', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, to=settings.AUTH_USER_MODEL)), - ('learning_package', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='oel_publishing.learningpackage')), + ('learning_package', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='publishable_entities', to='oel_publishing.learningpackage')), ], options={ 'verbose_name': 'Publishable Entity', @@ -56,7 +55,7 @@ class Migration(migrations.Migration): ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), ('uuid', models.UUIDField(default=uuid.uuid4, editable=False, unique=True, verbose_name='UUID')), ('title', openedx_learning.lib.fields.MultiCollationCharField(blank=True, db_collations={'mysql': 'utf8mb4_unicode_ci', 'sqlite': 'NOCASE'}, default='', max_length=500)), - ('version_num', models.PositiveBigIntegerField(validators=[django.core.validators.MinValueValidator(1)])), + ('version_num', models.PositiveIntegerField(validators=[django.core.validators.MinValueValidator(1)])), ('created', models.DateTimeField(validators=[openedx_learning.lib.validators.validate_utc_datetime])), ('created_by', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, to=settings.AUTH_USER_MODEL)), ('entity', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='versions', to='oel_publishing.publishableentity')), @@ -84,13 +83,13 @@ class Migration(migrations.Migration): migrations.CreateModel( name='Draft', fields=[ - ('entity', models.OneToOneField(on_delete=django.db.models.deletion.RESTRICT, primary_key=True, serialize=False, to='oel_publishing.publishableentity')), + ('entity', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, serialize=False, to='oel_publishing.publishableentity')), ], ), migrations.CreateModel( name='Published', fields=[ - ('entity', models.OneToOneField(on_delete=django.db.models.deletion.RESTRICT, primary_key=True, serialize=False, to='oel_publishing.publishableentity')), + ('entity', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, serialize=False, to='oel_publishing.publishableentity')), ], options={ 'verbose_name': 'Published Entity', @@ -104,7 +103,7 @@ class Migration(migrations.Migration): ('entity', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, to='oel_publishing.publishableentity')), ('new_version', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.RESTRICT, to='oel_publishing.publishableentityversion')), ('old_version', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.RESTRICT, related_name='+', to='oel_publishing.publishableentityversion')), - ('publish_log', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='oel_publishing.publishlog')), + ('publish_log', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='records', to='oel_publishing.publishlog')), ], options={ 'verbose_name': 'Publish Log Record', diff --git a/openedx_learning/core/publishing/migrations/0002_alter_fk_on_delete.py b/openedx_learning/core/publishing/migrations/0002_alter_fk_on_delete.py deleted file mode 100644 index c77a20400..000000000 --- a/openedx_learning/core/publishing/migrations/0002_alter_fk_on_delete.py +++ /dev/null @@ -1,30 +0,0 @@ -# Generated by Django 3.2.21 on 2023-10-13 14:25 - -import django.db.models.deletion -from django.db import migrations, models - - -class Migration(migrations.Migration): - """ - Make it so that Draft and Published cascade deletes from PublishableEntity. - - This makes it so that deleting a LearningPackage properly cleans up these - models as well. - """ - - dependencies = [ - ('oel_publishing', '0001_initial'), - ] - - operations = [ - migrations.AlterField( - model_name='draft', - name='entity', - field=models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, serialize=False, to='oel_publishing.publishableentity'), - ), - migrations.AlterField( - model_name='published', - name='entity', - field=models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, serialize=False, to='oel_publishing.publishableentity'), - ), - ] diff --git a/openedx_learning/core/publishing/model_mixins.py b/openedx_learning/core/publishing/model_mixins.py index 02cc7f200..bba7f0c18 100644 --- a/openedx_learning/core/publishing/model_mixins.py +++ b/openedx_learning/core/publishing/model_mixins.py @@ -9,7 +9,7 @@ from django.db import models from django.db.models.query import QuerySet -from .models import Draft, PublishableEntity, PublishableEntityVersion, Published +from .models import PublishableEntity, PublishableEntityVersion class PublishableEntityMixin(models.Model): @@ -25,7 +25,12 @@ class PublishableEntityMixin(models.Model): class PublishableEntityMixinManager(models.Manager): def get_queryset(self) -> QuerySet: - return super().get_queryset().select_related("publishable_entity") + return super().get_queryset() \ + .select_related( + "publishable_entity", + "publishable_entity__published", + "publishable_entity__draft", + ) objects: models.Manager[PublishableEntityMixin] = PublishableEntityMixinManager() @@ -102,7 +107,7 @@ class VersioningHelper: # You need to manually refetch it from the database to see the new # publish status: - component = get_component_by_pk(component.pk) + component = get_component(component.pk) # Now this will work: assert component.versioning.published == component_version @@ -129,59 +134,110 @@ def __init__(self, content_obj): # get the reverse related field name so that we can use that later. self.related_name = field_to_pev.related_query_name() + def _content_obj_version(self, pub_ent_version: PublishableEntityVersion | None): + """ + PublishableEntityVersion -> Content object version + + Given a reference to a PublishableEntityVersion, return the version + of the content object that we've been mixed into. + """ + if pub_ent_version is None: + return None + return getattr(pub_ent_version, self.related_name) + @property def draft(self): """ Return the content version object that is the current draft. + + So if you mix ``PublishableEntityMixin`` into ``Component``, then + ``component.versioning.draft`` will return you the + ``ComponentVersion`` that is the current draft (not the underlying + ``PublishableEntityVersion``). + + If this is causing many queries, it might be the case that you need + to add ``select_related('publishable_entity__draft__version')`` to + the queryset. """ - try: - draft = Draft.objects.get( - entity_id=self.content_obj.publishable_entity_id - ) - except Draft.DoesNotExist: - # If no Draft object exists at all, then no - # PublishableEntityVersion was ever created (just the - # PublishableEntity). - return None + # Check if there's an entry in Drafts, i.e. has there ever been a + # draft version of this PublishableEntity? + if hasattr(self.content_obj.publishable_entity, 'draft'): + # This can still be None if a draft existed at one point, but + # was then "deleted". When deleting, the Draft row stays, but + # the version it points to becomes None. + draft_pub_ent_version = self.content_obj.publishable_entity.draft.version + else: + draft_pub_ent_version = None + + # The Draft.version points to a PublishableEntityVersion, so convert + # that over to the class we actually want (were mixed into), e.g. + # a ComponentVersion. + return self._content_obj_version(draft_pub_ent_version) - draft_pub_ent_version = draft.version + @property + def latest(self): + """ + Return the most recently created version for this content object. - # There is an entry for this Draft, but the entry is None. This means - # there was a Draft at some point, but it's been "deleted" (though the - # actual history is still preserved). - if draft_pub_ent_version is None: - return None + This can be None if no versions have been created. - # If we've gotten this far, then draft_pub_ent_version points to an - # actual PublishableEntityVersion, but we want to follow that and go to - # the matching content version model. - return getattr(draft_pub_ent_version, self.related_name) + This is often the same as the draft version, but can differ if the + content object was soft deleted or the draft was reverted. + """ + return self.versions.order_by('-publishable_entity_version__version_num').first() @property def published(self): """ Return the content version object that is currently published. - """ - try: - published = Published.objects.get( - entity_id=self.content_obj.publishable_entity_id - ) - except Published.DoesNotExist: - # This means it's never been published. - return None - published_pub_ent_version = published.version + So if you mix ``PublishableEntityMixin`` into ``Component``, then + ``component.versioning.published`` will return you the + ``ComponentVersion`` that is currently published (not the underlying + ``PublishableEntityVersion``). - # There is a Published entry, but the entry is None. This means that - # it was published at some point, but it's been "deleted" or - # unpublished (though the actual history is still preserved). - if published_pub_ent_version is None: - return None + If this is causing many queries, it might be the case that you need + to add ``select_related('publishable_entity__published__version')`` + to the queryset. + """ + # Check if there's an entry in Published, i.e. has there ever been a + # published version of this PublishableEntity? + if hasattr(self.content_obj.publishable_entity, 'published'): + # This can still be None if something was published and then + # later "deleted". When deleting, the Published row stays, but + # the version it points to becomes None. + published_pub_ent_version = self.content_obj.publishable_entity.published.version + else: + published_pub_ent_version = None + + # The Published.version points to a PublishableEntityVersion, so + # convert that over to the class we actually want (were mixed into), + # e.g. a ComponentVersion. + return self._content_obj_version(published_pub_ent_version) - # If we've gotten this far, then published_pub_ent_version points to an - # actual PublishableEntityVersion, but we want to follow that and go to - # the matching content version model. - return getattr(published_pub_ent_version, self.related_name) + @property + def has_unpublished_changes(self): + """ + Do we have unpublished changes? + + The simplest way to implement this would be to check self.published + vs. self.draft, but that would cause unnecessary queries. This + implementation should require no extra queries provided that the + model was instantiated using a queryset that used a select related + that has at least ``publishable_entity__draft`` and + ``publishable_entity__published``. + """ + pub_entity = self.content_obj.publishable_entity + if hasattr(pub_entity, 'draft'): + draft_version_id = pub_entity.draft.version_id + else: + draft_version_id = None + if hasattr(pub_entity, 'published'): + published_version_id = pub_entity.published.version_id + else: + published_version_id = None + + return draft_version_id != published_version_id @property def versions(self): diff --git a/openedx_learning/core/publishing/models.py b/openedx_learning/core/publishing/models.py index 1eb6803c8..9082e8f2d 100644 --- a/openedx_learning/core/publishing/models.py +++ b/openedx_learning/core/publishing/models.py @@ -16,6 +16,7 @@ from django.db import models from openedx_learning.lib.fields import ( + MultiCollationTextField, case_insensitive_char_field, immutable_uuid_field, key_field, @@ -32,6 +33,20 @@ class LearningPackage(models.Model): # type: ignore[django-manager-missing] uuid = immutable_uuid_field() key = key_field() title = case_insensitive_char_field(max_length=500, blank=False) + description = MultiCollationTextField( + blank=True, + null=False, + default="", + max_length=10_000, + # We don't really expect to ever sort by the text column, but we may + # want to do case-insensitive searches, so it's useful to have a case + # and accent insensitive collation. + db_collations={ + "sqlite": "NOCASE", + "mysql": "utf8mb4_unicode_ci", + } + ) + created = manual_date_time_field() updated = manual_date_time_field() @@ -140,7 +155,11 @@ class PublishableEntity(models.Model): """ uuid = immutable_uuid_field() - learning_package = models.ForeignKey(LearningPackage, on_delete=models.CASCADE) + learning_package = models.ForeignKey( + LearningPackage, + on_delete=models.CASCADE, + related_name="publishable_entities", + ) key = key_field() created = manual_date_time_field() created_by = models.ForeignKey( @@ -219,7 +238,7 @@ class PublishableEntityVersion(models.Model): # users to refer to than a hash or UUID value. It also helps us catch race # conditions on save, by setting a unique constraint on the entity and # version_num. - version_num = models.PositiveBigIntegerField( + version_num = models.PositiveIntegerField( null=False, validators=[MinValueValidator(1)], ) @@ -362,7 +381,11 @@ class PublishLogRecord(models.Model): and ``new_version`` field values. """ - publish_log = models.ForeignKey(PublishLog, on_delete=models.CASCADE) + publish_log = models.ForeignKey( + PublishLog, + on_delete=models.CASCADE, + related_name="records", + ) entity = models.ForeignKey(PublishableEntity, on_delete=models.RESTRICT) old_version = models.ForeignKey( PublishableEntityVersion, diff --git a/openedx_learning/lib/managers.py b/openedx_learning/lib/managers.py new file mode 100644 index 000000000..68c850f8e --- /dev/null +++ b/openedx_learning/lib/managers.py @@ -0,0 +1,37 @@ +""" +Custom Django ORM Managers. +""" +from django.db.models.query import QuerySet +from django.db import models + +class WithRelationsManager(models.Manager): + """ + Custom Manager that adds select_related to the default queryset. + + This is useful if people using a model will very frequently want to call + into some of its relations and you want to avoid unnecessary extra database + calls. + + Use this to create a distinctly named manager on your model class, instead + of overwriting ``objects``. So for example:: + + class Component(models.Model): + with_publishing_relations = WithRelationsManager( + 'publishable_entity', + 'publishable_entity__draft__version', + 'publishable_entity__draft__version__componentversion', + 'publishable_entity__published__version', + 'publishable_entity__published__version__componentversion', + ) + """ + def __init__(self, *relations): + """ + Init with a list of relations that you would use in select_related. + """ + self._relations = relations + super().__init__() + + def get_queryset(self) -> QuerySet: + return super().get_queryset().select_related( + *self._relations + ) diff --git a/tests/openedx_learning/core/components/test_api.py b/tests/openedx_learning/core/components/test_api.py new file mode 100644 index 000000000..29f3a3cad --- /dev/null +++ b/tests/openedx_learning/core/components/test_api.py @@ -0,0 +1,237 @@ +""" +Basic tests of the Components API. +""" +from datetime import datetime, timezone + +from openedx_learning.core.components import api as components_api +from openedx_learning.core.publishing import api as publishing_api +from openedx_learning.lib.test_utils import TestCase + + +class TestPerformance(TestCase): + """ + Performance related tests to make sure we don't get n + 1 queries. + """ + @classmethod + def setUpTestData(cls) -> None: + """ + Initialize our content data (all our tests are read only). + + We don't actually need to add content to the ComponentVersions, since + for this we only care about the metadata on Compnents, their versions, + and the associated draft/publish status. + """ + cls.learning_package = publishing_api.create_learning_package( + "components.TestPerformance", + "Learning Package for Testing Performance (measured by DB queries)", + ) + cls.now = datetime(2023, 5, 8, tzinfo=timezone.utc) + + def test_component_num_queries(self) -> None: + """ + Create a basic component and test that we fetch it back in 1 query. + """ + component, _version = components_api.create_component_and_version( + learning_package_id=self.learning_package.id, + namespace="xblock.v1", + type="problem", + local_key="Query Counting", + title="Querying Counting Problem", + created=self.now, + created_by=None, + ) + publishing_api.publish_all_drafts( + self.learning_package.pk, + published_at=self.now + ) + + # We should be fetching all of this with a select-related, so only one + # database query should happen here. + with self.assertNumQueries(1): + component = components_api.get_component(component.pk) + draft = component.versioning.draft + published = component.versioning.published + assert draft.title == published.title + +class TestGetComponents(TestCase): + """ + Test grabbing a queryset of Components. + """ + + @classmethod + def setUpTestData(cls) -> None: + """ + Initialize our content data (all our tests are read only). + + We don't actually need to add content to the ComponentVersions, since + for this we only care about the metadata on Compnents, their versions, + and the associated draft/publish status. + """ + cls.learning_package = publishing_api.create_learning_package( + "components.TestGetComponents", + "Learning Package for Testing Getting & Filtering Components", + ) + cls.now = datetime(2023, 5, 8, tzinfo=timezone.utc) + + # Components we're publishing... + cls.published_problem, _version = components_api.create_component_and_version( + learning_package_id=cls.learning_package.id, + namespace="xblock.v2", + type="problem", + local_key="published_problem", + title="Published Problem", + created=cls.now, + created_by=None, + ) + cls.published_html, _version = components_api.create_component_and_version( + learning_package_id=cls.learning_package.id, + namespace="xblock.v1", + type="html", + local_key="published_html", + title="Published HTML", + created=cls.now, + created_by=None, + ) + publishing_api.publish_all_drafts( + cls.learning_package.pk, + published_at=cls.now + ) + + # Components that exist only as Drafts + cls.unpublished_problem, _version = components_api.create_component_and_version( + learning_package_id=cls.learning_package.id, + namespace="xblock.v2", + type="problem", + local_key="unpublished_problem", + title="Unpublished Problem", + created=cls.now, + created_by=None, + ) + cls.unpublished_html, _version = components_api.create_component_and_version( + learning_package_id=cls.learning_package.id, + namespace="xblock.v1", + type="html", + local_key="unpublished_html", + title="Unpublished HTML", + created=cls.now, + created_by=None, + ) + + # Component we're putting here to soft delete (this will remove the + # Draft entry) + cls.deleted_video, _version = components_api.create_component_and_version( + learning_package_id=cls.learning_package.id, + namespace="xblock.v1", + type="html", + local_key="deleted_video", + title="Deleted Video", + created=cls.now, + created_by=None, + ) + publishing_api.soft_delete_draft(cls.deleted_video.pk) + + def test_no_filters(self): + """ + Test that we pull back everything, even unpublished or "deleted" items. + """ + all_components = components_api.get_components(self.learning_package.id).all() + assert list(all_components) == [ + self.published_problem, + self.published_html, + self.unpublished_problem, + self.unpublished_html, + self.deleted_video, + ] + + def test_draft_filter(self): + """ + Test the draft flag. + """ + components_with_draft_version = components_api.get_components( + self.learning_package.id, + draft=True, + ).all() + assert list(components_with_draft_version) == [ + self.published_problem, + self.published_html, + self.unpublished_problem, + self.unpublished_html + ] + + components_without_draft_version = components_api.get_components( + self.learning_package.id, + draft=False, + ).all() + assert list(components_without_draft_version) == [ + self.deleted_video + ] + + def test_published_filter(self): + """ + Test the published filter. + """ + components_with_published_version = components_api.get_components( + self.learning_package.id, + published=True, + ).all() + assert list(components_with_published_version) == [ + self.published_problem, + self.published_html, + ] + components_without_published_version = components_api.get_components( + self.learning_package.id, + published=False, + ).all() + assert list(components_without_published_version) == [ + self.unpublished_problem, + self.unpublished_html, + self.deleted_video, + ] + + def test_namespace_filter(self): + """ + Test the namespace filter. + + Note that xblock.v2 is being used to test filtering, but there's nothing + that's actually in the system for xblock.v2 at the moment. + """ + components_with_xblock_v2 = components_api.get_components( + self.learning_package.id, + namespace='xblock.v2', + ).all() + assert list(components_with_xblock_v2) == [ + self.published_problem, + self.unpublished_problem, + ] + + def test_types_filter(self): + """ + Test the types filter. + """ + html_and_video_components = components_api.get_components( + self.learning_package.id, + types=['html', 'video'] + ).all() + assert list(html_and_video_components) == [ + self.published_html, + self.unpublished_html, + self.deleted_video, + ] + + def test_title_filter(self): + """ + Test the title filter. + + Note that this should be doing a case-insensitive match. + """ + components = components_api.get_components( + self.learning_package.id, + title="PUBLISHED" + ).all() + # These all have a title with "published" in it somewhere. + assert list(components) == [ + self.published_problem, + self.published_html, + self.unpublished_problem, + self.unpublished_html, + ] diff --git a/tests/openedx_learning/core/components/test_models.py b/tests/openedx_learning/core/components/test_models.py index 7675f7050..2ef5daf64 100644 --- a/tests/openedx_learning/core/components/test_models.py +++ b/tests/openedx_learning/core/components/test_models.py @@ -3,7 +3,7 @@ """ from datetime import datetime, timezone -from openedx_learning.core.components.api import create_component_and_version +from openedx_learning.core.components.api import create_component_and_version, get_component from openedx_learning.core.publishing.api import LearningPackage, create_learning_package, publish_all_drafts from openedx_learning.lib.test_utils import TestCase @@ -37,7 +37,12 @@ def test_latest_version(self) -> None: assert component.versioning.published is None publish_all_drafts(self.learning_package.pk, published_at=self.now) - # Force the re-fetch from the database + # Publishing isn't immediately reflected in the component obj (it's + # using a cached version). + assert component.versioning.published is None + + # Re-fetching the component and the published version should be updated. + component = get_component(component.pk) assert component.versioning.published == component_version # Grabbing the list of versions for this component diff --git a/tests/openedx_learning/core/publishing/test_api.py b/tests/openedx_learning/core/publishing/test_api.py index a258f5b01..df4809d4e 100644 --- a/tests/openedx_learning/core/publishing/test_api.py +++ b/tests/openedx_learning/core/publishing/test_api.py @@ -22,7 +22,13 @@ def test_normal(self) -> None: # Note: we must specify '-> None' to opt in to t key = "my_key" title = "My Excellent Title with Emoji 🔥" created = datetime(2023, 4, 2, 15, 9, 0, tzinfo=timezone.utc) - package = publishing_api.create_learning_package(key, title, created) + description = "A fun Description!" + package = publishing_api.create_learning_package( + key=key, + title=title, + description=description, + created=created + ) assert package.key == "my_key" assert package.title == "My Excellent Title with Emoji 🔥" @@ -62,7 +68,11 @@ def test_non_utc_time(self) -> None: Require UTC timezone for created. """ with pytest.raises(ValidationError) as excinfo: - publishing_api.create_learning_package("my_key", "A Title", datetime(2023, 4, 2)) + publishing_api.create_learning_package( + key="my_key", + title="A Title", + created=datetime(2023, 4, 2) + ) message_dict = excinfo.value.message_dict # Both datetime fields should be marked as invalid From 18d3cf8cd5651083860e57a7a8b976111bdc88e1 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 24 Jan 2024 09:39:09 -0500 Subject: [PATCH 02/23] test: add more test coverage for components API --- openedx_learning/core/components/api.py | 4 +- .../core/components/test_api.py | 141 ++++++++++++++++++ 2 files changed, 144 insertions(+), 1 deletion(-) diff --git a/openedx_learning/core/components/api.py b/openedx_learning/core/components/api.py index 62b1556ae..40128a68c 100644 --- a/openedx_learning/core/components/api.py +++ b/openedx_learning/core/components/api.py @@ -79,7 +79,7 @@ def create_next_version( content_to_replace: dict[str: int], created: datetime, created_by: int | None = None, -): +) -> ComponentVersion: """ Create a new ComponentVersion based on the most recent version. @@ -138,6 +138,8 @@ def create_next_version( learner_downloadable=cvrc.learner_downloadable, ) + return component_version + def create_component_and_version( learning_package_id: int, diff --git a/tests/openedx_learning/core/components/test_api.py b/tests/openedx_learning/core/components/test_api.py index 29f3a3cad..0f7614e03 100644 --- a/tests/openedx_learning/core/components/test_api.py +++ b/tests/openedx_learning/core/components/test_api.py @@ -3,7 +3,10 @@ """ from datetime import datetime, timezone +from django.core.exceptions import ObjectDoesNotExist + from openedx_learning.core.components import api as components_api +from openedx_learning.core.contents import api as contents_api from openedx_learning.core.publishing import api as publishing_api from openedx_learning.lib.test_utils import TestCase @@ -235,3 +238,141 @@ def test_title_filter(self): self.unpublished_problem, self.unpublished_html, ] + + +class TestComponentGetAndExists(TestCase): + """ + Test getting a Component by primary key or key string. + """ + @classmethod + def setUpTestData(cls) -> None: + """ + Initialize our content data (all our tests are read only). + + We don't actually need to add content to the ComponentVersions, since + for this we only care about the metadata on Compnents, their versions, + and the associated draft/publish status. + """ + cls.learning_package = publishing_api.create_learning_package( + "components.TestComponentGetAndExists", + "Learning Package for Testing Getting a Component", + ) + cls.now = datetime(2023, 5, 8, tzinfo=timezone.utc) + cls.problem = components_api.create_component( + learning_package_id=cls.learning_package.id, + namespace='xblock.v1', + type='problem', + local_key='my_component', + created=cls.now, + created_by=None, + ) + cls.html = components_api.create_component( + learning_package_id=cls.learning_package.id, + namespace='xblock.v1', + type='html', + local_key='my_component', + created=cls.now, + created_by=None, + ) + + def test_simple_get(self): + assert components_api.get_component(self.problem.pk) == self.problem + with self.assertRaises(ObjectDoesNotExist): + components_api.get_component(-1) + + def test_get_by_key(self): + assert self.html == components_api.get_component_by_key( + self.learning_package.id, + namespace='xblock.v1', + type='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 + local_key='my_component', + ) + + def test_exists_by_key(self): + assert components_api.component_exists_by_key( + self.learning_package.id, + namespace='xblock.v1', + type='problem', + local_key='my_component', + ) + assert not components_api.component_exists_by_key( + self.learning_package.id, + namespace='xblock.v1', + type='problem', + local_key='not_my_component', + ) + + +class TestCreateNextVersion(TestCase): + + @classmethod + def setUpTestData(cls) -> None: + cls.learning_package = publishing_api.create_learning_package( + "components.TestCreateNextVersion", + "Learning Package for Testing Next Version Creation", + ) + cls.now = datetime(2023, 5, 8, tzinfo=timezone.utc) + cls.problem = components_api.create_component( + learning_package_id=cls.learning_package.id, + namespace='xblock.v1', + type='problem', + local_key='my_component', + created=cls.now, + created_by=None, + ) + + def test_multiple_versions(self): + hello_content, _created = contents_api.get_or_create_text_content_from_bytes( + learning_package_id=self.learning_package.id, + data_bytes="Hello World!".encode('utf-8'), + mime_type="text/plain", + created=self.now, + ) + goodbye_content, _created = contents_api.get_or_create_text_content_from_bytes( + learning_package_id=self.learning_package.id, + data_bytes="Goodbye World!".encode('utf-8'), + mime_type="text/plain", + created=self.now, + ) + blank_content, _created = contents_api.get_or_create_text_content_from_bytes( + learning_package_id=self.learning_package.id, + data_bytes="".encode('utf-8'), + mime_type="text/plain", + created=self.now, + ) + version_1 = components_api.create_next_version( + self.problem.pk, + title="Problem Version 1", + content_to_replace={ + "hello.txt": hello_content.pk, + }, + created=self.now, + ) + assert version_1.version_num == 1 + assert version_1.title == "Problem Version 1" + version_1_contents = list(version_1.raw_contents.all()) + assert len(version_1_contents) == 1 + assert version_1_contents[0].text_content == hello_content + # Probably need a better API for this later. + assert version_1.raw_contents \ + .filter(componentversionrawcontent__key="hello.txt") \ + .exists() + + version_2 = components_api.create_next_version( + self.problem.pk, + title="Problem Version 2", + content_to_replace={ + "hello.txt": blank_content.pk, + "goodbye.txt": goodbye_content.pk, + "blank.txt": blank_content.pk, + }, + created=self.now, + ) + assert version_2.version_num == 2 From 75f7a2e8d2c2959e170664daff20f8bd37c28926 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 24 Jan 2024 13:55:15 -0500 Subject: [PATCH 03/23] test: more test coverage --- .gitignore | 3 + openedx_learning/core/components/api.py | 8 +- test_settings.py | 2 + .../core/components/test_api.py | 77 +++++++++++++++++-- 4 files changed, 78 insertions(+), 12 deletions(-) diff --git a/.gitignore b/.gitignore index 6b5c4fed5..0f071cb7a 100644 --- a/.gitignore +++ b/.gitignore @@ -69,3 +69,6 @@ venv/ # Media files (for uploads) media/ + +# Media files generated during test runs +test_media/ diff --git a/openedx_learning/core/components/api.py b/openedx_learning/core/components/api.py index 40128a68c..db0b7dcc5 100644 --- a/openedx_learning/core/components/api.py +++ b/openedx_learning/core/components/api.py @@ -127,12 +127,12 @@ def create_next_version( # Now copy any old associations that existed, as long as they don't # conflict with the new stuff. last_version_content_mapping = ComponentVersionRawContent.objects \ - .filter(component_version=component_version) \ + .filter(component_version=last_version) \ .all() for cvrc in last_version_content_mapping: if cvrc.key not in content_to_replace: ComponentVersionRawContent.objects.create( - raw_content_id=cvrc.raw_content_pk, + raw_content_id=cvrc.raw_content_id, component_version=component_version, key=cvrc.key, learner_downloadable=cvrc.learner_downloadable, @@ -273,7 +273,7 @@ def get_component_version_content( def add_content_to_component_version( - component_version: ComponentVersion, + component_version_id: int, raw_content_id: int, key: str, learner_downloadable=False, @@ -282,7 +282,7 @@ def add_content_to_component_version( Add a RawContent to the given ComponentVersion """ cvrc, _created = ComponentVersionRawContent.objects.get_or_create( - component_version=component_version, + component_version_id=component_version_id, raw_content_id=raw_content_id, key=key, learner_downloadable=learner_downloadable, diff --git a/test_settings.py b/test_settings.py index 28676e13e..53548fb45 100644 --- a/test_settings.py +++ b/test_settings.py @@ -67,6 +67,8 @@ def root(*args): "STORAGE": None, } +MEDIA_ROOT = root("test_media") + ######################### Django Rest Framework ######################## REST_FRAMEWORK = { diff --git a/tests/openedx_learning/core/components/test_api.py b/tests/openedx_learning/core/components/test_api.py index 0f7614e03..3b59dbca0 100644 --- a/tests/openedx_learning/core/components/test_api.py +++ b/tests/openedx_learning/core/components/test_api.py @@ -310,7 +310,7 @@ def test_exists_by_key(self): ) -class TestCreateNextVersion(TestCase): +class TestCreateNewVersions(TestCase): @classmethod def setUpTestData(cls) -> None: @@ -328,6 +328,37 @@ def setUpTestData(cls) -> None: created_by=None, ) + def test_add(self): + new_version = components_api.create_component_version( + self.problem.pk, + version_num=1, + title="My Title", + created=self.now, + created_by=None, + ) + new_content, _created = contents_api.get_or_create_raw_content( + self.learning_package.pk, + b"This is some data", + mime_type="text/plain", + created=self.now, + ) + components_api.add_content_to_component_version( + new_version.pk, + raw_content_id=new_content.pk, + key="hello.txt", + learner_downloadable=False, + ) + # re-fetch from the database to check to see if we wrote it correctly + new_version = components_api.get_component(self.problem.pk) \ + .versions \ + .get(publishable_entity_version__version_num=1) + assert ( + new_content == + new_version.raw_contents.get(componentversionrawcontent__key="hello.txt") + ) + + + def test_multiple_versions(self): hello_content, _created = contents_api.get_or_create_text_content_from_bytes( learning_package_id=self.learning_package.id, @@ -347,32 +378,62 @@ def test_multiple_versions(self): mime_type="text/plain", created=self.now, ) + + # Two text files, hello.txt and goodbye.txt version_1 = components_api.create_next_version( self.problem.pk, title="Problem Version 1", content_to_replace={ "hello.txt": hello_content.pk, + "goodbye.txt": goodbye_content.pk, }, created=self.now, ) assert version_1.version_num == 1 assert version_1.title == "Problem Version 1" version_1_contents = list(version_1.raw_contents.all()) - assert len(version_1_contents) == 1 - assert version_1_contents[0].text_content == hello_content - # Probably need a better API for this later. - assert version_1.raw_contents \ - .filter(componentversionrawcontent__key="hello.txt") \ - .exists() + assert len(version_1_contents) == 2 + assert ( + hello_content == + version_1.raw_contents + .get(componentversionrawcontent__key="hello.txt") + .text_content + ) + assert ( + goodbye_content == + version_1.raw_contents + .get(componentversionrawcontent__key="goodbye.txt") + .text_content + ) + # This should keep the old value for goodbye.txt, add blank.txt, and set + # hello.txt to be a new value (blank). version_2 = components_api.create_next_version( self.problem.pk, title="Problem Version 2", content_to_replace={ "hello.txt": blank_content.pk, - "goodbye.txt": goodbye_content.pk, "blank.txt": blank_content.pk, }, created=self.now, ) assert version_2.version_num == 2 + assert version_2.raw_contents.count() == 3 + assert ( + blank_content == + version_2.raw_contents + .get(componentversionrawcontent__key="hello.txt") + .text_content + ) + assert ( + goodbye_content == + version_2.raw_contents + .get(componentversionrawcontent__key="goodbye.txt") + .text_content + ) + assert ( + blank_content == + version_2.raw_contents + .get(componentversionrawcontent__key="blank.txt") + .text_content + ) From 58661a511a9bd91897b091b87fd257d5cb82c15d Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Sat, 27 Jan 2024 15:32:32 -0500 Subject: [PATCH 04/23] refactor: adjust indentation --- openedx_learning/core/components/api.py | 65 +++++++++++-------- openedx_learning/core/publishing/api.py | 54 +++++++++------ .../core/components/test_api.py | 31 +++++---- 3 files changed, 90 insertions(+), 60 deletions(-) diff --git a/openedx_learning/core/components/api.py b/openedx_learning/core/components/api.py index db0b7dcc5..31ef2ad38 100644 --- a/openedx_learning/core/components/api.py +++ b/openedx_learning/core/components/api.py @@ -76,7 +76,7 @@ def create_component_version( def create_next_version( component_pk: int, title: str, - content_to_replace: dict[str: int], + content_to_replace: dict[str: int | None], created: datetime, created_by: int | None = None, ) -> ComponentVersion: @@ -118,17 +118,23 @@ def create_next_version( ) # First copy the new stuff over... for key, raw_content_pk in content_to_replace.items(): - ComponentVersionRawContent.objects.create( - raw_content_id=raw_content_pk, - component_version=component_version, - key=key, - learner_downloadable=False, - ) + # If the raw_content_pk is None, it means we want to remove the + # content represented by our key from the next version. Otherwise, + # we add our key->raw_content_pk mapping to the next version. + if raw_content_pk is not None: + ComponentVersionRawContent.objects.create( + raw_content_id=raw_content_pk, + component_version=component_version, + key=key, + learner_downloadable=False, + ) # Now copy any old associations that existed, as long as they don't # conflict with the new stuff. - last_version_content_mapping = ComponentVersionRawContent.objects \ - .filter(component_version=last_version) \ - .all() + last_version_content_mapping = ( + ComponentVersionRawContent + .objects + .filter(component_version=last_version) + ) for cvrc in last_version_content_mapping: if cvrc.key not in content_to_replace: ComponentVersionRawContent.objects.create( @@ -229,9 +235,12 @@ def get_components( info from the Component's draft and published versions, since we'll be referencing these a lot. """ - qset = Component.with_publishing_relations \ - .filter(learning_package_id=learning_package_id) \ - .order_by('pk') + qset = ( + Component + .with_publishing_relations + .filter(learning_package_id=learning_package_id) + .order_by('pk') + ) if draft is not None: qset = qset.filter(publishable_entity__draft__version__isnull = not draft) if published is not None: @@ -257,21 +266,25 @@ def get_component_version_content( Can raise a django.core.exceptions.ObjectDoesNotExist error if there is no matching ComponentVersionRawContent. """ - return ComponentVersionRawContent.objects.select_related( - "raw_content", - "raw_content__media_type", - "raw_content__textcontent", - "component_version", - "component_version__component", - "component_version__component__learning_package", - ).get( - Q(component_version__component__learning_package__key=learning_package_key) - & Q(component_version__component__publishable_entity__key=component_key) - & Q(component_version__publishable_entity_version__version_num=version_num) - & Q(key=key) + return ( + ComponentVersionRawContent + .objects + .select_related( + "raw_content", + "raw_content__media_type", + "raw_content__textcontent", + "component_version", + "component_version__component", + "component_version__component__learning_package", + ) + .get( + Q(component_version__component__learning_package__key=learning_package_key) + & Q(component_version__component__publishable_entity__key=component_key) + & Q(component_version__publishable_entity_version__version_num=version_num) + & Q(key=key) + ) ) - def add_content_to_component_version( component_version_id: int, raw_content_id: int, diff --git a/openedx_learning/core/publishing/api.py b/openedx_learning/core/publishing/api.py index 2a4f87c8a..c43d138af 100644 --- a/openedx_learning/core/publishing/api.py +++ b/openedx_learning/core/publishing/api.py @@ -163,10 +163,13 @@ def learning_package_exists(key: str) -> bool: def get_last_publish(learning_package_id: int) -> PublishLog | None: - return PublishLog.objects \ - .filter(learning_package_id=learning_package_id) \ - .order_by('-id') \ - .first() + return ( + PublishLog + .objects + .filter(learning_package_id=learning_package_id) + .order_by('-id') + .first() + ) def get_all_drafts(learning_package_id: int): return Draft.objects.filter( @@ -175,9 +178,12 @@ def get_all_drafts(learning_package_id: int): ) def get_entities_with_unpublished_changes(learning_package_id: int): - return PublishableEntity.objects \ - .filter(learning_package_id=learning_package_id) \ - .exclude(draft__version=F('published__version')) + return ( + PublishableEntity + .objects + .filter(learning_package_id=learning_package_id) + .exclude(draft__version=F('published__version')) + ) def get_entities_with_unpublished_deletes(learning_package_id: int): """ @@ -185,13 +191,15 @@ def get_entities_with_unpublished_deletes(learning_package_id: int): not-null Published version. (If both are null, it means it's already been deleted in a previous publish, or it was never published.) """ - return PublishableEntity.objects \ - .filter( - learning_package_id=learning_package_id, - draft__version__isnull=True, - ) \ - .exclude(published__version__isnull=True) - + return ( + PublishableEntity + .objects + .filter( + learning_package_id=learning_package_id, + draft__version__isnull=True, + ) + .exclude(published__version__isnull=True) + ) def publish_all_drafts( learning_package_id: int, @@ -203,9 +211,11 @@ def publish_all_drafts( Publish everything that is a Draft and is not already published. """ draft_qset = ( - Draft.objects.select_related("entity__published") - .filter(entity__learning_package_id=learning_package_id) - .exclude(entity__published__version_id=F("version_id")) + Draft + .objects + .select_related("entity__published") + .filter(entity__learning_package_id=learning_package_id) + .exclude(entity__published__version_id=F("version_id")) ) return publish_from_drafts( learning_package_id, draft_qset, message, published_at, published_by @@ -317,16 +327,18 @@ def reset_drafts_to_published(learning_package_id: int) -> None: """ # These are all the drafts that are different from the publisehd versions. draft_qset = ( - Draft.objects.select_related("entity__published") - .filter(entity__learning_package_id=learning_package_id) - .exclude(entity__published__version_id=F("version_id")) + Draft + .objects + .select_related("entity__published") + .filter(entity__learning_package_id=learning_package_id) + .exclude(entity__published__version_id=F("version_id")) ) # Note: We can't do an .update with a F() on a joined field in the ORM, so # we have to loop through the drafts individually to reset them. We can # rework this into a bulk update or custom SQL if it becomes a performance # issue. with atomic(): - for draft in draft_qset.all(): + for draft in draft_qset: if hasattr(draft.entity, 'published'): draft.version_id = draft.entity.published.version_id else: diff --git a/tests/openedx_learning/core/components/test_api.py b/tests/openedx_learning/core/components/test_api.py index 3b59dbca0..f8ea0992e 100644 --- a/tests/openedx_learning/core/components/test_api.py +++ b/tests/openedx_learning/core/components/test_api.py @@ -349,16 +349,18 @@ def test_add(self): learner_downloadable=False, ) # re-fetch from the database to check to see if we wrote it correctly - new_version = components_api.get_component(self.problem.pk) \ - .versions \ - .get(publishable_entity_version__version_num=1) + new_version = ( + components_api + .get_component(self.problem.pk) + .versions + .get(publishable_entity_version__version_num=1) + ) assert ( new_content == new_version.raw_contents.get(componentversionrawcontent__key="hello.txt") ) - def test_multiple_versions(self): hello_content, _created = contents_api.get_or_create_text_content_from_bytes( learning_package_id=self.learning_package.id, @@ -421,19 +423,22 @@ def test_multiple_versions(self): assert version_2.raw_contents.count() == 3 assert ( blank_content == - version_2.raw_contents - .get(componentversionrawcontent__key="hello.txt") - .text_content + version_2 + .raw_contents + .get(componentversionrawcontent__key="hello.txt") + .text_content ) assert ( goodbye_content == - version_2.raw_contents - .get(componentversionrawcontent__key="goodbye.txt") - .text_content + version_2 + .raw_contents + .get(componentversionrawcontent__key="goodbye.txt") + .text_content ) assert ( blank_content == - version_2.raw_contents - .get(componentversionrawcontent__key="blank.txt") - .text_content + version_2 + .raw_contents + .get(componentversionrawcontent__key="blank.txt") + .text_content ) From 75d1dfa9bfb4c7bff8b334b74f8f361e3e1d3f41 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Sat, 27 Jan 2024 15:59:18 -0500 Subject: [PATCH 05/23] feat: add deletion support to create_next_version and document it better --- openedx_learning/core/components/api.py | 12 ++++++++++ .../core/components/test_api.py | 23 +++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/openedx_learning/core/components/api.py b/openedx_learning/core/components/api.py index 31ef2ad38..e12d0b4dd 100644 --- a/openedx_learning/core/components/api.py +++ b/openedx_learning/core/components/api.py @@ -88,6 +88,18 @@ def create_next_version( Before calling this, you should create any new contents via the contents API, since ``content_to_replace`` needs RawContent IDs for the values. + The ``content_to_replace`` dict is a mapping of strings representing the + local path/key for a file, to ``RawContent.id`` values. Using a `None` for + a value in this dict means to delete that key in the next version. + + It is okay to mark entries for deletion that don't exist. For instance, if a + version has ``a.txt`` and ``b.txt``, sending a ``content_to_replace`` value + of ``{"a.txt": None, "c.txt": None}`` will remove ``a.txt`` from the next + version, leave ``b.txt`` alone, and will not error–even though there is no + ``c.txt`` in the previous version. This is to make it a little more + convenient to remove paths (e.g. due to deprecation) without having to + always check for its existence first. + TODO: Have to add learning_downloadable info to this when it comes time to support static asset download. """ diff --git a/tests/openedx_learning/core/components/test_api.py b/tests/openedx_learning/core/components/test_api.py index f8ea0992e..79e0bf54c 100644 --- a/tests/openedx_learning/core/components/test_api.py +++ b/tests/openedx_learning/core/components/test_api.py @@ -442,3 +442,26 @@ def test_multiple_versions(self): .get(componentversionrawcontent__key="blank.txt") .text_content ) + + # Now we're going to set "hello.txt" back to hello_content, but remove + # blank.txt, goodbye.txt, and an unknown "nothere.txt". + version_3 = components_api.create_next_version( + self.problem.pk, + title="Problem Version 3", + content_to_replace={ + "hello.txt": hello_content.pk, + "blank.txt": None, + "goodbye.txt": None, + "nothere.txt": None, # should not error + }, + created=self.now, + ) + assert version_3.version_num == 3 + assert version_3.raw_contents.count() == 1 + assert ( + hello_content == + version_3 + .raw_contents + .get(componentversionrawcontent__key="hello.txt") + .text_content + ) From 8a7a2945a0802393cbc15a6aba0def6532e0fa1e Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Sat, 27 Jan 2024 16:06:50 -0500 Subject: [PATCH 06/23] test: add more type annotations --- openedx_learning/core/components/api.py | 23 +++++++++++++---------- openedx_learning/core/publishing/api.py | 14 +++++++------- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/openedx_learning/core/components/api.py b/openedx_learning/core/components/api.py index e12d0b4dd..271f26978 100644 --- a/openedx_learning/core/components/api.py +++ b/openedx_learning/core/components/api.py @@ -29,7 +29,7 @@ def create_component( local_key: str, created: datetime, created_by: int | None, -): +) -> Component: """ Create a new Component (an entity like a Problem or Video) """ @@ -167,7 +167,7 @@ def create_component_and_version( title: str, created: datetime, created_by: int | None = None, -): +) -> (Component, ComponentVersion): """ Create a Component and associated ComponentVersion atomically """ @@ -197,17 +197,20 @@ def get_component_by_key( namespace: str, type: str, # pylint: disable=redefined-builtin local_key: str, -): +) -> Component: """ Get Componet by it unique namespace/type/local_key tuple. """ - return Component.with_publishing_relations \ - .get( - learning_package_id=learning_package_id, - namespace=namespace, - type=type, - local_key=local_key, - ) + return ( + Component + .with_publishing_relations + .get( + learning_package_id=learning_package_id, + namespace=namespace, + type=type, + local_key=local_key, + ) + ) def component_exists_by_key( learning_package_id: int, diff --git a/openedx_learning/core/publishing/api.py b/openedx_learning/core/publishing/api.py index c43d138af..a6f9e9199 100644 --- a/openedx_learning/core/publishing/api.py +++ b/openedx_learning/core/publishing/api.py @@ -149,7 +149,7 @@ def create_publishable_entity_version( ) return version -def get_publishable_entity_by_key(learning_package_id, key): +def get_publishable_entity_by_key(learning_package_id, key) -> PublishableEntity: return PublishableEntity.objects.get( learning_package_id=learning_package_id, key=key, @@ -171,13 +171,13 @@ def get_last_publish(learning_package_id: int) -> PublishLog | None: .first() ) -def get_all_drafts(learning_package_id: int): +def get_all_drafts(learning_package_id: int) -> QuerySet[Draft]: return Draft.objects.filter( entity__learning_package_id=learning_package_id, version__isnull=False, ) -def get_entities_with_unpublished_changes(learning_package_id: int): +def get_entities_with_unpublished_changes(learning_package_id: int) -> QuerySet[PublishableEntity]: return ( PublishableEntity .objects @@ -185,7 +185,7 @@ def get_entities_with_unpublished_changes(learning_package_id: int): .exclude(draft__version=F('published__version')) ) -def get_entities_with_unpublished_deletes(learning_package_id: int): +def get_entities_with_unpublished_deletes(learning_package_id: int) -> QuerySet[PublishableEntity]: """ Something will become "deleted" if it has a null Draft version but a not-null Published version. (If both are null, it means it's already been @@ -206,7 +206,7 @@ def publish_all_drafts( message="", published_at: datetime | None = None, published_by: int | None = None -): +) -> PublishLog: """ Publish everything that is a Draft and is not already published. """ @@ -312,7 +312,7 @@ def set_draft_version(publishable_entity_id: int, publishable_entity_version_pk: draft.save() -def soft_delete_draft(publishable_entity_id: int): +def soft_delete_draft(publishable_entity_id: int) -> None: draft = Draft.objects.get(entity_id=publishable_entity_id) draft.version_id = None draft.save() @@ -349,7 +349,7 @@ def reset_drafts_to_published(learning_package_id: int) -> None: def register_content_models( content_model_cls: type[PublishableEntityMixin], content_version_model_cls: type[PublishableEntityVersionMixin], -): +) -> PublishableContentModelRegistry: """ Register what content model maps to what content version model. From 19ed2ad6ed18ae5243f378bcb763a28a848b79e3 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Sat, 27 Jan 2024 16:16:40 -0500 Subject: [PATCH 07/23] fix: improve comments and fix annotation error --- openedx_learning/core/components/api.py | 2 +- openedx_learning/core/components/models.py | 7 +++++-- tests/openedx_learning/core/components/test_api.py | 8 ++++++-- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/openedx_learning/core/components/api.py b/openedx_learning/core/components/api.py index 271f26978..803226c7e 100644 --- a/openedx_learning/core/components/api.py +++ b/openedx_learning/core/components/api.py @@ -76,7 +76,7 @@ def create_component_version( def create_next_version( component_pk: int, title: str, - content_to_replace: dict[str: int | None], + content_to_replace: dict[str, int | None], created: datetime, created_by: int | None = None, ) -> ComponentVersion: diff --git a/openedx_learning/core/components/models.py b/openedx_learning/core/components/models.py index 3cf81a687..343f0c959 100644 --- a/openedx_learning/core/components/models.py +++ b/openedx_learning/core/components/models.py @@ -49,8 +49,11 @@ class Component(PublishableEntityMixin): # type: ignore[django-manager-missing] A Component belongs to exactly one LearningPackage. - A Component's is 1:1 with the same primary key values as the - PublishableEntity it uses for draft/publishing operations. + A Component is 1:1 with PublishableEntity and has matching primary key + values. More specifically, ``Component.pk`` maps to + ``Component.publishable_entity_id``, and any place where the Publishing API + module expects to get a ``PublishableEntity.id``, you can use a + ``Component.pk`` instead. Identifiers ----------- diff --git a/tests/openedx_learning/core/components/test_api.py b/tests/openedx_learning/core/components/test_api.py index 79e0bf54c..76c66b074 100644 --- a/tests/openedx_learning/core/components/test_api.py +++ b/tests/openedx_learning/core/components/test_api.py @@ -13,12 +13,16 @@ class TestPerformance(TestCase): """ - Performance related tests to make sure we don't get n + 1 queries. + Performance related tests for Components. + + These are mostly to ensure that when Components are fetched, they're fetched + with a select_related on the most commonly queried things; draft and + published version metadata. """ @classmethod def setUpTestData(cls) -> None: """ - Initialize our content data (all our tests are read only). + Initialize our base learning package. We don't actually need to add content to the ComponentVersions, since for this we only care about the metadata on Compnents, their versions, From 9f47e36260d810c2496fb350a006c6cc5c658e67 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Sat, 27 Jan 2024 17:00:21 -0500 Subject: [PATCH 08/23] fix; differentiate draft and published title search --- openedx_learning/core/components/api.py | 15 +++++-- .../core/components/test_api.py | 39 +++++++++++++------ 2 files changed, 39 insertions(+), 15 deletions(-) diff --git a/openedx_learning/core/components/api.py b/openedx_learning/core/components/api.py index 803226c7e..758a43bc4 100644 --- a/openedx_learning/core/components/api.py +++ b/openedx_learning/core/components/api.py @@ -217,7 +217,7 @@ def component_exists_by_key( namespace: str, type: str, # pylint: disable=redefined-builtin local_key: str -): +) -> bool: """ Return True/False for whether a Component exists. @@ -241,7 +241,8 @@ def get_components( published: bool | None = None, namespace: str | None = None, types: list[str] | None = None, - title: str | None = None, + draft_title: str | None = None, + published_title: str | None = None, ) -> QuerySet: """ Fetch a QuerySet of Components for a LearningPackage using various filters. @@ -264,8 +265,14 @@ def get_components( qset = qset.filter(namespace=namespace) if types is not None: qset = qset.filter(type__in=types) - if title is not None: - qset = qset.filter(publishable_entity__draft__version__title__icontains=title) + if draft_title is not None: + qset = qset.filter( + publishable_entity__draft__version__title__icontains=draft_title + ) + if published_title is not None: + qset = qset.filter( + publishable_entity__published__version__title__icontains=published_title + ) return qset diff --git a/tests/openedx_learning/core/components/test_api.py b/tests/openedx_learning/core/components/test_api.py index 76c66b074..4d9fbd672 100644 --- a/tests/openedx_learning/core/components/test_api.py +++ b/tests/openedx_learning/core/components/test_api.py @@ -141,7 +141,7 @@ def test_no_filters(self): """ Test that we pull back everything, even unpublished or "deleted" items. """ - all_components = components_api.get_components(self.learning_package.id).all() + all_components = components_api.get_components(self.learning_package.id) assert list(all_components) == [ self.published_problem, self.published_html, @@ -157,7 +157,7 @@ def test_draft_filter(self): components_with_draft_version = components_api.get_components( self.learning_package.id, draft=True, - ).all() + ) assert list(components_with_draft_version) == [ self.published_problem, self.published_html, @@ -168,7 +168,7 @@ def test_draft_filter(self): components_without_draft_version = components_api.get_components( self.learning_package.id, draft=False, - ).all() + ) assert list(components_without_draft_version) == [ self.deleted_video ] @@ -180,7 +180,7 @@ def test_published_filter(self): components_with_published_version = components_api.get_components( self.learning_package.id, published=True, - ).all() + ) assert list(components_with_published_version) == [ self.published_problem, self.published_html, @@ -188,7 +188,7 @@ def test_published_filter(self): components_without_published_version = components_api.get_components( self.learning_package.id, published=False, - ).all() + ) assert list(components_without_published_version) == [ self.unpublished_problem, self.unpublished_html, @@ -205,7 +205,7 @@ def test_namespace_filter(self): components_with_xblock_v2 = components_api.get_components( self.learning_package.id, namespace='xblock.v2', - ).all() + ) assert list(components_with_xblock_v2) == [ self.published_problem, self.unpublished_problem, @@ -218,14 +218,14 @@ def test_types_filter(self): html_and_video_components = components_api.get_components( self.learning_package.id, types=['html', 'video'] - ).all() + ) assert list(html_and_video_components) == [ self.published_html, self.unpublished_html, self.deleted_video, ] - def test_title_filter(self): + def test_draft_title_filter(self): """ Test the title filter. @@ -233,9 +233,9 @@ def test_title_filter(self): """ components = components_api.get_components( self.learning_package.id, - title="PUBLISHED" - ).all() - # These all have a title with "published" in it somewhere. + draft_title="PUBLISHED" + ) + # These all have a draft title with "published" in it somewhere. assert list(components) == [ self.published_problem, self.published_html, @@ -243,6 +243,23 @@ def test_title_filter(self): self.unpublished_html, ] + def test_published_title_filter(self): + """ + Test the title filter. + + Note that this should be doing a case-insensitive match. + """ + components = components_api.get_components( + self.learning_package.id, + published_title="problem" + ) + # These all have a published title with "problem" in it somewhere, + # meaning that it won't pick up the components that only exist as + # drafts. + assert list(components) == [ + self.published_problem, + ] + class TestComponentGetAndExists(TestCase): """ From 3639c149fb532cc1548da9122da98e0a387625f0 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Sat, 27 Jan 2024 17:05:31 -0500 Subject: [PATCH 09/23] fix: linter error on missing test docstring --- tests/openedx_learning/core/components/test_api.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/openedx_learning/core/components/test_api.py b/tests/openedx_learning/core/components/test_api.py index 4d9fbd672..a9fb8313f 100644 --- a/tests/openedx_learning/core/components/test_api.py +++ b/tests/openedx_learning/core/components/test_api.py @@ -332,6 +332,9 @@ def test_exists_by_key(self): class TestCreateNewVersions(TestCase): + """ + Create new ComponentVersions in various ways. + """ @classmethod def setUpTestData(cls) -> None: From 3270d2d90056852493065652d34d7be44fac4e23 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Sat, 27 Jan 2024 17:38:32 -0500 Subject: [PATCH 10/23] fix: correct annotations --- openedx_learning/core/components/api.py | 2 +- .../core/components/test_api.py | 21 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/openedx_learning/core/components/api.py b/openedx_learning/core/components/api.py index 758a43bc4..df51a99c2 100644 --- a/openedx_learning/core/components/api.py +++ b/openedx_learning/core/components/api.py @@ -167,7 +167,7 @@ def create_component_and_version( title: str, created: datetime, created_by: int | None = None, -) -> (Component, ComponentVersion): +) -> tuple[Component, ComponentVersion]: """ Create a Component and associated ComponentVersion atomically """ diff --git a/tests/openedx_learning/core/components/test_api.py b/tests/openedx_learning/core/components/test_api.py index a9fb8313f..07a189383 100644 --- a/tests/openedx_learning/core/components/test_api.py +++ b/tests/openedx_learning/core/components/test_api.py @@ -5,6 +5,9 @@ from django.core.exceptions import ObjectDoesNotExist +from openedx_learning.core.publishing.models import LearningPackage +from openedx_learning.core.components.models import Component, ComponentVersion + from openedx_learning.core.components import api as components_api from openedx_learning.core.contents import api as contents_api from openedx_learning.core.publishing import api as publishing_api @@ -19,6 +22,9 @@ class TestPerformance(TestCase): with a select_related on the most commonly queried things; draft and published version metadata. """ + learning_package: LearningPackage + now: datetime + @classmethod def setUpTestData(cls) -> None: """ @@ -64,6 +70,13 @@ class TestGetComponents(TestCase): """ Test grabbing a queryset of Components. """ + learning_package: LearningPackage + now: datetime + published_problem: Component + published_html: Component + unpublished_problem: Component + unpublished_html: Component + deleted_video: Component @classmethod def setUpTestData(cls) -> None: @@ -265,6 +278,11 @@ class TestComponentGetAndExists(TestCase): """ Test getting a Component by primary key or key string. """ + learning_package: LearningPackage + now: datetime + problem: Component + html: Component + @classmethod def setUpTestData(cls) -> None: """ @@ -335,6 +353,9 @@ class TestCreateNewVersions(TestCase): """ Create new ComponentVersions in various ways. """ + learning_package: LearningPackage + now: datetime + problem: Component @classmethod def setUpTestData(cls) -> None: From cbb8a93eaa1d2582e4e183ea3d5ac1795b040273 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Sat, 27 Jan 2024 17:42:47 -0500 Subject: [PATCH 11/23] fix: remove unnecessary import --- tests/openedx_learning/core/components/test_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/openedx_learning/core/components/test_api.py b/tests/openedx_learning/core/components/test_api.py index 07a189383..539ba1f93 100644 --- a/tests/openedx_learning/core/components/test_api.py +++ b/tests/openedx_learning/core/components/test_api.py @@ -6,7 +6,7 @@ from django.core.exceptions import ObjectDoesNotExist from openedx_learning.core.publishing.models import LearningPackage -from openedx_learning.core.components.models import Component, ComponentVersion +from openedx_learning.core.components.models import Component from openedx_learning.core.components import api as components_api from openedx_learning.core.contents import api as contents_api From 69b8874a3333833de5b254644e2a91ab626ce2b2 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 29 Jan 2024 08:58:20 -0500 Subject: [PATCH 12/23] refactor: soft delete changes, comments, test names --- openedx_learning/core/publishing/api.py | 48 +++++++++++++++---- .../core/components/test_api.py | 8 ++-- .../core/publishing/test_api.py | 33 +++++++++---- 3 files changed, 66 insertions(+), 23 deletions(-) diff --git a/openedx_learning/core/publishing/api.py b/openedx_learning/core/publishing/api.py index a6f9e9199..ec486cf3d 100644 --- a/openedx_learning/core/publishing/api.py +++ b/openedx_learning/core/publishing/api.py @@ -12,7 +12,11 @@ from django.db.models import F, QuerySet from django.db.transaction import atomic -from .model_mixins import PublishableContentModelRegistry, PublishableEntityMixin, PublishableEntityVersionMixin +from .model_mixins import ( + PublishableContentModelRegistry, + PublishableEntityMixin, + PublishableEntityVersionMixin, +) from .models import ( Draft, LearningPackage, @@ -296,16 +300,15 @@ def get_draft_version(publishable_entity_id: int) -> PublishableEntityVersion | return draft.version -def set_draft_version(publishable_entity_id: int, publishable_entity_version_pk: int) -> None: +def set_draft_version(publishable_entity_id: int, publishable_entity_version_pk: int | None) -> None: """ Modify the Draft of a PublishableEntity to be a PublishableEntityVersion. This would most commonly be used to set the Draft to point to a newly created PublishableEntityVersion that was created in Studio (because someone edited some content). Setting a Draft's version to None is like deleting it - from Studio's editing point of view. We don't actually delete the Draft row - because we'll need that for publishing purposes (i.e. to delete content from - the published branch). + from Studio's editing point of view (see ``soft_delete_draft`` for more + details). """ draft = Draft.objects.get(entity_id=publishable_entity_id) draft.version_id = publishable_entity_version_pk @@ -313,9 +316,16 @@ def set_draft_version(publishable_entity_id: int, publishable_entity_version_pk: def soft_delete_draft(publishable_entity_id: int) -> None: - draft = Draft.objects.get(entity_id=publishable_entity_id) - draft.version_id = None - draft.save() + """ + Sets the Draft version to None. + + This "deletes" the ``PublishableEntity`` from the point of an authoring + environment like Studio, but doesn't immediately remove the ``Published`` + version. No version data is actually deleted, so restoring is just a matter + of pointing the Draft back to the most recent ``PublishableEntityVersion`` + for a given ``PublishableEntity``. + """ + return set_draft_version(publishable_entity_id, None) def reset_drafts_to_published(learning_package_id: int) -> None: @@ -323,9 +333,27 @@ def reset_drafts_to_published(learning_package_id: int) -> None: Reset all Drafts to point to the most recently Published versions. This is a way to say "discard my unpublished changes" at the level of an - entire LearningPackage. + entire LearningPackage. Note that the ``PublishableEntityVersions`` that + were created in the mean time are not deleted. + + Let's look at the example of a PublishableEntity where Draft and Published + both point to version 4. + + * The PublishableEntity is then edited multiple times, creating a new + version with every save. Each new save also updates the the Draft to point + to it. After three such saves, Draft points at version 7. + * No new publishes have happened, so Published still points to version 4. + * ``reset_drafts_to_published`` is called. Draft now points to version 4 to + match Published. + * The PublishableEntity is edited again. This creates version 8, and Draft + now points to this new version. + + So in the above example, versions 5-7 aren't discarded from the history, and + it's important that the code creating the "next" version_num looks at the + latest version created for a PublishableEntity (its ``latest`` attribute), + rather than basing it off of the version that Draft points to. """ - # These are all the drafts that are different from the publisehd versions. + # These are all the drafts that are different from the published versions. draft_qset = ( Draft .objects diff --git a/tests/openedx_learning/core/components/test_api.py b/tests/openedx_learning/core/components/test_api.py index 539ba1f93..ff8a1fcae 100644 --- a/tests/openedx_learning/core/components/test_api.py +++ b/tests/openedx_learning/core/components/test_api.py @@ -14,7 +14,7 @@ from openedx_learning.lib.test_utils import TestCase -class TestPerformance(TestCase): +class PerformanceTestCase(TestCase): """ Performance related tests for Components. @@ -66,7 +66,7 @@ def test_component_num_queries(self) -> None: published = component.versioning.published assert draft.title == published.title -class TestGetComponents(TestCase): +class GetComponentsTestCase(TestCase): """ Test grabbing a queryset of Components. """ @@ -274,7 +274,7 @@ def test_published_title_filter(self): ] -class TestComponentGetAndExists(TestCase): +class ComponentGetAndExistsTestCase(TestCase): """ Test getting a Component by primary key or key string. """ @@ -349,7 +349,7 @@ def test_exists_by_key(self): ) -class TestCreateNewVersions(TestCase): +class CreateNewVersionsTestCase(TestCase): """ Create new ComponentVersions in various ways. """ diff --git a/tests/openedx_learning/core/publishing/test_api.py b/tests/openedx_learning/core/publishing/test_api.py index df4809d4e..b04ccfedf 100644 --- a/tests/openedx_learning/core/publishing/test_api.py +++ b/tests/openedx_learning/core/publishing/test_api.py @@ -8,6 +8,7 @@ from django.core.exceptions import ValidationError from openedx_learning.core.publishing import api as publishing_api +from openedx_learning.core.publishing.models import LearningPackage from openedx_learning.lib.test_utils import TestCase @@ -94,21 +95,26 @@ class DraftTestCase(TestCase): """ Test basic operations with Drafts. """ + now: datetime + learning_package: LearningPackage + + @classmethod + def setUpTestData(cls) -> None: + cls.now = datetime(2024, 1, 28, 16, 45, 30, tzinfo=timezone.utc) + cls.learning_package = publishing_api.create_learning_package( + "my_package_key", + "Draft Testing LearningPackage 🔥", + created=cls.now, + ) def test_draft_lifecycle(self): """ Test basic lifecycle of a Draft. """ - created = datetime(2023, 4, 2, 15, 9, 0, tzinfo=timezone.utc) - package = publishing_api.create_learning_package( - "my_package_key", - "Draft Testing LearningPackage 🔥", - created=created, - ) entity = publishing_api.create_publishable_entity( - package.id, + self.learning_package.id, "my_entity", - created, + created=self.now, created_by=None, ) # Drafts are NOT created when a PublishableEntity is created, only when @@ -119,7 +125,7 @@ def test_draft_lifecycle(self): entity_id=entity.id, version_num=1, title="An Entity 🌴", - created=created, + created=self.now, created_by=None, ) assert entity_version == publishing_api.get_draft_version(entity.id) @@ -129,3 +135,12 @@ def test_draft_lifecycle(self): publishing_api.set_draft_version(entity.id, None) entity_version = publishing_api.get_draft_version(entity.id) assert entity_version is None + + def test_reset_drafts_to_published(self): + """ + Test throwing out Draft data and resetting to the Published versions. + + One place this might turn up is if we've imported an older version of + the library and it causes a bunch of new versions to be created. + """ + From 66c7a671f2fcf52f790fcb64d9240986ca805d63 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 29 Jan 2024 10:37:17 -0500 Subject: [PATCH 13/23] test: test reset-to-published functionality --- openedx_learning/core/publishing/api.py | 6 + .../core/publishing/test_api.py | 145 +++++++++++++++++- 2 files changed, 147 insertions(+), 4 deletions(-) diff --git a/openedx_learning/core/publishing/api.py b/openedx_learning/core/publishing/api.py index ec486cf3d..3a6342459 100644 --- a/openedx_learning/core/publishing/api.py +++ b/openedx_learning/core/publishing/api.py @@ -153,6 +153,9 @@ def create_publishable_entity_version( ) return version +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: return PublishableEntity.objects.get( learning_package_id=learning_package_id, @@ -352,6 +355,9 @@ def reset_drafts_to_published(learning_package_id: int) -> None: it's important that the code creating the "next" version_num looks at the latest version created for a PublishableEntity (its ``latest`` attribute), rather than basing it off of the version that Draft points to. + + Also, there is no current immutable record for when a reset happens. It's + not like a publish that leaves an entry in the ``PublishLog``. """ # These are all the drafts that are different from the published versions. draft_qset = ( diff --git a/tests/openedx_learning/core/publishing/test_api.py b/tests/openedx_learning/core/publishing/test_api.py index b04ccfedf..747e78c95 100644 --- a/tests/openedx_learning/core/publishing/test_api.py +++ b/tests/openedx_learning/core/publishing/test_api.py @@ -1,6 +1,8 @@ """ Tests of the Publishing app's python API """ +from __future__ import annotations + from datetime import datetime, timezone from uuid import UUID @@ -8,7 +10,7 @@ from django.core.exceptions import ValidationError from openedx_learning.core.publishing import api as publishing_api -from openedx_learning.core.publishing.models import LearningPackage +from openedx_learning.core.publishing.models import LearningPackage, PublishableEntity from openedx_learning.lib.test_utils import TestCase @@ -107,7 +109,7 @@ def setUpTestData(cls) -> None: created=cls.now, ) - def test_draft_lifecycle(self): + def test_draft_lifecycle(self) -> None: """ Test basic lifecycle of a Draft. """ @@ -122,7 +124,7 @@ def test_draft_lifecycle(self): assert publishing_api.get_draft_version(entity.id) is None entity_version = publishing_api.create_publishable_entity_version( - entity_id=entity.id, + entity.id, version_num=1, title="An Entity 🌴", created=self.now, @@ -136,11 +138,146 @@ def test_draft_lifecycle(self): entity_version = publishing_api.get_draft_version(entity.id) assert entity_version is None - def test_reset_drafts_to_published(self): + def test_reset_drafts_to_published(self) -> None: """ Test throwing out Draft data and resetting to the Published versions. One place this might turn up is if we've imported an older version of the library and it causes a bunch of new versions to be created. + + Note that these tests don't associate any content with the versions + being created. They don't have to, because making those content + associations is the job of the ``components`` package, and potentially + other higher-level things. We're never deleting ``PublishableEntity`` + or ``PublishableEntityVersion`` instances, so we don't have to worry + about potentially breaking the associated models of those higher level + apps. These tests just need to ensure that the Published and Draft + models are updated properly to point to the correct versions. + + This could be broken up into separate tests for each scenario, but I + wanted to make sure nothing went wrong when multiple types of reset were + happening at the same time. """ + # This is the Entity that's going to get a couple of new versions + # between Draft and Published. + entity_with_changes = publishing_api.create_publishable_entity( + self.learning_package.id, + "entity_with_changes", + created=self.now, + created_by=None, + ) + publishing_api.create_publishable_entity_version( + entity_with_changes.id, + version_num=1, + title="I'm entity_with_changes v1", + created=self.now, + created_by=None, + ) + + # This Entity is going to remain unchanged between Draft and Published. + entity_with_no_changes = publishing_api.create_publishable_entity( + self.learning_package.id, + "entity_with_no_changes", + created=self.now, + created_by=None, + ) + publishing_api.create_publishable_entity_version( + entity_with_no_changes.id, + version_num=1, + title="I'm entity_with_no_changes v1", + created=self.now, + created_by=None, + ) + + # This Entity will be Published, but will then be soft-deleted. + entity_with_pending_delete = publishing_api.create_publishable_entity( + self.learning_package.id, + "entity_with_pending_delete", + created=self.now, + created_by=None, + ) + publishing_api.create_publishable_entity_version( + entity_with_pending_delete.id, + version_num=1, + title="I'm entity_with_pending_delete v1", + created=self.now, + created_by=None, + ) + + # Publish! + publishing_api.publish_all_drafts(self.learning_package.id) + + # Create versions 2, 3, 4 of entity_with_changes. After this, the + # Published version is 1 and the Draft version is 4. + for version_num in range(2, 5): + publishing_api.create_publishable_entity_version( + entity_with_changes.id, + version_num=version_num, + title=f"I'm entity_with_changes v{version_num}", + created=self.now, + created_by=None, + ) + + # Soft-delete entity_with_pending_delete. After this, the Published + # version is 1 and the Draft version is None. + publishing_api.soft_delete_draft(entity_with_pending_delete) + + # Create a new entity that only exists in Draft form (no Published + # version). + new_entity = publishing_api.create_publishable_entity( + self.learning_package.id, + "new_entity", + created=self.now, + created_by=None, + ) + publishing_api.create_publishable_entity_version( + new_entity.id, + version_num=1, + title="I'm new_entity v1", + created=self.now, + created_by=None, + ) + + # The versions we expect in (entity, published version_num, draft + # version_num) tuples. + expected_pre_reset_state = [ + (entity_with_changes, 1, 4), + (entity_with_no_changes, 1, 1), + (entity_with_pending_delete, 1, None), + (new_entity, None, 1), + ] + for entity, pub_version_num, draft_version_num in expected_pre_reset_state: + # Make sure we grab a new copy from the database so we're not + # getting stale cached values: + updated_entity = publishing_api.get_publishable_entity(entity.id) + assert pub_version_num == self._get_published_version_num(updated_entity) + assert draft_version_num == self._get_draft_version_num(updated_entity) + + # Now reset to draft here! + publishing_api.reset_drafts_to_published(self.learning_package.id) + + # Versions we expect after reset in (entity, published version num) + # tuples. The only entity that is not version 1 is the new one. + expected_post_reset_state = [ + (entity_with_changes, 1), + (entity_with_no_changes, 1), + (entity_with_pending_delete, 1), + (new_entity, None), + ] + for entity, pub_version_num in expected_post_reset_state: + updated_entity = publishing_api.get_publishable_entity(entity.id) + assert ( + self._get_published_version_num(updated_entity) == + self._get_draft_version_num(updated_entity) == + pub_version_num + ) + + def _get_published_version_num(self, entity: PublishableEntity) -> int | None: + if hasattr(entity, 'published') and entity.published.version is not None: + return entity.published.version.version_num + return None + def _get_draft_version_num(self, entity) -> int | None: + if hasattr(entity, 'draft') and entity.draft.version is not None: + return entity.draft.version.version_num + return None From 9f57f60088097fbeee46d7eac30ba29c93a8cb86 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 29 Jan 2024 11:02:19 -0500 Subject: [PATCH 14/23] feat: add get_published_version --- openedx_learning/core/publishing/api.py | 13 ++++++++ .../core/publishing/test_api.py | 30 +++++++++---------- 2 files changed, 28 insertions(+), 15 deletions(-) diff --git a/openedx_learning/core/publishing/api.py b/openedx_learning/core/publishing/api.py index 3a6342459..7085f2739 100644 --- a/openedx_learning/core/publishing/api.py +++ b/openedx_learning/core/publishing/api.py @@ -303,6 +303,19 @@ def get_draft_version(publishable_entity_id: int) -> PublishableEntityVersion | return draft.version +def get_published_version(publishable_entity_id: int) -> PublishableEntityVersion | None: + try: + published = Published.objects.select_related("version").get( + entity_id=publishable_entity_id + ) + except Published.DoesNotExist: + return None + + # published.version could be None if something was published at one point, + # had its draft soft deleted, and then was published again. + return published.version + + def set_draft_version(publishable_entity_id: int, publishable_entity_version_pk: int | None) -> None: """ Modify the Draft of a PublishableEntity to be a PublishableEntityVersion. diff --git a/tests/openedx_learning/core/publishing/test_api.py b/tests/openedx_learning/core/publishing/test_api.py index 747e78c95..5c7a89e2a 100644 --- a/tests/openedx_learning/core/publishing/test_api.py +++ b/tests/openedx_learning/core/publishing/test_api.py @@ -134,9 +134,9 @@ def test_draft_lifecycle(self) -> None: # We never really remove rows from the table holding Drafts. We just # mark the version as None. - publishing_api.set_draft_version(entity.id, None) - entity_version = publishing_api.get_draft_version(entity.id) - assert entity_version is None + publishing_api.soft_delete_draft(entity.id) + deleted_entity_version = publishing_api.get_draft_version(entity.id) + assert deleted_entity_version is None def test_reset_drafts_to_published(self) -> None: """ @@ -220,7 +220,7 @@ def test_reset_drafts_to_published(self) -> None: # Soft-delete entity_with_pending_delete. After this, the Published # version is 1 and the Draft version is None. - publishing_api.soft_delete_draft(entity_with_pending_delete) + publishing_api.soft_delete_draft(entity_with_pending_delete.id) # Create a new entity that only exists in Draft form (no Published # version). @@ -249,9 +249,8 @@ def test_reset_drafts_to_published(self) -> None: for entity, pub_version_num, draft_version_num in expected_pre_reset_state: # Make sure we grab a new copy from the database so we're not # getting stale cached values: - updated_entity = publishing_api.get_publishable_entity(entity.id) - assert pub_version_num == self._get_published_version_num(updated_entity) - assert draft_version_num == self._get_draft_version_num(updated_entity) + assert pub_version_num == self._get_published_version_num(entity) + assert draft_version_num == self._get_draft_version_num(entity) # Now reset to draft here! publishing_api.reset_drafts_to_published(self.learning_package.id) @@ -265,19 +264,20 @@ def test_reset_drafts_to_published(self) -> None: (new_entity, None), ] for entity, pub_version_num in expected_post_reset_state: - updated_entity = publishing_api.get_publishable_entity(entity.id) assert ( - self._get_published_version_num(updated_entity) == - self._get_draft_version_num(updated_entity) == + self._get_published_version_num(entity) == + self._get_draft_version_num(entity) == pub_version_num ) def _get_published_version_num(self, entity: PublishableEntity) -> int | None: - if hasattr(entity, 'published') and entity.published.version is not None: - return entity.published.version.version_num + published_version = publishing_api.get_published_version(entity.id) + if published_version is not None: + return published_version.version_num return None - def _get_draft_version_num(self, entity) -> int | None: - if hasattr(entity, 'draft') and entity.draft.version is not None: - return entity.draft.version.version_num + def _get_draft_version_num(self, entity: PublishableEntity) -> int | None: + draft_version = publishing_api.get_draft_version(entity.id) + if draft_version is not None: + return draft_version.version_num return None From f62f8c52d3cffbaf2ffe993ef754c125aecc3e7b Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 29 Jan 2024 11:18:22 -0500 Subject: [PATCH 15/23] tests: test LearningPackage modification --- openedx_learning/core/components/api.py | 2 +- openedx_learning/core/publishing/api.py | 5 +++++ .../openedx_learning/core/publishing/test_api.py | 16 +++++++++++++++- 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/openedx_learning/core/components/api.py b/openedx_learning/core/components/api.py index df51a99c2..d8bc1c8f6 100644 --- a/openedx_learning/core/components/api.py +++ b/openedx_learning/core/components/api.py @@ -199,7 +199,7 @@ def get_component_by_key( local_key: str, ) -> Component: """ - Get Componet by it unique namespace/type/local_key tuple. + Get a Component by its unique (namespace, type, local_key) tuple. """ return ( Component diff --git a/openedx_learning/core/publishing/api.py b/openedx_learning/core/publishing/api.py index 7085f2739..ab397e460 100644 --- a/openedx_learning/core/publishing/api.py +++ b/openedx_learning/core/publishing/api.py @@ -304,6 +304,11 @@ def get_draft_version(publishable_entity_id: int) -> PublishableEntityVersion | def get_published_version(publishable_entity_id: int) -> PublishableEntityVersion | None: + """ + Return current published PublishableEntityVersion for this PublishableEntity. + + This function will return None if there is no current published version. + """ try: published = Published.objects.select_related("version").get( entity_id=publishable_entity_id diff --git a/tests/openedx_learning/core/publishing/test_api.py b/tests/openedx_learning/core/publishing/test_api.py index 5c7a89e2a..56c914662 100644 --- a/tests/openedx_learning/core/publishing/test_api.py +++ b/tests/openedx_learning/core/publishing/test_api.py @@ -14,7 +14,7 @@ from openedx_learning.lib.test_utils import TestCase -class CreateLearningPackageTestCase(TestCase): +class LearningPackageTestCase(TestCase): """ Test creating a LearningPackage """ @@ -35,6 +35,7 @@ def test_normal(self) -> None: # Note: we must specify '-> None' to opt in to t assert package.key == "my_key" assert package.title == "My Excellent Title with Emoji 🔥" + assert package.description == "A fun Description!" assert package.created == created assert package.updated == created @@ -44,6 +45,19 @@ def test_normal(self) -> None: # Note: we must specify '-> None' to opt in to t # Having an actual value here means we were persisted to the database. assert isinstance(package.id, int) + # Now test editing the fields. + updated_package = publishing_api.update_learning_package( + package.id, + key="new_key", + title="new title", + description="new description", + ) + assert updated_package.key == "new_key" + assert updated_package.title == "new title" + assert updated_package.description == "new description" + assert updated_package.created == created + assert updated_package.updated != created # new time would be auto-generated + def test_auto_datetime(self) -> None: """ Auto-generated created datetime works as expected. From e810163d19706529316b074fae6cceb7af272970 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 29 Jan 2024 11:55:44 -0500 Subject: [PATCH 16/23] chore: linter fixes --- openedx_learning/core/components/api.py | 23 ++++---- openedx_learning/core/publishing/api.py | 57 +++++++++---------- .../core/publishing/model_mixins.py | 6 +- 3 files changed, 42 insertions(+), 44 deletions(-) diff --git a/openedx_learning/core/components/api.py b/openedx_learning/core/components/api.py index d8bc1c8f6..0324e37fb 100644 --- a/openedx_learning/core/components/api.py +++ b/openedx_learning/core/components/api.py @@ -142,11 +142,8 @@ def create_next_version( ) # Now copy any old associations that existed, as long as they don't # conflict with the new stuff. - last_version_content_mapping = ( - ComponentVersionRawContent - .objects - .filter(component_version=last_version) - ) + last_version_content_mapping = ComponentVersionRawContent.objects \ + .filter(component_version=last_version) for cvrc in last_version_content_mapping: if cvrc.key not in content_to_replace: ComponentVersionRawContent.objects.create( @@ -184,6 +181,7 @@ def create_component_and_version( ) return (component, component_version) + def get_component(component_pk: int) -> Component: """ Get Component by its primary key. @@ -192,6 +190,7 @@ def get_component(component_pk: int) -> Component: """ return Component.with_publishing_relations.get(pk=component_pk) + def get_component_by_key( learning_package_id: int, namespace: str, @@ -212,6 +211,7 @@ def get_component_by_key( ) ) + def component_exists_by_key( learning_package_id: int, namespace: str, @@ -235,6 +235,7 @@ def component_exists_by_key( except Component.DoesNotExist: return False + def get_components( learning_package_id: int, draft: bool | None = None, @@ -251,12 +252,10 @@ def get_components( info from the Component's draft and published versions, since we'll be referencing these a lot. """ - qset = ( - Component - .with_publishing_relations - .filter(learning_package_id=learning_package_id) - .order_by('pk') - ) + qset = Component.with_publishing_relations \ + .filter(learning_package_id=learning_package_id) \ + .order_by('pk') + if draft is not None: qset = qset.filter(publishable_entity__draft__version__isnull = not draft) if published is not None: @@ -276,6 +275,7 @@ def get_components( return qset + def get_component_version_content( learning_package_key: str, component_key: str, @@ -307,6 +307,7 @@ def get_component_version_content( ) ) + def add_content_to_component_version( component_version_id: int, raw_content_id: int, diff --git a/openedx_learning/core/publishing/api.py b/openedx_learning/core/publishing/api.py index ab397e460..49e6a3a1a 100644 --- a/openedx_learning/core/publishing/api.py +++ b/openedx_learning/core/publishing/api.py @@ -34,6 +34,7 @@ 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: """ Get LearningPackage by key. @@ -42,6 +43,7 @@ def get_learning_package_by_key(key: str) -> LearningPackage: """ return LearningPackage.objects.get(key=key) + def create_learning_package( key: str, title: str, description: str = "", created: datetime | None = None ) -> LearningPackage: @@ -69,6 +71,7 @@ def create_learning_package( return package + def update_learning_package( learning_package_id: int, key: str | None = None, @@ -153,15 +156,18 @@ def create_publishable_entity_version( ) return version + 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: return PublishableEntity.objects.get( learning_package_id=learning_package_id, key=key, ) + def learning_package_exists(key: str) -> bool: """ Check whether a LearningPackage with a particular key exists. @@ -178,19 +184,19 @@ def get_last_publish(learning_package_id: int) -> PublishLog | None: .first() ) + def get_all_drafts(learning_package_id: int) -> QuerySet[Draft]: return Draft.objects.filter( entity__learning_package_id=learning_package_id, version__isnull=False, ) + def get_entities_with_unpublished_changes(learning_package_id: int) -> QuerySet[PublishableEntity]: - return ( - PublishableEntity - .objects - .filter(learning_package_id=learning_package_id) - .exclude(draft__version=F('published__version')) - ) + return PublishableEntity.objects \ + .filter(learning_package_id=learning_package_id) \ + .exclude(draft__version=F('published__version')) + def get_entities_with_unpublished_deletes(learning_package_id: int) -> QuerySet[PublishableEntity]: """ @@ -198,15 +204,13 @@ def get_entities_with_unpublished_deletes(learning_package_id: int) -> QuerySet[ not-null Published version. (If both are null, it means it's already been deleted in a previous publish, or it was never published.) """ - return ( - PublishableEntity - .objects - .filter( - learning_package_id=learning_package_id, - draft__version__isnull=True, - ) - .exclude(published__version__isnull=True) - ) + return PublishableEntity.objects \ + .filter( + learning_package_id=learning_package_id, + draft__version__isnull=True, + ) \ + .exclude(published__version__isnull=True) + def publish_all_drafts( learning_package_id: int, @@ -217,13 +221,10 @@ def publish_all_drafts( """ Publish everything that is a Draft and is not already published. """ - draft_qset = ( - Draft - .objects - .select_related("entity__published") - .filter(entity__learning_package_id=learning_package_id) - .exclude(entity__published__version_id=F("version_id")) - ) + draft_qset = Draft.objects \ + .select_related("entity__published") \ + .filter(entity__learning_package_id=learning_package_id) \ + .exclude(entity__published__version_id=F("version_id")) return publish_from_drafts( learning_package_id, draft_qset, message, published_at, published_by ) @@ -378,13 +379,11 @@ def reset_drafts_to_published(learning_package_id: int) -> None: not like a publish that leaves an entry in the ``PublishLog``. """ # These are all the drafts that are different from the published versions. - draft_qset = ( - Draft - .objects - .select_related("entity__published") - .filter(entity__learning_package_id=learning_package_id) - .exclude(entity__published__version_id=F("version_id")) - ) + draft_qset = Draft.objects \ + .select_related("entity__published") \ + .filter(entity__learning_package_id=learning_package_id) \ + .exclude(entity__published__version_id=F("version_id")) + # Note: We can't do an .update with a F() on a joined field in the ORM, so # we have to loop through the drafts individually to reset them. We can # rework this into a bulk update or custom SQL if it becomes a performance diff --git a/openedx_learning/core/publishing/model_mixins.py b/openedx_learning/core/publishing/model_mixins.py index bba7f0c18..ce32cd45e 100644 --- a/openedx_learning/core/publishing/model_mixins.py +++ b/openedx_learning/core/publishing/model_mixins.py @@ -119,10 +119,8 @@ class VersioningHelper: def __init__(self, content_obj): self.content_obj = content_obj - self.content_version_model_cls = ( - PublishableContentModelRegistry.get_versioned_model_cls( - type(content_obj) - ) + self.content_version_model_cls = PublishableContentModelRegistry.get_versioned_model_cls( + type(content_obj) ) # Get the field that points from the *versioned* content model # (e.g. ComponentVersion) to the PublishableEntityVersion. From 5cf43d2a297581f53f5bbfa1f50d03d7801fb91c Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 29 Jan 2024 12:06:50 -0500 Subject: [PATCH 17/23] chore: linter fixes --- openedx_learning/__init__.py | 2 +- openedx_learning/core/components/api.py | 65 +++++++++---------- openedx_learning/core/publishing/api.py | 29 ++++----- .../core/components/test_api.py | 43 +++++------- 4 files changed, 62 insertions(+), 77 deletions(-) diff --git a/openedx_learning/__init__.py b/openedx_learning/__init__.py index 13243d24e..0dc1d5053 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -1,4 +1,4 @@ """ Open edX Learning ("Learning Core"). """ -__version__ = "0.4.4" +__version__ = "0.5.0" diff --git a/openedx_learning/core/components/api.py b/openedx_learning/core/components/api.py index 0324e37fb..a38515900 100644 --- a/openedx_learning/core/components/api.py +++ b/openedx_learning/core/components/api.py @@ -78,7 +78,7 @@ def create_next_version( title: str, content_to_replace: dict[str, int | None], created: datetime, - created_by: int | None = None, + created_by: int | None=None, ) -> ComponentVersion: """ Create a new ComponentVersion based on the most recent version. @@ -163,7 +163,7 @@ def create_component_and_version( local_key: str, title: str, created: datetime, - created_by: int | None = None, + created_by: int | None=None, ) -> tuple[Component, ComponentVersion]: """ Create a Component and associated ComponentVersion atomically @@ -200,16 +200,13 @@ def get_component_by_key( """ Get a Component by its unique (namespace, type, local_key) tuple. """ - return ( - Component - .with_publishing_relations - .get( - learning_package_id=learning_package_id, - namespace=namespace, - type=type, - local_key=local_key, - ) - ) + return Component.with_publishing_relations \ + .get( + learning_package_id=learning_package_id, + namespace=namespace, + type=type, + local_key=local_key, + ) def component_exists_by_key( @@ -238,12 +235,12 @@ def component_exists_by_key( def get_components( learning_package_id: int, - draft: bool | None = None, - published: bool | None = None, - namespace: str | None = None, - types: list[str] | None = None, - draft_title: str | None = None, - published_title: str | None = None, + draft: bool | None=None, + published: bool | None=None, + namespace: str | None=None, + types: list[str] | None=None, + draft_title: str | None=None, + published_title: str | None=None, ) -> QuerySet: """ Fetch a QuerySet of Components for a LearningPackage using various filters. @@ -288,24 +285,22 @@ def get_component_version_content( Can raise a django.core.exceptions.ObjectDoesNotExist error if there is no matching ComponentVersionRawContent. """ - return ( - ComponentVersionRawContent - .objects - .select_related( - "raw_content", - "raw_content__media_type", - "raw_content__textcontent", - "component_version", - "component_version__component", - "component_version__component__learning_package", - ) - .get( - Q(component_version__component__learning_package__key=learning_package_key) - & Q(component_version__component__publishable_entity__key=component_key) - & Q(component_version__publishable_entity_version__version_num=version_num) - & Q(key=key) - ) + queries = ( + Q(component_version__component__learning_package__key=learning_package_key) + & Q(component_version__component__publishable_entity__key=component_key) + & Q(component_version__publishable_entity_version__version_num=version_num) + & Q(key=key) ) + return ComponentVersionRawContent.objects \ + .select_related( + "raw_content", + "raw_content__media_type", + "raw_content__textcontent", + "component_version", + "component_version__component", + "component_version__component__learning_package", + ) \ + .get(queries) def add_content_to_component_version( diff --git a/openedx_learning/core/publishing/api.py b/openedx_learning/core/publishing/api.py index 49e6a3a1a..a1f7851c6 100644 --- a/openedx_learning/core/publishing/api.py +++ b/openedx_learning/core/publishing/api.py @@ -45,7 +45,7 @@ def get_learning_package_by_key(key: str) -> LearningPackage: def create_learning_package( - key: str, title: str, description: str = "", created: datetime | None = None + key: str, title: str, description: str="", created: datetime | None=None ) -> LearningPackage: """ Create a new LearningPackage. @@ -74,10 +74,10 @@ def create_learning_package( def update_learning_package( learning_package_id: int, - key: str | None = None, - title: str | None = None, - description: str | None = None, - updated: datetime | None = None, + key: str | None=None, + title: str | None=None, + description: str | None=None, + updated: datetime | None=None, ) -> LearningPackage: """ Make an update to LearningPackage metadata. @@ -176,13 +176,10 @@ def learning_package_exists(key: str) -> bool: def get_last_publish(learning_package_id: int) -> PublishLog | None: - return ( - PublishLog - .objects - .filter(learning_package_id=learning_package_id) - .order_by('-id') - .first() - ) + return PublishLog.objects \ + .filter(learning_package_id=learning_package_id) \ + .order_by('-id') \ + .first() def get_all_drafts(learning_package_id: int) -> QuerySet[Draft]: @@ -215,8 +212,8 @@ def get_entities_with_unpublished_deletes(learning_package_id: int) -> QuerySet[ def publish_all_drafts( learning_package_id: int, message="", - published_at: datetime | None = None, - published_by: int | None = None + published_at: datetime | None=None, + published_by: int | None=None ) -> PublishLog: """ Publish everything that is a Draft and is not already published. @@ -234,8 +231,8 @@ def publish_from_drafts( learning_package_id: int, # LearningPackage.id draft_qset: QuerySet, message: str = "", - published_at: datetime | None = None, - published_by: int | None = None, # User.id + published_at: datetime | None=None, + published_by: int | None=None, # User.id ) -> PublishLog: """ Publish the rows in the ``draft_model_qsets`` args passed in. diff --git a/tests/openedx_learning/core/components/test_api.py b/tests/openedx_learning/core/components/test_api.py index ff8a1fcae..2d26e2251 100644 --- a/tests/openedx_learning/core/components/test_api.py +++ b/tests/openedx_learning/core/components/test_api.py @@ -66,6 +66,7 @@ def test_component_num_queries(self) -> None: published = component.versioning.published assert draft.title == published.title + class GetComponentsTestCase(TestCase): """ Test grabbing a queryset of Components. @@ -139,7 +140,7 @@ def setUpTestData(cls) -> None: # Component we're putting here to soft delete (this will remove the # Draft entry) - cls.deleted_video, _version = components_api.create_component_and_version( + cls.deleted_video, _version = components_api.create_component_and_version( learning_package_id=cls.learning_package.id, namespace="xblock.v1", type="html", @@ -228,7 +229,7 @@ def test_types_filter(self): """ Test the types filter. """ - html_and_video_components = components_api.get_components( + html_and_video_components = components_api.get_components( self.learning_package.id, types=['html', 'video'] ) @@ -394,18 +395,14 @@ def test_add(self): learner_downloadable=False, ) # re-fetch from the database to check to see if we wrote it correctly - new_version = ( - components_api - .get_component(self.problem.pk) - .versions - .get(publishable_entity_version__version_num=1) - ) + new_version = components_api.get_component(self.problem.pk) \ + .versions \ + .get(publishable_entity_version__version_num=1) assert ( new_content == new_version.raw_contents.get(componentversionrawcontent__key="hello.txt") ) - def test_multiple_versions(self): hello_content, _created = contents_api.get_or_create_text_content_from_bytes( learning_package_id=self.learning_package.id, @@ -468,24 +465,21 @@ def test_multiple_versions(self): assert version_2.raw_contents.count() == 3 assert ( blank_content == - version_2 - .raw_contents - .get(componentversionrawcontent__key="hello.txt") - .text_content + version_2.raw_contents + .get(componentversionrawcontent__key="hello.txt") + .text_content ) assert ( goodbye_content == - version_2 - .raw_contents - .get(componentversionrawcontent__key="goodbye.txt") - .text_content + version_2.raw_contents + .get(componentversionrawcontent__key="goodbye.txt") + .text_content ) assert ( blank_content == - version_2 - .raw_contents - .get(componentversionrawcontent__key="blank.txt") - .text_content + version_2.raw_contents + .get(componentversionrawcontent__key="blank.txt") + .text_content ) # Now we're going to set "hello.txt" back to hello_content, but remove @@ -505,8 +499,7 @@ def test_multiple_versions(self): assert version_3.raw_contents.count() == 1 assert ( hello_content == - version_3 - .raw_contents - .get(componentversionrawcontent__key="hello.txt") - .text_content + version_3.raw_contents + .get(componentversionrawcontent__key="hello.txt") + .text_content ) From 2b13a254b39162183a385a595696ea3a0fc055e0 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 29 Jan 2024 13:46:56 -0500 Subject: [PATCH 18/23] chore: more linter fixes --- openedx_learning/core/components/api.py | 23 ++++++++-------- openedx_learning/core/components/models.py | 6 +---- openedx_learning/core/publishing/api.py | 27 ++++++++----------- openedx_learning/lib/managers.py | 3 ++- .../core/components/test_api.py | 5 ++-- 5 files changed, 27 insertions(+), 37 deletions(-) diff --git a/openedx_learning/core/components/api.py b/openedx_learning/core/components/api.py index a38515900..f96af9782 100644 --- a/openedx_learning/core/components/api.py +++ b/openedx_learning/core/components/api.py @@ -78,7 +78,7 @@ def create_next_version( title: str, content_to_replace: dict[str, int | None], created: datetime, - created_by: int | None=None, + created_by: int | None = None, ) -> ComponentVersion: """ Create a new ComponentVersion based on the most recent version. @@ -163,7 +163,7 @@ def create_component_and_version( local_key: str, title: str, created: datetime, - created_by: int | None=None, + created_by: int | None = None, ) -> tuple[Component, ComponentVersion]: """ Create a Component and associated ComponentVersion atomically @@ -235,12 +235,12 @@ def component_exists_by_key( def get_components( learning_package_id: int, - draft: bool | None=None, - published: bool | None=None, - namespace: str | None=None, - types: list[str] | None=None, - draft_title: str | None=None, - published_title: str | None=None, + draft: bool | None = None, + published: bool | None = None, + namespace: str | None = None, + types: list[str] | None = None, + draft_title: str | None = None, + published_title: str | None = None, ) -> QuerySet: """ Fetch a QuerySet of Components for a LearningPackage using various filters. @@ -254,9 +254,9 @@ def get_components( .order_by('pk') if draft is not None: - qset = qset.filter(publishable_entity__draft__version__isnull = not draft) + qset = qset.filter(publishable_entity__draft__version__isnull=not draft) if published is not None: - qset = qset.filter(publishable_entity__published__version__isnull = not published) + 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: @@ -299,8 +299,7 @@ def get_component_version_content( "component_version", "component_version__component", "component_version__component__learning_package", - ) \ - .get(queries) + ).get(queries) def add_content_to_component_version( diff --git a/openedx_learning/core/components/models.py b/openedx_learning/core/components/models.py index 343f0c959..4cb99ce81 100644 --- a/openedx_learning/core/components/models.py +++ b/openedx_learning/core/components/models.py @@ -20,11 +20,7 @@ from django.db import models -from openedx_learning.lib.fields import ( - case_sensitive_char_field, - immutable_uuid_field, - key_field, -) +from openedx_learning.lib.fields import case_sensitive_char_field, immutable_uuid_field, key_field from openedx_learning.lib.managers import WithRelationsManager from ..contents.models import RawContent diff --git a/openedx_learning/core/publishing/api.py b/openedx_learning/core/publishing/api.py index a1f7851c6..029b8eff0 100644 --- a/openedx_learning/core/publishing/api.py +++ b/openedx_learning/core/publishing/api.py @@ -12,11 +12,7 @@ from django.db.models import F, QuerySet from django.db.transaction import atomic -from .model_mixins import ( - PublishableContentModelRegistry, - PublishableEntityMixin, - PublishableEntityVersionMixin, -) +from .model_mixins import PublishableContentModelRegistry, PublishableEntityMixin, PublishableEntityVersionMixin from .models import ( Draft, LearningPackage, @@ -45,7 +41,7 @@ def get_learning_package_by_key(key: str) -> LearningPackage: def create_learning_package( - key: str, title: str, description: str="", created: datetime | None=None + key: str, title: str, description: str = "", created: datetime | None = None ) -> LearningPackage: """ Create a new LearningPackage. @@ -74,10 +70,10 @@ def create_learning_package( def update_learning_package( learning_package_id: int, - key: str | None=None, - title: str | None=None, - description: str | None=None, - updated: datetime | None=None, + key: str | None = None, + title: str | None = None, + description: str | None = None, + updated: datetime | None = None, ) -> LearningPackage: """ Make an update to LearningPackage metadata. @@ -205,15 +201,14 @@ def get_entities_with_unpublished_deletes(learning_package_id: int) -> QuerySet[ .filter( learning_package_id=learning_package_id, draft__version__isnull=True, - ) \ - .exclude(published__version__isnull=True) + ).exclude(published__version__isnull=True) def publish_all_drafts( learning_package_id: int, message="", - published_at: datetime | None=None, - published_by: int | None=None + published_at: datetime | None = None, + published_by: int | None = None ) -> PublishLog: """ Publish everything that is a Draft and is not already published. @@ -231,8 +226,8 @@ def publish_from_drafts( learning_package_id: int, # LearningPackage.id draft_qset: QuerySet, message: str = "", - published_at: datetime | None=None, - published_by: int | None=None, # User.id + published_at: datetime | None = None, + published_by: int | None = None, # User.id ) -> PublishLog: """ Publish the rows in the ``draft_model_qsets`` args passed in. diff --git a/openedx_learning/lib/managers.py b/openedx_learning/lib/managers.py index 68c850f8e..ed700072e 100644 --- a/openedx_learning/lib/managers.py +++ b/openedx_learning/lib/managers.py @@ -1,8 +1,9 @@ """ Custom Django ORM Managers. """ -from django.db.models.query import QuerySet from django.db import models +from django.db.models.query import QuerySet + class WithRelationsManager(models.Manager): """ diff --git a/tests/openedx_learning/core/components/test_api.py b/tests/openedx_learning/core/components/test_api.py index 2d26e2251..7522b57be 100644 --- a/tests/openedx_learning/core/components/test_api.py +++ b/tests/openedx_learning/core/components/test_api.py @@ -5,12 +5,11 @@ from django.core.exceptions import ObjectDoesNotExist -from openedx_learning.core.publishing.models import LearningPackage -from openedx_learning.core.components.models import Component - from openedx_learning.core.components import api as components_api +from openedx_learning.core.components.models import Component from openedx_learning.core.contents import api as contents_api from openedx_learning.core.publishing import api as publishing_api +from openedx_learning.core.publishing.models import LearningPackage from openedx_learning.lib.test_utils import TestCase From c39b3ab09776b3ecde400d95bb54592417f6d8ac Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 30 Jan 2024 11:43:20 -0500 Subject: [PATCH 19/23] chore: fix import order for migrations --- .../core/components/migrations/0001_initial.py | 6 ++++-- openedx_learning/core/contents/migrations/0001_initial.py | 3 ++- .../core/publishing/migrations/0001_initial.py | 8 +++++--- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/openedx_learning/core/components/migrations/0001_initial.py b/openedx_learning/core/components/migrations/0001_initial.py index 68a324f36..fdfb5e40b 100644 --- a/openedx_learning/core/components/migrations/0001_initial.py +++ b/openedx_learning/core/components/migrations/0001_initial.py @@ -1,9 +1,11 @@ # Generated by Django 3.2.23 on 2024-01-22 00:38 -from django.db import migrations, models +import uuid + import django.db.models.deletion +from django.db import migrations, models + import openedx_learning.lib.fields -import uuid class Migration(migrations.Migration): diff --git a/openedx_learning/core/contents/migrations/0001_initial.py b/openedx_learning/core/contents/migrations/0001_initial.py index b7ea865a5..d1099466c 100644 --- a/openedx_learning/core/contents/migrations/0001_initial.py +++ b/openedx_learning/core/contents/migrations/0001_initial.py @@ -1,8 +1,9 @@ # Generated by Django 3.2.23 on 2024-01-22 00:38 import django.core.validators -from django.db import migrations, models import django.db.models.deletion +from django.db import migrations, models + import openedx_learning.lib.fields import openedx_learning.lib.validators diff --git a/openedx_learning/core/publishing/migrations/0001_initial.py b/openedx_learning/core/publishing/migrations/0001_initial.py index dcfcf5fd8..0a4808f46 100644 --- a/openedx_learning/core/publishing/migrations/0001_initial.py +++ b/openedx_learning/core/publishing/migrations/0001_initial.py @@ -1,12 +1,14 @@ # Generated by Django 3.2.23 on 2024-01-22 00:37 -from django.conf import settings +import uuid + import django.core.validators -from django.db import migrations, models import django.db.models.deletion +from django.conf import settings +from django.db import migrations, models + import openedx_learning.lib.fields import openedx_learning.lib.validators -import uuid class Migration(migrations.Migration): From cc2df9975301c348026cd9f46ee67c70dbd98367 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 30 Jan 2024 11:50:26 -0500 Subject: [PATCH 20/23] test: add annotation for what kind of queryset --- openedx_learning/core/components/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx_learning/core/components/api.py b/openedx_learning/core/components/api.py index f96af9782..91f159c77 100644 --- a/openedx_learning/core/components/api.py +++ b/openedx_learning/core/components/api.py @@ -241,7 +241,7 @@ def get_components( types: list[str] | None = None, draft_title: str | None = None, published_title: str | None = None, -) -> QuerySet: +) -> QuerySet[Component]: """ Fetch a QuerySet of Components for a LearningPackage using various filters. From d36ec08e967400955a14d122db827bed94aa3e46 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 30 Jan 2024 17:18:26 -0500 Subject: [PATCH 21/23] refactor: punt the id vs. pk question by making it positional-only --- .../management/commands/load_components.py | 14 ++++---- openedx_learning/core/components/api.py | 14 ++++++-- openedx_learning/core/components/models.py | 4 +-- openedx_learning/core/contents/api.py | 3 ++ openedx_learning/core/publishing/api.py | 33 ++++++++++++------- .../core/components/test_api.py | 24 +++++++------- .../core/components/test_models.py | 2 +- 7 files changed, 57 insertions(+), 37 deletions(-) diff --git a/olx_importer/management/commands/load_components.py b/olx_importer/management/commands/load_components.py index 666eeeedc..6b8d2e423 100644 --- a/olx_importer/management/commands/load_components.py +++ b/olx_importer/management/commands/load_components.py @@ -13,7 +13,7 @@ has really happened between what's currently stored/published for a particular item and the new value we want to set? For RawContent that's easy, because we have actual hashes of the data. But it's not clear how that would work for -something like an ComponentVersion. We'd have to have some kind of mechanism where every +something like an ComponentVersion. We'd have to have some kind of mechanism where every pp that wants to attach data gets to answer the question of "has anything changed?" in order to decide if we really make a new ComponentVersion or not. """ @@ -35,7 +35,7 @@ SUPPORTED_TYPES = ["problem", "video", "html"] logger = logging.getLogger(__name__) - + class Command(BaseCommand): help = "Load sample Component data from course export" @@ -95,7 +95,7 @@ def load_course_data(self, learning_package_key): self.import_block_type(block_type, now) #, publish_log_entry) publishing_api.publish_all_drafts( - self.learning_package.id, + self.learning_package.id, message="Initial Import from load_components script" ) @@ -117,7 +117,7 @@ def create_content(self, static_local_path, now, component_version): return # Might as well bail if we can't find the file. raw_content, _created = contents_api.get_or_create_raw_content( - learning_package_id=self.learning_package.id, + self.learning_package.id, data_bytes=data_bytes, mime_type=mime_type, created=now, @@ -153,10 +153,10 @@ def import_block_type(self, block_type, now): # , publish_log_entry): logger.error(f"Parse error for {xml_file_path}: {err}") components_skipped += 1 continue - + display_name = block_root.attrib.get("display_name", "") _component, component_version = components_api.create_component_and_version( - learning_package_id=self.learning_package.id, + self.learning_package.id, namespace=namespace, type=block_type, local_key=local_key, @@ -168,7 +168,7 @@ def import_block_type(self, block_type, now): # , publish_log_entry): # Create the RawContent entry for the raw data... data_bytes = xml_file_path.read_bytes() text_content, _created = contents_api.get_or_create_text_content_from_bytes( - learning_package_id=self.learning_package.id, + self.learning_package.id, data_bytes=data_bytes, mime_type=f"application/vnd.openedx.xblock.v1.{block_type}+xml", created=now, diff --git a/openedx_learning/core/components/api.py b/openedx_learning/core/components/api.py index 91f159c77..787cb7e60 100644 --- a/openedx_learning/core/components/api.py +++ b/openedx_learning/core/components/api.py @@ -24,6 +24,7 @@ def create_component( learning_package_id: int, + /, namespace: str, type: str, # pylint: disable=redefined-builtin local_key: str, @@ -50,6 +51,7 @@ def create_component( def create_component_version( component_pk: int, + /, version_num: int, title: str, created: datetime, @@ -60,7 +62,7 @@ def create_component_version( """ with atomic(): publishable_entity_version = publishing_api.create_publishable_entity_version( - entity_id=component_pk, + component_pk, version_num=version_num, title=title, created=created, @@ -75,6 +77,7 @@ def create_component_version( def create_next_version( component_pk: int, + /, title: str, content_to_replace: dict[str, int | None], created: datetime, @@ -118,7 +121,7 @@ def create_next_version( with atomic(): publishable_entity_version = publishing_api.create_publishable_entity_version( - entity_id=component_pk, + component_pk, version_num=next_version_num, title=title, created=created, @@ -158,6 +161,7 @@ def create_next_version( def create_component_and_version( learning_package_id: int, + /, namespace: str, type: str, # pylint: disable=redefined-builtin local_key: str, @@ -182,7 +186,7 @@ def create_component_and_version( return (component, component_version) -def get_component(component_pk: int) -> Component: +def get_component(component_pk: int, /) -> Component: """ Get Component by its primary key. @@ -193,6 +197,7 @@ def get_component(component_pk: int) -> Component: def get_component_by_key( learning_package_id: int, + /, namespace: str, type: str, # pylint: disable=redefined-builtin local_key: str, @@ -211,6 +216,7 @@ def get_component_by_key( def component_exists_by_key( learning_package_id: int, + /, namespace: str, type: str, # pylint: disable=redefined-builtin local_key: str @@ -235,6 +241,7 @@ def component_exists_by_key( def get_components( learning_package_id: int, + /, draft: bool | None = None, published: bool | None = None, namespace: str | None = None, @@ -304,6 +311,7 @@ def get_component_version_content( def add_content_to_component_version( component_version_id: int, + /, raw_content_id: int, key: str, learner_downloadable=False, diff --git a/openedx_learning/core/components/models.py b/openedx_learning/core/components/models.py index 4cb99ce81..b7eba42d6 100644 --- a/openedx_learning/core/components/models.py +++ b/openedx_learning/core/components/models.py @@ -86,8 +86,8 @@ class Component(PublishableEntityMixin): # type: ignore[django-manager-missing] ) # 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 + # a single LearningPackage through our publishable_entity relation. However, + # having this foreign key directly allows us to make indexes that efficiently # query by other Component fields within a given LearningPackage, which is # going to be a common use case (and we can't make a compound index using # columns from different tables). diff --git a/openedx_learning/core/contents/api.py b/openedx_learning/core/contents/api.py index c29255b27..923dc1192 100644 --- a/openedx_learning/core/contents/api.py +++ b/openedx_learning/core/contents/api.py @@ -20,6 +20,7 @@ def create_raw_content( learning_package_id: int, + /, data_bytes: bytes, mime_type: str, created: datetime, @@ -91,6 +92,7 @@ def get_media_type_id(mime_type: str) -> int: def get_or_create_raw_content( learning_package_id: int, + /, data_bytes: bytes, mime_type: str, created: datetime, @@ -117,6 +119,7 @@ def get_or_create_raw_content( def get_or_create_text_content_from_bytes( learning_package_id: int, + /, data_bytes: bytes, mime_type: str, created: datetime, diff --git a/openedx_learning/core/publishing/api.py b/openedx_learning/core/publishing/api.py index 029b8eff0..595377573 100644 --- a/openedx_learning/core/publishing/api.py +++ b/openedx_learning/core/publishing/api.py @@ -24,7 +24,7 @@ ) -def get_learning_package(learning_package_id: int) -> LearningPackage: +def get_learning_package(learning_package_id: int, /) -> LearningPackage: """ Get LearningPackage by ID. """ @@ -70,6 +70,7 @@ def create_learning_package( def update_learning_package( learning_package_id: int, + /, key: str | None = None, title: str | None = None, description: str | None = None, @@ -106,6 +107,7 @@ def update_learning_package( def create_publishable_entity( learning_package_id: int, + /, key: str, created: datetime, # User ID who created this @@ -127,6 +129,7 @@ def create_publishable_entity( def create_publishable_entity_version( entity_id: int, + /, version_num: int, title: str, created: datetime, @@ -153,11 +156,11 @@ def create_publishable_entity_version( return version -def get_publishable_entity(publishable_entity_id: int) -> PublishableEntity: +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_key(learning_package_id, /, key) -> PublishableEntity: return PublishableEntity.objects.get( learning_package_id=learning_package_id, key=key, @@ -171,27 +174,27 @@ def learning_package_exists(key: str) -> bool: return LearningPackage.objects.filter(key=key).exists() -def get_last_publish(learning_package_id: int) -> PublishLog | None: +def get_last_publish(learning_package_id: int, /) -> PublishLog | None: return PublishLog.objects \ .filter(learning_package_id=learning_package_id) \ .order_by('-id') \ .first() -def get_all_drafts(learning_package_id: int) -> QuerySet[Draft]: +def get_all_drafts(learning_package_id: int, /) -> QuerySet[Draft]: return Draft.objects.filter( entity__learning_package_id=learning_package_id, version__isnull=False, ) -def get_entities_with_unpublished_changes(learning_package_id: int) -> QuerySet[PublishableEntity]: +def get_entities_with_unpublished_changes(learning_package_id: int, /) -> QuerySet[PublishableEntity]: return PublishableEntity.objects \ .filter(learning_package_id=learning_package_id) \ .exclude(draft__version=F('published__version')) -def get_entities_with_unpublished_deletes(learning_package_id: int) -> QuerySet[PublishableEntity]: +def get_entities_with_unpublished_deletes(learning_package_id: int, /) -> QuerySet[PublishableEntity]: """ Something will become "deleted" if it has a null Draft version but a not-null Published version. (If both are null, it means it's already been @@ -206,6 +209,7 @@ def get_entities_with_unpublished_deletes(learning_package_id: int) -> QuerySet[ def publish_all_drafts( learning_package_id: int, + /, message="", published_at: datetime | None = None, published_by: int | None = None @@ -224,6 +228,7 @@ def publish_all_drafts( def publish_from_drafts( learning_package_id: int, # LearningPackage.id + /, draft_qset: QuerySet, message: str = "", published_at: datetime | None = None, @@ -276,7 +281,7 @@ def publish_from_drafts( return publish_log -def get_draft_version(publishable_entity_id: int) -> PublishableEntityVersion | None: +def get_draft_version(publishable_entity_id: int, /) -> PublishableEntityVersion | None: """ Return current draft PublishableEntityVersion for this PublishableEntity. @@ -296,7 +301,7 @@ def get_draft_version(publishable_entity_id: int) -> PublishableEntityVersion | return draft.version -def get_published_version(publishable_entity_id: int) -> PublishableEntityVersion | None: +def get_published_version(publishable_entity_id: int, /) -> PublishableEntityVersion | None: """ Return current published PublishableEntityVersion for this PublishableEntity. @@ -314,7 +319,11 @@ def get_published_version(publishable_entity_id: int) -> PublishableEntityVersio return published.version -def set_draft_version(publishable_entity_id: int, publishable_entity_version_pk: int | None) -> None: +def set_draft_version( + publishable_entity_id: int, + publishable_entity_version_pk: int | None, + /, +) -> None: """ Modify the Draft of a PublishableEntity to be a PublishableEntityVersion. @@ -329,7 +338,7 @@ def set_draft_version(publishable_entity_id: int, publishable_entity_version_pk: draft.save() -def soft_delete_draft(publishable_entity_id: int) -> None: +def soft_delete_draft(publishable_entity_id: int, /) -> None: """ Sets the Draft version to None. @@ -342,7 +351,7 @@ def soft_delete_draft(publishable_entity_id: int) -> None: return set_draft_version(publishable_entity_id, None) -def reset_drafts_to_published(learning_package_id: int) -> None: +def reset_drafts_to_published(learning_package_id: int, /) -> None: """ Reset all Drafts to point to the most recently Published versions. diff --git a/tests/openedx_learning/core/components/test_api.py b/tests/openedx_learning/core/components/test_api.py index 7522b57be..69e503643 100644 --- a/tests/openedx_learning/core/components/test_api.py +++ b/tests/openedx_learning/core/components/test_api.py @@ -44,7 +44,7 @@ def test_component_num_queries(self) -> None: Create a basic component and test that we fetch it back in 1 query. """ component, _version = components_api.create_component_and_version( - learning_package_id=self.learning_package.id, + self.learning_package.id, namespace="xblock.v1", type="problem", local_key="Query Counting", @@ -95,7 +95,7 @@ def setUpTestData(cls) -> None: # Components we're publishing... cls.published_problem, _version = components_api.create_component_and_version( - learning_package_id=cls.learning_package.id, + cls.learning_package.id, namespace="xblock.v2", type="problem", local_key="published_problem", @@ -104,7 +104,7 @@ def setUpTestData(cls) -> None: created_by=None, ) cls.published_html, _version = components_api.create_component_and_version( - learning_package_id=cls.learning_package.id, + cls.learning_package.id, namespace="xblock.v1", type="html", local_key="published_html", @@ -119,7 +119,7 @@ def setUpTestData(cls) -> None: # Components that exist only as Drafts cls.unpublished_problem, _version = components_api.create_component_and_version( - learning_package_id=cls.learning_package.id, + cls.learning_package.id, namespace="xblock.v2", type="problem", local_key="unpublished_problem", @@ -128,7 +128,7 @@ def setUpTestData(cls) -> None: created_by=None, ) cls.unpublished_html, _version = components_api.create_component_and_version( - learning_package_id=cls.learning_package.id, + cls.learning_package.id, namespace="xblock.v1", type="html", local_key="unpublished_html", @@ -140,7 +140,7 @@ def setUpTestData(cls) -> None: # Component we're putting here to soft delete (this will remove the # Draft entry) cls.deleted_video, _version = components_api.create_component_and_version( - learning_package_id=cls.learning_package.id, + cls.learning_package.id, namespace="xblock.v1", type="html", local_key="deleted_video", @@ -298,7 +298,7 @@ def setUpTestData(cls) -> None: ) cls.now = datetime(2023, 5, 8, tzinfo=timezone.utc) cls.problem = components_api.create_component( - learning_package_id=cls.learning_package.id, + cls.learning_package.id, namespace='xblock.v1', type='problem', local_key='my_component', @@ -306,7 +306,7 @@ def setUpTestData(cls) -> None: created_by=None, ) cls.html = components_api.create_component( - learning_package_id=cls.learning_package.id, + cls.learning_package.id, namespace='xblock.v1', type='html', local_key='my_component', @@ -365,7 +365,7 @@ def setUpTestData(cls) -> None: ) cls.now = datetime(2023, 5, 8, tzinfo=timezone.utc) cls.problem = components_api.create_component( - learning_package_id=cls.learning_package.id, + cls.learning_package.id, namespace='xblock.v1', type='problem', local_key='my_component', @@ -404,19 +404,19 @@ def test_add(self): def test_multiple_versions(self): hello_content, _created = contents_api.get_or_create_text_content_from_bytes( - learning_package_id=self.learning_package.id, + self.learning_package.id, data_bytes="Hello World!".encode('utf-8'), mime_type="text/plain", created=self.now, ) goodbye_content, _created = contents_api.get_or_create_text_content_from_bytes( - learning_package_id=self.learning_package.id, + self.learning_package.id, data_bytes="Goodbye World!".encode('utf-8'), mime_type="text/plain", created=self.now, ) blank_content, _created = contents_api.get_or_create_text_content_from_bytes( - learning_package_id=self.learning_package.id, + self.learning_package.id, data_bytes="".encode('utf-8'), mime_type="text/plain", created=self.now, diff --git a/tests/openedx_learning/core/components/test_models.py b/tests/openedx_learning/core/components/test_models.py index 2ef5daf64..9342e2328 100644 --- a/tests/openedx_learning/core/components/test_models.py +++ b/tests/openedx_learning/core/components/test_models.py @@ -25,7 +25,7 @@ def setUpTestData(cls) -> None: # Note: we must specify '-> None' to opt in to def test_latest_version(self) -> None: component, component_version = create_component_and_version( - learning_package_id=self.learning_package.id, + self.learning_package.id, namespace="xblock.v1", type="problem", local_key="monty_hall", From 969537757a04214550102463032a16d0ecaa638c Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 30 Jan 2024 18:30:36 -0500 Subject: [PATCH 22/23] docs: better comments --- openedx_learning/core/components/api.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openedx_learning/core/components/api.py b/openedx_learning/core/components/api.py index 787cb7e60..7891ed372 100644 --- a/openedx_learning/core/components/api.py +++ b/openedx_learning/core/components/api.py @@ -143,8 +143,8 @@ def create_next_version( key=key, learner_downloadable=False, ) - # Now copy any old associations that existed, as long as they don't - # conflict with the new stuff. + # Now copy any old associations that existed, as long as they aren't + # in conflict with the new stuff or marked for deletion. last_version_content_mapping = ComponentVersionRawContent.objects \ .filter(component_version=last_version) for cvrc in last_version_content_mapping: From 07049133d484ce1ffd92d011d320a2c83bdd59ee Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 31 Jan 2024 00:36:55 -0500 Subject: [PATCH 23/23] refactor: normalize component types with ComponentType MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prior to this commit, Components encoded their namespace + type info as two columns on the Component model itself. This wasted a lot of space on a very common index lookup–many rows of ('xblock.v1', 'video'), ('xblock.v1', 'problem'), and ('xblock.v1', 'text'). But worse, the lack of a separate ComponentType model meant that there was no first class entity to store per-type policy data against. For example, we might want to say "the following types are supported by libraries, while these other types are experimental", or "these types are enabled for these orgs", etc. Components are required to have a ComponentType. We're rewriting the first migration for the components app here, since this app hasn't been added to edx-platform yet. --- .../management/commands/load_components.py | 2 +- openedx_learning/core/components/admin.py | 7 +- openedx_learning/core/components/api.py | 50 ++++++---- .../components/migrations/0001_initial.py | 29 ++++-- openedx_learning/core/components/models.py | 92 +++++++++++++------ openedx_learning/core/contents/api.py | 9 +- .../core/components/test_api.py | 28 +++--- .../core/components/test_models.py | 2 +- .../core/contents/test_media_types.py | 10 +- 9 files changed, 144 insertions(+), 85 deletions(-) diff --git a/olx_importer/management/commands/load_components.py b/olx_importer/management/commands/load_components.py index 6b8d2e423..575f01edd 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 c5fb9b16f..1e84391bc 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 7891ed372..c08acde19 100644 --- a/openedx_learning/core/components/api.py +++ b/openedx_learning/core/components/api.py @@ -18,15 +18,30 @@ 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. + + An example of + """ + 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 +49,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 +57,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 +177,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 +188,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 +213,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 +222,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 +232,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 +242,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 +259,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 +279,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 fdfb5e40b..b72d1aee8 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 b7eba42d6..920b3efb9 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 923dc1192..0b27690a1 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 69e503643..b1d46929c 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 9342e2328..d4bb20e3d 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 c880a008b..96390cb45 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