diff --git a/lms/djangoapps/courseware/user_state_client.py b/lms/djangoapps/courseware/user_state_client.py index 467f4e3d493e..cb163d5ec310 100644 --- a/lms/djangoapps/courseware/user_state_client.py +++ b/lms/djangoapps/courseware/user_state_client.py @@ -395,7 +395,7 @@ def get_history(self, username, block_key, scope=Scope.user_state): yield XBlockUserState(username, block_key, state, history_entry.created, scope) - def iter_all_for_block(self, block_key, batch_no=None, batch_size=None, scope=Scope.user_state): + def iter_all_for_block(self, block_key, scope=Scope.user_state): """ Return an iterator over the data stored in the block (e.g. a problem block). @@ -413,9 +413,9 @@ def iter_all_for_block(self, block_key, batch_no=None, batch_size=None, scope=Sc if scope != Scope.user_state: raise ValueError("Only Scope.user_state is supported") - results = StudentModule.objects.filter(module_state_key=block_key).order_by('student') - if batch_no: - results = results[(batch_no - 1) * batch_size:batch_no * batch_size] + results = StudentModule.objects.filter( + module_state_key=block_key + ).select_related('student').order_by('student') p = Paginator(results, settings.USER_STATE_BATCH_SIZE) diff --git a/lms/djangoapps/instructor_analytics/basic.py b/lms/djangoapps/instructor_analytics/basic.py index dc87bec41b57..e65e7bef53a4 100644 --- a/lms/djangoapps/instructor_analytics/basic.py +++ b/lms/djangoapps/instructor_analytics/basic.py @@ -14,6 +14,7 @@ from django.contrib.auth.models import User from django.core.exceptions import ObjectDoesNotExist from django.core.serializers.json import DjangoJSONEncoder +from django.core.paginator import Paginator from django.db.models import Count, Q from django.urls import reverse from edx_proctoring.api import get_exam_violation_report @@ -433,7 +434,7 @@ def extract_coupon(coupon, features): return [extract_coupon(coupon, features) for coupon in coupons_list] -def list_problem_responses(course_key, problem_location, limit_responses=None, batch_no=None, batch_size=None): +def list_problem_responses(course_key, problem_location, limit_responses=None): """ Return responses to a given problem as a dict. @@ -460,20 +461,19 @@ def list_problem_responses(course_key, problem_location, limit_responses=None, b return [] smdat = StudentModule.objects.filter( - course_id=course_key, module_state_key=problem_key - ) - smdat = smdat.order_by('student') + ).select_related('student').order_by('student') if limit_responses: smdat = smdat[:limit_responses] - if batch_no: - smdat = smdat[(batch_no - 1) * batch_size:batch_no * batch_size] - return [ - {'username': response.student.username, 'state': get_response_state(response)} - for response in smdat - ] + p = Paginator(smdat, settings.USER_STATE_BATCH_SIZE) + + for page_number in p.page_range: + page = p.page(page_number) + + for response in page.object_list: + yield {'username': response.student.username, 'state': get_response_state(response)} def get_response_state(response): diff --git a/lms/djangoapps/instructor_analytics/tests/test_basic.py b/lms/djangoapps/instructor_analytics/tests/test_basic.py index ef1d8d2b861b..78e0ccbbc7ea 100644 --- a/lms/djangoapps/instructor_analytics/tests/test_basic.py +++ b/lms/djangoapps/instructor_analytics/tests/test_basic.py @@ -122,13 +122,15 @@ def result_factory(result_id): patched_manager.filter.return_value = mock_results mock_problem_location = '' - problem_responses = list_problem_responses(self.course_key, problem_location=mock_problem_location) + problem_responses = list( + list_problem_responses(self.course_key, problem_location=mock_problem_location) + ) # Check if list_problem_responses called UsageKey.from_string to look up problem key: patched_from_string.assert_called_once_with(mock_problem_location) # Check if list_problem_responses called StudentModule.objects.filter to obtain relevant records: patched_manager.filter.assert_called_once_with( - course_id=self.course_key, module_state_key=mock_problem_key + module_state_key=mock_problem_key ) # Check if list_problem_responses returned expected results: diff --git a/lms/djangoapps/instructor_task/tasks_helper/grades.py b/lms/djangoapps/instructor_task/tasks_helper/grades.py index 34a3155965c4..8ebb7884ea31 100644 --- a/lms/djangoapps/instructor_task/tasks_helper/grades.py +++ b/lms/djangoapps/instructor_task/tasks_helper/grades.py @@ -896,83 +896,74 @@ def _build_student_data( student_data_keys = set() - batch_size = 10000 batch_no = 1 - while True: - student_data = [] - with store.bulk_operations(course_key): - 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 - - if filter_types is not None and block_key.block_type not in filter_types: - continue - - block = store.get_item(block_key) - generated_report_data = defaultdict(list) - - # 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, batch_no, - batch_size) - for username, state in block.generate_report_data(user_state_iterator, max_count): - generated_report_data[username].append(state) - except NotImplementedError: - pass - - responses = [] - - for response in list_problem_responses(course_key, block_key, max_count, batch_no, batch_size): - 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(user_state.keys()) - responses.append(user_response) - else: - responses.append(response) - - 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 - # 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'] - ) + with store.bulk_operations(course_key): + 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 + + if filter_types is not None and block_key.block_type not in filter_types: + continue + + block = store.get_item(block_key) + generated_report_data = defaultdict(list) + + # 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 + + 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(user_state.keys()) + responses.append(user_response) + else: + responses.append(response) + + # 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'] + ) - yield student_data, student_data_keys_list, batch_no + yield responses, student_data_keys_list, batch_no - if not student_data: - break - batch_no += 1 + batch_no += 1 + if max_count is not None: + max_count -= len(responses) + if max_count <= 0: + break @classmethod def generate(cls, _xmodule_instance_args, _entry_id, course_id, task_input, action_name): diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index fe48aa4a7030..af556978bca1 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -560,7 +560,7 @@ def test_build_student_data_for_block_without_generate_report_data(self, mock_li 'title': 'Problem1', }, student_data[0]) self.assertIn('state', student_data[0]) - mock_list_problem_responses.assert_called_with(self.course.id, ANY, ANY, ANY, ANY) + mock_list_problem_responses.assert_called_with(self.course.id, ANY, ANY) @patch('xmodule.capa_module.ProblemBlock.generate_report_data', create=True) def test_build_student_data_for_block_with_mock_generate_report_data(self, mock_generate_report_data):