chore(newchart): update chart creation dataset selection help text, s…

…tyles (#20369) * Update dataset selection help text. * Update 'Create a new chart' flow styles. * Add support for linking directly to Create Dataset modal via URL hash. * Add support for linking directly to Create Dataset modal via URL hash. * Update dataset help text to not include spaces in translated strings and only include an 'Add dataset' link when user has permission to add dataset. * Clean up test file Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
apache · Jun 21, 2022 · 44c5e28 · 44c5e28
1 parent f3b289d
commit 44c5e28
Show file tree

Hide file tree

Showing 6 changed files with 183 additions and 68 deletions.
diff --git a/superset-frontend/src/addSlice/AddSliceContainer.test.tsx b/superset-frontend/src/addSlice/AddSliceContainer.test.tsx
@@ -27,61 +27,97 @@ import AddSliceContainer, {
 import VizTypeGallery from 'src/explore/components/controls/VizTypeControl/VizTypeGallery';
 import { styledMount as mount } from 'spec/helpers/theming';
 import { act } from 'spec/helpers/testing-library';
+import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes';
 
 const datasource = {
   value: '1',
   label: 'table',
 };
 
-describe('AddSliceContainer', () => {
-  let wrapper: ReactWrapper<
+const mockUser: UserWithPermissionsAndRoles = {
+  createdOn: '2021-04-27T18:12:38.952304',
+  email: 'admin',
+  firstName: 'admin',
+  isActive: true,
+  lastName: 'admin',
+  permissions: {},
+  roles: { Admin: Array(173) },
+  userId: 1,
+  username: 'admin',
+  isAnonymous: false,
+};
+
+const mockUserWithDatasetWrite: UserWithPermissionsAndRoles = {
+  createdOn: '2021-04-27T18:12:38.952304',
+  email: 'admin',
+  firstName: 'admin',
+  isActive: true,
+  lastName: 'admin',
+  permissions: {},
+  roles: { Admin: [['can_write', 'Dataset']] },
+  userId: 1,
+  username: 'admin',
+  isAnonymous: false,
+};
+
+async function getWrapper(user = mockUser) {
+  const wrapper = mount(<AddSliceContainer user={user} />) as ReactWrapper<
     AddSliceContainerProps,
     AddSliceContainerState,
     AddSliceContainer
   >;
+  await act(() => new Promise(resolve => setTimeout(resolve, 0)));
+  return wrapper;
+}
 
-  beforeEach(async () => {
-    wrapper = mount(<AddSliceContainer />) as ReactWrapper<
-      AddSliceContainerProps,
-      AddSliceContainerState,
-      AddSliceContainer
-    >;
-    // suppress a warning caused by some unusual async behavior in Icon
-    await act(() => new Promise(resolve => setTimeout(resolve, 0)));
-  });
+test('renders a select and a VizTypeControl', async () => {
+  const wrapper = await getWrapper();
+  expect(wrapper.find(Select)).toExist();
+  expect(wrapper.find(VizTypeGallery)).toExist();
+});
 
-  it('renders a select and a VizTypeControl', () => {
-    expect(wrapper.find(Select)).toExist();
-    expect(wrapper.find(VizTypeGallery)).toExist();
-  });
+test('renders dataset help text when user lacks dataset write permissions', async () => {
+  const wrapper = await getWrapper();
+  expect(wrapper.find('[data-test="dataset-write"]')).not.toExist();
+  expect(wrapper.find('[data-test="no-dataset-write"]')).toExist();
+});
 
-  it('renders a button', () => {
-    expect(wrapper.find(Button)).toExist();
-  });
+test('renders dataset help text when user has dataset write permissions', async () => {
+  const wrapper = await getWrapper(mockUserWithDatasetWrite);
+  expect(wrapper.find('[data-test="dataset-write"]')).toExist();
+  expect(wrapper.find('[data-test="no-dataset-write"]')).not.toExist();
+});
 
-  it('renders a disabled button if no datasource is selected', () => {
-    expect(
-      wrapper.find(Button).find({ disabled: true }).hostNodes(),
-    ).toHaveLength(1);
-  });
+test('renders a button', async () => {
+  const wrapper = await getWrapper();
+  expect(wrapper.find(Button)).toExist();
+});
 
-  it('renders an enabled button if datasource and viz type is selected', () => {
-    wrapper.setState({
-      datasource,
-      visType: 'table',
-    });
-    expect(
-      wrapper.find(Button).find({ disabled: true }).hostNodes(),
-    ).toHaveLength(0);
+test('renders a disabled button if no datasource is selected', async () => {
+  const wrapper = await getWrapper();
+  expect(
+    wrapper.find(Button).find({ disabled: true }).hostNodes(),
+  ).toHaveLength(1);
+});
+
+test('renders an enabled button if datasource and viz type are selected', async () => {
+  const wrapper = await getWrapper();
+  wrapper.setState({
+    datasource,
+    visType: 'table',
   });
+  expect(
+    wrapper.find(Button).find({ disabled: true }).hostNodes(),
+  ).toHaveLength(0);
+});
 
-  it('formats explore url', () => {
-    wrapper.setState({
-      datasource,
-      visType: 'table',
-    });
-    const formattedUrl =
-      '/superset/explore/?form_data=%7B%22viz_type%22%3A%22table%22%2C%22datasource%22%3A%221%22%7D';
-    expect(wrapper.instance().exploreUrl()).toBe(formattedUrl);
+test('formats Explore url', async () => {
+  const wrapper = await getWrapper();
+  wrapper.setState({
+    datasource,
+    visType: 'table',
   });
+  const formattedUrl =
+    '/superset/explore/?form_data=%7B%22viz_type%22%3A%22table%22%2C%22datasource%22%3A%221%22%7D';
+  expect(wrapper.instance().exploreUrl()).toBe(formattedUrl);
 });
diff --git a/superset-frontend/src/addSlice/AddSliceContainer.tsx b/superset-frontend/src/addSlice/AddSliceContainer.tsx
@@ -24,12 +24,13 @@ import { URL_PARAMS } from 'src/constants';
 import { isNullish } from 'src/utils/common';
 import Button from 'src/components/Button';
 import { Select, Steps } from 'src/components';
-import { FormLabel } from 'src/components/Form';
 import { Tooltip } from 'src/components/Tooltip';
 
 import VizTypeGallery, {
   MAX_ADVISABLE_VIZ_GALLERY_WIDTH,
 } from 'src/explore/components/controls/VizTypeControl/VizTypeGallery';
+import findPermission from 'src/dashboard/util/findPermission';
+import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes';
 
 type Dataset = {
   id: number;
@@ -38,11 +39,14 @@ type Dataset = {
   datasource_type: string;
 };
 
-export type AddSliceContainerProps = {};
+export type AddSliceContainerProps = {
+  user: UserWithPermissionsAndRoles;
+};
 
 export type AddSliceContainerState = {
   datasource?: { label: string; value: string };
   visType: string | null;
+  canCreateDataset: boolean;
 };
 
 const ESTIMATED_NAV_HEIGHT = 56;
@@ -73,7 +77,6 @@ const StyledContainer = styled.div`
       display: flex;
       flex-direction: row;
       align-items: center;
-      margin-bottom: ${theme.gridUnit * 2}px;
 
       & > div {
         min-width: 200px;
@@ -180,6 +183,24 @@ const StyledLabel = styled.span`
   `}
 `;
 
+const StyledStepTitle = styled.span`
+  ${({
+    theme: {
+      typography: { sizes, weights },
+    },
+  }) => `
+      font-size: ${sizes.m}px;
+      font-weight: ${weights.bold};
+    `}
+`;
+
+const StyledStepDescription = styled.div`
+  ${({ theme: { gridUnit } }) => `
+    margin-top: ${gridUnit * 4}px;
+    margin-bottom: ${gridUnit * 3}px;
+  `}
+`;
+
 export default class AddSliceContainer extends React.PureComponent<
   AddSliceContainerProps,
   AddSliceContainerState
@@ -188,6 +209,11 @@ export default class AddSliceContainer extends React.PureComponent<
     super(props);
     this.state = {
       visType: null,
+      canCreateDataset: findPermission(
+        'can_write',
+        'Dataset',
+        props.user.roles,
+      ),
     };
 
     this.changeDatasource = this.changeDatasource.bind(this);
@@ -276,15 +302,49 @@ export default class AddSliceContainer extends React.PureComponent<
 
   render() {
     const isButtonDisabled = this.isBtnDisabled();
+    const datasetHelpText = this.state.canCreateDataset ? (
+      <span data-test="dataset-write">
+        <a
+          href="/tablemodelview/list/#create"
+          rel="noopener noreferrer"
+          target="_blank"
+        >
+          {t('Add a dataset')}
+        </a>
+        {` ${t('or')} `}
+        <a
+          href="https://superset.apache.org/docs/creating-charts-dashboards/creating-your-first-dashboard/#registering-a-new-table"
+          rel="noopener noreferrer"
+          target="_blank"
+        >
+          {`${t('view instructions')} `}
+          <i className="fa fa-external-link" />
+        </a>
+        .
+      </span>
+    ) : (
+      <span data-test="no-dataset-write">
+        <a
+          href="https://superset.apache.org/docs/creating-charts-dashboards/creating-your-first-dashboard/#registering-a-new-table"
+          rel="noopener noreferrer"
+          target="_blank"
+        >
+          {`${t('View instructions')} `}
+          <i className="fa fa-external-link" />
+        </a>
+        .
+      </span>
+    );
+
     return (
       <StyledContainer>
         <h3>{t('Create a new chart')}</h3>
         <Steps direction="vertical" size="small">
           <Steps.Step
-            title={<FormLabel>{t('Choose a dataset')}</FormLabel>}
+            title={<StyledStepTitle>{t('Choose a dataset')}</StyledStepTitle>}
             status={this.state.datasource?.value ? 'finish' : 'process'}
             description={
-              <div className="dataset">
+              <StyledStepDescription className="dataset">
                 <Select
                   autoFocus
                   ariaLabel={t('Dataset')}
@@ -296,30 +356,21 @@ export default class AddSliceContainer extends React.PureComponent<
                   showSearch
                   value={this.state.datasource}
                 />
-                <span>
-                  {t(
-                    'Instructions to add a dataset are available in the Superset tutorial.',
-                  )}{' '}
-                  <a
-                    href="https://superset.apache.org/docs/creating-charts-dashboards/creating-your-first-dashboard/#registering-a-new-table"
-                    rel="noopener noreferrer"
-                    target="_blank"
-                  >
-                    <i className="fa fa-external-link" />
-                  </a>
-                </span>
-              </div>
+                {datasetHelpText}
+              </StyledStepDescription>
             }
           />
           <Steps.Step
-            title={<FormLabel>{t('Choose chart type')}</FormLabel>}
+            title={<StyledStepTitle>{t('Choose chart type')}</StyledStepTitle>}
             status={this.state.visType ? 'finish' : 'process'}
             description={
-              <VizTypeGallery
-                className="viz-gallery"
-                onChange={this.changeVisType}
-                selectedViz={this.state.visType}
-              />
+              <StyledStepDescription>
+                <VizTypeGallery
+                  className="viz-gallery"
+                  onChange={this.changeVisType}
+                  selectedViz={this.state.visType}
+                />
+              </StyledStepDescription>
             }
           />
         </Steps>

diff --git a/superset-frontend/src/addSlice/App.tsx b/superset-frontend/src/addSlice/App.tsx
@@ -41,7 +41,7 @@ const App = () => (
   <ThemeProvider theme={theme}>
     <GlobalStyles />
     <DynamicPluginProvider>
-      <AddSliceContainer />
+      <AddSliceContainer user={bootstrapData.user} />
     </DynamicPluginProvider>
   </ThemeProvider>
 );

diff --git a/superset-frontend/src/views/CRUD/data/dataset/DatasetList.test.jsx b/superset-frontend/src/views/CRUD/data/dataset/DatasetList.test.jsx
@@ -183,6 +183,12 @@ describe('DatasetList', () => {
   });
 });
 
+jest.mock('react-router-dom', () => ({
+  ...jest.requireActual('react-router-dom'),
+  useLocation: () => ({}),
+  useHistory: () => ({}),
+}));
+
 describe('RTL', () => {
   async function renderAndWait() {
     const mounted = act(async () => {
@@ -191,7 +197,7 @@ describe('RTL', () => {
         <QueryParamProvider>
           <DatasetList {...mockedProps} user={mockUser} />
         </QueryParamProvider>,
-        { useRedux: true },
+        { useRedux: true, useRouter: true },
       );
     });
 

diff --git a/superset-frontend/src/views/CRUD/data/dataset/DatasetList.tsx b/superset-frontend/src/views/CRUD/data/dataset/DatasetList.tsx
@@ -22,8 +22,10 @@ import React, {
   useState,
   useMemo,
   useCallback,
+  useEffect,
 } from 'react';
 import rison from 'rison';
+import { useHistory, useLocation } from 'react-router-dom';
 import {
   createFetchRelated,
   createFetchDistinct,
@@ -553,14 +555,34 @@ const DatasetList: FunctionComponent<DatasetListProps> = ({
     });
   }
 
+  const CREATE_HASH = '#create';
+  const location = useLocation();
+  const history = useHistory();
+
+  //  Sync Dataset Add modal with #create hash
+  useEffect(() => {
+    const modalOpen = location.hash === CREATE_HASH && canCreate;
+    setDatasetAddModalOpen(modalOpen);
+  }, [canCreate, location.hash]);
+
+  //  Add #create hash
+  const openDatasetAddModal = useCallback(() => {
+    history.replace(`${location.pathname}${location.search}${CREATE_HASH}`);
+  }, [history, location.pathname, location.search]);
+
+  //  Remove #create hash
+  const closeDatasetAddModal = useCallback(() => {
+    history.replace(`${location.pathname}${location.search}`);
+  }, [history, location.pathname, location.search]);
+
   if (canCreate) {
     buttonArr.push({
       name: (
         <>
           <i className="fa fa-plus" /> {t('Dataset')}{' '}
         </>
       ),
-      onClick: () => setDatasetAddModalOpen(true),
+      onClick: openDatasetAddModal,
       buttonStyle: 'primary',
     });
 
@@ -631,7 +653,7 @@ const DatasetList: FunctionComponent<DatasetListProps> = ({
       <SubMenu {...menuData} />
       <AddDatasetModal
         show={datasetAddModalOpen}
-        onHide={() => setDatasetAddModalOpen(false)}
+        onHide={closeDatasetAddModal}
         onDatasetAdd={refreshData}
       />
       {datasetCurrentlyDeleting && (