diff --git a/CHANGELOG.rst b/CHANGELOG.rst index b7dd2442c8..d62db9efc3 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -17,6 +17,10 @@ Unreleased ---------- * nothing unreleased +[6.6.10] - 2026-03-11 +--------------------- +* fix: handling moodle error (ENT-9080) + [6.6.9] - 2026-03-10 --------------------- * fix: handle duplicate enterprise group name validation error (ENT-11506) diff --git a/enterprise/__init__.py b/enterprise/__init__.py index 35ec3f5af5..9bf01a5a82 100644 --- a/enterprise/__init__.py +++ b/enterprise/__init__.py @@ -2,4 +2,4 @@ Your project description goes here. """ -__version__ = "6.6.9" +__version__ = "6.6.10" diff --git a/integrated_channels/moodle/client.py b/integrated_channels/moodle/client.py index 25b6cf5444..49f7a29ca1 100644 --- a/integrated_channels/moodle/client.py +++ b/integrated_channels/moodle/client.py @@ -16,9 +16,108 @@ from integrated_channels.integrated_channel.client import IntegratedChannelApiClient from integrated_channels.utils import generate_formatted_log, stringify_and_store_api_record +MOODLE_ERROR_STATUS_MAP = { + # Duplicate/Conflict errors (409) + "shortnametaken": 409, + "courseidnumbertaken": 409, + "morethanonerecordinfetch": 409, + "duplicateemail": 409, + "duplicateidnumber": 409, + "duplicateusername": 409, + + # Not Found errors (404) + "cannotfindcourse": 404, + "cannotfinduser": 404, + "invalidrecord": 404, + "usernamenotfound": 404, + "coursenotfound": 404, + "usernotfound": 404, + "invalidcourseid": 404, + "invaliduserid": 404, + "modulenotfound": 404, + "activitynotfound": 404, + + # Bad Request errors (400) + "missingparam": 400, + "codingerror": 400, + "invalidparameter": 400, + "invalidparametervalue": 400, + "requiredparametermissing": 400, + "invalidresponse": 400, + "invaliddata": 400, + + # Authentication errors (401) + "invalidlogin": 401, + "invalidtoken": 401, + "sessiontimeout": 401, + "requiresauthentication": 401, + + # Authorization/Permission errors (403) + "accessexception": 403, + "errorcatcontextnotvalid": 403, + "usernotfullysetup": 403, + "nopermission": 403, + "nopermissions": 403, + "requirecapability": 403, + "contextupdatefailed": 403, + "notpermittedtoaccesscourse": 403, + + # Server/Database errors (503) + "dbconnectionfailed": 503, + "dmlreadexception": 503, + "dmlwriteexception": 503, + "dmlexception": 503, + "databaseerror": 503, + + # Service/Method errors (400) + "invalidservicename": 400, + "invalidfunctionname": 400, + "servicerequireslogin": 401, + + # Enrollment errors (400) + "cannotenrol": 400, + "enrolnotpermitted": 403, + "alreadyenrolled": 409, + + # Grade/Completion errors (400) + "invalidcompletion": 400, + "invalidgrade": 400, + "itemidnotfound": 404, +} + LOGGER = logging.getLogger(__name__) +def _effective_error_status_code(status_code, error_code=None): + """ + Map Moodle error codes to correct HTTP status codes. + Moodle returns 200 response status code on failed transmission jobs which is the + historical and default behavior of Moodle web services. This function corrects + the status code based on the error_code in the response body. + Args: + status_code: The HTTP status code from the response + error_code: The error code from Moodle's response body + Returns: + int: The effective status code (mapped from MOODLE_ERROR_STATUS_MAP, + or 555 for unknown semantic errors, or original status_code) + """ + # First check if we have a mapped status for this error code + if error_code and error_code in MOODLE_ERROR_STATUS_MAP: + return MOODLE_ERROR_STATUS_MAP[error_code] + + # If status code indicates error (>=400), keep it + if status_code and status_code >= 400: + return status_code + + # If we get here, Moodle returned a 2xx/3xx status with an unknown error code + # This is a semantic error - the HTTP status says success but there's an error + if error_code: + return 555 # Semantic error for unknown Moodle failure + + # No error code and good status - this shouldn't happen in error path + return status_code if status_code else 500 + + class MoodleClientError(ClientError): """ Indicate a problem when interacting with Moodle. @@ -82,17 +181,32 @@ def inner(self, *args, **kwargs): raise ClientError('Moodle API Grade Update failed with possible error: {body}'.format(body=body), 500) error_code = body.get('errorcode') warnings = body.get('warnings') + + # Define mapped_status based on error_code + mapped_status = { + 'invalidtoken': 'Invalid Token', + 'missingfield': 'Missing Field', + 'duplicatedata': 'Duplicate Data', + }.get(error_code, 'Unknown Error') + if error_code and error_code == 'invalidtoken': self.token = self._get_access_token() # pylint: disable=protected-access response = method(self, *args, **kwargs) elif error_code: + status_code = _effective_error_status_code(response.status_code, error_code) + raise MoodleClientError( 'Moodle API Client Task "{method}" failed with error code ' '"{code}" and message: "{msg}" '.format( - method=method.__name__, code=error_code, msg=body.get('message'), + method=method.__name__, + code=error_code, + msg=body.get('message'), ), - response.status_code, - error_code, + status_code, + moodle_error={ + 'mapped_status': mapped_status, + 'error_code': error_code, + }, ) elif warnings: # More Moodle nonsense! @@ -464,9 +578,18 @@ def create_content_metadata(self, serialized_data): except MoodleClientError as error: # treat duplicate as successful, but only if its a single course # set chunk size settings to 1 if youre seeing a lot of these errors - if error.moodle_error == 'shortnametaken' and not more_than_one_course: + if ( + error.moodle_error + and error.moodle_error.get('error_code') == 'shortnametaken' + and not more_than_one_course + ): return 200, "shortnametaken" - elif error.moodle_error == 'courseidnumbertaken' and not more_than_one_course: + + elif ( + error.moodle_error + and error.moodle_error.get('error_code') == 'courseidnumbertaken' + and not more_than_one_course + ): return 200, "courseidnumbertaken" else: raise error diff --git a/tests/test_integrated_channels/test_moodle/test_client.py b/tests/test_integrated_channels/test_moodle/test_client.py index 7f9a49f27e..0f6e23d423 100644 --- a/tests/test_integrated_channels/test_moodle/test_client.py +++ b/tests/test_integrated_channels/test_moodle/test_client.py @@ -460,3 +460,72 @@ def test_create_content_metadata_with_mocked_api_requests(self): client.create_content_metadata(SERIALIZED_DATA) assert IntegratedChannelAPIRequestLogs.objects.count() == 2 assert len(responses.calls) == 2 + + def test_missing_param_error_handling(self): + """ + Test handling of 400 response for missing parameters. + """ + # pylint: disable=protected-access + + client = MoodleAPIClient(self.enterprise_config) + + missing_param_response = unittest.mock.Mock(spec=Response) + missing_param_response.json.return_value = { + 'errorcode': 'missingparam', + 'message': 'A required parameter is missing.' + } + missing_param_response.status_code = 400 + + client._post = unittest.mock.MagicMock( # Mocking _wrapped_post to prevent real HTTP calls + name='_post', + return_value=missing_param_response + ) + client._wrapped_post = unittest.mock.MagicMock(name='_wrapped_post', return_value=missing_param_response) + + # Ensure the mocked _wrapped_post method raises the expected error + client._wrapped_post.side_effect = MoodleClientError( + message='A required parameter is missing.', + status_code=400, + moodle_error={'error_code': 'missingparam'} + ) + + with self.assertRaises(MoodleClientError) as context: + client._wrapped_post(SERIALIZED_DATA) + + self.assertEqual(context.exception.status_code, 400) + self.assertEqual( + context.exception.moodle_error.get('error_code'), + 'missingparam' + ) + self.assertIn( + 'A required parameter is missing.', + context.exception.message + ) + + def test_moodle_errorcode_200_response_maps_to_correct_status(self): + """ + Test that when Moodle returns HTTP 200 with an errorcode, + the client maps it to the correct HTTP status code using + MOODLE_ERROR_STATUS_MAP. + """ + # pylint: disable=protected-access + + client = MoodleAPIClient(self.enterprise_config) + + mock_response = unittest.mock.Mock(spec=Response) + mock_response.json.return_value = { + "errorcode": "missingparam", + "message": "A required parameter is missing." + } + mock_response.status_code = 200 + + client._post = unittest.mock.MagicMock(return_value=mock_response) + + with self.assertRaises(MoodleClientError) as context: + client._wrapped_post(SERIALIZED_DATA) + + self.assertEqual(context.exception.status_code, 400) + self.assertEqual( + context.exception.moodle_error.get("error_code"), + "missingparam" + )