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-8299-7d57cd6ef1edc45c0c2b8ff73c36bc7eda67a3e1 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 11e3b969a70063d17bfd670b1da85dba100e4339 Author: Tanishq Gandhi <[email protected]> AuthorDate: Mon Aug 31 23:48:46 2026 +0000 feat(frontend): add model sharing, publishing and cover images (#8299) ### What changes were proposed in this PR? Models can now be shared, published and given a cover image. The three surfaces that grew a per-type branch for each new resource kind move onto the resource-descriptor registry instead, so the model arm is a descriptor rather than another `if (type === ...)`. **Refactor:** `ResourceDescriptor` gains `isPublic`/`setPublished`, `coverUrl`/`setCover` and `affordances`. `ShareAccessComponent`'s four publish/unpublish methods become one `setPublished(next)`, and it stops injecting `WorkflowPersistService` and `DatasetService`. `card-item` and `list-item` build one share-modal config instead of two and gate the Share button on the descriptor, which retires the dead button on file cards. `browse-section` asks the descriptor for a cover instead of concatenating a URL, so a dataset's cover is the same picture in the hub as on its card, and it no longer needs a per-kind branch to add one. **Feature:** `ModelService` gains publicity, downloadable, cover and owners calls; the model detail page gets an **Access & visibility** card; and the file tree's existing "Set as cover" control is wired up. Four deliberate behavior changes, worth a reviewer's attention: - Publishing a **dataset or model** no longer warns about "the right to clone your work". Only workflows are clonable — `cloneWorkflow` is the only clone in the codebase, and `cloneCount` is hardcoded to 0 for every other kind — so datasets were being warned about a capability that does not exist. - The share modal's Private card read `this workflow {{ type }}`, rendering "this workflow dataset". The stray word is removed. - A `file`-typed share email no longer appends a broken `<origin>/<id>` link, since the file descriptor has no private route. - **Publishing and download permission now report the state the server ended up in, not the one that was requested.** Both endpoints toggle rather than set, so a surface holding a stale value could ask to publish and silently unpublish instead, while the switch and the toast claimed success — and for downloads that value also gates the download buttons through `isDownloadAllowed()`. `ShareAccessComponent.setPublished` and both switches on the model page now read the flag back after the write and render that, and `setPublished(id, next)` documents that `next` is only what the caller asked for. A read-back that itself fails keeps the requested value and still reports success: `GET /model/{mid}` sizes the repository through `withLakeFSErrorHandling` and so can fail where `/model/list` (which falls back to `0`) would not, and reporting that as a failure invited a retry that toggled the resource straight back. Knock-on effects: a failed publish inside the workspace no longer leaves a false published badge on the canvas; the toast names the real outcome; and the Settings hint text now updates on click rather than after the round trip. That last one is load-bearing, not cosmetic — the switch value is written optimistically so the correction is a real change, since with one-way `[ngModel]` writing the value the field already holds never reaches the control. Published models are not discoverable yet: `listModels` passes `includePublic = false` and models have no hub surface, so publishing makes a model readable by direct URL only. Discovery arrives with the hub PR. <img width="1238" height="672" alt="Screenshot 2026-08-31 at 11 37 25 AM" src="https://github.com/user-attachments/assets/5786cfca-9582-4213-a2fb-165146ee41aa"; /> <img width="910" height="525" alt="Screenshot 2026-08-31 at 11 37 35 AM" src="https://github.com/user-attachments/assets/a0865622-b80f-4375-b406-4a48908e3178"; /> <img width="695" height="806" alt="Screenshot 2026-08-31 at 11 41 34 AM" src="https://github.com/user-attachments/assets/3fa68e48-d974-4ac2-8847-e9f42cef4919"; /> <img width="770" height="249" alt="Screenshot 2026-08-31 at 11 42 05 AM" src="https://github.com/user-attachments/assets/31a5d4f1-7720-4356-b483-3966249dd59c"; /> ### Any related issues, documentation, discussions? Part of #6501. Fourth in the model frontend series. ### How was this PR tested? New tests: model publicity/downloadable/cover/owners requests in `model.service.spec`; both toggles with a rejected-request rollback, cover path prefixing and the no-version guard in `model-detail.component.spec`; descriptor slot presence, the `clonable` affordance and model delegation in `resource-registry.service.spec`; and the clone-warning copy in `share-access.component.spec`. The share-access publish tests moved onto `setPublished`, and the browse-section cover specs now assert the resolved URL rather than a concatenated one. For the read-back above, the share-access doubles became stateful — the workflow endpoint sets absolutely, the dataset one toggles — so a read after a write sees the new value rather than a constant. Both surfaces pin the stale case directly: a modal that loaded a private dataset, published elsewhere in the meantime, must report *unpublished* after asking to publish. Also covered on both: a landed toggle whose read-back throws still reports success, and a failed toggle rolls the switch back. The model page's stale case is asserted against the **rendered switch**, not just the field, since that is where it would surface. Two details make that assertion real rather than vacuous: the read-back is deferred through a `Subject`, because a synchronous one collapses both writes into a single change-detection pass, and `NgModel` pushes to the control in a microtask, so the test flushes one before reading the DOM. 2752 tests pass across `dashboard`, `hub`, `common` and the workspace menu/power-button specs. `tsc --noEmit` and Prettier are clean. Manually verified: sharing a model with a second user (grant, level change, revoke), publishing from both the share modal and the Settings tab, the downloadable toggle as owner and as a collaborator, setting a cover from a committed image and seeing it on the detail page and the model card, and regression checks on dataset and workflow sharing, publishing and covers including the workspace's in-canvas publish state. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5) --- .../list-item/card-item/card-item.component.html | 1 + .../card-item/card-item.component.spec.ts | 3 +- .../list-item/card-item/card-item.component.ts | 87 ++++------ .../user/list-item/list-item.component.html | 1 + .../user/list-item/list-item.component.spec.ts | 2 +- .../user/list-item/list-item.component.ts | 65 +++----- .../user/share-access/share-access.component.html | 4 +- .../share-access/share-access.component.spec.ts | 181 ++++++++++++++------- .../user/share-access/share-access.component.ts | 164 ++++++------------- .../model-detail.component.html | 44 ++++- .../model-detail.component.spec.ts | 133 ++++++++++++++- .../user-model-explorer/model-detail.component.ts | 91 ++++++++++- .../service/user/model/model.service.spec.ts | 26 +++ .../dashboard/service/user/model/model.service.ts | 33 ++++ .../dataset-resource.descriptor.ts | 10 +- .../resource-registry/model-resource.descriptor.ts | 12 +- .../resource-registry.service.spec.ts | 47 +++++- .../resource-registry/resource-registry.service.ts | 5 + .../workflow-resource.descriptor.ts | 9 +- .../src/app/dashboard/type/resource-descriptor.ts | 22 ++- .../browse-section.component.spec.ts | 21 ++- .../browse-section/browse-section.component.ts | 38 +++-- 22 files changed, 695 insertions(+), 304 deletions(-) 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 3846055a1f..383cc2ef95 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 @@ -211,6 +211,7 @@ nzType="text" class="action-btn" title="Share" + *ngIf="canShare" (click)="onClickOpenShareAccess(); $event.stopPropagation()"> <i nz-icon diff --git a/frontend/src/app/dashboard/component/user/list-item/card-item/card-item.component.spec.ts b/frontend/src/app/dashboard/component/user/list-item/card-item/card-item.component.spec.ts index e1410a6b0b..255e40b0d2 100644 --- a/frontend/src/app/dashboard/component/user/list-item/card-item/card-item.component.spec.ts +++ b/frontend/src/app/dashboard/component/user/list-item/card-item/card-item.component.spec.ts @@ -705,7 +705,7 @@ describe("CardItemComponent", () => { .spyOn(modalService, "create") .mockReturnValue({ componentInstance: { refresh: refresh$ } } as any); (workflowPersistService as any).retrieveOwners = vi.fn().mockReturnValue(of(["alice", "bob"])); - component.entry = makeWorkflowEntry({ id: 7, workflow: { isOwner: true, accessLevel: "WRITE" } } as any); + component.entry = makeWorkflowEntry({ id: 7, accessLevel: "WRITE", workflow: { isOwner: true } } as any); await component.onClickOpenShareAccess(); @@ -744,6 +744,7 @@ describe("CardItemComponent", () => { type: "dataset", id: 5, allOwners: ["carol"], + inWorkspace: false, }); expect(cfg.nzTitle).toBe("Share this dataset with others"); }); diff --git a/frontend/src/app/dashboard/component/user/list-item/card-item/card-item.component.ts b/frontend/src/app/dashboard/component/user/list-item/card-item/card-item.component.ts index 5b3dfe16a5..03cf31147e 100644 --- a/frontend/src/app/dashboard/component/user/list-item/card-item/card-item.component.ts +++ b/frontend/src/app/dashboard/component/user/list-item/card-item/card-item.component.ts @@ -38,17 +38,15 @@ import { NzCheckboxComponent } from "ng-zorro-antd/checkbox"; import { NzIconDirective } from "ng-zorro-antd/icon"; import { NzPopconfirmDirective } from "ng-zorro-antd/popconfirm"; import { ɵNzTransitionPatchDirective } from "ng-zorro-antd/core/transition-patch"; -import { NzModalRef, NzModalService } from "ng-zorro-antd/modal"; +import { NzModalService } from "ng-zorro-antd/modal"; import { DashboardEntry } from "src/app/dashboard/type/dashboard-entry"; import { ShareAccessComponent } from "../../share-access/share-access.component"; import { UserAvatarComponent } from "../../user-avatar/user-avatar.component"; -import { WorkflowPersistService } from "src/app/common/service/workflow-persist/workflow-persist.service"; -import { firstValueFrom } from "rxjs"; +import { firstValueFrom, Observable } from "rxjs"; import { HubWorkflowDetailComponent } from "../../../../../hub/component/workflow/detail/hub-workflow-detail.component"; import { ActionType, HubService } from "../../../../../hub/service/hub.service"; import { formatSize } from "src/app/common/util/size-formatter.util"; import { formatRelativeTime, formatCount } from "src/app/common/util/format.util"; -import { DatasetService } from "../../../../service/user/dataset/dataset.service"; import { NotificationService } from "../../../../../common/service/notification/notification.service"; import { extractErrorMessage } from "../../../../../common/util/error"; import { WorkflowCoverService } from "../../../../service/user/workflow-cover/workflow-cover.service"; @@ -95,7 +93,9 @@ export class CardItemComponent implements OnChanges { hovering: boolean = false; /** The default top image, used when the user has not uploaded a custom one. */ static readonly DEFAULT_PREVIEW_IMAGE = "assets/card_background.jpg"; - /** Resolved preview/cover image; stays the placeholder until a dataset cover loads. */ + /** Whether this kind can be shared at all; the button is hidden when it cannot. */ + public canShare: boolean = false; + /** Resolved preview/cover image; stays the placeholder until a file-backed cover loads. */ coverImageSrc: string = CardItemComponent.DEFAULT_PREVIEW_IMAGE; /** The workflow's custom cover image data URL, if one has been set. */ @@ -121,8 +121,6 @@ export class CardItemComponent implements OnChanges { constructor( private modalService: NzModalService, - private workflowPersistService: WorkflowPersistService, - private datasetService: DatasetService, private modal: NzModalService, private hubService: HubService, private cdr: ChangeDetectorRef, @@ -191,16 +189,17 @@ export class CardItemComponent implements OnChanges { this.iconType = descriptor.iconType; this.disableDelete = !descriptor.isOwner(this.entry); this.canDownload = descriptor.download !== undefined; + this.canShare = descriptor.retrieveOwners !== undefined; this.entryLink = this.resourceRegistry.entryLink(this.entry, this.currentUid); if (descriptor.hasSize && typeof this.entry.id === "number") { this.size = this.entry.size; } - // Covers are still per-type; they move onto the descriptor with the rest of the sharing surface. + // A workflow cover is a data URL already on the entry; the file-backed kinds have to fetch one. if (this.entry.type === "workflow") { this.coverImageSrc = this.entry.coverImageUrl ?? CardItemComponent.DEFAULT_PREVIEW_IMAGE; this.customImage = this.entry.coverImageUrl ?? undefined; - } else if (this.entry.type === "dataset" && typeof this.entry.id === "number") { - this.loadDatasetCover(this.entry.id); + } else if (descriptor.coverUrl && this.entry.coverImageUrl && typeof this.entry.id === "number") { + this.loadCover(descriptor.coverUrl, this.entry.id); } this.likeCount = this.entry.likeCount; this.viewCount = this.entry.viewCount; @@ -213,16 +212,12 @@ export class CardItemComponent implements OnChanges { } } - /** Loads the dataset cover into the preview slot, falling back to the placeholder. */ - private loadDatasetCover(did: number): void { - if (!this.entry.coverImageUrl) { - return; - } - this.datasetService - .getDatasetCoverUrl(did) + /** Loads a file-backed cover into the preview slot, falling back to the placeholder. */ + private loadCover(coverUrl: (id: number) => Observable<string | null>, id: number): void { + coverUrl(id) .pipe(untilDestroyed(this)) .subscribe({ - next: ({ url }) => { + next: url => { this.coverImageSrc = url ?? CardItemComponent.DEFAULT_PREVIEW_IMAGE; this.cdr.markForCheck(); }, @@ -245,43 +240,27 @@ export class CardItemComponent implements OnChanges { } public async onClickOpenShareAccess(): Promise<void> { - let modal: NzModalRef<ShareAccessComponent> | undefined; - - if (this.entry.type === "workflow") { - modal = this.modalService.create({ - nzContent: ShareAccessComponent, - nzData: { - writeAccess: this.entry.workflow.accessLevel === "WRITE", - type: this.entry.type, - id: this.entry.id, - allOwners: await firstValueFrom(this.workflowPersistService.retrieveOwners()), - inWorkspace: false, - }, - nzFooter: null, - nzTitle: "Share this workflow with others", - nzCentered: true, - nzWidth: "700px", - }); - } else if (this.entry.type === "dataset") { - modal = this.modalService.create({ - nzContent: ShareAccessComponent, - nzData: { - writeAccess: this.entry.accessLevel === "WRITE", - type: "dataset", - id: this.entry.id, - allOwners: await firstValueFrom(this.datasetService.retrieveOwners()), - }, - nzFooter: null, - nzTitle: "Share this dataset with others", - nzCentered: true, - nzWidth: "700px", - }); - } - if (modal) { - modal.componentInstance?.refresh.pipe(untilDestroyed(this)).subscribe(() => { - this.refresh.emit(); - }); + const retrieveOwners = this.resourceRegistry.get(this.entry.type).retrieveOwners; + if (!retrieveOwners) { + return; } + const modal = this.modalService.create({ + nzContent: ShareAccessComponent, + nzData: { + writeAccess: this.entry.accessLevel === "WRITE", + type: this.entry.type, + id: this.entry.id, + allOwners: await firstValueFrom(retrieveOwners()), + inWorkspace: false, + }, + nzFooter: null, + nzTitle: `Share this ${this.entry.type} with others`, + nzCentered: true, + nzWidth: "700px", + }); + modal.componentInstance?.refresh.pipe(untilDestroyed(this)).subscribe(() => { + this.refresh.emit(); + }); } public onClickDownload = (): void => { diff --git a/frontend/src/app/dashboard/component/user/list-item/list-item.component.html b/frontend/src/app/dashboard/component/user/list-item/list-item.component.html index a223afe6e4..7e67b6438e 100644 --- a/frontend/src/app/dashboard/component/user/list-item/list-item.component.html +++ b/frontend/src/app/dashboard/component/user/list-item/list-item.component.html @@ -198,6 +198,7 @@ nz-button nzType="text" title="Share" + *ngIf="canShare" (click)="onClickOpenShareAccess()"> <i nz-icon diff --git a/frontend/src/app/dashboard/component/user/list-item/list-item.component.spec.ts b/frontend/src/app/dashboard/component/user/list-item/list-item.component.spec.ts index 1482ca3429..e520343ce4 100644 --- a/frontend/src/app/dashboard/component/user/list-item/list-item.component.spec.ts +++ b/frontend/src/app/dashboard/component/user/list-item/list-item.component.spec.ts @@ -436,7 +436,7 @@ describe("ListItemComponent", () => { (workflowPersistService as any).retrieveOwners = vi.fn().mockReturnValue(of([])); let refreshed = false; component.refresh.subscribe(() => (refreshed = true)); - feed(entryOf({ type: "workflow", workflow: { isOwner: true, accessLevel: "WRITE" } })); + feed(entryOf({ type: "workflow", accessLevel: "WRITE", workflow: { isOwner: true } })); await component.onClickOpenShareAccess(); diff --git a/frontend/src/app/dashboard/component/user/list-item/list-item.component.ts b/frontend/src/app/dashboard/component/user/list-item/list-item.component.ts index 4e7227e6ea..2226f2e1db 100644 --- a/frontend/src/app/dashboard/component/user/list-item/list-item.component.ts +++ b/frontend/src/app/dashboard/component/user/list-item/list-item.component.ts @@ -29,17 +29,15 @@ import { } from "@angular/core"; import { Component } from "@angular/core"; import { UntilDestroy, untilDestroyed } from "@ngneat/until-destroy"; -import { NzModalRef, NzModalService } from "ng-zorro-antd/modal"; +import { NzModalService } from "ng-zorro-antd/modal"; import { DashboardEntry } from "src/app/dashboard/type/dashboard-entry"; import { MarkdownDescriptionComponent } from "../markdown-description/markdown-description.component"; import { ShareAccessComponent } from "../share-access/share-access.component"; -import { WorkflowPersistService } from "src/app/common/service/workflow-persist/workflow-persist.service"; import { firstValueFrom } from "rxjs"; import { HubWorkflowDetailComponent } from "../../../../hub/component/workflow/detail/hub-workflow-detail.component"; import { ActionType, HubService } from "../../../../hub/service/hub.service"; import { formatSize } from "src/app/common/util/size-formatter.util"; import { formatCount, formatRelativeTime } from "src/app/common/util/format.util"; -import { DatasetService } from "../../../service/user/dataset/dataset.service"; import { NotificationService } from "../../../../common/service/notification/notification.service"; import { extractErrorMessage } from "../../../../common/util/error"; import { isDefined } from "../../../../common/util/predicate"; @@ -84,6 +82,8 @@ export class ListItemComponent implements OnChanges { public originalDescription: string | undefined = undefined; public disableDelete: boolean = false; public canDownload: boolean = false; + /** Whether this kind can be shared at all; the button is hidden when it cannot. */ + public canShare: boolean = false; @Input() currentUid: number | undefined; @ViewChild("nameInput") nameInput!: ElementRef; @ViewChild("descriptionInput") descriptionInput!: ElementRef; @@ -121,8 +121,6 @@ export class ListItemComponent implements OnChanges { constructor( private modalService: NzModalService, - private workflowPersistService: WorkflowPersistService, - private datasetService: DatasetService, private modal: NzModalService, private hubService: HubService, private cdr: ChangeDetectorRef, @@ -135,6 +133,7 @@ export class ListItemComponent implements OnChanges { this.iconType = descriptor.iconType; this.disableDelete = !descriptor.isOwner(this.entry); this.canDownload = descriptor.download !== undefined; + this.canShare = descriptor.retrieveOwners !== undefined; this.entryLink = this.resourceRegistry.entryLink(this.entry, this.currentUid); if (descriptor.hasSize && typeof this.entry.id === "number") { this.size = this.entry.size; @@ -171,43 +170,27 @@ export class ListItemComponent implements OnChanges { } public async onClickOpenShareAccess(): Promise<void> { - let modal: NzModalRef<ShareAccessComponent> | undefined; - - if (this.entry.type === "workflow") { - modal = this.modalService.create({ - nzContent: ShareAccessComponent, - nzData: { - writeAccess: this.entry.workflow.accessLevel === "WRITE", - type: this.entry.type, - id: this.entry.id, - allOwners: await firstValueFrom(this.workflowPersistService.retrieveOwners()), - inWorkspace: false, - }, - nzFooter: null, - nzTitle: "Share this workflow with others", - nzCentered: true, - nzWidth: "700px", - }); - } else if (this.entry.type === "dataset") { - modal = this.modalService.create({ - nzContent: ShareAccessComponent, - nzData: { - writeAccess: this.entry.accessLevel === "WRITE", - type: "dataset", - id: this.entry.id, - allOwners: await firstValueFrom(this.datasetService.retrieveOwners()), - }, - nzFooter: null, - nzTitle: "Share this dataset with others", - nzCentered: true, - nzWidth: "700px", - }); - } - if (modal) { - modal.componentInstance?.refresh.pipe(untilDestroyed(this)).subscribe(() => { - this.refresh.emit(); - }); + const retrieveOwners = this.resourceRegistry.get(this.entry.type).retrieveOwners; + if (!retrieveOwners) { + return; } + const modal = this.modalService.create({ + nzContent: ShareAccessComponent, + nzData: { + writeAccess: this.entry.accessLevel === "WRITE", + type: this.entry.type, + id: this.entry.id, + allOwners: await firstValueFrom(retrieveOwners()), + inWorkspace: false, + }, + nzFooter: null, + nzTitle: `Share this ${this.entry.type} with others`, + nzCentered: true, + nzWidth: "700px", + }); + modal.componentInstance?.refresh.pipe(untilDestroyed(this)).subscribe(() => { + this.refresh.emit(); + }); } public onClickDownload = (): void => { diff --git a/frontend/src/app/dashboard/component/user/share-access/share-access.component.html b/frontend/src/app/dashboard/component/user/share-access/share-access.component.html index 2c155f1a5a..8128f33363 100644 --- a/frontend/src/app/dashboard/component/user/share-access/share-access.component.html +++ b/frontend/src/app/dashboard/component/user/share-access/share-access.component.html @@ -19,7 +19,7 @@ <div class="access-button-group" - *ngIf="isPublic !== null && type !== 'computing-unit'"> + *ngIf="isPublic !== null"> <button nz-button class="access-button" @@ -33,7 +33,7 @@ class="button-icon"></div> <div class="button-text"> <div class="button-text-header">Private</div> - <p>Only collaborators can view this workflow {{ type }}</p> + <p>Only collaborators can view this {{ type }}</p> </div> </button> diff --git a/frontend/src/app/dashboard/component/user/share-access/share-access.component.spec.ts b/frontend/src/app/dashboard/component/user/share-access/share-access.component.spec.ts index d11be498a2..202465b25f 100644 --- a/frontend/src/app/dashboard/component/user/share-access/share-access.component.spec.ts +++ b/frontend/src/app/dashboard/component/user/share-access/share-access.component.spec.ts @@ -66,6 +66,10 @@ describe("ShareAccessComponent", () => { }; let workflowActionSpy: { setWorkflowIsPublished: ReturnType<typeof vi.fn> }; let userServiceCurrentEmail: string | undefined; + // The component reads publicity back after writing it, so the doubles have to hold state: + // the workflow endpoint sets it absolutely, the dataset one toggles. + let workflowPublished: boolean; + let datasetPublished: boolean; let capturedModalConfigs: any[]; /** The NzModalRef stubs handed back by modalService.create, in creation order. */ let capturedModalRefs: { close: ReturnType<typeof vi.fn> }[]; @@ -129,13 +133,21 @@ describe("ShareAccessComponent", () => { return ref; }), }; + workflowPublished = false; + datasetPublished = false; workflowPersistSpy = { - getWorkflowIsPublished: vi.fn().mockReturnValue(of("Private")), - updateWorkflowIsPublished: vi.fn().mockReturnValue(of(null)), + getWorkflowIsPublished: vi.fn(() => of(workflowPublished ? "Public" : "Private")), + updateWorkflowIsPublished: vi.fn((_id: number, next: boolean) => { + workflowPublished = next; + return of(null); + }), }; datasetServiceSpy = { - getDataset: vi.fn().mockReturnValue(of({ dataset: { isPublic: false } })), - updateDatasetPublicity: vi.fn().mockReturnValue(of(null)), + getDataset: vi.fn(() => of({ dataset: { isPublic: datasetPublished } })), + updateDatasetPublicity: vi.fn(() => { + datasetPublished = !datasetPublished; + return of(null); + }), }; workflowActionSpy = { setWorkflowIsPublished: vi.fn() }; }); @@ -157,20 +169,20 @@ describe("ShareAccessComponent", () => { }); it("loads publish state for workflow via WorkflowPersistService", () => { - workflowPersistSpy.getWorkflowIsPublished.mockReturnValue(of("Public")); + workflowPublished = true; const c = setupComponent({ type: "workflow", id: 9 }); expect(workflowPersistSpy.getWorkflowIsPublished).toHaveBeenCalledWith(9); expect(c.isPublic).toBe(true); }); it("sets isPublic to false when workflow publish state is Private", () => { - workflowPersistSpy.getWorkflowIsPublished.mockReturnValue(of("Private")); + workflowPublished = false; const c = setupComponent({ type: "workflow" }); expect(c.isPublic).toBe(false); }); it("loads publish state for dataset via DatasetService.getDataset", () => { - datasetServiceSpy.getDataset.mockReturnValue(of({ dataset: { isPublic: true } })); + datasetPublished = true; const c = setupComponent({ type: "dataset", id: 12 }); expect(datasetServiceSpy.getDataset).toHaveBeenCalledWith(12); expect(c.isPublic).toBe(true); @@ -411,7 +423,7 @@ describe("ShareAccessComponent", () => { describe("verifyPublish / verifyUnpublish", () => { it("publishes a workflow on confirm and updates the action service when inWorkspace", () => { - workflowPersistSpy.getWorkflowIsPublished.mockReturnValue(of("Private")); + workflowPublished = false; const c = setupComponent({ type: "workflow", id: 8, inWorkspace: true }); c.verifyPublish(); getFooterButton(capturedModalConfigs[0], "Publish").onClick(); @@ -420,7 +432,7 @@ describe("ShareAccessComponent", () => { }); it("does not call WorkflowActionService.setWorkflowIsPublished when not inWorkspace", () => { - workflowPersistSpy.getWorkflowIsPublished.mockReturnValue(of("Private")); + workflowPublished = false; const c = setupComponent({ type: "workflow", id: 8, inWorkspace: false }); c.verifyPublish(); getFooterButton(capturedModalConfigs[0], "Publish").onClick(); @@ -428,22 +440,35 @@ describe("ShareAccessComponent", () => { }); it("publishes a dataset on confirm", () => { - datasetServiceSpy.getDataset.mockReturnValue(of({ dataset: { isPublic: false } })); + datasetPublished = false; const c = setupComponent({ type: "dataset", id: 9 }); c.verifyPublish(); getFooterButton(capturedModalConfigs[0], "Publish").onClick(); expect(datasetServiceSpy.updateDatasetPublicity).toHaveBeenCalledWith(9); }); + it("warns about cloning only for a clonable kind", () => { + workflowPublished = false; + setupComponent({ type: "workflow" }).verifyPublish(); + expect(capturedModalConfigs[0].nzContent).toContain("the right to clone your work"); + + TestBed.resetTestingModule(); + capturedModalConfigs = []; + datasetPublished = false; + setupComponent({ type: "dataset" }).verifyPublish(); + expect(capturedModalConfigs[0].nzContent).toContain("read access"); + expect(capturedModalConfigs[0].nzContent).not.toContain("clone"); + }); + it("does not open the publish modal when the item is already public", () => { - workflowPersistSpy.getWorkflowIsPublished.mockReturnValue(of("Public")); + workflowPublished = true; const c = setupComponent({ type: "workflow" }); c.verifyPublish(); expect(modalServiceSpy.create).not.toHaveBeenCalled(); }); it("unpublishes a workflow on confirm and updates the action service when inWorkspace", () => { - workflowPersistSpy.getWorkflowIsPublished.mockReturnValue(of("Public")); + workflowPublished = true; const c = setupComponent({ type: "workflow", id: 8, inWorkspace: true }); c.verifyUnpublish(); getFooterButton(capturedModalConfigs[0], "Unpublish").onClick(); @@ -452,7 +477,7 @@ describe("ShareAccessComponent", () => { }); it("unpublishes a dataset on confirm", () => { - datasetServiceSpy.getDataset.mockReturnValue(of({ dataset: { isPublic: true } })); + datasetPublished = true; const c = setupComponent({ type: "dataset", id: 9 }); c.verifyUnpublish(); getFooterButton(capturedModalConfigs[0], "Unpublish").onClick(); @@ -460,65 +485,107 @@ describe("ShareAccessComponent", () => { }); it("does not open the unpublish modal when the item is already private", () => { - workflowPersistSpy.getWorkflowIsPublished.mockReturnValue(of("Private")); + workflowPublished = false; const c = setupComponent({ type: "workflow" }); c.verifyUnpublish(); expect(modalServiceSpy.create).not.toHaveBeenCalled(); }); }); - describe("publish / unpublish methods", () => { - it("publishWorkflow flips isPublic and shows a success notification", () => { - workflowPersistSpy.getWorkflowIsPublished.mockReturnValue(of("Private")); + describe("setPublished", () => { + it("publishing a workflow flips isPublic and shows a success notification", () => { + workflowPublished = false; const c = setupComponent({ type: "workflow" }); - c.publishWorkflow(); + c.setPublished(true); expect(c.isPublic).toBe(true); expect(notificationSpy.success).toHaveBeenCalledWith("Workflow published successfully"); }); - it("publishWorkflow surfaces HttpErrorResponse via NotificationService.error", () => { - workflowPersistSpy.getWorkflowIsPublished.mockReturnValue(of("Private")); + it("a failed workflow publish surfaces HttpErrorResponse via NotificationService.error", () => { + workflowPublished = false; workflowPersistSpy.updateWorkflowIsPublished.mockReturnValue( throwError(() => new HttpErrorResponse({ error: { message: "publish failed" }, status: 500 })) ); const c = setupComponent({ type: "workflow" }); - c.publishWorkflow(); + c.setPublished(true); expect(notificationSpy.error).toHaveBeenCalledWith("publish failed"); }); - it("unpublishWorkflow flips isPublic to false and shows a success notification", () => { - workflowPersistSpy.getWorkflowIsPublished.mockReturnValue(of("Public")); + it("unpublishing a workflow flips isPublic to false and shows a success notification", () => { + workflowPublished = true; const c = setupComponent({ type: "workflow" }); - c.unpublishWorkflow(); + c.setPublished(false); expect(c.isPublic).toBe(false); expect(notificationSpy.success).toHaveBeenCalledWith("Workflow unpublished successfully"); }); - it("publishDataset flips isPublic and shows a success notification", () => { - datasetServiceSpy.getDataset.mockReturnValue(of({ dataset: { isPublic: false } })); + it("publishing a dataset flips isPublic and shows a success notification", () => { + datasetPublished = false; const c = setupComponent({ type: "dataset" }); - c.publishDataset(); + c.setPublished(true); expect(c.isPublic).toBe(true); expect(notificationSpy.success).toHaveBeenCalledWith("Dataset published successfully"); }); - it("publishDataset surfaces HttpErrorResponse via NotificationService.error", () => { - datasetServiceSpy.getDataset.mockReturnValue(of({ dataset: { isPublic: false } })); + it("a failed dataset publish surfaces HttpErrorResponse via NotificationService.error", () => { + datasetPublished = false; datasetServiceSpy.updateDatasetPublicity.mockReturnValue( throwError(() => new HttpErrorResponse({ error: { message: "dataset publish failed" }, status: 500 })) ); const c = setupComponent({ type: "dataset" }); - c.publishDataset(); + c.setPublished(true); expect(notificationSpy.error).toHaveBeenCalledWith("dataset publish failed"); }); - it("unpublishDataset flips isPublic to false and shows a success notification", () => { - datasetServiceSpy.getDataset.mockReturnValue(of({ dataset: { isPublic: true } })); + it("unpublishing a dataset flips isPublic to false and shows a success notification", () => { + datasetPublished = true; const c = setupComponent({ type: "dataset" }); - c.unpublishDataset(); + c.setPublished(false); expect(c.isPublic).toBe(false); expect(notificationSpy.success).toHaveBeenCalledWith("Dataset unpublished successfully"); }); + + it("reports what the server has, not what was asked for, when the toggle disagrees", () => { + datasetPublished = false; + const c = setupComponent({ type: "dataset", id: 9 }); + datasetPublished = true; + + c.setPublished(true); + + expect(c.isPublic).toBe(false); + expect(notificationSpy.success).toHaveBeenCalledWith("Dataset unpublished successfully"); + }); + + it("keeps a landed publish when the read-back fails", () => { + // The write succeeded; only the confirming read broke. Calling that a failure would invite a + // retry, and the retry would toggle the resource straight back. + datasetPublished = false; + const c = setupComponent({ type: "dataset", id: 9 }); + datasetServiceSpy.getDataset.mockReturnValue( + throwError(() => new HttpErrorResponse({ error: { message: "lakefs down" }, status: 500 })) + ); + + c.setPublished(true); + + expect(datasetServiceSpy.updateDatasetPublicity).toHaveBeenCalledWith(9); + expect(c.isPublic).toBe(true); + expect(notificationSpy.success).toHaveBeenCalledWith("Dataset published successfully"); + expect(notificationSpy.error).not.toHaveBeenCalled(); + }); + + it("does nothing for a registered kind that cannot be published", () => { + const c = setupComponent({ type: "file", id: 4 }); + c.setPublished(true); + expect(c.isPublic).toBeNull(); + expect(notificationSpy.success).not.toHaveBeenCalled(); + }); + + it("does nothing for a kind the registry does not carry at all", () => { + const c = setupComponent({ type: "computing-unit", id: 4 }); + c.setPublished(true); + expect(c.isPublic).toBeNull(); + expect(notificationSpy.success).not.toHaveBeenCalled(); + }); }); describe("hasWriteAccess without a resolved email", () => { @@ -615,7 +682,7 @@ describe("ShareAccessComponent", () => { }); it("closes the publish modal without publishing when Cancel is clicked", () => { - workflowPersistSpy.getWorkflowIsPublished.mockReturnValue(of("Private")); + workflowPublished = false; const modalRefs = captureModalRefs(); const c = setupComponent({ type: "workflow", inWorkspace: true }); c.verifyPublish(); @@ -626,7 +693,7 @@ describe("ShareAccessComponent", () => { }); it("closes the unpublish modal without unpublishing when Cancel is clicked", () => { - workflowPersistSpy.getWorkflowIsPublished.mockReturnValue(of("Public")); + workflowPublished = true; const modalRefs = captureModalRefs(); const c = setupComponent({ type: "workflow" }); c.verifyUnpublish(); @@ -651,24 +718,24 @@ describe("ShareAccessComponent", () => { }); describe("unpublish error branches", () => { - it("unpublishWorkflow surfaces HttpErrorResponse and leaves isPublic unchanged", () => { - workflowPersistSpy.getWorkflowIsPublished.mockReturnValue(of("Public")); + it("a failed workflow unpublish surfaces HttpErrorResponse and leaves isPublic unchanged", () => { + workflowPublished = true; workflowPersistSpy.updateWorkflowIsPublished.mockReturnValue( throwError(() => new HttpErrorResponse({ error: { message: "unpublish failed" }, status: 500 })) ); const c = setupComponent({ type: "workflow" }); - c.unpublishWorkflow(); + c.setPublished(false); expect(notificationSpy.error).toHaveBeenCalledWith("unpublish failed"); expect(c.isPublic).toBe(true); }); - it("unpublishDataset surfaces HttpErrorResponse and leaves isPublic unchanged", () => { - datasetServiceSpy.getDataset.mockReturnValue(of({ dataset: { isPublic: true } })); + it("a failed dataset unpublish surfaces HttpErrorResponse and leaves isPublic unchanged", () => { + datasetPublished = true; datasetServiceSpy.updateDatasetPublicity.mockReturnValue( throwError(() => new HttpErrorResponse({ error: { message: "dataset unpublish failed" }, status: 500 })) ); const c = setupComponent({ type: "dataset" }); - c.unpublishDataset(); + c.setPublished(false); expect(notificationSpy.error).toHaveBeenCalledWith("dataset unpublish failed"); expect(c.isPublic).toBe(true); }); @@ -692,35 +759,35 @@ describe("ShareAccessComponent", () => { expect(gmailSpy.sendEmail).not.toHaveBeenCalled(); }); - it("publishWorkflow is a no-op when the workflow is already public", () => { - workflowPersistSpy.getWorkflowIsPublished.mockReturnValue(of("Public")); + it("publishing is a no-op when the workflow is already public", () => { + workflowPublished = true; const c = setupComponent({ type: "workflow" }); workflowPersistSpy.updateWorkflowIsPublished.mockClear(); - c.publishWorkflow(); + c.setPublished(true); expect(workflowPersistSpy.updateWorkflowIsPublished).not.toHaveBeenCalled(); }); - it("unpublishWorkflow is a no-op when the workflow is already private", () => { - workflowPersistSpy.getWorkflowIsPublished.mockReturnValue(of("Private")); + it("unpublishing is a no-op when the workflow is already private", () => { + workflowPublished = false; const c = setupComponent({ type: "workflow" }); workflowPersistSpy.updateWorkflowIsPublished.mockClear(); - c.unpublishWorkflow(); + c.setPublished(false); expect(workflowPersistSpy.updateWorkflowIsPublished).not.toHaveBeenCalled(); }); - it("publishDataset is a no-op when the dataset is already public", () => { - datasetServiceSpy.getDataset.mockReturnValue(of({ dataset: { isPublic: true } })); + it("publishing is a no-op when the dataset is already public", () => { + datasetPublished = true; const c = setupComponent({ type: "dataset" }); datasetServiceSpy.updateDatasetPublicity.mockClear(); - c.publishDataset(); + c.setPublished(true); expect(datasetServiceSpy.updateDatasetPublicity).not.toHaveBeenCalled(); }); - it("unpublishDataset is a no-op when the dataset is already private", () => { - datasetServiceSpy.getDataset.mockReturnValue(of({ dataset: { isPublic: false } })); + it("unpublishing is a no-op when the dataset is already private", () => { + datasetPublished = false; const c = setupComponent({ type: "dataset" }); datasetServiceSpy.updateDatasetPublicity.mockClear(); - c.unpublishDataset(); + c.setPublished(false); expect(datasetServiceSpy.updateDatasetPublicity).not.toHaveBeenCalled(); }); }); @@ -936,7 +1003,7 @@ describe("ShareAccessComponent", () => { workflowPersistSpy.updateWorkflowIsPublished.mockReturnValue(offline()); const c = setupComponent({ type: "workflow" }); - c.publishWorkflow(); + c.setPublished(true); expect(workflowPersistSpy.updateWorkflowIsPublished).toHaveBeenCalledWith(1, true); expect(notificationSpy.error).not.toHaveBeenCalled(); @@ -948,7 +1015,7 @@ describe("ShareAccessComponent", () => { workflowPersistSpy.updateWorkflowIsPublished.mockReturnValue(offline()); const c = setupComponent({ type: "workflow" }); - c.unpublishWorkflow(); + c.setPublished(false); expect(workflowPersistSpy.updateWorkflowIsPublished).toHaveBeenCalledWith(1, false); expect(notificationSpy.error).not.toHaveBeenCalled(); @@ -960,7 +1027,7 @@ describe("ShareAccessComponent", () => { datasetServiceSpy.updateDatasetPublicity.mockReturnValue(offline()); const c = setupComponent({ type: "dataset" }); - c.publishDataset(); + c.setPublished(true); expect(datasetServiceSpy.updateDatasetPublicity).toHaveBeenCalledWith(1); expect(notificationSpy.error).not.toHaveBeenCalled(); @@ -972,7 +1039,7 @@ describe("ShareAccessComponent", () => { datasetServiceSpy.updateDatasetPublicity.mockReturnValue(offline()); const c = setupComponent({ type: "dataset" }); - c.unpublishDataset(); + c.setPublished(false); expect(datasetServiceSpy.updateDatasetPublicity).toHaveBeenCalledWith(1); expect(notificationSpy.error).not.toHaveBeenCalled(); diff --git a/frontend/src/app/dashboard/component/user/share-access/share-access.component.ts b/frontend/src/app/dashboard/component/user/share-access/share-access.component.ts index 26c35b7115..d5ebb4c595 100644 --- a/frontend/src/app/dashboard/component/user/share-access/share-access.component.ts +++ b/frontend/src/app/dashboard/component/user/share-access/share-access.component.ts @@ -27,11 +27,12 @@ import { GmailService } from "../../../../common/service/gmail/gmail.service"; import { NZ_MODAL_DATA, NzModalRef, NzModalService } from "ng-zorro-antd/modal"; import { NotificationService } from "../../../../common/service/notification/notification.service"; import { HttpErrorResponse } from "@angular/common/http"; -import { USER_DATASET, USER_WORKFLOW } from "../../../../app-routing.constant"; +import { catchError, of, switchMap } from "rxjs"; import { NzMessageService } from "ng-zorro-antd/message"; -import { DatasetService } from "../../../service/user/dataset/dataset.service"; -import { WorkflowPersistService } from "src/app/common/service/workflow-persist/workflow-persist.service"; import { WorkflowActionService } from "src/app/workspace/service/workflow-graph/model/workflow-action.service"; +import { ResourceRegistryService } from "../../../service/user/resource-registry/resource-registry.service"; +import { ResourceDescriptor } from "../../../type/resource-descriptor"; +import { EntityType } from "../../../../hub/service/hub.service"; import { NgIf, NgFor } from "@angular/common"; import { NzSpaceCompactItemDirective } from "ng-zorro-antd/space"; import { NzButtonComponent } from "ng-zorro-antd/button"; @@ -88,6 +89,8 @@ export class ShareAccessComponent implements OnInit, OnDestroy { public emailTags: string[] = []; currentEmail: string | undefined = ""; isPublic: boolean | null = null; + /** Undefined for kinds the registry does not carry, i.e. computing units. */ + private readonly descriptor: ResourceDescriptor | undefined; private shouldRefresh = false; @Output() refresh = new EventEmitter<void>(); @@ -99,9 +102,8 @@ export class ShareAccessComponent implements OnInit, OnDestroy { private notificationService: NotificationService, private message: NzMessageService, private modalService: NzModalService, - private workflowPersistService: WorkflowPersistService, - private datasetService: DatasetService, private workflowActionService: WorkflowActionService, + private resourceRegistry: ResourceRegistryService, private modalRef: NzModalRef ) { this.validateForm = this.formBuilder.group({ @@ -109,6 +111,7 @@ export class ShareAccessComponent implements OnInit, OnDestroy { accessLevel: ["WRITE"], }); this.currentEmail = this.userService.getCurrentUser()?.email; + this.descriptor = this.resourceRegistry.find(this.type as EntityType); } get hasWriteAccess(): boolean { @@ -133,21 +136,11 @@ export class ShareAccessComponent implements OnInit, OnDestroy { .subscribe(name => { this.owner = name; }); - if (this.type === "workflow") { - this.workflowPersistService - .getWorkflowIsPublished(this.id) - .pipe(untilDestroyed(this)) - .subscribe(dashboardWorkflow => { - this.isPublic = dashboardWorkflow === "Public"; - }); - } else if (this.type === "dataset") { - this.datasetService - .getDataset(this.id) - .pipe(untilDestroyed(this)) - .subscribe(dashboardDataset => { - this.isPublic = dashboardDataset.dataset.isPublic; - }); - } + // Stays null for kinds that cannot be published, which is what hides the publish buttons. + this.descriptor + ?.isPublic?.(this.id) + .pipe(untilDestroyed(this)) + .subscribe(isPublic => (this.isPublic = isPublic)); } ngOnDestroy(): void { @@ -190,10 +183,8 @@ export class ShareAccessComponent implements OnInit, OnDestroy { if (this.emailTags.length > 0) { this.emailTags.forEach(email => { let message = `${this.userService.getCurrentUser()?.email} shared a ${this.type} with you`; - if (this.type !== "computing-unit") { - let routePath = ""; - if (this.type === "workflow") routePath = USER_WORKFLOW; - if (this.type === "dataset") routePath = USER_DATASET; + const routePath = this.descriptor?.privateRoute; + if (routePath !== undefined) { message += `, access the ${this.type} at ${location.origin}${routePath}/${this.id}`; } this.accessService @@ -341,9 +332,11 @@ export class ShareAccessComponent implements OnInit, OnDestroy { public verifyPublish(): void { if (!this.isPublic) { + // Only a clonable kind hands out more than read access, so only it carries the warning. + const cloneWarning = this.descriptor?.affordances?.clonable ? ", along with the right to clone your work" : ""; const modal: NzModalRef = this.modalService.create({ nzTitle: "Notice", - nzContent: `Publishing your ${this.type} would grant all Texera users read access to your ${this.type} along with the right to clone your work.`, + nzContent: `Publishing your ${this.type} would grant all Texera users read access to your ${this.type}${cloneWarning}.`, nzFooter: [ { label: "Cancel", @@ -353,15 +346,7 @@ export class ShareAccessComponent implements OnInit, OnDestroy { label: "Publish", type: "primary", onClick: () => { - if (this.type === "workflow") { - this.publishWorkflow(); - - if (this.inWorkspace) { - this.workflowActionService.setWorkflowIsPublished(1); - } - } else if (this.type === "dataset") { - this.publishDataset(); - } + this.setPublished(true); modal.close(); }, }, @@ -384,14 +369,7 @@ export class ShareAccessComponent implements OnInit, OnDestroy { label: "Unpublish", type: "primary", onClick: () => { - if (this.type === "workflow") { - this.unpublishWorkflow(); - if (this.inWorkspace) { - this.workflowActionService.setWorkflowIsPublished(0); - } - } else if (this.type === "dataset") { - this.unpublishDataset(); - } + this.setPublished(false); modal.close(); }, }, @@ -400,79 +378,35 @@ export class ShareAccessComponent implements OnInit, OnDestroy { } } - public publishWorkflow(): void { - if (!this.isPublic) { - this.workflowPersistService - .updateWorkflowIsPublished(this.id, true) - .pipe(untilDestroyed(this)) - .subscribe({ - next: () => { - this.isPublic = true; - this.notificationService.success("Workflow published successfully"); - }, - error: (error: unknown) => { - if (error instanceof HttpErrorResponse) { - this.notificationService.error(error.error.message); - } - }, - }); - } - } - - public unpublishWorkflow(): void { - if (this.isPublic) { - this.workflowPersistService - .updateWorkflowIsPublished(this.id, false) - .pipe(untilDestroyed(this)) - .subscribe({ - next: () => { - this.isPublic = false; - this.notificationService.success("Workflow unpublished successfully"); - }, - error: (error: unknown) => { - if (error instanceof HttpErrorResponse) { - this.notificationService.error(error.error.message); - } - }, - }); - } - } - - public publishDataset(): void { - if (!this.isPublic) { - this.datasetService - .updateDatasetPublicity(this.id) - .pipe(untilDestroyed(this)) - .subscribe({ - next: (res: Response) => { - this.isPublic = true; - this.notificationService.success("Dataset published successfully"); - }, - error: (error: unknown) => { - if (error instanceof HttpErrorResponse) { - this.notificationService.error(error.error.message); - } - }, - }); - } - } - - public unpublishDataset(): void { - if (this.isPublic) { - this.datasetService - .updateDatasetPublicity(this.id) - .pipe(untilDestroyed(this)) - .subscribe({ - next: (res: Response) => { - this.isPublic = false; - this.notificationService.success("Dataset unpublished successfully"); - }, - error: (error: unknown) => { - if (error instanceof HttpErrorResponse) { - this.notificationService.error(error.error.message); - } - }, - }); + public setPublished(next: boolean): void { + const descriptor = this.descriptor; + if (!descriptor?.setPublished || this.isPublic === next) { + return; } + const readBack = descriptor.isPublic; + const label = this.type.charAt(0).toUpperCase() + this.type.slice(1); + descriptor + .setPublished(this.id, next) + // Toggle-style backends ignore `next`, so the server's own answer decides what is reported. + // A read-back that fails falls back to `next`: the write already landed, and reporting it as + // a failure would invite a retry that toggles it straight back. + .pipe( + switchMap(() => (readBack ? readBack(this.id).pipe(catchError(() => of(next))) : of(next))), + untilDestroyed(this) + ) + .subscribe({ + next: published => { + this.isPublic = published; + if (this.inWorkspace) { + this.workflowActionService.setWorkflowIsPublished(published ? 1 : 0); + } + this.notificationService.success(`${label} ${published ? "published" : "unpublished"} successfully`); + }, + error: (error: unknown) => { + if (error instanceof HttpErrorResponse) { + this.notificationService.error(error.error.message); + } + }, + }); } } diff --git a/frontend/src/app/dashboard/component/user/user-model/user-model-explorer/model-detail.component.html b/frontend/src/app/dashboard/component/user/user-model/user-model-explorer/model-detail.component.html index 94be936b12..1ab224dc02 100644 --- a/frontend/src/app/dashboard/component/user/user-model/user-model-explorer/model-detail.component.html +++ b/frontend/src/app/dashboard/component/user/user-model/user-model-explorer/model-detail.component.html @@ -347,7 +347,8 @@ [fileTreeNodes]="fileTreeNodeList" [isTreeNodeDeletable]="userHasWriteAccess()" (selectedTreeNode)="onVersionFileTreeNodeSelected($event)" - (deletedTreeNode)="onPreviouslyUploadedFileDeleted($event)"> + (deletedTreeNode)="onPreviouslyUploadedFileDeleted($event)" + (setCoverImage)="onSetCoverImage($event)"> </texera-user-dataset-version-filetree> </nz-collapse-panel> </nz-collapse> @@ -458,6 +459,47 @@ </div> </div> </nz-card> + + <nz-card class="settings-general-card"> + <h3 class="settings-card-title">Access & visibility</h3> + + <div class="settings-name-row"> + <div class="settings-name-label"> + <label>Visibility</label> + <p class="settings-hint"> + {{ modelIsPublic ? "Public — anyone can view this model." : "Private — only you and invited collaborators + can see this model." }} + </p> + </div> + <div class="settings-name-controls"> + <nz-switch + [ngModel]="modelIsPublic" + (ngModelChange)="onPublicStatusChange($event)" + nzCheckedChildren="public" + nzUnCheckedChildren="private"></nz-switch> + </div> + </div> + + <nz-divider></nz-divider> + + <div class="settings-name-row"> + <div class="settings-name-label"> + <label>Downloadable</label> + <p class="settings-hint"> + {{ modelIsDownloadable ? "Viewers can download this model." : "Viewers can browse files but cannot + download them." }} + </p> + </div> + <div class="settings-name-controls"> + <nz-switch + [ngModel]="modelIsDownloadable" + (ngModelChange)="onDownloadableStatusChange($event)" + [nzDisabled]="!isOwner" + nzCheckedChildren="allowed" + nzUnCheckedChildren="blocked"></nz-switch> + </div> + </div> + </nz-card> </div> </nz-tab> </nz-tabs> diff --git a/frontend/src/app/dashboard/component/user/user-model/user-model-explorer/model-detail.component.spec.ts b/frontend/src/app/dashboard/component/user/user-model/user-model-explorer/model-detail.component.spec.ts index 6555ccb75a..95f24e6ecb 100644 --- a/frontend/src/app/dashboard/component/user/user-model/user-model-explorer/model-detail.component.spec.ts +++ b/frontend/src/app/dashboard/component/user/user-model/user-model-explorer/model-detail.component.spec.ts @@ -20,7 +20,7 @@ import { ComponentFixture, TestBed } from "@angular/core/testing"; import { NoopAnimationsModule } from "@angular/platform-browser/animations"; import { ActivatedRoute } from "@angular/router"; -import { of, throwError } from "rxjs"; +import { of, Subject, throwError } from "rxjs"; import { MarkdownService } from "ngx-markdown"; import { NzModalService } from "ng-zorro-antd/modal"; import { NgModel } from "@angular/forms"; @@ -161,6 +161,13 @@ describe("ModelDetailComponent", () => { return fixture.nativeElement as HTMLElement; }; + /** Runs change detection, flushes NgModel's microtask write, and renders the result. */ + const settle = async (): Promise<void> => { + fixture.detectChanges(); + await Promise.resolve(); + fixture.detectChanges(); + }; + // nz-tabs only instantiates the active tab, so a tab has to be opened before // anything inside it exists to assert on. const openTab = (title: string): HTMLElement => { @@ -796,6 +803,130 @@ describe("ModelDetailComponent", () => { expect(component.formats).toEqual(MODEL_FORMATS); }); + it("toggles visibility and downloadability, keeping the switch on the server's answer", () => { + let published = false; + modelService["getModel"] = vi.fn(() => of(dashboardModel({ model: { isPublic: published } }))); + modelService["updateModelPublicity"] = vi.fn(() => { + published = !published; + return of({}); + }); + modelService["updateModelDownloadable"] = vi.fn(() => throwError(() => new Error("denied"))); + create(); + + component.onPublicStatusChange(true); + component.onDownloadableStatusChange(false); + + expect(modelService["updateModelPublicity"]).toHaveBeenCalledWith(MID); + expect(component.modelIsPublic).toBe(true); + // The rejected toggle leaves the model downloadable, so the switch snaps back. + expect(component.modelIsDownloadable).toBe(true); + expect(notificationService["error"]).toHaveBeenCalled(); + }); + + it("follows the server when a stale switch toggles the wrong way, and moves the switch back", async () => { + // The page loaded a private model; something else published it meanwhile. The endpoint toggles, + // so asking for "public" makes it private again — and the rendered switch has to follow, not + // just the field. The read-back is deferred here because that is what makes it work: with + // one-way [ngModel] the correction is only seen as a change if a detection pass observed the + // clicked value first, which a real HTTP round trip guarantees. + const readBack = new Subject<unknown>(); + modelService["getModel"] = vi + .fn() + .mockReturnValueOnce(of(dashboardModel({ model: { isPublic: false } }))) + .mockReturnValue(readBack); + modelService["updateModelPublicity"] = vi.fn(() => of({})); + create(); + const root = openTab("Settings"); + const visibility = root.querySelectorAll<HTMLElement>("nz-switch button.ant-switch")[0]; + expect(visibility.classList.contains("ant-switch-checked")).toBe(false); + + visibility.click(); + // NgModel pushes a new value to the control in a microtask, so each write needs one flushed + // before the switch reflects it. + await settle(); + expect(visibility.classList.contains("ant-switch-checked")).toBe(true); + + readBack.next(dashboardModel({ model: { isPublic: false } })); + await settle(); + + expect(component.modelIsPublic).toBe(false); + expect(visibility.classList.contains("ant-switch-checked")).toBe(false); + }); + + it("reads the downloadable flag back too, since that endpoint also toggles", () => { + // A stale switch would otherwise flip downloads the wrong way and mis-gate the download buttons. + let downloadable = false; + modelService["getModel"] = vi.fn(() => of(dashboardModel({ model: { isDownloadable: downloadable } }))); + modelService["updateModelDownloadable"] = vi.fn(() => { + downloadable = !downloadable; + return of({}); + }); + create(); + component.modelIsDownloadable = true; + + component.onDownloadableStatusChange(false); + + expect(modelService["updateModelDownloadable"]).toHaveBeenCalledWith(MID); + expect(component.modelIsDownloadable).toBe(true); + expect(component.isDownloadAllowed()).toBe(true); + }); + + it("keeps a landed toggle when the read-back fails", () => { + // GET /model/{mid} sizes the repository and can fail where the toggle did not. Reporting that + // as a failure would invite a retry, which would toggle the model straight back. + modelService["updateModelPublicity"] = vi.fn(() => of({})); + modelService["getModel"] = vi + .fn() + .mockReturnValueOnce(of(dashboardModel())) + .mockReturnValue(throwError(() => new Error("lakefs down"))); + create(); + + component.onPublicStatusChange(true); + + expect(component.modelIsPublic).toBe(true); + expect(notificationService["success"]).toHaveBeenCalledWith("Model resnet-50 is now public"); + expect(notificationService["error"]).not.toHaveBeenCalled(); + }); + + it("rolls the switch back when the toggle itself fails", () => { + modelService["updateModelPublicity"] = vi.fn(() => throwError(() => new Error("denied"))); + create(); + + component.onPublicStatusChange(true); + + expect(component.modelIsPublic).toBe(false); + expect(notificationService["error"]).toHaveBeenCalled(); + }); + + it("renders both visibility switches on the Settings tab", () => { + create(); + const root = openTab("Settings"); + + expect(root.querySelectorAll("nz-switch").length).toBe(2); + }); + + it("prefixes the cover path with the selected version and reloads the resolved url", () => { + modelService["updateModelCoverImage"] = vi.fn(() => of({})); + modelService["getModelCoverUrl"] = vi.fn(() => of({ url: "http://cover/new.png"; })); + create(); + component.selectedVersion = { mvid: 3, mid: MID, creatorUid: 1, name: "v2" } as any; + + component.onSetCoverImage("images/preview.png"); + + expect(modelService["updateModelCoverImage"]).toHaveBeenCalledWith(MID, "v2/images/preview.png"); + expect(component.coverImageUrl).toBe("http://cover/new.png";); + }); + + it("does not set a cover while no version is selected", () => { + modelService["updateModelCoverImage"] = vi.fn(() => of({})); + create(); + component.selectedVersion = undefined; + + component.onSetCoverImage("images/preview.png"); + + expect(modelService["updateModelCoverImage"]).not.toHaveBeenCalled(); + }); + it("collapses and restores the right sider, and maximizes the preview", () => { create(); const root = openTab("Versions & Files"); diff --git a/frontend/src/app/dashboard/component/user/user-model/user-model-explorer/model-detail.component.ts b/frontend/src/app/dashboard/component/user/user-model/user-model-explorer/model-detail.component.ts index cd5fa68a86..e21551c649 100644 --- a/frontend/src/app/dashboard/component/user/user-model/user-model-explorer/model-detail.component.ts +++ b/frontend/src/app/dashboard/component/user/user-model/user-model-explorer/model-detail.component.ts @@ -20,8 +20,8 @@ import { Component, OnInit, ViewChild } from "@angular/core"; import { ActivatedRoute } from "@angular/router"; import { UntilDestroy, untilDestroyed } from "@ngneat/until-destroy"; -import { switchMap } from "rxjs/operators"; -import { Observable } from "rxjs"; +import { catchError, map, switchMap } from "rxjs/operators"; +import { Observable, of } from "rxjs"; import { format } from "date-fns"; import { NgIf, NgClass, NgFor } from "@angular/common"; import { FormsModule } from "@angular/forms"; @@ -40,6 +40,7 @@ import { NzSelectComponent, NzOptionComponent } from "ng-zorro-antd/select"; import { NzTabsComponent, NzTabComponent } from "ng-zorro-antd/tabs"; import { NzDividerComponent } from "ng-zorro-antd/divider"; import { NzInputDirective } from "ng-zorro-antd/input"; +import { NzSwitchComponent } from "ng-zorro-antd/switch"; import { MODEL_FORMATS, @@ -56,7 +57,7 @@ import { EntityType } from "../../../../../hub/service/hub.service"; import { extractErrorMessage } from "../../../../../common/util/error"; import { formatCount } from "src/app/common/util/format.util"; import { formatSize } from "src/app/common/util/size-formatter.util"; -import { ModelVersion } from "../../../../../common/type/model"; +import { Model, ModelVersion } from "../../../../../common/type/model"; import { DatasetFileNode, getFullPathFromDatasetFileNode, @@ -98,6 +99,7 @@ import { UserDatasetVersionFiletreeComponent } from "../../user-dataset/user-dat NzTabComponent, NzDividerComponent, NzInputDirective, + NzSwitchComponent, MarkdownDescriptionComponent, VersionUploaderComponent, UserDatasetFileRendererComponent, @@ -546,6 +548,89 @@ export class ModelDetailComponent implements OnInit { }); } + onPublicStatusChange(checked: boolean): void { + if (!this.mid) { + return; + } + const previous = this.modelIsPublic; + // Written before the request so the confirmed value below can differ from it: with one-way + // `[ngModel]`, storing the value the field already holds never reaches the switch, which would + // then keep the clicked position while the hint and the toast said the opposite. + this.modelIsPublic = checked; + this.confirmToggle(this.modelService.updateModelPublicity(this.mid)) + .pipe(untilDestroyed(this)) + .subscribe({ + next: model => { + this.modelIsPublic = model?.isPublic ?? checked; + const state = this.modelIsPublic ? "public" : "private"; + this.notificationService.success(`Model ${this.modelName} is now ${state}`); + }, + error: (err: unknown) => { + this.modelIsPublic = previous; + this.notificationService.error(extractErrorMessage(err)); + }, + }); + } + + onDownloadableStatusChange(checked: boolean): void { + if (!this.mid) { + return; + } + const previous = this.modelIsDownloadable; + this.modelIsDownloadable = checked; + this.confirmToggle(this.modelService.updateModelDownloadable(this.mid)) + .pipe(untilDestroyed(this)) + .subscribe({ + next: model => { + this.modelIsDownloadable = model?.isDownloadable ?? checked; + const state = this.modelIsDownloadable ? "allowed" : "not allowed"; + this.notificationService.success(`Model downloads are now ${state}`); + }, + error: (err: unknown) => { + this.modelIsDownloadable = previous; + this.notificationService.error(extractErrorMessage(err)); + }, + }); + } + + /** + * Both visibility flags sit behind toggle endpoints, which cannot be told which way to go, so the + * model is re-read to find out where it landed. A failed re-read yields undefined rather than an + * error: the toggle itself already succeeded, and reporting a failure would invite a retry that + * toggles it straight back. + */ + private confirmToggle(toggle: Observable<unknown>): Observable<Model | undefined> { + const mid = this.mid; + return toggle.pipe( + switchMap(() => + mid === undefined + ? of(undefined) + : this.modelService.getModel(mid).pipe( + map(dashboardModel => dashboardModel.model), + catchError(() => of(undefined)) + ) + ) + ); + } + + /** The backend stores the cover relative to the model root, so the version name has to lead. */ + onSetCoverImage(filePath: string): void { + if (!this.mid || !this.selectedVersion) { + return; + } + const mid = this.mid; + this.modelService + .updateModelCoverImage(mid, `${this.selectedVersion.name}/${filePath}`) + .pipe(untilDestroyed(this)) + .subscribe({ + next: () => { + this.loadCoverImageUrl(mid); + this.notificationService.success("Cover image updated."); + }, + error: (err: unknown) => this.notificationService.error(extractErrorMessage(err)), + }); + } + onFormatChange(modelFormat: string): void { const previous = this.modelFormat; if (!this.mid || previous === modelFormat) { diff --git a/frontend/src/app/dashboard/service/user/model/model.service.spec.ts b/frontend/src/app/dashboard/service/user/model/model.service.spec.ts index 418052af20..d764bebb59 100644 --- a/frontend/src/app/dashboard/service/user/model/model.service.spec.ts +++ b/frontend/src/app/dashboard/service/user/model/model.service.spec.ts @@ -257,6 +257,32 @@ describe("ModelService", () => { format.flush({}); }); + it("toggles publicity and downloadability without a payload", () => { + service.updateModelPublicity(7).subscribe(); + const publicity = http.expectOne(`${API}/model/7/update/publicity`); + expect(publicity.request.method).toBe("POST"); + expect(publicity.request.body).toEqual({}); + publicity.flush({}); + + service.updateModelDownloadable(7).subscribe(); + const downloadable = http.expectOne(`${API}/model/7/update/downloadable`); + expect(downloadable.request.body).toEqual({}); + downloadable.flush({}); + }); + + it("points the cover at a path already committed to the model", () => { + service.updateModelCoverImage(7, "v2/preview.png").subscribe(); + const req = http.expectOne(`${API}/model/7/update/cover`); + expect(req.request.body).toEqual({ coverImage: "v2/preview.png" }); + req.flush({}); + }); + + it("lists the owners of the models the user can see", async () => { + const pending = firstValueFrom(service.retrieveOwners()); + http.expectOne(`${API}/model/user-model-owners`).flush(["[email protected]"]); + expect(await pending).toEqual(["[email protected]"]); + }); + it("surfaces a server error rather than swallowing it", async () => { const outcome = firstValueFrom(service.retrieveAccessibleModels()).catch((err: unknown) => err); http.expectOne(`${API}/model/list`).flush({ message: "nope" }, { status: 500, statusText: "Server Error" }); diff --git a/frontend/src/app/dashboard/service/user/model/model.service.ts b/frontend/src/app/dashboard/service/user/model/model.service.ts index b409513900..9bb9d19c88 100644 --- a/frontend/src/app/dashboard/service/user/model/model.service.ts +++ b/frontend/src/app/dashboard/service/user/model/model.service.ts @@ -33,7 +33,11 @@ export const MODEL_UPDATE_NAME_URL = MODEL_UPDATE_BASE_URL + "/name"; export const MODEL_UPDATE_DESCRIPTION_URL = MODEL_UPDATE_BASE_URL + "/description"; export const MODEL_UPDATE_FRAMEWORK_URL = MODEL_UPDATE_BASE_URL + "/framework"; export const MODEL_UPDATE_FORMAT_URL = MODEL_UPDATE_BASE_URL + "/format"; +export const MODEL_UPDATE_PUBLICITY_URL = "update/publicity"; +export const MODEL_UPDATE_DOWNLOADABLE_URL = "update/downloadable"; +export const MODEL_UPDATE_COVER_URL = "update/cover"; export const MODEL_LIST_URL = MODEL_BASE_URL + "/list"; +export const MODEL_GET_OWNERS_URL = MODEL_BASE_URL + "/user-model-owners"; export const MODEL_VERSION_BASE_URL = "version"; export const MODEL_VERSION_RETRIEVE_LIST_URL = MODEL_VERSION_BASE_URL + "/list"; @@ -189,6 +193,35 @@ export class ModelService { .pipe(switchMap(({ presignedUrl }) => this.http.get(presignedUrl, { responseType: "blob" }))); } + /** Flips the model between public and private; the endpoint toggles rather than taking a value. */ + public updateModelPublicity(mid: number): Observable<Response> { + return this.http.post<Response>( + `${AppSettings.getApiEndpoint()}/${MODEL_BASE_URL}/${mid}/${MODEL_UPDATE_PUBLICITY_URL}`, + {} + ); + } + + public updateModelDownloadable(mid: number): Observable<Response> { + return this.http.post<Response>( + `${AppSettings.getApiEndpoint()}/${MODEL_BASE_URL}/${mid}/${MODEL_UPDATE_DOWNLOADABLE_URL}`, + {} + ); + } + + public retrieveOwners(): Observable<string[]> { + return this.http.get<string[]>(`${AppSettings.getApiEndpoint()}/${MODEL_GET_OWNERS_URL}`); + } + + /** Points the model card at a committed image, given as "<version>/<path>". */ + public updateModelCoverImage(mid: number, coverImage: string): Observable<Response> { + return this.http.post<Response>( + `${AppSettings.getApiEndpoint()}/${MODEL_BASE_URL}/${mid}/${MODEL_UPDATE_COVER_URL}`, + { + coverImage: coverImage, + } + ); + } + public getModelCoverUrl(mid: number): Observable<{ url: string | null }> { return this.http.get<{ url: string | null }>(`${AppSettings.getApiEndpoint()}/${MODEL_BASE_URL}/${mid}/cover-url`); } diff --git a/frontend/src/app/dashboard/service/user/resource-registry/dataset-resource.descriptor.ts b/frontend/src/app/dashboard/service/user/resource-registry/dataset-resource.descriptor.ts index 4c4078c1b2..164a48c693 100644 --- a/frontend/src/app/dashboard/service/user/resource-registry/dataset-resource.descriptor.ts +++ b/frontend/src/app/dashboard/service/user/resource-registry/dataset-resource.descriptor.ts @@ -19,11 +19,12 @@ import { Injectable } from "@angular/core"; import { DashboardEntry } from "../../../type/dashboard-entry"; -import { ResourceDescriptor } from "../../../type/resource-descriptor"; +import { ResourceAffordances, ResourceDescriptor } from "../../../type/resource-descriptor"; import { EntityType } from "../../../../hub/service/hub.service"; import { DatasetService, DEFAULT_DATASET_NAME, validateDatasetName } from "../dataset/dataset.service"; import { HUB_DATASET_RESULT_DETAIL, USER_DATASET } from "../../../../app-routing.constant"; import { DownloadService } from "../download/download.service"; +import { map } from "rxjs/operators"; @Injectable({ providedIn: "root", @@ -35,6 +36,8 @@ export class DatasetResourceDescriptor implements ResourceDescriptor { readonly hubRoute = HUB_DATASET_RESULT_DETAIL; readonly hasSize = true; readonly defaultName = DEFAULT_DATASET_NAME; + // Publishing a dataset grants read access only; there is no clone action for it. + readonly affordances: ResourceAffordances = { clonable: false }; constructor( private datasetService: DatasetService, @@ -50,5 +53,10 @@ export class DatasetResourceDescriptor implements ResourceDescriptor { download = (id: number, name: string) => this.downloadService.downloadDataset(id, name); retrieveSingleFile = (filePath: string, isLogin: boolean) => this.datasetService.retrieveDatasetVersionSingleFile(filePath, isLogin); + isPublic = (id: number) => this.datasetService.getDataset(id).pipe(map(dashboard => dashboard.dataset.isPublic)); + // The endpoint toggles, so `next` is the caller's expectation rather than a payload. + setPublished = (id: number) => this.datasetService.updateDatasetPublicity(id); + coverUrl = (id: number) => this.datasetService.getDatasetCoverUrl(id).pipe(map(({ url }) => url)); + setCover = (id: number, path: string) => this.datasetService.updateDatasetCoverImage(id, path); // No dataset-id endpoint exists, so `retrieveIds` stays absent and the id filter hides itself. } diff --git a/frontend/src/app/dashboard/service/user/resource-registry/model-resource.descriptor.ts b/frontend/src/app/dashboard/service/user/resource-registry/model-resource.descriptor.ts index 90258baa1f..6edd17d6ac 100644 --- a/frontend/src/app/dashboard/service/user/resource-registry/model-resource.descriptor.ts +++ b/frontend/src/app/dashboard/service/user/resource-registry/model-resource.descriptor.ts @@ -19,12 +19,13 @@ import { Injectable } from "@angular/core"; import { DashboardEntry } from "../../../type/dashboard-entry"; -import { ResourceDescriptor } from "../../../type/resource-descriptor"; +import { ResourceAffordances, ResourceDescriptor } from "../../../type/resource-descriptor"; import { EntityType } from "../../../../hub/service/hub.service"; import { DEFAULT_MODEL_NAME, ModelService, validateModelName } from "../model/model.service"; import { MODEL_ICON } from "../../../../common/icon/model-icon"; import { USER_MODEL } from "../../../../app-routing.constant"; import { DownloadService } from "../download/download.service"; +import { map } from "rxjs/operators"; @Injectable({ providedIn: "root", @@ -36,6 +37,8 @@ export class ModelResourceDescriptor implements ResourceDescriptor { // `hubRoute` is deliberately absent: models reach the hub with the rest of the hub UI. readonly hasSize = true; readonly defaultName = DEFAULT_MODEL_NAME; + // Publishing a model grants read access only; there is no clone action for it. + readonly affordances: ResourceAffordances = { clonable: false }; constructor( private modelService: ModelService, @@ -49,5 +52,10 @@ export class ModelResourceDescriptor implements ResourceDescriptor { download = (id: number, name: string) => this.downloadService.downloadModel(id, name); retrieveSingleFile = (filePath: string, isLogin: boolean) => this.modelService.retrieveModelVersionSingleFile(filePath, isLogin); - // `retrieveOwners` arrives with the share modal and the filters, which need it. + retrieveOwners = () => this.modelService.retrieveOwners(); + isPublic = (id: number) => this.modelService.getModel(id).pipe(map(dashboard => dashboard.model.isPublic)); + // The endpoint toggles, so `next` is the caller's expectation rather than a payload. + setPublished = (id: number) => this.modelService.updateModelPublicity(id); + coverUrl = (id: number) => this.modelService.getModelCoverUrl(id).pipe(map(({ url }) => url)); + setCover = (id: number, path: string) => this.modelService.updateModelCoverImage(id, path); } diff --git a/frontend/src/app/dashboard/service/user/resource-registry/resource-registry.service.spec.ts b/frontend/src/app/dashboard/service/user/resource-registry/resource-registry.service.spec.ts index 0dba95dec4..fbfd351fef 100644 --- a/frontend/src/app/dashboard/service/user/resource-registry/resource-registry.service.spec.ts +++ b/frontend/src/app/dashboard/service/user/resource-registry/resource-registry.service.spec.ts @@ -19,7 +19,7 @@ import { TestBed } from "@angular/core/testing"; import { HttpClientTestingModule } from "@angular/common/http/testing"; -import { of } from "rxjs"; +import { firstValueFrom, of } from "rxjs"; import { ResourceRegistryService } from "./resource-registry.service"; import { DashboardEntry } from "../../../type/dashboard-entry"; import { EntityType } from "../../../../hub/service/hub.service"; @@ -66,6 +66,11 @@ describe("ResourceRegistryService", () => { updateModelName: vi.fn().mockReturnValue(of({})), updateModelDescription: vi.fn().mockReturnValue(of({})), retrieveModelVersionSingleFile: vi.fn().mockReturnValue(of(new Blob())), + retrieveOwners: vi.fn().mockReturnValue(of(["m-owner"])), + getModel: vi.fn().mockReturnValue(of({ model: { isPublic: true } })), + updateModelPublicity: vi.fn().mockReturnValue(of({})), + getModelCoverUrl: vi.fn().mockReturnValue(of({ url: "http://cover"; })), + updateModelCoverImage: vi.fn().mockReturnValue(of({})), }; downloadService = { @@ -102,6 +107,12 @@ describe("ResourceRegistryService", () => { expect(() => registry.get("quantum" as EntityType)).toThrowError("Unexpected type in DashboardEntry."); }); + it("answers with undefined instead of throwing when the caller can cope", () => { + // The share modal opens for computing units too, and asks the registry what they can do. + expect(registry.find(EntityType.ComputingUnit)).toBeUndefined(); + expect(registry.find(EntityType.Model)).toBeDefined(); + }); + // ─── capability checks ──────────────────────────────────────────────────── it("exposes rename and description only for the kinds that support them", () => { @@ -109,8 +120,6 @@ describe("ResourceRegistryService", () => { expect(registry.get(type).rename).toBeDefined(); expect(registry.get(type).updateDescription).toBeDefined(); } - // Models gain `retrieveOwners` with the share modal and the filters, which are the only callers. - expect(registry.get(EntityType.Model).retrieveOwners).toBeUndefined(); for (const type of [EntityType.File]) { expect(registry.get(type).rename).toBeUndefined(); expect(registry.get(type).updateDescription).toBeUndefined(); @@ -130,6 +139,26 @@ describe("ResourceRegistryService", () => { expect(registry.get(EntityType.Model).retrieveSingleFile).toBeDefined(); }); + it("offers publishing and covers only to the kinds the backend supports", () => { + for (const type of [EntityType.Workflow, EntityType.Dataset, EntityType.Model]) { + expect(registry.get(type).retrieveOwners).toBeDefined(); + expect(registry.get(type).isPublic).toBeDefined(); + expect(registry.get(type).setPublished).toBeDefined(); + } + expect(registry.get(EntityType.File).isPublic).toBeUndefined(); + expect(registry.get(EntityType.File).setPublished).toBeUndefined(); + // A workflow cover is a data URL on the entry, so only the file-backed kinds resolve one. + expect(registry.get(EntityType.Workflow).coverUrl).toBeUndefined(); + expect(registry.get(EntityType.Dataset).coverUrl).toBeDefined(); + expect(registry.get(EntityType.Model).coverUrl).toBeDefined(); + }); + + it("warns about cloning only for the kind that can be cloned", () => { + expect(registry.get(EntityType.Workflow).affordances?.clonable).toBe(true); + expect(registry.get(EntityType.Dataset).affordances?.clonable).toBe(false); + expect(registry.get(EntityType.Model).affordances?.clonable).toBe(false); + }); + it("offers an id filter only where the backend has an id endpoint", () => { expect(registry.get(EntityType.Workflow).retrieveIds).toBeDefined(); expect(registry.get(EntityType.Dataset).retrieveIds).toBeUndefined(); @@ -172,6 +201,18 @@ describe("ResourceRegistryService", () => { expect(modelService["retrieveModelVersionSingleFile"]).toHaveBeenCalledWith("/model/a/m/v1/f.pt", false); }); + it("delegates a model's publishing and cover work to ModelService", async () => { + const model = registry.get(EntityType.Model); + + expect(await firstValueFrom(model.isPublic!(3))).toBe(true); + model.setPublished!(3, false); + expect(await firstValueFrom(model.coverUrl!(3))).toBe("http://cover";); + model.setCover!(3, "v1/preview.png"); + + expect(modelService["updateModelPublicity"]).toHaveBeenCalledWith(3); + expect(modelService["updateModelCoverImage"]).toHaveBeenCalledWith(3, "v1/preview.png"); + }); + it("reads ownership off the kind's own payload", () => { expect(registry.get(EntityType.Workflow).isOwner(entry({ workflow: { isOwner: false } }))).toBe(false); expect(registry.get(EntityType.Dataset).isOwner(entry({ dataset: { isOwner: true } }))).toBe(true); diff --git a/frontend/src/app/dashboard/service/user/resource-registry/resource-registry.service.ts b/frontend/src/app/dashboard/service/user/resource-registry/resource-registry.service.ts index a850423cf3..592a7f2c0c 100644 --- a/frontend/src/app/dashboard/service/user/resource-registry/resource-registry.service.ts +++ b/frontend/src/app/dashboard/service/user/resource-registry/resource-registry.service.ts @@ -49,6 +49,11 @@ export class ResourceRegistryService { ); } + /** The descriptor for a kind, or undefined when the kind has none (computing units). */ + public find(type: EntityType): ResourceDescriptor | undefined { + return this.descriptors.get(type); + } + public get(type: EntityType): ResourceDescriptor { const descriptor = this.descriptors.get(type); if (!descriptor) { diff --git a/frontend/src/app/dashboard/service/user/resource-registry/workflow-resource.descriptor.ts b/frontend/src/app/dashboard/service/user/resource-registry/workflow-resource.descriptor.ts index 07b8614796..5930673c42 100644 --- a/frontend/src/app/dashboard/service/user/resource-registry/workflow-resource.descriptor.ts +++ b/frontend/src/app/dashboard/service/user/resource-registry/workflow-resource.descriptor.ts @@ -19,7 +19,7 @@ import { Injectable } from "@angular/core"; import { DashboardEntry } from "../../../type/dashboard-entry"; -import { ResourceDescriptor } from "../../../type/resource-descriptor"; +import { ResourceAffordances, ResourceDescriptor } from "../../../type/resource-descriptor"; import { EntityType } from "../../../../hub/service/hub.service"; import { DEFAULT_WORKFLOW_NAME, @@ -27,6 +27,7 @@ import { } from "../../../../common/service/workflow-persist/workflow-persist.service"; import { HUB_WORKFLOW_RESULT_DETAIL, USER_WORKSPACE } from "../../../../app-routing.constant"; import { DownloadService } from "../download/download.service"; +import { map } from "rxjs/operators"; @Injectable({ providedIn: "root", @@ -38,6 +39,7 @@ export class WorkflowResourceDescriptor implements ResourceDescriptor { readonly hubRoute = HUB_WORKFLOW_RESULT_DETAIL; readonly hasSize = true; readonly defaultName = DEFAULT_WORKFLOW_NAME; + readonly affordances: ResourceAffordances = { clonable: true }; constructor( private workflowPersistService: WorkflowPersistService, @@ -53,4 +55,9 @@ export class WorkflowResourceDescriptor implements ResourceDescriptor { retrieveOwners = () => this.workflowPersistService.retrieveOwners(); retrieveIds = () => this.workflowPersistService.retrieveWorkflowIDs(); download = (id: number, name: string) => this.downloadService.downloadWorkflow(id, name); + isPublic = (id: number) => + this.workflowPersistService.getWorkflowIsPublished(id).pipe(map(published => published === "Public")); + setPublished = (id: number, next: boolean) => this.workflowPersistService.updateWorkflowIsPublished(id, next); + // No `coverUrl`/`setCover`: a workflow cover is a data URL carried on the entry itself, not a + // committed file that has to be fetched. } diff --git a/frontend/src/app/dashboard/type/resource-descriptor.ts b/frontend/src/app/dashboard/type/resource-descriptor.ts index c2cdc39aa9..85ecada475 100644 --- a/frontend/src/app/dashboard/type/resource-descriptor.ts +++ b/frontend/src/app/dashboard/type/resource-descriptor.ts @@ -21,6 +21,12 @@ import { Observable } from "rxjs"; import { DashboardEntry } from "./dashboard-entry"; import { EntityType } from "../../hub/service/hub.service"; +/** What publishing an entry of a kind grants everyone else. */ +export interface ResourceAffordances { + /** Whether a published entry may also be cloned, not just read. */ + readonly clonable: boolean; +} + /** * What one dashboard resource kind can do. Optional members are the capability check: a component * asks `if (descriptor.rename)` instead of testing the entry type, so adding a resource is a new @@ -47,8 +53,22 @@ export interface ResourceDescriptor { download?(id: number, name: string): Observable<unknown>; /** Fetches one file of a version for preview, by its logical path. */ retrieveSingleFile?(filePath: string, isLogin: boolean): Observable<Blob>; - /** Owners of this kind, for the filter dropdown. */ + /** Owners of this kind, for the filter dropdown and the share modal. */ retrieveOwners?(): Observable<string[]>; + /** What other users get once an entry is published; absent when the kind cannot be published. */ + readonly affordances?: ResourceAffordances; + /** Whether the entry is currently published. */ + isPublic?(id: number): Observable<boolean>; + /** + * Requests `next` as the new published state. Some backends expose a toggle rather than an + * absolute set and so ignore `next`, which means the result is only what the caller asked for, + * never what it got: read `isPublic` back afterwards instead of assuming `next` took effect. + */ + setPublished?(id: number, next: boolean): Observable<unknown>; + /** A ready-to-render URL for the entry's cover image, or null when it has none. */ + coverUrl?(id: number): Observable<string | null>; + /** Points the cover at a committed file of the entry, as "<version>/<path>". */ + setCover?(id: number, path: string): Observable<unknown>; /** Entry ids of this kind; absent when the backend exposes no such endpoint. */ retrieveIds?(): Observable<number[]>; } diff --git a/frontend/src/app/hub/component/browse-section/browse-section.component.spec.ts b/frontend/src/app/hub/component/browse-section/browse-section.component.spec.ts index 68f4e4539c..a4b1ad301a 100644 --- a/frontend/src/app/hub/component/browse-section/browse-section.component.spec.ts +++ b/frontend/src/app/hub/component/browse-section/browse-section.component.spec.ts @@ -20,6 +20,7 @@ import { ComponentFixture, TestBed } from "@angular/core/testing"; import { RouterTestingModule } from "@angular/router/testing"; import { By } from "@angular/platform-browser"; +import { of } from "rxjs"; import { UserService } from "src/app/common/service/user/user.service"; import { StubUserService } from "src/app/common/service/user/stub-user.service"; import { BrowseSectionComponent } from "./browse-section.component"; @@ -27,7 +28,6 @@ import { WorkflowPersistService } from "../../../common/service/workflow-persist import { DatasetService } from "../../../dashboard/service/user/dataset/dataset.service"; import { commonTestProviders } from "../../../common/testing/test-utils"; import { DashboardEntry } from "../../../dashboard/type/dashboard-entry"; -import { AppSettings } from "../../../common/app-setting"; import { HUB_DATASET_RESULT_DETAIL, HUB_WORKFLOW_RESULT_DETAIL, @@ -35,6 +35,9 @@ import { USER_WORKSPACE, } from "../../../app-routing.constant"; +/** What the dataset service's cover-url endpoint hands back in these specs. */ +const PRESIGNED_COVER = "https://s3.example/cover.png?sig=abc";; + describe("BrowseSectionComponent", () => { let component: BrowseSectionComponent; let fixture: ComponentFixture<BrowseSectionComponent>; @@ -44,7 +47,8 @@ describe("BrowseSectionComponent", () => { imports: [BrowseSectionComponent], providers: [ { provide: WorkflowPersistService, useValue: {} }, - { provide: DatasetService, useValue: {} }, + // The cover now comes from the descriptor, so the double has to answer for it. + { provide: DatasetService, useValue: { getDatasetCoverUrl: () => of({ url: PRESIGNED_COVER }) } }, ...commonTestProviders, ], }); @@ -101,7 +105,7 @@ describe("BrowseSectionComponent", () => { }); describe("cover images", () => { - it("builds and caches the cover URL for a dataset that has a cover image", () => { + it("caches the cover URL the descriptor resolves for an entity that has a cover", () => { const entity = { id: 5, type: "dataset", @@ -111,11 +115,11 @@ describe("BrowseSectionComponent", () => { component.entities = [entity]; component.ngOnInit(); - expect(component.getCoverImage(entity)).toBe(`${AppSettings.getApiEndpoint()}/dataset/5/cover`); + expect(component.getCoverImage(entity)).toBe(PRESIGNED_COVER); }); it("falls back to the default background when no cover was cached", () => { - // No coverImageUrl -> loadCoverImages skips it -> getCoverImage returns the default. + // No coverImageUrl -> loadCoverImages never asks the descriptor -> getCoverImage defaults. const entity = { id: 6, type: "dataset", accessibleUserIds: [] } as unknown as DashboardEntry; component.entities = [entity]; component.ngOnInit(); @@ -141,7 +145,8 @@ describe("BrowseSectionComponent rendering", () => { // AuthService and its whole dependency chain, so the shared stub stands in for it. { provide: UserService, useClass: StubUserService }, { provide: WorkflowPersistService, useValue: {} }, - { provide: DatasetService, useValue: {} }, + // The cover now comes from the descriptor, so the double has to answer for it. + { provide: DatasetService, useValue: { getDatasetCoverUrl: () => of({ url: PRESIGNED_COVER }) } }, ...commonTestProviders, ], }); @@ -194,11 +199,11 @@ describe("BrowseSectionComponent rendering", () => { const el = render([entity({ id: 5, coverImageUrl: "has-cover" })]); const img = el.querySelector<HTMLImageElement>(".card-cover-image")!; - expect(img.getAttribute("src")).toBe(`${AppSettings.getApiEndpoint()}/dataset/5/cover`); + expect(img.getAttribute("src")).toBe(PRESIGNED_COVER); }); it("falls back to the default background when the cover image fails to load", () => { - // A cached cover URL can still 404; the inline error handler is the only thing that stops the + // A presigned cover URL can still 404; the inline error handler is the only thing that stops the // card from showing a broken image. const el = render([entity({ id: 5, coverImageUrl: "has-cover" })]); const img = el.querySelector<HTMLImageElement>(".card-cover-image")!; diff --git a/frontend/src/app/hub/component/browse-section/browse-section.component.ts b/frontend/src/app/hub/component/browse-section/browse-section.component.ts index 659e018e1e..42274e8244 100644 --- a/frontend/src/app/hub/component/browse-section/browse-section.component.ts +++ b/frontend/src/app/hub/component/browse-section/browse-section.component.ts @@ -19,16 +19,14 @@ import { ChangeDetectorRef, Component, Input, OnChanges, OnInit, SimpleChanges } from "@angular/core"; import { DashboardEntry } from "../../../dashboard/type/dashboard-entry"; -import { WorkflowPersistService } from "../../../common/service/workflow-persist/workflow-persist.service"; -import { DatasetService } from "../../../dashboard/service/user/dataset/dataset.service"; -import { UntilDestroy } from "@ngneat/until-destroy"; +import { ResourceRegistryService } from "../../../dashboard/service/user/resource-registry/resource-registry.service"; +import { UntilDestroy, untilDestroyed } from "@ngneat/until-destroy"; import { HUB_DATASET_RESULT_DETAIL, HUB_WORKFLOW_RESULT_DETAIL, USER_DATASET, USER_WORKSPACE, } from "../../../app-routing.constant"; -import { AppSettings } from "../../../common/app-setting"; import { NgIf, NgFor, NgStyle, DatePipe } from "@angular/common"; import { NzCardComponent } from "ng-zorro-antd/card"; import { RouterLink } from "@angular/router"; @@ -65,11 +63,11 @@ export class BrowseSectionComponent implements OnInit, OnChanges { protected readonly USER_DATASET = USER_DATASET; entityRoutes: { [key: number]: string[] } = {}; - private coverImageUrls = new Map<number, string>(); + /** Keyed by type and id: ids are only unique within a kind, and sections may mix kinds. */ + private coverImageUrls = new Map<string, string>(); constructor( - private workflowPersistService: WorkflowPersistService, - private datasetService: DatasetService, + private resourceRegistry: ResourceRegistryService, private cdr: ChangeDetectorRef ) {} @@ -112,24 +110,40 @@ export class BrowseSectionComponent implements OnInit, OnChanges { } } + private coverCacheKey(entity: DashboardEntry): string { + return `${entity.type}:${entity.id}`; + } + + /** Asks each kind's descriptor for its cover, so the hub renders the same picture as the cards. */ private loadCoverImages(): void { if (!this.entities) return; this.entities .filter( (entity): entity is DashboardEntry & { id: number } => - entity.type === "dataset" && entity.coverImageUrl !== undefined && entity.id !== undefined && - !this.coverImageUrls.has(entity.id) + !this.coverImageUrls.has(this.coverCacheKey(entity)) ) .forEach(entity => { - const coverUrl = `${AppSettings.getApiEndpoint()}/dataset/${entity.id}/cover`; - this.coverImageUrls.set(entity.id, coverUrl); + // `find`, not `get`: a section may hold a kind the registry does not carry. + const coverUrl = this.resourceRegistry.find(entity.type)?.coverUrl; + if (!coverUrl) { + return; + } + const key = this.coverCacheKey(entity); + coverUrl(entity.id) + .pipe(untilDestroyed(this)) + .subscribe(url => { + if (url) { + this.coverImageUrls.set(key, url); + this.cdr.markForCheck(); + } + }); }); } getCoverImage(entity: DashboardEntry): string { - return this.coverImageUrls.get(entity.id!) || this.defaultBackground; + return this.coverImageUrls.get(this.coverCacheKey(entity)) || this.defaultBackground; } }
![http://cover/new.png"](http://cover/new.png"){kind=link}
![https://s3.example/cover.png?sig=abc"](https://s3.example/cover.png?sig=abc"){kind=link}