fix: break populate_restoration out of sync_from_upstream (WIP)

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)

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)

cms/lib/xblock/test/test_upstream_sync.py

@@ -52,7 +52,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 +76,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 +89,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 +98,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 +111,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 +124,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 == "<html><body>Upstream content V2</body></html>"
@@ -141,7 +141,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 == "<html><body>Upstream content V3</body></html>"  # "unsafe" override is gone
@@ -158,7 +158,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 +168,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 +179,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 +190,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,20 +199,21 @@ 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):
+    def test_populate_restoration_fields(self):
         """
-        Can we sync *defaults* (not updates) from a content library block to a linked out-of-date course block?
+        Can we load *defaults* (not updates) from a content library block to a linked course block?
+
+        @@TODO fix this test
         """
-        # Initial state: Block is linked to upstream, but with some out-of-date fields, potentially
-        # from an import or a copy-paste operation.
+        # Initial state: Block is linked to upstream, with some fields.
         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"
+        downstream.display_name = "Title Vwhatever"
+        downstream.data = "Content Version Whatever"
 
         # Sync, but without applying updates
         sync_from_upstream(downstream, self.user, apply_updates=False)
@@ -234,7 +235,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.fetch_for_block(downstream)
         assert link.version_synced == 2
         assert link.version_declined is None

cms/lib/xblock/upstream_sync.py

@@ -101,7 +101,7 @@ def fetch_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
@@ -143,68 +143,82 @@ def fetch_for_block(cls, downstream: XBlock) -> t.Self | None:
         )
 
 
-def sync_from_upstream(downstream: XBlock, user: AbstractUser, *, apply_updates: bool) -> 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.fetch_for_block(downstream)  # Can raise BadUpstream or BadUpstream
-    if not upstream_link:
-        return  # No upstream -> nothing to sync.
+    if not (link := UpstreamLink.fetch_for_block(downstream)):  # Can raise BadUpstream or BadUpstream
+        return None
     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()
 
-    # For every field:
-    for field_name, field in upstream.__class__.fields.items():
+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)
 
-        # ...(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)...
-        new_upstream_value = getattr(upstream, field_name)
-        old_upstream_value = None
-        if restore_field_name := customizable_fields.get(field.name):
+def _update_customizable_fields(*, upstream: XBlock, downstream: XBlock, skip_actual_fields: bool) -> None:
+    """
+    Internal helper to @@TODO
+    """
+    for field_name, restoration_field_name in downstream.get_customizable_fields().items():
 
-            # ...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)
+        # 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)
+        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.)
 
-            # (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
+        #  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
+    """
+    customizable_fields = set(downstream.get_customizable_fields())
+    for field_name, field in upstream.__class__.fields.items():
+        if field.scope not in [Scope.content, Scope.settings]:
+            continue
+        if field_name in customizable_fields:
+            continue
+        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 decline_sync(downstream: XBlock) -> None:
     """
@@ -271,7 +285,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.
         """

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(),

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

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"))