Ensure that peer grading modules whose linked problem is removed change over to panel mode

common/lib/xmodule/xmodule/peer_grading_module.py

@@ -104,30 +104,28 @@ class PeerGradingModule(PeerGradingFields, XModule):
     def __init__(self, *args, **kwargs):
         super(PeerGradingModule, self).__init__(*args, **kwargs)
 
-        #We need to set the location here so the child modules can use it
+        # Copy this to a new variable so that we can edit it if needed.
+        # We need to edit it if the linked module cannot be found, so
+        # we can revert to panel model.
+        self.use_for_single_location_local = self.use_for_single_location
+
+        # We need to set the location here so the child modules can use it.
         self.runtime.set('location', self.location)
         if (self.runtime.open_ended_grading_interface):
             self.peer_gs = PeerGradingService(self.system.open_ended_grading_interface, self.system)
         else:
             self.peer_gs = MockPeerGradingService()
 
-        if self.use_for_single_location:
-            try:
-                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)
+        if self.use_for_single_location_local:
+            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)
+                # Change module over to panel mode from single problem mode.
+                self.use_for_single_location_local = False
+            else:
                 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
 
         try:
             self.timeinfo = TimeInfo(self.due, self.graceperiod)
@@ -175,7 +173,7 @@ class PeerGradingModule(PeerGradingFields, XModule):
         """
         if self.closed():
             return self.peer_grading_closed()
-        if not self.use_for_single_location:
+        if not self.use_for_single_location_local:
             return self.peer_grading()
         else:
             return self.peer_grading_problem({'location': self.link_to_location})['html']
@@ -236,7 +234,7 @@ class PeerGradingModule(PeerGradingFields, XModule):
             'score': score,
             'total': max_score,
         }
-        if not self.use_for_single_location or not self.graded:
+        if not self.use_for_single_location_local or not self.graded:
             return score_dict
 
         try:
@@ -270,7 +268,7 @@ class PeerGradingModule(PeerGradingFields, XModule):
               randomization, and 5/7 on another
         '''
         max_grade = None
-        if self.use_for_single_location and self.graded:
+        if self.use_for_single_location_local and self.graded:
             max_grade = self.weight
         return max_grade
 
@@ -492,7 +490,7 @@ class PeerGradingModule(PeerGradingFields, XModule):
         Show the Peer grading closed template
         '''
         html = self.system.render_template('peer_grading/peer_grading_closed.html', {
-            'use_for_single_location': self.use_for_single_location
+            'use_for_single_location': self.use_for_single_location_local
         })
         return html
 
@@ -578,7 +576,7 @@ class PeerGradingModule(PeerGradingFields, XModule):
             'error_text': error_text,
             # Checked above
             'staff_access': False,
-            'use_single_location': self.use_for_single_location,
+            'use_single_location': self.use_for_single_location_local,
         })
 
         return html
@@ -588,7 +586,7 @@ class PeerGradingModule(PeerGradingFields, XModule):
         Show individual problem interface
         '''
         if data is None or data.get('location') is None:
-            if not self.use_for_single_location:
+            if not self.use_for_single_location_local:
                 # This is an error case, because it must be set to use a single location to be called without get parameters
                 # This is a dev_facing_error
                 log.error(
@@ -610,7 +608,7 @@ class PeerGradingModule(PeerGradingFields, XModule):
             # Checked above
             'staff_access': False,
             'track_changes': getattr(module, 'track_changes', False),
-            'use_single_location': self.use_for_single_location,
+            'use_single_location': self.use_for_single_location_local,
         })
 
         return {'html': html, 'success': True}
@@ -656,10 +654,24 @@ class PeerGradingDescriptor(PeerGradingFields, RawDescriptor):
         return non_editable_fields
 
     def get_required_module_descriptors(self):
-        """Returns a list of XModuleDescritpor instances upon which this module depends, but are
-        not children of this module"""
+        """
+        Returns a list of XModuleDescriptor instances upon which this module depends, but are
+        not children of this module.
+        """
+
+        # If use_for_single_location is True, this is linked to an open ended problem.
         if self.use_for_single_location:
-            return [self.system.load_item(self.link_to_location)]
+            # Try to load the linked module.
+            # If we can't load it, return empty list to avoid exceptions on progress page.
+            try:
+                linked_module = self.system.load_item(self.link_to_location)
+                return [linked_module]
+            except (NoPathToItem, ItemNotFoundError):
+                error_message = ("Cannot find the combined open ended module "
+                                 "at location {0} being linked to from peer "
+                                 "grading module {1}").format(self.link_to_location, self.location)
+                log.error(error_message)
+                return []
         else:
             return []
 

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

 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
 from mock import Mock
-from xmodule.peer_grading_module import PeerGradingModule, InvalidLinkLocation
+
 from xblock.field_data import DictFieldData
 from xblock.fields import ScopeIds
-from xmodule.modulestore.exceptions import ItemNotFoundError, NoPathToItem
-import json
 
-import logging
+from xmodule.modulestore import Location
+from xmodule.tests import get_test_system, get_test_descriptor_system
+from xmodule.tests.test_util_open_ended import MockQueryDict, DummyModulestore
+from xmodule.open_ended_grading_classes.peer_grading_service import MockPeerGradingService
+from xmodule.peer_grading_module import PeerGradingModule, PeerGradingDescriptor
+from xmodule.modulestore.exceptions import ItemNotFoundError, NoPathToItem
 
 log = logging.getLogger(__name__)
 
@@ -37,7 +38,7 @@ class PeerGradingModuleTest(unittest.TestCase, DummyModulestore):
         'feedback': "",
         'rubric_scores[]': [0, 1],
         'submission_flagged': False,
-        'answer_unknown' : False,
+        'answer_unknown': False,
     })
 
     def setUp(self):
@@ -57,22 +58,22 @@ class PeerGradingModuleTest(unittest.TestCase, DummyModulestore):
         @return:
         """
         closed = self.peer_grading.closed()
-        self.assertEqual(closed, False)
+        self.assertFalse(closed)
 
     def test_get_html(self):
         """
         Test to see if the module can be rendered
         @return:
         """
-        html = self.peer_grading.get_html()
+        _html = self.peer_grading.get_html()
 
     def test_get_data(self):
         """
         Try getting data from the external grading service
         @return:
         """
-        success, data = self.peer_grading.query_data_for_location(self.problem_location.url())
-        self.assertEqual(success, True)
+        success, _data = self.peer_grading.query_data_for_location(self.problem_location.url())
+        self.assertTrue(success)
 
     def test_get_score(self):
         """
@@ -80,7 +81,7 @@ class PeerGradingModuleTest(unittest.TestCase, DummyModulestore):
         @return:
         """
         score = self.peer_grading.get_score()
-        self.assertEquals(score['score'], None)
+        self.assertIsNone(score['score'])
 
     def test_get_max_score(self):
         """
@@ -95,7 +96,7 @@ class PeerGradingModuleTest(unittest.TestCase, DummyModulestore):
         Test to see if we can get the next mock submission
         @return:
         """
-        success, next_submission = self.peer_grading.get_next_submission({'location': 'blah'})
+        success, _next_submission = self.peer_grading.get_next_submission({'location': 'blah'})
         self.assertEqual(success, True)
 
     def test_save_grade(self):
@@ -111,9 +112,8 @@ class PeerGradingModuleTest(unittest.TestCase, DummyModulestore):
         Check to see if the student has calibrated yet
         @return:
         """
-        calibrated_dict = {'location': "blah"}
         response = self.peer_grading.is_student_calibrated(self.calibrated_dict)
-        self.assertEqual(response['success'], True)
+        self.assertTrue(response['success'])
 
     def test_show_calibration_essay(self):
         """
@@ -121,7 +121,7 @@ class PeerGradingModuleTest(unittest.TestCase, DummyModulestore):
         @return:
         """
         response = self.peer_grading.show_calibration_essay(self.calibrated_dict)
-        self.assertEqual(response['success'], True)
+        self.assertTrue(response['success'])
 
     def test_save_calibration_essay(self):
         """
@@ -129,7 +129,7 @@ class PeerGradingModuleTest(unittest.TestCase, DummyModulestore):
         @return:
         """
         response = self.peer_grading.save_calibration_essay(self.save_dict)
-        self.assertEqual(response['success'], True)
+        self.assertTrue(response['success'])
 
     def test_peer_grading_problem(self):
         """
@@ -137,7 +137,7 @@ class PeerGradingModuleTest(unittest.TestCase, DummyModulestore):
         @return:
         """
         response = self.peer_grading.peer_grading_problem(self.coe_dict)
-        self.assertEqual(response['success'], True)
+        self.assertTrue(response['success'])
 
     def test___find_corresponding_module_for_location_exceptions(self):
         """
@@ -146,7 +146,7 @@ class PeerGradingModuleTest(unittest.TestCase, DummyModulestore):
         @return:
         """
         with self.assertRaises(ItemNotFoundError):
-            self.peer_grading._find_corresponding_module_for_location(Location('i4x','a','b','c','d'))
+            self.peer_grading._find_corresponding_module_for_location(Location('i4x', 'a', 'b', 'c', 'd'))
 
     def test_get_instance_state(self):
         """
@@ -155,6 +155,7 @@ 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,
@@ -162,13 +163,16 @@ class MockPeerGradingServiceProblemList(MockPeerGradingService):
                     {"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
     external grading service.
     """
-    problem_location = Location(["i4x", "edX", "open_ended", "peergrading",
-                                 "PeerGradingScored"])
+    problem_location = Location(
+        ["i4x", "edX", "open_ended", "peergrading", "PeerGradingScored"]
+    )
+
     def setUp(self):
         """
         Create a peer grading module from a test system
@@ -180,7 +184,7 @@ 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)
+        self.assertFalse(peer_grading.closed())
 
     def test_problem_list(self):
         """
@@ -217,6 +221,37 @@ class PeerGradingModuleLinkedTest(unittest.TestCase, DummyModulestore):
         self.test_system.open_ended_grading_interface = None
         self.setup_modulestore(COURSE)
 
+    @property
+    def field_data(self):
+        """
+        Setup the proper field data for a peer grading module.
+        """
+
+        return DictFieldData({
+            'data': '<peergrading/>',
+            'location': self.problem_location,
+            'use_for_single_location': True,
+            'link_to_location': self.coe_location.url(),
+            'graded': True,
+            })
+
+    @property
+    def scope_ids(self):
+        """
+        Return the proper scope ids for the peer grading module.
+        """
+        return ScopeIds(None, None, self.problem_location, self.problem_location)
+
+    def _create_peer_grading_descriptor_with_linked_problem(self):
+        # Initialize the peer grading module.
+        system = get_test_descriptor_system()
+
+        return system.construct_xblock_from_class(
+            PeerGradingDescriptor,
+            field_data=self.field_data,
+            scope_ids=self.scope_ids
+        )
+
     def _create_peer_grading_with_linked_problem(self, location, valid_linked_descriptor=True):
         """
         Create a peer grading problem with a linked location.
@@ -235,34 +270,56 @@ class PeerGradingModuleLinkedTest(unittest.TestCase, DummyModulestore):
         else:
             pg_descriptor.get_required_module_descriptors = lambda: []
 
-        # 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.url(),
-            'graded': True,
-        })
-
         # Initialize the peer grading module.
         peer_grading = PeerGradingModule(
             pg_descriptor,
             self.test_system,
-            field_data,
-            ScopeIds(None, None, self.problem_location, self.problem_location)
+            self.field_data,
+            self.scope_ids,
         )
 
         return peer_grading
 
+    def _get_descriptor_with_invalid_link(self, exception_to_raise):
+        """
+        Ensure that a peer grading descriptor with an invalid link will return an empty list.
+        """
+
+        # Create a descriptor, and make loading an item throw an error.
+        descriptor = self._create_peer_grading_descriptor_with_linked_problem()
+        descriptor.system.load_item = Mock(side_effect=exception_to_raise)
+
+        # Ensure that modules is a list of length 0.
+        modules = descriptor.get_required_module_descriptors()
+        self.assertIsInstance(modules, list)
+        self.assertEqual(len(modules), 0)
+
+    def test_descriptor_with_nopath(self):
+        """
+        Test to see if a descriptor with a NoPathToItem error when trying to get
+        its linked module behaves properly.
+        """
+
+        self._get_descriptor_with_invalid_link(NoPathToItem)
+
+    def test_descriptor_with_item_not_found(self):
+        """
+        Test to see if a descriptor with an ItemNotFound error when trying to get
+        its linked module behaves properly.
+        """
+
+        self._get_descriptor_with_invalid_link(ItemNotFoundError)
+
     def test_invalid_link(self):
         """
-        Ensure that a peer grading problem with no linked locations raises an error.
+        Ensure that a peer grading problem with no linked locations stays in panel mode.
         """
 
         # Setup the peer grading module with no linked locations.
-        with self.assertRaises(InvalidLinkLocation):
-            self._create_peer_grading_with_linked_problem(self.coe_location, valid_linked_descriptor=False)
+        peer_grading = self._create_peer_grading_with_linked_problem(self.coe_location, valid_linked_descriptor=False)
 
+        self.assertFalse(peer_grading.use_for_single_location_local)
+        self.assertTrue(peer_grading.use_for_single_location)
 
     def test_linked_problem(self):
         """
@@ -284,7 +341,7 @@ class PeerGradingModuleLinkedTest(unittest.TestCase, DummyModulestore):
         peer_grading = self._create_peer_grading_with_linked_problem(self.coe_location)
 
         # If we specify a location, it will render the problem for that location.
-        data = peer_grading.handle_ajax('problem', {'location' : self.coe_location})
+        data = peer_grading.handle_ajax('problem', {'location': self.coe_location})
         self.assertTrue(json.loads(data)['success'])
 
         # If we don't specify a location, it should use the linked location.
@@ -344,5 +401,5 @@ class PeerGradingModuleTrackChangesTest(unittest.TestCase, DummyModulestore):
         """
         self.peer_grading._find_corresponding_module_for_location = self.mock_track_changes_problem
         response = self.peer_grading.peer_grading_problem({'location': 'mocked'})
-        self.assertEqual(response['success'], True)
+        self.assertTrue(response['success'])
         self.assertIn("'track_changes': True", response['html'])