Skip to content
Snippets Groups Projects
Unverified Commit a7998d69 authored by Alexander Lesnenko's avatar Alexander Lesnenko Committed by GitHub
Browse files

search pagination (#15923)


* add global search results pagination

* fixup! add global search results pagination

* nit fix to allow aleksander test without borking perf too much

* fix incorrect question object in the search spec

* translate pagination

* show filters only for existing types of items for a specified search query

* fixup! show filters only for existing types of items for a specified search query

* fixup! show filters only for existing types of items for a specified search query

* Search pagination models (#15972)

* querying the models by a buncha queries

* up max filtered response because of limits

* archived string and table db id is still a thing cuz of weirdo joins and stuff

* shove models given in the normal api endpoint

* misplaced docstring

* redshift root is actually bonafide different than other roots

Co-authored-by: default avatarhowon lee <hlee.howon@gmail.com>
parent 765cef44
Branches
Tags
No related merge requests found
Showing
with 357 additions and 159 deletions
......@@ -22,7 +22,6 @@ import Group from "metabase/entities/groups";
import UserGroupSelect from "../components/UserGroupSelect";
import { USER_STATUS } from "../constants";
import { isLastPage } from "../utils";
import { loadMemberships } from "../people";
@Group.loadList({
......@@ -62,7 +61,9 @@ export default class PeopleList extends Component {
onNextPage: PropTypes.func,
onPreviousPage: PropTypes.func,
reload: PropTypes.func.isRequired,
total: PropTypes.number.isRequired,
metadata: PropTypes.shape({
total: PropTypes.number.isRequired,
}).isRequired,
};
componentDidMount() {
......@@ -99,10 +100,11 @@ export default class PeopleList extends Component {
users,
groups,
query,
total,
metadata,
onNextPage,
onPreviousPage,
} = this.props;
const { total } = metadata;
const { page, pageSize, status } = query;
......@@ -221,9 +223,10 @@ export default class PeopleList extends Component {
<PaginationControls
page={page}
pageSize={pageSize}
total={total}
itemsLength={users.length}
onNextPage={isLastPage(page, pageSize, total) ? null : onNextPage}
onPreviousPage={page > 0 ? onPreviousPage : null}
onNextPage={onNextPage}
onPreviousPage={onPreviousPage}
/>
</div>
)}
......
......@@ -10,7 +10,6 @@ import PaginationControls from "metabase/components/PaginationControls";
import User from "metabase/entities/users";
import { isLastPage } from "../../utils";
import AddMemberRow from "./AddMemberRow";
export default function GroupMembersTable({
......@@ -80,11 +79,8 @@ export default function GroupMembersTable({
page={page}
pageSize={pageSize}
itemsLength={list.length}
onNextPage={
isLastPage(page, pageSize, members.length)
? null
: onNextPage
}
total={members.length}
onNextPage={onNextPage}
onPreviousPage={onPreviousPage}
/>
</div>
......
import { useState, useMemo, useCallback, useEffect } from "react";
import { useState, useMemo, useEffect } from "react";
import { SEARCH_DEBOUNCE_DURATION } from "metabase/lib/constants";
import { usePagination } from "metabase/hooks/use-pagination";
import { USER_STATUS } from "../constants";
const MIN_SEARCH_LENGTH = 2;
export const usePagination = (initialPage = 0) => {
const [page, setPage] = useState(initialPage);
const handleNextPage = useCallback(() => setPage(prev => prev + 1), [
setPage,
]);
const handlePreviousPage = useCallback(() => setPage(prev => prev - 1), [
setPage,
]);
return {
handleNextPage,
handlePreviousPage,
setPage,
page,
};
};
// NOTE: EntityLoader is wrapped with PaginationState hoc, however,
// it is not the best place to store pagination state since we might want to
// change it from the ancestors, for instance, when we change list filter props.
......
export const isLastPage = (pageIndex, pageSize, total) =>
pageIndex === Math.ceil(total / pageSize) - 1;
import React from "react";
import PropTypes from "prop-types";
import { t } from "ttag";
import colors from "metabase/lib/colors";
import Icon, { IconWrapper } from "metabase/components/Icon";
......@@ -8,17 +9,29 @@ export default function PaginationControls({
page,
pageSize,
itemsLength,
total,
showTotal,
onNextPage,
onPreviousPage,
}) {
const isPreviousDisabled = page === 0;
const isNextDisabled =
total != null ? isLastPage(page, pageSize, total) : !onNextPage;
return (
<div className="flex align-center" aria-label="pagination">
<span className="text-bold mr1">
<div className="flex align-center text-bold" aria-label="pagination">
<span className="mr1">
{page * pageSize + 1} - {page * pageSize + itemsLength}
{showTotal && (
<React.Fragment>
<span className="text-light">&nbsp;{t`of`}&nbsp;</span>
<span data-testid="pagination-total">{total}</span>
</React.Fragment>
)}
</span>
<PaginationButton
onClick={onPreviousPage}
disabled={!onPreviousPage}
disabled={isPreviousDisabled}
data-testid="previous-page-btn"
>
<Icon name="chevronleft" />
......@@ -26,7 +39,7 @@ export default function PaginationControls({
<PaginationButton
small
onClick={onNextPage}
disabled={!onNextPage}
disabled={isNextDisabled}
data-testid="next-page-btn"
>
<Icon name="chevronright" />
......@@ -37,7 +50,7 @@ export default function PaginationControls({
const PaginationButton = IconWrapper.withComponent("button").extend`
&:disabled {
background-color: ${colors["white"]};
background-color: transparent;
color: ${colors["text-light"]};
}
`;
......@@ -46,6 +59,15 @@ PaginationControls.propTypes = {
page: PropTypes.number.isRequired,
pageSize: PropTypes.number.isRequired,
itemsLength: PropTypes.number.isRequired,
total: PropTypes.number,
showTotal: PropTypes.bool,
onNextPage: PropTypes.func,
onPreviousPage: PropTypes.func,
};
PaginationControls.defaultProps = {
showTotal: false,
};
export const isLastPage = (pageIndex, pageSize, total) =>
pageIndex === Math.ceil(total / pageSize) - 1;
......@@ -79,12 +79,12 @@ const getMemoizedEntityQuery = createMemoizedSelector(
const loaded = entityDef.selectors.getLoaded(state, { entityQuery });
const fetched = entityDef.selectors.getFetched(state, { entityQuery });
const error = entityDef.selectors.getError(state, { entityQuery });
const total = entityDef.selectors.getListTotal(state, { entityQuery });
const metadata = entityDef.selectors.getListMetadata(state, { entityQuery });
return {
entityQuery,
list: entityDef.selectors[selectorName](state, { entityQuery }),
total,
metadata,
loading,
loaded,
fetched,
......
/* eslint-disable react/prop-types */
import React from "react";
import React, { useState } from "react";
import PropTypes from "prop-types";
import { t, jt } from "ttag";
import _ from "underscore";
import Link from "metabase/components/Link";
import { Box, Flex } from "grid-styled";
......@@ -13,29 +12,92 @@ import Card from "metabase/components/Card";
import EmptyState from "metabase/components/EmptyState";
import SearchResult from "metabase/search/components/SearchResult";
import Subhead from "metabase/components/type/Subhead";
import { FILTERS } from "metabase/collections/components/ItemTypeFilterBar";
import { color } from "metabase/lib/colors";
import Icon from "metabase/components/Icon";
import NoResults from "assets/img/no_results.svg";
import PaginationControls from "metabase/components/PaginationControls";
import { usePagination } from "metabase/hooks/use-pagination";
const PAGE_PADDING = [1, 2, 4];
export default class SearchApp extends React.Component {
render() {
const { location } = this.props;
return (
<Box mx={PAGE_PADDING}>
{location.query.q && (
<Flex align="center" py={[2, 3]}>
<Subhead>{jt`Results for "${location.query.q}"`}</Subhead>
</Flex>
)}
<Box>
<Search.ListLoader query={location.query} wrapped>
{({ list }) => {
if (list.length === 0) {
return (
const PAGE_SIZE = 50;
const SEARCH_FILTERS = [
{
name: t`Dashboards`,
filter: "dashboard",
icon: "dashboard",
},
{
name: t`Collections`,
filter: "collection",
icon: "all",
},
{
name: t`Databases`,
filter: "database",
icon: "database",
},
{
name: t`Tables`,
filter: "table",
icon: "table",
},
{
name: t`Questions`,
filter: "card",
icon: "bar",
},
{
name: t`Pulses`,
filter: "pulse",
icon: "pulse",
},
{
name: t`Metrics`,
filter: "metric",
icon: "sum",
},
{
name: t`Segments`,
filter: "segment",
icon: "segment",
},
];
export default function SearchApp({ location }) {
const { handleNextPage, handlePreviousPage, setPage, page } = usePagination();
const [filter, setFilter] = useState(location.query.type);
const handleFilterChange = filterItem => {
setFilter(filterItem && filterItem.filter);
setPage(0);
};
const query = {
q: location.query.q,
limit: PAGE_SIZE,
offset: PAGE_SIZE * page,
};
if (filter) {
query.models = filter;
}
return (
<Box mx={PAGE_PADDING}>
{location.query.q && (
<Flex align="center" py={[2, 3]}>
<Subhead>{jt`Results for "${location.query.q}"`}</Subhead>
</Flex>
)}
<Box>
<Search.ListLoader query={query} wrapped>
{({ list, metadata }) => {
if (list.length === 0) {
return (
<Box w={2 / 3}>
<Card>
<EmptyState
title={t`Didn't find anything`}
......@@ -43,102 +105,92 @@ export default class SearchApp extends React.Component {
illustrationElement={<img src={NoResults} />}
/>
</Card>
);
}
const resultsByType = _.chain(list)
.groupBy("model")
.value();
const resultDisplay = resultsByType[location.query.type] || list;
const searchFilters = FILTERS.concat(
{
name: t`Metrics`,
filter: "metric",
icon: "sum",
},
{
name: t`Segments`,
filter: "segment",
icon: "segment",
},
{
name: t`Collections`,
filter: "collection",
icon: "all",
},
{
name: t`Tables`,
filter: "table",
icon: "table",
},
).filter(f => {
// check that results exist for a filter before displaying it
if (
resultsByType[f.filter] &&
resultsByType[f.filter].length > 0
) {
return f;
}
});
</Box>
);
}
return (
<Flex align="top">
<Box w={2 / 3}>
<SearchResultSection items={resultDisplay} />
</Box>
<Box ml={[1, 2]} pt={2} px={2}>
const availableModels = metadata.available_models || [];
const filters = SEARCH_FILTERS.filter(f =>
availableModels.includes(f.filter),
);
return (
<Flex align="top">
<Box w={2 / 3}>
<React.Fragment>
<SearchResultSection items={list} />
<div className="flex justify-end my2">
<PaginationControls
showTotal
pageSize={PAGE_SIZE}
page={page}
itemsLength={list.length}
total={metadata.total}
onNextPage={handleNextPage}
onPreviousPage={handlePreviousPage}
/>
</div>
</React.Fragment>
</Box>
<Box ml={[1, 2]} pt={2} px={2}>
{filters.length > 0 ? (
<Link
className="flex align-center"
mb={3}
color={!location.query.type ? color("brand") : "inherit"}
color={filter == null ? color("brand") : "inherit"}
onClick={() => handleFilterChange(null)}
to={{
pathname: location.pathname,
query: { ...location.query, type: null },
query: { ...location.query, type: undefined },
}}
>
<Icon name="search" mr={1} />
<h4>{t`All results`}</h4>
</Link>
{searchFilters.map(f => {
let isActive =
location && location.query.type === f.filter;
if (!location.query.type && !f.filter) {
isActive = true;
}
return (
<Link
key={f.filter}
className="flex align-center"
mb={3}
color={color(isActive ? "brand" : "text-medium")}
to={{
pathname: location.pathname,
query: { ...location.query, type: f.filter },
}}
>
<Icon mr={1} name={f.icon} />
<h4>{f.name}</h4>
</Link>
);
})}
</Box>
</Flex>
);
}}
</Search.ListLoader>
</Box>
) : null}
{filters.map(f => {
const isActive = filter === f.filter;
return (
<Link
key={f.filter}
className="flex align-center"
mb={3}
onClick={() => handleFilterChange(f)}
color={color(isActive ? "brand" : "text-medium")}
to={{
pathname: location.pathname,
query: { ...location.query, type: f.filter },
}}
>
<Icon mr={1} name={f.icon} />
<h4>{f.name}</h4>
</Link>
);
})}
</Box>
</Flex>
);
}}
</Search.ListLoader>
</Box>
);
}
</Box>
);
}
SearchApp.propTypes = {
location: PropTypes.object,
};
const SearchResultSection = ({ items }) => (
<Card pt={2}>
{items.map((item, index) => {
return <SearchResult key={index} result={item} />;
{items.map(item => {
return <SearchResult key={`${item.id}__${item.model}`} result={item} />;
})}
</Card>
);
SearchResultSection.propTypes = {
items: PropTypes.array,
};
import { useState, useCallback } from "react";
export const usePagination = (initialPage = 0) => {
const [page, setPage] = useState(initialPage);
const handleNextPage = useCallback(() => setPage(prev => prev + 1), [
setPage,
]);
const handlePreviousPage = useCallback(() => setPage(prev => prev - 1), [
setPage,
]);
return {
handleNextPage,
handlePreviousPage,
setPage,
page,
};
};
......@@ -405,15 +405,25 @@ export function createEntity(def: EntityDefinition): Entity {
// for now at least paginated endpoints have a 'data' property that
// contains the actual entries, if that is on the response we should
// use that as the 'results'
const results = fetched.data ? fetched.data : fetched;
const total = fetched.total;
let results;
let metadata = {};
if (fetched.data) {
const { data, ...rest } = fetched;
results = data;
metadata = rest;
} else {
results = fetched;
}
if (!Array.isArray(results)) {
throw `Invalid response listing ${entity.name}`;
}
return {
...entity.normalizeList(results),
metadata,
entityQuery,
total,
};
}),
......@@ -489,9 +499,9 @@ export function createEntity(def: EntityDefinition): Entity {
entities => entities && entities.list,
);
const getListTotal = createSelector(
const getListMetadata = createSelector(
[getEntityList],
entities => entities && entities.total,
entities => entities && entities.metadata,
);
const getList = createSelector(
......@@ -548,7 +558,7 @@ export function createEntity(def: EntityDefinition): Entity {
getLoading,
getLoaded,
getError,
getListTotal,
getListMetadata,
};
entity.selectors = {
...defaultSelectors,
......@@ -591,11 +601,12 @@ export function createEntity(def: EntityDefinition): Entity {
}
if (type === FETCH_LIST_ACTION) {
if (payload && payload.result) {
const { entityQuery, metadata, result: list } = payload;
return {
...state,
[getIdForQuery(payload.entityQuery)]: {
list: payload.result,
total: payload.total,
[getIdForQuery(entityQuery)]: {
list,
metadata,
},
};
}
......
......@@ -228,7 +228,11 @@ function InfoText({ result }) {
export default function SearchResult({ result, compact }) {
return (
<ResultLink to={result.getUrl()} compact={compact}>
<ResultLink
to={result.getUrl()}
compact={compact}
data-testid="search-result-item"
>
<Flex align="start">
<ItemIcon item={result} type={result.model} />
<Box>
......
......@@ -28,18 +28,45 @@ const setup = props => {
};
describe("PaginationControls", () => {
it("should disable pagination buttons when no callbacks provided", () => {
it("should disable pagination buttons when no callbacks provided and no total provided", () => {
const { nextPageButton, previousPageButton } = setup();
expect(nextPageButton).toBeDisabled();
expect(previousPageButton).toBeDisabled();
});
it("should disable pagination button on the first page", () => {
const { nextPageButton, previousPageButton } = setup({
total: 150,
page: 0,
pageSize: 50,
onNextPage: () => {},
onPreviousPage: () => {},
});
expect(previousPageButton).toBeDisabled();
expect(nextPageButton).not.toBeDisabled();
});
it("should disable pagination button on the last page when total is provided", () => {
const { nextPageButton, previousPageButton } = setup({
total: 150,
page: 2,
pageSize: 50,
onNextPage: () => {},
onPreviousPage: () => {},
});
expect(previousPageButton).not.toBeDisabled();
expect(nextPageButton).toBeDisabled();
});
it("should call pagination callbacks when buttons clicked", () => {
const onNextPageSpy = jest.fn();
const onPreviousPageSpy = jest.fn();
const { nextPageButton, previousPageButton } = setup({
page: 1,
onNextPage: onNextPageSpy,
onPreviousPage: onPreviousPageSpy,
});
......
import _ from "underscore";
import { restore } from "__support__/e2e/cypress";
import { SAMPLE_DATASET } from "__support__/e2e/cypress_sample_dataset";
const { ORDERS, ORDERS_ID } = SAMPLE_DATASET;
const PAGE_SIZE = 50;
const TOTAL_ITEMS = PAGE_SIZE + 1;
describe("scenarios > search", () => {
beforeEach(() => {
restore();
cy.signInAsAdmin();
});
it("should allow users to paginate results", () => {
generateQuestions(TOTAL_ITEMS);
cy.visit("/");
cy.findByPlaceholderText("Search…").type("generated question{enter}");
cy.findByTestId("previous-page-btn").should("be.disabled");
// First page
cy.findByText(`1 - ${PAGE_SIZE}`);
cy.findByTestId("pagination-total").should("have.text", TOTAL_ITEMS);
cy.findAllByTestId("search-result-item").should("have.length", PAGE_SIZE);
cy.findByTestId("next-page-btn").click();
// Second page
cy.findByText(`${PAGE_SIZE + 1} - ${TOTAL_ITEMS}`);
cy.findByTestId("pagination-total").should("have.text", TOTAL_ITEMS);
cy.findAllByTestId("search-result-item").should("have.length", 1);
cy.findByTestId("next-page-btn").should("be.disabled");
cy.findByTestId("previous-page-btn").click();
// First page
cy.findByText(`1 - ${PAGE_SIZE}`);
cy.findByTestId("pagination-total").should("have.text", TOTAL_ITEMS);
cy.findAllByTestId("search-result-item").should("have.length", PAGE_SIZE);
});
});
const generateQuestions = count => {
_.range(count).map(i =>
cy.createQuestion({
name: `generated question ${i}`,
query: {
"source-table": ORDERS_ID,
aggregation: [["count"]],
breakout: [
["field", ORDERS.CREATED_AT, { "temporal-unit": "hour-of-day" }],
],
},
}),
);
};
......@@ -352,6 +352,16 @@
(vec (map search-config/model-name->instance models))
default))
(defn- query-model-set
"Queries all models with respect to query for one result, to see if we get a result or not"
[search-ctx]
(map #((first %) :model)
(filter not-empty
(for [model search-config/searchable-models]
(let [search-query (search-query-for-model model search-ctx)
query-with-limit (h/limit search-query 1)]
(db/query query-with-limit))))))
(s/defn ^:private search
"Builds a search query that includes all of the searchable entities and runs it"
[search-ctx :- SearchContext]
......@@ -378,14 +388,15 @@
;; We get to do this slicing and dicing with the result data because
;; the pagination of search is for UI improvement, not for performance.
;; We intend for the cardinality of the search results to be below the default max before this slicing occurs
{ :total (count total-results)
:data (cond->> total-results
(some? (:offset-int search-ctx)) (drop (:offset-int search-ctx))
(some? (:limit-int search-ctx)) (take (:limit-int search-ctx)))
:limit (:limit-int search-ctx)
:offset (:offset-int search-ctx)
:table_db_id (:table-db-id search-ctx)
:models (:models search-ctx) })))
{ :total (count total-results)
:data (cond->> total-results
(some? (:offset-int search-ctx)) (drop (:offset-int search-ctx))
(some? (:limit-int search-ctx)) (take (:limit-int search-ctx)))
:available_models (query-model-set search-ctx)
:limit (:limit-int search-ctx)
:offset (:offset-int search-ctx)
:table_db_id (:table-db-id search-ctx)
:models (:models search-ctx) })))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Endpoint |
......@@ -410,11 +421,15 @@
(some? limit) (assoc :limit-int (Integer/parseInt limit))
(some? offset) (assoc :offset-int (Integer/parseInt offset))))
(api/defendpoint GET "/models"
"Get the set of models that a search query will return"
[q archived-string table-db-id] (query-model-set (search-context q archived-string table-db-id nil nil nil)))
(api/defendpoint GET "/"
"Search within a bunch of models for the substring `q`.
For the list of models, check `metabase.search.config/searchable-models.
To search in archived models, pass in `archived=true`.
To search in archived portions of models, pass in `archived=true`.
If you want, while searching tables, only tables of a certain DB id,
pass in a DB id value to `table_db_id`.
......
......@@ -22,7 +22,7 @@
(def ^:const max-filtered-results
"Number of results to return in an API response"
50)
1000)
(def ^:const stale-time-in-days
"Results older than this number of days are all considered to be equally old. In other words, there is a ranking
......
......@@ -212,6 +212,15 @@
(is (= 2 (:limit (search-request :crowberto :q "test" :limit "2" :offset "3"))))
(is (= 3 (:offset (search-request :crowberto :q "test" :limit "2" :offset "3")))))))
(deftest query-model-set
(testing "It returns some stuff when you get results"
(with-search-items-in-root-collection "test"
(is (contains? (apply hash-set (:available_models
(mt/user-http-request :crowberto :get 200 "search?q=test"))) "dashboard"))))
(testing "It returns nothing if there are no results"
(with-search-items-in-root-collection "test"
(is (= [] (:available_models (mt/user-http-request :crowberto :get 200 "search?q=noresults")))))))
(def ^:private dashboard-count-results
(letfn [(make-card [dashboard-count]
(make-result (str "dashboard-count " dashboard-count) :dashboardcard_count dashboard-count,
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment