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

@@ -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 == "<html><body>Upstream content V2</body></html>"
@@ -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 == "<html><body>Upstream content V3</body></html>"  # "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 = "<html><data>Some content</data></html>"
 
-        # 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 == "<html><data>Some content</data></html>"
+
+        # 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