diff --git a/backend/models/postgis/task.py b/backend/models/postgis/task.py index 5bb7352ad5..6918e99c5c 100644 --- a/backend/models/postgis/task.py +++ b/backend/models/postgis/task.py @@ -1103,71 +1103,6 @@ async def reset_task(task_id: int, project_id: int, user_id: int, db: Database): new_state=TaskStatus.READY, ) - @staticmethod - async def clear_task_lock(task_id: int, project_id: int, db: Database): - """Unlocks task in scope, clears the lock as though it never happened.""" - - # Get the last locked action and delete it from the task history - last_action = await TaskHistory.get_last_locked_action(project_id, task_id, db) - if last_action: - delete_action_query = """ - DELETE FROM task_history - WHERE id = :history_id - """ - await db.execute( - query=delete_action_query, values={"history_id": last_action["id"]} - ) - - # Clear the lock from the task itself - await Task.clear_lock(task_id=task_id, project_id=project_id, db=db) - - @staticmethod - async def record_auto_unlock( - task_id: int, project_id: int, lock_duration: str, db: Database - ): - """Automatically unlocks the task and records the auto-unlock action in task history""" - - # Fetch the locked user and last locked action for the task - locked_user_query = """ - SELECT locked_by - FROM tasks - WHERE id = :task_id AND project_id = :project_id - """ - locked_user = await db.fetch_one( - query=locked_user_query, - values={"task_id": task_id, "project_id": project_id}, - ) - - last_action = await TaskHistory.get_last_locked_action(project_id, task_id, db) - - if last_action and last_action["action"] == "LOCKED_FOR_MAPPING": - next_action = TaskAction.AUTO_UNLOCKED_FOR_MAPPING - else: - next_action = TaskAction.AUTO_UNLOCKED_FOR_VALIDATION - - # Clear the task lock (clear the lock and delete the last locked action) - await Task.clear_task_lock(task_id, project_id, db) - - # Add AUTO_UNLOCKED action in the task history - auto_unlocked = await Task.set_task_history( - task_id=task_id, - project_id=project_id, - user_id=locked_user["locked_by"], - action=next_action, - db=db, - ) - - # Update the action_text with the lock duration - update_history_query = """ - UPDATE task_history - SET action_text = :lock_duration - WHERE id = :history_id - """ - await db.execute( - query=update_history_query, - values={"lock_duration": lock_duration, "history_id": auto_unlocked["id"]}, - ) - @staticmethod async def unlock_task( task_id: int, @@ -1179,8 +1114,17 @@ async def unlock_task( undo: bool = False, issues: Optional[List[Dict[str, Any]]] = None, ): - """Unlock the task and change its state.""" - # Add task comment history if provided + """Unlock task and ensure duration task locked is saved in History""" + # If not undo, update the duration of the lock + if not undo: + last_history = await TaskHistory.get_last_action(project_id, task_id, db) + # To unlock a task the last action must have been either lock or extension + last_action = TaskAction[last_history["result"][0]["action"]] + await TaskHistory.update_task_locked_with_duration( + task_id, project_id, last_action, user_id, db + ) + + # Only create new history after updating the duration since we need the last action to update the duration. if comment: await Task.set_task_history( task_id, @@ -1191,6 +1135,7 @@ async def unlock_task( comment=comment, mapping_issues=issues, ) + # Record state change in history history = await Task.set_task_history( task_id, @@ -1202,6 +1147,7 @@ async def unlock_task( new_state=new_state, mapping_issues=issues, ) + # If undo, clear the mapped_by and validated_by fields if undo: if new_state == TaskStatus.MAPPED: update_query = """ @@ -1282,11 +1228,6 @@ async def unlock_task( }, ) - # Update task locked duration in the history when `undo` is False - # Using a slightly evil side effect of Actions and Statuses having the same name here :) - await TaskHistory.update_task_locked_with_duration( - task_id, project_id, TaskStatus(current_status), user_id, db - ) # Final query for updating task status final_update_query = """ UPDATE tasks @@ -1302,6 +1243,71 @@ async def unlock_task( }, ) + @staticmethod + async def clear_task_lock(task_id: int, project_id: int, db: Database): + """Unlocks task in scope, clears the lock as though it never happened.""" + + # Get the last locked action and delete it from the task history + last_action = await TaskHistory.get_last_locked_action(project_id, task_id, db) + if last_action: + delete_action_query = """ + DELETE FROM task_history + WHERE id = :history_id + """ + await db.execute( + query=delete_action_query, values={"history_id": last_action["id"]} + ) + + # Clear the lock from the task itself + await Task.clear_lock(task_id=task_id, project_id=project_id, db=db) + + @staticmethod + async def record_auto_unlock( + task_id: int, project_id: int, lock_duration: str, db: Database + ): + """Automatically unlocks the task and records the auto-unlock action in task history""" + + # Fetch the locked user and last locked action for the task + locked_user_query = """ + SELECT locked_by + FROM tasks + WHERE id = :task_id AND project_id = :project_id + """ + locked_user = await db.fetch_one( + query=locked_user_query, + values={"task_id": task_id, "project_id": project_id}, + ) + + last_action = await TaskHistory.get_last_locked_action(project_id, task_id, db) + + if last_action and last_action["action"] == "LOCKED_FOR_MAPPING": + next_action = TaskAction.AUTO_UNLOCKED_FOR_MAPPING + else: + next_action = TaskAction.AUTO_UNLOCKED_FOR_VALIDATION + + # Clear the task lock (clear the lock and delete the last locked action) + await Task.clear_task_lock(task_id, project_id, db) + + # Add AUTO_UNLOCKED action in the task history + auto_unlocked = await Task.set_task_history( + task_id=task_id, + project_id=project_id, + user_id=locked_user["locked_by"], + action=next_action, + db=db, + ) + + # Update the action_text with the lock duration + update_history_query = """ + UPDATE task_history + SET action_text = :lock_duration + WHERE id = :history_id + """ + await db.execute( + query=update_history_query, + values={"lock_duration": lock_duration, "history_id": auto_unlocked["id"]}, + ) + @staticmethod async def reset_lock( task_id: int, @@ -1320,6 +1326,15 @@ async def reset_lock( :param comment: Optional comment provided during the reset. :param db: The database connection. """ + + last_history = TaskHistory.get_last_action(project_id, task_id, db) + # To reset a lock the last action must have been either lock or extension + last_action = TaskAction[last_history["result"][0]["action"]] + await TaskHistory.update_task_locked_with_duration( + task_id, project_id, last_action, user_id, db + ) + + # Only set task history after updating the duration since we need the last action to update the duration. # If a comment is provided, set the task history with a comment action if comment: await Task.set_task_history( @@ -1330,14 +1345,6 @@ async def reset_lock( comment=comment, db=db, ) - # Update task lock history with duration - await TaskHistory.update_task_locked_with_duration( - task_id=task_id, - project_id=project_id, - lock_action=TaskStatus(task_status), - user_id=user_id, - db=db, - ) # Clear the lock on the task await Task.clear_lock(task_id=task_id, project_id=project_id, db=db) diff --git a/backend/services/mapping_service.py b/backend/services/mapping_service.py index c0ad17a914..c494b78236 100644 --- a/backend/services/mapping_service.py +++ b/backend/services/mapping_service.py @@ -578,10 +578,16 @@ async def extend_task_lock_time(extend_dto: ExtendLockTimeDTO, db: Database): else TaskAction.EXTENDED_FOR_VALIDATION ) + # Update the duration of the lock/extension before creating new history + last_history = await TaskHistory.get_last_action( + extend_dto.project_id, task_id + ) + # To reset a lock the last action must have been either lock or extension + last_action = TaskAction[last_history["result"][0]["action"]] await TaskHistory.update_task_locked_with_duration( task_id, extend_dto.project_id, - TaskStatus(task["task_status"]), + last_action, extend_dto.user_id, db, ) diff --git a/tests/backend/integration/api/tasks/test_actions.py b/tests/backend/integration/api/tasks/test_actions.py index d27621a08a..f5008d2452 100644 --- a/tests/backend/integration/api/tasks/test_actions.py +++ b/tests/backend/integration/api/tasks/test_actions.py @@ -560,9 +560,8 @@ def test_mapping_unlock_returns_200_on_success(self): """Test returns 200 on success.""" # Arrange task = Task.get(1, self.test_project.id) - task.task_status = TaskStatus.LOCKED_FOR_MAPPING.value - task.locked_by = self.test_user.id - task.update() + task.status = TaskStatus.READY.value + task.lock_task_for_mapping(self.test_user.id) # Act response = self.client.post( self.url, @@ -578,14 +577,17 @@ def test_mapping_unlock_returns_200_on_success(self): self.assertEqual(last_task_history["action"], TaskAction.STATE_CHANGE.name) self.assertEqual(last_task_history["actionText"], TaskStatus.MAPPED.name) self.assertEqual(last_task_history["actionBy"], self.test_user.username) + # Check if lock duration is saved + # Since locked duration is saved in entry with action as "LOCKED_FOR_MAPPING" + # we need to look for second entry in task history + self.assertIsNotNone(response.json["taskHistory"][1]["actionText"]) def test_mapping_unlock_returns_200_on_success_with_comment(self): """Test returns 200 on success.""" # Arrange task = Task.get(1, self.test_project.id) - task.task_status = TaskStatus.LOCKED_FOR_MAPPING.value - task.locked_by = self.test_user.id - task.update() + task.status = TaskStatus.READY.value + task.lock_task_for_mapping(self.test_user.id) # Act response = self.client.post( self.url, @@ -611,6 +613,11 @@ def test_mapping_unlock_returns_200_on_success_with_comment(self): self.assertEqual(last_comment_history["actionText"], "cannot map") self.assertEqual(last_comment_history["actionBy"], self.test_user.username) + # Check if lock duration is saved + # Since locked duration is saved in entry with action as "LOCKED_FOR_MAPPING" + # we need to look for third entry in task history as second entry is comment + self.assertIsNotNone(response.json["taskHistory"][2]["actionText"]) + class TestTasksActionsMappingStopAPI(BaseTestCase): def setUp(self): @@ -684,9 +691,8 @@ def test_mapping_stop_returns_200_on_success(self): """Test returns 200 on success.""" # Arrange task = Task.get(1, self.test_project.id) - task.task_status = TaskStatus.LOCKED_FOR_MAPPING.value - task.locked_by = self.test_user.id - task.update() + task.status = TaskStatus.READY.value + task.lock_task_for_mapping(self.test_user.id) # Act response = self.client.post( self.url, @@ -702,9 +708,8 @@ def test_mapping_stop_returns_200_on_success_with_comment(self): """Test returns 200 on success.""" # Arrange task = Task.get(1, self.test_project.id) - task.task_status = TaskStatus.LOCKED_FOR_MAPPING.value - task.locked_by = self.test_user.id - task.update() + task.status = TaskStatus.READY.value + task.lock_task_for_mapping(self.test_user.id) # Act response = self.client.post( self.url, @@ -991,11 +996,14 @@ def test_validation_unlock_returns_403_if_task_not_locked_for_validation(self): def lock_task_for_validation(task_id, project_id, user_id, mapped_by=None): """Lock task for validation.""" task = Task.get(task_id, project_id) - task.task_status = TaskStatus.LOCKED_FOR_VALIDATION.value - task.locked_by = user_id + if mapped_by: - task.mapped_by = mapped_by - task.update() + task.status = TaskStatus.READY.value + task.lock_task_for_mapping(mapped_by) + task.unlock_task(mapped_by, TaskStatus.MAPPED) + + task.status = TaskStatus.MAPPED.value + task.lock_task_for_validating(user_id) def test_validation_unlock_returns_403_if_task_locked_by_other_user(self): """Test returns 403 if task locked by other user.""" @@ -1206,7 +1214,6 @@ def test_validation_stop_returns_200_if_task_locked_by_user(self): """Test returns 200 if task locked by user.""" # Arrange task = Task.get(1, self.test_project.id) - task.unlock_task(self.test_user.id, TaskStatus.MAPPED) last_task_status = TaskStatus(task.task_status).name TestTasksActionsValidationUnlockAPI.lock_task_for_validation( 1, self.test_project.id, self.test_user.id, self.test_user.id @@ -1227,7 +1234,6 @@ def test_validation_stop_returns_200_if_task_locked_by_user_with_comment(self): """Test returns 200 if task locked by user with comment.""" # Arrange task = Task.get(1, self.test_project.id) - task.unlock_task(self.test_user.id, TaskStatus.MAPPED) last_task_status = TaskStatus(task.task_status).name TestTasksActionsValidationUnlockAPI.lock_task_for_validation( 1, self.test_project.id, self.test_user.id, self.test_user.id @@ -1255,6 +1261,11 @@ def test_validation_stop_returns_200_if_task_locked_by_user_with_comment(self): self.assertEqual(task_history_comment["actionText"], "Test comment") self.assertEqual(task_history_comment["actionBy"], self.test_user.username) + # Check if lock duration is saved + # Since locked duration is saved in entry with action as "LOCKED_FOR_MAPPING" + # we need to look for third entry in task history as second entry is comment + self.assertIsNotNone(response.json["tasks"][0]["taskHistory"][2]["actionText"]) + class TestTasksActionsSplitAPI(BaseTestCase): def setUp(self): diff --git a/tests/backend/integration/services/test_mapping_service.py b/tests/backend/integration/services/test_mapping_service.py index e8158d433d..7c4a0f339c 100644 --- a/tests/backend/integration/services/test_mapping_service.py +++ b/tests/backend/integration/services/test_mapping_service.py @@ -2,8 +2,13 @@ import xml.etree.ElementTree as ET from unittest.mock import patch +from backend.services.mapping_service import ( + MappingService, + Task, + TaskHistory, + ExtendLockTimeDTO, +) from backend.models.postgis.task import TaskStatus -from backend.services.mapping_service import MappingService, Task from tests.backend.base import BaseTestCase from tests.backend.helpers.test_helpers import create_canned_project @@ -165,3 +170,38 @@ def test_reset_all_bad_imagery( # Assert for task in self.test_project.tasks: self.assertNotEqual(task.task_status, TaskStatus.BADIMAGERY.value) + + def test_task_extend_duration_is_recorded(self): + if self.skip_tests: + return + + # Arrange + task = Task.get(1, self.test_project.id) + task.task_status = TaskStatus.READY.value + task.update() + task.lock_task_for_mapping(self.test_user.id) + extend_lock_dto = ExtendLockTimeDTO() + extend_lock_dto.task_ids = [task.id] + extend_lock_dto.project_id = self.test_project.id + extend_lock_dto.user_id = self.test_user.id + # Act + # Extend the task lock time twice and check the task history + MappingService.extend_task_lock_time(extend_lock_dto) + MappingService.extend_task_lock_time(extend_lock_dto) + task.reset_lock(self.test_user.id) + + # Assert + # Check that the task history has 2 entries for EXTENDED_FOR_MAPPING and that the action_text is not None + extended_task_history = ( + TaskHistory.query.filter_by( + task_id=task.id, + project_id=self.test_project.id, + ) + .order_by(TaskHistory.action_date.desc()) + .limit(5) + .all() + ) + self.assertEqual(extended_task_history[0].action, "EXTENDED_FOR_MAPPING") + self.assertEqual(extended_task_history[1].action, "EXTENDED_FOR_MAPPING") + self.assertIsNotNone(extended_task_history[0].action_text) + self.assertIsNotNone(extended_task_history[1].action_text) diff --git a/tests/backend/unit/services/test_mapping_service.py b/tests/backend/unit/services/test_mapping_service.py index 731de0c30c..0f7b01ee5f 100644 --- a/tests/backend/unit/services/test_mapping_service.py +++ b/tests/backend/unit/services/test_mapping_service.py @@ -3,6 +3,7 @@ from backend.models.dtos.mapping_dto import LockTaskDTO, MappedTaskDTO from backend.models.postgis.project_info import ProjectInfo from backend.models.postgis.task import TaskAction, TaskHistory, User + from backend.services.mapping_service import ( MappingNotAllowed, MappingService, @@ -128,6 +129,7 @@ def test_if_new_state_not_acceptable_raise_error(self, mock_task): with self.assertRaises(MappingServiceError): MappingService.unlock_task_after_mapping(self.mapped_task_dto) + @patch.object(TaskHistory, "get_last_action") @patch.object(ProjectService, "send_email_on_project_progress") @patch.object(ProjectInfo, "get_dto_for_locale") @patch.object(Task, "get_per_task_instructions") @@ -148,6 +150,7 @@ def test_unlock_with_comment_sets_history( mock_state, mock_project_name, mock_send_email, + mock_last_action, ): # Arrange self.task_stub.task_status = TaskStatus.LOCKED_FOR_MAPPING.value @@ -155,6 +158,10 @@ def test_unlock_with_comment_sets_history( mock_task.return_value = self.task_stub mock_state.return_value = TaskStatus.LOCKED_FOR_MAPPING mock_project_name.name.return_value = "Test project" + history = TaskHistory(1, 1, 1) + history.action = TaskAction.LOCKED_FOR_MAPPING.name + mock_last_action.return_value = history + # Act test_task = MappingService.unlock_task_after_mapping(self.mapped_task_dto) @@ -164,6 +171,7 @@ def test_unlock_with_comment_sets_history( self.assertEqual(TaskAction.COMMENT.name, test_task.task_history[0].action) self.assertEqual(test_task.task_history[0].action_text, "Test comment") + @patch.object(TaskHistory, "get_last_action") @patch.object(ProjectService, "send_email_on_project_progress") @patch.object(Task, "get_per_task_instructions") @patch.object(StatsService, "update_stats_after_task_state_change") @@ -180,11 +188,15 @@ def test_unlock_with_status_change_sets_history( mock_instructions, mock_state, mock_send_email, + mock_last_action, ): # Arrange self.task_stub.task_status = TaskStatus.LOCKED_FOR_MAPPING.value mock_task.return_value = self.task_stub mock_state.return_value = TaskStatus.LOCKED_FOR_MAPPING + history = TaskHistory(1, 1, 1) + history.action = TaskAction.LOCKED_FOR_MAPPING.name + mock_last_action.return_value = history # Act test_task = MappingService.unlock_task_after_mapping(self.mapped_task_dto)