From f7d7427d915fa7fc87772d58e29446e549c3858a Mon Sep 17 00:00:00 2001 From: flemming-it Date: Sun, 26 Jul 2026 16:05:55 +0200 Subject: [PATCH] fix(approvals): usertest-panel hardening (0.81.0) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Panel findings against the reworked approvals page, fixed in place: - Never fabricate the request time: ApprovalRecord.createdAt is nullable now; a missing created_at omits the line instead of rendering DateTime.now() (which drifted on refresh). Guard: approvals_origin_test pins the omit-on-null invariant. - Copyable errors on approve/reject/batch via showChainErrorSnack (the hard project rule) — batch surfaces the first real cause. - Reject requires a reason: ChainInlineHelp strip + confirm disabled while empty, no more silent close-and-nothing-happens. - Batch approve applies the same no-data confirmation as the single path, naming how many selected requests carry no show: data. - Plainer language: glossary "Vorgang (Flow)", history label FRAGE (was PROMPT), no-data hint drops developer jargon. - One-click copy of the run id; history payload pretty-prints like the card. Signed-off-by: flemming-it --- CHANGELOG.md | 25 ++++ lib/data/hub.dart | 14 +- lib/l10n/app_de.arb | 15 +- lib/l10n/app_en.arb | 15 +- lib/l10n/app_localizations.dart | 50 ++++++- lib/l10n/app_localizations_de.dart | 42 +++++- lib/l10n/app_localizations_en.dart | 41 +++++- lib/pages/approvals.dart | 216 +++++++++++++++++++++-------- test/approvals_origin_test.dart | 115 +++++++++------ 9 files changed, 411 insertions(+), 122 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c452fa7..600c887 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,31 @@ lockstep. local CLI falls back to it. Guard: two new `RunsLoadErrorView` widget tests. +### Fixed (0.81.0) + +Approvals usertest-panel hardening: + +- **Never fake the request time.** A missing/unparseable `created_at` + now leaves the "Angefordert" line off the card instead of showing + `now` (which even drifted on every refresh). `ApprovalRecord.createdAt` + is nullable; guard in `approvals_origin_test`. +- **Copyable errors on every path.** Approve/Reject and both batch + actions now route failures through `showChainErrorSnack` instead of + a bare `SnackBar(Text(e))` — the batch path surfaces the first real + cause, not just a count. +- **Reject needs a reason.** The reject dialog leads with a + `ChainInlineHelp` strip and disables the confirm button while the + reason is empty, instead of closing silently with nothing happening. +- **Batch approve respects the no-data guard.** A bulk approve that + includes requests without `show:` data now asks for the same + conscious confirmation the single approve already required, naming + how many are data-less. +- **Plainer language.** The intro and origin glossary "Flow" as + "Vorgang (Flow)", the history detail labels the question "FRAGE" + (not "PROMPT"), and the no-data hint drops developer jargon. +- **Copy the run id in one click**, and the history payload renders + with the same JSON pretty-printing as the card. + ### Added (0.80.0) - **Ollama joins the host services.** Ollama-backed store entries diff --git a/lib/data/hub.dart b/lib/data/hub.dart index 997a590..9f94b31 100644 --- a/lib/data/hub.dart +++ b/lib/data/hub.dart @@ -1152,7 +1152,13 @@ class HubService { stepId: e.stepId, prompt: e.prompt, payloadPreview: e.payloadPreview.isEmpty ? null : e.payloadPreview, - createdAt: DateTime.tryParse(e.createdAt) ?? DateTime.now(), + // Never fabricate the request time: a missing / unparseable + // created_at stays null so the card can omit the line + // instead of showing "now" (which would even change on + // every refresh) as if it were the truth. + createdAt: e.createdAt.isEmpty + ? null + : DateTime.tryParse(e.createdAt), expiresAt: e.expiresAt.isEmpty ? null : DateTime.tryParse(e.expiresAt), @@ -1762,7 +1768,11 @@ class ApprovalRecord { final String stepId; final String prompt; final String? payloadPreview; - final DateTime createdAt; + + /// When the approval was requested. Null when the hub sent no + /// parseable timestamp — the card omits the line rather than + /// inventing one. + final DateTime? createdAt; final DateTime? expiresAt; /// One of: pending / approved / rejected / expired. diff --git a/lib/l10n/app_de.arb b/lib/l10n/app_de.arb index 76c16d8..13bc40f 100644 --- a/lib/l10n/app_de.arb +++ b/lib/l10n/app_de.arb @@ -1095,13 +1095,20 @@ "approvalsPillRejected": "abgelehnt", "approvalsPillExpired": "abgelaufen", "approvalsPayloadPreview": "ZU PRÜFENDE DATEN", - "approvalsNoPayload": "Keine Daten zum Prüfen angehängt. Der system.approval-Schritt des Flows bestimmt über sein \"show:\"-Feld, was angezeigt wird — setzen Sie es, um die Daten hinter dieser Entscheidung sichtbar zu machen.", + "approvalsNoPayload": "Zu dieser Freigabe wurden keine Daten hinterlegt. Sie entscheiden ohne Detailansicht — fragen Sie im Zweifel die Person, die den Vorgang eingerichtet hat. (Technisch: das Feld \"show:\" des Freigabe-Schritts legt fest, was hier erscheint.)", "approvalsNoDataConfirmTitle": "Ohne Prüfdaten freigeben?", "approvalsNoDataConfirmBody": "Dieser Flow hat bewusst keine Prüfdaten hinterlegt (kein \"show:\" am Freigabe-Schritt). Sie können trotzdem freigeben — entscheiden dann aber, ohne die Daten hinter dieser Entscheidung gesehen zu haben.", "approvalsNoDataConfirmAction": "Trotzdem freigeben", "approvalsRequestFallback": "Freigabe für diesen Schritt erforderlich", "approvalsFlowStepMeta": "Flow: {flow} · Schritt: {step}", - "approvalsIntroHelp": "Hier warten pausierte Flows auf Ihre Entscheidung. Jede Karte zeigt, welcher Flow an welchem Schritt hält und welche Daten er Ihnen vorlegt — Freigeben setzt den Flow fort, Ablehnen stoppt ihn mit Ihrer Begründung.", + "approvalsIntroHelp": "Hier warten pausierte Vorgänge (Flows) auf Ihre Entscheidung. Jede Karte zeigt, welcher Vorgang an welchem Schritt hält und welche Daten er Ihnen vorlegt — Freigeben setzt ihn fort, Ablehnen stoppt ihn mit Ihrer Begründung.", + "approvalsRejectDialogHelp": "Die Ablehnung stoppt den Vorgang an diesem Schritt und wird mit Ihrer Begründung im Prüfprotokoll festgehalten.", + "approvalsRejectReasonHelper": "Pflichtfeld — wird im Prüfprotokoll (Audit-Log) festgehalten.", + "approvalsBatchNoDataTitle": "Ohne Prüfdaten freigeben?", + "approvalsBatchNoDataBody": "{count, plural, =1{Von {total} ausgewählten Freigaben hat 1 keine Prüfdaten. Sie geben sie ohne Detailansicht frei.} other{Von {total} ausgewählten Freigaben haben {count} keine Prüfdaten. Sie geben sie ohne Detailansicht frei.}}", + "approvalsCopyRun": "Lauf-Kennung kopieren", + "approvalsRunCopied": "Lauf-Kennung kopiert", + "approvalsUnknownReviewer": "(unbekannt)", "approvalsOriginLabel": "HERKUNFT", "approvalsOriginProject": "Projekt", "approvalsOriginRequested": "Angefordert", @@ -1111,7 +1118,7 @@ "approvalsApproveButton": "Freigeben", "approvalsRejectButton": "Ablehnen", "approvalsRejectDialogTitle": "Freigabe ablehnen", - "approvalsRejectReasonLabel": "Begründung (im Audit-Log festgehalten)", + "approvalsRejectReasonLabel": "Begründung", "approvalsApprovedToast": "freigegeben · {flow} › {step}", "@approvalsApprovedToast": { "placeholders": { @@ -1155,7 +1162,7 @@ "approvalsDialogDecided": "entschieden", "approvalsDialogCreated": "erstellt", "approvalsDialogReason": "Begründung", - "approvalsDialogPrompt": "PROMPT", + "approvalsDialogPrompt": "FRAGE", "approvalsExpiresIn": "{minutes}m", "@approvalsExpiresIn": { "placeholders": { diff --git a/lib/l10n/app_en.arb b/lib/l10n/app_en.arb index 53576c9..bbee0d6 100644 --- a/lib/l10n/app_en.arb +++ b/lib/l10n/app_en.arb @@ -1113,13 +1113,20 @@ "approvalsPillRejected": "rejected", "approvalsPillExpired": "expired", "approvalsPayloadPreview": "DATA TO REVIEW", - "approvalsNoPayload": "No data was attached for review. The flow's approval step chooses what to show via its \"show:\" field — set it to surface the data behind this decision.", + "approvalsNoPayload": "No data was attached to this approval. You are deciding without a detail view — when in doubt, ask whoever set up the process. (Technical: the approval step's \"show:\" field controls what appears here.)", "approvalsNoDataConfirmTitle": "Approve without review data?", "approvalsNoDataConfirmBody": "This flow deliberately attached no review data (no \"show:\" on its approval step). You can still approve — but you would be deciding without seeing the data behind this decision.", "approvalsNoDataConfirmAction": "Approve anyway", "approvalsRequestFallback": "Approval required for this step", "approvalsFlowStepMeta": "Flow: {flow} · Step: {step}", - "approvalsIntroHelp": "Paused flows waiting for your decision. Each card shows which flow is holding at which step and what data it puts in front of you — Approve resumes the flow, Reject stops it with your reason.", + "approvalsIntroHelp": "Paused processes (flows) waiting for your decision. Each card shows which process is holding at which step and what data it puts in front of you — Approve resumes it, Reject stops it with your reason.", + "approvalsRejectDialogHelp": "Rejecting stops the process at this step and is recorded with your reason in the audit trail.", + "approvalsRejectReasonHelper": "Required — recorded in the audit log.", + "approvalsBatchNoDataTitle": "Approve without review data?", + "approvalsBatchNoDataBody": "{count, plural, =1{1 of {total} selected approvals has no review data. You are approving it without a detail view.} other{{count} of {total} selected approvals have no review data. You are approving them without a detail view.}}", + "approvalsCopyRun": "Copy run id", + "approvalsRunCopied": "Run id copied", + "approvalsUnknownReviewer": "(unknown)", "approvalsOriginLabel": "ORIGIN", "approvalsOriginProject": "Project", "approvalsOriginRequested": "Requested", @@ -1135,7 +1142,7 @@ "approvalsApproveButton": "Approve", "approvalsRejectButton": "Reject", "approvalsRejectDialogTitle": "Reject approval", - "approvalsRejectReasonLabel": "Reason (recorded in audit log)", + "approvalsRejectReasonLabel": "Reason", "approvalsApprovedToast": "approved · {flow} › {step}", "@approvalsApprovedToast": { "placeholders": { @@ -1179,7 +1186,7 @@ "approvalsDialogDecided": "decided", "approvalsDialogCreated": "created", "approvalsDialogReason": "reason", - "approvalsDialogPrompt": "PROMPT", + "approvalsDialogPrompt": "QUESTION", "approvalsExpiresIn": "{minutes}m", "@approvalsExpiresIn": { "placeholders": { diff --git a/lib/l10n/app_localizations.dart b/lib/l10n/app_localizations.dart index a33e3e3..285e659 100644 --- a/lib/l10n/app_localizations.dart +++ b/lib/l10n/app_localizations.dart @@ -3521,7 +3521,7 @@ abstract class AppLocalizations { /// No description provided for @approvalsNoPayload. /// /// In en, this message translates to: - /// **'No data was attached for review. The flow\'s approval step chooses what to show via its \"show:\" field — set it to surface the data behind this decision.'** + /// **'No data was attached to this approval. You are deciding without a detail view — when in doubt, ask whoever set up the process. (Technical: the approval step\'s \"show:\" field controls what appears here.)'** String get approvalsNoPayload; /// No description provided for @approvalsNoDataConfirmTitle. @@ -3557,9 +3557,51 @@ abstract class AppLocalizations { /// No description provided for @approvalsIntroHelp. /// /// In en, this message translates to: - /// **'Paused flows waiting for your decision. Each card shows which flow is holding at which step and what data it puts in front of you — Approve resumes the flow, Reject stops it with your reason.'** + /// **'Paused processes (flows) waiting for your decision. Each card shows which process is holding at which step and what data it puts in front of you — Approve resumes it, Reject stops it with your reason.'** String get approvalsIntroHelp; + /// No description provided for @approvalsRejectDialogHelp. + /// + /// In en, this message translates to: + /// **'Rejecting stops the process at this step and is recorded with your reason in the audit trail.'** + String get approvalsRejectDialogHelp; + + /// No description provided for @approvalsRejectReasonHelper. + /// + /// In en, this message translates to: + /// **'Required — recorded in the audit log.'** + String get approvalsRejectReasonHelper; + + /// No description provided for @approvalsBatchNoDataTitle. + /// + /// In en, this message translates to: + /// **'Approve without review data?'** + String get approvalsBatchNoDataTitle; + + /// No description provided for @approvalsBatchNoDataBody. + /// + /// In en, this message translates to: + /// **'{count, plural, =1{1 of {total} selected approvals has no review data. You are approving it without a detail view.} other{{count} of {total} selected approvals have no review data. You are approving them without a detail view.}}'** + String approvalsBatchNoDataBody(num count, Object total); + + /// No description provided for @approvalsCopyRun. + /// + /// In en, this message translates to: + /// **'Copy run id'** + String get approvalsCopyRun; + + /// No description provided for @approvalsRunCopied. + /// + /// In en, this message translates to: + /// **'Run id copied'** + String get approvalsRunCopied; + + /// No description provided for @approvalsUnknownReviewer. + /// + /// In en, this message translates to: + /// **'(unknown)'** + String get approvalsUnknownReviewer; + /// No description provided for @approvalsOriginLabel. /// /// In en, this message translates to: @@ -3617,7 +3659,7 @@ abstract class AppLocalizations { /// No description provided for @approvalsRejectReasonLabel. /// /// In en, this message translates to: - /// **'Reason (recorded in audit log)'** + /// **'Reason'** String get approvalsRejectReasonLabel; /// No description provided for @approvalsApprovedToast. @@ -3677,7 +3719,7 @@ abstract class AppLocalizations { /// No description provided for @approvalsDialogPrompt. /// /// In en, this message translates to: - /// **'PROMPT'** + /// **'QUESTION'** String get approvalsDialogPrompt; /// No description provided for @approvalsExpiresIn. diff --git a/lib/l10n/app_localizations_de.dart b/lib/l10n/app_localizations_de.dart index e6427ea..a26d558 100644 --- a/lib/l10n/app_localizations_de.dart +++ b/lib/l10n/app_localizations_de.dart @@ -2037,7 +2037,7 @@ class AppLocalizationsDe extends AppLocalizations { @override String get approvalsNoPayload => - 'Keine Daten zum Prüfen angehängt. Der system.approval-Schritt des Flows bestimmt über sein \"show:\"-Feld, was angezeigt wird — setzen Sie es, um die Daten hinter dieser Entscheidung sichtbar zu machen.'; + 'Zu dieser Freigabe wurden keine Daten hinterlegt. Sie entscheiden ohne Detailansicht — fragen Sie im Zweifel die Person, die den Vorgang eingerichtet hat. (Technisch: das Feld \"show:\" des Freigabe-Schritts legt fest, was hier erscheint.)'; @override String get approvalsNoDataConfirmTitle => 'Ohne Prüfdaten freigeben?'; @@ -2060,7 +2060,40 @@ class AppLocalizationsDe extends AppLocalizations { @override String get approvalsIntroHelp => - 'Hier warten pausierte Flows auf Ihre Entscheidung. Jede Karte zeigt, welcher Flow an welchem Schritt hält und welche Daten er Ihnen vorlegt — Freigeben setzt den Flow fort, Ablehnen stoppt ihn mit Ihrer Begründung.'; + 'Hier warten pausierte Vorgänge (Flows) auf Ihre Entscheidung. Jede Karte zeigt, welcher Vorgang an welchem Schritt hält und welche Daten er Ihnen vorlegt — Freigeben setzt ihn fort, Ablehnen stoppt ihn mit Ihrer Begründung.'; + + @override + String get approvalsRejectDialogHelp => + 'Die Ablehnung stoppt den Vorgang an diesem Schritt und wird mit Ihrer Begründung im Prüfprotokoll festgehalten.'; + + @override + String get approvalsRejectReasonHelper => + 'Pflichtfeld — wird im Prüfprotokoll (Audit-Log) festgehalten.'; + + @override + String get approvalsBatchNoDataTitle => 'Ohne Prüfdaten freigeben?'; + + @override + String approvalsBatchNoDataBody(num count, Object total) { + String _temp0 = intl.Intl.pluralLogic( + count, + locale: localeName, + other: + 'Von $total ausgewählten Freigaben haben $count keine Prüfdaten. Sie geben sie ohne Detailansicht frei.', + one: + 'Von $total ausgewählten Freigaben hat 1 keine Prüfdaten. Sie geben sie ohne Detailansicht frei.', + ); + return '$_temp0'; + } + + @override + String get approvalsCopyRun => 'Lauf-Kennung kopieren'; + + @override + String get approvalsRunCopied => 'Lauf-Kennung kopiert'; + + @override + String get approvalsUnknownReviewer => '(unbekannt)'; @override String get approvalsOriginLabel => 'HERKUNFT'; @@ -2090,8 +2123,7 @@ class AppLocalizationsDe extends AppLocalizations { String get approvalsRejectDialogTitle => 'Freigabe ablehnen'; @override - String get approvalsRejectReasonLabel => - 'Begründung (im Audit-Log festgehalten)'; + String get approvalsRejectReasonLabel => 'Begründung'; @override String approvalsApprovedToast(String flow, String step) { @@ -2130,7 +2162,7 @@ class AppLocalizationsDe extends AppLocalizations { String get approvalsDialogReason => 'Begründung'; @override - String get approvalsDialogPrompt => 'PROMPT'; + String get approvalsDialogPrompt => 'FRAGE'; @override String approvalsExpiresIn(int minutes) { diff --git a/lib/l10n/app_localizations_en.dart b/lib/l10n/app_localizations_en.dart index c229ab6..bf6c5d9 100644 --- a/lib/l10n/app_localizations_en.dart +++ b/lib/l10n/app_localizations_en.dart @@ -2042,7 +2042,7 @@ class AppLocalizationsEn extends AppLocalizations { @override String get approvalsNoPayload => - 'No data was attached for review. The flow\'s approval step chooses what to show via its \"show:\" field — set it to surface the data behind this decision.'; + 'No data was attached to this approval. You are deciding without a detail view — when in doubt, ask whoever set up the process. (Technical: the approval step\'s \"show:\" field controls what appears here.)'; @override String get approvalsNoDataConfirmTitle => 'Approve without review data?'; @@ -2064,7 +2064,40 @@ class AppLocalizationsEn extends AppLocalizations { @override String get approvalsIntroHelp => - 'Paused flows waiting for your decision. Each card shows which flow is holding at which step and what data it puts in front of you — Approve resumes the flow, Reject stops it with your reason.'; + 'Paused processes (flows) waiting for your decision. Each card shows which process is holding at which step and what data it puts in front of you — Approve resumes it, Reject stops it with your reason.'; + + @override + String get approvalsRejectDialogHelp => + 'Rejecting stops the process at this step and is recorded with your reason in the audit trail.'; + + @override + String get approvalsRejectReasonHelper => + 'Required — recorded in the audit log.'; + + @override + String get approvalsBatchNoDataTitle => 'Approve without review data?'; + + @override + String approvalsBatchNoDataBody(num count, Object total) { + String _temp0 = intl.Intl.pluralLogic( + count, + locale: localeName, + other: + '$count of $total selected approvals have no review data. You are approving them without a detail view.', + one: + '1 of $total selected approvals has no review data. You are approving it without a detail view.', + ); + return '$_temp0'; + } + + @override + String get approvalsCopyRun => 'Copy run id'; + + @override + String get approvalsRunCopied => 'Run id copied'; + + @override + String get approvalsUnknownReviewer => '(unknown)'; @override String get approvalsOriginLabel => 'ORIGIN'; @@ -2094,7 +2127,7 @@ class AppLocalizationsEn extends AppLocalizations { String get approvalsRejectDialogTitle => 'Reject approval'; @override - String get approvalsRejectReasonLabel => 'Reason (recorded in audit log)'; + String get approvalsRejectReasonLabel => 'Reason'; @override String approvalsApprovedToast(String flow, String step) { @@ -2133,7 +2166,7 @@ class AppLocalizationsEn extends AppLocalizations { String get approvalsDialogReason => 'reason'; @override - String get approvalsDialogPrompt => 'PROMPT'; + String get approvalsDialogPrompt => 'QUESTION'; @override String approvalsExpiresIn(int minutes) { diff --git a/lib/pages/approvals.dart b/lib/pages/approvals.dart index f07057f..4d05883 100644 --- a/lib/pages/approvals.dart +++ b/lib/pages/approvals.dart @@ -2,7 +2,9 @@ import 'dart:convert'; import 'dart:io'; import 'package:flutter/material.dart'; +import 'package:flutter/services.dart'; +import '../data/error_presentation.dart'; import '../data/hub.dart'; import '../data/workspace.dart'; import '../l10n/app_localizations.dart'; @@ -129,7 +131,9 @@ class _ApprovalsPageState extends State { _toast(l.approvalsApprovedToast(a.flowName, a.stepId)); _refresh(); } catch (e) { - _toast(l.approvalsApproveFailed(e.toString())); + if (!mounted) return; + // Copyable error — never a bare SnackBar(Text(e)). + showChainErrorSnack(context, 'approvals.approve', e); } } @@ -142,7 +146,8 @@ class _ApprovalsPageState extends State { _toast(l.approvalsRejectedToast(a.flowName, a.stepId)); _refresh(); } catch (e) { - _toast(l.approvalsRejectFailed(e.toString())); + if (!mounted) return; + showChainErrorSnack(context, 'approvals.reject', e); } } @@ -169,25 +174,54 @@ class _ApprovalsPageState extends State { /// drains as items succeed. Future _batchApprove(List picked) async { final l = AppLocalizations.of(context)!; + // Same conscious-confirmation guard as the single approve, but for + // the whole batch: a one-click bulk approve must not silently + // sweep through the data-less requests the single path stops on. + final noData = picked.where((a) => a.payloadPreview == null).length; + if (noData > 0) { + final confirmed = await showDialog( + context: context, + builder: (ctx) => AlertDialog( + title: Text(l.approvalsBatchNoDataTitle), + content: Text(l.approvalsBatchNoDataBody(noData, picked.length)), + actions: [ + TextButton( + onPressed: () => Navigator.pop(ctx, false), + child: Text(l.buttonCancel), + ), + FilledButton( + onPressed: () => Navigator.pop(ctx, true), + child: Text(l.approvalsNoDataConfirmAction), + ), + ], + ), + ); + if (confirmed != true) return; + } + if (!mounted) return; setState(() => _batchInFlight = true); var ok = 0; var failed = 0; + Object? firstError; for (final a in picked) { try { await HubService.instance.approve(a.id, _reviewer); ok += 1; _selectedIds.remove(a.id); - } catch (_) { + } catch (e) { failed += 1; + firstError ??= e; } } if (!mounted) return; setState(() => _batchInFlight = false); - _toast( - failed == 0 - ? l.approvalsBatchApproveDoneToast(ok) - : l.approvalsBatchPartialFailure(ok, failed), - ); + if (failed == 0) { + _toast(l.approvalsBatchApproveDoneToast(ok)); + } else { + // Partial failure: surface the copyable cause, not just a count. + // The failed items stay selected so they remain visible. + showChainErrorSnack(context, 'approvals.batchApprove', firstError!); + } _refresh(); } @@ -198,22 +232,24 @@ class _ApprovalsPageState extends State { setState(() => _batchInFlight = true); var ok = 0; var failed = 0; + Object? firstError; for (final a in picked) { try { await HubService.instance.reject(a.id, _reviewer, reason); ok += 1; _selectedIds.remove(a.id); - } catch (_) { + } catch (e) { failed += 1; + firstError ??= e; } } if (!mounted) return; setState(() => _batchInFlight = false); - _toast( - failed == 0 - ? l.approvalsBatchRejectDoneToast(ok) - : l.approvalsBatchPartialFailure(ok, failed), - ); + if (failed == 0) { + _toast(l.approvalsBatchRejectDoneToast(ok)); + } else { + showChainErrorSnack(context, 'approvals.batchReject', firstError!); + } _refresh(); } @@ -229,26 +265,49 @@ class _ApprovalsPageState extends State { context: context, builder: (ctx) => AlertDialog( title: Text(l.approvalsRejectDialogTitle), - content: TextField( - controller: controller, - autofocus: true, - decoration: InputDecoration( - labelText: l.approvalsRejectReasonLabel, - border: const OutlineInputBorder(), - ), + content: Column( + mainAxisSize: MainAxisSize.min, + crossAxisAlignment: CrossAxisAlignment.stretch, + children: [ + // Explain the consequence at the point of input, and make + // the reason a real required field: the button stays + // disabled while it is empty instead of the dialog closing + // silently and nothing happening (usertest finding). + ChainInlineHelp(text: l.approvalsRejectDialogHelp), + const SizedBox(height: ChainSpace.md), + TextField( + controller: controller, + autofocus: true, + minLines: 1, + maxLines: 3, + decoration: InputDecoration( + labelText: l.approvalsRejectReasonLabel, + helperText: l.approvalsRejectReasonHelper, + border: const OutlineInputBorder(), + ), + ), + ], ), actions: [ TextButton( onPressed: () => Navigator.pop(ctx, null), child: Text(l.buttonCancel), ), - FilledButton( - onPressed: () => Navigator.pop(ctx, controller.text), - style: FilledButton.styleFrom( - backgroundColor: Theme.of(ctx).colorScheme.error, - foregroundColor: Theme.of(ctx).colorScheme.onError, - ), - child: Text(l.approvalsRejectButton), + ValueListenableBuilder( + valueListenable: controller, + builder: (ctx, value, _) { + final empty = value.text.trim().isEmpty; + return FilledButton( + onPressed: empty + ? null + : () => Navigator.pop(ctx, controller.text.trim()), + style: FilledButton.styleFrom( + backgroundColor: Theme.of(ctx).colorScheme.error, + foregroundColor: Theme.of(ctx).colorScheme.onError, + ), + child: Text(l.approvalsRejectButton), + ); + }, ), ], ), @@ -687,23 +746,58 @@ class _ApprovalCard extends StatelessWidget { style: metaStyle, ), ), - originLine( - Icons.schedule, - Text( - '${l.approvalsOriginRequested}: ' - '${_formatTimestamp(approval.createdAt.toLocal())}', - style: metaStyle, + if (approval.createdAt != null) + originLine( + Icons.schedule, + Text( + '${l.approvalsOriginRequested}: ' + '${_formatTimestamp(approval.createdAt!.toLocal())}', + style: metaStyle, + ), ), - ), if (approval.flowExecution != null) originLine( Icons.tag_outlined, - SelectableText( - '${l.approvalsOriginRun}: ${approval.flowExecution}', - style: ChainTheme.mono( - size: 11, - color: theme.colorScheme.onSurfaceVariant, - ), + Row( + children: [ + Expanded( + child: SelectableText( + '${l.approvalsOriginRun}: ${approval.flowExecution}', + style: ChainTheme.mono( + size: 11, + color: theme.colorScheme.onSurfaceVariant, + ), + ), + ), + // One-click copy so the run id can be pasted into the + // Audit page's search to reach the event timeline + // (a filtered deeplink is a larger shell change — see + // the backlog). + Tooltip( + message: l.approvalsCopyRun, + child: InkResponse( + radius: 16, + onTap: () async { + await Clipboard.setData( + ClipboardData(text: approval.flowExecution!), + ); + if (context.mounted) { + ScaffoldMessenger.of(context).showSnackBar( + SnackBar(content: Text(l.approvalsRunCopied)), + ); + } + }, + child: Padding( + padding: const EdgeInsets.all(2), + child: Icon( + Icons.content_copy, + size: 13, + color: theme.colorScheme.onSurfaceVariant, + ), + ), + ), + ), + ], ), ), const SizedBox(height: ChainSpace.md), @@ -733,7 +827,7 @@ class _ApprovalCard extends StatelessWidget { ), child: SingleChildScrollView( child: SelectableText( - _prettyPreview(approval.payloadPreview!), + _prettyJson(approval.payloadPreview!), style: ChainTheme.mono( size: 11, color: theme.colorScheme.onSurface, @@ -813,16 +907,6 @@ class _ApprovalCard extends StatelessWidget { ); } - String _prettyPreview(String raw) { - if (raw.isEmpty) return raw; - try { - final dynamic parsed = const JsonDecoder().convert(raw); - return const JsonEncoder.withIndent(' ').convert(parsed); - } catch (_) { - return raw; - } - } - String _expiresInLabel(BuildContext context, DateTime t) { final l = AppLocalizations.of(context)!; final remaining = t.difference(DateTime.now()); @@ -955,13 +1039,14 @@ class _HistoryDialog extends StatelessWidget { theme, AppLocalizations.of(context)!.approvalsDialogDecided, '${_formatTimestamp(record.decidedAt!.toLocal())} ' - '· ${record.decidedBy.isEmpty ? "(unknown)" : record.decidedBy}', + '· ${record.decidedBy.isEmpty ? AppLocalizations.of(context)!.approvalsUnknownReviewer : record.decidedBy}', + ), + if (record.createdAt != null) + _kv( + theme, + AppLocalizations.of(context)!.approvalsDialogCreated, + _formatTimestamp(record.createdAt!.toLocal()), ), - _kv( - theme, - AppLocalizations.of(context)!.approvalsDialogCreated, - _formatTimestamp(record.createdAt.toLocal()), - ), if (record.project.isNotEmpty) _kv( theme, @@ -1015,7 +1100,7 @@ class _HistoryDialog extends StatelessWidget { border: Border.all(color: theme.colorScheme.outlineVariant), ), child: SelectableText( - record.payloadPreview!, + _prettyJson(record.payloadPreview!), style: ChainTheme.mono(size: 11), ), ), @@ -1064,6 +1149,19 @@ class _HistoryDialog extends StatelessWidget { } } +/// Pretty-print a JSON payload preview; returns the raw string +/// unchanged when it is empty or not valid JSON. Shared by the +/// pending card + the history dialog so both render identically. +String _prettyJson(String raw) { + if (raw.isEmpty) return raw; + try { + final dynamic parsed = const JsonDecoder().convert(raw); + return const JsonEncoder.withIndent(' ').convert(parsed); + } catch (_) { + return raw; + } +} + /// Locale-unambiguous YYYY-MM-DD HH:mm:ss formatter shared by /// the history row + dialog. Always renders in the operator's /// local time zone — ISO timestamps from the wire still appear diff --git a/test/approvals_origin_test.dart b/test/approvals_origin_test.dart index d974b31..c38d171 100644 --- a/test/approvals_origin_test.dart +++ b/test/approvals_origin_test.dart @@ -5,6 +5,10 @@ // the Dart mapping layer; this pins that it reaches the card, next // to the rest of the origin facts and the plain-language intro. // +// It also pins the honesty invariant from the usertest: a missing +// created_at must NOT be rendered as a fabricated "now" — the line +// is simply omitted, exactly like the run id on a legacy hub. +// // Runs against the scriptable FakeHubService — never a real hub. import 'package:flutter/material.dart'; @@ -31,46 +35,57 @@ String _allText(WidgetTester tester) { return buf.toString(); } +ApprovalRecord _pending({DateTime? createdAt, String? runId = 'run-abc123'}) { + return ApprovalRecord( + id: 'apr-1', + flowName: 'classify-and-file', + stepId: 'review', + prompt: 'Bitte die Klassifikation prüfen', + payloadPreview: '{"label":"Rechnung"}', + createdAt: createdAt, + expiresAt: null, + status: 'pending', + decidedAt: null, + decidedBy: '', + reason: '', + project: 'lbs', + flowExecution: runId, + ); +} + +Future _renderApprovals( + WidgetTester tester, + List approvals, +) async { + final fake = installFakeHub(); + fake.approvals = approvals; + SharedPreferences.setMockInitialValues({}); + tester.view.physicalSize = const Size(1280, 900); + tester.view.devicePixelRatio = 1.0; + addTearDown(tester.view.reset); + await tester.pumpWidget( + const StudioApp( + initialThemeMode: ThemeModeValue.dark, + initialLocale: Locale('de'), + ), + ); + await tester.pump(const Duration(milliseconds: 100)); + await tester.tap(find.byKey(const ValueKey('sidebar-item-approvals'))); + for (var i = 0; i < 12; i++) { + await tester.pump(const Duration(milliseconds: 200)); + } + final text = _allText(tester); + await tester.pumpWidget(const SizedBox.shrink()); + await tester.pump(const Duration(minutes: 1)); + return text; +} + void main() { testWidgets('a pending approval card shows its full origin and an ' 'intro explaining approve / reject', (tester) async { - final fake = installFakeHub(); - fake.approvals = [ - ApprovalRecord( - id: 'apr-1', - flowName: 'classify-and-file', - stepId: 'review', - prompt: 'Bitte die Klassifikation prüfen', - payloadPreview: '{"label":"Rechnung"}', - createdAt: DateTime.utc(2026, 7, 26, 12, 0, 0), - expiresAt: null, - status: 'pending', - decidedAt: null, - decidedBy: '', - reason: '', - project: 'lbs', - flowExecution: 'run-abc123', - ), - ]; - - SharedPreferences.setMockInitialValues({}); - tester.view.physicalSize = const Size(1280, 900); - tester.view.devicePixelRatio = 1.0; - addTearDown(tester.view.reset); - - await tester.pumpWidget( - const StudioApp( - initialThemeMode: ThemeModeValue.dark, - initialLocale: Locale('de'), - ), - ); - await tester.pump(const Duration(milliseconds: 100)); - await tester.tap(find.byKey(const ValueKey('sidebar-item-approvals'))); - for (var i = 0; i < 12; i++) { - await tester.pump(const Duration(milliseconds: 200)); - } - - final text = _allText(tester); + final text = await _renderApprovals(tester, [ + _pending(createdAt: DateTime.utc(2026, 7, 26, 12, 0, 0)), + ]); // The human question (headline). expect(text, contains('Bitte die Klassifikation prüfen')); // The origin block — every fact a reviewer needs to place it. @@ -82,9 +97,29 @@ void main() { // The run id — the field that used to be dropped at the mapping. expect(text, contains('Lauf: run-abc123')); // The intro strip says what this inbox is and what the actions do. - expect(text, contains('pausierte Flows')); + expect(text, contains('pausierte')); + }); - await tester.pumpWidget(const SizedBox.shrink()); - await tester.pump(const Duration(minutes: 1)); + testWidgets('a missing requested-at time is omitted, never faked as ' + '"now"', (tester) async { + // created_at unknown: the honest behaviour is to drop the line, + // not to invent a timestamp (which would even drift on refresh). + final text = await _renderApprovals(tester, [_pending(createdAt: null)]); + // The rest of the origin still renders. + expect(text, contains('HERKUNFT')); + expect(text, contains('Flow: classify-and-file')); + expect(text, contains('Lauf: run-abc123')); + // But no requested-at line at all. + expect(text, isNot(contains('Angefordert:'))); + }); + + testWidgets('a legacy approval without a run id omits the run line', ( + tester, + ) async { + final text = await _renderApprovals(tester, [ + _pending(createdAt: DateTime.utc(2026, 7, 26, 12, 0, 0), runId: null), + ]); + expect(text, contains('HERKUNFT')); + expect(text, isNot(contains('Lauf:'))); }); }