Merge pull request #971 from edx/fix/vik/pg-hotfix

Fix/vik/pg hotfix

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):

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
@@ -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,11 +109,21 @@ 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))
                 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 +530,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 exists at 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 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
 
         good_problem_list = []
@@ -529,7 +549,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

common/lib/xmodule/xmodule/tests/test_peer_grading.py

@@ -2,6 +2,12 @@ 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
+from mock import Mock
+from xmodule.peer_grading_module import PeerGradingModule
+from xblock.field_data import DictFieldData
+from xblock.fields import ScopeIds
 
 import logging
 
@@ -136,6 +142,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 +168,70 @@ 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.assertNotIn("Peer-Graded", html)
+
+        # 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)
+
+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': '<peergrading/>',
+            '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)

common/test/data/open_ended/course/2012_Fall.xml

@@ -4,5 +4,6 @@
     <combinedopenended url_name="SampleQuestion1Attempt"/>
     <peergrading url_name="PeerGradingSample"/>
     <peergrading url_name="PeerGradingScored"/>
+    <peergrading url_name="PeerGradingLinked"/>
   </chapter>
 </course>

common/test/data/open_ended/peergrading/PeerGradingLinked.xml

+<peergrading is_graded="True" max_grade="1" use_for_single_location="True" link_to_location="i4x://edX/open_ended/combinedopenended/SampleQuestion"/>
\ No newline at end of file

common/test/data/open_ended_nopath/README.md

+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.

common/test/data/open_ended_nopath/course.xml

+<course org="edX" course="open_ended_nopath" url_name="2012_Fall"/>

common/test/data/open_ended_nopath/course/2012_Fall.xml

+<course>
+  <chapter url_name="Overview">
+  </chapter>
+</course>

common/test/data/open_ended_nopath/peergrading/PeerGradingNoPath.xml

+<peergrading/>
\ No newline at end of file

common/test/data/open_ended_nopath/policies/2012_Fall.json

+{
+    "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"
+    }
+}

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)

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

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
@@ -97,25 +97,32 @@ 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
-    if len(items) > 0:
-        item_location = items[0].location
-        #Generate a url for the first module and redirect the user to it
+    # 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.
+        try:
             problem_url_parts = search.path_to_location(modulestore(), course.id, item_location)
+        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))
+            continue
         problem_url = generate_problem_url(problem_url_parts, base_course_url)
         found_module = True