From 2d99a3538765bc0f582a611e6baea0f44c2e371a Mon Sep 17 00:00:00 2001
From: Raphael Krut-Landau <raphael.kl@gmail.com>
Date: Tue, 6 Aug 2024 22:56:35 -0400
Subject: [PATCH] fix(webapp/browse): Fix bug where verified filter is applied
to recents even when no verified models exist (#46462)
---
.../{browse.cy.spec.js => browse.cy.spec.ts} | 139 +++++++++++++-----
.../ModelFilterControls.tsx | 4 +-
.../components/BrowseContainer.styled.tsx | 2 +-
.../browse/components/BrowseModels.tsx | 22 +--
.../browse/components/ModelsTable.tsx | 2 +-
5 files changed, 118 insertions(+), 51 deletions(-)
rename e2e/test/scenarios/onboarding/home/{browse.cy.spec.js => browse.cy.spec.ts} (62%)
diff --git a/e2e/test/scenarios/onboarding/home/browse.cy.spec.js b/e2e/test/scenarios/onboarding/home/browse.cy.spec.ts
similarity index 62%
rename from e2e/test/scenarios/onboarding/home/browse.cy.spec.js
rename to e2e/test/scenarios/onboarding/home/browse.cy.spec.ts
index 201776910e2..769f7249a94 100644
--- a/e2e/test/scenarios/onboarding/home/browse.cy.spec.js
+++ b/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");
diff --git a/enterprise/frontend/src/metabase-enterprise/content_verification/ModelFilterControls.tsx b/enterprise/frontend/src/metabase-enterprise/content_verification/ModelFilterControls.tsx
index 30544dc5c35..1acb1250272 100644
--- a/enterprise/frontend/src/metabase-enterprise/content_verification/ModelFilterControls.tsx
+++ b/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"
/>
);
};
diff --git a/frontend/src/metabase/browse/components/BrowseContainer.styled.tsx b/frontend/src/metabase/browse/components/BrowseContainer.styled.tsx
index 131e2c71401..02715838ed5 100644
--- a/frontend/src/metabase/browse/components/BrowseContainer.styled.tsx
+++ b/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;
diff --git a/frontend/src/metabase/browse/components/BrowseModels.tsx b/frontend/src/metabase/browse/components/BrowseModels.tsx
index cdfd609aa1e..49acf1d69c1 100644
--- a/frontend/src/metabase/browse/components/BrowseModels.tsx
+++ b/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>
diff --git a/frontend/src/metabase/browse/components/ModelsTable.tsx b/frontend/src/metabase/browse/components/ModelsTable.tsx
index fc8620a91ba..b9cadf9fdbf 100644
--- a/frontend/src/metabase/browse/components/ModelsTable.tsx
+++ b/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} />
--
GitLab