Merge pull request #211 from…

Merge pull request #211 from edx/muhhshoaib/PHX-195-adding-a-non-negative-for-adding-allowance-will-raise-exception

PHX-195 Adding a non-integer value for the time extension allowance will cause an exception to be thrown

edx_proctoring/exceptions.py

@@ -51,6 +51,12 @@ class UserNotFoundException(ProctoredBaseException):
     """
 
 
+class AllowanceValueNotAllowedException(ProctoredBaseException):
+    """
+    Raised when the allowance value is not an non-negative integer
+    """
+
+
 class BackendProvideCannotRegisterAttempt(ProctoredBaseException):
     """
     Raised when a back-end provider cannot register an attempt

edx_proctoring/models.py

@@ -12,6 +12,7 @@ from django.contrib.auth.models import User
 from edx_proctoring.exceptions import (
     UserNotFoundException,
     ProctoredExamNotActiveException,
+    AllowanceValueNotAllowedException
 )
 from django.db.models.base import ObjectDoesNotExist
 
@@ -711,6 +712,11 @@ class ProctoredExamStudentAllowance(TimeStampedModel):
         if isinstance(key, tuple) and len(key) > 0:
             key = key[0]
 
+        if not cls.is_allowance_value_valid(key, value):
+            err_msg = (
+                'allowance_value "{value}" should be non-negative integer value.'
+            ).format(value=value)
+            raise AllowanceValueNotAllowedException(err_msg)
         # were we passed a PK?
         if isinstance(user_info, (int, long)):
             user_id = user_info
@@ -740,6 +746,18 @@ class ProctoredExamStudentAllowance(TimeStampedModel):
             cls.objects.create(proctored_exam_id=exam_id, user_id=user_id, key=key, value=value)
 
     @classmethod
+    def is_allowance_value_valid(cls, allowance_type, allowance_value):
+        """
+        Method that validates the allowance value against the allowance type
+        """
+        # validates the allowance value only when the allowance type is "ADDITIONAL_TIME_GRANTED"
+        if allowance_type in cls.ADDITIONAL_TIME_GRANTED:
+            if not allowance_value.isdigit():
+                return False
+
+        return True
+
+    @classmethod
     def get_additional_time_granted(cls, exam_id, user_id):
         """
         Helper method to get the additional time granted

edx_proctoring/tests/test_api.py

@@ -49,7 +49,8 @@ from edx_proctoring.exceptions import (
     StudentExamAttemptedAlreadyStarted,
     UserNotFoundException,
     ProctoredExamIllegalStatusTransition,
-    ProctoredExamPermissionDenied
+    ProctoredExamPermissionDenied,
+    AllowanceValueNotAllowedException
 )
 from edx_proctoring.models import (
     ProctoredExam,
@@ -90,8 +91,8 @@ class ProctoredExamApiTests(LoggedInTestCase):
         self.disabled_content_id = 'test_disabled_content_id'
         self.exam_name = 'Test Exam'
         self.user_id = self.user.id
-        self.key = 'Test Key'
-        self.value = 'Test Value'
+        self.key = 'additional_time_granted'
+        self.value = '10'
         self.external_id = 'test_external_id'
         self.proctored_exam_id = self._create_proctored_exam()
         self.timed_exam = self._create_timed_exam()
@@ -443,18 +444,26 @@ class ProctoredExamApiTests(LoggedInTestCase):
         with self.assertRaises(UserNotFoundException):
             add_allowance_for_user(self.proctored_exam_id, 'invalid_user', self.key, self.value)
 
+    @ddt.data('invalid', '-2', '-99', 'invalid123')
+    def test_add_invalid_allowance_value(self, allowance_value):
+        """
+        Test to add allowance for invalid allowance value.
+        """
+        with self.assertRaises(AllowanceValueNotAllowedException):
+            add_allowance_for_user(self.proctored_exam_id, self.user.username, self.key, allowance_value)
+
     def test_update_existing_allowance(self):
         """
         Test updation to the allowance that already exists.
         """
         student_allowance = self._add_allowance_for_user()
-        add_allowance_for_user(student_allowance.proctored_exam.id, self.user.username, self.key, 'new_value')
+        add_allowance_for_user(student_allowance.proctored_exam.id, self.user.username, self.key, '4')
 
         student_allowance = ProctoredExamStudentAllowance.get_allowance_for_user(
             student_allowance.proctored_exam.id, self.user_id, self.key
         )
         self.assertIsNotNone(student_allowance)
-        self.assertEqual(student_allowance.value, 'new_value')
+        self.assertEqual(student_allowance.value, '4')
 
     def test_get_allowances_for_course(self):
         """
@@ -762,7 +771,7 @@ class ProctoredExamApiTests(LoggedInTestCase):
             user_id=self.user_id,
         )
         add_allowance_for_user(self.proctored_exam_id, self.user.username, self.key, self.value)
-        add_allowance_for_user(self.proctored_exam_id, self.user.username, 'new_key', 'new_value')
+        add_allowance_for_user(self.proctored_exam_id, self.user.username, 'new_key', '2')
         student_active_exams = get_active_exams_for_user(self.user_id, self.course_id)
         self.assertEqual(len(student_active_exams), 2)
         self.assertEqual(len(student_active_exams[0]['allowances']), 0)

edx_proctoring/tests/test_views.py

@@ -1891,6 +1891,37 @@ class TestExamAllowanceView(LoggedInTestCase):
         self.assertEqual(len(response_data), 1)
         self.assertEqual(response_data['detail'], u"Cannot find user against invalid_user")
 
+    def test_add_invalid_allowance_value(self):
+        """
+        Add allowance for a invalid user_info.
+        """
+        # Create an exam.
+        proctored_exam = ProctoredExam.objects.create(
+            course_id='a/b/c',
+            content_id='test_content',
+            exam_name='Test Exam',
+            external_id='123aXqe3',
+            time_limit_mins=90,
+            is_active=True
+        )
+        allowance_data = {
+            'exam_id': proctored_exam.id,
+            'user_info': self.student_taking_exam.username,
+            'key': 'additional_time_granted',
+            'value': 'invalid_value'
+        }
+        response = self.client.put(
+            reverse('edx_proctoring.proctored_exam.allowance'),
+            allowance_data
+        )
+        self.assertEqual(response.status_code, 400)
+        response_data = json.loads(response.content)
+        self.assertEqual(len(response_data), 1)
+        self.assertEqual(
+            response_data['detail'],
+            u'allowance_value "invalid_value" should be non-negative integer value.'
+        )
+
     def test_add_allowance_for_inactive_exam(self):
         """
         Adding allowance for an inactive exam returns a 400 error.

edx_proctoring/views.py

@@ -39,6 +39,7 @@ from edx_proctoring.exceptions import (
     StudentExamAttemptDoesNotExistsException,
     ProctoredExamIllegalStatusTransition,
     ProctoredExamNotActiveException,
+    AllowanceValueNotAllowedException
 )
 from edx_proctoring import constants
 from edx_proctoring.runtime import get_runtime_service
@@ -737,7 +738,7 @@ class ExamAllowanceView(AuthenticatedAPIView):
                 value=request.DATA.get('value', None)
             ))
 
-        except (UserNotFoundException, ProctoredExamNotActiveException) as ex:
+        except (AllowanceValueNotAllowedException, UserNotFoundException, ProctoredExamNotActiveException) as ex:
             LOG.exception(ex)
             return Response(
                 status=status.HTTP_400_BAD_REQUEST,