[GitHub] [incubator-superset] ktmud commented on a change in pull request #11814: feat(dashboard): Dashboard-Native Filters

[GitBox <gi...@apache.org>]

ktmud commented on a change in pull request #11814:
URL: https://github.com/apache/incubator-superset/pull/11814#discussion_r538979639



##########
File path: superset-frontend/src/components/SupersetResourceSelect.tsx
##########
@@ -0,0 +1,108 @@
+/**
+ * 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, { useEffect } from 'react';
+import rison from 'rison';
+import { t, SupersetClient } from '@superset-ui/core';
+import { AsyncSelect } from 'src/components/Select';
+import { useToasts } from 'src/messageToasts/enhancers/withToasts';
+import getClientErrorObject from 'src/utils/getClientErrorObject';
+
+export type Value<V> = { value: V; label: string };
+
+export interface SupersetResourceSelectProps<T = unknown, V = string> {
+  value?: Value<V> | null;
+  initialId?: number | string;
+  onChange?: (value: Value<V>) => void;
+  isMulti?: boolean;
+  searchColumn?: string;
+  resource?: string; // e.g. "dataset", "dashboard/related/owners"
+  transformItem?: (item: T) => Value<V>;
+}
+
+/**
+ * This is a special-purpose select component for when you're selecting
+ * items from one of the standard Superset resource APIs.
+ * Such as selecting a datasource, a chart, or users.
+ *
+ * If you're selecting a "related" resource, leave the searchColumn prop unset.
+ * The api doesn't do columns on related resources for some reason.
+ *
+ * If you're doing anything more complex than selecting a standard resource,
+ * we'll all be better off if you use AsyncSelect directly instead.
+ */
+export default function SupersetResourceSelect<T = unknown, V = string>({
+  value,
+  initialId,
+  onChange,
+  isMulti,
+  resource,
+  searchColumn,
+  transformItem,
+}: SupersetResourceSelectProps<T, V>) {
+  const { addDangerToast } = useToasts();
+
+  useEffect(() => {
+    if (initialId == null) return;
+    SupersetClient.get({
+      endpoint: `/api/v1/${resource}/${initialId}`,
+    }).then(response => {
+      const { result } = response.json;
+      const value = transformItem ? transformItem(result) : result;
+      if (onChange) onChange(value);
+    });
+  }, [resource, initialId]); // eslint-disable-line react-hooks/exhaustive-deps
+
+  function loadOptions(input: string) {
+    const query = searchColumn
+      ? rison.encode({
+          filters: [{ col: searchColumn, opr: 'ct', value: input }],
+        })
+      : rison.encode({ filter: value });
+    return SupersetClient.get({
+      endpoint: `/api/v1/${resource}/?q=${query}`,
+    }).then(
+      response => {
+        return response.json.result
+          .map(transformItem)
+          .sort((a: Value<V>, b: Value<V>) => a.label.localeCompare(b.label));
+      },
+      async badResponse => {
+        const { error, message } = await getClientErrorObject(badResponse);
+        let errorText = message || error || t('An error has occurred');
+        if (message === 'Forbidden') {
+          errorText = t('You do not have permission to edit this dashboard');

Review comment:
       Will this component only be used in a dashboard? Maybe it's better to introduce an `onApiError` callback so the parent component can decide how to handle the errors? 

##########
File path: superset-frontend/src/dashboard/components/nativeFilters/types.ts
##########
@@ -0,0 +1,170 @@
+/**
+ * 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 { ExtraFormData, QueryObjectFilterClause } from '@superset-ui/core';
+import componentTypes from 'src/dashboard/util/componentTypes';
+
+export enum Scoping {
+  all,
+  specific,
+}
+
+interface NativeFiltersFormItem {
+  scoping: Scoping;
+  filterScope: Scope;
+  name: string;
+  dataset: {
+    value: number;
+    label: string;
+  };
+  column: string;
+  defaultValue: string;
+  inverseSelection: boolean;
+  isInstant: boolean;
+  allowsMultipleValues: boolean;
+  isRequired: boolean;
+}
+
+export interface NativeFiltersForm {
+  filters: Record<string, NativeFiltersFormItem>;
+}
+
+export interface Column {
+  name: string;
+  displayName?: string;
+}
+
+export interface Scope {
+  rootPath: string[];
+  excluded: number[];
+}
+
+/** The target of a filter is the datasource/column being filtered */
+export interface Target {
+  datasetId: number;
+  column: Column;
+
+  // maybe someday support this?
+  // show values from these columns in the filter options selector
+  // clarityColumns?: Column[];
+}
+
+export type FilterType = 'text' | 'date';
+
+/**
+ * This is a filter configuration object, stored in the dashboard's json metadata.
+ * The values here do not reflect the current state of the filter.
+ */
+export interface Filter {
+  allowsMultipleValues: boolean;
+  cascadeParentIds: string[];
+  defaultValue: string | null;
+  inverseSelection: boolean;
+  isInstant: boolean;
+  isRequired: boolean;
+  id: string; // randomly generated at filter creation
+  name: string;
+  scope: Scope;
+  type: FilterType;
+  // for now there will only ever be one target
+  // when multiple targets are supported, change this to Target[]
+  targets: [Target];
+}
+
+export type FilterConfiguration = Filter[];
+
+export type SelectedValues = string[] | null;
+
+/** Current state of the filter, stored in `nativeFilters` in redux */
+export type FilterState = {
+  id: string; // ties this filter state to the config object
+  optionsStatus: 'loading' | 'success' | 'fail';
+  options: string[] | null;
+  extraFormData?: ExtraFormData;
+  /**
+   * If the config changes, the current options/values may no longer be valid.
+   * isDirty indicates that state.
+   */
+  isDirty: boolean;
+};
+
+export type AllFilterState = {
+  column: Column;
+  datasetId: number;
+  datasource: string;
+  id: string;
+  selectedValues: SelectedValues;
+  filterClause?: QueryObjectFilterClause;
+};
+
+/** Chart state of redux */
+export type Chart = {
+  id: number;
+  slice_id: 2107;

Review comment:
       What is this magic number?

##########
File path: superset-frontend/src/dashboard/actions/nativeFilters.ts
##########
@@ -0,0 +1,126 @@
+/**
+ * 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 { ExtraFormData, SupersetClient } from '@superset-ui/core';
+import { Dispatch } from 'redux';
+import {
+  Filter,
+  FilterConfiguration,
+  SelectedValues,
+} from '../components/nativeFilters/types';
+import { dashboardInfoChanged } from './dashboardInfo';
+
+export const SET_FILTER_CONFIG_BEGIN = 'SET_FILTER_CONFIG_BEGIN';
+export interface SetFilterConfigBegin {
+  type: typeof SET_FILTER_CONFIG_BEGIN;
+  filterConfig: FilterConfiguration;
+}
+export const SET_FILTER_CONFIG_COMPLETE = 'SET_FILTER_CONFIG_COMPLETE';
+export interface SetFilterConfigComplete {
+  type: typeof SET_FILTER_CONFIG_COMPLETE;
+  filterConfig: FilterConfiguration;
+}
+export const SET_FILTER_CONFIG_FAIL = 'SET_FILTER_CONFIG_FAIL';
+export interface SetFilterConfigFail {
+  type: typeof SET_FILTER_CONFIG_FAIL;
+  filterConfig: FilterConfiguration;
+}
+
+export const SET_FILTER_STATE = 'SET_FILTER_STATE';
+export interface SetFilterState {
+  type: typeof SET_FILTER_STATE;
+  selectedValues: SelectedValues;
+  filter: Filter;
+  filters: FilterConfiguration;
+}
+
+export const setFilterConfiguration = (
+  filterConfig: FilterConfiguration,
+) => async (dispatch: Dispatch, getState: () => any) => {
+  dispatch({
+    type: SET_FILTER_CONFIG_BEGIN,
+    filterConfig,
+  });
+  const { id, metadata } = getState().dashboardInfo;
+  try {
+    const response = await SupersetClient.put({
+      endpoint: `/api/v1/dashboard/${id}`,
+      headers: { 'Content-Type': 'application/json' },
+      body: JSON.stringify({
+        json_metadata: JSON.stringify({
+          ...metadata,
+          filter_configuration: filterConfig,
+        }),
+      }),
+    });

Review comment:
       Could use `makeApi` so you don't have to manually pass the headers and parse the body.
   
   ```ts
   import { makeApi, JsonValue } from '@superset-ui/core';
   
   interface DashboardInfo {
     id: number;
     json_metadata: string;
     // ... other fields of the Dashboard API resource
   }
   
   const updateDashboard = makeApi<Partial<DashboardInfo>, { result: DashboardInfo }>({
     method: 'PUT',
     endpoint: `/api/v1/dashboard/${id}`
   });
   
   const { json_metadata } = await updatedFilterConfig({
     json_metadata: JSON.stringify({
       ...metadata,
       filter_configuration: filterConfig,
     }),
   });
   ```

##########
File path: superset-frontend/src/dashboard/components/nativeFilters/ColumnSelect.tsx
##########
@@ -0,0 +1,97 @@
+/**
+ * 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 } from 'react';
+import { FormInstance } from 'antd/lib/form';
+import { SupersetClient, t } from '@superset-ui/core';
+import { useChangeEffect } from 'src/common/hooks/useChangeEffect';
+import { AsyncSelect } from 'src/components/Select';
+import { useToasts } from 'src/messageToasts/enhancers/withToasts';
+import getClientErrorObject from 'src/utils/getClientErrorObject';
+import { NativeFiltersForm } from './types';
+
+type ColumnSelectValue = {
+  value: string;
+  label: string;
+};
+
+interface ColumnSelectProps {
+  form: FormInstance<NativeFiltersForm>;
+  filterId: string;
+  datasetId?: number | null | undefined;
+  value?: ColumnSelectValue | null;
+  onChange?: (value: ColumnSelectValue | null) => void;
+}
+
+/** Special purpose AsyncSelect that selects a column from a dataset */
+// eslint-disable-next-line import/prefer-default-export
+export function ColumnSelect({

Review comment:
       Any reason not to export this component as default?  If you just want to disable the ESLint warning, maybe export `ColumnSelectProps`, too? If people think this rule is just annoyance, we can disable it; but if not, let's stick to it as much as possible.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org