From 49c1e75ced43fe5035f5ed1027b38ff7bbdf4eec Mon Sep 17 00:00:00 2001 From: EthanHealy01 <80844253+EthanHealy01@users.noreply.github.com> Date: Mon, 24 Aug 2026 22:29:41 +0000 Subject: [PATCH] Surface recorded failures in a notification bell (Review Flow PR 4) (#7478) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review Flow PR 4. Stacked on #7477. Recorded failures appear in a notification bell, showing each reader the failures they are allowed to see and the actions they can actually take. Scope is deliberately viewing and routing only. Resolving a failure — retry, decrypt-and-retry — is #7479, which also brings the write path for it; nothing resolution-shaped ships here, not even dark. ## What's added **A notification bell** in the editor and the processor shell. Polls `GET /api/v1/notifications` every 30 seconds, shows an unread badge, and lists open failures newest first. Each row shows the failure's title, its message with **Copy error** and **Show full message** chips, an occurrence count, and its available actions. **A notification API** (`stirling.software.proprietary.notification`), derived from failures on read rather than stored in its own table: | Route | Purpose | |---|---| | `GET /api/v1/notifications` | the caller's open failures, newest first | Read-only by design: every action the bell offers is one the client runs on its own device, so there is nothing to post back. Every id is prefixed (`failure:`), so the bell never holds a raw failure id it could hand to a failure endpoint. **Per-reader actions.** A `FailureKind` declares each action with an audience (`OWNER`, `TEAM_REVIEWER`, `ANYONE_WHO_SEES`). The server resolves that against the reader and derives `Ownership` (`MINE` / `THEIRS` / `UNOWNED`) from the row's actor, so an admin reviewing someone else's failure is not offered a document their browser does not hold. Adding a failure kind requires no frontend change. **Server-run and client-run actions are distinguished.** `FailureActionId` carries an `Execution` facet; the registry requires a bean only for server actions, and dispatching a client action on the failure surface returns 400. The notification projection goes further: it carries only client-run offers, so the bell cannot be sent a button it would refuse to draw. **Actions in the bell:** at most two. The owner of the document gets **View file** (opens it in the editor); a team reviewer gets **View in processor** (dev builds only). Dismiss stays on the failure queue in `/processor/documents` — deciding a failure's fate belongs to the review surface, not the panel that announces it. An action id the build has not wired is skipped rather than rendered dead, so the server can ship new kinds ahead of the clients that understand them. **Attended policy runs record their document.** `POST /api/v1/policies/{id}/run` accepts an optional opaque `fileId`, recorded when the run carries exactly one primary document. This is what lets a repeat fold onto one incident instead of opening a new one per upload, lets deleting the file clear its failure, and lets the owner open the document from the row. ## Behaviour changes - **The bell re-reads as soon as a failure you caused is recorded**, rather than leaving you to wait out a poll interval for news of your own upload. Applies to a failed tool run and to a policy run reaching `FAILED`. Other people's failures still arrive on the poll, which is what it is for. - **An action the reader cannot use is not rendered.** Where the server gave a reason for withholding it, that reason appears as the row's one-line note. An action that was never offered to that reader produces no note. - **Deleting a document closes every incident about it that the deleter caused**, including a failed policy run on their own upload, so a user's own errors leave the bell with the file rather than lingering with a dead button. - **The failures list in `/processor/documents` stays behind `import.meta.env.DEV`**, and View in processor is gated to match so it cannot navigate to a section that is not mounted. Both lift when failures get their own review screen. - **One poll for all bells.** The bell is mounted in three places; the list, document lookups and read marker are shared, so mounting more than one does not multiply requests. - `ACKNOWLEDGE` is no longer offered by any kind. The id, bean and status remain so existing rows stay readable. ## Known limits - The poll does not pause when the tab is hidden. - No retention or per-team cap on `file_run_events`. ## How to test Needs a proprietary or SaaS build with login enabled. `task dev:all`, then sign in. 1. **Create a failure.** Add a password-protected PDF to the editor and choose **Skip for now** when it asks to unlock. The upload starts a policy run that fails on it. 2. **Watch the bell.** The badge should appear within a second or two, not after 30 — this is the refresh-on-failure path. Open it: a row titled "Password-protected document" with the error message and the two chips. 3. **The buttons should be View file and View in processor, nothing else.** No Dismiss and no retries: dispositions live on the review surface, resolutions in #7479. 4. **View file** closes the panel and selects that document in the editor. 5. **Dismiss from the queue instead.** Open `/processor/documents` (dev build), find the row in the failures list and dismiss it there; the bell drops it on its next read. 6. **Confirm the local-document probe.** Create a second failure, then delete that file from the editor and reload. Its incident closes with it; a row whose document is still present keeps **View file**. 7. **Confirm attribution end to end.** Sign in as a plain member, run a shared policy on your own upload so it fails. The member sees their own row in the bell. Sign in as the team leader: they see it too, but with **View in processor** instead of **View file**, because the document is not in their browser. 8. **Confirm folding.** Add the same locked PDF again and skip again. The existing row's occurrence count increases rather than a second row appearing. 9. **Confirm one poll for many bells.** Open the editor and the processor in two tabs. Each tab issues its own poll, but within a tab the several mounted bells share one — the Network tab should show one `GET /api/v1/notifications` per 30s per tab, not three. ## Migration None. No new column and no new value in any CHECK-constrained enum; `CheckConstrainedEnumsTest` fails if that changes. --- .../failure/FailureActionException.java | 37 +- .../proprietary/failure/FailureActionId.java | 51 +- .../failure/FailureActionRegistry.java | 17 +- .../proprietary/failure/FailureAudience.java | 15 + .../proprietary/failure/FailureKind.java | 64 ++- .../failure/FileRunEventController.java | 28 +- .../failure/FileRunEventRepository.java | 14 +- .../failure/FileRunEventService.java | 134 +++-- .../proprietary/failure/FileRunEventView.java | 16 +- .../proprietary/failure/Ownership.java | 17 + .../notification/NotificationController.java | 48 ++ .../notification/NotificationService.java | 53 ++ .../notification/NotificationSource.java | 21 + .../notification/NotificationView.java | 33 ++ .../policy/controller/PolicyController.java | 26 +- .../policy/controller/PolicyRunFiles.java | 16 +- .../policy/engine/PolicyEngine.java | 3 +- .../policy/engine/PolicyRunner.java | 11 +- .../failure/CheckConstrainedEnumsTest.java | 57 ++ .../proprietary/failure/FailureKindTest.java | 107 +++- .../failure/FileRunEventControllerTest.java | 71 ++- .../FileRunEventHttpIntegrationTest.java | 56 +- .../failure/FileRunEventServiceTest.java | 324 +++++++++-- .../failure/FileRunEventStoreDbTest.java | 38 +- .../InMemoryFileRunEventRepository.java | 3 +- .../failure/NotificationProjectionTest.java | 168 ++++++ .../failure/PolicyFailureOwnershipTest.java | 272 +++++++++ .../failure/PolicyFailureRecorderTest.java | 32 ++ .../failure/RecordFailurePrivacyTest.java | 12 + .../controller/PolicyControllerTest.java | 73 ++- .../policy/engine/PolicyRunnerTest.java | 53 +- .../public/locales/en-US/translation.toml | 28 + .../src/core/components/layout/Workbench.tsx | 10 + .../notifications/NotificationBell.css | 170 ++++++ .../notifications/NotificationBell.test.tsx | 530 ++++++++++++++++++ .../notifications/NotificationBell.tsx | 172 ++++++ .../notifications/NotificationItem.tsx | 251 +++++++++ .../notifications/notificationActions.ts | 40 ++ .../useNotificationsAvailable.ts | 11 + .../shared/DividerWithText.stories.tsx | 0 .../components/shared/DividerWithText.tsx | 0 .../core/components/shared/WorkbenchBar.tsx | 4 + .../dividerWithText/DividerWithText.css | 0 .../editor/src/core/contexts/FileContext.tsx | 8 +- .../src/core/contexts/NavigationContext.tsx | 6 +- .../file/removeFiles.reporting.test.tsx | 87 +++ .../hooks/tools/shared/useToolOperation.ts | 4 +- .../src/core/hooks/useNotifications.test.ts | 249 ++++++++ .../editor/src/core/hooks/useNotifications.ts | 240 ++++++++ .../editor/src/core/i18n/translationAudit.ts | 3 + .../editor/src/core/routes/portalBasename.ts | 6 + .../core/services/localFilePresence.test.ts | 36 ++ .../src/core/services/localFilePresence.ts | 18 + .../editor/src/core/services/notifications.ts | 66 +++ .../src/core/tests/helpers/api-stubs.ts | 6 + frontend/editor/src/core/theme/colors.css | 3 + frontend/editor/src/core/types/fileContext.ts | 5 + frontend/editor/src/core/ui/BellIcon.tsx | 22 + frontend/editor/src/core/ui/index.ts | 1 + .../editor/src/portal/components/AppShell.css | 11 + .../editor/src/portal/components/AppShell.tsx | 4 + .../failures/FileRunEventList.test.tsx | 43 +- .../components/failures/FileRunEventList.tsx | 19 +- .../editor/src/portal/views/Documents.tsx | 10 +- .../notificationActions.test.tsx | 271 +++++++++ .../notifications/notificationActions.ts | 155 +++++ .../useNotificationsAvailable.ts | 7 + .../policies/usePolicyAutoRun.chain.test.tsx | 38 +- .../policies/usePolicyAutoRun.retry.test.tsx | 8 +- .../components/policies/usePolicyAutoRun.ts | 7 +- .../proprietary/services/policyApi.test.ts | 48 ++ .../src/proprietary/services/policyApi.ts | 7 +- 72 files changed, 4246 insertions(+), 228 deletions(-) create mode 100644 app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureAudience.java create mode 100644 app/proprietary/src/main/java/stirling/software/proprietary/failure/Ownership.java create mode 100644 app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationController.java create mode 100644 app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationService.java create mode 100644 app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationSource.java create mode 100644 app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationView.java create mode 100644 app/proprietary/src/test/java/stirling/software/proprietary/failure/CheckConstrainedEnumsTest.java create mode 100644 app/proprietary/src/test/java/stirling/software/proprietary/failure/NotificationProjectionTest.java create mode 100644 app/proprietary/src/test/java/stirling/software/proprietary/failure/PolicyFailureOwnershipTest.java create mode 100644 frontend/editor/src/core/components/notifications/NotificationBell.css create mode 100644 frontend/editor/src/core/components/notifications/NotificationBell.test.tsx create mode 100644 frontend/editor/src/core/components/notifications/NotificationBell.tsx create mode 100644 frontend/editor/src/core/components/notifications/NotificationItem.tsx create mode 100644 frontend/editor/src/core/components/notifications/notificationActions.ts create mode 100644 frontend/editor/src/core/components/notifications/useNotificationsAvailable.ts rename frontend/editor/src/{proprietary => core}/components/shared/DividerWithText.stories.tsx (100%) rename frontend/editor/src/{proprietary => core}/components/shared/DividerWithText.tsx (100%) rename frontend/editor/src/{proprietary => core}/components/shared/dividerWithText/DividerWithText.css (100%) create mode 100644 frontend/editor/src/core/contexts/file/removeFiles.reporting.test.tsx create mode 100644 frontend/editor/src/core/hooks/useNotifications.test.ts create mode 100644 frontend/editor/src/core/hooks/useNotifications.ts create mode 100644 frontend/editor/src/core/services/localFilePresence.test.ts create mode 100644 frontend/editor/src/core/services/localFilePresence.ts create mode 100644 frontend/editor/src/core/services/notifications.ts create mode 100644 frontend/editor/src/core/ui/BellIcon.tsx create mode 100644 frontend/editor/src/proprietary/components/notifications/notificationActions.test.tsx create mode 100644 frontend/editor/src/proprietary/components/notifications/notificationActions.ts create mode 100644 frontend/editor/src/proprietary/components/notifications/useNotificationsAvailable.ts create mode 100644 frontend/editor/src/proprietary/services/policyApi.test.ts diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionException.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionException.java index 1c899ab529..bd0ed2d001 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionException.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionException.java @@ -1,20 +1,17 @@ package stirling.software.proprietary.failure; +import org.springframework.http.HttpStatus; + import lombok.Getter; -/** - * Why an action could not be dispatched. Carries a {@link Reason} rather than an HTTP status, so - * the service stays web-agnostic and the controller owns the mapping. - */ +/** Carries a {@link Reason} rather than an HTTP status, so the service stays web-agnostic. */ @Getter public class FailureActionException extends RuntimeException { public enum Reason { /** - * No such event, it belongs to another team, or the caller's team did not resolve. One - * reason for all three, so the response does not vary with which it was. Unrelated to - * {@link FailureKind#UNKNOWN}, which is an unclassified failure rather than a refused - * action. + * No such event, another team's, or an unresolved team: one reason, so the answer cannot + * vary. */ EVENT_NOT_FOUND, @@ -22,15 +19,13 @@ public class FailureActionException extends RuntimeException { ACTION_NOT_RECOGNISED, /** - * The action exists but this kind does not declare it, so an incoherent pairing (releasing - * a document whose destination is what failed) cannot be dispatched even by hand. - * - *

Unreachable today: both kinds declare both actions, so no request can trip this guard - * until a kind ships with a restricted action set. Declared now because the guard must - * exist before that kind does, not after. + * The action exists but this kind does not offer it, so it cannot be dispatched by hand. */ ACTION_NOT_DECLARED, + /** Offered, but the client is what runs it, so refused rather than half-performed. */ + ACTION_NOT_DISPATCHABLE, + /** The event is already closed, so no further transition is possible. */ ALREADY_CLOSED } @@ -41,9 +36,21 @@ public class FailureActionException extends RuntimeException { this(reason, message, null); } - /** For a refusal that follows from a lower-level failure, so its stack is not dropped. */ public FailureActionException(Reason reason, String message, Throwable cause) { super(message, cause); this.reason = reason; } + + /** + * Lives with the reasons it maps, so every surface that dispatches an action answers alike. A + * closed row is a conflict, not a bad request: it was well-formed and valid a moment earlier. + */ + public static HttpStatus statusOf(Reason reason) { + return switch (reason) { + case EVENT_NOT_FOUND -> HttpStatus.NOT_FOUND; + case ACTION_NOT_RECOGNISED, ACTION_NOT_DECLARED, ACTION_NOT_DISPATCHABLE -> + HttpStatus.BAD_REQUEST; + case ALREADY_CLOSED -> HttpStatus.CONFLICT; + }; + } } diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionId.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionId.java index dbfcff7fb4..8f60e9f4a0 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionId.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionId.java @@ -1,11 +1,52 @@ package stirling.software.proprietary.failure; +import lombok.Getter; + /** - * The actions a {@link FailureKind} may declare. Both are incident dispositions: they change how - * the event is shown and touch nothing else, which is what makes them valid for every kind - * including {@link FailureKind#UNKNOWN}, and why there is no {@code APPROVE} yet. + * The actions a {@link FailureKind} may declare. Client actions are declared here rather than + * invented per client, so the server keeps deciding what a kind offers, in what order and labelled + * how. */ +@Getter public enum FailureActionId { - ACKNOWLEDGE, - DISMISS + + /** + * Kept in the vocabulary for as long as any persisted row is {@code ACKNOWLEDGED}: such rows + * must stay readable and closable whether or not any kind currently offers this. + */ + ACKNOWLEDGE(Execution.SERVER, "Acknowledge"), + + DISMISS(Execution.SERVER, "Dismiss"), + + /** Open the document behind the incident, in whichever client can resolve its id. */ + VIEW_FILE(Execution.CLIENT, "View file"), + + VIEW_IN_PROCESSOR(Execution.CLIENT, "View in processor"); + + /** Dispatch refuses a {@code CLIENT} id, so this is enforced rather than merely documented. */ + public enum Execution { + + /** {@link FailureActionRegistry} requires a {@link FailureAction} bean for these. */ + SERVER, + + /** + * Declared and rendered, never dispatched: the server has neither the file nor the tool. + */ + CLIENT + } + + private final Execution execution; + + /** English fallback, for a client with no translation for the label key. */ + private final String defaultLabel; + + FailureActionId(Execution execution, String defaultLabel) { + this.execution = execution; + this.defaultLabel = defaultLabel; + } + + /** Also whether it can be dispatched. */ + public boolean runsOnServer() { + return execution == Execution.SERVER; + } } diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionRegistry.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionRegistry.java index 3928e91845..dad1bd5eda 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionRegistry.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionRegistry.java @@ -16,6 +16,9 @@ import lombok.extern.slf4j.Slf4j; * Resolves a {@link FailureActionId} to the bean that implements it. The startup check is the * point: because kinds declare action ids as data, one could name an action nobody implements, * which would otherwise show up as a button that 400s rather than as a failed boot. + * + *

Only {@link FailureActionId.Execution#SERVER} ids belong here: a bean for a client action is + * refused, because dispatch could never reach it. */ @Slf4j @Service @@ -25,6 +28,14 @@ public class FailureActionRegistry { public FailureActionRegistry(List actions) { for (FailureAction action : actions) { + if (!action.id().runsOnServer()) { + throw new IllegalStateException( + "Action " + + action.id() + + " is run by the client, so " + + action.getClass().getName() + + " could never be dispatched"); + } FailureAction clash = byId.put(action.id(), action); if (clash != null) { throw new IllegalStateException( @@ -38,10 +49,7 @@ public class FailureActionRegistry { } } - /** - * Fail fast if any kind declares an action with no handler, naming every gap rather than the - * first, so one boot tells you everything that is missing. - */ + /** Names every gap rather than the first, so one boot tells you everything that is missing. */ @PostConstruct void verifyEveryDeclaredActionHasAHandler() { List gaps = @@ -49,6 +57,7 @@ public class FailureActionRegistry { .flatMap( kind -> kind.getActions().stream() + .filter(FailureActionId::runsOnServer) .filter(action -> !byId.containsKey(action)) .map(action -> kind.getId() + " -> " + action)) .toList(); diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureAudience.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureAudience.java new file mode 100644 index 0000000000..217dfc7975 --- /dev/null +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureAudience.java @@ -0,0 +1,15 @@ +package stirling.software.proprietary.failure; + +/** + * Who an offered action is for, the read scope having already decided they may see the incident. + * The distinction is possession, not seniority: a reviewer cannot reach a document only its owner + * holds. + */ +public enum FailureAudience { + OWNER, + + /** Anyone who triages the team's incidents, whoever hit them. */ + TEAM_REVIEWER, + + ANYONE_WHO_SEES +} diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureKind.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureKind.java index c9acd0d590..fa2fd93448 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureKind.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureKind.java @@ -1,7 +1,11 @@ package stirling.software.proprietary.failure; -import static stirling.software.proprietary.failure.FailureActionId.ACKNOWLEDGE; import static stirling.software.proprietary.failure.FailureActionId.DISMISS; +import static stirling.software.proprietary.failure.FailureActionId.VIEW_FILE; +import static stirling.software.proprietary.failure.FailureActionId.VIEW_IN_PROCESSOR; +import static stirling.software.proprietary.failure.FailureAudience.ANYONE_WHO_SEES; +import static stirling.software.proprietary.failure.FailureAudience.OWNER; +import static stirling.software.proprietary.failure.FailureAudience.TEAM_REVIEWER; import java.util.Arrays; import java.util.HashMap; @@ -20,13 +24,8 @@ import lombok.Getter; * The registry of failure kinds, described as data: a stable id, i18n keys and an English fallback * like {@code ExceptionUtils.ErrorCode}, plus the facets a review surface needs. * - *

Actions are declared here but implemented in {@link FailureAction} beans resolved by id, so a - * new kind ships as a registry entry plus copy. Two members today: {@link #UNKNOWN} gives every - * failed run a record, and kinds get promoted out of it as production shows what occurs. - * - *

A kind offers an acknowledgement only where there is something to acknowledge doing. - * With nothing to fix, "seen it" and "clear it" are the same decision, so the row offers only the - * one that clears it. + *

A new kind ships as a registry entry plus copy. Each offer also says who it is for, since one + * incident is read both by whoever hit it and by whoever reviews after them. */ @Getter public enum FailureKind { @@ -37,8 +36,9 @@ public enum FailureKind { FailureScope.FILE, errorCodes("E004"), fallback("This document is password-protected, so the pipeline could not read it."), - offer(ACKNOWLEDGE), - offer(DISMISS, "dismissSkipFile")), + offer(VIEW_FILE, OWNER), + offer(VIEW_IN_PROCESSOR, TEAM_REVIEWER), + offer(DISMISS, ANYONE_WHO_SEES)), UNKNOWN( FailureStage.INTERNAL, @@ -47,7 +47,11 @@ public enum FailureKind { FailureScope.RUN, noErrorCodes(), fallback("This run failed for a reason Stirling does not yet recognise."), - offer(DISMISS)); + // Same order as every other kind: declaration order is display order, so the document + // leads wherever it is offered rather than moving between failures. + offer(VIEW_FILE, OWNER), + offer(VIEW_IN_PROCESSOR, TEAM_REVIEWER), + offer(DISMISS, ANYONE_WHO_SEES)); private static final String KEY_PREFIX = "portal.failures.kind."; private static final String ACTION_KEY_PREFIX = "portal.failures.action."; @@ -95,22 +99,26 @@ public enum FailureKind { } /** - * One action this kind offers, with the key to label it by. One ordered list rather than ids - * plus a parallel map of overrides, which could disagree with each other. + * One ordered list rather than ids plus parallel maps of audiences and labels, which could + * disagree with each other. * * @param labelKeySuffix key under {@code portal.failures.action.}, or null for the generic * label */ - private record Offer(FailureActionId id, String labelKeySuffix) {} + private record Offer(FailureActionId id, FailureAudience audience, String labelKeySuffix) {} - /** An action labelled by this kind's own wording, where the generic label reads badly. */ - private static Offer offer(FailureActionId id, String labelKeySuffix) { - return new Offer(id, labelKeySuffix); + /** Declaration order is display order. */ + private static Offer offer(FailureActionId id, FailureAudience audience) { + return new Offer(id, audience, null); } - /** An action labelled by the shared wording for that action. */ - private static Offer offer(FailureActionId id) { - return new Offer(id, null); + /** + * As {@link #offer(FailureActionId, FailureAudience)}, but labelled by this kind's own wording + * where the shared one reads badly. + */ + private static Offer offer( + FailureActionId id, FailureAudience audience, String labelKeySuffix) { + return new Offer(id, audience, labelKeySuffix); } /** @@ -149,6 +157,22 @@ public enum FailureKind { return offers.stream().map(Offer::id).toList(); } + /** + * What this kind offers, in declaration order, each with its label resolved. What a review + * surface reads, so it never has to ask two separate questions about one offer. + */ + public List getOfferedActions() { + return offers.stream() + .map( + offer -> + new OfferedAction( + offer.id(), labelKeyFor(offer.id()), offer.audience())) + .toList(); + } + + /** One action as a kind declares it: what to call it and who it is for. */ + public record OfferedAction(FailureActionId id, String labelKey, FailureAudience audience) {} + /** Whether this kind offers {@code action}. The dispatch guard: see {@code FailureActionId}. */ public boolean declares(FailureActionId action) { return offers.stream().anyMatch(offer -> offer.id() == action); diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventController.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventController.java index aac1c3364b..24d3558c25 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventController.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventController.java @@ -74,8 +74,10 @@ public class FileRunEventController { @Operation( summary = "Apply an action to a recorded failure", description = - "Rejected with 400 if the failure's kind does not declare the action, so an" - + " action that makes no sense for a given failure cannot be applied.") + "Rejected with 400 if the failure's kind does not declare the action, or if the" + + " action is one the client runs rather than the server, so neither an" + + " action that makes no sense for a given failure nor one the server" + + " cannot perform can be applied.") public FileRunEventView act( @PathVariable String eventId, @PathVariable String actionId, @@ -87,7 +89,8 @@ public class FileRunEventController { FileRunEvent updated = service.dispatch(eventId, actionId, inputs); return FileRunEventView.of(updated, service.availableActions(updated)); } catch (FailureActionException e) { - throw new ResponseStatusException(statusFor(e.getReason()), e.getMessage(), e); + throw new ResponseStatusException( + FailureActionException.statusOf(e.getReason()), e.getMessage(), e); } } @@ -147,18 +150,6 @@ public class FileRunEventController { return Arrays.stream(FailureKind.values()).map(FailureKindView::of).toList(); } - /** - * A closed row is a conflict rather than a bad request: the request was well-formed and would - * have been valid a moment earlier. - */ - private static HttpStatus statusFor(FailureActionException.Reason reason) { - return switch (reason) { - case EVENT_NOT_FOUND -> HttpStatus.NOT_FOUND; - case ACTION_NOT_RECOGNISED, ACTION_NOT_DECLARED -> HttpStatus.BAD_REQUEST; - case ALREADY_CLOSED -> HttpStatus.CONFLICT; - }; - } - /** Wrapped rather than a bare array so pagination can be added without breaking clients. */ public record FileRunEventsResponse(List events) {} @@ -178,10 +169,13 @@ public class FileRunEventController { } } - /** Inputs an action declared it needs. Empty for both actions that exist today. */ + /** + * Inputs an action declared it needs. Empty for every action the server runs today: the one + * that needs a password is run by the client, which never sends it here. + */ public record ActionRequest(Map inputs) { - Map safeInputs() { + public Map safeInputs() { return inputs == null ? Map.of() : inputs; } } diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventRepository.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventRepository.java index 1930b36e6a..316dd34925 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventRepository.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventRepository.java @@ -98,20 +98,18 @@ public interface FileRunEventRepository extends JpaRepositoryRestricted to that owner's own editor rows. File ids are minted by the client, so scoping - * on team alone would let one caller close a colleague's incidents by naming ids. Processor - * rows are excluded outright: nothing was deleted from an editor there. + *

Scoped by the absence of a source rather than by origin: a source-fed run's {@code fileId} + * is a hash no client can name. Narrowed to the owner's own rows, since clients mint the ids. */ @Modifying(clearAutomatically = true) @Transactional @Query( "update FileRunEventEntity e set e.status =" + " stirling.software.proprietary.failure.FileRunEventStatus.FILE_REMOVED," - + " e.statusActor = :actor, e.statusAt = :now where e.origin =" - + " stirling.software.proprietary.failure.FailureOrigin.TOOL and ((:teamId is" - + " null and e.teamId is null) or e.teamId = :teamId) and ((:actor is null and" - + " e.actor is null) or e.actor = :actor) and e.fileId in :fileIds and e.status in" - + " :allowedFrom") + + " e.statusActor = :actor, e.statusAt = :now where e.sourceId is null and" + + " ((:teamId is null and e.teamId is null) or e.teamId = :teamId) and" + + " ((:actor is null and e.actor is null) or e.actor = :actor) and e.fileId in" + + " :fileIds and e.status in :allowedFrom") int markFilesRemoved( @Param("teamId") Long teamId, @Param("actor") String actor, diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventService.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventService.java index fa858b6e22..10f45f46c0 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventService.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventService.java @@ -21,12 +21,22 @@ import stirling.software.proprietary.policy.config.PolicyManagementAuthority; * team always comes from the authenticated principal, and scoping applies only when login is * enabled so single-user deployments keep working. When the team cannot be resolved the caller * reads nothing; see {@link #readScope()}. + * + *

The read scope decides who sees an incident; {@link #availableActions} decides who may act. */ @Slf4j @Service @RequiredArgsConstructor public class FileRunEventService { + /** Why an offered action came back disabled. Copy lives under {@code portal.failures}. */ + private static final String CLOSED_REASON_KEY = "portal.failures.disabled.closed"; + + private static final String UNATTENDED_REASON_KEY = "portal.failures.disabled.unattended"; + + /** The row never named a document, so unlike the unattended case no client can find one. */ + private static final String DOCUMENTLESS_REASON_KEY = "portal.failures.disabled.noDocument"; + private final FileRunEventStore store; private final FailureActionRegistry actionRegistry; private final PolicyManagementAuthority policyManagementAuthority; @@ -116,36 +126,17 @@ public class FileRunEventService { * Dispatch an action against one event. * * @throws FailureActionException if the event is not the caller's, the action is unknown, the - * event's kind does not declare the action, or the event is already closed + * event's kind does not declare the action, the client is what runs the action, or the + * event is already closed */ public FileRunEvent dispatch(String eventId, String actionId, Map inputs) { // Whoever can see it can close it: a leader for the whole team, everyone else for the // failures they caused. Someone who fixes their own problem should not have to ask a leader // to clear the row. // - // Closing the row is all this covers. Acting on the document behind it, such as supplying a - // password for a retry, would need its own permission, and no such action exists yet. - ReadScope scope = readScope(); - if (!scope.permitted()) { - // Reported as "no such event", the same as an id from another team, so the response - // does - // not depend on whether the id happens to exist. - throw new FailureActionException( - FailureActionException.Reason.EVENT_NOT_FOUND, "No such event: " + eventId); - } - FileRunEvent event = - store.find(eventId, scope.teamId()) - // Reported as "no such event" rather than a refusal, so a member cannot - // learn that a colleague's incident exists by trying to close it. - .filter( - found -> - scope.actor() == null - || scope.actor().equals(found.actor())) - .orElseThrow( - () -> - new FailureActionException( - FailureActionException.Reason.EVENT_NOT_FOUND, - "No such event: " + eventId)); + // Audience decides what is offered, not what may be dispatched, so this scope is the whole + // gate. A server action aimed at OWNER alone would need its own guard here. + FileRunEvent event = requireVisible(eventId); FailureActionId resolvedId = parseActionId(actionId); @@ -156,6 +147,12 @@ public class FileRunEventService { FailureActionException.Reason.ACTION_NOT_DECLARED, "Kind " + event.kind().getId() + " does not offer action " + resolvedId); } + // Without this a client could post VIEW_FILE and be answered as though something happened. + if (!resolvedId.runsOnServer()) { + throw new FailureActionException( + FailureActionException.Reason.ACTION_NOT_DISPATCHABLE, + "Action " + resolvedId + " is run by the client, not the server"); + } if (event.status().terminal()) { throw new FailureActionException( FailureActionException.Reason.ALREADY_CLOSED, @@ -174,23 +171,91 @@ public class FileRunEventService { return action.execute(event, inputs == null ? Map.of() : inputs, currentActor()); } + /** "No such event" rather than a refusal, so trying does not confirm a colleague's exists. */ + private FileRunEvent requireVisible(String eventId) { + ReadScope scope = readScope(); + if (!scope.permitted()) { + return notFound(eventId); + } + return store.find(eventId, scope.teamId()) + .filter(found -> scope.actor() == null || scope.actor().equals(found.actor())) + .orElseGet(() -> notFound(eventId)); + } + + private FileRunEvent notFound(String eventId) { + throw new FailureActionException( + FailureActionException.Reason.EVENT_NOT_FOUND, "No such event: " + eventId); + } + + public Ownership ownershipOf(FileRunEvent event) { + if (event.actor() == null) { + return Ownership.UNOWNED; + } + String caller = currentActor(); + return event.actor().equals(caller) ? Ownership.MINE : Ownership.THEIRS; + } + /** - * Which of an event's declared actions are usable right now. Decided per row, so the client - * never renders a button that would be refused. + * Offers resolved for one caller, so no client renders a button that would be refused. Outside + * their audience is dropped, not disabled: greyed out would read as a permission problem. */ public List availableActions(FileRunEvent event) { + Ownership ownership = ownershipOf(event); + boolean reviewsTeam = reviewsTeam(); boolean closed = event.status().terminal(); - return event.kind().getActions().stream() - .map( - action -> - new AvailableAction( - action, - event.kind().labelKeyFor(action), - !closed, - closed ? "portal.failures.disabled.closed" : null)) + // Login disabled is excluded: its rows are unowned only for want of users, and its one + // operator owns everything they can see. + boolean unattended = enforced() && ownership == Ownership.UNOWNED; + // Answered here, or the client reports "not on this device" about a document the row never + // identified in the first place. + boolean documentless = event.fileId() == null || event.fileId().isBlank(); + return event.kind().getOfferedActions().stream() + .filter(offer -> offeredTo(offer.audience(), ownership, reviewsTeam)) + .map(offer -> availability(offer, closed, unattended, documentless)) .toList(); } + /** Enabled is derived from the reason, so a disabled button always has one to show. */ + private static AvailableAction availability( + FailureKind.OfferedAction offer, + boolean closed, + boolean unattended, + boolean documentless) { + String reason = disabledReasonFor(offer.audience(), closed, unattended, documentless); + return new AvailableAction(offer.id(), offer.labelKey(), reason == null, reason); + } + + /** Closed wins over everything, then the owner-only reasons, most specific first. */ + private static String disabledReasonFor( + FailureAudience audience, boolean closed, boolean unattended, boolean documentless) { + if (closed) { + return CLOSED_REASON_KEY; + } + if (audience != FailureAudience.OWNER) { + return null; + } + if (unattended) { + return UNATTENDED_REASON_KEY; + } + return documentless ? DOCUMENTLESS_REASON_KEY : null; + } + + /** An unattended incident has no owner, so its reviewer inherits the owner's actions. */ + private static boolean offeredTo( + FailureAudience audience, Ownership ownership, boolean reviewsTeam) { + return switch (audience) { + case OWNER -> + ownership == Ownership.MINE || (ownership == Ownership.UNOWNED && reviewsTeam); + case TEAM_REVIEWER -> reviewsTeam; + case ANYONE_WHO_SEES -> true; + }; + } + + /** Login disabled has no roles, so its one operator triages everything. */ + private boolean reviewsTeam() { + return !enforced() || policyManagementAuthority.canEditPolicies(); + } + private FailureActionId parseActionId(String actionId) { for (FailureActionId candidate : FailureActionId.values()) { if (candidate.name().equals(actionId)) { @@ -261,7 +326,6 @@ public class FileRunEventService { return applicationProperties.getSecurity().isEnableLogin(); } - /** One action as offered for a specific event, with its resolved availability. */ public record AvailableAction( FailureActionId id, String labelKey, boolean enabled, String disabledReasonKey) {} } diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventView.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventView.java index 3809fad2c6..b88ba3d48d 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventView.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventView.java @@ -60,14 +60,24 @@ public record FileRunEventView( event.lastSeenAt() == null ? 0L : event.lastSeenAt().toEpochMilli()); } - /** One button, as offered for this specific row. */ + /** + * {@code defaultLabel} and {@code execution} let a client render and route an action it was + * never built with. Declaration order is display order. + */ public record ActionView( - String id, String labelKey, boolean enabled, String disabledReasonKey) { + String id, + String labelKey, + String defaultLabel, + FailureActionId.Execution execution, + boolean enabled, + String disabledReasonKey) { - static ActionView of(FileRunEventService.AvailableAction action) { + public static ActionView of(FileRunEventService.AvailableAction action) { return new ActionView( action.id().name(), action.labelKey(), + action.id().getDefaultLabel(), + action.id().getExecution(), action.enabled(), action.disabledReasonKey()); } diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/Ownership.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/Ownership.java new file mode 100644 index 0000000000..65745a61b1 --- /dev/null +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/Ownership.java @@ -0,0 +1,17 @@ +package stirling.software.proprietary.failure; + +/** + * Whose incident this is, from the reader's point of view. Derived on read, never persisted: one + * row is {@code MINE} to whoever hit it and {@code THEIRS} to the leader reviewing after them. + */ +public enum Ownership { + MINE, + + /** A colleague's, visible because the caller reviews the team. */ + THEIRS, + + /** + * An unattended run: a folder, bucket or webhook is its only attribution, so there is no owner. + */ + UNOWNED +} diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationController.java b/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationController.java new file mode 100644 index 0000000000..f2bf36a8dc --- /dev/null +++ b/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationController.java @@ -0,0 +1,48 @@ +package stirling.software.proprietary.notification; + +import java.util.List; + +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.web.bind.annotation.RestController; + +import io.swagger.v3.oas.annotations.Hidden; +import io.swagger.v3.oas.annotations.Operation; +import io.swagger.v3.oas.annotations.tags.Tag; + +import lombok.RequiredArgsConstructor; + +/** + * Open to any authenticated user, unlike the failure endpoints it draws on: each source scopes its + * own rows. Read-only, because every action a notification offers runs on the client's own device. + */ +@RestController +@RequestMapping("/api/v1/notifications") +@Hidden +@RequiredArgsConstructor +@Tag(name = "Notifications", description = "Things worth telling the caller about") +public class NotificationController { + + /** How many notifications one read returns when the caller does not say: one panelful. */ + private static final int DEFAULT_LIMIT = 20; + + /** The most one read may return however large a limit the caller asks for. */ + private static final int MAX_LIMIT = 100; + + private final NotificationService notifications; + + @GetMapping + @Operation( + summary = "List the caller's notifications", + description = + "Newest first. Derived from the sources that produce them, so there is nothing" + + " to mark read here yet: the client tracks what it has shown.") + public NotificationsResponse list(@RequestParam(required = false) Integer limit) { + int capped = Math.min(limit == null ? DEFAULT_LIMIT : Math.max(1, limit), MAX_LIMIT); + return new NotificationsResponse(notifications.list(capped)); + } + + /** Wrapped so paging or a total can be added without breaking clients. */ + public record NotificationsResponse(List notifications) {} +} diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationService.java b/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationService.java new file mode 100644 index 0000000000..f7bf3b8530 --- /dev/null +++ b/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationService.java @@ -0,0 +1,53 @@ +package stirling.software.proprietary.notification; + +import java.util.List; + +import org.springframework.stereotype.Service; + +import lombok.RequiredArgsConstructor; + +import stirling.software.proprietary.failure.FileRunEvent; +import stirling.software.proprietary.failure.FileRunEventService; +import stirling.software.proprietary.failure.FileRunEventView; + +/** + * Derived on read rather than stored: one source today, and a table would need a write path, + * retention and a per-user read model first. Each source scopes its own rows, so this cannot widen. + */ +@Service +@RequiredArgsConstructor +public class NotificationService { + + private final FileRunEventService fileRunEvents; + + /** Newest first, and only open failures: one already dealt with is not news. */ + public List list(int limit) { + return fileRunEvents.list(null, null, limit).stream().map(this::fromFailure).toList(); + } + + /** Prefixes the row id on the way out, so it is never sent bare. */ + private NotificationView fromFailure(FileRunEvent event) { + return new NotificationView( + NotificationSource.FAILURE.qualify(event.id()), + NotificationSource.FAILURE, + event.kind().getId(), + event.origin(), + fileRunEvents.ownershipOf(event), + event.severity(), + event.status(), + event.kind().getTitleKey(), + event.kind().getDefaultTitle(), + event.detail(), + event.fileId(), + event.sourceId(), + event.policyId(), + event.occurrences(), + event.createdAt(), + event.lastSeenAt(), + // A disposition such as Dismiss belongs to the review surface, not the bell. + fileRunEvents.availableActions(event).stream() + .filter(action -> !action.id().runsOnServer()) + .map(FileRunEventView.ActionView::of) + .toList()); + } +} diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationSource.java b/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationSource.java new file mode 100644 index 0000000000..007e51616f --- /dev/null +++ b/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationSource.java @@ -0,0 +1,21 @@ +package stirling.software.proprietary.notification; + +import java.util.Locale; + +/** + * Which subsystem produced a notification. Every id is prefixed with it, so a client never holds + * the producing row's own id and cannot reach that source's endpoints by accident. + */ +public enum NotificationSource { + FAILURE; + + private static final char SEPARATOR = ':'; + + public String prefix() { + return name().toLowerCase(Locale.ROOT) + SEPARATOR; + } + + public String qualify(String sourceRowId) { + return prefix() + sourceRowId; + } +} diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationView.java b/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationView.java new file mode 100644 index 0000000000..be04153686 --- /dev/null +++ b/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationView.java @@ -0,0 +1,33 @@ +package stirling.software.proprietary.notification; + +import java.time.Instant; +import java.util.List; + +import stirling.software.proprietary.failure.FailureOrigin; +import stirling.software.proprietary.failure.FailureSeverity; +import stirling.software.proprietary.failure.FileRunEventStatus; +import stirling.software.proprietary.failure.FileRunEventView; +import stirling.software.proprietary.failure.Ownership; + +/** + * A source's row flattened to what a bell renders. {@code fileId} is an opaque reference, never a + * name, and two id spaces share it: {@code sourceId} tells them apart. + */ +public record NotificationView( + String id, + NotificationSource source, + String kindId, + FailureOrigin origin, + Ownership ownership, + FailureSeverity severity, + FileRunEventStatus status, + String titleKey, + String defaultTitle, + String detail, + String fileId, + String sourceId, + String policyId, + int occurrences, + Instant createdAt, + Instant lastSeenAt, + List actions) {} diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/policy/controller/PolicyController.java b/app/proprietary/src/main/java/stirling/software/proprietary/policy/controller/PolicyController.java index 9b9fca4133..5a5a3018b1 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/policy/controller/PolicyController.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/policy/controller/PolicyController.java @@ -576,7 +576,9 @@ public class PolicyController { + " under 'fileInput', supporting files under 'assets[i].key' /" + " 'assets[i].file' - only for bindings the policy does not already" + " store). Runs regardless of the policy's enabled flag, which only" - + " gates automatic triggering. Returns a run id.") + + " gates automatic triggering. A single-document run may also send its" + + " own opaque 'fileId', which is recorded against any failure so the" + + " caller can resolve it back to that document. Returns a run id.") public ResponseEntity> runStoredPolicy( @PathVariable String policyId, @Valid @ModelAttribute PolicyRunFiles files) throws IOException { @@ -590,7 +592,14 @@ public class PolicyController { HttpStatus.NOT_FOUND, "No policy: " + policyId)); stampPolicyAudit(policy.toDefinition()); PolicyInputs inputs = toInputs(files); - String runId = policyRunner.runWith(policy, inputs, PolicyProgressListener.NOOP).runId(); + String runId = + policyRunner + .runWith( + policy, + inputs, + PolicyProgressListener.NOOP, + documentReferenceFor(files, inputs)) + .runId(); return ResponseEntity.accepted().body(new JobResponse<>(true, runId, null)); } @@ -722,6 +731,19 @@ public class PolicyController { return new PolicyInputs(primary, supportingFiles); } + /** + * Only for a single-document run: an incident holds one file reference, so naming one of + * several would attribute the failure to whichever bound first. Counted off resolved inputs, + * not parts. + */ + private static String documentReferenceFor(PolicyRunFiles files, PolicyInputs inputs) { + String fileId = files.getFileId(); + if (fileId == null || fileId.isBlank() || inputs.primary().size() != 1) { + return null; + } + return fileId; + } + private PolicyProgressListener streamListener(SseEmitter emitter) { return new PolicyProgressListener() { @Override diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/policy/controller/PolicyRunFiles.java b/app/proprietary/src/main/java/stirling/software/proprietary/policy/controller/PolicyRunFiles.java index f42e02a07d..fd4a791c8b 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/policy/controller/PolicyRunFiles.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/policy/controller/PolicyRunFiles.java @@ -16,8 +16,8 @@ import lombok.Data; * from the multipart request via {@code @ModelAttribute}; the pipeline definition itself travels as * a separate typed {@code json} part. * - *

Wire form: {@code fileInput} (repeated) for primaries, and {@code assets[i].key} / {@code - * assets[i].file} for each supporting asset. + *

Wire form: {@code fileInput} (repeated) for primaries, {@code assets[i].key} / {@code + * assets[i].file} for each supporting asset, and the optional {@code fileId}. */ @Data @Schema(description = "Files for a policy run: primary documents plus keyed supporting assets") @@ -29,4 +29,16 @@ public class PolicyRunFiles { @Valid @Schema(description = "Supporting files, each bound to the asset key its step references") private List assets = new ArrayList<>(); + + /** + * Recorded against any failure of this run, so the client can resolve the row back to its + * document. Opaque by contract, never a name, and only honoured for a single-document run. + */ + @Schema( + description = + "The caller's opaque id for the document being run, echoed onto any failure" + + " recorded for this run so the originating client can resolve it." + + " Ignored unless exactly one primary document is supplied. Never a" + + " filename.") + private String fileId; } diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyEngine.java b/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyEngine.java index 0d75866a6f..f9c0f719ef 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyEngine.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyEngine.java @@ -151,7 +151,8 @@ public class PolicyEngine { * As {@link #runPolicy(Policy, PolicyInputs, PolicyProgressListener)}, recording which source * fed the run and its opaque reference to the document. The first says where an unattended * failure came from; the second says which document, and is what lets the same document failing - * again fold into one incident. Both null for a user's upload. + * again fold into one incident. With no source {@code fileIdentity} is the client's own + * reference, with one it is that source's hash; this engine only carries it either way. */ public PolicyRunHandle runPolicy( Policy policy, diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyRunner.java b/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyRunner.java index b4609f94ea..d159ebf012 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyRunner.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyRunner.java @@ -120,10 +120,17 @@ public class PolicyRunner { * Run a stored policy on caller-supplied files (e.g. an editor upload), bypassing its sources. * The supplied documents are still counted against the virtual {@link EditorSource}, scoped to * the policy's team, so the Sources overview reports the whole team's editor throughput. + * + * @param documentReference the caller's own opaque reference to the single document it runs on, + * or null when it supplied none or several. Passed through untouched. */ public PolicyRunHandle runWith( - Policy policy, PolicyInputs inputs, PolicyProgressListener listener) { - PolicyRunHandle handle = policyEngine.runPolicy(policy, inputs, listener); + Policy policy, + PolicyInputs inputs, + PolicyProgressListener listener, + String documentReference) { + PolicyRunHandle handle = + policyEngine.runPolicy(policy, inputs, listener, null, documentReference); docCounter.record(EditorSource.counterKey(policy.teamId()), inputs.primary().size()); return handle; } diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/CheckConstrainedEnumsTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/CheckConstrainedEnumsTest.java new file mode 100644 index 0000000000..7eaeb01eb4 --- /dev/null +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/CheckConstrainedEnumsTest.java @@ -0,0 +1,57 @@ +package stirling.software.proprietary.failure; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.Arrays; +import java.util.List; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +/** + * Pins the five enums {@code file_run_events} stores behind CHECK constraints: adding a value is a + * schema change dressed as a Java one, compiling here and failing against a real database. + */ +class CheckConstrainedEnumsTest { + + @Test + @DisplayName("no value has been added to a CHECK-constrained column's enum") + void everyPersistedEnumStillMatchesTheShippedCheckConstraints() { + assertThat(names(FileRunEventStatus.values())) + .containsExactlyInAnyOrder( + "NEW", "ACKNOWLEDGED", "DISMISSED", "RESOLVED", "FILE_REMOVED"); + assertThat(names(FailureOrigin.values())) + .containsExactlyInAnyOrder("TOOL", "POLICY", "PIPELINE"); + assertThat(names(FailureStage.values())) + .containsExactlyInAnyOrder("INPUT", "INTERNAL", "OUTPUT", "BLOCKED", "NEVER_RAN"); + assertThat(names(FailureSeverity.values())) + .containsExactlyInAnyOrder("ERROR", "WARNING", "INFO"); + assertThat(names(FailureScope.values())) + .containsExactlyInAnyOrder("FILE", "RUN", "POLICY", "SOURCE", "SERVER"); + } + + @Test + @DisplayName("the facets added since are derived, not stored") + void nothingAddedToTheModelReachedTheTable() throws Exception { + // Resolved per reader, so a column would hold the wrong answer for all but one person. + List> persisted = + Arrays.stream(FileRunEventEntity.class.getDeclaredFields()) + .filter(field -> !field.isSynthetic()) + .map(java.lang.reflect.Field::getType) + .toList(); + + assertThat(persisted) + .doesNotContain( + FailureAudience.class, + FailureActionId.class, + FailureActionId.Execution.class, + Ownership.class); + // A plain varchar with no CHECK, which is what lets a new kind ship without a migration. + assertThat(FileRunEventEntity.class.getDeclaredField("kindId").getType()) + .isEqualTo(String.class); + } + + private static List names(Enum[] values) { + return Arrays.stream(values).map(Enum::name).toList(); + } +} diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FailureKindTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FailureKindTest.java index a9baf5288f..03aa5c9402 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FailureKindTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FailureKindTest.java @@ -1,6 +1,9 @@ package stirling.software.proprietary.failure; import static org.assertj.core.api.Assertions.assertThat; +import static stirling.software.proprietary.failure.FailureAudience.ANYONE_WHO_SEES; +import static stirling.software.proprietary.failure.FailureAudience.OWNER; +import static stirling.software.proprietary.failure.FailureAudience.TEAM_REVIEWER; import java.io.IOException; import java.io.UncheckedIOException; @@ -29,6 +32,13 @@ import stirling.software.common.util.ExceptionUtils; */ class FailureKindTest { + /** In full, so a declaration pairing the right action with the wrong audience cannot pass. */ + private static FailureKind.OfferedAction offered( + FailureActionId id, FailureAudience audience, String labelKeySuffix) { + return new FailureKind.OfferedAction( + id, "portal.failures.action." + labelKeySuffix, audience); + } + @Nested @DisplayName("every kind is well formed") class Invariants { @@ -60,6 +70,27 @@ class FailureKindTest { assertThat(kind.getId()).matches("^[A-Z][A-Z0-9_]*$"); } + @ParameterizedTest + @EnumSource(FailureKind.class) + void declaresItsActionsInTheSameOrderAsEveryOtherKind(FailureKind kind) { + // Declaration order is display order and the first usable offer is the row's primary, + // so + // two kinds disagreeing would flip the solid button between rows. + List ranking = + List.of( + FailureActionId.VIEW_FILE, + FailureActionId.VIEW_IN_PROCESSOR, + FailureActionId.DISMISS); + + List declared = kind.getActions(); + assertThat(ranking) + .as("%s declares an action the shared ranking does not rank", kind.getId()) + .containsAll(declared); + assertThat(declared) + .as("%s declares its actions out of the shared order", kind.getId()) + .isEqualTo(ranking.stream().filter(declared::contains).toList()); + } + @Test void idsAreUnique() { Set ids = new HashSet<>(); @@ -88,6 +119,25 @@ class FailureKindTest { } } + @ParameterizedTest + @EnumSource(FailureKind.class) + void everyOfferSaysWhoItIsFor(FailureKind kind) { + // Read per row to decide what a caller is shown, so a null would leak a button. + for (FailureKind.OfferedAction offer : kind.getOfferedActions()) { + assertThat(offer.audience()) + .as("%s offers %s", kind.getId(), offer.id()) + .isNotNull(); + } + } + + @ParameterizedTest + @EnumSource(FailureKind.class) + void offersEachActionAtMostOnce(FailureKind kind) { + // The same action twice would be two buttons with one meaning, and labelKeyFor would + // answer for the first. + assertThat(kind.getActions()).doesNotHaveDuplicates(); + } + @Test void noTwoKindsClaimTheSameErrorCode() { // Computed independently of duplicateErrorCodes(), then checked against it: the boot @@ -182,10 +232,16 @@ class FailureKindTest { class Unknown { @Test - void offersOnlyTheActionThatClearsIt() { - // Nothing here can be fixed, so "seen it" and "clear it" would be the same decision. - // Offering both just asks the reviewer to press two buttons to reach one outcome. - assertThat(FailureKind.UNKNOWN.getActions()).containsExactly(FailureActionId.DISMISS); + void offersItsOwnerTheirDocumentAndTheRunToWhoeverReviews() { + // Nothing here is known to be fixable, so the offers are just the places to look. + assertThat(FailureKind.UNKNOWN.getOfferedActions()) + .containsExactly( + offered(FailureActionId.VIEW_FILE, OWNER, "viewFile"), + offered( + FailureActionId.VIEW_IN_PROCESSOR, + TEAM_REVIEWER, + "viewInProcessor"), + offered(FailureActionId.DISMISS, ANYONE_WHO_SEES, "dismiss")); } @Test @@ -238,24 +294,49 @@ class FailureKindTest { } @Test - void aKindWithSomethingToFixOffersTheFixAndAWayToSkipIt() { - assertThat(FailureKind.INPUT_PASSWORD_PROTECTED.getActions()) - .containsExactly(FailureActionId.ACKNOWLEDGE, FailureActionId.DISMISS); + void offersTheDocumentToItsOwnerAndTheRunToItsReviewer() { + // The point of the audiences: only the owner holds the document. + assertThat(FailureKind.INPUT_PASSWORD_PROTECTED.getOfferedActions()) + .containsExactly( + offered(FailureActionId.VIEW_FILE, OWNER, "viewFile"), + offered( + FailureActionId.VIEW_IN_PROCESSOR, + TEAM_REVIEWER, + "viewInProcessor"), + offered(FailureActionId.DISMISS, ANYONE_WHO_SEES, "dismiss")); } @Test - void overriddenLabelWinsOverTheGenericOne() { - String label = - FailureKind.INPUT_PASSWORD_PROTECTED.labelKeyFor(FailureActionId.DISMISS); - assertThat(label).isEqualTo("portal.failures.action.dismissSkipFile"); - assertThat(label).isNotEqualTo(FailureKind.genericLabelKey(FailureActionId.DISMISS)); + void noKindOffersAcknowledgeAnyMore() { + // Kept in the vocabulary for rows already ACKNOWLEDGED; offered by nothing, so + // dispatchable by nothing. + for (FailureKind kind : FailureKind.values()) { + assertThat(kind.declares(FailureActionId.ACKNOWLEDGE)) + .as("%s offers ACKNOWLEDGE", kind.getId()) + .isFalse(); + } } @Test - void genericLabelIsUsedWhenAKindDeclaresNoOverride() { + void everyKindLabelsItsActionsWithTheSharedWordingToday() { + // The per-kind override still exists for wording that reads badly in context. + for (FailureKind kind : FailureKind.values()) { + for (FailureActionId action : kind.getActions()) { + assertThat(kind.labelKeyFor(action)) + .isEqualTo(FailureKind.genericLabelKey(action)); + } + } + } + + @Test + void genericLabelIsDerivedFromTheActionId() { assertThat(FailureKind.UNKNOWN.labelKeyFor(FailureActionId.DISMISS)) .isEqualTo(FailureKind.genericLabelKey(FailureActionId.DISMISS)) .isEqualTo("portal.failures.action.dismiss"); + assertThat( + FailureKind.INPUT_PASSWORD_PROTECTED.labelKeyFor( + FailureActionId.VIEW_IN_PROCESSOR)) + .isEqualTo("portal.failures.action.viewInProcessor"); } @Test diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventControllerTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventControllerTest.java index 58b8d1b408..3ece59f6af 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventControllerTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventControllerTest.java @@ -128,7 +128,9 @@ class FileRunEventControllerTest { } @Test - void carriesActionsAlreadyResolvedForTheRow() { + void carriesActionsAlreadyResolvedForTheRowAndItsReader() { + // A leader reading a colleague's password failure: the unlock is not theirs to do, + // so it is not in the list at all. given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); List actions = @@ -136,10 +138,32 @@ class FileRunEventControllerTest { assertThat(actions) .extracting(FileRunEventView.ActionView::id) - .containsExactlyInAnyOrder("ACKNOWLEDGE", "DISMISS"); + .containsExactly("VIEW_IN_PROCESSOR", "DISMISS"); assertThat(actions).allMatch(FileRunEventView.ActionView::enabled); } + @Test + void carriesEnoughForAClientToRenderAndRouteAnActionItDoesNotKnow() { + // The English fallback, which side runs it, and where the kind wants it: everything a + // build with no copy for a newly shipped action still needs. + given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); + + assertThat(controller.list(null, null, null).events().getFirst().actions()) + .allSatisfy( + action -> { + assertThat(action.defaultLabel()).isNotBlank(); + assertThat(action.execution()).isNotNull(); + }) + .filteredOn(action -> "VIEW_IN_PROCESSOR".equals(action.id())) + .singleElement() + .satisfies( + action -> { + assertThat(action.execution()) + .isEqualTo(FailureActionId.Execution.CLIENT); + assertThat(action.defaultLabel()).isEqualTo("View in processor"); + }); + } + @Test void showsAClosedRowsActionsDisabledWithAReasonRatherThanHidingThem() { // Only visible by asking for the closed status: the default queue drops it. @@ -162,14 +186,18 @@ class FileRunEventControllerTest { void filtersByStatusAndByKind() { FileRunEvent locked = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "locked"); given(FailureKind.UNKNOWN, TEAM, "open"); - controller.act(locked.id(), "ACKNOWLEDGE", null); + controller.act(locked.id(), "DISMISS", null); - assertThat(controller.list(FileRunEventStatus.ACKNOWLEDGED, null, null).events()) - .hasSize(1); - assertThat(controller.list(null, "INPUT_PASSWORD_PROTECTED", null).events()) + assertThat(controller.list(FileRunEventStatus.DISMISSED, null, null).events()) .extracting(FileRunEventView::fileId) .containsExactly("locked"); - // Acknowledged is still open work, so it stays in the default queue. + // A dismissed row is decided, so the default queue holds only the other one. + assertThat(controller.list(null, null, null).events()) + .extracting(FileRunEventView::fileId) + .containsExactly("open"); + assertThat(controller.list(null, "INPUT_PASSWORD_PROTECTED", null).events()) + .extracting(FileRunEventView::fileId) + .isEmpty(); assertThat(controller.list(null, "NO_SUCH_KIND", null).events()).isEmpty(); } @@ -211,12 +239,31 @@ class FileRunEventControllerTest { void appliesADeclaredActionAndReturnsTheUpdatedRow() { FileRunEvent event = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); - FileRunEventView updated = controller.act(event.id(), "ACKNOWLEDGE", null); + FileRunEventView updated = controller.act(event.id(), "DISMISS", null); - assertThat(updated.status()).isEqualTo(FileRunEventStatus.ACKNOWLEDGED); + assertThat(updated.status()).isEqualTo(FileRunEventStatus.DISMISSED); assertThat(updated.statusActor()).isEqualTo("reviewer@example.com"); } + @Test + void anActionTheClientRunsIsABadRequest() { + // Offered, and still not the server's to perform: the document is in the browser. + FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1"); + + assertThat(statusOf(() -> controller.act(event.id(), "VIEW_FILE", null))) + .isEqualTo(HttpStatus.BAD_REQUEST); + } + + @Test + void anActionNoKindOffersAnyMoreIsABadRequest() { + // ACKNOWLEDGE is still in the vocabulary for the rows that carry it, and still not + // something any kind offers, so posting it is refused rather than applied. + FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1"); + + assertThat(statusOf(() -> controller.act(event.id(), "ACKNOWLEDGE", null))) + .isEqualTo(HttpStatus.BAD_REQUEST); + } + @Test void acceptsAnAbsentBodyBecauseTheseActionsNeedNoInput() { FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1"); @@ -238,7 +285,7 @@ class FileRunEventControllerTest { // 404 rather than 403, so the response does not confirm the row exists. FileRunEvent theirs = given(FailureKind.UNKNOWN, 99L, "f1"); - assertThat(statusOf(() -> controller.act(theirs.id(), "ACKNOWLEDGE", null))) + assertThat(statusOf(() -> controller.act(theirs.id(), "DISMISS", null))) .isEqualTo(HttpStatus.NOT_FOUND); } @@ -248,7 +295,7 @@ class FileRunEventControllerTest { FileRunEvent event = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); controller.act(event.id(), "DISMISS", null); - assertThat(statusOf(() -> controller.act(event.id(), "ACKNOWLEDGE", null))) + assertThat(statusOf(() -> controller.act(event.id(), "DISMISS", null))) .isEqualTo(HttpStatus.CONFLICT); } } @@ -276,7 +323,7 @@ class FileRunEventControllerTest { assertThat(locked.actions()) .extracting(FailureKindView.ActionDeclaration::labelKey) - .contains("portal.failures.action.dismissSkipFile"); + .contains("portal.failures.action.viewFile", "portal.failures.action.dismiss"); } @Test diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventHttpIntegrationTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventHttpIntegrationTest.java index 375e46f4ad..ae46a50c8a 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventHttpIntegrationTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventHttpIntegrationTest.java @@ -120,13 +120,20 @@ class FileRunEventHttpIntegrationTest { // Epoch millis, not an ISO string: the client renders relative times from a number. assertThat(row.get("lastSeenAt").isNumber()).isTrue(); + // Resolved for this reader: a leader looking at a colleague's password failure is + // offered the run and a way to close the row, not a password they do not have. JsonNode actions = row.get("actions"); assertThat(actions).hasSize(2); - assertThat(actions.get(0).get("id").asString()).isEqualTo("ACKNOWLEDGE"); + assertThat(actions.get(0).get("id").asString()).isEqualTo("VIEW_IN_PROCESSOR"); assertThat(actions.get(0).get("labelKey").asString()) - .isEqualTo("portal.failures.action.acknowledge"); + .isEqualTo("portal.failures.action.viewInProcessor"); + assertThat(actions.get(0).get("defaultLabel").asString()) + .isEqualTo("View in processor"); + assertThat(actions.get(0).get("execution").asString()).isEqualTo("CLIENT"); assertThat(actions.get(0).get("enabled").asBoolean()).isTrue(); assertThat(actions.get(0).get("disabledReasonKey").isNull()).isTrue(); + assertThat(actions.get(1).get("id").asString()).isEqualTo("DISMISS"); + assertThat(actions.get(1).get("execution").asString()).isEqualTo("SERVER"); } @Test @@ -154,16 +161,17 @@ class FileRunEventHttpIntegrationTest { void coercesQueryParametersAndFiltersOnThem() throws Exception { String locked = seed(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "locked", "b"); seed(FailureKind.UNKNOWN, TEAM, "open", "a"); - post("/api/v1/file-run-events/" + locked + "/actions/ACKNOWLEDGE", "{\"inputs\":{}}"); + post("/api/v1/file-run-events/" + locked + "/actions/DISMISS", "{\"inputs\":{}}"); - JsonNode acknowledged = - mapper.readTree(get("/api/v1/file-run-events?status=ACKNOWLEDGED").body()) + JsonNode dismissed = + mapper.readTree(get("/api/v1/file-run-events?status=DISMISSED").body()) .get("events"); - assertThat(acknowledged).hasSize(1); + assertThat(dismissed).hasSize(1); JsonNode byKind = mapper.readTree( - get("/api/v1/file-run-events?kindId=INPUT_PASSWORD_PROTECTED") + get("/api/v1/file-run-events?status=DISMISSED" + + "&kindId=INPUT_PASSWORD_PROTECTED") .body()) .get("events"); assertThat(byKind).hasSize(1); @@ -269,25 +277,23 @@ class FileRunEventHttpIntegrationTest { String id = seed(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1", "boom"); HttpResponse response = - post( - "/api/v1/file-run-events/" + id + "/actions/ACKNOWLEDGE", - "{\"inputs\":{}}"); + post("/api/v1/file-run-events/" + id + "/actions/DISMISS", "{\"inputs\":{}}"); assertThat(response.statusCode()).isEqualTo(200); JsonNode row = mapper.readTree(response.body()); - assertThat(row.get("status").asString()).isEqualTo("ACKNOWLEDGED"); + assertThat(row.get("status").asString()).isEqualTo("DISMISSED"); assertThat(row.get("statusActor").asString()).isEqualTo(ACTOR); } @Test void acceptsAPopulatedInputsMap() throws Exception { - // Nothing consumes inputs yet, but the shape must bind so the first action that needs - // one (a password) does not discover a broken contract. + // No server action consumes inputs, but the shape must still bind rather than 400, so a + // client that posts an empty or stale map is not refused over its body. String id = seed(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1", "locked"); HttpResponse response = post( - "/api/v1/file-run-events/" + id + "/actions/ACKNOWLEDGE", + "/api/v1/file-run-events/" + id + "/actions/DISMISS", "{\"inputs\":{\"password\":\"hunter2\"}}"); assertThat(response.statusCode()).isEqualTo(200); @@ -315,15 +321,27 @@ class FileRunEventHttpIntegrationTest { .isEqualTo(400); } + @Test + void mapsAnActionTheClientRunsToBadRequest() throws Exception { + // Declared by the kind, refused here: over the wire, so a client that posts a retry + // gets a refusal rather than a 200 implying the server did something. + String id = seed(FailureKind.UNKNOWN, TEAM, "f1", "boom"); + + assertThat( + post( + "/api/v1/file-run-events/" + id + "/actions/VIEW_FILE", + "{\"inputs\":{}}") + .statusCode()) + .isEqualTo(400); + } + @Test void mapsAnotherTeamsRowToNotFound() throws Exception { String id = seed(FailureKind.UNKNOWN, 999L, "theirs", "boom"); assertThat( post( - "/api/v1/file-run-events/" - + id - + "/actions/ACKNOWLEDGE", + "/api/v1/file-run-events/" + id + "/actions/DISMISS", "{\"inputs\":{}}") .statusCode()) .isEqualTo(404); @@ -336,9 +354,7 @@ class FileRunEventHttpIntegrationTest { assertThat( post( - "/api/v1/file-run-events/" - + id - + "/actions/ACKNOWLEDGE", + "/api/v1/file-run-events/" + id + "/actions/DISMISS", "{\"inputs\":{}}") .statusCode()) .isEqualTo(409); diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventServiceTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventServiceTest.java index 4e6508bed9..83ad173637 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventServiceTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventServiceTest.java @@ -1,6 +1,7 @@ package stirling.software.proprietary.failure; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.when; @@ -60,14 +61,20 @@ class FileRunEventServiceTest { } private FileRunEvent given(FailureKind kind, Long teamId, String fileId) { + return givenHitBy("author@example.com", kind, teamId, fileId); + } + + /** As {@link #given} but naming who the incident belongs to, which decides its ownership. */ + private FileRunEvent givenHitBy(String actor, FailureKind kind, Long teamId, String fileId) { return store.record( new RecordFailure( kind, FailureOrigin.POLICY, teamId, - "author@example.com", + actor, "policy-1", - "run-1", + // Distinct per file, so a RUN-scoped kind does not fold two rows into one. + "run-" + fileId, null, fileId, "detail")); @@ -77,11 +84,28 @@ class FileRunEventServiceTest { @DisplayName("acknowledge") class Acknowledge { + /** + * No kind offers it, so it cannot be dispatched; exercised directly for rows that have it. + */ + private FileRunEvent acknowledge(FileRunEvent event, String actor) { + return new AcknowledgeAction(store).execute(event, Map.of(), actor); + } + + @Test + void isNoLongerOfferedSoItCannotBeDispatched() { + FileRunEvent event = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); + + assertThatThrownBy(() -> service.dispatch(event.id(), "ACKNOWLEDGE", Map.of())) + .isInstanceOf(FailureActionException.class) + .extracting(e -> ((FailureActionException) e).getReason()) + .isEqualTo(FailureActionException.Reason.ACTION_NOT_DECLARED); + } + @Test void movesANewEventToAcknowledgedAndStampsTheActor() { FileRunEvent event = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); - FileRunEvent updated = service.dispatch(event.id(), "ACKNOWLEDGE", Map.of()); + FileRunEvent updated = acknowledge(event, ACTOR); assertThat(updated.status()).isEqualTo(FileRunEventStatus.ACKNOWLEDGED); assertThat(updated.statusActor()).isEqualTo(ACTOR); @@ -91,16 +115,24 @@ class FileRunEventServiceTest { @Test void isANoOpWhenAlreadyAcknowledgedSoOwnershipIsNotStolen() { FileRunEvent event = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); - FileRunEvent first = service.dispatch(event.id(), "ACKNOWLEDGE", Map.of()); - Instant originalAt = first.statusAt(); + Instant originalAt = acknowledge(event, ACTOR).statusAt(); - when(userService.getCurrentUsername()).thenReturn("someone-else@example.com"); - FileRunEvent second = service.dispatch(event.id(), "ACKNOWLEDGE", Map.of()); + FileRunEvent second = acknowledge(event, "someone-else@example.com"); assertThat(second.status()).isEqualTo(FileRunEventStatus.ACKNOWLEDGED); assertThat(second.statusActor()).isEqualTo(ACTOR); assertThat(second.statusAt()).isEqualTo(originalAt); } + + @Test + void anAlreadyAcknowledgedRowStaysReadableAndClosable() { + FileRunEvent event = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); + acknowledge(event, ACTOR); + + assertThat(service.list(FileRunEventStatus.ACKNOWLEDGED, null, 10)).hasSize(1); + assertThat(service.dispatch(event.id(), "DISMISS", Map.of()).status()) + .isEqualTo(FileRunEventStatus.DISMISSED); + } } @Nested @@ -118,7 +150,7 @@ class FileRunEventServiceTest { @Test void closesAnAcknowledgedEvent() { FileRunEvent event = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); - service.dispatch(event.id(), "ACKNOWLEDGE", Map.of()); + new AcknowledgeAction(store).execute(event, Map.of(), ACTOR); assertThat(service.dispatch(event.id(), "DISMISS", Map.of()).status()) .isEqualTo(FileRunEventStatus.DISMISSED); @@ -180,7 +212,7 @@ class FileRunEventServiceTest { void anotherTeamsEventIsNotFound() { FileRunEvent theirs = given(FailureKind.UNKNOWN, 99L, "f1"); - assertThatThrownBy(() -> service.dispatch(theirs.id(), "ACKNOWLEDGE", Map.of())) + assertThatThrownBy(() -> service.dispatch(theirs.id(), "DISMISS", Map.of())) .isInstanceOf(FailureActionException.class) .extracting(e -> ((FailureActionException) e).getReason()) .isEqualTo(FailureActionException.Reason.EVENT_NOT_FOUND); @@ -188,12 +220,44 @@ class FileRunEventServiceTest { @Test void anUnknownEventIdIsNotFound() { - assertThatThrownBy(() -> service.dispatch("nope", "ACKNOWLEDGE", Map.of())) + assertThatThrownBy(() -> service.dispatch("nope", "DISMISS", Map.of())) .isInstanceOf(FailureActionException.class) .extracting(e -> ((FailureActionException) e).getReason()) .isEqualTo(FailureActionException.Reason.EVENT_NOT_FOUND); } + @Test + void anActionTheClientRunsIsRefusedRatherThanPretendedTo() { + // Answering 200 would tell the client something happened when nothing did. + FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1"); + + assertThatThrownBy(() -> service.dispatch(event.id(), "VIEW_FILE", Map.of())) + .isInstanceOf(FailureActionException.class) + .extracting(e -> ((FailureActionException) e).getReason()) + .isEqualTo(FailureActionException.Reason.ACTION_NOT_DISPATCHABLE); + + assertThat(store.find(event.id(), TEAM).orElseThrow().status()) + .isEqualTo(FileRunEventStatus.NEW); + } + + @Test + void everyClientActionIsRefusedWhicheverKindDeclaresIt() { + // Over the whole vocabulary, so a client action added later cannot arrive dispatchable. + for (FailureKind kind : FailureKind.values()) { + FileRunEvent event = given(kind, TEAM, "f-" + kind.getId()); + for (FailureActionId action : kind.getActions()) { + if (action.runsOnServer()) { + continue; + } + assertThatThrownBy(() -> service.dispatch(event.id(), action.name(), Map.of())) + .as("%s offers %s", kind.getId(), action) + .isInstanceOf(FailureActionException.class) + .extracting(e -> ((FailureActionException) e).getReason()) + .isEqualTo(FailureActionException.Reason.ACTION_NOT_DISPATCHABLE); + } + } + } + @Test void anUnknownActionIdIsRejected() { FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1"); @@ -231,11 +295,6 @@ class FileRunEventServiceTest { FileRunEvent event = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); service.dispatch(event.id(), "DISMISS", Map.of()); - assertThatThrownBy(() -> service.dispatch(event.id(), "ACKNOWLEDGE", Map.of())) - .isInstanceOf(FailureActionException.class) - .extracting(e -> ((FailureActionException) e).getReason()) - .isEqualTo(FailureActionException.Reason.ALREADY_CLOSED); - assertThatThrownBy(() -> service.dispatch(event.id(), "DISMISS", Map.of())) .isInstanceOf(FailureActionException.class) .extracting(e -> ((FailureActionException) e).getReason()) @@ -244,18 +303,184 @@ class FileRunEventServiceTest { } @Nested - @DisplayName("available actions are resolved per row") - class Availability { + @DisplayName("ownership is derived against whoever is reading") + class OwnershipDerivation { @Test - void openRowOffersEveryDeclaredActionEnabled() { - FileRunEvent event = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); + void theCallersOwnFailureIsMine() { + FileRunEvent mine = givenHitBy(ACTOR, FailureKind.UNKNOWN, TEAM, "f1"); - List actions = service.availableActions(event); + assertThat(service.ownershipOf(mine)).isEqualTo(Ownership.MINE); + } - assertThat(actions).hasSize(2); - assertThat(actions).allMatch(FileRunEventService.AvailableAction::enabled); - assertThat(actions).allMatch(action -> action.disabledReasonKey() == null); + @Test + void aColleaguesIsTheirs() { + FileRunEvent theirs = + givenHitBy("colleague@example.com", FailureKind.UNKNOWN, TEAM, "f1"); + + assertThat(service.ownershipOf(theirs)).isEqualTo(Ownership.THEIRS); + } + + @Test + void anUnattendedRunsIsNobodys() { + // A trigger-fired run has no user to name, so there is nobody to hand the fix to. + FileRunEvent unattended = givenHitBy(null, FailureKind.UNKNOWN, TEAM, "f1"); + + assertThat(service.ownershipOf(unattended)).isEqualTo(Ownership.UNOWNED); + } + + @Test + void theSameRowIsMineToOnePersonAndTheirsToAnother() { + // Why it is derived: a stored answer would be wrong for everyone but one person. + FileRunEvent event = givenHitBy(ACTOR, FailureKind.UNKNOWN, TEAM, "f1"); + assertThat(service.ownershipOf(event)).isEqualTo(Ownership.MINE); + + when(userService.getCurrentUsername()).thenReturn("colleague@example.com"); + + assertThat(service.ownershipOf(event)).isEqualTo(Ownership.THEIRS); + } + } + + @Nested + @DisplayName("available actions are resolved per row and per reader") + class Availability { + + private List offeredFor(FileRunEvent event) { + return service.availableActions(event).stream() + .map(FileRunEventService.AvailableAction::id) + .toList(); + } + + @Test + void theOwnerIsOfferedTheirDocumentAndNotTheReviewersView() { + // The document is theirs to open; the processor view is for whoever reviews the team. + when(authority.canEditPolicies()).thenReturn(false); + FileRunEvent mine = givenHitBy(ACTOR, FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); + + assertThat(offeredFor(mine)) + .containsExactly(FailureActionId.VIEW_FILE, FailureActionId.DISMISS); + assertThat(service.availableActions(mine)) + .allMatch(FileRunEventService.AvailableAction::enabled); + } + + @Test + void aReviewerReadingAColleaguesIsNotOfferedTheDocumentTheyDoNotHave() { + // Dropped, not disabled: greyed out would read as their permission problem. + FileRunEvent theirs = + givenHitBy( + "colleague@example.com", + FailureKind.INPUT_PASSWORD_PROTECTED, + TEAM, + "f1"); + + assertThat(offeredFor(theirs)) + .containsExactly(FailureActionId.VIEW_IN_PROCESSOR, FailureActionId.DISMISS); + } + + @Test + void aReviewerInheritsTheOwnerActionsOnAnUnattendedRow() { + // Nobody owns it, so without the inheritance the row could only ever be dismissed. + FileRunEvent unattended = + givenHitBy(null, FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); + + assertThat(offeredFor(unattended)) + .containsExactly( + FailureActionId.VIEW_FILE, + FailureActionId.VIEW_IN_PROCESSOR, + FailureActionId.DISMISS); + } + + @Test + void inheritedOwnerActionsComeBackDisabledWithTheReasonWhy() { + // No browser holds a source-fed file, so it is stated rather than offered as a dead + // button. + FileRunEvent unattended = + givenHitBy(null, FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); + + assertThat(service.availableActions(unattended)) + .filteredOn(action -> action.id() != FailureActionId.DISMISS) + .filteredOn(action -> action.id() != FailureActionId.VIEW_IN_PROCESSOR) + .isNotEmpty() + .allSatisfy( + action -> { + assertThat(action.enabled()).isFalse(); + assertThat(action.disabledReasonKey()) + .isEqualTo("portal.failures.disabled.unattended"); + }); + } + + @Test + void theReviewersOwnActionsStayUsableOnAnUnattendedRow() { + FileRunEvent unattended = + givenHitBy(null, FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); + + assertThat(service.availableActions(unattended)) + .filteredOn( + action -> + action.id() == FailureActionId.DISMISS + || action.id() == FailureActionId.VIEW_IN_PROCESSOR) + .hasSize(2) + .allMatch(FileRunEventService.AvailableAction::enabled); + } + + @Test + void theOwnersActionsAreDisabledWhenTheRowNamesNoDocument() { + // Answered here, or the client calls it "not on this device" while it sits in their + // own workbench. + FileRunEvent documentless = + givenHitBy(ACTOR, FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, null); + + assertThat(service.ownershipOf(documentless)).isEqualTo(Ownership.MINE); + assertThat(service.availableActions(documentless)) + .filteredOn(action -> action.id() != FailureActionId.DISMISS) + .filteredOn(action -> action.id() != FailureActionId.VIEW_IN_PROCESSOR) + .isNotEmpty() + .allSatisfy( + action -> { + assertThat(action.enabled()).isFalse(); + assertThat(action.disabledReasonKey()) + .isEqualTo("portal.failures.disabled.noDocument"); + }); + } + + @Test + void aRowThatNamesADocumentKeepsItsOwnerActionsUsable() { + FileRunEvent withDocument = + givenHitBy(ACTOR, FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); + + assertThat(service.availableActions(withDocument)) + .isNotEmpty() + .allMatch(FileRunEventService.AvailableAction::enabled); + } + + @Test + void aMemberIsNotOfferedTheOwnerActionsOnAnUnattendedRow() { + // The inheritance is the reviewer's: a member has no claim on a run nobody attended. + when(authority.canEditPolicies()).thenReturn(false); + FileRunEvent unattended = + givenHitBy(null, FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); + + assertThat(offeredFor(unattended)).containsExactly(FailureActionId.DISMISS); + } + + @Test + void aLoginDisabledOperatorKeepsTheirOwnActions() { + // Unowned for want of users, not because nothing attended: the one operator holds the + // file. + ApplicationProperties props = new ApplicationProperties(); + props.getSecurity().setEnableLogin(false); + FileRunEventService unsecured = + new FileRunEventService( + store, + new FailureActionRegistry(List.of(new DismissAction(store))), + authority, + userService, + props); + FileRunEvent event = givenHitBy(null, FailureKind.INPUT_PASSWORD_PROTECTED, null, "f1"); + + assertThat(unsecured.availableActions(event)) + .extracting(FileRunEventService.AvailableAction::enabled) + .containsOnly(true); } @Test @@ -275,21 +500,14 @@ class FileRunEventServiceTest { } @Test - void carriesTheKindsOverriddenLabelWhereItHasOne() { - FileRunEvent event = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); - - assertThat(service.availableActions(event)) - .extracting(FileRunEventService.AvailableAction::labelKey) - .contains("portal.failures.action.dismissSkipFile"); - } - - @Test - void fallsBackToTheGenericLabelWhereTheKindDeclaresNoOverride() { + void carriesTheLabelKeyForEachOffer() { FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1"); assertThat(service.availableActions(event)) .extracting(FileRunEventService.AvailableAction::labelKey) - .containsExactly("portal.failures.action.dismiss"); + .containsExactly( + "portal.failures.action.viewInProcessor", + "portal.failures.action.dismiss"); } } @@ -450,7 +668,43 @@ class FileRunEventServiceTest { complete.verifyEveryDeclaredActionHasAHandler(); for (FailureActionId id : FailureActionId.values()) { - assertThat(complete.find(id)).isPresent(); + // Only server actions need a handler, which is why the boot check ignores the rest. + assertThat(complete.find(id).isPresent()).isEqualTo(id.runsOnServer()); + } + } + + @Test + void doesNotAskForAHandlerForAnActionTheClientRuns() { + // Otherwise every client action would need an empty handler beside it. + FailureActionRegistry serverOnly = + new FailureActionRegistry( + List.of(new AcknowledgeAction(store), new DismissAction(store))); + + assertThatCode(serverOnly::verifyEveryDeclaredActionHasAHandler) + .doesNotThrowAnyException(); + } + + @Test + void refusesAHandlerForAnActionTheClientRuns() { + // Dispatch refuses the id before resolving a handler, so the bean reads as live and is + // not. + assertThatThrownBy(() -> new FailureActionRegistry(List.of(new ClientSideAction()))) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("VIEW_FILE"); + } + + /** A handler for a client action, which is exactly what must not be registered. */ + private static final class ClientSideAction implements FailureAction { + + @Override + public FailureActionId id() { + return FailureActionId.VIEW_FILE; + } + + @Override + public FileRunEvent execute( + FileRunEvent event, Map inputs, String actor) { + throw new UnsupportedOperationException(); } } } diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventStoreDbTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventStoreDbTest.java index 982d804354..af1353477d 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventStoreDbTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventStoreDbTest.java @@ -233,7 +233,7 @@ class FileRunEventStoreDbTest { } @Test - @DisplayName("closing deleted files touches only that owner's own open editor rows") + @DisplayName("deleting a document closes every incident about it that the deleter caused") void markFilesRemovedIsScopedBySqlNotByTheCaller() { // The scoping is entirely in the JPQL, so the in-memory fake proves nothing about it: // it implements the same rules by hand and would agree with a wrong query. @@ -241,6 +241,10 @@ class FileRunEventStoreDbTest { store.record( RecordFailure.forEditor( FailureKind.UNKNOWN, TEAM, "owner@example.com", "f-1", "boom")); + // Recorded by the processor, about the document they just deleted. Keying on origin left + // these in the queue. + FileRunEvent myPolicyRun = + store.record(failure(FailureKind.UNKNOWN, TEAM, "owner@example.com", "f-1")); FileRunEvent theirs = store.record( RecordFailure.forEditor( @@ -253,21 +257,45 @@ class FileRunEventStoreDbTest { "owner@example.com", "f-1", "boom")); - FileRunEvent fromProcessor = store.record(failure(FailureKind.UNKNOWN, TEAM, "f-1")); int closed = store.markFilesRemoved(TEAM, "owner@example.com", List.of("f-1")); - assertThat(closed).isEqualTo(1); + assertThat(closed).isEqualTo(2); assertThat(store.find(mine.id(), TEAM).orElseThrow().status()) .isEqualTo(FileRunEventStatus.FILE_REMOVED); + assertThat(store.find(myPolicyRun.id(), TEAM).orElseThrow().status()) + .as("their upload, their document, now deleted") + .isEqualTo(FileRunEventStatus.FILE_REMOVED); assertThat(store.find(theirs.id(), TEAM).orElseThrow().status()) .as("another person's incident about their own file") .isEqualTo(FileRunEventStatus.NEW); assertThat(store.find(otherTeam.id(), OTHER_TEAM).orElseThrow().status()) .as("another team entirely") .isEqualTo(FileRunEventStatus.NEW); - assertThat(store.find(fromProcessor.id(), TEAM).orElseThrow().status()) - .as("nothing was deleted from an editor here") + } + + @Test + @DisplayName("a source-fed incident survives a client naming its file id") + void markFilesRemovedLeavesSourceFedRowsAlone() { + // With login disabled the actor is null on both sides, so the absence of a source is all + // that stands between a local delete and a sweep's incidents. + FileRunEvent sweep = + store.record( + new RecordFailure( + FailureKind.UNKNOWN, + FailureOrigin.POLICY, + null, + null, + "policy-1", + "run-1", + "src-watched-folder", + "collides-with-a-client-id", + "detail")); + + int closed = store.markFilesRemoved(null, null, List.of("collides-with-a-client-id")); + + assertThat(closed).isZero(); + assertThat(store.find(sweep.id(), null).orElseThrow().status()) .isEqualTo(FileRunEventStatus.NEW); } diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/InMemoryFileRunEventRepository.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/InMemoryFileRunEventRepository.java index 6dbb16322b..4f429fe9b1 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/InMemoryFileRunEventRepository.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/InMemoryFileRunEventRepository.java @@ -134,7 +134,8 @@ class InMemoryFileRunEventRepository implements FileRunEventRepository { Collection allowedFrom) { int closed = 0; for (FileRunEventEntity entity : rows.values()) { - if (entity.getOrigin() != FailureOrigin.TOOL + // Mirrors the real query: scoped by the absence of a source, not by origin. + if (entity.getSourceId() != null || !sameTeam(entity, teamId) || !Objects.equals(entity.getActor(), actor) || entity.getFileId() == null diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/NotificationProjectionTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/NotificationProjectionTest.java new file mode 100644 index 0000000000..e76a8b96ee --- /dev/null +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/NotificationProjectionTest.java @@ -0,0 +1,168 @@ +package stirling.software.proprietary.failure; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.lenient; + +import java.util.List; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import stirling.software.common.model.ApplicationProperties; +import stirling.software.common.service.UserServiceInterface; +import stirling.software.proprietary.notification.NotificationController; +import stirling.software.proprietary.notification.NotificationService; +import stirling.software.proprietary.notification.NotificationSource; +import stirling.software.proprietary.notification.NotificationView; +import stirling.software.proprietary.policy.config.PolicyManagementAuthority; + +/** + * What the bell is given to render: never a raw event id, and only the actions the client itself + * runs, resolved for this reader by the same service that scopes the queue. + */ +@ExtendWith(MockitoExtension.class) +class NotificationProjectionTest { + + private static final Long TEAM = 7L; + private static final String ACTOR = "reviewer@example.com"; + + @Mock private PolicyManagementAuthority authority; + @Mock private UserServiceInterface userService; + + private FileRunEventStore store; + private FileRunEventService failures; + private NotificationController controller; + + @BeforeEach + void setUp() { + ApplicationProperties props = new ApplicationProperties(); + props.getSecurity().setEnableLogin(true); + store = new FileRunEventStore(new InMemoryFileRunEventRepository()); + failures = + new FileRunEventService( + store, + new FailureActionRegistry( + List.of(new AcknowledgeAction(store), new DismissAction(store))), + authority, + userService, + props); + controller = new NotificationController(new NotificationService(failures)); + + lenient().when(authority.currentUserTeamId()).thenReturn(TEAM); + lenient().when(authority.canEditPolicies()).thenReturn(true); + lenient().when(userService.getCurrentUsername()).thenReturn(ACTOR); + } + + private FileRunEvent given(FailureKind kind, String actor, String fileId) { + return store.record(RecordFailure.forEditor(kind, TEAM, actor, fileId, "boom")); + } + + @Nested + @DisplayName("the bell holds a prefixed id and nothing else") + class Ids { + + @Test + void everyNotificationIsKeyedByItsSourceAndRowId() { + FileRunEvent event = given(FailureKind.UNKNOWN, ACTOR, "f-1"); + + NotificationView notification = controller.list(null).notifications().getFirst(); + + assertThat(notification.id()).isEqualTo("failure:" + event.id()); + assertThat(notification.source()).isEqualTo(NotificationSource.FAILURE); + } + } + + @Nested + @DisplayName("what the bell is given to render") + class Projection { + + @Test + void carriesTheKindOriginOwnershipAndTheQueuesClientActions() { + FileRunEvent mine = given(FailureKind.INPUT_PASSWORD_PROTECTED, ACTOR, "f-1"); + + NotificationView notification = controller.list(null).notifications().getFirst(); + + assertThat(notification.kindId()).isEqualTo("INPUT_PASSWORD_PROTECTED"); + assertThat(notification.origin()).isEqualTo(FailureOrigin.TOOL); + assertThat(notification.ownership()).isEqualTo(Ownership.MINE); + assertThat(notification.severity()).isEqualTo(FailureSeverity.ERROR); + assertThat(notification.status()).isEqualTo(FileRunEventStatus.NEW); + assertThat(notification.fileId()).isEqualTo("f-1"); + assertThat(notification.policyId()).isNull(); + // How the client knows the fileId above is one of its own and worth looking up. + assertThat(notification.sourceId()).isNull(); + assertThat(notification.defaultTitle()).isNotBlank(); + // The queue's own offers minus the server's: a bell offering different ones would lie. + assertThat(notification.actions()) + .containsExactlyElementsOf( + FileRunEventView.of(mine, failures.availableActions(mine)) + .actions() + .stream() + .filter( + action -> + action.execution() + == FailureActionId.Execution.CLIENT) + .toList()); + } + + @Test + void offersNoActionTheServerRunsBecauseDispositionsBelongToTheQueue() { + // Deciding a failure's fate belongs to the review surface, not the panel. + given(FailureKind.INPUT_PASSWORD_PROTECTED, ACTOR, "f-1"); + + assertThat(controller.list(null).notifications().getFirst().actions()) + .isNotEmpty() + .allMatch(action -> action.execution() == FailureActionId.Execution.CLIENT); + } + + @Test + void namesTheSourceThatFedAnUnattendedRunSoItsFileIdIsNotMistakenForAClientsOwn() { + // Without the source a client looks up a hash it can never resolve and calls it + // missing. + store.record( + RecordFailure.forRun( + FailureKind.INPUT_PASSWORD_PROTECTED, + TEAM, + null, + "policy-1", + "run-1", + "source-7", + "hashed-identity", + "boom")); + + NotificationView notification = controller.list(null).notifications().getFirst(); + + assertThat(notification.sourceId()).isEqualTo("source-7"); + assertThat(notification.fileId()).isEqualTo("hashed-identity"); + } + + @Test + void aColleaguesNotificationOffersTheReviewersActionsOnly() { + // A leader sees the team's failures, so audience filtering has to reach the bell too. + given(FailureKind.INPUT_PASSWORD_PROTECTED, "colleague@example.com", "f-1"); + + assertThat(controller.list(null).notifications().getFirst().actions()) + .extracting(FileRunEventView.ActionView::id) + .containsExactly("VIEW_IN_PROCESSOR"); + } + + @Test + void carriesWhatAClientNeedsToRenderAnActionItDoesNotKnow() { + given(FailureKind.INPUT_PASSWORD_PROTECTED, ACTOR, "f-1"); + + assertThat(controller.list(null).notifications().getFirst().actions()) + .isNotEmpty() + .allSatisfy( + action -> { + assertThat(action.labelKey()).startsWith("portal.failures.action."); + assertThat(action.defaultLabel()).isNotBlank(); + assertThat(action.execution()).isNotNull(); + }); + } + } +} diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/PolicyFailureOwnershipTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/PolicyFailureOwnershipTest.java new file mode 100644 index 0000000000..0ae6c3a945 --- /dev/null +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/PolicyFailureOwnershipTest.java @@ -0,0 +1,272 @@ +package stirling.software.proprietary.failure; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.when; + +import java.nio.file.Path; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.TimeUnit; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.api.io.TempDir; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.slf4j.MDC; +import org.springframework.core.io.ByteArrayResource; + +import stirling.software.common.model.ApplicationProperties; +import stirling.software.common.service.FileStorage; +import stirling.software.common.service.InternalApiClient; +import stirling.software.common.service.JobOwnershipService; +import stirling.software.common.service.JobQueue; +import stirling.software.common.service.ResourceMonitor; +import stirling.software.common.service.TaskManager; +import stirling.software.common.service.ToolMetadataService; +import stirling.software.common.service.UserServiceInterface; +import stirling.software.common.util.TempFileManager; +import stirling.software.common.util.TempFileRegistry; +import stirling.software.proprietary.policy.asset.InProcessPolicyAssetStore; +import stirling.software.proprietary.policy.asset.PolicyAssetResolver; +import stirling.software.proprietary.policy.config.PolicyManagementAuthority; +import stirling.software.proprietary.policy.engine.PolicyEngine; +import stirling.software.proprietary.policy.engine.PolicyExecutor; +import stirling.software.proprietary.policy.engine.PolicyRunRegistry; +import stirling.software.proprietary.policy.model.OutputSpec; +import stirling.software.proprietary.policy.model.PipelineStep; +import stirling.software.proprietary.policy.model.Policy; +import stirling.software.proprietary.policy.model.PolicyInputs; +import stirling.software.proprietary.policy.output.InlineOutputSink; +import stirling.software.proprietary.policy.output.PolicyOutputResolver; +import stirling.software.proprietary.policy.progress.PolicyProgressListener; +import stirling.software.proprietary.policy.source.InProcessSourceStore; +import stirling.software.proprietary.policy.store.PolicyStore; + +import tools.jackson.databind.json.JsonMapper; + +/** + * What a reader is offered on a real recorded row, every collaborator being the real one. Both + * directions are asserted: offered to the wrong reader is either a dead button or a leaked + * document. + */ +@ExtendWith(MockitoExtension.class) +class PolicyFailureOwnershipTest { + + private static final String ROTATE = "/api/v1/general/rotate-pdf"; + private static final Long TEAM = 3L; + + @Mock private InternalApiClient internalApiClient; + @Mock private ToolMetadataService toolMetadataService; + @Mock private TaskManager taskManager; + @Mock private FileStorage fileStorage; + @Mock private JobOwnershipService jobOwnershipService; + @Mock private ResourceMonitor resourceMonitor; + @Mock private JobQueue jobQueue; + @Mock private PolicyStore policyStore; + @Mock private PolicyManagementAuthority authority; + @Mock private UserServiceInterface userService; + + @TempDir Path tempDir; + + private PolicyEngine engine; + private FileRunEventService service; + + @BeforeEach + void setUp() { + ApplicationProperties props = new ApplicationProperties(); + props.getSecurity().setEnableLogin(true); + props.getSystem().getTempFileManagement().setBaseTmpDir(tempDir.toString()); + props.getSystem().getTempFileManagement().setPrefix("failure-ownership-test-"); + + FileRunEventStore store = new FileRunEventStore(new InMemoryFileRunEventRepository()); + service = + new FileRunEventService( + store, + new FailureActionRegistry( + List.of(new AcknowledgeAction(store), new DismissAction(store))), + authority, + userService, + props); + + PolicyFailureRecorder recorder = + new PolicyFailureRecorder( + new FailureClassifier(JsonMapper.builder().build()), store, policyStore); + PolicyExecutor executor = + new PolicyExecutor( + internalApiClient, + toolMetadataService, + new TempFileManager(new TempFileRegistry(), props), + JsonMapper.builder().build()); + engine = + new PolicyEngine( + executor, + taskManager, + new PolicyRunRegistry(new ApplicationProperties()), + recorder, + fileStorage, + jobOwnershipService, + List.of(new InlineOutputSink(fileStorage)), + new PolicyOutputResolver(new InProcessSourceStore()), + resourceMonitor, + jobQueue, + new PolicyAssetResolver(new InProcessPolicyAssetStore())); + + lenient() + .when(jobOwnershipService.createScopedJobKey(anyString())) + .thenAnswer(invocation -> invocation.getArgument(0)); + lenient().when(resourceMonitor.shouldQueueJob(anyInt())).thenReturn(false); + lenient().when(toolMetadataService.isMultiInput(anyString())).thenReturn(false); + // The team is resolved from the policy, so the recorded row lands in the reader's team. + lenient().when(policyStore.get(anyString())).thenReturn(Optional.of(sharedPolicy())); + lenient().when(authority.currentUserTeamId()).thenReturn(TEAM); + } + + /** Alice's policy, shared with her team. Bob is a member of it and does not own it. */ + private static Policy sharedPolicy() { + return new Policy( + "p1", + "rotate", + "alice", + true, + List.of(), + List.of(new PipelineStep(ROTATE, Map.of())), + OutputSpec.inline(), + TEAM); + } + + /** Fails the policy's single tool step as {@code triggeredBy} (null = sweep). */ + private void runAndFail(String triggeredBy, String sourceId, String fileIdentity) + throws Exception { + when(internalApiClient.post(eq(ROTATE), any())).thenThrow(new RuntimeException("boom")); + if (triggeredBy != null) { + MDC.put("auditPrincipal", triggeredBy); + } + try { + engine.runPolicy( + sharedPolicy(), + PolicyInputs.of(List.of(pdf())), + PolicyProgressListener.NOOP, + sourceId, + fileIdentity) + .completion() + .get(10, TimeUnit.SECONDS); + } finally { + MDC.remove("auditPrincipal"); + } + } + + private static ByteArrayResource pdf() { + return new ByteArrayResource("input".getBytes()) { + @Override + public String getFilename() { + return "input.pdf"; + } + }; + } + + /** + * Lenient because a leader's scope and an UNOWNED check both answer without asking who reads, + * so whether the name is consulted is the behaviour under test. + */ + private FileRunEvent asMember(String reader) { + lenient().when(userService.getCurrentUsername()).thenReturn(reader); + lenient().when(authority.canEditPolicies()).thenReturn(false); + List visible = service.list(null, null, 10); + return visible.isEmpty() ? null : visible.getFirst(); + } + + /** Read as a team leader, who reviews the whole team's incidents. See {@link #asMember}. */ + private FileRunEvent asReviewer(String reader) { + lenient().when(userService.getCurrentUsername()).thenReturn(reader); + lenient().when(authority.canEditPolicies()).thenReturn(true); + return service.list(null, null, 10).getFirst(); + } + + private List offeredTo(FileRunEvent event) { + return service.availableActions(event).stream() + .map(FileRunEventService.AvailableAction::id) + .toList(); + } + + @Nested + @DisplayName("a non-owner runs a shared policy on their own upload") + class AttendedByANonOwner { + + @Test + void theTriggeringUserHoldsItAndIsOfferedTheDocument() throws Exception { + runAndFail("bob", null, "bob-doc-1"); + + FileRunEvent mine = asMember("bob"); + assertThat(service.ownershipOf(mine)).isEqualTo(Ownership.MINE); + assertThat(offeredTo(mine)) + .as("he is holding the document, so opening it is his to do") + .contains(FailureActionId.VIEW_FILE); + assertThat(service.availableActions(mine)) + .filteredOn(action -> action.id() == FailureActionId.VIEW_FILE) + .singleElement() + .satisfies(action -> assertThat(action.enabled()).isTrue()); + } + + @Test + void thePolicyOwnerIsNotHandedADocumentSheNeverTouched() throws Exception { + runAndFail("bob", null, "bob-doc-1"); + + // She owns the policy and pays for the run, and still has no copy of Bob's file. + FileRunEvent theirs = asReviewer("alice"); + assertThat(service.ownershipOf(theirs)).isEqualTo(Ownership.THEIRS); + assertThat(offeredTo(theirs)).doesNotContain(FailureActionId.VIEW_FILE); + } + + @Test + void theReviewerIsStillOfferedWhatReviewingNeeds() throws Exception { + runAndFail("bob", null, "bob-doc-1"); + + // Not her document, still her team's incident. + assertThat(offeredTo(asReviewer("alice"))) + .contains(FailureActionId.VIEW_IN_PROCESSOR, FailureActionId.DISMISS); + } + } + + @Nested + @DisplayName("an unattended sweep pulls a file from a source") + class UnattendedSweep { + + @Test + void theRowIsOwnedByNobodySoTheReviewerInheritsTheOwnerActions() throws Exception { + runAndFail(null, "src-watched-folder", "file-hash-1"); + + FileRunEvent unattended = asReviewer("alice"); + assertThat(service.ownershipOf(unattended)).isEqualTo(Ownership.UNOWNED); + // No browser holds this document, so the offer is stated and disabled, not dropped. + assertThat(offeredTo(unattended)).contains(FailureActionId.VIEW_FILE); + assertThat(service.availableActions(unattended)) + .filteredOn(action -> action.id() == FailureActionId.VIEW_FILE) + .singleElement() + .satisfies( + action -> { + assertThat(action.enabled()).isFalse(); + assertThat(action.disabledReasonKey()) + .isEqualTo("portal.failures.disabled.unattended"); + }); + } + + @Test + void thePolicyOwnerDoesNotInheritItAsHerOwn() throws Exception { + // Being billed for the sweep must not become ownership: she gets these as reviewer + // only. + runAndFail(null, "src-watched-folder", "file-hash-1"); + + assertThat(service.ownershipOf(asReviewer("alice"))).isNotEqualTo(Ownership.MINE); + } + } +} diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/PolicyFailureRecorderTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/PolicyFailureRecorderTest.java index 73c5add6ad..7472379d36 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/PolicyFailureRecorderTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/PolicyFailureRecorderTest.java @@ -39,6 +39,7 @@ import tools.jackson.databind.json.JsonMapper; class PolicyFailureRecorderTest { private static final Long TEAM = 11L; + private static final String ACTOR = "dana@example.com"; @Mock private PolicyStore policyStore; @@ -379,5 +380,36 @@ class PolicyFailureRecorderTest { assertThat(store.list(TEAM, null, null, null, 10)).hasSize(2); } + + @Test + void theSameDocumentFailingInTwoAttendedRunsIsOneIncident() { + // Every upload is a new run, so with no reference the run id stands in for the document + // and the same broken file reads as a second incident rather than a second occurrence. + when(policyStore.get("policy-1")).thenReturn(Optional.of(policy("policy-1", TEAM))); + + recorder.recordRunFailure( + "run-1", "policy-1", null, "editor-file-1", ACTOR, "locked", passwordFailure()); + recorder.recordRunFailure( + "run-2", "policy-1", null, "editor-file-1", ACTOR, "locked", passwordFailure()); + + List events = store.list(TEAM, null, null, null, 10); + assertThat(events).hasSize(1); + assertThat(events.getFirst().occurrences()).isEqualTo(2); + } + + @Test + void twoDocumentsFailingTheSameWayStaySeparateIncidents() { + // Folding is per document, so neither row is credited with the other's occurrence. + when(policyStore.get("policy-1")).thenReturn(Optional.of(policy("policy-1", TEAM))); + + recorder.recordRunFailure( + "run-1", "policy-1", null, "editor-file-1", ACTOR, "locked", passwordFailure()); + recorder.recordRunFailure( + "run-2", "policy-1", null, "editor-file-2", ACTOR, "locked", passwordFailure()); + + assertThat(store.list(TEAM, null, null, null, 10)) + .hasSize(2) + .allMatch(event -> event.occurrences() == 1); + } } } diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/RecordFailurePrivacyTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/RecordFailurePrivacyTest.java index 86c52c4007..c876644133 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/RecordFailurePrivacyTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/RecordFailurePrivacyTest.java @@ -11,6 +11,8 @@ import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import stirling.software.proprietary.policy.controller.PolicyRunFiles; + /** * The privacy contract: a recorded failure carries no document identity of its own. There is no * name column and the dedup key is built only from opaque ids, so nothing here derives from what a @@ -53,6 +55,16 @@ class RecordFailurePrivacyTest { .doesNotContain("fileName"); } + @Test + void theRunRequestThatSuppliesADocumentReferenceCarriesNoNameEither() { + // The same discipline at the door as in the row: an id and nothing else, or a document name + // reaches a table that deliberately has nowhere to put it. + assertThat(List.of(PolicyRunFiles.class.getDeclaredFields())) + .extracting(Field::getName) + .contains("fileId") + .doesNotContain("fileName", "documentName", "name"); + } + @Test void dedupKeyIsBuiltOnlyFromOpaqueIdentifiers() { // Two files under the same policy hash differently (so they stay separate incidents), but diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/policy/controller/PolicyControllerTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/policy/controller/PolicyControllerTest.java index 36f5cc221b..c72a30f98a 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/policy/controller/PolicyControllerTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/policy/controller/PolicyControllerTest.java @@ -29,6 +29,7 @@ import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; +import org.springframework.mock.web.MockMultipartFile; import org.springframework.web.server.ResponseStatusException; import org.springframework.web.servlet.mvc.method.annotation.SseEmitter; @@ -39,6 +40,7 @@ import stirling.software.common.model.job.JobResponse; import stirling.software.common.model.tool.ToolDiagnostic; import stirling.software.common.service.JobOwnershipService; import stirling.software.common.util.TempFileManager; +import stirling.software.common.util.TempFileRegistry; import stirling.software.proprietary.policy.config.PolicyAccessGuard; import stirling.software.proprietary.policy.config.PolicyManagementAuthority; import stirling.software.proprietary.policy.engine.PolicyRunHandle; @@ -87,7 +89,10 @@ class PolicyControllerTest { @Mock private ProcessedLedger processedLedger; - @Mock private TempFileManager tempFileManager; + // Real, not mocked: the run endpoints spool uploads through it. + private final TempFileManager tempFileManager = + new TempFileManager(new TempFileRegistry(), new ApplicationProperties()); + @Mock private JobOwnershipService jobOwnershipService; private ApplicationProperties applicationProperties; @@ -700,13 +705,46 @@ class PolicyControllerTest { @DisplayName("runStoredPolicy") class RunStoredPolicy { + /** What an editor sends: the documents, plus its own id for a single one of them. */ + private PolicyRunFiles filesWith(String fileId, int documents) { + PolicyRunFiles files = new PolicyRunFiles(); + files.setFileId(fileId); + files.setFileInput( + java.util.stream.IntStream.range(0, documents) + .mapToObj( + i -> + (org.springframework.web.multipart.MultipartFile) + new MockMultipartFile( + "fileInput", + "doc" + i + ".pdf", + "application/pdf", + ("pdf-" + i).getBytes())) + .toList()); + return files; + } + + private String documentReferenceOf(PolicyRunFiles files) throws Exception { + Policy p = policy("a", 1L); + when(policyStore.get("a")).thenReturn(Optional.of(p)); + when(policyAccessGuard.canAccess(p)).thenReturn(true); + when(policyRunner.runWith(eq(p), any(), eq(PolicyProgressListener.NOOP), any())) + .thenReturn(handle("run-9")); + + controller.runStoredPolicy("a", files); + + ArgumentCaptor reference = ArgumentCaptor.forClass(String.class); + verify(policyRunner) + .runWith(eq(p), any(), eq(PolicyProgressListener.NOOP), reference.capture()); + return reference.getValue(); + } + @Test @DisplayName("runs a stored, accessible policy") void runsStored() throws Exception { Policy p = policy("a", 1L); when(policyStore.get("a")).thenReturn(Optional.of(p)); when(policyAccessGuard.canAccess(p)).thenReturn(true); - when(policyRunner.runWith(eq(p), any(), eq(PolicyProgressListener.NOOP))) + when(policyRunner.runWith(eq(p), any(), eq(PolicyProgressListener.NOOP), any())) .thenReturn(handle("run-9")); ResponseEntity> response = @@ -716,6 +754,35 @@ class PolicyControllerTest { assertThat(response.getBody().getJobId()).isEqualTo("run-9"); } + @Test + @DisplayName("records the caller's own id for a single-document run") + void carriesTheCallersDocumentReference() throws Exception { + // The point of the field: a failure names a document the client that started it can + // resolve. + assertThat(documentReferenceOf(filesWith("editor-file-1", 1))) + .isEqualTo("editor-file-1"); + } + + @Test + @DisplayName("records nothing when the run carries several documents") + void refusesToGuessWhichOfSeveralDocumentsItIs() throws Exception { + // One incident, one reference: naming one of several would attribute it to whichever + // bound first. + assertThat(documentReferenceOf(filesWith("editor-file-1", 3))).isNull(); + } + + @Test + @DisplayName("records nothing when the caller sent no id") + void toleratesACallerThatSendsNoReference() throws Exception { + assertThat(documentReferenceOf(filesWith(null, 1))).isNull(); + } + + @Test + @DisplayName("records nothing for a blank id") + void treatsABlankReferenceAsNone() throws Exception { + assertThat(documentReferenceOf(filesWith(" ", 1))).isNull(); + } + @Test @DisplayName("not found when the stored policy is inaccessible") void notFound() { @@ -838,7 +905,7 @@ class PolicyControllerTest { Policy p = policy("a", 1L); when(policyStore.get("a")).thenReturn(Optional.of(p)); when(policyAccessGuard.canAccess(p)).thenReturn(true); - when(policyRunner.runWith(eq(p), any(), eq(PolicyProgressListener.NOOP))) + when(policyRunner.runWith(eq(p), any(), eq(PolicyProgressListener.NOOP), any())) .thenReturn(handle("run-9")); ResponseEntity> response = diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/policy/engine/PolicyRunnerTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/policy/engine/PolicyRunnerTest.java index d6f5951e3a..ebcf316c25 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/policy/engine/PolicyRunnerTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/policy/engine/PolicyRunnerTest.java @@ -32,6 +32,7 @@ import org.springframework.core.io.ByteArrayResource; import stirling.software.proprietary.policy.input.InputSource; import stirling.software.proprietary.policy.input.ResolveContext; import stirling.software.proprietary.policy.input.ResolvedInput; +import stirling.software.proprietary.policy.ledger.IdentityHasher; import stirling.software.proprietary.policy.ledger.InProcessProcessedLedger; import stirling.software.proprietary.policy.ledger.ProcessedLedger; import stirling.software.proprietary.policy.model.InputSpec; @@ -293,13 +294,56 @@ class PolicyRunnerTest { Policy policy = policy(List.of(InputSpec.folder("/in"))); PolicyInputs inputs = PolicyInputs.of(List.of()); PolicyRunHandle handle = new PolicyRunHandle("r", new CompletableFuture<>()); - when(policyEngine.runPolicy(policy, inputs, PolicyProgressListener.NOOP)) + when(policyEngine.runPolicy(policy, inputs, PolicyProgressListener.NOOP, null, null)) .thenReturn(handle); - assertSame(handle, runner.runWith(policy, inputs, PolicyProgressListener.NOOP)); + assertSame(handle, runner.runWith(policy, inputs, PolicyProgressListener.NOOP, null)); verifyNoInteractions(folderSource); } + @Test + void anAttendedRunCarriesTheClientsOwnDocumentReferenceAndNoSource() { + // A failure of this run can then name the document the user is still holding, and the null + // sourceId is what marks the reference as the client's own rather than a source's hash. + Policy policy = policy(List.of()); + PolicyInputs inputs = PolicyInputs.of(List.of(new ByteArrayResource("a".getBytes()))); + when(policyEngine.runPolicy( + policy, inputs, PolicyProgressListener.NOOP, null, "editor-file-1")) + .thenReturn(new PolicyRunHandle("r", new CompletableFuture<>())); + + runner.runWith(policy, inputs, PolicyProgressListener.NOOP, "editor-file-1"); + + verify(policyEngine) + .runPolicy(policy, inputs, PolicyProgressListener.NOOP, null, "editor-file-1"); + } + + @Test + void anUnattendedRunStillCarriesItsSourcesHashedIdentity() throws Exception { + // The other id space, unchanged: a folder identity is a path, and a path is a filename, so + // what reaches the run is the one-way hash and never the client-minted kind of reference. + InputSpec spec = InputSpec.folder("/in"); + Policy policy = policy(List.of(spec)); + String sourceId = policy.inputs().getFirst().sourceId(); + when(folderSource.supports(spec)).thenReturn(true); + when(folderSource.resolve(eq(spec), any())) + .thenReturn( + List.of( + ResolvedInput.forFile( + PolicyInputs.of(List.of()), "/in/doc.pdf", success -> {}))); + when(policyEngine.runPolicy(any(), any(), any(), any(), any())) + .thenReturn(new PolicyRunHandle("r", new CompletableFuture<>())); + + runner.run(policy); + + verify(policyEngine) + .runPolicy( + eq(policy), + any(), + any(), + eq(sourceId), + eq(IdentityHasher.identityHash("/in/doc.pdf"))); + } + @Test void runWithRecordsSuppliedDocsAgainstTheEditorSourceForThePolicyTeam() { Policy policy = @@ -317,10 +361,11 @@ class PolicyRunnerTest { List.of( new ByteArrayResource("a".getBytes()), new ByteArrayResource("b".getBytes()))); - when(policyEngine.runPolicy(policy, inputs, PolicyProgressListener.NOOP)) + when(policyEngine.runPolicy( + policy, inputs, PolicyProgressListener.NOOP, null, "editor-file-1")) .thenReturn(new PolicyRunHandle("r", new CompletableFuture<>())); - runner.runWith(policy, inputs, PolicyProgressListener.NOOP); + runner.runWith(policy, inputs, PolicyProgressListener.NOOP, "editor-file-1"); String key = EditorSource.counterKey(7L); assertEquals(2, docCounter.statsFor(List.of(key)).get(key).total()); diff --git a/frontend/editor/public/locales/en-US/translation.toml b/frontend/editor/public/locales/en-US/translation.toml index 1a0f862773..7760f002dd 100644 --- a/frontend/editor/public/locales/en-US/translation.toml +++ b/frontend/editor/public/locales/en-US/translation.toml @@ -5187,6 +5187,30 @@ openProcessor = "Open PDF Processor" count = "{{remaining}} of {{total}}" label = "Free credits" +[notifications] +empty = "Nothing to report." +handoffUnavailable = "This browser will not let the processor pass the document to the editor. Open it from the editor instead." +noDocumentLinked = "This failure is not linked to a specific document, so there is nothing to open here." +notOnThisDevice = "This document is not on this device, so it cannot be opened here." +occurrences = "{{count}} times" +open = "Notifications" +title = "Notifications" +unread = "Unread" + +[notifications.action] +failed = "That did not work. Try again in a moment." +unavailable = "Not available for this notification." + +[notifications.detail] +copied = "Copied" +copy = "Copy error" +less = "Show less" +more = "Show full message" + +[notifications.section] +earlier = "Earlier" +new = "New" + [oauth.error] message = "Authentication was not successful. You can close this window and try again." title = "Authentication Failed" @@ -7506,6 +7530,8 @@ acknowledge = "Acknowledge" confirm = "Are you sure?" dismiss = "Dismiss" dismissSkipFile = "Skip this file" +viewFile = "View file" +viewInProcessor = "View in processor" [portal.failures.debug] copyJson = "Copy JSON" @@ -7517,6 +7543,8 @@ showJson = "Show raw JSON ({{total}})" [portal.failures.disabled] closed = "This failure is already closed." +noDocument = "This failure was not recorded against a specific document, so there is nothing here to open." +unattended = "This file was fed by a folder, bucket or webhook, so nobody's browser is holding it to open." unavailable = "Not available for this failure." [portal.failures.empty] diff --git a/frontend/editor/src/core/components/layout/Workbench.tsx b/frontend/editor/src/core/components/layout/Workbench.tsx index 8bc0794140..903c552fd2 100644 --- a/frontend/editor/src/core/components/layout/Workbench.tsx +++ b/frontend/editor/src/core/components/layout/Workbench.tsx @@ -22,6 +22,7 @@ import WorkbenchFloatingSearch from "@app/components/shared/WorkbenchFloatingSea import LandingPage from "@app/components/shared/LandingPage"; import DismissAllErrorsButton from "@app/components/shared/DismissAllErrorsButton"; import { ChatFAB } from "@app/components/chat/ChatFAB"; +import { NotificationBell } from "@app/components/notifications/NotificationBell"; // Workbench panels are loaded on demand. Viewer pulls in pdfjs-dist and the // full @embedpdf plugin set; FileEditor/PageEditor are only needed once a file @@ -248,6 +249,15 @@ export default function Workbench() { data-tour="workbench" style={{ backgroundColor: "var(--c-bg)", minWidth: 0 }} > + {/* The bell normally rides in the workbench bar. Wherever that bar is not shown - My Files, + an empty workbench, a custom view without top controls - it gets its own corner, rather + than those being the places a user cannot see that something of theirs failed. */} + {!showWorkbenchBar && ( +

+ +
+ )} + {showWorkbenchBar && (
diff --git a/frontend/editor/src/core/components/notifications/NotificationBell.css b/frontend/editor/src/core/components/notifications/NotificationBell.css new file mode 100644 index 0000000000..9352f6bef8 --- /dev/null +++ b/frontend/editor/src/core/components/notifications/NotificationBell.css @@ -0,0 +1,170 @@ +.notification-bell { + position: relative; + display: inline-flex; +} + +.notification-bell__trigger { + display: inline-flex; + align-items: center; + justify-content: center; + position: relative; + padding: var(--sp-2, 0.5rem); + border: none; + border-radius: var(--radius-md, 0.375rem); + background: transparent; + color: var(--c-text-muted); + cursor: pointer; +} + +.notification-bell__trigger:hover { + background: var(--c-hover); + color: var(--c-text); +} + +.notification-bell__badge { + position: absolute; + top: 0.125rem; + right: 0.125rem; + min-width: 1rem; + padding: 0 0.25rem; + border-radius: 999px; + background: var(--c-danger); + color: var(--c-text-on-danger); + font-size: 0.625rem; + line-height: 1rem; + text-align: center; +} + +.notification-bell__panel { + position: fixed; + z-index: var(--z-popover, 60); + width: min(22rem, calc(100vw - 2rem)); + max-height: 24rem; + overflow-y: auto; + padding: var(--sp-3, 0.75rem); + border: 1px solid var(--c-border); + border-radius: var(--radius-lg, 0.5rem); + background: var(--c-surface); + box-shadow: 0 10px 30px rgb(0 0 0 / 25%); +} + +.notification-bell__heading { + margin: 0 0 var(--sp-2, 0.5rem); + font-size: 0.875rem; + font-weight: 600; + color: var(--c-text); +} + +.notification-bell__empty { + margin: 0; + font-size: 0.8125rem; + color: var(--c-text-muted); +} + +.notification-bell__list { + margin: 0; + padding: 0; + list-style: none; + display: flex; + flex-direction: column; + gap: var(--sp-2, 0.5rem); +} + +.notification-bell__item { + position: relative; + display: grid; + grid-template-columns: auto 1fr; + gap: 0 var(--sp-2, 0.5rem); + padding: var(--sp-2, 0.5rem); + border-radius: var(--radius-md, 0.375rem); + background: var(--c-surface-sunken); +} + +/* Wraps rather than crowds: a row can carry three buttons, and the panel is narrow. */ +.notification-bell__actions { + grid-column: 2; + display: flex; + flex-wrap: wrap; + align-items: center; + justify-content: flex-end; + gap: var(--sp-1, 0.25rem); + margin-top: var(--sp-2, 0.5rem); +} + +.notification-bell__dot { + grid-row: 1; + align-self: center; + width: 0.5rem; + height: 0.5rem; + border-radius: 999px; + background: var(--c-danger); +} + +.notification-bell__item-title { + grid-column: 2; + font-size: 0.8125rem; + font-weight: 600; + color: var(--c-text); +} + +.notification-bell__count, +.notification-bell__detail { + grid-column: 2; + font-size: 0.75rem; + color: var(--c-text-muted); +} + +.notification-bell__detail { + overflow: hidden; + display: -webkit-box; + -webkit-line-clamp: 2; + -webkit-box-orient: vertical; + overflow-wrap: anywhere; +} + +/* Expanded, the message is the point of the row, so let it run and scroll rather than clamp. */ +.notification-bell__detail--full { + display: block; + max-height: 10rem; + overflow-y: auto; + -webkit-line-clamp: none; +} + +.notification-bell__chrome { + grid-column: 2; + display: flex; + gap: var(--sp-1, 0.25rem); + margin-top: var(--sp-1, 0.25rem); +} + +/* Reading aids for the message, tinted rather than filled: they sit next to the row's real actions + and must not read as one of them. */ +.notification-bell__chip { + padding: 0.0625rem 0.375rem; + border: none; + border-radius: var(--radius-sm, 0.25rem); + background: var(--c-primary-subtle); + color: var(--c-accent-fg, var(--c-primary)); + font-size: 0.6875rem; + cursor: pointer; +} + +.notification-bell__chip:hover, +.notification-bell__chip:focus-visible { + background: var(--c-hover); +} + +/* Why the actions this row could have had are absent. Muted: it explains, it does not warn. */ +.notification-bell__note { + grid-column: 2; + margin-top: var(--sp-1, 0.25rem); + font-size: 0.75rem; + color: var(--c-text-subtle); +} + +.notification-bell__message { + grid-column: 2; + margin-top: var(--sp-1, 0.25rem); + font-size: 0.75rem; + color: var(--c-danger); +} diff --git a/frontend/editor/src/core/components/notifications/NotificationBell.test.tsx b/frontend/editor/src/core/components/notifications/NotificationBell.test.tsx new file mode 100644 index 0000000000..8753a5880b --- /dev/null +++ b/frontend/editor/src/core/components/notifications/NotificationBell.test.tsx @@ -0,0 +1,530 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { + fireEvent, + render as baseRender, + screen, + waitFor, +} from "@testing-library/react"; +import { MantineProvider } from "@mantine/core"; +import type { + AppNotification, + NotificationActionOffer, +} from "@app/services/notifications"; + +// @app/ui Button is a Mantine wrapper, so it needs the provider in the tree. +const render = (ui: Parameters[0]) => + baseRender(ui, { wrapper: MantineProvider }); + +/** + * Two things are the bell's own and worth pinning: which notifications the user has already looked + * at, and how a row behaves around an action. + */ + +const fetchNotifications = vi.fn(); + +vi.mock("@app/services/notifications", () => ({ + fetchNotifications: (...args: unknown[]) => fetchNotifications(...args), +})); + +// IndexedDB, which jsdom has none of. Answered here so availability is a fact of the test. +const h = vi.hoisted(() => ({ + hasLocalFile: true, + // This build has the notifications API, except in the one test about the build that does not. + notificationsAvailable: true, + specs: {} as Record< + string, + { + available: (context: unknown) => boolean; + run: (context: unknown, password?: string) => unknown; + closesPanel?: boolean; + } + >, +})); + +vi.mock("@app/services/localFilePresence", () => ({ + hasLocalFile: () => Promise.resolve(h.hasLocalFile), +})); + +vi.mock("@app/components/notifications/useNotificationsAvailable", () => ({ + useNotificationsAvailable: () => h.notificationsAvailable, +})); + +// Core's own registry is empty, so without this there are no client actions to test. +vi.mock("@app/components/notifications/notificationActions", () => ({ + useNotificationActions: () => h.specs, +})); + +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ + // A string fallback, or an options object with defaultValue plus what it interpolates. + t: (key: string, fallback?: unknown) => { + if (typeof fallback === "string") return fallback; + if (fallback && typeof fallback === "object") { + const options = fallback as Record; + const template = options.defaultValue; + if (typeof template !== "string") return key; + return template.replace(/{{(\w+)}}/g, (_match, name: string) => + String(options[name] ?? ""), + ); + } + return key; + }, + }), +})); + +const { NotificationBell } = + await import("@app/components/notifications/NotificationBell"); + +function offer( + id: string, + overrides: Partial = {}, +): NotificationActionOffer { + return { + id, + labelKey: `portal.failures.action.${id.toLowerCase()}`, + defaultLabel: id, + enabled: true, + disabledReasonKey: null, + ...overrides, + }; +} + +function notification( + id: string, + title = "Unrecognised failure", + overrides: Partial = {}, +): AppNotification { + return { + id, + source: "FAILURE", + kindId: "UNKNOWN", + origin: "TOOL", + ownership: "MINE", + severity: "ERROR", + status: "NEW", + titleKey: `portal.failures.kind.${id}.title`, + defaultTitle: title, + detail: "boom", + fileId: "f-1", + sourceId: null, + policyId: null, + occurrences: 1, + createdAt: "2026-08-05T00:00:00Z", + lastSeenAt: "2026-08-05T00:00:00Z", + actions: [], + ...overrides, + }; +} + +async function openPanel() { + fireEvent.click(await screen.findByRole("button")); +} + +describe("NotificationBell", () => { + beforeEach(() => { + window.localStorage.clear(); + fetchNotifications.mockReset().mockResolvedValue([]); + h.hasLocalFile = true; + h.notificationsAvailable = true; + h.specs = {}; + }); + + it("mounts nothing at all in a build with no notifications API", async () => { + // No bell and, above all, no poll: an OSS build must not sit on a timer collecting 404s. + h.notificationsAvailable = false; + + render(); + + await Promise.resolve(); + expect(screen.queryByRole("button")).toBeNull(); + expect(fetchNotifications).not.toHaveBeenCalled(); + }); + + it("shows no badge when there is nothing to report", async () => { + render(); + + await waitFor(() => expect(fetchNotifications).toHaveBeenCalled()); + expect(screen.queryByText("1")).toBeNull(); + }); + + it("counts everything as unread the first time, since nothing has been seen", async () => { + fetchNotifications.mockResolvedValue([ + notification("a"), + notification("b"), + ]); + + render(); + + expect(await screen.findByText("2")).toBeTruthy(); + }); + + it("clears the badge once the user opens the panel", async () => { + fetchNotifications.mockResolvedValue([ + notification("a"), + notification("b"), + ]); + render(); + await openPanel(); + + // Opening marks them read: waiting for the close would leave the badge lit. + await waitFor(() => expect(screen.queryByText("2")).toBeNull()); + }); + + it("divides what is new from what the user has already seen", async () => { + // "b" was the newest last time, so "a" is the only new one. + window.localStorage.setItem("stirling.notifications.lastSeenId", "b"); + fetchNotifications.mockResolvedValue([ + notification("a"), + notification("b"), + ]); + render(); + await openPanel(); + + expect(await screen.findByText("New")).toBeTruthy(); + expect(screen.getByText("Earlier")).toBeTruthy(); + }); + + it("keeps the division on screen after opening marks them read", async () => { + // Frozen on open: read live it would collapse the moment the badge cleared. + window.localStorage.setItem("stirling.notifications.lastSeenId", "b"); + fetchNotifications.mockResolvedValue([ + notification("a"), + notification("b"), + ]); + render(); + await openPanel(); + + await waitFor(() => expect(screen.queryByText("1")).toBeNull()); + expect(screen.getByText("New")).toBeTruthy(); + expect(screen.getByText("Earlier")).toBeTruthy(); + }); + + it("does not divide a list with nothing new in it", async () => { + window.localStorage.setItem("stirling.notifications.lastSeenId", "a"); + fetchNotifications.mockResolvedValue([notification("a")]); + render(); + await openPanel(); + + // A lone "Earlier" heading over everything says nothing the empty badge has not. + expect(await screen.findByText("Unrecognised failure")).toBeTruthy(); + expect(screen.queryByText("Earlier")).toBeNull(); + expect(screen.queryByText("New")).toBeNull(); + }); + + it("labels an all-new list without inventing an earlier section", async () => { + fetchNotifications.mockResolvedValue([ + notification("a"), + notification("b"), + ]); + render(); + await openPanel(); + + expect(await screen.findByText("New")).toBeTruthy(); + expect(screen.queryByText("Earlier")).toBeNull(); + }); + + it("marks only what arrived since the user last looked", async () => { + fetchNotifications.mockResolvedValue([notification("a")]); + const first = render(); + await openPanel(); + await waitFor(() => expect(screen.queryByText("1")).toBeNull()); + first.unmount(); + + // A newer one arrives above the one already seen. + fetchNotifications.mockResolvedValue([ + notification("b"), + notification("a"), + ]); + render(); + + expect(await screen.findByText("1")).toBeTruthy(); + }); + + it("treats everything as unread when the last seen one is gone", async () => { + // We cannot tell how far the user got, so show them rather than marking the lot read. + window.localStorage.setItem( + "stirling.notifications.lastSeenId", + "vanished", + ); + fetchNotifications.mockResolvedValue([ + notification("a"), + notification("b"), + ]); + + render(); + + expect(await screen.findByText("2")).toBeTruthy(); + }); + + it("renders the server's title and repeat count without knowing the source", async () => { + fetchNotifications.mockResolvedValue([ + { ...notification("a", "Password-protected document"), occurrences: 3 }, + ]); + render(); + await openPanel(); + + expect(screen.getByText("Password-protected document")).toBeTruthy(); + expect(screen.getByText("3 times")).toBeTruthy(); + }); + + it("puts every one of the row's actions on the row", async () => { + h.specs = { + VIEW_IN_PROCESSOR: { + available: () => true, + run: vi.fn(), + closesPanel: true, + }, + VIEW_FILE: { available: () => true, run: vi.fn(), closesPanel: true }, + }; + fetchNotifications.mockResolvedValue([ + notification("a", "Unrecognised failure", { + actions: [offer("VIEW_IN_PROCESSOR"), offer("VIEW_FILE")], + }), + ]); + render(); + await openPanel(); + + // Named for their row: every button in the list says the same thing. + for (const id of ["VIEW_IN_PROCESSOR", "VIEW_FILE"]) + expect( + screen.getByRole("button", { name: `${id}: Unrecognised failure` }), + ).toBeTruthy(); + }); + + it("runs whichever of the row's actions is pressed", async () => { + const run = vi.fn(); + h.specs = { + VIEW_IN_PROCESSOR: { available: () => true, run: vi.fn() }, + VIEW_FILE: { available: () => true, run }, + }; + fetchNotifications.mockResolvedValue([ + notification("a", "Unrecognised failure", { + actions: [offer("VIEW_IN_PROCESSOR"), offer("VIEW_FILE")], + }), + ]); + render(); + await openPanel(); + + fireEvent.click( + screen.getByRole("button", { name: "VIEW_FILE: Unrecognised failure" }), + ); + + await waitFor(() => expect(run).toHaveBeenCalledTimes(1)); + expect(screen.getByText("Unrecognised failure")).toBeTruthy(); + }); + + it("closes the panel on its way to a destination behind it", async () => { + const run = vi.fn(); + h.specs = { VIEW_FILE: { available: () => true, run, closesPanel: true } }; + fetchNotifications.mockResolvedValue([ + notification("a", "Password-protected document", { + actions: [offer("VIEW_FILE")], + }), + ]); + render(); + await openPanel(); + + fireEvent.click( + screen.getByRole("button", { + name: "VIEW_FILE: Password-protected document", + }), + ); + + expect(run).toHaveBeenCalledTimes(1); + await waitFor(() => + expect(screen.queryByText("Password-protected document")).toBeNull(), + ); + }); + + it("skips an action id this build has never heard of", async () => { + // A new failure kind can ship with new actions; an unwired button would be worse than none. + fetchNotifications.mockResolvedValue([ + notification("a", "Unrecognised failure", { + actions: [offer("QUARANTINE")], + }), + ]); + render(); + await openPanel(); + + expect(screen.getByText("Unrecognised failure")).toBeTruthy(); + expect(screen.queryByRole("button", { name: /QUARANTINE/ })).toBeNull(); + }); + + it("drops an action the device cannot perform, and says why the row is thin", async () => { + h.hasLocalFile = false; + h.specs = { + VIEW_FILE: { + available: (context) => + (context as { hasLocalFile: boolean }).hasLocalFile, + run: vi.fn(), + }, + }; + fetchNotifications.mockResolvedValue([ + notification("a", "Unrecognised failure", { + actions: [offer("VIEW_FILE")], + }), + ]); + render(); + await openPanel(); + + await waitFor(() => + expect( + screen.getByText( + "This document is not on this device, so it cannot be opened here.", + ), + ).toBeTruthy(), + ); + expect(screen.queryByRole("button", { name: /VIEW_FILE/ })).toBeNull(); + }); + + it("says a row was never linked to a document, rather than that the document is missing", async () => { + h.hasLocalFile = false; + fetchNotifications.mockResolvedValue([ + notification("a", "Unrecognised failure", { fileId: null }), + ]); + render(); + await openPanel(); + + expect( + await screen.findByText( + "This failure is not linked to a specific document, so there is nothing to open here.", + ), + ).toBeTruthy(); + }); + + it("claims nothing about a device for a row it never looks up", async () => { + // Never on any device, so never probed, and an absent lookup is not an absent document. + h.hasLocalFile = false; + fetchNotifications.mockResolvedValue([ + notification("a", "Password-protected document", { + origin: "POLICY", + sourceId: "src-s3-invoices", + }), + ]); + render(); + await openPanel(); + + expect(await screen.findByText("Password-protected document")).toBeTruthy(); + expect( + screen.queryByText( + /not on this device|not linked to a specific document/, + ), + ).toBeNull(); + }); + + it("renders no button for an action the server would refuse, and says why in words", async () => { + // A greyed button that can never work is false hope, so the reason becomes the row's note. + h.specs = { + VIEW_FILE: { available: () => true, run: vi.fn() }, + VIEW_IN_PROCESSOR: { available: () => true, run: vi.fn() }, + }; + fetchNotifications.mockResolvedValue([ + notification("a", "Unrecognised failure", { + ownership: "UNOWNED", + actions: [ + offer("VIEW_FILE", { + enabled: false, + disabledReasonKey: "portal.failures.disabled.unattended", + }), + offer("VIEW_IN_PROCESSOR"), + ], + }), + ]); + render(); + await openPanel(); + + expect(screen.queryByRole("button", { name: /VIEW_FILE/ })).toBeNull(); + expect( + screen.getByRole("button", { + name: "VIEW_IN_PROCESSOR: Unrecognised failure", + }), + ).toBeTruthy(); + expect( + screen.getByText("Not available for this notification."), + ).toBeTruthy(); + }); + + it("leaves a closed row with no buttons rather than a row of dead ones", async () => { + h.specs = { + VIEW_IN_PROCESSOR: { available: () => true, run: vi.fn() }, + VIEW_FILE: { available: () => true, run: vi.fn() }, + }; + fetchNotifications.mockResolvedValue([ + notification("a", "Unrecognised failure", { + actions: [ + offer("VIEW_IN_PROCESSOR", { + enabled: false, + disabledReasonKey: "portal.failures.disabled.closed", + }), + offer("VIEW_FILE", { + enabled: false, + disabledReasonKey: "portal.failures.disabled.closed", + }), + ], + }), + ]); + render(); + await openPanel(); + + // The message and its chips remain, so the row still reads as a row. + expect(screen.getByText("Unrecognised failure")).toBeTruthy(); + expect( + screen.getByText("Not available for this notification."), + ).toBeTruthy(); + expect( + screen.queryByRole("button", { name: /VIEW_IN_PROCESSOR|VIEW_FILE/ }), + ).toBeNull(); + expect(document.querySelector(".notification-bell__actions")).toBeNull(); + }); + + it("shows a failed action in the row instead of leaving the user guessing", async () => { + h.specs = { + VIEW_FILE: { + available: () => true, + run: () => Promise.resolve({ ok: false, message: "Could not open" }), + }, + }; + fetchNotifications.mockResolvedValue([ + notification("a", "Password-protected document", { + actions: [offer("VIEW_FILE")], + }), + ]); + render(); + await openPanel(); + + fireEvent.click( + screen.getByRole("button", { + name: "VIEW_FILE: Password-protected document", + }), + ); + + expect(await screen.findByRole("alert")).toHaveProperty( + "textContent", + "Could not open", + ); + // Still on screen, so the row remains actionable. + expect(screen.getByText("Password-protected document")).toBeTruthy(); + }); + + it("expands the message without touching the row's actions", async () => { + fetchNotifications.mockResolvedValue([ + notification("a", "Unrecognised failure", { + detail: "org.apache.pdfbox.InvalidPasswordException", + }), + ]); + render(); + await openPanel(); + + const expand = screen.getByRole("button", { + name: "Show full message: Unrecognised failure", + }); + fireEvent.click(expand); + + expect( + screen.getByRole("button", { name: "Show less: Unrecognised failure" }), + ).toBeTruthy(); + expect( + screen.getByRole("button", { name: "Copy error: Unrecognised failure" }), + ).toBeTruthy(); + }); +}); diff --git a/frontend/editor/src/core/components/notifications/NotificationBell.tsx b/frontend/editor/src/core/components/notifications/NotificationBell.tsx new file mode 100644 index 0000000000..f2ac1e0a3a --- /dev/null +++ b/frontend/editor/src/core/components/notifications/NotificationBell.tsx @@ -0,0 +1,172 @@ +import { + Fragment, + useEffect, + useId, + useLayoutEffect, + useRef, + useState, +} from "react"; +import { useTranslation } from "react-i18next"; +import { BellIcon, Button } from "@app/ui"; +import DividerWithText from "@app/components/shared/DividerWithText"; +import { useNotifications } from "@app/hooks/useNotifications"; +import { useNotificationActions } from "@app/components/notifications/notificationActions"; +import { NotificationItem } from "@app/components/notifications/NotificationItem"; +import { useNotificationsAvailable } from "@app/components/notifications/useNotificationsAvailable"; +import "@app/components/notifications/NotificationBell.css"; + +/** + * Renders whatever the server sends without knowing which subsystem produced it or what its actions + * mean, so a new source or failure kind needs no change here. In core because both shells mount it. + */ +export function NotificationBell() { + // A build with no notifications API gets no bell at all, rather than one that polls a + // nonexistent endpoint forever to show nothing. + const available = useNotificationsAvailable(); + if (!available) return null; + return ; +} + +function MountedNotificationBell() { + const { t } = useTranslation(); + const { notifications, unreadCount, documentStateFor, markAllSeen } = + useNotifications(); + const registry = useNotificationActions(); + const [open, setOpen] = useState(false); + const container = useRef(null); + const headingId = useId(); + // Where the new ones stop, frozen when the panel opens (opening marks everything read). + const [firstSeenId, setFirstSeenId] = useState(null); + // Viewport-fixed, because the workbench bar clips its own overflow. + const [anchor, setAnchor] = useState<{ top: number; right: number } | null>( + null, + ); + + useLayoutEffect(() => { + if (!open) return; + const measure = () => { + const rect = container.current?.getBoundingClientRect(); + if (!rect) return; + setAnchor({ + top: rect.bottom + 8, + right: Math.max(8, window.innerWidth - rect.right), + }); + }; + measure(); + window.addEventListener("resize", measure); + window.addEventListener("scroll", measure, true); + return () => { + window.removeEventListener("resize", measure); + window.removeEventListener("scroll", measure, true); + }; + }, [open]); + + // Opening marks them read, not closing: waiting would leave the badge lit while they read. + const toggle = () => { + setOpen((wasOpen) => { + if (!wasOpen) { + // Before marking, or there is nothing left to read. + setFirstSeenId(notifications[unreadCount]?.id ?? null); + markAllSeen(); + } + return !wasOpen; + }); + }; + + /** + * How many count as new. No boundary id means all of them were; one that has since left the list + * leaves nothing to divide on, so it reads as none rather than guessing at a row. + */ + const boundaryIndex = firstSeenId + ? notifications.findIndex((notification) => notification.id === firstSeenId) + : notifications.length; + const dividedAt = Math.max(0, boundaryIndex); + + useEffect(() => { + if (!open) return; + const closeOnOutside = (event: MouseEvent) => { + const target = event.target as HTMLElement; + if (!container.current?.contains(target)) setOpen(false); + }; + const closeOnEscape = (event: KeyboardEvent) => { + if (event.key === "Escape") setOpen(false); + }; + document.addEventListener("mousedown", closeOnOutside); + document.addEventListener("keydown", closeOnEscape); + return () => { + document.removeEventListener("mousedown", closeOnOutside); + document.removeEventListener("keydown", closeOnEscape); + }; + }, [open]); + + return ( +
+ + + {open && ( +
+

+ {t("notifications.title", "Notifications")} +

+ + {notifications.length === 0 ? ( +

+ {t("notifications.empty", "Nothing to report.")} +

+ ) : ( +
    + {notifications.map((notification, index) => ( + + {index === 0 && dividedAt > 0 && ( +
  • + +
    • + )} + {/* Only with something on both sides: a lone "Earlier" over everything says + nothing the empty badge has not. */} + {index === dividedAt && dividedAt > 0 && ( +
  • + +
    • + )} + setOpen(false)} + /> +     + ))} +
+ )} +
+ )} +
+ ); +} diff --git a/frontend/editor/src/core/components/notifications/NotificationItem.tsx b/frontend/editor/src/core/components/notifications/NotificationItem.tsx new file mode 100644 index 0000000000..b53ca7894c --- /dev/null +++ b/frontend/editor/src/core/components/notifications/NotificationItem.tsx @@ -0,0 +1,251 @@ +import { useState } from "react"; +import type { TFunction } from "i18next"; +import { useTranslation } from "react-i18next"; +import { Button } from "@app/ui"; +import { isResolvableHere } from "@app/hooks/useNotifications"; +import type { NotificationDocumentState } from "@app/hooks/useNotifications"; +import type { + ClientActionRegistry, + NotificationActionContext, +} from "@app/components/notifications/notificationActions"; +import type { + AppNotification, + NotificationActionOffer, +} from "@app/services/notifications"; + +/** + * The server's reason wins, being about the failure rather than this browser. Otherwise only what we + * actually looked up, so a row we never probed is never called absent. + */ +function noteFor( + notification: AppNotification, + documentState: NotificationDocumentState, + withheldReasonKey: string | null, + t: TFunction, +): string | null { + if (withheldReasonKey) + return t(withheldReasonKey, { + defaultValue: t( + "notifications.action.unavailable", + "Not available for this notification.", + ), + }); + if (notification.ownership !== "MINE" || documentState.hasLocalFile) + return null; + if (!notification.fileId) + return t( + "notifications.noDocumentLinked", + "This failure is not linked to a specific document, so there is nothing to open here.", + ); + return isResolvableHere(notification) + ? t( + "notifications.notOnThisDevice", + "This document is not on this device, so it cannot be opened here.", + ) + : null; +} + +interface NotificationItemProps { + notification: AppNotification; + unread: boolean; + documentState: NotificationDocumentState; + registry: ClientActionRegistry; + onDismissPanel: () => void; +} + +/** Its own component because the last attempt's message and its expanded state are per-row. */ +export function NotificationItem({ + notification, + unread, + documentState, + registry, + onDismissPanel, +}: NotificationItemProps) { + const { t } = useTranslation(); + const [message, setMessage] = useState(null); + const [busy, setBusy] = useState(null); + const [expanded, setExpanded] = useState(false); + const [copied, setCopied] = useState(false); + + const title = t(notification.titleKey, notification.defaultTitle); + const context: NotificationActionContext = { + notification, + hasLocalFile: documentState.hasLocalFile, + }; + + // An id this build has never heard of is skipped rather than rendered unwired: the server ships + // new kinds, and new actions, ahead of the clients that understand them. + const usable = notification.actions.filter((offer) => { + if (!offer.enabled) return false; + const spec = registry[offer.id]; + return spec ? spec.available(context) : false; + }); + + // Only from an action this build would otherwise have rendered: a reason about one it cannot + // perform anyway is not this row's explanation. + const withheldReasonKey = + notification.actions.find( + (offer) => + !offer.enabled && + offer.disabledReasonKey !== null && + registry[offer.id] !== undefined, + )?.disabledReasonKey ?? null; + + const labelOf = (offer: NotificationActionOffer) => + t(offer.labelKey, offer.defaultLabel); + + const run = async (offer: NotificationActionOffer) => { + if (busy) return; + setMessage(null); + + const spec = registry[offer.id]; + if (!spec) return; + + setBusy(offer.id); + const outcome = await spec.run(context); + setBusy(null); + if (outcome && !outcome.ok) { + setMessage( + outcome.message ?? + t( + "notifications.action.failed", + "That did not work. Try again in a moment.", + ), + ); + return; + } + + if (spec.closesPanel) onDismissPanel(); + }; + + const copyDetail = async () => { + if (!notification.detail) return; + try { + await navigator.clipboard.writeText(notification.detail); + setCopied(true); + } catch { + // No clipboard permission, and the message is on screen and selectable anyway. + } + }; + + const note = noteFor(notification, documentState, withheldReasonKey, t); + + return ( +
  • + {unread && ( + + )} + {title} + {notification.occurrences > 1 && ( + + {t("notifications.occurrences", { + count: notification.occurrences, + defaultValue: "{{count}} times", + })} + + )} + + {notification.detail && ( + <> + + {notification.detail} + + + + + + + )} + + {note && {note}} + + {/* In the kind's declared order, the first leading. */} + {usable.length > 0 && ( + + {usable.map((offer, index) => ( + void run(offer)} + /> + ))} + + )} + + {message && ( + + {message} + + )} +
    • + ); +} + +interface ActionButtonProps { + variant: "primary" | "secondary"; + rowTitle: string; + label: string; + busy: boolean; + onRun: () => void; +} + +function ActionButton({ + variant, + rowTitle, + label, + busy, + onRun, +}: ActionButtonProps) { + return ( + + ); +} diff --git a/frontend/editor/src/core/components/notifications/notificationActions.ts b/frontend/editor/src/core/components/notifications/notificationActions.ts new file mode 100644 index 0000000000..874e6fb824 --- /dev/null +++ b/frontend/editor/src/core/components/notifications/notificationActions.ts @@ -0,0 +1,40 @@ +import type { AppNotification } from "@app/services/notifications"; + +/** + * Keyed by action rather than by row, because the server decides what a kind offers: adding a kind is + * no frontend change, and adding a button is one entry here. + */ + +export interface NotificationActionContext { + notification: AppNotification; + /** Whether the document is still in this browser, which is what most actions hinge on. */ + hasLocalFile: boolean; +} + +/** `void` means it did what it said; a failed outcome carries the message the row shows. */ +export interface ClientActionOutcome { + ok: boolean; + message?: string; +} + +export interface ClientActionSpec { + /** Asked per row, never during a request. */ + available(context: NotificationActionContext): boolean; + run( + context: NotificationActionContext, + ): ClientActionOutcome | void | Promise; + /** Whether the panel should get out of the way, the destination being behind it. */ + closesPanel?: boolean; +} + +/** An id with no entry is skipped rather than rendered unwired. */ +export type ClientActionRegistry = Readonly< + Record +>; + +const NONE: ClientActionRegistry = {}; + +/** Every destination ships in a higher layer, so this build's rows carry no buttons. */ +export function useNotificationActions(): ClientActionRegistry { + return NONE; +} diff --git a/frontend/editor/src/core/components/notifications/useNotificationsAvailable.ts b/frontend/editor/src/core/components/notifications/useNotificationsAvailable.ts new file mode 100644 index 0000000000..1835a0541f --- /dev/null +++ b/frontend/editor/src/core/components/notifications/useNotificationsAvailable.ts @@ -0,0 +1,11 @@ +/** + * Whether this build has a notifications API to read. When it does not, the bell must not + * mount at all: an unconditional mount would poll an endpoint that does not exist, leaving a + * permanent timer and a 404 in the network log for nothing it could ever show. + * + * Core has no failure registry and no notification routes, so the answer here is no; a build + * that ships them overrides this to say so. + */ +export function useNotificationsAvailable(): boolean { + return false; +} diff --git a/frontend/editor/src/proprietary/components/shared/DividerWithText.stories.tsx b/frontend/editor/src/core/components/shared/DividerWithText.stories.tsx similarity index 100% rename from frontend/editor/src/proprietary/components/shared/DividerWithText.stories.tsx rename to frontend/editor/src/core/components/shared/DividerWithText.stories.tsx diff --git a/frontend/editor/src/proprietary/components/shared/DividerWithText.tsx b/frontend/editor/src/core/components/shared/DividerWithText.tsx similarity index 100% rename from frontend/editor/src/proprietary/components/shared/DividerWithText.tsx rename to frontend/editor/src/core/components/shared/DividerWithText.tsx diff --git a/frontend/editor/src/core/components/shared/WorkbenchBar.tsx b/frontend/editor/src/core/components/shared/WorkbenchBar.tsx index 19026c9df0..a18b58661f 100644 --- a/frontend/editor/src/core/components/shared/WorkbenchBar.tsx +++ b/frontend/editor/src/core/components/shared/WorkbenchBar.tsx @@ -59,6 +59,7 @@ import { renderWithTooltip } from "@app/components/shared/workbenchBar/workbench import { WorkbenchBarActionsProps } from "@app/components/shared/workbenchBar/types"; import { useIsMobile } from "@app/hooks/useIsMobile"; import "@app/components/shared/WorkbenchBar.css"; +import { NotificationBell } from "@app/components/notifications/NotificationBell"; const SECTION_ORDER: WorkbenchBarSection[] = ["top", "middle", "bottom"]; @@ -602,6 +603,9 @@ export default function WorkbenchBar({ enforcingProgress={enforcingProgress} /> )} + {/* Last in the globals, so it is the rightmost control. */} +
    +
    ); diff --git a/frontend/editor/src/proprietary/components/shared/dividerWithText/DividerWithText.css b/frontend/editor/src/core/components/shared/dividerWithText/DividerWithText.css similarity index 100% rename from frontend/editor/src/proprietary/components/shared/dividerWithText/DividerWithText.css rename to frontend/editor/src/core/components/shared/dividerWithText/DividerWithText.css diff --git a/frontend/editor/src/core/contexts/FileContext.tsx b/frontend/editor/src/core/contexts/FileContext.tsx index 634562375c..d9bac4332c 100644 --- a/frontend/editor/src/core/contexts/FileContext.tsx +++ b/frontend/editor/src/core/contexts/FileContext.tsx @@ -611,9 +611,11 @@ function FileContextInner({ // Remove from memory and cleanup resources lifecycleManager.removeFiles(fileIds, stateRef); - // Any failure recorded against these stops needing attention: the document is gone. - // Fire-and-forget, so a server that cannot be told never blocks the delete. - void reportFilesRemoved(fileIds); + // Only a real delete closes a failure: most callers pass false and mean "take it out of the + // workbench", leaving the document, and its failures, very much alive. + if (deleteFromStorage !== false) { + void reportFilesRemoved(fileIds); + } // Remove from IndexedDB if enabled if (indexedDB && enablePersistence && deleteFromStorage !== false) { diff --git a/frontend/editor/src/core/contexts/NavigationContext.tsx b/frontend/editor/src/core/contexts/NavigationContext.tsx index ae1ee43e0d..a7cb95d349 100644 --- a/frontend/editor/src/core/contexts/NavigationContext.tsx +++ b/frontend/editor/src/core/contexts/NavigationContext.tsx @@ -132,7 +132,11 @@ export interface NavigationContextActionsValue { const NavigationStateContext = createContext< NavigationContextStateValue | undefined >(undefined); -const NavigationActionsContext = createContext< +/** + * Exported like {@link FileActionsContext}: a component mounting in both shells must ask whether + * these exist, and {@link useNavigationActions} throws when they do not. + */ +export const NavigationActionsContext = createContext< NavigationContextActionsValue | undefined >(undefined); diff --git a/frontend/editor/src/core/contexts/file/removeFiles.reporting.test.tsx b/frontend/editor/src/core/contexts/file/removeFiles.reporting.test.tsx new file mode 100644 index 0000000000..dec17e25c4 --- /dev/null +++ b/frontend/editor/src/core/contexts/file/removeFiles.reporting.test.tsx @@ -0,0 +1,87 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { render, act } from "@testing-library/react"; +import { MantineProvider } from "@mantine/core"; +import { FileContextProvider } from "@app/contexts/FileContext"; +import { useFileActions } from "@app/contexts/file/fileHooks"; +import type { FileContextActions } from "@app/types/fileContext"; +import type { FileId } from "@app/types/file"; + +/** + * `removeFiles` deletes a document or merely takes it out of the workbench, told apart only by + * `deleteFromStorage`. Reporting both closed the user's own notifications as they opened files. + */ + +const reportFilesRemoved = vi.fn(); +vi.mock("@app/services/failureReporting", () => ({ + reportFilesRemoved: (fileIds: string[]) => reportFilesRemoved(fileIds), + reportToolFailure: vi.fn(), +})); + +// IndexedDB, which jsdom has none of. Stubbed so the delete branch can run to the end. +vi.mock("@app/services/fileStorage", () => ({ + // FileContext subscribes to this to drop files whose bytes are unreadable. + onRecordUnreadable: () => () => {}, + fileStorage: { + init: vi.fn().mockResolvedValue(undefined), + deleteMultipleStirlingFiles: vi.fn().mockResolvedValue(undefined), + getAllStirlingFileStubs: vi.fn().mockResolvedValue([]), + }, +})); + +const FILE_ID = "f-1" as FileId; + +let actionsRef: FileContextActions | null = null; + +function Controller() { + actionsRef = useFileActions().actions; + return null; +} + +function setup() { + render( + + + + + , + ); +} + +beforeEach(() => { + reportFilesRemoved.mockReset(); + actionsRef = null; +}); + +describe("removeFiles and the failure queue", () => { + it("tells the server when a document is actually deleted", async () => { + setup(); + + await act(async () => { + await actionsRef?.removeFiles([FILE_ID], true); + }); + + expect(reportFilesRemoved).toHaveBeenCalledWith([FILE_ID]); + }); + + it("says nothing when the file is only closed in the workbench", async () => { + // Closing a tab or unchecking it leaves the document on the device, failures and all. + setup(); + + await act(async () => { + await actionsRef?.removeFiles([FILE_ID], false); + }); + + expect(reportFilesRemoved).not.toHaveBeenCalled(); + }); + + it("treats an unspecified removal as a delete, the way the storage path does", async () => { + // Same default as the IndexedDB branch: only an explicit false means keep. + setup(); + + await act(async () => { + await actionsRef?.removeFiles([FILE_ID]); + }); + + expect(reportFilesRemoved).toHaveBeenCalledWith([FILE_ID]); + }); +}); diff --git a/frontend/editor/src/core/hooks/tools/shared/useToolOperation.ts b/frontend/editor/src/core/hooks/tools/shared/useToolOperation.ts index fe1ddc7bc7..822d281c57 100644 --- a/frontend/editor/src/core/hooks/tools/shared/useToolOperation.ts +++ b/frontend/editor/src/core/hooks/tools/shared/useToolOperation.ts @@ -22,6 +22,7 @@ import { } from "@app/types/fileContext"; import { FILE_EVENTS } from "@app/services/errorUtils"; import { reportToolFailure } from "@app/services/failureReporting"; +import { refreshNotificationsNow } from "@app/hooks/useNotifications"; import { zipFileService } from "@app/services/zipFileService"; import { getFilenameWithoutExtension } from "@app/utils/fileUtils"; import { @@ -606,11 +607,12 @@ export const useToolOperation = ( // Report it so a leader sees the failure too, then carry on with the user's // own error handling. Fire-and-forget: the reporter swallows its own errors. + // Chained, not fired alongside: the re-read must happen after the row exists. void reportToolFailure({ operation: config.operationType, error, fileIds: validFiles.map((file) => file.fileId), - }); + }).then(refreshNotificationsNow); const errorMessage = config.getErrorMessage?.(error) || extractErrorMessage(error); diff --git a/frontend/editor/src/core/hooks/useNotifications.test.ts b/frontend/editor/src/core/hooks/useNotifications.test.ts new file mode 100644 index 0000000000..746db77390 --- /dev/null +++ b/frontend/editor/src/core/hooks/useNotifications.test.ts @@ -0,0 +1,249 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { act, renderHook, waitFor } from "@testing-library/react"; +import type { AppNotification } from "@app/services/notifications"; + +/** + * The bell is mounted several times over, so what is pinned here is that they share one read: one + * poll, one set of lookups, one marker, and no timer left running once the last has gone. + */ + +const fetchNotifications = vi.fn(); + +vi.mock("@app/services/notifications", () => ({ + fetchNotifications: (...args: unknown[]) => fetchNotifications(...args), +})); + +// Counted here so "resolved once per list, not once per row" is observable. +const hasLocalFile = vi.fn((_fileId: string) => Promise.resolve(true)); + +vi.mock("@app/services/localFilePresence", () => ({ + hasLocalFile: (fileId: string) => hasLocalFile(fileId), +})); + +const { useNotifications, refreshNotificationsNow } = + await import("@app/hooks/useNotifications"); + +function notification( + id: string, + overrides: Partial = {}, +): AppNotification { + return { + id, + source: "FAILURE", + kindId: "UNKNOWN", + origin: "TOOL", + ownership: "MINE", + severity: "ERROR", + status: "NEW", + titleKey: `portal.failures.kind.${id}.title`, + defaultTitle: id, + detail: "boom", + fileId: "f-1", + sourceId: null, + policyId: null, + occurrences: 1, + createdAt: "2026-08-05T00:00:00Z", + lastSeenAt: "2026-08-05T00:00:00Z", + actions: [], + ...overrides, + }; +} + +describe("useNotifications", () => { + beforeEach(() => { + window.localStorage.clear(); + fetchNotifications.mockReset().mockResolvedValue([]); + hasLocalFile.mockClear(); + }); + + it("reads the list once however many bells are mounted", async () => { + fetchNotifications.mockResolvedValue([notification("a")]); + + const first = renderHook(() => useNotifications()); + const second = renderHook(() => useNotifications()); + + await waitFor(() => + expect(first.result.current.notifications).toHaveLength(1), + ); + expect(second.result.current.notifications).toHaveLength(1); + expect(fetchNotifications).toHaveBeenCalledTimes(1); + }); + + it("looks a document up once for the list, not once per row", async () => { + fetchNotifications.mockResolvedValue([ + notification("a", { fileId: "f-1" }), + notification("b", { fileId: "f-1" }), + notification("c", { fileId: "f-2" }), + ]); + + const { result } = renderHook(() => useNotifications()); + + await waitFor(() => expect(result.current.notifications).toHaveLength(3)); + expect(hasLocalFile).toHaveBeenCalledTimes(2); + }); + + it("looks up an attended run's document but never an unattended run's", async () => { + // Asking storage about a source's hash can only miss, and would then be shown as "not on this + // device" about a document that never was. + fetchNotifications.mockResolvedValue([ + notification("attended", { + origin: "POLICY", + sourceId: null, + fileId: "editor-file-1", + }), + notification("unattended", { + origin: "POLICY", + sourceId: "src-s3-invoices", + fileId: "hashed-identity", + }), + ]); + + const { result } = renderHook(() => useNotifications()); + + await waitFor(() => expect(result.current.notifications).toHaveLength(2)); + expect(hasLocalFile).toHaveBeenCalledTimes(1); + expect(hasLocalFile).toHaveBeenCalledWith("editor-file-1"); + expect( + result.current.documentStateFor(result.current.notifications[0]) + .hasLocalFile, + ).toBe(true); + expect( + result.current.documentStateFor(result.current.notifications[1]) + .hasLocalFile, + ).toBe(false); + }); + + it("polls on one timer and stops it when the last bell unmounts", async () => { + vi.useFakeTimers(); + try { + const first = renderHook(() => useNotifications()); + const second = renderHook(() => useNotifications()); + await act(async () => {}); + expect(fetchNotifications).toHaveBeenCalledTimes(1); + + // Two bells, one tick: a timer per subscriber would read twice here. + await act(async () => { + vi.advanceTimersByTime(30_000); + }); + expect(fetchNotifications).toHaveBeenCalledTimes(2); + + first.unmount(); + await act(async () => { + vi.advanceTimersByTime(30_000); + }); + expect(fetchNotifications).toHaveBeenCalledTimes(3); + + second.unmount(); + await act(async () => { + vi.advanceTimersByTime(120_000); + }); + expect(fetchNotifications).toHaveBeenCalledTimes(3); + } finally { + vi.useRealTimers(); + } + }); + + it("marks every bell read, not just the one the user opened", async () => { + fetchNotifications.mockResolvedValue([ + notification("b"), + notification("a"), + ]); + const first = renderHook(() => useNotifications()); + const second = renderHook(() => useNotifications()); + await waitFor(() => expect(first.result.current.unreadCount).toBe(2)); + expect(second.result.current.unreadCount).toBe(2); + + // Async because subscribers are told on a microtask: a bell marks the list read while rendering. + await act(async () => first.result.current.markAllSeen()); + + expect(first.result.current.unreadCount).toBe(0); + expect(second.result.current.unreadCount).toBe(0); + expect( + window.localStorage.getItem("stirling.notifications.lastSeenId"), + ).toBe("b"); + }); + + it("chains one fresh read behind the read in flight rather than joining it", async () => { + // A refresh exists to observe a write the caller just made. The read in flight may have + // started before that write, so joining it would report the world without it - and the + // caller would wait a whole poll interval for news of their own action. + let release: (listed: AppNotification[]) => void = () => {}; + fetchNotifications.mockImplementationOnce( + () => + new Promise((resolve) => { + release = resolve; + }), + ); + + const first = renderHook(() => useNotifications()); + expect(fetchNotifications).toHaveBeenCalledTimes(1); + + // A refresh from a row, twice over, and a second bell mounting - all mid-read. The + // refreshes share ONE chained read; the mount joins what is already there. + fetchNotifications.mockResolvedValue([notification("a")]); + act(() => { + first.result.current.refresh(); + first.result.current.refresh(); + }); + const second = renderHook(() => useNotifications()); + expect(fetchNotifications).toHaveBeenCalledTimes(1); + + // The stale read lands empty; the chained fresh read is what delivers the row. + await act(async () => release([])); + await waitFor(() => expect(fetchNotifications).toHaveBeenCalledTimes(2)); + await waitFor(() => + expect(first.result.current.notifications).toHaveLength(1), + ); + expect(second.result.current.notifications).toHaveLength(1); + }); + + it("shows a just-reported failure without waiting for the poll", async () => { + const hook = renderHook(() => useNotifications()); + await waitFor(() => expect(fetchNotifications).toHaveBeenCalledTimes(1)); + expect(hook.result.current.unreadCount).toBe(0); + + // The failure report chain: row recorded server-side, then the re-read. + fetchNotifications.mockResolvedValue([notification("a")]); + act(() => refreshNotificationsNow()); + + await waitFor(() => expect(hook.result.current.unreadCount).toBe(1)); + }); + + it("still lands the row when the refresh races a poll read already in flight", async () => { + let releaseStale: (listed: AppNotification[]) => void = () => {}; + fetchNotifications.mockImplementationOnce( + () => + new Promise((resolve) => { + releaseStale = resolve; + }), + ); + + const hook = renderHook(() => useNotifications()); + expect(fetchNotifications).toHaveBeenCalledTimes(1); + + // The failure is recorded while a poll's read is still in flight, then its refresh fires. + // Joining that stale read would miss the row until the next poll interval. + fetchNotifications.mockResolvedValue([notification("a")]); + act(() => refreshNotificationsNow()); + await act(async () => releaseStale([])); + + await waitFor(() => expect(hook.result.current.unreadCount).toBe(1)); + }); + + it("keeps its own list rather than one left by a bell that has gone", async () => { + fetchNotifications.mockResolvedValue([notification("a")]); + const first = renderHook(() => useNotifications()); + await waitFor(() => + expect(first.result.current.notifications).toHaveLength(1), + ); + first.unmount(); + + // It must not show the old row while its own read is in flight. + fetchNotifications.mockResolvedValue([]); + const second = renderHook(() => useNotifications()); + + expect(second.result.current.notifications).toHaveLength(0); + await waitFor(() => expect(fetchNotifications).toHaveBeenCalledTimes(2)); + expect(second.result.current.notifications).toHaveLength(0); + }); +}); diff --git a/frontend/editor/src/core/hooks/useNotifications.ts b/frontend/editor/src/core/hooks/useNotifications.ts new file mode 100644 index 0000000000..9f307a9a42 --- /dev/null +++ b/frontend/editor/src/core/hooks/useNotifications.ts @@ -0,0 +1,240 @@ +import { useSyncExternalStore } from "react"; +import { + fetchNotifications, + type AppNotification, +} from "@app/services/notifications"; +import { hasLocalFile } from "@app/services/localFilePresence"; + +/** + * One polled store for however many bells are mounted. A module store rather than a context because + * the portal mounts its bell as a sibling of AppProviders, so there is no single tree to provide in. + */ + +// TODO: read state is per-browser. Move it server-side when notifications get their own table. +const POLL_INTERVAL_MS = 30_000; +const SEEN_STORAGE_KEY = "stirling.notifications.lastSeenId"; + +function readLastSeenId(): string | null { + try { + return window.localStorage.getItem(SEEN_STORAGE_KEY); + } catch { + // Private mode: everything reads as unseen, which errs towards showing failures. + return null; + } +} + +function writeLastSeenId(id: string): void { + try { + window.localStorage.setItem(SEEN_STORAGE_KEY, id); + } catch { + // The marker just will not survive a reload. + } +} + +export interface NotificationDocumentState { + hasLocalFile: boolean; +} + +const NO_DOCUMENT: NotificationDocumentState = { + hasLocalFile: false, +}; + +/** + * Whether this browser could resolve the document a row names. Two id spaces share `fileId`: an + * attended run reports the id its editor minted, a source-fed one a hash that was never on a device. + */ +export function isResolvableHere(notification: AppNotification): boolean { + return (notification.sourceId ?? null) === null; +} + +interface NotificationsSnapshot { + notifications: AppNotification[]; + /** Keyed by fileId, so several rows about one document cost one lookup. */ + documents: Record; + lastSeenId: string | null; +} + +const NOTHING_LOADED: NotificationsSnapshot = { + notifications: [], + documents: {}, + lastSeenId: null, +}; + +let snapshot: NotificationsSnapshot = NOTHING_LOADED; +const subscribers = new Set<() => void>(); +let pollTimer: number | null = null; +let inFlight: Promise | null = null; +/** Bumped when polling starts or stops, so a read from a finished cycle cannot write. */ +let cycle = 0; +let notifyQueued = false; + +function getSnapshot(): NotificationsSnapshot { + return snapshot; +} + +/** + * Subscribers told on a microtask: a bell marks the list read from inside its own state updater, and + * re-rendering the others from there is the render-phase update React refuses. + */ +function publish(next: NotificationsSnapshot): void { + snapshot = next; + if (notifyQueued) return; + notifyQueued = true; + queueMicrotask(() => { + notifyQueued = false; + subscribers.forEach((notify) => notify()); + }); +} + +async function read(forCycle: number): Promise { + const listed = await fetchNotifications(); + if (forCycle !== cycle) return; + + const fileIds = [ + ...new Set( + listed + .filter(isResolvableHere) + .map((notification) => notification.fileId) + .filter((fileId): fileId is string => fileId !== null), + ), + ]; + const resolved = await Promise.all( + fileIds.map( + async (fileId) => + [ + fileId, + { + hasLocalFile: await hasLocalFile(fileId), + }, + ] as const, + ), + ); + if (forCycle !== cycle) return; + + publish({ + ...snapshot, + notifications: listed, + documents: Object.fromEntries(resolved), + }); +} + +/** A caller arriving mid-read joins the one already running. */ +function load(): Promise { + if (inFlight) return inFlight; + const pending = read(cycle).finally(() => { + if (inFlight === pending) inFlight = null; + }); + inFlight = pending; + return pending; +} + +/** Set while a fresh read is chained behind the one in flight, so callers share it. */ +let freshReadQueued = false; + +/** + * A read that must observe a write the caller just made. It never joins a read already in + * flight, because that read may have started before the write and would report the world + * without it; a fresh read is chained behind it instead. Callers arriving in the same + * window share the one chained read. + */ +function loadFresh(): void { + const inFlightRead = inFlight; + if (!inFlightRead) { + void load(); + return; + } + if (freshReadQueued) return; + freshReadQueued = true; + void inFlightRead.finally(() => { + freshReadQueued = false; + // The last bell may have unmounted while the stale read was landing. + if (subscribers.size === 0) return; + void load(); + }); +} + +function startPolling(): void { + cycle += 1; + // From disk, not memory: another tab may have moved the marker on. + snapshot = { ...NOTHING_LOADED, lastSeenId: readLastSeenId() }; + pollTimer = window.setInterval(() => void load(), POLL_INTERVAL_MS); + void load(); +} + +function stopPolling(): void { + if (pollTimer !== null) { + window.clearInterval(pollTimer); + pollTimer = null; + } + // Drop anything in flight: its cycle has nobody watching it. + cycle += 1; + inFlight = null; + snapshot = NOTHING_LOADED; +} + +/** Polling lives exactly as long as there is a bell to show it. */ +function subscribe(onStoreChange: () => void): () => void { + subscribers.add(onStoreChange); + if (subscribers.size === 1) startPolling(); + return () => { + subscribers.delete(onStoreChange); + if (subscribers.size === 0) stopPolling(); + }; +} + +function markAllSeen(): void { + const newest = snapshot.notifications[0]; + if (!newest || snapshot.lastSeenId === newest.id) return; + writeLastSeenId(newest.id); + publish({ ...snapshot, lastSeenId: newest.id }); +} + +function refresh(): void { + // A row calls this after changing something server-side, so the read must be fresh. + loadFresh(); +} + +/** + * Re-read now, for a caller that just caused a notification: without it the person who triggered a + * failure waits a whole poll interval to hear about their own action. A no-op with no bell mounted. + */ +export function refreshNotificationsNow(): void { + if (subscribers.size === 0) return; + loadFresh(); +} + +export interface NotificationsState { + notifications: AppNotification[]; + /** Read before {@link markAllSeen}, which zeroes it. */ + unreadCount: number; + documentStateFor: ( + notification: AppNotification, + ) => NotificationDocumentState; + markAllSeen: () => void; + refresh: () => void; +} + +export function useNotifications(): NotificationsState { + const { notifications, documents, lastSeenId } = useSyncExternalStore( + subscribe, + getSnapshot, + getSnapshot, + ); + + // A marker no longer in the list means we cannot tell how far the user got, so everything reads + // as unread rather than being silently marked seen. + const seenIndex = lastSeenId + ? notifications.findIndex((n) => n.id === lastSeenId) + : -1; + const unreadCount = seenIndex === -1 ? notifications.length : seenIndex; + + return { + notifications, + unreadCount, + documentStateFor: (notification) => + (notification.fileId ? documents[notification.fileId] : null) ?? + NO_DOCUMENT, + markAllSeen, + refresh, + }; +} diff --git a/frontend/editor/src/core/i18n/translationAudit.ts b/frontend/editor/src/core/i18n/translationAudit.ts index 9dfbc5b21b..8545df99e0 100644 --- a/frontend/editor/src/core/i18n/translationAudit.ts +++ b/frontend/editor/src/core/i18n/translationAudit.ts @@ -115,6 +115,9 @@ export const I18N_PROJECTS: TranslationProject[] = [ // invisible to the static scan. The raw catalogue value is the fallback. /^policies\.field\./, /^policyOption\./, + // A failure's disabled reason arrives from the server as a key and is rendered with + // t(thatKey), so nothing in source names it, but the copy still has to exist. + /^portal\.failures\.disabled\./, ], minUsedKeys: 100, minLocaleKeys: 100, diff --git a/frontend/editor/src/core/routes/portalBasename.ts b/frontend/editor/src/core/routes/portalBasename.ts index c9ad3c44a7..a4fdb87a52 100644 --- a/frontend/editor/src/core/routes/portalBasename.ts +++ b/frontend/editor/src/core/routes/portalBasename.ts @@ -5,3 +5,9 @@ * portal (core, desktop, prototypes) must never resolve @portal. */ export const PORTAL_BASENAME = "/processor"; + +/** + * The recorded-failures section of the portal's Documents view. Here because whoever links to it and + * whoever renders it are in different layers. + */ +export const PORTAL_FAILURES_ANCHOR = "failures"; diff --git a/frontend/editor/src/core/services/localFilePresence.test.ts b/frontend/editor/src/core/services/localFilePresence.test.ts new file mode 100644 index 0000000000..1f00d03c0b --- /dev/null +++ b/frontend/editor/src/core/services/localFilePresence.test.ts @@ -0,0 +1,36 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import "fake-indexeddb/auto"; + +/** + * Tests for the one thing the bell asks about a failed document here: whether it is + * still in this browser, which is what decides if it can be opened. + */ + +const getStirlingFileStub = vi.fn(); + +vi.mock("@app/services/fileStorage", () => ({ + fileStorage: { + getStirlingFileStub: (...args: unknown[]) => getStirlingFileStub(...args), + }, +})); + +const { hasLocalFile } = await import("@app/services/localFilePresence"); + +beforeEach(() => { + getStirlingFileStub.mockReset().mockResolvedValue(null); +}); + +describe("hasLocalFile", () => { + it("is false once the document has left this browser", async () => { + getStirlingFileStub.mockResolvedValue(null); + + await expect(hasLocalFile("f-1")).resolves.toBe(false); + await expect(hasLocalFile(null)).resolves.toBe(false); + }); + + it("is true while the document is still stored here", async () => { + getStirlingFileStub.mockResolvedValue({ id: "f-1", name: "doc.pdf" }); + + await expect(hasLocalFile("f-1")).resolves.toBe(true); + }); +}); diff --git a/frontend/editor/src/core/services/localFilePresence.ts b/frontend/editor/src/core/services/localFilePresence.ts new file mode 100644 index 0000000000..9242c4ca59 --- /dev/null +++ b/frontend/editor/src/core/services/localFilePresence.ts @@ -0,0 +1,18 @@ +import { fileStorage } from "@app/services/fileStorage"; +import type { FileId } from "@app/types/file"; + +/** Whether the document is still in this browser. The id is this workspace's own, so only it can say. */ +export async function hasLocalFile(fileId: string | null): Promise { + if (!isUsableId(fileId)) return false; + + try { + const stub = await fileStorage.getStirlingFileStub(fileId as FileId); + return stub !== null; + } catch { + return false; + } +} + +function isUsableId(fileId: string | null | undefined): fileId is string { + return typeof fileId === "string" && fileId.trim() !== ""; +} diff --git a/frontend/editor/src/core/services/notifications.ts b/frontend/editor/src/core/services/notifications.ts new file mode 100644 index 0000000000..3101c6056e --- /dev/null +++ b/frontend/editor/src/core/services/notifications.ts @@ -0,0 +1,66 @@ +import apiClient from "@app/services/apiClient"; + +// Derived server-side from whatever produces them, so this client knows nothing about failures. +const NOTIFICATIONS_PATH = "/api/v1/notifications"; + +export type NotificationSource = "FAILURE"; + +export type NotificationSeverity = "ERROR" | "WARNING" | "INFO"; + +export type NotificationOrigin = "TOOL" | "POLICY" | "PIPELINE"; + +/** From this reader's point of view. `UNOWNED` is an unattended run: nobody holds the file. */ +export type NotificationOwnership = "MINE" | "THEIRS" | "UNOWNED"; + +/** `id` is an open string, not a union: the server may know actions this build does not. */ +export interface NotificationActionOffer { + id: string; + labelKey: string; + /** English fallback, for a build with no copy for `labelKey`. */ + defaultLabel: string; + /** False renders no button in the bell, and a disabled one in the portal's queue. */ + enabled: boolean; + disabledReasonKey: string | null; +} + +export interface AppNotification { + /** Prefixed with its source (`failure:`), so it is never an id a per-source endpoint takes. */ + id: string; + source: NotificationSource; + /** Open string, e.g. `INPUT_PASSWORD_PROTECTED`: the server adds kinds without a client change. */ + kindId: string; + origin: NotificationOrigin; + ownership: NotificationOwnership; + severity: NotificationSeverity; + status: string; + titleKey: string; + defaultTitle: string; + detail: string | null; + /** Two id spaces share this field, and `sourceId` says which: see `isResolvableHere`. */ + fileId: string | null; + /** Which folder, bucket or webhook fed the run, and null for an attended one. */ + sourceId: string | null; + policyId: string | null; + occurrences: number; + createdAt: string; + lastSeenAt: string; + actions: NotificationActionOffer[]; +} + +interface NotificationsResponse { + notifications: AppNotification[]; +} + +/** Newest first. Empty rather than throwing: a bell that cannot load is an empty bell, not an error. */ +export async function fetchNotifications( + limit = 20, +): Promise { + try { + const response = await apiClient.get( + `${NOTIFICATIONS_PATH}?limit=${limit}`, + ); + return response?.data?.notifications ?? []; + } catch { + return []; + } +} diff --git a/frontend/editor/src/core/tests/helpers/api-stubs.ts b/frontend/editor/src/core/tests/helpers/api-stubs.ts index d6ebfbe408..fc476af535 100644 --- a/frontend/editor/src/core/tests/helpers/api-stubs.ts +++ b/frontend/editor/src/core/tests/helpers/api-stubs.ts @@ -273,6 +273,12 @@ export async function mockAppApis( await page.route("**/api/v1/policies/runs", (route: Route) => route.fulfill({ json: [] }), ); + + // The bell polls this on load. The hook swallows the failure, but the browser still logs the + // request, which the console-hygiene guard counts. + await page.route("**/api/v1/notifications*", (route: Route) => + route.fulfill({ json: { notifications: [] } }), + ); } /** diff --git a/frontend/editor/src/core/theme/colors.css b/frontend/editor/src/core/theme/colors.css index dd3c2aec7c..8e416af124 100644 --- a/frontend/editor/src/core/theme/colors.css +++ b/frontend/editor/src/core/theme/colors.css @@ -38,6 +38,7 @@ html[data-app-theme="light"] { --c-success: var(--p-green-600); --c-danger: var(--p-red-600); + --c-text-on-danger: var(--p-white); --c-warning: var(--p-amber-600); /* Solid fills that carry a white label. Deeper than the --c- values above, which are picked for surfaces, borders and icons where the 3:1 @@ -169,6 +170,7 @@ html[data-app-theme="midnight"] { --c-success: var(--p-green-500); --c-danger: var(--p-red-500); + --c-text-on-danger: var(--p-white); --c-warning: var(--p-amber-500); /* Themed decorative dark overrides (see :root for light + rationale). */ @@ -371,6 +373,7 @@ html[data-app-theme="custom"][data-mantine-color-scheme="dark"] { /* Dark-tuned status shades (lighter than the light-theme :root values). */ --c-success: var(--p-green-500); --c-danger: var(--p-red-400); + --c-text-on-danger: var(--p-white); --c-warning: var(--p-amber-500); /* Themed decorative dark overrides (see :root for light + rationale). */ diff --git a/frontend/editor/src/core/types/fileContext.ts b/frontend/editor/src/core/types/fileContext.ts index c482e8e54a..b4a48e3d00 100644 --- a/frontend/editor/src/core/types/fileContext.ts +++ b/frontend/editor/src/core/types/fileContext.ts @@ -337,6 +337,11 @@ export interface FileContextActions { insertAfterPageId?: string; selectFiles?: boolean; skipUploadTracking?: boolean; + /** + * Produced in-app rather than uploaded, which stops the policy auto-run enforcing an upload + * policy on it. Set by anything adding a file already through a policy or a tool. + */ + derivedFromTool?: boolean; }, ) => Promise; addFilesWithOptions: ( diff --git a/frontend/editor/src/core/ui/BellIcon.tsx b/frontend/editor/src/core/ui/BellIcon.tsx new file mode 100644 index 0000000000..87908c4af6 --- /dev/null +++ b/frontend/editor/src/core/ui/BellIcon.tsx @@ -0,0 +1,22 @@ +/** + * An outline bell. The bundled Material Symbols set only carries the filled variant, + * which reads as permanently ringing. + */ +export function BellIcon({ size = 18 }: { size?: number }) { + return ( + + + + + ); +} diff --git a/frontend/editor/src/core/ui/index.ts b/frontend/editor/src/core/ui/index.ts index dcf0e9265f..c621319f0e 100644 --- a/frontend/editor/src/core/ui/index.ts +++ b/frontend/editor/src/core/ui/index.ts @@ -1,5 +1,6 @@ export * from "@app/ui/Button"; export * from "@app/ui/ActionIcon"; +export * from "@app/ui/BellIcon"; export * from "@app/ui/Logo"; export * from "@app/ui/FilePicker"; export * from "@app/ui/SegmentedControl"; diff --git a/frontend/editor/src/portal/components/AppShell.css b/frontend/editor/src/portal/components/AppShell.css index d3f1332969..ffbfc2fb21 100644 --- a/frontend/editor/src/portal/components/AppShell.css +++ b/frontend/editor/src/portal/components/AppShell.css @@ -14,7 +14,9 @@ color: var(--c-text-muted); } +/* Anchors .portal-shell__notifications without disturbing the column's flow. */ .portal-shell__main { + position: relative; flex: 1 1 auto; display: flex; flex-direction: column; @@ -69,3 +71,12 @@ animation: fadeIn var(--motion-fast) both; } } + +/* Sits over the main area's top-right corner. Outside .portal-shell__view so it stays put while + the view scrolls, and inside .portal-shell__main so it never overlaps the sidebar. */ +.portal-shell__notifications { + position: absolute; + top: var(--sp-3, 0.75rem); + right: var(--sp-4, 1rem); + z-index: var(--z-sticky, 30); +} diff --git a/frontend/editor/src/portal/components/AppShell.tsx b/frontend/editor/src/portal/components/AppShell.tsx index 1811fa02c4..fe8f775fd0 100644 --- a/frontend/editor/src/portal/components/AppShell.tsx +++ b/frontend/editor/src/portal/components/AppShell.tsx @@ -8,6 +8,7 @@ import { useUI } from "@portal/contexts/UIContext"; import { MenuIcon, SearchIcon } from "@portal/components/icons"; import { Logo } from "@app/ui/Logo"; import "@portal/components/AppShell.css"; +import { NotificationBell } from "@app/components/notifications/NotificationBell"; /** * Compact header shown only under the mobile breakpoint (CSS-hidden on @@ -89,6 +90,9 @@ export function AppShell({ children }: { children: ReactNode }) {
    +
    + +
    {children}
    diff --git a/frontend/editor/src/portal/components/failures/FileRunEventList.test.tsx b/frontend/editor/src/portal/components/failures/FileRunEventList.test.tsx index 9ec3af6842..d60f855566 100644 --- a/frontend/editor/src/portal/components/failures/FileRunEventList.test.tsx +++ b/frontend/editor/src/portal/components/failures/FileRunEventList.test.tsx @@ -1,13 +1,19 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import { render as baseRender, screen, waitFor } from "@testing-library/react"; +import { MemoryRouter } from "react-router-dom"; import { PortalTestProviders } from "@portal/test/TestQueryProvider"; import type { FileRunEvent } from "@portal/api/fileRunEvents"; /** * Tests for the list: the states it survives (loading, empty, no registry, refused), - * plus replacing a row in place after acting and re-reading when the server refuses. + * plus replacing a row in place after acting, re-reading when the server refuses, and + * bringing itself into view when a notification links to it. */ +// jsdom does no layout and so implements no scrollIntoView. +const scrollIntoView = vi.fn(); +Element.prototype.scrollIntoView = scrollIntoView; + const fetchFileRunEvents = vi.fn(); const applyFileRunEventAction = vi.fn(); @@ -55,9 +61,19 @@ vi.mock("react-i18next", () => ({ }), })); -// The list reads through the shared query hooks, and @app/ui needs Mantine. -const render = (ui: Parameters[0]) => - baseRender(ui, { wrapper: PortalTestProviders }); +// The list reads through the shared query hooks, @app/ui needs Mantine, and the section reads +// the location to know whether it was linked to. +const render = ( + ui: Parameters[0], + at = "/processor/documents", +) => + baseRender(ui, { + wrapper: ({ children }) => ( + + {children} + + ), + }); const { FileRunEventList } = await import("@portal/components/failures/FileRunEventList"); @@ -101,6 +117,7 @@ describe("FileRunEventList", () => { beforeEach(() => { fetchFileRunEvents.mockReset(); applyFileRunEventAction.mockReset(); + scrollIntoView.mockReset(); // The dev-panel test stubs import.meta.env.DEV, which would otherwise persist // into every test after it. vi.unstubAllEnvs(); @@ -215,6 +232,24 @@ describe("FileRunEventList", () => { expect(fetchFileRunEvents).toHaveBeenCalledTimes(1); }); + it("brings itself into view when a notification links to it", async () => { + // It sits below the review queue, so landing on the page is not the same as seeing it. + fetchFileRunEvents.mockResolvedValue([event()]); + + render(, "/processor/documents#failures"); + + await waitFor(() => expect(scrollIntoView).toHaveBeenCalled()); + }); + + it("stays where it is on an ordinary visit to the page", async () => { + fetchFileRunEvents.mockResolvedValue([event()]); + + render(); + + await screen.findByText("Password-protected document"); + expect(scrollIntoView).not.toHaveBeenCalled(); + }); + it("re-reads from the server when an action is refused", async () => { // A 409 means someone else closed it first; the server's view wins. fetchFileRunEvents.mockResolvedValue([event()]); diff --git a/frontend/editor/src/portal/components/failures/FileRunEventList.tsx b/frontend/editor/src/portal/components/failures/FileRunEventList.tsx index 2330546655..31c0671f0b 100644 --- a/frontend/editor/src/portal/components/failures/FileRunEventList.tsx +++ b/frontend/editor/src/portal/components/failures/FileRunEventList.tsx @@ -1,6 +1,8 @@ -import { useState } from "react"; +import { useEffect, useRef, useState } from "react"; +import { useLocation } from "react-router-dom"; import { useTranslation } from "react-i18next"; import { Button, EmptyState, Skeleton, StatusBadge } from "@app/ui"; +import { PORTAL_FAILURES_ANCHOR } from "@app/routes/portalBasename"; import type { FileRunEvent, FailureSeverity } from "@portal/api/fileRunEvents"; import { useFileRunEvents, @@ -30,6 +32,15 @@ export function FileRunEventList() { const [busy, setBusy] = useState<{ id: string; action: string } | null>(null); const [showJson, setShowJson] = useState(false); const [clearing, setClearing] = useState(false); + const section = useRef(null); + const { hash, key } = useLocation(); + + // A fragment is only honoured on a real page load, not a client-side route change. Keyed on the + // navigation too: a second notification changes neither the path nor the hash. + useEffect(() => { + if (hash !== `#${PORTAL_FAILURES_ANCHOR}`) return; + section.current?.scrollIntoView({ behavior: "smooth", block: "start" }); + }, [hash, key]); // A build without the proprietary module has no such route, and a caller who is // not a team leader gets a 403. Both mean there is nothing to show. @@ -122,7 +133,11 @@ export function FileRunEventList() { } return ( -
    +

    {t("portal.failures.title", "Failures")}

    diff --git a/frontend/editor/src/portal/views/Documents.tsx b/frontend/editor/src/portal/views/Documents.tsx index f47abd3ffe..6bd394c13d 100644 --- a/frontend/editor/src/portal/views/Documents.tsx +++ b/frontend/editor/src/portal/views/Documents.tsx @@ -107,9 +107,13 @@ export function Documents() { - {/* Recorded policy-run failures. Not mounted outside dev: the endpoints are live and - gated, but the surface itself is unfinished (no polling, no paging, no filters). - Vite folds this to false in a build, so neither the view nor its fetch ships. */} + {/* Recorded policy, pipeline and editor failures. DEV ONLY, deliberately: this list is a + stand-in until failures get their own review screen, and it is not the surface we want to + ship. The endpoints behind it are live and gated, so nothing here is load-bearing. + + Vite folds this to false in a build, so neither the section nor its fetch ships. The bell's + "View in processor" action is gated the same way and for the same reason - lift both + together when the review screen lands, or that button navigates nowhere. */} {import.meta.env.DEV && } ); diff --git a/frontend/editor/src/proprietary/components/notifications/notificationActions.test.tsx b/frontend/editor/src/proprietary/components/notifications/notificationActions.test.tsx new file mode 100644 index 0000000000..950a8d41ff --- /dev/null +++ b/frontend/editor/src/proprietary/components/notifications/notificationActions.test.tsx @@ -0,0 +1,271 @@ +import type { ReactNode } from "react"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { renderHook } from "@testing-library/react"; +import { MemoryRouter } from "react-router-dom"; +import type { + AppNotification, + NotificationActionOffer, +} from "@app/services/notifications"; +import type { NotificationActionContext } from "@core/components/notifications/notificationActions"; + +/** + * Where each action sends the reader. Only the editor has the workbench contexts above it, so the two + * shells are the interesting cases: opening the document, or handing it over. + */ + +const navigate = vi.fn(); +vi.mock("react-router-dom", async () => ({ + ...(await vi.importActual( + "react-router-dom", + )), + useNavigate: () => navigate, +})); + +// No i18n instance here, and the plugin is stubbed because the contexts below reach `core/i18n`. +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ + t: (_key: string, fallback: string) => fallback, + }), + initReactI18next: { type: "3rdParty", init: () => {} }, +})); + +// IndexedDB, which jsdom has none of. Answered here so presence is a fact of the test. +const h = vi.hoisted(() => ({ + stub: { id: "f-1" } as unknown, + getStirlingFileStub: vi.fn(), +})); + +vi.mock("@app/services/fileStorage", () => ({ + fileStorage: { + getStirlingFileStub: (fileId: string) => h.getStirlingFileStub(fileId), + }, +})); + +const { FileActionsContext, FileStoreContext } = + await import("@app/contexts/file/contexts"); +const { NavigationActionsContext } = + await import("@app/contexts/NavigationContext"); +const { ViewerContext } = await import("@app/contexts/ViewerContext"); +const { useNotificationActions } = + await import("@app/components/notifications/notificationActions"); + +const addStirlingFileStubs = vi.fn(); +const setActiveFileId = vi.fn(); +const setWorkbench = vi.fn(); +/** What the workbench already holds, so the "do not add it twice" path can be exercised. */ +let openFileIds: string[] = []; + +function notification( + overrides: Partial = {}, +): AppNotification { + return { + id: "failure:evt-1", + source: "FAILURE", + kindId: "INPUT_PASSWORD_PROTECTED", + origin: "TOOL", + ownership: "MINE", + severity: "ERROR", + status: "NEW", + titleKey: "portal.failures.kind.inputPasswordProtected.title", + defaultTitle: "Password-protected document", + detail: "The PDF Document is passworded", + fileId: "f-1", + sourceId: null, + policyId: null, + occurrences: 3, + createdAt: "2026-08-06T00:00:00Z", + lastSeenAt: "2026-08-06T00:00:00Z", + actions: [], + ...overrides, + }; +} + +function offer(id: string): NotificationActionOffer { + return { + id, + labelKey: `portal.failures.action.${id.toLowerCase()}`, + defaultLabel: id, + enabled: true, + disabledReasonKey: null, + }; +} + +function context( + overrides: Partial = {}, +): NotificationActionContext { + return { + notification: notification(), + hasLocalFile: true, + ...overrides, + }; +} + +/** The editor shell: the workbench's providers all sit above the bell. */ +const inEditor = ({ children }: { children: ReactNode }) => ( + + + ({ files: { ids: openFileIds } }), + subscribe: () => () => {}, + selectors: {}, + } as never + } + > + + + {children} + + + + + +); + +/** The processor shell: the portal mounts above the app's providers, so there is none. */ +const inProcessor = ({ children }: { children: ReactNode }) => ( + {children} +); + +function registry(wrapper = inEditor) { + return renderHook(() => useNotificationActions(), { wrapper }).result.current; +} + +beforeEach(() => { + navigate.mockReset(); + addStirlingFileStubs.mockReset().mockResolvedValue([]); + setActiveFileId.mockReset(); + setWorkbench.mockReset(); + h.getStirlingFileStub.mockReset().mockResolvedValue(h.stub); + openFileIds = []; + window.sessionStorage.clear(); + window.history.pushState({}, "", "/"); +}); + +describe("useNotificationActions", () => { + it("offers to open the document only while it is still in this browser", () => { + const actions = registry(); + + expect(actions.VIEW_FILE?.available(context())).toBe(true); + expect(actions.VIEW_FILE?.available(context({ hasLocalFile: false }))).toBe( + false, + ); + }); + + it("leaves View file as the only usable offer when the server offers actions this build cannot run", () => { + // An id this build wires nothing for drops out rather than rendering dead. + const actions = registry(); + const usable = [offer("QUARANTINE"), offer("VIEW_FILE")].filter( + (candidate) => actions[candidate.id]?.available(context()) ?? false, + ); + + expect(usable.map((candidate) => candidate.id)).toEqual(["VIEW_FILE"]); + }); + + it("opens the document into the viewer when an editor is above", async () => { + await registry().VIEW_FILE?.run(context()); + + // Selecting alone shows nothing: the workbench holds neither the file nor the viewer yet. + expect(addStirlingFileStubs).toHaveBeenCalledWith([h.stub]); + expect(setActiveFileId).toHaveBeenCalledWith("f-1"); + expect(setWorkbench).toHaveBeenCalledWith("viewer"); + }); + + it("stays where it is rather than routing through the role-based root", async () => { + // "/" lands on a page chosen by the reader's role, which reads as the app reloading. + await registry().VIEW_FILE?.run(context()); + + expect(window.location.pathname).toBe("/"); + expect(navigate).not.toHaveBeenCalled(); + }); + + it("does not add a document the workbench is already holding", async () => { + openFileIds = ["f-1"]; + + await registry().VIEW_FILE?.run(context()); + + expect(addStirlingFileStubs).not.toHaveBeenCalled(); + // Still brought to the front: the point of the click is to look at it. + expect(setActiveFileId).toHaveBeenCalledWith("f-1"); + expect(setWorkbench).toHaveBeenCalledWith("viewer"); + }); + + it("reports a document that has gone from storage instead of opening nothing", async () => { + h.getStirlingFileStub.mockResolvedValue(null); + + const outcome = await registry().VIEW_FILE?.run(context()); + + expect(outcome).toEqual({ ok: false }); + expect(setWorkbench).not.toHaveBeenCalled(); + }); + + it("hands the document over to the editor when there is no workbench above it", async () => { + await registry(inProcessor).VIEW_FILE?.run(context()); + + // The intent outlives the navigation that mounts the editor. + expect( + window.sessionStorage.getItem("stirling.notifications.pendingSelection"), + ).toBe("f-1"); + // The editor's own URL, not the role router at "/". + expect(window.location.pathname).toBe("/editor"); + }); + + it("picks up a handed-over document as soon as an editor is there", async () => { + window.sessionStorage.setItem( + "stirling.notifications.pendingSelection", + "f-9", + ); + + registry(); + await vi.waitFor(() => expect(setActiveFileId).toHaveBeenCalledWith("f-9")); + + expect(setWorkbench).toHaveBeenCalledWith("viewer"); + // One-shot: a later mount must not reopen a document the user has moved on from. + expect( + window.sessionStorage.getItem("stirling.notifications.pendingSelection"), + ).toBeNull(); + }); + + it("says it cannot hand the document over rather than navigating to nothing", async () => { + // Spied on the prototype: jsdom's storage is a proxy, so an own-property spy does not take. + const setItem = vi + .spyOn(Storage.prototype, "setItem") + .mockImplementation(() => { + throw new Error("denied"); + }); + + const outcome = await registry(inProcessor).VIEW_FILE?.run(context()); + + expect(outcome).toEqual({ + ok: false, + message: + "This browser will not let the processor pass the document to the editor. Open it from the editor instead.", + }); + // Still on the page it started on, so the failure is visible. + expect(window.location.pathname).toBe("/"); + setItem.mockRestore(); + }); + + it("links to the recorded failures section of the processor", () => { + registry().VIEW_IN_PROCESSOR?.run(context()); + + expect(navigate).toHaveBeenCalledWith("/processor/documents#failures"); + }); + + it("offers the processor link whenever the server did", () => { + // The server only sends it to someone it will let read the queue. + expect( + registry(inProcessor).VIEW_IN_PROCESSOR?.available( + context({ hasLocalFile: false }), + ), + ).toBe(true); + }); +}); diff --git a/frontend/editor/src/proprietary/components/notifications/notificationActions.ts b/frontend/editor/src/proprietary/components/notifications/notificationActions.ts new file mode 100644 index 0000000000..5f34561a5a --- /dev/null +++ b/frontend/editor/src/proprietary/components/notifications/notificationActions.ts @@ -0,0 +1,155 @@ +import { useCallback, useContext, useEffect, useMemo } from "react"; +import { useTranslation } from "react-i18next"; +import { useNavigate } from "react-router-dom"; +import { withBasePath } from "@app/constants/app"; +import { + FileActionsContext, + FileStoreContext, +} from "@app/contexts/file/contexts"; +import { NavigationActionsContext } from "@app/contexts/NavigationContext"; +import { ViewerContext } from "@app/contexts/ViewerContext"; +import { + PORTAL_BASENAME, + PORTAL_FAILURES_ANCHOR, +} from "@app/routes/portalBasename"; +import { EDITOR_BASENAME } from "@app/routes/editorBasename"; +import { fileStorage } from "@app/services/fileStorage"; +import type { FileId } from "@app/types/file"; +import { + type ClientActionOutcome, + type ClientActionRegistry, + type ClientActionSpec, + type NotificationActionContext, +} from "@core/components/notifications/notificationActions"; + +export { + type ClientActionOutcome, + type ClientActionRegistry, + type ClientActionSpec, + type NotificationActionContext, +}; + +/** + * The portal mounts as a sibling of `AppProviders`, so in the processor shell none of the workbench + * contexts exist above this hook. That is why contexts are read raw and a document is handed over. + */ + +const HANDOFF_KEY = "stirling.notifications.pendingSelection"; + +const FAILURES_DESTINATION = `${PORTAL_BASENAME}/documents#${PORTAL_FAILURES_ANCHOR}`; + +/** False when storage refused it: navigating anyway lands the user in an editor with nothing open. */ +function stashSelection(fileId: string): boolean { + try { + window.sessionStorage.setItem(HANDOFF_KEY, fileId); + return true; + } catch { + return false; + } +} + +function takeSelection(): string | null { + try { + const fileId = window.sessionStorage.getItem(HANDOFF_KEY); + if (fileId !== null) window.sessionStorage.removeItem(HANDOFF_KEY); + return fileId; + } catch { + return null; + } +} + +/** + * Not the router's `navigate`: the editor reads its tool from the URL on mount and on a history pop, + * and a router push is neither, so the address would change and the workbench would not. + */ +function goToEditor(path: string): void { + window.history.pushState({}, "", withBasePath(path)); + window.dispatchEvent(new PopStateEvent("popstate")); +} + +export function useNotificationActions(): ClientActionRegistry { + const { t } = useTranslation(); + const navigate = useNavigate(); + // Raw, because the hooks that wrap these throw when there is no provider, and in the processor + // shell there is none. All four are present together or not at all. + const fileContext = useContext(FileActionsContext); + const fileStore = useContext(FileStoreContext); + const navigation = useContext(NavigationActionsContext); + const viewer = useContext(ViewerContext); + const canOpenHere = Boolean(fileContext && fileStore && navigation && viewer); + + /** + * Opens the way the file sidebar does. Selecting alone shows nothing: an id the workbench does not + * hold has nothing to render, and the workbench keeps whatever view it was on. + */ + const openInWorkbench = useCallback( + async (fileId: string): Promise => { + if (!fileContext || !fileStore || !navigation || !viewer) return false; + + const stub = await fileStorage.getStirlingFileStub(fileId as FileId); + if (!stub) return false; + + const alreadyOpen = fileStore + .getState() + .files.ids.some((id) => (id as string) === fileId); + if (!alreadyOpen) { + await fileContext.actions.addStirlingFileStubs([stub]); + } + viewer.setActiveFileId(fileId); + navigation.actions.setWorkbench("viewer"); + return true; + }, + [fileContext, fileStore, navigation, viewer], + ); + + // One-shot: read and cleared, so a later render cannot reopen a file the user has moved on from. + useEffect(() => { + if (!canOpenHere) return; + const fileId = takeSelection(); + if (fileId) void openInWorkbench(fileId); + }, [canOpenHere, openInWorkbench]); + + return useMemo(() => { + const openDocument = async ( + fileId: string | null, + ): Promise => { + if (!fileId) return; + + // In place, with no navigation: "/" is the role-based router, so going there reads as the app + // reloading and lands the user wherever their role says rather than on their document. + if (canOpenHere) { + return (await openInWorkbench(fileId)) ? undefined : { ok: false }; + } + + if (!stashSelection(fileId)) { + return { + ok: false, + message: t( + "notifications.handoffUnavailable", + "This browser will not let the processor pass the document to the editor. Open it from the editor instead.", + ), + }; + } + goToEditor(EDITOR_BASENAME); + }; + + const viewFile: ClientActionSpec = { + available: (context) => context.hasLocalFile, + closesPanel: true, + run: (context) => openDocument(context.notification.fileId), + }; + + const viewInProcessor: ClientActionSpec = { + // Its destination is dev-only until failures get a review screen; the other half of this gate + // is in portal/views/Documents, and both lift together. + available: () => import.meta.env.DEV, + closesPanel: true, + run: () => navigate(FAILURES_DESTINATION), + }; + + return { + VIEW_FILE: viewFile, + VIEW_IN_PROCESSOR: viewInProcessor, + }; + }, [canOpenHere, openInWorkbench, navigate, t]); +} diff --git a/frontend/editor/src/proprietary/components/notifications/useNotificationsAvailable.ts b/frontend/editor/src/proprietary/components/notifications/useNotificationsAvailable.ts new file mode 100644 index 0000000000..2e21f0f33e --- /dev/null +++ b/frontend/editor/src/proprietary/components/notifications/useNotificationsAvailable.ts @@ -0,0 +1,7 @@ +/** + * This build ships the failure registry and the notification routes, so the bell has + * something to read and may mount. + */ +export function useNotificationsAvailable(): boolean { + return true; +} diff --git a/frontend/editor/src/proprietary/components/policies/usePolicyAutoRun.chain.test.tsx b/frontend/editor/src/proprietary/components/policies/usePolicyAutoRun.chain.test.tsx index b87cbda445..a6f78392be 100644 --- a/frontend/editor/src/proprietary/components/policies/usePolicyAutoRun.chain.test.tsx +++ b/frontend/editor/src/proprietary/components/policies/usePolicyAutoRun.chain.test.tsx @@ -91,7 +91,12 @@ describe("auto-run ordered chaining", () => { // The first policy (order 0) runs on the upload; the second waits for the chain. expect(runStored).toHaveBeenCalledTimes(1); - expect(runStored).toHaveBeenCalledWith("backend-sec", [{ size: 100 }]); + // Recorded against a document this browser can resolve, which is what makes its failure actionable. + expect(runStored).toHaveBeenCalledWith( + "backend-sec", + [{ size: 100 }], + "file-1", + ); }); it("chains the next policy onto a completed run's output", async () => { @@ -120,8 +125,12 @@ describe("auto-run ordered chaining", () => { await vi.advanceTimersByTimeAsync(1); }); - // The next policy (order 1) fires on the first policy's output, not the original. - expect(runStored).toHaveBeenCalledWith("backend-cls", [{ size: 100 }]); + // Fires on the first policy's output and reports that output's own id, not the original's. + expect(runStored).toHaveBeenCalledWith( + "backend-cls", + [{ size: 100 }], + "file-1-v2", + ); }); it("keeps classification out of the server chain when the AI engine is off", async () => { @@ -136,10 +145,31 @@ describe("auto-run ordered chaining", () => { await vi.advanceTimersByTimeAsync(1); }); - expect(runStored).toHaveBeenCalledWith("backend-sec", [{ size: 100 }]); + expect(runStored).toHaveBeenCalledWith( + "backend-sec", + [{ size: 100 }], + "file-1", + ); expect(runStored).not.toHaveBeenCalledWith( "backend-cls", expect.anything(), + expect.anything(), ); }); + + it("never dispatches on a file marked derivedFromTool", async () => { + // A policy run is billed, so this gate is what stops `importOutputs` re-enforcing a policy on + // its own output forever. If this fails, fix the gate rather than the test. + setFileStubs([ + { id: "file-1", name: "unlocked.pdf", derivedFromTool: true }, + ]); + runStored.mockResolvedValue("run-sec"); + + renderHook(() => usePolicyAutoRun()); + await act(async () => { + await vi.advanceTimersByTimeAsync(1); + }); + + expect(runStored).not.toHaveBeenCalled(); + }); }); diff --git a/frontend/editor/src/proprietary/components/policies/usePolicyAutoRun.retry.test.tsx b/frontend/editor/src/proprietary/components/policies/usePolicyAutoRun.retry.test.tsx index 3c54e5cc42..b837b4beba 100644 --- a/frontend/editor/src/proprietary/components/policies/usePolicyAutoRun.retry.test.tsx +++ b/frontend/editor/src/proprietary/components/policies/usePolicyAutoRun.retry.test.tsx @@ -113,7 +113,13 @@ describe("auto-run queue-rejection retry", () => { await act(async () => { await vi.advanceTimersByTimeAsync(6000); }); - expect(runStored).toHaveBeenCalledWith("backend-1", [{ size: 1234 }]); + // The workspace id travels with the retry too, so a failure of it can still name the + // document this browser is holding. + expect(runStored).toHaveBeenCalledWith( + "backend-1", + [{ size: 1234 }], + "file-1", + ); expect(getRun("run-1")).toBeUndefined(); expect(getRun("run-2")?.status).toBe("RUNNING"); }); diff --git a/frontend/editor/src/proprietary/components/policies/usePolicyAutoRun.ts b/frontend/editor/src/proprietary/components/policies/usePolicyAutoRun.ts index 8140691b6d..936fef8119 100644 --- a/frontend/editor/src/proprietary/components/policies/usePolicyAutoRun.ts +++ b/frontend/editor/src/proprietary/components/policies/usePolicyAutoRun.ts @@ -10,6 +10,7 @@ import { useFileContext, } from "@app/contexts/FileContext"; import { fileStorage } from "@app/services/fileStorage"; +import { refreshNotificationsNow } from "@app/hooks/useNotifications"; import { useIndexedDB } from "@app/contexts/IndexedDBContext"; import i18n from "@app/i18n"; import { @@ -241,6 +242,8 @@ export function usePolicyAutoRun(): void { dispatchKey(finished.categoryId, finished.fileId), ); } + // Read now rather than leaving them a poll interval to hear about their own upload. + if (view.status === "FAILED") refreshNotificationsNow(); const code = view.errorCode; if (code !== "PAYG_LIMIT_REACHED" && code !== "FEATURE_DEGRADED") return; if (firedLimitModal.current.has(view.runId)) return; @@ -881,7 +884,9 @@ async function runPolicyOnFile( await acquireDispatchSlot(priority); try { const target = resolvePolicyRunTarget(); - const runId = await runStoredPolicy(backendId, [file]); + // Recorded against a document this browser can resolve. One file per run, which is the only + // shape the server keeps a reference for. + const runId = await runStoredPolicy(backendId, [file], fileId); // recordRunStart marks this (policy, file) dispatched as it records the run. recordRunStart({ runId, diff --git a/frontend/editor/src/proprietary/services/policyApi.test.ts b/frontend/editor/src/proprietary/services/policyApi.test.ts new file mode 100644 index 0000000000..ae0b861e54 --- /dev/null +++ b/frontend/editor/src/proprietary/services/policyApi.test.ts @@ -0,0 +1,48 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +/** + * Only the document reference is pinned, being the one field with a rule attached: a filename here + * would reach a table that deliberately has nowhere to keep one. + */ + +const post = vi.fn().mockResolvedValue({ data: { jobId: "run-1" } }); + +vi.mock("@app/services/apiClient", () => ({ + default: { post: (...args: unknown[]) => post(...args) }, +})); + +const { runStoredPolicy } = await import("@app/services/policyApi"); + +/** The multipart body the last call sent. */ +function sentForm(): FormData { + return post.mock.calls.at(-1)?.[1] as FormData; +} + +const document = () => + new File(["%PDF-1.7"], "quarterly-report.pdf", { type: "application/pdf" }); + +describe("runStoredPolicy", () => { + beforeEach(() => { + post.mockClear(); + }); + + it("sends the workspace id of the document it is running on", async () => { + await runStoredPolicy("policy-1", [document()], "editor-file-1"); + + expect(post.mock.calls.at(-1)?.[0]).toBe("/api/v1/policies/policy-1/run"); + expect(sentForm().get("fileId")).toBe("editor-file-1"); + }); + + it("sends no reference when the caller has none", async () => { + // The export path can enforce on bytes with no workspace document behind them. + await runStoredPolicy("policy-1", [document()]); + + expect(sentForm().has("fileId")).toBe(false); + }); + + it("never sends the document's name as the reference", async () => { + await runStoredPolicy("policy-1", [document()], "editor-file-1"); + + expect(sentForm().get("fileId")).not.toContain("quarterly-report"); + }); +}); diff --git a/frontend/editor/src/proprietary/services/policyApi.ts b/frontend/editor/src/proprietary/services/policyApi.ts index b0d78f36f7..d93e51fa82 100644 --- a/frontend/editor/src/proprietary/services/policyApi.ts +++ b/frontend/editor/src/proprietary/services/policyApi.ts @@ -60,13 +60,18 @@ export async function reorderPolicies(orderedIds: string[]): Promise { await apiClient.put("/api/v1/policies/order", orderedIds); } -/** Run a stored policy by id on the supplied files; returns the run id. */ +/** + * Run a stored policy by id; returns the run id. `fileId` is this workspace's own opaque id, recorded + * against any failure of the run. Only honoured for a single-document run, and never a filename. + */ export async function runStoredPolicy( id: string, files: File[], + fileId?: string, ): Promise { const form = new FormData(); for (const file of files) form.append("fileInput", file); + if (fileId) form.append("fileId", fileId); // Don't set Content-Type: the HTTP client must generate multipart/form-data // WITH its boundary from the FormData body. A manual boundary-less header makes // the server reject the request ("no multipart boundary parameter").