diff --git a/lms/djangoapps/open_ended_grading/grading_service.py b/lms/djangoapps/open_ended_grading/grading_service.py index 3c92c5bddd..96bd931448 100644 --- a/lms/djangoapps/open_ended_grading/grading_service.py +++ b/lms/djangoapps/open_ended_grading/grading_service.py @@ -44,7 +44,7 @@ class GradingService(object): return response.json - def post(self, url, allow_redirects, data): + def post(self, url, data, allow_redirects=False): """ Make a post request to the grading controller """ @@ -58,7 +58,7 @@ class GradingService(object): return r.text - def get(self, url, allow_redirects, params): + def get(self, url, params, allow_redirects=False): """ Make a get request to the grading controller """ diff --git a/lms/djangoapps/open_ended_grading/peer_grading_service.py b/lms/djangoapps/open_ended_grading/peer_grading_service.py index b8d8d2dbe6..859499ff7e 100644 --- a/lms/djangoapps/open_ended_grading/peer_grading_service.py +++ b/lms/djangoapps/open_ended_grading/peer_grading_service.py @@ -1,3 +1,11 @@ +""" +This module provides an interface on the grading-service backend +for peer grading + +Use peer_grading_service() to get the version specified +in settings.PEER_GRADING_INTERFACE + +""" import json import logging import requests @@ -16,6 +24,10 @@ from student.models import unique_id_for_user log = logging.getLogger(__name__) +""" +This is a mock peer grading service that can be used for unit tests +without making actual service calls to the grading controller +""" class MockPeerGradingService(object): def get_next_submission(self, problem_location, grader_id): return json.dumps({'success': True, @@ -26,7 +38,8 @@ class MockPeerGradingService(object): 'rubric': 'fake rubric', 'max_score': 4}) - def save_grade(self, location, grader_id, submission_id, score, feedback, submission_key): + def save_grade(self, location, grader_id, submission_id, + score, feedback, submission_key): return json.dumps({'success': True}) def is_student_calibrated(self, problem_location, grader_id): @@ -41,15 +54,16 @@ class MockPeerGradingService(object): 'rubric': 'fake rubric', 'max_score': 4}) - def save_calibration_essay(self, problem_location, grader_id, calibration_essay_id, submission_key, score, feedback): + def save_calibration_essay(self, problem_location, grader_id, + calibration_essay_id, submission_key, score, feedback): return {'success': True, 'actual_score': 2} def get_problem_list(self, course_id, grader_id): return json.dumps({'success': True, 'problem_list': [ - json.dumps({'location': 'i4x://MITx/3.091x/problem/open_ended_demo1', \ + json.dumps({'location': 'i4x://MITx/3.091x/problem/open_ended_demo1', 'problem_name': "Problem 1", 'num_graded': 3, 'num_pending': 5}), - json.dumps({'location': 'i4x://MITx/3.091x/problem/open_ended_demo2', \ + json.dumps({'location': 'i4x://MITx/3.091x/problem/open_ended_demo2', 'problem_name': "Problem 2", 'num_graded': 1, 'num_pending': 5}) ]}) @@ -78,15 +92,15 @@ class PeerGradingService(GradingService): 'feedback' : feedback, 'submission_key': submission_key, 'location': location} - return self.post(self.save_grade_url, False, data) + return self.post(self.save_grade_url, data) def is_student_calibrated(self, problem_location, grader_id): params = {'problem_id' : problem_location, 'student_id': grader_id} - return self.get(self.is_student_calibrated_url, False, params) + return self.get(self.is_student_calibrated_url, params) def show_calibration_essay(self, problem_location, grader_id): params = {'problem_id' : problem_location, 'student_id': grader_id} - return self.get(self.show_calibration_essay_url, False, params) + return self.get(self.show_calibration_essay_url, params) def save_calibration_essay(self, problem_location, grader_id, calibration_essay_id, submission_key, score, feedback): data = {'location': problem_location, @@ -95,11 +109,11 @@ class PeerGradingService(GradingService): 'submission_key': submission_key, 'score': score, 'feedback': feedback} - return self.post(self.save_calibration_essay_url, False, data) + return self.post(self.save_calibration_essay_url, data) def get_problem_list(self, course_id, grader_id): params = {'course_id': course_id, 'student_id': grader_id} - response = self.get(self.get_problem_list_url, False, params) + response = self.get(self.get_problem_list_url, params) return response @@ -175,8 +189,8 @@ def get_next_submission(request, course_id): return HttpResponse(response, mimetype="application/json") except GradingServiceError: - log.exception("Error from grading service. server url: {0}" - .format(staff_grading_service().url)) + log.exception("Error getting next submission. server url: {0} location: {1}, grader_id: {2}" + .format(staff_grading_service().url, location, grader_id)) return json.dumps({'success': False, 'error': 'Could not connect to grading service'}) @@ -212,8 +226,11 @@ def save_grade(request, course_id): score, feedback, submission_key) return HttpResponse(response, mimetype="application/json") except GradingServiceError: - log.exception("Error from grading service. server url: {0}" - .format(staff_grading_service().url)) + log.exception("""Error saving grade. server url: {0}, location: {1}, submission_id:{2}, + submission_key: {3}, score: {4}""" + .format(staff_grading_service().url, + location, submission_id, submission_key, score) + ) return json.dumps({'success': False, 'error': 'Could not connect to grading service'}) @@ -249,8 +266,8 @@ def is_student_calibrated(request, course_id): response = peer_grading_service().is_student_calibrated(location, grader_id) return HttpResponse(response, mimetype="application/json") except GradingServiceError: - log.exception("Error from grading service. server url: {0}" - .format(staff_grading_service().url)) + log.exception("Error from grading service. server url: {0}, grader_id: {0}, location: {1}" + .format(staff_grading_service().url, grader_id, location)) return json.dumps({'success': False, 'error': 'Could not connect to grading service'}) @@ -293,8 +310,8 @@ def show_calibration_essay(request, course_id): response = peer_grading_service().show_calibration_essay(location, grader_id) return HttpResponse(response, mimetype="application/json") except GradingServiceError: - log.exception("Error from grading service. server url: {0}" - .format(staff_grading_service().url)) + log.exception("Error from grading service. server url: {0}, location: {0}" + .format(staff_grading_service().url, location)) return json.dumps({'success': False, 'error': 'Could not connect to grading service'}) @@ -334,5 +351,5 @@ def save_calibration_essay(request, course_id): response = peer_grading_service().save_calibration_essay(location, grader_id, calibration_essay_id, submission_key, score, feedback) return HttpResponse(response, mimetype="application/json") except GradingServiceError: - log.exception("Error saving calibration grade") + log.exception("Error saving calibration grade, location: {0}, submission_id: {1}, submission_key: {2}, grader_id: {3}".format(location, submission_id, submission_key, grader_id)) return _err_response('Could not connect to grading service') diff --git a/lms/djangoapps/open_ended_grading/staff_grading_service.py b/lms/djangoapps/open_ended_grading/staff_grading_service.py index 521983eeee..5c6cec17eb 100644 --- a/lms/djangoapps/open_ended_grading/staff_grading_service.py +++ b/lms/djangoapps/open_ended_grading/staff_grading_service.py @@ -85,7 +85,7 @@ class StaffGradingService(GradingService): GradingServiceError: something went wrong with the connection. """ params = {'course_id': course_id,'grader_id': grader_id} - return self.get(self.get_problem_list_url, False, params) + return self.get(self.get_problem_list_url, params) def get_next(self, course_id, location, grader_id): @@ -107,7 +107,6 @@ class StaffGradingService(GradingService): GradingServiceError: something went wrong with the connection. """ return self.get(self.get_next_url, - allow_redirects=False, params={'location': location, 'grader_id': grader_id}) @@ -131,8 +130,7 @@ class StaffGradingService(GradingService): 'grader_id': grader_id, 'skipped': skipped} - return self.post(self.save_grade_url, data=data, - allow_redirects=False) + return self.post(self.save_grade_url, data=data) # don't initialize until staff_grading_service() is called--means that just # importing this file doesn't create objects that may not have the right config diff --git a/lms/djangoapps/open_ended_grading/tests.py b/lms/djangoapps/open_ended_grading/tests.py index 30a58f6ee8..0c4376a44b 100644 --- a/lms/djangoapps/open_ended_grading/tests.py +++ b/lms/djangoapps/open_ended_grading/tests.py @@ -1,8 +1,7 @@ """ -This file demonstrates writing tests using the unittest module. These will pass -when you run "manage.py test". +Tests for open ended grading interfaces -Replace this with more appropriate tests for your application. +django-admin.py test --settings=lms.envs.test --pythonpath=. lms/djangoapps/open_ended_grading """ from django.test import TestCase diff --git a/lms/djangoapps/open_ended_grading/views.py b/lms/djangoapps/open_ended_grading/views.py index e1aaf7011f..858c9a4fd5 100644 --- a/lms/djangoapps/open_ended_grading/views.py +++ b/lms/djangoapps/open_ended_grading/views.py @@ -1,39 +1,20 @@ # Grading Views -from collections import defaultdict -import csv import logging -import os import urllib from django.conf import settings -from django.contrib.auth.models import User, Group -from django.http import HttpResponse -from django_future.csrf import ensure_csrf_cookie from django.views.decorators.cache import cache_control from mitxmako.shortcuts import render_to_response from django.core.urlresolvers import reverse -from courseware import grades -from courseware.access import has_access, get_access_group_name -from courseware.courses import get_course_with_access -from django_comment_client.models import Role, FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA -from django_comment_client.utils import has_forum_access -from psychometrics import psychoanalyze -from student.models import CourseEnrollment from student.models import unique_id_for_user -from xmodule.course_module import CourseDescriptor -from xmodule.modulestore import Location -from xmodule.modulestore.django import modulestore -from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError, NoPathToItem -from xmodule.modulestore.search import path_to_location +from courseware.courses import get_course_with_access from peer_grading_service import PeerGradingService from peer_grading_service import MockPeerGradingService from grading_service import GradingServiceError import json -import track.views - from .staff_grading import StaffGrading @@ -45,6 +26,18 @@ if settings.MOCK_PEER_GRADING: else: peer_gs = PeerGradingService(settings.PEER_GRADING_INTERFACE) +""" +Reverses the URL from the name and the course id, and then adds a trailing slash if +it does not exist yet + +""" +def _reverse_with_slash(url_name, course_id): + ajax_url = reverse(url_name, kwargs={'course_id': course_id}) + if not ajax_url.endswith('/'): + ajax_url += '/' + return ajax_url + + @cache_control(no_cache=True, no_store=True, must_revalidate=True) def staff_grading(request, course_id): """ @@ -52,14 +45,9 @@ def staff_grading(request, course_id): """ course = get_course_with_access(request.user, course_id, 'staff') - grading = StaffGrading(course) - - ajax_url = reverse('staff_grading', kwargs={'course_id': course_id}) - if not ajax_url.endswith('/'): - ajax_url += '/' + ajax_url = _reverse_with_slash('staff_grading', course_id) return render_to_response('instructor/staff_grading.html', { - 'view_html': '', 'course': course, 'course_id': course_id, 'ajax_url': ajax_url, @@ -79,27 +67,25 @@ def peer_grading(request, course_id): error_text = "" problem_list = [] try: - problem_list_text = peer_gs.get_problem_list(course_id, unique_id_for_user(request.user)) - problem_list_json = json.loads(problem_list_text) - success = problem_list_json['success'] - if 'error' in problem_list_json: - error_text = problem_list_json['error'] + problem_list_json = peer_gs.get_problem_list(course_id, unique_id_for_user(request.user)) + problem_list_dict = json.loads(problem_list_json) + success = problem_list_dict['success'] + if 'error' in problem_list_dict: + error_text = problem_list_dict['error'] - problem_list = problem_list_json['problem_list'] + problem_list = problem_list_dict['problem_list'] except GradingServiceError: error_text = "Error occured while contacting the grading service" success = False + # catch error if if the json loads fails except ValueError: error_text = "Could not get problem list" success = False - ajax_url = reverse('peer_grading', kwargs={'course_id': course_id}) - if not ajax_url.endswith('/'): - ajax_url += '/' + ajax_url = _reverse_with_slash('peer_grading', course_id) return render_to_response('peer_grading/peer_grading.html', { - 'view_html': '', 'course': course, 'course_id': course_id, 'ajax_url': ajax_url, @@ -118,9 +104,7 @@ def peer_grading_problem(request, course_id): course = get_course_with_access(request.user, course_id, 'load') problem_location = request.GET.get("location") - ajax_url = reverse('peer_grading', kwargs={'course_id': course_id}) - if not ajax_url.endswith('/'): - ajax_url += '/' + ajax_url = _reverse_with_slash('peer_grading', course_id) return render_to_response('peer_grading/peer_grading_problem.html', { 'view_html': '', diff --git a/lms/static/coffee/src/peer_grading/peer_grading.coffee b/lms/static/coffee/src/peer_grading/peer_grading.coffee index c20944252c..0736057df8 100644 --- a/lms/static/coffee/src/peer_grading/peer_grading.coffee +++ b/lms/static/coffee/src/peer_grading/peer_grading.coffee @@ -1,10 +1,13 @@ +# This is a simple class that just hides the error container +# and message container when they are empty +# Can (and should be) expanded upon when our problem list +# becomes more sophisticated class PeerGrading - constructor: (backend) -> + constructor: () -> @error_container = $('.error-container') @error_container.toggle(not @error_container.is(':empty')) @message_container = $('.message-container') @message_container.toggle(not @message_container.is(':empty')) -mock_backend = false -$(document).ready(() -> new PeerGrading(mock_backend)) +$(document).ready(() -> new PeerGrading()) diff --git a/lms/static/coffee/src/peer_grading/peer_grading_problem.coffee b/lms/static/coffee/src/peer_grading/peer_grading_problem.coffee index 639fcca947..e294c50f7c 100644 --- a/lms/static/coffee/src/peer_grading/peer_grading_problem.coffee +++ b/lms/static/coffee/src/peer_grading/peer_grading_problem.coffee @@ -1,3 +1,20 @@ +################################## +# +# This is the JS that renders the peer grading problem page. +# Fetches the correct problem and/or calibration essay +# and sends back the grades +# +# Should not be run when we don't have a location to send back +# to the server +# +# PeerGradingProblemBackend - +# makes all the ajax requests and provides a mock interface +# for testing purposes +# +# PeerGradingProblem - +# handles the rendering and user interactions with the interface +# +################################## class PeerGradingProblemBackend constructor: (ajax_url, mock_backend) -> @mock_backend = mock_backend @@ -8,7 +25,7 @@ class PeerGradingProblemBackend if @mock_backend callback(@mock(cmd, data)) else - # TODO: replace with postWithPrefix when that's loaded + # if this post request fails, the error callback will catch it $.post(@ajax_url + cmd, data, callback) .error => callback({success: false, error: "Error occured while performing this operation"}) @@ -90,13 +107,13 @@ class PeerGradingProblem @prompt_wrapper = $('.prompt-wrapper') @backend = backend - # ugly hack to prevent this code from trying to run on the - # general peer grading page - if( @prompt_wrapper.length == 0) - return # get the location of the problem @location = $('.peer-grading').data('location') + # prevent this code from trying to run + # when we don't have a location + if(!@location) + return # get the other elements we want to fill in @submission_container = $('.submission-container') @@ -182,6 +199,8 @@ class PeerGradingProblem # Callbacks for various events # ########## + + # called after we perform an is_student_calibrated check calibration_check_callback: (response) => if response.success # if we haven't been calibrating before @@ -201,12 +220,17 @@ class PeerGradingProblem else @render_error("Error contacting the grading service") + + # called after we submit a calibration score calibration_callback: (response) => if response.success @render_calibration_feedback(response) else if response.error @render_error(response.error) + else + @render_error("Error saving calibration score") + # called after we submit a submission score submission_callback: (response) => if response.success @is_calibrated_check() @@ -218,6 +242,7 @@ class PeerGradingProblem else @render_error("Error occurred while submitting grade") + # called after a grade is selected on the interface graded_callback: (event) => @grading_message.hide() @score = event.target.value @@ -242,6 +267,8 @@ class PeerGradingProblem @grading_panel.removeClass('current-state') # Display the right text + # both versions of the text are written into the template itself + # we only need to show/hide the correct ones at the correct time @calibration_panel.find('.calibration-text').show() @grading_panel.find('.calibration-text').show() @calibration_panel.find('.grading-text').hide() @@ -267,6 +294,8 @@ class PeerGradingProblem @grading_panel.addClass('current-state') # Display the correct text + # both versions of the text are written into the template itself + # we only need to show/hide the correct ones at the correct time @calibration_panel.find('.calibration-text').hide() @grading_panel.find('.calibration-text').hide() @calibration_panel.find('.grading-text').show() @@ -287,6 +316,7 @@ class PeerGradingProblem new_text += "

#{paragraph}

" return new_text + # render common information between calibration and grading render_submission_data: (response) => @content_panel.show() @@ -304,7 +334,6 @@ class PeerGradingProblem render_calibration_feedback: (response) => # display correct grade - #@grading_wrapper.hide() @calibration_feedback_panel.slideDown() calibration_wrapper = $('.calibration-feedback-wrapper') calibration_wrapper.html("

The score you gave was: #{@score}. The actual score is: #{response.actual_score}

") @@ -316,7 +345,7 @@ class PeerGradingProblem if score == actual_score calibration_wrapper.append("

Congratulations! Your score matches the actual score!

") else - calibration_wrapper.append("

Please try to understand the grading critera better so that you will be more accurate next time.

") + calibration_wrapper.append("

Please try to understand the grading critera better to be more accurate next time.

") # disable score selection and submission from the grading interface $("input[name='score-selection']").attr('disabled', true) @@ -325,6 +354,7 @@ class PeerGradingProblem render_interstitial_page: () => @content_panel.hide() @interstitial_page.show() + render_error: (error_message) => @error_container.show() @calibration_feedback_panel.hide()