Merge pull request #2228 from edx/waheed/ora201-large-posts-to-ora-like-peer-grading-feedback

Really large posts to ORA like peer grading feedback fixed.

common/lib/xmodule/xmodule/peer_grading_module.py

@@ -23,6 +23,7 @@ log = logging.getLogger(__name__)
 
 
 EXTERNAL_GRADER_NO_CONTACT_ERROR = "Failed to contact external graders.  Please notify course staff."
+MAX_ALLOWED_FEEDBACK_LENGTH = 5000
 
 
 class PeerGradingFields(object):
@@ -344,6 +345,10 @@ class PeerGradingModule(PeerGradingFields, XModule):
         if not success:
             return self._err_response(message)
 
+        success, message = self._check_feedback_length(data)
+        if not success:
+            return self._err_response(message)
+
         data_dict = {k:data.get(k) for k in required}
         if 'rubric_scores[]' in required:
             data_dict['rubric_scores'] = data.getall('rubric_scores[]')
@@ -638,6 +643,15 @@ class PeerGradingModule(PeerGradingFields, XModule):
 
         return json.dumps(state)
 
+    def _check_feedback_length(self, data):
+        feedback = data.get("feedback")
+        if feedback and len(feedback) > MAX_ALLOWED_FEEDBACK_LENGTH:
+            return False, "Feedback is too long, Max length is {0} characters.".format(
+                MAX_ALLOWED_FEEDBACK_LENGTH
+            )
+        else:
+            return True, ""
+
 
 class PeerGradingDescriptor(PeerGradingFields, RawDescriptor):
     """

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

@@ -11,7 +11,7 @@ from xmodule.modulestore import Location
 from xmodule.tests import get_test_system, get_test_descriptor_system
 from xmodule.tests.test_util_open_ended import DummyModulestore
 from xmodule.open_ended_grading_classes.peer_grading_service import MockPeerGradingService
-from xmodule.peer_grading_module import PeerGradingModule, PeerGradingDescriptor
+from xmodule.peer_grading_module import PeerGradingModule, PeerGradingDescriptor, MAX_ALLOWED_FEEDBACK_LENGTH
 from xmodule.modulestore.exceptions import ItemNotFoundError, NoPathToItem
 
 log = logging.getLogger(__name__)
@@ -158,6 +158,27 @@ class PeerGradingModuleTest(unittest.TestCase, DummyModulestore):
         """
         self.peer_grading.get_instance_state()
 
+    def test_save_grade_with_long_feedback(self):
+        """
+        Test if feedback is too long save_grade() should return error message.
+        """
+
+        feedback_fragment = "This is very long feedback."
+        self.save_dict["feedback"] = feedback_fragment * (
+            (MAX_ALLOWED_FEEDBACK_LENGTH / len(feedback_fragment) + 1)
+        )
+
+        response = self.peer_grading.save_grade(self.save_dict)
+
+        # Should not succeed.
+        self.assertEqual(response['success'], False)
+        self.assertEqual(
+            response['error'],
+            "Feedback is too long, Max length is {0} characters.".format(
+                MAX_ALLOWED_FEEDBACK_LENGTH
+            )
+        )
+
 
 class MockPeerGradingServiceProblemList(MockPeerGradingService):
     def get_problem_list(self, course_id, grader_id):

lms/djangoapps/open_ended_grading/staff_grading_service.py

@@ -30,6 +30,7 @@ STAFF_ERROR_MESSAGE = _(
         tech_support_email=settings.TECH_SUPPORT_EMAIL
     )
 )
+MAX_ALLOWED_FEEDBACK_LENGTH = 5000
 
 
 class MockStaffGradingService(object):
@@ -353,19 +354,21 @@ def save_grade(request, course_id):
     #If the instructor has skipped grading the submission, then there will not be any rubric scores.
     #Only add in the rubric scores if the instructor has not skipped.
     if not skipped:
-        required|=set(['rubric_scores[]'])
+        required.add('rubric_scores[]')
     actual = set(p.keys())
     missing = required - actual
     if len(missing) > 0:
         return _err_response('Missing required keys {0}'.format(
             ', '.join(missing)))
 
+    success, message = check_feedback_length(p)
+    if not success:
+        return _err_response(message)
+
     grader_id = unique_id_for_user(request.user)
 
-
     location = p['location']
 
-
     try:
         result_json = staff_grading_service().save_grade(course_id,
                                                          grader_id,
@@ -402,3 +405,13 @@ def save_grade(request, course_id):
     # Ok, save_grade seemed to work.  Get the next submission to grade.
     return HttpResponse(_get_next(course_id, grader_id, location),
                         mimetype="application/json")
+
+
+def check_feedback_length(data):
+    feedback = data.get("feedback")
+    if feedback and len(feedback) > MAX_ALLOWED_FEEDBACK_LENGTH:
+        return False, "Feedback is too long, Max length is {0} characters.".format(
+            MAX_ALLOWED_FEEDBACK_LENGTH
+        )
+    else:
+        return True, ""

lms/djangoapps/open_ended_grading/tests.py

@@ -217,6 +217,40 @@ class TestStaffGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase):
         # Check that the error text is correct.
         self.assertIn("Cannot find", response['error'])
 
+    def test_save_grade_with_long_feedback(self):
+        """
+        Test if feedback is too long save_grade() should return error message.
+        """
+        self.login(self.instructor, self.password)
+
+        url = reverse('staff_grading_save_grade', kwargs={'course_id': self.course_id})
+
+        data = {
+            'score': '12',
+            'feedback': '',
+            'submission_id': '123',
+            'location': self.location,
+            'submission_flagged': "false",
+            'rubric_scores[]': ['1', '2']
+        }
+
+        feedback_fragment = "This is very long feedback."
+        data["feedback"] = feedback_fragment * (
+            (staff_grading_service.MAX_ALLOWED_FEEDBACK_LENGTH / len(feedback_fragment) + 1)
+        )
+
+        response = check_for_post_code(self, 200, url, data)
+        content = json.loads(response.content)
+
+        # Should not succeed.
+        self.assertEquals(content['success'], False)
+        self.assertEquals(
+            content['error'],
+            "Feedback is too long, Max length is {0} characters.".format(
+                staff_grading_service.MAX_ALLOWED_FEEDBACK_LENGTH
+            )
+        )
+
 
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
 class TestPeerGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase):
@@ -371,6 +405,38 @@ class TestPeerGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self.assertTrue(response['error'].find('Missing required keys:') > -1)
         self.assertFalse('actual_score' in response)
 
+    def test_save_grade_with_long_feedback(self):
+        """
+        Test if feedback is too long save_grade() should return error message.
+        """
+        data = {
+            'rubric_scores[]': [0, 0],
+            'location': self.location,
+            'submission_id': 1,
+            'submission_key': 'fake key',
+            'score': 2,
+            'feedback': '',
+            'submission_flagged': 'false',
+            'answer_unknown': 'false',
+            'rubric_scores_complete': 'true'
+        }
+
+        feedback_fragment = "This is very long feedback."
+        data["feedback"] = feedback_fragment * (
+            (staff_grading_service.MAX_ALLOWED_FEEDBACK_LENGTH / len(feedback_fragment) + 1)
+        )
+
+        response_dict = self.peer_module.save_grade(data)
+
+        # Should not succeed.
+        self.assertEquals(response_dict['success'], False)
+        self.assertEquals(
+            response_dict['error'],
+            "Feedback is too long, Max length is {0} characters.".format(
+                staff_grading_service.MAX_ALLOWED_FEEDBACK_LENGTH
+            )
+        )
+
 
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
 class TestPanel(ModuleStoreTestCase):