mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-01 16:58:06 +00:00
fix(web): don't validate double-sided settings when the feature is disabled
Saving anything on the Display tab failed with a 400 when double-sided
mode was off:
Double-sided copies (2) must divide chain length (3) evenly
The Display form posts every field in one request, including
double_sided_copies (default 2) and double_sided_axis, whether or not
the Enabled checkbox is ticked. The server block was gated only on "is
any double-sided field present in the payload" — it wrote
ds_config['enabled'] but never read it. A user with chain_length: 3 and
the untouched default copies: 2 was locked out of saving any display
setting at all: brightness, GPIO slowdown, Vegas, sync.
Gate the checks on the enabled flag:
- Divisibility against chain_length/parallel is hardware-relational and
only runs when the feature is on.
- Structural checks (copies parses as an int in 2..8, axis in the
whitelist) still 400 when enabled; when disabled they drop the value
and leave the stored one untouched rather than rejecting the save.
The runtime already gated correctly (_resolve_double_sided returns None
when disabled), so nothing there changes.
Also in the Display tab:
- Hide Copies / Split Axis until Enabled is ticked, mirroring the Vegas
Scroll pattern. Hidden rather than disabled, so the fields keep
submitting and the server still sees an 'off' state to persist.
- Fix the save toast: the form's handler read xhr.responseJSON, a jQuery
property that doesn't exist on a native XMLHttpRequest, so it was
always undefined and every save reported a green "Display settings
saved" — even the 400s. Parse responseText and use the real status.
Tests: two existing double-sided tests asserted 200 on payloads that the
divisibility check (added later, in #373) turns into 400s; the first now
supplies matching hardware values and the second passes as written now
that a disabled save skips the check. Added coverage for the reported
regression, for bad values while disabled, and for the check still
firing when enabled.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016aUvzTXpEYhNEWYQvFqQU9
This commit is contained in:
@@ -146,6 +146,11 @@ class TestConfigAPI:
|
|||||||
|
|
||||||
def test_save_double_sided_settings(self, client, mock_config_manager):
|
def test_save_double_sided_settings(self, client, mock_config_manager):
|
||||||
"""Double-sided form fields are persisted under display.double_sided."""
|
"""Double-sided form fields are persisted under display.double_sided."""
|
||||||
|
# 2 copies on the vertical axis needs parallel to be a multiple of 2.
|
||||||
|
mock_config_manager.load_config.return_value['display']['hardware'] = {
|
||||||
|
'chain_length': 2, 'parallel': 2,
|
||||||
|
}
|
||||||
|
|
||||||
response = client.post(
|
response = client.post(
|
||||||
'/api/v3/config/main',
|
'/api/v3/config/main',
|
||||||
data={
|
data={
|
||||||
@@ -175,6 +180,70 @@ class TestConfigAPI:
|
|||||||
assert ds['enabled'] is False
|
assert ds['enabled'] is False
|
||||||
assert ds['copies'] == 4
|
assert ds['copies'] == 4
|
||||||
|
|
||||||
|
def test_save_double_sided_disabled_skips_divisibility_check(self, client, mock_config_manager):
|
||||||
|
"""A copies/chain_length mismatch must not block saves while disabled.
|
||||||
|
|
||||||
|
The Display form posts copies/axis on every save, so validating them
|
||||||
|
with the feature off locked users out of every other display setting.
|
||||||
|
"""
|
||||||
|
mock_config_manager.load_config.return_value['display']['hardware'] = {
|
||||||
|
'chain_length': 3, 'parallel': 1,
|
||||||
|
}
|
||||||
|
|
||||||
|
response = client.post(
|
||||||
|
'/api/v3/config/main',
|
||||||
|
data={
|
||||||
|
'double_sided_copies': '2',
|
||||||
|
'double_sided_axis': 'horizontal',
|
||||||
|
'brightness': '75',
|
||||||
|
},
|
||||||
|
content_type='application/x-www-form-urlencoded',
|
||||||
|
)
|
||||||
|
|
||||||
|
assert response.status_code == 200
|
||||||
|
ds = mock_config_manager.save_config_atomic.call_args[0][0]['display']['double_sided']
|
||||||
|
assert ds['enabled'] is False
|
||||||
|
assert ds['copies'] == 2
|
||||||
|
|
||||||
|
def test_save_double_sided_enabled_enforces_divisibility(self, client, mock_config_manager):
|
||||||
|
"""The same mismatch is still rejected once the feature is turned on."""
|
||||||
|
mock_config_manager.load_config.return_value['display']['hardware'] = {
|
||||||
|
'chain_length': 3, 'parallel': 1,
|
||||||
|
}
|
||||||
|
|
||||||
|
response = client.post(
|
||||||
|
'/api/v3/config/main',
|
||||||
|
data={
|
||||||
|
'double_sided_enabled': 'true',
|
||||||
|
'double_sided_copies': '2',
|
||||||
|
'double_sided_axis': 'horizontal',
|
||||||
|
},
|
||||||
|
content_type='application/x-www-form-urlencoded',
|
||||||
|
)
|
||||||
|
|
||||||
|
assert response.status_code == 400
|
||||||
|
assert 'chain length' in response.get_json()['message']
|
||||||
|
mock_config_manager.save_config_atomic.assert_not_called()
|
||||||
|
|
||||||
|
def test_save_double_sided_disabled_ignores_bad_values(self, client, mock_config_manager):
|
||||||
|
"""While disabled, unusable copies/axis are dropped rather than rejected."""
|
||||||
|
mock_config_manager.load_config.return_value['display']['double_sided'] = {
|
||||||
|
'enabled': True, 'copies': 2, 'axis': 'horizontal',
|
||||||
|
}
|
||||||
|
|
||||||
|
response = client.post(
|
||||||
|
'/api/v3/config/main',
|
||||||
|
data={'double_sided_copies': 'abc', 'double_sided_axis': 'diagonal'},
|
||||||
|
content_type='application/x-www-form-urlencoded',
|
||||||
|
)
|
||||||
|
|
||||||
|
assert response.status_code == 200
|
||||||
|
ds = mock_config_manager.save_config_atomic.call_args[0][0]['display']['double_sided']
|
||||||
|
assert ds['enabled'] is False
|
||||||
|
# Stored values left untouched rather than overwritten with junk.
|
||||||
|
assert ds['copies'] == 2
|
||||||
|
assert ds['axis'] == 'horizontal'
|
||||||
|
|
||||||
def test_save_double_sided_invalid_copies_rejected(self, client, mock_config_manager):
|
def test_save_double_sided_invalid_copies_rejected(self, client, mock_config_manager):
|
||||||
"""copies < 2 is rejected with a 400 before any save."""
|
"""copies < 2 is rejected with a 400 before any save."""
|
||||||
response = client.post(
|
response = client.post(
|
||||||
|
|||||||
@@ -864,16 +864,15 @@ def save_main_config():
|
|||||||
ds_config = current_config['display']['double_sided']
|
ds_config = current_config['display']['double_sided']
|
||||||
|
|
||||||
# Enabled checkbox: omitted from the form when unchecked.
|
# Enabled checkbox: omitted from the form when unchecked.
|
||||||
ds_config['enabled'] = _coerce_to_bool(data.get('double_sided_enabled'))
|
# The Display form posts copies/axis on every save regardless of this
|
||||||
|
# checkbox, so when the feature is off we accept the values without
|
||||||
|
# rejecting the whole save — otherwise a stale copies/chain_length
|
||||||
|
# mismatch locks the user out of every other display setting.
|
||||||
|
enabled = _coerce_to_bool(data.get('double_sided_enabled'))
|
||||||
|
ds_config['enabled'] = enabled
|
||||||
|
|
||||||
if 'double_sided_copies' in data and data['double_sided_copies'] not in ('', None):
|
def _copies_fits_hardware(copies):
|
||||||
try:
|
"""Error message if copies doesn't divide the panel evenly, else None."""
|
||||||
copies = int(data['double_sided_copies'])
|
|
||||||
except (ValueError, TypeError):
|
|
||||||
return jsonify({'status': 'error', 'message': "Double-sided copies must be an integer"}), 400
|
|
||||||
if not (2 <= copies <= 8):
|
|
||||||
return jsonify({'status': 'error', 'message': "Double-sided copies must be between 2 and 8"}), 400
|
|
||||||
# Validate divisibility against the relevant hardware dimension.
|
|
||||||
# Use axis from this request if provided, else from stored config.
|
# Use axis from this request if provided, else from stored config.
|
||||||
hw = current_config.get('display', {}).get('hardware', {})
|
hw = current_config.get('display', {}).get('hardware', {})
|
||||||
effective_axis = (data.get('double_sided_axis')
|
effective_axis = (data.get('double_sided_axis')
|
||||||
@@ -881,17 +880,40 @@ def save_main_config():
|
|||||||
if effective_axis == 'horizontal':
|
if effective_axis == 'horizontal':
|
||||||
chain_length = int(hw.get('chain_length', 2) or 2)
|
chain_length = int(hw.get('chain_length', 2) or 2)
|
||||||
if chain_length % copies != 0:
|
if chain_length % copies != 0:
|
||||||
return jsonify({'status': 'error', 'message': f"Double-sided copies ({copies}) must divide chain length ({chain_length}) evenly"}), 400
|
return f"Double-sided copies ({copies}) must divide chain length ({chain_length}) evenly"
|
||||||
elif effective_axis == 'vertical':
|
elif effective_axis == 'vertical':
|
||||||
parallel = int(hw.get('parallel', 1) or 1)
|
parallel = int(hw.get('parallel', 1) or 1)
|
||||||
if parallel % copies != 0:
|
if parallel % copies != 0:
|
||||||
return jsonify({'status': 'error', 'message': f"Double-sided copies ({copies}) must divide parallel ({parallel}) evenly"}), 400
|
return f"Double-sided copies ({copies}) must divide parallel ({parallel}) evenly"
|
||||||
|
return None
|
||||||
|
|
||||||
|
if 'double_sided_copies' in data and data['double_sided_copies'] not in ('', None):
|
||||||
|
copies = None
|
||||||
|
try:
|
||||||
|
copies = int(data['double_sided_copies'])
|
||||||
|
except (ValueError, TypeError):
|
||||||
|
if enabled:
|
||||||
|
return jsonify({'status': 'error', 'message': "Double-sided copies must be an integer"}), 400
|
||||||
|
if copies is not None and not (2 <= copies <= 8):
|
||||||
|
if enabled:
|
||||||
|
return jsonify({'status': 'error', 'message': "Double-sided copies must be between 2 and 8"}), 400
|
||||||
|
# Disabled: leave the stored value alone rather than writing junk.
|
||||||
|
copies = None
|
||||||
|
if copies is not None:
|
||||||
|
# Divisibility is a hardware-relational check — only meaningful
|
||||||
|
# when the feature is actually on.
|
||||||
|
if enabled:
|
||||||
|
fit_error = _copies_fits_hardware(copies)
|
||||||
|
if fit_error:
|
||||||
|
return jsonify({'status': 'error', 'message': fit_error}), 400
|
||||||
ds_config['copies'] = copies
|
ds_config['copies'] = copies
|
||||||
|
|
||||||
if 'double_sided_axis' in data:
|
if 'double_sided_axis' in data:
|
||||||
axis = data['double_sided_axis']
|
axis = data['double_sided_axis']
|
||||||
if axis not in ('horizontal', 'vertical'):
|
if axis not in ('horizontal', 'vertical'):
|
||||||
|
if enabled:
|
||||||
return jsonify({'status': 'error', 'message': "Double-sided axis must be 'horizontal' or 'vertical'"}), 400
|
return jsonify({'status': 'error', 'message': "Double-sided axis must be 'horizontal' or 'vertical'"}), 400
|
||||||
|
else:
|
||||||
ds_config['axis'] = axis
|
ds_config['axis'] = axis
|
||||||
|
|
||||||
# Handle Vegas scroll mode settings
|
# Handle Vegas scroll mode settings
|
||||||
|
|||||||
@@ -30,7 +30,7 @@
|
|||||||
hx-ext="json-enc"
|
hx-ext="json-enc"
|
||||||
hx-headers='{"Content-Type": "application/json"}'
|
hx-headers='{"Content-Type": "application/json"}'
|
||||||
hx-swap="none"
|
hx-swap="none"
|
||||||
hx-on:htmx:after-request="showNotification(event.detail.xhr.responseJSON?.message || 'Display settings saved', event.detail.xhr.responseJSON?.status || 'success')"
|
hx-on:htmx:after-request="showDisplaySaveResult(event.detail.xhr)"
|
||||||
class="space-y-6"
|
class="space-y-6"
|
||||||
novalidate
|
novalidate
|
||||||
onsubmit="fixInvalidNumberInputs(this); return true;">
|
onsubmit="fixInvalidNumberInputs(this); return true;">
|
||||||
@@ -475,8 +475,7 @@
|
|||||||
<h3 class="text-md font-medium text-gray-900 mb-1">Double-Sided Display</h3>
|
<h3 class="text-md font-medium text-gray-900 mb-1">Double-Sided Display</h3>
|
||||||
<p class="text-sm text-gray-600 mb-4">Show the same content on every panel in the chain — e.g. two 64×32 panels mirrored, or four panels as two identical screens. Rendered once and duplicated, so it adds no extra CPU. Takes effect after a display restart.</p>
|
<p class="text-sm text-gray-600 mb-4">Show the same content on every panel in the chain — e.g. two 64×32 panels mirrored, or four panels as two identical screens. Rendered once and duplicated, so it adds no extra CPU. Takes effect after a display restart.</p>
|
||||||
|
|
||||||
<div class="grid grid-cols-1 md:grid-cols-3 gap-4">
|
<div class="form-group mb-4" id="setting-display-double_sided_enabled" data-setting-key="display.double_sided.enabled">
|
||||||
<div class="form-group" id="setting-display-double_sided_enabled" data-setting-key="display.double_sided.enabled">
|
|
||||||
<label class="flex items-center gap-2">
|
<label class="flex items-center gap-2">
|
||||||
<input type="checkbox"
|
<input type="checkbox"
|
||||||
id="double_sided_enabled"
|
id="double_sided_enabled"
|
||||||
@@ -489,8 +488,13 @@
|
|||||||
</label>
|
</label>
|
||||||
</div>
|
</div>
|
||||||
|
|
||||||
|
<!-- Hidden (not disabled) when the feature is off, so the values are
|
||||||
|
still submitted and round-trip through a save. -->
|
||||||
|
<div id="double_sided_settings"
|
||||||
|
class="grid grid-cols-1 md:grid-cols-2 gap-4"
|
||||||
|
{% if not main_config.display.get('double_sided', {}).get('enabled') %}style="display: none;"{% endif %}>
|
||||||
<div class="form-group" id="setting-display-double_sided_copies" data-setting-key="display.double_sided.copies">
|
<div class="form-group" id="setting-display-double_sided_copies" data-setting-key="display.double_sided.copies">
|
||||||
<label for="double_sided_copies" class="block text-sm font-medium text-gray-700">Copies{{ ui.help_tip('How many identical screens to split the panel area into (2–8).\nMust divide the panel evenly — e.g. 2 for a two-sided cube.', 'Copies') }}</label>
|
<label for="double_sided_copies" class="block text-sm font-medium text-gray-700">Copies{{ ui.help_tip('How many identical screens to split the panel area into (2–8).\nWhen enabled, this must divide the panel evenly — e.g. 2 for a two-sided cube.', 'Copies') }}</label>
|
||||||
<input type="number"
|
<input type="number"
|
||||||
id="double_sided_copies"
|
id="double_sided_copies"
|
||||||
name="double_sided_copies"
|
name="double_sided_copies"
|
||||||
@@ -607,6 +611,22 @@ if (typeof window.fixInvalidNumberInputs !== 'function') {
|
|||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Report the outcome of a display-settings save. XMLHttpRequest has no
|
||||||
|
// `responseJSON` (that's a jQuery property) — read `responseText` and the real
|
||||||
|
// status code, otherwise a failed save reports success.
|
||||||
|
window.showDisplaySaveResult = function(xhr) {
|
||||||
|
let message = 'Display settings saved';
|
||||||
|
let status = xhr.status >= 400 ? 'error' : 'success';
|
||||||
|
try {
|
||||||
|
const data = JSON.parse(xhr.responseText);
|
||||||
|
if (data.message) message = data.message;
|
||||||
|
if (data.status) status = data.status;
|
||||||
|
} catch {
|
||||||
|
// Non-JSON body — fall back to the status-code verdict above.
|
||||||
|
}
|
||||||
|
showNotification(message, status);
|
||||||
|
};
|
||||||
|
|
||||||
// Vegas Scroll Mode Settings
|
// Vegas Scroll Mode Settings
|
||||||
(function() {
|
(function() {
|
||||||
// Escape HTML to prevent XSS
|
// Escape HTML to prevent XSS
|
||||||
@@ -631,6 +651,18 @@ if (typeof window.fixInvalidNumberInputs !== 'function') {
|
|||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Double-sided: copies/axis only mean anything while the feature is on.
|
||||||
|
// Hidden rather than disabled so the fields keep submitting and the server
|
||||||
|
// still sees an 'off' state to persist.
|
||||||
|
const doubleSidedCheckbox = document.getElementById('double_sided_enabled');
|
||||||
|
const doubleSidedSettings = document.getElementById('double_sided_settings');
|
||||||
|
|
||||||
|
if (doubleSidedCheckbox && doubleSidedSettings) {
|
||||||
|
doubleSidedCheckbox.addEventListener('change', function() {
|
||||||
|
doubleSidedSettings.style.display = this.checked ? 'grid' : 'none';
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
// Update scroll speed display
|
// Update scroll speed display
|
||||||
const scrollSpeedSlider = document.getElementById('vegas_scroll_speed');
|
const scrollSpeedSlider = document.getElementById('vegas_scroll_speed');
|
||||||
const scrollSpeedValue = document.getElementById('vegas_scroll_speed_value');
|
const scrollSpeedValue = document.getElementById('vegas_scroll_speed_value');
|
||||||
|
|||||||
Reference in New Issue
Block a user