diff --git a/lms/djangoapps/ccx/tests/test_views.py b/lms/djangoapps/ccx/tests/test_views.py index 1839cfd200..6dcda9e291 100644 --- a/lms/djangoapps/ccx/tests/test_views.py +++ b/lms/djangoapps/ccx/tests/test_views.py @@ -80,6 +80,40 @@ def ccx_dummy_request(): return request +def setup_students_and_grades(context): + """ + Create students and set their grades. + :param context: class reference + """ + if context.course: + context.student = student = UserFactory.create() + CourseEnrollmentFactory.create(user=student, course_id=context.course.id) + + context.student2 = student2 = UserFactory.create() + CourseEnrollmentFactory.create(user=student2, course_id=context.course.id) + + # create grades for self.student as if they'd submitted the ccx + for chapter in context.course.get_children(): + for i, section in enumerate(chapter.get_children()): + for j, problem in enumerate(section.get_children()): + # if not problem.visible_to_staff_only: + StudentModuleFactory.create( + grade=1 if i < j else 0, + max_grade=1, + student=context.student, + course_id=context.course.id, + module_state_key=problem.location + ) + + StudentModuleFactory.create( + grade=1 if i > j else 0, + max_grade=1, + student=context.student2, + course_id=context.course.id, + module_state_key=problem.location + ) + + @attr('shard_1') @ddt.ddt class TestCoachDashboard(SharedModuleStoreTestCase, LoginEnrollmentTestCase): @@ -696,28 +730,12 @@ class TestCCXGrades(SharedModuleStoreTestCase, LoginEnrollmentTestCase): # which emulates how a student would get access. self.ccx_key = CCXLocator.from_course_locator(self._course.id, ccx.id) self.course = get_course_by_id(self.ccx_key, depth=None) - - self.student = student = UserFactory.create() - CourseEnrollmentFactory.create(user=student, course_id=self.course.id) - - # create grades for self.student as if they'd submitted the ccx - for chapter in self.course.get_children(): - for i, section in enumerate(chapter.get_children()): - for j, problem in enumerate(section.get_children()): - # if not problem.visible_to_staff_only: - StudentModuleFactory.create( - grade=1 if i < j else 0, - max_grade=1, - student=self.student, - course_id=self.course.id, - module_state_key=problem.location - ) - + setup_students_and_grades(self) self.client.login(username=coach.username, password="test") - self.addCleanup(RequestCache.clear_request_cache) @patch('ccx.views.render_to_response', intercept_renderer) + @patch('instructor.views.gradebook_api.MAX_STUDENTS_PER_PAGE_GRADE_BOOK', 1) def test_gradebook(self): self.course.enable_ccx = True RequestCache.clear_request_cache() @@ -728,6 +746,8 @@ class TestCCXGrades(SharedModuleStoreTestCase, LoginEnrollmentTestCase): ) response = self.client.get(url) self.assertEqual(response.status_code, 200) + # Max number of student per page is one. Patched setting MAX_STUDENTS_PER_PAGE_GRADE_BOOK = 1 + self.assertEqual(len(response.mako_context['students']), 1) # pylint: disable=no-member student_info = response.mako_context['students'][0] # pylint: disable=no-member self.assertEqual(student_info['grade_summary']['percent'], 0.5) self.assertEqual( @@ -751,12 +771,11 @@ class TestCCXGrades(SharedModuleStoreTestCase, LoginEnrollmentTestCase): response['content-disposition'], 'attachment' ) + rows = response.content.strip().split('\r') + headers = rows[0] - headers, row = ( - row.strip().split(',') for row in - response.content.strip().split('\n') - ) - data = dict(zip(headers, row)) + # picking first student records + data = dict(zip(headers.strip().split(','), rows[1].strip().split(','))) self.assertNotIn('HW 04', data) self.assertEqual(data['HW 01'], '0.75') self.assertEqual(data['HW 02'], '0.5') diff --git a/lms/djangoapps/ccx/urls.py b/lms/djangoapps/ccx/urls.py index 9a2be83e7e..f670749087 100644 --- a/lms/djangoapps/ccx/urls.py +++ b/lms/djangoapps/ccx/urls.py @@ -18,8 +18,13 @@ urlpatterns = patterns( 'ccx.views.ccx_schedule', name='ccx_schedule'), url(r'^ccx_manage_student$', 'ccx.views.ccx_student_management', name='ccx_manage_student'), + + # Grade book url(r'^ccx_gradebook$', 'ccx.views.ccx_gradebook', name='ccx_gradebook'), + url(r'^ccx_gradebook/(?P[0-9]+)$', + 'ccx.views.ccx_gradebook', name='ccx_gradebook'), + url(r'^ccx_grades.csv$', 'ccx.views.ccx_grades_csv', name='ccx_grades_csv'), url(r'^ccx_set_grading_policy$', diff --git a/lms/djangoapps/ccx/views.py b/lms/djangoapps/ccx/views.py index cabb554b63..e88fa0b623 100644 --- a/lms/djangoapps/ccx/views.py +++ b/lms/djangoapps/ccx/views.py @@ -39,8 +39,8 @@ from ccx_keys.locator import CCXLocator from student.roles import CourseCcxCoachRole from student.models import CourseEnrollment -from instructor.offline_gradecalc import student_grades from instructor.views.api import _split_input_list +from instructor.views.gradebook_api import get_grade_book_page from instructor.views.tools import get_student_from_identifier from instructor.enrollment import ( enroll_email, @@ -551,24 +551,11 @@ def ccx_gradebook(request, course, ccx=None): ccx_key = CCXLocator.from_course_locator(course.id, ccx.id) with ccx_course(ccx_key) as course: prep_course_for_grading(course, request) - - enrolled_students = User.objects.filter( - courseenrollment__course_id=ccx_key, - courseenrollment__is_active=1 - ).order_by('username').select_related("profile") - - student_info = [ - { - 'username': student.username, - 'id': student.id, - 'email': student.email, - 'grade_summary': student_grades(student, request, course), - 'realname': student.profile.name, - } - for student in enrolled_students - ] + student_info, page = get_grade_book_page(request, course, course_key=ccx_key) return render_to_response('courseware/gradebook.html', { + 'page': page, + 'page_url': reverse('ccx_gradebook', kwargs={'course_id': ccx_key}), 'students': student_info, 'course': course, 'course_id': course.id, diff --git a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py index 4fed6a075e..37ea9c4e7a 100644 --- a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py +++ b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py @@ -8,10 +8,13 @@ from django.conf import settings from django.core.urlresolvers import reverse from django.test.client import RequestFactory from django.test.utils import override_settings +from edxmako.shortcuts import render_to_response +from ccx.tests.test_views import setup_students_and_grades from courseware.tabs import get_course_tab_list from courseware.tests.factories import UserFactory from courseware.tests.helpers import LoginEnrollmentTestCase +from instructor.views.gradebook_api import calculate_page_info from common.test.utils import XssTestMixin from student.tests.factories import AdminFactory @@ -23,6 +26,20 @@ from student.roles import CourseFinanceAdminRole from student.models import CourseEnrollment +def intercept_renderer(path, context): + """ + Intercept calls to `render_to_response` and attach the context dict to the + response for examination in unit tests. + """ + # I think Django already does this for you in their TestClient, except + # we're bypassing that by using edxmako. Probably edxmako should be + # integrated better with Django's rendering and event system. + response = render_to_response(path, context) + response.mako_context = context + response.mako_template = path + return response + + @ddt.ddt class TestInstructorDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase, XssTestMixin): """ @@ -252,3 +269,25 @@ class TestInstructorDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase, XssT """ response = self.client.get(self.url) self.assertIn('D: 0.5, C: 0.57, B: 0.63, A: 0.75', response.content) + + @patch('instructor.views.gradebook_api.MAX_STUDENTS_PER_PAGE_GRADE_BOOK', 2) + def test_calculate_page_info(self): + page = calculate_page_info(offset=0, total_students=2) + self.assertEqual(page["offset"], 0) + self.assertEqual(page["page_num"], 1) + self.assertEqual(page["next_offset"], None) + self.assertEqual(page["previous_offset"], None) + self.assertEqual(page["total_pages"], 1) + + @patch('instructor.views.gradebook_api.render_to_response', intercept_renderer) + @patch('instructor.views.gradebook_api.MAX_STUDENTS_PER_PAGE_GRADE_BOOK', 1) + def test_spoc_gradebook_pages(self): + setup_students_and_grades(self) + url = reverse( + 'spoc_gradebook', + kwargs={'course_id': self.course.id} + ) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + # Max number of student per page is one. Patched setting MAX_STUDENTS_PER_PAGE_GRADE_BOOK = 1 + self.assertEqual(len(response.mako_context['students']), 1) # pylint: disable=no-member diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index d626a45ed0..f394e4166a 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -81,7 +81,6 @@ from instructor.enrollment import ( unenroll_email, ) from instructor.access import list_with_level, allow_access, revoke_access, ROLES, update_forum_role -from instructor.offline_gradecalc import student_grades import instructor_analytics.basic import instructor_analytics.distributions import instructor_analytics.csvs @@ -2625,46 +2624,6 @@ def enable_certificate_generation(request, course_id=None): return redirect(_instructor_dash_url(course_key, section='certificates')) -#---- Gradebook (shown to small courses only) ---- -@cache_control(no_cache=True, no_store=True, must_revalidate=True) -@require_level('staff') -def spoc_gradebook(request, course_id): - """ - Show the gradebook for this course: - - Only shown for courses with enrollment < settings.FEATURES.get("MAX_ENROLLMENT_INSTR_BUTTONS") - - Only displayed to course staff - """ - course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) - course = get_course_with_access(request.user, 'staff', course_key, depth=None) - - enrolled_students = User.objects.filter( - courseenrollment__course_id=course_key, - courseenrollment__is_active=1 - ).order_by('username').select_related("profile") - - # possible extension: implement pagination to show to large courses - - student_info = [ - { - 'username': student.username, - 'id': student.id, - 'email': student.email, - 'grade_summary': student_grades(student, request, course), - 'realname': student.profile.name, - } - for student in enrolled_students - ] - - return render_to_response('courseware/gradebook.html', { - 'students': student_info, - 'course': course, - 'course_id': course_key, - # Checked above - 'staff_access': True, - 'ordered_grades': sorted(course.grade_cutoffs.items(), key=lambda i: i[1], reverse=True), - }) - - @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) @require_level('staff') diff --git a/lms/djangoapps/instructor/views/api_urls.py b/lms/djangoapps/instructor/views/api_urls.py index be99ef4600..700e99639c 100644 --- a/lms/djangoapps/instructor/views/api_urls.py +++ b/lms/djangoapps/instructor/views/api_urls.py @@ -124,7 +124,10 @@ urlpatterns = patterns( # spoc gradebook url(r'^gradebook$', - 'instructor.views.api.spoc_gradebook', name='spoc_gradebook'), + 'instructor.views.gradebook_api.spoc_gradebook', name='spoc_gradebook'), + + url(r'^gradebook/(?P[0-9]+)$', + 'instructor.views.gradebook_api.spoc_gradebook', name='spoc_gradebook'), # Cohort management url(r'add_users_to_cohorts$', diff --git a/lms/djangoapps/instructor/views/gradebook_api.py b/lms/djangoapps/instructor/views/gradebook_api.py new file mode 100644 index 0000000000..2d9e5fd0a2 --- /dev/null +++ b/lms/djangoapps/instructor/views/gradebook_api.py @@ -0,0 +1,120 @@ +""" +Grade book view for instructor and pagination work (for grade book) +which is currently use by ccx and instructor apps. +""" +import math + +from django.contrib.auth.models import User +from django.core.urlresolvers import reverse +from django.views.decorators.cache import cache_control + +from opaque_keys.edx.keys import CourseKey + +from edxmako.shortcuts import render_to_response +from courseware.courses import get_course_with_access +from instructor.offline_gradecalc import student_grades +from instructor.views.api import require_level + + +# Grade book: max students per page +MAX_STUDENTS_PER_PAGE_GRADE_BOOK = 20 + + +def calculate_page_info(offset, total_students): + """ + Takes care of sanitizing the offset of current page also calculates offsets for next and previous page + and information like total number of pages and current page number. + + :param offset: offset for database query + :return: tuple consist of page number, query offset for next and previous pages and valid offset + """ + + # validate offset. + if not (isinstance(offset, int) or offset.isdigit()) or int(offset) < 0 or int(offset) >= total_students: + offset = 0 + else: + offset = int(offset) + + # calculate offsets for next and previous pages. + next_offset = offset + MAX_STUDENTS_PER_PAGE_GRADE_BOOK + previous_offset = offset - MAX_STUDENTS_PER_PAGE_GRADE_BOOK + + # calculate current page number. + page_num = ((offset / MAX_STUDENTS_PER_PAGE_GRADE_BOOK) + 1) + + # calculate total number of pages. + total_pages = int(math.ceil(float(total_students) / MAX_STUDENTS_PER_PAGE_GRADE_BOOK)) or 1 + + if previous_offset < 0 or offset == 0: + # We are at first page, so there's no previous page. + previous_offset = None + + if next_offset >= total_students: + # We've reached the last page, so there's no next page. + next_offset = None + + return { + "previous_offset": previous_offset, + "next_offset": next_offset, + "page_num": page_num, + "offset": offset, + "total_pages": total_pages + } + + +def get_grade_book_page(request, course, course_key): + """ + Get student records per page along with page information i.e current page, total pages and + offset information. + """ + # Unsanitized offset + current_offset = request.GET.get('offset', 0) + enrolled_students = User.objects.filter( + courseenrollment__course_id=course_key, + courseenrollment__is_active=1 + ).order_by('username').select_related("profile") + + total_students = enrolled_students.count() + page = calculate_page_info(current_offset, total_students) + offset = page["offset"] + total_pages = page["total_pages"] + + if total_pages > 1: + # Apply limit on queryset only if total number of students are greater then MAX_STUDENTS_PER_PAGE_GRADE_BOOK. + enrolled_students = enrolled_students[offset: offset + MAX_STUDENTS_PER_PAGE_GRADE_BOOK] + + student_info = [ + { + 'username': student.username, + 'id': student.id, + 'email': student.email, + 'grade_summary': student_grades(student, request, course), + 'realname': student.profile.name, + } + for student in enrolled_students + ] + return student_info, page + + +@cache_control(no_cache=True, no_store=True, must_revalidate=True) +@require_level('staff') +def spoc_gradebook(request, course_id): + """ + Show the gradebook for this course: + - Only shown for courses with enrollment < settings.FEATURES.get("MAX_ENROLLMENT_INSTR_BUTTONS") + - Only displayed to course staff + """ + course_key = CourseKey.from_string(course_id) + course = get_course_with_access(request.user, 'staff', course_key, depth=None) + student_info, page = get_grade_book_page(request, course, course_key) + + return render_to_response('courseware/gradebook.html', { + 'page': page, + 'page_url': reverse('spoc_gradebook', kwargs={'course_id': unicode(course_key)}), + 'students': student_info, + 'course': course, + 'course_id': course_key, + # Checked above + 'staff_access': True, + 'ordered_grades': sorted(course.grade_cutoffs.items(), key=lambda i: i[1], reverse=True), + }) diff --git a/lms/static/sass/course/_gradebook.scss b/lms/static/sass/course/_gradebook.scss index 28bb5a87b4..1085195087 100644 --- a/lms/static/sass/course/_gradebook.scss +++ b/lms/static/sass/course/_gradebook.scss @@ -80,6 +80,16 @@ div.gradebook-wrapper { } } + .grade-book-footer { + position: relative; + top: 15px; + width: 100%; + border: 0; + box-shadow: 0; + text-align: center; + display: inline-block; + } + .grades { position: relative; float: left; diff --git a/lms/templates/courseware/gradebook.html b/lms/templates/courseware/gradebook.html index 2ec8dee0b4..ebc6bac37e 100644 --- a/lms/templates/courseware/gradebook.html +++ b/lms/templates/courseware/gradebook.html @@ -118,7 +118,23 @@ from django.core.urlresolvers import reverse + + %if page["previous_offset"] is not None: + + ${_('previous page')} + + %endif + ${_('Page')} ${page["page_num"]} ${_('of')} ${page["total_pages"]} + + %if page["next_offset"] is not None: + + ${_('next page')} + + %endif + %endif