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 8f60e9f4a0..968917ba4e 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 @@ -18,6 +18,19 @@ public enum FailureActionId { DISMISS(Execution.SERVER, "Dismiss"), + /** + * Open the failed operation in the client with its document, for the owner to run again + * themselves. Not a re-run: the settings are theirs to check first. + */ + OPEN_IN_TOOL(Execution.CLIENT, "Retry"), + + /** + * Ask the owner for the password and unlock the document in their client. Re-running is implied + * rather than named: an id says what a caller must supply, and a {@link + * FailureActionSlot#RESOLUTION} runs the failed work again once it has it. + */ + DECRYPT(Execution.CLIENT, "Decrypt and retry"), + /** Open the document behind the incident, in whichever client can resolve its id. */ VIEW_FILE(Execution.CLIENT, "View file"), diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionSlot.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionSlot.java new file mode 100644 index 0000000000..7a001dc8c1 --- /dev/null +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionSlot.java @@ -0,0 +1,14 @@ +package stirling.software.proprietary.failure; + +/** Placement intent, not layout: the client promotes, knowing what it can actually run. */ +public enum FailureActionSlot { + + /** The action that resolves the failure. At most one per kind. */ + RESOLUTION, + + /** Offered alongside the resolution, for a caller the resolution is not aimed at. */ + SECONDARY, + + /** Available but folded away: correct, rarely what anyone wants to press next. */ + OVERFLOW +} 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 fa2fd93448..9b507555e9 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,8 +1,12 @@ package stirling.software.proprietary.failure; +import static stirling.software.proprietary.failure.FailureActionId.DECRYPT; import static stirling.software.proprietary.failure.FailureActionId.DISMISS; +import static stirling.software.proprietary.failure.FailureActionId.OPEN_IN_TOOL; 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.FailureActionSlot.OVERFLOW; +import static stirling.software.proprietary.failure.FailureActionSlot.SECONDARY; 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; @@ -21,11 +25,8 @@ import lombok.AccessLevel; 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. - * - *

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. + * The registry of failure kinds as data: id, i18n keys, English fallback, plus the facets a review + * surface needs. A new kind ships as an entry plus copy; each offer says who it is for and where. */ @Getter public enum FailureKind { @@ -36,9 +37,12 @@ public enum FailureKind { FailureScope.FILE, errorCodes("E004"), fallback("This document is password-protected, so the pipeline could not read it."), - offer(VIEW_FILE, OWNER), - offer(VIEW_IN_PROCESSOR, TEAM_REVIEWER), - offer(DISMISS, ANYONE_WHO_SEES)), + // The password is the fix; the owner's own document is the runner-up. + resolution(DECRYPT, OWNER), + global(VIEW_FILE, OWNER, SECONDARY), + global(VIEW_IN_PROCESSOR, TEAM_REVIEWER, OVERFLOW), + global(OPEN_IN_TOOL, OWNER, OVERFLOW), + global(DISMISS, ANYONE_WHO_SEES, OVERFLOW)), UNKNOWN( FailureStage.INTERNAL, @@ -47,11 +51,11 @@ public enum FailureKind { FailureScope.RUN, noErrorCodes(), fallback("This run failed for a reason Stirling does not yet recognise."), - // 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)); + // No known fix to declare, so a plain retry leads: these are often one-offs. + global(OPEN_IN_TOOL, OWNER, SECONDARY), + global(VIEW_FILE, OWNER, SECONDARY), + global(VIEW_IN_PROCESSOR, TEAM_REVIEWER, OVERFLOW), + global(DISMISS, ANYONE_WHO_SEES, OVERFLOW)); private static final String KEY_PREFIX = "portal.failures.kind."; private static final String ACTION_KEY_PREFIX = "portal.failures.action."; @@ -98,27 +102,37 @@ public enum FailureKind { this.offers = List.of(offers); } - /** - * 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, FailureAudience audience, String labelKeySuffix) {} + /** One ordered list, not parallel maps of audiences, slots and labels that could disagree. */ + private record Offer( + FailureActionId id, + FailureAudience audience, + FailureActionSlot slot, + String labelKeySuffix) {} - /** Declaration order is display order. */ - private static Offer offer(FailureActionId id, FailureAudience audience) { - return new Offer(id, audience, null); + /** The action that fixes this kind. One per kind: needing two would make it two kinds. */ + private static Offer resolution(FailureActionId id, FailureAudience audience) { + return new Offer(id, audience, FailureActionSlot.RESOLUTION, null); } - /** - * As {@link #offer(FailureActionId, FailureAudience)}, but labelled by this kind's own wording - * where the shared one reads badly. - */ - private static Offer offer( + /** As {@link #resolution(FailureActionId, FailureAudience)}, with this kind's own wording. */ + private static Offer resolution( FailureActionId id, FailureAudience audience, String labelKeySuffix) { - return new Offer(id, audience, labelKeySuffix); + return new Offer(id, audience, FailureActionSlot.RESOLUTION, labelKeySuffix); + } + + /** Not this kind's fix: an offer any kind can make, with the shared wording. */ + private static Offer global( + FailureActionId id, FailureAudience audience, FailureActionSlot slot) { + return new Offer(id, audience, slot, null); + } + + /** As above, with this kind's own wording where the shared one reads badly. */ + private static Offer global( + FailureActionId id, + FailureAudience audience, + FailureActionSlot slot, + String labelKeySuffix) { + return new Offer(id, audience, slot, labelKeySuffix); } /** @@ -157,21 +171,25 @@ 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. - */ + /** What this kind offers, in declaration order, each with label and placement resolved. */ public List getOfferedActions() { return offers.stream() .map( offer -> new OfferedAction( - offer.id(), labelKeyFor(offer.id()), offer.audience())) + offer.id(), + labelKeyFor(offer.id()), + offer.audience(), + offer.slot())) .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) {} + /** One action as a kind declares it: what to call it, who it is for, where it wants to sit. */ + public record OfferedAction( + FailureActionId id, + String labelKey, + FailureAudience audience, + FailureActionSlot slot) {} /** Whether this kind offers {@code action}. The dispatch guard: see {@code FailureActionId}. */ public boolean declares(FailureActionId action) { 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 316dd34925..ae6b7acb0c 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 @@ -64,16 +64,17 @@ public interface FileRunEventRepository extends JpaRepository{@code "anonymous"} with login disabled, where the one operator is every viewer. + */ + public String viewerKey() { + String actor = currentActor(); + return actor == null || actor.isBlank() ? "anonymous" : sha256Prefix(actor); + } + + /** First 8 bytes of SHA-256 as hex: stable, one-way, and collision-safe enough to key on. */ + private static String sha256Prefix(String value) { + try { + byte[] digest = + MessageDigest.getInstance("SHA-256") + .digest(value.getBytes(StandardCharsets.UTF_8)); + return HexFormat.of().formatHex(digest, 0, 8); + } catch (NoSuchAlgorithmException e) { + // Every JVM ships SHA-256; a constant here would silently merge two viewers' read + // state, so the caller gets no key and the client falls back to showing everything. + log.warn("SHA-256 unavailable, so notifications cannot be scoped to a viewer", e); + return ""; + } + } + private FailureActionId parseActionId(String actionId) { for (FailureActionId candidate : FailureActionId.values()) { if (candidate.name().equals(actionId)) { @@ -326,6 +370,11 @@ public class FileRunEventService { return applicationProperties.getSecurity().isEnableLogin(); } + /** One action offered to one caller, availability resolved. */ public record AvailableAction( - FailureActionId id, String labelKey, boolean enabled, String disabledReasonKey) {} + FailureActionId id, + String labelKey, + FailureActionSlot slot, + boolean enabled, + String disabledReasonKey) {} } diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventStatus.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventStatus.java index 9bf0e0b607..5e2499753a 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventStatus.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventStatus.java @@ -3,10 +3,7 @@ package stirling.software.proprietary.failure; import java.util.Arrays; import java.util.List; -/** - * Disposition of one recorded failure. {@code RESOLVED} is declared but not set yet (it becomes - * system-set later); the rollup already defines what a repeat means for it, which is to reopen. - */ +/** Disposition of one recorded failure. {@code RESOLVED} is system-set; a repeat reopens it. */ public enum FileRunEventStatus { NEW(false), ACKNOWLEDGED(false), @@ -14,9 +11,8 @@ public enum FileRunEventStatus { RESOLVED(true), /** - * The document this incident was about was deleted from its owner's editor, so there is nothing - * left to act on. Distinct from {@code DISMISSED}, which is a reviewer's decision, and from - * {@code RESOLVED}, which reopens on recurrence: this one cannot recur, the file is gone. + * The document was deleted, so there is nothing left to act on. A recurrence reopens it like + * {@code RESOLVED}: a fresh failure is proof the document is back. */ FILE_REMOVED(true); 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 b88ba3d48d..3b7501e9e2 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 @@ -61,14 +61,15 @@ public record FileRunEventView( } /** - * {@code defaultLabel} and {@code execution} let a client render and route an action it was - * never built with. Declaration order is display order. + * {@code defaultLabel} and {@code execution} let a client render an action it was never built + * with; {@code slot} is placement intent. See {@link FailureActionSlot}. */ public record ActionView( String id, String labelKey, String defaultLabel, FailureActionId.Execution execution, + FailureActionSlot slot, boolean enabled, String disabledReasonKey) { @@ -78,6 +79,7 @@ public record FileRunEventView( action.labelKey(), action.id().getDefaultLabel(), action.id().getExecution(), + action.slot(), action.enabled(), action.disabledReasonKey()); } 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 index f2bf36a8dc..4c51dd4e72 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationController.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationController.java @@ -2,10 +2,14 @@ package stirling.software.proprietary.notification; import java.util.List; +import org.springframework.http.HttpStatus; import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.PathVariable; +import org.springframework.web.bind.annotation.PostMapping; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestParam; import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.server.ResponseStatusException; import io.swagger.v3.oas.annotations.Hidden; import io.swagger.v3.oas.annotations.Operation; @@ -13,9 +17,11 @@ import io.swagger.v3.oas.annotations.tags.Tag; import lombok.RequiredArgsConstructor; +import stirling.software.proprietary.failure.FailureActionException; + /** - * 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. + * Open to any authenticated user: each source scopes its own rows. Every action runs on the + * client's own device, so the only write is it reporting a fix. */ @RestController @RequestMapping("/api/v1/notifications") @@ -40,9 +46,34 @@ public class NotificationController { + " 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)); + return new NotificationsResponse( + notifications.list(capped), + notifications.callerReviewsTeam(), + notifications.callerViewerKey()); + } + + @PostMapping("/{notificationId}/resolved") + @Operation( + summary = "Record that a client-side retry fixed what a notification was about", + description = + "Takes the prefixed notification id, not the producing row's id. Not an action:" + + " nobody is offered a resolve button, and a recurrence brings the" + + " notification back.") + public NotificationView resolved(@PathVariable String notificationId) { + try { + return notifications.resolve(notificationId); + } catch (IllegalArgumentException e) { + throw new ResponseStatusException(HttpStatus.BAD_REQUEST, e.getMessage(), e); + } catch (FailureActionException e) { + throw new ResponseStatusException( + FailureActionException.statusOf(e.getReason()), e.getMessage(), e); + } } /** Wrapped so paging or a total can be added without breaking clients. */ - public record NotificationsResponse(List notifications) {} + public record NotificationsResponse( + List notifications, + boolean viewerReviewsTeam, + /** Opaque; the client scopes its own read state on it. Empty means "cannot scope". */ + String viewerKey) {} } 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 index f7bf3b8530..1922ed3e6b 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationService.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationService.java @@ -20,9 +20,47 @@ public class NotificationService { private final FileRunEventService fileRunEvents; - /** Newest first, and only open failures: one already dealt with is not news. */ + /** + * Newest first, and only open failures about a document: one already dealt with is not news, + * and a row naming no file has nothing the bell can offer beyond saying so. + * + *

Filtered on the named file rather than the kind's scope, because a RUN-scoped kind still + * names one when the editor reported it: a failed tool run belongs here. Applied after the + * limit, so a page can come back short while unattributed rows exist - the review surface is + * where those are meant to be read, and it lists them unfiltered. + */ public List list(int limit) { - return fileRunEvents.list(null, null, limit).stream().map(this::fromFailure).toList(); + return fileRunEvents.list(null, null, limit).stream() + .filter(event -> event.fileId() != null && !event.fileId().isBlank()) + .map(this::fromFailure) + .toList(); + } + + /** Whether the caller sees the whole team's incidents rather than only their own. */ + public boolean callerReviewsTeam() { + return fileRunEvents.reviewsTeam(); + } + + /** Opaque and stable, so a shared browser can keep one viewer's read state off another's. */ + public String callerViewerKey() { + return fileRunEvents.viewerKey(); + } + + /** Takes the prefixed id, so the bell cannot reach a failure endpoint even by accident. */ + public NotificationView resolve(String notificationId) { + NotificationSource.QualifiedId qualified = qualify(notificationId); + return switch (qualified.source()) { + case FAILURE -> fromFailure(fileRunEvents.resolve(qualified.rowId())); + }; + } + + /** The source and row id behind a notification id, refusing anything that is not one. */ + private static NotificationSource.QualifiedId qualify(String notificationId) { + return NotificationSource.parse(notificationId) + .orElseThrow( + () -> + new IllegalArgumentException( + "Not a notification id: " + notificationId)); } /** Prefixes the row id on the way out, so it is never sent bare. */ 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 index 007e51616f..0dbe99cee6 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationSource.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/notification/NotificationSource.java @@ -1,6 +1,8 @@ package stirling.software.proprietary.notification; +import java.util.Arrays; import java.util.Locale; +import java.util.Optional; /** * Which subsystem produced a notification. Every id is prefixed with it, so a client never holds @@ -18,4 +20,24 @@ public enum NotificationSource { public String qualify(String sourceRowId) { return prefix() + sourceRowId; } + + /** Empty rather than throwing for an unprefixed or unknown id: both arrive from clients. */ + public static Optional parse(String notificationId) { + if (notificationId == null) { + return Optional.empty(); + } + int separator = notificationId.indexOf(SEPARATOR); + if (separator <= 0 || separator == notificationId.length() - 1) { + return Optional.empty(); + } + String prefix = notificationId.substring(0, separator); + String rowId = notificationId.substring(separator + 1); + return Arrays.stream(values()) + .filter(source -> source.name().equalsIgnoreCase(prefix)) + .findFirst() + .map(source -> new QualifiedId(source, rowId)); + } + + /** A notification id split into the source that owns it and that source's own row id. */ + public record QualifiedId(NotificationSource source, String rowId) {} } diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/accountlink/ConnectServiceTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/accountlink/ConnectServiceTest.java index 5e040e4ad7..5af25d7250 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/accountlink/ConnectServiceTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/accountlink/ConnectServiceTest.java @@ -357,8 +357,6 @@ class ConnectServiceTest { assertThat(status.authorizeUrl()).isEqualTo("https://app.example.com/link?request=req-1"); } - // --------------------------------------------------------------------------------------- - /** A start with nothing but the reconstructed request URL, as a headless caller would send. */ private static ConnectService.CallbackHint fromRequest(String derivedBaseUrl) { return new ConnectService.CallbackHint(null, null, derivedBaseUrl); 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 index 7eaeb01eb4..87d6e02e31 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/CheckConstrainedEnumsTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/CheckConstrainedEnumsTest.java @@ -43,6 +43,7 @@ class CheckConstrainedEnumsTest { assertThat(persisted) .doesNotContain( FailureAudience.class, + FailureActionSlot.class, FailureActionId.class, FailureActionId.Execution.class, Ownership.class); 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 03aa5c9402..4d5a76bbdd 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.FailureActionSlot.OVERFLOW; +import static stirling.software.proprietary.failure.FailureActionSlot.RESOLUTION; +import static stirling.software.proprietary.failure.FailureActionSlot.SECONDARY; 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; @@ -34,9 +37,12 @@ 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) { + FailureActionId id, + FailureAudience audience, + FailureActionSlot slot, + String labelKeySuffix) { return new FailureKind.OfferedAction( - id, "portal.failures.action." + labelKeySuffix, audience); + id, "portal.failures.action." + labelKeySuffix, audience, slot); } @Nested @@ -70,27 +76,6 @@ 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<>(); @@ -121,12 +106,13 @@ 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. + void everyOfferSaysWhoItIsForAndWhereItGoes(FailureKind kind) { + // Both decide what a caller is shown, so a missing one places a button by accident. for (FailureKind.OfferedAction offer : kind.getOfferedActions()) { assertThat(offer.audience()) .as("%s offers %s", kind.getId(), offer.id()) .isNotNull(); + assertThat(offer.slot()).as("%s offers %s", kind.getId(), offer.id()).isNotNull(); } } @@ -138,6 +124,17 @@ class FailureKindTest { assertThat(kind.getActions()).doesNotHaveDuplicates(); } + @ParameterizedTest + @EnumSource(FailureKind.class) + void declaresAtMostOneResolution(FailureKind kind) { + // Two things that both claim to fix it is a sign of two kinds wearing one id. + assertThat( + kind.getOfferedActions().stream() + .filter(offer -> offer.slot() == FailureActionSlot.RESOLUTION) + .toList()) + .hasSizeLessThanOrEqualTo(1); + } + @Test void noTwoKindsClaimTheSameErrorCode() { // Computed independently of duplicateErrorCodes(), then checked against it: the boot @@ -232,16 +229,18 @@ class FailureKindTest { class Unknown { @Test - void offersItsOwnerTheirDocumentAndTheRunToWhoeverReviews() { - // Nothing here is known to be fixable, so the offers are just the places to look. + void offersARetryToItsOwnerAndTheRunToWhoeverReviews() { + // No known fix, so no resolution; a retry is still worth offering for a one-off. assertThat(FailureKind.UNKNOWN.getOfferedActions()) .containsExactly( - offered(FailureActionId.VIEW_FILE, OWNER, "viewFile"), + offered(FailureActionId.OPEN_IN_TOOL, OWNER, SECONDARY, "openInTool"), + offered(FailureActionId.VIEW_FILE, OWNER, SECONDARY, "viewFile"), offered( FailureActionId.VIEW_IN_PROCESSOR, TEAM_REVIEWER, + OVERFLOW, "viewInProcessor"), - offered(FailureActionId.DISMISS, ANYONE_WHO_SEES, "dismiss")); + offered(FailureActionId.DISMISS, ANYONE_WHO_SEES, OVERFLOW, "dismiss")); } @Test @@ -294,16 +293,19 @@ class FailureKindTest { } @Test - void offersTheDocumentToItsOwnerAndTheRunToItsReviewer() { - // The point of the audiences: only the owner holds the document. + void aKindWithSomethingToFixOffersTheFixToItsOwnerAndTheRunToItsReviewer() { + // Only the owner has the password, so a reviewer is offered the run and a dismiss. assertThat(FailureKind.INPUT_PASSWORD_PROTECTED.getOfferedActions()) .containsExactly( - offered(FailureActionId.VIEW_FILE, OWNER, "viewFile"), + offered(FailureActionId.DECRYPT, OWNER, RESOLUTION, "decrypt"), + offered(FailureActionId.VIEW_FILE, OWNER, SECONDARY, "viewFile"), offered( FailureActionId.VIEW_IN_PROCESSOR, TEAM_REVIEWER, + OVERFLOW, "viewInProcessor"), - offered(FailureActionId.DISMISS, ANYONE_WHO_SEES, "dismiss")); + offered(FailureActionId.OPEN_IN_TOOL, OWNER, OVERFLOW, "openInTool"), + offered(FailureActionId.DISMISS, ANYONE_WHO_SEES, OVERFLOW, "dismiss")); } @Test @@ -333,10 +335,8 @@ class FailureKindTest { 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"); + assertThat(FailureKind.INPUT_PASSWORD_PROTECTED.labelKeyFor(FailureActionId.DECRYPT)) + .isEqualTo("portal.failures.action.decrypt"); } @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 3ece59f6af..4627e37bd4 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 @@ -153,6 +153,7 @@ class FileRunEventControllerTest { action -> { assertThat(action.defaultLabel()).isNotBlank(); assertThat(action.execution()).isNotNull(); + assertThat(action.slot()).isNotNull(); }) .filteredOn(action -> "VIEW_IN_PROCESSOR".equals(action.id())) .singleElement() @@ -160,6 +161,7 @@ class FileRunEventControllerTest { action -> { assertThat(action.execution()) .isEqualTo(FailureActionId.Execution.CLIENT); + assertThat(action.slot()).isEqualTo(FailureActionSlot.OVERFLOW); assertThat(action.defaultLabel()).isEqualTo("View in processor"); }); } 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 ae46a50c8a..4bf886dbc8 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 @@ -130,6 +130,7 @@ class FileRunEventHttpIntegrationTest { 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("slot").asString()).isEqualTo("OVERFLOW"); 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"); 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 83ad173637..504ec92ba9 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 @@ -157,6 +157,103 @@ class FileRunEventServiceTest { } } + @Nested + @DisplayName("resolve") + class Resolve { + + @Test + void marksTheRowResolvedWhenAClientReportsItsRetryWorked() { + FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1"); + + FileRunEvent resolved = service.resolve(event.id()); + + assertThat(resolved.status()).isEqualTo(FileRunEventStatus.RESOLVED); + assertThat(resolved.statusActor()).isEqualTo(ACTOR); + assertThat(service.list(null, null, 10)).as("resolved work is not open work").isEmpty(); + } + + @Test + void isNotAnActionAnyoneCanPress() { + // System-set on a client-side retry, so there is no id to dispatch and no button. + assertThat(Arrays.stream(FailureActionId.values()).map(Enum::name)) + .doesNotContain("RESOLVE", "RESOLVED"); + } + + @Test + void reportingTheSameSuccessTwiceIsNotARefusal() { + // A client that retries, succeeds and reports twice is telling the truth twice. + FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1"); + Instant first = service.resolve(event.id()).statusAt(); + + assertThat(service.resolve(event.id()).statusAt()).isEqualTo(first); + } + + @Test + void aDismissedRowCannotBeResolvedBehindTheReviewersBack() { + FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1"); + service.dispatch(event.id(), "DISMISS", Map.of()); + + assertThatThrownBy(() -> service.resolve(event.id())) + .isInstanceOf(FailureActionException.class) + .extracting(e -> ((FailureActionException) e).getReason()) + .isEqualTo(FailureActionException.Reason.ALREADY_CLOSED); + } + + @Test + void anotherTeamsRowIsNotFound() { + FileRunEvent theirs = given(FailureKind.UNKNOWN, 99L, "f1"); + + assertThatThrownBy(() -> service.resolve(theirs.id())) + .isInstanceOf(FailureActionException.class) + .extracting(e -> ((FailureActionException) e).getReason()) + .isEqualTo(FailureActionException.Reason.EVENT_NOT_FOUND); + } + + @Test + void aRecurrenceReopensIt() { + // RESOLVED claims one attempt worked, not that the problem is gone for good. + service.report(new EditorFailureReport("compress", "E004", List.of("f-1"), "boom")); + FileRunEvent event = service.list(null, null, 10).getFirst(); + service.resolve(event.id()); + + service.report(new EditorFailureReport("compress", "E004", List.of("f-1"), "boom")); + + assertThat(service.list(null, null, 10)) + .singleElement() + .extracting(FileRunEvent::status) + .isEqualTo(FileRunEventStatus.NEW); + } + + @Test + void aRecurrenceReopensAnIncidentClosedBecauseTheFileWasRemoved() { + // A library file comes back under the same id, so without this every repeat folds + // into the closed row and the queue never shows the failure again. + service.report(new EditorFailureReport("compress", "E001", List.of("f-1"), "boom")); + service.forgetFiles(List.of("f-1")); + assertThat(service.list(null, null, 10)).isEmpty(); + + service.report(new EditorFailureReport("compress", "E001", List.of("f-1"), "boom")); + + assertThat(service.list(null, null, 10)) + .singleElement() + .extracting(FileRunEvent::status) + .isEqualTo(FileRunEventStatus.NEW); + } + + @Test + void aRecurrenceLeavesAReviewersDismissalAlone() { + // Dismiss is a decision about the incident, not a claim about the document, so it + // outlasts a repeat where FILE_REMOVED and RESOLVED do not. + service.report(new EditorFailureReport("compress", "E001", List.of("f-1"), "boom")); + FileRunEvent event = service.list(null, null, 10).getFirst(); + service.dispatch(event.id(), "DISMISS", Map.of()); + + service.report(new EditorFailureReport("compress", "E001", List.of("f-1"), "boom")); + + assertThat(service.list(null, null, 10)).isEmpty(); + } + } + @Nested @DisplayName("triage never touches the document") class NeverTouchesTheDocument { @@ -352,13 +449,17 @@ class FileRunEventServiceTest { } @Test - void theOwnerIsOfferedTheirDocumentAndNotTheReviewersView() { - // The document is theirs to open; the processor view is for whoever reviews the team. + void theOwnerIsOfferedTheFixAndNotTheReviewersView() { + // The unlock is the owner's to do; the processor view is for whoever reviews. when(authority.canEditPolicies()).thenReturn(false); FileRunEvent mine = givenHitBy(ACTOR, FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); assertThat(offeredFor(mine)) - .containsExactly(FailureActionId.VIEW_FILE, FailureActionId.DISMISS); + .containsExactly( + FailureActionId.DECRYPT, + FailureActionId.VIEW_FILE, + FailureActionId.OPEN_IN_TOOL, + FailureActionId.DISMISS); assertThat(service.availableActions(mine)) .allMatch(FileRunEventService.AvailableAction::enabled); } @@ -385,8 +486,10 @@ class FileRunEventServiceTest { assertThat(offeredFor(unattended)) .containsExactly( + FailureActionId.DECRYPT, FailureActionId.VIEW_FILE, FailureActionId.VIEW_IN_PROCESSOR, + FailureActionId.OPEN_IN_TOOL, FailureActionId.DISMISS); } @@ -499,6 +602,17 @@ class FileRunEventServiceTest { .equals(action.disabledReasonKey())); } + @Test + void carriesTheKindsPlacementIntentForEachOffer() { + FileRunEvent mine = givenHitBy(ACTOR, FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); + + assertThat(service.availableActions(mine)) + .filteredOn(action -> action.id() == FailureActionId.DECRYPT) + .singleElement() + .extracting(FileRunEventService.AvailableAction::slot) + .isEqualTo(FailureActionSlot.RESOLUTION); + } + @Test void carriesTheLabelKeyForEachOffer() { FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1"); 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 4f429fe9b1..7bf386c3b1 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 @@ -96,7 +96,9 @@ class InMemoryFileRunEventRepository implements FileRunEventRepository { @Override public int reopenIfResolved(String id) { FileRunEventEntity entity = rows.get(id); - if (entity == null || entity.getStatus() != FileRunEventStatus.RESOLVED) { + if (entity == null + || (entity.getStatus() != FileRunEventStatus.RESOLVED + && entity.getStatus() != FileRunEventStatus.FILE_REMOVED)) { return 0; } entity.setStatus(FileRunEventStatus.NEW); 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 index e76a8b96ee..06df6df843 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/NotificationProjectionTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/NotificationProjectionTest.java @@ -2,6 +2,7 @@ package stirling.software.proprietary.failure; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.when; import java.util.List; @@ -120,6 +121,32 @@ class NotificationProjectionTest { .allMatch(action -> action.execution() == FailureActionId.Execution.CLIENT); } + @Test + void holdsBackAFailureNamingNoDocumentBecauseTheBellCouldOnlySaySo() { + // The only row the bell can offer nothing for. The review surface still lists it. + given(FailureKind.UNKNOWN, ACTOR, null); + given(FailureKind.INPUT_PASSWORD_PROTECTED, ACTOR, "f-1"); + + assertThat(controller.list(null).notifications()) + .singleElement() + .satisfies(row -> assertThat(row.fileId()).isEqualTo("f-1")); + } + + @Test + void keepsARunScopedFailureThatStillNamesADocument() { + // An editor-reported tool failure is RUN-scoped but names the file it ran on, so + // filtering on the kind's scope rather than the row would have dropped it. + given(FailureKind.UNKNOWN, ACTOR, "f-2"); + + assertThat(controller.list(null).notifications()) + .singleElement() + .satisfies( + row -> { + assertThat(row.kindId()).isEqualTo("UNKNOWN"); + assertThat(row.fileId()).isEqualTo("f-2"); + }); + } + @Test void namesTheSourceThatFedAnUnattendedRunSoItsFileIdIsNotMistakenForAClientsOwn() { // Without the source a client looks up a hash it can never resolve and calls it @@ -162,7 +189,53 @@ class NotificationProjectionTest { assertThat(action.labelKey()).startsWith("portal.failures.action."); assertThat(action.defaultLabel()).isNotBlank(); assertThat(action.execution()).isNotNull(); + assertThat(action.slot()).isNotNull(); }); } } + + @Nested + @DisplayName("the response says whether the caller reviews the team") + class ReviewerFlag { + + @Test + void trueForAReviewerSoTheClientFiltersNothing() { + when(authority.canEditPolicies()).thenReturn(true); + + assertThat(controller.list(null).viewerReviewsTeam()).isTrue(); + } + + @Test + void falseForAMemberSoTheClientHidesRowsForFilesItDoesNotHold() { + when(authority.canEditPolicies()).thenReturn(false); + + assertThat(controller.list(null).viewerReviewsTeam()).isFalse(); + } + } + + @Nested + @DisplayName("the response names the viewer, opaquely, for a client to scope read state on") + class ViewerKey { + + @Test + void steadyForOneViewerAcrossReads() { + assertThat(controller.list(null).viewerKey()) + .isEqualTo(controller.list(null).viewerKey()) + .isNotBlank(); + } + + @Test + void differentForAnotherViewerSoOneCannotInheritTheOthersMarker() { + String mine = controller.list(null).viewerKey(); + when(userService.getCurrentUsername()).thenReturn("someone.else@example.com"); + + assertThat(controller.list(null).viewerKey()).isNotEqualTo(mine); + } + + @Test + void neverTheUsernameItself() { + // It lands in that browser's storage, and a client only needs to tell viewers apart. + assertThat(controller.list(null).viewerKey()).doesNotContain(ACTOR); + } + } } diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/NotificationResolveTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/NotificationResolveTest.java new file mode 100644 index 0000000000..b6cddd6a71 --- /dev/null +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/NotificationResolveTest.java @@ -0,0 +1,152 @@ +package stirling.software.proprietary.failure; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.when; + +import java.util.List; +import java.util.Map; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.http.HttpStatus; +import org.springframework.web.server.ResponseStatusException; + +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.NotificationView; +import stirling.software.proprietary.policy.config.PolicyManagementAuthority; + +/** Reporting a client-side retry that worked: the bell's one write. */ +@ExtendWith(MockitoExtension.class) +@DisplayName("reporting a client-side retry that worked") +class NotificationResolveTest { + + 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")); + } + + /** The status a refused call came back with. Fails the test if the call was allowed. */ + private HttpStatus statusOf(Runnable call) { + try { + call.run(); + } catch (ResponseStatusException e) { + return HttpStatus.valueOf(e.getStatusCode().value()); + } + throw new AssertionError("expected the call to be refused"); + } + + @Test + void closesTheRowBehindThePrefixedId() { + // Why the route exists: the bell has no raw id to close its own row with. + FileRunEvent event = given(FailureKind.UNKNOWN, ACTOR, "f-1"); + + NotificationView resolved = controller.resolved("failure:" + event.id()); + + assertThat(resolved.status()).isEqualTo(FileRunEventStatus.RESOLVED); + assertThat(store.find(event.id(), TEAM).orElseThrow().status()) + .isEqualTo(FileRunEventStatus.RESOLVED); + } + + @Test + void theRowsOwnIdIsNotANotificationId() { + // Refused outright rather than left to work by accident for whichever source it reaches. + FileRunEvent event = given(FailureKind.UNKNOWN, ACTOR, "f-1"); + + assertThat(statusOf(() -> controller.resolved(event.id()))) + .isEqualTo(HttpStatus.BAD_REQUEST); + assertThat(store.find(event.id(), TEAM).orElseThrow().status()) + .isEqualTo(FileRunEventStatus.NEW); + } + + @Test + void anUnknownSourcePrefixIsABadRequest() { + // Not a 404: it was never a notification id, so there is no row to report missing. + FileRunEvent event = given(FailureKind.UNKNOWN, ACTOR, "f-1"); + + assertThat(statusOf(() -> controller.resolved("quota:" + event.id()))) + .isEqualTo(HttpStatus.BAD_REQUEST); + assertThat(statusOf(() -> controller.resolved("failure:"))) + .isEqualTo(HttpStatus.BAD_REQUEST); + } + + @Test + void reportingTheSameSuccessTwiceIsNotARefusal() { + FileRunEvent event = given(FailureKind.UNKNOWN, ACTOR, "f-1"); + NotificationView first = controller.resolved("failure:" + event.id()); + + assertThat(controller.resolved("failure:" + event.id())) + .isEqualTo(first) + .extracting(NotificationView::status) + .isEqualTo(FileRunEventStatus.RESOLVED); + } + + @Test + void aRowAReviewerHasDismissedIsAConflict() { + // Their decision stands: a retry reporting in afterwards does not overwrite it. + FileRunEvent event = given(FailureKind.UNKNOWN, ACTOR, "f-1"); + failures.dispatch(event.id(), "DISMISS", Map.of()); + + assertThat(statusOf(() -> controller.resolved("failure:" + event.id()))) + .isEqualTo(HttpStatus.CONFLICT); + assertThat(store.find(event.id(), TEAM).orElseThrow().status()) + .isEqualTo(FileRunEventStatus.DISMISSED); + } + + @Test + void aColleaguesNotificationIsNotFoundForAMember() { + FileRunEvent theirs = given(FailureKind.UNKNOWN, "colleague@example.com", "f-1"); + when(authority.canEditPolicies()).thenReturn(false); + + assertThat(statusOf(() -> controller.resolved("failure:" + theirs.id()))) + .isEqualTo(HttpStatus.NOT_FOUND); + } + + @Test + void aReviewerClosesAColleaguesRowTheyFixed() { + // Visibility decides, not ownership: a reviewer reads the team's incidents, so a reviewer + // who fixes one closes it. The member's own row is unreachable to them the other way round. + FileRunEvent theirs = given(FailureKind.UNKNOWN, "colleague@example.com", "f-1"); + + controller.resolved("failure:" + theirs.id()); + + assertThat(store.find(theirs.id(), TEAM).orElseThrow().status()) + .isEqualTo(FileRunEventStatus.RESOLVED); + } +} diff --git a/app/saas/src/test/java/stirling/software/saas/accountlink/ConnectRequestServiceTest.java b/app/saas/src/test/java/stirling/software/saas/accountlink/ConnectRequestServiceTest.java index f75c96becb..e15d9749f2 100644 --- a/app/saas/src/test/java/stirling/software/saas/accountlink/ConnectRequestServiceTest.java +++ b/app/saas/src/test/java/stirling/software/saas/accountlink/ConnectRequestServiceTest.java @@ -324,8 +324,6 @@ class ConnectRequestServiceTest { assertThat(service.claim("nope", CLAIM_SECRET).outcome()).isEqualTo(ClaimOutcome.REJECTED); } - // --------------------------------------------------------------------------------------- - private static ConnectRequest pending() { ConnectRequest row = new ConnectRequest(); row.setRequestId("req"); diff --git a/frontend/editor/public/locales/en-US/translation.toml b/frontend/editor/public/locales/en-US/translation.toml index 345cf07ab2..d4a028388b 100644 --- a/frontend/editor/public/locales/en-US/translation.toml +++ b/frontend/editor/public/locales/en-US/translation.toml @@ -5506,25 +5506,22 @@ count = "{{remaining}} of {{total}}" label = "Free credits" [notifications] -empty = "Nothing to report." +empty = "You're all caught up." 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." +noDocumentLinked = "This failure is not linked to a specific document, so it cannot be opened or retried here." +notOnThisDevice = "This document is not on this device, so it cannot be opened or retried here." occurrences = "{{count}} times" open = "Notifications" title = "Notifications" unread = "Unread" [notifications.action] +copiedLog = "Copied" +copyLog = "Copy log" failed = "That did not work. Try again in a moment." +more = "More options" 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" @@ -7896,8 +7893,10 @@ title = "Failures" [portal.failures.action] acknowledge = "Acknowledge" confirm = "Are you sure?" +decrypt = "Decrypt and retry" dismiss = "Dismiss" dismissSkipFile = "Skip this file" +openInTool = "Retry" viewFile = "View file" viewInProcessor = "View in processor" @@ -7920,11 +7919,11 @@ description = "Policy runs that fail will appear here with the actions you can t title = "No failures recorded" [portal.failures.kind.inputPasswordProtected] -description = "The pipeline could not open the document because it is password-protected. Unlock it and run it again, or skip this file." +description = "Your file is password protected, so the run could not read it." title = "Password-protected document" [portal.failures.kind.unknown] -description = "This run failed for a reason Stirling does not yet recognise. The raw message is shown below." +description = "Something went wrong that Stirling does not recognise yet." title = "Unrecognised failure" [portal.failures.origin] diff --git a/frontend/editor/src/core/components/notifications/NotificationBell.css b/frontend/editor/src/core/components/notifications/NotificationBell.css index 46367a0b2b..fe8fe54fed 100644 --- a/frontend/editor/src/core/components/notifications/NotificationBell.css +++ b/frontend/editor/src/core/components/notifications/NotificationBell.css @@ -35,6 +35,25 @@ text-align: center; } +/* Scoped to the bell, so the shared DividerWithText is untouched elsewhere. */ +.notification-bell__divider.text-divider { + margin-top: 0.125rem; + margin-bottom: 0.125rem; +} + +/* Gray by default, because the shared rule is near-invisible here. */ +.notification-bell__divider .text-divider__rule { + background-color: var(--c-border-strong); +} + +.notification-bell__divider--new .text-divider__rule { + background-color: var(--c-danger); +} + +.notification-bell__divider--new .text-divider__label { + color: var(--c-danger); +} + .notification-bell__panel { position: fixed; z-index: var(--z-popover, 60); @@ -128,38 +147,6 @@ 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; diff --git a/frontend/editor/src/core/components/notifications/NotificationBell.test.tsx b/frontend/editor/src/core/components/notifications/NotificationBell.test.tsx index 8753a5880b..5e0d14e998 100644 --- a/frontend/editor/src/core/components/notifications/NotificationBell.test.tsx +++ b/frontend/editor/src/core/components/notifications/NotificationBell.test.tsx @@ -9,21 +9,25 @@ import { MantineProvider } from "@mantine/core"; import type { AppNotification, NotificationActionOffer, + NotificationActionSlot, } 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. - */ +// The bell's own two jobs: what counts as read, and how a row behaves around an action. const fetchNotifications = vi.fn(); +// A bare array is wrapped as a reviewer's response; member filtering is the hook's own test. vi.mock("@app/services/notifications", () => ({ - fetchNotifications: (...args: unknown[]) => fetchNotifications(...args), + fetchNotifications: async (...args: unknown[]) => { + const value = await fetchNotifications(...args); + return Array.isArray(value) + ? { notifications: value, viewerReviewsTeam: true, viewerKey: "viewer-a" } + : value; + }, })); // IndexedDB, which jsdom has none of. Answered here so availability is a fact of the test. @@ -35,7 +39,7 @@ const h = vi.hoisted(() => ({ string, { available: (context: unknown) => boolean; - run: (context: unknown, password?: string) => unknown; + run: (context: unknown) => unknown; closesPanel?: boolean; } >, @@ -58,6 +62,8 @@ vi.mock("react-i18next", () => ({ useTranslation: () => ({ // A string fallback, or an options object with defaultValue plus what it interpolates. t: (key: string, fallback?: unknown) => { + // The kinds' sentences live in the locale files, so one stands in here. + if (key.endsWith(".description")) return "Kind description"; if (typeof fallback === "string") return fallback; if (fallback && typeof fallback === "object") { const options = fallback as Record; @@ -77,18 +83,33 @@ const { NotificationBell } = function offer( id: string, + slot: NotificationActionSlot = "SECONDARY", overrides: Partial = {}, ): NotificationActionOffer { return { id, labelKey: `portal.failures.action.${id.toLowerCase()}`, defaultLabel: id, + slot, enabled: true, disabledReasonKey: null, ...overrides, }; } +// Read state watermarks the ordering time, so rows need distinct ones. "a" is the newest. +const AT: Record = { + a: "2026-08-05T02:00:00Z", + b: "2026-08-05T01:00:00Z", +}; + +/** Scoped to the viewer the mocked response names, as the store writes it. */ +const READ_THROUGH_KEY = "stirling.notifications.readThroughAt.viewer-a"; + +function markReadThrough(iso: string): void { + window.localStorage.setItem(READ_THROUGH_KEY, String(Date.parse(iso))); +} + function notification( id: string, title = "Unrecognised failure", @@ -109,8 +130,8 @@ function notification( sourceId: null, policyId: null, occurrences: 1, - createdAt: "2026-08-05T00:00:00Z", - lastSeenAt: "2026-08-05T00:00:00Z", + createdAt: AT[id] ?? "2026-08-05T00:00:00Z", + lastSeenAt: AT[id] ?? "2026-08-05T00:00:00Z", actions: [], ...overrides, }; @@ -172,7 +193,7 @@ describe("NotificationBell", () => { 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"); + markReadThrough(AT.b); fetchNotifications.mockResolvedValue([ notification("a"), notification("b"), @@ -186,7 +207,7 @@ describe("NotificationBell", () => { 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"); + markReadThrough(AT.b); fetchNotifications.mockResolvedValue([ notification("a"), notification("b"), @@ -200,7 +221,7 @@ describe("NotificationBell", () => { }); it("does not divide a list with nothing new in it", async () => { - window.localStorage.setItem("stirling.notifications.lastSeenId", "a"); + markReadThrough(AT.a); fetchNotifications.mockResolvedValue([notification("a")]); render(); await openPanel(); @@ -231,29 +252,26 @@ describe("NotificationBell", () => { first.unmount(); // A newer one arrives above the one already seen. - fetchNotifications.mockResolvedValue([ - notification("b"), - notification("a"), - ]); + const arrived = notification("c", "Unrecognised failure", { + lastSeenAt: "2026-08-05T03:00:00Z", + }); + fetchNotifications.mockResolvedValue([arrived, 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"), - ]); + it("leaves the rest read when the row that was newest has gone", async () => { + // The newest row leaves; marking read by id would then relight the badge for the older one. + markReadThrough(AT.a); + fetchNotifications.mockResolvedValue([notification("b")]); render(); + await openPanel(); - expect(await screen.findByText("2")).toBeTruthy(); + // Nothing is new, so nothing is labelled new: by id, this row would have counted as unread. + expect(await screen.findByText("Unrecognised failure")).toBeTruthy(); + expect(screen.queryByText("New")).toBeNull(); }); it("renders the server's title and repeat count without knowing the source", async () => { @@ -291,6 +309,49 @@ describe("NotificationBell", () => { ).toBeTruthy(); }); + it("tucks overflow actions into a menu, not a row of buttons", async () => { + h.specs = { + DECRYPT: { available: () => true, run: vi.fn() }, + VIEW_FILE: { available: () => true, run: vi.fn() }, + VIEW_IN_PROCESSOR: { available: () => true, run: vi.fn() }, + }; + fetchNotifications.mockResolvedValue([ + notification("a", "Unrecognised failure", { + actions: [ + offer("DECRYPT", "RESOLUTION"), + offer("VIEW_FILE", "SECONDARY"), + offer("VIEW_IN_PROCESSOR", "OVERFLOW"), + ], + }), + ]); + render(); + await openPanel(); + + // Two real buttons; the overflow one is off screen until the menu is opened. + expect( + screen.getByRole("button", { + name: "DECRYPT: Unrecognised failure", + }), + ).toBeTruthy(); + expect( + screen.getByRole("button", { name: "VIEW_FILE: Unrecognised failure" }), + ).toBeTruthy(); + expect( + screen.queryByRole("button", { + name: "VIEW_IN_PROCESSOR: Unrecognised failure", + }), + ).toBeNull(); + + fireEvent.click( + screen.getByRole("button", { + name: "More options: Unrecognised failure", + }), + ); + expect( + await screen.findByRole("menuitem", { name: "VIEW_IN_PROCESSOR" }), + ).toBeTruthy(); + }); + it("runs whichever of the row's actions is pressed", async () => { const run = vi.fn(); h.specs = { @@ -370,7 +431,7 @@ describe("NotificationBell", () => { await waitFor(() => expect( screen.getByText( - "This document is not on this device, so it cannot be opened here.", + "This document is not on this device, so it cannot be opened or retried here.", ), ).toBeTruthy(), ); @@ -387,7 +448,7 @@ describe("NotificationBell", () => { expect( await screen.findByText( - "This failure is not linked to a specific document, so there is nothing to open here.", + "This failure is not linked to a specific document, so it cannot be opened or retried here.", ), ).toBeTruthy(); }); @@ -422,7 +483,7 @@ describe("NotificationBell", () => { notification("a", "Unrecognised failure", { ownership: "UNOWNED", actions: [ - offer("VIEW_FILE", { + offer("VIEW_FILE", "SECONDARY", { enabled: false, disabledReasonKey: "portal.failures.disabled.unattended", }), @@ -452,11 +513,11 @@ describe("NotificationBell", () => { fetchNotifications.mockResolvedValue([ notification("a", "Unrecognised failure", { actions: [ - offer("VIEW_IN_PROCESSOR", { + offer("VIEW_IN_PROCESSOR", "SECONDARY", { enabled: false, disabledReasonKey: "portal.failures.disabled.closed", }), - offer("VIEW_FILE", { + offer("VIEW_FILE", "SECONDARY", { enabled: false, disabledReasonKey: "portal.failures.disabled.closed", }), @@ -474,7 +535,12 @@ describe("NotificationBell", () => { expect( screen.queryByRole("button", { name: /VIEW_IN_PROCESSOR|VIEW_FILE/ }), ).toBeNull(); - expect(document.querySelector(".notification-bell__actions")).toBeNull(); + // The error log stays reachable: a row with nothing left to do still owns its detail. + expect( + screen.getByRole("button", { + name: "More options: Unrecognised failure", + }), + ).toBeTruthy(); }); it("shows a failed action in the row instead of leaving the user guessing", async () => { @@ -506,25 +572,43 @@ describe("NotificationBell", () => { expect(screen.getByText("Password-protected document")).toBeTruthy(); }); - it("expands the message without touching the row's actions", async () => { + it("reads the kind's own words rather than the raw failure", async () => { + // A bell is not a log: the row gets a sentence, the message goes in the menu. + const stack = "org.apache.pdfbox.InvalidPasswordException"; fetchNotifications.mockResolvedValue([ - notification("a", "Unrecognised failure", { - detail: "org.apache.pdfbox.InvalidPasswordException", + notification("a", "Password-protected document", { + titleKey: "portal.failures.kind.inputPasswordProtected.title", + detail: stack, }), ]); render(); await openPanel(); - const expand = screen.getByRole("button", { - name: "Show full message: Unrecognised failure", - }); - fireEvent.click(expand); + expect(await screen.findByText("Kind description")).toBeTruthy(); + expect(screen.queryByText(stack)).toBeNull(); + }); - expect( - screen.getByRole("button", { name: "Show less: Unrecognised failure" }), - ).toBeTruthy(); - expect( - screen.getByRole("button", { name: "Copy error: Unrecognised failure" }), - ).toBeTruthy(); + it("keeps the log one click away, for a row whose only extra is the log", async () => { + h.specs = { VIEW_FILE: { available: () => true, run: vi.fn() } }; + const stack = "org.apache.pdfbox.InvalidPasswordException"; + const clipboard = vi.fn().mockResolvedValue(undefined); + Object.assign(navigator, { clipboard: { writeText: clipboard } }); + fetchNotifications.mockResolvedValue([ + notification("a", "Unrecognised failure", { + detail: stack, + actions: [offer("VIEW_FILE", "SECONDARY")], + }), + ]); + render(); + await openPanel(); + + fireEvent.click( + await screen.findByRole("button", { + name: "More options: Unrecognised failure", + }), + ); + fireEvent.click(await screen.findByRole("menuitem", { name: "Copy log" })); + + await waitFor(() => expect(clipboard).toHaveBeenCalledWith(stack)); }); }); diff --git a/frontend/editor/src/core/components/notifications/NotificationItem.tsx b/frontend/editor/src/core/components/notifications/NotificationItem.tsx index b53ca7894c..cbb17048d2 100644 --- a/frontend/editor/src/core/components/notifications/NotificationItem.tsx +++ b/frontend/editor/src/core/components/notifications/NotificationItem.tsx @@ -1,18 +1,26 @@ import { useState } from "react"; import type { TFunction } from "i18next"; import { useTranslation } from "react-i18next"; -import { Button } from "@app/ui"; +import { Menu, Tooltip } from "@mantine/core"; +import { ActionIcon, Button } from "@app/ui"; +import LocalIcon from "@app/components/shared/LocalIcon"; import { isResolvableHere } from "@app/hooks/useNotifications"; import type { NotificationDocumentState } from "@app/hooks/useNotifications"; import type { ClientActionRegistry, NotificationActionContext, } from "@app/components/notifications/notificationActions"; +import { promoteActions } from "@app/components/notifications/notificationActionSlots"; import type { AppNotification, NotificationActionOffer, } from "@app/services/notifications"; +/** The kind's own sentence, sharing the portal's copy. */ +function summaryKeyOf(titleKey: string): string { + return titleKey.replace(/\.title$/, ".description"); +} + /** * 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. @@ -35,12 +43,12 @@ function noteFor( if (!notification.fileId) return t( "notifications.noDocumentLinked", - "This failure is not linked to a specific document, so there is nothing to open here.", + "This failure is not linked to a specific document, so it cannot be opened or retried here.", ); return isResolvableHere(notification) ? t( "notifications.notOnThisDevice", - "This document is not on this device, so it cannot be opened here.", + "This document is not on this device, so it cannot be opened or retried here.", ) : null; } @@ -53,7 +61,7 @@ interface NotificationItemProps { onDismissPanel: () => void; } -/** Its own component because the last attempt's message and its expanded state are per-row. */ +/** Its own component because the last attempt's message and the copy state are per-row. */ export function NotificationItem({ notification, unread, @@ -64,7 +72,6 @@ export function NotificationItem({ 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); @@ -73,23 +80,17 @@ export function NotificationItem({ 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 { primary, secondary, overflow, withheldReasonKey } = promoteActions( + notification.actions, + (offer) => { + const spec = registry[offer.id]; + // An id this build has never heard of: skipped rather than rendered unwired. + if (!spec) return false; + return spec.available(context); + }, + // A reason from an action this build could not have rendered explains nothing. + (offer) => registry[offer.id] !== undefined, + ); const labelOf = (offer: NotificationActionOffer) => t(offer.labelKey, offer.defaultLabel); @@ -129,6 +130,7 @@ export function NotificationItem({ }; const note = noteFor(notification, documentState, withheldReasonKey, t); + const summary = t(summaryKeyOf(notification.titleKey), { defaultValue: "" }); return (

  • )} - {notification.detail && ( - <> - - {notification.detail} - - - - - - - )} + {summary && {summary}} {note && {note}} - {/* In the kind's declared order, the first leading. */} - {usable.length > 0 && ( + {/* The menu is not gated on a button existing: a row with no action still owns its log. */} + {(primary || notification.detail) && ( - {usable.map((offer, index) => ( + {primary && ( void run(offer)} + label={labelOf(primary)} + busy={busy === primary.id} + onRun={() => void run(primary)} /> - ))} + )} + {secondary && ( + void run(secondary)} + /> + )} + {(overflow.length > 0 || notification.detail) && ( + + + + + + + + + + {overflow.map((offer) => ( + void run(offer)} + > + {labelOf(offer)} + + ))} + {notification.detail && ( + void copyDetail()} + > + {copied + ? t("notifications.action.copiedLog", "Copied") + : t("notifications.action.copyLog", "Copy log")} + + )} + + + )} )} @@ -220,7 +231,8 @@ export function NotificationItem({ } interface ActionButtonProps { - variant: "primary" | "secondary"; + /** Solid for the row's answer, outlined for its runner-up, ghost for the rest. */ + variant: "primary" | "secondary" | "tertiary"; rowTitle: string; label: string; busy: boolean; diff --git a/frontend/editor/src/core/components/notifications/NotificationPanel.tsx b/frontend/editor/src/core/components/notifications/NotificationPanel.tsx index f3ff0c621e..b2b4581b5a 100644 --- a/frontend/editor/src/core/components/notifications/NotificationPanel.tsx +++ b/frontend/editor/src/core/components/notifications/NotificationPanel.tsx @@ -65,6 +65,8 @@ export function NotificationPanel({ if (panel.current?.contains(target)) return; // A trigger closes this itself; counting it as outside would reopen it. if (target.closest?.("[data-notifications-trigger]")) return; + // The overflow menu is portaled out, so a click in it would read as outside the panel. + if (target.closest?.(".notification-bell__menu")) return; onClose(); }; const closeOnEscape = (event: KeyboardEvent) => { @@ -98,7 +100,7 @@ export function NotificationPanel({ {notifications.length === 0 ? (

    - {t("notifications.empty", "Nothing to report.")} + {t("notifications.empty", "You're all caught up.")}

    ) : (