From 2ef69d513fd3f5785a80c11d0403f71cda225b5c Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 2 Dec 2021 11:32:45 +1030 Subject: [PATCH] refactor: removes xqueue_callback_url_prefix argument from the lms get_module_for_descriptor and related methods. This prefix was previously inferred from the request's base URL and passed through several methods to reach the XQueueService creation. But the request base URL always matches the LMS_ROOT_URL (or the preview URL), so this change simply uses that setting instead. --- lms/djangoapps/courseware/module_render.py | 24 +--------- .../tests/test_discussion_xblock.py | 4 -- .../courseware/tests/test_module_render.py | 48 +++++++++++++------ lms/djangoapps/instructor_task/api_helper.py | 7 +-- .../tasks_helper/module_state.py | 4 -- .../instructor_task/tests/test_tasks.py | 1 - 6 files changed, 38 insertions(+), 50 deletions(-) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index d3ef4d5ea7..828419d55a 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -364,21 +364,6 @@ def display_access_messages(user, block, view, frag, context): # pylint: disabl return msg_fragment -def get_xqueue_callback_url_prefix(request): - """ - Calculates default prefix based on request, but allows override via settings - - This is separated from get_module_for_descriptor so that it can be called - by the LMS before submitting background tasks to run. The xqueue callbacks - should go back to the LMS, not to the worker. - """ - prefix = '{proto}://{host}'.format( - proto=request.META.get('HTTP_X_FORWARDED_PROTO', 'https' if request.is_secure() else 'http'), - host=request.get_host() - ) - return settings.XQUEUE_INTERFACE.get('callback_url', prefix) - - # pylint: disable=too-many-statements def get_module_for_descriptor(user, request, descriptor, field_data_cache, course_key, position=None, wrap_xmodule_display=True, grade_bucket_type=None, @@ -392,7 +377,6 @@ def get_module_for_descriptor(user, request, descriptor, field_data_cache, cours See get_module() docstring for further details. """ track_function = make_track_function(request) - xqueue_callback_url_prefix = get_xqueue_callback_url_prefix(request) user_location = getattr(request, 'session', {}).get('country_code') @@ -407,7 +391,6 @@ def get_module_for_descriptor(user, request, descriptor, field_data_cache, cours student_data=student_data, course_id=course_key, track_function=track_function, - xqueue_callback_url_prefix=xqueue_callback_url_prefix, position=position, wrap_xmodule_display=wrap_xmodule_display, grade_bucket_type=grade_bucket_type, @@ -427,7 +410,6 @@ def get_module_system_for_user( descriptor, course_id, track_function, - xqueue_callback_url_prefix, request_token, position=None, wrap_xmodule_display=True, @@ -471,6 +453,7 @@ def get_module_system_for_user( dispatch=dispatch ), ) + xqueue_callback_url_prefix = settings.XQUEUE_INTERFACE.get('callback_url', settings.LMS_ROOT_URL) return xqueue_callback_url_prefix + relative_xqueue_callback_url # Default queuename is course-specific and is derived from the course that @@ -501,7 +484,6 @@ def get_module_system_for_user( student_data=student_data, course_id=course_id, track_function=track_function, - xqueue_callback_url_prefix=xqueue_callback_url_prefix, position=position, wrap_xmodule_display=wrap_xmodule_display, grade_bucket_type=grade_bucket_type, @@ -660,7 +642,6 @@ def get_module_system_for_user( descriptor=module, course_id=course_id, track_function=track_function, - xqueue_callback_url_prefix=xqueue_callback_url_prefix, position=position, wrap_xmodule_display=wrap_xmodule_display, grade_bucket_type=grade_bucket_type, @@ -845,7 +826,7 @@ def get_module_system_for_user( # TODO: Find all the places that this method is called and figure out how to # get a loaded course passed into it def get_module_for_descriptor_internal(user, descriptor, student_data, course_id, - track_function, xqueue_callback_url_prefix, request_token, + track_function, request_token, position=None, wrap_xmodule_display=True, grade_bucket_type=None, static_asset_path='', user_location=None, disable_staff_debug_info=False, course=None, will_recheck_access=False): @@ -864,7 +845,6 @@ def get_module_for_descriptor_internal(user, descriptor, student_data, course_id descriptor=descriptor, course_id=course_id, track_function=track_function, - xqueue_callback_url_prefix=xqueue_callback_url_prefix, position=position, wrap_xmodule_display=wrap_xmodule_display, grade_bucket_type=grade_bucket_type, diff --git a/lms/djangoapps/courseware/tests/test_discussion_xblock.py b/lms/djangoapps/courseware/tests/test_discussion_xblock.py index 6d6420a9df..385ec26fef 100644 --- a/lms/djangoapps/courseware/tests/test_discussion_xblock.py +++ b/lms/djangoapps/courseware/tests/test_discussion_xblock.py @@ -300,7 +300,6 @@ class TestXBlockInCourse(SharedModuleStoreTestCase): student_data=mock.Mock(name='student_data'), course_id=self.course.id, track_function=mock.Mock(name='track_function'), - xqueue_callback_url_prefix=mock.Mock(name='xqueue_callback_url_prefix'), request_token='request_token', ) @@ -321,7 +320,6 @@ class TestXBlockInCourse(SharedModuleStoreTestCase): student_data=mock.Mock(name='student_data'), course_id=self.course.id, track_function=mock.Mock(name='track_function'), - xqueue_callback_url_prefix=mock.Mock(name='xqueue_callback_url_prefix'), request_token='request_token', ) @@ -375,7 +373,6 @@ class TestXBlockInCourse(SharedModuleStoreTestCase): student_data=mock.Mock(name='student_data'), course_id=self.course.id, track_function=mock.Mock(name='track_function'), - xqueue_callback_url_prefix=mock.Mock(name='xqueue_callback_url_prefix'), request_token='request_token', ) @@ -447,7 +444,6 @@ class TestXBlockQueryLoad(SharedModuleStoreTestCase): student_data=mock.Mock(name='student_data'), course_id=course.id, track_function=mock.Mock(name='track_function'), - xqueue_callback_url_prefix=mock.Mock(name='xqueue_callback_url_prefix'), request_token='request_token', ) with self.assertNumQueries(num_queries): diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 3e138e27f9..7d2cd0b70e 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -1931,7 +1931,6 @@ class TestAnonymousStudentId(SharedModuleStoreTestCase, LoginEnrollmentTestCase) student_data=Mock(spec=FieldData, name='student_data'), course_id=course_id, track_function=Mock(name='track_function'), # Track Function - xqueue_callback_url_prefix=Mock(name='xqueue_callback_url_prefix'), # XQueue Callback Url Prefix request_token='request_token', course=self.course, ) @@ -2291,7 +2290,6 @@ class LMSXBlockServiceBindingTest(SharedModuleStoreTestCase): self.user = UserFactory() self.student_data = Mock() self.track_function = Mock() - self.xqueue_callback_url_prefix = Mock() self.request_token = Mock() @XBlock.register_temp_plugin(PureXBlock, identifier='pure') @@ -2307,7 +2305,6 @@ class LMSXBlockServiceBindingTest(SharedModuleStoreTestCase): descriptor, self.course.id, self.track_function, - self.xqueue_callback_url_prefix, self.request_token, course=self.course ) @@ -2326,7 +2323,6 @@ class LMSXBlockServiceBindingTest(SharedModuleStoreTestCase): descriptor, self.course.id, self.track_function, - self.xqueue_callback_url_prefix, self.request_token, course=self.course ) @@ -2589,7 +2585,6 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): self.user = UserFactory(id=232) self.student_data = Mock() self.track_function = Mock() - self.xqueue_callback_url_prefix = 'https://lms.url' self.request_token = Mock() @ddt.data( @@ -2608,7 +2603,6 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): self.descriptor, self.course.id, self.track_function, - self.xqueue_callback_url_prefix, self.request_token, course=self.course, ) @@ -2622,7 +2616,6 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): self.descriptor, self.course.id, self.track_function, - self.xqueue_callback_url_prefix, self.request_token, course=self.course, ) @@ -2650,7 +2643,6 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): self.descriptor, self.course.id, self.track_function, - self.xqueue_callback_url_prefix, self.request_token, course=self.course, ) @@ -2667,7 +2659,6 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): self.problem_descriptor, self.course.id, self.track_function, - self.xqueue_callback_url_prefix, self.request_token, course=self.course, ) @@ -2680,7 +2671,6 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): self.descriptor, self.course.id, self.track_function, - self.xqueue_callback_url_prefix, self.request_token, course=self.course, ) @@ -2697,7 +2687,6 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): self.descriptor, self.course.id, self.track_function, - self.xqueue_callback_url_prefix, self.request_token, course=self.course, ) @@ -2734,7 +2723,6 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): self.descriptor, self.course.id, self.track_function, - self.xqueue_callback_url_prefix, self.request_token, course=self.course, ) @@ -2748,14 +2736,46 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): self.descriptor, self.course.id, self.track_function, - self.xqueue_callback_url_prefix, self.request_token, course=self.course, ) xqueue = runtime.xqueue assert isinstance(xqueue['interface'], XQueueInterface) + assert xqueue['interface'].url == 'http://sandbox-xqueue.edx.org' assert xqueue['default_queuename'] == 'edX-LmsModuleShimTest' assert xqueue['waittime'] == 5 - callback_url = f'https://lms.url/courses/edX/LmsModuleShimTest/2021_Fall/xqueue/232/{self.descriptor.location}' + callback_url = ('http://localhost:8000/courses/edX/LmsModuleShimTest/2021_Fall/xqueue/232/' + + str(self.descriptor.location)) + assert xqueue['construct_callback']() == f'{callback_url}/score_update' + assert xqueue['construct_callback']('mock_dispatch') == f'{callback_url}/mock_dispatch' + + @override_settings( + XQUEUE_INTERFACE={ + 'callback_url': 'http://alt.url', + 'url': 'http://xqueue.url', + 'django_auth': { + 'username': 'user', + 'password': 'password', + }, + 'basic_auth': ('basic', 'auth'), + }, + XQUEUE_WAITTIME_BETWEEN_REQUESTS=15, + ) + def test_xqueue_settings(self): + runtime, _ = render.get_module_system_for_user( + self.user, + self.student_data, + self.descriptor, + self.course.id, + self.track_function, + self.request_token, + course=self.course, + ) + xqueue = runtime.xqueue + assert isinstance(xqueue['interface'], XQueueInterface) + assert xqueue['interface'].url == 'http://xqueue.url' + assert xqueue['default_queuename'] == 'edX-LmsModuleShimTest' + assert xqueue['waittime'] == 15 + callback_url = f'http://alt.url/courses/edX/LmsModuleShimTest/2021_Fall/xqueue/232/{self.descriptor.location}' assert xqueue['construct_callback']() == f'{callback_url}/score_update' assert xqueue['construct_callback']('mock_dispatch') == f'{callback_url}/mock_dispatch' diff --git a/lms/djangoapps/instructor_task/api_helper.py b/lms/djangoapps/instructor_task/api_helper.py index 54345f1821..dee85ef0f2 100644 --- a/lms/djangoapps/instructor_task/api_helper.py +++ b/lms/djangoapps/instructor_task/api_helper.py @@ -17,7 +17,6 @@ from opaque_keys.edx.keys import UsageKey from common.djangoapps.util.db import outer_atomic from lms.djangoapps.courseware.courses import get_problems_in_section -from lms.djangoapps.courseware.module_render import get_xqueue_callback_url_prefix from lms.djangoapps.instructor_task.models import PROGRESS, InstructorTask from xmodule.modulestore.django import modulestore @@ -131,8 +130,7 @@ def _get_xmodule_instance_args(request, task_id): Calculate parameters needed for instantiating xmodule instances. The `request_info` will be passed to a tracking log function, to provide information - about the source of the task request. The `xqueue_callback_url_prefix` is used to - permit old-style xqueue callbacks directly to the appropriate module in the LMS. + about the source of the task request. The `task_id` is also passed to the tracking log function. """ request_info = {'username': request.user.username, @@ -142,8 +140,7 @@ def _get_xmodule_instance_args(request, task_id): 'host': request.META['SERVER_NAME'], } - xmodule_instance_args = {'xqueue_callback_url_prefix': get_xqueue_callback_url_prefix(request), - 'request_info': request_info, + xmodule_instance_args = {'request_info': request_info, 'task_id': task_id, } return xmodule_instance_args diff --git a/lms/djangoapps/instructor_task/tasks_helper/module_state.py b/lms/djangoapps/instructor_task/tasks_helper/module_state.py index 73c94af40b..5a63836089 100644 --- a/lms/djangoapps/instructor_task/tasks_helper/module_state.py +++ b/lms/djangoapps/instructor_task/tasks_helper/module_state.py @@ -357,16 +357,12 @@ def _get_module_instance_for_task(course_id, student, module_descriptor, xmodule ''' return lambda event_type, event: task_track(request_info, task_info, event_type, event, page='x_module_task') - xqueue_callback_url_prefix = xmodule_instance_args.get('xqueue_callback_url_prefix', '') \ - if xmodule_instance_args is not None else '' - return get_module_for_descriptor_internal( user=student, descriptor=module_descriptor, student_data=student_data, course_id=course_id, track_function=make_track_function(), - xqueue_callback_url_prefix=xqueue_callback_url_prefix, grade_bucket_type=grade_bucket_type, # This module isn't being used for front-end rendering request_token=None, diff --git a/lms/djangoapps/instructor_task/tests/test_tasks.py b/lms/djangoapps/instructor_task/tests/test_tasks.py index ead71b5c7a..589a4d6316 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks.py @@ -83,7 +83,6 @@ class TestInstructorTasks(InstructorTaskModuleTestCase): Calculate dummy values for parameters needed for instantiating xmodule instances. """ return { - 'xqueue_callback_url_prefix': 'dummy_value', 'request_info': { 'username': 'dummy_username', 'user_id': 'dummy_id',