fix: squashme: service -> functions

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

cms/djangoapps/contentstore/rest_api/v1/views/vertical_block.py

@@ -21,7 +21,7 @@
     ContainerHandlerSerializer,
     VerticalContainerSerializer,
 )
-from cms.lib.xblock.upstream_sync import BadUpstream
+from cms.lib.xblock.upstream_sync import BadUpstream, inspect_upstream
 from openedx.core.lib.api.view_utils import view_auth_classes
 from xmodule.modulestore.django import modulestore  # lint-amnesty, pylint: disable=wrong-import-order
 from xmodule.modulestore.exceptions import ItemNotFoundError  # lint-amnesty, pylint: disable=wrong-import-order
@@ -283,9 +283,7 @@ def get(self, request: Request, usage_key_string: str):
                 for child in current_xblock.children:
                     child_info = modulestore().get_item(child)
                     try:
-                        upstream_info = child_info.runtime.service(
-                            child_info, "upstream_sync"
-                        ).inspect_upstream(child_info)
+                        upstream_info = inspect_upstream(child_info)
                     except BadUpstream as exc:
                         upstream_info_json = {
                             "upstream_ref": child_info.upstream,

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

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 = "<p>original upstream content</p>"
+        upstream.display_name = "Upstream Title V2"
+        upstream.data = "<html><body>Upstream content V2</body></html>"
         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<p>original upstream content</p>\n"  # @@TODO newlines??
+        assert downstream.display_name == "Upstream Title V2"
+        assert downstream.data == "<html><body>Upstream content V2</body></html>"
 
+        # Upstream updates
         upstream = xblock.load_block(self.upstream_key, self.user)
-        upstream.display_name = "NEW upstream title"
-        upstream.data = "<p>NEW upstream content</p>"
+        upstream.display_name = "Upstream Title V3"
+        upstream.data = "<html><body>Upstream content V3</body></html>"
         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<p>NEW upstream content</p>\n"  # @@TODO newlines??
+        assert downstream.display_name == "Upstream Title V3"
+        assert downstream.data == "<html><body>Upstream content V3</body></html>"
 
-    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 = "<p>downstream OVERRIDE of the content</p>"
+        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 == "<html><body>Upstream content V2</body></html>"
+
+        # Upstream updates
+        upstream = xblock.load_block(self.upstream_key, self.user)
+        upstream.display_name = "Upstream Title V3"
+        upstream.data = "<html><body>Upstream content V3</body></html>"
+        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 == "<html><body>Upstream content V3</body></html>"  # "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 = "<p>NEW upstream content</p>"
+        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<p>NEW upstream content</p>\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"