From f4cf44a55a1ce8995c5f5c5fcc8b08246818422c Mon Sep 17 00:00:00 2001 From: Alison Langston <46360176+alangsto@users.noreply.github.com> Date: Fri, 20 Sep 2024 09:20:12 -0400 Subject: [PATCH] fix: handle invalid unit ids (#110) * fix: handle invalid unit ids * fix: update log level --- CHANGELOG.rst | 4 ++++ learning_assistant/__init__.py | 2 +- learning_assistant/api.py | 15 +++++++++----- tests/test_api.py | 36 +++++++++++++++++++++++----------- 4 files changed, 40 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 14cb096..11899c4 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -14,6 +14,10 @@ Change Log Unreleased ********** +4.3.2 - 2024-09-19 +****************** +* Add error handling for invalid unit usage keys + 4.3.1 - 2024-09-10 ****************** * Remove GPT model field as part of POST request to Xpert backend diff --git a/learning_assistant/__init__.py b/learning_assistant/__init__.py index cc9f1cd..a4f70ba 100644 --- a/learning_assistant/__init__.py +++ b/learning_assistant/__init__.py @@ -2,6 +2,6 @@ Plugin for a learning assistant backend, intended for use within edx-platform. """ -__version__ = '4.3.1' +__version__ = '4.3.2' default_app_config = 'learning_assistant.apps.LearningAssistantConfig' # pylint: disable=invalid-name diff --git a/learning_assistant/api.py b/learning_assistant/api.py index 2ae3450..3e0b5e0 100644 --- a/learning_assistant/api.py +++ b/learning_assistant/api.py @@ -7,7 +7,7 @@ from django.core.cache import cache from edx_django_utils.cache import get_cache_key from jinja2 import BaseLoader, Environment -from opaque_keys.edx.keys import CourseKey +from opaque_keys import InvalidKeyError from learning_assistant.constants import ACCEPTED_CATEGORY_TYPES, CATEGORY_TYPE_MAP from learning_assistant.data import LearningAssistantCourseEnabledData @@ -22,7 +22,6 @@ traverse_block_pre_order, ) from learning_assistant.text_utils import html_to_text -from learning_assistant.toggles import course_content_enabled log = logging.getLogger(__name__) @@ -109,9 +108,15 @@ def render_prompt_template(request, user_id, course_run_id, unit_usage_key, cour """ unit_content = '' - course_run_key = CourseKey.from_string(course_run_id) - if unit_usage_key and course_content_enabled(course_run_key): - _, unit_content = get_block_content(request, user_id, course_run_id, unit_usage_key) + if unit_usage_key: + try: + _, unit_content = get_block_content(request, user_id, course_run_id, unit_usage_key) + except InvalidKeyError: + log.warning( + 'Failed to retrieve course content for course_id=%(course_run_id)s because of ' + 'invalid unit_id=%(unit_usage_key)s', + {'course_run_id': course_run_id, 'unit_usage_key': unit_usage_key} + ) course_data = get_cache_course_data(course_id, ['skill_names', 'title']) skill_names = course_data['skill_names'] diff --git a/tests/test_api.py b/tests/test_api.py index d2d4861..5cb2a43 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -8,6 +8,7 @@ from django.conf import settings from django.core.cache import cache from django.test import TestCase, override_settings +from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey from learning_assistant.api import ( @@ -178,20 +179,13 @@ def test_get_block_content(self, mock_get_children_contents, mock_get_single_blo self.assertEqual(items, content_items) @ddt.data( - ('This is content.', True), - ('', True), - ('This is content.', False), - ('', False), + 'This is content.', + '' ) - @ddt.unpack @patch('learning_assistant.api.get_cache_course_data') - @patch('learning_assistant.toggles._is_learning_assistant_waffle_flag_enabled') @patch('learning_assistant.api.get_block_content') - def test_render_prompt_template( - self, unit_content, flag_enabled, mock_get_content, mock_is_flag_enabled, mock_cache - ): + def test_render_prompt_template(self, unit_content, mock_get_content, mock_cache): mock_get_content.return_value = (len(unit_content), unit_content) - mock_is_flag_enabled.return_value = flag_enabled skills_content = ['skills'] title = 'title' mock_cache.return_value = {'skill_names': skills_content, 'title': title} @@ -209,13 +203,33 @@ def test_render_prompt_template( request, user_id, course_run_id, unit_usage_key, course_id, template_string ) - if unit_content and flag_enabled: + if unit_content: self.assertIn(unit_content, prompt_text) else: self.assertNotIn('The following text is useful.', prompt_text) self.assertIn(str(skills_content), prompt_text) self.assertIn(title, prompt_text) + @patch('learning_assistant.api.get_cache_course_data', MagicMock()) + @patch('learning_assistant.api.get_block_content') + def test_render_prompt_template_invalid_unit_key(self, mock_get_content): + mock_get_content.side_effect = InvalidKeyError('foo', 'bar') + + # mock arguments that are passed through to `get_block_content` function. the value of these + # args does not matter for this test right now, as the `get_block_content` function is entirely mocked. + request = MagicMock() + user_id = 1 + course_run_id = self.course_run_id + unit_usage_key = 'block-v1:edX+A+B+type@vertical+block@verticalD' + course_id = 'edx+test' + template_string = getattr(settings, 'LEARNING_ASSISTANT_PROMPT_TEMPLATE', '') + + prompt_text = render_prompt_template( + request, user_id, course_run_id, unit_usage_key, course_id, template_string + ) + + self.assertNotIn('The following text is useful.', prompt_text) + @ddt.ddt class LearningAssistantCourseEnabledApiTests(TestCase):