diff --git a/backend/models/postgis/task.py b/backend/models/postgis/task.py index 1f0433577f..5bb0a1810c 100644 --- a/backend/models/postgis/task.py +++ b/backend/models/postgis/task.py @@ -784,6 +784,16 @@ def unlock_task( self, user_id, new_state=None, comment=None, undo=False, issues=None ): """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 = TaskHistory.get_last_action(self.project_id, self.id) + # To unlock a task the last action must have been either lock or extension + last_action = TaskAction[last_history.action] + TaskHistory.update_task_locked_with_duration( + self.id, self.project_id, last_action, user_id + ) + + # Only create new history after updating the duration since we need the last action to update the duration. if comment: self.set_task_history( action=TaskAction.COMMENT, @@ -822,12 +832,6 @@ def unlock_task( self.mapped_by = None self.validated_by = None - if not undo: - # Using a slightly evil side effect of Actions and Statuses having the same name here :) - TaskHistory.update_task_locked_with_duration( - self.id, self.project_id, TaskStatus(self.task_status), user_id - ) - self.task_status = new_state.value self.locked_by = None self.update() @@ -835,15 +839,19 @@ def unlock_task( def reset_lock(self, user_id, comment=None): """Removes a current lock from a task, resets to last status and updates history with duration of lock""" + last_history = TaskHistory.get_last_action(self.project_id, self.id) + # To reset a lock the last action must have been either lock or extension + last_action = TaskAction[last_history.action] + TaskHistory.update_task_locked_with_duration( + self.id, self.project_id, last_action, user_id + ) + + # Only set task history after updating the duration since we need the last action to update the duration. if comment: self.set_task_history( action=TaskAction.COMMENT, comment=comment, user_id=user_id ) - # Using a slightly evil side effect of Actions and Statuses having the same name here :) - TaskHistory.update_task_locked_with_duration( - self.id, self.project_id, TaskStatus(self.task_status), user_id - ) self.clear_lock() def clear_lock(self): diff --git a/backend/services/mapping_service.py b/backend/services/mapping_service.py index ca8c1a47bd..f994f50adb 100644 --- a/backend/services/mapping_service.py +++ b/backend/services/mapping_service.py @@ -471,11 +471,16 @@ def extend_task_lock_time(extend_dto: ExtendLockTimeDTO): if task.task_status == TaskStatus.LOCKED_FOR_VALIDATION: action = TaskAction.EXTENDED_FOR_VALIDATION + # Update the duration of the lock/extension before creating new history + last_history = TaskHistory.get_last_action(task.project_id, task.id) + # To reset a lock the last action must have been either lock or extension + last_action = TaskAction[last_history.action] TaskHistory.update_task_locked_with_duration( task_id, extend_dto.project_id, - TaskStatus(task.task_status), + last_action, extend_dto.user_id, ) + task.set_task_history(action, extend_dto.user_id) task.update() diff --git a/tests/backend/integration/api/tasks/test_actions.py b/tests/backend/integration/api/tasks/test_actions.py index 4d7bb034aa..02d316e61f 100644 --- a/tests/backend/integration/api/tasks/test_actions.py +++ b/tests/backend/integration/api/tasks/test_actions.py @@ -561,9 +561,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, @@ -579,14 +578,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, @@ -612,6 +614,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): @@ -685,9 +692,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, @@ -703,9 +709,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, @@ -992,11 +997,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.""" @@ -1197,7 +1205,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 @@ -1218,7 +1225,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 @@ -1246,6 +1252,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 4d4b052a5b..bc42ae32a9 100644 --- a/tests/backend/integration/services/test_mapping_service.py +++ b/tests/backend/integration/services/test_mapping_service.py @@ -1,7 +1,12 @@ import datetime import xml.etree.ElementTree as ET from unittest.mock import patch -from backend.services.mapping_service import MappingService, Task +from backend.services.mapping_service import ( + MappingService, + Task, + TaskHistory, + ExtendLockTimeDTO, +) from backend.models.postgis.task import TaskStatus 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 21700db801..5e51f1b57b 100644 --- a/tests/backend/unit/services/test_mapping_service.py +++ b/tests/backend/unit/services/test_mapping_service.py @@ -1,4 +1,5 @@ from unittest.mock import patch, MagicMock + from backend.services.mapping_service import ( MappingService, Task, @@ -127,6 +128,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") @@ -147,6 +149,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 @@ -154,6 +157,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) @@ -163,6 +170,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") @@ -179,11 +187,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)