From 2b59c3ec491ca6bcd73e16f7854d1865433298d9 Mon Sep 17 00:00:00 2001 From: AdrianoDev Date: Mon, 21 Sep 2026 23:05:51 +0200 Subject: [PATCH] fix(recipe): measurement_interval_minutes e image_path si possono di nuovo azzerare (#37.3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit recipe_editor.html saveRecipe(): i due campi vengono sempre inviati nel payload (valore o null esplicito), non più omessi quando svuotati. recipe_service.py _header_fields(): per i due campi (unica eccezione concordata al confine dei lotti, righe ~37-60) distingue "non inviato" da "inviato null" con model_fields_set, invece di getattr(...) is not None — altrimenti un null esplicito veniva scartato allo stesso modo di un campo assente e il valore restava bloccato per sempre. Gli altri campi header (name, description, requires_*, allow_manual_input) non toccati. Test backend in test_recipes.py: TestUpdateRecipeClearNullableFields. Co-Authored-By: Claude Sonnet 5 --- src/backend/services/recipe_service.py | 33 +++++++-- src/backend/tests/test_recipes.py | 73 +++++++++++++++++++ .../templates/maker/recipe_editor.html | 17 +++-- 3 files changed, 109 insertions(+), 14 deletions(-) diff --git a/src/backend/services/recipe_service.py b/src/backend/services/recipe_service.py index 5ebf1e1..125658f 100644 --- a/src/backend/services/recipe_service.py +++ b/src/backend/services/recipe_service.py @@ -34,13 +34,34 @@ HEADER_FIELDS = ( ) +# image_path and measurement_interval_minutes are nullable columns where the +# caller may deliberately want to clear the value (send null), not merely +# leave it untouched by not mentioning the field at all. A plain +# "getattr(...) is not None" check cannot tell those two cases apart - +# once set, neither field could ever be cleared again (#37.3). The other +# header fields keep that simpler check: booleans are always sent by the +# client (never null) and clearing name/description to null is not a case +# this endpoint needs to support. +_NULLABLE_HEADER_FIELDS = ("image_path", "measurement_interval_minutes") + + def _header_fields(data: RecipeUpdate) -> dict: - """The header columns the caller actually asked to change.""" - return { - field: getattr(data, field) - for field in HEADER_FIELDS - if getattr(data, field) is not None - } + """The header columns the caller actually asked to change. + + For image_path/measurement_interval_minutes, "asked to change" includes + explicitly setting it to null - told apart from "field absent from the + request" via model_fields_set, since RecipeUpdate defaults both to None. + """ + result = {} + for field in HEADER_FIELDS: + if field in _NULLABLE_HEADER_FIELDS: + if field in data.model_fields_set: + result[field] = getattr(data, field) + else: + value = getattr(data, field) + if value is not None: + result[field] = value + return result async def _get_recipe_or_404(db: AsyncSession, recipe_id: int) -> Recipe: diff --git a/src/backend/tests/test_recipes.py b/src/backend/tests/test_recipes.py index 154339c..38dff83 100644 --- a/src/backend/tests/test_recipes.py +++ b/src/backend/tests/test_recipes.py @@ -250,6 +250,79 @@ class TestUpdateRecipe: assert version_numbers == [1, 2] +class TestUpdateRecipeClearNullableFields: + """gitea#37.3: measurement_interval_minutes/image_path, once set, could + never be cleared again - the request set them and the update endpoint + could not tell "not sent" apart from "sent as null" (both landed on the + same getattr(...) is not None check in recipe_service._header_fields).""" + + async def test_explicit_null_clears_measurement_interval( + self, client: AsyncClient, maker_user: User, db_session: AsyncSession, + ): + recipe = await create_test_recipe(db_session, maker_user.id) + + set_resp = await client.put( + f"/api/recipes/{recipe.id}", + headers=auth_headers(maker_user), + json={"measurement_interval_minutes": 30}, + ) + assert set_resp.status_code == 200 + assert set_resp.json()["measurement_interval_minutes"] == 30 + + clear_resp = await client.put( + f"/api/recipes/{recipe.id}", + headers=auth_headers(maker_user), + json={"measurement_interval_minutes": None}, + ) + assert clear_resp.status_code == 200 + assert clear_resp.json()["measurement_interval_minutes"] is None + + async def test_explicit_null_clears_image_path( + self, client: AsyncClient, maker_user: User, db_session: AsyncSession, + ): + recipe = await create_test_recipe(db_session, maker_user.id) + + set_resp = await client.put( + f"/api/recipes/{recipe.id}", + headers=auth_headers(maker_user), + json={"image_path": "uploads/1/1/preview.png"}, + ) + assert set_resp.status_code == 200 + assert set_resp.json()["image_path"] == "uploads/1/1/preview.png" + + clear_resp = await client.put( + f"/api/recipes/{recipe.id}", + headers=auth_headers(maker_user), + json={"image_path": None}, + ) + assert clear_resp.status_code == 200 + assert clear_resp.json()["image_path"] is None + + async def test_field_not_sent_leaves_existing_value_untouched( + self, client: AsyncClient, maker_user: User, db_session: AsyncSession, + ): + """Regression guard for the other half of the distinction: a request + that never mentions the field must still leave it as it was.""" + recipe = await create_test_recipe(db_session, maker_user.id) + + set_resp = await client.put( + f"/api/recipes/{recipe.id}", + headers=auth_headers(maker_user), + json={"measurement_interval_minutes": 45}, + ) + assert set_resp.status_code == 200 + assert set_resp.json()["measurement_interval_minutes"] == 45 + + unrelated_resp = await client.put( + f"/api/recipes/{recipe.id}", + headers=auth_headers(maker_user), + json={"name": "Renamed Recipe"}, + ) + assert unrelated_resp.status_code == 200 + assert unrelated_resp.json()["measurement_interval_minutes"] == 45 + assert unrelated_resp.json()["name"] == "Renamed Recipe" + + class TestDeleteRecipe: """DELETE /api/recipes/{recipe_id} tests.""" diff --git a/src/frontend/flask_app/templates/maker/recipe_editor.html b/src/frontend/flask_app/templates/maker/recipe_editor.html index bb30c69..a4a58fb 100644 --- a/src/frontend/flask_app/templates/maker/recipe_editor.html +++ b/src/frontend/flask_app/templates/maker/recipe_editor.html @@ -556,10 +556,13 @@ function recipeEditor() { description: this.description.trim(), }; - // Measurement interval - if (this.measurementIntervalMinutes) { - payload.measurement_interval_minutes = parseInt(this.measurementIntervalMinutes, 10); - } + // Measurement interval - always sent, explicit null when cleared + // (#37.3): omitting the key when the field was emptied used to leave + // the old value in place forever, because the backend could not tell + // "not sent" from "cleared". + payload.measurement_interval_minutes = this.measurementIntervalMinutes + ? parseInt(this.measurementIntervalMinutes, 10) + : null; // Rules of measurement. Sent every time, including when false: these are the // settings an operator gets told about, so turning one off has to travel. @@ -567,10 +570,8 @@ function recipeEditor() { payload.requires_serial = !!this.requiresSerial; payload.allow_manual_input = !!this.allowManualInput; - // Include image_path for preview thumbnail - if (this.currentFilePath) { - payload.image_path = this.currentFilePath; - } + // image_path for preview thumbnail - same null-when-cleared rule (#37.3). + payload.image_path = this.currentFilePath || null; // Include change notes if editing if (!this.isNew && this.changeNotes.trim()) {