Split checks for tasks status and user permission in function so that correct error response can be returned

Author: Aadesh-Baral

backend/api/tasks/actions.py

@@ -328,8 +328,8 @@ def post(self, project_id, task_id):
                 project_id, task_id, token_auth.current_user(), preferred_locale
             )
             return task.to_primitive(), 200
-        except MappingServiceError as e:  # FLAGGED FOR STATUS CODE
-            return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 403
+        except MappingServiceError as e:
+            return {"Error": str(e).split("-")[1], "SubCode": str(e).split("-")[0]}, 409
 
 
 class TasksActionsValidationLockAPI(Resource):

backend/error_messages.json

@@ -27,6 +27,7 @@
     "USER_NOT_ADMIN": "This action is only allowed for system administrators.",
     "USER_NOT_ORG_MANAGER": "This action is only allowed for organisation managers or system administrators.",
     "USER_NOT_PROJECT_MANAGER": "This action is only allowed for users with project manage permissions.",
+    "USER_NOT_VALIDATOR": "This action is only allowed for users with validation permissions in the project.",
     "USER_NOT_TEAM_MANAGER": "This action is only allowed for users with team manage permissions.",
     "INVALID_NEW_PROJECT_OWNER": "New project owner must be project's organisation manager or system admin",
     "USER_ACTION_NOT_PERMITTED": "This user does not have the required permissions to perform this action.",

backend/services/mapping_service.py

@@ -59,22 +59,31 @@ def get_task_as_dto(
     @staticmethod
     def _is_task_undoable(logged_in_user_id: int, task: Task) -> bool:
         """Determines if the current task status can be undone by the logged in user"""
-        # Test to see if user can undo status on this task
-        if logged_in_user_id and TaskStatus(task.task_status) not in [
+        # Check if task status is in a state that can be undone
+        if TaskStatus(task.task_status) in [
             TaskStatus.LOCKED_FOR_MAPPING,
             TaskStatus.LOCKED_FOR_VALIDATION,
             TaskStatus.READY,
         ]:
-            last_action = TaskHistory.get_last_action(task.project_id, task.id)
-
-            # User requesting task made the last change, so they are allowed to undo it.
-            is_user_permitted, _ = ProjectService.is_user_permitted_to_validate(
-                task.project_id, logged_in_user_id
-            )  # FLAGGED: Make use of error_reason
-            if last_action.user_id == int(logged_in_user_id) or is_user_permitted:
-                return True
+            raise MappingServiceError(
+                "UndoNotAllowed- Task is in a state that cannot be undone"
+            )
+        # Test to see if user can undo status on this task
+        last_action = TaskHistory.get_last_action(task.project_id, task.id)
 
-        return False  # FLAGGED: Split out for permission and state
+        # User requesting task made the last change, so they are allowed to undo it.
+        is_user_permitted, _ = ProjectService.is_user_permitted_to_validate(
+            task.project_id, logged_in_user_id
+        )
+        if last_action.user_id == int(logged_in_user_id) or is_user_permitted:
+            return True
+        else:
+            raise Forbidden(
+                sub_code="USER_NOT_VALIDATOR",
+                user_id=logged_in_user_id,
+                project_id=task.project_id,
+                task_id=task.id,
+            )
 
     @staticmethod
     def lock_task_for_mapping(lock_task_dto: LockTaskDTO) -> TaskDTO:

tests/backend/integration/api/tasks/test_actions.py

@@ -1388,10 +1388,10 @@ def test_returns_404_if_task_not_found(self):
         self.assertEqual(response.status_code, 404)
         self.assertEqual(response.json["error"]["sub_code"], TASK_NOT_FOUND_SUB_CODE)
 
-    def test_returns_403_if_task_in_invalid_state_for_undo(self):
-        """Test returns 403 if task in invalid state for undo."""
+    def test_returns_409_if_task_in_invalid_state_for_undo(self):
+        """Test returns 409 if task in invalid state for undo."""
         # Since task cannot be in READY, LOCKED_FOR_VALIDATION or LOCKED_FOR_MAPPING state for undo,
-        # we should get a 403
+        # we should get a 409
         # Arrange
         task = Task.get(1, self.test_project.id)
         task.task_status = TaskStatus.READY.value
@@ -1402,8 +1402,8 @@ def test_returns_403_if_task_in_invalid_state_for_undo(self):
             headers={"Authorization": self.user_session_token},
         )
         # Assert
-        self.assertEqual(response.status_code, 403)
-        self.assertEqual(response.json["SubCode"], "UndoPermissionError")
+        self.assertEqual(response.status_code, 409)
+        self.assertEqual(response.json["SubCode"], "UndoNotAllowed")
 
     @staticmethod
     def validate_task(task_id, project_id, user_id):
@@ -1428,7 +1428,7 @@ def test_returns_403_if_user_not_permitted_for_undo(self):
         )
         # Assert
         self.assertEqual(response.status_code, 403)
-        self.assertEqual(response.json["SubCode"], "UndoPermissionError")
+        self.assertEqual(response.json["error"]["sub_code"], "USER_NOT_VALIDATOR")
 
     @staticmethod
     def assert_undo_response(response, project_id, last_status, username, new_status):

tests/backend/unit/services/test_mapping_service.py

@@ -1,4 +1,6 @@
 from unittest.mock import patch, MagicMock
+
+from backend.exceptions import Forbidden
 from backend.services.mapping_service import (
     MappingService,
     Task,
@@ -231,7 +233,7 @@ def test_task_is_not_undoable_if_last_change_not_made_by_you(
 
         # Act
         mock_project.return_value = (False, None)
-        is_undoable = MappingService._is_task_undoable(1, task)
 
         # Assert
-        self.assertFalse(is_undoable)
+        with self.assertRaises(Forbidden):
+            MappingService._is_task_undoable(1, task)