From b222e66f45363a52660ce60f364c3da3d6d5e129 Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Sun, 8 Sep 2013 13:54:40 -0400 Subject: [PATCH 1/5] Fix nopathtoitem error in peer grading --- lms/djangoapps/open_ended_grading/views.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/open_ended_grading/views.py b/lms/djangoapps/open_ended_grading/views.py index 4c299db38d..e060f4c131 100644 --- a/lms/djangoapps/open_ended_grading/views.py +++ b/lms/djangoapps/open_ended_grading/views.py @@ -20,7 +20,7 @@ import open_ended_notifications from xmodule.modulestore.django import modulestore from xmodule.modulestore import search -from xmodule.modulestore.exceptions import ItemNotFoundError +from xmodule.modulestore.exceptions import ItemNotFoundError, NoPathToItem from django.http import HttpResponse, Http404, HttpResponseRedirect from mitxmako.shortcuts import render_to_string @@ -112,10 +112,14 @@ def find_peer_grading_module(course): #See if any of the modules are centralized modules (ie display info from multiple problems) items = [i for i in items if not getattr(i, "use_for_single_location", True)] #Get the first one - if len(items) > 0: - item_location = items[0].location + for item in items: + item_location = item.location #Generate a url for the first module and redirect the user to it - problem_url_parts = search.path_to_location(modulestore(), course.id, item_location) + try: + problem_url_parts = search.path_to_location(modulestore(), course.id, item_location) + except NoPathToItem: + log.info("Invalid peer grading module location {0} in course {1}.".format(item_location, course.id)) + continue problem_url = generate_problem_url(problem_url_parts, base_course_url) found_module = True From 009017fe077955156e7e4788a465d7d40cb45a73 Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Thu, 12 Sep 2013 11:41:14 -0400 Subject: [PATCH 2/5] Address review comments --- common/test/data/open_ended_nopath/README.md | 1 + common/test/data/open_ended_nopath/course.xml | 1 + .../open_ended_nopath/course/2012_Fall.xml | 4 ++++ .../peergrading/PeerGradingNoPath.xml | 1 + .../open_ended_nopath/policies/2012_Fall.json | 11 +++++++++++ .../courseware/tests/modulestore_config.py | 1 + lms/djangoapps/open_ended_grading/tests.py | 19 +++++++++++++++++++ lms/djangoapps/open_ended_grading/views.py | 15 +++++++++------ 8 files changed, 47 insertions(+), 6 deletions(-) create mode 100644 common/test/data/open_ended_nopath/README.md create mode 100644 common/test/data/open_ended_nopath/course.xml create mode 100644 common/test/data/open_ended_nopath/course/2012_Fall.xml create mode 100644 common/test/data/open_ended_nopath/peergrading/PeerGradingNoPath.xml create mode 100644 common/test/data/open_ended_nopath/policies/2012_Fall.json diff --git a/common/test/data/open_ended_nopath/README.md b/common/test/data/open_ended_nopath/README.md new file mode 100644 index 0000000000..1e57b483bc --- /dev/null +++ b/common/test/data/open_ended_nopath/README.md @@ -0,0 +1 @@ +This is a very very simple course, useful for debugging open ended grading code. This is specifically for testing if a peer grading module with no path to it in the course will be handled properly. diff --git a/common/test/data/open_ended_nopath/course.xml b/common/test/data/open_ended_nopath/course.xml new file mode 100644 index 0000000000..d60df1a471 --- /dev/null +++ b/common/test/data/open_ended_nopath/course.xml @@ -0,0 +1 @@ + diff --git a/common/test/data/open_ended_nopath/course/2012_Fall.xml b/common/test/data/open_ended_nopath/course/2012_Fall.xml new file mode 100644 index 0000000000..2535b4f462 --- /dev/null +++ b/common/test/data/open_ended_nopath/course/2012_Fall.xml @@ -0,0 +1,4 @@ + + + + diff --git a/common/test/data/open_ended_nopath/peergrading/PeerGradingNoPath.xml b/common/test/data/open_ended_nopath/peergrading/PeerGradingNoPath.xml new file mode 100644 index 0000000000..7e3afddf3a --- /dev/null +++ b/common/test/data/open_ended_nopath/peergrading/PeerGradingNoPath.xml @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/common/test/data/open_ended_nopath/policies/2012_Fall.json b/common/test/data/open_ended_nopath/policies/2012_Fall.json new file mode 100644 index 0000000000..321cdf21df --- /dev/null +++ b/common/test/data/open_ended_nopath/policies/2012_Fall.json @@ -0,0 +1,11 @@ +{ + "course/2012_Fall": { + "graceperiod": "2 days 5 hours 59 minutes 59 seconds", + "start": "2015-07-17T12:00", + "display_name": "Self Assessment Test", + "graded": "true" + }, + "chapter/Overview": { + "display_name": "Overview" + } +} diff --git a/lms/djangoapps/courseware/tests/modulestore_config.py b/lms/djangoapps/courseware/tests/modulestore_config.py index 74fd3da57f..681f3fd4d1 100644 --- a/lms/djangoapps/courseware/tests/modulestore_config.py +++ b/lms/djangoapps/courseware/tests/modulestore_config.py @@ -22,5 +22,6 @@ MAPPINGS = { 'edX/test_about_blob_end_date/2012_Fall': 'xml', 'edX/graded/2012_Fall': 'xml', 'edX/open_ended/2012_Fall': 'xml', + 'edX/open_ended_nopath/2012_Fall': 'xml', } TEST_DATA_MIXED_MODULESTORE = mixed_store_config(TEST_DATA_DIR, MAPPINGS) diff --git a/lms/djangoapps/open_ended_grading/tests.py b/lms/djangoapps/open_ended_grading/tests.py index 27e6fd6059..7ea81f4f3f 100644 --- a/lms/djangoapps/open_ended_grading/tests.py +++ b/lms/djangoapps/open_ended_grading/tests.py @@ -320,3 +320,22 @@ class TestPanel(ModuleStoreTestCase, LoginEnrollmentTestCase): request = Mock(user=self.user) response = views.student_problem_list(request, self.course.id) self.assertRegexpMatches(response.content, "Here are a list of open ended problems for this course.") + +@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) +class TestPeerGradingFound(ModuleStoreTestCase): + """ + Test to see if peer grading modules can be found properly. + """ + + def setUp(self): + self.course_name = 'edX/open_ended_nopath/2012_Fall' + self.course = modulestore().get_course(self.course_name) + + def test_peer_grading_nopath(self): + """ + The open_ended_nopath course contains a peer grading module with no path to it. + Ensure that the exception is caught. + """ + + found, url = views.find_peer_grading_module(self.course) + self.assertEqual(found, False) \ No newline at end of file diff --git a/lms/djangoapps/open_ended_grading/views.py b/lms/djangoapps/open_ended_grading/views.py index e060f4c131..f5a2471226 100644 --- a/lms/djangoapps/open_ended_grading/views.py +++ b/lms/djangoapps/open_ended_grading/views.py @@ -97,28 +97,31 @@ def find_peer_grading_module(course): @param course: A course object. @return: boolean found_module, string problem_url """ - #Reverse the base course url + + # Reverse the base course url. base_course_url = reverse('courses') found_module = False problem_url = "" - #Get the course id and split it + # 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. + # 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) #See if any of the modules are centralized modules (ie display info from multiple problems) items = [i for i in items if not getattr(i, "use_for_single_location", True)] - #Get the first one + # Loop through all potential peer grading modules, and find the first one that has a path to it. for item in items: item_location = item.location - #Generate a url for the first module and redirect the user to it + # Generate a url for the first module and redirect the user to it. try: problem_url_parts = search.path_to_location(modulestore(), course.id, item_location) except NoPathToItem: - log.info("Invalid peer grading module location {0} in course {1}.".format(item_location, course.id)) + # 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)) continue problem_url = generate_problem_url(problem_url_parts, base_course_url) found_module = True From 640b7fe6b5649b3da794f8907513598fcc770e46 Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Thu, 12 Sep 2013 15:17:04 -0400 Subject: [PATCH 3/5] Fix problem with peer grading list not rendering, test --- .../peer_grading_service.py | 12 ++++----- .../xmodule/xmodule/peer_grading_module.py | 26 ++++++++++++------- .../xmodule/tests/test_peer_grading.py | 26 +++++++++++++++++++ 3 files changed, 49 insertions(+), 15 deletions(-) diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/peer_grading_service.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/peer_grading_service.py index 0e5c9cdda1..ecac9a8a00 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/peer_grading_service.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/peer_grading_service.py @@ -94,9 +94,9 @@ class MockPeerGradingService(object): 'success': True, 'submission_id': 1, 'submission_key': "", - 'student_response': 'fake student response', - 'prompt': 'fake submission prompt', - 'rubric': 'fake rubric', + 'student_response': 'Sample student response.', + 'prompt': 'Sample submission prompt.', + 'rubric': 'Placeholder text for the full rubric.', 'max_score': 4 } @@ -110,9 +110,9 @@ class MockPeerGradingService(object): return {'success': True, 'submission_id': 1, 'submission_key': '', - 'student_response': 'fake student response', - 'prompt': 'fake submission prompt', - 'rubric': 'fake rubric', + 'student_response': 'Sample student response.', + 'prompt': 'Sample submission prompt.', + 'rubric': 'Placeholder text for the full rubric.', 'max_score': 4} def save_calibration_essay(self, **kwargs): diff --git a/common/lib/xmodule/xmodule/peer_grading_module.py b/common/lib/xmodule/xmodule/peer_grading_module.py index 3a4bbdb9f6..a40929a258 100644 --- a/common/lib/xmodule/xmodule/peer_grading_module.py +++ b/common/lib/xmodule/xmodule/peer_grading_module.py @@ -8,7 +8,7 @@ from pkg_resources import resource_string from .capa_module import ComplexEncoder from .x_module import XModule from xmodule.raw_module import RawDescriptor -from xmodule.modulestore.exceptions import ItemNotFoundError +from xmodule.modulestore.exceptions import ItemNotFoundError, NoPathToItem from .timeinfo import TimeInfo from xblock.fields import Dict, String, Scope, Boolean, Float from xmodule.fields import Date, Timedelta @@ -108,6 +108,10 @@ class PeerGradingModule(PeerGradingFields, XModule): log.error("Linked location {0} for peer grading module {1} does not exist".format( self.link_to_location, self.location)) raise + except NoPathToItem: + log.error("Linked location {0} for peer grading module {1} cannot be linked to.".format( + self.link_to_location, self.location)) + raise due_date = self.linked_problem.due if due_date: self.due = due_date @@ -514,14 +518,18 @@ class PeerGradingModule(PeerGradingFields, XModule): def _find_corresponding_module_for_location(location): - ''' - find the peer grading module that links to the given location - ''' + """ + Find the peer grading module that links to the given location. + """ try: - return modulestore().get_instance(self.system.course_id, location) - except Exception: - # the linked problem doesn't exist - log.error("Problem {0} does not exist in this course".format(location)) + return self.descriptor.system.load_item(location) + except ItemNotFoundError: + # The linked problem doesn't exist. + log.error("Problem {0} does not exist in this course.".format(location)) + raise + except NoPathToItem: + # The linked problem doesn't exist. + log.error("Cannot find a path to problem {0} in this course.".format(location)) raise good_problem_list = [] @@ -529,7 +537,7 @@ class PeerGradingModule(PeerGradingFields, XModule): problem_location = problem['location'] try: descriptor = _find_corresponding_module_for_location(problem_location) - except Exception: + except (NoPathToItem, ItemNotFoundError): continue if descriptor: problem['due'] = descriptor.due diff --git a/common/lib/xmodule/xmodule/tests/test_peer_grading.py b/common/lib/xmodule/xmodule/tests/test_peer_grading.py index 240fef4e87..a81050b306 100644 --- a/common/lib/xmodule/xmodule/tests/test_peer_grading.py +++ b/common/lib/xmodule/xmodule/tests/test_peer_grading.py @@ -2,6 +2,8 @@ import unittest from xmodule.modulestore import Location from .import get_test_system from test_util_open_ended import MockQueryDict, DummyModulestore +from xmodule.open_ended_grading_classes.peer_grading_service import MockPeerGradingService +import json import logging @@ -136,6 +138,13 @@ class PeerGradingModuleTest(unittest.TestCase, DummyModulestore): """ self.peer_grading.get_instance_state() +class MockPeerGradingServiceProblemList(MockPeerGradingService): + def get_problem_list(self, course_id, grader_id): + return {'success': True, + 'problem_list': [ + {"num_graded": 3, "num_pending": 681, "num_required": 3, "location": "i4x://edX/open_ended/combinedopenended/SampleQuestion", "problem_name": "Peer-Graded Essay"}, + ]} + class PeerGradingModuleScoredTest(unittest.TestCase, DummyModulestore): """ Test peer grading xmodule at the unit level. More detailed tests are difficult, as the module relies on an @@ -155,3 +164,20 @@ class PeerGradingModuleScoredTest(unittest.TestCase, DummyModulestore): def test_metadata_load(self): peer_grading = self.get_module_from_location(self.problem_location, COURSE) self.assertEqual(peer_grading.closed(), False) + + def test_problem_list(self): + """ + Test to see if a peer grading problem list can be correctly initialized. + """ + + # Initialize peer grading module. + peer_grading = self.get_module_from_location(self.problem_location, COURSE) + + # Ensure that it cannot find any peer grading. + html = peer_grading.peer_grading() + self.assertNotRegexpMatches(html, "Peer-Graded") + + #Swap for our mock class, which will find peer grading. + peer_grading.peer_gs = MockPeerGradingServiceProblemList() + html = peer_grading.peer_grading() + self.assertRegexpMatches(html, "Peer-Graded") \ No newline at end of file From 00050a08a4beb26232ff5da023e4afbde0d9767a Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Fri, 13 Sep 2013 10:48:02 -0400 Subject: [PATCH 4/5] Address review comments --- common/lib/xmodule/xmodule/peer_grading_module.py | 4 ++-- common/lib/xmodule/xmodule/tests/test_peer_grading.py | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/common/lib/xmodule/xmodule/peer_grading_module.py b/common/lib/xmodule/xmodule/peer_grading_module.py index a40929a258..7597a2d655 100644 --- a/common/lib/xmodule/xmodule/peer_grading_module.py +++ b/common/lib/xmodule/xmodule/peer_grading_module.py @@ -519,7 +519,7 @@ class PeerGradingModule(PeerGradingFields, XModule): def _find_corresponding_module_for_location(location): """ - Find the peer grading module that links to the given location. + Find the peer grading module that exists at the given location. """ try: return self.descriptor.system.load_item(location) @@ -528,7 +528,7 @@ class PeerGradingModule(PeerGradingFields, XModule): log.error("Problem {0} does not exist in this course.".format(location)) raise except NoPathToItem: - # The linked problem doesn't exist. + # The linked problem does not have a path to it (ie is in a draft or other strange state). log.error("Cannot find a path to problem {0} in this course.".format(location)) raise diff --git a/common/lib/xmodule/xmodule/tests/test_peer_grading.py b/common/lib/xmodule/xmodule/tests/test_peer_grading.py index a81050b306..67ea3a2408 100644 --- a/common/lib/xmodule/xmodule/tests/test_peer_grading.py +++ b/common/lib/xmodule/xmodule/tests/test_peer_grading.py @@ -175,9 +175,9 @@ class PeerGradingModuleScoredTest(unittest.TestCase, DummyModulestore): # Ensure that it cannot find any peer grading. html = peer_grading.peer_grading() - self.assertNotRegexpMatches(html, "Peer-Graded") + self.assertNotIn("Peer-Graded", html) - #Swap for our mock class, which will find peer grading. + # Swap for our mock class, which will find peer grading. peer_grading.peer_gs = MockPeerGradingServiceProblemList() html = peer_grading.peer_grading() - self.assertRegexpMatches(html, "Peer-Graded") \ No newline at end of file + self.assertIn("Peer-Graded", html) \ No newline at end of file From c6e5e67483f96449f9557c424dfcfc9443466993 Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Fri, 13 Sep 2013 11:09:06 -0400 Subject: [PATCH 5/5] Fix linking problem for peer grading, add in a test Conflicts: common/lib/xmodule/xmodule/tests/test_peer_grading.py --- .../xmodule/xmodule/peer_grading_module.py | 14 ++++- .../xmodule/tests/test_peer_grading.py | 56 ++++++++++++++++++- .../test/data/open_ended/course/2012_Fall.xml | 1 + .../peergrading/PeerGradingLinked.xml | 1 + 4 files changed, 70 insertions(+), 2 deletions(-) create mode 100644 common/test/data/open_ended/peergrading/PeerGradingLinked.xml diff --git a/common/lib/xmodule/xmodule/peer_grading_module.py b/common/lib/xmodule/xmodule/peer_grading_module.py index 7597a2d655..b890270f5b 100644 --- a/common/lib/xmodule/xmodule/peer_grading_module.py +++ b/common/lib/xmodule/xmodule/peer_grading_module.py @@ -72,6 +72,12 @@ class PeerGradingFields(object): scope=Scope.content ) +class InvalidLinkLocation(Exception): + """ + Exception for the case in which a peer grading module tries to link to an invalid location. + """ + pass + class PeerGradingModule(PeerGradingFields, XModule): """ @@ -103,7 +109,13 @@ class PeerGradingModule(PeerGradingFields, XModule): if self.use_for_single_location: try: - self.linked_problem = self.system.get_module(self.link_to_location) + linked_descriptors = self.descriptor.get_required_module_descriptors() + if len(linked_descriptors) == 0: + error_msg = "Peer grading module {0} is trying to use single problem mode without " + "a location specified.".format(self.location) + log.error(error_msg) + raise InvalidLinkLocation(error_msg) + self.linked_problem = self.system.get_module(linked_descriptors[0]) except ItemNotFoundError: log.error("Linked location {0} for peer grading module {1} does not exist".format( self.link_to_location, self.location)) diff --git a/common/lib/xmodule/xmodule/tests/test_peer_grading.py b/common/lib/xmodule/xmodule/tests/test_peer_grading.py index 67ea3a2408..0197e36cf5 100644 --- a/common/lib/xmodule/xmodule/tests/test_peer_grading.py +++ b/common/lib/xmodule/xmodule/tests/test_peer_grading.py @@ -4,6 +4,10 @@ from .import get_test_system from test_util_open_ended import MockQueryDict, DummyModulestore from xmodule.open_ended_grading_classes.peer_grading_service import MockPeerGradingService import json +from mock import Mock +from xmodule.peer_grading_module import PeerGradingModule +from xblock.field_data import DictFieldData +from xblock.fields import ScopeIds import logging @@ -180,4 +184,54 @@ class PeerGradingModuleScoredTest(unittest.TestCase, DummyModulestore): # Swap for our mock class, which will find peer grading. peer_grading.peer_gs = MockPeerGradingServiceProblemList() html = peer_grading.peer_grading() - self.assertIn("Peer-Graded", html) \ No newline at end of file + self.assertIn("Peer-Graded", html) + +class PeerGradingModuleLinkedTest(unittest.TestCase, DummyModulestore): + """ + Test peer grading that is linked to an open ended module. + """ + problem_location = Location(["i4x", "edX", "open_ended", "peergrading", + "PeerGradingLinked"]) + coe_location = Location(["i4x", "edX", "open_ended", "combinedopenended", + "SampleQuestion"]) + + def setUp(self): + """ + Create a peer grading module from a test system. + """ + self.test_system = get_test_system() + self.test_system.open_ended_grading_interface = None + self.setup_modulestore(COURSE) + + def test_linked_problem(self): + """ + Check to see if a peer grading module with a linked problem loads properly. + """ + + # Mock the linked problem descriptor. + linked_descriptor = Mock() + linked_descriptor.location = self.coe_location + + # Mock the peer grading descriptor. + pg_descriptor = Mock() + pg_descriptor.location = self.problem_location + pg_descriptor.get_required_module_descriptors = lambda: [linked_descriptor, ] + + # Setup the proper field data for the peer grading module. + field_data = DictFieldData({ + 'data': '', + 'location': self.problem_location, + 'use_for_single_location': True, + 'link_to_location': self.coe_location, + }) + + # Initialize the peer grading module. + peer_grading = PeerGradingModule( + pg_descriptor, + self.test_system, + field_data, + ScopeIds(None, None, self.problem_location, self.problem_location) + ) + + # Ensure that it is properly setup. + self.assertTrue(peer_grading.use_for_single_location) diff --git a/common/test/data/open_ended/course/2012_Fall.xml b/common/test/data/open_ended/course/2012_Fall.xml index 609d12f94c..232de855cc 100644 --- a/common/test/data/open_ended/course/2012_Fall.xml +++ b/common/test/data/open_ended/course/2012_Fall.xml @@ -4,5 +4,6 @@ + diff --git a/common/test/data/open_ended/peergrading/PeerGradingLinked.xml b/common/test/data/open_ended/peergrading/PeerGradingLinked.xml new file mode 100644 index 0000000000..b2380b1e1b --- /dev/null +++ b/common/test/data/open_ended/peergrading/PeerGradingLinked.xml @@ -0,0 +1 @@ + \ No newline at end of file