From f6d7cb506529c0f405bfaaaf29cfe77af874d56f Mon Sep 17 00:00:00 2001 From: Usman Khalid <2200617@gmail.com> Date: Mon, 3 Feb 2014 18:34:01 +0500 Subject: [PATCH 1/2] Renamed feature setting 'DISPLAY_HISTOGRAMS_TO_STAFF' to 'DISPLAY_DEBUG_INFO_TO_STAFF' to better communicate its purpose. Default is True. LMS-1199 --- common/djangoapps/xmodule_modifiers.py | 2 +- lms/djangoapps/courseware/module_render.py | 6 +- .../courseware/tests/test_module_render.py | 63 ++++++++++++++++--- lms/envs/common.py | 4 +- 4 files changed, 60 insertions(+), 15 deletions(-) diff --git a/common/djangoapps/xmodule_modifiers.py b/common/djangoapps/xmodule_modifiers.py index 2c34994c45..654482fab6 100644 --- a/common/djangoapps/xmodule_modifiers.py +++ b/common/djangoapps/xmodule_modifiers.py @@ -147,7 +147,7 @@ def grade_histogram(module_id): return grades -def add_histogram(user, block, view, frag, context): # pylint: disable=unused-argument +def add_staff_debug_info(user, block, view, frag, context): # pylint: disable=unused-argument """ Updates the supplied module with a new get_html function that wraps the output of the old get_html function with additional information diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 952d3734df..cb2036a04c 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -38,7 +38,7 @@ from xmodule.modulestore import Location from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.util.duedate import get_extended_due_date -from xmodule_modifiers import replace_course_urls, replace_jump_to_id_urls, replace_static_urls, add_histogram, wrap_xblock +from xmodule_modifiers import replace_course_urls, replace_jump_to_id_urls, replace_static_urls, add_staff_debug_info, wrap_xblock from xmodule.lti_module import LTIModule from xmodule.x_module import XModuleDescriptor @@ -368,9 +368,9 @@ def get_module_for_descriptor_internal(user, descriptor, field_data_cache, cours reverse('jump_to_id', kwargs={'course_id': course_id, 'module_id': ''}), )) - if settings.FEATURES.get('DISPLAY_HISTOGRAMS_TO_STAFF'): + if settings.FEATURES.get('DISPLAY_DEBUG_INFO_TO_STAFF'): if has_access(user, descriptor, 'staff', course_id): - block_wrappers.append(partial(add_histogram, user)) + block_wrappers.append(partial(add_staff_debug_info, user)) # These modules store data using the anonymous_student_id as a key. # To prevent loss of data, we will continue to provide old modules with diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 3bdc3b1049..0f942b4d3f 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -12,6 +12,7 @@ from django.test import TestCase from django.test.client import RequestFactory from django.test.utils import override_settings +from capa.tests.response_xml_factory import OptionResponseXMLFactory from xblock.field_data import FieldData from xblock.runtime import Runtime from xblock.fields import ScopeIds @@ -21,14 +22,14 @@ from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import ItemFactory, CourseFactory from xmodule.x_module import XModuleDescriptor -import courseware.module_render as render + +from courseware import module_render as render +from courseware.courses import get_course_with_access, course_image_url, get_course_info_section +from courseware.model_data import FieldDataCache +from courseware.tests.factories import StudentModuleFactory, UserFactory from courseware.tests.tests import LoginEnrollmentTestCase from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE -from courseware.model_data import FieldDataCache -from courseware.courses import get_course_with_access, course_image_url, get_course_info_section - -from .factories import UserFactory from lms.lib.xblock.runtime import quote_slashes @@ -506,8 +507,42 @@ class TestHtmlModifiers(ModuleStoreTestCase): result_fragment.content ) - @patch('courseware.module_render.has_access', Mock(return_value=True)) - def test_histogram(self): + +@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) +@patch.dict('django.conf.settings.FEATURES', {'DISPLAY_DEBUG_INFO_TO_STAFF': True, 'DISPLAY_HISTOGRAMS_TO_STAFF': True}) +@patch('courseware.module_render.has_access', Mock(return_value=True)) +class TestStaffDebugInfo(ModuleStoreTestCase): + """Tests to verify that Staff Debug Info panel and histograms are displayed to staff.""" + + def setUp(self): + self.user = UserFactory.create() + self.request = RequestFactory().get('/') + self.request.user = self.user + self.request.session = {} + self.course = CourseFactory.create() + + problem_xml = OptionResponseXMLFactory().build_xml( + question_text='The correct answer is Correct', + num_inputs=2, + weight=2, + options=['Correct', 'Incorrect'], + correct_option='Correct' + ) + self.descriptor = ItemFactory.create( + category='problem', + data=problem_xml, + display_name='Option Response Problem' + ) + + self.location = self.descriptor.location + self.field_data_cache = FieldDataCache.cache_for_descriptor_descendents( + self.course.id, + self.user, + self.descriptor + ) + + @patch.dict('django.conf.settings.FEATURES', {'DISPLAY_DEBUG_INFO_TO_STAFF': False}) + def test_staff_debug_info_disabled(self): module = render.get_module( self.user, self.request, @@ -516,11 +551,19 @@ class TestHtmlModifiers(ModuleStoreTestCase): self.course.id, ) result_fragment = module.render('student_view') + self.assertNotIn('Staff Debug', result_fragment.content) - self.assertIn( - 'Staff Debug', - result_fragment.content + def test_staff_debug_info_enabled(self): + module = render.get_module( + self.user, + self.request, + self.location, + self.field_data_cache, + self.course.id, ) + result_fragment = module.render('student_view') + self.assertIn('Staff Debug', result_fragment.content) + PER_COURSE_ANONYMIZED_DESCRIPTORS = (LTIDescriptor, ) diff --git a/lms/envs/common.py b/lms/envs/common.py index 0e2ab8bf34..d3724c68b2 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -55,7 +55,9 @@ DISCUSSION_SETTINGS = { FEATURES = { 'SAMPLE': False, 'USE_DJANGO_PIPELINE': True, - 'DISPLAY_HISTOGRAMS_TO_STAFF': True, + + 'DISPLAY_DEBUG_INFO_TO_STAFF': True, + 'REROUTE_ACTIVATION_EMAIL': False, # nonempty string = address for all activation emails 'DEBUG_LEVEL': 0, # 0 = lowest level, least verbose, 255 = max level, most verbose From 21d8b40576293ad88447307489943dbacdcb2be4 Mon Sep 17 00:00:00 2001 From: Usman Khalid <2200617@gmail.com> Date: Thu, 6 Feb 2014 23:05:02 +0500 Subject: [PATCH 2/2] Added feature setting 'DISPLAY_HISTOGRAMS_TO_STAFF' to configure display of histograms in Staff Debug Info panel. Generating histograms requires scanning the courseware_studentmodule table on each view. This can make staff access to courseware very slow on large courses. Default is False. LMS-1199 --- common/djangoapps/xmodule_modifiers.py | 10 +++- .../courseware/tests/test_module_render.py | 59 +++++++++++++++++++ lms/envs/common.py | 1 + 3 files changed, 67 insertions(+), 3 deletions(-) diff --git a/common/djangoapps/xmodule_modifiers.py b/common/djangoapps/xmodule_modifiers.py index 654482fab6..537a7592c2 100644 --- a/common/djangoapps/xmodule_modifiers.py +++ b/common/djangoapps/xmodule_modifiers.py @@ -126,8 +126,12 @@ def replace_static_urls(data_dir, block, view, frag, context, course_id=None, st def grade_histogram(module_id): - ''' Print out a histogram of grades on a given problem. - Part of staff member debug info. + ''' + Print out a histogram of grades on a given problem in staff member debug info. + + Warning: If a student has just looked at an xmodule and not attempted + it, their grade is None. Since there will always be at least one such student + this function almost always returns []. ''' from django.db import connection cursor = connection.cursor() @@ -161,7 +165,7 @@ def add_staff_debug_info(user, block, view, frag, context): # pylint: disable=u return frag block_id = block.id - if block.has_score: + if block.has_score and settings.FEATURES.get('DISPLAY_HISTOGRAMS_TO_STAFF'): histogram = grade_histogram(block_id) render_histogram = len(histogram) > 0 else: diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 0f942b4d3f..0dd527ed2b 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -564,6 +564,65 @@ class TestStaffDebugInfo(ModuleStoreTestCase): result_fragment = module.render('student_view') self.assertIn('Staff Debug', result_fragment.content) + @patch.dict('django.conf.settings.FEATURES', {'DISPLAY_HISTOGRAMS_TO_STAFF': False}) + def test_histogram_disabled(self): + module = render.get_module( + self.user, + self.request, + self.location, + self.field_data_cache, + self.course.id, + ) + result_fragment = module.render('student_view') + self.assertNotIn('histrogram', result_fragment.content) + + def test_histogram_enabled_for_unscored_xmodules(self): + """Histograms should not display for xmodules which are not scored.""" + + html_descriptor = ItemFactory.create( + category='html', + data='Here are some course details.' + ) + field_data_cache = FieldDataCache.cache_for_descriptor_descendents( + self.course.id, + self.user, + self.descriptor + ) + with patch('xmodule_modifiers.grade_histogram') as mock_grade_histogram: + mock_grade_histogram.return_value = [] + module = render.get_module( + self.user, + self.request, + html_descriptor.location, + field_data_cache, + self.course.id, + ) + module.render('student_view') + self.assertFalse(mock_grade_histogram.called) + + def test_histogram_enabled_for_scored_xmodules(self): + """Histograms should display for xmodules which are scored.""" + + StudentModuleFactory.create( + course_id=self.course.id, + module_state_key=self.location, + student=UserFactory(), + grade=1, + max_grade=1, + state="{}", + ) + with patch('xmodule_modifiers.grade_histogram') as mock_grade_histogram: + mock_grade_histogram.return_value = [] + module = render.get_module( + self.user, + self.request, + self.location, + self.field_data_cache, + self.course.id, + ) + module.render('student_view') + self.assertTrue(mock_grade_histogram.called) + PER_COURSE_ANONYMIZED_DESCRIPTORS = (LTIDescriptor, ) diff --git a/lms/envs/common.py b/lms/envs/common.py index d3724c68b2..e528c884e8 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -57,6 +57,7 @@ FEATURES = { 'USE_DJANGO_PIPELINE': True, 'DISPLAY_DEBUG_INFO_TO_STAFF': True, + 'DISPLAY_HISTOGRAMS_TO_STAFF': False, # For large courses this slows down courseware access for staff. 'REROUTE_ACTIVATION_EMAIL': False, # nonempty string = address for all activation emails 'DEBUG_LEVEL': 0, # 0 = lowest level, least verbose, 255 = max level, most verbose