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>

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') }}'",
       }),
     );
   });

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}')`,
+      );
+    });
+  });
 });

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 =

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(

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;
+};