From e660723a2f1ba7205878e8d600e01763514959cd Mon Sep 17 00:00:00 2001
From: Kamil Gabryjelski <kamil.gabryjelski@gmail.com>
Date: Wed, 6 Jan 2021 14:08:20 +0100
Subject: [PATCH] fix(explore): bugs in Custom SQL editor in filter popover
 (#12278)

* Fix Save button disabled until clause is switched

* Fix 'undefined undefined' when sql query is empty

* fix test

Co-authored-by: Ville Brofeldt <ville.v.brofeldt@gmail.com>
---
 ...FilterEditPopoverSimpleTabContent_spec.jsx |  2 +-
 .../spec/javascripts/explore/utils_spec.jsx   | 28 +++++++++++++++++++
 superset-frontend/src/explore/AdhocFilter.js  |  7 ++---
 .../AdhocFilterEditPopoverSqlTabContent.jsx   |  2 +-
 superset-frontend/src/explore/exploreUtils.js | 18 ++++++++++++
 5 files changed, 51 insertions(+), 6 deletions(-)

diff --git a/superset-frontend/spec/javascripts/explore/components/AdhocFilterEditPopoverSimpleTabContent_spec.jsx b/superset-frontend/spec/javascripts/explore/components/AdhocFilterEditPopoverSimpleTabContent_spec.jsx
index 3741aa1213532..91e5d3735a5e4 100644
--- a/superset-frontend/spec/javascripts/explore/components/AdhocFilterEditPopoverSimpleTabContent_spec.jsx
+++ b/superset-frontend/spec/javascripts/explore/components/AdhocFilterEditPopoverSimpleTabContent_spec.jsx
@@ -193,7 +193,7 @@ describe('AdhocFilterEditPopoverSimpleTabContent', () => {
         comparator: null,
         clause: 'WHERE',
         expressionType: 'SQL',
-        sqlExpression: "ds = '{{ presto.latest_partition('schema.table1') }}' ",
+        sqlExpression: "ds = '{{ presto.latest_partition('schema.table1') }}'",
       }),
     );
   });
diff --git a/superset-frontend/spec/javascripts/explore/utils_spec.jsx b/superset-frontend/spec/javascripts/explore/utils_spec.jsx
index 11a40b23762ae..5b68142714c38 100644
--- a/superset-frontend/spec/javascripts/explore/utils_spec.jsx
+++ b/superset-frontend/spec/javascripts/explore/utils_spec.jsx
@@ -25,6 +25,7 @@ import {
   getExploreLongUrl,
   getDataTablePageSize,
   shouldUseLegacyApi,
+  getSimpleSQLExpression,
 } from 'src/explore/exploreUtils';
 import {
   buildTimeRangeString,
@@ -298,4 +299,31 @@ describe('exploreUtils', () => {
       );
     });
   });
+
+  describe('getSimpleSQLExpression', () => {
+    const subject = 'subject';
+    const operator = '=';
+    const comparator = 'comparator';
+    it('returns empty string when subject is undefined', () => {
+      expect(getSimpleSQLExpression(undefined, '=', 10)).toBe('');
+      expect(getSimpleSQLExpression()).toBe('');
+    });
+    it('returns subject when its provided and operator is undefined', () => {
+      expect(getSimpleSQLExpression(subject, undefined, 10)).toBe(subject);
+      expect(getSimpleSQLExpression(subject)).toBe(subject);
+    });
+    it('returns subject and operator when theyre provided and comparator is undefined', () => {
+      expect(getSimpleSQLExpression(subject, operator)).toBe(
+        `${subject} ${operator}`,
+      );
+    });
+    it('returns full expression when subject, operator and comparator are provided', () => {
+      expect(getSimpleSQLExpression(subject, operator, comparator)).toBe(
+        `${subject} ${operator} ${comparator}`,
+      );
+      expect(getSimpleSQLExpression(subject, operator, comparator, true)).toBe(
+        `${subject} ${operator} ('${comparator}')`,
+      );
+    });
+  });
 });
diff --git a/superset-frontend/src/explore/AdhocFilter.js b/superset-frontend/src/explore/AdhocFilter.js
index c3af8cbbe2824..8c192c7792676 100644
--- a/superset-frontend/src/explore/AdhocFilter.js
+++ b/superset-frontend/src/explore/AdhocFilter.js
@@ -17,6 +17,7 @@
  * under the License.
  */
 import { MULTI_OPERATORS, CUSTOM_OPERATORS } from './constants';
+import { getSimpleSQLExpression } from './exploreUtils';
 
 export const EXPRESSION_TYPES = {
   SIMPLE: 'SIMPLE',
@@ -62,9 +63,7 @@ function translateToSql(adhocMetric, { useSimple } = {}) {
     const comparator = Array.isArray(adhocMetric.comparator)
       ? adhocMetric.comparator.join("','")
       : adhocMetric.comparator || '';
-    return `${subject} ${operator} ${isMulti ? "('" : ''}${comparator}${
-      isMulti ? "')" : ''
-    }`;
+    return getSimpleSQLExpression(subject, operator, comparator, isMulti);
   }
   if (adhocMetric.expressionType === EXPRESSION_TYPES.SQL) {
     return adhocMetric.sqlExpression;
@@ -79,7 +78,7 @@ export default class AdhocFilter {
       this.subject = adhocFilter.subject;
       this.operator = adhocFilter.operator?.toUpperCase();
       this.comparator = adhocFilter.comparator;
-      this.clause = adhocFilter.clause;
+      this.clause = adhocFilter.clause || CLAUSES.WHERE;
       this.sqlExpression = null;
     } else if (this.expressionType === EXPRESSION_TYPES.SQL) {
       this.sqlExpression =
diff --git a/superset-frontend/src/explore/components/AdhocFilterEditPopoverSqlTabContent.jsx b/superset-frontend/src/explore/components/AdhocFilterEditPopoverSqlTabContent.jsx
index 9bb168091fde8..ffd01751f5c15 100644
--- a/superset-frontend/src/explore/components/AdhocFilterEditPopoverSqlTabContent.jsx
+++ b/superset-frontend/src/explore/components/AdhocFilterEditPopoverSqlTabContent.jsx
@@ -90,7 +90,7 @@ export default class AdhocFilterEditPopoverSqlTabContent extends React.Component
 
     const clauseSelectProps = {
       placeholder: t('choose WHERE or HAVING...'),
-      value: adhocFilter.clause || CLAUSES.WHERE,
+      value: adhocFilter.clause,
       onChange: this.onSqlExpressionClauseChange,
     };
     const keywords = sqlKeywords.concat(
diff --git a/superset-frontend/src/explore/exploreUtils.js b/superset-frontend/src/explore/exploreUtils.js
index dc85c37e232b3..3cea751ea3dd0 100644
--- a/superset-frontend/src/explore/exploreUtils.js
+++ b/superset-frontend/src/explore/exploreUtils.js
@@ -308,3 +308,21 @@ export const useDebouncedEffect = (effect, delay) => {
     };
   }, [callback, delay]);
 };
+
+export const getSimpleSQLExpression = (
+  subject,
+  operator,
+  comparator,
+  isMulti,
+) => {
+  let expression = subject ?? '';
+  if (subject && operator) {
+    expression += ` ${operator}`;
+    if (comparator) {
+      expression += ` ${isMulti ? "('" : ''}${comparator}${
+        isMulti ? "')" : ''
+      }`;
+    }
+  }
+  return expression;
+};