feat: Add Saved Metrics tab to metrics popover (apache#12123)

* Implement saved metrics * Fix bug in sql editor * Fix unit tests * Fix outlines in popovers * Add types for saved metrics * Add translations * Move savedMetricType to a separate file
preset-io · Jan 7, 2021 · 9581444 · 9581444
1 parent 234639b
commit 9581444
Show file tree

Hide file tree

Showing 15 changed files with 239 additions and 152 deletions.
diff --git a/superset-frontend/spec/javascripts/explore/components/AdhocMetricEditPopover_spec.jsx b/superset-frontend/spec/javascripts/explore/components/AdhocMetricEditPopover_spec.jsx
@@ -49,6 +49,8 @@ function setup(overrides) {
   const onClose = sinon.spy();
   const props = {
     adhocMetric: sumValueAdhocMetric,
+    savedMetric: {},
+    savedMetrics: [],
     onChange,
     onClose,
     onResize: () => {},
@@ -62,7 +64,7 @@ function setup(overrides) {
 describe('AdhocMetricEditPopover', () => {
   it('renders a popover with edit metric form contents', () => {
     const { wrapper } = setup();
-    expect(wrapper.find(FormGroup)).toHaveLength(3);
+    expect(wrapper.find(FormGroup)).toHaveLength(4);
     expect(wrapper.find(Button)).toHaveLength(2);
   });
 

diff --git a/superset-frontend/spec/javascripts/explore/components/AdhocMetricOption_spec.jsx b/superset-frontend/spec/javascripts/explore/components/AdhocMetricOption_spec.jsx
@@ -41,11 +41,15 @@ function setup(overrides) {
   const onMetricEdit = sinon.spy();
   const props = {
     adhocMetric: sumValueAdhocMetric,
+    savedMetric: {},
+    savedMetrics: [],
     onMetricEdit,
     columns,
     ...overrides,
   };
-  const wrapper = shallow(<AdhocMetricOption {...props} />);
+  const wrapper = shallow(<AdhocMetricOption {...props} />)
+    .find('AdhocMetricPopoverTrigger')
+    .shallow();
   return { wrapper, onMetricEdit };
 }
 
@@ -56,13 +60,6 @@ describe('AdhocMetricOption', () => {
     expect(wrapper.find('OptionControlLabel')).toExist();
   });
 
-  it('overlay should open if metric is new', () => {
-    const { wrapper } = setup({
-      adhocMetric: sumValueAdhocMetric.duplicateWith({ isNew: true }),
-    });
-    expect(wrapper.find(Popover).props().defaultVisible).toBe(true);
-  });
-
   it('overwrites the adhocMetric in state with onLabelChange', () => {
     const { wrapper } = setup();
     wrapper.instance().onLabelChange({ target: { value: 'new label' } });

diff --git a/superset-frontend/spec/javascripts/explore/components/MetricDefinitionValue_spec.jsx b/superset-frontend/spec/javascripts/explore/components/MetricDefinitionValue_spec.jsx
@@ -35,7 +35,7 @@ describe('MetricDefinitionValue', () => {
     const wrapper = shallow(
       <MetricDefinitionValue option={{ metric_name: 'a_saved_metric' }} />,
     );
-    expect(wrapper.find('OptionControlLabel')).toExist();
+    expect(wrapper.find('AdhocMetricOption')).toExist();
   });
 
   it('renders an AdhocMetricOption given an adhoc metric', () => {

diff --git a/superset-frontend/spec/javascripts/explore/components/MetricsControl_spec.jsx b/superset-frontend/spec/javascripts/explore/components/MetricsControl_spec.jsx
@@ -169,7 +169,7 @@ describe('MetricsControl', () => {
       const editedMetric = sumValueAdhocMetric.duplicateWith({
         aggregate: AGGREGATES.AVG,
       });
-      component.instance().onMetricEdit(editedMetric);
+      component.instance().onMetricEdit(editedMetric, sumValueAdhocMetric);
 
       expect(onChange.lastCall.args).toEqual([[editedMetric]]);
     });

diff --git a/superset-frontend/src/common/components/Tabs/Tabs.tsx b/superset-frontend/src/common/components/Tabs/Tabs.tsx
@@ -23,15 +23,18 @@ import Icon from 'src/components/Icon';
 
 interface TabsProps {
   fullWidth?: boolean;
+  allowOverflow?: boolean;
 }
 
-const notForwardedProps = ['fullWidth'];
+const notForwardedProps = ['fullWidth', 'allowOverflow'];
 
 const StyledTabs = styled(AntdTabs, {
   shouldForwardProp: prop => !notForwardedProps.includes(prop),
 })<TabsProps>`
+  overflow: ${({ allowOverflow }) => (allowOverflow ? 'visible' : 'hidden')};
+
   .ant-tabs-content-holder {
-    overflow: auto;
+    overflow: ${({ allowOverflow }) => (allowOverflow ? 'visible' : 'auto')};
   }
 
   .ant-tabs-tab {

diff --git a/superset-frontend/src/explore/AdhocMetric.js b/superset-frontend/src/explore/AdhocMetric.js
@@ -92,9 +92,12 @@ export default class AdhocMetric {
 
   translateToSql() {
     if (this.expressionType === EXPRESSION_TYPES.SIMPLE) {
-      return `${this.aggregate || ''}(${
-        (this.column && this.column.column_name) || ''
-      })`;
+      const aggregate = this.aggregate || '';
+      // eslint-disable-next-line camelcase
+      const column = this.column?.column_name
+        ? `(${this.column.column_name})`
+        : '';
+      return aggregate + column;
     }
     if (this.expressionType === EXPRESSION_TYPES.SQL) {
       return this.sqlExpression;

diff --git a/superset-frontend/src/explore/components/AdhocFilterEditPopover.jsx b/superset-frontend/src/explore/components/AdhocFilterEditPopover.jsx
@@ -150,11 +150,12 @@ export default class AdhocFilterEditPopover extends React.Component {
           className="adhoc-filter-edit-tabs"
           data-test="adhoc-filter-edit-tabs"
           style={{ height: this.state.height, width: this.state.width }}
+          allowOverflow
         >
           <Tabs.TabPane
             className="adhoc-filter-edit-tab"
             key={EXPRESSION_TYPES.SIMPLE}
-            tab="Simple"
+            tab={t('Simple')}
           >
             <AdhocFilterEditPopoverSimpleTabContent
               adhocFilter={this.state.adhocFilter}
@@ -169,7 +170,7 @@ export default class AdhocFilterEditPopover extends React.Component {
           <Tabs.TabPane
             className="adhoc-filter-edit-tab"
             key={EXPRESSION_TYPES.SQL}
-            tab="Custom SQL"
+            tab={t('Custom SQL')}
           >
             {!this.props.datasource ||
             this.props.datasource.type !== 'druid' ? (

diff --git a/superset-frontend/src/explore/components/AdhocMetricEditPopover.jsx b/superset-frontend/src/explore/components/AdhocMetricEditPopover.jsx
@@ -16,6 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
+/* eslint-disable camelcase */
 import React from 'react';
 import PropTypes from 'prop-types';
 import { FormGroup } from 'react-bootstrap';
@@ -31,6 +32,7 @@ import sqlKeywords from 'src/SqlLab/utils/sqlKeywords';
 
 import { AGGREGATES_OPTIONS } from '../constants';
 import columnType from '../propTypes/columnType';
+import savedMetricType from '../propTypes/savedMetricType';
 import AdhocMetric, { EXPRESSION_TYPES } from '../AdhocMetric';
 
 const propTypes = {
@@ -39,6 +41,8 @@ const propTypes = {
   onClose: PropTypes.func.isRequired,
   onResize: PropTypes.func.isRequired,
   columns: PropTypes.arrayOf(columnType),
+  savedMetrics: PropTypes.arrayOf(savedMetricType),
+  savedMetric: savedMetricType,
   datasourceType: PropTypes.string,
   title: PropTypes.shape({
     label: PropTypes.string,
@@ -54,6 +58,8 @@ const ResizeIcon = styled.i`
   margin-left: ${({ theme }) => theme.gridUnit * 2}px;
 `;
 
+const SAVED_TAB_KEY = 'SAVED';
+
 const startingWidth = 320;
 const startingHeight = 240;
 
@@ -63,6 +69,7 @@ export default class AdhocMetricEditPopover extends React.Component {
     this.onSave = this.onSave.bind(this);
     this.onColumnChange = this.onColumnChange.bind(this);
     this.onAggregateChange = this.onAggregateChange.bind(this);
+    this.onSavedMetricChange = this.onSavedMetricChange.bind(this);
     this.onSqlExpressionChange = this.onSqlExpressionChange.bind(this);
     this.onDragDown = this.onDragDown.bind(this);
     this.onMouseMove = this.onMouseMove.bind(this);
@@ -72,6 +79,7 @@ export default class AdhocMetricEditPopover extends React.Component {
 
     this.state = {
       adhocMetric: this.props.adhocMetric,
+      savedMetric: this.props.savedMetric,
       width: startingWidth,
       height: startingHeight,
     };
@@ -88,16 +96,24 @@ export default class AdhocMetricEditPopover extends React.Component {
     const { title } = this.props;
     const { hasCustomLabel } = title;
     let { label } = title;
-    const { adhocMetric } = this.state;
+    const { adhocMetric, savedMetric } = this.state;
     const metricLabel = adhocMetric.label;
     if (!hasCustomLabel) {
       label = metricLabel;
     }
-    this.props.onChange({
-      ...adhocMetric,
-      label,
-      hasCustomLabel,
-    });
+
+    const metric = savedMetric?.metric_name ? savedMetric : adhocMetric;
+    const oldMetric = this.props.savedMetric?.metric_name
+      ? this.props.savedMetric
+      : this.props.adhocMetric;
+    this.props.onChange(
+      {
+        ...metric,
+        label,
+        hasCustomLabel,
+      },
+      oldMetric,
+    );
     this.props.onClose();
   }
 
@@ -108,6 +124,7 @@ export default class AdhocMetricEditPopover extends React.Component {
         column,
         expressionType: EXPRESSION_TYPES.SIMPLE,
       }),
+      savedMetric: undefined,
     }));
   }
 
@@ -118,6 +135,22 @@ export default class AdhocMetricEditPopover extends React.Component {
         aggregate,
         expressionType: EXPRESSION_TYPES.SIMPLE,
       }),
+      savedMetric: undefined,
+    }));
+  }
+
+  onSavedMetricChange(savedMetricId) {
+    const savedMetric = this.props.savedMetrics.find(
+      metric => metric.id === savedMetricId,
+    );
+    this.setState(prevState => ({
+      savedMetric,
+      adhocMetric: prevState.adhocMetric.duplicateWith({
+        column: undefined,
+        aggregate: undefined,
+        sqlExpression: undefined,
+        expressionType: EXPRESSION_TYPES.SIMPLE,
+      }),
     }));
   }
 
@@ -127,6 +160,7 @@ export default class AdhocMetricEditPopover extends React.Component {
         sqlExpression,
         expressionType: EXPRESSION_TYPES.SQL,
       }),
+      savedMetric: undefined,
     }));
   }
 
@@ -171,21 +205,27 @@ export default class AdhocMetricEditPopover extends React.Component {
   }
 
   renderColumnOption(option) {
-    return <ColumnOption column={option} showType />;
+    const column = { ...option };
+    if (column.metric_name && !column.verbose_name) {
+      column.verbose_name = column.metric_name;
+    }
+    return <ColumnOption column={column} showType />;
   }
 
   render() {
     const {
       adhocMetric: propsAdhocMetric,
+      savedMetric: propsSavedMetric,
       columns,
+      savedMetrics,
       onChange,
       onClose,
       onResize,
       datasourceType,
       ...popoverProps
     } = this.props;
 
-    const { adhocMetric } = this.state;
+    const { adhocMetric, savedMetric } = this.state;
     const keywords = sqlKeywords.concat(
       columns.map(column => ({
         name: column.column_name,
@@ -220,14 +260,32 @@ export default class AdhocMetricEditPopover extends React.Component {
       showSearch: true,
     };
 
+    const savedSelectProps = {
+      placeholder: t('%s saved metric(s)', savedMetrics?.length ?? 0),
+      value: savedMetric?.verbose_name || savedMetric?.metric_name,
+      onChange: this.onSavedMetricChange,
+      allowClear: true,
+      showSearch: true,
+      autoFocus: true,
+      filterOption: (input, option) =>
+        option.filterBy.toLowerCase().indexOf(input.toLowerCase()) >= 0,
+    };
+
     if (this.props.datasourceType === 'druid') {
       aggregateSelectProps.options = aggregateSelectProps.options.filter(
         aggregate => aggregate !== 'AVG',
       );
     }
 
-    const stateIsValid = adhocMetric.isValid();
-    const hasUnsavedChanges = !adhocMetric.equals(propsAdhocMetric);
+    const stateIsValid = adhocMetric.isValid() || savedMetric?.metric_name;
+    const hasUnsavedChanges =
+      !adhocMetric.equals(propsAdhocMetric) ||
+      (!(
+        typeof savedMetric?.metric_name === 'undefined' &&
+        typeof propsSavedMetric?.metric_name === 'undefined'
+      ) &&
+        savedMetric?.metric_name !== propsSavedMetric?.metric_name);
+
     return (
       <div
         id="metrics-edit-popover"
@@ -237,19 +295,20 @@ export default class AdhocMetricEditPopover extends React.Component {
         <Tabs
           id="adhoc-metric-edit-tabs"
           data-test="adhoc-metric-edit-tabs"
-          defaultActiveKey={adhocMetric.expressionType}
+          defaultActiveKey={
+            propsSavedMetric.metric_name
+              ? SAVED_TAB_KEY
+              : adhocMetric.expressionType
+          }
           className="adhoc-metric-edit-tabs"
           style={{ height: this.state.height, width: this.state.width }}
           onChange={this.refreshAceEditor}
+          allowOverflow
         >
-          <Tabs.TabPane
-            className="adhoc-metric-edit-tab"
-            key={EXPRESSION_TYPES.SIMPLE}
-            tab="Simple"
-          >
+          <Tabs.TabPane key={EXPRESSION_TYPES.SIMPLE} tab={t('Simple')}>
             <FormGroup>
               <FormLabel>
-                <strong>column</strong>
+                <strong>{t('column')}</strong>
               </FormLabel>
               <Select name="select-column" {...columnSelectProps}>
                 {columns.map(column => (
@@ -265,7 +324,7 @@ export default class AdhocMetricEditPopover extends React.Component {
             </FormGroup>
             <FormGroup>
               <FormLabel>
-                <strong>aggregate</strong>
+                <strong>{t('aggregate')}</strong>
               </FormLabel>
               <Select name="select-aggregate" {...aggregateSelectProps}>
                 {AGGREGATES_OPTIONS.map(option => (
@@ -276,10 +335,30 @@ export default class AdhocMetricEditPopover extends React.Component {
               </Select>
             </FormGroup>
           </Tabs.TabPane>
+          <Tabs.TabPane key={SAVED_TAB_KEY} tab={t('Saved')}>
+            <FormGroup>
+              <FormLabel>
+                <strong>{t('Saved metric')}</strong>
+              </FormLabel>
+              <Select name="select-saved" {...savedSelectProps}>
+                {Array.isArray(savedMetrics) &&
+                  savedMetrics.map(savedMetric => (
+                    <Select.Option
+                      value={savedMetric.id}
+                      filterBy={
+                        savedMetric.verbose_name || savedMetric.metric_name
+                      }
+                      key={savedMetric.id}
+                    >
+                      {this.renderColumnOption(savedMetric)}
+                    </Select.Option>
+                  ))}
+              </Select>
+            </FormGroup>
+          </Tabs.TabPane>
           <Tabs.TabPane
-            className="adhoc-metric-edit-tab"
             key={EXPRESSION_TYPES.SQL}
-            tab="Custom SQL"
+            tab={t('Custom SQL')}
             data-test="adhoc-metric-edit-tab#custom"
           >
             {this.props.datasourceType !== 'druid' ? (
@@ -315,7 +394,7 @@ export default class AdhocMetricEditPopover extends React.Component {
             data-test="AdhocMetricEdit#cancel"
             cta
           >
-            Close
+            {t('Close')}
           </Button>
           <Button
             disabled={!stateIsValid}
@@ -327,7 +406,7 @@ export default class AdhocMetricEditPopover extends React.Component {
             onClick={this.onSave}
             cta
           >
-            Save
+            {t('Save')}
           </Button>
           <ResizeIcon
             role="button"