fix: Revert "fix(list/chart views): Chart Properties modal now has tr…

…ansitions" (#30041)
apache · Aug 28, 2024 · 07985e2 · 07985e2
1 parent 75c500c
commit 07985e2
Show file tree

Hide file tree

Showing 4 changed files with 48 additions and 51 deletions.
diff --git a/superset-frontend/src/explore/components/PropertiesModal/index.tsx b/superset-frontend/src/explore/components/PropertiesModal/index.tsx
@@ -69,7 +69,7 @@ function PropertiesModal({
   const [submitting, setSubmitting] = useState(false);
   const [form] = AntdForm.useForm();
   // values of form inputs
-  const [name, setName] = useState(slice?.slice_name || '');
+  const [name, setName] = useState(slice.slice_name || '');
   const [selectedOwners, setSelectedOwners] = useState<SelectValue | null>(
     null,
   );
@@ -98,25 +98,23 @@ function PropertiesModal({
 
   const fetchChartOwners = useCallback(
     async function fetchChartOwners() {
-      if (slice?.slice_id) {
-        try {
-          const response = await SupersetClient.get({
-            endpoint: `/api/v1/chart/${slice?.slice_id}`,
-          });
-          const chart = response.json.result;
-          setSelectedOwners(
-            chart?.owners?.map((owner: any) => ({
-              value: owner.id,
-              label: `${owner.first_name} ${owner.last_name}`,
-            })),
-          );
-        } catch (response) {
-          const clientError = await getClientErrorObject(response);
-          showError(clientError);
-        }
+      try {
+        const response = await SupersetClient.get({
+          endpoint: `/api/v1/chart/${slice.slice_id}`,
+        });
+        const chart = response.json.result;
+        setSelectedOwners(
+          chart?.owners?.map((owner: any) => ({
+            value: owner.id,
+            label: `${owner.first_name} ${owner.last_name}`,
+          })),
+        );
+      } catch (response) {
+        const clientError = await getClientErrorObject(response);
+        showError(clientError);
       }
     },
-    [slice?.slice_id],
+    [slice.slice_id],
   );
 
   const loadOptions = useMemo(
@@ -177,7 +175,7 @@ function PropertiesModal({
 
     try {
       const res = await SupersetClient.put({
-        endpoint: `/api/v1/chart/${slice?.slice_id}`,
+        endpoint: `/api/v1/chart/${slice.slice_id}`,
         headers: { 'Content-Type': 'application/json' },
         body: JSON.stringify(payload),
       });
@@ -186,7 +184,7 @@ function PropertiesModal({
         ...payload,
         ...res.json.result,
         tags,
-        id: slice?.slice_id,
+        id: slice.slice_id,
         owners: selectedOwners,
       };
       onSave(updatedChart);
@@ -208,16 +206,16 @@ function PropertiesModal({
 
   // update name after it's changed in another modal
   useEffect(() => {
-    setName(slice?.slice_name || '');
-  }, [slice?.slice_name]);
+    setName(slice.slice_name || '');
+  }, [slice.slice_name]);
 
   useEffect(() => {
     if (!isFeatureEnabled(FeatureFlag.TaggingSystem)) return;
     try {
       fetchTags(
         {
           objectType: OBJECT_TYPES.CHART,
-          objectId: slice?.slice_id,
+          objectId: slice.slice_id,
           includeTypes: false,
         },
         (tags: TagType[]) => setTags(tags),
@@ -228,7 +226,7 @@ function PropertiesModal({
     } catch (error) {
       showError(error);
     }
-  }, [slice?.slice_id]);
+  }, [slice.slice_id]);
 
   const handleChangeTags = (tags: { label: string; value: number }[]) => {
     const parsedTags: TagType[] = ensureIsArray(tags).map(r => ({
@@ -264,9 +262,9 @@ function PropertiesModal({
             buttonSize="small"
             buttonStyle="primary"
             onClick={form.submit}
-            disabled={submitting || !name || slice?.is_managed_externally}
+            disabled={submitting || !name || slice.is_managed_externally}
             tooltip={
-              slice?.is_managed_externally
+              slice.is_managed_externally
                 ? t(
                     "This chart is managed externally, and can't be edited in Superset",
                   )
@@ -286,13 +284,12 @@ function PropertiesModal({
         onFinish={onSubmit}
         layout="vertical"
         initialValues={{
-          name: slice?.slice_name || '',
-          description: slice?.description || '',
-          cache_timeout:
-            slice?.cache_timeout != null ? slice.cache_timeout : '',
-          certified_by: slice?.certified_by || '',
+          name: slice.slice_name || '',
+          description: slice.description || '',
+          cache_timeout: slice.cache_timeout != null ? slice.cache_timeout : '',
+          certified_by: slice.certified_by || '',
           certification_details:
-            slice?.certified_by && slice?.certification_details
+            slice.certified_by && slice.certification_details
               ? slice.certification_details
               : '',
         }}

diff --git a/superset-frontend/src/features/home/ChartTable.tsx b/superset-frontend/src/features/home/ChartTable.tsx
@@ -171,12 +171,15 @@ function ChartTable({
   if (loading) return <LoadingCards cover={showThumbnails} />;
   return (
     <ErrorBoundary>
-      <PropertiesModal
-        onHide={closeChartEditModal}
-        onSave={handleChartUpdated}
-        show={sliceCurrentlyEditing}
-        slice={sliceCurrentlyEditing}
-      />
+      {sliceCurrentlyEditing && (
+        <PropertiesModal
+          onHide={closeChartEditModal}
+          onSave={handleChartUpdated}
+          show
+          slice={sliceCurrentlyEditing}
+        />
+      )}
+
       <SubMenu
         activeChild={activeTab}
         tabs={menuTabs}

diff --git a/superset-frontend/src/pages/ChartList/ChartList.test.jsx b/superset-frontend/src/pages/ChartList/ChartList.test.jsx
@@ -152,11 +152,6 @@ describe('ChartList', () => {
     expect(wrapper.find(ChartList)).toExist();
   });
 
-  it('renders, but PropertiesModal initially hidden', () => {
-    expect(wrapper.find(PropertiesModal).exists()).toBe(true);
-    expect(wrapper.find(PropertiesModal).prop('show')).toBe(false);
-  });
-
   it('renders a ListView', () => {
     expect(wrapper.find(ListView)).toExist();
   });
@@ -186,10 +181,10 @@ describe('ChartList', () => {
   });
 
   it('edits', async () => {
-    expect(wrapper.find(PropertiesModal).prop('show')).toBe(false);
+    expect(wrapper.find(PropertiesModal)).not.toExist();
     wrapper.find('[data-test="edit-alt"]').first().simulate('click');
     await waitForComponentToPaint(wrapper);
-    expect(wrapper.find(PropertiesModal).prop('show')).toBe(true);
+    expect(wrapper.find(PropertiesModal)).toExist();
   });
 
   it('delete', async () => {

diff --git a/superset-frontend/src/pages/ChartList/index.tsx b/superset-frontend/src/pages/ChartList/index.tsx
@@ -787,12 +787,14 @@ function ChartList(props: ChartListProps) {
   return (
     <>
       <SubMenu name={t('Charts')} buttons={subMenuButtons} />
-      <PropertiesModal
-        onHide={closeChartEditModal}
-        onSave={handleChartUpdated}
-        show={!!sliceCurrentlyEditing}
-        slice={sliceCurrentlyEditing}
-      />
+      {sliceCurrentlyEditing && (
+        <PropertiesModal
+          onHide={closeChartEditModal}
+          onSave={handleChartUpdated}
+          show
+          slice={sliceCurrentlyEditing}
+        />
+      )}
       <ConfirmStatusChange
         title={t('Please confirm')}
         description={t('Are you sure you want to delete the selected charts?')}