From 0dbaaad83d9a80b1c3ea46af7859f7f4f7d677e0 Mon Sep 17 00:00:00 2001
From: Anthony Gainor <anthony@againor.com>
Date: Wed, 7 Dec 2022 11:25:54 -0600
Subject: [PATCH] feat(explore-popover): Show disabled 'Save' button in explore
 popover (#21318)

Co-authored-by: Herbert Gainor <herbert.gainor@preset.io>
Co-authored-by: Michael S. Molina <michael.s.molina@gmail.com>
---
 .../ColumnSelectPopover.tsx                   |  6 +--
 .../AdhocFilterEditPopover.test.jsx           |  3 +-
 .../AdhocFilterEditPopover/index.jsx          |  8 ++--
 .../AdhocMetricEditPopover.test.jsx           |  6 +--
 .../AdhocMetricEditPopover.test.tsx           | 46 ++++++++++++-------
 .../AdhocMetricEditPopover/index.jsx          |  6 +--
 6 files changed, 42 insertions(+), 33 deletions(-)

diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopover.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopover.tsx
index 1eb77afb3e..fee2eb941f 100644
--- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopover.tsx
+++ b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopover.tsx
@@ -406,10 +406,8 @@ const ColumnSelectPopover = ({
           {t('Close')}
         </Button>
         <Button
-          disabled={!stateIsValid}
-          buttonStyle={
-            hasUnsavedChanges && stateIsValid ? 'primary' : 'default'
-          }
+          disabled={!stateIsValid || !hasUnsavedChanges}
+          buttonStyle="primary"
           buttonSize="small"
           onClick={onSave}
           data-test="ColumnEdit#save"
diff --git a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopover/AdhocFilterEditPopover.test.jsx b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopover/AdhocFilterEditPopover.test.jsx
index 907d78ff64..3fec073f94 100644
--- a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopover/AdhocFilterEditPopover.test.jsx
+++ b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopover/AdhocFilterEditPopover.test.jsx
@@ -122,7 +122,7 @@ describe('AdhocFilterEditPopover', () => {
 
   it('prevents saving if the filter is invalid', () => {
     const { wrapper } = setup();
-    expect(wrapper.find(Button).find({ disabled: true })).not.toExist();
+    expect(wrapper.find(Button).find({ disabled: true })).toExist();
     wrapper
       .instance()
       .onAdhocFilterChange(simpleAdhocFilter.duplicateWith({ operator: null }));
@@ -133,7 +133,6 @@ describe('AdhocFilterEditPopover', () => {
 
   it('highlights save if changes are present', () => {
     const { wrapper } = setup();
-    expect(wrapper.find(Button).find({ buttonStyle: 'primary' })).not.toExist();
     wrapper.instance().onAdhocFilterChange(sqlAdhocFilter);
     expect(wrapper.find(Button).find({ buttonStyle: 'primary' })).toExist();
   });
diff --git a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopover/index.jsx b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopover/index.jsx
index c39a497f73..fc3d5a3a01 100644
--- a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopover/index.jsx
+++ b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopover/index.jsx
@@ -245,10 +245,12 @@ export default class AdhocFilterEditPopover extends React.Component {
           </Button>
           <Button
             data-test="adhoc-filter-edit-popover-save-button"
-            disabled={!stateIsValid || !this.state.isSimpleTabValid}
-            buttonStyle={
-              hasUnsavedChanges && stateIsValid ? 'primary' : 'default'
+            disabled={
+              !stateIsValid ||
+              !this.state.isSimpleTabValid ||
+              !hasUnsavedChanges
             }
+            buttonStyle="primary"
             buttonSize="small"
             className="m-r-5"
             onClick={this.onSave}
diff --git a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/AdhocMetricEditPopover.test.jsx b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/AdhocMetricEditPopover.test.jsx
index 1025131e43..5fdd80e9d3 100644
--- a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/AdhocMetricEditPopover.test.jsx
+++ b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/AdhocMetricEditPopover.test.jsx
@@ -98,7 +98,7 @@ describe('AdhocMetricEditPopover', () => {
 
   it('prevents saving if no column or aggregate is chosen', () => {
     const { wrapper } = setup();
-    expect(wrapper.find(Button).find({ disabled: true })).not.toExist();
+    expect(wrapper.find(Button).find({ disabled: false })).not.toExist();
     wrapper.instance().onColumnChange(null);
     expect(wrapper.find(Button).find({ disabled: true })).toExist();
     wrapper.instance().onColumnChange(columns[0].column_name);
@@ -109,9 +109,9 @@ describe('AdhocMetricEditPopover', () => {
 
   it('highlights save if changes are present', () => {
     const { wrapper } = setup();
-    expect(wrapper.find(Button).find({ buttonStyle: 'primary' })).not.toExist();
+    expect(wrapper.find(Button).find({ disabled: true })).toExist();
     wrapper.instance().onColumnChange(columns[1].column_name);
-    expect(wrapper.find(Button).find({ buttonStyle: 'primary' })).toExist();
+    expect(wrapper.find(Button).find({ disabled: true })).not.toExist();
   });
 
   it('will initiate a drag when clicked', () => {
diff --git a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/AdhocMetricEditPopover.test.tsx b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/AdhocMetricEditPopover.test.tsx
index b364fa67a2..8829d4e283 100644
--- a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/AdhocMetricEditPopover.test.tsx
+++ b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/AdhocMetricEditPopover.test.tsx
@@ -18,7 +18,7 @@
  */
 import userEvent from '@testing-library/user-event';
 import React from 'react';
-import { render, screen } from 'spec/helpers/testing-library';
+import { render, screen, waitFor, within } from 'spec/helpers/testing-library';
 import AdhocMetric from 'src/explore/components/controls/MetricControl/AdhocMetric';
 import AdhocMetricEditPopover from '.';
 
@@ -35,10 +35,15 @@ const createProps = () => ({
   },
   savedMetricsOptions: [
     {
-      id: 65,
+      id: 64,
       metric_name: 'count',
       expression: 'COUNT(*)',
     },
+    {
+      id: 65,
+      metric_name: 'sum',
+      expression: 'sum(num)',
+    },
   ],
   adhocMetric: new AdhocMetric({ isNew: true }),
   datasource: {
@@ -118,26 +123,33 @@ test('Clicking on "Close" should call onClose', () => {
   expect(props.onClose).toBeCalledTimes(1);
 });
 
-test('Clicking on "Save" should call onChange and onClose', () => {
+test('Clicking on "Save" should call onChange and onClose', async () => {
+  const props = createProps();
+  render(<AdhocMetricEditPopover {...props} />);
+  expect(props.onChange).toBeCalledTimes(0);
+  expect(props.onClose).toBeCalledTimes(0);
+  userEvent.click(
+    screen.getByRole('combobox', {
+      name: 'Select saved metrics',
+    }),
+  );
+  const sumOption = await waitFor(() =>
+    within(document.querySelector('.rc-virtual-list')!).getByText('sum'),
+  );
+  userEvent.click(sumOption);
+  userEvent.click(screen.getByRole('button', { name: 'Save' }));
+  expect(props.onChange).toBeCalledTimes(1);
+  expect(props.onClose).toBeCalledTimes(1);
+});
+
+test('Clicking on "Save" should not call onChange and onClose', () => {
   const props = createProps();
   render(<AdhocMetricEditPopover {...props} />);
   expect(props.onChange).toBeCalledTimes(0);
   expect(props.onClose).toBeCalledTimes(0);
   userEvent.click(screen.getByRole('button', { name: 'Save' }));
-  expect(props.onChange).toBeCalledTimes(1);
-  expect(props.onChange).toBeCalledWith(
-    {
-      id: 64,
-      metric_name: 'count',
-      expression: 'COUNT(*)',
-    },
-    {
-      id: 64,
-      metric_name: 'count',
-      expression: 'COUNT(*)',
-    },
-  );
-  expect(props.onClose).toBeCalledTimes(1);
+  expect(props.onChange).toBeCalledTimes(0);
+  expect(props.onClose).toBeCalledTimes(0);
 });
 
 test('Should switch to tab:Simple', () => {
diff --git a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx
index 8c0bed1021..f49eb5b4f7 100644
--- a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx
+++ b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx
@@ -487,10 +487,8 @@ export default class AdhocMetricEditPopover extends React.PureComponent {
             {t('Close')}
           </Button>
           <Button
-            disabled={!stateIsValid}
-            buttonStyle={
-              hasUnsavedChanges && stateIsValid ? 'primary' : 'default'
-            }
+            disabled={!stateIsValid || !hasUnsavedChanges}
+            buttonStyle="primary"
             buttonSize="small"
             data-test="AdhocMetricEdit#save"
             onClick={this.onSave}