From 1f8e0b0a82ea1349d81378155c5b63585915eaab Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Sun, 6 Oct 2024 17:33:50 -0400 Subject: [PATCH] fix: break populate_restoration out of sync_from_upstream (WIP) --- cms/djangoapps/contentstore/helpers.py | 25 ++++- .../rest_api/v2/views/downstreams.py | 2 +- cms/lib/xblock/test/test_upstream_sync.py | 71 +++++++----- cms/lib/xblock/upstream_sync.py | 104 ++++++++++++------ .../core/djangoapps/content_staging/api.py | 1 + .../core/djangoapps/content_staging/data.py | 1 + .../core/djangoapps/content_staging/models.py | 2 + 7 files changed, 137 insertions(+), 69 deletions(-) diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index 62d12f4d1c2b..5bcc63cb3e88 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, sync_from_upstream +from cms.lib.xblock.upstream_sync import BadUpstream, BadDownstream, populate_block_restoration_fields 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 @@ -291,6 +291,7 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) -> user=request.user, slug_hint=user_clipboard.source_usage_key.block_id, copied_from_block=str(user_clipboard.source_usage_key), + copied_from_version_num=user_clipboard.content.version_num, tags=user_clipboard.content.tags, ) # Now handle static files that need to go into Files & Uploads: @@ -313,6 +314,9 @@ def _import_xml_node_to_parent( slug_hint: str | None = None, # UsageKey of the XBlock that this one is a copy of copied_from_block: str | None = None, + # Positive int version of source block, if applicable (e.g., library block). + # Zero if not applicable (e.g., course block). + copied_from_version_num: int = 0, # Content tags applied to the source XBlock(s) tags: dict[str, str] | None = None, ) -> XBlock: @@ -381,12 +385,25 @@ def _import_xml_node_to_parent( # Store a reference to where this block was copied from, in the 'copied_from_block' field (AuthoringMixin) temp_xblock.copied_from_block = copied_from_block if copied_from_block: - # If requested, link this block as a downstream of where it was copied from + # Try to link the pasted block (downstream) to the copied block (upstream). temp_xblock.upstream = copied_from_block try: - sync_from_upstream(temp_xblock, user, apply_updates=False) - except BadUpstream as exc: + UpstreamLink.fetch_for_block(temp_xblock) + except (BadDownstream, BadUpstream): + # Usually this will fail. For example, if the copied block is a modulestore course block, it can't be an + # upstream. That's fine; just don't link. temp_xblock.upstream = None + else: + # But if it doesn't fail, then populate the `upstream_version` field based on what was copied. Note that + # this could be the latest Published version, or it could be an an even newer Draft version. + temp_xblock.upstream_version = copied_from_version_num + # Also populate the restoration fields (`upstream_display_name`, etc.). Recall that the copied block could + # be a Draft. So, rather than loading the restoration values from the Published upstream (which could be + # older), load them from the copied block itself. That way, if an author customizes a field, but then 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 pull in new + # restoration field values from the Published upstream content. + populate_block_restoration_fields(from_block=temp_xblock, to_block=temp_xblock) # Save the XBlock into modulestore. We need to save the block and its parent for this to work: new_xblock = store.update_item(temp_xblock, user.id, allow_not_found=True) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py index 1f6b9e2134e1..c5629e14aa77 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py @@ -137,7 +137,7 @@ def post(self, request: Request, usage_key_string: str) -> Response: raise NotFound(detail=f"Block '{usage_key_string}' is not linked to a library block") old_version = downstream.upstream_version try: - sync_from_upstream(downstream, request.user, apply_updates=True) + sync_from_upstream(downstream, request.user) except (BadUpstream, BadDownstream) as exc: raise ValidationError(detail=str(exc)) from exc modulestore().update_item(downstream, request.user.id) diff --git a/cms/lib/xblock/test/test_upstream_sync.py b/cms/lib/xblock/test/test_upstream_sync.py index 85ac4d78b640..d783006edba7 100644 --- a/cms/lib/xblock/test/test_upstream_sync.py +++ b/cms/lib/xblock/test/test_upstream_sync.py @@ -6,7 +6,9 @@ from organizations.api import ensure_organization from organizations.models import Organization -from cms.lib.xblock.upstream_sync import sync_from_upstream, BadUpstream, UpstreamLink, decline_sync +from cms.lib.xblock.upstream_sync import ( + sync_from_upstream, BadUpstream, UpstreamLink, decline_sync, populate_block_restoration_fields +) 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 @@ -15,7 +17,7 @@ @ddt.ddt -class UpstreamSyncTestCase(ModuleStoreTestCase): +class UpstreamTestCase(ModuleStoreTestCase): """ Tests the UpstreamSyncMixin """ @@ -52,7 +54,7 @@ def test_sync_no_upstream(self): block.display_name = "Block Title" block.data = "Block content" - sync_from_upstream(block, self.user, apply_updates=True) + sync_from_upstream(block, self.user) assert block.display_name == "Block Title" assert block.data == "Block content" @@ -76,7 +78,7 @@ 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, apply_updates=True) + sync_from_upstream(block, self.user) assert block.display_name == "Block Title" assert block.data == "Block content" @@ -89,7 +91,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, apply_updates=True) + sync_from_upstream(downstream, user_who_cannot_read_upstream) def test_sync_updates_happy_path(self): """ @@ -98,7 +100,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, apply_updates=True) + sync_from_upstream(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" @@ -111,7 +113,7 @@ def test_sync_updates_happy_path(self): upstream.save() # Follow-up sync. Assert that updates are pulled into downstream. - sync_from_upstream(downstream, self.user, apply_updates=True) + sync_from_upstream(downstream, self.user) assert downstream.upstream_version == 3 assert downstream.upstream_display_name == "Upstream Title V3" assert downstream.display_name == "Upstream Title V3" @@ -124,7 +126,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, apply_updates=True) + sync_from_upstream(downstream, self.user) assert downstream.upstream_display_name == "Upstream Title V2" assert downstream.display_name == "Upstream Title V2" assert downstream.data == "Upstream content V2" @@ -141,7 +143,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, apply_updates=True) + sync_from_upstream(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 @@ -158,7 +160,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, apply_updates=True) + # sync_from_upstream(downstream, self.user) # assert downstream.downstream_customized == [] # assert downstream.display_name == downstream.upstream_display_name == "Upstream Title V2" # @@ -168,7 +170,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, apply_updates=True) + # sync_from_upstream(downstream, self.user) # assert downstream.upstream_version == 2 # assert downstream.upstream_display_name == "Upstream Title V2" # assert downstream.display_name == "Title V3" @@ -179,7 +181,7 @@ def test_sync_updates_to_modified_content(self): # upstream.save() # # # ...which is reflected when we sync. - # sync_from_upstream(downstream, self.user, apply_updates=True) + # sync_from_upstream(downstream, self.user) # assert downstream.upstream_version == 3 # assert downstream.upstream_display_name == downstream.display_name == "Title V3" # @@ -190,7 +192,7 @@ def test_sync_updates_to_modified_content(self): # upstream.save() # # # ...then the downstream title should remain put. - # sync_from_upstream(downstream, self.user, apply_updates=True) + # sync_from_upstream(downstream, self.user) # assert downstream.upstream_version == 4 # assert downstream.upstream_display_name == "Title V4" # assert downstream.display_name == "Title V3" @@ -199,27 +201,40 @@ def test_sync_updates_to_modified_content(self): # downstream.downstream_customized = [] # upstream.display_name = "Title V5" # upstream.save() - # sync_from_upstream(downstream, self.user, apply_updates=True) + # sync_from_upstream(downstream, self.user) # assert downstream.upstream_version == 5 # assert downstream.upstream_display_name == downstream.display_name == "Title V5" - def test_sync_without_applying_updates(self): + @ddt.data(None, "Title From Some Other Upstream Version") + def test_populate_restoration_fields(self, initial_upstream_display_name): """ - Can we sync *defaults* (not updates) from a content library block to a linked out-of-date course block? + Can we populate a block's restoration fields (i.e., upstream-set-defaults) without syncing it? + + Test both with and without a pre-populated restoration field. """ - # 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.unit, upstream=str(self.upstream_key)) - assert not downstream.upstream_display_name - downstream.display_name = "Title V1" - downstream.data = "Content V1" + to_block = BlockFactory.create(category='html', parent=self.unit) + to_block.upstream_display_name = initial_upstream_display_name + to_block.display_name = "Some Title" + to_block.data = "Some content" - # Sync, but without applying updates - sync_from_upstream(downstream, self.user, apply_updates=False) + # Note that we're not linked to any upstream. populate_ shouldn't care. + assert not to_block.upstream + assert not to_block.upstream_version - assert downstream.upstream_display_name == "Upstream Title V2" - assert downstream.display_name == "Title V1" - assert downstream.data == "Content V1" + # populate_ from an upstream. + from_block = xblock.load_block(self.upstream_key, self.user) + populate_block_restoration_fields(from_block=from_block, to_block=to_block) + + # Ensure: populate_ doesn't affect the upstream link (or lack thereof). + assert not to_block.upstream + assert not to_block.upstream_version + + # Ensure: populate_ doesn't affect actual content or settings. + assert to_block.display_name == "Some Title" + assert to_block.data == "Some content" + + # Ensure: populate_ DOES set the restoration field. + assert to_block.upstream_display_name == "Upstream Title V2" def test_prompt_and_decline_sync(self): """ @@ -234,7 +249,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, apply_updates=True) + sync_from_upstream(downstream, self.user) link = UpstreamLink.get_for_block(downstream) assert link.version_synced == 2 assert link.version_declined is None diff --git a/cms/lib/xblock/upstream_sync.py b/cms/lib/xblock/upstream_sync.py index 8cf99fb3b225..503af0f3efe6 100644 --- a/cms/lib/xblock/upstream_sync.py +++ b/cms/lib/xblock/upstream_sync.py @@ -101,7 +101,7 @@ def get_for_block(cls, downstream: XBlock) -> t.Self | None: If link is valid, returns an UpstreamLink object. If the syncing to `downstream` is not supported, raises BadDownstream. If the upstream link is invalid or broken, raises BadUpstream. - If `downstream` is not linked at all, does nothing. + If `downstream` is not linked at all, returns None. """ if not downstream.upstream: return None @@ -145,65 +145,97 @@ def get_for_block(cls, downstream: XBlock) -> t.Self | None: def sync_from_upstream(downstream: XBlock, user: AbstractUser) -> None: """ - Given an XBlock that is linked to upstream content, fetch latest upstream fields and optionally apply them. + 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. + 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 the syncing to `downstream` is not supported, raises BadDownstream. If the upstream link is invalid or broken, raises BadUpstream. If `downstream` is not linked at all, does nothing. """ - # Try to load the upstream. - upstream_link = UpstreamLink.get_for_block(downstream) # Can raise BadUpstream or BadUpstream - if not upstream_link: - return # No upstream -> nothing to sync. + if not (link := UpstreamLink.get_for_block(downstream)): # Can raise BadUpstream or BadUpstream + return try: - upstream = xblock_api.load_block(UsageKey.from_string(downstream.upstream), user) + lib_block = 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 + _update_customizable_fields(upstream=lib_block, downstream=downstream, skip_actual_fields=False) + _update_non_customizable_fields(upstream=lib_block, downstream=downstream) + downstream.upstream_version = link.version_available + - customizable_fields = downstream.get_customizable_fields() +def populate_block_restoration_fields(*, from_block: XBlock, to_block: XBlock): + """ + @@TODO finish implementing + """ + _update_customizable_fields(upstream=from_block, downstream=to_block, skip_actual_fields=True) + + +def _update_customizable_fields(*, upstream: XBlock, downstream: XBlock, skip_actual_fields: bool) -> None: + """ + Internal helper to @@TODO + """ + applicable_field_names = _get_shared_authored_field_names(upstream, downstream) - # For every field: - for field_name, field in upstream.__class__.fields.items(): + for field_name, restoration_field_name in downstream.get_customizable_fields().items(): - # ...(ignoring fields that aren't set in the authoring environment)... - if field.scope not in [Scope.content, Scope.settings]: + if field_name not in applicable_field_names: continue - # if the field *can be* customized (whether or not it *has been* customized)... + # Save the upstream's value to the restoration field on the downstream (ie, `downstream.upstream_$FIELD`). + old_upstream_value = getattr(downstream, restoration_field_name) new_upstream_value = getattr(upstream, field_name) - old_upstream_value = None - if restore_field_name := customizable_fields.get(field.name): - - # ...then save its latest upstream value to a hidden field. - old_upstream_value = getattr(downstream, restore_field_name) - setattr(downstream, restore_field_name, new_upstream_value) + setattr(downstream, restoration_field_name, new_upstream_value) - # And, if we're applying updates... - if not apply_updates: + if skip_actual_fields: continue - # ...*and* the field is non-customized... - if field_name in customizable_fields: + # 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 "PRESRVING 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 - # (Determining whether a field has been customized will differ in Beta vs Future release. - # See "PRESRVING DOWNSTREAM CUSTOMIZATIONS" comment below for details. + ## 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. - # FUTURE BEHAVIOR: field is "customized" iff we have noticed that the user edited it. - # if field_name in downstream.downstream_customized: - # continue + # Field isn't customized -- update it! + setattr(downstream, field_name, new_upstream_value) - # 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 - # ... then actually apply the upstream update to the downstream block! +def _update_non_customizable_fields(*, upstream: XBlock, downstream: XBlock) -> None: + """ + @@TODO + """ + applicable_fields = _get_shared_authored_field_names(upstream, downstream) + customizable_fields = set(downstream.get_customizable_fields().keys()) + non_customizable_fields: set[str] = applicable_fields - customizable_fields + for field_name in non_customizable_fields: + new_upstream_value = getattr(upstream, field_name) setattr(downstream, field_name, new_upstream_value) - # Done syncing. Record the latest upstream version for future reference. - downstream.upstream_version = upstream_link.version_available + +def _get_shared_authored_field_names(*blocks: XBlock) -> set[str]: + """ + Given one or more blocks, get the set of content- and settings-scoped fields which are defined on all of them. + """ + 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 blocks + ]) def decline_sync(downstream: XBlock) -> None: @@ -271,7 +303,7 @@ class UpstreamSyncMixin(XBlockMixin): @classmethod def get_customizable_fields(cls) -> dict[str, str]: """ - Mapping from each customizable field to field which stores its upstream default. + Mapping from each customizable field to the field which can be used to restore its upstream value. XBlocks outside of edx-platform can override this in order to set up their own customizable fields. """ diff --git a/openedx/core/djangoapps/content_staging/api.py b/openedx/core/djangoapps/content_staging/api.py index 8632d0cb0740..206740efe19a 100644 --- a/openedx/core/djangoapps/content_staging/api.py +++ b/openedx/core/djangoapps/content_staging/api.py @@ -205,6 +205,7 @@ def _user_clipboard_model_to_data(clipboard: _UserClipboard) -> UserClipboardDat block_type=content.block_type, display_name=content.display_name, tags=content.tags, + version_num=content.version_num, ), source_usage_key=clipboard.source_usage_key, source_context_title=clipboard.get_source_context_title(), diff --git a/openedx/core/djangoapps/content_staging/data.py b/openedx/core/djangoapps/content_staging/data.py index e952357f4cf9..0d52e53c1d92 100644 --- a/openedx/core/djangoapps/content_staging/data.py +++ b/openedx/core/djangoapps/content_staging/data.py @@ -43,6 +43,7 @@ class StagedContentData: block_type: str = field(validator=validators.instance_of(str)) display_name: str = field(validator=validators.instance_of(str)) tags: dict = field(validator=validators.optional(validators.instance_of(dict))) + version_num: int = field(validator=validators.optional(validators.instance_of(int))) @frozen diff --git a/openedx/core/djangoapps/content_staging/models.py b/openedx/core/djangoapps/content_staging/models.py index 334fc1c6b5d3..2eab7954e826 100644 --- a/openedx/core/djangoapps/content_staging/models.py +++ b/openedx/core/djangoapps/content_staging/models.py @@ -63,6 +63,8 @@ class Meta: # A _suggested_ URL name to use for this content. Since this suggestion may already be in use, it's fine to generate # a new url_name instead. suggested_url_name = models.CharField(max_length=1024) + # If applicable, an int >=1 indicating the version of copied content. If not applicable, zero (default). + version_num = models.PositiveIntegerField(default=0) # Tags applied to the original source block(s) will be copied to the new block(s) on paste. tags = models.JSONField(null=True, help_text=_("Content tags applied to these blocks"))