diff --git a/cms/djangoapps/contentstore/admin.py b/cms/djangoapps/contentstore/admin.py index 0b01abe05073..67bb39b7a32a 100644 --- a/cms/djangoapps/contentstore/admin.py +++ b/cms/djangoapps/contentstore/admin.py @@ -13,15 +13,15 @@ from cms.djangoapps.contentstore.models import ( BackfillCourseTabsConfig, CleanStaleCertificateAvailabilityDatesConfig, + ComponentLink, + ContainerLink, LearningContextLinksStatus, - PublishableEntityLink, - VideoUploadConfig + VideoUploadConfig, ) from cms.djangoapps.contentstore.outlines_regenerate import CourseOutlineRegenerate from openedx.core.djangoapps.content.learning_sequences.api import key_supports_outlines -from .tasks import update_outline_from_modulestore_task, update_all_outlines_from_modulestore_task - +from .tasks import update_all_outlines_from_modulestore_task, update_outline_from_modulestore_task log = logging.getLogger(__name__) @@ -88,10 +88,10 @@ class CleanStaleCertificateAvailabilityDatesConfigAdmin(ConfigurationModelAdmin) pass -@admin.register(PublishableEntityLink) -class PublishableEntityLinkAdmin(admin.ModelAdmin): +@admin.register(ComponentLink) +class ComponentLinkAdmin(admin.ModelAdmin): """ - PublishableEntityLink admin. + ComponentLink admin. """ fields = ( "uuid", @@ -127,6 +127,45 @@ def has_change_permission(self, request, obj=None): return False +@admin.register(ContainerLink) +class ContainerLinkAdmin(admin.ModelAdmin): + """ + ContainerLink admin. + """ + fields = ( + "uuid", + "upstream_container", + "upstream_container_key", + "upstream_context_key", + "downstream_usage_key", + "downstream_context_key", + "version_synced", + "version_declined", + "created", + "updated", + ) + readonly_fields = fields + list_display = [ + "upstream_container", + "upstream_container_key", + "downstream_usage_key", + "version_synced", + "updated", + ] + search_fields = [ + "upstream_container_key", + "upstream_context_key", + "downstream_usage_key", + "downstream_context_key", + ] + + def has_add_permission(self, request): + return False + + def has_change_permission(self, request, obj=None): + return False + + @admin.register(LearningContextLinksStatus) class LearningContextLinksStatusAdmin(admin.ModelAdmin): """ diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index 506ce766ff23..f80e304fb791 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -31,7 +31,8 @@ ) from cms.djangoapps.models.settings.course_grading import CourseGradingModel -from cms.lib.xblock.upstream_sync import UpstreamLink, UpstreamLinkException, fetch_customizable_fields +from cms.lib.xblock.upstream_sync import UpstreamLink, UpstreamLinkException +from cms.lib.xblock.upstream_sync_block import fetch_customizable_fields_from_block from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers import openedx.core.djangoapps.content_staging.api as content_staging_api import openedx.core.djangoapps.content_tagging.api as content_tagging_api @@ -416,7 +417,7 @@ def _fetch_and_set_upstream_link( user: User ): """ - Fetch and set upstream link for the given xblock. This function handles following cases: + Fetch and set upstream link for the given xblock which is being pasted. This function handles following cases: * the xblock is copied from a v2 library; the library block is set as upstream. * the xblock is copied from a course; no upstream is set, only copied_from_block is set. * the xblock is copied from a course where the source block was imported from a library; the original libary block @@ -425,7 +426,7 @@ def _fetch_and_set_upstream_link( # Try to link the pasted block (downstream) to the copied block (upstream). temp_xblock.upstream = copied_from_block try: - UpstreamLink.get_for_block(temp_xblock) + upstream_link = UpstreamLink.get_for_block(temp_xblock) except UpstreamLinkException: # Usually this will fail. For example, if the copied block is a modulestore course block, it can't be an # upstream. That's fine! Instead, we store a reference to where this block was copied from, in the @@ -456,7 +457,8 @@ def _fetch_and_set_upstream_link( # later wants to restore it, it will restore to the value that the field had when the block was pasted. Of # course, if the author later syncs updates from a *future* published upstream version, then that will fetch # new values from the published upstream content. - fetch_customizable_fields(upstream=temp_xblock, downstream=temp_xblock, user=user) + if isinstance(upstream_link.upstream_key, UsageKey): # only if upstream is a block, not a container + fetch_customizable_fields_from_block(downstream=temp_xblock, user=user, upstream=temp_xblock) def _import_xml_node_to_parent( @@ -790,3 +792,26 @@ def _get_usage_key_from_node(node, parent_id: str) -> UsageKey | None: ) return usage_key + + +def concat_static_file_notices(notices: list[StaticFileNotices]) -> StaticFileNotices: + """Combines multiple static file notices into a single object + + Args: + notices: list of StaticFileNotices + + Returns: + Single StaticFileNotices + """ + new_files = [] + conflicting_files = [] + error_files = [] + for notice in notices: + new_files.extend(notice.new_files) + conflicting_files.extend(notice.conflicting_files) + error_files.extend(notice.error_files) + return StaticFileNotices( + new_files=list(set(new_files)), + conflicting_files=list(set(conflicting_files)), + error_files=list(set(error_files)), + ) diff --git a/cms/djangoapps/contentstore/management/commands/recreate_upstream_links.py b/cms/djangoapps/contentstore/management/commands/recreate_upstream_links.py index c1a8454cd56c..a0f04bb279e9 100644 --- a/cms/djangoapps/contentstore/management/commands/recreate_upstream_links.py +++ b/cms/djangoapps/contentstore/management/commands/recreate_upstream_links.py @@ -1,5 +1,5 @@ """ -Management command to recreate upstream-dowstream links in PublishableEntityLink for course(s). +Management command to recreate upstream-dowstream links in ComponentLink for course(s). This command can be run for all the courses or for given list of courses. """ @@ -23,7 +23,7 @@ class Command(BaseCommand): """ - Recreate links for course(s) in PublishableEntityLink table. + Recreate upstream links for course(s) in ComponentLink and ContainerLink tables. Examples: # Recreate upstream links for two courses. diff --git a/cms/djangoapps/contentstore/migrations/0010_container_link_models.py b/cms/djangoapps/contentstore/migrations/0010_container_link_models.py new file mode 100644 index 000000000000..7a02a1c74df1 --- /dev/null +++ b/cms/djangoapps/contentstore/migrations/0010_container_link_models.py @@ -0,0 +1,58 @@ +# Generated by Django 4.2.20 on 2025-04-22 15:08 +import uuid + +import django.db.models.deletion +import opaque_keys.edx.django.models +import openedx_learning.lib.fields +import openedx_learning.lib.validators +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ('oel_components', '0003_remove_componentversioncontent_learner_downloadable'), + ('contentstore', '0009_learningcontextlinksstatus_publishableentitylink'), + ] + + operations = [ + migrations.RenameModel( + old_name='PublishableEntityLink', + new_name='ComponentLink', + ), + migrations.AlterModelOptions( + name='componentlink', + options={'verbose_name': 'Component Link', 'verbose_name_plural': 'Component Links'}, + ), + migrations.AlterField( + model_name='componentlink', + name='upstream_block', + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + related_name='links', + to='oel_components.component', + ), + ), + migrations.CreateModel( + name='ContainerLink', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('uuid', models.UUIDField(default=uuid.uuid4, editable=False, unique=True, verbose_name='UUID')), + ('upstream_context_key', openedx_learning.lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, db_index=True, help_text='Upstream context key i.e., learning_package/library key', max_length=500)), + ('downstream_usage_key', opaque_keys.edx.django.models.UsageKeyField(max_length=255, unique=True)), + ('downstream_context_key', opaque_keys.edx.django.models.CourseKeyField(db_index=True, max_length=255)), + ('version_synced', models.IntegerField()), + ('version_declined', models.IntegerField(blank=True, null=True)), + ('created', models.DateTimeField(validators=[openedx_learning.lib.validators.validate_utc_datetime])), + ('updated', models.DateTimeField(validators=[openedx_learning.lib.validators.validate_utc_datetime])), + ('upstream_container_key', opaque_keys.edx.django.models.ContainerKeyField(help_text='Upstream block key (e.g. lct:...), this value cannot be null and is useful to track upstream library blocks that do not exist yet or were deleted.', max_length=255)), + ('upstream_container', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='links', to='oel_publishing.container')), + ], + options={ + 'abstract': False, + 'verbose_name': 'Container Link', + 'verbose_name_plural': 'Container Links', + }, + ), + ] diff --git a/cms/djangoapps/contentstore/models.py b/cms/djangoapps/contentstore/models.py index ca6171118c00..24dc6748d2c7 100644 --- a/cms/djangoapps/contentstore/models.py +++ b/cms/djangoapps/contentstore/models.py @@ -12,9 +12,11 @@ from django.db.models.functions import Coalesce from django.db.models.lookups import GreaterThan from django.utils.translation import gettext_lazy as _ -from opaque_keys.edx.django.models import CourseKeyField, UsageKeyField +from opaque_keys.edx.django.models import CourseKeyField, ContainerKeyField, UsageKeyField from opaque_keys.edx.keys import CourseKey, UsageKey -from openedx_learning.api.authoring_models import Component, PublishableEntity +from opaque_keys.edx.locator import LibraryContainerLocator +from openedx_learning.api.authoring import get_published_version +from openedx_learning.api.authoring_models import Component, Container from openedx_learning.lib.fields import ( immutable_uuid_field, key_field, @@ -80,26 +82,12 @@ class Meta: ) -class PublishableEntityLink(models.Model): +class EntityLinkBase(models.Model): """ - This represents link between any two publishable entities or link between publishable entity and a course - xblock. It helps in tracking relationship between xblocks imported from libraries and used in different courses. + Abstract base class that defines fields and functions for storing link between two publishable entities + or links between publishable entity and a course xblock. """ uuid = immutable_uuid_field() - upstream_block = models.ForeignKey( - PublishableEntity, - on_delete=models.SET_NULL, - related_name="links", - null=True, - blank=True, - ) - upstream_usage_key = UsageKeyField( - max_length=255, - help_text=_( - "Upstream block usage key, this value cannot be null" - " and useful to track upstream library blocks that do not exist yet" - ) - ) # Search by library/upstream context key upstream_context_key = key_field( help_text=_("Upstream context key i.e., learning_package/library key"), @@ -115,31 +103,107 @@ class PublishableEntityLink(models.Model): created = manual_date_time_field() updated = manual_date_time_field() - def __str__(self): - return f"{self.upstream_usage_key}->{self.downstream_usage_key}" + class Meta: + abstract = True @property - def upstream_version(self) -> int | None: + def upstream_version_num(self) -> int | None: """ Returns upstream block version number if available. """ - version_num = None - if hasattr(self.upstream_block, 'published'): - if hasattr(self.upstream_block.published, 'version'): - if hasattr(self.upstream_block.published.version, 'version_num'): - version_num = self.upstream_block.published.version.version_num - return version_num + published_version = get_published_version(self.upstream_block.publishable_entity.id) + return published_version.version_num if published_version else None @property def upstream_context_title(self) -> str: """ Returns upstream context title. """ - return self.upstream_block.learning_package.title + return self.upstream_block.publishable_entity.learning_package.title + + @classmethod + def filter_links( + cls, + **link_filter, + ) -> QuerySet["EntityLinkBase"]: + """ + Get all links along with sync flag, upstream context title and version, with optional filtering. + """ + ready_to_sync = link_filter.pop('ready_to_sync', None) + result = cls.objects.filter(**link_filter).select_related( + "upstream_block__publishable_entity__published__version", + "upstream_block__publishable_entity__learning_package" + ).annotate( + ready_to_sync=( + GreaterThan( + Coalesce("upstream_block__publishable_entity__published__version__version_num", 0), + Coalesce("version_synced", 0) + ) & GreaterThan( + Coalesce("upstream_block__publishable_entity__published__version__version_num", 0), + Coalesce("version_declined", 0) + ) + ) + ) + if ready_to_sync is not None: + result = result.filter(ready_to_sync=ready_to_sync) + return result + + @classmethod + def summarize_by_downstream_context(cls, downstream_context_key: CourseKey) -> QuerySet: + """ + Returns a summary of links by upstream context for given downstream_context_key. + Example: + [ + { + "upstream_context_title": "CS problems 3", + "upstream_context_key": "lib:OpenedX:CSPROB3", + "ready_to_sync_count": 11, + "total_count": 14 + }, + { + "upstream_context_title": "CS problems 2", + "upstream_context_key": "lib:OpenedX:CSPROB2", + "ready_to_sync_count": 15, + "total_count": 24 + }, + ] + """ + result = cls.filter_links(downstream_context_key=downstream_context_key).values( + "upstream_context_key", + upstream_context_title=F("upstream_block__publishable_entity__learning_package__title"), + ).annotate( + ready_to_sync_count=Count("id", Q(ready_to_sync=True)), + total_count=Count('id') + ) + return result + + +class ComponentLink(EntityLinkBase): + """ + This represents link between any two publishable entities or link between publishable entity and a course + XBlock. It helps in tracking relationship between XBlocks imported from libraries and used in different courses. + """ + upstream_block = models.ForeignKey( + Component, + on_delete=models.SET_NULL, + related_name="links", + null=True, + blank=True, + ) + upstream_usage_key = UsageKeyField( + max_length=255, + help_text=_( + "Upstream block usage key, this value cannot be null" + " and useful to track upstream library blocks that do not exist yet" + ) + ) class Meta: - verbose_name = _("Publishable Entity Link") - verbose_name_plural = _("Publishable Entity Links") + verbose_name = _("Component Link") + verbose_name_plural = _("Component Links") + + def __str__(self): + return f"ComponentLink<{self.upstream_usage_key}->{self.downstream_usage_key}>" @classmethod def update_or_create( @@ -153,7 +217,7 @@ def update_or_create( version_synced: int, version_declined: int | None = None, created: datetime | None = None, - ) -> "PublishableEntityLink": + ) -> "ComponentLink": """ Update or create entity link. This will only update `updated` field if something has changed. """ @@ -170,7 +234,7 @@ def update_or_create( if upstream_block: new_values.update( { - 'upstream_block': upstream_block.publishable_entity, + 'upstream_block': upstream_block, } ) try: @@ -197,70 +261,90 @@ def update_or_create( link.save() return link - @classmethod - def filter_links( - cls, - **link_filter, - ) -> QuerySet["PublishableEntityLink"]: - """ - Get all links along with sync flag, upstream context title and version, with optional filtering. - """ - ready_to_sync = link_filter.pop('ready_to_sync', None) - result = cls.objects.filter(**link_filter).select_related( - "upstream_block__published__version", - "upstream_block__learning_package" - ).annotate( - ready_to_sync=( - GreaterThan( - Coalesce("upstream_block__published__version__version_num", 0), - Coalesce("version_synced", 0) - ) & GreaterThan( - Coalesce("upstream_block__published__version__version_num", 0), - Coalesce("version_declined", 0) - ) - ) - ) - if ready_to_sync is not None: - result = result.filter(ready_to_sync=ready_to_sync) - return result - @classmethod - def get_by_upstream_usage_key(cls, upstream_usage_key: UsageKey) -> QuerySet["PublishableEntityLink"]: - """ - Get all downstream context keys for given upstream usage key - """ - return cls.objects.filter( - upstream_usage_key=upstream_usage_key, +class ContainerLink(EntityLinkBase): + """ + This represents link between any two publishable entities or link between publishable entity and a course + xblock. It helps in tracking relationship between xblocks imported from libraries and used in different courses. + """ + upstream_container = models.ForeignKey( + Container, + on_delete=models.SET_NULL, + related_name="links", + null=True, + blank=True, + ) + upstream_container_key = ContainerKeyField( + max_length=255, + help_text=_( + "Upstream block key (e.g. lct:...), this value cannot be null " + "and is useful to track upstream library blocks that do not exist yet " + "or were deleted." ) + ) + + class Meta: + verbose_name = _("Container Link") + verbose_name_plural = _("Container Links") + + def __str__(self): + return f"ContainerLink<{self.upstream_container_key}->{self.downstream_usage_key}>" @classmethod - def summarize_by_downstream_context(cls, downstream_context_key: CourseKey) -> QuerySet: + def update_or_create( + cls, + upstream_container: Container | None, + /, + upstream_container_key: LibraryContainerLocator, + upstream_context_key: str, + downstream_usage_key: UsageKey, + downstream_context_key: CourseKey, + version_synced: int, + version_declined: int | None = None, + created: datetime | None = None, + ) -> "ContainerLink": """ - Returns a summary of links by upstream context for given downstream_context_key. - Example: - [ - { - "upstream_context_title": "CS problems 3", - "upstream_context_key": "lib:OpenedX:CSPROB3", - "ready_to_sync_count": 11, - "total_count": 14 - }, - { - "upstream_context_title": "CS problems 2", - "upstream_context_key": "lib:OpenedX:CSPROB2", - "ready_to_sync_count": 15, - "total_count": 24 - }, - ] + Update or create entity link. This will only update `updated` field if something has changed. """ - result = cls.filter_links(downstream_context_key=downstream_context_key).values( - "upstream_context_key", - upstream_context_title=F("upstream_block__learning_package__title"), - ).annotate( - ready_to_sync_count=Count("id", Q(ready_to_sync=True)), - total_count=Count('id') - ) - return result + if not created: + created = datetime.now(tz=timezone.utc) + new_values = { + 'upstream_container_key': upstream_container_key, + 'upstream_context_key': upstream_context_key, + 'downstream_usage_key': downstream_usage_key, + 'downstream_context_key': downstream_context_key, + 'version_synced': version_synced, + 'version_declined': version_declined, + } + if upstream_container: + new_values.update( + { + 'upstream_container': upstream_container, + } + ) + try: + link = cls.objects.get(downstream_usage_key=downstream_usage_key) + # TODO: until we save modified datetime for course xblocks in index, the modified time for links are updated + # everytime a downstream/course block is updated. This allows us to order links[1] based on recently + # modified downstream version. + # pylint: disable=line-too-long + # 1. https://github.com/open-craft/frontend-app-course-authoring/blob/0443d88824095f6f65a3a64b77244af590d4edff/src/course-libraries/ReviewTabContent.tsx#L222-L233 + has_changes = True # change to false once above condition is met. + for key, value in new_values.items(): + prev = getattr(link, key) + # None != None is True, so we need to check for it specially + if prev != value and ~(prev is None and value is None): + has_changes = True + setattr(link, key, value) + if has_changes: + link.updated = created + link.save() + except cls.DoesNotExist: + link = cls(**new_values) + link.created = created + link.updated = created + link.save() + return link class LearningContextLinksStatusChoices(models.TextChoices): @@ -275,7 +359,7 @@ class LearningContextLinksStatusChoices(models.TextChoices): class LearningContextLinksStatus(models.Model): """ - This table stores current processing status of upstream-downstream links in PublishableEntityLink table for a + This table stores current processing status of upstream-downstream links in ComponentLink table for a course or a learning context. """ context_key = CourseKeyField( diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_vertical_block.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_vertical_block.py index 7cac074a433f..68ef03ca8de9 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_vertical_block.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_vertical_block.py @@ -226,7 +226,7 @@ def test_children_content(self): "version_synced": 5, "version_available": None, "version_declined": None, - "error_message": "Linked library item was not found in the system", + "error_message": "Linked upstream library block was not found in the system", "ready_to_sync": False, }, "user_partition_info": expected_user_partition_info, @@ -236,7 +236,8 @@ def test_children_content(self): }, ] self.maxDiff = None - self.assertEqual(response.data["children"], expected_response) + # Using json() shows meaningful diff in case of error + self.assertEqual(response.json()["children"], expected_response) def test_not_valid_usage_key_string(self): """ diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/vertical_block.py b/cms/djangoapps/contentstore/rest_api/v1/views/vertical_block.py index 0798c341cc1c..0a5af1ab891b 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/vertical_block.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/vertical_block.py @@ -283,7 +283,7 @@ def get(self, request: Request, usage_key_string: str): child_info = modulestore().get_item(child) user_partition_info = get_visibility_partition_info(child_info, course=course) user_partitions = get_user_partition_info(child_info, course=course) - upstream_link = UpstreamLink.try_get_for_block(child_info) + upstream_link = UpstreamLink.try_get_for_block(child_info, log_error=False) validation_messages = get_xblock_validation_messages(child_info) render_error = get_xblock_render_error(request, child_info) diff --git a/cms/djangoapps/contentstore/rest_api/v2/serializers/__init__.py b/cms/djangoapps/contentstore/rest_api/v2/serializers/__init__.py index 1815b4435d25..4c275a1976b0 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/serializers/__init__.py +++ b/cms/djangoapps/contentstore/rest_api/v2/serializers/__init__.py @@ -1,13 +1,13 @@ """Module for v2 serializers.""" from cms.djangoapps.contentstore.rest_api.v2.serializers.downstreams import ( - PublishableEntityLinksSerializer, + ComponentLinksSerializer, PublishableEntityLinksSummarySerializer, ) from cms.djangoapps.contentstore.rest_api.v2.serializers.home import CourseHomeTabSerializerV2 __all__ = [ 'CourseHomeTabSerializerV2', - 'PublishableEntityLinksSerializer', + 'ComponentLinksSerializer', 'PublishableEntityLinksSummarySerializer', ] diff --git a/cms/djangoapps/contentstore/rest_api/v2/serializers/downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/serializers/downstreams.py index 1a2139d4cf91..390a32b6ed44 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/serializers/downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/serializers/downstreams.py @@ -4,19 +4,19 @@ from rest_framework import serializers -from cms.djangoapps.contentstore.models import PublishableEntityLink +from cms.djangoapps.contentstore.models import ComponentLink -class PublishableEntityLinksSerializer(serializers.ModelSerializer): +class ComponentLinksSerializer(serializers.ModelSerializer): """ Serializer for publishable entity links. """ upstream_context_title = serializers.CharField(read_only=True) - upstream_version = serializers.IntegerField(read_only=True) + upstream_version = serializers.IntegerField(read_only=True, source="upstream_version_num") ready_to_sync = serializers.BooleanField() class Meta: - model = PublishableEntityLink + model = ComponentLink exclude = ['upstream_block', 'uuid'] diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py index 29e6a3961dcc..39c2649118f5 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py @@ -48,7 +48,7 @@ /api/contentstore/v2/downstreams /api/contentstore/v2/downstreams?course_id=course-v1:A+B+C&ready_to_sync=true GET: List downstream blocks that can be synced, filterable by course or sync-readiness. - 200: A paginated list of applicable & accessible downstream blocks. Entries are PublishableEntityLinks. + 200: A paginated list of applicable & accessible downstream blocks. Entries are ComponentLinks. /api/contentstore/v2/downstreams//summary GET: List summary of links by course key @@ -87,6 +87,7 @@ from edx_rest_framework_extensions.paginators import DefaultPagination from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey +from opaque_keys.edx.locator import LibraryUsageLocatorV2, LibraryContainerLocator from rest_framework.exceptions import NotFound, ValidationError from rest_framework.fields import BooleanField from rest_framework.request import Request @@ -94,12 +95,12 @@ from rest_framework.views import APIView from xblock.core import XBlock -from cms.djangoapps.contentstore.helpers import import_static_assets_for_library_sync -from cms.djangoapps.contentstore.models import PublishableEntityLink +from cms.djangoapps.contentstore.models import ComponentLink from cms.djangoapps.contentstore.rest_api.v2.serializers import ( - PublishableEntityLinksSerializer, + ComponentLinksSerializer, PublishableEntityLinksSummarySerializer, ) +from cms.djangoapps.contentstore.xblock_storage_handlers.view_handlers import sync_library_content from cms.lib.xblock.upstream_sync import ( BadDownstream, BadUpstream, @@ -107,10 +108,10 @@ UpstreamLink, UpstreamLinkException, decline_sync, - fetch_customizable_fields, sever_upstream_link, - sync_from_upstream, ) +from cms.lib.xblock.upstream_sync_block import fetch_customizable_fields_from_block +from cms.lib.xblock.upstream_sync_container import fetch_customizable_fields_from_container from common.djangoapps.student.auth import has_studio_read_access, has_studio_write_access from openedx.core.lib.api.view_utils import ( DeveloperErrorViewMixin, @@ -183,9 +184,9 @@ def get(self, request: _AuthenticatedRequest): link_filter["upstream_usage_key"] = UsageKey.from_string(upstream_usage_key) except InvalidKeyError as exc: raise ValidationError(detail=f"Malformed usage key: {upstream_usage_key}") from exc - links = PublishableEntityLink.filter_links(**link_filter) + links = ComponentLink.filter_links(**link_filter) paginated_links = paginator.paginate_queryset(links, self.request, view=self) - serializer = PublishableEntityLinksSerializer(paginated_links, many=True) + serializer = ComponentLinksSerializer(paginated_links, many=True) return paginator.get_paginated_response(serializer.data, self.request) @@ -217,7 +218,7 @@ def get(self, request: _AuthenticatedRequest, course_key_string: str): course_key = CourseKey.from_string(course_key_string) except InvalidKeyError as exc: raise ValidationError(detail=f"Malformed course key: {course_key_string}") from exc - links = PublishableEntityLink.summarize_by_downstream_context(downstream_context_key=course_key) + links = ComponentLink.summarize_by_downstream_context(downstream_context_key=course_key) serializer = PublishableEntityLinksSummarySerializer(links, many=True) return Response(serializer.data) @@ -252,12 +253,21 @@ def put(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response raise ValidationError({"sync": "must be 'true' or 'false'"}) try: if sync_param == "true" or sync_param is True: - sync_from_upstream(downstream=downstream, user=request.user) + sync_library_content( + downstream=downstream, + request=request, + store=modulestore() + ) else: # Even if we're not syncing (i.e., updating the downstream's values with the upstream's), we still need # to fetch the upstream's customizable values and store them as hidden fields on the downstream. This # ensures that downstream authors can restore defaults based on the upstream. - fetch_customizable_fields(downstream=downstream, user=request.user) + link = UpstreamLink.get_for_block(downstream) + if isinstance(link.upstream_key, LibraryUsageLocatorV2): + fetch_customizable_fields_from_block(downstream=downstream, user=request.user) + else: + assert isinstance(link.upstream_key, LibraryContainerLocator) + fetch_customizable_fields_from_container(downstream=downstream, user=request.user) except BadDownstream as exc: logger.exception( "'%s' is an invalid downstream; refusing to set its upstream to '%s'", @@ -312,8 +322,11 @@ def post(self, request: _AuthenticatedRequest, usage_key_string: str) -> Respons if downstream.usage_key.block_type == "video": # Delete all transcripts so we can copy new ones from upstream clear_transcripts(downstream) - upstream = sync_from_upstream(downstream, request.user) - static_file_notices = import_static_assets_for_library_sync(downstream, upstream, request) + static_file_notices = sync_library_content( + downstream=downstream, + request=request, + store=modulestore() + ) except UpstreamLinkException as exc: logger.exception( "Could not sync from upstream '%s' to downstream '%s'", @@ -321,7 +334,6 @@ def post(self, request: _AuthenticatedRequest, usage_key_string: str) -> Respons usage_key_string, ) raise ValidationError(detail=str(exc)) from exc - modulestore().update_item(downstream, request.user.id) # Note: We call `get_for_block` (rather than `try_get_for_block`) because if anything is wrong with the # upstream at this point, then that is completely unexpected, so it's appropriate to let the 500 happen. response = UpstreamLink.get_for_block(downstream).to_json() diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstream_sync_integration.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstream_sync_integration.py new file mode 100644 index 000000000000..1541a0e5e0ba --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstream_sync_integration.py @@ -0,0 +1,462 @@ +""" +Unit and integration tests to ensure that syncing content from libraries to +courses is working. +""" +from typing import Any +from xml.etree import ElementTree + +import ddt +from opaque_keys.edx.keys import UsageKey + +from openedx.core.djangoapps.content_libraries.tests import ContentLibrariesRestApiTest +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory + + +@ddt.ddt +class CourseToLibraryTestCase(ContentLibrariesRestApiTest, ModuleStoreTestCase): + """ + Tests that involve syncing content from libraries to courses. + """ + maxDiff = None # Necessary for debugging OLX differences + + def setUp(self): + super().setUp() + # self.user is set up by ContentLibrariesRestApiTest + + # The source library (contains the upstreams): + self.library = self._create_library(slug="testlib", title="Upstream Library") + lib_id = self.library["id"] # the library ID as a string + self.upstream_problem1 = self._add_block_to_library(lib_id, "problem", "prob1", can_stand_alone=True) + self._set_library_block_olx( + self.upstream_problem1["id"], + 'multiple choice...' + ) + self.upstream_problem2 = self._add_block_to_library(lib_id, "problem", "prob2", can_stand_alone=True) + self._set_library_block_olx( + self.upstream_problem2["id"], + 'multi select...' + ) + self.upstream_html1 = self._add_block_to_library(lib_id, "html", "html1", can_stand_alone=False) + self._set_library_block_olx( + self.upstream_html1["id"], + 'This is the HTML.' + ) + self.upstream_unit = self._create_container(lib_id, "unit", slug="u1", display_name="Unit 1 Title") + self._add_container_components(self.upstream_unit["id"], [ + self.upstream_html1["id"], + self.upstream_problem1["id"], + self.upstream_problem2["id"], + ]) + self._commit_library_changes(lib_id) # publish everything + + # The destination course: + self.course = CourseFactory.create() + self.course_section = BlockFactory.create(category='chapter', parent=self.course) + self.course_subsection = BlockFactory.create(category='sequential', parent=self.course_section) + self.course_unit = BlockFactory.create(category='vertical', parent=self.course_subsection) + + def _get_sync_status(self, usage_key: str): + return self._api('get', f"/api/contentstore/v2/downstreams/{usage_key}", {}, expect_response=200) + + def _sync_downstream(self, usage_key: str): + return self._api('post', f"/api/contentstore/v2/downstreams/{usage_key}/sync", {}, expect_response=200) + + def _get_course_block_olx(self, usage_key: str): + data = self._api('get', f'/api/olx-export/v1/xblock/{usage_key}/', {}, expect_response=200) + return data["blocks"][data["root_block_id"]]["olx"] + + # def _get_course_block_fields(self, usage_key: str): + # return self._api('get', f'/xblock/{usage_key}', {}, expect_response=200) + + def _get_course_block_children(self, usage_key: str) -> list[str]: + """ Get the IDs of the child XBlocks of the given XBlock """ + # TODO: is there really no REST API to get the children of an XBlock in Studio? + # Maybe this one: /api/contentstore/v1/container/vertical/{usage_key_string}/children + return [str(k) for k in modulestore().get_item(UsageKey.from_string(usage_key), depth=0).children] + + def _create_block_from_upstream( + self, + block_category: str, + parent_usage_key: str, + upstream_key: str, + expect_response: int = 200, + ): + """ + Call the CMS API for inserting an XBlock that's cloned from a library + item. i.e. copy a *published* library block into a course, and create an + upstream link. + """ + return self._api('post', "/xblock/", { + "category": block_category, + "parent_locator": parent_usage_key, + "library_content_key": upstream_key, + }, expect_response=expect_response) + + def _update_course_block_fields(self, usage_key: str, fields: dict[str, Any] = None): + """ Update fields of an XBlock """ + return self._api('patch', f"/xblock/{usage_key}", { + "metadata": fields, + }, expect_response=200) + + def assertXmlEqual(self, xml_str_a: str, xml_str_b: str) -> bool: + """ Assert that the given XML strings are equal, ignoring attribute order and some whitespace variations. """ + self.assertEqual( + ElementTree.canonicalize(xml_str_a, strip_text=True), + ElementTree.canonicalize(xml_str_b, strip_text=True), + ) + + # OLX attributes that will appear on capa problems when saved/exported. Excludes "markdown" + standard_capa_attributes = """ + markdown_edited="false" + matlab_api_key="null" + name="null" + rerandomize="never" + source_code="null" + tags="[]" + use_latex_compiler="false" + """ + + #################################################################################################################### + + def test_problem_sync(self): + """ + Test that we can sync a problem from a library into a course. + """ + # 1️⃣ First, create the problem in the course, using the upstream problem as a template: + downstream_problem1 = self._create_block_from_upstream( + block_category="problem", + parent_usage_key=str(self.course_subsection.usage_key), + upstream_key=self.upstream_problem1["id"], + ) + status = self._get_sync_status(downstream_problem1["locator"]) + self.assertDictContainsEntries(status, { + 'upstream_ref': self.upstream_problem1["id"], # e.g. 'lb:CL-TEST:testlib:problem:prob1' + 'version_available': 2, + 'version_synced': 2, + 'version_declined': None, + 'ready_to_sync': False, + 'error_message': None, + # 'upstream_link': 'http://course-authoring-mfe/library/lib:CL-TEST:testlib/components?usageKey=...' + }) + assert status["upstream_link"].startswith("http://course-authoring-mfe/library/") + assert status["upstream_link"].endswith(f"/components?usageKey={self.upstream_problem1['id']}") + + # Check the OLX of the downstream block. Notice that: + # (1) fields display_name and markdown, as well as the 'data' (content/body of the ) are synced. + # (2) per UpstreamSyncMixin.get_customizable_fields(), some fields like weight and max_attempts are + # DROPPED entirely from the upstream version when creating the downstream: + self.assertXmlEqual(self._get_course_block_olx(downstream_problem1["locator"]), f""" + multiple choice... + """) + + # 2️⃣ Now, lets modify the upstream problem AND the downstream problem: + + self._update_course_block_fields(downstream_problem1["locator"], { + "display_name": "Custom Display Name", + "max_attempts": 3, + "markdown": "blow me away, scotty!", # This change will be lost + }) + + self._set_library_block_olx( + self.upstream_problem1["id"], + 'multiple choice v2...' + ) + self._publish_library_block(self.upstream_problem1["id"]) + + # Here's how the downstream OLX looks now, before we sync: + self.assertXmlEqual(self._get_course_block_olx(downstream_problem1["locator"]), f""" + multiple choice... + """) + + status = self._get_sync_status(downstream_problem1["locator"]) + self.assertDictContainsEntries(status, { + 'upstream_ref': self.upstream_problem1["id"], # e.g. 'lb:CL-TEST:testlib:problem:prob1' + 'version_available': 3, # <--- updated + 'version_synced': 2, + 'version_declined': None, + 'ready_to_sync': True, # <--- updated + 'error_message': None, + }) + + # 3️⃣ Now, sync and check the resulting OLX of the downstream + + self._sync_downstream(downstream_problem1["locator"]) + + # Here's how the downstream OLX looks now, after we synced it. + # Notice: + # (1) content like "markdown" and the body XML content are synced + # (2) the "display_name" is left alone (customized downstream), but + # (3) "upstream_display_name" is updated. + # (4) The customized "max_attempts" is also still present. + self.assertXmlEqual(self._get_course_block_olx(downstream_problem1["locator"]), f""" + multiple choice v2... + """) + + def test_unit_sync(self): + """ + Test that we can sync a unit from the library into the course + """ + # 1️⃣ Create a "vertical" block in the course based on a "unit" container: + downstream_unit = self._create_block_from_upstream( + # The API consumer needs to specify "vertical" here, even though upstream is "unit". + # In the future we could create a nicer REST API endpoint for this that's not part of + # the messy '/xblock/' API and which auto-detects the types based on the upstream_key. + block_category="vertical", + parent_usage_key=str(self.course_subsection.usage_key), + upstream_key=self.upstream_unit["id"], + ) + status = self._get_sync_status(downstream_unit["locator"]) + self.assertDictContainsEntries(status, { + 'upstream_ref': self.upstream_unit["id"], # e.g. 'lct:CL-TEST:testlib:unit:u1' + 'version_available': 2, + 'version_synced': 2, + 'version_declined': None, + 'ready_to_sync': False, + 'error_message': None, + # 'upstream_link': 'http://course-authoring-mfe/library/lib:CL-TEST:testlib/units/...' + }) + assert status["upstream_link"].startswith("http://course-authoring-mfe/library/") + assert status["upstream_link"].endswith(f"/units/{self.upstream_unit['id']}") + + # Check that the downstream container matches our expectations. + # Note that: + # (1) Every XBlock has an "upstream" field + # (2) some "downstream only" fields like weight and max_attempts are omitted. + self.assertXmlEqual(self._get_course_block_olx(downstream_unit["locator"]), f""" + + This is the HTML. + multiple choice... + multi select... + + """) + + # 2️⃣ Now, lets modify the upstream problem 1: + + self._set_library_block_olx( + self.upstream_problem1["id"], + 'multiple choice v2...' + ) + self._publish_container(self.upstream_unit["id"]) + + status = self._get_sync_status(downstream_unit["locator"]) + self.assertDictContainsEntries(status, { + 'upstream_ref': self.upstream_unit["id"], # e.g. 'lct:CL-TEST:testlib:unit:u1' + 'version_available': 2, # <--- not updated since we didn't directly modify the unit + 'version_synced': 2, + 'version_declined': None, + # FIXME: ready_to_sync should be true, since a child block needs syncing. + # This may need to be fixed post-Teak, as syncing the children directly is still possible. + 'ready_to_sync': False, + 'error_message': None, + }) + + # Check the upstream/downstream status of [one of] the children + + downstream_problem1 = self._get_course_block_children(downstream_unit["locator"])[1] + assert "type@problem" in downstream_problem1 + self.assertDictContainsEntries(self._get_sync_status(downstream_problem1), { + 'upstream_ref': self.upstream_problem1["id"], + 'version_available': 3, # <--- updated since we modified the problem + 'version_synced': 2, + 'version_declined': None, + 'ready_to_sync': True, # <--- updated + 'error_message': None, + }) + + # 3️⃣ Now, sync and check the resulting OLX of the downstream + + self._sync_downstream(downstream_unit["locator"]) + + self.assertXmlEqual(self._get_course_block_olx(downstream_unit["locator"]), f""" + + This is the HTML. + + multiple choice v2... + multi select... + + """) + + # Now, add and delete a component + upstream_problem3 = self._add_block_to_library( + self.library["id"], + "problem", + "prob3", + can_stand_alone=True + ) + self._set_library_block_olx( + upstream_problem3["id"], + 'single select...' + ) + self._add_container_components(self.upstream_unit["id"], [upstream_problem3["id"]]) + self._remove_container_components(self.upstream_unit["id"], [self.upstream_problem2["id"]]) + self._commit_library_changes(self.library["id"]) # publish everything + + status = self._get_sync_status(downstream_unit["locator"]) + self.assertDictContainsEntries(status, { + 'upstream_ref': self.upstream_unit["id"], # e.g. 'lct:CL-TEST:testlib:unit:u1' + 'version_available': 4, # <--- updated twice, delete and add component + 'version_synced': 2, + 'version_declined': None, + 'ready_to_sync': True, + 'error_message': None, + }) + + # 3️⃣ Now, sync and check the resulting OLX of the downstream + + self._sync_downstream(downstream_unit["locator"]) + self.assertXmlEqual(self._get_course_block_olx(downstream_unit["locator"]), f""" + + This is the HTML. + multiple choice v2... + + + single select... + + """) + + # Now, reorder components + self._patch_container_components(self.upstream_unit["id"], [ + upstream_problem3["id"], + self.upstream_problem1["id"], + self.upstream_html1["id"], + ]) + self._publish_container(self.upstream_unit["id"]) + + # 3️⃣ Now, sync and check the resulting OLX of the downstream + + self._sync_downstream(downstream_unit["locator"]) + self.assertXmlEqual(self._get_course_block_olx(downstream_unit["locator"]), f""" + + + single select... + + multiple choice v2... + + This is the HTML. + + """) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py index ca590273677f..426b49dc53e7 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py @@ -13,6 +13,7 @@ from cms.djangoapps.contentstore.helpers import StaticFileNotices from cms.lib.xblock.upstream_sync import BadUpstream, UpstreamLink from cms.djangoapps.contentstore.tests.utils import CourseTestCase +from cms.djangoapps.contentstore.xblock_storage_handlers import view_handlers as xblock_view_handlers from opaque_keys.edx.keys import UsageKey from common.djangoapps.student.tests.factories import UserFactory from xmodule.modulestore.django import modulestore @@ -32,6 +33,7 @@ def _get_upstream_link_good_and_syncable(downstream): return UpstreamLink( upstream_ref=downstream.upstream, + upstream_key=UsageKey.from_string(downstream.upstream), version_synced=downstream.upstream_version, version_available=(downstream.upstream_version or 0) + 1, version_declined=downstream.upstream_version_declined, @@ -235,8 +237,8 @@ def call_api(self, usage_key_string, sync: str | None = None): content_type="application/json", ) - @patch.object(downstreams_views, "fetch_customizable_fields") - @patch.object(downstreams_views, "sync_from_upstream") + @patch.object(downstreams_views, "fetch_customizable_fields_from_block") + @patch.object(downstreams_views, "sync_library_content") @patch.object(UpstreamLink, "get_for_block", _get_upstream_link_good_and_syncable) def test_200_with_sync(self, mock_sync, mock_fetch): """ @@ -250,8 +252,8 @@ def test_200_with_sync(self, mock_sync, mock_fetch): assert mock_fetch.call_count == 0 assert video_after.upstream == self.video_lib_id - @patch.object(downstreams_views, "fetch_customizable_fields") - @patch.object(downstreams_views, "sync_from_upstream") + @patch.object(downstreams_views, "fetch_customizable_fields_from_block") + @patch.object(downstreams_views, "sync_library_content") @patch.object(UpstreamLink, "get_for_block", _get_upstream_link_good_and_syncable) def test_200_no_sync(self, mock_sync, mock_fetch): """ @@ -265,7 +267,9 @@ def test_200_no_sync(self, mock_sync, mock_fetch): assert mock_fetch.call_count == 1 assert video_after.upstream == self.video_lib_id - @patch.object(downstreams_views, "fetch_customizable_fields", side_effect=BadUpstream(MOCK_UPSTREAM_ERROR)) + @patch.object( + downstreams_views, "fetch_customizable_fields_from_block", side_effect=BadUpstream(MOCK_UPSTREAM_ERROR), + ) def test_400(self, sync: str): """ Do we raise a 400 if the provided upstream reference is malformed or not accessible? @@ -366,7 +370,7 @@ def test_200(self): @patch("cms.djangoapps.contentstore.helpers._insert_static_files_into_downstream_xblock") @patch("cms.djangoapps.contentstore.helpers.content_staging_api.stage_xblock_temporarily") - @patch("cms.djangoapps.contentstore.xblock_storage_handlers.view_handlers.sync_from_upstream") + @patch("cms.djangoapps.contentstore.xblock_storage_handlers.view_handlers.sync_from_upstream_block") def test_200_video(self, mock_sync, mock_stage, mock_insert): mock_lib_block = MagicMock() mock_lib_block.runtime.get_block_assets.return_value = ['mocked_asset'] @@ -394,17 +398,15 @@ def call_api(self, usage_key_string): return self.client.post(f"/api/contentstore/v2/downstreams/{usage_key_string}/sync") @patch.object(UpstreamLink, "get_for_block", _get_upstream_link_good_and_syncable) - @patch.object(downstreams_views, "sync_from_upstream") - @patch.object(downstreams_views, "import_static_assets_for_library_sync", return_value=StaticFileNotices()) + @patch.object(xblock_view_handlers, "import_static_assets_for_library_sync", return_value=StaticFileNotices()) @patch.object(downstreams_views, "clear_transcripts") - def test_200(self, mock_sync_from_upstream, mock_import_staged_content, mock_clear_transcripts): + def test_200(self, mock_import_staged_content, mock_clear_transcripts): """ Does the happy path work? """ self.client.login(username="superuser", password="password") response = self.call_api(self.downstream_video_key) assert response.status_code == 200 - assert mock_sync_from_upstream.call_count == 1 assert mock_import_staged_content.call_count == 1 assert mock_clear_transcripts.call_count == 1 diff --git a/cms/djangoapps/contentstore/signals/handlers.py b/cms/djangoapps/contentstore/signals/handlers.py index 5635d4655622..b8be6ee84f8b 100644 --- a/cms/djangoapps/contentstore/signals/handlers.py +++ b/cms/djangoapps/contentstore/signals/handlers.py @@ -44,7 +44,7 @@ from xmodule.modulestore.django import SignalHandler, modulestore from xmodule.modulestore.exceptions import ItemNotFoundError -from ..models import PublishableEntityLink +from ..models import ComponentLink, ContainerLink from ..tasks import ( create_or_update_upstream_links, handle_create_or_update_xblock_upstream_link, @@ -230,7 +230,7 @@ def handle_item_deleted(**kwargs): gating_api.set_required_content(course_key, block.location, None, None, None) id_list.add(block.location) - PublishableEntityLink.objects.filter(downstream_usage_key__in=id_list).delete() + ComponentLink.objects.filter(downstream_usage_key__in=id_list).delete() @receiver(GRADING_POLICY_CHANGED) @@ -278,7 +278,10 @@ def delete_upstream_downstream_link_handler(**kwargs): log.error("Received null or incorrect data for event") return - PublishableEntityLink.objects.filter( + ComponentLink.objects.filter( + downstream_usage_key=xblock_info.usage_key + ).delete() + ContainerLink.objects.filter( downstream_usage_key=xblock_info.usage_key ).delete() diff --git a/cms/djangoapps/contentstore/tasks.py b/cms/djangoapps/contentstore/tasks.py index e36a8edae8a2..b6cb2af53d56 100644 --- a/cms/djangoapps/contentstore/tasks.py +++ b/cms/djangoapps/contentstore/tasks.py @@ -84,7 +84,7 @@ from xmodule.modulestore.xml_exporter import export_course_to_xml, export_library_to_xml from xmodule.modulestore.xml_importer import CourseImportException, import_course_from_xml, import_library_from_xml -from .models import LearningContextLinksStatus, LearningContextLinksStatusChoices, PublishableEntityLink +from .models import ContainerLink, LearningContextLinksStatus, LearningContextLinksStatusChoices, ComponentLink from .outlines import update_outline_from_modulestore from .outlines_regenerate import CourseOutlineRegenerate from .toggles import bypass_olx_failure_enabled @@ -1475,7 +1475,8 @@ def create_or_update_upstream_links( updated=created, ) if replace: - PublishableEntityLink.objects.filter(downstream_context_key=course_key).delete() + ComponentLink.objects.filter(downstream_context_key=course_key).delete() + ContainerLink.objects.filter(downstream_context_key=course_key).delete() try: xblocks = store.get_items(course_key, settings={"upstream": lambda x: x is not None}) except ItemNotFoundError: @@ -1483,7 +1484,7 @@ def create_or_update_upstream_links( course_status.update_status(LearningContextLinksStatusChoices.FAILED) return for xblock in xblocks: - create_or_update_xblock_upstream_link(xblock, course_key_str, created) + create_or_update_xblock_upstream_link(xblock, course_key, created) course_status.update_status(LearningContextLinksStatusChoices.COMPLETED) @@ -1501,7 +1502,11 @@ def handle_unlink_upstream_block(upstream_usage_key_string: str) -> None: LOGGER.exception(f'Invalid upstream usage_key: {upstream_usage_key_string}') return - for link in PublishableEntityLink.objects.filter( + for link in ComponentLink.objects.filter( + upstream_usage_key=upstream_usage_key, + ): + make_copied_tags_editable(str(link.downstream_usage_key)) + for link in ContainerLink.objects.filter( upstream_usage_key=upstream_usage_key, ): make_copied_tags_editable(str(link.downstream_usage_key)) diff --git a/cms/djangoapps/contentstore/tests/test_upstream_downstream_links.py b/cms/djangoapps/contentstore/tests/test_upstream_downstream_links.py index 3f0703d8b31e..90fec8471651 100644 --- a/cms/djangoapps/contentstore/tests/test_upstream_downstream_links.py +++ b/cms/djangoapps/contentstore/tests/test_upstream_downstream_links.py @@ -9,7 +9,7 @@ from django.core.management.base import CommandError from django.test import TestCase from opaque_keys.edx.keys import CourseKey -from opaque_keys.edx.locator import LibraryUsageLocatorV2 +from opaque_keys.edx.locator import LibraryContainerLocator, LibraryUsageLocatorV2 from openedx_events.tests.utils import OpenEdxEventsTestMixin from common.djangoapps.student.tests.factories import UserFactory @@ -17,7 +17,7 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory -from ..models import LearningContextLinksStatus, LearningContextLinksStatusChoices, PublishableEntityLink +from ..models import ContainerLink, LearningContextLinksStatus, LearningContextLinksStatusChoices, ComponentLink class BaseUpstreamLinksHelpers(TestCase): @@ -44,6 +44,39 @@ def _create_block(self, num: int, category="html"): upstream_version=num, ) + def _create_unit(self, num: int): + """ + Create xblock with random upstream key and version number. + """ + random_upstream = LibraryContainerLocator.from_string( + f"lct:OpenedX:CSPROB2:unit:{uuid4()}" + ) + return random_upstream, BlockFactory.create( + parent=self.sequence, # pylint: disable=attribute-defined-outside-init + category='vertical', + display_name=f"An unit Block - {num}", + upstream=str(random_upstream), + upstream_version=num, + ) + + def _create_unit_and_expected_container_link(self, course_key: str | CourseKey, num_blocks: int = 3): + """ + Create unit xblock with random upstream key and version number. + """ + data = [] + for i in range(num_blocks): + upstream, block = self._create_unit(i + 1) + data.append({ + "upstream_container": None, + "downstream_context_key": course_key, + "downstream_usage_key": block.usage_key, + "upstream_container_key": upstream, + "upstream_context_key": str(upstream.context_key), + "version_synced": i + 1, + "version_declined": None, + }) + return data + def _create_block_and_expected_links_data(self, course_key: str | CourseKey, num_blocks: int = 3): """ Creates xblocks and its expected links data for given course_key @@ -62,11 +95,11 @@ def _create_block_and_expected_links_data(self, course_key: str | CourseKey, num }) return data - def _compare_links(self, course_key, expected): + def _compare_links(self, course_key, expected_component_links, expected_container_links): """ Compares links for given course with passed expected list of dicts. """ - links = list(PublishableEntityLink.objects.filter(downstream_context_key=course_key).values( + links = list(ComponentLink.objects.filter(downstream_context_key=course_key).values( 'upstream_block', 'upstream_usage_key', 'upstream_context_key', @@ -75,7 +108,17 @@ def _compare_links(self, course_key, expected): 'version_synced', 'version_declined', )) - self.assertListEqual(links, expected) + self.assertListEqual(links, expected_component_links) + container_links = list(ContainerLink.objects.filter(downstream_context_key=course_key).values( + 'upstream_container', + 'upstream_container_key', + 'upstream_context_key', + 'downstream_usage_key', + 'downstream_context_key', + 'version_synced', + 'version_declined', + )) + self.assertListEqual(container_links, expected_container_links) @skip_unless_cms @@ -95,16 +138,19 @@ def setUp(self): with self.store.bulk_operations(course_key_1): self._set_course_data(course_1) self.expected_links_1 = self._create_block_and_expected_links_data(course_key_1) + self.expected_container_links_1 = self._create_unit_and_expected_container_link(course_key_1) self.course_2 = course_2 = CourseFactory.create(emit_signals=True) self.course_key_2 = course_key_2 = self.course_2.id with self.store.bulk_operations(course_key_2): self._set_course_data(course_2) self.expected_links_2 = self._create_block_and_expected_links_data(course_key_2) + self.expected_container_links_2 = self._create_unit_and_expected_container_link(course_key_2) self.course_3 = course_3 = CourseFactory.create(emit_signals=True) self.course_key_3 = course_key_3 = self.course_3.id with self.store.bulk_operations(course_key_3): self._set_course_data(course_3) self.expected_links_3 = self._create_block_and_expected_links_data(course_key_3) + self.expected_container_links_3 = self._create_unit_and_expected_container_link(course_key_3) def call_command(self, *args, **kwargs): """ @@ -132,14 +178,14 @@ def test_call_for_single_course(self): """ # Pre-checks assert not LearningContextLinksStatus.objects.filter(context_key=str(self.course_key_1)).exists() - assert not PublishableEntityLink.objects.filter(downstream_context_key=self.course_key_1).exists() + assert not ComponentLink.objects.filter(downstream_context_key=self.course_key_1).exists() # Run command self.call_command('--course', str(self.course_key_1)) # Post verfication assert LearningContextLinksStatus.objects.filter( context_key=str(self.course_key_1) ).first().status == LearningContextLinksStatusChoices.COMPLETED - self._compare_links(self.course_key_1, self.expected_links_1) + self._compare_links(self.course_key_1, self.expected_links_1, self.expected_container_links_1) def test_call_for_multiple_course(self): """ @@ -147,9 +193,9 @@ def test_call_for_multiple_course(self): """ # Pre-checks assert not LearningContextLinksStatus.objects.filter(context_key=str(self.course_key_2)).exists() - assert not PublishableEntityLink.objects.filter(downstream_context_key=self.course_key_2).exists() + assert not ComponentLink.objects.filter(downstream_context_key=self.course_key_2).exists() assert not LearningContextLinksStatus.objects.filter(context_key=str(self.course_key_3)).exists() - assert not PublishableEntityLink.objects.filter(downstream_context_key=self.course_key_3).exists() + assert not ComponentLink.objects.filter(downstream_context_key=self.course_key_3).exists() # Run command self.call_command('--course', str(self.course_key_2), '--course', str(self.course_key_3)) @@ -161,8 +207,8 @@ def test_call_for_multiple_course(self): assert LearningContextLinksStatus.objects.filter( context_key=str(self.course_key_3) ).first().status == LearningContextLinksStatusChoices.COMPLETED - self._compare_links(self.course_key_2, self.expected_links_2) - self._compare_links(self.course_key_3, self.expected_links_3) + self._compare_links(self.course_key_2, self.expected_links_2, self.expected_container_links_2) + self._compare_links(self.course_key_3, self.expected_links_3, self.expected_container_links_3) def test_call_for_all_courses(self): """ @@ -170,7 +216,7 @@ def test_call_for_all_courses(self): """ # Delete all links and status just to make sure --all option works LearningContextLinksStatus.objects.all().delete() - PublishableEntityLink.objects.all().delete() + ComponentLink.objects.all().delete() # Pre-checks assert not LearningContextLinksStatus.objects.filter(context_key=str(self.course_key_1)).exists() assert not LearningContextLinksStatus.objects.filter(context_key=str(self.course_key_2)).exists() @@ -189,9 +235,9 @@ def test_call_for_all_courses(self): assert LearningContextLinksStatus.objects.filter( context_key=str(self.course_key_3) ).first().status == LearningContextLinksStatusChoices.COMPLETED - self._compare_links(self.course_key_1, self.expected_links_1) - self._compare_links(self.course_key_2, self.expected_links_2) - self._compare_links(self.course_key_3, self.expected_links_3) + self._compare_links(self.course_key_1, self.expected_links_1, self.expected_container_links_1) + self._compare_links(self.course_key_2, self.expected_links_2, self.expected_container_links_2) + self._compare_links(self.course_key_3, self.expected_links_3, self.expected_container_links_3) def test_call_for_invalid_course(self): """ @@ -239,16 +285,19 @@ def setUp(self): with self.store.bulk_operations(course_key_1): self._set_course_data(course_1) self.expected_links_1 = self._create_block_and_expected_links_data(course_key_1) + self.expected_container_links_1 = self._create_unit_and_expected_container_link(course_key_1) self.course_2 = course_2 = CourseFactory.create(emit_signals=True) self.course_key_2 = course_key_2 = self.course_2.id with self.store.bulk_operations(course_key_2): self._set_course_data(course_2) self.expected_links_2 = self._create_block_and_expected_links_data(course_key_2) + self.expected_container_links_2 = self._create_unit_and_expected_container_link(course_key_2) self.course_3 = course_3 = CourseFactory.create(emit_signals=True) self.course_key_3 = course_key_3 = self.course_3.id with self.store.bulk_operations(course_key_3): self._set_course_data(course_3) self.expected_links_3 = self._create_block_and_expected_links_data(course_key_3) + self.expected_container_links_3 = self._create_unit_and_expected_container_link(course_key_3) def test_create_or_update_events(self): """ @@ -257,18 +306,23 @@ def test_create_or_update_events(self): assert not LearningContextLinksStatus.objects.filter(context_key=str(self.course_key_1)).exists() assert not LearningContextLinksStatus.objects.filter(context_key=str(self.course_key_2)).exists() assert not LearningContextLinksStatus.objects.filter(context_key=str(self.course_key_3)).exists() - assert PublishableEntityLink.objects.filter(downstream_context_key=self.course_key_1).count() == 3 - assert PublishableEntityLink.objects.filter(downstream_context_key=self.course_key_2).count() == 3 - assert PublishableEntityLink.objects.filter(downstream_context_key=self.course_key_3).count() == 3 - self._compare_links(self.course_key_1, self.expected_links_1) - self._compare_links(self.course_key_2, self.expected_links_2) - self._compare_links(self.course_key_3, self.expected_links_3) + assert ComponentLink.objects.filter(downstream_context_key=self.course_key_1).count() == 3 + assert ComponentLink.objects.filter(downstream_context_key=self.course_key_2).count() == 3 + assert ComponentLink.objects.filter(downstream_context_key=self.course_key_3).count() == 3 + self._compare_links(self.course_key_1, self.expected_links_1, self.expected_container_links_1) + self._compare_links(self.course_key_2, self.expected_links_2, self.expected_container_links_2) + self._compare_links(self.course_key_3, self.expected_links_3, self.expected_container_links_3) def test_delete_handler(self): """ Test whether links are deleted on deletion of xblock. """ usage_key = self.expected_links_1[0]["downstream_usage_key"] - assert PublishableEntityLink.objects.filter(downstream_usage_key=usage_key).exists() + assert ComponentLink.objects.filter(downstream_usage_key=usage_key).exists() + self.store.delete_item(usage_key, self.user.id) + assert not ComponentLink.objects.filter(downstream_usage_key=usage_key).exists() + + usage_key = self.expected_container_links_1[0]["downstream_usage_key"] + assert ContainerLink.objects.filter(downstream_usage_key=usage_key).exists() self.store.delete_item(usage_key, self.user.id) - assert not PublishableEntityLink.objects.filter(downstream_usage_key=usage_key).exists() + assert not ContainerLink.objects.filter(downstream_usage_key=usage_key).exists() diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 1ff309aa6e50..1a670e8fecc7 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -24,8 +24,9 @@ from help_tokens.core import HelpUrlExpert from lti_consumer.models import CourseAllowPIISharingInLTIFlag from milestones import api as milestones_api +from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey, UsageKeyV2 -from opaque_keys.edx.locator import LibraryLocator +from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocator from openedx_events.content_authoring.data import DuplicatedXBlockData from openedx_events.content_authoring.signals import XBLOCK_DUPLICATED from openedx_events.learning.data import CourseNotificationData @@ -86,6 +87,7 @@ from common.djangoapps.xblock_django.api import deprecated_xblocks from common.djangoapps.xblock_django.user_service import DjangoXBlockUserService from openedx.core import toggles as core_toggles +from openedx.core.djangoapps.content_libraries.api import get_container_from_key from openedx.core.djangoapps.content_tagging.toggles import is_tagging_feature_disabled from openedx.core.djangoapps.credit.api import get_credit_requirements, is_credit_course from openedx.core.djangoapps.discussions.config.waffle import ENABLE_PAGES_AND_RESOURCES_MICROFRONTEND @@ -113,7 +115,7 @@ ) from xmodule.services import ConfigurationService, SettingsService, TeamsConfigurationService -from .models import PublishableEntityLink +from .models import ComponentLink, ContainerLink IMPORTABLE_FILE_TYPES = ('.tar.gz', '.zip') log = logging.getLogger(__name__) @@ -2372,19 +2374,17 @@ def get_xblock_render_context(request, block): return "" -def create_or_update_xblock_upstream_link(xblock, course_key: str | CourseKey, created: datetime | None = None) -> None: +def _create_or_update_component_link(course_key: CourseKey, created: datetime | None, xblock): """ - Create or update upstream->downstream link in database for given xblock. + Create or update upstream->downstream link for components in database for given xblock. """ - if not xblock.upstream: - return None upstream_usage_key = UsageKeyV2.from_string(xblock.upstream) try: lib_component = get_component_from_usage_key(upstream_usage_key) except ObjectDoesNotExist: log.error(f"Library component not found for {upstream_usage_key}") lib_component = None - PublishableEntityLink.update_or_create( + ComponentLink.update_or_create( lib_component, upstream_usage_key=upstream_usage_key, upstream_context_key=str(upstream_usage_key.context_key), @@ -2394,3 +2394,43 @@ def create_or_update_xblock_upstream_link(xblock, course_key: str | CourseKey, c version_declined=xblock.upstream_version_declined, created=created, ) + + +def _create_or_update_container_link(course_key: CourseKey, created: datetime | None, xblock): + """ + Create or update upstream->downstream link for containers in database for given xblock. + """ + upstream_container_key = LibraryContainerLocator.from_string(xblock.upstream) + try: + lib_component = get_container_from_key(upstream_container_key) + except ObjectDoesNotExist: + log.error(f"Library component not found for {upstream_container_key}") + lib_component = None + ContainerLink.update_or_create( + lib_component, + upstream_container_key=upstream_container_key, + upstream_context_key=str(upstream_container_key.context_key), + downstream_context_key=course_key, + downstream_usage_key=xblock.usage_key, + version_synced=xblock.upstream_version, + version_declined=xblock.upstream_version_declined, + created=created, + ) + + +def create_or_update_xblock_upstream_link(xblock, course_key: CourseKey, created: datetime | None = None) -> None: + """ + Create or update upstream->downstream link in database for given xblock. + """ + if not xblock.upstream: + return None + try: + # Try to create component link + _create_or_update_component_link(course_key, created, xblock) + except InvalidKeyError: + # It is possible that the upstream is a container and UsageKeyV2 parse failed + # Create upstream container link + try: + _create_or_update_container_link(course_key, created, xblock) + except InvalidKeyError: + log.error(f"Invalid key: {xblock.upstream}") diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index aa7421bb87b3..c98e3d764148 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -11,6 +11,7 @@ from django.utils.translation import gettext as _ from django.views.decorators.clickjacking import xframe_options_exempt from opaque_keys.edx.keys import UsageKey +from opaque_keys.edx.locator import LibraryContainerLocator from rest_framework.request import Request from web_fragments.fragment import Fragment from xblock.django.request import django_to_webob_request, webob_to_django_response @@ -29,6 +30,7 @@ from cms.djangoapps.xblock_config.models import StudioConfig from cms.djangoapps.contentstore.toggles import individualize_anonymous_user_id from cms.lib.xblock.field_data import CmsFieldData +from cms.lib.xblock.upstream_sync import UpstreamLink from common.djangoapps.static_replace.services import ReplaceURLService from common.djangoapps.static_replace.wrapper import replace_urls_wrapper from common.djangoapps.student.models import anonymous_id_for_user @@ -299,8 +301,17 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False): if selected_groups_label: selected_groups_label = _('Access restricted to: {list_of_groups}').format(list_of_groups=selected_groups_label) # lint-amnesty, pylint: disable=line-too-long course = modulestore().get_course(xblock.location.course_key) + can_edit = context.get('can_edit', True) can_add = context.get('can_add', True) + upstream_link = UpstreamLink.try_get_for_block(root_xblock, log_error=False) + if upstream_link.error_message is None and isinstance(upstream_link.upstream_key, LibraryContainerLocator): + # If this unit is linked to a library unit, for now we make it completely read-only + # because when it is synced, all local changes like added components will be lost. + # (This is only on the frontend; the backend doesn't enforce it) + can_edit = False + can_add = False + # Is this a course or a library? is_course = xblock.context_key.is_course tags_count_map = context.get('tags_count_map') @@ -315,7 +326,7 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False): 'is_root': is_root, 'is_reorderable': is_reorderable, 'can_edit': can_edit, - 'can_edit_visibility': context.get('can_edit_visibility', is_course), + 'can_edit_visibility': can_edit and context.get('can_edit_visibility', is_course), 'course_authoring_url': settings.COURSE_AUTHORING_MICROFRONTEND_URL, 'is_loading': context.get('is_loading', False), 'is_selected': context.get('is_selected', False), diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index 3506825ae357..54beb783f3b0 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -10,6 +10,7 @@ """ import logging from datetime import datetime +from uuid import uuid4 from attrs import asdict from django.conf import settings @@ -19,6 +20,7 @@ from django.http import HttpResponse, HttpResponseBadRequest from django.utils.translation import gettext as _ from edx_django_utils.plugins import pluggable_override +from openedx.core.djangoapps.content_libraries.api import LibraryXBlockMetadata from openedx.core.djangoapps.content_tagging.api import get_object_tag_counts from edx_proctoring.api import ( does_backend_support_onboarding, @@ -27,15 +29,18 @@ ) from edx_proctoring.exceptions import ProctoredExamNotFoundException from help_tokens.core import HelpUrlExpert -from opaque_keys.edx.locator import LibraryUsageLocator +from opaque_keys.edx.locator import LibraryUsageLocator, LibraryUsageLocatorV2 from pytz import UTC from xblock.core import XBlock from xblock.fields import Scope from cms.djangoapps.contentstore.config.waffle import SHOW_REVIEW_RULES_FLAG +from cms.djangoapps.contentstore.helpers import StaticFileNotices from cms.djangoapps.models.settings.course_grading import CourseGradingModel from cms.lib.ai_aside_summary_config import AiAsideSummaryConfig -from cms.lib.xblock.upstream_sync import BadUpstream, sync_from_upstream +from cms.lib.xblock.upstream_sync import BadUpstream, UpstreamLink +from cms.lib.xblock.upstream_sync_block import sync_from_upstream_block +from cms.lib.xblock.upstream_sync_container import sync_from_upstream_container from common.djangoapps.static_replace import replace_static_urls from common.djangoapps.student.auth import ( has_studio_read_access, @@ -77,6 +82,7 @@ from .create_xblock import create_xblock from .xblock_helpers import usage_key_with_run from ..helpers import ( + concat_static_file_notices, get_parent_xblock, import_staged_content_from_user_clipboard, import_static_assets_for_library_sync, @@ -522,6 +528,59 @@ def create_item(request): return _create_block(request) +def sync_library_content(downstream: XBlock, request, store) -> StaticFileNotices: + """ + Handle syncing library content for given xblock depending on its upstream type. + It can sync unit containers and lower level xblocks. + """ + link = UpstreamLink.get_for_block(downstream) + upstream_key = link.upstream_key + if isinstance(upstream_key, LibraryUsageLocatorV2): + lib_block = sync_from_upstream_block(downstream=downstream, user=request.user) + static_file_notices = import_static_assets_for_library_sync(downstream, lib_block, request) + store.update_item(downstream, request.user.id) + else: + with store.bulk_operations(downstream.usage_key.context_key): + upstream_children = sync_from_upstream_container(downstream=downstream, user=request.user) + downstream_children = downstream.get_children() + downstream_children_keys = [child.upstream for child in downstream_children] + # Sync the children: + notices = [] + # Store final children keys to update order of components in unit + children = [] + for i in range(len(upstream_children)): + upstream_child = upstream_children[i] + assert isinstance(upstream_child, LibraryXBlockMetadata) # for now we only support units + if upstream_child.usage_key not in downstream_children_keys: + # This upstream_child is new, create it. + downstream_child = store.create_child( + parent_usage_key=downstream.usage_key, + position=i, + user_id=request.user.id, + block_type=upstream_child.usage_key.block_type, + # TODO: Can we generate a unique but friendly block_id, perhaps using upstream block_id + block_id=f"{upstream_child.usage_key.block_type}{uuid4().hex[:8]}", + fields={ + "upstream": str(upstream_child.usage_key), + }, + ) + else: + downstream_child_old_index = downstream_children_keys.index(upstream_child.usage_key) + downstream_child = downstream_children[downstream_child_old_index] + + result = sync_library_content(downstream=downstream_child, request=request, store=store) + children.append(downstream_child.usage_key) + notices.append(result) + for child in downstream_children: + if child.usage_key not in children: + # This downstream block was added, or deleted from upstream block. + store.delete_item(child.usage_key, user_id=request.user.id) + downstream.children = children + store.update_item(downstream, request.user.id) + static_file_notices = concat_static_file_notices(notices) + return static_file_notices + + @login_required @expect_json def _create_block(request): @@ -590,10 +649,11 @@ def _create_block(request): # If it contains library_content_key, the block is being imported from a v2 library # so it needs to be synced with upstream block. if upstream_ref := request.json.get("library_content_key"): + # Set `created_block.upstream` and then sync this with the upstream (library) version. + created_block.upstream = upstream_ref try: - # Set `created_block.upstream` and then sync this with the upstream (library) version. - created_block.upstream = upstream_ref - lib_block = sync_from_upstream(downstream=created_block, user=request.user) + store = modulestore() + static_file_notices = sync_library_content(created_block, request, store) except BadUpstream as exc: _delete_item(created_block.location, request.user) log.exception( @@ -601,8 +661,6 @@ def _create_block(request): f"using provided library_content_key='{upstream_ref}'" ) return JsonResponse({"error": str(exc)}, status=400) - static_file_notices = import_static_assets_for_library_sync(created_block, lib_block, request) - modulestore().update_item(created_block, request.user.id) response["upstreamRef"] = upstream_ref response["static_file_notices"] = asdict(static_file_notices) response["parent_locator"] = parent_locator @@ -1220,6 +1278,10 @@ def create_xblock_info( # lint-amnesty, pylint: disable=too-many-statements if is_xblock_unit: # if xblock is a Unit we add the discussion_enabled option xblock_info["discussion_enabled"] = xblock.discussion_enabled + + # Also add upstream info + xblock_info["upstream_info"] = UpstreamLink.try_get_for_block(xblock, log_error=False).to_json() + if xblock.category == "sequential": # Entrance exam subsection should be hidden. in_entrance_exam is # inherited metadata, all children will have it. diff --git a/cms/lib/xblock/test/test_upstream_sync.py b/cms/lib/xblock/test/test_upstream_sync.py index 71fa7d51bb9d..8902d67c8cb1 100644 --- a/cms/lib/xblock/test/test_upstream_sync.py +++ b/cms/lib/xblock/test/test_upstream_sync.py @@ -2,23 +2,27 @@ Test CMS's upstream->downstream syncing system """ import datetime -import ddt -from pytz import utc +import ddt from organizations.api import ensure_organization from organizations.models import Organization +from pytz import utc from cms.lib.xblock.upstream_sync import ( + BadDownstream, + BadUpstream, + NoUpstream, UpstreamLink, - sync_from_upstream, decline_sync, fetch_customizable_fields, sever_upstream_link, - NoUpstream, BadUpstream, BadDownstream, + decline_sync, + sever_upstream_link, ) +from cms.lib.xblock.upstream_sync_block import sync_from_upstream_block, fetch_customizable_fields_from_block from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangoapps.content_libraries import api as libs from openedx.core.djangoapps.content_tagging import api as tagging_api from openedx.core.djangoapps.xblock import api as xblock from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase -from xmodule.modulestore.tests.factories import CourseFactory, BlockFactory +from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory @ddt.ddt @@ -113,7 +117,7 @@ def test_sync_bad_downstream(self): downstream_lib_block.save() with self.assertRaises(BadDownstream): - sync_from_upstream(downstream_lib_block, self.user) + sync_from_upstream_block(downstream_lib_block, self.user) assert downstream_lib_block.display_name == "Another lib block" assert downstream_lib_block.data == "another lib block" @@ -127,7 +131,7 @@ def test_sync_no_upstream(self): block.data = "Block content" with self.assertRaises(NoUpstream): - sync_from_upstream(block, self.user) + sync_from_upstream_block(block, self.user) assert block.display_name == "Block Title" assert block.data == "Block content" @@ -138,7 +142,6 @@ def test_sync_no_upstream(self): ("course-v1:Oops+ItsA+CourseKey", ".*is malformed.*"), ("block-v1:The+Wrong+KindOfUsageKey+type@html+block@nope", ".*is malformed.*"), ("lb:TestX:NoSuchLib:html:block-id", ".*not found in the system.*"), - ("lb:TestX:TestLib:video:should-be-html-but-is-a-video", ".*type mismatch.*"), ("lb:TestX:TestLib:html:no-such-html", ".*not found in the system.*"), ) @ddt.unpack @@ -151,12 +154,29 @@ def test_sync_bad_upstream(self, upstream, message_regex): block.data = "Block content" with self.assertRaisesRegex(BadUpstream, message_regex): - sync_from_upstream(block, self.user) + sync_from_upstream_block(block, self.user) assert block.display_name == "Block Title" assert block.data == "Block content" assert not block.upstream_display_name + def test_sync_incompatible_upstream(self): + """ + Syncing with a bad upstream raises BadUpstream, but doesn't affect the block + """ + downstream_block = BlockFactory.create( + category='html', parent=self.unit, upstream=str(self.upstream_problem_key), + ) + downstream_block.display_name = "Block Title" + downstream_block.data = "Block content" + + with self.assertRaisesRegex(BadUpstream, "Content type mismatch.*"): + sync_from_upstream_block(downstream_block, self.user) + + assert downstream_block.display_name == "Block Title" + assert downstream_block.data == "Block content" + assert not downstream_block.upstream_display_name + def test_sync_not_accessible(self): """ Syncing with an block that exists, but is inaccessible, raises BadUpstream @@ -164,7 +184,7 @@ def test_sync_not_accessible(self): downstream = BlockFactory.create(category='html', parent=self.unit, upstream=str(self.upstream_key)) user_who_cannot_read_upstream = UserFactory.create(username="rando", is_staff=False, is_superuser=False) with self.assertRaisesRegex(BadUpstream, ".*could not be loaded.*") as exc: - sync_from_upstream(downstream, user_who_cannot_read_upstream) + sync_from_upstream_block(downstream, user_who_cannot_read_upstream) def test_sync_updates_happy_path(self): """ @@ -173,7 +193,7 @@ def test_sync_updates_happy_path(self): downstream = BlockFactory.create(category='html', parent=self.unit, upstream=str(self.upstream_key)) # Initial sync - sync_from_upstream(downstream, self.user) + sync_from_upstream_block(downstream, self.user) assert downstream.upstream_version == 2 # Library blocks start at version 2 (v1 is the empty new block) assert downstream.upstream_display_name == "Upstream Title V2" assert downstream.display_name == "Upstream Title V2" @@ -194,7 +214,7 @@ def test_sync_updates_happy_path(self): tagging_api.tag_object(str(self.upstream_key), self.taxonomy_all_org, new_upstream_tags) # Assert that un-published updates are not yet pulled into downstream - sync_from_upstream(downstream, self.user) + sync_from_upstream_block(downstream, self.user) assert downstream.upstream_version == 2 # Library blocks start at version 2 (v1 is the empty new block) assert downstream.upstream_display_name == "Upstream Title V2" assert downstream.display_name == "Upstream Title V2" @@ -204,7 +224,7 @@ def test_sync_updates_happy_path(self): libs.publish_changes(self.library.key, self.user.id) # Follow-up sync. Assert that updates are pulled into downstream. - sync_from_upstream(downstream, self.user) + sync_from_upstream_block(downstream, self.user) assert downstream.upstream_version == 3 assert downstream.upstream_display_name == "Upstream Title V3" assert downstream.display_name == "Upstream Title V3" @@ -224,7 +244,7 @@ def test_sync_updates_to_downstream_only_fields(self): downstream = BlockFactory.create(category='problem', parent=self.unit, upstream=str(self.upstream_problem_key)) # Initial sync - sync_from_upstream(downstream, self.user) + sync_from_upstream_block(downstream, self.user) # These fields are copied from upstream assert downstream.upstream_display_name == "Upstream Problem Title V2" @@ -288,7 +308,7 @@ def test_sync_updates_to_downstream_only_fields(self): downstream.save() # Follow-up sync. - sync_from_upstream(downstream, self.user) + sync_from_upstream_block(downstream, self.user) # "unsafe" customizations are overridden by upstream assert downstream.upstream_display_name == "Upstream Problem Title V3" @@ -317,7 +337,7 @@ def test_sync_updates_to_modified_content(self): downstream = BlockFactory.create(category='html', parent=self.unit, upstream=str(self.upstream_key)) # Initial sync - sync_from_upstream(downstream, self.user) + sync_from_upstream_block(downstream, self.user) assert downstream.upstream_display_name == "Upstream Title V2" assert downstream.display_name == "Upstream Title V2" assert downstream.data == "Upstream content V2" @@ -335,7 +355,7 @@ def test_sync_updates_to_modified_content(self): downstream.save() # Follow-up sync. Assert that updates are pulled into downstream, but customizations are saved. - sync_from_upstream(downstream, self.user) + sync_from_upstream_block(downstream, self.user) assert downstream.upstream_display_name == "Upstream Title V3" assert downstream.display_name == "Downstream Title Override" # "safe" customization survives assert downstream.data == "Upstream content V3" # "unsafe" override is gone @@ -352,7 +372,7 @@ def test_sync_updates_to_modified_content(self): # """ # # Start with an uncustomized downstream block. # downstream = BlockFactory.create(category='html', parent=self.unit, upstream=str(self.upstream_key)) - # sync_from_upstream(downstream, self.user) + # sync_from_upstream_block(downstream, self.user) # assert downstream.downstream_customized == [] # assert downstream.display_name == downstream.upstream_display_name == "Upstream Title V2" # @@ -362,7 +382,7 @@ def test_sync_updates_to_modified_content(self): # assert downstream.downstream_customized == ["display_name"] # # # Syncing should retain the customization. - # sync_from_upstream(downstream, self.user) + # sync_from_upstream_block(downstream, self.user) # assert downstream.upstream_version == 2 # assert downstream.upstream_display_name == "Upstream Title V2" # assert downstream.display_name == "Title V3" @@ -373,7 +393,7 @@ def test_sync_updates_to_modified_content(self): # upstream.save() # # # ...which is reflected when we sync. - # sync_from_upstream(downstream, self.user) + # sync_from_upstream_block(downstream, self.user) # assert downstream.upstream_version == 3 # assert downstream.upstream_display_name == downstream.display_name == "Title V3" # @@ -384,7 +404,7 @@ def test_sync_updates_to_modified_content(self): # upstream.save() # # # ...then the downstream title should remain put. - # sync_from_upstream(downstream, self.user) + # sync_from_upstream_block(downstream, self.user) # assert downstream.upstream_version == 4 # assert downstream.upstream_display_name == "Title V4" # assert downstream.display_name == "Title V3" @@ -393,12 +413,12 @@ def test_sync_updates_to_modified_content(self): # downstream.downstream_customized = [] # upstream.display_name = "Title V5" # upstream.save() - # sync_from_upstream(downstream, self.user) + # sync_from_upstream_block(downstream, self.user) # assert downstream.upstream_version == 5 # assert downstream.upstream_display_name == downstream.display_name == "Title V5" @ddt.data(None, "Title From Some Other Upstream Version") - def test_fetch_customizable_fields(self, initial_upstream_display_name): + def test_update_customizable_fields(self, initial_upstream_display_name): """ Can we fetch a block's upstream field values without syncing it? @@ -409,13 +429,13 @@ def test_fetch_customizable_fields(self, initial_upstream_display_name): downstream.display_name = "Some Title" downstream.data = "Some content" - # Note that we're not linked to any upstream. fetch_customizable_fields shouldn't care. + # Note that we're not linked to any upstream. fetch_customizable_fields_from_block shouldn't care. assert not downstream.upstream assert not downstream.upstream_version # fetch! upstream = xblock.load_block(self.upstream_key, self.user) - fetch_customizable_fields(upstream=upstream, downstream=downstream, user=self.user) + fetch_customizable_fields_from_block(upstream=upstream, downstream=downstream, user=self.user) # Ensure: fetching doesn't affect the upstream link (or lack thereof). assert not downstream.upstream @@ -441,7 +461,7 @@ def test_prompt_and_decline_sync(self): assert link.ready_to_sync is True # Initial sync to V2 - sync_from_upstream(downstream, self.user) + sync_from_upstream_block(downstream, self.user) link = UpstreamLink.get_for_block(downstream) assert link.version_synced == 2 assert link.version_declined is None @@ -491,7 +511,7 @@ def test_sever_upstream_link(self): """ # Start with a course block that is linked+synced to a content library block. downstream = BlockFactory.create(category='html', parent=self.unit, upstream=str(self.upstream_key)) - sync_from_upstream(downstream, self.user) + sync_from_upstream_block(downstream, self.user) # (sanity checks) assert downstream.upstream == str(self.upstream_key) @@ -531,7 +551,7 @@ def test_sync_library_block_tags(self): downstream = BlockFactory.create(category='html', parent=self.unit, upstream=str(upstream_lib_block_key)) # Initial sync - sync_from_upstream(downstream, self.user) + sync_from_upstream_block(downstream, self.user) # Verify tags object_tags = tagging_api.get_object_tags(str(downstream.location)) @@ -547,7 +567,7 @@ def test_sync_library_block_tags(self): tagging_api.tag_object(str(upstream_lib_block_key), self.taxonomy_all_org, new_upstream_tags) # Follow-up sync. - sync_from_upstream(downstream, self.user) + sync_from_upstream_block(downstream, self.user) #Verify tags object_tags = tagging_api.get_object_tags(str(downstream.location)) @@ -560,7 +580,7 @@ def test_sync_video_block(self): downstream.edx_video_id = "test_video_id" # Sync - sync_from_upstream(downstream, self.user) + sync_from_upstream_block(downstream, self.user) assert downstream.upstream_version == 2 assert downstream.upstream_display_name == "Video Test" assert downstream.display_name == "Video Test" diff --git a/cms/lib/xblock/upstream_sync.py b/cms/lib/xblock/upstream_sync.py index 22cad3c6d36a..805019f5850d 100644 --- a/cms/lib/xblock/upstream_sync.py +++ b/cms/lib/xblock/upstream_sync.py @@ -1,13 +1,17 @@ """ -Synchronize content and settings from upstream blocks to their downstream usages. +Synchronize content and settings from upstream content to their downstream +usages. At the time of writing, we assume that for any upstream-downstream linkage: -* The upstream is a Component from a Learning Core-backed Content Library. -* The downstream is a block of matching type in a SplitModuleStore-backed Course. +* The upstream is a Component or Container from a Learning Core-backed Content + Library. +* The downstream is a block of compatible type in a SplitModuleStore-backed + Course. * They are both on the same Open edX instance. -HOWEVER, those assumptions may loosen in the future. So, we consider these to be INTERNAL ASSUMPIONS that should not be -exposed through this module's public Python interface. +HOWEVER, those assumptions may loosen in the future. So, we consider these to be +INTERNAL ASSUMPIONS that should not be exposed through this module's public +Python interface. """ from __future__ import annotations @@ -16,12 +20,10 @@ from dataclasses import dataclass, asdict from django.conf import settings -from django.core.exceptions import PermissionDenied from django.utils.translation import gettext_lazy as _ -from rest_framework.exceptions import NotFound from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey -from opaque_keys.edx.locator import LibraryUsageLocatorV2 +from opaque_keys.edx.locator import LibraryContainerLocator, LibraryUsageLocatorV2 from xblock.exceptions import XBlockNotFoundError from xblock.fields import Scope, String, Integer from xblock.core import XBlockMixin, XBlock @@ -74,6 +76,7 @@ class UpstreamLink: Metadata about some downstream content's relationship with its linked upstream content. """ upstream_ref: str | None # Reference to the upstream content, e.g., a serialized library block usage key. + upstream_key: LibraryUsageLocatorV2 | LibraryContainerLocator | None # parsed opaque key version of upstream_ref version_synced: int | None # Version of the upstream to which the downstream was last synced. version_available: int | None # Latest version of the upstream that's available, or None if it couldn't be loaded. version_declined: int | None # Latest version which the user has declined to sync with, if any. @@ -96,40 +99,46 @@ def upstream_link(self) -> str | None: """ Link to edit/view upstream block in library. """ - if self.version_available is None or self.upstream_ref is None: + if self.version_available is None or self.upstream_key is None: return None - try: - usage_key = LibraryUsageLocatorV2.from_string(self.upstream_ref) - except InvalidKeyError: - return None - return _get_library_xblock_url(usage_key) + if isinstance(self.upstream_key, LibraryUsageLocatorV2): + return _get_library_xblock_url(self.upstream_key) + if isinstance(self.upstream_key, LibraryContainerLocator): + return _get_library_container_url(self.upstream_key) + return None def to_json(self) -> dict[str, t.Any]: """ Get an JSON-API-friendly representation of this upstream link. """ - return { + data = { **asdict(self), "ready_to_sync": self.ready_to_sync, "upstream_link": self.upstream_link, } + del data["upstream_key"] # As JSON (string), this would be redundant with upstream_ref + return data @classmethod - def try_get_for_block(cls, downstream: XBlock) -> t.Self: + def try_get_for_block(cls, downstream: XBlock, log_error: bool = True) -> t.Self: """ Same as `get_for_block`, but upon failure, sets `.error_message` instead of raising an exception. """ try: return cls.get_for_block(downstream) except UpstreamLinkException as exc: - logger.exception( - "Tried to inspect an unsupported, broken, or missing downstream->upstream link: '%s'->'%s'", - downstream.usage_key, - downstream.upstream, - ) + # Note: if we expect that an upstream may not be set at all (i.e. we're just inspecting a random + # unit that may be a regular course unit), we don't want to log this, so set log_error=False then. + if log_error: + logger.exception( + "Tried to inspect an unsupported, broken, or missing downstream->upstream link: '%s'->'%s'", + downstream.usage_key, + downstream.upstream, + ) return cls( - upstream_ref=downstream.upstream, - version_synced=downstream.upstream_version, + upstream_ref=getattr(downstream, "upstream", ""), + upstream_key=None, + version_synced=getattr(downstream, "upstream_version", None), version_available=None, version_declined=None, error_message=str(exc), @@ -138,201 +147,83 @@ def try_get_for_block(cls, downstream: XBlock) -> t.Self: @classmethod def get_for_block(cls, downstream: XBlock) -> t.Self: """ - Get info on a block's relationship with its linked upstream content (without actually loading the content). + Get info on a downstream block's relationship with its linked upstream + content (without actually loading the content). - Currently, the only supported upstreams are LC-backed Library Components. This may change in the future (see - module docstring). + Currently, the only supported upstreams are LC-backed Library Components + (XBlocks) or Containers. This may change in the future (see module + docstring). If link exists, is supported, and is followable, returns UpstreamLink. Otherwise, raises an UpstreamLinkException. """ + # We import this here b/c UpstreamSyncMixin is used by cms/envs, which loads before the djangoapps are ready. + from openedx.core.djangoapps.content_libraries import api as lib_api + + if not isinstance(downstream, UpstreamSyncMixin): + raise BadDownstream(_("Downstream is not an XBlock or is missing required UpstreamSyncMixin")) if not downstream.upstream: raise NoUpstream() if not isinstance(downstream.usage_key.context_key, CourseKey): raise BadDownstream(_("Cannot update content because it does not belong to a course.")) - if downstream.has_children: - raise BadDownstream(_("Updating content with children is not yet supported.")) + downstream_type = downstream.usage_key.block_type + + upstream_key: LibraryUsageLocatorV2 | LibraryContainerLocator try: upstream_key = LibraryUsageLocatorV2.from_string(downstream.upstream) - except InvalidKeyError as exc: - raise BadUpstream(_("Reference to linked library item is malformed")) from exc - downstream_type = downstream.usage_key.block_type - if upstream_key.block_type != downstream_type: - # Note: Currently, we strictly enforce that the downstream and upstream block_types must exactly match. - # It could be reasonable to relax this requirement in the future if there's product need for it. - # For example, there's no reason that a StaticTabBlock couldn't take updates from an HtmlBlock. + except InvalidKeyError: + try: + upstream_key = LibraryContainerLocator.from_string(downstream.upstream) + except InvalidKeyError as exc: + raise BadUpstream(_("Reference to linked library item is malformed")) from exc + + if isinstance(upstream_key, LibraryUsageLocatorV2): + # The upstream is an XBlock + if downstream.has_children: + raise BadDownstream( + _("Updating content with children is not yet supported unless the upstream is a container."), + ) + expected_downstream_block_type = upstream_key.block_type + try: + block_meta = lib_api.get_library_block(upstream_key) + except XBlockNotFoundError as exc: + raise BadUpstream(_("Linked upstream library block was not found in the system")) from exc + version_available = block_meta.published_version_num + else: + # The upstream is a Container: + assert isinstance(upstream_key, LibraryContainerLocator) + try: + container_meta = lib_api.get_container(upstream_key) + except lib_api.ContentLibraryContainerNotFound as exc: + raise BadUpstream(_("Linked upstream library container was not found in the system")) from exc + expected_downstream_block_type = container_meta.container_type.olx_tag + version_available = container_meta.published_version_num + + if downstream_type != expected_downstream_block_type: + # Note: generally the upstream and downstream types must match, except that upstream containers + # may have e.g. container_type=unit while the downstream block has the equivalent block_type=vertical. + # It could be reasonable to relax this requirement in the future if there's product need for it. + # for example, there's no reason that a StaticTabBlock couldn't take updates from an HtmlBlock. raise BadUpstream( - _("Content type mismatch: {downstream_type} cannot be linked to {upstream_type}.").format( - downstream_type=downstream_type, upstream_type=upstream_key.block_type + _( + "Content type mismatch: {downstream_id} ({downstream_type}) cannot be linked to {upstream_id}." + ).format( + downstream_id=downstream.usage_key, + downstream_type=downstream_type, + upstream_id=str(upstream_key), ) - ) from TypeError( - f"downstream block '{downstream.usage_key}' is linked to " - f"upstream block of different type '{upstream_key}'" ) - # We import this here b/c UpstreamSyncMixin is used by cms/envs, which loads before the djangoapps are ready. - from openedx.core.djangoapps.content_libraries.api import ( - get_library_block # pylint: disable=wrong-import-order - ) - try: - lib_meta = get_library_block(upstream_key) - except XBlockNotFoundError as exc: - raise BadUpstream(_("Linked library item was not found in the system")) from exc + return cls( upstream_ref=downstream.upstream, + upstream_key=upstream_key, version_synced=downstream.upstream_version, - version_available=(lib_meta.published_version_num if lib_meta else None), + version_available=version_available, version_declined=downstream.upstream_version_declined, error_message=None, ) -def sync_from_upstream(downstream: XBlock, user: User) -> XBlock: - """ - Update `downstream` with content+settings from the latest available version of its linked upstream content. - - Preserves overrides to customizable fields; overwrites overrides to other fields. - Does not save `downstream` to the store. That is left up to the caller. - - If `downstream` lacks a valid+supported upstream link, this raises an UpstreamLinkException. - """ - link, upstream = _load_upstream_link_and_block(downstream, user) - _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=False) - _update_non_customizable_fields(upstream=upstream, downstream=downstream) - _update_tags(upstream=upstream, downstream=downstream) - downstream.upstream_version = link.version_available - return upstream - - -def fetch_customizable_fields(*, downstream: XBlock, user: User, upstream: XBlock | None = None) -> None: - """ - Fetch upstream-defined value of customizable fields and save them on the downstream. - - If `upstream` is provided, use that block as the upstream. - Otherwise, load the block specified by `downstream.upstream`, which may raise an UpstreamLinkException. - """ - if not upstream: - _link, upstream = _load_upstream_link_and_block(downstream, user) - _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=True) - - -def _load_upstream_link_and_block(downstream: XBlock, user: User) -> tuple[UpstreamLink, XBlock]: - """ - Load the upstream metadata and content for a downstream block. - - Assumes that the upstream content is an XBlock in an LC-backed content libraries. This assumption may need to be - relaxed in the future (see module docstring). - - If `downstream` lacks a valid+supported upstream link, this raises an UpstreamLinkException. - """ - link = UpstreamLink.get_for_block(downstream) # can raise UpstreamLinkException - # We import load_block here b/c UpstreamSyncMixin is used by cms/envs, which loads before the djangoapps are ready. - from openedx.core.djangoapps.xblock.api import load_block, CheckPerm, LatestVersion # pylint: disable=wrong-import-order - try: - lib_block: XBlock = load_block( - LibraryUsageLocatorV2.from_string(downstream.upstream), - user, - check_permission=CheckPerm.CAN_READ_AS_AUTHOR, - version=LatestVersion.PUBLISHED, - ) - except (NotFound, PermissionDenied) as exc: - raise BadUpstream(_("Linked library item could not be loaded: {}").format(downstream.upstream)) from exc - return link, lib_block - - -def _update_customizable_fields(*, upstream: XBlock, downstream: XBlock, only_fetch: bool) -> None: - """ - For each customizable field: - * Save the upstream value to a hidden field on the downstream ("FETCH"). - * If `not only_fetch`, and if the field *isn't* customized on the downstream, then: - * Update it the downstream field's value from the upstream field ("SYNC"). - - Concrete example: Imagine `lib_problem` is our upstream and `course_problem` is our downstream. - - * Say that the customizable fields are [display_name, max_attempts]. - - * Set `course_problem.upstream_display_name = lib_problem.display_name` ("fetch"). - * If `not only_fetch`, and `course_problem.display_name` wasn't customized, then: - * Set `course_problem.display_name = lib_problem.display_name` ("sync"). - """ - syncable_field_names = _get_synchronizable_fields(upstream, downstream) - - for field_name, fetch_field_name in downstream.get_customizable_fields().items(): - - if field_name not in syncable_field_names: - continue - - # Downstream-only fields don't have an upstream fetch field - if fetch_field_name is None: - continue - - # FETCH the upstream's value and save it on the downstream (ie, `downstream.upstream_$FIELD`). - old_upstream_value = getattr(downstream, fetch_field_name) - new_upstream_value = getattr(upstream, field_name) - setattr(downstream, fetch_field_name, new_upstream_value) - - if only_fetch: - continue - - # Okay, now for the nuanced part... - # We need to update the downstream field *iff it has not been customized**. - # Determining whether a field has been customized will differ in Beta vs Future release. - # (See "PRESERVING DOWNSTREAM CUSTOMIZATIONS" comment below for details.) - - ## FUTURE BEHAVIOR: field is "customized" iff we have noticed that the user edited it. - # if field_name in downstream.downstream_customized: - # continue - - ## BETA BEHAVIOR: field is "customized" iff we have the prev upstream value, but field doesn't match it. - downstream_value = getattr(downstream, field_name) - if old_upstream_value and downstream_value != old_upstream_value: - continue # Field has been customized. Don't touch it. Move on. - - # Field isn't customized -- SYNC it! - setattr(downstream, field_name, new_upstream_value) - - -def _update_non_customizable_fields(*, upstream: XBlock, downstream: XBlock) -> None: - """ - For each field `downstream.blah` that isn't customizable: set it to `upstream.blah`. - """ - syncable_fields = _get_synchronizable_fields(upstream, downstream) - customizable_fields = set(downstream.get_customizable_fields().keys()) - isVideoBlock = downstream.usage_key.block_type == "video" - for field_name in syncable_fields - customizable_fields: - if isVideoBlock and field_name == 'edx_video_id': - # Avoid overwriting edx_video_id between blocks - continue - new_upstream_value = getattr(upstream, field_name) - setattr(downstream, field_name, new_upstream_value) - - -def _update_tags(*, upstream: XBlock, downstream: XBlock) -> None: - """ - Update tags from `upstream` to `downstream` - """ - from openedx.core.djangoapps.content_tagging.api import copy_tags_as_read_only - # For any block synced with an upstream, copy the tags as read_only - # This keeps tags added locally. - copy_tags_as_read_only( - str(upstream.location), - str(downstream.location), - ) - - -def _get_synchronizable_fields(upstream: XBlock, downstream: XBlock) -> set[str]: - """ - The syncable fields are the ones which are content- or settings-scoped AND are defined on both (up,down)stream. - """ - return set.intersection(*[ - set( - field_name - for (field_name, field) in block.__class__.fields.items() - if field.scope in [Scope.settings, Scope.content] - ) - for block in [upstream, downstream] - ]) - - def decline_sync(downstream: XBlock) -> None: """ Given an XBlock that is linked to upstream content, mark the latest available update as 'declined' so that its @@ -383,6 +274,18 @@ def _get_library_xblock_url(usage_key: LibraryUsageLocatorV2): return library_url +def _get_library_container_url(container_key: LibraryContainerLocator): + """ + Gets authoring url for given container_key. + """ + library_url = None + if mfe_base_url := settings.COURSE_AUTHORING_MICROFRONTEND_URL: # type: ignore + library_key = container_key.lib_key + if container_key.container_type == "unit": + library_url = f'{mfe_base_url}/library/{library_key}/units/{container_key}' + return library_url + + class UpstreamSyncMixin(XBlockMixin): """ Allows an XBlock in the CMS to be associated & synced with an upstream. @@ -393,10 +296,11 @@ class UpstreamSyncMixin(XBlockMixin): # Upstream synchronization metadata fields upstream = String( help=( - "The usage key of a block (generally within a content library) which serves as a source of upstream " - "updates for this block, or None if there is no such upstream. Please note: It is valid for this " - "field to hold a usage key for an upstream block that does not exist (or does not *yet* exist) on " - "this instance, particularly if this downstream block was imported from a different instance." + "The usage key or container key of the source block/container (generally within a content library) " + "which serves as a source of upstream updates for this block, or None if there is no such upstream. " + "Please note: It is valid for this field to hold a key for an upstream block/container that does not " + "exist (or does not *yet* exist) on this instance, particularly if this downstream block was imported " + "from a different instance." ), default=None, scope=Scope.settings, hidden=True, enforce_type=True ) @@ -419,7 +323,7 @@ class UpstreamSyncMixin(XBlockMixin): # Store the fetched upstream values for customizable fields. upstream_display_name = String( - help=("The value of display_name on the linked upstream block."), + help=("The value of display_name on the linked upstream block/container."), default=None, scope=Scope.settings, hidden=True, enforce_type=True, ) diff --git a/cms/lib/xblock/upstream_sync_block.py b/cms/lib/xblock/upstream_sync_block.py new file mode 100644 index 000000000000..12c0095fe3f1 --- /dev/null +++ b/cms/lib/xblock/upstream_sync_block.py @@ -0,0 +1,176 @@ +""" +Methods related to syncing a downstream XBlock with an upstream XBlock. + +See upstream_sync.py for general upstream sync code that applies even when the +upstream is a container, not an XBlock. +""" +from __future__ import annotations + +import typing as t + +from django.core.exceptions import PermissionDenied +from django.utils.translation import gettext_lazy as _ +from rest_framework.exceptions import NotFound +from opaque_keys.edx.locator import LibraryUsageLocatorV2 +from xblock.fields import Scope +from xblock.core import XBlock + +from .upstream_sync import UpstreamLink, BadUpstream + +if t.TYPE_CHECKING: + from django.contrib.auth.models import User # pylint: disable=imported-auth-user + + +def sync_from_upstream_block(downstream: XBlock, user: User) -> XBlock: + """ + Update `downstream` with content+settings from the latest available version of its linked upstream content. + + Preserves overrides to customizable fields; overwrites overrides to other fields. + Does not save `downstream` to the store. That is left up to the caller. + + ⭐️ Does not save changes to modulestore nor handle static assets. The caller + will have to take care of that. + + If `downstream` lacks a valid+supported upstream link, this raises an UpstreamLinkException. + """ + link = UpstreamLink.get_for_block(downstream) # can raise UpstreamLinkException + if not isinstance(link.upstream_key, LibraryUsageLocatorV2): + raise TypeError("sync_from_upstream_block() only supports XBlock upstreams, not containers") + # Upstream is a library block: + upstream = _load_upstream_block(downstream, user) + _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=False) + _update_non_customizable_fields(upstream=upstream, downstream=downstream) + _update_tags(upstream=upstream, downstream=downstream) + downstream.upstream_version = link.version_available + return upstream + + +def fetch_customizable_fields_from_block(*, downstream: XBlock, user: User, upstream: XBlock | None = None) -> None: + """ + Fetch upstream-defined value of customizable fields and save them on the downstream. + + If `upstream` is provided, use that block as the upstream. + Otherwise, load the block specified by `downstream.upstream`, which may raise an UpstreamLinkException. + """ + if not upstream: + upstream = _load_upstream_block(downstream, user) + _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=True) + + +def _load_upstream_block(downstream: XBlock, user: User) -> XBlock: + """ + Load the upstream metadata and content for a downstream block. + + Assumes that the upstream content is an XBlock in an LC-backed content libraries. This assumption may need to be + relaxed in the future (see module docstring). + + If `downstream` lacks a valid+supported upstream link, this raises an UpstreamLinkException. + """ + # We import load_block here b/c UpstreamSyncMixin is used by cms/envs, which loads before the djangoapps are ready. + from openedx.core.djangoapps.xblock.api import load_block, CheckPerm, LatestVersion # pylint: disable=wrong-import-order + try: + lib_block: XBlock = load_block( + LibraryUsageLocatorV2.from_string(downstream.upstream), + user, + check_permission=CheckPerm.CAN_READ_AS_AUTHOR, + version=LatestVersion.PUBLISHED, + ) + except (NotFound, PermissionDenied) as exc: + raise BadUpstream(_("Linked library item could not be loaded: {}").format(downstream.upstream)) from exc + return lib_block + + +def _update_customizable_fields(*, upstream: XBlock, downstream: XBlock, only_fetch: bool) -> None: + """ + For each customizable field: + * Save the upstream value to a hidden field on the downstream ("FETCH"). + * If `not only_fetch`, and if the field *isn't* customized on the downstream, then: + * Update it the downstream field's value from the upstream field ("SYNC"). + + Concrete example: Imagine `lib_problem` is our upstream and `course_problem` is our downstream. + + * Say that the customizable fields are [display_name, max_attempts]. + + * Set `course_problem.upstream_display_name = lib_problem.display_name` ("fetch"). + * If `not only_fetch`, and `course_problem.display_name` wasn't customized, then: + * Set `course_problem.display_name = lib_problem.display_name` ("sync"). + """ + syncable_field_names = _get_synchronizable_fields(upstream, downstream) + + for field_name, fetch_field_name in downstream.get_customizable_fields().items(): + + if field_name not in syncable_field_names: + continue + + # Downstream-only fields don't have an upstream fetch field + if fetch_field_name is None: + continue + + # FETCH the upstream's value and save it on the downstream (ie, `downstream.upstream_$FIELD`). + old_upstream_value = getattr(downstream, fetch_field_name) + new_upstream_value = getattr(upstream, field_name) + setattr(downstream, fetch_field_name, new_upstream_value) + + if only_fetch: + continue + + # Okay, now for the nuanced part... + # We need to update the downstream field *iff it has not been customized**. + # Determining whether a field has been customized will differ in Beta vs Future release. + # (See "PRESERVING DOWNSTREAM CUSTOMIZATIONS" comment below for details.) + + ## FUTURE BEHAVIOR: field is "customized" iff we have noticed that the user edited it. + # if field_name in downstream.downstream_customized: + # continue + + ## BETA BEHAVIOR: field is "customized" iff we have the prev upstream value, but field doesn't match it. + downstream_value = getattr(downstream, field_name) + if old_upstream_value and downstream_value != old_upstream_value: + continue # Field has been customized. Don't touch it. Move on. + + # Field isn't customized -- SYNC it! + setattr(downstream, field_name, new_upstream_value) + + +def _update_non_customizable_fields(*, upstream: XBlock, downstream: XBlock) -> None: + """ + For each field `downstream.blah` that isn't customizable: set it to `upstream.blah`. + """ + syncable_fields = _get_synchronizable_fields(upstream, downstream) + customizable_fields = set(downstream.get_customizable_fields().keys()) + # TODO: resolve this so there's no special-case happening for video block. + # e.g. by some non_cloneable_fields property of the XBlock class? + is_video_block = downstream.usage_key.block_type == "video" + for field_name in syncable_fields - customizable_fields: + if is_video_block and field_name == 'edx_video_id': + # Avoid overwriting edx_video_id between blocks + continue + new_upstream_value = getattr(upstream, field_name) + setattr(downstream, field_name, new_upstream_value) + + +def _update_tags(*, upstream: XBlock, downstream: XBlock) -> None: + """ + Update tags from `upstream` to `downstream` + """ + from openedx.core.djangoapps.content_tagging.api import copy_tags_as_read_only + # For any block synced with an upstream, copy the tags as read_only + # This keeps tags added locally. + copy_tags_as_read_only( + str(upstream.location), + str(downstream.location), + ) + + +def _get_synchronizable_fields(upstream: XBlock, downstream: XBlock) -> set[str]: + """ + The syncable fields are the ones which are content- or settings-scoped AND are defined on both (up,down)stream. + """ + return set.intersection(*[ + set( + field_name + for (field_name, field) in block.__class__.fields.items() + if field.scope in [Scope.settings, Scope.content] + ) + for block in [upstream, downstream] + ]) diff --git a/cms/lib/xblock/upstream_sync_container.py b/cms/lib/xblock/upstream_sync_container.py new file mode 100644 index 000000000000..44e3b429ec05 --- /dev/null +++ b/cms/lib/xblock/upstream_sync_container.py @@ -0,0 +1,139 @@ +""" +Methods related to syncing a downstream XBlock with an upstream Container. + +See upstream_sync.py for general upstream sync code that applies even when the +upstream is a container, not an XBlock. +""" +from __future__ import annotations + +import typing as t + +from django.utils.translation import gettext_lazy as _ +from opaque_keys.edx.locator import LibraryContainerLocator +from xblock.core import XBlock + +from openedx.core.djangoapps.content_libraries import api as lib_api +from .upstream_sync import UpstreamLink + +if t.TYPE_CHECKING: + from django.contrib.auth.models import User # pylint: disable=imported-auth-user + + +def sync_from_upstream_container( + downstream: XBlock, + user: User, +) -> list[lib_api.LibraryXBlockMetadata | lib_api.ContainerMetadata]: + """ + Update `downstream` with content+settings from the latest available version of its linked upstream content. + + Preserves overrides to customizable fields; overwrites overrides to other fields. + Does not save `downstream` to the store. That is left up to the caller. + + If `downstream` lacks a valid+supported upstream link, this raises an UpstreamLinkException. + + ⭐️ Does not directly sync static assets (containers don't have them) nor + children. Returns a list of the upstream children so the caller can do that. + + Should children be handled in here? Maybe if sync_from_upstream_block + were updated to handle static assets and also save changes to modulestore. + """ + link = UpstreamLink.get_for_block(downstream) # can raise UpstreamLinkException + if not isinstance(link.upstream_key, LibraryContainerLocator): + raise TypeError("sync_from_upstream_container() only supports Container upstreams, not containers") + lib_api.require_permission_for_library_key( # TODO: should permissions be checked at this low level? + link.upstream_key.lib_key, + user, + permission=lib_api.permissions.CAN_VIEW_THIS_CONTENT_LIBRARY, + ) + upstream_meta = lib_api.get_container(link.upstream_key, user) + upstream_children = lib_api.get_container_children(link.upstream_key, published=True) + _update_customizable_fields(upstream=upstream_meta, downstream=downstream, only_fetch=False) + _update_non_customizable_fields(upstream=upstream_meta, downstream=downstream) + _update_tags(upstream=upstream_meta, downstream=downstream) + downstream.upstream_version = link.version_available + return upstream_children + + +def fetch_customizable_fields_from_container(*, downstream: XBlock, user: User) -> None: + """ + Fetch upstream-defined value of customizable fields and save them on the downstream. + + The container version only retrieves values from *published* containers. + + Basically, this sets the value of "upstream_display_name" on the downstream block. + """ + upstream = lib_api.get_container(LibraryContainerLocator.from_string(downstream.upstream), user) + _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=True) + + +def _update_customizable_fields(*, upstream: lib_api.ContainerMetadata, downstream: XBlock, only_fetch: bool) -> None: + """ + For each customizable field: + * Save the upstream value to a hidden field on the downstream ("FETCH"). + * If `not only_fetch`, and if the field *isn't* customized on the downstream, then: + * Update it the downstream field's value from the upstream field ("SYNC"). + + Concrete example: Imagine `lib_problem` is our upstream and `course_problem` is our downstream. + + * Say that the customizable fields are [display_name, max_attempts]. + + * Set `course_problem.upstream_display_name = lib_problem.display_name` ("fetch"). + * If `not only_fetch`, and `course_problem.display_name` wasn't customized, then: + * Set `course_problem.display_name = lib_problem.display_name` ("sync"). + """ + # For now, the only supported container "field" is display_name + syncable_field_names = ["display_name"] + + for field_name, fetch_field_name in downstream.get_customizable_fields().items(): + + if field_name not in syncable_field_names: + continue + + # Downstream-only fields don't have an upstream fetch field + if fetch_field_name is None: + continue + + # FETCH the upstream's value and save it on the downstream (ie, `downstream.upstream_$FIELD`). + old_upstream_value = getattr(downstream, fetch_field_name) + new_upstream_value = getattr(upstream, f"published_{field_name}") + setattr(downstream, fetch_field_name, new_upstream_value) + + if only_fetch: + continue + + # Okay, now for the nuanced part... + # We need to update the downstream field *iff it has not been customized**. + # Determining whether a field has been customized will differ in Beta vs Future release. + # (See "PRESERVING DOWNSTREAM CUSTOMIZATIONS" comment below for details.) + + ## FUTURE BEHAVIOR: field is "customized" iff we have noticed that the user edited it. + # if field_name in downstream.downstream_customized: + # continue + + ## BETA BEHAVIOR: field is "customized" iff we have the prev upstream value, but field doesn't match it. + downstream_value = getattr(downstream, field_name) + if old_upstream_value and downstream_value != old_upstream_value: + continue # Field has been customized. Don't touch it. Move on. + + # Field isn't customized -- SYNC it! + setattr(downstream, field_name, new_upstream_value) + + +def _update_non_customizable_fields(*, upstream: lib_api.ContainerMetadata, downstream: XBlock) -> None: + """ + For each field `downstream.blah` that isn't customizable: set it to `upstream.blah`. + """ + # For now, there's nothing to do here - containers don't have any non-customizable fields. + + +def _update_tags(*, upstream: lib_api.ContainerMetadata, downstream: XBlock) -> None: + """ + Update tags from `upstream` to `downstream` + """ + from openedx.core.djangoapps.content_tagging.api import copy_tags_as_read_only + # For any block synced with an upstream, copy the tags as read_only + # This keeps tags added locally. + copy_tags_as_read_only( + str(upstream.container_key), + str(downstream.usage_key), + ) diff --git a/cms/templates/studio_xblock_wrapper.html b/cms/templates/studio_xblock_wrapper.html index 8f4090588613..2279f60e116e 100644 --- a/cms/templates/studio_xblock_wrapper.html +++ b/cms/templates/studio_xblock_wrapper.html @@ -25,7 +25,7 @@ has_not_configured_message = messages.get('summary',{}).get('type', None) == 'not-configured' block_is_unit = is_unit(xblock) -upstream_info = UpstreamLink.try_get_for_block(xblock) +upstream_info = UpstreamLink.try_get_for_block(xblock, log_error=False) %> <%namespace name='static' file='static_content.html'/> @@ -211,12 +211,13 @@ % endif - % elif not show_inline: -
  • - - ${_("Details")} - -
    • + % if not show_inline: +
  • + + ${_("Details")} + +
    • + % endif % endif % endif diff --git a/mypy.ini b/mypy.ini index 83255c1a5af1..f0267364ffb0 100644 --- a/mypy.ini +++ b/mypy.ini @@ -7,6 +7,7 @@ plugins = mypy_drf_plugin.main files = cms/lib/xblock/upstream_sync.py, + cms/lib/xblock/upstream_sync_container.py, cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py, cms/djangoapps/import_from_modulestore, openedx/core/djangoapps/content/learning_sequences, diff --git a/openedx/core/djangoapps/content_libraries/api/__init__.py b/openedx/core/djangoapps/content_libraries/api/__init__.py index 4e0b4fcce48e..6c5cbce2a2fa 100644 --- a/openedx/core/djangoapps/content_libraries/api/__init__.py +++ b/openedx/core/djangoapps/content_libraries/api/__init__.py @@ -1,6 +1,7 @@ """ Python API for working with content libraries """ +from .block_metadata import * from .collections import * from .containers import * from .courseware_import import * diff --git a/openedx/core/djangoapps/content_libraries/api/block_metadata.py b/openedx/core/djangoapps/content_libraries/api/block_metadata.py new file mode 100644 index 000000000000..032d21431a3a --- /dev/null +++ b/openedx/core/djangoapps/content_libraries/api/block_metadata.py @@ -0,0 +1,85 @@ +""" +Content libraries API methods related to XBlocks/Components. + +These methods don't enforce permissions (only the REST APIs do). +""" +from __future__ import annotations +from dataclasses import dataclass + +from django.utils.translation import gettext as _ +from opaque_keys.edx.locator import LibraryUsageLocatorV2 +from .libraries import ( + library_component_usage_key, + PublishableItem, +) + +# The public API is only the following symbols: +__all__ = [ + "LibraryXBlockMetadata", + "LibraryXBlockStaticFile", +] + + +@dataclass(frozen=True, kw_only=True) +class LibraryXBlockMetadata(PublishableItem): + """ + Class that represents the metadata about an XBlock in a content library. + """ + usage_key: LibraryUsageLocatorV2 + # TODO: move tags_count to LibraryItem as all objects under a library can be tagged. + tags_count: int = 0 + + @classmethod + def from_component(cls, library_key, component, associated_collections=None): + """ + Construct a LibraryXBlockMetadata from a Component object. + """ + # Import content_tagging.api here to avoid circular imports + from openedx.core.djangoapps.content_tagging.api import get_object_tag_counts + last_publish_log = component.versioning.last_publish_log + + published_by = None + if last_publish_log and last_publish_log.published_by: + published_by = last_publish_log.published_by.username + + draft = component.versioning.draft + published = component.versioning.published + last_draft_created = draft.created if draft else None + last_draft_created_by = draft.publishable_entity_version.created_by if draft else None + usage_key = library_component_usage_key(library_key, component) + tags = get_object_tag_counts(str(usage_key), count_implicit=True) + + return cls( + usage_key=library_component_usage_key( + library_key, + component, + ), + display_name=draft.title, + created=component.created, + modified=draft.created, + draft_version_num=draft.version_num, + published_version_num=published.version_num if published else None, + last_published=None if last_publish_log is None else last_publish_log.published_at, + published_by=published_by, + last_draft_created=last_draft_created, + last_draft_created_by=last_draft_created_by, + has_unpublished_changes=component.versioning.has_unpublished_changes, + collections=associated_collections or [], + tags_count=tags.get(str(usage_key), 0), + can_stand_alone=component.publishable_entity.can_stand_alone, + ) + + +@dataclass(frozen=True) +class LibraryXBlockStaticFile: + """ + Class that represents a static file in a content library, associated with + a particular XBlock. + """ + # File path e.g. "diagram.png" + # In some rare cases it might contain a folder part, e.g. "en/track1.srt" + path: str + # Publicly accessible URL where the file can be downloaded + url: str + # Size in bytes + size: int diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index 1bfb054f4239..d440055448f2 100644 --- a/openedx/core/djangoapps/content_libraries/api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/api/blocks.py @@ -6,7 +6,6 @@ from __future__ import annotations import logging import mimetypes -from dataclasses import dataclass from datetime import datetime, timezone from typing import TYPE_CHECKING from uuid import uuid4 @@ -55,6 +54,7 @@ InvalidNameError, LibraryBlockAlreadyExists, ) +from .block_metadata import LibraryXBlockMetadata, LibraryXBlockStaticFile from .containers import ( create_container, get_container, @@ -65,7 +65,6 @@ ) from .libraries import ( library_collection_locator, - library_component_usage_key, PublishableItem, ) @@ -79,9 +78,6 @@ # The public API is only the following symbols: __all__ = [ - # Models - "LibraryXBlockMetadata", - "LibraryXBlockStaticFile", # API methods "get_library_components", "get_library_block", @@ -100,63 +96,6 @@ ] -@dataclass(frozen=True, kw_only=True) -class LibraryXBlockMetadata(PublishableItem): - """ - Class that represents the metadata about an XBlock in a content library. - """ - usage_key: LibraryUsageLocatorV2 - - @classmethod - def from_component(cls, library_key, component, associated_collections=None): - """ - Construct a LibraryXBlockMetadata from a Component object. - """ - last_publish_log = component.versioning.last_publish_log - - published_by = None - if last_publish_log and last_publish_log.published_by: - published_by = last_publish_log.published_by.username - - draft = component.versioning.draft - published = component.versioning.published - last_draft_created = draft.created if draft else None - last_draft_created_by = draft.publishable_entity_version.created_by if draft else None - - return cls( - usage_key=library_component_usage_key( - library_key, - component, - ), - display_name=draft.title, - created=component.created, - modified=draft.created, - draft_version_num=draft.version_num, - published_version_num=published.version_num if published else None, - last_published=None if last_publish_log is None else last_publish_log.published_at, - published_by=published_by, - last_draft_created=last_draft_created, - last_draft_created_by=last_draft_created_by, - has_unpublished_changes=component.versioning.has_unpublished_changes, - collections=associated_collections or [], - ) - - -@dataclass(frozen=True) -class LibraryXBlockStaticFile: - """ - Class that represents a static file in a content library, associated with - a particular XBlock. - """ - # File path e.g. "diagram.png" - # In some rare cases it might contain a folder part, e.g. "en/track1.srt" - path: str - # Publicly accessible URL where the file can be downloaded - url: str - # Size in bytes - size: int - - def get_library_components( library_key: LibraryLocatorV2, text_search: str | None = None, diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index b04a43a9709c..d7ba0fcac01f 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -34,7 +34,8 @@ from ..models import ContentLibrary from .exceptions import ContentLibraryContainerNotFound -from .libraries import LibraryXBlockMetadata, PublishableItem, library_component_usage_key +from .libraries import PublishableItem, library_component_usage_key +from .block_metadata import LibraryXBlockMetadata # The public API is only the following symbols: __all__ = [ @@ -342,7 +343,7 @@ def restore_container(container_key: LibraryContainerLocator) -> None: LIBRARY_CONTAINER_CREATED.send_event( library_container=LibraryContainerData( - container_key=str(container_key), + container_key=container_key, ) ) @@ -372,7 +373,7 @@ def restore_container(container_key: LibraryContainerLocator) -> None: def get_container_children( container_key: LibraryContainerLocator, published=False, -) -> list[authoring_api.ContainerEntityListEntry]: +) -> list[LibraryXBlockMetadata | ContainerMetadata]: """ Get the entities contained in the given container (e.g. the components/xblocks in a unit) """ diff --git a/openedx/core/djangoapps/content_libraries/api/libraries.py b/openedx/core/djangoapps/content_libraries/api/libraries.py index d214dcc3fd86..8e238f278096 100644 --- a/openedx/core/djangoapps/content_libraries/api/libraries.py +++ b/openedx/core/djangoapps/content_libraries/api/libraries.py @@ -205,54 +205,6 @@ class PublishableItem(LibraryItem): can_stand_alone: bool = True -@dataclass(frozen=True, kw_only=True) -class LibraryXBlockMetadata(PublishableItem): - """ - Class that represents the metadata about an XBlock in a content library. - """ - usage_key: LibraryUsageLocatorV2 - # TODO: move tags_count to LibraryItem as all objects under a library can be tagged. - tags_count: int = 0 - - @classmethod - def from_component(cls, library_key, component, associated_collections=None): - """ - Construct a LibraryXBlockMetadata from a Component object. - """ - # Import content_tagging.api here to avoid circular imports - from openedx.core.djangoapps.content_tagging.api import get_object_tag_counts - - last_publish_log = component.versioning.last_publish_log - - published_by = None - if last_publish_log and last_publish_log.published_by: - published_by = last_publish_log.published_by.username - - draft = component.versioning.draft - published = component.versioning.published - last_draft_created = draft.created if draft else None - last_draft_created_by = draft.publishable_entity_version.created_by if draft else None - usage_key = library_component_usage_key(library_key, component) - tags = get_object_tag_counts(str(usage_key), count_implicit=True) - - return cls( - usage_key=usage_key, - display_name=draft.title, - created=component.created, - modified=draft.created, - draft_version_num=draft.version_num, - published_version_num=published.version_num if published else None, - last_published=None if last_publish_log is None else last_publish_log.published_at, - published_by=published_by, - last_draft_created=last_draft_created, - last_draft_created_by=last_draft_created_by, - has_unpublished_changes=component.versioning.has_unpublished_changes, - collections=associated_collections or [], - can_stand_alone=component.publishable_entity.can_stand_alone, - tags_count=tags.get(str(usage_key), 0), - ) - - @dataclass(frozen=True) class LibraryXBlockStaticFile: """ diff --git a/openedx/core/djangoapps/content_libraries/tests/__init__.py b/openedx/core/djangoapps/content_libraries/tests/__init__.py index e69de29bb2d1..18083ab3e0ac 100644 --- a/openedx/core/djangoapps/content_libraries/tests/__init__.py +++ b/openedx/core/djangoapps/content_libraries/tests/__init__.py @@ -0,0 +1,4 @@ +""" +Python API for testing content libraries +""" +from .base import * diff --git a/openedx/core/djangoapps/content_libraries/tests/base.py b/openedx/core/djangoapps/content_libraries/tests/base.py index b81382ab001e..6068d9c20e72 100644 --- a/openedx/core/djangoapps/content_libraries/tests/base.py +++ b/openedx/core/djangoapps/content_libraries/tests/base.py @@ -2,6 +2,7 @@ Tests for Learning-Core-based Content Libraries """ from contextlib import contextmanager +import json from io import BytesIO from urllib.parse import urlencode @@ -9,6 +10,7 @@ from rest_framework.test import APITransactionTestCase, APIClient from common.djangoapps.student.tests.factories import UserFactory +from common.djangoapps.util.json_request import JsonResponse as SpecialJsonResponse from openedx.core.djangoapps.content_libraries.constants import ALL_RIGHTS_RESERVED from openedx.core.djangolib.testing.utils import skip_unless_cms @@ -113,6 +115,8 @@ def _api(self, method, url, data, expect_response): response = getattr(self.client, method)(url, data, format="json") assert response.status_code == expect_response,\ 'Unexpected response code {}:\n{}'.format(response.status_code, getattr(response, 'data', '(no data)')) + if isinstance(response, SpecialJsonResponse): # Required for some old APIs in the CMS that aren't using DRF + return json.loads(response.content) return response.data @contextmanager diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index 9b97ddfdfa8b..db6456a14a20 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -380,7 +380,7 @@ def test_restore_unit(self): "signal": LIBRARY_CONTAINER_CREATED, "sender": None, "library_container": LibraryContainerData( - container_key="lct:CL-TEST:containers:unit:u1", + container_key=LibraryContainerLocator.from_string("lct:CL-TEST:containers:unit:u1"), ), }, create_receiver.call_args_list[0].kwargs, diff --git a/openedx/core/djangoapps/content_tagging/tests/test_objecttag_export_helpers.py b/openedx/core/djangoapps/content_tagging/tests/test_objecttag_export_helpers.py index f196549dd1a9..eaeea3bda225 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_objecttag_export_helpers.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_objecttag_export_helpers.py @@ -442,7 +442,7 @@ def test_build_library_object_tree(self) -> None: """ Test if we can export a library """ - with self.assertNumQueries(8): + with self.assertNumQueries(11): tagged_library = build_object_tree_with_objecttags(self.library.key, self.all_library_object_tags) assert tagged_library == self.expected_library_tagged_xblock diff --git a/openedx/core/lib/xblock_serializer/block_serializer.py b/openedx/core/lib/xblock_serializer/block_serializer.py index 53be26937d4a..b8eabfcb4f11 100644 --- a/openedx/core/lib/xblock_serializer/block_serializer.py +++ b/openedx/core/lib/xblock_serializer/block_serializer.py @@ -152,6 +152,9 @@ def _serialize_html_block(self, block) -> etree.Element: olx_node.attrib["editor"] = block.editor if block.use_latex_compiler: olx_node.attrib["use_latex_compiler"] = "true" + for field_name in block.fields: + if field_name.startswith("upstream") and block.fields[field_name].is_set_on(block): + olx_node.attrib[field_name] = str(getattr(block, field_name)) # Escape any CDATA special chars escaped_block_data = block.data.replace("]]>", "]]>") diff --git a/setup.cfg b/setup.cfg index d83ae7d2765e..122a61370102 100644 --- a/setup.cfg +++ b/setup.cfg @@ -183,6 +183,7 @@ allowed_modules = # See https://open-edx-proposals.readthedocs.io/en/latest/best-practices/oep-0049-django-app-patterns.html#api-py api data + tests [importlinter:contract:3] name = Do not import apps from openedx-learning (only import from openedx_learning.api.* and openedx_learning.lib.*). diff --git a/xmodule/modulestore/tests/test_mixed_modulestore.py b/xmodule/modulestore/tests/test_mixed_modulestore.py index bbc91f832f96..c7770beb0f3c 100644 --- a/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -172,11 +172,16 @@ def setUp(self): ) create_or_update_xblock_upstream_link_patch.start() self.addCleanup(create_or_update_xblock_upstream_link_patch.stop) - publishableEntityLinkPatch = patch( - 'cms.djangoapps.contentstore.signals.handlers.PublishableEntityLink' + component_link_patch = patch( + 'cms.djangoapps.contentstore.signals.handlers.ComponentLink' ) - publishableEntityLinkPatch.start() - self.addCleanup(publishableEntityLinkPatch.stop) + component_link_patch.start() + self.addCleanup(component_link_patch.stop) + container_link_patch = patch( + 'cms.djangoapps.contentstore.signals.handlers.ContainerLink' + ) + container_link_patch.start() + self.addCleanup(container_link_patch.stop) def _check_connection(self): """