From b1b18b5db6add9ad22d7f786a1e765cb5ac4c6fa Mon Sep 17 00:00:00 2001 From: gabrielC1409 Date: Wed, 12 Feb 2025 21:42:59 -0400 Subject: [PATCH 1/3] feat: Send xqueue submissions to edx-submission fix: Restructuring to send course_id and score to edx submission --- lms/urls.py | 8 + scripts/xsslint_thresholds.json | 5 +- xmodule/capa/capa_problem.py | 4 + xmodule/capa/inputtypes.py | 22 +- xmodule/capa/registry.py | 1 + xmodule/capa/tests/test_answer_pool.py | 1 + xmodule/capa/tests/test_capa_problem.py | 22 +- xmodule/capa/tests/test_customrender.py | 1 + xmodule/capa/tests/test_inputtypes.py | 45 ++++ xmodule/capa/tests/test_responsetypes.py | 11 +- xmodule/capa/tests/test_util.py | 2 +- xmodule/capa/tests/test_xqueue_interface.py | 123 ++++++++-- xmodule/capa/tests/test_xqueue_submission.py | 129 ++++++++++ xmodule/capa/xqueue_interface.py | 67 +++++- xmodule/capa/xqueue_submission.py | 127 ++++++++++ .../0005-send-data-to-edx-submission.rst | 220 ++++++++++++++++++ 16 files changed, 736 insertions(+), 52 deletions(-) create mode 100644 xmodule/capa/tests/test_xqueue_submission.py create mode 100644 xmodule/capa/xqueue_submission.py create mode 100644 xmodule/docs/decisions/0005-send-data-to-edx-submission.rst diff --git a/lms/urls.py b/lms/urls.py index 9c7dd433a6ff..3435b963d89d 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -347,6 +347,14 @@ xqueue_callback, name='xqueue_callback', ), + + re_path( + r'^courses/{}/xqueue/(?P[^/]*)/(?P.*?)/(?P[^/]*)$'.format( + settings.COURSE_ID_PATTERN, + ), + xqueue_callback, + name='callback_submission', + ), # TODO: These views need to be updated before they work path('calculate', util_views.calculate), diff --git a/scripts/xsslint_thresholds.json b/scripts/xsslint_thresholds.json index 26c267c074cd..f1c2293577dc 100644 --- a/scripts/xsslint_thresholds.json +++ b/scripts/xsslint_thresholds.json @@ -9,6 +9,7 @@ "django-trans-escape-variable-mismatch": 0, "django-trans-invalid-escape-filter": 0, "django-trans-missing-escape": 0, + "django-trans-escape-filter-parse-error": 0, "javascript-concat-html": 2, "javascript-escape": 1, "javascript-jquery-append": 2, @@ -36,5 +37,5 @@ "python-wrap-html": 0, "underscore-not-escaped": 2 }, - "total": 64 -} + "total": 65 +} \ No newline at end of file diff --git a/xmodule/capa/capa_problem.py b/xmodule/capa/capa_problem.py index 6d78e156f292..831454860146 100644 --- a/xmodule/capa/capa_problem.py +++ b/xmodule/capa/capa_problem.py @@ -32,6 +32,8 @@ import xmodule.capa.inputtypes as inputtypes import xmodule.capa.responsetypes as responsetypes import xmodule.capa.xqueue_interface as xqueue_interface +import xmodule.capa.xqueue_submission as xqueue_submission +from xmodule.capa.xqueue_interface import get_flag_by_name from xmodule.capa.correctmap import CorrectMap from xmodule.capa.safe_exec import safe_exec from xmodule.capa.util import contextualize_text, convert_files_to_filenames, get_course_id_from_capa_block @@ -96,6 +98,7 @@ class LoncapaSystem(object): See :class:`DescriptorSystem` for documentation of other attributes. """ + def __init__( self, ajax_url, @@ -130,6 +133,7 @@ class LoncapaProblem(object): """ Main class for capa Problems. """ + def __init__(self, problem_text, id, capa_system, capa_block, # pylint: disable=redefined-builtin state=None, seed=None, minimal_init=False, extract_tree=True): """ diff --git a/xmodule/capa/inputtypes.py b/xmodule/capa/inputtypes.py index 8dff57768688..df9e48b09b73 100644 --- a/xmodule/capa/inputtypes.py +++ b/xmodule/capa/inputtypes.py @@ -56,18 +56,16 @@ from lxml import etree -from xmodule.capa.xqueue_interface import XQUEUE_TIMEOUT +from xmodule.capa.xqueue_interface import XQUEUE_TIMEOUT, get_flag_by_name from openedx.core.djangolib.markup import HTML, Text from xmodule.stringify import stringify_children -from . import xqueue_interface +from . import xqueue_interface, xqueue_submission from .registry import TagRegistry from .util import sanitize_html log = logging.getLogger(__name__) -######################################################################### - registry = TagRegistry() # pylint: disable=invalid-name @@ -408,7 +406,7 @@ class OptionInput(InputTypeBase): Example: - The location of the sky + The location of the sky # TODO: allow ordering to be randomized """ @@ -505,7 +503,7 @@ def setup(self): raise Exception(msg) self.choices = self.extract_choices(self.xml, i18n) - self._choices_map = dict(self.choices,) + self._choices_map = dict(self.choices, ) @classmethod def get_attributes(cls): @@ -602,16 +600,16 @@ def get_attributes(cls): Register the attributes. """ return [ - Attribute('params', None), # extra iframe params + Attribute('params', None), # extra iframe params Attribute('html_file', None), Attribute('gradefn', "gradefn"), Attribute('get_statefn', None), # Function to call in iframe - # to get current state. + # to get current state. Attribute('initial_state', None), # JSON string to be used as initial state Attribute('set_statefn', None), # Function to call iframe to - # set state - Attribute('width', "400"), # iframe width - Attribute('height', "300"), # iframe height + # set state + Attribute('width', "400"), # iframe width + Attribute('height', "300"), # iframe height # Title for the iframe, which should be supplied by the author of the problem. Not translated # because we are in a class method and therefore do not have access to capa_system.i18n. # Note that the default "display name" for the problem is also not translated. @@ -1626,7 +1624,7 @@ class ChoiceTextGroup(InputTypeBase): CheckboxProblem: - A person randomly selects 100 times, with replacement, from the list of numbers \(\sqrt{2}\) , 2, 3, 4 ,5 ,6 + A person randomly selects 100 times, with replacement, from the list of numbers \(\sqrt{2}\), 2, 3, 4, 5, 6 and records the results. The first number they pick is \(\sqrt{2}\) Given this information select the correct choices and fill in numbers to make them accurate. diff --git a/xmodule/capa/registry.py b/xmodule/capa/registry.py index 1f0674771fd6..fc8b853ee617 100644 --- a/xmodule/capa/registry.py +++ b/xmodule/capa/registry.py @@ -7,6 +7,7 @@ class TagRegistry(object): (A dictionary with some extra error checking.) """ + def __init__(self): self._mapping = {} diff --git a/xmodule/capa/tests/test_answer_pool.py b/xmodule/capa/tests/test_answer_pool.py index f4f65d3ee9d8..d11df3a203f1 100644 --- a/xmodule/capa/tests/test_answer_pool.py +++ b/xmodule/capa/tests/test_answer_pool.py @@ -13,6 +13,7 @@ class CapaAnswerPoolTest(unittest.TestCase): """Capa Answer Pool Test""" + def setUp(self): super(CapaAnswerPoolTest, self).setUp() # lint-amnesty, pylint: disable=super-with-arguments self.system = test_capa_system() diff --git a/xmodule/capa/tests/test_capa_problem.py b/xmodule/capa/tests/test_capa_problem.py index 74cf4d096fd1..4e43efb50534 100644 --- a/xmodule/capa/tests/test_capa_problem.py +++ b/xmodule/capa/tests/test_capa_problem.py @@ -54,7 +54,7 @@ def test_label_and_description_inside_responsetype(self, question): """.format(question=question) problem = new_loncapa_problem(xml) assert problem.problem_data ==\ - {'1_2_1': {'label': question, 'descriptions': {'description_1_1_1': 'Only the paranoid survive.'}}} + {'1_2_1': {'label': question, 'descriptions': {'description_1_1_1': 'Only the paranoid survive.'}}} assert len(problem.tree.xpath('//label')) == 0 @ddt.unpack @@ -123,7 +123,7 @@ def test_neither_label_tag_nor_attribute(self, question1, question2): """.format(question1, question2) problem = new_loncapa_problem(xml) assert problem.problem_data ==\ - {'1_2_1': {'label': question1, 'descriptions': {}}, '1_3_1': {'label': question2, 'descriptions': {}}} + {'1_2_1': {'label': question1, 'descriptions': {}}, '1_3_1': {'label': question2, 'descriptions': {}}} for question in (question1, question2): assert len(problem.tree.xpath('//label[text()="{}"]'.format(question))) == 0 @@ -146,8 +146,8 @@ def test_multiple_descriptions(self): """.format(desc1, desc2) problem = new_loncapa_problem(xml) assert problem.problem_data ==\ - {'1_2_1': {'label': '___ requires sacrifices.', - 'descriptions': {'description_1_1_1': desc1, 'description_1_1_2': desc2}}} + {'1_2_1': {'label': '___ requires sacrifices.', + 'descriptions': {'description_1_1_1': desc1, 'description_1_1_2': desc2}}} def test_additional_answer_is_skipped_from_resulting_html(self): """Tests that additional_answer element is not present in transformed HTML""" @@ -236,10 +236,10 @@ def test_multiple_questions_problem(self): """ problem = new_loncapa_problem(xml) assert problem.problem_data ==\ - {'1_2_1': {'label': 'Select the correct synonym of paranoid?', - 'descriptions': {'description_1_1_1': 'Only the paranoid survive.'}}, - '1_3_1': {'label': 'What Apple device competed with the portable CD player?', - 'descriptions': {'description_1_2_1': 'Device looks like an egg plant.'}}} + {'1_2_1': {'label': 'Select the correct synonym of paranoid?', + 'descriptions': {'description_1_1_1': 'Only the paranoid survive.'}}, + '1_3_1': {'label': 'What Apple device competed with the portable CD player?', + 'descriptions': {'description_1_2_1': 'Device looks like an egg plant.'}}} assert len(problem.tree.xpath('//label')) == 0 def test_question_title_not_removed_got_children(self): @@ -291,8 +291,8 @@ def test_multiple_inputtypes(self, group_label): problem = new_loncapa_problem(xml) assert problem.problem_data ==\ - {'1_2_1': {'group_label': group_label, 'label': input1_label, 'descriptions': {}}, - '1_2_2': {'group_label': group_label, 'label': input2_label, 'descriptions': {}}} + {'1_2_1': {'group_label': group_label, 'label': input1_label, 'descriptions': {}}, + '1_2_2': {'group_label': group_label, 'label': input2_label, 'descriptions': {}}} def test_single_inputtypes(self): """ @@ -355,7 +355,7 @@ def assert_question_tag(self, question1, question2, tag, label_attr=False): ) problem = new_loncapa_problem(xml) assert problem.problem_data ==\ - {'1_2_1': {'label': question1, 'descriptions': {}}, '1_3_1': {'label': question2, 'descriptions': {}}} + {'1_2_1': {'label': question1, 'descriptions': {}}, '1_3_1': {'label': question2, 'descriptions': {}}} assert len(problem.tree.xpath('//{}'.format(tag))) == 0 @ddt.unpack diff --git a/xmodule/capa/tests/test_customrender.py b/xmodule/capa/tests/test_customrender.py index 933096084495..95dc7a68a0f4 100644 --- a/xmodule/capa/tests/test_customrender.py +++ b/xmodule/capa/tests/test_customrender.py @@ -28,6 +28,7 @@ class HelperTest(unittest.TestCase): ''' Make sure that our helper function works! ''' + def check(self, d): xml = etree.XML(test_capa_system().render_template('blah', d)) assert d == extract_context(xml) diff --git a/xmodule/capa/tests/test_inputtypes.py b/xmodule/capa/tests/test_inputtypes.py index 4e14bc42b76b..5f585fbe8450 100644 --- a/xmodule/capa/tests/test_inputtypes.py +++ b/xmodule/capa/tests/test_inputtypes.py @@ -476,10 +476,12 @@ def test_rendering(self): assert context == expected +@pytest.mark.django_db class MatlabTest(unittest.TestCase): """ Test Matlab input types """ + def setUp(self): super(MatlabTest, self).setUp() # lint-amnesty, pylint: disable=super-with-arguments self.rows = '10' @@ -928,6 +930,40 @@ def test_matlab_sanitize_msg(self): expected = "" assert self.the_input._get_render_context()['msg'] == expected # pylint: disable=protected-access + @patch('xmodule.capa.inputtypes.get_flag_by_name', return_value=True) + @patch('xmodule.capa.inputtypes.datetime') + def test_plot_data_with_flag_active(self, mock_datetime, mock_get_flag_by_name): + """ + Test that the correct date format is used when the flag is active. + """ + mock_datetime.utcnow.return_value.strftime.return_value = 'formatted_date_with_flag' + data = {'submission': 'x = 1234;'} + response = self.the_input.handle_ajax("plot", data) + self.the_input.capa_system.xqueue.interface.send_to_queue.assert_called_with(header=ANY, body=ANY) + assert response['success'] + assert self.the_input.input_state['queuekey'] is not None + assert self.the_input.input_state['queuestate'] == 'queued' + assert 'formatted_date_with_flag' in self.the_input.capa_system.xqueue.interface.send_to_queue.call_args[1][ + 'body' + ] + + @patch('xmodule.capa.inputtypes.get_flag_by_name', return_value=False) + @patch('xmodule.capa.inputtypes.datetime') + def test_plot_data_with_flag_inactive(self, mock_datetime, mock_get_flag_by_name): + """ + Test that the correct date format is used when the flag is inactive. + """ + mock_datetime.utcnow.return_value.strftime.return_value = 'formatted_date_without_flag' + data = {'submission': 'x = 1234;'} + response = self.the_input.handle_ajax("plot", data) + self.the_input.capa_system.xqueue.interface.send_to_queue.assert_called_with(header=ANY, body=ANY) + assert response['success'] + assert self.the_input.input_state['queuekey'] is not None + assert self.the_input.input_state['queuestate'] == 'queued' + assert 'formatted_date_without_flag' in self.the_input.capa_system.xqueue.interface.send_to_queue.call_args[1][ + 'body' + ] + def html_tree_equal(received, expected): """ @@ -947,6 +983,7 @@ class SchematicTest(unittest.TestCase): """ Check that schematic inputs work """ + def test_rendering(self): height = '12' width = '33' @@ -1002,6 +1039,7 @@ class ImageInputTest(unittest.TestCase): """ Check that image inputs work """ + def check(self, value, egx, egy): # lint-amnesty, pylint: disable=missing-function-docstring height = '78' width = '427' @@ -1061,6 +1099,7 @@ class CrystallographyTest(unittest.TestCase): """ Check that crystallography inputs work """ + def test_rendering(self): height = '12' width = '33' @@ -1102,6 +1141,7 @@ class VseprTest(unittest.TestCase): """ Check that vsepr inputs work """ + def test_rendering(self): height = '12' width = '33' @@ -1149,6 +1189,7 @@ class ChemicalEquationTest(unittest.TestCase): """ Check that chemical equation inputs work. """ + def setUp(self): super(ChemicalEquationTest, self).setUp() # lint-amnesty, pylint: disable=super-with-arguments self.size = "42" @@ -1244,6 +1285,7 @@ class FormulaEquationTest(unittest.TestCase): """ Check that formula equation inputs work. """ + def setUp(self): super(FormulaEquationTest, self).setUp() # lint-amnesty, pylint: disable=super-with-arguments self.size = "42" @@ -1392,6 +1434,7 @@ class DragAndDropTest(unittest.TestCase): """ Check that drag and drop inputs work """ + def test_rendering(self): path_to_images = '/dummy-static/images/' @@ -1466,6 +1509,7 @@ class AnnotationInputTest(unittest.TestCase): """ Make sure option inputs work """ + def test_rendering(self): xml_str = ''' @@ -1626,6 +1670,7 @@ class TestStatus(unittest.TestCase): """ Tests for Status class """ + def test_str(self): """ Test stringifing Status objects diff --git a/xmodule/capa/tests/test_responsetypes.py b/xmodule/capa/tests/test_responsetypes.py index e8df8894c78f..6e52175c53fb 100644 --- a/xmodule/capa/tests/test_responsetypes.py +++ b/xmodule/capa/tests/test_responsetypes.py @@ -39,6 +39,8 @@ ) from xmodule.capa.util import convert_files_to_filenames from xmodule.capa.xqueue_interface import dateformat +import xmodule.capa.xqueue_submission as xqueue_submission +from xmodule.capa.xqueue_interface import get_flag_by_name class ResponseTest(unittest.TestCase): @@ -929,6 +931,7 @@ def test_empty_answer_graded_as_incorrect(self): self.assert_grade(problem, " ", "incorrect") +@pytest.mark.django_db class CodeResponseTest(ResponseTest): # pylint: disable=missing-class-docstring xml_factory_class = CodeResponseXMLFactory @@ -944,8 +947,12 @@ def setUp(self): @staticmethod def make_queuestate(key, time): """Create queuestate dict""" - timestr = datetime.strftime(time, dateformat) - return {'key': key, 'time': timestr} + if get_flag_by_name('send_to_submission_course.enable'): + timestr = datetime.strftime(time, xqueue_submission.dateformat) + return {'key': key, 'time': timestr} + else: + timestr = datetime.strftime(time, dateformat) + return {'key': key, 'time': timestr} def test_is_queued(self): """ diff --git a/xmodule/capa/tests/test_util.py b/xmodule/capa/tests/test_util.py index 51ffc623a982..dc356d39db49 100644 --- a/xmodule/capa/tests/test_util.py +++ b/xmodule/capa/tests/test_util.py @@ -145,7 +145,7 @@ def test_remove_markup(self): Test for markup removal with nh3. """ assert remove_markup('The Truth is Out There & you need to find it') ==\ - 'The Truth is Out There & you need to find it' + 'The Truth is Out There & you need to find it' @ddt.data( 'When the root level failš the whole hierarchy won’t work anymore.', diff --git a/xmodule/capa/tests/test_xqueue_interface.py b/xmodule/capa/tests/test_xqueue_interface.py index 819fd73c798b..bb73ed42f94e 100644 --- a/xmodule/capa/tests/test_xqueue_interface.py +++ b/xmodule/capa/tests/test_xqueue_interface.py @@ -1,50 +1,79 @@ """Test the XQueue service and interface.""" from unittest import TestCase -from unittest.mock import Mock +from unittest.mock import Mock, patch 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 +import pytest +import json from openedx.core.djangolib.testing.utils import skip_unless_lms from xmodule.capa.xqueue_interface import XQueueInterface, XQueueService +@pytest.mark.django_db @skip_unless_lms class XQueueServiceTest(TestCase): """Test the XQueue service methods.""" + def setUp(self): super().setUp() - location = BlockUsageLocator(CourseLocator("test_org", "test_course", "test_run"), "problem", "ExampleProblem") - self.block = Mock(scope_ids=ScopeIds('user1', 'mock_problem', location, location)) + location = BlockUsageLocator( + CourseLocator("test_org", "test_course", "test_run"), + "problem", + "ExampleProblem", + ) + self.block = Mock(scope_ids=ScopeIds("user1", "mock_problem", location, location)) + self.block.max_score = Mock(return_value=10) # Mock max_score method 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): - """Test that the XQueue callback is initialized correctly, and can be altered through the settings.""" + 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" + + @patch("xmodule.capa.xqueue_interface.is_flag_active", return_value=True) + def test_construct_callback_with_flag_enabled(self, mock_flag): + """Test construct_callback when the waffle flag is enabled.""" + usage_id = self.block.scope_ids.usage_id + course_id = str(usage_id.course_key) + callback_url = f"courses/{course_id}/xqueue/user1/{usage_id}" + + assert self.service.construct_callback() == f"{settings.LMS_ROOT_URL}/{callback_url}/score_update10" + assert self.service.construct_callback("alt_dispatch") == ( + f"{settings.LMS_ROOT_URL}/{callback_url}/alt_dispatch10" + ) + + 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_update10" + + @patch("xmodule.capa.xqueue_interface.is_flag_active", return_value=False) + def test_construct_callback_with_flag_disabled(self, mock_flag): + """Test construct_callback when the waffle flag is disabled.""" usage_id = self.block.scope_ids.usage_id - callback_url = f'courses/{usage_id.context_key}/xqueue/user1/{usage_id}' + course_id = str(usage_id.course_key) + callback_url = f"courses/{course_id}/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' + 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' + 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): """Check the format of the default queue name.""" - assert self.service.default_queuename == 'test_org-test_course' + 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.""" @@ -52,3 +81,61 @@ def test_waittime(self): with override_settings(XQUEUE_WAITTIME_BETWEEN_REQUESTS=15): assert self.service.waittime == 15 + + +@pytest.mark.django_db +@patch("xmodule.capa.xqueue_interface.is_flag_active", return_value=True) +@patch("xmodule.capa.xqueue_submission.XQueueInterfaceSubmission.send_to_submission") +def test_send_to_queue_with_flag_enabled(mock_send_to_submission, mock_flag): + """Test send_to_queue when the waffle flag is enabled.""" + url = "http://example.com/xqueue" + django_auth = {"username": "user", "password": "pass"} + xqueue_interface = XQueueInterface(url, django_auth) + + header = json.dumps({ + "lms_callback_url": ( + "http://example.com/courses/course-v1:test_org+test_course+test_run/" + "xqueue/block@item_id/type@problem" + ), + }) + body = json.dumps({ + "student_info": json.dumps({"anonymous_student_id": "student_id"}), + "student_response": "student_answer", + }) + files_to_upload = None + + mock_send_to_submission.return_value = {"submission": "mock_submission"} + error, msg = xqueue_interface.send_to_queue(header, body, files_to_upload) + + mock_send_to_submission.assert_called_once_with(header, body, {}) + + +@pytest.mark.django_db +@patch("xmodule.capa.xqueue_interface.is_flag_active", return_value=False) +@patch("xmodule.capa.xqueue_interface.XQueueInterface._http_post") +def test_send_to_queue_with_flag_disabled(mock_http_post, mock_flag): + """Test send_to_queue when the waffle flag is disabled.""" + url = "http://example.com/xqueue" + django_auth = {"username": "user", "password": "pass"} + xqueue_interface = XQueueInterface(url, django_auth) + + header = json.dumps({ + "lms_callback_url": ( + "http://example.com/courses/course-v1:test_org+test_course+test_run/" + "xqueue/block@item_id/type@problem" + ), + }) + body = json.dumps({ + "student_info": json.dumps({"anonymous_student_id": "student_id"}), + "student_response": "student_answer", + }) + files_to_upload = None + + mock_http_post.return_value = (0, "Submission sent successfully") + error, msg = xqueue_interface.send_to_queue(header, body, files_to_upload) + + mock_http_post.assert_called_once_with( + "http://example.com/xqueue/xqueue/submit/", + {"xqueue_header": header, "xqueue_body": body}, + files={}, + ) diff --git a/xmodule/capa/tests/test_xqueue_submission.py b/xmodule/capa/tests/test_xqueue_submission.py new file mode 100644 index 000000000000..51858ada03e1 --- /dev/null +++ b/xmodule/capa/tests/test_xqueue_submission.py @@ -0,0 +1,129 @@ +""" +Tests for XQueueInterfaceSubmission. +""" + +import json +import pytest +from unittest.mock import Mock, patch +from xmodule.capa.xqueue_submission import XQueueInterfaceSubmission +from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator +from xblock.fields import ScopeIds + + +@pytest.fixture +def xqueue_service(): + """ + Fixture that returns an instance of XQueueInterfaceSubmission. + """ + location = BlockUsageLocator( + CourseLocator("test_org", "test_course", "test_run"), + "problem", + "ExampleProblem" + ) + block = Mock(scope_ids=ScopeIds('user1', 'mock_problem', location, location)) + return XQueueInterfaceSubmission() + + +def test_extract_item_data(): + """ + Test extracting item data from an xqueue submission. + """ + header = json.dumps({ + 'lms_callback_url': ( + 'http://example.com/courses/course-v1:org+course+run/xqueue/5/' + 'block-v1:org+course+run+type@problem+block@item_id/5.0' + ), + 'queue_name': 'default' + }) + payload = json.dumps({ + 'student_info': json.dumps({'anonymous_student_id': 'student_id'}), + 'student_response': 'student_answer', + 'grader_payload': json.dumps({'grader': 'test.py'}) + }) + + student_item, student_answer, queue_name, grader, score = ( + XQueueInterfaceSubmission().extract_item_data(header, payload) + ) + + assert student_item == { + 'item_id': ( + 'block-v1:org+course+run+type@problem+block@item_id' + ), + 'item_type': 'problem', + 'course_id': 'course-v1:org+course+run', + 'student_id': 'student_id' + } + assert student_answer == 'student_answer' + assert queue_name == 'default' + assert grader == 'test.py' + assert score == 5.0 + + +@pytest.mark.django_db +@patch('submissions.api.create_submission') +def test_send_to_submission(mock_create_submission, xqueue_service): + """ + Test sending a submission to the grading system. + """ + header = json.dumps({ + 'lms_callback_url': ( + 'http://example.com/courses/course-v1:test_org+test_course+test_run/xqueue/5/' + 'block-v1:test_org+test_course+test_run+type@problem+block@item_id/' + '0.85' + ), + }) + body = json.dumps({ + 'student_info': json.dumps({'anonymous_student_id': 'student_id'}), + 'student_response': 'student_answer', + 'grader_payload': json.dumps({'grader': 'test.py'}) + }) + + with patch('lms.djangoapps.courseware.models.StudentModule.objects.filter') as mock_filter: + mock_filter.return_value.first.return_value = Mock(grade=0.85) + + mock_create_submission.return_value = {'submission': 'mock_submission'} + + # Call send_to_submission + result = xqueue_service.send_to_submission(header, body) + + # Assertions + assert 'submission' in result + assert result['submission'] == 'mock_submission' + mock_create_submission.assert_called_once_with( + { + 'item_id': 'block-v1:test_org+test_course+test_run+type@problem+block@item_id', + 'item_type': 'problem', + 'course_id': 'course-v1:test_org+test_course+test_run', + 'student_id': 'student_id' + }, + 'student_answer', + queue_name='default', + grader='test.py', + score=0.85 + ) + + +@pytest.mark.django_db +@patch('submissions.api.create_submission') +def test_send_to_submission_with_missing_fields(mock_create_submission, xqueue_service): + """ + Test send_to_submission with missing required fields. + """ + header = json.dumps({ + 'lms_callback_url': ( + 'http://example.com/courses/course-v1:test_org+test_course+test_run/xqueue/5/' + 'block@item_id/5.0' + ) + }) + body = json.dumps({ + 'student_info': json.dumps({'anonymous_student_id': 'student_id'}), + 'grader_payload': json.dumps({'grader': 'test.py'}) + }) + + # Call send_to_submission + result = xqueue_service.send_to_submission(header, body) + + # Assertions + assert "error" in result + assert "Validation error" in result["error"] + mock_create_submission.assert_not_called() diff --git a/xmodule/capa/xqueue_interface.py b/xmodule/capa/xqueue_interface.py index aee7232a4133..ef02950028e9 100644 --- a/xmodule/capa/xqueue_interface.py +++ b/xmodule/capa/xqueue_interface.py @@ -5,12 +5,14 @@ import hashlib import json +import re import logging import requests from django.conf import settings from django.urls import reverse from requests.auth import HTTPBasicAuth +from xmodule.capa.xqueue_submission import XQueueInterfaceSubmission if TYPE_CHECKING: from xmodule.capa_block import ProblemBlock @@ -26,6 +28,34 @@ READ_TIMEOUT = 10 # seconds +def is_flag_active(flag_name, course_id): + """ + Look for the waffle flag by name and course_id. + """ + from openedx.core.djangoapps.waffle_utils.models import WaffleFlagCourseOverrideModel as waffle + flag = waffle.objects.filter(waffle_flag=flag_name, course_id=course_id, enabled=True).first() + return flag and flag.enabled + + +def get_flag_by_name(flag_name): + """ + Look for the waffle flag by name. + """ + from openedx.core.djangoapps.waffle_utils.models import WaffleFlagCourseOverrideModel + flag = WaffleFlagCourseOverrideModel.objects.filter(waffle_flag=flag_name, enabled=True).first() + return flag and flag.enabled + + +def get_course_id(callback_url): + """ + Extract course_id from the callback URL + """ + course_id_match = re.search(r'(course-v1:[^\/]+)', callback_url) + if not course_id_match: + raise ValueError("The callback_url does not contain the required information.") + return course_id_match.group(1) + + def make_hashkey(seed): """ Generate a string key by hashing @@ -135,7 +165,16 @@ def _send_to_queue(self, header, body, files_to_upload): # lint-amnesty, pylint for f in files_to_upload: files.update({f.name: f}) - return self._http_post(self.url + '/xqueue/submit/', payload, files=files) + header_info = json.loads(header) + course_id = get_course_id(header_info['lms_callback_url']) + if is_flag_active('send_to_submission_course.enable', course_id): + # Use the new edx-submissions workflow + submission = XQueueInterfaceSubmission().send_to_submission(header, body, files) + log.error(submission) + return None, '' + + else: + return self._http_post(self.url + '/xqueue/submit/', payload, files=files) def _http_post(self, url, data, files=None): # lint-amnesty, pylint: disable=missing-function-docstring try: @@ -165,6 +204,7 @@ class XQueueService: """ def __init__(self, block: 'ProblemBlock'): + #breakpoint() basic_auth = settings.XQUEUE_INTERFACE.get('basic_auth') requests_auth = HTTPBasicAuth(*basic_auth) if basic_auth else None self._interface = XQueueInterface( @@ -184,17 +224,32 @@ def construct_callback(self, dispatch: str = 'score_update') -> str: """ Return a fully qualified callback URL for external queueing system. """ + dispatch_callback = "callback_submission" relative_xqueue_callback_url = reverse( - 'xqueue_callback', + dispatch_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) + lms_callback_url = xqueue_callback_url_prefix + relative_xqueue_callback_url + course_id = get_course_id(lms_callback_url) + if is_flag_active('send_to_submission_course.enable', course_id): + return xqueue_callback_url_prefix + relative_xqueue_callback_url + str(self._block.max_score()) + else: + dispatch_callback = 'xqueue_callback' + relative_xqueue_callback_url = reverse( + dispatch_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 + )) + 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) -> str: diff --git a/xmodule/capa/xqueue_submission.py b/xmodule/capa/xqueue_submission.py new file mode 100644 index 000000000000..414218f5cd52 --- /dev/null +++ b/xmodule/capa/xqueue_submission.py @@ -0,0 +1,127 @@ +""" +This module provides an interface for submitting student responses +to an external grading system through XQueue. +""" + +import json +import logging +import re +from opaque_keys.edx.keys import CourseKey + +log = logging.getLogger(__name__) + +class XQueueInterfaceSubmission: + """Interface to the external grading system.""" + + def _parse_json(self, data, name): + """Helper function to parse JSON safely.""" + try: + return json.loads(data) if isinstance(data, str) else data + except json.JSONDecodeError as e: + raise ValueError(f"Error parsing {name}: {e}") from e + + def _extract_identifiers(self, callback_url): + """Extracts identifiers from the callback URL.""" + item_id_match = re.search(r'block@([^\/]+)', callback_url) + item_type_match = re.search(r'type@([^+]+)', callback_url) + course_id_match = re.search(r'(course-v1:[^\/]+)', callback_url) + max_score_match = re.search(r'(\d+(\.\d+)?)$', callback_url) + + if not all([item_id_match, item_type_match, course_id_match, max_score_match]): + raise ValueError("The callback_url does not contain the required information.") + + return ( + item_id_match.group(1), + item_type_match.group(1), + course_id_match.group(1), + float(max_score_match.group(1)) + ) + + def extract_item_data(self, header, payload): + """ + Extracts student submission data from the given header and payload. + """ + + header = self._parse_json(header, "header") + payload = self._parse_json(payload, "payload") + + callback_url = header.get('lms_callback_url') + queue_name = header.get('queue_name', 'default') + + if not callback_url: + raise ValueError("The header does not contain 'lms_callback_url'.") + + item_id, item_type, course_id, max_score = self._extract_identifiers(callback_url) + + student_info = self._parse_json(payload["student_info"], "student_info") + + full_block_id = None + + try: + full_block_id = ( + f"block-v1:{course_id.replace('course-v1:', '')}+type@{item_type}+block@{item_id}" + ) + except Exception as e: + raise ValueError( + f"Error creating BlockUsageLocator. Invalid ID: {full_block_id}, Error: {e}" + ) from e + + try: + course_key = CourseKey.from_string(course_id) + except Exception as e: + raise ValueError(f"Error creating CourseKey: {e}") from e + + try: + grader_payload = self._parse_json(payload["grader_payload"], "grader_payload") + grader = grader_payload.get("grader", '') + except KeyError as e: + raise ValueError(f"Error in payload: {e}") from e + + student_id = student_info.get("anonymous_student_id") + if not student_id: + raise ValueError("The field 'anonymous_student_id' is missing from student_info.") + + student_dict = { + 'item_id': full_block_id, + 'item_type': item_type, + 'course_id': course_id, + 'student_id': student_id + } + + student_answer = payload.get("student_response") + if student_answer is None: + raise ValueError("The field 'student_response' does not exist.") + + score = max_score + + return student_dict, student_answer, queue_name, grader, score + + def send_to_submission(self, header, body, files_to_upload=None): + """ + Submits the extracted student data to the edx-submissions system. + """ + from submissions.api import create_submission + + try: + student_item, answer, queue_name, grader, score = self.extract_item_data(header, body) + return create_submission(student_item, answer, queue_name=queue_name, grader=grader, score=score) + + except json.JSONDecodeError as e: + log.error("JSON decoding error: %s", e) + return {"error": "Invalid JSON format"} + + except KeyError as e: + log.error("Missing key: %s", e) + return {"error": f"Missing key: {e}"} + + except ValueError as e: + log.error("Validation error: %s", e) + return {"error": f"Validation error: {e}"} + + except TypeError as e: + log.error("Type error: %s", e) + return {"error": f"Type error: {e}"} + + except RuntimeError as e: + log.error("Runtime error: %s", e) + return {"error": f"Runtime error: {e}"} diff --git a/xmodule/docs/decisions/0005-send-data-to-edx-submission.rst b/xmodule/docs/decisions/0005-send-data-to-edx-submission.rst new file mode 100644 index 000000000000..5e8d45367277 --- /dev/null +++ b/xmodule/docs/decisions/0005-send-data-to-edx-submission.rst @@ -0,0 +1,220 @@ +######################################################### +Implementation to send student response to edx-submission +######################################################### + +Status +****** + +Accepted. + +2025-02-21 + +Context +******* + +On the Open edX platform, student responses to assignments are traditionally submitted to the `XQueue` service for assessment. However, a need was identified to allow certain responses to be submitted to the `edx-submission` service, which offers a more modern and efficient architecture for handling submissions. To facilitate a controlled transition and allow for A/B testing, the introduction of a *waffle flag* was proposed to enable dynamic selection of the submission service based on the specific course. + +Decision +******** + +A course-level waffle flag called `send_to_submission_course.enable` has been implemented. This flag can be set via the Django admin, allowing administrators to enable or disable the `edx-submission` submission functionality for specific courses without requiring any code changes. + +Key changes include: + +1. **Waffle Flag Definition**: The `send_to_submission_course.enable` flag was created in the Django admin, associating it with the corresponding `course_id`. + +2. **`xqueue_interfaces.py` Modification**: In the `xmodule/capa/xqueue_interfaces.py` file, a condition was added that checks the state of the *waffle flag*. If the flag is on for a given course, student responses are sent to the `edx-submission` service using the `send_to_submission` function. If the flag is off, the flow continues sending responses to `XQueue`. + +3. **`construct_callback` Method Update**: Within the `XQueueService` class in `xqueue_interfaces.py`, the `construct_callback` method was modified. This method generates the callback URL that `XQueue` or `edx-submission` will use to return the evaluation results. The method now checks the state of the `send_to_submission_course.enable` *waffle flag* to determine whether the callback URL should point to the `edx-submission` handler (`callback_submission`) or to the original `XQueue` handler (`xqueue_callback`). + + Additionally, the method now appends the `max_score` value to the callback URL when using `edx-submission`. This ensures that `xqueue_submission.py` can extract and pass the `max_score` value correctly to `create_submission`. + + **Updated Logic in `construct_callback`**: + .. code-block:: python + + if is_flag_active('send_to_submission_course.enable', course_id): + return f"{xqueue_callback_url_prefix}{relative_xqueue_callback_url}{self.block.max_score()}" + else: + return f"{xqueue_callback_url_prefix}{relative_xqueue_callback_url}" + +4. **Implementation of `send_to_submission` in `xqueue_submission.py`**: The `send_to_submission` function was developed in the `xqueue_submission.py` file. This function is responsible for: + - **Parse Submission Data**: Extracts and processes relevant information from the student response, including identifiers such as `course_id`, `item_id`, `student_id`, and now `max_score`. + + - **Extraction of `max_score` from Callback URL**: The function was updated to extract `max_score` using a regex pattern from the callback URL. + + .. code-block:: python + + def _extract_identifiers(self, callback_url): + max_score_match = re.search(r'(\d+(\.\d+)?)$', callback_url) + if max_score_match: + max_score = float(max_score_match.group(1)) + else: + raise ValueError("The callback URL does not contain max_score.") + return max_score + + - **Interaction with `edx-submission`**: Uses the API provided by `edx-submission` to create a new submission record in the submissions database, ensuring that the student response is stored and processed appropriately with the correct `max_score`. + + .. code-block:: python + + def send_to_submission(self, header, body, files_to_upload=None): + student_item, student_answer, queue_name, grader, max_score = self.extract_item_data(header, body) + return create_submission(student_item, student_answer, queue_name=queue_name, grader=grader, score=max_score) + +Configuration for Xqueue-watcher: + +Prerequisites +------------- + +- Ensure you have the Xqueue repository cloned and running. +- Basic understanding of microservices and Docker (if applicable). + +1. Setting up Xqueue-Server +--------------------------- + +**Requirements:** + +First, clone the Xqueue repository into your environment: + +.. code-block:: bash + + git clone https://github.com/openedx/xqueue.git + +**Steps:** + +1. Run the service using either Docker or as a microservice within your environment. +2. Make sure to expose port **18040** since the service listens on this port: + + :: + + http://127.0.0.1:18040 + +3. Verify that the service is running by accessing the configured URL or checking the service logs. + +2. Configuring Xqueue-Watcher +----------------------------- + +**Installation:** + +1. Clone the Xqueue-Watcher repository: + +.. code-block:: bash + + git clone https://github.com/openedx/xqueue-watcher.git + +2. Navigate to the project directory and install any necessary dependencies: + +.. code-block:: bash + + cd xqueue-watcher + pip install -r requirements.txt + +**Configuration:** + +1. Locate the configuration file at: + + :: + + config/conf.d/course.json + +2. Update the `course.json` file with the following configuration: + +.. code-block:: json + + { + "test-123": { + "SERVER": "http://127.0.0.1:18040", + "CONNECTIONS": 1, + "AUTH": ["username", "password"] + } + } + +- **test-123**: The name of the queue to listen to. +- **SERVER**: The Xqueue server address. +- **AUTH**: A list containing `[username, password]` for Xqueue Django user authentication. +- **CONNECTIONS**: Number of threads to spawn to watch the queue. + +3. Start the Xqueue-Watcher service: + +.. code-block:: bash + + python watcher.py + +3. Setting up Xqueue-Submission +------------------------------- + +This new flow sends queue data to **edx-submission** for processing. + +**Steps:** + +1. Create a new instance of Xqueue-Watcher: + +.. code-block:: bash + + git clone https://github.com/openedx/xqueue-watcher.git + +2. Configure the new instance to listen on port **8000**. Edit the `course.json` file located at: + + :: + + config/conf.d/course.json + +3. Add the following configuration: + +.. code-block:: json + + { + "test-123": { + "SERVER": "http://127.0.0.1:8000", + "CONNECTIONS": 1, + "AUTH": ["username", "password"] + } + } + +- **SERVER**: Now points to port **8000**, where edx-submission is running. + +4. Start the new instance of Xqueue-Watcher: + +.. code-block:: bash + + python watcher.py + +4. Verification +--------------- + +1. Ensure that all services are running: + - Verify that ports **18040** and **8000** are active. + - Check the logs for connection errors or authentication issues. + +2. Test the configuration: + - Send a test request to the queue `test-123` to confirm data processing through **edx-submission**. + + +Consequences +************ + +**Positives:** + +- **Flexibility**: Administrators can enable or disable the `edx-submission` submission functionality on a per-course basis, facilitating controlled testing and a smooth transition. + +- **Improved Submission Handling**: By using `edx-submission`, you can take advantage of a more modern architecture for processing responses. + +**Negatives:** + +- **Additional Complexity**: The introduction of a new flag-based flow adds complexity to the code, which can increase maintenance effort. + +- **Potential Inconsistency**: If flag states are not properly managed, there could be inconsistencies in submission handling across courses. + +References +********** + +- **Relevant commits**: [Implementation of the Waffle Flag and modification of xqueue_interfaces.py](https://github.com/aulasneo/edx-platform/commit/f50afcc301bdc3eeb42a 6dc2c051ffb2d799f868#diff-9b4290d2b574f54e4eca7831368727f7ddbac8292aa75ba4b28651d4bf2bbe6b) + +- **Feature Toggles documentation in Open edX**: [Feature Toggles — edx-platform documentation](https://docs.openedx.org/projects/edx-platform/en/latest/references/featuretoggles.html) + +- **edx-submissions repository**: [openedx/edx-submissions](https://github.com/openedx/edx-submissions) + +- **edx-platform repository**: [openedx/edx-platform](https://github.com/openedx/edx-platform) + +- **xqueue repository**: [openedx/xqueue](https://github.com/openedx/xqueue) + +- **xqueue-watcher repository**: [openedx/xqueue-watcher](https://github.com/openedx/xqueue-watcher) From 4575be53145caffa5e5a86dcdc7e001a2bdebd8d Mon Sep 17 00:00:00 2001 From: gabrielC1409 Date: Thu, 27 Feb 2025 18:17:27 -0400 Subject: [PATCH 2/3] fix: Refactoring of sending information to sent_to_submission --- xmodule/capa/capa_problem.py | 2 - xmodule/capa/errors.py | 50 +++++++ xmodule/capa/inputtypes.py | 4 +- xmodule/capa/tests/test_capa_problem.py | 22 ++-- xmodule/capa/tests/test_errors.py | 43 ++++++ xmodule/capa/tests/test_inputtypes.py | 45 ------- xmodule/capa/tests/test_responsetypes.py | 11 +- xmodule/capa/tests/test_xqueue_interface.py | 27 ++-- xmodule/capa/tests/test_xqueue_submission.py | 34 ++--- xmodule/capa/xqueue_interface.py | 71 +++++----- xmodule/capa/xqueue_submission.py | 122 ++++++++---------- .../0005-send-data-to-edx-submission.rst | 32 +---- 12 files changed, 231 insertions(+), 232 deletions(-) create mode 100644 xmodule/capa/errors.py create mode 100644 xmodule/capa/tests/test_errors.py diff --git a/xmodule/capa/capa_problem.py b/xmodule/capa/capa_problem.py index 831454860146..3bf5b78cd4a3 100644 --- a/xmodule/capa/capa_problem.py +++ b/xmodule/capa/capa_problem.py @@ -32,8 +32,6 @@ import xmodule.capa.inputtypes as inputtypes import xmodule.capa.responsetypes as responsetypes import xmodule.capa.xqueue_interface as xqueue_interface -import xmodule.capa.xqueue_submission as xqueue_submission -from xmodule.capa.xqueue_interface import get_flag_by_name from xmodule.capa.correctmap import CorrectMap from xmodule.capa.safe_exec import safe_exec from xmodule.capa.util import contextualize_text, convert_files_to_filenames, get_course_id_from_capa_block diff --git a/xmodule/capa/errors.py b/xmodule/capa/errors.py new file mode 100644 index 000000000000..f972c689ad39 --- /dev/null +++ b/xmodule/capa/errors.py @@ -0,0 +1,50 @@ +# errors.py +""" +Custom error handling for the XQueue submission interface. +""" + +class XQueueSubmissionError(Exception): + """Base class for all XQueue submission errors.""" + # No es necesario el `pass`, la clase ya hereda de Exception. + +class JSONParsingError(XQueueSubmissionError): + """Raised when JSON parsing fails.""" + MESSAGE = "Error parsing {name}: {error}" + + def __init__(self, name, error): + super().__init__(self.MESSAGE.format(name=name, error=error)) + +class MissingKeyError(XQueueSubmissionError): + """Raised when a required key is missing.""" + MESSAGE = "Missing key: {key}" + + def __init__(self, key): + super().__init__(self.MESSAGE.format(key=key)) + +class ValidationError(XQueueSubmissionError): + """Raised when a validation check fails.""" + MESSAGE = "Validation error: {error}" + + def __init__(self, error): + super().__init__(self.MESSAGE.format(error=error)) + +class TypeErrorSubmission(XQueueSubmissionError): + """Raised when an invalid type is encountered.""" + MESSAGE = "Type error: {error}" + + def __init__(self, error): + super().__init__(self.MESSAGE.format(error=error)) + +class RuntimeErrorSubmission(XQueueSubmissionError): + """Raised for runtime errors.""" + MESSAGE = "Runtime error: {error}" + + def __init__(self, error): + super().__init__(self.MESSAGE.format(error=error)) + +class GetSubmissionParamsError(XQueueSubmissionError): + """Raised when there is an issue getting submission parameters.""" + MESSAGE = "Block instance is not defined!" + + def __init__(self): + super().__init__(self.MESSAGE) diff --git a/xmodule/capa/inputtypes.py b/xmodule/capa/inputtypes.py index df9e48b09b73..03a9a59134d0 100644 --- a/xmodule/capa/inputtypes.py +++ b/xmodule/capa/inputtypes.py @@ -56,11 +56,11 @@ from lxml import etree -from xmodule.capa.xqueue_interface import XQUEUE_TIMEOUT, get_flag_by_name +from xmodule.capa.xqueue_interface import XQUEUE_TIMEOUT from openedx.core.djangolib.markup import HTML, Text from xmodule.stringify import stringify_children -from . import xqueue_interface, xqueue_submission +from . import xqueue_interface from .registry import TagRegistry from .util import sanitize_html diff --git a/xmodule/capa/tests/test_capa_problem.py b/xmodule/capa/tests/test_capa_problem.py index 4e43efb50534..74cf4d096fd1 100644 --- a/xmodule/capa/tests/test_capa_problem.py +++ b/xmodule/capa/tests/test_capa_problem.py @@ -54,7 +54,7 @@ def test_label_and_description_inside_responsetype(self, question): """.format(question=question) problem = new_loncapa_problem(xml) assert problem.problem_data ==\ - {'1_2_1': {'label': question, 'descriptions': {'description_1_1_1': 'Only the paranoid survive.'}}} + {'1_2_1': {'label': question, 'descriptions': {'description_1_1_1': 'Only the paranoid survive.'}}} assert len(problem.tree.xpath('//label')) == 0 @ddt.unpack @@ -123,7 +123,7 @@ def test_neither_label_tag_nor_attribute(self, question1, question2): """.format(question1, question2) problem = new_loncapa_problem(xml) assert problem.problem_data ==\ - {'1_2_1': {'label': question1, 'descriptions': {}}, '1_3_1': {'label': question2, 'descriptions': {}}} + {'1_2_1': {'label': question1, 'descriptions': {}}, '1_3_1': {'label': question2, 'descriptions': {}}} for question in (question1, question2): assert len(problem.tree.xpath('//label[text()="{}"]'.format(question))) == 0 @@ -146,8 +146,8 @@ def test_multiple_descriptions(self): """.format(desc1, desc2) problem = new_loncapa_problem(xml) assert problem.problem_data ==\ - {'1_2_1': {'label': '___ requires sacrifices.', - 'descriptions': {'description_1_1_1': desc1, 'description_1_1_2': desc2}}} + {'1_2_1': {'label': '___ requires sacrifices.', + 'descriptions': {'description_1_1_1': desc1, 'description_1_1_2': desc2}}} def test_additional_answer_is_skipped_from_resulting_html(self): """Tests that additional_answer element is not present in transformed HTML""" @@ -236,10 +236,10 @@ def test_multiple_questions_problem(self): """ problem = new_loncapa_problem(xml) assert problem.problem_data ==\ - {'1_2_1': {'label': 'Select the correct synonym of paranoid?', - 'descriptions': {'description_1_1_1': 'Only the paranoid survive.'}}, - '1_3_1': {'label': 'What Apple device competed with the portable CD player?', - 'descriptions': {'description_1_2_1': 'Device looks like an egg plant.'}}} + {'1_2_1': {'label': 'Select the correct synonym of paranoid?', + 'descriptions': {'description_1_1_1': 'Only the paranoid survive.'}}, + '1_3_1': {'label': 'What Apple device competed with the portable CD player?', + 'descriptions': {'description_1_2_1': 'Device looks like an egg plant.'}}} assert len(problem.tree.xpath('//label')) == 0 def test_question_title_not_removed_got_children(self): @@ -291,8 +291,8 @@ def test_multiple_inputtypes(self, group_label): problem = new_loncapa_problem(xml) assert problem.problem_data ==\ - {'1_2_1': {'group_label': group_label, 'label': input1_label, 'descriptions': {}}, - '1_2_2': {'group_label': group_label, 'label': input2_label, 'descriptions': {}}} + {'1_2_1': {'group_label': group_label, 'label': input1_label, 'descriptions': {}}, + '1_2_2': {'group_label': group_label, 'label': input2_label, 'descriptions': {}}} def test_single_inputtypes(self): """ @@ -355,7 +355,7 @@ def assert_question_tag(self, question1, question2, tag, label_attr=False): ) problem = new_loncapa_problem(xml) assert problem.problem_data ==\ - {'1_2_1': {'label': question1, 'descriptions': {}}, '1_3_1': {'label': question2, 'descriptions': {}}} + {'1_2_1': {'label': question1, 'descriptions': {}}, '1_3_1': {'label': question2, 'descriptions': {}}} assert len(problem.tree.xpath('//{}'.format(tag))) == 0 @ddt.unpack diff --git a/xmodule/capa/tests/test_errors.py b/xmodule/capa/tests/test_errors.py new file mode 100644 index 000000000000..412839a1ff29 --- /dev/null +++ b/xmodule/capa/tests/test_errors.py @@ -0,0 +1,43 @@ +""" +Unit tests for custom error handling in the XQueue submission interface. +""" + +import pytest +from xmodule.capa.errors import ( + JSONParsingError, + MissingKeyError, + ValidationError, + TypeErrorSubmission, + RuntimeErrorSubmission, + GetSubmissionParamsError +) + +def test_json_parsing_error(): + with pytest.raises(JSONParsingError) as excinfo: + raise JSONParsingError("test_name", "test_error") + assert str(excinfo.value) == "Error parsing test_name: test_error" + +def test_missing_key_error(): + with pytest.raises(MissingKeyError) as excinfo: + raise MissingKeyError("test_key") + assert str(excinfo.value) == "Missing key: test_key" + +def test_validation_error(): + with pytest.raises(ValidationError) as excinfo: + raise ValidationError("test_error") + assert str(excinfo.value) == "Validation error: test_error" + +def test_type_error_submission(): + with pytest.raises(TypeErrorSubmission) as excinfo: + raise TypeErrorSubmission("test_error") + assert str(excinfo.value) == "Type error: test_error" + +def test_runtime_error_submission(): + with pytest.raises(RuntimeErrorSubmission) as excinfo: + raise RuntimeErrorSubmission("test_error") + assert str(excinfo.value) == "Runtime error: test_error" + +def test_get_submission_params_error(): + with pytest.raises(GetSubmissionParamsError) as excinfo: + raise GetSubmissionParamsError() + assert str(excinfo.value) == "Block instance is not defined!" diff --git a/xmodule/capa/tests/test_inputtypes.py b/xmodule/capa/tests/test_inputtypes.py index 5f585fbe8450..4e14bc42b76b 100644 --- a/xmodule/capa/tests/test_inputtypes.py +++ b/xmodule/capa/tests/test_inputtypes.py @@ -476,12 +476,10 @@ def test_rendering(self): assert context == expected -@pytest.mark.django_db class MatlabTest(unittest.TestCase): """ Test Matlab input types """ - def setUp(self): super(MatlabTest, self).setUp() # lint-amnesty, pylint: disable=super-with-arguments self.rows = '10' @@ -930,40 +928,6 @@ def test_matlab_sanitize_msg(self): expected = "" assert self.the_input._get_render_context()['msg'] == expected # pylint: disable=protected-access - @patch('xmodule.capa.inputtypes.get_flag_by_name', return_value=True) - @patch('xmodule.capa.inputtypes.datetime') - def test_plot_data_with_flag_active(self, mock_datetime, mock_get_flag_by_name): - """ - Test that the correct date format is used when the flag is active. - """ - mock_datetime.utcnow.return_value.strftime.return_value = 'formatted_date_with_flag' - data = {'submission': 'x = 1234;'} - response = self.the_input.handle_ajax("plot", data) - self.the_input.capa_system.xqueue.interface.send_to_queue.assert_called_with(header=ANY, body=ANY) - assert response['success'] - assert self.the_input.input_state['queuekey'] is not None - assert self.the_input.input_state['queuestate'] == 'queued' - assert 'formatted_date_with_flag' in self.the_input.capa_system.xqueue.interface.send_to_queue.call_args[1][ - 'body' - ] - - @patch('xmodule.capa.inputtypes.get_flag_by_name', return_value=False) - @patch('xmodule.capa.inputtypes.datetime') - def test_plot_data_with_flag_inactive(self, mock_datetime, mock_get_flag_by_name): - """ - Test that the correct date format is used when the flag is inactive. - """ - mock_datetime.utcnow.return_value.strftime.return_value = 'formatted_date_without_flag' - data = {'submission': 'x = 1234;'} - response = self.the_input.handle_ajax("plot", data) - self.the_input.capa_system.xqueue.interface.send_to_queue.assert_called_with(header=ANY, body=ANY) - assert response['success'] - assert self.the_input.input_state['queuekey'] is not None - assert self.the_input.input_state['queuestate'] == 'queued' - assert 'formatted_date_without_flag' in self.the_input.capa_system.xqueue.interface.send_to_queue.call_args[1][ - 'body' - ] - def html_tree_equal(received, expected): """ @@ -983,7 +947,6 @@ class SchematicTest(unittest.TestCase): """ Check that schematic inputs work """ - def test_rendering(self): height = '12' width = '33' @@ -1039,7 +1002,6 @@ class ImageInputTest(unittest.TestCase): """ Check that image inputs work """ - def check(self, value, egx, egy): # lint-amnesty, pylint: disable=missing-function-docstring height = '78' width = '427' @@ -1099,7 +1061,6 @@ class CrystallographyTest(unittest.TestCase): """ Check that crystallography inputs work """ - def test_rendering(self): height = '12' width = '33' @@ -1141,7 +1102,6 @@ class VseprTest(unittest.TestCase): """ Check that vsepr inputs work """ - def test_rendering(self): height = '12' width = '33' @@ -1189,7 +1149,6 @@ class ChemicalEquationTest(unittest.TestCase): """ Check that chemical equation inputs work. """ - def setUp(self): super(ChemicalEquationTest, self).setUp() # lint-amnesty, pylint: disable=super-with-arguments self.size = "42" @@ -1285,7 +1244,6 @@ class FormulaEquationTest(unittest.TestCase): """ Check that formula equation inputs work. """ - def setUp(self): super(FormulaEquationTest, self).setUp() # lint-amnesty, pylint: disable=super-with-arguments self.size = "42" @@ -1434,7 +1392,6 @@ class DragAndDropTest(unittest.TestCase): """ Check that drag and drop inputs work """ - def test_rendering(self): path_to_images = '/dummy-static/images/' @@ -1509,7 +1466,6 @@ class AnnotationInputTest(unittest.TestCase): """ Make sure option inputs work """ - def test_rendering(self): xml_str = ''' @@ -1670,7 +1626,6 @@ class TestStatus(unittest.TestCase): """ Tests for Status class """ - def test_str(self): """ Test stringifing Status objects diff --git a/xmodule/capa/tests/test_responsetypes.py b/xmodule/capa/tests/test_responsetypes.py index 6e52175c53fb..e8df8894c78f 100644 --- a/xmodule/capa/tests/test_responsetypes.py +++ b/xmodule/capa/tests/test_responsetypes.py @@ -39,8 +39,6 @@ ) from xmodule.capa.util import convert_files_to_filenames from xmodule.capa.xqueue_interface import dateformat -import xmodule.capa.xqueue_submission as xqueue_submission -from xmodule.capa.xqueue_interface import get_flag_by_name class ResponseTest(unittest.TestCase): @@ -931,7 +929,6 @@ def test_empty_answer_graded_as_incorrect(self): self.assert_grade(problem, " ", "incorrect") -@pytest.mark.django_db class CodeResponseTest(ResponseTest): # pylint: disable=missing-class-docstring xml_factory_class = CodeResponseXMLFactory @@ -947,12 +944,8 @@ def setUp(self): @staticmethod def make_queuestate(key, time): """Create queuestate dict""" - if get_flag_by_name('send_to_submission_course.enable'): - timestr = datetime.strftime(time, xqueue_submission.dateformat) - return {'key': key, 'time': timestr} - else: - timestr = datetime.strftime(time, dateformat) - return {'key': key, 'time': timestr} + timestr = datetime.strftime(time, dateformat) + return {'key': key, 'time': timestr} def test_is_queued(self): """ diff --git a/xmodule/capa/tests/test_xqueue_interface.py b/xmodule/capa/tests/test_xqueue_interface.py index bb73ed42f94e..07529bafe8b2 100644 --- a/xmodule/capa/tests/test_xqueue_interface.py +++ b/xmodule/capa/tests/test_xqueue_interface.py @@ -46,30 +46,27 @@ def test_construct_callback_with_flag_enabled(self, mock_flag): course_id = str(usage_id.course_key) callback_url = f"courses/{course_id}/xqueue/user1/{usage_id}" - assert self.service.construct_callback() == f"{settings.LMS_ROOT_URL}/{callback_url}/score_update10" + 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_dispatch10" + 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_update10" + assert self.service.construct_callback() == f"{custom_callback_url}/{callback_url}/score_update" @patch("xmodule.capa.xqueue_interface.is_flag_active", return_value=False) def test_construct_callback_with_flag_disabled(self, mock_flag): """Test construct_callback when the waffle flag is disabled.""" usage_id = self.block.scope_ids.usage_id - course_id = str(usage_id.course_key) - callback_url = f"courses/{course_id}/xqueue/user1/{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" - ) + 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" + 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): """Check the format of the default queue name.""" @@ -90,7 +87,8 @@ def test_send_to_queue_with_flag_enabled(mock_send_to_submission, mock_flag): """Test send_to_queue when the waffle flag is enabled.""" url = "http://example.com/xqueue" django_auth = {"username": "user", "password": "pass"} - xqueue_interface = XQueueInterface(url, django_auth) + block = Mock() # Mock block for the constructor + xqueue_interface = XQueueInterface(url, django_auth, block=block) header = json.dumps({ "lms_callback_url": ( @@ -117,7 +115,8 @@ def test_send_to_queue_with_flag_disabled(mock_http_post, mock_flag): """Test send_to_queue when the waffle flag is disabled.""" url = "http://example.com/xqueue" django_auth = {"username": "user", "password": "pass"} - xqueue_interface = XQueueInterface(url, django_auth) + block = Mock() # Mock block for the constructor + xqueue_interface = XQueueInterface(url, django_auth, block=block) header = json.dumps({ "lms_callback_url": ( diff --git a/xmodule/capa/tests/test_xqueue_submission.py b/xmodule/capa/tests/test_xqueue_submission.py index 51858ada03e1..158bcf73310a 100644 --- a/xmodule/capa/tests/test_xqueue_submission.py +++ b/xmodule/capa/tests/test_xqueue_submission.py @@ -21,17 +21,17 @@ def xqueue_service(): "ExampleProblem" ) block = Mock(scope_ids=ScopeIds('user1', 'mock_problem', location, location)) - return XQueueInterfaceSubmission() + return XQueueInterfaceSubmission(block) -def test_extract_item_data(): +def test_get_submission_params(xqueue_service): """ Test extracting item data from an xqueue submission. """ header = json.dumps({ 'lms_callback_url': ( - 'http://example.com/courses/course-v1:org+course+run/xqueue/5/' - 'block-v1:org+course+run+type@problem+block@item_id/5.0' + 'http://example.com/courses/course-v1:test_org+test_course+test_run/xqueue/5/' + 'block-v1:test_org+test_course+test_run+type@problem+block@item_id/' ), 'queue_name': 'default' }) @@ -42,21 +42,21 @@ def test_extract_item_data(): }) student_item, student_answer, queue_name, grader, score = ( - XQueueInterfaceSubmission().extract_item_data(header, payload) + xqueue_service.get_submission_params(header, payload) ) assert student_item == { 'item_id': ( - 'block-v1:org+course+run+type@problem+block@item_id' + 'block-v1:test_org+test_course+test_run+type@mock_problem+block@ExampleProblem' ), - 'item_type': 'problem', - 'course_id': 'course-v1:org+course+run', + 'item_type': 'mock_problem', + 'course_id': 'course-v1:test_org+test_course+test_run', 'student_id': 'student_id' } assert student_answer == 'student_answer' assert queue_name == 'default' assert grader == 'test.py' - assert score == 5.0 + assert score == xqueue_service.block.max_score() # Mocked max_score value @pytest.mark.django_db @@ -68,8 +68,7 @@ def test_send_to_submission(mock_create_submission, xqueue_service): header = json.dumps({ 'lms_callback_url': ( 'http://example.com/courses/course-v1:test_org+test_course+test_run/xqueue/5/' - 'block-v1:test_org+test_course+test_run+type@problem+block@item_id/' - '0.85' + 'block-v1:test_org+test_course+test_run+type@mock_problem+block@ExampleProblem/' ), }) body = json.dumps({ @@ -79,7 +78,7 @@ def test_send_to_submission(mock_create_submission, xqueue_service): }) with patch('lms.djangoapps.courseware.models.StudentModule.objects.filter') as mock_filter: - mock_filter.return_value.first.return_value = Mock(grade=0.85) + mock_filter.return_value.first.return_value = Mock(grade=10) mock_create_submission.return_value = {'submission': 'mock_submission'} @@ -91,15 +90,16 @@ def test_send_to_submission(mock_create_submission, xqueue_service): assert result['submission'] == 'mock_submission' mock_create_submission.assert_called_once_with( { - 'item_id': 'block-v1:test_org+test_course+test_run+type@problem+block@item_id', - 'item_type': 'problem', + 'item_id': 'block-v1:test_org+test_course+test_run+type@mock_problem+block@ExampleProblem', + 'item_type': 'mock_problem', 'course_id': 'course-v1:test_org+test_course+test_run', 'student_id': 'student_id' }, 'student_answer', - queue_name='default', + queue_name='default', grader='test.py', - score=0.85 + score=xqueue_service.block.max_score(), # Mocked max_score value + file=None ) @@ -112,7 +112,7 @@ def test_send_to_submission_with_missing_fields(mock_create_submission, xqueue_s header = json.dumps({ 'lms_callback_url': ( 'http://example.com/courses/course-v1:test_org+test_course+test_run/xqueue/5/' - 'block@item_id/5.0' + 'block@item_id/' ) }) body = json.dumps({ diff --git a/xmodule/capa/xqueue_interface.py b/xmodule/capa/xqueue_interface.py index ef02950028e9..a3d2bd7609bc 100644 --- a/xmodule/capa/xqueue_interface.py +++ b/xmodule/capa/xqueue_interface.py @@ -106,11 +106,15 @@ class XQueueInterface: Interface to the external grading system """ - def __init__(self, url: str, django_auth: Dict[str, str], requests_auth: Optional[HTTPBasicAuth] = None): + def __init__(self, url: str, django_auth: Dict[str, str], + requests_auth: Optional[HTTPBasicAuth] = None, + block: 'ProblemBlock' = None): self.url = url self.auth = django_auth self.session = requests.Session() self.session.auth = requests_auth + self.block = block + self.submission = XQueueInterfaceSubmission(self.block) def send_to_queue(self, header, body, files_to_upload=None): """ @@ -165,16 +169,17 @@ def _send_to_queue(self, header, body, files_to_upload): # lint-amnesty, pylint for f in files_to_upload: files.update({f.name: f}) - header_info = json.loads(header) - course_id = get_course_id(header_info['lms_callback_url']) + course_id = str(self.block.scope_ids.usage_id.context_key) if is_flag_active('send_to_submission_course.enable', course_id): # Use the new edx-submissions workflow - submission = XQueueInterfaceSubmission().send_to_submission(header, body, files) + submission = self.submission.send_to_submission(header, body, files) log.error(submission) return None, '' else: - return self._http_post(self.url + '/xqueue/submit/', payload, files=files) + return self._http_post( + self.url + '/xqueue/submit/', payload, files=files + ) def _http_post(self, url, data, files=None): # lint-amnesty, pylint: disable=missing-function-docstring try: @@ -204,15 +209,16 @@ class XQueueService: """ def __init__(self, block: 'ProblemBlock'): - #breakpoint() 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 + settings.XQUEUE_INTERFACE['url'], settings.XQUEUE_INTERFACE['django_auth'], requests_auth, + block=block ) self._block = block + @property def interface(self): """ @@ -220,36 +226,33 @@ def interface(self): """ return self._interface - def construct_callback(self, dispatch: str = 'score_update') -> str: + def construct_callback(self, dispatch: str = "score_update") -> str: """ - Return a fully qualified callback URL for external queueing system. + Return a fully qualified callback URL for the external queueing system. """ - dispatch_callback = "callback_submission" + 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) + + callback_type = ( + "callback_submission" + if is_flag_active("send_to_submission_course.enable", course_id) + else "xqueue_callback" + ) + relative_xqueue_callback_url = reverse( - dispatch_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) - lms_callback_url = xqueue_callback_url_prefix + relative_xqueue_callback_url - course_id = get_course_id(lms_callback_url) - if is_flag_active('send_to_submission_course.enable', course_id): - return xqueue_callback_url_prefix + relative_xqueue_callback_url + str(self._block.max_score()) - else: - dispatch_callback = 'xqueue_callback' - relative_xqueue_callback_url = reverse( - dispatch_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 + callback_type, + kwargs={ + "course_id": course_id, + "userid": userid, + "mod_id": mod_id, + "dispatch": dispatch, + }, + ) + + xqueue_callback_url_prefix = settings.XQUEUE_INTERFACE.get("callback_url", settings.LMS_ROOT_URL) + return f"{xqueue_callback_url_prefix}{relative_xqueue_callback_url}" + @property def default_queuename(self) -> str: diff --git a/xmodule/capa/xqueue_submission.py b/xmodule/capa/xqueue_submission.py index 414218f5cd52..3f1284484856 100644 --- a/xmodule/capa/xqueue_submission.py +++ b/xmodule/capa/xqueue_submission.py @@ -5,95 +5,73 @@ import json import logging -import re -from opaque_keys.edx.keys import CourseKey + +from xmodule.capa.errors import ( + GetSubmissionParamsError, + JSONParsingError, + MissingKeyError, + ValidationError, + TypeErrorSubmission, + RuntimeErrorSubmission +) log = logging.getLogger(__name__) class XQueueInterfaceSubmission: """Interface to the external grading system.""" + def __init__(self, block): + self.block = block + def _parse_json(self, data, name): """Helper function to parse JSON safely.""" try: return json.loads(data) if isinstance(data, str) else data except json.JSONDecodeError as e: - raise ValueError(f"Error parsing {name}: {e}") from e - - def _extract_identifiers(self, callback_url): - """Extracts identifiers from the callback URL.""" - item_id_match = re.search(r'block@([^\/]+)', callback_url) - item_type_match = re.search(r'type@([^+]+)', callback_url) - course_id_match = re.search(r'(course-v1:[^\/]+)', callback_url) - max_score_match = re.search(r'(\d+(\.\d+)?)$', callback_url) - - if not all([item_id_match, item_type_match, course_id_match, max_score_match]): - raise ValueError("The callback_url does not contain the required information.") - - return ( - item_id_match.group(1), - item_type_match.group(1), - course_id_match.group(1), - float(max_score_match.group(1)) - ) - - def extract_item_data(self, header, payload): + raise JSONParsingError(name, str(e)) from e + + def get_submission_params(self, header, payload): """ Extracts student submission data from the given header and payload. """ - header = self._parse_json(header, "header") payload = self._parse_json(payload, "payload") - callback_url = header.get('lms_callback_url') queue_name = header.get('queue_name', 'default') - if not callback_url: - raise ValueError("The header does not contain 'lms_callback_url'.") - - item_id, item_type, course_id, max_score = self._extract_identifiers(callback_url) - - student_info = self._parse_json(payload["student_info"], "student_info") - - full_block_id = None + if not self.block: + raise GetSubmissionParamsError() - try: - full_block_id = ( - f"block-v1:{course_id.replace('course-v1:', '')}+type@{item_type}+block@{item_id}" - ) - except Exception as e: - raise ValueError( - f"Error creating BlockUsageLocator. Invalid ID: {full_block_id}, Error: {e}" - ) from e + course_id = str(self.block.scope_ids.usage_id.context_key) + item_type = self.block.scope_ids.block_type + block_id = self.block.scope_ids.def_id.block_id + score = self.block.max_score() - try: - course_key = CourseKey.from_string(course_id) - except Exception as e: - raise ValueError(f"Error creating CourseKey: {e}") from e + item_id = f"block-v1:{course_id.replace('course-v1:', '')}+type@{item_type}+block@{block_id}" try: grader_payload = self._parse_json(payload["grader_payload"], "grader_payload") grader = grader_payload.get("grader", '') except KeyError as e: - raise ValueError(f"Error in payload: {e}") from e + raise MissingKeyError("grader_payload") from e + student_info = self._parse_json(payload["student_info"], "student_info") student_id = student_info.get("anonymous_student_id") + if not student_id: - raise ValueError("The field 'anonymous_student_id' is missing from student_info.") + raise ValidationError("The field 'anonymous_student_id' is missing from student_info.") + + student_answer = payload.get("student_response") + if student_answer is None: + raise ValidationError("The field 'student_response' does not exist.") student_dict = { - 'item_id': full_block_id, + 'item_id': item_id, 'item_type': item_type, 'course_id': course_id, 'student_id': student_id } - student_answer = payload.get("student_response") - if student_answer is None: - raise ValueError("The field 'student_response' does not exist.") - - score = max_score - return student_dict, student_answer, queue_name, grader, score def send_to_submission(self, header, body, files_to_upload=None): @@ -103,25 +81,31 @@ def send_to_submission(self, header, body, files_to_upload=None): from submissions.api import create_submission try: - student_item, answer, queue_name, grader, score = self.extract_item_data(header, body) - return create_submission(student_item, answer, queue_name=queue_name, grader=grader, score=score) - - except json.JSONDecodeError as e: - log.error("JSON decoding error: %s", e) - return {"error": "Invalid JSON format"} + student_item, answer, queue_name, grader, score = self.get_submission_params(header, body) + return create_submission( + student_item, + answer, + queue_name=queue_name, + grader=grader, + score=score, + file=files_to_upload + ) + except JSONParsingError as e: + log.error("%s", e) + return {"error": str(e)} - except KeyError as e: - log.error("Missing key: %s", e) - return {"error": f"Missing key: {e}"} + except MissingKeyError as e: + log.error("%s", e) + return {"error": str(e)} - except ValueError as e: - log.error("Validation error: %s", e) - return {"error": f"Validation error: {e}"} + except ValidationError as e: + log.error("%s", e) + return {"error": str(e)} except TypeError as e: - log.error("Type error: %s", e) - return {"error": f"Type error: {e}"} + log.error("%s", e) + raise TypeErrorSubmission(str(e)) from e except RuntimeError as e: - log.error("Runtime error: %s", e) - return {"error": f"Runtime error: {e}"} + log.error("%s", e) + raise RuntimeErrorSubmission(str(e)) from e diff --git a/xmodule/docs/decisions/0005-send-data-to-edx-submission.rst b/xmodule/docs/decisions/0005-send-data-to-edx-submission.rst index 5e8d45367277..1cbbf3580ae3 100644 --- a/xmodule/docs/decisions/0005-send-data-to-edx-submission.rst +++ b/xmodule/docs/decisions/0005-send-data-to-edx-submission.rst @@ -7,7 +7,7 @@ Status Accepted. -2025-02-21 +2025-02-27 Context ******* @@ -27,39 +27,13 @@ Key changes include: 3. **`construct_callback` Method Update**: Within the `XQueueService` class in `xqueue_interfaces.py`, the `construct_callback` method was modified. This method generates the callback URL that `XQueue` or `edx-submission` will use to return the evaluation results. The method now checks the state of the `send_to_submission_course.enable` *waffle flag* to determine whether the callback URL should point to the `edx-submission` handler (`callback_submission`) or to the original `XQueue` handler (`xqueue_callback`). - Additionally, the method now appends the `max_score` value to the callback URL when using `edx-submission`. This ensures that `xqueue_submission.py` can extract and pass the `max_score` value correctly to `create_submission`. - - **Updated Logic in `construct_callback`**: - .. code-block:: python - - if is_flag_active('send_to_submission_course.enable', course_id): - return f"{xqueue_callback_url_prefix}{relative_xqueue_callback_url}{self.block.max_score()}" - else: - return f"{xqueue_callback_url_prefix}{relative_xqueue_callback_url}" + The updated logic in `construct_callback` involves checking the state of the waffle flag and constructing the callback URL accordingly. If the flag is active for a given course, the callback URL is constructed for `edx-submission`; otherwise, it is constructed for `XQueue`. 4. **Implementation of `send_to_submission` in `xqueue_submission.py`**: The `send_to_submission` function was developed in the `xqueue_submission.py` file. This function is responsible for: - - **Parse Submission Data**: Extracts and processes relevant information from the student response, including identifiers such as `course_id`, `item_id`, `student_id`, and now `max_score`. - - - **Extraction of `max_score` from Callback URL**: The function was updated to extract `max_score` using a regex pattern from the callback URL. + - **Parsing Submission Data**: Extracts and processes relevant information from the student response, including identifiers such as `course_id`, `item_id`, `student_id`, and `max_score`. - .. code-block:: python - - def _extract_identifiers(self, callback_url): - max_score_match = re.search(r'(\d+(\.\d+)?)$', callback_url) - if max_score_match: - max_score = float(max_score_match.group(1)) - else: - raise ValueError("The callback URL does not contain max_score.") - return max_score - - **Interaction with `edx-submission`**: Uses the API provided by `edx-submission` to create a new submission record in the submissions database, ensuring that the student response is stored and processed appropriately with the correct `max_score`. - .. code-block:: python - - def send_to_submission(self, header, body, files_to_upload=None): - student_item, student_answer, queue_name, grader, max_score = self.extract_item_data(header, body) - return create_submission(student_item, student_answer, queue_name=queue_name, grader=grader, score=max_score) - Configuration for Xqueue-watcher: Prerequisites From 167a7a537b5bb4c7d7b3246096c332341b4662b4 Mon Sep 17 00:00:00 2001 From: gabrielC1409 Date: Tue, 4 Mar 2025 15:34:14 -0400 Subject: [PATCH 3/3] fix: Elimination of unnecessary functions --- xmodule/capa/xqueue_interface.py | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/xmodule/capa/xqueue_interface.py b/xmodule/capa/xqueue_interface.py index a3d2bd7609bc..dc8dd84833ca 100644 --- a/xmodule/capa/xqueue_interface.py +++ b/xmodule/capa/xqueue_interface.py @@ -5,7 +5,6 @@ import hashlib import json -import re import logging import requests @@ -37,25 +36,6 @@ def is_flag_active(flag_name, course_id): return flag and flag.enabled -def get_flag_by_name(flag_name): - """ - Look for the waffle flag by name. - """ - from openedx.core.djangoapps.waffle_utils.models import WaffleFlagCourseOverrideModel - flag = WaffleFlagCourseOverrideModel.objects.filter(waffle_flag=flag_name, enabled=True).first() - return flag and flag.enabled - - -def get_course_id(callback_url): - """ - Extract course_id from the callback URL - """ - course_id_match = re.search(r'(course-v1:[^\/]+)', callback_url) - if not course_id_match: - raise ValueError("The callback_url does not contain the required information.") - return course_id_match.group(1) - - def make_hashkey(seed): """ Generate a string key by hashing