Merge pull request openedx#2154 from edx-solutions/release-candidate

[v1.62.5] Review sync up of master branch with release-candidate

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)
 

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):

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:

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):

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):