fix: the get_completion JS handler should not ignore FBE blocks

common/lib/xmodule/xmodule/js/spec/sequence/display_spec.js

@@ -12,9 +12,10 @@
             };
 
         beforeEach(function() {
+            var runtime = jasmine.createSpyObj('TestRuntime', ['handlerUrl']);
             loadFixtures('sequence.html');
             local.XBlock = window.XBlock = jasmine.createSpyObj('XBlock', ['initializeBlocks']);
-            this.sequence = new Sequence($('.xblock-student_view-sequential'));
+            this.sequence = new Sequence($('.xblock-student_view-sequential'), runtime);
         });
 
         afterEach(function() {

common/lib/xmodule/xmodule/js/src/sequence/display.js

@@ -5,7 +5,7 @@
     'use strict';
 
     this.Sequence = (function() {
-        function Sequence(element) {
+        function Sequence(element, runtime) {
             var self = this;
 
             this.removeBookmarkIconFromActiveNavItem = function(event) {
@@ -54,7 +54,8 @@
             this.sr_container = this.$('.sr-is-focusable');
             this.num_contents = this.contents.length;
             this.id = this.el.data('id');
-            this.ajaxUrl = this.el.data('ajax-url');
+            this.getCompletionUrl = runtime.handlerUrl(element, 'get_completion');
+            this.gotoPositionUrl = runtime.handlerUrl(element, 'goto_position');
             this.nextUrl = this.el.data('next-url');
             this.prevUrl = this.el.data('prev-url');
             this.savePosition = this.el.data('save-position');
@@ -227,7 +228,7 @@
         };
 
         Sequence.prototype.render = function(newPosition) {
-            var bookmarked, currentTab, modxFullUrl, sequenceLinks,
+            var bookmarked, currentTab, sequenceLinks,
                 self = this;
             if (this.position !== newPosition) {
                 if (this.position) {
@@ -236,10 +237,9 @@
                         this.update_completion(this.position);
                     }
                     if (this.savePosition) {
-                        modxFullUrl = '' + this.ajaxUrl + '/goto_position';
-                        $.postWithPrefix(modxFullUrl, {
+                        $.postWithPrefix(this.gotoPositionUrl, JSON.stringify({
                             position: newPosition
-                        });
+                        }));
                     }
                 }
 
@@ -414,13 +414,12 @@
 
         Sequence.prototype.update_completion = function(position) {
             var element = this.link_for(position);
-            var completionUrl = this.ajaxUrl + '/get_completion';
             var usageKey = element[0].attributes['data-id'].value;
             var completionIndicators = element.find('.check-circle');
             if (completionIndicators.length) {
-                $.postWithPrefix(completionUrl, {
+                $.postWithPrefix(this.getCompletionUrl, JSON.stringify({
                     usage_key: usageKey
-                }, function(data) {
+                }), function(data) {
                     if (data.complete === true) {
                         completionIndicators.removeClass('is-hidden');
                     }

common/lib/xmodule/xmodule/js/src/xmodule.js

@@ -78,7 +78,7 @@
         }
 
         try {
-            module = new window[moduleType](element);
+            module = new window[moduleType](element, runtime);
 
             if ($(element).hasClass('xmodule_edit')) {
                 $(document).trigger('XModule.loaded.edit', [element, module]);

common/lib/xmodule/xmodule/seq_module.py

@@ -308,50 +308,75 @@ def get_progress(self):
         progress = reduce(Progress.add_counts, progresses, None)
         return progress
 
-    def handle_ajax(self, dispatch, data, view=STUDENT_VIEW):  # TODO: bounds checking  # lint-amnesty, pylint: disable=arguments-differ
-        ''' get = request.POST instance '''
+    @XBlock.json_handler
+    def get_completion(self, data, _suffix=''):
+        """Returns whether the provided vertical is complete based off the 'usage_key' value in the incoming dict"""
+        return self._get_completion(data)
+    # This 'will_recheck_access' attribute is checked by the upper-level handler code, where it will avoid stripping
+    # inaccessible blocks from our tree. We don't want them stripped because 'get_completion' needs to know about FBE
+    # blocks even if the user can't complete them, otherwise it might accidentally say a vertical is complete when
+    # there are still incomplete but access-locked blocks left.
+    get_completion.will_recheck_access = True
+
+    def _get_completion(self, data):
+        """Returns whether the provided vertical is complete based off the 'usage_key' value in the incoming dict"""
+        complete = False
+        usage_key = data.get('usage_key', None)
+        if usage_key:
+            item = self.get_child(UsageKey.from_string(usage_key))
+            if item:
+                completion_service = self.runtime.service(self, 'completion')
+                complete = completion_service.vertical_is_complete(item)
+        return {'complete': complete}
+
+    @XBlock.json_handler
+    def goto_position(self, data, _suffix=''):
+        """Sets the xblock position based off the 'position' value in the incoming dict"""
+        return self._goto_position(data)
+
+    def _goto_position(self, data):
+        """Sets the xblock position based off the 'position' value in the incoming dict"""
+        # set position to default value if either 'position' argument not
+        # found in request or it is a non-positive integer
+        position = data.get('position', 1)
+        if isinstance(position, int) and position > 0:
+            self.position = position
+        else:
+            self.position = 1
+        return {'success': True}
+
+    # If you are reading this and it's past the 'Maple' Open edX release, you can delete this handle_ajax method, as
+    # these are now individual xblock-style handler methods. We want to keep these around for a single release, simply
+    # to handle learners that haven't refreshed their courseware page when the server gets updated and their old
+    # javascript calls these old handlers.
+    # If you do clean this up, you can also move the internal private versions just directly into the handler methods,
+    # as nothing else calls them (at time of writing).
+    def handle_ajax(self, dispatch, data):
+        """Old xmodule-style ajax handler"""
         if dispatch == 'goto_position':
-            # set position to default value if either 'position' argument not
-            # found in request or it is a non-positive integer
-            position = data.get('position', '1')
-            if position.isdigit() and int(position) > 0:
-                self.position = int(position)
-            else:
-                self.position = 1
-            return json.dumps({'success': True})
+            return json.dumps(self._goto_position(data))
+        elif dispatch == 'get_completion':
+            return json.dumps(self._get_completion(data))
+        raise NotFoundError('Unexpected dispatch type')
 
-        if dispatch == 'get_completion':
-            completion_service = self.runtime.service(self, 'completion')
+    def get_metadata(self, view=STUDENT_VIEW):
+        """Returns a dict of some common block properties"""
+        context = {'exclude_units': True}
+        prereq_met = True
+        prereq_meta_info = {}
+        banner_text = None
+        display_items = self.get_display_items()
 
-            usage_key = data.get('usage_key', None)
-            if not usage_key:
-                return None
-            item = self.get_child(UsageKey.from_string(usage_key))
-            if not item:
-                return None
-
-            complete = completion_service.vertical_is_complete(item)
-            return json.dumps({
-                'complete': complete
-            })
-        elif dispatch == 'metadata':
-            context = {'exclude_units': True}
-            prereq_met = True
-            prereq_meta_info = {}
-            banner_text = None
-            display_items = self.get_display_items()
-
-            if self._required_prereq():
-                if self.runtime.user_is_staff:
-                    banner_text = _('This subsection is unlocked for learners when they meet the prerequisite requirements.')  # lint-amnesty, pylint: disable=line-too-long
-                else:
-                    # check if prerequisite has been met
-                    prereq_met, prereq_meta_info = self._compute_is_prereq_met(True)
-            meta = self._get_render_metadata(context, display_items, prereq_met, prereq_meta_info, banner_text, view)
-            meta['display_name'] = self.display_name_with_default
-            meta['format'] = getattr(self, 'format', '')
-            return json.dumps(meta)
-        raise NotFoundError('Unexpected dispatch type')
+        if self._required_prereq():
+            if self.runtime.user_is_staff:
+                banner_text = _('This subsection is unlocked for learners when they meet the prerequisite requirements.')  # lint-amnesty, pylint: disable=line-too-long
+            else:
+                # check if prerequisite has been met
+                prereq_met, prereq_meta_info = self._compute_is_prereq_met(True)
+        meta = self._get_render_metadata(context, display_items, prereq_met, prereq_meta_info, banner_text, view)
+        meta['display_name'] = self.display_name_with_default
+        meta['format'] = getattr(self, 'format', '')
+        return meta
 
     @classmethod
     def verify_current_content_visibility(cls, date, hide_after_date):
@@ -518,7 +543,6 @@ def _get_render_metadata(self, context, display_items, prereq_met, prereq_meta_i
             'is_time_limited': self.is_time_limited,
             'position': self.position,
             'tag': self.location.block_type,
-            'ajax_url': self.ajax_url,
             'next_url': context.get('next_url'),
             'prev_url': context.get('prev_url'),
             'banner_text': banner_text,

common/lib/xmodule/xmodule/tests/test_sequence.py

@@ -10,6 +10,7 @@
 from unittest.mock import Mock, patch
 
 import ddt
+from django.test import RequestFactory
 from django.test.utils import override_settings
 from django.utils.timezone import now
 from freezegun import freeze_time
@@ -369,57 +370,83 @@ def test_gated_content(self):
         # assert content shown as normal
         self._assert_ungated(html, self.sequence_1_2)
 
-    def test_handle_ajax_get_completion_success(self):
-        """
-        Test that the completion data is returned successfully on
-        targeted vertical through ajax call
-        """
+    def test_xblock_handler_get_completion_success(self):
+        """Test that the completion data is returned successfully on targeted vertical through ajax call"""
         for child in self.sequence_3_1.get_children():
             usage_key = str(child.location)
-            completion_return = json.loads(self.sequence_3_1.handle_ajax(
-                'get_completion',
-                {'usage_key': usage_key}
-            ))
-            assert completion_return is not None
-            assert 'complete' in completion_return
-            assert completion_return['complete'] is True
-
-    def test_handle_ajax_get_completion_return_none(self):
-        """
-        Test that the completion data is returned successfully None
-        when usage key is None through ajax call
-        """
-        usage_key = None
-        completion_return = self.sequence_3_1.handle_ajax(
-            'get_completion',
-            {'usage_key': usage_key}
+            request = RequestFactory().post(
+                '/',
+                data=json.dumps({'usage_key': usage_key}),
+                content_type='application/json',
+            )
+            completion_return = self.sequence_3_1.handle('get_completion', request)
+            assert completion_return.json == {'complete': True}
+
+    def test_xblock_handler_get_completion_bad_key(self):
+        """Test that the completion data is returned as False when usage key is None through ajax call"""
+        request = RequestFactory().post(
+            '/',
+            data=json.dumps({'usage_key': None}),
+            content_type='application/json',
         )
-        assert completion_return is None
+        completion_return = self.sequence_3_1.handle('get_completion', request)
+        assert completion_return.json == {'complete': False}
 
-    def test_handle_ajax_metadata(self):
-        """
-        Test that the sequence metadata is returned from the
-        metadata ajax handler.
-        """
+    def test_handle_ajax_get_completion_success(self):
+        """Test that the old-style ajax handler for completion still works"""
+        for child in self.sequence_3_1.get_children():
+            usage_key = str(child.location)
+            completion_return = self.sequence_3_1.handle_ajax('get_completion', {'usage_key': usage_key})
+            assert json.loads(completion_return) == {'complete': True}
+
+    def test_xblock_handler_goto_position_success(self):
+        """Test that we can set position through ajax call"""
+        assert self.sequence_3_1.position != 5
+        request = RequestFactory().post(
+            '/',
+            data=json.dumps({'position': 5}),
+            content_type='application/json',
+        )
+        goto_return = self.sequence_3_1.handle('goto_position', request)
+        assert goto_return.json == {'success': True}
+        assert self.sequence_3_1.position == 5
+
+    def test_xblock_handler_goto_position_bad_position(self):
+        """Test that we gracefully handle bad positions as position 1"""
+        assert self.sequence_3_1.position != 1
+        request = RequestFactory().post(
+            '/',
+            data=json.dumps({'position': -10}),
+            content_type='application/json',
+        )
+        goto_return = self.sequence_3_1.handle('goto_position', request)
+        assert goto_return.json == {'success': True}
+        assert self.sequence_3_1.position == 1
+
+    def test_handle_ajax_goto_position_success(self):
+        """Test that the old-style ajax handler for setting position still works"""
+        goto_return = self.sequence_3_1.handle_ajax('goto_position', {'position': 5})
+        assert json.loads(goto_return) == {'success': True}
+        assert self.sequence_3_1.position == 5
+
+    def test_get_metadata(self):
+        """Test that the sequence metadata is returned correctly"""
         # rather than dealing with json serialization of the Mock object,
         # let's just disable the bookmarks service
         self.sequence_3_1.xmodule_runtime._services['bookmarks'] = None  # lint-amnesty, pylint: disable=protected-access
-        metadata = json.loads(self.sequence_3_1.handle_ajax('metadata', {}))
+        metadata = self.sequence_3_1.get_metadata()
         assert len(metadata['items']) == 3
         assert metadata['tag'] == 'sequential'
         assert metadata['display_name'] == self.sequence_3_1.display_name_with_default
 
     @override_settings(FIELD_OVERRIDE_PROVIDERS=(
         'openedx.features.content_type_gating.field_override.ContentTypeGatingFieldOverride',
     ))
-    def test_handle_ajax_metadata_content_type_gated_content(self):
-        """
-        The contains_content_type_gated_content field should reflect
-        whether the given item contains content type gated content
-        """
+    def test_get_metadata_content_type_gated_content(self):
+        """The contains_content_type_gated_content field tells whether the item contains content type gated content"""
         self.sequence_5_1.xmodule_runtime._services['bookmarks'] = None  # pylint: disable=protected-access
         ContentTypeGatingConfig.objects.create(enabled=True, enabled_as_of=datetime(2018, 1, 1))
-        metadata = json.loads(self.sequence_5_1.handle_ajax('metadata', {}))
+        metadata = self.sequence_5_1.get_metadata()
         assert metadata['items'][0]['contains_content_type_gated_content'] is False
 
         # When a block contains content type gated problems, set the contains_content_type_gated_content field
@@ -428,7 +455,7 @@ def test_handle_ajax_metadata_content_type_gated_content(self):
             enabled_for_enrollment=Mock(return_value=True),
             content_type_gate_for_block=Mock(return_value=Fragment('i_am_gated'))
         ))
-        metadata = json.loads(self.sequence_5_1.handle_ajax('metadata', {}))
+        metadata = self.sequence_5_1.get_metadata()
         assert metadata['items'][0]['contains_content_type_gated_content'] is True
 
     def get_context_dict_from_string(self, data):