From 5faf55a746a640ea45862221c50dc70a5df573bf Mon Sep 17 00:00:00 2001 From: martino <32328813+f3rs3n@users.noreply.github.com> Date: Tue, 29 Sep 2026 17:46:37 +0200 Subject: [PATCH] fix(monitor): reconcile backup outcome reports with maintainer review --- .../tests/test_command_descriptions.py | 12 ++ .../test_notification_outcome_wording.py | 124 ++++++++++++++++-- AppImage/messages/es/common.json | 2 +- AppImage/scripts/notification_events.py | 18 ++- AppImage/scripts/notification_templates.py | 29 +++- .../tests/test_notification_runtime_i18n.py | 19 ++- 6 files changed, 180 insertions(+), 24 deletions(-) diff --git a/.github/scripts/tests/test_command_descriptions.py b/.github/scripts/tests/test_command_descriptions.py index c1e3fd9d..be4ad419 100644 --- a/.github/scripts/tests/test_command_descriptions.py +++ b/.github/scripts/tests/test_command_descriptions.py @@ -130,6 +130,18 @@ class CommandDescriptionsTests(unittest.TestCase): for key in ("temperatureAlertTitle", "temperatureAlertBody", "recordedReason", "recordedDetails"): fallback.setdefault(key, source_fallback[key]) + # Slovak remains the exact upstream catalog; model the + # pending outcome-key generator additions in disposable + # copies rather than modifying its curated values. + if lang == 'sk': + local = temporary['runtime']['notifications'] + source = catalog('en')['runtime']['notifications'] + for key, value in source['backup'].items(): + local.setdefault('backup', {}).setdefault(key, value) + local['channels']['email']['severity'].setdefault( + 'observation', source['channels']['email']['severity']['observation']) + local['channels']['email']['status'].setdefault( + 'unconfirmed', source['channels']['email']['status']['unconfirmed']) path.write_text(json.dumps(temporary, ensure_ascii=False)) # Model steady state after the bot fills these intentional new # messages; keep repository locales and all other leaves intact. diff --git a/.github/scripts/tests/test_notification_outcome_wording.py b/.github/scripts/tests/test_notification_outcome_wording.py index d94b33fa..0bf932dd 100644 --- a/.github/scripts/tests/test_notification_outcome_wording.py +++ b/.github/scripts/tests/test_notification_outcome_wording.py @@ -18,7 +18,7 @@ EXPECTED = { 'error_resolved': { 'title': '{hostname}: No longer reported - {category}{entity_suffix}', 'body': 'The {category} issue is no longer in active health records.\n{reason}\n🚦 Previous severity: {original_severity}\n⏱️ Time since first observation: {duration}', - 'label': 'Health issue no longer reported', + 'label': 'Recovery notification', }, 'system_restore_completed': { @@ -71,6 +71,47 @@ class OutcomeWording(unittest.TestCase): cls.templates, render = renderer(cls.catalog) cls.render = staticmethod(render) + def test_upstream_slovak_stale_claims_use_english_report_fallback(self): + import importlib.util + spec = importlib.util.spec_from_file_location('isolated_slovak_report', SCRIPTS / 'notification_templates.py') + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + recovered = module.render_template('error_resolved', {'hostname':'node-a', + 'category':'temperature','reason':'old','duration':'3d', + 'original_severity':'WARNING'}, 'sk') + self.assertIn('No longer reported',recovered['title']) + self.assertIn('no longer in active health records',recovered['body']) + restored = module.render_template('system_restore_completed', {'hostname':'node-a', + 'guests':3,'stubs':0,'stale_nodes':0,'components':1,'duration':'2m', + 'warnings_block':'⚠️ Boot check pending'}, 'sk') + self.assertIn('Post-restore tasks completed',restored['body']) + self.assertNotIn('úplne pripravený',restored['body']) + + def test_spanish_restore_names_vms_and_containers_not_invitados(self): + import importlib.util + spec = importlib.util.spec_from_file_location('isolated_spanish_restore', SCRIPTS / 'notification_templates.py') + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + result = module.render_template('system_restore_completed', { + 'hostname':'node-a','guests':3,'stubs':0,'stale_nodes':0, + 'components':1,'duration':'2m','warnings_block':''}, 'es') + self.assertIn('máquinas virtuales y contenedores', result['body']) + self.assertNotIn('invitados', result['body'].lower()) + + def test_settings_labels_stay_at_upstream_values_in_all_locales(self): + import subprocess + for lang in ('en','de','es','fr','it','pt','sk','sv'): + path = f'AppImage/messages/{lang}/common.json' + upstream = json.loads(subprocess.check_output(['git','show',f'eb7cc548:{path}'], cwd=ROOT)) + current = json.loads((ROOT / path).read_text()) + for event in ('backup_complete', 'error_resolved'): + with self.subTest(lang=lang, event=event): + expected = upstream['runtime']['notifications']['templates'][event]['label'] + self.assertEqual(current['runtime']['notifications']['templates'][event]['label'], expected) + if lang == 'en': self.assertEqual(self.templates[event]['label'], expected) + def test_exact_four_english_leaves_match_source_and_catalog(self): for event, fields in EXPECTED.items(): for field, value in fields.items(): @@ -101,7 +142,19 @@ class OutcomeWording(unittest.TestCase): row_ok = '{:<8}{:<22}{:<10}{:<10}{:<14}{}'.format('104','alpha','OK','00:01:00','1.5 GiB','archive') row_warning = '{:<8}{:<22}{:<10}{:<10}{:<14}{}'.format('105','beta','WARNINGS','00:01:00','1.5 GiB','archive') row_error = '{:<8}{:<22}{:<10}{:<10}{:<14}{}'.format('105','beta','ERROR','00:01:00','1.5 GiB','archive') + row_err = '{:<8}{:<22}{:<10}{:<10}{:<14}{}'.format('105','beta','err','00:01:00','1.5 GiB','archive') + truncated = 'INFO: Log output was too long to be displayed. Please see task log for details.' cases = [ + ('vzdump', 'info', header+'\n'+row_ok+'\n'+row_err+'\nTotal running time: 00:02:00', 'failed'), + ('vzdump', 'info', 'INFO: Starting Backup of VM 104 (qemu)\n'+header+'\n'+row_ok+'\nTotal running time: 00:01:00\n'+truncated, 'confirmed'), + ('vzdump', 'info', header+'\n'+row_ok+'\nTotal running time: 00:01:00\n'+truncated, 'confirmed'), + ('vzdump', 'info', header+'\n'+row_err+'\nTotal running time: 00:01:00\n'+truncated, 'failed'), + ('vzdump', 'warning', header+'\n'+row_ok+'\nTotal running time: 00:01:00', 'unconfirmed'), + ('vzdump', 'info', header+'\n'+row_ok, 'unconfirmed'), + ('vzdump', 'info', 'INFO: Starting Backup of VM 104 (qemu)\nINFO: Finished Backup of VM 104 (00:01:00)\n'+header+'\n'+row_ok, 'unconfirmed'), + ('vzdump', 'warning', header+'\n'+row_err+'\nTotal running time: 00:01:00', 'failed'), + ('vzdump', 'info', header+'\n'+row_ok+'\n'+row_warning+'\nTotal running time: 00:02:00\n'+truncated, 'unconfirmed'), + ('vzdump', 'info', header+'\n'+row_ok+'\nTotal running time: 00:01:00\nERROR: archive write failed', 'failed'), ('vzdump', 'info', header+'\n'+row_ok+'\n'+row_error+'\nTotal running time: 00:02:00', 'failed'), ('vzdump', 'info', 'INFO: Starting Backup of VM 104 (qemu)\nINFO: Finished Backup of VM 104 (00:01:00)\n'+header+'\n'+row_error+'\nTotal running time: 00:02:00', 'failed'), ('vzdump', 'info', header+'\n'+row_error, 'failed'), @@ -175,6 +228,11 @@ class OutcomeWording(unittest.TestCase): parser = extract(SCRIPTS / 'notification_templates.py', '_parse_vzdump_message', namespace=ns) formatter = extract(SCRIPTS / 'notification_templates.py', '_format_vzdump_body', namespace=ns) incomplete = parser('INFO: Starting Backup of VM 104 (qemu)') + table_header = '{:<8}{:<22}{:<10}{:<10}{:<14}{}'.format('VMID','Name','Status','Time','Size','Filename') + table_err = '{:<8}{:<22}{:<10}{:<10}{:<14}{}'.format('104','alpha','err','00:01:00','1.5 GiB','archive') + failed_table = parser(table_header+'\n'+table_err+'\nTotal running time: 00:01:00') + self.assertEqual(failed_table['vms'][0]['status'].lower(), 'error') + self.assertIn('❌', formatter(failed_table, False, 'en')) self.assertEqual(incomplete['vms'][0]['status'], 'unknown') self.assertNotIn('✅', formatter(incomplete, False, 'en')) mixed = parser('INFO: Starting Backup of VM 104 (qemu)\nINFO: Finished Backup of VM 104 (00:01:00)\nINFO: Starting Backup of VM 105 (lxc)') @@ -205,16 +263,45 @@ class OutcomeWording(unittest.TestCase): self.assertIn('ERROR: archive write failed', conflict['body']) self.assertNotIn('Backup complete', conflict['title']) + def test_confirmed_title_keeps_single_guest_and_destination_without_misnaming_batches(self): + import importlib.util + spec = importlib.util.spec_from_file_location('isolated_backup_title', SCRIPTS / 'notification_templates.py') + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + actual_render = module.render_template + log = ('INFO: starting new backup job: vzdump 104 --storage PBS-Cloud --mode snapshot\n' + 'INFO: Starting Backup of VM 104 (qemu)\nINFO: VM Name: Alpha\n' + 'INFO: Finished Backup of VM 104 (00:01:00)') + single = actual_render('backup_complete', {'hostname':'node-a','backup_outcome':'confirmed', + 'pve_message':log}, 'en') + self.assertIn('PBS-Cloud',single['title']) + self.assertIn('VM Alpha (104)',single['title']) + batch = actual_render('backup_complete', {'hostname':'node-a','backup_outcome':'confirmed', + 'pve_message':log+'\nINFO: Starting Backup of VM 105 (lxc)\n' + 'INFO: Finished Backup of VM 105 (00:01:00)'}, 'en') + self.assertIn('PBS-Cloud',batch['title']) + self.assertNotIn('Alpha (104)',batch['title']) + no_context = actual_render('backup_complete', {'hostname':'node-a','backup_outcome':'confirmed'}, 'en') + self.assertEqual(no_context['title'], 'node-a: Backup complete') + named = actual_render('backup_complete', {'hostname':'node-a','backup_outcome':'confirmed', + 'pve_message':log.replace('Alpha','Alpha {literal}')}, 'en') + self.assertIn('Alpha {literal} (104)', named['title']) + def test_html_email_badge_and_backup_status_are_context_specific(self): import html path = SCRIPTS / 'notification_channels.py' for lang in ('en', 'de', 'es', 'fr', 'it', 'pt', 'sk', 'sv'): with self.subTest(lang=lang): catalog = json.loads((ROOT / 'AppImage/messages' / lang / 'common.json').read_text())['runtime']['notifications'] + english = self.catalog['runtime']['notifications'] def text(key, data=None, **values): - value = catalog['channels'] - for part in key.split('.'): value = value[part] - return value.format(**values) + def lookup(source): + value = source['channels'] + for part in key.split('.'): + value = value.get(part) if isinstance(value, dict) else None + return value + return (lookup(catalog) or lookup(english) or '').format(**values) ns = {'Dict': dict, 'Optional': __import__('typing').Optional, '_runtime_text': text, '_runtime_notification_text': lambda key, data=None: ''} build = extract(path, '_build_detail_rows', 'EmailChannel', ns) @@ -226,7 +313,7 @@ class OutcomeWording(unittest.TestCase): _SEV_DEFAULT = {'color':'#6b7280','bg':'#f9fafb','border':'#e5e7eb'} subject_prefix = 'ProxMenux' _build_detail_rows = staticmethod(build) - badge = catalog['channels']['email']['severity']['observation'] + badge = catalog['channels']['email']['severity'].get('observation') or english['channels']['email']['severity']['observation'] recovery = fmt(Email(), 'No longer reported', 'Body', 'OK', {'_event_type': 'error_resolved', '_notification_language': lang, '_group': 'health'}) self.assertIn('>' + badge.upper() + '', recovery) @@ -237,8 +324,9 @@ class OutcomeWording(unittest.TestCase): for outcome, status in [('confirmed', 'completed'), ('unconfirmed','unconfirmed'), ('failed','failed')]: email = fmt(Email(), 'Backup', 'Details', 'INFO', {'_event_type': 'backup_complete', 'backup_outcome': outcome, '_notification_language': lang, '_group': 'backup'}) - self.assertIn(catalog['channels']['email']['status'][status], html.unescape(email)) - badge_label = catalog['channels']['email']['status'][status].upper() + label = catalog['channels']['email']['status'].get(status) or english['channels']['email']['status'][status] + self.assertIn(label, html.unescape(email)) + badge_label = label.upper() self.assertIn('>' + badge_label + '', html.unescape(email)) if outcome == 'failed': self.assertIn('color:#dc2626;font-weight:600;', email) @@ -269,16 +357,28 @@ class OutcomeWording(unittest.TestCase): result = module.render_template('backup_complete', data, lang) if state != 'unconfirmed': key = 'confirmedTitle' if state == 'confirmed' else 'errorTitle' - self.assertEqual(result['title'], catalog['backup'][key].format(hostname=data['hostname'])) - else: self.assertEqual(result['title'], catalog['templates']['backup_complete']['title'].format(hostname=data['hostname'])) + expected_title = (catalog.get('backup', {}).get(key) or + self.catalog['runtime']['notifications']['backup'][key]).format(hostname=data['hostname']) + self.assertTrue(result['title'].startswith(expected_title), result['title']) + else: + source = (catalog if catalog.get('backup', {}).get('unconfirmedBody') + else self.catalog['runtime']['notifications']) + self.assertEqual(result['title'], source['templates']['backup_complete']['title'].format(hostname=data['hostname'])) self.assertNotIn('{hostname}', result['title']) - if state == 'unconfirmed': self.assertIn(catalog['backup']['unconfirmedBody'], result['body']) - if state == 'failed': self.assertIn(catalog['backup']['errorBody'], result['body']) + if state == 'unconfirmed': + source = (catalog if catalog.get('backup', {}).get('unconfirmedBody') + else self.catalog['runtime']['notifications']) + self.assertIn(source['backup']['unconfirmedBody'], result['body']) + if state == 'failed': + self.assertIn(catalog.get('backup', {}).get('errorBody') or + self.catalog['runtime']['notifications']['backup']['errorBody'], result['body']) enriched, _ = module.enrich_with_emojis('backup_complete', result['title'], result['body'], data) self.assertTrue(enriched.startswith({'confirmed':'💾✅','unconfirmed':'💾❔','failed':'💾❌'}[state])) recovery = module.render_template('error_resolved', {'hostname':'node','category':'temperature', 'reason':'Old observation','duration':'3d','original_severity':'WARNING'}, lang) - self.assertEqual(recovery['title'],catalog['templates']['error_resolved']['title'].format(hostname='node',category='temperature',entity_suffix='')) + recovery_source = (catalog if catalog.get('backup', {}).get('unconfirmedBody') + else self.catalog['runtime']['notifications']) + self.assertEqual(recovery['title'], recovery_source['templates']['error_resolved']['title'].format(hostname='node',category='temperature',entity_suffix='')) self.assertNotIn('resolved', recovery['title'].lower()) if lang == 'en' else None restore = module.render_template('system_restore_completed', {'hostname':'node', 'guests':4, 'stubs':1,'stale_nodes':2,'components':1,'duration':'2m','warnings_block':'Missing module'},lang) diff --git a/AppImage/messages/es/common.json b/AppImage/messages/es/common.json index 86786662..43cc5f24 100644 --- a/AppImage/messages/es/common.json +++ b/AppImage/messages/es/common.json @@ -6557,7 +6557,7 @@ }, "system_restore_completed": { "title": "{hostname}: restauración del host finalizada", - "body": "Tareas posteriores a la restauración completadas en segundo plano.\n\nConfiguraciones de invitados aplicadas: {guests}\nDirectorios auxiliares de montajes bind: {stubs}\nDirectorios de nodos obsoletos eliminados: {stale_nodes}\nComponentes reinstalados: {components}\nDuración: {duration}\n{warnings_block}", + "body": "Tareas posteriores a la restauración completadas en segundo plano.\n\nConfiguraciones de máquinas virtuales y contenedores aplicadas: {guests}\nDirectorios auxiliares de montajes bind: {stubs}\nDirectorios de nodos obsoletos eliminados: {stale_nodes}\nComponentes reinstalados: {components}\nDuración: {duration}\n{warnings_block}", "label": "Restauración del host completada" }, "system_problem": { diff --git a/AppImage/scripts/notification_events.py b/AppImage/scripts/notification_events.py index c3eded8d..9e949f60 100644 --- a/AppImage/scripts/notification_events.py +++ b/AppImage/scripts/notification_events.py @@ -4296,9 +4296,6 @@ class ProxmoxHookWatcher: if severity in ('error', 'err', 'critical') or re.search( r'(?im)^\s*(?:ERROR:|TASK ERROR:|.*\bStatus\s+ERROR\b)', text): return 'failed' - if severity not in ('info', 'ok', 'success') or re.search( - r'(?im)(?:^\s*WARNING:|\bWARNINGS\s*:\s*\d+)', text): - return 'unconfirmed' starts = re.findall(r'(?im)\bStarting Backup of VM (\d+)\s*\(', text) finished = re.findall(r'(?im)\bFinished Backup of VM (\d+)\s*\(', text) lines = text.splitlines() @@ -4321,15 +4318,24 @@ class ProxmoxHookWatcher: if not re.match(r'\s*\d+\s+', line): break status = line[status_start:status_end].strip().upper() - if status == 'ERROR': + if status in ('ERROR', 'ERR'): return 'failed' rows.append(status) break - if table_outcome == 'unconfirmed': + if severity not in ('info', 'ok', 'success') or re.search( + r'(?im)(?:^\s*WARNING:|\bWARNINGS\s*:\s*\d+)', text): + return 'unconfirmed' + # A present table is authoritative: do not certify an incomplete table + # from a finished guest log, or reject a complete OK table merely + # because the extra diagnostic log was truncated before its finishes. + if table_outcome is not None: + return table_outcome + if any(re.match(r'\s*VMID\s+Name\s+Status\b', line, re.IGNORECASE) + for line in lines): return 'unconfirmed' if starts: return 'confirmed' if sorted(starts) == sorted(finished) else 'unconfirmed' - if table_outcome == 'confirmed' or re.search( + if re.search( r'(?im)^\s*(?:INFO:\s*)?TASK OK\s*$', text): return 'confirmed' return 'unconfirmed' diff --git a/AppImage/scripts/notification_templates.py b/AppImage/scripts/notification_templates.py index e26018af..5ea8d925 100644 --- a/AppImage/scripts/notification_templates.py +++ b/AppImage/scripts/notification_templates.py @@ -252,6 +252,8 @@ def _parse_vzdump_message(message: str) -> Optional[Dict[str, Any]]: vmid = padded[col_starts[0]:col_starts[1]].strip() name = padded[col_starts[1]:col_starts[2]].strip() status = padded[col_starts[2]:col_starts[3]].strip() + if status.lower() in ('err', 'error'): + status = 'error' time_val = padded[col_starts[3]:col_starts[4]].strip() size = padded[col_starts[4]:col_starts[5]].strip() filename = padded[col_starts[5]:].strip() @@ -785,7 +787,7 @@ TEMPLATES = { # without a trailing dash. 'title': '{hostname}: No longer reported - {category}{entity_suffix}', 'body': 'The {category} issue is no longer in active health records.\n{reason}\n\U0001F6A6 Previous severity: {original_severity}\n\u23F1\uFE0F Time since first observation: {duration}', - 'label': 'Health issue no longer reported', + 'label': 'Recovery notification', 'group': 'health', 'default_enabled': True, }, @@ -1003,7 +1005,7 @@ TEMPLATES = { 'backup_complete': { 'title': '{hostname}: Backup outcome unconfirmed', 'body': 'The backup outcome could not be confirmed from this notice.', - 'label': 'Backup report', + 'label': 'Backup complete', 'group': 'backup', 'default_enabled': True, }, @@ -1854,13 +1856,35 @@ def render_template(event_type: str, data: Dict[str, Any], _catalog_value(requested_catalog, key) or _catalog_value(english_catalog, key) ) + # A catalog without the outcome keys predates this report contract. + # Keep its Settings labels, but do not render old recovery, restore + # or backup success claims (e.g. the exact upstream Slovak catalog). + if (event_type in ('backup_complete', 'error_resolved', 'system_restore_completed') + and field in ('title', 'body') + and not _catalog_value(requested_catalog, 'backup.unconfirmedBody')): + localized = _catalog_value(english_catalog, key) if localized: template[field] = localized + backup_title_target = '' if event_type == 'backup_complete': outcome = data.get('backup_outcome') if outcome == 'confirmed': template['title'] = runtime_message('backup.confirmedTitle', language, hostname=data.get('hostname') or _get_hostname()) + parsed_backup = _parse_vzdump_message(str(data.get('pve_message') or '')) + storage = str((parsed_backup or {}).get('storage_name') or data.get('storage') or '').strip() + guests = (parsed_backup or {}).get('vms') or [] + target = [] + if storage: + target.append(storage) + if len(guests) == 1: + guest = guests[0] + kind = 'VM' if guest.get('type') == 'qemu' else 'CT' if guest.get('type') == 'lxc' else 'VM/CT' + name = guest.get('name') or kind + target.append(f"{kind} {name} ({guest['vmid']})" if name != kind + else f"{kind} {guest['vmid']}") + if target: + backup_title_target = ' — ' + ' · '.join(target) template['body'] = runtime_message('backup.confirmedBody', language) elif outcome == 'failed': template['title'] = runtime_message('backup.errorTitle', language, @@ -2003,6 +2027,7 @@ def render_template(event_type: str, data: Dict[str, Any], title = template['title'].format_map(safe_vars) except (ValueError, IndexError): title = template['title'] + title += backup_title_target # ── PVE vzdump special formatting ── # When the event came from PVE webhook with a full vzdump message, diff --git a/AppImage/scripts/tests/test_notification_runtime_i18n.py b/AppImage/scripts/tests/test_notification_runtime_i18n.py index f2f79843..abd1b127 100644 --- a/AppImage/scripts/tests/test_notification_runtime_i18n.py +++ b/AppImage/scripts/tests/test_notification_runtime_i18n.py @@ -50,6 +50,10 @@ class RuntimeCatalogTests(unittest.TestCase): self.assertIsInstance(templates[event_type][field], str) self.assertTrue(templates[event_type][field]) if field in source: + # Upstream Slovak backup title/body still belong to + # the pre-outcome schema; runtime falls back to EN. + if language == 'sk' and event_type == 'backup_complete' and field != 'label': + continue self.assertEqual( _placeholders(templates[event_type][field]), _placeholders(source[field]), @@ -68,10 +72,17 @@ class RuntimeCatalogTests(unittest.TestCase): return result en = flatten(self.catalogs["en"]) + pending_slovak = {"backup.confirmedTitle", "backup.confirmedBody", + "backup.errorTitle", "backup.errorBody", "backup.unconfirmedBody", + "channels.email.severity.observation", "channels.email.status.unconfirmed"} for language, catalog in self.catalogs.items(): translated = flatten(catalog) - self.assertEqual(set(translated), set(en), language) - for key in en: + expected = set(en) - pending_slovak if language == 'sk' else set(en) + self.assertEqual(set(translated), expected, language) + for key in expected: + if language == 'sk' and key in ('templates.backup_complete.title', + 'templates.backup_complete.body'): + continue # exact upstream SK, superseded only at render time self.assertEqual(_placeholders(translated[key]), _placeholders(en[key]), f"{language}:{key}") def test_notification_language_ui_keys_exist_in_both_catalogs(self): @@ -323,7 +334,9 @@ class RuntimeCatalogTests(unittest.TestCase): }, language="sk", ) - self.assertIn("záloha dokončená", backup["title"]) + self.assertIn("Backup complete", backup["title"]) + self.assertIn("pbs-main", backup["title"]) + self.assertIn("VM alpha (100)", backup["title"]) self.assertNotIn("Backup job finished", backup["title"]) self.assertIn("Veľkosť: 1.5 GiB", backup["body"]) self.assertIn("Trvanie: 00:00:10", backup["body"])