fix(webapp/browse): Fix bug where verified filter is applied to recents even...

fix(webapp/browse): Fix bug where verified filter is applied to recents even when no verified models exist (#46462)

e2e/test/scenarios/onboarding/home/browse.cy.spec.js → e2e/test/scenarios/onboarding/home/browse.cy.spec.ts

@@ -74,11 +74,7 @@ describeWithSnowplow("scenarios > browse", () => {
     cy.go("back");
     cy.findByRole("heading", { name: "Sample Database" }).click();
     cy.findByRole("heading", { name: "Products" }).click();
-    cy.findByRole(
-      "gridcell",
-      { name: "Rustic Paper Wallet" },
-      { timeout: 10000 },
-    );
+    cy.findByRole("gridcell", { name: "Rustic Paper Wallet" });
   });
 
   it("on an open-source instance, the Browse models page has no controls for setting filters", () => {
@@ -97,15 +93,66 @@ describeWithSnowplowEE("scenarios > browse (EE)", () => {
     restore();
     cy.signInAsAdmin();
     enableTracking();
+    setTokenFeatures("all");
+    cy.intercept("PUT", "/api/setting/browse-filter-only-verified-models").as(
+      "updateFilter",
+    );
+    cy.intercept("POST", "/api/moderation-review").as("updateVerification");
   });
+  const openFilterPopover = () =>
+    cy.findByRole("button", { name: /filter icon/i }).click();
+  const toggle = () =>
+    cy.findByRole("switch", { name: /Show verified models only/ });
 
-  it("/browse/models allows models to be filtered, on an enterprise instance", () => {
-    const openFilterPopover = () =>
-      cy.findByRole("button", { name: /filter icon/i }).click();
-    const toggle = () =>
-      cy.findByRole("switch", { name: /Show verified models only/ });
-    setTokenFeatures("all");
+  const recentsGrid = () => cy.findByRole("grid", { name: "Recents" });
+  const modelsTable = () => cy.findByRole("table", { name: "Table of models" });
+  const productsModel = () =>
+    modelsTable().findByRole("heading", { name: "Products Model" });
+  const recentProductsModel = () => recentsGrid().findByText("Products Model");
+  const ordersModel = () =>
+    modelsTable().findByRole("heading", { name: "Orders Model" });
+  const recentOrdersModel = () => recentsGrid().findByText("Orders Model");
+  const productsModelRow = () =>
+    modelsTable().findByRole("row", { name: /Products Model/i });
+  const ordersModelRow = () =>
+    modelsTable().findByRole("row", { name: /Orders Model/i });
+  const filterButton = () =>
+    cy
+      .findByTestId("browse-models-header")
+      .findByRole("button", { name: /filter icon/i });
+
+  const setVerification = (linkSelector: RegExp | string) => {
+    cy.findByLabelText("Move, trash, and more...").click();
+    cy.findByRole("dialog", {
+      name: /ellipsis icon/i,
+    })
+      .findByText(linkSelector)
+      .click();
+  };
+  const verifyModel = () => {
+    setVerification(/Verify this model/);
+    cy.wait("@updateVerification");
+  };
+  const unverifyModel = () => {
+    setVerification(/Remove verification/);
+    cy.wait("@updateVerification");
+  };
 
+  const toggleVerificationFilter = () => {
+    openFilterPopover();
+    toggle().next("label").click();
+    cy.wait("@updateFilter");
+  };
+
+  const browseModels = () => {
+    cy.visit("/");
+
+    navigationSidebar()
+      .findByRole("listitem", { name: "Browse models" })
+      .click();
+  };
+
+  it("/browse/models allows models to be filtered, on an enterprise instance", () => {
     cy.log('Create a "Products" model');
     cy.createQuestion({
       name: "Products Model",
@@ -116,19 +163,9 @@ describeWithSnowplowEE("scenarios > browse (EE)", () => {
       type: "model",
     });
 
-    cy.visit("/");
-    cy.findByRole("listitem", { name: "Browse models" }).click();
-
-    const productsModel = () =>
-      cy.findByRole("heading", { name: "Products Model" });
-    const ordersModel = () =>
-      cy.findByRole("heading", { name: "Orders Model" });
-    const productsModelRow = () =>
-      cy.findByRole("row", { name: /Products Model/i });
-    const ordersModelRow = () =>
-      cy.findByRole("row", { name: /Orders Model/i });
-
-    cy.log("Cells for the Products and Orders models exist");
+    browseModels();
+
+    cy.log("Cells for the Products and Orders models exist in the table");
     productsModel().should("exist");
     ordersModel().should("exist");
 
@@ -148,19 +185,18 @@ describeWithSnowplowEE("scenarios > browse (EE)", () => {
     });
 
     cy.log("There are no verified models, so the filter toggle is not visible");
-    cy.findByRole("button", { name: /filter icon/i }).should("not.exist");
+    filterButton().should("not.exist");
 
     cy.log("Verify the Orders Model");
     cy.findByRole("heading", { name: "Orders Model" }).click();
-    cy.findByLabelText("Move, trash, and more...").click();
-    cy.findByRole("dialog", {
-      name: /ellipsis icon/i,
-    })
-      .findByText(/Verify this model/)
-      .click();
+    verifyModel();
 
-    cy.visit("/");
-    cy.findByRole("listitem", { name: "Browse models" }).click();
+    browseModels();
+
+    cy.log("Filter on verified models is enabled by default");
+    cy.findByTestId("browse-models-header")
+      .findByTestId("filter-dot")
+      .should("be.visible");
 
     cy.log(
       "The Products Model does not appear in the table, since it's not verified",
@@ -177,14 +213,43 @@ describeWithSnowplowEE("scenarios > browse (EE)", () => {
     });
 
     cy.log("The filter toggle is now visible");
-    cy.findByRole("button", { name: /filter icon/i }).should("be.visible");
+    filterButton().should("be.visible");
+
+    cy.log("Unverify the orders model");
+    cy.findByRole("heading", { name: "Orders Model" }).click();
+    unverifyModel();
+
+    browseModels();
+
+    cy.log("Visit the products model");
+    cy.findByRole("heading", { name: "Products Model" }).click();
+
+    browseModels();
+
+    cy.log("The filter toggle is not visible");
+    filterButton().should("not.exist");
+
+    cy.log(
+      "The verified filter, though still active, is not applied if there are no verified models",
+    );
+    cy.log("Both models are in the table - no filter is applied here");
+    ordersModel().should("exist");
+    productsModel().should("exist");
+    cy.log("Both models are in the recents grid - no filter is applied here");
+    recentOrdersModel().should("exist");
+    recentProductsModel().should("exist");
+
+    cy.log("Verify the Orders Model");
+    modelsTable().findByText("Orders Model").click();
+    verifyModel();
+
+    browseModels();
 
     cy.log("There are no icons in the table representing unverified models");
-    cy.findByRole("table").icon("model").should("not.exist");
+    modelsTable().icon("model").should("not.exist");
 
     cy.log("Show all models");
-    openFilterPopover();
-    toggle().next("label").click();
+    toggleVerificationFilter();
 
     cy.log("The Products and Orders models now both exist in the table");
     productsModel().should("exist");

enterprise/frontend/src/metabase-enterprise/content_verification/ModelFilterControls.tsx

@@ -7,7 +7,6 @@ import type {
   ModelFilterControlsProps,
 } from "metabase/browse/utils";
 import { useUserSetting } from "metabase/common/hooks";
-import { color } from "metabase/lib/colors";
 import { Button, Icon, Paper, Popover, Switch, Text } from "metabase/ui";
 
 export const ModelFilterControls = ({
@@ -74,9 +73,10 @@ const Dot = () => {
       right="0px"
       top="7px"
       radius="50%"
-      bg={color("brand")}
+      bg={"var(--mb-color-brand)"}
       w="sm"
       h="sm"
+      data-testid="filter-dot"
     />
   );
 };

frontend/src/metabase/browse/components/BrowseContainer.styled.tsx

@@ -22,7 +22,7 @@ export const BrowseSection = styled(Flex)`
   width: 100%;
 `;
 
-export const BrowseHeader = styled.header`
+export const BrowseHeader = styled.div`
   display: flex;
   flex-direction: column;
   padding: 1rem 2.5rem 3rem 2.5rem;

frontend/src/metabase/browse/components/BrowseModels.tsx

@@ -5,7 +5,6 @@ import NoResults from "assets/img/no_results.svg";
 import { useListRecentsQuery } from "metabase/api";
 import { useFetchModels } from "metabase/common/hooks/use-fetch-models";
 import { DelayedLoadingAndErrorWrapper } from "metabase/components/LoadingAndErrorWrapper/DelayedLoadingAndErrorWrapper";
-import { color } from "metabase/lib/colors";
 import {
   PLUGIN_COLLECTIONS,
   PLUGIN_CONTENT_VERIFICATION,
@@ -49,12 +48,14 @@ export const BrowseModels = () => {
   }, [modelsResult]);
 
   const { filteredModels } = useMemo(() => {
-    // If no models are verified, don't filter them
-    const filteredModels = doVerifiedModelsExist
-      ? filterModels(models, actualModelFilters, availableModelFilters)
-      : models;
+    const filteredModels = filterModels(
+      models,
+      // If no models are verified, don't filter them
+      doVerifiedModelsExist ? actualModelFilters : {},
+      availableModelFilters,
+    );
     return { filteredModels };
-  }, [actualModelFilters, doVerifiedModelsExist, models]);
+  }, [actualModelFilters, models, doVerifiedModelsExist]);
 
   const recentModelsResult = useListRecentsQuery(undefined, {
     refetchOnMountOrArgChange: true,
@@ -64,10 +65,11 @@ export const BrowseModels = () => {
     () =>
       filterModels(
         recentModelsResult.data?.filter(isRecentModel),
-        actualModelFilters,
+        // If no models are verified, don't filter them
+        doVerifiedModelsExist ? actualModelFilters : {},
         availableModelFilters,
       ),
-    [recentModelsResult.data, actualModelFilters],
+    [recentModelsResult.data, actualModelFilters, doVerifiedModelsExist],
   );
 
   const recentModels = useMemo(() => {
@@ -82,7 +84,7 @@ export const BrowseModels = () => {
 
   return (
     <BrowseContainer>
-      <BrowseHeader>
+      <BrowseHeader role="heading" data-testid="browse-models-header">
         <BrowseSection>
           <Flex
             w="100%"
@@ -93,7 +95,7 @@ export const BrowseModels = () => {
           >
             <Title order={1} color="text-dark">
               <Group spacing="sm">
-                <Icon size={24} color={color("brand")} name="model" />
+                <Icon size={24} color={"var(--mb-color-brand)"} name="model" />
                 {t`Models`}
               </Group>
             </Title>

frontend/src/metabase/browse/components/ModelsTable.tsx

@@ -115,7 +115,7 @@ export const ModelsTable = ({
   }, [isLargeDataset, showLoadingManyRows, sortedModels]);
 
   return (
-    <Table aria-label={skeleton ? undefined : "Table of models"}>
+    <Table aria-label={skeleton ? undefined : t`Table of models`}>
       <colgroup>
         {/* <col> for Name column */}
         <ModelNameColumn containerName={itemsTableContainerName} />