Kimirock/metabase
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix Add Series modal performance issues by moving the logic to the BE (#30321)

Browse Source 
* WIP adding a new endpoint /api/card/series to fetch compatible series for a given card

* change endpoint convention and add tests

* tighten the types and more tests

* here comes the tests

* indent

* Update test/metabase/api/card_test.clj

* more tests

* fix mis-use of MLv2

* wrong args order

* update the tests to cover over limit case

* use id as last_cursor and sort id desc by default

* use the new endpoint to fetch compatible cards in the add series modal (#30410)

* use new api handler for compatible series

* exclude selected cards

* rename payload prop

* add exclude_ids param

* update contract

* use multiple value for single param pattern instead of jsonified strings

* remove debug code

* cleanup

* specs

* fix failing 20637 e2e test due to missing viz-settings in test data

* do not exclude selected cards from search

* Noah's comments

* kondo

* test fix

---------

Co-authored-by: Aleksandr Lesnenko <alxnddr@users.noreply.github.com>
Co-authored-by: Aleksandr Lesnenko <alxnddr@gmail.com>
This commit is contained in:
Ngoc Khuat
2023-05-23 22:58:44 +07:00
committed by GitHub
parent 92c988919c
commit 222dbfa720
23 changed files with 861 additions and 973 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
e2e/test/scenarios/dashboard/reproductions/20637-add-series-to-dashcard.cy.spec.js
View File

@@ -43,6 +43,10 @@ function createQuestionsAndDashboard() {
aggregation: [["count"]],
breakout: [["field", PRODUCTS.CATEGORY, null]],
},
visualization_settings: {
"graph.dimensions": ["CATEGORY"],
"graph.metrics": ["count"],
},
display: "line",
};
@@ -53,6 +57,10 @@ function createQuestionsAndDashboard() {
aggregation: [["count"]],
breakout: [["field", PRODUCTS.CATEGORY, null]],
},
visualization_settings: {
"graph.dimensions": ["CATEGORY"],
"graph.metrics": ["count"],
},
display: "bar",
};

7
frontend/src/metabase-types/api/dashboard.ts
View File

@@ -110,3 +110,10 @@ export interface LinkCardSettings {
url?: string;
entity?: LinkEntity;
}
export interface GetCompatibleCardsPayload {
last_cursor?: number;
limit: number;
query?: string;
exclude_ids: number[];
}

125
frontend/src/metabase/dashboard/components/AddSeriesModal/AddSeriesModal.jsx
View File

@@ -1,25 +1,17 @@
/* eslint-disable react/prop-types */
import React, { Component } from "react";
import PropTypes from "prop-types";
import { t } from "ttag";
import { getIn } from "icepick";
import { connect } from "react-redux";
import _ from "underscore";
import Visualization from "metabase/visualizations/components/Visualization";
import * as MetabaseAnalytics from "metabase/lib/analytics";
import { color } from "metabase/lib/colors";
import Questions from "metabase/entities/questions";
import { loadMetadataForQueries } from "metabase/redux/metadata";
import { getVisualizationRaw } from "metabase/visualizations";
import { QuestionList } from "./QuestionList";
// TODO: rework this so we don't have to load all cards up front
class AddSeriesModal extends Component {
constructor(props, context) {
super(props, context);
@@ -27,87 +19,51 @@ class AddSeriesModal extends Component {
this.state = {
error: null,
series: props.dashcard.series || [],
badQuestions: {},
isLoadingMetadata: false,
};
}
static propTypes = {
dashcard: PropTypes.object.isRequired,
questions: PropTypes.array,
dashcardData: PropTypes.object.isRequired,
fetchCardData: PropTypes.func.isRequired,
fetchDatabaseMetadata: PropTypes.func.isRequired,
setDashCardAttributes: PropTypes.func.isRequired,
loadMetadataForQueries: PropTypes.func.isRequired,
onClose: PropTypes.func.isRequired,
};
static defaultProps = {};
handleQuestionSelectedChange = async (question, selected) => {
handleQuestionSelectedChange = async (card, selected) => {
const { dashcard, dashcardData } = this.props;
const { visualization } = getVisualizationRaw([{ card: dashcard.card }]);
const card = question.card();
try {
if (selected) {
if (getIn(dashcardData, [dashcard.id, card.id]) === undefined) {
this.setState({ state: "loading" });
await this.props.fetchCardData(card, dashcard, {
reload: false,
clear: true,
});
}
const sourceDataset = getIn(this.props.dashcardData, [
dashcard.id,
dashcard.card.id,
]);
const seriesDataset = getIn(this.props.dashcardData, [
dashcard.id,
card.id,
]);
if (
visualization.seriesAreCompatible(
{ card: dashcard.card, data: sourceDataset.data },
{ card: card, data: seriesDataset.data },
)
) {
this.setState({
state: null,
series: this.state.series.concat(card),
});
MetabaseAnalytics.trackStructEvent(
"Dashboard",
"Add Series",
card.display + ", success",
);
} else {
this.setState({
state: "incompatible",
badQuestions: { ...this.state.badQuestions, [card.id]: true },
});
setTimeout(() => this.setState({ state: null }), 2000);
MetabaseAnalytics.trackStructEvent(
"Dashboard",
"Add Series",
question.type() + ", " + card.display + ", fail",
);
}
} else {
this.setState({
series: this.state.series.filter(c => c.id !== card.id),
});
MetabaseAnalytics.trackStructEvent("Dashboard", "Remove Series");
}
} catch (e) {
console.error("AddSeriesModal handleQuestionChange", e);
if (!selected) {
this.setState({
state: "incompatible",
badQuestions: { ...this.state.badQuestions, [card.id]: true },
series: this.state.series.filter(c => c.id !== card.id),
});
setTimeout(() => this.setState({ state: null }), 2000);
MetabaseAnalytics.trackStructEvent("Dashboard", "Remove Series");
return;
}
if (getIn(dashcardData, [dashcard.id, card.id]) === undefined) {
this.setState({ state: "loading" });
await this.props.fetchCardData(card, dashcard, {
reload: false,
clear: true,
});
}
this.setState({
state: null,
series: this.state.series.concat(card),
});
MetabaseAnalytics.trackStructEvent(
"Dashboard",
"Add Series",
card.display + ", success",
);
};
handleRemoveSeries(card) {
@@ -135,17 +91,7 @@ class AddSeriesModal extends Component {
};
render() {
const { dashcard, dashcardData, questions } = this.props;
const { badQuestions } = this.state;
const { visualization } = getVisualizationRaw([{ card: dashcard.card }]);
const error = this.state.error;
const enabledQuestions = {};
for (const card of this.state.series) {
enabledQuestions[card.id] = true;
}
const { dashcard, dashcardData } = this.props;
const series = [dashcard.card]
.concat(this.state.series)
@@ -212,16 +158,9 @@ class AddSeriesModal extends Component {
}}
>
<QuestionList
questions={questions}
badQuestions={badQuestions}
enabledQuestions={enabledQuestions}
error={error}
enabledCards={this.state.series}
onSelect={this.handleQuestionSelectedChange}
dashcard={this.props.dashcard}
dashcardData={this.props.dashcardData}
loadMetadataForQueries={this.handleLoadMetadata}
visualization={visualization}
isLoadingMetadata={this.state.isLoadingMetadata}
/>
</div>
</div>
@@ -229,10 +168,4 @@ class AddSeriesModal extends Component {
}
}
export default _.compose(
Questions.loadList({
query: { f: "all" },
selectorName: "getListUnfiltered",
}),
connect(null, { loadMetadataForQueries }),
)(AddSeriesModal);
export default connect(null, { loadMetadataForQueries })(AddSeriesModal);

217
frontend/src/metabase/dashboard/components/AddSeriesModal/QuestionList.jsx
View File

@@ -1,217 +0,0 @@
import React, { useState, useMemo, useEffect, useCallback } from "react";
import PropTypes from "prop-types";
import { t } from "ttag";
import { AutoSizer, List } from "react-virtualized";
import Icon from "metabase/components/Icon";
import * as MetabaseAnalytics from "metabase/lib/analytics";
import { useDebouncedValue } from "metabase/hooks/use-debounced-value";
import { SEARCH_DEBOUNCE_DURATION } from "metabase/lib/constants";
import EmptyState from "metabase/components/EmptyState";
import {
LoadMoreButton,
LoadMoreRow,
SearchContainer,
SearchInput,
QuestionListContainer,
EmptyStateContainer,
QuestionListWrapper,
} from "./QuestionList.styled";
import { QuestionListItem } from "./QuestionListItem";
import { isQuestionCompatible } from "./utils";
const LOAD_CHUNK_SIZE = 15;
const propTypes = {
questions: PropTypes.object,
badQuestions: PropTypes.object,
enabledQuestions: PropTypes.object,
error: PropTypes.string,
onSelect: PropTypes.func,
dashcard: PropTypes.object,
dashcardData: PropTypes.object,
loadMetadataForQueries: PropTypes.function,
visualization: PropTypes.object,
isLoadingMetadata: PropTypes.bool,
};
export const QuestionList = React.memo(function QuestionList({
questions,
badQuestions,
enabledQuestions,
error,
onSelect,
dashcard,
dashcardData,
loadMetadataForQueries,
visualization,
isLoadingMetadata,
}) {
const [searchText, setSearchText] = useState("");
const debouncedSearchText = useDebouncedValue(
searchText,
SEARCH_DEBOUNCE_DURATION,
);
const handleSearchFocus = () => {
MetabaseAnalytics.trackStructEvent(
"Dashboard",
"Edit Series Modal",
"search",
);
};
const filteredQuestions = useMemo(() => {
const filterText = debouncedSearchText.toLowerCase();
const filteredQuestions = questions.filter(question =>
question.displayName().toLowerCase().includes(filterText),
);
filteredQuestions.sort((a, b) => {
if (!a.isNative()) {
return 1;
} else if (!b.isNative()) {
return -1;
} else {
return 0;
}
});
return filteredQuestions;
}, [questions, debouncedSearchText]);
const compatibleQuestions = useMemo(
() =>
filteredQuestions?.filter(question => {
try {
return isQuestionCompatible(
visualization,
dashcard,
dashcardData,
question,
);
} catch (e) {
console.warn(
`Could not check question compatibility for a question with id=${question?.id?.()}`,
e,
);
return false;
}
}),
[dashcard, dashcardData, filteredQuestions, visualization],
);
const questionsWithoutMetadata = useMemo(
() =>
filteredQuestions.filter(question => {
try {
return question.isStructured() && !question.query().hasMetadata();
} catch (e) {
console.warn(
`Could not check question metadata existence for a question with id=${question?.id?.()}`,
e,
);
return false;
}
}),
[filteredQuestions],
);
const handleLoadNext = useCallback(async () => {
if (questionsWithoutMetadata.length === 0) {
return;
}
const questionsChunk = questionsWithoutMetadata.slice(0, LOAD_CHUNK_SIZE);
await loadMetadataForQueries(
questionsChunk.map(question => question.query()),
);
}, [loadMetadataForQueries, questionsWithoutMetadata]);
useEffect(() => {
handleLoadNext();
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [filteredQuestions.length]);
const hasQuestionsToShow = !!(compatibleQuestions.length > 0);
const canLoadMore = questionsWithoutMetadata.length > 0;
const rowsCount = canLoadMore
? compatibleQuestions.length + 1
: compatibleQuestions.length;
return (
<>
<SearchContainer>
<SearchInput
fullWidth
value={searchText}
colorScheme="transparent"
icon={<Icon name="search" size={16} pt="0.25rem" />}
placeholder={t`Search for a question`}
onFocus={handleSearchFocus}
onChange={e => setSearchText(e.target.value)}
/>
</SearchContainer>
<QuestionListWrapper
className="flex flex-full overflow-auto"
loading={!filteredQuestions}
error={error}
noBackground
>
<QuestionListContainer>
<AutoSizer>
{({ width, height }) => (
<List
overscanRowCount={0}
width={width}
height={height}
rowCount={rowsCount}
rowHeight={36}
rowRenderer={({ index, key, style }) => {
const isLoadMoreRow = index === compatibleQuestions.length;
if (isLoadMoreRow) {
return (
<LoadMoreRow style={style}>
<LoadMoreButton
onClick={handleLoadNext}
disabled={isLoadingMetadata}
>
{isLoadingMetadata ? t`Loading` : t`Load more`}
</LoadMoreButton>
</LoadMoreRow>
);
}
const question = compatibleQuestions[index];
const isEnabled = enabledQuestions[question.id()];
const isBad = badQuestions[question.id()];
return (
<QuestionListItem
key={key}
question={question}
isEnabled={isEnabled}
isBad={isBad}
style={style}
onChange={e => onSelect(question, e.target.checked)}
/>
);
}}
/>
)}
</AutoSizer>
{!hasQuestionsToShow && (
<EmptyStateContainer>
<EmptyState message={t`Nothing here`} icon="folder" />
</EmptyStateContainer>
)}
</QuestionListContainer>
</QuestionListWrapper>
</>
);
});
QuestionList.propTypes = propTypes;

7
frontend/src/metabase/dashboard/components/AddSeriesModal/QuestionList.styled.jsx
View File

@@ -33,7 +33,7 @@ export const LoadMoreRow = styled.li`
justify-content: center;
list-style: none;
margin: 0;
padding: 0;
padding: 0.5rem 0;
width: 100%;
`;
@@ -47,6 +47,11 @@ export const SearchContainer = styled.div`
export const SearchInput = styled(Input)`
width: 100%;
${Input.Field} {
border-radius: 0;
outline: none;
}
`;
export const EmptyStateContainer = styled.div`

152
frontend/src/metabase/dashboard/components/AddSeriesModal/QuestionList.tsx Normal file
View File

@@ -0,0 +1,152 @@
import React, { useState, useEffect, useCallback, useMemo } from "react";
import { t } from "ttag";
import { useAsyncFn } from "react-use";
import * as MetabaseAnalytics from "metabase/lib/analytics";
import { useDebouncedValue } from "metabase/hooks/use-debounced-value";
import { SEARCH_DEBOUNCE_DURATION } from "metabase/lib/constants";
import EmptyState from "metabase/components/EmptyState";
import { CardApi } from "metabase/services";
import {
Card,
CardId,
DashboardOrderedCard,
GetCompatibleCardsPayload,
} from "metabase-types/api";
import {
LoadMoreButton,
LoadMoreRow,
SearchContainer,
SearchInput,
QuestionListContainer,
EmptyStateContainer,
QuestionListWrapper,
} from "./QuestionList.styled";
import { QuestionListItem } from "./QuestionListItem";
const PAGE_SIZE = 50;
interface QuestionListProps {
enabledCards: Card[];
onSelect: (card: Card, isChecked: boolean) => void;
dashcard: DashboardOrderedCard;
}
export const QuestionList = React.memo(function QuestionList({
enabledCards,
onSelect,
dashcard,
}: QuestionListProps) {
const enabledCardIds = useMemo(
() => new Set(enabledCards.map(card => card.id)),
[enabledCards],
);
const [hasMore, setHasMore] = useState(true);
const [cards, setCards] = useState<Card[]>(enabledCards);
const [searchText, setSearchText] = useState("");
const debouncedSearchText = useDebouncedValue(
searchText,
SEARCH_DEBOUNCE_DURATION,
);
const [{ error, loading }, loadCards] = useAsyncFn(
async (searchText: string, last_cursor?: CardId) => {
if (dashcard.card_id == null) {
return;
}
const hasSearch = searchText.length > 0;
const payload: GetCompatibleCardsPayload = {
last_cursor,
limit: PAGE_SIZE,
exclude_ids: hasSearch ? [] : Array.from(enabledCardIds.values()),
};
if (hasSearch) {
payload.query = searchText;
}
const cards = await CardApi.compatibleCards({
...payload,
cardId: dashcard.card_id,
});
setCards(prev => [...prev, ...cards]);
setHasMore(cards.length === PAGE_SIZE);
},
[dashcard, debouncedSearchText],
);
useEffect(() => {
setCards(debouncedSearchText.length > 0 ? [] : enabledCards);
loadCards(debouncedSearchText);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [debouncedSearchText, loadCards]);
const handleLoadNext = useCallback(async () => {
const lastCard = cards[cards.length - 1];
loadCards(debouncedSearchText, lastCard?.id);
}, [cards, debouncedSearchText, loadCards]);
const handleSearchFocus = () => {
MetabaseAnalytics.trackStructEvent(
"Dashboard",
"Edit Series Modal",
"search",
);
};
const hasQuestionsToShow = cards.length > 0;
return (
<>
<SearchContainer>
<SearchInput
fullWidth
value={searchText}
leftIcon="search"
placeholder={t`Search for a question`}
onFocus={handleSearchFocus}
onChange={e => setSearchText(e.target.value)}
/>
</SearchContainer>
<QuestionListWrapper
className="flex flex-full overflow-auto"
error={error}
noBackground
>
<QuestionListContainer>
{hasQuestionsToShow && (
<div>
{cards.map(card => (
<QuestionListItem
key={card.id}
card={card}
isEnabled={enabledCardIds.has(card.id)}
onChange={value => onSelect(card, value)}
/>
))}
{hasMore && (
<LoadMoreRow>
<LoadMoreButton onClick={handleLoadNext} disabled={loading}>
{loading ? t`Loading` : t`Load more`}
</LoadMoreButton>
</LoadMoreRow>
)}
</div>
)}
{!hasQuestionsToShow && (
<EmptyStateContainer>
<EmptyState message={t`Nothing here`} icon="folder" />
</EmptyStateContainer>
)}
</QuestionListContainer>
</QuestionListWrapper>
</>
);
});

146
frontend/src/metabase/dashboard/components/AddSeriesModal/QuestionList.unit.spec.tsx Normal file
View File

@@ -0,0 +1,146 @@
import fetchMock from "fetch-mock";
import React from "react";
import { screen, render } from "@testing-library/react";
import _ from "underscore";
import userEvent from "@testing-library/user-event";
import {
createMockCard,
createMockDashboardOrderedCard,
} from "metabase-types/api/mocks";
import { Card, GetCompatibleCardsPayload } from "metabase-types/api";
import { QuestionList } from "./QuestionList";
const compatibleCardsFirstPage = _.range(50).map(index =>
createMockCard({ id: index, name: `compatible card ${index} page 1` }),
);
const compatibleCardsSecondPage = _.range(10).map(index => {
const id = index + 50;
return createMockCard({
id,
name: `compatible card ${id} page 2`,
});
});
const mockCompatibleCardsPage = (
cards: Card[],
query: Partial<GetCompatibleCardsPayload> = {},
) => {
fetchMock.getOnce(
{
url: "path:/api/card/1/series",
overwriteRoutes: true,
query: {
limit: 50,
...query,
},
},
cards,
);
};
describe("QuestionList", () => {
afterEach(() => {
fetchMock.reset();
});
it("should paginate compatible cards", async () => {
mockCompatibleCardsPage(compatibleCardsFirstPage);
const onSelect = jest.fn();
const dashcard = createMockDashboardOrderedCard({ card_id: 1 });
render(
<QuestionList
enabledCards={[]}
dashcard={dashcard}
onSelect={onSelect}
/>,
);
expect(
await screen.findByText("compatible card 1 page 1"),
).toBeInTheDocument();
mockCompatibleCardsPage(compatibleCardsSecondPage, {
last_cursor: _.last(compatibleCardsFirstPage)?.id,
});
userEvent.click(
await screen.findByRole("button", {
name: /load more/i,
}),
);
const secondPageItem = await screen.findByText("compatible card 50 page 2");
userEvent.click(secondPageItem);
expect(onSelect).toHaveBeenCalledWith(compatibleCardsSecondPage[0], true);
});
it("should allow searching cards by name", async () => {
mockCompatibleCardsPage(compatibleCardsFirstPage);
const onSelect = jest.fn();
const dashcard = createMockDashboardOrderedCard({ card_id: 1 });
const selectedCard = createMockCard({
id: 500,
name: "search text selected card",
});
render(
<QuestionList
enabledCards={[selectedCard]}
dashcard={dashcard}
onSelect={onSelect}
/>,
);
expect(
await screen.findByText("compatible card 1 page 1"),
).toBeInTheDocument();
mockCompatibleCardsPage(
[selectedCard, createMockCard({ id: 1000, name: "search text card" })],
{
query: "search text",
},
);
userEvent.type(
screen.getByPlaceholderText("Search for a question"),
"search text",
);
expect(await screen.findByText("search text card")).toBeInTheDocument();
expect(
await screen.findByText("search text selected card"),
).toBeInTheDocument();
});
it("should show added cards checked and allow selecting new ones", async () => {
mockCompatibleCardsPage([createMockCard({ id: 3, name: "fetched card" })]);
const cards = [createMockCard({ id: 2, name: "added card" })];
const dashcard = createMockDashboardOrderedCard({ card_id: 1 });
render(
<QuestionList
enabledCards={cards}
dashcard={dashcard}
onSelect={jest.fn()}
/>,
);
expect(
await screen.findByRole("checkbox", {
name: /added card/i,
}),
).toBeChecked();
expect(
await screen.findByRole("checkbox", {
name: /fetched card/i,
}),
).not.toBeChecked();
});
});

49
frontend/src/metabase/dashboard/components/AddSeriesModal/QuestionListItem.jsx
View File

@@ -1,49 +0,0 @@
import React from "react";
import PropTypes from "prop-types";
import { t } from "ttag";
import Tooltip from "metabase/core/components/Tooltip";
import CheckBox from "metabase/core/components/CheckBox";
import {
QuestionListItemRoot,
CheckboxContainer,
WarningIcon,
} from "./QuestionListItem.styled";
const propTypes = {
question: PropTypes.object,
onChange: PropTypes.func.isRequired,
isEnabled: PropTypes.bool,
isBad: PropTypes.bool,
style: PropTypes.object,
};
export const QuestionListItem = React.memo(function QuestionListItem({
question,
onChange,
isEnabled,
isBad,
style,
}) {
const isStructuredQuestion = question.isStructured();
const questionName = question.displayName();
return (
<QuestionListItemRoot style={style} isDisabled={isBad}>
<CheckboxContainer hasIcon={!isStructuredQuestion}>
<CheckBox
label={questionName}
labelEllipsis
checked={isEnabled}
onChange={onChange}
/>
</CheckboxContainer>
{!isStructuredQuestion && (
<Tooltip tooltip={t`We're not sure if this question is compatible`}>
<WarningIcon />
</Tooltip>
)}
</QuestionListItemRoot>
);
});
QuestionListItem.propTypes = propTypes;

4
frontend/src/metabase/dashboard/components/AddSeriesModal/QuestionListItem.styled.jsx → frontend/src/metabase/dashboard/components/AddSeriesModal/QuestionListItem.styled.tsx
View File

@@ -7,9 +7,7 @@ export const QuestionListItemRoot = styled.li`
display: flex;
margin: 0;
min-height: 36px;
opacity: ${props => (props.isDisabled ? 0.5 : 1)};
padding: 0.25rem 0.5rem 0.25rem 0.75rem;
pointer-events: ${props => (props.isDisabled ? "none" : "all")};
&:first-child {
padding-top: 0.5rem;
@@ -24,7 +22,7 @@ export const CheckboxContainer = styled.div`
display: inline-block;
padding: 0 0.5rem 0 0;
max-width: 100%;
width: ${({ hasIcon }) => (hasIcon ? "calc(100% - 36px)" : "100%")};
width: 100%;
`;
export const WarningIcon = styled(Icon)`

33
frontend/src/metabase/dashboard/components/AddSeriesModal/QuestionListItem.tsx Normal file
View File

@@ -0,0 +1,33 @@
import React from "react";
import CheckBox from "metabase/core/components/CheckBox";
import { Card } from "metabase-types/api";
import {
QuestionListItemRoot,
CheckboxContainer,
} from "./QuestionListItem.styled";
interface QuestionListItemProps {
card: Card;
onChange: (value: boolean) => void;
isEnabled: boolean;
}
export const QuestionListItem = React.memo(function QuestionListItem({
card,
onChange,
isEnabled,
}: QuestionListItemProps) {
return (
<QuestionListItemRoot>
<CheckboxContainer>
<CheckBox
label={card.name}
labelEllipsis
checked={isEnabled}
onChange={e => onChange(e.currentTarget.checked)}
/>
</CheckboxContainer>
</QuestionListItemRoot>
);
});

33
frontend/src/metabase/dashboard/components/AddSeriesModal/utils.js
View File

@@ -1,33 +0,0 @@
import { getIn } from "icepick";
export function isQuestionCompatible(
visualization,
dashcard,
dashcardData,
question,
) {
const initialSeries = {
card: dashcard.card,
data: getIn(dashcardData, [dashcard.id, dashcard.card.id, "data"]),
};
try {
if (question.id() === dashcard.card.id) {
return false;
}
if (question.isStructured()) {
if (
!visualization.seriesAreCompatible(initialSeries, {
card: question.card(),
data: { cols: question.query().columns(), rows: [] },
})
) {
return false;
}
}
return true;
} catch (e) {
return false;
}
}

1
frontend/src/metabase/services.js
View File

@@ -174,6 +174,7 @@ export const CardApi = {
// related
related: GET("/api/card/:cardId/related"),
adHocRelated: POST("/api/card/related"),
compatibleCards: GET("/api/card/:cardId/series"),
parameterValues: GET("/api/card/:cardId/params/:paramId/values"),
parameterSearch: GET("/api/card/:cardId/params/:paramId/search/:query"),
};

58
frontend/src/metabase/visualizations/components/LineAreaBarChart.jsx
View File

@@ -23,12 +23,7 @@ import {
import { getOrderedSeries } from "metabase/visualizations/lib/series";
import { getAccentColors } from "metabase/lib/colors/groups";
import { isEmpty } from "metabase/lib/validate";
import {
isNumeric,
isDate,
isDimension,
isMetric,
} from "metabase-lib/types/utils/isa";
import { isDimension, isMetric } from "metabase-lib/types/utils/isa";
import {
LineAreaBarChartRoot,
@@ -108,50 +103,6 @@ export default class LineAreaBarChart extends Component {
validateStacking(settings);
}
static seriesAreCompatible(initialSeries, newSeries) {
const initialSettings = getComputedSettingsForSeries([initialSeries]);
const newSettings = getComputedSettingsForSeries([newSeries]);
const initialDimensions = getColumnsFromNames(
initialSeries.data.cols,
initialSettings["graph.dimensions"],
);
const newDimensions = getColumnsFromNames(
newSeries.data.cols,
newSettings["graph.dimensions"],
);
const newMetrics = getColumnsFromNames(
newSeries.data.cols,
newSettings["graph.metrics"],
);
// must have at least one dimension and one metric
if (newDimensions.length === 0 || newMetrics.length === 0) {
return false;
}
// all metrics must be numeric
if (!_.all(newMetrics, isNumeric)) {
return false;
}
// both or neither primary dimension must be dates
if (isDate(initialDimensions[0]) !== isDate(newDimensions[0])) {
return false;
}
// both or neither primary dimension must be numeric
// a timestamp field is both date and number so don't enforce the condition if both fields are dates; see #2811
if (
isNumeric(initialDimensions[0]) !== isNumeric(newDimensions[0]) &&
!(isDate(initialDimensions[0]) && isDate(newDimensions[0]))
) {
return false;
}
return true;
}
static placeholderSeries = [
{
card: {
@@ -415,13 +366,6 @@ export default class LineAreaBarChart extends Component {
}
}
function getColumnsFromNames(cols, names) {
if (!names) {
return [];
}
return names.map(name => _.findWhere(cols, { name }));
}
function transformSingleSeries(s, series, seriesIndex) {
const { card, data } = s;

492
frontend/src/metabase/visualizations/components/LineAreaBarChart.unit.spec.js
View File

@@ -1,492 +0,0 @@
// TODO: To be replaced by an e2e test which doesn't require hardcoding the card objects
// HACK: Needed because of conflicts caused by circular dependencies
import "metabase/visualizations/components/Visualization";
import LineAreaBarChart from "metabase/visualizations/components/LineAreaBarChart.jsx";
const millisecondCard = {
card: {
description: null,
archived: false,
table_id: 1784,
result_metadata: [
{
base_type: "type/BigInteger",
coercion_strategy: "Coercion/UNIXMilliSeconds->DateTime",
effective_type: "type/DateTime",
display_name: "Timestamp",
name: "timestamp",
unit: "week",
},
{
base_type: "type/Integer",
display_name: "count",
name: "count",
semantic_type: "type/Number",
},
],
creator: {
email: "atte@metabase.test",
first_name: "Atte",
last_login: "2017-07-21T17:51:23.181Z",
is_qbnewb: false,
is_superuser: true,
id: 1,
last_name: "Keinänen",
date_joined: "2017-03-17T03:37:27.396Z",
common_name: "Atte Keinänen",
},
database_id: 5,
enable_embedding: false,
collection_id: null,
query_type: "query",
name: "Toucan Incidents",
query_average_duration: 501,
creator_id: 1,
updated_at: "2017-07-24T22:15:33.343Z",
made_public_by_id: null,
embedding_params: null,
cache_ttl: null,
dataset_query: {
database: 5,
type: "query",
query: {
"source-table": 1784,
aggregation: [["count"]],
breakout: [["field", 8159, { "temporal-unit": "week" }]],
},
},
id: 83,
display: "line",
visualization_settings: {
"graph.dimensions": ["timestamp"],
"graph.metrics": ["severity"],
},
created_at: "2017-07-21T19:40:40.102Z",
public_uuid: null,
},
data: {
rows: [
["2015-05-31T00:00:00.000-07:00", 46],
["2015-06-07T00:00:00.000-07:00", 47],
["2015-06-14T00:00:00.000-07:00", 40],
["2015-06-21T00:00:00.000-07:00", 60],
["2015-06-28T00:00:00.000-07:00", 7],
],
columns: ["timestamp", "count"],
native_form: {
query:
"SELECT count(*) AS \"count\", (date_trunc('week', CAST((CAST((TIMESTAMP '1970-01-01T00:00:00Z' + ((\"schema_126\".\"sad_toucan_incidents_incidents\".\"timestamp\" / 1000) * INTERVAL '1 second')) AS timestamp) + INTERVAL '1 day') AS timestamp)) - INTERVAL '1 day') AS \"timestamp\" FROM \"schema_126\".\"sad_toucan_incidents_incidents\" GROUP BY (date_trunc('week', CAST((CAST((TIMESTAMP '1970-01-01T00:00:00Z' + ((\"schema_126\".\"sad_toucan_incidents_incidents\".\"timestamp\" / 1000) * INTERVAL '1 second')) AS timestamp) + INTERVAL '1 day') AS timestamp)) - INTERVAL '1 day') ORDER BY (date_trunc('week', CAST((CAST((TIMESTAMP '1970-01-01T00:00:00Z' + ((\"schema_126\".\"sad_toucan_incidents_incidents\".\"timestamp\" / 1000) * INTERVAL '1 second')) AS timestamp) + INTERVAL '1 day') AS timestamp)) - INTERVAL '1 day') ASC",
params: null,
},
cols: [
{
description: null,
table_id: 1784,
schema_name: "schema_126",
unit: "week",
name: "timestamp",
source: "breakout",
remapped_from: null,
fk_field_id: null,
remapped_to: null,
id: 8159,
visibility_type: "normal",
target: null,
display_name: "Timestamp",
base_type: "type/BigInteger",
coercion_strategy: "Coercion/UNIXSeconds->DateTime",
effective_type: "type/DateTime",
},
{
description: null,
table_id: null,
semantic_type: "type/Number",
name: "count",
source: "aggregation",
remapped_from: null,
remapped_to: null,
id: null,
target: null,
display_name: "count",
base_type: "type/Integer",
},
],
results_metadata: {
columns: [
{
base_type: "type/BigInteger",
coercion_strategy: "Coercion/UNIXSeconds->DateTime",
effective_type: "type/DateTime",
display_name: "Timestamp",
name: "timestamp",
unit: "week",
},
{
base_type: "type/Integer",
display_name: "count",
name: "count",
semantic_type: "type/Number",
},
],
},
},
};
const dateTimeCard = {
card: {
description: null,
archived: false,
table_id: 1,
result_metadata: [
{
base_type: "type/DateTime",
effective_type: "type/DateTime",
display_name: "Created At",
name: "CREATED_AT",
description: "The date and time an order was submitted.",
unit: "month",
},
{
base_type: "type/Float",
display_name: "sum",
name: "sum",
semantic_type: "type/Number",
},
],
creator: {
email: "atte@metabase.test",
first_name: "Atte",
last_login: "2017-07-21T17:51:23.181Z",
is_qbnewb: false,
is_superuser: true,
id: 1,
last_name: "Keinänen",
date_joined: "2017-03-17T03:37:27.396Z",
common_name: "Atte Keinänen",
},
database_id: 1,
enable_embedding: false,
collection_id: null,
query_type: "query",
name: "Orders over time",
query_average_duration: 798,
creator_id: 1,
updated_at: "2017-07-24T22:15:33.603Z",
made_public_by_id: null,
embedding_params: null,
cache_ttl: null,
dataset_query: {
database: 1,
type: "query",
query: {
"source-table": 1,
aggregation: [["sum", ["field", 4, null]]],
breakout: [["field", 1, { "temporal-unit": "month" }]],
},
},
id: 25,
display: "line",
visualization_settings: {
"graph.colors": [
"#F1B556",
"#9cc177",
"#a989c5",
"#ef8c8c",
"#f9d45c",
"#F1B556",
"#A6E7F3",
"#7172AD",
"#7B8797",
"#6450e3",
"#55e350",
"#e35850",
"#77c183",
"#7d77c1",
"#c589b9",
"#bec589",
"#89c3c5",
"#c17777",
"#899bc5",
"#efce8c",
"#50e3ae",
"#be8cef",
"#8cefc6",
"#ef8cde",
"#b5f95c",
"#5cc2f9",
"#f95cd0",
"#c1a877",
"#f95c67",
],
},
created_at: "2017-04-13T21:47:08.360Z",
public_uuid: null,
},
data: {
rows: [
["2015-09-01T00:00:00.000-07:00", 533.45],
["2015-10-01T00:00:00.000-07:00", 4130.049999999998],
["2015-11-01T00:00:00.000-07:00", 6786.2599999999975],
["2015-12-01T00:00:00.000-08:00", 12494.039999999994],
["2016-01-01T00:00:00.000-08:00", 13594.169999999995],
["2016-02-01T00:00:00.000-08:00", 16607.429999999997],
["2016-03-01T00:00:00.000-08:00", 23600.45000000002],
["2016-04-01T00:00:00.000-07:00", 24051.120000000024],
["2016-05-01T00:00:00.000-07:00", 30163.87000000002],
["2016-06-01T00:00:00.000-07:00", 30547.53000000002],
["2016-07-01T00:00:00.000-07:00", 35808.49000000004],
["2016-08-01T00:00:00.000-07:00", 43856.760000000075],
["2016-09-01T00:00:00.000-07:00", 42831.96000000008],
["2016-10-01T00:00:00.000-07:00", 50299.75000000006],
["2016-11-01T00:00:00.000-07:00", 51861.37000000006],
["2016-12-01T00:00:00.000-08:00", 55982.590000000106],
["2017-01-01T00:00:00.000-08:00", 64462.70000000016],
["2017-02-01T00:00:00.000-08:00", 58228.17000000016],
["2017-03-01T00:00:00.000-08:00", 65618.70000000017],
["2017-04-01T00:00:00.000-07:00", 66682.43000000018],
["2017-05-01T00:00:00.000-07:00", 71817.04000000012],
["2017-06-01T00:00:00.000-07:00", 72691.63000000018],
["2017-07-01T00:00:00.000-07:00", 86210.1600000002],
["2017-08-01T00:00:00.000-07:00", 81121.41000000008],
["2017-09-01T00:00:00.000-07:00", 24811.320000000007],
],
columns: ["CREATED_AT", "sum"],
native_form: {
query:
'SELECT sum("PUBLIC"."ORDERS"."SUBTOTAL") AS "sum", parsedatetime(formatdatetime("PUBLIC"."ORDERS"."CREATED_AT", \'yyyyMM\'), \'yyyyMM\') AS "CREATED_AT" FROM "PUBLIC"."ORDERS" GROUP BY parsedatetime(formatdatetime("PUBLIC"."ORDERS"."CREATED_AT", \'yyyyMM\'), \'yyyyMM\') ORDER BY parsedatetime(formatdatetime("PUBLIC"."ORDERS"."CREATED_AT", \'yyyyMM\'), \'yyyyMM\') ASC',
params: null,
},
cols: [
{
description: "The date and time an order was submitted.",
table_id: 1,
schema_name: "PUBLIC",
semantic_type: null,
unit: "month",
name: "CREATED_AT",
source: "breakout",
remapped_from: null,
fk_field_id: null,
remapped_to: null,
id: 1,
visibility_type: "normal",
target: null,
display_name: "Created At",
base_type: "type/DateTime",
},
{
description: null,
table_id: null,
semantic_type: "type/Number",
name: "sum",
source: "aggregation",
remapped_from: null,
remapped_to: null,
id: null,
target: null,
display_name: "sum",
base_type: "type/Float",
},
],
results_metadata: {
columns: [
{
base_type: "type/DateTime",
display_name: "Created At",
name: "CREATED_AT",
description: "The date and time an order was submitted.",
unit: "month",
},
{
base_type: "type/Float",
display_name: "sum",
name: "sum",
semantic_type: "type/Number",
},
],
},
},
};
const numberCard = {
card: {
description: null,
archived: false,
labels: [],
table_id: 4,
result_metadata: [
{
base_type: "type/Integer",
display_name: "Ratings",
name: "RATING",
description: "The rating (on a scale of 1-5) the user left.",
semantic_type: "type/Number",
},
{
base_type: "type/Integer",
display_name: "count",
name: "count",
semantic_type: "type/Number",
},
],
creator: {
email: "atte@metabase.test",
first_name: "Atte",
last_login: "2017-07-21T17:51:23.181Z",
is_qbnewb: false,
is_superuser: true,
id: 1,
last_name: "Keinänen",
date_joined: "2017-03-17T03:37:27.396Z",
common_name: "Atte Keinänen",
},
database_id: 1,
enable_embedding: false,
collection_id: 2,
query_type: "query",
name: "Reviews by Rating",
creator_id: 1,
updated_at: "2017-07-24T22:15:29.911Z",
made_public_by_id: null,
embedding_params: null,
cache_ttl: null,
dataset_query: {
database: 1,
type: "query",
query: {
"source-table": 4,
aggregation: [["count"]],
breakout: [["field", 33, null]],
},
},
id: 86,
display: "line",
visualization_settings: {},
collection: {
id: 2,
name: "Order Statistics",
slug: "order_statistics",
description: null,
color: "#7B8797",
archived: false,
},
favorite: false,
created_at: "2017-07-24T22:15:29.911Z",
public_uuid: null,
},
data: {
rows: [
[1, 59],
[2, 77],
[3, 64],
[4, 550],
[5, 328],
],
columns: ["RATING", "count"],
native_form: {
query:
'SELECT count(*) AS "count", "PUBLIC"."REVIEWS"."RATING" AS "RATING" FROM "PUBLIC"."REVIEWS" GROUP BY "PUBLIC"."REVIEWS"."RATING" ORDER BY "PUBLIC"."REVIEWS"."RATING" ASC',
params: null,
},
cols: [
{
description: "The rating (on a scale of 1-5) the user left.",
table_id: 4,
schema_name: "PUBLIC",
semantic_type: "type/Number",
name: "RATING",
source: "breakout",
remapped_from: null,
fk_field_id: null,
remapped_to: null,
id: 33,
visibility_type: "normal",
target: null,
display_name: "Ratings",
base_type: "type/Integer",
},
{
description: null,
table_id: null,
semantic_type: "type/Number",
name: "count",
source: "aggregation",
remapped_from: null,
remapped_to: null,
id: null,
target: null,
display_name: "count",
base_type: "type/Integer",
},
],
results_metadata: {
columns: [
{
base_type: "type/Integer",
display_name: "Ratings",
name: "RATING",
description: "The rating (on a scale of 1-5) the user left.",
semantic_type: "type/Number",
},
{
base_type: "type/Integer",
display_name: "count",
name: "count",
semantic_type: "type/Number",
},
],
},
},
};
// jsdom doesn't support layout methods like getBBox, so we need to mock it.
window.SVGElement.prototype.getBBox = () => ({
x: 0,
y: 0,
width: 1000,
height: 1000,
});
describe("LineAreaBarChart", () => {
it("should let you combine series with datetimes only", () => {
expect(
LineAreaBarChart.seriesAreCompatible(dateTimeCard, dateTimeCard),
).toBe(true);
});
it("should let you combine series with UNIX millisecond timestamps only", () => {
expect(
LineAreaBarChart.seriesAreCompatible(dateTimeCard, dateTimeCard),
).toBe(true);
});
it("should let you combine series with numbers only", () => {
expect(LineAreaBarChart.seriesAreCompatible(numberCard, numberCard)).toBe(
true,
);
});
it("should let you combine series with UNIX millisecond timestamps and datetimes", () => {
expect(
LineAreaBarChart.seriesAreCompatible(millisecondCard, dateTimeCard),
).toBe(true);
expect(
LineAreaBarChart.seriesAreCompatible(dateTimeCard, millisecondCard),
).toBe(true);
});
it("should not let you combine series with UNIX millisecond timestamps and numbers", () => {
expect(
LineAreaBarChart.seriesAreCompatible(numberCard, millisecondCard),
).toBe(false);
expect(
LineAreaBarChart.seriesAreCompatible(millisecondCard, numberCard),
).toBe(false);
});
it("should not let you combine series with datetimes and numbers", () => {
expect(LineAreaBarChart.seriesAreCompatible(numberCard, dateTimeCard)).toBe(
false,
);
expect(LineAreaBarChart.seriesAreCompatible(dateTimeCard, numberCard)).toBe(
false,
);
});
});

1
frontend/src/metabase/visualizations/visualizations/PivotTable/PivotTable.tsx
View File

@@ -497,7 +497,6 @@ export default Object.assign(connect(mapStateToProps)(PivotTable), {
settings,
columnSettings,
isLiveResizable: () => false,
seriesAreCompatible: () => false,
});
export { PivotTable };

7
frontend/src/metabase/visualizations/visualizations/Scalar.jsx
View File

@@ -51,13 +51,6 @@ export default class Scalar extends Component {
// scalar can always be rendered, nothing needed here
}
static seriesAreCompatible(initialSeries, newSeries) {
if (newSeries.data.cols && newSeries.data.cols.length === 1) {
return true;
}
return false;
}
static transformSeries(series) {
if (series.length > 1) {
return series.map((s, seriesIndex) => ({

182
src/metabase/api/card.clj
View File

@@ -16,6 +16,7 @@
[metabase.driver.util :as driver.u]
[metabase.email.messages :as messages]
[metabase.events :as events]
[metabase.lib.types.isa :as lib.types.isa]
[metabase.mbql.normalize :as mbql.normalize]
[metabase.mbql.util :as mbql.u]
[metabase.models
@@ -39,13 +40,14 @@
[metabase.query-processor.pivot :as qp.pivot]
[metabase.query-processor.util :as qp.util]
[metabase.related :as related]
[metabase.server.middleware.offset-paging :as mw.offset-paging]
[metabase.sync :as sync]
[metabase.sync.analyze.query-results :as qr]
[metabase.task.persist-refresh :as task.persist-refresh]
[metabase.upload :as upload]
[metabase.util :as u]
[metabase.util.date-2 :as u.date]
[metabase.util.i18n :refer [trs tru]]
[metabase.util.i18n :refer [deferred-tru trs tru]]
[metabase.util.log :as log]
[metabase.util.malli :as mu]
[metabase.util.malli.schema :as ms]
@@ -144,11 +146,10 @@
(filter (fn [card] (some #{model-id} (-> card :dataset_query query/collect-card-ids))))))
(defn- cards-for-filter-option [filter-option model-id-or-nil]
(-> (apply cards-for-filter-option* (or filter-option :all) (when model-id-or-nil [model-id-or-nil]))
(-> (apply cards-for-filter-option* filter-option (when model-id-or-nil [model-id-or-nil]))
(hydrate :creator :collection)))
;;; -------------------------------------------- Fetching a Card or Cards --------------------------------------------
(def ^:private card-filter-options
"a valid card filter option."
(map name (keys (methods cards-for-filter-option*))))
@@ -156,12 +157,12 @@
(api/defendpoint GET "/"
"Get all the Cards. Option filter param `f` can be used to change the set of Cards that are returned; default is
`all`, but other options include `mine`, `bookmarked`, `database`, `table`, `recent`, `popular`, :using_model
and `archived`. See corresponding implementation functions above for the specific behavior of each filter
and `archived`. See corresponditng implementation functions above for the specific behavior of each filter
option. :card_index:"
[f model_id]
{f [:maybe (into [:enum] card-filter-options)]
model_id [:maybe ms/PositiveInt]}
(let [f (keyword f)]
(let [f (or (keyword f) :all)]
(when (contains? #{:database :table :using_model} f)
(api/checkp (integer? model_id) "model_id" (format "model_id is a required parameter when filter mode is '%s'"
(name f)))
@@ -169,7 +170,7 @@
:database (api/read-check Database model_id)
:table (api/read-check Database (t2/select-one-fn :db_id Table, :id model_id))
:using_model (api/read-check Card model_id)))
(let [cards (filter mi/can-read? (cards-for-filter-option f model_id))
(let [cards (filter mi/can-read? (cards-for-filter-option f model_id))
last-edit-info (:card (last-edit/fetch-last-edited-info {:card-ids (map :id cards)}))]
(into []
(map (fn [{:keys [id] :as card}]
@@ -200,6 +201,175 @@
(when-not ignore_view
(events/publish-event! :card-read (assoc <> :actor_id api/*current-user-id*))))))
(defn- card-columns-from-names
[card names]
(when-let [names (set names)]
(filter #(names (:name %)) (:result_metadata card))))
(defn- cols->kebab-case
[cols]
(map #(update-keys % u/->kebab-case-en) cols))
(defn- area-bar-line-series-are-compatible?
[first-card second-card]
(and (#{:area :line :bar} (:display second-card))
(let [initial-dimensions (cols->kebab-case
(card-columns-from-names
first-card
(get-in first-card [:visualization_settings :graph.dimensions])))
new-dimensions (cols->kebab-case
(card-columns-from-names
second-card
(get-in second-card [:visualization_settings :graph.dimensions])))
new-metrics (cols->kebab-case
(card-columns-from-names
second-card
(get-in second-card [:visualization_settings :graph.metrics])))]
(cond
;; must have at least one dimension and one metric
(or (zero? (count new-dimensions))
(zero? (count new-metrics)))
false
;; all metrics must be numeric
(not (every? lib.types.isa/numeric? new-metrics))
false
;; both or neither primary dimension must be dates
(not= (lib.types.isa/date? (first initial-dimensions))
(lib.types.isa/date? (first new-dimensions)))
false
;; both or neither primary dimension must be numeric
;; a timestamp field is both date and number so don't enforce the condition if both fields are dates; see #2811
(and (not= (lib.types.isa/numeric? (first initial-dimensions))
(lib.types.isa/numeric? (first new-dimensions)))
(not (and
(lib.types.isa/date? (first initial-dimensions))
(lib.types.isa/date? (first new-dimensions)))))
false
:else true))))
(defmulti series-are-compatible?
"Check if the `second-card` is compatible to be used as series of `card`."
(fn [card _second-card]
(:display card)))
(defmethod series-are-compatible? :area
[first-card second-card]
(area-bar-line-series-are-compatible? first-card second-card))
(defmethod series-are-compatible? :line
[first-card second-card]
(area-bar-line-series-are-compatible? first-card second-card))
(defmethod series-are-compatible? :bar
[first-card second-card]
(area-bar-line-series-are-compatible? first-card second-card))
(defmethod series-are-compatible? :scalar
[first-card second-card]
(and (= :scalar (:display second-card))
(= 1
(count (:result_metadata first-card))
(count (:result_metadata second-card)))))
(def ^:private supported-series-display-type (set (keys (methods series-are-compatible?))))
(defn- fetch-compatible-series*
"Implementaiton of `fetch-compatible-series`.
Provide `page-size` to limit the number of cards returned, it does not guaranteed to return exactly `page-size` cards.
Use `fetch-compatible-series` for that."
[card {:keys [query last-cursor page-size exclude-ids] :as _options}]
(let [matching-cards (t2/select Card
:archived false
:display [:in supported-series-display-type]
:id [:not= (:id card)]
(cond-> {:order-by [[:id :desc]]
:where [:and]}
last-cursor
(update :where conj [:< :id last-cursor])
(seq exclude-ids)
(update :where conj [:not [:in :id exclude-ids]])
query
(update :where conj [:like :%lower.name (str "%" (u/lower-case-en query) "%")])
;; add a little buffer to the page to account for cards that are not
;; compatible + do not have permissions to read
;; this is just a heuristic, but it should be good enough
page-size
(assoc :limit (+ 10 page-size))))
compatible-cards (->> matching-cards
(filter mi/can-read?)
(filter #(or
;; columns name on native query are not match with the column name in viz-settings. why??
;; so we can't use series-are-compatible? to filter out incompatible native cards.
;; => we assume all native queries are compatible and FE will figure it out later
(= (:query_type %) :native)
(series-are-compatible? card %))))]
(if page-size
(take page-size compatible-cards)
compatible-cards)))
(defn- fetch-compatible-series
"Fetch a list of compatible series for `card`.
options:
- exclude-ids: filter out these card ids
- query: filter cards by name
- last-cursor: the id of the last card from the previous page
- page-size: is nullable, it'll try to fetches exactly `page-size` cards if there are enough cards."
([card options]
(fetch-compatible-series card options []))
([card {:keys [page-size] :as options} current-cards]
(let [cards (fetch-compatible-series* card options)
new-cards (concat current-cards cards)]
;; if the total card fetches is less than page-size and there are still more, continue fetching
(if (and (some? page-size)
(seq cards)
(< (count cards) page-size))
(fetch-compatible-series card
(merge options
{:page-size (- page-size (count cards))
:last-cursor (:id (last cards))})
new-cards)
new-cards))))
(api/defendpoint GET "/:id/series"
"Fetches a list of comptatible series with the card with id `card_id`.
- `last_cursor` with value is the id of the last card from the previous page to fetch the next page.
- `query` to search card by name.
- `exclude_ids` to filter out a list of card ids"
[id last_cursor query exclude_ids]
{id int?
last_cursor [:maybe ms/PositiveInt]
query [:maybe ms/NonBlankString]
exclude_ids [:maybe [:fn
{:error/fn (fn [_ _] (deferred-tru "value must be a sequence of positive integers"))}
(fn [ids]
(every? pos-int? (api/parse-multi-values-param ids parse-long)))]]}
(let [exclude_ids (when exclude_ids (api/parse-multi-values-param exclude_ids parse-long))
card (-> (t2/select-one :model/Card :id id) api/check-404 api/read-check)
card-display (:display card)]
(when-not (supported-series-display-type card-display)
(throw (ex-info (tru "Card with type {0} is not compatible to have series" (name card-display))
{:display card-display
:allowed-display (map name supported-series-display-type)
:status-code 400})))
(fetch-compatible-series
card
{:exclude-ids exclude_ids
:query query
:last-cursor last_cursor
:page-size mw.offset-paging/*limit*})))
(api/defendpoint GET "/:id/timelines"
"Get the timelines for card with ID. Looks up the collection the card is in and uses that."
[id include start end]

23
src/metabase/api/common.clj
View File

@@ -608,9 +608,32 @@
(reconcile-position-for-collection! old-collection-id old-position nil)
(reconcile-position-for-collection! new-collection-id nil new-position))))))
;;; ------------------------------------------ PARAM PARSING FNS ----------------------------------------
(defn bit->boolean
"Coerce a bit returned by some MySQL/MariaDB versions in some situations to Boolean."
[v]
(if (number? v)
(not (zero? v))
v))
(defn parse-multi-values-param
"Parse a param that could have a single value or multiple values using `parse-fn`.
Always return a vector.
Used for API that can parse single value or multiple values for a param:
e.g:
single value: api/card/series?exclude_ids=1
multi values: api/card/series?exclude_ids=1&exclude_ids=2
Example usage:
(parse-multi-values-param \"1\" parse-long)
=> [1]
(parse-multi-values-param [\"1\" \"2\"] parse-long)
=> [1, 2]"
[xs parse-fn]
(if (sequential? xs)
(map parse-fn xs)
[(parse-fn xs)]))

230
test/metabase/api/card_test.clj
View File

@@ -53,7 +53,8 @@
[metabase.util.schema :as su]
[schema.core :as s]
[toucan.hydrate :refer [hydrate]]
[toucan2.core :as t2])
[toucan2.core :as t2]
[toucan2.tools.with-temp :as t2.with-temp])
(:import
(java.io ByteArrayInputStream)
(java.util UUID)
@@ -213,9 +214,9 @@
(is (= true
(card-returned? :database (mt/id) card-1)))
(is (= false
(card-returned? :database db card-1)))
(card-returned? :database db card-1)))
(is (= true
(card-returned? :database db card-2))))))
(card-returned? :database db card-2))))))
(deftest authentication-test
@@ -398,6 +399,229 @@
(into #{} (map :name) (mt/user-http-request :rasta :get 200 "card"
:f :using_model :model_id model-id))))))))
(deftest get-series-for-card-permission-test
(t2.with-temp/with-temp
[:model/Card {card-id :id} {:name "Card"
:display "line"
:collection_id (t2/select-one-pk Collection :personal_owner_id (mt/user->id :crowberto))}]
(is (= "You don't have permissions to do that."
(mt/user-http-request :rasta :get 403 (format "card/%d/series" card-id))))
(is (seq? (mt/user-http-request :crowberto :get 200 (format "card/%d/series" card-id))))))
(deftest get-series-for-card-type-check-test
(testing "400 if the card's display is not comptaible"
(t2.with-temp/with-temp
[:model/Card {card-id :id} {:name "Card"
:display "table"}]
(is (= "Card with type table is not compatible to have series"
(:message (mt/user-http-request :crowberto :get 400 (format "card/%d/series" card-id)))))))
(testing "404 if the card does not exsits"
(is (= "Not found."
(mt/user-http-request :crowberto :get 404 (format "card/%d/series" Integer/MAX_VALUE))))))
(deftest get-series-check-compatibility-test
(let [simple-mbql-chart-query (fn [attrs]
(merge (mt/card-with-source-metadata-for-query
(mt/mbql-query venues {:aggregation [[:sum $venues.price]]
:breakout [$venues.category_id]}))
{:visualization_settings {:graph.metrics ["sum"]
:graph.dimensions ["CATEGORY_ID"]}}
attrs))]
(t2.with-temp/with-temp
[;; comptaible cards
:model/Card line (simple-mbql-chart-query {:name "A Line" :display :line})
:model/Card bar (simple-mbql-chart-query {:name "A Bar" :display :bar})
:model/Card area (simple-mbql-chart-query {:name "An Area" :display :area})
:model/Card scalar (merge (mt/card-with-source-metadata-for-query
(mt/mbql-query venues {:aggregation [[:count]]}))
{:name "A Scalar 1" :display :scalar})
:model/Card scalar-2(merge (mt/card-with-source-metadata-for-query
(mt/mbql-query venues {:aggregation [[:count]]}))
{:name "A Scalar 2" :display :scalar})
:model/Card native (merge (mt/card-with-source-metadata-for-query (mt/native-query {:query "select sum(price) from venues;"}))
{:name "A Native query"
:display :scalar
:query_type "native"})
;; compatible but user doesn't have access so should not be readble
:model/Card _ (simple-mbql-chart-query {:name "A Line with no access" :display :line})
;; incomptabile cards
:model/Card pie (simple-mbql-chart-query {:name "A pie" :display :pie})
:model/Card table (simple-mbql-chart-query {:name "A table" :display :table})
:model/Card native-2(merge (mt/card-with-source-metadata-for-query (mt/native-query {:query "select sum(price) from venues;"}))
{:name "A Native query table"
:display :table
:query_type "native"})]
(with-cards-in-readable-collection [line bar area scalar scalar-2 native pie table native-2]
(doseq [[card-id display-type expected]
[[(:id line) :line #{"A Native query" "An Area" "A Bar"}]
[(:id bar) :bar #{"A Native query" "An Area" "A Line"}]
[(:id area) :area #{"A Native query" "A Bar" "A Line"}]
[(:id scalar) :scalar #{"A Native query" "A Scalar 2"}]]]
(testing (format "Card with display-type=%s should have compatible cards correctly returned" display-type)
(let [returned-card-names (set (map :name (mt/user-http-request :rasta :get 200 (format "/card/%d/series" card-id))))]
(is (set/subset? expected returned-card-names))
(is (not (contains? returned-card-names #{"A pie" "A table" "A Line with no access"}))))))))))
(deftest paging-and-filtering-works-for-series-card-test
(let [simple-mbql-chart-query (fn [attrs]
(merge (mt/card-with-source-metadata-for-query
(mt/mbql-query venues {:aggregation [[:sum $venues.price]]
:breakout [$venues.category_id]}))
{:visualization_settings {:graph.metrics ["sum"]
:graph.dimensions ["CATEGORY_ID"]}}
attrs))]
(t2.with-temp/with-temp
[:model/Card card (simple-mbql-chart-query {:name "Captain Toad" :display :line})
:model/Card card1 (simple-mbql-chart-query {:name "Luigi 1" :display :line})
:model/Card _ (simple-mbql-chart-query {:name "Luigi 2" :display :line})
:model/Card _ (simple-mbql-chart-query {:name "Luigi 3" :display :line})
:model/Card card4 (simple-mbql-chart-query {:name "Luigi 4" :display :line})
:model/Card _ (simple-mbql-chart-query {:name "Luigi 5" :display :line})
:model/Card _ (simple-mbql-chart-query {:name "Luigi 6" :display :line})
:model/Card card7 (simple-mbql-chart-query {:name "Luigi 7" :display :line})
:model/Card card8 (simple-mbql-chart-query {:name "Luigi 8" :display :line})]
(testing "filter by name works"
(is (true? (every? #(str/includes? % "Toad")
(->> (mt/user-http-request :crowberto :get 200 (format "/card/%d/series" (:id card)) :query "toad")
(map :name)))))
(testing "exclude ids works"
(testing "with single id"
(is (true?
(every?
#(not (#{(:id card) (:id card8)} %))
(->> (mt/user-http-request :crowberto :get 200 (format "/card/%d/series" (:id card))
:query "luigi" :exclude_ids (:id card8))
(map :id))))))
(testing "with multiple ids"
(is (true?
(every?
#(not (#{(:id card) (:id card7) (:id card8)} %))
(->> (mt/user-http-request :crowberto :get 200 (format "/card/%d/series" (:id card))
:query "luigi" :exclude_ids (:id card7) :exclude_ids (:id card8))
(map :id)))))))
(testing "with limit and sort by id descending"
(is (= ["Luigi 8" "Luigi 7" "Luigi 6" "Luigi 5"]
(->> (mt/user-http-request :crowberto :get 200 (format "/card/%d/series" (:id card))
:query "luigi" :limit 4)
(map :name))))
(testing "and paging works too"
(is (= ["Luigi 3" "Luigi 2" "Luigi 1"]
(->> (mt/user-http-request :crowberto :get 200 (format "/card/%d/series" (:id card))
:query "luigi" :limit 10 :last_cursor (:id card4))
(map :name)))))
(testing "And returning empty list if reaches there are nothing..."
(is (= []
(->> (mt/user-http-request :crowberto :get 200 (format "/card/%d/series" (:id card))
:query "luigi" :limit 10 :last_cursor (:id card1))
(map :name))))))))))
(def ^:private millisecond-card
{:name "Card with dimension is unixtimestmap"
:visualization_settings {:graph.dimensions ["timestamp"]
:graph.metrics ["severity"]}
:display :line
:result_metadata [{:base_type :type/BigInteger
:coercion_strategy :Coercion/UNIXMilliSeconds->DateTime
:effective_type :type/DateTime
:display_name "Timestamp"
:name "timestamp"
:unit "week"}
{:base_type :type/Integer
:display_name "count"
:name "severity"
:semantic_type :type/Number}]})
(deftest sereies-are-compatible-test
(mt/dataset sample-dataset
(testing "area-line-bar charts"
(t2.with-temp/with-temp
[:model/Card datetime-card (merge (mt/card-with-source-metadata-for-query
(mt/mbql-query orders {:aggregation [[:sum $orders.total]]
:breakout [!month.orders.created_at]}))
{:visualization_settings {:graph.metrics ["sum"]
:graph.dimensions ["CREATED_AT"]}}
{:name "datetime card"
:display :line})
:model/Card number-card (merge (mt/card-with-source-metadata-for-query
(mt/mbql-query orders {:aggregation [:count]
:breakout [$orders.quantity]}))
{:visualization_settings {:graph.metrics ["count"]
:graph.dimensions ["QUANTITY"]}}
{:name "number card"
:display :line})
:model/Card without-metric-card (merge (mt/card-with-source-metadata-for-query
(mt/mbql-query orders {:breakout [!month.orders.created_at]}))
{:visualization_settings {:graph.dimensions ["CREATED_AT"]}}
{:name "card has no metric"
:display :line})
:model/Card combo-card (merge (mt/card-with-source-metadata-for-query
(mt/mbql-query orders {:aggregation [[:sum $orders.total]]
:breakout [!month.orders.created_at]}))
{:visualization_settings {:graph.metrics ["sum"]
:graph.dimensions ["CREATED_AT"]}}
{:name "table card"
:display :combo})]
(testing "2 datetime cards can be combined"
(is (true? (api.card/series-are-compatible? datetime-card datetime-card))))
(testing "2 number cards can be combined"
(is (true? (api.card/series-are-compatible? number-card number-card))))
(testing "number card can't be combined with datetime cards"
(is (false? (api.card/series-are-compatible? number-card datetime-card)))
(is (false? (api.card/series-are-compatible? datetime-card number-card))))
(testing "can combine series with UNIX millisecond timestamp and datetime"
(is (true? (api.card/series-are-compatible? millisecond-card datetime-card)))
(is (true? (api.card/series-are-compatible? datetime-card millisecond-card))))
(testing "can't combines series with UNIX milliseceond timestamp and number"
(is (false? (api.card/series-are-compatible? millisecond-card number-card)))
(is (false? (api.card/series-are-compatible? number-card millisecond-card))))
(testing "second card must has a metric"
(is (false? (api.card/series-are-compatible? datetime-card without-metric-card))))
(testing "can't combine card of any other types rather than line/bar/area"
(is (nil? (api.card/series-are-compatible? datetime-card combo-card)))))))
(testing "scalar test"
(t2.with-temp/with-temp
[:model/Card scalar-1 (merge (mt/card-with-source-metadata-for-query
(mt/mbql-query venues {:aggregation [[:count]]}))
{:name "A Scalar 1" :display :scalar})
:model/Card scalar-2 (merge (mt/card-with-source-metadata-for-query
(mt/mbql-query venues {:aggregation [[:count]]}))
{:name "A Scalar 2" :display :scalar})
:model/Card scalar-2-cols (merge (mt/card-with-source-metadata-for-query
(mt/mbql-query venues {:aggregation [[:count]
[:sum $venues.price]]}))
{:name "A Scalar with 2 columns" :display :scalar})
:model/Card line-card (merge (mt/card-with-source-metadata-for-query
(mt/mbql-query venues {:aggregation [[:sum $venues.price]]
:breakout [$venues.category_id]}))
{:visualization_settings {:graph.metrics ["sum"]
:graph.dimensions ["CATEGORY_ID"]}}
{:name "Line card" :display :line})]
(testing "2 scalars with 1 column can be combined"
(is (true? (api.card/series-are-compatible? scalar-1 scalar-2))))
(testing "can't be combined if either one of 2 cards has more than one column"
(is (false? (api.card/series-are-compatible? scalar-1 scalar-2-cols)))
(is (false? (api.card/series-are-compatible? scalar-2-cols scalar-2))))
(testing "can only be cominbed with scalar cards"
(is (false? (api.card/series-are-compatible? scalar-1 line-card)))))))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | CREATING A CARD (POST /api/card) |
;;; +----------------------------------------------------------------------------------------------------------------+

7
test/metabase/api/common_test.clj
View File

@@ -144,3 +144,10 @@
(macroexpand '(metabase.api.common/defendpoint-schema ^:multipart compojure.core/GET "/:id" [id]
{id metabase.util.schema/IntGreaterThanZero}
(select-one Card :id id))))))))
(deftest parse-multi-values-param-test
(testing "single value returns a vector with 1 elem"
(is (= [1] (api/parse-multi-values-param "1" parse-long))))
(testing "multi values a vector as well"
(is (= [1 2 3] (api/parse-multi-values-param ["1" "2" "3"] parse-long)))))

20
test/metabase/http_client.clj
View File

@@ -37,9 +37,14 @@
(str "?" (str/join \& (letfn [(url-encode [s]
(cond-> s
(keyword? s) u/qualified-name
true codec/url-encode))]
(for [[k v] query-parameters]
(str (url-encode k) \= (url-encode v)))))))))
true codec/url-encode))
(encode-key-value [k v]
(str (url-encode k) \= (url-encode v)))]
(flatten (for [[k value-or-values] query-parameters]
(if (sequential? value-or-values)
(for [v value-or-values]
(encode-key-value k v))
[(encode-key-value k value-or-values)])))))))))
;;; parse-response
@@ -238,8 +243,13 @@
(cond-> parsed
;; un-nest {:request-options {:request-options <my-options>}} => {:request-options <my-options>}
(:request-options parsed) (update :request-options :request-options)
;; convert query parameters into a flat map [{:k :a, :v 1} {:k :b, :v 2}] => {:a 1, :b 2}
(:query-parameters parsed) (update :query-parameters (partial into {} (map (juxt :k :v)))))))
;; convert query parameters into a flat map [{:k :a, :v 1} {:k :b, :v 2} {:k :b, :v 3}] => {:a 1, :b [2 3]}
(:query-parameters parsed) (update :query-parameters (fn [query-params]
(update-vals (group-by :k query-params)
(fn [values]
(if (> (count values) 1)
(map :v values)
(:v (first values))))))))))
(def ^:private response-timeout-ms (u/seconds->ms 45))

26
test/metabase/http_client_test.clj Normal file
View File

@@ -0,0 +1,26 @@
(ns ^:mb/once metabase.http-client-test
(:require
[clojure.test :refer :all]
[metabase.http-client :as client]))
(deftest build-url-test
(binding [client/*url-prefix* "http://localhost:3000/api/"]
(testing "correctly encode all data types"
(is (= "http://localhost:3000/api/database/1?int=1&float=1.23&string=a&keyword=b&seq=1&seq=2&seq=3"
(client/build-url "database/1" {:int 1
:float 1.23
:string "a"
:keyword :b
:seq [1 2 3]}))))))
(deftest parse-http-client-args-test
(testing "parse correctly"
(is (= {:credentials "829a5f19-5a83-44af-af69-6c6ae4444167",
:expected-status 200,
:method :get,
:query-parameters {:include_ids [1 2], :query "metabase"},
:url "/card"}
(#'client/parse-http-client-args ["829a5f19-5a83-44af-af69-6c6ae4444167" :get 200 "/card"
:query "metabase"
:include_ids 1
:include_ids 2])))))

6
test/metabase/lib/types/isa_test.cljc
View File

@@ -90,10 +90,10 @@
(testing "strings, regardless of the effective type is"
(are [typ] (= ::lib.types.constants/string (lib.types.isa/field-type {:effective-type :type/Float
:semantic-type typ}))
:type/Name :type/Category)))
:type/Name :type/Category))
(testing "boolean, regardless of the semantic type"
(is (= ::lib.types.constants/boolean (lib.types.isa/field-type {:effective-type :type/Boolean
:semantic-type :type/Category}))))
:semantic-type :type/Category}))))
(testing "unexpected things"
(are [column] (nil? (lib.types.isa/field-type column))
{:effective-type "DERP DERP DERP"}
@@ -102,7 +102,7 @@
{:semantic-type nil}
"DERP DERP DERP"
:type/Category
nil))
nil)))
(deftest ^:parallel type-predicate-test
(letfn [(column [x]