diff --git a/e2e/test/scenarios/visualizations/object_detail.cy.spec.js b/e2e/test/scenarios/visualizations/object_detail.cy.spec.js index 6b5ce217f5e9a2f51cf1369024f59868f5ab731a..4a3511fc685fa872e88b0bf6e89fac6c0e686165 100644 --- a/e2e/test/scenarios/visualizations/object_detail.cy.spec.js +++ b/e2e/test/scenarios/visualizations/object_detail.cy.spec.js @@ -35,6 +35,12 @@ const TEST_QUESTION = { }, }; +const TEST_PEOPLE_QUESTION = { + query: { + "source-table": PEOPLE_ID, + }, +}; + describe("scenarios > question > object details", () => { beforeEach(() => { restore(); @@ -119,7 +125,25 @@ describe("scenarios > question > object details", () => { cy.createQuestion(TEST_QUESTION).then(({ body: { id } }) => { cy.visit(`/question/${id}/${FILTERED_OUT_ID}`); cy.wait("@cardQuery"); - cy.findByText("The page you asked for couldn't be found."); + cy.findByRole("dialog").within(() => { + cy.findByText(/We're a little lost/i); + }); + }); + }); + + it("can view details of an out-of-range record", () => { + cy.intercept("POST", "/api/card/*/query").as("cardQuery"); + // since we only fetch 2000 rows, this ID is out of range + // and has to be fetched separately + const OUT_OF_RANGE_ID = 2150; + + cy.createQuestion(TEST_PEOPLE_QUESTION).then(({ body: { id } }) => { + cy.visit(`/question/${id}/${OUT_OF_RANGE_ID}`); + cy.wait("@cardQuery"); + cy.findByTestId("object-detail").within(() => { + // should appear in header and body of the modal + cy.findAllByText(/Marcelina Kuhn/i).should("have.length", 2); + }); }); }); @@ -182,7 +206,9 @@ describe("scenarios > question > object details", () => { openProductsTable({ limit: 5 }); - cy.findByTextEnsureVisible("Rustic Paper Wallet").click(); + cy.findByTestId("TableInteractive-root") + .findByTextEnsureVisible("Rustic Paper Wallet") + .click(); cy.location("search").should("eq", "?objectId=Rustic%20Paper%20Wallet"); cy.findByTestId("object-detail").contains("Rustic Paper Wallet"); @@ -276,56 +302,64 @@ function changeSorting(columnName, direction) { cy.wait("@dataset"); } -['postgres', 'mysql'].forEach(dialect => { - describe(`Object Detail > composite keys (${dialect})`, { tags: ['@external'] }, () => { - const TEST_TABLE = "composite_pk_table"; - - beforeEach(() => { - resetTestTable({ type: dialect, table: TEST_TABLE }); - restore(`${dialect}-writable`); - cy.signInAsAdmin(); - resyncDatabase({ dbId: WRITABLE_DB_ID, tableName: TEST_TABLE }); - }); - - it('can show object detail modal for items with composite keys', () => { - getTableId({ name: TEST_TABLE }).then(tableId => { - cy.visit(`/question#?db=${WRITABLE_DB_ID}&table=${tableId}`); +["postgres", "mysql"].forEach(dialect => { + describe( + `Object Detail > composite keys (${dialect})`, + { tags: ["@external"] }, + () => { + const TEST_TABLE = "composite_pk_table"; + + beforeEach(() => { + resetTestTable({ type: dialect, table: TEST_TABLE }); + restore(`${dialect}-writable`); + cy.signInAsAdmin(); + resyncDatabase({ dbId: WRITABLE_DB_ID, tableName: TEST_TABLE }); }); - cy.icon('expand').first().click(); + it("can show object detail modal for items with composite keys", () => { + getTableId({ name: TEST_TABLE }).then(tableId => { + cy.visit(`/question#?db=${WRITABLE_DB_ID}&table=${tableId}`); + }); - cy.findByRole('dialog').within(() => { - cy.findAllByText("Duck").should('have.length', 2); - cy.icon('chevrondown').click(); - cy.findAllByText("Horse").should('have.length', 2); - }); - }); - }); - - describe(`Object Detail > no primary keys (${dialect})`, { tags: ['@external'] }, () => { - const TEST_TABLE = "no_pk_table"; + cy.icon("expand").first().click(); - beforeEach(() => { - resetTestTable({ type: dialect, table: TEST_TABLE }); - restore(`${dialect}-writable`); - cy.signInAsAdmin(); - resyncDatabase({ dbId: WRITABLE_DB_ID, tableName: TEST_TABLE }); - }); - - it('can show object detail modal for items with no primary key', () => { - getTableId({ name: TEST_TABLE }).then(tableId => { - cy.visit(`/question#?db=${WRITABLE_DB_ID}&table=${tableId}`); + cy.findByRole("dialog").within(() => { + cy.findAllByText("Duck").should("have.length", 2); + cy.icon("chevrondown").click(); + cy.findAllByText("Horse").should("have.length", 2); + }); }); + }, + ); + + describe( + `Object Detail > no primary keys (${dialect})`, + { tags: ["@external"] }, + () => { + const TEST_TABLE = "no_pk_table"; + + beforeEach(() => { + resetTestTable({ type: dialect, table: TEST_TABLE }); + restore(`${dialect}-writable`); + cy.signInAsAdmin(); + resyncDatabase({ dbId: WRITABLE_DB_ID, tableName: TEST_TABLE }); + }); + + it("can show object detail modal for items with no primary key", () => { + getTableId({ name: TEST_TABLE }).then(tableId => { + cy.visit(`/question#?db=${WRITABLE_DB_ID}&table=${tableId}`); + }); - cy.icon('expand').first().click(); + cy.icon("expand").first().click(); - cy.findByRole('dialog').within(() => { - cy.findAllByText("Duck").should('have.length', 2); - cy.icon('chevrondown').click(); - cy.findAllByText("Horse").should('have.length', 2); + cy.findByRole("dialog").within(() => { + cy.findAllByText("Duck").should("have.length", 2); + cy.icon("chevrondown").click(); + cy.findAllByText("Horse").should("have.length", 2); + }); }); - }); - }); + }, + ); }); describe(`Object Detail > public`, () => { @@ -334,40 +368,40 @@ describe(`Object Detail > public`, () => { cy.signInAsAdmin(); }); - it('can view a public object detail question', () => { - - cy.createQuestion({ ...TEST_QUESTION, display: 'object' }).then( + it("can view a public object detail question", () => { + cy.createQuestion({ ...TEST_QUESTION, display: "object" }).then( ({ body: { id: questionId } }) => { visitPublicQuestion(questionId); }, ); - cy.icon('warning').should('not.exist'); + cy.icon("warning").should("not.exist"); - cy.findByTestId('object-detail').within(() => { - cy.findByText('User ID').should('be.visible'); - cy.findByText('1283').should('be.visible'); + cy.findByTestId("object-detail").within(() => { + cy.findByText("User ID").should("be.visible"); + cy.findByText("1283").should("be.visible"); }); - cy.findByTestId('pagination-footer').within(() => { - cy.findByText("Item 1 of 3").should('be.visible'); + cy.findByTestId("pagination-footer").within(() => { + cy.findByText("Item 1 of 3").should("be.visible"); }); }); - it('can view an object detail question on a public dashboard', () => { - cy.createQuestionAndDashboard({ questionDetails: { ...TEST_QUESTION, display: 'object' } }).then( - ({ body: { dashboard_id } }) => { - visitPublicDashboard(dashboard_id); - }); + it("can view an object detail question on a public dashboard", () => { + cy.createQuestionAndDashboard({ + questionDetails: { ...TEST_QUESTION, display: "object" }, + }).then(({ body: { dashboard_id } }) => { + visitPublicDashboard(dashboard_id); + }); - cy.icon('warning').should('not.exist'); + cy.icon("warning").should("not.exist"); - cy.findByTestId('object-detail').within(() => { - cy.findByText('User ID').should('be.visible'); - cy.findByText('1283').should('be.visible'); + cy.findByTestId("object-detail").within(() => { + cy.findByText("User ID").should("be.visible"); + cy.findByText("1283").should("be.visible"); }); - cy.findByTestId('pagination-footer').within(() => { - cy.findByText("Item 1 of 3").should('be.visible'); + cy.findByTestId("pagination-footer").within(() => { + cy.findByText("Item 1 of 3").should("be.visible"); }); }); }); diff --git a/frontend/src/metabase/components/ErrorDetails/types.ts b/frontend/src/metabase/components/ErrorDetails/types.ts index 3f0133559f63dbd99370a1f62ff94d1f38297a47..a0afba6c21a20960cacb60796167b4e31861c18c 100644 --- a/frontend/src/metabase/components/ErrorDetails/types.ts +++ b/frontend/src/metabase/components/ErrorDetails/types.ts @@ -1,5 +1,5 @@ export interface ErrorDetailsProps { - details: string | Record<string, any>; + details?: string | Record<string, any>; centered?: boolean; className?: string; } diff --git a/frontend/src/metabase/containers/ErrorPages.jsx b/frontend/src/metabase/containers/ErrorPages.tsx similarity index 77% rename from frontend/src/metabase/containers/ErrorPages.jsx rename to frontend/src/metabase/containers/ErrorPages.tsx index 408f9fdc698468df58c8de3c5a9dd8e7e0add7d9..f140240d1d0d88658fe262fbe7194578fa2e26c9 100644 --- a/frontend/src/metabase/containers/ErrorPages.jsx +++ b/frontend/src/metabase/containers/ErrorPages.tsx @@ -3,6 +3,7 @@ import React from "react"; import { t } from "ttag"; import { color } from "metabase/lib/colors"; +import type { ErrorDetailsProps } from "metabase/components/ErrorDetails/types"; import Icon from "metabase/components/Icon"; import EmptyState from "metabase/components/EmptyState"; import ErrorDetails from "metabase/components/ErrorDetails/ErrorDetails"; @@ -14,6 +15,10 @@ export const GenericError = ({ title = t`Something's gone wrong`, message = t`We've run into an error. You can try refreshing the page, or just go back.`, details, +}: { + title?: string; + message?: string; + details: ErrorDetailsProps["details"]; }) => ( <ErrorPageRoot> <EmptyState @@ -27,12 +32,18 @@ export const GenericError = ({ </ErrorPageRoot> ); -export const NotFound = () => ( +export const NotFound = ({ + title = t`We're a little lost...`, + message = t`The page you asked for couldn't be found.`, +}: { + title?: string; + message?: string; +}) => ( <ErrorPageRoot> <EmptyState illustrationElement={<img src={NoResults} />} - title={t`We're a little lost...`} - message={t`The page you asked for couldn't be found.`} + title={title} + message={message} /> </ErrorPageRoot> ); @@ -46,7 +57,13 @@ export const Unauthorized = () => ( </ErrorPageRoot> ); -export const Archived = ({ entityName, linkTo }) => ( +export const Archived = ({ + entityName, + linkTo, +}: { + entityName: string; + linkTo: string; +}) => ( <ErrorPageRoot> <EmptyState title={t`This ${entityName} has been archived`} diff --git a/frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetail.styled.tsx b/frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetail.styled.tsx index 7ef0e2ebbe4ce348e62d9f0210f1e2b8345baf29..dfc72f59e8bf3e03124ee30cb1bb6907cd482da2 100644 --- a/frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetail.styled.tsx +++ b/frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetail.styled.tsx @@ -61,6 +61,9 @@ export const CloseButton = styled.div` export const ErrorWrapper = styled.div` height: 480px; + display: flex; + justify-content: center; + align-items: center; `; type GridContainerProps = { cols?: number }; diff --git a/frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetail.tsx b/frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetail.tsx index 70fc6685bf1fb140be37bbc59eb5c62cd201fb7b..af5378b74b88c5db409b8db8de6f273291ea998f 100644 --- a/frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetail.tsx +++ b/frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetail.tsx @@ -1,6 +1,7 @@ -import React, { useState, useEffect, useCallback } from "react"; +import React, { useState, useEffect, useCallback, useMemo } from "react"; import { connect } from "react-redux"; import _ from "underscore"; +import { t } from "ttag"; import { useMount, usePrevious } from "react-use"; import { State } from "metabase-types/store"; @@ -13,6 +14,7 @@ import { DatasetData } from "metabase-types/types/Dataset"; import Button from "metabase/core/components/Button"; import { NotFound } from "metabase/containers/ErrorPages"; +import LoadingSpinner from "metabase/components/LoadingSpinner"; import Tables from "metabase/entities/tables"; import { @@ -34,6 +36,7 @@ import { } from "metabase/query_builder/selectors"; import { getUser } from "metabase/selectors/user"; +import { MetabaseApi } from "metabase/services"; import { isVirtualCardId } from "metabase-lib/metadata/utils/saved-questions"; import { isPK } from "metabase-lib/types/utils/isa"; import type { @@ -47,6 +50,7 @@ import { getDisplayId, getIdValue, getSingleResultsRow, + getSinglePKIndex, } from "./utils"; import { DetailsTable } from "./ObjectDetailsTable"; import { Relationships } from "./ObjectRelationships"; @@ -115,11 +119,11 @@ const mapDispatchToProps = (dispatch: any) => ({ closeObjectDetail: () => dispatch(closeObjectDetail()), }); -export function ObjectDetailFn({ - data, +export function ObjectDetailView({ + data: passedData, question, table, - zoomedRow, + zoomedRow: passedZoomedRow, zoomedRowID, tableForeignKeys, tableForeignKeyReferences, @@ -141,9 +145,25 @@ export function ObjectDetailFn({ className, }: ObjectDetailProps): JSX.Element | null { const [hasNotFoundError, setHasNotFoundError] = useState(false); + const [maybeLoading, setMaybeLoading] = useState(false); const prevZoomedRowId = usePrevious(zoomedRowID); - const prevData = usePrevious(data); + const prevData = usePrevious(passedData); const prevTableForeignKeys = usePrevious(tableForeignKeys); + const [data, setData] = useState<DatasetData>(passedData); + + const pkIndex = useMemo( + () => getSinglePKIndex(passedData?.cols), + [passedData], + ); + + const zoomedRow = useMemo( + () => + passedZoomedRow || + (pkIndex !== undefined && + data.rows.find(row => row[pkIndex] === zoomedRowID)) || + undefined, + [passedZoomedRow, pkIndex, data, zoomedRowID], + ); const loadFKReferences = useCallback(() => { if (zoomedRowID) { @@ -154,6 +174,7 @@ export function ObjectDetailFn({ useMount(() => { const notFoundObject = zoomedRowID != null && !zoomedRow; if (data && notFoundObject) { + setMaybeLoading(true); setHasNotFoundError(true); return; } @@ -172,6 +193,31 @@ export function ObjectDetailFn({ }; }); + useEffect(() => { + if (maybeLoading && pkIndex !== undefined) { + // if we don't have the row in the current data, try to fetch this single row + const pkField = passedData.cols[pkIndex]; + const filteredQuestion = question?.filter("=", pkField, zoomedRowID); + MetabaseApi.dataset(filteredQuestion?._card.dataset_query) + .then(result => { + if (result?.data?.rows?.length > 0) { + const newRow = result.data.rows[0]; + setData(prevData => ({ + ...prevData, + rows: [newRow, ...prevData.rows], + })); + setHasNotFoundError(false); + } + }) + .catch(() => { + setHasNotFoundError(true); + }) + .finally(() => { + setMaybeLoading(false); + }); + } + }, [maybeLoading, passedData, question, zoomedRowID, pkIndex]); + useEffect(() => { if (tableForeignKeys && prevZoomedRowId !== zoomedRowID) { loadFKReferences(); @@ -249,9 +295,13 @@ export function ObjectDetailFn({ return ( <ObjectDetailContainer wide={hasRelationships} className={className}> - {hasNotFoundError ? ( + {maybeLoading ? ( + <ErrorWrapper> + <LoadingSpinner /> + </ErrorWrapper> + ) : hasNotFoundError ? ( <ErrorWrapper> - <NotFound /> + <NotFound message={t`We couldn't find that record`} /> </ErrorWrapper> ) : ( <ObjectDetailWrapperDiv @@ -314,7 +364,7 @@ export function ObjectDetailWrapper({ onClose={closeObjectDetail} className={""} // need an empty className to override the Modal default width > - <ObjectDetailFn + <ObjectDetailView {...props} showHeader data={data} @@ -329,7 +379,7 @@ export function ObjectDetailWrapper({ return ( <> - <ObjectDetailFn + <ObjectDetailView {...props} zoomedRow={data.rows[currentObjectIndex]} data={data} diff --git a/frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetail.unit.spec.tsx b/frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetail.unit.spec.tsx index bcd50db05fff2ac2f1e451f7a394e89065d965be..0eb6079a158a6dbdfd43778602f33615be53ca11 100644 --- a/frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetail.unit.spec.tsx +++ b/frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetail.unit.spec.tsx @@ -2,12 +2,57 @@ import React from "react"; import { render, screen } from "@testing-library/react"; import testDataset from "__support__/testDataset"; +import { setupCardDataset } from "__support__/server-mocks"; +import { createMockCard } from "metabase-types/api/mocks"; +import Question from "metabase-lib/Question"; + import { - ObjectDetailFn as ObjectDetail, + ObjectDetailView, ObjectDetailHeader, ObjectDetailBody, ObjectDetailWrapper, } from "./ObjectDetail"; +import type { ObjectDetailProps } from "./types"; + +function setup(options?: Partial<ObjectDetailProps>) { + render( + <ObjectDetailView + data={testDataset as any} + question={ + new Question( + createMockCard({ + name: "Product", + }), + ) + } + table={ + { + objectName: () => "Product", + } as any + } + zoomedRow={testDataset.rows[0]} + zoomedRowID={0} + tableForeignKeys={[]} + tableForeignKeyReferences={[]} + settings={{ + column: () => null, + }} + showHeader + canZoom={true} + canZoomPreviousRow={false} + canZoomNextRow={false} + followForeignKey={() => null} + onVisualizationClick={() => null} + visualizationIsClickable={() => false} + fetchTableFks={() => null} + loadObjectDetailFKReferences={() => null} + viewPreviousObjectDetail={() => null} + viewNextObjectDetail={() => null} + closeObjectDetail={() => null} + {...options} + />, + ); +} describe("Object Detail", () => { it("renders an object detail header", () => { @@ -75,43 +120,7 @@ describe("Object Detail", () => { }); it("renders an object detail component", () => { - render( - <ObjectDetail - data={testDataset as any} - question={ - { - displayName: () => "Product", - database: () => ({ - getPlainObject: () => ({}), - }), - } as any - } - table={ - { - objectName: () => "Product", - } as any - } - zoomedRow={testDataset.rows[0]} - zoomedRowID={0} - tableForeignKeys={[]} - tableForeignKeyReferences={[]} - settings={{ - column: () => null, - }} - showHeader - canZoom={true} - canZoomPreviousRow={false} - canZoomNextRow={false} - followForeignKey={() => null} - onVisualizationClick={() => null} - visualizationIsClickable={() => false} - fetchTableFks={() => null} - loadObjectDetailFKReferences={() => null} - viewPreviousObjectDetail={() => null} - viewNextObjectDetail={() => null} - closeObjectDetail={() => null} - />, - ); + setup(); expect(screen.getByText(/Product/i)).toBeInTheDocument(); expect( @@ -250,4 +259,41 @@ describe("Object Detail", () => { expect(screen.queryByText(/Product/i)).not.toBeInTheDocument(); }); + + it("fetches a missing row", async () => { + setupCardDataset({ + data: { + rows: [ + [ + "101", + "1807963902339", + "Extremely Hungry Toucan", + "Gizmo", + "Larson, Pfeffer and Klocko", + 31.78621880685793, + 4.3, + "2017-01-09T09:51:20.352-07:00", + ], + ], + }, + }); + + // because this row is not in the test dataset, it should trigger a fetch + setup({ zoomedRowID: "101", zoomedRow: undefined }); + + expect(screen.getByTestId("loading-spinner")).toBeInTheDocument(); + expect( + await screen.findByText(/Extremely Hungry Toucan/i), + ).toBeInTheDocument(); + }); + + it("shows not found if it can't find a missing row", async () => { + setupCardDataset({ data: { rows: [] } }); + + // because this row is not in the test dataset, it should trigger a fetch + setup({ zoomedRowID: "102", zoomedRow: undefined }); + + expect(screen.getByTestId("loading-spinner")).toBeInTheDocument(); + expect(await screen.findByText(/we're a little lost/i)).toBeInTheDocument(); + }); }); diff --git a/frontend/src/metabase/visualizations/components/ObjectDetail/utils.ts b/frontend/src/metabase/visualizations/components/ObjectDetail/utils.ts index f124ac1b6d3338f15622d18aa0ead1e6daad7e85..ca537d453e58cdf87ab5bf2bd825db848bf6bd94 100644 --- a/frontend/src/metabase/visualizations/components/ObjectDetail/utils.ts +++ b/frontend/src/metabase/visualizations/components/ObjectDetail/utils.ts @@ -8,6 +8,7 @@ import type { TableId, VisualizationSettings } from "metabase-types/api"; import { getIsPKFromTablePredicate, isEntityName, + isPK, } from "metabase-lib/types/utils/isa"; import Question from "metabase-lib/Question"; import Table from "metabase-lib/metadata/Table"; @@ -112,3 +113,13 @@ export const getIdValue = ({ export function getSingleResultsRow(data: DatasetData) { return data.rows.length === 1 ? data.rows[0] : null; } + +export const getSinglePKIndex = (cols: Column[]) => { + const pkCount = cols?.filter(isPK)?.length; + if (pkCount !== 1) { + return undefined; + } + const index = cols?.findIndex(isPK); + + return index === -1 ? undefined : index; +}; diff --git a/frontend/src/metabase/visualizations/components/ObjectDetail/utils.unit.spec.ts b/frontend/src/metabase/visualizations/components/ObjectDetail/utils.unit.spec.ts index d86d08af2a51c09f809687c1838efa6b49cbf79f..2b3128ad68da02eb82b7a923113e13138dec41be 100644 --- a/frontend/src/metabase/visualizations/components/ObjectDetail/utils.unit.spec.ts +++ b/frontend/src/metabase/visualizations/components/ObjectDetail/utils.unit.spec.ts @@ -9,7 +9,12 @@ import type { DatetimeUnit } from "metabase-types/api"; import { Card } from "metabase-types/types/Card"; import Question from "metabase-lib/Question"; -import { getObjectName, getDisplayId, getIdValue } from "./utils"; +import { + getObjectName, + getDisplayId, + getIdValue, + getSinglePKIndex, +} from "./utils"; const card = { id: 1, @@ -220,4 +225,22 @@ describe("ObjectDetail utils", () => { expect(id).toBe(22); }); }); + + describe("getSinglePKIndex", () => { + it("should return the index of the single PK column", () => { + expect(getSinglePKIndex([idCol, qtyCol, nameCol])).toBe(0); + expect(getSinglePKIndex([qtyCol, idCol, nameCol])).toBe(1); + expect(getSinglePKIndex([qtyCol, nameCol, idCol])).toBe(2); + }); + + it("should return undefined if there are multiple PKs", () => { + expect(getSinglePKIndex([idCol, productIdCol, qtyCol, nameCol])).toBe( + undefined, + ); + }); + + it("should return undefined if there are no PKs", () => { + expect(getSinglePKIndex([qtyCol, nameCol])).toBe(undefined); + }); + }); });