show edit modal on dashboards list view (#9211)

* show edit modal on dashboards list view * lint * fix test * simplify PropertiesModal interface * lint * comply with method ordering * fix type issue
2020-03-19 08:05:35 -07:00 · 2020-03-19 08:05:35 -07:00 · 46598830e9
parent d7ea41a529
commit 46598830e9
6 changed files with 158 additions and 67 deletions
--- a/superset-frontend/spec/javascripts/views/dashboardList/DashboardList_spec.jsx
+++ b/superset-frontend/spec/javascripts/views/dashboardList/DashboardList_spec.jsx
@ -24,6 +24,7 @@ import fetchMock from 'fetch-mock';

 import DashboardList from 'src/views/dashboardList/DashboardList';
 import ListView from 'src/components/ListView/ListView';
+import PropertiesModal from 'src/dashboard/components/PropertiesModal';

 // store needed for withToasts(DashboardTable)
 const mockStore = configureStore([thunk]);
@ -93,4 +94,13 @@ describe('DashboardList', () => {
      `"/http//localhost/api/v1/dashboard/?q={%22order_column%22:%22changed_on%22,%22order_direction%22:%22desc%22,%22page%22:0,%22page_size%22:25}"`,
    );
  });
+
+  it('edits', async () => {
+    expect(wrapper.find(PropertiesModal)).toHaveLength(0);
+    wrapper
+      .find('.fa-pencil')
+      .first()
+      .simulate('click');
+    expect(wrapper.find(PropertiesModal)).toHaveLength(1);
+  });
 });
--- a/superset-frontend/src/dashboard/components/Header.jsx
+++ b/superset-frontend/src/dashboard/components/Header.jsx
@ -449,8 +449,7 @@ class Header extends React.PureComponent {

          {this.state.showingPropertiesModal && (
            <PropertiesModal
-              dashboardTitle={dashboardTitle}
-              dashboardInfo={dashboardInfo}
+              dahboardId={dashboardInfo.id}
              show={this.state.showingPropertiesModal}
              onHide={this.hidePropertiesModal}
              onDashboardSave={updates => {
--- a/superset-frontend/src/dashboard/components/PropertiesModal.jsx
+++ b/superset-frontend/src/dashboard/components/PropertiesModal.jsx
@ -24,14 +24,13 @@ import Select from 'react-select';
 import AceEditor from 'react-ace';
 import { t } from '@superset-ui/translation';
 import { SupersetClient } from '@superset-ui/connection';
+import '../stylesheets/buttons.less';

 import getClientErrorObject from '../../utils/getClientErrorObject';
 import withToasts from '../../messageToasts/enhancers/withToasts';

 const propTypes = {
-  dashboardTitle: PropTypes.string,
-  dashboardInfo: PropTypes.object,
-  owners: PropTypes.arrayOf(PropTypes.object),
+  dashboardId: PropTypes.number.isRequired,
  show: PropTypes.bool.isRequired,
  onHide: PropTypes.func,
  onDashboardSave: PropTypes.func,
@ -39,9 +38,6 @@ const propTypes = {
 };

 const defaultProps = {
-  dashboardInfo: {},
-  dashboardTitle: '[dashboard name]',
-  owners: [],
  onHide: () => {},
  onDashboardSave: () => {},
  show: false,
@ -50,21 +46,16 @@ const defaultProps = {
 class PropertiesModal extends React.PureComponent {
  constructor(props) {
    super(props);
-    this.defaultMetadataValue = JSON.stringify(
-      props.dashboardInfo.metadata,
-      null,
-      2,
-    );
    this.state = {
      errors: [],
      values: {
-        dashboard_title: props.dashboardTitle,
-        slug: props.dashboardInfo.slug,
-        owners: props.owners || [],
-        json_metadata: this.defaultMetadataValue,
+        dashboard_title: '',
+        slug: '',
+        owners: [],
+        json_metadata: '',
      },
-      isOwnersLoaded: false,
-      userOptions: null,
+      isDashboardLoaded: false,
+      ownerOptions: null,
      isAdvancedOpen: false,
    };
    this.onChange = this.onChange.bind(this);
@ -75,27 +66,8 @@ class PropertiesModal extends React.PureComponent {
  }

  componentDidMount() {
-    SupersetClient.get({
-      endpoint: `/api/v1/dashboard/related/owners`,
-    }).then(response => {
-      const options = response.json.result.map(item => ({
-        value: item.value,
-        label: item.text,
-      }));
-      this.setState({
-        userOptions: options,
-      });
-    });
-    SupersetClient.get({
-      endpoint: `/api/v1/dashboard/${this.props.dashboardInfo.id}`,
-    }).then(response => {
-      this.setState({ isOwnersLoaded: true });
-      const initialSelectedValues = response.json.result.owners.map(owner => ({
-        value: owner.id,
-        label: owner.username,
-      }));
-      this.onOwnersChange(initialSelectedValues);
-    });
+    this.fetchOwnerOptions();
+    this.fetchDashboardDetails();
  }

  onOwnersChange(value) {
@ -111,6 +83,50 @@ class PropertiesModal extends React.PureComponent {
    this.updateFormState(name, value);
  }

+  fetchDashboardDetails() {
+    // We fetch the dashboard details because not all code
+    // that renders this component have all the values we need.
+    // At some point when we have a more consistent frontend
+    // datamodel, the dashboard could probably just be passed as a prop.
+    SupersetClient.get({
+      endpoint: `/api/v1/dashboard/${this.props.dashboardId}`,
+    })
+      .then(response => {
+        const dashboard = response.json.result;
+        this.setState(state => ({
+          isDashboardLoaded: true,
+          values: {
+            ...state.values,
+            dashboard_title: dashboard.dashboard_title || '',
+            slug: dashboard.slug || '',
+            json_metadata: dashboard.json_metadata || '',
+          },
+        }));
+        const initialSelectedValues = dashboard.owners.map(owner => ({
+          value: owner.id,
+          label: owner.username,
+        }));
+        this.onOwnersChange(initialSelectedValues);
+      })
+      .catch(err => console.error(err));
+  }
+
+  fetchOwnerOptions() {
+    SupersetClient.get({
+      endpoint: `/api/v1/dashboard/related/owners`,
+    })
+      .then(response => {
+        const options = response.json.result.map(item => ({
+          value: item.value,
+          label: item.text,
+        }));
+        this.setState({
+          ownerOptions: options,
+        });
+      })
+      .catch(err => console.error(err));
+  }
+
  updateFormState(name, value) {
    this.setState(state => ({
      values: {
@ -129,18 +145,23 @@ class PropertiesModal extends React.PureComponent {
  save(e) {
    e.preventDefault();
    e.stopPropagation();
-    const owners = this.state.values.owners.map(o => o.value);
+    const { values } = this.state;
+    const owners = values.owners.map(o => o.value);
+
    SupersetClient.put({
-      endpoint: `/api/v1/dashboard/${this.props.dashboardInfo.id}`,
+      endpoint: `/api/v1/dashboard/${this.props.dashboardId}`,
      headers: { 'Content-Type': 'application/json' },
      body: JSON.stringify({
-        ...this.state.values,
+        dashboard_title: values.dashboard_title,
+        slug: values.slug || null,
+        json_metadata: values.json_metadata || null,
        owners,
      }),
    })
      .then(({ json }) => {
        this.props.addSuccessToast(t('The dashboard has been saved'));
        this.props.onDashboardSave({
+          id: this.props.dashboardId,
          title: json.result.dashboard_title,
          slug: json.result.slug,
          jsonMetadata: json.result.json_metadata,
@ -162,7 +183,12 @@ class PropertiesModal extends React.PureComponent {
  }

  render() {
-    const { userOptions, values, isOwnersLoaded, isAdvancedOpen } = this.state;
+    const {
+      ownerOptions,
+      values,
+      isDashboardLoaded,
+      isAdvancedOpen,
+    } = this.state;
    return (
      <Modal show={this.props.show} onHide={this.props.onHide} bsSize="lg">
        <form onSubmit={this.save}>
@ -190,6 +216,7 @@ class PropertiesModal extends React.PureComponent {
                  bsSize="sm"
                  value={values.dashboard_title}
                  onChange={this.onChange}
+                  disabled={!isDashboardLoaded}
                />
              </Col>
              <Col md={6}>
@ -202,6 +229,7 @@ class PropertiesModal extends React.PureComponent {
                  bsSize="sm"
                  value={values.slug || ''}
                  onChange={this.onChange}
+                  disabled={!isDashboardLoaded}
                />
                <p className="help-block">
                  {t('A readable URL for your dashboard')}
@ -217,11 +245,11 @@ class PropertiesModal extends React.PureComponent {
                <Select
                  name="owners"
                  multi
-                  isLoading={!userOptions}
+                  isLoading={!ownerOptions}
                  value={values.owners}
-                  options={userOptions || []}
+                  options={ownerOptions || []}
                  onChange={this.onOwnersChange}
-                  disabled={!userOptions || !isOwnersLoaded}
+                  disabled={!ownerOptions || !isDashboardLoaded}
                />
                <p className="help-block">
                  {t('Owners is a list of users who can alter the dashboard.')}
--- a/superset-frontend/src/messageToasts/enhancers/withToasts.tsx
+++ b/superset-frontend/src/messageToasts/enhancers/withToasts.tsx
@ -16,6 +16,8 @@
 * specific language governing permissions and limitations
 * under the License.
 */
+
+import { ComponentType } from 'react';
 import { bindActionCreators } from 'redux';
 import { connect } from 'react-redux';

@ -27,7 +29,7 @@ import {
 } from '../actions';

 // To work properly the redux state must have a `messageToasts` subtree
-export default function withToasts(BaseComponent) {
+export default function withToasts(BaseComponent: ComponentType) {
  return connect(null, dispatch =>
    bindActionCreators(
      {
@ -38,5 +40,7 @@ export default function withToasts(BaseComponent) {
      },
      dispatch,
    ),
-  )(BaseComponent);
+  )(BaseComponent) as any;
+  // Rsedux has some confusing typings that cause problems for consumers of this function.
+  // If someone can fix the types, great, but for now it's just any.
 }
--- a/superset-frontend/src/views/dashboardList/DashboardList.tsx
+++ b/superset-frontend/src/views/dashboardList/DashboardList.tsx
@ -31,6 +31,7 @@ import {
  Filters,
 } from 'src/components/ListView/types';
 import withToasts from 'src/messageToasts/enhancers/withToasts';
+import PropertiesModal from 'src/dashboard/components/PropertiesModal';

 const PAGE_SIZE = 25;

@ -48,6 +49,7 @@ interface State {
  owners: Array<{ text: string; value: number }>;
  permissions: string[];
  lastFetchDataConfig: FetchDataConfig | null;
+  dashboardToEdit: Dashboard | null;
 }

 interface Dashboard {
@ -75,6 +77,7 @@ class DashboardList extends React.PureComponent<Props, State> {
    loading: false,
    owners: [],
    permissions: [],
+    dashboardToEdit: null,
  };

  componentDidMount() {
@ -183,7 +186,7 @@ class DashboardList extends React.PureComponent<Props, State> {
    {
      Cell: ({ row: { state, original } }: any) => {
        const handleDelete = () => this.handleDashboardDelete(original);
-        const handleEdit = () => this.handleDashboardEdit(original);
+        const handleEdit = () => this.openDashboardEditModal(original);
        const handleExport = () => this.handleBulkDashboardExport([original]);
        if (!this.canEdit && !this.canDelete && !this.canExport) {
          return null;
@ -251,8 +254,33 @@ class DashboardList extends React.PureComponent<Props, State> {
    return Boolean(this.state.permissions.find(p => p === perm));
  };

-  handleDashboardEdit = ({ id }: { id: number }) => {
-    window.location.assign(`/dashboard/edit/${id}`);
+  openDashboardEditModal = (dashboard: Dashboard) => {
+    this.setState({
+      dashboardToEdit: dashboard,
+    });
+  };
+
+  handleDashboardEdit = (edits: any) => {
+    this.setState({ loading: true });
+    return SupersetClient.get({
+      endpoint: `/api/v1/dashboard/${edits.id}`,
+    })
+      .then(({ json = {} }) => {
+        this.setState({
+          dashboards: this.state.dashboards.map(dashboard => {
+            if (dashboard.id === json.id) {
+              return json.result;
+            }
+            return dashboard;
+          }),
+          loading: false,
+        });
+      })
+      .catch(() => {
+        this.props.addDangerToast(
+          t('An error occurred while fetching Dashboards'),
+        );
+      });
  };

  handleDashboardDelete = ({
@ -388,7 +416,13 @@ class DashboardList extends React.PureComponent<Props, State> {
  };

  render() {
-    const { dashboards, dashboardCount, loading, filters } = this.state;
+    const {
+      dashboards,
+      dashboardCount,
+      loading,
+      filters,
+      dashboardToEdit,
+    } = this.state;

    return (
      <div className="container welcome">
@ -425,19 +459,29 @@ class DashboardList extends React.PureComponent<Props, State> {
                });
              }
              return (
-                <ListView
-                  className="dashboard-list-view"
-                  title={'Dashboards'}
-                  columns={this.columns}
-                  data={dashboards}
-                  count={dashboardCount}
-                  pageSize={PAGE_SIZE}
-                  fetchData={this.fetchData}
-                  loading={loading}
-                  initialSort={this.initialSort}
-                  filters={filters}
-                  bulkActions={bulkActions}
-                />
+                <>
+                  {dashboardToEdit && (
+                    <PropertiesModal
+                      show
+                      dashboardId={dashboardToEdit.id}
+                      onHide={() => this.setState({ dashboardToEdit: null })}
+                      onDashboardSave={this.handleDashboardEdit}
+                    />
+                  )}
+                  <ListView
+                    className="dashboard-list-view"
+                    title={'Dashboards'}
+                    columns={this.columns}
+                    data={dashboards}
+                    count={dashboardCount}
+                    pageSize={PAGE_SIZE}
+                    fetchData={this.fetchData}
+                    loading={loading}
+                    initialSort={this.initialSort}
+                    filters={filters}
+                    bulkActions={bulkActions}
+                  />
+                </>
              );
            }}
          </ConfirmStatusChange>
--- a/superset/views/dashboard/api.py
+++ b/superset/views/dashboard/api.py
@ -116,7 +116,7 @@ class DashboardPutSchema(BaseDashboardSchema):
    owners = fields.List(fields.Integer(validate=validate_owner))
    position_json = fields.String(validate=validate_json)
    css = fields.String()
-    json_metadata = fields.String(validate=validate_json_metadata)
+    json_metadata = fields.String(allow_none=True, validate=validate_json_metadata)
    published = fields.Boolean()

    @post_load
@ -142,14 +142,20 @@ class DashboardRestApi(DashboardMixin, BaseOwnedModelRestApi):

    class_permission_name = "DashboardModelView"
    show_columns = [
+        "id",
        "charts",
        "css",
        "dashboard_title",
        "json_metadata",
        "owners.id",
        "owners.username",
+        "changed_by_name",
+        "changed_by_url",
+        "changed_by.username",
+        "changed_on",
        "position_json",
        "published",
+        "url",
        "slug",
        "table_names",
    ]