Merge pull request #5516 from metabase/fix-5015

Limit available actions in "Summarize this segment" action widget action

frontend/src/metabase/components/AccordianList.jsx

@@ -175,7 +175,7 @@ export default class AccordianList extends Component {
             searchable && (typeof searchable !== "function" || searchable(sections[sectionIndex]));
 
         return (
-            <div id={id} className={this.props.className} style={{ width: '300px', ...style }}>
+            <div id={id} className={this.props.className} style={{ minWidth: '300px', ...style }}>
                 {sections.map((section, sectionIndex) =>
                     <section key={sectionIndex} className={cx("List-section", section.className, { "List-section--open": sectionIsOpen(sectionIndex) })}>
                         { section.name && alwaysExpanded ?

frontend/src/metabase/qb/components/actions/SummarizeBySegmentMetricAction.jsx

@@ -12,6 +12,16 @@ import type {
 } from "metabase/meta/types/Visualization";
 import type { TableMetadata } from "metabase/meta/types/Metadata";
 
+const omittedAggregations = ["rows", "cum_sum", "cum_count", "stddev"];
+const getAggregationOptionsForSummarize = query => {
+    return query
+        .table()
+        .aggregations()
+        .filter(
+            aggregation => !omittedAggregations.includes(aggregation.short)
+        );
+};
+
 export default ({ question }: ClickActionProps): ClickAction[] => {
     const query = question.query();
     if (!(query instanceof StructuredQuery)) {
@@ -33,7 +43,9 @@ export default ({ question }: ClickActionProps): ClickAction[] => {
                     query={query}
                     tableMetadata={tableMetadata}
                     customFields={query.expressions()}
-                    availableAggregations={query.table().aggregation_options}
+                    availableAggregations={getAggregationOptionsForSummarize(
+                        query
+                    )}
                     onCommitAggregation={aggregation => {
                         onChangeCardAndRun({
                             nextCard: question.summarize(aggregation).card()
@@ -41,6 +53,7 @@ export default ({ question }: ClickActionProps): ClickAction[] => {
                         onClose && onClose();
                     }}
                     onClose={onClose}
+                    showOnlyProvidedAggregations
                 />
             )
         }

frontend/src/metabase/qb/components/actions/SummarizeBySegmentMetricAction.unit.spec.js

@@ -16,6 +16,31 @@ const question = Question.create({
 });
 
 describe("SummarizeBySegmentMetricAction", () => {
+    describe("aggregation options", () => {
+        it("should show only a subset of all query aggregations", () => {
+            const hasAggregationOption = (popover, optionName) =>
+                popover.find(
+                    `.List-item-title[children="${optionName}"]`
+                ).length === 1;
+
+            const action = SummarizeBySegmentMetricAction({ question })[0];
+            const popover = mount(
+                action.popover({
+                    onClose: () => {},
+                    onChangeCardAndRun: () => {}
+                })
+            );
+
+            expect(hasAggregationOption(popover, "Count of rows")).toBe(true);
+            expect(hasAggregationOption(popover, "Average of ...")).toBe(true);
+            expect(hasAggregationOption(popover, "Raw data")).toBe(false);
+            expect(
+                hasAggregationOption(popover, "Cumulative count of rows")
+            ).toBe(false);
+            expect(popover.find(".List-section-title").length).toBe(0);
+        });
+    });
+
     describe("onChangeCardAndRun", async () => {
         it("should be called for 'Count of rows' choice", async () => {
             const action = SummarizeBySegmentMetricAction({ question })[0];

frontend/src/metabase/qb/components/gui/AggregationPopover.jsx

@@ -12,7 +12,9 @@ type Props = {
     tableMetadata: TableMetadata,
     customFields: { [key: ExpressionName]: any },
     onCommitAggregation: (aggregation: Aggregation) => void,
-    onClose?: () => void
+    onClose?: () => void,
+    availableAggregations: [Aggregation],
+    showOnlyProvidedAggregations: boolean
 };
 
 const AggregationPopover = (props: Props) => (

frontend/src/metabase/query_builder/components/ActionsWidget.jsx

@@ -23,8 +23,8 @@ type Props = {
 };
 
 type State = {
-    isVisible: boolean,
-    isOpen: boolean,
+    iconIsVisible: boolean,
+    popoverIsOpen: boolean,
     isClosing: boolean,
     selectedActionIndex: ?number,
 };
@@ -36,8 +36,8 @@ const POPOVER_WIDTH = 350;
 export default class ActionsWidget extends Component {
     props: Props;
     state: State = {
-        isVisible: false,
-        isOpen: false,
+        iconIsVisible: false,
+        popoverIsOpen: false,
         isClosing: false,
         selectedActionIndex: null
     };
@@ -51,23 +51,26 @@ export default class ActionsWidget extends Component {
     }
 
     handleMouseMoved = () => {
-        if (!this.state.isVisible) {
-            this.setState({ isVisible: true });
+        // Don't auto-show or auto-hide the icon if popover is open
+        if (this.state.popoverIsOpen) return;
+
+        if (!this.state.iconIsVisible) {
+            this.setState({ iconIsVisible: true });
         }
         this.handleMouseStoppedMoving();
     };
 
     handleMouseStoppedMoving = _.debounce(
         () => {
-            if (this.state.isVisible) {
-                this.setState({ isVisible: false });
+            if (this.state.iconIsVisible) {
+                this.setState({ iconIsVisible: false });
             }
         },
         1000
     );
 
     close = () => {
-        this.setState({ isClosing: true, isOpen: false, selectedActionIndex: null });
+        this.setState({ isClosing: true, popoverIsOpen: false, selectedActionIndex: null });
         // Needed because when closing the action widget by clicking compass, this is triggered first
         // on mousedown (by OnClickOutsideWrapper) and toggle is triggered on mouseup
         setTimeout(() => this.setState({ isClosing: false }), 500);
@@ -76,11 +79,11 @@ export default class ActionsWidget extends Component {
     toggle = () => {
         if (this.state.isClosing) return;
 
-        if (!this.state.isOpen) {
+        if (!this.state.popoverIsOpen) {
             MetabaseAnalytics.trackEvent("Actions", "Opened Action Menu");
         }
         this.setState({
-            isOpen: !this.state.isOpen,
+            popoverIsOpen: !this.state.popoverIsOpen,
             selectedActionIndex: null
         });
     };
@@ -112,7 +115,7 @@ export default class ActionsWidget extends Component {
     };
     render() {
         const { className, question } = this.props;
-        const { isOpen, isVisible, selectedActionIndex } = this.state;
+        const { popoverIsOpen, iconIsVisible, selectedActionIndex } = this.state;
 
         const mode = question.mode();
         const actions = mode ? mode.actions() : [];
@@ -132,7 +135,7 @@ export default class ActionsWidget extends Component {
                         width: CIRCLE_SIZE,
                         height: CIRCLE_SIZE,
                         transition: "opacity 300ms ease-in-out",
-                        opacity: isOpen || isVisible ? 1 : 0,
+                        opacity: popoverIsOpen || iconIsVisible ? 1 : 0,
                         boxShadow: "2px 2px 4px rgba(0, 0, 0, 0.2)"
                     }}
                     onClick={this.toggle}
@@ -142,14 +145,14 @@ export default class ActionsWidget extends Component {
                         className="text-white"
                         style={{
                             transition: "transform 500ms ease-in-out",
-                            transform: isOpen
+                            transform: popoverIsOpen
                                 ? "rotate(0deg)"
                                 : "rotate(720deg)"
                         }}
                         size={NEEDLE_SIZE}
                     />
                 </div>
-                {isOpen &&
+                {popoverIsOpen &&
                     <OnClickOutsideWrapper handleDismissal={() => {
                         MetabaseAnalytics.trackEvent("Actions", "Dismissed Action Menu");
                         this.close();

frontend/src/metabase/query_builder/components/AggregationPopover.jsx

@@ -40,6 +40,8 @@ export default class AggregationPopover extends Component {
         datasetQuery: PropTypes.object,
         customFields: PropTypes.object,
         availableAggregations: PropTypes.array,
+        // Restricts the shown options to contents of `availableActions` only
+        showOnlyProvidedAggregations: PropTypes.boolean
     };
 
 
@@ -118,7 +120,7 @@ export default class AggregationPopover extends Component {
     }
 
     render() {
-        const { query, tableMetadata } = this.props;
+        const { query, tableMetadata, showOnlyProvidedAggregations } = this.props;
 
         const customFields = this.getCustomFields();
         const availableAggregations = this.getAvailableAggregations();
@@ -134,42 +136,45 @@ export default class AggregationPopover extends Component {
         }
 
         let sections = [];
+        let customExpressionIndex = null;
 
         if (availableAggregations.length > 0) {
             sections.push({
-                name: "Metabasics",
+                name: showOnlyProvidedAggregations ? null : "Metabasics",
                 items: availableAggregations.map(aggregation => ({
                     name: aggregation.name,
                     value: [aggregation.short].concat(aggregation.fields.map(field => null)),
                     isSelected: (agg) => !AggregationClause.isCustom(agg) && AggregationClause.getAggregation(agg) === aggregation.short,
                     aggregation: aggregation
                 })),
-                icon: "table2"
+                icon: showOnlyProvidedAggregations ? null : "table2"
             });
         }
 
-        // we only want to consider active metrics, with the ONE exception that if the currently selected aggregation is a
-        // retired metric then we include it in the list to maintain continuity
-        let metrics = tableMetadata.metrics && tableMetadata.metrics.filter((mtrc) => mtrc.is_active === true || (selectedAggregation && selectedAggregation.id === mtrc.id));
-        if (metrics && metrics.length > 0) {
-            sections.push({
-                name: METRICS_SECTION_NAME,
-                items: metrics.map(metric => ({
-                    name: metric.name,
-                    value: ["METRIC", metric.id],
-                    isSelected: (aggregation) => AggregationClause.getMetric(aggregation) === metric.id,
-                    metric: metric
-                })),
-                icon: "staroutline"
-            });
-        }
+        if (!showOnlyProvidedAggregations) {
+            // we only want to consider active metrics, with the ONE exception that if the currently selected aggregation is a
+            // retired metric then we include it in the list to maintain continuity
+            let metrics = tableMetadata.metrics && tableMetadata.metrics.filter((mtrc) => mtrc.is_active === true || (selectedAggregation && selectedAggregation.id === mtrc.id));
+            if (metrics && metrics.length > 0) {
+                sections.push({
+                    name: METRICS_SECTION_NAME,
+                    items: metrics.map(metric => ({
+                        name: metric.name,
+                        value: ["METRIC", metric.id],
+                        isSelected: (aggregation) => AggregationClause.getMetric(aggregation) === metric.id,
+                        metric: metric
+                    })),
+                    icon: "staroutline"
+                });
+            }
 
-        let customExpressionIndex = sections.length;
-        if (tableMetadata.db.features.indexOf("expression-aggregations") >= 0) {
-            sections.push({
-                name: CUSTOM_SECTION_NAME,
-                icon: "staroutline"
-            });
+            customExpressionIndex = sections.length;
+            if (tableMetadata.db.features.indexOf("expression-aggregations") >= 0) {
+                sections.push({
+                    name: CUSTOM_SECTION_NAME,
+                    icon: "staroutline"
+                });
+            }
         }
 
         if (sections.length === 1) {
@@ -204,7 +209,7 @@ export default class AggregationPopover extends Component {
                                 this.state.error.map(error =>
                                     <div className="text-error mb1" style={{ whiteSpace: "pre-wrap" }}>{error.message}</div>
                                 )
-                            :
+                                :
                                 <div className="text-error mb1">{this.state.error.message}</div>
                         )}
                         <input
@@ -226,7 +231,7 @@ export default class AggregationPopover extends Component {
         } else if (choosingField) {
             const [agg, fieldId] = aggregation;
             return (
-                <div style={{width: 300}}>
+                <div style={{minWidth: 300}}>
                     <div className="text-grey-3 p1 py2 border-bottom flex align-center">
                         <a className="cursor-pointer flex align-center" onClick={this.onClearAggregation}>
                             <Icon name="chevronleft" size={18}/>