diff --git a/lms/djangoapps/instructor_task/tasks_helper/grades.py b/lms/djangoapps/instructor_task/tasks_helper/grades.py index 2d524ae9ac..f0c883a41e 100644 --- a/lms/djangoapps/instructor_task/tasks_helper/grades.py +++ b/lms/djangoapps/instructor_task/tasks_helper/grades.py @@ -17,6 +17,7 @@ from six import text_type from course_blocks.api import get_course_blocks from courseware.courses import get_course_by_id +from courseware.user_state_client import DjangoXBlockUserStateClient from instructor_analytics.basic import list_problem_responses from instructor_analytics.csvs import format_dictlist from lms.djangoapps.certificates.models import CertificateWhitelist, GeneratedCertificate, certificate_info_for_user @@ -581,43 +582,64 @@ class ProblemResponses(object): yield result @classmethod - def _build_student_data(cls, user_id, course_id, problem_location): + def _build_student_data(cls, user_id, course_key, usage_key_str): """ Generate a list of problem responses for all problem under the ``problem_location`` root. Arguments: user_id (int): The user id for the user generating the report - course_id (CourseKey): The ``CourseKey`` for the course whose report + course_key (CourseKey): The ``CourseKey`` for the course whose report is being generated - problem_location (str): The generated report will include this + usage_key_str (str): The generated report will include this block and it child blocks. Returns: List[Dict]: Returns a list of dictionaries containing the student data which will be included in the final csv. """ - problem_key = UsageKey.from_string(problem_location).map_into_course(course_id) + usage_key = UsageKey.from_string(usage_key_str).map_into_course(course_key) user = get_user_model().objects.get(pk=user_id) - course_blocks = get_course_blocks(user, problem_key) + course_blocks = get_course_blocks(user, usage_key) student_data = [] max_count = settings.FEATURES.get('MAX_PROBLEM_RESPONSES_COUNT') - for title, path, block_key in cls._build_problem_list(course_blocks, problem_key): - # Chapter and sequential blocks are filtered out since they include state - # which isn't useful for this report. - if block_key.block_type in ('sequential', 'chapter'): - continue - responses = list_problem_responses(course_id, block_key, max_count) - student_data += responses - for response in responses: - response['title'] = title - response['location'] = ' > '.join(path) - response['block_key'] = str(block_key) - if max_count is not None: - max_count -= len(responses) - if max_count <= 0: - break + + store = modulestore() + user_state_client = DjangoXBlockUserStateClient() + + with store.bulk_operations(course_key): + for title, path, block_key in cls._build_problem_list(course_blocks, usage_key): + # Chapter and sequential blocks are filtered out since they include state + # which isn't useful for this report. + if block_key.block_type in ('sequential', 'chapter'): + continue + + block = store.get_item(block_key) + + # Blocks can implement the generate_report_data method to provide their own + # human-readable formatting for user state. + if hasattr(block, 'generate_report_data'): + user_state_iterator = user_state_client.iter_all_for_block(block_key) + responses = [ + {'username': username, 'state': state} + for username, state in + block.generate_report_data(user_state_iterator, max_count) + ] + else: + responses = list_problem_responses(course_key, block_key, max_count) + + student_data += responses + for response in responses: + response['title'] = title + # A human-readable location for the current block + response['location'] = ' > '.join(path) + # A machine-friendly location for the current block + response['block_key'] = str(block_key) + if max_count is not None: + max_count -= len(responses) + if max_count <= 0: + break return student_data @@ -638,8 +660,8 @@ class ProblemResponses(object): # Compute result table and format it student_data = cls._build_student_data( user_id=task_input.get('user_id'), - course_id=course_id, - problem_location=problem_location + course_key=course_id, + usage_key_str=problem_location ) features = ['username', 'title', 'location', 'block_key', 'state'] diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index 0bcfee9152..f775ceb180 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -24,8 +24,8 @@ from django.conf import settings from django.core.urlresolvers import reverse from django.test.utils import override_settings from freezegun import freeze_time -from instructor_analytics.basic import UNAVAILABLE -from mock import MagicMock, Mock, patch +from instructor_analytics.basic import UNAVAILABLE, list_problem_responses +from mock import MagicMock, Mock, patch, ANY from nose.plugins.attrib import attr from pytz import UTC from shoppingcart.models import ( @@ -473,8 +473,25 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): self.instructor = self.create_instructor('instructor') self.student = self.create_student('student') - @patch.dict("django.conf.settings.FEATURES", {"MAX_PROBLEM_RESPONSES_COUNT": 4}) + # Can be used once CapaDescriptor gets ``generate_report_data`` method. + # @contextmanager + # def _remove_report_generator(self): + # """ + # Temporarily removes the generate_report_data method so we can test + # report generation when it's absent. + # """ + # from xmodule.capa_module import CapaDescriptor + # generate_report_data = CapaDescriptor.generate_report_data + # del CapaDescriptor.generate_report_data + # yield + # CapaDescriptor.generate_report_data = generate_report_data + + @patch.dict('django.conf.settings.FEATURES', {'MAX_PROBLEM_RESPONSES_COUNT': 4}) def test_build_student_data_limit(self): + """ + Ensure that the _build_student_data method respects the global setting for + maximum responses to return in a report. + """ self.define_option_problem(u'Problem1') for ctr in range(5): student = self.create_student('student{}'.format(ctr)) @@ -482,20 +499,28 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): student_data = ProblemResponses._build_student_data( user_id=self.instructor.id, - course_id=self.course.id, - problem_location=str(self.course.location), + course_key=self.course.id, + usage_key_str=str(self.course.location), ) self.assertEquals(len(student_data), 4) - def test_build_student_data(self): + @patch( + 'lms.djangoapps.instructor_task.tasks_helper.grades.list_problem_responses', + wraps=list_problem_responses + ) + def test_build_student_data_for_block_without_generate_report_data(self, mock_list_problem_responses): + """ + Ensure that building student data for a block the doesn't have the + ``generate_report_data`` method works as expected. + """ self.define_option_problem(u'Problem1') self.submit_student_answer(self.student.username, u'Problem1', ['Option 1']) student_data = ProblemResponses._build_student_data( user_id=self.instructor.id, - course_id=self.course.id, - problem_location=str(self.course.location), + course_key=self.course.id, + usage_key_str=str(self.course.location), ) self.assertEquals(len(student_data), 1) self.assertDictContainsSubset({ @@ -504,6 +529,33 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): 'block_key': 'i4x://edx/1.23x/problem/Problem1', 'title': 'Problem1', }, student_data[0]) + self.assertIn('state', student_data[0]) + mock_list_problem_responses.assert_called_with(self.course.id, ANY, ANY) + + @patch('xmodule.capa_module.CapaDescriptor.generate_report_data', create=True) + def test_build_student_data_for_block_with_generate_report_data(self, mock_generate_report_data): + """ + Ensure that building student data for a block that supports the + ``generate_report_data`` method works as expected. + """ + self.define_option_problem(u'Problem1') + state = {'some': 'state'} + mock_generate_report_data.return_value = iter([ + ('student', state), + ]) + student_data = ProblemResponses._build_student_data( + user_id=self.instructor.id, + course_key=self.course.id, + usage_key_str=str(self.course.location), + ) + self.assertEquals(len(student_data), 1) + self.assertDictContainsSubset({ + 'username': 'student', + 'location': 'test_course > Section > Subsection > Problem1', + 'block_key': 'i4x://edx/1.23x/problem/Problem1', + 'title': 'Problem1', + 'state': state, + }, student_data[0]) def test_success(self): task_input = { @@ -512,8 +564,8 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): } with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task'): with patch('lms.djangoapps.instructor_task.tasks_helper.grades' - '.ProblemResponses._build_student_data') as patched_data_source: - patched_data_source.return_value = [ + '.ProblemResponses._build_student_data') as mock_build_student_data: + mock_build_student_data.return_value = [ {'username': 'user0', 'state': u'state0'}, {'username': 'user1', 'state': u'state1'}, {'username': 'user2', 'state': u'state2'},