From 8886f29e52bf23368a88cadc5e2c372a173ccbbe Mon Sep 17 00:00:00 2001 From: Demid Date: Wed, 8 Jun 2022 19:59:45 +0300 Subject: [PATCH] refactor: remove `debug` property from `ModuleSystem` (#30450) This also: 1. Removes this property from XBlock runtime shims. 2. Updates the minimum required version of the LTI Consumer XBlock. --- cms/djangoapps/contentstore/views/preview.py | 1 - common/lib/xmodule/xmodule/capa_module.py | 18 +++++++--- common/lib/xmodule/xmodule/lti_2_util.py | 3 +- common/lib/xmodule/xmodule/tests/__init__.py | 1 - .../xmodule/xmodule/tests/test_capa_module.py | 36 ++++++++++++++----- common/lib/xmodule/xmodule/x_module.py | 3 +- lms/djangoapps/courseware/module_render.py | 1 - .../core/djangoapps/xblock/runtime/shims.py | 8 ----- requirements/edx/base.in | 2 +- 9 files changed, 44 insertions(+), 29 deletions(-) diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index b64a77ddb2..0d7e8786b4 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -212,7 +212,6 @@ def _preview_module_system(request, descriptor, field_data): # TODO (cpennington): Do we want to track how instructors are using the preview problems? track_function=lambda event_type, event: None, get_module=partial(_load_preview_module, request), - debug=True, mixins=settings.XBLOCK_MIXINS, course_id=course_id, diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 250fd0974f..225d9aa46b 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -490,6 +490,15 @@ class ProblemBlock( return self.display_name + @property + def debug(self): + """ + If CAPA block fails to render, we want course authors to be able to see + the error in Studio. At the same time, in production, we don't want + to show errors to students. + """ + return getattr(self.runtime, 'is_author_mode', False) or settings.DEBUG + @classmethod def filter_templates(cls, template, course): """ @@ -825,7 +834,7 @@ class ProblemBlock( cache=cache_service, can_execute_unsafe_code=sandbox_service.can_execute_unsafe_code, get_python_lib_zip=sandbox_service.get_python_lib_zip, - DEBUG=self.runtime.DEBUG, + DEBUG=self.debug, i18n=self.runtime.service(self, "i18n"), render_template=self.runtime.service(self, 'mako').render_template, resources_fs=self.runtime.resources_fs, @@ -1060,8 +1069,7 @@ class ProblemBlock( str(err) ) - # TODO (vshnayder): another switch on DEBUG. - if self.runtime.DEBUG: + if self.debug: msg = HTML( '[courseware.capa.capa_module] ' 'Failed to generate HTML for problem {url}' @@ -1777,7 +1785,7 @@ class ProblemBlock( self.set_last_submission_time() except (StudentInputError, ResponseError, LoncapaProblemError) as inst: - if self.runtime.DEBUG: + if self.debug: log.warning( "StudentInputError in capa_module:problem_check", exc_info=True @@ -1811,7 +1819,7 @@ class ProblemBlock( self.set_state_from_lcp() self.set_score(self.score_from_lcp(self.lcp)) - if self.runtime.DEBUG: + if self.debug: msg = f"Error checking problem: {str(err)}" msg += f'\nTraceback:\n{traceback.format_exc()}' return {'success': msg} diff --git a/common/lib/xmodule/xmodule/lti_2_util.py b/common/lib/xmodule/xmodule/lti_2_util.py index 17dfe680c0..3829139b7b 100644 --- a/common/lib/xmodule/xmodule/lti_2_util.py +++ b/common/lib/xmodule/xmodule/lti_2_util.py @@ -12,6 +12,7 @@ import re from unittest import mock from urllib import parse +from django.conf import settings from oauthlib.oauth1 import Client from webob import Response from xblock.core import XBlock @@ -63,7 +64,7 @@ class LTI20BlockMixin: Returns: webob.response: response to this request. See above for details. """ - if self.system.debug: + if settings.DEBUG: self._log_correct_authorization_header(request) if not self.accept_grades_past_due and self.is_past_due(): diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 206784dc24..0925073099 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -153,7 +153,6 @@ def get_test_system( static_url='/static', track_function=Mock(name='get_test_system.track_function'), get_module=get_module, - debug=True, hostname="edx.org", services={ 'user': user_service, diff --git a/common/lib/xmodule/xmodule/tests/test_capa_module.py b/common/lib/xmodule/xmodule/tests/test_capa_module.py index 26ff575f81..41acc05a93 100644 --- a/common/lib/xmodule/xmodule/tests/test_capa_module.py +++ b/common/lib/xmodule/xmodule/tests/test_capa_module.py @@ -17,6 +17,7 @@ import ddt import requests import webob from codejail.safe_exec import SafeExecException +from django.test import override_settings from django.utils.encoding import smart_str from edx_user_state_client.interface import XBlockUserState from lxml import etree @@ -961,6 +962,7 @@ class ProblemBlockTest(unittest.TestCase): # lint-amnesty, pylint: disable=miss # but that this was considered the second attempt for grading purposes assert module.lcp.context['attempt'] == 2 + @override_settings(DEBUG=True) def test_submit_problem_other_errors(self): """ Test that errors other than the expected kinds give an appropriate message. @@ -970,9 +972,6 @@ class ProblemBlockTest(unittest.TestCase): # lint-amnesty, pylint: disable=miss # Create the module module = CapaFactory.create(attempts=1, user_is_staff=False) - # Ensure that DEBUG is on - module.system.DEBUG = True - # Simulate answering a problem that raises the exception with patch('capa.capa_problem.LoncapaProblem.grade_answers') as mock_grade: error_msg = "Superterrible error happened: ☠" @@ -1711,9 +1710,6 @@ class ProblemBlockTest(unittest.TestCase): # lint-amnesty, pylint: disable=miss # is asked to render itself as HTML module.lcp.get_html = Mock(side_effect=Exception("Test")) - # Turn off DEBUG - module.system.DEBUG = False - # Try to render the module with DEBUG turned off html = module.get_problem_html() @@ -1727,6 +1723,31 @@ class ProblemBlockTest(unittest.TestCase): # lint-amnesty, pylint: disable=miss # Expect that the module has created a new dummy problem with the error assert original_problem != module.lcp + def test_get_problem_html_error_preview(self): + """ + Test the html response when an error occurs with DEBUG off in Studio. + """ + render_template = Mock(return_value="
Test Template HTML
") + module = CapaFactory.create(render_template=render_template) + + # Simulate throwing an exception when the capa problem + # is asked to render itself as HTML + error_msg = "Superterrible error happened: ☠" + module.lcp.get_html = Mock(side_effect=Exception(error_msg)) + + module.system.is_author_mode = True + + # Try to render the module with the author mode turned on + html = module.get_problem_html() + + assert html is not None + + # Check the rendering context + render_args, _ = render_template.call_args + context = render_args[1] + assert error_msg in context['problem']['html'] + + @override_settings(DEBUG=True) def test_get_problem_html_error_w_debug(self): """ Test the html response when an error occurs with DEBUG on @@ -1739,9 +1760,6 @@ class ProblemBlockTest(unittest.TestCase): # lint-amnesty, pylint: disable=miss error_msg = "Superterrible error happened: ☠" module.lcp.get_html = Mock(side_effect=Exception(error_msg)) - # Make sure DEBUG is on - module.system.DEBUG = True - # Try to render the module with DEBUG turned on html = module.get_problem_html() diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index ffc6a58ff9..ef5072c9bb 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -1637,7 +1637,7 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemShim, def __init__( self, static_url, track_function, get_module, - descriptor_runtime, debug=False, hostname="", publish=None, + descriptor_runtime, hostname="", publish=None, course_id=None, error_descriptor_class=None, field_data=None, rebind_noauth_module_to_user=None, **kwargs): @@ -1678,7 +1678,6 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemShim, self.STATIC_URL = static_url self.track_function = track_function self.get_module = get_module - self.DEBUG = self.debug = debug self.HOSTNAME = self.hostname = hostname self.course_id = course_id diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 4d7ad48c79..9b1e7bc618 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -729,7 +729,6 @@ def get_module_system_for_user( static_url=settings.STATIC_URL, get_module=inner_get_module, user=user, - debug=settings.DEBUG, hostname=settings.SITE_NAME, publish=publish, course_id=course_id, diff --git a/openedx/core/djangoapps/xblock/runtime/shims.py b/openedx/core/djangoapps/xblock/runtime/shims.py index 637ce63cda..5225d249c5 100644 --- a/openedx/core/djangoapps/xblock/runtime/shims.py +++ b/openedx/core/djangoapps/xblock/runtime/shims.py @@ -97,14 +97,6 @@ class RuntimeShim: # TODO: Refactor capa to access this directly, don't bother the runtime. Then remove it from here. return False # Change this if/when we need to support unsafe courses in the new runtime. - @property - def DEBUG(self): - """ - Should DEBUG mode (?) be used? This flag is only read by capa. - """ - # TODO: Refactor capa to access this directly, don't bother the runtime. Then remove it from here. - return False - def get_python_lib_zip(self): """ A function returning a bytestring or None. The bytestring is the diff --git a/requirements/edx/base.in b/requirements/edx/base.in index 703fc0ee97..4f834e95d4 100644 --- a/requirements/edx/base.in +++ b/requirements/edx/base.in @@ -109,7 +109,7 @@ ipaddress # Ip network support for Embargo feature jsonfield # Django model field for validated JSON; used in several apps laboratory # Library for testing that code refactors/infrastructure changes produce identical results lxml # XML parser -lti-consumer-xblock>=4.1.0 +lti-consumer-xblock>=4.1.1 mailsnake # Needed for mailchimp (mailing djangoapp) mako # Primary template language used for server-side page rendering Markdown # Convert text markup to HTML; used in capa problems, forums, and course wikis