From 0c9a2a27481deede4dcb5d6cc62c5d8f3ac7612d Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Mon, 23 Sep 2024 14:51:26 -0400 Subject: [PATCH] fix: squashme: service -> functions --- cms/djangoapps/contentstore/helpers.py | 6 +- cms/djangoapps/contentstore/utils.py | 4 +- cms/lib/xblock/test/test_upstream_sync.py | 218 ++++++++++++++++++---- cms/lib/xblock/upstream_sync.py | 146 +++++++-------- 4 files changed, 247 insertions(+), 127 deletions(-) diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index 915545ae51a1..377314270913 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -23,7 +23,7 @@ from xmodule.xml_block import XmlMixin from cms.djangoapps.models.settings.course_grading import CourseGradingModel -from cms.lib.xblock.upstream_sync import BadUpstream +from cms.lib.xblock.upstream_sync import BadUpstream, sync_from_upstream 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 @@ -389,9 +389,7 @@ def _import_xml_node_to_parent( # If requested, link this block as a downstream of where it was copied from temp_xblock.upstream = copied_from_block try: - temp_xblock.runtime.service( - temp_xblock, 'upstream_sync' - ).sync_from_upstream(temp_xblock, apply_updates=False) + sync_from_upstream(temp_xblock, user, apply_updates=False) except BadUpstream as exc: log.exception( "Pasting content with link_to_upstream=True, but copied content is not a valid upstream. Will not link." diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 9fcdd542283e..214193918eb4 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -98,7 +98,6 @@ ) from cms.djangoapps.models.settings.course_grading import CourseGradingModel from cms.djangoapps.models.settings.course_metadata import CourseMetadata -from cms.lib.xblock.upstream_sync import UpstreamSyncService from xmodule.library_tools import LibraryToolsService from xmodule.course_block import DEFAULT_START_DATE # lint-amnesty, pylint: disable=wrong-import-order from xmodule.data import CertificatesDisplayBehaviors @@ -1266,8 +1265,7 @@ def load_services_for_studio(runtime, user): "settings": SettingsService(), "lti-configuration": ConfigurationService(CourseAllowPIISharingInLTIFlag), "teams_configuration": TeamsConfigurationService(), - "library_tools": LibraryToolsService(modulestore(), user.id), - "upstream_sync": UpstreamSyncService(user), + "library_tools": LibraryToolsService(modulestore(), user.id) } runtime._services.update(services) # lint-amnesty, pylint: disable=protected-access diff --git a/cms/lib/xblock/test/test_upstream_sync.py b/cms/lib/xblock/test/test_upstream_sync.py index 99e199d26b28..dd3a01ed4d49 100644 --- a/cms/lib/xblock/test/test_upstream_sync.py +++ b/cms/lib/xblock/test/test_upstream_sync.py @@ -1,16 +1,20 @@ """ Test CMS's upstream->downstream syncing system """ +import ddt + from organizations.api import ensure_organization from organizations.models import Organization -from cms.lib.xblock.upstream_sync import UpstreamSyncService +from cms.lib.xblock.upstream_sync import sync_from_upstream, BadUpstream +from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangoapps.content_libraries import api as libs 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 +@ddt.ddt class UpstreamSyncTestCase(ModuleStoreTestCase): """ Tests the UpstreamSyncMixin @@ -32,69 +36,203 @@ def setUp(self): parent=chapter, display_name='Test Sequential' ) - vertical = BlockFactory.create( + self.vertical = BlockFactory.create( category='vertical', parent=sequential, display_name='Test Vertical' ) + ensure_organization("TestX") - self.upstream_key = libs.create_library_block( - libs.create_library( - org=Organization.objects.get(short_name="TestX"), - slug="TestLib", - title="Test Upstream Library", - ).key, - "html", - "test-upstream", - ).usage_key + library = libs.create_library( + org=Organization.objects.get(short_name="TestX"), + slug="TestLib", + title="Test Upstream Library", + ) + self.upstream_key = libs.create_library_block(library.key, "html", "test-upstream").usage_key + libs.create_library_block(library.key, "video", "video-upstream") + upstream = xblock.load_block(self.upstream_key, self.user) - upstream.display_name = "original upstream title" - upstream.data = "

original upstream content

" + upstream.display_name = "Upstream Title V2" + upstream.data = "Upstream content V2" upstream.save() - downstream = BlockFactory.create(category='html', parent=vertical, upstream=str(self.upstream_key)) - self.sync_service = UpstreamSyncService(self.user) - self.sync_service.sync_from_upstream(downstream, apply_updates=True) - downstream.save() - self.store.update_item(downstream, self.user.id) - self.downstream_key = downstream.usage_key - def test_sync_to_unmodified_content(self): + def test_sync_no_upstream(self): + """ + Trivial case: Syncing a block with no upstream is a no-op + """ + block = BlockFactory.create(category='html', parent=self.vertical) + block.display_name = "Block Title" + block.data = "Block content" + + sync_from_upstream(block, self.user, apply_updates=True) + + assert block.display_name == "Block Title" + assert block.data == "Block content" + assert not block.upstream_display_name + assert block.downstream_customized == [] + + @ddt.data( + ("not-a-key-at-all", ".*is malformed.*"), + ("course-V2:Oops+ItsA+CourseKey", ".*is malformed.*"), + ("block-V2:The+Wrong+KindOfUsageKey+type@html+block@nope", ".*is malformed.*"), + ("lb:TestX:TestLib:video:video-upstream", ".*type mismatch.*"), + ("lb:TestX:NoSuchLib:html:block-id", ".*not found in the system.*"), + ("lb:TestX:TestLib:html:no-such-html", ".*not found in the system.*"), + ) + @ddt.unpack + def test_sync_bad_upstream(self, upstream, message_regex): + """ + Syncing with a bad upstream raises BadUpstream, but doesn't affect the block + """ + block = BlockFactory.create(category='html', parent=self.vertical, upstream=upstream) + block.display_name = "Block Title" + block.data = "Block content" + + with self.assertRaisesRegex(BadUpstream, message_regex): + sync_from_upstream(block, self.user, apply_updates=True) + + assert block.display_name == "Block Title" + assert block.data == "Block content" + assert not block.upstream_display_name + assert block.downstream_customized == [] + + def test_sync_not_accessible(self): + """ + Syncing with an block that exists, but is inaccessible, raises BadUpstream + """ + downstream = BlockFactory.create(category='html', parent=self.vertical, 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, apply_updates=True) + + def test_sync_updates_happy_path(self): """ Can we sync updates from a content library block to a linked out-of-date course block? """ - downstream = self.store.get_item(self.downstream_key) - assert downstream.upstream_display_name == "original upstream title" + downstream = BlockFactory.create(category='html', parent=self.vertical, upstream=str(self.upstream_key)) + + # Initial sync + sync_from_upstream(downstream, self.user, apply_updates=True) + 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.downstream_customized == [] - assert downstream.display_name == "original upstream title" - assert downstream.data == "\n

original upstream content

\n" # @@TODO newlines?? + assert downstream.display_name == "Upstream Title V2" + assert downstream.data == "Upstream content V2" + # Upstream updates upstream = xblock.load_block(self.upstream_key, self.user) - upstream.display_name = "NEW upstream title" - upstream.data = "

NEW upstream content

" + upstream.display_name = "Upstream Title V3" + upstream.data = "Upstream content V3" upstream.save() - self.sync_service.sync_from_upstream(downstream, apply_updates=True) - assert downstream.upstream_display_name == "NEW upstream title" + # Follow-up sync. Assert that updates are pulled into downstream. + sync_from_upstream(downstream, self.user, apply_updates=True) + assert downstream.upstream_version == 3 + assert downstream.upstream_display_name == "Upstream Title V3" assert downstream.downstream_customized == [] - assert downstream.display_name == "NEW upstream title" - assert downstream.data == "\n

NEW upstream content

\n" # @@TODO newlines?? + assert downstream.display_name == "Upstream Title V3" + assert downstream.data == "Upstream content V3" - def test_sync_to_modified_contenet(self): + def test_sync_updates_to_modified_content(self): """ If we sync to modified content, will it preserve customizable fields, but overwrite the rest? """ - downstream = self.store.get_item(self.downstream_key) - downstream.display_name = "downstream OVERRIDE of the title" - downstream.data = "

downstream OVERRIDE of the content

" + downstream = BlockFactory.create(category='html', parent=self.vertical, upstream=str(self.upstream_key)) + + # Initial sync + sync_from_upstream(downstream, self.user, apply_updates=True) + assert downstream.upstream_display_name == "Upstream Title V2" + assert downstream.downstream_customized == [] + assert downstream.display_name == "Upstream Title V2" + assert downstream.data == "Upstream content V2" + + # Upstream updates + upstream = xblock.load_block(self.upstream_key, self.user) + upstream.display_name = "Upstream Title V3" + upstream.data = "Upstream content V3" + upstream.save() + + # Downstream modifications + downstream.display_name = "Downstream Title Override" # "safe" customization + downstream.data = "Downstream content override" # "unsafe" override + downstream.save() + + # Follow-up sync. Assert that updates are pulled into downstream, but customizations are saved. + sync_from_upstream(downstream, self.user, apply_updates=True) + assert downstream.upstream_display_name == "Upstream Title V3" + assert downstream.downstream_customized == ["display_name"] + assert downstream.display_name == "Downstream Title Override" # "safe" customization survives + assert downstream.data == "Upstream content V3" # "unsafe" override is gone + + def test_sync_to_downstream_with_subtle_customization(self): + """ + Edge case: If our downstream customizes a field, but then the upstream is changed to match the customization, + do we still remember that the downstream field is customized? That is, if the upstream later changes + again, do we retain the downstream customization (rather than following the upstream update?) + """ + # Start with an uncustomized downstream block. + downstream = BlockFactory.create(category='html', parent=self.vertical, upstream=str(self.upstream_key)) + sync_from_upstream(downstream, self.user, apply_updates=True) + assert downstream.downstream_customized == [] + assert downstream.display_name == downstream.upstream_display_name == "Upstream Title V2" + + # Then, customize our downstream title. + downstream.display_name = "Title V3" downstream.save() + assert downstream.downstream_customized == ["display_name"] + + # Syncing should retain the customization. + sync_from_upstream(downstream, self.user, apply_updates=True) + assert downstream.upstream_version == 2 + assert downstream.upstream_display_name == "Upstream Title V2" + assert downstream.display_name == "Title V3" + # Whoa, look at that, the upstream has updated itself to the exact same title... upstream = xblock.load_block(self.upstream_key, self.user) - upstream.display_name = "NEW upstream title" - upstream.data = "

NEW upstream content

" + upstream.display_name = "Title V3" upstream.save() - self.sync_service.sync_from_upstream(downstream, apply_updates=True) - assert downstream.upstream_display_name == "NEW upstream title" + # ...which is reflected when we sync. + sync_from_upstream(downstream, self.user, apply_updates=True) + assert downstream.upstream_version == 3 + assert downstream.upstream_display_name == downstream.display_name == "Title V3" + + # But! Our downstream knows that its title is still customized. assert downstream.downstream_customized == ["display_name"] - assert downstream.display_name == "downstream OVERRIDE of the title" - assert downstream.data == "\n

NEW upstream content

\n" # @@TODO newlines?? + # So, if the upstream title changes again... + upstream.display_name = "Title V4" + upstream.save() + + # ...then the downstream title should remain put. + sync_from_upstream(downstream, self.user, apply_updates=True) + assert downstream.upstream_version == 4 + assert downstream.upstream_display_name == "Title V4" + assert downstream.display_name == "Title V3" + + # Finally, if we "de-customize" the display_name field, then it should go back to syncing normally. + downstream.downstream_customized = [] + upstream.display_name = "Title V5" + upstream.save() + sync_from_upstream(downstream, self.user, apply_updates=True) + assert downstream.upstream_version == 5 + assert downstream.upstream_display_name == downstream.display_name == "Title V5" + + def test_sync_skip_updates(self): + """ + Can we sync *defaults* (not updates) from a content library block to a linked out-of-date course block? + """ + # Initial state: Block is linked to upstream, but with some out-of-date fields, potentially + # from an import or a copy-paste operation. + downstream = BlockFactory.create(category='html', parent=self.vertical, upstream=str(self.upstream_key)) + assert not downstream.upstream_display_name + downstream.display_name = "Title V1" + downstream.data = "Content V1" + assert downstream.downstream_customized == [] + + # Sync, but without applying updates + sync_from_upstream(downstream, self.user, apply_updates=False) + + assert downstream.upstream_display_name == "Upstream Title V2" + assert downstream.downstream_customized == [] + assert downstream.display_name == "Title V1" + assert downstream.data == "Content V1" diff --git a/cms/lib/xblock/upstream_sync.py b/cms/lib/xblock/upstream_sync.py index cc0e781ae112..0b84a4ba2a41 100644 --- a/cms/lib/xblock/upstream_sync.py +++ b/cms/lib/xblock/upstream_sync.py @@ -46,104 +46,90 @@ class UpstreamInfo: latest_version: int | None # Latest version of the upstream that's available, or None if it couldn't be loaded. -class UpstreamSyncService: +def sync_from_upstream(downstream: XBlock, user: User, *, apply_updates: bool) -> None: """ @@TODO docstring + + Does not save `downstream` to the store. That is left up to the caller. + + Raises: BadUpstream """ - def __init__(self, user: User): - self.user = user + # No upstream -> no sync. + if not downstream.upstream: + return - def sync_from_upstream(self, downstream: XBlock, *, apply_updates: bool) -> None: - """ - @@TODO docstring + # Try to load the upstream. + upstream_info = inspect_upstream(downstream) # Can raise BadUpstream + try: + upstream = xblock_api.load_block(UsageKey.from_string(downstream.upstream), user) + except NotFound as exc: + raise BadUpstream(_("Linked library item could not be loaded: {}").format(downstream.upstream)) from exc - Does not save `downstream` to the store. That is left up to the caller. + # For every field... + customizable_fields_to_restore_fields = downstream.get_customizable_fields() + for field_name, field in upstream.__class__.fields.items(): - Raises: BadUpstream - """ + # ...(ignoring fields that aren't set in the authoring environment)... + if field.scope not in [Scope.content, Scope.settings]: + continue - # No upstream -> no sync. - if not downstream.upstream: - return - - # Try to load the upstream. - upstream_info = self.inspect_upstream(downstream) # Can raise BadUpstream - try: - upstream = xblock_api.load_block(UsageKey.from_string(downstream.upstream), self.user) - except NotFound: - raise BadUpstream( - _("Linked library item could not be loaded: {}").format(downstream.upstream) - ) from NotFound - - # For every field... - customizable_fields_to_restore_fields = downstream.get_customizable_fields() - for field_name, field in upstream.__class__.fields.items(): - - # ...(ignoring fields that aren't set in the authoring environment)... - if field.scope not in [Scope.content, Scope.settings]: - continue + # ...if the field *can be* customized (whether or not it *has been* customized), then write its latest + # upstream value to a hidden field, allowing authors to restore it as desired. + # Example: this sets `downstream.upstream_display_name = upstream.display_name`. + upstream_value = getattr(upstream, field_name) + if restore_field_name := customizable_fields_to_restore_fields.get(field.name): + setattr(downstream, restore_field_name, upstream_value) - # ...if the field *can be* customized (whether or not it *has been* customized), then write its latest - # upstream value to a hidden field, allowing authors to restore it as desired. - # Example: this sets `downstream.upstream_display_name = upstream.display_name`. - upstream_value = getattr(upstream, field_name) - if restore_field_name := customizable_fields_to_restore_fields.get(field.name): - setattr(downstream, restore_field_name, upstream_value) + # ...if we're applying updates, *and* the field hasn't been customized, then update the downstream value. + # This is the part of the sync that actually pulls in updated upstream content. + # Example: this sets `downstream.display_name = upstream.display_name`. + if apply_updates and field_name not in downstream.downstream_customized: + setattr(downstream, field_name, upstream_value) - # ...if we're applying updates, *and* the field hasn't been customized, then update the downstream value. - # This is the part of the sync that actually pulls in updated upstream content. - # Example: this sets `downstream.display_name = upstream.display_name`. - if apply_updates and field_name not in downstream.downstream_customized: - setattr(downstream, field_name, upstream_value) + # Done syncing. Record the latest upstream version for future reference. + downstream.upstream_version = upstream_info.latest_version - # Done syncing. Record the latest upstream version for future reference. - downstream.upstream_version = upstream_info.latest_version - def inspect_upstream(self, downstream: XBlock) -> UpstreamInfo | None: - """ - Get info on a block's relationship with its upstream without actually loading any upstream content. +def inspect_upstream(downstream: XBlock) -> UpstreamInfo | None: + """ + Get info on a block's relationship with its upstream without actually loading any upstream content. - Currently, the only supported upstream are LC-backed Library Components. This may change in the future (see - module docstring). + Currently, the only supported upstream are LC-backed Library Components. This may change in the future (see + module docstring). - Raises: BadUpstream - """ - if not downstream.upstream: - return None - try: - upstream_key = LibraryUsageLocatorV2.from_string(downstream.upstream) - except InvalidKeyError: - raise BadUpstream( - _("Reference to linked library item is malformed") - ) from InvalidKeyError - 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. - raise BadUpstream( - _("Content type mistmatch: {downstream_type} is linked to {upstream_type}").format( - downstream_type=downstream_type, upstream_type=upstream_key.block_type - ) - ) from TypeError( - f"downstream block '{downstream.usage_key}' is linked to " - f"upstream block of different type '{upstream_key}'" + Raises: BadUpstream + """ + if not downstream.upstream: + return None + 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. + raise BadUpstream( + _("Content type mismatch: {downstream_type} is linked to {upstream_type}").format( + downstream_type=downstream_type, upstream_type=upstream_key.block_type ) - try: - lib_meta = get_library_block(upstream_key) - except XBlockNotFoundError: - raise BadUpstream( - _("Linked library item was not found in the system").format(downstream.upstream) - ) from XBlockNotFoundError - return UpstreamInfo( - upstream_ref=downstream.upstream, - current_version=downstream.upstream_version, - latest_version=(lib_meta.version_num if lib_meta else None), + ) from TypeError( + f"downstream block '{downstream.usage_key}' is linked to " + f"upstream block of different type '{upstream_key}'" ) + try: + lib_meta = get_library_block(upstream_key) + except XBlockNotFoundError as exc: + raise BadUpstream(_("Linked library item was not found in the system").format(downstream.upstream)) from exc + return UpstreamInfo( + upstream_ref=downstream.upstream, + current_version=downstream.upstream_version, + latest_version=(lib_meta.version_num if lib_meta else None), + ) -@XBlockMixin.needs("upstream_sync") class UpstreamSyncMixin(XBlockMixin): """ Allows an XBlock in the CMS to be associated & synced with an upstream.