From 9c859bb8058ae5441de1b8b7087666dfb7d46a77 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 16 Jul 2024 12:08:53 +0200 Subject: [PATCH 01/11] Add startedAt timestamp to persistent progress monitor This will help in determining when the task might have expired. --- .../Common/PersistentTaskProgressMonitorAlert.vue | 7 ++++++- client/src/composables/persistentProgressMonitor.ts | 5 +++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/client/src/components/Common/PersistentTaskProgressMonitorAlert.vue b/client/src/components/Common/PersistentTaskProgressMonitorAlert.vue index 10af39cedcdb..80aee45e0437 100644 --- a/client/src/components/Common/PersistentTaskProgressMonitorAlert.vue +++ b/client/src/components/Common/PersistentTaskProgressMonitorAlert.vue @@ -66,7 +66,12 @@ watch( () => props.taskId, (newTaskId, oldTaskId) => { if (newTaskId && newTaskId !== oldTaskId) { - start({ taskId: newTaskId, taskType: props.monitorRequest.taskType, request: props.monitorRequest }); + start({ + taskId: newTaskId, + taskType: props.monitorRequest.taskType, + request: props.monitorRequest, + startedAt: new Date(), + }); } } ); diff --git a/client/src/composables/persistentProgressMonitor.ts b/client/src/composables/persistentProgressMonitor.ts index f339cac411b4..daf044add4c1 100644 --- a/client/src/composables/persistentProgressMonitor.ts +++ b/client/src/composables/persistentProgressMonitor.ts @@ -80,6 +80,11 @@ export interface MonitoringData { * The information about the task. */ request: MonitoringRequest; + + /** + * The time when the task was started. + */ + startedAt: Date; } /** From 60da91d760ecca2a9137990785c06313321f4777 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 16 Jul 2024 12:11:48 +0200 Subject: [PATCH 02/11] Add status field to MonitoringData interface The `status` field represents the status of the task when it was last checked, and can hold either a status string or an error message. This change allows to skip unnecessary monitoring if task was already finalized. --- .../composables/persistentProgressMonitor.ts | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/client/src/composables/persistentProgressMonitor.ts b/client/src/composables/persistentProgressMonitor.ts index daf044add4c1..8fff7c4c1dc6 100644 --- a/client/src/composables/persistentProgressMonitor.ts +++ b/client/src/composables/persistentProgressMonitor.ts @@ -1,5 +1,5 @@ import { StorageSerializers, useLocalStorage } from "@vueuse/core"; -import { computed } from "vue"; +import { computed, watch } from "vue"; import type { TaskMonitor } from "./genericTaskMonitor"; @@ -85,6 +85,13 @@ export interface MonitoringData { * The time when the task was started. */ startedAt: Date; + + /** + * The status of the task when it was last checked. + * The meaning of the status string is up to the monitor implementation. + * In case of an error, this will be the error message. + */ + status?: string; } /** @@ -110,6 +117,19 @@ export function usePersistentProgressTaskMonitor( const { waitForTask, isRunning, isCompleted, hasFailed, requestHasFailed, status } = useMonitor; + watch( + status, + (newStatus) => { + if (currentMonitoringData.value) { + currentMonitoringData.value = { + ...currentMonitoringData.value, + status: newStatus, + }; + } + }, + { immediate: true } + ); + async function start(monitoringData?: MonitoringData) { if (monitoringData) { currentMonitoringData.value = monitoringData; From c1abfb2687d1e9d015275ee6467ac7387c8208ef Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 16 Jul 2024 13:21:13 +0200 Subject: [PATCH 03/11] Add isFinalState method to TaskMonitor interface --- client/src/composables/genericTaskMonitor.ts | 12 ++++++++ .../shortTermStorageMonitor.test.ts | 28 +++++++++++++++++++ client/src/composables/taskMonitor.test.ts | 26 +++++++++++++++++ 3 files changed, 66 insertions(+) diff --git a/client/src/composables/genericTaskMonitor.ts b/client/src/composables/genericTaskMonitor.ts index 69236f44dab8..c24e1b2eff2d 100644 --- a/client/src/composables/genericTaskMonitor.ts +++ b/client/src/composables/genericTaskMonitor.ts @@ -43,6 +43,13 @@ export interface TaskMonitor { * In case of an error, this will be the error message. */ status: Readonly>; + + /** + * Determines if the status represents a final state. + * @param status The status string to check. + * @returns True if the status is a final state and is not expected to change. + */ + isFinalState: (status?: string) => boolean; } /** @@ -78,6 +85,10 @@ export function useGenericMonitor(options: { const isCompleted = computed(() => options.completedCondition(status.value)); const hasFailed = computed(() => options.failedCondition(status.value)); + function isFinalState(status?: string) { + return options.completedCondition(status) || options.failedCondition(status); + } + async function waitForTask(taskId: string, pollDelayInMs?: number) { pollDelay = pollDelayInMs ?? pollDelay; resetState(); @@ -130,6 +141,7 @@ export function useGenericMonitor(options: { return { waitForTask, + isFinalState, isRunning: readonly(isRunning), isCompleted: readonly(isCompleted), hasFailed: readonly(hasFailed), diff --git a/client/src/composables/shortTermStorageMonitor.test.ts b/client/src/composables/shortTermStorageMonitor.test.ts index 68b7700563df..43b91cf4b0e9 100644 --- a/client/src/composables/shortTermStorageMonitor.test.ts +++ b/client/src/composables/shortTermStorageMonitor.test.ts @@ -60,4 +60,32 @@ describe("useShortTermStorageMonitor", () => { expect(isCompleted.value).toBe(false); expect(status.value).toBe("Request failed"); }); + + describe("isFinalState", () => { + it("should indicate is final state when the task is completed", async () => { + const { waitForTask, isFinalState, isRunning, isCompleted, hasFailed, status } = + useShortTermStorageMonitor(); + + expect(isFinalState(status.value)).toBe(false); + waitForTask(COMPLETED_TASK_ID); + await flushPromises(); + expect(isFinalState(status.value)).toBe(true); + expect(isRunning.value).toBe(false); + expect(isCompleted.value).toBe(true); + expect(hasFailed.value).toBe(false); + }); + + it("should indicate is final state when the task has failed", async () => { + const { waitForTask, isFinalState, isRunning, isCompleted, hasFailed, status } = + useShortTermStorageMonitor(); + + expect(isFinalState(status.value)).toBe(false); + waitForTask(REQUEST_FAILED_TASK_ID); + await flushPromises(); + expect(isFinalState(status.value)).toBe(true); + expect(isRunning.value).toBe(false); + expect(isCompleted.value).toBe(false); + expect(hasFailed.value).toBe(true); + }); + }); }); diff --git a/client/src/composables/taskMonitor.test.ts b/client/src/composables/taskMonitor.test.ts index 13af327107d1..d99085806a45 100644 --- a/client/src/composables/taskMonitor.test.ts +++ b/client/src/composables/taskMonitor.test.ts @@ -75,4 +75,30 @@ describe("useTaskMonitor", () => { expect(isCompleted.value).toBe(false); expect(status.value).toBe("Request failed"); }); + + describe("isFinalState", () => { + it("should indicate is final state when the task is completed", async () => { + const { waitForTask, isFinalState, isRunning, isCompleted, hasFailed, status } = useTaskMonitor(); + + expect(isFinalState(status.value)).toBe(false); + waitForTask(COMPLETED_TASK_ID); + await flushPromises(); + expect(isFinalState(status.value)).toBe(true); + expect(isRunning.value).toBe(false); + expect(isCompleted.value).toBe(true); + expect(hasFailed.value).toBe(false); + }); + + it("should indicate is final state when the task has failed", async () => { + const { waitForTask, isFinalState, isRunning, isCompleted, hasFailed, status } = useTaskMonitor(); + + expect(isFinalState(status.value)).toBe(false); + waitForTask(FAILED_TASK_ID); + await flushPromises(); + expect(isFinalState(status.value)).toBe(true); + expect(isRunning.value).toBe(false); + expect(isCompleted.value).toBe(false); + expect(hasFailed.value).toBe(true); + }); + }); }); From 3785f2262594c8f7aca83d6adc997638107aff6e Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 16 Jul 2024 14:06:47 +0200 Subject: [PATCH 04/11] Add loadStatus method to TaskMonitor The loadStatus method allows loading the status of a task from a stored value. --- client/src/composables/genericTaskMonitor.ts | 11 +++++++++++ .../src/composables/shortTermStorageMonitor.test.ts | 12 ++++++++++++ client/src/composables/taskMonitor.test.ts | 12 ++++++++++++ 3 files changed, 35 insertions(+) diff --git a/client/src/composables/genericTaskMonitor.ts b/client/src/composables/genericTaskMonitor.ts index c24e1b2eff2d..e6c9227d17b9 100644 --- a/client/src/composables/genericTaskMonitor.ts +++ b/client/src/composables/genericTaskMonitor.ts @@ -44,6 +44,12 @@ export interface TaskMonitor { */ status: Readonly>; + /** + * Loads the status of the task from a stored value. + * @param storedStatus The status string to load. + */ + loadStatus: (storedStatus: string) => void; + /** * Determines if the status represents a final state. * @param status The status string to check. @@ -89,6 +95,10 @@ export function useGenericMonitor(options: { return options.completedCondition(status) || options.failedCondition(status); } + function loadStatus(storedStatus: string) { + status.value = storedStatus; + } + async function waitForTask(taskId: string, pollDelayInMs?: number) { pollDelay = pollDelayInMs ?? pollDelay; resetState(); @@ -142,6 +152,7 @@ export function useGenericMonitor(options: { return { waitForTask, isFinalState, + loadStatus, isRunning: readonly(isRunning), isCompleted: readonly(isCompleted), hasFailed: readonly(hasFailed), diff --git a/client/src/composables/shortTermStorageMonitor.test.ts b/client/src/composables/shortTermStorageMonitor.test.ts index 43b91cf4b0e9..d2f3dfe66340 100644 --- a/client/src/composables/shortTermStorageMonitor.test.ts +++ b/client/src/composables/shortTermStorageMonitor.test.ts @@ -61,6 +61,18 @@ describe("useShortTermStorageMonitor", () => { expect(status.value).toBe("Request failed"); }); + it("should load the status from the stored monitoring data", async () => { + const { loadStatus, isRunning, isCompleted, hasFailed, status } = useShortTermStorageMonitor(); + const storedStatus = "READY"; + + loadStatus(storedStatus); + + expect(status.value).toBe(storedStatus); + expect(isRunning.value).toBe(false); + expect(isCompleted.value).toBe(true); + expect(hasFailed.value).toBe(false); + }); + describe("isFinalState", () => { it("should indicate is final state when the task is completed", async () => { const { waitForTask, isFinalState, isRunning, isCompleted, hasFailed, status } = diff --git a/client/src/composables/taskMonitor.test.ts b/client/src/composables/taskMonitor.test.ts index d99085806a45..86fc4eab868c 100644 --- a/client/src/composables/taskMonitor.test.ts +++ b/client/src/composables/taskMonitor.test.ts @@ -76,6 +76,18 @@ describe("useTaskMonitor", () => { expect(status.value).toBe("Request failed"); }); + it("should load the status from the stored monitoring data", async () => { + const { loadStatus, isRunning, isCompleted, hasFailed, status } = useTaskMonitor(); + const storedStatus = "SUCCESS"; + + loadStatus(storedStatus); + + expect(status.value).toBe(storedStatus); + expect(isRunning.value).toBe(false); + expect(isCompleted.value).toBe(true); + expect(hasFailed.value).toBe(false); + }); + describe("isFinalState", () => { it("should indicate is final state when the task is completed", async () => { const { waitForTask, isFinalState, isRunning, isCompleted, hasFailed, status } = useTaskMonitor(); From f429b5ee16caf79d781ed17b490ca297332a5ec4 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 16 Jul 2024 14:09:45 +0200 Subject: [PATCH 05/11] Improve usePersistentProgressTaskMonitor This allows skipping unnecessary monitoring if the task has already been finalized and we can load the stored state. --- client/src/composables/persistentProgressMonitor.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/client/src/composables/persistentProgressMonitor.ts b/client/src/composables/persistentProgressMonitor.ts index 8fff7c4c1dc6..6669eecfcce0 100644 --- a/client/src/composables/persistentProgressMonitor.ts +++ b/client/src/composables/persistentProgressMonitor.ts @@ -115,12 +115,13 @@ export function usePersistentProgressTaskMonitor( return Boolean(currentMonitoringData.value); }); - const { waitForTask, isRunning, isCompleted, hasFailed, requestHasFailed, status } = useMonitor; + const { waitForTask, isFinalState, loadStatus, isRunning, isCompleted, hasFailed, requestHasFailed, status } = + useMonitor; watch( status, (newStatus) => { - if (currentMonitoringData.value) { + if (newStatus && currentMonitoringData.value) { currentMonitoringData.value = { ...currentMonitoringData.value, status: newStatus, @@ -139,6 +140,12 @@ export function usePersistentProgressTaskMonitor( throw new Error("No monitoring data provided or stored. Cannot start monitoring progress."); } + if (isFinalState(currentMonitoringData.value.status)) { + // The task has already finished no need to start monitoring again. + // Instead, reload the stored status to update the UI. + return loadStatus(currentMonitoringData.value.status!); + } + return waitForTask(currentMonitoringData.value.taskId); } From d0dfc21931c9fac67796079de6592074769bb32d Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 16 Jul 2024 14:35:39 +0200 Subject: [PATCH 06/11] Add some more docs to persistent progress monitor --- .../composables/persistentProgressMonitor.ts | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/client/src/composables/persistentProgressMonitor.ts b/client/src/composables/persistentProgressMonitor.ts index 6669eecfcce0..a80b42c19fe5 100644 --- a/client/src/composables/persistentProgressMonitor.ts +++ b/client/src/composables/persistentProgressMonitor.ts @@ -169,12 +169,42 @@ export function usePersistentProgressTaskMonitor( * Clears the monitoring data in the local storage. */ reset, + + /** + * The task is still running. + */ isRunning, + + /** + * The task has been completed successfully. + */ isCompleted, + + /** + * Indicates the task has failed and will not yield results. + */ hasFailed, + + /** + * If true, the status of the task cannot be determined because of a request error. + */ requestHasFailed, + + /** + * Indicates that there is monitoring data stored. + */ hasMonitoringData, + + /** + * The task ID stored in the monitoring data or undefined if no monitoring data is available. + */ storedTaskId: currentMonitoringData.value?.taskId, + + /** + * The current status of the task. + * The meaning of the status string is up to the monitor implementation. + * In case of an error, this will be the error message. + */ status, }; } From 3aade1ed6dd8b8a2a310b0547daf3da26904cf33 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 16 Jul 2024 18:18:05 +0200 Subject: [PATCH 07/11] Add expiration time to task monitors The `expirationTime` specifies the time (in milliseconds) after which a task should be considered expired. Requests to check the task status after this time should be avoided as they are not guaranteed to return the correct status. This is to avoid checking for tasks that were completed and then cleaned up from the tasks storage backend which will report the task as "pending" in the case of Celery tasks as they will be unknown after cleanup https://celery-safwan.readthedocs.io/en/latest/userguide/tasks.html?highlight=pending#pending --- .../PersistentTaskProgressMonitorAlert.vue | 23 +++++++- client/src/composables/genericTaskMonitor.ts | 13 +++++ .../composables/persistentProgressMonitor.ts | 54 ++++++++++++++++++- .../composables/shortTermStorageMonitor.ts | 2 + client/src/composables/taskMonitor.ts | 2 + 5 files changed, 90 insertions(+), 4 deletions(-) diff --git a/client/src/components/Common/PersistentTaskProgressMonitorAlert.vue b/client/src/components/Common/PersistentTaskProgressMonitorAlert.vue index 80aee45e0437..351715dde820 100644 --- a/client/src/components/Common/PersistentTaskProgressMonitorAlert.vue +++ b/client/src/components/Common/PersistentTaskProgressMonitorAlert.vue @@ -9,6 +9,8 @@ import { TaskMonitor } from "@/composables/genericTaskMonitor"; import { MonitoringRequest, usePersistentProgressTaskMonitor } from "@/composables/persistentProgressMonitor"; import { useShortTermStorage } from "@/composables/shortTermStorage"; +import UtcDate from "@/components/UtcDate.vue"; + library.add(faSpinner); interface Props { @@ -46,8 +48,19 @@ const props = withDefaults(defineProps(), { const { getDownloadObjectUrl } = useShortTermStorage(); -const { hasMonitoringData, isRunning, isCompleted, hasFailed, requestHasFailed, storedTaskId, status, start, reset } = - usePersistentProgressTaskMonitor(props.monitorRequest, props.useMonitor); +const { + hasMonitoringData, + isRunning, + isCompleted, + hasFailed, + requestHasFailed, + storedTaskId, + status, + hasExpired, + expirationDate, + start, + reset, +} = usePersistentProgressTaskMonitor(props.monitorRequest, props.useMonitor); const downloadUrl = computed(() => { // We can only download the result if the task is completed and the task type is short_term_storage. @@ -98,11 +111,17 @@ function dismissAlert() { {{ inProgressMessage }} + + The {{ monitorRequest.action }} task has expired and the result is no longer available. + {{ completedMessage }} Download here +

+ This result will expire . +

{{ failedMessage }} diff --git a/client/src/composables/genericTaskMonitor.ts b/client/src/composables/genericTaskMonitor.ts index e6c9227d17b9..cb0c1f7c7137 100644 --- a/client/src/composables/genericTaskMonitor.ts +++ b/client/src/composables/genericTaskMonitor.ts @@ -56,6 +56,12 @@ export interface TaskMonitor { * @returns True if the status is a final state and is not expected to change. */ isFinalState: (status?: string) => boolean; + + /** + * If defined, the time (in milliseconds) after which the task should be considered expired. + * Requests to check the task status after this time should be avoided. As they are not guaranteed to return the correct status. + */ + expirationTime?: number; } /** @@ -79,6 +85,12 @@ export function useGenericMonitor(options: { * The delay can be overridden when calling `waitForTask`. */ defaultPollDelay?: number; + + /** Optional expiration time for the task in milliseconds. + * After this time, the task should be considered expired. + * Requests to check the task status after this time should be avoided. + */ + expirationTime?: number; }): TaskMonitor { let timeout: NodeJS.Timeout | null = null; let pollDelay = options.defaultPollDelay ?? DEFAULT_POLL_DELAY; @@ -158,5 +170,6 @@ export function useGenericMonitor(options: { hasFailed: readonly(hasFailed), requestHasFailed: readonly(requestHasFailed), status: readonly(status), + expirationTime: options.expirationTime, }; } diff --git a/client/src/composables/persistentProgressMonitor.ts b/client/src/composables/persistentProgressMonitor.ts index a80b42c19fe5..4cfdd300173c 100644 --- a/client/src/composables/persistentProgressMonitor.ts +++ b/client/src/composables/persistentProgressMonitor.ts @@ -115,8 +115,41 @@ export function usePersistentProgressTaskMonitor( return Boolean(currentMonitoringData.value); }); - const { waitForTask, isFinalState, loadStatus, isRunning, isCompleted, hasFailed, requestHasFailed, status } = - useMonitor; + const canExpire = computed(() => { + return Boolean(expirationTime); + }); + + const hasExpired = computed(() => { + if (!currentMonitoringData.value || !expirationTime) { + return false; + } + + const now = new Date(); + const startedAt = new Date(currentMonitoringData.value.startedAt); + const elapsedTimeInMs = now.getTime() - startedAt.getTime(); + return elapsedTimeInMs > expirationTime!; + }); + + const expirationDate = computed(() => { + if (!currentMonitoringData.value || !expirationTime) { + return undefined; + } + + const startedAt = new Date(currentMonitoringData.value.startedAt); + return new Date(startedAt.getTime() + expirationTime); + }); + + const { + waitForTask, + isFinalState, + loadStatus, + isRunning, + isCompleted, + hasFailed, + requestHasFailed, + status, + expirationTime, + } = useMonitor; watch( status, @@ -206,5 +239,22 @@ export function usePersistentProgressTaskMonitor( * In case of an error, this will be the error message. */ status, + + /** + * True if the monitoring data can expire. + */ + canExpire, + + /** + * True if the monitoring data has expired. + * The monitoring data expires after the expiration time has passed since the task was started. + */ + hasExpired, + + /** + * The expiration date for the monitoring data. + * After this date, the monitoring data is considered expired and should not be used. + */ + expirationDate, }; } diff --git a/client/src/composables/shortTermStorageMonitor.ts b/client/src/composables/shortTermStorageMonitor.ts index a229737204f0..fd45a1810d48 100644 --- a/client/src/composables/shortTermStorageMonitor.ts +++ b/client/src/composables/shortTermStorageMonitor.ts @@ -3,6 +3,7 @@ import { fetcher } from "@/api/schema"; import { useGenericMonitor } from "./genericTaskMonitor"; const DEFAULT_POLL_DELAY = 10000; +const DEFAULT_EXPIRATION_TIME = 1000 * 60 * 60 * 24; // 24 hours const getTempStorageRequestReady = fetcher .path("/api/short_term_storage/{storage_request_id}/ready") @@ -27,5 +28,6 @@ export function useShortTermStorageMonitor() { completedCondition: (status?: string) => status === READY_STATE, failedCondition: (status?: string) => typeof status === "string" && !VALID_STATES.includes(status), defaultPollDelay: DEFAULT_POLL_DELAY, + expirationTime: DEFAULT_EXPIRATION_TIME, }); } diff --git a/client/src/composables/taskMonitor.ts b/client/src/composables/taskMonitor.ts index 4f2aad634ea3..f71d4b973209 100644 --- a/client/src/composables/taskMonitor.ts +++ b/client/src/composables/taskMonitor.ts @@ -5,6 +5,7 @@ import { useGenericMonitor } from "./genericTaskMonitor"; const SUCCESS_STATE = "SUCCESS"; const FAILURE_STATE = "FAILURE"; const DEFAULT_POLL_DELAY = 10000; +const DEFAULT_EXPIRATION_TIME = 1000 * 60 * 60 * 24; // 24 hours const getTaskStatus = fetcher.path("/api/tasks/{task_id}/state").method("get").create(); @@ -21,5 +22,6 @@ export function useTaskMonitor() { completedCondition: (status?: string) => status === SUCCESS_STATE, failedCondition: (status?: string) => status === FAILURE_STATE, defaultPollDelay: DEFAULT_POLL_DELAY, + expirationTime: DEFAULT_EXPIRATION_TIME, }); } From 6be54079705961546e48c34e09391537db2f9312 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 16 Jul 2024 18:50:03 +0200 Subject: [PATCH 08/11] Adapt unit tests --- ...PersistentTaskProgressMonitorAlert.test.ts | 10 +++++++- .../persistentProgressMonitor.test.ts | 11 ++++++++- .../composables/persistentProgressMonitor.ts | 24 +++++++++---------- 3 files changed, 31 insertions(+), 14 deletions(-) diff --git a/client/src/components/Common/PersistentTaskProgressMonitorAlert.test.ts b/client/src/components/Common/PersistentTaskProgressMonitorAlert.test.ts index 30be2201aace..a6490898f28b 100644 --- a/client/src/components/Common/PersistentTaskProgressMonitorAlert.test.ts +++ b/client/src/components/Common/PersistentTaskProgressMonitorAlert.test.ts @@ -26,7 +26,10 @@ const FAKE_MONITOR: TaskMonitor = { isCompleted: ref(false), hasFailed: ref(false), requestHasFailed: ref(false), - status: ref(""), + status: ref(), + expirationTime: 1000, + isFinalState: jest.fn(), + loadStatus: jest.fn(), }; const mountComponent = ( @@ -61,6 +64,7 @@ describe("PersistentTaskProgressMonitorAlert.vue", () => { taskId: "1", taskType: "task", request: FAKE_MONITOR_REQUEST, + startedAt: new Date(), }; usePersistentProgressTaskMonitor(FAKE_MONITOR_REQUEST, useMonitor, existingMonitoringData); @@ -85,6 +89,7 @@ describe("PersistentTaskProgressMonitorAlert.vue", () => { taskId: "1", taskType: "task", request: FAKE_MONITOR_REQUEST, + startedAt: new Date(), }; usePersistentProgressTaskMonitor(FAKE_MONITOR_REQUEST, useMonitor, existingMonitoringData); @@ -109,6 +114,7 @@ describe("PersistentTaskProgressMonitorAlert.vue", () => { taskId: "1", taskType: "task", request: FAKE_MONITOR_REQUEST, + startedAt: new Date(), }; usePersistentProgressTaskMonitor(FAKE_MONITOR_REQUEST, useMonitor, existingMonitoringData); @@ -138,6 +144,7 @@ describe("PersistentTaskProgressMonitorAlert.vue", () => { taskId: taskId, taskType: "short_term_storage", request: monitoringRequest, + startedAt: new Date(), }; usePersistentProgressTaskMonitor(monitoringRequest, useMonitor, existingMonitoringData); @@ -166,6 +173,7 @@ describe("PersistentTaskProgressMonitorAlert.vue", () => { taskId: "1", taskType: "task", request: FAKE_MONITOR_REQUEST, + startedAt: new Date(), }; usePersistentProgressTaskMonitor(FAKE_MONITOR_REQUEST, useMonitor, existingMonitoringData); diff --git a/client/src/composables/persistentProgressMonitor.test.ts b/client/src/composables/persistentProgressMonitor.test.ts index 29c5f4eed3e9..c35305d2ed29 100644 --- a/client/src/composables/persistentProgressMonitor.test.ts +++ b/client/src/composables/persistentProgressMonitor.test.ts @@ -20,6 +20,8 @@ jest.mock("@vueuse/core", () => ({ function useMonitorMock(): TaskMonitor { const isRunning = ref(false); + const status = ref(); + return { waitForTask: jest.fn().mockImplementation(() => { isRunning.value = true; @@ -28,7 +30,12 @@ function useMonitorMock(): TaskMonitor { isCompleted: ref(false), hasFailed: ref(false), requestHasFailed: ref(false), - status: ref(""), + status, + expirationTime: 1000, + isFinalState: jest.fn(), + loadStatus(storedStatus) { + status.value = storedStatus; + }, }; } const mockUseMonitor = useMonitorMock(); @@ -55,6 +62,7 @@ describe("usePersistentProgressTaskMonitor", () => { taskId: "123", taskType: "task", request: MOCK_REQUEST, + startedAt: new Date(), }; const { start, isRunning } = usePersistentProgressTaskMonitor(MOCK_REQUEST, mockUseMonitor, monitoringData); @@ -75,6 +83,7 @@ describe("usePersistentProgressTaskMonitor", () => { taskId: "123", taskType: "task", request: MOCK_REQUEST, + startedAt: new Date(), }; const { reset, hasMonitoringData } = usePersistentProgressTaskMonitor( diff --git a/client/src/composables/persistentProgressMonitor.ts b/client/src/composables/persistentProgressMonitor.ts index 4cfdd300173c..414e65d38ab2 100644 --- a/client/src/composables/persistentProgressMonitor.ts +++ b/client/src/composables/persistentProgressMonitor.ts @@ -105,6 +105,18 @@ export function usePersistentProgressTaskMonitor( useMonitor: TaskMonitor, monitoringData: MonitoringData | null = null ) { + const { + waitForTask, + isFinalState, + loadStatus, + isRunning, + isCompleted, + hasFailed, + requestHasFailed, + status, + expirationTime, + } = useMonitor; + const localStorageKey = getPersistentKey(request); const currentMonitoringData = useLocalStorage(localStorageKey, monitoringData, { @@ -139,18 +151,6 @@ export function usePersistentProgressTaskMonitor( return new Date(startedAt.getTime() + expirationTime); }); - const { - waitForTask, - isFinalState, - loadStatus, - isRunning, - isCompleted, - hasFailed, - requestHasFailed, - status, - expirationTime, - } = useMonitor; - watch( status, (newStatus) => { From 17343730d2cfe44b74cfe9fcaccb7243d36b06a6 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 16 Jul 2024 19:02:32 +0200 Subject: [PATCH 09/11] Add test for expired task alert --- ...PersistentTaskProgressMonitorAlert.test.ts | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/client/src/components/Common/PersistentTaskProgressMonitorAlert.test.ts b/client/src/components/Common/PersistentTaskProgressMonitorAlert.test.ts index a6490898f28b..44bc70cfebfa 100644 --- a/client/src/components/Common/PersistentTaskProgressMonitorAlert.test.ts +++ b/client/src/components/Common/PersistentTaskProgressMonitorAlert.test.ts @@ -20,6 +20,8 @@ const FAKE_MONITOR_REQUEST: MonitoringRequest = { description: "Test description", }; +const FAKE_EXPIRATION_TIME = 1000; + const FAKE_MONITOR: TaskMonitor = { waitForTask: jest.fn(), isRunning: ref(false), @@ -27,7 +29,7 @@ const FAKE_MONITOR: TaskMonitor = { hasFailed: ref(false), requestHasFailed: ref(false), status: ref(), - expirationTime: 1000, + expirationTime: FAKE_EXPIRATION_TIME, isFinalState: jest.fn(), loadStatus: jest.fn(), }; @@ -188,4 +190,28 @@ describe("PersistentTaskProgressMonitorAlert.vue", () => { expect(completedAlert.exists()).toBe(true); expect(completedAlert.text()).not.toContain("Download here"); }); + + it("should render a warning alert when the task has expired", () => { + const useMonitor = { + ...FAKE_MONITOR, + }; + const existingMonitoringData: MonitoringData = { + taskId: "1", + taskType: "task", + request: FAKE_MONITOR_REQUEST, + startedAt: new Date(Date.now() - FAKE_EXPIRATION_TIME * 2), // Make sure the task has expired + }; + usePersistentProgressTaskMonitor(FAKE_MONITOR_REQUEST, useMonitor, existingMonitoringData); + + const wrapper = mountComponent({ + monitorRequest: FAKE_MONITOR_REQUEST, + useMonitor, + }); + + expect(wrapper.find(".d-flex").exists()).toBe(true); + + const warningAlert = wrapper.find('[variant="warning"]'); + expect(warningAlert.exists()).toBe(true); + expect(warningAlert.text()).toContain("The testing task has expired and the result is no longer available"); + }); }); From a87408b3a141eb2cdc1b55545a81cb075f795150 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 16 Jul 2024 19:07:18 +0200 Subject: [PATCH 10/11] Improve readability of task expiration message --- .../Common/PersistentTaskProgressMonitorAlert.vue | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/client/src/components/Common/PersistentTaskProgressMonitorAlert.vue b/client/src/components/Common/PersistentTaskProgressMonitorAlert.vue index 351715dde820..4c821e858c1c 100644 --- a/client/src/components/Common/PersistentTaskProgressMonitorAlert.vue +++ b/client/src/components/Common/PersistentTaskProgressMonitorAlert.vue @@ -119,9 +119,10 @@ function dismissAlert() { Download here -

- This result will expire . -

+
+ + This result will expire + {{ failedMessage }} From 0eb7eeeb8a2e2e7c6ab34b9fbc7aea4868b4dd61 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 16 Jul 2024 19:56:29 +0200 Subject: [PATCH 11/11] Ensure task expiration has precedence over any other state --- .../Common/PersistentTaskProgressMonitorAlert.test.ts | 3 ++- .../Common/PersistentTaskProgressMonitorAlert.vue | 8 ++++---- client/src/composables/persistentProgressMonitor.ts | 6 ++++++ 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/client/src/components/Common/PersistentTaskProgressMonitorAlert.test.ts b/client/src/components/Common/PersistentTaskProgressMonitorAlert.test.ts index 44bc70cfebfa..f9e63f915c39 100644 --- a/client/src/components/Common/PersistentTaskProgressMonitorAlert.test.ts +++ b/client/src/components/Common/PersistentTaskProgressMonitorAlert.test.ts @@ -191,9 +191,10 @@ describe("PersistentTaskProgressMonitorAlert.vue", () => { expect(completedAlert.text()).not.toContain("Download here"); }); - it("should render a warning alert when the task has expired", () => { + it("should render a warning alert when the task has expired even if the status is running", () => { const useMonitor = { ...FAKE_MONITOR, + isRunning: ref(true), }; const existingMonitoringData: MonitoringData = { taskId: "1", diff --git a/client/src/components/Common/PersistentTaskProgressMonitorAlert.vue b/client/src/components/Common/PersistentTaskProgressMonitorAlert.vue index 4c821e858c1c..34467f095646 100644 --- a/client/src/components/Common/PersistentTaskProgressMonitorAlert.vue +++ b/client/src/components/Common/PersistentTaskProgressMonitorAlert.vue @@ -107,13 +107,13 @@ function dismissAlert() {