From 3847cec503df0ca4c3c452d6daa6cd66474f92f1 Mon Sep 17 00:00:00 2001 From: Jillian Date: Tue, 28 Jan 2025 02:07:53 +1030 Subject: [PATCH] feat: add and switch to downstream-only fields [FC-0076] (#36158) Adds the concept of "downstream-only" fields to the XBlock upstream sync logic. Downstream-only fields are customizable fields set only on the downstream XBlock -- we don't keep track of the upstream field value anywhere on the downstream XBlock. Changes made to these fields in the upstream block are ignored, and if the link to the upstream block is severed, the downstream changes are preserved (not reset back to defaults, like the upstream-tracked customizable fields are). The fields chosen as "downstream-only" are those related to scoring and grading. The `max_attempts` field was previously a customizable field that tracked the upstream value. However, because it is scoring-related, it has been converted to a "downstream-only" field. This change impacts course authors' use of library content in their courses. --- .../views/tests/test_clipboard_paste.py | 1 - cms/lib/xblock/test/test_upstream_sync.py | 118 +++++++++++++++++- cms/lib/xblock/upstream_sync.py | 36 ++++-- 3 files changed, 143 insertions(+), 12 deletions(-) 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.