Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for TaskGroup toggles for duplicated labels #34072

Merged
merged 2 commits into from
Sep 6, 2023
Merged

Fix for TaskGroup toggles for duplicated labels #34072

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5557173
Fix for TaskGroup toggles for duplicated labels
Usiel Sep 4, 2023
61bcecd
Fix toggle for Gantt as well
Usiel Sep 4, 2023
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
2 changes: 1 addition & 1 deletion airflow/www/static/js/dag/details/gantt/Row.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions airflow/www/static/js/dag/details/graph/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
},
Expand Down
12 changes: 9 additions & 3 deletions airflow/www/static/js/dag/grid/TaskName.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
);

Expand All @@ -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 }
);

Expand All @@ -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 }
);

Expand Down
3 changes: 3 additions & 0 deletions airflow/www/static/js/dag/grid/TaskName.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ interface Props {
isOpen?: boolean;
level?: number;
label: string;
id: string;
}

const TaskName = ({
Expand All @@ -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%"
Expand Down
2 changes: 1 addition & 1 deletion airflow/www/static/js/dag/grid/ToggleGroups.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
});
Expand Down
117 changes: 99 additions & 18 deletions airflow/www/static/js/dag/grid/index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: [],
},
Expand Down Expand Up @@ -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", () => {
Expand All @@ -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();
Expand All @@ -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");
Expand Down
11 changes: 6 additions & 5 deletions airflow/www/static/js/dag/grid/renderTaskRows.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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}
/>
Expand Down
4 changes: 2 additions & 2 deletions airflow/www/static/js/utils/graph.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) : [];
Expand Down Expand Up @@ -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,
Expand Down