From 2eff127e8de9391e301f1a8fba5eb034109d1682 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Mon, 11 Feb 2019 13:07:56 +0530 Subject: [PATCH] Add support for generating problem response reports for multiple blocks, or filtered block types. This change adds support for specifying multiple root blocks while generating problem response reports. It also allows specifying a block type filter so that only blocks of the filtered types will be included in the report. Finally, this change also consistenly uses absolute path for the location in the report instead of relative paths. --- lms/djangoapps/instructor/views/api.py | 56 ++++-- lms/djangoapps/instructor_task/api.py | 10 +- lms/djangoapps/instructor_task/tasks.py | 6 +- .../instructor_task/tasks_helper/grades.py | 171 ++++++++++++------ .../instructor_task/tests/test_api.py | 2 +- .../tests/test_tasks_helper.py | 116 +++++++++++- 6 files changed, 275 insertions(+), 86 deletions(-) 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