diff --git a/CHANGELOG.md b/CHANGELOG.md index e97433f..eda90d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,36 @@ All notable changes to netbox-super-cli are tracked here. Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) loosely. From v1.0.0 onward, releases follow [Semantic Versioning](https://semver.org/) and the version in `pyproject.toml` matches the git tag. Pre-1.0 milestones (Phase 1-5) were pinned by tag while `pyproject.toml` stayed at `0.0.1`. +## v1.6.1 — 2026-06-29 + +Patch release. Fixes the TUI bulk-edit form so custom-field edits are visible and +actually save. + +### Fixed + +- **Bulk-edit include toggles were invisible, so edits silently didn't save** + ([#137]). The per-field include switch in the bulk-edit form was clipped to + zero width by its stylesheet and rendered blank — users couldn't see or flip + it, so no field was opted in and the apply produced no change with no error. + The toggle now sizes to fit its slider. +- **Edit forms showed the raw `custom_fields.` key** ([#137]). The + single-edit and bulk-edit forms, and the change-preview diff, now show a custom + field's human label (e.g. "Site Contact") instead of the raw dotted key, + matching the list-column labels added in v1.6.0. +- **Opting in a custom-field select with an unresolved choice set could silently + null the field** ([#137]). A select whose choices couldn't be fetched rendered + empty; opting it in seeded a null and cleared a value the user never edited. + Opting in a blank select now contributes no change; only the explicit ∅ button + nulls. +- **Bulk-editing a shared custom field could overwrite it on opt-in** ([#137]). + Custom-field widgets didn't seed the value the selected records share, so a + shared boolean rendered as `False` and opting it in unchanged flipped every + record to `False` (and a shared multiselect cleared to empty). Custom-field + widgets now seed the shared value, so opting a field in without editing it is a + no-op. + +[#137]: https://github.com/thomaschristory/netbox-super-cli/issues/137 + ## v1.6.0 — 2026-06-29 Minor release. The interactive TUI gets human-readable custom-field columns, diff --git a/docs/guides/interactive-tui.md b/docs/guides/interactive-tui.md index 42f85bb..2d3f1ef 100644 --- a/docs/guides/interactive-tui.md +++ b/docs/guides/interactive-tui.md @@ -144,9 +144,11 @@ endpoint can filter by this one. Switch tabs with Tab and press ## Bulk editing Select rows on a list with v/Space, then press -B. Choose *which* fields to set (each has an include toggle) and one -value each. Fields are **prepopulated** with the value the selected records -share, so a small tweak doesn't mean retyping. +B. Each field is one row: flip its **include toggle** (the switch on +the left) to opt it into the change, then set its value — a field with the toggle +off is left untouched. Custom fields appear as individual rows under their human +label. Fields are **prepopulated** with the value the selected records share, so +a small tweak doesn't mean retyping. Press p to **preview** — a per-record diff of exactly what will change (records already matching are left untouched). Confirm to apply; a diff --git a/nsc/_version.py b/nsc/_version.py index 1405779..7b3c3b4 100644 --- a/nsc/_version.py +++ b/nsc/_version.py @@ -1,3 +1,3 @@ """Single source of truth for the package version.""" -__version__ = "1.6.0" +__version__ = "1.6.1" diff --git a/nsc/tui/bulk.py b/nsc/tui/bulk.py index c1595f1..4d6a213 100644 --- a/nsc/tui/bulk.py +++ b/nsc/tui/bulk.py @@ -29,6 +29,7 @@ def bulk_diff( bulk_set: dict[str, object], sensitive_paths: tuple[str, ...], new_displays: dict[str, str] | None = None, + field_labels: dict[str, str] | None = None, ) -> list[RecordChange]: """Compute the per-record patch and diff rows for a uniform bulk ``set``. @@ -36,12 +37,13 @@ def bulk_diff( record's current value contributes no patch entry and no row for that record, so heterogeneous current values yield different changed subsets. ``new_displays`` maps a field to the human label of its chosen FK value so - the diff renders the name rather than the id. + the diff renders the name rather than the id. ``field_labels`` maps a field + key to its human label so ``custom_fields.`` rows aren't shown raw. """ changes: list[RecordChange] = [] for record in selected: patch = compute_patch(record, bulk_set) - rows = diff_rows(record, patch, sensitive_paths, new_displays) + rows = diff_rows(record, patch, sensitive_paths, new_displays, field_labels) changes.append(RecordChange(record_id=record.get("id"), patch=patch, rows=rows)) return changes diff --git a/nsc/tui/forms.py b/nsc/tui/forms.py index 9a21930..64e6463 100644 --- a/nsc/tui/forms.py +++ b/nsc/tui/forms.py @@ -35,6 +35,9 @@ class WidgetSpec(BaseModel): kind: WidgetKind name: str + # Human-friendly row label; falls back to ``name`` when blank (regular fields + # keep their key as the label; custom fields carry their NetBox label here). + label: str = "" choices: tuple[str, ...] = () nullable: bool = False sensitive: bool = False @@ -83,15 +86,20 @@ def custom_field_widget(cf: CustomFieldDef) -> WidgetSpec: kind = _CF_KINDS.get(cf.type, "text") nullable = not cf.required if kind == "select": - return WidgetSpec(kind="select", name=name, choices=cf.choices, nullable=nullable) + return WidgetSpec( + kind="select", name=name, label=cf.label, choices=cf.choices, nullable=nullable + ) if kind == "multi_select": return WidgetSpec( kind="multi_select", name=name, + label=cf.label, options=tuple((c, c) for c in cf.choices), nullable=nullable, ) - return WidgetSpec(kind=kind, name=name, nullable=nullable, is_float=cf.type == "decimal") + return WidgetSpec( + kind=kind, name=name, label=cf.label, nullable=nullable, is_float=cf.type == "decimal" + ) def expand_custom_fields(defs: dict[str, CustomFieldDef]) -> list[WidgetSpec]: @@ -242,20 +250,25 @@ def diff_rows( patch: dict[str, object], sensitive_paths: tuple[str, ...], new_displays: dict[str, str] | None = None, + field_labels: dict[str, str] | None = None, ) -> list[DiffRow]: """Render ``patch`` as human-readable old -> new rows for the confirm modal. ``new_displays`` overrides a field's rendered *new* value — used for FK fields whose staged value is a bare id but whose chosen label is known (e.g. picked from the record chooser). The patch still carries the id. + ``field_labels`` overrides a field's *name* — used so a ``custom_fields.`` + key renders as its human label rather than the raw dotted key. """ overrides = new_displays or {} + labels = field_labels or {} rows: list[DiffRow] = [] for name, new_value in patch.items(): + field = labels.get(name, name) if name in sensitive_paths: - rows.append(DiffRow(field=name, old_display="****", new_display="****")) + rows.append(DiffRow(field=field, old_display="****", new_display="****")) continue old_display = fk_display(original[name]) if name in original else "" new_display = overrides.get(name, str(new_value)) - rows.append(DiffRow(field=name, old_display=old_display, new_display=new_display)) + rows.append(DiffRow(field=field, old_display=old_display, new_display=new_display)) return rows diff --git a/nsc/tui/screens/bulk_edit_form.py b/nsc/tui/screens/bulk_edit_form.py index 6351fb4..18721e2 100644 --- a/nsc/tui/screens/bulk_edit_form.py +++ b/nsc/tui/screens/bulk_edit_form.py @@ -100,9 +100,15 @@ def __init__( self._fk_labels: dict[str, str] = {} body = update_op.request_body field_names = list(body.fields) if body is not None else [] + # Custom fields are staged under flattened ``custom_fields.`` keys, + # so seed their shared value from flattened records too — otherwise a + # widget defaults (e.g. a boolean to False) and opting it in unchanged + # would silently overwrite the records' shared value. + cf_names = [f"custom_fields.{cf.name}" for cf in (custom_field_defs or {}).values()] + flattened_records = [flatten_custom_fields(record) for record in selected_records] # Shared current value per field, to seed the widgets (does NOT opt the # field in — the include toggle still gates what gets set). - self._shared = shared_values(selected_records, field_names) + self._shared = shared_values(flattened_records, field_names + cf_names) self.progress_total = 0 self.progress_done = 0 self.title = f"Bulk edit {len(selected_records)} {resource_name}" @@ -111,6 +117,10 @@ def __init__( def bulk_set(self) -> dict[str, Any]: return {name: self._values[name] for name in self._included if name in self._values} + def _field_labels(self) -> dict[str, str]: + """Human labels for fields that carry one (custom fields), for the diff.""" + return {name: spec.label for name, spec in self._specs.items() if spec.label} + def compose(self) -> ComposeResult: yield Header() with VerticalScroll(id="bulk-form-body"): @@ -142,7 +152,7 @@ def _specs_for(self, name: str, field: Any, sensitive: tuple[str, ...]) -> list[ def _compose_field(self, name: str, spec: WidgetSpec) -> ComposeResult: with Horizontal(classes="bulk-field"): yield Switch(value=False, id=f"include-{encode_field_id(name)}", classes="bulk-include") - yield Label(name, classes="bulk-label") + yield Label(spec.label or name, classes="bulk-label") yield from self._compose_widget(name, spec) if spec.nullable and spec.kind != "multi_select": yield Button("∅", id=f"setnull-{encode_field_id(name)}", classes="bulk-setnull") @@ -191,8 +201,16 @@ def _compose_fk(self, name: str) -> ComposeResult: def _compose_widget(self, name: str, spec: WidgetSpec) -> ComposeResult: wid = f"field-{encode_field_id(name)}" if spec.kind == "multi_select": + # Tags seed via spec.selected; a custom-field multiselect seeds its + # shared current list so opting it in unchanged isn't a destructive + # clear (tags are intentionally left blank — they have their own flow). + selected = set(spec.selected) + if not selected and name != "tags": + shared = self._shared.get(name) + if isinstance(shared, list): + selected = {str(v) for v in shared} yield SelectionList[str]( - *(Selection(label, val, val in spec.selected) for label, val in spec.options), + *(Selection(label, val, val in selected) for label, val in spec.options), id=wid, classes="bulk-multiselect", ) @@ -264,7 +282,10 @@ def _read_widget_value(self, name: str) -> Any: return self._multiselect_value(name, self.query_one(wid, SelectionList).selected) if spec is not None and spec.kind == "select": value = self.query_one(wid, Select).value - return None if value is Select.NULL else value + # A blank select (no/unresolved choices, or a current value not in the + # options) must not seed a null on opt-in — that would silently clear a + # field the user never edited. Only the explicit ∅ button nulls. + return _NO_SEED if value is Select.NULL else value if spec is not None and spec.kind == "switch": return self.query_one(wid, Switch).value return self._coerce_input(name, self.query_one(wid, Input).value) @@ -279,6 +300,10 @@ def on_select_changed(self, event: Select.Changed) -> None: name = self._strip(event.select.id, "field-") if name is None: return + # A blank select on a field the user hasn't opted in must not stage a null + # (which would clear the field on apply). Only the explicit ∅ button nulls. + if event.value is Select.NULL and name not in self._included: + return self._values[name] = None if event.value is Select.NULL else event.value def on_selection_list_selected_changed(self, event: SelectionList.SelectedChanged[str]) -> None: @@ -329,7 +354,9 @@ def action_preview(self) -> None: sensitive = body.sensitive_paths if body is not None else () # Flatten custom_fields so each chosen custom_fields. diffs per record. flattened = [flatten_custom_fields(record) for record in self._selected] - changes = bulk_diff(flattened, self.bulk_set, sensitive, self._fk_labels) + changes = bulk_diff( + flattened, self.bulk_set, sensitive, self._fk_labels, self._field_labels() + ) def _on_confirm(confirmed: bool | None) -> None: if confirmed: diff --git a/nsc/tui/screens/edit_form.py b/nsc/tui/screens/edit_form.py index 0604643..27d3804 100644 --- a/nsc/tui/screens/edit_form.py +++ b/nsc/tui/screens/edit_form.py @@ -132,7 +132,7 @@ def _specs_for(self, name: str, field: Any, sensitive: tuple[str, ...]) -> list[ def _compose_field(self, name: str, spec: WidgetSpec) -> ComposeResult: value = _record_value(self._record, name) with Horizontal(classes="edit-field"): - yield Label(name, classes="edit-label") + yield Label(spec.label or name, classes="edit-label") yield from self._compose_widget(name, spec, value) if spec.nullable and spec.kind != "multi_select": yield Button("∅", id=f"setnull-{encode_field_id(name)}", classes="edit-setnull") @@ -292,8 +292,10 @@ def _on_confirm(confirmed: bool | None) -> None: if confirmed: self._apply_patch(patch, sensitive) + labels = {name: spec.label for name, spec in self._specs.items() if spec.label} self.app.push_screen( - DiffModal(diff_rows(self._record, patch, sensitive, self._fk_labels)), _on_confirm + DiffModal(diff_rows(self._record, patch, sensitive, self._fk_labels, labels)), + _on_confirm, ) def _apply_patch(self, patch: dict[str, Any], sensitive_paths: tuple[str, ...]) -> None: diff --git a/nsc/tui/styles.tcss b/nsc/tui/styles.tcss index ea4c086..7d78f7d 100644 --- a/nsc/tui/styles.tcss +++ b/nsc/tui/styles.tcss @@ -68,7 +68,10 @@ BulkEditForm #bulk-form-body { } .bulk-include { - width: 6; + /* A Switch needs room for its border + padding + slider; a too-narrow + width clips the slider to zero so the toggle becomes invisible. */ + width: auto; + padding: 0 1; } .bulk-label { diff --git a/pyproject.toml b/pyproject.toml index 551f9f4..51a8b02 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "netbox-super-cli" -version = "1.6.0" +version = "1.6.1" description = "Dynamic NetBox CLI generated from the live OpenAPI schema." readme = "README.md" requires-python = ">=3.12" diff --git a/tests/tui/test_bulk_edit_form.py b/tests/tui/test_bulk_edit_form.py index 279502d..37724ec 100644 --- a/tests/tui/test_bulk_edit_form.py +++ b/tests/tui/test_bulk_edit_form.py @@ -1,10 +1,11 @@ from __future__ import annotations +from pathlib import Path from typing import Any import pytest from textual.app import App, ComposeResult -from textual.widgets import Button, Input, ListView, Select, SelectionList, Static, Switch +from textual.widgets import Button, Input, Label, ListView, Select, SelectionList, Static, Switch from nsc.http.errors import NetBoxAPIError from nsc.model.command_model import ( @@ -18,7 +19,8 @@ ) from nsc.savedfilters.custom_fields import CustomFieldDef from nsc.savedfilters.tags import TagDef -from nsc.tui.forms import SET_NULL, tags_payload +from nsc.tui.bulk import bulk_diff +from nsc.tui.forms import SET_NULL, flatten_custom_fields, tags_payload from nsc.tui.screens.bulk_edit_form import BulkEditForm from nsc.tui.screens.list import ListScreen from nsc.tui.screens.record_picker import RecordPicker @@ -886,6 +888,37 @@ def _cf_screen(app: _CfBulkApp) -> BulkEditForm: return screen +class _StyledCfBulkApp(_CfBulkApp): + """Same form, but with the real stylesheet loaded so layout is exercised.""" + + CSS_PATH = str(Path(__file__).resolve().parents[2] / "nsc" / "tui" / "styles.tcss") + + +@pytest.mark.asyncio +async def test_include_toggle_is_not_clipped_to_zero_width() -> None: + # Regression: a too-narrow `.bulk-include` clipped the Switch slider to zero, + # making every include toggle invisible so fields could never be opted in. + app = _StyledCfBulkApp(_SpyClient([])) + async with app.run_test(size=(120, 60)) as pilot: + await pilot.pause() + screen = _cf_screen(app) + for sw in screen.query(".bulk-include").results(Switch): + assert sw.content_region.width > 0, f"{sw.id} slider clipped to zero width" + + +@pytest.mark.asyncio +async def test_custom_field_rows_show_human_labels_not_raw_keys() -> None: + app = _CfBulkApp(_SpyClient([])) + async with app.run_test(size=(120, 60)) as pilot: + await pilot.pause() + screen = _cf_screen(app) + labels = {w.render().plain for w in screen.query(".bulk-label").results(Label)} + assert "Tier" in labels + assert "Count" in labels + assert "custom_fields.tier" not in labels + assert "custom_fields.count" not in labels + + @pytest.mark.asyncio async def test_custom_fields_expand_into_per_field_widgets_with_toggles() -> None: app = _CfBulkApp(_SpyClient([])) @@ -908,6 +941,106 @@ async def test_tags_render_as_selection_list() -> None: assert isinstance(screen.query_one("#field-tags"), SelectionList) +@pytest.mark.asyncio +async def test_bulk_diff_preview_shows_custom_field_label_not_raw_key() -> None: + app = _CfBulkApp(_SpyClient([])) + async with app.run_test(size=(120, 60)) as pilot: + await pilot.pause() + screen = _cf_screen(app) + screen._included.add("custom_fields.tier") + screen._values["custom_fields.tier"] = "gold" + screen.action_preview() + await pilot.pause() + modal = app.screen + assert isinstance(modal, BulkDiffModal) + text = modal.render_text() + assert "Tier:" in text + assert "custom_fields.tier" not in text + + +@pytest.mark.asyncio +async def test_custom_field_ui_edit_saves_end_to_end() -> None: + """Guard: a UI edit + opt-in of a custom field sends the nested PATCH.""" + client = _SpyClient([]) + app = _CfBulkApp(client) + async with app.run_test(size=(120, 60)) as pilot: + await pilot.pause() + screen = _cf_screen(app) + screen.query_one("#field-custom_fields-tier", Select).value = "gold" + await pilot.pause() + await pilot.click("#include-custom_fields-tier") + await pilot.pause() + screen.action_preview() + await pilot.pause() + await pilot.press("enter") + await app.workers.wait_for_complete() + await pilot.pause() + assert client.patch_calls + for call in client.patch_calls: + assert call["json"] == {"custom_fields": {"tier": "gold"}} + + +@pytest.mark.asyncio +async def test_opting_in_shared_boolean_cf_without_editing_is_a_noop() -> None: + # Records share custom_fields.flag == True. The widget must seed that shared + # value so opting it in (no edit) does NOT silently flip every record to False. + defs = {"flag": CustomFieldDef("flag", "Flag", type="boolean")} + sel = [ + {"id": 1, "name": "a", "custom_fields": {"flag": True}, "tags": []}, + {"id": 2, "name": "b", "custom_fields": {"flag": True}, "tags": []}, + ] + app = _CfBulkApp(_SpyClient([]), defs=defs, tags=None, selected=sel) + async with app.run_test(size=(120, 60)) as pilot: + await pilot.pause() + screen = _cf_screen(app) + assert screen.query_one("#field-custom_fields-flag", Switch).value is True + screen.query_one("#include-custom_fields-flag", Switch).value = True + await pilot.pause() + # Opted in but unchanged: stages the shared True, so no per-record patch. + flattened = [flatten_custom_fields(r) for r in sel] + changes = bulk_diff(flattened, screen.bulk_set, ()) + assert all(not c.patch for c in changes) + + +@pytest.mark.asyncio +async def test_opting_in_shared_multiselect_cf_without_editing_is_a_noop() -> None: + # Records share a multiselect custom field; opting it in unchanged must not + # clear it to an empty list. + defs = {"envs": CustomFieldDef("envs", "Envs", type="multiselect", choices=("dev", "qa"))} + sel = [ + {"id": 1, "name": "a", "custom_fields": {"envs": ["dev", "qa"]}, "tags": []}, + {"id": 2, "name": "b", "custom_fields": {"envs": ["dev", "qa"]}, "tags": []}, + ] + app = _CfBulkApp(_SpyClient([]), defs=defs, tags=None, selected=sel) + async with app.run_test(size=(120, 60)) as pilot: + await pilot.pause() + screen = _cf_screen(app) + screen.query_one("#include-custom_fields-envs", Switch).value = True + await pilot.pause() + flattened = [flatten_custom_fields(r) for r in sel] + changes = bulk_diff(flattened, screen.bulk_set, ()) + assert all(not c.patch for c in changes) + + +@pytest.mark.asyncio +async def test_opting_in_blank_select_does_not_null_the_field() -> None: + # A select whose choice-set didn't resolve renders empty (value == NULL). + # Opting it in must NOT silently null a value the user couldn't even pick; + # only the explicit ∅ button should null. So it contributes no patch. + defs = {"region": CustomFieldDef("region", "Region", type="select", choices=())} + sel = [ + {"id": 1, "name": "a", "custom_fields": {"region": "eu"}, "tags": []}, + {"id": 2, "name": "b", "custom_fields": {"region": "eu"}, "tags": []}, + ] + app = _CfBulkApp(_SpyClient([]), defs=defs, tags=None, selected=sel) + async with app.run_test(size=(120, 60)) as pilot: + await pilot.pause() + screen = _cf_screen(app) + screen.query_one("#include-custom_fields-region", Switch).value = True + await pilot.pause() + assert "custom_fields.region" not in screen.bulk_set + + @pytest.mark.asyncio async def test_bulk_apply_nests_custom_field_value() -> None: client = _SpyClient([]) diff --git a/tests/tui/test_edit_form.py b/tests/tui/test_edit_form.py index 74b5ddc..d4e5714 100644 --- a/tests/tui/test_edit_form.py +++ b/tests/tui/test_edit_form.py @@ -4,7 +4,7 @@ import pytest from textual.app import App, ComposeResult -from textual.widgets import Button, Input, ListView, Select, SelectionList, Static, Switch +from textual.widgets import Button, Input, Label, ListView, Select, SelectionList, Static, Switch from nsc.http.errors import NetBoxAPIError from nsc.model.command_model import ( @@ -684,6 +684,33 @@ async def test_edit_expands_custom_field_widget() -> None: assert isinstance(screen.query_one("#field-tags"), SelectionList) +@pytest.mark.asyncio +async def test_edit_custom_field_row_shows_human_label_not_raw_key() -> None: + record = {"id": 5, "name": "sw1", "custom_fields": {"tier": "silver"}, "tags": []} + app = _CfEditApp(_SpyClient([]), record) + async with app.run_test(size=(120, 60)) as pilot: + await pilot.pause() + screen = _cf_edit_screen(app) + labels = {w.render().plain for w in screen.query(".edit-label").results(Label)} + assert "Tier" in labels + assert "custom_fields.tier" not in labels + + +@pytest.mark.asyncio +async def test_edit_diff_modal_shows_custom_field_label_not_raw_key() -> None: + record = {"id": 5, "name": "sw1", "custom_fields": {"tier": "silver"}, "tags": []} + app = _CfEditApp(_SpyClient([]), record) + async with app.run_test(size=(120, 60)) as pilot: + await pilot.pause() + screen = _cf_edit_screen(app) + screen.staged["custom_fields.tier"] = "gold" + screen.action_save() + await pilot.pause() + assert isinstance(app.screen, DiffModal) + fields = [row.field for row in app.screen._rows] + assert fields == ["Tier"] + + @pytest.mark.asyncio async def test_edit_saves_custom_field_nested() -> None: client = _SpyClient([]) diff --git a/tests/tui/test_forms.py b/tests/tui/test_forms.py index ff7ed9e..0950065 100644 --- a/tests/tui/test_forms.py +++ b/tests/tui/test_forms.py @@ -132,6 +132,21 @@ def test_diff_rows_override_absent_falls_back_to_str() -> None: assert rows == [DiffRow(field="status", old_display="active", new_display="offline")] +def test_diff_rows_uses_field_label_for_custom_field_key() -> None: + rows = diff_rows( + {"custom_fields.tier": "silver"}, + {"custom_fields.tier": "gold"}, + (), + field_labels={"custom_fields.tier": "Tier"}, + ) + assert rows == [DiffRow(field="Tier", old_display="silver", new_display="gold")] + + +def test_diff_rows_field_label_absent_falls_back_to_raw_key() -> None: + rows = diff_rows({"status": "active"}, {"status": "offline"}, (), field_labels={}) + assert rows == [DiffRow(field="status", old_display="active", new_display="offline")] + + # --- #134: custom-field expansion, tags multi-select, payload nesting --- from nsc.savedfilters.custom_fields import CustomFieldDef # noqa: E402 diff --git a/uv.lock b/uv.lock index 3989515..962750a 100644 --- a/uv.lock +++ b/uv.lock @@ -675,7 +675,7 @@ wheels = [ [[package]] name = "netbox-super-cli" -version = "1.6.0" +version = "1.6.1" source = { editable = "." } dependencies = [ { name = "click" },