From 455e0b862f2336f2b1522bb521ea8cb4a89229d0 Mon Sep 17 00:00:00 2001 From: Ahmed Awan Date: Tue, 20 Aug 2024 16:02:28 -0500 Subject: [PATCH 1/2] [24.1] Add error handling and initial loading state to Invocation State Fixes https://github.com/galaxyproject/galaxy/issues/18723 Without this fix, if you get to the `WorkflowInvocationState` route for another user's invocation, there is a 404 error which is unhandled since there is a computed `invocation` ref that we constantly poll. Adding a `initialLoading` ref that ensures the first fetch for the invocation was valid (or the error is caught) fixes this. --- .../WorkflowInvocationState.vue | 30 ++++++++++++++----- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/client/src/components/WorkflowInvocationState/WorkflowInvocationState.vue b/client/src/components/WorkflowInvocationState/WorkflowInvocationState.vue index c71970e3691c..002ba22ed833 100644 --- a/client/src/components/WorkflowInvocationState/WorkflowInvocationState.vue +++ b/client/src/components/WorkflowInvocationState/WorkflowInvocationState.vue @@ -11,6 +11,7 @@ import { useAnimationFrameResizeObserver } from "@/composables/sensors/animation import { useInvocationStore } from "@/stores/invocationStore"; import { useWorkflowStore } from "@/stores/workflowStore"; import localize from "@/utils/localization"; +import { errorMessageAsString } from "@/utils/simple-error"; import { cancelWorkflowScheduling } from "./services"; import { isTerminal, jobCount, runningCount } from "./util"; @@ -49,6 +50,8 @@ const invocationStore = useInvocationStore(); const stepStatesInterval = ref(undefined); const jobStatesInterval = ref(undefined); +const initialLoading = ref(true); +const errorMessage = ref(null); // after the report tab is first activated, no longer lazy-render it from then on const reportActive = ref(false); @@ -69,8 +72,10 @@ useAnimationFrameResizeObserver(scrollableDiv, ({ clientSize, scrollSize }) => { isScrollable.value = scrollSize.height >= clientSize.height + 1; }); -const invocation = computed( - () => invocationStore.getInvocationById(props.invocationId) as WorkflowInvocationElementView +const invocation = computed(() => + !initialLoading.value + ? (invocationStore.getInvocationById(props.invocationId) as WorkflowInvocationElementView) + : null ); const invocationState = computed(() => invocation.value?.state || "new"); const invocationAndJobTerminal = computed(() => invocationSchedulingTerminal.value && jobStatesTerminal.value); @@ -105,9 +110,17 @@ const workflowStore = useWorkflowStore(); const isDeletedWorkflow = computed(() => getWorkflow()?.deleted === true); const workflowVersion = computed(() => getWorkflow()?.version); -onMounted(() => { - pollStepStatesUntilTerminal(); - pollJobStatesUntilTerminal(); +onMounted(async () => { + try { + await invocationStore.fetchInvocationForId({ id: props.invocationId }); + initialLoading.value = false; + if (invocation.value) { + await pollStepStatesUntilTerminal(); + await pollJobStatesUntilTerminal(); + } + } catch (e) { + errorMessage.value = errorMessageAsString(e); + } }); onUnmounted(() => { @@ -116,7 +129,7 @@ onUnmounted(() => { }); async function pollStepStatesUntilTerminal() { - if (!invocation.value || !invocationSchedulingTerminal.value) { + if (!invocationSchedulingTerminal.value) { await invocationStore.fetchInvocationForId({ id: props.invocationId }); stepStatesInterval.value = setTimeout(pollStepStatesUntilTerminal, 3000); } @@ -152,7 +165,7 @@ function getWorkflowId() { } function getWorkflowName() { - return workflowStore.getStoredWorkflowNameByInstanceId(invocation.value?.workflow_id); + return workflowStore.getStoredWorkflowNameByInstanceId(invocation.value?.workflow_id || ""); } @@ -261,6 +274,9 @@ function getWorkflowName() { + + {{ errorMessage }} + From 303577c531dffeeca12e2b3e5c0d6de0535c72a2 Mon Sep 17 00:00:00 2001 From: Ahmed Awan Date: Tue, 20 Aug 2024 18:55:30 -0500 Subject: [PATCH 2/2] fix/add jest for `WorkflowInvocationState` for error handling fix --- .../WorkflowInvocationState.test.ts | 42 +++++++++++++++---- .../WorkflowInvocationState.vue | 9 +++- 2 files changed, 40 insertions(+), 11 deletions(-) diff --git a/client/src/components/WorkflowInvocationState/WorkflowInvocationState.test.ts b/client/src/components/WorkflowInvocationState/WorkflowInvocationState.test.ts index a57261db8110..93b3a22c9b09 100644 --- a/client/src/components/WorkflowInvocationState/WorkflowInvocationState.test.ts +++ b/client/src/components/WorkflowInvocationState/WorkflowInvocationState.test.ts @@ -51,7 +51,11 @@ const invocationJobsSummaryById = { // Mock the invocation store to return the expected invocation data given the invocation ID jest.mock("@/stores/invocationStore", () => { const originalModule = jest.requireActual("@/stores/invocationStore"); - const mockFetchInvocationForId = jest.fn(); + const mockFetchInvocationForId = jest.fn().mockImplementation((fetchParams) => { + if (fetchParams.id === "error-invocation") { + throw new Error("User does not own specified item."); + } + }); const mockFetchInvocationJobsSummaryForId = jest.fn(); return { ...originalModule, @@ -106,8 +110,8 @@ describe("WorkflowInvocationState check invocation and job terminal states", () const wrapper = await mountWorkflowInvocationState(invocationData.id); expect(isInvocationAndJobTerminal(wrapper)).toBe(true); - // Neither the invocation nor the jobs summary should be fetched for terminal invocations - assertInvocationFetched(0); + // Invocation is fetched once and the jobs summary isn't fetched at all for terminal invocations + assertInvocationFetched(1); assertJobsSummaryFetched(0); }); @@ -115,17 +119,23 @@ describe("WorkflowInvocationState check invocation and job terminal states", () const wrapper = await mountWorkflowInvocationState("not-fetched-invocation"); expect(isInvocationAndJobTerminal(wrapper)).toBe(false); - // Both, the invocation and jobs summary should be fetched once if the invocation is not in the store + // Invocation is fetched once and the jobs summary is then never fetched if the invocation is not in the store assertInvocationFetched(1); - assertJobsSummaryFetched(1); + assertJobsSummaryFetched(0); + + // expect there to be an alert for the missing invocation + const alert = wrapper.find("balert-stub"); + expect(alert.attributes("variant")).toBe("info"); + const span = alert.find("span"); + expect(span.text()).toBe("Invocation not found."); }); it("determines that invocation is not terminal with non-terminal state", async () => { const wrapper = await mountWorkflowInvocationState("non-terminal-id"); expect(isInvocationAndJobTerminal(wrapper)).toBe(false); - // Only the invocation should be fetched for non-terminal invocations - assertInvocationFetched(1); + // Only the invocation is fetched for non-terminal invocations; once for the initial fetch and then for the polling + assertInvocationFetched(2); assertJobsSummaryFetched(0); }); @@ -133,10 +143,24 @@ describe("WorkflowInvocationState check invocation and job terminal states", () const wrapper = await mountWorkflowInvocationState("non-terminal-jobs"); expect(isInvocationAndJobTerminal(wrapper)).toBe(false); - // Only the jobs summary should be fetched, not the invocation since it is in scheduled/terminal state - assertInvocationFetched(0); + // Only the jobs summary should be polled, the invocation is initially fetched only since it is in scheduled/terminal state + assertInvocationFetched(1); assertJobsSummaryFetched(1); }); + + it("determines that errored invocation fetches are handled correctly", async () => { + const wrapper = await mountWorkflowInvocationState("error-invocation"); + expect(isInvocationAndJobTerminal(wrapper)).toBe(false); + + // Invocation is fetched once and the jobs summary isn't fetched at all for errored invocations + assertInvocationFetched(1); + assertJobsSummaryFetched(0); + + // expect there to be an alert for the handled error + const alert = wrapper.find("balert-stub"); + expect(alert.attributes("variant")).toBe("danger"); + expect(alert.text()).toBe("User does not own specified item."); + }); }); describe("WorkflowInvocationState check 'Report' tab disabled state", () => { diff --git a/client/src/components/WorkflowInvocationState/WorkflowInvocationState.vue b/client/src/components/WorkflowInvocationState/WorkflowInvocationState.vue index 002ba22ed833..b1e590cd203d 100644 --- a/client/src/components/WorkflowInvocationState/WorkflowInvocationState.vue +++ b/client/src/components/WorkflowInvocationState/WorkflowInvocationState.vue @@ -73,7 +73,7 @@ useAnimationFrameResizeObserver(scrollableDiv, ({ clientSize, scrollSize }) => { }); const invocation = computed(() => - !initialLoading.value + !initialLoading.value && !errorMessage.value ? (invocationStore.getInvocationById(props.invocationId) as WorkflowInvocationElementView) : null ); @@ -120,6 +120,8 @@ onMounted(async () => { } } catch (e) { errorMessage.value = errorMessageAsString(e); + } finally { + initialLoading.value = false; } }); @@ -274,10 +276,13 @@ function getWorkflowName() { + + + {{ errorMessage }} - + Invocation not found.