Add new parameter fields utils (#22764)

use new util and remove unused code

Split out WidgetStatusIcon

fix waiting on request that no longer happens

frontend/src/metabase/parameters/components/ParameterValueWidget.jsx

 import React, { Component } from "react";
 import PropTypes from "prop-types";
-import { connect } from "react-redux";
 import { t } from "ttag";
+import cx from "classnames";
+import _ from "underscore";
 
+import { getParameterIconName } from "metabase/parameters/utils/ui";
+import { isDashboardParameterWithoutMapping } from "metabase/parameters/utils/dashboards";
+import { isOnlyMappedToFields } from "metabase/parameters/utils/fields";
 import PopoverWithTrigger from "metabase/components/PopoverWithTrigger";
 import Icon from "metabase/components/Icon";
 import DateSingleWidget from "metabase/components/DateSingleWidget";
@@ -11,22 +15,13 @@ import DateRelativeWidget from "metabase/components/DateRelativeWidget";
 import DateMonthYearWidget from "metabase/components/DateMonthYearWidget";
 import DateQuarterYearWidget from "metabase/components/DateQuarterYearWidget";
 import DateAllOptionsWidget from "metabase/components/DateAllOptionsWidget";
-import TextWidget from "metabase/components/TextWidget";
-import ParameterFieldWidget from "./widgets/ParameterFieldWidget/ParameterFieldWidget";
 import Tooltip from "metabase/components/Tooltip";
+import TextWidget from "metabase/components/TextWidget";
+import WidgetStatusIcon from "metabase/parameters/components/WidgetStatusIcon";
 
-import { fetchField, fetchFieldValues } from "metabase/redux/metadata";
-import { getMetadata } from "metabase/selectors/metadata";
-
-import { getParameterIconName } from "metabase/parameters/utils/ui";
-import { isDashboardParameterWithoutMapping } from "metabase/parameters/utils/dashboards";
-import { hasFieldValues, getFieldIds } from "metabase/parameters/utils/fields";
-
+import ParameterFieldWidget from "./widgets/ParameterFieldWidget/ParameterFieldWidget";
 import S from "./ParameterWidget.css";
 
-import cx from "classnames";
-import _ from "underscore";
-
 const DATE_WIDGETS = {
   "date/single": DateSingleWidget,
   "date/range": DateRangeWidget,
@@ -36,18 +31,6 @@ const DATE_WIDGETS = {
   "date/all-options": DateAllOptionsWidget,
 };
 
-const makeMapStateToProps = () => {
-  const mapStateToProps = (state, props) => ({
-    metadata: getMetadata(state),
-  });
-  return mapStateToProps;
-};
-
-const mapDispatchToProps = {
-  fetchFieldValues,
-  fetchField,
-};
-
 class ParameterValueWidget extends Component {
   static propTypes = {
     parameter: PropTypes.object.isRequired,
@@ -63,15 +46,6 @@ class ParameterValueWidget extends Component {
     className: PropTypes.string,
     parameters: PropTypes.array,
     dashboard: PropTypes.object,
-
-    metadata: PropTypes.object.isRequired,
-  };
-
-  static defaultProps = {
-    isEditing: false,
-    noReset: false,
-    commitImmediately: false,
-    className: "",
   };
 
   state = { isFocused: false };
@@ -79,40 +53,10 @@ class ParameterValueWidget extends Component {
   constructor(props) {
     super(props);
 
-    // In public dashboards we receive field values before mounting this component and
-    // without need to call `fetchFieldValues` separately
-    if (!hasFieldValues(this.props.parameter)) {
-      this.updateFieldValues(this.props);
-    }
-
     this.valuePopover = React.createRef();
     this.trigger = React.createRef();
   }
 
-  componentDidUpdate(prevProps) {
-    if (
-      !_.isEqual(
-        getFieldIds(prevProps.parameter),
-        getFieldIds(this.props.parameter),
-      )
-    ) {
-      this.updateFieldValues(this.props);
-    }
-  }
-
-  updateFieldValues({ dashboard, parameter, fetchField, fetchFieldValues }) {
-    // in a dashboard? the field values will be fetched via
-    // DashboardApi.parameterValues instead and thus, no need to
-    // manually update field values
-    const useChainFilter = dashboard && dashboard.id;
-    if (!useChainFilter) {
-      for (const id of getFieldIds(parameter)) {
-        fetchField(id);
-        fetchFieldValues(id);
-      }
-    }
-  }
-
   onFocusChanged = isFocused => {
     const { focusChanged: parentFocusChanged } = this.props;
     if (parentFocusChanged) {
@@ -133,7 +77,6 @@ class ParameterValueWidget extends Component {
 
   render() {
     const {
-      metadata,
       parameter,
       value,
       setValue,
@@ -151,8 +94,8 @@ class ParameterValueWidget extends Component {
       dashboard,
     );
     const isDashParamWithoutMappingText = t`This filter needs to be connected to a card.`;
-    const WidgetDefinition = getWidgetDefinition(metadata, parameter);
-    const { noPopover } = WidgetDefinition;
+    const { noPopover, format } = getWidgetDefinition(parameter);
+    const parameterTypeIcon = getParameterIconName(parameter);
     const showTypeIcon = !isEditing && !hasValue && !isFocused;
 
     if (noPopover) {
@@ -168,7 +111,13 @@ class ParameterValueWidget extends Component {
               [S.isEditing]: isEditing,
             })}
           >
-            {showTypeIcon && <ParameterTypeIcon parameter={parameter} />}
+            {showTypeIcon && (
+              <Icon
+                name={parameterTypeIcon}
+                className="flex-align-left mr1 flex-no-shrink"
+                size={14}
+              />
+            )}
             <Widget
               {...this.props}
               target={this.getTargetRef()}
@@ -207,9 +156,15 @@ class ParameterValueWidget extends Component {
                   "cursor-not-allowed": isDashParamWithoutMapping,
                 })}
               >
-                {showTypeIcon && <ParameterTypeIcon parameter={parameter} />}
+                {showTypeIcon && (
+                  <Icon
+                    name={parameterTypeIcon}
+                    className="flex-align-left mr1 flex-no-shrink"
+                    size={14}
+                  />
+                )}
                 <div className="mr1 text-nowrap">
-                  {hasValue ? WidgetDefinition.format(value) : placeholderText}
+                  {hasValue ? format(value) : placeholderText}
                 </div>
                 <WidgetStatusIcon
                   isFullscreen={isFullscreen}
@@ -239,14 +194,10 @@ class ParameterValueWidget extends Component {
   }
 }
 
-export default connect(
-  makeMapStateToProps,
-  mapDispatchToProps,
-)(ParameterValueWidget);
+export default ParameterValueWidget;
 
 function Widget({
   parameter,
-  metadata,
   value,
   setValue,
   onPopoverClose,
@@ -260,9 +211,6 @@ function Widget({
   disabled,
   target,
 }) {
-  const DateWidget = DATE_WIDGETS[parameter.type];
-  const fields = parameter.fields || [];
-
   if (disabled) {
     return (
       <TextWidget
@@ -274,11 +222,12 @@ function Widget({
     );
   }
 
+  const DateWidget = DATE_WIDGETS[parameter.type];
   if (DateWidget) {
     return (
       <DateWidget value={value} setValue={setValue} onClose={onPopoverClose} />
     );
-  } else if (fields.length > 0 && parameter.hasOnlyFieldTargets) {
+  } else if (isOnlyMappedToFields(parameter)) {
     return (
       <ParameterFieldWidget
         target={target}
@@ -287,7 +236,7 @@ function Widget({
         dashboard={dashboard}
         placeholder={placeholder}
         value={value}
-        fields={fields}
+        fields={parameter.fields}
         setValue={setValue}
         isEditing={isEditing}
         focusChanged={onFocusChanged}
@@ -314,91 +263,12 @@ Widget.propTypes = {
   onFocusChanged: PropTypes.func.isRequired,
 };
 
-function getWidgetDefinition(metadata, parameter) {
-  const fields = parameter.fields || [];
+function getWidgetDefinition(parameter) {
   if (DATE_WIDGETS[parameter.type]) {
     return DATE_WIDGETS[parameter.type];
-  } else if (fields.length > 0 && parameter.hasOnlyFieldTargets) {
+  } else if (isOnlyMappedToFields(parameter)) {
     return ParameterFieldWidget;
   } else {
     return TextWidget;
   }
 }
-
-function ParameterTypeIcon({ parameter }) {
-  return (
-    <Icon
-      name={getParameterIconName(parameter)}
-      className="flex-align-left mr1 flex-no-shrink"
-      size={14}
-    />
-  );
-}
-
-ParameterTypeIcon.propTypes = {
-  parameter: PropTypes.object.isRequired,
-};
-
-function WidgetStatusIcon({
-  isFullscreen,
-  hasValue,
-  noReset,
-  noPopover,
-  isFocused,
-  setValue,
-}) {
-  if (isFullscreen) {
-    return null;
-  }
-
-  if (hasValue && !noReset) {
-    return (
-      <Icon
-        name="close"
-        className="flex-align-right cursor-pointer flex-no-shrink"
-        size={12}
-        onClick={e => {
-          if (hasValue) {
-            e.stopPropagation();
-            setValue(null);
-          }
-        }}
-      />
-    );
-  } else if (noPopover && isFocused) {
-    return (
-      <Icon
-        name="enter_or_return"
-        className="flex-align-right flex-no-shrink"
-        size={12}
-      />
-    );
-  } else if (noPopover) {
-    return (
-      <Icon
-        name="empty"
-        className="flex-align-right cursor-pointer flex-no-shrink"
-        size={12}
-      />
-    );
-  } else if (!noPopover) {
-    return (
-      <Icon
-        name="chevrondown"
-        className="flex-align-right flex-no-shrink"
-        size={12}
-      />
-    );
-  }
-
-  return null;
-}
-
-WidgetStatusIcon.propTypes = {
-  isFullscreen: PropTypes.bool.isRequired,
-  hasValue: PropTypes.bool.isRequired,
-  noReset: PropTypes.bool.isRequired,
-  noPopover: PropTypes.bool.isRequired,
-  isFocused: PropTypes.bool.isRequired,
-  setValue: PropTypes.func.isRequired,
-};

frontend/src/metabase/parameters/components/WidgetStatusIcon/WidgetStatusIcon.tsx

+import React from "react";
+import PropTypes from "prop-types";
+
+import Icon from "metabase/components/Icon";
+
+const propTypes = {
+  isFullscreen: PropTypes.bool.isRequired,
+  hasValue: PropTypes.bool.isRequired,
+  noReset: PropTypes.bool.isRequired,
+  noPopover: PropTypes.bool.isRequired,
+  isFocused: PropTypes.bool.isRequired,
+  setValue: PropTypes.func.isRequired,
+};
+
+type Props = {
+  isFullscreen: boolean;
+  hasValue: boolean;
+  noReset: boolean;
+  noPopover: boolean;
+  isFocused: boolean;
+  setValue: (value: any) => void;
+};
+
+function WidgetStatusIcon({
+  isFullscreen,
+  hasValue,
+  noReset,
+  noPopover,
+  isFocused,
+  setValue,
+}: Props) {
+  if (isFullscreen) {
+    return null;
+  }
+
+  if (hasValue && !noReset) {
+    return (
+      <Icon
+        name="close"
+        className="flex-align-right cursor-pointer flex-no-shrink"
+        size={12}
+        onClick={e => {
+          if (hasValue) {
+            e.stopPropagation();
+            setValue(null);
+          }
+        }}
+      />
+    );
+  } else if (noPopover && isFocused) {
+    return (
+      <Icon
+        name="enter_or_return"
+        className="flex-align-right flex-no-shrink"
+        size={12}
+      />
+    );
+  } else if (noPopover) {
+    return (
+      <Icon
+        name="empty"
+        className="flex-align-right cursor-pointer flex-no-shrink"
+        size={12}
+      />
+    );
+  } else if (!noPopover) {
+    return (
+      <Icon
+        name="chevrondown"
+        className="flex-align-right flex-no-shrink"
+        size={12}
+      />
+    );
+  }
+
+  return null;
+}
+
+export default WidgetStatusIcon;
+
+WidgetStatusIcon.propTypes = propTypes;

frontend/src/metabase/parameters/components/WidgetStatusIcon/index.ts

+export { default } from "./WidgetStatusIcon";

frontend/src/metabase/parameters/utils/fields.js

@@ -11,3 +11,12 @@ export function getFieldIds(parameter) {
   const fieldIds = field_id ? [field_id] : field_ids;
   return fieldIds.filter(id => typeof id === "number");
 }
+
+export function hasFields(parameter) {
+  const { fields } = parameter;
+  return Array.isArray(fields) && fields.length > 0;
+}
+
+export function isOnlyMappedToFields(parameter) {
+  return hasFields(parameter) && parameter.hasOnlyFieldTargets;
+}

frontend/src/metabase/parameters/utils/fields.unit.spec.js

 import Field from "metabase-lib/lib/metadata/Field";
-import { hasFieldValues, getFieldIds } from "./fields";
+import {
+  hasFieldValues,
+  getFieldIds,
+  hasFields,
+  isOnlyMappedToFields,
+} from "./fields";
 
 describe("parameters/utils/fields", () => {
   describe("hasFieldValues", () => {
@@ -49,4 +54,50 @@ describe("parameters/utils/fields", () => {
       ).toEqual([1]);
     });
   });
+
+  describe("hasFields", () => {
+    it("should be false when the parameter has no fields", () => {
+      expect(hasFields({ fields: [] })).toBe(false);
+      expect(hasFields({})).toBe(false);
+    });
+
+    it("should be true when a field on the parameter has values", () => {
+      const mockField = new Field({ id: 1, name: "foo" });
+      expect(hasFields({ fields: [mockField] })).toBe(true);
+    });
+  });
+
+  describe("isOnlyMappedToFields", () => {
+    it("should be false when the parameter has no fields", () => {
+      expect(
+        isOnlyMappedToFields({ fields: [], hasOnlyFieldTargets: false }),
+      ).toBe(false);
+    });
+
+    it("should be false in a broken scenario where it has no fields but claims to only target fields", () => {
+      expect(
+        isOnlyMappedToFields({ fields: [], hasOnlyFieldTargets: true }),
+      ).toBe(false);
+    });
+
+    it("should be false when the parameter has fields but is mapped to more than fields", () => {
+      const mockField = new Field({ id: 1, name: "foo" });
+      expect(
+        isOnlyMappedToFields({
+          fields: [mockField],
+          hasOnlyFieldTargets: false,
+        }),
+      ).toBe(false);
+    });
+
+    it("should be true when the parameter has fields and is mapped only to fields", () => {
+      const mockField = new Field({ id: 1, name: "foo" });
+      expect(
+        isOnlyMappedToFields({
+          fields: [mockField],
+          hasOnlyFieldTargets: true,
+        }),
+      ).toBe(true);
+    });
+  });
 });

frontend/test/metabase/scenarios/dashboard-filters/parameters.cy.spec.js

@@ -19,7 +19,6 @@ describe("scenarios > dashboard > parameters", () => {
     cy.intercept("POST", "/api/card/**/query").as("cardQuery");
     cy.intercept("GET", "/api/dashboard/**").as("dashboard");
     cy.intercept("GET", "/api/collection/**").as("collection");
-    cy.intercept("GET", "/api/field/**/values").as("fieldValues");
   });
 
   it("should be visible if previously added", () => {
@@ -558,7 +557,6 @@ describe("scenarios > dashboard > parameters", () => {
     it("should not see mapping options", () => {
       cy.icon("pencil").click();
       cy.findByText("Location").click({ force: true });
-      cy.wait("@fieldValues");
 
       cy.icon("key");
     });
@@ -570,7 +568,6 @@ function selectFilter(selection, filterName) {
   popover()
     .contains(filterName)
     .click({ force: true });
-  cy.wait("@fieldValues");
 }
 
 function addQuestion(name) {
@@ -590,7 +587,6 @@ function addCityFilterWithDefault() {
   cy.findByText("Select…").click();
   popover().within(() => {
     cy.findByText("City").click();
-    cy.wait("@fieldValues");
   });
 
   // Create a default value and save filter