diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 3050417dd9..640d27e46c 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -1003,30 +1003,62 @@ def get_problem_responses(request, course_id): Initiate generation of a CSV file containing all student answers to a given problem. - Responds with JSON - {"status": "... status message ...", "task_id": created_task_UUID} + **Example requests** - if initiation is successful (or generation task is already running). + POST /courses/{course_id}/instructor/api/get_problem_responses { + "problem_location": "{usage_key1},{usage_key2},{usage_key3}"" + } + POST /courses/{course_id}/instructor/api/get_problem_responses { + "problem_location": "{usage_key}", + "problem_types_filter": "problem" + } - Responds with BadRequest if problem location is faulty. + **POST Parameters** + + A POST request can include the following parameters: + + * problem_location: A comma-separated list of usage keys for the blocks + to include in the report. If the location is a block that contains + other blocks, (such as the course, section, subsection, or unit blocks) + then all blocks under that block will be included in the report. + * problem_types_filter: Optional. A comma-separated list of block types + to include in the repot. If set, only blocks of the specified types will + be included in the report. + + To get data on all the poll and survey blocks in a course, you could + POST the usage key of the course for `problem_location`, and + "poll, survey" as the value for `problem_types_filter`. + + + **Example Response:** + If initiation is successful (or generation task is already running): + ```json + { + "status": "The problem responses report is being created. ...", + "task_id": "4e49522f-31d9-431a-9cff-dd2a2bf4c85a" + } + ``` + + Responds with BadRequest if any of the provided problem locations are 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 5ec1ffcb65..013840cb81 100644 --- a/lms/djangoapps/instructor_task/api.py +++ b/lms/djangoapps/instructor_task/api.py @@ -322,7 +322,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. @@ -331,7 +333,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_locations': 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 2e00eabfc9..d87627f079 100644 --- a/lms/djangoapps/instructor_task/tasks.py +++ b/lms/djangoapps/instructor_task/tasks.py @@ -159,7 +159,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 8491e9a2dd..1fda96f281 100644 --- a/lms/djangoapps/instructor_task/tasks_helper/grades.py +++ b/lms/djangoapps/instructor_task/tasks_helper/grades.py @@ -3,13 +3,14 @@ Functionality for generating grade reports. """ import logging -import re from collections import OrderedDict, defaultdict from datetime import datetime from itertools import chain from time import time +import re import six +from course_blocks.api import get_course_blocks from django.conf import settings from django.contrib.auth import get_user_model from lazy import lazy @@ -18,33 +19,33 @@ from pytz import UTC from six import text_type from six.moves import zip, zip_longest -from course_blocks.api import get_course_blocks from course_modes.models import CourseMode from lms.djangoapps.certificates.models import CertificateWhitelist, GeneratedCertificate, certificate_info_for_user from lms.djangoapps.courseware.courses import get_course_by_id from lms.djangoapps.courseware.user_state_client import DjangoXBlockUserStateClient -from lms.djangoapps.grades.api import CourseGradeFactory -from lms.djangoapps.grades.api import context as grades_context -from lms.djangoapps.grades.api import prefetch_course_and_subsection_grades +from lms.djangoapps.grades.api import ( + CourseGradeFactory, + context as grades_context, + prefetch_course_and_subsection_grades, +) from lms.djangoapps.instructor_analytics.basic import list_problem_responses from lms.djangoapps.instructor_analytics.csvs import format_dictlist from lms.djangoapps.instructor_task.config.waffle import ( course_grade_report_verified_only, optimize_get_learners_switch_enabled, - problem_grade_report_verified_only + problem_grade_report_verified_only, ) from lms.djangoapps.teams.models import CourseTeamMembership from lms.djangoapps.verify_student.services import IDVerificationService -from openedx.core.lib.cache_utils import get_cache from openedx.core.djangoapps.content.block_structure.api import get_course_in_cache from openedx.core.djangoapps.course_groups.cohorts import bulk_cache_cohorts, get_cohort, is_course_cohorted from openedx.core.djangoapps.user_api.course_tag.api import BulkCourseTags +from openedx.core.lib.cache_utils import get_cache from student.models import CourseEnrollment from student.roles import BulkRoleCache from xmodule.modulestore.django import modulestore from xmodule.partitions.partitions_service import PartitionService from xmodule.split_test_module import get_split_user_partitions - from .runner import TaskProgress from .utils import upload_csv_to_report_store @@ -822,6 +823,23 @@ class ProblemResponses(object): Class to encapsulate functionality related to generating Problem Responses Reports. """ + @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): """ @@ -836,19 +854,21 @@ class ProblemResponses(object): Tuple[str, List[str], UsageKey]: tuple of a block's display name, path, and usage key """ - name = course_blocks.get_xblock_field(root, 'display_name') or root.category + name = course_blocks.get_xblock_field(root, 'display_name') or root.block_type if path is None: path = [name] yield name, path, root for block in course_blocks.get_children(root): - name = course_blocks.get_xblock_field(block, 'display_name') or block.category + name = course_blocks.get_xblock_field(block, 'display_name') or block.block_type for result in cls._build_problem_list(course_blocks, block, path + [name]): 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. @@ -856,16 +876,20 @@ 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') @@ -876,53 +900,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 @@ -947,13 +979,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').split(',') + 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, + filter_types=filter_types, ) for data in student_data: @@ -971,8 +1009,7 @@ class ProblemResponses(object): task_progress.update_task_state(extra_meta=current_step) # Perform the upload - problem_location = re.sub(r'[:/]', '_', problem_location) - csv_name = 'student_state_from_{}'.format(problem_location) + csv_name = cls._generate_upload_file_name(problem_locations, filter_types) report_name, report_path = upload_csv_to_report_store(rows, csv_name, course_id, start_date) current_step = { 'step': 'CSV uploaded', @@ -981,3 +1018,17 @@ class ProblemResponses(object): } return task_progress.update_task_state(extra_meta=current_step) + + @staticmethod + def _generate_upload_file_name(problem_locations, filters): + """Generate a concise file name based on the report generation parameters.""" + multiple_problems = len(problem_locations) > 1 + csv_name = 'student_state' + if multiple_problems: + csv_name += '_from_multiple_blocks' + else: + problem_location = re.sub(r'[:/]', '_', problem_locations[0]) + csv_name += '_from_' + problem_location + if filters: + csv_name += '_for_' + ','.join(filters) + return csv_name diff --git a/lms/djangoapps/instructor_task/tests/test_api.py b/lms/djangoapps/instructor_task/tests/test_api.py index d0658391d2..9d10f2e040 100644 --- a/lms/djangoapps/instructor_task/tests/test_api.py +++ b/lms/djangoapps/instructor_task/tests/test_api.py @@ -238,7 +238,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 ba841d8799..87761e0c77 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -33,6 +33,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 lms.djangoapps.courseware.models import StudentModule from lms.djangoapps.certificates.models import CertificateStatuses, GeneratedCertificate from lms.djangoapps.certificates.tests.factories import CertificateWhitelistFactory, GeneratedCertificateFactory from lms.djangoapps.courseware.tests.factories import InstructorFactory @@ -472,6 +473,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 @@ -512,7 +514,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.assertEqual(len(student_data), 4) @@ -532,12 +534,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.assertEqual(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]) @@ -561,7 +563,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.assertEqual(len(student_data), 2) self.assertDictContainsSubset({ @@ -592,7 +594,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.assertEqual(len(student_data), 1) self.assertDictContainsSubset({ @@ -607,6 +609,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.assertEqual(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.assertEqual(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( @@ -623,14 +685,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'): @@ -650,9 +712,43 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): report_store = ReportStore.from_config(config_name='GRADES_DOWNLOAD') links = report_store.links_for(self.course.id) - self.assertEqual(len(links), 1) - self.assertDictContainsSubset({'attempted': 3, 'succeeded': 3, 'failed': 0}, result) - self.assertIn("report_name", result) + assert len(links) == 1 + assert set(({'attempted': 3, 'succeeded': 3, 'failed': 0}).items()).issubset(set(result.items())) + assert "report_name" in result + + @ddt.data( + ('blkid', None, 'edx_1.23x_test_course_student_state_from_blkid_2020-01-01-0000.csv'), + ('blkid', 'poll,survey', 'edx_1.23x_test_course_student_state_from_blkid_for_poll,survey_2020-01-01-0000.csv'), + ('blkid1,blkid2', None, 'edx_1.23x_test_course_student_state_from_multiple_blocks_2020-01-01-0000.csv'), + ( + 'blkid1,blkid2', + 'poll,survey', + 'edx_1.23x_test_course_student_state_from_multiple_blocks_for_poll,survey_2020-01-01-0000.csv', + ), + ) + @ddt.unpack + def test_file_names(self, problem_locations, problem_types_filter, file_name): + task_input = { + 'problem_locations': problem_locations, + 'problem_types_filter': problem_types_filter, + 'user_id': self.instructor.id + } + with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task'), \ + freeze_time('2020-01-01'): + with patch('lms.djangoapps.instructor_task.tasks_helper.grades' + '.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'}, + ], + ['username', 'state'] + ) + result = ProblemResponses.generate( + None, None, self.course.id, task_input, 'calculated' + ) + assert result.get('report_name') == file_name @ddt.ddt