Make Dags sidebar button a link and move Dag Runs and Task Instances to Browse

airflow-core/src/airflow/ui/src/layouts/Nav/BrowseButton.test.tsx

@@ -0,0 +1,83 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import type { PropsWithChildren } from "react";
+
+import "@testing-library/jest-dom/vitest";
+import { fireEvent, render, screen, waitFor } from "@testing-library/react";
+import { MemoryRouter } from "react-router-dom";
+import { describe, expect, it } from "vitest";
+
+import { BaseWrapper, Wrapper } from "src/utils/Wrapper";
+
+import { BrowseButton } from "./BrowseButton";
+
+const getTrigger = () => screen.getByRole("button", { name: "nav.browse" });
+
+const openMenu = async () => {
+  fireEvent.click(getTrigger());
+  await waitFor(() => expect(screen.getAllByRole("menuitem").length).toBeGreaterThan(0));
+};
+
+const hrefs = () => screen.getAllByRole("menuitem").map((item) => item.getAttribute("href"));
+
+const wrapperAt = (path: string) => {
+  const wrapper = ({ children }: PropsWithChildren) => (
+    <BaseWrapper>
+      <MemoryRouter initialEntries={[path]}>{children}</MemoryRouter>
+    </BaseWrapper>
+  );
+
+  return wrapper;
+};
+
+describe("BrowseButton", () => {
+  it("lists Dag Runs and Task Instances for users who can see Dags", async () => {
+    render(<BrowseButton authorizedMenuItems={["Dags"]} externalViews={[]} />, { wrapper: Wrapper });
+
+    await openMenu();
+
+    expect(hrefs()).toStrictEqual(["/dag_runs", "/task_instances"]);
+  });
+
+  it("hides Dag Runs and Task Instances without access to Dags", async () => {
+    render(<BrowseButton authorizedMenuItems={["Jobs"]} externalViews={[]} />, { wrapper: Wrapper });
+
+    await openMenu();
+
+    expect(hrefs()).toStrictEqual(["/jobs"]);
+  });
+
+  it("keeps the entries in alphabetical order", async () => {
+    render(<BrowseButton authorizedMenuItems={["Dags", "Jobs", "XComs"]} externalViews={[]} />, {
+      wrapper: Wrapper,
+    });
+
+    await openMenu();
+
+    expect(hrefs()).toStrictEqual(["/dag_runs", "/jobs", "/task_instances", "/xcoms"]);
+  });
+
+  it.each(["/dag_runs", "/task_instances"])("is highlighted on %s", (path) => {
+    render(<BrowseButton authorizedMenuItems={["Dags"]} externalViews={[]} />, {
+      wrapper: wrapperAt(path),
+    });
+
+    expect(getTrigger()).toHaveAttribute("aria-current", "page");
+  });
+});

airflow-core/src/airflow/ui/src/layouts/Nav/BrowseButton.tsx

@@ -42,6 +42,14 @@ const links = [
     // Must stay byte-identical to MenuItem.DAG_BUNDLES: the filter below matches on this string.
     title: "Dag Bundles",
   },
+  {
+    href: "/dag_runs",
+    key: "dagRuns",
+    // Reuses the label the Dags pages already show on their tabs.
+    labelKey: "dagRun_other",
+    // There is no menu item of its own for Dag runs: they follow the Dags permission.
+    title: "Dags",
+  },
   {
     href: "/deadlines",
     key: "deadlines",
@@ -52,6 +60,12 @@ const links = [
     key: "jobs",
     title: "Jobs",
   },
+  {
+    href: "/task_instances",
+    key: "taskInstances",
+    labelKey: "taskInstance_other",
+    title: "Dags",
+  },
   {
     href: "/xcoms",
     key: "xcoms",
@@ -73,13 +87,17 @@ export const BrowseButton = ({
 }) => {
   const { t: translate } = useTranslation();
   const authorizedLinks = links.filter(({ title }) => authorizedMenuItems.includes(title as MenuItem));
-  const menuItems = authorizedLinks.map((link) => (
-    <Menu.Item asChild key={link.key} value={translate(`browse.${link.key}`)}>
-      <RouterLink aria-label={translate(`browse.${link.key}`)} to={link.href}>
-        {translate(`browse.${link.key}`)}
-      </RouterLink>
-    </Menu.Item>
-  ));
+  const menuItems = authorizedLinks.map((link) => {
+    const label = translate(link.labelKey ?? `browse.${link.key}`);
+
+    return (
+      <Menu.Item asChild key={link.key} value={label}>
+        <RouterLink aria-label={label} to={link.href}>
+          {label}
+        </RouterLink>
+      </Menu.Item>
+    );
+  });
 
   if (!menuItems.length && !externalViews.length) {
     return undefined;

airflow-core/src/airflow/ui/src/layouts/Nav/DagsButton.test.tsx

@@ -19,15 +19,15 @@
 import type { PropsWithChildren } from "react";
 
 import "@testing-library/jest-dom/vitest";
-import { fireEvent, render, screen, waitFor } from "@testing-library/react";
+import { fireEvent, render, screen } from "@testing-library/react";
 import { MemoryRouter } from "react-router-dom";
 import { describe, expect, it } from "vitest";
 
 import { BaseWrapper, Wrapper } from "src/utils/Wrapper";
 
 import { DagsButton } from "./DagsButton";
 
-const getTrigger = () => screen.getByRole("button", { name: "nav.dags" });
+const getLink = () => screen.getByRole("link", { name: "nav.dags" });
 
 const wrapperAt = (path: string) => {
   const wrapper = ({ children }: PropsWithChildren) => (
@@ -40,56 +40,36 @@ const wrapperAt = (path: string) => {
 };
 
 describe("DagsButton", () => {
-  it("opens the menu on click instead of navigating", async () => {
+  it("is a link to the Dags page", () => {
     render(<DagsButton authorizedMenuItems={["Dags"]} />, { wrapper: Wrapper });
 
-    expect(getTrigger()).not.toHaveAttribute("href");
-
-    fireEvent.click(getTrigger());
-
-    await waitFor(() => {
-      expect(screen.getAllByRole("menuitem")).toHaveLength(3);
-    });
+    expect(getLink()).toHaveAttribute("href", "/dags");
   });
 
-  it.each(["/dags", "/dags/example_dag/runs", "/dag_runs", "/task_instances"])(
-    "is highlighted on %s",
-    (path) => {
-      render(<DagsButton authorizedMenuItems={["Dags"]} />, { wrapper: wrapperAt(path) });
-
-      expect(getTrigger()).toHaveAttribute("aria-current", "page");
-    },
-  );
-
-  it("reveals the dags, dag runs and task instances links on hover", async () => {
-    render(<DagsButton authorizedMenuItems={["Dags"]} />, { wrapper: Wrapper });
-
-    expect(screen.queryByRole("menuitem")).toBeNull();
+  it.each(["/dags", "/dags/example_dag/runs"])("is highlighted on %s", (path) => {
+    render(<DagsButton authorizedMenuItems={["Dags"]} />, { wrapper: wrapperAt(path) });
 
-    fireEvent.pointerEnter(getTrigger(), { pointerType: "mouse" });
+    expect(getLink()).toHaveAttribute("aria-current", "page");
+  });
 
-    await waitFor(() => {
-      expect(screen.getAllByRole("menuitem")).toHaveLength(3);
-    });
+  it.each(["/dag_runs", "/task_instances"])("is not highlighted on %s", (path) => {
+    render(<DagsButton authorizedMenuItems={["Dags"]} />, { wrapper: wrapperAt(path) });
 
-    expect(screen.getAllByRole("menuitem").map((item) => item.getAttribute("href"))).toStrictEqual([
-      "/dags",
-      "/dag_runs",
-      "/task_instances",
-    ]);
+    expect(getLink()).not.toHaveAttribute("aria-current");
   });
 
-  it("ignores hover from a touch pointer", async () => {
+  it("does not open a menu on hover", () => {
     render(<DagsButton authorizedMenuItems={["Dags"]} />, { wrapper: Wrapper });
 
-    fireEvent.pointerEnter(getTrigger(), { pointerType: "touch" });
+    fireEvent.pointerEnter(getLink(), { pointerType: "mouse" });
 
-    await expect(waitFor(() => expect(screen.getAllByRole("menuitem")).toHaveLength(3))).rejects.toThrow();
+    expect(screen.queryByRole("menuitem")).toBeNull();
   });
 
-  it("renders a plain button with no menu when Dags is not authorized", () => {
+  it("is not a link when Dags is not authorized", () => {
     render(<DagsButton authorizedMenuItems={[]} />, { wrapper: Wrapper });
 
-    expect(getTrigger()).not.toHaveAttribute("aria-haspopup");
+    expect(screen.queryByRole("link", { name: "nav.dags" })).toBeNull();
+    expect(screen.getByRole("button", { name: "nav.dags" })).toBeDisabled();
   });
 });

airflow-core/src/airflow/ui/src/layouts/Nav/DagsButton.tsx

@@ -17,59 +17,26 @@
  * under the License.
  */
 import { useTranslation } from "react-i18next";
-import { Link as RouterLink } from "react-router-dom";
 
 import type { MenuItem } from "openapi/requests/types.gen";
 
-import { Menu } from "src/system-components";
-
 import { DagIcon } from "src/assets/DagIcon";
 
 import { NavButton } from "./NavButton";
-import { NavMenu } from "./NavMenu";
-
-const links = [
-  {
-    href: "/dags",
-    key: "dag_other",
-  },
-  {
-    href: "/dag_runs",
-    key: "dagRun_other",
-  },
-  {
-    href: "/task_instances",
-    key: "taskInstance_other",
-  },
-];
 
 export const DagsButton = ({ authorizedMenuItems }: { readonly authorizedMenuItems: Array<MenuItem> }) => {
   const { t: translate } = useTranslation();
   const isAuthorized = authorizedMenuItems.includes("Dags");
 
-  const trigger = (
+  // Without a destination NavButton renders a real <button>, which can be disabled. A disabled
+  // anchor would stay clickable.
+  return (
     <NavButton
       data-testid="nav-dags-link"
       disabled={!isAuthorized}
       icon={DagIcon}
       title={translate("nav.dags")}
-      to={links.map(({ href }) => href)}
+      to={isAuthorized ? "/dags" : undefined}
     />
   );
-
-  if (!isAuthorized) {
-    return trigger;
-  }
-
-  return (
-    <NavMenu trigger={trigger}>
-      {links.map(({ href, key }) => (
-        <Menu.Item asChild key={key} value={key}>
-          <RouterLink aria-label={translate(key)} to={href}>
-            {translate(key)}
-          </RouterLink>
-        </Menu.Item>
-      ))}
-    </NavMenu>
-  );
 };