From 491532c2db4e8d8367bd1fd28b0652ef5a93c060 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Wed, 4 Jul 2018 00:30:33 +0530 Subject: [PATCH] Update report generation code to include old state for backwards compatibility Include report data generated by blocks as columns instead of a json dict --- cms/djangoapps/contentstore/tasks.py | 2 +- .../instructor_task/tasks_helper/grades.py | 43 ++++++++++---- .../tests/test_tasks_helper.py | 59 +++++++++++-------- 3 files changed, 68 insertions(+), 36 deletions(-) diff --git a/cms/djangoapps/contentstore/tasks.py b/cms/djangoapps/contentstore/tasks.py index d695d5cf10..cf56587706 100644 --- a/cms/djangoapps/contentstore/tasks.py +++ b/cms/djangoapps/contentstore/tasks.py @@ -185,7 +185,7 @@ def async_migrate_transcript(self, course_key, **kwargs): # pylint: disable=un sub_tasks = [] video_location = unicode(video.location) - for lang in all_transcripts.keys(): + for lang in all_transcripts: sub_tasks.append(async_migrate_transcript_subtask.s( video_location, revision, lang, force_update, **kwargs )) diff --git a/lms/djangoapps/instructor_task/tasks_helper/grades.py b/lms/djangoapps/instructor_task/tasks_helper/grades.py index 606d46b840..73ec6fb9d0 100644 --- a/lms/djangoapps/instructor_task/tasks_helper/grades.py +++ b/lms/djangoapps/instructor_task/tasks_helper/grades.py @@ -595,8 +595,9 @@ class ProblemResponses(object): 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. + 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) user = get_user_model().objects.get(pk=user_id) @@ -608,6 +609,8 @@ class ProblemResponses(object): store = modulestore() user_state_client = DjangoXBlockUserStateClient() + 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 @@ -616,21 +619,22 @@ class ProblemResponses(object): continue block = store.get_item(block_key) + generated_report_data = {} # 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) - responses = [ - {'username': username, 'state': state} + generated_report_data = { + username: state for username, state in block.generate_report_data(user_state_iterator, max_count) - ] + } except NotImplementedError: - responses = list_problem_responses(course_key, block_key, max_count) - else: - responses = list_problem_responses(course_key, block_key, max_count) + pass + + responses = list_problem_responses(course_key, block_key, max_count) student_data += responses for response in responses: @@ -639,12 +643,24 @@ class ProblemResponses(object): response['location'] = ' > '.join(path) # A machine-friendly location for the current block response['block_key'] = str(block_key) + user_data = generated_report_data.get(response['username'], {}) + response.update(user_data) + student_data_keys = student_data_keys.union(user_data.keys()) if max_count is not None: max_count -= len(responses) if max_count <= 0: break - return student_data + # 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 + # finally end with the more machine friendly block_key and state. + student_data_keys_list = ( + ['username', 'title', 'location'] + + sorted(student_data_keys) + + ['block_key', 'state'] + ) + + return student_data, student_data_keys_list @classmethod def generate(cls, _xmodule_instance_args, _entry_id, course_id, task_input, action_name): @@ -661,14 +677,17 @@ class ProblemResponses(object): problem_location = task_input.get('problem_location') # Compute result table and format it - student_data = cls._build_student_data( + 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 ) - features = ['username', 'title', 'location', 'block_key', 'state'] - header, rows = format_dictlist(student_data, features) + for data in student_data: + for key in student_data_keys: + data.setdefault(key, '') + + header, rows = format_dictlist(student_data, student_data_keys) task_progress.attempted = task_progress.succeeded = len(rows) task_progress.skipped = task_progress.total - task_progress.attempted diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index e761465f36..9c11305af9 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -36,10 +36,15 @@ from shoppingcart.models import ( Invoice, InvoiceTransaction, Order, - PaidCourseRegistration + PaidCourseRegistration, ) from six import text_type -from student.models import ALLOWEDTOENROLL_TO_ENROLLED, CourseEnrollment, CourseEnrollmentAllowed, ManualEnrollmentAudit +from student.models import ( + ALLOWEDTOENROLL_TO_ENROLLED, + CourseEnrollment, + CourseEnrollmentAllowed, + ManualEnrollmentAudit, +) from student.tests.factories import CourseEnrollmentFactory, UserFactory from survey.models import SurveyAnswer, SurveyForm from xmodule.modulestore import ModuleStoreEnum @@ -57,24 +62,24 @@ from lms.djangoapps.instructor_task.tasks_helper.enrollments import ( upload_enrollment_report, upload_exec_summary_report, upload_may_enroll_csv, - upload_students_csv + upload_students_csv, ) from lms.djangoapps.instructor_task.tasks_helper.grades import ( ENROLLED_IN_COURSE, NOT_ENROLLED_IN_COURSE, CourseGradeReport, ProblemGradeReport, - ProblemResponses + ProblemResponses, ) from lms.djangoapps.instructor_task.tasks_helper.misc import ( cohort_students_and_upload, upload_course_survey_report, - upload_ora2_data + upload_ora2_data, ) from lms.djangoapps.instructor_task.tests.test_base import ( InstructorTaskCourseTestCase, InstructorTaskModuleTestCase, - TestReportMixin + TestReportMixin, ) from lms.djangoapps.teams.tests.factories import CourseTeamFactory, CourseTeamMembershipFactory from lms.djangoapps.verify_student.tests.factories import SoftwareSecurePhotoVerificationFactory @@ -481,6 +486,7 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): Tests that generation of CSV files listing student answers to a given problem works. """ + def setUp(self): super(TestProblemResponsesReport, self).setUp() self.initialize_course() @@ -510,7 +516,7 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): student = self.create_student('student{}'.format(ctr)) self.submit_student_answer(student.username, u'Problem1', ['Option 1']) - student_data = ProblemResponses._build_student_data( + student_data, _ = ProblemResponses._build_student_data( user_id=self.instructor.id, course_key=self.course.id, usage_key_str=str(self.course.location), @@ -530,7 +536,7 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): problem = self.define_option_problem(u'Problem1') self.submit_student_answer(self.student.username, u'Problem1', ['Option 1']) with self._remove_capa_report_generator(): - student_data = ProblemResponses._build_student_data( + student_data, _ = ProblemResponses._build_student_data( user_id=self.instructor.id, course_key=self.course.id, usage_key_str=str(problem.location), @@ -552,11 +558,12 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): ``generate_report_data`` method works as expected. """ self.define_option_problem(u'Problem1') - state = {'some': 'state'} + self.submit_student_answer(self.student.username, u'Problem1', ['Option 1']) + state = {'some': 'state', 'more': 'state!'} mock_generate_report_data.return_value = iter([ ('student', state), ]) - student_data = ProblemResponses._build_student_data( + student_data, _ = ProblemResponses._build_student_data( user_id=self.instructor.id, course_key=self.course.id, usage_key_str=str(self.course.location), @@ -567,7 +574,8 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): 'location': 'test_course > Section > Subsection > Problem1', 'block_key': 'i4x://edx/1.23x/problem/Problem1', 'title': 'Problem1', - 'state': state, + 'some': 'state', + 'more': 'state!', }, student_data[0]) def test_build_student_data_for_block_with_real_generate_report_data(self): @@ -577,7 +585,7 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): """ self.define_option_problem(u'Problem1') self.submit_student_answer(self.student.username, u'Problem1', ['Option 1']) - student_data = ProblemResponses._build_student_data( + student_data, _ = ProblemResponses._build_student_data( user_id=self.instructor.id, course_key=self.course.id, usage_key_str=str(self.course.location), @@ -588,12 +596,11 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): 'location': 'test_course > Section > Subsection > Problem1', 'block_key': 'i4x://edx/1.23x/problem/Problem1', 'title': 'Problem1', - 'state': { - 'Answer ID': 'i4x-edx-1_23x-problem-Problem1_2_1', - 'Answer': 'Option 1', - 'Question': u'The correct answer is Option 1' - }, + 'Answer ID': 'i4x-edx-1_23x-problem-Problem1_2_1', + 'Answer': 'Option 1', + 'Question': u'The correct answer is Option 1', }, student_data[0]) + self.assertIn('state', student_data[0]) @patch('lms.djangoapps.instructor_task.tasks_helper.grades.list_problem_responses') @patch('xmodule.capa_module.CapaDescriptor.generate_report_data', create=True) @@ -624,11 +631,14 @@ 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 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'}, - ] + 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' ) @@ -1620,7 +1630,10 @@ class TestCohortStudents(TestReportMixin, InstructorTaskCourseTestCase): self.cohort_2 = CohortFactory(course_id=self.course.id, name='Cohort 2') self.student_1 = self.create_student(username=u'student_1\xec', email='student_1@example.com') self.student_2 = self.create_student(username='student_2', email='student_2@example.com') - self.csv_header_row = ['Cohort Name', 'Exists', 'Learners Added', 'Learners Not Found', 'Invalid Email Addresses', 'Preassigned Learners'] + self.csv_header_row = [ + 'Cohort Name', 'Exists', 'Learners Added', 'Learners Not Found', + 'Invalid Email Addresses', 'Preassigned Learners', + ] def _cohort_students_and_upload(self, csv_data): """