From 0fb85f2d89116b7e9861017fcaea4f897575881b Mon Sep 17 00:00:00 2001 From: gabrielC1409 Date: Thu, 20 Feb 2025 17:20:33 -0400 Subject: [PATCH] fix: Workflow update to integrate the waffle flag course and be able to send to edx-submission test: Adjust styles fix: ADR document update for change from Xqueue to edx-submission fix: Update 0005-send-data-to-edx-submission.rst Updated ADR doc to add Xqueue-watcher configuration --- docs/decisions/0022-create-waffle-switch.rst | 58 ------ lms/envs/common.py | 2 + lms/urls.py | 8 + pylint.log | 0 xmodule/capa/capa_problem.py | 8 +- xmodule/capa/inputtypes.py | 37 ++-- xmodule/capa/responsetypes.py | 8 +- xmodule/capa/tests/test_inputtypes.py | 47 ++++- xmodule/capa/tests/test_responsetypes.py | 11 +- xmodule/capa/tests/test_xqueue_interface.py | 106 ++++++---- xmodule/capa/tests/test_xqueue_submission.py | 66 +++++- xmodule/capa/xqueue_interface.py | 39 +++- xmodule/capa/xqueue_submission.py | 129 ++++++------ .../0005-send-data-to-edx-submission.rst | 192 ++++++++++++++++++ 14 files changed, 515 insertions(+), 196 deletions(-) delete mode 100644 docs/decisions/0022-create-waffle-switch.rst create mode 100644 pylint.log create mode 100644 xmodule/docs/decisions/0005-send-data-to-edx-submission.rst diff --git a/docs/decisions/0022-create-waffle-switch.rst b/docs/decisions/0022-create-waffle-switch.rst deleted file mode 100644 index 055c40bf1550..000000000000 --- a/docs/decisions/0022-create-waffle-switch.rst +++ /dev/null @@ -1,58 +0,0 @@ -############################################################### -Integration of Waffle Switch for XQueue Submission -############################################################### - -Status -****** - -**Pending** *2025-02-11* - -Implementation in progress. - -Context -******* - -In the `edx-platform` repository, there was a need to implement a mechanism that allows conditional execution of a new functionality: sending a student's response within an exercise to the `created_submission` function. This mechanism should be easily toggleable without requiring code changes or deployments. - -Decision -******** - -A `waffle switch` named `xqueue_submission.enabled` was introduced within the Django admin interface. When this switch is activated, it enables the functionality to send data to the `send_to_submission` function, which parses and forwards the data to the `created_submission` function. - -The `created_submission` function resides in the `edx-submissions` repository and is responsible for storing the data in the submissions database. - -Implementation Details ----------------------- - -This functionality was implemented within the `edx-platform` repository by modifying the following files: - -1. **`xmodule/capa/xqueue_interfaces.py`** - - The `waffle switch` **`xqueue_submission.enabled`** was added here. - - This switch is checked before invoking `send_to_submission`, ensuring that the submission logic is only executed when enabled. - -2. **`xmodule/capa/xqueue_submission.py`** - - This file contains the newly implemented logic that parses the student’s response. - - It processes and formats the data before calling `created_submission`, ensuring that it is correctly stored in the **edx-submissions** repository. - -Consequences -************ - -Positive: ---------- - -- **Flexibility:** The use of a `waffle switch` allows administrators to enable or disable the new submission functionality without modifying the codebase or redeploying the application. -- **Control:** Administrators can manage the feature directly from the Django admin interface, providing a straightforward method to toggle the feature as needed. -- **Modular Design:** The logic was added in a way that allows future modifications without affecting existing submission workflows. - -Negative: ---------- - -- **Potential Misconfiguration:** If the `waffle switch` is not properly managed, there could be inconsistencies in submission processing. -- **Admin Overhead:** Requires monitoring to ensure the toggle is enabled when needed. - -References -********** - -- Commit implementing the change: [f50afcc301bdc3eeb42a6dc2c051ffb2d799f868#diff-9b4290d2b574f54e4eca7831368727f7ddbac8292aa75ba4b28651d4bf2bbe6b](https://github.com/aulasneo/edx-platform/commit/f50afcc301bdc3eeb42a6dc2c051ffb2d799f868#diff-9b4290d2b574f54e4eca7831368727f7ddbac8292aa75ba4b28651d4bf2bbe6b) -- Open edX Feature Toggles Documentation: [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) \ No newline at end of file diff --git a/lms/envs/common.py b/lms/envs/common.py index 166966db45be..77f3d35af702 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -3060,6 +3060,8 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring 'openedx.core.djangoapps.staticfiles.apps.EdxPlatformStaticFilesConfig', 'django_celery_results', + + # Common Initialization 'openedx.core.djangoapps.common_initialization.apps.CommonInitializationConfig', 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/pylint.log b/pylint.log new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/xmodule/capa/capa_problem.py b/xmodule/capa/capa_problem.py index 3bf5b78cd4a3..dd7d46703708 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 @@ -432,8 +434,12 @@ def get_recentmost_queuetime(self): for answer_id in self.correct_map if self.correct_map.is_queued(answer_id) ] + if get_flag_by_name('send_to_submission_course.enable'): + data_format = xqueue_submission.dateformat + else: + data_format = xqueue_interface.dateformat queuetimes = [ - datetime.strptime(qt_str, xqueue_interface.dateformat).replace(tzinfo=UTC) + datetime.strptime(qt_str, data_format).replace(tzinfo=UTC) for qt_str in queuetime_strs ] diff --git a/xmodule/capa/inputtypes.py b/xmodule/capa/inputtypes.py index 8dff57768688..6d463b8a0033 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 """ @@ -423,8 +421,8 @@ def parse_options(options): id==description for now. TODO: make it possible to specify different id and descriptions. """ # convert single quotes inside option values to html encoded string - options = re.sub(r"([a-zA-Z])('|\\')([a-zA-Z])", r"\1'\3", options) - options = re.sub(r"\\'", r"'", options) # replace already escaped single quotes + options = re.sub(r"([a-zA-Z])('|\\')([a-zA-Z])", r"\1& #39;\3", options) + options = re.sub(r"\\'", r"& #39;", options) # replace already escaped single quotes # parse the set of possible options lexer = shlex.shlex(options[1:-1]) @@ -434,7 +432,7 @@ def parse_options(options): # remove quotes # convert escaped single quotes (html encoded string) back to single quotes - tokens = [x[1:-1].replace("'", "'") for x in lexer] + tokens = [x[1:-1].replace("& #39;", "'") for x in lexer] # make list of (option_id, option_description), with description=id return [(t, t) for t in tokens] @@ -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. @@ -987,7 +985,10 @@ def _plot_data(self, data): # construct xqueue headers qinterface = self.capa_system.xqueue.interface - qtime = datetime.utcnow().strftime(xqueue_interface.dateformat) + if get_flag_by_name('send_to_submission_course.enable'): + qtime = datetime.utcnow().strftime(xqueue_submission.dateformat) + else: + qtime = datetime.utcnow().strftime(xqueue_interface.dateformat) callback_url = self.capa_system.xqueue.construct_callback('ungraded_response') anonymous_student_id = self.capa_system.anonymous_student_id # TODO: Why is this using self.capa_system.seed when we have self.seed??? @@ -1086,9 +1087,9 @@ def get_attributes(cls): def setup(self): """ - if value is of the form [x,y] then parse it and send along coordinates of previous answer + if value is of the form [x, y] then parse it and send along coordinates of previous answer """ - m = re.match(r'\[([0-9]+),([0-9]+)]', + m = re.match(r'\[([0-9]+), ([0-9]+)]', self.value.strip().replace(' ', '')) if m: # Note: we subtract 15 to compensate for the size of the dot on the screen. @@ -1626,7 +1627,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/responsetypes.py b/xmodule/capa/responsetypes.py index 73378e7c0a2b..7a76606cc74a 100644 --- a/xmodule/capa/responsetypes.py +++ b/xmodule/capa/responsetypes.py @@ -37,6 +37,8 @@ import xmodule.capa.safe_exec as safe_exec 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 openedx.core.djangolib.markup import HTML, Text from openedx.core.lib.grade_utils import round_away_from_zero @@ -2675,8 +2677,10 @@ def get_score(self, student_answers): #------------------------------------------------------------ qinterface = self.capa_system.xqueue.interface - qtime = datetime.strftime(datetime.now(UTC), xqueue_interface.dateformat) - + if get_flag_by_name('send_to_submission_course.enable'): + qtime = datetime.strftime(datetime.now(UTC), xqueue_submission.dateformat) + else: + qtime = datetime.strftime(datetime.now(UTC), xqueue_submission.dateformat) anonymous_student_id = self.capa_system.anonymous_student_id # Generate header diff --git a/xmodule/capa/tests/test_inputtypes.py b/xmodule/capa/tests/test_inputtypes.py index 4e14bc42b76b..272f5cc621c1 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' @@ -1044,7 +1082,7 @@ def check(self, value, egx, egy): # lint-amnesty, pylint: disable=missing-funct def test_with_value(self): # Check that compensating for the dot size works properly. - self.check('[50,40]', 35, 25) + print("Context:", self.check('[50,40]', 35, 25)) def test_without_value(self): self.check('', 0, 0) @@ -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_xqueue_interface.py b/xmodule/capa/tests/test_xqueue_interface.py index 46c18869ba9e..037f901f48ba 100644 --- a/xmodule/capa/tests/test_xqueue_interface.py +++ b/xmodule/capa/tests/test_xqueue_interface.py @@ -7,12 +7,11 @@ from django.test.utils import override_settings from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator from xblock.fields import ScopeIds -from waffle.testutils import override_switch +import pytest import json from openedx.core.djangolib.testing.utils import skip_unless_lms from xmodule.capa.xqueue_interface import XQueueInterface, XQueueService -import pytest @pytest.mark.django_db @@ -22,34 +21,56 @@ class XQueueServiceTest(TestCase): 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.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 - callback_url = f'courses/{usage_id.context_key}/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" + + @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}" + + 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): """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.""" @@ -60,45 +81,50 @@ def test_waittime(self): @pytest.mark.django_db -@override_switch('xqueue_submission.enabled', active=True) -@patch('xmodule.capa.xqueue_submission.XQueueInterfaceSubmission.send_to_submission') -def test_send_to_queue_with_waffle_enabled(mock_send_to_submission): +@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"} - requests_auth = None - xqueue_interface = XQueueInterface(url, django_auth, requests_auth) + 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', + "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' + "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'} + 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 -@override_switch('xqueue_submission.enabled', active=False) -@patch('xmodule.capa.xqueue_interface.XQueueInterface._http_post') -def test_send_to_queue_with_waffle_disabled(mock_http_post): - +@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"} - requests_auth = None - xqueue_interface = XQueueInterface(url, django_auth, requests_auth) + 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', + "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' + "student_info": json.dumps({"anonymous_student_id": "student_id"}), + "student_response": "student_answer", }) files_to_upload = None @@ -106,7 +132,7 @@ def test_send_to_queue_with_waffle_disabled(mock_http_post): 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={} + "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 index 5c6cf8ba0f35..e1b0e6a0d895 100644 --- a/xmodule/capa/tests/test_xqueue_submission.py +++ b/xmodule/capa/tests/test_xqueue_submission.py @@ -1,23 +1,38 @@ +""" +Tests for XQueueInterfaceSubmission. +""" + import json import pytest from unittest.mock import Mock, patch -from django.conf import settings from xmodule.capa.xqueue_submission import XQueueInterfaceSubmission from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator -from opaque_keys.edx.keys import UsageKey, CourseKey from xblock.fields import ScopeIds @pytest.fixture def xqueue_service(): - location = BlockUsageLocator(CourseLocator("test_org", "test_course", "test_run"), "problem", "ExampleProblem") + """ + 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/score_update', + 'lms_callback_url': ( + 'http://example.com/courses/course-v1:org+course+run/xqueue/5/' + 'block-v1:org+course+run+type@problem+block@item_id/score_update' + ), }) payload = json.dumps({ 'student_info': json.dumps({'anonymous_student_id': 'student_id'}), @@ -27,10 +42,14 @@ def test_extract_item_data(): with patch('lms.djangoapps.courseware.models.StudentModule.objects.filter') as mock_filter: mock_filter.return_value.first.return_value = Mock(grade=0.85) - student_item, student_answer, queue_name, grader, score = XQueueInterfaceSubmission().extract_item_data(header, payload) + 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_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' @@ -44,8 +63,14 @@ def test_extract_item_data(): @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/score_update', + '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/score_update' + ), }) body = json.dumps({ 'student_info': json.dumps({'anonymous_student_id': 'student_id'}), @@ -58,10 +83,10 @@ def test_send_to_submission(mock_create_submission, xqueue_service): mock_create_submission.return_value = {'submission': 'mock_submission'} - # Llamada a send_to_submission + # Call send_to_submission result = xqueue_service.send_to_submission(header, body) - # Afirmaciones + # Assertions assert 'submission' in result assert result['submission'] == 'mock_submission' mock_create_submission.assert_called_once_with( @@ -76,3 +101,26 @@ def test_send_to_submission(mock_create_submission, xqueue_service): 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' + }) + 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 0c6b495bfbb5..341c6bdc99ed 100644 --- a/xmodule/capa/xqueue_interface.py +++ b/xmodule/capa/xqueue_interface.py @@ -5,20 +5,20 @@ 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 waffle import switch_is_active from xmodule.capa.xqueue_submission import XQueueInterfaceSubmission if TYPE_CHECKING: from xmodule.capa_block import ProblemBlock log = logging.getLogger(__name__) -dateformat = '%Y-%m-%dT%H:%M:%S' +dateformat = '%Y%m%d%H%M%S' XQUEUE_METRIC_NAME = 'edxapp.xqueue' @@ -28,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 @@ -137,7 +165,9 @@ def _send_to_queue(self, header, body, files_to_upload): # lint-amnesty, pylint for f in files_to_upload: files.update({f.name: f}) - if switch_is_active('xqueue_submission.enabled'): + 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) @@ -193,11 +223,10 @@ def construct_callback(self, dispatch: str = 'score_update') -> str: """ Return a fully qualified callback URL for external queueing system. """ - if switch_is_active('callback_submission.enabled'): + if get_flag_by_name('send_to_submission_course.enable'): dispatch_callback = "callback_submission" else: dispatch_callback = 'xqueue_callback' - relative_xqueue_callback_url = reverse( dispatch_callback, kwargs=dict( diff --git a/xmodule/capa/xqueue_submission.py b/xmodule/capa/xqueue_submission.py index 6282883ac704..b92b75ecf0c1 100644 --- a/xmodule/capa/xqueue_submission.py +++ b/xmodule/capa/xqueue_submission.py @@ -1,82 +1,81 @@ +""" +xqueue_submission.py + +This module handles the extraction and processing of student submission data +from edx-submission. +""" -import hashlib import json import logging -import requests -from django.conf import settings -from django.urls import reverse -from requests.auth import HTTPBasicAuth import re -from typing import Dict, Optional, TYPE_CHECKING -from opaque_keys.edx.keys import CourseKey, UsageKey -from django.core.exceptions import ObjectDoesNotExist -if TYPE_CHECKING: - from xmodule.capa_block import ProblemBlock +from opaque_keys.edx.keys import CourseKey log = logging.getLogger(__name__) dateformat = '%Y-%m-%dT%H:%M:%S' -XQUEUE_METRIC_NAME = 'edxapp.xqueue' -# Wait time for response from Xqueue. -XQUEUE_TIMEOUT = 35 # seconds -CONNECT_TIMEOUT = 3.05 # seconds -READ_TIMEOUT = 10 # seconds +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 -class XQueueInterfaceSubmission: - """ - Interface to the external grading system - """ + 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) + + if not item_id_match or not item_type_match or not course_id_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) def extract_item_data(self, header, payload): + """ + Extracts student submission data from the given header and payload. + """ from lms.djangoapps.courseware.models import StudentModule from opaque_keys.edx.locator import BlockUsageLocator - if isinstance(header, str): - try: - header = json.loads(header) - except json.JSONDecodeError as e: - raise ValueError(f"Error to header: {e}") - - if isinstance(payload, str): - try: - payload = json.loads(payload) - except json.JSONDecodeError as e: - raise ValueError(f"Error to payload: {e}") + + 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("El header is not content 'lms_callback_url'.") + raise ValueError("The header does not contain 'lms_callback_url'.") - item_id = re.search(r'block@([^\/]+)', callback_url).group(1) - item_type = re.search(r'type@([^+]+)', callback_url).group(1) - course_id = re.search(r'(course-v1:[^\/]+)', callback_url).group(1) + item_id, item_type, course_id = self._extract_identifiers(callback_url) + + student_info = self._parse_json(payload["student_info"], "student_info") try: - student_info = json.loads(payload["student_info"]) - except json.JSONDecodeError as e: - raise ValueError(f"Error to student_info: {e}") + full_block_id = f"block-v1:{course_id.replace('course-v1:', '')}+type@{item_type}+block@{item_id}" + usage_key = BlockUsageLocator.from_string(full_block_id) + except Exception as e: + raise ValueError(f"Error creating BlockUsageLocator. Invalid ID: {full_block_id}, Error: {e}") from e - usage_key = BlockUsageLocator.from_string(item_id) - course_key = CourseKey.from_string(course_id) + try: + course_key = CourseKey.from_string(course_id) + except Exception as e: + raise ValueError(f"Error creating CourseKey: {e}") from e - full_block_id = f"block-v1:{course_id.replace('course-v1:', '')}+type@{item_type}+block@{item_id}" try: - grader_payload = payload["grader_payload"] - if isinstance(grader_payload, str): - grader_payload = json.loads(grader_payload) + grader_payload = self._parse_json(payload["grader_payload"], "grader_payload") grader = grader_payload.get("grader", '') - except json.JSONDecodeError as e: - raise ValueError(f"Error grader_payload: {e}") except KeyError as e: - raise ValueError(f"Error payload: {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 not student_info.") + raise ValueError("The field 'anonymous_student_id' is missing from student_info.") student_dict = { 'item_id': full_block_id, @@ -87,7 +86,7 @@ def extract_item_data(self, header, payload): student_answer = payload.get("student_response") if student_answer is None: - raise ValueError("The field 'student_response' do not exist.") + raise ValueError("The field 'student_response' does not exist.") student_module = StudentModule.objects.filter( module_state_key=usage_key, @@ -96,27 +95,37 @@ def extract_item_data(self, header, payload): log.error(f"student_module: {student_module}") - if student_module and student_module.grade is not None: - score = student_module.grade - else: - score = None + score = student_module.grade if student_module and student_module.grade is not None else None log.error(f"Score: {student_id}: {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(f"JSON decoding error: {e}") + return {"error": "Invalid JSON format"} + + except KeyError as e: + log.error(f"Missing key: {e}") + return {"error": f"Missing key: {e}"} - log.error(f"student_item: {student_item}") - log.error(f"header: {header}") - log.error(f"body: {body}") - log.error(f"grader: {grader}") + except ValueError as e: + log.error(f"Validation error: {e}") + return {"error": f"Validation error: {e}"} - submission = create_submission(student_item, answer, queue_name=queue_name, grader=grader, score=score) + except TypeError as e: + log.error(f"Type error: {e}") + return {"error": f"Type error: {e}"} - return submission - except Exception as e: - return {"error": str(e)} + except RuntimeError as e: + log.error(f"Runtime error: {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..daaa3918b91d --- /dev/null +++ b/xmodule/docs/decisions/0005-send-data-to-edx-submission.rst @@ -0,0 +1,192 @@ +######################################################### +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`). + +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`, and `student_id`. + + - **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. + +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)