From 500e7af224819ef49278a4cde88264fdba1deb37 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 17:24:00 +0000 Subject: [PATCH] fix(config): make the device location the default for plugin location fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A user in Kansas City reported their radar centred on Dallas, TX with nothing in config.json to explain it. The radar is the `ledmatrix-weather` plugin's `radar` mode, and it centres on the same coordinates as every other weather mode: `forecast_data` lat/lon, geocoded from the plugin's own `location_city` / `location_state` / `location_country`. Those ship with schema defaults of Dallas / Texas / US. A user who never opened the weather plugin's config form therefore has no `location_city` on disk, and `PluginManager` merges the schema default in at load time — so the whole plugin (not just the radar) silently runs on Dallas. Radar is just the only mode that draws a recognisable map and gives the mismatch away. Meanwhile the device-wide `location` block that General settings writes was read by nothing at all, despite its own help text promising it was "used for weather, sunrise/sunset, and other location-based content". `SchemaManager.generate_default_config()` now substitutes the device `location` into the three fully-namespaced `location_*` keys before handing defaults back, so the promise holds: - Only `location_city` / `location_state` / `location_country` are substituted. A bare `state` key is left alone — `ledmatrix-elections` uses it for a two-letter code, and rewriting it would break that plugin. - A value the user saved on the plugin still wins: this replaces the schema default, and `merge_with_defaults` puts user config on top. - The substitution is applied on the way out of the defaults cache rather than into it, so changing the device location takes effect immediately. - No config manager, no `location` block, or an unreadable config all fall back to the plugin's own schema defaults. Every caller benefits: the plugin loader, the config form (which now pre-fills the user's real city), config save, and reset-to-defaults. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GNLrSZ32FNKpHRaduKEJsg --- docs/CONFIG_REFERENCE.md | 2 +- src/plugin_system/plugin_manager.py | 3 +- src/plugin_system/schema_manager.py | 91 ++++++++- test/test_schema_manager_device_location.py | 179 ++++++++++++++++++ web_interface/app.py | 3 +- .../templates/v3/partials/general.html | 6 +- 6 files changed, 274 insertions(+), 10 deletions(-) create mode 100644 test/test_schema_manager_device_location.py diff --git a/docs/CONFIG_REFERENCE.md b/docs/CONFIG_REFERENCE.md index c54aca08..4af3c0fb 100644 --- a/docs/CONFIG_REFERENCE.md +++ b/docs/CONFIG_REFERENCE.md @@ -18,7 +18,7 @@ tooling against it. | `web_display_autostart` | bool, `true` | Whether the web interface service starts with the system | `scripts/utils/start_web_conditionally.py` | | `timezone` | string, `"America/New_York"` | IANA timezone for schedules and displays | `ConfigManager.get_timezone()` | | `target_fps` | int, `100` | Frame-rate ceiling for plugin rendering | `src/plugin_system/base_plugin.py`, `src/common/sports_scroll.py` | -| `location` | object | `city` / `state` / `country`, offered to plugins that need a location (weather, etc.) | plugins via merged config | +| `location` | object | `city` / `state` / `country`. Supplies the **default** for a plugin's own `location_city` / `location_state` / `location_country` setting, so weather, radar and friends follow this device without being configured twice. A value saved on the plugin itself still overrides it. | `SchemaManager.apply_device_location()`, then plugins via merged config | ## `schedule` — display on/off hours diff --git a/src/plugin_system/plugin_manager.py b/src/plugin_system/plugin_manager.py index 60356385..ae7d9ec8 100644 --- a/src/plugin_system/plugin_manager.py +++ b/src/plugin_system/plugin_manager.py @@ -71,7 +71,8 @@ class PluginManager: self.plugin_loader = PluginLoader(logger=self.logger) self.plugin_executor = PluginExecutor(default_timeout=30.0, logger=self.logger) self.state_manager = PluginStateManager(logger=self.logger) - self.schema_manager = SchemaManager(plugins_dir=self.plugins_dir, logger=self.logger) + self.schema_manager = SchemaManager(plugins_dir=self.plugins_dir, logger=self.logger, + config_manager=self.config_manager) # Lock protecting plugin_manifests and plugin_directories from # concurrent mutation (background reconciliation) and reads (requests). diff --git a/src/plugin_system/schema_manager.py b/src/plugin_system/schema_manager.py index 5b894100..3db4a5a2 100644 --- a/src/plugin_system/schema_manager.py +++ b/src/plugin_system/schema_manager.py @@ -26,7 +26,25 @@ class SchemaManager: - Cache invalidation on plugin changes """ - def __init__(self, plugins_dir: Optional[Path] = None, project_root: Optional[Path] = None, logger: Optional[logging.Logger] = None): + # Plugin config keys that mean "where this device is". A plugin declaring + # any of these in its schema gets the device-wide ``location`` block from + # config.json as the *default* for that field, instead of whatever city the + # plugin author happened to ship. A value the user set on the plugin itself + # always wins -- this only ever replaces the schema default, so an explicit + # per-plugin location is still honoured. + # + # Only these fully-namespaced keys are substituted. A bare ``state`` or + # ``city`` key is deliberately left alone: plugins use those for unrelated + # things (ledmatrix-elections' ``state`` is a two-letter code, not a place + # name), and silently rewriting them would break those plugins. + DEVICE_LOCATION_KEYS: Dict[str, str] = { + 'location_city': 'city', + 'location_state': 'state', + 'location_country': 'country', + } + + def __init__(self, plugins_dir: Optional[Path] = None, project_root: Optional[Path] = None, + logger: Optional[logging.Logger] = None, config_manager: Optional[Any] = None): """ Initialize the Schema Manager. @@ -34,10 +52,14 @@ class SchemaManager: plugins_dir: Base plugins directory path project_root: Project root directory path logger: Optional logger instance + config_manager: Optional config manager, used to resolve the + device-wide ``location`` that seeds plugin location defaults. + Omitting it simply leaves schema defaults untouched. """ self.logger = logger or logging.getLogger(__name__) self.plugins_dir = plugins_dir self.project_root = project_root or Path.cwd() + self.config_manager = config_manager # Schema cache: plugin_id -> schema dict self._schema_cache: Dict[str, Dict[str, Any]] = {} @@ -212,10 +234,70 @@ class SchemaManager: return defaults + def get_device_location(self) -> Optional[Dict[str, Any]]: + """ + Return the device-wide ``location`` block from config.json, or None. + + This is the City/State/Country the user sets once under General + settings. Returns None when there is no config manager wired, the + config can't be read, or no location has been configured. + """ + if self.config_manager is None: + return None + try: + config = self.config_manager.load_config() + except Exception as e: + # A config that can't be read must never stop defaults being + # generated -- the plugin's own schema defaults still apply. + self.logger.debug(f"Could not read device location from config: {e}") + return None + if not isinstance(config, dict): + return None + location = config.get('location') + return location if isinstance(location, dict) else None + + def apply_device_location(self, defaults: Dict[str, Any]) -> Dict[str, Any]: + """ + Replace location-shaped schema defaults with the device's own location. + + Without this, a plugin that ships ``"location_city": "Dallas"`` as its + schema default silently reports Dallas weather (and centres its radar + there) for every user who never opened that plugin's config form -- + even though they set their real city under General settings. The + substituted value is still only a *default*: ``merge_with_defaults`` + lets any per-plugin value the user saved win over it. + + Mutates and returns ``defaults`` for convenience. + """ + if not defaults: + return defaults + if not any(key in defaults for key in self.DEVICE_LOCATION_KEYS): + return defaults + + location = self.get_device_location() + if not location: + return defaults + + for key, field in self.DEVICE_LOCATION_KEYS.items(): + if key not in defaults: + continue + value = location.get(field) + # Only a non-empty string is a real answer; a blank or missing + # field means "not configured", which leaves the schema default. + if isinstance(value, str) and value.strip(): + defaults[key] = value.strip() + + return defaults + def generate_default_config(self, plugin_id: str, use_cache: bool = True) -> Dict[str, Any]: """ Generate default configuration for a plugin from its schema. + Location fields (see ``DEVICE_LOCATION_KEYS``) default to the device's + configured location rather than the plugin author's. That substitution + is applied on the way out rather than being cached, so changing the + device location takes effect without invalidating the defaults cache. + Args: plugin_id: Plugin identifier use_cache: If True, return cached defaults if available @@ -225,7 +307,7 @@ class SchemaManager: """ # Check cache first if use_cache and plugin_id in self._defaults_cache: - return self._defaults_cache[plugin_id].copy() + return self.apply_device_location(self._defaults_cache[plugin_id].copy()) schema = self.load_schema(plugin_id, use_cache=use_cache) if not schema: @@ -249,10 +331,11 @@ class SchemaManager: if 'live_priority' not in defaults: defaults['live_priority'] = schema.get('properties', {}).get('live_priority', {}).get('default', False) - # Cache the defaults + # Cache the defaults *before* the device location is layered on, so a + # later change to the device location is picked up by the next call. self._defaults_cache[plugin_id] = defaults.copy() - return defaults + return self.apply_device_location(defaults) def validate_config_against_schema(self, config: Dict[str, Any], schema: Dict[str, Any], plugin_id: Optional[str] = None) -> Tuple[bool, List[str]]: diff --git a/test/test_schema_manager_device_location.py b/test/test_schema_manager_device_location.py new file mode 100644 index 00000000..2908b95a --- /dev/null +++ b/test/test_schema_manager_device_location.py @@ -0,0 +1,179 @@ +""" +Tests for the device-location default: a plugin that ships a location field in +its schema must default to the device's configured City/State/Country, not to +whatever place the plugin author hard-coded. + +The bug this pins: ledmatrix-weather ships ``"location_city": "Dallas"`` as a +schema default, so a user who set Kansas City under General settings but never +opened the weather plugin's own config form got Dallas weather — and a radar +centred on Dallas — with nothing in config.json to explain it. +""" + +import json + +import pytest + +from src.plugin_system.schema_manager import SchemaManager + + +class FakeConfigManager: + """Minimal stand-in exposing the load_config() SchemaManager relies on.""" + + def __init__(self, config): + self.config = config + self.load_count = 0 + + def load_config(self): + self.load_count += 1 + return self.config + + +class ExplodingConfigManager: + def load_config(self): + raise OSError("config.json is unreadable") + + +WEATHER_SCHEMA = { + "type": "object", + "properties": { + "location_city": {"type": "string", "default": "Dallas"}, + "location_state": {"type": "string", "default": "Texas"}, + "location_country": {"type": "string", "default": "US"}, + "units": {"type": "string", "default": "imperial"}, + }, +} + + +def write_plugin(plugins_dir, plugin_id, schema): + plugin_dir = plugins_dir / plugin_id + plugin_dir.mkdir(parents=True, exist_ok=True) + (plugin_dir / "config_schema.json").write_text(json.dumps(schema)) + return plugin_dir + + +@pytest.fixture +def plugins_dir(tmp_path): + d = tmp_path / "plugin-repos" + d.mkdir() + return d + + +def make_sm(plugins_dir, tmp_path, location): + config = {} if location is None else {"location": location} + cm = FakeConfigManager(config) + sm = SchemaManager(plugins_dir=plugins_dir, project_root=tmp_path, + config_manager=cm) + return sm, cm + + +class TestDeviceLocationDefaults: + def test_device_location_replaces_plugin_default(self, plugins_dir, tmp_path): + write_plugin(plugins_dir, "ledmatrix-weather", WEATHER_SCHEMA) + sm, _ = make_sm(plugins_dir, tmp_path, + {"city": "Kansas City", "state": "Missouri", "country": "US"}) + + defaults = sm.generate_default_config("ledmatrix-weather") + + assert defaults["location_city"] == "Kansas City" + assert defaults["location_state"] == "Missouri" + assert defaults["location_country"] == "US" + # Non-location defaults are untouched. + assert defaults["units"] == "imperial" + + def test_user_set_plugin_value_still_wins(self, plugins_dir, tmp_path): + write_plugin(plugins_dir, "ledmatrix-weather", WEATHER_SCHEMA) + sm, _ = make_sm(plugins_dir, tmp_path, + {"city": "Kansas City", "state": "Missouri", "country": "US"}) + + defaults = sm.generate_default_config("ledmatrix-weather") + merged = sm.merge_with_defaults({"location_city": "Denver"}, defaults) + + assert merged["location_city"] == "Denver" + # Fields the user did not override still follow the device. + assert merged["location_state"] == "Missouri" + + def test_blank_and_missing_device_fields_leave_schema_default(self, plugins_dir, tmp_path): + write_plugin(plugins_dir, "ledmatrix-weather", WEATHER_SCHEMA) + sm, _ = make_sm(plugins_dir, tmp_path, {"city": "Kansas City", "state": " "}) + + defaults = sm.generate_default_config("ledmatrix-weather") + + assert defaults["location_city"] == "Kansas City" + assert defaults["location_state"] == "Texas" # blank -> not configured + assert defaults["location_country"] == "US" # absent -> schema default + + def test_no_device_location_configured_is_a_no_op(self, plugins_dir, tmp_path): + write_plugin(plugins_dir, "ledmatrix-weather", WEATHER_SCHEMA) + sm, _ = make_sm(plugins_dir, tmp_path, None) + + defaults = sm.generate_default_config("ledmatrix-weather") + + assert defaults["location_city"] == "Dallas" + + def test_no_config_manager_is_a_no_op(self, plugins_dir, tmp_path): + write_plugin(plugins_dir, "ledmatrix-weather", WEATHER_SCHEMA) + sm = SchemaManager(plugins_dir=plugins_dir, project_root=tmp_path) + + assert sm.generate_default_config("ledmatrix-weather")["location_city"] == "Dallas" + + def test_unreadable_config_falls_back_to_schema_defaults(self, plugins_dir, tmp_path): + write_plugin(plugins_dir, "ledmatrix-weather", WEATHER_SCHEMA) + sm = SchemaManager(plugins_dir=plugins_dir, project_root=tmp_path, + config_manager=ExplodingConfigManager()) + + assert sm.generate_default_config("ledmatrix-weather")["location_city"] == "Dallas" + + +class TestScopedToNamespacedKeys: + def test_bare_state_key_is_not_rewritten(self, plugins_dir, tmp_path): + """ledmatrix-elections' ``state`` is a two-letter code, not a place name.""" + write_plugin(plugins_dir, "ledmatrix-elections", { + "type": "object", + "properties": { + "state": {"type": "string", "default": "CA"}, + "city": {"type": "string", "default": "Springfield"}, + }, + }) + sm, _ = make_sm(plugins_dir, tmp_path, + {"city": "Kansas City", "state": "Missouri", "country": "US"}) + + defaults = sm.generate_default_config("ledmatrix-elections") + + assert defaults["state"] == "CA" + assert defaults["city"] == "Springfield" + + def test_plugin_without_location_fields_never_reads_config(self, plugins_dir, tmp_path): + write_plugin(plugins_dir, "clock-simple", { + "type": "object", + "properties": {"format": {"type": "string", "default": "12h"}}, + }) + sm, cm = make_sm(plugins_dir, tmp_path, {"city": "Kansas City"}) + + defaults = sm.generate_default_config("clock-simple") + + assert defaults["format"] == "12h" + assert cm.load_count == 0 + + +class TestCachingStaysFresh: + def test_location_change_is_picked_up_through_the_defaults_cache(self, plugins_dir, tmp_path): + write_plugin(plugins_dir, "ledmatrix-weather", WEATHER_SCHEMA) + sm, cm = make_sm(plugins_dir, tmp_path, {"city": "Kansas City"}) + + assert sm.generate_default_config("ledmatrix-weather")["location_city"] == "Kansas City" + + cm.config["location"]["city"] = "Omaha" + + # Second call is served from the defaults cache, but must not serve a + # stale location. + assert sm.generate_default_config("ledmatrix-weather")["location_city"] == "Omaha" + + def test_cached_defaults_are_not_mutated_by_the_overlay(self, plugins_dir, tmp_path): + write_plugin(plugins_dir, "ledmatrix-weather", WEATHER_SCHEMA) + sm, cm = make_sm(plugins_dir, tmp_path, {"city": "Kansas City"}) + + sm.generate_default_config("ledmatrix-weather") + assert sm._defaults_cache["ledmatrix-weather"]["location_city"] == "Dallas" + + cm.config.pop("location") + assert sm.generate_default_config("ledmatrix-weather")["location_city"] == "Dallas" diff --git a/web_interface/app.py b/web_interface/app.py index 8ed9cb8c..98bf0f71 100644 --- a/web_interface/app.py +++ b/web_interface/app.py @@ -118,7 +118,8 @@ saved_repositories_manager = SavedRepositoriesManager() schema_manager = SchemaManager( plugins_dir=plugins_dir, project_root=project_root, - logger=None + logger=None, + config_manager=config_manager ) # Initialize operation queue for plugin operations diff --git a/web_interface/templates/v3/partials/general.html b/web_interface/templates/v3/partials/general.html index eaf87a2c..ef6e8589 100644 --- a/web_interface/templates/v3/partials/general.html +++ b/web_interface/templates/v3/partials/general.html @@ -95,7 +95,7 @@
- +
- +
- +