From ff1192657015e67f0f057309842fa094d867a2e7 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Mon, 19 Nov 2012 15:48:38 -0500 Subject: [PATCH] Initial impl and basic access tests for staff grading service --- lms/djangoapps/courseware/tests/tests.py | 14 +- .../instructor/staff_grading_service.py | 120 +++++++++++++++++- lms/djangoapps/instructor/tests.py | 54 ++++++-- lms/envs/common.py | 4 + 4 files changed, 175 insertions(+), 17 deletions(-) diff --git a/lms/djangoapps/courseware/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py index 1eebf0f408..eb026e9c6b 100644 --- a/lms/djangoapps/courseware/tests/tests.py +++ b/lms/djangoapps/courseware/tests/tests.py @@ -221,8 +221,7 @@ class PageLoader(ActivateLoginTestCase): def check_for_get_code(self, code, url): """ - Check that we got the expected code. Hacks around our broken 404 - handling. + Check that we got the expected code when accessing url via GET. """ resp = self.client.get(url) self.assertEqual(resp.status_code, code, @@ -230,6 +229,17 @@ class PageLoader(ActivateLoginTestCase): .format(resp.status_code, url, code)) + def check_for_post_code(self, code, url, data={}): + """ + Check that we got the expected code when accessing url via POST. + """ + resp = self.client.post(url, data) + self.assertEqual(resp.status_code, code, + "got code {0} for url '{1}'. Expected code {2}" + .format(resp.status_code, url, code)) + + + def check_pages_load(self, course_name, data_dir, modstore): """Make all locations in course load""" print "Checking course {0} in {1}".format(course_name, data_dir) diff --git a/lms/djangoapps/instructor/staff_grading_service.py b/lms/djangoapps/instructor/staff_grading_service.py index d4a4d82e63..e4f82cd5e0 100644 --- a/lms/djangoapps/instructor/staff_grading_service.py +++ b/lms/djangoapps/instructor/staff_grading_service.py @@ -3,14 +3,20 @@ This module provides views that proxy to the staff grading backend service. """ import json +import logging import requests import sys +from django.conf import settings from django.http import Http404 from django.http import HttpResponse - +from courseware.access import has_access from util.json_request import expect_json +from xmodule.course_module import CourseDescriptor + +log = logging.getLogger("mitx.courseware") + class GradingServiceError(Exception): pass @@ -37,21 +43,121 @@ class StaffGradingService(object): return r.text + def save_grade(course_id, submission_id, score, feedback): + """ + Save a grade. + + TODO: what is data? + + Returns json, or raises GradingServiceError if there's a problem. + """ + try: + r = self.session.get(url + 'save_grade') + except requests.exceptions.ConnectionError as err: + # reraise as promised GradingServiceError, but preserve stacktrace. + raise GradingServiceError, str(err), sys.exc_info()[2] + + return r.text + +_service = StaffGradingService(settings.STAFF_GRADING_BACKEND_URL) + + +def _err_response(msg): + """ + Return a HttpResponse with a json dump with success=False, and the given error message. + """ + return HttpResponse(json.dumps({'success': False, 'error': msg})) + + +def _check_access(user, course_id): + """ + Raise 404 if user doesn't have staff access to course_id + """ + course_location = CourseDescriptor.id_to_location(course_id) + if not has_access(user, course_location, 'staff'): + raise Http404 + + return + -#@login_required def get_next(request, course_id): """ + Get the next thing to grade for course_id. + + Returns a json dict with the following keys: + + 'success': bool + + 'submission_id': a unique identifier for the submission, to be passed back + with the grade. + + 'submission': the submission, rendered as read-only html for grading + + 'rubric': the rubric, also rendered as html. + + 'message': if there was no submission available, but nothing went wrong, + there will be a message field. + + 'error': if success is False, will have an error message with more info. """ - d = {'success': False} - return HttpResponse(json.dumps(d)) + _check_access(request.user, course_id) + + return HttpResponse(_get_next(course_id)) + + +def _get_next(course_id): + """ + Implementation of get_next (also called from save_grade) -- returns a json string + """ + + try: + return _service.get_next(course_id) + except GradingServiceError: + log.exception("Error from grading service") + return json.dumps({'success': False, 'error': 'Could not connect to grading service'}) -#@login_required @expect_json def save_grade(request, course_id): """ + Save the grade and feedback for a submission, and, if all goes well, return + the next thing to grade. + Expects the following POST parameters: + 'score': int + 'feedback': string + 'submission_id': int + + Returns the same thing as get_next, except that additional error messages + are possible if something goes wrong with saving the grade. """ - d = {'success': False} - return HttpResponse(json.dumps(d)) + _check_access(request.user, course_id) + + if request.method != 'POST': + raise Http404 + + required = ('score', 'feedback', 'submission_id') + for k in required: + if k not in request.POST.keys(): + return _err_response('Missing required key {0}'.format(k)) + + p = request.POST + + try: + result_json = _service.save_grade(course_id, p['submission_id'], p['score'], p['feedback']) + except GradingServiceError: + log.exception("Error saving grade") + return _err_response('Could not connect to grading service') + + try: + result = json.loads(result_json) + except ValueError: + return _err_response('Grading service returned mal-formatted data.') + + if not result.get('success', False): + log.warning('Got success=False from grading service. Response: %s', result_json) + return _err_response('Grading service failed') + + # Ok, save_grade seemed to work. Get the next submission to grade. + return HttpResponse(_get_next(course_id)) diff --git a/lms/djangoapps/instructor/tests.py b/lms/djangoapps/instructor/tests.py index 4f8ac140b0..5f740b8ff9 100644 --- a/lms/djangoapps/instructor/tests.py +++ b/lms/djangoapps/instructor/tests.py @@ -31,7 +31,6 @@ class TestInstructorDashboardGradeDownloadCSV(ct.PageLoader): def setUp(self): xmodule.modulestore.django._MODULESTORES = {} - courses = modulestore().get_courses() self.full = modulestore().get_course("edX/full/6.002_Spring_2012") self.toy = modulestore().get_course("edX/toy/2012_Fall") @@ -79,6 +78,7 @@ class TestInstructorDashboardGradeDownloadCSV(ct.PageLoader): "2","u2","Fred Weasley","view2@test.com","","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0.0","0.0" ''' self.assertEqual(body, expected_body, msg) + FORUM_ROLES = [ FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA ] FORUM_ADMIN_ACTION_SUFFIX = { FORUM_ROLE_ADMINISTRATOR : 'admin', FORUM_ROLE_MODERATOR : 'moderator', FORUM_ROLE_COMMUNITY_TA : 'community TA'} @@ -90,22 +90,19 @@ def action_name(operation, rolename): else: return '{0} forum {1}'.format(operation, FORUM_ADMIN_ACTION_SUFFIX[rolename]) + @override_settings(MODULESTORE=ct.TEST_DATA_XML_MODULESTORE) class TestInstructorDashboardForumAdmin(ct.PageLoader): ''' Check for change in forum admin role memberships ''' - + def setUp(self): xmodule.modulestore.django._MODULESTORES = {} courses = modulestore().get_courses() - def find_course(name): - """Assumes the course is present""" - return [c for c in courses if c.location.course==name][0] - - self.full = find_course("full") - self.toy = find_course("toy") + self.full = modulestore().get_course("edX/full/6.002_Spring_2012") + self.toy = modulestore().get_course("edX/toy/2012_Fall") # Create two accounts self.student = 'view@test.com' @@ -124,6 +121,8 @@ class TestInstructorDashboardForumAdmin(ct.PageLoader): self.login(self.instructor, self.password) self.enroll(self.toy) + + def initialize_roles(self, course_id): self.admin_role = Role.objects.get_or_create(name=FORUM_ROLE_ADMINISTRATOR, course_id=course_id)[0] self.moderator_role = Role.objects.get_or_create(name=FORUM_ROLE_MODERATOR, course_id=course_id)[0] @@ -210,3 +209,42 @@ class TestInstructorDashboardForumAdmin(ct.PageLoader): added_roles.sort() roles = ', '.join(added_roles) self.assertTrue(response.content.find('{0}'.format(roles))>=0, 'not finding roles "{0}"'.format(roles)) + + +@override_settings(MODULESTORE=ct.TEST_DATA_XML_MODULESTORE) +class TestStaffGradingService(ct.PageLoader): + ''' + Check that staff grading service proxy works. Basically just checking the + access control and error handling logic -- all the actual work is on the + backend. + ''' + + + + def setUp(self): + xmodule.modulestore.django._MODULESTORES = {} + + self.course_id = "edX/toy/2012_Fall" + self.toy = modulestore().get_course(self.course_id) + def make_instructor(course): + group_name = _course_staff_group_name(course.location) + g = Group.objects.create(name=group_name) + g.user_set.add(ct.user(self.instructor)) + + make_instructor(self.toy) + + self.logout() + + def test_access(self): + """ + Make sure only staff have access. + """ + self.login(self.student, self.password) + self.enroll(self.toy) + + # both get and post should return 404 + for view_name in ('staff_grading_get_next', 'staff_grading_save_grade'): + url = reverse(view_name, kwargs={'course_id': self.course_id}) + self.check_for_get_code(404, url) + self.check_for_post_code(404, url) + diff --git a/lms/envs/common.py b/lms/envs/common.py index 008de7ac84..7ddd2ffb9a 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -322,6 +322,10 @@ WIKI_USE_BOOTSTRAP_SELECT_WIDGET = False WIKI_LINK_LIVE_LOOKUPS = False WIKI_LINK_DEFAULT_LEVEL = 2 +################################# Staff grading config ##################### + +STAFF_GRADING_BACKEND_URL = None + ################################# Jasmine ################################### JASMINE_TEST_DIRECTORY = PROJECT_ROOT + '/static/coffee'