diff --git a/backend/testjam/routers/custom_fields.py b/backend/testjam/routers/custom_fields.py index 0b85925..af23486 100644 --- a/backend/testjam/routers/custom_fields.py +++ b/backend/testjam/routers/custom_fields.py @@ -1,5 +1,6 @@ from __future__ import annotations +import json from datetime import datetime, timezone from fastapi import APIRouter, Depends, HTTPException, status @@ -164,6 +165,8 @@ def update_custom_field( update_data = body.model_dump(exclude_unset=True) if "options" in update_data: _enforce_options_payload_rule(definition.type, update_data["options"]) + if definition.type in SELECT_TYPES: + _guard_options_change(db, definition, update_data) if update_data.get("required") and definition.entity_type in REQUIRES_DEFAULT_ENTITY_TYPES: new_default = update_data.get("default_value", definition.default_value) if new_default is None: @@ -273,6 +276,88 @@ def _enforce_options_payload_rule( } +def _guard_options_change( + db: Session, + definition: CustomFieldDefinition, + update_data: dict, +) -> None: + """Refuse option edits that orphan existing values or invalidate the default. + + Admins routinely curate option lists after the fact, but two failure + modes are silent today: + + * Removing an option that is still referenced from rows leaves those + values as ghosts — the UI cannot re-select them and a PUT that + omits ``custom_fields`` drops them entirely. + * Replacing the option set without revisiting ``default_value`` makes + future create calls explode with a 422 the user can't act on. + + We refuse both with a 409 / 422 that names the offending values so + the admin can archive the field, migrate rows, or update the default + before retrying. + """ + new_options = update_data["options"] or [] + new_values = {option["value"] for option in new_options} + old_values = {option["value"] for option in (definition.options or [])} + removed = old_values - new_values + if removed: + in_use = _option_values_in_use(db, definition) + blocking = sorted(removed & in_use) + if blocking: + raise HTTPException( + status_code=409, + detail=( + f"Cannot remove option(s) {blocking}: still referenced " + f"by existing rows. Migrate the values or archive the " + f"field instead." + ), + ) + + next_default = update_data.get("default_value", definition.default_value) + if next_default is None: + return + if definition.type == "single_select" and next_default not in new_values: + raise HTTPException( + status_code=422, + detail=( + f"default_value '{next_default}' is not one of the new " + f"options. Update or clear the default before changing options." + ), + ) + if definition.type == "multi_select": + invalid = sorted(item for item in (next_default or []) if item not in new_values) + if invalid: + raise HTTPException( + status_code=422, + detail=( + f"default_value entries {invalid} are not in the new " + f"options. Update or clear the default before changing options." + ), + ) + + +def _option_values_in_use( + db: Session, definition: CustomFieldDefinition +) -> set[str]: + table = ENTITY_TABLE.get(definition.entity_type) + if table is None: + return set() + rows = db.execute(text(f"SELECT custom_fields FROM {table}")).fetchall() + seen: set[str] = set() + for (raw,) in rows: + if raw is None: + continue + data = raw if isinstance(raw, dict) else json.loads(raw) + value = data.get(definition.key) + if value is None: + continue + if isinstance(value, list): + seen.update(item for item in value if isinstance(item, str)) + elif isinstance(value, str): + seen.add(value) + return seen + + def _is_definition_in_use(db: Session, definition: CustomFieldDefinition) -> bool: table = ENTITY_TABLE.get(definition.entity_type) if table is None: diff --git a/backend/tests/test_custom_fields_update_options.py b/backend/tests/test_custom_fields_update_options.py new file mode 100644 index 0000000..d7edc0f --- /dev/null +++ b/backend/tests/test_custom_fields_update_options.py @@ -0,0 +1,183 @@ +"""Option-edit guards on ``PUT /custom-fields/{id}``. + +Admins occasionally curate option lists after a select field is live. +Two failure modes are silent without these guards: + +* Removing an option that is still stored on existing rows leaves those + values as ghosts — the form can't re-select them and a follow-up PUT + that omits ``custom_fields`` strips them entirely. +* Replacing the option set without revisiting ``default_value`` makes + future creates explode with a 422 the user can't act on (the default + applies, then validation rejects it). + +These tests pin both behaviours so a regression is caught immediately. +""" +import pytest + +CUSTOM_FIELDS_PATH = "/api/v1/projects/{project_id}/custom-fields" + + +def _create_priority(client, project_id, **overrides): + payload = { + "entity_type": "bug", + "key": "priority", + "label": "Priority", + "type": "single_select", + "options": [ + {"value": "p0", "label": "P0"}, + {"value": "p1", "label": "P1"}, + {"value": "p2", "label": "P2"}, + ], + } + payload.update(overrides) + response = client.post( + CUSTOM_FIELDS_PATH.format(project_id=project_id), json=payload, + ) + assert response.status_code == 201, response.text + return response.json() + + +def _create_components(client, project_id): + return _create_priority( + client, project_id, + key="components", label="Components", type="multi_select", + options=[ + {"value": "api", "label": "API"}, + {"value": "ui", "label": "UI"}, + {"value": "docs", "label": "Docs"}, + ], + ) + + +def _create_bug(client, project_id, custom_fields=None): + body = {"title": "Bug"} + if custom_fields is not None: + body["custom_fields"] = custom_fields + response = client.post(f"/api/v1/projects/{project_id}/bugs", json=body) + assert response.status_code == 201, response.text + return response.json() + + +@pytest.fixture +def priority_field(auth_client, project_id): + return _create_priority(auth_client, project_id) + + +@pytest.fixture +def components_field(auth_client, project_id): + return _create_components(auth_client, project_id) + + +def test_removing_unused_option_is_allowed(auth_client, project_id, priority_field): + response = auth_client.put( + f"/api/v1/custom-fields/{priority_field['id']}", + json={"options": [ + {"value": "p0", "label": "P0"}, + {"value": "p1", "label": "P1"}, + ]}, + ) + + assert response.status_code == 200, response.text + assert {opt["value"] for opt in response.json()["options"]} == {"p0", "p1"} + + +def test_removing_option_referenced_by_a_bug_is_refused(auth_client, project_id, priority_field): + _create_bug(auth_client, project_id, custom_fields={"priority": "p2"}) + + response = auth_client.put( + f"/api/v1/custom-fields/{priority_field['id']}", + json={"options": [ + {"value": "p0", "label": "P0"}, + {"value": "p1", "label": "P1"}, + ]}, + ) + + assert response.status_code == 409 + assert "p2" in response.json()["detail"] + + +def test_removing_multi_select_option_referenced_by_a_bug_is_refused( + auth_client, project_id, components_field, +): + _create_bug(auth_client, project_id, custom_fields={"components": ["api", "docs"]}) + + response = auth_client.put( + f"/api/v1/custom-fields/{components_field['id']}", + json={"options": [ + {"value": "api", "label": "API"}, + {"value": "ui", "label": "UI"}, + ]}, + ) + + assert response.status_code == 409 + assert "docs" in response.json()["detail"] + + +def test_replacing_options_invalidates_existing_default(auth_client, project_id): + field = _create_priority(auth_client, project_id, default_value="p0") + + response = auth_client.put( + f"/api/v1/custom-fields/{field['id']}", + json={"options": [ + {"value": "p1", "label": "P1"}, + {"value": "p2", "label": "P2"}, + ]}, + ) + + assert response.status_code == 422 + assert "p0" in response.json()["detail"] + + +def test_replacing_options_and_default_together_is_accepted(auth_client, project_id): + field = _create_priority(auth_client, project_id, default_value="p0") + + response = auth_client.put( + f"/api/v1/custom-fields/{field['id']}", + json={ + "options": [ + {"value": "p1", "label": "P1"}, + {"value": "p2", "label": "P2"}, + ], + "default_value": "p1", + }, + ) + + assert response.status_code == 200, response.text + assert response.json()["default_value"] == "p1" + + +def test_clearing_default_alongside_options_change_is_accepted(auth_client, project_id): + field = _create_priority(auth_client, project_id, default_value="p0") + + response = auth_client.put( + f"/api/v1/custom-fields/{field['id']}", + json={ + "options": [ + {"value": "p1", "label": "P1"}, + {"value": "p2", "label": "P2"}, + ], + "default_value": None, + }, + ) + + assert response.status_code == 200, response.text + assert response.json()["default_value"] is None + + +def test_multi_select_default_revalidated_against_new_options(auth_client, project_id): + field = _create_components(auth_client, project_id) + auth_client.put( + f"/api/v1/custom-fields/{field['id']}", + json={"default_value": ["api", "ui"]}, + ) + + response = auth_client.put( + f"/api/v1/custom-fields/{field['id']}", + json={"options": [ + {"value": "api", "label": "API"}, + {"value": "docs", "label": "Docs"}, + ]}, + ) + + assert response.status_code == 422 + assert "ui" in response.json()["detail"]