feat(dashboard): Support changing filter bar location (#22004)

superset-frontend/src/dashboard/actions/dashboardInfo.ts

@@ -17,9 +17,17 @@
  * under the License.
  */
 import { Dispatch } from 'redux';
-import { makeApi, CategoricalColorNamespace } from '@superset-ui/core';
+import { makeApi, CategoricalColorNamespace, t } from '@superset-ui/core';
 import { isString } from 'lodash';
-import { ChartConfiguration, DashboardInfo } from '../reducers/types';
+import { getClientErrorObject } from 'src/utils/getClientErrorObject';
+import { addDangerToast } from 'src/components/MessageToasts/actions';
+import {
+  DashboardInfo,
+  FilterBarLocation,
+  RootState,
+} from 'src/dashboard/types';
+import { ChartConfiguration } from 'src/dashboard/reducers/types';
+import { onSave } from './dashboardState';
 
 export const DASHBOARD_INFO_UPDATED = 'DASHBOARD_INFO_UPDATED';
 
@@ -111,3 +119,59 @@ export const setChartConfiguration =
       dispatch({ type: SET_CHART_CONFIG_FAIL, chartConfiguration });
     }
   };
+
+export const SET_FILTER_BAR_LOCATION = 'SET_FILTER_BAR_LOCATION';
+export interface SetFilterBarLocation {
+  type: typeof SET_FILTER_BAR_LOCATION;
+  filterBarLocation: FilterBarLocation;
+}
+export function setFilterBarLocation(filterBarLocation: FilterBarLocation) {
+  return { type: SET_FILTER_BAR_LOCATION, filterBarLocation };
+}
+
+export function saveFilterBarLocation(location: FilterBarLocation) {
+  return async (dispatch: Dispatch, getState: () => RootState) => {
+    const { id, metadata } = getState().dashboardInfo;
+    const updateDashboard = makeApi<
+      Partial<DashboardInfo>,
+      { result: Partial<DashboardInfo>; last_modified_time: number }
+    >({
+      method: 'PUT',
+      endpoint: `/api/v1/dashboard/${id}`,
+    });
+    try {
+      const response = await updateDashboard({
+        json_metadata: JSON.stringify({
+          ...metadata,
+          filter_bar_location: location,
+        }),
+      });
+      const updatedDashboard = response.result;
+      const lastModifiedTime = response.last_modified_time;
+      if (updatedDashboard.json_metadata) {
+        const metadata = JSON.parse(updatedDashboard.json_metadata);
+        if (metadata.filter_bar_location) {
+          dispatch(setFilterBarLocation(metadata.filter_bar_location));
+        }
+      }
+      if (lastModifiedTime) {
+        dispatch(onSave(lastModifiedTime));
+      }
+    } catch (errorObject) {
+      const { error, message } = await getClientErrorObject(errorObject);
+      let errorText = t('Sorry, an unknown error occurred.');
+
+      if (error) {
+        errorText = t(
+          'Sorry, there was an error saving this dashboard: %s',
+          error,
+        );
+      }
+      if (typeof message === 'string' && message === 'Forbidden') {
+        errorText = t('You do not have permission to edit this dashboard');
+      }
+      dispatch(addDangerToast(errorText));
+      throw errorObject;
+    }
+  };
+}

superset-frontend/src/dashboard/actions/dashboardState.js

@@ -284,7 +284,7 @@ export function saveDashboardRequest(data, id, saveType) {
     const onUpdateSuccess = response => {
       const updatedDashboard = response.json.result;
       const lastModifiedTime = response.json.last_modified_time;
-      // synching with the backend transformations of the metadata
+      // syncing with the backend transformations of the metadata
       if (updatedDashboard.json_metadata) {
         const metadata = JSON.parse(updatedDashboard.json_metadata);
         dispatch(

superset-frontend/src/dashboard/actions/hydrate.js

@@ -57,6 +57,7 @@ import getNativeFilterConfig from '../util/filterboxMigrationHelper';
 import { updateColorSchema } from './dashboardInfo';
 import { getChartIdsInFilterScope } from '../util/getChartIdsInFilterScope';
 import updateComponentParentsList from '../util/updateComponentParentsList';
+import { FilterBarLocation } from '../types';
 
 export const HYDRATE_DASHBOARD = 'HYDRATE_DASHBOARD';
 
@@ -428,6 +429,8 @@ export const hydrateDashboard =
             flash_messages: common?.flash_messages,
             conf: common?.conf,
           },
+          filterBarLocation:
+            metadata.filter_bar_location ?? FilterBarLocation.VERTICAL,
         },
         dataMask,
         dashboardFilters,

superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarLocationSelect/FilterBarLocationSelect.test.tsx

@@ -0,0 +1,175 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import fetchMock from 'fetch-mock';
+import { waitFor } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
+import { render, screen, within } from 'spec/helpers/testing-library';
+import { DashboardInfo, FilterBarLocation } from 'src/dashboard/types';
+import * as mockedMessageActions from 'src/components/MessageToasts/actions';
+import { FilterBarLocationSelect } from './index';
+
+const initialState: { dashboardInfo: DashboardInfo } = {
+  dashboardInfo: {
+    id: 1,
+    userId: '1',
+    metadata: {
+      native_filter_configuration: {},
+      show_native_filters: true,
+      chart_configuration: {},
+      color_scheme: '',
+      color_namespace: '',
+      color_scheme_domain: [],
+      label_colors: {},
+      shared_label_colors: {},
+    },
+    json_metadata: '',
+    dash_edit_perm: true,
+    filterBarLocation: FilterBarLocation.VERTICAL,
+    common: {
+      conf: {},
+      flash_messages: [],
+    },
+  },
+};
+
+const setup = (dashboardInfoOverride: Partial<DashboardInfo> = {}) =>
+  render(<FilterBarLocationSelect />, {
+    useRedux: true,
+    initialState: {
+      ...initialState,
+      dashboardInfo: {
+        ...initialState.dashboardInfo,
+        ...dashboardInfoOverride,
+      },
+    },
+  });
+
+test('Dropdown trigger renders', () => {
+  setup();
+  expect(screen.getByLabelText('gear')).toBeVisible();
+});
+
+test('Popover opens with "Vertical" selected', async () => {
+  setup();
+  userEvent.click(screen.getByLabelText('gear'));
+  expect(await screen.findByText('Vertical (Left)')).toBeInTheDocument();
+  expect(screen.getByText('Horizontal (Top)')).toBeInTheDocument();
+  expect(
+    within(screen.getAllByRole('menuitem')[0]).getByLabelText('check'),
+  ).toBeInTheDocument();
+});
+
+test('Popover opens with "Horizontal" selected', async () => {
+  setup({ filterBarLocation: FilterBarLocation.HORIZONTAL });
+  userEvent.click(screen.getByLabelText('gear'));
+  expect(await screen.findByText('Vertical (Left)')).toBeInTheDocument();
+  expect(screen.getByText('Horizontal (Top)')).toBeInTheDocument();
+  expect(
+    within(screen.getAllByRole('menuitem')[1]).getByLabelText('check'),
+  ).toBeInTheDocument();
+});
+
+test('On selection change, send request and update checked value', async () => {
+  fetchMock.reset();
+  fetchMock.put('glob:*/api/v1/dashboard/1', {
+    result: {
+      json_metadata: JSON.stringify({
+        ...initialState.dashboardInfo.metadata,
+        filter_bar_location: 'HORIZONTAL',
+      }),
+    },
+  });
+
+  setup();
+  userEvent.click(screen.getByLabelText('gear'));
+
+  expect(
+    within(screen.getAllByRole('menuitem')[0]).getByLabelText('check'),
+  ).toBeInTheDocument();
+  expect(
+    within(screen.getAllByRole('menuitem')[1]).queryByLabelText('check'),
+  ).not.toBeInTheDocument();
+
+  userEvent.click(await screen.findByText('Horizontal (Top)'));
+
+  // 1st check - checkmark appears immediately after click
+  expect(
+    await within(screen.getAllByRole('menuitem')[1]).findByLabelText('check'),
+  ).toBeInTheDocument();
+  expect(
+    within(screen.getAllByRole('menuitem')[0]).queryByLabelText('check'),
+  ).not.toBeInTheDocument();
+
+  // successful query
+  await waitFor(() =>
+    expect(fetchMock.lastCall()?.[1]?.body).toEqual(
+      JSON.stringify({
+        json_metadata: JSON.stringify({
+          ...initialState.dashboardInfo.metadata,
+          filter_bar_location: 'HORIZONTAL',
+        }),
+      }),
+    ),
+  );
+
+  // 2nd check - checkmark stays after successful query
+  expect(
+    await within(screen.getAllByRole('menuitem')[1]).findByLabelText('check'),
+  ).toBeInTheDocument();
+  expect(
+    within(screen.getAllByRole('menuitem')[0]).queryByLabelText('check'),
+  ).not.toBeInTheDocument();
+
+  fetchMock.reset();
+});
+
+test('On failed request, restore previous selection', async () => {
+  fetchMock.reset();
+  fetchMock.put('glob:*/api/v1/dashboard/1', 400);
+
+  const dangerToastSpy = jest.spyOn(mockedMessageActions, 'addDangerToast');
+
+  setup();
+  userEvent.click(screen.getByLabelText('gear'));
+
+  expect(
+    within(screen.getAllByRole('menuitem')[0]).getByLabelText('check'),
+  ).toBeInTheDocument();
+  expect(
+    within(screen.getAllByRole('menuitem')[1]).queryByLabelText('check'),
+  ).not.toBeInTheDocument();
+
+  userEvent.click(await screen.findByText('Horizontal (Top)'));
+
+  // checkmark gets rolled back to the original selection after successful query
+  expect(
+    await within(screen.getAllByRole('menuitem')[0]).findByLabelText('check'),
+  ).toBeInTheDocument();
+  expect(
+    within(screen.getAllByRole('menuitem')[1]).queryByLabelText('check'),
+  ).not.toBeInTheDocument();
+
+  expect(dangerToastSpy).toHaveBeenCalledWith(
+    'Sorry, there was an error saving this dashboard: Unknown Error',
+  );
+
+  fetchMock.reset();
+});

superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarLocationSelect/index.tsx

@@ -0,0 +1,80 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { useCallback, useState } from 'react';
+import { useDispatch, useSelector } from 'react-redux';
+import { t, useTheme } from '@superset-ui/core';
+import { MenuProps } from 'src/components/Menu';
+import { FilterBarLocation, RootState } from 'src/dashboard/types';
+import { saveFilterBarLocation } from 'src/dashboard/actions/dashboardInfo';
+import Icons from 'src/components/Icons';
+import DropdownSelectableIcon from 'src/components/DropdownSelectableIcon';
+
+export const FilterBarLocationSelect = () => {
+  const dispatch = useDispatch();
+  const theme = useTheme();
+  const filterBarLocation = useSelector<RootState, FilterBarLocation>(
+    ({ dashboardInfo }) => dashboardInfo.filterBarLocation,
+  );
+  const [selectedFilterBarLocation, setSelectedFilterBarLocation] =
+    useState(filterBarLocation);
+
+  const toggleFilterBarLocation = useCallback(
+    async (
+      selection: Parameters<
+        Required<Pick<MenuProps, 'onSelect'>>['onSelect']
+      >[0],
+    ) => {
+      const selectedKey = selection.key as FilterBarLocation;
+      if (selectedKey !== filterBarLocation) {
+        // set displayed selection in local state for immediate visual response after clicking
+        setSelectedFilterBarLocation(selectedKey);
+        try {
+          // save selection in Redux and backend
+          await dispatch(
+            saveFilterBarLocation(selection.key as FilterBarLocation),
+          );
+        } catch {
+          // revert local state in case of error when saving
+          setSelectedFilterBarLocation(filterBarLocation);
+        }
+      }
+    },
+    [dispatch, filterBarLocation],
+  );
+
+  return (
+    <DropdownSelectableIcon
+      onSelect={toggleFilterBarLocation}
+      info={t('Placement of filter bar')}
+      icon={<Icons.Gear name="gear" iconColor={theme.colors.grayscale.base} />}
+      menuItems={[
+        {
+          key: FilterBarLocation.VERTICAL,
+          label: t('Vertical (Left)'),
+        },
+        {
+          key: FilterBarLocation.HORIZONTAL,
+          label: t('Horizontal (Top)'),
+        },
+      ]}
+      selectedKeys={[selectedFilterBarLocation]}
+    />
+  );
+};

superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Header/index.tsx

@@ -32,8 +32,8 @@ import { useSelector } from 'react-redux';
 import FilterConfigurationLink from 'src/dashboard/components/nativeFilters/FilterBar/FilterConfigurationLink';
 import { useFilters } from 'src/dashboard/components/nativeFilters/FilterBar/state';
 import { RootState } from 'src/dashboard/types';
-import DropdownSelectableIcon from 'src/components/DropdownSelectableIcon';
 import { getFilterBarTestId } from '..';
+import { FilterBarLocationSelect } from '../FilterBarLocationSelect';
 
 const TitleArea = styled.h4`
   display: flex;
@@ -100,26 +100,7 @@ const Header: FC<HeaderProps> = ({ toggleFiltersBar }) => {
     <Wrapper>
       <TitleArea>
         <span>{t('Filters')}</span>
-        {canSetHorizontalFilterBar && (
-          <DropdownSelectableIcon
-            onSelect={item => console.log('Selected item', item)}
-            info={t('Placement of filter bar')}
-            icon={
-              <Icons.Gear name="gear" iconColor={theme.colors.grayscale.base} />
-            }
-            menuItems={[
-              {
-                key: 'vertical',
-                label: t('Vertical (Left)'),
-              },
-              {
-                key: 'horizontal',
-                label: t('Horizontal (Top)'),
-              },
-            ]}
-            selectedKeys={['vertical']}
-          />
-        )}
+        {canSetHorizontalFilterBar && <FilterBarLocationSelect />}
         <HeaderButton
           {...getFilterBarTestId('collapse-button')}
           buttonStyle="link"

superset-frontend/src/dashboard/reducers/dashboardInfo.js

@@ -17,7 +17,10 @@
  * under the License.
  */
 
-import { DASHBOARD_INFO_UPDATED } from '../actions/dashboardInfo';
+import {
+  DASHBOARD_INFO_UPDATED,
+  SET_FILTER_BAR_LOCATION,
+} from '../actions/dashboardInfo';
 import { HYDRATE_DASHBOARD } from '../actions/hydrate';
 
 export default function dashboardStateReducer(state = {}, action) {
@@ -35,6 +38,11 @@ export default function dashboardStateReducer(state = {}, action) {
         ...action.data.dashboardInfo,
         // set async api call data
       };
+    case SET_FILTER_BAR_LOCATION:
+      return {
+        ...state,
+        filterBarLocation: action.filterBarLocation,
+      };
     default:
       return state;
   }

superset-frontend/src/dashboard/types.ts

@@ -52,6 +52,11 @@ export type Chart = ChartState & {
   };
 };
 
+export enum FilterBarLocation {
+  VERTICAL = 'VERTICAL',
+  HORIZONTAL = 'HORIZONTAL',
+}
+
 export type ActiveTabs = string[];
 export type DashboardLayout = { [key: string]: LayoutItem };
 export type DashboardLayoutState = { present: DashboardLayout };
@@ -92,6 +97,7 @@ export type DashboardInfo = {
     label_colors: JsonObject;
     shared_label_colors: JsonObject;
   };
+  filterBarLocation: FilterBarLocation;
 };
 
 export type ChartsState = { [key: string]: Chart };

superset-frontend/src/dashboard/util/permissionUtils.test.ts

@@ -20,7 +20,7 @@ import {
   UndefinedUser,
   UserWithPermissionsAndRoles,
 } from 'src/types/bootstrapTypes';
-import Dashboard from 'src/types/Dashboard';
+import { Dashboard } from 'src/types/Dashboard';
 import Owner from 'src/types/Owner';
 import { canUserEditDashboard, isUserAdmin } from './permissionUtils';
 

superset-frontend/src/dashboard/util/permissionUtils.ts

@@ -21,7 +21,7 @@ import {
   UndefinedUser,
   UserWithPermissionsAndRoles,
 } from 'src/types/bootstrapTypes';
-import Dashboard from 'src/types/Dashboard';
+import { Dashboard } from 'src/types/Dashboard';
 import { findPermission } from 'src/utils/findPermission';
 
 // this should really be a config value,

superset-frontend/src/types/Dashboard.ts

@@ -36,5 +36,3 @@ export interface Dashboard {
   owners: Owner[];
   roles: Role[];
 }
-
-export default Dashboard;

superset/dashboards/schemas.py

@@ -133,6 +133,7 @@ class DashboardJSONMetadataSchema(Schema):
     # used for v0 import/export
     import_time = fields.Integer()
     remote_id = fields.Integer()
+    filter_bar_location = fields.Str(allow_none=True)
 
 
 class UserSchema(Schema):