fix: Uneditable descriptions with no value or possible actions should not show header (#533)

* added renderDescription method

* simplified method

* Added unit test cases

* modified logic and unit test cases

* code cleanup

* incorporated PR review suggestions

* minor changes to unit test cases

amundsen_application/static/js/components/TableDetail/ColumnListItem/index.spec.tsx

@@ -68,9 +68,9 @@ describe('ColumnListItem', () => {
 
     it('turns expanded state to the opposite state', () => {
       setStateSpy.mockClear();
-      const { isExpanded } = instance.state;
+      const prevState = instance.state;
       instance.toggleExpand(null);
-      expect(setStateSpy).toHaveBeenCalledWith({ isExpanded: !isExpanded });
+      expect(setStateSpy).toHaveBeenCalled();
     });
   });
 
@@ -124,27 +124,137 @@ describe('ColumnListItem', () => {
       expect(wrapper.find('.column-dropdown').exists()).toBe(false);
     });
 
-    it('renders column stats and editable text when expanded', () => {
-      instance.setState({ isExpanded: true });
-      const newWrapper = shallow(instance.render());
-      expect(newWrapper.find('.expanded-content').exists()).toBe(true);
-      expect(newWrapper.find(ColumnDescEditableText).exists()).toBe(true);
-      expect(newWrapper.find(ColumnStats).exists()).toBe(true);
-    });
+    describe('when expanded', () => {
+      it('renders column stats and editable text', () => {
+        instance.setState({ isExpanded: true });
+        const newWrapper = shallow(instance.render());
+        expect(newWrapper.find('.expanded-content').exists()).toBe(true);
+        expect(newWrapper.find(EditableSection).exists()).toBe(true);
+        expect(newWrapper.find(ColumnDescEditableText).exists()).toBe(true);
+        expect(newWrapper.find(ColumnStats).exists()).toBe(true);
+      });
 
-    it('renders EditableSection with correct edit text and Url when expanded', () => {
-      instance.setState({ isExpanded: true });
-      const newWrapper = shallow(instance.render());
-      expect(newWrapper.find(EditableSection).exists()).toBe(true);
-      const editableSection = newWrapper.find(EditableSection);
-      expect(editableSection.props()).toMatchObject({
-        title: 'Description',
-        readOnly: !props.data.is_editable,
-        editText: props.editText,
-        editUrl: props.editUrl,
+      it('renders EditableSection with non-empty description, edit text and url', () => {
+        instance.setState({ isExpanded: true });
+        const newWrapper = shallow(instance.render());
+        const editableSection = newWrapper.find(EditableSection);
+        expect(editableSection.props()).toMatchObject({
+          title: 'Description',
+          readOnly: !props.data.is_editable,
+          editText: props.editText,
+          editUrl: props.editUrl,
+        });
       });
-    });
 
+      it('renders EditableSection with non-empty description, empty edit text and url', () => {
+        const { props, wrapper } = setup({
+          editText: '',
+          editUrl: '',
+        });
+        const instance = wrapper.instance();
+        instance.setState({ isExpanded: true });
+        const newWrapper = shallow(instance.render());
+        expect(newWrapper.find(EditableSection).exists()).toBe(true);
+        const editableSection = newWrapper.find(EditableSection);
+        expect(editableSection.props()).toMatchObject({
+          title: 'Description',
+          readOnly: !props.data.is_editable,
+          editText: props.editText,
+          editUrl: props.editUrl,
+        });
+      });
+
+      describe('when empty description', () => {
+        it('renders EditableSection with empty edit text and url for editable column description', () => {
+          const { props, wrapper } = setup({
+            data: {
+              name: 'test_column_name',
+              description: '',
+              is_editable: true,
+              col_type: 'varchar(32)',
+              stats: [
+                {
+                  end_epoch: 1571616000,
+                  start_epoch: 1571616000,
+                  stat_type: 'count',
+                  stat_val: '12345',
+                },
+              ],
+            },
+            editText: '',
+            editUrl: '',
+          });
+          const instance = wrapper.instance();
+          instance.setState({ isExpanded: true });
+          const newWrapper = shallow(instance.render());
+          expect(newWrapper.find(EditableSection).exists()).toBe(true);
+          const editableSection = newWrapper.find(EditableSection);
+          expect(editableSection.props()).toMatchObject({
+            title: 'Description',
+            readOnly: !props.data.is_editable,
+            editText: props.editText,
+            editUrl: props.editUrl,
+          });
+        });
+
+        it('does not render EditableSection with empty edit text and url for non-editable column description', () => {
+          const { wrapper } = setup({
+            data: {
+              name: 'test_column_name',
+              description: '',
+              is_editable: false,
+              col_type: 'varchar(32)',
+              stats: [
+                {
+                  end_epoch: 1571616000,
+                  start_epoch: 1571616000,
+                  stat_type: 'count',
+                  stat_val: '12345',
+                },
+              ],
+            },
+            editText: '',
+            editUrl: '',
+          });
+          const instance = wrapper.instance();
+          instance.setState({ isExpanded: true });
+          const newWrapper = shallow(instance.render());
+          expect(newWrapper.find(EditableSection).exists()).toBe(false);
+        });
+
+        it('renders EditableSection with non-empty edit text and url for non-editable column description', () => {
+          const { props, wrapper } = setup({
+            data: {
+              name: 'test_column_name',
+              description: '',
+              is_editable: false,
+              col_type: 'varchar(32)',
+              stats: [
+                {
+                  end_epoch: 1571616000,
+                  start_epoch: 1571616000,
+                  stat_type: 'count',
+                  stat_val: '12345',
+                },
+              ],
+            },
+            editText: 'Click to edit description in source',
+            editUrl: 'source/test_column_name',
+          });
+          const instance = wrapper.instance();
+          instance.setState({ isExpanded: true });
+          const newWrapper = shallow(instance.render());
+          expect(newWrapper.find(EditableSection).exists()).toBe(true);
+          const editableSection = newWrapper.find(EditableSection);
+          expect(editableSection.props()).toMatchObject({
+            title: 'Description',
+            readOnly: !props.data.is_editable,
+            editText: props.editText,
+            editUrl: props.editUrl,
+          });
+        });
+      });
+    });
     describe('when not expanded', () => {
       let newWrapper;
       beforeAll(() => {

amundsen_application/static/js/components/TableDetail/ColumnListItem/index.tsx

@@ -18,6 +18,7 @@ import './styles.scss';
 import EditableSection from 'components/common/EditableSection';
 
 const MORE_BUTTON_TEXT = 'More options';
+const EDITABLE_SECTION_TITLE = 'Description';
 
 interface DispatchFromProps {
   openRequestDescriptionDialog: (
@@ -51,15 +52,19 @@ export class ColumnListItem extends React.Component<
   }
 
   toggleExpand = (e) => {
+    const metadata = this.props.data;
     if (!this.state.isExpanded) {
-      const metadata = this.props.data;
       logClick(e, {
         target_id: `column::${metadata.name}`,
         target_type: 'column stats',
         label: `${metadata.name} ${metadata.col_type}`,
       });
     }
-    this.setState({ isExpanded: !this.state.isExpanded });
+    if (this.shouldRenderDescription() || metadata.stats.length !== 0) {
+      this.setState((prevState) => ({
+        isExpanded: !prevState.isExpanded,
+      }));
+    }
   };
 
   openRequest = () => {
@@ -73,6 +78,17 @@ export class ColumnListItem extends React.Component<
     e.stopPropagation();
   };
 
+  shouldRenderDescription = (): boolean => {
+    const { data, editText, editUrl } = this.props;
+    if (data.description) {
+      return true;
+    }
+    if (!editText && !editUrl && !data.is_editable) {
+      return false;
+    }
+    return true;
+  };
+
   renderColumnType = (columnIndex: number, type: string) => {
     const truncatedTypes: string[] = ['array', 'struct', 'map', 'row'];
     let shouldTrucate = false;
@@ -172,19 +188,21 @@ export class ColumnListItem extends React.Component<
           {this.state.isExpanded && (
             <section className="expanded-content">
               <div className="stop-propagation" onClick={this.stopPropagation}>
-                <EditableSection
-                  title="Description"
-                  readOnly={!metadata.is_editable}
-                  editText={this.props.editText}
-                  editUrl={this.props.editUrl}
-                >
-                  <ColumnDescEditableText
-                    columnIndex={this.props.index}
-                    editable={metadata.is_editable}
-                    maxLength={getMaxLength('columnDescLength')}
-                    value={metadata.description}
-                  />
-                </EditableSection>
+                {this.shouldRenderDescription() && (
+                  <EditableSection
+                    title={EDITABLE_SECTION_TITLE}
+                    readOnly={!metadata.is_editable}
+                    editText={this.props.editText}
+                    editUrl={this.props.editUrl}
+                  >
+                    <ColumnDescEditableText
+                      columnIndex={this.props.index}
+                      editable={metadata.is_editable}
+                      maxLength={getMaxLength('columnDescLength')}
+                      value={metadata.description}
+                    />
+                  </EditableSection>
+                )}
               </div>
               <ColumnStats stats={metadata.stats} />
             </section>