This is an automated email from the ASF dual-hosted git repository.
bbovenzi pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/airflow.git
The following commit(s) were added to refs/heads/main by this push:
new b9acffa81b Fix for TaskGroup toggles for duplicated labels (#34072)
b9acffa81b is described below
commit b9acffa81bf61dcf0c5553942c52629c7f75ebe2
Author: Usiel Riedl <[email protected]>
AuthorDate: Wed Sep 6 12:23:20 2023 +0200
Fix for TaskGroup toggles for duplicated labels (#34072)
* Fix for TaskGroup toggles for duplicated labels
We have been using `_.label` so far to remember which task groups are
opened/closed. Unfortunately, this leads to issues when the same label is used
twice, which can happen as described in the linked issue.
With this change we switch to use `_.id` instead, which must be unique
(otherwise parsing the DAG would lead to `DuplicateTaskIdFound`).
Fixes #34066
* Fix toggle for Gantt as well
---
airflow/www/static/js/dag/details/gantt/Row.tsx | 2 +-
airflow/www/static/js/dag/details/graph/utils.ts | 4 +-
airflow/www/static/js/dag/grid/TaskName.test.tsx | 12 ++-
airflow/www/static/js/dag/grid/TaskName.tsx | 3 +
airflow/www/static/js/dag/grid/ToggleGroups.tsx | 2 +-
airflow/www/static/js/dag/grid/index.test.tsx | 117 ++++++++++++++++++----
airflow/www/static/js/dag/grid/renderTaskRows.tsx | 11 +-
airflow/www/static/js/utils/graph.ts | 4 +-
8 files changed, 123 insertions(+), 32 deletions(-)
diff --git a/airflow/www/static/js/dag/details/gantt/Row.tsx
b/airflow/www/static/js/dag/details/gantt/Row.tsx
index 304fb83e22..bf1ce7ac60 100644
--- a/airflow/www/static/js/dag/details/gantt/Row.tsx
+++ b/airflow/www/static/js/dag/details/gantt/Row.tsx
@@ -53,7 +53,7 @@ const Row = ({
const instance = task.instances.find((ti) => ti.runId === runId);
const isSelected = taskId === instance?.taskId;
const hasQueuedDttm = !!instance?.queuedDttm;
- const isOpen = openGroupIds.includes(task.label || "");
+ const isOpen = openGroupIds.includes(task.id || "");
// Calculate durations in ms
const taskDuration = getDuration(instance?.startDate, instance?.endDate);
diff --git a/airflow/www/static/js/dag/details/graph/utils.ts
b/airflow/www/static/js/dag/details/graph/utils.ts
index 3d85053619..ac940d46c7 100644
--- a/airflow/www/static/js/dag/details/graph/utils.ts
+++ b/airflow/www/static/js/dag/details/graph/utils.ts
@@ -79,9 +79,9 @@ export const flattenNodes = ({
onToggleCollapse: () => {
let newGroupIds = [];
if (!node.value.isOpen) {
- newGroupIds = [...openGroupIds, node.value.label];
+ newGroupIds = [...openGroupIds, node.id];
} else {
- newGroupIds = openGroupIds.filter((g) => g !== node.value.label);
+ newGroupIds = openGroupIds.filter((g) => g !== node.id);
}
onToggleGroups(newGroupIds);
},
diff --git a/airflow/www/static/js/dag/grid/TaskName.test.tsx
b/airflow/www/static/js/dag/grid/TaskName.test.tsx
index f7a4c7267d..8002ab78ca 100644
--- a/airflow/www/static/js/dag/grid/TaskName.test.tsx
+++ b/airflow/www/static/js/dag/grid/TaskName.test.tsx
@@ -29,7 +29,7 @@ import TaskName from "./TaskName";
describe("Test TaskName", () => {
test("Displays a normal task name", () => {
const { getByText } = render(
- <TaskName label="test" onToggle={() => {}} />,
+ <TaskName label="test" id="test" onToggle={() => {}} />,
{ wrapper: ChakraWrapper }
);
@@ -38,7 +38,13 @@ describe("Test TaskName", () => {
test("Displays a mapped task name", () => {
const { getByText } = render(
- <TaskName level={0} label="test" isMapped onToggle={() => {}} />,
+ <TaskName
+ level={0}
+ label="test"
+ id="test"
+ isMapped
+ onToggle={() => {}}
+ />,
{ wrapper: ChakraWrapper }
);
@@ -47,7 +53,7 @@ describe("Test TaskName", () => {
test("Displays a group task name", () => {
const { getByText, getByTestId } = render(
- <TaskName level={0} label="test" isGroup onToggle={() => {}} />,
+ <TaskName level={0} label="test" id="test" isGroup onToggle={() => {}}
/>,
{ wrapper: ChakraWrapper }
);
diff --git a/airflow/www/static/js/dag/grid/TaskName.tsx
b/airflow/www/static/js/dag/grid/TaskName.tsx
index 030767212f..ce2de10bf6 100644
--- a/airflow/www/static/js/dag/grid/TaskName.tsx
+++ b/airflow/www/static/js/dag/grid/TaskName.tsx
@@ -28,6 +28,7 @@ interface Props {
isOpen?: boolean;
level?: number;
label: string;
+ id: string;
}
const TaskName = ({
@@ -37,11 +38,13 @@ const TaskName = ({
isOpen = false,
level = 0,
label,
+ id,
}: Props) => (
<Flex
as={isGroup ? "button" : "div"}
onClick={onToggle}
aria-label={label}
+ data-testid={id}
title={label}
mr={4}
width="100%"
diff --git a/airflow/www/static/js/dag/grid/ToggleGroups.tsx
b/airflow/www/static/js/dag/grid/ToggleGroups.tsx
index 1473dd13aa..730e2e61a4 100644
--- a/airflow/www/static/js/dag/grid/ToggleGroups.tsx
+++ b/airflow/www/static/js/dag/grid/ToggleGroups.tsx
@@ -28,7 +28,7 @@ const getGroupIds = (groups: Task[]) => {
const checkTasks = (tasks: Task[]) =>
tasks.forEach((task) => {
if (task.children) {
- groupIds.push(task.label!);
+ groupIds.push(task.id!);
checkTasks(task.children);
}
});
diff --git a/airflow/www/static/js/dag/grid/index.test.tsx
b/airflow/www/static/js/dag/grid/index.test.tsx
index 66e1cafaeb..5cc6e71788 100644
--- a/airflow/www/static/js/dag/grid/index.test.tsx
+++ b/airflow/www/static/js/dag/grid/index.test.tsx
@@ -86,6 +86,55 @@ const mockGridData = {
},
],
},
+ {
+ id: "group_2",
+ label: "group_2",
+ instances: [
+ {
+ endDate: "2021-10-26T15:42:03.391939+00:00",
+ runId: "run1",
+ startDate: "2021-10-26T15:42:03.391917+00:00",
+ state: "success",
+ taskId: "group_1",
+ tryNumber: 1,
+ },
+ ],
+ children: [
+ {
+ id: "group_2.task_1",
+ operator: "DummyOperator",
+ label: "task_1",
+ instances: [
+ {
+ endDate: "2021-10-26T15:42:03.391939+00:00",
+ runId: "run1",
+ startDate: "2021-10-26T15:42:03.391917+00:00",
+ state: "success",
+ taskId: "group_1.task_1",
+ tryNumber: 1,
+ },
+ ],
+ children: [
+ {
+ id: "group_2.task_1.sub_task_1",
+ label: "sub_task_1",
+ extraLinks: [],
+ operator: "DummyOperator",
+ instances: [
+ {
+ endDate: "2021-10-26T15:42:03.391939+00:00",
+ runId: "run1",
+ startDate: "2021-10-26T15:42:03.391917+00:00",
+ state: "success",
+ taskId: "group_2.task_1.sub_task_1",
+ tryNumber: 1,
+ },
+ ],
+ },
+ ],
+ },
+ ],
+ },
],
instances: [],
},
@@ -144,16 +193,18 @@ describe("Test ToggleGroups", () => {
});
test("Group defaults to closed", () => {
- const { getByTestId, getByText, getAllByTestId } = render(
+ const { queryAllByTestId, getByText, getAllByTestId } = render(
<Grid openGroupIds={[]} onToggleGroups={() => {}} />,
{ wrapper: Wrapper }
);
- const groupName = getByText("group_1");
+ const groupName1 = getByText("group_1");
+ const groupName2 = getByText("group_2");
- expect(getAllByTestId("task-instance")).toHaveLength(1);
- expect(groupName).toBeInTheDocument();
- expect(getByTestId("open-group")).toBeInTheDocument();
+ expect(getAllByTestId("task-instance")).toHaveLength(2);
+ expect(groupName1).toBeInTheDocument();
+ expect(groupName2).toBeInTheDocument();
+ expect(queryAllByTestId("open-group")).toHaveLength(2);
});
test("Buttons are disabled if all groups are expanded or collapsed", () => {
@@ -168,12 +219,12 @@ describe("Test ToggleGroups", () => {
expect(collapseButton).toBeDisabled();
const taskElements = queryAllByTestId("task-instance");
- expect(taskElements).toHaveLength(1);
+ expect(taskElements).toHaveLength(2);
fireEvent.click(expandButton);
const newTaskElements = queryAllByTestId("task-instance");
- expect(newTaskElements).toHaveLength(3);
+ expect(newTaskElements).toHaveLength(6);
expect(collapseButton).toBeEnabled();
expect(expandButton).toBeDisabled();
@@ -188,41 +239,71 @@ describe("Test ToggleGroups", () => {
const expandButton = getByTitle(EXPAND);
const collapseButton = getByTitle(COLLAPSE);
- const groupName = getByText("group_1");
+ const groupName1 = getByText("group_1");
+ const groupName2 = getByText("group_2");
- expect(queryAllByTestId("task-instance")).toHaveLength(3);
- expect(groupName).toBeInTheDocument();
+ expect(queryAllByTestId("task-instance")).toHaveLength(6);
+ expect(groupName1).toBeInTheDocument();
+ expect(groupName2).toBeInTheDocument();
- expect(queryAllByTestId("close-group")).toHaveLength(2);
+ expect(queryAllByTestId("close-group")).toHaveLength(4);
expect(queryAllByTestId("open-group")).toHaveLength(0);
fireEvent.click(collapseButton);
await waitFor(() =>
- expect(queryAllByTestId("task-instance")).toHaveLength(1)
+ expect(queryAllByTestId("task-instance")).toHaveLength(2)
);
expect(queryAllByTestId("close-group")).toHaveLength(0);
- // Since the groups are nested, only the parent row is rendered
- expect(queryAllByTestId("open-group")).toHaveLength(1);
+ // Since the groups are nested, only the parent rows is rendered
+ expect(queryAllByTestId("open-group")).toHaveLength(2);
+
+ fireEvent.click(expandButton);
+
+ await waitFor(() =>
+ expect(queryAllByTestId("task-instance")).toHaveLength(6)
+ );
+ expect(queryAllByTestId("close-group")).toHaveLength(4);
+ expect(queryAllByTestId("open-group")).toHaveLength(0);
+ });
+
+ test("Toggling a group does not affect groups with the same label", async ()
=> {
+ const { getByTitle, getByTestId, queryAllByTestId } = render(
+ <GridWithGroups />,
+ { wrapper: Wrapper }
+ );
+
+ const expandButton = getByTitle(EXPAND);
fireEvent.click(expandButton);
await waitFor(() =>
- expect(queryAllByTestId("task-instance")).toHaveLength(3)
+ expect(queryAllByTestId("task-instance")).toHaveLength(6)
);
- expect(queryAllByTestId("close-group")).toHaveLength(2);
+ expect(queryAllByTestId("close-group")).toHaveLength(4);
expect(queryAllByTestId("open-group")).toHaveLength(0);
+
+ const group2Task1 = getByTestId("group_2.task_1");
+
+ fireEvent.click(group2Task1);
+
+ // We only expect group_2.task_1 to be affected, not group_1.task_1
+ await waitFor(() =>
+ expect(queryAllByTestId("task-instance")).toHaveLength(5)
+ );
+ expect(queryAllByTestId("close-group")).toHaveLength(3);
+ expect(queryAllByTestId("open-group")).toHaveLength(1);
});
test("Hovered effect on task state", async () => {
- const openGroupIds = ["group_1", "task_1"];
+ const openGroupIds = ["group_1", "group_1.task_1"];
const { rerender, queryAllByTestId } = render(
<Grid openGroupIds={openGroupIds} onToggleGroups={() => {}} />,
{ wrapper: Wrapper }
);
const taskElements = queryAllByTestId("task-instance");
- expect(taskElements).toHaveLength(3);
+ expect(taskElements).toHaveLength(4);
taskElements.forEach((taskElement) => {
expect(taskElement).toHaveStyle("opacity: 1");
diff --git a/airflow/www/static/js/dag/grid/renderTaskRows.tsx
b/airflow/www/static/js/dag/grid/renderTaskRows.tsx
index c661eba677..181186b172 100644
--- a/airflow/www/static/js/dag/grid/renderTaskRows.tsx
+++ b/airflow/www/static/js/dag/grid/renderTaskRows.tsx
@@ -122,20 +122,20 @@ const Row = (props: RowProps) => {
const isGroup = !!task.children;
const isSelected = selected.taskId === task.id;
- const isOpen = openGroupIds.some((g) => g === task.label);
+ const isOpen = openGroupIds.some((g) => g === task.id);
// assure the function is the same across renders
const memoizedToggle = useCallback(() => {
- if (isGroup && task.label) {
+ if (isGroup && task.id) {
let newGroupIds = [];
if (!isOpen) {
- newGroupIds = [...openGroupIds, task.label];
+ newGroupIds = [...openGroupIds, task.id];
} else {
- newGroupIds = openGroupIds.filter((g) => g !== task.label);
+ newGroupIds = openGroupIds.filter((g) => g !== task.id);
}
onToggleGroups(newGroupIds);
}
- }, [isGroup, isOpen, task.label, openGroupIds, onToggleGroups]);
+ }, [isGroup, isOpen, task.id, openGroupIds, onToggleGroups]);
// check if the group's parents are all open, if not, return null
if (level !== openParentCount) return null;
@@ -168,6 +168,7 @@ const Row = (props: RowProps) => {
isGroup={isGroup}
isMapped={task.isMapped && !isParentMapped}
label={task.label || task.id || ""}
+ id={task.id || ""}
isOpen={isOpen}
level={level}
/>
diff --git a/airflow/www/static/js/utils/graph.ts
b/airflow/www/static/js/utils/graph.ts
index 08d5415833..1f77990930 100644
--- a/airflow/www/static/js/utils/graph.ts
+++ b/airflow/www/static/js/utils/graph.ts
@@ -117,7 +117,7 @@ const generateGraph = ({
height?: number;
} => {
const { id, value, children } = node;
- const isOpen = openGroupIds?.includes(value.label);
+ const isOpen = openGroupIds?.includes(id);
const childCount =
children?.filter((c: DepNode) => !c.id.includes("join_id")).length || 0;
const childIds = children?.length ? getNestedChildIds(children) : [];
@@ -170,7 +170,7 @@ const generateGraph = ({
sourceId: childIds.indexOf(e.sourceId) > -1 ? node.id : e.sourceId,
targetId: childIds.indexOf(e.targetId) > -1 ? node.id : e.targetId,
}));
- closedGroupIds.push(value.label);
+ closedGroupIds.push(id);
}
return {
id,
