From 0f1caae83051a9e39073f71446cb54268976d3f0 Mon Sep 17 00:00:00 2001 From: Kunwoo Park Date: Thu, 24 Oct 2024 12:56:37 -0700 Subject: [PATCH 1/5] restore to the original workflow name if there is an error updating the workflow name --- .../user/list-item/list-item.component.ts | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/core/gui/src/app/dashboard/component/user/list-item/list-item.component.ts b/core/gui/src/app/dashboard/component/user/list-item/list-item.component.ts index 480f4c3f9b9..95206513850 100644 --- a/core/gui/src/app/dashboard/component/user/list-item/list-item.component.ts +++ b/core/gui/src/app/dashboard/component/user/list-item/list-item.component.ts @@ -32,6 +32,7 @@ import { formatSize } from "src/app/common/util/size-formatter.util"; }) export class ListItemComponent implements OnInit, OnChanges { private owners: number[] = []; + private originalName: string = ""; @Input() currentUid: number | undefined; @ViewChild("nameInput") nameInput!: ElementRef; @ViewChild("descriptionInput") descriptionInput!: ElementRef; @@ -159,6 +160,7 @@ export class ListItemComponent implements OnInit, OnChanges { }; onEditName(): void { + this.originalName = this.entry.name; this.editingName = true; setTimeout(() => { if (this.nameInput) { @@ -183,14 +185,23 @@ export class ListItemComponent implements OnInit, OnChanges { } public confirmUpdateWorkflowCustomName(name: string): void { + const workflowName = name || DEFAULT_WORKFLOW_NAME; + this.workflowPersistService - .updateWorkflowName(this.entry.id, name || DEFAULT_WORKFLOW_NAME) + .updateWorkflowName(this.entry.id, workflowName) .pipe(untilDestroyed(this)) - .subscribe(() => { - this.entry.name = name || DEFAULT_WORKFLOW_NAME; - }) - .add(() => { - this.editingName = false; + .subscribe({ + next: () => { + this.entry.name = workflowName; + }, + error: (err) => { + console.error("Failed to update workflow name:", err); + this.entry.name = this.originalName; + this.editingName = false; + }, + complete: () => { + this.editingName = false; + } }); } From ba398d83a88807344dc9b2b6bede659233eca7bb Mon Sep 17 00:00:00 2001 From: Kunwoo Park Date: Thu, 24 Oct 2024 12:58:07 -0700 Subject: [PATCH 2/5] format file --- .../component/user/list-item/list-item.component.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/gui/src/app/dashboard/component/user/list-item/list-item.component.ts b/core/gui/src/app/dashboard/component/user/list-item/list-item.component.ts index 95206513850..93162593ec5 100644 --- a/core/gui/src/app/dashboard/component/user/list-item/list-item.component.ts +++ b/core/gui/src/app/dashboard/component/user/list-item/list-item.component.ts @@ -186,7 +186,7 @@ export class ListItemComponent implements OnInit, OnChanges { public confirmUpdateWorkflowCustomName(name: string): void { const workflowName = name || DEFAULT_WORKFLOW_NAME; - + this.workflowPersistService .updateWorkflowName(this.entry.id, workflowName) .pipe(untilDestroyed(this)) @@ -194,14 +194,14 @@ export class ListItemComponent implements OnInit, OnChanges { next: () => { this.entry.name = workflowName; }, - error: (err) => { + error: (err: unknown) => { console.error("Failed to update workflow name:", err); this.entry.name = this.originalName; this.editingName = false; }, complete: () => { this.editingName = false; - } + }, }); } From ac77df65a5cd6295c2d8a0b68a7e81afea2de0a6 Mon Sep 17 00:00:00 2001 From: Kunwoo Park Date: Thu, 24 Oct 2024 13:09:43 -0700 Subject: [PATCH 3/5] Add unit tests --- .../list-item/list-item.component.spec.ts | 87 +++++++++++++++++++ .../user/list-item/list-item.component.ts | 21 +++-- 2 files changed, 102 insertions(+), 6 deletions(-) create mode 100644 core/gui/src/app/dashboard/component/user/list-item/list-item.component.spec.ts diff --git a/core/gui/src/app/dashboard/component/user/list-item/list-item.component.spec.ts b/core/gui/src/app/dashboard/component/user/list-item/list-item.component.spec.ts new file mode 100644 index 00000000000..cc6517b8c32 --- /dev/null +++ b/core/gui/src/app/dashboard/component/user/list-item/list-item.component.spec.ts @@ -0,0 +1,87 @@ +import { ComponentFixture, TestBed } from "@angular/core/testing"; +import { ListItemComponent } from "./list-item.component"; +import { WorkflowPersistService } from "src/app/common/service/workflow-persist/workflow-persist.service"; +import { HttpClientTestingModule } from "@angular/common/http/testing"; +import { NzModalService } from "ng-zorro-antd/modal"; +import { of, throwError } from "rxjs"; +import { NO_ERRORS_SCHEMA } from "@angular/core"; + +describe("ListItemComponent", () => { + let component: ListItemComponent; + let fixture: ComponentFixture; + let workflowPersistService: jasmine.SpyObj; + let nzModalService: jasmine.SpyObj; + + beforeEach(async () => { + const workflowPersistServiceSpy = jasmine.createSpyObj("WorkflowPersistService", [ + "updateWorkflowName", + "updateWorkflowDescription", + ]); + const nzModalServiceSpy = jasmine.createSpyObj("NzModalService", ["create"]); + + await TestBed.configureTestingModule({ + imports: [HttpClientTestingModule], + declarations: [ListItemComponent], + providers: [ + { provide: WorkflowPersistService, useValue: workflowPersistServiceSpy }, + { provide: NzModalService, useValue: nzModalServiceSpy }, + ], + schemas: [NO_ERRORS_SCHEMA], + }).compileComponents(); + + fixture = TestBed.createComponent(ListItemComponent); + component = fixture.componentInstance; + workflowPersistService = TestBed.inject(WorkflowPersistService) as jasmine.SpyObj; + nzModalService = TestBed.inject(NzModalService) as jasmine.SpyObj; + }); + + it("should update workflow name successfully", () => { + const newName = "New Workflow Name"; + component.entry = { id: 1, name: "Old Name" } as any; + workflowPersistService.updateWorkflowName.and.returnValue(of({} as Response)); + + component.confirmUpdateWorkflowCustomName(newName); + + expect(workflowPersistService.updateWorkflowName).toHaveBeenCalledWith(1, newName); + expect(component.entry.name).toBe(newName); + expect(component.editingName).toBeFalse(); + }); + + it("should handle error when updating workflow name", () => { + const newName = "New Workflow Name"; + component.entry = { id: 1, name: "Old Name" } as any; + component.originalName = "Old Name"; + workflowPersistService.updateWorkflowName.and.returnValue(throwError(() => new Error("Error"))); + + component.confirmUpdateWorkflowCustomName(newName); + + expect(workflowPersistService.updateWorkflowName).toHaveBeenCalledWith(1, newName); + expect(component.entry.name).toBe("Old Name"); + expect(component.editingName).toBeFalse(); + }); + + it("should update workflow description successfully", () => { + const newDescription = "New Description"; + component.entry = { id: 1, description: "Old Description" } as any; + workflowPersistService.updateWorkflowDescription.and.returnValue(of({} as Response)); + + component.confirmUpdateWorkflowCustomDescription(newDescription); + + expect(workflowPersistService.updateWorkflowDescription).toHaveBeenCalledWith(1, newDescription); + expect(component.entry.description).toBe(newDescription); + expect(component.editingDescription).toBeFalse(); + }); + + it("should handle error when updating workflow description", () => { + const newDescription = "New Description"; + component.entry = { id: 1, description: "Old Description" } as any; + component.originalDescription = "Old Description"; + workflowPersistService.updateWorkflowDescription.and.returnValue(throwError(() => new Error("Error"))); + + component.confirmUpdateWorkflowCustomDescription(newDescription); + + expect(workflowPersistService.updateWorkflowDescription).toHaveBeenCalledWith(1, newDescription); + expect(component.entry.description).toBe("Old Description"); + expect(component.editingDescription).toBeFalse(); + }); +}); diff --git a/core/gui/src/app/dashboard/component/user/list-item/list-item.component.ts b/core/gui/src/app/dashboard/component/user/list-item/list-item.component.ts index 93162593ec5..e8f068b0ce6 100644 --- a/core/gui/src/app/dashboard/component/user/list-item/list-item.component.ts +++ b/core/gui/src/app/dashboard/component/user/list-item/list-item.component.ts @@ -32,7 +32,8 @@ import { formatSize } from "src/app/common/util/size-formatter.util"; }) export class ListItemComponent implements OnInit, OnChanges { private owners: number[] = []; - private originalName: string = ""; + public originalName: string = ""; + public originalDescription: string | undefined = undefined; @Input() currentUid: number | undefined; @ViewChild("nameInput") nameInput!: ElementRef; @ViewChild("descriptionInput") descriptionInput!: ElementRef; @@ -173,6 +174,7 @@ export class ListItemComponent implements OnInit, OnChanges { } onEditDescription(): void { + this.originalDescription = this.entry.description; this.editingDescription = true; setTimeout(() => { if (this.descriptionInput) { @@ -211,11 +213,18 @@ export class ListItemComponent implements OnInit, OnChanges { this.workflowPersistService .updateWorkflowDescription(this.entry.id, updatedDescription) .pipe(untilDestroyed(this)) - .subscribe(() => { - this.entry.description = updatedDescription; - }) - .add(() => { - this.editingDescription = false; + .subscribe({ + next: () => { + this.entry.description = updatedDescription; + }, + error: (err: unknown) => { + console.error("Failed to update workflow description:", err); + this.entry.description = this.originalDescription; + this.editingDescription = false; + }, + complete: () => { + this.editingDescription = false; + }, }); } From 88eea2170e92e7bac4637592502b8013330fe9e1 Mon Sep 17 00:00:00 2001 From: Kunwoo Park Date: Thu, 24 Oct 2024 14:27:22 -0700 Subject: [PATCH 4/5] Fix backend check logic --- .../user/workflow/WorkflowResource.scala | 54 +++++++++---------- 1 file changed, 24 insertions(+), 30 deletions(-) diff --git a/core/amber/src/main/scala/edu/uci/ics/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala b/core/amber/src/main/scala/edu/uci/ics/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala index d06607445a3..d3d4b09d161 100644 --- a/core/amber/src/main/scala/edu/uci/ics/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala +++ b/core/amber/src/main/scala/edu/uci/ics/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala @@ -103,6 +103,28 @@ object WorkflowResource { case class WorkflowIDs(wids: List[UInteger], pid: Option[UInteger]) + private def updateWorkflowField( + workflow: Workflow, + sessionUser: SessionUser, + updateFunction: Workflow => Unit + ): Unit = { + val wid = workflow.getWid + val user = sessionUser.getUser + + if ( + workflowOfUserExists(wid, user.getUid) || WorkflowAccessResource.hasWriteAccess( + wid, + user.getUid + ) + ) { + val userWorkflow = workflowDao.fetchOneByWid(wid) + updateFunction(userWorkflow) + workflowDao.update(userWorkflow) + } else { + throw new ForbiddenException("No sufficient access privilege.") + } + } + } @Produces(Array(MediaType.APPLICATION_JSON)) @RolesAllowed(Array("REGULAR", "ADMIN")) @@ -466,11 +488,6 @@ class WorkflowResource extends LazyLogging { } } - /** - * This method updates the name of a given workflow - * - * @return Response - */ @POST @Consumes(Array(MediaType.APPLICATION_JSON)) @Produces(Array(MediaType.APPLICATION_JSON)) @@ -479,18 +496,7 @@ class WorkflowResource extends LazyLogging { workflow: Workflow, @Auth sessionUser: SessionUser ): Unit = { - val wid = workflow.getWid - val name = workflow.getName - val user = sessionUser.getUser - if (!WorkflowAccessResource.hasWriteAccess(wid, user.getUid)) { - throw new ForbiddenException("No sufficient access privilege.") - } else if (!workflowOfUserExists(wid, user.getUid)) { - throw new BadRequestException("The workflow does not exist.") - } else { - val userWorkflow = workflowDao.fetchOneByWid(wid) - userWorkflow.setName(name) - workflowDao.update(userWorkflow) - } + updateWorkflowField(workflow, sessionUser, _.setName(workflow.getName)) } @POST @@ -501,19 +507,7 @@ class WorkflowResource extends LazyLogging { workflow: Workflow, @Auth sessionUser: SessionUser ): Unit = { - val wid = workflow.getWid - val description = workflow.getDescription - val user = sessionUser.getUser - - if (!WorkflowAccessResource.hasWriteAccess(wid, user.getUid)) { - throw new ForbiddenException("No sufficient access privilege.") - } else if (!workflowOfUserExists(wid, user.getUid)) { - throw new BadRequestException("The workflow does not exist.") - } else { - val userWorkflow = workflowDao.fetchOneByWid(wid) - userWorkflow.setDescription(description) - workflowDao.update(userWorkflow) - } + updateWorkflowField(workflow, sessionUser, _.setDescription(workflow.getDescription)) } @PUT From 7588a0a728a5f9650e11a6b677d98fce8b5b5d47 Mon Sep 17 00:00:00 2001 From: Kunwoo Park Date: Thu, 24 Oct 2024 14:50:53 -0700 Subject: [PATCH 5/5] refactor frontend code --- .../user/list-item/list-item.component.ts | 66 +++++++++++-------- 1 file changed, 38 insertions(+), 28 deletions(-) diff --git a/core/gui/src/app/dashboard/component/user/list-item/list-item.component.ts b/core/gui/src/app/dashboard/component/user/list-item/list-item.component.ts index e8f068b0ce6..897a779bb43 100644 --- a/core/gui/src/app/dashboard/component/user/list-item/list-item.component.ts +++ b/core/gui/src/app/dashboard/component/user/list-item/list-item.component.ts @@ -186,46 +186,56 @@ export class ListItemComponent implements OnInit, OnChanges { }, 0); } - public confirmUpdateWorkflowCustomName(name: string): void { - const workflowName = name || DEFAULT_WORKFLOW_NAME; - - this.workflowPersistService - .updateWorkflowName(this.entry.id, workflowName) + private updateWorkflowProperty( + updateMethod: (id: number | undefined, value: string) => any, + propertyName: "name" | "description", + newValue: string, + originalValue: string | undefined + ): void { + updateMethod(this.entry.id, newValue) .pipe(untilDestroyed(this)) .subscribe({ next: () => { - this.entry.name = workflowName; + this.entry[propertyName] = newValue; }, error: (err: unknown) => { - console.error("Failed to update workflow name:", err); - this.entry.name = this.originalName; - this.editingName = false; + console.error(`Failed to update workflow ${propertyName}:`, err); + // Use a fallback empty string if originalValue is undefined + this.entry[propertyName] = originalValue ?? ""; + this.setEditingState(propertyName, false); }, complete: () => { - this.editingName = false; + this.setEditingState(propertyName, false); }, }); } - public confirmUpdateWorkflowCustomDescription(description: string | undefined): void { - const updatedDescription = description !== undefined ? description : ""; + private setEditingState(propertyName: "name" | "description", state: boolean): void { + if (propertyName === "name") { + this.editingName = state; + } else if (propertyName === "description") { + this.editingDescription = state; + } + } - this.workflowPersistService - .updateWorkflowDescription(this.entry.id, updatedDescription) - .pipe(untilDestroyed(this)) - .subscribe({ - next: () => { - this.entry.description = updatedDescription; - }, - error: (err: unknown) => { - console.error("Failed to update workflow description:", err); - this.entry.description = this.originalDescription; - this.editingDescription = false; - }, - complete: () => { - this.editingDescription = false; - }, - }); + public confirmUpdateWorkflowCustomName(name: string): void { + const workflowName = name || DEFAULT_WORKFLOW_NAME; + this.updateWorkflowProperty( + this.workflowPersistService.updateWorkflowName.bind(this.workflowPersistService), + "name", + workflowName, + this.originalName + ); + } + + public confirmUpdateWorkflowCustomDescription(description: string | undefined): void { + const updatedDescription = description ?? ""; + this.updateWorkflowProperty( + this.workflowPersistService.updateWorkflowDescription.bind(this.workflowPersistService), + "description", + updatedDescription, + this.originalDescription + ); } formatTime(timestamp: number | undefined): string {