Highlight Trash collection in sidebar when viewing root trash collection or a trashed item (#44258)

* highlights the trash link in the sidebar when viewing root trash collection or a trashed item

* impls pr feedback

* impls more pr feedback

* fixes unit tests and logical mistake that allowed urls with invalid collection ids to match on undefined trash id due to it not being loaded yet

e2e/test/scenarios/collections/trash.cy.spec.js

@@ -12,9 +12,13 @@ import {
   sidebar,
   entityPickerModal,
   modal,
+  openNavigationSidebar,
   navigationSidebar,
   restore,
   entityPickerModalTab,
+  visitCollection,
+  visitDashboard,
+  visitQuestion,
 } from "e2e/support/helpers";
 
 describe("scenarios > collections > trash", () => {
@@ -514,7 +518,7 @@ describe("scenarios > collections > trash", () => {
     ).as("question");
 
     cy.get("@question").then(question => {
-      cy.visit(`/question/${question.id}-question-a`);
+      visitQuestion(question.id);
       // should not have disabled actions in top navbar
       cy.findAllByTestId("qb-header-action-panel").within(() => {
         cy.findByText("Filter").should("not.exist");
@@ -533,7 +537,7 @@ describe("scenarios > collections > trash", () => {
     });
 
     cy.get("@dashboard").then(dashboard => {
-      cy.visit(`/dashboard/${dashboard.id}-dashboard-a`);
+      visitDashboard(dashboard.id);
 
       cy.findAllByTestId("dashboard-header").within(() => {
         cy.icon("pencil").should("not.exist");
@@ -589,7 +593,7 @@ describe("scenarios > collections > trash", () => {
     cy.signInAsNormalUser();
 
     cy.get("@collection").then(collection => {
-      cy.visit(`/collection/${collection.id}-collection-a`);
+      visitCollection(collection.id);
       archiveBanner().findByText("Restore").should("not.exist");
       archiveBanner().findByText("Move").should("not.exist");
       archiveBanner().findByText("Delete permanently").should("not.exist");
@@ -637,9 +641,54 @@ describe("scenarios > collections > trash", () => {
       cy.findByText(CURATEABLE_NAME).should("be.visible");
     });
   });
-});
 
-describe("Restoring items", () => {});
+  it("should highlight the trash in the navbar when viewing root trash collection or an entity in the trash", () => {
+    createCollection({ name: "Collection A" }, true).as("collection");
+    createDashboard({ name: "Dashboard A" }, true).as("dashboard");
+    createNativeQuestion(
+      {
+        name: "Question A",
+        native: { query: "select 1;" },
+      },
+      true,
+    ).as("question");
+
+    cy.log("Make sure trash is selected for root trash collection");
+    cy.visit("/trash");
+    assertTrashSelectinInNavigationSidebar();
+
+    cy.log("Make sure trash is selected for a trashed collection");
+    cy.get("@collection").then(collection => {
+      cy.intercept("GET", `/api/collection/${collection.id}`).as(
+        "getCollection",
+      );
+      visitCollection(collection.id);
+      cy.wait("@getCollection");
+      assertTrashSelectinInNavigationSidebar();
+    });
+
+    cy.log("Make sure trash is selected for a trashed dashboard");
+    cy.get("@dashboard").then(dashboard => {
+      cy.intercept("GET", `/api/dashboard/${dashboard.id}`).as("getDashboard");
+      visitDashboard(dashboard.id);
+      cy.wait("@getDashboard");
+      openNavigationSidebar();
+      assertTrashSelectinInNavigationSidebar();
+    });
+
+    cy.log("Make sure trash is selected for a trashed question");
+    cy.get("@question").then(question => {
+      cy.log(question.id);
+      cy.intercept("POST", `/api/card/${question.id}/query`).as(
+        "getQuestionResult",
+      );
+      visitQuestion(question.id);
+      cy.wait("@getQuestionResult");
+      openNavigationSidebar();
+      assertTrashSelectinInNavigationSidebar();
+    });
+  });
+});
 
 function toggleEllipsisMenuFor(item) {
   collectionTable().within(() => {
@@ -720,6 +769,14 @@ function selectItem(name) {
     .within(() => cy.findByRole("checkbox").click());
 }
 
+function assertTrashSelectinInNavigationSidebar() {
+  navigationSidebar().within(() => {
+    cy.findByText("Trash")
+      .parents("li")
+      .should("have.attr", "aria-selected", "true");
+  });
+}
+
 function ensureBookmarkVisible(bookmark) {
   cy.findByRole("tab", { name: /bookmarks/i })
     .findByText(bookmark)

frontend/src/metabase/collections/components/CollectionLanding/CollectionLanding.tsx

@@ -29,11 +29,18 @@ const CollectionLanding = ({
 
   useEffect(
     function redirectIfTrashCollection() {
-      if (trashCollection?.id === collectionId) {
+      // redirect /collection/trash and /collection/<trash-collection-id> to /trash
+      const isTrashSlug = slug === "trash";
+      const isTrashCollectionId =
+        collectionId &&
+        trashCollection?.id &&
+        trashCollection.id === collectionId;
+
+      if (isTrashSlug || isTrashCollectionId) {
         dispatch(replace("/trash"));
       }
     },
-    [dispatch, trashCollection?.id, collectionId],
+    [dispatch, slug, trashCollection?.id, collectionId],
   );
 
   if (!isNotNull(collectionId)) {

frontend/src/metabase/nav/containers/MainNavbar/MainNavbar.tsx

@@ -4,17 +4,19 @@ import { connect } from "react-redux";
 import { push } from "react-router-redux";
 import _ from "underscore";
 
+import { skipToken, useGetCollectionQuery } from "metabase/api";
 import { useQuestionQuery } from "metabase/common/hooks";
 import { getDashboard } from "metabase/dashboard/selectors";
 import * as Urls from "metabase/lib/urls";
 import { closeNavbar, openNavbar } from "metabase/redux/app";
 import type Question from "metabase-lib/v1/Question";
-import type { Dashboard } from "metabase-types/api";
+import type { CollectionId, Dashboard } from "metabase-types/api";
 import type { State } from "metabase-types/store";
 
 import { NavRoot, Sidebar } from "./MainNavbar.styled";
 import MainNavbarContainer from "./MainNavbarContainer";
 import getSelectedItems, {
+  isCollectionPath,
   isModelPath,
   isQuestionPath,
 } from "./getSelectedItems";
@@ -31,6 +33,7 @@ interface EntityLoaderProps {
 interface StateProps {
   dashboard?: Dashboard;
   questionId?: number;
+  collectionId?: CollectionId;
 }
 
 interface DispatchProps extends MainNavbarDispatchProps {
@@ -50,6 +53,7 @@ function mapStateToProps(state: State, props: MainNavbarOwnProps) {
     dashboard: getDashboard(state),
 
     questionId: maybeGetQuestionId(state, props),
+    collectionId: maybeGetCollectionId(state, props),
   };
 }
 
@@ -64,6 +68,7 @@ function MainNavbar({
   location,
   params,
   questionId,
+  collectionId,
   dashboard,
   openNavbar,
   closeNavbar,
@@ -74,6 +79,10 @@ function MainNavbar({
     id: questionId,
   });
 
+  const { data: collection } = useGetCollectionQuery(
+    collectionId ? { id: collectionId } : skipToken,
+  );
+
   useEffect(() => {
     function handleSidebarKeyboardShortcut(e: KeyboardEvent) {
       if (e.key === "." && (e.ctrlKey || e.metaKey)) {
@@ -97,9 +106,10 @@ function MainNavbar({
         pathname: location.pathname,
         params,
         question,
+        collection,
         dashboard,
       }),
-    [location, params, question, dashboard],
+    [location, params, question, dashboard, collection],
   );
 
   return (
@@ -134,6 +144,15 @@ function maybeGetQuestionId(
   return canFetchQuestion ? Urls.extractEntityId(params.slug) : null;
 }
 
+function maybeGetCollectionId(
+  state: State,
+  { location, params }: MainNavbarOwnProps,
+) {
+  const { pathname } = location;
+  const canFetchQuestion = isCollectionPath(pathname);
+  return canFetchQuestion ? Urls.extractEntityId(params.slug) : null;
+}
+
 // eslint-disable-next-line import/no-default-export -- deprecated usage
 export default _.compose(connect(mapStateToProps, mapDispatchToProps))(
   MainNavbar,

frontend/src/metabase/nav/containers/MainNavbar/MainNavbar.unit.spec.tsx

@@ -4,6 +4,7 @@ import { Route } from "react-router";
 import {
   setupCardsEndpoints,
   setupCollectionsEndpoints,
+  setupCollectionByIdEndpoint,
   setupDatabasesEndpoints,
   setupSearchEndpoints,
 } from "__support__/server-mocks";
@@ -103,6 +104,9 @@ async function setup({
   }
 
   setupCollectionsEndpoints({ collections });
+  setupCollectionByIdEndpoint({
+    collections: [PERSONAL_COLLECTION_BASE, TEST_COLLECTION],
+  });
   setupDatabasesEndpoints(databases);
   setupSearchEndpoints(models);
   fetchMock.get("path:/api/bookmark", []);
@@ -234,10 +238,10 @@ describe("nav > containers > MainNavbar", () => {
   describe("browse models link", () => {
     it("should render when there are models", async () => {
       await setup({ models: [createMockModelResult()] });
-      const listItem = screen.getByRole("listitem", {
+      const listItem = await screen.findByRole("listitem", {
         name: /Browse models/i,
       });
-      const link = within(listItem).getByRole("link");
+      const link = await within(listItem).findByRole("link");
       expect(link).toBeInTheDocument();
       expect(link).toHaveAttribute("href", "/browse/models");
     });
@@ -254,7 +258,7 @@ describe("nav > containers > MainNavbar", () => {
         models: [createMockModelResult()],
         pathname: "/browse/models",
       });
-      const listItem = screen.getByRole("listitem", {
+      const listItem = await screen.findByRole("listitem", {
         name: /Browse models/i,
       });
       expect(listItem).toHaveAttribute("aria-selected", "true");

frontend/src/metabase/nav/containers/MainNavbar/MainNavbarContainer/MainNavbarContainer.tsx

@@ -113,6 +113,7 @@ function MainNavbarContainer({
     if (trashCollection) {
       const trash: CollectionTreeItem = {
         ...trashCollection,
+        id: "trash",
         icon: getCollectionIcon(trashCollection),
         children: [],
       };

frontend/src/metabase/nav/containers/MainNavbar/getSelectedItems.ts

 import { coerceCollectionId } from "metabase/collections/utils";
 import * as Urls from "metabase/lib/urls";
 import type Question from "metabase-lib/v1/Question";
-import type { Dashboard } from "metabase-types/api";
+import type { Collection, Dashboard } from "metabase-types/api";
 
 import type { SelectedItem } from "./types";
 
@@ -13,12 +13,32 @@ type Opts = {
   };
   question?: Question;
   dashboard?: Dashboard;
+  collection?: Collection;
 };
 
-function isCollectionPath(pathname: string): boolean {
+export function isCollectionPath(pathname: string): boolean {
   return pathname.startsWith("/collection");
 }
 
+function isTrashPath(pathname: string): boolean {
+  return pathname.startsWith("/trash");
+}
+
+function isInTrash({
+  pathname,
+  collection,
+  question,
+  dashboard,
+}: Pick<Opts, "pathname" | "collection" | "question" | "dashboard">): boolean {
+  return (
+    isTrashPath(pathname) ||
+    collection?.archived ||
+    question?.isArchived() ||
+    dashboard?.archived ||
+    false
+  );
+}
+
 function isUsersCollectionPath(pathname: string): boolean {
   return pathname.startsWith("/collection/users");
 }
@@ -40,9 +60,18 @@ function getSelectedItems({
   params,
   question,
   dashboard,
+  collection,
 }: Opts): SelectedItem[] {
   const { slug } = params;
 
+  if (isInTrash({ pathname, collection, question, dashboard })) {
+    return [
+      {
+        id: "trash",
+        type: "collection",
+      },
+    ];
+  }
   if (isCollectionPath(pathname)) {
     return [
       {

frontend/test/__support__/server-mocks/collection.ts

@@ -35,6 +35,7 @@ export function setupCollectionsEndpoints({
 }: CollectionEndpoints) {
   fetchMock.get("path:/api/collection/root", rootCollection);
   fetchMock.get(`path:/api/collection/trash`, trashCollection);
+  fetchMock.get(`path:/api/collection/${trashCollection.id}`, trashCollection);
   fetchMock.get(
     {
       url: "path:/api/collection/tree",