From 0137f21627eed60ca7c5b30a57221a0d4b11df8e Mon Sep 17 00:00:00 2001 From: Piotr Surowiec Date: Wed, 21 Jun 2023 20:08:31 +0200 Subject: [PATCH] feat: move XQueueService from the runtime to ProblemBlock The XQueueService is used only by the ProblemBlock. Therefore, we are moving it out of the runtime, and into the ProblemBlock, where it's initialized only when it's going to be used. --- lms/djangoapps/courseware/block_render.py | 34 ---------- .../courseware/tests/test_block_render.py | 44 ------------- xmodule/capa/tests/test_xqueue_interface.py | 53 +++++++++------- xmodule/capa/xqueue_interface.py | 62 +++++++++++-------- xmodule/capa_block.py | 9 +-- xmodule/tests/__init__.py | 16 ----- xmodule/x_module.py | 25 -------- 7 files changed, 74 insertions(+), 169 deletions(-) diff --git a/lms/djangoapps/courseware/block_render.py b/lms/djangoapps/courseware/block_render.py index 2bab345b83..4bb7a129b1 100644 --- a/lms/djangoapps/courseware/block_render.py +++ b/lms/djangoapps/courseware/block_render.py @@ -53,7 +53,6 @@ from xmodule.util.sandboxing import SandboxService from xmodule.services import EventPublishingService, RebindUserService, SettingsService, TeamsConfigurationService from common.djangoapps.static_replace.services import ReplaceURLService from common.djangoapps.static_replace.wrapper import replace_urls_wrapper -from xmodule.capa.xqueue_interface import XQueueService # lint-amnesty, pylint: disable=wrong-import-order 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 ( @@ -439,44 +438,12 @@ def prepare_runtime_for_user( KvsFieldData: student_data bound to, primarily, the user and block """ - def make_xqueue_callback(dispatch='score_update'): - """ - Returns fully qualified callback URL for external queueing system - """ - relative_xqueue_callback_url = reverse( - 'xqueue_callback', - kwargs=dict( - course_id=str(course_id), - userid=str(user.id), - mod_id=str(block.location), - 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 - # contains the current block. - # TODO: Queuename should be derived from 'course_settings.json' of each course - xqueue_default_queuename = block.location.org + '-' + block.location.course - - 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_block(block): """ Delegate to get_block_for_descriptor_internal() with all values except `block` set. Because it does an access check, it may return None. """ - # TODO: fix this so that make_xqueue_callback uses the block passed into - # inner_get_block, not the parent's callback. Add it as an argument.... return get_block_for_descriptor_internal( user=user, block=block, @@ -579,7 +546,6 @@ def prepare_runtime_for_user( 'content_type_gating': ContentTypeGatingService(), 'cache': CacheService(cache), 'sandbox': SandboxService(contentstore=contentstore, course_id=course_id), - 'xqueue': xqueue_service, 'replace_urls': replace_url_service, # Rebind module service to deal with noauth modules getting attached to users. 'rebind_user': RebindUserService( diff --git a/lms/djangoapps/courseware/tests/test_block_render.py b/lms/djangoapps/courseware/tests/test_block_render.py index dc654a3995..e0f97f5418 100644 --- a/lms/djangoapps/courseware/tests/test_block_render.py +++ b/lms/djangoapps/courseware/tests/test_block_render.py @@ -41,7 +41,6 @@ from xblock.runtime import DictKeyValueStore, KvsFieldData # lint-amnesty, pyli from xblock.test.tools import TestRuntime # lint-amnesty, pylint: disable=wrong-import-order from xmodule.capa.tests.response_xml_factory import OptionResponseXMLFactory # lint-amnesty, pylint: disable=reimported -from xmodule.capa.xqueue_interface import XQueueInterface from xmodule.capa_block import ProblemBlock from xmodule.contentstore.django import contentstore from xmodule.html_block import AboutBlock, CourseInfoBlock, HtmlBlock, StaticTabBlock @@ -121,7 +120,6 @@ TEST_DATA_DIR = settings.COMMON_TEST_DATA_ROOT @XBlock.needs('content_type_gating') @XBlock.needs('cache') @XBlock.needs('sandbox') -@XBlock.needs('xqueue') @XBlock.needs('replace_urls') @XBlock.needs('rebind_user') @XBlock.needs('completion') @@ -2333,7 +2331,6 @@ class LMSXBlockServiceBindingTest(LMSXBlockServiceMixin): 'content_type_gating', 'cache', 'sandbox', - 'xqueue', 'replace_urls', 'rebind_user', 'completion', @@ -2865,47 +2862,6 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): rendered = self.block.runtime.render_template('templates/edxmako.html', {'element_id': 'hi'}) # pylint: disable=not-callable assert rendered == '
Testing the MakoService
\n' - def test_xqueue(self): - xqueue = self.block.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'http://localhost:8000/courses/{self.course.id}/xqueue/232/{self.block.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): - _ = render.prepare_runtime_for_user( - self.user, - self.student_data, - self.block, - self.course.id, - self.track_function, - self.request_token, - course=self.course, - ) - xqueue = self.block.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/{self.course.id}/xqueue/232/{self.block.location}' - assert xqueue['construct_callback']() == f'{callback_url}/score_update' - assert xqueue['construct_callback']('mock_dispatch') == f'{callback_url}/mock_dispatch' - @override_settings(COURSES_WITH_UNSAFE_CODE=[r'course-v1:edX\+LmsModuleShimTest\+2021_Fall']) def test_can_execute_unsafe_code_when_allowed(self): assert self.block.runtime.can_execute_unsafe_code() diff --git a/xmodule/capa/tests/test_xqueue_interface.py b/xmodule/capa/tests/test_xqueue_interface.py index 315e0c0b0a..5fd913a15e 100644 --- a/xmodule/capa/tests/test_xqueue_interface.py +++ b/xmodule/capa/tests/test_xqueue_interface.py @@ -1,41 +1,52 @@ -# -*- coding: utf-8 -*- -""" -Tests the xqueue service interface. -""" +"""Test the XQueue service and interface.""" from unittest import TestCase +from unittest.mock import Mock + from django.conf import settings +from django.test.utils import override_settings +from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator +from xblock.fields import ScopeIds from xmodule.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' - + """Test the XQueue service methods.""" 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, - ) + location = BlockUsageLocator(CourseLocator("test_org", "test_course", "test_run"), "problem", "ExampleProblem") + self.block = Mock(scope_ids=ScopeIds('user1', 'mock_problem', location, location)) + self.service = XQueueService(self.block) def test_interface(self): + """Test that the `XQUEUE_INTERFACE` settings are passed from the service to the interface.""" assert isinstance(self.service.interface, XQueueInterface) + assert self.service.interface.url == 'http://sandbox-xqueue.edx.org' + assert self.service.interface.auth['username'] == 'lms' + assert self.service.interface.auth['password'] == '***REMOVED***' + assert self.service.interface.session.auth.username == 'anant' + assert self.service.interface.session.auth.password == 'agarwal' def test_construct_callback(self): - assert self.service.construct_callback() == 'https://lms.url/callback' + """Test that the XQueue callback is initialized correctly, and can be altered through the settings.""" + usage_id = self.block.scope_ids.usage_id + callback_url = f'courses/{usage_id.context_key}/xqueue/user1/{usage_id}' + + assert self.service.construct_callback() == f'{settings.LMS_ROOT_URL}/{callback_url}/score_update' + assert self.service.construct_callback('alt_dispatch') == f'{settings.LMS_ROOT_URL}/{callback_url}/alt_dispatch' + + custom_callback_url = 'http://alt.url' + with override_settings(XQUEUE_INTERFACE={**settings.XQUEUE_INTERFACE, 'callback_url': custom_callback_url}): + assert self.service.construct_callback() == f'{custom_callback_url}/{callback_url}/score_update' def test_default_queuename(self): - assert self.service.default_queuename == 'my-very-own-queue' + """Check the format of the default queue name.""" + assert self.service.default_queuename == 'test_org-test_course' def test_waittime(self): + """Check that the time between requests is retrieved correctly from the settings.""" assert self.service.waittime == 5 + + with override_settings(XQUEUE_WAITTIME_BETWEEN_REQUESTS=15): + assert self.service.waittime == 15 diff --git a/xmodule/capa/xqueue_interface.py b/xmodule/capa/xqueue_interface.py index f035d12201..aee7232a41 100644 --- a/xmodule/capa/xqueue_interface.py +++ b/xmodule/capa/xqueue_interface.py @@ -1,13 +1,19 @@ """ LMS Interface to external queueing system (xqueue) """ +from typing import Dict, Optional, TYPE_CHECKING import hashlib import json import logging import requests -import six +from django.conf import settings +from django.urls import reverse +from requests.auth import HTTPBasicAuth + +if TYPE_CHECKING: + from xmodule.capa_block import ProblemBlock log = logging.getLogger(__name__) dateformat = '%Y%m%d%H%M%S' @@ -25,7 +31,7 @@ def make_hashkey(seed): Generate a string key by hashing """ h = hashlib.md5() - h.update(six.b(str(seed))) + h.update(str(seed).encode('latin-1')) return h.hexdigest() @@ -65,13 +71,13 @@ def parse_xreply(xreply): return (return_code, content) -class XQueueInterface(object): +class XQueueInterface: """ Interface to the external grading system """ - def __init__(self, url, django_auth, requests_auth=None): - self.url = six.text_type(url) + def __init__(self, url: str, django_auth: Dict[str, str], requests_auth: Optional[HTTPBasicAuth] = None): + self.url = url self.auth = django_auth self.session = requests.Session() self.session.auth = requests_auth @@ -155,21 +161,17 @@ 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. + block: The `ProblemBlock` instance. """ - 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) + def __init__(self, block: 'ProblemBlock'): + basic_auth = settings.XQUEUE_INTERFACE.get('basic_auth') + requests_auth = HTTPBasicAuth(*basic_auth) if basic_auth else None + self._interface = XQueueInterface( + settings.XQUEUE_INTERFACE['url'], settings.XQUEUE_INTERFACE['django_auth'], requests_auth + ) - self._construct_callback = construct_callback - self._default_queuename = default_queuename.replace(' ', '_') - self._waittime = waittime + self._block = block @property def interface(self): @@ -178,23 +180,33 @@ class XQueueService: """ return self._interface - @property - def construct_callback(self): + def construct_callback(self, dispatch: str = 'score_update') -> str: """ - Returns the function to construct the XQueue callback. + Return a fully qualified callback URL for external queueing system. """ - return self._construct_callback + relative_xqueue_callback_url = reverse( + 'xqueue_callback', + kwargs=dict( + course_id=str(self._block.scope_ids.usage_id.context_key), + userid=str(self._block.scope_ids.user_id), + mod_id=str(self._block.scope_ids.usage_id), + 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 @property - def default_queuename(self): + def default_queuename(self) -> str: """ Returns the default queue name for the current course. """ - return self._default_queuename + course_id = self._block.scope_ids.usage_id.context_key + return f'{course_id.org}-{course_id.course}'.replace(' ', '_') @property - def waittime(self): + def waittime(self) -> int: """ Returns the number of seconds to wait in between calls to XQueue. """ - return self._waittime + return settings.XQUEUE_WAITTIME_BETWEEN_REQUESTS diff --git a/xmodule/capa_block.py b/xmodule/capa_block.py index c70e619f27..9100077127 100644 --- a/xmodule/capa_block.py +++ b/xmodule/capa_block.py @@ -52,6 +52,7 @@ from common.djangoapps.xblock_django.constants import ( ATTR_KEY_USER_ID, ) from openedx.core.djangolib.markup import HTML, Text +from .capa.xqueue_interface import XQueueService from .fields import Date, ScoreField, Timedelta from .progress import Progress @@ -123,8 +124,6 @@ class Randomization(String): @XBlock.needs('cache') @XBlock.needs('sandbox') @XBlock.needs('replace_urls') -# Studio doesn't provide XQueueService, but the LMS does. -@XBlock.wants('xqueue') @XBlock.wants('call_to_action') class ProblemBlock( ScorableXBlockMixin, @@ -814,6 +813,8 @@ class ProblemBlock( sandbox_service = self.runtime.service(self, 'sandbox') cache_service = self.runtime.service(self, 'cache') + is_studio = getattr(self.runtime, 'is_author_mode', False) + capa_system = LoncapaSystem( ajax_url=self.ajax_url, anonymous_student_id=anonymous_student_id, @@ -825,7 +826,7 @@ class ProblemBlock( render_template=self.runtime.service(self, 'mako').render_template, resources_fs=self.runtime.resources_fs, seed=seed, # Why do we do this if we have self.seed? - xqueue=self.runtime.service(self, 'xqueue'), + xqueue=None if is_studio else XQueueService(self), matlab_api_key=self.matlab_api_key ) @@ -1736,7 +1737,7 @@ class ProblemBlock( if self.lcp.is_queued(): prev_submit_time = self.lcp.get_recentmost_queuetime() - xqueue_service = self.runtime.service(self, 'xqueue') + xqueue_service = self.lcp.capa_system.xqueue waittime_between_requests = xqueue_service.waittime if xqueue_service else 0 if (current_time - prev_submit_time).total_seconds() < waittime_between_requests: msg = _("You must wait at least {wait} seconds between submissions.").format( diff --git a/xmodule/tests/__init__.py b/xmodule/tests/__init__.py index a68c8161c2..b2cdd67b71 100644 --- a/xmodule/tests/__init__.py +++ b/xmodule/tests/__init__.py @@ -161,14 +161,6 @@ def get_test_system( services = { 'user': user_service, 'mako': mako_service, - '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="/"), - ), 'replace_urls': replace_url_service, 'cache': CacheService(DoNothingCache()), 'field-data': DictFieldData({}), @@ -224,14 +216,6 @@ def prepare_block_runtime( services = { 'user': user_service, 'mako': mako_service, - '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="/"), - ), 'replace_urls': replace_url_service, 'cache': CacheService(DoNothingCache()), 'field-data': DictFieldData({}), diff --git a/xmodule/x_module.py b/xmodule/x_module.py index b1c8d5bfb0..fbea2b2896 100644 --- a/xmodule/x_module.py +++ b/xmodule/x_module.py @@ -1239,31 +1239,6 @@ class ModuleSystemShim: """ self._deprecated_render_template = render_template - @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._runtime_services.get('xqueue') or 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 - @property def can_execute_unsafe_code(self): """