diff --git a/lms/djangoapps/instructor_analytics/basic.py b/lms/djangoapps/instructor_analytics/basic.py index 237e030ccd..0851f4a6ac 100644 --- a/lms/djangoapps/instructor_analytics/basic.py +++ b/lms/djangoapps/instructor_analytics/basic.py @@ -406,7 +406,7 @@ def coupon_codes_features(features, coupons_list, course_id): return [extract_coupon(coupon, features) for coupon in coupons_list] -def list_problem_responses(course_key, problem_location): +def list_problem_responses(course_key, problem_location, limit_responses=None): """ Return responses to a given problem as a dict. @@ -421,7 +421,10 @@ def list_problem_responses(course_key, problem_location): where `state` represents a student's response to the problem identified by `problem_location`. """ - problem_key = UsageKey.from_string(problem_location) + if isinstance(problem_location, UsageKey): + problem_key = problem_location + else: + problem_key = UsageKey.from_string(problem_location) # Are we dealing with an "old-style" problem location? run = problem_key.run if not run: @@ -434,6 +437,8 @@ def list_problem_responses(course_key, problem_location): module_state_key=problem_key ) smdat = smdat.order_by('student') + if limit_responses is not None: + smdat = smdat[:limit_responses] return [ {'username': response.student.username, 'state': response.state} diff --git a/lms/djangoapps/instructor_task/api.py b/lms/djangoapps/instructor_task/api.py index 99f2b7b4b9..da8c284b97 100644 --- a/lms/djangoapps/instructor_task/api.py +++ b/lms/djangoapps/instructor_task/api.py @@ -330,7 +330,7 @@ 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} + task_input = {'problem_location': problem_location, '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_helper/grades.py b/lms/djangoapps/instructor_task/tasks_helper/grades.py index 42e9e47e22..0d25d71912 100644 --- a/lms/djangoapps/instructor_task/tasks_helper/grades.py +++ b/lms/djangoapps/instructor_task/tasks_helper/grades.py @@ -8,11 +8,16 @@ from datetime import datetime from itertools import chain, izip, izip_longest from time import time +from django.contrib.auth import get_user_model +from django.conf import settings from lazy import lazy +from opaque_keys.edx.keys import UsageKey from pytz import UTC from six import text_type +from course_blocks.api import get_course_blocks from courseware.courses import get_course_by_id +from courseware.user_state_client import DjangoXBlockUserStateClient from instructor_analytics.basic import list_problem_responses from instructor_analytics.csvs import format_dictlist from lms.djangoapps.certificates.models import CertificateWhitelist, GeneratedCertificate, certificate_info_for_user @@ -548,6 +553,99 @@ class ProblemGradeReport(object): class ProblemResponses(object): + + @classmethod + def _build_problem_list(cls, course_blocks, root, path=None): + """ + Generate a tuple of display names, block location paths and block keys + for all problem blocks under the ``root`` block. + + Arguments: + course_blocks (BlockStructureBlockData): Block structure for a course. + root (UsageKey): This block and its children will be used to generate + the problem list + path (List[str]): The list of display names for the parent of root block + + Yields: + Tuple[str, List[str], UsageKey]: tuple of a block's display name, path, and + usage key + """ + display_name = course_blocks.get_xblock_field(root, 'display_name') + if path is None: + path = [display_name] + + yield display_name, path, root + + for block in course_blocks.get_children(root): + display_name = course_blocks.get_xblock_field(block, 'display_name') + for result in cls._build_problem_list(course_blocks, block, path + [display_name]): + yield result + + @classmethod + def _build_student_data(cls, user_id, course_key, usage_key_str): + """ + Generate a list of problem responses for all problem under the + ``problem_location`` root. + + Arguments: + 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. + + Returns: + List[Dict]: Returns a list of dictionaries containing the student + data which will be included in the final csv. + """ + usage_key = UsageKey.from_string(usage_key_str).map_into_course(course_key) + 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') + + store = modulestore() + user_state_client = DjangoXBlockUserStateClient() + + 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 + + block = store.get_item(block_key) + + # 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} + 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) + + student_data += responses + for response in responses: + 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) + if max_count is not None: + max_count -= len(responses) + if max_count <= 0: + break + + return student_data + @classmethod def generate(cls, _xmodule_instance_args, _entry_id, course_id, task_input, action_name): """ @@ -560,11 +658,16 @@ 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') # Compute result table and format it - problem_location = task_input.get('problem_location') - student_data = list_problem_responses(course_id, problem_location) - features = ['username', 'state'] + student_data = 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) task_progress.attempted = task_progress.succeeded = len(rows) diff --git a/lms/djangoapps/instructor_task/tests/test_base.py b/lms/djangoapps/instructor_task/tests/test_base.py index 26236f39f0..1bd6277bd4 100644 --- a/lms/djangoapps/instructor_task/tests/test_base.py +++ b/lms/djangoapps/instructor_task/tests/test_base.py @@ -233,12 +233,12 @@ class InstructorTaskModuleTestCase(InstructorTaskCourseTestCase): factory = OptionResponseXMLFactory() factory_args = self._option_problem_factory_args() problem_xml = factory.build_xml(**factory_args) - ItemFactory.create(parent_location=parent.location, - parent=parent, - category="problem", - display_name=problem_url_name, - data=problem_xml, - **kwargs) + return ItemFactory.create(parent_location=parent.location, + parent=parent, + category="problem", + display_name=problem_url_name, + data=problem_xml, + **kwargs) def redefine_option_problem(self, problem_url_name, correct_answer=OPTION_1, num_inputs=1, num_responses=2): """Change the problem definition so the answer is Option 2""" diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index 992a115fab..be2a0feb47 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -12,27 +12,44 @@ import os import shutil import tempfile import urllib +from contextlib import contextmanager from datetime import datetime, timedelta import ddt import unicodecsv +from capa.tests.response_xml_factory import MultipleChoiceResponseXMLFactory +from course_modes.models import CourseMode +from course_modes.tests.factories import CourseModeFactory +from courseware.tests.factories import InstructorFactory from django.conf import settings from django.core.urlresolvers import reverse from django.test.utils import override_settings from freezegun import freeze_time -from mock import MagicMock, Mock, patch +from instructor_analytics.basic import UNAVAILABLE, list_problem_responses +from mock import MagicMock, Mock, patch, ANY from nose.plugins.attrib import attr from pytz import UTC +from shoppingcart.models import ( + Coupon, + CourseRegistrationCode, + CourseRegistrationCodeInvoiceItem, + Invoice, + InvoiceTransaction, + Order, + PaidCourseRegistration +) from six import text_type +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 +from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, check_mongo_calls +from xmodule.partitions.partitions import Group, UserPartition import openedx.core.djangoapps.user_api.course_tag.api as course_tag_api -from capa.tests.response_xml_factory import MultipleChoiceResponseXMLFactory from lms.djangoapps.certificates.models import CertificateStatuses, GeneratedCertificate from lms.djangoapps.certificates.tests.factories import CertificateWhitelistFactory, GeneratedCertificateFactory -from course_modes.models import CourseMode -from course_modes.tests.factories import CourseModeFactory -from courseware.tests.factories import InstructorFactory -from instructor_analytics.basic import UNAVAILABLE from lms.djangoapps.grades.models import PersistentCourseGrade from lms.djangoapps.grades.transformer import GradesTransformer from lms.djangoapps.instructor_task.tasks_helper.certs import generate_students_certificates @@ -64,26 +81,9 @@ from lms.djangoapps.verify_student.tests.factories import SoftwareSecurePhotoVer from openedx.core.djangoapps.course_groups.models import CohortMembership, CourseUserGroupPartitionGroup from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory from openedx.core.djangoapps.credit.tests.factories import CreditCourseFactory +from openedx.core.djangoapps.request_cache.middleware import RequestCache from openedx.core.djangoapps.user_api.partition_schemes import RandomUserPartitionScheme from openedx.core.djangoapps.util.testing import ContentGroupTestCase, TestConditionalContent -from openedx.core.djangoapps.request_cache.middleware import RequestCache -from shoppingcart.models import ( - Coupon, - CourseRegistrationCode, - CourseRegistrationCodeInvoiceItem, - Invoice, - InvoiceTransaction, - Order, - PaidCourseRegistration -) -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 -from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase -from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, check_mongo_calls -from xmodule.partitions.partitions import Group, UserPartition - from ..models import ReportStore from ..tasks_helper.utils import UPDATE_STATUS_FAILED, UPDATE_STATUS_SUCCEEDED @@ -462,25 +462,163 @@ class TestTeamGradeReport(InstructorGradeReportTestCase): self._verify_cell_data_for_user(self.student2.username, self.course.id, 'Team Name', team2.name) -class TestProblemResponsesReport(TestReportMixin, InstructorTaskCourseTestCase): +# pylint: disable=protected-access +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.course = CourseFactory.create() + self.initialize_course() + self.instructor = self.create_instructor('instructor') + self.student = self.create_student('student') + + @contextmanager + def _remove_capa_report_generator(self): + """ + Temporarily removes the generate_report_data method so we can test + report generation when it's absent. + """ + from xmodule.capa_module import CapaDescriptor + generate_report_data = CapaDescriptor.generate_report_data + del CapaDescriptor.generate_report_data + yield + CapaDescriptor.generate_report_data = generate_report_data + + @patch.dict('django.conf.settings.FEATURES', {'MAX_PROBLEM_RESPONSES_COUNT': 4}) + def test_build_student_data_limit(self): + """ + Ensure that the _build_student_data method respects the global setting for + maximum responses to return in a report. + """ + self.define_option_problem(u'Problem1') + for ctr in range(5): + student = self.create_student('student{}'.format(ctr)) + self.submit_student_answer(student.username, u'Problem1', ['Option 1']) + + student_data = ProblemResponses._build_student_data( + user_id=self.instructor.id, + course_key=self.course.id, + usage_key_str=str(self.course.location), + ) + + self.assertEquals(len(student_data), 4) + + @patch( + 'lms.djangoapps.instructor_task.tasks_helper.grades.list_problem_responses', + wraps=list_problem_responses + ) + def test_build_student_data_for_block_without_generate_report_data(self, mock_list_problem_responses): + """ + Ensure that building student data for a block the doesn't have the + ``generate_report_data`` method works as expected. + """ + 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( + user_id=self.instructor.id, + course_key=self.course.id, + usage_key_str=str(problem.location), + ) + self.assertEquals(len(student_data), 1) + self.assertDictContainsSubset({ + 'username': 'student', + 'location': 'Problem1', + 'block_key': 'i4x://edx/1.23x/problem/Problem1', + 'title': 'Problem1', + }, student_data[0]) + self.assertIn('state', student_data[0]) + mock_list_problem_responses.assert_called_with(self.course.id, ANY, ANY) + + @patch('xmodule.capa_module.CapaDescriptor.generate_report_data', create=True) + def test_build_student_data_for_block_with_mock_generate_report_data(self, mock_generate_report_data): + """ + Ensure that building student data for a block that supports the + ``generate_report_data`` method works as expected. + """ + self.define_option_problem(u'Problem1') + state = {'some': 'state'} + mock_generate_report_data.return_value = iter([ + ('student', state), + ]) + student_data = ProblemResponses._build_student_data( + user_id=self.instructor.id, + course_key=self.course.id, + usage_key_str=str(self.course.location), + ) + self.assertEquals(len(student_data), 1) + self.assertDictContainsSubset({ + 'username': 'student', + 'location': 'test_course > Section > Subsection > Problem1', + 'block_key': 'i4x://edx/1.23x/problem/Problem1', + 'title': 'Problem1', + 'state': state, + }, student_data[0]) + + def test_build_student_data_for_block_with_real_generate_report_data(self): + """ + Ensure that building student data for a block that supports the + ``generate_report_data`` method works as expected. + """ + self.define_option_problem(u'Problem1') + self.submit_student_answer(self.student.username, u'Problem1', ['Option 1']) + student_data = ProblemResponses._build_student_data( + user_id=self.instructor.id, + course_key=self.course.id, + usage_key_str=str(self.course.location), + ) + self.assertEquals(len(student_data), 1) + self.assertDictContainsSubset({ + 'username': 'student', + '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' + }, + }, student_data[0]) + + @patch('lms.djangoapps.instructor_task.tasks_helper.grades.list_problem_responses') + @patch('xmodule.capa_module.CapaDescriptor.generate_report_data', create=True) + def test_build_student_data_for_block_with_generate_report_data_not_implemented( + self, + mock_generate_report_data, + mock_list_problem_responses, + ): + """ + Ensure that if ``generate_report_data`` raises a NotImplementedError, + the report falls back to the alternative method. + """ + problem = self.define_option_problem(u'Problem1') + mock_generate_report_data.side_effect = NotImplementedError + ProblemResponses._build_student_data( + user_id=self.instructor.id, + course_key=self.course.id, + usage_key_str=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': ''} + task_input = { + 'problem_location': str(self.course.location), + 'user_id': self.instructor.id + } with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task'): - with patch('lms.djangoapps.instructor_task.tasks_helper.grades.list_problem_responses') as patched_data_source: - patched_data_source.return_value = [ + 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'}, ] - result = ProblemResponses.generate(None, None, self.course.id, task_input, 'calculated') + result = ProblemResponses.generate( + None, None, self.course.id, task_input, 'calculated' + ) report_store = ReportStore.from_config(config_name='GRADES_DOWNLOAD') links = report_store.links_for(self.course.id) diff --git a/lms/envs/common.py b/lms/envs/common.py index 7dd42d762c..f456bb7ca8 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -206,6 +206,9 @@ FEATURES = { # Automatically approve student identity verification attempts 'AUTOMATIC_VERIFY_STUDENT_IDENTITY_FOR_TESTING': False, + # Maximum number of rows to include in the csv file for downloading problem responses. + 'MAX_PROBLEM_RESPONSES_COUNT': 5000, + # whether to use password policy enforcement or not 'ENFORCE_PASSWORD_POLICY': True,