Skip to content
Snippets Groups Projects
Unverified Commit 2af32bbc authored by Oleg Gromov's avatar Oleg Gromov Committed by GitHub
Browse files

Fix card filter connected to a field not causing an update (#36206) 

* Rename DashboardHeaderView to match filename
* Fix non updating cards on filter mapping to a field
* Clean up DashboardApp tests a bit
* Clean up Dashboard.jsx cruft
* Add Dashboard componentDidUpdate "unit tests"
* Simplify setup code as per feedback
* Add test for changing dashboard prop in componentDidMount
* Change constant names
* Fix optional chaining in Dashboard
parent 2aae0cdf
Branches
Tags
No related merge requests found
Hide whitespace changes
Inline Side-by-side
Showing
with 274 additions and 136 deletions
frontend/src/metabase/dashboard/components/Dashboard/Dashboard.jsx
+ 70
− 105
View file @ 2af32bbc
......@@ -42,10 +42,6 @@ class DashboardInner extends Component {
isSharing: false,
};
constructor(props) {
super(props);
}
static getDerivedStateFromProps({ parameters }, { parametersListLength }) {
const visibleParameters = getVisibleParameters(parameters);
return visibleParameters.length !== parametersListLength
......@@ -54,14 +50,14 @@ class DashboardInner extends Component {
}
async componentDidMount() {
await this.loadDashboard(this.props.dashboardId);
await this.loadDashboard();
}
async componentDidUpdate(prevProps) {
updateParametersWidgetStickiness(this);
if (prevProps.dashboardId !== this.props.dashboardId) {
await this.loadDashboard(this.props.dashboardId);
await this.loadDashboard();
return;
}
......@@ -73,6 +69,7 @@ class DashboardInner extends Component {
if (
!_.isEqual(prevProps.parameterValues, this.props.parameterValues) ||
!_.isEqual(prevProps.parameters, this.props.parameters) ||
(!prevProps.dashboard && this.props.dashboard)
) {
this.props.fetchDashboardCardData({ reload: false, clearCache: true });
......@@ -83,51 +80,39 @@ class DashboardInner extends Component {
this.props.cancelFetchDashboardCardData();
}
async loadDashboard(dashboardId) {
const {
editingOnLoad,
addCardOnLoad,
addCardToDashboard,
fetchDashboard,
initialize,
loadDashboardParams,
location,
setErrorPage,
isNavigatingBackToDashboard,
} = this.props;
initialize({ clearCache: !isNavigatingBackToDashboard });
loadDashboardParams();
const result = await fetchDashboard({
dashId: dashboardId,
queryParams: location.query,
async loadDashboard() {
const p = this.props;
p.initialize({ clearCache: !p.isNavigatingBackToDashboard });
p.loadDashboardParams();
const result = await p.fetchDashboard({
dashId: p.dashboardId,
queryParams: p.location.query,
options: {
clearCache: !isNavigatingBackToDashboard,
preserveParameters: isNavigatingBackToDashboard,
clearCache: !p.isNavigatingBackToDashboard,
preserveParameters: p.isNavigatingBackToDashboard,
},
});
if (result.error) {
setErrorPage(result.payload);
p.setErrorPage(result.payload);
return;
}
try {
if (editingOnLoad) {
if (p.editingOnLoad) {
this.setEditing(this.props.dashboard);
}
if (addCardOnLoad != null) {
addCardToDashboard({
dashId: dashboardId,
cardId: addCardOnLoad,
tabId: this.props.dashboard.tabs[0]?.id ?? null,
if (p.addCardOnLoad != null) {
p.addCardToDashboard({
dashId: p.dashboardId,
cardId: p.addCardOnLoad,
tabId: p.dashboard?.tabs[0]?.id ?? null,
});
}
} catch (error) {
if (error.status === 404) {
setErrorPage({ ...error, context: "dashboard" });
p.setErrorPage({ ...error, context: "dashboard" });
} else {
console.error(error);
this.setState({ error });
......@@ -156,133 +141,113 @@ class DashboardInner extends Component {
};
onAddQuestion = () => {
const { dashboard } = this.props;
this.setEditing(dashboard);
this.setEditing(this.props.dashboard);
this.props.toggleSidebar(SIDEBAR_NAME.addQuestion);
};
renderContent = () => {
const { dashboard, selectedTabId, isNightMode, isFullscreen } = this.props;
const canWrite = dashboard?.can_write ?? false;
const dashboardHasCards = dashboard?.dashcards.length > 0 ?? false;
shouldRenderAsNightMode() {
return this.props.isNightMode && this.props.isFullscreen;
}
renderContent = () => {
const p = this.props;
const canWrite = p.dashboard?.can_write ?? false;
const dashboardHasCards = p.dashboard?.dashcards.length > 0 ?? false;
const tabHasCards =
dashboard?.dashcards.filter(
p.dashboard?.dashcards.filter(
c =>
selectedTabId !== undefined && c.dashboard_tab_id === selectedTabId,
p.selectedTabId !== undefined &&
c.dashboard_tab_id === p.selectedTabId,
).length > 0 ?? false;
const shouldRenderAsNightMode = isNightMode && isFullscreen;
if (!dashboardHasCards && !canWrite) {
return (
<DashboardEmptyStateWithoutAddPrompt
isNightMode={shouldRenderAsNightMode}
isNightMode={this.shouldRenderAsNightMode()}
/>
);
}
if (!dashboardHasCards) {
return (
<DashboardEmptyState
dashboard={dashboard}
isNightMode={shouldRenderAsNightMode}
dashboard={p.dashboard}
isNightMode={this.shouldRenderAsNightMode()}
addQuestion={this.onAddQuestion}
closeNavbar={this.props.closeNavbar}
closeNavbar={p.closeNavbar}
/>
);
}
if (dashboardHasCards && !tabHasCards) {
return (
<DashboardEmptyStateWithoutAddPrompt
isNightMode={shouldRenderAsNightMode}
isNightMode={this.shouldRenderAsNightMode()}
/>
);
}
return (
<DashboardGridConnected
{...this.props}
dashboard={this.props.dashboard}
isNightMode={shouldRenderAsNightMode}
dashboard={p.dashboard}
isNightMode={this.shouldRenderAsNightMode()}
onEditingChange={this.setEditing}
/>
);
};
render() {
const {
addParameter,
dashboard,
isEditing,
isEditingParameter,
isFullscreen,
isNightMode,
isSharing,
parameters,
parameterValues,
draftParameterValues,
editingParameter,
setParameterValue,
setParameterIndex,
setEditingParameter,
isHeaderVisible,
isAutoApplyFilters,
} = this.props;
const { error, isParametersWidgetSticky } = this.state;
const shouldRenderAsNightMode = isNightMode && isFullscreen;
const visibleParameters = getVisibleParameters(parameters);
const p = this.props;
const visibleParameters = getVisibleParameters(p.parameters);
const parametersWidget = (
<SyncedParametersList
parameters={getValuePopulatedParameters(
parameters,
isAutoApplyFilters ? parameterValues : draftParameterValues,
p.parameters,
p.isAutoApplyFilters ? p.parameterValues : p.draftParameterValues,
)}
editingParameter={editingParameter}
dashboard={dashboard}
isFullscreen={isFullscreen}
isNightMode={shouldRenderAsNightMode}
isEditing={isEditing}
setParameterValue={setParameterValue}
setParameterIndex={setParameterIndex}
setEditingParameter={setEditingParameter}
editingParameter={p.editingParameter}
dashboard={p.dashboard}
isFullscreen={p.isFullscreen}
isNightMode={this.shouldRenderAsNightMode()}
isEditing={p.isEditing}
setParameterValue={p.setParameterValue}
setParameterIndex={p.setParameterIndex}
setEditingParameter={p.setEditingParameter}
/>
);
const shouldRenderParametersWidgetInViewMode =
!isEditing && !isFullscreen && visibleParameters.length > 0;
!p.isEditing && !p.isFullscreen && visibleParameters.length > 0;
const shouldRenderParametersWidgetInEditMode =
isEditing && visibleParameters.length > 0;
p.isEditing && visibleParameters.length > 0;
const cardsContainerShouldHaveMarginTop =
!shouldRenderParametersWidgetInViewMode &&
(!isEditing || isEditingParameter);
(!p.isEditing || p.isEditingParameter);
return (
<DashboardLoadingAndErrorWrapper
isFullHeight={isEditing || isSharing}
isFullscreen={isFullscreen}
isNightMode={shouldRenderAsNightMode}
loading={!dashboard}
error={error}
isFullHeight={p.isEditing || p.isSharing}
isFullscreen={p.isFullscreen}
isNightMode={this.shouldRenderAsNightMode()}
loading={!p.dashboard}
error={this.state.error}
>
{() => (
<DashboardStyled>
{isHeaderVisible && (
{p.isHeaderVisible && (
<DashboardHeaderContainer
isFullscreen={isFullscreen}
isNightMode={shouldRenderAsNightMode}
isFullscreen={p.isFullscreen}
isNightMode={this.shouldRenderAsNightMode()}
>
<DashboardHeader
{...this.props}
onEditingChange={this.setEditing}
setDashboardAttribute={this.setDashboardAttribute}
addParameter={addParameter}
addParameter={p.addParameter}
parametersWidget={parametersWidget}
onSharingClick={this.onSharingClick}
/>
......@@ -290,7 +255,7 @@ class DashboardInner extends Component {
{shouldRenderParametersWidgetInEditMode && (
<ParametersWidgetContainer
data-testid="edit-dashboard-parameters-widget-container"
isEditing={isEditing}
isEditing={p.isEditing}
>
{parametersWidget}
</ParametersWidgetContainer>
......@@ -298,17 +263,17 @@ class DashboardInner extends Component {
</DashboardHeaderContainer>
)}
<DashboardBody isEditingOrSharing={isEditing || isSharing}>
<DashboardBody isEditingOrSharing={p.isEditing || p.isSharing}>
<ParametersAndCardsContainer
data-testid="dashboard-parameters-and-cards"
shouldMakeDashboardHeaderStickyAfterScrolling={
!isFullscreen && (isEditing || isSharing)
!p.isFullscreen && (p.isEditing || p.isSharing)
}
>
{shouldRenderParametersWidgetInViewMode && (
<ParametersWidgetContainer
data-testid="dashboard-parameters-widget-container"
isSticky={isParametersWidgetSticky}
isSticky={this.state.isParametersWidgetSticky}
>
{parametersWidget}
<FilterApplyButton />
......
frontend/src/metabase/dashboard/components/Dashboard/Dashboard.unit.spec.tsx 0 → 100644
+ 183
− 0
View file @ 2af32bbc
import { renderWithProviders, waitForLoaderToBeRemoved } from "__support__/ui";
import {
createMockActionDashboardCard as _createMockActionDashboardCard,
createMockActionParameter,
createMockDashboard,
} from "metabase-types/api/mocks";
import type { Dashboard, Parameter } from "metabase-types/api";
import { Dashboard as DashboardComponent } from "./Dashboard";
type SetupOpts = {
dashboardId: number | null;
dashboard: Dashboard | null;
selectedTabId: number | null; // when there's no tabs, is null
parameters: Parameter[]; // when empty, is an empty array
parameterValues?: Record<string, any>; // when empty, is undefined
skipLoader?: boolean;
};
async function setup(overrides: Partial<SetupOpts> = {}) {
const mockDashboard = createMockDashboard({ id: 10 }); // value is irrelevant
const opts: SetupOpts = {
dashboardId: 10,
dashboard: mockDashboard,
selectedTabId: null,
parameters: [],
parameterValues: undefined,
...overrides,
};
const mockLoadDashboardParams = jest.fn();
const mockFetchDashboard = jest.fn(() => mockDashboard);
const mockFetchDashboardCardData = jest.fn();
const mockFetchDashboardCardMetadata = jest.fn();
function TestComponent(props: SetupOpts) {
return (
<DashboardComponent
dashboard={props.dashboard}
dashboardId={props.dashboardId}
parameters={props.parameters}
parameterValues={props.parameterValues}
loadDashboardParams={mockLoadDashboardParams}
fetchDashboard={mockFetchDashboard}
fetchDashboardCardData={mockFetchDashboardCardData}
fetchDashboardCardMetadata={mockFetchDashboardCardMetadata}
// stuff below doesn't change
location={global.location}
isAdmin={false}
isFullscreen={false}
isNightMode={false}
isSharing={false}
isEditing={false}
isEditingParameter={false}
isNavbarOpen={false}
isHeaderVisible={false}
isAdditionalInfoVisible={false}
isNavigatingBackToDashboard={false}
editingOnLoad={false}
addCardOnLoad={null}
isAutoApplyFilters={false} // TODO true or false?
dashcardData={{}}
selectedTabId={props.selectedTabId}
draftParameterValues={{}}
editingParameter={{}}
sidebar={{ name: "any", props: {} }}
addCardToDashboard={jest.fn()}
addParameter={jest.fn()}
archiveDashboard={jest.fn()}
cancelFetchDashboardCardData={jest.fn()}
initialize={jest.fn()}
onRefreshPeriodChange={jest.fn()}
updateDashboardAndCards={jest.fn()}
setDashboardAttributes={jest.fn()}
setEditingDashboard={jest.fn()}
setErrorPage={jest.fn()}
setSharing={jest.fn()}
setParameterValue={jest.fn()}
setEditingParameter={jest.fn()}
setParameterIndex={jest.fn()}
onUpdateDashCardVisualizationSettings={jest.fn()}
onUpdateDashCardColumnSettings={jest.fn()}
onReplaceAllDashCardVisualizationSettings={jest.fn()}
onChangeLocation={jest.fn()}
onSharingClick={jest.fn()}
onEmbeddingClick={jest.fn()}
toggleSidebar={jest.fn()}
closeSidebar={jest.fn()}
closeNavbar={jest.fn()}
/>
);
}
const { rerender } = renderWithProviders(
<TestComponent
dashboard={opts.dashboard}
dashboardId={opts.dashboardId}
selectedTabId={opts.selectedTabId}
parameters={opts.parameters}
parameterValues={opts.parameterValues}
/>,
);
if (!opts.skipLoader) {
await waitForLoaderToBeRemoved();
}
return {
rerender: (overrides: Partial<SetupOpts> = {}) =>
rerender(<TestComponent {...{ ...opts, ...overrides }} />),
mockLoadDashboardParams,
mockFetchDashboard,
mockFetchDashboardCardData,
mockFetchDashboardCardMetadata,
};
}
describe("Dashboard data fetching", () => {
afterEach(() => jest.clearAllMocks());
it("should fetch dashboard on first load", async () => {
const mocks = await setup();
expect(mocks.mockFetchDashboard).toHaveBeenCalledTimes(1);
});
it("should not fetch anything on re-render", async () => {
const mocks = await setup();
jest.clearAllMocks();
mocks.rerender();
expect(mocks.mockFetchDashboard).toHaveBeenCalledTimes(0);
expect(mocks.mockFetchDashboardCardData).toHaveBeenCalledTimes(0);
expect(mocks.mockFetchDashboardCardMetadata).toHaveBeenCalledTimes(0);
});
it("should fetch dashboard on dashboard id change", async () => {
const mocks = await setup();
mocks.rerender({ dashboardId: 20 });
expect(mocks.mockFetchDashboard).toHaveBeenCalledTimes(2);
});
it("should fetch card data and metadata when tab id changes", async () => {
const mocks = await setup();
mocks.rerender({ selectedTabId: 1 });
expect(mocks.mockFetchDashboardCardData).toHaveBeenCalledTimes(1);
expect(mocks.mockFetchDashboardCardMetadata).toHaveBeenCalledTimes(1);
});
it("should fetch card data when parameters change", async () => {
const mocks = await setup();
jest.clearAllMocks();
mocks.rerender({
parameters: [createMockActionParameter()],
});
expect(mocks.mockFetchDashboardCardMetadata).toHaveBeenCalledTimes(0);
expect(mocks.mockFetchDashboardCardData).toHaveBeenCalledTimes(1);
});
it("should fetch card data when parameter properties change", async () => {
const mocks = await setup({
parameters: [createMockActionParameter()],
});
jest.clearAllMocks();
mocks.rerender({
parameters: [createMockActionParameter({ id: "another" })],
});
expect(mocks.mockFetchDashboardCardMetadata).toHaveBeenCalledTimes(0);
expect(mocks.mockFetchDashboardCardData).toHaveBeenCalledTimes(1);
});
it("should fetch card data when dashboard changes to non-empty", async () => {
const mocks = await setup({
dashboardId: null,
dashboard: null,
skipLoader: true,
});
expect(mocks.mockFetchDashboardCardData).toHaveBeenCalledTimes(0);
mocks.rerender({
dashboardId: null,
dashboard: createMockDashboard({ id: 20 }),
});
expect(mocks.mockFetchDashboardCardData).toHaveBeenCalledTimes(1);
});
});
frontend/src/metabase/dashboard/components/DashboardHeader/DashboardHeader.jsx
+ 2
− 2
View file @ 2af32bbc
......@@ -46,7 +46,7 @@ import Link from "metabase/core/components/Link/Link";
import Collections from "metabase/entities/collections/collections";
import { isInstanceAnalyticsCollection } from "metabase/collections/utils";
import { SIDEBAR_NAME } from "../../constants";
import { DashboardHeaderComponent } from "./DashboardHeaderView";
import { DashboardHeaderView } from "./DashboardHeaderView";
import {
DashboardHeaderButton,
DashboardHeaderActionDivider,
......@@ -565,7 +565,7 @@ class DashboardHeaderContainer extends Component {
return (
<>
<DashboardHeaderComponent
<DashboardHeaderView
headerClassName="wrapper"
objectType="dashboard"
analyticsContext="Dashboard"
......
frontend/src/metabase/dashboard/components/DashboardHeader/DashboardHeaderView.tsx
+ 1
− 1
View file @ 2af32bbc
......@@ -44,7 +44,7 @@ interface DashboardHeaderViewProps {
setDashboardAttribute: (prop: string, value: string) => null;
}
export function DashboardHeaderComponent({
export function DashboardHeaderView({
editingTitle = "",
editingSubtitle = "",
editingButtons = [],
......
frontend/src/metabase/dashboard/containers/DashboardApp/DashboardApp.unit.spec.tsx
+ 18
− 28
View file @ 2af32bbc
......@@ -34,21 +34,6 @@ import {
import { createMockEntitiesState } from "__support__/store";
import { callMockEvent } from "__support__/events";
const TEST_COLLECTION = createMockCollection();
const TEST_DATABASE_WITH_ACTIONS = createMockDatabase({
settings: { "database-enable-actions": true },
});
const TEST_COLLECTION_ITEM = createMockCollectionItem({
collection: TEST_COLLECTION,
model: "dataset",
});
const TEST_CARD = createMockCard();
const TEST_TABLE = createMockTable();
const TestHome = () => <div />;
interface Options {
......@@ -56,22 +41,33 @@ interface Options {
}
async function setup({ dashboard }: Options = {}) {
const testCollection = createMockCollection();
const testDatabaseWithActions = createMockDatabase({
settings: { "database-enable-actions": true },
});
const mockDashboard = createMockDashboard(dashboard);
const dashboardId = mockDashboard.id;
const channelData = { channels: {} };
fetchMock.get("path:/api/pulse/form_input", channelData);
setupDatabasesEndpoints([TEST_DATABASE_WITH_ACTIONS]);
setupDatabasesEndpoints([testDatabaseWithActions]);
setupDashboardEndpoints(mockDashboard);
setupCollectionsEndpoints({ collections: [] });
setupCollectionItemsEndpoint({
collection: TEST_COLLECTION,
collection: testCollection,
collectionItems: [],
});
setupSearchEndpoints([TEST_COLLECTION_ITEM]);
setupCardsEndpoints([TEST_CARD]);
setupTableEndpoints(TEST_TABLE);
setupSearchEndpoints([
createMockCollectionItem({
collection: testCollection,
model: "dataset",
}),
]);
setupCardsEndpoints([createMockCard()]);
setupTableEndpoints(createMockTable());
setupBookmarksEndpoints([]);
setupActionsEndpoints([]);
......@@ -99,7 +95,7 @@ async function setup({ dashboard }: Options = {}) {
storeInitialState: {
dashboard: createMockDashboardState(),
entities: createMockEntitiesState({
databases: [TEST_DATABASE_WITH_ACTIONS],
databases: [testDatabaseWithActions],
}),
},
},
......@@ -115,15 +111,9 @@ async function setup({ dashboard }: Options = {}) {
}
describe("DashboardApp", function () {
afterEach(() => {
jest.clearAllMocks();
});
afterEach(() => jest.clearAllMocks());
describe("beforeunload events", () => {
afterEach(() => {
jest.clearAllMocks();
});
it("should have a beforeunload event when the user tries to leave a dirty dashboard", async function () {
const { mockEventListener } = await setup();
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment