From 7d6e031bb0c9d964ef7d55f41f1047c1304467d3 Mon Sep 17 00:00:00 2001 From: Anton Kulyk <kuliks.anton@gmail.com> Date: Fri, 17 Sep 2021 12:15:58 +0300 Subject: [PATCH] Feature | More Granular Caching Controls (Frontend) (#17802) * Rename dashboard details edit action * Use render props at dashboard edit form * Add enterprise caching plugin * Move dashboard entity forms to own file * Make FormField's margin configurable * Pass custom style to FormField * Use null as a default cacheTTLFormField value * Add tests for DashboardDetailsModal * Add cache_ttl field to DashboardDetailsModal * Add tests for CreateDashboardModal * Test cache_ttl is invisible when creating a dashboard * Remove not used mocks * Add tests for EditQuestionInfoModal * Move question entity forms to own file * Add cache_ttl field to EditQuestionInfoModal * Test cache_ttl is invisible when creating a question * Extract CacheTTLField component * Don't show cache field if caching is disabled * Fix typo * Update cache_ttl field look * Revert "Pass custom style to FormField" This reverts commit 3944ca8ff1dcad478f0f1c3a1973f454447c88c1. * Revert "Make FormField's margin configurable" This reverts commit ca7dc434532038da736012147f6889e6cc01ec11. * Fix CacheTTLField width * Add descriptionPosition prop to FormField * Add DatabaseCacheTTLField * Show cache_ttl field on database form * Use 24h as a default database cache TTL field value * Fallback to database cache_ttl in question form * Remove console.log * Fix request body assertion for CreateDashboardModal * Fix request body assertions for EditQuestionModal * Fix request body assertions for DashboardDetailsModal * Fix cache_ttl not included in `PUT /api/card/:id` * Use simple for for EditQuestionInfoModal * Refactor how unit tests mock caching settings * Add dashboard caching EE test * Add question caching EE test * Add caching e2e test for database add/edit * Remove debug stuff * Add error state to CacheTTLField * Fix DB cache field reverts to default value on 0 * Validate cache_ttl field * Add getQuestionsImplicitCacheTTL helper * Remove redundant tests * Remove not used import * Nomalize `cache_ttl: 0` to null * Fix duplicated card update request * Add QuestionCacheTTLField component * Add test-id to Radio's option names * Use a new cache TTL field for questions * Check setting cache_ttl to null in E2E tests * Style question cache TTL field * Add dashboard ID to query request body * Extend `duration` formatting helper to support hours * Fix default caching duration display * Fix tests * Fix old dashboard edit action title * Fix missing translations * Add prop types to EditQuestionInfoModal * Remove `test.todo` * Simplify `getQuestionsImplicitCacheTTL` * Use `space` style helper * Add `key` prop to `jt` * Move time coverting utils to lib/time * Hide caching field label if just showing the input * Fix unit tests * Fix flacky E2E test * Add hover style for "More options" control * Fix missing translation * Use placeholder for cache inputs * Fix E2E tests to use cache field's placeholder * Feature flag for caching UI --- .../CacheTTLField/CacheTTLField.jsx | 41 +++ .../CacheTTLField/CacheTTLField.styled.js | 33 +++ .../CacheTTLField/CacheTTLField.unit.spec.js | 64 +++++ .../caching/components/CacheTTLField/index.js | 1 + .../DatabaseCacheTTLField.jsx | 57 ++++ .../DatabaseCacheTTLField.styled.jsx | 12 + .../DatabaseCacheTTLField.unit.spec.js | 72 +++++ .../components/DatabaseCacheTTLField/index.js | 1 + .../QuestionCacheTTLField.jsx | 75 ++++++ .../QuestionCacheTTLField.styled.jsx | 21 ++ .../QuestionCacheTTLField.unit.spec.js | 149 +++++++++++ .../components/QuestionCacheTTLField/index.js | 1 + .../src/metabase-enterprise/caching/index.js | 51 ++++ .../src/metabase-enterprise/caching/utils.js | 56 ++++ .../caching/utils.unit.spec.js | 94 +++++++ .../src/metabase-enterprise/plugins.js | 1 + .../containers/DatabaseEditApp.unit.spec.js | 105 ++++++++ .../components/CreateDashboardModal.jsx | 1 + .../CreateDashboardModal.unit.spec.js | 170 ++++++++++++ frontend/src/metabase/components/Radio.jsx | 2 +- .../metabase/components/form/FormField.jsx | 11 +- frontend/src/metabase/dashboard/actions.js | 1 + .../components/DashboardDetailsModal.jsx | 37 ++- .../DashboardDetailsModal.unit.spec.js | 245 ++++++++++++++++++ .../dashboard/components/DashboardHeader.jsx | 2 +- frontend/src/metabase/entities/dashboards.js | 29 +-- .../src/metabase/entities/dashboards/forms.js | 48 ++++ .../src/metabase/entities/databases/forms.js | 124 +++++---- frontend/src/metabase/entities/questions.js | 36 +-- .../src/metabase/entities/questions/forms.js | 42 +++ frontend/src/metabase/lib/formatting.js | 17 +- frontend/src/metabase/lib/time.js | 17 ++ frontend/src/metabase/plugins/index.js | 7 + .../components/view/EditQuestionInfoModal.jsx | 92 +++++-- .../view/EditQuestionInfoModal.unit.spec.js | 219 ++++++++++++++++ .../containers/SaveQuestionModal.unit.spec.js | 59 +++++ frontend/test/metabase/lib/time.unit.spec.js | 56 ++++ .../scenarios/admin/databases/add.cy.spec.js | 54 +++- .../scenarios/admin/databases/edit.cy.spec.js | 46 +++- .../collections/items-metadata.cy.spec.js | 2 +- .../collections/permissions.cy.spec.js | 6 +- .../scenarios/dashboard/caching.cy.spec.js | 56 ++++ .../scenarios/question/caching.cy.spec.js | 56 ++++ .../scenarios/sharing/embed.cy.spec.js | 6 +- 44 files changed, 2125 insertions(+), 150 deletions(-) create mode 100644 enterprise/frontend/src/metabase-enterprise/caching/components/CacheTTLField/CacheTTLField.jsx create mode 100644 enterprise/frontend/src/metabase-enterprise/caching/components/CacheTTLField/CacheTTLField.styled.js create mode 100644 enterprise/frontend/src/metabase-enterprise/caching/components/CacheTTLField/CacheTTLField.unit.spec.js create mode 100644 enterprise/frontend/src/metabase-enterprise/caching/components/CacheTTLField/index.js create mode 100644 enterprise/frontend/src/metabase-enterprise/caching/components/DatabaseCacheTTLField/DatabaseCacheTTLField.jsx create mode 100644 enterprise/frontend/src/metabase-enterprise/caching/components/DatabaseCacheTTLField/DatabaseCacheTTLField.styled.jsx create mode 100644 enterprise/frontend/src/metabase-enterprise/caching/components/DatabaseCacheTTLField/DatabaseCacheTTLField.unit.spec.js create mode 100644 enterprise/frontend/src/metabase-enterprise/caching/components/DatabaseCacheTTLField/index.js create mode 100644 enterprise/frontend/src/metabase-enterprise/caching/components/QuestionCacheTTLField/QuestionCacheTTLField.jsx create mode 100644 enterprise/frontend/src/metabase-enterprise/caching/components/QuestionCacheTTLField/QuestionCacheTTLField.styled.jsx create mode 100644 enterprise/frontend/src/metabase-enterprise/caching/components/QuestionCacheTTLField/QuestionCacheTTLField.unit.spec.js create mode 100644 enterprise/frontend/src/metabase-enterprise/caching/components/QuestionCacheTTLField/index.js create mode 100644 enterprise/frontend/src/metabase-enterprise/caching/index.js create mode 100644 enterprise/frontend/src/metabase-enterprise/caching/utils.js create mode 100644 enterprise/frontend/src/metabase-enterprise/caching/utils.unit.spec.js create mode 100644 frontend/src/metabase/admin/databases/containers/DatabaseEditApp.unit.spec.js create mode 100644 frontend/src/metabase/components/CreateDashboardModal.unit.spec.js create mode 100644 frontend/src/metabase/dashboard/components/DashboardDetailsModal.unit.spec.js create mode 100644 frontend/src/metabase/entities/dashboards/forms.js create mode 100644 frontend/src/metabase/entities/questions/forms.js create mode 100644 frontend/src/metabase/query_builder/components/view/EditQuestionInfoModal.unit.spec.js create mode 100644 frontend/test/metabase/scenarios/dashboard/caching.cy.spec.js create mode 100644 frontend/test/metabase/scenarios/question/caching.cy.spec.js diff --git a/enterprise/frontend/src/metabase-enterprise/caching/components/CacheTTLField/CacheTTLField.jsx b/enterprise/frontend/src/metabase-enterprise/caching/components/CacheTTLField/CacheTTLField.jsx new file mode 100644 index 00000000000..e9ceac8762c --- /dev/null +++ b/enterprise/frontend/src/metabase-enterprise/caching/components/CacheTTLField/CacheTTLField.jsx @@ -0,0 +1,41 @@ +import React from "react"; +import PropTypes from "prop-types"; +import { t } from "ttag"; +import { formDomOnlyProps } from "metabase/lib/redux"; +import { + CacheTTLFieldContainer, + FieldText, + Input, +} from "./CacheTTLField.styled"; + +const propTypes = { + field: PropTypes.shape({ + name: PropTypes.string.isRequired, + value: PropTypes.number, + error: PropTypes.string, + }), + message: PropTypes.string, +}; + +export function CacheTTLField({ field, message, ...props }) { + const hasError = !!field.error; + return ( + <CacheTTLFieldContainer {...props}> + {message && ( + <FieldText margin="right" hasError={hasError}> + {message} + </FieldText> + )} + <Input + aria-labelledby={`${field.name}-label`} + {...formDomOnlyProps(field)} + value={field.value} + placeholder="24" + hasError={hasError} + /> + <FieldText margin="left" hasError={hasError}>{t`hours`}</FieldText> + </CacheTTLFieldContainer> + ); +} + +CacheTTLField.propTypes = propTypes; diff --git a/enterprise/frontend/src/metabase-enterprise/caching/components/CacheTTLField/CacheTTLField.styled.js b/enterprise/frontend/src/metabase-enterprise/caching/components/CacheTTLField/CacheTTLField.styled.js new file mode 100644 index 00000000000..40d1e32647f --- /dev/null +++ b/enterprise/frontend/src/metabase-enterprise/caching/components/CacheTTLField/CacheTTLField.styled.js @@ -0,0 +1,33 @@ +import styled, { css } from "styled-components"; +import { color } from "metabase/lib/colors"; +import NumericInput from "metabase/components/NumericInput"; + +export const CacheTTLFieldContainer = styled.div` + display: flex; + align-items: center; +`; + +export const FieldText = styled.span` + color: ${props => color(props.hasError ? "error" : "text-dark")}; + ${props => css`margin-${props.margin}: 10px;`} +`; + +export const Input = styled(NumericInput)` + width: 50px; + text-align: center; + + color: ${props => color(props.hasError ? "error" : "text-dark")}; + font-weight: bold; + padding: 0.75em; + + border: 1px solid ${color("border")}; + border-radius: 4px; + outline: none; + + :focus, + :hover { + border-color: ${color("brand")}; + } + + transition: border 300ms ease-in-out; +`; diff --git a/enterprise/frontend/src/metabase-enterprise/caching/components/CacheTTLField/CacheTTLField.unit.spec.js b/enterprise/frontend/src/metabase-enterprise/caching/components/CacheTTLField/CacheTTLField.unit.spec.js new file mode 100644 index 00000000000..1c2bc1a09ac --- /dev/null +++ b/enterprise/frontend/src/metabase-enterprise/caching/components/CacheTTLField/CacheTTLField.unit.spec.js @@ -0,0 +1,64 @@ +import React from "react"; +import { render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { CacheTTLField } from "./CacheTTLField"; + +function setup({ name = "cache_ttl", message, value }) { + const onChange = jest.fn(); + render( + <form> + <span id={`${name}-label`}>Label</span> + <CacheTTLField + field={{ name, value, onChange }} + message={message} + onChange={onChange} + /> + </form>, + ); + const field = screen.getByLabelText("Label"); + return { field, onChange }; +} + +describe("CacheTTLField", () => { + [ + { value: 0, expected: "0" }, + { value: 1, expected: "1" }, + { value: 12, expected: "12" }, + ].forEach(({ value, expected }) => { + it(`displays ${value} value as ${expected}`, () => { + const { field } = setup({ value }); + expect(field).toHaveValue(expected); + }); + }); + + it("displays a placeholder for null values", () => { + const { field } = setup({ value: null }); + + expect(field).toHaveAttribute("placeholder", "24"); + expect(field).toHaveValue(""); + }); + + it("displays message", () => { + setup({ message: "Cache results for" }); + expect(screen.queryByText("Cache results for")).toBeInTheDocument(); + }); + + it("calls onChange correctly", () => { + const { field, onChange } = setup({ value: 4 }); + + userEvent.clear(field); + userEvent.type(field, "14"); + field.blur(); + + expect(onChange).toHaveBeenLastCalledWith(14); + }); + + it("calls onChange with null value if input is cleared", () => { + const { field, onChange } = setup({ value: 4 }); + + userEvent.clear(field); + field.blur(); + + expect(onChange).toHaveBeenLastCalledWith(null); + }); +}); diff --git a/enterprise/frontend/src/metabase-enterprise/caching/components/CacheTTLField/index.js b/enterprise/frontend/src/metabase-enterprise/caching/components/CacheTTLField/index.js new file mode 100644 index 00000000000..35634c0a52f --- /dev/null +++ b/enterprise/frontend/src/metabase-enterprise/caching/components/CacheTTLField/index.js @@ -0,0 +1 @@ +export * from "./CacheTTLField"; diff --git a/enterprise/frontend/src/metabase-enterprise/caching/components/DatabaseCacheTTLField/DatabaseCacheTTLField.jsx b/enterprise/frontend/src/metabase-enterprise/caching/components/DatabaseCacheTTLField/DatabaseCacheTTLField.jsx new file mode 100644 index 00000000000..c0cda2d8ddd --- /dev/null +++ b/enterprise/frontend/src/metabase-enterprise/caching/components/DatabaseCacheTTLField/DatabaseCacheTTLField.jsx @@ -0,0 +1,57 @@ +import React, { useCallback, useEffect, useState } from "react"; +import PropTypes from "prop-types"; +import { t } from "ttag"; +import Select, { Option } from "metabase/components/Select"; +import { CacheTTLField } from "../CacheTTLField"; +import { + CacheFieldContainer, + FieldContainer, +} from "./DatabaseCacheTTLField.styled"; + +const MODE = { + INSTANCE_DEFAULT: "instance-default", + CUSTOM: "custom", +}; + +const INSTANCE_DEFAULT_CACHE_TTL = null; +const DEFAULT_CUSTOM_CACHE_TTL = 24; // hours + +const propTypes = { + field: PropTypes.object.isRequired, +}; + +export function DatabaseCacheTTLField({ field }) { + const [mode, setMode] = useState( + field.value > 0 ? MODE.CUSTOM : MODE.INSTANCE_DEFAULT, + ); + + const onModeChange = useCallback(e => { + setMode(e.target.value); + }, []); + + useEffect(() => { + if (mode === MODE.INSTANCE_DEFAULT) { + field.onChange(INSTANCE_DEFAULT_CACHE_TTL); + } else if (field.value == null) { + field.onChange(DEFAULT_CUSTOM_CACHE_TTL); + } + }, [field, mode]); + + return ( + <FieldContainer> + <Select value={mode} onChange={onModeChange}> + <Option value={MODE.INSTANCE_DEFAULT}> + {t`Use instance default (TTL)`} + </Option> + <Option value={MODE.CUSTOM}>{t`Custom`}</Option> + </Select> + {mode === MODE.CUSTOM && ( + <CacheFieldContainer> + <CacheTTLField field={field} /> + </CacheFieldContainer> + )} + </FieldContainer> + ); +} + +DatabaseCacheTTLField.propTypes = propTypes; diff --git a/enterprise/frontend/src/metabase-enterprise/caching/components/DatabaseCacheTTLField/DatabaseCacheTTLField.styled.jsx b/enterprise/frontend/src/metabase-enterprise/caching/components/DatabaseCacheTTLField/DatabaseCacheTTLField.styled.jsx new file mode 100644 index 00000000000..65f8eac58b5 --- /dev/null +++ b/enterprise/frontend/src/metabase-enterprise/caching/components/DatabaseCacheTTLField/DatabaseCacheTTLField.styled.jsx @@ -0,0 +1,12 @@ +import styled from "styled-components"; +import { space } from "metabase/styled-components/theme"; + +export const FieldContainer = styled.div` + display: flex; + flex-direction: row; + align-items: center; +`; + +export const CacheFieldContainer = styled.div` + margin-left: ${space(2)}; +`; diff --git a/enterprise/frontend/src/metabase-enterprise/caching/components/DatabaseCacheTTLField/DatabaseCacheTTLField.unit.spec.js b/enterprise/frontend/src/metabase-enterprise/caching/components/DatabaseCacheTTLField/DatabaseCacheTTLField.unit.spec.js new file mode 100644 index 00000000000..e4670251a82 --- /dev/null +++ b/enterprise/frontend/src/metabase-enterprise/caching/components/DatabaseCacheTTLField/DatabaseCacheTTLField.unit.spec.js @@ -0,0 +1,72 @@ +import React from "react"; +import { render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { DatabaseCacheTTLField } from "./DatabaseCacheTTLField"; + +function setup({ value = null } = {}) { + const onChange = jest.fn(); + render( + <DatabaseCacheTTLField field={{ name: "cache_ttl", value, onChange }} />, + ); + return { onChange }; +} + +function selectMode(nextMode) { + const currentModeLabel = + nextMode === "custom" ? "Use instance default (TTL)" : "Custom"; + const nextModeLabel = + nextMode === "instance-default" ? "Use instance default (TTL)" : "Custom"; + + userEvent.click(screen.getByText(currentModeLabel)); + userEvent.click(screen.getByText(nextModeLabel)); +} + +describe("DatabaseCacheTTLField", () => { + it("displays 'Use instance default' option when cache_ttl is null", () => { + setup({ value: null }); + expect( + screen.queryByText("Use instance default (TTL)"), + ).toBeInTheDocument(); + expect(screen.queryByLabelText("Cache TTL Field")).not.toBeInTheDocument(); + }); + + it("displays 'Use instance default' option when cache_ttl is 0", () => { + setup({ value: 0 }); + expect( + screen.queryByText("Use instance default (TTL)"), + ).toBeInTheDocument(); + expect(screen.queryByLabelText("Cache TTL Field")).not.toBeInTheDocument(); + }); + + it("sets 24 hours as a default TTL custom value", () => { + const { onChange } = setup(); + selectMode("custom"); + expect(onChange).toHaveBeenLastCalledWith(24); + }); + + it("can select and fill custom cache TTL value", () => { + const { onChange } = setup(); + + selectMode("custom"); + const input = screen.getByPlaceholderText("24"); + userEvent.type(input, "{selectall}{backspace}14"); + input.blur(); + + expect(onChange).toHaveBeenLastCalledWith(14); + }); + + it("displays input when cache_ttl has value", () => { + setup({ value: 4 }); + expect(screen.queryByDisplayValue("4")).toBeInTheDocument(); + expect(screen.queryByText("Custom")).toBeInTheDocument(); + expect( + screen.queryByText("Use instance default (TTL)"), + ).not.toBeInTheDocument(); + }); + + it("can reset cache_ttl to instance default", () => { + const { onChange } = setup({ value: 48 }); + selectMode("instance-default"); + expect(onChange).toHaveBeenLastCalledWith(null); + }); +}); diff --git a/enterprise/frontend/src/metabase-enterprise/caching/components/DatabaseCacheTTLField/index.js b/enterprise/frontend/src/metabase-enterprise/caching/components/DatabaseCacheTTLField/index.js new file mode 100644 index 00000000000..0e1d95230ac --- /dev/null +++ b/enterprise/frontend/src/metabase-enterprise/caching/components/DatabaseCacheTTLField/index.js @@ -0,0 +1 @@ +export * from "./DatabaseCacheTTLField"; diff --git a/enterprise/frontend/src/metabase-enterprise/caching/components/QuestionCacheTTLField/QuestionCacheTTLField.jsx b/enterprise/frontend/src/metabase-enterprise/caching/components/QuestionCacheTTLField/QuestionCacheTTLField.jsx new file mode 100644 index 00000000000..51ab91308c9 --- /dev/null +++ b/enterprise/frontend/src/metabase-enterprise/caching/components/QuestionCacheTTLField/QuestionCacheTTLField.jsx @@ -0,0 +1,75 @@ +import React, { useEffect, useMemo, useState } from "react"; +import { t } from "ttag"; +import PropTypes from "prop-types"; +import { duration } from "metabase/lib/formatting"; +import { getQuestionsImplicitCacheTTL } from "../../utils"; +import { + CacheTTLInput, + CacheTTLExpandedField, + StyledRadio, +} from "./QuestionCacheTTLField.styled"; + +const propTypes = { + field: PropTypes.shape({ + value: PropTypes.number, + onChange: PropTypes.func.isRequired, + }).isRequired, + question: PropTypes.object.isRequired, // metabase-lib's Question instance +}; + +const DEFAULT_CACHE_TTL = null; + +const MODE = { + DEFAULT: "default", + CUSTOM: "custom", +}; + +function getInitialMode(question, implicitCacheTTL) { + if (question.card().cache_ttl > 0 || !implicitCacheTTL) { + return MODE.CUSTOM; + } + return MODE.DEFAULT; +} + +export function QuestionCacheTTLField({ field, question, ...props }) { + const implicitCacheTTL = useMemo( + () => getQuestionsImplicitCacheTTL(question), + [question], + ); + + const [mode, setMode] = useState(getInitialMode(question, implicitCacheTTL)); + + useEffect(() => { + if (mode === MODE.DEFAULT) { + field.onChange(DEFAULT_CACHE_TTL); + } + }, [field, mode]); + + if (!implicitCacheTTL) { + return <CacheTTLInput field={field} />; + } + + // implicitCacheTTL is in seconds and duration works with milliseconds + const defaultCachingLabel = duration(implicitCacheTTL * 1000); + + return ( + <div {...props}> + <StyledRadio + value={mode} + onChange={val => setMode(val)} + options={[ + { + name: t`Use default` + ` (${defaultCachingLabel})`, + value: MODE.DEFAULT, + }, + { name: t`Custom`, value: MODE.CUSTOM }, + ]} + vertical + showButtons + /> + {mode === MODE.CUSTOM && <CacheTTLExpandedField field={field} />} + </div> + ); +} + +QuestionCacheTTLField.propTypes = propTypes; diff --git a/enterprise/frontend/src/metabase-enterprise/caching/components/QuestionCacheTTLField/QuestionCacheTTLField.styled.jsx b/enterprise/frontend/src/metabase-enterprise/caching/components/QuestionCacheTTLField/QuestionCacheTTLField.styled.jsx new file mode 100644 index 00000000000..1d00df64c42 --- /dev/null +++ b/enterprise/frontend/src/metabase-enterprise/caching/components/QuestionCacheTTLField/QuestionCacheTTLField.styled.jsx @@ -0,0 +1,21 @@ +import React from "react"; +import { t } from "ttag"; +import styled from "styled-components"; +import { space } from "metabase/styled-components/theme"; +import Radio from "metabase/components/Radio"; +import { CacheTTLField } from "../CacheTTLField"; + +export function CacheTTLInput(props) { + return <CacheTTLField {...props} message={t`Cache results for`} />; +} + +export const CacheTTLExpandedField = styled(CacheTTLInput)` + margin-left: 1.3rem; +`; + +export const StyledRadio = styled(Radio)` + li { + margin-top: ${space(0)}; + font-weight: bold; + } +`; diff --git a/enterprise/frontend/src/metabase-enterprise/caching/components/QuestionCacheTTLField/QuestionCacheTTLField.unit.spec.js b/enterprise/frontend/src/metabase-enterprise/caching/components/QuestionCacheTTLField/QuestionCacheTTLField.unit.spec.js new file mode 100644 index 00000000000..dda8007c75e --- /dev/null +++ b/enterprise/frontend/src/metabase-enterprise/caching/components/QuestionCacheTTLField/QuestionCacheTTLField.unit.spec.js @@ -0,0 +1,149 @@ +import React from "react"; +import { render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { msToMinutes, msToHours } from "metabase/lib/time"; +import MetabaseSettings from "metabase/lib/settings"; +import { QuestionCacheTTLField } from "./QuestionCacheTTLField"; + +const TEN_MINUTES = 10 * 60 * 1000; + +function setup({ + value = null, + avgQueryDuration, + databaseCacheTTL = null, + cacheTTLMultiplier, + minCacheThreshold, +} = {}) { + const onChange = jest.fn(); + + const spy = jest.spyOn(MetabaseSettings, "get"); + spy.mockImplementation(key => { + if (key === "enable-query-caching") { + return true; + } + if (key === "query-caching-ttl-ratio") { + return cacheTTLMultiplier; + } + if (key === "query-caching-min-ttl") { + return minCacheThreshold; + } + }); + + const question = { + card: () => ({ + average_query_time: avgQueryDuration, + cache_ttl: value, + }), + database: () => ({ + cache_ttl: databaseCacheTTL, + }), + }; + + render( + <form> + <span id="cache_ttl-label">Label</span> + <QuestionCacheTTLField + field={{ name: "cache_ttl", value, onChange }} + question={question} + /> + </form>, + ); + return { onChange, avgQueryDuration }; +} + +const DEFAULT_MODE_REGEXP = /Use default \([.0-9]+ hours\)/; + +function selectMode(nextMode) { + const currentModeLabel = + nextMode === "custom" ? DEFAULT_MODE_REGEXP : "Custom"; + const nextModeLabel = nextMode === "default" ? DEFAULT_MODE_REGEXP : "Custom"; + + userEvent.click(screen.getByText(currentModeLabel)); + userEvent.click(screen.getByText(nextModeLabel)); +} + +function fillValue(input, value) { + userEvent.clear(input); + userEvent.type(input, String(value)); + input.blur(); +} + +const DEFAULT_MODE_TEXT_TEST_ID = /radio-[0-9]+-default-name/; + +describe("QuestionCacheTTLField", () => { + it("displays a placeholder if question is not cached", () => { + setup(); + expect(screen.getByLabelText("Label")).toHaveAttribute("placeholder", "24"); + }); + + it("displays question's cache TTL value", () => { + setup({ value: 21 }); + expect(screen.getByLabelText("Label")).toHaveValue("21"); + }); + + it("displays default caching value if question is cached on a db level", () => { + setup({ databaseCacheTTL: 32 }); + expect(screen.queryByTestId(DEFAULT_MODE_TEXT_TEST_ID)).toHaveTextContent( + "Use default (32 hours)", + ); + }); + + it("displays default caching value if question is cached on an instance level", () => { + setup({ + avgQueryDuration: TEN_MINUTES, + minCacheThreshold: 0, + cacheTTLMultiplier: 100, + }); + const expectedTTL = Math.round(msToHours(TEN_MINUTES * 100)); + expect(screen.queryByTestId(DEFAULT_MODE_TEXT_TEST_ID)).toHaveTextContent( + `Use default (${expectedTTL} hours)`, + ); + }); + + it("handles if cache duration is in minutes", () => { + setup({ + avgQueryDuration: 14400, + minCacheThreshold: 0, + cacheTTLMultiplier: 100, + }); + const expectedTTL = Math.round(msToMinutes(14400 * 100)); + expect(screen.queryByTestId(DEFAULT_MODE_TEXT_TEST_ID)).toHaveTextContent( + `Use default (${expectedTTL} minutes)`, + ); + }); + + it("calls onChange correctly when filling the input", () => { + const { onChange } = setup(); + fillValue(screen.getByLabelText("Label"), 48); + expect(onChange).toHaveBeenLastCalledWith(48); + }); + + it("offers to provide custom cache TTL when question is cached on a db level", () => { + setup({ databaseCacheTTL: 32 }); + + expect(screen.queryByLabelText("Use default (32 hours)")).toBeChecked(); + expect(screen.queryByLabelText("Custom")).not.toBeChecked(); + }); + + it("allows to overwrite default caching with custom value", () => { + const { onChange } = setup({ databaseCacheTTL: 32 }); + + selectMode("custom"); + fillValue(screen.getByLabelText("Label"), 24); + + expect(onChange).toHaveBeenLastCalledWith(24); + }); + + it("offers to switch to default caching instead of a custom TTL", () => { + setup({ value: 24, databaseCacheTTL: 32 }); + + expect(screen.queryByLabelText("Use default (32 hours)")).not.toBeChecked(); + expect(screen.queryByLabelText("Custom")).toBeChecked(); + }); + + it("allows to switch to default caching instead of a custom TTL", () => { + const { onChange } = setup({ value: 24, databaseCacheTTL: 32 }); + selectMode("default"); + expect(onChange).toHaveBeenLastCalledWith(null); + }); +}); diff --git a/enterprise/frontend/src/metabase-enterprise/caching/components/QuestionCacheTTLField/index.js b/enterprise/frontend/src/metabase-enterprise/caching/components/QuestionCacheTTLField/index.js new file mode 100644 index 00000000000..bbc522fae52 --- /dev/null +++ b/enterprise/frontend/src/metabase-enterprise/caching/components/QuestionCacheTTLField/index.js @@ -0,0 +1 @@ +export * from "./QuestionCacheTTLField"; diff --git a/enterprise/frontend/src/metabase-enterprise/caching/index.js b/enterprise/frontend/src/metabase-enterprise/caching/index.js new file mode 100644 index 00000000000..68af6b6f0f1 --- /dev/null +++ b/enterprise/frontend/src/metabase-enterprise/caching/index.js @@ -0,0 +1,51 @@ +import React from "react"; +import { t, jt } from "ttag"; +import { hasPremiumFeature } from "metabase-enterprise/settings"; +import { PLUGIN_CACHING, PLUGIN_FORM_WIDGETS } from "metabase/plugins"; +import Link from "metabase/components/Link"; +import { CacheTTLField } from "./components/CacheTTLField"; +import { DatabaseCacheTTLField } from "./components/DatabaseCacheTTLField"; +import { QuestionCacheTTLField } from "./components/QuestionCacheTTLField"; +import { + getQuestionsImplicitCacheTTL, + validateCacheTTL, + normalizeCacheTTL, +} from "./utils"; + +function getDatabaseCacheTTLFieldDescription() { + return ( + <span> + {jt`How long to keep question results. By default, Metabase will use the value you supply on the ${( + <Link + key="caching-link" + className="text-brand" + href="/admin/settings/caching" + >{t`cache settings page`}</Link> + )}, but if this database has other factors that influence the freshness of data, it could make sense to set a custom duration. You can also choose custom durations on individual questions or dashboards to help improve performance.`} + </span> + ); +} + +if (hasPremiumFeature("advanced_config")) { + PLUGIN_CACHING.cacheTTLFormField = { + name: "cache_ttl", + validate: validateCacheTTL, + normalize: normalizeCacheTTL, + }; + + PLUGIN_CACHING.databaseCacheTTLFormField = { + name: "cache_ttl", + type: "databaseCacheTTL", + title: t`Default result cache duration`, + description: getDatabaseCacheTTLFieldDescription(), + descriptionPosition: "bottom", + validate: validateCacheTTL, + normalize: normalizeCacheTTL, + }; + + PLUGIN_FORM_WIDGETS.dashboardCacheTTL = CacheTTLField; + PLUGIN_FORM_WIDGETS.databaseCacheTTL = DatabaseCacheTTLField; + PLUGIN_FORM_WIDGETS.questionCacheTTL = QuestionCacheTTLField; + + PLUGIN_CACHING.getQuestionsImplicitCacheTTL = getQuestionsImplicitCacheTTL; +} diff --git a/enterprise/frontend/src/metabase-enterprise/caching/utils.js b/enterprise/frontend/src/metabase-enterprise/caching/utils.js new file mode 100644 index 00000000000..525ff74b75e --- /dev/null +++ b/enterprise/frontend/src/metabase-enterprise/caching/utils.js @@ -0,0 +1,56 @@ +import { t } from "ttag"; +import { msToSeconds } from "metabase/lib/time"; +import MetabaseSettings from "metabase/lib/settings"; + +/** + * If a question doesn't have an explicitly set cache TTL, + * its results can still be cached with a db-level cache TTL + * or with an instance level setting + * + * More on caching: + * https://www.metabase.com/docs/latest/administration-guide/14-caching.html + * + * @param {Question} metabase-lib Question instance + * @returns {number} — cache TTL value in seconds (from db or instance default) that will be used + */ +export function getQuestionsImplicitCacheTTL(question) { + if (!MetabaseSettings.get("enable-query-caching")) { + return null; + } + if (question.database().cache_ttl) { + // Database's cache TTL is in hours, need to convert that to seconds + return question.database().cache_ttl * 60 * 60; + } + const avgQueryDurationInSeconds = msToSeconds( + question.card().average_query_time, + ); + if (checkQuestionWillBeCached(avgQueryDurationInSeconds)) { + return calcQuestionMagicCacheDuration(avgQueryDurationInSeconds); + } + return null; +} + +function checkQuestionWillBeCached(avgQueryDurationInSeconds) { + const minQueryDurationThresholdSeconds = MetabaseSettings.get( + "query-caching-min-ttl", + ); + return avgQueryDurationInSeconds > minQueryDurationThresholdSeconds; +} + +function calcQuestionMagicCacheDuration(avgQueryDurationInSeconds) { + const cacheTTLMultiplier = MetabaseSettings.get("query-caching-ttl-ratio"); + return avgQueryDurationInSeconds * cacheTTLMultiplier; +} + +export function validateCacheTTL(value) { + if (value === null) { + return; + } + if (!Number.isSafeInteger(value) || value < 0) { + return t`Must be a positive integer value`; + } +} + +export function normalizeCacheTTL(value) { + return value === 0 ? null : value; +} diff --git a/enterprise/frontend/src/metabase-enterprise/caching/utils.unit.spec.js b/enterprise/frontend/src/metabase-enterprise/caching/utils.unit.spec.js new file mode 100644 index 00000000000..9b73ebab870 --- /dev/null +++ b/enterprise/frontend/src/metabase-enterprise/caching/utils.unit.spec.js @@ -0,0 +1,94 @@ +import { msToSeconds, hoursToSeconds } from "metabase/lib/time"; +import MetabaseSettings from "metabase/lib/settings"; +import { getQuestionsImplicitCacheTTL, validateCacheTTL } from "./utils"; + +describe("validateCacheTTL", () => { + const validTestCases = [null, 0, 1, 6, 42]; + const invalidTestCases = [-1, -1.2, 0.5, 4.3]; + + validTestCases.forEach(value => { + it(`should be valid for ${value}`, () => { + expect(validateCacheTTL(value)).toBe(undefined); + }); + }); + + invalidTestCases.forEach(value => { + it(`should return error for ${value}`, () => { + expect(validateCacheTTL(value)).toBe("Must be a positive integer value"); + }); + }); +}); + +describe("getQuestionsImplicitCacheTTL", () => { + const TEN_MINUTES = 10 * 60 * 1000; + const DEFAULT_CACHE_TTL_MULTIPLIER = 10; + + function setup({ + cachingEnabled = true, + avgQueryTime = null, + databaseCacheTTL = null, + cacheTTLMultiplier = DEFAULT_CACHE_TTL_MULTIPLIER, + minCacheThreshold = 60, + } = {}) { + const spy = jest.spyOn(MetabaseSettings, "get"); + spy.mockImplementation(key => { + if (key === "enable-query-caching") { + return cachingEnabled; + } + if (key === "query-caching-ttl-ratio") { + return cachingEnabled ? cacheTTLMultiplier : null; + } + if (key === "query-caching-min-ttl") { + return cachingEnabled ? minCacheThreshold : null; + } + }); + + return { + card: () => ({ + average_query_time: avgQueryTime, + }), + database: () => ({ + cache_ttl: databaseCacheTTL, + }), + }; + } + + it("returns database's cache TTL if set", () => { + const question = setup({ databaseCacheTTL: 10 }); + expect(getQuestionsImplicitCacheTTL(question)).toBe(hoursToSeconds(10)); + }); + + it("returns 'magic TTL' if there is no prior caching strategy", () => { + const question = setup({ avgQueryTime: TEN_MINUTES }); + + expect(getQuestionsImplicitCacheTTL(question)).toBe( + msToSeconds(TEN_MINUTES * DEFAULT_CACHE_TTL_MULTIPLIER), + ); + }); + + it("returns null if instance-level caching enabled, but the query doesn't pass the min exec time threshold", () => { + const question = setup({ + avgQueryTime: TEN_MINUTES, + minCacheThreshold: TEN_MINUTES * 2, + }); + expect(getQuestionsImplicitCacheTTL(question)).toBe(null); + }); + + it("prefers database cache TTL over instance-level one", () => { + const question = setup({ databaseCacheTTL: 10, avgQueryTime: TEN_MINUTES }); + expect(getQuestionsImplicitCacheTTL(question)).toBe(hoursToSeconds(10)); + }); + + it("returns null if caching disabled, but instance level caching parameters are present", () => { + const question = setup({ + avgQueryTime: TEN_MINUTES, + cachingEnabled: false, + }); + expect(getQuestionsImplicitCacheTTL(question)).toBe(null); + }); + + it("returns null if caching disabled, but database has a cache ttl", () => { + const question = setup({ databaseCacheTTL: 10, cachingEnabled: false }); + expect(getQuestionsImplicitCacheTTL(question)).toBe(null); + }); +}); diff --git a/enterprise/frontend/src/metabase-enterprise/plugins.js b/enterprise/frontend/src/metabase-enterprise/plugins.js index 6c63d223ffa..98cf731291f 100644 --- a/enterprise/frontend/src/metabase-enterprise/plugins.js +++ b/enterprise/frontend/src/metabase-enterprise/plugins.js @@ -13,6 +13,7 @@ import "./audit_app"; import "./tools"; import "./sandboxes"; import "./auth"; +import "./caching"; import "./collections"; import "./whitelabel"; import "./embedding"; diff --git a/frontend/src/metabase/admin/databases/containers/DatabaseEditApp.unit.spec.js b/frontend/src/metabase/admin/databases/containers/DatabaseEditApp.unit.spec.js new file mode 100644 index 00000000000..1e61ab53958 --- /dev/null +++ b/frontend/src/metabase/admin/databases/containers/DatabaseEditApp.unit.spec.js @@ -0,0 +1,105 @@ +import React from "react"; +import { Provider } from "react-redux"; +import { reducer as form } from "redux-form"; +import { Router, Route } from "react-router"; +import { createMemoryHistory } from "history"; +import { + render, + screen, + waitForElementToBeRemoved, +} from "@testing-library/react"; +import admin from "metabase/admin/admin"; +import MetabaseSettings from "metabase/lib/settings"; +import { PLUGIN_CACHING } from "metabase/plugins"; +import { getStore } from "__support__/entities-store"; +import DatabaseEditApp from "./DatabaseEditApp"; + +const ENGINES_MOCK = { + h2: { + "details-fields": [ + { "display-name": "Connection String", name: "db", required: true }, + ], + "driver-name": "H2", + "superseded-by": null, + }, + sqlite: { + "details-fields": [ + { "display-name": "Filename", name: "db", required: true }, + ], + "driver-name": "SQLite", + "superseded-by": null, + }, +}; + +function mockSettings({ cachingEnabled = false }) { + const spy = jest.spyOn(MetabaseSettings, "get"); + spy.mockImplementation(key => { + if (key === "engines") { + return ENGINES_MOCK; + } + if (key === "enable-query-caching") { + return cachingEnabled; + } + if (key === "site-url") { + return "http://localhost:3333"; + } + }); +} + +async function setup({ cachingEnabled = false } = {}) { + mockSettings({ cachingEnabled }); + + render( + <Provider store={getStore({ admin, form })}> + <Router history={createMemoryHistory()}> + <Route path="/" component={DatabaseEditApp} /> + </Router> + </Provider>, + ); + + await waitForElementToBeRemoved(() => screen.queryByText("Loading...")); +} + +describe("DatabaseEditApp", () => { + describe("Cache TTL field", () => { + describe("OSS", () => { + it("is invisible", async () => { + await setup({ cachingEnabled: true }); + + expect( + screen.queryByText("Default result cache duration"), + ).not.toBeInTheDocument(); + }); + }); + + describe("EE", () => { + beforeEach(() => { + PLUGIN_CACHING.databaseCacheTTLFormField = { + name: "cache_ttl", + type: "integer", + title: "Default result cache duration", + }; + }); + + afterEach(() => { + PLUGIN_CACHING.databaseCacheTTLFormField = null; + }); + + it("is visible", async () => { + await setup({ cachingEnabled: true }); + + expect( + screen.queryByText("Default result cache duration"), + ).toBeInTheDocument(); + }); + + it("is invisible when caching disabled", async () => { + await setup({ cachingEnabled: false }); + + expect( + screen.queryByText("Default result cache duration"), + ).not.toBeInTheDocument(); + }); + }); + }); +}); diff --git a/frontend/src/metabase/components/CreateDashboardModal.jsx b/frontend/src/metabase/components/CreateDashboardModal.jsx index b4854717fcd..113e24d9900 100644 --- a/frontend/src/metabase/components/CreateDashboardModal.jsx +++ b/frontend/src/metabase/components/CreateDashboardModal.jsx @@ -44,6 +44,7 @@ export default class CreateDashboardModal extends Component { const { initialCollectionId, onSaved, onClose } = this.props; return ( <Dashboard.ModalForm + form={Dashboard.forms.create} overwriteOnInitialValuesChange dashboard={{ collection_id: initialCollectionId }} onClose={onClose} diff --git a/frontend/src/metabase/components/CreateDashboardModal.unit.spec.js b/frontend/src/metabase/components/CreateDashboardModal.unit.spec.js new file mode 100644 index 00000000000..2875ea7a1d2 --- /dev/null +++ b/frontend/src/metabase/components/CreateDashboardModal.unit.spec.js @@ -0,0 +1,170 @@ +import React from "react"; +import { Provider } from "react-redux"; +import { reducer as form } from "redux-form"; +import { fireEvent, render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import xhrMock from "xhr-mock"; +import { getStore } from "__support__/entities-store"; +import MetabaseSettings from "metabase/lib/settings"; +import { PLUGIN_CACHING } from "metabase/plugins"; +import CreateDashboardModal from "./CreateDashboardModal"; + +function mockCachingEnabled(enabled = true) { + const original = MetabaseSettings.get; + const spy = jest.spyOn(MetabaseSettings, "get"); + spy.mockImplementation(key => { + if (key === "enable-query-caching") { + return enabled; + } + return original(key); + }); +} + +function setup({ mockCreateDashboardResponse = true } = {}) { + const onClose = jest.fn(); + + if (mockCreateDashboardResponse) { + xhrMock.post(`/api/dashboard`, (req, res) => + res.status(200).body(req.body()), + ); + } + + render( + <Provider store={getStore({ form })}> + <CreateDashboardModal onClose={onClose} /> + </Provider>, + ); + + return { + onClose, + }; +} + +function setupCreateRequestAssertion(doneCallback, changedValues) { + xhrMock.post("/api/dashboard", req => { + try { + expect(JSON.parse(req.body())).toEqual({ + ...changedValues, + collection_id: null, + }); + doneCallback(); + } catch (err) { + doneCallback(err); + } + }); +} + +function fillForm({ name, description } = {}) { + const nextDashboardState = {}; + if (name) { + const input = screen.getByLabelText("Name"); + userEvent.clear(input); + userEvent.type(input, name); + nextDashboardState.name = name; + } + if (description) { + const input = screen.getByLabelText("Description"); + userEvent.clear(input); + userEvent.type(input, description); + nextDashboardState.description = description; + } + return nextDashboardState; +} + +describe("CreateDashboardModal", () => { + beforeEach(() => { + xhrMock.setup(); + xhrMock.get("/api/collection", { + body: JSON.stringify([ + { + id: "root", + name: "Our analytics", + can_write: true, + }, + ]), + }); + }); + + afterEach(() => { + xhrMock.teardown(); + }); + + it("displays empty form fields", () => { + setup(); + + expect(screen.queryByLabelText("Name")).toBeInTheDocument(); + expect(screen.queryByLabelText("Name")).toHaveValue(""); + + expect(screen.queryByLabelText("Description")).toBeInTheDocument(); + expect(screen.queryByLabelText("Description")).toHaveValue(""); + + expect(screen.queryByText("Our analytics")).toBeInTheDocument(); + + expect( + screen.queryByRole("button", { name: "Create" }), + ).toBeInTheDocument(); + expect( + screen.queryByRole("button", { name: "Create" }), + ).toBeInTheDocument(); + }); + + it("can't submit if name is empty", () => { + setup(); + expect(screen.queryByRole("button", { name: "Create" })).toBeDisabled(); + }); + + it("calls onClose when Cancel button is clicked", () => { + const { onClose } = setup(); + fireEvent.click(screen.queryByRole("button", { name: "Cancel" })); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it("submits a create request correctly", done => { + const FORM = { + name: "New fancy dashboard", + description: "Just testing the form", + }; + setupCreateRequestAssertion(done, FORM); + setup({ mockCreateDashboardResponse: false }); + + fillForm(FORM); + fireEvent.click(screen.queryByRole("button", { name: "Create" })); + }); + + describe("Cache TTL field", () => { + beforeEach(() => { + mockCachingEnabled(); + }); + + describe("OSS", () => { + it("is not shown", () => { + setup(); + expect(screen.queryByText("More options")).not.toBeInTheDocument(); + expect( + screen.queryByText("Cache all question results for"), + ).not.toBeInTheDocument(); + }); + }); + + describe("EE", () => { + beforeEach(() => { + PLUGIN_CACHING.cacheTTLFormField = { + name: "cache_ttl", + type: "integer", + }; + }); + + afterEach(() => { + PLUGIN_CACHING.cacheTTLFormField = null; + }); + + it("is not shown", () => { + setup(); + expect(screen.queryByText("More options")).not.toBeInTheDocument(); + expect( + screen.queryByText("Cache all question results for"), + ).not.toBeInTheDocument(); + }); + }); + }); +}); diff --git a/frontend/src/metabase/components/Radio.jsx b/frontend/src/metabase/components/Radio.jsx index e5dc2e036f3..7a81c2ff73c 100644 --- a/frontend/src/metabase/components/Radio.jsx +++ b/frontend/src/metabase/components/Radio.jsx @@ -133,7 +133,7 @@ function Radio({ aria-labelledby={labelId} /> {showButtons && <RadioButton checked={selected} />} - {optionNameFn(option)} + <span data-testid={`${id}-name`}>{optionNameFn(option)}</span> </Item> </li> ); diff --git a/frontend/src/metabase/components/form/FormField.jsx b/frontend/src/metabase/components/form/FormField.jsx index bd7c2909117..68ddad3646c 100644 --- a/frontend/src/metabase/components/form/FormField.jsx +++ b/frontend/src/metabase/components/form/FormField.jsx @@ -9,6 +9,7 @@ import { FieldRow, Label, InfoIcon, InputContainer } from "./FormField.styled"; const formFieldCommon = { title: PropTypes.string, description: PropTypes.string, + descriptionPosition: PropTypes.oneOf(["top", "bottom"]), info: PropTypes.string, hidden: PropTypes.bool, horizontal: PropTypes.bool, @@ -44,6 +45,9 @@ function FormField(props) { formField, title = formField && formField.title, description = formField && formField.description, + descriptionPosition = descriptionPosition || + (formField && formField.descriptionPosition) || + "top", info = formField && formField.info, hidden = formField && (formField.hidden || formField.type === "hidden"), horizontal = formField && @@ -93,10 +97,15 @@ function FormField(props) { </Tooltip> )} </FieldRow> - {description && <div className="mb1">{description}</div>} + {description && descriptionPosition === "top" && ( + <div className="mb1">{description}</div> + )} </div> )} <InputContainer horizontal={horizontal}>{children}</InputContainer> + {description && descriptionPosition === "bottom" && ( + <div className="mt1">{description}</div> + )} </div> ); } diff --git a/frontend/src/metabase/dashboard/actions.js b/frontend/src/metabase/dashboard/actions.js index 130a8ce8325..5e339a230de 100644 --- a/frontend/src/metabase/dashboard/actions.js +++ b/frontend/src/metabase/dashboard/actions.js @@ -611,6 +611,7 @@ export const fetchCardData = createThunkAction(FETCH_CARD_DATA, function( cardId: card.id, parameters: datasetQuery.parameters, ignore_cache: ignoreCache, + dashboard_id: dashcard.dashboard_id, }, queryOptions, ), diff --git a/frontend/src/metabase/dashboard/components/DashboardDetailsModal.jsx b/frontend/src/metabase/dashboard/components/DashboardDetailsModal.jsx index 6fde9e44798..90233161017 100644 --- a/frontend/src/metabase/dashboard/components/DashboardDetailsModal.jsx +++ b/frontend/src/metabase/dashboard/components/DashboardDetailsModal.jsx @@ -2,11 +2,13 @@ import React from "react"; import { withRouter } from "react-router"; import { connect } from "react-redux"; +import _ from "underscore"; import * as Urls from "metabase/lib/urls"; import { t } from "ttag"; import Dashboard from "metabase/entities/dashboards"; +import CollapseSection from "metabase/components/CollapseSection"; import { setDashboardAttributes } from "../actions"; import { getDashboardComplete } from "../selectors"; @@ -17,6 +19,8 @@ const mapStateToProps = (state, props) => ({ const mapDispatchToProps = { setDashboardAttributes }; +const COLLAPSED_FIELDS = ["cache_ttl"]; + @withRouter @connect( mapStateToProps, @@ -33,7 +37,8 @@ class DashboardDetailsModal extends React.Component { } = this.props; return ( <Dashboard.ModalForm - title={t`Change title and description`} + title={t`Edit dashboard details`} + form={Dashboard.forms.edit} dashboard={dashboard} onClose={onClose} onSaved={dashboard => { @@ -45,7 +50,35 @@ class DashboardDetailsModal extends React.Component { }} overwriteOnInitialValuesChange {...props} - /> + > + {({ Form, FormField, FormFooter, formFields, onClose }) => { + const [visibleFields, collapsedFields] = _.partition( + formFields, + field => !COLLAPSED_FIELDS.includes(field.name), + ); + return ( + <Form> + {visibleFields.map(field => ( + <FormField key={field.name} name={field.name} /> + ))} + {collapsedFields.length > 0 && ( + <CollapseSection + header={t`More options`} + iconVariant="up-down" + iconPosition="right" + headerClass="text-bold text-medium text-brand-hover" + bodyClass="pt1" + > + {collapsedFields.map(field => ( + <FormField key={field.name} name={field.name} /> + ))} + </CollapseSection> + )} + <FormFooter submitTitle={t`Update`} onCancel={onClose} /> + </Form> + ); + }} + </Dashboard.ModalForm> ); } } diff --git a/frontend/src/metabase/dashboard/components/DashboardDetailsModal.unit.spec.js b/frontend/src/metabase/dashboard/components/DashboardDetailsModal.unit.spec.js new file mode 100644 index 00000000000..e65613dc775 --- /dev/null +++ b/frontend/src/metabase/dashboard/components/DashboardDetailsModal.unit.spec.js @@ -0,0 +1,245 @@ +import React from "react"; +import _ from "underscore"; +import { Provider } from "react-redux"; +import { reducer as form } from "redux-form"; +import { fireEvent, render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import xhrMock from "xhr-mock"; +import { getStore } from "__support__/entities-store"; +import MetabaseSettings from "metabase/lib/settings"; +import { PLUGIN_CACHING, PLUGIN_FORM_WIDGETS } from "metabase/plugins"; +import NumericFormField from "metabase/components/form/widgets/FormNumericInputWidget"; +import DashboardDetailsModal from "./DashboardDetailsModal"; + +const DASHBOARD = { + id: 1, + name: "Dashboard", + description: "I'm here for your unit tests", + cache_ttl: 0, + collection_id: null, + ordered_cards: [], + archived: false, +}; + +function mockCachingEnabled(enabled = true) { + const original = MetabaseSettings.get; + const spy = jest.spyOn(MetabaseSettings, "get"); + spy.mockImplementation(key => { + if (key === "enable-query-caching") { + return enabled; + } + return original(key); + }); +} + +function setup({ mockDashboardUpdateResponse = true } = {}) { + const onClose = jest.fn(); + + if (mockDashboardUpdateResponse) { + xhrMock.put(`/api/dashboard/${DASHBOARD.id}`, (req, res) => + res.status(200).body(req.body()), + ); + } + + const dashboardReducer = () => ({ + dashboardId: DASHBOARD.id, + dashboards: { + [DASHBOARD.id]: DASHBOARD, + }, + }); + + render( + <Provider store={getStore({ form, dashboard: dashboardReducer })}> + <DashboardDetailsModal onClose={onClose} /> + </Provider>, + ); + + return { + onClose, + }; +} + +function setupUpdateRequestAssertion( + doneCallback, + changedValues, + { hasCacheTTLField = false } = {}, +) { + const editableFields = ["name", "description", "collection_id"]; + if (hasCacheTTLField) { + editableFields.push("cache_ttl"); + } + xhrMock.put(`/api/dashboard/${DASHBOARD.id}`, req => { + try { + expect(JSON.parse(req.body())).toEqual({ + ..._.pick(DASHBOARD, ...editableFields), + ...changedValues, + }); + doneCallback(); + } catch (err) { + doneCallback(err); + } + }); +} + +function fillForm({ name, description, cache_ttl } = {}) { + const nextDashboardState = { ...DASHBOARD }; + if (name) { + const input = screen.getByLabelText("Name"); + userEvent.clear(input); + userEvent.type(input, name); + nextDashboardState.name = name; + } + if (description) { + const input = screen.getByLabelText("Description"); + userEvent.clear(input); + userEvent.type(input, description); + nextDashboardState.description = description; + } + if (cache_ttl) { + const input = screen.getByLabelText("Cache TTL"); + userEvent.clear(input); + userEvent.type(input, String(cache_ttl)); + input.blur(); + nextDashboardState.cache_ttl = cache_ttl; + } + return nextDashboardState; +} + +describe("DashboardDetailsModal", () => { + beforeEach(() => { + xhrMock.setup(); + xhrMock.get("/api/collection", { + body: JSON.stringify([ + { + id: "root", + name: "Our analytics", + can_write: true, + }, + ]), + }); + }); + + afterEach(() => { + xhrMock.teardown(); + }); + + it("displays fields with filled values", () => { + setup(); + + expect(screen.queryByLabelText("Name")).toBeInTheDocument(); + expect(screen.queryByLabelText("Name")).toHaveValue(DASHBOARD.name); + + expect(screen.queryByLabelText("Description")).toBeInTheDocument(); + expect(screen.queryByLabelText("Description")).toHaveValue( + DASHBOARD.description, + ); + + expect(screen.queryByText("Our analytics")).toBeInTheDocument(); + + expect( + screen.queryByRole("button", { name: "Update" }), + ).toBeInTheDocument(); + expect( + screen.queryByRole("button", { name: "Cancel" }), + ).toBeInTheDocument(); + }); + + it("calls onClose when Cancel button is clicked", () => { + const { onClose } = setup(); + fireEvent.click(screen.queryByRole("button", { name: "Cancel" })); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it("can't submit if name is empty", () => { + setup(); + + fillForm({ name: "" }); + fireEvent.click(screen.queryByRole("button", { name: "Update" })); + + expect(screen.queryByRole("button", { name: "Update" })).toBeDisabled(); + }); + + it("submits an update request correctly", done => { + const UPDATES = { + name: "New fancy dashboard name", + description: "Just testing if updates work correctly", + }; + setup({ mockDashboardUpdateResponse: false }); + setupUpdateRequestAssertion(done, UPDATES); + + fillForm(UPDATES); + fireEvent.click(screen.queryByRole("button", { name: "Update" })); + }); + + describe("Cache TTL field", () => { + describe("OSS", () => { + it("is not shown", () => { + mockCachingEnabled(); + setup(); + expect(screen.queryByText("More options")).not.toBeInTheDocument(); + expect( + screen.queryByText("Cache all question results for"), + ).not.toBeInTheDocument(); + }); + }); + + describe("EE", () => { + beforeEach(() => { + PLUGIN_CACHING.cacheTTLFormField = { + name: "cache_ttl", + title: "Cache TTL", + }; + PLUGIN_FORM_WIDGETS.dashboardCacheTTL = NumericFormField; + }); + + afterEach(() => { + PLUGIN_CACHING.cacheTTLFormField = null; + PLUGIN_FORM_WIDGETS.dashboardCacheTTL = null; + }); + + describe("caching enabled", () => { + beforeEach(() => { + mockCachingEnabled(); + }); + + it("is shown", () => { + setup(); + fireEvent.click(screen.queryByText("More options")); + expect(screen.queryByLabelText("Cache TTL")).toHaveValue("0"); + }); + + it("can be changed", done => { + setup({ mockDashboardUpdateResponse: false }); + setupUpdateRequestAssertion( + done, + { cache_ttl: 10 }, + { hasCacheTTLField: true }, + ); + + fireEvent.click(screen.queryByText("More options")); + fillForm({ cache_ttl: 10 }); + fireEvent.click(screen.queryByRole("button", { name: "Update" })); + }); + }); + + describe("caching disabled", () => { + it("is not shown if caching is disabled", () => { + mockCachingEnabled(false); + setup(); + expect(screen.queryByText("More options")).not.toBeInTheDocument(); + expect( + screen.queryByText("Cache all question results for"), + ).not.toBeInTheDocument(); + }); + + it("can still submit the form", done => { + setup({ mockDashboardUpdateResponse: false }); + setupUpdateRequestAssertion(done, { name: "Test" }); + + fillForm({ name: "Test" }); + fireEvent.click(screen.queryByRole("button", { name: "Update" })); + }); + }); + }); + }); +}); diff --git a/frontend/src/metabase/dashboard/components/DashboardHeader.jsx b/frontend/src/metabase/dashboard/components/DashboardHeader.jsx index d0ac2bf67f1..e192838490a 100644 --- a/frontend/src/metabase/dashboard/components/DashboardHeader.jsx +++ b/frontend/src/metabase/dashboard/components/DashboardHeader.jsx @@ -305,7 +305,7 @@ export default class DashboardHeader extends Component { to={location.pathname + "/details"} data-metabase-event={"Dashboard;EditDetails"} > - {t`Change title and description`} + {t`Edit dashboard details`} </Link>, ); } diff --git a/frontend/src/metabase/entities/dashboards.js b/frontend/src/metabase/entities/dashboards.js index c7c011d1359..984b730019d 100644 --- a/frontend/src/metabase/entities/dashboards.js +++ b/frontend/src/metabase/entities/dashboards.js @@ -20,6 +20,8 @@ import { normalizedCollection, } from "metabase/entities/collections"; +import forms from "./dashboards/forms"; + const FAVORITE_ACTION = `metabase/entities/dashboards/FAVORITE`; const UNFAVORITE_ACTION = `metabase/entities/dashboards/UNFAVORITE`; const COPY_ACTION = `metabase/entities/dashboards/COPY`; @@ -137,35 +139,12 @@ const Dashboards = createEntity({ getColor: () => color("dashboard"), }, - form: { - fields: [ - { - name: "name", - title: t`Name`, - placeholder: t`What is the name of your dashboard?`, - autoFocus: true, - validate: name => (!name ? "Name is required" : null), - }, - { - name: "description", - title: t`Description`, - type: "text", - placeholder: t`It's optional but oh, so helpful`, - }, - { - name: "collection_id", - title: t`Which collection should this go in?`, - type: "collection", - validate: collectionId => - collectionId === undefined ? "Collection is required" : null, - }, - ], - }, - getAnalyticsMetadata([object], { action }, getState) { const type = object && getCollectionType(object.collection_id, getState()); return type && `collection=${type}`; }, + + forms, }); export default Dashboards; diff --git a/frontend/src/metabase/entities/dashboards/forms.js b/frontend/src/metabase/entities/dashboards/forms.js new file mode 100644 index 00000000000..a94d58701ba --- /dev/null +++ b/frontend/src/metabase/entities/dashboards/forms.js @@ -0,0 +1,48 @@ +import { t } from "ttag"; +import MetabaseSettings from "metabase/lib/settings"; +import { PLUGIN_CACHING } from "metabase/plugins"; + +const FORM_FIELDS = [ + { + name: "name", + title: t`Name`, + placeholder: t`What is the name of your dashboard?`, + autoFocus: true, + validate: name => (!name ? t`Name is required` : null), + }, + { + name: "description", + title: t`Description`, + type: "text", + placeholder: t`It's optional but oh, so helpful`, + }, + { + name: "collection_id", + title: t`Which collection should this go in?`, + type: "collection", + validate: collectionId => + collectionId === undefined ? t`Collection is required` : null, + }, +]; + +export default { + create: { + fields: FORM_FIELDS, + }, + edit: { + fields: () => { + const fields = [...FORM_FIELDS]; + if ( + MetabaseSettings.get("enable-query-caching") && + PLUGIN_CACHING.cacheTTLFormField + ) { + fields.push({ + ...PLUGIN_CACHING.cacheTTLFormField, + type: "dashboardCacheTTL", + message: t`Cache all question results for`, + }); + } + return fields; + }, + }, +}; diff --git a/frontend/src/metabase/entities/databases/forms.js b/frontend/src/metabase/entities/databases/forms.js index 9ac6a6b9a6c..fc2e571ffa5 100644 --- a/frontend/src/metabase/entities/databases/forms.js +++ b/frontend/src/metabase/entities/databases/forms.js @@ -3,6 +3,7 @@ import { t, jt } from "ttag"; import MetabaseSettings from "metabase/lib/settings"; import ExternalLink from "metabase/components/ExternalLink"; +import { PLUGIN_CACHING } from "metabase/plugins"; import getFieldsForBigQuery from "./big-query-fields"; import getFieldsForMongo from "./mongo-fields"; @@ -309,65 +310,74 @@ function getEngineOptions(currentEngine) { }); } +function getDatabaseCachingField() { + const hasField = + PLUGIN_CACHING.databaseCacheTTLFormField && + MetabaseSettings.get("enable-query-caching"); + return hasField ? PLUGIN_CACHING.databaseCacheTTLFormField : null; +} + const forms = { details: { - fields: ({ id, engine, details = {} } = {}) => [ - { - name: "engine", - title: t`Database type`, - type: "select", - options: getEngineOptions(engine), - placeholder: t`Select a database`, - }, - { - name: "name", - title: t`Name`, - placeholder: t`How would you like to refer to this database?`, - validate: value => !value && t`required`, - hidden: !engine, - }, - ...(getEngineFormFields(engine, details, id) || []), - { - name: "auto_run_queries", - type: "boolean", - title: t`Automatically run queries when doing simple filtering and summarizing`, - description: t`When this is on, Metabase will automatically run queries when users do simple explorations with the Summarize and Filter buttons when viewing a table or chart. You can turn this off if querying this database is slow. This setting doesn’t affect drill-throughs or SQL queries.`, - hidden: !engine, - }, - { - name: "details.let-user-control-scheduling", - type: "boolean", - title: t`This is a large database, so let me choose when Metabase syncs and scans`, - description: t`By default, Metabase does a lightweight hourly sync and an intensive daily scan of field values. If you have a large database, we recommend turning this on and reviewing when and how often the field value scans happen.`, - hidden: !engine, - }, - { - name: "refingerprint", - type: "boolean", - title: t`Periodically refingerprint tables`, - description: t`When syncing with this database, Metabase will scan a subset of values of fields to gather statistics that enable things like improved binning behavior in charts, and to generally make your Metabase instance smarter.`, - hidden: !engine, - }, - { name: "is_full_sync", type: "hidden" }, - { name: "is_on_demand", type: "hidden" }, - { - name: "schedules.metadata_sync", - type: MetadataSyncScheduleWidget, - title: t`Database syncing`, - description: t`This is a lightweight process that checks for updates to this database’s schema. In most cases, you should be fine leaving this set to sync hourly.`, - hidden: !engine || !details["let-user-control-scheduling"], - }, - { - name: "schedules.cache_field_values", - type: CacheFieldValuesScheduleWidget, - title: t`Scanning for Filter Values`, - description: - t`Metabase can scan the values present in each field in this database to enable checkbox filters in dashboards and questions. This can be a somewhat resource-intensive process, particularly if you have a very large database.` + - " " + - t`When should Metabase automatically scan and cache field values?`, - hidden: !engine || !details["let-user-control-scheduling"], - }, - ], + fields: ({ id, engine, details = {} } = {}) => + [ + { + name: "engine", + title: t`Database type`, + type: "select", + options: getEngineOptions(engine), + placeholder: t`Select a database`, + }, + { + name: "name", + title: t`Name`, + placeholder: t`How would you like to refer to this database?`, + validate: value => !value && t`required`, + hidden: !engine, + }, + ...(getEngineFormFields(engine, details, id) || []), + { + name: "auto_run_queries", + type: "boolean", + title: t`Automatically run queries when doing simple filtering and summarizing`, + description: t`When this is on, Metabase will automatically run queries when users do simple explorations with the Summarize and Filter buttons when viewing a table or chart. You can turn this off if querying this database is slow. This setting doesn’t affect drill-throughs or SQL queries.`, + hidden: !engine, + }, + { + name: "details.let-user-control-scheduling", + type: "boolean", + title: t`This is a large database, so let me choose when Metabase syncs and scans`, + description: t`By default, Metabase does a lightweight hourly sync and an intensive daily scan of field values. If you have a large database, we recommend turning this on and reviewing when and how often the field value scans happen.`, + hidden: !engine, + }, + { + name: "refingerprint", + type: "boolean", + title: t`Periodically refingerprint tables`, + description: t`When syncing with this database, Metabase will scan a subset of values of fields to gather statistics that enable things like improved binning behavior in charts, and to generally make your Metabase instance smarter.`, + hidden: !engine, + }, + getDatabaseCachingField(), + { name: "is_full_sync", type: "hidden" }, + { name: "is_on_demand", type: "hidden" }, + { + name: "schedules.metadata_sync", + type: MetadataSyncScheduleWidget, + title: t`Database syncing`, + description: t`This is a lightweight process that checks for updates to this database’s schema. In most cases, you should be fine leaving this set to sync hourly.`, + hidden: !engine || !details["let-user-control-scheduling"], + }, + { + name: "schedules.cache_field_values", + type: CacheFieldValuesScheduleWidget, + title: t`Scanning for Filter Values`, + description: + t`Metabase can scan the values present in each field in this database to enable checkbox filters in dashboards and questions. This can be a somewhat resource-intensive process, particularly if you have a very large database.` + + " " + + t`When should Metabase automatically scan and cache field values?`, + hidden: !engine || !details["let-user-control-scheduling"], + }, + ].filter(Boolean), normalize: function(database) { if (!database.details["let-user-control-scheduling"]) { // TODO Atte Keinänen 8/15/17: Implement engine-specific scheduling defaults diff --git a/frontend/src/metabase/entities/questions.js b/frontend/src/metabase/entities/questions.js index ec70eef840b..843b7eed9d7 100644 --- a/frontend/src/metabase/entities/questions.js +++ b/frontend/src/metabase/entities/questions.js @@ -1,5 +1,4 @@ import { assocIn } from "icepick"; -import { t } from "ttag"; import { createEntity, undo } from "metabase/lib/entities"; import * as Urls from "metabase/lib/urls"; @@ -13,6 +12,8 @@ import { import { POST, DELETE } from "metabase/lib/api"; +import forms from "./questions/forms"; + const FAVORITE_ACTION = `metabase/entities/questions/FAVORITE`; const UNFAVORITE_ACTION = `metabase/entities/questions/UNFAVORITE`; @@ -84,39 +85,10 @@ const Questions = createEntity({ return state; }, - forms: { - details: { - fields: [ - { name: "name", title: t`Name` }, - { - name: "description", - title: t`Description`, - type: "text", - placeholder: t`It's optional but oh, so helpful`, - }, - { - name: "collection_id", - title: t`Collection`, - type: "collection", - }, - ], - }, - details_without_collection: { - fields: [ - { name: "name", title: t`Name` }, - { - name: "description", - title: t`Description`, - type: "text", - placeholder: t`It's optional but oh, so helpful`, - }, - ], - }, - }, - // NOTE: keep in sync with src/metabase/api/card.clj writableProperties: [ "name", + "cache_ttl", "dataset_query", "display", "description", @@ -134,6 +106,8 @@ const Questions = createEntity({ const type = object && getCollectionType(object.collection_id, getState()); return type && `collection=${type}`; }, + + forms, }); export default Questions; diff --git a/frontend/src/metabase/entities/questions/forms.js b/frontend/src/metabase/entities/questions/forms.js new file mode 100644 index 00000000000..a147f00255d --- /dev/null +++ b/frontend/src/metabase/entities/questions/forms.js @@ -0,0 +1,42 @@ +import { t } from "ttag"; +import MetabaseSettings from "metabase/lib/settings"; +import { PLUGIN_CACHING } from "metabase/plugins"; + +const FORM_FIELDS = [ + { name: "name", title: t`Name` }, + { + name: "description", + title: t`Description`, + type: "text", + placeholder: t`It's optional but oh, so helpful`, + }, +]; + +export default { + create: { + fields: [ + ...FORM_FIELDS, + { + name: "collection_id", + title: t`Collection`, + type: "collection", + }, + ], + }, + edit: { + fields: () => { + const fields = [...FORM_FIELDS]; + if ( + MetabaseSettings.get("enable-query-caching") && + PLUGIN_CACHING.cacheTTLFormField + ) { + fields.push({ + ...PLUGIN_CACHING.cacheTTLFormField, + title: t`Caching`, + type: "questionCacheTTL", + }); + } + return fields; + }, + }, +}; diff --git a/frontend/src/metabase/lib/formatting.js b/frontend/src/metabase/lib/formatting.js index c462d4b44ab..23d7caf3168 100644 --- a/frontend/src/metabase/lib/formatting.js +++ b/frontend/src/metabase/lib/formatting.js @@ -930,13 +930,20 @@ export function conjunct(list: string[], conjunction: string) { } export function duration(milliseconds: number) { - if (milliseconds < 60000) { - const seconds = Math.round(milliseconds / 1000); - return ngettext(msgid`${seconds} second`, `${seconds} seconds`, seconds); - } else { - const minutes = Math.round(milliseconds / 1000 / 60); + const SECOND = 1000; + const MINUTE = 60 * SECOND; + const HOUR = 60 * MINUTE; + + if (milliseconds >= HOUR) { + const hours = Math.round(milliseconds / HOUR); + return ngettext(msgid`${hours} hour`, `${hours} hours`, hours); + } + if (milliseconds >= MINUTE) { + const minutes = Math.round(milliseconds / MINUTE); return ngettext(msgid`${minutes} minute`, `${minutes} minutes`, minutes); } + const seconds = Math.round(milliseconds / SECOND); + return ngettext(msgid`${seconds} second`, `${seconds} seconds`, seconds); } // Removes trailing "id" from field names diff --git a/frontend/src/metabase/lib/time.js b/frontend/src/metabase/lib/time.js index d6c9c1dfb01..fa5a550d653 100644 --- a/frontend/src/metabase/lib/time.js +++ b/frontend/src/metabase/lib/time.js @@ -175,3 +175,20 @@ export function getRelativeTimeAbbreviated(timestamp) { return getRelativeTime(timestamp); } + +export function msToSeconds(ms) { + return ms / 1000; +} + +export function msToMinutes(ms) { + return msToSeconds(ms) / 60; +} + +export function msToHours(ms) { + const hours = msToMinutes(ms) / 60; + return hours; +} + +export function hoursToSeconds(hours) { + return hours * 60 * 60; +} diff --git a/frontend/src/metabase/plugins/index.js b/frontend/src/metabase/plugins/index.js index b5c1e30890f..721239aa0e6 100644 --- a/frontend/src/metabase/plugins/index.js +++ b/frontend/src/metabase/plugins/index.js @@ -92,3 +92,10 @@ export const PLUGIN_MODERATION = { getStatusIcon: object, getModerationTimelineEvents: array, }; + +export const PLUGIN_CACHING = { + dashboardCacheTTLFormField: null, + databaseCacheTTLFormField: null, + questionCacheTTLFormField: null, + getQuestionsImplicitCacheTTL: () => null, +}; diff --git a/frontend/src/metabase/query_builder/components/view/EditQuestionInfoModal.jsx b/frontend/src/metabase/query_builder/components/view/EditQuestionInfoModal.jsx index a11f9708bac..5edb329dca5 100644 --- a/frontend/src/metabase/query_builder/components/view/EditQuestionInfoModal.jsx +++ b/frontend/src/metabase/query_builder/components/view/EditQuestionInfoModal.jsx @@ -1,26 +1,84 @@ -/* eslint-disable react/prop-types */ -import React from "react"; - +import React, { useCallback } from "react"; +import PropTypes from "prop-types"; import { t } from "ttag"; +import _ from "underscore"; import Form from "metabase/containers/Form"; import ModalContent from "metabase/components/ModalContent"; +import CollapseSection from "metabase/components/CollapseSection"; +import { PLUGIN_CACHING } from "metabase/plugins"; import Questions from "metabase/entities/questions"; -const EditQuestionInfoModal = ({ question, onClose, onSave }) => ( - <ModalContent title={t`Edit question`} onClose={onClose}> - <Form - form={Questions.forms.details_without_collection} - initialValues={question.card()} - submitTitle={t`Save`} - onClose={onClose} - onSubmit={async card => { - await onSave({ ...question.card(), ...card }); - onClose(); - }} - /> - </ModalContent> -); +const COLLAPSED_FIELDS = ["cache_ttl"]; + +const propTypes = { + question: PropTypes.object.isRequired, // metabase-lib Question instance + onSave: PropTypes.func.isRequired, + onClose: PropTypes.func.isRequired, +}; + +function EditQuestionInfoModal({ question, onClose, onSave }) { + const onSubmit = useCallback( + async card => { + await onSave({ ...question.card(), ...card }); + onClose(); + }, + [question, onSave, onClose], + ); + + const isCachedImplicitly = + PLUGIN_CACHING.getQuestionsImplicitCacheTTL(question) > 0; + + return ( + <ModalContent title={t`Edit question`} onClose={onClose}> + <Form + initialValues={question.card()} + form={Questions.forms.edit} + onSubmit={onSubmit} + > + {({ Form, FormField, FormFooter, formFields }) => { + const [visibleFields, collapsedFields] = _.partition( + formFields, + field => !COLLAPSED_FIELDS.includes(field.name), + ); + + const cacheFieldExtraProps = {}; + if (isCachedImplicitly) { + cacheFieldExtraProps.className = "mt1"; + } else { + cacheFieldExtraProps.title = null; + } + + return ( + <Form> + {visibleFields.map(field => ( + <FormField key={field.name} name={field.name} /> + ))} + {collapsedFields.length > 0 && ( + <CollapseSection + header={t`More options`} + iconVariant="up-down" + iconPosition="right" + headerClass="text-bold text-medium text-brand-hover" + bodyClass="pt2" + > + <FormField + name="cache_ttl" + question={question} + {...cacheFieldExtraProps} + /> + </CollapseSection> + )} + <FormFooter submitTitle={t`Save`} onCancel={onClose} /> + </Form> + ); + }} + </Form> + </ModalContent> + ); +} + +EditQuestionInfoModal.propTypes = propTypes; export default EditQuestionInfoModal; diff --git a/frontend/src/metabase/query_builder/components/view/EditQuestionInfoModal.unit.spec.js b/frontend/src/metabase/query_builder/components/view/EditQuestionInfoModal.unit.spec.js new file mode 100644 index 00000000000..52a2e405999 --- /dev/null +++ b/frontend/src/metabase/query_builder/components/view/EditQuestionInfoModal.unit.spec.js @@ -0,0 +1,219 @@ +import React from "react"; +import { Provider } from "react-redux"; +import { reducer as form } from "redux-form"; +import { fireEvent, render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import xhrMock from "xhr-mock"; +import { getStore } from "__support__/entities-store"; +import { PLUGIN_CACHING, PLUGIN_FORM_WIDGETS } from "metabase/plugins"; +import NumericFormField from "metabase/components/form/widgets/FormNumericInputWidget"; +import MetabaseSettings from "metabase/lib/settings"; +import EditQuestionInfoModal from "./EditQuestionInfoModal"; + +const QUESTION = { + id: 1, + name: "Question", + description: "I'm here for your unit tests", + collection_id: null, + cache_ttl: 0, + archived: false, +}; + +function mockCachingSettings({ enabled = true } = {}) { + const spy = jest.spyOn(MetabaseSettings, "get"); + spy.mockImplementation(key => { + if (key === "enable-query-caching") { + return enabled; + } + }); +} + +function setup({ cachingEnabled = true } = {}) { + mockCachingSettings({ + enabled: cachingEnabled, + }); + + const onSave = jest.fn(); + const onClose = jest.fn(); + + const question = { + card: () => QUESTION, + }; + + render( + <Provider store={getStore({ form })}> + <EditQuestionInfoModal + question={question} + onSave={onSave} + onClose={onClose} + /> + </Provider>, + ); + + return { + onSave, + onClose, + }; +} + +function fillNumericInput(input, value) { + userEvent.clear(input); + userEvent.type(input, String(value)); + input.blur(); +} + +function fillForm({ name, description, cache_ttl } = {}) { + if (name) { + const input = screen.getByLabelText("Name"); + userEvent.clear(input); + userEvent.type(input, name); + } + if (description) { + const input = screen.getByLabelText("Description"); + userEvent.clear(input); + userEvent.type(input, description); + } + if (cache_ttl) { + const input = screen.getByLabelText("Caching"); + fillNumericInput(input, cache_ttl); + } +} + +describe("EditQuestionInfoModal", () => { + beforeEach(() => { + xhrMock.setup(); + xhrMock.get("/api/collection", { + body: JSON.stringify([ + { + id: "root", + name: "Our analytics", + can_write: true, + }, + ]), + }); + }); + + afterEach(() => { + xhrMock.teardown(); + }); + + it("displays fields with filled values", () => { + setup(); + + expect(screen.queryByLabelText("Name")).toBeInTheDocument(); + expect(screen.queryByLabelText("Name")).toHaveValue(QUESTION.name); + + expect(screen.queryByLabelText("Description")).toBeInTheDocument(); + expect(screen.queryByLabelText("Description")).toHaveValue( + QUESTION.description, + ); + + expect(screen.queryByRole("button", { name: "Save" })).toBeInTheDocument(); + expect( + screen.queryByRole("button", { name: "Cancel" }), + ).toBeInTheDocument(); + }); + + it("calls onClose when Cancel button is clicked", () => { + const { onClose } = setup(); + fireEvent.click(screen.queryByRole("button", { name: "Cancel" })); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it("can't submit if name is empty", () => { + setup(); + + fillForm({ name: "" }); + fireEvent.click(screen.queryByRole("button", { name: "Save" })); + + expect(screen.queryByRole("button", { name: "Save" })).toBeDisabled(); + }); + + it("calls onSave callback on successful update", () => { + const UPDATES = { + name: "New fancy question name", + description: "Just testing if updates work correctly", + }; + const { onSave } = setup(); + + fillForm(UPDATES); + fireEvent.click(screen.queryByRole("button", { name: "Save" })); + + expect(onSave).toHaveBeenCalledTimes(1); + expect(onSave).toHaveBeenCalledWith({ + ...QUESTION, + ...UPDATES, + }); + }); + + describe("Cache TTL field", () => { + describe("OSS", () => { + it("is not shown", () => { + setup(); + expect(screen.queryByText("More options")).not.toBeInTheDocument(); + expect( + screen.queryByText("Cache all question results for"), + ).not.toBeInTheDocument(); + }); + }); + + describe("EE", () => { + beforeEach(() => { + PLUGIN_CACHING.cacheTTLFormField = { + name: "cache_ttl", + }; + PLUGIN_FORM_WIDGETS.questionCacheTTL = NumericFormField; + }); + + afterEach(() => { + PLUGIN_CACHING.cacheTTLFormField = null; + PLUGIN_FORM_WIDGETS.questionCacheTTL = null; + }); + + describe("caching enabled", () => { + it("is shown", () => { + setup(); + fireEvent.click(screen.queryByText("More options")); + expect(screen.getByDisplayValue("0")).toBeInTheDocument(); + }); + + it("can be changed", () => { + const { onSave } = setup(); + + fireEvent.click(screen.queryByText("More options")); + fillNumericInput(screen.getByDisplayValue("0"), 10); + fireEvent.click(screen.queryByRole("button", { name: "Save" })); + + expect(onSave).toHaveBeenCalledWith({ + ...QUESTION, + cache_ttl: 10, + }); + }); + }); + + describe("caching disabled", () => { + it("is not shown if caching is disabled", () => { + setup({ cachingEnabled: false }); + expect(screen.queryByText("More options")).not.toBeInTheDocument(); + expect( + screen.queryByText("Cache all question results for"), + ).not.toBeInTheDocument(); + }); + + it("can still submit the form", () => { + const { onSave } = setup({ + cachingEnabled: false, + }); + + fillForm({ name: "Test" }); + fireEvent.click(screen.queryByRole("button", { name: "Save" })); + + expect(onSave).toHaveBeenCalledWith({ + ...QUESTION, + name: "Test", + }); + }); + }); + }); + }); +}); diff --git a/frontend/test/metabase/containers/SaveQuestionModal.unit.spec.js b/frontend/test/metabase/containers/SaveQuestionModal.unit.spec.js index 09b965b03b7..fbe90f49fcc 100644 --- a/frontend/test/metabase/containers/SaveQuestionModal.unit.spec.js +++ b/frontend/test/metabase/containers/SaveQuestionModal.unit.spec.js @@ -4,6 +4,8 @@ import mock from "xhr-mock"; import SaveQuestionModal from "metabase/containers/SaveQuestionModal"; import Question from "metabase-lib/lib/Question"; +import MetabaseSettings from "metabase/lib/settings"; +import { PLUGIN_CACHING } from "metabase/plugins"; import { SAMPLE_DATASET, @@ -16,6 +18,17 @@ import { getStore } from "__support__/entities-store"; import { Provider } from "react-redux"; import { reducer as form } from "redux-form"; +function mockCachingEnabled(enabled = true) { + const original = MetabaseSettings.get; + const spy = jest.spyOn(MetabaseSettings, "get"); + spy.mockImplementation(key => { + if (key === "enable-query-caching") { + return enabled; + } + return original(key); + }); +} + const renderSaveQuestionModal = (question, originalQuestion) => { const store = getStore({ form }); const onCreateMock = jest.fn(() => Promise.resolve()); @@ -141,4 +154,50 @@ describe("SaveQuestionModal", () => { fireEvent.click(screen.getByText("Save")); expect(onSaveMock.mock.calls[0][0].collection_id).toEqual(5); }); + + describe("Cache TTL field", () => { + beforeEach(() => { + mockCachingEnabled(); + }); + + const question = Question.create({ + databaseId: SAMPLE_DATASET.id, + tableId: ORDERS.id, + metadata, + }) + .query() + .aggregate(["count"]) + .question(); + + describe("OSS", () => { + it("is not shown", () => { + renderSaveQuestionModal(question); + expect(screen.queryByText("More options")).not.toBeInTheDocument(); + expect( + screen.queryByText("Cache all question results for"), + ).not.toBeInTheDocument(); + }); + }); + + describe("EE", () => { + beforeEach(() => { + PLUGIN_CACHING.cacheTTLFormField = { + name: "cache_ttl", + type: "integer", + }; + }); + + afterEach(() => { + PLUGIN_CACHING.cacheTTLFormField = null; + }); + + it("is not shown", () => { + renderSaveQuestionModal(question); + expect(screen.queryByText("More options")).not.toBeInTheDocument(); + expect( + screen.queryByText("Cache all question results for"), + ).not.toBeInTheDocument(); + }); + }); + }); }); diff --git a/frontend/test/metabase/lib/time.unit.spec.js b/frontend/test/metabase/lib/time.unit.spec.js index 65f3e28c147..b802856301c 100644 --- a/frontend/test/metabase/lib/time.unit.spec.js +++ b/frontend/test/metabase/lib/time.unit.spec.js @@ -2,6 +2,10 @@ import { parseTime, parseTimestamp, getRelativeTimeAbbreviated, + msToSeconds, + msToMinutes, + msToHours, + hoursToSeconds, } from "metabase/lib/time"; import moment from "moment"; @@ -99,4 +103,56 @@ describe("time", () => { ).toEqual("5 d"); }); }); + + const SECOND = 1000; + const MINUTE = 60 * 1000; + const HOUR = MINUTE * 60; + + describe("msToSeconds", () => { + [ + { value: 0, expected: 0 }, + { value: SECOND, expected: 1 }, + { value: 1.5 * SECOND, expected: 1.5 }, + ].forEach(({ value, expected }) => { + it(`returns ${expected} for ${value}`, () => { + expect(msToSeconds(value)).toBe(expected); + }); + }); + }); + + describe("msToMinutes", () => { + [ + { value: 0, expected: 0 }, + { value: MINUTE, expected: 1 }, + { value: 2.5 * MINUTE, expected: 2.5 }, + ].forEach(({ value, expected }) => { + it(`returns ${expected} for ${value}`, () => { + expect(msToMinutes(value)).toBe(expected); + }); + }); + }); + + describe("msToHours", () => { + [ + { value: 0, expected: 0 }, + { value: HOUR, expected: 1 }, + { value: 5.5 * HOUR, expected: 5.5 }, + ].forEach(({ value, expected }) => { + it(`returns ${expected} for ${value}`, () => { + expect(msToHours(value)).toBe(expected); + }); + }); + }); + + describe("hoursToSecond", () => { + [ + { value: 0, expected: 0 }, + { value: 1, expected: 60 * 60 }, + { value: 2.5, expected: 2.5 * 60 * 60 }, + ].forEach(({ value, expected }) => { + it(`returns ${expected} for ${value}`, () => { + expect(hoursToSeconds(value)).toBe(expected); + }); + }); + }); }); diff --git a/frontend/test/metabase/scenarios/admin/databases/add.cy.spec.js b/frontend/test/metabase/scenarios/admin/databases/add.cy.spec.js index a1430d6ac82..46b708a439e 100644 --- a/frontend/test/metabase/scenarios/admin/databases/add.cy.spec.js +++ b/frontend/test/metabase/scenarios/admin/databases/add.cy.spec.js @@ -1,4 +1,9 @@ -import { restore, popover } from "__support__/e2e/cypress"; +import { + restore, + popover, + describeWithToken, + mockSessionProperty, +} from "__support__/e2e/cypress"; function typeField(label, value) { cy.findByLabelText(label) @@ -304,6 +309,53 @@ describe("scenarios > admin > databases > add", () => { }); }); }); + + describeWithToken("caching", () => { + beforeEach(() => { + mockSessionProperty("enable-query-caching", true); + }); + + it("sets cache ttl to null by default", () => { + cy.intercept("POST", "/api/database", { id: 42 }).as("createDatabase"); + cy.visit("/admin/databases/create"); + + typeField("Name", "Test db name"); + typeField("Host", "localhost"); + typeField("Database name", "test_postgres_db"); + typeField("Username", "uberadmin"); + + cy.button("Save").click(); + + cy.wait("@createDatabase").then(({ request }) => { + expect(request.body.cache_ttl).to.equal(null); + }); + }); + + it("allows to set cache ttl", () => { + cy.intercept("POST", "/api/database", { id: 42 }).as("createDatabase"); + cy.visit("/admin/databases/create"); + + typeField("Name", "Test db name"); + typeField("Host", "localhost"); + typeField("Database name", "test_postgres_db"); + typeField("Username", "uberadmin"); + + cy.findByText("Use instance default (TTL)").click(); + popover() + .findByText("Custom") + .click(); + cy.findByDisplayValue("24") + .clear() + .type("48") + .blur(); + + cy.button("Save").click(); + + cy.wait("@createDatabase").then(({ request }) => { + expect(request.body.cache_ttl).to.equal(48); + }); + }); + }); }); function chooseDatabase(database) { diff --git a/frontend/test/metabase/scenarios/admin/databases/edit.cy.spec.js b/frontend/test/metabase/scenarios/admin/databases/edit.cy.spec.js index 8f5f1ede519..834a2214397 100644 --- a/frontend/test/metabase/scenarios/admin/databases/edit.cy.spec.js +++ b/frontend/test/metabase/scenarios/admin/databases/edit.cy.spec.js @@ -1,4 +1,10 @@ -import { restore, popover, modal } from "__support__/e2e/cypress"; +import { + restore, + popover, + modal, + describeWithToken, + mockSessionProperty, +} from "__support__/e2e/cypress"; describe("scenarios > admin > databases > edit", () => { beforeEach(() => { @@ -61,6 +67,44 @@ describe("scenarios > admin > databases > edit", () => { "Automatically run queries when doing simple filtering and summarizing", ).should("have.attr", "aria-checked", "false"); }); + + describeWithToken("caching", () => { + beforeEach(() => { + mockSessionProperty("enable-query-caching", true); + }); + + it("allows to manage cache ttl", () => { + cy.visit("/admin/databases/1"); + + cy.findByText("Use instance default (TTL)").click(); + popover() + .findByText("Custom") + .click(); + cy.findByDisplayValue("24") + .clear() + .type("32") + .blur(); + + cy.button("Save changes").click(); + cy.wait("@databaseUpdate").then(({ request, response }) => { + expect(request.body.cache_ttl).to.equal(32); + expect(response.body.cache_ttl).to.equal(32); + + cy.visit("/admin/databases"); + cy.findByText("Sample Dataset").click(); + + cy.findByText("Custom").click(); + popover() + .findByText("Use instance default (TTL)") + .click(); + + cy.button("Save changes").click(); + cy.wait("@databaseUpdate").then(({ request }) => { + expect(request.body.cache_ttl).to.equal(null); + }); + }); + }); + }); }); describe("Scheduling tab", () => { diff --git a/frontend/test/metabase/scenarios/collections/items-metadata.cy.spec.js b/frontend/test/metabase/scenarios/collections/items-metadata.cy.spec.js index 7a6edd6a53d..41bfa212ebd 100644 --- a/frontend/test/metabase/scenarios/collections/items-metadata.cy.spec.js +++ b/frontend/test/metabase/scenarios/collections/items-metadata.cy.spec.js @@ -72,7 +72,7 @@ describe("scenarios > collection items metadata", () => { function changeDashboard() { cy.intercept("PUT", "/api/dashboard/**").as("updateDashboard"); cy.icon("ellipsis").click(); - cy.findByText("Change title and description").click(); + cy.findByText("Edit dashboard details").click(); cy.findByLabelText("Description") .click() .type("This dashboard is just beautiful"); diff --git a/frontend/test/metabase/scenarios/collections/permissions.cy.spec.js b/frontend/test/metabase/scenarios/collections/permissions.cy.spec.js index ddb64f8917b..7034dc188bc 100644 --- a/frontend/test/metabase/scenarios/collections/permissions.cy.spec.js +++ b/frontend/test/metabase/scenarios/collections/permissions.cy.spec.js @@ -384,7 +384,7 @@ describe("collection permissions", () => { }); it("should be able to change title and description", () => { - cy.findByText("Change title and description").click(); + cy.findByText("Edit dashboard details").click(); cy.location("pathname").should("eq", "/dashboard/1/details"); cy.findByLabelText("Name") .click() @@ -549,11 +549,11 @@ describe("collection permissions", () => { }); describe("managing dashboard from the dashboard's edit menu", () => { - it("should not be offered to change title and description for dashboard in collections they have `read` access to (metabase#15280)", () => { + it("should not be offered to edit dashboard details for dashboard in collections they have `read` access to (metabase#15280)", () => { cy.visit("/dashboard/1"); cy.icon("ellipsis").click(); popover() - .findByText("Change title and description") + .findByText("Edit dashboard details") .should("not.exist"); }); diff --git a/frontend/test/metabase/scenarios/dashboard/caching.cy.spec.js b/frontend/test/metabase/scenarios/dashboard/caching.cy.spec.js new file mode 100644 index 00000000000..326bbffc295 --- /dev/null +++ b/frontend/test/metabase/scenarios/dashboard/caching.cy.spec.js @@ -0,0 +1,56 @@ +import { + restore, + describeWithToken, + mockSessionProperty, + modal, +} from "__support__/e2e/cypress"; + +describeWithToken("scenarios > dashboard > caching", () => { + beforeEach(() => { + restore(); + mockSessionProperty("enable-query-caching", true); + cy.signInAsAdmin(); + }); + + it("can set cache ttl for a saved question", () => { + cy.intercept("PUT", "/api/dashboard/1").as("updateDashboard"); + cy.visit("/dashboard/1"); + + openEditingModalForm(); + modal().within(() => { + cy.findByText("More options").click(); + cy.findByPlaceholderText("24") + .clear() + .type("48") + .blur(); + cy.button("Update").click(); + }); + + cy.wait("@updateDashboard"); + cy.reload(); + + openEditingModalForm(); + modal().within(() => { + cy.findByText("More options").click(); + cy.findByDisplayValue("48") + .clear() + .type("0") + .blur(); + cy.button("Update").click(); + }); + + cy.wait("@updateDashboard"); + cy.reload(); + + openEditingModalForm(); + modal().within(() => { + cy.findByText("More options").click(); + cy.findByPlaceholderText("24"); + }); + }); +}); + +function openEditingModalForm() { + cy.icon("ellipsis").click(); + cy.findByText("Edit dashboard details").click(); +} diff --git a/frontend/test/metabase/scenarios/question/caching.cy.spec.js b/frontend/test/metabase/scenarios/question/caching.cy.spec.js new file mode 100644 index 00000000000..7506db9f0b2 --- /dev/null +++ b/frontend/test/metabase/scenarios/question/caching.cy.spec.js @@ -0,0 +1,56 @@ +import { + restore, + describeWithToken, + mockSessionProperty, + modal, +} from "__support__/e2e/cypress"; + +describeWithToken("scenarios > question > caching", () => { + beforeEach(() => { + restore(); + mockSessionProperty("enable-query-caching", true); + cy.signInAsAdmin(); + }); + + it("can set cache ttl for a saved question", () => { + cy.intercept("PUT", "/api/card/1").as("updateQuestion"); + cy.visit("/question/1"); + + openEditingModalForm(); + modal().within(() => { + cy.findByText("More options").click(); + cy.findByPlaceholderText("24") + .clear() + .type("48") + .blur(); + cy.button("Save").click(); + }); + + cy.wait("@updateQuestion"); + cy.reload(); + + openEditingModalForm(); + modal().within(() => { + cy.findByText("More options").click(); + cy.findByDisplayValue("48") + .clear() + .type("0") + .blur(); + cy.button("Save").click(); + }); + + cy.wait("@updateQuestion"); + cy.reload(); + + openEditingModalForm(); + modal().within(() => { + cy.findByText("More options").click(); + cy.findByPlaceholderText("24"); + }); + }); +}); + +function openEditingModalForm() { + cy.findByTestId("saved-question-header-button").click(); + cy.findByTestId("edit-details-button").click(); +} diff --git a/frontend/test/metabase/scenarios/sharing/embed.cy.spec.js b/frontend/test/metabase/scenarios/sharing/embed.cy.spec.js index c1795972b15..d2161155ba8 100644 --- a/frontend/test/metabase/scenarios/sharing/embed.cy.spec.js +++ b/frontend/test/metabase/scenarios/sharing/embed.cy.spec.js @@ -58,12 +58,10 @@ var iframeUrl = METABASE_SITE_URL + "/embed/dashboard/" + token + "#bordered=tru // click pencil icon to edit cy.icon("ellipsis").click(); // update title - popover().within(() => - cy.findByText("Change title and description").click(), - ); + popover().within(() => cy.findByText("Edit dashboard details").click()); modal().within(() => { - cy.findByText("Change title and description"); + cy.findByText("Edit dashboard details"); cy.findByLabelText("Name").type("{selectall}Orders per year"); cy.findByLabelText("Description").type( "{selectall}How many orders were placed in each year?", -- GitLab