From 9bfc3620763f2da503db7c586b5ff3fa02ec6b99 Mon Sep 17 00:00:00 2001
From: Dalton <daltojohnso@users.noreply.github.com>
Date: Tue, 24 May 2022 14:09:32 -0700
Subject: [PATCH] Add new parameter fields utils (#22764)
use new util and remove unused code
Split out WidgetStatusIcon
fix waiting on request that no longer happens
---
.../components/ParameterValueWidget.jsx | 192 +++---------------
.../WidgetStatusIcon/WidgetStatusIcon.tsx | 81 ++++++++
.../components/WidgetStatusIcon/index.ts | 1 +
.../src/metabase/parameters/utils/fields.js | 9 +
.../parameters/utils/fields.unit.spec.js | 53 ++++-
.../dashboard-filters/parameters.cy.spec.js | 4 -
6 files changed, 174 insertions(+), 166 deletions(-)
create mode 100644 frontend/src/metabase/parameters/components/WidgetStatusIcon/WidgetStatusIcon.tsx
create mode 100644 frontend/src/metabase/parameters/components/WidgetStatusIcon/index.ts
diff --git a/frontend/src/metabase/parameters/components/ParameterValueWidget.jsx b/frontend/src/metabase/parameters/components/ParameterValueWidget.jsx
index 75a91d478b2..79bf44708e4 100644
--- a/frontend/src/metabase/parameters/components/ParameterValueWidget.jsx
+++ b/frontend/src/metabase/parameters/components/ParameterValueWidget.jsx
@@ -1,8 +1,12 @@
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,
-};
diff --git a/frontend/src/metabase/parameters/components/WidgetStatusIcon/WidgetStatusIcon.tsx b/frontend/src/metabase/parameters/components/WidgetStatusIcon/WidgetStatusIcon.tsx
new file mode 100644
index 00000000000..89774fbd995
--- /dev/null
+++ b/frontend/src/metabase/parameters/components/WidgetStatusIcon/WidgetStatusIcon.tsx
@@ -0,0 +1,81 @@
+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;
diff --git a/frontend/src/metabase/parameters/components/WidgetStatusIcon/index.ts b/frontend/src/metabase/parameters/components/WidgetStatusIcon/index.ts
new file mode 100644
index 00000000000..7da8c2e5ec7
--- /dev/null
+++ b/frontend/src/metabase/parameters/components/WidgetStatusIcon/index.ts
@@ -0,0 +1 @@
+export { default } from "./WidgetStatusIcon";
diff --git a/frontend/src/metabase/parameters/utils/fields.js b/frontend/src/metabase/parameters/utils/fields.js
index 522c45cb1bc..7023953a112 100644
--- a/frontend/src/metabase/parameters/utils/fields.js
+++ b/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;
+}
diff --git a/frontend/src/metabase/parameters/utils/fields.unit.spec.js b/frontend/src/metabase/parameters/utils/fields.unit.spec.js
index 8f568dad887..3516ca87dd9 100644
--- a/frontend/src/metabase/parameters/utils/fields.unit.spec.js
+++ b/frontend/src/metabase/parameters/utils/fields.unit.spec.js
@@ -1,5 +1,10 @@
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);
+ });
+ });
});
diff --git a/frontend/test/metabase/scenarios/dashboard-filters/parameters.cy.spec.js b/frontend/test/metabase/scenarios/dashboard-filters/parameters.cy.spec.js
index b28efbaf4ae..c3b5c68a08c 100644
--- a/frontend/test/metabase/scenarios/dashboard-filters/parameters.cy.spec.js
+++ b/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
--
GitLab