From 5dcf1735c7682971308dd4cebbf369480953f2ce Mon Sep 17 00:00:00 2001
From: Anton Kulyk <kuliks.anton@gmail.com>
Date: Fri, 11 Feb 2022 10:14:11 +0200
Subject: [PATCH] Fix object detail browsing (#20101)
* Group `Urls.question` args into object
* Use `Urls.serializedQuestion`
* Fix `tableRowsQuery` URL builder
* Add E2E tests
* Add routes with `/:objectId`
* Add `objectId` support to question URL builders
* Use `queryParams` from `location` in QB init
* Fix `useSyncedQueryString` drops other params
* Add `zoomInRow` action
* Add `resetRowZoom` action
* Add basic selectors
* Modify `getIsObjectDetail` selector
* Handle `objectId` during QB init
* Replace actions opening prev/next object detail
* Use zoomed row in object detail view
* Update `ObjectDetailDrill`
* Fix URLs update for detail views
* Handle browser "back" button in detail view
* Use question name as a fallback for object view title field
* Fix E2E tests
* Fix E2E test
* Fix routes order
* Fix opening a link to filtered out record
* Move tests for object details
* Fix regexp
* Fix link style
* Better routing handling
---
.../metabase/hooks/use-synced-query-string.ts | 16 +-
frontend/src/metabase/lib/card.js | 7 -
frontend/src/metabase/lib/urls.js | 21 ++-
.../components/drill/ObjectDetailDrill.jsx | 83 ++++++++--
.../src/metabase/query_builder/actions.js | 98 ++++++------
.../query_builder/containers/QueryBuilder.jsx | 6 +-
.../src/metabase/query_builder/reducers.js | 16 ++
.../src/metabase/query_builder/selectors.js | 101 +++++++++++-
frontend/src/metabase/query_builder/utils.js | 31 ++++
frontend/src/metabase/routes.jsx | 2 +
.../components/ChartClickActions.jsx | 13 +-
.../visualizations/ObjectDetail.jsx | 151 +++++++++++++-----
frontend/test/metabase/lib/urls.unit.spec.js | 31 ++++
.../drill/ObjectDetailDrill.unit.spec.js | 121 +++++++++++---
.../dashboard/dashboard-drill.cy.spec.js | 4 +-
.../scenarios/question/new.cy.spec.js | 14 +-
.../visualizations/object_detail.cy.spec.js | 131 ++++++++++++---
17 files changed, 658 insertions(+), 188 deletions(-)
diff --git a/frontend/src/metabase/hooks/use-synced-query-string.ts b/frontend/src/metabase/hooks/use-synced-query-string.ts
index c25aa1ce320..a6a47b70779 100644
--- a/frontend/src/metabase/hooks/use-synced-query-string.ts
+++ b/frontend/src/metabase/hooks/use-synced-query-string.ts
@@ -21,7 +21,21 @@ export function useSyncedQueryString(
}, [deps]);
}
+const QUERY_PARAMS_ALLOW_LIST = ["objectId"];
+
function buildSearchString(object: Record<string, any>) {
- const search = querystring.stringify(object);
+ const currentSearchParams = querystring.parse(
+ window.location.search.replace("?", ""),
+ );
+ const filteredSearchParams = Object.fromEntries(
+ Object.entries(currentSearchParams).filter(entry =>
+ QUERY_PARAMS_ALLOW_LIST.includes(entry[0]),
+ ),
+ );
+
+ const search = querystring.stringify({
+ ...filteredSearchParams,
+ ...object,
+ });
return search ? `?${search}` : "";
}
diff --git a/frontend/src/metabase/lib/card.js b/frontend/src/metabase/lib/card.js
index 4f6f2667ea4..7af2667ef8e 100644
--- a/frontend/src/metabase/lib/card.js
+++ b/frontend/src/metabase/lib/card.js
@@ -1,7 +1,6 @@
import _ from "underscore";
import * as Q_DEPRECATED from "metabase/lib/query";
import Utils from "metabase/lib/utils";
-import * as Urls from "metabase/lib/urls";
import { CardApi } from "metabase/services";
import { b64hash_to_utf8, utf8_to_b64url } from "metabase/lib/encoding";
@@ -62,12 +61,6 @@ export function deserializeCardFromUrl(serialized) {
return JSON.parse(b64hash_to_utf8(serialized));
}
-export function urlForCardState(state, dirty) {
- return Urls.question(state.card, {
- hash: state.serializedCard && dirty ? state.serializedCard : "",
- });
-}
-
export function cleanCopyCard(card) {
const cardCopy = {};
for (const name in card) {
diff --git a/frontend/src/metabase/lib/urls.js b/frontend/src/metabase/lib/urls.js
index c721c300efe..6e1fec2a5de 100644
--- a/frontend/src/metabase/lib/urls.js
+++ b/frontend/src/metabase/lib/urls.js
@@ -25,7 +25,7 @@ export const newPulse = () => `/pulse/create`;
export const newCollection = collectionId =>
`collection/${collectionId}/new_collection`;
-export function question(card, { hash = "", query = "" } = {}) {
+export function question(card, { hash = "", query = "", objectId } = {}) {
if (hash && typeof hash === "object") {
hash = serializeCardForUrl(hash);
}
@@ -49,8 +49,7 @@ export function question(card, { hash = "", query = "" } = {}) {
}
const { card_id, id, name } = card;
- const basePath =
- card?.dataset || card?.model === "dataset" ? "model" : "question";
+ let path = card?.dataset || card?.model === "dataset" ? "model" : "question";
/**
* If the question has been added to the dashboard we're reading the dashCard's properties.
@@ -59,6 +58,7 @@ export function question(card, { hash = "", query = "" } = {}) {
* There can be multiple instances of the same question in a dashboard, hence this distinction.
*/
const questionId = card_id || id;
+ path = `/${path}/${questionId}`;
/**
* Although it's not possible to intentionally save a question without a name,
@@ -66,11 +66,13 @@ export function question(card, { hash = "", query = "" } = {}) {
*
* Please see: https://github.com/metabase/metabase/pull/15989#pullrequestreview-656646149
*/
- if (!name) {
- return `/${basePath}/${questionId}${query}${hash}`;
+ if (name) {
+ path = appendSlug(path, slugg(name));
}
- const path = appendSlug(`/${basePath}/${questionId}`, slugg(name));
+ if (objectId) {
+ path = `${path}/${objectId}`;
+ }
return `${path}${query}${hash}`;
}
@@ -91,8 +93,11 @@ const flattenParam = ([key, value]) => {
return [[key, value]];
};
-export function newQuestion({ mode, creationType, ...options } = {}) {
- const url = Question.create(options).getUrl({ creationType });
+export function newQuestion({ mode, creationType, objectId, ...options } = {}) {
+ const url = Question.create(options).getUrl({
+ creationType,
+ query: objectId && { objectId },
+ });
if (mode) {
return url.replace(/^\/question/, `/question\/${mode}`);
} else {
diff --git a/frontend/src/metabase/modes/components/drill/ObjectDetailDrill.jsx b/frontend/src/metabase/modes/components/drill/ObjectDetailDrill.jsx
index d79227fce9f..6ebcbdf3aec 100644
--- a/frontend/src/metabase/modes/components/drill/ObjectDetailDrill.jsx
+++ b/frontend/src/metabase/modes/components/drill/ObjectDetailDrill.jsx
@@ -1,5 +1,42 @@
import { t } from "ttag";
import { isFK, isPK } from "metabase/lib/schema_metadata";
+import * as Urls from "metabase/lib/urls";
+import { zoomInRow } from "metabase/query_builder/actions";
+
+function hasManyPKColumns(question) {
+ return (
+ question
+ .query()
+ .table()
+ .fields.filter(field => field.isPK()).length > 1
+ );
+}
+
+function getActionForPKColumn({ question, column, objectId, isDashboard }) {
+ if (hasManyPKColumns(question)) {
+ // Filter by a clicked value, then a user can click on the 2nd, 3d, ..., Nth PK cells
+ // to narrow down filtering and eventually enter the object detail view once all PKs are filtered
+ return ["question", () => question.filter("=", column, objectId)];
+ }
+ if (isDashboard) {
+ return ["url", () => Urls.question(question.card(), { objectId })];
+ }
+ return ["action", () => zoomInRow({ objectId })];
+}
+
+function getActionForFKColumn({ targetField, objectId }) {
+ const databaseId = targetField.table.database.id;
+ const tableId = targetField.table_id;
+ return [
+ "url",
+ () =>
+ Urls.newQuestion({
+ databaseId,
+ tableId,
+ objectId,
+ }),
+ ];
+}
export default ({ question, clicked }) => {
if (
@@ -10,25 +47,41 @@ export default ({ question, clicked }) => {
return [];
}
- let field = question.metadata().field(clicked.column.id);
+ const { column, value: objectId, extraData } = clicked;
+ const isDashboard = !!extraData?.dashboard;
+
+ let field = question.metadata().field(column.id);
+ if (isFK(column)) {
+ field = field.target;
+ }
if (!field) {
return [];
}
- if (field.target) {
- field = field.target;
+ const actionObject = {
+ name: "object-detail",
+ section: "details",
+ title: t`View details`,
+ buttonType: "horizontal",
+ icon: "document",
+ default: true,
+ };
+
+ if (isPK(column)) {
+ const [actionKey, action] = getActionForPKColumn({
+ question,
+ column,
+ objectId,
+ isDashboard,
+ });
+ actionObject[actionKey] = action;
+ } else {
+ const [actionKey, action] = getActionForFKColumn({
+ targetField: field,
+ objectId,
+ });
+ actionObject[actionKey] = action;
}
- return [
- {
- name: "object-detail",
- section: "details",
- title: t`View details`,
- buttonType: "horizontal",
- icon: "document",
- default: true,
- question: () =>
- field ? question.drillPK(field, clicked.value) : question,
- },
- ];
+ return [actionObject];
};
diff --git a/frontend/src/metabase/query_builder/actions.js b/frontend/src/metabase/query_builder/actions.js
index 21a5ea57dad..484e284538e 100644
--- a/frontend/src/metabase/query_builder/actions.js
+++ b/frontend/src/metabase/query_builder/actions.js
@@ -23,7 +23,6 @@ import {
deserializeCardFromUrl,
serializeCardForUrl,
cleanCopyCard,
- urlForCardState,
} from "metabase/lib/card";
import { open, shouldOpenInBlankWindow } from "metabase/lib/dom";
import * as Q_DEPRECATED from "metabase/lib/query";
@@ -62,6 +61,9 @@ import {
getNativeEditorSelectedText,
getSnippetCollectionId,
getQueryResults,
+ getZoomedObjectId,
+ getPreviousRowPKValue,
+ getNextRowPKValue,
isBasedOnExistingQuestion,
} from "./selectors";
import { trackNewQuestionSaved } from "./analytics";
@@ -84,6 +86,8 @@ import { getMetadata } from "metabase/selectors/metadata";
import { setRequestUnloaded } from "metabase/redux/requests";
import {
+ getCurrentQueryParams,
+ getURLForCardState,
getQueryBuilderModeFromLocation,
getPathNameFromQueryBuilderMode,
getNextTemplateTagVisibilityState,
@@ -154,6 +158,24 @@ export const popState = createThunkAction(
POP_STATE,
location => async (dispatch, getState) => {
dispatch(cancelQuery());
+
+ const zoomedObjectId = getZoomedObjectId(getState());
+ if (zoomedObjectId) {
+ const { locationBeforeTransitions = {} } = getState().routing;
+ const { state, query } = locationBeforeTransitions;
+ const previouslyZoomedObjectId = state?.objectId || query?.objectId;
+
+ if (
+ previouslyZoomedObjectId &&
+ zoomedObjectId !== previouslyZoomedObjectId
+ ) {
+ dispatch(zoomInRow({ objectId: previouslyZoomedObjectId }));
+ } else {
+ dispatch(resetRowZoom());
+ }
+ return;
+ }
+
const card = getCard(getState());
if (location.state && location.state.card) {
if (!Utils.equals(card, location.state.card)) {
@@ -245,6 +267,7 @@ export const updateUrl = createThunkAction(
preserveParameters = true,
queryBuilderMode,
datasetEditorTab,
+ objectId,
} = {},
) => (dispatch, getState) => {
let question;
@@ -282,10 +305,12 @@ export const updateUrl = createThunkAction(
card: copy,
cardId: copy.id,
serializedCard: serializeCardForUrl(copy),
+ objectId,
};
const { currentState } = getState().qb;
- const url = urlForCardState(newState, dirty);
+ const queryParams = preserveParameters ? getCurrentQueryParams() : {};
+ const url = getURLForCardState(newState, dirty, queryParams, objectId);
const urlParsed = urlParse(url);
const locationDescriptor = {
@@ -294,7 +319,7 @@ export const updateUrl = createThunkAction(
queryBuilderMode,
datasetEditorTab,
}),
- search: preserveParameters ? window.location.search : "",
+ search: urlParsed.search,
hash: urlParsed.hash,
state: newState,
};
@@ -366,8 +391,9 @@ async function verifyMatchingDashcardAndParameters({
}
export const INITIALIZE_QB = "metabase/qb/INITIALIZE_QB";
-export const initializeQB = (location, params, queryParams) => {
+export const initializeQB = (location, params) => {
return async (dispatch, getState) => {
+ const queryParams = location.query;
// do this immediately to ensure old state is cleared before the user sees it
dispatch(resetQB());
dispatch(cancelQuery());
@@ -627,12 +653,15 @@ export const initializeQB = (location, params, queryParams) => {
metadata,
);
+ const objectId = params?.objectId || queryParams?.objectId;
+
// Update the question to Redux state together with the initial state of UI controls
dispatch.action(INITIALIZE_QB, {
card,
originalCard,
uiControls,
parameterValues,
+ objectId,
});
// if we have loaded up a card that we can run then lets kick that off as well
@@ -653,6 +682,7 @@ export const initializeQB = (location, params, queryParams) => {
updateUrl(card, {
replaceState: true,
preserveParameters,
+ objectId,
}),
);
}
@@ -1403,6 +1433,18 @@ export const cancelQuery = () => (dispatch, getState) => {
}
};
+export const ZOOM_IN_ROW = "metabase/qb/ZOOM_IN_ROW";
+export const zoomInRow = ({ objectId }) => dispatch => {
+ dispatch({ type: ZOOM_IN_ROW, payload: { objectId } });
+ dispatch(updateUrl(null, { objectId, replaceState: false }));
+};
+
+export const RESET_ROW_ZOOM = "metabase/qb/RESET_ROW_ZOOM";
+export const resetRowZoom = () => dispatch => {
+ dispatch({ type: RESET_ROW_ZOOM });
+ dispatch(updateUrl());
+};
+
// We use this for two things:
// - counting the rows with this as an FK (loadObjectDetailFKReferences)
// - following those links to a new card that's filtered (followForeignKey)
@@ -1506,55 +1548,21 @@ export const loadObjectDetailFKReferences = createThunkAction(
export const CLEAR_OBJECT_DETAIL_FK_REFERENCES =
"metabase/qb/CLEAR_OBJECT_DETAIL_FK_REFERENCES";
-export const VIEW_NEXT_OBJECT_DETAIL = "metabase/qb/VIEW_NEXT_OBJECT_DETAIL";
export const viewNextObjectDetail = () => {
return (dispatch, getState) => {
- const question = getQuestion(getState());
- const filter = question.query().filters()[0];
-
- const newFilter = ["=", filter[1], parseInt(filter[2]) + 1];
-
- dispatch.action(VIEW_NEXT_OBJECT_DETAIL);
-
- dispatch(
- updateQuestion(
- question
- .query()
- .updateFilter(0, newFilter)
- .question(),
- ),
- );
-
- dispatch(runQuestionQuery());
+ const objectId = getNextRowPKValue(getState());
+ if (objectId != null) {
+ dispatch(zoomInRow({ objectId }));
+ }
};
};
-export const VIEW_PREVIOUS_OBJECT_DETAIL =
- "metabase/qb/VIEW_PREVIOUS_OBJECT_DETAIL";
-
export const viewPreviousObjectDetail = () => {
return (dispatch, getState) => {
- const question = getQuestion(getState());
- const filter = question.query().filters()[0];
-
- if (filter[2] === 1) {
- return false;
+ const objectId = getPreviousRowPKValue(getState());
+ if (objectId != null) {
+ dispatch(zoomInRow({ objectId }));
}
-
- const newFilter = ["=", filter[1], parseInt(filter[2]) - 1];
-
- dispatch.action(VIEW_PREVIOUS_OBJECT_DETAIL);
-
- dispatch(
- updateQuestion(
- question
- .query()
- .updateFilter(0, newFilter)
- .question(),
- ),
- );
-
- dispatch(runQuestionQuery());
};
};
diff --git a/frontend/src/metabase/query_builder/containers/QueryBuilder.jsx b/frontend/src/metabase/query_builder/containers/QueryBuilder.jsx
index 415e46d6910..a2024aeb865 100644
--- a/frontend/src/metabase/query_builder/containers/QueryBuilder.jsx
+++ b/frontend/src/metabase/query_builder/containers/QueryBuilder.jsx
@@ -167,11 +167,7 @@ export default class QueryBuilder extends Component {
}
UNSAFE_componentWillMount() {
- this.props.initializeQB(
- this.props.location,
- this.props.params,
- this.props.location.query,
- );
+ this.props.initializeQB(this.props.location, this.props.params);
}
componentDidMount() {
diff --git a/frontend/src/metabase/query_builder/reducers.js b/frontend/src/metabase/query_builder/reducers.js
index 5d108c8009a..d68e20532c9 100644
--- a/frontend/src/metabase/query_builder/reducers.js
+++ b/frontend/src/metabase/query_builder/reducers.js
@@ -39,6 +39,8 @@ import {
CANCEL_DATASET_CHANGES,
SET_RESULTS_METADATA,
SET_METADATA_DIFF,
+ ZOOM_IN_ROW,
+ RESET_ROW_ZOOM,
onEditSummary,
onCloseSummary,
onAddFilter,
@@ -273,6 +275,20 @@ export const uiControls = handleActions(
DEFAULT_UI_CONTROLS,
);
+export const zoomedRowObjectId = handleActions(
+ {
+ [INITIALIZE_QB]: {
+ next: (state, { payload }) => payload?.objectId ?? null,
+ },
+ [ZOOM_IN_ROW]: {
+ next: (state, { payload }) => payload.objectId,
+ },
+ [RESET_ROW_ZOOM]: { next: () => null },
+ [RESET_QB]: { next: () => null },
+ },
+ null,
+);
+
// the card that is actively being worked on
export const card = handleActions(
{
diff --git a/frontend/src/metabase/query_builder/selectors.js b/frontend/src/metabase/query_builder/selectors.js
index c9cb43cd094..f0418150c60 100644
--- a/frontend/src/metabase/query_builder/selectors.js
+++ b/frontend/src/metabase/query_builder/selectors.js
@@ -14,7 +14,7 @@ import {
import { getComputedSettingsForSeries } from "metabase/visualizations/lib/settings/visualization";
import { getValueAndFieldIdPopulatedParametersFromCard } from "metabase/parameters/utils/cards";
import { normalizeParameterValue } from "metabase/parameters/utils/parameter-values";
-
+import { isPK } from "metabase/lib/schema_metadata";
import Utils from "metabase/lib/utils";
import Question from "metabase-lib/lib/Question";
@@ -87,6 +87,33 @@ export const getFirstQueryResult = createSelector([getQueryResults], results =>
Array.isArray(results) ? results[0] : null,
);
+export const getPKColumnIndex = createSelector(
+ [getFirstQueryResult],
+ result => {
+ if (!result) {
+ return;
+ }
+ const { cols } = result.data;
+ return cols.findIndex(isPK);
+ },
+);
+
+export const getPKRowIndexMap = createSelector(
+ [getFirstQueryResult, getPKColumnIndex],
+ (result, PKColumnIndex) => {
+ if (!result || !Number.isSafeInteger(PKColumnIndex)) {
+ return {};
+ }
+ const { rows } = result.data;
+ const map = {};
+ rows.forEach((row, index) => {
+ const PKValue = row[PKColumnIndex];
+ map[PKValue] = index;
+ });
+ return map;
+ },
+);
+
// get instance settings, used for determining whether to display certain actions
export const getSettings = state => state.settings.values;
@@ -329,14 +356,82 @@ export const getLastRunQuestion = createSelector(
card && metadata && new Question(card, metadata, parameterValues),
);
+export const getZoomedObjectId = state => state.qb.zoomedRowObjectId;
+
+const getZoomedObjectRowIndex = createSelector(
+ [getPKRowIndexMap, getZoomedObjectId],
+ (PKRowIndexMap, objectId) => {
+ if (!PKRowIndexMap) {
+ return;
+ }
+ return PKRowIndexMap[objectId] || PKRowIndexMap[parseInt(objectId)];
+ },
+);
+
+export const getPreviousRowPKValue = createSelector(
+ [getFirstQueryResult, getPKColumnIndex, getZoomedObjectRowIndex],
+ (result, PKColumnIndex, rowIndex) => {
+ if (!result) {
+ return;
+ }
+ const { rows } = result.data;
+ return rows[rowIndex - 1][PKColumnIndex];
+ },
+);
+
+export const getNextRowPKValue = createSelector(
+ [getFirstQueryResult, getPKColumnIndex, getZoomedObjectRowIndex],
+ (result, PKColumnIndex, rowIndex) => {
+ if (!result) {
+ return;
+ }
+ const { rows } = result.data;
+ return rows[rowIndex + 1][PKColumnIndex];
+ },
+);
+
+export const getCanZoomPreviousRow = createSelector(
+ [getZoomedObjectRowIndex],
+ rowIndex => rowIndex !== 0,
+);
+
+export const getCanZoomNextRow = createSelector(
+ [getQueryResults, getZoomedObjectRowIndex],
+ (queryResults, rowIndex) => {
+ if (!Array.isArray(queryResults) || !queryResults.length) {
+ return;
+ }
+ const rowCount = queryResults[0].data.rows.length;
+ return rowIndex !== rowCount - 1;
+ },
+);
+
+export const getZoomRow = createSelector(
+ [getQueryResults, getZoomedObjectRowIndex],
+ (queryResults, rowIndex) => {
+ if (!Array.isArray(queryResults) || !queryResults.length) {
+ return;
+ }
+ return queryResults[0].data.rows[rowIndex];
+ },
+);
+
+const isZoomingRow = createSelector(
+ [getZoomedObjectId],
+ index => index != null,
+);
+
export const getMode = createSelector(
[getLastRunQuestion],
question => question && question.mode(),
);
export const getIsObjectDetail = createSelector(
- [getMode, getQueryResults],
- (mode, results) => {
+ [getMode, getQueryResults, isZoomingRow],
+ (mode, results, isZoomingSingleRow) => {
+ if (isZoomingSingleRow) {
+ return true;
+ }
// It handles filtering by a manually set PK column that is not unique
const hasMultipleRows = results?.some(({ data }) => data?.rows.length > 1);
return mode?.name() === "object" && !hasMultipleRows;
diff --git a/frontend/src/metabase/query_builder/utils.js b/frontend/src/metabase/query_builder/utils.js
index 1a4e76b7f94..48ae8571811 100644
--- a/frontend/src/metabase/query_builder/utils.js
+++ b/frontend/src/metabase/query_builder/utils.js
@@ -1,4 +1,6 @@
+import querystring from "querystring";
import { isSupportedTemplateTagForModel } from "metabase/lib/data-modeling/utils";
+import * as Urls from "metabase/lib/urls";
import NativeQuery from "metabase-lib/lib/queries/NativeQuery";
// Query Builder Mode
@@ -35,6 +37,35 @@ export function getPathNameFromQueryBuilderMode({
return `${pathname}/${queryBuilderMode}`;
}
+export function getCurrentQueryParams() {
+ const search =
+ window.location.search.charAt(0) === "?"
+ ? window.location.search.slice(0)
+ : window.location.search;
+ return querystring.parse(search);
+}
+
+export function getURLForCardState(
+ { card, serializedCard },
+ dirty,
+ query = {},
+ objectId,
+) {
+ const options = {
+ hash: serializedCard && dirty ? serializedCard : "",
+ query,
+ };
+ const isAdHocQuestion = !card.id;
+ if (objectId != null) {
+ if (isAdHocQuestion) {
+ options.query.objectId = objectId;
+ } else {
+ options.objectId = objectId;
+ }
+ }
+ return Urls.question(card, options);
+}
+
function getTemplateTagWithoutSnippetsCount(question) {
const query = question.query();
return query instanceof NativeQuery
diff --git a/frontend/src/metabase/routes.jsx b/frontend/src/metabase/routes.jsx
index 78bc52ebcc1..c8fdade965d 100644
--- a/frontend/src/metabase/routes.jsx
+++ b/frontend/src/metabase/routes.jsx
@@ -234,6 +234,7 @@ export const getRoutes = store => (
<Route path="notebook" component={QueryBuilder} />
<Route path=":slug" component={QueryBuilder} />
<Route path=":slug/notebook" component={QueryBuilder} />
+ <Route path=":slug/:objectId" component={QueryBuilder} />
</Route>
<Route path="/model">
@@ -243,6 +244,7 @@ export const getRoutes = store => (
<Route path=":slug/notebook" component={QueryBuilder} />
<Route path=":slug/query" component={QueryBuilder} />
<Route path=":slug/metadata" component={QueryBuilder} />
+ <Route path=":slug/:objectId" component={QueryBuilder} />
</Route>
<Route path="browse" component={BrowseApp}>
diff --git a/frontend/src/metabase/visualizations/components/ChartClickActions.jsx b/frontend/src/metabase/visualizations/components/ChartClickActions.jsx
index fe0c93b7623..0bcbb1e05d0 100644
--- a/frontend/src/metabase/visualizations/components/ChartClickActions.jsx
+++ b/frontend/src/metabase/visualizations/components/ChartClickActions.jsx
@@ -277,20 +277,9 @@ export const ChartClickAction = ({ action, isLastItem, handleClickAction }) => {
"token token-filter text-small text-white-hover mr1":
action.buttonType === "token-filter",
});
- // NOTE: Tom Robinson 4/16/2018: disabling <Link> for `question` click actions
- // for now since on dashboards currently they need to go through
- // navigateToNewCardFromDashboard to merge in parameters.,
- // Also need to sort out proper logic in QueryBuilder's UNSAFE_componentWillReceiveProps
- // if (action.question) {
- // return (
- // <Link to={action.question().getUrl()} className={className}>
- // {action.title}
- // </Link>
- // );
- // } else
if (action.url) {
return (
- <div>
+ <div className="full">
<Link
to={action.url()}
className={className}
diff --git a/frontend/src/metabase/visualizations/visualizations/ObjectDetail.jsx b/frontend/src/metabase/visualizations/visualizations/ObjectDetail.jsx
index 048bdfe3271..444310e6380 100644
--- a/frontend/src/metabase/visualizations/visualizations/ObjectDetail.jsx
+++ b/frontend/src/metabase/visualizations/visualizations/ObjectDetail.jsx
@@ -8,6 +8,7 @@ import Icon from "metabase/components/Icon";
import IconBorder from "metabase/components/IconBorder";
import LoadingSpinner from "metabase/components/LoadingSpinner";
+import { NotFound } from "metabase/containers/ErrorPages";
import {
isID,
isPK,
@@ -15,7 +16,11 @@ import {
} from "metabase/lib/schema_metadata";
import { TYPE, isa } from "metabase/lib/types";
import { inflect } from "inflection";
-import { formatValue, formatColumn } from "metabase/lib/formatting";
+import {
+ formatValue,
+ formatColumn,
+ singularize,
+} from "metabase/lib/formatting";
import Tables from "metabase/entities/tables";
import {
@@ -25,9 +30,14 @@ import {
viewNextObjectDetail,
} from "metabase/query_builder/actions";
import {
+ getQuestion,
getTableMetadata,
getTableForeignKeys,
getTableForeignKeyReferences,
+ getZoomRow,
+ getZoomedObjectId,
+ getCanZoomPreviousRow,
+ getCanZoomNextRow,
} from "metabase/query_builder/selectors";
import { columnSettings } from "metabase/visualizations/lib/settings/column";
@@ -36,9 +46,14 @@ import cx from "classnames";
import _ from "underscore";
const mapStateToProps = state => ({
+ question: getQuestion(state),
table: getTableMetadata(state),
tableForeignKeys: getTableForeignKeys(state),
tableForeignKeyReferences: getTableForeignKeyReferences(state),
+ zoomedRow: getZoomRow(state),
+ zoomedRowID: getZoomedObjectId(state),
+ canZoomPreviousRow: getCanZoomPreviousRow(state),
+ canZoomNextRow: getCanZoomNextRow(state),
});
// ugh, using function form of mapDispatchToProps here due to circlular dependency with actions
@@ -64,8 +79,18 @@ export class ObjectDetail extends Component {
...columnSettings({ hidden: true }),
};
+ state = {
+ hasNotFoundError: false,
+ };
+
componentDidMount() {
- const { table } = this.props;
+ const { data, table, zoomedRow, zoomedRowID } = this.props;
+ const notFoundObject = zoomedRowID != null && !zoomedRow;
+ if (data && notFoundObject) {
+ this.setState({ hasNotFoundError: true });
+ return;
+ }
+
if (table && table.fks == null) {
this.props.fetchTableFks(table.id);
}
@@ -76,6 +101,16 @@ export class ObjectDetail extends Component {
window.addEventListener("keydown", this.onKeyDown, true);
}
+ componentDidUpdate(prevProps) {
+ const { data: prevData } = prevProps;
+ const { data, zoomedRow, zoomedRowID } = this.props;
+ const queryCompleted = !prevData && data;
+ const notFoundObject = zoomedRowID != null && !zoomedRow;
+ if (queryCompleted && notFoundObject) {
+ this.setState({ hasNotFoundError: true });
+ }
+ }
+
componentWillUnmount() {
window.removeEventListener("keydown", this.onKeyDown, true);
}
@@ -90,13 +125,15 @@ export class ObjectDetail extends Component {
}
getIdValue() {
- if (!this.props.data) {
+ const { data, zoomedRowID } = this.props;
+ if (!data) {
return null;
}
+ if (zoomedRowID) {
+ return zoomedRowID;
+ }
- const {
- data: { cols, rows },
- } = this.props;
+ const { cols, rows } = data;
const columnIndex = _.findIndex(cols, col => isPK(col));
return rows[0][columnIndex];
}
@@ -178,18 +215,20 @@ export class ObjectDetail extends Component {
renderDetailsTable() {
const {
- data: { cols, rows },
+ zoomedRow,
+ data: { rows, cols },
} = this.props;
+ const row = zoomedRow || rows[0];
return cols.map((column, columnIndex) => (
<div className="Grid Grid--1of2 mb2" key={columnIndex}>
<div className="Grid-cell">
- {this.cellRenderer(column, rows[0][columnIndex], true)}
+ {this.cellRenderer(column, row[columnIndex], true)}
</div>
<div
style={{ wordWrap: "break-word" }}
className="Grid-cell text-bold text-dark"
>
- {this.cellRenderer(column, rows[0][columnIndex], false)}
+ {this.cellRenderer(column, row[columnIndex], false)}
</div>
</div>
));
@@ -293,15 +332,30 @@ export class ObjectDetail extends Component {
}
};
+ getObjectName = () => {
+ const { question, table } = this.props;
+ const tableObjectName = table && table.objectName();
+ if (tableObjectName) {
+ return tableObjectName;
+ }
+ const questionName = question && question.displayName();
+ if (questionName) {
+ return singularize(questionName);
+ }
+ return t`Unknown`;
+ };
+
render() {
- const { data, table } = this.props;
+ const { data, zoomedRow, canZoomPreviousRow, canZoomNextRow } = this.props;
if (!data) {
return false;
}
+ if (this.state.hasNotFoundError) {
+ return <NotFound />;
+ }
- const tableName = table ? table.objectName() : t`Unknown`;
- // TODO: once we nail down the "title" column of each table this should be something other than the id
- const idValue = this.getIdValue();
+ const canZoom = !!zoomedRow;
+ const objectName = this.getObjectName();
return (
<div className="scroll-y pt2 px4">
@@ -309,8 +363,8 @@ export class ObjectDetail extends Component {
<div className="Grid border-bottom relative">
<div className="Grid-cell border-right px4 py3 ml2 arrow-right">
<div className="text-brand text-bold">
- <span>{tableName}</span>
- <h1>{idValue}</h1>
+ <span>{objectName}</span>
+ <h1>{this.getIdValue()}</h1>
</div>
</div>
<div className="Grid-cell flex align-center Cell--1of3 bg-alt">
@@ -318,39 +372,50 @@ export class ObjectDetail extends Component {
<Icon name="connections" size={17} />
<div className="ml2">
{jt`This ${(
- <span className="text-dark">{tableName}</span>
+ <span className="text-dark">{objectName}</span>
)} is connected to:`}
</div>
</div>
</div>
- <div
- className={cx(
- "absolute left cursor-pointer text-brand-hover lg-ml2",
- { disabled: idValue <= 1 },
- )}
- style={{
- top: "50%",
- transform: "translate(-50%, -50%)",
- }}
- >
- <DirectionalButton
- direction="left"
- onClick={this.props.viewPreviousObjectDetail}
- />
- </div>
- <div
- className="absolute right cursor-pointer text-brand-hover lg-ml2"
- style={{
- top: "50%",
- transform: "translate(50%, -50%)",
- }}
- >
- <DirectionalButton
- direction="right"
- onClick={this.props.viewNextObjectDetail}
- />
- </div>
+ {canZoom && (
+ <div
+ className={cx(
+ "absolute left cursor-pointer text-brand-hover lg-ml2",
+ { disabled: !canZoomPreviousRow },
+ )}
+ aria-disabled={!canZoomPreviousRow}
+ style={{
+ top: "50%",
+ transform: "translate(-50%, -50%)",
+ }}
+ data-testid="view-previous-object-detail"
+ >
+ <DirectionalButton
+ direction="left"
+ onClick={this.props.viewPreviousObjectDetail}
+ />
+ </div>
+ )}
+ {canZoom && (
+ <div
+ className={cx(
+ "absolute right cursor-pointer text-brand-hover lg-ml2",
+ { disabled: !canZoomNextRow },
+ )}
+ aria-disabled={!canZoomNextRow}
+ style={{
+ top: "50%",
+ transform: "translate(50%, -50%)",
+ }}
+ data-testid="view-next-object-detail"
+ >
+ <DirectionalButton
+ direction="right"
+ onClick={this.props.viewNextObjectDetail}
+ />
+ </div>
+ )}
</div>
<div className="Grid">
<div
diff --git a/frontend/test/metabase/lib/urls.unit.spec.js b/frontend/test/metabase/lib/urls.unit.spec.js
index cefead4d0fa..aecc42bc90f 100644
--- a/frontend/test/metabase/lib/urls.unit.spec.js
+++ b/frontend/test/metabase/lib/urls.unit.spec.js
@@ -71,17 +71,48 @@ describe("urls", () => {
});
});
+ describe("with object ID", () => {
+ it("should append object ID to path", () => {
+ const url = question({ id: 1 }, { objectId: 5 });
+ expect(url).toBe("/question/1/5");
+ });
+
+ it("should support query params", () => {
+ const url = question({ id: 1 }, { query: "?a=b", objectId: 5 });
+ expect(url).toBe("/question/1/5?a=b");
+ });
+
+ it("should support hash", () => {
+ const url = question({ id: 1 }, { hash: "abc", objectId: 5 });
+ expect(url).toBe("/question/1/5#abc");
+ });
+
+ it("should support both hash and query params", () => {
+ const url = question(
+ { id: 1, name: "foo" },
+ { hash: "abc", query: "a=b", objectId: 5 },
+ );
+ expect(url).toBe("/question/1-foo/5?a=b#abc");
+ });
+ });
+
describe("model", () => {
it("returns /model URLS", () => {
expect(question({ id: 1, dataset: true, name: "Foo" })).toEqual(
"/model/1-foo",
);
+
expect(
question({ id: 1, card_id: 42, dataset: true, name: "Foo" }),
).toEqual("/model/42-foo");
+
expect(
question({ id: 1, card_id: 42, model: "dataset", name: "Foo" }),
).toEqual("/model/42-foo");
+
+ expect(
+ question({ id: 1, dataset: true, name: "Foo" }, { objectId: 4 }),
+ ).toEqual("/model/1-foo/4");
});
});
});
diff --git a/frontend/test/metabase/modes/components/drill/ObjectDetailDrill.unit.spec.js b/frontend/test/metabase/modes/components/drill/ObjectDetailDrill.unit.spec.js
index 6f58abbd11b..17577b978bf 100644
--- a/frontend/test/metabase/modes/components/drill/ObjectDetailDrill.unit.spec.js
+++ b/frontend/test/metabase/modes/components/drill/ObjectDetailDrill.unit.spec.js
@@ -1,5 +1,11 @@
+import Question from "metabase-lib/lib/Question";
import ObjectDetailDrill from "metabase/modes/components/drill/ObjectDetailDrill";
-import { ORDERS, PRODUCTS } from "__support__/sample_database_fixture";
+import { ZOOM_IN_ROW } from "metabase/query_builder/actions";
+import {
+ ORDERS,
+ SAMPLE_DATABASE,
+ metadata,
+} from "__support__/sample_database_fixture";
const DEFAULT_CELL_VALUE = 1;
@@ -7,10 +13,11 @@ function setup({
question = ORDERS.question(),
column = ORDERS.ID.column(),
value = DEFAULT_CELL_VALUE,
+ extraData,
} = {}) {
const actions = ObjectDetailDrill({
question,
- clicked: { column, value },
+ clicked: { column, value, extraData },
});
return {
actions,
@@ -18,6 +25,21 @@ function setup({
};
}
+const SAVED_QUESTION = new Question(
+ {
+ id: 1,
+ name: "orders",
+ dataset_query: {
+ type: "query",
+ database: SAMPLE_DATABASE.id,
+ query: {
+ "source-table": ORDERS.id,
+ },
+ },
+ },
+ metadata,
+);
+
describe("ObjectDetailDrill", () => {
it("should not be valid for top level actions", () => {
const actions = ObjectDetailDrill({ question: ORDERS.question() });
@@ -47,20 +69,81 @@ describe("ObjectDetailDrill", () => {
});
describe("PK cells", () => {
- const { actions, cellValue } = setup({
- column: ORDERS.ID.column(),
- });
+ describe("general", () => {
+ const mockDispatch = jest.fn();
+ const { actions, cellValue } = setup({
+ column: ORDERS.ID.column(),
+ });
- it("should return object detail filter", () => {
- expect(actions).toMatchObject([{ name: "object-detail" }]);
+ it("should return object detail filter", () => {
+ expect(actions).toMatchObject([
+ { name: "object-detail", action: expect.any(Function) },
+ ]);
+ });
+
+ it("should return correct redux action", () => {
+ const [action] = actions;
+ action.action()(mockDispatch);
+ expect(mockDispatch).toHaveBeenCalledWith({
+ type: ZOOM_IN_ROW,
+ payload: {
+ objectId: cellValue,
+ },
+ });
+ });
+
+ describe("composed PK", () => {
+ const question = ORDERS.question();
+ const orderTotalField = question
+ .query()
+ .table()
+ .fields.find(field => field.id === ORDERS.TOTAL.id);
+ orderTotalField.semantic_type = "type/PK";
+
+ const { actions, cellValue } = setup({
+ question,
+ column: ORDERS.ID.column(),
+ });
+
+ it("should return object detail filter", () => {
+ expect(actions).toMatchObject([
+ { name: "object-detail", question: expect.any(Function) },
+ ]);
+ });
+
+ it("should apply '=' filter to one of the PKs on click", () => {
+ const [action] = actions;
+ const card = action.question().card();
+ expect(card.dataset_query.query).toEqual({
+ "source-table": ORDERS.id,
+ filter: ["=", ORDERS.ID.reference(), cellValue],
+ });
+ });
+
+ orderTotalField.semantic_type = null;
+ });
});
- it("should apply object detail filter correctly", () => {
- const [action] = actions;
- const card = action.question().card();
- expect(card.dataset_query.query).toEqual({
- "source-table": ORDERS.id,
- filter: ["=", ORDERS.ID.reference(), cellValue],
+ describe("from dashboard", () => {
+ const { actions, cellValue } = setup({
+ question: SAVED_QUESTION,
+ column: ORDERS.ID.column(),
+ extraData: {
+ dashboard: { id: 5 },
+ },
+ });
+
+ it("should return object detail filter", () => {
+ expect(actions).toMatchObject([
+ { name: "object-detail", url: expect.any(Function) },
+ ]);
+ });
+
+ it("should return correct URL to object detail", () => {
+ const [action] = actions;
+ expect(action.url()).toBe(
+ `/question/${SAVED_QUESTION.id()}-${SAVED_QUESTION.displayName()}/${cellValue}`,
+ );
});
});
});
@@ -71,16 +154,16 @@ describe("ObjectDetailDrill", () => {
});
it("should return object detail filter", () => {
- expect(actions).toMatchObject([{ name: "object-detail" }]);
+ expect(actions).toMatchObject([
+ { name: "object-detail", url: expect.any(Function) },
+ ]);
});
it("should apply object detail filter correctly", () => {
const [action] = actions;
- const card = action.question().card();
- expect(card.dataset_query.query).toEqual({
- "source-table": PRODUCTS.id,
- filter: ["=", PRODUCTS.ID.reference(), cellValue],
- });
+ const [urlPath, urlHash] = action.url().split("#");
+ expect(urlPath).toBe(`/question?objectId=${cellValue}`);
+ expect(urlHash.length).toBeGreaterThan(0);
});
});
});
diff --git a/frontend/test/metabase/scenarios/dashboard/dashboard-drill.cy.spec.js b/frontend/test/metabase/scenarios/dashboard/dashboard-drill.cy.spec.js
index 0049359a778..5a59aa18065 100644
--- a/frontend/test/metabase/scenarios/dashboard/dashboard-drill.cy.spec.js
+++ b/frontend/test/metabase/scenarios/dashboard/dashboard-drill.cy.spec.js
@@ -599,7 +599,7 @@ describe("scenarios > dashboard > dashboard drill", () => {
it('should drill-through on PK/FK to the "object detail" when filtered by explicit joined column (metabase#15331)', () => {
cy.server();
- cy.route("POST", "/api/dataset").as("dataset");
+ cy.route("POST", "/api/card/*/query").as("cardQuery");
cy.createQuestion({
name: "15331",
@@ -675,7 +675,7 @@ describe("scenarios > dashboard > dashboard drill", () => {
.contains("1")
.click();
- cy.wait("@dataset").then(xhr => {
+ cy.wait("@cardQuery").then(xhr => {
expect(xhr.response.body.error).to.not.exist;
});
cy.findByText("37.65");
diff --git a/frontend/test/metabase/scenarios/question/new.cy.spec.js b/frontend/test/metabase/scenarios/question/new.cy.spec.js
index 6d502ab99cc..5e75c778a7b 100644
--- a/frontend/test/metabase/scenarios/question/new.cy.spec.js
+++ b/frontend/test/metabase/scenarios/question/new.cy.spec.js
@@ -298,18 +298,11 @@ describe("scenarios > question > new", () => {
});
it("should correctly choose between 'Object Detail' and 'Table (metabase#13717)", () => {
- // set ID to `No semantic type`
- cy.request("PUT", `/api/field/${ORDERS.ID}`, {
- semantic_type: null,
- });
- // set Quantity to `Entity Key`
cy.request("PUT", `/api/field/${ORDERS.QUANTITY}`, {
semantic_type: "type/PK",
});
openOrdersTable();
- // this url check is just to give some time for the render to finish
- cy.url().should("include", "/question#");
cy.get(".TableInteractive-cellWrapper--lastColumn") // Quantity (last in the default order for Sample Database)
.eq(1) // first table body cell
@@ -323,6 +316,13 @@ describe("scenarios > question > new", () => {
"**It should display the table with all orders with the selected quantity.**",
);
cy.get(".TableInteractive");
+
+ cy.get(".TableInteractive-cellWrapper--firstColumn") // ID (first in the default order for Sample Database)
+ .eq(1) // first table body cell
+ .should("contain", 1)
+ .click();
+
+ cy.get(".ObjectDetail");
});
// flaky test (#19454)
diff --git a/frontend/test/metabase/scenarios/visualizations/object_detail.cy.spec.js b/frontend/test/metabase/scenarios/visualizations/object_detail.cy.spec.js
index 43f95db744b..62a91d736ff 100644
--- a/frontend/test/metabase/scenarios/visualizations/object_detail.cy.spec.js
+++ b/frontend/test/metabase/scenarios/visualizations/object_detail.cy.spec.js
@@ -1,9 +1,84 @@
-import { restore } from "__support__/e2e/cypress";
+import { restore, popover } from "__support__/e2e/cypress";
+import { SAMPLE_DATABASE } from "__support__/e2e/cypress_sample_database";
+
+const { ORDERS, ORDERS_ID } = SAMPLE_DATABASE;
+
+describe("scenarios > question > object details", () => {
+ const FIRST_ORDER_ID = 9676;
+ const SECOND_ORDER_ID = 10874;
+ const THIRD_ORDER_ID = 11246;
+
+ const TEST_QUESTION = {
+ query: {
+ "source-table": ORDERS_ID,
+ filter: [
+ "and",
+ [">", ["field", ORDERS.TOTAL, null], 149],
+ [">", ["field", ORDERS.TAX, null], 10],
+ ["not-null", ["field", ORDERS.DISCOUNT, null]],
+ ],
+ },
+ };
-describe("scenarios > visualizations > object detail", () => {
beforeEach(() => {
restore();
- cy.signInAsNormalUser();
+ cy.signInAsAdmin();
+ });
+
+ it("handles browsing records by PKs", () => {
+ cy.createQuestion(TEST_QUESTION, { visitQuestion: true });
+ getFirstTableColumn()
+ .eq(1)
+ .should("contain", FIRST_ORDER_ID)
+ .click();
+
+ assertOrderDetailView({ id: FIRST_ORDER_ID });
+ getPreviousObjectDetailButton().should(
+ "have.attr",
+ "aria-disabled",
+ "true",
+ );
+
+ getNextObjectDetailButton().click();
+ assertOrderDetailView({ id: SECOND_ORDER_ID });
+
+ getNextObjectDetailButton().click();
+ assertOrderDetailView({ id: THIRD_ORDER_ID });
+ getNextObjectDetailButton().should("have.attr", "aria-disabled", "true");
+
+ getPreviousObjectDetailButton().click();
+ assertOrderDetailView({ id: SECOND_ORDER_ID });
+
+ getPreviousObjectDetailButton().click();
+ assertOrderDetailView({ id: FIRST_ORDER_ID });
+ });
+
+ it("handles browsing records by FKs", () => {
+ cy.createQuestion(TEST_QUESTION, { visitQuestion: true });
+ const FIRST_USER_ID = 1283;
+
+ cy.findByText(String(FIRST_USER_ID)).click();
+ popover()
+ .findByText("View details")
+ .click();
+
+ assertUserDetailView({ id: FIRST_USER_ID });
+ getPreviousObjectDetailButton().click();
+ assertUserDetailView({ id: FIRST_USER_ID - 1 });
+ getNextObjectDetailButton().click();
+ getNextObjectDetailButton().click();
+ assertUserDetailView({ id: FIRST_USER_ID + 1 });
+ });
+
+ it("handles opening a filtered out record", () => {
+ cy.intercept("POST", "/api/card/*/query").as("cardQuery");
+ const FILTERED_OUT_ID = 1;
+
+ 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.");
+ });
});
it("should show orders/reviews connected to a product", () => {
@@ -19,23 +94,37 @@ describe("scenarios > visualizations > object detail", () => {
.parent()
.contains("8");
});
+});
- it("should show the correct filter when clicking through on a fk", () => {
- cy.visit("/browse/1");
- cy.findByText("Products").click();
- cy.findByText("1").click();
- cy.findByText("Orders")
- .parent()
- .findByText("93")
- .click();
- cy.findByText("Product ID is 1");
- });
+function getFirstTableColumn() {
+ return cy.get(".TableInteractive-cellWrapper--firstColumn");
+}
- it("should allow clicking the next arrow", () => {
- cy.visit("/browse/1");
- cy.findByText("Products").click();
- cy.findByText("1").click();
- cy.get(".Icon-arrow_right").click();
- cy.findByText("Small Marble Shoes");
- });
-});
+function assertDetailView({ id, entityName, byFK = false }) {
+ cy.get("h1")
+ .parent()
+ .should("contain", entityName)
+ .should("contain", id);
+
+ const pattern = byFK
+ ? new RegExp(`/question\\?objectId=${id}#*`)
+ : new RegExp(`/question/[1-9]d*.*/${id}`);
+
+ cy.url().should("match", pattern);
+}
+
+function assertOrderDetailView({ id }) {
+ assertDetailView({ id, entityName: "Order" });
+}
+
+function assertUserDetailView({ id }) {
+ assertDetailView({ id, entityName: "Person", byFK: true });
+}
+
+function getPreviousObjectDetailButton() {
+ return cy.findByTestId("view-previous-object-detail");
+}
+
+function getNextObjectDetailButton() {
+ return cy.findByTestId("view-next-object-detail");
+}
--
GitLab