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

Jump to bottom

Persist sorting in Browse models #42537

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Persist sorting in Browse models #42537

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
ef10d41
rebase
rafpaf May 20, 2024
14a79a9
Fix
rafpaf May 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 35 additions & 2 deletions e2e/test/scenarios/onboarding/home/browse.cy.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ import {

const { PRODUCTS_ID } = SAMPLE_DATABASE;

const browseModels = () =>
navigationSidebar().findByRole("listitem", { name: "Browse models" }).click();

describeWithSnowplow("scenarios > browse", () => {
beforeEach(() => {
resetSnowplow();
Expand All @@ -25,7 +28,7 @@ describeWithSnowplow("scenarios > browse", () => {

it("can browse to a model", () => {
cy.visit("/");
navigationSidebar().findByLabelText("Browse models").click();
browseModels().click();
cy.location("pathname").should("eq", "/browse/models");
cy.findByRole("heading", { name: "Orders Model" }).click();
cy.url().should("include", `/model/${ORDERS_MODEL_ID}-`);
Expand Down Expand Up @@ -79,12 +82,42 @@ describeWithSnowplow("scenarios > browse", () => {

it("on an open-source instance, the Browse models page has no controls for setting filters", () => {
cy.visit("/");
cy.findByRole("listitem", { name: "Browse models" }).click();
browseModels().click();
cy.findByRole("button", { name: /filter icon/i }).should("not.exist");
cy.findByRole("switch", { name: /Only show verified models/ }).should(
"not.exist",
);
});

it("can change sorting options in the 'Browse models' table", () => {
cy.intercept("PUT", "/api/setting/browse-models-sort-column").as(
"persistSortColumn",
);
cy.intercept("PUT", "/api/setting/browse-models-sort-direction").as(
"persistSortDirection",
);
cy.visit("/");
browseModels().click();
cy.findByRole("table").within(() => {
cy.log("A model exists on the page");
cy.findByRole("heading", { name: "Orders Model" });
cy.findByRole("button", { name: "Sort by collection" })
.realHover()
.within(() => {
cy.log(
"Table is sorted by collection, ascending, by default, so the chevron up icon is shown",
);
cy.findByLabelText(/chevronup icon/).click();
cy.wait("@persistSortDirection");
});
cy.findByRole("button", { name: "Sort by name" })
.realHover()
.within(() => {
cy.findByLabelText(/chevrondown icon/).click();
cy.wait("@persistSortColumn");
});
});
});
});

describeWithSnowplowEE("scenarios > browse (EE)", () => {
Expand Down
2 changes: 2 additions & 0 deletions frontend/src/metabase-types/api/settings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,8 @@ export type UserSettings = {
"expand-browse-in-nav"?: boolean;
"expand-bookmarks-in-nav"?: boolean;
"browse-filter-only-verified-models"?: boolean;
"browse-models-sort-column"?: string;
"browse-models-sort-direction"?: string;
};

export type Settings = InstanceSettings &
Expand Down
61 changes: 56 additions & 5 deletions frontend/src/metabase/browse/components/ModelsTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { useState } from "react";
import { useEffect, useState } from "react";
import { push } from "react-router-redux";
import { t } from "ttag";

import { useUserSetting } from "metabase/common/hooks";
import EntityItem from "metabase/components/EntityItem";
import {
SortableColumnHeader,
Expand All @@ -21,7 +22,8 @@ import { color } from "metabase/lib/colors";
import { useDispatch, useSelector } from "metabase/lib/redux";
import * as Urls from "metabase/lib/urls";
import { getLocale } from "metabase/setup/selectors";
import { Icon, type IconProps } from "metabase/ui";
import type { IconProps } from "metabase/ui";
import { Icon } from "metabase/ui";
import type { ModelResult } from "metabase-types/api";

import { trackModelClick } from "../analytics";
Expand All @@ -34,7 +36,12 @@ import {
ModelNameColumn,
ModelTableRow,
} from "./ModelsTable.styled";
import { getModelDescription, sortModels } from "./utils";
import {
getModelDescription,
isValidSortColumn,
isValidSortDirection,
sortModels,
} from "./utils";

export interface ModelsTableProps {
models: ModelResult[];
Expand Down Expand Up @@ -65,6 +72,50 @@ export const ModelsTable = ({ models }: ModelsTableProps) => {
DEFAULT_SORTING_OPTIONS,
);

const [persistedSortColumn, setPersistedSortColumn] = useUserSetting(
"browse-models-sort-column",
{ debounceOnLeadingEdge: true },
);
const [persistedSortDirection, setPersistedSortDirection] = useUserSetting(
"browse-models-sort-direction",
{ debounceOnLeadingEdge: true },
);

const updateSortingOptions = ({
sort_column,
sort_direction,
}: SortingOptions) => {
setSortingOptions({ sort_column, sort_direction });
setPersistedSortColumn(sort_column);
setPersistedSortDirection(sort_direction);
};

useEffect(() => {
if (persistedSortColumn) {
if (isValidSortColumn(persistedSortColumn)) {
setSortingOptions(options => ({
...options,
sort_column: persistedSortColumn,
}));
} else {
console.error("Invalid sort column:", persistedSortColumn);
}
}
}, [persistedSortColumn, setSortingOptions]);

useEffect(() => {
if (persistedSortDirection) {
if (isValidSortDirection(persistedSortDirection)) {
setSortingOptions(options => ({
...options,
sort_direction: persistedSortDirection,
}));
} else {
console.error("Invalid sort direction:", persistedSortDirection);
}
}
}, [persistedSortDirection, setSortingOptions]);

const sortedModels = sortModels(models, sortingOptions, localeCode);

/** The name column has an explicitly set width. The remaining columns divide the remaining width. This is the percentage allocated to the collection column */
Expand All @@ -90,7 +141,7 @@ export const ModelsTable = ({ models }: ModelsTableProps) => {
<SortableColumnHeader
name="name"
sortingOptions={sortingOptions}
onSortingOptionsChange={setSortingOptions}
onSortingOptionsChange={updateSortingOptions}
style={{ paddingInlineStart: ".625rem" }}
columnHeaderProps={{
style: { paddingInlineEnd: ".5rem" },
Expand All @@ -101,7 +152,7 @@ export const ModelsTable = ({ models }: ModelsTableProps) => {
<SortableColumnHeader
name="collection"
sortingOptions={sortingOptions}
onSortingOptionsChange={setSortingOptions}
onSortingOptionsChange={updateSortingOptions}
{...collectionProps}
columnHeaderProps={{
style: {
Expand Down
10 changes: 8 additions & 2 deletions frontend/src/metabase/browse/components/utils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,15 @@ const getValueForSorting = (
};

export const isValidSortColumn = (
sort_column: string,
sort_column: string | undefined,
): sort_column is keyof ModelResult => {
return ["name", "collection"].includes(sort_column);
return !!sort_column && ["name", "collection"].includes(sort_column);
};

export const isValidSortDirection = (
direction: string | undefined,
): direction is SortDirection => {
return !!direction && ["asc", "desc"].includes(direction);
};

export const getSecondarySortColumn = (
Expand Down
1 change: 1 addition & 0 deletions frontend/src/metabase/common/hooks/use-setting/use-setting.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export const useUserSetting = <T extends keyof UserSettings>(
debounceTimeout = 200,
debounceOnLeadingEdge,
}: {
/** Should all settings be retrieved again from the API? */
shouldRefresh?: boolean;
shouldDebounce?: boolean;
debounceTimeout?: number;
Expand Down
1 change: 1 addition & 0 deletions frontend/src/metabase/components/ItemsTable/BaseItemsTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ export const SortableColumnHeader = ({
onClick={onSortingControlClick}
role="button"
isSortable={isSortable}
aria-label={isSortable ? `Sort by ${name}` : undefined}
>
{children}
{isSortable && (
Expand Down
3 changes: 2 additions & 1 deletion frontend/src/metabase/redux/settings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,10 @@ export const refreshSiteSettings = createAsyncThunk(
},
);

interface UpdateUserSettingProps<K extends keyof UserSettings> {
export interface UpdateUserSettingProps<K extends keyof UserSettings> {
key: K;
value: UserSettings[K];
/** Should all settings be retrieved again from the API? */
shouldRefresh?: boolean;
}

Expand Down
16 changes: 16 additions & 0 deletions src/metabase/models/user.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,6 +518,22 @@
:type :boolean
:default true)

(defsetting browse-models-sort-column
(deferred-tru "User preference for the sort column for the 'Browse models' table")
:user-local :only
:export? false
:visibility :authenticated
:type :string
:default "collection")

(defsetting browse-models-sort-direction
(deferred-tru "User preference for the sort direction for the 'Browse models' table")
:user-local :only
:export? false
:visibility :authenticated
:type :string
:default "asc")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are so closely related I thought about storing them together as a JSONified string, or as a string like "+name" or "-name", but this flat, explicit approach seems simplest

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We often will used a closed set of valid choices here. No worries not to, but need to make sure that the FE can handle invalid input.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the sort column is invalid - neither "asc" or "desc" - then the FE defaults to ascending

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and also the column to sort on? (no need to respond, just making sure we are defensive)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, to correct my earlier statement: if the sort direction in the API is invalid (or absent) we default to asc.

If the sort column in the API is invalid (or absent), we default to collection.

;;; ## ------------------------------------------ AUDIT LOG ------------------------------------------

(defmethod audit-log/model-details :model/User
Expand Down