[Charts] Use the Edit Properties modal throughout React views (#9267)

* typescriptification * use the chart edit modal on the react list view * linting * typings don't work on old react-bootstrap version * lint * remove duplicate field
2020-03-13 15:14:50 -07:00 · 2020-03-13 15:14:50 -07:00 · f6f40c815a
parent 91f3cb9878
commit f6f40c815a
10 changed files with 255 additions and 55 deletions
--- a/superset-frontend/.eslintrc.js
+++ b/superset-frontend/.eslintrc.js
@ -48,6 +48,7 @@ module.exports = {
            properties: 'never',
          },
        ],
+        '@typescript-eslint/no-empty-function': 0,
        '@typescript-eslint/no-explicit-any': 0,
        '@typescript-eslint/explicit-function-return-type': 0,
        camelcase: 0,
--- a/superset-frontend/package-lock.json
+++ b/superset-frontend/package-lock.json
@ -5031,6 +5031,56 @@
        "@types/webpack": "*"
      }
    },
+    "@types/react-redux": {
+      "version": "7.1.7",
+      "resolved": "https://registry.npmjs.org/@types/react-redux/-/react-redux-7.1.7.tgz",
+      "integrity": "sha512-U+WrzeFfI83+evZE2dkZ/oF/1vjIYgqrb5dGgedkqVV8HEfDFujNgWCwHL89TDuWKb47U0nTBT6PLGq4IIogWg==",
+      "dev": true,
+      "requires": {
+        "@types/hoist-non-react-statics": "^3.3.0",
+        "@types/react": "*",
+        "hoist-non-react-statics": "^3.3.0",
+        "redux": "^4.0.0"
+      },
+      "dependencies": {
+        "hoist-non-react-statics": {
+          "version": "3.3.2",
+          "resolved": "https://registry.npmjs.org/hoist-non-react-statics/-/hoist-non-react-statics-3.3.2.tgz",
+          "integrity": "sha512-/gGivxi8JPKWNm/W0jSmzcMPpfpPLc3dY/6GxhX2hQ9iGj3aDfklV4ET7NjKpSinLpJ5vafa9iiGIEZg10SfBw==",
+          "dev": true,
+          "requires": {
+            "react-is": "^16.7.0"
+          }
+        },
+        "react-is": {
+          "version": "16.13.0",
+          "resolved": "https://registry.npmjs.org/react-is/-/react-is-16.13.0.tgz",
+          "integrity": "sha512-GFMtL0vHkiBv9HluwNZTggSn/sCyEt9n02aM0dSAjGGyqyNlAyftYm4phPxdvCigG15JreC5biwxCgTAJZ7yAA==",
+          "dev": true
+        },
+        "redux": {
+          "version": "4.0.5",
+          "resolved": "https://registry.npmjs.org/redux/-/redux-4.0.5.tgz",
+          "integrity": "sha512-VSz1uMAH24DM6MF72vcojpYPtrTUu3ByVWfPL1nPfVRb5mZVTve5GnNCUV53QM/BZ66xfWrm0CTWoM+Xlz8V1w==",
+          "dev": true,
+          "requires": {
+            "loose-envify": "^1.4.0",
+            "symbol-observable": "^1.2.0"
+          }
+        }
+      }
+    },
+    "@types/react-select": {
+      "version": "3.0.10",
+      "resolved": "https://registry.npmjs.org/@types/react-select/-/react-select-3.0.10.tgz",
+      "integrity": "sha512-oUHXqvbkRhC07q5JjeY6hE+NUqgUM6CyaRXEKYPvMCBqUOuLnYltyhiNx6Jpb+iFpYtNHSQtF4dNJfMdMooKoQ==",
+      "dev": true,
+      "requires": {
+        "@types/react": "*",
+        "@types/react-dom": "*",
+        "@types/react-transition-group": "*"
+      }
+    },
    "@types/react-table": {
      "version": "7.0.2",
      "resolved": "https://registry.npmjs.org/@types/react-table/-/react-table-7.0.2.tgz",
@ -5040,6 +5090,15 @@
        "@types/react": "*"
      }
    },
+    "@types/react-transition-group": {
+      "version": "4.2.4",
+      "resolved": "https://registry.npmjs.org/@types/react-transition-group/-/react-transition-group-4.2.4.tgz",
+      "integrity": "sha512-8DMUaDqh0S70TjkqU0DxOu80tFUiiaS9rxkWip/nb7gtvAsbqOXm02UCmR8zdcjWujgeYPiPNTVpVpKzUDotwA==",
+      "dev": true,
+      "requires": {
+        "@types/react": "*"
+      }
+    },
    "@types/react-virtualized": {
      "version": "9.21.8",
      "resolved": "https://registry.npmjs.org/@types/react-virtualized/-/react-virtualized-9.21.8.tgz",
--- a/superset-frontend/package.json
+++ b/superset-frontend/package.json
@ -174,6 +174,8 @@
    "@types/react": "^16.4.18",
    "@types/react-dom": "^16.0.9",
    "@types/react-json-tree": "^0.6.11",
+    "@types/react-redux": "^7.1.7",
+    "@types/react-select": "^3.0.10",
    "@types/react-table": "^7.0.2",
    "@typescript-eslint/eslint-plugin": "^2.20.0",
    "@typescript-eslint/parser": "^2.20.0",
--- a/superset-frontend/spec/javascripts/explore/components/DisplayQueryButton_spec.jsx
+++ b/superset-frontend/spec/javascripts/explore/components/DisplayQueryButton_spec.jsx
@ -20,7 +20,7 @@ import React from 'react';
 import { mount } from 'enzyme';
 import ModalTrigger from './../../../../src/components/ModalTrigger';

-import DisplayQueryButton from '../../../../src/explore/components/DisplayQueryButton';
+import { DisplayQueryButton } from '../../../../src/explore/components/DisplayQueryButton';

 describe('DisplayQueryButton', () => {
  const defaultProps = {
--- a/superset-frontend/spec/javascripts/explore/components/ExploreChartHeader_spec.jsx
+++ b/superset-frontend/spec/javascripts/explore/components/ExploreChartHeader_spec.jsx
@ -19,7 +19,7 @@
 import React from 'react';
 import { shallow } from 'enzyme';

-import ExploreChartHeader from '../../../../src/explore/components/ExploreChartHeader';
+import { ExploreChartHeader } from '../../../../src/explore/components/ExploreChartHeader';
 import ExploreActionButtons from '../../../../src/explore/components/ExploreActionButtons';
 import EditableTitle from '../../../../src/components/EditableTitle';

--- a/superset-frontend/src/explore/components/DisplayQueryButton.jsx
+++ b/superset-frontend/src/explore/components/DisplayQueryButton.jsx
@ -17,6 +17,8 @@
 * under the License.
 */
 import React from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
 import PropTypes from 'prop-types';
 import SyntaxHighlighter, {
  registerLanguage,
@ -47,6 +49,7 @@ import Button from '../../components/Button';
 import RowCountLabel from './RowCountLabel';
 import { prepareCopyToClipboardTabularData } from '../../utils/common';
 import PropertiesModal from './PropertiesModal';
+import { sliceUpdated } from '../actions/exploreActions';

 registerLanguage('markdown', markdownSyntax);
 registerLanguage('html', htmlSyntax);
@ -65,7 +68,7 @@ const defaultProps = {
  animation: true,
 };

-export default class DisplayQueryButton extends React.PureComponent {
+export class DisplayQueryButton extends React.PureComponent {
  constructor(props) {
    super(props);
    const { datasource } = props.latestQueryFormData;
@ -220,6 +223,7 @@ export default class DisplayQueryButton extends React.PureComponent {
    return null;
  }
  render() {
+    const { animation, slice } = this.props;
    return (
      <DropdownButton
        noCaret
@ -233,22 +237,23 @@ export default class DisplayQueryButton extends React.PureComponent {
        pullRight
        id="query"
      >
-        {this.props.slice && (
+        {slice && (
          <>
            <MenuItem onClick={this.openPropertiesModal}>
              {t('Edit properties')}
            </MenuItem>
            <PropertiesModal
-              slice={this.props.slice}
+              slice={slice}
              show={this.state.isPropertiesModalOpen}
              onHide={this.closePropertiesModal}
-              animation={this.props.animation}
+              onSave={this.props.sliceUpdated}
+              animation={animation}
            />
          </>
        )}
        <ModalTrigger
          isMenuItem
-          animation={this.props.animation}
+          animation={animation}
          triggerNode={<span>{t('View query')}</span>}
          modalTitle={t('View query')}
          bsSize="large"
@ -257,7 +262,7 @@ export default class DisplayQueryButton extends React.PureComponent {
        />
        <ModalTrigger
          isMenuItem
-          animation={this.props.animation}
+          animation={animation}
          triggerNode={<span>{t('View results')}</span>}
          modalTitle={t('View results')}
          bsSize="large"
@ -266,7 +271,7 @@ export default class DisplayQueryButton extends React.PureComponent {
        />
        <ModalTrigger
          isMenuItem
-          animation={this.props.animation}
+          animation={animation}
          triggerNode={<span>{t('View samples')}</span>}
          modalTitle={t('View samples')}
          bsSize="large"
@ -285,3 +290,9 @@ export default class DisplayQueryButton extends React.PureComponent {

 DisplayQueryButton.propTypes = propTypes;
 DisplayQueryButton.defaultProps = defaultProps;
+
+function mapDispatchToProps(dispatch) {
+  return bindActionCreators({ sliceUpdated }, dispatch);
+}
+
+export default connect(null, mapDispatchToProps)(DisplayQueryButton);
--- a/superset-frontend/src/explore/components/ExploreChartHeader.jsx
+++ b/superset-frontend/src/explore/components/ExploreChartHeader.jsx
@ -17,6 +17,8 @@
 * under the License.
 */
 import React from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
 import PropTypes from 'prop-types';
 import { t } from '@superset-ui/translation';

@ -29,6 +31,8 @@ import FaveStar from '../../components/FaveStar';
 import TooltipWrapper from '../../components/TooltipWrapper';
 import Timer from '../../components/Timer';
 import CachedLabel from '../../components/CachedLabel';
+import PropertiesModal from './PropertiesModal';
+import { sliceUpdated } from '../actions/exploreActions';

 const CHART_STATUS_MAP = {
  failed: 'danger',
@ -49,7 +53,16 @@ const propTypes = {
  chart: chartPropShape,
 };

-class ExploreChartHeader extends React.PureComponent {
+export class ExploreChartHeader extends React.PureComponent {
+  constructor(props) {
+    super(props);
+    this.state = {
+      isPropertiesModalOpen: false,
+    };
+    this.openProperiesModal = this.openProperiesModal.bind(this);
+    this.closePropertiesModal = this.closePropertiesModal.bind(this);
+  }
+
  postChartFormData() {
    this.props.actions.postChartFormData(
      this.props.form_data,
@ -93,6 +106,18 @@ class ExploreChartHeader extends React.PureComponent {
    });
  }

+  openProperiesModal() {
+    this.setState({
+      isPropertiesModalOpen: true,
+    });
+  }
+
+  closePropertiesModal() {
+    this.setState({
+      isPropertiesModalOpen: false,
+    });
+  }
+
  renderChartTitle() {
    let title;
    if (this.props.slice) {
@ -131,17 +156,24 @@ class ExploreChartHeader extends React.PureComponent {
              saveFaveStar={this.props.actions.saveFaveStar}
              isStarred={this.props.isStarred}
            />
-
+            <PropertiesModal
+              show={this.state.isPropertiesModalOpen}
+              onHide={this.closePropertiesModal}
+              onSave={this.props.sliceUpdated}
+              slice={this.props.slice}
+            />
            <TooltipWrapper
              label="edit-desc"
              tooltip={t('Edit chart properties')}
            >
-              <a
+              <span
+                role="button"
+                tabIndex={0}
                className="edit-desc-icon"
-                href={`/chart/edit/${this.props.slice.slice_id}`}
+                onClick={this.openProperiesModal}
              >
                <i className="fa fa-edit" />
-              </a>
+              </span>
            </TooltipWrapper>
          </span>
        )}
@ -187,4 +219,8 @@ class ExploreChartHeader extends React.PureComponent {

 ExploreChartHeader.propTypes = propTypes;

-export default ExploreChartHeader;
+function mapDispatchToProps(dispatch) {
+  return bindActionCreators({ sliceUpdated }, dispatch);
+}
+
+export default connect(null, mapDispatchToProps)(ExploreChartHeader);
--- a/superset-frontend/src/explore/components/PropertiesModal.tsx
+++ b/superset-frontend/src/explore/components/PropertiesModal.tsx
@ -17,8 +17,6 @@
 * under the License.
 */
 import React, { useState, useEffect, useRef } from 'react';
-import { connect } from 'react-redux';
-import { bindActionCreators } from 'redux';
 import {
  Button,
  Modal,
@ -26,16 +24,41 @@ import {
  Col,
  FormControl,
  FormGroup,
+  // @ts-ignore
 } from 'react-bootstrap';
+// @ts-ignore
 import Dialog from 'react-bootstrap-dialog';
 import Select from 'react-select';
 import { t } from '@superset-ui/translation';
-import { SupersetClient } from '@superset-ui/connection';
-
-import { sliceUpdated } from '../actions/exploreActions';
+import { SupersetClient, Json } from '@superset-ui/connection';
+import Chart from 'src/types/Chart';
 import getClientErrorObject from '../../utils/getClientErrorObject';

-function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+export type Slice = {
+  slice_id: number;
+  slice_name: string;
+  description: string | null;
+  cache_timeout: number | null;
+};
+
+type InternalProps = {
+  slice: Slice;
+  onHide: () => void;
+  onSave: (chart: Chart) => void;
+};
+
+export type WrapperProps = InternalProps & {
+  show: boolean;
+  animation?: boolean; // for the modal
+};
+
+export default function PropertiesModalWrapper({
+  show,
+  onHide,
+  animation,
+  slice,
+  onSave,
+}: WrapperProps) {
  // The wrapper is a separate component so that hooks only run when the modal opens
  return (
    <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
@ -44,9 +67,9 @@ function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
  );
 }

-function PropertiesModal({ slice, onHide, onSave }) {
+function PropertiesModal({ slice, onHide, onSave }: InternalProps) {
  const [submitting, setSubmitting] = useState(false);
-  const errorDialog = useRef();
+  const errorDialog = useRef<any>(null);
  const [ownerOptions, setOwnerOptions] = useState(null);

  // values of form inputs
@ -55,9 +78,9 @@ function PropertiesModal({ slice, onHide, onSave }) {
  const [cacheTimeout, setCacheTimeout] = useState(
    slice.cache_timeout != null ? slice.cache_timeout : '',
  );
-  const [owners, setOwners] = useState(null);
+  const [owners, setOwners] = useState<any[] | null>(null);

-  function showError({ error, statusText }) {
+  function showError({ error, statusText }: any) {
    errorDialog.current.show({
      title: 'Error',
      bsSize: 'medium',
@ -72,8 +95,9 @@ function PropertiesModal({ slice, onHide, onSave }) {
      const response = await SupersetClient.get({
        endpoint: `/api/v1/chart/${slice.slice_id}`,
      });
+      const chart = (response.json as Json).result;
      setOwners(
-        response.json.result.owners.map(owner => ({
+        chart.owners.map((owner: any) => ({
          value: owner.id,
          label: owner.username,
        })),
@ -94,8 +118,9 @@ function PropertiesModal({ slice, onHide, onSave }) {
    SupersetClient.get({
      endpoint: `/api/v1/chart/related/owners`,
    }).then(res => {
+      const { result } = res.json as Json;
      setOwnerOptions(
-        res.json.result.map(item => ({
+        result.map((item: any) => ({
          value: item.value,
          label: item.text,
        })),
@ -103,7 +128,7 @@ function PropertiesModal({ slice, onHide, onSave }) {
    });
  }, []);

-  const onSubmit = async event => {
+  const onSubmit = async (event: React.FormEvent) => {
    event.stopPropagation();
    event.preventDefault();
    setSubmitting(true);
@ -111,7 +136,7 @@ function PropertiesModal({ slice, onHide, onSave }) {
      slice_name: name || null,
      description: description || null,
      cache_timeout: cacheTimeout || null,
-      owners: owners.map(o => o.value),
+      owners: owners!.map(o => o.value),
    };
    try {
      const res = await SupersetClient.put({
@ -120,7 +145,11 @@ function PropertiesModal({ slice, onHide, onSave }) {
        body: JSON.stringify(payload),
      });
      // update the redux state
-      onSave(res.json.result);
+      const updatedChart = {
+        ...(res.json as Json).result,
+        id: slice.slice_id,
+      };
+      onSave(updatedChart);
      onHide();
    } catch (res) {
      const clientError = await getClientErrorObject(res);
@ -147,7 +176,9 @@ function PropertiesModal({ slice, onHide, onSave }) {
                type="text"
                bsSize="sm"
                value={name}
-                onChange={event => setName(event.target.value)}
+                onChange={(event: React.ChangeEvent<HTMLInputElement>) =>
+                  setName(event.target.value)
+                }
              />
            </FormGroup>
            <FormGroup>
@ -160,7 +191,9 @@ function PropertiesModal({ slice, onHide, onSave }) {
                componentClass="textarea"
                bsSize="sm"
                value={description}
-                onChange={event => setDescription(event.target.value)}
+                onChange={(event: React.ChangeEvent<HTMLInputElement>) =>
+                  setDescription(event.target.value)
+                }
                style={{ maxWidth: '100%' }}
              />
              <p className="help-block">
@ -181,7 +214,7 @@ function PropertiesModal({ slice, onHide, onSave }) {
                type="text"
                bsSize="sm"
                value={cacheTimeout}
-                onChange={event =>
+                onChange={(event: React.ChangeEvent<HTMLInputElement>) =>
                  setCacheTimeout(event.target.value.replace(/[^0-9]/, ''))
                }
              />
@ -218,7 +251,7 @@ function PropertiesModal({ slice, onHide, onSave }) {
          bsSize="sm"
          bsStyle="primary"
          className="m-r-5"
-          disabled={submitting}
+          disabled={!owners || submitting}
        >
          {t('Save')}
        </Button>
@ -230,10 +263,3 @@ function PropertiesModal({ slice, onHide, onSave }) {
    </form>
  );
 }
-
-function mapDispatchToProps(dispatch) {
-  return bindActionCreators({ onSave: sliceUpdated }, dispatch);
-}
-
-export { PropertiesModalWrapper };
-export default connect(null, mapDispatchToProps)(PropertiesModalWrapper);
--- a/superset-frontend/src/types/Chart.ts
+++ b/superset-frontend/src/types/Chart.ts
@ -0,0 +1,33 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * The Chart model as returned from the API
+ */
+
+export default interface Chart {
+  id: number;
+  url: string;
+  viz_type: string;
+  slice_name: string;
+  creator: string;
+  changed_on: string;
+  description: string | null;
+  cache_timeout: number | null;
+}
--- a/superset-frontend/src/views/chartList/ChartList.tsx
+++ b/superset-frontend/src/views/chartList/ChartList.tsx
@ -31,6 +31,8 @@ import {
  Filters,
 } from 'src/components/ListView/types';
 import withToasts from 'src/messageToasts/enhancers/withToasts';
+import PropertiesModal, { Slice } from 'src/explore/components/PropertiesModal';
+import Chart from 'src/types/Chart';

 const PAGE_SIZE = 25;

@ -48,15 +50,9 @@ interface State {
  owners: Array<{ text: string; value: number }>;
  lastFetchDataConfig: FetchDataConfig | null;
  permissions: string[];
-}
-
-interface Chart {
-  changed_on: string;
-  creator: string;
-  id: number;
-  slice_name: string;
-  url: string;
-  viz_type: string;
+  // for now we need to use the Slice type defined in PropertiesModal.
+  // In future it would be better to have a unified Chart entity.
+  sliceCurrentlyEditing: Slice | null;
 }

 class ChartList extends React.PureComponent<Props, State> {
@ -73,6 +69,7 @@ class ChartList extends React.PureComponent<Props, State> {
    loading: false,
    owners: [],
    permissions: [],
+    sliceCurrentlyEditing: null,
  };

  componentDidMount() {
@ -181,7 +178,7 @@ class ChartList extends React.PureComponent<Props, State> {
    {
      Cell: ({ row: { state, original } }: any) => {
        const handleDelete = () => this.handleChartDelete(original);
-        const handleEdit = () => this.handleChartEdit(original);
+        const openEditModal = () => this.openChartEditModal(original);
        if (!this.canEdit && !this.canDelete) {
          return null;
        }
@ -218,7 +215,7 @@ class ChartList extends React.PureComponent<Props, State> {
                role="button"
                tabIndex={0}
                className="action-button"
-                onClick={handleEdit}
+                onClick={openEditModal}
              >
                <i className="fa fa-pencil" />
              </span>
@ -239,8 +236,29 @@ class ChartList extends React.PureComponent<Props, State> {
    return this.state.permissions.some(p => p === perm);
  };

-  handleChartEdit = ({ id }: { id: number }) => {
-    window.location.assign(`/chart/edit/${id}`);
+  openChartEditModal = (chart: Chart) => {
+    this.setState({
+      sliceCurrentlyEditing: {
+        slice_id: chart.id,
+        slice_name: chart.slice_name,
+        description: chart.description,
+        cache_timeout: chart.cache_timeout,
+      },
+    });
+  };
+
+  closeChartEditModal = () => {
+    this.setState({ sliceCurrentlyEditing: null });
+  };
+
+  handleChartUpdated = (edits: Chart) => {
+    // update the chart in our state with the edited info
+    const newCharts = this.state.charts.map(chart =>
+      chart.id === edits.id ? { ...chart, ...edits } : chart,
+    );
+    this.setState({
+      charts: newCharts,
+    });
  };

  handleChartDelete = ({ id, slice_name: sliceName }: Chart) => {
@ -367,10 +385,24 @@ class ChartList extends React.PureComponent<Props, State> {
  };

  render() {
-    const { charts, chartCount, loading, filters } = this.state;
+    const {
+      charts,
+      chartCount,
+      loading,
+      filters,
+      sliceCurrentlyEditing,
+    } = this.state;
    return (
      <div className="container welcome">
        <Panel>
+          {sliceCurrentlyEditing && (
+            <PropertiesModal
+              show
+              onHide={this.closeChartEditModal}
+              onSave={this.handleChartUpdated}
+              slice={sliceCurrentlyEditing}
+            />
+          )}
          <ConfirmStatusChange
            title={t('Please confirm')}
            description={t(