diff --git a/frontend/src/metabase-lib/lib/metadata/Field.js b/frontend/src/metabase-lib/lib/metadata/Field.js index 752fe9905dbd94a296a61d20cfb5cead0089fb47..e89b438d96ec6246ea028607b2748903b80bd634 100644 --- a/frontend/src/metabase-lib/lib/metadata/Field.js +++ b/frontend/src/metabase-lib/lib/metadata/Field.js @@ -27,8 +27,6 @@ import { import type { FieldValues } from "metabase/meta/types/Field"; -import _ from "underscore"; - /** * Wrapper class for field metadata objects. Belongs to a Table. */ @@ -37,6 +35,7 @@ export default class Field extends Base { description: string; table: Table; + name_field: ?Field; fieldType() { return getFieldType(this); @@ -147,11 +146,8 @@ export default class Field extends Base { } // this enables "implicit" remappings from type/PK to type/Name on the same table, // used in FieldValuesWidget, but not table/object detail listings - if (this.isPK()) { - const nameField = _.find(this.table.fields, f => f.isEntityName()); - if (nameField) { - return nameField; - } + if (this.name_field) { + return this.name_field; } return null; } diff --git a/frontend/src/metabase/admin/settings/components/widgets/PublicLinksListing.jsx b/frontend/src/metabase/admin/settings/components/widgets/PublicLinksListing.jsx index 7378fa0f291e1cda6b748bd68e78799e155d498d..6d92dcd7c341edcd26c7e9d35dfd4435e96adcf0 100644 --- a/frontend/src/metabase/admin/settings/components/widgets/PublicLinksListing.jsx +++ b/frontend/src/metabase/admin/settings/components/widgets/PublicLinksListing.jsx @@ -159,7 +159,7 @@ export const PublicLinksQuestionListing = () => ( revoke={CardApi.deletePublicLink} type={t`Public Card Listing`} getUrl={({ id }) => Urls.question(id)} - getPublicUrl={({ public_uuid }) => Urls.publicCard(public_uuid)} + getPublicUrl={({ public_uuid }) => Urls.publicQuestion(public_uuid)} noLinksMessage={t`No questions have been publicly shared yet.`} /> ); diff --git a/frontend/src/metabase/components/Select.jsx b/frontend/src/metabase/components/Select.jsx index a2a138b068aa3e7f1a790f4433af2e269d7bb80e..4047f77e659f9fb30bc74e16b61a63e4b5d279ef 100644 --- a/frontend/src/metabase/components/Select.jsx +++ b/frontend/src/metabase/components/Select.jsx @@ -132,7 +132,7 @@ class BrowserSelect extends Component { ) } triggerClasses={className} - verticalAttachments={["top"]} + verticalAttachments={["top", "bottom"]} isInitiallyOpen={isInitiallyOpen} {...extraProps} > diff --git a/frontend/src/metabase/dashboard/dashboard.js b/frontend/src/metabase/dashboard/dashboard.js index 1aa7a42c607c8f7a6a6d0317cb5f406c5860d923..8c3056784298b23ef17a18324c0c6735879d2061 100644 --- a/frontend/src/metabase/dashboard/dashboard.js +++ b/frontend/src/metabase/dashboard/dashboard.js @@ -33,7 +33,11 @@ import Utils from "metabase/lib/utils"; import { getPositionForNewDashCard } from "metabase/lib/dashboard_grid"; import { createCard } from "metabase/lib/card"; -import { addParamValues, fetchDatabaseMetadata } from "metabase/redux/metadata"; +import { + addParamValues, + addFields, + fetchDatabaseMetadata, +} from "metabase/redux/metadata"; import { push } from "react-router-redux"; import { @@ -551,6 +555,9 @@ export const fetchDashboard = createThunkAction(FETCH_DASHBOARD, function( if (result.param_values) { dispatch(addParamValues(result.param_values)); } + if (result.param_fields) { + dispatch(addFields(result.param_fields)); + } return { ...normalize(result, dashboard), // includes `result` and `entities` diff --git a/frontend/src/metabase/lib/api.js b/frontend/src/metabase/lib/api.js index 78c4a0dd53a8d0cbf7ba7f8f410760a13fcfde1b..5a6c2361e2b106892345f6691e3b9cda3eab8a8a 100644 --- a/frontend/src/metabase/lib/api.js +++ b/frontend/src/metabase/lib/api.js @@ -6,12 +6,12 @@ import EventEmitter from "events"; type TransformFn = (o: any) => any; -type Options = { +export type Options = { noEvent?: boolean, transformResponse?: TransformFn, cancelled?: Promise<any>, }; -type Data = { +export type Data = { [key: string]: any, }; diff --git a/frontend/src/metabase/lib/urls.js b/frontend/src/metabase/lib/urls.js index a3d7095645467dbd621ec4aa8525414e6001f18f..7885dd94b84ae835e875b0ac199ff2e4a4f8c17f 100644 --- a/frontend/src/metabase/lib/urls.js +++ b/frontend/src/metabase/lib/urls.js @@ -83,7 +83,7 @@ export function label(label) { return `/questions/search?label=${encodeURIComponent(label.slug)}`; } -export function publicCard(uuid, type = null) { +export function publicQuestion(uuid, type = null) { const siteUrl = MetabaseSettings.get("site_url"); return `${siteUrl}/public/question/${uuid}` + (type ? `.${type}` : ``); } @@ -96,3 +96,7 @@ export function publicDashboard(uuid) { export function embedCard(token, type = null) { return `/embed/question/${token}` + (type ? `.${type}` : ``); } + +export function embedDashboard(token) { + return `/embed/dashboard/${token}`; +} diff --git a/frontend/src/metabase/public/containers/PublicDashboard.jsx b/frontend/src/metabase/public/containers/PublicDashboard.jsx index 88a1940cdc3743a89f8d3f8a3f4ba32a96691153..ca4bd424a5e043e5693bd65d2f2f73af71a7af1a 100644 --- a/frontend/src/metabase/public/containers/PublicDashboard.jsx +++ b/frontend/src/metabase/public/containers/PublicDashboard.jsx @@ -26,6 +26,11 @@ import { import * as dashboardActions from "metabase/dashboard/dashboard"; +import { + setPublicDashboardEndpoints, + setEmbedDashboardEndpoints, +} from "metabase/services"; + import type { Dashboard } from "metabase/meta/types/Dashboard"; import type { Parameter } from "metabase/meta/types/Parameter"; @@ -89,6 +94,13 @@ export default class PublicDashboard extends Component { location, params: { uuid, token }, } = this.props; + + if (uuid) { + setPublicDashboardEndpoints(uuid); + } else if (token) { + setEmbedDashboardEndpoints(token); + } + initialize(); try { // $FlowFixMe diff --git a/frontend/src/metabase/public/containers/PublicQuestion.jsx b/frontend/src/metabase/public/containers/PublicQuestion.jsx index 248eac3053a09c316e4be71842f38e6ee87dfb4f..42b59077d79a315b3be7250bc85cbc246b53e76c 100644 --- a/frontend/src/metabase/public/containers/PublicQuestion.jsx +++ b/frontend/src/metabase/public/containers/PublicQuestion.jsx @@ -20,10 +20,15 @@ import { applyParameters, } from "metabase/meta/Card"; -import { PublicApi, EmbedApi } from "metabase/services"; +import { + PublicApi, + EmbedApi, + setPublicQuestionEndpoints, + setEmbedQuestionEndpoints, +} from "metabase/services"; import { setErrorPage } from "metabase/redux/app"; -import { addParamValues } from "metabase/redux/metadata"; +import { addParamValues, addFields } from "metabase/redux/metadata"; import { updateIn } from "icepick"; @@ -34,6 +39,7 @@ type Props = { height: number, setErrorPage: (error: { status: number }) => void, addParamValues: any => void, + addFields: any => void, }; type State = { @@ -45,6 +51,7 @@ type State = { const mapDispatchToProps = { setErrorPage, addParamValues, + addFields, }; @connect(null, mapDispatchToProps) @@ -69,6 +76,13 @@ export default class PublicQuestion extends Component { params: { uuid, token }, location: { query }, } = this.props; + + if (uuid) { + setPublicQuestionEndpoints(uuid); + } else if (token) { + setEmbedQuestionEndpoints(token); + } + try { let card; if (token) { @@ -82,6 +96,9 @@ export default class PublicQuestion extends Component { if (card.param_values) { this.props.addParamValues(card.param_values); } + if (card.param_fields) { + this.props.addFields(card.param_fields); + } let parameterValues: ParameterValues = {}; for (let parameter of getParameters(card)) { diff --git a/frontend/src/metabase/query_builder/components/QueryDownloadWidget.jsx b/frontend/src/metabase/query_builder/components/QueryDownloadWidget.jsx index aeed7f18dc541ea6487bca45792a9aff6d4d9448..29caa19ef248d3e23e938291ed6d8ece18ffffd8 100644 --- a/frontend/src/metabase/query_builder/components/QueryDownloadWidget.jsx +++ b/frontend/src/metabase/query_builder/components/QueryDownloadWidget.jsx @@ -118,7 +118,7 @@ const PublicQueryButton = ({ <DownloadButton className={className} method="GET" - url={Urls.publicCard(uuid, type)} + url={Urls.publicQuestion(uuid, type)} params={{ parameters: JSON.stringify(json_query.parameters) }} extensions={[type]} > diff --git a/frontend/src/metabase/query_builder/components/__mocks__/NativeQueryEditor.jsx b/frontend/src/metabase/query_builder/components/__mocks__/NativeQueryEditor.jsx new file mode 100644 index 0000000000000000000000000000000000000000..e98c274a1a0b5f47e6cb4279e6ae0c706cf37192 --- /dev/null +++ b/frontend/src/metabase/query_builder/components/__mocks__/NativeQueryEditor.jsx @@ -0,0 +1,15 @@ +import React from "react"; + +import Parameters from "metabase/parameters/components/Parameters"; + +const MockNativeQueryEditor = ({ location, query, setParameterValue }) => ( + <Parameters + parameters={query.question().parameters()} + query={location.query} + setParameterValue={setParameterValue} + syncQueryString + isQB + commitImmediately + /> +); +export default MockNativeQueryEditor; diff --git a/frontend/src/metabase/query_builder/containers/QuestionEmbedWidget.jsx b/frontend/src/metabase/query_builder/containers/QuestionEmbedWidget.jsx index 8cad882d28b119e1fadfef91d763387d2b434229..bc6376144dbc9f04d96144d1c6046ecfa39910a0 100644 --- a/frontend/src/metabase/query_builder/containers/QuestionEmbedWidget.jsx +++ b/frontend/src/metabase/query_builder/containers/QuestionEmbedWidget.jsx @@ -50,7 +50,7 @@ export default class QuestionEmbedWidget extends Component { updateEmbeddingParams(card, embeddingParams) } getPublicUrl={({ public_uuid }, extension) => - Urls.publicCard(public_uuid, extension) + Urls.publicQuestion(public_uuid, extension) } extensions={["csv", "xlsx", "json"]} /> diff --git a/frontend/src/metabase/redux/metadata.js b/frontend/src/metabase/redux/metadata.js index 6c7016151658d8629fdd137ccdcb581cd2162f09..806e05c32e46d5118c771e40db0e9fdaed6e665f 100644 --- a/frontend/src/metabase/redux/metadata.js +++ b/frontend/src/metabase/redux/metadata.js @@ -355,7 +355,8 @@ export const fetchField = createThunkAction(FETCH_FIELD, function( return async function(dispatch, getState) { const requestStatePath = ["metadata", "fields", fieldId]; const existingStatePath = requestStatePath; - const getData = () => MetabaseApi.field_get({ fieldId }); + const getData = async () => + normalize(await MetabaseApi.field_get({ fieldId }), FieldSchema); return await fetchData({ dispatch, @@ -420,6 +421,11 @@ export const updateFieldValues = createThunkAction( export const ADD_PARAM_VALUES = "metabase/metadata/ADD_PARAM_VALUES"; export const addParamValues = createAction(ADD_PARAM_VALUES); +export const ADD_FIELDS = "metabase/metadata/ADD_FIELDS"; +export const addFields = createAction(ADD_FIELDS, fields => { + return normalize(fields, [FieldSchema]); +}); + export const UPDATE_FIELD = "metabase/metadata/UPDATE_FIELD"; export const updateField = createThunkAction(UPDATE_FIELD, function(field) { return async function(dispatch, getState) { @@ -689,15 +695,6 @@ const tables = handleActions({}, {}); const fields = handleActions( { - [FETCH_FIELD]: { - next: (state, { payload: field }) => ({ - ...state, - [field.id]: { - ...(state[field.id] || {}), - ...field, - }, - }), - }, [FETCH_FIELD_VALUES]: { next: (state, { payload: fieldValues }) => fieldValues diff --git a/frontend/src/metabase/schema.js b/frontend/src/metabase/schema.js index 2a2b61874945e6cd547b04fcbb6bac8a471a16fe..656b8141eed80b1034d9194007b5e10305c997c7 100644 --- a/frontend/src/metabase/schema.js +++ b/frontend/src/metabase/schema.js @@ -22,6 +22,10 @@ TableSchema.define({ FieldSchema.define({ target: FieldSchema, table: TableSchema, + name_field: FieldSchema, + dimensions: { + human_readable_field: FieldSchema, + }, }); SegmentSchema.define({ diff --git a/frontend/src/metabase/selectors/metadata.js b/frontend/src/metabase/selectors/metadata.js index acb3dce8da15f26be3c9d25b9def6748fe2678af..492529a3619d37778ca954fc709ed39d8b0ec905 100644 --- a/frontend/src/metabase/selectors/metadata.js +++ b/frontend/src/metabase/selectors/metadata.js @@ -84,13 +84,20 @@ export const getMetadata = createSelector( hydrateList(meta.tables, "segments", meta.segments); hydrateList(meta.tables, "metrics", meta.metrics); - hydrate(meta.tables, "db", t => meta.databases[t.db_id || t.db]); - - hydrate(meta.segments, "table", s => meta.tables[s.table_id]); - hydrate(meta.metrics, "table", m => meta.tables[m.table_id]); - hydrate(meta.fields, "table", f => meta.tables[f.table_id]); - - hydrate(meta.fields, "target", f => meta.fields[f.fk_target_field_id]); + hydrate(meta.tables, "db", t => meta.database(t.db_id || t.db)); + + hydrate(meta.segments, "table", s => meta.table(s.table_id)); + hydrate(meta.metrics, "table", m => meta.table(m.table_id)); + hydrate(meta.fields, "table", f => meta.table(f.table_id)); + + hydrate(meta.fields, "target", f => meta.field(f.fk_target_field_id)); + hydrate(meta.fields, "name_field", f => { + if (f.name_field != null) { + return meta.field(f.name_field); + } else if (f.table && f.isPK()) { + return _.find(f.table.fields, f => f.isEntityName()); + } + }); hydrate(meta.fields, "operators", f => getOperators(f, f.table)); hydrate(meta.tables, "aggregation_options", t => @@ -221,10 +228,14 @@ export const makeGetMergedParameterFieldValues = () => { export function copyObjects(metadata, objects, Klass) { let copies = {}; for (const object of Object.values(objects)) { - // $FlowFixMe - copies[object.id] = new Klass(object); - // $FlowFixMe - copies[object.id].metadata = metadata; + if (object && object.id != null) { + // $FlowFixMe + copies[object.id] = new Klass(object); + // $FlowFixMe + copies[object.id].metadata = metadata; + } else { + console.warn("Missing id:", object); + } } return copies; } diff --git a/frontend/src/metabase/services.js b/frontend/src/metabase/services.js index d1922d8d1098df3c62de49d8e33da304b74dbe8b..f63510cecd33deb3d6b54d7d520d03f543e55dc6 100644 --- a/frontend/src/metabase/services.js +++ b/frontend/src/metabase/services.js @@ -11,6 +11,8 @@ const embedBase = IS_EMBED_PREVIEW ? "/api/preview_embed" : "/api/embed"; // $FlowFixMe: Flow doesn't understand webpack loader syntax import getGAMetadata from "promise-loader?global!metabase/lib/ga-metadata"; // eslint-disable-line import/default +import type { Data, Options } from "metabase/lib/api"; + export const ActivityApi = { list: GET("/api/activity"), recent_views: GET("/api/activity/recent_views"), @@ -315,4 +317,41 @@ export const I18NApi = { locale: GET("/app/locales/:locale.json"), }; +export function setPublicQuestionEndpoints(uuid: string) { + setFieldEndpoints("/api/public/card/:uuid", { uuid }); +} +export function setPublicDashboardEndpoints(uuid: string) { + setFieldEndpoints("/api/public/dashboard/:uuid", { uuid }); +} +export function setEmbedQuestionEndpoints(token: string) { + if (!IS_EMBED_PREVIEW) { + setFieldEndpoints("/api/embed/card/:token", { token }); + } +} +export function setEmbedDashboardEndpoints(token: string) { + if (!IS_EMBED_PREVIEW) { + setFieldEndpoints("/api/embed/dashboard/:token", { token }); + } +} + +function GET_with(url: string, params: Data) { + return (data: Data, options?: Options) => + GET(url)({ ...params, ...data }, options); +} + +export function setFieldEndpoints(prefix: string, params: Data) { + MetabaseApi.field_values = GET_with( + prefix + "/field/:fieldId/values", + params, + ); + MetabaseApi.field_search = GET_with( + prefix + "/field/:fieldId/search/:searchFieldId", + params, + ); + MetabaseApi.field_remapping = GET_with( + prefix + "/field/:fieldId/remapping/:remappedFieldId", + params, + ); +} + global.services = exports; diff --git a/frontend/test/__support__/integrated_tests.js b/frontend/test/__support__/integrated_tests.js index 5514281e3f13fcd7c7d646923ab19a3dabff0a64..ae0cefb8d525196cbbcea37e2d0859d2bfa2ebf5 100644 --- a/frontend/test/__support__/integrated_tests.js +++ b/frontend/test/__support__/integrated_tests.js @@ -10,6 +10,7 @@ import "./mocks"; import { format as urlFormat } from "url"; import api from "metabase/lib/api"; +import { defer } from "metabase/lib/promise"; import { DashboardApi, SessionApi } from "metabase/services"; import { METABASE_SESSION_COOKIE } from "metabase/lib/cookies"; import normalReducers from "metabase/reducers-main"; @@ -438,6 +439,17 @@ export const waitForRequestToComplete = ( }); }; +export const waitForAllRequestsToComplete = () => { + if (pendingRequests > 0) { + if (!pendingRequestsDeferred) { + pendingRequestsDeferred = defer(); + } + return pendingRequestsDeferred.promise; + } else { + return Promise.resolve(); + } +}; + /** * Lets you replace given API endpoints with mocked implementations for the lifetime of a test */ @@ -475,68 +487,82 @@ export async function withApiMocks(mocks, test) { } } +let pendingRequests = 0; +let pendingRequestsDeferred = null; + // Patches the metabase/lib/api module so that all API queries contain the login credential cookie. // Needed because we are not in a real web browser environment. api._makeRequest = async (method, url, headers, requestBody, data, options) => { - const headersWithSessionCookie = { - ...headers, - ...(loginSession - ? { Cookie: `${METABASE_SESSION_COOKIE}=${loginSession.id}` } - : {}), - }; + pendingRequests++; + try { + const headersWithSessionCookie = { + ...headers, + ...(loginSession + ? { Cookie: `${METABASE_SESSION_COOKIE}=${loginSession.id}` } + : {}), + }; - const fetchOptions = { - credentials: "include", - method, - headers: new Headers(headersWithSessionCookie), - ...(requestBody ? { body: requestBody } : {}), - }; + const fetchOptions = { + credentials: "include", + method, + headers: new Headers(headersWithSessionCookie), + ...(requestBody ? { body: requestBody } : {}), + }; - let isCancelled = false; - if (options.cancelled) { - options.cancelled.then(() => { - isCancelled = true; - }); - } - const result = simulateOfflineMode - ? { status: 0, responseText: "" } - : await fetch(api.basename + url, fetchOptions); + let isCancelled = false; + if (options.cancelled) { + options.cancelled.then(() => { + isCancelled = true; + }); + } + const result = simulateOfflineMode + ? { status: 0, responseText: "" } + : await fetch(api.basename + url, fetchOptions); - if (isCancelled) { - throw { status: 0, data: "", isCancelled: true }; - } + if (isCancelled) { + throw { status: 0, data: "", isCancelled: true }; + } - let resultBody = null; - try { - resultBody = await result.text(); - // Even if the result conversion to JSON fails, we still return the original text - // This is 1-to-1 with the real _makeRequest implementation - resultBody = JSON.parse(resultBody); - } catch (e) {} - - apiRequestCompletedCallback && - setTimeout(() => apiRequestCompletedCallback(method, url), 0); - - if (result.status >= 200 && result.status <= 299) { - if (options.transformResponse) { - return options.transformResponse(resultBody, { data }); + let resultBody = null; + try { + resultBody = await result.text(); + // Even if the result conversion to JSON fails, we still return the original text + // This is 1-to-1 with the real _makeRequest implementation + resultBody = JSON.parse(resultBody); + } catch (e) {} + + apiRequestCompletedCallback && + setTimeout(() => apiRequestCompletedCallback(method, url), 0); + + if (result.status >= 200 && result.status <= 299) { + if (options.transformResponse) { + return options.transformResponse(resultBody, { data }); + } else { + return resultBody; + } } else { - return resultBody; + const error = { + status: result.status, + data: resultBody, + isCancelled: false, + }; + if (!simulateOfflineMode) { + console.log( + "A request made in a test failed with the following error:", + ); + console.log(error, { depth: null }); + console.log(`The original request: ${method} ${url}`); + if (requestBody) console.log(`Original payload: ${requestBody}`); + } + + throw error; } - } else { - const error = { - status: result.status, - data: resultBody, - isCancelled: false, - }; - if (!simulateOfflineMode) { - console.log("A request made in a test failed with the following error:"); - console.log(error, { depth: null }); - console.log(`The original request: ${method} ${url}`); - if (requestBody) console.log(`Original payload: ${requestBody}`); + } finally { + pendingRequests--; + if (pendingRequests === 0 && pendingRequestsDeferred) { + process.nextTick(pendingRequestsDeferred.resolve); + pendingRequestsDeferred = null; } - - throw error; } }; diff --git a/frontend/test/__support__/mocks.js b/frontend/test/__support__/mocks.js index 9dbe9c99289643f62be3e0e4afb1609244f50af9..e79230113f75503891ae2771ba05e0dd3536b918 100644 --- a/frontend/test/__support__/mocks.js +++ b/frontend/test/__support__/mocks.js @@ -46,8 +46,6 @@ bodyComponent.default = bodyComponent.TestBodyComponent; import * as table from "metabase/visualizations/visualizations/Table"; table.default = table.TestTable; -jest.mock("metabase/hoc/Remapped"); - // Replace addEventListener with a test implementation which collects all event listeners to `eventListeners` map export let eventListeners = {}; const testAddEventListener = jest.fn((event, listener) => { diff --git a/frontend/test/parameters/parameters.integ.spec.js b/frontend/test/parameters/parameters.integ.spec.js index fc6c91fbac86169604884f213e5f91fefae6c5b3..697217469733d60ed05a636caa753300f52a4169 100644 --- a/frontend/test/parameters/parameters.integ.spec.js +++ b/frontend/test/parameters/parameters.integ.spec.js @@ -1,370 +1,480 @@ -// Converted from an old Selenium E2E test +jest.mock("metabase/query_builder/components/NativeQueryEditor"); + +import { mount } from "enzyme"; + import { + createSavedQuestion, + createDashboard, + createTestStore, useSharedAdminLogin, logout, - createTestStore, - restorePreviousLogin, waitForRequestToComplete, + waitForAllRequestsToComplete, } from "__support__/integrated_tests"; -import { click, clickButton, setInputValue } from "__support__/enzyme_utils"; -import { mount } from "enzyme"; +import jwt from "jsonwebtoken"; -import { LOAD_CURRENT_USER } from "metabase/redux/user"; -import { - INITIALIZE_SETTINGS, - UPDATE_SETTING, - updateSetting, -} from "metabase/admin/settings/settings"; -import SettingToggle from "metabase/admin/settings/components/widgets/SettingToggle"; -import Toggle from "metabase/components/Toggle"; -import EmbeddingLegalese from "metabase/admin/settings/components/widgets/EmbeddingLegalese"; -import { - CREATE_PUBLIC_LINK, - INITIALIZE_QB, - API_CREATE_QUESTION, - QUERY_COMPLETED, - RUN_QUERY, - SET_QUERY_MODE, - setDatasetQuery, - UPDATE_EMBEDDING_PARAMS, - UPDATE_ENABLE_EMBEDDING, - UPDATE_TEMPLATE_TAG, -} from "metabase/query_builder/actions"; -import NativeQueryEditor from "metabase/query_builder/components/NativeQueryEditor"; -import { delay } from "metabase/lib/promise"; -import TagEditorSidebar from "metabase/query_builder/components/template_tags/TagEditorSidebar"; -import { getQuery } from "metabase/query_builder/selectors"; -import { - ADD_PARAM_VALUES, - FETCH_TABLE_METADATA, -} from "metabase/redux/metadata"; -import RunButton from "metabase/query_builder/components/RunButton"; -import Scalar from "metabase/visualizations/visualizations/Scalar"; -import Parameters from "metabase/parameters/components/Parameters"; -import CategoryWidget from "metabase/parameters/components/widgets/CategoryWidget"; +import { FETCH_DASHBOARD } from "metabase/dashboard/dashboard"; +import { fetchTableMetadata } from "metabase/redux/metadata"; +import { getMetadata } from "metabase/selectors/metadata"; + +import ParameterWidget from "metabase/parameters/components/ParameterWidget"; +import FieldValuesWidget from "metabase/components/FieldValuesWidget"; import ParameterFieldWidget from "metabase/parameters/components/widgets/ParameterFieldWidget"; -import SaveQuestionModal from "metabase/containers/SaveQuestionModal"; -import { LOAD_COLLECTIONS } from "metabase/questions/collections"; -import SharingPane from "metabase/public/components/widgets/SharingPane"; -import { EmbedTitle } from "metabase/public/components/widgets/EmbedModalContent"; -import PreviewPane from "metabase/public/components/widgets/PreviewPane"; -import CopyWidget from "metabase/components/CopyWidget"; -import ListSearchField from "metabase/components/ListSearchField"; -import * as Urls from "metabase/lib/urls"; -import QuestionEmbedWidget from "metabase/query_builder/containers/QuestionEmbedWidget"; -import EmbedWidget from "metabase/public/components/widgets/EmbedWidget"; +import TokenField from "metabase/components/TokenField"; -async function updateQueryText(store, queryText) { - // We don't have Ace editor so we have to trigger the Redux action manually - const newDatasetQuery = getQuery(store.getState()) - .updateQueryText(queryText) - .datasetQuery(); +import * as Urls from "metabase/lib/urls"; +import Question from "metabase-lib/lib/Question"; - return store.dispatch(setDatasetQuery(newDatasetQuery)); -} +import { + CardApi, + DashboardApi, + SettingsApi, + MetabaseApi, +} from "metabase/services"; -const getRelativeUrlWithoutHash = url => - url.replace(/#.*$/, "").replace(/http:\/\/.*?\//, "/"); +const ORDER_USER_ID_FIELD_ID = 7; +const PEOPLE_ID_FIELD_ID = 13; +const PEOPLE_NAME_FIELD_ID = 16; +const PEOPLE_SOURCE_FIELD_ID = 18; -const COUNT_ALL = "200"; -const COUNT_DOOHICKEY = "51"; -const COUNT_GADGET = "47"; +const METABASE_SECRET_KEY = + "24134bd93e081773fb178e8e1abb4e8a973822f7e19c872bd92c8d5a122ef63f"; describe("parameters", () => { - beforeAll(async () => useSharedAdminLogin()); + let question, dashboard; - describe("questions", () => { - let publicUrl = null; - let embedUrl = null; + beforeAll(async () => { + useSharedAdminLogin(); - it("should allow users to enable public sharing", async () => { - const store = await createTestStore(); + // enable public sharing + await SettingsApi.put({ key: "enable-public-sharing", value: true }); + await SettingsApi.put({ key: "enable-embedding", value: true }); + await SettingsApi.put({ + key: "embedding-secret-key", + value: METABASE_SECRET_KEY, + }); - // load public sharing settings - store.pushPath("/admin/settings/public_sharing"); - const app = mount(store.getAppContainer()); + await MetabaseApi.field_dimension_update({ + fieldId: ORDER_USER_ID_FIELD_ID, + type: "external", + name: "User ID", + human_readable_field_id: PEOPLE_NAME_FIELD_ID, + }); - await store.waitForActions([LOAD_CURRENT_USER, INITIALIZE_SETTINGS]); + const store = await createTestStore(); + await store.dispatch(fetchTableMetadata(1)); + const metadata = getMetadata(store.getState()); + + let unsavedQuestion = Question.create({ + databaseId: 1, + metadata, + }) + .setDatasetQuery({ + type: "native", + database: 1, + native: { + query: + "SELECT COUNT(*) FROM people WHERE {{id}} AND {{name}} AND {{source}} /* AND {{user_id}} */", + template_tags: { + id: { + id: "1", + name: "id", + display_name: "ID", + type: "dimension", + dimension: ["field-id", PEOPLE_ID_FIELD_ID], + widget_type: "id", + }, + name: { + id: "2", + name: "name", + display_name: "Name", + type: "dimension", + dimension: ["field-id", PEOPLE_NAME_FIELD_ID], + widget_type: "category", + }, + source: { + id: "3", + name: "source", + display_name: "Source", + type: "dimension", + dimension: ["field-id", PEOPLE_SOURCE_FIELD_ID], + widget_type: "category", + }, + user_id: { + id: "4", + name: "user_id", + display_name: "User", + type: "dimension", + dimension: ["field-id", ORDER_USER_ID_FIELD_ID], + widget_type: "id", + }, + }, + }, + parameters: [], + }) + .setDisplay("scalar") + .setDisplayName("Test Question"); + question = await createSavedQuestion(unsavedQuestion); + + // create a dashboard + dashboard = await createDashboard({ + name: "Test Dashboard", + description: null, + parameters: [ + { name: "ID", slug: "id", id: "1", type: "id" }, + { name: "Name", slug: "name", id: "2", type: "category" }, + { name: "Source", slug: "source", id: "3", type: "category" }, + { name: "User", slug: "user_id", id: "4", type: "id" }, + ], + }); + const dashcard = await DashboardApi.addcard({ + dashId: dashboard.id, + cardId: question.id(), + }); + await DashboardApi.reposition_cards({ + dashId: dashboard.id, + cards: [ + { + id: dashcard.id, + card_id: question.id(), + row: 0, + col: 0, + sizeX: 4, + sizeY: 4, + series: [], + visualization_settings: {}, + parameter_mappings: [ + { + parameter_id: "1", + card_id: question.id(), + target: ["dimension", ["template-tag", "id"]], + }, + { + parameter_id: "2", + card_id: question.id(), + target: ["dimension", ["template-tag", "name"]], + }, + { + parameter_id: "3", + card_id: question.id(), + target: ["dimension", ["template-tag", "source"]], + }, + { + parameter_id: "4", + card_id: question.id(), + target: ["dimension", ["template-tag", "user_id"]], + }, + ], + }, + ], + }); + }); - // // if enabled, disable it so we're in a known state - // // TODO Atte Keinänen 8/9/17: This should be done with a direct API call in afterAll instead - const enabledToggleContainer = app.find(SettingToggle).first(); + describe("private questions", () => { + let app, store; + it("should be possible to view a private question", async () => { + useSharedAdminLogin(); - expect(enabledToggleContainer.text()).toBe("Disabled"); + store = await createTestStore(); + store.pushPath(Urls.question(question.id()) + "?id=1"); + app = mount(store.getAppContainer()); - // toggle it on - click(enabledToggleContainer.find(Toggle)); - await store.waitForActions([UPDATE_SETTING]); + await waitForRequestToComplete("GET", /^\/api\/card\/\d+/); + expect(app.find(".Header-title-name").text()).toEqual("Test Question"); - // make sure it's enabled - expect(enabledToggleContainer.text()).toBe("Enabled"); + // wait for the query to load + await waitForRequestToComplete("POST", /^\/api\/card\/\d+\/query/); }); + sharedParametersTests(() => ({ app, store })); + }); - it("should allow users to enable embedding", async () => { - const store = await createTestStore(); + describe("public questions", () => { + let app, store; + it("should be possible to view a public question", async () => { + useSharedAdminLogin(); + const publicQuestion = await CardApi.createPublicLink({ + id: question.id(), + }); - // load public sharing settings - store.pushPath("/admin/settings/embedding_in_other_applications"); - const app = mount(store.getAppContainer()); + logout(); - await store.waitForActions([LOAD_CURRENT_USER, INITIALIZE_SETTINGS]); + store = await createTestStore({ publicApp: true }); + store.pushPath(Urls.publicQuestion(publicQuestion.uuid) + "?id=1"); + app = mount(store.getAppContainer()); - click(app.find(EmbeddingLegalese).find('button[children="Enable"]')); - await store.waitForActions([UPDATE_SETTING]); + await waitForRequestToComplete("GET", /^\/api\/[^\/]*\/card/); + expect(app.find(".EmbedFrame-header .h4").text()).toEqual( + "Test Question", + ); - expect(app.find(EmbeddingLegalese).length).toBe(0); - const enabledToggleContainer = app.find(SettingToggle).first(); - expect(enabledToggleContainer.text()).toBe("Enabled"); + // wait for the query to load + await waitForRequestToComplete( + "GET", + /^\/api\/public\/card\/[^\/]+\/query/, + ); }); + sharedParametersTests(() => ({ app, store })); + }); - // Note: Test suite is sequential, so individual test cases can't be run individually - it("should allow users to create parameterized SQL questions", async () => { - // Don't render Ace editor in tests because it uses many DOM methods that aren't supported by jsdom - // NOTE Atte Keinänen 8/9/17: Ace provides a MockRenderer class which could be used for pseudo-rendering and - // testing Ace editor in tests, but it doesn't render stuff to DOM so I'm not sure how practical it would be - NativeQueryEditor.prototype.loadAceEditor = () => {}; + describe("embed questions", () => { + let app, store; + it("should be possible to view a embedded question", async () => { + useSharedAdminLogin(); + await CardApi.update({ + id: question.id(), + embedding_params: { + id: "enabled", + name: "enabled", + source: "enabled", + user_id: "enabled", + }, + enable_embedding: true, + }); - const store = await createTestStore(); + logout(); - // load public sharing settings - store.pushPath(Urls.plainQuestion()); - const app = mount(store.getAppContainer()); - await store.waitForActions([INITIALIZE_QB]); + const token = jwt.sign( + { + resource: { question: question.id() }, + params: {}, + }, + METABASE_SECRET_KEY, + ); - click(app.find(".Icon-sql")); - await store.waitForActions([SET_QUERY_MODE]); + store = await createTestStore({ embedApp: true }); + store.pushPath(Urls.embedCard(token) + "?id=1"); + app = mount(store.getAppContainer()); - await updateQueryText( - store, - "select count(*) from products where {{category}}", - ); + await waitForRequestToComplete("GET", /\/card\/[^\/]+/); - const tagEditorSidebar = app.find(TagEditorSidebar); - - const fieldFilterVarType = tagEditorSidebar - .find(".ColumnarSelector-row") - .at(3); - expect(fieldFilterVarType.text()).toBe("Field Filter"); - click(fieldFilterVarType); - - // there's an async error here for some reason - await store.waitForActions([UPDATE_TEMPLATE_TAG]); - - await delay(500); - - const productsRow = tagEditorSidebar - .find(".TestPopoverBody .List-section") - .at(4) - .find("a"); - expect(productsRow.text()).toBe("Products"); - click(productsRow); - - // Table fields should be loaded on-the-fly before showing the field selector - await store.waitForActions(FETCH_TABLE_METADATA); - // Needed due to state update after fetching metadata - await delay(100); - - const searchField = tagEditorSidebar - .find(".TestPopoverBody") - .find(ListSearchField) - .find("input") - .first(); - setInputValue(searchField, "cat"); - - const categoryRow = tagEditorSidebar - .find(".TestPopoverBody .List-section") - .at(2) - .find("a"); - expect(categoryRow.text()).toBe("Category"); - click(categoryRow); - - await store.waitForActions([UPDATE_TEMPLATE_TAG]); - - // close the template variable sidebar - click(tagEditorSidebar.find(".Icon-close")); - - // test without the parameter - click(app.find(RunButton)); - await store.waitForActions([RUN_QUERY, QUERY_COMPLETED]); - expect(app.find(Scalar).text()).toBe(COUNT_ALL); - - // test the parameter - const parameter = app.find(ParameterFieldWidget).first(); - click(parameter.find("div").first()); - click(parameter.find('span[children="Doohickey"]')); - clickButton(parameter.find(".Button")); - click(app.find(RunButton)); - await store.waitForActions([RUN_QUERY, QUERY_COMPLETED]); - expect(app.find(Scalar).text()).toBe(COUNT_DOOHICKEY); - - // save the question, required for public link/embedding - click( - app - .find(".Header-buttonSection a") - .first() - .find("a"), + expect(app.find(".EmbedFrame-header .h4").text()).toEqual( + "Test Question", ); - await store.waitForActions([LOAD_COLLECTIONS]); - setInputValue( - app.find(SaveQuestionModal).find("input[name='name']"), - "sql parametrized", + // wait for the query to load + await waitForRequestToComplete( + "GET", + /^\/api\/embed\/card\/[^\/]+\/query/, ); + }); + sharedParametersTests(() => ({ app, store })); + }); - clickButton( - app - .find(SaveQuestionModal) - .find("button") - .last(), - ); - await store.waitForActions([API_CREATE_QUESTION]); - await delay(100); - - click(app.find('#QuestionSavedModal .Button[children="Not now"]')); - // wait for modal to close :'( - await delay(500); - - // open sharing panel - click(app.find(QuestionEmbedWidget).find(EmbedWidget)); - - // "Embed this question in an application" - click( - app - .find(SharingPane) - .find("h3") - .last(), - ); + describe("private dashboards", () => { + let app, store; + it("should be possible to view a private dashboard", async () => { + useSharedAdminLogin(); - // make the parameter editable - click(app.find(".AdminSelect-content[children='Disabled']")); + store = await createTestStore(); + store.pushPath(Urls.dashboard(dashboard.id) + "?id=1"); + app = mount(store.getAppContainer()); + + await store.waitForActions([FETCH_DASHBOARD]); + expect(app.find(".DashboardHeader .Entity h2").text()).toEqual( + "Test Dashboard", + ); - click(app.find(".TestPopoverBody .Icon-pencil")); + // wait for the query to load + await waitForRequestToComplete("POST", /^\/api\/card\/[^\/]+\/query/); - await delay(500); + // wait for required field metadata to load + await waitForRequestToComplete("GET", /^\/api\/field\/[^\/]+/); + }); + sharedParametersTests(() => ({ app, store })); + }); - click(app.find("div[children='Publish']")); - await store.waitForActions([ - UPDATE_ENABLE_EMBEDDING, - UPDATE_EMBEDDING_PARAMS, - ]); + describe("public dashboards", () => { + let app, store; + it("should be possible to view a public dashboard", async () => { + useSharedAdminLogin(); + const publicDash = await DashboardApi.createPublicLink({ + id: dashboard.id, + }); - // save the embed url for next tests - embedUrl = getRelativeUrlWithoutHash( - app - .find(PreviewPane) - .find("iframe") - .prop("src"), - ); + logout(); - // back to main share panel - click(app.find(EmbedTitle)); + store = await createTestStore({ publicApp: true }); + store.pushPath(Urls.publicDashboard(publicDash.uuid) + "?id=1"); + app = mount(store.getAppContainer()); - // toggle public link on - click(app.find(SharingPane).find(Toggle)); - await store.waitForActions([CREATE_PUBLIC_LINK]); + await store.waitForActions([FETCH_DASHBOARD]); + expect(app.find(".EmbedFrame-header .h4").text()).toEqual( + "Test Dashboard", + ); - // save the public url for next tests - publicUrl = getRelativeUrlWithoutHash( - app - .find(CopyWidget) - .find("input") - .first() - .prop("value"), + // wait for the query to load + await waitForRequestToComplete( + "GET", + /^\/api\/public\/dashboard\/[^\/]+\/card\/[^\/]+/, ); }); + sharedParametersTests(() => ({ app, store })); + }); - describe("as an anonymous user", () => { - beforeAll(() => logout()); - - async function runSharedQuestionTests(store, questionUrl, apiRegex) { - store.pushPath(questionUrl); - const app = mount(store.getAppContainer()); - - await store.waitForActions([ADD_PARAM_VALUES]); - - // Loading the query results is done in PublicQuestion itself so we have to listen to API request instead of Redux action - await waitForRequestToComplete("GET", apiRegex); - // use `update()` because of setState - expect( - app - .update() - .find(Scalar) - .text(), - ).toBe(COUNT_ALL + "sql parametrized"); - - // manually click parameter (sadly the query results loading happens inline again) - click( - app - .find(Parameters) - .find("a") - .first(), - ); - click(app.find(CategoryWidget).find('li h4[children="Doohickey"]')); - clickButton(app.find(CategoryWidget).find(".Button")); - - await waitForRequestToComplete("GET", apiRegex); - expect( - app - .update() - .find(Scalar) - .text(), - ).toBe(COUNT_DOOHICKEY + "sql parametrized"); - - // set parameter via url - store.pushPath("/"); // simulate a page reload by visiting other page - store.pushPath(questionUrl + "?category=Gadget"); - await waitForRequestToComplete("GET", apiRegex); - // use `update()` because of setState - expect( - app - .update() - .find(Scalar) - .text(), - ).toBe(COUNT_GADGET + "sql parametrized"); - } - - it("should allow seeing an embedded question", async () => { - if (!embedUrl) - throw new Error( - "This test fails because previous tests didn't produce an embed url.", - ); - const embedUrlTestStore = await createTestStore({ embedApp: true }); - await runSharedQuestionTests( - embedUrlTestStore, - embedUrl, - new RegExp("/api/embed/card/.*/query"), - ); + describe("embed dashboards", () => { + let app, store; + it("should be possible to view a embed dashboard", async () => { + useSharedAdminLogin(); + await DashboardApi.update({ + id: dashboard.id, + embedding_params: { + id: "enabled", + name: "enabled", + source: "enabled", + user_id: "enabled", + }, + enable_embedding: true, }); - it("should allow seeing a public question", async () => { - if (!publicUrl) - throw new Error( - "This test fails because previous tests didn't produce a public url.", - ); - const publicUrlTestStore = await createTestStore({ publicApp: true }); - await runSharedQuestionTests( - publicUrlTestStore, - publicUrl, - new RegExp("/api/public/card/.*/query"), - ); - }); + logout(); - // I think it's cleanest to restore the login here so that there are no surprises if you want to add tests - // that expect that we're already logged in - afterAll(() => restorePreviousLogin()); - }); + const token = jwt.sign( + { + resource: { dashboard: dashboard.id }, + params: {}, + }, + METABASE_SECRET_KEY, + ); - afterAll(async () => { - const store = await createTestStore(); + store = await createTestStore({ embedApp: true }); + store.pushPath(Urls.embedDashboard(token) + "?id=1"); + app = mount(store.getAppContainer()); - // Disable public sharing and embedding after running tests - await store.dispatch( - updateSetting({ key: "enable-public-sharing", value: false }), + await store.waitForActions([FETCH_DASHBOARD]); + + expect(app.find(".EmbedFrame-header .h4").text()).toEqual( + "Test Dashboard", ); - await store.dispatch( - updateSetting({ key: "enable-embedding", value: false }), + + // wait for the query to load + await waitForRequestToComplete( + "GET", + /^\/api\/embed\/dashboard\/[^\/]+\/dashcard\/\d+\/card\/\d+/, ); }); + sharedParametersTests(() => ({ app, store })); + }); + + afterAll(async () => { + useSharedAdminLogin(); + // archive the dash so we don't impact other tests + await DashboardApi.update({ + id: dashboard.id, + archived: true, + }); + // archive the card so we don't impact other tests + await CardApi.update({ + id: question.id(), + archived: true, + }); + // do some cleanup so that we don't impact other tests + await SettingsApi.put({ key: "enable-public-sharing", value: false }); + await SettingsApi.put({ key: "enable-embedding", value: false }); + await MetabaseApi.field_dimension_delete({ + fieldId: ORDER_USER_ID_FIELD_ID, + }); }); }); + +async function sharedParametersTests(getAppAndStore) { + let app; + beforeEach(() => { + const info = getAppAndStore(); + app = info.app; + }); + + it("should have 4 ParameterFieldWidgets", async () => { + await waitForAllRequestsToComplete(); + + expect(app.find(ParameterWidget).length).toEqual(4); + expect(app.find(ParameterFieldWidget).length).toEqual(4); + }); + + it("open 4 FieldValuesWidgets", async () => { + // click each parameter to open the widget + app.find(ParameterFieldWidget).map(widget => widget.simulate("click")); + + const widgets = app.find(FieldValuesWidget); + expect(widgets.length).toEqual(4); + }); + + // it("should have the correct field and searchField", () => { + // const widgets = app.find(FieldValuesWidget); + // expect( + // widgets.map(widget => { + // const { field, searchField } = widget.props(); + // return [field && field.id, searchField && searchField.id]; + // }), + // ).toEqual([ + // [PEOPLE_ID_FIELD_ID, PEOPLE_NAME_FIELD_ID], + // [PEOPLE_NAME_FIELD_ID, PEOPLE_NAME_FIELD_ID], + // [PEOPLE_SOURCE_FIELD_ID, PEOPLE_SOURCE_FIELD_ID], + // [ORDER_USER_ID_FIELD_ID, PEOPLE_NAME_FIELD_ID], + // ]); + // }); + + it("should have the correct values", () => { + const widgets = app.find(FieldValuesWidget); + const values = widgets.map( + widget => + widget + .find("ul") // first ul is options + .at(0) + .find("li") + .map(li => li.text()) + .slice(0, -1), // the last item is the input, remove it + ); + expect(values).toEqual([ + ["Adelia Eichmann - 1"], // remapped value + [], + [], + [], + ]); + }); + + it("should have the correct placeholders", () => { + const widgets = app.find(FieldValuesWidget); + const placeholders = widgets.map( + widget => widget.find(TokenField).props().placeholder, + ); + expect(placeholders).toEqual([ + "Search by Name or enter an ID", + "Search by Name", + "Search the list", + "Search by Name or enter an ID", + ]); + }); + + it("should allow searching PEOPLE.ID by PEOPLE.NAME", async () => { + const widget = app.find(FieldValuesWidget).at(0); + // tests `search` endpoint + expect(widget.find("li").length).toEqual(1 + 1); + widget.find("input").simulate("change", { target: { value: "Aly" } }); + await waitForRequestToComplete("GET", /\/field\/.*\/search/); + expect(widget.find("li").length).toEqual(1 + 1 + 6); + }); + it("should allow searching PEOPLE.NAME by PEOPLE.NAME", async () => { + const widget = app.find(FieldValuesWidget).at(1); + // tests `search` endpoint + expect(widget.find("li").length).toEqual(1); + widget.find("input").simulate("change", { target: { value: "Aly" } }); + await waitForRequestToComplete("GET", /\/field\/.*\/search/); + expect(widget.find("li").length).toEqual(1 + 6); + }); + it("should show values for PEOPLE.SOURCE", async () => { + const widget = app.find(FieldValuesWidget).at(2); + // tests `values` endpoint + // NOTE: no need for waitForRequestToComplete because it was previously loaded? + // await waitForRequestToComplete("GET", /\/field\/.*\/values/); + expect(widget.find("li").length).toEqual(1 + 5); // 5 options + 1 for the input + }); + it("should allow searching ORDER.USER_ID by PEOPLE.NAME", async () => { + const widget = app.find(FieldValuesWidget).at(3); + // tests `search` endpoint + expect(widget.find("li").length).toEqual(1); + widget.find("input").simulate("change", { target: { value: "Aly" } }); + await waitForRequestToComplete("GET", /\/field\/.*\/search/); + expect(widget.find("li").length).toEqual(1 + 6); + }); +} diff --git a/frontend/test/public/public.integ.spec.js b/frontend/test/public/public.integ.spec.js index 576a74606795ed647c1d03e6b7693ae0c1f165aa..3021cb9eec6bb679917ecc5965c24924089b8852 100644 --- a/frontend/test/public/public.integ.spec.js +++ b/frontend/test/public/public.integ.spec.js @@ -1,62 +1,352 @@ -import { mount } from "enzyme"; - +// Converted from an old Selenium E2E test import { - createDashboard, - createTestStore, useSharedAdminLogin, + logout, + createTestStore, + restorePreviousLogin, + waitForRequestToComplete, } from "__support__/integrated_tests"; +import { click, clickButton, setInputValue } from "__support__/enzyme_utils"; -import { FETCH_DASHBOARD } from "metabase/dashboard/dashboard"; +import { mount } from "enzyme"; +import { LOAD_CURRENT_USER } from "metabase/redux/user"; +import { + INITIALIZE_SETTINGS, + UPDATE_SETTING, + updateSetting, +} from "metabase/admin/settings/settings"; +import SettingToggle from "metabase/admin/settings/components/widgets/SettingToggle"; +import Toggle from "metabase/components/Toggle"; +import EmbeddingLegalese from "metabase/admin/settings/components/widgets/EmbeddingLegalese"; +import { + CREATE_PUBLIC_LINK, + INITIALIZE_QB, + API_CREATE_QUESTION, + QUERY_COMPLETED, + RUN_QUERY, + SET_QUERY_MODE, + setDatasetQuery, + UPDATE_EMBEDDING_PARAMS, + UPDATE_ENABLE_EMBEDDING, + UPDATE_TEMPLATE_TAG, +} from "metabase/query_builder/actions"; +import NativeQueryEditor from "metabase/query_builder/components/NativeQueryEditor"; +import { delay } from "metabase/lib/promise"; +import TagEditorSidebar from "metabase/query_builder/components/template_tags/TagEditorSidebar"; +import { getQuery } from "metabase/query_builder/selectors"; +import { + ADD_PARAM_VALUES, + FETCH_TABLE_METADATA, +} from "metabase/redux/metadata"; +import RunButton from "metabase/query_builder/components/RunButton"; +import Scalar from "metabase/visualizations/visualizations/Scalar"; +import ParameterFieldWidget from "metabase/parameters/components/widgets/ParameterFieldWidget"; +import SaveQuestionModal from "metabase/containers/SaveQuestionModal"; +import { LOAD_COLLECTIONS } from "metabase/questions/collections"; +import SharingPane from "metabase/public/components/widgets/SharingPane"; +import { EmbedTitle } from "metabase/public/components/widgets/EmbedModalContent"; +import PreviewPane from "metabase/public/components/widgets/PreviewPane"; +import CopyWidget from "metabase/components/CopyWidget"; +import ListSearchField from "metabase/components/ListSearchField"; import * as Urls from "metabase/lib/urls"; +import QuestionEmbedWidget from "metabase/query_builder/containers/QuestionEmbedWidget"; +import EmbedWidget from "metabase/public/components/widgets/EmbedWidget"; -import { DashboardApi, SettingsApi } from "metabase/services"; +async function updateQueryText(store, queryText) { + // We don't have Ace editor so we have to trigger the Redux action manually + const newDatasetQuery = getQuery(store.getState()) + .updateQueryText(queryText) + .datasetQuery(); -describe("public pages", () => { - beforeAll(async () => { - // needed to create the public dash - useSharedAdminLogin(); - }); + return store.dispatch(setDatasetQuery(newDatasetQuery)); +} - describe("public dashboards", () => { - let dashboard, store, publicDash; +const getRelativeUrlWithoutHash = url => + url.replace(/#.*$/, "").replace(/http:\/\/.*?\//, "/"); - beforeAll(async () => { - store = await createTestStore(); +const COUNT_ALL = "200"; +const COUNT_DOOHICKEY = "51"; +const COUNT_GADGET = "47"; - // enable public sharing - await SettingsApi.put({ key: "enable-public-sharing", value: true }); +describe("public/embedded", () => { + beforeAll(async () => useSharedAdminLogin()); - // create a dashboard - dashboard = await createDashboard({ - name: "Test public dash", - description: "A dashboard for testing public things", - }); + describe("questions", () => { + let publicUrl = null; + let embedUrl = null; + + it("should allow users to enable public sharing", async () => { + const store = await createTestStore(); + + // load public sharing settings + store.pushPath("/admin/settings/public_sharing"); + const app = mount(store.getAppContainer()); + + await store.waitForActions([LOAD_CURRENT_USER, INITIALIZE_SETTINGS]); + + // // if enabled, disable it so we're in a known state + // // TODO Atte Keinänen 8/9/17: This should be done with a direct API call in afterAll instead + const enabledToggleContainer = app.find(SettingToggle).first(); + + expect(enabledToggleContainer.text()).toBe("Disabled"); - // create the public link for that dashboard - publicDash = await DashboardApi.createPublicLink({ id: dashboard.id }); + // toggle it on + click(enabledToggleContainer.find(Toggle)); + await store.waitForActions([UPDATE_SETTING]); + + // make sure it's enabled + expect(enabledToggleContainer.text()).toBe("Enabled"); }); - it("should be possible to view a public dashboard", async () => { - store.pushPath(Urls.publicDashboard(publicDash.uuid)); + it("should allow users to enable embedding", async () => { + const store = await createTestStore(); + // load public sharing settings + store.pushPath("/admin/settings/embedding_in_other_applications"); const app = mount(store.getAppContainer()); - await store.waitForActions([FETCH_DASHBOARD]); + await store.waitForActions([LOAD_CURRENT_USER, INITIALIZE_SETTINGS]); - const headerText = app.find(".EmbedFrame-header .h4").text(); + click(app.find(EmbeddingLegalese).find('button[children="Enable"]')); + await store.waitForActions([UPDATE_SETTING]); - expect(headerText).toEqual("Test public dash"); + expect(app.find(EmbeddingLegalese).length).toBe(0); + const enabledToggleContainer = app.find(SettingToggle).first(); + expect(enabledToggleContainer.text()).toBe("Enabled"); }); - afterAll(async () => { - // archive the dash so we don't impact other tests - await DashboardApi.update({ - id: dashboard.id, - archived: true, + // Note: Test suite is sequential, so individual test cases can't be run individually + it("should allow users to create parameterized SQL questions", async () => { + // Don't render Ace editor in tests because it uses many DOM methods that aren't supported by jsdom + // NOTE Atte Keinänen 8/9/17: Ace provides a MockRenderer class which could be used for pseudo-rendering and + // testing Ace editor in tests, but it doesn't render stuff to DOM so I'm not sure how practical it would be + NativeQueryEditor.prototype.loadAceEditor = () => {}; + + const store = await createTestStore(); + + // load public sharing settings + store.pushPath(Urls.plainQuestion()); + const app = mount(store.getAppContainer()); + await store.waitForActions([INITIALIZE_QB]); + + click(app.find(".Icon-sql")); + await store.waitForActions([SET_QUERY_MODE]); + + await updateQueryText( + store, + "select count(*) from products where {{category}}", + ); + + const tagEditorSidebar = app.find(TagEditorSidebar); + + const fieldFilterVarType = tagEditorSidebar + .find(".ColumnarSelector-row") + .at(3); + expect(fieldFilterVarType.text()).toBe("Field Filter"); + click(fieldFilterVarType); + + // there's an async error here for some reason + await store.waitForActions([UPDATE_TEMPLATE_TAG]); + + await delay(500); + + const productsRow = tagEditorSidebar + .find(".TestPopoverBody .List-section") + .at(4) + .find("a"); + expect(productsRow.text()).toBe("Products"); + click(productsRow); + + // Table fields should be loaded on-the-fly before showing the field selector + await store.waitForActions(FETCH_TABLE_METADATA); + // Needed due to state update after fetching metadata + await delay(100); + + const searchField = tagEditorSidebar + .find(".TestPopoverBody") + .find(ListSearchField) + .find("input") + .first(); + setInputValue(searchField, "cat"); + + const categoryRow = tagEditorSidebar + .find(".TestPopoverBody .List-section") + .at(2) + .find("a"); + expect(categoryRow.text()).toBe("Category"); + click(categoryRow); + + await store.waitForActions([UPDATE_TEMPLATE_TAG]); + + // close the template variable sidebar + click(tagEditorSidebar.find(".Icon-close")); + + // test without the parameter + click(app.find(RunButton)); + await store.waitForActions([RUN_QUERY, QUERY_COMPLETED]); + expect(app.find(Scalar).text()).toBe(COUNT_ALL); + + // test the parameter + const parameter = app.find(ParameterFieldWidget).first(); + click(parameter.find("div").first()); + click(parameter.find('span[children="Doohickey"]')); + clickButton(parameter.find(".Button")); + click(app.find(RunButton)); + await store.waitForActions([RUN_QUERY, QUERY_COMPLETED]); + expect(app.find(Scalar).text()).toBe(COUNT_DOOHICKEY); + + // save the question, required for public link/embedding + click( + app + .find(".Header-buttonSection a") + .first() + .find("a"), + ); + await store.waitForActions([LOAD_COLLECTIONS]); + + setInputValue( + app.find(SaveQuestionModal).find("input[name='name']"), + "sql parametrized", + ); + + clickButton( + app + .find(SaveQuestionModal) + .find("button") + .last(), + ); + await store.waitForActions([API_CREATE_QUESTION]); + await delay(100); + + click(app.find('#QuestionSavedModal .Button[children="Not now"]')); + // wait for modal to close :'( + await delay(500); + + // open sharing panel + click(app.find(QuestionEmbedWidget).find(EmbedWidget)); + + // "Embed this question in an application" + click( + app + .find(SharingPane) + .find("h3") + .last(), + ); + + // make the parameter editable + click(app.find(".AdminSelect-content[children='Disabled']")); + + click(app.find(".TestPopoverBody .Icon-pencil")); + + await delay(500); + + click(app.find("div[children='Publish']")); + await store.waitForActions([ + UPDATE_ENABLE_EMBEDDING, + UPDATE_EMBEDDING_PARAMS, + ]); + + // save the embed url for next tests + embedUrl = getRelativeUrlWithoutHash( + app + .find(PreviewPane) + .find("iframe") + .prop("src"), + ); + + // back to main share panel + click(app.find(EmbedTitle)); + + // toggle public link on + click(app.find(SharingPane).find(Toggle)); + await store.waitForActions([CREATE_PUBLIC_LINK]); + + // save the public url for next tests + publicUrl = getRelativeUrlWithoutHash( + app + .find(CopyWidget) + .find("input") + .first() + .prop("value"), + ); + }); + + describe("as an anonymous user", () => { + beforeAll(() => logout()); + + async function runSharedQuestionTests(store, questionUrl, apiRegex) { + store.pushPath(questionUrl); + const app = mount(store.getAppContainer()); + + await store.waitForActions([ADD_PARAM_VALUES]); + + // Loading the query results is done in PublicQuestion itself so we have to listen to API request instead of Redux action + await waitForRequestToComplete("GET", apiRegex); + // use `update()` because of setState + expect( + app + .update() + .find(Scalar) + .text(), + ).toBe(COUNT_ALL + "sql parametrized"); + + // NOTE: parameters tests moved to parameters.integ.spec.js + + // set parameter via url + store.pushPath("/"); // simulate a page reload by visiting other page + store.pushPath(questionUrl + "?category=Gadget"); + await waitForRequestToComplete("GET", apiRegex); + // use `update()` because of setState + expect( + app + .update() + .find(Scalar) + .text(), + ).toBe(COUNT_GADGET + "sql parametrized"); + } + + it("should allow seeing an embedded question", async () => { + if (!embedUrl) + throw new Error( + "This test fails because previous tests didn't produce an embed url.", + ); + const embedUrlTestStore = await createTestStore({ embedApp: true }); + await runSharedQuestionTests( + embedUrlTestStore, + embedUrl, + new RegExp("/api/embed/card/.*/query"), + ); + }); + + it("should allow seeing a public question", async () => { + if (!publicUrl) + throw new Error( + "This test fails because previous tests didn't produce a public url.", + ); + const publicUrlTestStore = await createTestStore({ publicApp: true }); + await runSharedQuestionTests( + publicUrlTestStore, + publicUrl, + new RegExp("/api/public/card/.*/query"), + ); }); - // do some cleanup so that we don't impact other tests - await SettingsApi.put({ key: "enable-public-sharing", value: false }); + + // I think it's cleanest to restore the login here so that there are no surprises if you want to add tests + // that expect that we're already logged in + afterAll(() => restorePreviousLogin()); + }); + + afterAll(async () => { + const store = await createTestStore(); + + // Disable public sharing and embedding after running tests + await store.dispatch( + updateSetting({ key: "enable-public-sharing", value: false }), + ); + await store.dispatch( + updateSetting({ key: "enable-embedding", value: false }), + ); }); }); }); diff --git a/frontend/test/query_builder/components/FieldList.integ.spec.js b/frontend/test/query_builder/components/FieldList.integ.spec.js index 72257f3c0a86002e43b1af725c677cbfb93e736f..b73bdb6ec01c8f32fa16d77067f0f42e3ce7ec0a 100644 --- a/frontend/test/query_builder/components/FieldList.integ.spec.js +++ b/frontend/test/query_builder/components/FieldList.integ.spec.js @@ -1,3 +1,5 @@ +jest.mock("metabase/hoc/Remapped"); + // Important: import of integrated_tests always comes first in tests because of mocked modules import { createTestStore, diff --git a/src/metabase/api/embed.clj b/src/metabase/api/embed.clj index 31cb2449aa87dd34bfd8ce0c573474dec1f503ef..c96ced34d882d90944e784056d8d3bd413d5ebc2 100644 --- a/src/metabase/api/embed.clj +++ b/src/metabase/api/embed.clj @@ -343,4 +343,70 @@ :query-params query-params))) +;;; +----------------------------------------------------------------------------------------------------------------+ +;;; | FieldValues, Search, Remappings | +;;; +----------------------------------------------------------------------------------------------------------------+ + +;;; -------------------------------------------------- Field Values -------------------------------------------------- + +(api/defendpoint GET "/card/:token/field/:field-id/values" + "Fetch FieldValues for a Field that is referenced by an embedded Card." + [token field-id] + (let [unsigned-token (eu/unsign token) + card-id (eu/get-in-unsigned-token-or-throw unsigned-token [:resource :question])] + (check-embedding-enabled-for-card card-id) + (public-api/card-and-field-id->values card-id field-id))) + +(api/defendpoint GET "/dashboard/:token/field/:field-id/values" + "Fetch FieldValues for a Field that is used as a param in an embedded Dashboard." + [token field-id] + (let [unsigned-token (eu/unsign token) + dashboard-id (eu/get-in-unsigned-token-or-throw unsigned-token [:resource :dashboard])] + (check-embedding-enabled-for-dashboard dashboard-id) + (public-api/dashboard-and-field-id->values dashboard-id field-id))) + + +;;; --------------------------------------------------- Searching ---------------------------------------------------- + +(api/defendpoint GET "/card/:token/field/:field-id/search/:search-field-id" + "Search for values of a Field that is referenced by an embedded Card." + [token field-id search-field-id value limit] + {value su/NonBlankString + limit (s/maybe su/IntStringGreaterThanZero)} + (let [unsigned-token (eu/unsign token) + card-id (eu/get-in-unsigned-token-or-throw unsigned-token [:resource :question])] + (check-embedding-enabled-for-card card-id) + (public-api/search-card-fields card-id field-id search-field-id value (when limit (Integer/parseInt limit))))) + +(api/defendpoint GET "/dashboard/:token/field/:field-id/search/:search-field-id" + "Search for values of a Field that is referenced by a Card in an embedded Dashboard." + [token field-id search-field-id value limit] + {value su/NonBlankString + limit (s/maybe su/IntStringGreaterThanZero)} + (let [unsigned-token (eu/unsign token) + dashboard-id (eu/get-in-unsigned-token-or-throw unsigned-token [:resource :dashboard])] + (check-embedding-enabled-for-dashboard dashboard-id) + (public-api/search-dashboard-fields dashboard-id field-id search-field-id value (when limit + (Integer/parseInt limit))))) + + +;;; --------------------------------------------------- Remappings --------------------------------------------------- + +(api/defendpoint GET "/card/:token/field/:field-id/remapping/:remapped-id" + [token field-id remapped-id value] + {value su/NonBlankString} + (let [unsigned-token (eu/unsign token) + card-id (eu/get-in-unsigned-token-or-throw unsigned-token [:resource :question])] + (check-embedding-enabled-for-card card-id) + (public-api/card-field-remapped-values card-id field-id remapped-id value))) + +(api/defendpoint GET "/dashboard/:token/field/:field-id/remapping/:remapped-id" + [token field-id remapped-id value] + {value su/NonBlankString} + (let [unsigned-token (eu/unsign token) + dashboard-id (eu/get-in-unsigned-token-or-throw unsigned-token [:resource :dashboard])] + (check-embedding-enabled-for-dashboard dashboard-id) + (public-api/dashboard-field-remapped-values dashboard-id field-id remapped-id value))) + + (api/define-routes) diff --git a/src/metabase/api/field.clj b/src/metabase/api/field.clj index 5b700aef73f3ff24781acc19accf6f6affd4584f..baa1476d40305e3659b734e2ef843607533b7f2c 100644 --- a/src/metabase/api/field.clj +++ b/src/metabase/api/field.clj @@ -32,7 +32,7 @@ "Get `Field` with ID." [id] (-> (api/read-check Field id) - (hydrate [:table :db] :has_field_values))) + (hydrate [:table :db] :has_field_values :dimensions :name_field))) (defn- clear-dimension-on-fk-change! [{{dimension-id :id dimension-type :type} :dimensions :as field}] (when (and dimension-id (= :external dimension-type)) @@ -150,17 +150,23 @@ (def ^:private empty-field-values {:values []}) +(defn field->values + "Fetch FieldValues, if they exist, for a `field` and return them in an appropriate format for public/embedded + use-cases." + [field] + (api/check-404 field) + (if-let [field-values (and (field-values/field-should-have-field-values? field) + (field-values/create-field-values-if-needed! field))] + (-> field-values + (assoc :values (field-values/field-values->pairs field-values)) + (dissoc :human_readable_values :created_at :updated_at :id)) + {:values [], :field_id (:id field)})) + (api/defendpoint GET "/:id/values" "If `Field`'s special type derives from `type/Category`, or its base type is `type/Boolean`, return all distinct values of the field, and a map of human-readable values defined by the user." [id] - (let [field (api/read-check Field id)] - (if-let [field-values (and (field-values/field-should-have-field-values? field) - (field-values/create-field-values-if-needed! field))] - (-> field-values - (assoc :values (field-values/field-values->pairs field-values)) - (dissoc :human_readable_values)) - {:values []}))) + (field->values (api/read-check Field id))) ;; match things like GET /field-literal%2Ccreated_at%2Ctype%2FDatetime/values ;; (this is how things like [field-literal,created_at,type/Datetime] look when URL-encoded) @@ -254,7 +260,7 @@ field)) -(s/defn ^:private search-values +(s/defn search-values "Search for values of `search-field` that start with `value` (up to `limit`, if specified), and return like [<value-of-field> <matching-value-of-search-field>]. @@ -314,14 +320,20 @@ ;; return first row if it exists (first (get-in results [:data :rows])))) +(defn parse-query-param-value-for-field + "Parse a `value` passed as a URL query param in a way appropriate for the `field` it belongs to. E.g. for text Fields + the value doesn't need to be parsed; for numeric Fields we should parse it as a number." + [field, ^String value] + (if (isa? (:base_type field) :type/Number) + (.parse (NumberFormat/getInstance) value) + value)) + (api/defendpoint GET "/:id/remapping/:remapped-id" "Fetch remapped Field values." [id remapped-id, ^String value] (let [field (api/read-check Field id) remapped-field (api/read-check Field remapped-id) - value (if (isa? (:base_type field) :type/Number) - (.parse (NumberFormat/getInstance) value) - value)] + value (parse-query-param-value-for-field field value)] (remapped-value field remapped-field value))) diff --git a/src/metabase/api/public.clj b/src/metabase/api/public.clj index 5f8018a8d5f18da49933c42331cf0aa38a9a1639..3d665bdb476e860e15f4bb6a2efbfbc56767639f 100644 --- a/src/metabase/api/public.clj +++ b/src/metabase/api/public.clj @@ -1,22 +1,29 @@ (ns metabase.api.public "Metabase API endpoints for viewing publicly-accessible Cards and Dashboards." (:require [cheshire.core :as json] + [clojure.walk :as walk] [compojure.core :refer [GET]] [metabase + [db :as mdb] [query-processor :as qp] [util :as u]] [metabase.api [card :as card-api] [common :as api] [dataset :as dataset-api] - [dashboard :as dashboard-api]] + [dashboard :as dashboard-api] + [field :as field-api]] [metabase.models - [card :refer [Card]] + [card :refer [Card] :as card] [dashboard :refer [Dashboard]] [dashboard-card :refer [DashboardCard]] [dashboard-card-series :refer [DashboardCardSeries]] + [dimension :refer [Dimension]] + [field :refer [Field]] [field-values :refer [FieldValues]] [params :as params]] + [metabase.query-processor :as qp] + metabase.query-processor.interface ; because we refer to Field [metabase.util [embed :as embed] [schema :as su]] @@ -31,18 +38,21 @@ ;;; -------------------------------------------------- Public Cards -------------------------------------------------- -(defn- remove-card-non-public-fields +(defn- remove-card-non-public-columns "Remove everyting from public CARD that shouldn't be visible to the general public." [card] - (u/select-nested-keys card [:id :name :description :display :visualization_settings [:dataset_query :type [:native :template_tags]]])) + (card/map->CardInstance + (u/select-nested-keys card [:id :name :description :display :visualization_settings + [:dataset_query :type [:native :template_tags]]]))) (defn public-card - "Return a public Card matching key-value CONDITIONS, removing all fields that should not be visible to the general + "Return a public Card matching key-value CONDITIONS, removing all columns that should not be visible to the general public. Throws a 404 if the Card doesn't exist." [& conditions] - (-> (api/check-404 (apply db/select-one [Card :id :dataset_query :description :display :name :visualization_settings], :archived false, conditions)) - remove-card-non-public-fields - params/add-card-param-values)) + (-> (api/check-404 (apply db/select-one [Card :id :dataset_query :description :display :name :visualization_settings] + :archived false, conditions)) + remove-card-non-public-columns + (hydrate :param_values :param_fields))) (defn- card-with-uuid [uuid] (public-card :public_uuid uuid)) @@ -90,24 +100,24 @@ (dataset-api/as-format export-format (run-query-for-card-with-public-uuid uuid parameters, :constraints nil))) + ;;; ----------------------------------------------- Public Dashboards ------------------------------------------------ (defn public-dashboard - "Return a public Dashboard matching key-value CONDITIONS, removing all fields that should not be visible to the + "Return a public Dashboard matching key-value CONDITIONS, removing all columns that should not be visible to the general public. Throws a 404 if the Dashboard doesn't exist." [& conditions] (-> (api/check-404 (apply db/select-one [Dashboard :name :description :id :parameters], :archived false, conditions)) - (hydrate [:ordered_cards :card :series]) - params/add-field-values-for-parameters + (hydrate [:ordered_cards :card :series] :param_values :param_fields) dashboard-api/add-query-average-durations (update :ordered_cards (fn [dashcards] (for [dashcard dashcards] (-> (select-keys dashcard [:id :card :card_id :dashboard_id :series :col :row :sizeX :sizeY :parameter_mappings :visualization_settings]) - (update :card remove-card-non-public-fields) + (update :card remove-card-non-public-columns) (update :series (fn [series] (for [series series] - (remove-card-non-public-fields series)))))))))) + (remove-card-non-public-columns series)))))))))) (defn- dashboard-with-uuid [uuid] (public-dashboard :public_uuid uuid)) @@ -117,19 +127,25 @@ (api/check-public-sharing-enabled) (dashboard-with-uuid uuid)) +(defn- check-card-is-in-dashboard + "Check that the Card with `card-id` is in Dashboard with `dashboard-id`, either in a DashboardCard at the top level or + as a series, or throw an Exception. If not such relationship exists this will throw a 404 Exception." + [card-id dashboard-id] + (api/check-404 + (or (db/exists? DashboardCard + :dashboard_id dashboard-id + :card_id card-id) + (when-let [dashcard-ids (db/select-ids DashboardCard :dashboard_id dashboard-id)] + (db/exists? DashboardCardSeries + :card_id card-id + :dashboardcard_id [:in dashcard-ids]))))) (defn public-dashcard-results "Return the results of running a query with PARAMETERS for Card with CARD-ID belonging to Dashboard with DASHBOARD-ID. Throws a 404 if the Card isn't part of the Dashboard." [dashboard-id card-id parameters & {:keys [context] :or {context :public-dashboard}}] - (api/check-404 (or (db/exists? DashboardCard - :dashboard_id dashboard-id - :card_id card-id) - (when-let [dashcard-ids (db/select-ids DashboardCard :dashboard_id dashboard-id)] - (db/exists? DashboardCardSeries - :card_id card-id - :dashboardcard_id [:in dashcard-ids])))) + (check-card-is-in-dashboard card-id dashboard-id) (run-query-for-card-with-id card-id parameters, :context context, :dashboard-id dashboard-id)) (api/defendpoint GET "/dashboard/:uuid/card/:card-id" @@ -138,7 +154,8 @@ [uuid card-id parameters] {parameters (s/maybe su/JSONString)} (api/check-public-sharing-enabled) - (public-dashcard-results (api/check-404 (db/select-one-id Dashboard :public_uuid uuid, :archived false)) card-id parameters)) + (public-dashcard-results + (api/check-404 (db/select-one-id Dashboard :public_uuid uuid, :archived false)) card-id parameters)) (api/defendpoint GET "/oembed" @@ -159,4 +176,176 @@ :html (embed/iframe url width height)})) +;;; +----------------------------------------------------------------------------------------------------------------+ +;;; | FieldValues, Search, Remappings | +;;; +----------------------------------------------------------------------------------------------------------------+ + +;;; -------------------------------------------------- Field Values -------------------------------------------------- + +;; TODO - this is a stupid, inefficient way of doing things. Figure out a better way to do it. :( +(defn- query->referenced-field-ids + "Get the IDs of all Fields referenced by an MBQL `query` (not including any parameters)." + [query] + (let [field-ids (atom [])] + (walk/postwalk + (fn [x] + (if (instance? metabase.query_processor.interface.Field x) + (swap! field-ids conj (:field-id x)) + x)) + (qp/expand query)) + @field-ids)) + +(defn- card->referenced-field-ids + "Return a set of all Field IDs referenced by `card`, in both the MBQL query itself and in its parameters ('template + tags')." + [card] + (set (concat (query->referenced-field-ids (:dataset_query card)) + (params/card->template-tag-field-ids card)))) + +(defn- check-field-is-referenced-by-card + "Check to make sure the query for Card with `card-id` references Field with `field-id`. Otherwise, or if the Card + cannot be found, throw an Exception." + [field-id card-id] + (let [card (api/check-404 (db/select-one [Card :dataset_query] :id card-id)) + referenced-field-ids (card->referenced-field-ids card)] + (api/check-404 (contains? referenced-field-ids field-id)))) + +(defn- check-search-field-is-allowed + "Check whether a search Field is allowed to be used in conjunction with another Field. A search Field is allowed if + *any* of the following conditions is true: + + * `search-field-id` and `field-id` are both the same Field + * `search-field-id` is equal to the other Field's Dimension's `human-readable-field-id` + * field is a `:type/PK` Field and search field is a `:type/Name` Field belonging to the same Table. + + If none of these conditions are met, you are not allowed to use the search field in combination with the other + field, and an 400 exception will be thrown." + [field-id search-field-id] + {:pre [(integer? field-id) (integer? search-field-id)]} + (api/check-400 + (or (= field-id search-field-id) + (db/exists? Dimension :field_id field-id, :human_readable_field_id search-field-id) + ;; just do a couple small queries to figure this out, we could write a fancy query to join Field against itself + ;; and do this in one but the extra code complexity isn't worth it IMO + (when-let [table-id (db/select-one-field :table_id Field :id field-id, :special_type (mdb/isa :type/PK))] + (db/exists? Field :id search-field-id, :table_id table-id, :special_type (mdb/isa :type/Name)))))) + + +(defn- check-field-is-referenced-by-dashboard + "Check that `field-id` belongs to a Field that is used as a parameter in a Dashboard with `dashboard-id`, or throw a + 404 Exception." + [field-id dashboard-id] + (let [param-field-ids (params/dashboard->param-field-ids (api/check-404 (Dashboard dashboard-id)))] + (api/check-404 (contains? param-field-ids field-id)))) + +(defn card-and-field-id->values + "Return the FieldValues for a Field with `field-id` that is referenced by Card with `card-id`." + [card-id field-id] + (check-field-is-referenced-by-card field-id card-id) + (field-api/field->values (Field field-id))) + +(api/defendpoint GET "/card/:uuid/field/:field-id/values" + "Fetch FieldValues for a Field that is referenced by a public Card." + [uuid field-id] + (api/check-public-sharing-enabled) + (let [card-id (db/select-one-id Card :public_uuid uuid, :archived false)] + (card-and-field-id->values card-id field-id))) + +(defn dashboard-and-field-id->values + "Return the FieldValues for a Field with `field-id` that is referenced by Card with `card-id` which itself is present + in Dashboard with `dashboard-id`." + [dashboard-id field-id] + (check-field-is-referenced-by-dashboard field-id dashboard-id) + (field-api/field->values (Field field-id))) + +(api/defendpoint GET "/dashboard/:uuid/field/:field-id/values" + "Fetch FieldValues for a Field that is referenced by a Card in a public Dashboard." + [uuid field-id] + (api/check-public-sharing-enabled) + (let [dashboard-id (api/check-404 (db/select-one-id Dashboard :public_uuid uuid, :archived false))] + (dashboard-and-field-id->values dashboard-id field-id))) + + +;;; --------------------------------------------------- Searching ---------------------------------------------------- + +(defn search-card-fields + "Wrapper for `metabase.api.field/search-values` for use with public/embedded Cards. See that functions + documentation for a more detailed explanation of exactly what this does." + [card-id field-id search-id value limit] + (check-field-is-referenced-by-card field-id card-id) + (check-search-field-is-allowed field-id search-id) + (field-api/search-values (Field field-id) (Field search-id) value limit)) + +(defn search-dashboard-fields + "Wrapper for `metabase.api.field/search-values` for use with public/embedded Dashboards. See that functions + documentation for a more detailed explanation of exactly what this does." + [dashboard-id field-id search-id value limit] + (check-field-is-referenced-by-dashboard field-id dashboard-id) + (check-search-field-is-allowed field-id search-id) + (field-api/search-values (Field field-id) (Field search-id) value limit)) + +(api/defendpoint GET "/card/:uuid/field/:field-id/search/:search-field-id" + "Search for values of a Field that is referenced by a public Card." + [uuid field-id search-field-id value limit] + {value su/NonBlankString + limit (s/maybe su/IntStringGreaterThanZero)} + (api/check-public-sharing-enabled) + (let [card-id (db/select-one-id Card :public_uuid uuid, :archived false)] + (search-card-fields card-id field-id search-field-id value (when limit (Integer/parseInt limit))))) + +(api/defendpoint GET "/dashboard/:uuid/field/:field-id/search/:search-field-id" + "Search for values of a Field that is referenced by a Card in a public Dashboard." + [uuid field-id search-field-id value limit] + {value su/NonBlankString + limit (s/maybe su/IntStringGreaterThanZero)} + (api/check-public-sharing-enabled) + (let [dashboard-id (api/check-404 (db/select-one-id Dashboard :public_uuid uuid, :archived false))] + (search-dashboard-fields dashboard-id field-id search-field-id value (when limit (Integer/parseInt limit))))) + + +;;; --------------------------------------------------- Remappings --------------------------------------------------- + +(defn- field-remapped-values [field-id remapped-field-id, ^String value-str] + (let [field (api/check-404 (Field field-id)) + remapped-field (api/check-404 (Field remapped-field-id))] + (check-search-field-is-allowed field-id remapped-field-id) + (field-api/remapped-value field remapped-field (field-api/parse-query-param-value-for-field field value-str)))) + +(defn card-field-remapped-values + "Return the reampped Field values for a Field referenced by a *Card*. This explanation is almost useless, so see the + one in `metabase.api.field/remapped-value` if you would actually like to understand what is going on here." + [card-id field-id remapped-field-id, ^String value-str] + (check-field-is-referenced-by-card field-id card-id) + (field-remapped-values field-id remapped-field-id value-str)) + +(defn dashboard-field-remapped-values + "Return the reampped Field values for a Field referenced by a *Dashboard*. This explanation is almost useless, so see + the one in `metabase.api.field/remapped-value` if you would actually like to understand what is going on here." + [dashboard-id field-id remapped-field-id, ^String value-str] + (check-field-is-referenced-by-dashboard field-id dashboard-id) + (field-remapped-values field-id remapped-field-id value-str)) + +(api/defendpoint GET "/card/:uuid/field/:field-id/remapping/:remapped-id" + [uuid field-id remapped-id value] + {value su/NonBlankString} + (api/check-public-sharing-enabled) + (let [card-id (api/check-404 (db/select-one-id Card :public_uuid uuid, :archived false))] + (card-field-remapped-values card-id field-id remapped-id value))) + +(api/defendpoint GET "/dashboard/:uuid/field/:field-id/remapping/:remapped-id" + [uuid field-id remapped-id value] + {value su/NonBlankString} + (api/check-public-sharing-enabled) + (let [dashboard-id (db/select-one-id Dashboard :public_uuid uuid, :archived false)] + (dashboard-field-remapped-values dashboard-id field-id remapped-id value))) + + +;;; ----------------------------------------- Route Definitions & Complaints ----------------------------------------- + +;; TODO - why don't we just make these routes have a bit of middleware that includes the +;; `api/check-public-sharing-enabled` check in each of them? That way we don't need to remember to include the line in +;; every single endpoint definition here? Wouldn't that be 100x better?! +;; +;; TODO - also a smart person would probably just parse the UUIDs automatically in middleware as appropriate for +;;`/dashboard` vs `/card` (api/define-routes) diff --git a/src/metabase/db.clj b/src/metabase/db.clj index 3cf9ccbfda8900d52fa024eb0c6bb2af00feae00..10cfd8cbf70ab11493b8e4a28d472b250e8173db 100644 --- a/src/metabase/db.clj +++ b/src/metabase/db.clj @@ -421,6 +421,18 @@ (db/qualify dest-entity pk)]]}) +(defn- type-keyword->descendants + "Return a set of descendents of Metabase `type-keyword`. This includes `type-keyword` itself, so the set will always + have at least one element. + + (type-keyword->descendants :type/Coordinate) ; -> #{\"type/Latitude\" \"type/Longitude\" \"type/Coordinate\"}" + [type-keyword] + ;; make sure `type-keyword` is a valid MB type. There may be some cases where we want to use these functions for + ;; types outside of the `:type/` hierarchy. If and when that happens, we can reconsider this check. But since no + ;; such cases currently exist, adding this check to catch typos makes sense. + {:pre [(isa? type-keyword :type/*)]} + (set (map u/keyword->qualified-name (cons type-keyword (descendants type-keyword))))) + (defn isa "Convenience for generating an HoneySQL `IN` clause for a keyword and all of its descendents. Intended for use with the type hierarchy in `metabase.types`. @@ -435,6 +447,9 @@ -> (db/select Field {:where [:in :special_type #{\"type/URL\" \"type/ImageURL\" \"type/AvatarURL\"}]})" ([type-keyword] - [:in (set (map u/keyword->qualified-name (cons type-keyword (descendants type-keyword))))]) + [:in (type-keyword->descendants type-keyword)]) + ;; when using this with an `expr` (e.g. `(isa :special_type :type/URL)`) just go ahead and take the results of the + ;; one-arity impl above and splice expr in as the second element (`[:in #{"type/URL" "type/ImageURL"}]` becomes + ;; `[:in :special_type #{"type/URL" "type/ImageURL"}]`) ([expr type-keyword] - [:in expr (last (isa type-keyword))])) + [:in expr (type-keyword->descendants type-keyword)])) diff --git a/src/metabase/models/field.clj b/src/metabase/models/field.clj index 8a995ff01cab4bc2ccade04f93fd8fdd6f1a1e0f..87c58980fe4a25b8d1a72cfa1f780198273696f5 100644 --- a/src/metabase/models/field.clj +++ b/src/metabase/models/field.clj @@ -92,7 +92,7 @@ (u/strict-extend (class Field) models/IModel (merge models/IModelDefaults - {:hydration-keys (constantly [:destination :field :origin]) + {:hydration-keys (constantly [:destination :field :origin :human_readable_field]) :types (constantly {:base_type :keyword :special_type :keyword :visibility_type :keyword @@ -124,9 +124,12 @@ [{:keys [id]}] (db/select [FieldValues :field_id :values], :field_id id)) -(defn- keyed-by-field-ids - "Queries for `MODEL` instances related by `FIELDS`, returns a map - keyed by :field_id" +(defn- select-field-id->instance + "Select instances of `model` related by `field_id` FK to a Field in `fields`, and return a map of Field ID -> model + instance. This only returns a single instance for each Field! Duplicates are discarded! + + (select-field-id->instance [(Field 1) (Field 2)] FieldValues) + ;; -> {1 #FieldValues{...}, 2 #FieldValues{...}}" [fields model] (let [field-ids (set (map :id fields))] (u/key-by :field_id (when (seq field-ids) @@ -136,7 +139,7 @@ "Efficiently hydrate the `FieldValues` for a collection of FIELDS." {:batched-hydrate :values} [fields] - (let [id->field-values (keyed-by-field-ids fields FieldValues)] + (let [id->field-values (select-field-id->instance fields FieldValues)] (for [field fields] (assoc field :values (get id->field-values (:id field) []))))) @@ -144,7 +147,7 @@ "Efficiently hydrate the `FieldValues` for visibility_type normal FIELDS." {:batched-hydrate :normal_values} [fields] - (let [id->field-values (keyed-by-field-ids (filter fv/field-should-have-field-values? fields) + (let [id->field-values (select-field-id->instance (filter fv/field-should-have-field-values? fields) [FieldValues :id :human_readable_values :values :field_id])] (for [field fields] (assoc field :values (get id->field-values (:id field) []))))) @@ -153,7 +156,12 @@ "Efficiently hydrate the `Dimension` for a collection of FIELDS." {:batched-hydrate :dimensions} [fields] - (let [id->dimensions (keyed-by-field-ids fields Dimension)] + ;; TODO - it looks like we obviously thought this code would return *all* of the Dimensions for a Field, not just + ;; one! This code is obviously wrong! It will either assoc a single Dimension or an empty vector under the + ;; `:dimensions` key!!!! + ;; TODO - consult with tom and see if fixing this will break any hacks that surely must exist in the frontend to deal + ;; with this + (let [id->dimensions (select-field-id->instance fields Dimension)] (for [field fields] (assoc field :dimensions (get id->dimensions (:id field) []))))) diff --git a/src/metabase/models/params.clj b/src/metabase/models/params.clj index 8ff5b66265d24370f337c9d9dfc35dec971e2e50..1a31f251125858badaeb2deaa47851e7358311f8 100644 --- a/src/metabase/models/params.clj +++ b/src/metabase/models/params.clj @@ -1,9 +1,14 @@ (ns metabase.models.params "Utility functions for dealing with parameters for Dashboards and Cards." - (:require [metabase.query-processor.middleware.expand :as ql] + (:require [clojure.set :as set] + [metabase.query-processor.middleware.expand :as ql] metabase.query-processor.interface - [metabase.util :as u] - [toucan.db :as db]) + [metabase + [db :as mdb] + [util :as u]] + [toucan + [db :as db] + [hydrate :refer [hydrate]]]) (:import metabase.query_processor.interface.FieldPlaceholder)) ;;; +----------------------------------------------------------------------------------------------------------------+ @@ -35,8 +40,8 @@ (get-in dashcard [:card :dataset_query :native :template_tags (keyword tag) :dimension])) (defn- param-target->field-id - "Parse a Card parameter TARGET form, which looks something like `[:dimension [:field-id 100]]`, and return the Field ID - it references (if any)." + "Parse a Card parameter TARGET form, which looks something like `[:dimension [:field-id 100]]`, and return the Field + ID it references (if any)." [target dashcard] (when (ql/is-clause? :dimension target) (let [[_ dimension] target] @@ -45,12 +50,101 @@ dimension))))) +(defn- pk-fields + "Return the `fields` that are PK Fields." + [fields] + (filter #(isa? (:special_type %) :type/PK) fields)) + +(def ^:private Field:params-columns-only + "Form for use in Toucan `db/select` expressions (as a drop-in replacement for using `Field`) that returns Fields with + only the columns that are appropriate for returning in public/embedded API endpoints, which make heavy use of the + functions in this namespace. Use `conj` to add additional Fields beyond the ones already here. Use `rest` to get + just the column identifiers, perhaps for use with something like `select-keys`. Clutch! + + (db/select Field:params-columns-only)" + ['Field :id :table_id :display_name :base_type :special_type]) + +(defn- fields->table-id->name-field + "Given a sequence of `fields,` return a map of Table ID -> to a `:type/Name` Field in that Table, if one exists. In + cases where more than one name Field exists for a Table, this just adds the first one it finds." + [fields] + (when-let [table-ids (seq (map :table_id fields))] + (u/key-by :table_id (db/select Field:params-columns-only + :table_id [:in table-ids] + :special_type (mdb/isa :type/Name))))) + +(defn add-name-field + "For all `fields` that are `:type/PK` Fields, look for a `:type/Name` Field belonging to the same Table. For each + Field, if a matching name Field exists, add it under the `:name_field` key. This is so the Fields can be used in + public/embedded field values search widgets. This only includes the information needed to power those widgets, and + no more." + {:batched-hydrate :name_field} + [fields] + (let [table-id->name-field (fields->table-id->name-field (pk-fields fields))] + (for [field fields] + ;; add matching `:name_field` if it's a PK + (assoc field :name_field (when (isa? (:special_type field) :type/PK) + (table-id->name-field (:table_id field))))))) + + +;; We hydrate the `:human_readable_field` for each Dimension using the usual hydration logic, so it contains columns we +;; don't want to return. The two functions below work to remove the unneeded ones. + +(defn- remove-dimension-nonpublic-columns + "Strip nonpublic columns from a `dimension` and from its hydrated human-readable Field." + [dimension] + (-> dimension + (update :human_readable_field #(select-keys % (rest Field:params-columns-only))) + ;; these aren't exactly secret but you the frontend doesn't need them either so while we're at it let's go ahead + ;; and strip them out + (dissoc :created_at :updated_at))) + +(defn- remove-dimensions-nonpublic-columns + "Strip nonpublic columns from the hydrated human-readable Field in the hydrated Dimensions in `fields`." + [fields] + (for [field fields] + (update field :dimensions + (fn [dimension-or-dimensions] + ;; as disucssed in `metabase.models.field` the hydration code for `:dimensions` is + ;; WRONG and the value ends up either being a single Dimension or an empty vector. + ;; However at some point we will fix this so deal with either a map or a sequence of + ;; maps + (cond + (map? dimension-or-dimensions) + (remove-dimension-nonpublic-columns dimension-or-dimensions) + + (sequential? dimension-or-dimensions) + (map remove-dimension-nonpublic-columns dimension-or-dimensions)))))) + + +(defn- param-field-ids->fields + "Get the Fields (as a map of Field ID -> Field) that shoudl be returned for hydrated `:param_fields` for a Card or + Dashboard. These only contain the minimal amount of information neccesary needed to power public or embedded + parameter widgets." + [field-ids] + (when (seq field-ids) + (u/key-by :id (-> (db/select Field:params-columns-only :id [:in field-ids]) + (hydrate :has_field_values :name_field [:dimensions :human_readable_field]) + remove-dimensions-nonpublic-columns)))) + +(defmulti ^:private ^{:hydrate :param_values} param-values + "Add a `:param_values` map (Field ID -> FieldValues) containing FieldValues for the Fields referenced by the + parameters of a Card or a Dashboard. Implementations are in respective sections below." + name) + +(defmulti ^:private ^{:hydrate :param_fields} param-fields + "Add a `:param_fields` map (Field ID -> Field) for all of the Fields referenced by the parameters of a Card or + Dashboard. Implementations are below in respective sections." + name) + + ;;; +----------------------------------------------------------------------------------------------------------------+ ;;; | DASHBOARD-SPECIFIC | ;;; +----------------------------------------------------------------------------------------------------------------+ -(defn dashboard->param-field-ids - "Return a set of Field IDs referenced by parameters in Cards in this DASHBOARD, or `nil` if none are referenced." +(defn- dashboard->parameter-mapping-field-ids + "Return the IDs of any Fields referenced directly by the Dashboard's `:parameters` (i.e., 'explicit' parameters) by + looking at the appropriate `:parameter_mappings` entries for its Dashcards." [dashboard] (when-let [ids (seq (for [dashcard (:ordered_cards dashboard) param (:parameter_mappings dashcard) @@ -59,16 +153,38 @@ field-id))] (set ids))) +(declare card->template-tag-field-ids) + +(defn- dashboard->card-param-field-ids + "Return the IDs of any Fields referenced in the 'implicit' template tag field filter parameters for native queries in + the Cards in `dashboard`." + [dashboard] + (reduce + set/union + (for [{card :card} (:ordered_cards dashboard)] + (card->template-tag-field-ids card)))) + +(defn dashboard->param-field-ids + "Return a set of Field IDs referenced by parameters in Cards in this DASHBOARD, or `nil` if none are referenced. This + also includes IDs of Fields that are to be found in the 'implicit' parameters for SQL template tag Field filters." + [dashboard] + (let [dashboard (hydrate dashboard [:ordered_cards :card])] + (set/union + (dashboard->parameter-mapping-field-ids dashboard) + (dashboard->card-param-field-ids dashboard)))) + (defn- dashboard->param-field-values "Return a map of Field ID to FieldValues (if any) for any Fields referenced by Cards in DASHBOARD, or `nil` if none are referenced or none of them have FieldValues." [dashboard] (field-ids->param-field-values (dashboard->param-field-ids dashboard))) -(defn add-field-values-for-parameters - "Add a `:param_values` map containing FieldValues for the parameter Fields in the DASHBOARD." - [dashboard] - (assoc dashboard :param_values (dashboard->param-field-values dashboard))) +(defmethod param-values "Dashboard" [dashboard] + (dashboard->param-field-values dashboard)) + +(defmethod param-fields "Dashboard" [dashboard] + (-> dashboard dashboard->param-field-ids param-field-ids->fields)) + ;;; +----------------------------------------------------------------------------------------------------------------+ ;;; | CARD-SPECIFIC | @@ -83,7 +199,8 @@ :when field-id] field-id))) -(defn add-card-param-values - "Add FieldValues for any Fields referenced in CARD's `:template_tags`." - [card] - (assoc card :param_values (field-ids->param-field-values (card->template-tag-field-ids card)))) +(defmethod param-values "Card" [card] + (field-ids->param-field-values (card->template-tag-field-ids card))) + +(defmethod param-fields "Card" [card] + (-> card card->template-tag-field-ids param-field-ids->fields)) diff --git a/test/metabase/api/embed_test.clj b/test/metabase/api/embed_test.clj index 6818ada0e8b198d27844d0fcf6ad07c8bf549715..3a7540a4e52591de1de8da138d6c6384c34592d6 100644 --- a/test/metabase/api/embed_test.clj +++ b/test/metabase/api/embed_test.clj @@ -19,6 +19,7 @@ [metabase.test [data :as data] [util :as tu]] + [toucan.db :as db] [toucan.util.test :as tt]) (:import java.io.ByteArrayInputStream)) @@ -93,10 +94,11 @@ :visualization_settings {} :dataset_query {:type "query"} :parameters () - :param_values nil}) + :param_values nil + :param_fields nil}) (def successful-dashboard-info - {:description nil, :parameters (), :ordered_cards (), :param_values nil}) + {:description nil, :parameters (), :ordered_cards (), :param_values nil, :param_fields nil}) ;;; ------------------------------------------- GET /api/embed/card/:token ------------------------------------------- @@ -485,3 +487,275 @@ :card_id (u/get-id series-card) :position 0}] (:status (http/client :get 200 (str (dashcard-url (assoc dashcard :card_id (u/get-id series-card))))))))))) + + +;;; ------------------------------- GET /api/embed/card/:token/field/:field-id/values -------------------------------- + +(defn- field-values-url [card-or-dashboard field-or-id] + (str + "embed/" + (condp instance? card-or-dashboard + (class Card) (str "card/" (card-token card-or-dashboard)) + (class Dashboard) (str "dashboard/" (dash-token card-or-dashboard))) + "/field/" + (u/get-id field-or-id) + "/values")) + +(defn- do-with-embedding-enabled-and-temp-card-referencing {:style/indent 2} [table-kw field-kw f] + (with-embedding-enabled-and-new-secret-key + (tt/with-temp Card [card (assoc (public-test/mbql-card-referencing table-kw field-kw) + :enable_embedding true)] + (f card)))) + +(defmacro ^:private with-embedding-enabled-and-temp-card-referencing + {:style/indent 3} + [table-kw field-kw [card-binding] & body] + `(do-with-embedding-enabled-and-temp-card-referencing ~table-kw ~field-kw + (fn [~(or card-binding '_)] + ~@body))) + +;; should be able to fetch values for a Field referenced by a public Card +(expect + {:values [["20th Century Cafe"] + ["25°"] + ["33 Taps"] + ["800 Degrees Neapolitan Pizzeria"] + ["BCD Tofu House"]] + :field_id (data/id :venues :name)} + (with-embedding-enabled-and-temp-card-referencing :venues :name [card] + (-> (http/client :get 200 (field-values-url card (data/id :venues :name))) + (update :values (partial take 5))))) + +;; but for Fields that are not referenced we should get an Exception +(expect + "Not found." + (with-embedding-enabled-and-temp-card-referencing :venues :name [card] + (http/client :get 400 (field-values-url card (data/id :venues :price))))) + +;; Endpoint should fail if embedding is disabled +(expect + "Embedding is not enabled." + (with-embedding-enabled-and-temp-card-referencing :venues :name [card] + (tu/with-temporary-setting-values [enable-embedding false] + (http/client :get 400 (field-values-url card (data/id :venues :name)))))) + +(expect + "Embedding is not enabled for this object." + (with-embedding-enabled-and-temp-card-referencing :venues :name [card] + (db/update! Card (u/get-id card) :enable_embedding false) + (http/client :get 400 (field-values-url card (data/id :venues :name))))) + + +;;; ----------------------------- GET /api/embed/dashboard/:token/field/:field-id/values ----------------------------- + +(defn- do-with-embedding-enabled-and-temp-dashcard-referencing {:style/indent 2} [table-kw field-kw f] + (with-embedding-enabled-and-new-secret-key + (tt/with-temp* [Dashboard [dashboard {:enable_embedding true}] + Card [card (public-test/mbql-card-referencing table-kw field-kw)] + DashboardCard [dashcard {:dashboard_id (u/get-id dashboard) + :card_id (u/get-id card) + :parameter_mappings [{:card_id (u/get-id card) + :target [:dimension + [:field-id + (data/id table-kw field-kw)]]}]}]] + (f dashboard card dashcard)))) + + +(defmacro ^:private with-embedding-enabled-and-temp-dashcard-referencing + {:style/indent 3} + [table-kw field-kw [dash-binding card-binding dashcard-binding] & body] + `(do-with-embedding-enabled-and-temp-dashcard-referencing ~table-kw ~field-kw + (fn [~(or dash-binding '_) ~(or card-binding '_) ~(or dashcard-binding '_)] + ~@body))) + +;; should be able to use it when everything is g2g +(expect + {:values [["20th Century Cafe"] + ["25°"] + ["33 Taps"] + ["800 Degrees Neapolitan Pizzeria"] + ["BCD Tofu House"]] + :field_id (data/id :venues :name)} + (with-embedding-enabled-and-temp-dashcard-referencing :venues :name [dashboard] + (-> (http/client :get 200 (field-values-url dashboard (data/id :venues :name))) + (update :values (partial take 5))))) + +;; shound NOT be able to use the endpoint with a Field not referenced by the Dashboard +(expect + "Not found." + (with-embedding-enabled-and-temp-dashcard-referencing :venues :name [dashboard] + (http/client :get 400 (field-values-url dashboard (data/id :venues :price))))) + +;; Endpoint should fail if embedding is disabled +(expect + "Embedding is not enabled." + (with-embedding-enabled-and-temp-dashcard-referencing :venues :name [dashboard] + (tu/with-temporary-setting-values [enable-embedding false] + (http/client :get 400 (field-values-url dashboard (data/id :venues :name)))))) + +;; Endpoint should fail if embedding is disabled for the Dashboard +(expect + "Embedding is not enabled for this object." + (with-embedding-enabled-and-temp-dashcard-referencing :venues :name [dashboard] + (db/update! Dashboard (u/get-id dashboard) :enable_embedding false) + (http/client :get 400 (field-values-url dashboard (data/id :venues :name))))) + + +;;; ----------------------- GET /api/embed/card/:token/field/:field-id/search/:search-field-id ----------------------- + +(defn- field-search-url [card-or-dashboard field-or-id search-field-or-id] + (str "embed/" + (condp instance? card-or-dashboard + (class Card) (str "card/" (card-token card-or-dashboard)) + (class Dashboard) (str "dashboard/" (dash-token card-or-dashboard))) + "/field/" (u/get-id field-or-id) + "/search/" (u/get-id search-field-or-id))) + +(expect + [[93 "33 Taps"]] + (with-embedding-enabled-and-temp-card-referencing :venues :id [card] + (http/client :get 200 (field-search-url card (data/id :venues :id) (data/id :venues :name)) + :value "33 T"))) + +;; if search field isn't allowed to be used with the other Field endpoint should return exception +(expect + "Invalid Request." + (with-embedding-enabled-and-temp-card-referencing :venues :id [card] + (http/client :get 400 (field-search-url card (data/id :venues :id) (data/id :venues :price)) + :value "33 T"))) + +;; Endpoint should fail if embedding is disabled +(expect + "Embedding is not enabled." + (with-embedding-enabled-and-temp-card-referencing :venues :id [card] + (tu/with-temporary-setting-values [enable-embedding false] + (http/client :get 400 (field-search-url card (data/id :venues :id) (data/id :venues :name)) + :value "33 T")))) + +;; Endpoint should fail if embedding is disabled for the Card +(expect + "Embedding is not enabled for this object." + (with-embedding-enabled-and-temp-card-referencing :venues :id [card] + (db/update! Card (u/get-id card) :enable_embedding false) + (http/client :get 400 (field-search-url card (data/id :venues :id) (data/id :venues :name)) + :value "33 T"))) + + +;;; -------------------- GET /api/embed/dashboard/:token/field/:field-id/search/:search-field-id --------------------- + +(expect + [[93 "33 Taps"]] + (with-embedding-enabled-and-temp-dashcard-referencing :venues :id [dashboard] + (http/client :get (field-search-url dashboard (data/id :venues :id) (data/id :venues :name)) + :value "33 T"))) + +;; if search field isn't allowed to be used with the other Field endpoint should return exception +(expect + "Invalid Request." + (with-embedding-enabled-and-temp-dashcard-referencing :venues :id [dashboard] + (http/client :get 400 (field-search-url dashboard (data/id :venues :id) (data/id :venues :price)) + :value "33 T"))) + +;; Endpoint should fail if embedding is disabled +(expect + "Embedding is not enabled." + (with-embedding-enabled-and-temp-dashcard-referencing :venues :id [dashboard] + (tu/with-temporary-setting-values [enable-embedding false] + (http/client :get 400 (field-search-url dashboard (data/id :venues :name) (data/id :venues :name)) + :value "33 T")))) + +;; Endpoint should fail if embedding is disabled for the Dashboard +(expect + "Embedding is not enabled for this object." + (with-embedding-enabled-and-temp-dashcard-referencing :venues :id [dashboard] + (db/update! Dashboard (u/get-id dashboard) :enable_embedding false) + (http/client :get 400 (field-search-url dashboard (data/id :venues :name) (data/id :venues :name)) + :value "33 T"))) + + +;;; ----------------------- GET /api/embed/card/:token/field/:field-id/remapping/:remapped-id ------------------------ + +(defn- field-remapping-url [card-or-dashboard field-or-id remapped-field-or-id] + (str "embed/" + (condp instance? card-or-dashboard + (class Card) (str "card/" (card-token card-or-dashboard)) + (class Dashboard) (str "dashboard/" (dash-token card-or-dashboard))) + "/field/" (u/get-id field-or-id) + "/remapping/" (u/get-id remapped-field-or-id))) + +;; we should be able to use the API endpoint and get the same results we get by calling the function above directly +(expect + [10 "Fred 62"] + (with-embedding-enabled-and-temp-card-referencing :venues :id [card] + (http/client :get 200 (field-remapping-url card (data/id :venues :id) (data/id :venues :name)) + :value "10"))) + +;; shouldn't work if Card doesn't reference the Field in question +(expect + "Not found." + (with-embedding-enabled-and-temp-card-referencing :venues :price [card] + (http/client :get 400 (field-remapping-url card (data/id :venues :id) (data/id :venues :name)) + :value "10"))) + +;; ...or if the remapping Field isn't allowed to be used with the other Field +(expect + "Invalid Request." + (with-embedding-enabled-and-temp-card-referencing :venues :id [card] + (http/client :get 400 (field-remapping-url card (data/id :venues :id) (data/id :venues :price)) + :value "10"))) + +;; ...or if embedding is disabled +(expect + "Embedding is not enabled." + (with-embedding-enabled-and-temp-card-referencing :venues :id [card] + (tu/with-temporary-setting-values [enable-embedding false] + (http/client :get 400 (field-remapping-url card (data/id :venues :id) (data/id :venues :name)) + :value "10")))) + +;; ...or if embedding is disabled for the Card +(expect + "Embedding is not enabled for this object." + (with-embedding-enabled-and-temp-card-referencing :venues :id [card] + (db/update! Card (u/get-id card) :enable_embedding false) + (http/client :get 400 (field-remapping-url card (data/id :venues :id) (data/id :venues :name)) + :value "10"))) + + +;;; --------------------- GET /api/embed/dashboard/:token/field/:field-id/remapping/:remapped-id --------------------- + +;; we should be able to use the API endpoint and get the same results we get by calling the function above directly +(expect + [10 "Fred 62"] + (with-embedding-enabled-and-temp-dashcard-referencing :venues :id [dashboard] + (http/client :get 200 (field-remapping-url dashboard (data/id :venues :id) (data/id :venues :name)) + :value "10"))) + +;; shouldn't work if Card doesn't reference the Field in question +(expect + "Not found." + (with-embedding-enabled-and-temp-dashcard-referencing :venues :price [dashboard] + (http/client :get 400 (field-remapping-url dashboard (data/id :venues :id) (data/id :venues :name)) + :value "10"))) + +;; ...or if the remapping Field isn't allowed to be used with the other Field +(expect + "Invalid Request." + (with-embedding-enabled-and-temp-dashcard-referencing :venues :id [dashboard] + (http/client :get 400 (field-remapping-url dashboard (data/id :venues :id) (data/id :venues :price)) + :value "10"))) + +;; ...or if embedding is disabled +(expect + "Embedding is not enabled." + (with-embedding-enabled-and-temp-dashcard-referencing :venues :id [dashboard] + (tu/with-temporary-setting-values [enable-embedding false] + (http/client :get 400 (field-remapping-url dashboard (data/id :venues :id) (data/id :venues :name)) + :value "10")))) + +;; ...or if embedding is disabled for the Dashboard +(expect + "Embedding is not enabled for this object." + (with-embedding-enabled-and-temp-dashcard-referencing :venues :id [dashboard] + (db/update! Dashboard (u/get-id dashboard) :enable_embedding false) + (http/client :get 400 (field-remapping-url dashboard (data/id :venues :id) (data/id :venues :name)) + :value "10"))) diff --git a/test/metabase/api/field_test.clj b/test/metabase/api/field_test.clj index 2658d5c229b49c2382e2d6cfe635af90ac133db3..f25b40c3d814aab33eb516b3518b4a1f6f9d7534 100644 --- a/test/metabase/api/field_test.clj +++ b/test/metabase/api/field_test.clj @@ -6,7 +6,7 @@ [field-values :refer [FieldValues]] [table :refer [Table]]] [metabase.test - [data :refer :all] + [data :as data] [util :as tu]] [metabase.test.data.users :refer :all] [ring.util.codec :as codec] @@ -17,11 +17,8 @@ ;; Helper Fns -(def ^:private default-field-values - {:id true, :created_at true, :updated_at true, :field_id true}) - (defn- db-details [] - (tu/match-$ (db) + (tu/match-$ (data/db) {:created_at $ :engine "h2" :caveats nil @@ -41,13 +38,13 @@ ;; ## GET /api/field/:id (expect - (tu/match-$ (Field (id :users :name)) + (tu/match-$ (Field (data/id :users :name)) {:description nil - :table_id (id :users) + :table_id (data/id :users) :raw_column_id $ :fingerprint $ :fingerprint_version $ - :table (tu/match-$ (Table (id :users)) + :table (tu/match-$ (Table (data/id :users)) {:description nil :entity_type nil :visibility_type nil @@ -59,8 +56,8 @@ :updated_at $ :entity_name nil :active true - :id (id :users) - :db_id (id) + :id (data/id :users) + :db_id (data/id) :caveats nil :points_of_interest nil :show_in_getting_started false @@ -74,7 +71,7 @@ :updated_at $ :last_analyzed $ :active true - :id (id :users :name) + :id (data/id :users :name) :visibility_type "normal" :position 0 :preview_display true @@ -83,14 +80,16 @@ :base_type "type/Text" :has_field_values "list" :fk_target_field_id nil - :parent_id nil}) - ((user->client :rasta) :get 200 (format "field/%d" (id :users :name)))) + :parent_id nil + :dimensions [] + :name_field nil}) + ((user->client :rasta) :get 200 (format "field/%d" (data/id :users :name)))) ;; ## GET /api/field/:id/summary (expect [["count" 75] ; why doesn't this come back as a dictionary ? ["distincts" 75]] - ((user->client :rasta) :get 200 (format "field/%d/summary" (id :categories :name)))) + ((user->client :rasta) :get 200 (format "field/%d/summary" (data/id :categories :name)))) ;; ## PUT /api/field/:id @@ -158,7 +157,7 @@ (defn- field->field-values "Fetch the `FieldValues` object that corresponds to a given `Field`." [table-kw field-kw] - (FieldValues :field_id (id table-kw field-kw))) + (FieldValues :field_id (data/id table-kw field-kw))) (defn- field-values-id [table-key field-key] (:id (field->field-values table-key field-key))) @@ -166,38 +165,33 @@ ;; ## GET /api/field/:id/values ;; Should return something useful for a field that has special_type :type/Category (expect - (merge default-field-values {:values (mapv vector [1 2 3 4])}) + {:values [[1] [2] [3] [4]], :field_id (data/id :venues :price)} (do ;; clear out existing human_readable_values in case they're set (db/update! FieldValues (field-values-id :venues :price) :human_readable_values nil) ;; now update the values via the API - (tu/boolean-ids-and-timestamps ((user->client :rasta) :get 200 (format "field/%d/values" (id :venues :price)))))) + ((user->client :rasta) :get 200 (format "field/%d/values" (data/id :venues :price))))) ;; Should return nothing for a field whose special_type is *not* :type/Category (expect - {:values []} - ((user->client :rasta) :get 200 (format "field/%d/values" (id :venues :id)))) + {:values [], :field_id (data/id :venues :id)} + ((user->client :rasta) :get 200 (format "field/%d/values" (data/id :venues :id)))) ;; Sensisitive fields do not have field values and should return empty (expect - {:values []} - ((user->client :rasta) :get 200 (format "field/%d/values" (id :users :password)))) + {:values [], :field_id (data/id :users :password)} + ((user->client :rasta) :get 200 (format "field/%d/values" (data/id :users :password)))) -(defn- num->$ [num-seq] - (mapv (fn [idx] - (vector idx (apply str (repeat idx \$)))) - num-seq)) - -(def category-field {:name "Field Test" :base_type :type/Integer :special_type :type/Category}) +(def ^:private category-field {:name "Field Test" :base_type :type/Integer :special_type :type/Category}) ;; ## POST /api/field/:id/values ;; Human readable values are optional (expect - [(merge default-field-values {:values (map vector (range 5 10))}) + [{:values [[5] [6] [7] [8] [9]], :field_id true} {:status "success"} - (merge default-field-values {:values (map vector (range 1 5))})] + {:values [[1] [2] [3] [4]], :field_id true}] (tt/with-temp* [Field [{field-id :id} category-field] FieldValues [{field-value-id :id} {:values (range 5 10), :field_id field-id}]] (mapv tu/boolean-ids-and-timestamps @@ -208,54 +202,54 @@ ;; Existing field values can be updated (with their human readable values) (expect - [(merge default-field-values {:values (map vector (range 1 5))}) + [{:values [[1] [2] [3] [4]], :field_id true} {:status "success"} - (merge default-field-values {:values (num->$ (range 1 5))})] + {:values [[1 "$"] [2 "$$"] [3 "$$$"] [4 "$$$$"]], :field_id true}] (tt/with-temp* [Field [{field-id :id} category-field] FieldValues [{field-value-id :id} {:values (range 1 5), :field_id field-id}]] (mapv tu/boolean-ids-and-timestamps [((user->client :crowberto) :get 200 (format "field/%d/values" field-id)) ((user->client :crowberto) :post 200 (format "field/%d/values" field-id) - {:values (num->$ (range 1 5))}) + {:values [[1 "$"] [2 "$$"] [3 "$$$"] [4 "$$$$"]]}) ((user->client :crowberto) :get 200 (format "field/%d/values" field-id))]))) ;; Field values are created when not present (expect - [(merge default-field-values {:values []}) + [{:values [], :field_id true} {:status "success"} - (merge default-field-values {:values (num->$ (range 1 5))})] + {:values [[1 "$"] [2 "$$"] [3 "$$$"] [4 "$$$$"]], :field_id true}] (tt/with-temp* [Field [{field-id :id} category-field]] (mapv tu/boolean-ids-and-timestamps [((user->client :crowberto) :get 200 (format "field/%d/values" field-id)) ((user->client :crowberto) :post 200 (format "field/%d/values" field-id) - {:values (num->$ (range 1 5))}) + {:values [[1 "$"] [2 "$$"] [3 "$$$"] [4 "$$$$"]]}) ((user->client :crowberto) :get 200 (format "field/%d/values" field-id))]))) ;; Can unset values (expect - [(merge default-field-values {:values (mapv vector (range 1 5))}) + [{:values [[1] [2] [3] [4]], :field_id true} {:status "success"} - (merge default-field-values {:values []})] + {:values [], :field_id true}] (tt/with-temp* [Field [{field-id :id} category-field] FieldValues [{field-value-id :id} {:values (range 1 5), :field_id field-id}]] (mapv tu/boolean-ids-and-timestamps [((user->client :crowberto) :get 200 (format "field/%d/values" field-id)) ((user->client :crowberto) :post 200 (format "field/%d/values" field-id) - {:values []}) + {:values [], :field_id true}) ((user->client :crowberto) :get 200 (format "field/%d/values" field-id))]))) ;; Can unset just human readable values (expect - [(merge default-field-values {:values (num->$ (range 1 5))}) + [{:values [[1 "$"] [2 "$$"] [3 "$$$"] [4 "$$$$"]], :field_id true} {:status "success"} - (merge default-field-values {:values (mapv vector (range 1 5))})] + {:values [[1] [2] [3] [4]], :field_id true}] (tt/with-temp* [Field [{field-id :id} category-field] FieldValues [{field-value-id :id} {:values (range 1 5), :field_id field-id :human_readable_values ["$" "$$" "$$$" "$$$$"]}]] (mapv tu/boolean-ids-and-timestamps [((user->client :crowberto) :get 200 (format "field/%d/values" field-id)) ((user->client :crowberto) :post 200 (format "field/%d/values" field-id) - {:values (mapv vector (range 1 5))}) + {:values [[1] [2] [3] [4]]}) ((user->client :crowberto) :get 200 (format "field/%d/values" field-id))]))) ;; Should throw when human readable values are present but not for every value @@ -264,15 +258,15 @@ (tt/with-temp* [Field [{field-id :id} {:name "Field Test" :base_type :type/Integer :special_type :type/Category}]] ((user->client :crowberto) :post 400 (format "field/%d/values" field-id) {:values [[1 "$"] [2 "$$"] [3] [4]]}))) - -;; ## PUT /api/field/:id/dimension +, :field_id true +;; ## PUT /api/field/:id/dimensio, :field_id truen (defn- dimension-for-field [field-id] (-> (Field :id field-id) (hydrate :dimensions) :dimensions)) -(defn dimension-post [field-id map-to-post] +(defn- create-dimension-via-API! [field-id map-to-post] ((user->client :crowberto) :post 200 (format "field/%d/dimension" field-id) map-to-post)) ;; test that we can do basic field update work, including unsetting some fields such as special-type @@ -295,9 +289,9 @@ true] (tt/with-temp* [Field [{field-id :id} {:name "Field Test"}]] (let [before-creation (dimension-for-field field-id) - _ (dimension-post field-id {:name "some dimension name", :type "internal"}) + _ (create-dimension-via-API! field-id {:name "some dimension name", :type "internal"}) new-dim (dimension-for-field field-id) - _ (dimension-post field-id {:name "different dimension name", :type "internal"}) + _ (create-dimension-via-API! field-id {:name "different dimension name", :type "internal"}) updated-dim (dimension-for-field field-id)] [before-creation (tu/boolean-ids-and-timestamps new-dim) @@ -312,17 +306,17 @@ ;; test that we can do basic field update work, including unsetting some fields such as special-type (expect [[] - {:id true - :created_at true - :updated_at true - :type :external - :name "some dimension name" + {:id true + :created_at true + :updated_at true + :type :external + :name "some dimension name" :human_readable_field_id true - :field_id true}] + :field_id true}] (tt/with-temp* [Field [{field-id-1 :id} {:name "Field Test 1"}] Field [{field-id-2 :id} {:name "Field Test 2"}]] (let [before-creation (dimension-for-field field-id-1) - _ (dimension-post field-id-1 {:name "some dimension name", :type "external" :human_readable_field_id field-id-2}) + _ (create-dimension-via-API! field-id-1 {:name "some dimension name", :type "external" :human_readable_field_id field-id-2}) new-dim (dimension-for-field field-id-1)] [before-creation (tu/boolean-ids-and-timestamps new-dim)]))) @@ -331,9 +325,9 @@ (expect clojure.lang.ExceptionInfo (tt/with-temp* [Field [{field-id-1 :id} {:name "Field Test 1"}]] - (dimension-post field-id-1 {:name "some dimension name", :type "external"}))) - -;; Non-admin users can't update dimensions + (create-dimension-via-API! field-id-1 {:name "some dimension name", :type "external"}))) +, :field_id true +;; Non-admin users can't update dimension, :field_id trues (expect "You don't have permissions to do that." (tt/with-temp* [Field [{field-id :id} {:name "Field Test 1"}]] @@ -351,7 +345,7 @@ []] (tt/with-temp* [Field [{field-id :id} {:name "Field Test"}]] - (dimension-post field-id {:name "some dimension name", :type "internal"}) + (create-dimension-via-API! field-id {:name "some dimension name", :type "internal"}) (let [new-dim (dimension-for-field field-id)] ((user->client :crowberto) :delete 204 (format "field/%d/dimension" field-id)) @@ -378,7 +372,7 @@ :special_type :type/FK}] Field [{field-id-2 :id} {:name "Field Test 2"}]] - (dimension-post field-id-1 {:name "fk-remove-dimension", :type "external" :human_readable_field_id field-id-2}) + (create-dimension-via-API! field-id-1 {:name "fk-remove-dimension", :type "external" :human_readable_field_id field-id-2}) (let [new-dim (dimension-for-field field-id-1) _ ((user->client :crowberto) :put 200 (format "field/%d" field-id-1) {:special_type nil}) @@ -399,7 +393,7 @@ :special_type :type/FK}] Field [{field-id-2 :id} {:name "Field Test 2"}]] - (dimension-post field-id-1 {:name "fk-remove-dimension", :type "external" :human_readable_field_id field-id-2}) + (create-dimension-via-API! field-id-1 {:name "fk-remove-dimension", :type "external" :human_readable_field_id field-id-2}) (let [new-dim (dimension-for-field field-id-1) _ ((user->client :crowberto) :put 200 (format "field/%d" field-id-1) {:description "something diffrent"}) @@ -522,7 +516,7 @@ []] (tt/with-temp* [Field [{field-id :id} {:name "Field Test" :base_type "type/Integer"}]] - (dimension-post field-id {:name "some dimension name", :type "internal"}) + (create-dimension-via-API! field-id {:name "some dimension name", :type "internal"}) (let [new-dim (dimension-for-field field-id)] ((user->client :crowberto) :put 200 (format "field/%d" field-id) {:special_type "type/Text"}) [(tu/boolean-ids-and-timestamps new-dim) @@ -539,7 +533,7 @@ :field_id true}) (tt/with-temp* [Field [{field-id :id} {:name "Field Test" :base_type "type/Integer"}]] - (dimension-post field-id {:name "some dimension name", :type "internal"}) + (create-dimension-via-API! field-id {:name "some dimension name", :type "internal"}) (let [new-dim (dimension-for-field field-id)] ((user->client :crowberto) :put 200 (format "field/%d" field-id) {:special_type "type/Category"}) [(tu/boolean-ids-and-timestamps new-dim) diff --git a/test/metabase/api/public_test.clj b/test/metabase/api/public_test.clj index 1356f0ceda35488926818f189546569b3b426793..9db314ba78d3ae304c049e64e07e883a854fb830 100644 --- a/test/metabase/api/public_test.clj +++ b/test/metabase/api/public_test.clj @@ -14,6 +14,8 @@ [dashboard :refer [Dashboard]] [dashboard-card :refer [DashboardCard]] [dashboard-card-series :refer [DashboardCardSeries]] + [dimension :refer [Dimension]] + [field :refer [Field]] [field-values :refer [FieldValues]]] [metabase.test [data :as data] @@ -88,7 +90,7 @@ ;; Check that we can fetch a PublicCard (expect - #{:dataset_query :description :display :id :name :visualization_settings :param_values} + #{:dataset_query :description :display :id :name :visualization_settings :param_values :param_fields} (tu/with-temporary-setting-values [enable-public-sharing true] (with-temp-public-card [{uuid :public_uuid}] (set (keys (http/client :get 200 (str "public/card/" uuid))))))) @@ -364,3 +366,429 @@ (add-price-param-to-dashboard! dash) (add-dimension-param-mapping-to-dashcard! dashcard card ["fk->" (data/id :checkins :venue_id) (data/id :venues :price)]) (GET-param-values dash))) + + +;;; +----------------------------------------------------------------------------------------------------------------+ +;;; | New FieldValues search endpoints | +;;; +----------------------------------------------------------------------------------------------------------------+ + +(defn- mbql-card-referencing-nothing [] + {:dataset_query {:database (data/id)}}) + +(defn mbql-card-referencing [table-kw field-kw] + {:dataset_query + {:database (data/id) + :type :query + :query {:source-table (data/id table-kw) + :filter [:= [:field-id (data/id table-kw field-kw)] "Krua Siri"]}}}) + +(defn- mbql-card-referencing-venue-name [] + (mbql-card-referencing :venues :name)) + +(defn- sql-card-referencing-venue-name [] + {:dataset_query + {:database (data/id) + :type :native + :native {:query "SELECT COUNT(*) FROM VENUES WHERE {{x}}" + :template_tags {:x {:name :x + :display_name "X" + :type :dimension + :dimension [:field-id (data/id :venues :name)]}}}}}) + +;;; ------------------------------------------- card->referenced-field-ids ------------------------------------------- + +(expect + #{} + (tt/with-temp Card [card (mbql-card-referencing-nothing)] + (#'public-api/card->referenced-field-ids card))) + +;; It should pick up on Fields referenced in the MBQL query itself... +(expect + #{(data/id :venues :name)} + (tt/with-temp Card [card (mbql-card-referencing-venue-name)] + (#'public-api/card->referenced-field-ids card))) + +;; ...as well as template tag "implict" params for SQL queries +(expect + #{(data/id :venues :name)} + (tt/with-temp Card [card (sql-card-referencing-venue-name)] + (#'public-api/card->referenced-field-ids card))) + + +;;; --------------------------------------- check-field-is-referenced-by-card ---------------------------------------- + +;; Check that the check succeeds when Field is referenced +(expect + (tt/with-temp Card [card (mbql-card-referencing-venue-name)] + (#'public-api/check-field-is-referenced-by-card (data/id :venues :name) (u/get-id card)))) + +;; check that exception is thrown if the Field isn't referenced +(expect + Exception + (tt/with-temp Card [card (mbql-card-referencing-venue-name)] + (#'public-api/check-field-is-referenced-by-card (data/id :venues :category_id) (u/get-id card)))) + + +;;; ----------------------------------------- check-search-field-is-allowed ------------------------------------------ + +;; search field is allowed IF: +;; A) search-field is the same field as the other one +(expect + (#'public-api/check-search-field-is-allowed (data/id :venues :id) (data/id :venues :id))) + +(expect + Exception + (#'public-api/check-search-field-is-allowed (data/id :venues :id) (data/id :venues :category_id))) + +;; B) there's a Dimension that lists search field as the human_readable_field for the other field +(expect + (tt/with-temp Dimension [_ {:field_id (data/id :venues :id), :human_readable_field_id (data/id :venues :category_id)}] + (#'public-api/check-search-field-is-allowed (data/id :venues :id) (data/id :venues :category_id)))) + +;; C) search-field is a Name Field belonging to the same table as the other field, which is a PK +(expect + (do ;tu/with-temp-vals-in-db Field (data/id :venues :name) {:special_type "type/Name"} + (#'public-api/check-search-field-is-allowed (data/id :venues :id) (data/id :venues :name)))) + +;; not allowed if search field isn't a NAME +(expect + Exception + (tu/with-temp-vals-in-db Field (data/id :venues :name) {:special_type "type/Latitude"} + (#'public-api/check-search-field-is-allowed (data/id :venues :id) (data/id :venues :name)))) + +;; not allowed if search field belongs to a different TABLE +(expect + Exception + (tu/with-temp-vals-in-db Field (data/id :categories :name) {:special_type "type/Name"} + (#'public-api/check-search-field-is-allowed (data/id :venues :id) (data/id :categories :name)))) + + +;;; ------------------------------------- check-field-is-referenced-by-dashboard ------------------------------------- + +(defn- dashcard-with-param-mapping-to-venue-id [dashboard card] + {:dashboard_id (u/get-id dashboard) + :card_id (u/get-id card) + :parameter_mappings [{:card_id (u/get-id card) + :target [:dimension [:field-id (data/id :venues :id)]]}]}) + +;; Field is "referenced" by Dashboard if it's one of the Dashboard's params... +(expect + (tt/with-temp* [Dashboard [dashboard] + Card [card] + DashboardCard [_ (dashcard-with-param-mapping-to-venue-id dashboard card)]] + (#'public-api/check-field-is-referenced-by-dashboard (data/id :venues :id) (u/get-id dashboard)))) + +(expect + Exception + (tt/with-temp* [Dashboard [dashboard] + Card [card] + DashboardCard [_ (dashcard-with-param-mapping-to-venue-id dashboard card)]] + (#'public-api/check-field-is-referenced-by-dashboard (data/id :venues :name) (u/get-id dashboard)))) + +;; ...*or* if it's a so-called "implicit" param (a Field Filter Template Tag (FFTT) in a SQL Card) +(expect + (tt/with-temp* [Dashboard [dashboard] + Card [card (sql-card-referencing-venue-name)] + DashboardCard [_ {:dashboard_id (u/get-id dashboard), :card_id (u/get-id card)}]] + (#'public-api/check-field-is-referenced-by-dashboard (data/id :venues :name) (u/get-id dashboard)))) + +(expect + Exception + (tt/with-temp* [Dashboard [dashboard] + Card [card (sql-card-referencing-venue-name)] + DashboardCard [_ {:dashboard_id (u/get-id dashboard), :card_id (u/get-id card)}]] + (#'public-api/check-field-is-referenced-by-dashboard (data/id :venues :id) (u/get-id dashboard)))) + + +;;; ------------------------------------------- card-and-field-id->values -------------------------------------------- + +;; We should be able to get values for a Field referenced by a Card +(expect + {:values [["20th Century Cafe"] + ["25°"] + ["33 Taps"] + ["800 Degrees Neapolitan Pizzeria"] + ["BCD Tofu House"]] + :field_id (data/id :venues :name)} + (tt/with-temp Card [card (mbql-card-referencing :venues :name)] + (-> (public-api/card-and-field-id->values (u/get-id card) (data/id :venues :name)) + (update :values (partial take 5))))) + +;; SQL param field references should work just as well as MBQL field referenced +(expect + {:values [["20th Century Cafe"] + ["25°"] + ["33 Taps"] + ["800 Degrees Neapolitan Pizzeria"] + ["BCD Tofu House"]] + :field_id (data/id :venues :name)} + (tt/with-temp Card [card (sql-card-referencing-venue-name)] + (-> (public-api/card-and-field-id->values (u/get-id card) (data/id :venues :name)) + (update :values (partial take 5))))) + +;; But if the Field is not referenced we should get an Exception +(expect + Exception + (tt/with-temp Card [card (mbql-card-referencing :venues :price)] + (public-api/card-and-field-id->values (u/get-id card) (data/id :venues :name)))) + + +;;; ------------------------------- GET /api/public/card/:uuid/field/:field-id/values -------------------------------- + +(defn- field-values-url [card-or-dashboard field-or-id] + (str "public/" + (condp instance? card-or-dashboard + (class Card) "card" + (class Dashboard) "dashboard") + "/" (or (:public_uuid card-or-dashboard) + (throw (Exception. (str "Missing public UUID: " card-or-dashboard)))) + "/field/" (u/get-id field-or-id) + "/values")) + +(defn- do-with-sharing-enabled-and-temp-card-referencing {:style/indent 2} [table-kw field-kw f] + (tu/with-temporary-setting-values [enable-public-sharing true] + (tt/with-temp Card [card (merge (shared-obj) (mbql-card-referencing table-kw field-kw))] + (f card)))) + +(defmacro ^:private with-sharing-enabled-and-temp-card-referencing + {:style/indent 3} + [table-kw field-kw [card-binding] & body] + `(do-with-sharing-enabled-and-temp-card-referencing ~table-kw ~field-kw + (fn [~card-binding] + ~@body))) + +;; should be able to fetch values for a Field referenced by a public Card +(expect + {:values [["20th Century Cafe"] + ["25°"] + ["33 Taps"] + ["800 Degrees Neapolitan Pizzeria"] + ["BCD Tofu House"]] + :field_id (data/id :venues :name)} + (with-sharing-enabled-and-temp-card-referencing :venues :name [card] + (-> (http/client :get 200 (field-values-url card (data/id :venues :name))) + (update :values (partial take 5))))) + +;; but for Fields that are not referenced we should get an Exception +(expect + "An error occurred." + (with-sharing-enabled-and-temp-card-referencing :venues :name [card] + (http/client :get 400 (field-values-url card (data/id :venues :price))))) + +;; Endpoint should fail if public sharing is disabled +(expect + "An error occurred." + (with-sharing-enabled-and-temp-card-referencing :venues :name [card] + (tu/with-temporary-setting-values [enable-public-sharing false] + (http/client :get 400 (field-values-url card (data/id :venues :name)))))) + + +;;; ----------------------------- GET /api/public/dashboard/:uuid/field/:field-id/values ----------------------------- + +(defn do-with-sharing-enabled-and-temp-dashcard-referencing {:style/indent 2} [table-kw field-kw f] + (tu/with-temporary-setting-values [enable-public-sharing true] + (tt/with-temp* [Dashboard [dashboard (shared-obj)] + Card [card (mbql-card-referencing table-kw field-kw)] + DashboardCard [dashcard {:dashboard_id (u/get-id dashboard) + :card_id (u/get-id card) + :parameter_mappings [{:card_id (u/get-id card) + :target [:dimension + [:field-id + (data/id table-kw field-kw)]]}]}]] + (f dashboard card dashcard)))) + +(defmacro with-sharing-enabled-and-temp-dashcard-referencing + {:style/indent 3} + [table-kw field-kw [dashboard-binding card-binding dashcard-binding] & body] + `(do-with-sharing-enabled-and-temp-dashcard-referencing ~table-kw ~field-kw + (fn [~(or dashboard-binding '_) ~(or card-binding '_) ~(or dashcard-binding '_)] + ~@body))) + +;; should be able to use it when everything is g2g +(expect + {:values [["20th Century Cafe"] + ["25°"] + ["33 Taps"] + ["800 Degrees Neapolitan Pizzeria"] + ["BCD Tofu House"]] + :field_id (data/id :venues :name)} + (with-sharing-enabled-and-temp-dashcard-referencing :venues :name [dashboard] + (-> (http/client :get 200 (field-values-url dashboard (data/id :venues :name))) + (update :values (partial take 5))))) + +;; shound NOT be able to use the endpoint with a Field not referenced by the Dashboard +(expect + "An error occurred." + (with-sharing-enabled-and-temp-dashcard-referencing :venues :name [dashboard] + (http/client :get 400 (field-values-url dashboard (data/id :venues :price))))) + +;; Endpoint should fail if public sharing is disabled +(expect + "An error occurred." + (with-sharing-enabled-and-temp-dashcard-referencing :venues :name [dashboard] + (tu/with-temporary-setting-values [enable-public-sharing false] + (http/client :get 400 (field-values-url dashboard (data/id :venues :name)))))) + + +;;; ----------------------------------------------- search-card-fields ----------------------------------------------- + +(expect + [[93 "33 Taps"]] + (with-sharing-enabled-and-temp-card-referencing :venues :id [card] + (do ;tu/with-temp-vals-in-db Field (data/id :venues :name) {:special_type "type/Name"} + (public-api/search-card-fields (u/get-id card) (data/id :venues :id) (data/id :venues :name) "33 T" 10)))) + +;; shouldn't work if the search-field isn't allowed to be used in combination with the other Field +(expect + Exception + (with-sharing-enabled-and-temp-card-referencing :venues :id [card] + (public-api/search-card-fields (u/get-id card) (data/id :venues :id) (data/id :venues :price) "33 T" 10))) + +;; shouldn't work if the field isn't referenced by CARD +(expect + Exception + (with-sharing-enabled-and-temp-card-referencing :venues :name [card] + (public-api/search-card-fields (u/get-id card) (data/id :venues :id) (data/id :venues :id) "33 T" 10))) + + +;;; ----------------------- GET /api/public/card/:uuid/field/:field-id/search/:search-field-id ----------------------- + +(defn- field-search-url [card-or-dashboard field-or-id search-field-or-id] + (str "public/" + (condp instance? card-or-dashboard + (class Card) "card" + (class Dashboard) "dashboard") + "/" (:public_uuid card-or-dashboard) + "/field/" (u/get-id field-or-id) + "/search/" (u/get-id search-field-or-id))) + +(expect + [[93 "33 Taps"]] + (with-sharing-enabled-and-temp-card-referencing :venues :id [card] + (http/client :get 200 (field-search-url card (data/id :venues :id) (data/id :venues :name)) + :value "33 T"))) + +;; if search field isn't allowed to be used with the other Field endpoint should return exception +(expect + "An error occurred." + (with-sharing-enabled-and-temp-card-referencing :venues :id [card] + (http/client :get 400 (field-search-url card (data/id :venues :id) (data/id :venues :price)) + :value "33 T"))) + +;; Endpoint should fail if public sharing is disabled +(expect + "An error occurred." + (with-sharing-enabled-and-temp-card-referencing :venues :id [card] + (tu/with-temporary-setting-values [enable-public-sharing false] + (http/client :get 400 (field-search-url card (data/id :venues :id) (data/id :venues :name)) + :value "33 T")))) + + +;;; -------------------- GET /api/public/dashboard/:uuid/field/:field-id/search/:search-field-id --------------------- + +(expect + [[93 "33 Taps"]] + (with-sharing-enabled-and-temp-dashcard-referencing :venues :id [dashboard] + (http/client :get (field-search-url dashboard (data/id :venues :id) (data/id :venues :name)) + :value "33 T"))) + +;; if search field isn't allowed to be used with the other Field endpoint should return exception +(expect + "An error occurred." + (with-sharing-enabled-and-temp-dashcard-referencing :venues :id [dashboard] + (http/client :get 400 (field-search-url dashboard (data/id :venues :id) (data/id :venues :price)) + :value "33 T"))) + +;; Endpoint should fail if public sharing is disabled +(expect + "An error occurred." + (with-sharing-enabled-and-temp-dashcard-referencing :venues :id [dashboard] + (tu/with-temporary-setting-values [enable-public-sharing false] + (http/client :get 400 (field-search-url dashboard (data/id :venues :name) (data/id :venues :name)) + :value "33 T")))) + +;;; --------------------------------------------- field-remapped-values ---------------------------------------------- + +;; `field-remapped-values` should return remappings in the expected format when the combination of Fields is allowed. +;; It should parse the value string (it comes back from the API as a string since it is a query param) +(expect + [10 "Fred 62"] + (#'public-api/field-remapped-values (data/id :venues :id) (data/id :venues :name) "10")) + +;; if the Field isn't allowed +(expect + Exception + (#'public-api/field-remapped-values (data/id :venues :id) (data/id :venues :price) "10")) + + +;;; ----------------------- GET /api/public/card/:uuid/field/:field-id/remapping/:remapped-id ------------------------ + +(defn- field-remapping-url [card-or-dashboard field-or-id remapped-field-or-id] + (str "public/" + (condp instance? card-or-dashboard + (class Card) "card" + (class Dashboard) "dashboard") + "/" (:public_uuid card-or-dashboard) + "/field/" (u/get-id field-or-id) + "/remapping/" (u/get-id remapped-field-or-id))) + +;; we should be able to use the API endpoint and get the same results we get by calling the function above directly +(expect + [10 "Fred 62"] + (with-sharing-enabled-and-temp-card-referencing :venues :id [card] + (http/client :get 200 (field-remapping-url card (data/id :venues :id) (data/id :venues :name)) + :value "10"))) + +;; shouldn't work if Card doesn't reference the Field in question +(expect + "An error occurred." + (with-sharing-enabled-and-temp-card-referencing :venues :price [card] + (http/client :get 400 (field-remapping-url card (data/id :venues :id) (data/id :venues :name)) + :value "10"))) + +;; ...or if the remapping Field isn't allowed to be used with the other Field +(expect + "An error occurred." + (with-sharing-enabled-and-temp-card-referencing :venues :id [card] + (http/client :get 400 (field-remapping-url card (data/id :venues :id) (data/id :venues :price)) + :value "10"))) + +;; ...or if public sharing is disabled +(expect + "An error occurred." + (with-sharing-enabled-and-temp-card-referencing :venues :id [card] + (tu/with-temporary-setting-values [enable-public-sharing false] + (http/client :get 400 (field-remapping-url card (data/id :venues :id) (data/id :venues :name)) + :value "10")))) + + +;;; --------------------- GET /api/public/dashboard/:uuid/field/:field-id/remapping/:remapped-id --------------------- + +;; we should be able to use the API endpoint and get the same results we get by calling the function above directly +(expect + [10 "Fred 62"] + (with-sharing-enabled-and-temp-dashcard-referencing :venues :id [dashboard] + (http/client :get 200 (field-remapping-url dashboard (data/id :venues :id) (data/id :venues :name)) + :value "10"))) + +;; shouldn't work if Card doesn't reference the Field in question +(expect + "An error occurred." + (with-sharing-enabled-and-temp-dashcard-referencing :venues :price [dashboard] + (http/client :get 400 (field-remapping-url dashboard (data/id :venues :id) (data/id :venues :name)) + :value "10"))) + +;; ...or if the remapping Field isn't allowed to be used with the other Field +(expect + "An error occurred." + (with-sharing-enabled-and-temp-dashcard-referencing :venues :id [dashboard] + (http/client :get 400 (field-remapping-url dashboard (data/id :venues :id) (data/id :venues :price)) + :value "10"))) + +;; ...or if public sharing is disabled +(expect + "An error occurred." + (with-sharing-enabled-and-temp-dashcard-referencing :venues :id [dashboard] + (tu/with-temporary-setting-values [enable-public-sharing false] + (http/client :get 400 (field-remapping-url dashboard (data/id :venues :id) (data/id :venues :name)) + :value "10")))) diff --git a/test/metabase/models/params_test.clj b/test/metabase/models/params_test.clj new file mode 100644 index 0000000000000000000000000000000000000000..399178146663bc8d3983454cb5653740e6dc18c4 --- /dev/null +++ b/test/metabase/models/params_test.clj @@ -0,0 +1,100 @@ +(ns metabase.models.params-test + "Tests for the utility functions for dealing with parameters in `metabase.models.params`." + (:require [expectations :refer :all] + [metabase.api.public-test :as public-test] + [metabase.models + [card :refer [Card]] + [field :refer [Field]] + [params :as params]] + [metabase.test.data :as data] + [toucan + [db :as db] + [hydrate :refer [hydrate]]] + [toucan.util.test :as tt])) + +;;; ---------------------------------------------- name_field hydration ---------------------------------------------- + +;; make sure that we can hydrate the `name_field` property for PK Fields +(expect + {:name "ID" + :table_id (data/id :venues) + :special_type :type/PK + :name_field {:id (data/id :venues :name) + :table_id (data/id :venues) + :display_name "Name" + :base_type :type/Text + :special_type :type/Name}} + (-> (db/select-one [Field :name :table_id :special_type], :id (data/id :venues :id)) + (hydrate :name_field))) + +;; make sure it works for multiple fields efficiently. Should only require one DB call to hydrate many Fields +(expect + 1 + (let [venues-fields (db/select Field :table_id (data/id :venues))] + (db/with-call-counting [call-count] + (hydrate venues-fields :name_field) + (call-count)))) + +;; It shouldn't hydrate for Fields that aren't PKs +(expect + {:name "PRICE" + :table_id (data/id :venues) + :special_type :type/Category + :name_field nil} + (-> (db/select-one [Field :name :table_id :special_type], :id (data/id :venues :price)) + (hydrate :name_field))) + +;; Or if it *is* a PK, but no name Field is available for that Table, it shouldn't hydrate +(expect + {:name "ID" + :table_id (data/id :checkins) + :special_type :type/PK + :name_field nil} + (-> (db/select-one [Field :name :table_id :special_type], :id (data/id :checkins :id)) + (hydrate :name_field))) + + +;;; -------------------------------------------------- param_fields -------------------------------------------------- + +;; check that we can hydrate param_fields for a Card +(expect + {(data/id :venues :id) {:id (data/id :venues :id) + :table_id (data/id :venues) + :display_name "ID" + :base_type :type/BigInteger + :special_type :type/PK + :has_field_values :search + :name_field {:id (data/id :venues :name) + :table_id (data/id :venues) + :display_name "Name" + :base_type :type/Text + :special_type :type/Name} + :dimensions []}} + (tt/with-temp Card [card {:dataset_query + {:database (data/id) + :type :native + :native {:query "SELECT COUNT(*) FROM VENUES WHERE {{x}}" + :template_tags {:name {:name :name + :display_name "Name" + :type :dimension + :dimension [:field-id (data/id :venues :id)]}}}}}] + (-> (hydrate card :param_fields) + :param_fields))) + +;; check that we can hydrate param_fields for a Dashboard +(expect + {(data/id :venues :id) {:id (data/id :venues :id) + :table_id (data/id :venues) + :display_name "ID" + :base_type :type/BigInteger + :special_type :type/PK + :has_field_values :search + :name_field {:id (data/id :venues :name) + :table_id (data/id :venues) + :display_name "Name" + :base_type :type/Text + :special_type :type/Name} + :dimensions []}} + (public-test/with-sharing-enabled-and-temp-dashcard-referencing :venues :id [dashboard] + (-> (hydrate dashboard :param_fields) + :param_fields))) diff --git a/test/metabase/test/util.clj b/test/metabase/test/util.clj index 2b359601c68650a0aae3a8ae0a95eff740827fd9..5548f93a102747b36d263e1a9cd941213b5f29b7 100644 --- a/test/metabase/test/util.clj +++ b/test/metabase/test/util.clj @@ -16,6 +16,7 @@ [dashboard :refer [Dashboard]] [dashboard-card-series :refer [DashboardCardSeries]] [database :refer [Database]] + [dimension :refer [Dimension]] [field :refer [Field]] [metric :refer [Metric]] [permissions-group :refer [PermissionsGroup]] @@ -155,6 +156,11 @@ :is_sample false :name (random-name)})}) +(u/strict-extend (class Dimension) + test/WithTempDefaults + {:with-temp-defaults (fn [_] {:name (random-name) + :type "internal"})}) + (u/strict-extend (class Field) test/WithTempDefaults {:with-temp-defaults (fn [_] {:database_type "VARCHAR"