From 46598830e9592feb552daa2e1aaf8c6a9defd042 Mon Sep 17 00:00:00 2001
From: David Aaron Suddjian <1858430+suddjian@users.noreply.github.com>
Date: Thu, 19 Mar 2020 08:05:35 -0700
Subject: [PATCH] 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
---
 .../dashboardList/DashboardList_spec.jsx      |  10 ++
 .../src/dashboard/components/Header.jsx       |   3 +-
 .../dashboard/components/PropertiesModal.jsx  | 118 +++++++++++-------
 .../{withToasts.jsx => withToasts.tsx}        |   8 +-
 .../src/views/dashboardList/DashboardList.tsx |  78 +++++++++---
 superset/views/dashboard/api.py               |   8 +-
 6 files changed, 158 insertions(+), 67 deletions(-)
 rename superset-frontend/src/messageToasts/enhancers/{withToasts.jsx => withToasts.tsx} (81%)

diff --git a/superset-frontend/spec/javascripts/views/dashboardList/DashboardList_spec.jsx b/superset-frontend/spec/javascripts/views/dashboardList/DashboardList_spec.jsx
index 86dd72990..ba04a8db5 100644
--- 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);
+  });
 });
diff --git a/superset-frontend/src/dashboard/components/Header.jsx b/superset-frontend/src/dashboard/components/Header.jsx
index c889d063c..77aba1d91 100644
--- 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 => {
diff --git a/superset-frontend/src/dashboard/components/PropertiesModal.jsx b/superset-frontend/src/dashboard/components/PropertiesModal.jsx
index a10fe8111..f266465b8 100644
--- 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.')}
diff --git a/superset-frontend/src/messageToasts/enhancers/withToasts.jsx b/superset-frontend/src/messageToasts/enhancers/withToasts.tsx
similarity index 81%
rename from superset-frontend/src/messageToasts/enhancers/withToasts.jsx
rename to superset-frontend/src/messageToasts/enhancers/withToasts.tsx
index 4bce7953c..5e4115251 100644
--- a/superset-frontend/src/messageToasts/enhancers/withToasts.jsx
+++ 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.
 }
diff --git a/superset-frontend/src/views/dashboardList/DashboardList.tsx b/superset-frontend/src/views/dashboardList/DashboardList.tsx
index 8ade11e4e..38b7d2b2b 100644
--- 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>
diff --git a/superset/views/dashboard/api.py b/superset/views/dashboard/api.py
index 496aa2fe4..f547cbf87 100644
--- 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",
     ]