This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-8539-c49c9c3007d04fd6e6309aed675155d5f302ab7f in repository https://gitbox.apache.org/repos/asf/texera.git
commit 9709525f9b14a6b2ea05f0fce0156b9c09cd5e62 Author: yangzhang75 <[email protected]> AuthorDate: Fri Sep 25 04:46:33 2026 +0000 fix(frontend): make a result cell's download work off the canvas (#8539) ### What changes were proposed in this PR? The download button on a result cell did nothing when pressed from the Form View: the dialog opened, Export closed it, and no file arrived — no request, no error, no message. An export scopes itself to the operators selected on the canvas unless the caller asked for all of them. That is the right scope for the two callers that read the canvas: the top menu asks for everything, the context menu exports the selection. The third caller is the download button on a result cell, and a cell belongs to one operator, whoever happens to be selected. The same cell is also mounted on the Form View, where the selection holds the step the user is configuring and holds nothing at all until they click one. There the export found an empty scope, so `performExport` returned at its `operatorIds.length === 0` check before sending anything. The dialog's own checks read the same empty selection, so a restricted operator went unreported there too. A caller that knows which operators it is exporting now says so, and both the dialog and the service prefer that over the selection: - `workflow-result-export.service.ts`: a trailing optional `requestedOperatorIds` on `exportWorkflowExecutionResult`, threaded to `performExport`. Precedence is `exportAll` (whole workflow) > caller-named > canvas selection. - `result-exportation.component.ts`: reads `operatorIds` off the modal data; `getOperatorIdsToCheck()` returns it when present, and `onClickExportResult` forwards it. - `result-table-frame.component.ts`: the cell's dialog names the operator whose results the frame is showing. Nothing changes for the menu or the context menu, which name no operators and keep the old scope. Ten lines of logic in three files. ### Any related issues, documentation, discussions? Closes #8538, which has a recording of the before state. One thing that PR does not change: `export-execution-result-enabled` still defaults to `false` in `gui.conf`, and `performExport` returns on that switch before it reaches any of this. A deployment with the switch off sees no change from this PR; every deployment that has it on gets a working cell download. Whether that default should flip is a separate question and is noted at the end of the issue. ### How was this PR tested? - Nine new unit tests across the three spec files: the service prefers a named scope over the selection and works with an empty selection, falls back to the selection when none is named, and still lets `exportAll` win; the dialog names none by default, checks the named operator, and hands it to the export; the frame names its operator, and names none when it has none. - Mutation check, each restored afterwards: dropping the named scope in the service, in the dialog, or in the frame each turns exactly one named test red. - Coverage on the three changed files: 100% lines, functions and branches, except one pre-existing unreachable branch elsewhere in `result-table-frame.component.ts` that this PR does not touch. - `ng test --watch=false`: 223 files, 6065 passed, 1 skipped (pre-existing), 0 failed. `ng build --configuration=production` (AOT): clean. `eslint` and `prettier --check` on all six files: clean. - Exercised in a running instance built from this branch on top of `main`: a Form View workflow whose result column holds binary files. With the fix, the cell download sends `POST /api/executions/result/export/local` and returns the bytes (4096 bytes, HDF5 magic `89 48 44 46`, byte-exact) while the canvas selection is empty. With the three production hunks reverted on the same stack, the same click sends no request and produces no file within two minutes. #### After the fix: downloading a result file from the Form View https://github.com/user-attachments/assets/ba8da56b-429d-43d9-9961-aa078268e2d0 ### Was this PR authored or co-authored using generative AI tooling? Yes. Generated-by: Claude Code (Claude Opus 5, Anthropic). Co-authored with Claude; the author reviewed the change before submission. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <[email protected]> --- .../result-exportation.component.spec.ts | 173 +++++++++++++++++++-- .../result-exportation.component.ts | 37 +++-- .../result-table-frame.component.html | 4 + .../result-table-frame.component.spec.ts | 74 +++++++++ .../result-table-frame.component.ts | 15 +- .../workflow-result-export.service.spec.ts | 111 ++++++++++--- .../workflow-result-export.service.ts | 39 +++-- 7 files changed, 389 insertions(+), 64 deletions(-) diff --git a/frontend/src/app/workspace/component/result-exportation/result-exportation.component.spec.ts b/frontend/src/app/workspace/component/result-exportation/result-exportation.component.spec.ts index ed3c62c8fc..9470b694dd 100644 --- a/frontend/src/app/workspace/component/result-exportation/result-exportation.component.spec.ts +++ b/frontend/src/app/workspace/component/result-exportation/result-exportation.component.spec.ts @@ -88,7 +88,10 @@ describe("ResultExportationComponent", () => { { provide: WorkflowActionService, useValue: { - getTexeraGraph: vi.fn().mockReturnValue({ getAllOperators: vi.fn().mockReturnValue([]) }), + getTexeraGraph: vi.fn().mockReturnValue({ + getAllOperators: vi.fn().mockReturnValue([]), + getAllOperatorIDs: vi.fn().mockReturnValue([]), + }), getJointGraphWrapper: vi .fn() .mockReturnValue({ getCurrentHighlightedOperatorIDs: vi.fn().mockReturnValue([]) }), @@ -141,8 +144,7 @@ describe("ResultExportationComponent", () => { expect(args[0]).toBe("csv"); // exportType, from modal data expect(args[1]).toBe("my-workflow"); // workflowName expect(args[2]).toEqual([1]); // datasetIds resolved from ds.dataset.did - expect(args[6]).toBe(true); // exportAll, because sourceTriggered === "menu" - expect(args[7]).toBe("dataset"); // destination + expect(args[6]).toBe("dataset"); // destination expect(modalClose).toHaveBeenCalledTimes(1); }); @@ -152,10 +154,45 @@ describe("ResultExportationComponent", () => { expect(exportWorkflowExecutionResult).toHaveBeenCalledTimes(1); const args = exportWorkflowExecutionResult.mock.calls[0]; expect(args[2]).toEqual([]); // local download carries no dataset ids - expect(args[7]).toBe("local"); + expect(args[6]).toBe("local"); expect(modalClose).toHaveBeenCalledTimes(1); }); + // The dialog reports on a scope (what a blocking dataset blocks, what kind of output is on + // offer) and then exports. Those have to be the same scope, so the export is handed the list + // the dialog resolved rather than a flag the service would resolve again, differently. + describe("the scope the dialog exports", () => { + const exportedOperatorIds = (): readonly string[] => { + component.onClickExportResult("local"); + return exportWorkflowExecutionResult.mock.calls[0][8]; + }; + const graph = () => TestBed.inject(WorkflowActionService).getTexeraGraph() as any; + const wrapper = () => TestBed.inject(WorkflowActionService).getJointGraphWrapper() as any; + + it("is the operators the caller named, whatever the canvas has selected", () => { + component.operatorIds = ["opNamed"]; + wrapper().getCurrentHighlightedOperatorIDs.mockReturnValue(["opHighlighted"]); + + expect(exportedOperatorIds()).toEqual(["opNamed"]); + }); + + it("is the whole workflow when the top menu opened the dialog", () => { + component.operatorIds = []; + component.sourceTriggered = "menu"; + graph().getAllOperatorIDs.mockReturnValue(["op1", "op2"]); + + expect(exportedOperatorIds()).toEqual(["op1", "op2"]); + }); + + it("is the canvas selection for anyone else who named nothing", () => { + component.operatorIds = []; + component.sourceTriggered = "context-menu"; + wrapper().getCurrentHighlightedOperatorIDs.mockReturnValue(["opHighlighted"]); + + expect(exportedOperatorIds()).toEqual(["opHighlighted"]); + }); + }); + it("onClickCreateNewDataset opens the dataset-creator modal and adopts the created dataset", () => { const created = { dataset: { did: 9, name: "brand-new" }, @@ -183,6 +220,7 @@ describe("ResultExportationComponent", () => { }; graph.getTexeraGraph.mockReturnValue({ getAllOperators: () => ids.map(id => ({ operatorID: id })), + getAllOperatorIDs: () => ids, }); } @@ -240,6 +278,7 @@ describe("ResultExportationComponent", () => { }; graph.getTexeraGraph.mockReturnValue({ getAllOperators: () => ids.map(id => ({ operatorID: id })), + getAllOperatorIDs: () => ids, }); } @@ -381,6 +420,7 @@ describe("ResultExportationComponent", () => { }; graph.getTexeraGraph.mockReturnValue({ getAllOperators: () => ids.map(id => ({ operatorID: id })), + getAllOperatorIDs: () => ids, }); } @@ -484,7 +524,7 @@ describe("ResultExportationComponent", () => { exportBtn!.triggerEventHandler("click", null); expect(exportWorkflowExecutionResult).toHaveBeenCalledTimes(1); const args = exportWorkflowExecutionResult.mock.calls[0]; - expect(args[7]).toBe("local"); + expect(args[6]).toBe("local"); }); it("renders the dataset destination with its list and create button", () => { @@ -601,7 +641,7 @@ describe("ResultExportationComponent", () => { expect(exportWorkflowExecutionResult).toHaveBeenCalledTimes(1); const args = exportWorkflowExecutionResult.mock.calls[0]; expect(args[2]).toEqual([1]); // the clicked dataset's did - expect(args[7]).toBe("dataset"); + expect(args[6]).toBe("dataset"); expect(modalClose).toHaveBeenCalledTimes(1); }); @@ -780,7 +820,10 @@ describe("ResultExportationComponent (context-menu source with default modal dat { provide: WorkflowActionService, useValue: { - getTexeraGraph: vi.fn().mockReturnValue({ getAllOperators: vi.fn().mockReturnValue([]) }), + getTexeraGraph: vi.fn().mockReturnValue({ + getAllOperators: vi.fn().mockReturnValue([]), + getAllOperatorIDs: vi.fn().mockReturnValue([]), + }), getJointGraphWrapper: vi.fn().mockReturnValue({ getCurrentHighlightedOperatorIDs: vi.fn().mockReturnValue(["hl-1", "hl-2"]), }), @@ -826,7 +869,7 @@ describe("ResultExportationComponent (context-menu source with default modal dat expect(component.blockedOperatorIds).toEqual([]); }); - it("exports highlighted operators only (exportAll === false) for a context-menu trigger", () => { + it("exports to the destination it was given for a context-menu trigger", () => { const exportService = TestBed.inject(WorkflowResultExportService) .exportWorkflowExecutionResult as unknown as ReturnType<typeof vi.fn>; @@ -834,7 +877,117 @@ describe("ResultExportationComponent (context-menu source with default modal dat expect(exportService).toHaveBeenCalledTimes(1); const args = exportService.mock.calls[0]; - expect(args[6]).toBe(false); // exportAll is false because sourceTriggered !== "menu" - expect(args[7]).toBe("local"); + expect(args[6]).toBe("local"); // destination + }); + + // The context menu names no operators of its own, so the dialog resolves the selection and + // hands that over. It does not pass nothing and leave the export to read the canvas a second + // time, which would let what is exported differ from what the dialog reported on. + it("hands over the selection it resolved, not an empty scope", () => { + const exportService = TestBed.inject(WorkflowResultExportService) + .exportWorkflowExecutionResult as unknown as ReturnType<typeof vi.fn>; + + component.onClickExportResult("local"); + + expect(exportService.mock.calls[0][8]).toEqual(["hl-1", "hl-2"]); + }); +}); + +// A result cell opens this dialog naming the one operator whose results it shows. Both the +// dialog's own checks and the export it triggers have to use that operator: the cell is also +// mounted on the Form View, where nothing is selected until the user clicks a step, so reading +// the selection there answered "no operators" and the export sent nothing. +describe("ResultExportationComponent (a caller that names its operators)", () => { + let component: ResultExportationComponent; + let fixture: ComponentFixture<ResultExportationComponent>; + + // Exactly what result-table-frame.component.ts puts in nzData, including the absence of + // sourceTriggered: a result cell names its operator instead of naming a trigger. + const CELL_DATA = { + exportType: "data", + workflowName: "cell-workflow", + defaultFileName: "content_3", + rowIndex: 3, + columnIndex: 1, + operatorIds: ["op-named"], + }; + + beforeEach(async () => { + await TestBed.configureTestingModule({ + imports: [ResultExportationComponent], + providers: [ + { provide: NZ_MODAL_DATA, useValue: CELL_DATA }, + { provide: NzModalRef, useValue: { close: vi.fn(), getConfig: () => ({}) } }, + { provide: NzModalService, useValue: { create: vi.fn().mockReturnValue({ afterClose: of(null) }) } }, + { + provide: WorkflowResultExportService, + useValue: { + computeRestrictionAnalysis: vi.fn().mockReturnValue(of(new WorkflowResultDownloadability(new Map()))), + exportWorkflowExecutionResult: vi.fn(), + }, + }, + { + provide: DatasetService, + useValue: { retrieveAccessibleDatasets: vi.fn().mockReturnValue(of([])) }, + }, + { + provide: WorkflowActionService, + useValue: { + // Both fallbacks answer with something else, so a passing test can only be reading + // the operator the caller named. + getTexeraGraph: vi.fn().mockReturnValue({ + getAllOperators: vi.fn().mockReturnValue([{ operatorID: "op-all" }]), + getAllOperatorIDs: vi.fn().mockReturnValue(["op-all"]), + }), + getJointGraphWrapper: vi.fn().mockReturnValue({ + getCurrentHighlightedOperatorIDs: vi.fn().mockReturnValue(["op-highlighted"]), + }), + }, + }, + { + provide: WorkflowResultService, + useValue: { + determineOutputTypes: vi.fn().mockReturnValue({ + hasAnyResult: true, + isTableOutput: true, + isVisualizationOutput: false, + containsBinaryData: true, + }), + }, + }, + { + provide: ComputingUnitStatusService, + useValue: { getSelectedComputingUnit: vi.fn().mockReturnValue(of(null)) }, + }, + ], + }).compileComponents(); + fixture = TestBed.createComponent(ResultExportationComponent); + component = fixture.componentInstance; + fixture.detectChanges(); + }); + + afterEach(() => { + fixture?.destroy(); + }); + + // A result cell names its operator and sends no trigger of its own, so this field arrives + // absent and must read as a string rather than as undefined wearing a string's type. + it("reads an absent source trigger as an empty string", () => { + expect(component.sourceTriggered).toBe(""); + }); + + it("checks the named operator rather than the canvas selection", () => { + expect(component.exportableOperatorIds).toEqual(["op-named"]); + expect(component.blockedOperatorIds).toEqual([]); + }); + + it("hands the named operator to the export", () => { + const exportService = TestBed.inject(WorkflowResultExportService) + .exportWorkflowExecutionResult as unknown as ReturnType<typeof vi.fn>; + + component.onClickExportResult("local"); + + expect(exportService).toHaveBeenCalledTimes(1); + expect(exportService.mock.calls[0][8]).toEqual(["op-named"]); }); }); diff --git a/frontend/src/app/workspace/component/result-exportation/result-exportation.component.ts b/frontend/src/app/workspace/component/result-exportation/result-exportation.component.ts index 33abb62524..96fafe7ca7 100644 --- a/frontend/src/app/workspace/component/result-exportation/result-exportation.component.ts +++ b/frontend/src/app/workspace/component/result-exportation/result-exportation.component.ts @@ -81,17 +81,20 @@ import { NzIconDirective } from "ng-zorro-antd/icon"; ], }) export class ResultExportationComponent implements OnInit { - /* Two sources can trigger this dialog, one from context-menu - which only export highlighted operators - and second is menu which wants to export all operators + /* Three sources can trigger this dialog: the context-menu, which exports the highlighted + operators; the menu, which wants to export all of them; and a result cell, which names the + one operator whose results it shows in operatorIds below and sends no trigger of its own. */ - sourceTriggered: string = inject(NZ_MODAL_DATA).sourceTriggered; + sourceTriggered: string = inject(NZ_MODAL_DATA).sourceTriggered ?? ""; workflowName: string = inject(NZ_MODAL_DATA).workflowName; inputFileName: string = inject(NZ_MODAL_DATA).defaultFileName ?? ""; rowIndex: number = inject(NZ_MODAL_DATA).rowIndex ?? -1; columnIndex: number = inject(NZ_MODAL_DATA).columnIndex ?? -1; destination: string = ""; exportType: string = inject(NZ_MODAL_DATA).exportType ?? ""; + // The operators this export covers, when the caller knows them. Empty leaves the scope to + // the rule below: the whole workflow for the menu, the canvas selection for the rest. + operatorIds: readonly string[] = inject(NZ_MODAL_DATA).operatorIds ?? []; isTableOutput: boolean = false; isVisualizationOutput: boolean = false; containsBinaryData: boolean = false; @@ -103,15 +106,21 @@ export class ResultExportationComponent implements OnInit { filteredUserAccessibleDatasets: DashboardDataset[] = []; /** - * Gets the operator IDs to check for restrictions based on the source trigger. - * Menu: all operators, Context menu: highlighted operators only + * The operators this export covers, which everything below reads: what may be exported, what + * a blocking dataset blocks, and what kind of output the dialog is offering. + * + * A caller that named them wins. The two fallbacks each belong to a caller reading the + * canvas -- the menu exports the whole workflow, the context menu exports the selection -- + * and a result cell is neither: it belongs to one operator, whoever is selected. On the Form + * View nothing is selected until the user clicks a step, so this answered "no operators", and + * with nothing in scope a blocked operator went unreported. */ private getOperatorIdsToCheck(): readonly string[] { + if (this.operatorIds.length > 0) { + return this.operatorIds; + } if (this.sourceTriggered === "menu") { - return this.workflowActionService - .getTexeraGraph() - .getAllOperators() - .map(op => op.operatorID); + return this.workflowActionService.getTexeraGraph().getAllOperatorIDs(); } else { return this.workflowActionService.getJointGraphWrapper().getCurrentHighlightedOperatorIDs(); } @@ -199,7 +208,7 @@ export class ResultExportationComponent implements OnInit { const operatorIds = this.getOperatorIdsToCheck(); if (operatorIds.length === 0) { - // No operators highlighted + // No operators in scope this.isTableOutput = false; this.isVisualizationOutput = false; this.containsBinaryData = false; @@ -261,9 +270,11 @@ export class ResultExportationComponent implements OnInit { this.rowIndex, this.columnIndex, this.inputFileName, - this.sourceTriggered === "menu", destination, - this.selectedComputingUnit + this.selectedComputingUnit, + // The same scope the dialog reported on, so what is exported is what the dialog said it + // would export. + this.getOperatorIdsToCheck() ); this.modalRef.close(); } diff --git a/frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.html b/frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.html index e9c5f83ee4..be6b1c06b1 100644 --- a/frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.html +++ b/frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.html @@ -193,7 +193,11 @@ <ng-container *ngSwitchDefault>{{ column.getCell(row) }}</ng-container> </ng-container> </span> + <!-- Not rendered when result export is switched off for the deployment: every action + behind it returns without sending a request, so the button would be there and do + nothing. The top menu and the context menu already honour the same switch. --> <button + *ngIf="guiConfigService.env.exportExecutionResultEnabled" (click)="downloadData(currentResult[i][column.columnDef], i, columnIndex, column.columnDef); $event.stopPropagation()" nz-button nzType="link" diff --git a/frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.spec.ts b/frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.spec.ts index 50e7c224a2..9afb07d04c 100644 --- a/frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.spec.ts +++ b/frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.spec.ts @@ -31,6 +31,8 @@ import { By, DomSanitizer } from "@angular/platform-browser"; import { of, Subject } from "rxjs"; import { commonTestProviders } from "../../../../common/testing/test-utils"; import { GuiConfigService } from "../../../../common/service/gui-config.service"; +import { MockGuiConfigService } from "../../../../common/service/gui-config.service.mock"; +import { WorkflowActionService } from "../../../service/workflow-graph/model/workflow-action.service"; import { isAudioUrl, isImageUrl, isVideoUrl } from "../../../../common/util/media-type.util"; import { OperatorPaginationResultService, @@ -92,6 +94,13 @@ describe("ResultTableFrameComponent", () => { const queryParams = (pageIndex: number): NzTableQueryParams => ({ pageIndex, pageSize: 5, sort: [], filter: [] }); + // `commonTestProviders` supplies MockGuiConfigService, which is what the component receives, + // so the deployment's export switch is driven through it. + const setExport = (enabled: boolean): void => + (TestBed.inject(GuiConfigService) as unknown as MockGuiConfigService).setConfig({ + exportExecutionResultEnabled: enabled, + }); + // Re-creates the component so spies installed on service streams are picked up by ngOnInit. // Destroys the fixture created in beforeEach (or a prior recreate) first so its // untilDestroyed subscriptions are torn down and cannot leak across the test. @@ -612,6 +621,7 @@ describe("ResultTableFrameComponent", () => { describe("downloadData", () => { it("opens the export modal for the clicked cell's absolute row index", () => { const createSpy = vi.spyOn(modalService, "create").mockReturnValue({} as any); + component.operatorId = "op1"; component.currentPageIndex = 2; component.pageSize = 5; @@ -625,6 +635,45 @@ describe("ResultTableFrameComponent", () => { expect.objectContaining({ exportType: "data", defaultFileName: "name_6", rowIndex: 6, columnIndex: 3 }) ); }); + + // The export otherwise scopes itself to whatever the canvas has selected, which answers a + // different question: what the user picked, not what this frame shows. The frame also mounts + // on the Form View, where nothing is selected until the user clicks a step, so the export + // found an empty scope and did nothing. + it("names the operator whose results it is showing, so the export has a scope", () => { + const createSpy = vi.spyOn(modalService, "create").mockReturnValue({} as any); + component.operatorId = "op1"; + + component.downloadData("alice", 0, 0, "name"); + + expect((createSpy.mock.calls[0][0] as any).nzData).toEqual(expect.objectContaining({ operatorIds: ["op1"] })); + }); + + // `getWorkflowMetadata` is a method: read without calling it, `.name` is the function's own + // name, so every export carried the string "getWorkflowMetadata" as the workflow's name. + it("carries the workflow's real name, not the name of the accessor", () => { + const createSpy = vi.spyOn(modalService, "create").mockReturnValue({} as any); + vi.spyOn(TestBed.inject(WorkflowActionService), "getWorkflowMetadata").mockReturnValue({ + name: "scGPT", + } as any); + component.operatorId = "op1"; + + component.downloadData("alice", 0, 0, "name"); + + expect((createSpy.mock.calls[0][0] as any).nzData).toEqual(expect.objectContaining({ workflowName: "scGPT" })); + }); + + // A cell belongs to the operator whose results the frame shows. With no operator there is + // nothing to scope an export to, and opening the dialog would only let the reader press + // buttons that export nothing. + it("does not open the dialog at all when the frame has no operator", () => { + const createSpy = vi.spyOn(modalService, "create").mockReturnValue({} as any); + component.operatorId = undefined; + + component.downloadData("alice", 0, 0, "name"); + + expect(createSpy).not.toHaveBeenCalled(); + }); }); describe("column navigation and search", () => { @@ -688,6 +737,8 @@ describe("ResultTableFrameComponent", () => { }); it("renders headers, per-column stats, and clickable row cells once results arrive", () => { + // The download click below needs a rendered button, and the button is gated on the switch. + setExport(true); component.operatorId = "op1"; component.setupResultTable([SAMPLE_ROW], 1); component.isFrontPagination = false; @@ -736,6 +787,29 @@ describe("ResultTableFrameComponent", () => { download.triggerEventHandler("click", { stopPropagation: vi.fn() }); expect(downloadSpy).toHaveBeenCalledWith("alice", 0, 0, "name"); }); + + // Result export is a deployment switch. Every action behind this button returns without + // sending a request when it is off, so the button is not rendered rather than left there + // to do nothing. The top menu and the context menu already honour the same switch. + describe("the per-cell download button and the export switch", () => { + const renderOneRow = (exportEnabled: boolean) => { + setExport(exportEnabled); + component.operatorId = "op1"; + component.setupResultTable([SAMPLE_ROW], 1); + component.isFrontPagination = false; + fixture.detectChanges(); + }; + + it("is rendered when result export is on", () => { + renderOneRow(true); + expect(fixture.debugElement.query(By.css("button.download-button"))).not.toBeNull(); + }); + + it("is not rendered when result export is off", () => { + renderOneRow(false); + expect(fixture.debugElement.query(By.css("button.download-button"))).toBeNull(); + }); + }); }); it("should detect media URLs for result cells", () => { diff --git a/frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.ts b/frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.ts index 7fcfd126a3..a329161369 100644 --- a/frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.ts +++ b/frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.ts @@ -130,7 +130,8 @@ export class ResultTableFrameComponent implements OnInit, OnChanges { private changeDetectorRef: ChangeDetectorRef, private sanitizer: DomSanitizer, private workflowStatusService: WorkflowStatusService, - private guiConfigService: GuiConfigService + // Read by the template only (the export button's flag); the template can see a protected member. + protected guiConfigService: GuiConfigService ) {} ngOnChanges(changes: SimpleChanges): void { @@ -464,6 +465,11 @@ export class ResultTableFrameComponent implements OnInit, OnChanges { } downloadData(data: any, rowIndex: number, columnIndex: number, columnName: string): void { + // A cell belongs to the operator whose results this frame is showing. Without one there is + // nothing to scope an export to, and the dialog would open only to export nothing. + if (!this.operatorId) { + return; + } const realRowNumber = (this.currentPageIndex - 1) * this.pageSize + rowIndex; const defaultFileName = `${columnName}_${realRowNumber}`; const modal = this.modalService.create({ @@ -471,10 +477,15 @@ export class ResultTableFrameComponent implements OnInit, OnChanges { nzContent: ResultExportationComponent, nzData: { exportType: "data", - workflowName: this.workflowActionService.getWorkflowMetadata.name, + workflowName: this.workflowActionService.getWorkflowMetadata()?.name, defaultFileName: defaultFileName, rowIndex: realRowNumber, columnIndex: columnIndex, + // Named rather than left to the canvas selection, which answers a different question: + // what the user has selected. This frame also mounts on the Form View, where nothing is + // selected until the user clicks a step, so the export found an empty scope and the + // button did nothing. + operatorIds: [this.operatorId], }, nzFooter: null, }); diff --git a/frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.spec.ts b/frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.spec.ts index b546e6a1a2..a1b13898d6 100644 --- a/frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.spec.ts +++ b/frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.spec.ts @@ -25,7 +25,7 @@ import { WorkflowActionService } from "../workflow-graph/model/workflow-action.s import { NotificationService } from "../../../common/service/notification/notification.service"; import { ExecuteWorkflowService } from "../execute-workflow/execute-workflow.service"; import { WorkflowResultService } from "../workflow-result/workflow-result.service"; -import { Observable, of, throwError } from "rxjs"; +import { Observable, of, Subject, throwError } from "rxjs"; import { ExecutionState } from "../../types/execute-workflow.interface"; import { DownloadService, ExportWorkflowJsonResponse } from "src/app/dashboard/service/user/download/download.service"; import { DatasetService } from "../../../dashboard/service/user/dataset/dataset.service"; @@ -255,7 +255,7 @@ describe("WorkflowResultExportService", () => { const download = stubDownloadService(); texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }] as any); - service.exportWorkflowExecutionResult("csv", "wf", [1], 0, 0, "file", true, "dataset", makeUnit()); + service.exportWorkflowExecutionResult("csv", "wf", [1], 0, 0, "file", "dataset", makeUnit(), ["op1"]); expect(notificationServiceSpy.loading).not.toHaveBeenCalled(); expect(notificationServiceSpy.error).not.toHaveBeenCalled(); @@ -267,7 +267,7 @@ describe("WorkflowResultExportService", () => { const download = stubDownloadService(); texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }] as any); - service.exportWorkflowExecutionResult("csv", "wf", [1], 0, 0, "file", true, "dataset", null); + service.exportWorkflowExecutionResult("csv", "wf", [1], 0, 0, "file", "dataset", null, ["op1"]); expect(notificationServiceSpy.error).toHaveBeenCalledWith( "Cannot export result: computing unit is not available" @@ -282,30 +282,103 @@ describe("WorkflowResultExportService", () => { const download = stubDownloadService(); texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }] as any); - service.exportWorkflowExecutionResult("csv", "wf", [1], 0, 0, "file", true, "dataset", makeUnit()); + service.exportWorkflowExecutionResult("csv", "wf", [1], 0, 0, "file", "dataset", makeUnit(), ["op1"]); expect(notificationServiceSpy.error).toHaveBeenCalledWith("Cannot export result: workflow ID is not available"); expect(download.exportWorkflowResultToDataset).not.toHaveBeenCalled(); }); - it("does nothing when no operators are selected (highlighted export with empty selection)", () => { + it("does nothing when the caller's scope is empty", () => { enableExport(); const download = stubDownloadService(); - jointGraphWrapperSpy.getCurrentHighlightedOperatorIDs.mockReturnValue([]); - service.exportWorkflowExecutionResult("csv", "wf", [1], 0, 0, "file", false, "dataset", makeUnit()); + service.exportWorkflowExecutionResult("csv", "wf", [1], 0, 0, "file", "dataset", makeUnit(), []); expect(notificationServiceSpy.loading).not.toHaveBeenCalled(); expect(notificationServiceSpy.error).not.toHaveBeenCalled(); expect(download.exportWorkflowResultToDataset).not.toHaveBeenCalled(); }); + // The scope is the caller's to decide and the service does not second-guess it: the dialog + // resolves what the export covers in order to report on it, and the same list is what gets + // exported. Which list that is, for each way of opening the dialog, is the dialog's own spec. + it("exports exactly the operators it was given, whatever the canvas has selected", () => { + enableExport(); + const download = stubDownloadService(); + jointGraphWrapperSpy.getCurrentHighlightedOperatorIDs.mockReturnValue(["opHighlighted"]); + texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "opA" }, { operatorID: "opB" }] as any); + + service.exportWorkflowExecutionResult("csv", "wf", [7], 1, 2, "file", "dataset", makeUnit(), ["opNamed"]); + + expect(download.exportWorkflowResultToDataset).toHaveBeenCalledWith( + "csv", + "workflow1", + "wf", + [{ id: "opNamed", outputType: "csv" }], + [7], + 1, + 2, + "file", + expect.anything() + ); + }); + + // The restriction analysis is asynchronous, and a caller may hand over the canvas selection, + // which is the live array rather than a copy of it. Without a snapshot the export would go + // out with whatever is selected when the analysis answers, not what was asked for. + it("exports the scope it was given, not what that array became while the analysis ran", () => { + enableExport(); + const download = stubDownloadService(); + const pending = new Subject<Record<string, string[]>>(); + downloadServiceSpy.getWorkflowResultDownloadability.mockReturnValue(pending as any); + const liveSelection = ["opA"]; + + service.exportWorkflowExecutionResult("csv", "wf", [7], 1, 2, "file", "dataset", makeUnit(), liveSelection); + liveSelection.push("opB"); + pending.next({}); + pending.complete(); + + expect(download.exportWorkflowResultToDataset).toHaveBeenCalledWith( + "csv", + "workflow1", + "wf", + [{ id: "opA", outputType: "csv" }], + [7], + 1, + 2, + "file", + expect.anything() + ); + }); + + it("exports every operator it was given, in the order it was given them", () => { + enableExport(); + const download = stubDownloadService(); + + service.exportWorkflowExecutionResult("csv", "wf", [7], 1, 2, "file", "dataset", makeUnit(), ["opA", "opB"]); + + expect(download.exportWorkflowResultToDataset).toHaveBeenCalledWith( + "csv", + "workflow1", + "wf", + [ + { id: "opA", outputType: "csv" }, + { id: "opB", outputType: "csv" }, + ], + [7], + 1, + 2, + "file", + expect.anything() + ); + }); + it("errors (no export) when every selected operator is blocked by a non-downloadable dataset", () => { enableExport(); const download = stubDownloadService({ downloadability: { op1: ["ds1 ([email protected])"] } }); texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }] as any); - service.exportWorkflowExecutionResult("csv", "wf", [1], 0, 0, "file", true, "dataset", makeUnit()); + service.exportWorkflowExecutionResult("csv", "wf", [1], 0, 0, "file", "dataset", makeUnit(), ["op1"]); expect(notificationServiceSpy.error).toHaveBeenCalledWith( "Cannot export result: selection depends on dataset(s) that are not downloadable: ds1 ([email protected])" @@ -320,7 +393,8 @@ describe("WorkflowResultExportService", () => { const download = stubDownloadService({ downloadability: { op2: ["ds2 ([email protected])"] } }); texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }, { operatorID: "op2" }] as any); - service.exportWorkflowExecutionResult("csv", "wf", [7], 1, 2, "file", true, "dataset", makeUnit()); + // Both are in scope; only op2 is blocked, which is what makes this the partial case. + service.exportWorkflowExecutionResult("csv", "wf", [7], 1, 2, "file", "dataset", makeUnit(), ["op1", "op2"]); expect(notificationServiceSpy.warning).toHaveBeenCalledWith( "Some operators were skipped because their results depend on dataset(s) that are not downloadable" + @@ -348,7 +422,7 @@ describe("WorkflowResultExportService", () => { const download = stubDownloadService({ downloadability: { op1: [] } }); texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }] as any); - service.exportWorkflowExecutionResult("csv", "wf", [1], 0, 0, "file", true, "dataset", makeUnit()); + service.exportWorkflowExecutionResult("csv", "wf", [1], 0, 0, "file", "dataset", makeUnit(), ["op1"]); expect(notificationServiceSpy.error).toHaveBeenCalledWith( "Cannot export result: selection depends on dataset(s) that are not downloadable" @@ -363,7 +437,8 @@ describe("WorkflowResultExportService", () => { const download = stubDownloadService({ downloadability: { op2: [] } }); texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }, { operatorID: "op2" }] as any); - service.exportWorkflowExecutionResult("csv", "wf", [7], 1, 2, "file", true, "dataset", makeUnit()); + // Both are in scope; only op2 is blocked, which is what makes this the partial case. + service.exportWorkflowExecutionResult("csv", "wf", [7], 1, 2, "file", "dataset", makeUnit(), ["op1", "op2"]); expect(notificationServiceSpy.warning).toHaveBeenCalledWith( "Some operators were skipped because their results depend on dataset(s) that are not downloadable" @@ -389,7 +464,7 @@ describe("WorkflowResultExportService", () => { const download = stubDownloadService(); texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }] as any); - service.exportWorkflowExecutionResult("csv", "wf", [5], 0, 0, "file", true, "dataset", makeUnit()); + service.exportWorkflowExecutionResult("csv", "wf", [5], 0, 0, "file", "dataset", makeUnit(), ["op1"]); expect(notificationServiceSpy.warning).not.toHaveBeenCalled(); expect(notificationServiceSpy.loading).toHaveBeenCalledWith("Exporting..."); @@ -404,7 +479,7 @@ describe("WorkflowResultExportService", () => { }); texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }] as any); - service.exportWorkflowExecutionResult("csv", "wf", [5], 0, 0, "file", true, "dataset", makeUnit()); + service.exportWorkflowExecutionResult("csv", "wf", [5], 0, 0, "file", "dataset", makeUnit(), ["op1"]); expect(notificationServiceSpy.error).toHaveBeenCalledWith("quota exceeded"); expect(notificationServiceSpy.success).not.toHaveBeenCalled(); @@ -415,7 +490,7 @@ describe("WorkflowResultExportService", () => { stubDownloadService({ datasetResponse: new HttpResponse({}) }); texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }] as any); - service.exportWorkflowExecutionResult("csv", "wf", [5], 0, 0, "file", true, "dataset", makeUnit()); + service.exportWorkflowExecutionResult("csv", "wf", [5], 0, 0, "file", "dataset", makeUnit(), ["op1"]); expect(notificationServiceSpy.error).toHaveBeenCalledWith("An error occurred during export"); }); @@ -425,7 +500,7 @@ describe("WorkflowResultExportService", () => { stubDownloadService({ datasetError: { error: { message: "server exploded" } } }); texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }] as any); - service.exportWorkflowExecutionResult("csv", "wf", [5], 0, 0, "file", true, "dataset", makeUnit()); + service.exportWorkflowExecutionResult("csv", "wf", [5], 0, 0, "file", "dataset", makeUnit(), ["op1"]); expect(notificationServiceSpy.error).toHaveBeenCalledWith( "An error happened in exporting operator results: server exploded" @@ -438,7 +513,7 @@ describe("WorkflowResultExportService", () => { stubDownloadService({ datasetError: { error: "dataset quota exceeded" } }); texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }] as any); - service.exportWorkflowExecutionResult("csv", "wf", [5], 0, 0, "file", true, "dataset", makeUnit()); + service.exportWorkflowExecutionResult("csv", "wf", [5], 0, 0, "file", "dataset", makeUnit(), ["op1"]); expect(notificationServiceSpy.error).toHaveBeenCalledWith( "An error happened in exporting operator results: dataset quota exceeded" @@ -451,7 +526,7 @@ describe("WorkflowResultExportService", () => { stubDownloadService({ datasetError: "connection reset by peer" }); texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }] as any); - service.exportWorkflowExecutionResult("csv", "wf", [5], 0, 0, "file", true, "dataset", makeUnit()); + service.exportWorkflowExecutionResult("csv", "wf", [5], 0, 0, "file", "dataset", makeUnit(), ["op1"]); expect(notificationServiceSpy.error).toHaveBeenCalledWith( "An error happened in exporting operator results: connection reset by peer" @@ -463,7 +538,7 @@ describe("WorkflowResultExportService", () => { const download = stubDownloadService(); texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }] as any); - service.exportWorkflowExecutionResult("json", "wf", [], 3, 4, "local-file", true, "local", makeUnit()); + service.exportWorkflowExecutionResult("json", "wf", [], 3, 4, "local-file", "local", makeUnit(), ["op1"]); expect(notificationServiceSpy.loading).toHaveBeenCalledWith("Exporting..."); expect(download.exportWorkflowResultToLocal).toHaveBeenCalledWith( diff --git a/frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.ts b/frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.ts index dad95c5d15..52a04387a6 100644 --- a/frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.ts +++ b/frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.ts @@ -197,12 +197,18 @@ export class WorkflowResultExportService { rowIndex: number, columnIndex: number, filename: string, - exportAll: boolean = false, // if the user click export button on the top bar (a.k.a menu), - // we should export all operators, otherwise, only highlighted ones - // which means export button is selected from context-menu - destination: "dataset" | "local" = "dataset", // default to dataset - unit: DashboardWorkflowComputingUnit | null // computing unit for cluster setting + destination: "dataset" | "local", + unit: DashboardWorkflowComputingUnit | null, // computing unit for cluster setting + // The operators this export covers. The caller resolves them: the dialog already works out + // its own scope in order to report what a blocking dataset blocks, so it says so here rather + // than leaving the scope to be worked out a second time, separately, from a flag and the + // canvas -- two answers to one question that agree only for as long as nobody edits one. + operatorIds: readonly string[] ): void { + // Copied now, not read later: the restriction analysis below is asynchronous, and the canvas + // selection a caller may have handed us is the live array, so the scope would otherwise be + // whatever is selected when the analysis answers rather than what was asked for. + const scope = [...operatorIds]; this.computeRestrictionAnalysis() .pipe(take(1)) .subscribe(restrictionResult => @@ -213,10 +219,10 @@ export class WorkflowResultExportService { rowIndex, columnIndex, filename, - exportAll, destination, unit, - restrictionResult + restrictionResult, + scope ) ); } @@ -226,10 +232,9 @@ export class WorkflowResultExportService { * * This method handles the core export logic: * 1. Validates configuration and computing unit availability - * 2. Determines operator scope (all vs highlighted) - * 3. Applies restriction filtering with user feedback - * 4. Makes the export API call - * 5. Handles response and shows appropriate notifications + * 2. Applies restriction filtering with user feedback + * 3. Makes the export API call + * 4. Handles response and shows appropriate notifications * * Shows error messages if all operators are blocked, warning messages if some are blocked. * @@ -242,10 +247,10 @@ export class WorkflowResultExportService { rowIndex: number, columnIndex: number, filename: string, - exportAll: boolean, destination: "dataset" | "local", unit: DashboardWorkflowComputingUnit | null, - downloadability: WorkflowResultDownloadability + downloadability: WorkflowResultDownloadability, + operatorIds: readonly string[] ): void { // Validates configuration and computing unit availability if (!this.config.env.exportExecutionResultEnabled) { @@ -262,14 +267,6 @@ export class WorkflowResultExportService { return; } - // Determines operator scope - const operatorIds = exportAll - ? this.workflowActionService - .getTexeraGraph() - .getAllOperators() - .map(operator => operator.operatorID) - : [...this.workflowActionService.getJointGraphWrapper().getCurrentHighlightedOperatorIDs()]; - if (operatorIds.length === 0) { return; }
