diff --git a/lms/djangoapps/ccx/tests/test_views.py b/lms/djangoapps/ccx/tests/test_views.py index 0633f4eb6178..44d8c0c83077 100644 --- a/lms/djangoapps/ccx/tests/test_views.py +++ b/lms/djangoapps/ccx/tests/test_views.py @@ -25,6 +25,7 @@ from common.djangoapps.student.models import CourseEnrollment, CourseEnrollmentAllowed from common.djangoapps.student.roles import CourseCcxCoachRole, CourseInstructorRole, CourseStaffRole from common.djangoapps.student.tests.factories import AdminFactory, CourseEnrollmentFactory, UserFactory +from common.djangoapps.util.file import course_filename_prefix_generator from lms.djangoapps.ccx.models import CustomCourseForEdX from lms.djangoapps.ccx.overrides import get_override_for_ccx, override_field_for_ccx from lms.djangoapps.ccx.tests.factories import CcxFactory @@ -36,7 +37,7 @@ from lms.djangoapps.courseware.tests.helpers import LoginEnrollmentTestCase from lms.djangoapps.courseware.testutils import FieldOverrideTestMixin from lms.djangoapps.discussion.django_comment_client.utils import has_forum_access -from lms.djangoapps.grades.api import task_compute_all_grades_for_course +from lms.djangoapps.grades.api import prefetch_course_and_subsection_grades, task_compute_all_grades_for_course from lms.djangoapps.instructor.access import allow_access, list_with_level from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.django_comment_common.models import FORUM_ROLE_ADMINISTRATOR @@ -1050,9 +1051,20 @@ def test_grades_csv(self): ) response = self.client.get(url) assert response.status_code == 200 - # Are the grades downloaded as an attachment? - assert response['content-disposition'] == 'attachment' - rows = response.content.decode('utf-8').strip().split('\r') + # The grades download as an attachment named after the course, matching + # the naming used by the asynchronous course grade report. + expected_prefix = course_filename_prefix_generator(self.ccx_key) + assert re.match( + rf'attachment; filename="{re.escape(expected_prefix)}_grade_report_' + r'\d{4}-\d{2}-\d{2}-\d{4}\.csv"$', + response['content-disposition'] + ) + body = response.content.decode('utf-8') + # Emails and usernames are written as plain text, not as a bytes repr. + assert "b'" not in body + assert self.student.email in body + assert self.student2.username in body + rows = body.strip().split('\r') headers = rows[0] # picking first student records data = dict(list(zip(headers.strip().split(','), rows[1].strip().split(',')))) # noqa: B905 @@ -1062,6 +1074,32 @@ def test_grades_csv(self): assert data['HW 03'] == '0.25' assert data['HW Avg'] == '0.5' + def test_grades_csv_prefetches_persisted_grades(self): + """ + The report bulk-prefetches persisted course/subsection grades once for + the whole class instead of reading them one student at a time (N+1). + """ + self.course.enable_ccx = True + RequestCache.clear_all_namespaces() + + url = reverse( + 'ccx_grades_csv', + kwargs={'course_id': self.ccx_key} + ) + with patch( + 'lms.djangoapps.ccx.views.prefetch_course_and_subsection_grades', + wraps=prefetch_course_and_subsection_grades, + ) as mock_prefetch: + response = self.client.get(url) + + assert response.status_code == 200 + # A single bulk prefetch is issued for the CCX, covering every enrolled student. + mock_prefetch.assert_called_once() + called_course_key, called_users = mock_prefetch.call_args[0] + assert called_course_key == self.ccx_key + prefetched_ids = {user.id for user in called_users} + assert {self.student.id, self.student2.id} <= prefetched_ids + @patch('lms.djangoapps.courseware.views.views.render_to_response', intercept_renderer) def test_student_progress(self): self.course.enable_ccx = True diff --git a/lms/djangoapps/ccx/views.py b/lms/djangoapps/ccx/views.py index 0bd475e9d777..19350311e4c5 100644 --- a/lms/djangoapps/ccx/views.py +++ b/lms/djangoapps/ccx/views.py @@ -26,6 +26,7 @@ from common.djangoapps.edxmako.shortcuts import render_to_response from common.djangoapps.student.models import CourseEnrollment from common.djangoapps.student.roles import CourseCcxCoachRole +from common.djangoapps.util.file import course_filename_prefix_generator from lms.djangoapps.ccx.models import CustomCourseForEdX from lms.djangoapps.ccx.overrides import ( bulk_delete_ccx_override_fields, @@ -47,13 +48,13 @@ parse_date, ) from lms.djangoapps.courseware.field_overrides import disable_overrides -from lms.djangoapps.grades.api import CourseGradeFactory +from lms.djangoapps.grades.api import CourseGradeFactory, prefetch_course_and_subsection_grades from lms.djangoapps.instructor.enrollment import enroll_email, get_email_params from lms.djangoapps.instructor.views.gradebook_api import get_grade_book_page from openedx.core.djangoapps.django_comment_common.models import FORUM_ROLE_ADMINISTRATOR, assign_role from openedx.core.djangoapps.django_comment_common.utils import seed_permissions_roles from openedx.core.lib.courses import get_course_by_id -from xmodule.modulestore.django import SignalHandler # pylint: disable=wrong-import-order +from xmodule.modulestore.django import SignalHandler, modulestore # pylint: disable=wrong-import-order log = logging.getLogger(__name__) TODAY = datetime.datetime.today # for patching in tests @@ -520,34 +521,38 @@ def ccx_grades_csv(request, course, ccx=None): ccx_key = CCXLocator.from_course_locator(course.id, str(ccx.id)) with ccx_course(ccx_key) as course: # pylint: disable=redefined-argument-from-local - enrolled_students = User.objects.filter( + enrolled_students = list(User.objects.filter( courseenrollment__course_id=ccx_key, courseenrollment__is_active=1 - ).order_by('username').select_related("profile") - grades = CourseGradeFactory().iter(enrolled_students, course) + ).order_by('username').select_related("profile")) header = None rows = [] - for student, course_grade, __ in grades: - if course_grade: - # We were able to successfully grade this student for this - # course. - if not header: - # Encode the header row in utf-8 encoding in case there are - # unicode characters - header = [section['label'] for section in course_grade.summary['section_breakdown']] - rows.append(["id", "email", "username", "grade"] + header) - - percents = { - section['label']: section.get('percent', 0.0) - for section in course_grade.summary['section_breakdown'] - if 'label' in section - } - - row_percents = [percents.get(label, 0.0) for label in header] - rows.append([student.id, student.email.encode('utf-8'), - student.username.encode('utf-8'), - course_grade.percent] + row_percents) + # Bulk-read the persisted course and subsection grades for the whole + # class in a single pass instead of reading them once per student, + # mirroring the asynchronous CourseGradeReport. + with modulestore().bulk_operations(ccx_key): + prefetch_course_and_subsection_grades(ccx_key, enrolled_students) + grades = CourseGradeFactory().iter(enrolled_students, course) + + for student, course_grade, __ in grades: + if course_grade: + # We were able to successfully grade this student for this + # course. + if not header: + header = [section['label'] for section in course_grade.summary['section_breakdown']] + rows.append(["id", "email", "username", "grade"] + header) + + percents = { + section['label']: section.get('percent', 0.0) + for section in course_grade.summary['section_breakdown'] + if 'label' in section + } + + row_percents = [percents.get(label, 0.0) for label in header] + rows.append([student.id, student.email, + student.username, + course_grade.percent] + row_percents) buf = StringIO() writer = csv.writer(buf) @@ -555,6 +560,8 @@ def ccx_grades_csv(request, course, ccx=None): writer.writerow(row) response = HttpResponse(buf.getvalue(), content_type='text/csv') - response['Content-Disposition'] = 'attachment' + timestamp = datetime.datetime.now(pytz.UTC).strftime('%Y-%m-%d-%H%M') + report_name = f'{course_filename_prefix_generator(ccx_key)}_grade_report_{timestamp}.csv' + response['Content-Disposition'] = f'attachment; filename="{report_name}"' return response