mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-04 06:15:09 +00:00
fix(web): plugin settings form shows schema defaults for unsaved keys (#597)
The server-rendered plugin settings partial rendered straight from the saved config, so an option added in a plugin update (geochron 1.2.0's show_date / show_date_line, default true) drew as an unchecked box, and the save route's missing-checkbox handling then stored it as false. Enum dropdowns likewise showed their first option instead of the default. - _load_plugin_config_partial runs the stored section through prepare_plugin_config (as GET /plugins/config does) before masking secrets, so a secret's schema default is masked too. - render_field falls back to the field's own default, covering children of objects that declare a default of their own (where the defaults extraction stops). - The legacy-boolean parity test now compares against the config the plugin actually runs with (defaults included). Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Chuck
@@ -40,6 +40,14 @@ Config saves and plugin config preparation:
|
||||
the object, posting it back saves, and hot reload hands plugins the same
|
||||
shape (schema defaults included) they were constructed with.
|
||||
`schema_manager.prepare_plugin_config` is the one implementation.
|
||||
- A plugin's settings tab shows schema defaults for options its saved config
|
||||
doesn't have yet. A boolean added with `"default": true` in a plugin update
|
||||
(geochron 1.2.0's `show_date` and `show_date_line`) used to render unchecked,
|
||||
and the next save of that tab stored it as `false`. Enum dropdowns likewise
|
||||
showed their first option instead of the default. The partial now runs the
|
||||
stored section through `prepare_plugin_config` like `GET /plugins/config`
|
||||
(secrets are still masked, after the merge), and the form falls back to a
|
||||
field's own `default` inside objects that declare a default of their own.
|
||||
- `scripts/dev_server.py`, `check_plugin.py`, `render_plugin.py` and the plugin
|
||||
harness build configs the way a device does: nested defaults are included,
|
||||
a schema `enabled: false` no longer beats the forced `enabled: true` in the
|
||||
|
||||
@@ -30,6 +30,8 @@ from src.plugin_system.plugin_manager import PluginManager
|
||||
from src.plugin_system.schema_manager import (
|
||||
legacy_bool_as_object,
|
||||
normalize_legacy_booleans,
|
||||
plugin_config_defaults,
|
||||
prepare_plugin_config,
|
||||
)
|
||||
|
||||
|
||||
@@ -361,26 +363,33 @@ def _form_checkboxes(stored):
|
||||
return parser.checked
|
||||
|
||||
|
||||
def _loader_enabled(normalized, *path):
|
||||
node = normalized
|
||||
def _loader_enabled(prepared, *path):
|
||||
node = prepared
|
||||
for key in path:
|
||||
node = node.get(key) if isinstance(node, dict) else None
|
||||
return isinstance(node, dict) and node.get("enabled") is True
|
||||
if not isinstance(node, dict):
|
||||
# A value the loader cannot read as the object (1, "true") fails
|
||||
# validation; the form treats it as missing and draws the schema
|
||||
# default, as it does for every other missing field.
|
||||
return True
|
||||
return node.get("enabled") is True
|
||||
|
||||
|
||||
@pytest.mark.parametrize("value", [True, False, 1, "true", None])
|
||||
def test_form_and_loader_agree_on_what_a_stored_value_means(value):
|
||||
"""Where the loader reads a stored value as ``{"enabled": true}``, the form
|
||||
draws the object's ``enabled`` checkbox ticked, and nowhere else."""
|
||||
"""The form draws the object's ``enabled`` checkbox ticked exactly when the
|
||||
plugin runs with it on -- legacy booleans read as objects, then schema
|
||||
defaults filled in (prepare_plugin_config)."""
|
||||
stored = {"enabled": True,
|
||||
"global": {"dynamic_duration": value, "outer": {"inner": value}}}
|
||||
normalized = normalize_legacy_booleans(stored, PARITY_SCHEMA)
|
||||
prepared = prepare_plugin_config(stored, PARITY_SCHEMA,
|
||||
plugin_config_defaults(PARITY_SCHEMA))
|
||||
boxes = _form_checkboxes(stored)
|
||||
|
||||
for path in (("global", "dynamic_duration"), ("global", "outer", "inner")):
|
||||
name = ".".join(path) + ".enabled"
|
||||
assert name in boxes, f"form drew no {name} checkbox"
|
||||
assert boxes[name] is _loader_enabled(normalized, *path), name
|
||||
assert boxes[name] is _loader_enabled(prepared, *path), name
|
||||
|
||||
|
||||
@pytest.mark.parametrize("value", [True, False])
|
||||
|
||||
@@ -0,0 +1,198 @@
|
||||
"""The server-rendered plugin form must show schema defaults for unsaved keys.
|
||||
|
||||
A plugin update that adds a boolean option with ``"default": true`` (geochron
|
||||
1.2.0's ``show_date`` / ``show_date_line``) leaves every existing install with
|
||||
a saved config that lacks the key. The partial rendered it from the raw config,
|
||||
so the box came up unchecked -- and the save route treats a drawn but unposted
|
||||
checkbox as false, so the first save turned the option off for good.
|
||||
|
||||
These drive the real partial route (pages_v3) and post what a browser would
|
||||
submit from its HTML back through the real save route (api_v3).
|
||||
"""
|
||||
|
||||
import json
|
||||
import sys
|
||||
from html.parser import HTMLParser
|
||||
from pathlib import Path
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
import pytest
|
||||
from flask import Flask
|
||||
from werkzeug.datastructures import MultiDict
|
||||
|
||||
PROJECT_ROOT = Path(__file__).parent.parent.parent
|
||||
sys.path.insert(0, str(PROJECT_ROOT))
|
||||
|
||||
SCHEMA = {
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"enabled": {"type": "boolean", "default": True},
|
||||
"show_date": {"type": "boolean", "default": True},
|
||||
"show_date_line": {"type": "boolean", "default": True},
|
||||
"show_seconds": {"type": "boolean", "default": False},
|
||||
"style": {"type": "string", "enum": ["flat", "globe", "night"],
|
||||
"default": "globe"},
|
||||
"brightness": {"type": "integer", "default": 70,
|
||||
"minimum": 0, "maximum": 100},
|
||||
"label": {"type": "string", "default": "UTC"},
|
||||
# An object with its own default: extract_schema_defaults stops here,
|
||||
# so the route merge leaves the children missing.
|
||||
"overlay": {"type": "object", "default": {}, "properties": {
|
||||
"show_sun": {"type": "boolean", "default": True},
|
||||
"mode": {"type": "string", "enum": ["dot", "ring"],
|
||||
"default": "ring"},
|
||||
}, "additionalProperties": False},
|
||||
# An object without one: the route merge fills the children in.
|
||||
"grid": {"type": "object", "properties": {
|
||||
"show_lines": {"type": "boolean", "default": True},
|
||||
}, "additionalProperties": False},
|
||||
"api_key": {"type": "string", "x-secret": True,
|
||||
"default": "not-a-real-secret"},
|
||||
},
|
||||
"required": ["enabled"],
|
||||
"additionalProperties": False,
|
||||
}
|
||||
|
||||
# Saved before any of the options above existed.
|
||||
STORED = {"enabled": True, "show_seconds": True}
|
||||
|
||||
|
||||
class _FormFields(HTMLParser):
|
||||
"""Collect what a browser would submit from the rendered form."""
|
||||
|
||||
def __init__(self):
|
||||
super().__init__()
|
||||
self.pairs = []
|
||||
self._select = None
|
||||
|
||||
def handle_starttag(self, tag, attrs):
|
||||
a = dict(attrs)
|
||||
if tag == "select":
|
||||
self._select = a.get("name")
|
||||
return
|
||||
if tag == "option" and self._select and "selected" in a:
|
||||
self.pairs.append((self._select, a.get("value", "")))
|
||||
return
|
||||
if tag != "input" or not a.get("name") or "disabled" in a:
|
||||
return
|
||||
if a.get("type") in ("checkbox", "radio") and "checked" not in a:
|
||||
return
|
||||
if a.get("type") in ("button", "submit", "file"):
|
||||
return
|
||||
self.pairs.append((a["name"], a.get("value", "")))
|
||||
|
||||
def handle_endtag(self, tag):
|
||||
if tag == "select":
|
||||
self._select = None
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def app_client(tmp_path):
|
||||
from src.plugin_system.schema_manager import SchemaManager
|
||||
from web_interface.blueprints import api_v3 as api
|
||||
from web_interface.blueprints import pages_v3 as pages
|
||||
|
||||
plugins_dir = tmp_path / "plugin-repos"
|
||||
pdir = plugins_dir / "demo"
|
||||
pdir.mkdir(parents=True)
|
||||
(pdir / "config_schema.json").write_text(json.dumps(SCHEMA), encoding="utf-8")
|
||||
(pdir / "manifest.json").write_text(
|
||||
json.dumps({"id": "demo", "name": "Demo", "version": "1.0.0"}),
|
||||
encoding="utf-8")
|
||||
|
||||
store = {"demo": json.loads(json.dumps(STORED))}
|
||||
cm = MagicMock()
|
||||
cm.load_config.side_effect = lambda: json.loads(json.dumps(store))
|
||||
cm.get_raw_file_content.return_value = {}
|
||||
cm.get_config_path.return_value = str(tmp_path / "config.json")
|
||||
|
||||
def _save(cfg, **_kw):
|
||||
store.clear()
|
||||
store.update(cfg)
|
||||
return type("R", (), {"status": type("S", (), {"value": "success"})(),
|
||||
"message": None})()
|
||||
|
||||
cm.save_config_atomic.side_effect = _save
|
||||
|
||||
pm = MagicMock()
|
||||
pm.plugins = {}
|
||||
pm.plugins_dir = plugins_dir
|
||||
pm.get_plugin.return_value = None
|
||||
pm.get_plugin_info.return_value = {"name": "Demo", "version": "1.0.0"}
|
||||
sm = SchemaManager(plugins_dir=plugins_dir, project_root=tmp_path)
|
||||
|
||||
names = ("config_manager", "schema_manager", "plugin_manager")
|
||||
originals = {(bp, k): getattr(bp, k, None)
|
||||
for bp in (api.api_v3, pages.pages_v3) for k in names}
|
||||
for bp in (api.api_v3, pages.pages_v3):
|
||||
bp.config_manager = cm
|
||||
bp.schema_manager = sm
|
||||
bp.plugin_manager = pm
|
||||
|
||||
base = Path(pages.__file__).resolve().parent.parent
|
||||
app = Flask(__name__, template_folder=str(base / "templates"),
|
||||
static_folder=str(base / "static"))
|
||||
app.config["TESTING"] = True
|
||||
app.register_blueprint(pages.pages_v3, url_prefix="/v3")
|
||||
app.register_blueprint(api.api_v3, url_prefix="/api/v3")
|
||||
|
||||
try:
|
||||
yield app.test_client(), store
|
||||
finally:
|
||||
for (bp, k), v in originals.items():
|
||||
setattr(bp, k, v)
|
||||
|
||||
|
||||
def _render(client):
|
||||
resp = client.get("/v3/partials/plugin-config/demo")
|
||||
assert resp.status_code == 200
|
||||
return resp.get_data(as_text=True)
|
||||
|
||||
|
||||
def _fields(html):
|
||||
parser = _FormFields()
|
||||
parser.feed(html)
|
||||
return dict(parser.pairs)
|
||||
|
||||
|
||||
def test_missing_booleans_render_their_schema_default(app_client):
|
||||
client, _ = app_client
|
||||
fields = _fields(_render(client))
|
||||
# Checked, so a browser posts them.
|
||||
assert fields.get("show_date") == "true"
|
||||
assert fields.get("show_date_line") == "true"
|
||||
assert fields.get("grid.show_lines") == "true"
|
||||
assert fields.get("overlay.show_sun") == "true"
|
||||
# A saved value still wins over the default.
|
||||
assert fields.get("show_seconds") == "true"
|
||||
|
||||
|
||||
def test_missing_non_boolean_fields_render_their_schema_default(app_client):
|
||||
client, _ = app_client
|
||||
fields = _fields(_render(client))
|
||||
assert fields.get("style") == "globe" # not the first option
|
||||
assert fields.get("overlay.mode") == "ring"
|
||||
assert fields.get("brightness") == "70"
|
||||
assert fields.get("label") == "UTC"
|
||||
|
||||
|
||||
def test_a_secret_default_is_still_masked(app_client):
|
||||
client, _ = app_client
|
||||
assert "not-a-real-secret" not in _render(client)
|
||||
|
||||
|
||||
def test_saving_the_rendered_form_keeps_default_true_booleans_on(app_client):
|
||||
client, store = app_client
|
||||
pairs = list(_fields(_render(client)).items())
|
||||
resp = client.post("/api/v3/plugins/config?plugin_id=demo",
|
||||
data=MultiDict(pairs))
|
||||
assert resp.status_code == 200, resp.get_json()
|
||||
|
||||
saved = store["demo"]
|
||||
assert saved["show_date"] is True
|
||||
assert saved["show_date_line"] is True
|
||||
assert saved["grid"]["show_lines"] is True
|
||||
assert saved["overlay"]["show_sun"] is True
|
||||
assert saved["overlay"]["mode"] == "ring"
|
||||
assert saved["style"] == "globe"
|
||||
assert saved["show_seconds"] is True
|
||||
@@ -13,6 +13,7 @@ _SAFE_WEB_UI_FILE_RE = re.compile(r'^[a-zA-Z0-9_-]{1,64}\.html$')
|
||||
_SAFE_WIDGET_NAME_RE = re.compile(r'^[a-zA-Z0-9_-]{1,64}$')
|
||||
_SAFE_WIDGET_SCRIPT_RE = re.compile(r'^[a-zA-Z0-9_-]{1,64}\.js$')
|
||||
from src.web_interface.secret_helpers import mask_secret_fields
|
||||
from src.plugin_system.schema_manager import plugin_config_defaults, prepare_plugin_config
|
||||
from src.common.path_safety import resolve_under, safe_path_component
|
||||
from src.pi5_matrix_support import is_raspberry_pi_5
|
||||
from web_interface import widget_bundle
|
||||
@@ -917,10 +918,25 @@ def _load_plugin_config_partial(plugin_id):
|
||||
except Exception as e:
|
||||
logger.warning("Could not load manifest for plugin: %s", e)
|
||||
|
||||
# Mask secret fields before rendering template (fail closed — never leak secrets)
|
||||
schema_properties = schema.get('properties') if isinstance(schema, dict) else None
|
||||
if not isinstance(schema_properties, dict):
|
||||
return '<div class="text-red-500 p-4">Error loading plugin config securely: schema unavailable.</div>', 500
|
||||
|
||||
# Fill in schema defaults for keys the saved config doesn't have yet,
|
||||
# as GET /api/v3/plugins/config does. Without this, an option added in
|
||||
# a plugin update (geochron 1.2.0's show_date, default true) renders
|
||||
# as an unchecked box, and because the save treats every drawn but
|
||||
# unposted checkbox as false, the first save turns it off for good.
|
||||
try:
|
||||
defaults = plugin_config_defaults(schema)
|
||||
if schema_mgr is not None:
|
||||
defaults = schema_mgr.apply_device_location(defaults)
|
||||
config = prepare_plugin_config(config, schema, defaults)
|
||||
except Exception as e:
|
||||
logger.warning("Could not merge schema defaults for %s: %s", plugin_id, e)
|
||||
|
||||
# Mask secret fields before rendering template (fail closed — never
|
||||
# leak secrets). After the merge, so a secret's default is masked too.
|
||||
config = mask_secret_fields(config, schema_properties)
|
||||
|
||||
# Determine enabled status
|
||||
|
||||
@@ -29,6 +29,11 @@
|
||||
|
||||
{% macro render_field(key, prop, value, prefix='', plugin_id='') %}
|
||||
{% if not prop_is_hidden(prop)|trim %}
|
||||
{# A key the saved config doesn't have renders its schema default. The
|
||||
route merges defaults first, but that merge stops at an object that
|
||||
declares its own default (often {}), leaving its children missing here.
|
||||
Booleans matter most: an unchecked box saves as false. #}
|
||||
{% if value is none and prop.default is defined %}{% set value = prop.default %}{% endif %}
|
||||
{% set full_key = (prefix ~ '.' ~ key) if prefix else key %}
|
||||
{% set field_id = (plugin_id ~ '-' ~ full_key)|replace('.', '-')|replace('_', '-') %}
|
||||
{% set label = prop.title if prop.title else key|replace('_', ' ')|title %}
|
||||
|
||||
Reference in New Issue
Block a user