diff --git a/common/lib/xmodule/xmodule/peer_grading_module.py b/common/lib/xmodule/xmodule/peer_grading_module.py index 59d75b29d3..b16ca81656 100644 --- a/common/lib/xmodule/xmodule/peer_grading_module.py +++ b/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): """ diff --git a/common/lib/xmodule/xmodule/tests/test_peer_grading.py b/common/lib/xmodule/xmodule/tests/test_peer_grading.py index 0d992428be..ebfeaa0693 100644 --- a/common/lib/xmodule/xmodule/tests/test_peer_grading.py +++ b/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): diff --git a/lms/djangoapps/open_ended_grading/staff_grading_service.py b/lms/djangoapps/open_ended_grading/staff_grading_service.py index 55ab87a8f0..090b36d13c 100644 --- a/lms/djangoapps/open_ended_grading/staff_grading_service.py +++ b/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, "" diff --git a/lms/djangoapps/open_ended_grading/tests.py b/lms/djangoapps/open_ended_grading/tests.py index ba3996655b..517d3d8824 100644 --- a/lms/djangoapps/open_ended_grading/tests.py +++ b/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):