feat: address PR comments

cms/djangoapps/contentstore/tests/test_utils.py

@@ -336,6 +336,60 @@ def test_no_inheritance_for_orphan(self):
         self.assertFalse(utils.ancestor_has_staff_lock(self.orphan))
 
 
+class InheritedOptionalCompletionTest(CourseTestCase):
+    """Tests for determining if an xblock inherits optional completion."""
+
+    def setUp(self):
+        super().setUp()
+        chapter = BlockFactory.create(category='chapter', parent=self.course)
+        sequential = BlockFactory.create(category='sequential', parent=chapter)
+        vertical = BlockFactory.create(category='vertical', parent=sequential)
+        html = BlockFactory.create(category='html', parent=vertical)
+        problem = BlockFactory.create(
+            category='problem', parent=vertical, data="<problem></problem>"
+        )
+        self.chapter = self.store.get_item(chapter.location)
+        self.sequential = self.store.get_item(sequential.location)
+        self.vertical = self.store.get_item(vertical.location)
+        self.html = self.store.get_item(html.location)
+        self.problem = self.store.get_item(problem.location)
+
+    def set_optional_completion(self, xblock, value):
+        """ Sets optional_completion to specified value and calls update_item to persist the change. """
+        xblock.optional_completion = value
+        self.store.update_item(xblock, self.user.id)
+
+    def update_optional_completions(self, chapter, sequential, vertical):
+        self.set_optional_completion(self.chapter, chapter)
+        self.set_optional_completion(self.sequential, sequential)
+        self.set_optional_completion(self.vertical, vertical)
+
+    def test_no_inheritance(self):
+        """Tests that an optional or non-optional vertical with no optional ancestors does not have an inherited optional completion"""
+        self.update_optional_completions(False, False, False)
+        self.assertFalse(utils.ancestor_has_optional_completion(self.vertical))
+        self.update_optional_completions(False, False, True)
+        self.assertFalse(utils.ancestor_has_optional_completion(self.vertical))
+
+    def test_inheritance_in_optional_section(self):
+        """Tests that a optional or non optional vertical in an optional section has an inherited optional completion"""
+        self.update_optional_completions(True, False, False)
+        self.assertTrue(utils.ancestor_has_optional_completion(self.vertical))
+        self.update_optional_completions(True, False, True)
+        self.assertTrue(utils.ancestor_has_optional_completion(self.vertical))
+
+    def test_inheritance_in_optional_subsection(self):
+        """Tests that a optional or non optional vertical in an optional subsection has an inherited optional completion"""
+        self.update_optional_completions(False, True, False)
+        self.assertTrue(utils.ancestor_has_optional_completion(self.vertical))
+        self.update_optional_completions(False, True, True)
+        self.assertTrue(utils.ancestor_has_optional_completion(self.vertical))
+
+    def test_no_inheritance_for_orphan(self):
+        """Tests that an orphaned xblock does not inherit optional completion"""
+        self.assertFalse(utils.ancestor_has_optional_completion(self.orphan))
+
+
 class GroupVisibilityTest(CourseTestCase):
     """
     Test content group access rules.

cms/djangoapps/contentstore/utils.py

@@ -354,6 +354,20 @@ def ancestor_has_staff_lock(xblock, parent_xblock=None):
     return parent_xblock.visible_to_staff_only
 
 
+def ancestor_has_optional_completion(xblock, parent_xblock=None):
+    """
+    Returns True iff one of xblock's ancestors has optional completion.
+    Can avoid mongo query by passing in parent_xblock.
+    """
+    if parent_xblock is None:
+        parent_location = modulestore().get_parent_location(xblock.location,
+                                                            revision=ModuleStoreEnum.RevisionOption.draft_preferred)
+        if not parent_location:
+            return False
+        parent_xblock = modulestore().get_item(parent_location)
+    return parent_xblock.optional_completion
+
+
 def reverse_url(handler_name, key_name=None, key_value=None, kwargs=None):
     """
     Creates the URL for the given handler.

cms/djangoapps/contentstore/views/block.py

@@ -60,6 +60,7 @@
 from xmodule.x_module import AUTHOR_VIEW, PREVIEW_VIEWS, STUDENT_VIEW, STUDIO_VIEW  # lint-amnesty, pylint: disable=wrong-import-order
 
 from ..utils import (
+    ancestor_has_optional_completion,
     ancestor_has_staff_lock,
     find_release_date_source,
     find_staff_lock_source,
@@ -1321,7 +1322,7 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F
             'group_access': xblock.group_access,
             'user_partitions': user_partitions,
             'show_correctness': xblock.show_correctness,
-            'optional_content': xblock.optional_content,
+            'optional_completion': xblock.optional_completion,
         })
 
         if xblock.category == 'sequential':
@@ -1406,6 +1407,11 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F
             xblock_info['ancestor_has_staff_lock'] = False
 
         if course_outline:
+            if xblock_info['optional_completion']:
+                xblock_info['ancestor_has_optional_completion'] = ancestor_has_optional_completion(xblock, parent_xblock)
+            else:
+                xblock_info['ancestor_has_optional_completion'] = False
+
             if xblock_info['has_explicit_staff_lock']:
                 xblock_info['staff_only_message'] = True
             elif child_info and child_info['children']:

cms/djangoapps/models/settings/course_metadata.py

@@ -78,7 +78,7 @@ class CourseMetadata:
         'highlights_enabled_for_messaging',
         'is_onboarding_exam',
         'discussions_settings',
-        'optional',
+        'optional_completion',
     ]
 
     @classmethod

cms/static/js/models/xblock_info.js

@@ -120,6 +120,10 @@ define(
              */
                 ancestor_has_staff_lock: null,
                 /**
+             * True if this any of this xblock's ancestors are optional for completion.
+             */
+                ancestor_has_optional_completion: null,
+                /**
              * The xblock which is determining the staff lock value. For instance, for a unit,
              * this will either be the parent subsection or the grandparent section.
              * This can be null if the xblock has no inherited staff lock. Will only be present if

cms/static/js/views/modals/course_outline_modals.js

@@ -18,7 +18,7 @@ define(['jquery', 'backbone', 'underscore', 'gettext', 'js/views/baseview',
         ReleaseDateEditor, DueDateEditor, SelfPacedDueDateEditor, GradingEditor, PublishEditor, AbstractVisibilityEditor,
         StaffLockEditor, UnitAccessEditor, ContentVisibilityEditor, TimedExaminationPreferenceEditor,
         AccessEditor, ShowCorrectnessEditor, HighlightsEditor, HighlightsEnableXBlockModal, HighlightsEnableEditor,
-        DiscussionEditor, OptionalContentEditor;
+        DiscussionEditor, OptionalCompletionEditor;
 
     CourseOutlineXBlockModal = BaseModal.extend({
         events: _.extend({}, BaseModal.prototype.events, {
@@ -1201,43 +1201,45 @@ define(['jquery', 'backbone', 'underscore', 'gettext', 'js/views/baseview',
         }
     });
 
-    OptionalContentEditor = AbstractEditor.extend(
+    OptionalCompletionEditor = AbstractEditor.extend(
     {
-        templateName: 'optional-content-editor',
-        className: 'edit-optional-content',
+        templateName: 'optional-completion-editor',
+        className: 'edit-optional-completion',
 
         afterRender: function() {
             AbstractEditor.prototype.afterRender.call(this);
-            this.setValue(this.model.get("optional_content"));
+            this.setValue(this.model.get("optional_completion"));
         },
 
         setValue: function(value) {
-            this.$('input[name=optional_content]').prop('checked', value);
+            this.$('input[name=optional_completion]').prop('checked', value);
         },
 
         currentValue: function() {
-            return this.$('input[name=optional_content]').is(':checked');
+            return this.$('input[name=optional_completion]').is(':checked');
         },
 
         hasChanges: function() {
-            return this.model.get('optional_content') !== this.currentValue();
+            return this.model.get('optional_completion') !== this.currentValue();
         },
 
         getRequestData: function() {
             if (this.hasChanges()) {
                 return {
                     publish: 'republish',
                     metadata: {
-                        optional_content: this.currentValue()
+                        optional_completion: this.currentValue()
                     }
                 };
             } else {
                 return {};
             }
         },
+
         getContext: function() {
             return {
-                optional_content: this.model.get('optional_content')
+                optional_completion: this.model.get('optional_completion'),
+                optionalAncestor: this.model.get('ancestor_has_optional_completion')
             };
         },
     })
@@ -1265,7 +1267,7 @@ define(['jquery', 'backbone', 'underscore', 'gettext', 'js/views/baseview',
                 editors: []
             };
             if (xblockInfo.isVertical()) {
-                editors = [StaffLockEditor, UnitAccessEditor, DiscussionEditor];
+                editors = [StaffLockEditor, UnitAccessEditor, DiscussionEditor, OptionalCompletionEditor];
             } else {
                 tabs = [
                     {
@@ -1280,10 +1282,10 @@ define(['jquery', 'backbone', 'underscore', 'gettext', 'js/views/baseview',
                     }
                 ];
                 if (xblockInfo.isChapter()) {
-                    tabs[0].editors = [ReleaseDateEditor, OptionalContentEditor];
+                    tabs[0].editors = [ReleaseDateEditor, OptionalCompletionEditor];
                     tabs[1].editors = [StaffLockEditor];
                 } else if (xblockInfo.isSequential()) {
-                    tabs[0].editors = [ReleaseDateEditor, GradingEditor, DueDateEditor, OptionalContentEditor];
+                    tabs[0].editors = [ReleaseDateEditor, GradingEditor, DueDateEditor, OptionalCompletionEditor];
                     tabs[1].editors = [ContentVisibilityEditor, ShowCorrectnessEditor];
                     if (course.get('self_paced') && course.get('is_custom_relative_dates_active')) {
                         tabs[0].editors.push(SelfPacedDueDateEditor);

cms/static/sass/elements/_modal-window.scss

@@ -747,7 +747,7 @@
 
     .edit-discussion,
     .edit-staff-lock,
-    .edit-optional-content,
+    .edit-optional-completion,
     .edit-content-visibility,
     .edit-unit-access {
       margin-bottom: $baseline;
@@ -761,20 +761,20 @@
     // UI: staff lock and discussion
     .edit-discussion,
     .edit-staff-lock,
-    .edit-optional-content,
+    .edit-optional-completion,
     .edit-settings-timed-examination,
     .edit-unit-access {
       .checkbox-cosmetic .input-checkbox {
         @extend %cont-text-sr;
 
         // CASE: unchecked
-        ~.tip-warning {
+        ~ .tip-warning {
           display: block;
         }
 
         // CASE: checked
         &:checked {
-          ~.tip-warning {
+          ~ .tip-warning {
             display: none;
           }
         }
@@ -832,9 +832,17 @@
     }
   }
 
+  .edit-optional-completion {
+    .field-message {
+      @extend %t-copy-sub1;
+      color: $gray-d1;
+      margin-bottom: ($baseline/4);
+    }
+  }
+
   .edit-discussion,
   .edit-unit-access,
-  .edit-optional-content,
+  .edit-optional-completion,
   .edit-staff-lock {
     .modal-section-content {
       @include font-size(16);
@@ -877,7 +885,7 @@
 
   .edit-discussion,
   .edit-unit-access,
-  .edit-optional-content,
+  .edit-optional-completion,
   .edit-staff-lock {
     .modal-section-content {
       @include font-size(16);

cms/templates/course_outline.html

@@ -29,7 +29,7 @@
 
 <%block name="header_extras">
 <link rel="stylesheet" type="text/css" href="${static.url('js/vendor/timepicker/jquery.timepicker.css')}" />
-% for template_name in ['course-outline', 'xblock-string-field-editor', 'basic-modal', 'modal-button', 'course-outline-modal', 'due-date-editor', 'self-paced-due-date-editor', 'release-date-editor', 'grading-editor', 'publish-editor', 'staff-lock-editor', 'unit-access-editor', 'discussion-editor', 'content-visibility-editor', 'verification-access-editor', 'timed-examination-preference-editor', 'access-editor', 'settings-modal-tabs', 'show-correctness-editor', 'highlights-editor', 'highlights-enable-editor', 'course-highlights-enable', 'optional-content-editor']:
+% for template_name in ['course-outline', 'xblock-string-field-editor', 'basic-modal', 'modal-button', 'course-outline-modal', 'due-date-editor', 'self-paced-due-date-editor', 'release-date-editor', 'grading-editor', 'publish-editor', 'staff-lock-editor', 'unit-access-editor', 'discussion-editor', 'content-visibility-editor', 'verification-access-editor', 'timed-examination-preference-editor', 'access-editor', 'settings-modal-tabs', 'show-correctness-editor', 'highlights-editor', 'highlights-enable-editor', 'course-highlights-enable', 'optional-completion-editor']:
 <script type="text/template" id="${template_name}-tpl">
     <%static:include path="js/${template_name}.underscore" />
 </script>

cms/templates/js/course-outline.underscore

@@ -225,7 +225,7 @@ if (is_proctored_exam) {
                             <% if (xblockInfo.get('release_date')) { %>
                                 <%- xblockInfo.get('release_date') %>
                             <% } %>
-                            <% if (xblockInfo.get('optional_content')) { %>
+                            <% if (xblockInfo.get('optional_completion')) { %>
                                 - <%- gettext('Optional') %>
                             <% } %>
                         <% } %>

cms/templates/js/optional-completion-editor.underscore

@@ -0,0 +1,26 @@
+<form>
+<h3 class="modal-section-title">
+    <%- gettext('Completion') %>
+</h3>
+<div class="modal-section-content optional-completion">
+    <label class="label">
+	    <% if (optionalAncestor) { %>
+		    <input disabled type="checkbox" id="optional_completion" name="optional_completion"
+		          class="input input-checkbox" />
+			<%- gettext('Optional') %>
+	        <p class="tip tip-warning">
+	            <% var message = gettext('This %(xblockType)s already has an optional parent.') %>
+	            <%- interpolate(message, { xblockType: xblockType }, true) %>
+	        </p>
+	    <% } else { %>
+		    <input type="checkbox" id="optional_completion" name="optional_completion"
+		          class="input input-checkbox" />
+			<%- gettext('Optional') %>
+          <% } %>
+        <p class="field-message">
+            <% var message = gettext('Optional %(xblockType)ss won\'t count towards course or parent completion. Only has an effect if completion module is enabled.') %>
+            <%- interpolate(message, { xblockType: xblockType }, true) %>
+        </p>
+	</label>
+</div>
+</form>

lms/djangoapps/course_api/blocks/serializers.py

@@ -56,7 +56,7 @@ def __init__(
     SupportedFieldType('has_score'),
     SupportedFieldType('has_scheduled_content'),
     SupportedFieldType('weight'),
-    SupportedFieldType('optional_content'),
+    SupportedFieldType('optional_completion'),
     SupportedFieldType('show_correctness'),
     # 'student_view_data'
     SupportedFieldType(StudentViewTransformer.STUDENT_VIEW_DATA, StudentViewTransformer),

lms/djangoapps/course_api/blocks/transformers/block_completion.py

@@ -14,7 +14,7 @@ class BlockCompletionTransformer(BlockStructureTransformer):
     Keep track of the completion of each block within the block structure.
     """
     READ_VERSION = 1
-    WRITE_VERSION = 1
+    WRITE_VERSION = 2
     COMPLETION = 'completion'
     COMPLETE = 'complete'
     RESUME_BLOCK = 'resume_block'
@@ -43,7 +43,7 @@ def get_block_completion(cls, block_structure, block_key):
 
     @classmethod
     def collect(cls, block_structure):
-        block_structure.request_xblock_fields('completion_mode', 'optional_content')
+        block_structure.request_xblock_fields('completion_mode', 'optional_completion')
 
     @staticmethod
     def _is_block_excluded(block_structure, block_key):

lms/djangoapps/course_home_api/outline/serializers.py

@@ -54,7 +54,7 @@ def get_blocks(self, block):  # pylint: disable=missing-function-docstring
                 'resume_block': block.get('resume_block', False),
                 'type': block_type,
                 'has_scheduled_content': block.get('has_scheduled_content'),
-                'optional_content': block.get('optional_content'),
+                'optional_completion': block.get('optional_completion', False),
             },
         }
         for child in children:

lms/djangoapps/course_home_api/outline/tests/test_view.py

@@ -438,17 +438,17 @@ def test_cannot_enroll_if_full(self):
         CourseEnrollment.enroll(UserFactory(), self.course.id)  # grr, some rando took our spot!
         self.assert_can_enroll(False)
 
-    def test_optional_content(self):
+    def test_optional_completion(self):
         CourseEnrollment.enroll(self.user, self.course.id)
-        assert not self.course.optional_content
+        assert not self.course.optional_completion
         response = self.client.get(self.url)
         for block in response.data['course_blocks']['blocks'].values():
-            assert not block['optional_content']
+            assert not block['optional_completion']
 
-    def test_optional_content_true(self):
-        self.course.optional_content = True
+    def test_optional_completion_true(self):
+        self.course.optional_completion = True
         self.update_course_and_overview()
         CourseEnrollment.enroll(self.user, self.course.id)
         response = self.client.get(self.url)
         for block in response.data['course_blocks']['blocks'].values():
-            assert block['optional_content']
+            assert block['optional_completion']

lms/djangoapps/course_home_api/progress/tests/test_views.py

@@ -315,7 +315,7 @@ def test_course_grade_considers_subsection_grade_visibility(self, is_staff, expe
         assert response.data['course_grade']['percent'] == expected_percent
         assert response.data['course_grade']['is_passing'] == (expected_percent >= 0.5)
 
-    def test_optional_content(self):
+    def test_optional_completion(self):
         CourseEnrollment.enroll(self.user, self.course.id)
         response = self.client.get(self.url)
         assert response.status_code == 200