diff --git a/frontend/src/app/common/service/workflow-persist/workflow-persist.service.spec.ts b/frontend/src/app/common/service/workflow-persist/workflow-persist.service.spec.ts index 1ae30d331cf..a35c9ec106e 100644 --- a/frontend/src/app/common/service/workflow-persist/workflow-persist.service.spec.ts +++ b/frontend/src/app/common/service/workflow-persist/workflow-persist.service.spec.ts @@ -221,6 +221,45 @@ describe("WorkflowPersistService", () => { expect(result?.isPublished).toBe(1); }); + it("sends saves one at a time, in order, each caller getting its own result", () => { + // Two saves in flight at once can land out of order and the older content would win; the + // autosave and a Save or a view switch are independent callers, so the ordering lives here. + const wf = (name: string) => ({ wid: 9, name, description: "", content: validContent }) as unknown as Workflow; + const seen: string[] = []; + service.persistWorkflow(wf("first")).subscribe(w => seen.push("first:" + w.name)); + service.persistWorkflow(wf("second")).subscribe(w => seen.push("second:" + w.name)); + + // Only the first request has gone out; the second waits for it. + const first = httpTestingController.expectOne(`${API}/${WORKFLOW_PERSIST_URL}`); + expect(first.request.body.name).toBe("first"); + expect(httpTestingController.match(`${API}/${WORKFLOW_PERSIST_URL}`)).toHaveLength(0); + + first.flush({ wid: 9, name: "first", content: "{}" }); + const second = httpTestingController.expectOne(`${API}/${WORKFLOW_PERSIST_URL}`); + expect(second.request.body.name).toBe("second"); + second.flush({ wid: 9, name: "second", content: "{}" }); + + expect(seen).toEqual(["first:first", "second:second"]); + }); + + it("fails only its own caller when a save fails, and still sends the next", () => { + const wf = (name: string) => ({ wid: 9, name, description: "", content: validContent }) as unknown as Workflow; + let firstError: unknown; + let secondName: string | undefined; + service.persistWorkflow(wf("first")).subscribe({ error: (e: unknown) => (firstError = e) }); + service.persistWorkflow(wf("second")).subscribe(w => (secondName = w.name)); + + httpTestingController + .expectOne(`${API}/${WORKFLOW_PERSIST_URL}`) + .flush("boom", { status: 500, statusText: "Server Error" }); + expect(firstError).toBeDefined(); + + httpTestingController + .expectOne(`${API}/${WORKFLOW_PERSIST_URL}`) + .flush({ wid: 9, name: "second", content: "{}" }); + expect(secondName).toBe("second"); + }); + it("persistWorkflow notifies the user when the workflow is broken but still POSTs", () => { const errorSpy = vi.spyOn(notificationService, "error").mockImplementation(() => {}); const workflow = { @@ -274,6 +313,24 @@ describe("WorkflowPersistService", () => { expect(result).toEqual(created); }); + it("createWorkflow sends the default view when given one, and omits it otherwise", () => { + const content = jsonCast(testContent); + + service.createWorkflow(content, "form default", DefaultView.FORM).subscribe(); + const withView = httpTestingController.expectOne(`${API}/${WORKFLOW_CREATE_URL}`); + expect(withView.request.body).toEqual({ + name: "form default", + content: JSON.stringify(content), + defaultView: DefaultView.FORM, + }); + withView.flush({ workflow: { wid: 1 } } as unknown as DashboardWorkflow); + + service.createWorkflow(content, "no view").subscribe(); + const withoutView = httpTestingController.expectOne(`${API}/${WORKFLOW_CREATE_URL}`); + expect(withoutView.request.body).toEqual({ name: "no view", content: JSON.stringify(content) }); + withoutView.flush({ workflow: { wid: 2 } } as unknown as DashboardWorkflow); + }); + it("createWorkflow filters out a null response so no value is emitted", () => { let emitted = false; service.createWorkflow(jsonCast(testContent)).subscribe(() => (emitted = true)); diff --git a/frontend/src/app/common/service/workflow-persist/workflow-persist.service.ts b/frontend/src/app/common/service/workflow-persist/workflow-persist.service.ts index 9b8f4741bd1..66d267a673d 100644 --- a/frontend/src/app/common/service/workflow-persist/workflow-persist.service.ts +++ b/frontend/src/app/common/service/workflow-persist/workflow-persist.service.ts @@ -19,8 +19,8 @@ import { HttpClient, HttpParams } from "@angular/common/http"; import { Injectable } from "@angular/core"; -import { Observable, throwError } from "rxjs"; -import { catchError, filter, map } from "rxjs/operators"; +import { EMPTY, Observable, ReplaySubject, Subject, throwError } from "rxjs"; +import { catchError, concatMap, filter, map, tap } from "rxjs/operators"; import { AppSettings } from "../../app-setting"; import { Workflow, WorkflowContent } from "../../type/workflow"; import { DashboardWorkflow } from "../../../dashboard/type/dashboard-workflow.interface"; @@ -59,13 +59,42 @@ export class WorkflowPersistService { // flag to disable workflow persist when displaying the read only particular version private workflowPersistFlag = true; + /** + * Saves, one at a time and in call order. Two saves in flight at once can reach the backend out + * of order, and then the older content wins: the canvas's autosave (debounced) and a Save or a + * view switch are independent requests, and the Form View's own queue only orders that page's + * saves. Ordering them here, at the one place every save goes through, covers all of them and + * lets a caller that hands over on completion (the view switches) know that everything asked for + * before it has landed too. Each request snapshots its payload when asked for; it is sent when its + * turn comes, and its outcome is relayed to that caller alone. A failed save fails its own caller + * and does not hold up the next. + */ + private readonly persistQueue = new Subject<{ send: Observable; result: Subject }>(); + constructor( private http: HttpClient, private notificationService: NotificationService - ) {} + ) { + this.persistQueue + .pipe( + concatMap(({ send, result }) => + send.pipe( + tap({ + next: updated => result.next(updated), + error: (err: unknown) => result.error(err), + complete: () => result.complete(), + }), + catchError(() => EMPTY) + ) + ) + ) + .subscribe(); + } /** - * persists a workflow to backend database and returns its updated information (e.g., new wid) + * persists a workflow to backend database and returns its updated information (e.g., new wid). + * The request is queued behind any save still in flight (see persistQueue); the returned + * observable completes once this save has come back. * @param workflow */ public persistWorkflow(workflow: Workflow): Observable { @@ -79,7 +108,7 @@ export class WorkflowPersistService { // backend does not read it on this endpoint (publishing goes through /public and /private), // and it is not reliably known here anyway, since the metadata fed back after a save names // it differently (see WorkflowUtilService.parseWorkflowInfo). - return this.http + const send = this.http .post(`${AppSettings.getApiEndpoint()}/${WORKFLOW_PERSIST_URL}`, { wid: workflow.wid, name: workflow.name, @@ -90,6 +119,11 @@ export class WorkflowPersistService { filter((updatedWorkflow: Workflow) => updatedWorkflow != null), map(WorkflowUtilService.parseWorkflowInfo) ); + // Replayed, so a caller that subscribes after the queue has already relayed the outcome (a + // save that was quick, or a synchronous test double) still receives it. + const result = new ReplaySubject(1); + this.persistQueue.next({ send, result }); + return result.asObservable(); } /** @@ -99,12 +133,16 @@ export class WorkflowPersistService { */ public createWorkflow( newWorkflowContent: WorkflowContent, - newWorkflowName: string = DEFAULT_WORKFLOW_NAME + newWorkflowName: string = DEFAULT_WORKFLOW_NAME, + defaultView?: DefaultView ): Observable { return this.http .post(`${AppSettings.getApiEndpoint()}/${WORKFLOW_CREATE_URL}`, { name: newWorkflowName, content: JSON.stringify(newWorkflowContent), + // Bound onto the workflow row on the server, so an uploaded form-default workflow + // still opens as a form. Omitted (server default CANVAS) when the file carries none. + ...(defaultView === undefined ? {} : { defaultView }), }) .pipe(filter((createdWorkflow: DashboardWorkflow) => createdWorkflow != null)); } diff --git a/frontend/src/app/common/type/workflow.ts b/frontend/src/app/common/type/workflow.ts index 4129346b338..701086e2a71 100644 --- a/frontend/src/app/common/type/workflow.ts +++ b/frontend/src/app/common/type/workflow.ts @@ -17,7 +17,7 @@ * under the License. */ -import { WorkflowMetadata } from "../../dashboard/type/workflow-metadata.interface"; +import { DefaultView, WorkflowMetadata } from "../../dashboard/type/workflow-metadata.interface"; import { CommentBox, OperatorLink, OperatorPredicate, Point } from "../../workspace/types/workflow-common.interface"; export enum ExecutionMode { @@ -104,3 +104,17 @@ export interface WorkflowContent }> {} export type Workflow = { content: WorkflowContent } & WorkflowMetadata; + +/** + * The JSON a workflow is exported as, from the dashboard download and the canvas menu alike: the + * content plus, when the workflow has one, the landing view as one extra top-level key next to the + * content's own (operators/links/...). The importer (upload) destructures it back out onto the + * workflow row, so a download-then-upload keeps a form-default workflow opening as a form; an + * older importer that reads the whole object as content simply ignores the unknown key, and an + * older export without it imports unchanged. + */ +export type ExportedWorkflow = WorkflowContent & { defaultView?: DefaultView }; + +export function exportedWorkflow(content: WorkflowContent, defaultView: DefaultView | undefined): ExportedWorkflow { + return defaultView === undefined ? content : { ...content, defaultView }; +} diff --git a/frontend/src/app/dashboard/component/user/list-item/card-item/card-item.component.html b/frontend/src/app/dashboard/component/user/list-item/card-item/card-item.component.html index 4cd50dc8dea..2c072e7830b 100644 --- a/frontend/src/app/dashboard/component/user/list-item/card-item/card-item.component.html +++ b/frontend/src/app/dashboard/component/user/list-item/card-item/card-item.component.html @@ -36,7 +36,7 @@ [(ngModel)]="entry.checked" (ngModelChange)="onCheckboxChange(entry)"> - +
+ + + + +
+ +
+ + + +
+ diff --git a/frontend/src/app/workspace/component/menu/menu.component.scss b/frontend/src/app/workspace/component/menu/menu.component.scss index deb31c02586..12dafb6a2ec 100644 --- a/frontend/src/app/workspace/component/menu/menu.component.scss +++ b/frontend/src/app/workspace/component/menu/menu.component.scss @@ -81,6 +81,7 @@ #user-buttons, #execution-buttons, nz-button-group, +nz-upload, texera-computing-unit-selection { display: inline-flex; align-items: center; @@ -215,3 +216,64 @@ texera-coeditor-user-icon { width: auto; vertical-align: -0.2em; } + +/* One workflow, two ways of working on it. Rendered identically in the operator canvas + and the Form View, in the same slot of the same title row, so the control + never moves when the view does -- that stillness is what makes the two read as two + views of one thing rather than two pages. + + Deliberately quiet: the indicator is a rule sitting on the row's own bottom border, + not a filled button. This is secondary navigation and it shares a screen with Run, + which is the one thing here that should be solid blue. The current view is inert -- + clicking the view you are already in should do nothing. */ +.view-switch { + display: inline-flex; + align-self: stretch; + align-items: stretch; + flex: none; + gap: 20px; + margin-right: 20px; + + button { + appearance: none; + border: 0; + background: none; + cursor: pointer; + font: inherit; + font-size: 13px; + color: rgba(0, 0, 0, 0.45); + padding: 0; + position: relative; + display: inline-flex; + align-items: center; + white-space: nowrap; + transition: color 0.15s; + + /* Sits on the row's bottom rule, so the two read as tabs of the row rather than + as a widget dropped into it. */ + &::after { + content: ""; + position: absolute; + left: -2px; + right: -2px; + bottom: -1px; + height: 2px; + background: transparent; + transition: background 0.15s; + } + + &:hover:not(.on) { + color: rgba(0, 0, 0, 0.85); + } + } + + button.on { + color: rgba(0, 0, 0, 0.85); + font-weight: 500; + cursor: default; + + &::after { + background: #1890ff; + } + } +} diff --git a/frontend/src/app/workspace/component/menu/menu.component.spec.ts b/frontend/src/app/workspace/component/menu/menu.component.spec.ts index da69bf3adf0..3e15706cf0f 100644 --- a/frontend/src/app/workspace/component/menu/menu.component.spec.ts +++ b/frontend/src/app/workspace/component/menu/menu.component.spec.ts @@ -116,6 +116,118 @@ describe("MenuComponent", () => { expect(component).toBeTruthy(); }); + it("does not open the Form View for a workflow that has not been saved yet", () => { + vi.spyOn(component["workflowActionService"], "getWorkflowMetadata").mockReturnValue({ wid: undefined } as any); + const href = window.location.href; + + component.onClickOpenFormView(); + + expect(window.location.href).toBe(href); + }); + + it("hands over to the id the save assigned when the canvas held a workflow never saved yet", () => { + // After "new workflow" the canvas holds the default workflow (wid 0); the switch's save creates + // it, and the page to open is the created one, not /workflow/0/form. + component.writeAccess = true; + vi.spyOn(component["workflowActionService"], "getWorkflowMetadata").mockReturnValue({ wid: 0 } as any); + vi.spyOn(workflowPersistService, "persistWorkflow").mockReturnValue(of({ wid: 42, name: "created" } as any)); + vi.spyOn(component["workflowActionService"], "setWorkflowMetadata").mockImplementation(() => {}); + const navigate = vi.spyOn(component as any, "openFormViewPage").mockImplementation(() => {}); + + component.onClickOpenFormView(); + + expect(navigate).toHaveBeenCalledWith(42); + }); + + it("saves, then hands over to the Form View only once the save has completed", () => { + component.writeAccess = true; + vi.spyOn(component["workflowActionService"], "getWorkflowMetadata").mockReturnValue({ wid: 7 } as any); + const saved = { wid: 7, name: "saved" } as any; + const persistSpy = vi.spyOn(workflowPersistService, "persistWorkflow").mockReturnValue(of(saved)); + const metadataSpy = vi + .spyOn(component["workflowActionService"], "setWorkflowMetadata") + .mockImplementation(() => {}); + const navigate = vi.spyOn(component as any, "openFormViewPage").mockImplementation(() => {}); + + component.onClickOpenFormView(); + + // The navigation unloads the document and aborts anything still in flight, so it must wait for + // the save's completion rather than be fired right after the request. + expect(persistSpy).toHaveBeenCalled(); + expect(metadataSpy).toHaveBeenCalledWith(saved); + expect(navigate).toHaveBeenCalledWith(7); + expect(component.isSaving).toBe(false); + }); + + it("stays on the canvas and reports the error when the save before the switch fails", () => { + component.writeAccess = true; + vi.spyOn(component["workflowActionService"], "getWorkflowMetadata").mockReturnValue({ wid: 7 } as any); + vi.spyOn(workflowPersistService, "persistWorkflow").mockReturnValue(throwError(() => new Error("nope"))); + const errorSpy = vi.spyOn(notificationService, "error").mockImplementation(() => {}); + const navigate = vi.spyOn(component as any, "openFormViewPage").mockImplementation(() => {}); + + component.onClickOpenFormView(); + + // Leaving would take the user away from changes that were never stored. + expect(navigate).not.toHaveBeenCalled(); + expect(errorSpy).toHaveBeenCalledWith("Could not save. Your latest changes are not stored yet."); + expect(component.isSaving).toBe(false); + }); + + it("saves once more when an edit lands while the switch's save is out, then hands over", () => { + // The page stays editable until the full-page load; an edit made after the click is not in the + // save's snapshot and its own autosave (debounced) would be aborted by the load. workflowChanged + // marks it, and the hand-over saves again before leaving. + const edits = new Subject(); + vi.spyOn(component["workflowActionService"], "workflowChanged").mockReturnValue(edits.asObservable()); + component.ngOnInit(); + component.writeAccess = true; + vi.spyOn(component["workflowActionService"], "getWorkflowMetadata").mockReturnValue({ wid: 7 } as any); + vi.spyOn(component["workflowActionService"], "setWorkflowMetadata").mockImplementation(() => {}); + const first$ = new Subject(); + const second$ = new Subject(); + const persistSpy = vi + .spyOn(workflowPersistService, "persistWorkflow") + .mockReturnValueOnce(first$) + .mockReturnValueOnce(second$); + const navigate = vi.spyOn(component as any, "openFormViewPage").mockImplementation(() => {}); + + component.onClickOpenFormView(); + edits.next(undefined); // an edit while the first save is out + first$.complete(); + + expect(persistSpy).toHaveBeenCalledTimes(2); // saved once more + expect(navigate).not.toHaveBeenCalled(); + second$.complete(); + expect(navigate).toHaveBeenCalledWith(7); + expect(component.isSaving).toBe(false); + }); + + it("ignores a second click while the hand-over is already in progress", () => { + component.writeAccess = true; + vi.spyOn(component["workflowActionService"], "getWorkflowMetadata").mockReturnValue({ wid: 7 } as any); + const persistSpy = vi.spyOn(workflowPersistService, "persistWorkflow").mockReturnValue(new Subject()); + vi.spyOn(component as any, "openFormViewPage").mockImplementation(() => {}); + + component.onClickOpenFormView(); + component.onClickOpenFormView(); + + expect(persistSpy).toHaveBeenCalledTimes(1); + }); + + it("takes a reader straight over without a save, which they could not make", () => { + // Every save of a reader's is a 403 that would keep them on the canvas with an error. + component.writeAccess = false; + vi.spyOn(component["workflowActionService"], "getWorkflowMetadata").mockReturnValue({ wid: 7 } as any); + const persistSpy = vi.spyOn(workflowPersistService, "persistWorkflow"); + const navigate = vi.spyOn(component as any, "openFormViewPage").mockImplementation(() => {}); + + component.onClickOpenFormView(); + + expect(persistSpy).not.toHaveBeenCalled(); + expect(navigate).toHaveBeenCalledWith(7); + }); + describe("getRunButtonBehavior", () => { it("returns 'Invalid Workflow' when the workflow is invalid", () => { component.isWorkflowValid = false; @@ -542,6 +654,44 @@ describe("MenuComponent", () => { expect(blobArg).toBeInstanceOf(Blob); expect(blobArg.type).toBe("text/plain;charset=utf-8"); }); + + // Blob.text() is missing in jsdom, but FileReader.readAsText works. + const readBlob = (blob: Blob) => + new Promise((resolve, reject) => { + const reader = new FileReader(); + reader.onload = () => resolve(reader.result as string); + reader.onerror = () => reject(reader.error); + reader.readAsText(blob); + }); + + it("carries the workflow's default view next to the content, as the dashboard download does", async () => { + const saveAs = vi.spyOn(TestBed.inject(FileSaverService), "saveAs").mockImplementation(() => {}); + vi.spyOn(workflowActionService, "getWorkflowContent").mockReturnValue({ + operators: [], + links: [], + } as unknown as WorkflowContent); + vi.spyOn(workflowActionService, "getWorkflowMetadata").mockReturnValue({ wid: 7, defaultView: "FORM" } as any); + + component.onClickExportWorkflow(); + + const parsed = JSON.parse(await readBlob(saveAs.mock.calls[0][0] as Blob)); + expect(parsed.defaultView).toBe("FORM"); + expect(parsed.operators).toEqual([]); + }); + + it("leaves the key out when the workflow has no default view", async () => { + const saveAs = vi.spyOn(TestBed.inject(FileSaverService), "saveAs").mockImplementation(() => {}); + vi.spyOn(workflowActionService, "getWorkflowContent").mockReturnValue({ + operators: [], + links: [], + } as unknown as WorkflowContent); + vi.spyOn(workflowActionService, "getWorkflowMetadata").mockReturnValue({ wid: 7 } as any); + + component.onClickExportWorkflow(); + + const parsed = JSON.parse(await readBlob(saveAs.mock.calls[0][0] as Blob)); + expect("defaultView" in parsed).toBe(false); + }); }); describe("version history", () => { @@ -1168,6 +1318,38 @@ describe("MenuComponent", () => { vi.restoreAllMocks(); }); + describe("view switch", () => { + const flag = (formViewEnabled: boolean) => + (TestBed.inject(GuiConfigService) as unknown as MockGuiConfigService).setConfig({ formViewEnabled }); + + it("shows Canvas pressed and hands Form View to onClickOpenFormView, only with the flag on", () => { + flag(false); + fixture.detectChanges(); + expect(q(".view-switch")).toBeNull(); + + flag(true); + fixture.detectChanges(); + const open = vi.spyOn(component, "onClickOpenFormView").mockImplementation(() => {}); + const buttons = fixture.debugElement.queryAll(By.css(".view-switch button")); + expect(buttons.map(b => (b.nativeElement.textContent ?? "").trim())).toEqual(["Canvas", "Form View"]); + // The current view is the pressed segment: announced as such, and a live button on purpose. + expect(buttons[0].nativeElement.getAttribute("aria-pressed")).toBe("true"); + expect(buttons[0].nativeElement.classList.contains("on")).toBe(true); + expect(buttons[1].nativeElement.getAttribute("aria-pressed")).toBe("false"); + buttons[1].triggerEventHandler("click", null); + expect(open).toHaveBeenCalledTimes(1); + }); + + it("hides the switch while an older version is displayed", () => { + flag(true); + component.displayParticularWorkflowVersion = true; + fixture.detectChanges(); + + // A past version has no form to switch to. + expect(q(".view-switch")).toBeNull(); + }); + }); + describe("version display bar", () => { /** Puts the menu into the "viewing an older version" state and renders it. */ function showVersion(versionId: number | null = 7): void { diff --git a/frontend/src/app/workspace/component/menu/menu.component.ts b/frontend/src/app/workspace/component/menu/menu.component.ts index c3ad4aa979e..3a46e987f33 100644 --- a/frontend/src/app/workspace/component/menu/menu.component.ts +++ b/frontend/src/app/workspace/component/menu/menu.component.ts @@ -22,7 +22,7 @@ import { Component, ElementRef, Input, OnDestroy, OnInit, ViewChild } from "@ang import { Router, RouterLink } from "@angular/router"; import { UserService } from "../../../common/service/user/user.service"; import { WorkflowPersistService } from "../../../common/service/workflow-persist/workflow-persist.service"; -import { Workflow, WorkflowContent } from "../../../common/type/workflow"; +import { exportedWorkflow, Workflow, WorkflowContent } from "../../../common/type/workflow"; import { ExecuteWorkflowService } from "../../service/execute-workflow/execute-workflow.service"; import { UndoRedoService } from "../../service/undo-redo/undo-redo.service"; import { ValidationWorkflowService } from "../../service/validation/validation-workflow.service"; @@ -45,7 +45,7 @@ import { ResultExportationComponent } from "../result-exportation/result-exporta import { ReportGenerationService } from "../../service/report-generation/report-generation.service"; import { ShareAccessComponent } from "src/app/dashboard/component/user/share-access/share-access.component"; import { PanelService } from "../../service/panel/panel.service"; -import { USER_WORKFLOW } from "../../../app-routing.constant"; +import { USER_WORKFLOW, USER_WORKSPACE } from "../../../app-routing.constant"; import { ComputingUnitStatusService } from "../../../common/service/computing-unit/computing-unit-status/computing-unit-status.service"; import { ComputingUnitState } from "../../../common/type/computing-unit-connection.interface"; import { ComputingUnitSelectionComponent } from "../power-button/computing-unit-selection.component"; @@ -130,6 +130,10 @@ export class MenuComponent implements OnInit, OnDestroy { public isWorkflowValid: boolean = true; // this will check whether the workflow error or not public isWorkflowEmpty: boolean = false; public isSaving: boolean = false; + /** A Form View hand-over is in progress (saving, then a full-page load); a second click is a no-op. */ + private handingOverToFormView = false; + /** An edit has been reported since the hand-over's last save snapshot (see onClickOpenFormView). */ + private editedSinceSwitchSnapshot = false; public isWorkflowModifiable: boolean = false; public workflowId?: number; public isExportDeactivate: boolean = false; @@ -219,6 +223,13 @@ export class MenuComponent implements OnInit, OnDestroy { } public ngOnInit(): void { + // Marks an edit for the Form View hand-over (see onClickOpenFormView): set the moment an edit is + // reported, before the autosave debounce, cleared when the switch's save snapshots the workflow. + this.workflowActionService + .workflowChanged() + .pipe(untilDestroyed(this)) + .subscribe(() => (this.editedSinceSwitchSnapshot = true)); + this.executeWorkflowService .getExecutionStateStream() .pipe(untilDestroyed(this)) @@ -617,8 +628,13 @@ export class MenuComponent implements OnInit, OnDestroy { } public onClickExportWorkflow(): void { - const workflowContent: WorkflowContent = this.workflowActionService.getWorkflowContent(); - const workflowContentJson = JSON.stringify(workflowContent, null, 2); + // The same shape the dashboard download produces (see exportedWorkflow): the content plus the + // landing view as a sibling key, so a file exported here uploads as a form-default workflow too. + const exported = exportedWorkflow( + this.workflowActionService.getWorkflowContent(), + this.workflowActionService.getWorkflowMetadata().defaultView + ); + const workflowContentJson = JSON.stringify(exported, null, 2); const fileName = this.currentWorkflowName + ".json"; // Through the injectable wrapper (as the dashboard downloads already do), so a spec stubs it // with TestBed instead of module-mocking the CommonJS file-saver package, which the unit-test @@ -626,6 +642,83 @@ export class MenuComponent implements OnInit, OnDestroy { this.fileSaverService.saveAs(new Blob([workflowContentJson], { type: "text/plain;charset=utf-8" }), fileName); } + /** + * Open the Form View -- a full page load, not a route: the two views share root-level + * singletons (graph, Yjs shared model), and routing left the old collaboration client + * alive (you appeared as your own coeditor). A fresh document is the clean handover. + */ + public onClickOpenFormView(): void { + const wid = this.workflowActionService.getWorkflowMetadata().wid; + if (wid === undefined || this.handingOverToFormView) { + return; + } + // A reader has nothing to save, and every save of theirs is a guaranteed 403 that would keep + // them here with an error: straight over, as the form's own switch does for a reader. + if (!this.writeAccess) { + this.openFormViewPage(wid); + return; + } + // Save first, and hand over only once the save has completed. The full-page load that + // follows unloads this document, and a request still in flight at that moment is aborted, so + // navigating right after firing the save could lose the very edit the switch is meant to carry + // across; the workspace's beforeunload save runs into the same unload and is no safety net. A + // save that fails keeps the user here with the error shown, rather than leaving with changes + // that were never stored. The form's own switch (openRegularCanvas) does the same. + // + // Two more things the hand-over must not lose. An autosave already in flight when the switch + // is clicked: WorkflowPersistService sends saves one at a time and in order, so ours lands after + // it and completes after it. And an edit made while our save is out (the page stays editable + // until the load): workflowChanged marks it, and the drain below saves once more before handing + // over rather than letting the full-page load abort that edit's own debounced autosave. + this.handingOverToFormView = true; + this.isSaving = true; + this.saveThenOpenFormView(wid); + } + + private saveThenOpenFormView(wid: number): void { + // The snapshot below carries everything reported up to now. + this.editedSinceSwitchSnapshot = false; + // A workflow the canvas holds but has never saved carries the default id (0); the save creates + // it and answers with the id it was given, which is the one to open -- as the autosave, which + // moves the URL to the answered id, already does. + let target = wid; + this.workflowPersistService + .persistWorkflow(this.workflowActionService.getWorkflow()) + .pipe(untilDestroyed(this)) + .subscribe({ + next: (updatedWorkflow: Workflow) => { + target = updatedWorkflow.wid ?? wid; + this.workflowActionService.setWorkflowMetadata(updatedWorkflow); + }, + error: () => { + this.isSaving = false; + this.handingOverToFormView = false; + // The same wording as the form's own save, so the two switches read alike. + this.notificationService.error("Could not save. Your latest changes are not stored yet."); + }, + complete: () => { + if (this.editedSinceSwitchSnapshot) { + // An edit landed while the save was out; the full-page load would kill its autosave. + this.saveThenOpenFormView(target); + return; + } + this.isSaving = false; + this.openFormViewPage(target); + }, + }); + } + + /** + * The full-page handover to the Form View, apart from the save so the order is testable. + * Excluded from coverage as a whole: jsdom cannot navigate, so the specs stub this method and + * assert when it is called rather than what it does. + */ + /* v8 ignore start */ + private openFormViewPage(wid: number): void { + window.location.href = `${USER_WORKSPACE}/${wid}/form`; + } + /* v8 ignore stop */ + /** * Calls Markdown Description Component */ diff --git a/frontend/src/app/workspace/component/power-button/computing-unit-selection.component.html b/frontend/src/app/workspace/component/power-button/computing-unit-selection.component.html index 6a9a7d90611..066e39c836c 100644 --- a/frontend/src/app/workspace/component/power-button/computing-unit-selection.component.html +++ b/frontend/src/app/workspace/component/power-button/computing-unit-selection.component.html @@ -113,7 +113,7 @@ 'unit-connecting': unit.status === 'Pending', }" [nz-tooltip]="cannotSelectUnit(unit) ? getUnitStatusTooltip(unit) + '. Cannot select.' : ''" - (click)="selectedComputingUnit = unit; selectComputingUnit(this.workflowId, unit?.computingUnit?.cuid)"> + (click)="onPickComputingUnit(unit)">
{ ], }).compileComponents(); + // Selecting a unit now remembers it per workflow in localStorage, which survives + // between tests and would let one spec's selection steer another's auto-select. + Object.keys(localStorage) + .filter(key => key.startsWith("computing-unit-of-workflow-")) + .forEach(key => localStorage.removeItem(key)); + fixture = TestBed.createComponent(ComputingUnitSelectionComponent); component = fixture.componentInstance; fixture.detectChanges(); @@ -1357,6 +1363,191 @@ describe("PowerButtonComponent", () => { expect(selectSpy).not.toHaveBeenCalled(); }); + + it("drops a latest-execution answer that arrives after the workflow changed underneath it", () => { + const execService = TestBed.inject(WorkflowExecutionsService); + const late$ = new Subject(); + vi.spyOn(execService, "retrieveLatestWorkflowExecution").mockImplementation((wid: number) => + wid === 100 ? late$ : of({ cuId: 9 } as unknown as WorkflowExecutionsEntry) + ); + const { comp, emit } = bootWithMetaStream(); + const selectSpy = vi.spyOn(comp, "selectComputingUnit").mockImplementation(() => {}); + + emit(100); + emit(101); // decides at once from its own latest execution + late$.next({ cuId: 55 } as unknown as WorkflowExecutionsEntry); + + expect(selectSpy).toHaveBeenCalledWith(101, 9); + expect(selectSpy).not.toHaveBeenCalledWith(100, 55); + }); + + it("drops the running-unit fallback when the failed lookup was for a workflow no longer shown", () => { + const execService = TestBed.inject(WorkflowExecutionsService); + const late$ = new Subject(); + vi.spyOn(execService, "retrieveLatestWorkflowExecution").mockImplementation((wid: number) => + wid === 100 ? late$ : of({ cuId: 9 } as unknown as WorkflowExecutionsEntry) + ); + const { comp, emit } = bootWithMetaStream(); + comp.allComputingUnits = [makeComputingUnit({ cuid: 2, status: "Running" })]; + const selectSpy = vi.spyOn(comp, "selectComputingUnit").mockImplementation(() => {}); + + emit(100); + emit(101); + late$.error(new Error("no execution")); + + expect(selectSpy).not.toHaveBeenCalledWith(100, 2); + expect(selectSpy).toHaveBeenCalledTimes(1); // 101's own decision only + }); + + it("prefers the remembered unit for this workflow over the latest execution", () => { + localStorage.setItem("computing-unit-of-workflow-100", "77"); + const execService = TestBed.inject(WorkflowExecutionsService); + const latestSpy = vi + .spyOn(execService, "retrieveLatestWorkflowExecution") + .mockReturnValue(of({ cuId: 55 } as unknown as WorkflowExecutionsEntry)); + const { comp, emit } = bootWithMetaStream(); + vi.spyOn(TestBed.inject(ComputingUnitStatusService), "getAllComputingUnits").mockReturnValue( + of([makeComputingUnit({ cuid: 77, status: "Running" })]) + ); + const selectSpy = vi.spyOn(comp, "selectComputingUnit").mockImplementation(() => {}); + + emit(100); + + expect(selectSpy).toHaveBeenCalledWith(100, 77); + expect(latestSpy).not.toHaveBeenCalled(); + }); + + // A remembered unit that has since been terminated must not be chased: the status service + // would wait for it to appear forever and the fallbacks would never run. + it("forgets a remembered unit that no longer exists and uses the latest execution", () => { + localStorage.setItem("computing-unit-of-workflow-100", "77"); + const execService = TestBed.inject(WorkflowExecutionsService); + vi.spyOn(execService, "retrieveLatestWorkflowExecution").mockReturnValue( + of({ cuId: 55 } as unknown as WorkflowExecutionsEntry) + ); + const { comp, emit } = bootWithMetaStream(); + vi.spyOn(TestBed.inject(ComputingUnitStatusService), "getAllComputingUnits").mockReturnValue( + of([makeComputingUnit({ cuid: 55, status: "Running" })]) + ); + const selectSpy = vi.spyOn(comp, "selectComputingUnit").mockImplementation(() => {}); + + emit(100); + + expect(selectSpy).toHaveBeenCalledWith(100, 55); + expect(localStorage.getItem("computing-unit-of-workflow-100")).toBeNull(); + }); + + it("still falls back when forgetting the stale unit throws", () => { + localStorage.setItem("computing-unit-of-workflow-100", "77"); + const execService = TestBed.inject(WorkflowExecutionsService); + vi.spyOn(execService, "retrieveLatestWorkflowExecution").mockReturnValue( + of({ cuId: 55 } as unknown as WorkflowExecutionsEntry) + ); + const { comp, emit } = bootWithMetaStream(); + vi.spyOn(TestBed.inject(ComputingUnitStatusService), "getAllComputingUnits").mockReturnValue( + of([makeComputingUnit({ cuid: 55, status: "Running" })]) + ); + // Only the component's one removeItem call throws; the storage keeps working for the + // afterEach clean-up and the tests that follow. + const removeSpy = vi.spyOn(Storage.prototype, "removeItem").mockImplementationOnce(() => { + throw new Error("storage unavailable"); + }); + const selectSpy = vi.spyOn(comp, "selectComputingUnit").mockImplementation(() => {}); + + expect(() => emit(100)).not.toThrow(); + + expect(removeSpy).toHaveBeenCalledWith("computing-unit-of-workflow-100"); + expect(selectSpy).toHaveBeenCalledWith(100, 55); + removeSpy.mockRestore(); + }); + + // Deciding on an empty list would throw the choice away before the list has loaded, so the + // decision waits for the first non-empty list. + it("waits for the unit list before honouring a remembered unit", () => { + localStorage.setItem("computing-unit-of-workflow-100", "77"); + const execService = TestBed.inject(WorkflowExecutionsService); + const latestSpy = vi.spyOn(execService, "retrieveLatestWorkflowExecution"); + const { comp, emit } = bootWithMetaStream(); + const units$ = new Subject(); + vi.spyOn(TestBed.inject(ComputingUnitStatusService), "getAllComputingUnits").mockReturnValue(units$); + const selectSpy = vi.spyOn(comp, "selectComputingUnit").mockImplementation(() => {}); + + emit(100); + units$.next([]); + + expect(selectSpy).not.toHaveBeenCalled(); + expect(latestSpy).not.toHaveBeenCalled(); + + units$.next([makeComputingUnit({ cuid: 77, status: "Running" })]); + + expect(selectSpy).toHaveBeenCalledWith(100, 77); + }); + + it("drops a pending remembered decision once the workflow has changed underneath it", () => { + localStorage.setItem("computing-unit-of-workflow-100", "77"); + const execService = TestBed.inject(WorkflowExecutionsService); + vi.spyOn(execService, "retrieveLatestWorkflowExecution").mockReturnValue( + of({ cuId: 55 } as unknown as WorkflowExecutionsEntry) + ); + const { comp, emit } = bootWithMetaStream(); + const units$ = new Subject(); + vi.spyOn(TestBed.inject(ComputingUnitStatusService), "getAllComputingUnits").mockReturnValue(units$); + const selectSpy = vi.spyOn(comp, "selectComputingUnit").mockImplementation(() => {}); + + emit(100); + // Workflow 101 has nothing remembered, so it decides at once from its latest execution. + emit(101); + units$.next([makeComputingUnit({ cuid: 77, status: "Running" })]); + + expect(selectSpy).toHaveBeenCalledWith(101, 55); + expect(selectSpy).not.toHaveBeenCalledWith(100, 77); + }); + + it("does not carry a remembered unit across workflows", () => { + localStorage.setItem("computing-unit-of-workflow-100", "77"); + const execService = TestBed.inject(WorkflowExecutionsService); + vi.spyOn(execService, "retrieveLatestWorkflowExecution").mockReturnValue( + of({ cuId: 55 } as unknown as WorkflowExecutionsEntry) + ); + const { comp, emit } = bootWithMetaStream(); + comp.allComputingUnits = [makeComputingUnit({ cuid: 77, status: "Running" })]; + const selectSpy = vi.spyOn(comp, "selectComputingUnit").mockImplementation(() => {}); + + emit(101); + + expect(selectSpy).toHaveBeenCalledWith(101, 55); + }); + + // Number() is lenient enough to turn several kinds of junk into a "valid" cuid. + ["0", "-3", "1.5", "", " "].forEach(stored => { + it(`ignores a remembered value of ${JSON.stringify(stored)}`, () => { + localStorage.setItem("computing-unit-of-workflow-100", stored); + const execService = TestBed.inject(WorkflowExecutionsService); + vi.spyOn(execService, "retrieveLatestWorkflowExecution").mockReturnValue( + of({ cuId: 55 } as unknown as WorkflowExecutionsEntry) + ); + const { comp, emit } = bootWithMetaStream(); + const selectSpy = vi.spyOn(comp, "selectComputingUnit").mockImplementation(() => {}); + + emit(100); + + expect(selectSpy).toHaveBeenCalledWith(100, 55); + }); + }); + + it("ignores a corrupt remembered value", () => { + localStorage.setItem("computing-unit-of-workflow-100", "not-a-number"); + const execService = TestBed.inject(WorkflowExecutionsService); + vi.spyOn(execService, "retrieveLatestWorkflowExecution").mockReturnValue( + of({ cuId: 55 } as unknown as WorkflowExecutionsEntry) + ); + const { comp, emit } = bootWithMetaStream(); + const selectSpy = vi.spyOn(comp, "selectComputingUnit").mockImplementation(() => {}); + + emit(100); + + expect(selectSpy).toHaveBeenCalledWith(100, 55); + }); }); describe("selectComputingUnit guards", () => { @@ -2200,4 +2391,50 @@ describe("PowerButtonComponent", () => { expect(component.pveModalVisible).toBe(false); }); }); + + describe("remembering the selected unit per workflow", () => { + const unit77 = { computingUnit: { cuid: 77 } } as unknown as DashboardWorkflowComputingUnit; + + it("writes the user's own pick so the other view of the same workflow restores it", () => { + component.workflowId = 100; + const select = vi.spyOn(component, "selectComputingUnit"); + + component.onPickComputingUnit(unit77); + + expect(select).toHaveBeenCalledWith(100, 77); + expect(component.selectedComputingUnit).toBe(unit77); + expect(localStorage.getItem("computing-unit-of-workflow-100")).toBe("77"); + }); + + it("does not remember a unit selected on load, which is derived rather than chosen", () => { + // The remembered unit, the last execution's or a running one are picked FOR the user; storing + // them would let a derived unit outrank a fresher last execution on the next load. + component.selectComputingUnit(100, 77); + expect(localStorage.getItem("computing-unit-of-workflow-100")).toBeNull(); + }); + + it("does not record a pick the component refused to make", () => { + component.workflowId = DEFAULT_WORKFLOW.wid; + component.onPickComputingUnit(unit77); + expect(localStorage.getItem(`computing-unit-of-workflow-${DEFAULT_WORKFLOW.wid}`)).toBeNull(); + component.workflowId = 100; + component.onPickComputingUnit({ computingUnit: {} } as unknown as DashboardWorkflowComputingUnit); + expect(localStorage.getItem("computing-unit-of-workflow-100")).toBeNull(); + }); + + it("skips remembering when there is no workflow to remember it for", () => { + const setItem = vi.spyOn(Storage.prototype, "setItem"); + (component as any).rememberComputingUnit(undefined, 5); + expect(setItem).not.toHaveBeenCalled(); + setItem.mockRestore(); + }); + + it("recalls nothing when storage cannot be read", () => { + const getItem = vi.spyOn(Storage.prototype, "getItem").mockImplementation(() => { + throw new Error("storage blocked"); + }); + expect((component as any).recallComputingUnit(100)).toBeUndefined(); + getItem.mockRestore(); + }); + }); }); diff --git a/frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts b/frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts index 06da3e69abe..eb899448f3d 100644 --- a/frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts +++ b/frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts @@ -18,7 +18,7 @@ */ import { ChangeDetectorRef, Component, OnInit, NgZone, ViewChild } from "@angular/core"; -import { take } from "rxjs/operators"; +import { filter, take } from "rxjs/operators"; import { WorkflowComputingUnitManagingService } from "../../../common/service/computing-unit/workflow-computing-unit/workflow-computing-unit-managing.service"; import { DashboardWorkflowComputingUnit } from "../../../common/type/workflow-computing-unit"; import { NotificationService } from "../../../common/service/notification/notification.service"; @@ -269,25 +269,71 @@ export class ComputingUnitSelectionComponent implements OnInit { if (wid !== this.workflowId) { this.workflowId = wid; if (isDefined(this.workflowId) && this.workflowId !== DEFAULT_WORKFLOW.wid) { - this.workflowExecutionsService - .retrieveLatestWorkflowExecution(this.workflowId) - .pipe(untilDestroyed(this)) - .subscribe({ - next: (latestWorkflowExecution: WorkflowExecutionsEntry) => { - this.selectComputingUnit(this.workflowId, latestWorkflowExecution.cuId); - }, - error: (err: unknown) => { - const runningUnit = this.allComputingUnits.find(unit => unit.status === "Running"); - if (runningUnit) { - this.selectComputingUnit(this.workflowId, runningUnit.computingUnit.cuid); - } - }, - }); + this.selectInitialUnit(this.workflowId); } } }); } + /** + * Pick the unit for a workflow that has just come into view. An explicit choice remembered for + * it is newer than its last run, so it wins -- but only once the unit list has arrived and still + * holds that unit. Deciding on an empty list would either chase a unit that has since been + * terminated (the status service waits for it to appear, forever, and the fallbacks below never + * run) or throw the choice away before the list has loaded. A remembered unit that is gone is + * forgotten, and the fallbacks take over: the last execution's unit, else any running unit. + */ + private selectInitialUnit(wid: number): void { + const remembered = this.recallComputingUnit(wid); + if (!isDefined(remembered)) { + this.selectFromLastExecution(wid); + return; + } + this.computingUnitStatusService + .getAllComputingUnits() + .pipe( + filter(units => units.length > 0), + take(1), + untilDestroyed(this) + ) + .subscribe(units => { + // The workflow can change while the list is still loading; that later change made its own + // decision, so this one is stale. + if (wid !== this.workflowId) { + return; + } + if (units.some(unit => unit.computingUnit.cuid === remembered)) { + this.selectComputingUnit(wid, remembered); + } else { + this.forgetComputingUnit(wid); + this.selectFromLastExecution(wid); + } + }); + } + + /** The unit the workflow last ran on, else any unit that is running. */ + private selectFromLastExecution(wid: number): void { + // The workflow can change while the lookup is out; that later change decided for itself, so an + // answer (or a failure) that arrives for the earlier one is stale. + const stillShown = () => wid === this.workflowId; + this.workflowExecutionsService + .retrieveLatestWorkflowExecution(wid) + .pipe(untilDestroyed(this)) + .subscribe({ + next: (latestWorkflowExecution: WorkflowExecutionsEntry) => { + if (stillShown()) { + this.selectComputingUnit(wid, latestWorkflowExecution.cuId); + } + }, + error: () => { + const runningUnit = this.allComputingUnits.find(unit => unit.status === "Running"); + if (stillShown() && runningUnit) { + this.selectComputingUnit(wid, runningUnit.computingUnit.cuid); + } + }, + }); + } + /** * Called whenever the selected computing unit changes. */ @@ -297,6 +343,72 @@ export class ComputingUnitSelectionComponent implements OnInit { } } + /** + * The user's own pick from the list: select it and remember it for this workflow. Only an explicit + * choice is remembered -- the units selected on load (the remembered one, the last execution's, + * a running one) are derived and must not be stored as if chosen, or a derived unit would later + * outrank a fresher last execution. + */ + public onPickComputingUnit(unit: DashboardWorkflowComputingUnit): void { + this.selectedComputingUnit = unit; + const cuid = unit?.computingUnit?.cuid; + // The same rule as selectComputingUnit: nothing is selected, or remembered, for a workflow that + // has not been saved yet or for a unit without an id. + if (!isDefined(cuid) || this.workflowId === DEFAULT_WORKFLOW.wid) { + return; + } + this.selectComputingUnit(this.workflowId, cuid); + this.rememberComputingUnit(this.workflowId, cuid); + } + + /** + * The live selection lives only in ComputingUnitStatusService, re-derived on load from the + * last execution -- but that only exists once the workflow has run (pick a unit, reload + * before running, and it is gone). Canvas<->Form View switches reload, so we remember the + * last explicit choice per workflow to keep the two views agreeing. One unit per workflow. + */ + private static computingUnitStorageKey(wid: number): string { + return `computing-unit-of-workflow-${wid}`; + } + + private rememberComputingUnit(wid: number | undefined, cuid: number): void { + if (!isDefined(wid)) { + return; + } + try { + localStorage.setItem(ComputingUnitSelectionComponent.computingUnitStorageKey(wid), String(cuid)); + } catch { + // Private browsing or a full quota; remembering is an optimisation, not a + // requirement -- the last-execution lookup still applies on the next load. + } + } + + private recallComputingUnit(wid: number): number | undefined { + let stored: string | null = null; + try { + stored = localStorage.getItem(ComputingUnitSelectionComponent.computingUnitStorageKey(wid)); + } catch { + return undefined; + } + // A cuid is a positive integer. Number() would also accept "0" and "1.5", and handing + // either on would mean chasing a unit that cannot exist. Whether the unit still exists is + // not decided here but against the loaded unit list (selectInitialUnit). + const cuid = Number(stored); + if (!stored || !Number.isInteger(cuid) || cuid <= 0) { + return undefined; + } + return cuid; + } + + /** Drop a remembered unit that no longer exists, so the next load goes straight to the fallbacks. */ + private forgetComputingUnit(wid: number): void { + try { + localStorage.removeItem(ComputingUnitSelectionComponent.computingUnitStorageKey(wid)); + } catch { + // Best effort, like remembering: a stale entry only costs the list check on the next load. + } + } + isComputingUnitRunning(): boolean { return this.selectedComputingUnit != null && this.selectedComputingUnit.status === "Running"; } @@ -348,7 +460,8 @@ export class ComputingUnitSelectionComponent implements OnInit { } onComputingUnitCreated(unit: DashboardWorkflowComputingUnit): void { - this.selectComputingUnit(this.workflowId, unit.computingUnit.cuid); + // Creating a unit from here is as explicit a choice as picking one. + this.onPickComputingUnit(unit); } openComputingUnitMetadataModal(unit: DashboardWorkflowComputingUnit) {