From b90039b23d63b51a5ea5cf3e76310c5189267610 Mon Sep 17 00:00:00 2001 From: Michael Terry Date: Mon, 26 Apr 2021 14:24:01 -0400 Subject: [PATCH] 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 --- .../xmodule/js/spec/sequence/display_spec.js | 3 +- .../xmodule/js/src/sequence/display.js | 17 ++- common/lib/xmodule/xmodule/js/src/xmodule.js | 2 +- common/lib/xmodule/xmodule/seq_module.py | 104 +++++++++++------- .../xmodule/xmodule/tests/test_sequence.py | 97 ++++++++++------ lms/djangoapps/courseware/module_render.py | 69 ++++++++---- .../courseware/tests/test_module_render.py | 25 +++++ lms/templates/seq_module.html | 2 +- .../core/djangoapps/courseware_api/views.py | 2 +- 9 files changed, 210 insertions(+), 111 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/spec/sequence/display_spec.js b/common/lib/xmodule/xmodule/js/spec/sequence/display_spec.js index 7413547b8e..a45d54a2a7 100644 --- a/common/lib/xmodule/xmodule/js/spec/sequence/display_spec.js +++ b/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() { diff --git a/common/lib/xmodule/xmodule/js/src/sequence/display.js b/common/lib/xmodule/xmodule/js/src/sequence/display.js index 1a283e55b8..1778fe84e3 100644 --- a/common/lib/xmodule/xmodule/js/src/sequence/display.js +++ b/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'); } diff --git a/common/lib/xmodule/xmodule/js/src/xmodule.js b/common/lib/xmodule/xmodule/js/src/xmodule.js index 329a7273c2..3dd386dfe9 100644 --- a/common/lib/xmodule/xmodule/js/src/xmodule.js +++ b/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]); diff --git a/common/lib/xmodule/xmodule/seq_module.py b/common/lib/xmodule/xmodule/seq_module.py index 618eefe62f..a42a8eff4b 100644 --- a/common/lib/xmodule/xmodule/seq_module.py +++ b/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, diff --git a/common/lib/xmodule/xmodule/tests/test_sequence.py b/common/lib/xmodule/xmodule/tests/test_sequence.py index 20c2b4eca1..9c96039800 100644 --- a/common/lib/xmodule/xmodule/tests/test_sequence.py +++ b/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): diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index c450c5e294..ac2de79d2e 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/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 diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 6a8d601221..8f7d8b70ec 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/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}) diff --git a/lms/templates/seq_module.html b/lms/templates/seq_module.html index 29d79b1104..7a7d6cadab 100644 --- a/lms/templates/seq_module.html +++ b/lms/templates/seq_module.html @@ -2,7 +2,7 @@ <%! from django.utils.translation import pgettext, ugettext as _ %>