From 44ec7f541cef8bdcbde275aeb90219053b628187 Mon Sep 17 00:00:00 2001 From: aamj Date: Fri, 28 Aug 2026 16:30:38 -0300 Subject: [PATCH 1/2] CLI TH configuration using editor application --- README.md | 9 + tests/test_project_commands.py | 340 +++++++++++++++++++++++++++++++++ th_cli/commands/project.py | 182 ++++++++++++++++++ 3 files changed, 531 insertions(+) diff --git a/README.md b/README.md index 462c335..f4e63f7 100644 --- a/README.md +++ b/README.md @@ -165,6 +165,15 @@ Run `th-cli delete-project --id {id}` to delete a project. Run `th-cli update-project --id {id} --config {config file}` to update a project. Both parameters are required. Config must be a full test environment config file. +### edit-project + +Run `th-cli project edit --id {id}` to interactively edit a project's config in your +local editor ($EDITOR/$VISUAL). Only the `config` portion is editable. On save, the +CLI checks for new/unknown keys not present in the original config (the backend does +not reject all unknown fields) and asks for confirmation before persisting. If the +JSON is invalid or the server rejects the change, the editor reopens with your last +edit preserved so nothing is lost. + ## Command Colors By default, the CLI application presents colored texts for all the available commands, specially for the log of test run executions from the `th-cli run-tests` command. If the users need to disable the colors from the tool's output, they may use one of the options presented below: diff --git a/tests/test_project_commands.py b/tests/test_project_commands.py index 15ba9de..0f7c91e 100644 --- a/tests/test_project_commands.py +++ b/tests/test_project_commands.py @@ -522,6 +522,346 @@ def test_update_project_help_message(self, cli_runner: CliRunner) -> None: assert "--config" in result.output +@pytest.mark.unit +@pytest.mark.cli +class TestEditProjectCommand: + """Test cases for the edit_project command.""" + + def test_edit_project_success( + self, cli_runner: CliRunner, mock_sync_apis: Mock, sample_project: api_models.Project + ) -> None: + """Test successful project edit with a single field changed.""" + # Arrange + edited_config = json.loads(json.dumps(sample_project.config)) + edited_config["dut_config"]["setup_code"] = "99999999" + + mock_sync_apis.projects_api.read_project_api_v1_projects__id__get.return_value = sample_project + mock_sync_apis.projects_api.update_project_api_v1_projects__id__put.return_value = sample_project + + with patch("th_cli.commands.project.SyncApis", return_value=mock_sync_apis): + with patch("th_cli.commands.project.click.edit", return_value=json.dumps(edited_config, indent=2)): + # Act + result = cli_runner.invoke(project, ["edit", "--id", "1"]) + + # Assert + assert result.exit_code == 0 + assert "Project 'Test Project' was updated." in result.output + mock_sync_apis.projects_api.update_project_api_v1_projects__id__put.assert_called_once() + _, kwargs = mock_sync_apis.projects_api.update_project_api_v1_projects__id__put.call_args + assert kwargs["body"].config["dut_config"]["setup_code"] == "99999999" + + def test_edit_project_no_changes_made_aborts( + self, cli_runner: CliRunner, mock_sync_apis: Mock, sample_project: api_models.Project + ) -> None: + """Test that returning None from click.edit (no save) aborts cleanly.""" + # Arrange + mock_sync_apis.projects_api.read_project_api_v1_projects__id__get.return_value = sample_project + + with patch("th_cli.commands.project.SyncApis", return_value=mock_sync_apis): + with patch("th_cli.commands.project.click.edit", return_value=None): + # Act + result = cli_runner.invoke(project, ["edit", "--id", "1"]) + + # Assert + assert result.exit_code == 0 + assert "No changes made" in result.output + mock_sync_apis.projects_api.update_project_api_v1_projects__id__put.assert_not_called() + + def test_edit_project_identical_content_aborts( + self, cli_runner: CliRunner, mock_sync_apis: Mock, sample_project: api_models.Project + ) -> None: + """Test that saving unchanged content aborts without calling the API.""" + # Arrange + mock_sync_apis.projects_api.read_project_api_v1_projects__id__get.return_value = sample_project + unchanged_text = json.dumps(sample_project.config, indent=2) + + with patch("th_cli.commands.project.SyncApis", return_value=mock_sync_apis): + with patch("th_cli.commands.project.click.edit", return_value=unchanged_text): + # Act + result = cli_runner.invoke(project, ["edit", "--id", "1"]) + + # Assert + assert result.exit_code == 0 + assert "No changes detected" in result.output + mock_sync_apis.projects_api.update_project_api_v1_projects__id__put.assert_not_called() + + def test_edit_project_invalid_json_retries_then_succeeds( + self, cli_runner: CliRunner, mock_sync_apis: Mock, sample_project: api_models.Project + ) -> None: + """Test that invalid JSON reopens the editor and a subsequent valid save succeeds.""" + # Arrange + edited_config = json.loads(json.dumps(sample_project.config)) + edited_config["dut_config"]["setup_code"] = "99999999" + + mock_sync_apis.projects_api.read_project_api_v1_projects__id__get.return_value = sample_project + mock_sync_apis.projects_api.update_project_api_v1_projects__id__put.return_value = sample_project + + with patch("th_cli.commands.project.SyncApis", return_value=mock_sync_apis): + with patch( + "th_cli.commands.project.click.edit", + side_effect=["{ invalid json", json.dumps(edited_config, indent=2)], + ) as mock_edit: + # Act + result = cli_runner.invoke(project, ["edit", "--id", "1"]) + + # Assert + assert result.exit_code == 0 + assert "Invalid JSON" in result.output + assert mock_edit.call_count == 2 + mock_sync_apis.projects_api.update_project_api_v1_projects__id__put.assert_called_once() + + def test_edit_project_invalid_json_exceeds_retries_aborts( + self, cli_runner: CliRunner, mock_sync_apis: Mock, sample_project: api_models.Project + ) -> None: + """Test that persistently invalid JSON exhausts retries and aborts.""" + # Arrange + mock_sync_apis.projects_api.read_project_api_v1_projects__id__get.return_value = sample_project + + with patch("th_cli.commands.project.SyncApis", return_value=mock_sync_apis): + with patch("th_cli.commands.project.click.edit", return_value="{ still invalid") as mock_edit: + # Act + result = cli_runner.invoke(project, ["edit", "--id", "1"]) + + # Assert + assert result.exit_code == 1 + assert "Exceeded maximum retry attempts" in result.output + assert mock_edit.call_count == 3 + mock_sync_apis.projects_api.update_project_api_v1_projects__id__put.assert_not_called() + + def test_edit_project_new_key_prompts_confirm_accept( + self, cli_runner: CliRunner, mock_sync_apis: Mock, sample_project: api_models.Project + ) -> None: + """Test that a new top-level key triggers a confirmation prompt, and accepting proceeds.""" + # Arrange + edited_config = json.loads(json.dumps(sample_project.config)) + edited_config["new_field"] = "value" + + mock_sync_apis.projects_api.read_project_api_v1_projects__id__get.return_value = sample_project + mock_sync_apis.projects_api.update_project_api_v1_projects__id__put.return_value = sample_project + + with patch("th_cli.commands.project.SyncApis", return_value=mock_sync_apis): + with patch("th_cli.commands.project.click.edit", return_value=json.dumps(edited_config, indent=2)): + with patch("th_cli.commands.project.click.confirm", return_value=True) as mock_confirm: + # Act + result = cli_runner.invoke(project, ["edit", "--id", "1"]) + + # Assert + assert result.exit_code == 0 + assert "new_field" in result.output + mock_confirm.assert_called_once() + mock_sync_apis.projects_api.update_project_api_v1_projects__id__put.assert_called_once() + + def test_edit_project_new_key_prompts_confirm_decline_retries( + self, cli_runner: CliRunner, mock_sync_apis: Mock, sample_project: api_models.Project + ) -> None: + """Test that declining the new-key confirmation reopens the editor for another attempt.""" + # Arrange + config_with_new_key = json.loads(json.dumps(sample_project.config)) + config_with_new_key["new_field"] = "value" + + fixed_config = json.loads(json.dumps(sample_project.config)) + fixed_config["dut_config"]["setup_code"] = "99999999" + + mock_sync_apis.projects_api.read_project_api_v1_projects__id__get.return_value = sample_project + mock_sync_apis.projects_api.update_project_api_v1_projects__id__put.return_value = sample_project + + with patch("th_cli.commands.project.SyncApis", return_value=mock_sync_apis): + with patch( + "th_cli.commands.project.click.edit", + side_effect=[ + json.dumps(config_with_new_key, indent=2), + json.dumps(fixed_config, indent=2), + ], + ) as mock_edit: + with patch("th_cli.commands.project.click.confirm", return_value=False): + # Act + result = cli_runner.invoke(project, ["edit", "--id", "1"]) + + # Assert + assert result.exit_code == 0 + assert mock_edit.call_count == 2 + mock_sync_apis.projects_api.update_project_api_v1_projects__id__put.assert_called_once() + + def test_edit_project_nested_new_key_detected( + self, cli_runner: CliRunner, mock_sync_apis: Mock, sample_project: api_models.Project + ) -> None: + """Test that a new key nested below the top level is detected via the dotted-path diff.""" + # Arrange + edited_config = json.loads(json.dumps(sample_project.config)) + edited_config["network"]["wifi"]["new_nested_key"] = "value" + + mock_sync_apis.projects_api.read_project_api_v1_projects__id__get.return_value = sample_project + mock_sync_apis.projects_api.update_project_api_v1_projects__id__put.return_value = sample_project + + with patch("th_cli.commands.project.SyncApis", return_value=mock_sync_apis): + with patch("th_cli.commands.project.click.edit", return_value=json.dumps(edited_config, indent=2)): + with patch("th_cli.commands.project.click.confirm", return_value=True): + # Act + result = cli_runner.invoke(project, ["edit", "--id", "1"]) + + # Assert + assert result.exit_code == 0 + assert "network.wifi.new_nested_key" in result.output + + def test_edit_project_backend_422_string_detail_retries( + self, cli_runner: CliRunner, mock_sync_apis: Mock, sample_project: api_models.Project + ) -> None: + """Test that a string-shaped 422 detail is surfaced and the editor reopens to retry.""" + # Arrange + edited_config = json.loads(json.dumps(sample_project.config)) + edited_config["dut_config"]["setup_code"] = "99999999" + + api_exception = UnexpectedResponse(status_code=422, content={"detail": "some validation error message"}) + + mock_sync_apis.projects_api.read_project_api_v1_projects__id__get.return_value = sample_project + mock_sync_apis.projects_api.update_project_api_v1_projects__id__put.side_effect = [ + api_exception, + sample_project, + ] + + with patch("th_cli.commands.project.SyncApis", return_value=mock_sync_apis): + with patch( + "th_cli.commands.project.click.edit", + return_value=json.dumps(edited_config, indent=2), + ) as mock_edit: + # Act + result = cli_runner.invoke(project, ["edit", "--id", "1"]) + + # Assert + assert result.exit_code == 0 + assert "some validation error message" in result.output + assert mock_edit.call_count == 2 + assert mock_sync_apis.projects_api.update_project_api_v1_projects__id__put.call_count == 2 + + def test_edit_project_backend_422_list_detail_formats_locs( + self, cli_runner: CliRunner, mock_sync_apis: Mock, sample_project: api_models.Project + ) -> None: + """Test that a list-shaped 422 detail (FastAPI request validation) is formatted readably.""" + # Arrange + edited_config = json.loads(json.dumps(sample_project.config)) + edited_config["dut_config"]["setup_code"] = "99999999" + + api_exception = UnexpectedResponse( + status_code=422, + content={ + "detail": [ + { + "loc": ["body", "config", "th_config", "prompt_timeout_seconds"], + "msg": "value is not a valid integer", + "type": "type_error.integer", + } + ] + }, + ) + + mock_sync_apis.projects_api.read_project_api_v1_projects__id__get.return_value = sample_project + mock_sync_apis.projects_api.update_project_api_v1_projects__id__put.side_effect = [ + api_exception, + sample_project, + ] + + with patch("th_cli.commands.project.SyncApis", return_value=mock_sync_apis): + with patch( + "th_cli.commands.project.click.edit", + return_value=json.dumps(edited_config, indent=2), + ): + # Act + result = cli_runner.invoke(project, ["edit", "--id", "1"]) + + # Assert + assert result.exit_code == 0 + assert "config.th_config.prompt_timeout_seconds: value is not a valid integer" in result.output + + def test_edit_project_backend_422_exceeds_retries_aborts( + self, cli_runner: CliRunner, mock_sync_apis: Mock, sample_project: api_models.Project + ) -> None: + """Test that a persistent 422 exhausts retries and aborts without saving.""" + # Arrange + edited_config = json.loads(json.dumps(sample_project.config)) + edited_config["dut_config"]["setup_code"] = "99999999" + + api_exception = UnexpectedResponse(status_code=422, content={"detail": "always invalid"}) + + mock_sync_apis.projects_api.read_project_api_v1_projects__id__get.return_value = sample_project + mock_sync_apis.projects_api.update_project_api_v1_projects__id__put.side_effect = api_exception + + with patch("th_cli.commands.project.SyncApis", return_value=mock_sync_apis): + with patch( + "th_cli.commands.project.click.edit", + return_value=json.dumps(edited_config, indent=2), + ) as mock_edit: + # Act + result = cli_runner.invoke(project, ["edit", "--id", "1"]) + + # Assert + assert result.exit_code == 1 + assert "Exceeded maximum retry attempts" in result.output + assert mock_edit.call_count == 3 + assert mock_sync_apis.projects_api.update_project_api_v1_projects__id__put.call_count == 3 + + def test_edit_project_backend_404_does_not_retry( + self, cli_runner: CliRunner, mock_sync_apis: Mock, sample_project: api_models.Project + ) -> None: + """Test that a non-422 error (e.g. 404) fails immediately without retrying.""" + # Arrange + edited_config = json.loads(json.dumps(sample_project.config)) + edited_config["dut_config"]["setup_code"] = "99999999" + + api_exception = UnexpectedResponse(status_code=404, content=b"Not Found") + + mock_sync_apis.projects_api.read_project_api_v1_projects__id__get.return_value = sample_project + mock_sync_apis.projects_api.update_project_api_v1_projects__id__put.side_effect = api_exception + + with patch("th_cli.commands.project.SyncApis", return_value=mock_sync_apis): + with patch( + "th_cli.commands.project.click.edit", + return_value=json.dumps(edited_config, indent=2), + ) as mock_edit: + # Act + result = cli_runner.invoke(project, ["edit", "--id", "1"]) + + # Assert + assert result.exit_code == 1 + assert "Error: Failed to update project with '1' (Status: 404) - Not Found" in result.output + assert mock_edit.call_count == 1 + + def test_edit_project_read_project_api_error(self, cli_runner: CliRunner, mock_sync_apis: Mock) -> None: + """Test that a failure fetching the project aborts before opening the editor.""" + # Arrange + api_exception = UnexpectedResponse(status_code=404, content=b"Not Found") + mock_sync_apis.projects_api.read_project_api_v1_projects__id__get.side_effect = api_exception + + with patch("th_cli.commands.project.SyncApis", return_value=mock_sync_apis): + with patch("th_cli.commands.project.click.edit") as mock_edit: + # Act + result = cli_runner.invoke(project, ["edit", "--id", "1"]) + + # Assert + assert result.exit_code == 1 + assert "Error: Failed to fetch project with ID '1'" in result.output + mock_edit.assert_not_called() + + def test_edit_project_help_message(self, cli_runner: CliRunner) -> None: + """Test the help message for the edit_project command.""" + # Act + result = cli_runner.invoke(project, ["edit", "--help"]) + + # Assert + assert result.exit_code == 0 + assert "edit" in result.output + assert "--id" in result.output + + def test_edit_project_missing_required_id(self, cli_runner: CliRunner, mock_sync_apis: Mock) -> None: + """Test that omitting the required --id option fails without making API calls.""" + with patch("th_cli.commands.project.SyncApis", return_value=mock_sync_apis): + # Act + result = cli_runner.invoke(project, ["edit"]) + + # Assert + assert result.exit_code != 0 + mock_sync_apis.projects_api.read_project_api_v1_projects__id__get.assert_not_called() + + @pytest.mark.unit @pytest.mark.cli class TestExportProjectCommand: diff --git a/th_cli/commands/project.py b/th_cli/commands/project.py index 8911535..33ec51c 100644 --- a/th_cli/commands/project.py +++ b/th_cli/commands/project.py @@ -45,6 +45,7 @@ from th_cli.validation import validate_directory_path TABLE_FORMAT = "{:<5} {:25} {:28}" +MAX_EDIT_RETRIES = 3 # Click command group for project management @@ -190,6 +191,24 @@ def update(id: int, config: str | None, name: str | None, pics_config_folder: st _update_project(sync_apis, id, name, config, pics_config_folder) +# Click command to interactively edit an existing project's config +@project.command( + "edit", + short_help=colorize_help("Edit a project's config interactively"), +) +@click.option( + "--id", + "-i", + type=int, + required=True, + help=colorize_help("Project ID to edit"), +) +def edit(id: int) -> None: + """Edit a project's config interactively in your local editor""" + with get_sync_apis("edit") as sync_apis: + _edit_project(sync_apis, id) + + # Click command to delete an existing project @project.command( "delete", @@ -454,6 +473,169 @@ def _update_project( handle_api_error(e, f"update project with '{id}'") +def _collect_dotted_keys(config: dict, prefix: str = "") -> set: + """Recursively collect dotted-path key names from a nested config dict. + + Includes keys at every nesting level (not just leaves), and indexes into + lists of dicts (e.g. "some_list[0].field"), so a newly introduced + intermediate section is caught even if its own contents aren't compared. + """ + keys: set = set() + for key, value in config.items(): + dotted = f"{prefix}.{key}" if prefix else key + keys.add(dotted) + if isinstance(value, dict): + keys |= _collect_dotted_keys(value, dotted) + elif isinstance(value, list): + for i, item in enumerate(value): + if isinstance(item, dict): + keys |= _collect_dotted_keys(item, f"{dotted}[{i}]") + return keys + + +def _strip_error_banner(text: str) -> str: + """Strip a leading block of '//'-prefixed error-banner lines before parsing as JSON.""" + lines = text.splitlines(keepends=True) + stripped = [] + banner_done = False + for line in lines: + if not banner_done and line.lstrip().startswith("//"): + continue + banner_done = True + stripped.append(line) + return "".join(stripped) + + +def _build_json_error_banner(e: json.JSONDecodeError) -> str: + return ( + f"// Error: Invalid JSON - {e.msg} (line {e.lineno}, column {e.colno}).\n" + "// Fix the issue below and save again, or make no changes to abort.\n" + ) + + +def _build_backend_error_banner(message: str) -> str: + return ( + f"// Error: Server rejected the update - {message}\n" + "// Fix the issue below and save again, or make no changes to abort.\n" + ) + + +def _format_422_detail(e: UnexpectedResponse) -> str: + """Format an UnexpectedResponse's 422 body, which may take one of two shapes: + a plain string (from TestEnvironmentConfigError) or a list of FastAPI + request-validation error dicts (from the ProjectUpdate envelope itself). + """ + content = e.content + if isinstance(content, bytes): + try: + content = json.loads(content.decode("utf-8", errors="ignore")) + except json.JSONDecodeError: + return content.decode("utf-8", errors="ignore") + + if isinstance(content, dict): + detail = content.get("detail") + if isinstance(detail, str): + return detail + if isinstance(detail, list): + lines = [] + for err in detail: + loc = ".".join(str(part) for part in err.get("loc", []) if part != "body") + msg = err.get("msg", "") + lines.append(f"{loc}: {msg}" if loc else msg) + return "; ".join(lines) if lines else str(detail) + return str(content) + + return str(content) + + +def _edit_project(sync_apis: SyncApis, id: int) -> None: + """Edit a project's config interactively in the user's local editor""" + try: + existing_project = sync_apis.projects_api.read_project_api_v1_projects__id__get(id=id) + except UnexpectedResponse as e: + handle_api_error(e, f"fetch project with ID '{id}'") + return + + original_config = existing_project.config or {} + original_keys = _collect_dotted_keys(original_config) + + text = json.dumps(original_config, indent=2) + error_banner = "" + + for attempt in range(MAX_EDIT_RETRIES): + edited_text = click.edit(text=error_banner + text, extension=".json") + + if edited_text is None: + click.echo(colorize_warning("No changes made. Aborting edit.")) + return + + json_candidate = _strip_error_banner(edited_text) + last_attempt = attempt == MAX_EDIT_RETRIES - 1 + + try: + edited_config = json.loads(json_candidate) + except json.JSONDecodeError as e: + click.echo(colorize_error(f"Invalid JSON: {e.msg} (line {e.lineno}, col {e.colno})")) + if last_attempt: + raise CLIError("Exceeded maximum retry attempts to fix invalid JSON. Aborting edit.") + click.echo(colorize_warning("Reopening editor so you can fix the JSON...")) + text = json_candidate + error_banner = _build_json_error_banner(e) + continue + + if not isinstance(edited_config, dict): + click.echo(colorize_error("Config must be a JSON object (dict) at the top level.")) + if last_attempt: + raise CLIError("Exceeded maximum retry attempts. Aborting edit.") + text = json_candidate + error_banner = "// Error: top-level JSON must be an object, not a list/scalar.\n" + continue + + if edited_config == original_config: + click.echo(colorize_warning("No changes detected. Aborting edit.")) + return + + edited_keys = _collect_dotted_keys(edited_config) + new_keys = sorted(edited_keys - original_keys) + if new_keys: + click.echo(colorize_warning("The following new/unknown keys were introduced that did not exist before:")) + for key in new_keys: + click.echo(f" - {key}") + click.echo( + colorize_warning( + "Note: the backend silently ignores unknown fields outside 'dut_config', " + "so a typo here may be dropped rather than rejected." + ) + ) + if not click.confirm("Continue anyway?", default=False): + click.echo(colorize_warning("Reopening editor so you can fix the keys...")) + text = json_candidate + error_banner = "" + continue + + project_update = ProjectUpdate( + name=existing_project.name, + config=edited_config, + pics=existing_project.pics, + ) + try: + response = sync_apis.projects_api.update_project_api_v1_projects__id__put(id=id, body=project_update) + click.echo(colorize_success(f"Project '{response.name}' was updated.")) + return + except UnexpectedResponse as e: + if e.status_code != 422: + handle_api_error(e, f"update project with '{id}'") + return + message = _format_422_detail(e) + click.echo(colorize_error(f"Server rejected the update: {message}")) + if last_attempt: + raise CLIError("Exceeded maximum retry attempts. Aborting edit without saving.") + click.echo(colorize_warning("Reopening editor so you can fix the config...")) + text = json_candidate + error_banner = _build_backend_error_banner(message) + continue + + def _delete_project(sync_apis: SyncApis, id: int) -> None: """Delete a project""" try: From 0b88ce3e1926c374b743398412e31e2f030a4926 Mon Sep 17 00:00:00 2001 From: aamj Date: Fri, 28 Aug 2026 17:20:59 -0300 Subject: [PATCH 2/2] Adding note about changing editor in the command --- README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.md b/README.md index f4e63f7..d3f9238 100644 --- a/README.md +++ b/README.md @@ -174,6 +174,10 @@ not reject all unknown fields) and asks for confirmation before persisting. If t JSON is invalid or the server rejects the change, the editor reopens with your last edit preserved so nothing is lost. +The editor used is picked from the `$VISUAL` environment variable first, then `$EDITOR`, +falling back to `vim`/`nano`/`vi` if neither is set. To use a different editor, set one +of these before running the command, e.g. `EDITOR=nano th-cli project edit --id {id}`. + ## Command Colors By default, the CLI application presents colored texts for all the available commands, specially for the log of test run executions from the `th-cli run-tests` command. If the users need to disable the colors from the tool's output, they may use one of the options presented below: