From 1df51b172a4cf04db631c20fbe639c4eb6e3773e Mon Sep 17 00:00:00 2001
From: Chuck <33324927+ChuckBuilds@users.noreply.github.com>
Date: Sun, 13 Sep 2026 10:36:28 -0400
Subject: [PATCH] fix(web): a partial save no longer switches off checkboxes it
 never showed

An HTML checkbox posts nothing when unchecked, so the save route walked the
schema and forced every boolean missing from the form to False. That is right
for the rendered form and wrong for every other caller: a script, the MQTT
bridge or a curl against the documented endpoint never rendered a checkbox, and
reading its silence as "all off" turns a one-field save into a mass disable.

Found on hardware. Posting four customization.* keys to a live device switched
off nfl.enabled, ncaa_fb.enabled and every display-mode toggle in one request.

The form now reports the top-level sections it drew (__rendered_section), and
inside those an absent checkbox still means unchecked -- including a section
whose only fields are checkboxes that are all off, which no heuristic could
recover. A post with no marker only touches objects it actually posted a field
from. Meta fields are dropped before form keys are treated as config paths,
because unknown keys are otherwise written straight into config.json.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---
 .../test_partial_save_booleans.py             | 249 ++++++++++++++++++
 web_interface/blueprints/api_v3/__init__.py   |  71 ++++-
 web_interface/blueprints/api_v3/plugins.py    |  16 +-
 .../templates/v3/partials/plugin_config.html  |   8 +
 4 files changed, 335 insertions(+), 9 deletions(-)
 create mode 100644 test/web_interface/test_partial_save_booleans.py

diff --git a/test/web_interface/test_partial_save_booleans.py b/test/web_interface/test_partial_save_booleans.py
new file mode 100644
index 00000000..4a4ff984
--- /dev/null
+++ b/test/web_interface/test_partial_save_booleans.py
@@ -0,0 +1,249 @@
+"""A save must only switch off checkboxes the caller actually saw.
+
+An HTML checkbox posts nothing when unchecked, so the save route walks the
+schema and forces every boolean missing from the form to ``False``. That is
+right for the rendered form and wrong for everything else: a caller that posts
+four fields -- a script, the MQTT bridge, a curl against the documented
+endpoint -- never rendered a checkbox, and reading its silence as "all off"
+turns a one-field save into a mass disable.
+
+That is not hypothetical. Posting four ``customization.*`` keys to a live
+device switched off ``nfl.enabled``, ``ncaa_fb.enabled`` and every display-mode
+toggle in one request.
+
+The fix keeps both halves working, so both are pinned here:
+
+* the rendered form reports the sections it drew (``__rendered_section``), and
+  inside those, an absent checkbox still means unchecked -- including a section
+  whose only fields are checkboxes that are all off, which no heuristic could
+  recover;
+* a post with no such marker only touches objects it actually posted a field
+  from.
+"""
+
+import json
+import sys
+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},
+        "nfl": {
+            "type": "object",
+            "properties": {
+                "enabled": {"type": "boolean", "default": True},
+                "live_priority": {"type": "boolean", "default": True},
+                "update_interval": {"type": "integer", "default": 3600},
+                # A section with nothing but checkboxes: when they are all
+                # unchecked it posts no keys at all.
+                "display_modes": {
+                    "type": "object",
+                    "properties": {
+                        "show_live": {"type": "boolean", "default": True},
+                        "show_recent": {"type": "boolean", "default": True},
+                    },
+                },
+            },
+        },
+        "customization": {
+            "type": "object",
+            "properties": {
+                "score_text": {
+                    "type": "object",
+                    "properties": {
+                        "visible": {"type": "boolean", "default": True},
+                        "text_color": {
+                            "type": "array",
+                            "items": {"type": "integer"},
+                            "minItems": 3, "maxItems": 3,
+                            "default": [255, 255, 255],
+                        },
+                    },
+                },
+            },
+        },
+    },
+}
+
+STORED = {
+    "enabled": True,
+    "nfl": {
+        "enabled": True,
+        "live_priority": True,
+        "update_interval": 3600,
+        "display_modes": {"show_live": True, "show_recent": True},
+    },
+    "customization": {"score_text": {"visible": True,
+                                     "text_color": [255, 255, 255]}},
+}
+
+
+class _SaveOk:
+    status = type("S", (), {"value": "success"})()
+    message = None
+
+
+@pytest.fixture
+def post(tmp_path):
+    """POST form data to the real save route; yields the stored config."""
+    from src.plugin_system.schema_manager import SchemaManager
+    from web_interface.blueprints import api_v3 as a
+
+    originals = {k: getattr(a.api_v3, k, None)
+                 for k in ("config_manager", "schema_manager", "plugin_manager")}
+
+    pdir = tmp_path / "plugins" / "demo"
+    pdir.mkdir(parents=True)
+    (pdir / "config_schema.json").write_text(json.dumps(SCHEMA), 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_config_path.return_value = str(tmp_path / "config.json")
+
+    def _save(cfg, **_kw):
+        store.clear()
+        store.update(cfg)
+        return _SaveOk()
+
+    cm.save_config_atomic.side_effect = _save
+
+    a.api_v3.config_manager = cm
+    a.api_v3.schema_manager = SchemaManager(plugins_dir=tmp_path / "plugins",
+                                            project_root=tmp_path)
+    pm = MagicMock()
+    pm.plugins = {}
+    pm.get_plugin.return_value = None
+    a.api_v3.plugin_manager = pm
+
+    app = Flask(__name__)
+    app.config["TESTING"] = True
+    app.register_blueprint(a.api_v3, url_prefix="/api/v3")
+    client = app.test_client()
+
+    def _post(form):
+        # A list of pairs, not a dict: __rendered_section repeats, and that
+        # repetition is the whole point of the marker.
+        data = MultiDict(form) if isinstance(form, list) else form
+        resp = client.post("/api/v3/plugins/config?plugin_id=demo", data=data)
+        return resp, store.get("demo", {})
+
+    try:
+        yield _post
+    finally:
+        for k, v in originals.items():
+            setattr(a.api_v3, k, v)
+
+
+# What the style editor posts when someone changes one colour and nothing else.
+PARTIAL = [
+    ("customization.score_text.text_color.0", "255"),
+    ("customization.score_text.text_color.1", "0"),
+    ("customization.score_text.text_color.2", "255"),
+]
+
+
+class TestAPartialPost:
+    def test_it_does_not_disable_another_section(self, post):
+        """The live-device regression, in one assertion."""
+        _post = post
+        resp, cfg = _post(list(PARTIAL))
+        assert resp.status_code == 200, resp.get_json()
+        assert cfg["nfl"]["enabled"] is True
+        assert cfg["nfl"]["live_priority"] is True
+        assert cfg["nfl"]["display_modes"] == {"show_live": True,
+                                               "show_recent": True}
+
+    def test_it_still_applies_what_was_posted(self, post):
+        _post = post
+        _resp, cfg = _post(list(PARTIAL))
+        assert cfg["customization"]["score_text"]["text_color"] == [255, 0, 255]
+
+    def test_it_leaves_a_sibling_checkbox_it_never_mentioned(self, post):
+        """`visible` lives beside the colour, but the post carried no field
+        from that object -- only from the colour array inside it."""
+        _post = post
+        _resp, cfg = _post(list(PARTIAL))
+        assert cfg["customization"]["score_text"]["visible"] is True
+
+    def test_it_does_clear_a_box_in_an_object_it_posted_to(self, post):
+        """Evidence, not guesswork: a field from `display_modes` was posted,
+        so the checkbox missing from that same object really is unchecked."""
+        _post = post
+        _resp, cfg = _post([("nfl.display_modes.show_live", "true")])
+        assert cfg["nfl"]["display_modes"] == {"show_live": True,
+                                               "show_recent": False}
+        assert cfg["nfl"]["enabled"] is True
+
+
+class TestTheRenderedForm:
+    """With section markers the old behaviour must be exactly preserved."""
+
+    def test_an_unchecked_box_is_still_cleared(self, post):
+        _post = post
+        resp, cfg = _post([("__rendered_section", "nfl"),
+                           ("__rendered_section", "customization"),
+                           ("enabled", "true"),
+                           ("nfl.update_interval", "3600"),
+                           ("nfl.display_modes.show_live", "true")])
+        assert resp.status_code == 200, resp.get_json()
+        assert cfg["nfl"]["enabled"] is False
+        assert cfg["nfl"]["live_priority"] is False
+        assert cfg["nfl"]["display_modes"]["show_recent"] is False
+
+    def test_a_section_of_only_checkboxes_all_unchecked_is_cleared(self, post):
+        """It posts no keys of its own, so only the marker can vouch for it."""
+        _post = post
+        _resp, cfg = _post([("__rendered_section", "nfl"),
+                            ("nfl.update_interval", "3600")])
+        assert cfg["nfl"]["display_modes"] == {"show_live": False,
+                                               "show_recent": False}
+
+    def test_a_section_the_form_did_not_draw_is_untouched(self, post):
+        _post = post
+        _resp, cfg = _post([("__rendered_section", "nfl"),
+                            ("nfl.update_interval", "3600")])
+        assert cfg["customization"]["score_text"]["visible"] is True
+
+    def test_the_marker_is_not_written_into_the_config(self, post):
+        """It describes the submission; it is not a config path. Unknown form
+        keys are otherwise stored verbatim."""
+        _post = post
+        _resp, cfg = _post([("__rendered_section", "nfl"),
+                            ("nfl.update_interval", "3600")])
+        assert "__rendered_section" not in cfg
+        assert not [k for k in cfg if k.startswith("__")]
+
+
+class TestTheFormEmitsTheMarker:
+    """The fix only works if the rendered form actually reports its sections."""
+
+    def test_every_top_level_section_is_reported(self, tmp_path):
+        import re
+
+        from jinja2 import Environment, FileSystemLoader
+
+        templates = PROJECT_ROOT / "web_interface" / "templates"
+        env = Environment(loader=FileSystemLoader(str(templates)))
+        source = (templates / "v3" / "partials" / "plugin_config.html").read_text(
+            encoding="utf-8")
+        assert '__rendered_section' in source, (
+            "the form must tell the save route which sections it drew")
+        # It has to cover the advanced tier too, or collapsing a section into
+        # Advanced Settings would quietly stop its checkboxes clearing.
+        marker = re.search(r"\{% for key in ([^%]+) %\}\s*"
+                           r"<input type=\"hidden\" name=\"__rendered_section\"",
+                           source)
+        assert marker, "marker loop not found"
+        assert "tiers.basic" in marker.group(1)
+        assert "tiers.advanced" in marker.group(1)
diff --git a/web_interface/blueprints/api_v3/__init__.py b/web_interface/blueprints/api_v3/__init__.py
index c1786667..b4f8c35d 100644
--- a/web_interface/blueprints/api_v3/__init__.py
+++ b/web_interface/blueprints/api_v3/__init__.py
@@ -1031,14 +1031,59 @@ def _set_nested_value(config, key_path, value):
     # value (e.g. the per-mode "inherit the base" override) and must overwrite
     # whatever was already stored.
     current[segments[-1]] = value
-def _set_missing_booleans_to_false(config, schema_props, form_keys, prefix='', config_node=None):
+
+
+#: Hidden field the rendered plugin form repeats once per top-level section it
+#: drew. Named with a leading underscore pair so the save path can drop it (and
+#: anything else meta) before treating form keys as config paths.
+_RENDERED_SECTION_FIELD = '__rendered_section'
+
+
+def _boolean_is_in_scope(full_path, prefix, sections, submitted_parents):
+    """Whether a missing checkbox at ``full_path`` may be forced to False.
+
+    "Missing" only means "unchecked" for a form that actually rendered the
+    control. A caller that posts a handful of fields -- a script, the MQTT
+    bridge, a curl against the documented endpoint -- never rendered anything,
+    and reading its silence as "every other checkbox is off" turns a one-field
+    save into a mass disable. That is not hypothetical: a partial post of four
+    ``customization.*`` keys switched off ``nfl.enabled``, ``ncaa_fb.enabled``
+    and every display-mode toggle on a live device.
+
+    Two ways to be in scope:
+
+    * ``sections`` -- the rendered form lists the top-level sections it drew
+      (``__rendered_section``). Anything it drew is fair game, including a
+      section whose only fields are checkboxes that are all unchecked, which
+      is the case no heuristic can recover.
+    * ``submitted_parents`` -- no marker, so fall back to evidence: the
+      containing object must have had at least one field posted.
+    """
+    if sections is not None:
+        return full_path.split('.', 1)[0] in sections
+    if submitted_parents is None:
+        return True
+    return prefix in submitted_parents
+
+
+def _submitted_parents(form_keys):
+    """The object paths a form actually posted a field from ('' = top level)."""
+    parents = set()
+    for key in form_keys:
+        parents.add(key.rsplit('.', 1)[0] if '.' in key else '')
+    return parents
+
+
+def _set_missing_booleans_to_false(config, schema_props, form_keys, prefix='', config_node=None,
+                                   sections=None, submitted_parents=None):
     """Walk schema and set missing boolean form fields to False.
 
     HTML checkboxes don't submit values when unchecked. When saving plugin config,
     the backend starts from existing config (to support partial form updates), which
     means an unchecked checkbox's old ``True`` value persists. This function detects
     boolean schema properties not present in the form submission and explicitly sets
-    them to ``False``.
+    them to ``False`` -- but only where that silence is evidence, see
+    :func:`_boolean_is_in_scope`.
 
     The top-level ``enabled`` field is excluded because it has its own preservation
     logic in the save endpoint.
@@ -1053,7 +1098,13 @@ def _set_missing_booleans_to_false(config, schema_props, form_keys, prefix='', c
         prefix: Dot-notation prefix for the current nesting level
         config_node: The current config subtree when inside an array item (avoids
                      using _set_nested_value which corrupts lists)
+        sections: Top-level sections the form reported rendering, or None when it
+                  reported none (then submitted_parents decides)
+        submitted_parents: Object paths with at least one posted field; computed
+                  on the first call when there are no section markers
     """
+    if sections is None and submitted_parents is None:
+        submitted_parents = _submitted_parents(form_keys)
     # Determine which config node to operate on
     node = config_node if config_node is not None else config
 
@@ -1065,8 +1116,11 @@ def _set_missing_booleans_to_false(config, schema_props, form_keys, prefix='', c
         prop_type = prop_schema.get('type')
 
         if prop_type == 'boolean' and full_path != 'enabled':
-            # If this boolean wasn't submitted in the form, it's an unchecked checkbox
-            if full_path not in form_keys:
+            # If this boolean wasn't submitted in the form, it's an unchecked
+            # checkbox -- provided the form drew it at all.
+            if (full_path not in form_keys
+                    and _boolean_is_in_scope(full_path, prefix, sections,
+                                             submitted_parents)):
                 if config_node is not None:
                     # Inside an array item — set directly on the item dict
                     node[prop_name] = False
@@ -1082,11 +1136,13 @@ def _set_missing_booleans_to_false(config, schema_props, form_keys, prefix='', c
                     node[prop_name] = {}
                 _set_missing_booleans_to_false(
                     config, prop_schema['properties'], form_keys, full_path,
-                    config_node=node[prop_name]
+                    config_node=node[prop_name],
+                    sections=sections, submitted_parents=submitted_parents
                 )
             else:
                 _set_missing_booleans_to_false(
-                    config, prop_schema['properties'], form_keys, full_path
+                    config, prop_schema['properties'], form_keys, full_path,
+                    sections=sections, submitted_parents=submitted_parents
                 )
 
         elif prop_type == 'array':
@@ -1136,7 +1192,8 @@ def _set_missing_booleans_to_false(config, schema_props, form_keys, prefix='', c
                     item_prefix = f"{full_path}.{idx}"
                     _set_missing_booleans_to_false(
                         config, items_schema['properties'], form_keys, item_prefix,
-                        config_node=array_list[idx]
+                        config_node=array_list[idx],
+                        sections=sections, submitted_parents=submitted_parents
                     )
 def _enhance_schema_with_core_properties(schema):
     """
diff --git a/web_interface/blueprints/api_v3/plugins.py b/web_interface/blueprints/api_v3/plugins.py
index a9cd252b..27d8a901 100644
--- a/web_interface/blueprints/api_v3/plugins.py
+++ b/web_interface/blueprints/api_v3/plugins.py
@@ -5,7 +5,7 @@ endpoint names are unchanged by living here.
 """
 from web_interface.blueprints.api_v3 import (
     ErrorCode, OperationType, PROJECT_ROOT, Path, Response,
-    _CALENDAR_LIST_MAX_PAGES, _SKIP_FIELD, _coerce_to_bool,
+    _CALENDAR_LIST_MAX_PAGES, _RENDERED_SECTION_FIELD, _SKIP_FIELD, _coerce_to_bool,
     _do_transactional_uninstall, _enhance_schema_with_core_properties,
     _filter_config_by_schema, _get_plugin_version, _get_schema_property,
     _installed_plugin_ids, _is_plugin_update_available,
@@ -1730,6 +1730,11 @@ def save_plugin_config():
             # Convert form data to config dict
             # Form fields can use dot notation for nested values (e.g., "transition.type")
             form_data = request.form.to_dict()
+            # Meta fields describe the submission, they are not config paths.
+            # Unknown keys are otherwise written straight into config.json by
+            # the non-indexed pass below.
+            form_data = {k: v for k, v in form_data.items()
+                         if not k.startswith('__')}
 
             # First pass: handle bracket notation array fields (e.g., "field_name[]" from checkbox-group)
             # These fields use getlist() to preserve all values, then replace in form_data
@@ -2089,7 +2094,14 @@ def save_plugin_config():
             # Walk the schema and set any boolean fields missing from form data to False.
             if schema and 'properties' in schema:
                 form_keys = set(request.form.keys())
-                _set_missing_booleans_to_false(plugin_config, schema['properties'], form_keys)
+                # The rendered form reports which top-level sections it drew, so
+                # an unchecked box can be told apart from a field the caller
+                # never had in front of it. A caller that sends none gets the
+                # evidence-based fallback in _boolean_is_in_scope.
+                rendered_sections = set(request.form.getlist(_RENDERED_SECTION_FIELD))
+                _set_missing_booleans_to_false(
+                    plugin_config, schema['properties'], form_keys,
+                    sections=rendered_sections or None)
 
         # Get schema manager instance (for JSON requests)
         schema_mgr = api_v3.schema_manager
diff --git a/web_interface/templates/v3/partials/plugin_config.html b/web_interface/templates/v3/partials/plugin_config.html
index 699e4081..07615448 100644
--- a/web_interface/templates/v3/partials/plugin_config.html
+++ b/web_interface/templates/v3/partials/plugin_config.html
@@ -1191,6 +1191,14 @@
                                 {% endif %}
                             {% endif %}
                         {% endfor %}
+                        {# Tell the save path which sections this form actually
+                           drew. An unchecked checkbox posts nothing, so without
+                           this the server cannot tell "the user cleared it"
+                           from "the caller never had that field" -- and a
+                           partial post would read as every box being off. #}
+                        {% for key in tiers.basic + tiers.advanced %}
+                            <input type="hidden" name="__rendered_section" value="{{ key }}">
+                        {% endfor %}
                         {% for key in tiers.basic %}
                             {% set prop = schema.properties[key] %}
                             {% set value = config[key] if key in config else none %}