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").