diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index de3544a98e..d191ffd460 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -1023,22 +1023,23 @@ def get_problem_responses(request, course_id): Responds with BadRequest if problem location is faulty. """ course_key = CourseKey.from_string(course_id) - problem_location = request.POST.get('problem_location', '') + # A comma-separated list of problem locations + # The name of the POST parameter is `problem_location` (not pluralised) in + # order to preserve backwards compatibility with existing third-party + # scripts. + problem_locations = request.POST.get('problem_location', '') + # A comma-separated list of block types + problem_types_filter = request.POST.get('problem_types_filter', '') report_type = _('problem responses') try: - problem_key = UsageKey.from_string(problem_location) - # Are we dealing with an "old-style" problem location? - run = problem_key.run - if not run: + for problem_location in problem_locations.split(','): problem_key = UsageKey.from_string(problem_location).map_into_course(course_key) - if problem_key.course_key != course_key: - raise InvalidKeyError(type(problem_key), problem_key) except InvalidKeyError: return JsonResponseBadRequest(_("Could not find problem with this location.")) task = task_api.submit_calculate_problem_responses_csv( - request, course_key, problem_location + request, course_key, problem_locations, problem_types_filter, ) success_status = SUCCESS_MESSAGE_TEMPLATE.format(report_type=report_type) diff --git a/lms/djangoapps/instructor_task/api.py b/lms/djangoapps/instructor_task/api.py index b587aa4a1a..fa3ac90930 100644 --- a/lms/djangoapps/instructor_task/api.py +++ b/lms/djangoapps/instructor_task/api.py @@ -324,7 +324,9 @@ def submit_bulk_course_email(request, course_key, email_id): return submit_task(request, task_type, task_class, course_key, task_input, task_key) -def submit_calculate_problem_responses_csv(request, course_key, problem_location): +def submit_calculate_problem_responses_csv( + request, course_key, problem_locations, problem_types_filter=None, +): """ Submits a task to generate a CSV file containing all student answers to a given problem. @@ -333,7 +335,11 @@ def submit_calculate_problem_responses_csv(request, course_key, problem_location """ task_type = 'problem_responses_csv' task_class = calculate_problem_responses_csv - task_input = {'problem_location': problem_location, 'user_id': request.user.pk} + task_input = { + 'problem_location': problem_locations, + 'problem_types_filter': problem_types_filter, + 'user_id': request.user.pk, + } task_key = "" return submit_task(request, task_type, task_class, course_key, task_input, task_key) diff --git a/lms/djangoapps/instructor_task/tasks.py b/lms/djangoapps/instructor_task/tasks.py index 89dcd6f6b8..4946b0c6d5 100644 --- a/lms/djangoapps/instructor_task/tasks.py +++ b/lms/djangoapps/instructor_task/tasks.py @@ -161,7 +161,11 @@ def send_bulk_course_email(entry_id, _xmodule_instance_args): return run_main_task(entry_id, visit_fcn, action_name) -@task(base=BaseInstructorTask, routing_key=settings.GRADES_DOWNLOAD_ROUTING_KEY) +@task( + name='lms.djangoapps.instructor_task.tasks.calculate_problem_responses_csv.v2', + base=BaseInstructorTask, + routing_key=settings.GRADES_DOWNLOAD_ROUTING_KEY, +) def calculate_problem_responses_csv(entry_id, xmodule_instance_args): """ Compute student answers to a given problem and upload the CSV to diff --git a/lms/djangoapps/instructor_task/tasks_helper/grades.py b/lms/djangoapps/instructor_task/tasks_helper/grades.py index c97887b065..9de0a3ca61 100644 --- a/lms/djangoapps/instructor_task/tasks_helper/grades.py +++ b/lms/djangoapps/instructor_task/tasks_helper/grades.py @@ -604,6 +604,23 @@ class ProblemGradeReport(object): class ProblemResponses(object): + @staticmethod + def _build_block_base_path(block): + """ + Return the display names of the blocks that lie above the supplied block in hierarchy. + + Arguments: + block: a single block + + Returns: + List[str]: a list of display names of blocks starting from the root block (Course) + """ + path = [] + while block.parent: + block = block.get_parent() + path.append(block.display_name) + return list(reversed(path)) + @classmethod def _build_problem_list(cls, course_blocks, root, path=None): """ @@ -632,7 +649,9 @@ class ProblemResponses(object): yield result @classmethod - def _build_student_data(cls, user_id, course_key, usage_key_str): + def _build_student_data( + cls, user_id, course_key, usage_key_str_list, filter_types=None, + ): """ Generate a list of problem responses for all problem under the ``problem_location`` root. @@ -641,17 +660,21 @@ class ProblemResponses(object): user_id (int): The user id for the user generating the report course_key (CourseKey): The ``CourseKey`` for the course whose report is being generated - usage_key_str (str): The generated report will include this - block and it child blocks. + usage_key_str_list (List[str]): The generated report will include these + blocks and their child blocks. + filter_types (List[str]): The report generator will only include data for + block types in this list. Returns: Tuple[List[Dict], List[str]]: Returns a list of dictionaries containing the student data which will be included in the final csv, and the features/keys to include in that CSV. """ - usage_key = UsageKey.from_string(usage_key_str).map_into_course(course_key) + usage_keys = [ + UsageKey.from_string(usage_key_str).map_into_course(course_key) + for usage_key_str in usage_key_str_list + ] user = get_user_model().objects.get(pk=user_id) - course_blocks = get_course_blocks(user, usage_key) student_data = [] max_count = settings.FEATURES.get('MAX_PROBLEM_RESPONSES_COUNT') @@ -662,53 +685,61 @@ class ProblemResponses(object): student_data_keys = set() 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 + for usage_key in usage_keys: + if max_count is not None and max_count <= 0: + break + course_blocks = get_course_blocks(user, usage_key) + base_path = cls._build_block_base_path(store.get_item(usage_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) - generated_report_data = defaultdict(list) + if filter_types is not None and block_key.block_type not in filter_types: + continue - # Blocks can implement the generate_report_data method to provide their own - # human-readable formatting for user state. - if hasattr(block, 'generate_report_data'): - try: - user_state_iterator = user_state_client.iter_all_for_block(block_key) - for username, state in block.generate_report_data(user_state_iterator, max_count): - generated_report_data[username].append(state) - except NotImplementedError: - pass + block = store.get_item(block_key) + generated_report_data = defaultdict(list) - responses = [] + # Blocks can implement the generate_report_data method to provide their own + # human-readable formatting for user state. + if hasattr(block, 'generate_report_data'): + try: + user_state_iterator = user_state_client.iter_all_for_block(block_key) + for username, state in block.generate_report_data(user_state_iterator, max_count): + generated_report_data[username].append(state) + except NotImplementedError: + pass - for response in list_problem_responses(course_key, block_key, max_count): - 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) - # A block that has a single state per user can contain multiple responses - # within the same state. - user_states = generated_report_data.get(response['username'], []) - if user_states: - # For each response in the block, copy over the basic data like the - # title, location, block_key and state, and add in the responses - for user_state in user_states: - user_response = response.copy() - user_response.update(user_state) - student_data_keys = student_data_keys.union(list(user_state.keys())) - responses.append(user_response) - else: - responses.append(response) + responses = [] - student_data += responses + for response in list_problem_responses(course_key, block_key, max_count): + response['title'] = title + # A human-readable location for the current block + response['location'] = ' > '.join(base_path + path) + # A machine-friendly location for the current block + response['block_key'] = str(block_key) + # A block that has a single state per user can contain multiple responses + # within the same state. + user_states = generated_report_data.get(response['username']) + if user_states: + # For each response in the block, copy over the basic data like the + # title, location, block_key and state, and add in the responses + for user_state in user_states: + user_response = response.copy() + user_response.update(user_state) + student_data_keys = student_data_keys.union(list(user_state.keys())) + responses.append(user_response) + else: + responses.append(response) - if max_count is not None: - max_count -= len(responses) - if max_count <= 0: - break + student_data += responses + + if max_count is not None: + max_count -= len(responses) + if max_count <= 0: + break # Keep the keys in a useful order, starting with username, title and location, # then the columns returned by the xblock report generator in sorted order and @@ -733,13 +764,19 @@ class ProblemResponses(object): task_progress = TaskProgress(action_name, num_reports, start_time) current_step = {'step': 'Calculating students answers to problem'} task_progress.update_task_state(extra_meta=current_step) - problem_location = task_input.get('problem_location') + problem_locations = task_input.get('problem_locations') + problem_types_filter = task_input.get('problem_types_filter') + + filter_types = None + if problem_types_filter: + filter_types = problem_types_filter.split(',') # Compute result table and format it student_data, student_data_keys = cls._build_student_data( user_id=task_input.get('user_id'), course_key=course_id, - usage_key_str=problem_location + usage_key_str_list=problem_locations.split(','), + filter_types=filter_types, ) for data in student_data: @@ -757,7 +794,9 @@ class ProblemResponses(object): task_progress.update_task_state(extra_meta=current_step) # Perform the upload - problem_location = re.sub(r'[:/]', '_', problem_location) + # Limit problem locations string to 200 characters in case a large number of + # problem locations are selected. + problem_location = re.sub(r'[:/]', '_', problem_locations)[:200] csv_name = 'student_state_from_{}'.format(problem_location) report_name = upload_csv_to_report_store(rows, csv_name, course_id, start_date) current_step = {'step': 'CSV uploaded', 'report_name': report_name} diff --git a/lms/djangoapps/instructor_task/tests/test_api.py b/lms/djangoapps/instructor_task/tests/test_api.py index 042cba4a9d..ed8a678894 100644 --- a/lms/djangoapps/instructor_task/tests/test_api.py +++ b/lms/djangoapps/instructor_task/tests/test_api.py @@ -262,7 +262,7 @@ class InstructorTaskCourseSubmitTest(TestReportMixin, InstructorTaskCourseTestCa api_call = lambda: submit_calculate_problem_responses_csv( self.create_task_request(self.instructor), self.course.id, - problem_location='' + problem_locations='', ) self._test_resubmission(api_call) diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index bf4c760f51..271d1f33a7 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -32,6 +32,7 @@ import openedx.core.djangoapps.user_api.course_tag.api as course_tag_api from capa.tests.response_xml_factory import MultipleChoiceResponseXMLFactory from course_modes.models import CourseMode from course_modes.tests.factories import CourseModeFactory +from courseware.models import StudentModule from courseware.tests.factories import InstructorFactory from instructor_analytics.basic import UNAVAILABLE, list_problem_responses from lms.djangoapps.certificates.models import CertificateStatuses, GeneratedCertificate @@ -478,6 +479,7 @@ class TestTeamGradeReport(InstructorGradeReportTestCase): # pylint: disable=protected-access +@ddt.ddt class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): """ Tests that generation of CSV files listing student answers to a @@ -518,7 +520,7 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): student_data, _ = ProblemResponses._build_student_data( user_id=self.instructor.id, course_key=self.course.id, - usage_key_str=str(self.course.location), + usage_key_str_list=[str(self.course.location)], ) self.assertEquals(len(student_data), 4) @@ -538,12 +540,12 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): student_data, _ = ProblemResponses._build_student_data( user_id=self.instructor.id, course_key=self.course.id, - usage_key_str=str(problem.location), + usage_key_str_list=[str(problem.location)], ) self.assertEquals(len(student_data), 1) self.assertDictContainsSubset({ 'username': 'student', - 'location': 'Problem1', + 'location': 'test_course > Section > Subsection > Problem1', 'block_key': 'i4x://edx/1.23x/problem/Problem1', 'title': 'Problem1', }, student_data[0]) @@ -567,7 +569,7 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): student_data, _ = ProblemResponses._build_student_data( user_id=self.instructor.id, course_key=self.course.id, - usage_key_str=str(self.course.location), + usage_key_str_list=[str(self.course.location)], ) self.assertEquals(len(student_data), 2) self.assertDictContainsSubset({ @@ -598,7 +600,7 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): student_data, _ = ProblemResponses._build_student_data( user_id=self.instructor.id, course_key=self.course.id, - usage_key_str=str(self.course.location), + usage_key_str_list=[str(self.course.location)], ) self.assertEquals(len(student_data), 1) self.assertDictContainsSubset({ @@ -613,6 +615,66 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): }, student_data[0]) self.assertIn('state', student_data[0]) + def test_build_student_data_for_multiple_problems(self): + """ + Ensure that building student data works when supplied multiple usage keys. + """ + problem1 = self.define_option_problem(u'Problem1') + problem2 = self.define_option_problem(u'Problem2') + self.submit_student_answer(self.student.username, u'Problem1', ['Option 1']) + self.submit_student_answer(self.student.username, u'Problem2', ['Option 1']) + student_data, _ = ProblemResponses._build_student_data( + user_id=self.instructor.id, + course_key=self.course.id, + usage_key_str_list=[str(problem1.location), str(problem2.location)], + ) + self.assertEquals(len(student_data), 2) + for idx in range(1, 3): + self.assertDictContainsSubset({ + 'username': 'student', + 'location': u'test_course > Section > Subsection > Problem{}'.format(idx), + 'block_key': 'i4x://edx/1.23x/problem/Problem{}'.format(idx), + 'title': u'Problem{}'.format(idx), + 'Answer ID': 'i4x-edx-1_23x-problem-Problem{}_2_1'.format(idx), + 'Answer': u'Option 1', + 'Correct Answer': u'Option 1', + 'Question': u'The correct answer is Option 1', + }, student_data[idx - 1]) + self.assertIn('state', student_data[idx - 1]) + + @ddt.data( + (['problem'], 5), + (['other'], 0), + (['problem', 'test-category'], 10), + (None, 10), + ) + @ddt.unpack + def test_build_student_data_with_filter(self, filters, filtered_count): + """ + Ensure that building student data works when supplied multiple usage keys. + """ + for idx in range(1, 6): + self.define_option_problem(u'Problem{}'.format(idx)) + item = ItemFactory.create( + parent_location=self.problem_section.location, + parent=self.problem_section, + category="test-category", + display_name=u"Item{}".format(idx), + data='' + ) + StudentModule.save_state(self.student, self.course.id, item.location, {}) + + for idx in range(1, 6): + self.submit_student_answer(self.student.username, u'Problem{}'.format(idx), ['Option 1']) + + student_data, _ = ProblemResponses._build_student_data( + user_id=self.instructor.id, + course_key=self.course.id, + usage_key_str_list=[str(self.course.location)], + filter_types=filters, + ) + self.assertEquals(len(student_data), filtered_count) + @patch('lms.djangoapps.instructor_task.tasks_helper.grades.list_problem_responses') @patch('xmodule.capa_module.ProblemBlock.generate_report_data', create=True) def test_build_student_data_for_block_with_generate_report_data_not_implemented( @@ -629,14 +691,14 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): ProblemResponses._build_student_data( user_id=self.instructor.id, course_key=self.course.id, - usage_key_str=str(problem.location), + usage_key_str_list=[str(problem.location)], ) mock_generate_report_data.assert_called_with(ANY, ANY) mock_list_problem_responses.assert_called_with(self.course.id, ANY, ANY) def test_success(self): task_input = { - 'problem_location': str(self.course.location), + 'problem_locations': str(self.course.location), 'user_id': self.instructor.id } with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task'):