fix: the get_completion JS handler should not ignore FBE blocks

Because xblock handlers normally get a block tree that already has
inaccessible blocks stripped, they don't see FBE gated blocks at
all. So the get_completion handler would return True for FBE units
incorrectly. Leading to a visual bug as an audit user went through
their units in the courseware.

In order to let the handler know about the full tree, I've added a
new attribute you can set on your xblock handlers:
my_handler.will_recheck_access = True

This will tell the top-level handler code get the full tree for you.

As part of this, I've also changed the sequence xblock handler's
into proper xblock handlers (not old-style xmodule handlers).
This changes their URLs slightly. I've kept the old URLs for now
as well, but they'll be removed after Maple.

AA-409

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,51 +308,76 @@ class SequenceBlock(
         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 '''
-        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})
+    @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
 
-        if dispatch == 'get_completion':
-            completion_service = self.runtime.service(self, 'completion')
-
-            usage_key = data.get('usage_key', None)
-            if not usage_key:
-                return None
+    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 not item:
-                return None
+            if item:
+                completion_service = self.runtime.service(self, 'completion')
+                complete = completion_service.vertical_is_complete(item)
+        return {'complete': complete}
 
-            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()
+    @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)
 
-            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)
+    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':
+            return json.dumps(self._goto_position(data))
+        elif dispatch == 'get_completion':
+            return json.dumps(self._get_completion(data))
         raise NotFoundError('Unexpected dispatch type')
 
+    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()
+
+        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 @@ class SequenceBlock(
             '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 datetime import datetime, timedelta
 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,42 +370,71 @@ class SequenceBlockTestCase(XModuleXmlImportTest):
         # 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
+            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_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}
+    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
@@ -412,14 +442,11 @@ class SequenceBlockTestCase(XModuleXmlImportTest):
     @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 @@ class SequenceBlockTestCase(XModuleXmlImportTest):
             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):

lms/djangoapps/courseware/module_render.py

@@ -1068,31 +1068,32 @@ def handle_xblock_callback(request, course_id, usage_id, handler, suffix=None):
         return _invoke_xblock_handler(request, course_id, usage_id, handler, suffix, course=course)
 
 
-def get_module_by_usage_id(request, course_id, usage_id, disable_staff_debug_info=False, course=None,
-                           will_recheck_access=False):
+def _get_usage_key_for_course(course_key, usage_id) -> UsageKey:
     """
-    Gets a module instance based on its `usage_id` in a course, for a given request/user
+    Returns UsageKey mapped into the course for a given usage_id string
+    """
+    try:
+        return UsageKey.from_string(unquote_slashes(usage_id)).map_into_course(course_key)
+    except InvalidKeyError as exc:
+        raise Http404("Invalid location") from exc
+
+
+def _get_descriptor_by_usage_key(usage_key):
+    """
+    Gets a descriptor instance based on a mapped-to-course usage_key
 
     Returns (instance, tracking_context)
     """
-    user = request.user
-
-    try:
-        course_id = CourseKey.from_string(course_id)
-        usage_key = UsageKey.from_string(unquote_slashes(usage_id)).map_into_course(course_id)
-    except InvalidKeyError:
-        raise Http404("Invalid location")  # lint-amnesty, pylint: disable=raise-missing-from
-
     try:
         descriptor = modulestore().get_item(usage_key)
         descriptor_orig_usage_key, descriptor_orig_version = modulestore().get_block_original_usage(usage_key)
-    except ItemNotFoundError:
+    except ItemNotFoundError as exc:
         log.warning(
             "Invalid location for course id %s: %s",
             usage_key.course_key,
             usage_key
         )
-        raise Http404  # lint-amnesty, pylint: disable=raise-missing-from
+        raise Http404 from exc
 
     tracking_context = {
         'module': {
@@ -1107,9 +1108,23 @@ def get_module_by_usage_id(request, course_id, usage_id, disable_staff_debug_inf
         tracking_context['module']['original_usage_key'] = str(descriptor_orig_usage_key)
         tracking_context['module']['original_usage_version'] = str(descriptor_orig_version)
 
-    unused_masquerade, user = setup_masquerade(request, course_id, has_access(user, 'staff', descriptor, course_id))
+    return descriptor, tracking_context
+
+
+def get_module_by_usage_id(request, course_id, usage_id, disable_staff_debug_info=False, course=None,
+                           will_recheck_access=False):
+    """
+    Gets a module instance based on its `usage_id` in a course, for a given request/user
+
+    Returns (instance, tracking_context)
+    """
+    course_key = CourseKey.from_string(course_id)
+    usage_key = _get_usage_key_for_course(course_key, usage_id)
+    descriptor, tracking_context = _get_descriptor_by_usage_key(usage_key)
+
+    _, user = setup_masquerade(request, course_key, has_access(request.user, 'staff', descriptor, course_key))
     field_data_cache = FieldDataCache.cache_for_descriptor_descendents(
-        course_id,
+        course_key,
         user,
         descriptor,
         read_only=CrawlersConfig.is_crawler(request),
@@ -1130,7 +1145,7 @@ def get_module_by_usage_id(request, course_id, usage_id, disable_staff_debug_inf
         log.debug("No module %s for user %s -- access denied?", usage_key, user)
         raise Http404
 
-    return (instance, tracking_context)
+    return instance, tracking_context
 
 
 def _invoke_xblock_handler(request, course_id, usage_id, handler, suffix, course=None):
@@ -1154,23 +1169,31 @@ def _invoke_xblock_handler(request, course_id, usage_id, handler, suffix, course
     # Make a CourseKey from the course_id, raising a 404 upon parse error.
     try:
         course_key = CourseKey.from_string(course_id)
-    except InvalidKeyError:
-        raise Http404  # lint-amnesty, pylint: disable=raise-missing-from
+    except InvalidKeyError as exc:
+        raise Http404 from exc
 
     set_custom_attributes_for_course_key(course_key)
 
     with modulestore().bulk_operations(course_key):
-        try:
-            usage_key = UsageKey.from_string(unquote_slashes(usage_id))
-        except InvalidKeyError:
-            raise Http404  # lint-amnesty, pylint: disable=raise-missing-from
+        usage_key = _get_usage_key_for_course(course_key, usage_id)
         if is_xblock_aside(usage_key):
             # Get the usage key for the block being wrapped by the aside (not the aside itself)
             block_usage_key = usage_key.usage_key
         else:
             block_usage_key = usage_key
+
+        # Peek at the handler method to see if it actually wants to check access itself. (The handler may not want
+        # inaccessible blocks stripped from the tree.) This ends up doing two modulestore lookups for the descriptor,
+        # but the blocks should be available in the request cache the second time.
+        # At the time of writing, this is only used by one handler. If this usage grows, we may want to re-evaluate
+        # how we do this to something more elegant. If you are the author of a third party block that decides it wants
+        # to set this too, please let us know so we can consider making this easier / better-documented.
+        descriptor, _ = _get_descriptor_by_usage_key(block_usage_key)
+        handler_method = getattr(descriptor, handler, False)
+        will_recheck_access = handler_method and getattr(handler_method, 'will_recheck_access', False)
+
         instance, tracking_context = get_module_by_usage_id(
-            request, course_id, str(block_usage_key), course=course
+            request, course_id, str(block_usage_key), course=course, will_recheck_access=will_recheck_access,
         )
 
         # Name the transaction so that we can view XBlock handlers separately in

lms/djangoapps/courseware/tests/test_module_render.py

@@ -897,6 +897,31 @@ class TestHandleXBlockCallback(SharedModuleStoreTestCase, LoginEnrollmentTestCas
         )
         assert not mock_score_signal.called
 
+    @ddt.data(
+        # See seq_module.py for the definition of these handlers
+        ('get_completion', True),  # has the 'will_recheck_access' attribute set to True
+        ('goto_position', False),  # does not set it
+    )
+    @ddt.unpack
+    @patch('lms.djangoapps.courseware.module_render.get_module_for_descriptor', wraps=get_module_for_descriptor)
+    def test_will_recheck_access_handler_attribute(self, handler, will_recheck_access, mock_get_module):
+        """Confirm that we pay attention to any 'will_recheck_access' attributes on handler methods"""
+        course = CourseFactory.create()
+        descriptor_kwargs = {
+            'category': 'sequential',
+            'parent': course,
+        }
+        descriptor = ItemFactory.create(**descriptor_kwargs)
+        usage_id = str(descriptor.location)
+
+        # Send no special parameters, which will be invalid, but we don't care
+        request = self.request_factory.post('/', data='{}', content_type='application/json')
+        request.user = self.mock_user
+
+        render.handle_xblock_callback(request, str(course.id), usage_id, handler)
+        assert mock_get_module.call_count == 1
+        assert mock_get_module.call_args[1]['will_recheck_access'] == will_recheck_access
+
 
 @ddt.ddt
 @patch.dict('django.conf.settings.FEATURES', {'ENABLE_XBLOCK_VIEW_ENDPOINT': True})

lms/templates/seq_module.html

@@ -2,7 +2,7 @@
 <%! from django.utils.translation import pgettext, ugettext as _ %>
 
 <div id="sequence_${element_id}" class="sequence" data-id="${item_id}"
-     data-position="${position}" data-ajax-url="${ajax_url}"
+     data-position="${position}"
      data-next-url="${next_url}" data-prev-url="${prev_url}"
      data-save-position="${'true' if save_position else 'false'}"
      data-show-completion="${'true' if show_completion else 'false'}"

openedx/core/djangoapps/courseware_api/views.py

@@ -514,7 +514,7 @@ class SequenceMetadata(DeveloperErrorViewMixin, APIView):
         if request.user.is_anonymous:
             view = PUBLIC_VIEW
 
-        return Response(json.loads(sequence.handle_ajax('metadata', None, view=view)))
+        return Response(sequence.get_metadata(view=view))
 
 
 class Resume(DeveloperErrorViewMixin, APIView):