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 cedb4d03c9..43925500ca 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 @@ -24,6 +24,12 @@ public enum FailureActionId { /** "No remediation will happen; close it." See {@link DismissAction}. */ DISMISS(Execution.SERVER, "Dismiss"), + /** Run the failed operation again on the document the client still holds. */ + RETRY(Execution.CLIENT, "Retry"), + + /** Ask the owner for the password, unlock the document in their client, then retry. */ + DECRYPT_AND_RETRY(Execution.CLIENT, "Decrypt and retry"), + /** Open the document this incident is about. Only its owner's client can resolve the 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..a604a239d0 --- /dev/null +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureActionSlot.java @@ -0,0 +1,21 @@ +package stirling.software.proprietary.failure; + +/** + * Where a kind would like one of its actions to sit: the thing that fixes it, a supporting action, + * or one folded away in a menu. + * + *

Intent, not layout. The client does the final promotion, because only it knows whether the + * document is still in its own file store, and a resolution it cannot run is worth less than a + * secondary action it can. Declaration order in {@link FailureKind} breaks a tie within a slot. + */ +public enum FailureActionSlot { + + /** The action that resolves the failure. At most one per kind is worth declaring here. */ + RESOLUTION, + + /** Offered alongside the resolution, for a caller the resolution is not aimed at. */ + SECONDARY, + + /** Available but folded away: correct, rarely the next thing anyone wants to press. */ + 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 978fbfacf2..39d5212fc0 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_AND_RETRY; import static stirling.software.proprietary.failure.FailureActionId.DISMISS; +import static stirling.software.proprietary.failure.FailureActionId.RETRY; 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; @@ -29,9 +33,9 @@ import lombok.Getter; * 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. * - *

Each offer also says who it is for, because the same incident is read by the person who hit it - * and by whoever reviews after them: only the owner holds the document, only a reviewer wants the - * run. + *

Each offer also says who it is for and where the kind wants it, because the same incident is + * read by the person who hit it and by whoever reviews after them: only the owner can supply a + * password, only a reviewer wants the run. */ @Getter public enum FailureKind { @@ -42,11 +46,13 @@ public enum FailureKind { FailureScope.FILE, errorCodes("E004"), fallback("This document is password-protected, so the pipeline could not read it."), - // Only the owner holds the document, so the file is theirs to open; a reviewer - // gets the run instead, and anyone who sees the row may close it. - offer(VIEW_FILE, OWNER), - offer(VIEW_IN_PROCESSOR, TEAM_REVIEWER), - offer(DISMISS, ANYONE_WHO_SEES)), + // The password is the fix and only the owner has it, so everyone else is + // offered the run and a way to close the row. + resolution(DECRYPT_AND_RETRY, OWNER), + global(RETRY, OWNER, OVERFLOW), + global(VIEW_FILE, OWNER, OVERFLOW), + global(VIEW_IN_PROCESSOR, TEAM_REVIEWER, SECONDARY), + global(DISMISS, ANYONE_WHO_SEES, OVERFLOW)), UNKNOWN( FailureStage.INTERNAL, @@ -55,14 +61,12 @@ public enum FailureKind { FailureScope.RUN, noErrorCodes(), fallback("This run failed for a reason Stirling does not yet recognise."), - // Nothing here is known to be fixable, so the offers are the places to look: - // the owner their document, a reviewer the run, and anyone may close the row. - // Declared in the same order as every other kind, because declaration order is - // display order: the document leads wherever it is offered, so a reader is not - // asked to re-learn which button leads from one failure to the next. - offer(VIEW_FILE, OWNER), - offer(VIEW_IN_PROCESSOR, TEAM_REVIEWER), - offer(DISMISS, ANYONE_WHO_SEES)); + // Nothing here is known to be fixable, so there is no resolution to declare. A plain + // retry is still worth offering: an unrecognised failure is often a one-off. + global(RETRY, OWNER, SECONDARY), + global(VIEW_IN_PROCESSOR, TEAM_REVIEWER, SECONDARY), + global(VIEW_FILE, OWNER, 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."; @@ -110,30 +114,52 @@ public enum FailureKind { } /** - * One action this kind offers: who it is for, and the key to label it by. One ordered list - * rather than ids plus parallel maps of audiences and label overrides, which could disagree - * with each other. + * One action this kind offers: who it is for, where it wants to sit, and the key to label it + * by. One ordered list rather than ids plus parallel maps of audiences, slots and label + * overrides, 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) {} + private record Offer( + FailureActionId id, + FailureAudience audience, + FailureActionSlot slot, + String labelKeySuffix) {} /** - * An action this kind offers, for whoever can actually take it, labelled by the shared wording. - * Declaration order is display order. + * The action that fixes this kind, for whoever can actually apply it. In the resolution slot by + * definition: a kind needing two of these would be two kinds. */ - private static Offer offer(FailureActionId id, FailureAudience audience) { - return new Offer(id, audience, null); + private static Offer resolution(FailureActionId id, FailureAudience audience) { + return new Offer(id, audience, FailureActionSlot.RESOLUTION, null); + } + + /** 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, FailureActionSlot.RESOLUTION, labelKeySuffix); } /** - * As {@link #offer(FailureActionId, FailureAudience)}, but labelled by this kind's own wording - * where the shared one reads badly. + * An action that is not this kind's fix: the same offer any kind can make, placed where this + * kind wants it and labelled by the shared wording. */ - private static Offer offer( - FailureActionId id, FailureAudience audience, String labelKeySuffix) { - return new Offer(id, audience, labelKeySuffix); + private static Offer global( + FailureActionId id, FailureAudience audience, FailureActionSlot slot) { + return new Offer(id, audience, slot, null); + } + + /** + * As {@link #global(FailureActionId, FailureAudience, FailureActionSlot)}, but labelled by 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); } /** @@ -173,20 +199,27 @@ public enum FailureKind { } /** - * 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 its label and placement resolved. What + * a review surface reads, so it never has to ask three separate questions about one offer. */ 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/FileRunEventService.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventService.java index fcdaf1de47..4863a391e1 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 @@ -173,6 +173,26 @@ public class FileRunEventService { return action.execute(event, inputs == null ? Map.of() : inputs, currentActor()); } + /** + * Mark an incident resolved, because a client retried the operation and it worked. Nobody is + * offered a "resolve" button, which is why {@code RESOLVED} is a status and not a {@link + * FailureActionId}. Idempotent: a client reporting the same success twice reads its row back. + * + * @throws FailureActionException if the event is not the caller's, or is already closed some + * other way + */ + public FileRunEvent resolve(String eventId) { + FileRunEvent event = requireVisible(eventId); + // No terminal pre-check: the store's guarded UPDATE decides, and tells a dismissed row + // apart from a deleted one after the fact rather than racing a read against the write. + return store.applyStatusOnce( + event.id(), + event.teamId(), + FileRunEventStatus.RESOLVED, + currentActor(), + FileRunEventStatus.open()); + } + /** * The event, if this caller may act on it at all. 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. @@ -232,7 +252,8 @@ public class FileRunEventService { boolean unattended, boolean documentless) { String reason = disabledReasonFor(offer.audience(), closed, unattended, documentless); - return new AvailableAction(offer.id(), offer.labelKey(), reason == null, reason); + return new AvailableAction( + offer.id(), offer.labelKey(), offer.slot(), reason == null, reason); } /** @@ -343,7 +364,14 @@ public class FileRunEventService { return applicationProperties.getSecurity().isEnableLogin(); } - /** One action as offered to one caller about one event, with its availability resolved. */ + /** + * One action as offered to one caller about one event, with its availability resolved. {@code + * slot} is the kind's placement intent, carried through for the client to make the final call. + */ 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/FileRunEventView.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventView.java index af68233e25..aa7621d83f 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 @@ -63,13 +63,15 @@ public record FileRunEventView( /** * One button, as offered to this caller about this row. {@code defaultLabel} and {@code * execution} are here for the reason {@code defaultTitle} is on the row: a client can then - * render, and route, an action it was never built with. Declaration order is display order. + * render, and route, 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) { @@ -80,6 +82,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 047376a655..915afeed98 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,14 +17,16 @@ import io.swagger.v3.oas.annotations.tags.Tag; import lombok.RequiredArgsConstructor; +import stirling.software.proprietary.failure.FailureActionException; + /** * The caller's notifications. Open to any authenticated user, unlike the failure endpoints it draws * on: each source scopes its own rows and resolves its own actions, so a member is told about their * own failures and a leader about their team's. * - *

Read-only: every action a notification offers is one the client runs on its own device, so - * there is nothing to post back here yet. Ids are still prefixed with their source, so the bell is - * never given the producing row's id; see {@link NotificationSource}. + *

Every action a notification offers is one the client runs on its own device, so the only write + * here is the client reporting that its own retry worked. Ids are prefixed with their source, so + * the bell is never given the producing row's id; see {@link NotificationSource}. */ @RestController @RequestMapping("/api/v1/notifications") @@ -47,6 +53,32 @@ public class NotificationController { return new NotificationsResponse(notifications.list(capped)); } + /** + * Record that the client's own retry of this notification worked. A mirror of the failure + * surface's resolve semantics rather than a client that strips the prefix, because the bell + * must never hand a raw failure id to a failure endpoint. + * + *

Idempotent. A row a reviewer has since dismissed is a conflict, not a silent overwrite of + * their decision. + */ + @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 rather than a bare array so paging or a total can be added without breaking clients. */ 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 690d8abf8b..a68ef01d29 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 @@ -11,9 +11,10 @@ import stirling.software.proprietary.failure.FileRunEventService; import stirling.software.proprietary.failure.FileRunEventView; /** - * Assembles the caller's notifications from whatever produces them. Derived on read rather than - * stored: there is one source today, and a table would need a write path, a retention story and a - * per-user read model before it earned itself. + * Assembles the caller's notifications from whatever produces them, and routes a client's report of + * its own fix back to whichever source produced the row. Derived on read rather than stored: there + * is one source today, and a table would need a write path, a retention story and a per-user read + * model before it earned itself. * *

Who sees what is decided by each source rather than here. {@link FileRunEventService} already * scopes its reads and resolves each row's actions against that reader, so this cannot widen either @@ -33,6 +34,32 @@ public class NotificationService { return fileRunEvents.list(null, null, limit).stream().map(this::fromFailure).toList(); } + /** + * Record that the client's own retry of this notification worked, and return it as it now + * stands. Takes the prefixed id even though nobody pressed a button: it is still a call made + * from the bell, and the bell holds no raw failure id, so it cannot reach a failure endpoint + * even by accident. + * + * @throws IllegalArgumentException if the id names no source this build has + * @throws stirling.software.proprietary.failure.FailureActionException if the source refuses, + * e.g. a row a reviewer has since dismissed + */ + 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)); + } + /** * One failure as a notification, with its row id prefixed on the way out and 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 bdb8bac680..e6870b6bd6 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,13 +1,16 @@ package stirling.software.proprietary.notification; +import java.util.Arrays; import java.util.Locale; +import java.util.Optional; /** * Which subsystem produced a notification. One member today; the point of the field is that a * client already branches on it, so a second source needs no client change to be ignorable. * - *

Every notification id is prefixed with its source, so a client never holds the producing row's - * own id and cannot hand it to that source's endpoints by accident. + *

Also the routing table for a notification id: every id is prefixed with its source, so an id + * handed back to a notification endpoint says which subsystem to ask, and a client never holds the + * producing row's own id. */ public enum NotificationSource { @@ -25,4 +28,27 @@ public enum NotificationSource { public String qualify(String sourceRowId) { return prefix() + sourceRowId; } + + /** + * The source a notification id belongs to, and the row id within it. Empty rather than throwing + * for an unprefixed id or an unknown source, since 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/failure/CheckConstrainedEnumsTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/CheckConstrainedEnumsTest.java index 15fd66cb1e..7ed759f8af 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 @@ -47,6 +47,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 0f0f451d65..ab167bff20 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; @@ -37,9 +40,12 @@ class FailureKindTest { * declaration that pairs 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 @@ -73,28 +79,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 offer a reader can use is the one - // rendered as the row's primary. Two kinds listing the same actions in different orders - // therefore flip the solid button between rows, which reads as a bug rather than as - // emphasis. Asserted as a shared ranking so a kind added later cannot reintroduce it. - 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<>(); @@ -125,13 +109,14 @@ class FailureKindTest { @ParameterizedTest @EnumSource(FailureKind.class) - void everyOfferSaysWhoItIsFor(FailureKind kind) { - // Read per row to decide what a caller is shown, so a missing one would be a button - // offered to whoever the null case happened to let through. + void everyOfferSaysWhoItIsForAndWhereItGoes(FailureKind kind) { + // Both are read per row to decide what a caller is shown, so a missing one would be a + // button placed by whatever the null case happened to do. 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(); } } @@ -143,6 +128,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 @@ -237,17 +233,19 @@ class FailureKindTest { class Unknown { @Test - void offersItsOwnerTheirDocumentAndTheRunToWhoeverReviews() { - // Nothing here is known to be fixable, so the offers are the places to look: the - // owner their document, a reviewer the run, and anyone may close the row. + void offersARetryToItsOwnerAndTheRunToWhoeverReviews() { + // Nothing here is known to be fixable, so there is no resolution. A retry is still + // worth offering the person who hit it: an unrecognised failure is often a one-off. assertThat(FailureKind.UNKNOWN.getOfferedActions()) .containsExactly( - offered(FailureActionId.VIEW_FILE, OWNER, "viewFile"), + offered(FailureActionId.RETRY, OWNER, SECONDARY, "retry"), offered( FailureActionId.VIEW_IN_PROCESSOR, TEAM_REVIEWER, + SECONDARY, "viewInProcessor"), - offered(FailureActionId.DISMISS, ANYONE_WHO_SEES, "dismiss")); + offered(FailureActionId.VIEW_FILE, OWNER, OVERFLOW, "viewFile"), + offered(FailureActionId.DISMISS, ANYONE_WHO_SEES, OVERFLOW, "dismiss")); } @Test @@ -300,17 +298,24 @@ class FailureKindTest { } @Test - void offersTheDocumentToItsOwnerAndTheRunToItsReviewer() { - // The whole point of the audiences: only the owner holds the document, so a reviewer - // is offered the run and a way to close the row instead. + void aKindWithSomethingToFixOffersTheFixToItsOwnerAndTheRunToItsReviewer() { + // The whole point of the audiences: the password is the fix and only the owner has it, + // so a reviewer is offered the run and a way to close the row instead. assertThat(FailureKind.INPUT_PASSWORD_PROTECTED.getOfferedActions()) .containsExactly( - offered(FailureActionId.VIEW_FILE, OWNER, "viewFile"), + offered( + FailureActionId.DECRYPT_AND_RETRY, + OWNER, + RESOLUTION, + "decryptAndRetry"), + offered(FailureActionId.RETRY, OWNER, OVERFLOW, "retry"), + offered(FailureActionId.VIEW_FILE, OWNER, OVERFLOW, "viewFile"), offered( FailureActionId.VIEW_IN_PROCESSOR, TEAM_REVIEWER, + SECONDARY, "viewInProcessor"), - offered(FailureActionId.DISMISS, ANYONE_WHO_SEES, "dismiss")); + offered(FailureActionId.DISMISS, ANYONE_WHO_SEES, OVERFLOW, "dismiss")); } @Test @@ -343,8 +348,8 @@ class FailureKindTest { .isEqualTo("portal.failures.action.dismiss"); assertThat( FailureKind.INPUT_PASSWORD_PROTECTED.labelKeyFor( - FailureActionId.VIEW_IN_PROCESSOR)) - .isEqualTo("portal.failures.action.viewInProcessor"); + FailureActionId.DECRYPT_AND_RETRY)) + .isEqualTo("portal.failures.action.decryptAndRetry"); } @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..d0375cdd7b 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.SECONDARY); 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..6351c52fa5 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("SECONDARY"); 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 cff75e2fff..094c770099 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 @@ -161,6 +161,75 @@ 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 successful client-side retry, so there is no id to dispatch and no + // button to render. Stated as a test because the absence is the property. + 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); + } + } + @Nested @DisplayName("triage never touches the document") class NeverTouchesTheDocument { @@ -359,14 +428,18 @@ class FileRunEventServiceTest { } @Test - void theOwnerIsOfferedTheirDocumentAndNotTheReviewersView() { - // A member reading their own password failure: the document is theirs to open, and the + void theOwnerIsOfferedTheFixAndNotTheReviewersView() { + // A member reading their own password failure: the unlock is theirs to do, and the // processor view is for whoever reviews the team rather than owns the file. 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_AND_RETRY, + FailureActionId.RETRY, + FailureActionId.VIEW_FILE, + FailureActionId.DISMISS); assertThat(service.availableActions(mine)) .allMatch(FileRunEventService.AvailableAction::enabled); } @@ -394,6 +467,8 @@ class FileRunEventServiceTest { assertThat(offeredFor(unattended)) .containsExactly( + FailureActionId.DECRYPT_AND_RETRY, + FailureActionId.RETRY, FailureActionId.VIEW_FILE, FailureActionId.VIEW_IN_PROCESSOR, FailureActionId.DISMISS); @@ -510,6 +585,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_AND_RETRY) + .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/NotificationProjectionTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/NotificationProjectionTest.java index b0ddd09fcc..37d14a435f 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 @@ -167,6 +167,7 @@ class NotificationProjectionTest { assertThat(action.labelKey()).startsWith("portal.failures.action."); assertThat(action.defaultLabel()).isNotBlank(); assertThat(action.execution()).isNotNull(); + assertThat(action.slot()).isNotNull(); }); } } 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..4aaa668e18 --- /dev/null +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/NotificationResolveTest.java @@ -0,0 +1,147 @@ +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. Lives beside the failure tests + * because the invariants are failure invariants: the bell holds only the prefixed notification id, + * so it cannot hand a raw event id to a failure endpoint, and the same service that scopes the + * queue decides whose row a caller may close. + */ +@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: a successful retry has to close its row, and the bell + // has no raw id to close it 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() { + // This mirror exists so no client has to strip the prefix, so an unprefixed id is + // refused rather than working by accident because there is only one source today. + 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); + } +} diff --git a/frontend/editor/public/locales/en-US/translation.toml b/frontend/editor/public/locales/en-US/translation.toml index a76db0ccff..9a1a6466bc 100644 --- a/frontend/editor/public/locales/en-US/translation.toml +++ b/frontend/editor/public/locales/en-US/translation.toml @@ -5058,13 +5058,19 @@ tags = "Multi Tool,Multi operation,UI,click drag,front end,client side,interacti title = "PDF Multi Tool" [notifications] +adoptFailed = "The document was unlocked but could not be opened here. Try the tool directly." 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." +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" +rerunRejected = "The policy could not be run again just now. Try again in a moment." +rerunUndelivered = "The policy re-run started, but its result cannot be delivered here, so this failure stays open." +retryUnavailable = "This document can no longer be retried from this browser." title = "Notifications" +unlockedNotRerun = "The document was unlocked and opened here, but the policy could not be run on it again." +unlockedRerunUndelivered = "The document was unlocked and the policy re-run started, but its result cannot be delivered here, so this failure stays open." unread = "Unread" [notifications.action] @@ -5077,6 +5083,11 @@ copy = "Copy error" less = "Show less" more = "Show full message" +[notifications.password] +cancel = "Cancel" +label = "Document password" +working = "Unlocking..." + [notifications.section] earlier = "Earlier" new = "New" @@ -7393,8 +7404,10 @@ title = "Failures" [portal.failures.action] acknowledge = "Acknowledge" confirm = "Are you sure?" +decryptAndRetry = "Decrypt and retry" dismiss = "Dismiss" dismissSkipFile = "Skip this file" +retry = "Retry" viewFile = "View file" viewInProcessor = "View in processor" diff --git a/frontend/editor/src/core/components/notifications/NotificationBell.css b/frontend/editor/src/core/components/notifications/NotificationBell.css index 9352f6bef8..0d6a1b5855 100644 --- a/frontend/editor/src/core/components/notifications/NotificationBell.css +++ b/frontend/editor/src/core/components/notifications/NotificationBell.css @@ -162,6 +162,14 @@ color: var(--c-text-subtle); } +.notification-bell__password { + grid-column: 2; + display: flex; + align-items: center; + gap: var(--sp-1, 0.25rem); + margin-top: var(--sp-2, 0.5rem); +} + .notification-bell__message { grid-column: 2; margin-top: var(--sp-1, 0.25rem); diff --git a/frontend/editor/src/core/components/notifications/NotificationBell.test.tsx b/frontend/editor/src/core/components/notifications/NotificationBell.test.tsx index 9237e9224c..c3b7b59029 100644 --- a/frontend/editor/src/core/components/notifications/NotificationBell.test.tsx +++ b/frontend/editor/src/core/components/notifications/NotificationBell.test.tsx @@ -9,6 +9,7 @@ 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. @@ -18,7 +19,8 @@ const render = (ui: Parameters[0]) => /** * The bell renders whatever the server sends, and does with each row's actions only what the registry * for this build says it can. Two things are its own and worth pinning: which notifications the user has - * already looked at, and how a row behaves around an action (message on failure, re-read on success). + * already looked at, and how a row behaves around an action (password first, message on failure, re-read + * on success). */ const fetchNotifications = vi.fn(); @@ -31,18 +33,21 @@ vi.mock("@app/services/notifications", () => ({ // availability is a fact of the test rather than of the environment. const h = vi.hoisted(() => ({ hasLocalFile: true, + retryPayload: { operation: "removePassword" } as unknown, specs: {} as Record< string, { available: (context: unknown) => boolean; run: (context: unknown, password?: string) => unknown; + needsPassword?: boolean; closesPanel?: boolean; } >, })); -vi.mock("@app/services/localFilePresence", () => ({ +vi.mock("@app/services/notificationRetry", () => ({ hasLocalFile: () => Promise.resolve(h.hasLocalFile), + loadRetryPayload: () => Promise.resolve(h.retryPayload), })); // Stands in for the layer that owns the destinations. Core's own registry is empty, so without @@ -75,12 +80,14 @@ 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, @@ -123,6 +130,7 @@ describe("NotificationBell", () => { window.localStorage.clear(); fetchNotifications.mockReset().mockResolvedValue([]); h.hasLocalFile = true; + h.retryPayload = { operation: "removePassword" }; h.specs = {}; }); @@ -359,7 +367,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(), ); @@ -376,7 +384,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(); }); @@ -413,7 +421,7 @@ describe("NotificationBell", () => { notification("a", "Unrecognised failure", { ownership: "UNOWNED", actions: [ - offer("VIEW_FILE", { + offer("VIEW_FILE", "SECONDARY", { enabled: false, disabledReasonKey: "portal.failures.disabled.unattended", }), @@ -443,11 +451,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", }), @@ -468,16 +476,59 @@ describe("NotificationBell", () => { expect(document.querySelector(".notification-bell__actions")).toBeNull(); }); - it("shows a failed action in the row instead of leaving the user guessing", async () => { + it("asks for the password in the row before it retries", async () => { + const run = vi.fn().mockResolvedValue({ ok: true }); h.specs = { - VIEW_FILE: { + DECRYPT_AND_RETRY: { available: () => true, - run: () => Promise.resolve({ ok: false, message: "Could not open" }), + run, + needsPassword: true, + closesPanel: true, }, }; fetchNotifications.mockResolvedValue([ notification("a", "Password-protected document", { - actions: [offer("VIEW_FILE")], + actions: [offer("DECRYPT_AND_RETRY", "RESOLUTION")], + }), + ]); + render(); + await openPanel(); + + // First click reveals the field rather than running anything. + fireEvent.click( + screen.getByRole("button", { + name: "DECRYPT_AND_RETRY: Password-protected document", + }), + ); + expect(run).not.toHaveBeenCalled(); + + const field = screen.getByLabelText( + "Document password: Password-protected document", + ); + fireEvent.change(field, { target: { value: "hunter2" } }); + fireEvent.submit(field.closest("form") as HTMLFormElement); + + await waitFor(() => expect(run).toHaveBeenCalledTimes(1)); + expect(run.mock.calls[0][1]).toBe("hunter2"); + // The server resolved the incident, so the list is re-read rather than patched here, and the + // panel gets out of the way of the document it just produced. + await waitFor(() => expect(fetchNotifications).toHaveBeenCalledTimes(2)); + await waitFor(() => + expect(screen.queryByText("Password-protected document")).toBeNull(), + ); + }); + + it("shows a failed unlock in the row instead of leaving the user guessing", async () => { + h.specs = { + DECRYPT_AND_RETRY: { + available: () => true, + run: () => Promise.resolve({ ok: false, message: "Wrong password" }), + needsPassword: true, + }, + }; + fetchNotifications.mockResolvedValue([ + notification("a", "Password-protected document", { + actions: [offer("DECRYPT_AND_RETRY", "RESOLUTION")], }), ]); render(); @@ -485,15 +536,20 @@ describe("NotificationBell", () => { fireEvent.click( screen.getByRole("button", { - name: "VIEW_FILE: Password-protected document", + name: "DECRYPT_AND_RETRY: Password-protected document", }), ); + const field = screen.getByLabelText( + "Document password: Password-protected document", + ); + fireEvent.change(field, { target: { value: "nope" } }); + fireEvent.submit(field.closest("form") as HTMLFormElement); expect(await screen.findByRole("alert")).toHaveProperty( "textContent", - "Could not open", + "Wrong password", ); - // Still on screen, so the row remains actionable. + // Still on screen, so the user can try another password. expect(screen.getByText("Password-protected document")).toBeTruthy(); }); diff --git a/frontend/editor/src/core/components/notifications/NotificationBell.tsx b/frontend/editor/src/core/components/notifications/NotificationBell.tsx index c9f7a8de6f..915732d86a 100644 --- a/frontend/editor/src/core/components/notifications/NotificationBell.tsx +++ b/frontend/editor/src/core/components/notifications/NotificationBell.tsx @@ -8,7 +8,7 @@ import { } from "react"; import type { TFunction } from "i18next"; import { useTranslation } from "react-i18next"; -import { Button } from "@app/ui"; +import { Button, Input } from "@app/ui"; import { BellIcon } from "@app/components/notifications/BellIcon"; import DividerWithText from "@app/components/shared/DividerWithText"; import { @@ -20,6 +20,7 @@ import { type ClientActionRegistry, type NotificationActionContext, } from "@app/components/notifications/notificationActions"; +import { promoteActions } from "@app/components/notifications/notificationActionSlots"; import type { AppNotification, NotificationActionOffer, @@ -36,7 +37,7 @@ import "@app/components/notifications/NotificationBell.css"; */ export function NotificationBell() { const { t } = useTranslation(); - const { notifications, unreadCount, documentStateFor, markAllSeen } = + const { notifications, unreadCount, documentStateFor, markAllSeen, refresh } = useNotifications(); const registry = useNotificationActions(); const [open, setOpen] = useState(false); @@ -180,6 +181,7 @@ export function NotificationBell() { documentState={documentStateFor(notification)} registry={registry} onDismissPanel={() => setOpen(false)} + onChanged={refresh} /> ))} @@ -214,12 +216,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; } @@ -230,11 +232,13 @@ interface NotificationItemProps { documentState: NotificationDocumentState; registry: ClientActionRegistry; onDismissPanel: () => void; + /** The row changed something server-side; re-read rather than patch a local copy. */ + onChanged: () => void; } /** - * One row. Its own component because the message its last attempt came back with and whether that - * message is expanded are per-row state the panel cannot hold. + * One row. Its own component because the password it is collecting, the message its last attempt came + * back with and whether that message is expanded are all per-row state the panel cannot hold. */ function NotificationItem({ notification, @@ -242,8 +246,13 @@ function NotificationItem({ documentState, registry, onDismissPanel, + onChanged, }: NotificationItemProps) { const { t } = useTranslation(); + // Held only while the field is open, and dropped as soon as the row is done with it. Never stashed, + // never logged. + const [password, setPassword] = useState(""); + const [askingFor, setAskingFor] = useState(null); const [message, setMessage] = useState(null); const [busy, setBusy] = useState(null); const [expanded, setExpanded] = useState(false); @@ -253,27 +262,19 @@ function NotificationItem({ const context: NotificationActionContext = { notification, hasLocalFile: documentState.hasLocalFile, + retryPayload: documentState.retryPayload, }; - // What this device can actually do, in the order the kind declared. 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; - }); - - // Why the row is thin, when the server withheld something and said so. Only from an action this - // build would otherwise have rendered, so a reason about an action it cannot perform anyway is not - // presented as the 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, not rendered unwired. The server ships new kinds + // and their actions ahead of the clients that understand them. + if (!spec) return false; + return spec.available(context); + }, + ); const labelOf = (offer: NotificationActionOffer) => t(offer.labelKey, offer.defaultLabel); @@ -284,9 +285,18 @@ function NotificationItem({ const spec = registry[offer.id]; if (!spec) return; + // First click on a password action opens the field, the second one runs it. An empty field means + // the user clicked the button again rather than filling it in, so there is nothing to send yet. + if (spec.needsPassword && (askingFor !== offer.id || password === "")) { + setAskingFor(offer.id); + return; + } setBusy(offer.id); - const outcome = await spec.run(context); + const outcome = await spec.run( + context, + spec.needsPassword ? password : undefined, + ); setBusy(null); if (outcome && !outcome.ok) { setMessage( @@ -299,6 +309,10 @@ function NotificationItem({ return; } + setPassword(""); + setAskingFor(null); + // A password action resolves the incident server-side, so the row is expected to drop out. + if (spec.needsPassword) onChanged(); if (spec.closesPanel) onDismissPanel(); }; @@ -312,6 +326,10 @@ function NotificationItem({ } }; + const asking = askingFor + ? (notification.actions.find((offer) => offer.id === askingFor) ?? null) + : null; + const note = noteFor(notification, documentState, withheldReasonKey, t); return ( @@ -381,15 +399,30 @@ function NotificationItem({ {/* Actions were taken away from this row, so say why rather than leaving a bare row. */} {note && {note}} - {/* Every action the row has, in the kind's declared order, the first leading. Three is the most - any kind offers once the unusable ones are dropped, so hiding the tail behind a menu would - cost more than it saves. */} - {usable.length > 0 && ( + {/* Every action the row has, in the server's order. Three is the most any kind offers once the + unusable ones are dropped, so hiding the tail behind a menu would cost more than it saves. */} + {primary && ( - {usable.map((offer, index) => ( + void run(primary)} + /> + {secondary && ( + void run(secondary)} + /> + )} + {overflow.map((offer) => ( )} + {asking && ( +

{ + event.preventDefault(); + void run(asking); + }} + > + setPassword(event.target.value)} + /> + + +
+ )} + {message && ( {message} @@ -410,7 +488,7 @@ function NotificationItem({ interface ActionButtonProps { /** Solid for the row's answer, outlined for its runner-up, ghost for the rest. */ - variant: "primary" | "secondary"; + variant: "primary" | "secondary" | "tertiary"; rowTitle: string; label: string; busy: boolean; diff --git a/frontend/editor/src/core/components/notifications/notificationActionSlots.test.ts b/frontend/editor/src/core/components/notifications/notificationActionSlots.test.ts new file mode 100644 index 0000000000..7ad6f19e4a --- /dev/null +++ b/frontend/editor/src/core/components/notifications/notificationActionSlots.test.ts @@ -0,0 +1,281 @@ +import { describe, expect, it } from "vitest"; +import { promoteActions } from "@app/components/notifications/notificationActionSlots"; +import type { + NotificationActionOffer, + NotificationActionSlot, +} from "@app/services/notifications"; + +/** + * The one rule that decides how loud a notification is allowed to be. Pinned against the shapes the + * server actually sends for the two failure kinds that exist, because the promotions are only + * correct in combination: what is left over depends on what won the buttons. + * + * Dispositions such as Dismiss never appear here: the projection carries only the actions the + * client itself runs, so the bell is never handed a button it would refuse to draw. + */ + +/** + * The offers as `FailureKind` declares them. An unrecognised failure has no known fix, so its retry + * is only ever a supporting action; a password failure has one, and its plain retry drops in behind + * the unlock. + */ +const UNKNOWN_OFFERS: Record = { + RETRY: offer("RETRY", "SECONDARY"), + VIEW_IN_PROCESSOR: offer("VIEW_IN_PROCESSOR", "SECONDARY"), + VIEW_FILE: offer("VIEW_FILE", "OVERFLOW"), +}; + +const PASSWORD_OFFERS: Record = { + DECRYPT_AND_RETRY: offer("DECRYPT_AND_RETRY", "RESOLUTION"), + RETRY: offer("RETRY", "OVERFLOW"), + VIEW_FILE: offer("VIEW_FILE", "OVERFLOW"), + VIEW_IN_PROCESSOR: offer("VIEW_IN_PROCESSOR", "SECONDARY"), +}; + +/** The reasons the server sends with an action it would refuse. */ +const NO_DOCUMENT = "portal.failures.disabled.noDocument"; +const UNATTENDED = "portal.failures.disabled.unattended"; +const CLOSED = "portal.failures.disabled.closed"; + +function offer( + id: string, + slot: NotificationActionSlot, + overrides: Partial = {}, +): NotificationActionOffer { + return { + id, + labelKey: `portal.failures.action.${id.toLowerCase()}`, + defaultLabel: id, + slot, + enabled: true, + disabledReasonKey: null, + ...overrides, + }; +} + +function from( + declared: Record, + ids: string[], +): NotificationActionOffer[] { + return ids.map((id) => { + const found = declared[id]; + if (!found) throw new Error(`That kind offers no ${id}`); + return found; + }); +} + +/** What the server sends for an unrecognised failure, for a reader offered these actions. */ +const unknown = (...ids: string[]) => from(UNKNOWN_OFFERS, ids); + +/** The same for a password-protected one. */ +const password = (...ids: string[]) => from(PASSWORD_OFFERS, ids); + +/** The same offers, with the named ones marked as the server would refuse them, and why. */ +function refusing( + offers: NotificationActionOffer[], + reasonKey: string, + ...ids: string[] +): NotificationActionOffer[] { + return offers.map((action) => + ids.includes(action.id) + ? { ...action, enabled: false, disabledReasonKey: reasonKey } + : action, + ); +} + +/** Everything this client can do, as the registry would answer with the file on this device. */ +const RUNNABLE = new Set([ + "RETRY", + "DECRYPT_AND_RETRY", + "VIEW_FILE", + "VIEW_IN_PROCESSOR", +]); + +/** The predicate the bell supplies: a known id, on a device that can act on it. */ +const canRun = (action: NotificationActionOffer) => RUNNABLE.has(action.id); + +function promoted(list: NotificationActionOffer[]) { + const { primary, secondary, overflow, withheldReasonKey } = promoteActions( + list, + canRun, + ); + return { + primary: primary?.id ?? null, + secondary: secondary?.id ?? null, + overflow: overflow.map((action) => action.id), + withheldReasonKey, + }; +} + +describe("promoteActions", () => { + it("gives the owner the retry, and keeps the rest quiet behind it", () => { + // No portal access, so the server never offered the processor link. + expect(promoted(unknown("RETRY", "VIEW_FILE"))).toEqual({ + primary: "RETRY", + secondary: null, + overflow: ["VIEW_FILE"], + withheldReasonKey: null, + }); + }); + + it("leads an attended policy failure with the queue, and states what was refused", () => { + // The document is not the reader's to open, so a greyed unlock would be false hope: the row loses + // the buttons and keeps the explanation. + expect( + promoted( + refusing( + unknown("RETRY", "VIEW_IN_PROCESSOR", "VIEW_FILE"), + NO_DOCUMENT, + "RETRY", + "VIEW_FILE", + ), + ), + ).toEqual({ + primary: "VIEW_IN_PROCESSOR", + secondary: null, + overflow: [], + withheldReasonKey: NO_DOCUMENT, + }); + }); + + it("leads an unattended failure with the queue, and says retrying is not available", () => { + // Nobody holds the document, so retrying is coming rather than missing. One reason for the row, + // taken from the best thing it lost. + expect( + promoted( + refusing( + unknown("RETRY", "VIEW_IN_PROCESSOR", "VIEW_FILE"), + UNATTENDED, + "RETRY", + "VIEW_FILE", + ), + ), + ).toEqual({ + primary: "VIEW_IN_PROCESSOR", + secondary: null, + overflow: [], + withheldReasonKey: UNATTENDED, + }); + }); + + it("explains nothing on a colleague's failure, having taken nothing away", () => { + // Not their document, so nothing that needs the bytes was offered at all. There is no loss to + // account for, and a note would only puzzle the reader. + expect(promoted(unknown("VIEW_IN_PROCESSOR"))).toEqual({ + primary: "VIEW_IN_PROCESSOR", + secondary: null, + overflow: [], + withheldReasonKey: null, + }); + }); + + it("leads a password failure with the unlock, not the plain retry", () => { + // Running it again unchanged is a second answer to the same problem, so it drops behind. + expect(promoted(password("DECRYPT_AND_RETRY", "RETRY", "VIEW_FILE"))).toEqual( + { + primary: "DECRYPT_AND_RETRY", + secondary: null, + overflow: ["RETRY", "VIEW_FILE"], + withheldReasonKey: null, + }, + ); + }); + + it("gives a reviewer their own password failure the unlock plus the queue", () => { + expect( + promoted( + password("DECRYPT_AND_RETRY", "RETRY", "VIEW_FILE", "VIEW_IN_PROCESSOR"), + ), + ).toEqual({ + primary: "DECRYPT_AND_RETRY", + secondary: "VIEW_IN_PROCESSOR", + overflow: ["RETRY", "VIEW_FILE"], + withheldReasonKey: null, + }); + }); + + it("leaves a closed row no buttons at all, only its reason", () => { + // Already resolved elsewhere: every offer is refused, so the row is its message plus one line + // saying why there is nothing left to do. + expect( + promoted( + refusing(unknown("RETRY", "VIEW_FILE"), CLOSED, "RETRY", "VIEW_FILE"), + ), + ).toEqual({ + primary: null, + secondary: null, + overflow: [], + withheldReasonKey: CLOSED, + }); + }); + + it("promotes past a resolution the shell cannot deliver", () => { + // The owner reading their own password failure from the processor: that shell has no FileContext, + // so the unlock has nowhere to put its output and reports itself unavailable. What is left is + // coherent on its own - the queue becomes the row's button. + const inProcessor = (action: NotificationActionOffer) => + action.id !== "DECRYPT_AND_RETRY" && canRun(action); + + const { primary, secondary, overflow } = promoteActions( + password("DECRYPT_AND_RETRY", "RETRY", "VIEW_FILE", "VIEW_IN_PROCESSOR"), + inProcessor, + ); + + expect(primary?.id).toBe("VIEW_IN_PROCESSOR"); + expect(secondary).toBeNull(); + expect(overflow.map((action) => action.id)).toEqual(["RETRY", "VIEW_FILE"]); + }); + + it("drops a client action this device cannot perform, without inventing a reason", () => { + // The document is gone from this browser: the actions disappear rather than failing on click. The + // server withheld nothing, so the row has no server reason and the bell falls back to what this + // device knows. + const { primary, overflow, withheldReasonKey } = promoteActions( + unknown("RETRY", "VIEW_FILE"), + () => false, + ); + + expect(primary).toBeNull(); + expect(overflow).toEqual([]); + expect(withheldReasonKey).toBeNull(); + }); + + it("skips an action id it has never heard of without touching the rest", () => { + // The server ships a kind with a new action before this build knows what it means. + const list = [offer("QUARANTINE", "RESOLUTION"), ...unknown("RETRY")]; + + expect(promoted(list)).toEqual({ + primary: "RETRY", + secondary: null, + overflow: [], + withheldReasonKey: null, + }); + }); + + it("has nothing to promote when nothing survives", () => { + expect(promoteActions([], () => true)).toEqual({ + primary: null, + secondary: null, + overflow: [], + withheldReasonKey: null, + }); + }); + + it("takes the reason from the best action lost, not the first declared", () => { + // Two refusals, one row: the reader gets the one attached to the action they would have reached + // for first. + const list = [ + offer("VIEW_FILE", "OVERFLOW", { + enabled: false, + disabledReasonKey: CLOSED, + }), + offer("DECRYPT_AND_RETRY", "RESOLUTION", { + enabled: false, + disabledReasonKey: NO_DOCUMENT, + }), + ...password("VIEW_IN_PROCESSOR"), + ]; + + expect(promoted(list).withheldReasonKey).toBe(NO_DOCUMENT); + }); +}); diff --git a/frontend/editor/src/core/components/notifications/notificationActionSlots.ts b/frontend/editor/src/core/components/notifications/notificationActionSlots.ts new file mode 100644 index 0000000000..ab605eee1c --- /dev/null +++ b/frontend/editor/src/core/components/notifications/notificationActionSlots.ts @@ -0,0 +1,79 @@ +import type { + NotificationActionOffer, + NotificationActionSlot, +} from "@app/services/notifications"; + +/** + * Where each of a row's actions ends up on screen. The server says what an action does and how much of + * the row it has earned; this turns that into an order and a prominence. A pure function because it is + * the one piece of the bell that decides prominence, and every failure kind goes through it. + */ + +const SLOT_RANK: Record = { + RESOLUTION: 0, + SECONDARY: 1, + OVERFLOW: 2, +}; + +export interface PromotedActions { + /** The row's own button. Null when nothing survived the filter. */ + primary: NotificationActionOffer | null; + /** A second button, only ever an action the server marked SECONDARY. */ + secondary: NotificationActionOffer | null; + /** Everything else, in the server's order, for the row to render quietly after those two. */ + overflow: NotificationActionOffer[]; + /** + * The reason the server gave for the best action it withheld, for the row to state once. Null when + * it withheld nothing, or gave no reason. + */ + withheldReasonKey: string | null; +} + +/** + * Promote a row's offers into one primary button, at most one secondary button, and the quiet rest. + * + * Every offer the bell is given is one this client runs itself, so each is asked past + * `canRenderClientAction`: whether this build knows the id, and whether this device can currently + * perform it. + * + * A dropped action leaves no hole, and a disabled one is dropped too: a button that can never work is + * false hope. Its reason comes back instead, for the row to say in words. + */ +export function promoteActions( + offers: readonly NotificationActionOffer[], + canRenderClientAction: (offer: NotificationActionOffer) => boolean, +): PromotedActions { + const ranked = offers + .map((offer, declaredAt) => ({ offer, declaredAt })) + // Slot first, then declaration order, so two actions in one slot keep the server's ranking. + .sort( + (a, b) => + SLOT_RANK[a.offer.slot] - SLOT_RANK[b.offer.slot] || + a.declaredAt - b.declaredAt, + ) + .map(({ offer }) => offer); + + // The best one withheld, so a row explains itself once rather than once per lost action. + const withheldReasonKey = + ranked.find((offer) => !offer.enabled && offer.disabledReasonKey) + ?.disabledReasonKey ?? null; + + const renderable = ranked.filter( + (offer) => offer.enabled && canRenderClientAction(offer), + ); + + const [primary, next, ...rest] = renderable; + if (!primary) + return { primary: null, secondary: null, overflow: [], withheldReasonKey }; + + // Only if the server ranked it SECONDARY: a second RESOLUTION would read as two answers to the same + // problem, and an OVERFLOW one was ranked below the row's own buttons by the server itself. + const secondary = next?.slot === "SECONDARY" ? next : null; + + return { + primary, + secondary, + overflow: secondary ? rest : next ? [next, ...rest] : rest, + withheldReasonKey, + }; +} diff --git a/frontend/editor/src/core/components/notifications/notificationActions.ts b/frontend/editor/src/core/components/notifications/notificationActions.ts index b9ffddaed8..53c847443e 100644 --- a/frontend/editor/src/core/components/notifications/notificationActions.ts +++ b/frontend/editor/src/core/components/notifications/notificationActions.ts @@ -1,4 +1,5 @@ import type { AppNotification } from "@app/services/notifications"; +import type { RetryPayload } from "@app/services/notificationRetry"; /** * What this client can do about a notification, keyed by the action id the server offered. Keyed by @@ -11,6 +12,8 @@ export interface NotificationActionContext { notification: AppNotification; /** Whether the document is still in this browser, which is what most actions hinge on. */ hasLocalFile: boolean; + /** What the failed operation was, when this browser stashed it. */ + retryPayload: RetryPayload | null; } /** @@ -26,10 +29,13 @@ export interface ClientActionOutcome { export interface ClientActionSpec { /** Whether this device can perform it right now. Asked per row, never during a request. */ available(context: NotificationActionContext): boolean; - /** May answer synchronously. */ + /** `password` is only ever passed for a spec that asked for one. May answer synchronously. */ run( context: NotificationActionContext, + password?: string, ): ClientActionOutcome | void | Promise; + /** Collect a password in the row before running. Never stored, never logged. */ + needsPassword?: boolean; /** Whether the panel should get out of the way, because the destination is behind it. */ closesPanel?: boolean; } diff --git a/frontend/editor/src/core/hooks/tools/shared/useToolOperation.ts b/frontend/editor/src/core/hooks/tools/shared/useToolOperation.ts index 60ae392ca0..395d4967be 100644 --- a/frontend/editor/src/core/hooks/tools/shared/useToolOperation.ts +++ b/frontend/editor/src/core/hooks/tools/shared/useToolOperation.ts @@ -21,7 +21,11 @@ import { StirlingFileStub, } from "@app/types/fileContext"; import { FILE_EVENTS } from "@app/services/errorUtils"; -import { reportToolFailure } from "@app/services/failureReporting"; +import { + reportToolFailure, + wasCancelled, +} from "@app/services/failureReporting"; +import { stashRetryPayload } from "@app/services/notificationRetry"; import { refreshNotificationsNow } from "@app/hooks/useNotifications"; import { zipFileService } from "@app/services/zipFileService"; import { getFilenameWithoutExtension } from "@app/utils/fileUtils"; @@ -615,6 +619,20 @@ export const useToolOperation = ( fileIds: validFiles.map((file) => file.fileId), }).then(refreshNotificationsNow); + // Keep what a retry would need, since the report itself carries no + // operation and answers 204. Gated on the reporter's own cancellation + // test so the two cannot disagree about what counts as a failure, and on + // there being an endpoint: a custom processor has nothing to re-submit to. + if (!wasCancelled(error) && runtimeEndpoint) { + void stashRetryPayload({ + operation: config.operationType, + endpoint: runtimeEndpoint, + params: params as Record, + fileIds: validFiles.map((file) => file.fileId), + recordedAt: Date.now(), + }); + } + const errorMessage = config.getErrorMessage?.(error) || extractErrorMessage(error); actions.setError(errorMessage); diff --git a/frontend/editor/src/core/hooks/useNotifications.test.ts b/frontend/editor/src/core/hooks/useNotifications.test.ts index 1a13f75bf6..3c6dda45b8 100644 --- a/frontend/editor/src/core/hooks/useNotifications.test.ts +++ b/frontend/editor/src/core/hooks/useNotifications.test.ts @@ -18,9 +18,11 @@ vi.mock("@app/services/notifications", () => ({ // The document lookups read IndexedDB, which jsdom has none of. Counted here so that "resolved once // per list, not once per row" is observable. const hasLocalFile = vi.fn((_fileId: string) => Promise.resolve(true)); +const loadRetryPayload = vi.fn((_fileId: string) => Promise.resolve(null)); -vi.mock("@app/services/localFilePresence", () => ({ +vi.mock("@app/services/notificationRetry", () => ({ hasLocalFile: (fileId: string) => hasLocalFile(fileId), + loadRetryPayload: (fileId: string) => loadRetryPayload(fileId), })); const { useNotifications } = await import("@app/hooks/useNotifications"); @@ -56,6 +58,7 @@ describe("useNotifications", () => { window.localStorage.clear(); fetchNotifications.mockReset().mockResolvedValue([]); hasLocalFile.mockClear(); + loadRetryPayload.mockClear(); }); it("reads the list once however many bells are mounted", async () => { @@ -82,6 +85,7 @@ describe("useNotifications", () => { await waitFor(() => expect(result.current.notifications).toHaveLength(3)); expect(hasLocalFile).toHaveBeenCalledTimes(2); + expect(loadRetryPayload).toHaveBeenCalledTimes(2); }); it("looks up an attended run's document but never an unattended run's", async () => { diff --git a/frontend/editor/src/core/hooks/useNotifications.ts b/frontend/editor/src/core/hooks/useNotifications.ts index d1cf58daa9..405bc2d664 100644 --- a/frontend/editor/src/core/hooks/useNotifications.ts +++ b/frontend/editor/src/core/hooks/useNotifications.ts @@ -3,7 +3,11 @@ import { fetchNotifications, type AppNotification, } from "@app/services/notifications"; -import { hasLocalFile } from "@app/services/localFilePresence"; +import { + hasLocalFile, + loadRetryPayload, + type RetryPayload, +} from "@app/services/notificationRetry"; /** * The caller's notifications, refreshed on a timer because they arrive from background work rather @@ -50,10 +54,12 @@ function writeLastSeenId(id: string): void { */ export interface NotificationDocumentState { hasLocalFile: boolean; + retryPayload: RetryPayload | null; } const NO_DOCUMENT: NotificationDocumentState = { hasLocalFile: false, + retryPayload: null, }; /** @@ -132,6 +138,7 @@ async function read(forCycle: number): Promise { fileId, { hasLocalFile: await hasLocalFile(fileId), + retryPayload: await loadRetryPayload(fileId), }, ] as const, ), diff --git a/frontend/editor/src/core/services/failureReporting.ts b/frontend/editor/src/core/services/failureReporting.ts index af0765f8bd..702ee57669 100644 --- a/frontend/editor/src/core/services/failureReporting.ts +++ b/frontend/editor/src/core/services/failureReporting.ts @@ -165,8 +165,11 @@ function messageOf(error: unknown): string { *

Everything else is reported, client-side refusals included: an unsupported input * format is the same class of problem as the processor rejecting a file type, which * is already recorded. + * + *

Exported for `useToolOperation`, which must not stash a retry for a run the user + * cancelled themselves. */ -function wasCancelled(error: unknown): boolean { +export function wasCancelled(error: unknown): boolean { const candidate = error as { code?: unknown; name?: unknown; diff --git a/frontend/editor/src/core/services/localFilePresence.test.ts b/frontend/editor/src/core/services/localFilePresence.test.ts deleted file mode 100644 index 1f00d03c0b..0000000000 --- a/frontend/editor/src/core/services/localFilePresence.test.ts +++ /dev/null @@ -1,36 +0,0 @@ -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 deleted file mode 100644 index 2ba39b526b..0000000000 --- a/frontend/editor/src/core/services/localFilePresence.ts +++ /dev/null @@ -1,21 +0,0 @@ -import { fileStorage } from "@app/services/fileStorage"; -import type { FileId } from "@app/types/file"; - -/** - * Whether the document a failure was filed against is still in this browser. It decides whether the - * bell can offer to open it: the id is this workspace's own, so no other device can answer yes. - */ -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/notificationRetry.test.ts b/frontend/editor/src/core/services/notificationRetry.test.ts new file mode 100644 index 0000000000..f14dec8ad7 --- /dev/null +++ b/frontend/editor/src/core/services/notificationRetry.test.ts @@ -0,0 +1,333 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import "fake-indexeddb/auto"; +import { indexedDBManager } from "@app/services/indexedDBManager"; + +/** + * Tests for the notification bell's retry stash. Three properties matter: a + * retry offered by the bell can still find what it needs after a reload, the + * store cannot grow without bound, and a password never lands in it. + */ + +const getStirlingFileStub = vi.fn(); +const getStirlingFiles = vi.fn(); +const post = vi.fn(); + +vi.mock("@app/services/fileStorage", () => ({ + fileStorage: { + getStirlingFileStub: (...args: unknown[]) => getStirlingFileStub(...args), + getStirlingFiles: (...args: unknown[]) => getStirlingFiles(...args), + }, +})); + +vi.mock("@app/services/apiClient", () => ({ + default: { post: (...args: unknown[]) => post(...args) }, +})); + +const { + stashRetryPayload, + loadRetryPayload, + hasLocalFile, + retryWithPassword, + unlockLocalDocument, +} = await import("@app/services/notificationRetry"); + +/** Duplicated from the service, which keeps its storage details private. */ +const DB_NAME = "stirling-pdf-retry"; +const STORE_NAME = "retryPayloads"; + +function payload(overrides: Partial> = {}) { + return { + operation: "remove-password", + endpoint: "/api/v1/security/remove-password", + params: {}, + fileIds: ["f-1"], + recordedAt: 1_000, + ...overrides, + } as Parameters[0]; +} + +/** Reads records straight out of IndexedDB, bypassing the service's own mapping. */ +async function storedRecords(): Promise[]> { + const db = await indexedDBManager.openDatabase({ + name: DB_NAME, + version: 1, + stores: [{ name: STORE_NAME, keyPath: "fileId" }], + }); + return new Promise((resolve, reject) => { + const request = db + .transaction([STORE_NAME], "readonly") + .objectStore(STORE_NAME) + .getAll(); + request.onsuccess = () => + resolve((request.result ?? []) as Record[]); + request.onerror = () => reject(request.error); + }); +} + +beforeEach(async () => { + getStirlingFileStub.mockReset().mockResolvedValue(null); + getStirlingFiles.mockReset().mockResolvedValue([]); + post.mockReset().mockResolvedValue({ status: 200, data: new Blob() }); + await indexedDBManager.deleteDatabase(DB_NAME); +}); + +describe("the retry stash", () => { + it("gives back what was stashed, keyed on the file the failure was filed against", async () => { + await stashRetryPayload( + payload({ params: { onlyPages: "1-3" }, fileIds: ["f-1", "f-2"] }), + ); + + await expect(loadRetryPayload("f-1")).resolves.toEqual({ + operation: "remove-password", + endpoint: "/api/v1/security/remove-password", + params: { onlyPages: "1-3" }, + fileIds: ["f-1", "f-2"], + recordedAt: 1_000, + }); + // Every file in the run gets a record, so the bell can retry from any of them. + await expect(loadRetryPayload("f-2")).resolves.toMatchObject({ + operation: "remove-password", + }); + }); + + it("has nothing for a file it never saw, or for no file at all", async () => { + await stashRetryPayload(payload()); + + await expect(loadRetryPayload("f-other")).resolves.toBeNull(); + await expect(loadRetryPayload(null)).resolves.toBeNull(); + await expect(loadRetryPayload(" ")).resolves.toBeNull(); + }); + + it("keeps the most recent operation that failed on a file, matching the server's one-incident-per-file dedup", async () => { + await stashRetryPayload( + payload({ operation: "compress", endpoint: "/api/v1/misc/compress-pdf" }), + ); + await stashRetryPayload( + payload({ operation: "rotate", endpoint: "/api/v1/general/rotate-pdf" }), + ); + + await expect(loadRetryPayload("f-1")).resolves.toMatchObject({ + operation: "rotate", + endpoint: "/api/v1/general/rotate-pdf", + }); + expect(await storedRecords()).toHaveLength(1); + }); + + it("evicts the oldest once it is full, so it cannot grow for the lifetime of the origin", async () => { + // One past the cap: the first failure stashed is the one that goes. + for (let i = 0; i < 26; i += 1) { + await stashRetryPayload(payload({ fileIds: [`f-${i}`], recordedAt: i })); + } + + expect(await storedRecords()).toHaveLength(25); + await expect(loadRetryPayload("f-0")).resolves.toBeNull(); + await expect(loadRetryPayload("f-25")).resolves.toMatchObject({ + operation: "remove-password", + }); + }); + + it("stores no password, whichever field the tool submitted it in", async () => { + await stashRetryPayload( + payload({ + params: { + password: "hunter2", + newOwnerPassword: "hunter2", + passphrase: "hunter2", + apiToken: "hunter2", + nested: { ownerPassword: "hunter2", keep: "yes" }, + keepThese: ["a", "b"], + }, + }), + ); + + const stored = await storedRecords(); + expect(JSON.stringify(stored)).not.toContain("hunter2"); + // No password-shaped field survives either. Scoped to params, since the tool + // this failure came from is itself called remove-password. + expect(JSON.stringify(stored.map((record) => record.params))).not.toMatch( + /pass(word|phrase)|token/i, + ); + // The rest of the parameters survive: without them a retry re-runs a + // different operation than the one that failed. + expect((await loadRetryPayload("f-1"))?.params).toEqual({ + nested: { keep: "yes" }, + keepThese: ["a", "b"], + }); + }); + + it("stops descending into a pathologically deep object without exhausting the stack", async () => { + // 5000 levels: enough to overflow an unbounded walk, and nothing a tool would ever submit. + let deep: Record = { bottom: "reached" }; + for (let i = 0; i < 5000; i++) deep = { down: deep }; + + await expect( + stashRetryPayload(payload({ params: { deep } })), + ).resolves.toBeUndefined(); + expect(await loadRetryPayload("f-1")).not.toBeNull(); + }); + + it("drops a secret sitting just past the depth limit rather than passing the subtree through", async () => { + // Deliberately only a little past the limit, so the truncated subtree is small enough to store. + // A far deeper object would fail to store for unrelated reasons and pass this vacuously. + let past: Record = { password: "hunter2" }; + for (let i = 0; i < 25; i++) past = { down: past }; + + await stashRetryPayload(payload({ params: { past } })); + + // The point where the walk gives up is the one place it must not hand back a subtree it never + // examined: returning the value there would persist every secret below the limit. + expect(JSON.stringify(await storedRecords())).not.toContain("hunter2"); + }); + + it("survives a cycle in the parameters", async () => { + // A depth bound is what saves this: a cycle has no leaves to reach. + const cyclic: Record = { keep: "yes" }; + cyclic.self = cyclic; + + await expect( + stashRetryPayload(payload({ params: { cyclic } })), + ).resolves.toBeUndefined(); + expect(await loadRetryPayload("f-1")).not.toBeNull(); + }); +}); + +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); + }); +}); + +describe("retryWithPassword", () => { + it("reports the file is gone instead of throwing, which is an expected outcome here", async () => { + getStirlingFiles.mockResolvedValue([]); + + const result = await retryWithPassword(payload(), "hunter2"); + + expect(result.ok).toBe(false); + expect(result.message).toBeTruthy(); + expect(post).not.toHaveBeenCalled(); + }); + + it("re-submits the stashed operation with the password added", async () => { + getStirlingFiles.mockResolvedValue([ + new File(["%PDF-1.7"], "doc.pdf", { type: "application/pdf" }), + ]); + + const result = await retryWithPassword( + payload({ params: { onlyPages: "1-3" } }), + "hunter2", + ); + + expect(result.ok).toBe(true); + const [path, formData] = post.mock.calls[0] as [string, FormData]; + expect(path).toBe("/api/v1/security/remove-password"); + expect(formData.get("password")).toBe("hunter2"); + expect(formData.get("onlyPages")).toBe("1-3"); + expect(formData.get("fileInput")).toBeInstanceOf(File); + // The password was used for the one call and nothing else. + expect(JSON.stringify(await storedRecords())).not.toContain("hunter2"); + }); + + it("hands the output back, since a retry the user cannot see the result of is no retry", async () => { + getStirlingFiles.mockResolvedValue([new File(["%PDF-1.7"], "doc.pdf")]); + const unlocked = new Blob(["unlocked"]); + post.mockResolvedValue({ + data: unlocked, + headers: { + "content-disposition": 'attachment; filename="doc_unlocked.pdf"', + }, + }); + + const result = await retryWithPassword(payload(), "hunter2"); + + expect(result.ok).toBe(true); + expect(result.files).toHaveLength(1); + expect(result.files?.[0].filename).toBe("doc_unlocked.pdf"); + // The response body itself, so the caller adopts the bytes the server sent. + expect(result.files?.[0].blob).toBe(unlocked); + }); + + it("names the output after its input when the server sent no filename", async () => { + getStirlingFiles.mockResolvedValue([new File(["%PDF-1.7"], "doc.pdf")]); + post.mockResolvedValue({ data: new Blob(["unlocked"]), headers: {} }); + + const result = await retryWithPassword(payload(), "hunter2"); + + expect(result.files?.[0].filename).toBe("doc.pdf"); + }); + + it("returns the server's own message when the retry fails again", async () => { + getStirlingFiles.mockResolvedValue([new File(["%PDF-1.7"], "doc.pdf")]); + post.mockRejectedValue({ + response: { data: "The password is incorrect." }, + message: "Request failed with status code 400", + }); + + const result = await retryWithPassword(payload(), "hunter2"); + + expect(result.ok).toBe(false); + expect(result.message).toBe("The password is incorrect."); + expect(result.message).not.toContain("hunter2"); + }); +}); + +/** + * The unlock for a failure with no stash behind it - an attended policy run, whose notification + * names the document and needs nothing else. Same request, fixed endpoint, no payload. + */ +describe("unlockLocalDocument", () => { + it("removes the password from the document this browser holds, and stores nothing", async () => { + getStirlingFiles.mockResolvedValue([ + new File(["%PDF-1.7"], "locked.pdf", { type: "application/pdf" }), + ]); + post.mockResolvedValue({ + data: new Blob(["unlocked"]), + headers: { + "content-disposition": 'attachment; filename="locked_unlocked.pdf"', + }, + }); + + const result = await unlockLocalDocument("f-1", "hunter2"); + + const [path, formData] = post.mock.calls[0] as [string, FormData]; + expect(path).toBe("/api/v1/security/remove-password"); + expect(formData.get("password")).toBe("hunter2"); + expect(formData.get("fileInput")).toBeInstanceOf(File); + expect(result.files?.[0].filename).toBe("locked_unlocked.pdf"); + // The password was used for the one call and nothing else: no stash is written here at all. + expect(await storedRecords()).toHaveLength(0); + }); + + it("reports the document is gone instead of posting a password nowhere", async () => { + getStirlingFiles.mockResolvedValue([]); + + const result = await unlockLocalDocument("f-1", "hunter2"); + + expect(result.ok).toBe(false); + expect(result.message).toBeTruthy(); + expect(post).not.toHaveBeenCalled(); + }); + + it("returns the server's own message when the password is wrong", async () => { + getStirlingFiles.mockResolvedValue([new File(["%PDF-1.7"], "locked.pdf")]); + post.mockRejectedValue({ + response: { data: "The password is incorrect." }, + message: "Request failed with status code 400", + }); + + const result = await unlockLocalDocument("f-1", "wrong"); + + expect(result.ok).toBe(false); + expect(result.message).toBe("The password is incorrect."); + expect(result.message).not.toContain("wrong"); + }); +}); diff --git a/frontend/editor/src/core/services/notificationRetry.ts b/frontend/editor/src/core/services/notificationRetry.ts new file mode 100644 index 0000000000..0e1ca43289 --- /dev/null +++ b/frontend/editor/src/core/services/notificationRetry.ts @@ -0,0 +1,370 @@ +import apiClient from "@app/services/apiClient"; +import { fileStorage } from "@app/services/fileStorage"; +import { + indexedDBManager, + type DatabaseConfig, +} from "@app/services/indexedDBManager"; +import type { FileId } from "@app/types/file"; +import type { ToolEndpoint } from "@app/types/toolApiTypes"; + +/** + * What the notification bell needs to offer "Retry" or "Decrypt and retry" on a + * failure the editor reported. The server keeps none of it: the report drops the + * operation and answers 204, so this lives here, keyed on the opaque `fileId` it was + * filed against. Last-write-wins per fileId, matching the server's actor|kind|file + * dedup: one file failing two operations is one incident with one retry button. + */ +export interface RetryPayload { + /** tool/endpoint identifier, e.g. "remove-password" */ + operation: string; + /** the API path that failed, so a retry needs no tool registry lookup */ + endpoint: string; + /** the tool parameters as submitted */ + params: Record; + fileIds: string[]; + recordedAt: number; +} + +/** + * Its own database rather than a store on `stirling-pdf-files`: that schema has + * shipped at v9, and adding a store there means a version bump plus an upgrade path + * on every install for a hint that is safe to lose. Still opened through + * `indexedDBManager`, so this is not a second way to reach IndexedDB. + */ +const RETRY_DB_CONFIG: DatabaseConfig = { + name: "stirling-pdf-retry", + version: 1, + stores: [{ name: "retryPayloads", keyPath: "fileId" }], +}; + +const STORE_NAME = "retryPayloads"; + +/** Capped, oldest evicted first, so the stash cannot grow for the origin's lifetime. */ +const MAX_RETAINED_PAYLOADS = 25; + +/** One record per file involved, so a retry can be found from any of them. */ +interface StoredRetryRecord extends RetryPayload { + fileId: string; +} + +/** + * Secret-looking field names. A tool's parameters can carry one (remove-password submits + * `password`), so they are stripped on the way in rather than trusted to be absent. + */ +const SECRET_FIELD = /pass(word|phrase)|secret|token|credential/i; + +/** + * Stash the retry payload for a failure that was just reported. Never rejects, like + * `reportToolFailure`: a browser that refuses IndexedDB should cost the user the retry + * button, not a second error on top of the failure they already have. + */ +export async function stashRetryPayload(payload: RetryPayload): Promise { + try { + const fileIds = payload.fileIds.filter(isUsableId); + if (!payload.operation.trim() || fileIds.length === 0) return; + + const record = { + ...payload, + fileIds, + // Persisting a password would defeat the point of asking for it again. + params: withoutSecrets(payload.params), + }; + + await writeRecords(fileIds.map((fileId) => ({ ...record, fileId }))); + } catch { + // Nothing to recover: the bell simply offers no retry for this failure. + } +} + +/** The most recent operation that failed on this file, or null when nothing is stashed. */ +export async function loadRetryPayload( + fileId: string | null, +): Promise { + if (!isUsableId(fileId)) return null; + + let record: StoredRetryRecord | undefined; + try { + record = await readRecord(fileId); + } catch { + return null; + } + if (!record) return null; + + // A record written by an older shape of this service is unusable rather than + // half-usable: a retry with no endpoint has nowhere to go. + if (!record.operation || !record.endpoint) return null; + + return { + operation: record.operation, + endpoint: record.endpoint, + params: record.params ?? {}, + fileIds: record.fileIds ?? [fileId], + recordedAt: record.recordedAt, + }; +} + +/** + * Whether the document is still in this browser, which decides whether a retry can run at + * all. Null once the user deletes the file, and on every other device they own. + */ +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; + } +} + +/** A file the retry produced, handed back for the caller to adopt. */ +export interface RetryOutputFile { + blob: Blob; + filename: string; +} + +/** What a password-carrying call comes back with. `files` only ever on success. */ +export interface PasswordRetryOutcome { + ok: boolean; + message?: string; + files?: RetryOutputFile[]; +} + +/** Checked against the generated endpoints, so a renamed route fails the build here. */ +const UNLOCK_ENDPOINT = + "/api/v1/security/remove-password" satisfies ToolEndpoint; + +/** + * Unlock a document this browser is holding, for a failure with no stashed operation such + * as an attended policy run: a password-protected input is fixed the same way whatever was + * reading it. Same contract as {@link retryWithPassword} otherwise. + */ +export async function unlockLocalDocument( + fileId: string, + password: string, +): Promise { + return postWithPassword(UNLOCK_ENDPOINT, {}, [fileId], password); +} + +/** + * Re-run the stashed operation with the password the user just typed, and hand back + * what it produced. The password is appended to a single request and then out of + * scope: never stashed, never logged, never in the message returned here. + * + * `files` is returned rather than adopted because every file operation goes through + * FileContext, which a service cannot reach. + */ +export async function retryWithPassword( + payload: RetryPayload, + password: string, +): Promise { + if (!payload.endpoint) { + return { ok: false, message: "This operation cannot be retried." }; + } + + return postWithPassword( + payload.endpoint, + payload.params, + payload.fileIds, + password, + ); +} + +/** Shared by both callers above, so a password reaches the network from one place only. */ +async function postWithPassword( + endpoint: string, + params: Record, + requestedFileIds: string[], + password: string, +): Promise { + const fileIds = requestedFileIds.filter(isUsableId); + let files: File[] = []; + try { + files = await fileStorage.getStirlingFiles(fileIds as FileId[]); + } catch { + files = []; + } + + // getStirlingFiles drops what it cannot find, so a short result means an input is + // gone. Resolved rather than thrown: the caller shows this next to the notification. + if (files.length === 0 || files.length !== fileIds.length) { + return { + ok: false, + message: + "This file is no longer stored in this browser, so it cannot be retried here.", + }; + } + + try { + const formData = toFormData(params, files); + formData.append("password", password); + const response = await apiClient.post(endpoint, formData, { + responseType: "blob", + }); + return { + ok: true, + files: [ + { + blob: response.data, + filename: filenameOf(response.headers, files[0].name), + }, + ], + }; + } catch (error) { + return { ok: false, message: messageOf(error) }; + } +} + +/** + * The name the server gave the output, falling back to the input's: a caller adopting an + * unnamed blob would put a file called "blob" in the user's workbench. + */ +function filenameOf(headers: unknown, fallback: string): string { + const disposition = (headers as Record | undefined)?.[ + "content-disposition" + ]; + if (typeof disposition !== "string") return fallback; + + // filename* (RFC 5987, percent-encoded) wins over plain filename, which is how a + // server sends a non-ASCII name. + const encoded = /filename\*=(?:UTF-8'')?([^;]+)/i.exec(disposition)?.[1]; + const plain = /filename="?([^";]+)"?/i.exec(disposition)?.[1]; + const name = encoded ?? plain; + if (!name) return fallback; + + try { + return decodeURIComponent(name.trim().replace(/^"|"$/g, "")) || fallback; + } catch { + // A malformed escape is not worth failing an otherwise successful retry over. + return name.trim().replace(/^"|"$/g, "") || fallback; + } +} + +function isUsableId(fileId: string | null | undefined): fileId is string { + return typeof fileId === "string" && fileId.trim() !== ""; +} + +/** + * The tool's parameters as form fields, alongside the documents under `fileInput`. + * `objectToFormData` is not reused: it is typed to the generated request union and throws on + * anything non-primitive, whereas a stashed payload is an opaque record read out of storage. + */ +function toFormData(params: Record, files: File[]): FormData { + const formData = new FormData(); + + for (const [key, value] of Object.entries(params)) { + if (value === undefined || value === null) continue; + if (Array.isArray(value)) { + for (const item of value) formData.append(key, asField(item)); + } else { + formData.append(key, asField(value)); + } + } + + for (const file of files) formData.append("fileInput", file); + + return formData; +} + +function asField(value: unknown): string { + return typeof value === "object" ? JSON.stringify(value) : `${value}`; +} + +/** + * How deep the walk below goes before it stops descending. Tool parameters are shallow, so this is + * far above anything real; it exists so a pathological or cyclic object cannot exhaust the stack. + */ +const MAX_PARAM_DEPTH = 20; + +/** Stands in for a subtree too deep to walk. Never the value itself: see below. */ +const TOO_DEEP = "[nested too deeply to store]"; + +/** + * Every secret-looking field dropped, at any depth: a tool can nest its parameters, and a + * password one level down is still a password. + * + * Past {@link MAX_PARAM_DEPTH} the subtree is replaced rather than returned. Returning it would + * mean anything below the limit is persisted unexamined, so the one place this function must not + * fail open is exactly the place it stops looking. + */ +function withoutSecrets( + value: Record, +): Record; +function withoutSecrets(value: unknown): unknown; +function withoutSecrets(value: unknown): unknown { + return prunedBelow(value, 0); +} + +/** + * The walk itself. Separate from {@link withoutSecrets} because the depth is bookkeeping between + * one level and the next, and no caller should be able to start part-way down. + */ +function prunedBelow(value: unknown, depth: number): unknown { + if (depth >= MAX_PARAM_DEPTH) return TOO_DEEP; + if (Array.isArray(value)) + return value.map((item) => prunedBelow(item, depth + 1)); + if (value === null || typeof value !== "object") return value; + + const kept: Record = {}; + for (const [key, nested] of Object.entries(value)) { + if (SECRET_FIELD.test(key)) continue; + kept[key] = prunedBelow(nested, depth + 1); + } + return kept; +} + +/** What the user saw. Never carries the password: it is not interpolated here. */ +function messageOf(error: unknown): string { + const response = (error as { response?: { data?: unknown } })?.response?.data; + if (typeof response === "string" && response.trim() !== "") return response; + + const message = (error as { message?: unknown })?.message; + return typeof message === "string" && message.trim() !== "" + ? message + : "Retrying the operation failed."; +} + +async function writeRecords(records: StoredRetryRecord[]): Promise { + const db = await indexedDBManager.openDatabase(RETRY_DB_CONFIG); + + return new Promise((resolve, reject) => { + const transaction = db.transaction([STORE_NAME], "readwrite"); + const store = transaction.objectStore(STORE_NAME); + transaction.oncomplete = () => resolve(); + transaction.onerror = () => reject(transaction.error); + transaction.onabort = () => + reject(transaction.error ?? new Error("Retry stash transaction aborted")); + + // put, not add: last write wins per fileId, matching the server's dedup. + for (const record of records) store.put(record); + + // Evict in the same transaction as the writes, so two concurrent stashes cannot + // both decide the store is under the cap. + const all = store.getAll(); + all.onsuccess = () => { + const stored = (all.result ?? []) as StoredRetryRecord[]; + const excess = stored.length - MAX_RETAINED_PAYLOADS; + if (excess <= 0) return; + stored + .sort((a, b) => a.recordedAt - b.recordedAt) + .slice(0, excess) + .forEach((record) => store.delete(record.fileId)); + }; + all.onerror = () => reject(all.error); + }); +} + +async function readRecord( + fileId: string, +): Promise { + const db = await indexedDBManager.openDatabase(RETRY_DB_CONFIG); + + return new Promise((resolve, reject) => { + const transaction = db.transaction([STORE_NAME], "readonly"); + const request = transaction.objectStore(STORE_NAME).get(fileId); + request.onsuccess = () => + resolve(request.result as StoredRetryRecord | undefined); + request.onerror = () => reject(request.error); + }); +} diff --git a/frontend/editor/src/core/services/notifications.ts b/frontend/editor/src/core/services/notifications.ts index 9749ffdcd8..f5c17fabcb 100644 --- a/frontend/editor/src/core/services/notifications.ts +++ b/frontend/editor/src/core/services/notifications.ts @@ -22,6 +22,12 @@ export type NotificationOrigin = "TOOL" | "POLICY" | "PIPELINE"; */ export type NotificationOwnership = "MINE" | "THEIRS" | "UNOWNED"; +/** + * How much of the row an action has earned. The server ranks by what the action does, not by where it + * ends up on screen; `promoteActions` turns a slot into a button or a menu entry. + */ +export type NotificationActionSlot = "RESOLUTION" | "SECONDARY" | "OVERFLOW"; + /** * One action as offered for one notification, all of them run by this client on its own device. * `id` is a plain string rather than a union because the server may know actions this build does @@ -32,6 +38,7 @@ export interface NotificationActionOffer { labelKey: string; /** English fallback, for a build with no copy for `labelKey`. */ defaultLabel: string; + slot: NotificationActionSlot; /** False means it cannot work for this row. The bell renders no button and states the reason * instead; the portal's queue still shows it disabled. */ enabled: boolean; @@ -91,3 +98,24 @@ export async function fetchNotifications( return []; } } + +/** + * Tell the server that the client fixed what a notification was about, so the bell stops reporting a + * failure the user has already dealt with. Takes the prefixed id, so the bell never hands a raw row id + * to a failure endpoint. + * + * Never throws. A refusal (already dismissed, not the caller's row) is not worth interrupting the user + * over: their document is already fixed and in front of them, and the next read tidies up the row. + */ +export async function reportNotificationResolved( + notificationId: string, +): Promise { + try { + await apiClient.post( + `${NOTIFICATIONS_PATH}/${encodeURIComponent(notificationId)}/resolved`, + ); + return true; + } catch { + return false; + } +} diff --git a/frontend/editor/src/proprietary/components/notifications/notificationActions.test.tsx b/frontend/editor/src/proprietary/components/notifications/notificationActions.test.tsx index ecc021a770..e81df4ccd5 100644 --- a/frontend/editor/src/proprietary/components/notifications/notificationActions.test.tsx +++ b/frontend/editor/src/proprietary/components/notifications/notificationActions.test.tsx @@ -14,6 +14,29 @@ import type { NotificationActionContext } from "@core/components/notifications/n * are the interesting cases: selecting the document directly, or handing it over. */ +const retryWithPassword = vi.fn(); +const unlockLocalDocument = vi.fn(); +vi.mock("@app/services/notificationRetry", () => ({ + retryWithPassword: (...args: unknown[]) => retryWithPassword(...args), + unlockLocalDocument: (...args: unknown[]) => unlockLocalDocument(...args), +})); + +const rerunPolicy = vi.fn(); +const rerunPolicyOnDocument = vi.fn(); +vi.mock("@app/services/notificationPolicyRetry", () => ({ + rerunPolicy: (...args: unknown[]) => rerunPolicy(...args), + rerunPolicyOnDocument: (...args: unknown[]) => rerunPolicyOnDocument(...args), +})); + +const reportNotificationResolved = vi.fn(); +vi.mock("@app/services/notifications", async () => ({ + ...(await vi.importActual( + "@app/services/notifications", + )), + reportNotificationResolved: (...args: unknown[]) => + reportNotificationResolved(...args), +})); + const navigate = vi.fn(); vi.mock("react-router-dom", async () => ({ ...(await vi.importActual( @@ -57,6 +80,8 @@ 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[] = []; +const setSelectedFiles = vi.fn(); +const addFiles = vi.fn(); function notification( overrides: Partial = {}, @@ -89,6 +114,7 @@ function offer(id: string): NotificationActionOffer { id, labelKey: `portal.failures.action.${id.toLowerCase()}`, defaultLabel: id, + slot: "SECONDARY", enabled: true, disabledReasonKey: null, }; @@ -100,6 +126,32 @@ function context( return { notification: notification(), hasLocalFile: true, + retryPayload: { + operation: "removePassword", + endpoint: "/api/v1/security/remove-password", + params: {}, + fileIds: ["f-1"], + recordedAt: 0, + }, + ...overrides, + }; +} + +/** + * A failure of an attended policy run: the editor started it on a document it was holding, so the + * row names the policy and that document, and no stash was ever written for it. + */ +function policyContext( + overrides: Partial = {}, +): NotificationActionContext { + return { + notification: notification({ + origin: "POLICY", + policyId: "pol-1", + sourceId: null, + }), + hasLocalFile: true, + retryPayload: null, ...overrides, }; } @@ -109,7 +161,7 @@ const inEditor = ({ children }: { children: ReactNode }) => ( @@ -143,6 +195,16 @@ function registry(wrapper = inEditor) { return renderHook(() => useNotificationActions(), { wrapper }).result.current; } +/** A file's own bytes. Via FileReader because this environment's Blob has no `text`. */ +function bytesOf(file: File): Promise { + return new Promise((resolve, reject) => { + const reader = new FileReader(); + reader.onload = () => resolve(String(reader.result)); + reader.onerror = () => reject(reader.error); + reader.readAsText(file); + }); +} + beforeEach(() => { navigate.mockReset(); addStirlingFileStubs.mockReset().mockResolvedValue([]); @@ -150,15 +212,68 @@ beforeEach(() => { setWorkbench.mockReset(); h.getStirlingFileStub.mockReset().mockResolvedValue(h.stub); openFileIds = []; + setSelectedFiles.mockReset(); + // The workspace's own id for the adopted document, which is what a policy re-run's output belongs + // to - not the reference the failure was filed against. + addFiles.mockReset().mockResolvedValue([{ fileId: "f-unlocked" }]); + reportNotificationResolved.mockReset().mockResolvedValue(true); + retryWithPassword.mockReset().mockResolvedValue({ ok: true, files: [] }); + // The unlock succeeds by default and produces a document, since almost every case below is about + // what happens to it afterwards. + unlockLocalDocument.mockReset().mockResolvedValue({ + ok: true, + files: [ + { + blob: new Blob(["pdf"], { type: "application/pdf" }), + filename: "invoice.pdf", + }, + ], + }); + // Tracked by default: the run is in the store, so something is polling it and its output will + // arrive. An untracked run is a separate case below, since it changes what the row may claim. + rerunPolicy.mockReset().mockResolvedValue({ ok: true, tracked: true }); + rerunPolicyOnDocument.mockReset().mockResolvedValue({ + ok: true, + tracked: true, + }); window.sessionStorage.clear(); window.history.pushState({}, "", "/"); }); describe("useNotificationActions", () => { - it("offers to open the document only while it is still in this browser", () => { + it("opens the failed tool with the document selected", () => { + registry().RETRY?.run(context()); + + expect(setSelectedFiles).toHaveBeenCalledWith(["f-1"]); + // The tool the stashed operation names, so the user sees the settings before it runs again. + expect(window.location.pathname).toBe("/remove-password"); + }); + + it("opens the editor itself when the stashed operation names no tool this build has", () => { + registry().RETRY?.run( + context({ + retryPayload: { + operation: "quarantine", + endpoint: "/api/v1/quarantine", + params: {}, + fileIds: ["f-1"], + recordedAt: 0, + }, + }), + ); + + expect(window.location.pathname).toBe("/"); + }); + + it("offers no retry once the document has left this browser", () => { const actions = registry(); - expect(actions.VIEW_FILE?.available(context())).toBe(true); + expect(actions.RETRY?.available(context({ hasLocalFile: false }))).toBe( + false, + ); + expect(actions.RETRY?.available(context({ retryPayload: null }))).toBe( + false, + ); expect(actions.VIEW_FILE?.available(context({ hasLocalFile: false }))).toBe( false, ); @@ -241,6 +356,119 @@ describe("useNotificationActions", () => { ).toBeNull(); }); + it("unlocks with the password it was given and reports what came back", async () => { + retryWithPassword.mockResolvedValue({ ok: false, message: "Wrong" }); + + const outcome = await registry().DECRYPT_AND_RETRY?.run( + context(), + "hunter2", + ); + + expect(retryWithPassword).toHaveBeenCalledWith( + expect.objectContaining({ endpoint: "/api/v1/security/remove-password" }), + "hunter2", + ); + expect(outcome).toEqual({ ok: false, message: "Wrong" }); + }); + + it("takes the unlocked document into the workbench through FileContext", async () => { + // The whole point of the password: the user must end up holding the unlocked file. + retryWithPassword.mockResolvedValue({ + ok: true, + files: [ + { + blob: new Blob(["pdf"], { type: "application/pdf" }), + filename: "invoice.pdf", + }, + ], + }); + + const outcome = await registry().DECRYPT_AND_RETRY?.run( + context(), + "hunter2", + ); + + expect(outcome).toEqual({ ok: true }); + const [files, options] = addFiles.mock.calls[0]; + expect((files as File[]).map((file) => file.name)).toEqual(["invoice.pdf"]); + // Selected as well as added, so it is the document on screen when the panel closes; and marked + // in-app so `usePolicyAutoRun` does not enforce the upload chain on it by itself. + expect(options).toEqual({ selectFiles: true, derivedFromTool: true }); + // And the incident is closed, with the prefixed id: nothing else tells the server the client + // fixed it, so the bell would otherwise keep reporting a failure the user has dealt with. + expect(reportNotificationResolved).toHaveBeenCalledWith("failure:evt-1"); + }); + + it("closes the incident only once the document is safely in", async () => { + // Reported first, then a failed adoption, would leave the row closed with nothing to show. + retryWithPassword.mockResolvedValue({ + ok: true, + files: [{ blob: new Blob(["pdf"]), filename: "invoice.pdf" }], + }); + addFiles.mockRejectedValue(new Error("quota")); + + await registry().DECRYPT_AND_RETRY?.run(context(), "hunter2"); + + expect(reportNotificationResolved).not.toHaveBeenCalled(); + }); + + it("keeps the unlock a success when the server will not record it", async () => { + // A reviewer dismissed the row first, or it was never this caller's. The document is already in + // the workbench, so a refused resolve must not present as a failed unlock. + retryWithPassword.mockResolvedValue({ + ok: true, + files: [{ blob: new Blob(["pdf"]), filename: "invoice.pdf" }], + }); + reportNotificationResolved.mockResolvedValue(false); + + expect( + await registry().DECRYPT_AND_RETRY?.run(context(), "hunter2"), + ).toEqual({ ok: true }); + }); + + it("reports a failure when the unlocked document cannot be taken in", async () => { + // Unlocked but dropped is the one outcome that leaves the user with nothing, so it is never + // reported as success. + retryWithPassword.mockResolvedValue({ + ok: true, + files: [{ blob: new Blob(["pdf"]), filename: "invoice.pdf" }], + }); + addFiles.mockRejectedValue(new Error("quota")); + + const outcome = await registry().DECRYPT_AND_RETRY?.run( + context(), + "hunter2", + ); + + expect(outcome).toEqual({ + ok: false, + message: + "The document was unlocked but could not be opened here. Try the tool directly.", + }); + }); + + it("offers no unlock where there is nowhere to put the result", async () => { + // The processor shell has no FileContext, so unlocking would produce a document with nowhere to + // go. The row promotes its next offer instead. + const actions = registry(inProcessor); + + expect(actions.DECRYPT_AND_RETRY?.available(context())).toBe(false); + expect(actions.VIEW_IN_PROCESSOR?.available(context())).toBe(true); + // And it refuses rather than posting a password whose output would be discarded. + expect(await actions.DECRYPT_AND_RETRY?.run(context(), "hunter2")).toEqual({ + ok: false, + message: "This document can no longer be retried from this browser.", + }); + expect(retryWithPassword).not.toHaveBeenCalled(); + }); + + it("offers the unlock where the editor can take the result", () => { + expect(registry().DECRYPT_AND_RETRY?.available(context())).toBe(true); + expect( + registry().DECRYPT_AND_RETRY?.available(context({ hasLocalFile: false })), + ).toBe(false); + }); + it("says it cannot hand the document over rather than navigating to nothing", async () => { // Storage refused, so nothing would be selected on arrival: the row reports it and stays put. // On the prototype: jsdom's storage object is a proxy, so an own-property spy does not take. @@ -262,6 +490,19 @@ describe("useNotificationActions", () => { setItem.mockRestore(); }); + it("says so rather than posting nothing when the stash has gone", async () => { + const outcome = await registry().DECRYPT_AND_RETRY?.run( + context({ retryPayload: null }), + "hunter2", + ); + + expect(retryWithPassword).not.toHaveBeenCalled(); + expect(outcome).toEqual({ + ok: false, + message: "This document can no longer be retried from this browser.", + }); + }); + it("links to the recorded failures section of the processor", () => { registry().VIEW_IN_PROCESSOR?.run(context()); @@ -273,8 +514,303 @@ describe("useNotificationActions", () => { // to gate here. expect( registry(inProcessor).VIEW_IN_PROCESSOR?.available( - context({ hasLocalFile: false }), + context({ hasLocalFile: false, retryPayload: null }), ), ).toBe(true); }); }); + +/** + * The other retry shape. An attended policy run stashes nothing, so everything these actions need + * comes off the row itself: which policy failed, and which document this browser was holding. + */ +describe("retrying an attended policy run", () => { + it("runs the policy again on the document it already holds", async () => { + const outcome = await registry().RETRY?.run(policyContext()); + + expect(rerunPolicy).toHaveBeenCalledWith({ + policyId: "pol-1", + fileId: "f-1", + }); + expect(outcome).toEqual({ ok: true }); + // Nothing was stashed for this row, so nothing may be read from one either. + expect(retryWithPassword).not.toHaveBeenCalled(); + }); + + it("re-runs the policy rather than reopening a tool, even where a stash happens to exist", async () => { + // The same document can have failed a tool run earlier, leaving a stash keyed on it. The row + // is about the policy, so that is what runs again. + await registry().RETRY?.run( + policyContext({ + retryPayload: { + operation: "removePassword", + endpoint: "/api/v1/security/remove-password", + params: {}, + fileIds: ["f-1"], + recordedAt: 0, + }, + }), + ); + + expect(rerunPolicy).toHaveBeenCalled(); + expect(window.location.pathname).toBe("/"); + }); + + it("says the server refused rather than looking like it worked", async () => { + rerunPolicy.mockResolvedValue({ + ok: false, + reason: "rejected", + message: "That policy is no longer enabled.", + }); + + expect(await registry().RETRY?.run(policyContext())).toEqual({ + ok: false, + message: "That policy is no longer enabled.", + }); + }); + + it("has its own wording when the server refuses without any", async () => { + rerunPolicy.mockResolvedValue({ + ok: false, + reason: "rejected", + message: null, + }); + + expect(await registry().RETRY?.run(policyContext())).toEqual({ + ok: false, + message: + "The policy could not be run again just now. Try again in a moment.", + }); + }); + + it("reports the document is gone rather than blaming the policy", async () => { + rerunPolicy.mockResolvedValue({ ok: false, reason: "missingFile" }); + + expect(await registry().RETRY?.run(policyContext())).toEqual({ + ok: false, + message: + "This document is not on this device, so it cannot be opened or retried here.", + }); + }); + + it("is offered for an attended row whose document is here, and for nothing else", () => { + const actions = registry(); + + expect(actions.RETRY?.available(policyContext())).toBe(true); + expect(actions.DECRYPT_AND_RETRY?.available(policyContext())).toBe(true); + + // Unattended: the fileId is a source's hash of a path that was never on any device, so there + // is nothing here to re-submit. The server disables the owner's actions on these anyway. + const unattended = policyContext({ + notification: notification({ + origin: "POLICY", + policyId: "pol-1", + sourceId: "src-1", + }), + }); + expect(actions.RETRY?.available(unattended)).toBe(false); + expect(actions.DECRYPT_AND_RETRY?.available(unattended)).toBe(false); + + // No policy named, and no stash either: nothing describes what would run again. + expect( + actions.RETRY?.available( + policyContext({ + notification: notification({ origin: "POLICY", policyId: null }), + }), + ), + ).toBe(false); + + // Document gone from this browser. + expect( + actions.RETRY?.available(policyContext({ hasLocalFile: false })), + ).toBe(false); + }); + + it("is offered nowhere without an editor to collect the result", () => { + // The processor shell mounts the bell outside the app's providers, so a run fired from there + // would have no workspace to land its output in. + const actions = registry(inProcessor); + + expect(actions.RETRY?.available(policyContext())).toBe(false); + expect(actions.DECRYPT_AND_RETRY?.available(policyContext())).toBe(false); + }); + + it("refuses rather than firing a run the processor shell could not collect", async () => { + expect(await registry(inProcessor).RETRY?.run(policyContext())).toEqual({ + ok: false, + message: "This document can no longer be retried from this browser.", + }); + expect(rerunPolicy).not.toHaveBeenCalled(); + }); + + it("unlocks, takes the document in, runs the policy again, then closes the incident", async () => { + const order: string[] = []; + addFiles.mockImplementation(async () => { + order.push("adopt"); + return [{ fileId: "f-unlocked" }]; + }); + rerunPolicyOnDocument.mockImplementation(async () => { + order.push("rerun"); + return { ok: true, tracked: true }; + }); + reportNotificationResolved.mockImplementation(async () => { + order.push("resolve"); + return true; + }); + + const outcome = await registry().DECRYPT_AND_RETRY?.run( + policyContext(), + "hunter2", + ); + + expect(outcome).toEqual({ ok: true }); + // The unlock is the remove-password call on the document the row names, not a stashed endpoint. + expect(unlockLocalDocument).toHaveBeenCalledWith("f-1", "hunter2"); + // Added and selected, so the unlocked document is what is on screen once the panel closes. The + // encrypted original is left alone: the user never asked to lose it. + const [files, options] = addFiles.mock.calls[0]; + expect((files as File[]).map((file) => file.name)).toEqual(["invoice.pdf"]); + // derivedFromTool is what stops the adoption starting a SECOND run of this same policy: the + // dispatch effect in usePolicyAutoRun treats a plain upload as work to enforce. A policy run is + // a billed automation run, so a double dispatch double-charges and can open a second incident. + expect(options).toEqual({ selectFiles: true, derivedFromTool: true }); + // Re-submitted under the ORIGINAL reference, so a second failure folds onto this same incident + // instead of opening a new one about the same document - while the run's output is attributed to + // the ADOPTED document, which is the one now in front of the user. + expect(rerunPolicyOnDocument).toHaveBeenCalledWith( + { policyId: "pol-1", fileId: "f-1" }, + expect.any(File), + "f-unlocked", + ); + // And with the prefixed notification id, never a raw failure id. + expect(reportNotificationResolved).toHaveBeenCalledWith("failure:evt-1"); + expect(order).toEqual(["adopt", "rerun", "resolve"]); + }); + + it("starts exactly one run for one click", async () => { + await registry().DECRYPT_AND_RETRY?.run(policyContext(), "hunter2"); + + // One submission, from here. The other possible source is the adoption, which is silenced by + // derivedFromTool above; see the gate's own test in usePolicyAutoRun.chain.test.tsx. + expect(rerunPolicyOnDocument).toHaveBeenCalledTimes(1); + expect(rerunPolicy).not.toHaveBeenCalled(); + expect(addFiles.mock.calls[0][1]).toMatchObject({ derivedFromTool: true }); + }); + + it("still runs when the adoption reports no workspace id, rather than guessing one", async () => { + // Nothing to attribute the output to, so the run goes untracked rather than being filed against + // the encrypted original, which would version the wrong document. + addFiles.mockResolvedValue([]); + rerunPolicyOnDocument.mockResolvedValue({ ok: true, tracked: false }); + + await registry().DECRYPT_AND_RETRY?.run(policyContext(), "hunter2"); + + expect(rerunPolicyOnDocument).toHaveBeenCalledWith( + { policyId: "pol-1", fileId: "f-1" }, + expect.any(File), + null, + ); + }); + + it("leaves the row open when the re-run cannot deliver, and says why", async () => { + // The run went, but untracked: nothing polls it, so the processed document never reaches the + // workbench. The unlocked INPUT is in, which is not what the user was after, so this may not + // present as success. Closing the row here would retire a failure that produced nothing and + // still billed a run. + rerunPolicyOnDocument.mockResolvedValue({ ok: true, tracked: false }); + + expect( + await registry().DECRYPT_AND_RETRY?.run(policyContext(), "hunter2"), + ).toEqual({ + ok: false, + message: + "The document was unlocked and the policy re-run started, but its result cannot be delivered here, so this failure stays open.", + }); + // Adopted regardless: the password bought them the unlocked document either way. + expect(addFiles).toHaveBeenCalled(); + expect(reportNotificationResolved).not.toHaveBeenCalled(); + }); + + it("says an untracked plain re-run cannot be delivered either", async () => { + // Same hole without a password in it: the local cache could not place the policy, so the run is + // unpolled and its output is not coming. The reader is told rather than shown a silent success. + rerunPolicy.mockResolvedValue({ ok: true, tracked: false }); + + expect(await registry().RETRY?.run(policyContext())).toEqual({ + ok: false, + message: + "The policy re-run started, but its result cannot be delivered here, so this failure stays open.", + }); + expect(reportNotificationResolved).not.toHaveBeenCalled(); + }); + + it("shows a wrong password for what it is, and touches nothing else", async () => { + unlockLocalDocument.mockResolvedValue({ + ok: false, + message: "The password is incorrect.", + }); + + expect( + await registry().DECRYPT_AND_RETRY?.run(policyContext(), "wrong"), + ).toEqual({ ok: false, message: "The password is incorrect." }); + expect(addFiles).not.toHaveBeenCalled(); + expect(rerunPolicyOnDocument).not.toHaveBeenCalled(); + // The row is still a failure, so nothing may report it fixed. + expect(reportNotificationResolved).not.toHaveBeenCalled(); + }); + + it("neither re-runs nor closes the incident when the document cannot be taken in", async () => { + addFiles.mockRejectedValue(new Error("quota")); + + expect( + await registry().DECRYPT_AND_RETRY?.run(policyContext(), "hunter2"), + ).toEqual({ + ok: false, + message: + "The document was unlocked but could not be opened here. Try the tool directly.", + }); + expect(rerunPolicyOnDocument).not.toHaveBeenCalled(); + expect(reportNotificationResolved).not.toHaveBeenCalled(); + }); + + it("says the unlock worked but the re-run did not, and leaves the row open", async () => { + rerunPolicyOnDocument.mockResolvedValue({ + ok: false, + reason: "rejected", + message: "Queue full.", + }); + + expect( + await registry().DECRYPT_AND_RETRY?.run(policyContext(), "hunter2"), + ).toEqual({ + ok: false, + message: + "The document was unlocked and opened here, but the policy could not be run on it again.", + }); + // Adopted anyway: the password bought them the unlocked document, and that is theirs to keep. + expect(addFiles).toHaveBeenCalled(); + // But nothing is fixed server-side, so the incident stays open. + expect(reportNotificationResolved).not.toHaveBeenCalled(); + }); + + it("never hands the password to anything but the unlock", async () => { + await registry().DECRYPT_AND_RETRY?.run(policyContext(), "hunter2"); + + // Everything downstream of the unlock: the adoption, the re-run, the resolve. The password is + // an argument to one call and goes out of scope after it - it is in no payload, no stash and no + // id, so nothing here can persist it. + const downstream = [ + ...addFiles.mock.calls, + ...rerunPolicyOnDocument.mock.calls, + ...reportNotificationResolved.mock.calls, + ]; + expect(JSON.stringify(downstream)).not.toContain("hunter2"); + // Not in the file that goes back to the policy either: those are the server's unlocked bytes. + const [, document] = rerunPolicyOnDocument.mock.calls[0] as [ + unknown, + File, + unknown, + ]; + expect(await bytesOf(document)).not.toContain("hunter2"); + }); +}); diff --git a/frontend/editor/src/proprietary/components/notifications/notificationActions.ts b/frontend/editor/src/proprietary/components/notifications/notificationActions.ts index a34d88c882..97921d294f 100644 --- a/frontend/editor/src/proprietary/components/notifications/notificationActions.ts +++ b/frontend/editor/src/proprietary/components/notifications/notificationActions.ts @@ -8,12 +8,28 @@ import { } from "@app/contexts/file/contexts"; import { NavigationActionsContext } from "@app/contexts/NavigationContext"; import { ViewerContext } from "@app/contexts/ViewerContext"; +import { getToolUrlPath } from "@app/data/toolsTaxonomy"; import { PORTAL_BASENAME, PORTAL_FAILURES_ANCHOR, } from "@app/routes/portalBasename"; import { EDITOR_BASENAME } from "@app/routes/editorBasename"; import { fileStorage } from "@app/services/fileStorage"; +import { + retryWithPassword, + unlockLocalDocument, + type RetryOutputFile, + type RetryPayload, +} from "@app/services/notificationRetry"; +import { + rerunPolicy, + rerunPolicyOnDocument, + type PolicyRerunOutcome, + type PolicyRetryTarget, +} from "@app/services/notificationPolicyRetry"; +import { reportNotificationResolved } from "@app/services/notifications"; +import { isValidToolId } from "@app/types/toolId"; +import type { FileContextActions } from "@app/types/fileContext"; import type { FileId } from "@app/types/file"; import { type ClientActionOutcome, @@ -30,8 +46,9 @@ export { }; /** - * What this build can do about a failure notification: open the document it is about, or go to the - * recorded failures in the processor. + * What this build can do about a failure notification: unlock the document and take the result, run the + * failing work again, open the tool that failed, or go to the incident in the processor. "Run it again" + * means two different things, so see {@link RetryTarget}. * * THE SHELL PROBLEM. The portal mounts as a sibling of the route that renders `AppProviders` (see * `proprietary/App.tsx`), so in the processor shell there is no FileContext, ToolWorkflowContext or @@ -85,6 +102,82 @@ function goToEditor(path: string): void { window.dispatchEvent(new PopStateEvent("popstate")); } +/** + * The tool whose run failed, when the stashed operation names one this build still has. The stashed + * `params` stay in the stash: a tool's parameters live in component state inside `useBaseParameters`, + * which has no seam for initial values, and this is the place that would hand them over once it does. + */ +function toolPathOf(payload: RetryPayload): string { + return isValidToolId(payload.operation) + ? getToolUrlPath(payload.operation) + : "/"; +} + +/** + * What a retry would re-run. A union because the two are genuinely different: a tool retry is an + * endpoint plus parameters, which exist only in the client that submitted them, and a policy retry is a + * stored policy plus a document, which the server named on the notification itself. + */ +type RetryTarget = + | { readonly kind: "tool"; readonly payload: RetryPayload } + /** The policy and the document, exactly as the re-run takes them. */ + | { readonly kind: "policy"; readonly policy: PolicyRetryTarget }; + +/** + * Which of the two a notification describes, or null when nothing here can re-run it. + * + * The policy shape wins where it applies, being the more specific claim: the row says which policy + * failed on which document, whereas a stash only says which operation this browser last saw fail on it. + * + * The attended check repeats `isResolvableHere` in `useNotifications` so this function holds on its own + * arguments rather than by arrangement with the caller. + */ +function retryTargetOf(context: NotificationActionContext): RetryTarget | null { + const { notification, hasLocalFile, retryPayload } = context; + if (!hasLocalFile) return null; + + const attended = (notification.sourceId ?? null) === null; + if (attended && notification.policyId && notification.fileId) { + return { + kind: "policy", + policy: { policyId: notification.policyId, fileId: notification.fileId }, + }; + } + + return retryPayload ? { kind: "tool", payload: retryPayload } : null; +} + +/** The documents a password-carrying call produced, as files the workbench can take. */ +function asFiles(outputs: RetryOutputFile[]): File[] { + return outputs.map( + (output) => + new File([output.blob], output.filename, { + type: output.blob.type || "application/pdf", + }), + ); +} + +/** + * Take what the retry produced into the workbench, so the unlocked document is what the user is looking + * at once the panel closes. Added and selected rather than replacing the encrypted original: the unlock + * is a new document, and deleting their input is not this button's business. + * + * `derivedFromTool` is load-bearing. A plain upload is what `usePolicyAutoRun`'s dispatch effect watches + * for, so without it this adoption would fire the whole upload policy chain by itself: BILLED automation + * runs nobody asked for, on a document that is only here because a retry produced it. It is also simply + * true, since the document came out of the remove-password tool. + */ +async function adopt( + actions: FileContextActions, + files: File[], +): Promise { + const adopted = await actions.addFiles(files, { + selectFiles: true, + derivedFromTool: true, + }); + return adopted.map((file) => file.fileId); +} + export function useNotificationActions(): ClientActionRegistry { const { t } = useTranslation(); const navigate = useNavigate(); @@ -166,6 +259,195 @@ export function useNotificationActions(): ClientActionRegistry { goToEditor(EDITOR_BASENAME); }; + /** + * Open the editor on a specific tool with the document selected, from either shell. Unlike + * {@link openDocument} this always goes through the URL, because the editor reads its tool out + * of the URL: selecting alone would leave the workbench on whatever view it was on. + */ + const openToolWithDocument = ( + fileId: string | null, + path: string, + ): ClientActionOutcome | void => { + if (fileId) { + if (fileContext) { + fileContext.actions.setSelectedFiles([fileId as FileId]); + } else if (!stashSelection(fileId)) { + // Nothing would be selected on arrival, so say so here rather than navigate to a page + // that looks like it worked. + 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(path); + }; + + const unavailable = (): ClientActionOutcome => ({ + ok: false, + message: t( + "notifications.retryUnavailable", + "This document can no longer be retried from this browser.", + ), + }); + + /** + * What a policy re-run amounted to, in the reader's terms. A rejection after the unlock reads + * differently from one before it, so the reader is not left thinking their password was wrong. + * + * An untracked run is reported as a failure on purpose. It did go, but nothing here will collect + * what it produces, and the processed document was the point of the retry: presenting that as + * success would close the row on a result that is never arriving. + */ + const rerunOutcome = ( + outcome: PolicyRerunOutcome, + adopted: boolean, + ): ClientActionOutcome => { + if (outcome.ok && outcome.tracked) return { ok: true }; + if (outcome.ok) { + return { + ok: false, + message: adopted + ? t( + "notifications.unlockedRerunUndelivered", + "The document was unlocked and the policy re-run started, but its result cannot be delivered here, so this failure stays open.", + ) + : t( + "notifications.rerunUndelivered", + "The policy re-run started, but its result cannot be delivered here, so this failure stays open.", + ), + }; + } + if (outcome.reason === "missingFile") { + return { + ok: false, + message: t( + "notifications.notOnThisDevice", + "This document is not on this device, so it cannot be opened or retried here.", + ), + }; + } + if (adopted) { + return { + ok: false, + message: t( + "notifications.unlockedNotRerun", + "The document was unlocked and opened here, but the policy could not be run on it again.", + ), + }; + } + return { + ok: false, + message: + outcome.message ?? + t( + "notifications.rerunRejected", + "The policy could not be run again just now. Try again in a moment.", + ), + }; + }; + + /** + * Whether this device can re-run what the row describes. A policy re-run also needs the editor's + * providers above the bell, not to submit the run but because a run fired from the processor shell + * has no mounted workspace to collect its output. + */ + const canRetry = (context: NotificationActionContext): boolean => { + const target = retryTargetOf(context); + if (!target) return false; + return target.kind === "tool" || fileContext !== undefined; + }; + + const retry: ClientActionSpec = { + available: canRetry, + closesPanel: true, + run: async (context): Promise => { + const target = retryTargetOf(context); + if (!target) return unavailable(); + + // A tool opens with the document selected rather than re-running from here: it failed once, so + // the user gets to see the settings first. A stored policy has none to show, so it simply goes. + if (target.kind === "tool") { + return openToolWithDocument( + context.notification.fileId, + toolPathOf(target.payload), + ); + } + if (!fileContext) return unavailable(); + return rerunOutcome(await rerunPolicy(target.policy), false); + }, + }; + + const decryptAndRetry: ClientActionSpec = { + // Only where there is somewhere to put the result: in the processor shell an unlocked document + // would have nowhere to go, so the row promotes its next offer instead. + available: (context) => fileContext !== undefined && canRetry(context), + needsPassword: true, + // On success the adopted document is the destination, and it is behind the panel. + closesPanel: true, + run: async (context, password): Promise => { + const target = retryTargetOf(context); + if (!target || !password || !fileContext) return unavailable(); + + // The stash for a tool, because only it knows what failed and with which parameters; the unlock + // endpoint for a policy, since a locked input is fixed the same way whatever was reading it. + const outcome = + target.kind === "tool" + ? await retryWithPassword(target.payload, password) + : await unlockLocalDocument(target.policy.fileId, password); + // A wrong password lands here, carrying the server's own words, which the row shows. + if (!outcome.ok) return outcome; + + // It unlocked, so the user must end up holding it. A failed adoption fails the whole action: + // claiming success and dropping the result leaves them nothing for the password they typed. + const unlocked = asFiles(outcome.files ?? []); + let adopted: FileId[] = []; + try { + adopted = await adopt(fileContext.actions, unlocked); + } catch { + return { + ok: false, + message: t( + "notifications.adoptFailed", + "The document was unlocked but could not be opened here. Try the tool directly.", + ), + }; + } + + // Back through the run that choked on the locked document, under the ORIGINAL reference, so a + // second failure folds onto this same incident. The adopted id goes too, since that is the + // document the output belongs to now. After the adoption, so a refused re-run still leaves the + // user holding what their password bought them. + if (target.kind === "policy") { + const document = unlocked[0]; + const rerun: PolicyRerunOutcome = document + ? await rerunPolicyOnDocument( + target.policy, + document, + adopted[0] ?? null, + ) + : { ok: false, reason: "missingFile" }; + // Anything short of a tracked run stops here, untracked included. The unlocked document + // being in the workbench is not the result the user asked for: they wanted what the policy + // makes of it, and that output has nowhere to land. Closing the row on the input alone + // would retire a failure that is still costing them a billed run and still producing + // nothing. One mapper decides, so the message and the resolve cannot disagree. + const result = rerunOutcome(rerun, true); + if (!result.ok) return result; + } + + // Nothing else tells the server the retry worked, so without this the bell keeps reporting a + // failure the user has fixed. Reached only once the whole retry has landed: the document is in, + // and the re-run is being polled by something that will deliver it. Its result is ignored, + // since a refused resolve is not a failed unlock. + await reportNotificationResolved(context.notification.id); + return { ok: true }; + }, + }; + const viewFile: ClientActionSpec = { available: (context) => context.hasLocalFile, closesPanel: true, @@ -183,8 +465,10 @@ export function useNotificationActions(): ClientActionRegistry { }; return { + RETRY: retry, + DECRYPT_AND_RETRY: decryptAndRetry, VIEW_FILE: viewFile, VIEW_IN_PROCESSOR: viewInProcessor, }; - }, [canOpenHere, openInWorkbench, navigate, t]); + }, [canOpenHere, openInWorkbench, fileContext, navigate, t]); } diff --git a/frontend/editor/src/proprietary/services/notificationPolicyRetry.test.ts b/frontend/editor/src/proprietary/services/notificationPolicyRetry.test.ts new file mode 100644 index 0000000000..5cfca7201a --- /dev/null +++ b/frontend/editor/src/proprietary/services/notificationPolicyRetry.test.ts @@ -0,0 +1,214 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +/** + * Re-running the policy a notification says failed. The whole point is that nothing was stashed for + * it: the policy and the document both come off the row, and the bytes come out of storage under the + * same reference the failure was filed against. + */ + +const getStirlingFile = vi.fn(); +vi.mock("@app/services/fileStorage", () => ({ + fileStorage: { + getStirlingFile: (...args: unknown[]) => getStirlingFile(...args), + }, +})); + +const runStoredPolicy = vi.fn(); +vi.mock("@app/services/policyApi", () => ({ + runStoredPolicy: (...args: unknown[]) => runStoredPolicy(...args), + resolvePolicyRunTarget: () => "saas", +})); + +/** The local policy cache, which is how a backend policy id becomes a category without any hook. */ +const policies = vi.hoisted(() => ({ + value: { security: { backendId: "pol-1" } } as Record< + string, + { backendId?: string } + >, +})); +vi.mock("@app/services/policyStorage", () => ({ + loadPolicies: () => policies.value, +})); + +// The REAL run store, because the point of registering is that the auto-run controller finds the run +// there and polls it. A mock would assert the call and prove nothing about the record. +const { getRun, isDispatched, resetPolicyRuns } = + await import("@app/components/policies/policyRunStore"); +const { rerunPolicy, rerunPolicyOnDocument } = + await import("@app/services/notificationPolicyRetry"); + +const target = { policyId: "pol-1", fileId: "f-1" }; + +beforeEach(() => { + getStirlingFile.mockReset().mockResolvedValue(null); + runStoredPolicy.mockReset().mockResolvedValue("run-1"); + policies.value = { security: { backendId: "pol-1" } }; + localStorage.clear(); + resetPolicyRuns(); +}); + +describe("rerunPolicy", () => { + it("submits the stored document under the reference the failure named", async () => { + const document = new File(["%PDF-1.7"], "invoice.pdf", { + type: "application/pdf", + }); + getStirlingFile.mockResolvedValue(document); + + await expect(rerunPolicy(target)).resolves.toEqual({ + ok: true, + tracked: true, + }); + // The original reference, not a new one: the server folds a repeat failure onto the same + // incident rather than opening a second row about the same document. + expect(runStoredPolicy).toHaveBeenCalledWith("pol-1", [document], "f-1"); + }); + + it("records the run so the editor polls it and delivers its output", async () => { + // Without this the retry is invisible: nothing polls the run, no output reaches the workspace, + // and the row sits open until the run fails again. usePolicyAutoRun drives all of that off the + // store, so being in the store IS the progress. + const document = new File(["%PDF-1.7"], "invoice.pdf"); + getStirlingFile.mockResolvedValue(document); + + await rerunPolicy(target); + + expect(getRun("run-1")).toMatchObject({ + runId: "run-1", + // The category the backend policy belongs to, which is what the import step needs to honour + // the policy's output mode and what the chain continues from. + categoryId: "security", + // The document that failed is still the document in the workspace, so the output belongs to it. + fileId: "f-1", + fileName: "invoice.pdf", + status: "PENDING", + target: "saas", + }); + // Marked dispatched as any other run is, so the pair is not treated as never having run. + expect(isDispatched("security", "f-1")).toBe(true); + }); + + it("still runs a policy the local cache cannot place, and says the run is untracked", async () => { + // A policy deleted since, or a cache this browser never built. The run is left to go, since the + // server-side effect is real and the submission has already happened, but a run with no category + // cannot be imported or chained, so nothing will ever deliver its output here. The caller is told + // as much rather than being handed a bare success it would close the failure on. + policies.value = {}; + getStirlingFile.mockResolvedValue(new File(["%PDF-1.7"], "invoice.pdf")); + + await expect(rerunPolicy(target)).resolves.toEqual({ + ok: true, + tracked: false, + }); + expect(runStoredPolicy).toHaveBeenCalled(); + expect(getRun("run-1")).toBeUndefined(); + }); + + it("records nothing when the run was refused, so no phantom sits in the feed", async () => { + getStirlingFile.mockResolvedValue(new File(["%PDF-1.7"], "invoice.pdf")); + runStoredPolicy.mockRejectedValue(new Error("refused")); + + await rerunPolicy(target); + + expect(getRun("run-1")).toBeUndefined(); + }); + + it("reports the document is gone rather than submitting nothing", async () => { + getStirlingFile.mockResolvedValue(null); + + await expect(rerunPolicy(target)).resolves.toEqual({ + ok: false, + reason: "missingFile", + }); + expect(runStoredPolicy).not.toHaveBeenCalled(); + }); + + it("treats a browser that will not answer for the file as not having it", async () => { + // Same outcome for the reader either way, and an exception here is not theirs to see. + getStirlingFile.mockRejectedValue(new Error("storage unavailable")); + + await expect(rerunPolicy(target)).resolves.toEqual({ + ok: false, + reason: "missingFile", + }); + }); +}); + +describe("rerunPolicyOnDocument", () => { + const unlocked = new File(["%PDF-1.7"], "invoice.pdf"); + + it("submits bytes the caller already holds, still under the original reference", async () => { + await expect( + rerunPolicyOnDocument(target, unlocked, "f-unlocked"), + ).resolves.toEqual({ + ok: true, + tracked: true, + }); + expect(runStoredPolicy).toHaveBeenCalledWith("pol-1", [unlocked], "f-1"); + // Nothing was read from storage: the unlocked document is not there and never will be. + expect(getStirlingFile).not.toHaveBeenCalled(); + }); + + it("attributes the run to the adopted document, not the one the failure named", async () => { + // Two references, deliberately: the server gets the failure's, so a repeat folds onto the same + // incident; the run store gets the adopted one, so the output versions the unlocked document the + // user is now looking at rather than the encrypted original they still have. + await rerunPolicyOnDocument(target, unlocked, "f-unlocked"); + + expect(runStoredPolicy).toHaveBeenCalledWith("pol-1", [unlocked], "f-1"); + expect(getRun("run-1")).toMatchObject({ fileId: "f-unlocked" }); + expect(isDispatched("security", "f-unlocked")).toBe(true); + }); + + it("runs untracked rather than filing the output against the wrong document, and admits it", async () => { + // No workspace id came back from the adoption. Recording it against the failure's reference + // would version the encrypted original, which is not what the run produced. So it goes + // unrecorded, and `tracked` carries that outward: the caller needs it to keep the failure open, + // since an unpolled run delivers nothing however well the submission went. + await expect( + rerunPolicyOnDocument(target, unlocked, null), + ).resolves.toEqual({ ok: true, tracked: false }); + + expect(runStoredPolicy).toHaveBeenCalled(); + expect(getRun("run-1")).toBeUndefined(); + }); + + it("carries the server's own words when it refuses", async () => { + runStoredPolicy.mockRejectedValue({ + response: { data: "That policy is no longer enabled." }, + }); + + await expect( + rerunPolicyOnDocument(target, unlocked, "f-unlocked"), + ).resolves.toEqual({ + ok: false, + reason: "rejected", + message: "That policy is no longer enabled.", + }); + }); + + it("reads the message out of a structured error body too", async () => { + runStoredPolicy.mockRejectedValue({ + response: { data: { message: "Job queue is full." } }, + }); + + await expect( + rerunPolicyOnDocument(target, unlocked, "f-unlocked"), + ).resolves.toEqual({ + ok: false, + reason: "rejected", + message: "Job queue is full.", + }); + }); + + it("says nothing rather than something unreadable, leaving the wording to the caller", async () => { + runStoredPolicy.mockRejectedValue(new Error("Network Error")); + + await expect( + rerunPolicyOnDocument(target, unlocked, "f-unlocked"), + ).resolves.toEqual({ + ok: false, + reason: "rejected", + message: null, + }); + }); +}); diff --git a/frontend/editor/src/proprietary/services/notificationPolicyRetry.ts b/frontend/editor/src/proprietary/services/notificationPolicyRetry.ts new file mode 100644 index 0000000000..7f242cbe87 --- /dev/null +++ b/frontend/editor/src/proprietary/services/notificationPolicyRetry.ts @@ -0,0 +1,153 @@ +import { recordRunStart } from "@app/components/policies/policyRunStore"; +import { fileStorage } from "@app/services/fileStorage"; +import { loadPolicies } from "@app/services/policyStorage"; +import { + resolvePolicyRunTarget, + runStoredPolicy, +} from "@app/services/policyApi"; +import type { FileId } from "@app/types/file"; + +/** + * Re-running the stored policy that a notification says failed. + * + * Nothing is stashed for this: the row names the policy and the workspace's own reference to the + * document, and the bytes are in this browser's storage under that same reference, so the whole retry + * is derivable from the row. A tool retry cannot be, which is why `notificationRetry` has a stash. + * + * Not a reuse of the auto-run controller, which is a hook. This makes the same single call the + * auto-run makes and hands it to the same run store, so from there it is like any other run. + */ + +/** The document, and the policy to put it back through. Both read straight off the notification. */ +export interface PolicyRetryTarget { + policyId: string; + /** + * The workspace reference the failing run was filed against. Sent back unchanged so the server folds + * a repeat failure onto the same incident. + */ + fileId: string; +} + +/** + * What became of a re-run, so the caller can say it in the reader's own language. The wording belongs to + * the component layer, which has `t`. + * + * `ok` alone is not enough to act on. A run that went but could not be recorded has no one polling it, so + * its output never reaches this workspace: nothing about the failure is demonstrably fixed, however + * cleanly the submission itself went. `tracked` is that difference, and the caller must not close a row + * on the strength of `ok` without it. + */ +export type PolicyRerunOutcome = + /** In the store, so `usePolicyAutoRun` polls it to terminal and imports what it produced. */ + | { ok: true; tracked: true } + /** Running on the server, with nothing here to collect it. See {@link submit}. */ + | { ok: true; tracked: false } + | { ok: false; reason: "missingFile" } + /** The server refused the run. `message` is its own, or null when it gave nothing usable. */ + | { ok: false; reason: "rejected"; message: string | null }; + +/** Re-run on the document still in this browser's storage, under the reference the failure named. */ +export async function rerunPolicy( + target: PolicyRetryTarget, +): Promise { + let document: File | null = null; + try { + document = await fileStorage.getStirlingFile(target.fileId as FileId); + } catch { + // Treated as absent: a browser that will not answer for the file cannot supply its bytes either. + document = null; + } + if (!document) return { ok: false, reason: "missingFile" }; + + // The document that failed is still the one in the workspace, so the output belongs to it. + return submit(target, document, target.fileId); +} + +/** + * Re-run on bytes the caller already holds: the just-unlocked document, which is not in storage under + * the failing run's reference and never will be. + * + * @param workspaceFileId the workspace file this run's output belongs to, which is the ADOPTED document + * rather than the one the failure named. Two references on purpose: the server gets the failure's, + * so a repeat folds onto the same incident, and the run store gets this one, so the output versions + * the document now in front of the user. Null when the adoption produced no id, in which case the + * run still goes untracked rather than being filed against the wrong document. + */ +export async function rerunPolicyOnDocument( + target: PolicyRetryTarget, + document: File, + workspaceFileId: string | null, +): Promise { + return submit(target, document, workspaceFileId); +} + +/** + * Fire the run, then record it where every other run is recorded. The recording is what makes the retry + * visible: `usePolicyAutoRun` polls every run in the store to completion and imports its outputs, and + * that follows from the run being in the store with a real category, hence the lookup below. + * + * Two things can stop the recording without stopping the run: a local cache that cannot place the policy, + * and an adoption that produced no workspace id. Neither is worth refusing the retry over, since the + * server-side effect is real, but neither is a delivered result either. Both are reported as untracked so + * the caller can say so and leave the failure open rather than closing a row whose output is not coming. + */ +async function submit( + target: PolicyRetryTarget, + document: File, + workspaceFileId: string | null, +): Promise { + // Resolved before the run, so a lookup that throws cannot leave a live run unrecorded. + const categoryId = categoryForPolicy(target.policyId); + const runTarget = resolvePolicyRunTarget(); + + let runId: string; + try { + runId = await runStoredPolicy(target.policyId, [document], target.fileId); + } catch (error) { + return { ok: false, reason: "rejected", message: rejectionMessage(error) }; + } + + // Nothing to file it under, or nothing to file it against. The run itself already went, so it is left + // to run: refusing it now would only add a wasted submission to an undeliverable one. + if (!categoryId || !workspaceFileId) return { ok: true, tracked: false }; + + // Marks (category, file) dispatched as it records, same as any other run: the pair has already run + // once, and this is that run again rather than a new one to dispatch later. + recordRunStart({ + runId, + categoryId, + fileId: workspaceFileId, + fileName: document.name, + fileSize: document.size, + target: runTarget, + status: "PENDING", + outputs: [], + error: null, + startedAt: Date.now(), + }); + return { ok: true, tracked: true }; +} + +/** + * The category whose configured policy this is. The non-hook read rather than `usePolicies`, which needs + * the app-config and team contexts the bell's shell may not have. Same cache the auto-run's reconcile + * writes, so the same answer. + */ +function categoryForPolicy(policyId: string): string | undefined { + try { + return Object.entries(loadPolicies()).find( + ([, state]) => state.backendId === policyId, + )?.[0]; + } catch { + return undefined; + } +} + +/** What the server said, when it said anything readable. Nothing is interpolated here. */ +function rejectionMessage(error: unknown): string | null { + const data = (error as { response?: { data?: unknown } })?.response?.data; + if (typeof data === "string" && data.trim() !== "") return data; + + const message = (data as { message?: unknown } | undefined)?.message; + return typeof message === "string" && message.trim() !== "" ? message : null; +}