From 1974bacaddc6f884c3949ff23de966a01a01f917 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 10 Nov 2021 18:13:53 +1030 Subject: [PATCH] refactor: deprecates runtime.xqueue in favor of the XQueueService * Deprecates ModuleSystem.xqueue property * Adds new XQueueService to provide the deprecated property values to the LMS runtime (Studio does not need the XQueueService.) * Adds tests for new service and updates the ModuleSystemShim tests in LMS and Studio * Fixes existing tests. --- .../contentstore/views/tests/test_preview.py | 10 ++++ cms/envs/common.py | 1 + .../capa/capa/tests/test_xqueue_interface.py | 41 +++++++++++++ common/lib/capa/capa/xqueue_interface.py | 57 +++++++++++++++++-- common/lib/xmodule/xmodule/capa_module.py | 2 + common/lib/xmodule/xmodule/tests/__init__.py | 16 +++--- .../xmodule/xmodule/tests/test_capa_module.py | 35 ++++++------ common/lib/xmodule/xmodule/x_module.py | 34 ++++++++--- lms/djangoapps/courseware/module_render.py | 30 ++++------ .../courseware/tests/test_module_render.py | 26 ++++++++- .../tests/test_submitting_problems.py | 17 +++--- 11 files changed, 201 insertions(+), 68 deletions(-) create mode 100644 common/lib/capa/capa/tests/test_xqueue_interface.py diff --git a/cms/djangoapps/contentstore/views/tests/test_preview.py b/cms/djangoapps/contentstore/views/tests/test_preview.py index 91b77523b8..eee4b7b158 100644 --- a/cms/djangoapps/contentstore/views/tests/test_preview.py +++ b/cms/djangoapps/contentstore/views/tests/test_preview.py @@ -246,3 +246,13 @@ class CmsModuleSystemShimTest(ModuleStoreTestCase): descriptor = ItemFactory(category="pure", parent=self.course) html = get_preview_fragment(self.request, descriptor, {'element_id': 142}).content assert '
Testing the MakoService
' in html + + def test_xqueue_is_not_available_in_studio(self): + descriptor = ItemFactory(category="problem", parent=self.course) + runtime = _preview_module_system( + self.request, + descriptor=descriptor, + field_data=mock.Mock(), + ) + assert runtime.xqueue is None + assert runtime.service(descriptor, 'xqueue') is None diff --git a/cms/envs/common.py b/cms/envs/common.py index 23e3073c00..6f4ab636a4 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -725,6 +725,7 @@ CROSS_DOMAIN_CSRF_COOKIE_NAME = '' CSRF_TRUSTED_ORIGINS = [] #################### CAPA External Code Evaluation ############################# +XQUEUE_WAITTIME_BETWEEN_REQUESTS = 5 # seconds XQUEUE_INTERFACE = { 'url': 'http://localhost:18040', 'basic_auth': ['edx', 'edx'], diff --git a/common/lib/capa/capa/tests/test_xqueue_interface.py b/common/lib/capa/capa/tests/test_xqueue_interface.py new file mode 100644 index 0000000000..77cd6ccad0 --- /dev/null +++ b/common/lib/capa/capa/tests/test_xqueue_interface.py @@ -0,0 +1,41 @@ +# -*- coding: utf-8 -*- +""" +Tests the xqueue service interface. +""" + +from unittest import TestCase +from django.conf import settings + +from capa.xqueue_interface import XQueueInterface, XQueueService + + +class XQueueServiceTest(TestCase): + """ + Tests the XQueue service methods. + """ + @staticmethod + def construct_callback(*args, **kwargs): + return 'https://lms.url/callback' + + def setUp(self): + super().setUp() + self.service = XQueueService( + url=settings.XQUEUE_INTERFACE['url'], + django_auth=settings.XQUEUE_INTERFACE['django_auth'], + basic_auth=settings.XQUEUE_INTERFACE['basic_auth'], + construct_callback=self.construct_callback, + default_queuename='my-very-own-queue', + waittime=settings.XQUEUE_WAITTIME_BETWEEN_REQUESTS, + ) + + def test_interface(self): + assert isinstance(self.service.interface, XQueueInterface) + + def test_construct_callback(self): + assert self.service.construct_callback() == 'https://lms.url/callback' + + def test_default_queuename(self): + assert self.service.default_queuename == 'my-very-own-queue' + + def test_waittime(self): + assert self.service.waittime == 5 diff --git a/common/lib/capa/capa/xqueue_interface.py b/common/lib/capa/capa/xqueue_interface.py index 3deff6a3dd..f035d12201 100644 --- a/common/lib/capa/capa/xqueue_interface.py +++ b/common/lib/capa/capa/xqueue_interface.py @@ -1,7 +1,6 @@ -# lint-amnesty, pylint: disable=missing-module-docstring -# LMS Interface to external queueing system (xqueue) -# - +""" +LMS Interface to external queueing system (xqueue) +""" import hashlib import json @@ -149,3 +148,53 @@ class XQueueInterface(object): return 1, 'unexpected HTTP status code [%d]' % response.status_code return parse_xreply(response.text) + + +class XQueueService: + """ + XBlock service providing an interface to the XQueue service. + + Args: + construct_callback(callable): function which constructs a fully-qualified callback URL to make xqueue requests. + default_queuename(string): course-specific queue name. + waittime(int): number of seconds to wait between xqueue requests + url(string): base URL for the XQueue service. + django_auth(dict): username and password for the XQueue service. + basic_auth(array or None): basic authentication credentials, if needed. + """ + def __init__(self, construct_callback, default_queuename, waittime, url, django_auth, basic_auth=None): + + requests_auth = requests.auth.HTTPBasicAuth(*basic_auth) if basic_auth else None + self._interface = XQueueInterface(url, django_auth, requests_auth) + + self._construct_callback = construct_callback + self._default_queuename = default_queuename.replace(' ', '_') + self._waittime = waittime + + @property + def interface(self): + """ + Returns the XQueueInterface instance. + """ + return self._interface + + @property + def construct_callback(self): + """ + Returns the function to construct the XQueue callback. + """ + return self._construct_callback + + @property + def default_queuename(self): + """ + Returns the default queue name for the current course. + """ + return self._default_queuename + + @property + def waittime(self): + """ + Returns the number of seconds to wait in between calls to XQueue. + """ + return self._waittime diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 26a42d59d6..c6732ada83 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -121,6 +121,8 @@ class Randomization(String): @XBlock.needs('user') @XBlock.needs('i18n') @XBlock.needs('mako') +# Studio doesn't provide XQueueService, but the LMS does. +@XBlock.wants('xqueue') @XBlock.wants('call_to_action') class ProblemBlock( ScorableXBlockMixin, diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 2dd1a71326..ccc460e05e 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -26,6 +26,7 @@ from xblock.core import XBlock from xblock.field_data import DictFieldData from xblock.fields import Reference, ReferenceList, ReferenceValueDict, ScopeIds +from capa.xqueue_interface import XQueueService from xmodule.assetstore import AssetMetadata from xmodule.error_module import ErrorBlock from xmodule.mako_module import MakoDescriptorSystem @@ -144,13 +145,14 @@ def get_test_system( services={ 'user': user_service, 'mako': mako_service, - }, - xqueue={ - 'interface': None, - 'callback_url': '/', - 'default_queuename': 'testqueue', - 'waittime': 10, - 'construct_callback': Mock(name='get_test_system.xqueue.construct_callback', side_effect="/"), + 'xqueue': XQueueService( + url='http://xqueue.url', + django_auth={}, + basic_auth=[], + default_queuename='testqueue', + waittime=10, + construct_callback=Mock(name='get_test_system.xqueue.construct_callback', side_effect="/"), + ), }, node_path=os.environ.get("NODE_PATH", "/usr/local/lib/node_modules"), course_id=course_id, diff --git a/common/lib/xmodule/xmodule/tests/test_capa_module.py b/common/lib/xmodule/xmodule/tests/test_capa_module.py index 4121cac007..2d3f2a4e42 100644 --- a/common/lib/xmodule/xmodule/tests/test_capa_module.py +++ b/common/lib/xmodule/xmodule/tests/test_capa_module.py @@ -817,7 +817,8 @@ class ProblemBlockTest(unittest.TestCase): # lint-amnesty, pylint: disable=miss # Expect that the number of attempts is NOT incremented assert module.attempts == 1 - def test_submit_problem_with_files(self): + @patch.object(XQueueInterface, '_http_post') + def test_submit_problem_with_files(self, mock_xqueue_post): # Check a problem with uploaded files, using the submit_problem API. # pylint: disable=protected-access @@ -830,10 +831,8 @@ class ProblemBlockTest(unittest.TestCase): # lint-amnesty, pylint: disable=miss module = CapaFactoryWithFiles.create() - # Mock the XQueueInterface. - xqueue_interface = XQueueInterface("http://example.com/xqueue", Mock()) - xqueue_interface._http_post = Mock(return_value=(0, "ok")) - module.system.xqueue['interface'] = xqueue_interface + # Mock the XQueueInterface post method + mock_xqueue_post.return_value = (0, "ok") # Create a request dictionary for submit_problem. get_request_dict = { @@ -862,13 +861,14 @@ class ProblemBlockTest(unittest.TestCase): # lint-amnesty, pylint: disable=miss # ) # pylint: enable=line-too-long - assert xqueue_interface._http_post.call_count == 1 - _, kwargs = xqueue_interface._http_post.call_args + assert mock_xqueue_post.call_count == 1 + _, kwargs = mock_xqueue_post.call_args self.assertCountEqual(fpaths, list(kwargs['files'].keys())) for fpath, fileobj in kwargs['files'].items(): assert fpath == fileobj.name - def test_submit_problem_with_files_as_xblock(self): + @patch.object(XQueueInterface, '_http_post') + def test_submit_problem_with_files_as_xblock(self, mock_xqueue_post): # Check a problem with uploaded files, using the XBlock API. # pylint: disable=protected-access @@ -881,10 +881,8 @@ class ProblemBlockTest(unittest.TestCase): # lint-amnesty, pylint: disable=miss module = CapaFactoryWithFiles.create() - # Mock the XQueueInterface. - xqueue_interface = XQueueInterface("http://example.com/xqueue", Mock()) - xqueue_interface._http_post = Mock(return_value=(0, "ok")) - module.system.xqueue['interface'] = xqueue_interface + # Mock the XQueueInterface post method + mock_xqueue_post.return_value = (0, "ok") # Create a webob Request with the files uploaded. post_data = [] @@ -895,8 +893,8 @@ class ProblemBlockTest(unittest.TestCase): # lint-amnesty, pylint: disable=miss module.handle('xmodule_handler', request, 'problem_check') - assert xqueue_interface._http_post.call_count == 1 - _, kwargs = xqueue_interface._http_post.call_args + assert mock_xqueue_post.call_count == 1 + _, kwargs = mock_xqueue_post.call_args self.assertCountEqual(fnames, list(kwargs['files'].keys())) for fpath, fileobj in kwargs['files'].items(): assert fpath == fileobj.name @@ -3101,7 +3099,8 @@ class ProblemCheckTrackingTest(unittest.TestCase): 'group_label': '', 'variant': module.seed}} - def test_file_inputs(self): + @patch.object(XQueueInterface, '_http_post') + def test_file_inputs(self, mock_xqueue_post): fnames = ["prog1.py", "prog2.py", "prog3.py"] fpaths = [os.path.join(DATA_DIR, "capa", fname) for fname in fnames] fileobjs = [open(fpath) for fpath in fpaths] @@ -3111,10 +3110,8 @@ class ProblemCheckTrackingTest(unittest.TestCase): factory = CapaFactoryWithFiles module = factory.create() - # Mock the XQueueInterface. - xqueue_interface = XQueueInterface("http://example.com/xqueue", Mock()) - xqueue_interface._http_post = Mock(return_value=(0, "ok")) # pylint: disable=protected-access - module.system.xqueue['interface'] = xqueue_interface + # Mock the XQueueInterface post method + mock_xqueue_post.return_value = (0, "ok") answer_input_dict = { CapaFactoryWithFiles.input_key(response_num=2): fileobjs, diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 607c1b2ecf..00c3bf66ab 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -1881,6 +1881,31 @@ class ModuleSystemShim: return render_service.render_template return None + @property + def xqueue(self): + """ + Returns a dict containing the XQueueInterface object, as well as parameters for the specific StudentModule: + * interface: XQueueInterface object + * construct_callback: function to construct the fully-qualified LMS callback URL. + * default_queuename: default queue name for the course in XQueue + * waittime: number of seconds to wait in between calls to XQueue + + Deprecated in favor of the xqueue service. + """ + warnings.warn( + 'runtime.xqueue is deprecated. Please use the xqueue service instead.', + DeprecationWarning, stacklevel=3, + ) + xqueue_service = self._services.get('xqueue') + if xqueue_service: + return { + 'interface': xqueue_service.interface, + 'construct_callback': xqueue_service.construct_callback, + 'default_queuename': xqueue_service.default_queuename, + 'waittime': xqueue_service.waittime, + } + return None + class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemShim, Runtime): """ @@ -1898,7 +1923,7 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemShim, def __init__( self, static_url, track_function, get_module, replace_urls, descriptor_runtime, filestore=None, - debug=False, hostname="", xqueue=None, publish=None, node_path="", + debug=False, hostname="", publish=None, node_path="", course_id=None, cache=None, can_execute_unsafe_code=None, replace_course_urls=None, replace_jump_to_id_urls=None, error_descriptor_class=None, @@ -1921,12 +1946,6 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemShim, filestore - A filestore ojbect. Defaults to an instance of OSFS based at settings.DATA_DIR. - xqueue - Dict containing XqueueInterface object, as well as parameters - for the specific StudentModule: - xqueue = {'interface': XQueueInterface object, - 'callback_url': Callback into the LMS, - 'queue_name': Target queuename in Xqueue} - replace_urls - TEMPORARY - A function like static_replace.replace_urls that capa_module can use to fix up the static urls in ajax results. @@ -1963,7 +1982,6 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemShim, super().__init__(field_data=field_data, **kwargs) self.STATIC_URL = static_url - self.xqueue = xqueue self.track_function = track_function self.filestore = filestore self.get_module = get_module diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 00ffa2a9b8..d3ef4d5ea7 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -31,7 +31,6 @@ from edx_when.field_data import DateLookupFieldData from eventtracking import tracker from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey -from requests.auth import HTTPBasicAuth from rest_framework.decorators import api_view from rest_framework.exceptions import APIException from web_fragments.fragment import Fragment @@ -42,7 +41,7 @@ from xblock.runtime import KvsFieldData from common.djangoapps import static_replace from common.djangoapps.xblock_django.constants import ATTR_KEY_USER_ID -from capa.xqueue_interface import XQueueInterface # lint-amnesty, pylint: disable=wrong-import-order +from capa.xqueue_interface import XQueueService from lms.djangoapps.courseware.access import get_user_role, has_access from lms.djangoapps.courseware.entrance_exams import user_can_skip_entrance_exam, user_has_passed_entrance_exam from lms.djangoapps.courseware.masquerade import ( @@ -100,17 +99,6 @@ from xmodule.util.sandboxing import can_execute_unsafe_code, get_python_lib_zip log = logging.getLogger(__name__) -if settings.XQUEUE_INTERFACE.get('basic_auth') is not None: - REQUESTS_AUTH = HTTPBasicAuth(*settings.XQUEUE_INTERFACE['basic_auth']) -else: - REQUESTS_AUTH = None - -XQUEUE_INTERFACE = XQueueInterface( - settings.XQUEUE_INTERFACE['url'], - settings.XQUEUE_INTERFACE['django_auth'], - REQUESTS_AUTH, -) - # TODO: course_id and course_key are used interchangeably in this file, which is wrong. # Some brave person should make the variable names consistently someday, but the code's # coupled enough that it's kind of tricky--you've been warned! @@ -490,12 +478,14 @@ def get_module_system_for_user( # TODO: Queuename should be derived from 'course_settings.json' of each course xqueue_default_queuename = descriptor.location.org + '-' + descriptor.location.course - xqueue = { - 'interface': XQUEUE_INTERFACE, - 'construct_callback': make_xqueue_callback, - 'default_queuename': xqueue_default_queuename.replace(' ', '_'), - 'waittime': settings.XQUEUE_WAITTIME_BETWEEN_REQUESTS - } + xqueue_service = XQueueService( + construct_callback=make_xqueue_callback, + default_queuename=xqueue_default_queuename, + url=settings.XQUEUE_INTERFACE['url'], + django_auth=settings.XQUEUE_INTERFACE['django_auth'], + basic_auth=settings.XQUEUE_INTERFACE.get('basic_auth'), + waittime=settings.XQUEUE_WAITTIME_BETWEEN_REQUESTS, + ) def inner_get_module(descriptor): """ @@ -774,7 +764,6 @@ def get_module_system_for_user( system = LmsModuleSystem( track_function=track_function, static_url=settings.STATIC_URL, - xqueue=xqueue, # TODO (cpennington): Figure out how to share info between systems filestore=descriptor.runtime.resources_fs, get_module=inner_get_module, @@ -822,6 +811,7 @@ def get_module_system_for_user( 'grade_utils': GradesUtilService(course_id=course_id), 'user_state': UserStateService(), 'content_type_gating': ContentTypeGatingService(), + 'xqueue': xqueue_service, }, descriptor_runtime=descriptor._runtime, # pylint: disable=protected-access rebind_noauth_module_to_user=rebind_noauth_module_to_user, diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index bdbf449a12..3e138e27f9 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -41,6 +41,7 @@ from xblock.runtime import DictKeyValueStore, KvsFieldData, Runtime # lint-amne from xblock.test.tools import TestRuntime # lint-amnesty, pylint: disable=wrong-import-order from capa.tests.response_xml_factory import OptionResponseXMLFactory # lint-amnesty, pylint: disable=reimported +from capa.xqueue_interface import XQueueInterface from common.djangoapps.course_modes.models import CourseMode # lint-amnesty, pylint: disable=reimported from common.djangoapps.student.tests.factories import GlobalStaffFactory from common.djangoapps.student.tests.factories import RequestFactoryNoCsrf @@ -2567,6 +2568,7 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): """ Tests that the deprecated attributes in the LMS Module System (XBlock Runtime) return the expected values. """ + COURSE_ID = 'edX/LmsModuleShimTest/2021_Fall' @classmethod def setUpClass(cls): @@ -2574,7 +2576,8 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): Set up the course and descriptor used to instantiate the runtime. """ super().setUpClass() - cls.course = CourseFactory.create() + org, number, run = cls.COURSE_ID.split('/') + cls.course = CourseFactory.create(org=org, number=number, run=run) cls.descriptor = ItemFactory(category="vertical", parent=cls.course) cls.problem_descriptor = ItemFactory(category="problem", parent=cls.course) @@ -2586,7 +2589,7 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): self.user = UserFactory(id=232) self.student_data = Mock() self.track_function = Mock() - self.xqueue_callback_url_prefix = Mock() + self.xqueue_callback_url_prefix = 'https://lms.url' self.request_token = Mock() @ddt.data( @@ -2737,3 +2740,22 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): ) rendered = runtime.render_template('templates/edxmako.html', {'element_id': 'hi'}) # pylint: disable=not-callable assert rendered == '
Testing the MakoService
\n' + + def test_xqueue(self): + runtime, _ = render.get_module_system_for_user( + self.user, + self.student_data, + 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['default_queuename'] == 'edX-LmsModuleShimTest' + assert xqueue['waittime'] == 5 + callback_url = f'https://lms.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/courseware/tests/test_submitting_problems.py b/lms/djangoapps/courseware/tests/test_submitting_problems.py index a9b15db3b1..2757bd77db 100644 --- a/lms/djangoapps/courseware/tests/test_submitting_problems.py +++ b/lms/djangoapps/courseware/tests/test_submitting_problems.py @@ -26,6 +26,7 @@ from capa.tests.response_xml_factory import ( OptionResponseXMLFactory, SchematicResponseXMLFactory ) +from capa.xqueue_interface import XQueueInterface from common.djangoapps.course_modes.models import CourseMode from lms.djangoapps.courseware.models import BaseStudentModuleHistory, StudentModule from lms.djangoapps.courseware.tests.helpers import LoginEnrollmentTestCase @@ -776,7 +777,8 @@ class ProblemWithUploadedFilesTest(TestSubmittingProblems): # re-fetch the course from the database so the object is up to date self.refresh_course() - def test_three_files(self): + @patch.object(XQueueInterface, '_http_post') + def test_three_files(self, mock_xqueue_post): # Open the test files, and arrange to close them later. filenames = "prog1.py prog2.py prog3.py" fileobjs = [ @@ -787,20 +789,19 @@ class ProblemWithUploadedFilesTest(TestSubmittingProblems): self.addCleanup(fileobj.close) self.problem_setup("the_problem", filenames) - with patch('lms.djangoapps.courseware.module_render.XQUEUE_INTERFACE.session') as mock_session: - resp = self.submit_question_answer("the_problem", {'2_1': fileobjs}) + mock_xqueue_post.return_value = (0, "ok") + resp = self.submit_question_answer("the_problem", {'2_1': fileobjs}) assert resp.status_code == 200 json_resp = json.loads(resp.content.decode('utf-8')) assert json_resp['success'] == 'incorrect' # See how post got called. - name, args, kwargs = mock_session.mock_calls[0] - assert name == 'post' - assert len(args) == 1 + assert mock_xqueue_post.call_count == 1 + args, kwargs = mock_xqueue_post.call_args + assert len(args) == 2 assert args[0].endswith('/submit/') - self.assertCountEqual(list(kwargs.keys()), ["files", "data", "timeout"]) - self.assertCountEqual(list(kwargs['files'].keys()), filenames.split()) + self.assertEqual(list(kwargs['files'].keys()), filenames.split()) class TestPythonGradedResponse(TestSubmittingProblems):