diff --git a/test/test_update_prompts_restart.py b/test/test_update_prompts_restart.py new file mode 100644 index 00000000..d7871e85 --- /dev/null +++ b/test/test_update_prompts_restart.py @@ -0,0 +1,84 @@ +"""A pull that changed nothing on the running system is not an applied update. + +git_pull replaces files on disk and restarts nothing -- there is no systemctl +call anywhere in the handler. The display and web services keep running the +code they loaded at boot, so the user is told "Code updated successfully" and +sees no change until they happen to reboot. The response now says whether a +restart is owed, and the UI raises the existing restart-pending banner. +""" +import subprocess +import sys +from pathlib import Path +from unittest.mock import patch + +import pytest +from flask import Flask + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from web_interface.blueprints import api_v3 as mod # noqa: E402 +from web_interface.blueprints.api_v3 import api_v3 # noqa: E402 + + +@pytest.fixture +def client(): + app = Flask(__name__) + app.config['TESTING'] = True + app.register_blueprint(api_v3, url_prefix='/api/v3') + # The handler consults these after a successful pull; None is the + # "not wired up" case it already guards for. + api_v3.plugin_store_manager = None + api_v3.config_manager = None + return app.test_client() + + +def _git(heads, pull_rc=0, pull_out='Updating a1b2c3..d4e5f6\n'): + """Fake git. `heads` are the successive answers to rev-parse HEAD.""" + seq = list(heads) + + def run(args, **kwargs): + def ok(stdout='', rc=0, b=False): + return subprocess.CompletedProcess( + args, rc, stdout=(stdout.encode() if b else stdout), + stderr=(b'' if b else '')) + if args[:2] == ['git', 'rev-parse'] and args[-1] == 'HEAD': + return ok(seq.pop(0) + '\n' if seq else 'deadbeef\n') + if 'symbolic-full-name' in args or '@{u}' in args: + return ok('origin/main\n') + if args[:2] == ['git', 'status']: + return ok('') + if args[:2] == ['git', 'diff']: + return ok('') + if args[:2] == ['git', 'pull']: + return ok(pull_out, pull_rc) + return ok('') + return run + + +def _pull(client): + return client.post('/api/v3/system/action', + json={'action': 'git_pull'}).get_json() + + +class TestRestartIsRequestedWhenCodeChanged: + def test_a_pull_that_moved_head_asks_for_a_restart(self, client): + with patch.object(mod.subprocess, 'run', _git(['aaa111', 'bbb222'])): + data = _pull(client) + assert data['status'] == 'success' + assert data['restart_required'] is True, ( + "new code on disk, services still running the old code, and " + "nothing told the user to restart") + + def test_already_up_to_date_does_not(self, client): + with patch.object(mod.subprocess, 'run', + _git(['aaa111', 'aaa111'], pull_out='Already up to date.\n')): + data = _pull(client) + assert data['status'] == 'success' + assert data['restart_required'] is False, ( + "prompting after a no-op update trains users to ignore the prompt") + + def test_a_failed_pull_does_not(self, client): + with patch.object(mod.subprocess, 'run', _git(['aaa111'], pull_rc=1)): + data = _pull(client) + assert data['status'] == 'error' + assert data['restart_required'] is False diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index 984637e8..83c8aa75 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -1996,6 +1996,11 @@ def execute_system_action(): except subprocess.TimeoutExpired: logger.warning("git rev-parse timed out before pull") + # Whether the pull actually brought new code in. "Already up to + # date" is a success too, and prompting for a restart then would + # train users to ignore the prompt. + code_changed = False + # Perform the git pull. Branches without an upstream were given # an explicit "origin " above so the update still works. result = subprocess.run( @@ -2039,6 +2044,7 @@ def execute_system_action(): capture_output=True, text=True, timeout=10, cwd=project_dir) new_head = _post.stdout.strip() if _post.returncode == 0 else None if old_head and new_head and old_head != new_head: + code_changed = True diff = subprocess.run( ['git', 'diff', '--name-only', f'{old_head}..{new_head}'], capture_output=True, text=True, timeout=15, cwd=project_dir) @@ -2098,9 +2104,14 @@ def execute_system_action(): if ln.strip()), '') pull_message = f"Update failed: {detail}" if detail else "Update failed; check logs for details" + # Nothing here restarts anything: the pull replaces files on + # disk while the display and web services keep running the code + # they loaded at boot. Without this the user is told the update + # succeeded and sees no change until they happen to reboot. return jsonify({ 'status': 'success' if result.returncode == 0 else 'error', 'message': pull_message, + 'restart_required': bool(result.returncode == 0 and code_changed), }) elif action == 'checkout_branch': # Switch branches from the Tools tab. Needed because a checkout diff --git a/web_interface/static/v3/app.js b/web_interface/static/v3/app.js index 82b2a9e5..581c66f2 100644 --- a/web_interface/static/v3/app.js +++ b/web_interface/static/v3/app.js @@ -116,14 +116,25 @@ document.body.addEventListener('htmx:afterRequest', function(event) { // ===== Restart-pending banner ===== // Shown after restart-requiring saves; persists across tab switches (and // reloads, via sessionStorage) until the display restarts or it's dismissed. -window.showRestartPending = function() { - try { sessionStorage.setItem('ledmatrix-restart-pending', '1'); } catch { /* private browsing */ } +window.showRestartPending = function(message) { + try { + sessionStorage.setItem('ledmatrix-restart-pending', '1'); + // Persisted alongside the flag: a code update and a config save want + // different wording, and the banner outlives the page that raised it. + if (message) sessionStorage.setItem('ledmatrix-restart-pending-text', message); + else sessionStorage.removeItem('ledmatrix-restart-pending-text'); + } catch { /* private browsing */ } const banner = document.getElementById('restart-pending-banner'); + const text = document.getElementById('restart-pending-text'); + if (text && message) text.textContent = message; if (banner) banner.style.display = 'block'; }; window.dismissRestartPending = function() { - try { sessionStorage.removeItem('ledmatrix-restart-pending'); } catch { /* no-op */ } + try { + sessionStorage.removeItem('ledmatrix-restart-pending'); + sessionStorage.removeItem('ledmatrix-restart-pending-text'); + } catch { /* no-op */ } const banner = document.getElementById('restart-pending-banner'); if (banner) banner.style.display = 'none'; }; @@ -151,6 +162,9 @@ document.addEventListener('DOMContentLoaded', function() { try { if (sessionStorage.getItem('ledmatrix-restart-pending') === '1') { const banner = document.getElementById('restart-pending-banner'); + const saved = sessionStorage.getItem('ledmatrix-restart-pending-text'); + const text = document.getElementById('restart-pending-text'); + if (text && saved) text.textContent = saved; if (banner) banner.style.display = 'block'; } } catch { /* no-op */ } diff --git a/web_interface/templates/v3/base.html b/web_interface/templates/v3/base.html index fb4cfa6a..f3e0b9ad 100644 --- a/web_interface/templates/v3/base.html +++ b/web_interface/templates/v3/base.html @@ -413,7 +413,8 @@

- + Configuration saved — restart the display to apply the changes
@@ -1146,6 +1147,13 @@

Run Plugin On-Deman if (data.status === 'success') { document.getElementById('update-banner').style.display = 'none'; try { sessionStorage.removeItem('update-sha-dismissed'); } catch(e) {} + // The pull replaced files on disk; the running services still + // hold the code they loaded at boot. Ask for the restart that + // makes the update actually take effect. + if (data.restart_required && typeof window.showRestartPending === 'function') { + window.showRestartPending( + 'Update installed \u2014 restart the display to run the new code'); + } } if (typeof showNotification === 'function') { showNotification(data.message || 'Update complete', data.status || 'success');