From 2ec291845887b3db6bb56913ce1b12654d7e2574 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 8 Jan 2024 17:34:05 -0500 Subject: [PATCH] feat: add features to support new content libraries This commit adds a number of features needed to accomodate content libraries. These are all collected together because this work was done while iteratively making more and more of libraries functional, and it was difficult to properly separate out later. This bumps the version to 0.5.0. New publishing API functions: * Learning Packages * get_learning_package * get_learning_package_by_key, * update_learning_package * Publishable Entities * get_publishable_entity and * get_publishable_entity_by_key * Draft/Publishing * get_last_publish * get_all_drafts * get_entities_with_unpublished_changes * get_entities_with_unpublished_deletes * get_published_version * set_draft_version * soft_delete_draft * reset_drafts_to_published New Components API functions: * create_next_version * get_component_by_key * component_exists_by_key * get_components PublishableEntityMixin improvements: * New property methods: latest, has_unpublished_changes * Existing methods were improved to use model-cached values. Encoding fixes: * collations for multiple fields were changed to be case insensitive in order to better support simple text searches. Model optimizations: * created the WithRelationsManager to more easily define querysets that involve adding select_related fields. This was used to elininate a number of n+1 queries that came up during libraries development. * PublishableEntityVersion.version_num was reduced to a 4-byte PositiveIntegerField instead of using the 8-byte default for openedx_learning apps (it's assumed we will never make more than 2 billion versions of a particular PublishableEntity). This was done to help reduce the size of indexes. Typing: * type annotations were added to many functions and test functions. --- .gitignore | 3 + .../management/commands/load_components.py | 14 +- openedx_learning/__init__.py | 2 +- openedx_learning/core/components/api.py | 219 +++++++- .../components/migrations/0001_initial.py | 4 +- openedx_learning/core/components/models.py | 19 +- openedx_learning/core/contents/api.py | 3 + .../core/contents/migrations/0001_initial.py | 6 +- openedx_learning/core/contents/models.py | 10 +- openedx_learning/core/publishing/admin.py | 4 +- openedx_learning/core/publishing/api.py | 208 +++++++- .../publishing/migrations/0001_initial.py | 13 +- .../migrations/0002_alter_fk_on_delete.py | 30 -- .../core/publishing/model_mixins.py | 142 +++-- openedx_learning/core/publishing/models.py | 29 +- openedx_learning/lib/managers.py | 38 ++ test_settings.py | 2 + .../core/components/test_api.py | 504 ++++++++++++++++++ .../core/components/test_models.py | 11 +- .../core/publishing/test_api.py | 210 +++++++- 20 files changed, 1315 insertions(+), 156 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/.gitignore b/.gitignore index 6b5c4fed..0f071cb7 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/olx_importer/management/commands/load_components.py b/olx_importer/management/commands/load_components.py index 666eeeed..6b8d2e42 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/__init__.py b/openedx_learning/__init__.py index 13243d24..0dc1d505 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 4275c5d1..7891ed37 100644 --- a/openedx_learning/core/components/api.py +++ b/openedx_learning/core/components/api.py @@ -15,27 +15,28 @@ 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 def create_component( learning_package_id: int, + /, namespace: str, type: str, # pylint: disable=redefined-builtin local_key: str, created: datetime, created_by: int | None, -): +) -> Component: """ Create a new Component (an entity like a Problem or Video) """ 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( @@ -50,6 +51,7 @@ def create_component( def create_component_version( component_pk: int, + /, version_num: int, title: str, created: datetime, @@ -59,8 +61,8 @@ def create_component_version( Create a new ComponentVersion """ with atomic(): - publishable_entity_version = create_publishable_entity_version( - entity_id=component_pk, + publishable_entity_version = publishing_api.create_publishable_entity_version( + component_pk, version_num=version_num, title=title, created=created, @@ -73,15 +75,100 @@ def create_component_version( return component_version +def create_next_version( + component_pk: int, + /, + title: str, + content_to_replace: dict[str, int | None], + created: datetime, + created_by: int | None = None, +) -> ComponentVersion: + """ + 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. + + 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. + """ + # 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( + 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(): + # 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 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: + if cvrc.key not in content_to_replace: + ComponentVersionRawContent.objects.create( + raw_content_id=cvrc.raw_content_id, + component_version=component_version, + key=cvrc.key, + learner_downloadable=cvrc.learner_downloadable, + ) + + return component_version + + def create_component_and_version( learning_package_id: int, + /, namespace: str, type: str, # pylint: disable=redefined-builtin local_key: str, title: str, created: datetime, - created_by: int | None, -): + created_by: int | None = None, +) -> tuple[Component, ComponentVersion]: """ Create a Component and associated ComponentVersion atomically """ @@ -99,8 +186,98 @@ def create_component_and_version( return (component, component_version) -def get_component_by_pk(component_pk: int) -> Component: - return Component.objects.get(pk=component_pk) +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, +) -> Component: + """ + 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, + ) + + +def component_exists_by_key( + learning_package_id: int, + /, + namespace: str, + type: str, # pylint: disable=redefined-builtin + local_key: str +) -> bool: + """ + 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, + draft_title: str | None = None, + published_title: str | None = None, +) -> QuerySet[Component]: + """ + 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 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 def get_component_version_content( @@ -115,22 +292,26 @@ 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", - "component_version", - "component_version__component", - "component_version__component__learning_package", - ).get( + 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( - component_version: ComponentVersion, + component_version_id: int, + /, raw_content_id: int, key: str, learner_downloadable=False, @@ -139,7 +320,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/openedx_learning/core/components/migrations/0001_initial.py b/openedx_learning/core/components/migrations/0001_initial.py index bbe0e15d..fdfb5e40 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 2023-12-04 00:41 +# Generated by Django 3.2.23 on 2024-01-22 00:38 import uuid @@ -13,8 +13,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 2a24beb6..b7eba42d 100644 --- a/openedx_learning/core/components/models.py +++ b/openedx_learning/core/components/models.py @@ -21,6 +21,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.managers import WithRelationsManager from ..contents.models import RawContent from ..publishing.model_mixins import PublishableEntityMixin, PublishableEntityVersionMixin @@ -44,6 +45,12 @@ class Component(PublishableEntityMixin): # type: ignore[django-manager-missing] A Component belongs to exactly one LearningPackage. + 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 ----------- @@ -70,9 +77,17 @@ 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 + # 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 c29255b2..923dc119 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/contents/migrations/0001_initial.py b/openedx_learning/core/contents/migrations/0001_initial.py index 7dac9a3f..d1099466 100644 --- a/openedx_learning/core/contents/migrations/0001_initial.py +++ b/openedx_learning/core/contents/migrations/0001_initial.py @@ -1,4 +1,4 @@ -# 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 @@ -13,7 +13,7 @@ class Migration(migrations.Migration): initial = True dependencies = [ - ('oel_publishing', '0002_alter_fk_on_delete'), + ('oel_publishing', '0001_initial'), ] operations = [ @@ -46,7 +46,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 e7de7375..d90c2abd 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 90f72bcc..a2b5dbde 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 ab4099ad..59537757 100644 --- a/openedx_learning/core/publishing/api.py +++ b/openedx_learning/core/publishing/api.py @@ -24,8 +24,24 @@ ) +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 +58,7 @@ def create_learning_package( package = LearningPackage( key=key, title=title, + description=description, created=created, updated=created, ) @@ -51,8 +68,46 @@ 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, + /, key: str, created: datetime, # User ID who created this @@ -74,6 +129,7 @@ def create_publishable_entity( def create_publishable_entity_version( entity_id: int, + /, version_num: int, title: str, created: datetime, @@ -100,6 +156,17 @@ 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. @@ -107,20 +174,53 @@ 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, /) -> 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')) + + +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 + 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="", published_at: datetime | None = None, published_by: int | None = None -): +) -> PublishLog: """ 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 ) @@ -128,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, @@ -180,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. @@ -200,26 +301,107 @@ 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 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 + ) + 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. 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 draft.save() +def soft_delete_draft(publishable_entity_id: int, /) -> None: + """ + 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: + """ + 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. 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. + + 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 = 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: + 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], -): +) -> PublishableContentModelRegistry: """ Register what content model maps to what content version model. diff --git a/openedx_learning/core/publishing/migrations/0001_initial.py b/openedx_learning/core/publishing/migrations/0001_initial.py index d7663748..0a4808f4 100644 --- a/openedx_learning/core/publishing/migrations/0001_initial.py +++ b/openedx_learning/core/publishing/migrations/0001_initial.py @@ -1,4 +1,4 @@ -# 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 @@ -27,6 +27,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 +44,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 +57,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 +85,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 +105,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 c77a2040..00000000 --- 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 02cc7f20..ce32cd45 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 @@ -114,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. @@ -129,59 +132,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 1eb6803c..9082e8f2 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 00000000..ed700072 --- /dev/null +++ b/openedx_learning/lib/managers.py @@ -0,0 +1,38 @@ +""" +Custom Django ORM Managers. +""" +from django.db import models +from django.db.models.query import QuerySet + + +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/test_settings.py b/test_settings.py index 28676e13..53548fb4 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 new file mode 100644 index 00000000..69e50364 --- /dev/null +++ b/tests/openedx_learning/core/components/test_api.py @@ -0,0 +1,504 @@ +""" +Basic tests of the Components API. +""" +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.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 + + +class PerformanceTestCase(TestCase): + """ + 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. + """ + learning_package: LearningPackage + now: datetime + + @classmethod + def setUpTestData(cls) -> None: + """ + 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, + 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( + 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 GetComponentsTestCase(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: + """ + 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( + 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( + 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( + 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( + 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( + 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) + 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, + ) + 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, + ) + 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, + ) + 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, + ) + 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', + ) + 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'] + ) + assert list(html_and_video_components) == [ + self.published_html, + self.unpublished_html, + self.deleted_video, + ] + + def test_draft_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, + draft_title="PUBLISHED" + ) + # These all have a draft title with "published" in it somewhere. + assert list(components) == [ + self.published_problem, + self.published_html, + self.unpublished_problem, + 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 ComponentGetAndExistsTestCase(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: + """ + 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( + 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( + 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 CreateNewVersionsTestCase(TestCase): + """ + Create new ComponentVersions in various ways. + """ + learning_package: LearningPackage + now: datetime + problem: Component + + @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( + cls.learning_package.id, + namespace='xblock.v1', + type='problem', + local_key='my_component', + created=cls.now, + 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( + 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( + 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( + self.learning_package.id, + data_bytes="".encode('utf-8'), + 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) == 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, + "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 + ) + + # 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 + ) diff --git a/tests/openedx_learning/core/components/test_models.py b/tests/openedx_learning/core/components/test_models.py index 7675f705..9342e232 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 @@ -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", @@ -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 a258f5b0..56c91466 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,10 +10,11 @@ from django.core.exceptions import ValidationError from openedx_learning.core.publishing import api as publishing_api +from openedx_learning.core.publishing.models import LearningPackage, PublishableEntity from openedx_learning.lib.test_utils import TestCase -class CreateLearningPackageTestCase(TestCase): +class LearningPackageTestCase(TestCase): """ Test creating a LearningPackage """ @@ -22,10 +25,17 @@ 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 🔥" + assert package.description == "A fun Description!" assert package.created == created assert package.updated == created @@ -35,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. @@ -62,7 +85,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 @@ -84,21 +111,26 @@ class DraftTestCase(TestCase): """ Test basic operations with Drafts. """ + now: datetime + learning_package: LearningPackage - 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( + @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=created, + created=cls.now, ) + + def test_draft_lifecycle(self) -> None: + """ + Test basic lifecycle of a Draft. + """ 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 @@ -106,16 +138,160 @@ 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=created, + created=self.now, created_by=None, ) assert entity_version == publishing_api.get_draft_version(entity.id) # 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: + """ + 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.id) + + # 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: + 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) + + # 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: + assert ( + 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: + 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: 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