Fix Missing task extension duration in action_text.

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,28 +832,26 @@ 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()
 
     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):

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()

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):

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)

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,13 +149,18 @@ 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
         self.mapped_task_dto.comment = "Test comment"
         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)