diff --git a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py index 9244ffa989..3b39f2918a 100644 --- a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py +++ b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py @@ -555,7 +555,6 @@ class ClipboardPasteFromV2LibraryTestCase(ModuleStoreTestCase): assert new_block.upstream == str(self.lib_block_key) assert new_block.upstream_version == 3 assert new_block.upstream_display_name == "MCQ-draft" - assert new_block.upstream_max_attempts == 5 return new_block_key # first verify link for copied block from library diff --git a/cms/lib/xblock/test/test_upstream_sync.py b/cms/lib/xblock/test/test_upstream_sync.py index cc3d661ca6..2f8f77ab65 100644 --- a/cms/lib/xblock/test/test_upstream_sync.py +++ b/cms/lib/xblock/test/test_upstream_sync.py @@ -1,7 +1,9 @@ """ Test CMS's upstream->downstream syncing system """ +import datetime import ddt +from pytz import utc from organizations.api import ensure_organization from organizations.models import Organization @@ -42,13 +44,33 @@ class UpstreamTestCase(ModuleStoreTestCase): title="Test Upstream Library", ) self.upstream_key = libs.create_library_block(self.library.key, "html", "test-upstream").usage_key - libs.create_library_block(self.library.key, "video", "video-upstream") upstream = xblock.load_block(self.upstream_key, self.user) upstream.display_name = "Upstream Title V2" upstream.data = "Upstream content V2" upstream.save() + self.upstream_problem_key = libs.create_library_block(self.library.key, "problem", "problem-upstream").usage_key + libs.set_library_block_olx(self.upstream_problem_key, ( + '\n' + )) + libs.publish_changes(self.library.key, self.user.id) self.taxonomy_all_org = tagging_api.create_taxonomy( @@ -179,6 +201,100 @@ class UpstreamTestCase(ModuleStoreTestCase): for object_tag in object_tags: assert object_tag.value in new_upstream_tags + # pylint: disable=too-many-statements + def test_sync_updates_to_downstream_only_fields(self): + """ + If we sync to modified content, will it preserve downstream-only fields, and overwrite the rest? + """ + downstream = BlockFactory.create(category='problem', parent=self.unit, upstream=str(self.upstream_problem_key)) + + # Initial sync + sync_from_upstream(downstream, self.user) + + # These fields are copied from upstream + assert downstream.upstream_display_name == "Upstream Problem Title V2" + assert downstream.display_name == "Upstream Problem Title V2" + assert downstream.rerandomize == '"always"' + assert downstream.matlab_api_key == 'abc' + assert not downstream.use_latex_compiler + + # These fields are "downstream only", so field defaults are preserved, and values are NOT copied from upstream + assert downstream.attempts_before_showanswer_button == 0 + assert downstream.due is None + assert not downstream.force_save_button + assert downstream.graceperiod is None + assert downstream.grading_method == 'last_score' + assert downstream.max_attempts is None + assert downstream.show_correctness == 'always' + assert not downstream.show_reset_button + assert downstream.showanswer == 'finished' + assert downstream.submission_wait_seconds == 0 + assert downstream.weight is None + + # Upstream updates + libs.set_library_block_olx(self.upstream_problem_key, ( + '\n' + )) + libs.publish_changes(self.library.key, self.user.id) + + # Modifing downstream-only fields are "safe" customizations + downstream.display_name = "Downstream Title Override" + downstream.attempts_before_showanswer_button = 2 + downstream.due = datetime.datetime(2025, 2, 2, tzinfo=utc) + downstream.force_save_button = True + downstream.graceperiod = '2d' + downstream.grading_method = 'last_score' + downstream.max_attempts = 100 + downstream.show_correctness = 'always' + downstream.show_reset_button = True + downstream.showanswer = 'on_expired' + downstream.submission_wait_seconds = 100 + downstream.weight = 3 + + # Modifying synchronized fields are "unsafe" customizations + downstream.rerandomize = '"onreset"' + downstream.matlab_api_key = 'hij' + downstream.save() + + # Follow-up sync. + sync_from_upstream(downstream, self.user) + + # "unsafe" customizations are overridden by upstream + assert downstream.upstream_display_name == "Upstream Problem Title V3" + assert downstream.rerandomize == '"per_student"' + assert downstream.matlab_api_key == 'def' + assert downstream.use_latex_compiler + + # but "safe" customizations survive + assert downstream.display_name == "Downstream Title Override" + assert downstream.attempts_before_showanswer_button == 2 + assert downstream.due == datetime.datetime(2025, 2, 2, tzinfo=utc) + assert downstream.force_save_button + assert downstream.graceperiod == '2d' + assert downstream.grading_method == 'last_score' + assert downstream.max_attempts == 100 + assert downstream.show_correctness == 'always' + assert downstream.show_reset_button + assert downstream.showanswer == 'on_expired' + assert downstream.submission_wait_seconds == 100 + assert downstream.weight == 3 + def test_sync_updates_to_modified_content(self): """ If we sync to modified content, will it preserve customizable fields, but overwrite the rest? diff --git a/cms/lib/xblock/upstream_sync.py b/cms/lib/xblock/upstream_sync.py index 0d95931ce2..4723e566ff 100644 --- a/cms/lib/xblock/upstream_sync.py +++ b/cms/lib/xblock/upstream_sync.py @@ -252,10 +252,6 @@ def _update_customizable_fields(*, upstream: XBlock, downstream: XBlock, only_fe * Set `course_problem.upstream_display_name = lib_problem.display_name` ("fetch"). * If `not only_fetch`, and `course_problem.display_name` wasn't customized, then: * Set `course_problem.display_name = lib_problem.display_name` ("sync"). - - * Set `course_problem.upstream_max_attempts = lib_problem.max_attempts` ("fetch"). - * If `not only_fetch`, and `course_problem.max_attempts` wasn't customized, then: - * Set `course_problem.max_attempts = lib_problem.max_attempts` ("sync"). """ syncable_field_names = _get_synchronizable_fields(upstream, downstream) @@ -264,6 +260,10 @@ def _update_customizable_fields(*, upstream: XBlock, downstream: XBlock, only_fe if field_name not in syncable_field_names: continue + # Downstream-only fields don't have an upstream fetch field + if fetch_field_name is None: + continue + # FETCH the upstream's value and save it on the downstream (ie, `downstream.upstream_$FIELD`). old_upstream_value = getattr(downstream, fetch_field_name) new_upstream_value = getattr(upstream, field_name) @@ -361,6 +361,9 @@ def sever_upstream_link(downstream: XBlock) -> None: downstream.upstream = None downstream.upstream_version = None for _, fetched_upstream_field in downstream.get_customizable_fields().items(): + # Downstream-only fields don't have an upstream fetch field + if fetched_upstream_field is None: + continue setattr(downstream, fetched_upstream_field, None) # Null out upstream_display_name, et al. @@ -414,21 +417,30 @@ class UpstreamSyncMixin(XBlockMixin): help=("The value of display_name on the linked upstream block."), default=None, scope=Scope.settings, hidden=True, enforce_type=True, ) - upstream_max_attempts = Integer( - help=("The value of max_attempts on the linked upstream block."), - default=None, scope=Scope.settings, hidden=True, enforce_type=True, - ) @classmethod - def get_customizable_fields(cls) -> dict[str, str]: + def get_customizable_fields(cls) -> dict[str, str | None]: """ Mapping from each customizable field to the field which can be used to restore its upstream value. + If the customizable field is mapped to None, then it is considered "downstream only", and cannot be restored + from the upstream value. + XBlocks outside of edx-platform can override this in order to set up their own customizable fields. """ return { "display_name": "upstream_display_name", - "max_attempts": "upstream_max_attempts", + "attempts_before_showanswer_button": None, + "due": None, + "force_save_button": None, + "graceperiod": None, + "grading_method": None, + "max_attempts": None, + "show_correctness": None, + "show_reset_button": None, + "showanswer": None, + "submission_wait_seconds": None, + "weight": None, } # PRESERVING DOWNSTREAM CUSTOMIZATIONS and RESTORING UPSTREAM VALUES @@ -485,6 +497,10 @@ class UpstreamSyncMixin(XBlockMixin): # if field_name in self.downstream_customized: # continue # + # # If there is no restore_field name, it's a downstream-only field + # if restore_field_name is None: + # continue + # # # If this field's value doesn't match the synced upstream value, then mark the field # # as customized so that we don't clobber it later when syncing. # # NOTE: Need to consider the performance impact of all these field lookups.