pierrejeambrun commented on code in PR #60591:
URL: https://github.com/apache/airflow/pull/60591#discussion_r2769229379
##########
airflow-core/src/airflow/ui/src/components/Clear/TaskInstance/ClearTaskInstanceDialog.tsx:
##########
Review Comment:
We need to update the note of all mapped task instances. This is just
plainly skipping the note I believe when clearing all task instances with a
note.
##########
airflow-core/src/airflow/ui/src/components/Clear/TaskInstance/ClearTaskInstanceDialog.tsx:
##########
@@ -35,19 +35,34 @@ import { isStatePending, useAutoRefresh } from "src/utils";
import ClearTaskInstanceConfirmationDialog from
"./ClearTaskInstanceConfirmationDialog";
type Props = {
+ readonly allMapped?: boolean;
+ readonly dagId?: string;
+ readonly dagRunId?: string;
+ readonly mapIndex?: number;
readonly onClose: () => void;
readonly open: boolean;
- readonly taskInstance: TaskInstanceResponse;
+ readonly taskId?: string;
+ readonly taskInstance?: TaskInstanceResponse;
};
-const ClearTaskInstanceDialog = ({ onClose: onCloseDialog, open: openDialog,
taskInstance }: Props) => {
- const taskId = taskInstance.task_id;
- const mapIndex = taskInstance.map_index;
+const ClearTaskInstanceDialog = ({
+ allMapped = false,
+ dagId: propDagId,
+ dagRunId: propDagRunId,
+ mapIndex: propMapIndex,
+ onClose: onCloseDialog,
+ open: openDialog,
+ taskId: propTaskId,
+ taskInstance
+}: Props) => {
const { t: translate } = useTranslation();
const { onClose, onOpen, open } = useDisclosure();
- const dagId = taskInstance.dag_id;
- const dagRunId = taskInstance.dag_run_id;
+ // Support both taskInstance and explicit props
+ const dagId = propDagId ?? taskInstance?.dag_id ?? "";
+ const dagRunId = propDagRunId ?? taskInstance?.dag_run_id ?? "";
+ const taskId = propTaskId ?? taskInstance?.task_id ?? "";
+ const mapIndex = propMapIndex ?? taskInstance?.map_index ?? -1;
Review Comment:
This is confusing and we should probably support only one mode. (TI or
explicit props)
##########
airflow-core/src/airflow/ui/src/components/Clear/TaskInstance/ClearTaskInstanceDialog.tsx:
##########
@@ -65,11 +80,11 @@ const ClearTaskInstanceDialog = ({ onClose: onCloseDialog,
open: openDialog, tas
const [runOnLatestVersion, setRunOnLatestVersion] = useState(false);
const [preventRunningTask, setPreventRunningTask] = useState(true);
- const [note, setNote] = useState<string | null>(taskInstance.note);
+ const [note, setNote] = useState<string | null>(taskInstance?.note ?? null);
const { isPending: isPendingPatchDagRun, mutate: mutatePatchTaskInstance } =
usePatchTaskInstance({
dagId,
dagRunId,
- mapIndex,
+ mapIndex: allMapped ? -1 : mapIndex,
Review Comment:
I think that's wrong. If "allMapped" we should update the note for all of
the mapped instances. And this is done when passing 'None' to the backend,
problaby passing null or undefined there.
##########
airflow-core/src/airflow/ui/testsSetup.ts:
##########
@@ -17,13 +17,43 @@
* under the License.
*/
import "@testing-library/jest-dom/vitest";
+import axios from "axios";
+import httpAdapter from "axios/lib/adapters/http.js";
import { cleanup } from "@testing-library/react";
import type { HttpHandler } from "msw";
import { setupServer, type SetupServerApi } from "msw/node";
import { beforeEach, beforeAll, afterAll, afterEach, vi } from "vitest";
import { handlers } from "src/mocks/handlers";
+// Ensure MSW can intercept axios requests in the happy-dom environment.
+// Axios picks the XHR adapter in browser-like environments; switch to http.
+axios.defaults.adapter = httpAdapter;
+
+const createLocalStorage = () => {
+ const store = new Map<string, string>();
+
+ return {
+ clear: () => store.clear(),
+ getItem: (key: string) => store.get(key) ?? null,
+ key: (index: number) => Array.from(store.keys())[index] ?? null,
+ removeItem: (key: string) => {
+ store.delete(key);
+ },
+ setItem: (key: string, value: string) => {
+ store.set(key, value);
+ },
+ get length() {
+ return store.size;
+ },
+ };
+};
+
+Object.defineProperty(globalThis, "localStorage", {
+ configurable: true,
+ value: createLocalStorage(),
+});
+
Review Comment:
Can you explain why is this code needed? I suppose the test was failing ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
