feat: add and switch to downstream-only fields [FC-0076] (openedx#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.
mitodl · Jan 27, 2025 · 3847cec · 3847cec
1 parent 46c7f6d
commit 3847cec
Show file tree

Hide file tree

Showing 3 changed files with 143 additions and 12 deletions.
diff --git a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py
@@ -555,7 +555,6 @@ def _copy_paste_and_assert_link(key_to_copy):
             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
@@ -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 @@ def setUp(self):
             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 = "<html><body>Upstream content V2</body></html>"
         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, (
+            '<problem'
+            ' attempts_before_showanswer_button="1"'
+            ' display_name="Upstream Problem Title V2"'
+            ' due="2024-01-01T00:00:00Z"'
+            ' force_save_button="false"'
+            ' graceperiod="1d"'
+            ' grading_method="last_attempt"'
+            ' matlab_api_key="abc"'
+            ' max_attempts="10"'
+            ' rerandomize="&quot;always&quot;"'
+            ' show_correctness="never"'
+            ' show_reset_button="false"'
+            ' showanswer="on_correct"'
+            ' submission_wait_seconds="10"'
+            ' use_latex_compiler="false"'
+            ' weight="1"'
+            '/>\n'
+        ))
+
         libs.publish_changes(self.library.key, self.user.id)
 
         self.taxonomy_all_org = tagging_api.create_taxonomy(
@@ -179,6 +201,100 @@ def test_sync_updates_happy_path(self):
         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, (
+            '<problem'
+            ' attempts_before_showanswer_button="10"'
+            ' display_name="Upstream Problem Title V3"'
+            ' due="2024-02-02T00:00:00Z"'
+            ' force_save_button="false"'
+            ' graceperiod=""'
+            ' grading_method="final_attempt"'
+            ' matlab_api_key="def"'
+            ' max_attempts="11"'
+            ' rerandomize="&quot;per_student&quot;"'
+            ' show_correctness="past_due"'
+            ' show_reset_button="false"'
+            ' showanswer="attempted"'
+            ' submission_wait_seconds="11"'
+            ' use_latex_compiler="true"'
+            ' weight="2"'
+            '/>\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
@@ -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 @@ def get_customizable_fields(cls) -> dict[str, str]:
     #            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.