Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -489,18 +489,20 @@ class WorkflowResource extends LazyLogging {
}

/**
* Persists a plain save by updating only the fields the client sends
* (name/description/content/is_public). It deliberately leaves `default_view` untouched --
* that column is owned by /set-default-view alone -- so a save can never clobber a
* concurrent change. Timestamps are likewise not rewritten here.
* Persists a plain save by updating only what a save is: name, description and content.
* `is_public` is not written here. Publishing has its own endpoints (/public, /private), and
* a save payload does not reliably carry the flag: the frontend feeds the saved row straight
* back as its metadata, where the flag has another name, so the very next autosave arrives
* without it. Writing that null violated the column's NOT NULL constraint and every second
* save failed with 500. `default_view` is likewise owned by /set-default-view alone, and the
* timestamps are not rewritten here, so a save can never clobber a concurrent change to either.
*/
private def saveWorkflowFields(workflow: Workflow): Unit = {
context
.update(WORKFLOW)
.set(WORKFLOW.NAME, workflow.getName)
.set(WORKFLOW.DESCRIPTION, workflow.getDescription)
.set(WORKFLOW.CONTENT, workflow.getContent)
.set(WORKFLOW.IS_PUBLIC, workflow.getIsPublic)
.where(WORKFLOW.WID.eq(workflow.getWid))
.execute()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1340,9 +1340,9 @@ class WorkflowResourceSpec
assert(defaultView(wid) == DefaultViewEnum.CANVAS)
}

// A plain save (persistWorkflow) only writes the fields the client sends -- name,
// description, content, is_public -- and never `default_view`, so saving the canvas must
// not reset the default view. The edit payload mirrors what the frontend sends.
// A plain save (persistWorkflow) only writes name, description and content -- never
// `default_view`, so saving the canvas must not reset the default view. The edit payload mirrors
// what the frontend sends.
it should "survive a subsequent save of the workflow" in {
val wid = persistFreshWorkflow("param_survives_save")
workflowResource.setDefaultView(wid, DefaultViewRequest("FORM"), sessionUser1)
Expand All @@ -1351,7 +1351,6 @@ class WorkflowResourceSpec
edit.setWid(wid)
edit.setName("param_survives_save_edited")
edit.setContent("{\"operators\":[],\"links\":[]}")
edit.setIsPublic(false)
workflowResource.persistWorkflow(edit, sessionUser1)

assert(
Expand All @@ -1360,6 +1359,39 @@ class WorkflowResourceSpec
)
}

// The frontend feeds the saved row back as its metadata, where the publish flag has another
// name, so the very next autosave arrives with isPublic unset. A save must neither fail on that
// (the column is NOT NULL, and writing the null made every second save a 500) nor rewrite the
// flag: publishing is /public and /private's job alone.
"/persist API" should "neither fail nor change is_public when the save carries no flag" in {
val wid = persistFreshWorkflow("persist_keeps_public")
workflowResource.makePublic(wid, sessionUser1)

val edit = new Workflow()
edit.setWid(wid)
edit.setName("persist_keeps_public_edited")
edit.setContent("{\"operators\":[],\"links\":[]}")
// isPublic deliberately left null, exactly as the frontend's second save sends it
val saved = workflowResource.persistWorkflow(edit, sessionUser1)

assert(saved.getName == "persist_keeps_public_edited")
assert(saved.getIsPublic, "a plain save must not touch the publish flag")
}

it should "not un-publish a workflow when the save says isPublic = false" in {
val wid = persistFreshWorkflow("persist_ignores_flag")
workflowResource.makePublic(wid, sessionUser1)

val edit = new Workflow()
edit.setWid(wid)
edit.setName("persist_ignores_flag_edited")
edit.setContent("{\"operators\":[],\"links\":[]}")
edit.setIsPublic(false)
val saved = workflowResource.persistWorkflow(edit, sessionUser1)

assert(saved.getIsPublic, "a stale flag on a save must not un-publish the workflow")
}

// A biologist's path is hub -> clone -> use, so a copy has to stay usable.
it should "be inherited by a duplicated workflow" in {
val wid = persistFreshWorkflow("param_source")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -202,19 +202,23 @@ describe("WorkflowPersistService", () => {

const req = httpTestingController.expectOne(`${API}/${WORKFLOW_PERSIST_URL}`);
expect(req.request.method).toBe("POST");
// The publish flag is not part of a save: the endpoint does not read it, and sending a
// stale copy is what used to null the column after the first save.
expect(req.request.body).toEqual({
wid: 9,
name: "my wf",
description: "a description",
content: JSON.stringify(validContent),
isPublic: true,
});

req.flush({ wid: 9, name: "my wf", content: '{"operators":[]}' });
// The saved row comes back with the flag under the backend's name; the response the
// caller sees carries it as isPublished, so metadata fed back from a save stays complete.
req.flush({ wid: 9, name: "my wf", content: '{"operators":[]}', isPublic: true });

// valid workflow -> no error notification, and string content is parsed
expect(errorSpy).not.toHaveBeenCalled();
expect(result?.content).toEqual({ operators: [] });
expect(result?.isPublished).toBe(1);
});

it("persistWorkflow notifies the user when the workflow is broken but still POSTs", () => {
Expand All @@ -235,7 +239,7 @@ describe("WorkflowPersistService", () => {
);

const req = httpTestingController.expectOne(`${API}/${WORKFLOW_PERSIST_URL}`);
expect(req.request.body.isPublic).toBe(false);
expect("isPublic" in req.request.body).toBe(false);
req.flush({ wid: 1, name: "broken", content: '{"operators":[]}' });
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,16 @@ export class WorkflowPersistService {
);
}

// A save carries name, description and content only. The publish flag is not sent: the
// backend does not read it on this endpoint (publishing goes through /public and /private),
// and it is not reliably known here anyway, since the metadata fed back after a save names
// it differently (see WorkflowUtilService.parseWorkflowInfo).
return this.http
.post<Workflow>(`${AppSettings.getApiEndpoint()}/${WORKFLOW_PERSIST_URL}`, {
wid: workflow.wid,
name: workflow.name,
description: workflow.description,
content: JSON.stringify(workflow.content),
isPublic: workflow.isPublished,
})
.pipe(
filter((updatedWorkflow: Workflow) => updatedWorkflow != null),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,21 @@ describe("WorkflowUtilService", () => {
expect(parsed.content).toBe(content);
});

// The persist endpoint returns the stored row, which names the publish flag isPublic; the rest
// of the frontend knows it as isPublished. Without the carry-over, a save fed back as metadata
// lost the flag, and the next save went out without it.
it("should carry the persist response's isPublic over to isPublished", () => {
const fromPersist = { wid: 1, name: "n", content: "{}", isPublic: true } as unknown as Workflow;

expect(WorkflowUtilService.parseWorkflowInfo(fromPersist).isPublished).toBe(1);
});

it("should leave an isPublished the payload already carries alone", () => {
const fromRetrieve = { wid: 1, name: "n", content: "{}", isPublished: 0, isPublic: true } as unknown as Workflow;

expect(WorkflowUtilService.parseWorkflowInfo(fromRetrieve).isPublished).toBe(0);
});

it("should create a fresh comment box at the default position", () => {
const commentBox = workflowUtilService.getNewCommentBox();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,14 @@ export class WorkflowUtilService {
if (workflow != null && typeof workflow.content === "string") {
workflow.content = jsonCast<WorkflowContent>(workflow.content);
}
// The persist endpoint answers with the stored row, whose publish flag is named isPublic;
// every other workflow endpoint, and the Workflow type, call it isPublished. Carry it across,
// or the metadata a save feeds back would silently drop the publish state until the next
// full load.
const stored = workflow as Workflow & { isPublic?: boolean };
if (workflow != null && workflow.isPublished === undefined && stored.isPublic !== undefined) {
workflow.isPublished = Number(stored.isPublic);
}
return workflow;
}

Expand Down
Loading