rick/LEDMatrix
1
0
Fork 0
mirror of https://github.com/ChuckBuilds/LEDMatrix.git synced 2026-04-11 13:23:00 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: address second round of review findings

Browse Source 
- Tests: replace conditional `if response.status_code == 200` guards
  with unconditional `assert response.status_code == 200` so failures
  are not silently swallowed
- dotToNested: guard finalKey write with `if (i < parts.length)` to
  prevent empty-string key pollution when tail-matching consumed all
  parts
- Extract normalizeFormDataForConfig() helper from handlePluginConfigSubmit
  and call it from both handlePluginConfigSubmit and syncFormToJson so
  the JSON tab sync uses the same robust FormData processing (including
  _data JSON inputs, bracket-notation checkboxes, array-of-objects,
  file-upload widgets, checkbox DOM detection, and unchecked boolean
  handling via collectBooleanFields)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
ChuckBuilds
2026-03-26 19:49:16 -04:00
parent 4585cc1eff
commit a6d12e3953
2 changed files with 79 additions and 103 deletions
Download Patch File Download Diff File Expand all files Collapse all files

25
test/test_web_api.py
View File

@@ -643,13 +643,13 @@ class TestDottedKeyNormalization:
content_type='application/json', content_type='application/json',
) )
if response.status_code == 200: assert response.status_code == 200, f"Expected 200, got {response.status_code}: {response.data}"
saved = mock_config_manager.save_config_atomic.call_args[0][0] saved = mock_config_manager.save_config_atomic.call_args[0][0]
soccer_cfg = saved.get('soccer-scoreboard', {}) soccer_cfg = saved.get('soccer-scoreboard', {})
leagues = soccer_cfg.get('leagues', {}) leagues = soccer_cfg.get('leagues', {})
assert 'eng.1' in leagues, f"Expected 'eng.1' key, got: {list(leagues.keys())}" assert 'eng.1' in leagues, f"Expected 'eng.1' key, got: {list(leagues.keys())}"
assert isinstance(leagues['eng.1'].get('favorite_teams'), list) assert isinstance(leagues['eng.1'].get('favorite_teams'), list)
assert leagues['eng.1']['favorite_teams'] == ['Arsenal', 'Chelsea'] assert leagues['eng.1']['favorite_teams'] == ['Arsenal', 'Chelsea']
def test_save_plugin_config_none_array_gets_default(self, client, mock_config_manager): def test_save_plugin_config_none_array_gets_default(self, client, mock_config_manager):
"""None array fields under dotted-key parents are replaced with defaults.""" """None array fields under dotted-key parents are replaced with defaults."""
@@ -704,8 +704,9 @@ class TestDottedKeyNormalization:
content_type='application/json', content_type='application/json',
) )
if response.status_code == 200: assert response.status_code == 200, f"Expected 200, got {response.status_code}: {response.data}"
saved = mock_config_manager.save_config_atomic.call_args[0][0] saved = mock_config_manager.save_config_atomic.call_args[0][0]
soccer_cfg = saved.get('soccer-scoreboard', {}) soccer_cfg = saved.get('soccer-scoreboard', {})
teams = soccer_cfg.get('leagues', {}).get('eng.1', {}).get('favorite_teams') teams = soccer_cfg.get('leagues', {}).get('eng.1', {}).get('favorite_teams')
assert isinstance(teams, list), f"Expected list, got: {type(teams)}" assert isinstance(teams, list), f"Expected list, got: {type(teams)}"
assert teams == [], f"Expected empty default list, got: {teams}"

157
web_interface/static/v3/plugins_manager.js
View File

@@ -2378,8 +2378,11 @@ function dotToNested(obj, schema) {
} }
// Set the final key (remaining parts joined — may itself be dotted) // Set the final key (remaining parts joined — may itself be dotted)
const finalKey = parts.slice(i).join('.'); // Skip if tail-matching already consumed all parts and wrote the value
current[finalKey] = obj[key]; if (i < parts.length) {
const finalKey = parts.slice(i).join('.');
current[finalKey] = obj[key];
}
} }
return result; return result;
@@ -2404,42 +2407,20 @@ function collectBooleanFields(schema, prefix = '') {
return boolFields; return boolFields;
} }
function handlePluginConfigSubmit(e) { /**
e.preventDefault(); * Normalize FormData from a plugin config form into a nested config object.
console.log('Form submitted'); * Handles _data JSON inputs, bracket-notation checkboxes, array-of-objects,
* file-upload widgets, proper checkbox DOM detection, unchecked boolean
if (!currentPluginConfig) { * handling, and schema-aware dotted-key nesting.
showNotification('Plugin configuration not loaded', 'error'); *
return; * @param {HTMLFormElement} form - The form element (needed for checkbox DOM detection)
} * @param {Object|null} schema - The plugin's JSON Schema
* @returns {Object} Nested config object ready for saving
const pluginId = currentPluginConfig.pluginId; */
const schema = currentPluginConfig.schema; function normalizeFormDataForConfig(form, schema) {
const form = e.target;
// Fix invalid hidden fields before submission
// This prevents "invalid form control is not focusable" errors
const allInputs = form.querySelectorAll('input[type="number"]');
allInputs.forEach(input => {
const min = parseFloat(input.getAttribute('min'));
const max = parseFloat(input.getAttribute('max'));
const value = parseFloat(input.value);
if (!isNaN(value)) {
if (!isNaN(min) && value < min) {
input.value = min;
} else if (!isNaN(max) && value > max) {
input.value = max;
}
}
});
const formData = new FormData(form); const formData = new FormData(form);
const flatConfig = {}; const flatConfig = {};
console.log('Schema loaded:', schema ? 'Yes' : 'No');
// Process form data with type conversion (using dot notation for nested fields)
for (const [key, value] of formData.entries()) { for (const [key, value] of formData.entries()) {
// Check if this is a patternProperties or array-of-objects hidden input (contains JSON data) // Check if this is a patternProperties or array-of-objects hidden input (contains JSON data)
// Only match keys ending with '_data' to avoid false positives like 'meta_data_field' // Only match keys ending with '_data' to avoid false positives like 'meta_data_field'
@@ -2451,36 +2432,35 @@ function handlePluginConfigSubmit(e) {
// Only treat as JSON-backed when it's a non-null object (null is typeof 'object' in JavaScript) // Only treat as JSON-backed when it's a non-null object (null is typeof 'object' in JavaScript)
if (jsonValue !== null && typeof jsonValue === 'object') { if (jsonValue !== null && typeof jsonValue === 'object') {
flatConfig[baseKey] = jsonValue; flatConfig[baseKey] = jsonValue;
console.log(`JSON data field ${baseKey}: parsed ${Array.isArray(jsonValue) ? 'array' : 'object'}`, jsonValue);
continue; // Skip normal processing for JSON data fields continue; // Skip normal processing for JSON data fields
} }
} catch (e) { } catch (e) {
// Not valid JSON, continue with normal processing // Not valid JSON, continue with normal processing
} }
} }
// Skip checkbox-group inputs with bracket notation (they're handled by the hidden _data input) // Skip checkbox-group inputs with bracket notation (they're handled by the hidden _data input)
// Pattern: fieldName[] - these are individual checkboxes, actual data is in fieldName_data // Pattern: fieldName[] - these are individual checkboxes, actual data is in fieldName_data
if (key.endsWith('[]')) { if (key.endsWith('[]')) {
continue; continue;
} }
// Skip key_value pair inputs (they're handled by the hidden _data input) // Skip key_value pair inputs (they're handled by the hidden _data input)
if (key.includes('[key_') || key.includes('[value_')) { if (key.includes('[key_') || key.includes('[value_')) {
continue; continue;
} }
// Skip array-of-objects per-item inputs (they're handled by the hidden _data input) // Skip array-of-objects per-item inputs (they're handled by the hidden _data input)
// Pattern: feeds_item_0_name, feeds_item_1_url, etc. // Pattern: feeds_item_0_name, feeds_item_1_url, etc.
if (key.includes('_item_') && /_item_\d+_/.test(key)) { if (key.includes('_item_') && /_item_\d+_/.test(key)) {
continue; continue;
} }
// Try to get schema property - handle both dot notation and underscore notation // Try to get schema property - handle both dot notation and underscore notation
let propSchema = getSchemaPropertyType(schema, key); let propSchema = getSchemaPropertyType(schema, key);
let actualKey = key; let actualKey = key;
let actualValue = value; let actualValue = value;
// If not found with dots, try converting underscores to dots (for nested fields) // If not found with dots, try converting underscores to dots (for nested fields)
if (!propSchema && key.includes('_')) { if (!propSchema && key.includes('_')) {
const dotKey = key.replace(/_/g, '.'); const dotKey = key.replace(/_/g, '.');
@@ -2491,10 +2471,10 @@ function handlePluginConfigSubmit(e) {
actualValue = value; actualValue = value;
} }
} }
if (propSchema) { if (propSchema) {
const propType = propSchema.type; const propType = propSchema.type;
if (propType === 'array') { if (propType === 'array') {
// Check if this is a file upload widget (JSON array) // Check if this is a file upload widget (JSON array)
if (propSchema['x-widget'] === 'file-upload') { if (propSchema['x-widget'] === 'file-upload') {
@@ -2508,11 +2488,10 @@ function handlePluginConfigSubmit(e) {
tempDiv.innerHTML = actualValue; tempDiv.innerHTML = actualValue;
decodedValue = tempDiv.textContent || tempDiv.innerText || actualValue; decodedValue = tempDiv.textContent || tempDiv.innerText || actualValue;
} }
const jsonValue = JSON.parse(decodedValue); const jsonValue = JSON.parse(decodedValue);
if (Array.isArray(jsonValue)) { if (Array.isArray(jsonValue)) {
flatConfig[actualKey] = jsonValue; flatConfig[actualKey] = jsonValue;
console.log(`File upload array field ${actualKey}: parsed JSON array with ${jsonValue.length} items`);
} else { } else {
// Fallback to comma-separated // Fallback to comma-separated
const arrayValue = decodedValue ? decodedValue.split(',').map(v => v.trim()).filter(v => v) : []; const arrayValue = decodedValue ? decodedValue.split(',').map(v => v.trim()).filter(v => v) : [];
@@ -2522,13 +2501,11 @@ function handlePluginConfigSubmit(e) {
// Not JSON, use comma-separated // Not JSON, use comma-separated
const arrayValue = actualValue ? actualValue.split(',').map(v => v.trim()).filter(v => v) : []; const arrayValue = actualValue ? actualValue.split(',').map(v => v.trim()).filter(v => v) : [];
flatConfig[actualKey] = arrayValue; flatConfig[actualKey] = arrayValue;
console.log(`Array field ${actualKey}: "${actualValue}" -> `, arrayValue);
} }
} else { } else {
// Regular array: convert comma-separated string to array // Regular array: convert comma-separated string to array
const arrayValue = actualValue ? actualValue.split(',').map(v => v.trim()).filter(v => v) : []; const arrayValue = actualValue ? actualValue.split(',').map(v => v.trim()).filter(v => v) : [];
flatConfig[actualKey] = arrayValue; flatConfig[actualKey] = arrayValue;
console.log(`Array field ${actualKey}: "${actualValue}" -> `, arrayValue);
} }
} else if (propType === 'integer') { } else if (propType === 'integer') {
flatConfig[actualKey] = parseInt(actualValue, 10); flatConfig[actualKey] = parseInt(actualValue, 10);
@@ -2539,14 +2516,13 @@ function handlePluginConfigSubmit(e) {
// Escape special CSS selector characters in the name // Escape special CSS selector characters in the name
const escapedKey = escapeCssSelector(key); const escapedKey = escapeCssSelector(key);
const formElement = form.querySelector(`input[type="checkbox"][name="${escapedKey}"]`); const formElement = form.querySelector(`input[type="checkbox"][name="${escapedKey}"]`);
if (formElement) { if (formElement) {
// Element found - use its checked state // Element found - use its checked state
flatConfig[actualKey] = formElement.checked; flatConfig[actualKey] = formElement.checked;
} else { } else {
// Element not found - normalize string booleans and check FormData value // Element not found - normalize string booleans and check FormData value
// Checkboxes send "on" when checked, nothing when unchecked // Checkboxes send "on" when checked, nothing when unchecked
// Normalize string representations of booleans
if (typeof actualValue === 'string') { if (typeof actualValue === 'string') {
const lowerValue = actualValue.toLowerCase().trim(); const lowerValue = actualValue.toLowerCase().trim();
if (lowerValue === 'true' || lowerValue === '1' || lowerValue === 'on') { if (lowerValue === 'true' || lowerValue === '1' || lowerValue === 'on') {
@@ -2554,13 +2530,11 @@ function handlePluginConfigSubmit(e) {
} else if (lowerValue === 'false' || lowerValue === '0' || lowerValue === 'off' || lowerValue === '') { } else if (lowerValue === 'false' || lowerValue === '0' || lowerValue === 'off' || lowerValue === '') {
flatConfig[actualKey] = false; flatConfig[actualKey] = false;
} else { } else {
// Non-empty string that's not a boolean representation - treat as truthy
flatConfig[actualKey] = true; flatConfig[actualKey] = true;
} }
} else if (actualValue === undefined || actualValue === null) { } else if (actualValue === undefined || actualValue === null) {
flatConfig[actualKey] = false; flatConfig[actualKey] = false;
} else { } else {
// Non-string value - coerce to boolean
flatConfig[actualKey] = Boolean(actualValue); flatConfig[actualKey] = Boolean(actualValue);
} }
} }
@@ -2577,10 +2551,9 @@ function handlePluginConfigSubmit(e) {
const tempDiv = document.createElement('div'); const tempDiv = document.createElement('div');
tempDiv.innerHTML = actualValue; tempDiv.innerHTML = actualValue;
decodedValue = tempDiv.textContent || tempDiv.innerText || actualValue; decodedValue = tempDiv.textContent || tempDiv.innerText || actualValue;
const parsed = JSON.parse(decodedValue); const parsed = JSON.parse(decodedValue);
flatConfig[actualKey] = parsed; flatConfig[actualKey] = parsed;
console.log(`No schema for ${actualKey}, but parsed as JSON:`, parsed);
} catch (e) { } catch (e) {
// Not valid JSON, save as string // Not valid JSON, save as string
flatConfig[actualKey] = actualValue; flatConfig[actualKey] = actualValue;
@@ -2589,12 +2562,10 @@ function handlePluginConfigSubmit(e) {
// No schema - try to detect checkbox by finding the element // No schema - try to detect checkbox by finding the element
const escapedKey = escapeCssSelector(key); const escapedKey = escapeCssSelector(key);
const formElement = form.querySelector(`input[type="checkbox"][name="${escapedKey}"]`); const formElement = form.querySelector(`input[type="checkbox"][name="${escapedKey}"]`);
if (formElement && formElement.type === 'checkbox') { if (formElement && formElement.type === 'checkbox') {
// Found checkbox element - use its checked state
flatConfig[actualKey] = formElement.checked; flatConfig[actualKey] = formElement.checked;
} else { } else {
// Not a checkbox or element not found - normalize string booleans
if (typeof actualValue === 'string') { if (typeof actualValue === 'string') {
const lowerValue = actualValue.toLowerCase().trim(); const lowerValue = actualValue.toLowerCase().trim();
if (lowerValue === 'true' || lowerValue === '1' || lowerValue === 'on') { if (lowerValue === 'true' || lowerValue === '1' || lowerValue === 'on') {
@@ -2602,18 +2573,16 @@ function handlePluginConfigSubmit(e) {
} else if (lowerValue === 'false' || lowerValue === '0' || lowerValue === 'off' || lowerValue === '') { } else if (lowerValue === 'false' || lowerValue === '0' || lowerValue === 'off' || lowerValue === '') {
flatConfig[actualKey] = false; flatConfig[actualKey] = false;
} else { } else {
// Non-empty string that's not a boolean representation - keep as string
flatConfig[actualKey] = actualValue; flatConfig[actualKey] = actualValue;
} }
} else { } else {
// Non-string value - use as-is
flatConfig[actualKey] = actualValue; flatConfig[actualKey] = actualValue;
} }
} }
} }
} }
} }
// Handle unchecked checkboxes (not in FormData) - including nested ones // Handle unchecked checkboxes (not in FormData) - including nested ones
if (schema && schema.properties) { if (schema && schema.properties) {
const allBoolFields = collectBooleanFields(schema); const allBoolFields = collectBooleanFields(schema);
@@ -2623,11 +2592,43 @@ function handlePluginConfigSubmit(e) {
} }
}); });
} }
// Convert dot notation to nested object // Convert dot notation to nested object
const config = dotToNested(flatConfig, schema); return dotToNested(flatConfig, schema);
}
console.log('Flat config:', flatConfig);
function handlePluginConfigSubmit(e) {
e.preventDefault();
console.log('Form submitted');
if (!currentPluginConfig) {
showNotification('Plugin configuration not loaded', 'error');
return;
}
const pluginId = currentPluginConfig.pluginId;
const schema = currentPluginConfig.schema;
const form = e.target;
// Fix invalid hidden fields before submission
// This prevents "invalid form control is not focusable" errors
const allInputs = form.querySelectorAll('input[type="number"]');
allInputs.forEach(input => {
const min = parseFloat(input.getAttribute('min'));
const max = parseFloat(input.getAttribute('max'));
const value = parseFloat(input.value);
if (!isNaN(value)) {
if (!isNaN(min) && value < min) {
input.value = min;
} else if (!isNaN(max) && value > max) {
input.value = max;
}
}
});
const config = normalizeFormDataForConfig(form, schema);
console.log('Nested config to save:', config); console.log('Nested config to save:', config);
// Save the configuration // Save the configuration
@@ -4473,34 +4474,8 @@ function syncFormToJson() {
const form = document.getElementById('plugin-config-form'); const form = document.getElementById('plugin-config-form');
if (!form) return; if (!form) return;
const formData = new FormData(form);
const flatConfig = {};
// Get schema for type conversion
const schema = currentPluginConfigState.schema; const schema = currentPluginConfigState.schema;
const config = normalizeFormDataForConfig(form, schema);
for (let [key, value] of formData.entries()) {
if (key === 'enabled') continue; // Skip enabled, managed separately
// Use schema-aware property lookup for type conversion
const prop = schema ? getSchemaProperty(schema, key) : null;
// Type conversion based on schema
if (prop?.type === 'array') {
flatConfig[key] = value.split(',').map(item => item.trim()).filter(item => item.length > 0);
} else if (prop?.type === 'integer' || key === 'display_duration') {
flatConfig[key] = parseInt(value) || 0;
} else if (prop?.type === 'number') {
flatConfig[key] = parseFloat(value) || 0;
} else if (prop?.type === 'boolean') {
flatConfig[key] = value === 'true' || value === true;
} else {
flatConfig[key] = value;
}
}
// Convert flat dot-notation keys to nested object using schema-aware matching
const config = dotToNested(flatConfig, schema);
// Deep merge with existing config to preserve nested structures // Deep merge with existing config to preserve nested structures
function deepMerge(target, source) { function deepMerge(target, source) {