diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index d070297e54..ac12af8673 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -1063,23 +1063,22 @@ def get_problem_responses(request, course_id): Responds with BadRequest if problem location is faulty. """ course_key = CourseKey.from_string(course_id) - # 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', '') + problem_location = request.POST.get('problem_location', '') report_type = _('problem responses') try: - for problem_location in problem_locations.split(','): + problem_key = UsageKey.from_string(problem_location) + # Are we dealing with an "old-style" problem location? + run = problem_key.run + if not run: 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_locations, problem_types_filter, + request, course_key, problem_location ) 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 fa3ac90930..b587aa4a1a 100644 --- a/lms/djangoapps/instructor_task/api.py +++ b/lms/djangoapps/instructor_task/api.py @@ -324,9 +324,7 @@ 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_locations, problem_types_filter=None, -): +def submit_calculate_problem_responses_csv(request, course_key, problem_location): """ Submits a task to generate a CSV file containing all student answers to a given problem. @@ -335,11 +333,7 @@ def submit_calculate_problem_responses_csv( """ task_type = 'problem_responses_csv' task_class = calculate_problem_responses_csv - task_input = { - 'problem_location': problem_locations, - 'problem_types_filter': problem_types_filter, - 'user_id': request.user.pk, - } + task_input = {'problem_location': problem_location, '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 4946b0c6d5..89dcd6f6b8 100644 --- a/lms/djangoapps/instructor_task/tasks.py +++ b/lms/djangoapps/instructor_task/tasks.py @@ -161,11 +161,7 @@ def send_bulk_course_email(entry_id, _xmodule_instance_args): return run_main_task(entry_id, visit_fcn, action_name) -@task( - name='lms.djangoapps.instructor_task.tasks.calculate_problem_responses_csv.v2', - base=BaseInstructorTask, - routing_key=settings.GRADES_DOWNLOAD_ROUTING_KEY, -) +@task(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 9804ced427..5ca6f833f2 100644 --- a/lms/djangoapps/instructor_task/tasks_helper/grades.py +++ b/lms/djangoapps/instructor_task/tasks_helper/grades.py @@ -604,23 +604,6 @@ 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): """ @@ -649,9 +632,7 @@ class ProblemResponses(object): yield result @classmethod - def _build_student_data( - cls, user_id, course_key, usage_key_str_list, filter_types=None, - ): + 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. @@ -660,21 +641,17 @@ 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_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. + usage_key_str (str): The generated report will include this + block and it child blocks. 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_keys = [ - UsageKey.from_string(usage_key_str).map_into_course(course_key) - for usage_key_str in usage_key_str_list - ] + 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, usage_key) student_data = [] max_count = settings.FEATURES.get('MAX_PROBLEM_RESPONSES_COUNT') @@ -685,61 +662,53 @@ class ProblemResponses(object): student_data_keys = set() with store.bulk_operations(course_key): - 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 + 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 - if filter_types is not None and block_key.block_type not in filter_types: - continue + block = store.get_item(block_key) + generated_report_data = defaultdict(list) - block = store.get_item(block_key) - generated_report_data = defaultdict(list) + # 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 - # 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 + responses = [] - 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(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) - 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) + student_data += responses - student_data += responses - - if max_count is not None: - max_count -= len(responses) - if max_count <= 0: - break + 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 @@ -764,19 +733,13 @@ 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_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(',') + problem_location = task_input.get('problem_location') # 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_list=problem_locations.split(','), - filter_types=filter_types, + usage_key_str=problem_location ) for data in student_data: @@ -794,9 +757,7 @@ class ProblemResponses(object): task_progress.update_task_state(extra_meta=current_step) # Perform the upload - # 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] + problem_location = re.sub(r'[:/]', '_', problem_location) 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 531378d4de..eae423d5b0 100644 --- a/lms/djangoapps/instructor_task/tests/test_api.py +++ b/lms/djangoapps/instructor_task/tests/test_api.py @@ -240,7 +240,7 @@ class InstructorTaskCourseSubmitTest(TestReportMixin, InstructorTaskCourseTestCa api_call = lambda: submit_calculate_problem_responses_csv( self.create_task_request(self.instructor), self.course.id, - problem_locations='', + problem_location='' ) 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 d9c68e82fb..1bf308fda9 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -32,7 +32,6 @@ 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 lms.djangoapps.instructor_analytics.basic import UNAVAILABLE, list_problem_responses from lms.djangoapps.certificates.models import CertificateStatuses, GeneratedCertificate @@ -479,7 +478,6 @@ class TestTeamGradeReport(InstructorGradeReportTestCase): # pylint: disable=protected-access -@ddt.ddt class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): """ Tests that generation of CSV files listing student answers to a @@ -520,7 +518,7 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): student_data, _ = ProblemResponses._build_student_data( user_id=self.instructor.id, course_key=self.course.id, - usage_key_str_list=[str(self.course.location)], + usage_key_str=str(self.course.location), ) self.assertEquals(len(student_data), 4) @@ -540,12 +538,12 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): student_data, _ = ProblemResponses._build_student_data( user_id=self.instructor.id, course_key=self.course.id, - usage_key_str_list=[str(problem.location)], + usage_key_str=str(problem.location), ) self.assertEquals(len(student_data), 1) self.assertDictContainsSubset({ 'username': 'student', - 'location': 'test_course > Section > Subsection > Problem1', + 'location': 'Problem1', 'block_key': 'i4x://edx/1.23x/problem/Problem1', 'title': 'Problem1', }, student_data[0]) @@ -569,7 +567,7 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): student_data, _ = ProblemResponses._build_student_data( user_id=self.instructor.id, course_key=self.course.id, - usage_key_str_list=[str(self.course.location)], + usage_key_str=str(self.course.location), ) self.assertEquals(len(student_data), 2) self.assertDictContainsSubset({ @@ -600,7 +598,7 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): student_data, _ = ProblemResponses._build_student_data( user_id=self.instructor.id, course_key=self.course.id, - usage_key_str_list=[str(self.course.location)], + usage_key_str=str(self.course.location), ) self.assertEquals(len(student_data), 1) self.assertDictContainsSubset({ @@ -615,66 +613,6 @@ 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( @@ -691,14 +629,14 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): ProblemResponses._build_student_data( user_id=self.instructor.id, course_key=self.course.id, - usage_key_str_list=[str(problem.location)], + usage_key_str=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_locations': str(self.course.location), + 'problem_location': str(self.course.location), 'user_id': self.instructor.id } with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task'):