Skip to content

Commit

Permalink
fix: Incorrect dependency between filters related feature flags (#24608)
Browse files Browse the repository at this point in the history
(cherry picked from commit 781a204)
  • Loading branch information
michael-s-molina committed Jul 7, 2023
1 parent 967ca04 commit 26909bf
Show file tree
Hide file tree
Showing 8 changed files with 73 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -525,9 +525,11 @@ const DashboardBuilder: FC<DashboardBuilderProps> = () => {
threshold: [1],
});

const filterSetEnabled = isFeatureEnabled(
FeatureFlag.DASHBOARD_NATIVE_FILTERS_SET,
);
// Filter sets depend on native filters
const filterSetEnabled =
isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS_SET) &&
isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS);

const showFilterBar =
(crossFiltersEnabled || nativeFiltersEnabled) && !editMode;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,12 @@ const getModalTestId = testWithId<string>(FILTERS_CONFIG_MODAL_TEST_ID, true);
const FILTER_NAME = 'Time filter 1';
const FILTER_SET_NAME = 'New filter set';

// @ts-ignore
global.featureFlags = {
[FeatureFlag.DASHBOARD_NATIVE_FILTERS]: true,
[FeatureFlag.DASHBOARD_NATIVE_FILTERS_SET]: true,
};

const addFilterFlow = async () => {
// open filter config modal
userEvent.click(screen.getByTestId(getTestId('collapsable')));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,20 +212,23 @@ const FilterControls: FC<FilterControlsProps> = ({
selectedCrossFilters.at(-1),
),
}));
const nativeFiltersInScope = filtersInScope.map((filter, index) => ({
id: filter.id,
element: (
<div
className="filter-item-wrapper"
css={css`
flex-shrink: 0;
`}
>
{renderer(filter, index)}
</div>
),
}));
return [...crossFilters, ...nativeFiltersInScope];
if (isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS)) {
const nativeFiltersInScope = filtersInScope.map((filter, index) => ({
id: filter.id,
element: (
<div
className="filter-item-wrapper"
css={css`
flex-shrink: 0;
`}
>
{renderer(filter, index)}
</div>
),
}));
return [...crossFilters, ...nativeFiltersInScope];
}
return [...crossFilters];
}, [filtersInScope, renderer, rendererCrossFilter, selectedCrossFilters]);

const renderHorizontalContent = () => (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,14 @@
* under the License.
*/
/* eslint-disable no-param-reassign */
import { css, styled, t, useTheme } from '@superset-ui/core';
import {
FeatureFlag,
css,
isFeatureEnabled,
styled,
t,
useTheme,
} from '@superset-ui/core';
import React, { FC, useMemo } from 'react';
import Icons from 'src/components/Icons';
import Button from 'src/components/Button';
Expand Down Expand Up @@ -117,7 +124,7 @@ const Header: FC<HeaderProps> = ({ toggleFiltersBar }) => {
<Icons.Expand iconColor={theme.colors.grayscale.base} />
</HeaderButton>
</TitleArea>
{canEdit && (
{canEdit && isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS) && (
<AddFiltersButtonContainer>
<FilterConfigurationLink
dashboardId={dashboardId}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/

import React from 'react';
import React, { useMemo } from 'react';
import {
DataMaskStateWithId,
FeatureFlag,
Expand Down Expand Up @@ -129,6 +129,12 @@ const HorizontalFilterBar: React.FC<HorizontalBarProps> = ({
: [];
const hasFilters = filterValues.length > 0 || selectedCrossFilters.length > 0;

const actionsElement = useMemo(
() =>
isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS) ? actions : null,
[actions],
);

return (
<HorizontalBar {...getFilterBarTestId()}>
<HorizontalBarContent>
Expand All @@ -137,7 +143,7 @@ const HorizontalFilterBar: React.FC<HorizontalBarProps> = ({
) : (
<>
<FilterBarSettings />
{canEdit && (
{canEdit && isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS) && (
<FiltersLinkContainer hasFilters={hasFilters}>
<FilterConfigurationLink
dashboardId={dashboardId}
Expand All @@ -158,7 +164,7 @@ const HorizontalFilterBar: React.FC<HorizontalBarProps> = ({
onFilterSelectionChange={onSelectionChange}
/>
)}
{actions}
{actionsElement}
</>
)}
</HorizontalBarContent>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/

import { NativeFilterType } from '@superset-ui/core';
import { FeatureFlag, NativeFilterType } from '@superset-ui/core';
import React from 'react';
import { render, screen, waitFor } from 'spec/helpers/testing-library';
import HorizontalBar from './Horizontal';
Expand All @@ -32,6 +32,11 @@ const defaultProps = {
onSelectionChange: jest.fn(),
};

// @ts-ignore
global.featureFlags = {
[FeatureFlag.DASHBOARD_NATIVE_FILTERS]: true,
};

const renderWrapper = (overrideProps?: Record<string, any>) =>
waitFor(() =>
render(<HorizontalBar {...defaultProps} {...overrideProps} />, {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,17 @@ const VerticalFilterBar: React.FC<VerticalBarProps> = ({
[],
);

const actionsElement = useMemo(
() =>
isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS) ? actions : null,
[actions],
);

// Filter sets depend on native filters
const filterSetEnabled =
isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS_SET) &&
isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS);

return (
<FilterBarScrollContext.Provider value={isScrolling}>
<BarWrapper
Expand Down Expand Up @@ -315,7 +326,7 @@ const VerticalFilterBar: React.FC<VerticalBarProps> = ({
<div css={{ height }}>
<Loading />
</div>
) : isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS_SET) ? (
) : filterSetEnabled ? (
<>
{crossFilters}
{filterSetsTabs}
Expand All @@ -324,11 +335,12 @@ const VerticalFilterBar: React.FC<VerticalBarProps> = ({
<div css={tabPaneStyle} onScroll={onScroll}>
<>
{crossFilters}
{filterControls}
{isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS) &&
filterControls}
</>
</div>
)}
{actions}
{actionsElement}
</Bar>
</BarWrapper>
</FilterBarScrollContext.Provider>
Expand Down
7 changes: 6 additions & 1 deletion superset-frontend/src/dashboard/containers/DashboardPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,11 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
const readyToRender = Boolean(dashboard && charts);
const { dashboard_title, css, metadata, id = 0 } = dashboard || {};

// Filter sets depend on native filters
const filterSetEnabled =
isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS_SET) &&
isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS);

useEffect(() => {
// mark tab id as redundant when user closes browser tab - a new id will be
// generated next time user opens a dashboard and the old one won't be reused
Expand Down Expand Up @@ -216,7 +221,7 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
if (readyToRender) {
if (!isDashboardHydrated.current) {
isDashboardHydrated.current = true;
if (isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS_SET)) {
if (filterSetEnabled) {
// only initialize filterset once
dispatch(getFilterSets(id));
}
Expand Down

0 comments on commit 26909bf

Please sign in to comment.