Fix Missing task extension duration in action_text.

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)

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

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