From 491532c2db4e8d8367bd1fd28b0652ef5a93c060 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Wed, 4 Jul 2018 00:30:33 +0530 Subject: [PATCH 1/3] 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): """ From c7f980c9de2362563f4fff3af5cfd28084920e62 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Thu, 5 Jul 2018 05:16:39 +0530 Subject: [PATCH 2/3] Add correct answer to response --- common/lib/capa/capa/capa_problem.py | 21 ++++++++++ .../lib/capa/capa/tests/test_capa_problem.py | 38 +++++++++++++++++++ common/lib/xmodule/xmodule/capa_module.py | 8 +++- .../tests/test_tasks_helper.py | 1 + 4 files changed, 66 insertions(+), 2 deletions(-) diff --git a/common/lib/capa/capa/capa_problem.py b/common/lib/capa/capa/capa_problem.py index 5c4e642345..c13567c82d 100644 --- a/common/lib/capa/capa/capa_problem.py +++ b/common/lib/capa/capa/capa_problem.py @@ -495,6 +495,27 @@ class LoncapaProblem(object): answer_ids.append(results.keys()) return answer_ids + def find_correct_answer_text(self, answer_id): + """ + Returns the correct answer(s) for the provided answer_id as a single string. + + Arguments:: + answer_id (str): a string like "98e6a8e915904d5389821a94e48babcf_13_1" + + Returns: + str: A string containing the answer or multiple answers separated by commas. + """ + xml_elements = self.tree.xpath('//*[@id="' + answer_id + '"]') + if not xml_elements: + return + xml_element = xml_elements[0] + answer_text = xml_element.xpath('@answer') + if answer_text: + return answer_id[0] + if xml_element.tag == 'optioninput': + return xml_element.xpath('@correct')[0] + return ', '.join(xml_element.xpath('*[@correct="true"]/text()')) + def find_question_label(self, answer_id): """ Obtain the most relevant question text for a particular answer. diff --git a/common/lib/capa/capa/tests/test_capa_problem.py b/common/lib/capa/capa/tests/test_capa_problem.py index 828c5fdc5c..59ca1d4421 100644 --- a/common/lib/capa/capa/tests/test_capa_problem.py +++ b/common/lib/capa/capa/tests/test_capa_problem.py @@ -659,6 +659,44 @@ class CAPAProblemReportHelpersTest(unittest.TestCase): ) self.assertEquals(problem.find_answer_text(answer_id, choice_id), answer_text) + @ddt.data( + # Test for ChoiceResponse + ('1_2_1', 'over-suspicious'), + # Test for MultipleChoiceResponse + ('1_3_1', 'The iPad, Napster'), + # Test for OptionResponse + ('1_4_1', 'blue'), + ) + @ddt.unpack + def test_find_correct_answer_text_choices(self, answer_id, answer_text): + """ + Verify that ``find_correct_answer_text`` can find the correct answer for + ChoiceResponse, MultipleChoiceResponse and OptionResponse problems. + """ + problem = new_loncapa_problem( + """ + + + + over-suspicious + funny + + + + + The iPad + Napster + The iPod + + + + + + + """ + ) + self.assertEquals(problem.find_correct_answer_text(answer_id), answer_text) + def test_find_answer_text_textinput(self): problem = new_loncapa_problem( """ diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index a6077b2da3..2c9e73d101 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -393,13 +393,17 @@ class CapaDescriptor(CapaFields, RawDescriptor): question_text = lcp.find_question_label(answer_id) answer_text = lcp.find_answer_text(answer_id, current_answer=orig_answers) + correct_answer_text = lcp.find_correct_answer_text(answer_id) count += 1 - yield (user_state.username, { + report = { _("Answer ID"): answer_id, _("Question"): question_text, _("Answer"): answer_text, - }) + } + if correct_answer_text is not None: + report[_("Correct Answer")] = correct_answer_text + yield (user_state.username, report) # Proxy to CapaModule for access to any of its attributes answer_available = module_attr('answer_available') diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index 9c11305af9..6f6fd348b9 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -598,6 +598,7 @@ class TestProblemResponsesReport(TestReportMixin, InstructorTaskModuleTestCase): 'title': 'Problem1', 'Answer ID': 'i4x-edx-1_23x-problem-Problem1_2_1', 'Answer': 'Option 1', + 'Correct Answer': u'Option 1', 'Question': u'The correct answer is Option 1', }, student_data[0]) self.assertIn('state', student_data[0]) From 4d48a59f3f7f26d703482ca1156834c094bfd608 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Tue, 10 Jul 2018 02:21:02 +0530 Subject: [PATCH 3/3] Make stylelint fixes to get over quality threshold --- cms/static/sass/views/_video-upload.scss | 96 +++++++++++++++++------- 1 file changed, 67 insertions(+), 29 deletions(-) diff --git a/cms/static/sass/views/_video-upload.scss b/cms/static/sass/views/_video-upload.scss index ad02b56a60..0b8c2a3c47 100644 --- a/cms/static/sass/views/_video-upload.scss +++ b/cms/static/sass/views/_video-upload.scss @@ -1,5 +1,6 @@ .view-video-uploads { - .content-primary, .content-supplementary { + .content-primary, + .content-supplementary { @include box-sizing(border-box); } @@ -8,6 +9,7 @@ @extend %t-copy; vertical-align: bottom; + @include margin-right($baseline/45); } } @@ -18,11 +20,11 @@ } .button-link { - background:none; - border:none; - padding:0; + background: none; + border: none; + padding: 0; color: $ui-link-color; - cursor:pointer + cursor: pointer; } .video-transcripts-wrapper { @@ -41,8 +43,8 @@ margin-top: ($baseline/2); .transcript-upload-status-container { - - .video-transcript-detail-status, .more-details-action { + .video-transcript-detail-status, + .more-details-action { @include font-size(12); @include line-height(12); @include margin-left($baseline/4); @@ -72,9 +74,9 @@ width: 352px; transition: all 0.3s ease; background-color: $white; - -webkit-box-shadow: -3px 0px 3px 0px rgba(153,153,153,0.3); - -moz-box-shadow: -3px 0px 3px 0px rgba(153,153,153,0.3); - box-shadow: -3px 0px 3px 0px rgba(153,153,153,0.3); + -webkit-box-shadow: -3px 0 3px 0 rgba(153, 153, 153, 0.3); + -moz-box-shadow: -3px 0 3px 0 rgba(153, 153, 153, 0.3); + box-shadow: -3px 0 3px 0 rgba(153, 153, 153, 0.3); .action-close-wrapper { .action-close-course-video-settings { @@ -84,19 +86,21 @@ border: transparent; height: ($baseline*2.4); color: $text-dark-black-blue; + @include font-size(16); @include text-align(left); } } .course-video-settings-wrapper { - margin-top: ($baseline*1.60); + margin-top: ($baseline*1.6); padding: ($baseline) ($baseline*0.8); .course-video-settings-title { color: $black-t4; margin: ($baseline*1.6) 0 ($baseline*0.8) 0; font-weight: font-weight(semi-bold); + @include font-size(24); } @@ -105,7 +109,9 @@ margin-bottom: ($baseline*0.8); max-height: ($baseline*2.4); color: $black; + @include font-size(16); + .icon { @include margin-right($baseline/4); } @@ -123,6 +129,7 @@ .organization-credentials-content { margin-top: ($baseline*1.6); + .org-credentials-wrapper input { width: 65%; margin-top: ($baseline*0.8); @@ -132,15 +139,18 @@ .transcript-preferance-wrapper { margin-top: ($baseline*1.6); + .icon.fa-info-circle { @include margin-left($baseline*0.75); } } + .transcript-preferance-wrapper.error .transcript-preferance-label { color: $color-error; } - .error-info, .error-icon .fa-info-circle { + .error-info, + .error-icon .fa-info-circle { color: $color-error; } @@ -150,13 +160,18 @@ } .transcript-preferance-label { - color: $black-t4; @include font-size(15); + + color: $black-t4; font-weight: font-weight(semi-bold); display: block; } - .transcript-provider-group, .transcript-turnaround, .transcript-fidelity, .video-source-language, .selected-transcript-provider { + .transcript-provider-group, + .transcript-turnaround, + .transcript-fidelity, + .video-source-language, + .selected-transcript-provider { margin-top: ($baseline*0.8); } @@ -167,17 +182,22 @@ } .transcript-provider-group { - input[type=radio] { + input[type=radio] { margin: 0 ($baseline/2); } + label { font-weight: normal; color: $black-t4; + @include font-size(15); } } - .transcript-turnaround-wrapper, .transcript-fidelity-wrapper, .video-source-language-wrapper, .transcript-languages-wrapper { + .transcript-turnaround-wrapper, + .transcript-fidelity-wrapper, + .video-source-language-wrapper, + .transcript-languages-wrapper { display: none; } @@ -187,36 +207,46 @@ .transcript-languages-container .languages-container { margin-top: ($baseline*0.8); + .transcript-language-container { padding: ($baseline/4); background-color: $gray-l6; border-top: solid 1px $gray-l4; border-bottom: solid 1px $gray-l4; + .remove-language-action { display: inline-block; + @include float(right); } } } + .transcript-language-menu-container { margin-top: ($baseline*0.8); + .add-language-action { display: inline-block; + .action-add-language { @include margin-left($baseline/4); } + .error-info { display: inline-block; } } } - .transcript-language-menu, .video-source-language { + + .transcript-language-menu, + .video-source-language { width: 60%; } } .transcription-account-details { margin-top: ($baseline*0.8); + span { @include font-size(15); } @@ -233,8 +263,10 @@ .course-video-settings-footer { margin-top: ($baseline*1.6); + .last-updated-text { @include font-size(12); + display: block; margin-top: ($baseline/2); } @@ -279,11 +311,11 @@ } &:hover .upload-text-link { - text-decoration:underline; + text-decoration: underline; } - .fa-cloud-upload{ - font-size:7em; + .fa-cloud-upload { + font-size: 7em; vertical-align: top; @include margin-right(0.1em); @@ -300,8 +332,8 @@ .video-uploads-header { font-size: 1.5em; - margin-bottom: .25em; - font-weight: 600 + margin-bottom: 0.25em; + font-weight: 600; } .video-max-file-size-text { @@ -335,12 +367,14 @@ font-size: 90%; } - .video-detail-status, .more-details-action { + .video-detail-status, + .more-details-action { @include font-size(12); @include line-height(12); } - .more-details-action, .upload-failure { + .more-details-action, + .upload-failure { display: none; } @@ -393,7 +427,8 @@ background-color: $color-error; } - .more-details-action, .upload-failure { + .more-details-action, + .upload-failure { display: inline-block; color: $color-error; } @@ -458,11 +493,13 @@ width: 17%; } - .thumbnail-col, .video-id-col { + .thumbnail-col, + .video-id-col { width: 15%; } - .date-col, .status-col { + .date-col, + .status-col { width: 10%; } @@ -532,7 +569,8 @@ &:hover, &:focus, &.focused { - img, .video-duration { + img, + .video-duration { @include transition(all 0.3s linear); opacity: 0.1; @@ -592,7 +630,7 @@ position: absolute; text-align: center; top: 50%; - left: 5px;; + left: 5px; right: 5px; @include transform(translateY(-50%));