Merge pull request #18512: Include original state in learner response report

2018-07-09 16:14:24 -07:00
parent fabbdabc5a 4d48a59f3f
commit f1d026b172
7 changed files with 201 additions and 67 deletions
--- 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
                ))
--- 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%));
--- 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.
--- 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(
+            """
+            <problem>
+                <choiceresponse>
+                    <checkboxgroup label="Select the correct synonym of paranoid?">
+                        <choice correct="true">over-suspicious</choice>
+                        <choice correct="false">funny</choice>
+                    </checkboxgroup>
+                </choiceresponse>
+                <multiplechoiceresponse>
+                    <choicegroup type="MultipleChoice">
+                        <choice correct="true">The iPad</choice>
+                        <choice correct="true">Napster</choice>
+                        <choice correct="false">The iPod</choice>
+                    </choicegroup>
+                </multiplechoiceresponse>
+                <optionresponse>
+                    <optioninput options="('yellow','blue','green')" correct="blue" label="Color_1"/>
+                </optionresponse>
+            </problem>
+            """
+        )
+        self.assertEquals(problem.find_correct_answer_text(answer_id), answer_text)
+
    def test_find_answer_text_textinput(self):
        problem = new_loncapa_problem(
            """
--- 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')
--- 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
--- 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,12 @@ 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',
+            'Correct Answer': u'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 +632,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 +1631,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):
        """