From 0cf05e24e47b738e04519a1d87bc745cbf467ddc Mon Sep 17 00:00:00 2001
From: Anton Kulyk <kuliks.anton@gmail.com>
Date: Mon, 27 Mar 2023 18:11:17 +0100
Subject: [PATCH] Clean up `NotebookSteps` component (#29554)
* Remove commented out code
* Fix typo
* Convert `steps` to TypeScript
* Convert `NotebookSteps` to TypeScript
* Turn `NotebookSteps` into functional component
* Turn `NotebookSteps` into a functional component
* Move `NotebookSteps` into its own directory
* Extract styled component
* Fix type
---
.../components/notebook/Notebook.tsx | 4 +-
.../components/notebook/NotebookSteps.jsx | 84 --------------
.../NotebookSteps/NotebookSteps.styled.tsx | 5 +
.../notebook/NotebookSteps/NotebookSteps.tsx | 105 ++++++++++++++++++
.../notebook/NotebookSteps/index.ts | 1 +
.../notebook/lib/{steps.js => steps.ts} | 96 +++++++++-------
.../components/notebook/lib/steps.types.ts | 11 +-
7 files changed, 178 insertions(+), 128 deletions(-)
delete mode 100644 frontend/src/metabase/query_builder/components/notebook/NotebookSteps.jsx
create mode 100644 frontend/src/metabase/query_builder/components/notebook/NotebookSteps/NotebookSteps.styled.tsx
create mode 100644 frontend/src/metabase/query_builder/components/notebook/NotebookSteps/NotebookSteps.tsx
create mode 100644 frontend/src/metabase/query_builder/components/notebook/NotebookSteps/index.ts
rename frontend/src/metabase/query_builder/components/notebook/lib/{steps.js => steps.ts} (74%)
diff --git a/frontend/src/metabase/query_builder/components/notebook/Notebook.tsx b/frontend/src/metabase/query_builder/components/notebook/Notebook.tsx
index d54c8403b53..d04f91d0c19 100644
--- a/frontend/src/metabase/query_builder/components/notebook/Notebook.tsx
+++ b/frontend/src/metabase/query_builder/components/notebook/Notebook.tsx
@@ -23,7 +23,7 @@ interface NotebookOwnProps {
isRunnable: boolean;
isResultDirty: boolean;
hasVisualizeButton?: boolean;
- updateQuestion: (question: Question) => void;
+ updateQuestion: (question: Question) => Promise<void>;
runQuestionQuery: () => void;
setQueryBuilderMode: (mode: string) => void;
}
@@ -61,7 +61,7 @@ const Notebook = ({ className, ...props }: NotebookProps) => {
await updateQuestion(cleanQuestion);
}
- // vizualize switches the view to the question's visualization.
+ // visualize switches the view to the question's visualization.
async function visualize() {
// Only cleanup the question if it's dirty, otherwise Metabase
// will incorrectly display the Save button, even though there are no changes to save.
diff --git a/frontend/src/metabase/query_builder/components/notebook/NotebookSteps.jsx b/frontend/src/metabase/query_builder/components/notebook/NotebookSteps.jsx
deleted file mode 100644
index b42af349a9d..00000000000
--- a/frontend/src/metabase/query_builder/components/notebook/NotebookSteps.jsx
+++ /dev/null
@@ -1,84 +0,0 @@
-/* eslint-disable react/prop-types */
-import React from "react";
-
-import cx from "classnames";
-import NotebookStep from "./NotebookStep";
-
-import { getQuestionSteps } from "./lib/steps";
-
-export default class NotebookSteps extends React.Component {
- constructor(props) {
- super(props);
- const isNew = !props.question.table();
- this.state = {
- openSteps: isNew
- ? {
- "0:filter": true,
- // "0:aggregate": true,
- "0:summarize": true,
- }
- : {},
- lastOpenedStep: null,
- };
- }
-
- openStep = id => {
- this.setState({
- openSteps: { ...this.state.openSteps, [id]: true },
- lastOpenedStep: id,
- });
- };
-
- closeStep = id => {
- this.setState({
- openSteps: { ...this.state.openSteps, [id]: false },
- lastOpenedStep:
- this.state.lastOpenedStep === id ? null : this.state.lastOpenedStep,
- });
- };
-
- render() {
- const {
- question,
- className,
- reportTimezone,
- sourceQuestion,
- updateQuestion,
- } = this.props;
- const { openSteps, lastOpenedStep } = this.state;
-
- if (!question) {
- return null;
- }
-
- const steps = getQuestionSteps(question, openSteps);
-
- return (
- <div className={cx(className, "pt3")}>
- {steps.map((step, index) => {
- // pass a version of updateQuery that cleans subsequent steps etc
- const updateQuery = async query => {
- const datasetQuery = query.datasetQuery();
- const updatedQuery = step.update(datasetQuery);
- await updateQuestion(updatedQuery.question());
- // mark the step as "closed" since we can assume it's been added or removed by the updateQuery
- this.closeStep(step.id);
- };
- return (
- <NotebookStep
- key={step.id}
- step={step}
- sourceQuestion={sourceQuestion}
- updateQuery={updateQuery}
- openStep={this.openStep}
- closeStep={this.closeStep}
- isLastStep={index === steps.length - 1}
- isLastOpened={lastOpenedStep === step.id}
- reportTimezone={reportTimezone}
- />
- );
- })}
- </div>
- );
- }
-}
diff --git a/frontend/src/metabase/query_builder/components/notebook/NotebookSteps/NotebookSteps.styled.tsx b/frontend/src/metabase/query_builder/components/notebook/NotebookSteps/NotebookSteps.styled.tsx
new file mode 100644
index 00000000000..92910a82c6e
--- /dev/null
+++ b/frontend/src/metabase/query_builder/components/notebook/NotebookSteps/NotebookSteps.styled.tsx
@@ -0,0 +1,5 @@
+import styled from "@emotion/styled";
+
+export const Container = styled.div`
+ padding-top: 1.5rem;
+`;
diff --git a/frontend/src/metabase/query_builder/components/notebook/NotebookSteps/NotebookSteps.tsx b/frontend/src/metabase/query_builder/components/notebook/NotebookSteps/NotebookSteps.tsx
new file mode 100644
index 00000000000..557295b3283
--- /dev/null
+++ b/frontend/src/metabase/query_builder/components/notebook/NotebookSteps/NotebookSteps.tsx
@@ -0,0 +1,105 @@
+import React, { useCallback, useMemo, useState } from "react";
+
+import type Question from "metabase-lib/Question";
+import type StructuredQuery from "metabase-lib/queries/StructuredQuery";
+
+import { getQuestionSteps } from "../lib/steps";
+import { NotebookStep as INotebookStep } from "../lib/steps.types";
+import NotebookStep from "../NotebookStep";
+import { Container } from "./NotebookSteps.styled";
+
+interface NotebookStepsProps {
+ className?: string;
+ question: Question;
+ sourceQuestion?: Question;
+ reportTimezone?: string;
+ updateQuestion: (question: Question) => Promise<void>;
+}
+
+type OpenSteps = { [key: string]: boolean };
+
+function getInitialOpenSteps(question: Question): OpenSteps {
+ const isNew = !question.table();
+ return isNew
+ ? {
+ "0:filter": true,
+ "0:summarize": true,
+ }
+ : {};
+}
+
+function NotebookSteps({
+ className,
+ question,
+ sourceQuestion,
+ reportTimezone,
+ updateQuestion,
+}: NotebookStepsProps) {
+ const [openSteps, setOpenSteps] = useState<OpenSteps>(
+ getInitialOpenSteps(question),
+ );
+ const [lastOpenedStep, setLastOpenedStep] = useState<string | null>(null);
+
+ const steps = useMemo(() => {
+ if (!question) {
+ return [];
+ }
+ return getQuestionSteps(question, openSteps);
+ }, [question, openSteps]);
+
+ const handleStepOpen = useCallback((id: string) => {
+ setOpenSteps(openSteps => ({ ...openSteps, [id]: true }));
+ setLastOpenedStep(id);
+ }, []);
+
+ const handleStepClose = useCallback((id: string) => {
+ setOpenSteps(openSteps => ({ ...openSteps, [id]: false }));
+ setLastOpenedStep(lastOpenedStep =>
+ lastOpenedStep === id ? null : lastOpenedStep,
+ );
+ }, []);
+
+ const handleQueryChange = useCallback(
+ async (step: INotebookStep, query: StructuredQuery) => {
+ const datasetQuery = query.datasetQuery();
+ const updatedQuery = step.update(datasetQuery);
+ await updateQuestion(updatedQuery.question());
+
+ // mark the step as "closed" since we can assume
+ // it's been added or removed by the updateQuery
+ handleStepClose(step.id);
+ },
+ [updateQuestion, handleStepClose],
+ );
+
+ if (!question) {
+ return null;
+ }
+
+ return (
+ <Container className={className}>
+ {steps.map((step, index) => {
+ const isLast = index === steps.length - 1;
+ const isLastOpened = lastOpenedStep === step.id;
+ const onChange = (query: StructuredQuery) =>
+ handleQueryChange(step, query);
+
+ return (
+ <NotebookStep
+ key={step.id}
+ step={step}
+ sourceQuestion={sourceQuestion}
+ isLastStep={isLast}
+ isLastOpened={isLastOpened}
+ reportTimezone={reportTimezone}
+ updateQuery={onChange}
+ openStep={handleStepOpen}
+ closeStep={handleStepClose}
+ />
+ );
+ })}
+ </Container>
+ );
+}
+
+export default NotebookSteps;
diff --git a/frontend/src/metabase/query_builder/components/notebook/NotebookSteps/index.ts b/frontend/src/metabase/query_builder/components/notebook/NotebookSteps/index.ts
new file mode 100644
index 00000000000..adebaeb4b67
--- /dev/null
+++ b/frontend/src/metabase/query_builder/components/notebook/NotebookSteps/index.ts
@@ -0,0 +1 @@
+export { default } from "./NotebookSteps";
diff --git a/frontend/src/metabase/query_builder/components/notebook/lib/steps.js b/frontend/src/metabase/query_builder/components/notebook/lib/steps.ts
similarity index 74%
rename from frontend/src/metabase/query_builder/components/notebook/lib/steps.js
rename to frontend/src/metabase/query_builder/components/notebook/lib/steps.ts
index 62186a1300a..f5d8a080266 100644
--- a/frontend/src/metabase/query_builder/components/notebook/lib/steps.js
+++ b/frontend/src/metabase/query_builder/components/notebook/lib/steps.ts
@@ -1,5 +1,9 @@
import _ from "underscore";
-import StructuredQuery from "metabase-lib/queries/StructuredQuery";
+
+import type Question from "metabase-lib/Question";
+import type StructuredQuery from "metabase-lib/queries/StructuredQuery";
+
+import { NotebookStep, NotebookStepFn } from "./steps.types";
// This converts an MBQL query into a sequence of notebook "steps", with special logic to determine which steps are
// allowed to be added at every other step, generating a preview query at each step, how to delete a step,
@@ -7,24 +11,32 @@ import StructuredQuery from "metabase-lib/queries/StructuredQuery";
// identifier for this step, e.x. `0:data` (or `0:join:1` for sub-steps)
-const STEPS = [
+type NotebookStepDef = Pick<NotebookStep, "type" | "clean" | "revert"> & {
+ valid: NotebookStepFn<boolean>;
+ active: NotebookStepFn<boolean>;
+ subSteps?: (query: StructuredQuery) => number;
+};
+
+const STEPS: NotebookStepDef[] = [
{
type: "data",
valid: query => !query.sourceQuery(),
active: query => true,
clean: query => query,
+ revert: null,
},
{
type: "join",
- valid: query => query.hasData() && query.database().hasFeature("join"),
- // active: query => query.hasJoins(),
- // revert: query => query.clearJoins(),
- // clean: query => query.cleanJoins(),
+ valid: query => {
+ const database = query.database();
+ return query.hasData() && database != null && database.hasFeature("join");
+ },
subSteps: query => query.joins().length,
- active: (query, index) => query.joins().length > index,
+ active: (query, index) =>
+ typeof index === "number" && query.joins().length > index,
revert: (query, index) => query.removeJoin(index),
clean: (query, index) => {
- const join = query.joins()[index];
+ const join = typeof index === "number" ? query.joins()[index] : null;
if (!join || join.isValid() || join.hasGaps()) {
return query;
}
@@ -37,7 +49,12 @@ const STEPS = [
},
{
type: "expression",
- valid: query => query.hasData() && query.database().supportsExpressions(),
+ valid: query => {
+ const database = query.database();
+ return (
+ query.hasData() && database != null && database.supportsExpressions()
+ );
+ },
active: query => query.hasExpressions(),
revert: query => query.clearExpressions(),
clean: query => query.cleanExpressions(),
@@ -49,20 +66,6 @@ const STEPS = [
revert: query => query.clearFilters(),
clean: query => query.cleanFilters(),
},
- // {
- // type: "aggregate",
- // valid: query => query.hasData(),
- // active: query => query.hasAggregations,
- // revert: query => query.clearAggregations(),
- // clean: query => query.cleanAggregations(),
- // },
- // {
- // type: "breakout",
- // valid: query => query.hasData() && query.hasAggregations() ,
- // active: query => query.hasBreakouts(),
- // revert: query => query.clearBreakouts(),
- // clean: query => query.cleanBreakouts(),
- // },
{
// NOTE: summarize is a combination of aggregate and breakout
type: "summarize",
@@ -100,11 +103,11 @@ const STEPS = [
/**
* Returns an array of "steps" to be displayed in the notebook for one "stage" (nesting) of a query
*/
-export function getQuestionSteps(question, openSteps = {}) {
- const allSteps = [];
+export function getQuestionSteps(question: Question, openSteps = {}) {
+ const allSteps: NotebookStep[] = [];
- let query = question.query();
- if (query instanceof StructuredQuery) {
+ if (question.isStructured()) {
+ let query = question.query() as StructuredQuery;
const database = question.database();
const allowsNesting = database && database.hasFeature("nested-queries");
@@ -142,13 +145,21 @@ export function getQuestionSteps(question, openSteps = {}) {
/**
* Returns an array of "steps" to be displayed in the notebook for one "stage" (nesting) of a query
*/
-export function getStageSteps(stageQuery, stageIndex, openSteps) {
- const getId = (step, itemIndex) =>
- `${stageIndex}:${step.type}` + (itemIndex > 0 ? `:${itemIndex}` : ``);
-
- function getStep(STEP, itemIndex = null) {
+export function getStageSteps(
+ stageQuery: StructuredQuery,
+ stageIndex: number,
+ openSteps: Record<NotebookStep["id"], boolean>,
+) {
+ const getId = (step: NotebookStepDef, itemIndex: number | null) => {
+ const isValidItemIndex = itemIndex != null && itemIndex > 0;
+ return (
+ `${stageIndex}:${step.type}` + (isValidItemIndex ? `:${itemIndex}` : "")
+ );
+ };
+
+ function getStep(STEP: NotebookStepDef, itemIndex: number | null = null) {
const id = getId(STEP, itemIndex);
- const step = {
+ const step: NotebookStep = {
id: id,
type: STEP.type,
stageIndex: stageIndex,
@@ -158,14 +169,17 @@ export function getStageSteps(stageQuery, stageIndex, openSteps) {
active: STEP.active(stageQuery, itemIndex),
visible:
STEP.valid(stageQuery, itemIndex) &&
- (STEP.active(stageQuery, itemIndex) || openSteps[id]),
- revert: STEP.revert ? query => STEP.revert(query, itemIndex) : null,
+ !!(STEP.active(stageQuery, itemIndex) || openSteps[id]),
+ revert: STEP.revert
+ ? (query: StructuredQuery) =>
+ STEP.revert ? STEP.revert(query, itemIndex) : null
+ : null,
clean: query => STEP.clean(query, itemIndex),
update: datasetQuery => {
let newQuery = stageQuery.setDatasetQuery(datasetQuery);
// clean each subsequent step individually. we have to do this rather than calling newQuery.clean() in case
// the current step is in a temporarily invalid state
- let current = step;
+ let current: NotebookStep | null = step;
while ((current = current.next)) {
// when switching to the next stage we need to setSourceQuery
if (
@@ -201,7 +215,7 @@ export function getStageSteps(stageQuery, stageIndex, openSteps) {
}),
);
- let previewQuery = stageQuery;
+ let previewQuery: StructuredQuery | null = stageQuery;
let actions = [];
// iterate over steps in reverse so we can revert query for previewing and accumulate valid actions
@@ -218,13 +232,17 @@ export function getStageSteps(stageQuery, stageIndex, openSteps) {
if (step.valid) {
actions.unshift({
type: step.type,
- action: ({ openStep }) => openStep(step.id),
+ action: ({
+ openStep,
+ }: {
+ openStep: (id: NotebookStep["id"]) => void;
+ }) => openStep(step.id),
});
}
steps.splice(i, 1);
}
// revert the previewQuery for this step
- if (step.revert) {
+ if (step.revert && previewQuery) {
previewQuery = step.revert(previewQuery);
}
}
diff --git a/frontend/src/metabase/query_builder/components/notebook/lib/steps.types.ts b/frontend/src/metabase/query_builder/components/notebook/lib/steps.types.ts
index d6c26ba36e5..0b5e06525e6 100644
--- a/frontend/src/metabase/query_builder/components/notebook/lib/steps.types.ts
+++ b/frontend/src/metabase/query_builder/components/notebook/lib/steps.types.ts
@@ -13,17 +13,22 @@ export type NotebookStepType =
| "sort"
| "limit";
+export type NotebookStepFn<ReturnType> = (
+ query: StructuredQuery,
+ index?: number | null,
+) => ReturnType;
+
export interface NotebookStep {
id: string;
type: NotebookStepType;
stageIndex: number;
- itemIndex: number;
+ itemIndex: number | null;
query: StructuredQuery;
valid: boolean;
active: boolean;
visible: boolean;
- revert: ((query: StructuredQuery) => StructuredQuery | null) | null;
- clean: (query: StructuredQuery) => StructuredQuery;
+ revert: NotebookStepFn<StructuredQuery | null> | null;
+ clean: NotebookStepFn<StructuredQuery>;
update: (datasetQuery: DatasetQuery) => StructuredQuery;
actions: NotebookStepAction[];
previewQuery: StructuredQuery | null;
--
GitLab