From c7f997220e03c2565542511baeab88fae50e528b Mon Sep 17 00:00:00 2001 From: Aadesh-Baral Date: Mon, 28 Aug 2023 14:35:19 +0545 Subject: [PATCH] Split checks for tasks status and user permission in function so that correct error response can be returned --- backend/api/tasks/actions.py | 4 +-- backend/error_messages.json | 1 + backend/services/mapping_service.py | 31 ++++++++++++------- .../integration/api/tasks/test_actions.py | 12 +++---- .../unit/services/test_mapping_service.py | 6 ++-- 5 files changed, 33 insertions(+), 21 deletions(-) diff --git a/backend/api/tasks/actions.py b/backend/api/tasks/actions.py index 0c0b542a3a..017dc8e0f9 100644 --- a/backend/api/tasks/actions.py +++ b/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): diff --git a/backend/error_messages.json b/backend/error_messages.json index ba2d128dbf..1a193c8b12 100644 --- a/backend/error_messages.json +++ b/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.", diff --git a/backend/services/mapping_service.py b/backend/services/mapping_service.py index 7e22c6128d..b4b41b8684 100644 --- a/backend/services/mapping_service.py +++ b/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: diff --git a/tests/backend/integration/api/tasks/test_actions.py b/tests/backend/integration/api/tasks/test_actions.py index f9629e1401..a7209de04a 100644 --- a/tests/backend/integration/api/tasks/test_actions.py +++ b/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): diff --git a/tests/backend/unit/services/test_mapping_service.py b/tests/backend/unit/services/test_mapping_service.py index 21700db801..ac6e3d3e05 100644 --- a/tests/backend/unit/services/test_mapping_service.py +++ b/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)