From e0aa8cf78a1dc342ed4e00082768fb0a400a585f Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Thu, 14 Nov 2013 16:08:53 -0500 Subject: [PATCH 1/5] Grade report celery task and direct file push to S3 from the new instructor dashboard. Hook up display of grade files ready for download to new instructor dashboard. LMS-58 --- lms/djangoapps/courseware/grades.py | 67 ++++++- lms/djangoapps/instructor/views/api.py | 35 ++++ lms/djangoapps/instructor/views/api_urls.py | 6 + .../instructor/views/instructor_dashboard.py | 2 + lms/djangoapps/instructor_task/api.py | 14 +- lms/djangoapps/instructor_task/models.py | 180 ++++++++++++++++++ lms/djangoapps/instructor_task/tasks.py | 12 ++ .../instructor_task/tasks_helper.py | 109 ++++++++++- lms/djangoapps/instructor_task/views.py | 1 - lms/envs/aws.py | 18 +- lms/envs/common.py | 17 +- lms/envs/dev.py | 7 + lms/envs/test.py | 7 + .../instructor_dashboard/data_download.coffee | 74 ++++++- .../instructor_dashboard_2/data_download.html | 17 +- 15 files changed, 548 insertions(+), 18 deletions(-) diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index 9b702da4a2..93c0d5bf47 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -1,20 +1,26 @@ # Compute grades using real division, with no integer truncation from __future__ import division - +from collections import defaultdict import random import logging from collections import defaultdict from django.conf import settings from django.contrib.auth.models import User +from django.db import transaction +from django.core.handlers.base import BaseHandler +from django.test.client import RequestFactory +from dogapi import dog_stats_api + +from courseware import courses from courseware.model_data import FieldDataCache, DjangoKeyValueStore from xblock.fields import Scope -from .module_render import get_module, get_module_for_descriptor from xmodule import graders from xmodule.capa_module import CapaModule from xmodule.graders import Score from .models import StudentModule +from .module_render import get_module, get_module_for_descriptor log = logging.getLogger("mitx.courseware") @@ -411,3 +417,60 @@ def get_score(course_id, user, problem_descriptor, module_creator, field_data_ca total = weight return (correct, total) + + +@contextmanager +def manual_transaction(): + """A context manager for managing manual transactions""" + try: + yield + except Exception: + transaction.rollback() + log.exception('Due to an error, this transaction has been rolled back') + raise + else: + transaction.commit() + +def iterate_grades_for(course_id, students): + """Given a course_id and an iterable of students (User), yield a tuple of: + + (student, gradeset, err_msg) for every student enrolled in the course. + + If an error occured, gradeset will be an empty dict and err_msg will be an + exception message. If there was no error, err_msg is an empty string. + + The gradeset is a dictionary with the following fields: + + - grade : A final letter grade. + - percent : The final percent for the class (rounded up). + - section_breakdown : A breakdown of each section that makes + up the grade. (For display) + - grade_breakdown : A breakdown of the major components that + make up the final grade. (For display) + - raw_scores contains scores for every graded module + """ + course = courses.get_course_by_id(course_id) + + # We make a fake request because grading code expects to be able to look at + # the request. We have to attach the correct user to the request before + # grading that student. + request = RequestFactory().get('/') + + for student in students: + with dog_stats_api.timer('lms.grades.iterate_grades_for', tags=['action:{}'.format(course_id)]): + try: + request.user = student + gradeset = grade(student, request, course) + yield student, gradeset, "" + except Exception as exc: + # Keep marching on even if this student couldn't be graded for + # some reason. + log.exception( + 'Cannot grade student %s (%s) in course %s because of exception: %s', + student.username, + student.id, + course_id, + exc.message + ) + yield student, {}, exc.message + diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 5596ba9c41..cac8ebf981 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -32,6 +32,7 @@ from student.models import unique_id_for_user import instructor_task.api from instructor_task.api_helper import AlreadyRunningError from instructor_task.views import get_task_completion_info +from instructor_task.models import GradesStore import instructor.enrollment as enrollment from instructor.enrollment import enroll_email, unenroll_email, get_email_params from instructor.views.tools import strip_if_string, get_student_from_identifier @@ -750,6 +751,40 @@ def list_instructor_tasks(request, course_id): return JsonResponse(response_payload) +@ensure_csrf_cookie +@cache_control(no_cache=True, no_store=True, must_revalidate=True) +@require_level('staff') +def list_grade_downloads(_request, course_id): + """ + List grade CSV files that are available for download for this course. + """ + grades_store = GradesStore.from_config() + + response_payload = { + 'downloads' : [ + dict(name=name, url=url, link='{}'.format(url, name)) + for name, url in grades_store.links_for(course_id) + ] + } + return JsonResponse(response_payload) + + +@ensure_csrf_cookie +@cache_control(no_cache=True, no_store=True, must_revalidate=True) +@require_level('staff') +def calculate_grades_csv(request, course_id): + """ + AlreadyRunningError is raised if the course's grades are already being updated. + """ + try: + instructor_task.api.submit_calculate_grades_csv(request, course_id) + return JsonResponse({"status" : "Grade calculation started"}) + except AlreadyRunningError: + return JsonResponse({ + "status" : "Grade calculation already running" + }) + + @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 3f05f1cc70..f016b9820f 100644 --- a/lms/djangoapps/instructor/views/api_urls.py +++ b/lms/djangoapps/instructor/views/api_urls.py @@ -37,4 +37,10 @@ urlpatterns = patterns('', # nopep8 'instructor.views.api.proxy_legacy_analytics', name="proxy_legacy_analytics"), url(r'^send_email$', 'instructor.views.api.send_email', name="send_email"), + + # Grade downloads... + url(r'^list_grade_downloads$', + 'instructor.views.api.list_grade_downloads', name="list_grade_downloads"), + url(r'calculate_grades_csv$', + 'instructor.views.api.calculate_grades_csv', name="calculate_grades_csv"), ) diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index 7c3fd32a0a..c1fa3e67bd 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -171,6 +171,8 @@ def _section_data_download(course_id, access): 'get_students_features_url': reverse('get_students_features', kwargs={'course_id': course_id}), 'get_anon_ids_url': reverse('get_anon_ids', kwargs={'course_id': course_id}), 'list_instructor_tasks_url': reverse('list_instructor_tasks', kwargs={'course_id': course_id}), + 'list_grade_downloads_url' : reverse('list_grade_downloads', kwargs={'course_id' : course_id}), + 'calculate_grades_csv_url' : reverse('calculate_grades_csv', kwargs={'course_id' : course_id}), } return section_data diff --git a/lms/djangoapps/instructor_task/api.py b/lms/djangoapps/instructor_task/api.py index 7521a8eb3a..23d85ebf95 100644 --- a/lms/djangoapps/instructor_task/api.py +++ b/lms/djangoapps/instructor_task/api.py @@ -16,7 +16,8 @@ from instructor_task.models import InstructorTask from instructor_task.tasks import (rescore_problem, reset_problem_attempts, delete_problem_state, - send_bulk_course_email) + send_bulk_course_email, + calculate_grades_csv) from instructor_task.api_helper import (check_arguments_for_rescoring, encode_problem_and_student_input, @@ -206,3 +207,14 @@ def submit_bulk_course_email(request, course_id, email_id): # create the key value by using MD5 hash: task_key = hashlib.md5(task_key_stub).hexdigest() return submit_task(request, task_type, task_class, course_id, task_input, task_key) + +def submit_calculate_grades_csv(request, course_id): + """ + AlreadyRunningError is raised if the course's grades are already being updated. + """ + task_type = 'grade_course' + task_class = calculate_grades_csv + task_input = {} + task_key = "" + + return submit_task(request, task_type, task_class, course_id, task_input, task_key) diff --git a/lms/djangoapps/instructor_task/models.py b/lms/djangoapps/instructor_task/models.py index 8d6376fae3..2acf78cbaa 100644 --- a/lms/djangoapps/instructor_task/models.py +++ b/lms/djangoapps/instructor_task/models.py @@ -12,9 +12,20 @@ file and check it in at the same time as your model changes. To do that, ASSUMPTIONS: modules have unique IDs, even across different module_types """ +from cStringIO import StringIO +from gzip import GzipFile from uuid import uuid4 +import csv import json +import hashlib +import os +import os.path +import urllib +from boto.s3.connection import S3Connection +from boto.s3.key import Key + +from django.conf import settings from django.contrib.auth.models import User from django.db import models, transaction @@ -176,3 +187,172 @@ class InstructorTask(models.Model): def create_output_for_revoked(): """Creates standard message to store in output format for revoked tasks.""" return json.dumps({'message': 'Task revoked before running'}) + + +class GradesStore(object): + """ + Simple abstraction layer that can fetch and store CSV files for grades + download. Should probably refactor later to create a GradesFile object that + can simply be appended to for the sake of memory efficiency, rather than + passing in the whole dataset. Doing that for now just because it's simpler. + """ + @classmethod + def from_config(cls): + """ + Return one of the GradesStore subclasses depending on django + configuration. Look at subclasses for expected configuration. + """ + storage_type = settings.GRADES_DOWNLOAD.get("STORAGE_TYPE") + if storage_type.lower() == "s3": + return S3GradesStore.from_config() + elif storage_type.lower() == "localfs": + return LocalFSGradesStore.from_config() + + +class S3GradesStore(GradesStore): + """ + + """ + def __init__(self, bucket_name, root_path): + self.root_path = root_path + + conn = S3Connection( + settings.AWS_ACCESS_KEY_ID, + settings.AWS_SECRET_ACCESS_KEY + ) + self.bucket = conn.get_bucket(bucket_name) + + @classmethod + def from_config(cls): + return cls( + settings.GRADES_DOWNLOAD['BUCKET'], + settings.GRADES_DOWNLOAD['ROOT_PATH'] + ) + + def key_for(self, course_id, filename): + """Return the key we would use to store and retrive the data for the + given filename.""" + hashed_course_id = hashlib.sha1(course_id) + + key = Key(self.bucket) + key.key = "{}/{}/{}".format( + self.root_path, + hashed_course_id.hexdigest(), + filename + ) + + return key + + def store(self, course_id, filename, buff): + key = self.key_for(course_id, filename) + + data = buff.getvalue() + key.size = len(data) + key.content_encoding = "gzip" + key.content_type = "text/csv" + + key.set_contents_from_string( + data, + headers={ + "Content-Encoding" : "gzip", + "Content-Length" : len(data), + "Content-Type" : "text/csv", + } + ) + + def store_rows(self, course_id, filename, rows): + """ + Given a course_id, filename, and rows (each row is an iterable of strings), + write this data out. + """ + output_buffer = StringIO() + gzip_file = GzipFile(fileobj=output_buffer, mode="wb") + csv.writer(gzip_file).writerows(rows) + gzip_file.close() + + self.store(course_id, filename, output_buffer) + + def links_for(self, course_id): + """ + For a given `course_id`, return a list of `(filename, url)` tuples. `url` + can be plugged straight into an href + """ + course_dir = self.key_for(course_id, '') + return sorted( + [ + (key.key.split("/")[-1], key.generate_url(expires_in=300)) + for key in self.bucket.list(prefix=course_dir.key) + ], + reverse=True + ) + + +class LocalFSGradesStore(GradesStore): + """ + LocalFS implementation of a GradesStore. This is meant for debugging + purposes and is *absolutely not for production use*. Use S3GradesStore for + that. + """ + def __init__(self, root_path): + """ + Initialize with root_path where we're going to store our files. We + will build a directory structure under this for each course. + """ + self.root_path = root_path + if not os.path.exists(root_path): + os.makedirs(root_path) + + @classmethod + def from_config(cls): + """ + Generate an instance of this object from Django settings. It assumes + that there is a dict in settings named GRADES_DOWNLOAD and that it has + a ROOT_PATH that maps to an absolute file path that the web app has + write permissions to. `LocalFSGradesStore` will create any intermediate + directories as needed. + """ + return cls(settings.GRADES_DOWNLOAD['ROOT_PATH']) + + def path_to(self, course_id, filename): + """Return the full path to a given file for a given course.""" + return os.path.join(self.root_path, urllib.quote(course_id, safe=''), filename) + + def store(self, course_id, filename, buff): + """ + Given the `course_id` and `filename`, store the contents of `buff` in + that file. Overwrite anything that was there previously. `buff` is + assumed to be a StringIO objecd (or anything that can flush its contents + to string using `.getvalue()`). + """ + full_path = self.path_to(course_id, filename) + directory = os.path.dirname(full_path) + if not os.path.exists(directory): + os.mkdir(directory) + + with open(full_path, "wb") as f: + f.write(buff.getvalue()) + + def store_rows(self, course_id, filename, rows): + """ + Given a course_id, filename, and rows (each row is an iterable of strings), + write this data out. + """ + output_buffer = StringIO() + csv.writer(output_buffer).writerows(rows) + self.store(course_id, filename, output_buffer) + + def links_for(self, course_id): + """ + For a given `course_id`, return a list of `(filename, url)` tuples. `url` + can be plugged straight into an href + """ + course_dir = self.path_to(course_id, '') + if not os.path.exists(course_dir): + return [] + return sorted( + [ + (filename, ("file://" + urllib.quote(os.path.join(course_dir, filename)))) + for filename in os.listdir(course_dir) + ], + reverse=True + ) \ No newline at end of file diff --git a/lms/djangoapps/instructor_task/tasks.py b/lms/djangoapps/instructor_task/tasks.py index f30ffe3af2..2c8ad51e83 100644 --- a/lms/djangoapps/instructor_task/tasks.py +++ b/lms/djangoapps/instructor_task/tasks.py @@ -19,6 +19,7 @@ a problem URL and optionally a student. These are used to set up the initial va of the query for traversing StudentModule objects. """ +from django.conf import settings from django.utils.translation import ugettext_noop from celery import task from functools import partial @@ -29,6 +30,7 @@ from instructor_task.tasks_helper import ( rescore_problem_module_state, reset_attempts_module_state, delete_problem_module_state, + push_grades_to_s3, ) from bulk_email.tasks import perform_delegate_email_batches @@ -127,3 +129,13 @@ def send_bulk_course_email(entry_id, _xmodule_instance_args): action_name = ugettext_noop('emailed') visit_fcn = perform_delegate_email_batches return run_main_task(entry_id, visit_fcn, action_name) + + +@task(base=BaseInstructorTask, routing_key=settings.GRADES_DOWNLOAD_ROUTING_KEY) # pylint: disable=E1102 +def calculate_grades_csv(entry_id, xmodule_instance_args): + """ + Grade a course and push the results to an S3 bucket for download. + """ + action_name = ugettext_noop('graded') + task_fn = partial(push_grades_to_s3, xmodule_instance_args) + return run_main_task(entry_id, task_fn, action_name) \ No newline at end of file diff --git a/lms/djangoapps/instructor_task/tasks_helper.py b/lms/djangoapps/instructor_task/tasks_helper.py index cf828edb5b..c64f71b5f2 100644 --- a/lms/djangoapps/instructor_task/tasks_helper.py +++ b/lms/djangoapps/instructor_task/tasks_helper.py @@ -4,24 +4,26 @@ running state of a course. """ import json +import urllib +from datetime import datetime from time import time from celery import Task, current_task from celery.utils.log import get_task_logger from celery.states import SUCCESS, FAILURE - from django.contrib.auth.models import User from django.db import transaction, reset_queries from dogapi import dog_stats_api +from pytz import UTC from xmodule.modulestore.django import modulestore - from track.views import task_track +from courseware.grades import iterate_grades_for from courseware.models import StudentModule from courseware.model_data import FieldDataCache from courseware.module_render import get_module_for_descriptor_internal -from instructor_task.models import InstructorTask, PROGRESS +from instructor_task.models import GradesStore, InstructorTask, PROGRESS # define different loggers for use within tasks and on client side TASK_LOG = get_task_logger(__name__) @@ -465,3 +467,104 @@ def delete_problem_module_state(xmodule_instance_args, _module_descriptor, stude track_function = _get_track_function_for_task(student_module.student, xmodule_instance_args) track_function('problem_delete_state', {}) return UPDATE_STATUS_SUCCEEDED + + +def push_grades_to_s3(_xmodule_instance_args, _entry_id, course_id, _task_input, action_name): + """ + For a given `course_id`, generate a grades CSV file for all students that + are enrolled, and store using a `GradesStore`. Once created, the files can + be accessed by instantiating another `GradesStore` (via + `GradesStore.from_config()`) and calling `link_for()` on it. Writes are + buffered, so we'll never write part of a CSV file to S3 -- i.e. any files + that are visible in GradesStore will be complete ones. + """ + # Get start time for task: + start_time = datetime.now(UTC) + status_interval = 100 + + # The pre-fetching of groups is done to make auth checks not require an + # additional DB lookup (this kills the Progress page in particular). + # But when doing grading at this scale, the memory required to store the resulting + # enrolled_students is too large to fit comfortably in memory, and subsequent + # course grading requests lead to memory fragmentation. So we will err here on the + # side of smaller memory allocations at the cost of additional lookups. + enrolled_students = User.objects.filter( + courseenrollment__course_id=course_id, + courseenrollment__is_active=True + ) + + # perform the main loop + num_attempted = 0 + num_succeeded = 0 + num_failed = 0 + num_total = enrolled_students.count() + curr_step = "Calculating Grades" + + def update_task_progress(): + """Return a dict containing info about current task""" + current_time = datetime.now(UTC) + progress = { + 'action_name': action_name, + 'attempted': num_attempted, + 'succeeded': num_succeeded, + 'failed' : num_failed, + 'total' : num_total, + 'duration_ms': int((current_time - start_time).total_seconds() * 1000), + 'step' : curr_step, + } + _get_current_task().update_state(state=PROGRESS, meta=progress) + + return progress + + # Loop over all our students and build a + header = None + rows = [] + err_rows = [["id", "username", "error_msg"]] + for student, gradeset, err_msg in iterate_grades_for(course_id, enrolled_students): + # Periodically update task status (this is a db write) + if num_attempted % status_interval == 0: + update_task_progress() + num_attempted += 1 + + if gradeset: + num_succeeded += 1 + if not header: + header = [section['label'] for section in gradeset[u'section_breakdown']] + rows.append(["id", "email", "username", "grade"] + header) + + percents = { + section['label']: section.get('percent', 0.0) + for section in gradeset[u'section_breakdown'] + if 'label' in section + } + row_percents = [percents[label] for label in header] + rows.append([student.id, student.email, student.username, gradeset['percent']] + row_percents) + else: + # An empty gradeset means we failed to grade a student. + num_failed += 1 + err_rows.append([student.id, student.username, err_msg]) + + curr_step = "Uploading CSVs" + update_task_progress() + + grades_store = GradesStore.from_config() + timestamp_str = start_time.strftime("%Y-%m-%d-%H%M") + + TASK_LOG.debug("Uploading CSV files for course {}".format(course_id)) + + course_id_prefix = urllib.quote(course_id.replace("/", "_")) + grades_store.store_rows( + course_id, + "{}_grade_report_{}.csv".format(course_id_prefix, timestamp_str), + rows + ) + # If there are any error rows (don't count the header), write that out as well + if len(err_rows) > 1: + grades_store.store_rows( + course_id, + "{}_grade_report_{}_err.csv".format(course_id_prefix, timestamp_str), + err_rows + ) + + # One last update before we close out... + return update_task_progress() diff --git a/lms/djangoapps/instructor_task/views.py b/lms/djangoapps/instructor_task/views.py index 9a23841425..a8e7eade0b 100644 --- a/lms/djangoapps/instructor_task/views.py +++ b/lms/djangoapps/instructor_task/views.py @@ -75,7 +75,6 @@ def instructor_task_status(request): 'traceback': optional, returned if task failed and produced a traceback. """ - output = {} if 'task_id' in request.REQUEST: task_id = request.REQUEST['task_id'] diff --git a/lms/envs/aws.py b/lms/envs/aws.py index 4e39ea89d3..e9eebb8781 100644 --- a/lms/envs/aws.py +++ b/lms/envs/aws.py @@ -86,6 +86,7 @@ CELERY_DEFAULT_EXCHANGE = 'edx.{0}core'.format(QUEUE_VARIANT) HIGH_PRIORITY_QUEUE = 'edx.{0}core.high'.format(QUEUE_VARIANT) DEFAULT_PRIORITY_QUEUE = 'edx.{0}core.default'.format(QUEUE_VARIANT) LOW_PRIORITY_QUEUE = 'edx.{0}core.low'.format(QUEUE_VARIANT) +HIGH_MEM_QUEUE = 'edx.{0}core.high_mem'.format(QUEUE_VARIANT) CELERY_DEFAULT_QUEUE = DEFAULT_PRIORITY_QUEUE CELERY_DEFAULT_ROUTING_KEY = DEFAULT_PRIORITY_QUEUE @@ -93,9 +94,19 @@ CELERY_DEFAULT_ROUTING_KEY = DEFAULT_PRIORITY_QUEUE CELERY_QUEUES = { HIGH_PRIORITY_QUEUE: {}, LOW_PRIORITY_QUEUE: {}, - DEFAULT_PRIORITY_QUEUE: {} + DEFAULT_PRIORITY_QUEUE: {}, + HIGH_MEM_QUEUE: {}, } +# If we're a worker on the high_mem queue, set ourselves to die after processing +# one request to avoid having memory leaks take down the worker server. This env +# var is set in /etc/init/edx-workers.conf -- this should probably be replaced +# with some celery API call to see what queue we started listening to, but I +# don't know what that call is or if it's active at this point in the code. +if os.environ.get('QUEUE') == 'high_mem': + CELERYD_MAX_TASKS_PER_CHILD = 1 + + ########################## NON-SECURE ENV CONFIG ############################## # Things like server locations, ports, etc. @@ -312,3 +323,8 @@ TRACKING_BACKENDS.update(AUTH_TOKENS.get("TRACKING_BACKENDS", {})) # Student identity verification settings VERIFY_STUDENT = AUTH_TOKENS.get("VERIFY_STUDENT", VERIFY_STUDENT) + +# Grades download +GRADES_DOWNLOAD_ROUTING_KEY = HIGH_MEM_QUEUE + +GRADES_DOWNLOAD = ENV_TOKENS.get("GRADES_DOWNLOAD", GRADES_DOWNLOAD) diff --git a/lms/envs/common.py b/lms/envs/common.py index c698ce24be..73de4c00fc 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -192,6 +192,10 @@ MITX_FEATURES = { # Disable instructor dash buttons for downloading course data # when enrollment exceeds this number 'MAX_ENROLLMENT_INSTR_BUTTONS': 200, + + # Grade calculation started from the new instructor dashboard will write + # grades CSV files to S3 and give links for downloads. + 'ENABLE_S3_GRADE_DOWNLOADS' : True, } # Used for A/B testing @@ -846,6 +850,7 @@ CELERY_DEFAULT_EXCHANGE_TYPE = 'direct' HIGH_PRIORITY_QUEUE = 'edx.core.high' DEFAULT_PRIORITY_QUEUE = 'edx.core.default' LOW_PRIORITY_QUEUE = 'edx.core.low' +HIGH_MEM_QUEUE = 'edx.core.high_mem' CELERY_QUEUE_HA_POLICY = 'all' @@ -857,7 +862,8 @@ CELERY_DEFAULT_ROUTING_KEY = DEFAULT_PRIORITY_QUEUE CELERY_QUEUES = { HIGH_PRIORITY_QUEUE: {}, LOW_PRIORITY_QUEUE: {}, - DEFAULT_PRIORITY_QUEUE: {} + DEFAULT_PRIORITY_QUEUE: {}, + HIGH_MEM_QUEUE: {}, } # let logging work as configured: @@ -1061,3 +1067,12 @@ REGISTRATION_OPTIONAL_FIELDS = set([ 'mailing_address', 'goals', ]) + +# Grades download +GRADES_DOWNLOAD_ROUTING_KEY = HIGH_MEM_QUEUE + +GRADES_DOWNLOAD = { + 'STORAGE_TYPE' : 'localfs', + 'BUCKET' : 'edx-grades', + 'ROOT_PATH' : '/tmp/edx-s3/grades', +} diff --git a/lms/envs/dev.py b/lms/envs/dev.py index 1fe7fc330c..86e214263d 100644 --- a/lms/envs/dev.py +++ b/lms/envs/dev.py @@ -283,6 +283,13 @@ EDX_API_KEY = None ####################### Shoppingcart ########################### MITX_FEATURES['ENABLE_SHOPPING_CART'] = True +###################### Grade Downloads ###################### +GRADES_DOWNLOAD = { + 'STORAGE_TYPE' : 'localfs', + 'BUCKET' : 'edx-grades', + 'ROOT_PATH' : '/tmp/edx-s3/grades', +} + ##################################################################### # Lastly, see if the developer has any local overrides. try: diff --git a/lms/envs/test.py b/lms/envs/test.py index 49f8d29aa2..4a56b41c09 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -247,6 +247,13 @@ PASSWORD_HASHERS = ( # 'django.contrib.auth.hashers.CryptPasswordHasher', ) +###################### Grade Downloads ###################### +GRADES_DOWNLOAD = { + 'STORAGE_TYPE' : 'localfs', + 'BUCKET' : 'edx-grades', + 'ROOT_PATH' : '/tmp/edx-s3/grades', +} + ################### Make tests quieter # OpenID spews messages like this to stderr, we don't need to see them: diff --git a/lms/static/coffee/src/instructor_dashboard/data_download.coffee b/lms/static/coffee/src/instructor_dashboard/data_download.coffee index e6108b1055..e39b2f60c7 100644 --- a/lms/static/coffee/src/instructor_dashboard/data_download.coffee +++ b/lms/static/coffee/src/instructor_dashboard/data_download.coffee @@ -21,9 +21,12 @@ class DataDownload @$display_text = @$display.find '.data-display-text' @$display_table = @$display.find '.data-display-table' @$request_response_error = @$display.find '.request-response-error' + @$list_studs_btn = @$section.find("input[name='list-profiles']'") @$list_anon_btn = @$section.find("input[name='list-anon-ids']'") @$grade_config_btn = @$section.find("input[name='dump-gradeconf']'") + + @grade_downloads = new GradeDownloads(@$section) @instructor_tasks = new (PendingInstructorTasks()) @$section # attach click handlers @@ -84,10 +87,14 @@ class DataDownload @$display_text.html data['grading_config_summary'] # handler for when the section title is clicked. - onClickTitle: -> @instructor_tasks.task_poller.start() + onClickTitle: -> + @instructor_tasks.task_poller.start() + @grade_downloads.downloads_poller.start() # handler for when the section is closed - onExit: -> @instructor_tasks.task_poller.stop() + onExit: -> + @instructor_tasks.task_poller.stop() + @grade_downloads.downloads_poller.stop() clear_display: -> @$display_text.empty() @@ -95,6 +102,69 @@ class DataDownload @$request_response_error.empty() +class GradeDownloads + ### Grade Downloads -- links expire quickly, so we refresh every 5 mins #### + constructor: (@$section) -> + @$grade_downloads_table = @$section.find ".grade-downloads-table" + @$calculate_grades_csv_btn = @$section.find("input[name='calculate-grades-csv']'") + + @$display = @$section.find '.data-display' + @$display_text = @$display.find '.data-display-text' + @$request_response_error = @$display.find '.request-response-error' + + POLL_INTERVAL = 1000 * 60 * 5 # 5 minutes in ms + @downloads_poller = new window.InstructorDashboard.util.IntervalManager( + POLL_INTERVAL, => @reload_grade_downloads() + ) + + @$calculate_grades_csv_btn.click (e) => + url = @$calculate_grades_csv_btn.data 'endpoint' + $.ajax + dataType: 'json' + url: url + error: std_ajax_err => + @$request_response_error.text "Error generating grades." + success: (data) => + @$display_text.html data['status'] + + reload_grade_downloads: -> + endpoint = @$grade_downloads_table.data 'endpoint' + $.ajax + dataType: 'json' + url: endpoint + success: (data) => + if data.downloads.length + @create_grade_downloads_table data.downloads + else + console.log "No grade CSVs ready for download" + error: std_ajax_err => console.error "Error finding grade download CSVs" + + create_grade_downloads_table: (grade_downloads_data) -> + @$grade_downloads_table.empty() + + options = + enableCellNavigation: true + enableColumnReorder: false + autoHeight: true + forceFitColumns: true + + columns = [ + id: 'link' + field: 'link' + name: 'File' + sortable: false, + minWidth: 200, + formatter: (row, cell, value, columnDef, dataContext) -> + '' + dataContext['name'] + '' + ] + + $table_placeholder = $ '
', class: 'slickgrid' + @$grade_downloads_table.append $table_placeholder + grid = new Slick.Grid($table_placeholder, grade_downloads_data, columns, options) + + + + # export for use # create parent namespaces if they do not already exist. _.defaults window, InstructorDashboard: {} diff --git a/lms/templates/instructor/instructor_dashboard_2/data_download.html b/lms/templates/instructor/instructor_dashboard_2/data_download.html index b57fd7b30e..7eb55fabfe 100644 --- a/lms/templates/instructor/instructor_dashboard_2/data_download.html +++ b/lms/templates/instructor/instructor_dashboard_2/data_download.html @@ -7,11 +7,6 @@
-## -## -##
-## -##
@@ -20,15 +15,23 @@
+%if settings.MITX_FEATURES.get('ENABLE_S3_GRADE_DOWNLOADS'): +
+

${_("Grades")}

+ +
+

${_("Available grades downloads:")}

+
+
+%endif + %if settings.MITX_FEATURES.get('ENABLE_INSTRUCTOR_BACKGROUND_TASKS'):

${_("Pending Instructor Tasks")}

${_("The status for any active tasks appears in a table below.")}


-
- %endif
From e2423386cbdf984967d290e0657c7059ba7ae4a2 Mon Sep 17 00:00:00 2001 From: Sarina Canelake Date: Wed, 20 Nov 2013 11:05:10 -0500 Subject: [PATCH 2/5] UX for Data Download tab on instructor dash Restrict grade report generation to 'is_superuser' users (can be overridden with feature flag ALLOW_COURSE_STAFF_GRADE_DOWNLOADS); all staff users can download generated files. LMS-58 --- lms/djangoapps/instructor/views/api.py | 6 +- lms/envs/common.py | 3 + .../instructor_dashboard/data_download.coffee | 91 ++++++++++++------- .../src/instructor_dashboard/util.coffee | 2 +- .../sass/course/instructor/_instructor_2.scss | 24 ++++- .../instructor_dashboard_2/data_download.html | 50 +++++++--- .../instructor_dashboard_2/student_admin.html | 12 +-- 7 files changed, 127 insertions(+), 61 deletions(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index cac8ebf981..d4180eef0e 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -778,10 +778,12 @@ def calculate_grades_csv(request, course_id): """ try: instructor_task.api.submit_calculate_grades_csv(request, course_id) - return JsonResponse({"status" : "Grade calculation started"}) + success_status = _("Your grade report is being generated! You can view the status of the generation task in the 'Pending Instructor Tasks' section.") + return JsonResponse({"status": success_status}) except AlreadyRunningError: + already_running_status = _("A grade report generation task is already in progress. Check the 'Pending Instructor Tasks' table for the status of the task. When completed, the report will be available for download in the table below.") return JsonResponse({ - "status" : "Grade calculation already running" + "status": already_running_status }) diff --git a/lms/envs/common.py b/lms/envs/common.py index 73de4c00fc..a8575b89b6 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -196,6 +196,9 @@ MITX_FEATURES = { # Grade calculation started from the new instructor dashboard will write # grades CSV files to S3 and give links for downloads. 'ENABLE_S3_GRADE_DOWNLOADS' : True, + # Give course staff unrestricted access to grade downloads (if set to False, + # only edX superusers can perform the downloads) + 'ALLOW_COURSE_STAFF_GRADE_DOWNLOADS' : False, } # Used for A/B testing diff --git a/lms/static/coffee/src/instructor_dashboard/data_download.coffee b/lms/static/coffee/src/instructor_dashboard/data_download.coffee index e39b2f60c7..22bda4151d 100644 --- a/lms/static/coffee/src/instructor_dashboard/data_download.coffee +++ b/lms/static/coffee/src/instructor_dashboard/data_download.coffee @@ -17,17 +17,23 @@ class DataDownload # this object to call event handlers like 'onClickTitle' @$section.data 'wrapper', @ # gather elements - @$display = @$section.find '.data-display' - @$display_text = @$display.find '.data-display-text' - @$display_table = @$display.find '.data-display-table' - @$request_response_error = @$display.find '.request-response-error' - @$list_studs_btn = @$section.find("input[name='list-profiles']'") @$list_anon_btn = @$section.find("input[name='list-anon-ids']'") @$grade_config_btn = @$section.find("input[name='dump-gradeconf']'") + @$calculate_grades_csv_btn = @$section.find("input[name='calculate-grades-csv']'") + + # response areas + @$download = @$section.find '.data-download-container' + @$download_display_text = @$download.find '.data-display-text' + @$download_display_table = @$download.find '.data-display-table' + @$download_request_response_error = @$download.find '.request-response-error' + @$grades = @$section.find '.grades-download-container' + @$grades_request_response = @$grades.find '.request-response' + @$grades_request_response_error = @$grades.find '.request-response-error' @grade_downloads = new GradeDownloads(@$section) @instructor_tasks = new (PendingInstructorTasks()) @$section + @clear_display() # attach click handlers # The list-anon case is always CSV @@ -46,8 +52,9 @@ class DataDownload url += '/csv' location.href = url else + # Dynamically generate slickgrid table for displaying student profile information @clear_display() - @$display_table.text 'Loading...' + @$download_display_table.text gettext('Loading...') # fetch user list $.ajax @@ -55,7 +62,7 @@ class DataDownload url: url error: std_ajax_err => @clear_display() - @$request_response_error.text "Error getting student list." + @$download_request_response_error.text gettext("Error getting student list.") success: (data) => @clear_display() @@ -64,12 +71,13 @@ class DataDownload enableCellNavigation: true enableColumnReorder: false forceFitColumns: true + rowHeight: 35 columns = ({id: feature, field: feature, name: feature} for feature in data.queried_features) grid_data = data.students $table_placeholder = $ '
', class: 'slickgrid' - @$display_table.append $table_placeholder + @$download_display_table.append $table_placeholder grid = new Slick.Grid($table_placeholder, grid_data, columns, options) # grid.autosizeColumns() @@ -81,13 +89,31 @@ class DataDownload url: url error: std_ajax_err => @clear_display() - @$request_response_error.text "Error getting grading configuration." + @$download_request_response_error.text gettext("Error retrieving grading configuration.") success: (data) => @clear_display() - @$display_text.html data['grading_config_summary'] + @$download_display_text.html data['grading_config_summary'] + + @$calculate_grades_csv_btn.click (e) => + # Clear any CSS styling from the request-response areas + #$(".msg-confirm").css({"display":"none"}) + #$(".msg-error").css({"display":"none"}) + @clear_display() + url = @$calculate_grades_csv_btn.data 'endpoint' + $.ajax + dataType: 'json' + url: url + error: std_ajax_err => + @$grades_request_response_error.text gettext("Error generating grades. Please try again.") + $(".msg-error").css({"display":"block"}) + success: (data) => + @$grades_request_response.text data['status'] + $(".msg-confirm").css({"display":"block"}) # handler for when the section title is clicked. onClickTitle: -> + # Clear display of anything that was here before + @clear_display() @instructor_tasks.task_poller.start() @grade_downloads.downloads_poller.start() @@ -97,36 +123,32 @@ class DataDownload @grade_downloads.downloads_poller.stop() clear_display: -> - @$display_text.empty() - @$display_table.empty() - @$request_response_error.empty() + # Clear any generated tables, warning messages, etc. + @$download_display_text.empty() + @$download_display_table.empty() + @$download_request_response_error.empty() + @$grades_request_response.empty() + @$grades_request_response_error.empty() + # Clear any CSS styling from the request-response areas + $(".msg-confirm").css({"display":"none"}) + $(".msg-error").css({"display":"none"}) class GradeDownloads ### Grade Downloads -- links expire quickly, so we refresh every 5 mins #### constructor: (@$section) -> - @$grade_downloads_table = @$section.find ".grade-downloads-table" - @$calculate_grades_csv_btn = @$section.find("input[name='calculate-grades-csv']'") - @$display = @$section.find '.data-display' - @$display_text = @$display.find '.data-display-text' - @$request_response_error = @$display.find '.request-response-error' + + @$grades = @$section.find '.grades-download-container' + @$grades_request_response = @$grades.find '.request-response' + @$grades_request_response_error = @$grades.find '.request-response-error' + @$grade_downloads_table = @$grades.find ".grade-downloads-table" POLL_INTERVAL = 1000 * 60 * 5 # 5 minutes in ms @downloads_poller = new window.InstructorDashboard.util.IntervalManager( POLL_INTERVAL, => @reload_grade_downloads() ) - @$calculate_grades_csv_btn.click (e) => - url = @$calculate_grades_csv_btn.data 'endpoint' - $.ajax - dataType: 'json' - url: url - error: std_ajax_err => - @$request_response_error.text "Error generating grades." - success: (data) => - @$display_text.html data['status'] - reload_grade_downloads: -> endpoint = @$grade_downloads_table.data 'endpoint' $.ajax @@ -145,15 +167,17 @@ class GradeDownloads options = enableCellNavigation: true enableColumnReorder: false - autoHeight: true + rowHeight: 30 forceFitColumns: true columns = [ id: 'link' field: 'link' - name: 'File' - sortable: false, - minWidth: 200, + name: gettext('File Name (Newest First)') + toolTip: gettext("Links are generated on demand and expire within 5 minutes due to the sensitive nature of student grade information.") + sortable: false + minWidth: 150 + cssClass: "file-download-link" formatter: (row, cell, value, columnDef, dataContext) -> '' + dataContext['name'] + '' ] @@ -161,8 +185,7 @@ class GradeDownloads $table_placeholder = $ '
', class: 'slickgrid' @$grade_downloads_table.append $table_placeholder grid = new Slick.Grid($table_placeholder, grade_downloads_data, columns, options) - - + grid.autosizeColumns() # export for use diff --git a/lms/static/coffee/src/instructor_dashboard/util.coffee b/lms/static/coffee/src/instructor_dashboard/util.coffee index af5b99f419..14a2f2b8ca 100644 --- a/lms/static/coffee/src/instructor_dashboard/util.coffee +++ b/lms/static/coffee/src/instructor_dashboard/util.coffee @@ -43,7 +43,7 @@ create_task_list_table = ($table_tasks, tasks_data) -> id: 'task_type' field: 'task_type' name: 'Task Type' - minWidth: 100 + minWidth: 102 , id: 'task_input' field: 'task_input' diff --git a/lms/static/sass/course/instructor/_instructor_2.scss b/lms/static/sass/course/instructor/_instructor_2.scss index d1a04ce185..2f8eb8947e 100644 --- a/lms/static/sass/course/instructor/_instructor_2.scss +++ b/lms/static/sass/course/instructor/_instructor_2.scss @@ -26,6 +26,13 @@ @include font-size(16); } + .file-download-link a { + font-size: 15px; + color: $link-color; + text-decoration: underline; + padding: 5px; + } + // system feedback - messages .msg { border-radius: 1px; @@ -117,7 +124,7 @@ section.instructor-dashboard-content-2 { .slickgrid { margin-left: 1px; color:#333333; - font-size:11px; + font-size:12px; font-family: verdana,arial,sans-serif; .slick-header-column { @@ -428,13 +435,26 @@ section.instructor-dashboard-content-2 { line-height: 1.3em; } - .data-display { + .data-download-container { .data-display-table { .slickgrid { height: 400px; } } } + + .grades-download-container { + .grade-downloads-table { + .slickgrid { + height: 300px; + padding: 5px; + } + // Disable horizontal scroll bar when grid only has 1 column. Remove this CSS class when more columns added. + .slick-viewport { + overflow-x: hidden !important; + } + } + } } diff --git a/lms/templates/instructor/instructor_dashboard_2/data_download.html b/lms/templates/instructor/instructor_dashboard_2/data_download.html index 7eb55fabfe..cbbc2a871b 100644 --- a/lms/templates/instructor/instructor_dashboard_2/data_download.html +++ b/lms/templates/instructor/instructor_dashboard_2/data_download.html @@ -2,25 +2,46 @@ <%page args="section_data"/> -

${_("Data Download")}

+
+

${_("Data Download")}

+
- - -
- - +

${_("The following button displays a list of all students enrolled in this course, along with profile information such as email address and username. The data can also be downloaded as a CSV file.")}

-
-
+

+

-
+ +
+

${_("Displays the grading configuration for the course. The grading configuration is the breakdown of graded subsections of the course (such as exams and problem sets), and can be changed on the 'Grading' page (under 'Settings') in Studio.")}

+

+ +
+
+ +

${_("Download a CSV of anonymized student IDs by clicking this button.")}

+

+
%if settings.MITX_FEATURES.get('ENABLE_S3_GRADE_DOWNLOADS'): -
-

${_("Grades")}

- -
-

${_("Available grades downloads:")}

+
+
+

${_("Grade Reports")}

+ + %if settings.MITX_FEATURES.get('ALLOW_COURSE_STAFF_GRADE_DOWNLOADS') or section_data['access']['admin']: +

${_("The following button will generate a CSV grade report for all currently enrolled students. For large courses, generating this report may take a few hours.")}

+ +

${_("The report is generated in the background, meaning it is OK to navigate away from this page while your report is generating. Generated reports appear in a table below and can be downloaded.")}

+ +
+
+
+ +

+ %endif + +

${_("Reports Available for Download")}

+

${_("File links are generated on demand and expire within 5 minutes due to the sensitive nature of student grade information. Please note that the report filename contains a timestamp that represents when your file was generated; this timestamp is UTC, not your local timezone.")}


%endif @@ -34,4 +55,3 @@
%endif -
diff --git a/lms/templates/instructor/instructor_dashboard_2/student_admin.html b/lms/templates/instructor/instructor_dashboard_2/student_admin.html index 4d60810008..2762ba4899 100644 --- a/lms/templates/instructor/instructor_dashboard_2/student_admin.html +++ b/lms/templates/instructor/instructor_dashboard_2/student_admin.html @@ -5,7 +5,6 @@

${_("Student-specific grade inspection")}

- ${_("Specify the {platform_name} email address or username of a student here:").format(platform_name=settings.PLATFORM_NAME)}

@@ -26,7 +25,6 @@

${_("Student-specific grade adjustment")}

- ${_("Specify the {platform_name} email address or username of a student here:").format(platform_name=settings.PLATFORM_NAME)}

@@ -60,18 +58,18 @@

%if section_data['access']['instructor']:

${_('You may also delete the entire state of a student for the specified problem:')}

- +

%endif

%if settings.MITX_FEATURES.get('ENABLE_INSTRUCTOR_BACKGROUND_TASKS') and section_data['access']['instructor']:

- ${_("Rescoring runs in the background, and status for active tasks will appear in a table on the Course Info tab. " + ${_("Rescoring runs in the background, and status for active tasks will appear in the 'Pending Instructor Tasks' table. " "To see status for all tasks submitted for this problem and student, click on this button:")}

- +

%endif
@@ -101,10 +99,10 @@

- ${_("These actions run in the background, and status for active tasks will appear in a table on the Course Info tab. " + ${_("The above actions run in the background, and status for active tasks will appear in a table on the Course Info tab. " "To see status for all tasks submitted for this problem, click on this button")}:

- +

From a99fd08004c3dc64466c90d05e1ea9af0b45edda Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 20 Nov 2013 15:46:49 -0500 Subject: [PATCH 3/5] Fix error case where we have items in our grading csv output that are not present in a given student's gradeset. General code cleanup and addition of comments. Instructor dashboard API unit tests. LMS-58 --- common/djangoapps/student/models.py | 10 ++- lms/djangoapps/courseware/grades.py | 15 ++-- lms/djangoapps/instructor/tests/test_api.py | 49 ++++++++++++- lms/djangoapps/instructor/views/api.py | 2 +- .../instructor/views/instructor_dashboard.py | 4 +- lms/djangoapps/instructor_task/api.py | 1 + lms/djangoapps/instructor_task/models.py | 68 ++++++++++++++++--- lms/djangoapps/instructor_task/tasks.py | 2 +- .../instructor_task/tasks_helper.py | 53 ++++++++------- lms/envs/common.py | 12 ++-- lms/envs/dev.py | 7 -- lms/envs/test.py | 7 -- 12 files changed, 163 insertions(+), 67 deletions(-) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index d7a918b569..6774632634 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -769,7 +769,7 @@ class CourseEnrollment(models.Model): activation_changed = True mode_changed = False - # if mode is None, the call to update_enrollment didn't specify a new + # if mode is None, the call to update_enrollment didn't specify a new # mode, so leave as-is if self.mode != mode and mode is not None: self.mode = mode @@ -967,6 +967,14 @@ class CourseEnrollment(models.Model): def enrollments_for_user(cls, user): return CourseEnrollment.objects.filter(user=user, is_active=1) + @classmethod + def users_enrolled_in(cls, course_id): + """Return a queryset of User for every user enrolled in the course.""" + return User.objects.filter( + courseenrollment__course_id=course_id, + courseenrollment__is_active=True + ) + def activate(self): """Makes this `CourseEnrollment` record active. Saves immediately.""" self.update_enrollment(is_active=True) diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index 93c0d5bf47..438466c0ea 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -8,13 +8,12 @@ from collections import defaultdict from django.conf import settings from django.contrib.auth.models import User from django.db import transaction -from django.core.handlers.base import BaseHandler from django.test.client import RequestFactory from dogapi import dog_stats_api from courseware import courses -from courseware.model_data import FieldDataCache, DjangoKeyValueStore +from courseware.model_data import FieldDataCache from xblock.fields import Scope from xmodule import graders from xmodule.capa_module import CapaModule @@ -431,6 +430,7 @@ def manual_transaction(): else: transaction.commit() + def iterate_grades_for(course_id, students): """Given a course_id and an iterable of students (User), yield a tuple of: @@ -447,7 +447,7 @@ def iterate_grades_for(course_id, students): up the grade. (For display) - grade_breakdown : A breakdown of the major components that make up the final grade. (For display) - - raw_scores contains scores for every graded module + - raw_scores: contains scores for every graded module """ course = courses.get_course_by_id(course_id) @@ -460,11 +460,16 @@ def iterate_grades_for(course_id, students): with dog_stats_api.timer('lms.grades.iterate_grades_for', tags=['action:{}'.format(course_id)]): try: request.user = student + # Grading calls problem rendering, which calls masquerading, + # which checks session vars -- thus the empty session dict below. + # It's not pretty, but untangling that is currently beyond the + # scope of this feature. + request.session = {} gradeset = grade(student, request, course) yield student, gradeset, "" - except Exception as exc: + except Exception as exc: # pylint: disable=broad-except # Keep marching on even if this student couldn't be graded for - # some reason. + # some reason, but log it for future reference. log.exception( 'Cannot grade student %s (%s) in course %s because of exception: %s', student.username, diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 6744031c6f..016b5bc39c 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -29,7 +29,6 @@ from courseware.models import StudentModule # modules which are mocked in test cases. import instructor_task.api - from instructor.access import allow_access import instructor.views.api from instructor.views.api import _split_input_list, _msk_from_problem_urlname, common_exceptions_400 @@ -128,6 +127,8 @@ class TestInstructorAPIDenyLevels(ModuleStoreTestCase, LoginEnrollmentTestCase): ('send_email', {'send_to': 'staff', 'subject': 'test', 'message': 'asdf'}), ('list_instructor_tasks', {}), ('list_background_email_tasks', {}), + ('list_grade_downloads', {}), + ('calculate_grades_csv', {}), ] # Endpoints that only Instructors can access self.instructor_level_endpoints = [ @@ -790,6 +791,50 @@ class TestInstructorAPILevelsDataDump(ModuleStoreTestCase, LoginEnrollmentTestCa self.assertTrue(body.startswith('"User ID","Anonymized user ID"\n"2","42"\n')) self.assertTrue(body.endswith('"7","42"\n')) + def test_list_grade_downloads(self): + url = reverse('list_grade_downloads', kwargs={'course_id': self.course.id}) + with patch('instructor_task.models.LocalFSGradesStore.links_for') as mock_links_for: + mock_links_for.return_value = [ + ('mock_file_name_1', 'https://1.mock.url'), + ('mock_file_name_2', 'https://2.mock.url'), + ] + response = self.client.get(url, {}) + + expected_response = { + "downloads": [ + { + "url": "https://1.mock.url", + "link": "mock_file_name_1", + "name": "mock_file_name_1" + }, + { + "url": "https://2.mock.url", + "link": "mock_file_name_2", + "name": "mock_file_name_2" + } + ] + } + res_json = json.loads(response.content) + self.assertEqual(res_json, expected_response) + + def test_calculate_grades_csv_success(self): + url = reverse('calculate_grades_csv', kwargs={'course_id': self.course.id}) + + with patch('instructor_task.api.submit_calculate_grades_csv') as mock_cal_grades: + mock_cal_grades.return_value = True + response = self.client.get(url, {}) + success_status = "Your grade report is being generated! You can view the status of the generation task in the 'Pending Instructor Tasks' section." + self.assertIn(success_status, response.content) + + def test_calculate_grades_csv_already_running(self): + url = reverse('calculate_grades_csv', kwargs={'course_id': self.course.id}) + + with patch('instructor_task.api.submit_calculate_grades_csv') as mock_cal_grades: + mock_cal_grades.side_effect = AlreadyRunningError() + response = self.client.get(url, {}) + already_running_status = "A grade report generation task is already in progress. Check the 'Pending Instructor Tasks' table for the status of the task. When completed, the report will be available for download in the table below." + self.assertIn(already_running_status, response.content) + def test_get_students_features_csv(self): """ Test that some minimum of information is formatted @@ -802,7 +847,7 @@ class TestInstructorAPILevelsDataDump(ModuleStoreTestCase, LoginEnrollmentTestCa def test_get_distribution_no_feature(self): """ Test that get_distribution lists available features - when supplied no feature quparameter. + when supplied no feature parameter. """ url = reverse('get_distribution', kwargs={'course_id': self.course.id}) response = self.client.get(url) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index d4180eef0e..215f775d77 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -761,7 +761,7 @@ def list_grade_downloads(_request, course_id): grades_store = GradesStore.from_config() response_payload = { - 'downloads' : [ + 'downloads': [ dict(name=name, url=url, link='{}'.format(url, name)) for name, url in grades_store.links_for(course_id) ] diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index c1fa3e67bd..de14df2940 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -171,8 +171,8 @@ def _section_data_download(course_id, access): 'get_students_features_url': reverse('get_students_features', kwargs={'course_id': course_id}), 'get_anon_ids_url': reverse('get_anon_ids', kwargs={'course_id': course_id}), 'list_instructor_tasks_url': reverse('list_instructor_tasks', kwargs={'course_id': course_id}), - 'list_grade_downloads_url' : reverse('list_grade_downloads', kwargs={'course_id' : course_id}), - 'calculate_grades_csv_url' : reverse('calculate_grades_csv', kwargs={'course_id' : course_id}), + 'list_grade_downloads_url': reverse('list_grade_downloads', kwargs={'course_id': course_id}), + 'calculate_grades_csv_url': reverse('calculate_grades_csv', kwargs={'course_id': course_id}), } return section_data diff --git a/lms/djangoapps/instructor_task/api.py b/lms/djangoapps/instructor_task/api.py index 23d85ebf95..73af3e4378 100644 --- a/lms/djangoapps/instructor_task/api.py +++ b/lms/djangoapps/instructor_task/api.py @@ -208,6 +208,7 @@ def submit_bulk_course_email(request, course_id, email_id): task_key = hashlib.md5(task_key_stub).hexdigest() return submit_task(request, task_type, task_class, course_id, task_input, task_key) + def submit_calculate_grades_csv(request, course_id): """ AlreadyRunningError is raised if the course's grades are already being updated. diff --git a/lms/djangoapps/instructor_task/models.py b/lms/djangoapps/instructor_task/models.py index 2acf78cbaa..8df6a95c71 100644 --- a/lms/djangoapps/instructor_task/models.py +++ b/lms/djangoapps/instructor_task/models.py @@ -211,7 +211,15 @@ class GradesStore(object): class S3GradesStore(GradesStore): """ + Grades store backed by S3. The directory structure we use to store things + is:: + `{bucket}/{root_path}/{sha1 hash of course_id}/filename` + + We might later use subdirectories or metadata to do more intelligent + grouping and querying, but right now it simply depends on its own + conventions on where files are stored to know what to display. Clients using + this class can name the final file whatever they want. """ def __init__(self, bucket_name, root_path): self.root_path = root_path @@ -224,13 +232,26 @@ class S3GradesStore(GradesStore): @classmethod def from_config(cls): + """ + The expected configuration for an `S3GradesStore` is to have a + `GRADES_DOWNLOAD` dict in settings with the following fields:: + + STORAGE_TYPE : "s3" + BUCKET : Your bucket name, e.g. "grades-bucket" + ROOT_PATH : The path you want to store all course files under. Do not + use a leading or trailing slash. e.g. "staging" or + "staging/2013", not "/staging", or "/staging/" + + Since S3 access relies on boto, you must also define `AWS_ACCESS_KEY_ID` + and `AWS_SECRET_ACCESS_KEY` in settings. + """ return cls( settings.GRADES_DOWNLOAD['BUCKET'], settings.GRADES_DOWNLOAD['ROOT_PATH'] ) def key_for(self, course_id, filename): - """Return the key we would use to store and retrive the data for the + """Return the S3 key we would use to store and retrive the data for the given filename.""" hashed_course_id = hashlib.sha1(course_id) @@ -244,6 +265,16 @@ class S3GradesStore(GradesStore): return key def store(self, course_id, filename, buff): + """ + Store the contents of `buff` in a directory determined by hashing + `course_id`, and name the file `filename`. `buff` is typically a + `StringIO`, but can be anything that implements `.getvalue()`. + + This method assumes that the contents of `buff` are gzip-encoded (it + will add the appropriate headers to S3 to make the decompression + transparent via the browser). Filenames should end in whatever + suffix makes sense for the original file, so `.txt` instead of `.gz` + """ key = self.key_for(course_id, filename) data = buff.getvalue() @@ -251,19 +282,26 @@ class S3GradesStore(GradesStore): key.content_encoding = "gzip" key.content_type = "text/csv" + # Just setting the content encoding and type above should work + # according to the docs, but when experimenting, this was necessary for + # it to actually take. key.set_contents_from_string( data, headers={ - "Content-Encoding" : "gzip", - "Content-Length" : len(data), - "Content-Type" : "text/csv", + "Content-Encoding": "gzip", + "Content-Length": len(data), + "Content-Type": "text/csv", } ) def store_rows(self, course_id, filename, rows): """ - Given a course_id, filename, and rows (each row is an iterable of strings), - write this data out. + Given a `course_id`, `filename`, and `rows` (each row is an iterable of + strings), create a buffer that is a gzip'd csv file, and then `store()` + that buffer. + + Even though we store it in gzip format, browsers will transparently + download and decompress it. Filenames should end in `.csv`, not `.gz`. """ output_buffer = StringIO() gzip_file = GzipFile(fileobj=output_buffer, mode="wb") @@ -291,7 +329,11 @@ class LocalFSGradesStore(GradesStore): """ LocalFS implementation of a GradesStore. This is meant for debugging purposes and is *absolutely not for production use*. Use S3GradesStore for - that. + that. We use this in tests and for local development. When it generates + links, it will make file:/// style links. That means you actually have to + copy them and open them in a separate browser window, for security reasons. + This lets us do the cheap thing locally for debugging without having to open + up a separate URL that would only be used to send files in dev. """ def __init__(self, root_path): """ @@ -309,7 +351,10 @@ class LocalFSGradesStore(GradesStore): that there is a dict in settings named GRADES_DOWNLOAD and that it has a ROOT_PATH that maps to an absolute file path that the web app has write permissions to. `LocalFSGradesStore` will create any intermediate - directories as needed. + directories as needed. Example:: + + STORAGE_TYPE : "localfs" + ROOT_PATH : /tmp/edx/grade-downloads/ """ return cls(settings.GRADES_DOWNLOAD['ROOT_PATH']) @@ -344,7 +389,10 @@ class LocalFSGradesStore(GradesStore): def links_for(self, course_id): """ For a given `course_id`, return a list of `(filename, url)` tuples. `url` - can be plugged straight into an href + can be plugged straight into an href. Note that `LocalFSGradesStore` + will generate `file://` type URLs, so you'll need to copy the URL and + open it in a new browser window. Again, this class is only meant for + local development. """ course_dir = self.path_to(course_id, '') if not os.path.exists(course_dir): @@ -355,4 +403,4 @@ class LocalFSGradesStore(GradesStore): for filename in os.listdir(course_dir) ], reverse=True - ) \ No newline at end of file + ) diff --git a/lms/djangoapps/instructor_task/tasks.py b/lms/djangoapps/instructor_task/tasks.py index 2c8ad51e83..69e8ee1865 100644 --- a/lms/djangoapps/instructor_task/tasks.py +++ b/lms/djangoapps/instructor_task/tasks.py @@ -138,4 +138,4 @@ def calculate_grades_csv(entry_id, xmodule_instance_args): """ action_name = ugettext_noop('graded') task_fn = partial(push_grades_to_s3, xmodule_instance_args) - return run_main_task(entry_id, task_fn, action_name) \ No newline at end of file + return run_main_task(entry_id, task_fn, action_name) diff --git a/lms/djangoapps/instructor_task/tasks_helper.py b/lms/djangoapps/instructor_task/tasks_helper.py index c64f71b5f2..06941e7fdf 100644 --- a/lms/djangoapps/instructor_task/tasks_helper.py +++ b/lms/djangoapps/instructor_task/tasks_helper.py @@ -24,6 +24,7 @@ from courseware.models import StudentModule from courseware.model_data import FieldDataCache from courseware.module_render import get_module_for_descriptor_internal from instructor_task.models import GradesStore, InstructorTask, PROGRESS +from student.models import CourseEnrollment # define different loggers for use within tasks and on client side TASK_LOG = get_task_logger(__name__) @@ -477,27 +478,19 @@ def push_grades_to_s3(_xmodule_instance_args, _entry_id, course_id, _task_input, `GradesStore.from_config()`) and calling `link_for()` on it. Writes are buffered, so we'll never write part of a CSV file to S3 -- i.e. any files that are visible in GradesStore will be complete ones. + + As we start to add more CSV downloads, it will probably be worthwhile to + make a more general CSVDoc class instead of building out the rows like we + do here. """ - # Get start time for task: start_time = datetime.now(UTC) status_interval = 100 - # The pre-fetching of groups is done to make auth checks not require an - # additional DB lookup (this kills the Progress page in particular). - # But when doing grading at this scale, the memory required to store the resulting - # enrolled_students is too large to fit comfortably in memory, and subsequent - # course grading requests lead to memory fragmentation. So we will err here on the - # side of smaller memory allocations at the cost of additional lookups. - enrolled_students = User.objects.filter( - courseenrollment__course_id=course_id, - courseenrollment__is_active=True - ) - - # perform the main loop + enrolled_students = CourseEnrollment.users_enrolled_in(course_id) + num_total = enrolled_students.count() num_attempted = 0 num_succeeded = 0 num_failed = 0 - num_total = enrolled_students.count() curr_step = "Calculating Grades" def update_task_progress(): @@ -507,26 +500,27 @@ def push_grades_to_s3(_xmodule_instance_args, _entry_id, course_id, _task_input, 'action_name': action_name, 'attempted': num_attempted, 'succeeded': num_succeeded, - 'failed' : num_failed, - 'total' : num_total, + 'failed': num_failed, + 'total': num_total, 'duration_ms': int((current_time - start_time).total_seconds() * 1000), - 'step' : curr_step, + 'step': curr_step, } _get_current_task().update_state(state=PROGRESS, meta=progress) return progress - # Loop over all our students and build a + # Loop over all our students and build our CSV lists in memory header = None rows = [] err_rows = [["id", "username", "error_msg"]] for student, gradeset, err_msg in iterate_grades_for(course_id, enrolled_students): - # Periodically update task status (this is a db write) + # Periodically update task status (this is a cache write) if num_attempted % status_interval == 0: update_task_progress() num_attempted += 1 if gradeset: + # We were able to successfully grade this student for this course. num_succeeded += 1 if not header: header = [section['label'] for section in gradeset[u'section_breakdown']] @@ -537,28 +531,37 @@ def push_grades_to_s3(_xmodule_instance_args, _entry_id, course_id, _task_input, for section in gradeset[u'section_breakdown'] if 'label' in section } - row_percents = [percents[label] for label in header] + + # Not everybody has the same gradable items. If the item is not + # found in the user's gradeset, just assume it's a 0. The aggregated + # grades for their sections and overall course will be calculated + # without regard for the item they didn't have access to, so it's + # possible for a student to have a 0.0 show up in their row but + # still have 100% for the course. + row_percents = [percents.get(label, 0.0) for label in header] rows.append([student.id, student.email, student.username, gradeset['percent']] + row_percents) else: # An empty gradeset means we failed to grade a student. num_failed += 1 err_rows.append([student.id, student.username, err_msg]) + # By this point, we've got the rows we're going to stuff into our CSV files. curr_step = "Uploading CSVs" update_task_progress() - grades_store = GradesStore.from_config() + # Generate parts of the file name timestamp_str = start_time.strftime("%Y-%m-%d-%H%M") - - TASK_LOG.debug("Uploading CSV files for course {}".format(course_id)) - course_id_prefix = urllib.quote(course_id.replace("/", "_")) + + # Perform the actual upload + grades_store = GradesStore.from_config() grades_store.store_rows( course_id, "{}_grade_report_{}.csv".format(course_id_prefix, timestamp_str), rows ) - # If there are any error rows (don't count the header), write that out as well + + # If there are any error rows (don't count the header), write them out as well if len(err_rows) > 1: grades_store.store_rows( course_id, diff --git a/lms/envs/common.py b/lms/envs/common.py index a8575b89b6..09299b960a 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -195,10 +195,10 @@ MITX_FEATURES = { # Grade calculation started from the new instructor dashboard will write # grades CSV files to S3 and give links for downloads. - 'ENABLE_S3_GRADE_DOWNLOADS' : True, + 'ENABLE_S3_GRADE_DOWNLOADS': True, # Give course staff unrestricted access to grade downloads (if set to False, # only edX superusers can perform the downloads) - 'ALLOW_COURSE_STAFF_GRADE_DOWNLOADS' : False, + 'ALLOW_COURSE_STAFF_GRADE_DOWNLOADS': False, } # Used for A/B testing @@ -1071,11 +1071,11 @@ REGISTRATION_OPTIONAL_FIELDS = set([ 'goals', ]) -# Grades download +###################### Grade Downloads ###################### GRADES_DOWNLOAD_ROUTING_KEY = HIGH_MEM_QUEUE GRADES_DOWNLOAD = { - 'STORAGE_TYPE' : 'localfs', - 'BUCKET' : 'edx-grades', - 'ROOT_PATH' : '/tmp/edx-s3/grades', + 'STORAGE_TYPE': 'localfs', + 'BUCKET': 'edx-grades', + 'ROOT_PATH': '/tmp/edx-s3/grades', } diff --git a/lms/envs/dev.py b/lms/envs/dev.py index 86e214263d..1fe7fc330c 100644 --- a/lms/envs/dev.py +++ b/lms/envs/dev.py @@ -283,13 +283,6 @@ EDX_API_KEY = None ####################### Shoppingcart ########################### MITX_FEATURES['ENABLE_SHOPPING_CART'] = True -###################### Grade Downloads ###################### -GRADES_DOWNLOAD = { - 'STORAGE_TYPE' : 'localfs', - 'BUCKET' : 'edx-grades', - 'ROOT_PATH' : '/tmp/edx-s3/grades', -} - ##################################################################### # Lastly, see if the developer has any local overrides. try: diff --git a/lms/envs/test.py b/lms/envs/test.py index 4a56b41c09..49f8d29aa2 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -247,13 +247,6 @@ PASSWORD_HASHERS = ( # 'django.contrib.auth.hashers.CryptPasswordHasher', ) -###################### Grade Downloads ###################### -GRADES_DOWNLOAD = { - 'STORAGE_TYPE' : 'localfs', - 'BUCKET' : 'edx-grades', - 'ROOT_PATH' : '/tmp/edx-s3/grades', -} - ################### Make tests quieter # OpenID spews messages like this to stderr, we don't need to see them: From 901ad226526448863576f1fd7fc70e4df63741c4 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Fri, 22 Nov 2013 09:23:40 -0500 Subject: [PATCH 4/5] Make ENABLE_S3_GRADE_DOWNLOADS disabled by default, enabled in dev. Having ENABLE_S3_GRADE_DOWNLOADS enabled by default in common.py could lead to surprising behavior for folks downstream. They'd suddenly see a grade download screen on their new instructor dashboard, but the links by default would be local files and couldn't be used in an actual production environment. So we disable by default and let people explicitly enable it and set it up for S3 if they wish. LMS-58 --- CHANGELOG.rst | 4 ++++ lms/envs/common.py | 3 ++- lms/envs/dev.py | 1 + 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 69522b0584..b53780b916 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,10 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +LMS: Add feature for providing background grade report generation via Celery + instructor task, with reports uploaded to S3. Feature is visible on the beta + instructor dashboard. LMS-58 + LMS: Add a user-visible alert modal when a forums AJAX request fails. Blades: Add template for checkboxes response to studio. BLD-193. diff --git a/lms/envs/common.py b/lms/envs/common.py index 09299b960a..2fdfe3325d 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -195,7 +195,8 @@ MITX_FEATURES = { # Grade calculation started from the new instructor dashboard will write # grades CSV files to S3 and give links for downloads. - 'ENABLE_S3_GRADE_DOWNLOADS': True, + 'ENABLE_S3_GRADE_DOWNLOADS': False, + # Give course staff unrestricted access to grade downloads (if set to False, # only edX superusers can perform the downloads) 'ALLOW_COURSE_STAFF_GRADE_DOWNLOADS': False, diff --git a/lms/envs/dev.py b/lms/envs/dev.py index 1fe7fc330c..8fe5e7a19c 100644 --- a/lms/envs/dev.py +++ b/lms/envs/dev.py @@ -40,6 +40,7 @@ MITX_FEATURES['ENABLE_INSTRUCTOR_BETA_DASHBOARD'] = True MITX_FEATURES['MULTIPLE_ENROLLMENT_ROLES'] = True MITX_FEATURES['ENABLE_SHOPPING_CART'] = True MITX_FEATURES['AUTOMATIC_VERIFY_STUDENT_IDENTITY_FOR_TESTING'] = True +MITX_FEATURES['ENABLE_S3_GRADE_DOWNLOADS'] = True FEEDBACK_SUBMISSION_EMAIL = "dummy@example.com" From cf524906e0a3082f4b54424b3bc5c1fb92e787b6 Mon Sep 17 00:00:00 2001 From: Adam Palay Date: Tue, 12 Nov 2013 13:18:27 -0500 Subject: [PATCH 5/5] more granular transactions in grading [LMS-1480] remove field_data_cache from grades.grade and grades.progress_summary cleans grading code by adding wrappers --- lms/djangoapps/courseware/grades.py | 185 ++++++++++-------- .../tests/test_submitting_problems.py | 26 +-- lms/djangoapps/courseware/views.py | 67 ++++--- 3 files changed, 155 insertions(+), 123 deletions(-) diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index 438466c0ea..4cc0f90943 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -4,6 +4,7 @@ from collections import defaultdict import random import logging +from contextlib import contextmanager from collections import defaultdict from django.conf import settings from django.contrib.auth.models import User @@ -126,8 +127,20 @@ def answer_distributions(request, course): return counts -def grade(student, request, course, field_data_cache=None, keep_raw_scores=False): +@transaction.commit_manually +def grade(student, request, course, keep_raw_scores=False): """ + Wraps "_grade" with the manual_transaction context manager just in case + there are unanticipated errors. + """ + with manual_transaction(): + return _grade(student, request, course, keep_raw_scores) + + +def _grade(student, request, course, keep_raw_scores): + """ + Unwrapped version of "grade" + This grades a student as quickly as possible. It returns the output from the course grader, augmented with the final letter grade. The keys in the output are: @@ -137,20 +150,17 @@ def grade(student, request, course, field_data_cache=None, keep_raw_scores=False - grade : A final letter grade. - percent : The final percent for the class (rounded up). - section_breakdown : A breakdown of each section that makes - up the grade. (For display) + up the grade. (For display) - grade_breakdown : A breakdown of the major components that - make up the final grade. (For display) - - keep_raw_scores : if True, then value for key 'raw_scores' contains scores for every graded module + make up the final grade. (For display) + - keep_raw_scores : if True, then value for key 'raw_scores' contains scores + for every graded module More information on the format is in the docstring for CourseGrader. """ - grading_context = course.grading_context raw_scores = [] - if field_data_cache is None: - field_data_cache = FieldDataCache(grading_context['all_descriptors'], course.id, student) - totaled_scores = {} # This next complicated loop is just to collect the totaled_scores, which is # passed to the grader @@ -160,26 +170,22 @@ def grade(student, request, course, field_data_cache=None, keep_raw_scores=False section_descriptor = section['section_descriptor'] section_name = section_descriptor.display_name_with_default - should_grade_section = False + # some problems have state that is updated independently of interaction + # with the LMS, so they need to always be scored. (E.g. foldit., + # combinedopenended) + should_grade_section = any( + descriptor.always_recalculate_grades for descriptor in section['xmoduledescriptors'] + ) + # If we haven't seen a single problem in the section, we don't have to grade it at all! We can assume 0% - for moduledescriptor in section['xmoduledescriptors']: - # some problems have state that is updated independently of interaction - # with the LMS, so they need to always be scored. (E.g. foldit.) - if moduledescriptor.always_recalculate_grades: - should_grade_section = True - break - - # Create a fake key to pull out a StudentModule object from the FieldDataCache - - key = DjangoKeyValueStore.Key( - Scope.user_state, - student.id, - moduledescriptor.location, - None - ) - if field_data_cache.find(key): - should_grade_section = True - break + if not should_grade_section: + with manual_transaction(): + should_grade_section = StudentModule.objects.filter( + student=student, + module_state_key__in=[ + descriptor.location for descriptor in section['xmoduledescriptors'] + ] + ).exists() if should_grade_section: scores = [] @@ -188,11 +194,13 @@ def grade(student, request, course, field_data_cache=None, keep_raw_scores=False '''creates an XModule instance given a descriptor''' # TODO: We need the request to pass into here. If we could forego that, our arguments # would be simpler + with manual_transaction(): + field_data_cache = FieldDataCache([descriptor], course.id, student) return get_module_for_descriptor(student, request, descriptor, field_data_cache, course.id) for module_descriptor in yield_dynamic_descriptor_descendents(section_descriptor, create_module): - (correct, total) = get_score(course.id, student, module_descriptor, create_module, field_data_cache) + (correct, total) = get_score(course.id, student, module_descriptor, create_module) if correct is None and total is None: continue @@ -261,11 +269,23 @@ def grade_for_percentage(grade_cutoffs, percentage): return letter_grade +@transaction.commit_manually +def progress_summary(student, request, course): + """ + Wraps "_progress_summary" with the manual_transaction context manager just + in case there are unanticipated errors. + """ + with manual_transaction(): + return _progress_summary(student, request, course) + + # TODO: This method is not very good. It was written in the old course style and # then converted over and performance is not good. Once the progress page is redesigned # to not have the progress summary this method should be deleted (so it won't be copied). -def progress_summary(student, request, course, field_data_cache): +def _progress_summary(student, request, course): """ + Unwrapped version of "progress_summary". + This pulls a summary of all problems in the course. Returns @@ -278,20 +298,21 @@ def progress_summary(student, request, course, field_data_cache): Arguments: student: A User object for the student to grade course: A Descriptor containing the course to grade - field_data_cache: A FieldDataCache initialized with all - instance_modules for the student If the student does not have access to load the course module, this function will return None. """ - - # TODO: We need the request to pass into here. If we could forego that, our arguments - # would be simpler - course_module = get_module_for_descriptor(student, request, course, field_data_cache, course.id) - if not course_module: - # This student must not have access to the course. - return None + with manual_transaction(): + field_data_cache = FieldDataCache.cache_for_descriptor_descendents( + course.id, student, course, depth=None + ) + # TODO: We need the request to pass into here. If we could + # forego that, our arguments would be simpler + course_module = get_module_for_descriptor(student, request, course, field_data_cache, course.id) + if not course_module: + # This student must not have access to the course. + return None chapters = [] # Don't include chapters that aren't displayable (e.g. due to error) @@ -301,50 +322,52 @@ def progress_summary(student, request, course, field_data_cache): continue sections = [] + for section_module in chapter_module.get_display_items(): # Skip if the section is hidden - if section_module.hide_from_toc: - continue - - # Same for sections - graded = section_module.graded - scores = [] - - module_creator = section_module.xmodule_runtime.get_module - - for module_descriptor in yield_dynamic_descriptor_descendents(section_module, module_creator): - - course_id = course.id - (correct, total) = get_score(course_id, student, module_descriptor, module_creator, field_data_cache) - if correct is None and total is None: + with manual_transaction(): + if section_module.hide_from_toc: continue - scores.append(Score(correct, total, graded, module_descriptor.display_name_with_default)) + graded = section_module.graded + scores = [] - scores.reverse() - section_total, _ = graders.aggregate_scores( - scores, section_module.display_name_with_default) + module_creator = section_module.xmodule_runtime.get_module - module_format = section_module.format if section_module.format is not None else '' - sections.append({ - 'display_name': section_module.display_name_with_default, - 'url_name': section_module.url_name, - 'scores': scores, - 'section_total': section_total, - 'format': module_format, - 'due': section_module.due, - 'graded': graded, - }) + for module_descriptor in yield_dynamic_descriptor_descendents(section_module, module_creator): - chapters.append({'course': course.display_name_with_default, - 'display_name': chapter_module.display_name_with_default, - 'url_name': chapter_module.url_name, - 'sections': sections}) + course_id = course.id + (correct, total) = get_score(course_id, student, module_descriptor, module_creator) + if correct is None and total is None: + continue + + scores.append(Score(correct, total, graded, module_descriptor.display_name_with_default)) + + scores.reverse() + section_total, _ = graders.aggregate_scores( + scores, section_module.display_name_with_default) + + module_format = section_module.format if section_module.format is not None else '' + sections.append({ + 'display_name': section_module.display_name_with_default, + 'url_name': section_module.url_name, + 'scores': scores, + 'section_total': section_total, + 'format': module_format, + 'due': section_module.due, + 'graded': graded, + }) + + chapters.append({ + 'course': course.display_name_with_default, + 'display_name': chapter_module.display_name_with_default, + 'url_name': chapter_module.url_name, + 'sections': sections + }) return chapters - -def get_score(course_id, user, problem_descriptor, module_creator, field_data_cache): +def get_score(course_id, user, problem_descriptor, module_creator): """ Return the score for a user on a problem, as a tuple (correct, total). e.g. (5,7) if you got 5 out of 7 points. @@ -377,15 +400,14 @@ def get_score(course_id, user, problem_descriptor, module_creator, field_data_ca # These are not problems, and do not have a score return (None, None) - # Create a fake KeyValueStore key to pull out the StudentModule - key = DjangoKeyValueStore.Key( - Scope.user_state, - user.id, - problem_descriptor.location, - None - ) - - student_module = field_data_cache.find(key) + try: + student_module = StudentModule.objects.get( + student=user, + course_id=course_id, + module_state_key=problem_descriptor.location + ) + except StudentModule.DoesNotExist: + student_module = None if student_module is not None and student_module.max_grade is not None: correct = student_module.grade if student_module.grade is not None else 0 @@ -478,4 +500,3 @@ def iterate_grades_for(course_id, students): exc.message ) yield student, {}, exc.message - diff --git a/lms/djangoapps/courseware/tests/test_submitting_problems.py b/lms/djangoapps/courseware/tests/test_submitting_problems.py index 0736e08baf..854750f8d1 100644 --- a/lms/djangoapps/courseware/tests/test_submitting_problems.py +++ b/lms/djangoapps/courseware/tests/test_submitting_problems.py @@ -236,14 +236,11 @@ class TestCourseGrader(TestSubmittingProblems): make up the final grade. (For display) """ - field_data_cache = FieldDataCache.cache_for_descriptor_descendents( - self.course.id, self.student_user, self.course) + fake_request = self.factory.get( + reverse('progress', kwargs={'course_id': self.course.id}) + ) - fake_request = self.factory.get(reverse('progress', - kwargs={'course_id': self.course.id})) - - return grades.grade(self.student_user, fake_request, - self.course, field_data_cache) + return grades.grade(self.student_user, fake_request, self.course) def get_progress_summary(self): """ @@ -257,16 +254,13 @@ class TestCourseGrader(TestSubmittingProblems): etc. """ - field_data_cache = FieldDataCache.cache_for_descriptor_descendents( - self.course.id, self.student_user, self.course) + fake_request = self.factory.get( + reverse('progress', kwargs={'course_id': self.course.id}) + ) - fake_request = self.factory.get(reverse('progress', - kwargs={'course_id': self.course.id})) - - progress_summary = grades.progress_summary(self.student_user, - fake_request, - self.course, - field_data_cache) + progress_summary = grades.progress_summary( + self.student_user, fake_request, self.course + ) return progress_summary def check_grade_percent(self, percent): diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 819f65fca6..6646ea1e63 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -14,6 +14,7 @@ from django.shortcuts import redirect from mitxmako.shortcuts import render_to_response, render_to_string from django_future.csrf import ensure_csrf_cookie from django.views.decorators.cache import cache_control +from django.db import transaction from markupsafe import escape from courseware import grades @@ -643,8 +644,9 @@ def mktg_course_about(request, course_id): except (ValueError, Http404) as e: # if a course does not exist yet, display a coming # soon button - return render_to_response('courseware/mktg_coming_soon.html', - {'course_id': course_id}) + return render_to_response( + 'courseware/mktg_coming_soon.html', {'course_id': course_id} + ) registered = registered_for_course(course, request.user) @@ -659,21 +661,36 @@ def mktg_course_about(request, course_id): settings.MITX_FEATURES.get('ENABLE_LMS_MIGRATION')) course_modes = CourseMode.modes_for_course(course.id) - return render_to_response('courseware/mktg_course_about.html', - { - 'course': course, - 'registered': registered, - 'allow_registration': allow_registration, - 'course_target': course_target, - 'show_courseware_link': show_courseware_link, - 'course_modes': course_modes, - }) + return render_to_response( + 'courseware/mktg_course_about.html', + { + 'course': course, + 'registered': registered, + 'allow_registration': allow_registration, + 'course_target': course_target, + 'show_courseware_link': show_courseware_link, + 'course_modes': course_modes, + } + ) @login_required @cache_control(no_cache=True, no_store=True, must_revalidate=True) +@transaction.commit_manually def progress(request, course_id, student_id=None): - """ User progress. We show the grade bar and every problem score. + """ + Wraps "_progress" with the manual_transaction context manager just in case + there are unanticipated errors. + """ + with grades.manual_transaction(): + return _progress(request, course_id, student_id) + + +def _progress(request, course_id, student_id): + """ + Unwrapped version of "progress". + + User progress. We show the grade bar and every problem score. Course staff are allowed to see the progress of students in their class. """ @@ -696,26 +713,26 @@ def progress(request, course_id, student_id=None): # additional DB lookup (this kills the Progress page in particular). student = User.objects.prefetch_related("groups").get(id=student.id) - field_data_cache = FieldDataCache.cache_for_descriptor_descendents( - course_id, student, course, depth=None) + courseware_summary = grades.progress_summary(student, request, course) - courseware_summary = grades.progress_summary(student, request, course, - field_data_cache) - grade_summary = grades.grade(student, request, course, field_data_cache) + grade_summary = grades.grade(student, request, course) if courseware_summary is None: #This means the student didn't have access to the course (which the instructor requested) raise Http404 - context = {'course': course, - 'courseware_summary': courseware_summary, - 'grade_summary': grade_summary, - 'staff_access': staff_access, - 'student': student, - } - context.update() + context = { + 'course': course, + 'courseware_summary': courseware_summary, + 'grade_summary': grade_summary, + 'staff_access': staff_access, + 'student': student, + } - return render_to_response('courseware/progress.html', context) + with grades.manual_transaction(): + response = render_to_response('courseware/progress.html', context) + + return response @login_required