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 a35c9ec106e..8a42df86384 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 @@ -46,6 +46,7 @@ import { DashboardWorkflow } from "../../../dashboard/type/dashboard-workflow.in import { DefaultView } from "../../../dashboard/type/workflow-metadata.interface"; import { SearchFilterParameters, toQueryStrings } from "../../../dashboard/type/search-filter-parameters"; import { NotificationService } from "../notification/notification.service"; +import { WorkflowActionService } from "../../../workspace/service/workflow-graph/model/workflow-action.service"; import { last } from "rxjs/operators"; describe("WorkflowPersistService", () => { @@ -70,9 +71,15 @@ describe("WorkflowPersistService", () => { '{"linkID":"link-c94e24a6-2c77-40cf-ba22-1a7ffba64b7d","source":{"operatorID":' + '"MySQLSource-operator-1ee619b1-8884-4564-a136-29ef77dfcc50","portID":"output-0"},"target":' + '{"operatorID":"Limit-operator-a11370eb-940a-4f10-8b36-8b413b2396c9","portID":"input-0"}}],"breakpoints":{}}'; + // What the page currently holds as the open workflow's metadata (read at response time to keep + // the user's name/description edits). Another workflow by default, so a response is relayed as + // is; the tests about local edits point it at the saved workflow. + let currentMetadata: { wid: number | undefined; name: string; description: string | undefined }; beforeEach(() => { + currentMetadata = { wid: 999, name: "another workflow", description: undefined }; TestBed.configureTestingModule({ imports: [HttpClientTestingModule], + providers: [{ provide: WorkflowActionService, useValue: { getWorkflowMetadata: () => currentMetadata } }], }); service = TestBed.inject(WorkflowPersistService); httpTestingController = TestBed.inject(HttpTestingController); @@ -260,6 +267,87 @@ describe("WorkflowPersistService", () => { expect(secondName).toBe("second"); }); + describe("a response versus an edit made since the save was sent", () => { + const wf = (name: string) => ({ wid: 9, name, description: "d1", content: validContent }) as unknown as Workflow; + + it("relays the response with the page's current name and description, not the ones it was saved with", () => { + currentMetadata = { wid: 9, name: "renamed meanwhile", description: "described meanwhile" }; + let result: Workflow | undefined; + service.persistWorkflow(wf("old")).subscribe(w => (result = w)); + + httpTestingController + .expectOne(`${API}/${WORKFLOW_PERSIST_URL}`) + .flush({ wid: 9, name: "old", description: "d1", lastModifiedTime: 777, content: "{}" }); + + // Feeding this back as the metadata keeps the rename; the server-owned fields still arrive. + expect(result?.name).toBe("renamed meanwhile"); + expect(result?.description).toBe("described meanwhile"); + expect(result?.lastModifiedTime).toBe(777); + }); + + it("keeps the local name for a workflow the save has just created (the page still holds the default id)", () => { + currentMetadata = { wid: 0, name: "named before the first save answered", description: undefined }; + let result: Workflow | undefined; + service.persistWorkflow({ ...wf("Untitled workflow"), wid: 0 } as Workflow).subscribe(w => (result = w)); + + httpTestingController + .expectOne(`${API}/${WORKFLOW_PERSIST_URL}`) + .flush({ wid: 42, name: "Untitled workflow", content: "{}" }); + + expect(result?.wid).toBe(42); + expect(result?.name).toBe("named before the first save answered"); + }); + + it("leaves the response alone when another workflow is open by the time it answers", () => { + currentMetadata = { wid: 10, name: "the other one", description: undefined }; + let result: Workflow | undefined; + service.persistWorkflow(wf("old")).subscribe(w => (result = w)); + + httpTestingController.expectOne(`${API}/${WORKFLOW_PERSIST_URL}`).flush({ wid: 9, name: "old", content: "{}" }); + + expect(result?.name).toBe("old"); + }); + }); + + describe("whenSavesDrained", () => { + const wf = (name: string) => ({ wid: 9, name, description: "", content: validContent }) as unknown as Workflow; + + it("emits at once when no save is pending", () => { + let emitted = false; + service.whenSavesDrained().subscribe(() => (emitted = true)); + expect(emitted).toBe(true); + }); + + it("emits only once the last queued save has answered, not when the first has", () => { + service.persistWorkflow(wf("first")).subscribe(); + service.persistWorkflow(wf("second")).subscribe(); + let emitted = false; + service.whenSavesDrained().subscribe(() => (emitted = true)); + + httpTestingController + .expectOne(`${API}/${WORKFLOW_PERSIST_URL}`) + .flush({ wid: 9, name: "first", content: "{}" }); + expect(emitted).toBe(false); // the second is still out + + httpTestingController + .expectOne(`${API}/${WORKFLOW_PERSIST_URL}`) + .flush({ wid: 9, name: "second", content: "{}" }); + expect(emitted).toBe(true); + }); + + it("counts a failed save as done, so a failure does not hold the drain forever", () => { + service.persistWorkflow(wf("first")).subscribe({ error: () => {} }); + let emitted = false; + service.whenSavesDrained().subscribe(() => (emitted = true)); + + httpTestingController + .expectOne(`${API}/${WORKFLOW_PERSIST_URL}`) + .flush("boom", { status: 500, statusText: "Server Error" }); + + expect(emitted).toBe(true); + }); + }); + it("persistWorkflow notifies the user when the workflow is broken but still POSTs", () => { const errorSpy = vi.spyOn(notificationService, "error").mockImplementation(() => {}); const workflow = { 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 66d267a673d..41123280eef 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 @@ -18,14 +18,18 @@ */ import { HttpClient, HttpParams } from "@angular/common/http"; -import { Injectable } from "@angular/core"; -import { EMPTY, Observable, ReplaySubject, Subject, throwError } from "rxjs"; -import { catchError, concatMap, filter, map, tap } from "rxjs/operators"; +import { Injectable, Injector } from "@angular/core"; +import { EMPTY, Observable, of, ReplaySubject, Subject, throwError } from "rxjs"; +import { catchError, concatMap, filter, finalize, map, take, tap } from "rxjs/operators"; import { AppSettings } from "../../app-setting"; import { Workflow, WorkflowContent } from "../../type/workflow"; import { DashboardWorkflow } from "../../../dashboard/type/dashboard-workflow.interface"; import { DefaultView } from "../../../dashboard/type/workflow-metadata.interface"; import { WorkflowUtilService } from "../../../workspace/service/workflow-graph/util/workflow-util.service"; +import { + DEFAULT_WORKFLOW, + WorkflowActionService, +} from "../../../workspace/service/workflow-graph/model/workflow-action.service"; import { NotificationService } from "../notification/notification.service"; import { SearchFilterParameters, toQueryStrings } from "../../../dashboard/type/search-filter-parameters"; import { User } from "../../type/user"; @@ -68,29 +72,74 @@ export class WorkflowPersistService { * 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. + * + * A response is relayed with the page's current name and description in place of its own (see + * withLocalEdits): those are the two fields a user edits, and a response answers the save it was + * sent for, which may be older than an edit made since. Callers feed the response back as the + * workflow's metadata; without this, a rename made while a save was out came back undone. */ private readonly persistQueue = new Subject<{ send: Observable; result: Subject }>(); + /** Saves asked for and not yet answered (or failed); see whenSavesDrained. */ + private pendingSaves = 0; + private readonly savesDrained = new Subject(); + constructor( private http: HttpClient, - private notificationService: NotificationService + private notificationService: NotificationService, + // Looked up lazily, at response time: the persist service is also used by the dashboard, where + // no workflow is open and constructing the (graph-owning) action service would be a side effect. + private injector: Injector ) { this.persistQueue .pipe( concatMap(({ send, result }) => send.pipe( + map(updated => this.withLocalEdits(updated)), tap({ next: updated => result.next(updated), error: (err: unknown) => result.error(err), complete: () => result.complete(), }), - catchError(() => EMPTY) + catchError(() => EMPTY), + finalize(() => this.saveDone()) ) ) ) .subscribe(); } + /** + * Emits once every save asked for so far has been answered or has failed; at once when none is + * pending. For a caller that leaves the page on completion (the view switches): its own save + * completing is not enough, a save queued behind it (a rename's, a description's) would still be + * aborted by the page load. + */ + public whenSavesDrained(): Observable { + return this.pendingSaves === 0 ? of(undefined) : this.savesDrained.pipe(take(1)); + } + + private saveDone(): void { + this.pendingSaves -= 1; + if (this.pendingSaves === 0) { + this.savesDrained.next(); + } + } + + /** + * The response with the page's current name and description: a response carries the values the + * save was sent with, and an edit made since would be undone by feeding them back. Left alone when + * another workflow is open by now (nothing local belongs to this response); a workflow just created + * still carries the default id locally, and its rename made meanwhile is kept too. + */ + private withLocalEdits(response: Workflow): Workflow { + const current = this.injector.get(WorkflowActionService).getWorkflowMetadata(); + if (current.wid !== response.wid && current.wid !== DEFAULT_WORKFLOW.wid) { + return response; + } + return { ...response, name: current.name, description: current.description }; + } + /** * 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 @@ -122,6 +171,7 @@ export class WorkflowPersistService { // 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.pendingSaves += 1; this.persistQueue.next({ send, result }); return result.asObservable(); } 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 3e15706cf0f..dc8c39ecad1 100644 --- a/frontend/src/app/workspace/component/menu/menu.component.spec.ts +++ b/frontend/src/app/workspace/component/menu/menu.component.spec.ts @@ -203,6 +203,26 @@ describe("MenuComponent", () => { expect(component.isSaving).toBe(false); }); + it("leaves only once every queued save has landed, not just its own", () => { + // A rename made while the switch's save is out saves through the menu itself and is queued + // behind the switch's save; the hand-over waits for the queue to drain, or the page load + // would abort that save. + component.writeAccess = true; + vi.spyOn(component["workflowActionService"], "getWorkflowMetadata").mockReturnValue({ wid: 7 } as any); + vi.spyOn(workflowPersistService, "persistWorkflow").mockReturnValue(of({ wid: 7, name: "saved" } as any)); + vi.spyOn(component["workflowActionService"], "setWorkflowMetadata").mockImplementation(() => {}); + const drained$ = new Subject(); + vi.spyOn(workflowPersistService, "whenSavesDrained").mockReturnValue(drained$.asObservable()); + const navigate = vi.spyOn(component as any, "openFormViewPage").mockImplementation(() => {}); + + component.onClickOpenFormView(); + + expect(navigate).not.toHaveBeenCalled(); // its own save is done, another is still queued + drained$.next(); + 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); diff --git a/frontend/src/app/workspace/component/menu/menu.component.ts b/frontend/src/app/workspace/component/menu/menu.component.ts index 3a46e987f33..26c06ea3129 100644 --- a/frontend/src/app/workspace/component/menu/menu.component.ts +++ b/frontend/src/app/workspace/component/menu/menu.component.ts @@ -665,11 +665,14 @@ export class MenuComponent implements OnInit, OnDestroy { // 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 + // Three 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. + // it and completes after it. A graph edit made while our save is out (the page stays editable + // until the load): workflowChanged marks it, and saveThenOpenFormView saves once more before + // handing over rather than letting the full-page load abort that edit's own debounced autosave. + // And a save queued behind ours (a rename's or a description's, which save through the menu + // itself and do not go through workflowChanged): the hand-over leaves only once the service's + // save queue has drained. this.handingOverToFormView = true; this.isSaving = true; this.saveThenOpenFormView(wid); @@ -702,8 +705,15 @@ export class MenuComponent implements OnInit, OnDestroy { this.saveThenOpenFormView(target); return; } - this.isSaving = false; - this.openFormViewPage(target); + // A save queued behind ours (a rename's, a description's: those save through the menu + // itself, not the autosave) must land too, or the page load aborts it. + this.workflowPersistService + .whenSavesDrained() + .pipe(untilDestroyed(this)) + .subscribe(() => { + this.isSaving = false; + this.openFormViewPage(target); + }); }, }); }