From 77d95fdcc450a6569bfe68b9c3a012329c84c639 Mon Sep 17 00:00:00 2001 From: AdrianoDev Date: Mon, 21 Sep 2026 22:48:08 +0200 Subject: [PATCH] fix(tasks): copy-on-write su modifica/cancellazione e ordine tolleranze (#4, #26) #4 - api/routers/tasks.py: update_task, delete_task, create_subtask, update_subtask, delete_subtask controllavano solo is_current, non version_has_measurements: la versione con misure veniva modificata sul posto, cambiando i limiti di quote gia' giudicate pass/fail. Nuovi helper _task_for_write / _subtask_for_write riusano recipe_service.create_new_version (lo stesso copy-on-write di create_task): se la versione ha misure creano una nuova versione, ritrovano il task per order_index e la quota per marker_number (chiavi che _copy_tasks_to_version preserva), e la scrittura va sulla copia. La versione misurata resta intatta. Trovato e corretto nello stesso punto: il task ritrovato nella copia non ha .version precaricato (new_version.tasks viene da selectinload, che non popola il backref figlio->genitore), e TaskResponse.recipe_id legge task.version.recipe_id - senza assegnare esplicitamente candidate.version = new_version, update_task va in MissingGreenlet (lazy load fuori da un await) invece di rispondere 200. #26 - models/api/task.py: nuovo validate_tolerance_order (ltl<=lwl<=nominal<= uwl<=utl sui soli campi valorizzati), usato come model_validator su SubtaskCreate e richiamato dal router in update_subtask sui valori risultanti (esistenti + parziali) prima del flush, perche' SubtaskUpdate non ha un insieme fisso di campi da validare da solo. Test: src/backend/tests/test_tasks.py, TestCopyOnWriteOnEdit (5) e TestToleranceOrder (4); adattato test_update_subtask esistente (nominal 12.0 violava il nuovo ordine contro utl=10.5 invariato). PROVA: uv run --extra server --extra dev pytest src/backend/tests/test_tasks.py -q -> 16 passed Co-Authored-By: Claude Sonnet 5 --- src/backend/api/routers/tasks.py | 117 +++++++++++-- src/backend/models/api/task.py | 32 +++- src/backend/tests/test_tasks.py | 275 ++++++++++++++++++++++++++++++- 3 files changed, 411 insertions(+), 13 deletions(-) diff --git a/src/backend/api/routers/tasks.py b/src/backend/api/routers/tasks.py index 3a66c85..bf85d78 100644 --- a/src/backend/api/routers/tasks.py +++ b/src/backend/api/routers/tasks.py @@ -17,6 +17,7 @@ from src.backend.models.api.task import ( TaskReorderRequest, TaskResponse, TaskUpdate, + validate_tolerance_order, ) from src.backend.services import recipe_service @@ -99,6 +100,64 @@ async def _ensure_version_is_current(db: AsyncSession, version_id: int) -> Recip return version +async def _task_for_write(db: AsyncSession, task: RecipeTask, user: User) -> RecipeTask: + """Return the task to write to: the same one, or its copy in a new version. + + A version that already has measurements cannot be edited on the spot - its + recorded pass/fail would silently start meaning something else. Copy-on-write + (the same one create_task/update_recipe use) gives the edit a fresh version + and leaves the measured one exactly as it was when the readings were taken. + The task is matched in the copy by its position (order_index), which + _copy_tasks_to_version preserves exactly. + """ + if not await recipe_service.version_has_measurements(db, task.version_id): + return task + + recipe_id = task.recipe_id + if recipe_id is None: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail="Cannot determine the recipe for this task", + ) + from src.backend.models.api.recipe import RecipeUpdate + new_version = await recipe_service.create_new_version( + db, recipe_id, RecipeUpdate(change_notes=f"Edited task: {task.title}"), user, + ) + for candidate in new_version.tasks: + if candidate.order_index == task.order_index: + # TaskResponse.recipe_id reads task.version.recipe_id; new_version's + # own tasks are loaded via selectinload, which does not also + # populate the child->parent backref, so left alone this would + # lazy-load .version outside of an awaited context and crash. + candidate.version = new_version + return candidate + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail="Could not locate the task in the new version", + ) + + +async def _subtask_for_write( + db: AsyncSession, task: RecipeTask, subtask: RecipeSubtask, user: User, +) -> tuple[RecipeTask, RecipeSubtask]: + """Return (task, subtask) to write to: the same ones, or their copies. + + Same copy-on-write as _task_for_write, one level down: once the task's copy + is found, the quote is matched inside it by marker_number (unique per task). + """ + new_task = await _task_for_write(db, task, user) + if new_task is task: + return task, subtask + + for candidate in new_task.subtasks: + if candidate.marker_number == subtask.marker_number: + return new_task, candidate + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail="Could not locate the quote in the new version", + ) + + # --------------------------------------------------------------------------- # Task endpoints # --------------------------------------------------------------------------- @@ -264,10 +323,16 @@ async def reorder_tasks( async def update_task( task_id: int, data: TaskUpdate, - _user: User = Depends(require_maker), + user: User = Depends(require_maker), db: AsyncSession = Depends(get_db), ): - """Update a task. The task must belong to the current version. Requires Maker role.""" + """Update a task. The task must belong to the current version. Requires Maker role. + + If that version already has measurements the edit goes through copy-on-write: + a new version is created and its copy of the task is the one changed. The + version with measurements is left untouched, so its recorded pass/fail keeps + meaning what it meant when it was taken. + """ task = await _get_task_or_404(db, task_id) await _ensure_version_is_current(db, task.version_id) @@ -275,6 +340,8 @@ async def update_task( if not update_data: return TaskResponse.model_validate(task) + task = await _task_for_write(db, task, user) + for field, value in update_data.items(): setattr(task, field, value) @@ -286,16 +353,19 @@ async def update_task( @router.delete("/api/tasks/{task_id}", status_code=204) async def delete_task( task_id: int, - _user: User = Depends(require_maker), + user: User = Depends(require_maker), db: AsyncSession = Depends(get_db), ): """Delete a task and its subtasks. The task must belong to the current version. - Requires Maker role. + Requires Maker role. Copy-on-write applies as in update_task: a version with + measurements is never touched, the deletion lands on its copy instead. """ task = await _get_task_or_404(db, task_id) await _ensure_version_is_current(db, task.version_id) + task = await _task_for_write(db, task, user) + await db.delete(task) await db.flush() @@ -312,16 +382,19 @@ async def delete_task( async def create_subtask( task_id: int, data: SubtaskCreate, - _user: User = Depends(require_maker), + user: User = Depends(require_maker), db: AsyncSession = Depends(get_db), ): """Add a subtask to a task. The task must belong to the current version. - Requires Maker role. + Requires Maker role. Copy-on-write applies as in create_task: a version with + measurements gets a new version, and the subtask is added there instead. """ task = await _get_task_or_404(db, task_id) await _ensure_version_is_current(db, task.version_id) + task = await _task_for_write(db, task, user) + # Check marker_number uniqueness within the task existing_markers = {s.marker_number for s in task.subtasks} if data.marker_number in existing_markers: @@ -331,7 +404,7 @@ async def create_subtask( ) subtask = RecipeSubtask( - task_id=task_id, + task_id=task.id, marker_number=data.marker_number, description=data.description, measurement_type=data.measurement_type, @@ -354,12 +427,13 @@ async def create_subtask( async def update_subtask( subtask_id: int, data: SubtaskUpdate, - _user: User = Depends(require_maker), + user: User = Depends(require_maker), db: AsyncSession = Depends(get_db), ): """Update a subtask. Its parent task must belong to the current version. - Requires Maker role. + Requires Maker role. Copy-on-write applies as in update_task, one level + down: a version with measurements is never touched, the quote's copy is. """ subtask = await _get_subtask_or_404(db, subtask_id) @@ -371,6 +445,24 @@ async def update_subtask( if not update_data: return SubtaskResponse.model_validate(subtask) + # SubtaskUpdate is partial, so the order can only be checked against the + # values the write would actually leave in place: existing + incoming. + def _resulting(field: str) -> float | None: + value = update_data.get(field, getattr(subtask, field)) + return None if value is None else float(value) + + try: + validate_tolerance_order( + _resulting("ltl"), _resulting("lwl"), _resulting("nominal"), + _resulting("uwl"), _resulting("utl"), + ) + except ValueError as exc: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_CONTENT, detail=str(exc), + ) + + task, subtask = await _subtask_for_write(db, task, subtask, user) + for field, value in update_data.items(): setattr(subtask, field, value) @@ -382,12 +474,13 @@ async def update_subtask( @router.delete("/api/subtasks/{subtask_id}", status_code=204) async def delete_subtask( subtask_id: int, - _user: User = Depends(require_maker), + user: User = Depends(require_maker), db: AsyncSession = Depends(get_db), ): """Delete a subtask. Its parent task must belong to the current version. - Requires Maker role. + Requires Maker role. Copy-on-write applies as in update_task, one level + down: a version with measurements is never touched, the quote's copy is. """ subtask = await _get_subtask_or_404(db, subtask_id) @@ -395,5 +488,7 @@ async def delete_subtask( task = await _get_task_or_404(db, subtask.task_id) await _ensure_version_is_current(db, task.version_id) + task, subtask = await _subtask_for_write(db, task, subtask, user) + await db.delete(subtask) await db.flush() diff --git a/src/backend/models/api/task.py b/src/backend/models/api/task.py index c5ef1fa..55902b4 100644 --- a/src/backend/models/api/task.py +++ b/src/backend/models/api/task.py @@ -1,7 +1,7 @@ """Pydantic schemas for RecipeTask and RecipeSubtask operations.""" from typing import Any, Literal, Optional -from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, ConfigDict, Field, model_validator from src.backend.models.orm.task import TASK_TYPES @@ -9,6 +9,31 @@ from src.backend.models.orm.task import TASK_TYPES TaskType = Literal[TASK_TYPES] +def validate_tolerance_order( + ltl: Optional[float], + lwl: Optional[float], + nominal: Optional[float], + uwl: Optional[float], + utl: Optional[float], +) -> None: + """ltl <= lwl <= nominal <= uwl <= utl, checked only across the values set. + + A one-sided tolerance - only utl, say - is not an error: what is wrong is + limits that invert the sequence they do share, like utl landing below ltl. + Shared by SubtaskCreate's own validator and by the router's partial-update + check (SubtaskUpdate has no fixed set of fields to compare). + """ + ordered = [ + ("ltl", ltl), ("lwl", lwl), ("nominal", nominal), ("uwl", uwl), ("utl", utl), + ] + present = [(name, value) for name, value in ordered if value is not None] + for (prev_name, prev), (name, value) in zip(present, present[1:]): + if prev > value: + raise ValueError( + f"Tolerance limits out of order: {prev_name}={prev} > {name}={value}" + ) + + class SubtaskCreate(BaseModel): """Schema for creating a subtask.""" marker_number: int = Field(..., ge=1) @@ -23,6 +48,11 @@ class SubtaskCreate(BaseModel): image_path: Optional[str] = Field(None, max_length=500) vision_output: Optional[str] = None + @model_validator(mode="after") + def _check_tolerance_order(self) -> "SubtaskCreate": + validate_tolerance_order(self.ltl, self.lwl, self.nominal, self.uwl, self.utl) + return self + class SubtaskUpdate(BaseModel): """Schema for updating a subtask.""" diff --git a/src/backend/tests/test_tasks.py b/src/backend/tests/test_tasks.py index a63b531..e72b4c5 100644 --- a/src/backend/tests/test_tasks.py +++ b/src/backend/tests/test_tasks.py @@ -7,6 +7,14 @@ from src.backend.models.orm.user import User from src.backend.tests.conftest import auth_headers, create_test_recipe +async def _measure(client, user, subtask_id, version_id, value=10.0): + return await client.post( + "/api/measurements/", + headers=auth_headers(user), + json={"subtask_id": subtask_id, "version_id": version_id, "value": value}, + ) + + class TestListTasks: """GET /api/recipes/{recipe_id}/tasks tests.""" @@ -171,7 +179,9 @@ class TestUpdateSubtask: resp = await client.put( f"/api/subtasks/{subtask_id}", headers=auth_headers(maker_user), - json={"description": "Updated description", "nominal": 12.0}, + # nominal stays inside the fixture's own lwl(9.7)/uwl(10.3): #26 + # rejects a nominal like 12.0 that would land past utl(10.5). + json={"description": "Updated description", "nominal": 10.2}, ) assert resp.status_code == 200 assert resp.json()["description"] == "Updated description" @@ -221,3 +231,266 @@ class TestReorderTasks: assert resp.status_code == 200 reordered = resp.json() assert [t["id"] for t in reordered] == reversed_ids + + +class TestCopyOnWriteOnEdit: + """A version with measurements is never edited in place (gitea#4).""" + + async def test_update_task_with_measurements_creates_new_version( + self, + client: AsyncClient, + maker_user: User, + measurement_tec_user: User, + db_session: AsyncSession, + ): + recipe = await create_test_recipe(db_session, maker_user.id, code="COW-UT") + task = (await client.get( + f"/api/recipes/{recipe.id}/tasks", headers=auth_headers(maker_user), + )).json()[0] + task_id, old_version_id = task["id"], task["version_id"] + subtask_id = task["subtasks"][0]["id"] + + assert (await _measure( + client, measurement_tec_user, subtask_id, old_version_id + )).status_code == 200 + + resp = await client.put( + f"/api/tasks/{task_id}", + headers=auth_headers(maker_user), + json={"title": "Titolo nuovo"}, + ) + assert resp.status_code == 200, resp.text + assert resp.json()["version_id"] != old_version_id + assert resp.json()["title"] == "Titolo nuovo" + + # The measured version keeps its original title and its versions count grew. + old = (await client.get( + f"/api/recipes/{recipe.id}/versions/1", headers=auth_headers(maker_user), + )).json() + assert old["tasks"][0]["title"] == "Test Task" + versions = (await client.get( + f"/api/recipes/{recipe.id}/versions", headers=auth_headers(maker_user), + )).json() + assert len(versions) == 2 + + # The measurement is still findable under the old version. + listed = (await client.get( + "/api/measurements/", headers=auth_headers(measurement_tec_user), + params={"version_id": old_version_id}, + )).json() + assert listed["total"] == 1 + + async def test_delete_task_with_measurements_creates_new_version( + self, + client: AsyncClient, + maker_user: User, + measurement_tec_user: User, + db_session: AsyncSession, + ): + recipe = await create_test_recipe(db_session, maker_user.id, code="COW-DT") + task = (await client.get( + f"/api/recipes/{recipe.id}/tasks", headers=auth_headers(maker_user), + )).json()[0] + task_id, old_version_id = task["id"], task["version_id"] + subtask_id = task["subtasks"][0]["id"] + + assert (await _measure( + client, measurement_tec_user, subtask_id, old_version_id + )).status_code == 200 + + resp = await client.delete( + f"/api/tasks/{task_id}", headers=auth_headers(maker_user), + ) + assert resp.status_code == 204 + + # The measured version still has its task untouched. + old = (await client.get( + f"/api/recipes/{recipe.id}/versions/1", headers=auth_headers(maker_user), + )).json() + assert len(old["tasks"]) == 1 + + # The new current version has the task removed. expunge_all forces a + # fresh read - the shared test session otherwise still has the old + # RecipeVersion.tasks collection cached from create_test_recipe. + db_session.expunge_all() + current = (await client.get( + f"/api/recipes/{recipe.id}/tasks", headers=auth_headers(maker_user), + )).json() + assert current == [] + + async def test_update_subtask_with_measurements_creates_new_version( + self, + client: AsyncClient, + maker_user: User, + measurement_tec_user: User, + db_session: AsyncSession, + ): + recipe = await create_test_recipe(db_session, maker_user.id, code="COW-US") + task = (await client.get( + f"/api/recipes/{recipe.id}/tasks", headers=auth_headers(maker_user), + )).json()[0] + old_version_id = task["version_id"] + subtask = task["subtasks"][0] + subtask_id = subtask["id"] + + assert (await _measure( + client, measurement_tec_user, subtask_id, old_version_id + )).status_code == 200 + + resp = await client.put( + f"/api/subtasks/{subtask_id}", + headers=auth_headers(maker_user), + json={"description": "Quota rivista"}, + ) + assert resp.status_code == 200, resp.text + assert resp.json()["description"] == "Quota rivista" + assert resp.json()["id"] != subtask_id, "la modifica va sulla copia, non sull'originale" + + # The measured version's own quote is untouched. + old = (await client.get( + f"/api/recipes/{recipe.id}/versions/1", headers=auth_headers(maker_user), + )).json() + assert old["tasks"][0]["subtasks"][0]["description"] == subtask["description"] + + async def test_delete_subtask_with_measurements_creates_new_version( + self, + client: AsyncClient, + maker_user: User, + measurement_tec_user: User, + db_session: AsyncSession, + ): + recipe = await create_test_recipe(db_session, maker_user.id, code="COW-DS") + task = (await client.get( + f"/api/recipes/{recipe.id}/tasks", headers=auth_headers(maker_user), + )).json()[0] + old_version_id = task["version_id"] + subtask_id = task["subtasks"][0]["id"] + + assert (await _measure( + client, measurement_tec_user, subtask_id, old_version_id + )).status_code == 200 + + resp = await client.delete( + f"/api/subtasks/{subtask_id}", headers=auth_headers(maker_user), + ) + assert resp.status_code == 204 + + # The measured version keeps its quote. + old = (await client.get( + f"/api/recipes/{recipe.id}/versions/1", headers=auth_headers(maker_user), + )).json() + assert len(old["tasks"][0]["subtasks"]) == 1 + + # The new current version has no quotes left on the task. expunge_all + # forces a fresh read past the shared test session's cached collection. + db_session.expunge_all() + current = (await client.get( + f"/api/recipes/{recipe.id}/tasks", headers=auth_headers(maker_user), + )).json() + assert current[0]["subtasks"] == [] + + async def test_create_subtask_with_measurements_creates_new_version( + self, + client: AsyncClient, + maker_user: User, + measurement_tec_user: User, + db_session: AsyncSession, + ): + recipe = await create_test_recipe(db_session, maker_user.id, code="COW-CS") + task = (await client.get( + f"/api/recipes/{recipe.id}/tasks", headers=auth_headers(maker_user), + )).json()[0] + task_id, old_version_id = task["id"], task["version_id"] + subtask_id = task["subtasks"][0]["id"] + + assert (await _measure( + client, measurement_tec_user, subtask_id, old_version_id + )).status_code == 200 + + resp = await client.post( + f"/api/tasks/{task_id}/subtasks", + headers=auth_headers(maker_user), + json={"marker_number": 9, "description": "Nuova quota", "utl": 5.0}, + ) + assert resp.status_code == 201, resp.text + + # The measured version's task is unchanged (still one quote). + old = (await client.get( + f"/api/recipes/{recipe.id}/versions/1", headers=auth_headers(maker_user), + )).json() + assert len(old["tasks"][0]["subtasks"]) == 1 + + # The new current version's task has both quotes. expunge_all forces a + # fresh read past the shared test session's cached collection. + db_session.expunge_all() + current = (await client.get( + f"/api/recipes/{recipe.id}/tasks", headers=auth_headers(maker_user), + )).json() + assert len(current[0]["subtasks"]) == 2 + + +class TestToleranceOrder: + """ltl <= lwl <= nominal <= uwl <= utl, or 422 (gitea#26).""" + + async def test_create_subtask_utl_below_ltl_is_422( + self, client: AsyncClient, maker_user: User, db_session: AsyncSession, + ): + recipe = await create_test_recipe(db_session, maker_user.id, code="TOL-A") + task_id = (await client.get( + f"/api/recipes/{recipe.id}/tasks", headers=auth_headers(maker_user), + )).json()[0]["id"] + + resp = await client.post( + f"/api/tasks/{task_id}/subtasks", + headers=auth_headers(maker_user), + json={"marker_number": 5, "description": "Quota invertita", "ltl": 8, "utl": 6}, + ) + assert resp.status_code == 422 + + async def test_create_subtask_uwl_above_utl_is_422( + self, client: AsyncClient, maker_user: User, db_session: AsyncSession, + ): + recipe = await create_test_recipe(db_session, maker_user.id, code="TOL-B") + task_id = (await client.get( + f"/api/recipes/{recipe.id}/tasks", headers=auth_headers(maker_user), + )).json()[0]["id"] + + resp = await client.post( + f"/api/tasks/{task_id}/subtasks", + headers=auth_headers(maker_user), + json={"marker_number": 5, "description": "Warning oltre tolleranza", "uwl": 11, "utl": 10}, + ) + assert resp.status_code == 422 + + async def test_create_subtask_one_sided_tolerance_is_ok( + self, client: AsyncClient, maker_user: User, db_session: AsyncSession, + ): + recipe = await create_test_recipe(db_session, maker_user.id, code="TOL-C") + task_id = (await client.get( + f"/api/recipes/{recipe.id}/tasks", headers=auth_headers(maker_user), + )).json()[0]["id"] + + resp = await client.post( + f"/api/tasks/{task_id}/subtasks", + headers=auth_headers(maker_user), + json={"marker_number": 5, "description": "Solo utl", "utl": 10.0}, + ) + assert resp.status_code == 201, resp.text + + async def test_update_subtask_that_would_invert_the_order_is_422( + self, client: AsyncClient, maker_user: User, db_session: AsyncSession, + ): + """Partial update: the new value is checked against what stays (#26).""" + recipe = await create_test_recipe(db_session, maker_user.id, code="TOL-D") + subtask_id = (await client.get( + f"/api/recipes/{recipe.id}/tasks", headers=auth_headers(maker_user), + )).json()[0]["subtasks"][0]["id"] + + # Fixture quote: ltl=9.5, lwl=9.7, nominal=10.0, uwl=10.3, utl=10.5. + # Raising ltl past the existing utl inverts the sequence. + resp = await client.put( + f"/api/subtasks/{subtask_id}", + headers=auth_headers(maker_user), + json={"ltl": 11.0}, + ) + assert resp.status_code == 422