Merge branch 'diana/peer-grading-views' into diana/peer-grading-improvements

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
         """

lms/djangoapps/open_ended_grading/peer_grading_service.py

+"""
+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')

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

lms/djangoapps/open_ended_grading/tests.py

 """
-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

lms/djangoapps/open_ended_grading/views.py

 # 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': '',

lms/static/coffee/src/peer_grading/peer_grading.coffee

+# 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())

lms/static/coffee/src/peer_grading/peer_grading_problem.coffee

+##################################
+#
+#  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 += "<p>#{paragraph}</p>"
     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("<p>The score you gave was: #{@score}. The actual score is: #{response.actual_score}</p>")
@@ -316,7 +345,7 @@ class PeerGradingProblem
     if score == actual_score
       calibration_wrapper.append("<p>Congratulations! Your score matches the actual score!</p>")
     else
-      calibration_wrapper.append("<p>Please try to understand the grading critera better so that you will be more accurate next time.</p>") 
+      calibration_wrapper.append("<p>Please try to understand the grading critera better to be more accurate next time.</p>") 
 
     # 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()