From ead59cb412d9b072c496623109f210c681fee6ec Mon Sep 17 00:00:00 2001 From: Aadesh-Baral Date: Fri, 1 Sep 2023 11:53:57 +0545 Subject: [PATCH 1/3] Record task extension duration in action_text. ------------------ In the previous implementation, there was an issue where the duration of a task extension was not being recorded when a task was extended and then unlocked/stopped. Prior to the implementation of the task lock extension feature, the last recorded action for a task was always either "LOCKED_FOR_MAPPING" or "LOCKED_FOR_VALIDATION," which matched the task status. To update the duration, we used the current task status as the last action to filter the task history and update the last entry with the lock duration. However, after the task extension feature was introduced, two additional last actions were possible: "EXTENDED_FOR_MAPPING" and "EXTENDED_FOR_VALIDATION." This introduced a discrepancy between the last recorded action and the task status. For example, the task status could still be "LOCKED_FOR_MAPPING" even when a user had extended the task. As a result, the entry with the action "EXTENDED_FOR_MAPPING" in the task history was not being updated with the correct duration. To address this issue, this commit fixed the problem by retrieving the last task action and using it as the filter criteria for updating the task history, rather than relying on the task status as the last action. --- backend/models/postgis/task.py | 28 ++++++++++++++++++---------- backend/services/mapping_service.py | 7 ++++++- 2 files changed, 24 insertions(+), 11 deletions(-) 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() From 0f5b2ebb030de3d7a7e6a9285436a3346cb51e40 Mon Sep 17 00:00:00 2001 From: Aadesh-Baral Date: Fri, 1 Sep 2023 12:10:47 +0545 Subject: [PATCH 2/3] Fix failing test cases due to missing task lock history. --- .../integration/api/tasks/test_actions.py | 33 +++++++++---------- .../unit/services/test_mapping_service.py | 12 +++++++ 2 files changed, 27 insertions(+), 18 deletions(-) diff --git a/tests/backend/integration/api/tasks/test_actions.py b/tests/backend/integration/api/tasks/test_actions.py index 4d7bb034aa..f6fba2fecc 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, @@ -584,9 +583,8 @@ 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, @@ -685,9 +683,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 +700,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 +988,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 +1196,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 +1216,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 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) From 4f9be01e3a78d5fb03d66defac78681543d2d62a Mon Sep 17 00:00:00 2001 From: Aadesh-Baral Date: Mon, 4 Sep 2023 16:37:49 +0545 Subject: [PATCH 3/3] Add test case to check if extension duration is recorded --- .../integration/api/tasks/test_actions.py | 14 +++++++ .../services/test_mapping_service.py | 42 ++++++++++++++++++- 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/tests/backend/integration/api/tasks/test_actions.py b/tests/backend/integration/api/tasks/test_actions.py index f6fba2fecc..02d316e61f 100644 --- a/tests/backend/integration/api/tasks/test_actions.py +++ b/tests/backend/integration/api/tasks/test_actions.py @@ -578,6 +578,10 @@ 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.""" @@ -610,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): @@ -1243,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)