From 9a6ae8f7403bfe310e9e1334850fb3d4f9042392 Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Fri, 20 Sep 2013 13:53:09 -0400 Subject: [PATCH] Clean up open ended problems view and fix error. --- lms/djangoapps/open_ended_grading/tests.py | 115 +++++++++- lms/djangoapps/open_ended_grading/utils.py | 147 ++++++++++++ lms/djangoapps/open_ended_grading/views.py | 212 ++++++------------ .../open_ended_problems.html | 56 +++-- 4 files changed, 350 insertions(+), 180 deletions(-) diff --git a/lms/djangoapps/open_ended_grading/tests.py b/lms/djangoapps/open_ended_grading/tests.py index 5a0d9e69e5..5224386664 100644 --- a/lms/djangoapps/open_ended_grading/tests.py +++ b/lms/djangoapps/open_ended_grading/tests.py @@ -18,8 +18,9 @@ from xmodule.modulestore.django import modulestore from xmodule.x_module import ModuleSystem from xblock.fields import ScopeIds -from open_ended_grading import staff_grading_service, views +from open_ended_grading import staff_grading_service, views, utils from courseware.access import _course_staff_group_name +from student.models import unique_id_for_user import logging @@ -46,6 +47,57 @@ class EmptyStaffGradingService(object): """ return json.dumps({'success': True, 'error': 'No problems found.'}) +def make_instructor(course, user_email): + """ + Makes a given user an instructor in a course. + """ + group_name = _course_staff_group_name(course.location) + group = Group.objects.create(name=group_name) + group.user_set.add(User.objects.get(email=user_email)) + +class StudentProblemListMockQuery(object): + """ + Mock controller query service for testing student problem list functionality. + """ + def get_grading_status_list(self, *args, **kwargs): + """ + Get a mock grading status list with locations from the open_ended test course. + @returns: json formatted grading status message. + """ + grading_status_list = json.dumps( + { + "version": 1, + "problem_list": [ + { + "problem_name": "Test1", + "grader_type": "IN", + "eta_available": True, + "state": "Finished", + "eta": 259200, + "location": "i4x://edX/open_ended/combinedopenended/SampleQuestion1Attempt" + }, + { + "problem_name": "Test2", + "grader_type": "NA", + "eta_available": True, + "state": "Waiting to be Graded", + "eta": 259200, + "location": "i4x://edX/open_ended/combinedopenended/SampleQuestion" + }, + { + "problem_name": "Test3", + "grader_type": "PE", + "eta_available": True, + "state": "Waiting to be Graded", + "eta": 259200, + "location": "i4x://edX/open_ended/combinedopenended/SampleQuestion454" + }, + ], + "success": True + } + ) + return grading_status_list + @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) class TestStaffGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase): ''' @@ -67,12 +119,7 @@ class TestStaffGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase): 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) - group = Group.objects.create(name=group_name) - group.user_set.add(User.objects.get(email=self.instructor)) - - make_instructor(self.toy) + make_instructor(self.toy, self.instructor) self.mock_service = staff_grading_service.staff_grading_service() @@ -324,7 +371,7 @@ class TestPeerGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase): @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) -class TestPanel(ModuleStoreTestCase, LoginEnrollmentTestCase): +class TestPanel(ModuleStoreTestCase): """ Run tests on the open ended panel """ @@ -343,7 +390,15 @@ class TestPanel(ModuleStoreTestCase, LoginEnrollmentTestCase): found_module, peer_grading_module = views.find_peer_grading_module(self.course) self.assertTrue(found_module) - @patch('open_ended_grading.views.controller_qs', controller_query_service.MockControllerQueryService(settings.OPEN_ENDED_GRADING_INTERFACE, views.system)) + @patch( + 'open_ended_grading.utils.create_controller_query_service', + Mock( + return_value=controller_query_service.MockControllerQueryService( + settings.OPEN_ENDED_GRADING_INTERFACE, + utils.system + ) + ) + ) def test_problem_list(self): """ Ensure that the problem list from the grading controller server can be rendered properly locally @@ -370,4 +425,44 @@ class TestPeerGradingFound(ModuleStoreTestCase): """ found, url = views.find_peer_grading_module(self.course) - self.assertEqual(found, False) \ No newline at end of file + self.assertEqual(found, False) + +@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) +class TestStudentProblemList(ModuleStoreTestCase): + """ + Test if the student problem list correctly fetches and parses problems. + """ + + def setUp(self): + # Load an open ended course with several problems. + self.course_name = 'edX/open_ended/2012_Fall' + self.course = modulestore().get_course(self.course_name) + self.user = factories.UserFactory() + # Enroll our user in our course and make them an instructor. + make_instructor(self.course, self.user.email) + + @patch( + 'open_ended_grading.utils.create_controller_query_service', + Mock(return_value=StudentProblemListMockQuery()) + ) + def test_get_problem_list(self): + """ + Test to see if the StudentProblemList class can get and parse a problem list from ORA. + Mock the get_grading_status_list function using StudentProblemListMockQuery. + """ + # Initialize a StudentProblemList object. + student_problem_list = utils.StudentProblemList(self.course.id, unique_id_for_user(self.user)) + # Get the initial problem list from ORA. + success = student_problem_list.fetch_from_grading_service() + # Should be successful, and we should have three problems. See mock class for details. + self.assertTrue(success) + self.assertEqual(len(student_problem_list.problem_list), 3) + + # See if the problem locations are valid. + valid_problems = student_problem_list.add_problem_data(reverse('courses')) + # One location is invalid, so we should now have two. + self.assertEqual(len(valid_problems), 2) + # Ensure that human names are being set properly. + self.assertEqual(valid_problems[0]['grader_type_display_name'], "Instructor Assessment") + + diff --git a/lms/djangoapps/open_ended_grading/utils.py b/lms/djangoapps/open_ended_grading/utils.py index 15aaf0246f..1f860b1a6d 100644 --- a/lms/djangoapps/open_ended_grading/utils.py +++ b/lms/djangoapps/open_ended_grading/utils.py @@ -1,10 +1,58 @@ +import json + from xmodule.modulestore import search from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError, NoPathToItem +from xmodule.x_module import ModuleSystem +from xmodule.open_ended_grading_classes.controller_query_service import ControllerQueryService +from xmodule.open_ended_grading_classes.grading_service_module import GradingServiceError + +from django.utils.translation import ugettext as _ +from django.conf import settings + +from mitxmako.shortcuts import render_to_string + +from xblock.field_data import DictFieldData + import logging log = logging.getLogger(__name__) +GRADER_DISPLAY_NAMES = { + 'ML': _("AI Assessment"), + 'PE': _("Peer Assessment"), + 'NA': _("Not yet available"), + 'BC': _("Automatic Checker"), + 'IN': _("Instructor Assessment"), + } + +STUDENT_ERROR_MESSAGE = _("Error occurred while contacting the grading service. Please notify course staff.") +STAFF_ERROR_MESSAGE = _("Error occurred while contacting the grading service. Please notify your edX point of contact.") + +system = ModuleSystem( + ajax_url=None, + track_function=None, + get_module=None, + render_template=render_to_string, + replace_urls=None, + xmodule_field_data=DictFieldData({}), + ) + +def generate_problem_url(problem_url_parts, base_course_url): + """ + From a list of problem url parts generated by search.path_to_location and a base course url, generates a url to a problem + @param problem_url_parts: Output of search.path_to_location + @param base_course_url: Base url of a given course + @return: A path to the problem + """ + problem_url = base_course_url + "/" + for i, part in enumerate(problem_url_parts): + if part is not None: + if i == 1: + problem_url += "courseware/" + problem_url += part + "/" + return problem_url + def does_location_exist(course_id, location): """ Checks to see if a valid module exists at a given location (ie has not been deleted) @@ -25,3 +73,102 @@ def does_location_exist(course_id, location): log.warn(("Got an unexpected NoPathToItem error in staff grading with a non-draft location {0}. " "Ensure that the location is valid.").format(location)) return False + +def create_controller_query_service(): + """ + Return an instance of a service that can query edX ORA. + """ + + return ControllerQueryService(settings.OPEN_ENDED_GRADING_INTERFACE, system) + +class StudentProblemList(object): + """ + Get a list of problems that the student has attempted from ORA. + Add in metadata as needed. + """ + def __init__(self, course_id, user_id): + """ + @param course_id: The id of a course object. Get using course.id. + @param user_id: The anonymous id of the user, from the unique_id_for_user function. + """ + self.course_id = course_id + self.user_id = user_id + + # We want to append this string to all of our error messages. + self.course_error_ending = _("for course {0} and student {1}.").format(self.course_id, user_id) + + # This is our generic error message. + self.error_text = STUDENT_ERROR_MESSAGE + self.success = False + + # Create a service to query edX ORA. + self.controller_qs = create_controller_query_service() + + def fetch_from_grading_service(self): + """ + Fetch a list of problems that the student has answered from ORA. + Handle various error conditions. + @return: A boolean success indicator. + """ + # In the case of multiple calls, ensure that success is false initially. + self.success = False + try: + #Get list of all open ended problems that the grading server knows about + problem_list_json = self.controller_qs.get_grading_status_list(self.course_id, self.user_id) + except GradingServiceError: + log.error("Problem contacting open ended grading service " + self.course_error_ending) + return self.success + try: + problem_list_dict = json.loads(problem_list_json) + except ValueError: + log.error("Problem with results from external grading service for open ended" + self.course_error_ending) + return self.success + + success = problem_list_dict['success'] + if 'error' in problem_list_dict: + self.error_text = problem_list_dict['error'] + return success + if 'problem_list' not in problem_list_dict: + log.error("Did not receive a problem list in ORA response" + self.course_error_ending) + return success + + self.problem_list = problem_list_dict['problem_list'] + + self.success = True + return self.success + + def add_problem_data(self, base_course_url): + """ + Add metadata to problems. + @param base_course_url: the base url for any course. Can get with reverse('course') + @return: A list of valid problems in the course and their appended data. + """ + # Our list of valid problems. + valid_problems = [] + + if not self.success or not isinstance(self.problem_list, list): + log.error("Called add_problem_data without a valid problem list" + self.course_error_ending) + return valid_problems + + # Iterate through all of our problems and add data. + for problem in self.problem_list: + try: + # Try to load the problem. + problem_url_parts = search.path_to_location(modulestore(), self.course_id, problem['location']) + except (ItemNotFoundError, NoPathToItem): + # If the problem cannot be found at the location received from the grading controller server, + # it has been deleted by the course author. We should not display it. + error_message = "Could not find module for course {0} at location {1}".format(self.course_id, + problem['location']) + log.error(error_message) + continue + + # Get the problem url in the courseware. + problem_url = generate_problem_url(problem_url_parts, base_course_url) + + # Map the grader name from ORA to a human readable version. + grader_type_display_name = GRADER_DISPLAY_NAMES.get(problem['grader_type'], "edX Assessment") + problem['actual_url'] = problem_url + problem['grader_type_display_name'] = grader_type_display_name + valid_problems.append(problem) + return valid_problems diff --git a/lms/djangoapps/open_ended_grading/views.py b/lms/djangoapps/open_ended_grading/views.py index dcc1e11730..e8002e0883 100644 --- a/lms/djangoapps/open_ended_grading/views.py +++ b/lms/djangoapps/open_ended_grading/views.py @@ -1,5 +1,3 @@ -# Grading Views - import logging from django.conf import settings @@ -7,13 +5,9 @@ from django.views.decorators.cache import cache_control from mitxmako.shortcuts import render_to_response from django.core.urlresolvers import reverse -from xblock.field_data import DictFieldData - from student.models import unique_id_for_user from courseware.courses import get_course_with_access -from xmodule.x_module import ModuleSystem -from xmodule.open_ended_grading_classes.controller_query_service import ControllerQueryService, convert_seconds_to_human_readable from xmodule.open_ended_grading_classes.grading_service_module import GradingServiceError import json from student.models import unique_id_for_user @@ -26,28 +20,21 @@ from xmodule.modulestore.exceptions import ItemNotFoundError, NoPathToItem from django.http import HttpResponse, Http404, HttpResponseRedirect from mitxmako.shortcuts import render_to_string +from django.utils.translation import ugettext as _ + +from open_ended_grading.utils import (STAFF_ERROR_MESSAGE, STUDENT_ERROR_MESSAGE, + StudentProblemList, generate_problem_url, create_controller_query_service) log = logging.getLogger(__name__) -system = ModuleSystem( - ajax_url=None, - track_function=None, - get_module=None, - render_template=render_to_string, - replace_urls=None, - xmodule_field_data=DictFieldData({}), -) - -controller_qs = ControllerQueryService(settings.OPEN_ENDED_GRADING_INTERFACE, system) - -""" -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): + """ + Reverses the URL given the name and the course id, and then adds a trailing slash if + it does not exist yet. + @param url_name: The name of the url (eg 'staff_grading'). + @param course_id: The id of the course object (eg course.id). + @returns: The reversed url with a trailing slash. + """ ajax_url = _reverse_without_slash(url_name, course_id) if not ajax_url.endswith('/'): ajax_url += '/' @@ -60,22 +47,19 @@ def _reverse_without_slash(url_name, course_id): DESCRIPTION_DICT = { - 'Peer Grading': "View all problems that require peer assessment in this particular course.", - 'Staff Grading': "View ungraded submissions submitted by students for the open ended problems in the course.", - 'Problems you have submitted': "View open ended problems that you have previously submitted for grading.", - 'Flagged Submissions': "View submissions that have been flagged by students as inappropriate." + 'Peer Grading': _("View all problems that require peer assessment in this particular course."), + 'Staff Grading': _("View ungraded submissions submitted by students for the open ended problems in the course."), + 'Problems you have submitted': _("View open ended problems that you have previously submitted for grading."), + 'Flagged Submissions': _("View submissions that have been flagged by students as inappropriate."), } + ALERT_DICT = { - 'Peer Grading': "New submissions to grade", - 'Staff Grading': "New submissions to grade", - 'Problems you have submitted': "New grades have been returned", - 'Flagged Submissions': "Submissions have been flagged for review" + 'Peer Grading': _("New submissions to grade"), + 'Staff Grading': _("New submissions to grade"), + 'Problems you have submitted': _("New grades have been returned"), + 'Flagged Submissions': _("Submissions have been flagged for review"), } -STUDENT_ERROR_MESSAGE = "Error occurred while contacting the grading service. Please notify course staff." -STAFF_ERROR_MESSAGE = "Error occurred while contacting the grading service. Please notify the development team. If you do not have a point of contact, please email Vik at vik@edx.org" - - @cache_control(no_cache=True, no_store=True, must_revalidate=True) def staff_grading(request, course_id): """ @@ -107,8 +91,6 @@ def find_peer_grading_module(course): # Get the course id and split it. course_id_parts = course.id.split("/") - log.info("COURSE ID PARTS") - log.info(course_id_parts) # Get the peer grading modules currently in the course. Explicitly specify the course id to avoid issues with different runs. items = modulestore().get_items(['i4x', course_id_parts[0], course_id_parts[1], 'peergrading', None], course_id=course.id) @@ -123,7 +105,7 @@ def find_peer_grading_module(course): except NoPathToItem: # In the case of nopathtoitem, the peer grading module that was found is in an invalid state, and # can no longer be accessed. Log an informational message, but this will not impact normal behavior. - log.info("Invalid peer grading module location {0} in course {1}. This module may need to be removed.".format(item_location, course.id)) + log.info(u"Invalid peer grading module location {0} in course {1}. This module may need to be removed.".format(item_location, course.id)) continue problem_url = generate_problem_url(problem_url_parts, base_course_url) found_module = True @@ -143,121 +125,60 @@ def peer_grading(request, course_id): found_module, problem_url = find_peer_grading_module(course) if not found_module: - #This is a student_facing_error - error_message = """ + error_message = _(""" Error with initializing peer grading. There has not been a peer grading module created in the courseware that would allow you to grade others. Please check back later for this. - """ - #This is a dev_facing_error - log.exception(error_message + "Current course is: {0}".format(course_id)) + """) + log.exception(error_message + u"Current course is: {0}".format(course_id)) return HttpResponse(error_message) return HttpResponseRedirect(problem_url) - -def generate_problem_url(problem_url_parts, base_course_url): - """ - From a list of problem url parts generated by search.path_to_location and a base course url, generates a url to a problem - @param problem_url_parts: Output of search.path_to_location - @param base_course_url: Base url of a given course - @return: A path to the problem - """ - problem_url = base_course_url + "/" - for z in xrange(0, len(problem_url_parts)): - part = problem_url_parts[z] - if part is not None: - if z == 1: - problem_url += "courseware/" - problem_url += part + "/" - return problem_url - - @cache_control(no_cache=True, no_store=True, must_revalidate=True) def student_problem_list(request, course_id): - ''' - Show a student problem list to a student. Fetch the list from the grading controller server, get some metadata, - and then show it to the student. - ''' + """ + Show a list of problems they have attempted to a student. + Fetch the list from the grading controller server and append some data. + @param request: The request object for this view. + @param course_id: The id of the course to get the problem list for. + @return: Renders an HTML problem list table. + """ + + # Load the course. Don't catch any errors here, as we want them to be loud. course = get_course_with_access(request.user, course_id, 'load') + + # The anonymous student id is needed for communication with ORA. student_id = unique_id_for_user(request.user) - - # call problem list service - success = False - error_text = "" - problem_list = [] base_course_url = reverse('courses') - list_to_remove = [] + error_text = "" - try: - #Get list of all open ended problems that the grading server knows about - problem_list_json = controller_qs.get_grading_status_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 = [] - else: - problem_list = problem_list_dict['problem_list'] + student_problem_list = StudentProblemList(course_id, student_id) + # Get the problem list from ORA. + success = student_problem_list.fetch_from_grading_service() + # If we fetched the problem list properly, add in additional problem data. + if success: + # Add in links to problems. + valid_problems = student_problem_list.add_problem_data(base_course_url) + else: + # Get an error message to show to the student. + valid_problems = [] + error_text = student_problem_list.error_text - #A list of problems to remove (problems that can't be found in the course) - for i in xrange(0, len(problem_list)): - try: - #Try to load each problem in the courseware to get links to them - problem_url_parts = search.path_to_location(modulestore(), course.id, problem_list[i]['location']) - except ItemNotFoundError: - #If the problem cannot be found at the location received from the grading controller server, it has been deleted by the course author. - #Continue with the rest of the location to construct the list - error_message = "Could not find module for course {0} at location {1}".format(course.id, - problem_list[i][ - 'location']) - log.error(error_message) - #Mark the problem for removal from the list - list_to_remove.append(i) - continue - problem_url = generate_problem_url(problem_url_parts, base_course_url) - problem_list[i].update({'actual_url': problem_url}) - eta_available = problem_list[i]['eta_available'] - if isinstance(eta_available, basestring): - eta_available = (eta_available.lower() == "true") - - eta_string = "N/A" - if eta_available: - try: - eta_string = convert_seconds_to_human_readable(int(problem_list[i]['eta'])) - except: - #This is a student_facing_error - eta_string = "Error getting ETA." - problem_list[i].update({'eta_string': eta_string}) - - except GradingServiceError: - #This is a student_facing_error - error_text = STUDENT_ERROR_MESSAGE - #This is a dev facing error - log.error("Problem contacting open ended grading service.") - success = False - # catch error if if the json loads fails - except ValueError: - #This is a student facing error - error_text = STUDENT_ERROR_MESSAGE - #This is a dev_facing_error - log.error("Problem with results from external grading service for open ended.") - success = False - - #Remove problems that cannot be found in the courseware from the list - problem_list = [problem_list[i] for i in xrange(0, len(problem_list)) if i not in list_to_remove] ajax_url = _reverse_with_slash('open_ended_problems', course_id) - return render_to_response('open_ended_problems/open_ended_problems.html', { + context = { 'course': course, 'course_id': course_id, 'ajax_url': ajax_url, 'success': success, - 'problem_list': problem_list, + 'problem_list': valid_problems, 'error_text': error_text, # Checked above - 'staff_access': False, }) + 'staff_access': False, + } + return render_to_response('open_ended_problems/open_ended_problems.html', context) @cache_control(no_cache=True, no_store=True, must_revalidate=True) def flagged_problem_list(request, course_id): @@ -273,6 +194,8 @@ def flagged_problem_list(request, course_id): problem_list = [] base_course_url = reverse('courses') + # Make a service that can query edX ORA. + controller_qs = create_controller_query_service() try: problem_list_json = controller_qs.get_flagged_problem_list(course_id) problem_list_dict = json.loads(problem_list_json) @@ -384,11 +307,12 @@ def take_action_on_flags(request, course_id): required = ['submission_id', 'action_type', 'student_id'] for key in required: if key not in request.POST: - #This is a staff_facing_error - return HttpResponse(json.dumps({'success': False, - 'error': STAFF_ERROR_MESSAGE + 'Missing key {0} from submission. Please reload and try again.'.format( - key)}), - mimetype="application/json") + error_message = u'Missing key {0} from submission. Please reload and try again.'.format(key) + response = { + 'success': False, + 'error': STAFF_ERROR_MESSAGE + error_message + } + return HttpResponse(json.dumps(response), mimetype="application/json") p = request.POST submission_id = p['submission_id'] @@ -397,12 +321,20 @@ def take_action_on_flags(request, course_id): student_id = student_id.strip(' \t\n\r') submission_id = submission_id.strip(' \t\n\r') action_type = action_type.lower().strip(' \t\n\r') + + # Make a service that can query edX ORA. + controller_qs = create_controller_query_service() try: response = controller_qs.take_action_on_flags(course_id, student_id, submission_id, action_type) return HttpResponse(response, mimetype="application/json") except GradingServiceError: - #This is a dev_facing_error log.exception( - "Error taking action on flagged peer grading submissions, submission_id: {0}, action_type: {1}, grader_id: {2}".format( - submission_id, action_type, grader_id)) - return _err_response(STAFF_ERROR_MESSAGE) + u"Error taking action on flagged peer grading submissions, " + u"submission_id: {0}, action_type: {1}, grader_id: {2}".format( + submission_id, action_type, student_id) + ) + response = { + 'success': False, + 'error': STAFF_ERROR_MESSAGE + } + return HttpResponse(json.dumps(response),mimetype="application/json") diff --git a/lms/templates/open_ended_problems/open_ended_problems.html b/lms/templates/open_ended_problems/open_ended_problems.html index aa053111d8..1c7e2657ed 100644 --- a/lms/templates/open_ended_problems/open_ended_problems.html +++ b/lms/templates/open_ended_problems/open_ended_problems.html @@ -19,36 +19,32 @@

${_("Instructions")}

${_("Here is a list of open ended problems for this course.")}

% if success: - % if len(problem_list) == 0: -
- ${_("You have not attempted any open ended problems yet.")} -
- %else: - - - - - - - - %for problem in problem_list: - - - - - - - %endfor -
${_("Problem Name")}${_("Status")}${_("Grader Type")}${_("ETA")}
- ${problem['problem_name']} - - ${problem['state']} - - ${problem['grader_type']} - - ${problem['eta_string']} -
- %endif + % if len(problem_list) == 0: +
+ ${_("You have not attempted any open ended problems yet.")} +
+ %else: + + + + + + + %for problem in problem_list: + + + + + + %endfor +
${_("Problem Name")}${_("Status")}${_("Grader Type")}
+ ${problem['problem_name']} + + ${problem['state']} + + ${problem['grader_type_display_name']} +
+ %endif %endif