From 0573b2e4d47af879a3dad9a2b2b697af4bf1d590 Mon Sep 17 00:00:00 2001 From: "Dave St.Germain" Date: Fri, 3 May 2019 16:32:23 -0400 Subject: [PATCH 1/2] Grade runtime service support for Staff Graded Points --- cms/envs/common.py | 2 +- .../djangoapps/xblock_django/user_service.py | 1 + lms/djangoapps/grades/grade_utils.py | 191 ++++++++++++++++++ lms/djangoapps/grades/tasks.py | 1 + lms/djangoapps/grades/util_services.py | 30 ++- lms/envs/common.py | 1 - requirements/edx/base.txt | 1 + requirements/edx/development.txt | 1 + requirements/edx/github.in | 1 + requirements/edx/testing.txt | 1 + 10 files changed, 224 insertions(+), 6 deletions(-) diff --git a/cms/envs/common.py b/cms/envs/common.py index acc9b682ba..1b2e6cd06e 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1362,7 +1362,7 @@ ADVANCED_PROBLEM_TYPES = [ { 'component': 'drag-and-drop-v2', 'boilerplate_name': None - } + }, ] USERNAME_REPLACEMENT_WORKER = "REPLACE WITH VALID USERNAME" diff --git a/common/djangoapps/xblock_django/user_service.py b/common/djangoapps/xblock_django/user_service.py index 9ce52ca5c1..0ac947d63d 100644 --- a/common/djangoapps/xblock_django/user_service.py +++ b/common/djangoapps/xblock_django/user_service.py @@ -10,6 +10,7 @@ from xblock.reference.user_service import UserService, XBlockUser from openedx.core.djangoapps.user_api.preferences.api import get_user_preferences from student.models import anonymous_id_for_user, get_user_by_username_or_email + ATTR_KEY_IS_AUTHENTICATED = 'edx-platform.is_authenticated' ATTR_KEY_USER_ID = 'edx-platform.user_id' ATTR_KEY_USERNAME = 'edx-platform.username' diff --git a/lms/djangoapps/grades/grade_utils.py b/lms/djangoapps/grades/grade_utils.py index b3009d2bd4..f37aaa544e 100644 --- a/lms/djangoapps/grades/grade_utils.py +++ b/lms/djangoapps/grades/grade_utils.py @@ -1,11 +1,143 @@ """ This module contains utility functions for grading. """ +from __future__ import absolute_import, unicode_literals + +import logging +import time from datetime import timedelta + +from django.core.exceptions import ObjectDoesNotExist from django.utils import timezone +from django.utils.translation import ugettext as _ +from opaque_keys.edx.keys import UsageKey + +from courseware.models import StudentModule from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from student.models import CourseEnrollment +from super_csv import ChecksumMixin, CSVProcessor, DeferrableMixin + from .config.waffle import ENFORCE_FREEZE_GRADE_AFTER_COURSE_END, waffle_flags +log = logging.getLogger(__name__) + + +class ScoreCSVProcessor(ChecksumMixin, DeferrableMixin, CSVProcessor): + """ + CSV Processor for file format defined for Staff Graded Points + """ + columns = ['user_id', 'username', 'full_name', 'email', 'student_uid', + 'enrolled', 'track', 'block_id', 'title', 'date_last_graded', + 'who_last_graded', 'csum', 'last_points', 'points'] + required_columns = ['user_id', 'points', 'csum', 'block_id', 'last_points'] + checksum_columns = ['user_id', 'block_id', 'last_points'] + # files larger than 100 rows will be processed asynchronously + size_to_defer = 100 + max_file_size = 4 * 1024 * 1024 + handle_undo = False + + def __init__(self, **kwargs): + self.now = time.time() + self.max_points = 1 + self.display_name = '' + super(ScoreCSVProcessor, self).__init__(**kwargs) + self.users_seen = {} + + def get_unique_path(self): + return 'csv/state/{}/{}'.format(self.block_id, self.now) + + def validate_row(self, row): + valid = super(ScoreCSVProcessor, self).validate_row(row) + if valid: + valid = row['block_id'] == self.block_id + if valid: + points = row['points'] + if points: + try: + valid = float(row['points']) <= self.max_points + if not valid: + self.add_error(_('Points must not be greater than {}.').format(self.max_points)) + except ValueError: + self.add_error(_('Points must be numbers.')) + valid = False + else: + valid = True + else: + self.add_error(_('The CSV does not match this problem. Check that you uploaded the right CSV.')) + return valid + + def preprocess_row(self, row): + if row['points'] and row['user_id'] not in self.users_seen: + to_save = { + 'user_id': row['user_id'], + 'block_id': self.block_id, + 'new_points': float(row['points']), + 'max_points': self.max_points + } + self.users_seen[row['user_id']] = 1 + return to_save + + def process_row(self, row): + if self.handle_undo: + # get the current score, for undo. expensive + undo = get_score(row['block_id'], row['user_id']) + undo['new_points'] = undo['score'] + undo['max_points'] = row['max_points'] + else: + undo = None + set_score(row['block_id'], row['user_id'], row['new_points'], row['max_points']) + return True, undo + + def _get_enrollments(self, course_id, **kwargs): # pylint: disable=unused-argument + """ + Return iterator of enrollments, as dicts. + """ + enrollments = CourseEnrollment.objects.filter( + course_id=course_id).select_related('programcourseenrollment') + for enrollment in enrollments: + enrd = { + 'user_id': enrollment.user.id, + 'username': enrollment.user.username, + 'full_name': enrollment.user.profile.name, + 'email': enrollment.user.email, + 'enrolled': enrollment.is_active, + 'track': enrollment.mode, + } + try: + pce = enrollment.programcourseenrollment.program_enrollment + enrd['student_uid'] = pce.external_user_key + except ObjectDoesNotExist: + enrd['student_uid'] = None + yield enrd + + def get_rows_to_export(self): + """ + Return iterator of rows for file export. + """ + location = UsageKey.from_string(self.block_id) + my_name = self.display_name + + students = get_scores(location) + + enrollments = self._get_enrollments(location.course_key) + for enrollment in enrollments: + row = { + 'block_id': location, + 'title': my_name, + 'points': None, + 'last_points': None, + 'date_last_graded': None, + 'who_last_graded': None, + } + row.update(enrollment) + score = students.get(enrollment['user_id'], None) + + if score: + row['last_points'] = int(score['grade'] * self.max_points) + row['date_last_graded'] = score['modified'] + # TODO: figure out who last graded + yield row + def are_grades_frozen(course_key): """ Returns whether grades are frozen for the given course. """ @@ -16,3 +148,62 @@ def are_grades_frozen(course_key): now = timezone.now() return now > freeze_grade_date return False + + +def set_score(usage_key, student_id, score, max_points, **defaults): + """ + Set a score. + """ + if not isinstance(usage_key, UsageKey): + usage_key = UsageKey.from_string(usage_key) + defaults['module_type'] = 'problem' + defaults['grade'] = score / max_points + defaults['max_grade'] = max_points + StudentModule.objects.update_or_create( + student_id=student_id, + course_id=usage_key.course_key, + module_state_key=usage_key, + defaults=defaults) + + +def get_score(usage_key, user_id): + """ + Return score for user_id and usage_key. + """ + if not isinstance(usage_key, UsageKey): + usage_key = UsageKey.from_string(usage_key) + try: + score = StudentModule.objects.get( + course_id=usage_key.course_key, + module_state_key=usage_key, + student_id=user_id + ) + except StudentModule.DoesNotExist: + return None + else: + return { + 'grade': score.grade, + 'score': score.grade * (score.max_grade or 1), + 'max_grade': score.max_grade, + 'created': score.created, + 'modified': score.modified + } + + +def get_scores(usage_key, user_ids=None): + """ + Return dictionary of student_id: scores. + """ + scores_qset = StudentModule.objects.filter( + course_id=usage_key.course_key, + module_state_key=usage_key, + ) + if user_ids: + scores_qset = scores_qset.filter(student_id__in=user_ids) + + return {row.student_id: {'grade': row.grade, + 'score': row.grade * (row.max_grade or 1), + 'max_grade': row.max_grade, + 'created': row.created, + 'modified': row.modified, + 'state': row.state} for row in scores_qset} diff --git a/lms/djangoapps/grades/tasks.py b/lms/djangoapps/grades/tasks.py index 6c10b47d22..9329fd37c0 100644 --- a/lms/djangoapps/grades/tasks.py +++ b/lms/djangoapps/grades/tasks.py @@ -35,6 +35,7 @@ from .subsection_grade_factory import SubsectionGradeFactory from .transformer import GradesTransformer from .grade_utils import are_grades_frozen + log = getLogger(__name__) COURSE_GRADE_TIMEOUT_SECONDS = 1200 diff --git a/lms/djangoapps/grades/util_services.py b/lms/djangoapps/grades/util_services.py index dff8f11b5c..88be25879e 100644 --- a/lms/djangoapps/grades/util_services.py +++ b/lms/djangoapps/grades/util_services.py @@ -1,7 +1,5 @@ "A light weight interface to grading helper functions" - - -from .grade_utils import are_grades_frozen +from . import grade_utils class GradesUtilService(object): @@ -14,4 +12,28 @@ class GradesUtilService(object): def are_grades_frozen(self): "Check if grades are frozen for given course key" - return are_grades_frozen(self.course_id) + return grade_utils.are_grades_frozen(self.course_id) + + def get_score(self, usage_key, user_id): + """ + Return score for user_id and usage_key. + """ + return grade_utils.get_score(usage_key, user_id) + + def get_scores(self, usage_key, user_ids=None): + """ + Return dictionary of student_id: scores. + """ + return grade_utils.get_scores(usage_key, user_ids) + + def set_score(self, usage_key, student_id, score, max_points, **defaults): + """ + Set a score. + """ + return grade_utils.set_score(usage_key, student_id, score, max_points, **defaults) + + def get_score_processor(self, **kwargs): + """ + Return a csv score processor. + """ + return grade_utils.ScoreCSVProcessor(**kwargs) diff --git a/lms/envs/common.py b/lms/envs/common.py index a0ac3829e3..78125dafc6 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -574,7 +574,6 @@ def _make_mako_template_dirs(settings): CONTEXT_PROCESSORS = [ 'django.template.context_processors.request', 'django.template.context_processors.static', - 'django.contrib.messages.context_processors.messages', 'django.template.context_processors.i18n', 'django.contrib.auth.context_processors.auth', # this is required for admin 'django.template.context_processors.csrf', diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index ada9da6755..0a74d849ad 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -29,6 +29,7 @@ git+https://github.com/edx/edx-ora2.git@2.2.3#egg=ora2==2.2.3 git+https://github.com/edx/RecommenderXBlock.git@1.4.0#egg=recommender-xblock==1.4.0 -e common/lib/safe_lxml -e common/lib/sandbox-packages +git+https://github.com/davestgermain/super-csv@4963bf5da5489f06369da9cce84c92e3e42fe3d2#egg=super-csv==0.1.0 -e common/lib/symmath -e openedx/core/lib/xblock_builtin/xblock_discussion git+https://github.com/edx-solutions/xblock-drag-and-drop-v2@v2.2.1#egg=xblock-drag-and-drop-v2==2.2.1 diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index e47f6aaa96..813f57d958 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -30,6 +30,7 @@ git+https://github.com/edx/edx-ora2.git@2.2.3#egg=ora2==2.2.3 git+https://github.com/edx/RecommenderXBlock.git@1.4.0#egg=recommender-xblock==1.4.0 -e common/lib/safe_lxml -e common/lib/sandbox-packages +git+https://github.com/davestgermain/super-csv@4963bf5da5489f06369da9cce84c92e3e42fe3d2#egg=super-csv==0.1.0 -e common/lib/symmath -e openedx/core/lib/xblock_builtin/xblock_discussion git+https://github.com/edx-solutions/xblock-drag-and-drop-v2@v2.2.1#egg=xblock-drag-and-drop-v2==2.2.1 diff --git a/requirements/edx/github.in b/requirements/edx/github.in index f9c021e6f7..c362d21bf3 100644 --- a/requirements/edx/github.in +++ b/requirements/edx/github.in @@ -94,6 +94,7 @@ -e git+https://github.com/edx/DoneXBlock.git@01a14f3bd80ae47dd08cdbbe2f88f3eb88d00fba#egg=done-xblock -e git+https://github.com/edx-solutions/xblock-google-drive.git@138e6fa0bf3a2013e904a085b9fed77dab7f3f21#egg=xblock-google-drive -e git+https://github.com/edx/xblock-lti-consumer.git@v1.1.8#egg=lti_consumer-xblock==1.1.8 +-e git+https://github.com/davestgermain/super-csv@4963bf5da5489f06369da9cce84c92e3e42fe3d2#egg=super-csv==0.1.0 # Third Party XBlocks diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 1274a877b5..bbc73ea0e2 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -29,6 +29,7 @@ git+https://github.com/edx/edx-ora2.git@2.2.3#egg=ora2==2.2.3 git+https://github.com/edx/RecommenderXBlock.git@1.4.0#egg=recommender-xblock==1.4.0 -e common/lib/safe_lxml -e common/lib/sandbox-packages +git+https://github.com/davestgermain/super-csv@4963bf5da5489f06369da9cce84c92e3e42fe3d2#egg=super-csv==0.1.0 -e common/lib/symmath -e openedx/core/lib/xblock_builtin/xblock_discussion git+https://github.com/edx-solutions/xblock-drag-and-drop-v2@v2.2.1#egg=xblock-drag-and-drop-v2==2.2.1 From 5889d397f0d230c26e976bec04370f31fb8b2070 Mon Sep 17 00:00:00 2001 From: "Dave St.Germain" Date: Mon, 20 May 2019 12:10:28 -0400 Subject: [PATCH 2/2] Addressed review --- lms/djangoapps/grades/grade_utils.py | 67 +++++++++++++++------------- 1 file changed, 36 insertions(+), 31 deletions(-) diff --git a/lms/djangoapps/grades/grade_utils.py b/lms/djangoapps/grades/grade_utils.py index f37aaa544e..37c8f0c80f 100644 --- a/lms/djangoapps/grades/grade_utils.py +++ b/lms/djangoapps/grades/grade_utils.py @@ -26,7 +26,7 @@ class ScoreCSVProcessor(ChecksumMixin, DeferrableMixin, CSVProcessor): """ CSV Processor for file format defined for Staff Graded Points """ - columns = ['user_id', 'username', 'full_name', 'email', 'student_uid', + columns = ['user_id', 'username', 'full_name', 'student_uid', 'enrolled', 'track', 'block_id', 'title', 'date_last_graded', 'who_last_graded', 'csum', 'last_points', 'points'] required_columns = ['user_id', 'points', 'csum', 'block_id', 'last_points'] @@ -47,24 +47,20 @@ class ScoreCSVProcessor(ChecksumMixin, DeferrableMixin, CSVProcessor): return 'csv/state/{}/{}'.format(self.block_id, self.now) def validate_row(self, row): - valid = super(ScoreCSVProcessor, self).validate_row(row) - if valid: - valid = row['block_id'] == self.block_id - if valid: - points = row['points'] - if points: - try: - valid = float(row['points']) <= self.max_points - if not valid: - self.add_error(_('Points must not be greater than {}.').format(self.max_points)) - except ValueError: - self.add_error(_('Points must be numbers.')) - valid = False - else: - valid = True - else: - self.add_error(_('The CSV does not match this problem. Check that you uploaded the right CSV.')) - return valid + if not super(ScoreCSVProcessor, self).validate_row(row): + return False + if row['block_id'] != self.block_id: + self.add_error(_('The CSV does not match this problem. Check that you uploaded the right CSV.')) + return False + if row['points']: + try: + if float(row['points']) > self.max_points: + self.add_error(_('Points must not be greater than {}.').format(self.max_points)) + return False + except ValueError: + self.add_error(_('Points must be numbers.')) + return False + return True def preprocess_row(self, row): if row['points'] and row['user_id'] not in self.users_seen: @@ -78,6 +74,11 @@ class ScoreCSVProcessor(ChecksumMixin, DeferrableMixin, CSVProcessor): return to_save def process_row(self, row): + """ + Set the score for the given row, returning (status, undo) + undo is a dict of an operation which would undo the set_score. In this case, + that means we would have to call get_score, which could be expensive to do for the entire file. + """ if self.handle_undo: # get the current score, for undo. expensive undo = get_score(row['block_id'], row['user_id']) @@ -95,20 +96,19 @@ class ScoreCSVProcessor(ChecksumMixin, DeferrableMixin, CSVProcessor): enrollments = CourseEnrollment.objects.filter( course_id=course_id).select_related('programcourseenrollment') for enrollment in enrollments: - enrd = { + enrollment_dict = { 'user_id': enrollment.user.id, 'username': enrollment.user.username, 'full_name': enrollment.user.profile.name, - 'email': enrollment.user.email, 'enrolled': enrollment.is_active, 'track': enrollment.mode, } try: pce = enrollment.programcourseenrollment.program_enrollment - enrd['student_uid'] = pce.external_user_key + enrollment_dict['student_uid'] = pce.external_user_key except ObjectDoesNotExist: - enrd['student_uid'] = None - yield enrd + enrollment_dict['student_uid'] = None + yield enrollment_dict def get_rows_to_export(self): """ @@ -157,7 +157,7 @@ def set_score(usage_key, student_id, score, max_points, **defaults): if not isinstance(usage_key, UsageKey): usage_key = UsageKey.from_string(usage_key) defaults['module_type'] = 'problem' - defaults['grade'] = score / max_points + defaults['grade'] = score / (max_points or 1.0) defaults['max_grade'] = max_points StudentModule.objects.update_or_create( student_id=student_id, @@ -201,9 +201,14 @@ def get_scores(usage_key, user_ids=None): if user_ids: scores_qset = scores_qset.filter(student_id__in=user_ids) - return {row.student_id: {'grade': row.grade, - 'score': row.grade * (row.max_grade or 1), - 'max_grade': row.max_grade, - 'created': row.created, - 'modified': row.modified, - 'state': row.state} for row in scores_qset} + scores = {} + for row in scores_qset: + scores[row.student_id] = { + 'grade': row.grade, + 'score': row.grade * (row.max_grade or 1), + 'max_grade': row.max_grade, + 'created': row.created, + 'modified': row.modified, + 'state': row.state, + } + return scores